* [PATCHv2] backlight: pwm_bl: disable PWM when 'duty_cycle' is zero @ 2016-06-07 10:13 Lothar Waßmann 2016-06-08 20:06 ` Rob Herring 2016-06-09 13:51 ` Lee Jones 0 siblings, 2 replies; 12+ messages in thread From: Lothar Waßmann @ 2016-06-07 10:13 UTC (permalink / raw) To: Jean-Christophe Plagniol-Villard, Jingoo Han, Lee Jones, Thierry Reding, Tomi Valkeinen, linux-fbdev, linux-kernel, linux-pwm, Marcel Ziswiler, Ian Campbell, Kumar Gala, Lothar Waßmann, Mark Rutland, Pawel Moll, Rob Herring, devicetree 'brightness' is usually an index into a table of duty_cycle values, where the value at index 0 may well be non-zero (tegra30-apalis-eval.dts and tegra30-colibri-eval-v3.dts are real-life examples). Thus brightness == 0 does not necessarily mean that the PWM output will be inactive. Check for 'duty_cycle == 0' rather than 'brightness == 0' to decide whether to disable the PWM. Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de> --- Changes wrt. v1: - update binding docs to reflect the change .../devicetree/bindings/leds/backlight/pwm-backlight.txt | 9 ++++++--- drivers/video/backlight/pwm_bl.c | 4 ++-- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt index 764db86..95fa8a9 100644 --- a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt +++ b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt @@ -4,10 +4,13 @@ Required properties: - compatible: "pwm-backlight" - pwms: OF device-tree PWM specification (see PWM binding[0]) - brightness-levels: Array of distinct brightness levels. Typically these - are in the range from 0 to 255, but any range starting at 0 will do. + are in the range from 0 to 255, but any range will do. The actual brightness level (PWM duty cycle) will be interpolated - from these values. 0 means a 0% duty cycle (darkest/off), while the - last value in the array represents a 100% duty cycle (brightest). + from these values. 0 means a 0% duty cycle, while the highest value in + the array represents a 100% duty cycle. + The range may be in reverse order (starting with the maximum duty cycle + value) to create a PWM signal with the 100% duty cycle representing + minimum and 0% duty cycle maximum brigthness. - default-brightness-level: the default brightness level (index into the array defined by the "brightness-levels" property) - power-supply: regulator for supply voltage diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c index b2b366b..80b2b52 100644 --- a/drivers/video/backlight/pwm_bl.c +++ b/drivers/video/backlight/pwm_bl.c @@ -103,8 +103,8 @@ static int pwm_backlight_update_status(struct backlight_device *bl) if (pb->notify) brightness = pb->notify(pb->dev, brightness); - if (brightness > 0) { - duty_cycle = compute_duty_cycle(pb, brightness); + duty_cycle = compute_duty_cycle(pb, brightness); + if (duty_cycle > 0) { pwm_config(pb->pwm, duty_cycle, pb->period); pwm_backlight_power_on(pb, brightness); } else -- 2.1.4 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCHv2] backlight: pwm_bl: disable PWM when 'duty_cycle' is zero 2016-06-07 10:13 [PATCHv2] backlight: pwm_bl: disable PWM when 'duty_cycle' is zero Lothar Waßmann @ 2016-06-08 20:06 ` Rob Herring 2016-06-09 13:51 ` Lee Jones 1 sibling, 0 replies; 12+ messages in thread From: Rob Herring @ 2016-06-08 20:06 UTC (permalink / raw) To: Lothar Waßmann Cc: Jean-Christophe Plagniol-Villard, Jingoo Han, Lee Jones, Thierry Reding, Tomi Valkeinen, linux-fbdev, linux-kernel, linux-pwm, Marcel Ziswiler, Ian Campbell, Kumar Gala, Mark Rutland, Pawel Moll, devicetree On Tue, Jun 07, 2016 at 12:13:49PM +0200, Lothar Waßmann wrote: > 'brightness' is usually an index into a table of duty_cycle values, > where the value at index 0 may well be non-zero > (tegra30-apalis-eval.dts and tegra30-colibri-eval-v3.dts are real-life > examples). > Thus brightness == 0 does not necessarily mean that the PWM output > will be inactive. > Check for 'duty_cycle == 0' rather than 'brightness == 0' to decide > whether to disable the PWM. > > Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de> > --- > Changes wrt. v1: > - update binding docs to reflect the change > > .../devicetree/bindings/leds/backlight/pwm-backlight.txt | 9 ++++++--- > drivers/video/backlight/pwm_bl.c | 4 ++-- > 2 files changed, 8 insertions(+), 5 deletions(-) Acked-by: Rob Herring <robh@kernel.org> ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCHv2] backlight: pwm_bl: disable PWM when 'duty_cycle' is zero 2016-06-07 10:13 [PATCHv2] backlight: pwm_bl: disable PWM when 'duty_cycle' is zero Lothar Waßmann 2016-06-08 20:06 ` Rob Herring @ 2016-06-09 13:51 ` Lee Jones 2016-06-10 5:23 ` Lothar Waßmann 1 sibling, 1 reply; 12+ messages in thread From: Lee Jones @ 2016-06-09 13:51 UTC (permalink / raw) To: Lothar Waßmann Cc: Jean-Christophe Plagniol-Villard, Jingoo Han, Thierry Reding, Tomi Valkeinen, linux-fbdev, linux-kernel, linux-pwm, Marcel Ziswiler, Ian Campbell, Kumar Gala, Mark Rutland, Pawel Moll, Rob Herring, devicetree On Tue, 07 Jun 2016, Lothar Waßmann wrote: > 'brightness' is usually an index into a table of duty_cycle values, > where the value at index 0 may well be non-zero > (tegra30-apalis-eval.dts and tegra30-colibri-eval-v3.dts are real-life > examples). > Thus brightness == 0 does not necessarily mean that the PWM output > will be inactive. > Check for 'duty_cycle == 0' rather than 'brightness == 0' to decide > whether to disable the PWM. > > Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de> > --- > Changes wrt. v1: > - update binding docs to reflect the change > > .../devicetree/bindings/leds/backlight/pwm-backlight.txt | 9 ++++++--- > drivers/video/backlight/pwm_bl.c | 4 ++-- > 2 files changed, 8 insertions(+), 5 deletions(-) > > diff --git a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt > index 764db86..95fa8a9 100644 > --- a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt > +++ b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt > @@ -4,10 +4,13 @@ Required properties: > - compatible: "pwm-backlight" > - pwms: OF device-tree PWM specification (see PWM binding[0]) > - brightness-levels: Array of distinct brightness levels. Typically these > - are in the range from 0 to 255, but any range starting at 0 will do. > + are in the range from 0 to 255, but any range will do. > The actual brightness level (PWM duty cycle) will be interpolated > - from these values. 0 means a 0% duty cycle (darkest/off), while the > - last value in the array represents a 100% duty cycle (brightest). > + from these values. 0 means a 0% duty cycle, while the highest value in > + the array represents a 100% duty cycle. > + The range may be in reverse order (starting with the maximum duty cycle > + value) to create a PWM signal with the 100% duty cycle representing > + minimum and 0% duty cycle maximum brigthness. > - default-brightness-level: the default brightness level (index into the > array defined by the "brightness-levels" property) > - power-supply: regulator for supply voltage > diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c > index b2b366b..80b2b52 100644 > --- a/drivers/video/backlight/pwm_bl.c > +++ b/drivers/video/backlight/pwm_bl.c > @@ -103,8 +103,8 @@ static int pwm_backlight_update_status(struct backlight_device *bl) > if (pb->notify) > brightness = pb->notify(pb->dev, brightness); > > - if (brightness > 0) { > - duty_cycle = compute_duty_cycle(pb, brightness); > + duty_cycle = compute_duty_cycle(pb, brightness); > + if (duty_cycle > 0) { How does this work in the aforementioned: "The range may be in reverse order" ... case? Surely when duty_cycle is when the screen should be at it's brightest? Wouldn't it confuse the user if they turn their brightness *up* and the screen goes *off*? > pwm_config(pb->pwm, duty_cycle, pb->period); > pwm_backlight_power_on(pb, brightness); > } else -- Lee Jones Linaro STMicroelectronics Landing Team Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCHv2] backlight: pwm_bl: disable PWM when 'duty_cycle' is zero 2016-06-09 13:51 ` Lee Jones @ 2016-06-10 5:23 ` Lothar Waßmann 2016-06-10 7:44 ` Lee Jones 0 siblings, 1 reply; 12+ messages in thread From: Lothar Waßmann @ 2016-06-10 5:23 UTC (permalink / raw) To: Lee Jones Cc: Jean-Christophe Plagniol-Villard, Jingoo Han, Thierry Reding, Tomi Valkeinen, linux-fbdev, linux-kernel, linux-pwm, Marcel Ziswiler, Ian Campbell, Kumar Gala, Mark Rutland, Pawel Moll, Rob Herring, devicetree Hi, On Thu, 9 Jun 2016 14:51:25 +0100 Lee Jones wrote: > On Tue, 07 Jun 2016, Lothar Waßmann wrote: > > > 'brightness' is usually an index into a table of duty_cycle values, > > where the value at index 0 may well be non-zero > > (tegra30-apalis-eval.dts and tegra30-colibri-eval-v3.dts are real-life > > examples). > > Thus brightness == 0 does not necessarily mean that the PWM output > > will be inactive. > > Check for 'duty_cycle == 0' rather than 'brightness == 0' to decide > > whether to disable the PWM. > > > > Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de> > > --- > > Changes wrt. v1: > > - update binding docs to reflect the change > > > > .../devicetree/bindings/leds/backlight/pwm-backlight.txt | 9 ++++++--- > > drivers/video/backlight/pwm_bl.c | 4 ++-- > > 2 files changed, 8 insertions(+), 5 deletions(-) > > > > diff --git a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt > > index 764db86..95fa8a9 100644 > > --- a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt > > +++ b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt > > @@ -4,10 +4,13 @@ Required properties: > > - compatible: "pwm-backlight" > > - pwms: OF device-tree PWM specification (see PWM binding[0]) > > - brightness-levels: Array of distinct brightness levels. Typically these > > - are in the range from 0 to 255, but any range starting at 0 will do. > > + are in the range from 0 to 255, but any range will do. > > The actual brightness level (PWM duty cycle) will be interpolated > > - from these values. 0 means a 0% duty cycle (darkest/off), while the > > - last value in the array represents a 100% duty cycle (brightest). > > + from these values. 0 means a 0% duty cycle, while the highest value in > > + the array represents a 100% duty cycle. > > + The range may be in reverse order (starting with the maximum duty cycle > > + value) to create a PWM signal with the 100% duty cycle representing > > + minimum and 0% duty cycle maximum brigthness. > > - default-brightness-level: the default brightness level (index into the > > array defined by the "brightness-levels" property) > > - power-supply: regulator for supply voltage > > diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c > > index b2b366b..80b2b52 100644 > > --- a/drivers/video/backlight/pwm_bl.c > > +++ b/drivers/video/backlight/pwm_bl.c > > @@ -103,8 +103,8 @@ static int pwm_backlight_update_status(struct backlight_device *bl) > > if (pb->notify) > > brightness = pb->notify(pb->dev, brightness); > > > > - if (brightness > 0) { > > - duty_cycle = compute_duty_cycle(pb, brightness); > > + duty_cycle = compute_duty_cycle(pb, brightness); > > + if (duty_cycle > 0) { > > How does this work in the aforementioned: > > "The range may be in reverse order" > > ... case? Surely when duty_cycle is when the screen should be at it's > brightest? Wouldn't it confuse the user if they turn their brightness > *up* and the screen goes *off*? > Assuming that the PWM output is inactive (LOW) when the duty_cycle is set to zero, there will be no difference between operating the PWM at duty_cycle 0 or disabling it. Currently, the screen will go bright when it should be off in this case. Lothar Waßmann ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCHv2] backlight: pwm_bl: disable PWM when 'duty_cycle' is zero 2016-06-10 5:23 ` Lothar Waßmann @ 2016-06-10 7:44 ` Lee Jones 2016-06-10 10:34 ` Lothar Waßmann 0 siblings, 1 reply; 12+ messages in thread From: Lee Jones @ 2016-06-10 7:44 UTC (permalink / raw) To: Lothar Waßmann Cc: Jean-Christophe Plagniol-Villard, Jingoo Han, Thierry Reding, Tomi Valkeinen, linux-fbdev, linux-kernel, linux-pwm, Marcel Ziswiler, Ian Campbell, Kumar Gala, Mark Rutland, Pawel Moll, Rob Herring, devicetree On Fri, 10 Jun 2016, Lothar Waßmann wrote: > Hi, > > On Thu, 9 Jun 2016 14:51:25 +0100 Lee Jones wrote: > > On Tue, 07 Jun 2016, Lothar Waßmann wrote: > > > > > 'brightness' is usually an index into a table of duty_cycle values, > > > where the value at index 0 may well be non-zero > > > (tegra30-apalis-eval.dts and tegra30-colibri-eval-v3.dts are real-life > > > examples). > > > Thus brightness == 0 does not necessarily mean that the PWM output > > > will be inactive. > > > Check for 'duty_cycle == 0' rather than 'brightness == 0' to decide > > > whether to disable the PWM. > > > > > > Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de> > > > --- > > > Changes wrt. v1: > > > - update binding docs to reflect the change > > > > > > .../devicetree/bindings/leds/backlight/pwm-backlight.txt | 9 ++++++--- > > > drivers/video/backlight/pwm_bl.c | 4 ++-- > > > 2 files changed, 8 insertions(+), 5 deletions(-) > > > > > > diff --git a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt > > > index 764db86..95fa8a9 100644 > > > --- a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt > > > +++ b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt > > > @@ -4,10 +4,13 @@ Required properties: > > > - compatible: "pwm-backlight" > > > - pwms: OF device-tree PWM specification (see PWM binding[0]) > > > - brightness-levels: Array of distinct brightness levels. Typically these > > > - are in the range from 0 to 255, but any range starting at 0 will do. > > > + are in the range from 0 to 255, but any range will do. > > > The actual brightness level (PWM duty cycle) will be interpolated > > > - from these values. 0 means a 0% duty cycle (darkest/off), while the > > > - last value in the array represents a 100% duty cycle (brightest). > > > + from these values. 0 means a 0% duty cycle, while the highest value in > > > + the array represents a 100% duty cycle. > > > + The range may be in reverse order (starting with the maximum duty cycle > > > + value) to create a PWM signal with the 100% duty cycle representing > > > + minimum and 0% duty cycle maximum brigthness. > > > - default-brightness-level: the default brightness level (index into the > > > array defined by the "brightness-levels" property) > > > - power-supply: regulator for supply voltage > > > diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c > > > index b2b366b..80b2b52 100644 > > > --- a/drivers/video/backlight/pwm_bl.c > > > +++ b/drivers/video/backlight/pwm_bl.c > > > @@ -103,8 +103,8 @@ static int pwm_backlight_update_status(struct backlight_device *bl) > > > if (pb->notify) > > > brightness = pb->notify(pb->dev, brightness); > > > > > > - if (brightness > 0) { > > > - duty_cycle = compute_duty_cycle(pb, brightness); > > > + duty_cycle = compute_duty_cycle(pb, brightness); > > > + if (duty_cycle > 0) { > > > > How does this work in the aforementioned: > > > > "The range may be in reverse order" > > > > ... case? Surely when duty_cycle is when the screen should be at it's > > brightest? Wouldn't it confuse the user if they turn their brightness > > *up* and the screen goes *off*? > > > Assuming that the PWM output is inactive (LOW) when the duty_cycle is > set to zero, there will be no difference between operating the PWM at > duty_cycle 0 or disabling it. > > Currently, the screen will go bright when it should be off in this > case. It sounds like we need something that lets the framework know if duty_cycle = MAX is the brightest or if duty_cycle = 0 is. Either way someone is going to get screwed by this logic. -- Lee Jones Linaro STMicroelectronics Landing Team Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCHv2] backlight: pwm_bl: disable PWM when 'duty_cycle' is zero 2016-06-10 7:44 ` Lee Jones @ 2016-06-10 10:34 ` Lothar Waßmann 2016-06-10 14:54 ` Lee Jones 0 siblings, 1 reply; 12+ messages in thread From: Lothar Waßmann @ 2016-06-10 10:34 UTC (permalink / raw) To: Lee Jones Cc: Jean-Christophe Plagniol-Villard, Jingoo Han, Thierry Reding, Tomi Valkeinen, linux-fbdev, linux-kernel, linux-pwm, Marcel Ziswiler, Ian Campbell, Kumar Gala, Mark Rutland, Pawel Moll, Rob Herring, devicetree Hi, On Fri, 10 Jun 2016 08:44:49 +0100 Lee Jones wrote: > On Fri, 10 Jun 2016, Lothar Waßmann wrote: > > > Hi, > > > > On Thu, 9 Jun 2016 14:51:25 +0100 Lee Jones wrote: > > > On Tue, 07 Jun 2016, Lothar Waßmann wrote: > > > > > > > 'brightness' is usually an index into a table of duty_cycle values, > > > > where the value at index 0 may well be non-zero > > > > (tegra30-apalis-eval.dts and tegra30-colibri-eval-v3.dts are real-life > > > > examples). > > > > Thus brightness == 0 does not necessarily mean that the PWM output > > > > will be inactive. > > > > Check for 'duty_cycle == 0' rather than 'brightness == 0' to decide > > > > whether to disable the PWM. > > > > > > > > Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de> > > > > --- > > > > Changes wrt. v1: > > > > - update binding docs to reflect the change > > > > > > > > .../devicetree/bindings/leds/backlight/pwm-backlight.txt | 9 ++++++--- > > > > drivers/video/backlight/pwm_bl.c | 4 ++-- > > > > 2 files changed, 8 insertions(+), 5 deletions(-) > > > > > > > > diff --git a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt > > > > index 764db86..95fa8a9 100644 > > > > --- a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt > > > > +++ b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt > > > > @@ -4,10 +4,13 @@ Required properties: > > > > - compatible: "pwm-backlight" > > > > - pwms: OF device-tree PWM specification (see PWM binding[0]) > > > > - brightness-levels: Array of distinct brightness levels. Typically these > > > > - are in the range from 0 to 255, but any range starting at 0 will do. > > > > + are in the range from 0 to 255, but any range will do. > > > > The actual brightness level (PWM duty cycle) will be interpolated > > > > - from these values. 0 means a 0% duty cycle (darkest/off), while the > > > > - last value in the array represents a 100% duty cycle (brightest). > > > > + from these values. 0 means a 0% duty cycle, while the highest value in > > > > + the array represents a 100% duty cycle. > > > > + The range may be in reverse order (starting with the maximum duty cycle > > > > + value) to create a PWM signal with the 100% duty cycle representing > > > > + minimum and 0% duty cycle maximum brigthness. > > > > - default-brightness-level: the default brightness level (index into the > > > > array defined by the "brightness-levels" property) > > > > - power-supply: regulator for supply voltage > > > > diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c > > > > index b2b366b..80b2b52 100644 > > > > --- a/drivers/video/backlight/pwm_bl.c > > > > +++ b/drivers/video/backlight/pwm_bl.c > > > > @@ -103,8 +103,8 @@ static int pwm_backlight_update_status(struct backlight_device *bl) > > > > if (pb->notify) > > > > brightness = pb->notify(pb->dev, brightness); > > > > > > > > - if (brightness > 0) { > > > > - duty_cycle = compute_duty_cycle(pb, brightness); > > > > + duty_cycle = compute_duty_cycle(pb, brightness); > > > > + if (duty_cycle > 0) { > > > > > > How does this work in the aforementioned: > > > > > > "The range may be in reverse order" > > > > > > ... case? Surely when duty_cycle is when the screen should be at it's > > > brightest? Wouldn't it confuse the user if they turn their brightness > > > *up* and the screen goes *off*? > > > > > Assuming that the PWM output is inactive (LOW) when the duty_cycle is > > set to zero, there will be no difference between operating the PWM at > > duty_cycle 0 or disabling it. > > > > Currently, the screen will go bright when it should be off in this > > case. > > It sounds like we need something that lets the framework know if > duty_cycle = MAX is the brightest or if duty_cycle = 0 is. Either way > someone is going to get screwed by this logic. > The backlight framework does not (and does not need to) know anything about PWM duty cycles. Its 'brightness' values are consistently 0 == dark, max == brightest in either case. Lothar Waßmann ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCHv2] backlight: pwm_bl: disable PWM when 'duty_cycle' is zero 2016-06-10 10:34 ` Lothar Waßmann @ 2016-06-10 14:54 ` Lee Jones 2016-06-11 7:08 ` Lothar Waßmann 0 siblings, 1 reply; 12+ messages in thread From: Lee Jones @ 2016-06-10 14:54 UTC (permalink / raw) To: Lothar Waßmann Cc: Jean-Christophe Plagniol-Villard, Jingoo Han, Thierry Reding, Tomi Valkeinen, linux-fbdev, linux-kernel, linux-pwm, Marcel Ziswiler, Ian Campbell, Kumar Gala, Mark Rutland, Pawel Moll, Rob Herring, devicetree On Fri, 10 Jun 2016, Lothar Waßmann wrote: > On Fri, 10 Jun 2016 08:44:49 +0100 Lee Jones wrote: > > On Fri, 10 Jun 2016, Lothar Waßmann wrote: > > > > > Hi, > > > > > > On Thu, 9 Jun 2016 14:51:25 +0100 Lee Jones wrote: > > > > On Tue, 07 Jun 2016, Lothar Waßmann wrote: > > > > > > > > > 'brightness' is usually an index into a table of duty_cycle values, > > > > > where the value at index 0 may well be non-zero > > > > > (tegra30-apalis-eval.dts and tegra30-colibri-eval-v3.dts are real-life > > > > > examples). > > > > > Thus brightness == 0 does not necessarily mean that the PWM output > > > > > will be inactive. > > > > > Check for 'duty_cycle == 0' rather than 'brightness == 0' to decide > > > > > whether to disable the PWM. > > > > > > > > > > Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de> > > > > > --- > > > > > Changes wrt. v1: > > > > > - update binding docs to reflect the change > > > > > > > > > > .../devicetree/bindings/leds/backlight/pwm-backlight.txt | 9 ++++++--- > > > > > drivers/video/backlight/pwm_bl.c | 4 ++-- > > > > > 2 files changed, 8 insertions(+), 5 deletions(-) > > > > > > > > > > diff --git a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt > > > > > index 764db86..95fa8a9 100644 > > > > > --- a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt > > > > > +++ b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt > > > > > @@ -4,10 +4,13 @@ Required properties: > > > > > - compatible: "pwm-backlight" > > > > > - pwms: OF device-tree PWM specification (see PWM binding[0]) > > > > > - brightness-levels: Array of distinct brightness levels. Typically these > > > > > - are in the range from 0 to 255, but any range starting at 0 will do. > > > > > + are in the range from 0 to 255, but any range will do. > > > > > The actual brightness level (PWM duty cycle) will be interpolated > > > > > - from these values. 0 means a 0% duty cycle (darkest/off), while the > > > > > - last value in the array represents a 100% duty cycle (brightest). > > > > > + from these values. 0 means a 0% duty cycle, while the highest value in > > > > > + the array represents a 100% duty cycle. > > > > > + The range may be in reverse order (starting with the maximum duty cycle > > > > > + value) to create a PWM signal with the 100% duty cycle representing > > > > > + minimum and 0% duty cycle maximum brigthness. > > > > > - default-brightness-level: the default brightness level (index into the > > > > > array defined by the "brightness-levels" property) > > > > > - power-supply: regulator for supply voltage > > > > > diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c > > > > > index b2b366b..80b2b52 100644 > > > > > --- a/drivers/video/backlight/pwm_bl.c > > > > > +++ b/drivers/video/backlight/pwm_bl.c > > > > > @@ -103,8 +103,8 @@ static int pwm_backlight_update_status(struct backlight_device *bl) > > > > > if (pb->notify) > > > > > brightness = pb->notify(pb->dev, brightness); > > > > > > > > > > - if (brightness > 0) { > > > > > - duty_cycle = compute_duty_cycle(pb, brightness); > > > > > + duty_cycle = compute_duty_cycle(pb, brightness); > > > > > + if (duty_cycle > 0) { > > > > > > > > How does this work in the aforementioned: > > > > > > > > "The range may be in reverse order" > > > > > > > > ... case? Surely when duty_cycle is when the screen should be at it's > > > > brightest? Wouldn't it confuse the user if they turn their brightness > > > > *up* and the screen goes *off*? > > > > > > > Assuming that the PWM output is inactive (LOW) when the duty_cycle is > > > set to zero, there will be no difference between operating the PWM at > > > duty_cycle 0 or disabling it. > > > > > > Currently, the screen will go bright when it should be off in this > > > case. > > > > It sounds like we need something that lets the framework know if > > duty_cycle = MAX is the brightest or if duty_cycle = 0 is. Either way > > someone is going to get screwed by this logic. > > > The backlight framework does not (and does not need to) know anything > about PWM duty cycles. Its 'brightness' values are consistently 0 == > dark, max == brightest in either case. What I'm getting at is; by the look of the documentation, the brightest setting can either be a duty cycle of 0 or 255. So what happens with your new semantics when the duty cycle of 0 represents the brightest setting and you reach 0? Didn't you just turn the backlight off? -- Lee Jones Linaro STMicroelectronics Landing Team Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCHv2] backlight: pwm_bl: disable PWM when 'duty_cycle' is zero 2016-06-10 14:54 ` Lee Jones @ 2016-06-11 7:08 ` Lothar Waßmann 2016-06-17 14:17 ` Lee Jones 0 siblings, 1 reply; 12+ messages in thread From: Lothar Waßmann @ 2016-06-11 7:08 UTC (permalink / raw) To: Lee Jones Cc: Jean-Christophe Plagniol-Villard, Jingoo Han, Thierry Reding, Tomi Valkeinen, linux-fbdev, linux-kernel, linux-pwm, Marcel Ziswiler, Ian Campbell, Kumar Gala, Mark Rutland, Pawel Moll, Rob Herring, devicetree Hi, On Fri, 10 Jun 2016 15:54:49 +0100 Lee Jones wrote: > On Fri, 10 Jun 2016, Lothar Waßmann wrote: > > On Fri, 10 Jun 2016 08:44:49 +0100 Lee Jones wrote: > > > On Fri, 10 Jun 2016, Lothar Waßmann wrote: > > > > > > > Hi, > > > > > > > > On Thu, 9 Jun 2016 14:51:25 +0100 Lee Jones wrote: > > > > > On Tue, 07 Jun 2016, Lothar Waßmann wrote: > > > > > > > > > > > 'brightness' is usually an index into a table of duty_cycle values, > > > > > > where the value at index 0 may well be non-zero > > > > > > (tegra30-apalis-eval.dts and tegra30-colibri-eval-v3.dts are real-life > > > > > > examples). > > > > > > Thus brightness == 0 does not necessarily mean that the PWM output > > > > > > will be inactive. > > > > > > Check for 'duty_cycle == 0' rather than 'brightness == 0' to decide > > > > > > whether to disable the PWM. > > > > > > > > > > > > Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de> > > > > > > --- > > > > > > Changes wrt. v1: > > > > > > - update binding docs to reflect the change > > > > > > > > > > > > .../devicetree/bindings/leds/backlight/pwm-backlight.txt | 9 ++++++--- > > > > > > drivers/video/backlight/pwm_bl.c | 4 ++-- > > > > > > 2 files changed, 8 insertions(+), 5 deletions(-) > > > > > > > > > > > > diff --git a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt > > > > > > index 764db86..95fa8a9 100644 > > > > > > --- a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt > > > > > > +++ b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt > > > > > > @@ -4,10 +4,13 @@ Required properties: > > > > > > - compatible: "pwm-backlight" > > > > > > - pwms: OF device-tree PWM specification (see PWM binding[0]) > > > > > > - brightness-levels: Array of distinct brightness levels. Typically these > > > > > > - are in the range from 0 to 255, but any range starting at 0 will do. > > > > > > + are in the range from 0 to 255, but any range will do. > > > > > > The actual brightness level (PWM duty cycle) will be interpolated > > > > > > - from these values. 0 means a 0% duty cycle (darkest/off), while the > > > > > > - last value in the array represents a 100% duty cycle (brightest). > > > > > > + from these values. 0 means a 0% duty cycle, while the highest value in > > > > > > + the array represents a 100% duty cycle. > > > > > > + The range may be in reverse order (starting with the maximum duty cycle > > > > > > + value) to create a PWM signal with the 100% duty cycle representing > > > > > > + minimum and 0% duty cycle maximum brigthness. > > > > > > - default-brightness-level: the default brightness level (index into the > > > > > > array defined by the "brightness-levels" property) > > > > > > - power-supply: regulator for supply voltage > > > > > > diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c > > > > > > index b2b366b..80b2b52 100644 > > > > > > --- a/drivers/video/backlight/pwm_bl.c > > > > > > +++ b/drivers/video/backlight/pwm_bl.c > > > > > > @@ -103,8 +103,8 @@ static int pwm_backlight_update_status(struct backlight_device *bl) > > > > > > if (pb->notify) > > > > > > brightness = pb->notify(pb->dev, brightness); > > > > > > > > > > > > - if (brightness > 0) { > > > > > > - duty_cycle = compute_duty_cycle(pb, brightness); > > > > > > + duty_cycle = compute_duty_cycle(pb, brightness); > > > > > > + if (duty_cycle > 0) { > > > > > > > > > > How does this work in the aforementioned: > > > > > > > > > > "The range may be in reverse order" > > > > > > > > > > ... case? Surely when duty_cycle is when the screen should be at it's > > > > > brightest? Wouldn't it confuse the user if they turn their brightness > > > > > *up* and the screen goes *off*? > > > > > > > > > Assuming that the PWM output is inactive (LOW) when the duty_cycle is > > > > set to zero, there will be no difference between operating the PWM at > > > > duty_cycle 0 or disabling it. > > > > > > > > Currently, the screen will go bright when it should be off in this > > > > case. > > > > > > It sounds like we need something that lets the framework know if > > > duty_cycle = MAX is the brightest or if duty_cycle = 0 is. Either way > > > someone is going to get screwed by this logic. > > > > > The backlight framework does not (and does not need to) know anything > > about PWM duty cycles. Its 'brightness' values are consistently 0 == > > dark, max == brightest in either case. > > What I'm getting at is; by the look of the documentation, the > brightest setting can either be a duty cycle of 0 or 255. So what > happens with your new semantics when the duty cycle of 0 represents > the brightest setting and you reach 0? Didn't you just turn the > backlight off? > As mentioned earlier, disabling the PWM has generally the same result as setting the duty cycle to 0. The current behaviour is broken in this case, since setting brightness to 0 with a non-zero duty_cycle as the first element of brightness-levels, the PWM will be disabled rather than switched to the given duty cycle. Disabling the PWM should have the same effect as setting the duty cycle to 0, so it is safe to check for duty_cycle == 0 to decide whether to disable the PWM. Lothar Waßmann ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCHv2] backlight: pwm_bl: disable PWM when 'duty_cycle' is zero 2016-06-11 7:08 ` Lothar Waßmann @ 2016-06-17 14:17 ` Lee Jones 2016-06-20 6:21 ` Lothar Waßmann 0 siblings, 1 reply; 12+ messages in thread From: Lee Jones @ 2016-06-17 14:17 UTC (permalink / raw) To: Lothar Waßmann Cc: Jean-Christophe Plagniol-Villard, Jingoo Han, Thierry Reding, Tomi Valkeinen, linux-fbdev, linux-kernel, linux-pwm, Marcel Ziswiler, Ian Campbell, Kumar Gala, Mark Rutland, Pawel Moll, Rob Herring, devicetree On Sat, 11 Jun 2016, Lothar Waßmann wrote: > On Fri, 10 Jun 2016 15:54:49 +0100 Lee Jones wrote: > > On Fri, 10 Jun 2016, Lothar Waßmann wrote: > > > On Fri, 10 Jun 2016 08:44:49 +0100 Lee Jones wrote: > > > > On Fri, 10 Jun 2016, Lothar Waßmann wrote: > > > > > > > > > Hi, > > > > > > > > > > On Thu, 9 Jun 2016 14:51:25 +0100 Lee Jones wrote: > > > > > > On Tue, 07 Jun 2016, Lothar Waßmann wrote: > > > > > > > > > > > > > 'brightness' is usually an index into a table of duty_cycle values, > > > > > > > where the value at index 0 may well be non-zero > > > > > > > (tegra30-apalis-eval.dts and tegra30-colibri-eval-v3.dts are real-life > > > > > > > examples). > > > > > > > Thus brightness == 0 does not necessarily mean that the PWM output > > > > > > > will be inactive. > > > > > > > Check for 'duty_cycle == 0' rather than 'brightness == 0' to decide > > > > > > > whether to disable the PWM. > > > > > > > > > > > > > > Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de> > > > > > > > --- > > > > > > > Changes wrt. v1: > > > > > > > - update binding docs to reflect the change > > > > > > > > > > > > > > .../devicetree/bindings/leds/backlight/pwm-backlight.txt | 9 ++++++--- > > > > > > > drivers/video/backlight/pwm_bl.c | 4 ++-- > > > > > > > 2 files changed, 8 insertions(+), 5 deletions(-) > > > > > > > > > > > > > > diff --git a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt > > > > > > > index 764db86..95fa8a9 100644 > > > > > > > --- a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt > > > > > > > +++ b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt > > > > > > > @@ -4,10 +4,13 @@ Required properties: > > > > > > > - compatible: "pwm-backlight" > > > > > > > - pwms: OF device-tree PWM specification (see PWM binding[0]) > > > > > > > - brightness-levels: Array of distinct brightness levels. Typically these > > > > > > > - are in the range from 0 to 255, but any range starting at 0 will do. > > > > > > > + are in the range from 0 to 255, but any range will do. > > > > > > > The actual brightness level (PWM duty cycle) will be interpolated > > > > > > > - from these values. 0 means a 0% duty cycle (darkest/off), while the > > > > > > > - last value in the array represents a 100% duty cycle (brightest). > > > > > > > + from these values. 0 means a 0% duty cycle, while the highest value in > > > > > > > + the array represents a 100% duty cycle. > > > > > > > + The range may be in reverse order (starting with the maximum duty cycle > > > > > > > + value) to create a PWM signal with the 100% duty cycle representing > > > > > > > + minimum and 0% duty cycle maximum brigthness. > > > > > > > - default-brightness-level: the default brightness level (index into the > > > > > > > array defined by the "brightness-levels" property) > > > > > > > - power-supply: regulator for supply voltage > > > > > > > diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c > > > > > > > index b2b366b..80b2b52 100644 > > > > > > > --- a/drivers/video/backlight/pwm_bl.c > > > > > > > +++ b/drivers/video/backlight/pwm_bl.c > > > > > > > @@ -103,8 +103,8 @@ static int pwm_backlight_update_status(struct backlight_device *bl) > > > > > > > if (pb->notify) > > > > > > > brightness = pb->notify(pb->dev, brightness); > > > > > > > > > > > > > > - if (brightness > 0) { > > > > > > > - duty_cycle = compute_duty_cycle(pb, brightness); > > > > > > > + duty_cycle = compute_duty_cycle(pb, brightness); > > > > > > > + if (duty_cycle > 0) { > > > > > > > > > > > > How does this work in the aforementioned: > > > > > > > > > > > > "The range may be in reverse order" > > > > > > > > > > > > ... case? Surely when duty_cycle is when the screen should be at it's > > > > > > brightest? Wouldn't it confuse the user if they turn their brightness > > > > > > *up* and the screen goes *off*? > > > > > > > > > > > Assuming that the PWM output is inactive (LOW) when the duty_cycle is > > > > > set to zero, there will be no difference between operating the PWM at > > > > > duty_cycle 0 or disabling it. > > > > > > > > > > Currently, the screen will go bright when it should be off in this > > > > > case. > > > > > > > > It sounds like we need something that lets the framework know if > > > > duty_cycle = MAX is the brightest or if duty_cycle = 0 is. Either way > > > > someone is going to get screwed by this logic. > > > > > > > The backlight framework does not (and does not need to) know anything > > > about PWM duty cycles. Its 'brightness' values are consistently 0 == > > > dark, max == brightest in either case. > > > > What I'm getting at is; by the look of the documentation, the > > brightest setting can either be a duty cycle of 0 or 255. So what > > happens with your new semantics when the duty cycle of 0 represents > > the brightest setting and you reach 0? Didn't you just turn the > > backlight off? > > > As mentioned earlier, disabling the PWM has generally the same result as > setting the duty cycle to 0. The current behaviour is broken in this > case, since setting brightness to 0 with a non-zero duty_cycle as the > first element of brightness-levels, the PWM will be disabled rather than > switched to the given duty cycle. > Disabling the PWM should have the same effect as setting the duty cycle > to 0, so it is safe to check for duty_cycle == 0 to decide whether to > disable the PWM. I agree with this. BUT, that's not what you're doing is it? Look at the code you're trying to write: duty_cycle = compute_duty_cycle(pb, brightness); if (duty_cycle > 0) { pwm_config(pb->pwm, duty_cycle, pb->period); pwm_backlight_power_on(pb, brightness); } else pwm_backlight_power_off(pb); Let's say duty_cycle == 0. In some cases this can mean "turn brightness up to the *maximum*", but with your new logic you just turned the backlight *off*. Conversely, let's say duty_cycle == 255. In some cases this can mean "turn the brightness to the *lowest* setting" i.e. *off*. Well your logic just turned the backlight *on*. If there is something I'm missing, you're going to have to find a better way to explain it to me. -- Lee Jones Linaro STMicroelectronics Landing Team Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCHv2] backlight: pwm_bl: disable PWM when 'duty_cycle' is zero 2016-06-17 14:17 ` Lee Jones @ 2016-06-20 6:21 ` Lothar Waßmann 2016-06-20 6:29 ` Phil Reid 0 siblings, 1 reply; 12+ messages in thread From: Lothar Waßmann @ 2016-06-20 6:21 UTC (permalink / raw) To: Lee Jones Cc: Jean-Christophe Plagniol-Villard, Jingoo Han, Thierry Reding, Tomi Valkeinen, linux-fbdev, linux-kernel, linux-pwm, Marcel Ziswiler, Ian Campbell, Kumar Gala, Mark Rutland, Pawel Moll, Rob Herring, devicetree Hi, On Fri, 17 Jun 2016 15:17:19 +0100 Lee Jones wrote: > On Sat, 11 Jun 2016, Lothar Waßmann wrote: > > On Fri, 10 Jun 2016 15:54:49 +0100 Lee Jones wrote: > > > On Fri, 10 Jun 2016, Lothar Waßmann wrote: > > > > On Fri, 10 Jun 2016 08:44:49 +0100 Lee Jones wrote: > > > > > On Fri, 10 Jun 2016, Lothar Waßmann wrote: > > > > > > > > > > > Hi, > > > > > > > > > > > > On Thu, 9 Jun 2016 14:51:25 +0100 Lee Jones wrote: > > > > > > > On Tue, 07 Jun 2016, Lothar Waßmann wrote: > > > > > > > > > > > > > > > 'brightness' is usually an index into a table of duty_cycle values, > > > > > > > > where the value at index 0 may well be non-zero > > > > > > > > (tegra30-apalis-eval.dts and tegra30-colibri-eval-v3.dts are real-life > > > > > > > > examples). > > > > > > > > Thus brightness == 0 does not necessarily mean that the PWM output > > > > > > > > will be inactive. > > > > > > > > Check for 'duty_cycle == 0' rather than 'brightness == 0' to decide > > > > > > > > whether to disable the PWM. > > > > > > > > > > > > > > > > Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de> > > > > > > > > --- > > > > > > > > Changes wrt. v1: > > > > > > > > - update binding docs to reflect the change > > > > > > > > > > > > > > > > .../devicetree/bindings/leds/backlight/pwm-backlight.txt | 9 ++++++--- > > > > > > > > drivers/video/backlight/pwm_bl.c | 4 ++-- > > > > > > > > 2 files changed, 8 insertions(+), 5 deletions(-) > > > > > > > > > > > > > > > > diff --git a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt > > > > > > > > index 764db86..95fa8a9 100644 > > > > > > > > --- a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt > > > > > > > > +++ b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt > > > > > > > > @@ -4,10 +4,13 @@ Required properties: > > > > > > > > - compatible: "pwm-backlight" > > > > > > > > - pwms: OF device-tree PWM specification (see PWM binding[0]) > > > > > > > > - brightness-levels: Array of distinct brightness levels. Typically these > > > > > > > > - are in the range from 0 to 255, but any range starting at 0 will do. > > > > > > > > + are in the range from 0 to 255, but any range will do. > > > > > > > > The actual brightness level (PWM duty cycle) will be interpolated > > > > > > > > - from these values. 0 means a 0% duty cycle (darkest/off), while the > > > > > > > > - last value in the array represents a 100% duty cycle (brightest). > > > > > > > > + from these values. 0 means a 0% duty cycle, while the highest value in > > > > > > > > + the array represents a 100% duty cycle. > > > > > > > > + The range may be in reverse order (starting with the maximum duty cycle > > > > > > > > + value) to create a PWM signal with the 100% duty cycle representing > > > > > > > > + minimum and 0% duty cycle maximum brigthness. > > > > > > > > - default-brightness-level: the default brightness level (index into the > > > > > > > > array defined by the "brightness-levels" property) > > > > > > > > - power-supply: regulator for supply voltage > > > > > > > > diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c > > > > > > > > index b2b366b..80b2b52 100644 > > > > > > > > --- a/drivers/video/backlight/pwm_bl.c > > > > > > > > +++ b/drivers/video/backlight/pwm_bl.c > > > > > > > > @@ -103,8 +103,8 @@ static int pwm_backlight_update_status(struct backlight_device *bl) > > > > > > > > if (pb->notify) > > > > > > > > brightness = pb->notify(pb->dev, brightness); > > > > > > > > > > > > > > > > - if (brightness > 0) { > > > > > > > > - duty_cycle = compute_duty_cycle(pb, brightness); > > > > > > > > + duty_cycle = compute_duty_cycle(pb, brightness); > > > > > > > > + if (duty_cycle > 0) { > > > > > > > > > > > > > > How does this work in the aforementioned: > > > > > > > > > > > > > > "The range may be in reverse order" > > > > > > > > > > > > > > ... case? Surely when duty_cycle is when the screen should be at it's > > > > > > > brightest? Wouldn't it confuse the user if they turn their brightness > > > > > > > *up* and the screen goes *off*? > > > > > > > > > > > > > Assuming that the PWM output is inactive (LOW) when the duty_cycle is > > > > > > set to zero, there will be no difference between operating the PWM at > > > > > > duty_cycle 0 or disabling it. > > > > > > > > > > > > Currently, the screen will go bright when it should be off in this > > > > > > case. > > > > > > > > > > It sounds like we need something that lets the framework know if > > > > > duty_cycle = MAX is the brightest or if duty_cycle = 0 is. Either way > > > > > someone is going to get screwed by this logic. > > > > > > > > > The backlight framework does not (and does not need to) know anything > > > > about PWM duty cycles. Its 'brightness' values are consistently 0 == > > > > dark, max == brightest in either case. > > > > > > What I'm getting at is; by the look of the documentation, the > > > brightest setting can either be a duty cycle of 0 or 255. So what > > > happens with your new semantics when the duty cycle of 0 represents > > > the brightest setting and you reach 0? Didn't you just turn the > > > backlight off? > > > > > As mentioned earlier, disabling the PWM has generally the same result as > > setting the duty cycle to 0. The current behaviour is broken in this > > case, since setting brightness to 0 with a non-zero duty_cycle as the > > first element of brightness-levels, the PWM will be disabled rather than > > switched to the given duty cycle. > > Disabling the PWM should have the same effect as setting the duty cycle > > to 0, so it is safe to check for duty_cycle == 0 to decide whether to > > disable the PWM. > > I agree with this. BUT, that's not what you're doing is it? > > Look at the code you're trying to write: > > duty_cycle = compute_duty_cycle(pb, brightness); > if (duty_cycle > 0) { > pwm_config(pb->pwm, duty_cycle, pb->period); > pwm_backlight_power_on(pb, brightness); > } else > pwm_backlight_power_off(pb); > > Let's say duty_cycle == 0. In some cases this can mean "turn > brightness up to the *maximum*", but with your new logic you just > turned the backlight *off*. > Huh? Please think again! - duty_cycle == 0 means a CONSTANT LOW level on the PWM output. Agreed? - Disabling the PWM usually achieves a CONSTANT LOW level on the PWM output. Agreed? So duty_cycle == 0 <=> disable the PWM no matter whether the backlight is darkest or brightest at this duty cycle setting! The backlight controller does not know anything about the value of the 'brightness' variable in the code but only sees the 'duty_cycle' value. When brightness == 0 translates into max. duty cycle, the original code will switch the PWM OFF (which is equivalent to a ZERO duty cycle), when it rather should operate at the max. duty cycle. When duty_cycle is '0', this is equivalent to the PWM output being at constant LOW level which is the same as being switched OFF in the usual cases. When the brightness is maximum at duty_cycle == 0, that means, that the backlight is brightest when the control pin is constantly LOW, which is usually the case when the PWM is disabled. This is exactly what the patch does achieve! With the current code a backlight that is brightest at a constant '0' level will turn to max. brightness rather than off when selecting brightness level 0 (max. PWM duty cycle). > Conversely, let's say duty_cycle == 255. In some cases this can mean > "turn the brightness to the *lowest* setting" i.e. *off*. Well your > logic just turned the backlight *on*. > OK. Let's try a sequence of brightness levels and duty cycles: For simplicity assume a range of brightness levels from 0..100, so that the 'brightness' value directly represents the duty cycle of the PWM. So either: brightness == 0 => duty cycle == 0% => constant LOW Or: brightnes == 0 => duty cycle == 100% => constant HIGH. Normal range with current and patched code: brightness duty_cycle 0 0 PWM disabled => constant LOW 1 1 PWM active ... 100 100 PWM active => constant HIGH Inverted range (backlight brightest at duty cycle 0) Current code: brightness duty_cycle 0 100 PWM disabled (OUTPUT CONSTANT LOW!) 1 99 PWM active with near full duty cycle ... 99 1 PWM active with near ZERO duty cycle 100 0 PWM active with 0% duty cycle => constant LOW With my patch: brightness duty_cycle 0 100 PWM active with 100% duty cycle (constant HIGH) 1 99 PWM active with near full duty cycle ... 99 1 PWM active with near ZERO duty cycle 100 0 PWM disabled => constant LOW Lothar Waßmann ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCHv2] backlight: pwm_bl: disable PWM when 'duty_cycle' is zero 2016-06-20 6:21 ` Lothar Waßmann @ 2016-06-20 6:29 ` Phil Reid 2016-06-20 8:18 ` Lee Jones 0 siblings, 1 reply; 12+ messages in thread From: Phil Reid @ 2016-06-20 6:29 UTC (permalink / raw) To: Lothar Waßmann, Lee Jones Cc: Jean-Christophe Plagniol-Villard, Jingoo Han, Thierry Reding, Tomi Valkeinen, linux-fbdev, linux-kernel, linux-pwm, Marcel Ziswiler, Ian Campbell, Kumar Gala, Mark Rutland, Pawel Moll, Rob Herring, devicetree On 20/06/2016 14:21, Lothar Waßmann wrote: > Hi, > > On Fri, 17 Jun 2016 15:17:19 +0100 Lee Jones wrote: >> On Sat, 11 Jun 2016, Lothar Waßmann wrote: >>> On Fri, 10 Jun 2016 15:54:49 +0100 Lee Jones wrote: >>>> On Fri, 10 Jun 2016, Lothar Waßmann wrote: >>>>> On Fri, 10 Jun 2016 08:44:49 +0100 Lee Jones wrote: >>>>>> On Fri, 10 Jun 2016, Lothar Waßmann wrote: >>>>>> >>>>>>> Hi, >>>>>>> >>>>>>> On Thu, 9 Jun 2016 14:51:25 +0100 Lee Jones wrote: >>>>>>>> On Tue, 07 Jun 2016, Lothar Waßmann wrote: >>>>>>>> >>>>>>>>> 'brightness' is usually an index into a table of duty_cycle values, >>>>>>>>> where the value at index 0 may well be non-zero >>>>>>>>> (tegra30-apalis-eval.dts and tegra30-colibri-eval-v3.dts are real-life >>>>>>>>> examples). >>>>>>>>> Thus brightness == 0 does not necessarily mean that the PWM output >>>>>>>>> will be inactive. >>>>>>>>> Check for 'duty_cycle == 0' rather than 'brightness == 0' to decide >>>>>>>>> whether to disable the PWM. >>>>>>>>> >>>>>>>>> Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de> >>>>>>>>> --- >>>>>>>>> Changes wrt. v1: >>>>>>>>> - update binding docs to reflect the change >>>>>>>>> >>>>>>>>> .../devicetree/bindings/leds/backlight/pwm-backlight.txt | 9 ++++++--- >>>>>>>>> drivers/video/backlight/pwm_bl.c | 4 ++-- >>>>>>>>> 2 files changed, 8 insertions(+), 5 deletions(-) >>>>>>>>> >>>>>>>>> diff --git a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt >>>>>>>>> index 764db86..95fa8a9 100644 >>>>>>>>> --- a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt >>>>>>>>> +++ b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt >>>>>>>>> @@ -4,10 +4,13 @@ Required properties: >>>>>>>>> - compatible: "pwm-backlight" >>>>>>>>> - pwms: OF device-tree PWM specification (see PWM binding[0]) >>>>>>>>> - brightness-levels: Array of distinct brightness levels. Typically these >>>>>>>>> - are in the range from 0 to 255, but any range starting at 0 will do. >>>>>>>>> + are in the range from 0 to 255, but any range will do. >>>>>>>>> The actual brightness level (PWM duty cycle) will be interpolated >>>>>>>>> - from these values. 0 means a 0% duty cycle (darkest/off), while the >>>>>>>>> - last value in the array represents a 100% duty cycle (brightest). >>>>>>>>> + from these values. 0 means a 0% duty cycle, while the highest value in >>>>>>>>> + the array represents a 100% duty cycle. >>>>>>>>> + The range may be in reverse order (starting with the maximum duty cycle >>>>>>>>> + value) to create a PWM signal with the 100% duty cycle representing >>>>>>>>> + minimum and 0% duty cycle maximum brigthness. >>>>>>>>> - default-brightness-level: the default brightness level (index into the >>>>>>>>> array defined by the "brightness-levels" property) >>>>>>>>> - power-supply: regulator for supply voltage >>>>>>>>> diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c >>>>>>>>> index b2b366b..80b2b52 100644 >>>>>>>>> --- a/drivers/video/backlight/pwm_bl.c >>>>>>>>> +++ b/drivers/video/backlight/pwm_bl.c >>>>>>>>> @@ -103,8 +103,8 @@ static int pwm_backlight_update_status(struct backlight_device *bl) >>>>>>>>> if (pb->notify) >>>>>>>>> brightness = pb->notify(pb->dev, brightness); >>>>>>>>> >>>>>>>>> - if (brightness > 0) { >>>>>>>>> - duty_cycle = compute_duty_cycle(pb, brightness); >>>>>>>>> + duty_cycle = compute_duty_cycle(pb, brightness); >>>>>>>>> + if (duty_cycle > 0) { >>>>>>>> >>>>>>>> How does this work in the aforementioned: >>>>>>>> >>>>>>>> "The range may be in reverse order" >>>>>>>> >>>>>>>> ... case? Surely when duty_cycle is when the screen should be at it's >>>>>>>> brightest? Wouldn't it confuse the user if they turn their brightness >>>>>>>> *up* and the screen goes *off*? >>>>>>>> >>>>>>> Assuming that the PWM output is inactive (LOW) when the duty_cycle is >>>>>>> set to zero, there will be no difference between operating the PWM at >>>>>>> duty_cycle 0 or disabling it. >>>>>>> >>>>>>> Currently, the screen will go bright when it should be off in this >>>>>>> case. >>>>>> >>>>>> It sounds like we need something that lets the framework know if >>>>>> duty_cycle = MAX is the brightest or if duty_cycle = 0 is. Either way >>>>>> someone is going to get screwed by this logic. >>>>>> >>>>> The backlight framework does not (and does not need to) know anything >>>>> about PWM duty cycles. Its 'brightness' values are consistently 0 == >>>>> dark, max == brightest in either case. >>>> >>>> What I'm getting at is; by the look of the documentation, the >>>> brightest setting can either be a duty cycle of 0 or 255. So what >>>> happens with your new semantics when the duty cycle of 0 represents >>>> the brightest setting and you reach 0? Didn't you just turn the >>>> backlight off? >>>> >>> As mentioned earlier, disabling the PWM has generally the same result as >>> setting the duty cycle to 0. The current behaviour is broken in this >>> case, since setting brightness to 0 with a non-zero duty_cycle as the >>> first element of brightness-levels, the PWM will be disabled rather than >>> switched to the given duty cycle. >>> Disabling the PWM should have the same effect as setting the duty cycle >>> to 0, so it is safe to check for duty_cycle == 0 to decide whether to >>> disable the PWM. >> >> I agree with this. BUT, that's not what you're doing is it? >> >> Look at the code you're trying to write: >> >> duty_cycle = compute_duty_cycle(pb, brightness); >> if (duty_cycle > 0) { >> pwm_config(pb->pwm, duty_cycle, pb->period); >> pwm_backlight_power_on(pb, brightness); >> } else >> pwm_backlight_power_off(pb); >> >> Let's say duty_cycle == 0. In some cases this can mean "turn >> brightness up to the *maximum*", but with your new logic you just >> turned the backlight *off*. >> > Huh? Please think again! > - duty_cycle == 0 means a CONSTANT LOW level on the PWM output. Agreed? > - Disabling the PWM usually achieves a CONSTANT LOW level on the PWM > output. Agreed? > So duty_cycle == 0 <=> disable the PWM no matter whether the backlight > is darkest or brightest at this duty cycle setting! > > The backlight controller does not know anything about the value of the > 'brightness' variable in the code but only sees the 'duty_cycle' value. > When brightness == 0 translates into max. duty cycle, the original code > will switch the PWM OFF (which is equivalent to a ZERO duty cycle), when > it rather should operate at the max. duty cycle. > When duty_cycle is '0', this is equivalent to the PWM output being at > constant LOW level which is the same as being switched OFF in the usual > cases. > > When the brightness is maximum at duty_cycle == 0, that means, that the > backlight is brightest when the control pin is constantly LOW, which > is usually the case when the PWM is disabled. This is exactly what the > patch does achieve! > With the current code a backlight that is brightest at a constant '0' > level will turn to max. brightness rather than off when selecting > brightness level 0 (max. PWM duty cycle). > >> Conversely, let's say duty_cycle == 255. In some cases this can mean >> "turn the brightness to the *lowest* setting" i.e. *off*. Well your >> logic just turned the backlight *on*. >> > OK. Let's try a sequence of brightness levels and duty cycles: > For simplicity assume a range of brightness levels from 0..100, so > that the 'brightness' value directly represents the duty cycle of the > PWM. So either: brightness == 0 => duty cycle == 0% => constant LOW > Or: brightnes == 0 => duty cycle == 100% => constant HIGH. > > Normal range with current and patched code: > brightness duty_cycle > 0 0 PWM disabled => constant LOW > 1 1 PWM active > ... > 100 100 PWM active => constant HIGH > > Inverted range (backlight brightest at duty cycle 0) > Current code: > brightness duty_cycle > 0 100 PWM disabled (OUTPUT CONSTANT LOW!) > 1 99 PWM active with near full duty cycle > ... > 99 1 PWM active with near ZERO duty cycle > 100 0 PWM active with 0% duty cycle => constant LOW > > With my patch: > brightness duty_cycle > 0 100 PWM active with 100% duty cycle (constant HIGH) > 1 99 PWM active with near full duty cycle > ... > 99 1 PWM active with near ZERO duty cycle > 100 0 PWM disabled => constant LOW > > pwm_backlight_power_off() disables the regulator. So the supply to Backlight disappears, regardless of constant low... -- Regards Phil Reid ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCHv2] backlight: pwm_bl: disable PWM when 'duty_cycle' is zero 2016-06-20 6:29 ` Phil Reid @ 2016-06-20 8:18 ` Lee Jones 0 siblings, 0 replies; 12+ messages in thread From: Lee Jones @ 2016-06-20 8:18 UTC (permalink / raw) To: Phil Reid Cc: Lothar Waßmann, Jean-Christophe Plagniol-Villard, Jingoo Han, Thierry Reding, Tomi Valkeinen, linux-fbdev, linux-kernel, linux-pwm, Marcel Ziswiler, Ian Campbell, Kumar Gala, Mark Rutland, Pawel Moll, Rob Herring, devicetree On Mon, 20 Jun 2016, Phil Reid wrote: > On 20/06/2016 14:21, Lothar Waßmann wrote: > >Hi, > > > >On Fri, 17 Jun 2016 15:17:19 +0100 Lee Jones wrote: > >>On Sat, 11 Jun 2016, Lothar Waßmann wrote: > >>>On Fri, 10 Jun 2016 15:54:49 +0100 Lee Jones wrote: > >>>>On Fri, 10 Jun 2016, Lothar Waßmann wrote: > >>>>>On Fri, 10 Jun 2016 08:44:49 +0100 Lee Jones wrote: > >>>>>>On Fri, 10 Jun 2016, Lothar Waßmann wrote: > >>>>>> > >>>>>>>Hi, > >>>>>>> > >>>>>>>On Thu, 9 Jun 2016 14:51:25 +0100 Lee Jones wrote: > >>>>>>>>On Tue, 07 Jun 2016, Lothar Waßmann wrote: > >>>>>>>> > >>>>>>>>>'brightness' is usually an index into a table of duty_cycle values, > >>>>>>>>>where the value at index 0 may well be non-zero > >>>>>>>>>(tegra30-apalis-eval.dts and tegra30-colibri-eval-v3.dts are real-life > >>>>>>>>>examples). > >>>>>>>>>Thus brightness == 0 does not necessarily mean that the PWM output > >>>>>>>>>will be inactive. > >>>>>>>>>Check for 'duty_cycle == 0' rather than 'brightness == 0' to decide > >>>>>>>>>whether to disable the PWM. > >>>>>>>>> > >>>>>>>>>Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de> > >>>>>>>>>--- > >>>>>>>>>Changes wrt. v1: > >>>>>>>>> - update binding docs to reflect the change > >>>>>>>>> > >>>>>>>>> .../devicetree/bindings/leds/backlight/pwm-backlight.txt | 9 ++++++--- > >>>>>>>>> drivers/video/backlight/pwm_bl.c | 4 ++-- > >>>>>>>>> 2 files changed, 8 insertions(+), 5 deletions(-) > >>>>>>>>> > >>>>>>>>>diff --git a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt > >>>>>>>>>index 764db86..95fa8a9 100644 > >>>>>>>>>--- a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt > >>>>>>>>>+++ b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt > >>>>>>>>>@@ -4,10 +4,13 @@ Required properties: > >>>>>>>>> - compatible: "pwm-backlight" > >>>>>>>>> - pwms: OF device-tree PWM specification (see PWM binding[0]) > >>>>>>>>> - brightness-levels: Array of distinct brightness levels. Typically these > >>>>>>>>>- are in the range from 0 to 255, but any range starting at 0 will do. > >>>>>>>>>+ are in the range from 0 to 255, but any range will do. > >>>>>>>>> The actual brightness level (PWM duty cycle) will be interpolated > >>>>>>>>>- from these values. 0 means a 0% duty cycle (darkest/off), while the > >>>>>>>>>- last value in the array represents a 100% duty cycle (brightest). > >>>>>>>>>+ from these values. 0 means a 0% duty cycle, while the highest value in > >>>>>>>>>+ the array represents a 100% duty cycle. > >>>>>>>>>+ The range may be in reverse order (starting with the maximum duty cycle > >>>>>>>>>+ value) to create a PWM signal with the 100% duty cycle representing > >>>>>>>>>+ minimum and 0% duty cycle maximum brigthness. > >>>>>>>>> - default-brightness-level: the default brightness level (index into the > >>>>>>>>> array defined by the "brightness-levels" property) > >>>>>>>>> - power-supply: regulator for supply voltage > >>>>>>>>>diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c > >>>>>>>>>index b2b366b..80b2b52 100644 > >>>>>>>>>--- a/drivers/video/backlight/pwm_bl.c > >>>>>>>>>+++ b/drivers/video/backlight/pwm_bl.c > >>>>>>>>>@@ -103,8 +103,8 @@ static int pwm_backlight_update_status(struct backlight_device *bl) > >>>>>>>>> if (pb->notify) > >>>>>>>>> brightness = pb->notify(pb->dev, brightness); > >>>>>>>>> > >>>>>>>>>- if (brightness > 0) { > >>>>>>>>>- duty_cycle = compute_duty_cycle(pb, brightness); > >>>>>>>>>+ duty_cycle = compute_duty_cycle(pb, brightness); > >>>>>>>>>+ if (duty_cycle > 0) { > >>>>>>>> > >>>>>>>>How does this work in the aforementioned: > >>>>>>>> > >>>>>>>> "The range may be in reverse order" > >>>>>>>> > >>>>>>>>... case? Surely when duty_cycle is when the screen should be at it's > >>>>>>>>brightest? Wouldn't it confuse the user if they turn their brightness > >>>>>>>>*up* and the screen goes *off*? > >>>>>>>> > >>>>>>>Assuming that the PWM output is inactive (LOW) when the duty_cycle is > >>>>>>>set to zero, there will be no difference between operating the PWM at > >>>>>>>duty_cycle 0 or disabling it. > >>>>>>> > >>>>>>>Currently, the screen will go bright when it should be off in this > >>>>>>>case. > >>>>>> > >>>>>>It sounds like we need something that lets the framework know if > >>>>>>duty_cycle = MAX is the brightest or if duty_cycle = 0 is. Either way > >>>>>>someone is going to get screwed by this logic. > >>>>>> > >>>>>The backlight framework does not (and does not need to) know anything > >>>>>about PWM duty cycles. Its 'brightness' values are consistently 0 == > >>>>>dark, max == brightest in either case. > >>>> > >>>>What I'm getting at is; by the look of the documentation, the > >>>>brightest setting can either be a duty cycle of 0 or 255. So what > >>>>happens with your new semantics when the duty cycle of 0 represents > >>>>the brightest setting and you reach 0? Didn't you just turn the > >>>>backlight off? > >>>> > >>>As mentioned earlier, disabling the PWM has generally the same result as > >>>setting the duty cycle to 0. The current behaviour is broken in this > >>>case, since setting brightness to 0 with a non-zero duty_cycle as the > >>>first element of brightness-levels, the PWM will be disabled rather than > >>>switched to the given duty cycle. > >>>Disabling the PWM should have the same effect as setting the duty cycle > >>>to 0, so it is safe to check for duty_cycle == 0 to decide whether to > >>>disable the PWM. > >> > >>I agree with this. BUT, that's not what you're doing is it? > >> > >>Look at the code you're trying to write: > >> > >>duty_cycle = compute_duty_cycle(pb, brightness); > >>if (duty_cycle > 0) { > >> pwm_config(pb->pwm, duty_cycle, pb->period); > >> pwm_backlight_power_on(pb, brightness); > >>} else > >> pwm_backlight_power_off(pb); > >> > >>Let's say duty_cycle == 0. In some cases this can mean "turn > >>brightness up to the *maximum*", but with your new logic you just > >>turned the backlight *off*. > >> > >Huh? Please think again! > > - duty_cycle == 0 means a CONSTANT LOW level on the PWM output. Agreed? > > - Disabling the PWM usually achieves a CONSTANT LOW level on the PWM > > output. Agreed? > >So duty_cycle == 0 <=> disable the PWM no matter whether the backlight > >is darkest or brightest at this duty cycle setting! > > > >The backlight controller does not know anything about the value of the > >'brightness' variable in the code but only sees the 'duty_cycle' value. > >When brightness == 0 translates into max. duty cycle, the original code > >will switch the PWM OFF (which is equivalent to a ZERO duty cycle), when > >it rather should operate at the max. duty cycle. > >When duty_cycle is '0', this is equivalent to the PWM output being at > >constant LOW level which is the same as being switched OFF in the usual > >cases. > > > >When the brightness is maximum at duty_cycle == 0, that means, that the > >backlight is brightest when the control pin is constantly LOW, which > >is usually the case when the PWM is disabled. This is exactly what the > >patch does achieve! > >With the current code a backlight that is brightest at a constant '0' > >level will turn to max. brightness rather than off when selecting > >brightness level 0 (max. PWM duty cycle). > > > >>Conversely, let's say duty_cycle == 255. In some cases this can mean > >>"turn the brightness to the *lowest* setting" i.e. *off*. Well your > >>logic just turned the backlight *on*. > >> > >OK. Let's try a sequence of brightness levels and duty cycles: > >For simplicity assume a range of brightness levels from 0..100, so > >that the 'brightness' value directly represents the duty cycle of the > >PWM. So either: brightness == 0 => duty cycle == 0% => constant LOW > >Or: brightnes == 0 => duty cycle == 100% => constant HIGH. > > > >Normal range with current and patched code: > > brightness duty_cycle > > 0 0 PWM disabled => constant LOW > > 1 1 PWM active > > ... > > 100 100 PWM active => constant HIGH > > > >Inverted range (backlight brightest at duty cycle 0) > >Current code: > > brightness duty_cycle > > 0 100 PWM disabled (OUTPUT CONSTANT LOW!) > > 1 99 PWM active with near full duty cycle > > ... > > 99 1 PWM active with near ZERO duty cycle > > 100 0 PWM active with 0% duty cycle => constant LOW > > > >With my patch: > > brightness duty_cycle > > 0 100 PWM active with 100% duty cycle (constant HIGH) > > 1 99 PWM active with near full duty cycle > > ... > > 99 1 PWM active with near ZERO duty cycle > > 100 0 PWM disabled => constant LOW You're repleting yourself. I already told you I agreed with this. I think you need to re-read my previous reply. Your code is buggy. > pwm_backlight_power_off() disables the regulator. > So the supply to Backlight disappears, regardless of constant low... Yes exactly. Look at this again: > Let's say duty_cycle == 0. In some cases this can mean "turn > brightness up to the *maximum*", but with your new logic you just > turned the backlight *off*. When I say that you turned the backlight *off*. I didn't mean you turned it right down using the duty_cycle mechanism, I mean you ripped the socket from the wall. The IP is trying to turn the backlight to the highest setting, but it no longer has power. It's *off*, *off*! -- Lee Jones Linaro STMicroelectronics Landing Team Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2016-06-20 8:17 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-06-07 10:13 [PATCHv2] backlight: pwm_bl: disable PWM when 'duty_cycle' is zero Lothar Waßmann 2016-06-08 20:06 ` Rob Herring 2016-06-09 13:51 ` Lee Jones 2016-06-10 5:23 ` Lothar Waßmann 2016-06-10 7:44 ` Lee Jones 2016-06-10 10:34 ` Lothar Waßmann 2016-06-10 14:54 ` Lee Jones 2016-06-11 7:08 ` Lothar Waßmann 2016-06-17 14:17 ` Lee Jones 2016-06-20 6:21 ` Lothar Waßmann 2016-06-20 6:29 ` Phil Reid 2016-06-20 8:18 ` Lee Jones
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).