All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 0/2] iio: humidity: si7020: Check device property for skipping reset in probe
@ 2022-05-12 16:20 Eddie James
  2022-05-12 16:20 ` [PATCH v2 1/2] dt-bindings: iio: humidity: Add si7020 bindings Eddie James
                   ` (2 more replies)
  0 siblings, 3 replies; 17+ messages in thread
From: Eddie James @ 2022-05-12 16:20 UTC (permalink / raw)
  To: linux-iio
  Cc: devicetree, linux-kernel, krzysztof.kozlowski+dt, robh+dt, lars,
	jic23, miltonm, eajames

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

-- 
2.27.0


^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH v2 1/2] dt-bindings: iio: humidity: Add si7020 bindings
  2022-05-12 16:20 [PATCH v2 0/2] iio: humidity: si7020: Check device property for skipping reset in probe Eddie James
@ 2022-05-12 16:20 ` Eddie James
  2022-05-12 16:51   ` Jonathan Cameron
  2022-05-13  8:55   ` Krzysztof Kozlowski
  2022-05-12 16:20 ` [PATCH v2 2/2] iio: humidity: si7020: Check device property for skipping reset in probe Eddie James
  2022-05-12 16:48 ` [PATCH v2 0/2] " Jonathan Cameron
  2 siblings, 2 replies; 17+ messages in thread
From: Eddie James @ 2022-05-12 16:20 UTC (permalink / raw)
  To: linux-iio
  Cc: devicetree, linux-kernel, krzysztof.kozlowski+dt, robh+dt, lars,
	jic23, miltonm, eajames

Document the si7020 bindings with a new "silabs,skip-reset" property.

Signed-off-by: Eddie James <eajames@linux.ibm.com>
---
 .../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

diff --git a/Documentation/devicetree/bindings/iio/humidity/silabs,si7020.yaml b/Documentation/devicetree/bindings/iio/humidity/silabs,si7020.yaml
new file mode 100644
index 000000000000..9bee010f8d56
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/humidity/silabs,si7020.yaml
@@ -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 <dbarksdale@uplogix.com>
+
+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>;
+      };
+    };
+...
diff --git a/Documentation/devicetree/bindings/trivial-devices.yaml b/Documentation/devicetree/bindings/trivial-devices.yaml
index e5295faef52f..47a00b478867 100644
--- a/Documentation/devicetree/bindings/trivial-devices.yaml
+++ b/Documentation/devicetree/bindings/trivial-devices.yaml
@@ -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
-- 
2.27.0


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH v2 2/2] iio: humidity: si7020: Check device property for skipping reset in probe
  2022-05-12 16:20 [PATCH v2 0/2] iio: humidity: si7020: Check device property for skipping reset in probe Eddie James
  2022-05-12 16:20 ` [PATCH v2 1/2] dt-bindings: iio: humidity: Add si7020 bindings Eddie James
@ 2022-05-12 16:20 ` Eddie James
  2022-05-12 16:48 ` [PATCH v2 0/2] " Jonathan Cameron
  2 siblings, 0 replies; 17+ messages in thread
From: Eddie James @ 2022-05-12 16:20 UTC (permalink / raw)
  To: linux-iio
  Cc: devicetree, linux-kernel, krzysztof.kozlowski+dt, robh+dt, lars,
	jic23, miltonm, eajames

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.

Signed-off-by: Eddie James <eajames@linux.ibm.com>
---
 drivers/iio/humidity/si7020.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/iio/humidity/si7020.c b/drivers/iio/humidity/si7020.c
index ab6537f136ba..49f6a1b1f5c4 100644
--- a/drivers/iio/humidity/si7020.c
+++ b/drivers/iio/humidity/si7020.c
@@ -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)
-- 
2.27.0


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 0/2] iio: humidity: si7020: Check device property for skipping reset in probe
  2022-05-12 16:20 [PATCH v2 0/2] iio: humidity: si7020: Check device property for skipping reset in probe Eddie James
  2022-05-12 16:20 ` [PATCH v2 1/2] dt-bindings: iio: humidity: Add si7020 bindings Eddie James
  2022-05-12 16:20 ` [PATCH v2 2/2] iio: humidity: si7020: Check device property for skipping reset in probe Eddie James
