Re: [PATCH] power: max77693_charger: Better sysfs creation and using devm APIs

[Krzysztof Kozlowski <krzk@kernel.org>]

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