Re: [PATCH 1/2] backlight: pwm: don't call legacy pwm request for device defined in dt

Re: [PATCH 1/2] backlight: pwm: don't call legacy pwm request for device defined in dt

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2450 bytes --]

On Sat, Oct 11, 2014 at 04:46:25PM +0300, Vladimir Zapolskiy wrote:
> Platform PWM backlight data provided by board's device tree should be
> complete enough to successfully request a pwm device using pwm_get() API.
> 
> Based on initial implementation done by Dmitry Eremin-Solenikov.
> 
> Reported-by: Dmitry Eremin-Solenikov <dmitry_eremin@mentor.com>
> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
> Cc: Thierry Reding <thierry.reding@gmail.com>
> Cc: Jingoo Han <jg1.han@samsung.com>
> Cc: Bryan Wu <cooloney@gmail.com>
> Cc: Lee Jones <lee.jones@linaro.org>
> ---
>  drivers/video/backlight/pwm_bl.c |   14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)

This fell off my radar, but I think it's good. I used to have a local
patch somewhere that solved the same problem by initializing the pwm_id
field of platform_pwm_backlight_data to -1 in pwm_backlight_parse_dt(),
but I like this variant better because it's more explicit and doesn't
even attempt to request using the legacy API (which will inevitably fail
in the DT case anyway).

Vladimir, do you think you'd have the time to rebase this patch on top
of something recent and perhaps extend the commit message with some of
the arguments that you brought forth in this thread? Specifically it'd
be useful to mention that this enforces the DT binding and fixes a real
bug where the legacy path would try to request a PWM that's not
necessarily the right one.

Thierry

> diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
> index cb5ae4c..dd7aaf7 100644
> --- a/drivers/video/backlight/pwm_bl.c
> +++ b/drivers/video/backlight/pwm_bl.c
> @@ -272,15 +272,15 @@ static int pwm_backlight_probe(struct platform_device *pdev)
>  	}
>  
>  	pb->pwm = devm_pwm_get(&pdev->dev, NULL);
> -	if (IS_ERR(pb->pwm)) {
> +	if (IS_ERR(pb->pwm) && !pdev->dev.of_node) {
>  		dev_err(&pdev->dev, "unable to request PWM, trying legacy API\n");
> -
>  		pb->pwm = pwm_request(data->pwm_id, "pwm-backlight");
> -		if (IS_ERR(pb->pwm)) {
> -			dev_err(&pdev->dev, "unable to request legacy PWM\n");
> -			ret = PTR_ERR(pb->pwm);
> -			goto err_alloc;
> -		}
> +	}
> +
> +	if (IS_ERR(pb->pwm)) {
> +		dev_err(&pdev->dev, "unable to request PWM\n");
> +		ret = PTR_ERR(pb->pwm);
> +		goto err_alloc;
>  	}
>  
>  	dev_dbg(&pdev->dev, "got pwm for backlight\n");
> -- 
> 1.7.10.4
> 

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/2] backlight: pwm: don't call legacy pwm request for device defined in dt

[Vladimir Zapolskiy]

Hi Thierry,

On 12.06.2015 14:31, Thierry Reding wrote:
> On Sat, Oct 11, 2014 at 04:46:25PM +0300, Vladimir Zapolskiy wrote:
>> Platform PWM backlight data provided by board's device tree should be
>> complete enough to successfully request a pwm device using pwm_get() API.
>>
>> Based on initial implementation done by Dmitry Eremin-Solenikov.
>>
>> Reported-by: Dmitry Eremin-Solenikov <dmitry_eremin@mentor.com>
>> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
>> Cc: Thierry Reding <thierry.reding@gmail.com>
>> Cc: Jingoo Han <jg1.han@samsung.com>
>> Cc: Bryan Wu <cooloney@gmail.com>
>> Cc: Lee Jones <lee.jones@linaro.org>
>> ---
>>  drivers/video/backlight/pwm_bl.c |   14 +++++++-------
>>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> This fell off my radar, but I think it's good. I used to have a local
> patch somewhere that solved the same problem by initializing the pwm_id
> field of platform_pwm_backlight_data to -1 in pwm_backlight_parse_dt(),
> but I like this variant better because it's more explicit and doesn't
> even attempt to request using the legacy API (which will inevitably fail
> in the DT case anyway).
> 
> Vladimir, do you think you'd have the time to rebase this patch on top
> of something recent and perhaps extend the commit message with some of
> the arguments that you brought forth in this thread? Specifically it'd
> be useful to mention that this enforces the DT binding and fixes a real
> bug where the legacy path would try to request a PWM that's not
> necessarily the right one.

sure, no problem, I'll find time to rebase on top of Lee's
backlight/for-backlight-next and resend the change this weekend.

Thank you for reviewing :)

--
With best wishes,
Vladimir

Re: [PATCH 1/2] backlight: pwm: don't call legacy pwm request for device defined in dt

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2001 bytes --]

On Fri, Jun 12, 2015 at 03:57:57PM +0300, Vladimir Zapolskiy wrote:
> Hi Thierry,
> 
> On 12.06.2015 14:31, Thierry Reding wrote:
> > On Sat, Oct 11, 2014 at 04:46:25PM +0300, Vladimir Zapolskiy wrote:
> >> Platform PWM backlight data provided by board's device tree should be
> >> complete enough to successfully request a pwm device using pwm_get() API.
> >>
> >> Based on initial implementation done by Dmitry Eremin-Solenikov.
> >>
> >> Reported-by: Dmitry Eremin-Solenikov <dmitry_eremin@mentor.com>
> >> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
> >> Cc: Thierry Reding <thierry.reding@gmail.com>
> >> Cc: Jingoo Han <jg1.han@samsung.com>
> >> Cc: Bryan Wu <cooloney@gmail.com>
> >> Cc: Lee Jones <lee.jones@linaro.org>
> >> ---
> >>  drivers/video/backlight/pwm_bl.c |   14 +++++++-------
> >>  1 file changed, 7 insertions(+), 7 deletions(-)
> > 
> > This fell off my radar, but I think it's good. I used to have a local
> > patch somewhere that solved the same problem by initializing the pwm_id
> > field of platform_pwm_backlight_data to -1 in pwm_backlight_parse_dt(),
> > but I like this variant better because it's more explicit and doesn't
> > even attempt to request using the legacy API (which will inevitably fail
> > in the DT case anyway).
> > 
> > Vladimir, do you think you'd have the time to rebase this patch on top
> > of something recent and perhaps extend the commit message with some of
> > the arguments that you brought forth in this thread? Specifically it'd
> > be useful to mention that this enforces the DT binding and fixes a real
> > bug where the legacy path would try to request a PWM that's not
> > necessarily the right one.
> 
> sure, no problem, I'll find time to rebase on top of Lee's
> backlight/for-backlight-next and resend the change this weekend.
> 
> Thank you for reviewing :)

I'll be away for two weeks, but feel free to add my:

Acked-by: Thierry Reding <thierry.reding@gmail.com>

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]