[PATCH v7 0/4] leds: aw2013: Add AW2027 support

[PATCH v7 0/4] leds: aw2013: Add AW2027 support

[Paul Sajna]

Refer to PATCH 4 for a full description of the similarities and
differences between AW2013 and AW2027.

I'm hoping the sashiko reviews about unbalanced regulators that I was
struggling to fix are covered by
https://lore.kernel.org/linux-leds/20261021123406.3163547-1-lee@kernel.org/
and I can drop my failed attempts to fix them.
Let me know if I misunderstood.

I'll do my best to address all of Sashiko's comments, but it might take
me some time to circle back, don't bother reviewing until I've done
this.

This PATCH 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>
---
Changes in v7:
- Fix multiple imax setting/regulators per Sashiko
- Small description updates to AW20XX per Heidelberg
- Reorder dev_err_probe commit so that introducing new code using
  dev_err_probe is cleaner
- Simplify dev_err_probe, prevent unneccessary assignment
- Revert early return / else suggested by Heidelberg, Travkin and I both
  prefer else
- Lower Hex
- split c99 for-loops to separate commit
- simplify aw2027_set_local_imax and aw2027_set_global_imax
- Link to v6: https://patch.msgid.link/20261112-aw2013-aw20xx-rename-v6-0-251eca340abc@postmarketos.org

Changes in v6:
- Fix the compile error introduced by my unchecked hasty rebase
- Some adjustments per David Heidelberg's comments
- Link to v5: https://patch.msgid.link/20261112-aw2013-aw20xx-rename-v5-0-b2c5e323a90a@postmarketos.org

Changes in v5:
- Drop move of chip->num_leds
- Make aw2013_microamp_to_imax more readable
- rebase to next-20261112 for Lee's changes
- Link to v4: https://lore.kernel.org/linux-leds/20261108-aw2013-aw20xx-rename-v4-0-826529beeb52@postmarketos.org/

