Re: [PATCH 2/2] backlight: pwm_bl: Get number of brightness levels for CIE 1931 from the device tree
From: Daniel Thompson <hidden>
Date: 2019-06-11 15:33:22
Also in:
dri-devel, linux-devicetree, linux-leds, linux-pwm, lkml
On Mon, Jun 10, 2019 at 04:37:39PM -0700, Matthias Kaehlcke wrote:
Commit 88ba95bedb79 ("backlight: pwm_bl: Compute brightness of LED
linearly to human eye") uses pwm_period / hweight32(pwm_period) as
as heuristic to determine the number of brightness levels when the DT
doesn't provide a brightness level table. This heuristic is broken
and can result in excessively large brightness tables.
Instead of using the heuristic try to retrieve the number of
brightness levels from the device tree (property 'max-brightness'
+ 1). If the value is not specified use a default of 256 levels.I'll look at the code tomorrow but why 256? To me it feels simultaneously too big for a simple 8-bit PWM and too small for animated backlight effects. I certainly agree that an override could be useful but I'm not clear why deriving a default based on the period is bogus (and the description is merely concerned about uselessly big tables). /* * Once we have 4096 levels there's little point going much higher... * neither interactive sliders nor animation benefits from having * more values in the table. */ max_brightness = min(DIV_ROUND_UP(period, ffs(period), 4096); Daniel.
quoted hunk ↗ jump to hunk
Fixes: 88ba95bedb79 ("backlight: pwm_bl: Compute brightness of LED linearly to human eye") Signed-off-by: Matthias Kaehlcke <mka@chromium.org> --- drivers/video/backlight/pwm_bl.c | 59 ++++++++++++-------------------- 1 file changed, 21 insertions(+), 38 deletions(-)diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c index fb45f866b923..2913cbe9cfcb 100644 --- a/drivers/video/backlight/pwm_bl.c +++ b/drivers/video/backlight/pwm_bl.c@@ -194,38 +194,19 @@ int pwm_backlight_brightness_default(struct device *dev, struct platform_pwm_backlight_data *data, unsigned int period) { - unsigned int counter = 0; - unsigned int i, n; + unsigned int i; + unsigned int nlevels = data->max_brightness + 1; u64 retval; - /* - * Count the number of bits needed to represent the period number. The - * number of bits is used to calculate the number of levels used for the - * brightness-levels table, the purpose of this calculation is have a - * pre-computed table with enough levels to get linear brightness - * perception. The period is divided by the number of bits so for a - * 8-bit PWM we have 255 / 8 = 32 brightness levels or for a 16-bit PWM - * we have 65535 / 16 = 4096 brightness levels. - * - * Note that this method is based on empirical testing on different - * devices with PWM of 8 and 16 bits of resolution. - */ - n = period; - while (n) { - counter += n % 2; - n >>= 1; - } - - data->max_brightness = DIV_ROUND_UP(period, counter); - data->levels = devm_kcalloc(dev, data->max_brightness, + data->levels = devm_kcalloc(dev, nlevels, sizeof(*data->levels), GFP_KERNEL); if (!data->levels) return -ENOMEM; /* Fill the table using the cie1931 algorithm */ - for (i = 0; i < data->max_brightness; i++) { + for (i = 0; i < nlevels; i++) { retval = cie1931((i * PWM_LUMINANCE_SCALE) / - data->max_brightness, PWM_LUMINANCE_SCALE) * + nlevels, PWM_LUMINANCE_SCALE) * period; retval = DIV_ROUND_CLOSEST_ULL(retval, PWM_LUMINANCE_SCALE); if (retval > UINT_MAX)@@ -233,8 +214,7 @@ int pwm_backlight_brightness_default(struct device *dev, data->levels[i] = (unsigned int)retval; } - data->dft_brightness = data->max_brightness / 2; - data->max_brightness--; + data->dft_brightness = nlevels / 2; return 0; }@@ -272,8 +252,13 @@ static int pwm_backlight_parse_dt(struct device *dev, * set a default table of brightness levels will be used. */ prop = of_find_property(node, "brightness-levels", &length); - if (!prop) + if (!prop) { + if (of_property_read_u32(node, "max-brightness", + &data->max_brightness)) + data->max_brightness = 255; + return 0; + } data->max_brightness = length / sizeof(u32);@@ -565,13 +550,10 @@ static int pwm_backlight_probe(struct platform_device *pdev) pb->levels = data->levels; } - } else if (!data->max_brightness) { + } else if (node) { /* - * If no brightness levels are provided and max_brightness is - * not set, use the default brightness table. For the DT case, - * max_brightness is set to 0 when brightness levels is not - * specified. For the non-DT case, max_brightness is usually - * set to some value. + * If no brightness levels are provided use the default + * brightness table. */ /* Get the PWM period (in nanoseconds) */@@ -591,12 +573,13 @@ static int pwm_backlight_probe(struct platform_device *pdev) pb->levels = data->levels; } - } else { - /* - * That only happens for the non-DT case, where platform data - * sets the max_brightness value. - */ + } else if (data->max_brightness) { + /* non-DT case, max_brightness value set in platform data. */ pb->scale = data->max_brightness; + } else { + dev_err(&pdev->dev, "max brightness is not specified\n"); + ret = -EINVAL; + goto err_alloc; } pb->lth_brightness = data->lth_brightness * (state.period / pb->scale);-- 2.22.0.rc2.383.gf4fbbf30c2-goog