Re: [PATCH v18 10/10] drm/omapdrm: Use of_find_backlight helper

[Noralf TrÃnnes]

Den 23.01.2018 18.41, skrev Noralf TrÃnnes:
> Den 23.01.2018 17.55, skrev Meghana Madhyastha:
> > On Tue, Jan 23, 2018 at 03:37:38PM +0100, Noralf TrÃnnes wrote:
> > > Den 22.01.2018 15.56, skrev Meghana Madhyastha:
> > > > Replace of_find_backlight_by_node and of the code around it
> > > > with of_find_backlight helper to avoid repetition of code.
> > > >
> > > > Signed-off-by: Meghana Madhyastha <meghana.madhyastha@xxxxxxxxx>
> > > > ---
> > > > Changes in v18:
> > > > -Fixed warnings resulting from passing device_node* to 
> > > > of_find_backlight.
> > > > Â Fixed it by passing struct device* to of_find_backlight
> > > >
> > > > Â drivers/gpu/drm/omapdrm/displays/panel-dpi.c | 33 
> > > > ++++++++++------------------
> > > > Â 1 file changed, 11 insertions(+), 22 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/omapdrm/displays/panel-dpi.c 
> > > > b/drivers/gpu/drm/omapdrm/displays/panel-dpi.c
> > > > index ac9596251..93b7a176d 100644
> > > > --- a/drivers/gpu/drm/omapdrm/displays/panel-dpi.c
> > > > +++ b/drivers/gpu/drm/omapdrm/displays/panel-dpi.c
> > > > @@ -156,14 +156,14 @@ static struct omap_dss_driver panel_dpi_ops = {
> > > > Â static int panel_dpi_probe_of(struct platform_device *pdev)
> > > > Â {
> > > > ÂÂÂÂÂ struct panel_drv_data *ddata = platform_get_drvdata(pdev);
> > > > +ÂÂÂ struct device *dev = &pdev->dev;
> > > > ÂÂÂÂÂ struct device_node *node = pdev->dev.of_node;
> > > > -ÂÂÂ struct device_node *bl_node;
> > > > ÂÂÂÂÂ struct omap_dss_device *in;
> > > > ÂÂÂÂÂ int r;
> > > > ÂÂÂÂÂ struct display_timing timing;
> > > > ÂÂÂÂÂ struct gpio_desc *gpio;
> > > > -ÂÂÂ gpio = devm_gpiod_get_optional(&pdev->dev, "enable", 
> > > > GPIOD_OUT_LOW);
> > > > +ÂÂÂ gpio = devm_gpiod_get_optional(dev, "enable", GPIOD_OUT_LOW);
> > > Please don't make unrelated changes like this. It clutters the patch.
> > > You can just as well use &pdev->dev when getting the backlight also.
> > I had made the change in order to be more consistent with how the other
> > drivers were doing this. Most of them had a variable struct device *dev.
> > However, I can undo this if necessary.
>
> It's best to be consistent with the coding style in the driver you're
> changing. If you make an extra dev variable or not isn't that important,
> unless the driver maintainer have a strict coding style for their driver.
>
> I try to stay on the safe side, change as little as possible and do thing
> the way it's done in the driver to increase the change of getting the
> patch accepted as-is the first time around.
>
> The important feedback from me is to remove the unrelated changes.

and the use of the devm_ version ofc.

> Noralf.
>
> > > > ÂÂÂÂÂ if (IS_ERR(gpio))
> > > > ÂÂÂÂÂÂÂÂÂ return PTR_ERR(gpio);
> > > > @@ -175,47 +175,36 @@ static int panel_dpi_probe_of(struct 
> > > > platform_device *pdev)
> > > > ÂÂÂÂÂÂ * timing and order relative to the enable gpio. So for now 
> > > > it's just
> > > > ÂÂÂÂÂÂ * ensured that the reset line isn't active.
> > > > ÂÂÂÂÂÂ */
> > > > -ÂÂÂ gpio = devm_gpiod_get_optional(&pdev->dev, "reset", 
> > > > GPIOD_OUT_LOW);
> > > > +ÂÂÂ gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
> > > > ÂÂÂÂÂ if (IS_ERR(gpio))
> > > > ÂÂÂÂÂÂÂÂÂ return PTR_ERR(gpio);
> > > > -ÂÂÂ ddata->vcc_supply = devm_regulator_get(&pdev->dev, "vcc");
> > > > +ÂÂÂ ddata->vcc_supply = devm_regulator_get(dev, "vcc");
> > > > ÂÂÂÂÂ if (IS_ERR(ddata->vcc_supply))
> > > > ÂÂÂÂÂÂÂÂÂ return PTR_ERR(ddata->vcc_supply);
> > > > -ÂÂÂ bl_node = of_parse_phandle(node, "backlight", 0);
> > > > -ÂÂÂ if (bl_node) {
> > > > -ÂÂÂÂÂÂÂ ddata->backlight = of_find_backlight_by_node(bl_node);
> > > > -ÂÂÂÂÂÂÂ of_node_put(bl_node);
> > > > +ÂÂÂ ddata->backlight = of_find_backlight(dev);
> > > Any reason you don't use the devm_ version here?
> > > You do remove error_free_backlight...
> > >
> > > With the devm_ version remember to drop the put_device in
> > > panel_dpi_remove().
> > >
> > > Noralf.
> > >
> > > > -ÂÂÂÂÂÂÂ if (!ddata->backlight)
> > > > -ÂÂÂÂÂÂÂÂÂÂÂ return -EPROBE_DEFER;
> > > > -ÂÂÂ }
> > > > +ÂÂÂ if (IS_ERR(ddata->backlight))
> > > > +ÂÂÂÂÂÂÂ return PTR_ERR(ddata->backlight);
> > > > ÂÂÂÂÂ r = of_get_display_timing(node, "panel-timing", &timing);
> > > > ÂÂÂÂÂ if (r) {
> > > > -ÂÂÂÂÂÂÂ dev_err(&pdev->dev, "failed to get video timing\n");
> > > > -ÂÂÂÂÂÂÂ goto error_free_backlight;
> > > > +ÂÂÂÂÂÂÂ dev_err(dev, "failed to get video timing\n");
> > > > +ÂÂÂÂÂÂÂ return r;
> > > > ÂÂÂÂÂ }
> > > > ÂÂÂÂÂ videomode_from_timing(&timing, &ddata->vm);
> > > > ÂÂÂÂÂ in = omapdss_of_find_source_for_first_ep(node);
> > > > ÂÂÂÂÂ if (IS_ERR(in)) {
> > > > -ÂÂÂÂÂÂÂ dev_err(&pdev->dev, "failed to find video source\n");
> > > > -ÂÂÂÂÂÂÂ r = PTR_ERR(in);
> > > > -ÂÂÂÂÂÂÂ goto error_free_backlight;
> > > > +ÂÂÂÂÂÂÂ dev_err(dev, "failed to find video source\n");
> > > > +ÂÂÂÂÂÂÂ return PTR_ERR(in);
> > > > ÂÂÂÂÂ }
> > > > ÂÂÂÂÂ ddata->in = in;
> > > > ÂÂÂÂÂ return 0;
> > > > -
> > > > -error_free_backlight:
> > > > -ÂÂÂ if (ddata->backlight)
> > > > -ÂÂÂÂÂÂÂ put_device(&ddata->backlight->dev);
> > > > -
> > > > -ÂÂÂ return r;
> > > > Â }
> > > > Â static int panel_dpi_probe(struct platform_device *pdev)
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/dri-devel