[PATCH 0/3] leds: add support for apa102c leds

[PATCH 0/3] leds: add support for apa102c leds

[Nicolas Belin]

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.

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

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

-- 
2.7.4

[PATCH 1/3] dt-bindings: Document shiji vendor-prefix

[Nicolas Belin]

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

Signed-off-by: Nicolas Belin <nbelin@baylibre.com>
---
 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

[PATCH 2/3] dt-bindings: leds: Shiji Lighting APA102C LED driver

[Nicolas Belin]

Document Shiji Lighting APA102C LED driver device tree bindings.

Signed-off-by: Nicolas Belin <nbelin@baylibre.com>
---
 .../devicetree/bindings/leds/leds-apa102c.yaml     | 91 ++++++++++++++++++++++
 1 file changed, 91 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..24bc2fc19fcb
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/leds-apa102c.yaml
@@ -0,0 +1,91 @@
+# 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 <nbelin@baylibre.com>
+
+description:
+  Each LED is represented as a sub-node of the leds-apa102c device.  Each LED
+  is a three color RGB LED with 32 levels brightness adjustment that 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
+
+required:
+  - compatible
+  - reg
+  - spi-max-frequency
+  - '#address-cells'
+  - '#size-cells'
+
+patternProperties:
+  "^led@[0-9]+$":
+    type: object
+    description: |
+      Properties for an array of connected 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 led. If not set,
+          the led index will be used to create the led name instead
+        maxItems: 1
+
+      linux,default-trigger: true
+
+    required:
+      - reg
+
+examples:
+  - |
+    spi {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        led-controller@0 {
+            compatible = "shiji,apa102c";
+            reg = <0>;
+            #address-cells = <1>;
+            #size-cells = <0>;
+            spi-max-frequency = <1000000>;
+            led@0 {
+                reg = <0>;
+                label = "led1";
+            };
+
+            led@1 {
+                reg = <1>;
+                label = "led2";
+            };
+
+            led@2 {
+                reg = <2>;
+                label = "led3";
+            };
+        };
+    };
-- 
2.7.4

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

[Nicolas Belin]

Initilial commit in order to support the apa102c RGB leds.

Signed-off-by: Nicolas Belin <nbelin@baylibre.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@baylibre.com>
+ */
+
+#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;
+};
+
+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;
+	}
+
+	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"};
+
+	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);
+
+			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
+							 = 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);
+			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@baylibre.com>");
+MODULE_DESCRIPTION("apa102c LED driver");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("spi:apa102c");
-- 
2.7.4

Re: [PATCH 1/3] dt-bindings: Document shiji vendor-prefix

[Dan Murphy]

Nicolas

On 2/18/20 3:37 AM, Nicolas Belin wrote:
> Shenzhen Shiji Lighting Co.,Ltd is a LED manufacturer.
>
> Signed-off-by: Nicolas Belin <nbelin@baylibre.com>
> ---

You need to add the devicetree mailing list and Rob Herring to this for 
their review of the dt bindings patches

Dan

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

[Dan Murphy]

Hellp

On 2/18/20 3:37 AM, Nicolas Belin wrote:
> 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.

Is this something that can benefit from the Multicolor framework patches?

https://lore.kernel.org/patchwork/project/lkml/list/?series=427513

Can you RFC the APA102C driver on top of the Multicolor FW to see how it 
blends?

Dan

Re: [PATCH 2/3] dt-bindings: leds: Shiji Lighting APA102C LED driver

[Jacek Anaszewski]

Hi Nicolas,

Thank you for the patch set.

On 2/18/20 10:37 AM, Nicolas Belin wrote:
> Document Shiji Lighting APA102C LED driver device tree bindings.
> 
> Signed-off-by: Nicolas Belin <nbelin@baylibre.com>
> ---
>  .../devicetree/bindings/leds/leds-apa102c.yaml     | 91 ++++++++++++++++++++++
>  1 file changed, 91 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..24bc2fc19fcb
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/leds-apa102c.yaml
> @@ -0,0 +1,91 @@
> +# 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 <nbelin@baylibre.com>
> +
> +description:
> +  Each LED is represented as a sub-node of the leds-apa102c device.  Each LED
> +  is a three color RGB LED with 32 levels brightness adjustment that 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
> +
> +required:
> +  - compatible
> +  - reg
> +  - spi-max-frequency
> +  - '#address-cells'
> +  - '#size-cells'
> +
> +patternProperties:
> +  "^led@[0-9]+$":
> +    type: object
> +    description: |
> +      Properties for an array of connected 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:

