Re: [PATCH v8 1/2] mfd: bd71837: mfd driver for ROHM BD71837 PMIC

[Lee Jones <lee.jones@linaro.org>]

On Wed, 04 Jul 2018, Matti Vaittinen wrote:

> On Wed, Jul 04, 2018 at 11:11:02AM +0100, Lee Jones wrote:
> > On Wed, 04 Jul 2018, Matti Vaittinen wrote:
> > 
> > > On Wed, Jul 04, 2018 at 11:39:11AM +0300, Matti Vaittinen wrote:
> > > > On Tue, Jul 03, 2018 at 11:26:00AM +0200, Enric Balletbo Serra wrote:
> > > > > Missatge de Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> del
> > > > > dia dv., 29 de juny 2018 a les 11:47:
> > > > > 
> > > > > Now that you use devm calls and you don't need to unwind things I
> > > > > think is better to use plain returns. So,
> > > > > 
> > > > > return -ENOMEM;
> > > > 
> > > > I have never really understood why use of gotos in error handling is
> > > > discouraged.
> > 
> > They're not.
> > 
> > > > Personally I would always choose single point of exit from
> > > > a function when it is simple enough to achieve (like in this case). I've
> > > > written and fixed way too many functions which leak resources or
> > > > accidentally keep a lock when exiting from error branches. But I know
> > > > many colleagues like you who prefer not to have gotos but  in place returns
> > > > instead. So I guess I'll leave the final call on this to the one who is
> > > > maintainer for this code. And it is true there is no things to unwind
> > > > now - which does not mean that next updater won't add such. But as I
> > > > said, I know plenty of people share your view - and even though I rather
> > > > maintain code with only one exit the final call is on subsystem maintainer
> > > > here.
> > 
> > Please use gotos in the error path.
> > 
> > IMO, it's the nicest way to unwind (as you call it).
> 
> I'll keep the gotos but clean other stuff for patch v9 then.

Sounds good.

> > > Actually, If it was completely my call the probe would look something
> > > like this:
> > > 
> > > +static int bd71837_i2c_probe(struct i2c_client *i2c,
> > > +                           const struct i2c_device_id *id)
> > > +{

[...]

> > > +}
> > > 
> > > What do you think of this? To my eye it is nice. It keeps single point of
> > > exit and introduces only simple if-statements without the need of curly
> > > brackets. And finally the error prints string works as a comment too.
> > > I've seen bunch of constructs like this on the networking side but I
> > > have no idea if this is frowned on this subsystem =) Oh, and probe abowe
> > > is just to illustrate the idea, I did not even try compiling it yet.
> > 
> > That is horrible.  I nearly vomited on my keyboard. 
> 
> Note to self: Never to buy second hand keyboard from Lee =)

That is sound advice.

Not sure I would buy 2nd-hand keyboard from anyone - ewe! :\

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog