Hi Nicolas,
On 3/6/20 2:40 PM, Nicolas Belin wrote:
> Initilial commit in order to support the apa102c RGB leds. The
> RGB and global brightness management is done by creating 4 leds
> from the Led Framework per apa102c led.
>
> Signed-off-by: Nicolas Belin <nbelin@xxxxxxxxxxxx>
> ---
> drivers/leds/Kconfig | 11 ++
> drivers/leds/Makefile | 1 +
> drivers/leds/leds-apa102c.c | 306 ++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 318 insertions(+)
> create mode 100644 drivers/leds/leds-apa102c.c
>
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index d82f1dea3711..28fa6c4f65cc 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 SPI
> + depends on LEDS_CLASS
> + help
> + This option enables support for the APA102C RGB LEDs
> + from Shiji Lighting.
> +
> + 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..28dfe82900c5 100644
> --- a/drivers/leds/Makefile
> +++ b/drivers/leds/Makefile
> @@ -88,6 +88,7 @@ obj-$(CONFIG_LEDS_LM36274) += leds-lm36274.o
> obj-$(CONFIG_LEDS_TPS6105X) += leds-tps6105x.o
>
> # LED SPI Drivers
> +obj-$(CONFIG_LEDS_APA102C) += leds-apa102c.o
> obj-$(CONFIG_LEDS_CR0014114) += leds-cr0014114.o
> obj-$(CONFIG_LEDS_DAC124S085) += leds-dac124s085.o
> obj-$(CONFIG_LEDS_EL15203000) += leds-el15203000.o
> diff --git a/drivers/leds/leds-apa102c.c b/drivers/leds/leds-apa102c.c
> new file mode 100644
> index 000000000000..0043e7a6235b
> --- /dev/null
> +++ b/drivers/leds/leds-apa102c.c
> @@ -0,0 +1,306 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/leds.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/spi/spi.h>
> +#include <uapi/linux/uleds.h>
> +#include "leds.h"
> +
> +/*
> + * Copyright (C) 2020 BayLibre, SAS
> + * Author: Nicolas Belin <nbelin@xxxxxxxxxxxx>
> + */
> +
> +/*
> + * 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 GLOBAL_MAX_BRIGHTNESS GENMASK(4, 0)
> +#define RGB_MAX_BRIGHTNESS GENMASK(7, 0)
> +#define START_BYTE 0
> +#define END_BYTE GENMASK(7, 0)
> +#define LED_FRAME_HEADER GENMASK(7, 5)
> +
> +struct apa102c_led {
> + struct apa102c *priv;
> + char name[LED_MAX_NAME_SIZE];
This is not needed, LED core takes care of parsing LED name.
> + int color_id;
> + struct led_classdev cdev;
> +};
> +
> +struct apa102c_rgbled {
> + /* the 4 components of the apa102c rgb led:
> + * global brightness, blue, green and red in that order
> + */
> + struct apa102c_led component[4];
> +};
> +
> +struct apa102c {
> + size_t led_count;
> + struct device *dev;
> + struct mutex lock;
> + struct spi_device *spi;
> + u8 *spi_buf;
> + struct apa102c_rgbled rgb_leds[];
> +};
> +
> +static int apa102c_sync(struct apa102c *priv)
> +{
> + int ret;
> + size_t i;
> + struct apa102c_rgbled *leds = priv->rgb_leds;
> + u8 *buf = priv->spi_buf;
> +
> + /* creating the data that will be sent to all the leds at once */
> + for (i = 0; i < 4; i++)
> + *buf++ = START_BYTE;
> +
> + /* looping on each RGB led component, getting the global brightness
> + * then B, G and R values
> + */
> + for (i = 0; i < priv->led_count; i++) {
> + *buf++ = LED_FRAME_HEADER |
> + leds[i].component[0].cdev.brightness;
> + *buf++ = leds[i].component[1].cdev.brightness;
> + *buf++ = leds[i].component[2].cdev.brightness;
> + *buf++ = leds[i].component[3].cdev.brightness;
> + }
The problem is that in the monochrome LED approach (i.e. the only
available one) you have to alter the state of only one LED - that
on behalf of which the call is made.
And anyway, you're doing here much too much, taking into account
that this function is called from brightness_set op.
Isn't it possible to update only one LED ?
> +
> + for (i = 0; i < 4; i++)
> + *buf++ = END_BYTE;
> +
> + ret = spi_write(priv->spi, priv->spi_buf,
> + (priv->led_count + 2) * sizeof(u32));
> +
> + return ret;
> +}
> +
> +static int apa102c_brightness_set(struct led_classdev *cdev,
> + enum led_brightness brightness)
> +{
> + int ret;
> + struct apa102c_led *led = container_of(cdev,
> + struct apa102c_led,
> + cdev);
> +
> + mutex_lock(&led->priv->lock);
> + /* updating the brightness then synching all the leds */
> + cdev->brightness = brightness;
LED core handles that. Please drop this assignment.
> + ret = apa102c_sync(led->priv);
> + mutex_unlock(&led->priv->lock);
> +
> + return ret;
> +}
> +
> +static int apa102c_probe_dt(struct apa102c *priv)
> +{
> + int ret = 0;
> + u32 i;
> + struct apa102c_rgbled *rgb_led;
> + struct apa102c_led *led;
> + struct fwnode_handle *child;
> + const char *rgb_led_name;
> + const char *led_component_name;
> +
> + /* each node corresponds to an rgb led */
> + device_for_each_child_node(priv->dev, child) {
> +
> + ret = fwnode_property_read_u32(child, "reg", &i);
> + if (ret) {
> + dev_err(priv->dev, "coudld not read reg %d\n", ret);
> + goto child_out;
> + }
> +
> + if (i >= priv->led_count) {
> + ret = -EINVAL;
> + dev_err(priv->dev, "reg value too big\n");
> + goto child_out;
> + }
> +
> + rgb_led = &priv->rgb_leds[i];
> + /* checking if this led config is already used by checking if
> + * the priv member of the global_brightness led as already
> + * been set
> + */
> + if (rgb_led->component[0].priv) {
> + ret = -EINVAL;
> + dev_err(priv->dev, "using the same reg value twice\n");
> + goto child_out;
> + }
> +
> + ret = fwnode_property_read_string(child, "label",
> + &rgb_led_name);
> + if (ret) {
> + ret = -EINVAL;
> + dev_err(priv->dev, "missing rgb led name\n");
> + goto child_out;
> + }
> +
> + /* setting a color_id value for each of the 4 components of the
> + * apa102c RGB led. The first component is the global brightness
> + * of the led and thus has no color. The order of the colors
> + * after the global brightness is then blue, green and red
> + * in that order. It corresponds to the order in which the
> + * values are sent using spi
> + */
> + rgb_led->component[0].color_id = -1; //no color
> + rgb_led->component[1].color_id = LED_COLOR_ID_BLUE;
> + rgb_led->component[2].color_id = LED_COLOR_ID_GREEN;
> + rgb_led->component[3].color_id = LED_COLOR_ID_RED;
Each LED has to have separate DT node. We haven't had so far use case
with global brightness but I think it would be OK to add
LED_COLOR_ID_LUMA for that. But please hold on with making any changes
until Pavel will express his opinion on that.
> +
> + /* now looping on each component and registering a corresponding
> + * led
> + */
> + for (i = 0; i < 4; i++) {
> + led = &rgb_led->component[i];
> + if (led->color_id == -1) {
> + /* the global brightness has no defined name
> + * so setting it to "brightness". It also has
> + * a different MAX_BRIGHTNESS value.
> + * If a trigger is present, setting it on this
> + * component
> + */
> + led->cdev.max_brightness =
> + GLOBAL_MAX_BRIGHTNESS;
> + led_component_name = "brightness";
> + fwnode_property_read_string(child,
> + "linux,default-trigger",
> + &led->cdev.default_trigger);
> + } else {
> + /* using the color name defined by the framework
> + * for the B, G and R components
> + */
> + led->cdev.max_brightness = RGB_MAX_BRIGHTNESS;
> + led_component_name = led_colors[led->color_id];
> + }
> +
> + /* the rest is common to all the components */
> + led->priv = priv;
> + led->cdev.brightness_set_blocking =
> + apa102c_brightness_set;
> + /* creating our own led name to differentiate the 4
> + * components
> + */
> + snprintf(led->name, sizeof(led->name),
> + "%s_%s", rgb_led_name, led_component_name);
> + led->cdev.name = led->name;
> +
LED core can do the above for you, if only you provide it with LED
fwnode.
> + ret = devm_led_classdev_register_ext(priv->dev,
> + &led->cdev,
> + NULL);
There is no point in using *ext API and not passing led_init_data to it.
Please pass struct led_init_data in the third argument, with initialized
only its fwnode property.
> + if (ret) {
> + dev_err(priv->dev, "led register err: %d\n",
> + ret);
> + goto child_out;
> + }
> + }
> + }
> +
> +child_out:
> + return ret;
> +}
> +
> +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, rgb_leds, led_count),
> + GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + priv->spi_buf = devm_kzalloc(&spi->dev,
> + (led_count + 2) * sizeof(u32),
> + GFP_KERNEL);
> + if (!priv->spi_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
