All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/2] leds: aw21024: Add support for Awinic's AW21024
@ 2022-05-13 19:04 Kyle Swenson
  2022-05-13 19:04 ` [PATCH 2/2] dt-bindings: leds: Add aw21024 binding Kyle Swenson
                   ` (4 more replies)
  0 siblings, 5 replies; 14+ messages in thread
From: Kyle Swenson @ 2022-05-13 19:04 UTC (permalink / raw)
  To: pavel, robh+dt, krzk+dt; +Cc: linux-leds, devicetree, Kyle Swenson

The Awinic AW21024 LED controller is a 24-channel RGB LED controller.
Each LED on the controller can be controlled individually or grouped
with other LEDs on the controller to form a multi-color LED.  Arbitrary
combinations of individual and grouped LED control should be possible.

Signed-off-by: Kyle Swenson <kyle.swenson@est.tech>
---
 drivers/leds/Kconfig        |  11 ++
 drivers/leds/Makefile       |   1 +
 drivers/leds/leds-aw21024.c | 314 ++++++++++++++++++++++++++++++++++++
 3 files changed, 326 insertions(+)
 create mode 100644 drivers/leds/leds-aw21024.c

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index a49979f41eee..c813d7c16ff8 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -102,10 +102,21 @@ config LEDS_AW2013
 	  LED driver.
 
 	  To compile this driver as a module, choose M here: the module
 	  will be called leds-aw2013.
 
+config LEDS_AW21024
+	tristate "LED Support for Awinic AW21024"
+	depends on LEDS_CLASS
+	depends on LEDS_CLASS_MULTICOLOR || !LEDS_CLASS_MULTICOLOR
+	help
+	  If you say yes here you get support for Awinic's AW21024, a 24-channel
+	  RGB LED Driver.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called leds-aw21024.
+
 config LEDS_BCM6328
 	tristate "LED Support for Broadcom BCM6328"
 	depends on LEDS_CLASS
 	depends on HAS_IOMEM
 	depends on OF
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index 4fd2f92cd198..09a0e3cb21cb 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -14,10 +14,11 @@ obj-$(CONFIG_LEDS_ADP5520)		+= leds-adp5520.o
 obj-$(CONFIG_LEDS_AN30259A)		+= leds-an30259a.o
 obj-$(CONFIG_LEDS_APU)			+= leds-apu.o
 obj-$(CONFIG_LEDS_ARIEL)		+= leds-ariel.o
 obj-$(CONFIG_LEDS_ASIC3)		+= leds-asic3.o
 obj-$(CONFIG_LEDS_AW2013)		+= leds-aw2013.o
+obj-$(CONFIG_LEDS_AW21024)		+= leds-aw21024.o
 obj-$(CONFIG_LEDS_BCM6328)		+= leds-bcm6328.o
 obj-$(CONFIG_LEDS_BCM6358)		+= leds-bcm6358.o
 obj-$(CONFIG_LEDS_BD2802)		+= leds-bd2802.o
 obj-$(CONFIG_LEDS_BLINKM)		+= leds-blinkm.o
 obj-$(CONFIG_LEDS_CLEVO_MAIL)		+= leds-clevo-mail.o
