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

[Nicolas Belin]

Hi Jacek,

That's a shame that it is not going to be upstreamed soon as it making
my led driver much nicer :)
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.

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



-- 
Nicolas Belin
Software design engineer
BayLibre
www.baylibre.com