LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH v3 0/3] leds: add support for apa102c leds
@ 2020-03-06 13:40 Nicolas Belin
  2020-03-06 13:40 ` [PATCH v3 1/3] dt-bindings: Document shiji vendor-prefix Nicolas Belin
                   ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: Nicolas Belin @ 2020-03-06 13:40 UTC (permalink / raw)
  To: linux-kernel, linux-leds, jacek.anaszewski, pavel, dmurphy, devicetree
  Cc: baylibre-upstreaming, 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.

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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v3 1/3] dt-bindings: Document shiji vendor-prefix
  2020-03-06 13:40 [PATCH v3 0/3] leds: add support for apa102c leds Nicolas Belin
@ 2020-03-06 13:40 ` Nicolas Belin
  2020-03-09 21:05   ` Rob Herring
  2020-03-06 13:40 ` [PATCH v3 2/3] dt-bindings: leds: Shiji Lighting APA102C LED Nicolas Belin
  2020-03-06 13:40 ` [PATCH v3 3/3] drivers: leds: add support for apa102c leds Nicolas Belin
  2 siblings, 1 reply; 10+ messages in thread
From: Nicolas Belin @ 2020-03-06 13:40 UTC (permalink / raw)
  To: linux-kernel, linux-leds, jacek.anaszewski, pavel, dmurphy, devicetree
  Cc: baylibre-upstreaming, 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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v3 2/3] dt-bindings: leds: Shiji Lighting APA102C LED
  2020-03-06 13:40 [PATCH v3 0/3] leds: add support for apa102c leds Nicolas Belin
  2020-03-06 13:40 ` [PATCH v3 1/3] dt-bindings: Document shiji vendor-prefix Nicolas Belin
@ 2020-03-06 13:40 ` Nicolas Belin
  2020-03-06 20:19   ` Jacek Anaszewski
  2020-03-13 13:06   ` Rob Herring
  2020-03-06 13:40 ` [PATCH v3 3/3] drivers: leds: add support for apa102c leds Nicolas Belin
  2 siblings, 2 replies; 10+ messages in thread
From: Nicolas Belin @ 2020-03-06 13:40 UTC (permalink / raw)
  To: linux-kernel, linux-leds, jacek.anaszewski, pavel, dmurphy, devicetree
  Cc: baylibre-upstreaming, 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     | 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 <nbelin@baylibre.com>
+
+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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v3 3/3] drivers: leds: add support for apa102c leds
  2020-03-06 13:40 [PATCH v3 0/3] leds: add support for apa102c leds Nicolas Belin
  2020-03-06 13:40 ` [PATCH v3 1/3] dt-bindings: Document shiji vendor-prefix Nicolas Belin
  2020-03-06 13:40 ` [PATCH v3 2/3] dt-bindings: leds: Shiji Lighting APA102C LED Nicolas Belin
