[PATCH v2 0/2] leds: add Awinic AW2026 driver

[PATCH v2 0/2] leds: add Awinic AW2026 driver

[Vladimir Barinov]

Hello,

This adds the folowing:
- LED driver for AW2026 chip
- Document Awinic AW2026 DT bindings

Datasheet file can be found here:
https://www.awinic.com/en/productDetail/AW2026DNR#tech-docs

Vladimir Barinov (2):
[1/2] leds: Add Awinic AW2026 LED driver
[2/2] dt-bindings: leds: Document Awinic AW2026 bindings

---
This patchset is against the 'kernel/git/lee/leds.git' repo, 'for-leds-next' branch.

 Documentation/devicetree/bindings/leds/awinic,aw2026.yaml |   93 ++
 drivers/leds/Kconfig                                      |   10 
 drivers/leds/Makefile                                     |    1 
 drivers/leds/leds-aw2026.c                                |  578 ++++++++++++++
 4 files changed, 682 insertions(+)

[PATCH v2 1/2] leds: add Awinic AW2026 LED driver

[Vladimir Barinov]

This adds support for Awinic AW2026 3-channel LED driver with
I2C insterface. It supports hardware blinking and hardware
pattern generator.

Signed-off-by: Vladimir Barinov <v.barinov@yadro.com>
---
Changes in version 2:
- fixed typos in patch header 2016 -> 2026

 drivers/leds/Kconfig       |  10 +
 drivers/leds/Makefile      |   1 +
 drivers/leds/leds-aw2026.c | 578 +++++++++++++++++++++++++++++++++++++
 3 files changed, 589 insertions(+)
 create mode 100644 drivers/leds/leds-aw2026.c

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index aaa9140bc351..574f3cc47d3e 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -104,6 +104,16 @@ config LEDS_AW2013
 	  To compile this driver as a module, choose M here: the module
 	  will be called leds-aw2013.
 
+config LEDS_AW2026
+	tristate "LED support for Awinic AW2026"
+	depends on LEDS_CLASS && I2C && OF
+	help
+	  This option enables support for the AW2026 3-channel
+	  LED driver.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called leds-aw2026.
+
 config LEDS_BCM6328
 	tristate "LED Support for Broadcom BCM6328"
 	depends on LEDS_CLASS
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index d30395d11fd8..7fb7b48329ff 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -15,6 +15,7 @@ obj-$(CONFIG_LEDS_AN30259A)		+= leds-an30259a.o
 obj-$(CONFIG_LEDS_APU)			+= leds-apu.o
 obj-$(CONFIG_LEDS_ARIEL)		+= leds-ariel.o
 obj-$(CONFIG_LEDS_AW2013)		+= leds-aw2013.o
+obj-$(CONFIG_LEDS_AW2026)		+= leds-aw2026.o
 obj-$(CONFIG_LEDS_BCM6328)		+= leds-bcm6328.o
 obj-$(CONFIG_LEDS_BCM6358)		+= leds-bcm6358.o
 obj-$(CONFIG_LEDS_BD2802)		+= leds-bd2802.o