Changes in v4:
- Drop sashiko fixes covered by https://lore.kernel.org/linux-leds/20261021123406.3163547-1-lee@kernel.org/
- Rename the chip id register value to reflect that it covers more chips
  (https://www.awinic.com/en/driver/AW2027DNR/640)
- Address preliminary review comments from Lee Jones
- Link to v3: https://lore.kernel.org/r/20261012-aw2013-aw20xx-rename-v3-0-cab1fdd050b6@postmarketos.org

Changes in v3:
- Drop rename
- Capitalize commit subject
- Extra tweaks to cleanup patches (preventing more unbalanced regulator
  stuff)
- Whitespace fiddling
- Link to v2: https://lore.kernel.org/r/20261005-aw2013-aw20xx-rename-v2-0-108ecbdf2775@postmarketos.org

Changes in v2:
- Reduce renames to internal driver scope, to prevent breaking compat
- Address Sashiko Review Comments
- Split renames into a separate commit to make the actual changes easier
to identify
- Change from RFC to PATCH
- Link to v1: https://lore.kernel.org/r/20260907-aw2013-aw20xx-rename-v1-0-c9a7df12a701@postmarketos.org

To: Lee Jones <lee@kernel.org>
To: Pavel Machek <pavel@kernel.org>
To: Griffin Kroah-Hartman <griffin@kroah.com>
To: Rob Herring <robh@kernel.org>
To: Krzysztof Kozlowski <krzk+dt@kernel.org>
To: Conor Dooley <conor+dt@kernel.org>
To: Nikita Travkin <nikitos.tr@gmail.com>
Cc: linux-leds@vger.kernel.org
Cc: devicetree@vger.kernel.org
Cc: linux-kernel@vger.kernel.org

---
Paul Sajna (4):
      dt-bindings: leds: aw2013: Add awinic,aw2027 compatible
      leds: aw2013: Use dev_err_probe
      drivers: leds: aw2013: Leverage C99 for-loops
      leds: aw2013: Add AW2027 support

 .../devicetree/bindings/leds/leds-aw2013.yaml      |   6 +-
 drivers/leds/leds-aw2013.c                         | 240 ++++++++++++++++-----
 2 files changed, 194 insertions(+), 52 deletions(-)
---
base-commit: 6d582d78fad9d828e575ca0cdcff8ea47a77220b
change-id: 20260907-aw2013-aw20xx-rename-df233c5921c6

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

[PATCH v7 1/4] dt-bindings: leds: aw2013: Add awinic,aw2027 compatible

[Paul Sajna]

The aw2027 is largely the same as the aw2013, with the
exception of the way the max current is written to registers.
This exception makes it different enough to not work without
driver modifications, but similar enough to fall under the
umbrella of the same driver.

Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>
Signed-off-by: Paul Sajna <sajattack@postmarketos.org>
---
 Documentation/devicetree/bindings/leds/leds-aw2013.yaml | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/leds/leds-aw2013.yaml b/Documentation/devicetree/bindings/leds/leds-aw2013.yaml
index 26238446f2bd..1ad165cfe34a 100644
--- a/Documentation/devicetree/bindings/leds/leds-aw2013.yaml
+++ b/Documentation/devicetree/bindings/leds/leds-aw2013.yaml
@@ -4,7 +4,7 @@
 $id: http://devicetree.org/schemas/leds/leds-aw2013.yaml#
 $schema: http://devicetree.org/meta-schemas/core.yaml#
 
-title: AWINIC AW2013 3-channel LED Driver
+title: AWINIC AW2013/AW2027 3-channel LED Driver
 
 maintainers:
   - Nikita Travkin <nikitos.tr@gmail.com>
@@ -15,7 +15,9 @@ description: |
 
 properties:
   compatible:
-    const: awinic,aw2013
+    enum:
+      - awinic,aw2013
+      - awinic,aw2027
 
   reg:
     maxItems: 1

-- 
2.56.1

[PATCH v7 2/4] leds: aw2013: Use dev_err_probe

[Paul Sajna]

Failure to use dev_err_probe() for regulator requests violates
LED subsystem guidelines. (At least according to sashiko [1])

The original driver didn't use it, but it's worth cleaning up
while I'm here.

[1]: https://github.com/sashiko-dev/sashiko/blob/main/third_party/prompts/kernel/subsystem/leds.md?plain=1#L71

Reviewed-by: Griffin Kroah-Hartman <griffin@kroah.com>
Reviewed-by: David Heidelberg <david@ixit.cz>

Signed-off-by: Paul Sajna <sajattack@postmarketos.org>
---
 drivers/leds/leds-aw2013.c | 30 ++++++++++++++----------------
 1 file changed, 14 insertions(+), 16 deletions(-)

diff --git a/drivers/leds/leds-aw2013.c b/drivers/leds/leds-aw2013.c
index cf20e94920fd..2937a0f92d92 100644
--- a/drivers/leds/leds-aw2013.c
+++ b/drivers/leds/leds-aw2013.c
@@ -361,9 +361,8 @@ static int aw2013_probe(struct i2c_client *client)
 
 	chip->regmap = devm_regmap_init_i2c(client, &aw2013_regmap_config);
 	if (IS_ERR(chip->regmap)) {
-		ret = PTR_ERR(chip->regmap);
-		dev_err(&client->dev, "Failed to allocate register map: %d\n",
-			ret);
+		ret = dev_err_probe(&client->dev, PTR_ERR(chip->regmap),
+				    "Failed to allocate register map\n");
 		goto error;
 	}
 
@@ -373,45 +372,44 @@ static int aw2013_probe(struct i2c_client *client)
 				      ARRAY_SIZE(chip->regulators),
 				      chip->regulators);
 	if (ret < 0) {
-		if (ret != -EPROBE_DEFER)
-			dev_err(&client->dev,
-				"Failed to request regulators: %d\n", ret);
+		dev_err_probe(&client->dev, ret,
+			      "Failed to request regulators\n");
 		goto error;
 	}
 
 	ret = regulator_bulk_enable(ARRAY_SIZE(chip->regulators),
 				    chip->regulators);
 	if (ret) {
-		dev_err(&client->dev,
-			"Failed to enable regulators: %d\n", ret);
+		dev_err_probe(&client->dev, ret,
+			      "Failed to enable regulators\n");
 		goto error;
 	}
 
 	ret = regmap_read(chip->regmap, AW2013_RSTR, &chipid);
 	if (ret) {
-		dev_err(&client->dev, "Failed to read chip ID: %d\n",
-			ret);
+		dev_err_probe(&client->dev, ret,
+			      "Failed to read chip ID\n");
 		goto error_reg;
 	}
 
 	if (chipid != AW2013_RSTR_CHIP_ID) {
-		dev_err(&client->dev, "Chip reported wrong ID: %x\n",
-			chipid);
-		ret = -ENODEV;
+		ret = dev_err_probe(&client->dev, -ENODEV,
+				    "Chip reported wrong ID: %x\n",
+				     chipid);
 		goto error_reg;
 	}
 
 	ret = regmap_write(chip->regmap, AW2013_RSTR, AW2013_RSTR_RESET);
 	if (ret) {
-		dev_err(&client->dev, "Failed to reset chip: %d\n", ret);
+		dev_err_probe(&client->dev, ret, "Failed to reset chip\n");
 		goto error_reg;
 	}
 
 	ret = regulator_bulk_disable(ARRAY_SIZE(chip->regulators),
 				     chip->regulators);
 	if (ret) {
-		dev_err(&client->dev,
-			"Failed to disable regulators: %d\n", ret);
+		dev_err_probe(&client->dev, ret,
+			      "Failed to disable regulators\n");
 		goto error;
 	}
 