@ 2020-03-06 13:40 ` Nicolas Belin
  2020-03-06 20:20   ` Jacek Anaszewski
  2020-03-24  9:42   ` Pavel Machek
  2 siblings, 2 replies; 10+ messages in thread
From: Nicolas Belin @ 2020-03-06 13:40 UTC (permalink / raw)
  To: linux-kernel, linux-leds, jacek.anaszewski, pavel, dmurphy, devicetree
  Cc: baylibre-upstreaming, Nicolas Belin

Initilial commit in order to support the apa102c RGB leds. The
RGB and global brightness management is done by creating 4 leds
from the Led Framework per apa102c led.

Signed-off-by: Nicolas Belin <nbelin@baylibre.com>
---
 drivers/leds/Kconfig        |  11 ++
 drivers/leds/Makefile       |   1 +
 drivers/leds/leds-apa102c.c | 306 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 318 insertions(+)
 create mode 100644 drivers/leds/leds-apa102c.c

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index d82f1dea3711..28fa6c4f65cc 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -69,6 +69,17 @@ config LEDS_AN30259A
 	  To compile this driver as a module, choose M here: the module
 	  will be called leds-an30259a.
 
+config LEDS_APA102C
+	tristate "LED Support for Shiji APA102C"
+	depends on SPI
+	depends on LEDS_CLASS
+	help
+	  This option enables support for the APA102C RGB LEDs
+	  from Shiji Lighting.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called leds-apa102c.
+
 config LEDS_APU
 	tristate "Front panel LED support for PC Engines APU/APU2/APU3 boards"
 	depends on LEDS_CLASS
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index d7e1107753fb..28dfe82900c5 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -88,6 +88,7 @@ obj-$(CONFIG_LEDS_LM36274)		+= leds-lm36274.o
 obj-$(CONFIG_LEDS_TPS6105X)		+= leds-tps6105x.o
 
 # LED SPI Drivers
+obj-$(CONFIG_LEDS_APA102C)		+= leds-apa102c.o
 obj-$(CONFIG_LEDS_CR0014114)		+= leds-cr0014114.o
 obj-$(CONFIG_LEDS_DAC124S085)		+= leds-dac124s085.o
 obj-$(CONFIG_LEDS_EL15203000)		+= leds-el15203000.o
diff --git a/drivers/leds/leds-apa102c.c b/drivers/leds/leds-apa102c.c
new file mode 100644
index 000000000000..0043e7a6235b
--- /dev/null
+++ b/drivers/leds/leds-apa102c.c
@@ -0,0 +1,306 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/leds.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/spi/spi.h>
+#include <uapi/linux/uleds.h>
+#include "leds.h"
+
+/*
+ * Copyright (C) 2020 BayLibre, SAS
+ * Author: Nicolas Belin <nbelin@baylibre.com>
+ */
+
+/*
+ *  APA102C SPI protocol description:
+ *  +------+----------------------------------------+------+
+ *  |START |               DATA FIELD:              | END  |
+ *  |FRAME |               N LED FRAMES             |FRAME |
+ *  +------+------+------+------+------+-----+------+------+
+ *  | 0*32 | LED1 | LED2 | LED3 | LED4 | --- | LEDN | 1*32 |
+ *  +------+------+------+------+------+-----+------+------+
+ *
+ *  +-----------------------------------+
+ *  |START FRAME 32bits                 |
+ *  +--------+--------+--------+--------+
+ *  |00000000|00000000|00000000|00000000|
+ *  +--------+--------+--------+--------+
+ *
+ *  +------------------------------------+
+ *  |LED  FRAME 32bits                   |
+ *  +---+-----+--------+--------+--------+
+ *  |111|LUMA |  BLUE  | GREEN  |  RED   |
+ *  +---+-----+--------+--------+--------+
+ *  |3b |5bits| 8bits  | 8bits  | 8bits  |
+ *  +---+-----+--------+--------+--------+
+ *  |MSB   LSB|MSB  LSB|MSB  LSB|MSB  LSB|
+ *  +---+-----+--------+--------+--------+
+ *
+ *  +-----------------------------------+
+ *  |END FRAME 32bits                   |
+ *  +--------+--------+--------+--------+
+ *  |11111111|11111111|11111111|11111111|
+ *  +--------+--------+--------+--------+
+ */
+
+/* apa102c default settings */
+#define GLOBAL_MAX_BRIGHTNESS	GENMASK(4, 0)
+#define RGB_MAX_BRIGHTNESS	GENMASK(7, 0)
+#define START_BYTE		0
+#define END_BYTE		GENMASK(7, 0)
+#define LED_FRAME_HEADER	GENMASK(7, 5)
+
+struct apa102c_led {
+	struct apa102c		*priv;
+	char			name[LED_MAX_NAME_SIZE];
+	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 <nbelin@baylibre.com>");
+MODULE_DESCRIPTION("apa102c LED driver");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("spi:apa102c");
-- 
2.7.4


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v3 2/3] dt-bindings: leds: Shiji Lighting APA102C LED
  2020-03-06 13:40 ` [PATCH v3 2/3] dt-bindings: leds: Shiji Lighting APA102C LED Nicolas Belin
@ 2020-03-06 20:19   ` Jacek Anaszewski
  2020-03-13 13:06   ` Rob Herring
  1 sibling, 0 replies; 10+ messages in thread
From: Jacek Anaszewski @ 2020-03-06 20:19 UTC (permalink / raw)
  To: Nicolas Belin, linux-kernel, linux-leds, pavel, dmurphy, devicetree
  Cc: baylibre-upstreaming

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 <nbelin@baylibre.com>
> ---
>  .../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 <nbelin@baylibre.com>
> +
> +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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v3 3/3] drivers: leds: add support for apa102c leds
  2020-03-06 13:40 ` [PATCH v3 3/3] drivers: leds: add support for apa102c leds Nicolas Belin
@ 2020-03-06 20:20   ` Jacek Anaszewski
  2020-03-09  9:59     ` Nicolas Belin
  2020-03-24  9:42   ` Pavel Machek
  1 sibling, 1 reply; 10+ messages in thread
From: Jacek Anaszewski @ 2020-03-06 20:20 UTC (permalink / raw)
  To: Nicolas Belin, linux-kernel, linux-leds, pavel, dmurphy, devicetree
  Cc: baylibre-upstreaming

Hi Nicolas,