diff --git a/drivers/leds/leds-aw2026.c b/drivers/leds/leds-aw2026.c
new file mode 100644
index 000000000000..7c2d5f62797c
--- /dev/null
+++ b/drivers/leds/leds-aw2026.c
@@ -0,0 +1,578 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Awinic AW2026 3-channel LED driver
+ *
+ * Author: Vladimir Barinov <v.barinov@yadro.com>
+ * Copyright (C) 2023 KNS Group LLC (YADRO)
+ */
+
+#include <linux/i2c.h>
+#include <linux/leds.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/of.h>
+#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
+
+#define AW2026_MAX_LEDS		3
+
+/* Chip ID and Software Reset Register */
+#define AW2026_RSTIDR		0x00
+#define AW2026_RSTIDR_RESET	0x55
+#define AW2026_RSTIDR_CHIP_ID	0x31
+
+/* Global Control Register */
+#define AW2026_GCR		0x01
+#define AW2026_GCR_CHIPEN	BIT(0)
+
+/* LED Maximum Current Register */
+#define AW2026_IMAX		0x03
+#define AW2026_IMAX_MASK	(BIT(0) | BIT(1))
+
+/* LED Configure Register */
+#define AW2026_LCFG(x)		(0x04 + (x))
+#define AW2026_LCFG_LEDMD	BIT(0)
+#define AW2026_LCFG_FADE_IN	BIT(1)
+#define AW2026_LCFG_FADE_OUT	BIT(2)
+
+/* LED Channel Enable Register */
+#define AW2026_LEDEN		0x07
+
+/* Pattern Run/Stop Register */
+#define AW2026_PATRUN		0x09
+
+/* LED Current Register */
+#define AW2026_ILED(x)		(0x10 + (x))
+#define AW2026_ILED_MAX		0xFF
+
+/* PWM duty level Register */
+#define AW2026_PWM(x)		(0x1C + (x))
+#define AW2026_PWM_DUTY_MAX	0xFF
+
+/* T1 Time Parameter of Pattern */
+#define AW2026_PAT_T1(x)	(0x30 + 5*(x))
+
+/* T2 Time Parameter of Pattern */
+#define AW2026_PAT_T2(x)	(0x31 + 5*(x))
+
+struct aw2026;
+
+enum aw2026_state {
+	AW2026_STATE_OFF,
+	AW2026_STATE_ON,
+	AW2026_STATE_KEEP,
+};
+
+struct aw2026_led {
+	struct aw2026 *chip;
+	struct fwnode_handle *fwnode;
+	struct led_classdev cdev;
+	enum aw2026_state default_state;
+	u32 idx;
+};
+
+struct aw2026 {
+	struct i2c_client *client;
+	struct regmap *regmap;
+	struct mutex lock;
+	struct regulator *vcc_regulator;
+	struct aw2026_led leds[AW2026_MAX_LEDS];
+	int num_leds;
+	unsigned int imax;
+	bool enabled;
+};
+
+static const struct regmap_config aw2026_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.max_register = 0x3e,
+};
+
+struct msec_reg {
+	u32 msec;
+	u8 reg;
+};
+
+static const struct msec_reg aw2026_msec_reg[] = {
+	{ .msec =    4, .reg = 0x0 },
+	{ .msec =  130, .reg = 0x1 },
+	{ .msec =  260, .reg = 0x2 },
+	{ .msec =  380, .reg = 0x3 },
+	{ .msec =  510, .reg = 0x4 },
+	{ .msec =  770, .reg = 0x5 },
+	{ .msec = 1040, .reg = 0x6 },
+	{ .msec = 1600, .reg = 0x7 },
+	{ .msec = 2100, .reg = 0x8 },
+	{ .msec = 2600, .reg = 0x9 },
+	{ .msec = 3100, .reg = 0xa },
+	{ .msec = 4200, .reg = 0xb },
+	{ .msec = 5200, .reg = 0xc },
+	{ .msec = 6200, .reg = 0xd },
+	{ .msec = 7300, .reg = 0xe },
+	{ .msec = 8300, .reg = 0xf },
+	{ /* sentinel */ },
+};
+
+static int aw2026_chip_init(struct aw2026 *chip)
+{
+	int idx, ret;
+
+	ret = regmap_update_bits(chip->regmap, AW2026_GCR, AW2026_GCR_CHIPEN,
+				 AW2026_GCR_CHIPEN);
+	if (ret)
+		return ret;
+
+	/* Max current */
+	ret = regmap_update_bits(chip->regmap, AW2026_IMAX,
+				 AW2026_IMAX_MASK, chip->imax);
+	if (ret)
+		return ret;
+
+	for (idx = 0; idx < chip->num_leds; idx++) {
+		/* PWM level */
+		ret = regmap_write(chip->regmap, AW2026_PWM(idx), AW2026_PWM_DUTY_MAX);
+		if (ret)
+			return ret;
+	}
+
+	return ret;
+}
+
+static void aw2026_chip_disable(struct aw2026 *chip)
+{
+	int ret;
+
+	if (!chip->enabled)
+		return;
+
+	regmap_update_bits(chip->regmap, AW2026_GCR, AW2026_GCR_CHIPEN, 0);
+
+	ret = regulator_disable(chip->vcc_regulator);
+	if (ret) {
+		dev_err(&chip->client->dev,
+			"Failed to disable regulator: %d\n", ret);
+		return;
+	}
+
+	chip->enabled = false;
+}
+
+static int aw2026_chip_enable(struct aw2026 *chip)
+{
+	int ret;
+
+	if (chip->enabled)
+		return 0;
+
+	ret = regulator_enable(chip->vcc_regulator);
+	if (ret) {
+		dev_err(&chip->client->dev,
+			"Failed to enable regulator: %d\n", ret);
+		return ret;
+	}
+	chip->enabled = true;
+
+	ret = aw2026_chip_init(chip);
+	if (ret)
+		aw2026_chip_disable(chip);
+
+	return ret;
+}
+
+static bool aw2026_chip_in_use(struct aw2026 *chip)
+{
+	int i;
+
+	for (i = 0; i < chip->num_leds; i++)
+		if (chip->leds[i].cdev.brightness)
+			return true;
+
+	return false;
+}
+
+static int aw2026_brightness_set(struct led_classdev *cdev,
+				 enum led_brightness brightness)
+{
+	struct aw2026_led *led = container_of(cdev, struct aw2026_led, cdev);
+	int ret, idx = led->idx;
+
+	mutex_lock(&led->chip->lock);
+
+	if (aw2026_chip_in_use(led->chip)) {
+		ret = aw2026_chip_enable(led->chip);
+		if (ret)
+			goto error;
+	}
+
+	if (brightness) {
+		/* Manual mode */
+		ret = regmap_update_bits(led->chip->regmap, AW2026_LCFG(idx),
+					 AW2026_LCFG_LEDMD, 0);
+		if (ret)
+			goto error;
+		/* Current configure */
+		ret = regmap_write(led->chip->regmap, AW2026_ILED(idx), brightness);
+		if (ret)
+			goto error;
+		/* Enable LED */
+		ret = regmap_update_bits(led->chip->regmap, AW2026_LEDEN, BIT(idx), 0xFF);
+	} else {
+		/* Disable LED */
+		ret = regmap_update_bits(led->chip->regmap, AW2026_LEDEN, BIT(idx), 0);
+	}
+	if (ret)
+		goto error;
+
+	if (!aw2026_chip_in_use(led->chip))
+		aw2026_chip_disable(led->chip);
+
+error:
+	mutex_unlock(&led->chip->lock);
+
+	return ret;
+}
+
+static int aw2026_convert_msec_to_reg(struct aw2026 *chip, u32 *msec, u8 *reg)
+{
+	const struct msec_reg *value;
+	const struct msec_reg *prev_value = NULL;
+
+	for (value = aw2026_msec_reg; value->msec; value++) {
+		if (value->msec >= *msec)
+			break;
+		prev_value = value;
+	}
+
+	if (!value->msec) {
+		dev_err(&chip->client->dev, "Unsupported msec (%u)", *msec);
+		return -ERANGE;
+	}
+
+	if (prev_value && ((*msec - prev_value->msec) <= (value->msec - *msec)))
+		value = prev_value;
+
+	*reg = value->reg;
+	*msec = value->msec;
+
+	return 0;
+}
+
+static int aw2026_pattern_setup(struct aw2026_led *led, u8 trise, u8 ton, u8 tfall, u8 toff)
+{
+	int ret, idx = led->idx;
+
+	mutex_lock(&led->chip->lock);
+
+	if (aw2026_chip_in_use(led->chip)) {
+		ret = aw2026_chip_enable(led->chip);
+		if (ret)
+			goto error;
+	}
+
+	/* Pattern mode */
+	ret = regmap_update_bits(led->chip->regmap, AW2026_LCFG(idx),
+				 AW2026_LCFG_LEDMD, 0xFF);
+	if (ret)
+		goto error;
+	/* Current configure */
+	ret = regmap_write(led->chip->regmap, AW2026_ILED(idx), led->cdev.brightness);
+	if (ret)
+		goto error;
+	/* Rise and On time of Pattern */
+	ret = regmap_write(led->chip->regmap, AW2026_PAT_T1(idx), (trise << 4) | ton);
+	if (ret)
+		goto error;
+	/* Fall and Off time of Pattern */
+	ret = regmap_write(led->chip->regmap, AW2026_PAT_T2(idx), (tfall << 4) | toff);
+	if (ret)
+		goto error;
+	/* Pattern run for individual LED */
+	ret = regmap_update_bits(led->chip->regmap, AW2026_PATRUN, BIT(idx), 0xFF);
+	if (ret)
+		goto error;
+	/* Enable LED */
+	ret = regmap_update_bits(led->chip->regmap, AW2026_LEDEN, BIT(idx), 0xFF);
+
+error:
+	mutex_unlock(&led->chip->lock);
+
+	return ret;
+}
+
+static int aw2026_pattern_set(struct led_classdev *cdev,
+			      struct led_pattern *pattern,
+			      u32 len, int repeat)
+{
+	struct aw2026_led *led = container_of(cdev, struct aw2026_led, cdev);
+	struct aw2026 *chip = led->chip;
+	int ret;
+	u8 trise, ton, tfall, toff;
+
+	if (len == 1) {
+		led->cdev.brightness = pattern[0].brightness;
+		return aw2026_brightness_set(cdev, led->cdev.brightness);
+	}
+
+	if (repeat > 0 || len != 4)
+		return -EINVAL;
+
+	ret = aw2026_convert_msec_to_reg(chip, &pattern[0].delta_t, &trise);
+	if (ret)
+		return ret;
+	ret = aw2026_convert_msec_to_reg(chip, &pattern[1].delta_t, &ton);
+	if (ret)
+		return ret;
+	ret = aw2026_convert_msec_to_reg(chip, &pattern[2].delta_t, &tfall);
+	if (ret)
+		return ret;
+	ret = aw2026_convert_msec_to_reg(chip, &pattern[3].delta_t, &toff);
+	if (ret)
+		return ret;
+
+	dev_dbg(&chip->client->dev, "pattern timings: %d %d %d %d\n",
+		trise, ton, tfall, toff);
+
+	led->cdev.brightness = max(max(pattern[0].brightness, pattern[1].brightness),
+				   max(pattern[2].brightness, pattern[3].brightness));
+
+	return aw2026_pattern_setup(led, trise, ton, tfall, toff);
+}
+
+static int aw2026_pattern_clear(struct led_classdev *cdev)
+{
+	return aw2026_brightness_set(cdev, LED_OFF);
+}
+
+static int aw2026_blink_set(struct led_classdev *cdev,
+			    unsigned long *delay_on, unsigned long *delay_off)
+{
+	struct aw2026_led *led = container_of(cdev, struct aw2026_led, cdev);
+	struct aw2026 *chip = led->chip;
+	u8 ton, toff;
+	int ret;
+
+	if (!*delay_on) {
+		led->cdev.brightness = LED_OFF;
+		return aw2026_brightness_set(&led->cdev, LED_OFF);
+	}
+
+	led->cdev.brightness = LED_FULL;
+
+	if (!*delay_off)
+		return aw2026_brightness_set(&led->cdev, LED_FULL);
+
+	ret = aw2026_convert_msec_to_reg(chip, (u32 *)delay_on, &ton);
+	if (ret)
+		return ret;
+	ret = aw2026_convert_msec_to_reg(chip, (u32 *)delay_off, &toff);
+	if (ret)
+		return ret;
+
+	dev_dbg(&chip->client->dev, "blink timings: %d %d\n", ton, toff);
+
+	return aw2026_pattern_setup(led, 0, ton, 0, toff);
+}
+
+static int aw2026_parse_dt(struct i2c_client *client, struct aw2026 *chip)
+{
+	struct device_node *np = dev_of_node(&client->dev), *child;
+	int count, ret = 0, i = 0;
+	struct aw2026_led *led;
+	const char *str;
+	u32 imax;
+
+	count = of_get_available_child_count(np);
+	if (!count || count > AW2026_MAX_LEDS)
+		return -EINVAL;
+
+	if (!of_property_read_u32(np, "awinic,led-max-microamp", &imax)) {
+		chip->imax = min_t(u32, hweight32(imax / 3187 - 1), 3);
+	} else {
+		chip->imax = 1; /* 6.375mA */
+		dev_info(&client->dev,
+			 "DT property led-max-microamp is missing\n");
+	}
+
+	for_each_available_child_of_node(np, child) {
+		u32 source;
+
+		ret = of_property_read_u32(child, "reg", &source);
+		if (ret != 0 || source >= AW2026_MAX_LEDS) {
+			dev_err(&chip->client->dev,
+				"Couldn't read LED address: %d\n", ret);
+			count--;
+			continue;
+		}
+
+		led = &chip->leds[i];
+		led->idx = source;
+		led->chip = chip;
+		led->fwnode = of_fwnode_handle(child);
+
+		if (!of_property_read_string(child, "default-state", &str)) {
+			if (!strcmp(str, "on"))
+				led->default_state = AW2026_STATE_ON;
+			else if (!strcmp(str, "keep"))
+				led->default_state = AW2026_STATE_KEEP;
+			else
+				led->default_state = AW2026_STATE_OFF;
+		}
+
+		i++;
+	}
+
+	if (!count)
+		return -EINVAL;
+
+	chip->num_leds = i;
+
+	return 0;
+}
+
+static void aw2026_init_default_state(struct aw2026_led *led)
+{
+	struct aw2026 *chip = led->chip;
+	int led_on;
+	int ret, idx = led->idx;
+
+	switch (led->default_state) {
+	case AW2026_STATE_ON:
+		led->cdev.brightness = LED_FULL;
+		break;
+	case AW2026_STATE_KEEP:
+		/* keep setup made in loader */
+		ret = regmap_read(chip->regmap, AW2026_LEDEN, &led_on);
+		if (ret)
+			return;
+
+		if (!(led_on & BIT(idx))) {
+			led->cdev.brightness = LED_OFF;
+			break;
+		}
+		regmap_read(chip->regmap, AW2026_ILED(idx),
+			    &led->cdev.brightness);
+		return;
+	default:
+		led->cdev.brightness = LED_OFF;
+	}
+
+	aw2026_brightness_set(&led->cdev, led->cdev.brightness);
+}
+
+static int aw2026_probe(struct i2c_client *client)
+{
+	struct aw2026 *chip;
+	int i, ret;
+	unsigned int chipid;
+
+	chip = devm_kzalloc(&client->dev, sizeof(struct aw2026), GFP_KERNEL);
+	if (!chip)
+		return -ENOMEM;
+
+	ret = aw2026_parse_dt(client, chip);
+	if (ret < 0)
+		return ret;
+
+	mutex_init(&chip->lock);
+	chip->client = client;
+	i2c_set_clientdata(client, chip);
+
+	chip->regmap = devm_regmap_init_i2c(client, &aw2026_regmap_config);
+	if (IS_ERR(chip->regmap)) {
+		ret = PTR_ERR(chip->regmap);
+		dev_err(&client->dev, "Failed to allocate register map: %d\n",
+			ret);
+		goto error;
+	}
+
+	chip->vcc_regulator = devm_regulator_get(&client->dev, "vcc");
+	ret = PTR_ERR_OR_ZERO(chip->vcc_regulator);
+	if (ret) {
+		if (ret != -EPROBE_DEFER)
+			dev_err(&client->dev,
+				"Failed to request regulator: %d\n", ret);
+		goto error;
+	}
+
+	ret = regulator_enable(chip->vcc_regulator);
+	if (ret) {
+		dev_err(&client->dev,
+			"Failed to enable regulator: %d\n", ret);
+		goto error;
+	}
+
+	ret = regmap_read(chip->regmap, AW2026_RSTIDR, &chipid);
+	if (ret) {
+		dev_err(&client->dev, "Failed to read chip ID: %d\n", ret);
+		goto error_reg;
+	}
+
+	if (chipid != AW2026_RSTIDR_CHIP_ID) {
+		ret = -ENODEV;
+		goto error_reg;
+	}
+
+	for (i = 0; i < chip->num_leds; i++) {
+		struct led_init_data init_data = {};
+		struct aw2026_led *led = &chip->leds[i];
+
+		aw2026_init_default_state(&chip->leds[i]);
+		led->cdev.brightness_set_blocking = aw2026_brightness_set;
+		led->cdev.blink_set = aw2026_blink_set;
+		led->cdev.pattern_set = aw2026_pattern_set;
+		led->cdev.pattern_clear = aw2026_pattern_clear;
+
+		init_data.fwnode = chip->leds[i].fwnode;
+
+		ret = devm_led_classdev_register_ext(&client->dev,
+						     &led->cdev, &init_data);
+		if (ret < 0)
+			goto error_reg;
+	}
+
+	ret = regulator_disable(chip->vcc_regulator);
+	if (ret) {
+		dev_err(&client->dev,
+			"Failed to disable regulator: %d\n", ret);
+		goto error;
+	}
+
+	return 0;
+
+error_reg:
+	regulator_disable(chip->vcc_regulator);
+error:
+	mutex_destroy(&chip->lock);
+	return ret;
+}
+
+static void aw2026_remove(struct i2c_client *client)
+{
+	struct aw2026 *chip = i2c_get_clientdata(client);
+
+	aw2026_chip_disable(chip);
+
+	mutex_destroy(&chip->lock);
+}
+
+static const struct of_device_id aw2026_match_table[] = {
+	{ .compatible = "awinic,aw2026", },
+	{ /* sentinel */ },
+};
+
+MODULE_DEVICE_TABLE(of, aw2026_match_table);
+
+static struct i2c_driver aw2026_driver = {
+	.driver = {
+		.name = "leds-aw2026",
+		.of_match_table = of_match_ptr(aw2026_match_table),
+	},
+	.probe_new = aw2026_probe,
+	.remove = aw2026_remove,
+};
+
+module_i2c_driver(aw2026_driver);
+
+MODULE_DESCRIPTION("AW2026 LED driver");
+MODULE_AUTHOR("Vladimir Barinov");
+MODULE_LICENSE("GPL");
-- 
2.34.1

