linux-kernel - Re: [PATCH v18 10/10] drm/omapdrm: Use of_find

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Tue, 23 Jan 2018 18:41:50 +0100
From:   Noralf Trønnes <noralf@...nnes.org>
To:     Meghana Madhyastha <meghana.madhyastha@...il.com>,
        Lee Jones <lee.jones@...aro.org>,
        Daniel Thompson <daniel.thompson@...aro.org>,
        Jingoo Han <jingoohan1@...il.com>,
        Thierry Reding <thierry.reding@...il.com>,
        Tomi Valkeinen <tomi.valkeinen@...com>,
        Daniel Vetter <daniel.vetter@...el.com>,
        Sean Paul <seanpaul@...omium.org>,
        dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v18 10/10] drm/omapdrm: Use of_find_backlight helper


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@...il.com>
>>> ---
>>> 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.

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)