[PATCH RFC 0/3] leds: aw2013: Rename aw2013 to aw20xx and add AW2027 support

[PATCH RFC 0/3] leds: aw2013: Rename aw2013 to aw20xx and add AW2027 support

[Paul Sajna]

Requesting feedback on this approach of adding additional chip support
to leds-aw2013. 

It comes with some downsides, such as the rename
introducing Kconfig option and module-name incompatibilities. 

Additionally, there may be easy confusion between the pre-existing 
aw200xx and the proposed aw20xx, differing only by plus-or-minus 
a zero.

However, given this approach in aw200xx, I think aw20xx can follow
suit in a similar fashion. The 20xx series chips are all similar 
3-channel LED drivers, see the driver commit for a comparison link.

Lastly, I noticed this driver does not have an entry in the MAINTAINERS
file. Let me know if that needs updating and how please.

My working tree is also available on GitHub if you prefer that interface for
any reason: https://github.com/sajattack/sdm660-linux/commits/b4/aw2013-aw20xx-rename/

This RFC resulted out of discussions pertaining to
sdm660-mainline/linux, a close-to-mainline fork for Qualcomm Snapdragon 660
devices, specifically for the BlackBerry KEY2 Keyboard Backlight,
in the following GitHub PR: https://github.com/sdm660-mainline/linux/pull/218

Signed-off-by: Paul Sajna <sajattack@postmarketos.org>
---
Paul Sajna (3):
      dt-bindings: leds: aw2013: Add aw2027 compatible and rename to aw20xx
      leds: aw2013: Rename to aw20xx
      leds: aw2013: Add AW2027 support and rename to aw20xx

 .../leds/{leds-aw2013.yaml => leds-aw20xx.yaml}    |  10 +-
 drivers/leds/Kconfig                               |  14 +-
 drivers/leds/Makefile                              |   2 +-
 drivers/leds/leds-aw2013.c                         | 441 -----------------
 drivers/leds/leds-aw20xx.c                         | 525 +++++++++++++++++++++
 5 files changed, 541 insertions(+), 451 deletions(-)
---
base-commit: 944a035ecca915ae947905dcfb03f2b9dc6d032c
change-id: 20260907-aw2013-aw20xx-rename-df233c5921c6

Best regards,
-- 
Paul Sajna <sajattack@postmarketos.org>

[PATCH RFC 1/3] dt-bindings: leds: aw2013: Add aw2027 compatible and rename to aw20xx

[Paul Sajna]

Document the awinic,aw2027, which is very similar to aw2013.
Rename the driver to support all of the 20xx series.

Signed-off-by: Paul Sajna <sajattack@postmarketos.org>
---
 .../bindings/leds/{leds-aw2013.yaml => leds-aw20xx.yaml}       | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/leds/leds-aw2013.yaml b/Documentation/devicetree/bindings/leds/leds-aw20xx.yaml
similarity index 91%
rename from Documentation/devicetree/bindings/leds/leds-aw2013.yaml
rename to Documentation/devicetree/bindings/leds/leds-aw20xx.yaml
index 26238446f2bd..f20b5344e3e1 100644
--- a/Documentation/devicetree/bindings/leds/leds-aw2013.yaml
+++ b/Documentation/devicetree/bindings/leds/leds-aw20xx.yaml
@@ -1,21 +1,23 @@
 # SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
 %YAML 1.2
 ---
-$id: http://devicetree.org/schemas/leds/leds-aw2013.yaml#
+$id: http://devicetree.org/schemas/leds/leds-aw20xx.yaml#
 $schema: http://devicetree.org/meta-schemas/core.yaml#
 
-title: AWINIC AW2013 3-channel LED Driver
+title: AWINIC AW20XX 3-channel LED Driver
 
 maintainers:
   - Nikita Travkin <nikitos.tr@gmail.com>
 
 description: |
-  The AW2013 is a 3-channel LED driver with I2C interface. It can control
+  The AW20XX is a 3-channel LED driver with I2C interface. It can control
   LED brightness with PWM output.
 
 properties:
   compatible:
-    const: awinic,aw2013
+    enum:
+      - awinic,aw2013
+      - awinic,aw2027
 
   reg:
     maxItems: 1

-- 
2.55.0

[PATCH RFC 2/3] leds: aw2013: Rename to aw20xx

[Paul Sajna]

I had to split this into a separate commit for git to properly detect
this as a rename rather than remove/add. Next commit will add another
chip to justify the rename.

Includes update of Makefile, Kconfig, Kconfig help, and raw leds-aw2013.c
rename.

Signed-off-by: Paul Sajna <sajattack@postmarketos.org>
---
 drivers/leds/Kconfig                          | 14 +++++++++-----
 drivers/leds/Makefile                         |  2 +-
 drivers/leds/{leds-aw2013.c => leds-aw20xx.c} |  0
 3 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index d0d3182236be..7548c320e55c 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -137,16 +137,20 @@ config LEDS_AW200XX
 	  To compile this driver as a module, choose M here: the module
 	  will be called leds-aw200xx.
 
-config LEDS_AW2013
-	tristate "LED support for Awinic AW2013"
+config LEDS_AW20XX
+	tristate "LED support for Awinic AW2013/AW2027"
 	depends on LEDS_CLASS && I2C && OF
 	select REGMAP_I2C
 	help
-	  This option enables support for the AW2013 3-channel
-	  LED driver.
+	  This option enables support for the Awinic AW20XX 3-channel
+	  LED controllers. Devices have a set of individually controlled
+	  LEDs and support individual dimming and blinking patterns via
+	  an I2C interface. Supported devices:
+	  - AW2013
+	  - AW2027
 
 	  To compile this driver as a module, choose M here: the module
-	  will be called leds-aw2013.
+	  will be called leds-aw20xx.
 
 config LEDS_BCM6328
 	tristate "LED Support for Broadcom BCM6328"
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index a68244bd50fb..b20a744065a5 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -17,7 +17,7 @@ obj-$(CONFIG_LEDS_APU)			+= leds-apu.o
 obj-$(CONFIG_LEDS_ARIEL)		+= leds-ariel.o
 obj-$(CONFIG_LEDS_OSRAM_AMS_AS3668)	+= leds-as3668.o
 obj-$(CONFIG_LEDS_AW200XX)		+= leds-aw200xx.o
-obj-$(CONFIG_LEDS_AW2013)		+= leds-aw2013.o
+obj-$(CONFIG_LEDS_AW20XX)		+= leds-aw20xx.o
 obj-$(CONFIG_LEDS_BCM6328)		+= leds-bcm6328.o
 obj-$(CONFIG_LEDS_BCM6358)		+= leds-bcm6358.o
 obj-$(CONFIG_LEDS_BD2606MVV)		+= leds-bd2606mvv.o