[PATCH v2 2/2] dt-bindings: leds: Document Awinic AW2026 bindings

[Vladimir Barinov]

Add Awinic AW2026 binding documentation

Signed-off-by: Vladimir Barinov <v.barinov@yadro.com>
---
Changes in version 2:
- fixed typos in patch header 2016 -> 2026
- fixed typo in example section that break dt_binding_check

 .../bindings/leds/awinic,aw2026.yaml          | 92 +++++++++++++++++++
 1 file changed, 92 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/leds/awinic,aw2026.yaml

diff --git a/Documentation/devicetree/bindings/leds/awinic,aw2026.yaml b/Documentation/devicetree/bindings/leds/awinic,aw2026.yaml
new file mode 100664
index 000000000000..abacf746677b
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/awinic,aw2026.yaml
@@ -0,0 +1,93 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/leds/awinic,aw2026.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Awinic AW2026 3-channel LED Driver
+
+maintainers:
+  - Vladimir Barinov <v.barinov@yadro.com>
+
+description: |
+  The AW2026 is a 3-channel LED driver with I2C interface. It can control
+  LED brightness with PWM output. It supports hardware blinking and
+  hardware patterns.
+
+properties:
+  compatible:
+    const: awinic,aw2026
+
+  reg:
+    maxItems: 1
+
+  "#address-cells":
+    const: 1
+
+  "#size-cells":
+    const: 0
+
+  awinic,led-max-microamp:
+    description:
+      Maximum current at LED output
+    enum:
+      [3000, 6375, 12750, 25500]
+
+  vcc-supply:
+    description: Regulator providing power to the "VBAT" pin.
+
+patternProperties:
+  "^led@[0-2]$":
+    type: object
+    $ref: common.yaml#
+
+    properties:
+      reg:
+        description: Index of the LED.
+        minimum: 0
+        maximum: 2
+
+required:
+  - compatible
+  - reg
+  - "#address-cells"
+  - "#size-cells"
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/leds/common.h>
+
+    i2c0 {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        led-controller@64 {
+            compatible = "awinic,aw2026";
+            reg = <0x64>;
+            #address-cells = <1>;
+            #size-cells = <0>;
+            awinic,led-max-microamp = <6375>;
+            vcc-supply = <&vcc_3v3_s0>;
+
+            led@0 {
+                    reg = <0>;
+                    function = LED_FUNCTION_INDICATOR;
+                    color = <LED_COLOR_ID_RED>;
+            };
+
+            led@1 {
+                    reg = <1>;
+                    function = LED_FUNCTION_INDICATOR;
+                    color = <LED_COLOR_ID_BLUE>;
+            };
+
+            led@2 {
+                    reg = <2>;
+                    function = LED_FUNCTION_INDICATOR;
+                    color = <LED_COLOR_ID_GREEN>;
+            };
+        };
+    };
+...
-- 
2.34.1