-- 
2.56.1

[PATCH v7 3/4] drivers: leds: aw2013: Leverage C99 for-loops

[Paul Sajna]

Declare the int within the for-loop declaration rather than as a
separate local variable. To make the code more readable.

Signed-off-by: Paul Sajna <sajattack@postmarketos.org>
---
 drivers/leds/leds-aw2013.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/leds/leds-aw2013.c b/drivers/leds/leds-aw2013.c
index 2937a0f92d92..7fe7e9dae903 100644
--- a/drivers/leds/leds-aw2013.c
+++ b/drivers/leds/leds-aw2013.c
@@ -72,7 +72,7 @@ struct aw2013 {
 
 static int aw2013_chip_init(struct aw2013 *chip)
 {
-	int i, ret;
+	int ret;
 
 	ret = regmap_write(chip->regmap, AW2013_GCR, AW2013_GCR_ENABLE);
 	if (ret) {
@@ -81,7 +81,7 @@ static int aw2013_chip_init(struct aw2013 *chip)
 		return ret;
 	}
 
-	for (i = 0; i < chip->num_leds; i++) {
+	for (int i = 0; i < chip->num_leds; i++) {
 		ret = regmap_update_bits(chip->regmap,
 					 AW2013_LCFG(chip->leds[i].num),
 					 AW2013_LCFG_IMAX_MASK,
@@ -142,9 +142,7 @@ static int aw2013_chip_enable(struct aw2013 *chip)
 
 static bool aw2013_chip_in_use(struct aw2013 *chip)
 {
-	int i;
-
-	for (i = 0; i < chip->num_leds; i++)
+	for (int i = 0; i < chip->num_leds; i++)
 		if (chip->leds[i].cdev.brightness)
 			return true;
 

-- 
2.56.1

[PATCH v7 4/4] leds: aw2013: Add AW2027 support

[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-aw2013.c | 206 ++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 175 insertions(+), 31 deletions(-)

diff --git a/drivers/leds/leds-aw2013.c b/drivers/leds/leds-aw2013.c
index 7fe7e9dae903..f62f977e2bfe 100644
--- a/drivers/leds/leds-aw2013.c
+++ b/drivers/leds/leds-aw2013.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>
@@ -20,13 +20,15 @@
 #define AW2013_GCR 0x01
 #define AW2013_GCR_ENABLE BIT(0)
 
+#define AW20XX_GCR2 0x04
+
 /* LED channel enable register */
 #define AW2013_LCTR 0x30
 #define AW2013_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_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)
@@ -51,6 +53,16 @@
 
 #define AW2013_TIME_STEP 130 /* ms */
 
+/* AW2027-specific defines */
+#define AW2016_23_27_33_CHIPID 0x09 /* vendor shared same chipid between several chips */
+#define AW2027_REG_MAX 0x7f /* copied from vendor driver, but only up to 0x3F is documented */
+#define AW2027_LOCAL_IMAX_MASK GENMASK(3, 0)
+
+#define AW2027_GLOBAL_IMAX_15MA 0
+#define AW2027_GLOBAL_IMAX_30MA 1
+#define AW2027_GLOBAL_IMAX_5MA  2
+#define AW2027_GLOBAL_IMAX_10MA 3
+
 struct aw2013;
 
 struct aw2013_led {
@@ -60,16 +72,88 @@ struct aw2013_led {
 	unsigned int imax;
 };
 
+struct aw2013_chipdef {
+	u8 chip_id;
+	const struct regmap_config *regmap_cfg;
+	u32 default_imax;
+	bool gcr2_imax;
+	unsigned int current_levels;
+	unsigned int current_max_ua;
+};
+
 struct aw2013 {
 	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;
+	const struct aw2013_chipdef *cdef;
 	int num_leds;
 	bool enabled;
 };
 
+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,
+};
+
+static const struct aw2013_chipdef aw2013_chipdef = {
+	.chip_id = AW2013_RSTR_CHIP_ID,
+	.regmap_cfg = &aw2013_regmap_config,
+	.default_imax = 1, /* 5mA */
+	.gcr2_imax = false,
+	.current_levels = 4,
+	.current_max_ua = 15000,
+};
+
+static const struct aw2013_chipdef aw2027_chipdef = {
+	.chip_id = AW2016_23_27_33_CHIPID,
+	.regmap_cfg = &aw2027_regmap_config,
+	.default_imax = 3, /* 6mA */
+	.gcr2_imax = true,
+	.current_levels = 16,
+	.current_max_ua = 30000,
+};
+
+/**
+ * aw2013_microamp_to_imax() - Finds the closest current step to the given microamp
+ * @microamp: Desired current in microamps
+ * @cdef: Chip definition
+ *
+ * Return: The closest current-step to the given microamp, rounded down
+ * and limited by the max current step possible for the chip.
+ */
+static inline u32 aw2013_microamp_to_imax(u32 microamp, const struct aw2013_chipdef *cdef)
+{
+	u32 max_current_step = cdef->current_levels - 1;
+	u32 closest_current_step_floor = (microamp * max_current_step) / cdef->current_max_ua;
+
+	return min_t(u32, closest_current_step_floor, max_current_step);
+}
+
+static int aw2027_set_global_imax(struct aw2013 *chip)
+{
+	return regmap_update_bits(chip->regmap,
+				 AW20XX_GCR2,
+				 AW2013_IMAX_MASK,
+				 AW2027_GLOBAL_IMAX_30MA);
+}
+
+static int aw2027_set_local_imax(struct aw2013 *chip, struct aw2013_led *led, u32 imax)
+{
+	return regmap_update_bits(chip->regmap,
+				 AW2013_LCFG(led->num),
+				 AW2027_LOCAL_IMAX_MASK,
+				 imax);
+}
+
 static int aw2013_chip_init(struct aw2013 *chip)
 {
 	int ret;
@@ -81,17 +165,47 @@ static int aw2013_chip_init(struct aw2013 *chip)
 		return ret;
 	}
 
-	for (int i = 0; i < chip->num_leds; i++) {
-		ret = regmap_update_bits(chip->regmap,
-					 AW2013_LCFG(chip->leds[i].num),
-					 AW2013_LCFG_IMAX_MASK,
-					 chip->leds[i].imax);
+	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 = aw2027_set_global_imax(chip);
 		if (ret) {
 			dev_err(&chip->client->dev,
-				"Failed to set maximum current for led %d: %d\n",
-				chip->leds[i].num, ret);
+				"Failed to set maximum global current: %d\n",
+				ret);
 			return ret;
 		}
+		/*
+		 * This still needs to be here despite also being in aw2013_probe_dt,
+		 * for cases where chip is disabled and re-enabled.
+		 */
+		for (int i = 0; i < chip->num_leds; i++) {
+			ret = aw2027_set_local_imax(chip, &chip->leds[i], 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 (int i = 0; i < chip->num_leds; i++) {
+			ret = regmap_update_bits(chip->regmap,
+						 AW2013_LCFG(chip->leds[i].num),
+						 AW2013_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;
@@ -291,9 +405,9 @@ 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 = aw2013_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");
 		}
@@ -303,14 +417,30 @@ static int aw2013_probe_dt(struct aw2013 *chip)
 
 		mutex_lock(&chip->mutex);
 		chip->num_leds = ++i;
-		if (chip->enabled) {
+		/*
+		 * needs to be here and in aw2013_chip_init because chip->num_leds increments
+		 * just above here while we're parsing the devicetree, and chip->enabled
+		 * ensures chip_init runs as little as possible.
+		 * Requires this function to have regulators enabled.
+		 */
+		if (chip->cdef->gcr2_imax) {
+			ret = aw2027_set_local_imax(chip, led, led->imax);
+			if (ret) {
+				mutex_unlock(&chip->mutex);
+				return dev_err_probe(&chip->client->dev, ret,
+						     "Failed to set maximum current for led %d\n",
+						     led->num);
+			}
+		} else {
 			ret = regmap_update_bits(chip->regmap,
 						 AW2013_LCFG(led->num),
-						 AW2013_LCFG_IMAX_MASK,
+						 AW2013_IMAX_MASK,
 						 led->imax);
 			if (ret) {
 				mutex_unlock(&chip->mutex);
-				return ret;
+				return dev_err_probe(&chip->client->dev, ret,
+						     "Failed to set maximum current for led %d\n",
+						     led->num);
 			}
 		}
 		mutex_unlock(&chip->mutex);
@@ -332,18 +462,18 @@ static void aw2013_chip_disable_action(void *data)
 	aw2013_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)
 {
 	struct aw2013 *chip;
+	const struct aw2013_chipdef *cdef;
 	int ret;
 	unsigned int chipid;
 
+	cdef = device_get_match_data(&client->dev);
+	if (!cdef)
+		return -ENODEV;
+
 	chip = devm_kzalloc_obj(&client->dev, *chip);
 	if (!chip)
 		return -ENOMEM;
@@ -355,9 +485,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 = dev_err_probe(&client->dev, PTR_ERR(chip->regmap),
 				    "Failed to allocate register map\n");
@@ -390,7 +521,7 @@ static int aw2013_probe(struct i2c_client *client)
 		goto error_reg;
 	}
 
-	if (chipid != AW2013_RSTR_CHIP_ID) {
+	if (chipid != chip->cdef->chip_id) {
 		ret = dev_err_probe(&client->dev, -ENODEV,
 				    "Chip reported wrong ID: %x\n",
 				     chipid);
@@ -403,21 +534,28 @@ static int aw2013_probe(struct i2c_client *client)
 		goto error_reg;
 	}
 
+	mutex_unlock(&chip->mutex);
+
+	ret = devm_add_action(&client->dev, aw2013_chip_disable_action, chip);
+	if (ret)
+		goto error_reg_no_mutex;
+
+	ret = aw2013_probe_dt(chip);
+	if (ret) {
+		dev_err_probe(&client->dev, ret,
+			      "Failed to parse devicetree\n");
+		goto error_reg_no_mutex;
+	}
+
 	ret = regulator_bulk_disable(ARRAY_SIZE(chip->regulators),
 				     chip->regulators);
 	if (ret) {
 		dev_err_probe(&client->dev, ret,
 			      "Failed to disable regulators\n");
-		goto error;
-	}
-
-	mutex_unlock(&chip->mutex);
-
-	ret = devm_add_action(&client->dev, aw2013_chip_disable_action, chip);
-	if (ret)
 		return ret;
+	}
 
-	return aw2013_probe_dt(chip);
+	return 0;
 
 error_reg:
 	regulator_bulk_disable(ARRAY_SIZE(chip->regulators),
@@ -426,10 +564,16 @@ static int aw2013_probe(struct i2c_client *client)
 error:
 	mutex_unlock(&chip->mutex);
 	return ret;
+
+error_reg_no_mutex:
+	regulator_bulk_disable(ARRAY_SIZE(chip->regulators),
+			       chip->regulators);
+	return ret;
 }
 
 static const struct of_device_id aw2013_match_table[] = {
-	{ .compatible = "awinic,aw2013", },
+	{ .compatible = "awinic,aw2013", .data = &aw2013_chipdef },
+	{ .compatible = "awinic,aw2027", .data = &aw2027_chipdef },
 	{ /* sentinel */ },
 };
 
@@ -446,5 +590,5 @@ static struct i2c_driver aw2013_driver = {
 module_i2c_driver(aw2013_driver);
 
 MODULE_AUTHOR("Nikita Travkin <nikitos.tr@gmail.com>");
-MODULE_DESCRIPTION("AW2013 LED driver");
+MODULE_DESCRIPTION("AW20XX LED driver");
 MODULE_LICENSE("GPL v2");

-- 
2.56.1

Re: [PATCH v7 3/4] drivers: leds: aw2013: Leverage C99 for-loops

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Low] leds: aw2013: commit subject uses drivers: leds: prefix instead
  of leds:
--

commit dd420770c3e97b4e4988e32c3e735789959ed94e
Author: Paul Sajna <sajattack@postmarketos.org>

drivers: leds: aw2013: Leverage C99 for-loops

This commit moves loop counter declarations into the for-loop headers in
aw2013_chip_init() and aw2013_chip_in_use().

> drivers: leds: aw2013: Leverage C99 for-loops

[Severity: Low]
This isn't a bug, but LED subsystem commit subjects use the "leds:
<driver>: <Capitalized description>" format, so this should use the
"leds: aw2013:" prefix instead of "drivers: leds: aw2013:".

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

Re: [PATCH v7 3/4] drivers: leds: aw2013: Leverage C99 for-loops

[Paul Sajna]

On 2026-11-14 4:00 p.m., sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue to consider:
> - [Low] leds: aw2013: commit subject uses drivers: leds: prefix instead
>    of leds:
> --
> 
> commit dd420770c3e97b4e4988e32c3e735789959ed94e
> Author: Paul Sajna <sajattack@postmarketos.org>
> 
> drivers: leds: aw2013: Leverage C99 for-loops
> 
> This commit moves loop counter declarations into the for-loop headers in
> aw2013_chip_init() and aw2013_chip_in_use().
> 
>> drivers: leds: aw2013: Leverage C99 for-loops
> 
> [Severity: Low]
> This isn't a bug, but LED subsystem commit subjects use the "leds:
> <driver>: <Capitalized description>" format, so this should use the
> "leds: aw2013:" prefix instead of "drivers: leds: aw2013:".
> 

Acknowledged, must've wrote this commit msg on autopilot. I'll fix in v8 
but wait and see if there are any more pressing concerns before re-send.

---
Hal: They're going for the kernel!
Margo: Colonel Who?

Re: [PATCH v7 4/4] leds: aw2013: Add AW2027 support

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Low] leds: aw2013: duplicated per-LED imax update in
  aw2013_chip_init()
--

commit fe70881eccdb317dc7ce37e26b9965b979fe6113
Author: Paul Sajna <sajattack@postmarketos.org>

leds: aw2013: Add AW2027 support

This commit adds support for the Awinic AW2027 3-channel LED controller to the
aw2013 driver, introducing chip-specific definitions and handling for its
global and per-channel current limits.

> diff --git a/drivers/leds/leds-aw2013.c b/drivers/leds/leds-aw2013.c
> index 7fe7e9dae9031..f62f977e2bfef 100644
> --- a/drivers/leds/leds-aw2013.c
> +++ b/drivers/leds/leds-aw2013.c

[ ... ]

> @@ -60,16 +72,88 @@ struct aw2013_led {

[ ... ]

> +static int aw2027_set_local_imax(struct aw2013 *chip, struct aw2013_led *led, u32 imax)
> +{
> +	return regmap_update_bits(chip->regmap,
> +				 AW2013_LCFG(led->num),
> +				 AW2027_LOCAL_IMAX_MASK,
> +				 imax);
> +}
> +
>  static int aw2013_chip_init(struct aw2013 *chip)
>  {
>  	int ret;
> @@ -81,17 +165,47 @@ static int aw2013_chip_init(struct aw2013 *chip)
>  		return ret;
>  	}
>  
> -	for (int i = 0; i < chip->num_leds; i++) {
> -		ret = regmap_update_bits(chip->regmap,
> -					 AW2013_LCFG(chip->leds[i].num),
> -					 AW2013_LCFG_IMAX_MASK,
> -					 chip->leds[i].imax);
> +	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 = aw2027_set_global_imax(chip);
>  		if (ret) {
>  			dev_err(&chip->client->dev,
> -				"Failed to set maximum current for led %d: %d\n",
> -				chip->leds[i].num, ret);
> +				"Failed to set maximum global current: %d\n",
> +				ret);
>  			return ret;
>  		}
> +		/*
> +		 * This still needs to be here despite also being in aw2013_probe_dt,
> +		 * for cases where chip is disabled and re-enabled.
> +		 */
> +		for (int i = 0; i < chip->num_leds; i++) {
> +			ret = aw2027_set_local_imax(chip, &chip->leds[i], 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 (int i = 0; i < chip->num_leds; i++) {
> +			ret = regmap_update_bits(chip->regmap,
> +						 AW2013_LCFG(chip->leds[i].num),
> +						 AW2013_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;
> +			}
> +		}
>  	}

[ ... ]

> @@ -303,14 +417,30 @@ static int aw2013_probe_dt(struct aw2013 *chip)
>  
>  		mutex_lock(&chip->mutex);
>  		chip->num_leds = ++i;
> -		if (chip->enabled) {
> +		/*
> +		 * needs to be here and in aw2013_chip_init because chip->num_leds increments
> +		 * just above here while we're parsing the devicetree, and chip->enabled
> +		 * ensures chip_init runs as little as possible.
> +		 * Requires this function to have regulators enabled.
> +		 */
> +		if (chip->cdef->gcr2_imax) {
> +			ret = aw2027_set_local_imax(chip, led, led->imax);
> +			if (ret) {
> +				mutex_unlock(&chip->mutex);
> +				return dev_err_probe(&chip->client->dev, ret,
> +						     "Failed to set maximum current for led %d\n",
> +						     led->num);
> +			}
> +		} else {
>  			ret = regmap_update_bits(chip->regmap,
>  						 AW2013_LCFG(led->num),
> -						 AW2013_LCFG_IMAX_MASK,
> +						 AW2013_IMAX_MASK,
>  						 led->imax);
>  			if (ret) {
>  				mutex_unlock(&chip->mutex);
> -				return ret;
> +				return dev_err_probe(&chip->client->dev, ret,
> +						     "Failed to set maximum current for led %d\n",
> +						     led->num);
>  			}
>  		}

[Severity: Low]
As reported by Lee Jones on v3 [1], why duplicate the per-LED current update
and error handling across the gcr2_imax branches in both aw2013_chip_init()
and aw2013_probe_dt()?

Both AW2027 and AW2013 update AW2013_LCFG(led->num) with led->imax via
regmap_update_bits() and differ only in the bitmask (AW2027_LOCAL_IMAX_MASK vs
AW2013_IMAX_MASK, i.e. chip->cdef->current_levels - 1), so a single common
loop in aw2013_chip_init() and a single update call in aw2013_probe_dt() can
handle both chips.

[1] https://lore.kernel.org/r/179198402838.839288.17911568760182808689@kernel.org

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261114-aw2013-aw20xx-rename-v7-0-ea5a3113a479@postmarketos.org?part=4

Re: [PATCH v7 4/4] leds: aw2013: Add AW2027 support

[Paul Sajna]

On 2026-11-14 4:20 p.m., sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue to consider:
> - [Low] leds: aw2013: duplicated per-LED imax update in
>    aw2013_chip_init()
> --

> [Severity: Low]
> As reported by Lee Jones on v3 [1], why duplicate the per-LED current update
> and error handling across the gcr2_imax branches in both aw2013_chip_init()
> and aw2013_probe_dt()?
> 
> Both AW2027 and AW2013 update AW2013_LCFG(led->num) with led->imax via
> regmap_update_bits() and differ only in the bitmask (AW2027_LOCAL_IMAX_MASK vs
> AW2013_IMAX_MASK, i.e. chip->cdef->current_levels - 1), so a single common
> loop in aw2013_chip_init() and a single update call in aw2013_probe_dt() can
> handle both chips.
> 
> [1] https://lore.kernel.org/r/179198402838.839288.17911568760182808689@kernel.org
> 

The reasoning is explained in the code comments, I tried to remove it, 
but it turns out it needs both. aw2013_chip_init serves to re-enable the 
chip after all leds have been shut off. aw2013_probe_dt increments 
chip->num_leds during probe, and chip_init will not be called after 
subsequent leds are discovered, because chip->enabled serves to limit 
calls of chip_init

-- 
Hal: They're going for the kernel!
Margo: Colonel Who?