Re: [PATCH v3 6/9] leds: multicolor: Introduce a multicolor class definition

[Dan Murphy]

Jacek

Thanks for the review

On 6/20/19 11:10 AM, Jacek Anaszewski wrote:
> Hi Dan,
>
> Thank you for the v5.
>
> I will confine myself to commenting only some parts since
> the rest will undergo rework due to removal of sync API.
>
> On 5/23/19 9:08 PM, Dan Murphy wrote:
> > Introduce a multicolor class that groups colored LEDs
> > within a LED node.
> >
> > The framework allows for dynamically setting individual LEDs
> > or setting brightness levels of LEDs and updating them virtually
> > simultaneously.
> >
> > Signed-off-by: Dan Murphy <dmurphy@xxxxxx>
> > ---
> > Â drivers/leds/KconfigÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ |Â 10 +
> > Â drivers/leds/MakefileÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ |ÂÂ 1 +
> >  drivers/leds/led-class-multicolor.c | 421 +++++++++++++++++++++++++++
> > Â include/linux/led-class-multicolor.h |Â 95 ++++++
> > Â 4 files changed, 527 insertions(+)
> > Â create mode 100644 drivers/leds/led-class-multicolor.c
> > Â create mode 100644 include/linux/led-class-multicolor.h
> >
> > diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> > index 0414adebb177..0696a13c9527 100644
> > --- a/drivers/leds/Kconfig
> > +++ b/drivers/leds/Kconfig
> > @@ -29,6 +29,16 @@ config LEDS_CLASS_FLASH
> > ÂÂÂÂÂÂÂ for the flash related features of a LED device. It can be built
> > ÂÂÂÂÂÂÂ as a module.
> > Â +config LEDS_CLASS_MULTI_COLOR
> > +ÂÂÂ tristate "LED Mulit Color LED Class Support"
> > +ÂÂÂ depends on LEDS_CLASS
> > +ÂÂÂ help
> > +ÂÂÂÂÂ This option enables the multicolor LED sysfs class in 
> > /sys/class/leds.
> > +ÂÂÂÂÂ It wraps LED Class and adds multicolor LED specific sysfs 
> > attributes
> > +ÂÂÂÂÂ and kernel internal API to it. You'll need this to provide 
> > support
> > +ÂÂÂÂÂ for multicolor LEDs that are grouped together. This class is not
> > + intended for single color LEDs. It can be built as a module.
>
> extra whitespace:
>
> s/ It can/It can/

Ack