Re: [PATCH v2 1/2] leds: add Awinic AW2026 LED driver

[Lee Jones]

On Thu, 25 May 2023, Vladimir Barinov wrote:

> This adds support for Awinic AW2026 3-channel LED driver with
> I2C insterface. It supports hardware blinking and hardware
> pattern generator.
> 
> Signed-off-by: Vladimir Barinov <v.barinov@yadro.com>
> ---
> Changes in version 2:
> - fixed typos in patch header 2016 -> 2026
> 
>  drivers/leds/Kconfig       |  10 +
>  drivers/leds/Makefile      |   1 +
>  drivers/leds/leds-aw2026.c | 578 +++++++++++++++++++++++++++++++++++++
>  3 files changed, 589 insertions(+)
>  create mode 100644 drivers/leds/leds-aw2026.c
> 
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index aaa9140bc351..574f3cc47d3e 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -104,6 +104,16 @@ config LEDS_AW2013
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called leds-aw2013.
>  
> +config LEDS_AW2026
> +	tristate "LED support for Awinic AW2026"
> +	depends on LEDS_CLASS && I2C && OF
> +	help
> +	  This option enables support for the AW2026 3-channel
> +	  LED driver.
> +
> +	  To compile this driver as a module, choose M here: the module
> +	  will be called leds-aw2026.
> +
>  config LEDS_BCM6328
>  	tristate "LED Support for Broadcom BCM6328"
>  	depends on LEDS_CLASS
> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> index d30395d11fd8..7fb7b48329ff 100644
> --- a/drivers/leds/Makefile
> +++ b/drivers/leds/Makefile
> @@ -15,6 +15,7 @@ obj-$(CONFIG_LEDS_AN30259A)		+= leds-an30259a.o
>  obj-$(CONFIG_LEDS_APU)			+= leds-apu.o
>  obj-$(CONFIG_LEDS_ARIEL)		+= leds-ariel.o
>  obj-$(CONFIG_LEDS_AW2013)		+= leds-aw2013.o
> +obj-$(CONFIG_LEDS_AW2026)		+= leds-aw2026.o
>  obj-$(CONFIG_LEDS_BCM6328)		+= leds-bcm6328.o
>  obj-$(CONFIG_LEDS_BCM6358)		+= leds-bcm6358.o
>  obj-$(CONFIG_LEDS_BD2802)		+= leds-bd2802.o
> diff --git a/drivers/leds/leds-aw2026.c b/drivers/leds/leds-aw2026.c
> new file mode 100644
> index 000000000000..7c2d5f62797c
> --- /dev/null
> +++ b/drivers/leds/leds-aw2026.c
> @@ -0,0 +1,578 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Awinic AW2026 3-channel LED driver
> + *
> + * Author: Vladimir Barinov <v.barinov@yadro.com>
> + * Copyright (C) 2023 KNS Group LLC (YADRO)
> + */
> +
> +#include <linux/i2c.h>
> +#include <linux/leds.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/of.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
> +
> +#define AW2026_MAX_LEDS		3
> +
> +/* Chip ID and Software Reset Register */
> +#define AW2026_RSTIDR		0x00
> +#define AW2026_RSTIDR_RESET	0x55
> +#define AW2026_RSTIDR_CHIP_ID	0x31
> +
> +/* Global Control Register */
> +#define AW2026_GCR		0x01
> +#define AW2026_GCR_CHIPEN	BIT(0)
> +
> +/* LED Maximum Current Register */
> +#define AW2026_IMAX		0x03
> +#define AW2026_IMAX_MASK	(BIT(0) | BIT(1))
> +
> +/* LED Configure Register */
> +#define AW2026_LCFG(x)		(0x04 + (x))
> +#define AW2026_LCFG_LEDMD	BIT(0)
> +#define AW2026_LCFG_FADE_IN	BIT(1)
> +#define AW2026_LCFG_FADE_OUT	BIT(2)
> +
> +/* LED Channel Enable Register */
> +#define AW2026_LEDEN		0x07
> +
> +/* Pattern Run/Stop Register */
> +#define AW2026_PATRUN		0x09
> +
> +/* LED Current Register */
> +#define AW2026_ILED(x)		(0x10 + (x))
> +#define AW2026_ILED_MAX		0xFF
> +
> +/* PWM duty level Register */
> +#define AW2026_PWM(x)		(0x1C + (x))
> +#define AW2026_PWM_DUTY_MAX	0xFF
> +
> +/* T1 Time Parameter of Pattern */
> +#define AW2026_PAT_T1(x)	(0x30 + 5*(x))
> +
> +/* T2 Time Parameter of Pattern */
> +#define AW2026_PAT_T2(x)	(0x31 + 5*(x))
> +
> +struct aw2026;

