[PATCH v2 0/2] leds: aw2013: Add vddio regulator

[PATCH v2 0/2] leds: aw2013: Add vddio regulator

[Lin, Meng-Bo]

v2: Rename vdd to vddio and reword

Some LEDs controllers are used with external pull-up for the interrupt
line and the I2C lines, so we might need to enable a regulator to bring
the lines into usable state. Otherwise, this might cause spurious
interrupts and reading from I2C will fail.

Implement support for "vddio-supply" that is enabled by the aw2013 driver
so that the regulator gets enabled when needed.

[PATCH v2 2/2] leds: aw2013: Add vddio regulator

[Lin, Meng-Bo]

Some LEDs controllers are used with external pull-up for the interrupt
line and the I2C lines, so we might need to enable a regulator to bring
the lines into usable state. Otherwise, this might cause spurious
interrupts and reading from I2C will fail.

Implement support for "vddio-supply" that is enabled by the aw2013 driver
so that the regulator gets enabled when needed.

Signed-off-by: Lin, Meng-Bo <linmengbo0689@protonmail.com>
---
 drivers/leds/leds-aw2013.c | 26 +++++++++++++++++---------
 1 file changed, 17 insertions(+), 9 deletions(-)

diff --git a/drivers/leds/leds-aw2013.c b/drivers/leds/leds-aw2013.c
index 0b52fc9097c6..95f2f9bf95ee 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,7 +106,8 @@ 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);
@@ -123,7 +124,8 @@ 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);
@@ -348,16 +350,20 @@ 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 = "vddio";
+	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);
 		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);
@@ -382,7 +388,8 @@ 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);
@@ -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.30.2

Re: [PATCH v2 2/2] leds: aw2013: Add vddio regulator

[Pavel Machek]

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

Hi!

> Some LEDs controllers are used with external pull-up for the interrupt
> line and the I2C lines, so we might need to enable a regulator to bring
> the lines into usable state. Otherwise, this might cause spurious
> interrupts and reading from I2C will fail.
> 
> Implement support for "vddio-supply" that is enabled by the aw2013 driver
> so that the regulator gets enabled when needed.
> 
> Signed-off-by: Lin, Meng-Bo <linmengbo0689@protonmail.com>

> @@ -348,16 +350,20 @@ 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 = "vddio";
> +	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);
>  		goto error;

Won't this cause failures when optional vddio is unavailable?

BR,
							Pavel
							
-- 
People of Russia, stop Putin before his war on Ukraine escalates.

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

Re: [PATCH v2 2/2] leds: aw2013: Add vddio regulator

[Stephan Gerhold]

Hi Pavel,

On Thu, Mar 23, 2023 at 08:35:02PM +0100, Pavel Machek wrote:
> > Some LEDs controllers are used with external pull-up for the interrupt
> > line and the I2C lines, so we might need to enable a regulator to bring
> > the lines into usable state. Otherwise, this might cause spurious
> > interrupts and reading from I2C will fail.
> > 
> > Implement support for "vddio-supply" that is enabled by the aw2013 driver
> > so that the regulator gets enabled when needed.
> > 
> > Signed-off-by: Lin, Meng-Bo <linmengbo0689@protonmail.com>
> 
> > @@ -348,16 +350,20 @@ 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 = "vddio";
> > +	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);
> >  		goto error;
> 
> Won't this cause failures when optional vddio is unavailable?
> 

The regulator core should print a warning "supply vddio not found, using
dummy regulator" but then proceed normally.

I think in almost all cases a separate I/O supply should actually exist
that could be described in the device tree. It was likely just omitted
because it's always-on or indirectly being enabled by other devices.
So perhaps having this warning is even a good thing?

Thanks,
Stephan

Re: [PATCH v2 2/2] leds: aw2013: Add vddio regulator

[Luca Weiss]

