| Message ID | 20241018100338.19420-2-wojciech.siudy@nokia.com |
|---|---|
| State | New |
| Headers | show |
| Series | pca954x: Add DT bindings and driver changes for reset after timeout | expand |
Hi! > If you have a reset GPIO for the mux, then why wouldn't just use it > on timeout? Because we cannot do that on every board. The reset GPIO is provided to ensure, that we have reset signal de-asserted during initialisation. You might have connected other devices to the same reset line so this must be a configurable option. > What happens if you timeout and don't have this property? Just give up? This would be the case just like before introducting this change. Some aplications might do other attempt, the bus driver can try recovery. Unfortunately common reset line for multiple chips is not a rare situation. > Does the timeout time need to be configurable? The timeout we are talking about is error code returned from function pca954x_reg_write(), which calls smbus xfer that is out of this driver scope so there is nothing we can configure. Regards, Wojciech
On Sat, Oct 19, 2024 at 11:09:43AM +0000, Wojciech Siudy (Nokia) wrote: > Hi! > > > If you have a reset GPIO for the mux, then why wouldn't just use it > > on timeout? > > Because we cannot do that on every board. The reset GPIO is provided to > ensure, that we have reset signal de-asserted during initialisation. > You might have connected other devices to the same reset line so this > must be a configurable option. > > > What happens if you timeout and don't have this property? Just > give up? > > This would be the case just like before introducting this change. Some > aplications might do other attempt, the bus driver can try recovery. > Unfortunately common reset line for multiple chips is not a rare > situation. And Linux handles it well now. This is reset of the I2C devices (children), not the controller, right? If so, then: 1. It's not a property of the controller, 2. Linux already handles it - switch to reset controller framework. Best regards, Krzysztof
Hello, > This is reset of the I2C devices (children), not the controller, right? Right, the mux is child device. > switch to reset controller framework Please note that I left the function pca954x_reset_deassert() unchanged, just moved it up and implemented two corresponding ones. Do you mean that we should get rid of gpiod_set_value_cansleep(), because the reset controller will handle it? I can agree but this is topic for another submission since there we change when the reset is done, not the way we achieve that. Regards, Wojciech
On 24/10/2024 09:05, Wojciech Siudy (Nokia) wrote: > Hello, > >> This is reset of the I2C devices (children), not the controller, right? > Right, the mux is child device. Not sure to which part I replied to. I have DT 200 emails in inbox per day, I respond to less but still more than I can track. Keep some context. I suspect you are adding reset for some other device, not for the one here. > >> switch to reset controller framework > Please note that I left the function pca954x_reset_deassert() unchanged, > just moved it up and implemented two corresponding ones. > Do you mean that we should get rid of gpiod_set_value_cansleep(), > because the reset controller will handle it? I can agree but this is topic > for another submission since there we change when the reset is done, > not the way we achieve that. No, I meant I suspect you place property in inappropriate binding to avoid shared GPIO problem. Best regards, Krzysztof
> I suspect you are adding reset for some other device, not for the one here. No, I reset pac954x chip in pca954x driver. > I suspect you place property in inappropriate binding Many boards provide reset-gpio for this very device, but it was used only to deassert the reset during initialization, so shared gpio line was not a problem until now. New binding is to reset later in runtime if necessary only if explicitly specified. Best regards, Wojciech
On 24/10/2024 11:18, Wojciech Siudy (Nokia) wrote:
> No, I reset pac954x chip in pca954x driver.
How useful is such context? That's the last reply here, I am not going
to waste more time on such style.
All my earlier comments stay valid. You have shared reset and you want
some hacks to avoid that problem. I gave you the solution.
Best regards,
Krzysztof
diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.yaml b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.yaml index 9aa0585200c9..37882a5a8c87 100644 --- a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.yaml +++ b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.yaml @@ -63,6 +63,11 @@ properties: necessary for example, if there are several multiplexers on the bus and the devices behind them use same I2C addresses. + i2c-mux-timeout-reset: + type: boolean + description: Sends reset pulse if channel selection or deselection times + out. Do not use if other chips share the same reset line. + idle-state: description: if present, overrides i2c-mux-idle-disconnect $ref: /schemas/mux/mux-controller.yaml#/properties/idle-state @@ -88,6 +93,9 @@ properties: register activates a channel to detect a stuck high fault. On fault the channel is isolated from the upstream bus. +dependencies: + i2c-mux-timeout-reset: [ reset-gpios ] + required: - compatible - reg @@ -146,6 +154,9 @@ examples: interrupt-parent = <&ipic>; interrupts = <17 IRQ_TYPE_LEVEL_LOW>; interrupt-controller; + reset-gpios = <&gpio1 27 1>; + i2c-mux-idle-disconnect; + i2c-mux-timeout-reset; #interrupt-cells = <2>; i2c@2 {
For cases when the mux shares reset line with other chips we cannot use it always when channel selection or deselection times out, because it could break them without proper init/probe. The property is necessary, because reset lines are board-specific. Signed-off-by: Wojciech Siudy <wojciech.siudy@nokia.com> --- Changelog: v5: * Declare dependency of a new property --- .../devicetree/bindings/i2c/i2c-mux-pca954x.yaml | 11 +++++++++++ 1 file changed, 11 insertions(+)