Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <1582727592-4510-1-git-send-email-nbelin@baylibre.com>
 <1582727592-4510-4-git-send-email-nbelin@baylibre.com> <85f536f0-0078-08a5-5eeb-e401d1ed8782@gmail.com>
In-Reply-To: <85f536f0-0078-08a5-5eeb-e401d1ed8782@gmail.com>
From:   Nicolas Belin <nbelin@baylibre.com>
Date:   Tue, 3 Mar 2020 11:30:45 +0100
Message-ID: <CAJZgTGFMks7Fm+tWpYAAjxBLgD0nk9cfBiVZcAhojaXCvqfLPA@mail.gmail.com>
Subject: Re: [PATCH RFC v2 3/3] drivers: leds: add support for apa102c leds
To:     Jacek Anaszewski <jacek.anaszewski@gmail.com>
Cc:     linux-kernel@vger.kernel.org, linux-leds@vger.kernel.org,
        Pavel Machek <pavel@ucw.cz>, Dan Murphy <dmurphy@ti.com>,
        devicetree@vger.kernel.org, baylibre-upstreaming@groups.io
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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=C3=A9vr. 2020 =C3=A0 21:14, Jacek Anaszewski
<jacek.anaszewski@gmail.com> a =C3=A9crit :
>
> 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@baylibre.com>
> > Signed-off-by: Nicolas Belin <nbelin@baylibre.com>
> > ---
> >  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 bo=
ards"
> >       depends on LEDS_CLASS
> > diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> > index b5305b7d43fb..8334cb6dc7e8 100644
> [...]
> > +
> > +             led->priv                       =3D priv;
> > +             led->ldev.max_brightness        =3D MAX_BRIGHTNESS;
> > +             fwnode_property_read_string(child, "linux,default-trigger=
",
> > +                                         &led->ldev.default_trigger);
> > +
> > +             init_data.fwnode =3D child;
> > +             init_data.devicename =3D APA_DEV_NAME;
> > +             init_data.default_label =3D ":";
>
> 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 =3D 0;
> > +             fwnode_for_each_child_node(child, grandchild) {
> > +                     ret =3D fwnode_property_read_u32(grandchild, "col=
or",
> > +                                                    &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 !=3D 3) {
> > +                     ret =3D -EINVAL;
> > +                     dev_err(priv->dev, "There should be 3 colors\n");
> > +                     goto child_out;
> > +             }
> > +
> > +             if (led->mc_cdev.available_colors !=3D IS_RGB) {
> > +                     ret =3D -EINVAL;
> > +                     dev_err(priv->dev, "The led is expected to be RGB=
\n");
> > +                     goto child_out;
> > +             }
> > +
> > +             led->mc_cdev.num_leds =3D num_colors;
> > +             led->mc_cdev.led_cdev =3D &led->ldev;
> > +             led->ldev.brightness_set_blocking =3D apa102c_brightness_=
set;
> > +             ret =3D devm_led_classdev_multicolor_register_ext(priv->d=
ev,
>
> --
> Best regards,
> Jacek Anaszewski


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