@ 2022-05-12 16:48 ` Jonathan Cameron
  2022-05-12 17:08   ` Eddie James
  2022-05-12 19:11   ` Eddie James
  2 siblings, 2 replies; 17+ messages in thread
From: Jonathan Cameron @ 2022-05-12 16:48 UTC (permalink / raw)
  To: Eddie James
  Cc: linux-iio, devicetree, linux-kernel, krzysztof.kozlowski+dt,
	robh+dt, lars, jic23, miltonm

On Thu, 12 May 2022 11:20:18 -0500
Eddie James <eajames@linux.ibm.com> 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

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


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 1/2] dt-bindings: iio: humidity: Add si7020 bindings
  2022-05-12 16:20 ` [PATCH v2 1/2] dt-bindings: iio: humidity: Add si7020 bindings Eddie James
@ 2022-05-12 16:51   ` Jonathan Cameron
  2022-05-12 17:08     ` Eddie James
  2022-05-13  8:55   ` Krzysztof Kozlowski
  1 sibling, 1 reply; 17+ messages in thread
From: Jonathan Cameron @ 2022-05-12 16:51 UTC (permalink / raw)
  To: Eddie James
  Cc: linux-iio, devicetree, linux-kernel, krzysztof.kozlowski+dt,
	robh+dt, lars, jic23, miltonm, David Barksdale

On Thu, 12 May 2022 11:20:19 -0500
Eddie James <eajames@linux.ibm.com> wrote:

> Document the si7020 bindings with a new "silabs,skip-reset" property.
> 
> Signed-off-by: Eddie James <eajames@linux.ibm.com>
> ---
>  .../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
> 
> diff --git a/Documentation/devicetree/bindings/iio/humidity/silabs,si7020.yaml b/Documentation/devicetree/bindings/iio/humidity/silabs,si7020.yaml
> new file mode 100644
> index 000000000000..9bee010f8d56
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/humidity/silabs,si7020.yaml
> @@ -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 <dbarksdale@uplogix.com>

At least cc David if you are going to commit him to maintaining this binding :)
+CC David at that address.

> +
> +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>;
> +      };
> +    };
> +...
> diff --git a/Documentation/devicetree/bindings/trivial-devices.yaml b/Documentation/devicetree/bindings/trivial-devices.yaml
> index e5295faef52f..47a00b478867 100644
> --- a/Documentation/devicetree/bindings/trivial-devices.yaml
> +++ b/Documentation/devicetree/bindings/trivial-devices.yaml
> @@ -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


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 0/2] iio: humidity: si7020: Check device property for skipping reset in probe
  2022-05-12 16:48 ` [PATCH v2 0/2] " Jonathan Cameron
@ 2022-05-12 17:08   ` Eddie James
  2022-05-13 16:45     ` Jonathan Cameron
  2022-05-12 19:11   ` Eddie James
  1 sibling, 1 reply; 17+ messages in thread
From: Eddie James @ 2022-05-12 17:08 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: linux-iio, devicetree, linux-kernel, krzysztof.kozlowski+dt,
	robh+dt, lars, jic23, miltonm


On 5/12/22 11:48, Jonathan Cameron wrote:
> On Thu, 12 May 2022 11:20:18 -0500
> Eddie James <eajames@linux.ibm.com> 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.


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


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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 1/2] dt-bindings: iio: humidity: Add si7020 bindings
  2022-05-12 16:51   ` Jonathan Cameron
@ 2022-05-12 17:08     ` Eddie James
  2022-05-13 16:47       ` Jonathan Cameron
  0 siblings, 1 reply; 17+ messages in thread
From: Eddie James @ 2022-05-12 17:08 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: linux-iio, devicetree, linux-kernel, krzysztof.kozlowski+dt,
	robh+dt, lars, jic23, miltonm, David Barksdale


