[PATCH v6 0/2] i2c: bus-reset-gpios

[PATCH v6 0/2] i2c: bus-reset-gpios

[Chris Packham]

This series adds the ability to associate a gpio with an I2C bus so that
downstream devices can be brought out of reset when the host controller is
probed.

v5: https://lore.kernel.org/lkml/20231027033104.1348921-1-chris.packham@alliedtelesis.co.nz/

The conversation sparked from v5 was heading in the direction of this needing
to be more generic. So I've moved the changes up to the i2c core, which is
about as generic as I'm comfortable with.

Chris Packham (2):
  dt-bindings: i2c: add bus-reset-gpios property
  i2c: add an optional bus-reset-gpios property

 Documentation/devicetree/bindings/i2c/i2c.txt |  8 ++++
 drivers/i2c/i2c-core-base.c                   | 39 +++++++++++++++++++
 include/linux/i2c.h                           |  3 ++
 3 files changed, 50 insertions(+)

-- 
2.42.0

[PATCH v6 1/2] dt-bindings: i2c: add bus-reset-gpios property

[Chris Packham]

Add bus-reset-gpios and bus-reset-duration-us properties to the binding
description for i2c busses. These can be used to describe hardware where
a common reset GPIO is connected to all downstream devices on and I2C
bus. This reset will be asserted then released before the downstream
devices on the bus are probed.

Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---

Notes:
    I expect the first reaction to this will be a request to convert the
    binding to dtschema. I can attempt such a conversion but given it's one
    of the more core bindings I expect others may have strong opinions. I
    didn't want to start a conversion without hearing those opinions (or if
    I could get away without doing the conversion). It's also likely to spin
    off a whole lot of work to bring existing device trees into line.
    
    Changes in v6:
    - Retarget changes at the i2c core instead of an individual driver
    Changes in v5:
    - Rename reset-gpios and reset-duration-us to bus-reset-gpios and
      bus-reset-duration-us as requested by Wolfram
    Changes in v4:
    - Add r-by from Krzysztof
    Changes in v3:
    - Rename reset-delay-us to reset-duration-us to better reflect its
      purpose
    - Add default: for reset-duration-us
    - Add description: for reset-gpios
    Changes in v2:
    - Update commit message
    - Add reset-delay-us property

 Documentation/devicetree/bindings/i2c/i2c.txt | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/Documentation/devicetree/bindings/i2c/i2c.txt b/Documentation/devicetree/bindings/i2c/i2c.txt
index fc3dd7ec0445..3f95d71b9985 100644
--- a/Documentation/devicetree/bindings/i2c/i2c.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c.txt
@@ -99,6 +99,14 @@ wants to support one of the below features, it should adapt these bindings.
 	indicates that the system is accessible via this bus as an endpoint for
 	MCTP over I2C transport.
 
+- bus-reset-gpios:
+	GPIO pin providing a common reset for all downstream devices. This GPIO
+	will be asserted then released before the downstream devices are probed.
+
+- bus-reset-duration-us:
+	Reset duration in us.
+	default: 1
+
 Required properties (per child device)
 --------------------------------------
 
-- 
2.42.0

Re: [PATCH v6 1/2] dt-bindings: i2c: add bus-reset-gpios property

[Krzysztof Kozlowski]

On 15/11/2023 04:57, Chris Packham wrote:
> Add bus-reset-gpios and bus-reset-duration-us properties to the binding
> description for i2c busses. These can be used to describe hardware where
> a common reset GPIO is connected to all downstream devices on and I2C
> bus. This reset will be asserted then released before the downstream
> devices on the bus are probed.
> 
> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> ---
> 

...

> 
>  Documentation/devicetree/bindings/i2c/i2c.txt | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/i2c/i2c.txt b/Documentation/devicetree/bindings/i2c/i2c.txt
> index fc3dd7ec0445..3f95d71b9985 100644
> --- a/Documentation/devicetree/bindings/i2c/i2c.txt
> +++ b/Documentation/devicetree/bindings/i2c/i2c.txt
> @@ -99,6 +99,14 @@ wants to support one of the below features, it should adapt these bindings.
>  	indicates that the system is accessible via this bus as an endpoint for
>  	MCTP over I2C transport.
>  
> +- bus-reset-gpios:
> +	GPIO pin providing a common reset for all downstream devices. This GPIO
> +	will be asserted then released before the downstream devices are probed.

I initially reviewed it, but did not think enough about it. After more
consideration, I believe this is not a property of the I2C bus
controller. This is a property of each device, even if the GPIO is the same.

Linux kernel already supports shared GPIO, so you only need
enable-ref-counting on it.

Putting it into the controller bindings looks like solving OS issue with
incorrect hardware description.

Best regards,
Krzysztof