On Freitag, 24. März 2023 09:32:52 CEST Stephan Gerhold wrote:
> Hi Pavel,
> 
> On Thu, Mar 23, 2023 at 08:35:02PM +0100, Pavel Machek wrote:
> > > Some LEDs controllers are used with external pull-up for the interrupt
> > > line and the I2C lines, so we might need to enable a regulator to bring
> > > the lines into usable state. Otherwise, this might cause spurious
> > > interrupts and reading from I2C will fail.
> > > 
> > > Implement support for "vddio-supply" that is enabled by the aw2013
> > > driver
> > > so that the regulator gets enabled when needed.
> > > 
> > > Signed-off-by: Lin, Meng-Bo <linmengbo0689@protonmail.com>
> > > 
> > > @@ -348,16 +350,20 @@ 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 = "vddio";
> > > +	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);
> > >  		
> > >  		goto error;
> > 
> > Won't this cause failures when optional vddio is unavailable?
> 
> The regulator core should print a warning "supply vddio not found, using
> dummy regulator" but then proceed normally.
> 
> I think in almost all cases a separate I/O supply should actually exist
> that could be described in the device tree. It was likely just omitted
> because it's always-on or indirectly being enabled by other devices.
> So perhaps having this warning is even a good thing?

Just briefly jumping in, there was some activity adding bus_regulator to the 
i2c-core a while back, maybe that can be revived instead? For CCI (camera i2c) 
we also need pull-ups and I don't think adding vddio or whatever to all 
sensors is a good idea long term...

https://lore.kernel.org/lkml/20210527075556.1709140-1-hsinyi@chromium.org/

Regards
Luca

> 
> Thanks,
> Stephan

[PATCH v2 1/2] dt-bindings: leds: aw2013: Document vddio-supply

[Lin, Meng-Bo]

Some LEDs controllers are used with external pull-up for the interrupt
line and the I2C lines, so we might need to enable a regulator to bring
the lines into usable state. Otherwise, this might cause spurious
interrupts and reading from I2C will fail.

Document support for "vddio-supply" that is enabled by the aw2013 driver
so that the regulator gets enabled when needed.

Signed-off-by: Lin, Meng-Bo <linmengbo0689@protonmail.com>
---
 Documentation/devicetree/bindings/leds/leds-aw2013.yaml | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/leds/leds-aw2013.yaml b/Documentation/devicetree/bindings/leds/leds-aw2013.yaml
index 08f3e1cfc1b1..79b69cf1d1fe 100644
--- a/Documentation/devicetree/bindings/leds/leds-aw2013.yaml
+++ b/Documentation/devicetree/bindings/leds/leds-aw2013.yaml
@@ -23,6 +23,11 @@ properties:
   vcc-supply:
     description: Regulator providing power to the "VCC" pin.
 
+  vddio-supply:
+    description: |
+      Optional regulator that provides digital I/O voltage,
+      e.g. for pulling up the interrupt line or the I2C pins.
+
   "#address-cells":
     const: 1
 
-- 
2.30.2

Re: [PATCH v2 1/2] dt-bindings: leds: aw2013: Document vddio-supply

[Krzysztof Kozlowski]

On 20/03/2023 18:55, Lin, Meng-Bo wrote:
> Some LEDs controllers are used with external pull-up for the interrupt
> line and the I2C lines, so we might need to enable a regulator to bring
> the lines into usable state.

Not a property of this device.

> Otherwise, this might cause spurious
> interrupts and reading from I2C will fail.
> 
> Document support for "vddio-supply" that is enabled by the aw2013 driver
> so that the regulator gets enabled when needed.
> 
> Signed-off-by: Lin, Meng-Bo <linmengbo0689@protonmail.com>
> ---
>  Documentation/devicetree/bindings/leds/leds-aw2013.yaml | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/leds/leds-aw2013.yaml b/Documentation/devicetree/bindings/leds/leds-aw2013.yaml
> index 08f3e1cfc1b1..79b69cf1d1fe 100644
> --- a/Documentation/devicetree/bindings/leds/leds-aw2013.yaml
> +++ b/Documentation/devicetree/bindings/leds/leds-aw2013.yaml
> @@ -23,6 +23,11 @@ properties:
>    vcc-supply:
>      description: Regulator providing power to the "VCC" pin.
>  
> +  vddio-supply:
> +    description: |
> +      Optional regulator that provides digital I/O voltage,

NAK. I responded to your patch and you just send a v2 without explanation.

