Re: [PATCH 3/5] fbdev: omap2: panel-dpi: drop assignment to local variable

["Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>]

Hello,

On Wed, Dec 16, 2015 at 07:29:17PM +0200, Tomi Valkeinen wrote:
> On 10/12/15 15:11, Uwe Kleine-König wrote:
> > From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > 
> > The variable gpio is only used to store the return value of
> > devm_gpiod_get_optional just to assign it to a member of the driver
> > data.
> > 
> > Get rid of this local variable and assign to driver data directly.
> > 
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > ---
> >  drivers/video/fbdev/omap2/displays-new/panel-dpi.c | 10 ++++------
> >  1 file changed, 4 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/video/fbdev/omap2/displays-new/panel-dpi.c b/drivers/video/fbdev/omap2/displays-new/panel-dpi.c
> > index e780fd4f8b46..1216341a0d19 100644
> > --- a/drivers/video/fbdev/omap2/displays-new/panel-dpi.c
> > +++ b/drivers/video/fbdev/omap2/displays-new/panel-dpi.c
> > @@ -205,13 +205,11 @@ static int panel_dpi_probe_of(struct platform_device *pdev)
> >  	int r;
> >  	struct display_timing timing;
> >  	struct videomode vm;
> > -	struct gpio_desc *gpio;
> >  
> > -	gpio = devm_gpiod_get_optional(&pdev->dev, "enable", GPIOD_OUT_LOW);
> > -	if (IS_ERR(gpio))
> > -		return PTR_ERR(gpio);
> > -
> > -	ddata->enable_gpio = gpio;
> > +	ddata->enable_gpio = devm_gpiod_get_optional(&pdev->dev,
> > +						     "enable", GPIOD_OUT_LOW);
> > +	if (IS_ERR(ddata->enable_gpio))
> > +		return PTR_ERR(ddata->enable_gpio);
> >  
> >  	ddata->backlight_gpio = -ENOENT;
> 
> I usually try to avoid writing bad values to fields. Here
> ddata->enable_gpio may get an error ptr. It probably doesn't matter as
> we bail out right away, but still. If devm_gpiod_get_optional's return
> value would be NULL or valid gpio_desc*, then it'd be fine.

this is probably a matter of taste but still I don't see why people
don't like writing to structs immediately.

With the local variable you might have

	gpio = -ESOMETHING

and

	ddata->enable_gpio = NULL;

In the case that the error is handled correctly it doesn't matter if the
value was written to the struct or not (if you accept a little
performance penalty for writing the value actually to memory maybe). So
the motivation is the consideration that the error might not be handled
correctly after a later patch, right? But when ddata->enable_gpio is a
negative error code this probably results in a crash already during
development of the faulty patch, while when the struct's member isn't
assigned it probably doesn't.

This convinces me that writing to the struct is actually a good thing.

Additionally even though the line length of 

	gpio = devm_gpiod_get_optional(&pdev->dev, "enable", GPIOD_OUT_LOW);
	if (IS_ERR(gpio))
		return PTR_ERR(gpio);

	ddata->enable_gpio = gpio;

is shorter (which is good), with my approach of doing:

	ddata->enable_gpio = devm_gpiod_get_optional(&pdev->dev,
						     "enable", GPIOD_OUT_LOW);
	if (IS_ERR(ddata->enable_gpio))
		return PTR_ERR(ddata->enable_gpio);

apart from saving an assignment also "enable_gpio" and "enable" are
nearer to each other which IMHO makes it easier to see that the
assignment is correct which outweighs the longer lines. This argument
even gets more important when reset_gpio is added in patch 4 when the
situation looks as follows:

	gpio = devm_gpiod_get_optional(... "enable" ...);
	if (IS_ERR(gpio))
		...
	ddata->enable_gpio = gpio;

	gpio = devm_gpiod_get_optional(... "reset" ...);
	if (IS_ERR(gpio))
		...
	ddata->reset_gpio = gpio;

vs.

	ddata->enable_gpio = devm_gpiod_get_optional(... "enable" ...);
	if (IS_ERR(ddata->enable_gpio))
		...

	ddata->reset_gpio = devm_gpiod_get_optional(... "reset" ...);
	if (IS_ERR(ddata->reset_gpio))
		...

I like my approach better, but if you don't agree, I don't care enough
to argue (more).

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |