[PATCH v3 0/6] leds: aw2013: Add AW2027 support

[PATCH v3 0/6] leds: aw2013: Add AW2027 support

[Paul Sajna]

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

Several pre-existing bugs were also caught by Sashiko Review
and fixed in this patch-series. The joys of LLM-inflicted bikeshedding.

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 v3:
- Drop rename
- Capitalize commit subject
- Other requested commit rewording
- Extra tweaks to cleanup patches (preventing more unbalanced regulator
  stuff)
- Whitespace fiddling for `checkpatch --strict`
- 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: 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: Nikita Travkin <nikita@trvn.ru>
Cc: linux-leds@vger.kernel.org
Cc: devicetree@vger.kernel.org
Cc: linux-kernel@vger.kernel.org

---
Paul Sajna (6):
      dt-bindings: leds: aw2013: Add awinic,aw2027 compatible
      leds: aw2013: Add AW2027 support
      leds: aw2013: Use dev_err_probe
      leds: aw2013: Move assignment of chip->num_leds
      leds: aw2013: Prevent writes to unpowered chip
      leds: aw2013: Move reset register write

 .../devicetree/bindings/leds/leds-aw2013.yaml      |   6 +-
 drivers/leds/leds-aw2013.c                         | 207 ++++++++++++++++-----
 2 files changed, 165 insertions(+), 48 deletions(-)
---
base-commit: 944a035ecca915ae947905dcfb03f2b9dc6d032c
change-id: 20260907-aw2013-aw20xx-rename-df233c5921c6

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

[PATCH v3 1/6] 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.

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

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

[Krzysztof Kozlowski]

On 13/10/2026 07:39, Paul Sajna wrote:
> 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.
> 
> Signed-off-by: Paul Sajna <sajattack@postmarketos.org>
> ---
>  Documentation/devicetree/bindings/leds/leds-aw2013.yaml | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)


Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>

Best regards,
Krzysztof

[PATCH v3 2/6] 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 | 137 ++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 117 insertions(+), 20 deletions(-)

diff --git a/drivers/leds/leds-aw2013.c b/drivers/leds/leds-aw2013.c
index 216755d6010f..4da33a86af16 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
+#define AW2013_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))
 
 /* 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)
@@ -50,6 +52,13 @@
 #define AW2013_REG_MAX 0x77
 
 #define AW2013_TIME_STEP 130 /* ms */
+#define AW2027_RSTR_CHIP_ID 0x09
+#define AW2027_REG_MAX 0x7F /* copied from vendor driver, but only up to 0x3F is documented */
+
+#define AW2027_IMAX_15MA (0)
+#define AW2027_IMAX_30MA (1)
+#define AW2027_IMAX_5MA  (2)
+#define AW2027_IMAX_10MA (3)
 
 struct aw2013;
 
@@ -60,16 +69,70 @@ 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;
+};
+
 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 = 15000,
+};
+
+static const struct aw2013_chipdef aw2027_chipdef = {
+	.chip_id = AW2027_RSTR_CHIP_ID,
+	.regmap_cfg = &aw2027_regmap_config,
+	.default_imax = 3, // 6mA
+	.gcr2_imax = true,
+	.current_levels = 16,
+	.current_max = 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
+ */
+static inline u32 aw2013_microamp_to_imax(u32 microamp, const struct aw2013_chipdef *cdef)
+{
+	return min_t(u32, (microamp * (cdef->current_levels - 1))
+			  / cdef->current_max,
+		     cdef->current_levels - 1);
+}
+
 static int aw2013_chip_init(struct aw2013 *chip)
 {
 	int i, ret;
@@ -81,17 +144,50 @@ 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 = regmap_update_bits(chip->regmap,
-					 AW2013_LCFG(chip->leds[i].num),
-					 AW2013_LCFG_IMAX_MASK,
-					 chip->leds[i].imax);
+				AW20XX_GCR2,
+				AW2013_IMAX_MASK,
+				AW2027_IMAX_30MA);
 		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;
 		}
+
+		for (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;
+			}
+		}
+	} else {
+		/* AW2013 only supports 4-step individual current per LED */
+		for (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;
@@ -292,9 +388,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");
 		}
@@ -323,18 +419,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(&client->dev, sizeof(*chip), GFP_KERNEL);
 	if (!chip)
 		return -ENOMEM;
@@ -346,9 +442,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",
@@ -382,8 +479,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;
@@ -420,7 +516,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 */ },
 };
 
@@ -437,5 +534,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.55.0

Re: [PATCH v3 2/6] leds: aw2013: Add AW2027 support

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] locking: double mutex unlock on probe error path
- [Medium] leds: aw2013: 4-bit AW2027 local current limit truncated by 2-bit mask
--

commit 117fb3f76f08e72c0e73f7c0543a74753854f334
Author: Paul Sajna <sajattack@postmarketos.org>
leds: aw2013: Add AW2027 support

This commit adds support for the AW2027 LED driver. It introduces a chip
definition structure to handle differences such as maximum current, current
levels for color-mixing, and global versus local current configuration.

> diff --git a/drivers/leds/leds-aw2013.c b/drivers/leds/leds-aw2013.c
> index 216755d6010fe..4da33a86af16a 100644
> --- a/drivers/leds/leds-aw2013.c
> +++ b/drivers/leds/leds-aw2013.c
> @@ -81,17 +144,50 @@ static int aw2013_chip_init(struct aw2013 *chip)

[ ... ]

> +	if (chip->cdef->gcr2_imax) {

[ ... ]

> +		for (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);

[Severity: Medium]
Does this code inadvertently truncate the AW2027 local current limit?

For the AW2027, aw2013_microamp_to_imax() calculates a 4-bit limit (0-15) based
on the led-max-microamp property. However, AW2013_IMAX_MASK is defined as
BIT(0) | BIT(1) (0x03).

By using a 2-bit mask here, the upper 2 bits of the 4-bit imax value are
silently discarded. Does this effectively limit the maximum LED brightness to a
fraction of its intended capability and break the 16-level color-mixing control?

[ ... ]

> @@ -420,7 +516,8 @@ static int aw2013_probe(struct i2c_client *client)
>  }

[Severity: High]
This problem wasn't introduced by this patch, but patch 6 in this series moved
the mutex_unlock(&chip->mutex) before aw2013_probe_dt() without updating the
error paths.

If aw2013_probe_dt() fails, or if the final regulator_bulk_disable() fails,
the probe function jumps to the error_reg or error cleanup labels:

error_reg:
	regulator_bulk_disable(ARRAY_SIZE(chip->regulators),
			       chip->regulators);

error:
	mutex_unlock(&chip->mutex);
	return ret;

Since the mutex is already unlocked prior to calling aw2013_probe_dt(), does
this path cause a double-unlock of chip->mutex?

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

[PATCH v3 3/6] 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 | 28 +++++++++++++---------------
 1 file changed, 13 insertions(+), 15 deletions(-)

diff --git a/drivers/leds/leds-aw2013.c b/drivers/leds/leds-aw2013.c
index 4da33a86af16..05d61f4e5804 100644
--- a/drivers/leds/leds-aw2013.c
+++ b/drivers/leds/leds-aw2013.c
@@ -447,9 +447,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;
 	}
 
@@ -459,30 +458,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;
 	}
 
@@ -497,8 +495,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.55.0

[PATCH v3 4/6] leds: aw2013: Move assignment of chip->num_leds

[Paul Sajna]

devm_led_classdev_register_ext() executes default triggers
synchronously, which call aw20xx_brightness_set().
aw20xx_chip_in_use() silently returns false, skipping chip enablement and
causing unbalanced regulator disables while probe is still running.

Increment chip->num_leds inside the loop, so that each call gets the
correct value for it's default trigger.

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

diff --git a/drivers/leds/leds-aw2013.c b/drivers/leds/leds-aw2013.c
index 05d61f4e5804..3b2be960c04d 100644
--- a/drivers/leds/leds-aw2013.c
+++ b/drivers/leds/leds-aw2013.c
@@ -398,6 +398,8 @@ static int aw2013_probe_dt(struct aw2013 *chip)
 		led->cdev.brightness_set_blocking = aw2013_brightness_set;
 		led->cdev.blink_set = aw2013_blink_set;
 
+		chip->num_leds = i + 1;
+
 		ret = devm_led_classdev_register_ext(&chip->client->dev,
 						     &led->cdev, &init_data);
 		if (ret < 0)
@@ -409,8 +411,6 @@ static int aw2013_probe_dt(struct aw2013 *chip)
 	if (!count)
 		return -EINVAL;
 
-	chip->num_leds = i;
-
 	return 0;
 }
 

-- 
2.55.0

[PATCH v3 5/6] leds: aw2013: Prevent writes to unpowered chip

[Paul Sajna]

Replace chip_in_use check with chip->enabled boolean in
aw20xx_brightness_set

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:

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.

To prevent writing gcr=0 when we couldn't power on the chip,
add aw2013_chip_poweroff()

To prevent turning the chip on/off unneccessarily, add a check for
writing brightness to 0.

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

diff --git a/drivers/leds/leds-aw2013.c b/drivers/leds/leds-aw2013.c
index 3b2be960c04d..6e661afd7775 100644
--- a/drivers/leds/leds-aw2013.c
+++ b/drivers/leds/leds-aw2013.c
@@ -193,15 +193,13 @@ static int aw2013_chip_init(struct aw2013 *chip)
 	return ret;
 }
 
-static void aw2013_chip_disable(struct aw2013 *chip)
+static void aw2013_chip_poweroff(struct aw2013 *chip)
 {
 	int ret;
 
 	if (!chip->enabled)
 		return;
 
-	regmap_write(chip->regmap, AW2013_GCR, 0);
-
 	ret = regulator_bulk_disable(ARRAY_SIZE(chip->regulators),
 				     chip->regulators);
 	if (ret) {
@@ -213,6 +211,16 @@ static void aw2013_chip_disable(struct aw2013 *chip)
 	chip->enabled = false;
 }
 
+static void aw2013_chip_disable(struct aw2013 *chip)
+{
+	if (!chip->enabled)
+		return;
+
+	regmap_write(chip->regmap, AW2013_GCR, 0);
+
+	aw2013_chip_poweroff(chip);
+}
+
 static int aw2013_chip_enable(struct aw2013 *chip)
 {
 	int ret;
@@ -231,7 +239,7 @@ static int aw2013_chip_enable(struct aw2013 *chip)
 
 	ret = aw2013_chip_init(chip);
 	if (ret)
-		aw2013_chip_disable(chip);
+		aw2013_chip_poweroff(chip);
 
 	return ret;
 }
@@ -253,9 +261,12 @@ static int aw2013_brightness_set(struct led_classdev *cdev,
 	struct aw2013_led *led = container_of(cdev, struct aw2013_led, cdev);
 	int ret, num;
 
+	if (!led->chip->enabled && brightness == 0)
+		return 0;
+
 	mutex_lock(&led->chip->mutex);
 
-	if (aw2013_chip_in_use(led->chip)) {
+	if (!led->chip->enabled) {
 		ret = aw2013_chip_enable(led->chip);
 		if (ret)
 			goto error;
@@ -284,7 +295,12 @@ static int aw2013_brightness_set(struct led_classdev *cdev,
 	if (!aw2013_chip_in_use(led->chip))
 		aw2013_chip_disable(led->chip);
 
+	mutex_unlock(&led->chip->mutex);
+
+	return ret;
+
 error:
+	aw2013_chip_poweroff(led->chip);
 	mutex_unlock(&led->chip->mutex);
 
 	return ret;

-- 
2.55.0

Re: [PATCH v3 5/6] leds: aw2013: Prevent writes to unpowered chip

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] leds: aw2013: Unconditional chip poweroff on I2C error breaks active LEDs
- [Medium] leds: aw2013: lockless early return skips PWM clearing on shared regulators
- [Medium] leds: aw2013: state desync if regulator_bulk_disable() fails
- [Low] leds: aw2013: writes to unpowered chip in aw2013_blink_set()
--

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

    leds: aw2013: Prevent writes to unpowered chip

    This patch replaces the chip_in_use check with a chip->enabled boolean in
    aw2013_brightness_set to prevent writing to the chip when it is unpowered.
    It also adds aw2013_chip_poweroff() to avoid sending I2C commands when
    powering on the chip fails.

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

[Severity: Low]
Does this same issue still occur in aw2013_blink_set()?

If a prior aw2013_brightness_set() call fails (e.g., regulator enable fails),
cdev->brightness can remain > 0 while led->chip->enabled is false.

A subsequent sysfs write to blink delays invokes aw2013_blink_set(), which
sees brightness > 0, bypasses aw2013_brightness_set(), and unconditionally
writes to the unpowered chip's registers:

	/* 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);
		goto out;
	}

[ ... ]

> diff --git a/drivers/leds/leds-aw2013.c b/drivers/leds/leds-aw2013.c
> index 3b2be960c04d5..6e661afd77755 100644
> --- a/drivers/leds/leds-aw2013.c
> +++ b/drivers/leds/leds-aw2013.c
> @@ -193,15 +193,13 @@ static int aw2013_chip_init(struct aw2013 *chip)
>  	return ret;
>  }
>  
> -static void aw2013_chip_disable(struct aw2013 *chip)
> +static void aw2013_chip_poweroff(struct aw2013 *chip)
>  {
>  	int ret;
>  
>  	if (!chip->enabled)
>  		return;
>  
> -	regmap_write(chip->regmap, AW2013_GCR, 0);
> -
>  	ret = regulator_bulk_disable(ARRAY_SIZE(chip->regulators),
>  				     chip->regulators);
>  	if (ret) {
>  		dev_err(&chip->client->dev,
>  			"Failed to disable regulators: %d\n", ret);
>  		return;
>  	}
>  
>  	chip->enabled = false;
>  }

[Severity: Medium]
Could this early return cause a hardware state desynchronization if
regulator_bulk_disable() fails?

During aw2013_chip_disable(), the driver first writes 0 to AW2013_GCR to
enter software standby, then calls aw2013_chip_poweroff(). If the regulator
disable fails here, it returns early leaving chip->enabled = true.

On subsequent brightness changes, aw2013_chip_enable() will see
chip->enabled == true and skip aw2013_chip_init(). This skips restoring
AW2013_GCR to 1, leaving the chip stuck in standby but completely
unresponsive to brightness changes.

[ ... ]

> @@ -253,9 +261,12 @@ static int aw2013_brightness_set(struct led_classdev *cdev,
>  	struct aw2013_led *led = container_of(cdev, struct aw2013_led, cdev);
>  	int ret, num;
>  
> +	if (!led->chip->enabled && brightness == 0)
> +		return 0;
> +

[Severity: Medium]
Can this lockless check introduce a TOCTOU race that skips clearing the
LED's hardware registers?

If two LEDs are on and both are set to 0 concurrently, the LED core updates
cdev->brightness = 0 for both before scheduling their asynchronous works.

The first worker executes aw2013_brightness_set(), acquires the mutex,
clears its own PWM, and evaluates aw2013_chip_in_use(). Because both
cdev->brightness values are now 0, aw2013_chip_in_use() returns false,
causing the first worker to disable the chip (chip->enabled = false) and
unlock the mutex.

The second worker then evaluates this new lockless check, which is now
true, and returns early without ever taking the lock or clearing its PWM
register over I2C.

When the regulators are shared with other devices, power is not physically
cut, and the chip retains its register states in software standby. The next
time the chip is enabled, will the second LED erroneously turn on because
its registers were never cleared?

>  	mutex_lock(&led->chip->mutex);
>  
> -	if (aw2013_chip_in_use(led->chip)) {
> +	if (!led->chip->enabled) {
>  		ret = aw2013_chip_enable(led->chip);
>  		if (ret)
>  			goto error;

[ ... ]

> @@ -284,7 +295,12 @@ static int aw2013_brightness_set(struct led_classdev *cdev,
>  	if (!aw2013_chip_in_use(led->chip))
>  		aw2013_chip_disable(led->chip);
>  
> +	mutex_unlock(&led->chip->mutex);
> +
> +	return ret;
> +
>  error:
> +	aw2013_chip_poweroff(led->chip);
>  	mutex_unlock(&led->chip->mutex);
>  
>  	return ret;

[Severity: High]
Will this unconditional poweroff disrupt the hardware state of all other
active LEDs on the same chip?

If multiple LEDs are ON (so chip->enabled = true) and a transient I2C error
occurs while changing the brightness of one LED, the code jumps to this
error label.

The new aw2013_chip_poweroff() call unconditionally drops power to the
entire chip. This shuts down all other physically active LEDs without
updating their cdev->brightness in the LED core.

Should this error path only roll back aw2013_chip_enable() if it was the
call that successfully enabled the chip on this specific invocation?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261012-aw2013-aw20xx-rename-v3-0-cab1fdd050b6@postmarketos.org?part=5

[PATCH v3 6/6] leds: aw2013: Move reset register write

[Paul Sajna]

It has no reason to be in probe_dt, and putting it there complicates
the mutex requirements.

probe_dt calls devm_led_classdev_register_ext, which may trigger a
callback to aw2013_brightness_set, which would result in deadlock.

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

diff --git a/drivers/leds/leds-aw2013.c b/drivers/leds/leds-aw2013.c
index 6e661afd7775..8d1d649064c7 100644
--- a/drivers/leds/leds-aw2013.c
+++ b/drivers/leds/leds-aw2013.c
@@ -383,8 +383,6 @@ static int aw2013_probe_dt(struct aw2013 *chip)
 	if (!count || count > AW2013_MAX_LEDS)
 		return -EINVAL;
 
-	regmap_write(chip->regmap, AW2013_RSTR, AW2013_RSTR_RESET);
-
 	for_each_available_child_of_node_scoped(np, child) {
 		struct led_init_data init_data = {};
 		u32 source;
@@ -504,6 +502,14 @@ static int aw2013_probe(struct i2c_client *client)
 	if (ret)
 		goto error_reg;
 
+	ret = regmap_write(chip->regmap, AW2013_RSTR, AW2013_RSTR_RESET);
+	if (ret) {
+		dev_err(&chip->client->dev, "Failed to reset the chip: %d\n", ret);
+		goto error_reg;
+	}
+
+	mutex_unlock(&chip->mutex);
+
 	ret = aw2013_probe_dt(chip);
 	if (ret < 0)
 		goto error_reg;
@@ -516,8 +522,6 @@ static int aw2013_probe(struct i2c_client *client)
 		goto error;
 	}
 
-	mutex_unlock(&chip->mutex);
-
 	return 0;
 
 error_reg:

-- 
2.55.0

Re: [PATCH v3 6/6] leds: aw2013: Move reset register write

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] leds: aw2013: Double mutex unlock on error paths in aw2013_probe()
- [Low] leds: aw2013: Use of dev_err() instead of dev_err_probe() for probe failures
--

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

leds: aw2013: Move reset register write

This patch moves the chip reset register write from aw2013_probe_dt() to
aw2013_probe(), and unlocks the mutex before calling aw2013_probe_dt() to
avoid a deadlock. The deadlock would otherwise occur when LED classdev
registration triggers a brightness set callback.

> diff --git a/drivers/leds/leds-aw2013.c b/drivers/leds/leds-aw2013.c
> index 6e661afd77755..8d1d649064c71 100644
> --- a/drivers/leds/leds-aw2013.c
> +++ b/drivers/leds/leds-aw2013.c
[ ... ]
> @@ -504,6 +502,14 @@ static int aw2013_probe(struct i2c_client *client)
>  	if (ret)
>  		goto error_reg;
>  
> +	ret = regmap_write(chip->regmap, AW2013_RSTR, AW2013_RSTR_RESET);
> +	if (ret) {
> +		dev_err(&chip->client->dev, "Failed to reset the chip: %d\n", ret);

[Severity: Low]
Is dev_err() the right function to use here for reporting a probe failure?
The subsystem guidelines recommend using dev_err_probe() in probe paths
to ensure proper handling of -EPROBE_DEFER and correct kernel log formatting.

> +		goto error_reg;
> +	}
> +
> +	mutex_unlock(&chip->mutex);
> +
>  	ret = aw2013_probe_dt(chip);
>  	if (ret < 0)
>  		goto error_reg;

[Severity: High]
Can jumping to error_reg here cause a double unlock of chip->mutex?

By moving mutex_unlock(&chip->mutex) to execute before aw2013_probe_dt(),
the mutex is now unlocked on this error path. However, jumping to error_reg
causes it to fall through to the error label, which unconditionally unlocks
the mutex again:

error_reg:
	regulator_bulk_disable(ARRAY_SIZE(chip->regulators),
			       chip->regulators);

error:
	mutex_unlock(&chip->mutex);
	return ret;

Additionally, if aw2013_probe_dt() succeeds but the subsequent
regulator_bulk_disable() fails, it also jumps to the error label and unlocks
the already-unlocked mutex.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261012-aw2013-aw20xx-rename-v3-0-cab1fdd050b6@postmarketos.org?part=6