| Message ID | 20231115035753.925534-2-chris.packham@alliedtelesis.co.nz |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v6,1/2] dt-bindings: i2c: add bus-reset-gpios property | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success |
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 >
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) --------------------------------------
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(+)