diff --git a/drivers/leds/leds-aw2013.c b/drivers/leds/leds-aw20xx.c
similarity index 100%
rename from drivers/leds/leds-aw2013.c
rename to drivers/leds/leds-aw20xx.c

-- 
2.55.0

[PATCH RFC 3/3] leds: aw2013: Add AW2027 support and rename to aw20xx

[Paul Sajna]

The AW2027 is basically the same as the AW2013.

It differs in:
- the max current (30mA rather than 15)
- the number of current levels for colour-mixing (16 rather than 4)
- the supply voltage max
- the charge indicator (unsupported by our driver at this time)

The max current register (imax) also moves from LCFG to GCR2, and the
selectable current levels for colour-mixing (LCFG_CUR) take their place.

The current driver implementation provided here sets the global imax to
it's maximum, and scales each led to the closest value using the local
imax. It's the best result achievable short of adding custom sysfs
entries and mucking up the linux interfaces.

Comparison of all AW20xx chips https://www.awinic.com/en/productCompareList?id=47&ids=1022,74,96,75,72,71,928,927,73,76,578
AW2027 datasheet: https://doc.awinic.com/doc/202507/77b87097-27bf-48cd-af9b-9b615064c1e1.pdf
Vendor driver: https://www.awinic.com/en/driver/AW2027DNR/454

Signed-off-by: Paul Sajna <sajattack@postmarketos.org>
---
 drivers/leds/leds-aw20xx.c | 306 +++++++++++++++++++++++++++++----------------
 1 file changed, 195 insertions(+), 111 deletions(-)

diff --git a/drivers/leds/leds-aw20xx.c b/drivers/leds/leds-aw20xx.c
index 216755d6010f..635965b712d4 100644
--- a/drivers/leds/leds-aw20xx.c
+++ b/drivers/leds/leds-aw20xx.c
@@ -1,5 +1,5 @@
 // SPDX-License-Identifier: GPL-2.0+
-// Driver for Awinic AW2013 3-channel LED driver
+// Driver for Awinic AW20XX 3-channel LED drivers
 
 #include <linux/i2c.h>
 #include <linux/leds.h>
@@ -9,102 +9,188 @@
 #include <linux/of.h>
 #include <linux/regmap.h>
 
-#define AW2013_MAX_LEDS 3
+#define AW20XX_MAX_LEDS 3
 
 /* Reset and ID register */
-#define AW2013_RSTR 0x00
-#define AW2013_RSTR_RESET 0x55
+#define AW20XX_RSTR 0x00
+#define AW20XX_RSTR_RESET 0x55
 #define AW2013_RSTR_CHIP_ID 0x33
+#define AW2027_RSTR_CHIP_ID 0x09
 
 /* Global control register */
-#define AW2013_GCR 0x01
-#define AW2013_GCR_ENABLE BIT(0)
+#define AW20XX_GCR 0x01
+#define AW20XX_GCR_ENABLE BIT(0)
+
+#define AW20XX_GCR2 0x04
+#define AW20XX_IMAX_MASK (BIT(0) | BIT(1)) // Should be 0-3
 
 /* LED channel enable register */
-#define AW2013_LCTR 0x30
-#define AW2013_LCTR_LE(x) BIT((x))
+#define AW20XX_LCTR 0x30
+#define AW20XX_LCTR_LE(x) BIT((x))
 
 /* LED channel control registers */
-#define AW2013_LCFG(x) (0x31 + (x))
-#define AW2013_LCFG_IMAX_MASK (BIT(0) | BIT(1)) // Should be 0-3
-#define AW2013_LCFG_MD BIT(4)
-#define AW2013_LCFG_FI BIT(5)
-#define AW2013_LCFG_FO BIT(6)
+#define AW20XX_LCFG(x) (0x31 + (x))
+#define AW20XX_LCFG_CUR_MASK GENMASK(3, 0)
+#define AW20XX_LCFG_MD BIT(4)
+#define AW20XX_LCFG_FI BIT(5)
+#define AW20XX_LCFG_FO BIT(6)
+
+#define AW2027_IMAX_15MA (0)
+#define AW2027_IMAX_30MA (1)
+#define AW2027_IMAX_5MA  (2)
+#define AW2027_IMAX_10MA (3)
 
 /* LED channel PWM registers */
-#define AW2013_REG_PWM(x) (0x34 + (x))
+#define AW20XX_REG_PWM(x) (0x34 + (x))
 
 /* LED channel timing registers */
-#define AW2013_LEDT0(x) (0x37 + (x) * 3)
-#define AW2013_LEDT0_T1(x) ((x) << 4) // Should be 0-7
-#define AW2013_LEDT0_T2(x) (x) // Should be 0-5
+#define AW20XX_LEDT0(x) (0x37 + (x) * 3)
+#define AW20XX_LEDT0_T1(x) ((x) << 4) // Should be 0-7
+#define AW20XX_LEDT0_T2(x) (x) // Should be 0-5
 
-#define AW2013_LEDT1(x) (0x38 + (x) * 3)
-#define AW2013_LEDT1_T3(x) ((x) << 4) // Should be 0-7
-#define AW2013_LEDT1_T4(x) (x) // Should be 0-7
+#define AW20XX_LEDT1(x) (0x38 + (x) * 3)
+#define AW20XX_LEDT1_T3(x) ((x) << 4) // Should be 0-7
+#define AW20XX_LEDT1_T4(x) (x) // Should be 0-7
 
-#define AW2013_LEDT2(x) (0x39 + (x) * 3)
-#define AW2013_LEDT2_T0(x) ((x) << 4) // Should be 0-8
-#define AW2013_LEDT2_REPEAT(x) (x) // Should be 0-15
+#define AW20XX_LEDT2(x) (0x39 + (x) * 3)
+#define AW20XX_LEDT2_T0(x) ((x) << 4) // Should be 0-8
+#define AW20XX_LEDT2_REPEAT(x) (x) // Should be 0-15
 
 #define AW2013_REG_MAX 0x77
+#define AW2027_REG_MAX 0x7F /* copied from vendor driver, but only up to 0x3F is documented */
 
-#define AW2013_TIME_STEP 130 /* ms */
+#define AW20XX_TIME_STEP 130 /* ms */
 
-struct aw2013;
+struct aw20xx;
 
