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

[Jacek Anaszewski]

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