Re: [PATCH v6 1/2] dt-bindings: i2c: add bus-reset-gpios property

[Chris Packham]

Hi Krystof,

On 16/11/23 10:29, Krzysztof Kozlowski wrote:
> On 15/11/2023 04:57, Chris Packham wrote:
>> Add bus-reset-gpios and bus-reset-duration-us properties to the binding
>> description for i2c busses. These can be used to describe hardware where
>> a common reset GPIO is connected to all downstream devices on and I2C
>> bus. This reset will be asserted then released before the downstream
>> devices on the bus are probed.
>>
>> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
>> ---
>>
> ...
>
>>   Documentation/devicetree/bindings/i2c/i2c.txt | 8 ++++++++
>>   1 file changed, 8 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/i2c/i2c.txt b/Documentation/devicetree/bindings/i2c/i2c.txt
>> index fc3dd7ec0445..3f95d71b9985 100644
>> --- a/Documentation/devicetree/bindings/i2c/i2c.txt
>> +++ b/Documentation/devicetree/bindings/i2c/i2c.txt
>> @@ -99,6 +99,14 @@ wants to support one of the below features, it should adapt these bindings.
>>   	indicates that the system is accessible via this bus as an endpoint for
>>   	MCTP over I2C transport.
>>   
>> +- bus-reset-gpios:
>> +	GPIO pin providing a common reset for all downstream devices. This GPIO
>> +	will be asserted then released before the downstream devices are probed.
> I initially reviewed it, but did not think enough about it. After more
> consideration, I believe this is not a property of the I2C bus
> controller. This is a property of each device, even if the GPIO is the same.
>
> Linux kernel already supports shared GPIO, so you only need
> enable-ref-counting on it.

That's the kind of breadcrumb I need. Although I can't see 
enable-ref-counting as any kind of DT property. Do you mean 
GPIOD_FLAGS_BIT_NONEXCLUSIVE?

> Putting it into the controller bindings looks like solving OS issue with
> incorrect hardware description.
Yes that's entirely whats happening here.
> Best regards,
> Krzysztof
>

Re: [PATCH v6 1/2] dt-bindings: i2c: add bus-reset-gpios property

[Krzysztof Kozlowski]

On 15/11/2023 22:53, Chris Packham wrote:
> Hi Krystof,
> 
> On 16/11/23 10:29, Krzysztof Kozlowski wrote:
>> On 15/11/2023 04:57, Chris Packham wrote:
>>> Add bus-reset-gpios and bus-reset-duration-us properties to the binding
>>> description for i2c busses. These can be used to describe hardware where
>>> a common reset GPIO is connected to all downstream devices on and I2C
>>> bus. This reset will be asserted then released before the downstream
>>> devices on the bus are probed.
>>>
>>> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
>>> ---
>>>
>> ...
>>
>>>   Documentation/devicetree/bindings/i2c/i2c.txt | 8 ++++++++
>>>   1 file changed, 8 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/i2c/i2c.txt b/Documentation/devicetree/bindings/i2c/i2c.txt
>>> index fc3dd7ec0445..3f95d71b9985 100644
>>> --- a/Documentation/devicetree/bindings/i2c/i2c.txt
>>> +++ b/Documentation/devicetree/bindings/i2c/i2c.txt
>>> @@ -99,6 +99,14 @@ wants to support one of the below features, it should adapt these bindings.
>>>   	indicates that the system is accessible via this bus as an endpoint for
>>>   	MCTP over I2C transport.
>>>   
>>> +- bus-reset-gpios:
>>> +	GPIO pin providing a common reset for all downstream devices. This GPIO
>>> +	will be asserted then released before the downstream devices are probed.
>> I initially reviewed it, but did not think enough about it. After more
>> consideration, I believe this is not a property of the I2C bus
>> controller. This is a property of each device, even if the GPIO is the same.
>>
>> Linux kernel already supports shared GPIO, so you only need
>> enable-ref-counting on it.
> 
> That's the kind of breadcrumb I need. Although I can't see 
> enable-ref-counting as any kind of DT property. Do you mean 
> GPIOD_FLAGS_BIT_NONEXCLUSIVE?

It's not a feature or property of Devicetree, but missing feature of OS.

Best regards,
Krzysztof

Re: [PATCH v6 1/2] dt-bindings: i2c: add bus-reset-gpios property

[wsa]

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


> > Putting it into the controller bindings looks like solving OS issue with
> > incorrect hardware description.
> Yes that's entirely whats happening here.

So, this series can be dropped?


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

Re: [PATCH v6 1/2] dt-bindings: i2c: add bus-reset-gpios property

[Chris Packham]

