2020-03-06 13:41:19

by Nicolas Belin

[permalink] [raw]
Subject: [PATCH v3 0/3] leds: add support for apa102c leds

This patch series adds the driver and its related documentation
for the APA102C RGB Leds.

Patch 1 adds the APA102C led manufacturer to the vendor-prefixes list.

Patch 2 Documents the APA102C led driver.

Patch 3 contains the actual driver code and modifications in the Kconfig
and the Makefile.

Updates since v1:
- Moved the apa102c line in the Makefile to the LED SPI Drivers part
- The driver is now based on the Multicolor Framework.

Updates since v2:
- The driver is now back to using the normal Led Framework.

Nicolas Belin (3):
dt-bindings: Document shiji vendor-prefix
dt-bindings: leds: Shiji Lighting APA102C LED
drivers: leds: add support for apa102c leds

.../devicetree/bindings/leds/leds-apa102c.yaml | 97 +++++++
.../devicetree/bindings/vendor-prefixes.yaml | 2 +
drivers/leds/Kconfig | 11 +
drivers/leds/Makefile | 1 +
drivers/leds/leds-apa102c.c | 306 +++++++++++++++++++++
5 files changed, 417 insertions(+)
create mode 100644 Documentation/devicetree/bindings/leds/leds-apa102c.yaml
create mode 100644 drivers/leds/leds-apa102c.c

--
2.7.4


2020-03-06 13:41:24

by Nicolas Belin

[permalink] [raw]
Subject: [PATCH v3 1/3] dt-bindings: Document shiji vendor-prefix

Shenzhen Shiji Lighting Co.,Ltd is a LED manufacturer.

Signed-off-by: Nicolas Belin <[email protected]>
---
Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++
1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml
index 9e67944bec9c..a463286c3960 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
+++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
@@ -863,6 +863,8 @@ patternProperties:
description: SGX Sensortech
"^sharp,.*":
description: Sharp Corporation
+ "^shiji,.*":
+ description: Shenzhen Shiji Lighting Co.,Ltd
"^shimafuji,.*":
description: Shimafuji Electric, Inc.
"^si-en,.*":
--
2.7.4

2020-03-06 13:41:39

by Nicolas Belin

[permalink] [raw]
Subject: [PATCH v3 2/3] dt-bindings: leds: Shiji Lighting APA102C LED

Document Shiji Lighting APA102C LED driver device tree bindings.

Signed-off-by: Nicolas Belin <[email protected]>
---
.../devicetree/bindings/leds/leds-apa102c.yaml | 97 ++++++++++++++++++++++
1 file changed, 97 insertions(+)
create mode 100644 Documentation/devicetree/bindings/leds/leds-apa102c.yaml

diff --git a/Documentation/devicetree/bindings/leds/leds-apa102c.yaml b/Documentation/devicetree/bindings/leds/leds-apa102c.yaml
new file mode 100644
index 000000000000..21457fc3762d
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/leds-apa102c.yaml
@@ -0,0 +1,97 @@
+# SPDX-License-Identifier: GPL-2.0
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/leds/leds-apa102c.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: LED driver for Shiji Lighting - APA102C
+
+maintainers:
+ - Nicolas Belin <[email protected]>
+
+description:
+ Each RGB LED is represented as a rgb-led sub-node of the leds-apa102c
+ device. Each LED is a three color rgb LED with an additional 32 levels
+ brightness adjustment. They can be cascaded so that multiple LEDs can be set
+ with a single command.
+
+properties:
+ compatible:
+ const: shiji,apa102c
+
+ reg:
+ maxItems: 1
+
+ spi-max-frequency:
+ maximum: 1000000
+
+ "#address-cells":
+ const: 1
+
+ "#size-cells":
+ const: 0
+
+patternProperties:
+ "^rgb-led@[0-9]+$":
+ type: object
+ description: |
+ Array of connected RGB LEDs.
+
+ properties:
+ reg:
+ description: |
+ This property corresponds to the led index. It has to be between 0
+ and the number of managed leds minus 1
+ maxItems: 1
+
+ label:
+ description: |
+ This property corresponds to the name of the RGB led.
+ maxItems: 1
+
+ linux,default-trigger: true
+
+ "#address-cells":
+ const: 1
+
+ "#size-cells":
+ const: 0
+
+ required:
+ - reg
+ - label
+ - '#address-cells'
+ - '#size-cells'
+
+required:
+ - compatible
+ - reg
+ - spi-max-frequency
+ - '#address-cells'
+ - '#size-cells'
+
+examples:
+ - |
+ spi {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ led-controller@0 {
+ compatible = "shiji,apa102c";
+ reg = <0>;
+ spi-max-frequency = <1000000>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+ rgb-led@0 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <0>;
+ label = "rgb_led1";
+ };
+ rgb-led@1 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <1>;
+ label = "rgb_led2";
+ };
+ };
+ };
--
2.7.4