The device does not have VDDIO pin, either.

Best regards,
Krzysztof

Re: [PATCH v2 1/2] dt-bindings: leds: aw2013: Document vddio-supply

[Stephan Gerhold]

On Mon, Mar 20, 2023 at 07:04:22PM +0100, Krzysztof Kozlowski wrote:
> On 20/03/2023 18:55, Lin, Meng-Bo wrote:
> > Some LEDs controllers are used with external pull-up for the interrupt
> > line and the I2C lines, so we might need to enable a regulator to bring
> > the lines into usable state.
> 
> Not a property of this device.
> 
> > Otherwise, this might cause spurious
> > interrupts and reading from I2C will fail.
> > 
> > Document support for "vddio-supply" that is enabled by the aw2013 driver
> > so that the regulator gets enabled when needed.
> > 
> > Signed-off-by: Lin, Meng-Bo <linmengbo0689@protonmail.com>
> > ---
> >  Documentation/devicetree/bindings/leds/leds-aw2013.yaml | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/leds/leds-aw2013.yaml b/Documentation/devicetree/bindings/leds/leds-aw2013.yaml
> > index 08f3e1cfc1b1..79b69cf1d1fe 100644
> > --- a/Documentation/devicetree/bindings/leds/leds-aw2013.yaml
> > +++ b/Documentation/devicetree/bindings/leds/leds-aw2013.yaml
> > @@ -23,6 +23,11 @@ properties:
> >    vcc-supply:
> >      description: Regulator providing power to the "VCC" pin.
> >  
> > +  vddio-supply:
> > +    description: |
> > +      Optional regulator that provides digital I/O voltage,
> 
> NAK. I responded to your patch and you just send a v2 without explanation.
> 
> The device does not have VDDIO pin, either.
> 

The power supply Lin is trying to add here is basically the "VIO1"
example in "Figure 1 AW2013 Typical Application Circuit" on page 1 of
the AW2013 datasheet [1]. The I2C pins and the interrupt output are both
open-drain and therefore require external pull-up resistors, connected
to a power supply that might not be always on.

Because of the open-drain pins AW2013 does indeed not have a dedicated
input pin for the I/O supply voltage. However, it is still necessary to
describe the power supply _somewhere_, to ensure that it is enabled when
needed.

It is hard to model this properly but it's generally easiest to handle
this inside the peripheral driver since it knows exactly when I2C and/or
interrupt lines are currently needed or not. This situation is fairly
common for I2C devices so there are several precedents, e.g.:

  1. cypress,tm2-touchkey.yaml: "vddio-supply"
     https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=3e730ec11d51283ad62a98436967c01b718132ab
  2. goodix,gt7375p.yaml: "mainboard-vddio-supply"
     https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=1d18c1f3b7d938bdefc44289d137b4e6c7a3d502

Would "mainboard-vddio-supply" from the second example be more clear
than the simple "vddio-supply"? Naming things properly while staying
concise is hard. :)

And if you have any suggestions how to describe this in a better way
please let us know!

Thanks,
Stephan

[1]: https://datasheet.lcsc.com/lcsc/1808311530_AWINIC-Shanghai-Awinic-Tech-AW2013DNR_C252440.pdf

Re: [PATCH v2 1/2] dt-bindings: leds: aw2013: Document vddio-supply

[Krzysztof Kozlowski]