diff --git a/drivers/leds/leds-aw21024.c b/drivers/leds/leds-aw21024.c
new file mode 100644
index 000000000000..4eebc3de1a8a
--- /dev/null
+++ b/drivers/leds/leds-aw21024.c
@@ -0,0 +1,314 @@
+// SPDX-License-Identifier: GPL-2.0
+// Awinic AW21024 LED chip driver
+// Copyright (C) 2022 Nordix Foundation https://www.nordix.org
+
+#include <linux/gpio/consumer.h>
+#include <linux/i2c.h>
+#include <linux/init.h>
+#include <linux/leds.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/delay.h>
+#include <linux/slab.h>
+#include <uapi/linux/uleds.h>
+
+#include <linux/led-class-multicolor.h>
+
+/* Called COL0, COL1,..., COL23 in datasheet */
+#define AW21024_REG_DC_CURRENT(_led)	(0x4a + (_led))
+
+/* Called BR0, BR1,..., BR23 in datasheet */
+#define AW21024_REG_BRIGHTNESS(_led)	(0x01 + (_led))
+
+#define AW21024_REG_UPDATE				0x49 /* Write 0x00 to update BR */
+
+#define AW21024_REG_GCR0				0x00 /* Global configuration register */
+#define AW21024_REG_GCC					0x6e /* Global current control */
+#define AW21024_REG_SW_RESET			0x7f
+#define AW21024_REG_VERSION				0x7e
+
+#define AW21024_GCR0_CHIPEN				BIT(0)
+#define AW21024_CHIP_ID					0x18
+#define AW21024_CHIP_VERSION			0xA8
+
+struct aw21024_led_data {
+	struct led_classdev_mc mc_cdev;
+	struct work_struct work;
+	unsigned int *regs;
+	unsigned int nregs;
+	struct aw21024 *parent;
+};
+
+struct aw21024 {
+	struct i2c_client *client;
+	struct device *dev;
+	struct gpio_desc *enable_gpio;
+	struct mutex lock;
+	struct aw21024_led_data **leds;
+	unsigned int nleds;
+};
+
+static int aw21024_led_brightness_set(struct led_classdev *led_cdev,
+					enum led_brightness brightness)
+{
+	struct led_classdev_mc *mc_cdev = lcdev_to_mccdev(led_cdev);
+	struct aw21024_led_data *led = container_of(mc_cdev, struct aw21024_led_data, mc_cdev);
+	struct aw21024 *parent = led->parent;
+	int i;
+	int ret = 0;
+
+	mutex_lock(&parent->lock);
+	if (mc_cdev->num_colors && mc_cdev->subled_info) {
+		for (i = 0; i < led->nregs; i++) {
+			ret = i2c_smbus_write_byte_data(parent->client,
+					AW21024_REG_DC_CURRENT(led->regs[i]),
+					mc_cdev->subled_info[i].intensity);
+			if (ret < 0)
+				goto unlock_ret;
+
+			ret = i2c_smbus_write_byte_data(parent->client,
+					AW21024_REG_BRIGHTNESS(led->regs[i]),
+					brightness);
+			if (ret < 0)
+				goto unlock_ret;
+		}
+	} else {
+		ret = i2c_smbus_write_byte_data(parent->client,
+					AW21024_REG_DC_CURRENT(led->regs[0]), 0xFF);
+		if (ret < 0)
+			goto unlock_ret;
+
+		ret = i2c_smbus_write_byte_data(parent->client,
+					AW21024_REG_BRIGHTNESS(led->regs[0]),
+					brightness);
+		if (ret < 0)
+			goto unlock_ret;
+	}
+	ret = i2c_smbus_write_byte_data(parent->client, AW21024_REG_UPDATE, 0x0);
+unlock_ret:
+	mutex_unlock(&parent->lock);
+	return ret;
+}
+
+static int aw21024_probe_dt(struct aw21024 *data)
+{
+	struct device *dev = &data->client->dev;
+	struct fwnode_handle *child = NULL;
+	struct fwnode_handle *led_node = NULL;
+	struct led_init_data init_data = {};
+	u32 color_id;
+	int  ret, num_colors;
+	unsigned int nleds = 0;
+	struct aw21024_led_data *led;
+	struct led_classdev *led_cdev;
+	struct mc_subled *mc_led_info;
+
+	nleds = device_get_child_node_count(dev);
+
+	data->leds = devm_kcalloc(dev, nleds, sizeof(*(data->leds)), GFP_KERNEL);
+	if (!data->leds)
+		return -ENOMEM;
+
+	device_for_each_child_node(dev, child) {
+		led = devm_kzalloc(dev, sizeof(struct aw21024_led_data), GFP_KERNEL);
+		if (!led) {
+			ret = -ENOMEM;
+			goto ret_put_child;
+		}
+		led->parent = data;
+		led_cdev = &led->mc_cdev.led_cdev;
+		init_data.fwnode = child;
+
+		led_cdev->brightness_set_blocking = aw21024_led_brightness_set;
+		data->leds[data->nleds] = led;
+
+		ret = fwnode_property_count_u32(child, "reg");
+		if (ret < 0) {
+			dev_err(dev, "reg property is invalid in node %s\n",
+					fwnode_get_name(child));
+			goto ret_put_child;
+		}
+
+		led->regs = devm_kcalloc(dev, ret, sizeof(*(led->regs)), GFP_KERNEL);
+		led->nregs = ret;
+		if (!led->regs) {
+			ret = -ENOMEM;
+			goto ret_put_child;
+		}
+
+		ret = fwnode_property_read_u32_array(child, "reg", led->regs, led->nregs);
+		if (ret) {
+			dev_err(dev, "Failed to read reg array, error=%d\n", ret);
+			goto ret_put_child;
+		}
+
+		if (led->nregs > 1) {
+			mc_led_info = devm_kcalloc(dev, led->nregs,
+						    sizeof(*mc_led_info), GFP_KERNEL);
+			if (!mc_led_info) {
+				ret = -ENOMEM;
+				goto ret_put_child;
+			}
+
+			num_colors = 0;
+			fwnode_for_each_child_node(child, led_node) {
+				if (num_colors > led->nregs) {
+					ret = -EINVAL;
+					fwnode_handle_put(led_node);
+					goto ret_put_child;
+				}
+
+				ret = fwnode_property_read_u32(led_node, "color",
+							       &color_id);
+				if (ret) {
+					fwnode_handle_put(led_node);
+					goto ret_put_child;
+				}
+				mc_led_info[num_colors].color_index = color_id;
+				num_colors++;
+			}
+
+			led->mc_cdev.num_colors = num_colors;
+			led->mc_cdev.subled_info = mc_led_info;
+			ret = devm_led_classdev_multicolor_register_ext(dev,
+								&led->mc_cdev,
+								&init_data);
+			if (ret < 0) {
+				dev_warn(dev, "Failed to register multicolor LED %s, err=%d\n",
+						    fwnode_get_name(child), ret);
+				goto ret_put_child;
+			}
+		} else {
+			ret = devm_led_classdev_register_ext(dev,
+							    &led->mc_cdev.led_cdev, &init_data);
+			if (ret < 0) {
+				dev_warn(dev, "Failed to register LED %s, err=%d\n",
+						fwnode_get_name(child), ret);
+				goto ret_put_child;
+			}
+		}
+		data->nleds++;
+	}
+
+	return 0;
+
+ret_put_child:
+	fwnode_handle_put(child);
+	return ret;
+}
+
+/* Expected to be called prior to registering with the LEDs class */
+static int aw21024_configure(struct aw21024 *priv)
+{
+	int ret = 0;
+	struct i2c_client *client = priv->client;
+
+	ret = i2c_smbus_write_byte_data(client, AW21024_REG_GCR0, AW21024_GCR0_CHIPEN);
+	if (ret < 0) {
+		dev_err(&client->dev, "Failed to write chip enable\n");
+		return -ENODEV;
+	}
+
+	ret = i2c_smbus_read_byte_data(client, AW21024_REG_SW_RESET);
+	if (ret < 0) {
+		dev_err(&client->dev, "Failed to read chip id\n");
+		return -ENODEV;
+	}
+
+	if (ret != AW21024_CHIP_ID) {
+		dev_err(&client->dev, "Chip ID 0x%02X doesn't match expected (0x%02X)\n",
+								ret, AW21024_CHIP_ID);
+		return -ENODEV;
+	}
+
+	ret = i2c_smbus_read_byte_data(client, AW21024_REG_VERSION);
+	if (ret < 0) {
+		dev_err(&client->dev, "Failed to read chip version\n");
+		return -ENODEV;
+	}
+	if (ret != AW21024_CHIP_VERSION)
+		dev_warn(&client->dev, "Chip version 0x%02X doesn't match expected 0x%02X\n",
+								ret, AW21024_CHIP_VERSION);
+
+	i2c_smbus_write_byte_data(client, AW21024_REG_SW_RESET, 0x00);
+	mdelay(2);
+	i2c_smbus_write_byte_data(client, AW21024_REG_GCR0, AW21024_GCR0_CHIPEN);
+	i2c_smbus_write_byte_data(client, AW21024_REG_GCC, 0xFF);
+
+	return 0;
+}
+
+static int aw21024_probe(struct i2c_client *client)
+{
+	struct aw21024 *priv;
+	int ret;
+
+	priv = devm_kzalloc(&client->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->client = client;
+	priv->dev = &client->dev;
+
+	mutex_init(&priv->lock);
+
+	priv->enable_gpio = devm_gpiod_get_optional(priv->dev, "enable", GPIOD_OUT_LOW);
+	if (IS_ERR(priv->enable_gpio))
+		return dev_err_probe(priv->dev, PTR_ERR(priv->enable_gpio),
+				       "Failed to get enable GPIO\n");
+
+	if (priv->enable_gpio) {
+		mdelay(1);
+		gpiod_direction_output(priv->enable_gpio, 1);
+		mdelay(1);
+	}
+
+	i2c_set_clientdata(client, priv);
+
+	ret = aw21024_configure(priv);
+	if (ret < 0)
+		return ret;
+
+	return aw21024_probe_dt(priv);
+}
+
+static int aw21024_remove(struct i2c_client *client)
+{
+	struct aw21024 *priv = i2c_get_clientdata(client);
+	int ret;
+
+	ret = gpiod_direction_output(priv->enable_gpio, 0);
+	if (ret)
+		dev_err(priv->dev, "Failed to disable chip, err=%d\n", ret);
+
+	mutex_destroy(&priv->lock);
+	return 0;
+}
+
+static const struct i2c_device_id aw21024_id[] = {
+	{ "aw21024", 0 }, /* 24 Channel */
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, aw21024_id);
+
+static const struct of_device_id of_aw21024_leds_match[] = {
+	{ .compatible = "awinic,aw21024", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, of_aw21024_leds_match);
+
+static struct i2c_driver aw21024_driver = {
+	.driver		= {
+		.name	= "aw21024",
+		.of_match_table = of_match_ptr(of_aw21024_leds_match),
+	},
+	.probe_new		= aw21024_probe,
+	.remove		= aw21024_remove,
+	.id_table = aw21024_id,
+};
+module_i2c_driver(aw21024_driver);
+
+MODULE_AUTHOR("Kyle Swenson <kyle.swenson@est.tech>");
+MODULE_DESCRIPTION("Awinic AW21024 LED driver");
+MODULE_LICENSE("GPL");
-- 
2.36.1


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

* [PATCH 2/2] dt-bindings: leds: Add aw21024 binding
  2022-05-13 19:04 [PATCH 1/2] leds: aw21024: Add support for Awinic's AW21024 Kyle Swenson
@ 2022-05-13 19:04 ` Kyle Swenson
  2022-05-13 20:48   ` Rob Herring
  2022-05-17  9:08   ` Krzysztof Kozlowski
  2022-05-16 10:21 ` [PATCH 1/2] leds: aw21024: Add support for Awinic's AW21024 kernel test robot
                   ` (3 subsequent siblings)
  4 siblings, 2 replies; 14+ messages in thread
From: Kyle Swenson @ 2022-05-13 19:04 UTC (permalink / raw)
  To: pavel, robh+dt, krzk+dt; +Cc: linux-leds, devicetree, Kyle Swenson

Add device-tree bindings for Awinic's aw21024 24 channel RGB LED Driver.

Datasheet:
https://www.awinic.com/Public/Uploads/uploadfile/files/20200511/20200511165751_5eb9138fcd9e3.PDF

Signed-off-by: Kyle Swenson <kyle.swenson@est.tech>
---
 .../bindings/leds/leds-aw21024.yaml           | 157 ++++++++++++++++++
 1 file changed, 157 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/leds/leds-aw21024.yaml

diff --git a/Documentation/devicetree/bindings/leds/leds-aw21024.yaml b/Documentation/devicetree/bindings/leds/leds-aw21024.yaml
new file mode 100644
index 000000000000..1180c02b5d21
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/leds-aw21024.yaml
@@ -0,0 +1,157 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/leds/leds-aw21024.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: AWINIC AW21024 24-channel LED Driver
+
+maintainers:
+  - Kyle Swenson <kyle.swenson@est.tech>
+
+description: |
+  The AW21024 is a 24-channel LED driver with an I2C interface.
+
+  For more product information please see the link below:
+  https://www.awinic.com/en/index/pageview/catid/19/id/28.html
+
+properties:
+  compatible:
+    const: awinic,aw21024
+
+  reg:
+    maxItems: 1
+    description:
+      I2C peripheral address
+
+  enable-gpios:
+    maxItems: 1
+    description: GPIO pin to enable/disable the device.
+
+  '#address-cells':
+    const: 1
+
+  '#size-cells':
+    const: 0
+
+patternProperties:
+  '^multi-led@[0-9a-f]$':
+    type: object
+    $ref: leds-class-multicolor.yaml#
+    properties:
+      reg:
+        minItems: 1
+        maxItems: 24
+        description:
+          Denotes the LED indicies that should be grouped into a
+          single multi-color LED.
+
+    patternProperties:
+      "(^led-[0-9a-f]$|led)":
+        type: object
+        $ref: common.yaml#
+
+patternProperties:
+  "^led@[0-2]$":
+    type: object
+    $ref: common.yaml#
+
+    properties:
+      reg:
+        description: Index of the LED.
+        minimum: 0
+        maximum: 23
+
+    description:
+      Specifies a single LED at the specified index
+
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+   #include <dt-bindings/gpio/gpio.h>
+   #include <dt-bindings/leds/common.h>
+
+   i2c {
+       #address-cells = <1>;
+       #size-cells = <0>;
+
+        led-controller@30 {
+            compatible = "awinic,aw21024";
+            #address-cells = <1>;
+            #size-cells = <0>;
+            reg = <0x30>;
+            enable-gpios = <&gpio1 23>;
+
+            multi-led@1 {
+                #address-cells = <1>;
+                #size-cells = <2>;
+                reg = <0x0 0x1 0x2>;
+                color = <LED_COLOR_ID_RGB>;
+                label = "RGB_LED1";
+
+                led-0 {
+                    color = <LED_COLOR_ID_RED>;
+                };
+
+                led-1 {
+                    color = <LED_COLOR_ID_GREEN>;
+                };
+
+                led-2 {
+                    color = <LED_COLOR_ID_BLUE>;
+                };
+
+            };
+            multi-led@2 {
+                #address-cells = <1>;
+                #size-cells = <3>;
+                reg = <0x3 0x4 0x5 0x6>;
+                color = <LED_COLOR_ID_RGB>;
+                label = "RGBW_LED1";
+
+                led-4 {
+                    color = <LED_COLOR_ID_RED>;
+                };
+
+                led-5 {
+                    color = <LED_COLOR_ID_GREEN>;
+                };
+
+                led-6 {
+                    color = <LED_COLOR_ID_BLUE>;
+                };
+
+                led-7 {
+                    color = <LED_COLOR_ID_WHITE>;
+                };
+            };
+            ready_led@3 {
+                #address-cells = <1>;
+                #size-cells = <1>;
+                reg = <0x7 0x8>;
+                label = "READY";
+                color = <LED_COLOR_ID_MULTI>;
+
+                led-8 {
+                  color = <LED_COLOR_ID_RED>;
+                };
+
+                led-9 {
+                  color = <LED_COLOR_ID_GREEN>;
+                };
+            };
+            connected_led@4 {
+                reg = <0x9>;
+                label = "CONNECTED";
+                color = <LED_COLOR_ID_BLUE>;
+            };
+        };
+    };
+
+...
-- 
2.36.1


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

* Re: [PATCH 2/2] dt-bindings: leds: Add aw21024 binding
  2022-05-13 19:04 ` [PATCH 2/2] dt-bindings: leds: Add aw21024 binding Kyle Swenson
@ 2022-05-13 20:48   ` Rob Herring
  2022-05-17  9:08   ` Krzysztof Kozlowski
  1 sibling, 0 replies; 14+ messages in thread
From: Rob Herring @ 2022-05-13 20:48 UTC (permalink / raw)
  To: Kyle Swenson; +Cc: krzk+dt, linux-leds, pavel, devicetree, robh+dt

On Fri, 13 May 2022 13:04:09 -0600, Kyle Swenson wrote:
> Add device-tree bindings for Awinic's aw21024 24 channel RGB LED Driver.
> 
> Datasheet:
> https://www.awinic.com/Public/Uploads/uploadfile/files/20200511/20200511165751_5eb9138fcd9e3.PDF
> 
> Signed-off-by: Kyle Swenson <kyle.swenson@est.tech>
> ---
>  .../bindings/leds/leds-aw21024.yaml           | 157 ++++++++++++++++++
>  1 file changed, 157 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/leds/leds-aw21024.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:
./Documentation/devicetree/bindings/leds/leds-aw21024.yaml:54:1: [error] duplication of key "patternProperties" in mapping (key-duplicates)

dtschema/dtc warnings/errors:
make[1]: *** Deleting file 'Documentation/devicetree/bindings/leds/leds-aw21024.example.dts'
Documentation/devicetree/bindings/leds/leds-aw21024.yaml: found duplicate key "patternProperties" with value "{}" (original value: "{}")
make[1]: *** [Documentation/devicetree/bindings/Makefile:26: Documentation/devicetree/bindings/leds/leds-aw21024.example.dts] Error 1
make[1]: *** Waiting for unfinished jobs....
Traceback (most recent call last):
  File "/usr/local/bin/dt-doc-validate", line 25, in check_doc
    testtree = dtschema.load(filename, line_number=line_number)
  File "/usr/local/lib/python3.10/dist-packages/dtschema/lib.py", line 914, in load
    return yaml.load(f.read())
  File "/usr/local/lib/python3.10/dist-packages/ruamel/yaml/main.py", line 434, in load
    return constructor.get_single_data()
  File "/usr/local/lib/python3.10/dist-packages/ruamel/yaml/constructor.py", line 121, in get_single_data
    return self.construct_document(node)
  File "/usr/local/lib/python3.10/dist-packages/ruamel/yaml/constructor.py", line 131, in construct_document
    for _dummy in generator:
  File "/usr/local/lib/python3.10/dist-packages/ruamel/yaml/constructor.py", line 674, in construct_yaml_map
    value = self.construct_mapping(node)
  File "/usr/local/lib/python3.10/dist-packages/ruamel/yaml/constructor.py", line 445, in construct_mapping
    return BaseConstructor.construct_mapping(self, node, deep=deep)
  File "/usr/local/lib/python3.10/dist-packages/ruamel/yaml/constructor.py", line 263, in construct_mapping
    if self.check_mapping_key(node, key_node, mapping, key, value):
  File "/usr/local/lib/python3.10/dist-packages/ruamel/yaml/constructor.py", line 294, in check_mapping_key
    raise DuplicateKeyError(*args)
ruamel.yaml.constructor.DuplicateKeyError: while constructing a mapping
  in "<unicode string>", line 4, column 1
found duplicate key "patternProperties" with value "{}" (original value: "{}")
  in "<unicode string>", line 54, column 1

To suppress this check see:
    http://yaml.readthedocs.io/en/latest/api.html#duplicate-keys


During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/usr/local/bin/dt-doc-validate", line 74, in <module>
    ret = check_doc(f)
  File "/usr/local/bin/dt-doc-validate", line 30, in check_doc
    print(filename + ":", exc.path[-1], exc.message, file=sys.stderr)
AttributeError: 'DuplicateKeyError' object has no attribute 'path'
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/leds/leds-aw21024.yaml: ignoring, error parsing file
make: *** [Makefile:1401: dt_binding_check] Error 2

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/patch/

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.


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

* Re: [PATCH 1/2] leds: aw21024: Add support for Awinic's AW21024
  2022-05-13 19:04 [PATCH 1/2] leds: aw21024: Add support for Awinic's AW21024 Kyle Swenson
  2022-05-13 19:04 ` [PATCH 2/2] dt-bindings: leds: Add aw21024 binding Kyle Swenson
@ 2022-05-16 10:21 ` kernel test robot
  2022-05-16 10:42 ` kernel test robot
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 14+ messages in thread
From: kernel test robot @ 2022-05-16 10:21 UTC (permalink / raw)
  To: Kyle Swenson, pavel, robh+dt, krzk+dt
  Cc: kbuild-all, linux-leds, devicetree, Kyle Swenson

Hi Kyle,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on pavel-leds/for-next]
[also build test ERROR on robh/for-next v5.18-rc7 next-20220513]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/intel-lab-lkp/linux/commits/Kyle-Swenson/leds-aw21024-Add-support-for-Awinic-s-AW21024/20220514-030705
base:   git://git.kernel.org/pub/scm/linux/kernel/git/pavel/linux-leds.git for-next
config: mips-randconfig-r006-20220516 (https://download.01.org/0day-ci/archive/20220516/202205161802.8WYsbizM-lkp@intel.com/config)
compiler: mips-linux-gcc (GCC) 11.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/38eeda60299918b5599f4a58714dc91f9741677c
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Kyle-Swenson/leds-aw21024-Add-support-for-Awinic-s-AW21024/20220514-030705
        git checkout 38eeda60299918b5599f4a58714dc91f9741677c
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.3.0 make.cross W=1 O=build_dir ARCH=mips SHELL=/bin/bash drivers/leds/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All error/warnings (new ones prefixed by >>):

   drivers/leds/leds-aw21024.c: In function 'aw21024_led_brightness_set':
>> drivers/leds/leds-aw21024.c:64:31: error: implicit declaration of function 'i2c_smbus_write_byte_data' [-Werror=implicit-function-declaration]
      64 |                         ret = i2c_smbus_write_byte_data(parent->client,
         |                               ^~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/leds/leds-aw21024.c: In function 'aw21024_configure':
>> drivers/leds/leds-aw21024.c:213:15: error: implicit declaration of function 'i2c_smbus_read_byte_data' [-Werror=implicit-function-declaration]
     213 |         ret = i2c_smbus_read_byte_data(client, AW21024_REG_SW_RESET);
         |               ^~~~~~~~~~~~~~~~~~~~~~~~
   drivers/leds/leds-aw21024.c: At top level:
>> drivers/leds/leds-aw21024.c:310:1: warning: data definition has no type or storage class
     310 | module_i2c_driver(aw21024_driver);
         | ^~~~~~~~~~~~~~~~~
>> drivers/leds/leds-aw21024.c:310:1: error: type defaults to 'int' in declaration of 'module_i2c_driver' [-Werror=implicit-int]
>> drivers/leds/leds-aw21024.c:310:1: warning: parameter names (without types) in function declaration
   drivers/leds/leds-aw21024.c:301:26: warning: 'aw21024_driver' defined but not used [-Wunused-variable]
     301 | static struct i2c_driver aw21024_driver = {
         |                          ^~~~~~~~~~~~~~
   drivers/leds/leds-aw21024.c:295:34: warning: 'of_aw21024_leds_match' defined but not used [-Wunused-const-variable=]
     295 | static const struct of_device_id of_aw21024_leds_match[] = {
         |                                  ^~~~~~~~~~~~~~~~~~~~~
   cc1: some warnings being treated as errors


vim +/i2c_smbus_write_byte_data +64 drivers/leds/leds-aw21024.c

    51	
    52	static int aw21024_led_brightness_set(struct led_classdev *led_cdev,
    53						enum led_brightness brightness)
    54	{
    55		struct led_classdev_mc *mc_cdev = lcdev_to_mccdev(led_cdev);
    56		struct aw21024_led_data *led = container_of(mc_cdev, struct aw21024_led_data, mc_cdev);
    57		struct aw21024 *parent = led->parent;
    58		int i;
    59		int ret = 0;
    60	
    61		mutex_lock(&parent->lock);
    62		if (mc_cdev->num_colors && mc_cdev->subled_info) {
    63			for (i = 0; i < led->nregs; i++) {
  > 64				ret = i2c_smbus_write_byte_data(parent->client,
    65						AW21024_REG_DC_CURRENT(led->regs[i]),
    66						mc_cdev->subled_info[i].intensity);
    67				if (ret < 0)
    68					goto unlock_ret;
    69	
    70				ret = i2c_smbus_write_byte_data(parent->client,
    71						AW21024_REG_BRIGHTNESS(led->regs[i]),
    72						brightness);
    73				if (ret < 0)
    74					goto unlock_ret;
    75			}
    76		} else {
    77			ret = i2c_smbus_write_byte_data(parent->client,
    78						AW21024_REG_DC_CURRENT(led->regs[0]), 0xFF);
    79			if (ret < 0)
    80				goto unlock_ret;
    81	
    82			ret = i2c_smbus_write_byte_data(parent->client,
    83						AW21024_REG_BRIGHTNESS(led->regs[0]),
    84						brightness);
    85			if (ret < 0)
    86				goto unlock_ret;
    87		}
    88		ret = i2c_smbus_write_byte_data(parent->client, AW21024_REG_UPDATE, 0x0);
    89	unlock_ret:
    90		mutex_unlock(&parent->lock);
    91		return ret;
    92	}
    93	
    94	static int aw21024_probe_dt(struct aw21024 *data)
    95	{
    96		struct device *dev = &data->client->dev;
    97		struct fwnode_handle *child = NULL;
    98		struct fwnode_handle *led_node = NULL;
    99		struct led_init_data init_data = {};
   100		u32 color_id;
   101		int  ret, num_colors;
   102		unsigned int nleds = 0;
   103		struct aw21024_led_data *led;
   104		struct led_classdev *led_cdev;
   105		struct mc_subled *mc_led_info;
   106	
   107		nleds = device_get_child_node_count(dev);
   108	
   109		data->leds = devm_kcalloc(dev, nleds, sizeof(*(data->leds)), GFP_KERNEL);
   110		if (!data->leds)
   111			return -ENOMEM;
   112	
   113		device_for_each_child_node(dev, child) {
   114			led = devm_kzalloc(dev, sizeof(struct aw21024_led_data), GFP_KERNEL);
   115			if (!led) {
   116				ret = -ENOMEM;
   117				goto ret_put_child;
   118			}
   119			led->parent = data;
   120			led_cdev = &led->mc_cdev.led_cdev;
   121			init_data.fwnode = child;
   122	
   123			led_cdev->brightness_set_blocking = aw21024_led_brightness_set;
   124			data->leds[data->nleds] = led;
   125	
   126			ret = fwnode_property_count_u32(child, "reg");
   127			if (ret < 0) {
   128				dev_err(dev, "reg property is invalid in node %s\n",
   129						fwnode_get_name(child));
   130				goto ret_put_child;
   131			}
   132	
   133			led->regs = devm_kcalloc(dev, ret, sizeof(*(led->regs)), GFP_KERNEL);
   134			led->nregs = ret;
   135			if (!led->regs) {
   136				ret = -ENOMEM;
   137				goto ret_put_child;
   138			}
   139	
   140			ret = fwnode_property_read_u32_array(child, "reg", led->regs, led->nregs);
   141			if (ret) {
   142				dev_err(dev, "Failed to read reg array, error=%d\n", ret);
   143				goto ret_put_child;
   144			}
   145	
   146			if (led->nregs > 1) {
   147				mc_led_info = devm_kcalloc(dev, led->nregs,
   148							    sizeof(*mc_led_info), GFP_KERNEL);
   149				if (!mc_led_info) {
   150					ret = -ENOMEM;
   151					goto ret_put_child;
   152				}
   153	
   154				num_colors = 0;
   155				fwnode_for_each_child_node(child, led_node) {
   156					if (num_colors > led->nregs) {
   157						ret = -EINVAL;
   158						fwnode_handle_put(led_node);
   159						goto ret_put_child;
   160					}
   161	
   162					ret = fwnode_property_read_u32(led_node, "color",
   163								       &color_id);
   164					if (ret) {
   165						fwnode_handle_put(led_node);
   166						goto ret_put_child;
   167					}
   168					mc_led_info[num_colors].color_index = color_id;
   169					num_colors++;
   170				}
   171	
   172				led->mc_cdev.num_colors = num_colors;
   173				led->mc_cdev.subled_info = mc_led_info;
   174				ret = devm_led_classdev_multicolor_register_ext(dev,
   175									&led->mc_cdev,
   176									&init_data);
   177				if (ret < 0) {
   178					dev_warn(dev, "Failed to register multicolor LED %s, err=%d\n",
   179							    fwnode_get_name(child), ret);
   180					goto ret_put_child;
   181				}
   182			} else {
   183				ret = devm_led_classdev_register_ext(dev,
   184								    &led->mc_cdev.led_cdev, &init_data);
   185				if (ret < 0) {
   186					dev_warn(dev, "Failed to register LED %s, err=%d\n",
   187							fwnode_get_name(child), ret);
   188					goto ret_put_child;
   189				}
   190			}
   191			data->nleds++;
   192		}
   193	
   194		return 0;
   195	
   196	ret_put_child:
   197		fwnode_handle_put(child);
   198		return ret;
   199	}
   200	
   201	/* Expected to be called prior to registering with the LEDs class */
   202	static int aw21024_configure(struct aw21024 *priv)
   203	{
   204		int ret = 0;
   205		struct i2c_client *client = priv->client;
   206	
   207		ret = i2c_smbus_write_byte_data(client, AW21024_REG_GCR0, AW21024_GCR0_CHIPEN);
   208		if (ret < 0) {
   209			dev_err(&client->dev, "Failed to write chip enable\n");
   210			return -ENODEV;
   211		}
   212	
 > 213		ret = i2c_smbus_read_byte_data(client, AW21024_REG_SW_RESET);
   214		if (ret < 0) {
   215			dev_err(&client->dev, "Failed to read chip id\n");
   216			return -ENODEV;
   217		}
   218	
   219		if (ret != AW21024_CHIP_ID) {
   220			dev_err(&client->dev, "Chip ID 0x%02X doesn't match expected (0x%02X)\n",
   221									ret, AW21024_CHIP_ID);
   222			return -ENODEV;
   223		}
   224	
   225		ret = i2c_smbus_read_byte_data(client, AW21024_REG_VERSION);
   226		if (ret < 0) {
   227			dev_err(&client->dev, "Failed to read chip version\n");
   228			return -ENODEV;
   229		}
   230		if (ret != AW21024_CHIP_VERSION)
   231			dev_warn(&client->dev, "Chip version 0x%02X doesn't match expected 0x%02X\n",
   232									ret, AW21024_CHIP_VERSION);
   233	
   234		i2c_smbus_write_byte_data(client, AW21024_REG_SW_RESET, 0x00);
   235		mdelay(2);
   236		i2c_smbus_write_byte_data(client, AW21024_REG_GCR0, AW21024_GCR0_CHIPEN);
   237		i2c_smbus_write_byte_data(client, AW21024_REG_GCC, 0xFF);
   238	
   239		return 0;
   240	}
   241	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

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

* Re: [PATCH 1/2] leds: aw21024: Add support for Awinic's AW21024
  2022-05-13 19:04 [PATCH 1/2] leds: aw21024: Add support for Awinic's AW21024 Kyle Swenson
  2022-05-13 19:04 ` [PATCH 2/2] dt-bindings: leds: Add aw21024 binding Kyle Swenson
  2022-05-16 10:21 ` [PATCH 1/2] leds: aw21024: Add support for Awinic's AW21024 kernel test robot
@ 2022-05-16 10:42 ` kernel test robot
  2022-05-16 12:34 ` kernel test robot
  2022-05-17  9:11 ` Krzysztof Kozlowski
  4 siblings, 0 replies; 14+ messages in thread
From: kernel test robot @ 2022-05-16 10:42 UTC (permalink / raw)
  To: Kyle Swenson, pavel, robh+dt, krzk+dt
  Cc: kbuild-all, linux-leds, devicetree, Kyle Swenson

Hi Kyle,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on pavel-leds/for-next]
[also build test WARNING on robh/for-next v5.18-rc7 next-20220513]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/intel-lab-lkp/linux/commits/Kyle-Swenson/leds-aw21024-Add-support-for-Awinic-s-AW21024/20220514-030705
base:   git://git.kernel.org/pub/scm/linux/kernel/git/pavel/linux-leds.git for-next
config: x86_64-randconfig-r011-20220516 (https://download.01.org/0day-ci/archive/20220516/202205161859.bDAFU09i-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.2.0-20) 11.2.0
reproduce (this is a W=1 build):
        # https://github.com/intel-lab-lkp/linux/commit/38eeda60299918b5599f4a58714dc91f9741677c
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Kyle-Swenson/leds-aw21024-Add-support-for-Awinic-s-AW21024/20220514-030705
        git checkout 38eeda60299918b5599f4a58714dc91f9741677c
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/leds/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/leds/leds-aw21024.c:295:34: warning: 'of_aw21024_leds_match' defined but not used [-Wunused-const-variable=]
     295 | static const struct of_device_id of_aw21024_leds_match[] = {
         |                                  ^~~~~~~~~~~~~~~~~~~~~


vim +/of_aw21024_leds_match +295 drivers/leds/leds-aw21024.c

   294	
 > 295	static const struct of_device_id of_aw21024_leds_match[] = {
   296		{ .compatible = "awinic,aw21024", },
   297		{},
   298	};
   299	MODULE_DEVICE_TABLE(of, of_aw21024_leds_match);
   300	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

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

* Re: [PATCH 1/2] leds: aw21024: Add support for Awinic's AW21024
  2022-05-13 19:04 [PATCH 1/2] leds: aw21024: Add support for Awinic's AW21024 Kyle Swenson
                   ` (2 preceding siblings ...)
  2022-05-16 10:42 ` kernel test robot
@ 2022-05-16 12:34 ` kernel test robot
  2022-05-17  9:11 ` Krzysztof Kozlowski
  4 siblings, 0 replies; 14+ messages in thread
From: kernel test robot @ 2022-05-16 12:34 UTC (permalink / raw)
  To: Kyle Swenson, pavel, robh+dt, krzk+dt
  Cc: llvm, kbuild-all, linux-leds, devicetree, Kyle Swenson

Hi Kyle,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on pavel-leds/for-next]
[also build test WARNING on robh/for-next v5.18-rc7 next-20220513]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/intel-lab-lkp/linux/commits/Kyle-Swenson/leds-aw21024-Add-support-for-Awinic-s-AW21024/20220514-030705
base:   git://git.kernel.org/pub/scm/linux/kernel/git/pavel/linux-leds.git for-next
config: hexagon-randconfig-r014-20220516 (https://download.01.org/0day-ci/archive/20220516/202205162025.GEAQ50Y7-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 853fa8ee225edf2d0de94b0dcbd31bea916e825e)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/38eeda60299918b5599f4a58714dc91f9741677c
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Kyle-Swenson/leds-aw21024-Add-support-for-Awinic-s-AW21024/20220514-030705
        git checkout 38eeda60299918b5599f4a58714dc91f9741677c
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/leds/leds-aw21024.c:295:34: warning: unused variable 'of_aw21024_leds_match' [-Wunused-const-variable]
   static const struct of_device_id of_aw21024_leds_match[] = {
                                    ^
   1 warning generated.


vim +/of_aw21024_leds_match +295 drivers/leds/leds-aw21024.c

   294	
 > 295	static const struct of_device_id of_aw21024_leds_match[] = {
   296		{ .compatible = "awinic,aw21024", },
   297		{},
   298	};
   299	MODULE_DEVICE_TABLE(of, of_aw21024_leds_match);
   300	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

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

* Re: [PATCH 2/2] dt-bindings: leds: Add aw21024 binding
  2022-05-13 19:04 ` [PATCH 2/2] dt-bindings: leds: Add aw21024 binding Kyle Swenson
  2022-05-13 20:48   ` Rob Herring
@ 2022-05-17  9:08   ` Krzysztof Kozlowski
  2022-05-17 18:31     ` Kyle Swenson
  1 sibling, 1 reply; 14+ messages in thread
From: Krzysztof Kozlowski @ 2022-05-17  9:08 UTC (permalink / raw)
  To: Kyle Swenson, pavel, robh+dt, krzk+dt; +Cc: linux-leds, devicetree

On 13/05/2022 21:04, Kyle Swenson wrote:
> Add device-tree bindings for Awinic's aw21024 24 channel RGB LED Driver.
> 
> Datasheet:
> https://www.awinic.com/Public/Uploads/uploadfile/files/20200511/20200511165751_5eb9138fcd9e3.PDF
> 
> Signed-off-by: Kyle Swenson <kyle.swenson@est.tech>
> ---
>  .../bindings/leds/leds-aw21024.yaml           | 157 ++++++++++++++++++
>  1 file changed, 157 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/leds/leds-aw21024.yaml
> 
> diff --git a/Documentation/devicetree/bindings/leds/leds-aw21024.yaml b/Documentation/devicetree/bindings/leds/leds-aw21024.yaml
> new file mode 100644
> index 000000000000..1180c02b5d21
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/leds-aw21024.yaml
> @@ -0,0 +1,157 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/leds/leds-aw21024.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: AWINIC AW21024 24-channel LED Driver
> +
> +maintainers:
> +  - Kyle Swenson <kyle.swenson@est.tech>
> +
> +description: |
> +  The AW21024 is a 24-channel LED driver with an I2C interface.
> +
> +  For more product information please see the link below:
> +  https://www.awinic.com/en/index/pageview/catid/19/id/28.html
> +
> +properties:
> +  compatible:
> +    const: awinic,aw21024
> +
> +  reg:
> +    maxItems: 1
> +    description:
> +      I2C peripheral address

Skip description, it's obvious.

> +
> +  enable-gpios:
> +    maxItems: 1
> +    description: GPIO pin to enable/disable the device.

Skip description, it's obvious.

> +
> +  '#address-cells':
> +    const: 1
> +
> +  '#size-cells':
> +    const: 0
> +
> +patternProperties:
> +  '^multi-led@[0-9a-f]$':
> +    type: object
> +    $ref: leds-class-multicolor.yaml#
> +    properties:
> +      reg:
> +        minItems: 1
> +        maxItems: 24
> +        description:
> +          Denotes the LED indicies that should be grouped into a
> +          single multi-color LED.
> +
> +    patternProperties:
> +      "(^led-[0-9a-f]$|led)":

How does this pass your own bindings? In the DTS you use underscofer
which is not here...

You need to test the bindings before sending them to people.

> +        type: object
> +        $ref: common.yaml#
> +
> +patternProperties:
> +  "^led@[0-2]$":
> +    type: object
> +    $ref: common.yaml#
> +
> +    properties:
> +      reg:
> +        description: Index of the LED.
> +        minimum: 0
> +        maximum: 23
> +
> +    description:
> +      Specifies a single LED at the specified index
> +
> +

Just one line. Plus errors pointed out by Rob's bot.

> +required:
> +  - compatible
> +  - reg
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +   #include <dt-bindings/gpio/gpio.h>
> +   #include <dt-bindings/leds/common.h>
> +
> +   i2c {
> +       #address-cells = <1>;
> +       #size-cells = <0>;
> +
> +        led-controller@30 {
> +            compatible = "awinic,aw21024";
> +            #address-cells = <1>;
> +            #size-cells = <0>;
> +            reg = <0x30>;

reg after compatible.

> +            enable-gpios = <&gpio1 23>;
> +
> +            multi-led@1 {
> +                #address-cells = <1>;
> +                #size-cells = <2>;
> +                reg = <0x0 0x1 0x2>;

This is confusing. Does not match unit address and address/size cells.
Perhaps you wanted three separate regs?


> +                color = <LED_COLOR_ID_RGB>;
> +                label = "RGB_LED1";
> +
> +                led-0 {
> +                    color = <LED_COLOR_ID_RED>;
> +                };
> +
> +                led-1 {
> +                    color = <LED_COLOR_ID_GREEN>;
> +                };
> +
> +                led-2 {
> +                    color = <LED_COLOR_ID_BLUE>;
> +                };
> +
> +            };
> +            multi-led@2 {
> +                #address-cells = <1>;
> +                #size-cells = <3>;
> +                reg = <0x3 0x4 0x5 0x6>;

The same

> +                color = <LED_COLOR_ID_RGB>;
> +                label = "RGBW_LED1";

Why labels are upper-case?

> +
> +                led-4 {
> +                    color = <LED_COLOR_ID_RED>;
> +                };
> +
> +                led-5 {
> +                    color = <LED_COLOR_ID_GREEN>;
> +                };
> +
> +                led-6 {
> +                    color = <LED_COLOR_ID_BLUE>;
> +                };
> +
> +                led-7 {
> +                    color = <LED_COLOR_ID_WHITE>;
> +                };
> +            };
> +            ready_led@3 {

No underscores in node names. Generic node name, so just led.

> +                #address-cells = <1>;
> +                #size-cells = <1>;
> +                reg = <0x7 0x8>;

The same problem with reg.

> +                label = "READY";
> +                color = <LED_COLOR_ID_MULTI>;
> +
> +                led-8 {
> +                  color = <LED_COLOR_ID_RED>;
> +                };
> +
> +                led-9 {
> +                  color = <LED_COLOR_ID_GREEN>;
> +                };
> +            };
> +            connected_led@4 {
> +                reg = <0x9>;
> +                label = "CONNECTED";
> +                color = <LED_COLOR_ID_BLUE>;
> +            };
> +        };
> +    };
> +
> +...


Best regards,
Krzysztof

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

* Re: [PATCH 1/2] leds: aw21024: Add support for Awinic's AW21024
  2022-05-13 19:04 [PATCH 1/2] leds: aw21024: Add support for Awinic's AW21024 Kyle Swenson
                   ` (3 preceding siblings ...)
  2022-05-16 12:34 ` kernel test robot
@ 2022-05-17  9:11 ` Krzysztof Kozlowski
  2022-05-17 18:36   ` Kyle Swenson
  4 siblings, 1 reply; 14+ messages in thread
From: Krzysztof Kozlowski @ 2022-05-17  9:11 UTC (permalink / raw)
  To: Kyle Swenson, pavel, robh+dt, krzk+dt; +Cc: linux-leds, devicetree

On 13/05/2022 21:04, Kyle Swenson wrote:
> The Awinic AW21024 LED controller is a 24-channel RGB LED controller.
> Each LED on the controller can be controlled individually or grouped
> with other LEDs on the controller to form a multi-color LED.  Arbitrary
> combinations of individual and grouped LED control should be possible.
> 
> Signed-off-by: Kyle Swenson <kyle.swenson@est.tech>

Thank you for your patch. There is something to discuss/improve.

> +
> +static const struct i2c_device_id aw21024_id[] = {
> +	{ "aw21024", 0 }, /* 24 Channel */
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, aw21024_id);
> +
> +static const struct of_device_id of_aw21024_leds_match[] = {
> +	{ .compatible = "awinic,aw21024", },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, of_aw21024_leds_match);
> +
> +static struct i2c_driver aw21024_driver = {
> +	.driver		= {
> +		.name	= "aw21024",
> +		.of_match_table = of_match_ptr(of_aw21024_leds_match),

of_match_ptr causes this being unused. kbuild robot probably pointed
this out... if not - of_match_ptr goes with maybe_unused. You need both
or none, depending on intended usage.

> +	},
> +	.probe_new		= aw21024_probe,
> +	.remove		= aw21024_remove,
> +	.id_table = aw21024_id,

Why other places are indented but this not?


> +};
> +module_i2c_driver(aw21024_driver);
> +
> +MODULE_AUTHOR("Kyle Swenson <kyle.swenson@est.tech>");
> +MODULE_DESCRIPTION("Awinic AW21024 LED driver");
> +MODULE_LICENSE("GPL");


Best regards,
Krzysztof

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

* Re: [PATCH 2/2] dt-bindings: leds: Add aw21024 binding
  2022-05-17  9:08   ` Krzysztof Kozlowski
@ 2022-05-17 18:31     ` Kyle Swenson
  2022-05-18  8:17       ` Krzysztof Kozlowski
  0 siblings, 1 reply; 14+ messages in thread
From: Kyle Swenson @ 2022-05-17 18:31 UTC (permalink / raw)
  To: krzysztof.kozlowski, kyle.swenson, pavel, robh+dt, krzk+dt
  Cc: linux-leds, devicetree

On Tue, May 17, 2022 at 11:08:08AM +0200, Krzysztof Kozlowski wrote:
> On 13/05/2022 21:04, Kyle Swenson wrote:
> > Add device-tree bindings for Awinic's aw21024 24 channel RGB LED Driver.
> > 
> > Datasheet:
> > https://www.awinic.com/Public/Uploads/uploadfile/files/20200511/20200511165751_5eb9138fcd9e3.PDF
> > 
> > Signed-off-by: Kyle Swenson <kyle.swenson@est.tech>
> > ---
> >  .../bindings/leds/leds-aw21024.yaml           | 157 ++++++++++++++++++
> >  1 file changed, 157 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/leds/leds-aw21024.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/leds/leds-aw21024.yaml b/Documentation/devicetree/bindings/leds/leds-aw21024.yaml
> > new file mode 100644
> > index 000000000000..1180c02b5d21
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/leds/leds-aw21024.yaml
> > @@ -0,0 +1,157 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/leds/leds-aw21024.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: AWINIC AW21024 24-channel LED Driver
> > +
> > +maintainers:
> > +  - Kyle Swenson <kyle.swenson@est.tech>
> > +
> > +description: |
> > +  The AW21024 is a 24-channel LED driver with an I2C interface.
> > +
> > +  For more product information please see the link below:
> > +  https://www.awinic.com/en/index/pageview/catid/19/id/28.html
> > +
> > +properties:
> > +  compatible:
> > +    const: awinic,aw21024
> > +
> > +  reg:
> > +    maxItems: 1
> > +    description:
> > +      I2C peripheral address
> 
> Skip description, it's obvious.
Okay.
> 
> > +
> > +  enable-gpios:
> > +    maxItems: 1
> > +    description: GPIO pin to enable/disable the device.
> 
> Skip description, it's obvious.
Sounds good, will do.
> 
> > +
> > +  '#address-cells':
> > +    const: 1
> > +
> > +  '#size-cells':
> > +    const: 0
> > +
> > +patternProperties:
> > +  '^multi-led@[0-9a-f]$':
> > +    type: object
> > +    $ref: leds-class-multicolor.yaml#
> > +    properties:
> > +      reg:
> > +        minItems: 1
> > +        maxItems: 24
> > +        description:
> > +          Denotes the LED indicies that should be grouped into a
> > +          single multi-color LED.
> > +
> > +    patternProperties:
> > +      "(^led-[0-9a-f]$|led)":
> 
> How does this pass your own bindings? In the DTS you use underscofer
> which is not here...
> 
> You need to test the bindings before sending them to people.
> 
So honestly, and you've probably guessed as much from this patch and
Rob's bot, that it doesn't pass the binding checks.  I learned about make
dt_binding_check shortly after Rob's bot pointed it out to me, and I
apologize for my ignorance wasting your time.
> > +        type: object
> > +        $ref: common.yaml#
> > +
> > +patternProperties:
> > +  "^led@[0-2]$":
> > +    type: object
> > +    $ref: common.yaml#
> > +
> > +    properties:
> > +      reg:
> > +        description: Index of the LED.
> > +        minimum: 0
> > +        maximum: 23
> > +
> > +    description:
> > +      Specifies a single LED at the specified index
> > +
> > +
> 
> Just one line. Plus errors pointed out by Rob's bot.
Yep, got it.
> > +required:
> > +  - compatible
> > +  - reg
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +   #include <dt-bindings/gpio/gpio.h>
> > +   #include <dt-bindings/leds/common.h>
> > +
> > +   i2c {
> > +       #address-cells = <1>;
> > +       #size-cells = <0>;
> > +
> > +        led-controller@30 {
> > +            compatible = "awinic,aw21024";
> > +            #address-cells = <1>;
> > +            #size-cells = <0>;
> > +            reg = <0x30>;
> 
> reg after compatible.
Okay.
> > +            enable-gpios = <&gpio1 23>;
> > +
> > +            multi-led@1 {
> > +                #address-cells = <1>;
> > +                #size-cells = <2>;
> > +                reg = <0x0 0x1 0x2>;
> 
> This is confusing. Does not match unit address and address/size cells.
> Perhaps you wanted three separate regs?
The wrong address and size cells and not matching the unit address is a
mistake on my part, and the next version will actually pass make
dt_binding_check.

That said, it's not clear to me how best to handle a combination of
multi-leds and individual LEDs on a particular board. For example, a
particular board with this driver might have the first six outputs
connected to two RGB LEDs, and then the remainder of the outputs
connected to individual LEDs.

My (poor) attempt at handling this resulted in this approach where I
(ab)used the 'reg' property to be able to address each individual LED of
a multi-led.  I'm sure this problem has been solved before, but I'm
struggling finding a driver in the tree that has solved it.

Any advice or pointers will be welcome, and in the mean time I'll plan
on fixing the (now obvious) issues with the binding.  At the very least,
cleaning up the binding will make the problem I'm trying to solve more
clear.

> > +                color = <LED_COLOR_ID_RGB>;
> > +                label = "RGB_LED1";
> > +
> > +                led-0 {
> > +                    color = <LED_COLOR_ID_RED>;
> > +                };
> > +
> > +                led-1 {
> > +                    color = <LED_COLOR_ID_GREEN>;
> > +                };
> > +
> > +                led-2 {
> > +                    color = <LED_COLOR_ID_BLUE>;
> > +                };
> > +
> > +            };
> > +            multi-led@2 {
> > +                #address-cells = <1>;
> > +                #size-cells = <3>;
> > +                reg = <0x3 0x4 0x5 0x6>;
> 
> The same
Yep, will fix
> 
> > +                color = <LED_COLOR_ID_RGB>;
> > +                label = "RGBW_LED1";
> 
> Why labels are upper-case?
No reason, they won't be in the next version, sorry.
> 
> > +
> > +                led-4 {
> > +                    color = <LED_COLOR_ID_RED>;
> > +                };
> > +
> > +                led-5 {
> > +                    color = <LED_COLOR_ID_GREEN>;
> > +                };
> > +
> > +                led-6 {
> > +                    color = <LED_COLOR_ID_BLUE>;
> > +                };
> > +
> > +                led-7 {
> > +                    color = <LED_COLOR_ID_WHITE>;
> > +                };
> > +            };
> > +            ready_led@3 {
> 
> No underscores in node names. Generic node name, so just led.
Understood, I'll fix this and all the other node names.
> 
> > +                #address-cells = <1>;
> > +                #size-cells = <1>;
> > +                reg = <0x7 0x8>;
> 
> The same problem with reg.
Yep, will fix.
> 
> > +                label = "READY";
> > +                color = <LED_COLOR_ID_MULTI>;
> > +
> > +                led-8 {
> > +                  color = <LED_COLOR_ID_RED>;
> > +                };
> > +
> > +                led-9 {
> > +                  color = <LED_COLOR_ID_GREEN>;
> > +                };
> > +            };
> > +            connected_led@4 {
> > +                reg = <0x9>;
> > +                label = "CONNECTED";
> > +                color = <LED_COLOR_ID_BLUE>;
> > +            };
> > +        };
> > +    };
> > +
> > +...
> 
> 
> Best regards,
> Krzysztof

Thanks so much for even looking at this, despite it obviously not being
tested- that won't happen again.

Thanks,
Kyle

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

* Re: [PATCH 1/2] leds: aw21024: Add support for Awinic's AW21024
  2022-05-17  9:11 ` Krzysztof Kozlowski
@ 2022-05-17 18:36   ` Kyle Swenson
  2022-05-18  8:30     ` Krzysztof Kozlowski
  0 siblings, 1 reply; 14+ messages in thread
From: Kyle Swenson @ 2022-05-17 18:36 UTC (permalink / raw)
  To: Krzysztof Kozlowski, kyle.swenson, pavel, robh+dt, krzk+dt
  Cc: linux-leds, devicetree

On Tue, May 17, 2022 at 11:11:37AM +0200, Krzysztof Kozlowski wrote:
> On 13/05/2022 21:04, Kyle Swenson wrote:
> > The Awinic AW21024 LED controller is a 24-channel RGB LED controller.
> > Each LED on the controller can be controlled individually or grouped
> > with other LEDs on the controller to form a multi-color LED.  Arbitrary
> > combinations of individual and grouped LED control should be possible.
> > 
> > Signed-off-by: Kyle Swenson <kyle.swenson@est.tech>
> 
> Thank you for your patch. There is something to discuss/improve.
> 
> > +
> > +static const struct i2c_device_id aw21024_id[] = {
> > +	{ "aw21024", 0 }, /* 24 Channel */
> > +	{ }
> > +};
> > +MODULE_DEVICE_TABLE(i2c, aw21024_id);
> > +
> > +static const struct of_device_id of_aw21024_leds_match[] = {
> > +	{ .compatible = "awinic,aw21024", },
> > +	{},
> > +};
> > +MODULE_DEVICE_TABLE(of, of_aw21024_leds_match);
> > +
> > +static struct i2c_driver aw21024_driver = {
> > +	.driver		= {
> > +		.name	= "aw21024",
> > +		.of_match_table = of_match_ptr(of_aw21024_leds_match),
> 
> of_match_ptr causes this being unused. kbuild robot probably pointed
> this out... if not - of_match_ptr goes with maybe_unused. You need both
> or none, depending on intended usage.
> 
Ah, yes, the kbuild robot did point this out to me, and I had planned on
fixing by adding 'depends on OF' to the Kconfig.  Perhaps that isn't
correct or complete (or even relevant)?

I'll do some investigating and determine if I need to use of_match_ptr
or not and I'll fix it either by removing it or adding maybe_unused in
the next version.

> > +	},
> > +	.probe_new		= aw21024_probe,
> > +	.remove		= aw21024_remove,
> > +	.id_table = aw21024_id,
> 
> Why other places are indented but this not?
Sorry, it should be.  My editor was configured wrong and this now looks
bad.  There are a few other places in the driver that also now look bad
and I'll fix those before submitting v2.
> 
> 
> > +};
> > +module_i2c_driver(aw21024_driver);
> > +
> > +MODULE_AUTHOR("Kyle Swenson <kyle.swenson@est.tech>");
> > +MODULE_DESCRIPTION("Awinic AW21024 LED driver");
> > +MODULE_LICENSE("GPL");
> 
> 
> Best regards,
> Krzysztof

Thanks so much for taking a look at this, I really appreciate your time
and patience.

Thanks,
Kyle

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

* Re: [PATCH 2/2] dt-bindings: leds: Add aw21024 binding
  2022-05-17 18:31     ` Kyle Swenson
@ 2022-05-18  8:17       ` Krzysztof Kozlowski
  2022-05-18 21:18         ` Kyle Swenson
  0 siblings, 1 reply; 14+ messages in thread
From: Krzysztof Kozlowski @ 2022-05-18  8:17 UTC (permalink / raw)
  To: Kyle Swenson, pavel, robh+dt, krzk+dt; +Cc: linux-leds, devicetree

On 17/05/2022 20:31, Kyle Swenson wrote:
>>> +
>>> +            multi-led@1 {
>>> +                #address-cells = <1>;
>>> +                #size-cells = <2>;
>>> +                reg = <0x0 0x1 0x2>;
>>
>> This is confusing. Does not match unit address and address/size cells.
>> Perhaps you wanted three separate regs?
> The wrong address and size cells and not matching the unit address is a
> mistake on my part, and the next version will actually pass make
> dt_binding_check.
> 
> That said, it's not clear to me how best to handle a combination of
> multi-leds and individual LEDs on a particular board. For example, a
> particular board with this driver might have the first six outputs
> connected to two RGB LEDs, and then the remainder of the outputs
> connected to individual LEDs.
> 
> My (poor) attempt at handling this resulted in this approach where I
> (ab)used the 'reg' property to be able to address each individual LED of
> a multi-led.  I'm sure this problem has been solved before, but I'm
> struggling finding a driver in the tree that has solved it.
> 
> Any advice or pointers will be welcome, and in the mean time I'll plan
> on fixing the (now obvious) issues with the binding.  At the very least,
> cleaning up the binding will make the problem I'm trying to solve more
> clear.

The immediate solution to the DTS reg issue is to use the same unit
address, so:

multi-led@0 {
	reg = <0x0>, <0x1>, <0x2>;
}

However your case is partially (or entirely) covered by multicolor LEDs.
You should add allOf:$ref with reference to leds-class-multicolor.yaml.
I see exactly your pattern being used there - just the fixed one, I
think. I'll send a patch for it and put you on Cc.

Best regards,
Krzysztof

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

* Re: [PATCH 1/2] leds: aw21024: Add support for Awinic's AW21024
  2022-05-17 18:36   ` Kyle Swenson
@ 2022-05-18  8:30     ` Krzysztof Kozlowski
  0 siblings, 0 replies; 14+ messages in thread
From: Krzysztof Kozlowski @ 2022-05-18  8:30 UTC (permalink / raw)
  To: Kyle Swenson, pavel, robh+dt, krzk+dt; +Cc: linux-leds, devicetree

On 17/05/2022 20:36, Kyle Swenson wrote:
>>> +static const struct of_device_id of_aw21024_leds_match[] = {
>>> +	{ .compatible = "awinic,aw21024", },
>>> +	{},
>>> +};
>>> +MODULE_DEVICE_TABLE(of, of_aw21024_leds_match);
>>> +
>>> +static struct i2c_driver aw21024_driver = {
>>> +	.driver		= {
>>> +		.name	= "aw21024",
>>> +		.of_match_table = of_match_ptr(of_aw21024_leds_match),
>>
>> of_match_ptr causes this being unused. kbuild robot probably pointed
>> this out... if not - of_match_ptr goes with maybe_unused. You need both
>> or none, depending on intended usage.
>>
> Ah, yes, the kbuild robot did point this out to me, and I had planned on
> fixing by adding 'depends on OF' to the Kconfig.  Perhaps that isn't
> correct or complete (or even relevant)?
> 
> I'll do some investigating and determine if I need to use of_match_ptr
> or not and I'll fix it either by removing it or adding maybe_unused in
> the next version.

Your has i2c_device_id so it could bind without OF, however obviously
aw21024_probe_dt() will do nothing and return 0.

Therefore it is up to you if you want to add dependency on OF. If you
add, please add it with "|| COMPILE_TEST".

Then in both cases you need to handle the case of building (not running)
a driver without OF: using maybe_unused+of_match_ptr() or nothing (thus
always referencing of_device_id). Which one to choose matters less.
Using it causes the code to be smaller for !OF case, which might matter
for some distros which build everything as module. Not using it allows
to match the driver on ACPI systems, although I am not sure if this is
relevant.

I don't have recommendation on that - just be sure there are no warnings.


Best regards,
Krzysztof

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

* Re: [PATCH 2/2] dt-bindings: leds: Add aw21024 binding
  2022-05-18  8:17       ` Krzysztof Kozlowski
@ 2022-05-18 21:18         ` Kyle Swenson
  2022-05-19  8:04           ` Krzysztof Kozlowski
  0 siblings, 1 reply; 14+ messages in thread
From: Kyle Swenson @ 2022-05-18 21:18 UTC (permalink / raw)
  To: Krzysztof Kozlowski; +Cc: pavel, robh+dt, krzk+dt, linux-leds, devicetree

On Wed, May 18, 2022 at 10:17:21AM +0200, Krzysztof Kozlowski wrote:
> On 17/05/2022 20:31, Kyle Swenson wrote:
> >>> +
> >>> +            multi-led@1 {
> >>> +                #address-cells = <1>;
> >>> +                #size-cells = <2>;
> >>> +                reg = <0x0 0x1 0x2>;
> >>
> >> This is confusing. Does not match unit address and address/size cells.
> >> Perhaps you wanted three separate regs?
> > The wrong address and size cells and not matching the unit address is a
> > mistake on my part, and the next version will actually pass make
> > dt_binding_check.
> > 
> > That said, it's not clear to me how best to handle a combination of
> > multi-leds and individual LEDs on a particular board. For example, a
> > particular board with this driver might have the first six outputs
> > connected to two RGB LEDs, and then the remainder of the outputs
> > connected to individual LEDs.
> > 
> > My (poor) attempt at handling this resulted in this approach where I
> > (ab)used the 'reg' property to be able to address each individual LED of
> > a multi-led.  I'm sure this problem has been solved before, but I'm
> > struggling finding a driver in the tree that has solved it.
> > 
> > Any advice or pointers will be welcome, and in the mean time I'll plan
> > on fixing the (now obvious) issues with the binding.  At the very least,
> > cleaning up the binding will make the problem I'm trying to solve more
> > clear.
> 
> The immediate solution to the DTS reg issue is to use the same unit
> address, so:
> 
> multi-led@0 {
> 	reg = <0x0>, <0x1>, <0x2>;
> }
> 
> However your case is partially (or entirely) covered by multicolor LEDs.
> You should add allOf:$ref with reference to leds-class-multicolor.yaml.
> I see exactly your pattern being used there - just the fixed one, I
> think. I'll send a patch for it and put you on Cc.

I suspect you're right: mutlicolor LEDs will do exactly what I need and
the patch you cc'd me on teaches me how to specify it in the DTS.  I'll
make the changes and send up a v2 in a few days.

> 
> Best regards,
> Krzysztof

Thanks again for your time and guidance.  I happen to have a board with that
lp50xx LED controller and I'll be happy to test out the example DTS from the
binding if you'd like.

Thanks,
Kyle

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

* Re: [PATCH 2/2] dt-bindings: leds: Add aw21024 binding
  2022-05-18 21:18         ` Kyle Swenson
@ 2022-05-19  8:04           ` Krzysztof Kozlowski
  0 siblings, 0 replies; 14+ messages in thread
From: Krzysztof Kozlowski @ 2022-05-19  8:04 UTC (permalink / raw)
  To: Kyle Swenson; +Cc: pavel, robh+dt, krzk+dt, linux-leds, devicetree

On 18/05/2022 23:18, Kyle Swenson wrote:
> Thanks again for your time and guidance.  I happen to have a board with that
> lp50xx LED controller and I'll be happy to test out the example DTS from the
> binding if you'd like.

Sure, that would be good. The DTS using that lp50xx LED follows
different concept, so maybe the binding example is not correct.


Best regards,
Krzysztof

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

end of thread, other threads:[~2022-05-19  8:04 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-13 19:04 [PATCH 1/2] leds: aw21024: Add support for Awinic's AW21024 Kyle Swenson
2022-05-13 19:04 ` [PATCH 2/2] dt-bindings: leds: Add aw21024 binding Kyle Swenson
2022-05-13 20:48   ` Rob Herring
2022-05-17  9:08   ` Krzysztof Kozlowski
2022-05-17 18:31     ` Kyle Swenson
2022-05-18  8:17       ` Krzysztof Kozlowski
2022-05-18 21:18         ` Kyle Swenson
2022-05-19  8:04           ` Krzysztof Kozlowski
2022-05-16 10:21 ` [PATCH 1/2] leds: aw21024: Add support for Awinic's AW21024 kernel test robot
2022-05-16 10:42 ` kernel test robot
2022-05-16 12:34 ` kernel test robot
2022-05-17  9:11 ` Krzysztof Kozlowski
2022-05-17 18:36   ` Kyle Swenson
2022-05-18  8:30     ` Krzysztof Kozlowski

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.