-struct aw2013_led {
-	struct aw2013 *chip;
+struct aw20xx_led {
+	struct aw20xx *chip;
 	struct led_classdev cdev;
 	u32 num;
 	unsigned int imax;
 };
 
-struct aw2013 {
+struct aw20xx_chipdef {
+	u8 chip_id;
+	const struct regmap_config regmap_cfg;
+	u32 default_imax;
+	bool gcr2_imax;
+	unsigned int current_levels;
+	unsigned int current_max;
+};
+
+struct aw20xx {
 	struct mutex mutex; /* held when writing to registers */
 	struct regulator_bulk_data regulators[2];
-	struct i2c_client *client;
-	struct aw2013_led leds[AW2013_MAX_LEDS];
 	struct regmap *regmap;
+	struct i2c_client *client;
+	struct aw20xx_led leds[AW20XX_MAX_LEDS];
+	const struct aw20xx_chipdef *cdef;
 	int num_leds;
 	bool enabled;
 };
 
-static int aw2013_chip_init(struct aw2013 *chip)
+static const struct regmap_config aw2013_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.max_register = AW2013_REG_MAX,
+};
+
+static const struct regmap_config aw2027_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.max_register = AW2027_REG_MAX,
+};
+
+/* finds the closest current step to the given microamp
+ * given the max current and number of levels
+ */
+inline u32 aw20xx_microamp_to_imax(u32 microamp, const struct aw20xx_chipdef *cdef)
+{
+	return min_t(u32, ((microamp * (cdef->current_levels - 1))
+			  + (cdef->current_max / 2)) / cdef->current_max,
+		     cdef->current_levels - 1);
+}
+
+static const struct aw20xx_chipdef aw2013_chipdef = {
+	.chip_id = AW2013_RSTR_CHIP_ID,
+	.regmap_cfg = aw2013_regmap_config,
+	.default_imax = 0b01, // 5mA
+	.gcr2_imax = false,
+	.current_levels = 4,
+	.current_max = 15000,
+};
+
+static const struct aw20xx_chipdef aw2027_chipdef = {
+	.chip_id = AW2027_RSTR_CHIP_ID,
+	.regmap_cfg = aw2027_regmap_config,
+	.default_imax = AW2027_IMAX_15MA,
+	.gcr2_imax = true,
+	.current_levels = 16,
+	.current_max = 30000,
+};
+
+static int aw20xx_chip_init(struct aw20xx *chip)
 {
 	int i, ret;
 
-	ret = regmap_write(chip->regmap, AW2013_GCR, AW2013_GCR_ENABLE);
+	ret = regmap_write(chip->regmap, AW20XX_GCR, AW20XX_GCR_ENABLE);
 	if (ret) {
 		dev_err(&chip->client->dev, "Failed to enable the chip: %d\n",
 			ret);
 		return ret;
 	}
 
-	for (i = 0; i < chip->num_leds; i++) {
+	if (chip->cdef->gcr2_imax) {
+		/* AW2027 supports 4-step global imax, and also a 16-step control for limiting
+		 * individual current per LED. This doesn't map to the single value the
+		 * devicetree provides super well, so set global imax  to maximum, and
+		 * local imax to whatever gets us closest to the value in the devicetree
+		 */
 		ret = regmap_update_bits(chip->regmap,
-					 AW2013_LCFG(chip->leds[i].num),
-					 AW2013_LCFG_IMAX_MASK,
-					 chip->leds[i].imax);
-		if (ret) {
-			dev_err(&chip->client->dev,
-				"Failed to set maximum current for led %d: %d\n",
-				chip->leds[i].num, ret);
-			return ret;
+				AW20XX_GCR2,
+				AW20XX_IMAX_MASK,
+				AW2027_IMAX_30MA);
+
+		for (i = 0; i < chip->num_leds; i++) {
+			ret = regmap_update_bits(chip->regmap,
+						 AW20XX_LCFG(chip->leds[i].num),
+						 AW20XX_LCFG_CUR_MASK,
+						 chip->leds[i].imax);
+			if (ret) {
+				dev_err(&chip->client->dev,
+					"Failed to set maximum current for led %d: %d\n",
+					chip->leds[i].num, ret);
+				return ret;
+			}
+		}
+	} else {
+		/* AW2013 only supports 4-step individual current per LED */
+		for (i = 0; i < chip->num_leds; i++) {
+			ret = regmap_update_bits(chip->regmap,
+						 AW20XX_LCFG(chip->leds[i].num),
+						 AW20XX_IMAX_MASK,
+						 chip->leds[i].imax);
+			if (ret) {
+				dev_err(&chip->client->dev,
+					"Failed to set maximum current for led %d: %d\n",
+					chip->leds[i].num, ret);
+				return ret;
+			}
 		}
 	}
 
 	return ret;
 }
 
-static void aw2013_chip_disable(struct aw2013 *chip)
+static void aw20xx_chip_disable(struct aw20xx *chip)
 {
 	int ret;
 
 	if (!chip->enabled)
 		return;
 
-	regmap_write(chip->regmap, AW2013_GCR, 0);
+	regmap_write(chip->regmap, AW20XX_GCR, 0);
 
 	ret = regulator_bulk_disable(ARRAY_SIZE(chip->regulators),
 				     chip->regulators);
@@ -117,7 +203,7 @@ static void aw2013_chip_disable(struct aw2013 *chip)
 	chip->enabled = false;
 }
 
-static int aw2013_chip_enable(struct aw2013 *chip)
+static int aw20xx_chip_enable(struct aw20xx *chip)
 {
 	int ret;
 
@@ -133,14 +219,14 @@ static int aw2013_chip_enable(struct aw2013 *chip)
 	}
 	chip->enabled = true;
 
-	ret = aw2013_chip_init(chip);
+	ret = aw20xx_chip_init(chip);
 	if (ret)
-		aw2013_chip_disable(chip);
+		aw20xx_chip_disable(chip);
 
 	return ret;
 }
 
-static bool aw2013_chip_in_use(struct aw2013 *chip)
+static bool aw20xx_chip_in_use(struct aw20xx *chip)
 {
 	int i;
 
@@ -151,42 +237,42 @@ static bool aw2013_chip_in_use(struct aw2013 *chip)
 	return false;
 }
 