On 20/03/2023 19:59, Stephan Gerhold wrote:
> On Mon, Mar 20, 2023 at 07:04:22PM +0100, Krzysztof Kozlowski wrote:
>> On 20/03/2023 18:55, Lin, Meng-Bo wrote:
>>> Some LEDs controllers are used with external pull-up for the interrupt
>>> line and the I2C lines, so we might need to enable a regulator to bring
>>> the lines into usable state.
>>
>> Not a property of this device.
>>
>>> Otherwise, this might cause spurious
>>> interrupts and reading from I2C will fail.
>>>
>>> Document support for "vddio-supply" that is enabled by the aw2013 driver
>>> so that the regulator gets enabled when needed.
>>>
>>> Signed-off-by: Lin, Meng-Bo <linmengbo0689@protonmail.com>
>>> ---
>>>  Documentation/devicetree/bindings/leds/leds-aw2013.yaml | 5 +++++
>>>  1 file changed, 5 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/leds/leds-aw2013.yaml b/Documentation/devicetree/bindings/leds/leds-aw2013.yaml
>>> index 08f3e1cfc1b1..79b69cf1d1fe 100644
>>> --- a/Documentation/devicetree/bindings/leds/leds-aw2013.yaml
>>> +++ b/Documentation/devicetree/bindings/leds/leds-aw2013.yaml
>>> @@ -23,6 +23,11 @@ properties:
>>>    vcc-supply:
>>>      description: Regulator providing power to the "VCC" pin.
>>>  
>>> +  vddio-supply:
>>> +    description: |
>>> +      Optional regulator that provides digital I/O voltage,
>>
>> NAK. I responded to your patch and you just send a v2 without explanation.
>>
>> The device does not have VDDIO pin, either.
>>
> 
> The power supply Lin is trying to add here is basically the "VIO1"
> example in "Figure 1 AW2013 Typical Application Circuit" on page 1 of
> the AW2013 datasheet [1]. The I2C pins and the interrupt output are both
> open-drain and therefore require external pull-up resistors, connected
> to a power supply that might not be always on.
> 
> Because of the open-drain pins AW2013 does indeed not have a dedicated
> input pin for the I/O supply voltage. However, it is still necessary to
> describe the power supply _somewhere_, to ensure that it is enabled when
> needed.
> 
> It is hard to model this properly but it's generally easiest to handle
> this inside the peripheral driver since it knows exactly when I2C and/or
> interrupt lines are currently needed or not. This situation is fairly
> common for I2C devices so there are several precedents, e.g.:
> 
>   1. cypress,tm2-touchkey.yaml: "vddio-supply"
>      https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=3e730ec11d51283ad62a98436967c01b718132ab
>   2. goodix,gt7375p.yaml: "mainboard-vddio-supply"
>      https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=1d18c1f3b7d938bdefc44289d137b4e6c7a3d502

Both are mistaken. How can you enumerate or autodetect a device if its
regulator pulling up I2C are not on? What's more, on I2C lines you could
have more devices, so you expect each of them having the supply?

These are properties of I2C controller, not the consumer. I2C controller
should enable any regulators necessary for the IO pins.

Best regards,
Krzysztof

Re: [PATCH v2 1/2] dt-bindings: leds: aw2013: Document vddio-supply

[Stephan Gerhold]

On Tue, Mar 21, 2023 at 07:42:37AM +0100, Krzysztof Kozlowski wrote:
> On 20/03/2023 19:59, Stephan Gerhold wrote:
> > On Mon, Mar 20, 2023 at 07:04:22PM +0100, Krzysztof Kozlowski wrote:
> >> On 20/03/2023 18:55, Lin, Meng-Bo wrote:
> >>> Some LEDs controllers are used with external pull-up for the interrupt
> >>> line and the I2C lines, so we might need to enable a regulator to bring
> >>> the lines into usable state.
> >>
> >> Not a property of this device.
> >>
> >>> Otherwise, this might cause spurious
> >>> interrupts and reading from I2C will fail.
> >>>
> >>> Document support for "vddio-supply" that is enabled by the aw2013 driver
> >>> so that the regulator gets enabled when needed.
> >>>
> >>> Signed-off-by: Lin, Meng-Bo <linmengbo0689@protonmail.com>
> >>> ---
> >>>  Documentation/devicetree/bindings/leds/leds-aw2013.yaml | 5 +++++
> >>>  1 file changed, 5 insertions(+)
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/leds/leds-aw2013.yaml b/Documentation/devicetree/bindings/leds/leds-aw2013.yaml
> >>> index 08f3e1cfc1b1..79b69cf1d1fe 100644
> >>> --- a/Documentation/devicetree/bindings/leds/leds-aw2013.yaml
> >>> +++ b/Documentation/devicetree/bindings/leds/leds-aw2013.yaml
> >>> @@ -23,6 +23,11 @@ properties:
> >>>    vcc-supply:
> >>>      description: Regulator providing power to the "VCC" pin.
> >>>  
> >>> +  vddio-supply:
> >>> +    description: |
> >>> +      Optional regulator that provides digital I/O voltage,
> >>
> >> NAK. I responded to your patch and you just send a v2 without explanation.
> >>
> >> The device does not have VDDIO pin, either.
> >>
> > 
> > The power supply Lin is trying to add here is basically the "VIO1"
> > example in "Figure 1 AW2013 Typical Application Circuit" on page 1 of
> > the AW2013 datasheet [1]. The I2C pins and the interrupt output are both
> > open-drain and therefore require external pull-up resistors, connected
> > to a power supply that might not be always on.
> > 
> > Because of the open-drain pins AW2013 does indeed not have a dedicated
> > input pin for the I/O supply voltage. However, it is still necessary to
> > describe the power supply _somewhere_, to ensure that it is enabled when
> > needed.
> > 
> > It is hard to model this properly but it's generally easiest to handle
> > this inside the peripheral driver since it knows exactly when I2C and/or
> > interrupt lines are currently needed or not. This situation is fairly
> > common for I2C devices so there are several precedents, e.g.:
> > 
> >   1. cypress,tm2-touchkey.yaml: "vddio-supply"
> >      https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=3e730ec11d51283ad62a98436967c01b718132ab
> >   2. goodix,gt7375p.yaml: "mainboard-vddio-supply"
> >      https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=1d18c1f3b7d938bdefc44289d137b4e6c7a3d502
> 
> Both are mistaken. How can you enumerate or autodetect a device if its
> regulator pulling up I2C are not on?

