Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH v18 10/10] drm/omapdrm: Use of_find_backlight helper
To:     Meghana Madhyastha <meghana.madhyastha@gmail.com>,
        Lee Jones <lee.jones@linaro.org>,
        Daniel Thompson <daniel.thompson@linaro.org>,
        Jingoo Han <jingoohan1@gmail.com>,
        Thierry Reding <thierry.reding@gmail.com>,
        Tomi Valkeinen <tomi.valkeinen@ti.com>,
        Daniel Vetter <daniel.vetter@intel.com>,
        Sean Paul <seanpaul@chromium.org>,
        dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org
References: <cover.1516632387.git.meghana.madhyastha@gmail.com>
 <69379bffdebdddf49952eabeaaee7f1dd7cff99c.1516632388.git.meghana.madhyastha@gmail.com>
 <394a73f6-3459-449f-6d9f-4cb52a1d3328@tronnes.org>
 <20180123165501.GA5908@meghana-HP-Pavilion-Notebook>
From:   =?UTF-8?Q?Noralf_Tr=c3=b8nnes?= <noralf@tronnes.org>
Message-ID: <853882e3-5f96-56c9-4fe5-a55fe6fbb030@tronnes.org>
Date:   Tue, 23 Jan 2018 18:41:50 +0100
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101
 Thunderbird/52.5.2
MIME-Version: 1.0
In-Reply-To: <20180123165501.GA5908@meghana-HP-Pavilion-Notebook>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


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@gmail.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)