Re: [PATCH RFC v2 3/3] drivers: leds: add support for apa102c leds

[Jacek Anaszewski]

Hi Nicolas,

On 3/3/20 11:30 AM, Nicolas Belin wrote:
> Hi Jacek,
> 
> That's a shame that it is not going to be upstreamed soon as it making
> my led driver much nicer :)

Now when we clarified interface related issues I hope it will
be rather weeks than months when it is ready.

> So what's the plan with the multicolor framework?
> I am happy to send a new version to fix your remark and then adapt my
> driver to the future changes in the Framework.

Just rework the driver to create one LED class device per LED color.
After LED mc framework is upstream you will be free to add the
support for it to your driver.

Best regards,
Jacek Anaszewski

> Let me know what you think.
> 
> Thanks,
> 
> Regards,
> 
> Nicolas
> 
> Le mer. 26 fÃvr. 2020 Ã 21:14, Jacek Anaszewski
> <jacek.anaszewski@xxxxxxxxx> a Ãcrit :
>>
>> Hi Nicolas,
>>
>> Regardless of the fact that LED mc framework in current shape
>> will probably not materialize in mainline, I have single
>> remark regarding LED initialization. Please take a look below.
>>
>> On 2/26/20 3:33 PM, Nicolas Belin wrote:
>>> Initilial commit in order to support the apa102c RGB leds. This
>>> is based on the Multicolor Framework.
>>>
>>> Reviewed-by: Corentin Labbe <clabbe@xxxxxxxxxxxx>
>>> Signed-off-by: Nicolas Belin <nbelin@xxxxxxxxxxxx>
>>> ---
>>>  drivers/leds/Kconfig        |   7 ++
>>>  drivers/leds/Makefile       |   1 +
>>>  drivers/leds/leds-apa102c.c | 291 ++++++++++++++++++++++++++++++++++++++++++++
>>>  3 files changed, 299 insertions(+)
>>>  create mode 100644 drivers/leds/leds-apa102c.c
>>>
>>> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
>>> index 5dc6535a88ef..71e29727c6ec 100644
>>> --- a/drivers/leds/Kconfig
>>> +++ b/drivers/leds/Kconfig
>>> @@ -79,6 +79,13 @@ config LEDS_AN30259A
>>>         To compile this driver as a module, choose M here: the module
>>>         will be called leds-an30259a.
>>>
>>> +config LEDS_APA102C
>>> +     tristate "LED Support for Shiji APA102C"
>>> +     depends on SPI
>>> +     depends on LEDS_CLASS_MULTI_COLOR
>>> +     help
>>> +       This option enables support for APA102C LEDs.
>>> +
>>>  config LEDS_APU
>>>       tristate "Front panel LED support for PC Engines APU/APU2/APU3 boards"
>>>       depends on LEDS_CLASS
>>> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
>>> index b5305b7d43fb..8334cb6dc7e8 100644
>> [...]
>>> +
>>> +             led->priv                       = priv;
>>> +             led->ldev.max_brightness        = MAX_BRIGHTNESS;
>>> +             fwnode_property_read_string(child, "linux,default-trigger",
>>> +                                         &led->ldev.default_trigger);
>>> +
>>> +             init_data.fwnode = child;
>>> +             init_data.devicename = APA_DEV_NAME;
>>> +             init_data.default_label = ":";
>>
>> devicename property should be filled in new drivers only in case
>> devname_mandatory is set to true.
>> default_label property is for legacy drivers, for backward compatibility
>> with old LED naming convention.
>>
>> For more information please refer to:
>> - Documentation/leds/leds-class.rst, "LED Device Naming" section
>> - struct led_init_data documention in linux/leds.h
>>
>> In effect you need only fwnode here,
>>
>>> +
>>> +             num_colors = 0;
>>> +             fwnode_for_each_child_node(child, grandchild) {
>>> +                     ret = fwnode_property_read_u32(grandchild, "color",
>>> +                                                    &color_id);
>>> +                     if (ret) {
>>> +                             dev_err(priv->dev, "Cannot read color\n");
>>> +                             goto child_out;
>>> +                     }
>>> +
>>> +                     set_bit(color_id, &led->mc_cdev.available_colors);
>>> +                     num_colors++;
>>> +             }
>>> +
>>> +             if (num_colors != 3) {
>>> +                     ret = -EINVAL;
>>> +                     dev_err(priv->dev, "There should be 3 colors\n");
>>> +                     goto child_out;
>>> +             }
>>> +
>>> +             if (led->mc_cdev.available_colors != IS_RGB) {
>>> +                     ret = -EINVAL;
>>> +                     dev_err(priv->dev, "The led is expected to be RGB\n");
>>> +                     goto child_out;
>>> +             }
>>> +
>>> +             led->mc_cdev.num_leds = num_colors;
>>> +             led->mc_cdev.led_cdev = &led->ldev;
>>> +             led->ldev.brightness_set_blocking = apa102c_brightness_set;
>>> +             ret = devm_led_classdev_multicolor_register_ext(priv->dev,
>>
>> --
>> Best regards,
>> Jacek Anaszewski
> 
> 
>