On 20/12/23 06:02, wsa@kernel.org wrote:
>>> Putting it into the controller bindings looks like solving OS issue with
>>> incorrect hardware description.
>> Yes that's entirely whats happening here.
> So, this series can be dropped?
>
I personally would like to see it accepted but it seems there are 
objections to this approach. I've yet to come up with anything better to 
offer as an alternative.

Re: [PATCH v6 1/2] dt-bindings: i2c: add bus-reset-gpios property

[wsa]

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


> I personally would like to see it accepted but it seems there are 
> objections to this approach. I've yet to come up with anything better to 
> offer as an alternative.

I see. Thanks for the heads up!


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

Re: [PATCH v6 1/2] dt-bindings: i2c: add bus-reset-gpios property

[Andi Shyti]

Hi,

> > I personally would like to see it accepted but it seems there are 
> > objections to this approach. I've yet to come up with anything better to 
> > offer as an alternative.
> 
> I see. Thanks for the heads up!

I'm also inclined to have this merged. A real fix might take
time.

Myself I have developed a prototype for what has been discussed
with Krzysztof, but I don't know how much time it will take to
get things done.

Andi

Re: [PATCH v6 1/2] dt-bindings: i2c: add bus-reset-gpios property

[Krzysztof Kozlowski]

On 20/12/2023 00:25, Andi Shyti wrote:
> Hi,
> 
>>> I personally would like to see it accepted but it seems there are 
>>> objections to this approach. I've yet to come up with anything better to 
>>> offer as an alternative.
>>
>> I see. Thanks for the heads up!
> 
> I'm also inclined to have this merged. A real fix might take
> time.

NAK

If you intend to merge it, then please carry:

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

The patchset is wrong and made of wrong reasons. It claimed GPIO cannot
be shared, which is simply not true.

> 
> Myself I have developed a prototype for what has been discussed
> with Krzysztof, but I don't know how much time it will take to
> get things done.


Best regards,
Krzysztof

Re: [PATCH v6 1/2] dt-bindings: i2c: add bus-reset-gpios property

[wsa]

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


> >>> I personally would like to see it accepted but it seems there are 
> >>> objections to this approach. I've yet to come up with anything better to 
> >>> offer as an alternative.
> >>
> >> I see. Thanks for the heads up!
> > 
> > I'm also inclined to have this merged. A real fix might take
> > time.
> 
> NAK
> 
> If you intend to merge it, then please carry:

No worries. If this is "abusing" DT, then it is not going to be merged
by me. I am sorry for Chris, but sometimes simple problems create quite
some fuzz because Linux hardware abstractions has not foreseen certain
use cases. Or the APIs dealing with them didn't forsee that. We have
been there a lot of times :/


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

Re: [PATCH v6 1/2] dt-bindings: i2c: add bus-reset-gpios property

[Krzysztof Kozlowski]

On 20/12/2023 12:03, wsa@kernel.org wrote:
> 
>>>>> I personally would like to see it accepted but it seems there are 
>>>>> objections to this approach. I've yet to come up with anything better to 
>>>>> offer as an alternative.
>>>>
>>>> I see. Thanks for the heads up!
>>>
>>> I'm also inclined to have this merged. A real fix might take
>>> time.
>>
>> NAK
>>
>> If you intend to merge it, then please carry:
> 
> No worries. If this is "abusing" DT, then it is not going to be merged
> by me. I am sorry for Chris, but sometimes simple problems create quite
> some fuzz because Linux hardware abstractions has not foreseen certain
> use cases. Or the APIs dealing with them didn't forsee that. We have
> been there a lot of times :/

I need the same solution for WSA884x speaker, for which Mark rejected
simple shared GPIO (even though it would work there), so I am trying to
solve it. It's basically the same case. Now, I am waiting on answer from
Sean Anderson whether he continued his work on reset-gpios controller
from two years ago. Rob wanted handling reset-gpios by generic reset
framework, which would solve these simple cases, here and mine, nicely.

Best regards,
Krzysztof

Re: [PATCH v6 1/2] dt-bindings: i2c: add bus-reset-gpios property

[wsa]

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


> from two years ago. Rob wanted handling reset-gpios by generic reset
> framework, which would solve these simple cases, here and mine, nicely.

That sounds good. Good luck with it!


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

Re: [PATCH v6 1/2] dt-bindings: i2c: add bus-reset-gpios property

[Andi Shyti]

Hi Krzysztof,

On Wed, Dec 20, 2023 at 08:22:38AM +0100, Krzysztof Kozlowski wrote:
> On 20/12/2023 00:25, Andi Shyti wrote:
> > Hi,
> > 
> >>> I personally would like to see it accepted but it seems there are 
> >>> objections to this approach. I've yet to come up with anything better to 
> >>> offer as an alternative.
> >>
> >> I see. Thanks for the heads up!
> > 
> > I'm also inclined to have this merged. A real fix might take
> > time.
> 
> NAK
> 
> If you intend to merge it, then please carry:
> 
> Nacked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

