[PATCH v6 0/3] leds: aw2013: Add AW2027 support

[PATCH v6 0/3] leds: aw2013: Add AW2027 support

[Paul Sajna via B4 Relay]

Refer to PATCH 2 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 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 (3):
      dt-bindings: leds: aw2013: Add awinic,aw2027 compatible
      leds: aw2013: Add AW2027 support
      leds: aw2013: Use dev_err_probe

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

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

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

[Paul Sajna via B4 Relay]

From: Paul Sajna <sajattack@postmarketos.org>

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.0

[PATCH v6 2/3] leds: aw2013: Add AW2027 support

[Paul Sajna via B4 Relay]

From: Paul Sajna <sajattack@postmarketos.org>

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 | 190 ++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 163 insertions(+), 27 deletions(-)

diff --git a/drivers/leds/leds-aw2013.c b/drivers/leds/leds-aw2013.c
index cf20e94920fd..5ff9887086a6 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 AW2013/AW2027 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,19 +72,99 @@ 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)
+{
+	int ret;
+
+	ret = regmap_update_bits(chip->regmap,
+				 AW20XX_GCR2,
+				 AW2013_IMAX_MASK,
+				 AW2027_GLOBAL_IMAX_30MA);
+
+	return ret;
+}
+
+static int aw2027_set_local_imax(struct aw2013 *chip, struct aw2013_led led, u32 imax)
+{
+	int ret;
+
+	ret = regmap_update_bits(chip->regmap,
+				 AW2013_LCFG(led.num),
+				 AW2027_LOCAL_IMAX_MASK,
+				 imax);
+
+	return ret;
+}
+
 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,10 +173,37 @@ static int aw2013_chip_init(struct aw2013 *chip)
 		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 = aw2027_set_global_imax(chip);
+		if (ret) {
+			dev_err(&chip->client->dev,
+				"Failed to set maximum global current: %d\n",
+				ret);
+			return ret;
+		}
+		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;
+			}
+		}
+		return ret;
+	}
+
+	/* 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_LCFG_IMAX_MASK,
+					 AW2013_IMAX_MASK,
 					 chip->leds[i].imax);
 		if (ret) {
 			dev_err(&chip->client->dev,
@@ -142,9 +261,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;
 
@@ -271,6 +388,16 @@ static int aw2013_probe_dt(struct aw2013 *chip)
 	if (!count || count > AW2013_MAX_LEDS)
 		return -EINVAL;
 
+	if (chip->cdef->gcr2_imax) {
+		mutex_lock(&chip->mutex);
+		ret = aw2027_set_global_imax(chip);
+		if (ret) {
+			mutex_unlock(&chip->mutex);
+			return ret;
+		}
+		mutex_unlock(&chip->mutex);
+	}
+
 	for_each_available_child_of_node_scoped(np, child) {
 		struct led_init_data init_data = {};
 		u32 source;
@@ -293,9 +420,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");
 		}
@@ -306,13 +433,21 @@ static int aw2013_probe_dt(struct aw2013 *chip)
 		mutex_lock(&chip->mutex);
 		chip->num_leds = ++i;
 		if (chip->enabled) {
-			ret = regmap_update_bits(chip->regmap,
-						 AW2013_LCFG(led->num),
-						 AW2013_LCFG_IMAX_MASK,
-						 led->imax);
-			if (ret) {
-				mutex_unlock(&chip->mutex);
-				return ret;
+			if (chip->cdef->gcr2_imax) {
+				ret = aw2027_set_local_imax(chip, *led, led->imax);
+				if (ret) {
+					mutex_unlock(&chip->mutex);
+					return ret;
+				}
+			} else {
+				ret = regmap_update_bits(chip->regmap,
+							 AW2013_LCFG(led->num),
+							 AW2013_IMAX_MASK,
+							 led->imax);
+				if (ret) {
+					mutex_unlock(&chip->mutex);
+					return ret;
+				}
 			}
 		}
 		mutex_unlock(&chip->mutex);
@@ -334,18 +469,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;
@@ -357,9 +492,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",
@@ -393,8 +529,7 @@ static int aw2013_probe(struct i2c_client *client)
 			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;
@@ -433,7 +568,8 @@ static int aw2013_probe(struct i2c_client *client)
 }
 
 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 */ },
 };
 
