Re: [PATCH v10 12/16] leds: lp55xx: Add multicolor framework support to lp55xx

From: Jacek Anaszewski
Date: Mon Oct 07 2019 - 15:27:42 EST

Next message: Thomas Hellstrom: "Re: [PATCH] x86/cpu/vmware: use the full form of inl in VMWARE_PORT"
Previous message: Linus Torvalds: "Re: WARNING in filldir64"
In reply to: Dan Murphy: "Re: [PATCH v10 12/16] leds: lp55xx: Add multicolor framework support to lp55xx"
Next in thread: Dan Murphy: "[PATCH v10 01/16] dt: bindings: Add multicolor class dt bindings documention"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Dan,

On 10/7/19 7:08 PM, Dan Murphy wrote:
> Jacek
>
> On 10/6/19 2:52 PM, Jacek Anaszewski wrote:
>> Dan,
>>
>> On 10/1/19 4:56 PM, Dan Murphy wrote:
>>> Add multicolor framework support for the lp55xx family.
>>>
>>> Signed-off-by: Dan Murphy <dmurphy@xxxxxx>
>>> ---
>>> Â drivers/leds/KconfigÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ |ÂÂ 1 +
>>> Â drivers/leds/leds-lp55xx-common.cÂÂÂÂÂÂÂÂ | 169 +++++++++++++++++++---
>>> Â drivers/leds/leds-lp55xx-common.hÂÂÂÂÂÂÂÂ |Â 11 ++
>>> Â include/linux/platform_data/leds-lp55xx.h |ÂÂ 6 +
>>> Â 4 files changed, 163 insertions(+), 24 deletions(-)
>>>
>>> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
>>> index 84f60e35c5ee..dc3d9f2194cd 100644
>>> --- a/drivers/leds/Kconfig
>>> +++ b/drivers/leds/Kconfig
>>> @@ -377,6 +377,7 @@ config LEDS_LP50XX
>>> Â config LEDS_LP55XX_COMMON
>>> ÂÂÂÂÂ tristate "Common Driver for TI/National
>>> LP5521/5523/55231/5562/8501"
>>> ÂÂÂÂÂ depends on LEDS_LP5521 || LEDS_LP5523 || LEDS_LP5562 ||
>>> LEDS_LP8501
>>> +ÂÂÂ depends on LEDS_CLASS_MULTI_COLOR && OF
>>> ÂÂÂÂÂ select FW_LOADER
>>> ÂÂÂÂÂ select FW_LOADER_USER_HELPER
>>> ÂÂÂÂÂ help
>>> diff --git a/drivers/leds/leds-lp55xx-common.c
>>> b/drivers/leds/leds-lp55xx-common.c
>>> index 44ced02b49f9..5de4f1789a44 100644
>>> --- a/drivers/leds/leds-lp55xx-common.c
>>> +++ b/drivers/leds/leds-lp55xx-common.c
>>> @@ -131,14 +131,50 @@ static struct attribute *lp55xx_led_attrs[] = {
>>> Â };
>>> Â ATTRIBUTE_GROUPS(lp55xx_led);
>>> Â +struct led_mc_color_conversion
>>> color_component[LP55XX_MAX_GROUPED_CHAN];
>> Why is this global? Please move it to lp55xx_set_brightness().
> ACK
>>
>>> +
>>> +static int lp55xx_get_channel(struct lp55xx_led *led, int color_id)
>>> +{
>>> +ÂÂÂ int i;
>>> +
>>> +ÂÂÂ for (i = 0; i < led->mc_cdev.num_leds; i++) {
>>> +ÂÂÂÂÂÂÂ if (led->channel_color[i] == color_id)
>>> +ÂÂÂÂÂÂÂÂÂÂÂ return led->grouped_channels[i];
>>> +ÂÂÂ }
>>> +
>>> +ÂÂÂ return -EINVAL;
>>> +}
>>> +
>>> Â static int lp55xx_set_brightness(struct led_classdev *cdev,
>>> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ enum led_brightness brightness)
>>> Â {
>>> ÂÂÂÂÂ struct lp55xx_led *led = cdev_to_lp55xx_led(cdev);
>>> ÂÂÂÂÂ struct lp55xx_device_config *cfg = led->chip->cfg;
>>> +ÂÂÂ int channel_num;
>>> +ÂÂÂ int ret;
>>> +ÂÂÂ int i;
>>> Â -ÂÂÂ led->brightness = (u8)brightness;
>>> -ÂÂÂ return cfg->brightness_fn(led);
>>> +ÂÂÂ if (led->mc_cdev.num_leds > 1) {
>>> +ÂÂÂÂÂÂÂ led_mc_calc_brightness(&led->mc_cdev, brightness,
>>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ &color_component[0]);
>> s/&color_component[0]/color_component/
>
> ACK
>
>
>>> +
>>> +ÂÂÂÂÂÂÂ for (i = 0; i < led->mc_cdev.num_leds; i++) {
>>> +ÂÂÂÂÂÂÂÂÂÂÂ channel_num = lp55xx_get_channel(led,
>>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ color_component[i].color_id);
>>> +ÂÂÂÂÂÂÂÂÂÂÂ if (channel_num < 0)
>>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ return channel_num;
>>> +
>>> +ÂÂÂÂÂÂÂÂÂÂÂ ret = cfg->color_intensity_fn(led, channel_num,
>>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ color_component[i].brightness);
>> If you passed struct led_mc_color_conversion instead of brightness,
>> then in the color_intensity_fn op you could obtain channel numbers
>> by calling lp55xx_get_channel in a loop. And you could setup the whole
>> cluster in a single call.
>
> Hmm.Â How would that be an improvement?Â Maybe the answer lies down

I mentioned that before - think of sleeping on mutex on contention
(keep it mind that brightness will be also set from triggers and
from the workqueue in effect, and userspace can interfere that).

That could add unpleasant delays between setting color components.
It could be worked around by executing the for loop here under mutex,
but would be non-uniform with regard to handling monochrome brightness
setting via cfg->brightness_fn, which relies on chip->lock taken by it.

You need one of two options: either the whole for loop here under mutex
or on the driver side.

> below in my response and we will not have to get the channel_num as we
> can make the output part of the mc_color_conversion struct.
>
> As I pointed out in v9 "Beyond that in coding this and thinking about
> the design it is better to have the lp55xx_common code to do all the
> heavy lifting and the children to just perform the action on the device
> itself"
>
> https://lore.kernel.org/linux-leds/4186e454-48fd-1578-cd26-083b54c707ab@xxxxxxxxx/T/#u
>
>
> The children should not have to know if the LED is registered to the LED
> class, MC Class or Flash class only the common code should know this
> information.Â Just need to keep the child code simple. This is why I
> pass in the values as opposed to having the child figure it out.

With mc class we have a bit different perspective - the children are not
standalone LED class devices. Of course it can be modeled this way as
well, but it can result in a misleading impression that the LEDs are
independent. If we set the all colors in one op for mc class case then
it would nicely highlight that use case on the driver side.

>
>>> +ÂÂÂÂÂÂÂÂÂÂÂ if (ret)
>>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ return ret;
>>> +ÂÂÂÂÂÂÂ }
>>> +ÂÂÂ } else {
>>> +ÂÂÂÂÂÂÂ led->brightness = (u8)brightness;
>>> +ÂÂÂÂÂÂÂ ret = cfg->brightness_fn(led);
>>> +ÂÂÂ }
>>> +
>>> +ÂÂÂ return ret;
>>> Â }
>>> Â Â static int lp55xx_init_led(struct lp55xx_led *led,
>>> @@ -147,9 +183,9 @@ static int lp55xx_init_led(struct lp55xx_led *led,
>>> ÂÂÂÂÂ struct lp55xx_platform_data *pdata = chip->pdata;
>>> ÂÂÂÂÂ struct lp55xx_device_config *cfg = chip->cfg;
>>> ÂÂÂÂÂ struct device *dev = &chip->cl->dev;
>>> +ÂÂÂ int max_channel = cfg->max_channel;
>>> ÂÂÂÂÂ char name[32];
>>> ÂÂÂÂÂ int ret;
>>> -ÂÂÂ int max_channel = cfg->max_channel;
>>> Â ÂÂÂÂÂ if (chan >= max_channel) {
>>> ÂÂÂÂÂÂÂÂÂ dev_err(dev, "invalid channel: %d / %d\n", chan, max_channel);
>>> @@ -159,10 +195,37 @@ static int lp55xx_init_led(struct lp55xx_led *led,
>>> ÂÂÂÂÂ if (pdata->led_config[chan].led_current == 0)
>>> ÂÂÂÂÂÂÂÂÂ return 0;
>>> Â +ÂÂÂ if (pdata->led_config[chan].name) {
>>> +ÂÂÂÂÂÂÂ led->cdev.name = pdata->led_config[chan].name;
>>> +ÂÂÂ } else {
>>> +ÂÂÂÂÂÂÂ snprintf(name, sizeof(name), "%s:channel%d",
>>> +ÂÂÂÂÂÂÂÂÂÂÂ pdata->label ? : chip->cl->name, chan);
>>> +ÂÂÂÂÂÂÂ led->cdev.name = name;
>>> +ÂÂÂ }
>>> +
>>> +ÂÂÂ if (pdata->led_config[chan].num_colors > 1) {
>>> +ÂÂÂÂÂÂÂ led->mc_cdev.led_cdev = &led->cdev;
>>> +ÂÂÂÂÂÂÂ led->cdev.brightness_set_blocking = lp55xx_set_brightness;
>>> +ÂÂÂÂÂÂÂ led->cdev.groups = lp55xx_led_groups;
>>> +ÂÂÂÂÂÂÂ led->mc_cdev.num_leds = pdata->led_config[chan].num_colors;
>>> +ÂÂÂÂÂÂÂ led->mc_cdev.available_colors =
>>> +ÂÂÂÂÂÂÂÂÂÂÂ pdata->led_config[chan].available_colors;
>>> +ÂÂÂÂÂÂÂ memcpy(led->channel_color,
>>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ pdata->led_config[chan].channel_color,
>>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ sizeof(led->channel_color));
>>> +ÂÂÂÂÂÂÂ memcpy(led->grouped_channels,
>>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ pdata->led_config[chan].grouped_channels,
>>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ sizeof(led->grouped_channels));
>>> +ÂÂÂ } else {
>>> +
>>> +ÂÂÂÂÂÂÂ led->cdev.default_trigger =
>>> +ÂÂÂÂÂÂÂÂÂÂÂ pdata->led_config[chan].default_trigger;
>>> +ÂÂÂÂÂÂÂ led->cdev.brightness_set_blocking = lp55xx_set_brightness;
>>> +ÂÂÂ }ÂÂÂ led->cdev.groups = lp55xx_led_groups;
>>> +
>>> ÂÂÂÂÂ led->led_current = pdata->led_config[chan].led_current;
>>> ÂÂÂÂÂ led->max_current = pdata->led_config[chan].max_current;
>>> ÂÂÂÂÂ led->chan_nr = pdata->led_config[chan].chan_nr;
>>> -ÂÂÂ led->cdev.default_trigger =
>>> pdata->led_config[chan].default_trigger;
>>> Â ÂÂÂÂÂ if (led->chan_nr >= max_channel) {
>>> ÂÂÂÂÂÂÂÂÂ dev_err(dev, "Use channel numbers between 0 and %d\n",
>>> @@ -170,18 +233,11 @@ static int lp55xx_init_led(struct lp55xx_led *led,
>>> ÂÂÂÂÂÂÂÂÂ return -EINVAL;
>>> ÂÂÂÂÂ }
>>> Â -ÂÂÂ led->cdev.brightness_set_blocking = lp55xx_set_brightness;
>>> -ÂÂÂ led->cdev.groups = lp55xx_led_groups;
>>> +ÂÂÂ if (pdata->led_config[chan].num_colors > 1)
>>> +ÂÂÂÂÂÂÂ ret = led_classdev_multicolor_register(dev, &led->mc_cdev);
>>> +ÂÂÂ else
>>> +ÂÂÂÂÂÂÂ ret = led_classdev_register(dev, &led->cdev);
>>> Â -ÂÂÂ if (pdata->led_config[chan].name) {
>>> -ÂÂÂÂÂÂÂ led->cdev.name = pdata->led_config[chan].name;
>>> -ÂÂÂ } else {
>>> -ÂÂÂÂÂÂÂ snprintf(name, sizeof(name), "%s:channel%d",
>>> -ÂÂÂÂÂÂÂÂÂÂÂ pdata->label ? : chip->cl->name, chan);
>>> -ÂÂÂÂÂÂÂ led->cdev.name = name;
>>> -ÂÂÂ }
>>> -
>>> -ÂÂÂ ret = led_classdev_register(dev, &led->cdev);
>>> ÂÂÂÂÂ if (ret) {
>>> ÂÂÂÂÂÂÂÂÂ dev_err(dev, "led register err: %d\n", ret);
>>> ÂÂÂÂÂÂÂÂÂ return ret;
>>> @@ -466,7 +522,6 @@ int lp55xx_register_leds(struct lp55xx_led *led,
>>> struct lp55xx_chip *chip)
>>> ÂÂÂÂÂÂÂÂÂ dev_err(&chip->cl->dev, "empty brightness configuration\n");
>>> ÂÂÂÂÂÂÂÂÂ return -EINVAL;
>>> ÂÂÂÂÂ }
>>> -
>>> ÂÂÂÂÂ for (i = 0; i < num_channels; i++) {
>>> Â ÂÂÂÂÂÂÂÂÂ /* do not initialize channels that are not connected */
>>> @@ -538,6 +593,76 @@ void lp55xx_unregister_sysfs(struct lp55xx_chip
>>> *chip)
>>> Â }
>>> Â EXPORT_SYMBOL_GPL(lp55xx_unregister_sysfs);
>>> Â +static int lp5xx_parse_common_child(struct device_node *np,
>>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct lp55xx_led_config *cfg,
>>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ int chan_num, bool is_multicolor)
>>> +{
>>> +ÂÂÂ u32 led_number;
>>> +ÂÂÂ int ret;
>>> +
>>> +ÂÂÂ of_property_read_string(np, "chan-name",
>>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ &cfg[chan_num].name);
>>> +ÂÂÂ of_property_read_u8(np, "led-cur",
>>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ &cfg[chan_num].led_current);
>>> +ÂÂÂ of_property_read_u8(np, "max-cur",
>>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ &cfg[chan_num].max_current);
>>> +
>>> +ÂÂÂ ret = of_property_read_u32(np, "reg", &led_number);
>>> +ÂÂÂ if (ret)
>>> +ÂÂÂÂÂÂÂ return ret;
>>> +
>>> +ÂÂÂ if (led_number < 0 || led_number > 6)
>>> +ÂÂÂÂÂÂÂ return -EINVAL;
>>> +
>>> +ÂÂÂ if (is_multicolor)
>>> +ÂÂÂÂÂÂÂ cfg[chan_num].grouped_channels[cfg[chan_num].num_colors]
>> Please pass the index for grouped channels as an argument to this
>> function. Referring here directly to a temporary state of num_colors
>> that is incremented in the loop from which this function is called
>> is ugly IMO.
>
> Ack
>
>
>>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ = led_number;
>>> +ÂÂÂ else
>>> +ÂÂÂÂÂÂÂ cfg[chan_num].chan_nr = led_number;
>>> +
>>> +ÂÂÂ return 0;
>>> +}
>>> +
>>> +static int lp5xx_parse_channel_child(struct device_node *np,
>>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct lp55xx_led_config *cfg,
>>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ int child_number)
>>> +{
>>> +ÂÂÂ struct device_node *child;
>>> +ÂÂÂ int channel_color;
>>> +ÂÂÂ u32 color_id;
>>> +ÂÂÂ int ret;
>>> +
>>> +ÂÂÂ cfg[child_number].default_trigger =
>>> +ÂÂÂÂÂÂÂÂÂÂÂ of_get_property(np, "linux,default-trigger", NULL);
>>> +
>>> +ÂÂÂ ret = of_property_read_u32(np, "color", &channel_color);
>>> +ÂÂÂ if (ret)
>>> +ÂÂÂÂÂÂÂ channel_color = ret;
>>> +
>>> +
>>> +ÂÂÂ if (channel_color == LED_COLOR_ID_MULTI) {
>>> +ÂÂÂÂÂÂÂ for_each_child_of_node(np, child) {
>>> +ÂÂÂÂÂÂÂÂÂÂÂ ret = lp5xx_parse_common_child(child, cfg,
>>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ child_number, true);
>>> +ÂÂÂÂÂÂÂÂÂÂÂ if (ret)
>>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ return ret;
>>> +ÂÂÂÂÂÂÂÂÂÂÂ ret = of_property_read_u32(child, "color", &color_id);
>>> +ÂÂÂÂÂÂÂÂÂÂÂ if (ret)
>>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ return ret;
>>> +
>>> +ÂÂÂÂÂÂÂÂÂÂÂ
>>> cfg[child_number].channel_color[cfg[child_number].num_colors] =
>>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ color_id;
>>> +ÂÂÂÂÂÂÂÂÂÂÂ set_bit(color_id, &cfg[child_number].available_colors);
>>> +
>>> +ÂÂÂÂÂÂÂÂÂÂÂ cfg[child_number].num_colors++;
>>> +ÂÂÂÂÂÂÂ }
>>> +ÂÂÂ } else {
>>> +ÂÂÂÂÂÂÂ return lp5xx_parse_common_child(np, cfg, child_number, false);
>>> +ÂÂÂ }
>>> +
>>> +ÂÂÂ return 0;
>>> +}
>>> +
>>> Â struct lp55xx_platform_data *lp55xx_of_populate_pdata(struct device
>>> *dev,
>>> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct device_node *np)
>>> Â {
>>> @@ -546,6 +671,7 @@ struct lp55xx_platform_data
>>> *lp55xx_of_populate_pdata(struct device *dev,
>>> ÂÂÂÂÂ struct lp55xx_led_config *cfg;
>>> ÂÂÂÂÂ int num_channels;
>>> ÂÂÂÂÂ int i = 0;
>>> +ÂÂÂ int ret;
>>> Â ÂÂÂÂÂ pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
>>> ÂÂÂÂÂ if (!pdata)
>>> @@ -565,14 +691,9 @@ struct lp55xx_platform_data
>>> *lp55xx_of_populate_pdata(struct device *dev,
>>> ÂÂÂÂÂ pdata->num_channels = num_channels;
>>> Â ÂÂÂÂÂ for_each_child_of_node(np, child) {
>>> -ÂÂÂÂÂÂÂ cfg[i].chan_nr = i;
>>> -
>>> -ÂÂÂÂÂÂÂ of_property_read_string(child, "chan-name", &cfg[i].name);
>>> -ÂÂÂÂÂÂÂ of_property_read_u8(child, "led-cur", &cfg[i].led_current);
>>> -ÂÂÂÂÂÂÂ of_property_read_u8(child, "max-cur", &cfg[i].max_current);
>>> -ÂÂÂÂÂÂÂ cfg[i].default_trigger =
>>> -ÂÂÂÂÂÂÂÂÂÂÂ of_get_property(child, "linux,default-trigger", NULL);
>>> -
>>> +ÂÂÂÂÂÂÂ ret = lp5xx_parse_channel_child(child, cfg, i);
>>> +ÂÂÂÂÂÂÂ if (ret)
>>> +ÂÂÂÂÂÂÂÂÂÂÂ return ERR_PTR(-EINVAL);
>>> ÂÂÂÂÂÂÂÂÂ i++;
>>> ÂÂÂÂÂ }
>>> Â diff --git a/drivers/leds/leds-lp55xx-common.h
>>> b/drivers/leds/leds-lp55xx-common.h
>>> index 783ed5103ce5..5ea2a292a97e 100644
>>> --- a/drivers/leds/leds-lp55xx-common.h
>>> +++ b/drivers/leds/leds-lp55xx-common.h
>>> @@ -12,6 +12,10 @@
>>> Â #ifndef _LEDS_LP55XX_COMMON_H
>>> Â #define _LEDS_LP55XX_COMMON_H
>>> Â +#include <linux/led-class-multicolor.h>
>>> +
>>> +#define LP55XX_MAX_GROUPED_CHANÂÂÂ 4
>>> +
>>> Â enum lp55xx_engine_index {
>>> ÂÂÂÂÂ LP55XX_ENGINE_INVALID,
>>> ÂÂÂÂÂ LP55XX_ENGINE_1,
>>> @@ -109,6 +113,9 @@ struct lp55xx_device_config {
>>> ÂÂÂÂÂ /* access brightness register */
>>> ÂÂÂÂÂ int (*brightness_fn)(struct lp55xx_led *led);
>>> Â +ÂÂÂ /* access specific brightness register */
>>> +ÂÂÂ int (*color_intensity_fn)(struct lp55xx_led *led, int chan_num,
>>> int brightness);
>>> +
>>> ÂÂÂÂÂ /* current setting function */
>>> ÂÂÂÂÂ void (*set_led_current) (struct lp55xx_led *led, u8 led_current);
>>> Â @@ -159,6 +166,7 @@ struct lp55xx_chip {
>>> ÂÂ * struct lp55xx_led
>>> ÂÂ * @chan_nrÂÂÂÂÂÂÂÂ : Channel number
>>> ÂÂ * @cdevÂÂÂÂÂÂÂÂÂÂÂ : LED class device
>>> + * @mc_cdevÂÂÂÂÂÂÂ : Multi color class device
>>> ÂÂ * @led_currentÂÂÂÂ : Current setting at each led channel
>>> ÂÂ * @max_currentÂÂÂÂ : Maximun current at each led channel
>>> ÂÂ * @brightnessÂÂÂÂÂ : Brightness value
>> Documentation for channel_color and grouped_channels is missing.
>>
> ACK
>
>
>>> @@ -167,9 +175,12 @@ struct lp55xx_chip {
>>> Â struct lp55xx_led {
>>> ÂÂÂÂÂ int chan_nr;
>>> ÂÂÂÂÂ struct led_classdev cdev;
>>> +ÂÂÂ struct led_classdev_mc mc_cdev;
>>> ÂÂÂÂÂ u8 led_current;
>>> ÂÂÂÂÂ u8 max_current;
>>> ÂÂÂÂÂ u8 brightness;
>>> +ÂÂÂ int channel_color[LP55XX_MAX_GROUPED_CHAN];
>>> +ÂÂÂ int grouped_channels[LP55XX_MAX_GROUPED_CHAN];
>> I propose to create structure:
>>
>> struct lp55xx_mc_cluster {
>> ÂÂÂÂint channel_color;
>> ÂÂÂÂint channel_id;
>> };
>>
>> and instead of the above two arrays create one
>>
>> struct lp55xx_mc_cluster mc_cluster[LP55XX_MAX_GROUPED_CHAN];
>
> Maybe we can extend the mc_color_converion struct to add output_num.
>
> Now I did try to do this but the design of the code made it a bit
> wonky.Â I will look at it again.
>
> If the output_num information is contain in a single struct as opposed
> to having each driver create their own struct.
>
> struct led_mc_color_conversion {
> ÂÂÂ int color_id;
> ÂÂÂ int brightness;
>
> ÂÂÂ int output_num;
>
> };
>
> struct led_mc_color_conversion mc_cluster[LP55XX_MAX_GROUPED_CHAN];
>
> If other drivers do not need that information then they do not need to
> populate it

Maybe, feel free to give it a try.

ÂÂÂÂÂ struct lp55xx_chip *chip;
>>> Â };
>>> Â diff --git a/include/linux/platform_data/leds-lp55xx.h
>>> b/include/linux/platform_data/leds-lp55xx.h
>>> index 96a787100fda..0ac29f537ab6 100644
>>> --- a/include/linux/platform_data/leds-lp55xx.h
>>> +++ b/include/linux/platform_data/leds-lp55xx.h
>>> @@ -12,6 +12,8 @@
>>> Â #ifndef _LEDS_LP55XX_H
>>> Â #define _LEDS_LP55XX_H
>>> Â +#include <linux/led-class-multicolor.h>
>>> +
>>> Â /* Clock configuration */
>>> Â #define LP55XX_CLOCK_AUTOÂÂÂ 0
>>> Â #define LP55XX_CLOCK_INTÂÂÂ 1
>>> @@ -23,6 +25,10 @@ struct lp55xx_led_config {
>>> ÂÂÂÂÂ u8 chan_nr;
>>> ÂÂÂÂÂ u8 led_current; /* mA x10, 0 if led is not connected */
>>> ÂÂÂÂÂ u8 max_current;
>>> +ÂÂÂ int num_colors;
>>> +ÂÂÂ unsigned long available_colors;
>>> +ÂÂÂ u32 channel_color[LED_COLOR_ID_MAX];
>> channel_color array is redundant if you have available_colors flags.
>
> I will look this again.
>
>
>>> +ÂÂÂ int grouped_channels[LED_COLOR_ID_MAX];
>>> Â };
>>> Â Â struct lp55xx_predef_pattern {
>>>
>

--
Best regards,
Jacek Anaszewski

Next message: Thomas Hellstrom: "Re: [PATCH] x86/cpu/vmware: use the full form of inl in VMWARE_PORT"
Previous message: Linus Torvalds: "Re: WARNING in filldir64"
In reply to: Dan Murphy: "Re: [PATCH v10 12/16] leds: lp55xx: Add multicolor framework support to lp55xx"
Next in thread: Dan Murphy: "[PATCH v10 01/16] dt: bindings: Add multicolor class dt bindings documention"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]