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

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

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
  

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
>
  

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
  

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

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.
  

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

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
  

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
  

> >>> 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 :/
  

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
  

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

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
>