[PATCH v2 2/2] hwmon: (sht4x) Add device tree match table and document it

[PATCH v2 2/2] hwmon: (sht4x) Add device tree match table and document it

[David Mosberger-Tang]

This patch enables automatic loading of the sht4x module via a device
tree table entry.

Signed-off-by: David Mosberger-Tang <davidm@egauge.net>
---
 .../bindings/hwmon/sensirion,sht4x.yaml       | 50 +++++++++++++++++++
 drivers/hwmon/sht4x.c                         |  7 +++
 2 files changed, 57 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwmon/sensirion,sht4x.yaml

diff --git a/Documentation/devicetree/bindings/hwmon/sensirion,sht4x.yaml b/Documentation/devicetree/bindings/hwmon/sensirion,sht4x.yaml
new file mode 100644
index 000000000000..588c2e37b035
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/sensirion,sht4x.yaml
@@ -0,0 +1,50 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/hwmon/sensirion,sht4x.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Sensirion SHT4x Humidity and Temperature Sensor IC
+
+maintainers:
+  - Navin Sankar Velliangiri navin@linumiz.com
+
+description: |
+  The SHT4x is a high-accuracy digital humidity and temperature sensor
+  designed especially for battery-driven high-volume consumer
+  electronics applications.  For further information refere to
+  Documentation/hwmon/sht4x.rst
+
+  This binding document describes the binding for the hardware monitor
+  portion of the driver.
+
+properties:
+  compatible:
+    enum:
+      - sensirion,sht4x
+
+  reg:
+    const: 0x44
+    description:
+      The I2c base address of the SHT4x.  0x44 for all chip versions
+      except for SHT41-BD1B, where it is 0x45.
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    i2c {
+      #address-cells = <1>;
+      #size-cells = <0>;
+      clock-frequency = <400000>;
+
+      sht4x@44 {
+        compatible = "sensirion,sht4x";
+        reg = <0x44>;
+      };
+    };
+...
diff --git a/drivers/hwmon/sht4x.c b/drivers/hwmon/sht4x.c
index 3415d7a0e0fc..6e53d81e32d4 100644
--- a/drivers/hwmon/sht4x.c
+++ b/drivers/hwmon/sht4x.c
@@ -281,9 +281,16 @@ static const struct i2c_device_id sht4x_id[] = {
 };
 MODULE_DEVICE_TABLE(i2c, sht4x_id);
 