@@ -450,5 +586,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("AW2013/AW2027 LED driver");
 MODULE_LICENSE("GPL v2");

-- 
2.56.0

[PATCH v6 3/3] leds: aw2013: Use dev_err_probe

[Paul Sajna via B4 Relay]

From: Paul Sajna <sajattack@postmarketos.org>

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 | 28 +++++++++++++---------------
 1 file changed, 13 insertions(+), 15 deletions(-)

diff --git a/drivers/leds/leds-aw2013.c b/drivers/leds/leds-aw2013.c
index 5ff9887086a6..f1106456d4ed 100644
--- a/drivers/leds/leds-aw2013.c
+++ b/drivers/leds/leds-aw2013.c
@@ -497,9 +497,8 @@ static int aw2013_probe(struct i2c_client *client)
 
 	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",
-			ret);
+		ret = dev_err_probe(&client->dev, PTR_ERR(chip->regmap),
+				    "Failed to allocate register map\n");
 		goto error;
 	}
 
@@ -509,30 +508,29 @@ 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);
+		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);
+		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);
+		ret = dev_err_probe(&client->dev, ret,
+				    "Failed to read chip ID\n");
 		goto error_reg;
 	}
 	if (chipid != chip->cdef->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;
 	}
 
@@ -545,8 +543,8 @@ static int aw2013_probe(struct i2c_client *client)
 	ret = regulator_bulk_disable(ARRAY_SIZE(chip->regulators),
 				     chip->regulators);
 	if (ret) {
-		dev_err(&client->dev,
-			"Failed to disable regulators: %d\n", ret);
+		ret = dev_err_probe(&client->dev, ret,
+				    "Failed to disable regulators\n");
 		goto error;
 	}
 

-- 
2.56.0

Re: [PATCH v6 1/3] dt-bindings: leds: aw2013: Add awinic,aw2027 compatible

[sashiko-bot]

> From: Paul Sajna <sajattack@postmarketos.org>
> 
> 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>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112-aw2013-aw20xx-rename-v6-0-251eca340abc@postmarketos.org?part=1

Re: [PATCH v6 3/3] leds: aw2013: Use dev_err_probe

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] leds: aw2013: I2C write in aw2013_probe_dt() while regulators
  are disabled
- [Low] leds: aw2013: missed dev_err_probe() conversion in
  aw2013_probe()
--

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

leds: aw2013: Use dev_err_probe

This commit converts error logging in aw2013_probe() from dev_err() to
dev_err_probe().

> diff --git a/drivers/leds/leds-aw2013.c b/drivers/leds/leds-aw2013.c
> index 5ff9887086a61..f1106456d4edc 100644
> --- a/drivers/leds/leds-aw2013.c
> +++ b/drivers/leds/leds-aw2013.c

[ ... ]

> @@ -509,30 +508,29 @@ static int aw2013_probe(struct i2c_client *client)

[ ... ]

>  	if (chipid != chip->cdef->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;
>  	}

[Severity: Low]
Should the regmap_write(chip->regmap, AW2013_RSTR, AW2013_RSTR_RESET) error
path immediately following this block also use dev_err_probe() for
consistency with the rest of aw2013_probe()?

> @@ -545,8 +543,8 @@ static int aw2013_probe(struct i2c_client *client)
>  	ret = regulator_bulk_disable(ARRAY_SIZE(chip->regulators),
>  				     chip->regulators);
>  	if (ret) {
> -		dev_err(&client->dev,
> -			"Failed to disable regulators: %d\n", ret);
> +		ret = dev_err_probe(&client->dev, ret,
> +				    "Failed to disable regulators\n");
>  		goto error;
>  	}

[Severity: High]
Does aw2013_probe_dt() write to AW20XX_GCR2 over I2C after
regulator_bulk_disable() has powered down the vcc and vio supplies?

