| Message ID | 1449753107-11410-3-git-send-email-uwe@kleine-koenig.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. And the code is shorter (more readable) when using just "gpio" instead of "ddata->enable_gpio". So I'll leave this one out. Tomi
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
On 20/12/15 12:31, Uwe Kleine-König wrote: > 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. I don't have a problem with writing to struct immediately. My point was that I don't like writing invalid values to "long term storage". What's "invalid" is of course up to the case, but here I think it's quite clear that 'ddata->enable_gpio' should either be a valid gpiod or NULL. Generally, I also like to work on a resource/object in local variables until it's "ready", and only then push it to the public storage. In my experience that style results in less possibilities for bugs and confusion. > 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. Yes, my motivation is that later patches may change the behavior, and keeping a valid value in 'data->enable_gpio' may make future patches simpler and prevent problems. I think it's quite common to do cleanups later, using the values in the long term storage. Here it might mean setting enable-gpio to 0, or if devm_* was not used, freeing the gpio, and that would result in using an error ptr as a pointer to gpiod. And it's quite common to later change error handling slightly so that instead of failing, the driver continues if it doesn't actually require the resource that it failed to get. Of course, patches for both can and should be written correctly so that both local and direct write to struct work correctly. > 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); No big diff here, but I can easily imagine this turning in some future patch into: ddata->enable_gpio = devm_gpiod_get_optional(&pdev->dev, "enable", GPIOD_OUT_LOW); if (IS_ERR(ddata->enable_gpio)) { r = PTR_ERR(data->enable_gpio); ddata->enable_gpio = NULL; return r; } > 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; It is, of course, possible to use more exactly named locals instead of a single '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). I don't agree, but I don't care too much either =). Both ways are valid, and they can always be changed later if the driver changes enough to require that change. Tomi
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;