On 3/6/20 2:40 PM, Nicolas Belin wrote:
> Initilial commit in order to support the apa102c RGB leds. The
> RGB and global brightness management is done by creating 4 leds
> from the Led Framework per apa102c led.
> 
> Signed-off-by: Nicolas Belin <nbelin@baylibre.com>
> ---
>  drivers/leds/Kconfig        |  11 ++
>  drivers/leds/Makefile       |   1 +
>  drivers/leds/leds-apa102c.c | 306 ++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 318 insertions(+)
>  create mode 100644 drivers/leds/leds-apa102c.c
> 
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index d82f1dea3711..28fa6c4f65cc 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -69,6 +69,17 @@ config LEDS_AN30259A
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called leds-an30259a.
>  
> +config LEDS_APA102C
> +	tristate "LED Support for Shiji APA102C"
> +	depends on SPI
> +	depends on LEDS_CLASS
> +	help
> +	  This option enables support for the APA102C RGB LEDs
> +	  from Shiji Lighting.
> +
> +	  To compile this driver as a module, choose M here: the module
> +	  will be called leds-apa102c.
> +
>  config LEDS_APU
>  	tristate "Front panel LED support for PC Engines APU/APU2/APU3 boards"
>  	depends on LEDS_CLASS
> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> index d7e1107753fb..28dfe82900c5 100644
> --- a/drivers/leds/Makefile
> +++ b/drivers/leds/Makefile
> @@ -88,6 +88,7 @@ obj-$(CONFIG_LEDS_LM36274)		+= leds-lm36274.o
>  obj-$(CONFIG_LEDS_TPS6105X)		+= leds-tps6105x.o
>  
>  # LED SPI Drivers
> +obj-$(CONFIG_LEDS_APA102C)		+= leds-apa102c.o
>  obj-$(CONFIG_LEDS_CR0014114)		+= leds-cr0014114.o
>  obj-$(CONFIG_LEDS_DAC124S085)		+= leds-dac124s085.o
>  obj-$(CONFIG_LEDS_EL15203000)		+= leds-el15203000.o
> diff --git a/drivers/leds/leds-apa102c.c b/drivers/leds/leds-apa102c.c
> new file mode 100644
> index 000000000000..0043e7a6235b
> --- /dev/null
> +++ b/drivers/leds/leds-apa102c.c
> @@ -0,0 +1,306 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/leds.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/spi/spi.h>
> +#include <uapi/linux/uleds.h>
> +#include "leds.h"
> +
> +/*
> + * Copyright (C) 2020 BayLibre, SAS
> + * Author: Nicolas Belin <nbelin@baylibre.com>
> + */
> +
> +/*
> + *  APA102C SPI protocol description:
> + *  +------+----------------------------------------+------+
> + *  |START |               DATA FIELD:              | END  |
> + *  |FRAME |               N LED FRAMES             |FRAME |
> + *  +------+------+------+------+------+-----+------+------+
> + *  | 0*32 | LED1 | LED2 | LED3 | LED4 | --- | LEDN | 1*32 |
> + *  +------+------+------+------+------+-----+------+------+
> + *
> + *  +-----------------------------------+
> + *  |START FRAME 32bits                 |
> + *  +--------+--------+--------+--------+
> + *  |00000000|00000000|00000000|00000000|
> + *  +--------+--------+--------+--------+
> + *
> + *  +------------------------------------+
> + *  |LED  FRAME 32bits                   |
> + *  +---+-----+--------+--------+--------+
> + *  |111|LUMA |  BLUE  | GREEN  |  RED   |
> + *  +---+-----+--------+--------+--------+
> + *  |3b |5bits| 8bits  | 8bits  | 8bits  |
> + *  +---+-----+--------+--------+--------+
> + *  |MSB   LSB|MSB  LSB|MSB  LSB|MSB  LSB|
> + *  +---+-----+--------+--------+--------+
> + *
> + *  +-----------------------------------+
> + *  |END FRAME 32bits                   |
> + *  +--------+--------+--------+--------+
> + *  |11111111|11111111|11111111|11111111|
> + *  +--------+--------+--------+--------+
> + */
> +
> +/* apa102c default settings */
> +#define GLOBAL_MAX_BRIGHTNESS	GENMASK(4, 0)
> +#define RGB_MAX_BRIGHTNESS	GENMASK(7, 0)
> +#define START_BYTE		0
> +#define END_BYTE		GENMASK(7, 0)
> +#define LED_FRAME_HEADER	GENMASK(7, 5)
> +
> +struct apa102c_led {
> +	struct apa102c		*priv;
> +	char			name[LED_MAX_NAME_SIZE];

This is not needed, LED core takes care of parsing LED name.

> +	int			color_id;
> +	struct led_classdev	cdev;
> +};
> +
> +struct apa102c_rgbled {
> +	/* the 4 components of the apa102c rgb led:
> +	 * global brightness, blue, green and red in that order
> +	 */
> +	struct apa102c_led	component[4];
> +};
> +
> +struct apa102c {
> +	size_t			led_count;
> +	struct device		*dev;
> +	struct mutex		lock;
> +	struct spi_device	*spi;
> +	u8			*spi_buf;
> +	struct apa102c_rgbled	rgb_leds[];
> +};
> +
> +static int apa102c_sync(struct apa102c *priv)
> +{
> +	int	ret;
> +	size_t	i;
> +	struct apa102c_rgbled *leds = priv->rgb_leds;
> +	u8	*buf = priv->spi_buf;
> +
> +	/* creating the data that will be sent to all the leds at once */
> +	for (i = 0; i < 4; i++)
> +		*buf++ = START_BYTE;
> +
> +	/* looping on each RGB led component, getting the global brightness
> +	 * then B, G and R values
> +	 */
> +	for (i = 0; i < priv->led_count; i++) {
> +		*buf++ = LED_FRAME_HEADER |
> +			 leds[i].component[0].cdev.brightness;
> +		*buf++ = leds[i].component[1].cdev.brightness;
> +		*buf++ = leds[i].component[2].cdev.brightness;
> +		*buf++ = leds[i].component[3].cdev.brightness;
> +	}

The problem is that in the monochrome LED approach (i.e. the only
available one) you have to alter the state of only one LED - that
on behalf of which the call is made.

And anyway, you're doing here much too much, taking into account
that this function is called from brightness_set op.

Isn't it possible to update only one LED ?

> +
> +	for (i = 0; i < 4; i++)
> +		*buf++ = END_BYTE;
> +
> +	ret = spi_write(priv->spi, priv->spi_buf,
> +			(priv->led_count + 2) * sizeof(u32));
> +
> +	return ret;
> +}
> +
> +static int apa102c_brightness_set(struct led_classdev *cdev,
> +			   enum led_brightness brightness)
> +{
> +	int			ret;
> +	struct apa102c_led	*led = container_of(cdev,
> +						    struct apa102c_led,
> +						    cdev);
> +
> +	mutex_lock(&led->priv->lock);
> +	/* updating the brightness then synching all the leds */
> +	cdev->brightness = brightness;

LED core handles that. Please drop this assignment.

> +	ret = apa102c_sync(led->priv);
> +	mutex_unlock(&led->priv->lock);
> +
> +	return ret;
> +}
> +
> +static int apa102c_probe_dt(struct apa102c *priv)
> +{
> +	int			ret = 0;
> +	u32			i;
> +	struct apa102c_rgbled	*rgb_led;
> +	struct apa102c_led	*led;
> +	struct fwnode_handle	*child;
> +	const char		*rgb_led_name;
> +	const char		*led_component_name;
> +
> +	/* each node corresponds to an rgb led */
> +	device_for_each_child_node(priv->dev, child) {
> +
> +		ret = fwnode_property_read_u32(child, "reg", &i);
> +		if (ret) {
> +			dev_err(priv->dev, "coudld not read reg %d\n", ret);
> +			goto child_out;
> +		}
> +
> +		if (i >= priv->led_count) {
> +			ret = -EINVAL;
> +			dev_err(priv->dev, "reg value too big\n");
> +			goto child_out;
> +		}
> +
> +		rgb_led = &priv->rgb_leds[i];
> +		/* checking if this led config is already used by checking if
> +		 * the priv member of the global_brightness led as already
> +		 * been set
> +		 */
> +		if (rgb_led->component[0].priv) {
> +			ret = -EINVAL;
> +			dev_err(priv->dev, "using the same reg value twice\n");
> +			goto child_out;
> +		}
> +
> +		ret = fwnode_property_read_string(child, "label",
> +						  &rgb_led_name);
> +		if (ret) {
> +			ret = -EINVAL;
> +			dev_err(priv->dev, "missing rgb led name\n");
> +			goto child_out;
> +		}
> +
> +		/* setting a color_id value for each of the 4 components of the
> +		 * apa102c RGB led. The first component is the global brightness
> +		 * of the led and thus has no color. The order of the colors
> +		 * after the global brightness is then blue, green and red
> +		 * in that order. It corresponds to the order in which the
> +		 * values are sent using spi
> +		 */
> +		rgb_led->component[0].color_id = -1; //no color
> +		rgb_led->component[1].color_id = LED_COLOR_ID_BLUE;
> +		rgb_led->component[2].color_id = LED_COLOR_ID_GREEN;
> +		rgb_led->component[3].color_id = LED_COLOR_ID_RED;

Each LED has to have separate DT node. We haven't had so far use case
with global brightness but I think it would be OK to add
LED_COLOR_ID_LUMA for that. But please hold on with making any changes
until Pavel will express his opinion on that.

> +
> +		/* now looping on each component and registering a corresponding
> +		 * led
> +		 */
> +		for (i = 0; i < 4; i++) {
> +			led = &rgb_led->component[i];
> +			if (led->color_id == -1) {
> +				/* the global brightness has no defined name
> +				 * so setting it to "brightness". It also has
> +				 * a different MAX_BRIGHTNESS value.
> +				 * If a trigger is present, setting it on this
> +				 * component
> +				 */
> +				led->cdev.max_brightness =
> +					GLOBAL_MAX_BRIGHTNESS;
> +				led_component_name = "brightness";
> +				fwnode_property_read_string(child,
> +					"linux,default-trigger",
> +					&led->cdev.default_trigger);
> +			} else {
> +				/* using the color name defined by the framework
> +				 * for the B, G and R components
> +				 */
> +				led->cdev.max_brightness = RGB_MAX_BRIGHTNESS;
> +				led_component_name = led_colors[led->color_id];
> +			}
> +
> +			/* the rest is common to all the components */
> +			led->priv = priv;
> +			led->cdev.brightness_set_blocking =
> +				apa102c_brightness_set;
> +			/* creating our own led name to differentiate the 4
> +			 * components
> +			 */
> +			snprintf(led->name, sizeof(led->name),
> +				 "%s_%s", rgb_led_name, led_component_name);
> +			led->cdev.name = led->name;
> +

LED core can do the above for you, if only you provide it with LED
fwnode.

> +			ret = devm_led_classdev_register_ext(priv->dev,
> +							     &led->cdev,
> +							     NULL);

There is no point in using *ext API and not passing led_init_data to it.
Please pass struct led_init_data in the third argument, with initialized
only its fwnode property.

> +			if (ret) {
> +				dev_err(priv->dev, "led register err: %d\n",
> +					ret);
> +				goto child_out;
> +			}
> +		}
> +	}
> +
> +child_out:
> +	return ret;
> +}
> +
> +static int apa102c_probe(struct spi_device *spi)
> +{
> +	struct apa102c	*priv;
> +	size_t		led_count;
> +	int		ret;
> +
> +	led_count = device_get_child_node_count(&spi->dev);
> +	if (!led_count) {
> +		dev_err(&spi->dev, "No LEDs defined in device tree!");
> +		return -ENODEV;
> +	}
> +
> +	priv = devm_kzalloc(&spi->dev,
> +			    struct_size(priv, rgb_leds, led_count),
> +			    GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->spi_buf = devm_kzalloc(&spi->dev,
> +				     (led_count + 2) * sizeof(u32),
> +				      GFP_KERNEL);
> +	if (!priv->spi_buf)
> +		return -ENOMEM;
> +
> +	mutex_init(&priv->lock);
> +	priv->led_count	= led_count;
> +	priv->dev	= &spi->dev;
> +	priv->spi	= spi;
> +
> +	ret = apa102c_probe_dt(priv);
> +	if (ret)
> +		return ret;
> +
> +	/* Set the LEDs with default values at start */
> +	apa102c_sync(priv);
> +	if (ret)
> +		return ret;
> +
> +	spi_set_drvdata(spi, priv);
> +
> +	return 0;
> +}
> +
> +static int apa102c_remove(struct spi_device *spi)
> +{
> +	struct apa102c *priv = spi_get_drvdata(spi);
> +
> +	mutex_destroy(&priv->lock);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id apa102c_dt_ids[] = {
> +	{ .compatible = "shiji,apa102c", },
> +	{},
> +};
> +
> +MODULE_DEVICE_TABLE(of, apa102c_dt_ids);
> +
> +static struct spi_driver apa102c_driver = {
> +	.probe		= apa102c_probe,
> +	.remove		= apa102c_remove,
> +	.driver = {
> +		.name		= KBUILD_MODNAME,
> +		.of_match_table	= apa102c_dt_ids,
> +	},
> +};
> +
> +module_spi_driver(apa102c_driver);
> +
> +MODULE_AUTHOR("Nicolas Belin <nbelin@baylibre.com>");
> +MODULE_DESCRIPTION("apa102c LED driver");
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("spi:apa102c");
> 

-- 
Best regards,
Jacek Anaszewski

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v3 3/3] drivers: leds: add support for apa102c leds
  2020-03-06 20:20   ` Jacek Anaszewski
@ 2020-03-09  9:59     ` Nicolas Belin
  0 siblings, 0 replies; 10+ messages in thread
From: Nicolas Belin @ 2020-03-09  9:59 UTC (permalink / raw)
  To: Jacek Anaszewski
  Cc: linux-kernel, linux-leds, Pavel Machek, Dan Murphy, devicetree,
	baylibre-upstreaming

Hi Jacek,

Le ven. 6 mars 2020 à 21:20, Jacek Anaszewski
<jacek.anaszewski@gmail.com> a écrit :
>
> Hi Nicolas,
>
> On 3/6/20 2:40 PM, Nicolas Belin wrote:
> > Initilial commit in order to support the apa102c RGB leds. The
> > RGB and global brightness management is done by creating 4 leds
> > from the Led Framework per apa102c led.
> >
> > Signed-off-by: Nicolas Belin <nbelin@baylibre.com>
> > ---
> >  drivers/leds/Kconfig        |  11 ++
> >  drivers/leds/Makefile       |   1 +
> >  drivers/leds/leds-apa102c.c | 306 ++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 318 insertions(+)
> >  create mode 100644 drivers/leds/leds-apa102c.c
> >
> > diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> > index d82f1dea3711..28fa6c4f65cc 100644
> > --- a/drivers/leds/Kconfig
> > +++ b/drivers/leds/Kconfig
> > @@ -69,6 +69,17 @@ config LEDS_AN30259A
> >         To compile this driver as a module, choose M here: the module
> >         will be called leds-an30259a.
> >
> > +config LEDS_APA102C
> > +     tristate "LED Support for Shiji APA102C"
> > +     depends on SPI
> > +     depends on LEDS_CLASS
> > +     help
> > +       This option enables support for the APA102C RGB LEDs
> > +       from Shiji Lighting.
> > +
> > +       To compile this driver as a module, choose M here: the module
> > +       will be called leds-apa102c.
> > +
> >  config LEDS_APU
> >       tristate "Front panel LED support for PC Engines APU/APU2/APU3 boards"
> >       depends on LEDS_CLASS
> > diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> > index d7e1107753fb..28dfe82900c5 100644
> > --- a/drivers/leds/Makefile
> > +++ b/drivers/leds/Makefile
> > @@ -88,6 +88,7 @@ obj-$(CONFIG_LEDS_LM36274)          += leds-lm36274.o
> >  obj-$(CONFIG_LEDS_TPS6105X)          += leds-tps6105x.o
> >
> >  # LED SPI Drivers
> > +obj-$(CONFIG_LEDS_APA102C)           += leds-apa102c.o
> >  obj-$(CONFIG_LEDS_CR0014114)         += leds-cr0014114.o
> >  obj-$(CONFIG_LEDS_DAC124S085)                += leds-dac124s085.o
> >  obj-$(CONFIG_LEDS_EL15203000)                += leds-el15203000.o
> > diff --git a/drivers/leds/leds-apa102c.c b/drivers/leds/leds-apa102c.c
> > new file mode 100644
> > index 000000000000..0043e7a6235b
> > --- /dev/null
> > +++ b/drivers/leds/leds-apa102c.c
> > @@ -0,0 +1,306 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +#include <linux/leds.h>
> > +#include <linux/module.h>
> > +#include <linux/of_device.h>
> > +#include <linux/spi/spi.h>
> > +#include <uapi/linux/uleds.h>
> > +#include "leds.h"
> > +
> > +/*
> > + * Copyright (C) 2020 BayLibre, SAS
> > + * Author: Nicolas Belin <nbelin@baylibre.com>
> > + */
> > +
> > +/*
> > + *  APA102C SPI protocol description:
> > + *  +------+----------------------------------------+------+
> > + *  |START |               DATA FIELD:              | END  |
> > + *  |FRAME |               N LED FRAMES             |FRAME |
> > + *  +------+------+------+------+------+-----+------+------+
> > + *  | 0*32 | LED1 | LED2 | LED3 | LED4 | --- | LEDN | 1*32 |
> > + *  +------+------+------+------+------+-----+------+------+
> > + *
> > + *  +-----------------------------------+
> > + *  |START FRAME 32bits                 |
> > + *  +--------+--------+--------+--------+
> > + *  |00000000|00000000|00000000|00000000|
> > + *  +--------+--------+--------+--------+
> > + *
> > + *  +------------------------------------+
> > + *  |LED  FRAME 32bits                   |
> > + *  +---+-----+--------+--------+--------+
> > + *  |111|LUMA |  BLUE  | GREEN  |  RED   |
> > + *  +---+-----+--------+--------+--------+
> > + *  |3b |5bits| 8bits  | 8bits  | 8bits  |
> > + *  +---+-----+--------+--------+--------+
> > + *  |MSB   LSB|MSB  LSB|MSB  LSB|MSB  LSB|
> > + *  +---+-----+--------+--------+--------+
> > + *
> > + *  +-----------------------------------+
> > + *  |END FRAME 32bits                   |
> > + *  +--------+--------+--------+--------+
> > + *  |11111111|11111111|11111111|11111111|
> > + *  +--------+--------+--------+--------+
> > + */
> > +
> > +/* apa102c default settings */
> > +#define GLOBAL_MAX_BRIGHTNESS        GENMASK(4, 0)
> > +#define RGB_MAX_BRIGHTNESS   GENMASK(7, 0)
> > +#define START_BYTE           0
> > +#define END_BYTE             GENMASK(7, 0)
> > +#define LED_FRAME_HEADER     GENMASK(7, 5)
> > +
> > +struct apa102c_led {
> > +     struct apa102c          *priv;
> > +     char                    name[LED_MAX_NAME_SIZE];
>
> This is not needed, LED core takes care of parsing LED name.
>
> > +     int                     color_id;
> > +     struct led_classdev     cdev;
> > +};
> > +
> > +struct apa102c_rgbled {
> > +     /* the 4 components of the apa102c rgb led:
> > +      * global brightness, blue, green and red in that order
> > +      */
> > +     struct apa102c_led      component[4];
> > +};
> > +
> > +struct apa102c {
> > +     size_t                  led_count;
> > +     struct device           *dev;
> > +     struct mutex            lock;
> > +     struct spi_device       *spi;
> > +     u8                      *spi_buf;
> > +     struct apa102c_rgbled   rgb_leds[];
> > +};
> > +
> > +static int apa102c_sync(struct apa102c *priv)
> > +{
> > +     int     ret;
> > +     size_t  i;
> > +     struct apa102c_rgbled *leds = priv->rgb_leds;
> > +     u8      *buf = priv->spi_buf;
> > +
> > +     /* creating the data that will be sent to all the leds at once */
> > +     for (i = 0; i < 4; i++)
> > +             *buf++ = START_BYTE;
> > +
> > +     /* looping on each RGB led component, getting the global brightness
> > +      * then B, G and R values
> > +      */
> > +     for (i = 0; i < priv->led_count; i++) {
> > +             *buf++ = LED_FRAME_HEADER |
> > +                      leds[i].component[0].cdev.brightness;
> > +             *buf++ = leds[i].component[1].cdev.brightness;
> > +             *buf++ = leds[i].component[2].cdev.brightness;
> > +             *buf++ = leds[i].component[3].cdev.brightness;
> > +     }
>
> The problem is that in the monochrome LED approach (i.e. the only
> available one) you have to alter the state of only one LED - that
> on behalf of which the call is made.
>
> And anyway, you're doing here much too much, taking into account
> that this function is called from brightness_set op.
>
> Isn't it possible to update only one LED ?

No, it is not possible. When one LED is updated and as all the leds
are cascaded, the values for all the leds have to be sent using spi.
However, what I can do is not updating the full buffer and simply the
part that corresponds to the modified LED.

>
> > +
> > +     for (i = 0; i < 4; i++)
> > +             *buf++ = END_BYTE;
> > +
> > +     ret = spi_write(priv->spi, priv->spi_buf,
> > +                     (priv->led_count + 2) * sizeof(u32));
> > +
> > +     return ret;
> > +}
> > +
> > +static int apa102c_brightness_set(struct led_classdev *cdev,
> > +                        enum led_brightness brightness)
> > +{
> > +     int                     ret;
> > +     struct apa102c_led      *led = container_of(cdev,
> > +                                                 struct apa102c_led,
> > +                                                 cdev);
> > +
> > +     mutex_lock(&led->priv->lock);
> > +     /* updating the brightness then synching all the leds */
> > +     cdev->brightness = brightness;
>
> LED core handles that. Please drop this assignment.

OK

>
> > +     ret = apa102c_sync(led->priv);
> > +     mutex_unlock(&led->priv->lock);
> > +
> > +     return ret;
> > +}
> > +
> > +static int apa102c_probe_dt(struct apa102c *priv)
> > +{
> > +     int                     ret = 0;
> > +     u32                     i;
> > +     struct apa102c_rgbled   *rgb_led;
> > +     struct apa102c_led      *led;
> > +     struct fwnode_handle    *child;
> > +     const char              *rgb_led_name;
> > +     const char              *led_component_name;
> > +
> > +     /* each node corresponds to an rgb led */
> > +     device_for_each_child_node(priv->dev, child) {
> > +
> > +             ret = fwnode_property_read_u32(child, "reg", &i);
> > +             if (ret) {
> > +                     dev_err(priv->dev, "coudld not read reg %d\n", ret);
> > +                     goto child_out;
> > +             }
> > +
> > +             if (i >= priv->led_count) {
> > +                     ret = -EINVAL;
> > +                     dev_err(priv->dev, "reg value too big\n");
> > +                     goto child_out;
> > +             }
> > +
> > +             rgb_led = &priv->rgb_leds[i];
> > +             /* checking if this led config is already used by checking if
> > +              * the priv member of the global_brightness led as already
> > +              * been set
> > +              */
> > +             if (rgb_led->component[0].priv) {
> > +                     ret = -EINVAL;
> > +                     dev_err(priv->dev, "using the same reg value twice\n");
> > +                     goto child_out;
> > +             }
> > +
> > +             ret = fwnode_property_read_string(child, "label",
> > +                                               &rgb_led_name);
> > +             if (ret) {
> > +                     ret = -EINVAL;
> > +                     dev_err(priv->dev, "missing rgb led name\n");
> > +                     goto child_out;
> > +             }
> > +
> > +             /* setting a color_id value for each of the 4 components of the
> > +              * apa102c RGB led. The first component is the global brightness
> > +              * of the led and thus has no color. The order of the colors
> > +              * after the global brightness is then blue, green and red
> > +              * in that order. It corresponds to the order in which the
> > +              * values are sent using spi
> > +              */
> > +             rgb_led->component[0].color_id = -1; //no color
> > +             rgb_led->component[1].color_id = LED_COLOR_ID_BLUE;
> > +             rgb_led->component[2].color_id = LED_COLOR_ID_GREEN;
> > +             rgb_led->component[3].color_id = LED_COLOR_ID_RED;
>
> Each LED has to have separate DT node. We haven't had so far use case
> with global brightness but I think it would be OK to add
> LED_COLOR_ID_LUMA for that. But please hold on with making any changes
> until Pavel will express his opinion on that.

It would be nice to have LED_COLOR_ID_LUMA. However, I don't feel like
it is really a color. I'll wait for Pavel's opinion.

>
> > +
> > +             /* now looping on each component and registering a corresponding
> > +              * led
> > +              */
> > +             for (i = 0; i < 4; i++) {
> > +                     led = &rgb_led->component[i];
> > +                     if (led->color_id == -1) {
> > +                             /* the global brightness has no defined name
> > +                              * so setting it to "brightness". It also has
> > +                              * a different MAX_BRIGHTNESS value.
> > +                              * If a trigger is present, setting it on this
> > +                              * component
> > +                              */
> > +                             led->cdev.max_brightness =
> > +                                     GLOBAL_MAX_BRIGHTNESS;
> > +                             led_component_name = "brightness";
> > +                             fwnode_property_read_string(child,
> > +                                     "linux,default-trigger",
> > +                                     &led->cdev.default_trigger);
> > +                     } else {
> > +                             /* using the color name defined by the framework
> > +                              * for the B, G and R components
> > +                              */
> > +                             led->cdev.max_brightness = RGB_MAX_BRIGHTNESS;
> > +                             led_component_name = led_colors[led->color_id];
> > +                     }
> > +
> > +                     /* the rest is common to all the components */
> > +                     led->priv = priv;
> > +                     led->cdev.brightness_set_blocking =
> > +                             apa102c_brightness_set;
> > +                     /* creating our own led name to differentiate the 4
> > +                      * components
> > +                      */
> > +                     snprintf(led->name, sizeof(led->name),
> > +                              "%s_%s", rgb_led_name, led_component_name);
> > +                     led->cdev.name = led->name;
> > +
>
> LED core can do the above for you, if only you provide it with LED
> fwnode.

I was doing that to have a led_name formatted as "label_color". I am
going to check for LED functions that could work for me and come back
to you if find other functions that are not present.

>
> > +                     ret = devm_led_classdev_register_ext(priv->dev,
> > +                                                          &led->cdev,
> > +                                                          NULL);
>
> There is no point in using *ext API and not passing led_init_data to it.
> Please pass struct led_init_data in the third argument, with initialized
> only its fwnode property.
>
> > +                     if (ret) {
> > +                             dev_err(priv->dev, "led register err: %d\n",
> > +                                     ret);
> > +                             goto child_out;
> > +                     }
> > +             }
> > +     }
> > +
> > +child_out:
> > +     return ret;
> > +}
> > +
> > +static int apa102c_probe(struct spi_device *spi)
> > +{
> > +     struct apa102c  *priv;
> > +     size_t          led_count;
> > +     int             ret;
> > +
> > +     led_count = device_get_child_node_count(&spi->dev);
> > +     if (!led_count) {
> > +             dev_err(&spi->dev, "No LEDs defined in device tree!");
> > +             return -ENODEV;
> > +     }
> > +
> > +     priv = devm_kzalloc(&spi->dev,
> > +                         struct_size(priv, rgb_leds, led_count),
> > +                         GFP_KERNEL);
> > +     if (!priv)
> > +             return -ENOMEM;
> > +
> > +     priv->spi_buf = devm_kzalloc(&spi->dev,
> > +                                  (led_count + 2) * sizeof(u32),
> > +                                   GFP_KERNEL);
> > +     if (!priv->spi_buf)
> > +             return -ENOMEM;
> > +
> > +     mutex_init(&priv->lock);
> > +     priv->led_count = led_count;
> > +     priv->dev       = &spi->dev;
> > +     priv->spi       = spi;
> > +
> > +     ret = apa102c_probe_dt(priv);
> > +     if (ret)
> > +             return ret;
> > +
> > +     /* Set the LEDs with default values at start */
> > +     apa102c_sync(priv);
> > +     if (ret)
> > +             return ret;
> > +
> > +     spi_set_drvdata(spi, priv);
> > +
> > +     return 0;
> > +}
> > +
> > +static int apa102c_remove(struct spi_device *spi)
> > +{
> > +     struct apa102c *priv = spi_get_drvdata(spi);
> > +
> > +     mutex_destroy(&priv->lock);
> > +
> > +     return 0;
> > +}
> > +
> > +static const struct of_device_id apa102c_dt_ids[] = {
> > +     { .compatible = "shiji,apa102c", },
> > +     {},
> > +};
> > +
> > +MODULE_DEVICE_TABLE(of, apa102c_dt_ids);
> > +
> > +static struct spi_driver apa102c_driver = {
> > +     .probe          = apa102c_probe,
> > +     .remove         = apa102c_remove,
> > +     .driver = {
> > +             .name           = KBUILD_MODNAME,
> > +             .of_match_table = apa102c_dt_ids,
> > +     },
> > +};
> > +
> > +module_spi_driver(apa102c_driver);
> > +
> > +MODULE_AUTHOR("Nicolas Belin <nbelin@baylibre.com>");
> > +MODULE_DESCRIPTION("apa102c LED driver");
> > +MODULE_LICENSE("GPL v2");
> > +MODULE_ALIAS("spi:apa102c");
> >
>
> --
> Best regards,
> Jacek Anaszewski

Thanks,

Regards,

Nicolas

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v3 1/3] dt-bindings: Document shiji vendor-prefix
  2020-03-06 13:40 ` [PATCH v3 1/3] dt-bindings: Document shiji vendor-prefix Nicolas Belin
@ 2020-03-09 21:05   ` Rob Herring
  0 siblings, 0 replies; 10+ messages in thread