drivers/leds/leds-aw2013.c:aw2013_probe_dt() {
	...
	if (chip->cdef->gcr2_imax) {
		mutex_lock(&chip->mutex);
		ret = aw2027_set_global_imax(chip);
		if (ret) {
			mutex_unlock(&chip->mutex);
			return ret;
		}
		mutex_unlock(&chip->mutex);
	}
	...
}

On AW2027 boards with switchable vcc or vio regulators, this I2C access
While the supplies are disabled will fail and abort probe.  Since
aw2013_chip_init() already calls aw2027_set_global_imax() after enabling
the regulators and the chip, the call in aw2013_probe_dt() can be dropped.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112-aw2013-aw20xx-rename-v6-0-251eca340abc@postmarketos.org?part=3

Re: [PATCH v6 2/3] leds: aw2013: Add AW2027 support

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] leds: aw2013: unpowered I2C access in aw2013_probe_dt() via
  aw2027_set_global_imax()
- [Low] leds: aw2013: pass-by-value of struct aw2013_led in
  aw2027_set_local_imax()
--

commit a83734f3f7807e1e9702a5495a99e5cba9b2c327
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 definition structures and configuring
both global and per-channel maximum current registers.

> diff --git a/drivers/leds/leds-aw2013.c b/drivers/leds/leds-aw2013.c
> index cf20e94920fd6..5ff9887086a61 100644
> --- a/drivers/leds/leds-aw2013.c
> +++ b/drivers/leds/leds-aw2013.c
[ ... ]
> @@ -60,19 +72,99 @@ struct aw2013_led {
[ ... ]
> +static int aw2027_set_local_imax(struct aw2013 *chip, struct aw2013_led led, u32 imax)
> +{
> +	int ret;
> +
> +	ret = regmap_update_bits(chip->regmap,
> +				 AW2013_LCFG(led.num),
> +				 AW2027_LOCAL_IMAX_MASK,
> +				 imax);
> +
> +	return ret;
> +}

[Severity: Low]
Does aw2027_set_local_imax() need to take struct aw2013_led by value?
struct aw2013_led embeds struct led_classdev, so each call copies the entire
structure onto the stack just to read led.num, while also passing led.imax
as a separate argument.  Passing a const struct aw2013_led *led pointer (or
unifying the AW2013_LCFG update using a mask from struct aw2013_chipdef)
would avoid the stack copy and duplicate loops.

[ ... ]

> @@ -271,6 +388,16 @@ static int aw2013_probe_dt(struct aw2013 *chip)
>  	if (!count || count > AW2013_MAX_LEDS)
>  		return -EINVAL;
>  
> +	if (chip->cdef->gcr2_imax) {
> +		mutex_lock(&chip->mutex);
> +		ret = aw2027_set_global_imax(chip);
> +		if (ret) {
> +			mutex_unlock(&chip->mutex);
> +			return ret;
> +		}
> +		mutex_unlock(&chip->mutex);
> +	}

[Severity: High]
Does aw2027_set_global_imax() perform an I2C register access here while the
chip is unpowered?

aw2013_probe() resets the chip and disables the vcc and vio regulators via
regulator_bulk_disable() right before calling aw2013_probe_dt(), with
chip->enabled still false:

drivers/leds/leds-aw2013.c:aw2013_probe() {
    ...
	ret = regulator_bulk_disable(ARRAY_SIZE(chip->regulators),
				     chip->regulators);
	if (ret) {
		dev_err(&client->dev,
			"Failed to disable regulators: %d\n", ret);
		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);
    ...
}

Because aw2027_regmap_config has no register cache, calling
aw2027_set_global_imax() at the start of aw2013_probe_dt() issues an I2C
transaction to AW20XX_GCR2 while vcc and vio are disabled, causing probe to
fail when those supplies are gated.  Since aw2013_chip_init() already calls
aw2027_set_global_imax() once aw2013_chip_enable() powers on the chip, can
this block in aw2013_probe_dt() be dropped?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112-aw2013-aw20xx-rename-v6-0-251eca340abc@postmarketos.org?part=2