> [...]
> > +
> > +static int multicolor_set_brightness(struct led_classdev *led_cdev,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ enum led_brightness brightness)
> > +{
> > +ÂÂÂ struct led_classdev_mc *mcled_cdev = lcdev_to_mccdev(led_cdev);
> > +ÂÂÂ struct led_classdev_mc_data *data = mcled_cdev->data;
> > +ÂÂÂ struct led_multicolor_ops *ops = mcled_cdev->ops;
> > +ÂÂÂ struct led_classdev_mc_priv *priv;
> > +ÂÂÂ unsigned long state = brightness;
> > +ÂÂÂ int adj_value;
> > +ÂÂÂ ssize_t ret = -EINVAL;
> > +
> > +ÂÂÂ mutex_lock(&led_cdev->led_access);
> > +
> > +ÂÂÂ if (ops->set_module_brightness) {
> > +ÂÂÂÂÂÂÂ ret = ops->set_module_brightness(mcled_cdev, state);
> > +ÂÂÂÂÂÂÂ goto unlock;
> > +ÂÂÂ }
> > +
> > +ÂÂÂ list_for_each_entry(priv, &data->color_list, list) {
> > +ÂÂÂÂÂÂÂ if (state && priv->brightness && priv->max_brightness) {
> > +ÂÂÂÂÂÂÂÂÂÂÂ adj_value = state * ((priv->brightness * 100) / 
> > priv->max_brightness);
> > +ÂÂÂÂÂÂÂÂÂÂÂ adj_value = adj_value / 100;
>
> Why the multiplication an then division by 100? And priv->max_brightness
> stays unaltered? This changes the proportions. My python script works
> just fine without those.

Because the kernel does not do floating point math and the calculation 
is using the ratio

between the intensity and max_intensity and multiplying against the 
requested brightness.

priv->intensity = 100 (This is the current intensity of the color LED)

priv->max_intensity = 255

state = 80 (This is the requested cluster brightness)

100/255 = 0.392 which is 0.

0 * 80 = 0 this is not what the value should be

But with the multiplier.

10000/255 = 39.2 which is 39 which means that the intensity is only 39% 
of the

max_intensity.

39 * 80 = 3120Â So to preserve the 39% from the 80 we multiply the 
percentage * requested cluster brightness

3120 / 100 = 31 then we normalize back

I am not sure how your script is working without the multiplier.


> > +ÂÂÂÂÂÂÂ } else
> > +ÂÂÂÂÂÂÂÂÂÂÂ adj_value = LED_OFF;
> > +
> > +ÂÂÂÂÂÂÂ ret = ops->set_color_brightness(priv->mcled_cdev,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ priv->color_id,ÂÂÂ adj_value);
> > +ÂÂÂÂÂÂÂ if (ret < 0)
> > +ÂÂÂÂÂÂÂÂÂÂÂ goto unlock;
> > +ÂÂÂ }
> > +
> > +unlock:
> > +ÂÂÂ mutex_unlock(&led_cdev->led_access);
> > +ÂÂÂ return ret;
> > +}
> [...]
> > +int led_classdev_multicolor_register_ext(struct device *parent,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct led_classdev_mc *mcled_cdev,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct led_init_data *init_data)
> > +{
> > +ÂÂÂ struct led_classdev *led_cdev;
> > +ÂÂÂ struct led_multicolor_ops *ops;
> > +ÂÂÂ struct led_classdev_mc_data *data;
> > +ÂÂÂ int ret;
> > +ÂÂÂ int i;
> > +
> > +ÂÂÂ if (!mcled_cdev)
> > +ÂÂÂÂÂÂÂ return -EINVAL;
> > +
> > +ÂÂÂ ops = mcled_cdev->ops;
> > +ÂÂÂ if (!ops || !ops->set_color_brightness)
> > +ÂÂÂÂÂÂÂ return -EINVAL;
> > +
> > +ÂÂÂ data = kzalloc(sizeof(*data), GFP_KERNEL);
> > +ÂÂÂ if (!data)
> > +ÂÂÂÂÂÂÂ return -ENOMEM;
> > +
> > +ÂÂÂ mcled_cdev->data = data;
> > +ÂÂÂ led_cdev = &mcled_cdev->led_cdev;
> > +
> > +ÂÂÂ if (led_cdev->brightness_set_blocking)
> > +ÂÂÂÂÂÂÂ led_cdev->brightness_set_blocking = multicolor_set_brightness;
>
> This is weird. In leds-lp50xx.c you don't initialize
> brightness_set_blocking and this still works?

I will have to look. I don't believe I retested this on lp50xx only the 
lp55xx code.

> I believe this is kind of omission.
>
> And it is not reasonable to just override driver supplied op with
> generic one just like that.
>
> I propose to initialize brightness_set or brightness_set_blocking
> op as we used to do it for monochrome LEDs. Those function(s) on
> driver side will either use device's hardware support for setting
> color lightness, or will call a generic function provided by
> LED multi color class for calculating intensities of all colors
> it comprises in the cluster.
>
> I know this is different to what we've discussed on IRC, but now
> it looks for me the most reasonable way to go.

So you want the device driver to handle the brightness request and call 
into the framework for

calculating the color intensities?

That would work as well and solves a problem of HW supported brightness 
control like the LP50xx.

The LP50xx would not need to call into the function for calculated 
intensities.


> > + INIT_LIST_HEAD(&data->color_list);
> > +
> > +ÂÂÂ /* Register led class device */
> > +ÂÂÂ ret = led_classdev_register_ext(parent, led_cdev, init_data);
> > +ÂÂÂ if (ret)
> > +ÂÂÂÂÂÂÂ return ret;
> > +
> > +ÂÂÂ ret = led_multicolor_init_color_dir(data, mcled_cdev);
> > +ÂÂÂ if (ret)
> > +ÂÂÂÂÂÂÂ return ret;
> > +
> > +ÂÂÂ /* Select the sysfs attributes to be created for the device */
> > +ÂÂÂ for (i = 0; i < mcled_cdev->num_leds; i++) {
> > +ÂÂÂÂÂÂÂ ret = led_multicolor_init_color(data, mcled_cdev,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ mcled_cdev->available_colors[i]);
> > +ÂÂÂÂÂÂÂ if (ret)
> > +ÂÂÂÂÂÂÂÂÂÂÂ break;
> > +ÂÂÂ }
> > +
> > +ÂÂÂ return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(led_classdev_multicolor_register_ext);
> > +
> > +void led_classdev_multicolor_unregister(struct led_classdev_mc 
> > *mcled_cdev)
> > +{
> > +ÂÂÂ if (!mcled_cdev)
> > +ÂÂÂÂÂÂÂ return;
> > +
> > +ÂÂÂ led_classdev_unregister(&mcled_cdev->led_cdev);
> > +}
> > +EXPORT_SYMBOL_GPL(led_classdev_multicolor_unregister);
> > +
> > +static void devm_led_classdev_multicolor_release(struct device *dev, 
> > void *res)
> > +{
> > +ÂÂÂ led_classdev_multicolor_unregister(*(struct led_classdev_mc 
> > **)res);
> > +}
> > +
> > +/**
> > + * devm_of_led_classdev_register - resource managed 
> > led_classdev_register()
> > + *
> > + * @parent: parent of LED device
> > + * @led_cdev: the led_classdev structure for this device.
> > + */
> > +int devm_led_classdev_multicolor_register(struct device *parent,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct led_classdev_mc *mcled_cdev)
> > +{
> > +ÂÂÂ struct led_classdev_mc **dr;
> > +ÂÂÂ int ret;
> > +
> > +ÂÂÂ dr = devres_alloc(devm_led_classdev_multicolor_release,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂ sizeof(*dr), GFP_KERNEL);
> > +ÂÂÂ if (!dr)
> > +ÂÂÂÂÂÂÂ return -ENOMEM;
> > +
> > +ÂÂÂ ret = led_classdev_multicolor_register(parent, mcled_cdev);
> > +ÂÂÂ if (ret) {
> > +ÂÂÂÂÂÂÂ devres_free(dr);
> > +ÂÂÂÂÂÂÂ return ret;
> > +ÂÂÂ }
> > +
> > +ÂÂÂ *dr = mcled_cdev;
> > +ÂÂÂ devres_add(parent, dr);
> > +
> > +ÂÂÂ return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(devm_led_classdev_multicolor_register);
> > +
> > +static int devm_led_classdev_multicolor_match(struct device *dev,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ void *res, void *data)
> > +{
> > +ÂÂÂ struct mcled_cdev **p = res;
> > +
> > +ÂÂÂ if (WARN_ON(!p || !*p))
> > +ÂÂÂÂÂÂÂ return 0;
> > +
> > +ÂÂÂ return *p == data;
> > +}
> > +
> > +/**
> > + * devm_led_classdev_multicolor_unregister() - resource managed
> > + *ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ led_classdev_multicolor_unregister()
> > + * @parent: The device to unregister.
> > + * @mcled_cdev: the led_classdev_mc structure for this device.
> > + */
> > +void devm_led_classdev_multicolor_unregister(struct device *dev,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct led_classdev_mc *mcled_cdev)
> > +{
> > +ÂÂÂ WARN_ON(devres_release(dev,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ devm_led_classdev_multicolor_release,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ devm_led_classdev_multicolor_match, mcled_cdev));
> > +}
> > +EXPORT_SYMBOL_GPL(devm_led_classdev_multicolor_unregister);
> > +
> > +MODULE_AUTHOR("Dan Murphy <dmurphy@xxxxxx>");
> > +MODULE_DESCRIPTION("Multi Color LED class interface");
> > +MODULE_LICENSE("GPL v2");
> > diff --git a/include/linux/led-class-multicolor.h 
> > b/include/linux/led-class-multicolor.h
> > new file mode 100644
> > index 000000000000..f9e71d984b03
> > --- /dev/null
> > +++ b/include/linux/led-class-multicolor.h
> > @@ -0,0 +1,95 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +/* LED Multicolor class interface
> > + * Copyright (C) 2019 Texas Instruments Incorporated - 
> > http://www.ti.com/
> > + */
>
> Let's have C++ comment style also here.
ACK


> > +#ifndef __LINUX_MULTICOLOR_LEDS_H_INCLUDED
> > +#define __LINUX_MULTICOLOR_LEDS_H_INCLUDED
> > +
> > +#include <linux/leds.h>
> > +#include <dt-bindings/leds/common.h>
> [...]