You don't. By design I2C does not support enumeration or autodetection.
Nothing we implement in software can change that.

I2C devices have all sorts of requirements before they show up on the
bus at all (power supplies, enable GPIOs, clocks, ...). All these are
currently modelled as part of the consumer IC.

> What's more, on I2C lines you could have more devices, so you expect
> each of them having the supply?

Yes, I don't think this is a problem since it's typical for regulators
to be shared. If at least one of the I2C devices is active, the bus will
be active as well.

> These are properties of I2C controller, not the consumer. I2C controller
> should enable any regulators necessary for the IO pins.

In general I agree with you here. But as I mentioned already there is
usually more than just the I2C I/O lines. For AW2013 there is at least
also the open-drain interrupt line. On other ICs there could also be
arbitrary GPIO lines that are used in open-drain mode. Those are
completely unrelated to the I2C controller.

Do you have any suggestions how to handle the power supply for those?

IMO for interrupts lines the pull-up I/O supply is hardly a property of
the interrupt controller. It just cares that a line switches from high
to low. It's not exactly a property of the consumer IC either. However,
since operating the interrupt line in open-drain mode is part of the
consumer IC specification I would say that the I/O supply for interrupt
lines is better described on the consumer side.

For sake of completeness we could additionally describe the supply for
the I2C lines on the I2C controller, but then we still need this patch
or something else for the interrupt lines.

Thanks,
Stephan

Re: [PATCH v2 1/2] dt-bindings: leds: aw2013: Document vddio-supply

[Rob Herring]

