* [PATCH] power: max77693_charger: Better sysfs creation and using devm APIs
@ 2016-12-09 8:51 Srikant Ritolia
2016-12-10 7:48 ` Krzysztof Kozlowski
0 siblings, 1 reply; 6+ messages in thread
From: Srikant Ritolia @ 2016-12-09 8:51 UTC (permalink / raw)
To: Krzysztof Kozlowski, Bartlomiej Zolnierkiewicz, Chanwoo Choi,
Sebastian Reichel, Srikant Ritolia
Cc: linux-pm, d.wadhawan, vidushi.koul, srikant.ritolia
Creating sysfs group instead of creating each attribute one by one
and then handling its remove and error condition.
Also using manged resource function devm_power_supply_register API
instead of power_supply_register which automatically does
unregister on error and remove.
Signed-off-by: Srikant Ritolia <s.ritolia@samsung.com>
---
drivers/power/supply/max77693_charger.c | 51 +++++++++++--------------------
1 file changed, 18 insertions(+), 33 deletions(-)
diff --git a/drivers/power/supply/max77693_charger.c b/drivers/power/supply/max77693_charger.c
index 6c78884..f8f7188 100644
--- a/drivers/power/supply/max77693_charger.c
+++ b/drivers/power/supply/max77693_charger.c
@@ -445,6 +445,17 @@ static ssize_t top_off_timer_store(struct device *dev,
static DEVICE_ATTR_RW(top_off_threshold_current);
static DEVICE_ATTR_RW(top_off_timer);
+static struct attribute *max77693_charger_attribute[] = {
+ &dev_attr_fast_charge_timer.attr,
+ &dev_attr_top_off_threshold_current.attr,
+ &dev_attr_top_off_timer.attr,
+ NULL,
+};
+
+static const struct attribute_group max77693_attr_group = {
+ .attrs = max77693_charger_attribute,
+};
+
static int max77693_set_constant_volt(struct max77693_charger *chg,
unsigned int uvolt)
{
@@ -699,53 +710,27 @@ static int max77693_charger_probe(struct platform_device *pdev)
psy_cfg.drv_data = chg;
- ret = device_create_file(&pdev->dev, &dev_attr_fast_charge_timer);
- if (ret) {
- dev_err(&pdev->dev, "failed: create fast charge timer sysfs entry\n");
- goto err;
- }
-
- ret = device_create_file(&pdev->dev,
- &dev_attr_top_off_threshold_current);
- if (ret) {
- dev_err(&pdev->dev, "failed: create top off current sysfs entry\n");
- goto err;
- }
-
- ret = device_create_file(&pdev->dev, &dev_attr_top_off_timer);
- if (ret) {
- dev_err(&pdev->dev, "failed: create top off timer sysfs entry\n");
- goto err;
+ ret = sysfs_create_group(&pdev->dev.kobj, &max77693_attr_group);
+ if (ret != 0) {
+ dev_err(&pdev->dev, "Can't create sysfs entries\n");
+ return ret;
}
- chg->charger = power_supply_register(&pdev->dev,
+ chg->charger = devm_power_supply_register(&pdev->dev,
&max77693_charger_desc,
&psy_cfg);
if (IS_ERR(chg->charger)) {
dev_err(&pdev->dev, "failed: power supply register\n");
ret = PTR_ERR(chg->charger);
- goto err;
+ return ret;
}
return 0;
-
-err:
- device_remove_file(&pdev->dev, &dev_attr_top_off_timer);
- device_remove_file(&pdev->dev, &dev_attr_top_off_threshold_current);
- device_remove_file(&pdev->dev, &dev_attr_fast_charge_timer);
-
- return ret;
}
static int max77693_charger_remove(struct platform_device *pdev)
{
- struct max77693_charger *chg = platform_get_drvdata(pdev);
-
- device_remove_file(&pdev->dev, &dev_attr_top_off_timer);
- device_remove_file(&pdev->dev, &dev_attr_top_off_threshold_current);
- device_remove_file(&pdev->dev, &dev_attr_fast_charge_timer);
-
- power_supply_unregister(chg->charger);
+ sysfs_remove_group(&pdev->dev.kobj, &max77693_attr_group);
return 0;
}
--
1.7.9.5
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] power: max77693_charger: Better sysfs creation and using devm APIs
2016-12-09 8:51 [PATCH] power: max77693_charger: Better sysfs creation and using devm APIs Srikant Ritolia
@ 2016-12-10 7:48 ` Krzysztof Kozlowski
2016-12-10 14:19 ` Srikant Ritolia
0 siblings, 1 reply; 6+ messages in thread
From: Krzysztof Kozlowski @ 2016-12-10 7:48 UTC (permalink / raw)
To: Srikant Ritolia
Cc: Krzysztof Kozlowski, Bartlomiej Zolnierkiewicz, Chanwoo Choi,
Sebastian Reichel, linux-pm, d.wadhawan, vidushi.koul,
srikant.ritolia
On Fri, Dec 09, 2016 at 02:21:38PM +0530, Srikant Ritolia wrote:
> Creating sysfs group instead of creating each attribute one by one
> and then handling its remove and error condition.
>
> Also using manged resource function devm_power_supply_register API
> instead of power_supply_register which automatically does
> unregister on error and remove.
>
> Signed-off-by: Srikant Ritolia <s.ritolia@samsung.com>
> ---
> drivers/power/supply/max77693_charger.c | 51 +++++++++++--------------------
> 1 file changed, 18 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/power/supply/max77693_charger.c b/drivers/power/supply/max77693_charger.c
> index 6c78884..f8f7188 100644
> --- a/drivers/power/supply/max77693_charger.c
> +++ b/drivers/power/supply/max77693_charger.c
> @@ -445,6 +445,17 @@ static ssize_t top_off_timer_store(struct device *dev,
> static DEVICE_ATTR_RW(top_off_threshold_current);
> static DEVICE_ATTR_RW(top_off_timer);
>
> +static struct attribute *max77693_charger_attribute[] = {
> + &dev_attr_fast_charge_timer.attr,
> + &dev_attr_top_off_threshold_current.attr,
> + &dev_attr_top_off_timer.attr,
> + NULL,
> +};
> +
> +static const struct attribute_group max77693_attr_group = {
> + .attrs = max77693_charger_attribute,
> +};
> +
> static int max77693_set_constant_volt(struct max77693_charger *chg,
> unsigned int uvolt)
> {
> @@ -699,53 +710,27 @@ static int max77693_charger_probe(struct platform_device *pdev)
>
> psy_cfg.drv_data = chg;
>
> - ret = device_create_file(&pdev->dev, &dev_attr_fast_charge_timer);
> - if (ret) {
> - dev_err(&pdev->dev, "failed: create fast charge timer sysfs entry\n");
> - goto err;
> - }
> -
> - ret = device_create_file(&pdev->dev,
> - &dev_attr_top_off_threshold_current);
> - if (ret) {
> - dev_err(&pdev->dev, "failed: create top off current sysfs entry\n");
> - goto err;
> - }
> -
> - ret = device_create_file(&pdev->dev, &dev_attr_top_off_timer);
> - if (ret) {
> - dev_err(&pdev->dev, "failed: create top off timer sysfs entry\n");
> - goto err;
> + ret = sysfs_create_group(&pdev->dev.kobj, &max77693_attr_group);
> + if (ret != 0) {
> + dev_err(&pdev->dev, "Can't create sysfs entries\n");
> + return ret;
> }
>
> - chg->charger = power_supply_register(&pdev->dev,
> + chg->charger = devm_power_supply_register(&pdev->dev,
I would prefer splitting this into separate patche. You are altering
the order of cleanup in unbind. Previously power supply was unregistered
before sysfs, now it will be after. I don't see a problem with that but
these as separate ideas with different possible outcomes.
> &max77693_charger_desc,
> &psy_cfg);
> if (IS_ERR(chg->charger)) {
> dev_err(&pdev->dev, "failed: power supply register\n");
> ret = PTR_ERR(chg->charger);
> - goto err;
Missing sysfs cleanup.
Best regards,
Krzysztof
> + return ret;
> }
>
> return 0;
> -
> -err:
> - device_remove_file(&pdev->dev, &dev_attr_top_off_timer);
> - device_remove_file(&pdev->dev, &dev_attr_top_off_threshold_current);
> - device_remove_file(&pdev->dev, &dev_attr_fast_charge_timer);
> -
> - return ret;
> }
>
> static int max77693_charger_remove(struct platform_device *pdev)
> {
> - struct max77693_charger *chg = platform_get_drvdata(pdev);
> -
> - device_remove_file(&pdev->dev, &dev_attr_top_off_timer);
> - device_remove_file(&pdev->dev, &dev_attr_top_off_threshold_current);
> - device_remove_file(&pdev->dev, &dev_attr_fast_charge_timer);
> -
> - power_supply_unregister(chg->charger);
> + sysfs_remove_group(&pdev->dev.kobj, &max77693_attr_group);
>
> return 0;
> }
> --
> 1.7.9.5
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] power: max77693_charger: Better sysfs creation and using devm APIs
2016-12-10 7:48 ` Krzysztof Kozlowski
@ 2016-12-10 14:19 ` Srikant Ritolia
2016-12-10 19:56 ` Krzysztof Kozlowski
0 siblings, 1 reply; 6+ messages in thread
From: Srikant Ritolia @ 2016-12-10 14:19 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Srikant Ritolia, Bartlomiej Zolnierkiewicz, Chanwoo Choi,
Sebastian Reichel, linux-pm, d.wadhawan, vidushi.koul
On Sat, Dec 10, 2016 at 1:18 PM, Krzysztof Kozlowski <krzk@kernel.org> wrote:
> On Fri, Dec 09, 2016 at 02:21:38PM +0530, Srikant Ritolia wrote:
>> Creating sysfs group instead of creating each attribute one by one
>> and then handling its remove and error condition.
>>
>> Also using manged resource function devm_power_supply_register API
>> instead of power_supply_register which automatically does
>> unregister on error and remove.
>>
>> Signed-off-by: Srikant Ritolia <s.ritolia@samsung.com>
>> ---
>> drivers/power/supply/max77693_charger.c | 51 +++++++++++--------------------
>> 1 file changed, 18 insertions(+), 33 deletions(-)
>>
>> diff --git a/drivers/power/supply/max77693_charger.c b/drivers/power/supply/max77693_charger.c
>> index 6c78884..f8f7188 100644
>> --- a/drivers/power/supply/max77693_charger.c
>> +++ b/drivers/power/supply/max77693_charger.c
>> @@ -445,6 +445,17 @@ static ssize_t top_off_timer_store(struct device *dev,
>> static DEVICE_ATTR_RW(top_off_threshold_current);
>> static DEVICE_ATTR_RW(top_off_timer);
>>
>> +static struct attribute *max77693_charger_attribute[] = {
>> + &dev_attr_fast_charge_timer.attr,
>> + &dev_attr_top_off_threshold_current.attr,
>> + &dev_attr_top_off_timer.attr,
>> + NULL,
>> +};
>> +
>> +static const struct attribute_group max77693_attr_group = {
>> + .attrs = max77693_charger_attribute,
>> +};
>> +
>> static int max77693_set_constant_volt(struct max77693_charger *chg,
>> unsigned int uvolt)
>> {
>> @@ -699,53 +710,27 @@ static int max77693_charger_probe(struct platform_device *pdev)
>>
>> psy_cfg.drv_data = chg;
>>
>> - ret = device_create_file(&pdev->dev, &dev_attr_fast_charge_timer);
>> - if (ret) {
>> - dev_err(&pdev->dev, "failed: create fast charge timer sysfs entry\n");
>> - goto err;
>> - }
>> -
>> - ret = device_create_file(&pdev->dev,
>> - &dev_attr_top_off_threshold_current);
>> - if (ret) {
>> - dev_err(&pdev->dev, "failed: create top off current sysfs entry\n");
>> - goto err;
>> - }
>> -
>> - ret = device_create_file(&pdev->dev, &dev_attr_top_off_timer);
>> - if (ret) {
>> - dev_err(&pdev->dev, "failed: create top off timer sysfs entry\n");
>> - goto err;
>> + ret = sysfs_create_group(&pdev->dev.kobj, &max77693_attr_group);
>> + if (ret != 0) {
>> + dev_err(&pdev->dev, "Can't create sysfs entries\n");
>> + return ret;
>> }
>>
>> - chg->charger = power_supply_register(&pdev->dev,
>> + chg->charger = devm_power_supply_register(&pdev->dev,
>
> I would prefer splitting this into separate patche. You are altering
> the order of cleanup in unbind. Previously power supply was unregistered
> before sysfs, now it will be after. I don't see a problem with that but
> these as separate ideas with different possible outcomes.
Ok. I will send 2 separate patches for this.
>
>> &max77693_charger_desc,
>> &psy_cfg);
>> if (IS_ERR(chg->charger)) {
>> dev_err(&pdev->dev, "failed: power supply register\n");
>> ret = PTR_ERR(chg->charger);
>> - goto err;
>
> Missing sysfs cleanup.
>
> Best regards,
> Krzysztof
>
Thanks for pointing this out.
To overcome this I will use sysfs_create_group after devm_power_supply_register.
Then I would not need to do this sysfs cleanup on failure of power
supply register.
Thanks & Regards,
Srikant Ritolia
>> + return ret;
>> }
>>
>> return 0;
>> -
>> -err:
>> - device_remove_file(&pdev->dev, &dev_attr_top_off_timer);
>> - device_remove_file(&pdev->dev, &dev_attr_top_off_threshold_current);
>> - device_remove_file(&pdev->dev, &dev_attr_fast_charge_timer);
>> -
>> - return ret;
>> }
>>
>> static int max77693_charger_remove(struct platform_device *pdev)
>> {
>> - struct max77693_charger *chg = platform_get_drvdata(pdev);
>> -
>> - device_remove_file(&pdev->dev, &dev_attr_top_off_timer);
>> - device_remove_file(&pdev->dev, &dev_attr_top_off_threshold_current);
>> - device_remove_file(&pdev->dev, &dev_attr_fast_charge_timer);
>> -
>> - power_supply_unregister(chg->charger);
>> + sysfs_remove_group(&pdev->dev.kobj, &max77693_attr_group);
>>
>> return 0;
>> }
>> --
>> 1.7.9.5
>>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] power: max77693_charger: Better sysfs creation and using devm APIs
2016-12-10 14:19 ` Srikant Ritolia
@ 2016-12-10 19:56 ` Krzysztof Kozlowski
2016-12-17 16:04 ` Sebastian Reichel
0 siblings, 1 reply; 6+ messages in thread
From: Krzysztof Kozlowski @ 2016-12-10 19:56 UTC (permalink / raw)
To: Srikant Ritolia
Cc: Krzysztof Kozlowski, Srikant Ritolia, Bartlomiej Zolnierkiewicz,
Chanwoo Choi, Sebastian Reichel, linux-pm, d.wadhawan,
vidushi.koul
On Sat, Dec 10, 2016 at 07:49:25PM +0530, Srikant Ritolia wrote:
> On Sat, Dec 10, 2016 at 1:18 PM, Krzysztof Kozlowski <krzk@kernel.org> wrote:
> > On Fri, Dec 09, 2016 at 02:21:38PM +0530, Srikant Ritolia wrote:
>
> >> &max77693_charger_desc,
> >> &psy_cfg);
> >> if (IS_ERR(chg->charger)) {
> >> dev_err(&pdev->dev, "failed: power supply register\n");
> >> ret = PTR_ERR(chg->charger);
> >> - goto err;
> >
> > Missing sysfs cleanup.
> >
> > Best regards,
> > Krzysztof
> >
>
> Thanks for pointing this out.
> To overcome this I will use sysfs_create_group after devm_power_supply_register.
> Then I would not need to do this sysfs cleanup on failure of power
> supply register.
I am not sure if this is good idea. This patch does not bring any
particular noticeable benefit except less lines of code. It is not worth
breaking things just for that reason...
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] power: max77693_charger: Better sysfs creation and using devm APIs
2016-12-10 19:56 ` Krzysztof Kozlowski
@ 2016-12-17 16:04 ` Sebastian Reichel
2016-12-17 16:28 ` Krzysztof Kozlowski
0 siblings, 1 reply; 6+ messages in thread
From: Sebastian Reichel @ 2016-12-17 16:04 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Srikant Ritolia, Srikant Ritolia, Bartlomiej Zolnierkiewicz,
Chanwoo Choi, linux-pm, d.wadhawan, vidushi.koul
[-- Attachment #1: Type: text/plain, Size: 1362 bytes --]
Hi,
On Sat, Dec 10, 2016 at 09:56:27PM +0200, Krzysztof Kozlowski wrote:
> On Sat, Dec 10, 2016 at 07:49:25PM +0530, Srikant Ritolia wrote:
> > On Sat, Dec 10, 2016 at 1:18 PM, Krzysztof Kozlowski <krzk@kernel.org> wrote:
> > > On Fri, Dec 09, 2016 at 02:21:38PM +0530, Srikant Ritolia wrote:
> >
> > >> &max77693_charger_desc,
> > >> &psy_cfg);
> > >> if (IS_ERR(chg->charger)) {
> > >> dev_err(&pdev->dev, "failed: power supply register\n");
> > >> ret = PTR_ERR(chg->charger);
> > >> - goto err;
> > >
> > > Missing sysfs cleanup.
> > >
> > > Best regards,
> > > Krzysztof
> > >
> >
> > Thanks for pointing this out.
> > To overcome this I will use sysfs_create_group after devm_power_supply_register.
> > Then I would not need to do this sysfs cleanup on failure of power
> > supply register.
>
> I am not sure if this is good idea. This patch does not bring any
> particular noticeable benefit except less lines of code. It is not worth
> breaking things just for that reason...
I like less lines of code. How does the changed registration order
break anything? The changed registration order makes sense anyways,
since it then matches the (reversed) removal order.
-- Sebastian
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] power: max77693_charger: Better sysfs creation and using devm APIs
2016-12-17 16:04 ` Sebastian Reichel
@ 2016-12-17 16:28 ` Krzysztof Kozlowski
0 siblings, 0 replies; 6+ messages in thread
From: Krzysztof Kozlowski @ 2016-12-17 16:28 UTC (permalink / raw)
To: Sebastian Reichel
Cc: Krzysztof Kozlowski, Srikant Ritolia, Srikant Ritolia,
Bartlomiej Zolnierkiewicz, Chanwoo Choi, linux-pm, d.wadhawan,
vidushi.koul
On Sat, Dec 17, 2016 at 05:04:46PM +0100, Sebastian Reichel wrote:
> Hi,
>
> On Sat, Dec 10, 2016 at 09:56:27PM +0200, Krzysztof Kozlowski wrote:
> > On Sat, Dec 10, 2016 at 07:49:25PM +0530, Srikant Ritolia wrote:
> > > On Sat, Dec 10, 2016 at 1:18 PM, Krzysztof Kozlowski <krzk@kernel.org> wrote:
> > > > On Fri, Dec 09, 2016 at 02:21:38PM +0530, Srikant Ritolia wrote:
> > >
> > > >> &max77693_charger_desc,
> > > >> &psy_cfg);
> > > >> if (IS_ERR(chg->charger)) {
> > > >> dev_err(&pdev->dev, "failed: power supply register\n");
> > > >> ret = PTR_ERR(chg->charger);
> > > >> - goto err;
> > > >
> > > > Missing sysfs cleanup.
> > > >
> > > > Best regards,
> > > > Krzysztof
> > > >
> > >
> > > Thanks for pointing this out.
> > > To overcome this I will use sysfs_create_group after devm_power_supply_register.
> > > Then I would not need to do this sysfs cleanup on failure of power
> > > supply register.
> >
> > I am not sure if this is good idea. This patch does not bring any
> > particular noticeable benefit except less lines of code. It is not worth
> > breaking things just for that reason...
>
> I like less lines of code. How does the changed registration order
> break anything? The changed registration order makes sense anyways,
> since it then matches the (reversed) removal order.
By broken things I meant possible errors introduced with devm
conversion (like that one spotted above). To me personally, converting
to devm on its own mostly does not bring benefits except few cases when
a lot of code disappears. On the other hand it hides the order of
cleanup making it slightly more difficult to review. Overall - not many
benefits, some things hidden.
However I understand that this is highly subjective so I am not against
if others like this approach.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-12-17 16:28 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-12-09 8:51 [PATCH] power: max77693_charger: Better sysfs creation and using devm APIs Srikant Ritolia
2016-12-10 7:48 ` Krzysztof Kozlowski
2016-12-10 14:19 ` Srikant Ritolia
2016-12-10 19:56 ` Krzysztof Kozlowski
2016-12-17 16:04 ` Sebastian Reichel
2016-12-17 16:28 ` Krzysztof Kozlowski
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.