Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932112AbdLOOk7 (ORCPT ); Fri, 15 Dec 2017 09:40:59 -0500 Received: from mail-wm0-f67.google.com ([74.125.82.67]:33570 "EHLO mail-wm0-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755426AbdLOOk5 (ORCPT ); Fri, 15 Dec 2017 09:40:57 -0500 X-Google-Smtp-Source: ACJfBotmJdbaer1AEtRbSbRwOs8NUUIdKB0NLWbVwQRxo9H9uLV8NWrn+L5P+yVoiyal5PI1QBj8tA== Date: Fri, 15 Dec 2017 14:40:52 +0000 From: Daniel Thompson To: Enric Balletbo i Serra Cc: Jingoo Han , Richard Purdie , Jacek Anaszewski , Pavel Machek , Rob Herring , Doug Anderson , Brian Norris , Guenter Roeck , Lee Jones , Alexandru Stan , linux-leds@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [RFC v2 1/2] backlight: pwm_bl: linear interpolation between values of brightness-levels Message-ID: <20171215144052.3b62gd3bkiqtuvk7@oak.lan> References: <20171116141151.21171-1-enric.balletbo@collabora.com> <20171116141151.21171-2-enric.balletbo@collabora.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20171116141151.21171-2-enric.balletbo@collabora.com> User-Agent: NeoMutt/20170113 (1.7.2) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5609 Lines: 157 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 > --- > .../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 > #include > > +#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.