+static const struct of_device_id sht4x_of_match[] = {
+	{ .compatible = "sensirion,sht4x" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, sht4x_of_match);
+
 static struct i2c_driver sht4x_driver = {
 	.driver = {
 		.name = "sht4x",
+		.of_match_table = sht4x_of_match
 	},
 	.probe		= sht4x_probe,
 	.id_table	= sht4x_id,
-- 
2.25.1

[PATCH v2 1/2] hwmon: (sht4x) Fix EREMOTEIO errors

[David Mosberger-Tang]

Per datasheet, SHT4x may need up to 8.2ms for a "high repeatability"
measurement to complete.  Attempting to read the result too early
triggers a NAK which then causes an EREMOTEIO error.

This behavior has been confirmed with a logic analyzer while running
the I2C bus at only 40kHz.  The low frequency precludes any
signal-integrity issues, which was also confirmed by the absence of
any CRC8 errors.  In this configuration, a NAK occurred on any read
that followed the measurement command within less than 8.2ms.

Signed-off-by: David Mosberger-Tang <davidm@egauge.net>
---
 drivers/hwmon/sht4x.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/hwmon/sht4x.c b/drivers/hwmon/sht4x.c
index 09c2a0b06444..3415d7a0e0fc 100644
--- a/drivers/hwmon/sht4x.c
+++ b/drivers/hwmon/sht4x.c
@@ -23,7 +23,7 @@
 /*
  * I2C command delays (in microseconds)
  */
-#define SHT4X_MEAS_DELAY	1000
+#define SHT4X_MEAS_DELAY_HPM	8200	/* see t_MEAS,h in datasheet */
 #define SHT4X_DELAY_EXTRA	10000
 
 /*
@@ -90,7 +90,7 @@ static int sht4x_read_values(struct sht4x_data *data)
 	if (ret < 0)
 		goto unlock;
 
-	usleep_range(SHT4X_MEAS_DELAY, SHT4X_MEAS_DELAY + SHT4X_DELAY_EXTRA);
+	usleep_range(SHT4X_MEAS_DELAY_HPM, SHT4X_MEAS_DELAY_HPM + SHT4X_DELAY_EXTRA);
 
 	ret = i2c_master_recv(client, raw_data, SHT4X_RESPONSE_LENGTH);
 	if (ret != SHT4X_RESPONSE_LENGTH) {
-- 
2.25.1

Re: [PATCH v2 1/2] hwmon: (sht4x) Fix EREMOTEIO errors

[Guenter Roeck]

On 11/20/21 12:40 PM, David Mosberger-Tang wrote:
> Per datasheet, SHT4x may need up to 8.2ms for a "high repeatability"
> measurement to complete.  Attempting to read the result too early
> triggers a NAK which then causes an EREMOTEIO error.
> 
> This behavior has been confirmed with a logic analyzer while running
> the I2C bus at only 40kHz.  The low frequency precludes any
> signal-integrity issues, which was also confirmed by the absence of
> any CRC8 errors.  In this configuration, a NAK occurred on any read
> that followed the measurement command within less than 8.2ms.
> 
> Signed-off-by: David Mosberger-Tang <davidm@egauge.net>

You sent v2 after 5 minutes, and there is no change log.
Please never do that, and always provide a change log.

Guenter

> ---
>   drivers/hwmon/sht4x.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/hwmon/sht4x.c b/drivers/hwmon/sht4x.c
> index 09c2a0b06444..3415d7a0e0fc 100644
> --- a/drivers/hwmon/sht4x.c
> +++ b/drivers/hwmon/sht4x.c
> @@ -23,7 +23,7 @@
>   /*
>    * I2C command delays (in microseconds)
>    */
> -#define SHT4X_MEAS_DELAY	1000
> +#define SHT4X_MEAS_DELAY_HPM	8200	/* see t_MEAS,h in datasheet */
>   #define SHT4X_DELAY_EXTRA	10000
>   
>   /*
> @@ -90,7 +90,7 @@ static int sht4x_read_values(struct sht4x_data *data)
>   	if (ret < 0)
>   		goto unlock;
>   
> -	usleep_range(SHT4X_MEAS_DELAY, SHT4X_MEAS_DELAY + SHT4X_DELAY_EXTRA);
> +	usleep_range(SHT4X_MEAS_DELAY_HPM, SHT4X_MEAS_DELAY_HPM + SHT4X_DELAY_EXTRA);
>   
>   	ret = i2c_master_recv(client, raw_data, SHT4X_RESPONSE_LENGTH);
>   	if (ret != SHT4X_RESPONSE_LENGTH) {
> 

Re: [PATCH v2 2/2] hwmon: (sht4x) Add device tree match table and document it

[Guenter Roeck]

On 11/20/21 12:40 PM, David Mosberger-Tang wrote:
> This patch enables automatic loading of the sht4x module via a device
> tree table entry.
> 
> Signed-off-by: David Mosberger-Tang <davidm@egauge.net>
> ---
>   .../bindings/hwmon/sensirion,sht4x.yaml       | 50 +++++++++++++++++++
>   drivers/hwmon/sht4x.c                         |  7 +++

This needs to be two separate patches, one the devicetree change and the other
the source code change. DT maintainers need to be copied on the devicetree
patch.

Also, please consider adding the device to trivial-devices.yaml instead since
there are no special bindings.

Thanks,
Guenter

>   2 files changed, 57 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/hwmon/sensirion,sht4x.yaml
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/sensirion,sht4x.yaml b/Documentation/devicetree/bindings/hwmon/sensirion,sht4x.yaml
> new file mode 100644
> index 000000000000..588c2e37b035
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/sensirion,sht4x.yaml
> @@ -0,0 +1,50 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/hwmon/sensirion,sht4x.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Sensirion SHT4x Humidity and Temperature Sensor IC
> +
> +maintainers:
> +  - Navin Sankar Velliangiri navin@linumiz.com
> +
> +description: |
> +  The SHT4x is a high-accuracy digital humidity and temperature sensor
> +  designed especially for battery-driven high-volume consumer
> +  electronics applications.  For further information refere to
> +  Documentation/hwmon/sht4x.rst
> +
> +  This binding document describes the binding for the hardware monitor
> +  portion of the driver.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - sensirion,sht4x
> +
> +  reg:
> +    const: 0x44
> +    description:
> +      The I2c base address of the SHT4x.  0x44 for all chip versions
> +      except for SHT41-BD1B, where it is 0x45.
> +
> +required:
> +  - compatible
> +  - reg
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    i2c {
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +      clock-frequency = <400000>;
> +
> +      sht4x@44 {
> +        compatible = "sensirion,sht4x";
> +        reg = <0x44>;
> +      };
> +    };
> +...
> diff --git a/drivers/hwmon/sht4x.c b/drivers/hwmon/sht4x.c
> index 3415d7a0e0fc..6e53d81e32d4 100644
> --- a/drivers/hwmon/sht4x.c
> +++ b/drivers/hwmon/sht4x.c
> @@ -281,9 +281,16 @@ static const struct i2c_device_id sht4x_id[] = {
>   };
>   MODULE_DEVICE_TABLE(i2c, sht4x_id);
>   
> +static const struct of_device_id sht4x_of_match[] = {
> +	{ .compatible = "sensirion,sht4x" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, sht4x_of_match);
> +
>   static struct i2c_driver sht4x_driver = {
>   	.driver = {
>   		.name = "sht4x",
> +		.of_match_table = sht4x_of_match
>   	},
>   	.probe		= sht4x_probe,
>   	.id_table	= sht4x_id,
> 

[PATCH v3] hwmon: (sht4x) Fix EREMOTEIO errors

[David Mosberger-Tang]

Repeat of previous patch but split into separate emails so the
device-tree bits can be sent to the proper group of maintainers, as
per Guenter's suggestion.

[PATCH v3] hwmon: (sht4x) Fix EREMOTEIO errors

[David Mosberger-Tang]

Per datasheet, SHT4x may need up to 8.2ms for a "high repeatability"
measurement to complete.  Attempting to read the result too early
triggers a NAK which then causes an EREMOTEIO error.

This behavior has been confirmed with a logic analyzer while running
the I2C bus at only 40kHz.  The low frequency precludes any
signal-integrity issues, which was also confirmed by the absence of
any CRC8 errors.  In this configuration, a NAK occurred on any read
that followed the measurement command within less than 8.2ms.

Signed-off-by: David Mosberger-Tang <davidm@egauge.net>
---
 drivers/hwmon/sht4x.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/hwmon/sht4x.c b/drivers/hwmon/sht4x.c
index 09c2a0b06444..3415d7a0e0fc 100644
--- a/drivers/hwmon/sht4x.c
+++ b/drivers/hwmon/sht4x.c
@@ -23,7 +23,7 @@
 /*
  * I2C command delays (in microseconds)
  */
-#define SHT4X_MEAS_DELAY	1000
+#define SHT4X_MEAS_DELAY_HPM	8200	/* see t_MEAS,h in datasheet */
 #define SHT4X_DELAY_EXTRA	10000
 
 /*
@@ -90,7 +90,7 @@ static int sht4x_read_values(struct sht4x_data *data)
 	if (ret < 0)
 		goto unlock;
 
-	usleep_range(SHT4X_MEAS_DELAY, SHT4X_MEAS_DELAY + SHT4X_DELAY_EXTRA);
+	usleep_range(SHT4X_MEAS_DELAY_HPM, SHT4X_MEAS_DELAY_HPM + SHT4X_DELAY_EXTRA);
 
 	ret = i2c_master_recv(client, raw_data, SHT4X_RESPONSE_LENGTH);
 	if (ret != SHT4X_RESPONSE_LENGTH) {
-- 
2.25.1

Re: [PATCH v2 2/2] hwmon: (sht4x) Add device tree match table and document it

[David Mosberger-Tang]

On Sat, 2021-11-20 at 13:12 -0800, Guenter Roeck wrote:
> On 11/20/21 12:40 PM, David Mosberger-Tang wrote:
> > This patch enables automatic loading of the sht4x module via a device
> > tree table entry.
> > 
> > Signed-off-by: David Mosberger-Tang <davidm@egauge.net>
> > ---
> >   .../bindings/hwmon/sensirion,sht4x.yaml       | 50 +++++++++++++++++++
> >   drivers/hwmon/sht4x.c                         |  7 +++
> 
> This needs to be two separate patches, one the devicetree change and the other
> the source code change. DT maintainers need to be copied on the devicetree
> patch.

Isn't that going to be confusing if one but not the other patch makes
it into a repository?  Either you end up with an undocumented device
tree property or you end up with documentation for an unsupported
property.

> Also, please consider adding the device to trivial-devices.yaml instead since
> there are no special bindings.

I didn't know it existed.  Sure, that's much easier.

  --david

> Thanks,
> Guenter
> 
> >   2 files changed, 57 insertions(+)
> >   create mode 100644 Documentation/devicetree/bindings/hwmon/sensirion,sht4x.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/hwmon/sensirion,sht4x.yaml b/Documentation/devicetree/bindings/hwmon/sensirion,sht4x.yaml
> > new file mode 100644
> > index 000000000000..588c2e37b035
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/hwmon/sensirion,sht4x.yaml
> > @@ -0,0 +1,50 @@
> > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/hwmon/sensirion,sht4x.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Sensirion SHT4x Humidity and Temperature Sensor IC
> > +
> > +maintainers:
> > +  - Navin Sankar Velliangiri navin@linumiz.com
> > +
> > +description: |
> > +  The SHT4x is a high-accuracy digital humidity and temperature sensor
> > +  designed especially for battery-driven high-volume consumer
> > +  electronics applications.  For further information refere to
> > +  Documentation/hwmon/sht4x.rst
> > +
> > +  This binding document describes the binding for the hardware monitor
> > +  portion of the driver.
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - sensirion,sht4x
> > +
> > +  reg:
> > +    const: 0x44
> > +    description:
> > +      The I2c base address of the SHT4x.  0x44 for all chip versions
> > +      except for SHT41-BD1B, where it is 0x45.
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    i2c {
> > +      #address-cells = <1>;
> > +      #size-cells = <0>;
> > +      clock-frequency = <400000>;
> > +
> > +      sht4x@44 {
> > +        compatible = "sensirion,sht4x";
> > +        reg = <0x44>;
> > +      };
> > +    };
> > +...
> > diff --git a/drivers/hwmon/sht4x.c b/drivers/hwmon/sht4x.c
> > index 3415d7a0e0fc..6e53d81e32d4 100644
> > --- a/drivers/hwmon/sht4x.c
> > +++ b/drivers/hwmon/sht4x.c
> > @@ -281,9 +281,16 @@ static const struct i2c_device_id sht4x_id[] = {
> >   };
> >   MODULE_DEVICE_TABLE(i2c, sht4x_id);
> >   
> > +static const struct of_device_id sht4x_of_match[] = {
> > +	{ .compatible = "sensirion,sht4x" },
> > +	{ }
> > +};
> > +MODULE_DEVICE_TABLE(of, sht4x_of_match);
> > +
> >   static struct i2c_driver sht4x_driver = {
> >   	.driver = {
> >   		.name = "sht4x",
> > +		.of_match_table = sht4x_of_match
> >   	},
> >   	.probe		= sht4x_probe,
> >   	.id_table	= sht4x_id,
> > 

Re: [PATCH v2 2/2] hwmon: (sht4x) Add device tree match table and document it

[Guenter Roeck]

On Sat, Nov 20, 2021 at 02:36:39PM -0700, David Mosberger-Tang wrote:
> On Sat, 2021-11-20 at 13:12 -0800, Guenter Roeck wrote:
> > On 11/20/21 12:40 PM, David Mosberger-Tang wrote:
> > > This patch enables automatic loading of the sht4x module via a device
> > > tree table entry.
> > > 
> > > Signed-off-by: David Mosberger-Tang <davidm@egauge.net>
> > > ---
> > >   .../bindings/hwmon/sensirion,sht4x.yaml       | 50 +++++++++++++++++++
> > >   drivers/hwmon/sht4x.c                         |  7 +++
> > 
> > This needs to be two separate patches, one the devicetree change and the other
> > the source code change. DT maintainers need to be copied on the devicetree
> > patch.
> 
> Isn't that going to be confusing if one but not the other patch makes
> it into a repository?  Either you end up with an undocumented device
> tree property or you end up with documentation for an unsupported
> property.
> 
This is a trivial device, so that isn't really an issue. Otherwise,
if there are real bindings to approve, I would not accept the patch
making the code change unless the devicetree patch is approved,
and I would typically apply both together.

Anyway, those are the rules. Devicetree patches need to be sent
separately and approved by a devicetree maintainer. We should not
[have to] discuss rules here. If you are unhappy with it, I would
suggest to start a discussion on the devicetree mailing list and
suggest alternatives.

Thanks,
Guenter

Re: [PATCH v3] hwmon: (sht4x) Fix EREMOTEIO errors

[Guenter Roeck]

On Sat, Nov 20, 2021 at 09:28:53PM +0000, David Mosberger-Tang wrote:
> Repeat of previous patch but split into separate emails so the
> device-tree bits can be sent to the proper group of maintainers, as
> per Guenter's suggestion.
> 

Another rule: Never send a follow-up patch series as reply to a
previous one; otherwise it might get lost.

Also, if you send a single patch, there is not need for an
introduction patch like this one.

Thanks,
Guenter

Re: [PATCH v3] hwmon: (sht4x) Fix EREMOTEIO errors

[Guenter Roeck]

On Sat, Nov 20, 2021 at 09:28:56PM +0000, David Mosberger-Tang wrote:
> Per datasheet, SHT4x may need up to 8.2ms for a "high repeatability"
> measurement to complete.  Attempting to read the result too early
> triggers a NAK which then causes an EREMOTEIO error.
> 
> This behavior has been confirmed with a logic analyzer while running
> the I2C bus at only 40kHz.  The low frequency precludes any
> signal-integrity issues, which was also confirmed by the absence of
> any CRC8 errors.  In this configuration, a NAK occurred on any read
> that followed the measurement command within less than 8.2ms.
> 
> Signed-off-by: David Mosberger-Tang <davidm@egauge.net>

Applied.

Thanks,
Guenter

> ---
>  drivers/hwmon/sht4x.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/hwmon/sht4x.c b/drivers/hwmon/sht4x.c
> index 09c2a0b06444..3415d7a0e0fc 100644
> --- a/drivers/hwmon/sht4x.c
> +++ b/drivers/hwmon/sht4x.c
> @@ -23,7 +23,7 @@
>  /*
>   * I2C command delays (in microseconds)
>   */
> -#define SHT4X_MEAS_DELAY	1000
> +#define SHT4X_MEAS_DELAY_HPM	8200	/* see t_MEAS,h in datasheet */
>  #define SHT4X_DELAY_EXTRA	10000
>  
>  /*
> @@ -90,7 +90,7 @@ static int sht4x_read_values(struct sht4x_data *data)
>  	if (ret < 0)
>  		goto unlock;
>  
> -	usleep_range(SHT4X_MEAS_DELAY, SHT4X_MEAS_DELAY + SHT4X_DELAY_EXTRA);
> +	usleep_range(SHT4X_MEAS_DELAY_HPM, SHT4X_MEAS_DELAY_HPM + SHT4X_DELAY_EXTRA);
>  
>  	ret = i2c_master_recv(client, raw_data, SHT4X_RESPONSE_LENGTH);
>  	if (ret != SHT4X_RESPONSE_LENGTH) {

Re: [PATCH v2 2/2] hwmon: (sht4x) Add device tree match table and document it

[David Mosberger-Tang]

On Sat, 2021-11-20 at 13:51 -0800, Guenter Roeck wrote:
> On Sat, Nov 20, 2021 at 02:36:39PM -0700, David Mosberger-Tang wrote:
> > On Sat, 2021-11-20 at 13:12 -0800, Guenter Roeck wrote:
> > > On 11/20/21 12:40 PM, David Mosberger-Tang wrote:
> > > > This patch enables automatic loading of the sht4x module via a device
> > > > tree table entry.
> > > > 
> > > > Signed-off-by: David Mosberger-Tang <davidm@egauge.net>
> > > > ---
> > > >   .../bindings/hwmon/sensirion,sht4x.yaml       | 50 +++++++++++++++++++
> > > >   drivers/hwmon/sht4x.c                         |  7 +++
> > > 
> > > This needs to be two separate patches, one the devicetree change and the other
> > > the source code change. DT maintainers need to be copied on the devicetree
> > > patch.
> > 
> > Isn't that going to be confusing if one but not the other patch makes
> > it into a repository?  Either you end up with an undocumented device
> > tree property or you end up with documentation for an unsupported
> > property.
> > 
> This is a trivial device, so that isn't really an issue. Otherwise,
> if there are real bindings to approve, I would not accept the patch
> making the code change unless the devicetree patch is approved,
> and I would typically apply both together.

Got it, thanks for the explanation.

  --david