label property has been deprecated, please refer to common LED bindings
[0] and use function and color properties instead.

> +        description: |
> +          This property corresponds to the name of the led. If not set,
> +          the led index will be used to create the led name instead
> +        maxItems: 1
> +
> +      linux,default-trigger: true
> +
> +    required:
> +      - reg
> +
> +examples:
> +  - |
> +    spi {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        led-controller@0 {
> +            compatible = "shiji,apa102c";
> +            reg = <0>;
> +            #address-cells = <1>;
> +            #size-cells = <0>;
> +            spi-max-frequency = <1000000>;
> +            led@0 {
> +                reg = <0>;
> +                label = "led1";
> +            };
> +
> +            led@1 {
> +                reg = <1>;
> +                label = "led2";
> +            };
> +
> +            led@2 {
> +                reg = <2>;
> +                label = "led3";
> +            };
> +        };
> +    };
> 

[0] Documentation/devicetree/bindings/leds/common.txt

-- 
Best regards,
Jacek Anaszewski

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

-- 
Best regards,
Jacek Anaszewski

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

[Jacek Anaszewski]

Hi Dan,

On 2/18/20 1:43 PM, Dan Murphy wrote:
> Hellp
> 
> On 2/18/20 3:37 AM, Nicolas Belin wrote:
>> 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.
> 
> Is this something that can benefit from the Multicolor framework patches?
> 
> https://lore.kernel.org/patchwork/project/lkml/list/?series=427513
> 
> Can you RFC the APA102C driver on top of the Multicolor FW to see how it
> blends?

Just to let you know - I am currently playing with Samsung Galaxy S3,
which has mainline support, and also with a driver for its RGB LED:
leds-an30259a.c. I am adjusting that driver to LED multicolor class
and I will have certainly some more remarks (also to the variable naming
I proposed myself, which after few months feels awkward to me).

Generally speaking - we have to do everything to make the addition
of LED multicolor support to the driver way easier and more
straightforward since currently it is painful. I would go for
a separate compilation unit for multicolor specifc part of LED drivers.
But I will be able to tell more after few more days.

-- 
Best regards,
Jacek Anaszewski

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

[Nicolas Belin]

Hi Dan,

Le mar. 18 févr. 2020 à 13:47, Dan Murphy <dmurphy@ti.com> a écrit :
>
> Hellp
>
> On 2/18/20 3:37 AM, Nicolas Belin wrote:
> > 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.
>
> Is this something that can benefit from the Multicolor framework patches?
>
> https://lore.kernel.org/patchwork/project/lkml/list/?series=427513
>
> Can you RFC the APA102C driver on top of the Multicolor FW to see how it
> blends?

Sure, the Multicolor framework will probably improve my driver !
I'll send you a new version once I have tested it.

>
> Dan
>

Thanks,

Nicolas

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

[Nicolas Belin]

Hi Jacek,

Thanks for you feedback.
I am going to use multicolor framework as Dan mentioned, and fix the
issues you pointed out.

Regards,

Nicolas

Le mar. 18 févr. 2020 à 22:13, Jacek Anaszewski
<jacek.anaszewski@gmail.com> a écrit :
>
> 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@baylibre.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@baylibre.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@baylibre.com>");
> > +MODULE_DESCRIPTION("apa102c LED driver");
> > +MODULE_LICENSE("GPL v2");
> > +MODULE_ALIAS("spi:apa102c");
> >
>
> --
> Best regards,
> Jacek Anaszewski

Re: [PATCH 2/3] dt-bindings: leds: Shiji Lighting APA102C LED driver

[Geert Uytterhoeven]

Hi Nicolas,

CC devicetree, Lukas

