Re: [PATCH 3/3] drivers: leds: add support for apa102c leds
From: Nicolas Belin
Date: Thu Feb 20 2020 - 05:25:51 EST
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