| Message ID | 20171116141151.21171-2-enric.balletbo@collabora.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | backlight: pwm_bl: support linear brightness to human eye | expand |
On Thu, Nov 16, 2017 at 03:11:50PM +0100, Enric Balletbo i Serra wrote: > Setting use-linear-interpolation in the dts will allow you to have linear > interpolation between values of brightness-levels. > > There are now 256 between each of the values of brightness-levels. If > something is requested halfway between 2 values, we'll use linear > interpolation. > > This way a high resolution pwm duty cycle can be used without having to > list out every possible value in the dts. I thought we already had a way to do that. > This system also allows for > gamma corrected values (eg: "brightness-levels = <0 2 4 8 16 32>;"). > > Patch based on the Alexandru M Stan work done for ChromeOS kernels. > > Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com> > --- > .../bindings/leds/backlight/pwm-backlight.txt | 2 + > drivers/video/backlight/pwm_bl.c | 55 +++++++++++++++++----- > include/linux/pwm_backlight.h | 2 + > 3 files changed, 47 insertions(+), 12 deletions(-) > > diff --git a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt > index 764db86..7c48f20 100644 > --- a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt > +++ b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt > @@ -17,6 +17,8 @@ Optional properties: > "pwms" property (see PWM binding[0]) > - enable-gpios: contains a single GPIO specifier for the GPIO which enables > and disables the backlight (see GPIO binding[1]) > + - use-linear-interpolation: set this propriety to enable linear interpolation > + between each of the values of brightness-levels. Isn't this really just whether you allow values not listed because what other kind of interpolation would you do? Seems like you should just be able to query the resolution of the PWM and decide if it can take more values than listed. Rob -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Rob, 2017-11-20 19:58 GMT+01:00 Rob Herring <robh@kernel.org>: > On Thu, Nov 16, 2017 at 03:11:50PM +0100, Enric Balletbo i Serra wrote: >> Setting use-linear-interpolation in the dts will allow you to have linear >> interpolation between values of brightness-levels. >> >> There are now 256 between each of the values of brightness-levels. If >> something is requested halfway between 2 values, we'll use linear >> interpolation. >> >> This way a high resolution pwm duty cycle can be used without having to >> list out every possible value in the dts. > > I thought we already had a way to do that. > >> This system also allows for >> gamma corrected values (eg: "brightness-levels = <0 2 4 8 16 32>;"). >> >> Patch based on the Alexandru M Stan work done for ChromeOS kernels. >> >> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com> >> --- >> .../bindings/leds/backlight/pwm-backlight.txt | 2 + >> drivers/video/backlight/pwm_bl.c | 55 +++++++++++++++++----- >> include/linux/pwm_backlight.h | 2 + >> 3 files changed, 47 insertions(+), 12 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt >> index 764db86..7c48f20 100644 >> --- a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt >> +++ b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt >> @@ -17,6 +17,8 @@ Optional properties: >> "pwms" property (see PWM binding[0]) >> - enable-gpios: contains a single GPIO specifier for the GPIO which enables >> and disables the backlight (see GPIO binding[1]) >> + - use-linear-interpolation: set this propriety to enable linear interpolation >> + between each of the values of brightness-levels. > > Isn't this really just whether you allow values not listed because what > other kind of interpolation would you do? Yes, the idea behind this is just allow values not listed. > Seems like you should just be > able to query the resolution of the PWM and decide if it can take more > values than listed. > Without using a new DT propriety and let decide the driver when use intepolation and when not? Shouldn't this break current behavior then? You can have a high resolution PWM but I'm not sure if you want allow always more values than listed. Regards, Enric > Rob -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Thu, Nov 16, 2017 at 6:11 AM, Enric Balletbo i Serra <enric.balletbo@collabora.com> wrote: > Setting use-linear-interpolation in the dts will allow you to have linear > interpolation between values of brightness-levels. > > There are now 256 between each of the values of brightness-levels. If > something is requested halfway between 2 values, we'll use linear > interpolation. > > This way a high resolution pwm duty cycle can be used without having to > list out every possible value in the dts. This system also allows for > gamma corrected values (eg: "brightness-levels = <0 2 4 8 16 32>;"). > > Patch based on the Alexandru M Stan work done for ChromeOS kernels. > > Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com> > --- > .../bindings/leds/backlight/pwm-backlight.txt | 2 + > drivers/video/backlight/pwm_bl.c | 55 +++++++++++++++++----- > include/linux/pwm_backlight.h | 2 + > 3 files changed, 47 insertions(+), 12 deletions(-) > > diff --git a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt > index 764db86..7c48f20 100644 > --- a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt > +++ b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt > @@ -17,6 +17,8 @@ Optional properties: > "pwms" property (see PWM binding[0]) > - enable-gpios: contains a single GPIO specifier for the GPIO which enables > and disables the backlight (see GPIO binding[1]) > + - use-linear-interpolation: set this propriety to enable linear interpolation > + between each of the values of brightness-levels. > > [0]: Documentation/devicetree/bindings/pwm/pwm.txt > [1]: Documentation/devicetree/bindings/gpio/gpio.txt > diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c > index 9bd1768..59b1bfb 100644 > --- a/drivers/video/backlight/pwm_bl.c > +++ b/drivers/video/backlight/pwm_bl.c > @@ -24,6 +24,8 @@ > #include <linux/regulator/consumer.h> > #include <linux/slab.h> > > +#define NSTEPS 256 I'm not sure this is quite the ideal way to specify it. I could sorta imagine wanting to specify just: brightness-levels = <0 65535> ...and in such a case you'll only give 256 steps in between. 256 isn't quite granular enough and the human eye can notice each step. I could fake it by putting this in the device tree: brightness-levels = <0 4095 8191 ... ... 61439 65535> ...but that's kinda silly. I'd rather just say that when we're using interpolation we just say that there will be a certain number of steps (like 32768). Is there really a huge advantage of picking 256 steps between each specified value instead of just picking a fixed number of brightness levels? -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Mon, Nov 27, 2017 at 3:21 AM, Enric Balletbo Serra <eballetbo@gmail.com> wrote: > Hi Rob, > > 2017-11-20 19:58 GMT+01:00 Rob Herring <robh@kernel.org>: >> On Thu, Nov 16, 2017 at 03:11:50PM +0100, Enric Balletbo i Serra wrote: >>> Setting use-linear-interpolation in the dts will allow you to have linear >>> interpolation between values of brightness-levels. >>> >>> There are now 256 between each of the values of brightness-levels. If >>> something is requested halfway between 2 values, we'll use linear >>> interpolation. >>> >>> This way a high resolution pwm duty cycle can be used without having to >>> list out every possible value in the dts. >> >> I thought we already had a way to do that. >> >>> This system also allows for >>> gamma corrected values (eg: "brightness-levels = <0 2 4 8 16 32>;"). >>> >>> Patch based on the Alexandru M Stan work done for ChromeOS kernels. >>> >>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com> >>> --- >>> .../bindings/leds/backlight/pwm-backlight.txt | 2 + >>> drivers/video/backlight/pwm_bl.c | 55 +++++++++++++++++----- >>> include/linux/pwm_backlight.h | 2 + >>> 3 files changed, 47 insertions(+), 12 deletions(-) >>> >>> diff --git a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt >>> index 764db86..7c48f20 100644 >>> --- a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt >>> +++ b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt >>> @@ -17,6 +17,8 @@ Optional properties: >>> "pwms" property (see PWM binding[0]) >>> - enable-gpios: contains a single GPIO specifier for the GPIO which enables >>> and disables the backlight (see GPIO binding[1]) >>> + - use-linear-interpolation: set this propriety to enable linear interpolation >>> + between each of the values of brightness-levels. >> >> Isn't this really just whether you allow values not listed because what >> other kind of interpolation would you do? > > Yes, the idea behind this is just allow values not listed. > >> Seems like you should just be >> able to query the resolution of the PWM and decide if it can take more >> values than listed. >> > > Without using a new DT propriety and let decide the driver when use > intepolation and when not? Shouldn't this break current behavior then? > You can have a high resolution PWM but I'm not sure if you want allow > always more values than listed. Right. The assumption is that it's _possible_ that only the exact values listed are valid duty cycles. We don't want to break old hardware by suddenly allowing other duty cycles to be used. However, it's expected that on most hardware you can simply use any duty cycle between any of the specified ones and it should be fine. Anyone specifying this property would say that's OK for their hardware. -Doug -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Nov 16, 2017 at 03:11:50PM +0100, Enric Balletbo i Serra wrote: > > Setting use-linear-interpolation in the dts will allow you to have linear > interpolation between values of brightness-levels. > > There are now 256 between each of the values of brightness-levels. If > something is requested halfway between 2 values, we'll use linear > interpolation. Why 256? > > This way a high resolution pwm duty cycle can be used without having to > list out every possible value in the dts. This system also allows for > gamma corrected values (eg: "brightness-levels = <0 2 4 8 16 32>;"). > > Patch based on the Alexandru M Stan work done for ChromeOS kernels. > > Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com> > --- > .../bindings/leds/backlight/pwm-backlight.txt | 2 + > drivers/video/backlight/pwm_bl.c | 55 +++++++++++++++++----- > include/linux/pwm_backlight.h | 2 + > 3 files changed, 47 insertions(+), 12 deletions(-) > > diff --git a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt > index 764db86..7c48f20 100644 > --- a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt > +++ b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt > @@ -17,6 +17,8 @@ Optional properties: > "pwms" property (see PWM binding[0]) > - enable-gpios: contains a single GPIO specifier for the GPIO which enables > and disables the backlight (see GPIO binding[1]) > + - use-linear-interpolation: set this propriety to enable linear interpolation > + between each of the values of brightness-levels. Deciding whether or not this deployment of interpolation is a property of the hardware is a finely balanced judgement. On the whole I conclude that since the lookup table is a property of the hardware so too is the deployment of interpolation. Following up on the "why 256?" comment. IMHO either the number of interpolated steps should be calculated from the underlying PWM resolution or it could simply be specified in the DT (e.g. replace use-linear-interpolation with num-interpolated-steps). > [0]: Documentation/devicetree/bindings/pwm/pwm.txt > [1]: Documentation/devicetree/bindings/gpio/gpio.txt > diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c > index 9bd1768..59b1bfb 100644 > --- a/drivers/video/backlight/pwm_bl.c > +++ b/drivers/video/backlight/pwm_bl.c > @@ -24,6 +24,8 @@ > #include <linux/regulator/consumer.h> > #include <linux/slab.h> > > +#define NSTEPS 256 > + > struct pwm_bl_data { > struct pwm_device *pwm; > struct device *dev; > @@ -35,6 +37,7 @@ struct pwm_bl_data { > struct gpio_desc *enable_gpio; > unsigned int scale; > bool legacy; > + bool piecewise; > int (*notify)(struct device *, > int brightness); > void (*notify_after)(struct device *, > @@ -76,17 +79,36 @@ static void pwm_backlight_power_off(struct pwm_bl_data *pb) > pb->enabled = false; > } > > -static int compute_duty_cycle(struct pwm_bl_data *pb, int brightness) > +static int scale(struct pwm_bl_data *pb, int x) > { > unsigned int lth = pb->lth_brightness; > + > + return (x * (pb->period - lth) / pb->scale) + lth; > +} > + > +static int compute_duty_cycle(struct pwm_bl_data *pb, int brightness) > +{ > + int coarse = brightness / NSTEPS; > + int fine = brightness % NSTEPS; > int duty_cycle; > > - if (pb->levels) > - duty_cycle = pb->levels[brightness]; > - else > - duty_cycle = brightness; > + if (pb->levels) { > + if (pb->piecewise) { > + duty_cycle = scale(pb, pb->levels[coarse]); > + if (fine > 0) > + duty_cycle += (scale(pb, pb->levels[coarse + 1]) > + - scale(pb, pb->levels[coarse])) > + * fine / NSTEPS; > + dev_dbg(pb->dev, "brightness=%d coarse=%d fine=%d\n", > + brightness, coarse, fine); > + } else { > + duty_cycle = scale(pb, pb->levels[brightness]); > + } > + } else { > + duty_cycle = scale(pb, brightness); > + } > > - return (duty_cycle * (pb->period - lth) / pb->scale) + lth; > + return duty_cycle; > } > > static int pwm_backlight_update_status(struct backlight_device *bl) > @@ -149,11 +171,11 @@ static int pwm_backlight_parse_dt(struct device *dev, > if (!prop) > return -EINVAL; > > - data->max_brightness = length / sizeof(u32); > + data->levels_count = length / sizeof(u32); > > /* read brightness levels from DT property */ > - if (data->max_brightness > 0) { > - size_t size = sizeof(*data->levels) * data->max_brightness; > + if (data->levels_count > 0) { > + size_t size = sizeof(*data->levels) * data->levels_count; > > data->levels = devm_kzalloc(dev, size, GFP_KERNEL); > if (!data->levels) > @@ -161,7 +183,7 @@ static int pwm_backlight_parse_dt(struct device *dev, > > ret = of_property_read_u32_array(node, "brightness-levels", > data->levels, > - data->max_brightness); > + data->levels_count); > if (ret < 0) > return ret; > > @@ -170,10 +192,18 @@ static int pwm_backlight_parse_dt(struct device *dev, > if (ret < 0) > return ret; > > + data->piecewise = of_property_read_bool(node, > + "use-linear-interpolation"); > + > data->dft_brightness = value; > - data->max_brightness--; > + data->levels_count--; > } > > + if (data->piecewise) > + data->max_brightness = data->levels_count * NSTEPS; > + else > + data->max_brightness = data->levels_count; I think we lost a -1 here? Daniel. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Daniel, 2017-12-15 15:40 GMT+01:00 Daniel Thompson <daniel.thompson@linaro.org>: > On Thu, Nov 16, 2017 at 03:11:50PM +0100, Enric Balletbo i Serra wrote: >> >> Setting use-linear-interpolation in the dts will allow you to have linear >> interpolation between values of brightness-levels. >> >> There are now 256 between each of the values of brightness-levels. If >> something is requested halfway between 2 values, we'll use linear >> interpolation. > > Why 256? To be honest there isn't a strong reason, I thought that 256 was a good value because is the minimum number of steps possible (8 bits pwm). But yeah, guess the discussion is more if this value should be calculated, or specified in the the DT more than the value itself. >> >> This way a high resolution pwm duty cycle can be used without having to >> list out every possible value in the dts. This system also allows for >> gamma corrected values (eg: "brightness-levels = <0 2 4 8 16 32>;"). >> >> Patch based on the Alexandru M Stan work done for ChromeOS kernels. >> >> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com> >> --- >> .../bindings/leds/backlight/pwm-backlight.txt | 2 + >> drivers/video/backlight/pwm_bl.c | 55 +++++++++++++++++----- >> include/linux/pwm_backlight.h | 2 + >> 3 files changed, 47 insertions(+), 12 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt >> index 764db86..7c48f20 100644 >> --- a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt >> +++ b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt >> @@ -17,6 +17,8 @@ Optional properties: >> "pwms" property (see PWM binding[0]) >> - enable-gpios: contains a single GPIO specifier for the GPIO which enables >> and disables the backlight (see GPIO binding[1]) >> + - use-linear-interpolation: set this propriety to enable linear interpolation >> + between each of the values of brightness-levels. > > Deciding whether or not this deployment of interpolation is a property > of the hardware is a finely balanced judgement. On the whole I conclude > that since the lookup table is a property of the hardware so too is the > deployment of interpolation. > > Following up on the "why 256?" comment. IMHO either the number of > interpolated steps should be calculated from the underlying PWM > resolution or it could simply be specified in the DT (e.g. replace > use-linear-interpolation with num-interpolated-steps). > Personally I like the idea to have the possibility to specify the number of interpolated steps in the DT, I think that will be more flexible for the user. If it's ok let me send a first version using num-interpolated-steps, and continue the discussion about if makes sense to have that in the DT or not. > >> [0]: Documentation/devicetree/bindings/pwm/pwm.txt >> [1]: Documentation/devicetree/bindings/gpio/gpio.txt >> diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c >> index 9bd1768..59b1bfb 100644 >> --- a/drivers/video/backlight/pwm_bl.c >> +++ b/drivers/video/backlight/pwm_bl.c >> @@ -24,6 +24,8 @@ >> #include <linux/regulator/consumer.h> >> #include <linux/slab.h> >> >> +#define NSTEPS 256 >> + >> struct pwm_bl_data { >> struct pwm_device *pwm; >> struct device *dev; >> @@ -35,6 +37,7 @@ struct pwm_bl_data { >> struct gpio_desc *enable_gpio; >> unsigned int scale; >> bool legacy; >> + bool piecewise; >> int (*notify)(struct device *, >> int brightness); >> void (*notify_after)(struct device *, >> @@ -76,17 +79,36 @@ static void pwm_backlight_power_off(struct pwm_bl_data *pb) >> pb->enabled = false; >> } >> >> -static int compute_duty_cycle(struct pwm_bl_data *pb, int brightness) >> +static int scale(struct pwm_bl_data *pb, int x) >> { >> unsigned int lth = pb->lth_brightness; >> + >> + return (x * (pb->period - lth) / pb->scale) + lth; >> +} >> + >> +static int compute_duty_cycle(struct pwm_bl_data *pb, int brightness) >> +{ >> + int coarse = brightness / NSTEPS; >> + int fine = brightness % NSTEPS; >> int duty_cycle; >> >> - if (pb->levels) >> - duty_cycle = pb->levels[brightness]; >> - else >> - duty_cycle = brightness; >> + if (pb->levels) { >> + if (pb->piecewise) { >> + duty_cycle = scale(pb, pb->levels[coarse]); >> + if (fine > 0) >> + duty_cycle += (scale(pb, pb->levels[coarse + 1]) >> + - scale(pb, pb->levels[coarse])) >> + * fine / NSTEPS; >> + dev_dbg(pb->dev, "brightness=%d coarse=%d fine=%d\n", >> + brightness, coarse, fine); >> + } else { >> + duty_cycle = scale(pb, pb->levels[brightness]); >> + } >> + } else { >> + duty_cycle = scale(pb, brightness); >> + } >> >> - return (duty_cycle * (pb->period - lth) / pb->scale) + lth; >> + return duty_cycle; >> } >> >> static int pwm_backlight_update_status(struct backlight_device *bl) >> @@ -149,11 +171,11 @@ static int pwm_backlight_parse_dt(struct device *dev, >> if (!prop) >> return -EINVAL; >> >> - data->max_brightness = length / sizeof(u32); >> + data->levels_count = length / sizeof(u32); >> >> /* read brightness levels from DT property */ >> - if (data->max_brightness > 0) { >> - size_t size = sizeof(*data->levels) * data->max_brightness; >> + if (data->levels_count > 0) { >> + size_t size = sizeof(*data->levels) * data->levels_count; >> >> data->levels = devm_kzalloc(dev, size, GFP_KERNEL); >> if (!data->levels) >> @@ -161,7 +183,7 @@ static int pwm_backlight_parse_dt(struct device *dev, >> >> ret = of_property_read_u32_array(node, "brightness-levels", >> data->levels, >> - data->max_brightness); >> + data->levels_count); >> if (ret < 0) >> return ret; >> >> @@ -170,10 +192,18 @@ static int pwm_backlight_parse_dt(struct device *dev, >> if (ret < 0) >> return ret; >> >> + data->piecewise = of_property_read_bool(node, >> + "use-linear-interpolation"); >> + >> data->dft_brightness = value; >> - data->max_brightness--; >> + data->levels_count--; >> } >> >> + if (data->piecewise) >> + data->max_brightness = data->levels_count * NSTEPS; >> + else >> + data->max_brightness = data->levels_count; > > I think we lost a -1 here? > Good catch, I think so. Regards, Enric > > Daniel. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Dec 18, 2017 at 10:47:24AM +0100, Enric Balletbo Serra wrote: > Hi Daniel, > > 2017-12-15 15:40 GMT+01:00 Daniel Thompson <daniel.thompson@linaro.org>: > > On Thu, Nov 16, 2017 at 03:11:50PM +0100, Enric Balletbo i Serra wrote: > >> > >> Setting use-linear-interpolation in the dts will allow you to have linear > >> interpolation between values of brightness-levels. > >> > >> There are now 256 between each of the values of brightness-levels. If > >> something is requested halfway between 2 values, we'll use linear > >> interpolation. > > > > Why 256? > > To be honest there isn't a strong reason, I thought that 256 was a > good value because is the minimum number of steps possible (8 bits > pwm). But yeah, guess the discussion is more if this value should be > calculated, or specified in the the DT more than the value itself. > > >> > >> This way a high resolution pwm duty cycle can be used without having to > >> list out every possible value in the dts. This system also allows for > >> gamma corrected values (eg: "brightness-levels = <0 2 4 8 16 32>;"). > >> > >> Patch based on the Alexandru M Stan work done for ChromeOS kernels. > >> > >> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com> > >> --- > >> .../bindings/leds/backlight/pwm-backlight.txt | 2 + > >> drivers/video/backlight/pwm_bl.c | 55 +++++++++++++++++----- > >> include/linux/pwm_backlight.h | 2 + > >> 3 files changed, 47 insertions(+), 12 deletions(-) > >> > >> diff --git a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt > >> index 764db86..7c48f20 100644 > >> --- a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt > >> +++ b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt > >> @@ -17,6 +17,8 @@ Optional properties: > >> "pwms" property (see PWM binding[0]) > >> - enable-gpios: contains a single GPIO specifier for the GPIO which enables > >> and disables the backlight (see GPIO binding[1]) > >> + - use-linear-interpolation: set this propriety to enable linear interpolation > >> + between each of the values of brightness-levels. > > > > Deciding whether or not this deployment of interpolation is a property > > of the hardware is a finely balanced judgement. On the whole I conclude > > that since the lookup table is a property of the hardware so too is the > > deployment of interpolation. > > > > Following up on the "why 256?" comment. IMHO either the number of > > interpolated steps should be calculated from the underlying PWM > > resolution or it could simply be specified in the DT (e.g. replace > > use-linear-interpolation with num-interpolated-steps). > > > > Personally I like the idea to have the possibility to specify the > number of interpolated steps in the DT, I think that will be more > flexible for the user. If it's ok let me send a first version using > num-interpolated-steps, and continue the discussion about if makes > sense to have that in the DT or not. It's ok from my side. Daniel. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt index 764db86..7c48f20 100644 --- a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt +++ b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt @@ -17,6 +17,8 @@ Optional properties: "pwms" property (see PWM binding[0]) - enable-gpios: contains a single GPIO specifier for the GPIO which enables and disables the backlight (see GPIO binding[1]) + - use-linear-interpolation: set this propriety to enable linear interpolation + between each of the values of brightness-levels. [0]: Documentation/devicetree/bindings/pwm/pwm.txt [1]: Documentation/devicetree/bindings/gpio/gpio.txt diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c index 9bd1768..59b1bfb 100644 --- a/drivers/video/backlight/pwm_bl.c +++ b/drivers/video/backlight/pwm_bl.c @@ -24,6 +24,8 @@ #include <linux/regulator/consumer.h> #include <linux/slab.h> +#define NSTEPS 256 + struct pwm_bl_data { struct pwm_device *pwm; struct device *dev; @@ -35,6 +37,7 @@ struct pwm_bl_data { struct gpio_desc *enable_gpio; unsigned int scale; bool legacy; + bool piecewise; int (*notify)(struct device *, int brightness); void (*notify_after)(struct device *, @@ -76,17 +79,36 @@ static void pwm_backlight_power_off(struct pwm_bl_data *pb) pb->enabled = false; } -static int compute_duty_cycle(struct pwm_bl_data *pb, int brightness) +static int scale(struct pwm_bl_data *pb, int x) { unsigned int lth = pb->lth_brightness; + + return (x * (pb->period - lth) / pb->scale) + lth; +} + +static int compute_duty_cycle(struct pwm_bl_data *pb, int brightness) +{ + int coarse = brightness / NSTEPS; + int fine = brightness % NSTEPS; int duty_cycle; - if (pb->levels) - duty_cycle = pb->levels[brightness]; - else - duty_cycle = brightness; + if (pb->levels) { + if (pb->piecewise) { + duty_cycle = scale(pb, pb->levels[coarse]); + if (fine > 0) + duty_cycle += (scale(pb, pb->levels[coarse + 1]) + - scale(pb, pb->levels[coarse])) + * fine / NSTEPS; + dev_dbg(pb->dev, "brightness=%d coarse=%d fine=%d\n", + brightness, coarse, fine); + } else { + duty_cycle = scale(pb, pb->levels[brightness]); + } + } else { + duty_cycle = scale(pb, brightness); + } - return (duty_cycle * (pb->period - lth) / pb->scale) + lth; + return duty_cycle; } static int pwm_backlight_update_status(struct backlight_device *bl) @@ -149,11 +171,11 @@ static int pwm_backlight_parse_dt(struct device *dev, if (!prop) return -EINVAL; - data->max_brightness = length / sizeof(u32); + data->levels_count = length / sizeof(u32); /* read brightness levels from DT property */ - if (data->max_brightness > 0) { - size_t size = sizeof(*data->levels) * data->max_brightness; + if (data->levels_count > 0) { + size_t size = sizeof(*data->levels) * data->levels_count; data->levels = devm_kzalloc(dev, size, GFP_KERNEL); if (!data->levels) @@ -161,7 +183,7 @@ static int pwm_backlight_parse_dt(struct device *dev, ret = of_property_read_u32_array(node, "brightness-levels", data->levels, - data->max_brightness); + data->levels_count); if (ret < 0) return ret; @@ -170,10 +192,18 @@ static int pwm_backlight_parse_dt(struct device *dev, if (ret < 0) return ret; + data->piecewise = of_property_read_bool(node, + "use-linear-interpolation"); + data->dft_brightness = value; - data->max_brightness--; + data->levels_count--; } + if (data->piecewise) + data->max_brightness = data->levels_count * NSTEPS; + else + data->max_brightness = data->levels_count; + data->enable_gpio = -EINVAL; return 0; } @@ -258,7 +288,7 @@ static int pwm_backlight_probe(struct platform_device *pdev) if (data->levels) { unsigned int i; - for (i = 0; i <= data->max_brightness; i++) + for (i = 0; i <= data->levels_count; i++) if (data->levels[i] > pb->scale) pb->scale = data->levels[i]; @@ -272,6 +302,7 @@ static int pwm_backlight_probe(struct platform_device *pdev) pb->exit = data->exit; pb->dev = &pdev->dev; pb->enabled = false; + pb->piecewise = data->piecewise; pb->enable_gpio = devm_gpiod_get_optional(&pdev->dev, "enable", GPIOD_ASIS); diff --git a/include/linux/pwm_backlight.h b/include/linux/pwm_backlight.h index e8afbd7..444a91b 100644 --- a/include/linux/pwm_backlight.h +++ b/include/linux/pwm_backlight.h @@ -14,6 +14,8 @@ struct platform_pwm_backlight_data { unsigned int lth_brightness; unsigned int pwm_period_ns; unsigned int *levels; + unsigned int levels_count; + bool piecewise; /* TODO remove once all users are switched to gpiod_* API */ int enable_gpio; int (*init)(struct device *dev);
Setting use-linear-interpolation in the dts will allow you to have linear interpolation between values of brightness-levels. There are now 256 between each of the values of brightness-levels. If something is requested halfway between 2 values, we'll use linear interpolation. This way a high resolution pwm duty cycle can be used without having to list out every possible value in the dts. This system also allows for gamma corrected values (eg: "brightness-levels = <0 2 4 8 16 32>;"). Patch based on the Alexandru M Stan work done for ChromeOS kernels. Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com> --- .../bindings/leds/backlight/pwm-backlight.txt | 2 + drivers/video/backlight/pwm_bl.c | 55 +++++++++++++++++----- include/linux/pwm_backlight.h | 2 + 3 files changed, 47 insertions(+), 12 deletions(-)