On 5/12/22 11:51, Jonathan Cameron wrote:
> On Thu, 12 May 2022 11:20:19 -0500
> Eddie James <eajames@linux.ibm.com> wrote:
>
>> Document the si7020 bindings with a new "silabs,skip-reset" property.
>>
>> Signed-off-by: Eddie James <eajames@linux.ibm.com>
>> ---
>>   .../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
>>
>> diff --git a/Documentation/devicetree/bindings/iio/humidity/silabs,si7020.yaml b/Documentation/devicetree/bindings/iio/humidity/silabs,si7020.yaml
>> new file mode 100644
>> index 000000000000..9bee010f8d56
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/iio/humidity/silabs,si7020.yaml
>> @@ -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 <dbarksdale@uplogix.com>
> 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.


>
>> +
>> +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>;
>> +      };
>> +    };
>> +...
>> diff --git a/Documentation/devicetree/bindings/trivial-devices.yaml b/Documentation/devicetree/bindings/trivial-devices.yaml
>> index e5295faef52f..47a00b478867 100644
>> --- a/Documentation/devicetree/bindings/trivial-devices.yaml
>> +++ b/Documentation/devicetree/bindings/trivial-devices.yaml
>> @@ -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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 0/2] iio: humidity: si7020: Check device property for skipping reset in probe
  2022-05-12 16:48 ` [PATCH v2 0/2] " Jonathan Cameron
  2022-05-12 17:08   ` Eddie James
@ 2022-05-12 19:11   ` Eddie James
  2022-05-14 13:40     ` Jonathan Cameron
  1 sibling, 1 reply; 17+ messages in thread
From: Eddie James @ 2022-05-12 19:11 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: linux-iio, devicetree, linux-kernel, krzysztof.kozlowski+dt,
	robh+dt, lars, jic23, miltonm


On 5/12/22 11:48, Jonathan Cameron wrote:
> On Thu, 12 May 2022 11:20:18 -0500
> Eddie James <eajames@linux.ibm.com> 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.


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


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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 1/2] dt-bindings: iio: humidity: Add si7020 bindings
  2022-05-12 16:20 ` [PATCH v2 1/2] dt-bindings: iio: humidity: Add si7020 bindings Eddie James
  2022-05-12 16:51   ` Jonathan Cameron
@ 2022-05-13  8:55   ` Krzysztof Kozlowski
  1 sibling, 0 replies; 17+ messages in thread
From: Krzysztof Kozlowski @ 2022-05-13  8:55 UTC (permalink / raw)
  To: Eddie James, linux-iio
  Cc: devicetree, linux-kernel, krzysztof.kozlowski+dt, robh+dt, lars,
	jic23, miltonm

On 12/05/2022 18:20, Eddie James wrote:
> Document the si7020 bindings with a new "silabs,skip-reset" property.
> 
> Signed-off-by: Eddie James <eajames@linux.ibm.com>
> ---
>  .../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
> 
> diff --git a/Documentation/devicetree/bindings/iio/humidity/silabs,si7020.yaml b/Documentation/devicetree/bindings/iio/humidity/silabs,si7020.yaml
> new file mode 100644
> index 000000000000..9bee010f8d56
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/humidity/silabs,si7020.yaml
> @@ -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 <dbarksdale@uplogix.com>
> +
> +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 {

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 0/2] iio: humidity: si7020: Check device property for skipping reset in probe
  2022-05-12 17:08   ` Eddie James
@ 2022-05-13 16:45     ` Jonathan Cameron
  2022-05-13 22:48       ` Peter Rosin
  0 siblings, 1 reply; 17+ messages in thread
From: Jonathan Cameron @ 2022-05-13 16:45 UTC (permalink / raw)
  To: Eddie James
  Cc: linux-iio, devicetree, linux-kernel, krzysztof.kozlowski+dt,
	robh+dt, lars, jic23, miltonm, Peter Rosin, linux-i2c

On Thu, 12 May 2022 12:08:07 -0500
Eddie James <eajames@linux.ibm.com> wrote:

> On 5/12/22 11:48, Jonathan Cameron wrote:
> > On Thu, 12 May 2022 11:20:18 -0500
> > Eddie James <eajames@linux.ibm.com> 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.  
> 
> 
> 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




> 
> Thanks,
> 
> Eddie
> 
> 
> >
> > Jonathan
> >  
> >> 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
> >>  


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 1/2] dt-bindings: iio: humidity: Add si7020 bindings
  2022-05-12 17:08     ` Eddie James
@ 2022-05-13 16:47       ` Jonathan Cameron
  0 siblings, 0 replies; 17+ messages in thread
From: Jonathan Cameron @ 2022-05-13 16:47 UTC (permalink / raw)
  To: Eddie James
  Cc: linux-iio, devicetree, linux-kernel, krzysztof.kozlowski+dt,
	robh+dt, lars, jic23, miltonm, David Barksdale

On Thu, 12 May 2022 12:08:57 -0500
Eddie James <eajames@linux.ibm.com> wrote:

> On 5/12/22 11:51, Jonathan Cameron wrote:
> > On Thu, 12 May 2022 11:20:19 -0500
> > Eddie James <eajames@linux.ibm.com> wrote:
> >  
> >> Document the si7020 bindings with a new "silabs,skip-reset" property.
> >>
> >> Signed-off-by: Eddie James <eajames@linux.ibm.com>
> >> ---
> >>   .../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
> >>
> >> diff --git a/Documentation/devicetree/bindings/iio/humidity/silabs,si7020.yaml b/Documentation/devicetree/bindings/iio/humidity/silabs,si7020.yaml
> >> new file mode 100644
> >> index 000000000000..9bee010f8d56
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/iio/humidity/silabs,si7020.yaml
> >> @@ -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 <dbarksdale@uplogix.com>  
> > 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.
> 

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

> 
> >  
> >> +
> >> +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>;
> >> +      };
> >> +    };
> >> +...
> >> diff --git a/Documentation/devicetree/bindings/trivial-devices.yaml b/Documentation/devicetree/bindings/trivial-devices.yaml
> >> index e5295faef52f..47a00b478867 100644
> >> --- a/Documentation/devicetree/bindings/trivial-devices.yaml
> >> +++ b/Documentation/devicetree/bindings/trivial-devices.yaml
> >> @@ -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  


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 0/2] iio: humidity: si7020: Check device property for skipping reset in probe
  2022-05-13 16:45     ` Jonathan Cameron
@ 2022-05-13 22:48       ` Peter Rosin
  2022-05-14 13:43         ` Jonathan Cameron
  0 siblings, 1 reply; 17+ messages in thread
From: Peter Rosin @ 2022-05-13 22:48 UTC (permalink / raw)
  To: Jonathan Cameron, Eddie James
  Cc: linux-iio, devicetree, linux-kernel, krzysztof.kozlowski+dt,
	robh+dt, lars, jic23, miltonm, linux-i2c

Hi!

2022-05-13 at 18:45, Jonathan Cameron wrote:
> On Thu, 12 May 2022 12:08:07 -0500
> Eddie James <eajames@linux.ibm.com> wrote:
> 
>> On 5/12/22 11:48, Jonathan Cameron wrote:
>>> On Thu, 12 May 2022 11:20:18 -0500
>>> Eddie James <eajames@linux.ibm.com> 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.  
>>
>>
>> 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!)

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

> 
> Jonathan
> 
> 
> 
> 
>>
>> Thanks,
>>
>> Eddie
>>
>>
>>>
>>> Jonathan
>>>  
>>>> 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
>>>>  
> 

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 0/2] iio: humidity: si7020: Check device property for skipping reset in probe
  2022-05-12 19:11   ` Eddie James