On Tue, Mar 21, 2023 at 09:21:36PM +0100, Stephan Gerhold wrote:
> On Tue, Mar 21, 2023 at 07:42:37AM +0100, Krzysztof Kozlowski wrote:
> > On 20/03/2023 19:59, Stephan Gerhold wrote:
> > > On Mon, Mar 20, 2023 at 07:04:22PM +0100, Krzysztof Kozlowski wrote:
> > >> On 20/03/2023 18:55, Lin, Meng-Bo wrote:
> > >>> Some LEDs controllers are used with external pull-up for the interrupt
> > >>> line and the I2C lines, so we might need to enable a regulator to bring
> > >>> the lines into usable state.
> > >>
> > >> Not a property of this device.
> > >>
> > >>> Otherwise, this might cause spurious
> > >>> interrupts and reading from I2C will fail.
> > >>>
> > >>> Document support for "vddio-supply" that is enabled by the aw2013 driver
> > >>> so that the regulator gets enabled when needed.
> > >>>
> > >>> Signed-off-by: Lin, Meng-Bo <linmengbo0689@protonmail.com>
> > >>> ---
> > >>>  Documentation/devicetree/bindings/leds/leds-aw2013.yaml | 5 +++++
> > >>>  1 file changed, 5 insertions(+)
> > >>>
> > >>> diff --git a/Documentation/devicetree/bindings/leds/leds-aw2013.yaml b/Documentation/devicetree/bindings/leds/leds-aw2013.yaml
> > >>> index 08f3e1cfc1b1..79b69cf1d1fe 100644
> > >>> --- a/Documentation/devicetree/bindings/leds/leds-aw2013.yaml
> > >>> +++ b/Documentation/devicetree/bindings/leds/leds-aw2013.yaml
> > >>> @@ -23,6 +23,11 @@ properties:
> > >>>    vcc-supply:
> > >>>      description: Regulator providing power to the "VCC" pin.
> > >>>  
> > >>> +  vddio-supply:
> > >>> +    description: |
> > >>> +      Optional regulator that provides digital I/O voltage,
> > >>
> > >> NAK. I responded to your patch and you just send a v2 without explanation.
> > >>
> > >> The device does not have VDDIO pin, either.
> > >>
> > > 
> > > The power supply Lin is trying to add here is basically the "VIO1"
> > > example in "Figure 1 AW2013 Typical Application Circuit" on page 1 of
> > > the AW2013 datasheet [1]. The I2C pins and the interrupt output are both
> > > open-drain and therefore require external pull-up resistors, connected
> > > to a power supply that might not be always on.
> > > 
> > > Because of the open-drain pins AW2013 does indeed not have a dedicated
> > > input pin for the I/O supply voltage. However, it is still necessary to
> > > describe the power supply _somewhere_, to ensure that it is enabled when
> > > needed.
> > > 
> > > It is hard to model this properly but it's generally easiest to handle
> > > this inside the peripheral driver since it knows exactly when I2C and/or
> > > interrupt lines are currently needed or not. This situation is fairly
> > > common for I2C devices so there are several precedents, e.g.:
> > > 
> > >   1. cypress,tm2-touchkey.yaml: "vddio-supply"
> > >      https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=3e730ec11d51283ad62a98436967c01b718132ab
> > >   2. goodix,gt7375p.yaml: "mainboard-vddio-supply"
> > >      https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=1d18c1f3b7d938bdefc44289d137b4e6c7a3d502
> > 
> > Both are mistaken. How can you enumerate or autodetect a device if its
> > regulator pulling up I2C are not on?
> 
> You don't. By design I2C does not support enumeration or autodetection.
> Nothing we implement in software can change that.
> 
> I2C devices have all sorts of requirements before they show up on the
> bus at all (power supplies, enable GPIOs, clocks, ...). All these are
> currently modelled as part of the consumer IC.
> 
> > What's more, on I2C lines you could have more devices, so you expect
> > each of them having the supply?
> 
> Yes, I don't think this is a problem since it's typical for regulators
> to be shared. If at least one of the I2C devices is active, the bus will
> be active as well.
> 
> > These are properties of I2C controller, not the consumer. I2C controller
> > should enable any regulators necessary for the IO pins.
> 
> In general I agree with you here. But as I mentioned already there is
> usually more than just the I2C I/O lines. For AW2013 there is at least
> also the open-drain interrupt line. On other ICs there could also be
> arbitrary GPIO lines that are used in open-drain mode. Those are
> completely unrelated to the I2C controller.
> 
> Do you have any suggestions how to handle the power supply for those?
> 
> IMO for interrupts lines the pull-up I/O supply is hardly a property of
> the interrupt controller. It just cares that a line switches from high
> to low. It's not exactly a property of the consumer IC either. However,
> since operating the interrupt line in open-drain mode is part of the
> consumer IC specification I would say that the I/O supply for interrupt
> lines is better described on the consumer side.
> 
> For sake of completeness we could additionally describe the supply for
> the I2C lines on the I2C controller, but then we still need this patch
> or something else for the interrupt lines.

I think a supply on the device side is fine here. Just be clear in the 
description about its purpose. We have much worse abuses than this 
(random bus clocks added to SoC devices).

Rob