2020-03-06 13:42:12

by Nicolas Belin

[permalink] [raw]
Subject: [PATCH v3 3/3] drivers: leds: add support for apa102c leds

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 <[email protected]>
---
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 <[email protected]>
+ */
+
+/*
+ * 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];
+ 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;
+ }
+
+ 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;
+ 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;
+
+ /* 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;
+
+ ret = devm_led_classdev_register_ext(priv->dev,
+ &led->cdev,
+ NULL);
+ 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 <[email protected]>");
+MODULE_DESCRIPTION("apa102c LED driver");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("spi:apa102c");
--
2.7.4

2020-03-06 20:20:32

by Jacek Anaszewski

[permalink] [raw]
Subject: Re: [PATCH v3 2/3] dt-bindings: leds: Shiji Lighting APA102C LED

Hi Nicolas,

On 3/6/20 2:40 PM, Nicolas Belin wrote:
> Document Shiji Lighting APA102C LED driver device tree bindings.
>
> Signed-off-by: Nicolas Belin <[email protected]>
> ---
> .../devicetree/bindings/leds/leds-apa102c.yaml | 97 ++++++++++++++++++++++
> 1 file changed, 97 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/leds/leds-apa102c.yaml
>
> diff --git a/Documentation/devicetree/bindings/leds/leds-apa102c.yaml b/Documentation/devicetree/bindings/leds/leds-apa102c.yaml
> new file mode 100644
> index 000000000000..21457fc3762d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/leds-apa102c.yaml
> @@ -0,0 +1,97 @@
> +# SPDX-License-Identifier: GPL-2.0
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/leds/leds-apa102c.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: LED driver for Shiji Lighting - APA102C
> +
> +maintainers:
> + - Nicolas Belin <[email protected]>
> +
> +description:
> + Each RGB LED is represented as a rgb-led sub-node of the leds-apa102c
> + device. Each LED is a three color rgb LED with an additional 32 levels
> + brightness adjustment. They can be cascaded so that multiple LEDs can be set
> + with a single command.
> +
> +properties:
> + compatible:
> + const: shiji,apa102c
> +
> + reg:
> + maxItems: 1
> +
> + spi-max-frequency:
> + maximum: 1000000
> +
> + "#address-cells":
> + const: 1
> +
> + "#size-cells":
> + const: 0
> +
> +patternProperties:
> + "^rgb-led@[0-9]+$":
> + type: object
> + description: |
> + Array of connected RGB LEDs.
> +
> + properties:
> + reg:
> + description: |
> + This property corresponds to the led index. It has to be between 0
> + and the number of managed leds minus 1
> + maxItems: 1
> +
> + label:

As mentioned before - don't use label and use function
and color properties instead.

> + description: |
> + This property corresponds to the name of the RGB led.
> + maxItems: 1
> +
> + linux,default-trigger: true
> +
> + "#address-cells":
> + const: 1
> +
> + "#size-cells":
> + const: 0
> +
> + required:
> + - reg
> + - label
> + - '#address-cells'
> + - '#size-cells'
> +
> +required:
> + - compatible
> + - reg
> + - spi-max-frequency
> + - '#address-cells'
> + - '#size-cells'
> +
> +examples:
> + - |
> + spi {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + led-controller@0 {
> + compatible = "shiji,apa102c";
> + reg = <0>;
> + spi-max-frequency = <1000000>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> + rgb-led@0 {

s/rgb-led/led/

> + #address-cells = <1>;
> + #size-cells = <0>;
> + reg = <0>;
> + label = "rgb_led1";

Instead of label please use function and color properties.

E.g.

color = LED_COLOR_ID_RED;
function = LED_FUNCTION_STATUS;

If the function of your interest is not available in
include/dt-bindings/leds/common.h then you can propose one.

Please refer to Documentation/devicetree/bindings/leds/common.txt.


> + };
> + rgb-led@1 {

s/rgb-led/led/

> + #address-cells = <1>;
> + #size-cells = <0>;
> + reg = <1>;
> + label = "rgb_led2";
> + };

You have to apply DT scheme from LED common bindings and have
separate child node for each color of RGB LED.

> + };
> + };
>

--
Best regards,
Jacek Anaszewski

2020-03-06 20:20:55

by Jacek Anaszewski

[permalink] [raw]
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 <[email protected]>
> ---
> 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 <[email protected]>
> + */
> +
> +/*
> + * 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 <[email protected]>");
> +MODULE_DESCRIPTION("apa102c LED driver");
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("spi:apa102c");
>

--
Best regards,
Jacek Anaszewski

2020-03-09 10:01:47

by Nicolas Belin

[permalink] [raw]
Subject: Re: [PATCH v3 3/3] drivers: leds: add support for apa102c leds

Hi Jacek,

Le ven. 6 mars 2020 à 21:20, Jacek Anaszewski
<[email protected]> 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 <[email protected]>
> > ---
> > 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 <[email protected]>
> > + */
> > +
> > +/*
> > + * 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 <[email protected]>");
> > +MODULE_DESCRIPTION("apa102c LED driver");
> > +MODULE_LICENSE("GPL v2");
> > +MODULE_ALIAS("spi:apa102c");
> >
>
> --
> Best regards,
> Jacek Anaszewski

Thanks,

Regards,

Nicolas

2020-03-09 21:06:39

by Rob Herring

[permalink] [raw]
Subject: Re: [PATCH v3 1/3] dt-bindings: Document shiji vendor-prefix

On Fri, 6 Mar 2020 14:40:08 +0100, Nicolas Belin wrote:
> Shenzhen Shiji Lighting Co.,Ltd is a LED manufacturer.
>
> Signed-off-by: Nicolas Belin <[email protected]>
> ---
> Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++
> 1 file changed, 2 insertions(+)
>

Please add Acked-by/Reviewed-by tags when posting new versions. However,
there's no need to repost patches *only* to add the tags. The upstream
maintainer will do that for acks received on the version they apply.

If a tag was not added on purpose, please state why and what changed.

2020-03-13 13:07:51

by Rob Herring

[permalink] [raw]
Subject: Re: [PATCH v3 2/3] dt-bindings: leds: Shiji Lighting APA102C LED

On Fri, Mar 06, 2020 at 02:40:09PM +0100, Nicolas Belin wrote:
> Document Shiji Lighting APA102C LED driver device tree bindings.
>
> Signed-off-by: Nicolas Belin <[email protected]>
> ---
> .../devicetree/bindings/leds/leds-apa102c.yaml | 97 ++++++++++++++++++++++
> 1 file changed, 97 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/leds/leds-apa102c.yaml
>
> diff --git a/Documentation/devicetree/bindings/leds/leds-apa102c.yaml b/Documentation/devicetree/bindings/leds/leds-apa102c.yaml
> new file mode 100644
> index 000000000000..21457fc3762d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/leds-apa102c.yaml
> @@ -0,0 +1,97 @@
> +# SPDX-License-Identifier: GPL-2.0

Dual license new bindings:

(GPL-2.0-only OR BSD-2-Clause)

> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/leds/leds-apa102c.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: LED driver for Shiji Lighting - APA102C
> +
> +maintainers:
> + - Nicolas Belin <[email protected]>
> +
> +description:
> + Each RGB LED is represented as a rgb-led sub-node of the leds-apa102c
> + device. Each LED is a three color rgb LED with an additional 32 levels
> + brightness adjustment. They can be cascaded so that multiple LEDs can be set
> + with a single command.
> +
> +properties:
> + compatible:
> + const: shiji,apa102c
> +
> + reg:
> + maxItems: 1
> +
> + spi-max-frequency:
> + maximum: 1000000
> +
> + "#address-cells":
> + const: 1
> +
> + "#size-cells":
> + const: 0
> +
> +patternProperties:
> + "^rgb-led@[0-9]+$":
> + type: object
> + description: |
> + Array of connected RGB LEDs.

This should reference leds/common.yaml:

allOf:
- $ref: common.yaml#

> +
> + properties:
> + reg:
> + description: |
> + This property corresponds to the led index. It has to be between 0
> + and the number of managed leds minus 1
> + maxItems: 1
> +
> + label:
> + description: |
> + This property corresponds to the name of the RGB led.
> + maxItems: 1
> +
> + linux,default-trigger: true
> +
> + "#address-cells":
> + const: 1
> +
> + "#size-cells":
> + const: 0
> +
> + required:
> + - reg
> + - label
> + - '#address-cells'
> + - '#size-cells'
> +
> +required:
> + - compatible
> + - reg
> + - spi-max-frequency
> + - '#address-cells'
> + - '#size-cells'

Add:

additionalProperties: false

> +
> +examples:
> + - |
> + spi {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + led-controller@0 {
> + compatible = "shiji,apa102c";
> + reg = <0>;
> + spi-max-frequency = <1000000>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> + rgb-led@0 {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + reg = <0>;
> + label = "rgb_led1";
> + };
> + rgb-led@1 {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + reg = <1>;
> + label = "rgb_led2";
> + };
> + };
> + };
> --
> 2.7.4
>

2020-03-24 09:43:32

by Pavel Machek

[permalink] [raw]
Subject: Re: [PATCH v3 3/3] drivers: leds: add support for apa102c leds

On Fri 2020-03-06 14:40:10, Nicolas Belin wrote:
> Initilial commit in order to support the apa102c RGB leds. The

Initial

> RGB and global brightness management is done by creating 4 leds
> from the Led Framework per apa102c led.
>

> +
> + /* 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

Please follow codingstyle on comment style and avoid // comments.

Best regards,
Pavel

--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


Attachments:
(No filename) (930.00 B)
signature.asc (188.00 B)
Digital signature
Download all attachments