No forward declarations.

Please reorder the struct definitions.

> +enum aw2026_state {
> +	AW2026_STATE_OFF,
> +	AW2026_STATE_ON,
> +	AW2026_STATE_KEEP,
> +};
> +
> +struct aw2026_led {
> +	struct aw2026 *chip;
> +	struct fwnode_handle *fwnode;
> +	struct led_classdev cdev;
> +	enum aw2026_state default_state;
> +	u32 idx;
> +};
> +
> +struct aw2026 {
> +	struct i2c_client *client;
> +	struct regmap *regmap;
> +	struct mutex lock;
> +	struct regulator *vcc_regulator;
> +	struct aw2026_led leds[AW2026_MAX_LEDS];
> +	int num_leds;
> +	unsigned int imax;
> +	bool enabled;
> +};
> +
> +static const struct regmap_config aw2026_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +	.max_register = 0x3e,
> +};
> +
> +struct msec_reg {
> +	u32 msec;
> +	u8 reg;
> +};
> +
> +static const struct msec_reg aw2026_msec_reg[] = {
> +	{ .msec =    4, .reg = 0x0 },
> +	{ .msec =  130, .reg = 0x1 },
> +	{ .msec =  260, .reg = 0x2 },
> +	{ .msec =  380, .reg = 0x3 },
> +	{ .msec =  510, .reg = 0x4 },
> +	{ .msec =  770, .reg = 0x5 },
> +	{ .msec = 1040, .reg = 0x6 },
> +	{ .msec = 1600, .reg = 0x7 },
> +	{ .msec = 2100, .reg = 0x8 },
> +	{ .msec = 2600, .reg = 0x9 },
> +	{ .msec = 3100, .reg = 0xa },
> +	{ .msec = 4200, .reg = 0xb },
> +	{ .msec = 5200, .reg = 0xc },
> +	{ .msec = 6200, .reg = 0xd },
> +	{ .msec = 7300, .reg = 0xe },
> +	{ .msec = 8300, .reg = 0xf },
> +	{ /* sentinel */ },

We really don't need this comment.

> +};
> +
> +static int aw2026_chip_init(struct aw2026 *chip)
> +{
> +	int idx, ret;
> +
> +	ret = regmap_update_bits(chip->regmap, AW2026_GCR, AW2026_GCR_CHIPEN,
> +				 AW2026_GCR_CHIPEN);
> +	if (ret)
> +		return ret;
> +
> +	/* Max current */
> +	ret = regmap_update_bits(chip->regmap, AW2026_IMAX,
> +				 AW2026_IMAX_MASK, chip->imax);
> +	if (ret)
> +		return ret;
> +
> +	for (idx = 0; idx < chip->num_leds; idx++) {
> +		/* PWM level */

Looks obvious from the quality define naming, no?

> +		ret = regmap_write(chip->regmap, AW2026_PWM(idx), AW2026_PWM_DUTY_MAX);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return ret;
> +}
> +
> +static void aw2026_chip_disable(struct aw2026 *chip)
> +{
> +	int ret;
> +
> +	if (!chip->enabled)
> +		return;

What happens if we re-disable a disabled chip or re-enable an enabled
chip?  If the answer is 'nothing, the op is inert' I suggest we remove
this variable from the struct and save ourselves a bunch of lines.

> +	regmap_update_bits(chip->regmap, AW2026_GCR, AW2026_GCR_CHIPEN, 0);
> +
> +	ret = regulator_disable(chip->vcc_regulator);
> +	if (ret) {
> +		dev_err(&chip->client->dev,
> +			"Failed to disable regulator: %d\n", ret);
> +		return;
> +	}
> +
> +	chip->enabled = false;
> +}
> +
> +static int aw2026_chip_enable(struct aw2026 *chip)
> +{
> +	int ret;
> +
> +	if (chip->enabled)
> +		return 0;
> +
> +	ret = regulator_enable(chip->vcc_regulator);
> +	if (ret) {
> +		dev_err(&chip->client->dev,
> +			"Failed to enable regulator: %d\n", ret);
> +		return ret;
> +	}
> +	chip->enabled = true;
> +
> +	ret = aw2026_chip_init(chip);
> +	if (ret)
> +		aw2026_chip_disable(chip);
> +
> +	return ret;
> +}
> +
> +static bool aw2026_chip_in_use(struct aw2026 *chip)
> +{
> +	int i;
> +
> +	for (i = 0; i < chip->num_leds; i++)
> +		if (chip->leds[i].cdev.brightness)
> +			return true;
> +
> +	return false;
> +}
> +
> +static int aw2026_brightness_set(struct led_classdev *cdev,
> +				 enum led_brightness brightness)
> +{
> +	struct aw2026_led *led = container_of(cdev, struct aw2026_led, cdev);
> +	int ret, idx = led->idx;
> +
> +	mutex_lock(&led->chip->lock);
> +
> +	if (aw2026_chip_in_use(led->chip)) {
> +		ret = aw2026_chip_enable(led->chip);
> +		if (ret)
> +			goto error;
> +	}
> +
> +	if (brightness) {
> +		/* Manual mode */
> +		ret = regmap_update_bits(led->chip->regmap, AW2026_LCFG(idx),
> +					 AW2026_LCFG_LEDMD, 0);
> +		if (ret)
> +			goto error;

Nit: '\n'

> +		/* Current configure */
> +		ret = regmap_write(led->chip->regmap, AW2026_ILED(idx), brightness);
> +		if (ret)
> +			goto error;

Nit: '\n'

> +		/* Enable LED */
> +		ret = regmap_update_bits(led->chip->regmap, AW2026_LEDEN, BIT(idx), 0xFF);
> +	} else {
> +		/* Disable LED */
> +		ret = regmap_update_bits(led->chip->regmap, AW2026_LEDEN, BIT(idx), 0);
> +	}
> +	if (ret)
> +		goto error;
> +
> +	if (!aw2026_chip_in_use(led->chip))
> +		aw2026_chip_disable(led->chip);
> +
> +error:
> +	mutex_unlock(&led->chip->lock);
> +
> +	return ret;
> +}
> +
> +static int aw2026_convert_msec_to_reg(struct aw2026 *chip, u32 *msec, u8 *reg)
> +{
> +	const struct msec_reg *value;
> +	const struct msec_reg *prev_value = NULL;
> +
> +	for (value = aw2026_msec_reg; value->msec; value++) {
> +		if (value->msec >= *msec)
> +			break;
> +		prev_value = value;
> +	}
> +
> +	if (!value->msec) {
> +		dev_err(&chip->client->dev, "Unsupported msec (%u)", *msec);
> +		return -ERANGE;

"Math result not representable"

Not sure this is correct.

Would -EINVAL suit your needs better?

> +	}
> +
> +	if (prev_value && ((*msec - prev_value->msec) <= (value->msec - *msec)))

Comment this please.

> +		value = prev_value;
> +
> +	*reg = value->reg;
> +	*msec = value->msec;
> +
> +	return 0;
> +}
> +
> +static int aw2026_pattern_setup(struct aw2026_led *led, u8 trise, u8 ton, u8 tfall, u8 toff)
> +{
> +	int ret, idx = led->idx;
> +
> +	mutex_lock(&led->chip->lock);
> +
> +	if (aw2026_chip_in_use(led->chip)) {
> +		ret = aw2026_chip_enable(led->chip);
> +		if (ret)
> +			goto error;
> +	}
> +
> +	/* Pattern mode */
> +	ret = regmap_update_bits(led->chip->regmap, AW2026_LCFG(idx),
> +				 AW2026_LCFG_LEDMD, 0xFF);
> +	if (ret)
> +		goto error;

Nit: '\n'

Etc ...  you get the idea.  Throughout please.

> +	/* Current configure */
> +	ret = regmap_write(led->chip->regmap, AW2026_ILED(idx), led->cdev.brightness);
> +	if (ret)
> +		goto error;
> +	/* Rise and On time of Pattern */
> +	ret = regmap_write(led->chip->regmap, AW2026_PAT_T1(idx), (trise << 4) | ton);
> +	if (ret)
> +		goto error;
> +	/* Fall and Off time of Pattern */
> +	ret = regmap_write(led->chip->regmap, AW2026_PAT_T2(idx), (tfall << 4) | toff);
> +	if (ret)
> +		goto error;
> +	/* Pattern run for individual LED */
> +	ret = regmap_update_bits(led->chip->regmap, AW2026_PATRUN, BIT(idx), 0xFF);
> +	if (ret)
> +		goto error;
> +	/* Enable LED */
> +	ret = regmap_update_bits(led->chip->regmap, AW2026_LEDEN, BIT(idx), 0xFF);
> +
> +error:
> +	mutex_unlock(&led->chip->lock);
> +
> +	return ret;
> +}
> +
> +static int aw2026_pattern_set(struct led_classdev *cdev,
> +			      struct led_pattern *pattern,
> +			      u32 len, int repeat)
> +{
> +	struct aw2026_led *led = container_of(cdev, struct aw2026_led, cdev);
> +	struct aw2026 *chip = led->chip;
> +	int ret;
> +	u8 trise, ton, tfall, toff;
> +
> +	if (len == 1) {

Comment this section please.

> +		led->cdev.brightness = pattern[0].brightness;
> +		return aw2026_brightness_set(cdev, led->cdev.brightness);
> +	}
> +
> +	if (repeat > 0 || len != 4)
> +		return -EINVAL;
> +
> +	ret = aw2026_convert_msec_to_reg(chip, &pattern[0].delta_t, &trise);
> +	if (ret)
> +		return ret;
> +	ret = aw2026_convert_msec_to_reg(chip, &pattern[1].delta_t, &ton);
> +	if (ret)
> +		return ret;
> +	ret = aw2026_convert_msec_to_reg(chip, &pattern[2].delta_t, &tfall);
> +	if (ret)
> +		return ret;
> +	ret = aw2026_convert_msec_to_reg(chip, &pattern[3].delta_t, &toff);
> +	if (ret)
> +		return ret;
> +
> +	dev_dbg(&chip->client->dev, "pattern timings: %d %d %d %d\n",
> +		trise, ton, tfall, toff);
> +
> +	led->cdev.brightness = max(max(pattern[0].brightness, pattern[1].brightness),
> +				   max(pattern[2].brightness, pattern[3].brightness));
> +
> +	return aw2026_pattern_setup(led, trise, ton, tfall, toff);
> +}
> +
> +static int aw2026_pattern_clear(struct led_classdev *cdev)
> +{
> +	return aw2026_brightness_set(cdev, LED_OFF);
> +}
> +
> +static int aw2026_blink_set(struct led_classdev *cdev,
> +			    unsigned long *delay_on, unsigned long *delay_off)
> +{
> +	struct aw2026_led *led = container_of(cdev, struct aw2026_led, cdev);
> +	struct aw2026 *chip = led->chip;
> +	u8 ton, toff;
> +	int ret;
> +
> +	if (!*delay_on) {

Comment this please.

> +		led->cdev.brightness = LED_OFF;
> +		return aw2026_brightness_set(&led->cdev, LED_OFF);
> +	}
> +
> +	led->cdev.brightness = LED_FULL;
> +
> +	if (!*delay_off)

As above and for everything else that you think relevant.

> +		return aw2026_brightness_set(&led->cdev, LED_FULL);
> +
> +	ret = aw2026_convert_msec_to_reg(chip, (u32 *)delay_on, &ton);
> +	if (ret)
> +		return ret;
> +	ret = aw2026_convert_msec_to_reg(chip, (u32 *)delay_off, &toff);
> +	if (ret)
> +		return ret;
> +
> +	dev_dbg(&chip->client->dev, "blink timings: %d %d\n", ton, toff);
> +
> +	return aw2026_pattern_setup(led, 0, ton, 0, toff);
> +}
> +
> +static int aw2026_parse_dt(struct i2c_client *client, struct aw2026 *chip)
> +{
> +	struct device_node *np = dev_of_node(&client->dev), *child;
> +	int count, ret = 0, i = 0;
> +	struct aw2026_led *led;
> +	const char *str;
> +	u32 imax;
> +
> +	count = of_get_available_child_count(np);
> +	if (!count || count > AW2026_MAX_LEDS)
> +		return -EINVAL;
> +
> +	if (!of_property_read_u32(np, "awinic,led-max-microamp", &imax)) {
> +		chip->imax = min_t(u32, hweight32(imax / 3187 - 1), 3);

No magic numbers.  Please define them all.

> +	} else {
> +		chip->imax = 1; /* 6.375mA */
> +		dev_info(&client->dev,
> +			 "DT property led-max-microamp is missing\n");
> +	}
> +
> +	for_each_available_child_of_node(np, child) {
> +		u32 source;
> +
> +		ret = of_property_read_u32(child, "reg", &source);
> +		if (ret != 0 || source >= AW2026_MAX_LEDS) {

Nit: !ret

> +			dev_err(&chip->client->dev,
> +				"Couldn't read LED address: %d\n", ret);
> +			count--;
> +			continue;
> +		}
> +
> +		led = &chip->leds[i];
> +		led->idx = source;
> +		led->chip = chip;
> +		led->fwnode = of_fwnode_handle(child);
> +
> +		if (!of_property_read_string(child, "default-state", &str)) {
> +			if (!strcmp(str, "on"))
> +				led->default_state = AW2026_STATE_ON;
> +			else if (!strcmp(str, "keep"))
> +				led->default_state = AW2026_STATE_KEEP;
> +			else
> +				led->default_state = AW2026_STATE_OFF;
> +		}
> +
> +		i++;
> +	}
> +
> +	if (!count)
> +		return -EINVAL;
> +
> +	chip->num_leds = i;
> +
> +	return 0;
> +}
> +
> +static void aw2026_init_default_state(struct aw2026_led *led)
> +{
> +	struct aw2026 *chip = led->chip;
> +	int led_on;
> +	int ret, idx = led->idx;
> +
> +	switch (led->default_state) {
> +	case AW2026_STATE_ON:
> +		led->cdev.brightness = LED_FULL;
> +		break;
> +	case AW2026_STATE_KEEP:
> +		/* keep setup made in loader */

Please use correct grammar in comments.

"Keep"

> +		ret = regmap_read(chip->regmap, AW2026_LEDEN, &led_on);
> +		if (ret)
> +			return;
> +
> +		if (!(led_on & BIT(idx))) {
> +			led->cdev.brightness = LED_OFF;
> +			break;
> +		}
> +		regmap_read(chip->regmap, AW2026_ILED(idx),
> +			    &led->cdev.brightness);
> +		return;
> +	default:
> +		led->cdev.brightness = LED_OFF;
> +	}
> +
> +	aw2026_brightness_set(&led->cdev, led->cdev.brightness);
> +}
> +
> +static int aw2026_probe(struct i2c_client *client)
> +{
> +	struct aw2026 *chip;
> +	int i, ret;
> +	unsigned int chipid;

Nit: Please swap the last 2 lines.

> +	chip = devm_kzalloc(&client->dev, sizeof(struct aw2026), GFP_KERNEL);

sizeof(*chip)

> +	if (!chip)
> +		return -ENOMEM;
> +
> +	ret = aw2026_parse_dt(client, chip);
> +	if (ret < 0)

Can aw2026_parse_dt() return >0?

If not, just (ret).

> +		return ret;
> +
> +	mutex_init(&chip->lock);
> +	chip->client = client;
> +	i2c_set_clientdata(client, chip);
> +
> +	chip->regmap = devm_regmap_init_i2c(client, &aw2026_regmap_config);
> +	if (IS_ERR(chip->regmap)) {
> +		ret = PTR_ERR(chip->regmap);
> +		dev_err(&client->dev, "Failed to allocate register map: %d\n",
> +			ret);
> +		goto error;

Do you need to have the mutex init'ed by this point?

Might save a lot of trouble to do it as late as possible.

> +	}
> +
> +	chip->vcc_regulator = devm_regulator_get(&client->dev, "vcc");
> +	ret = PTR_ERR_OR_ZERO(chip->vcc_regulator);
> +	if (ret) {
> +		if (ret != -EPROBE_DEFER)

dev_err_probe()?

> +			dev_err(&client->dev,
> +				"Failed to request regulator: %d\n", ret);
> +		goto error;
> +	}
> +
> +	ret = regulator_enable(chip->vcc_regulator);
> +	if (ret) {
> +		dev_err(&client->dev,
> +			"Failed to enable regulator: %d\n", ret);
> +		goto error;
> +	}
> +
> +	ret = regmap_read(chip->regmap, AW2026_RSTIDR, &chipid);
> +	if (ret) {
> +		dev_err(&client->dev, "Failed to read chip ID: %d\n", ret);
> +		goto error_reg;
> +	}
> +
> +	if (chipid != AW2026_RSTIDR_CHIP_ID) {
> +		ret = -ENODEV;
> +		goto error_reg;
> +	}
> +
> +	for (i = 0; i < chip->num_leds; i++) {
> +		struct led_init_data init_data = {};
> +		struct aw2026_led *led = &chip->leds[i];
> +
> +		aw2026_init_default_state(&chip->leds[i]);
> +		led->cdev.brightness_set_blocking = aw2026_brightness_set;
> +		led->cdev.blink_set = aw2026_blink_set;
> +		led->cdev.pattern_set = aw2026_pattern_set;
> +		led->cdev.pattern_clear = aw2026_pattern_clear;
> +
> +		init_data.fwnode = chip->leds[i].fwnode;
> +
> +		ret = devm_led_classdev_register_ext(&client->dev,
> +						     &led->cdev, &init_data);
> +		if (ret < 0)
> +			goto error_reg;
> +	}
> +
> +	ret = regulator_disable(chip->vcc_regulator);
> +	if (ret) {
> +		dev_err(&client->dev,
> +			"Failed to disable regulator: %d\n", ret);
> +		goto error;
> +	}
> +
> +	return 0;
> +
> +error_reg:
> +	regulator_disable(chip->vcc_regulator);
> +error:
> +	mutex_destroy(&chip->lock);
> +	return ret;
> +}
> +
> +static void aw2026_remove(struct i2c_client *client)
> +{
> +	struct aw2026 *chip = i2c_get_clientdata(client);
> +
> +	aw2026_chip_disable(chip);
> +
> +	mutex_destroy(&chip->lock);
> +}
> +
> +static const struct of_device_id aw2026_match_table[] = {
> +	{ .compatible = "awinic,aw2026", },
> +	{ /* sentinel */ },

Remove this comment please.

> +};
> +
> +MODULE_DEVICE_TABLE(of, aw2026_match_table);
> +
> +static struct i2c_driver aw2026_driver = {
> +	.driver = {
> +		.name = "leds-aw2026",
> +		.of_match_table = of_match_ptr(aw2026_match_table),
> +	},
> +	.probe_new = aw2026_probe,
> +	.remove = aw2026_remove,
> +};
> +

Remove this line.

> +module_i2c_driver(aw2026_driver);
> +
> +MODULE_DESCRIPTION("AW2026 LED driver");
> +MODULE_AUTHOR("Vladimir Barinov");
> +MODULE_LICENSE("GPL");
> -- 
> 2.34.1
> 
> 

-- 
Lee Jones [李琼斯]

Re: [PATCH v2 2/2] dt-bindings: leds: Document Awinic AW2026 bindings

[Krzysztof Kozlowski]

On 25/05/2023 12:14, Vladimir Barinov wrote:
> Add Awinic AW2026 binding documentation
> 
> Signed-off-by: Vladimir Barinov <v.barinov@yadro.com>
> ---
> Changes in version 2:
> - fixed typos in patch header 2016 -> 2026
> - fixed typo in example section that break dt_binding_check
> 
>  .../bindings/leds/awinic,aw2026.yaml          | 92 +++++++++++++++++++
>  1 file changed, 92 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/leds/awinic,aw2026.yaml
> 
> diff --git a/Documentation/devicetree/bindings/leds/awinic,aw2026.yaml b/Documentation/devicetree/bindings/leds/awinic,aw2026.yaml
> new file mode 100664
> index 000000000000..abacf746677b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/awinic,aw2026.yaml
> @@ -0,0 +1,93 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/leds/awinic,aw2026.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Awinic AW2026 3-channel LED Driver
> +
> +maintainers:
> +  - Vladimir Barinov <v.barinov@yadro.com>
> +
> +description: |

Do not need '|' unless you need to preserve formatting.

> +  The AW2026 is a 3-channel LED driver with I2C interface. It can control
> +  LED brightness with PWM output. It supports hardware blinking and
> +  hardware patterns.
> +
> +properties:
> +  compatible:
> +    const: awinic,aw2026
> +
> +  reg:
> +    maxItems: 1
> +
> +  "#address-cells":
> +    const: 1
> +
> +  "#size-cells":
> +    const: 0
> +
> +  awinic,led-max-microamp:

microamp is per sub-LED, not per entire device. This is already
expressed by common bindings. Drop entire property and use common one in
children.



> +    description:
> +      Maximum current at LED output
> +    enum:
> +      [3000, 6375, 12750, 25500]
> +
> +  vcc-supply:
> +    description: Regulator providing power to the "VBAT" pin.
> +
> +patternProperties:
> +  "^led@[0-2]$":
> +    type: object
> +    $ref: common.yaml#

unevaluatedProperties: false

Just open existing bindings and do not code it differently...

> +
> +    properties:
> +      reg:
> +        description: Index of the LED.
> +        minimum: 0
> +        maximum: 2
> +
> +required:
> +  - compatible
> +  - reg
> +  - "#address-cells"
> +  - "#size-cells"
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/leds/common.h>
> +
> +    i2c0 {

i2c

> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        led-controller@64 {
> +            compatible = "awinic,aw2026";
> +            reg = <0x64>;
> +            #address-cells = <1>;
> +            #size-cells = <0>;
> +            awinic,led-max-microamp = <6375>;
> +            vcc-supply = <&vcc_3v3_s0>;
> +
> +            led@0 {
> +                    reg = <0>;

Wrong indenration.
Use 4 spaces for example indentation.

> +                    function = LED_FUNCTION_INDICATOR;
> +                    color = <LED_COLOR_ID_RED>;
> +            };

Best regards,
Krzysztof