* [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.