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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Jacek,

Thanks for you feedback.
I am going to use multicolor framework as Dan mentioned, and fix the
issues you pointed out.

Regards,

Nicolas

Le mar. 18 févr. 2020 à 22:13, Jacek Anaszewski
<jacek.anaszewski@xxxxxxxxx> a écrit :
>
> Hi Nicolas,
>
> On 2/18/20 10:37 AM, Nicolas Belin wrote:
> > Initilial commit in order to support the apa102c RGB leds.
> >
> > Signed-off-by: Nicolas Belin <nbelin@xxxxxxxxxxxx>
> > ---
> >  drivers/leds/Kconfig        |  11 ++
> >  drivers/leds/Makefile       |   1 +
> >  drivers/leds/leds-apa102c.c | 268 ++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 280 insertions(+)
> >  create mode 100644 drivers/leds/leds-apa102c.c
> >
> > diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> > index d82f1dea3711..4fafeaaf6ee8 100644
> > --- a/drivers/leds/Kconfig
> > +++ b/drivers/leds/Kconfig
> > @@ -69,6 +69,17 @@ 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 LEDS_CLASS
> > +     depends on SPI
> > +     help
> > +       This option enables support for the Shiji Lighthing APA102C RGB full color
> > +       LEDs.
> > +
> > +       To compile this driver as a module, choose M here: the module
> > +       will be called leds-apa102c.
> > +
> >  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 d7e1107753fb..ab17f90347cb 100644
> > --- a/drivers/leds/Makefile
> > +++ b/drivers/leds/Makefile
> > @@ -9,6 +9,7 @@ obj-$(CONFIG_LEDS_TRIGGERS)           += led-triggers.o
> >  # LED Platform Drivers
> >  obj-$(CONFIG_LEDS_88PM860X)          += leds-88pm860x.o
> >  obj-$(CONFIG_LEDS_AAT1290)           += leds-aat1290.o
> > +obj-$(CONFIG_LEDS_APA102C)           += leds-apa102c.o
> >  obj-$(CONFIG_LEDS_APU)                       += leds-apu.o
> >  obj-$(CONFIG_LEDS_AS3645A)           += leds-as3645a.o
> >  obj-$(CONFIG_LEDS_AN30259A)          += leds-an30259a.o
> > diff --git a/drivers/leds/leds-apa102c.c b/drivers/leds/leds-apa102c.c
> > new file mode 100644
> > index 000000000000..e7abe3f5b7c2
> > --- /dev/null
> > +++ b/drivers/leds/leds-apa102c.c
> > @@ -0,0 +1,268 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +/*
> > + * Copyright (C) 2020 BayLibre, SAS
> > + * Author: Nicolas Belin <nbelin@xxxxxxxxxxxx>
> > + */
>
> Please use "//" comment style for all the above lines.
>
> > +
> > +#include <linux/leds.h>
> > +#include <linux/module.h>
> > +#include <linux/of_device.h>
> > +#include <linux/spi/spi.h>
> > +#include <uapi/linux/uleds.h>
> > +
> > +/*
> > + *  APA102C SPI protocol description:
> > + *  +------+----------------------------------------+------+
> > + *  |START |               DATA FIELD:              | END  |
> > + *  |FRAME |               N LED FRAMES             |FRAME |
> > + *  +------+------+------+------+------+-----+------+------+
> > + *  | 0*32 | LED1 | LED2 | LED3 | LED4 | --- | LEDN | 1*32 |
> > + *  +------+------+------+------+------+-----+------+------+
> > + *
> > + *  +-----------------------------------+
> > + *  |START FRAME 32bits                 |
> > + *  +--------+--------+--------+--------+
> > + *  |00000000|00000000|00000000|00000000|
> > + *  +--------+--------+--------+--------+
> > + *
> > + *  +------------------------------------+
> > + *  |LED  FRAME 32bits                   |
> > + *  +---+-----+--------+--------+--------+
> > + *  |111|LUMA |  BLUE  | GREEN  |  RED   |
> > + *  +---+-----+--------+--------+--------+
> > + *  |3b |5bits| 8bits  | 8bits  | 8bits  |
> > + *  +---+-----+--------+--------+--------+
> > + *  |MSB   LSB|MSB  LSB|MSB  LSB|MSB  LSB|
> > + *  +---+-----+--------+--------+--------+
> > + *
> > + *  +-----------------------------------+
> > + *  |END FRAME 32bits                   |
> > + *  +--------+--------+--------+--------+
> > + *  |11111111|11111111|11111111|11111111|
> > + *  +--------+--------+--------+--------+
> > + */
> > +
> > +/* apa102c default settings */
> > +#define CR_MAX_BRIGHTNESS    GENMASK(7, 0)
> > +#define LM_MAX_BRIGHTNESS    GENMASK(4, 0)
> > +#define CH_NUM                       4
> > +#define START_BYTE           0
> > +#define END_BYTE             GENMASK(7, 0)
> > +#define LED_FRAME_HEADER     GENMASK(7, 5)
> > +
> > +enum led_channels {
> > +     RED,
> > +     GREEN,
> > +     BLUE,
> > +     LUMA,
> > +};
> > +
> > +struct apa102c_led {
> > +     char                    name[LED_MAX_NAME_SIZE];
> > +     struct apa102c          *priv;
> > +     struct led_classdev     ldev;
> > +     u8                      brightness;
>
> Please drop this one, struct led_classdev already holds brightness
> value.
>
> > +};
> > +
> > +struct apa102c {
> > +     size_t                  led_count;
> > +     struct device           *dev;
> > +     struct mutex            lock;
> > +     struct spi_device       *spi;
> > +     u8                      *buf;
> > +     struct apa102c_led      leds[];
> > +};
> > +
> > +static int apa102c_sync(struct apa102c *priv)
> > +{
> > +     int     ret;
> > +     size_t  i;
> > +     size_t  bytes = 0;
> > +
> > +     for (i = 0; i < 4; i++)
> > +             priv->buf[bytes++] = START_BYTE;
> > +
> > +     for (i = 0; i < priv->led_count; i++) {
> > +             priv->buf[bytes++] = LED_FRAME_HEADER |
> > +                                  priv->leds[i * CH_NUM + LUMA].brightness;
> > +             priv->buf[bytes++] = priv->leds[i * CH_NUM + BLUE].brightness;
> > +             priv->buf[bytes++] = priv->leds[i * CH_NUM + GREEN].brightness;
> > +             priv->buf[bytes++] = priv->leds[i * CH_NUM + RED].brightness;
>
> This is odd. You create separate LED class device for each color anyway,
> so this seems pointless. We have pending LED multi color framework patch
> set, as Dan mentioned, so you could try to use it. If you want to have
> the patch set accepted quicker then just set brightness for one LED at
> a time. You will be able to add LED multicolor class support later when
> it will be ready.
>
> > +     }
> > +
> > +     for (i = 0; i < 4; i++)
> > +             priv->buf[bytes++] = END_BYTE;
> > +
> > +     ret = spi_write(priv->spi, priv->buf, bytes);
> > +
> > +     return ret;
> > +}
> > +
> > +static int apa102c_set_sync(struct led_classdev *ldev,
> > +                        enum led_brightness brightness)
> > +{
> > +     int                     ret;
> > +     struct apa102c_led      *led = container_of(ldev,
> > +                                                 struct apa102c_led,
> > +                                                 ldev);
> > +
> > +     dev_dbg(led->priv->dev, "Set brightness of %s to %d\n",
> > +             led->name, brightness);
> > +
> > +     mutex_lock(&led->priv->lock);
> > +     led->brightness = (u8)brightness;
> > +     ret = apa102c_sync(led->priv);
> > +     mutex_unlock(&led->priv->lock);
> > +
> > +     return ret;
> > +}
> > +
> > +static int apa102c_probe_dt(struct apa102c *priv)
> > +{
> > +     u32                     i = 0;
> > +     int                     j = 0;
> > +     struct apa102c_led      *led;
> > +     struct fwnode_handle    *child;
> > +     struct device_node      *np;
> > +     int                     ret;
> > +     int                     use_index;
> > +     const char              *str;
> > +     static const char       * const rgb_name[] = {"red",
> > +                                                   "green",
> > +                                                   "blue",
> > +                                                   "luma"};
>
> We have LED_COLOR_ID* definitions in dt-bindings/leds/common.h
> for red, green and blue. And regarding "luma" - what is specificity
> of that one? If neither of existing definitions fits for it then
> you are welcome to submit a patch adding LED_COLOR_ID_LUMA.
>
> > +
> > +     device_for_each_child_node(priv->dev, child) {
> > +             np = to_of_node(child);
> > +
> > +             ret = fwnode_property_read_u32(child, "reg", &i);
> > +             if (ret)
> > +                     return ret;
> > +
> > +             if (i >= priv->led_count)
> > +                     return -EINVAL;
> > +
> > +             /* use the index to create the name if the label is not set */
> > +             use_index = fwnode_property_read_string(child, "label", &str);
> > +
> > +             /* for each physical LED, 4 LEDs are created representing
> > +              * the 4 components: red, green, blue and global luma.
> > +              */
> > +             for (j = 0; j < CH_NUM; j++) {
> > +                     led = &priv->leds[i * CH_NUM + j];
> > +
> > +                     if (use_index)
> > +                             snprintf(led->name, sizeof(led->name),
> > +                                      "apa102c:%s:%d", rgb_name[j], i);
> > +                     else
> > +                             snprintf(led->name, sizeof(led->name),
> > +                                      "apa102c:%s:%s", rgb_name[j], str);
>
> LED core already handles LED name composition. Please refer to existing
> LED class drivers that use devm_led_classdev_register_ext() API and use
> it in your driver.
>
> > +
> > +                     fwnode_property_read_string(child,
> > +                                                 "linux,default-trigger",
> > +                                                 &led->ldev.default_trigger);
> > +
> > +                     led->priv                        = priv;
> > +                     led->ldev.name                   = led->name;
> > +                     if (j == LUMA) {
> > +                             led->ldev.brightness     = led->brightness
>
> What do you want to achieve here?
>
> > +                                                      = LM_MAX_BRIGHTNESS;
> > +                             led->ldev.max_brightness = LM_MAX_BRIGHTNESS;
> > +                     } else {
> > +                             led->ldev.brightness     = led->brightness
> > +                                                      = 0;
> > +                             led->ldev.max_brightness = CR_MAX_BRIGHTNESS;
> > +                     }
> > +
> > +                     led->ldev.brightness_set_blocking = apa102c_set_sync;
> > +
> > +                     ret = devm_led_classdev_register(priv->dev, &led->ldev);
>
> As mentioned above - new *ext API will make your life easier.
>
> > +                     if (ret) {
> > +                             dev_err(priv->dev,
> > +                                     "failed to register LED %s, err %d",
> > +                                     led->name, ret);
> > +                             fwnode_handle_put(child);
> > +                             return ret;
> > +                     }
> > +
> > +                     led->ldev.dev->of_node = np;
> > +
> > +             }
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +static int apa102c_probe(struct spi_device *spi)
> > +{
> > +     struct apa102c  *priv;
> > +     size_t          led_count;
> > +     int             ret;
> > +
> > +     led_count = device_get_child_node_count(&spi->dev);
> > +     if (!led_count) {
> > +             dev_err(&spi->dev, "No LEDs defined in device tree!");
> > +             return -ENODEV;
> > +     }
> > +
> > +     priv = devm_kzalloc(&spi->dev,
> > +                         struct_size(priv, leds, led_count * CH_NUM),
> > +                         GFP_KERNEL);
> > +     if (!priv)
> > +             return -ENOMEM;
> > +
> > +     priv->buf = devm_kzalloc(&spi->dev, led_count * CH_NUM + 8, GFP_KERNEL);
> > +     if (!priv->buf)
> > +             return -ENOMEM;
> > +
> > +     mutex_init(&priv->lock);
> > +     priv->led_count = led_count;
> > +     priv->dev       = &spi->dev;
> > +     priv->spi       = spi;
> > +
> > +     ret = apa102c_probe_dt(priv);
> > +     if (ret)
> > +             return ret;
> > +
> > +     /* Set the LEDs with default values at start */
> > +     apa102c_sync(priv);
> > +     if (ret)
> > +             return ret;
> > +
> > +     spi_set_drvdata(spi, priv);
> > +
> > +     return 0;
> > +}
> > +
> > +static int apa102c_remove(struct spi_device *spi)
> > +{
> > +     struct apa102c *priv = spi_get_drvdata(spi);
> > +
> > +     mutex_destroy(&priv->lock);
> > +
> > +     return 0;
> > +}
> > +
> > +static const struct of_device_id apa102c_dt_ids[] = {
> > +     { .compatible = "shiji,apa102c", },
> > +     {},
> > +};
> > +
> > +MODULE_DEVICE_TABLE(of, apa102c_dt_ids);
> > +
> > +static struct spi_driver apa102c_driver = {
> > +     .probe          = apa102c_probe,
> > +     .remove         = apa102c_remove,
> > +     .driver = {
> > +             .name           = KBUILD_MODNAME,
> > +             .of_match_table = apa102c_dt_ids,
> > +     },
> > +};
> > +
> > +module_spi_driver(apa102c_driver);
> > +
> > +MODULE_AUTHOR("Nicolas Belin <nbelin@xxxxxxxxxxxx>");
> > +MODULE_DESCRIPTION("apa102c LED driver");
> > +MODULE_LICENSE("GPL v2");
> > +MODULE_ALIAS("spi:apa102c");
> >
>
> --
> Best regards,
> Jacek Anaszewski




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux