[PATCH v3 0/3] leds: aw2013: Document interrupt and pull-up supply

[PATCH v3 0/3] leds: aw2013: Document interrupt and pull-up supply

[Stephan Gerhold]

AW2013 has an optional interrupt pin "INTN" which is used to report 
completion of started operations (e.g. power up or LED breath effects). 
The driver does not use it (yet) but it should be already described for 
completeness inside the DT schema.

Since the interrupt and I2C lines operate in open drain low active mode 
a pull-up supply is needed for correct operation. Unfortunately there 
is no ideal place to describe it in the DT: The pull-up needed for the 
I2C lines could be described on the I2C bus. However, the pull-up 
needed for the interrupt line belongs neither directly to the interrupt 
controller nor to AW2013. Since the AW2013 driver will be typically in 
control of the power management and interrupt masking it makes more 
sense to describe it inside the AW2013 device tree node.

Changes in v3:
  - Rewrite commit messages based on discussion on v2
  - Also document missing interrupt in DT schema (new patch)

Discussion on v2:
https://lore.kernel.org/linux-leds/20230321220825.GA1685482-robh@kernel.org/

Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
---
Lin, Meng-Bo (1):
      leds: aw2013: Enable pull-up supply for interrupt and I2C

Stephan Gerhold (2):
      dt-bindings: leds: aw2013: Document interrupt
      dt-bindings: leds: Document pull-up supply for interrupt and I2C

 .../devicetree/bindings/leds/leds-aw2013.yaml      | 13 ++++++++
 drivers/leds/leds-aw2013.c                         | 36 +++++++++++++---------
 2 files changed, 35 insertions(+), 14 deletions(-)
---
base-commit: 841165267827955bb3295b066cb6a906ba9265c0
change-id: 20230815-aw2013-vio-92a4566c5863

Best regards,
-- 
Stephan Gerhold <stephan@gerhold.net>

[PATCH v3 1/3] dt-bindings: leds: aw2013: Document interrupt

[Stephan Gerhold]

AW2013 has an optional interrupt pin "INTN" which is used to report
completion of started operations (e.g. power up or LED breath effects).
The driver does not use it (yet) but it should be described for
completeness inside the DT schema.

Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
---
 Documentation/devicetree/bindings/leds/leds-aw2013.yaml | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/Documentation/devicetree/bindings/leds/leds-aw2013.yaml b/Documentation/devicetree/bindings/leds/leds-aw2013.yaml
index 08f3e1cfc1b1..a0a0dabcfbf3 100644
--- a/Documentation/devicetree/bindings/leds/leds-aw2013.yaml
+++ b/Documentation/devicetree/bindings/leds/leds-aw2013.yaml
@@ -20,6 +20,11 @@ properties:
   reg:
     maxItems: 1
 
+  interrupts:
+    maxItems: 1
+    description: Open-drain, low active interrupt pin "INTN".
+      Used to report completion of operations (power up, LED breath effects).
+
   vcc-supply:
     description: Regulator providing power to the "VCC" pin.
 
@@ -52,6 +57,7 @@ additionalProperties: false
 examples:
   - |
     #include <dt-bindings/gpio/gpio.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
     #include <dt-bindings/leds/common.h>
 
     i2c {
@@ -61,6 +67,7 @@ examples:
         led-controller@45 {
             compatible = "awinic,aw2013";
             reg = <0x45>;
+            interrupts = <42 IRQ_TYPE_LEVEL_LOW>;
             #address-cells = <1>;
             #size-cells = <0>;
 

-- 
2.41.0

[PATCH v3 2/3] dt-bindings: leds: Document pull-up supply for interrupt and I2C

[Stephan Gerhold]

Since the interrupt and I2C lines of AW2013 operate in open drain low
active mode a pull-up supply is needed for correct operation.
Unfortunately there is no ideal place to describe it in the DT: The
pull-up needed for the I2C lines could be described on the I2C bus.
However, the pull-up needed for the interrupt line belongs neither
directly to the interrupt controller nor to AW2013. Since the AW2013
driver will be typically in control of the power management and
interrupt masking it makes more sense to describe it inside the AW2013
device tree node.

Add it to the AW2013 DT schema together with a comment that makes it
clear what exactly it represents.

Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
---
Related previous discussion:
https://lore.kernel.org/linux-leds/20230321220825.GA1685482-robh@kernel.org/
---
 Documentation/devicetree/bindings/leds/leds-aw2013.yaml | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/leds/leds-aw2013.yaml b/Documentation/devicetree/bindings/leds/leds-aw2013.yaml
index a0a0dabcfbf3..26238446f2bd 100644
--- a/Documentation/devicetree/bindings/leds/leds-aw2013.yaml
+++ b/Documentation/devicetree/bindings/leds/leds-aw2013.yaml
@@ -28,6 +28,12 @@ properties:
   vcc-supply:
     description: Regulator providing power to the "VCC" pin.
 
+  vio-supply:
+    description: Regulator providing power for pull-up of the I/O lines.
+      "VIO1" in the typical application circuit example of the datasheet.
+      Note that this regulator does not directly connect to AW2013, but is
+      needed for the correct operation of the interrupt and I2C lines.
+
   "#address-cells":
     const: 1
 

-- 
2.41.0

[PATCH v3 3/3] leds: aw2013: Enable pull-up supply for interrupt and I2C

[Stephan Gerhold]

From: "Lin, Meng-Bo" <linmengbo0689@protonmail.com>

Request and enable the "vio" regulator that represents the power supply
that is needed for the pull-up resistors of the interrupt and I2C lines
of AW2013. While this regulator is not wired directly to the AW2013
chip it is best managed as part of the AW2013 driver since it decides
when AW2013 is powered on and when the interrupt is enabled or
disabled.

This regulator should always be enabled in conjunction with the main
VCC power supply, so use the bulk regulator functions to enable both
at the same time.

Signed-off-by: Lin, Meng-Bo <linmengbo0689@protonmail.com>
[rewrite commit message based on discussion]
Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
---
 drivers/leds/leds-aw2013.c | 36 ++++++++++++++++++++++--------------
 1 file changed, 22 insertions(+), 14 deletions(-)

diff --git a/drivers/leds/leds-aw2013.c b/drivers/leds/leds-aw2013.c
index 59765640b70f..0010c0927434 100644
--- a/drivers/leds/leds-aw2013.c
+++ b/drivers/leds/leds-aw2013.c
@@ -62,7 +62,7 @@ struct aw2013_led {
 
 struct aw2013 {
 	struct mutex mutex; /* held when writing to registers */
-	struct regulator *vcc_regulator;
+	struct regulator_bulk_data regulators[2];
 	struct i2c_client *client;
 	struct aw2013_led leds[AW2013_MAX_LEDS];
 	struct regmap *regmap;
@@ -106,10 +106,11 @@ static void aw2013_chip_disable(struct aw2013 *chip)
 
 	regmap_write(chip->regmap, AW2013_GCR, 0);
 
-	ret = regulator_disable(chip->vcc_regulator);
+	ret = regulator_bulk_disable(ARRAY_SIZE(chip->regulators),
+				     chip->regulators);
 	if (ret) {
 		dev_err(&chip->client->dev,
-			"Failed to disable regulator: %d\n", ret);
+			"Failed to disable regulators: %d\n", ret);
 		return;
 	}
 
@@ -123,10 +124,11 @@ static int aw2013_chip_enable(struct aw2013 *chip)
 	if (chip->enabled)
 		return 0;
 
-	ret = regulator_enable(chip->vcc_regulator);
+	ret = regulator_bulk_enable(ARRAY_SIZE(chip->regulators),
+				    chip->regulators);
 	if (ret) {
 		dev_err(&chip->client->dev,
-			"Failed to enable regulator: %d\n", ret);
+			"Failed to enable regulators: %d\n", ret);
 		return ret;
 	}
 	chip->enabled = true;
@@ -348,19 +350,23 @@ static int aw2013_probe(struct i2c_client *client)
 		goto error;
 	}
 
-	chip->vcc_regulator = devm_regulator_get(&client->dev, "vcc");
-	ret = PTR_ERR_OR_ZERO(chip->vcc_regulator);
-	if (ret) {
+	chip->regulators[0].supply = "vcc";
+	chip->regulators[1].supply = "vio";
+	ret = devm_regulator_bulk_get(&client->dev,
+				      ARRAY_SIZE(chip->regulators),
+				      chip->regulators);
+	if (ret < 0) {
 		if (ret != -EPROBE_DEFER)
 			dev_err(&client->dev,
-				"Failed to request regulator: %d\n", ret);
+				"Failed to request regulators: %d\n", ret);
 		goto error;
 	}
 
-	ret = regulator_enable(chip->vcc_regulator);
+	ret = regulator_bulk_enable(ARRAY_SIZE(chip->regulators),
+				    chip->regulators);
 	if (ret) {
 		dev_err(&client->dev,
-			"Failed to enable regulator: %d\n", ret);
+			"Failed to enable regulators: %d\n", ret);
 		goto error;
 	}
 
@@ -382,10 +388,11 @@ static int aw2013_probe(struct i2c_client *client)
 	if (ret < 0)
 		goto error_reg;
 
-	ret = regulator_disable(chip->vcc_regulator);
+	ret = regulator_bulk_disable(ARRAY_SIZE(chip->regulators),
+				     chip->regulators);
 	if (ret) {
 		dev_err(&client->dev,
-			"Failed to disable regulator: %d\n", ret);
+			"Failed to disable regulators: %d\n", ret);
 		goto error;
 	}
 
@@ -394,7 +401,8 @@ static int aw2013_probe(struct i2c_client *client)
 	return 0;
 
 error_reg:
-	regulator_disable(chip->vcc_regulator);
+	regulator_bulk_disable(ARRAY_SIZE(chip->regulators),
+			       chip->regulators);
 
 error:
 	mutex_destroy(&chip->mutex);