-static int aw2013_brightness_set(struct led_classdev *cdev,
+static int aw20xx_brightness_set(struct led_classdev *cdev,
 				 enum led_brightness brightness)
 {
-	struct aw2013_led *led = container_of(cdev, struct aw2013_led, cdev);
+	struct aw20xx_led *led = container_of(cdev, struct aw20xx_led, cdev);
 	int ret, num;
 
 	mutex_lock(&led->chip->mutex);
 
-	if (aw2013_chip_in_use(led->chip)) {
-		ret = aw2013_chip_enable(led->chip);
+	if (aw20xx_chip_in_use(led->chip)) {
+		ret = aw20xx_chip_enable(led->chip);
 		if (ret)
 			goto error;
 	}
 
 	num = led->num;
 
-	ret = regmap_write(led->chip->regmap, AW2013_REG_PWM(num), brightness);
+	ret = regmap_write(led->chip->regmap, AW20XX_REG_PWM(num), brightness);
 	if (ret)
 		goto error;
 
 	if (brightness) {
-		ret = regmap_update_bits(led->chip->regmap, AW2013_LCTR,
-					 AW2013_LCTR_LE(num), 0xFF);
+		ret = regmap_update_bits(led->chip->regmap, AW20XX_LCTR,
+					 AW20XX_LCTR_LE(num), 0xFF);
 	} else {
-		ret = regmap_update_bits(led->chip->regmap, AW2013_LCTR,
-					 AW2013_LCTR_LE(num), 0);
+		ret = regmap_update_bits(led->chip->regmap, AW20XX_LCTR,
+					 AW20XX_LCTR_LE(num), 0);
 		if (ret)
 			goto error;
-		ret = regmap_update_bits(led->chip->regmap, AW2013_LCFG(num),
-					 AW2013_LCFG_MD, 0);
+		ret = regmap_update_bits(led->chip->regmap, AW20XX_LCFG(num),
+					 AW20XX_LCFG_MD, 0);
 	}
 	if (ret)
 		goto error;
 
-	if (!aw2013_chip_in_use(led->chip))
-		aw2013_chip_disable(led->chip);
+	if (!aw20xx_chip_in_use(led->chip))
+		aw20xx_chip_disable(led->chip);
 
 error:
 	mutex_unlock(&led->chip->mutex);
@@ -194,10 +280,10 @@ static int aw2013_brightness_set(struct led_classdev *cdev,
 	return ret;
 }
 
-static int aw2013_blink_set(struct led_classdev *cdev,
+static int aw20xx_blink_set(struct led_classdev *cdev,
 			    unsigned long *delay_on, unsigned long *delay_off)
 {
-	struct aw2013_led *led = container_of(cdev, struct aw2013_led, cdev);
+	struct aw20xx_led *led = container_of(cdev, struct aw20xx_led, cdev);
 	int ret, num = led->num;
 	unsigned long off = 0, on = 0;
 
@@ -209,7 +295,7 @@ static int aw2013_blink_set(struct led_classdev *cdev,
 
 	if (!led->cdev.brightness) {
 		led->cdev.brightness = LED_FULL;
-		ret = aw2013_brightness_set(&led->cdev, led->cdev.brightness);
+		ret = aw20xx_brightness_set(&led->cdev, led->cdev.brightness);
 		if (ret)
 			return ret;
 	}
@@ -217,43 +303,43 @@ static int aw2013_blink_set(struct led_classdev *cdev,
 	/* Never on - just set to off */
 	if (!*delay_on) {
 		led->cdev.brightness = LED_OFF;
-		return aw2013_brightness_set(&led->cdev, LED_OFF);
+		return aw20xx_brightness_set(&led->cdev, LED_OFF);
 	}
 
 	mutex_lock(&led->chip->mutex);
 
 	/* Never off - brightness is already set, disable blinking */
 	if (!*delay_off) {
-		ret = regmap_update_bits(led->chip->regmap, AW2013_LCFG(num),
-					 AW2013_LCFG_MD, 0);
+		ret = regmap_update_bits(led->chip->regmap, AW20XX_LCFG(num),
+					 AW20XX_LCFG_MD, 0);
 		goto out;
 	}
 
 	/* Convert into values the HW will understand. */
-	off = min(5, ilog2((*delay_off - 1) / AW2013_TIME_STEP) + 1);
-	on = min(7, ilog2((*delay_on - 1) / AW2013_TIME_STEP) + 1);
+	off = min(5, ilog2((*delay_off - 1) / AW20XX_TIME_STEP) + 1);
+	on = min(7, ilog2((*delay_on - 1) / AW20XX_TIME_STEP) + 1);
 
-	*delay_off = BIT(off) * AW2013_TIME_STEP;
-	*delay_on = BIT(on) * AW2013_TIME_STEP;
+	*delay_off = BIT(off) * AW20XX_TIME_STEP;
+	*delay_on = BIT(on) * AW20XX_TIME_STEP;
 
 	/* Set timings */
 	ret = regmap_write(led->chip->regmap,
-			   AW2013_LEDT0(num), AW2013_LEDT0_T2(on));
+			   AW20XX_LEDT0(num), AW20XX_LEDT0_T2(on));
 	if (ret)
 		goto out;
 	ret = regmap_write(led->chip->regmap,
-			   AW2013_LEDT1(num), AW2013_LEDT1_T4(off));
+			   AW20XX_LEDT1(num), AW20XX_LEDT1_T4(off));
 	if (ret)
 		goto out;
 
 	/* Finally, enable the LED */
-	ret = regmap_update_bits(led->chip->regmap, AW2013_LCFG(num),
-				 AW2013_LCFG_MD, 0xFF);
+	ret = regmap_update_bits(led->chip->regmap, AW20XX_LCFG(num),
+				 AW20XX_LCFG_MD, 0xFF);
 	if (ret)
 		goto out;
 
-	ret = regmap_update_bits(led->chip->regmap, AW2013_LCTR,
-				 AW2013_LCTR_LE(num), 0xFF);
+	ret = regmap_update_bits(led->chip->regmap, AW20XX_LCTR,
+				 AW20XX_LCTR_LE(num), 0xFF);
 
 out:
 	mutex_unlock(&led->chip->mutex);
@@ -261,17 +347,17 @@ static int aw2013_blink_set(struct led_classdev *cdev,
 	return ret;
 }
 
-static int aw2013_probe_dt(struct aw2013 *chip)
+static int aw20xx_probe_dt(struct aw20xx *chip)
 {
 	struct device_node *np = dev_of_node(&chip->client->dev);
 	int count, ret = 0, i = 0;
-	struct aw2013_led *led;
+	struct aw20xx_led *led;
 
 	count = of_get_available_child_count(np);
-	if (!count || count > AW2013_MAX_LEDS)
+	if (!count || count > AW20XX_MAX_LEDS)
 		return -EINVAL;
 
-	regmap_write(chip->regmap, AW2013_RSTR, AW2013_RSTR_RESET);
+	regmap_write(chip->regmap, AW20XX_RSTR, AW20XX_RSTR_RESET);
 
 	for_each_available_child_of_node_scoped(np, child) {
 		struct led_init_data init_data = {};
@@ -279,7 +365,7 @@ static int aw2013_probe_dt(struct aw2013 *chip)
 		u32 imax;
 
 		ret = of_property_read_u32(child, "reg", &source);
-		if (ret != 0 || source >= AW2013_MAX_LEDS) {
+		if (ret != 0 || source >= AW20XX_MAX_LEDS) {
 			dev_err(&chip->client->dev,
 				"Couldn't read LED address: %d\n", ret);
 			count--;
@@ -292,15 +378,15 @@ static int aw2013_probe_dt(struct aw2013 *chip)
 		init_data.fwnode = of_fwnode_handle(child);
 
 		if (!of_property_read_u32(child, "led-max-microamp", &imax)) {
-			led->imax = min_t(u32, imax / 5000, 3);
+			led->imax = aw20xx_microamp_to_imax(imax, chip->cdef);
 		} else {
-			led->imax = 1; // 5mA
+			led->imax = chip->cdef->default_imax;
 			dev_info(&chip->client->dev,
 				 "DT property led-max-microamp is missing\n");
 		}
 
-		led->cdev.brightness_set_blocking = aw2013_brightness_set;
-		led->cdev.blink_set = aw2013_blink_set;
+		led->cdev.brightness_set_blocking = aw20xx_brightness_set;
+		led->cdev.blink_set = aw20xx_blink_set;
 
 		ret = devm_led_classdev_register_ext(&chip->client->dev,
 						     &led->cdev, &init_data);
@@ -318,23 +404,20 @@ static int aw2013_probe_dt(struct aw2013 *chip)
 	return 0;
 }
 
-static void aw2013_chip_disable_action(void *data)
+static void aw20xx_chip_disable_action(void *data)
 {
-	aw2013_chip_disable(data);
+	aw20xx_chip_disable(data);
 }
 
-static const struct regmap_config aw2013_regmap_config = {
-	.reg_bits = 8,
-	.val_bits = 8,
-	.max_register = AW2013_REG_MAX,
-};
-
-static int aw2013_probe(struct i2c_client *client)
+static int aw20xx_probe(struct i2c_client *client)
 {
-	struct aw2013 *chip;
+	struct aw20xx *chip;
+	const struct aw20xx_chipdef *cdef;
 	int ret;
 	unsigned int chipid;
 
+	cdef = device_get_match_data(&client->dev);
+
 	chip = devm_kzalloc(&client->dev, sizeof(*chip), GFP_KERNEL);
 	if (!chip)
 		return -ENOMEM;
@@ -346,9 +429,10 @@ static int aw2013_probe(struct i2c_client *client)
 	mutex_lock(&chip->mutex);
 
 	chip->client = client;
+	chip->cdef = cdef;
 	i2c_set_clientdata(client, chip);
 
-	chip->regmap = devm_regmap_init_i2c(client, &aw2013_regmap_config);
+	chip->regmap = devm_regmap_init_i2c(client, &chip->cdef->regmap_cfg);
 	if (IS_ERR(chip->regmap)) {
 		ret = PTR_ERR(chip->regmap);
 		dev_err(&client->dev, "Failed to allocate register map: %d\n",
@@ -369,32 +453,31 @@ static int aw2013_probe(struct i2c_client *client)
 	}
 
 	ret = regulator_bulk_enable(ARRAY_SIZE(chip->regulators),
-				    chip->regulators);
+					chip->regulators);
 	if (ret) {
 		dev_err(&client->dev,
 			"Failed to enable regulators: %d\n", ret);
 		goto error;
 	}
 
-	ret = regmap_read(chip->regmap, AW2013_RSTR, &chipid);
+	ret = regmap_read(chip->regmap, AW20XX_RSTR, &chipid);
 	if (ret) {
 		dev_err(&client->dev, "Failed to read chip ID: %d\n",
 			ret);
 		goto error_reg;
 	}
-
-	if (chipid != AW2013_RSTR_CHIP_ID) {
+	if (chipid != chip->cdef->chip_id) {
 		dev_err(&client->dev, "Chip reported wrong ID: %x\n",
 			chipid);
 		ret = -ENODEV;
 		goto error_reg;
 	}
 
-	ret = devm_add_action(&client->dev, aw2013_chip_disable_action, chip);
+	ret = devm_add_action(&client->dev, aw20xx_chip_disable_action, chip);
 	if (ret)
 		goto error_reg;
 
-	ret = aw2013_probe_dt(chip);
+	ret = aw20xx_probe_dt(chip);
 	if (ret < 0)
 		goto error_reg;
 
@@ -419,23 +502,24 @@ static int aw2013_probe(struct i2c_client *client)
 	return ret;
 }
 
-static const struct of_device_id aw2013_match_table[] = {
-	{ .compatible = "awinic,aw2013", },
+static const struct of_device_id aw20xx_match_table[] = {
+	{ .compatible = "awinic,aw2013", .data = &aw2013_chipdef },
+	{ .compatible = "awinic,aw2027", .data = &aw2027_chipdef },
 	{ /* sentinel */ },
 };
 
-MODULE_DEVICE_TABLE(of, aw2013_match_table);
+MODULE_DEVICE_TABLE(of, aw20xx_match_table);
 
-static struct i2c_driver aw2013_driver = {
+static struct i2c_driver aw20xx_driver = {
 	.driver = {
-		.name = "leds-aw2013",
-		.of_match_table = aw2013_match_table,
+		.name = "leds-aw20xx",
+		.of_match_table = aw20xx_match_table,
 	},
-	.probe = aw2013_probe,
+	.probe = aw20xx_probe,
 };
 
-module_i2c_driver(aw2013_driver);
+module_i2c_driver(aw20xx_driver);
 
 MODULE_AUTHOR("Nikita Travkin <nikitos.tr@gmail.com>");
-MODULE_DESCRIPTION("AW2013 LED driver");
+MODULE_DESCRIPTION("AW20XX LED driver");
 MODULE_LICENSE("GPL v2");

-- 
2.55.0

Re: [PATCH RFC 1/3] dt-bindings: leds: aw2013: Add aw2027 compatible and rename to aw20xx

[sashiko-bot]

> Document the awinic,aw2027, which is very similar to aw2013.
> Rename the driver to support all of the 20xx series.
> 
> Signed-off-by: Paul Sajna <sajattack@postmarketos.org>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260907-aw2013-aw20xx-rename-v1-0-c9a7df12a701@postmarketos.org?part=1

Re: [PATCH RFC 2/3] leds: aw2013: Rename to aw20xx

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] I2C writes are sent to an unpowered chip when turning off an already disabled LED during teardown, probe, or unregistration.
--

commit 61355e5955755b194e075840a94eea7f1c7e4fa1
Author: Paul Sajna <sajattack@postmarketos.org>
Date:   Mon Sep 7 23:47:52 2026 +0000

    leds: aw2013: Rename to aw20xx
    
    I had to split this into a separate commit for git to properly detect
    this as a rename rather than remove/add. Next commit will add another
    chip to justify the rename.
    
    Includes update of Makefile, Kconfig, Kconfig help, and raw leds-aw2013.c
    rename.
    
    Signed-off-by: Paul Sajna <sajattack@postmarketos.org>

[ ... ]

> diff --git a/drivers/leds/leds-aw2013.c b/drivers/leds/leds-aw20xx.c
> similarity index 100%
> rename from drivers/leds/leds-aw2013.c
> rename to drivers/leds/leds-aw20xx.c

[Severity: High]
This isn't a bug introduced by this patch, but is it possible that I2C writes
are sent to an unpowered chip when turning off an already disabled LED?

During teardown, probe, or unregistration, the LED core initializes
cdev->brightness to 0 and queues a work to set the brightness to 0. This can
also be triggered by unprivileged user space via sysfs by writing 0 to the
brightness file when already 0.

When this occurs, aw2013_chip_in_use() evaluates to false and skips enabling
the regulators:

drivers/leds/leds-aw20xx.c:aw2013_brightness_set() {
    ...
	if (aw2013_chip_in_use(led->chip)) {
		ret = aw2013_chip_enable(led->chip);
		if (ret)
			goto error;
	}

	num = led->num;

	ret = regmap_write(led->chip->regmap, AW2013_REG_PWM(num), brightness);
    ...
}

Because aw2013_chip_enable() is skipped, the driver then unconditionally
calls regmap_write() and regmap_update_bits(), which perform I2C
transactions to the unpowered chip.

Could writing to an unpowered I2C device clamp the SDA/SCL lines to ground
via the chip's internal ESD diodes, hanging the entire I2C bus and making
all other devices on the bus unusable?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260907-aw2013-aw20xx-rename-v1-0-c9a7df12a701@postmarketos.org?part=2

Re: [PATCH RFC 3/3] leds: aw2013: Add AW2027 support and rename to aw20xx

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:

New issues:
- [High] Missing NULL check for the result of `device_get_match_data()`, which leads to a kernel oops during manual sysfs driver binding.
- [Medium] The return value from `regmap_update_bits()` for configuring the global IMAX register is ignored and overwritten.
- [Medium] Incorrect default current configuration for AW2027 assigns a global register enum to a local scale, leading to 0mA output.
- [Low] The inline function `aw20xx_microamp_to_imax` lacks the `static` keyword, creating a non-static inline definition that causes linker errors or compiler warnings.
- [Low] Struct initialization uses a non-constant expression at file scope, causing a hard compiler error.

Pre-existing issues:
- [High] Self-deadlock in `aw20xx_probe()` due to holding `chip->mutex` during LED registration.
- [High] Late assignment of `chip->num_leds` breaks initialization during probe and causes unbalanced regulator disables.
- [Low] Failure to use dev_err_probe() for regulator requests violates LED subsystem guidelines.
--

commit 66ff1fb4c5655ece202f9cdb3c5a3a75b313c787
Author: Paul Sajna <sajattack@postmarketos.org>
Subject: leds: aw2013: Add AW2027 support and rename to aw20xx

This commit adds support for the AW2027 LED driver by renaming the existing
AW2013 driver to aw20xx and abstracting chip-specific features. It introduces
a common chipdef structure to handle differences in maximum current, current
mixing levels, and initialization registers.

> diff --git a/drivers/leds/leds-aw20xx.c b/drivers/leds/leds-aw20xx.c
> index 216755d6010fe..635965b712d48 100644
> --- a/drivers/leds/leds-aw20xx.c
> +++ b/drivers/leds/leds-aw20xx.c

[ ... ]

> +/* finds the closest current step to the given microamp
> + * given the max current and number of levels
> + */
> +inline u32 aw20xx_microamp_to_imax(u32 microamp, const struct aw20xx_chipdef *cdef)
> +{
> +	return min_t(u32, ((microamp * (cdef->current_levels - 1))
> +			  + (cdef->current_max / 2)) / cdef->current_max,
> +		     cdef->current_levels - 1);
> +}

[Severity: Low]
Does this inline function need the static keyword? When compiling with C99/C11
standards, an inline function without static might require an external
definition, which could lead to build failures or linker errors.

[ ... ]

> +static const struct aw20xx_chipdef aw2013_chipdef = {
> +	.chip_id = AW2013_RSTR_CHIP_ID,
> +	.regmap_cfg = aw2013_regmap_config,
> +	.default_imax = 0b01, // 5mA
> +	.gcr2_imax = false,
> +	.current_levels = 4,
> +	.current_max = 15000,
> +};
> +
> +static const struct aw20xx_chipdef aw2027_chipdef = {
> +	.chip_id = AW2027_RSTR_CHIP_ID,
> +	.regmap_cfg = aw2027_regmap_config,

[Severity: Low]
Does this cause a compiler error due to non-constant initialization? Using
the global variable aw2013_regmap_config to initialize this struct by value
at file scope is not a constant expression in standard C. The same applies
below for aw2027_regmap_config.

> +	.default_imax = AW2027_IMAX_15MA,

[Severity: Medium]
Does this assign a global register enum to a property used for the LCFG local
current step? AW2027_IMAX_15MA expands to 0. Writing 0 to the 16-level local
current scale (AW20XX_LCFG_CUR_MASK) configures the LED for 0mA instead of
15mA if the led-max-microamp devicetree property is missing.

[ ... ]

> +	if (chip->cdef->gcr2_imax) {
> +		/* AW2027 supports 4-step global imax, and also a 16-step control for limiting
> +		 * individual current per LED. This doesn't map to the single value the
> +		 * devicetree provides super well, so set global imax  to maximum, and
> +		 * local imax to whatever gets us closest to the value in the devicetree
> +		 */
>  		ret = regmap_update_bits(chip->regmap,
> -					 AW2013_LCFG(chip->leds[i].num),
> -					 AW2013_LCFG_IMAX_MASK,
> -					 chip->leds[i].imax);
> -		if (ret) {
> -			dev_err(&chip->client->dev,
> -				"Failed to set maximum current for led %d: %d\n",
> -				chip->leds[i].num, ret);
> -			return ret;
> +				AW20XX_GCR2,
> +				AW20XX_IMAX_MASK,
> +				AW2027_IMAX_30MA);
> +
> +		for (i = 0; i < chip->num_leds; i++) {
> +			ret = regmap_update_bits(chip->regmap,

[Severity: Medium]
Is the return value for the global IMAX register update intentionally ignored?
The ret variable is immediately overwritten by the loop, meaning any hardware
I2C errors during AW20XX_GCR2 configuration will not be caught.

[ ... ]

> -		led->cdev.brightness_set_blocking = aw2013_brightness_set;
> -		led->cdev.blink_set = aw2013_blink_set;
> +		led->cdev.brightness_set_blocking = aw20xx_brightness_set;
> +		led->cdev.blink_set = aw20xx_blink_set;
>  
>  		ret = devm_led_classdev_register_ext(&chip->client->dev,
>  						     &led->cdev, &init_data);

[Severity: High]
This is a pre-existing issue, but does assigning chip->num_leds after
registering the LED class devices cause initialization problems?
Looking at aw20xx_probe_dt(), chip->num_leds is set to i after the loop
completes. However, devm_led_classdev_register_ext() executes default triggers
synchronously, which call aw20xx_brightness_set(). If num_leds is still 0,
aw20xx_chip_in_use() silently returns false, skipping chip enablement and
causing unbalanced regulator disables while probe is still running.

[ ... ]

>  static int aw20xx_probe(struct i2c_client *client)
>  {
> -	struct aw2013 *chip;
> +	struct aw20xx *chip;
> +	const struct aw20xx_chipdef *cdef;
>  	int ret;
>  	unsigned int chipid;
>  
> +	cdef = device_get_match_data(&client->dev);
> +

[Severity: High]
Does this need a NULL check? If a user with privileges manually binds the
driver to an I2C device via sysfs, device_get_match_data() will return NULL,
causing a kernel oops when it is later dereferenced.

[ ... ]

>  	mutex_lock(&chip->mutex);
>  
>  	chip->client = client;
> +	chip->cdef = cdef;
>  	i2c_set_clientdata(client, chip);
>  
> -	chip->regmap = devm_regmap_init_i2c(client, &aw2013_regmap_config);
> +	chip->regmap = devm_regmap_init_i2c(client, &chip->cdef->regmap_cfg);

[Severity: High]
This is a pre-existing issue, but does holding chip->mutex here cause a
self-deadlock during probe? Later in the probe path, aw20xx_probe_dt() calls
devm_led_classdev_register_ext(). If a default trigger is configured, it
synchronously calls aw20xx_brightness_set() which attempts to acquire the
same chip->mutex lock.

[ ... ]

>  	}
>  
>  	ret = regulator_bulk_enable(ARRAY_SIZE(chip->regulators),
> -				    chip->regulators);
> +					chip->regulators);
>  	if (ret) {

[Severity: Low]
This is a pre-existing issue, but does the regulator request code earlier
in aw20xx_probe() violate the LED subsystem guidelines? It uses manual
ret != -EPROBE_DEFER checks and dev_err() instead of dev_err_probe(), which
is required to correctly handle deferred probes and clean up the kernel log.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260907-aw2013-aw20xx-rename-v1-0-c9a7df12a701@postmarketos.org?part=3

Re: [PATCH RFC 1/3] dt-bindings: leds: aw2013: Add aw2027 compatible and rename to aw20xx

[Conor Dooley]

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

On Mon, Sep 07, 2026 at 04:47:51PM -0700, Paul Sajna wrote:
> Document the awinic,aw2027, which is very similar to aw2013.
> Rename the driver to support all of the 20xx series.
> 
> Signed-off-by: Paul Sajna <sajattack@postmarketos.org>
> ---
>  .../bindings/leds/{leds-aw2013.yaml => leds-aw20xx.yaml}       | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/leds/leds-aw2013.yaml b/Documentation/devicetree/bindings/leds/leds-aw20xx.yaml
> similarity index 91%
> rename from Documentation/devicetree/bindings/leds/leds-aw2013.yaml
> rename to Documentation/devicetree/bindings/leds/leds-aw20xx.yaml
> index 26238446f2bd..f20b5344e3e1 100644
> --- a/Documentation/devicetree/bindings/leds/leds-aw2013.yaml
> +++ b/Documentation/devicetree/bindings/leds/leds-aw20xx.yaml

Please retain a filename matching a compatible. IOW, leave this alone.
pw-bot: changes-requested

The rest of this is fine, but please note in your commit message what
makes this device different to the existing one.

Thanks,
Conor.

> @@ -1,21 +1,23 @@
>  # SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>  %YAML 1.2
>  ---
> -$id: http://devicetree.org/schemas/leds/leds-aw2013.yaml#
> +$id: http://devicetree.org/schemas/leds/leds-aw20xx.yaml#
>  $schema: http://devicetree.org/meta-schemas/core.yaml#
>  
> -title: AWINIC AW2013 3-channel LED Driver
> +title: AWINIC AW20XX 3-channel LED Driver
>  
>  maintainers:
>    - Nikita Travkin <nikitos.tr@gmail.com>
>  
>  description: |
> -  The AW2013 is a 3-channel LED driver with I2C interface. It can control
> +  The AW20XX is a 3-channel LED driver with I2C interface. It can control
>    LED brightness with PWM output.
>  
>  properties:
>    compatible:
> -    const: awinic,aw2013
> +    enum:
> +      - awinic,aw2013
> +      - awinic,aw2027
>  
>    reg:
>      maxItems: 1
> 
> -- 
> 2.55.0
> 

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

Re: [PATCH RFC 3/3] leds: aw2013: Add AW2027 support and rename to aw20xx

[Lee Jones]

On Tue, 08 Sep 2026, sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
> 
> New issues:
> - [High] Missing NULL check for the result of `device_get_match_data()`, which leads to a kernel oops during manual sysfs driver binding.
> - [Medium] The return value from `regmap_update_bits()` for configuring the global IMAX register is ignored and overwritten.
> - [Medium] Incorrect default current configuration for AW2027 assigns a global register enum to a local scale, leading to 0mA output.
> - [Low] The inline function `aw20xx_microamp_to_imax` lacks the `static` keyword, creating a non-static inline definition that causes linker errors or compiler warnings.
> - [Low] Struct initialization uses a non-constant expression at file scope, causing a hard compiler error.

Please review, explain or fix the Sashiko review(s).

If this is a set, this request includes the other patches too.

> Pre-existing issues:
> - [High] Self-deadlock in `aw20xx_probe()` due to holding `chip->mutex` during LED registration.
> - [High] Late assignment of `chip->num_leds` breaks initialization during probe and causes unbalanced regulator disables.
> - [Low] Failure to use dev_err_probe() for regulator requests violates LED subsystem guidelines.
> --
> 
> commit 66ff1fb4c5655ece202f9cdb3c5a3a75b313c787
> Author: Paul Sajna <sajattack@postmarketos.org>
> Subject: leds: aw2013: Add AW2027 support and rename to aw20xx
> 
> This commit adds support for the AW2027 LED driver by renaming the existing
> AW2013 driver to aw20xx and abstracting chip-specific features. It introduces
> a common chipdef structure to handle differences in maximum current, current
> mixing levels, and initialization registers.
> 
> > diff --git a/drivers/leds/leds-aw20xx.c b/drivers/leds/leds-aw20xx.c
> > index 216755d6010fe..635965b712d48 100644
> > --- a/drivers/leds/leds-aw20xx.c
> > +++ b/drivers/leds/leds-aw20xx.c
> 
> [ ... ]
> 
> > +/* finds the closest current step to the given microamp
> > + * given the max current and number of levels
> > + */
> > +inline u32 aw20xx_microamp_to_imax(u32 microamp, const struct aw20xx_chipdef *cdef)
> > +{
> > +	return min_t(u32, ((microamp * (cdef->current_levels - 1))
> > +			  + (cdef->current_max / 2)) / cdef->current_max,
> > +		     cdef->current_levels - 1);
> > +}
> 
> [Severity: Low]
> Does this inline function need the static keyword? When compiling with C99/C11
> standards, an inline function without static might require an external
> definition, which could lead to build failures or linker errors.
> 
> [ ... ]
> 
> > +static const struct aw20xx_chipdef aw2013_chipdef = {
> > +	.chip_id = AW2013_RSTR_CHIP_ID,
> > +	.regmap_cfg = aw2013_regmap_config,
> > +	.default_imax = 0b01, // 5mA
> > +	.gcr2_imax = false,
> > +	.current_levels = 4,
> > +	.current_max = 15000,
> > +};
> > +
> > +static const struct aw20xx_chipdef aw2027_chipdef = {
> > +	.chip_id = AW2027_RSTR_CHIP_ID,
> > +	.regmap_cfg = aw2027_regmap_config,
> 
> [Severity: Low]
> Does this cause a compiler error due to non-constant initialization? Using
> the global variable aw2013_regmap_config to initialize this struct by value
> at file scope is not a constant expression in standard C. The same applies
> below for aw2027_regmap_config.
> 
> > +	.default_imax = AW2027_IMAX_15MA,
> 
> [Severity: Medium]
> Does this assign a global register enum to a property used for the LCFG local
> current step? AW2027_IMAX_15MA expands to 0. Writing 0 to the 16-level local
> current scale (AW20XX_LCFG_CUR_MASK) configures the LED for 0mA instead of
> 15mA if the led-max-microamp devicetree property is missing.
> 
> [ ... ]
> 
> > +	if (chip->cdef->gcr2_imax) {
> > +		/* AW2027 supports 4-step global imax, and also a 16-step control for limiting
> > +		 * individual current per LED. This doesn't map to the single value the
> > +		 * devicetree provides super well, so set global imax  to maximum, and
> > +		 * local imax to whatever gets us closest to the value in the devicetree
> > +		 */
> >  		ret = regmap_update_bits(chip->regmap,
> > -					 AW2013_LCFG(chip->leds[i].num),
> > -					 AW2013_LCFG_IMAX_MASK,
> > -					 chip->leds[i].imax);
> > -		if (ret) {
> > -			dev_err(&chip->client->dev,
> > -				"Failed to set maximum current for led %d: %d\n",
> > -				chip->leds[i].num, ret);
> > -			return ret;
> > +				AW20XX_GCR2,
> > +				AW20XX_IMAX_MASK,
> > +				AW2027_IMAX_30MA);
> > +
> > +		for (i = 0; i < chip->num_leds; i++) {
> > +			ret = regmap_update_bits(chip->regmap,
> 
> [Severity: Medium]
> Is the return value for the global IMAX register update intentionally ignored?
> The ret variable is immediately overwritten by the loop, meaning any hardware
> I2C errors during AW20XX_GCR2 configuration will not be caught.
> 
> [ ... ]
> 
> > -		led->cdev.brightness_set_blocking = aw2013_brightness_set;
> > -		led->cdev.blink_set = aw2013_blink_set;
> > +		led->cdev.brightness_set_blocking = aw20xx_brightness_set;
> > +		led->cdev.blink_set = aw20xx_blink_set;
> >  
> >  		ret = devm_led_classdev_register_ext(&chip->client->dev,
> >  						     &led->cdev, &init_data);
> 
> [Severity: High]
> This is a pre-existing issue, but does assigning chip->num_leds after
> registering the LED class devices cause initialization problems?
> Looking at aw20xx_probe_dt(), chip->num_leds is set to i after the loop
> completes. However, devm_led_classdev_register_ext() executes default triggers
> synchronously, which call aw20xx_brightness_set(). If num_leds is still 0,
> aw20xx_chip_in_use() silently returns false, skipping chip enablement and
> causing unbalanced regulator disables while probe is still running.
> 
> [ ... ]
> 
> >  static int aw20xx_probe(struct i2c_client *client)
> >  {
> > -	struct aw2013 *chip;
> > +	struct aw20xx *chip;
> > +	const struct aw20xx_chipdef *cdef;
> >  	int ret;
> >  	unsigned int chipid;
> >  
> > +	cdef = device_get_match_data(&client->dev);
> > +
> 
> [Severity: High]
> Does this need a NULL check? If a user with privileges manually binds the
> driver to an I2C device via sysfs, device_get_match_data() will return NULL,
> causing a kernel oops when it is later dereferenced.
> 
> [ ... ]
> 
> >  	mutex_lock(&chip->mutex);
> >  
> >  	chip->client = client;
> > +	chip->cdef = cdef;
> >  	i2c_set_clientdata(client, chip);
> >  
> > -	chip->regmap = devm_regmap_init_i2c(client, &aw2013_regmap_config);
> > +	chip->regmap = devm_regmap_init_i2c(client, &chip->cdef->regmap_cfg);
> 
> [Severity: High]
> This is a pre-existing issue, but does holding chip->mutex here cause a
> self-deadlock during probe? Later in the probe path, aw20xx_probe_dt() calls
> devm_led_classdev_register_ext(). If a default trigger is configured, it
> synchronously calls aw20xx_brightness_set() which attempts to acquire the
> same chip->mutex lock.
> 
> [ ... ]
> 
> >  	}
> >  
> >  	ret = regulator_bulk_enable(ARRAY_SIZE(chip->regulators),
> > -				    chip->regulators);
> > +					chip->regulators);
> >  	if (ret) {
> 
> [Severity: Low]
> This is a pre-existing issue, but does the regulator request code earlier
> in aw20xx_probe() violate the LED subsystem guidelines? It uses manual
> ret != -EPROBE_DEFER checks and dev_err() instead of dev_err_probe(), which
> is required to correctly handle deferred probes and clean up the kernel log.
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260907-aw2013-aw20xx-rename-v1-0-c9a7df12a701@postmarketos.org?part=3
> 

-- 
Lee Jones