Message-ID: <f599f5f7-4023-10c0-a582-4dce2a601b95@gmail.com>
Date:   Fri, 6 Mar 2020 21:20:02 +0100
From:   Jacek Anaszewski <jacek.anaszewski@...il.com>
To:     Nicolas Belin <nbelin@...libre.com>, linux-kernel@...r.kernel.org,
        linux-leds@...r.kernel.org, pavel@....cz, dmurphy@...com,
        devicetree@...r.kernel.org
Cc:     baylibre-upstreaming@...ups.io
Subject: Re: [PATCH v3 3/3] drivers: leds: add support for apa102c leds

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@...libre.com>
> ---
>  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@...libre.com>
> + */
> +
> +/*
> + *  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@...libre.com>");
> +MODULE_DESCRIPTION("apa102c LED driver");
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("spi:apa102c");
> 

-- 
Best regards,
Jacek Anaszewski

Powered by blists - more mailing lists