Re: [PATCH 2/2 v1] iio: light: Add a driver for Sharp GP2AP002x00F

[Jonathan Cameron <jic23@kernel.org>]

On Thu, 2 Jan 2020 14:15:33 -0800
Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:

> Hi Jonathan,
> 
> On Mon, Dec 30, 2019 at 05:39:19PM +0000, Jonathan Cameron wrote:
> > On Sat, 28 Dec 2019 21:11:09 +0100
> > Linus Walleij <linus.walleij@linaro.org> wrote:
> >   
> > > +	/* Enable chip and IRQ, disable analog sleep */
> > > +	ret = regmap_write(gp2ap002->map, GP2AP002_OPMOD,
> > > +			   OPMOD_SSD_OPERATING | OPMOD_VCON_IRQ);
> > > +	if (ret < 0) {
> > > +		dev_err(gp2ap002->dev, "error setting up operation mode\n");
> > > +		return ret;
> > > +	}
> > > +
> > > +	/* Interrupt on VOUT enabled */
> > > +	ret = regmap_write(gp2ap002->map, GP2AP002_CON, CON_OCON_ENABLE);
> > > +	if (ret < 0) {
> > > +		dev_err(gp2ap002->dev, "error setting up VOUT control\n");
> > > +		return ret;  
> > 
> > drop this return ret out of the brackets as it's either 0 or negative anyway.  
> 
> Not my subsystem, but $0.02 anyways: I like calling the temp as "error"
> and explicitly return 0 in the success path even if it could be
> collapsed, as you can easily add more initialization without needing
> to go and alter previous blocks.

That's a perfectly valid method as long as ret is only ever an error
(or good).  We've tended to go with ret in IIO, so better to carry on with
that. 

I'm not that fussed about dropping the return ret; out, but definitely
prefer explicit if (ret) to make it clear ret is never positive in the
good path though.

Thanks,

Jonathan

> 
> Thanks.
>