On Tue, Feb 18, 2020 at 10:39 AM Nicolas Belin <nbelin@baylibre.com> wrote:
> Document Shiji Lighting APA102C LED driver device tree bindings.
>
> Signed-off-by: Nicolas Belin <nbelin@baylibre.com>
> ---
>  .../devicetree/bindings/leds/leds-apa102c.yaml     | 91 ++++++++++++++++++++++
>  1 file changed, 91 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..24bc2fc19fcb
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/leds-apa102c.yaml
> @@ -0,0 +1,91 @@
> +# 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 <nbelin@baylibre.com>
> +
> +description:
> +  Each LED is represented as a sub-node of the leds-apa102c device.  Each LED
> +  is a three color RGB LED with 32 levels brightness adjustment that 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
> +
> +required:
> +  - compatible
> +  - reg
> +  - spi-max-frequency
> +  - '#address-cells'
> +  - '#size-cells'
> +
> +patternProperties:
> +  "^led@[0-9]+$":
> +    type: object
> +    description: |
> +      Properties for an array of connected 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 led. If not set,
> +          the led index will be used to create the led name instead
> +        maxItems: 1
> +
> +      linux,default-trigger: true
> +
> +    required:
> +      - reg
> +
> +examples:
> +  - |
> +    spi {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        led-controller@0 {
> +            compatible = "shiji,apa102c";
> +            reg = <0>;
> +            #address-cells = <1>;
> +            #size-cells = <0>;
> +            spi-max-frequency = <1000000>;
> +            led@0 {
> +                reg = <0>;
> +                label = "led1";
> +            };
> +
> +            led@1 {
> +                reg = <1>;
> +                label = "led2";
> +            };
> +
> +            led@2 {
> +                reg = <2>;
> +                label = "led3";
> +            };
> +        };
> +    };

Perhaps this should use "#daisy-chained-devices" instead of listing all LEDs
explicitly?
Or would that cause problems w.r.t. LED labeling?

Documentation/devicetree/bindings/common-properties.txt

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 2/3] dt-bindings: leds: Shiji Lighting APA102C LED driver

[Jacek Anaszewski]

Hi Geert,

On 2/20/20 11:30 AM, Geert Uytterhoeven wrote:
> Hi Nicolas,
> 
> CC devicetree, Lukas
> 
> On Tue, Feb 18, 2020 at 10:39 AM Nicolas Belin <nbelin@baylibre.com> wrote:
>> Document Shiji Lighting APA102C LED driver device tree bindings.
>>
>> Signed-off-by: Nicolas Belin <nbelin@baylibre.com>
>> ---
>>  .../devicetree/bindings/leds/leds-apa102c.yaml     | 91 ++++++++++++++++++++++
>>  1 file changed, 91 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..24bc2fc19fcb
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/leds/leds-apa102c.yaml
>> @@ -0,0 +1,91 @@
>> +# 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 <nbelin@baylibre.com>
>> +
>> +description:
>> +  Each LED is represented as a sub-node of the leds-apa102c device.  Each LED
>> +  is a three color RGB LED with 32 levels brightness adjustment that 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
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - spi-max-frequency
>> +  - '#address-cells'
>> +  - '#size-cells'
>> +
>> +patternProperties:
>> +  "^led@[0-9]+$":
>> +    type: object
>> +    description: |
>> +      Properties for an array of connected 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 led. If not set,
>> +          the led index will be used to create the led name instead
>> +        maxItems: 1
>> +
>> +      linux,default-trigger: true
>> +
>> +    required:
>> +      - reg
>> +
>> +examples:
>> +  - |
>> +    spi {
>> +        #address-cells = <1>;
>> +        #size-cells = <0>;
>> +
>> +        led-controller@0 {
>> +            compatible = "shiji,apa102c";
>> +            reg = <0>;
>> +            #address-cells = <1>;
>> +            #size-cells = <0>;
>> +            spi-max-frequency = <1000000>;
>> +            led@0 {
>> +                reg = <0>;
>> +                label = "led1";
>> +            };
>> +
>> +            led@1 {
>> +                reg = <1>;
>> +                label = "led2";
>> +            };
>> +
>> +            led@2 {
>> +                reg = <2>;
>> +                label = "led3";
>> +            };
>> +        };
>> +    };
> 
> Perhaps this should use "#daisy-chained-devices" instead of listing all LEDs
> explicitly?
> Or would that cause problems w.r.t. LED labeling?

It would be against common LED bindings.

Besides, this daisy-chain-devices property is not well documented.
I.e. I don't see how it facilitates conveying information on output
ID (here reg is used for that), and as you already mentioned
- LED labeling.

Generally speaking, I don't think it is applicable to the LED subsystem.

> Documentation/devicetree/bindings/common-properties.txt
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 

-- 
Best regards,
Jacek Anaszewski

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

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 684 bytes --]

Hi!

> > Is this something that can benefit from the Multicolor framework patches?
> >
> > https://lore.kernel.org/patchwork/project/lkml/list/?series=427513
> >
> > Can you RFC the APA102C driver on top of the Multicolor FW to see how it
> > blends?
> 
> Sure, the Multicolor framework will probably improve my driver !
> I'll send you a new version once I have tested it.

If you want to submit basic version of your driver that does _not_
support RGB, that may help get the driver merged early.

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]