@ 2022-05-14 13:40     ` Jonathan Cameron
  0 siblings, 0 replies; 17+ messages in thread
From: Jonathan Cameron @ 2022-05-14 13:40 UTC (permalink / raw)
  To: Eddie James
  Cc: Jonathan Cameron, linux-iio, devicetree, linux-kernel,
	krzysztof.kozlowski+dt, robh+dt, lars, miltonm

On Thu, 12 May 2022 14:11:06 -0500
Eddie James <eajames@linux.ibm.com> wrote:

> On 5/12/22 11:48, Jonathan Cameron wrote:
> > On Thu, 12 May 2022 11:20:18 -0500
> > Eddie James <eajames@linux.ibm.com> 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.  
> 
> 
> 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


> 
> Eddie
> 
> 
> >
> > Jonathan
> >  
> >> 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
> >>  


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 0/2] iio: humidity: si7020: Check device property for skipping reset in probe
  2022-05-13 22:48       ` Peter Rosin
@ 2022-05-14 13:43         ` Jonathan Cameron
  2022-05-14 15:02           ` Peter Rosin
  0 siblings, 1 reply; 17+ messages in thread
From: Jonathan Cameron @ 2022-05-14 13:43 UTC (permalink / raw)
  To: Peter Rosin, devicetree
  Cc: Jonathan Cameron, Eddie James, linux-iio, linux-kernel,
	krzysztof.kozlowski+dt, robh+dt, lars, miltonm, linux-i2c

On Sat, 14 May 2022 00:48:51 +0200
Peter Rosin <peda@axentia.se> wrote:

> Hi!
> 
> 2022-05-13 at 18:45, Jonathan Cameron wrote:
> > On Thu, 12 May 2022 12:08:07 -0500
> > Eddie James <eajames@linux.ibm.com> wrote:
> >   
> >> On 5/12/22 11:48, Jonathan Cameron wrote:  
> >>> On Thu, 12 May 2022 11:20:18 -0500
> >>> Eddie James <eajames@linux.ibm.com> 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.    
> >>
> >>
> >> 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!)  
> 
> 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.

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

> 
> Cheers,
> Peter
> 
> > 
> > Jonathan
> > 
> > 
> > 
> >   
> >>
> >> Thanks,
> >>
> >> Eddie
> >>
> >>  
> >>>
> >>> Jonathan
> >>>    
> >>>> 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
> >>>>    
> >   


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 0/2] iio: humidity: si7020: Check device property for skipping reset in probe
  2022-05-14 13:43         ` Jonathan Cameron
@ 2022-05-14 15:02           ` Peter Rosin
  2022-05-18 15:28             ` Eddie James
  0 siblings, 1 reply; 17+ messages in thread
From: Peter Rosin @ 2022-05-14 15:02 UTC (permalink / raw)
  To: Jonathan Cameron, devicetree
  Cc: Jonathan Cameron, Eddie James, linux-iio, linux-kernel,
	krzysztof.kozlowski+dt, robh+dt, lars, miltonm, linux-i2c

2022-05-14 at 15:43, Jonathan Cameron wrote:
> On Sat, 14 May 2022 00:48:51 +0200
> Peter Rosin <peda@axentia.se> wrote:
> 
>> Hi!
>>
>> 2022-05-13 at 18:45, Jonathan Cameron wrote:
>>> On Thu, 12 May 2022 12:08:07 -0500
>>> Eddie James <eajames@linux.ibm.com> wrote:
>>>   
>>>> On 5/12/22 11:48, Jonathan Cameron wrote:  
>>>>> On Thu, 12 May 2022 11:20:18 -0500
>>>>> Eddie James <eajames@linux.ibm.com> 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.    
>>>>
>>>>
>>>> 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!)  
>>
>> 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.
> 
> 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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 0/2] iio: humidity: si7020: Check device property for skipping reset in probe
  2022-05-14 15:02           ` Peter Rosin
@ 2022-05-18 15:28             ` Eddie James
  2022-05-18 15:51               ` Eddie James
  0 siblings, 1 reply; 17+ messages in thread
From: Eddie James @ 2022-05-18 15:28 UTC (permalink / raw)
  To: Peter Rosin, Jonathan Cameron, devicetree
  Cc: Jonathan Cameron, linux-iio, linux-kernel,
	krzysztof.kozlowski+dt, robh+dt, lars, miltonm, linux-i2c