-- 
2.41.0

Re: [PATCH v3 1/3] dt-bindings: leds: aw2013: Document interrupt

[Krzysztof Kozlowski]

On 15/08/2023 19:21, Stephan Gerhold wrote:
> AW2013 has an optional interrupt pin "INTN" which is used to report
> completion of started operations (e.g. power up or LED breath effects).
> The driver does not use it (yet) but it should be described for
> completeness inside the DT schema.
> 
> Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
> ---


Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: [PATCH v3 2/3] dt-bindings: leds: Document pull-up supply for interrupt and I2C

[Krzysztof Kozlowski]

On 15/08/2023 19:21, Stephan Gerhold wrote:
> Since the interrupt and I2C lines of AW2013 operate in open drain low
> active mode a pull-up supply is needed for correct operation.
> Unfortunately there is no ideal place to describe it in the DT: The
> pull-up needed for the I2C lines could be described on the I2C bus.
> However, the pull-up needed for the interrupt line belongs neither
> directly to the interrupt controller nor to AW2013. Since the AW2013
> driver will be typically in control of the power management and
> interrupt masking it makes more sense to describe it inside the AW2013
> device tree node.
> 
> Add it to the AW2013 DT schema together with a comment that makes it
> clear what exactly it represents.
> 

Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: [PATCH v3 0/3] leds: aw2013: Document interrupt and pull-up supply

[Nikita Travkin]

Stephan Gerhold писал(а) 15.08.2023 22:21:
> AW2013 has an optional interrupt pin "INTN" which is used to report 
> completion of started operations (e.g. power up or LED breath effects). 
> The driver does not use it (yet) but it should be already described for 
> completeness inside the DT schema.
> 
> Since the interrupt and I2C lines operate in open drain low active mode 
> a pull-up supply is needed for correct operation. Unfortunately there 
> is no ideal place to describe it in the DT: The pull-up needed for the 
> I2C lines could be described on the I2C bus. However, the pull-up 
> needed for the interrupt line belongs neither directly to the interrupt 
> controller nor to AW2013. Since the AW2013 driver will be typically in 
> control of the power management and interrupt masking it makes more 
> sense to describe it inside the AW2013 device tree node.
> 

Oh indeed, seems like even on the hardware I was initially targeting,
the pull is tied to a regulator, I probably missed it because it was
always on. Thank you both for adding that and fixing up the bindings!

Reviewed-by: Nikita Travkin <nikita@trvn.ru>

Nikita

> Changes in v3:
>   - Rewrite commit messages based on discussion on v2
>   - Also document missing interrupt in DT schema (new patch)
> 
> Discussion on v2:
> https://lore.kernel.org/linux-leds/20230321220825.GA1685482-robh@kernel.org/
> 
> Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
> ---
> Lin, Meng-Bo (1):
>       leds: aw2013: Enable pull-up supply for interrupt and I2C
> 
> Stephan Gerhold (2):
>       dt-bindings: leds: aw2013: Document interrupt
>       dt-bindings: leds: Document pull-up supply for interrupt and I2C
> 
>  .../devicetree/bindings/leds/leds-aw2013.yaml      | 13 ++++++++
>  drivers/leds/leds-aw2013.c                         | 36 +++++++++++++---------
>  2 files changed, 35 insertions(+), 14 deletions(-)
> ---
> base-commit: 841165267827955bb3295b066cb6a906ba9265c0
> change-id: 20230815-aw2013-vio-92a4566c5863
> 
> Best regards,

Re: [PATCH v3 0/3] leds: aw2013: Document interrupt and pull-up supply

[Lee Jones]

On Tue, 15 Aug 2023 19:21:03 +0200, Stephan Gerhold wrote:
> AW2013 has an optional interrupt pin "INTN" which is used to report
> completion of started operations (e.g. power up or LED breath effects).
> The driver does not use it (yet) but it should be already described for
> completeness inside the DT schema.
> 
> Since the interrupt and I2C lines operate in open drain low active mode
> a pull-up supply is needed for correct operation. Unfortunately there
> is no ideal place to describe it in the DT: The pull-up needed for the
> I2C lines could be described on the I2C bus. However, the pull-up
> needed for the interrupt line belongs neither directly to the interrupt
> controller nor to AW2013. Since the AW2013 driver will be typically in
> control of the power management and interrupt masking it makes more
> sense to describe it inside the AW2013 device tree node.
> 
> [...]

Applied, thanks!

[1/3] dt-bindings: leds: aw2013: Document interrupt
      commit: 9422bcf125b94e553c795af4f6c59d8e8fd8affa
[2/3] dt-bindings: leds: Document pull-up supply for interrupt and I2C
      commit: 2cccb179addedff6a5234e37237fc6b22d9217d4
[3/3] leds: aw2013: Enable pull-up supply for interrupt and I2C
      commit: baca986e1f2c31f8e4b2a6d99d47c3bc844033e8

--
Lee Jones [李琼斯]