| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 9 Mar 2020 10:59:50 +0100
From: Nicolas Belin <nbelin@...libre.com>
To: Jacek Anaszewski <jacek.anaszewski@...il.com>
Cc: linux-kernel@...r.kernel.org, linux-leds@...r.kernel.org,
Pavel Machek <pavel@....cz>, Dan Murphy <dmurphy@...com>,
devicetree@...r.kernel.org, baylibre-upstreaming@...ups.io
Subject: Re: [PATCH v3 3/3] drivers: leds: add support for apa102c leds
Hi Jacek,
Le ven. 6 mars 2020 à 21:20, Jacek Anaszewski
<jacek.anaszewski@...il.com> a écrit :
>
> 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 ?
No, it is not possible. When one LED is updated and as all the leds
are cascaded, the values for all the leds have to be sent using spi.
However, what I can do is not updating the full buffer and simply the
part that corresponds to the modified 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.
OK
>
> > + 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.
It would be nice to have LED_COLOR_ID_LUMA. However, I don't feel like
it is really a color. I'll wait for Pavel's opinion.
>
> > +
> > + /* 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.
I was doing that to have a led_name formatted as "label_color". I am
going to check for LED functions that could work for me and come back
to you if find other functions that are not present.
>
> > + 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
Thanks,
Regards,
Nicolas
Powered by blists - more mailing lists