| 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
| ||
|
Message-ID: <00d63872-0856-602a-e24b-4e27300d9254@gmail.com>
Date: Tue, 18 Feb 2020 22:13:07 +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
Subject: Re: [PATCH 3/3] drivers: leds: add support for apa102c leds
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@...libre.com>
> ---
> 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@...libre.com>
> + */
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@...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