| Message ID | 1413035186-11771-2-git-send-email-vladimir_zapolskiy@mentor.com |
|---|---|
| State | Changes Requested |
| Headers | show |
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> -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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 -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 >
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 -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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>
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");
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(-)