On 5/14/22 10:02, Peter Rosin wrote:
> 2022-05-14 at 15:43, Jonathan Cameron wrote:
>> On Sat, 14 May 2022 00:48:51 +0200
>> Peter Rosin <peda@axentia.se> wrote:
>>
>>> Hi!
>>>
>>> 2022-05-13 at 18:45, Jonathan Cameron wrote:
>>>> On Thu, 12 May 2022 12:08:07 -0500
>>>> Eddie James <eajames@linux.ibm.com> wrote:
>>>>    
>>>>> On 5/12/22 11:48, Jonathan Cameron wrote:
>>>>>> On Thu, 12 May 2022 11:20:18 -0500
>>>>>> Eddie James <eajames@linux.ibm.com> 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.
>>>>>
>>>>> 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!)
>>> 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.
>> 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.


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



>
> Me waves hand a bit...
>
> Cheers,
> Peter

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 0/2] iio: humidity: si7020: Check device property for skipping reset in probe
  2022-05-18 15:28             ` Eddie James
@ 2022-05-18 15:51               ` Eddie James
  0 siblings, 0 replies; 17+ messages in thread
From: Eddie James @ 2022-05-18 15:51 UTC (permalink / raw)
  To: Peter Rosin, Jonathan Cameron, devicetree
  Cc: Jonathan Cameron, linux-iio, linux-kernel,
	krzysztof.kozlowski+dt, robh+dt, lars, miltonm, linux-i2c


On 5/18/22 10:28, Eddie James wrote:
>
> On 5/14/22 10:02, Peter Rosin wrote:
>> 2022-05-14 at 15:43, Jonathan Cameron wrote:
>>> On Sat, 14 May 2022 00:48:51 +0200
>>> Peter Rosin <peda@axentia.se> wrote:
>>>
>>>> Hi!
>>>>
>>>> 2022-05-13 at 18:45, Jonathan Cameron wrote:
>>>>> On Thu, 12 May 2022 12:08:07 -0500
>>>>> Eddie James <eajames@linux.ibm.com> wrote:
>>>>>> On 5/12/22 11:48, Jonathan Cameron wrote:
>>>>>>> On Thu, 12 May 2022 11:20:18 -0500
>>>>>>> Eddie James <eajames@linux.ibm.com> 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.
>>>>>>
>>>>>> 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!)
>>>> 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.
>>> 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.
>
>
> 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?


Um, nevermind... that would require changes in all the bus drivers. I'll 
look into implementing the hook/callback.

Thanks,

Eddie


>
>
> Thanks,
>
> Eddie
>
>
>
>>
>> Me waves hand a bit...
>>
>> Cheers,
>> Peter
>
>

^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2022-05-18 15:52 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-12 16:20 [PATCH v2 0/2] iio: humidity: si7020: Check device property for skipping reset in probe Eddie James
2022-05-12 16:20 ` [PATCH v2 1/2] dt-bindings: iio: humidity: Add si7020 bindings Eddie James
2022-05-12 16:51   ` Jonathan Cameron
2022-05-12 17:08     ` Eddie James
2022-05-13 16:47       ` Jonathan Cameron
2022-05-13  8:55   ` Krzysztof Kozlowski
2022-05-12 16:20 ` [PATCH v2 2/2] iio: humidity: si7020: Check device property for skipping reset in probe Eddie James
2022-05-12 16:48 ` [PATCH v2 0/2] " Jonathan Cameron
2022-05-12 17:08   ` Eddie James
2022-05-13 16:45     ` Jonathan Cameron
2022-05-13 22:48       ` Peter Rosin
2022-05-14 13:43         ` Jonathan Cameron
2022-05-14 15:02           ` Peter Rosin
2022-05-18 15:28             ` Eddie James
2022-05-18 15:51               ` Eddie James
2022-05-12 19:11   ` Eddie James
2022-05-14 13:40     ` Jonathan Cameron

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.