Patchwork [13/15] power: supply: olpc_battery: Move priv data to a struct

login
register
mail settings
Submitter Lubomir Rintel
Date Oct. 10, 2018, 5:22 p.m.
Message ID <20181010172300.317643-14-lkundrak@v3.sk>
Download mbox | patch
Permalink /patch/632581/
State New
Headers show

Comments

Lubomir Rintel - Oct. 10, 2018, 5:22 p.m.
The global variables for private data are not too nice. I'd like some
more, and that would clutter the global name space even further.

Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
---
 drivers/power/supply/olpc_battery.c | 73 +++++++++++++++--------------
 1 file changed, 38 insertions(+), 35 deletions(-)
Andy Shevchenko - Oct. 19, 2018, 1:48 p.m.
On Wed, Oct 10, 2018 at 8:24 PM Lubomir Rintel <lkundrak@v3.sk> wrote:
>
> The global variables for private data are not too nice. I'd like some
> more, and that would clutter the global name space even further.
>

Good change!
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

> Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
> ---
>  drivers/power/supply/olpc_battery.c | 73 +++++++++++++++--------------
>  1 file changed, 38 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/power/supply/olpc_battery.c b/drivers/power/supply/olpc_battery.c
> index 540d44bf536f..2a2d7cc995f0 100644
> --- a/drivers/power/supply/olpc_battery.c
> +++ b/drivers/power/supply/olpc_battery.c
> @@ -53,6 +53,12 @@
>
>  #define BAT_ADDR_MFR_TYPE      0x5F
>
> +struct olpc_battery_data {
> +       struct power_supply *olpc_ac;
> +       struct power_supply *olpc_bat;
> +       char bat_serial[17];
> +};
> +
>  /*********************************************************************
>   *             Power
>   *********************************************************************/
> @@ -91,11 +97,8 @@ static const struct power_supply_desc olpc_ac_desc = {
>         .get_property = olpc_ac_get_prop,
>  };
>
> -static struct power_supply *olpc_ac;
> -
> -static char bat_serial[17]; /* Ick */
> -
> -static int olpc_bat_get_status(union power_supply_propval *val, uint8_t ec_byte)
> +static int olpc_bat_get_status(struct olpc_battery_data *data,
> +               union power_supply_propval *val, uint8_t ec_byte)
>  {
>         if (olpc_platform_info.ecver > 0x44) {
>                 if (ec_byte & (BAT_STAT_CHARGING | BAT_STAT_TRICKLE))
> @@ -326,6 +329,7 @@ static int olpc_bat_get_property(struct power_supply *psy,
>                                  enum power_supply_property psp,
>                                  union power_supply_propval *val)
>  {
> +       struct olpc_battery_data *data = power_supply_get_drvdata(psy);
>         int ret = 0;
>         __be16 ec_word;
>         uint8_t ec_byte;
> @@ -347,7 +351,7 @@ static int olpc_bat_get_property(struct power_supply *psy,
>
>         switch (psp) {
>         case POWER_SUPPLY_PROP_STATUS:
> -               ret = olpc_bat_get_status(val, ec_byte);
> +               ret = olpc_bat_get_status(data, val, ec_byte);
>                 if (ret)
>                         return ret;
>                 break;
> @@ -450,8 +454,8 @@ static int olpc_bat_get_property(struct power_supply *psy,
>                 if (ret)
>                         return ret;
>
> -               sprintf(bat_serial, "%016llx", (long long)be64_to_cpu(ser_buf));
> -               val->strval = bat_serial;
> +               sprintf(data->bat_serial, "%016llx", (long long)be64_to_cpu(ser_buf));
> +               val->strval = data->bat_serial;
>                 break;
>         case POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN:
>                 ret = olpc_bat_get_voltage_max_design(val);
> @@ -579,17 +583,17 @@ static struct power_supply_desc olpc_bat_desc = {
>         .use_for_apm = 1,
>  };
>
> -static struct power_supply *olpc_bat;
> -
>  static int olpc_battery_suspend(struct platform_device *pdev,
>                                 pm_message_t state)
>  {
> -       if (device_may_wakeup(&olpc_ac->dev))
> +       struct olpc_battery_data *data = platform_get_drvdata(pdev);
> +
> +       if (device_may_wakeup(&data->olpc_ac->dev))
>                 olpc_ec_wakeup_set(EC_SCI_SRC_ACPWR);
>         else
>                 olpc_ec_wakeup_clear(EC_SCI_SRC_ACPWR);
>
> -       if (device_may_wakeup(&olpc_bat->dev))
> +       if (device_may_wakeup(&data->olpc_bat->dev))
>                 olpc_ec_wakeup_set(EC_SCI_SRC_BATTERY | EC_SCI_SRC_BATSOC
>                                    | EC_SCI_SRC_BATERR);
>         else
> @@ -601,7 +605,8 @@ static int olpc_battery_suspend(struct platform_device *pdev,
>
>  static int olpc_battery_probe(struct platform_device *pdev)
>  {
> -       int ret;
> +       struct power_supply_config psy_cfg = {};
> +       struct olpc_battery_data *data;
>         uint8_t status;
>
>         /*
> @@ -620,9 +625,13 @@ static int olpc_battery_probe(struct platform_device *pdev)
>
>         /* Ignore the status. It doesn't actually matter */
>
> -       olpc_ac = power_supply_register(&pdev->dev, &olpc_ac_desc, NULL);
> -       if (IS_ERR(olpc_ac))
> -               return PTR_ERR(olpc_ac);
> +       psy_cfg.of_node = pdev->dev.of_node;
> +       psy_cfg.drv_data = data;
> +
> +       data->olpc_ac = devm_power_supply_register(&pdev->dev, &olpc_ac_desc, &psy_cfg);
> +       if (IS_ERR(data->olpc_ac))
> +               return PTR_ERR(data->olpc_ac);
> +
>         if (of_property_match_string(pdev->dev.of_node, "compatible",
>                                         "olpc,xo1.5-battery") >= 0) {
>                 /* XO-1.5 */
> @@ -634,42 +643,36 @@ static int olpc_battery_probe(struct platform_device *pdev)
>                 olpc_bat_desc.num_properties = ARRAY_SIZE(olpc_xo1_bat_props);
>         }
>
> -       olpc_bat = power_supply_register(&pdev->dev, &olpc_bat_desc, NULL);
> -       if (IS_ERR(olpc_bat)) {
> -               ret = PTR_ERR(olpc_bat);
> -               goto battery_failed;
> -       }
> +       data->olpc_bat = devm_power_supply_register(&pdev->dev, &olpc_bat_desc, &psy_cfg);
> +       if (IS_ERR(data->olpc_bat))
> +               return PTR_ERR(data->olpc_bat);
>
> -       ret = device_create_bin_file(&olpc_bat->dev, &olpc_bat_eeprom);
> +       ret = device_create_bin_file(&data->olpc_bat->dev, &olpc_bat_eeprom);
>         if (ret)
> -               goto eeprom_failed;
> +               return ret;
>
> -       ret = device_create_file(&olpc_bat->dev, &olpc_bat_error);
> +       ret = device_create_file(&data->olpc_bat->dev, &olpc_bat_error);
>         if (ret)
>                 goto error_failed;
>
>         if (olpc_ec_wakeup_available()) {
> -               device_set_wakeup_capable(&olpc_ac->dev, true);
> -               device_set_wakeup_capable(&olpc_bat->dev, true);
> +               device_set_wakeup_capable(&data->olpc_ac->dev, true);
> +               device_set_wakeup_capable(&data->olpc_bat->dev, true);
>         }
>
>         return 0;
>
>  error_failed:
> -       device_remove_bin_file(&olpc_bat->dev, &olpc_bat_eeprom);
> -eeprom_failed:
> -       power_supply_unregister(olpc_bat);
> -battery_failed:
> -       power_supply_unregister(olpc_ac);
> +       device_remove_bin_file(&data->olpc_bat->dev, &olpc_bat_eeprom);
>         return ret;
>  }
>
>  static int olpc_battery_remove(struct platform_device *pdev)
>  {
> -       device_remove_file(&olpc_bat->dev, &olpc_bat_error);
> -       device_remove_bin_file(&olpc_bat->dev, &olpc_bat_eeprom);
> -       power_supply_unregister(olpc_bat);
> -       power_supply_unregister(olpc_ac);
> +       struct olpc_battery_data *data = platform_get_drvdata(pdev);
> +
> +       device_remove_file(&data->olpc_bat->dev, &olpc_bat_error);
> +       device_remove_bin_file(&data->olpc_bat->dev, &olpc_bat_eeprom);
>         return 0;
>  }
>
> --
> 2.19.0
>
Pavel Machek - Nov. 4, 2018, 2:37 p.m.
Hi!

> The global variables for private data are not too nice. I'd like some
> more, and that would clutter the global name space even further.
> 
> Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

Ok...

> -	olpc_bat = power_supply_register(&pdev->dev, &olpc_bat_desc, NULL);
> -	if (IS_ERR(olpc_bat)) {
> -		ret = PTR_ERR(olpc_bat);
> -		goto battery_failed;
> -	}
> +	data->olpc_bat = devm_power_supply_register(&pdev->dev, &olpc_bat_desc, &psy_cfg);
> +	if (IS_ERR(data->olpc_bat))
> +		return PTR_ERR(data->olpc_bat);
>  
> -	ret = device_create_bin_file(&olpc_bat->dev, &olpc_bat_eeprom);
> +	ret = device_create_bin_file(&data->olpc_bat->dev, &olpc_bat_eeprom);
>  	if (ret)
> -		goto eeprom_failed;
> +		return ret;
>  
> -	ret = device_create_file(&olpc_bat->dev, &olpc_bat_error);
> +	ret = device_create_file(&data->olpc_bat->dev, &olpc_bat_error);
>  	if (ret)
>  		goto error_failed;
>  
>  	if (olpc_ec_wakeup_available()) {
> -		device_set_wakeup_capable(&olpc_ac->dev, true);
> -		device_set_wakeup_capable(&olpc_bat->dev, true);
> +		device_set_wakeup_capable(&data->olpc_ac->dev, true);
> +		device_set_wakeup_capable(&data->olpc_bat->dev, true);
>  	}
>  
>  	return 0;
>  
>  error_failed:
> -	device_remove_bin_file(&olpc_bat->dev, &olpc_bat_eeprom);
> -eeprom_failed:
> -	power_supply_unregister(olpc_bat);
> -battery_failed:
> -	power_supply_unregister(olpc_ac);
> +	device_remove_bin_file(&data->olpc_bat->dev, &olpc_bat_eeprom);
>  	return ret;
>  }

...but you are changing error handling here, which is not mentioned in
the changelog, and I'm nut sure you got it right.

Are you sure?

>  static int olpc_battery_remove(struct platform_device *pdev)
>  {
> -	device_remove_file(&olpc_bat->dev, &olpc_bat_error);
> -	device_remove_bin_file(&olpc_bat->dev, &olpc_bat_eeprom);
> -	power_supply_unregister(olpc_bat);
> -	power_supply_unregister(olpc_ac);
> +	struct olpc_battery_data *data = platform_get_drvdata(pdev);
> +
> +	device_remove_file(&data->olpc_bat->dev, &olpc_bat_error);
> +	device_remove_bin_file(&data->olpc_bat->dev, &olpc_bat_eeprom);
>  	return 0;
>  }

Here too.
									Pavel
Lubomir Rintel - Nov. 14, 2018, 5:10 p.m.
On Sun, 2018-11-04 at 15:37 +0100, Pavel Machek wrote:
> Hi!
> 
> > The global variables for private data are not too nice. I'd like some
> > more, and that would clutter the global name space even further.
> > 
> > Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
> > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> 
> Ok...
> 
> > -	olpc_bat = power_supply_register(&pdev->dev, &olpc_bat_desc, NULL);
> > -	if (IS_ERR(olpc_bat)) {
> > -		ret = PTR_ERR(olpc_bat);
> > -		goto battery_failed;
> > -	}
> > +	data->olpc_bat = devm_power_supply_register(&pdev->dev, &olpc_bat_desc, &psy_cfg);
> > +	if (IS_ERR(data->olpc_bat))
> > +		return PTR_ERR(data->olpc_bat);
> >  
> > -	ret = device_create_bin_file(&olpc_bat->dev, &olpc_bat_eeprom);
> > +	ret = device_create_bin_file(&data->olpc_bat->dev, &olpc_bat_eeprom);
> >  	if (ret)
> > -		goto eeprom_failed;
> > +		return ret;
> >  
> > -	ret = device_create_file(&olpc_bat->dev, &olpc_bat_error);
> > +	ret = device_create_file(&data->olpc_bat->dev, &olpc_bat_error);
> >  	if (ret)
> >  		goto error_failed;
> >  
> >  	if (olpc_ec_wakeup_available()) {
> > -		device_set_wakeup_capable(&olpc_ac->dev, true);
> > -		device_set_wakeup_capable(&olpc_bat->dev, true);
> > +		device_set_wakeup_capable(&data->olpc_ac->dev, true);
> > +		device_set_wakeup_capable(&data->olpc_bat->dev, true);
> >  	}
> >  
> >  	return 0;
> >  
> >  error_failed:
> > -	device_remove_bin_file(&olpc_bat->dev, &olpc_bat_eeprom);
> > -eeprom_failed:
> > -	power_supply_unregister(olpc_bat);
> > -battery_failed:
> > -	power_supply_unregister(olpc_ac);
> > +	device_remove_bin_file(&data->olpc_bat->dev, &olpc_bat_eeprom);
> >  	return ret;
> >  }
> 
> ...but you are changing error handling here, which is not mentioned in
> the changelog, and I'm nut sure you got it right.
> 
> Are you sure?

I can't see what's wrong. I'll split the priv structure and devm/error
handling changes into two separate patches as you're right they indeed
are somewhat unrelated.

If v2 (tomorrow or so) will still seem wrong to you I'd be thankful if
you could elaborate a bit more.
> 
> >  static int olpc_battery_remove(struct platform_device *pdev)
> >  {
> > -	device_remove_file(&olpc_bat->dev, &olpc_bat_error);
> > -	device_remove_bin_file(&olpc_bat->dev, &olpc_bat_eeprom);
> > -	power_supply_unregister(olpc_bat);
> > -	power_supply_unregister(olpc_ac);
> > +	struct olpc_battery_data *data = platform_get_drvdata(pdev);
> > +
> > +	device_remove_file(&data->olpc_bat->dev, &olpc_bat_error);
> > +	device_remove_bin_file(&data->olpc_bat->dev, &olpc_bat_eeprom);
> >  	return 0;
> >  }
> 
> Here too.
> 									Pavel

Cheers
Lubo

Patch

diff --git a/drivers/power/supply/olpc_battery.c b/drivers/power/supply/olpc_battery.c
index 540d44bf536f..2a2d7cc995f0 100644
--- a/drivers/power/supply/olpc_battery.c
+++ b/drivers/power/supply/olpc_battery.c
@@ -53,6 +53,12 @@ 
 
 #define BAT_ADDR_MFR_TYPE	0x5F
 
+struct olpc_battery_data {
+	struct power_supply *olpc_ac;
+	struct power_supply *olpc_bat;
+	char bat_serial[17];
+};
+
 /*********************************************************************
  *		Power
  *********************************************************************/
@@ -91,11 +97,8 @@  static const struct power_supply_desc olpc_ac_desc = {
 	.get_property = olpc_ac_get_prop,
 };
 
-static struct power_supply *olpc_ac;
-
-static char bat_serial[17]; /* Ick */
-
-static int olpc_bat_get_status(union power_supply_propval *val, uint8_t ec_byte)
+static int olpc_bat_get_status(struct olpc_battery_data *data,
+		union power_supply_propval *val, uint8_t ec_byte)
 {
 	if (olpc_platform_info.ecver > 0x44) {
 		if (ec_byte & (BAT_STAT_CHARGING | BAT_STAT_TRICKLE))
@@ -326,6 +329,7 @@  static int olpc_bat_get_property(struct power_supply *psy,
 				 enum power_supply_property psp,
 				 union power_supply_propval *val)
 {
+	struct olpc_battery_data *data = power_supply_get_drvdata(psy);
 	int ret = 0;
 	__be16 ec_word;
 	uint8_t ec_byte;
@@ -347,7 +351,7 @@  static int olpc_bat_get_property(struct power_supply *psy,
 
 	switch (psp) {
 	case POWER_SUPPLY_PROP_STATUS:
-		ret = olpc_bat_get_status(val, ec_byte);
+		ret = olpc_bat_get_status(data, val, ec_byte);
 		if (ret)
 			return ret;
 		break;
@@ -450,8 +454,8 @@  static int olpc_bat_get_property(struct power_supply *psy,
 		if (ret)
 			return ret;
 
-		sprintf(bat_serial, "%016llx", (long long)be64_to_cpu(ser_buf));
-		val->strval = bat_serial;
+		sprintf(data->bat_serial, "%016llx", (long long)be64_to_cpu(ser_buf));
+		val->strval = data->bat_serial;
 		break;
 	case POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN:
 		ret = olpc_bat_get_voltage_max_design(val);
@@ -579,17 +583,17 @@  static struct power_supply_desc olpc_bat_desc = {
 	.use_for_apm = 1,
 };
 
-static struct power_supply *olpc_bat;
-
 static int olpc_battery_suspend(struct platform_device *pdev,
 				pm_message_t state)
 {
-	if (device_may_wakeup(&olpc_ac->dev))
+	struct olpc_battery_data *data = platform_get_drvdata(pdev);
+
+	if (device_may_wakeup(&data->olpc_ac->dev))
 		olpc_ec_wakeup_set(EC_SCI_SRC_ACPWR);
 	else
 		olpc_ec_wakeup_clear(EC_SCI_SRC_ACPWR);
 
-	if (device_may_wakeup(&olpc_bat->dev))
+	if (device_may_wakeup(&data->olpc_bat->dev))
 		olpc_ec_wakeup_set(EC_SCI_SRC_BATTERY | EC_SCI_SRC_BATSOC
 				   | EC_SCI_SRC_BATERR);
 	else
@@ -601,7 +605,8 @@  static int olpc_battery_suspend(struct platform_device *pdev,
 
 static int olpc_battery_probe(struct platform_device *pdev)
 {
-	int ret;
+	struct power_supply_config psy_cfg = {};
+	struct olpc_battery_data *data;
 	uint8_t status;
 
 	/*
@@ -620,9 +625,13 @@  static int olpc_battery_probe(struct platform_device *pdev)
 
 	/* Ignore the status. It doesn't actually matter */
 
-	olpc_ac = power_supply_register(&pdev->dev, &olpc_ac_desc, NULL);
-	if (IS_ERR(olpc_ac))
-		return PTR_ERR(olpc_ac);
+	psy_cfg.of_node = pdev->dev.of_node;
+	psy_cfg.drv_data = data;
+
+	data->olpc_ac = devm_power_supply_register(&pdev->dev, &olpc_ac_desc, &psy_cfg);
+	if (IS_ERR(data->olpc_ac))
+		return PTR_ERR(data->olpc_ac);
+
 	if (of_property_match_string(pdev->dev.of_node, "compatible",
 					"olpc,xo1.5-battery") >= 0) {
 		/* XO-1.5 */
@@ -634,42 +643,36 @@  static int olpc_battery_probe(struct platform_device *pdev)
 		olpc_bat_desc.num_properties = ARRAY_SIZE(olpc_xo1_bat_props);
 	}
 
-	olpc_bat = power_supply_register(&pdev->dev, &olpc_bat_desc, NULL);
-	if (IS_ERR(olpc_bat)) {
-		ret = PTR_ERR(olpc_bat);
-		goto battery_failed;
-	}
+	data->olpc_bat = devm_power_supply_register(&pdev->dev, &olpc_bat_desc, &psy_cfg);
+	if (IS_ERR(data->olpc_bat))
+		return PTR_ERR(data->olpc_bat);
 
-	ret = device_create_bin_file(&olpc_bat->dev, &olpc_bat_eeprom);
+	ret = device_create_bin_file(&data->olpc_bat->dev, &olpc_bat_eeprom);
 	if (ret)
-		goto eeprom_failed;
+		return ret;
 
-	ret = device_create_file(&olpc_bat->dev, &olpc_bat_error);
+	ret = device_create_file(&data->olpc_bat->dev, &olpc_bat_error);
 	if (ret)
 		goto error_failed;
 
 	if (olpc_ec_wakeup_available()) {
-		device_set_wakeup_capable(&olpc_ac->dev, true);
-		device_set_wakeup_capable(&olpc_bat->dev, true);
+		device_set_wakeup_capable(&data->olpc_ac->dev, true);
+		device_set_wakeup_capable(&data->olpc_bat->dev, true);
 	}
 
 	return 0;
 
 error_failed:
-	device_remove_bin_file(&olpc_bat->dev, &olpc_bat_eeprom);
-eeprom_failed:
-	power_supply_unregister(olpc_bat);
-battery_failed:
-	power_supply_unregister(olpc_ac);
+	device_remove_bin_file(&data->olpc_bat->dev, &olpc_bat_eeprom);
 	return ret;
 }
 
 static int olpc_battery_remove(struct platform_device *pdev)
 {
-	device_remove_file(&olpc_bat->dev, &olpc_bat_error);
-	device_remove_bin_file(&olpc_bat->dev, &olpc_bat_eeprom);
-	power_supply_unregister(olpc_bat);
-	power_supply_unregister(olpc_ac);
+	struct olpc_battery_data *data = platform_get_drvdata(pdev);
+
+	device_remove_file(&data->olpc_bat->dev, &olpc_bat_error);
+	device_remove_bin_file(&data->olpc_bat->dev, &olpc_bat_eeprom);
 	return 0;
 }