ehehe... too much drama here :-)

I know you nacked this patch and of course won't be taken
anywhere.

I was actually referring to Chris previous patch rather than this
one.

Andi

> The patchset is wrong and made of wrong reasons. It claimed GPIO cannot
> be shared, which is simply not true.
> 
> > 
> > Myself I have developed a prototype for what has been discussed
> > with Krzysztof, but I don't know how much time it will take to
> > get things done.
> 
> 
> Best regards,
> Krzysztof
> 

[PATCH v6 2/2] i2c: add an optional bus-reset-gpios property

[Chris Packham]

Some hardware designs have a GPIO used to control the reset of all the
devices on and I2C bus. It's not possible for every child node to
declare a reset-gpios property as only the first device probed would be
able to successfully request it. Represent this kind of hardware design
by associating the bus-reset-gpios with the parent I2C bus. The reset
line will be released prior to the child I2C devices being probed.

Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---

Notes:
    Changes in v6:
    - Retarget changes at the i2c core instead of an individual driver
    Changes in v5:
    - Rename reset-gpios and reset-duration-us to bus-reset-gpios and
      bus-reset-duration-us as requested by Wolfram
    Changes in v4:
    - Add missing gpio/consumer.h
    - use fsleep() for enforcing reset-duration
    Changes in v3:
    - Rename reset-delay to reset-duration
    - Use reset-duration-us property to control the reset pulse rather than
      delaying after the reset
    Changes in v2:
    - Add a property to cover the length of delay after releasing the reset
      GPIO
    - Use dev_err_probe() when requesing the GPIO fails

 drivers/i2c/i2c-core-base.c | 39 +++++++++++++++++++++++++++++++++++++
 include/linux/i2c.h         |  3 +++
 2 files changed, 42 insertions(+)

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 60746652fd52..d7f53272487b 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -1468,6 +1468,39 @@ int i2c_handle_smbus_host_notify(struct i2c_adapter *adap, unsigned short addr)
 }
 EXPORT_SYMBOL_GPL(i2c_handle_smbus_host_notify);
 
+static int i2c_setup_bus_reset_gpio(struct i2c_adapter *adap)
+{
+	int res;
+
+	adap->reset_gpios = devm_gpiod_get_array_optional(&adap->dev, "bus-reset", GPIOD_OUT_HIGH);
+	if (IS_ERR(adap->reset_gpios))
+		return dev_err_probe(&adap->dev, PTR_ERR(adap->reset_gpios),
+				     "Cannot get reset gpio\n");
+	res = device_property_read_u32(&adap->dev, "bus-reset-duration-us", &adap->reset_duration);
+	if (res)
+		adap->reset_duration = 1;
+
+	return 0;
+}
+
+static void i2c_deassert_bus_reset_gpio(struct i2c_adapter *adap)
+{
+	unsigned long *values;
+
+	if (!adap->reset_gpios)
+		return;
+
+	values = bitmap_zalloc(adap->reset_gpios->ndescs, GFP_KERNEL);
+	if (!values)
+		return;
+
+	gpiod_set_array_value_cansleep(adap->reset_gpios->ndescs,
+			adap->reset_gpios->desc, adap->reset_gpios->info,
+			values);
+
+	bitmap_free(values);
+}
+
 static int i2c_register_adapter(struct i2c_adapter *adap)
 {
 	int res = -EINVAL;
@@ -1521,6 +1554,10 @@ static int i2c_register_adapter(struct i2c_adapter *adap)
 	if (res)
 		goto out_reg;
 
+	res = i2c_setup_bus_reset_gpio(adap);
+	if (res)
+		goto out_reg;
+
 	device_enable_async_suspend(&adap->dev);
 	pm_runtime_no_callbacks(&adap->dev);
 	pm_suspend_ignore_children(&adap->dev, true);
@@ -1539,6 +1576,8 @@ static int i2c_register_adapter(struct i2c_adapter *adap)
 		dev_warn(&adap->dev,
 			 "Failed to create compatibility class link\n");
 #endif
+	/* bring downstream devices out of reset */
+	i2c_deassert_bus_reset_gpio(adap);
 
 	/* create pre-declared device nodes */
 	of_i2c_register_devices(adap);
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 0dae9db27538..1110a49dcdaf 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -746,6 +746,9 @@ struct i2c_adapter {
 
 	struct irq_domain *host_notify_domain;
 	struct regulator *bus_regulator;
+
+	struct gpio_descs *reset_gpios;
+	u32 reset_duration;
 };
 #define to_i2c_adapter(d) container_of(d, struct i2c_adapter, dev)
 
-- 
2.42.0