iio: humidity: si7020: Check device property for skipping reset in probe
Subsystems (3)
v2 cover iio: humidity: si7020: Check device property for skipping reset in probe
I2C commands issued after the SI7020 is starting up or after reset can potentially upset the startup sequence. Therefore, the host needs to wait for the startup sequence to finish before issuing further i2c commands. This is impractical in cases where the SI7020 is on a shared bus or behind a mux, which may switch channels at any time (generating I2C traffic). Therefore, check for a device property that indicates that the driver should skip resetting the device when probing.
Changes since v1:
- Fix dt binding document
Eddie James (2):
dt-bindings: iio: humidity: Add si7020 bindings
iio: humidity: si7020: Check device property for skipping reset in probe
.../bindings/iio/humidity/silabs,si7020.yaml | 47 +++++++++++++++++++ .../devicetree/bindings/trivial-devices.yaml | 2 - drivers/iio/humidity/si7020.c | 14 +++--- 3 files changed, 55 insertions(+), 8 deletions(-) create mode 100644 Documentation/devicetree/bindings/iio/humidity/silabs,si7020.yaml
Signature · 1 line
2.27.0
PATCH v2 1/2 dt-bindings: iio: humidity: Add si7020 bindings
Document the si7020 bindings with a new "silabs,skip-reset" property.
--- .../bindings/iio/humidity/silabs,si7020.yaml | 47 +++++++++++++++++++ .../devicetree/bindings/trivial-devices.yaml | 2 - 2 files changed, 47 insertions(+), 2 deletions(-) create mode 100644 Documentation/devicetree/bindings/iio/humidity/silabs,si7020.yaml
Documentation/devicetree/bindings/iio/humidity/silabs,si7020.yaml new file +47 -0
@@ -0,0 +1,47 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/iio/humidity/silabs,si7020.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: SI7020 humidity + temperature sensor + +maintainers: + - David Barksdale <[email protected]> + +description: | + The Silicon Labs Si7013/20/21 Relative Humidity and Temperature Sensors + are i2c devices which have an identical programming interface for + measuring relative humidity and temperature. + +properties: + compatible: + const: silabs,si7020 + + reg: + maxItems: 1 + + siliabs,skip-reset: + $ref: /schemas/types.yaml#/definitions/flag + description: + Disables resetting of the device during probe + +required: + - compatible + - reg + +additionalProperties: false + +examples: + - | + i2c0 { + #address-cells = <1>; + #size-cells = <0>; + + si7021-a20@40 { + silabs,skip-reset; + compatible = "silabs,si7020"; + reg = <0x40>; + }; + }; +...
Documentation/devicetree/bindings/trivial-devices.yaml +0 -2
@@ -317,8 +317,6 @@ properties: - sensortek,stk8ba50 # SGX Sensortech VZ89X Sensors - sgx,vz89x - # Relative Humidity and Temperature Sensors - - silabs,si7020 # Skyworks SKY81452: Six-Channel White LED Driver with Touch Panel Bias Supply - skyworks,sky81452 # Socionext SynQuacer TPM MMIO module
Signature · 1 line
2.27.0
Re: [PATCH v2 1/2] dt-bindings: iio: humidity: Add si7020 bindings
Quoted 15 lines · On Thu, 12 May 2022 11:20:19 -0500 Eddie James <[email protected]> wrote:
Document the si7020 bindings with a new "silabs,skip-reset" property.
Signed-off-by Eddie James <[email protected]>--- .../bindings/iio/humidity/silabs,si7020.yaml | 47 +++++++++++++++++++ .../devicetree/bindings/trivial-devices.yaml | 2 - 2 files changed, 47 insertions(+), 2 deletions(-) create mode 100644 Documentation/devicetree/bindings/iio/humidity/silabs,si7020.yamlDocumentation/devicetree/bindings/iio/humidity/silabs,si7020.yaml new file +0
-0
Documentation/devicetree/bindings/iio/humidity/silabs,si7020.yaml line 10
Quoted 10 lines
Documentation/devicetree/bindings/iio/humidity/silabs,si7020.yaml new file +10
-0@@ -0,0 +1,47 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/iio/humidity/silabs,si7020.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: SI7020 humidity + temperature sensor + +maintainers: + - David Barksdale <[email protected]>
At least cc David if you are going to commit him to maintaining this binding :) +CC David at that address.
Quoted 50 lines
Documentation/devicetree/bindings/iio/humidity/silabs,si7020.yaml new file +37
-0+ +description: | + The Silicon Labs Si7013/20/21 Relative Humidity and Temperature Sensors + are i2c devices which have an identical programming interface for + measuring relative humidity and temperature. + +properties: + compatible: + const: silabs,si7020 + + reg: + maxItems: 1 + + siliabs,skip-reset: + $ref: /schemas/types.yaml#/definitions/flag + description: + Disables resetting of the device during probe + +required: + - compatible + - reg + +additionalProperties: false + +examples: + - | + i2c0 { + #address-cells = <1>; + #size-cells = <0>; + + si7021-a20@40 { + silabs,skip-reset; + compatible = "silabs,si7020"; + reg = <0x40>; + }; + }; +...Documentation/devicetree/bindings/trivial-devices.yaml +0
-2@@ -317,8 +317,6 @@ properties: - sensortek,stk8ba50 # SGX Sensortech VZ89X Sensors - sgx,vz89x - # Relative Humidity and Temperature Sensors - - silabs,si7020 # Skyworks SKY81452: Six-Channel White LED Driver with Touch Panel Bias Supply - skyworks,sky81452 # Socionext SynQuacer TPM MMIO module
Re: [PATCH v2 1/2] dt-bindings: iio: humidity: Add si7020 bindings
Quoted 22 lines · Jonathan Cameron
Quoted 19 lines · On Thu, 12 May 2022 11:20:19 -0500 Eddie James <[email protected]> wrote:
Document the si7020 bindings with a new "silabs,skip-reset" property.
Signed-off-by Eddie James <[email protected]>--- .../bindings/iio/humidity/silabs,si7020.yaml | 47 +++++++++++++++++++ .../devicetree/bindings/trivial-devices.yaml | 2 - 2 files changed, 47 insertions(+), 2 deletions(-)create mode 100644
Documentation/devicetree/bindings/iio/humidity/silabs,si7020.yamlDocumentation/devicetree/bindings/iio/humidity/silabs,si7020.yaml new file +4
-0@@ -0,0 +1,47 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/iio/humidity/silabs,si7020.yaml#
Quoted 8 lines
Documentation/devicetree/bindings/iio/humidity/silabs,si7020.yaml line 10
Quoted 6 lines
Documentation/devicetree/bindings/iio/humidity/silabs,si7020.yaml new file +6
-0+$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: SI7020 humidity + temperature sensor + +maintainers: + - David Barksdale <[email protected]>At least cc David if you are going to commit him to maintaining this binding :) +CC David at that address.
Yes, my mail to him for v1 was undeliverable... I guess I should put myself instead.
Quoted 51 lines
Quoted 51 lines
Documentation/devicetree/bindings/iio/humidity/silabs,si7020.yaml new file +37
-0+ +description: | + The Silicon Labs Si7013/20/21 Relative Humidity and Temperature Sensors + are i2c devices which have an identical programming interface for + measuring relative humidity and temperature. + +properties: + compatible: + const: silabs,si7020 + + reg: + maxItems: 1 + + siliabs,skip-reset: + $ref: /schemas/types.yaml#/definitions/flag + description: + Disables resetting of the device during probe + +required: + - compatible + - reg + +additionalProperties: false + +examples: + - | + i2c0 { + #address-cells = <1>; + #size-cells = <0>; + + si7021-a20@40 { + silabs,skip-reset; + compatible = "silabs,si7020"; + reg = <0x40>; + }; + }; +...Documentation/devicetree/bindings/trivial-devices.yaml +0
-2@@ -317,8 +317,6 @@ properties: - sensortek,stk8ba50 # SGX Sensortech VZ89X Sensors - sgx,vz89x - # Relative Humidity and Temperature Sensors - - silabs,si7020 # Skyworks SKY81452: Six-Channel White LED Driver with Touch Panel Bias Supply - skyworks,sky81452 # Socionext SynQuacer TPM MMIO module
Re: [PATCH v2 1/2] dt-bindings: iio: humidity: Add si7020 bindings
Quoted 33 lines · On Thu, 12 May 2022 12:08:57 -0500 Eddie James <[email protected]> wrote:
Quoted 22 lines · Jonathan Cameron
Quoted 19 lines · On Thu, 12 May 2022 11:20:19 -0500 Eddie James <[email protected]> wrote:
Document the si7020 bindings with a new "silabs,skip-reset" property.
Signed-off-by Eddie James <[email protected]>--- .../bindings/iio/humidity/silabs,si7020.yaml | 47 +++++++++++++++++++ .../devicetree/bindings/trivial-devices.yaml | 2 - 2 files changed, 47 insertions(+), 2 deletions(-)create mode 100644
Documentation/devicetree/bindings/iio/humidity/silabs,si7020.yamlDocumentation/devicetree/bindings/iio/humidity/silabs,si7020.yaml new file +4
-0@@ -0,0 +1,47 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/iio/humidity/silabs,si7020.yaml#Quoted 8 lines
Documentation/devicetree/bindings/iio/humidity/silabs,si7020.yaml line 10
Quoted 6 lines
Documentation/devicetree/bindings/iio/humidity/silabs,si7020.yaml new file +6
-0+$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: SI7020 humidity + temperature sensor + +maintainers: + - David Barksdale <[email protected]>At least cc David if you are going to commit him to maintaining this binding :) +CC David at that address.
Quoted 3 lines
Yes, my mail to him for v1 was undeliverable... I guess I should put myself instead.
That's the best answer :)
(backup is to rely on the fallback which is me but I'd definitely rather bindings had attentive maintainers where possible!).
Quoted 52 lines
Quoted 52 lines
Quoted 52 lines
Documentation/devicetree/bindings/iio/humidity/silabs,si7020.yaml new file +37
-0+ +description: | + The Silicon Labs Si7013/20/21 Relative Humidity and Temperature Sensors + are i2c devices which have an identical programming interface for + measuring relative humidity and temperature. + +properties: + compatible: + const: silabs,si7020 + + reg: + maxItems: 1 + + siliabs,skip-reset: + $ref: /schemas/types.yaml#/definitions/flag + description: + Disables resetting of the device during probe + +required: + - compatible + - reg + +additionalProperties: false + +examples: + - | + i2c0 { + #address-cells = <1>; + #size-cells = <0>; + + si7021-a20@40 { + silabs,skip-reset; + compatible = "silabs,si7020"; + reg = <0x40>; + }; + }; +...Documentation/devicetree/bindings/trivial-devices.yaml +0
-2@@ -317,8 +317,6 @@ properties: - sensortek,stk8ba50 # SGX Sensortech VZ89X Sensors - sgx,vz89x - # Relative Humidity and Temperature Sensors - - silabs,si7020 # Skyworks SKY81452: Six-Channel White LED Driver with Touch Panel Bias Supply - skyworks,sky81452 # Socionext SynQuacer TPM MMIO module
Re: [PATCH v2 1/2] dt-bindings: iio: humidity: Add si7020 bindings
In reply to Eddie JamesQuoted 46 lines · Eddie James
Document the si7020 bindings with a new "silabs,skip-reset" property.
Signed-off-by Eddie James <[email protected]>--- .../bindings/iio/humidity/silabs,si7020.yaml | 47 +++++++++++++++++++ .../devicetree/bindings/trivial-devices.yaml | 2 - 2 files changed, 47 insertions(+), 2 deletions(-) create mode 100644 Documentation/devicetree/bindings/iio/humidity/silabs,si7020.yamlDocumentation/devicetree/bindings/iio/humidity/silabs,si7020.yaml new file +31
-0@@ -0,0 +1,47 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/iio/humidity/silabs,si7020.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: SI7020 humidity + temperature sensor + +maintainers: + - David Barksdale <[email protected]> + +description: | + The Silicon Labs Si7013/20/21 Relative Humidity and Temperature Sensors + are i2c devices which have an identical programming interface for + measuring relative humidity and temperature. + +properties: + compatible: + const: silabs,si7020 + + reg: + maxItems: 1 + + siliabs,skip-reset: + $ref: /schemas/types.yaml#/definitions/flag + description: + Disables resetting of the device during probe + +required: + - compatible + - reg
Documentation/devicetree/bindings/iio/humidity/silabs,si7020.yaml line 41
Quoted 10 lines
Documentation/devicetree/bindings/iio/humidity/silabs,si7020.yaml new file +10
-0+ +additionalProperties: false + +examples: + - | + i2c0 { + #address-cells = <1>; + #size-cells = <0>; + + si7021-a20@40 {
Same comments as your v1. Generic node name, wrong property skip-reset (implementation specific) and so on...
Give some time for review, before resending.
Best regards,
Krzysztof
PATCH v2 2/2 iio: humidity: si7020: Check device property for skipping reset in probe
In reply to Eddie JamesI2C commands issued after the SI7020 is starting up or after reset can potentially upset the startup sequence. Therefore, the host needs to wait for the startup sequence to finish before issuing further i2c commands. This is impractical in cases where the SI7020 is on a shared bus or behind a mux, which may switch channels at any time (generating I2C traffic). Therefore, check for a device property that indicates that the driver should skip resetting the device when probing.
--- drivers/iio/humidity/si7020.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-)
drivers/iio/humidity/si7020.c +8 -6
@@ -115,12 +115,14 @@ static int si7020_probe(struct i2c_client *client, I2C_FUNC_SMBUS_READ_WORD_DATA))return -EOPNOTSUPP ; -/* Reset device, loads default settings. */ - ret =i2c_smbus_write_byte (client,SI7020CMD_RESET ); -if (ret <0 ) -return ret; -/* Wait the maximum power-up time after software reset. */ -msleep (15 ); +if (!device_property_read_bool (&client->dev,"silabs,skip-reset" )) { +/* Reset device, loads default settings. */ + ret =i2c_smbus_write_byte (client,SI7020CMD_RESET ); +if (ret <0 ) +return ret; +/* Wait the maximum power-up time after software reset. */ +msleep (15 ); + } indio_dev =devm_iio_device_alloc (&client->dev,sizeof (*data));if (!indio_dev)
Signature · 1 line
2.27.0
Quoted 8 lines · On Thu, 12 May 2022 11:20:18 -0500 Eddie James <[email protected]> wrote:
I2C commands issued after the SI7020 is starting up or after reset can potentially upset the startup sequence. Therefore, the host needs to wait for the startup sequence to finish before issuing further i2c commands. This is impractical in cases where the SI7020 is on a shared bus or behind a mux, which may switch channels at any time (generating I2C traffic). Therefore, check for a device property that indicates that the driver should skip resetting the device when probing.
Why not lock the bus? It's not ideal, but then not resetting and hence potentially ending up in an unknown state isn't great either.
Jonathan
Quoted 14 lines
Changes since v1:
- Fix dt binding documentEddie James (2):
dt-bindings: iio: humidity: Add si7020 bindings
iio: humidity: si7020: Check device property for skipping reset in probe.../bindings/iio/humidity/silabs,si7020.yaml | 47 +++++++++++++++++++ .../devicetree/bindings/trivial-devices.yaml | 2 - drivers/iio/humidity/si7020.c | 14 +++--- 3 files changed, 55 insertions(+), 8 deletions(-) create mode 100644 Documentation/devicetree/bindings/iio/humidity/silabs,si7020.yaml
Quoted 11 lines · Jonathan Cameron
Quoted 8 lines · On Thu, 12 May 2022 11:20:18 -0500 Eddie James <[email protected]> wrote:
I2C commands issued after the SI7020 is starting up or after reset can potentially upset the startup sequence. Therefore, the host needs to wait for the startup sequence to finish before issuing further i2c commands. This is impractical in cases where the SI7020 is on a shared bus or behind a mux, which may switch channels at any time (generating I2C traffic). Therefore, check for a device property that indicates that the driver should skip resetting the device when probing.
Quoted 2 lines
Why not lock the bus? It's not ideal, but then not resetting and hence potentially ending up in an unknown state isn't great either.
Agreed, but locking the bus doesn't work in the case where the chip is behind a mux. The mux core driver deselects the mux immediately after the transfer to reset the si7020, causing some i2c traffic, breaking the si7020. So it would also be a requirement to configure the mux to idle as-is... That's why I went with the optional skipping of the reset. Maybe I should add the bus lock too?
Thanks,
Eddie
Quoted 16 lines
Jonathan
Quoted 13 lines
Changes since v1:
- Fix dt binding documentEddie James (2):
dt-bindings: iio: humidity: Add si7020 bindings
iio: humidity: si7020: Check device property for skipping reset in probe.../bindings/iio/humidity/silabs,si7020.yaml | 47 +++++++++++++++++++ .../devicetree/bindings/trivial-devices.yaml | 2 - drivers/iio/humidity/si7020.c | 14 +++--- 3 files changed, 55 insertions(+), 8 deletions(-)create mode 100644
Documentation/devicetree/bindings/iio/humidity/silabs,si7020.yaml
Quoted 16 lines · On Thu, 12 May 2022 12:08:07 -0500 Eddie James <[email protected]> wrote:
Quoted 11 lines · Jonathan Cameron
Quoted 8 lines · On Thu, 12 May 2022 11:20:18 -0500 Eddie James <[email protected]> wrote:
I2C commands issued after the SI7020 is starting up or after reset can potentially upset the startup sequence. Therefore, the host needs to wait for the startup sequence to finish before issuing further i2c commands. This is impractical in cases where the SI7020 is on a shared bus or behind a mux, which may switch channels at any time (generating I2C traffic). Therefore, check for a device property that indicates that the driver should skip resetting the device when probing.
Quoted 2 lines
Why not lock the bus? It's not ideal, but then not resetting and hence potentially ending up in an unknown state isn't great either.
Quoted 7 lines
Agreed, but locking the bus doesn't work in the case where the chip is behind a mux. The mux core driver deselects the mux immediately after the transfer to reset the si7020, causing some i2c traffic, breaking the si7020. So it would also be a requirement to configure the mux to idle as-is... That's why I went with the optional skipping of the reset. Maybe I should add the bus lock too?
+Cc Peter and linux-i2c for advice as we should resolve any potential issue with the mux side of things rather than hiding it in the driver (if possible!)
Jonathan
Quoted 22 lines
Thanks,
Eddie
Quoted 16 lines
Jonathan
Quoted 13 lines
Changes since v1:
- Fix dt binding documentEddie James (2):
dt-bindings: iio: humidity: Add si7020 bindings
iio: humidity: si7020: Check device property for skipping reset in probe.../bindings/iio/humidity/silabs,si7020.yaml | 47 +++++++++++++++++++ .../devicetree/bindings/trivial-devices.yaml | 2 - drivers/iio/humidity/si7020.c | 14 +++--- 3 files changed, 55 insertions(+), 8 deletions(-)create mode 100644
Documentation/devicetree/bindings/iio/humidity/silabs,si7020.yaml
Hi!
Quoted 27 lines · Jonathan Cameron
Quoted 16 lines · On Thu, 12 May 2022 12:08:07 -0500 Eddie James <[email protected]> wrote:
Quoted 11 lines · Jonathan Cameron
Quoted 8 lines · On Thu, 12 May 2022 11:20:18 -0500 Eddie James <[email protected]> wrote:
I2C commands issued after the SI7020 is starting up or after reset can potentially upset the startup sequence. Therefore, the host needs to wait for the startup sequence to finish before issuing further i2c commands. This is impractical in cases where the SI7020 is on a shared bus or behind a mux, which may switch channels at any time (generating I2C traffic). Therefore, check for a device property that indicates that the driver should skip resetting the device when probing.
Quoted 2 lines
Why not lock the bus? It's not ideal, but then not resetting and hence potentially ending up in an unknown state isn't great either.
Quoted 7 lines
Agreed, but locking the bus doesn't work in the case where the chip is behind a mux. The mux core driver deselects the mux immediately after the transfer to reset the si7020, causing some i2c traffic, breaking the si7020. So it would also be a requirement to configure the mux to idle as-is... That's why I went with the optional skipping of the reset. Maybe I should add the bus lock too?
Quoted 3 lines
+Cc Peter and linux-i2c for advice as we should resolve any potential issue with the mux side of things rather than hiding it in the driver (if possible!)
IIUC, the chip in question cannot handle *any* action on the I2C bus for 15ms (or so) after a "soft reset", or something bad<tm> happens (or at least may happen).
If that's the case, then providing a means of skipping the reset is insufficient. If you don't lock the bus, you would need to *always* skip the reset, because you don't know for certain if something else does I2C xfers.
So, in order to make the soft reset not be totally dangerous even in a normal non-muxed environment, the bus must be locked for the 15ms.
However, Eddie is correct in that the I2C mux code may indeed do its muxing xfer right after the soft reset command. There is currently no way to avoid that muxing xfer. However, it should be noted that there are ways to mux an I2C bus without using xfers on the bus itself, so it's not problematic for *all* mux variants.
It can be debated if the problem should be worked around with extra dt properties like this, or if a capability should be added to delay a trailing muxing xfer.
I bet there are other broken chips that have drivers that do in fact lock the bus to give the chip a break, but then it all stumbles because of the unexpected noise if there's a (wrong kind of) mux in the mix.
Cheers,
Peter
Quoted 29 lines
Jonathan
Quoted 22 lines
Thanks,
Eddie
Quoted 16 lines
Jonathan
Quoted 13 lines
Changes since v1:
- Fix dt binding documentEddie James (2):
dt-bindings: iio: humidity: Add si7020 bindings
iio: humidity: si7020: Check device property for skipping reset in probe.../bindings/iio/humidity/silabs,si7020.yaml | 47 +++++++++++++++++++ .../devicetree/bindings/trivial-devices.yaml | 2 - drivers/iio/humidity/si7020.c | 14 +++--- 3 files changed, 55 insertions(+), 8 deletions(-)create mode 100644
Documentation/devicetree/bindings/iio/humidity/silabs,si7020.yaml
Jonathan Cameron
maintainerQuoted 52 lines · On Sat, 14 May 2022 00:48:51 +0200 Peter Rosin <[email protected]> wrote:
Hi!
Quoted 27 lines · Jonathan Cameron
Quoted 16 lines · On Thu, 12 May 2022 12:08:07 -0500 Eddie James <[email protected]> wrote:
Quoted 11 lines · Jonathan Cameron
Quoted 8 lines · On Thu, 12 May 2022 11:20:18 -0500 Eddie James <[email protected]> wrote:
I2C commands issued after the SI7020 is starting up or after reset can potentially upset the startup sequence. Therefore, the host needs to wait for the startup sequence to finish before issuing further i2c commands. This is impractical in cases where the SI7020 is on a shared bus or behind a mux, which may switch channels at any time (generating I2C traffic). Therefore, check for a device property that indicates that the driver should skip resetting the device when probing.
Quoted 2 lines
Why not lock the bus? It's not ideal, but then not resetting and hence potentially ending up in an unknown state isn't great either.
Quoted 7 lines
Agreed, but locking the bus doesn't work in the case where the chip is behind a mux. The mux core driver deselects the mux immediately after the transfer to reset the si7020, causing some i2c traffic, breaking the si7020. So it would also be a requirement to configure the mux to idle as-is... That's why I went with the optional skipping of the reset. Maybe I should add the bus lock too?
Quoted 3 lines
+Cc Peter and linux-i2c for advice as we should resolve any potential issue with the mux side of things rather than hiding it in the driver (if possible!)
IIUC, the chip in question cannot handle *any* action on the I2C bus for 15ms (or so) after a "soft reset", or something bad<tm> happens (or at least may happen).
If that's the case, then providing a means of skipping the reset is insufficient. If you don't lock the bus, you would need to *always* skip the reset, because you don't know for certain if something else does I2C xfers.
So, in order to make the soft reset not be totally dangerous even in a normal non-muxed environment, the bus must be locked for the 15ms.
However, Eddie is correct in that the I2C mux code may indeed do its muxing xfer right after the soft reset command. There is currently no way to avoid that muxing xfer. However, it should be noted that there are ways to mux an I2C bus without using xfers on the bus itself, so it's not problematic for *all* mux variants.
Quoted 8 lines
It can be debated if the problem should be worked around with extra dt properties like this, or if a capability should be added to delay a trailing muxing xfer.
I bet there are other broken chips that have drivers that do in fact lock the bus to give the chip a break, but then it all stumbles because of the unexpected noise if there's a (wrong kind of) mux in the mix.
Ok, so for now I think we need the bus lock for the reset + either a work around similar to this series, or additions to the i2c mux code to stop it doing a muxing xfer if the bus is locked?
Jonathan
Quoted 33 lines
Cheers,
PeterQuoted 29 lines
Jonathan
Quoted 22 lines
Thanks,
Eddie
Quoted 16 lines
Jonathan
Quoted 13 lines
Changes since v1:
- Fix dt binding documentEddie James (2):
dt-bindings: iio: humidity: Add si7020 bindings
iio: humidity: si7020: Check device property for skipping reset in probe.../bindings/iio/humidity/silabs,si7020.yaml | 47 +++++++++++++++++++ .../devicetree/bindings/trivial-devices.yaml | 2 - drivers/iio/humidity/si7020.c | 14 +++--- 3 files changed, 55 insertions(+), 8 deletions(-)create mode 100644
Documentation/devicetree/bindings/iio/humidity/silabs,si7020.yaml
Quoted 59 lines · Jonathan Cameron
Quoted 56 lines · On Sat, 14 May 2022 00:48:51 +0200 Peter Rosin <[email protected]> wrote:
Hi!
Quoted 27 lines · Jonathan Cameron
Quoted 16 lines · On Thu, 12 May 2022 12:08:07 -0500 Eddie James <[email protected]> wrote:
Quoted 11 lines · Jonathan Cameron
Quoted 8 lines · On Thu, 12 May 2022 11:20:18 -0500 Eddie James <[email protected]> wrote:
I2C commands issued after the SI7020 is starting up or after reset can potentially upset the startup sequence. Therefore, the host needs to wait for the startup sequence to finish before issuing further i2c commands. This is impractical in cases where the SI7020 is on a shared bus or behind a mux, which may switch channels at any time (generating I2C traffic). Therefore, check for a device property that indicates that the driver should skip resetting the device when probing.
Quoted 2 lines
Why not lock the bus? It's not ideal, but then not resetting and hence potentially ending up in an unknown state isn't great either.
Quoted 7 lines
Agreed, but locking the bus doesn't work in the case where the chip is behind a mux. The mux core driver deselects the mux immediately after the transfer to reset the si7020, causing some i2c traffic, breaking the si7020. So it would also be a requirement to configure the mux to idle as-is... That's why I went with the optional skipping of the reset. Maybe I should add the bus lock too?
Quoted 3 lines
+Cc Peter and linux-i2c for advice as we should resolve any potential issue with the mux side of things rather than hiding it in the driver (if possible!)
IIUC, the chip in question cannot handle *any* action on the I2C bus for 15ms (or so) after a "soft reset", or something bad<tm> happens (or at least may happen).
If that's the case, then providing a means of skipping the reset is insufficient. If you don't lock the bus, you would need to *always* skip the reset, because you don't know for certain if something else does I2C xfers.
So, in order to make the soft reset not be totally dangerous even in a normal non-muxed environment, the bus must be locked for the 15ms.
However, Eddie is correct in that the I2C mux code may indeed do its muxing xfer right after the soft reset command. There is currently no way to avoid that muxing xfer. However, it should be noted that there are ways to mux an I2C bus without using xfers on the bus itself, so it's not problematic for *all* mux variants.
It can be debated if the problem should be worked around with extra dt properties like this, or if a capability should be added to delay a trailing muxing xfer.
Quoted 8 lines
Quoted 4 lines
I bet there are other broken chips that have drivers that do in fact lock the bus to give the chip a break, but then it all stumbles because of the unexpected noise if there's a (wrong kind of) mux in the mix.
Ok, so for now I think we need the bus lock for the reset + either a work around similar to this series, or additions to the i2c mux code to stop it doing a muxing xfer if the bus is locked?
I think there might be cases where it might be valid to restore the mux directly after an xfer even if the mux is externally locked prior to the muxed xfer. But I'm not sure? In any case, it will be a bit convoluted for the mux code to remember that it might need to restore the mux later. And it will get even hairier when multiple levels of muxing is considered...
Maybe some kind of hook/callback that could be installed temporarily on the I2C adapter that is called right after the "real" xfer, where the driver could then make the needed mdelay call?
I.e.
1. lock the bus
2. install this new hook/callback
3. do an unlocked xfer, get notified and call mdelay
5. uninstall the hook/callback
6. unlock the bus
The hook/callback could be uninstalled automatically on unlock, then you would not need to handle multiple notifications. But then again, there is probably some existing framework that should be used that handles all than neatly and efficiently.
Me waves hand a bit...
Cheers,
Peter
Quoted 82 lines · Peter Rosin
Quoted 56 lines · Jonathan Cameron
Quoted 53 lines · On Sat, 14 May 2022 00:48:51 +0200 Peter Rosin <[email protected]> wrote:
Hi!
Quoted 25 lines · Jonathan Cameron
Quoted 15 lines · On Thu, 12 May 2022 12:08:07 -0500 Eddie James <[email protected]> wrote:
Quoted 11 lines · Jonathan Cameron
Quoted 8 lines · On Thu, 12 May 2022 11:20:18 -0500 Eddie James <[email protected]> wrote:
I2C commands issued after the SI7020 is starting up or after reset can potentially upset the startup sequence. Therefore, the host needs to wait for the startup sequence to finish before issuing further i2c commands. This is impractical in cases where the SI7020 is on a shared bus or behind a mux, which may switch channels at any time (generating I2C traffic). Therefore, check for a device property that indicates that the driver should skip resetting the device when probing.
Quoted 2 lines
Why not lock the bus? It's not ideal, but then not resetting and hence potentially ending up in an unknown state isn't great either.
Quoted 7 lines
Agreed, but locking the bus doesn't work in the case where the chip is behind a mux. The mux core driver deselects the mux immediately after the transfer to reset the si7020, causing some i2c traffic, breaking the si7020. So it would also be a requirement to configure the mux to idle as-is... That's why I went with the optional skipping of the reset. Maybe I should add the bus lock too?
Quoted 3 lines
+Cc Peter and linux-i2c for advice as we should resolve any potential issue with the mux side of things rather than hiding it in the driver (if possible!)
IIUC, the chip in question cannot handle *any* action on the I2C bus for 15ms (or so) after a "soft reset", or something bad<tm> happens (or at least may happen).
If that's the case, then providing a means of skipping the reset is insufficient. If you don't lock the bus, you would need to *always* skip the reset, because you don't know for certain if something else does I2C xfers.
So, in order to make the soft reset not be totally dangerous even in a normal non-muxed environment, the bus must be locked for the 15ms.
However, Eddie is correct in that the I2C mux code may indeed do its muxing xfer right after the soft reset command. There is currently no way to avoid that muxing xfer. However, it should be noted that there are ways to mux an I2C bus without using xfers on the bus itself, so it's not problematic for *all* mux variants.
It can be debated if the problem should be worked around with extra dt properties like this, or if a capability should be added to delay a trailing muxing xfer.
Quoted 7 lines
Quoted 4 lines
I bet there are other broken chips that have drivers that do in fact lock the bus to give the chip a break, but then it all stumbles because of the unexpected noise if there's a (wrong kind of) mux in the mix.
Ok, so for now I think we need the bus lock for the reset + either a work around similar to this series, or additions to the i2c mux code to stop it doing a muxing xfer if the bus is locked?
I think there might be cases where it might be valid to restore the mux directly after an xfer even if the mux is externally locked prior to the muxed xfer. But I'm not sure? In any case, it will be a bit convoluted for the mux code to remember that it might need to restore the mux later. And it will get even hairier when multiple levels of muxing is considered...
Maybe some kind of hook/callback that could be installed temporarily on the I2C adapter that is called right after the "real" xfer, where the driver could then make the needed mdelay call?
I.e.
1. lock the bus
2. install this new hook/callback
3. do an unlocked xfer, get notified and call mdelay
5. uninstall the hook/callback
6. unlock the bus
Quoted 4 lines
The hook/callback could be uninstalled automatically on unlock, then you would not need to handle multiple notifications. But then again, there is probably some existing framework that should be used that handles all than neatly and efficiently.
Hm, interesting. Sounds a bit complicated, though very flexible. For a less flexible, but less complex, approch, we could add a i2c_msg flag that says to do a delay in the core? And then si7020 could just submit a couple of raw messages rather than smbus... What do you think?
Thanks,
Eddie
Quoted 5 lines
Me waves hand a bit...
Cheers,
Peter
Quoted 81 lines · Eddie James
Quoted 79 lines · Peter Rosin
Quoted 53 lines · Jonathan Cameron
Quoted 50 lines · On Sat, 14 May 2022 00:48:51 +0200 Peter Rosin <[email protected]> wrote:
Hi!
Quoted 22 lines · Jonathan Cameron
Quoted 14 lines · On Thu, 12 May 2022 12:08:07 -0500 Eddie James <[email protected]> wrote:
Quoted 10 lines · Jonathan Cameron
Quoted 8 lines · On Thu, 12 May 2022 11:20:18 -0500 Eddie James <[email protected]> wrote:
I2C commands issued after the SI7020 is starting up or after reset can potentially upset the startup sequence. Therefore, the host needs to wait for the startup sequence to finish before issuing further i2c commands. This is impractical in cases where the SI7020 is on a shared bus or behind a mux, which may switch channels at any time (generating I2C traffic). Therefore, check for a device property that indicates that the driver should skip resetting the device when probing.
Quoted 2 lines
Why not lock the bus? It's not ideal, but then not resetting and hence potentially ending up in an unknown state isn't great either.
Quoted 6 lines
Agreed, but locking the bus doesn't work in the case where the chip is behind a mux. The mux core driver deselects the mux immediately after the transfer to reset the si7020, causing some i2c traffic, breaking the si7020. So it would also be a requirement to configure the mux to idle as-is... That's why I went with the optional skipping of the reset. Maybe I should add the bus lock too?
Quoted 3 lines
+Cc Peter and linux-i2c for advice as we should resolve any potential issue with the mux side of things rather than hiding it in the driver (if possible!)
IIUC, the chip in question cannot handle *any* action on the I2C bus for 15ms (or so) after a "soft reset", or something bad<tm> happens (or at least may happen).
If that's the case, then providing a means of skipping the reset is insufficient. If you don't lock the bus, you would need to *always* skip the reset, because you don't know for certain if something else does I2C xfers.
So, in order to make the soft reset not be totally dangerous even in a normal non-muxed environment, the bus must be locked for the 15ms.
However, Eddie is correct in that the I2C mux code may indeed do its muxing xfer right after the soft reset command. There is currently no way to avoid that muxing xfer. However, it should be noted that there are ways to mux an I2C bus without using xfers on the bus itself, so it's not problematic for *all* mux variants.
It can be debated if the problem should be worked around with extra dt properties like this, or if a capability should be added to delay a trailing muxing xfer.
Quoted 7 lines
Quoted 4 lines
I bet there are other broken chips that have drivers that do in fact lock the bus to give the chip a break, but then it all stumbles because of the unexpected noise if there's a (wrong kind of) mux in the mix.
Ok, so for now I think we need the bus lock for the reset + either a work around similar to this series, or additions to the i2c mux code to stop it doing a muxing xfer if the bus is locked?
I think there might be cases where it might be valid to restore the mux directly after an xfer even if the mux is externally locked prior to the muxed xfer. But I'm not sure? In any case, it will be a bit convoluted for the mux code to remember that it might need to restore the mux later. And it will get even hairier when multiple levels of muxing is considered...
Maybe some kind of hook/callback that could be installed temporarily on the I2C adapter that is called right after the "real" xfer, where the driver could then make the needed mdelay call?
I.e.
1. lock the bus
2. install this new hook/callback
3. do an unlocked xfer, get notified and call mdelay
5. uninstall the hook/callback
6. unlock the bus
Quoted 7 lines
Quoted 4 lines
The hook/callback could be uninstalled automatically on unlock, then you would not need to handle multiple notifications. But then again, there is probably some existing framework that should be used that handles all than neatly and efficiently.
Hm, interesting. Sounds a bit complicated, though very flexible. For a less flexible, but less complex, approch, we could add a
i2c_msgflag that says to do a delay in the core? And then si7020 could just submit a couple of raw messages rather than smbus... What do you think?
Um, nevermind... that would require changes in all the bus drivers. I'll look into implementing the hook/callback.
Thanks,
Eddie
Quoted 15 lines
Thanks,
Eddie
Quoted 5 lines
Me waves hand a bit...
Cheers,
Peter
Quoted 11 lines · Jonathan Cameron
Quoted 8 lines · On Thu, 12 May 2022 11:20:18 -0500 Eddie James <[email protected]> wrote:
I2C commands issued after the SI7020 is starting up or after reset can potentially upset the startup sequence. Therefore, the host needs to wait for the startup sequence to finish before issuing further i2c commands. This is impractical in cases where the SI7020 is on a shared bus or behind a mux, which may switch channels at any time (generating I2C traffic). Therefore, check for a device property that indicates that the driver should skip resetting the device when probing.
Quoted 2 lines
Why not lock the bus? It's not ideal, but then not resetting and hence potentially ending up in an unknown state isn't great either.
Also, I should mention that in our case we can rely on the power on reset, so the device should be in a known state.
Eddie
Quoted 16 lines
Jonathan
Quoted 13 lines
Changes since v1:
- Fix dt binding documentEddie James (2):
dt-bindings: iio: humidity: Add si7020 bindings
iio: humidity: si7020: Check device property for skipping reset in probe.../bindings/iio/humidity/silabs,si7020.yaml | 47 +++++++++++++++++++ .../devicetree/bindings/trivial-devices.yaml | 2 - drivers/iio/humidity/si7020.c | 14 +++--- 3 files changed, 55 insertions(+), 8 deletions(-)create mode 100644
Documentation/devicetree/bindings/iio/humidity/silabs,si7020.yaml
Jonathan Cameron
maintainerQuoted 12 lines · On Thu, 12 May 2022 14:11:06 -0500 Eddie James <[email protected]> wrote:
Quoted 11 lines · Jonathan Cameron
Quoted 8 lines · On Thu, 12 May 2022 11:20:18 -0500 Eddie James <[email protected]> wrote:
I2C commands issued after the SI7020 is starting up or after reset can potentially upset the startup sequence. Therefore, the host needs to wait for the startup sequence to finish before issuing further i2c commands. This is impractical in cases where the SI7020 is on a shared bus or behind a mux, which may switch channels at any time (generating I2C traffic). Therefore, check for a device property that indicates that the driver should skip resetting the device when probing.
Quoted 6 lines
Quoted 2 lines
Why not lock the bus? It's not ideal, but then not resetting and hence potentially ending up in an unknown state isn't great either.
Also, I should mention that in our case we can rely on the power on reset, so the device should be in a known state.
Until someone unbinds and rebinds the driver...
It's very hard to have any guarantees once users are involved :)
Jonathan
Quoted 20 lines
Eddie
Quoted 16 lines
Jonathan
Quoted 13 lines
Changes since v1:
- Fix dt binding documentEddie James (2):
dt-bindings: iio: humidity: Add si7020 bindings
iio: humidity: si7020: Check device property for skipping reset in probe.../bindings/iio/humidity/silabs,si7020.yaml | 47 +++++++++++++++++++ .../devicetree/bindings/trivial-devices.yaml | 2 - drivers/iio/humidity/si7020.c | 14 +++--- 3 files changed, 55 insertions(+), 8 deletions(-)create mode 100644
Documentation/devicetree/bindings/iio/humidity/silabs,si7020.yaml