[PATCH 0/2] backlight: pwm: fix oops on accessing removed legacy pwm device

[PATCH 0/2] backlight: pwm: fix oops on accessing removed legacy pwm device

[Vladimir Zapolskiy]

The changeset fixes an oops, if pwm device was allocated by calling
legacy API and not deregistered appropriately on removal:

  % dmesg | grep -i pwm
  pwm-backlight backlight.19: unable to request PWM, trying legacy API
  % rmmod pwm_bl
  % cat /sys/kernel/debug/pwm
  Unable to handle kernel paging request at virtual address 7f0c18cc
  pgd = ace18000
  [7f0c18cc] *pgd<5eb811, *pte\0000000, *ppte\0000000
  Internal error: Oops: 7 [#1] PREEMPT SMP ARM
  ......

Vladimir Zapolskiy (2):
  backlight: pwm: don't call legacy pwm request for device defined in dt
  backlight: pwm: clean up pwm requested using legacy API

 drivers/video/backlight/pwm_bl.c |   18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

-- 
1.7.10.4

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

[Vladimir Zapolskiy]

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(-)

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

[PATCH 2/2] backlight: pwm: clean up pwm requested using legacy API

[Vladimir Zapolskiy]

If PWM device is requested by means of legacy API pwm_request(), its
resources are not freed on module unbind, which may cause an oops on
access, e.g. by reading /sys/kernel/debug/pwm.

Reported-by: Dmitry Eremin-Solenikov <dmitry_eremin@mentor.com>
Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
---
 drivers/video/backlight/pwm_bl.c |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index dd7aaf7..40770dd 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -34,6 +34,7 @@ struct pwm_bl_data {
 	struct regulator	*power_supply;
 	struct gpio_desc	*enable_gpio;
 	unsigned int		scale;
+	bool			legacy;
 	int			(*notify)(struct device *,
 					  int brightness);
 	void			(*notify_after)(struct device *,
@@ -274,6 +275,7 @@ static int pwm_backlight_probe(struct platform_device *pdev)
 	pb->pwm = devm_pwm_get(&pdev->dev, NULL);
 	if (IS_ERR(pb->pwm) && !pdev->dev.of_node) {
 		dev_err(&pdev->dev, "unable to request PWM, trying legacy API\n");
+		pb->legacy = true;
 		pb->pwm = pwm_request(data->pwm_id, "pwm-backlight");
 	}
 
@@ -339,6 +341,8 @@ static int pwm_backlight_remove(struct platform_device *pdev)
 
 	if (pb->exit)
 		pb->exit(&pdev->dev);
+	if (pb->legacy)
+		pwm_free(pb->pwm);
 
 	return 0;
 }
-- 
1.7.10.4

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

[Greg KH]

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(-)

<formletter>

This is not the correct way to submit patches for inclusion in the
stable kernel tree.  Please read Documentation/stable_kernel_rules.txt
for how to do this properly.

</formletter>

Re: [PATCH 2/2] backlight: pwm: clean up pwm requested using legacy API

[Greg KH]

On Sat, Oct 11, 2014 at 04:46:26PM +0300, Vladimir Zapolskiy wrote:
> If PWM device is requested by means of legacy API pwm_request(), its
> resources are not freed on module unbind, which may cause an oops on
> access, e.g. by reading /sys/kernel/debug/pwm.
> 
> Reported-by: Dmitry Eremin-Solenikov <dmitry_eremin@mentor.com>
> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
> ---
>  drivers/video/backlight/pwm_bl.c |    4 ++++
>  1 file changed, 4 insertions(+)

<formletter>

This is not the correct way to submit patches for inclusion in the
stable kernel tree.  Please read Documentation/stable_kernel_rules.txt
for how to do this properly.

</formletter>

Re: [PATCH 2/2] backlight: pwm: clean up pwm requested using legacy API

[Vladimir Zapolskiy]

Hello Greg, Thierry,

On 11.10.2014 17:21, Greg KH wrote:
> On Sat, Oct 11, 2014 at 04:46:26PM +0300, Vladimir Zapolskiy wrote:
>> If PWM device is requested by means of legacy API pwm_request(), its
>> resources are not freed on module unbind, which may cause an oops on
>> access, e.g. by reading /sys/kernel/debug/pwm.
>>
>> Reported-by: Dmitry Eremin-Solenikov <dmitry_eremin@mentor.com>
>> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
>> ---
>>  drivers/video/backlight/pwm_bl.c |    4 ++++
>>  1 file changed, 4 insertions(+)
> 
> <formletter>
> 
> This is not the correct way to submit patches for inclusion in the
> stable kernel tree.  Please read Documentation/stable_kernel_rules.txt
> for how to do this properly.
> 
> </formletter>
> --

could you please review the change? I believe it would be nice to have
oops fix in v3.18.

--
With best wishes,
Vladimir

Re: [PATCH 2/2] backlight: pwm: clean up pwm requested using legacy API

[Greg KH]

On Fri, Nov 07, 2014 at 12:10:25AM +0200, Vladimir Zapolskiy wrote:
> Hello Greg, Thierry,
> 
> On 11.10.2014 17:21, Greg KH wrote:
> > On Sat, Oct 11, 2014 at 04:46:26PM +0300, Vladimir Zapolskiy wrote:
> >> If PWM device is requested by means of legacy API pwm_request(), its
> >> resources are not freed on module unbind, which may cause an oops on
> >> access, e.g. by reading /sys/kernel/debug/pwm.
> >>
> >> Reported-by: Dmitry Eremin-Solenikov <dmitry_eremin@mentor.com>
> >> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
> >> ---
> >>  drivers/video/backlight/pwm_bl.c |    4 ++++
> >>  1 file changed, 4 insertions(+)
> > 
> > <formletter>
> > 
> > This is not the correct way to submit patches for inclusion in the
> > stable kernel tree.  Please read Documentation/stable_kernel_rules.txt
> > for how to do this properly.
> > 
> > </formletter>
> > --
> 
> could you please review the change? I believe it would be nice to have
> oops fix in v3.18.

drivers/video/ is not under my control, so there's nothing I can do
here, sorry...

Re: [PATCH 2/2] backlight: pwm: clean up pwm requested using legacy API

[Lee Jones]

On Thu, 06 Nov 2014, Greg KH wrote:

> On Fri, Nov 07, 2014 at 12:10:25AM +0200, Vladimir Zapolskiy wrote:
> > Hello Greg, Thierry,
> > 
> > On 11.10.2014 17:21, Greg KH wrote:
> > > On Sat, Oct 11, 2014 at 04:46:26PM +0300, Vladimir Zapolskiy wrote:
> > >> If PWM device is requested by means of legacy API pwm_request(), its
> > >> resources are not freed on module unbind, which may cause an oops on
> > >> access, e.g. by reading /sys/kernel/debug/pwm.
> > >>
> > >> Reported-by: Dmitry Eremin-Solenikov <dmitry_eremin@mentor.com>
> > >> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
> > >> ---
> > >>  drivers/video/backlight/pwm_bl.c |    4 ++++
> > >>  1 file changed, 4 insertions(+)
> > > 
> > > <formletter>
> > > 
> > > This is not the correct way to submit patches for inclusion in the
> > > stable kernel tree.  Please read Documentation/stable_kernel_rules.txt
> > > for how to do this properly.
> > > 
> > > </formletter>
> > > --
> > 
> > could you please review the change? I believe it would be nice to have
> > oops fix in v3.18.
> 
> drivers/video/ is not under my control, so there's nothing I can do
> here, sorry...

I can apply this with Thierry's Ack.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 2/2] backlight: pwm: clean up pwm requested using legacy API

[Thierry Reding]

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

On Sat, Oct 11, 2014 at 04:46:26PM +0300, Vladimir Zapolskiy wrote:
> If PWM device is requested by means of legacy API pwm_request(), its
> resources are not freed on module unbind, which may cause an oops on
> access, e.g. by reading /sys/kernel/debug/pwm.
> 
> Reported-by: Dmitry Eremin-Solenikov <dmitry_eremin@mentor.com>
> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
> ---
>  drivers/video/backlight/pwm_bl.c |    4 ++++
>  1 file changed, 4 insertions(+)

Hopefully this can soon go away when all users of the legacy API have
been converted. Until then:

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

[-- 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

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 947 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(-)

I don't really understand what this is supposed to do. The commit
message doesn't make a very good job of explaining it either.

Can you describe in more detail what problem this fixes and why it
should be merged?

Thierry

[-- 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 07.11.2014 15:48, 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(-)
> 
> I don't really understand what this is supposed to do. The commit
> message doesn't make a very good job of explaining it either.
> 
> Can you describe in more detail what problem this fixes and why it
> should be merged?

thank you for review.

As it is shown by the code this particular change rejects fallback to
legacy PWM device request (which itself in turn is fixed in the next
commit) for boards with supplied DTS, "pwm-backlight" compatible node
and unregistered corresponding PWM device in that node.

I don't know if there is a good enough reason to register PWM backlight
device connected to some quite arbitrary PWM device, if no PWM device
information is given in the "pwm-backlight" compatible node, so I think
it makes sense to change the default policy.

--
With best wishes,
Vladimir

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

[Vladimir Zapolskiy]

Thierry,

On 07.11.2014 16:19, Vladimir Zapolskiy wrote:
> Hi Thierry,
> 
> On 07.11.2014 15:48, 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(-)
>>
>> I don't really understand what this is supposed to do. The commit
>> message doesn't make a very good job of explaining it either.
>>
>> Can you describe in more detail what problem this fixes and why it
>> should be merged?
> 
> thank you for review.
> 
> As it is shown by the code this particular change rejects fallback to
> legacy PWM device request (which itself in turn is fixed in the next
> commit) for boards with supplied DTS, "pwm-backlight" compatible node
> and unregistered corresponding PWM device in that node.
> 
> I don't know if there is a good enough reason to register PWM backlight
> device connected to some quite arbitrary PWM device, if no PWM device
> information is given in the "pwm-backlight" compatible node, so I think
> it makes sense to change the default policy.
> 

also please note that
Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
quite fairly describes "pwms" as a required property, but right now this
statement from the documentation is wrong, it is possible to register
pwm-backlight device driver (using notorious pwm_request() legacy API)
connected to some unspecified pwm device.

I don't think that the current registration policy is correct, that's
why I propose to fix the logic instead of making a documentation update.

--
With best wishes,
Vladimir

Re: [PATCH 2/2] backlight: pwm: clean up pwm requested using legacy API

[Lee Jones]

On Sat, 11 Oct 2014, Vladimir Zapolskiy wrote:

> If PWM device is requested by means of legacy API pwm_request(), its
> resources are not freed on module unbind, which may cause an oops on
> access, e.g. by reading /sys/kernel/debug/pwm.
> 
> Reported-by: Dmitry Eremin-Solenikov <dmitry_eremin@mentor.com>
> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
> ---
>  drivers/video/backlight/pwm_bl.c |    4 ++++
>  1 file changed, 4 insertions(+)

Applied with Thierry's Ack.

> diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
> index dd7aaf7..40770dd 100644
> --- a/drivers/video/backlight/pwm_bl.c
> +++ b/drivers/video/backlight/pwm_bl.c
> @@ -34,6 +34,7 @@ struct pwm_bl_data {
>  	struct regulator	*power_supply;
>  	struct gpio_desc	*enable_gpio;
>  	unsigned int		scale;
> +	bool			legacy;
>  	int			(*notify)(struct device *,
>  					  int brightness);
>  	void			(*notify_after)(struct device *,
> @@ -274,6 +275,7 @@ static int pwm_backlight_probe(struct platform_device *pdev)
>  	pb->pwm = devm_pwm_get(&pdev->dev, NULL);
>  	if (IS_ERR(pb->pwm) && !pdev->dev.of_node) {
>  		dev_err(&pdev->dev, "unable to request PWM, trying legacy API\n");
> +		pb->legacy = true;
>  		pb->pwm = pwm_request(data->pwm_id, "pwm-backlight");
>  	}
>  
> @@ -339,6 +341,8 @@ static int pwm_backlight_remove(struct platform_device *pdev)
>  
>  	if (pb->exit)
>  		pb->exit(&pdev->dev);
> +	if (pb->legacy)
> +		pwm_free(pb->pwm);
>  
>  	return 0;
>  }

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

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

[Vladimir Zapolskiy]

Hello Thierry,

On 07.11.2014 16:57, Vladimir Zapolskiy wrote:
> Thierry,
> 
> On 07.11.2014 16:19, Vladimir Zapolskiy wrote:
>> Hi Thierry,
>>
>> On 07.11.2014 15:48, 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(-)
>>>
>>> I don't really understand what this is supposed to do. The commit
>>> message doesn't make a very good job of explaining it either.
>>>
>>> Can you describe in more detail what problem this fixes and why it
>>> should be merged?
>>
>> thank you for review.
>>
>> As it is shown by the code this particular change rejects fallback to
>> legacy PWM device request (which itself in turn is fixed in the next
>> commit) for boards with supplied DTS, "pwm-backlight" compatible node
>> and unregistered corresponding PWM device in that node.
>>
>> I don't know if there is a good enough reason to register PWM backlight
>> device connected to some quite arbitrary PWM device, if no PWM device
>> information is given in the "pwm-backlight" compatible node, so I think
>> it makes sense to change the default policy.
>>
> 
> also please note that
> Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
> quite fairly describes "pwms" as a required property, but right now this
> statement from the documentation is wrong, it is possible to register
> pwm-backlight device driver (using notorious pwm_request() legacy API)
> connected to some unspecified pwm device.
> 
> I don't think that the current registration policy is correct, that's
> why I propose to fix the logic instead of making a documentation update.
> 

have you had a chance to check the rationale of the change?

If you accept it, should I make the commit message more verbose?

--
With best wishes,
Vladimir

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

[Vladimir Zapolskiy]

Hello Thierry,

On 01.12.2014 16:47, Vladimir Zapolskiy wrote:
> Hello Thierry,
> 
> On 07.11.2014 16:57, Vladimir Zapolskiy wrote:
>> Thierry,
>>
>> On 07.11.2014 16:19, Vladimir Zapolskiy wrote:
>>> Hi Thierry,
>>>
>>> On 07.11.2014 15:48, 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(-)
>>>>
>>>> I don't really understand what this is supposed to do. The commit
>>>> message doesn't make a very good job of explaining it either.
>>>>
>>>> Can you describe in more detail what problem this fixes and why it
>>>> should be merged?
>>>
>>> thank you for review.
>>>
>>> As it is shown by the code this particular change rejects fallback to
>>> legacy PWM device request (which itself in turn is fixed in the next
>>> commit) for boards with supplied DTS, "pwm-backlight" compatible node
>>> and unregistered corresponding PWM device in that node.
>>>
>>> I don't know if there is a good enough reason to register PWM backlight
>>> device connected to some quite arbitrary PWM device, if no PWM device
>>> information is given in the "pwm-backlight" compatible node, so I think
>>> it makes sense to change the default policy.
>>>
>>
>> also please note that
>> Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
>> quite fairly describes "pwms" as a required property, but right now this
>> statement from the documentation is wrong, it is possible to register
>> pwm-backlight device driver (using notorious pwm_request() legacy API)
>> connected to some unspecified pwm device.
>>
>> I don't think that the current registration policy is correct, that's
>> why I propose to fix the logic instead of making a documentation update.
>>
> 
> have you had a chance to check the rationale of the change?
> 
> If you accept it, should I make the commit message more verbose?

any updates? Do you have plans to merge the change?

--
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: 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 --]