| Message ID | 20200417113312.24340-5-tomi.valkeinen@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/5] backlight: led_bl: fix cosmetic issues | expand |
On Fri, Apr 17, 2020 at 02:33:12PM +0300, Tomi Valkeinen wrote: > led_bl_parse_levels() is rather difficult to follow. Rewrite it with a > more obvious code flow. ... that introduces new behaviour. There's a couple of new behaviours here but the one that particular attracted my attention is the disregarding the "default-brightness-level" if there is no table. That looks like a bug to me. Please can you add any intended changes of behaviour in the patch header? Daniel. > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> > --- > drivers/video/backlight/led_bl.c | 63 ++++++++++++++++---------------- > 1 file changed, 32 insertions(+), 31 deletions(-) > > diff --git a/drivers/video/backlight/led_bl.c b/drivers/video/backlight/led_bl.c > index 021b5edd895c..7b3889035540 100644 > --- a/drivers/video/backlight/led_bl.c > +++ b/drivers/video/backlight/led_bl.c > @@ -132,50 +132,51 @@ static int led_bl_parse_levels(struct device *dev, > int num_levels; > u32 value; > int ret; > + int i; > + u32 *levels; > > if (!node) > return -ENODEV; > > num_levels = of_property_count_u32_elems(node, "brightness-levels"); > - if (num_levels > 1) { > - int i; > - unsigned int db; > - u32 *levels; > - > - levels = devm_kzalloc(dev, sizeof(u32) * num_levels, > - GFP_KERNEL); > - if (!levels) > - return -ENOMEM; > - > - ret = of_property_read_u32_array(node, "brightness-levels", > - levels, > - num_levels); > - if (ret < 0) > - return ret; > - > - /* > - * Try to map actual LED brightness to backlight brightness > - * level > - */ > - db = priv->default_brightness; > + > + if (num_levels < 0) > + return 0; > + > + if (num_levels == 0) { > + dev_warn(dev, "No brightness-levels defined\n"); > + return -EINVAL; > + } > + > + levels = devm_kzalloc(dev, sizeof(u32) * num_levels, > + GFP_KERNEL); > + if (!levels) > + return -ENOMEM; > + > + ret = of_property_read_u32_array(node, "brightness-levels", > + levels, > + num_levels); > + if (ret < 0) > + return ret; > + > + priv->max_brightness = num_levels - 1; > + priv->levels = levels; > + > + ret = of_property_read_u32(node, "default-brightness-level", &value); > + if (!ret) { > + priv->default_brightness = min(value, priv->max_brightness); > + } else { > + /* Map LED default-brightness to backlight brightness level */ > + unsigned int db = priv->default_brightness; > + > for (i = 0 ; i < num_levels; i++) { > if ((i == 0 || db > levels[i - 1]) && db <= levels[i]) > break; > } > > priv->default_brightness = i < num_levels ? i : 0; > - priv->max_brightness = num_levels - 1; > - priv->levels = levels; > - } else if (num_levels >= 0) { > - dev_warn(dev, "Not enough levels defined\n"); > } > > - ret = of_property_read_u32(node, "default-brightness-level", &value); > - if (!ret && value <= priv->max_brightness) > - priv->default_brightness = value; > - else if (!ret && value > priv->max_brightness) > - dev_warn(dev, "Invalid default brightness. Ignoring it\n"); > - > return 0; > } > > -- > Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. > Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki >
On 20/04/2020 19:01, Daniel Thompson wrote: > On Fri, Apr 17, 2020 at 02:33:12PM +0300, Tomi Valkeinen wrote: >> led_bl_parse_levels() is rather difficult to follow. Rewrite it with a >> more obvious code flow. > > ... that introduces new behaviour. > > There's a couple of new behaviours here but the one that particular > attracted my attention is the disregarding the "default-brightness-level" if > there is no table. That looks like a bug to me. I think the previous behavior was a (minor) bug: how can there be default brightness level if there are no brightness levels? The led-backlight.txt is a bit lacking (another thing to improve...) but led-backlight mimics pwm-backlight, and pwm-backlight.txt says default-brightness-level: The default brightness level (index into the array defined by the "brightness-levels" property) But I agree, it's a change, so good to mention. > Please can you add any intended changes of behaviour in the patch > header? Ok. Tomi
On Tue, Apr 21, 2020 at 08:52:02AM +0300, Tomi Valkeinen wrote: > On 20/04/2020 19:01, Daniel Thompson wrote: > > On Fri, Apr 17, 2020 at 02:33:12PM +0300, Tomi Valkeinen wrote: > > > led_bl_parse_levels() is rather difficult to follow. Rewrite it with a > > > more obvious code flow. > > > > ... that introduces new behaviour. > > > > There's a couple of new behaviours here but the one that particular > > attracted my attention is the disregarding the "default-brightness-level" if > > there is no table. That looks like a bug to me. > > I think the previous behavior was a (minor) bug: how can there be default > brightness level if there are no brightness levels? I don't think this was a bug. If there is no brightness table then backlight will adopt a 1:1 mapping versus the underlying LED meaning the concept of default brightness applies equally well whether or not a brightness table is supplied. > The led-backlight.txt is > a bit lacking (another thing to improve...) but led-backlight mimics > pwm-backlight, and pwm-backlight.txt says > > default-brightness-level: The default brightness level (index into the array > defined by the "brightness-levels" property) I think this implies we should improve the binding documentation! The parenthetic text's main purpose is to make clear which scale should be used when interpreting the default brightness. Just because the scale is 1:1 doesn't render it meaningless. Daniel.
On 21/04/2020 13:48, Daniel Thompson wrote: > On Tue, Apr 21, 2020 at 08:52:02AM +0300, Tomi Valkeinen wrote: >> On 20/04/2020 19:01, Daniel Thompson wrote: >>> On Fri, Apr 17, 2020 at 02:33:12PM +0300, Tomi Valkeinen wrote: >>>> led_bl_parse_levels() is rather difficult to follow. Rewrite it with a >>>> more obvious code flow. >>> >>> ... that introduces new behaviour. >>> >>> There's a couple of new behaviours here but the one that particular >>> attracted my attention is the disregarding the "default-brightness-level" if >>> there is no table. That looks like a bug to me. >> >> I think the previous behavior was a (minor) bug: how can there be default >> brightness level if there are no brightness levels? > > I don't think this was a bug. > > If there is no brightness table then backlight will adopt a 1:1 mapping > versus the underlying LED meaning the concept of default brightness > applies equally well whether or not a brightness table is supplied. > > >> The led-backlight.txt is >> a bit lacking (another thing to improve...) but led-backlight mimics >> pwm-backlight, and pwm-backlight.txt says >> >> default-brightness-level: The default brightness level (index into the array >> defined by the "brightness-levels" property) > > I think this implies we should improve the binding documentation! > > The parenthetic text's main purpose is to make clear which scale should > be used when interpreting the default brightness. Just because the scale > is 1:1 doesn't render it meaningless. If I read pwm_bl.c right, that's not how the code works. If pwm_bl has no 'brightness-levels', then 'default-brightness-level' is ignored. Which matches the binding documentation. What you suggest makes sense, though, so I can adjust this patch to make led_bl behave like that. Tomi
On 21/04/2020 14:26, Tomi Valkeinen wrote: >>> The led-backlight.txt is >>> a bit lacking (another thing to improve...) but led-backlight mimics >>> pwm-backlight, and pwm-backlight.txt says >>> >>> default-brightness-level: The default brightness level (index into the array >>> defined by the "brightness-levels" property) >> >> I think this implies we should improve the binding documentation! >> >> The parenthetic text's main purpose is to make clear which scale should >> be used when interpreting the default brightness. Just because the scale >> is 1:1 doesn't render it meaningless. > > If I read pwm_bl.c right, that's not how the code works. If pwm_bl has no 'brightness-levels', then > 'default-brightness-level' is ignored. Which matches the binding documentation. > > What you suggest makes sense, though, so I can adjust this patch to make led_bl behave like that. On the other hand... If we want to use LED's (or PWM's) brightness levels directly, should the default brightness be defined in the LED's (or PWM's) DT node? And only if we create a backlight brightness-levels mapping, we need to add a new property to define a default for those levels. Which, I think, the name implies with the "-level". Well, in any case, there should be no harm in using 'default-brightness-level' also for the 1:1 mapping without the 'brightness-levels'. So maybe that's the best route. Tomi
diff --git a/drivers/video/backlight/led_bl.c b/drivers/video/backlight/led_bl.c index 021b5edd895c..7b3889035540 100644 --- a/drivers/video/backlight/led_bl.c +++ b/drivers/video/backlight/led_bl.c @@ -132,50 +132,51 @@ static int led_bl_parse_levels(struct device *dev, int num_levels; u32 value; int ret; + int i; + u32 *levels; if (!node) return -ENODEV; num_levels = of_property_count_u32_elems(node, "brightness-levels"); - if (num_levels > 1) { - int i; - unsigned int db; - u32 *levels; - - levels = devm_kzalloc(dev, sizeof(u32) * num_levels, - GFP_KERNEL); - if (!levels) - return -ENOMEM; - - ret = of_property_read_u32_array(node, "brightness-levels", - levels, - num_levels); - if (ret < 0) - return ret; - - /* - * Try to map actual LED brightness to backlight brightness - * level - */ - db = priv->default_brightness; + + if (num_levels < 0) + return 0; + + if (num_levels == 0) { + dev_warn(dev, "No brightness-levels defined\n"); + return -EINVAL; + } + + levels = devm_kzalloc(dev, sizeof(u32) * num_levels, + GFP_KERNEL); + if (!levels) + return -ENOMEM; + + ret = of_property_read_u32_array(node, "brightness-levels", + levels, + num_levels); + if (ret < 0) + return ret; + + priv->max_brightness = num_levels - 1; + priv->levels = levels; + + ret = of_property_read_u32(node, "default-brightness-level", &value); + if (!ret) { + priv->default_brightness = min(value, priv->max_brightness); + } else { + /* Map LED default-brightness to backlight brightness level */ + unsigned int db = priv->default_brightness; + for (i = 0 ; i < num_levels; i++) { if ((i == 0 || db > levels[i - 1]) && db <= levels[i]) break; } priv->default_brightness = i < num_levels ? i : 0; - priv->max_brightness = num_levels - 1; - priv->levels = levels; - } else if (num_levels >= 0) { - dev_warn(dev, "Not enough levels defined\n"); } - ret = of_property_read_u32(node, "default-brightness-level", &value); - if (!ret && value <= priv->max_brightness) - priv->default_brightness = value; - else if (!ret && value > priv->max_brightness) - dev_warn(dev, "Invalid default brightness. Ignoring it\n"); - return 0; }
led_bl_parse_levels() is rather difficult to follow. Rewrite it with a more obvious code flow. Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> --- drivers/video/backlight/led_bl.c | 63 ++++++++++++++++---------------- 1 file changed, 32 insertions(+), 31 deletions(-)