From: Rob Herring @ 2020-03-09 21:05 UTC (permalink / raw)
  To: Nicolas Belin
  Cc: linux-kernel, linux-leds, jacek.anaszewski, pavel, dmurphy,
	devicetree, baylibre-upstreaming, Nicolas Belin

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 <nbelin@baylibre.com>
> ---
>  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.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v3 2/3] dt-bindings: leds: Shiji Lighting APA102C LED
  2020-03-06 13:40 ` [PATCH v3 2/3] dt-bindings: leds: Shiji Lighting APA102C LED Nicolas Belin
  2020-03-06 20:19   ` Jacek Anaszewski
@ 2020-03-13 13:06   ` Rob Herring
  1 sibling, 0 replies; 10+ messages in thread
From: Rob Herring @ 2020-03-13 13:06 UTC (permalink / raw)
  To: Nicolas Belin
  Cc: linux-kernel, linux-leds, jacek.anaszewski, pavel, dmurphy,
	devicetree, baylibre-upstreaming

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 <nbelin@baylibre.com>
> ---
>  .../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 <nbelin@baylibre.com>
> +
> +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
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v3 3/3] drivers: leds: add support for apa102c leds
  2020-03-06 13:40 ` [PATCH v3 3/3] drivers: leds: add support for apa102c leds Nicolas Belin
  2020-03-06 20:20   ` Jacek Anaszewski
@ 2020-03-24  9:42   ` Pavel Machek
  1 sibling, 0 replies; 10+ messages in thread
From: Pavel Machek @ 2020-03-24  9:42 UTC (permalink / raw)
  To: Nicolas Belin
  Cc: linux-kernel, linux-leds, jacek.anaszewski, dmurphy, devicetree,
	baylibre-upstreaming

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

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2020-03-24  9:42 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-06 13:40 [PATCH v3 0/3] leds: add support for apa102c leds Nicolas Belin
2020-03-06 13:40 ` [PATCH v3 1/3] dt-bindings: Document shiji vendor-prefix Nicolas Belin
2020-03-09 21:05   ` Rob Herring
2020-03-06 13:40 ` [PATCH v3 2/3] dt-bindings: leds: Shiji Lighting APA102C LED Nicolas Belin
2020-03-06 20:19   ` Jacek Anaszewski
2020-03-13 13:06   ` Rob Herring
2020-03-06 13:40 ` [PATCH v3 3/3] drivers: leds: add support for apa102c leds Nicolas Belin
2020-03-06 20:20   ` Jacek Anaszewski
2020-03-09  9:59     ` Nicolas Belin
2020-03-24  9:42   ` Pavel Machek

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).