[PATCH v2 0/2] hwmon: sht3x: improve docs, read serial number

[PATCH v2 0/2] hwmon: sht3x: improve docs, read serial number

[Stefan Gloor]

The temperature/humidity sensors of the STS3x/SHT3x family are
calibrated and factory-programmed with a unique serial number.
For some sensors, this serial number can be used to obtain a calibration
certificate via an API provided by the manufacturer (Sensirion).
Expose the serial number via debugfs.

Documentation is missing information about the sts3x support, add it.

Changelog
=========

v1 -> v2:
	- Change from sysfs to debugfs
	- Add documentation improvements

In v1 I stated that the serial number readout was not
documented for the whole SHT3x/STS3x series. I found that there is a
separate document for SHT3x documenting this feature.
For STS3x, the manufacturer confirmed with me that both families work
identically. Therefore, it is not needed to introduce more IDs than "sts3x"
and "sht3x", as they don't have different functionality.

Stefan Gloor (2):
  hwmon: sht3x: add sts3x support
  hwmon: sht3x: read out sensor serial number

 Documentation/hwmon/sht3x.rst | 40 ++++++++++++++++++++-----
 drivers/hwmon/sht3x.c         | 55 +++++++++++++++++++++++++++++++++++
 2 files changed, 87 insertions(+), 8 deletions(-)

-- 
2.41.0

[PATCH v2 1/2] hwmon: sht3x: add sts3x support

[Stefan Gloor]

Add information regarding the existing support for sts3x series and
update the datasheet links.

Signed-off-by: Stefan Gloor <code@stefan-gloor.ch>
---
 Documentation/hwmon/sht3x.rst | 29 +++++++++++++++++++++--------
 1 file changed, 21 insertions(+), 8 deletions(-)

diff --git a/Documentation/hwmon/sht3x.rst b/Documentation/hwmon/sht3x.rst
index 87864ffd1777..957c854f5d08 100644
--- a/Documentation/hwmon/sht3x.rst
+++ b/Documentation/hwmon/sht3x.rst
@@ -9,7 +9,19 @@ Supported chips:
 
     Addresses scanned: none
 
-    Datasheet: https://www.sensirion.com/file/datasheet_sht3x_digital
+    Datasheets:
+        - https://sensirion.com/media/documents/213E6A3B/63A5A569/Datasheet_SHT3x_DIS.pdf
+        - https://sensirion.com/media/documents/051DF50B/639C8101/Sensirion_Humidity_and_Temperature_Sensors_Datasheet_SHT33.pdf
+
+  * Sensirion STS3x-DIS
+
+    Prefix: 'sts3x'
+
+    Addresses scanned: none
+
+    Datasheets:
+        - https://sensirion.com/media/documents/1DA31AFD/61641F76/Sensirion_Temperature_Sensors_STS3x_Datasheet.pdf
+        - https://sensirion.com/media/documents/292A335C/65537BAF/Sensirion_Datasheet_STS32_STS33.pdf
 
 Author:
 
@@ -19,16 +31,17 @@ Author:
 Description
 -----------
 
-This driver implements support for the Sensirion SHT3x-DIS chip, a humidity
-and temperature sensor. Temperature is measured in degrees celsius, relative
-humidity is expressed as a percentage. In the sysfs interface, all values are
-scaled by 1000, i.e. the value for 31.5 degrees celsius is 31500.
+This driver implements support for the Sensirion SHT3x-DIS and STS3x-DIS
+series of humidity and temperature sensors. Temperature is measured in degrees
+celsius, relative humidity is expressed as a percentage. In the sysfs interface,
+all values are scaled by 1000, i.e. the value for 31.5 degrees celsius is 31500.
 
 The device communicates with the I2C protocol. Sensors can have the I2C
-addresses 0x44 or 0x45, depending on the wiring. See
-Documentation/i2c/instantiating-devices.rst for methods to instantiate the device.
+addresses 0x44 or 0x45 (0x4a or 0x4b for sts3x), depending on the wiring. See
+Documentation/i2c/instantiating-devices.rst for methods to instantiate the
+device.
 
-Even if sht3x sensor supports clock-strech(blocking mode) and non-strench
+Even if sht3x sensor supports clock-stretch (blocking mode) and non-stretch
 (non-blocking mode) in single-shot mode, this driver only supports the latter.
 
 The sht3x sensor supports a single shot mode as well as 5 periodic measure
-- 
2.41.0

Re: [PATCH v2 1/2] hwmon: sht3x: add sts3x support

[Guenter Roeck]

On Mon, Dec 04, 2023 at 05:50:03PM +0100, Stefan Gloor wrote:
> Add information regarding the existing support for sts3x series and
> update the datasheet links.
> 
> Signed-off-by: Stefan Gloor <code@stefan-gloor.ch>

Applied.

Thanks,
Guenter

[PATCH v2 2/2] hwmon: sht3x: read out sensor serial number

[Stefan Gloor]

The temperature/humidity sensors of the STS3x/SHT3x family are
calibrated and factory-programmed with a unique serial number.
For some sensors, this serial number can be used to obtain a calibration
certificate via an API provided by the manufacturer (Sensirion).
Expose the serial number via debugfs.

Tested with: 2x STS31, 1x STS32, 1x SHT31

Signed-off-by: Stefan Gloor <code@stefan-gloor.ch>
---
 Documentation/hwmon/sht3x.rst | 11 +++++++
 drivers/hwmon/sht3x.c         | 55 +++++++++++++++++++++++++++++++++++
 2 files changed, 66 insertions(+)

diff --git a/Documentation/hwmon/sht3x.rst b/Documentation/hwmon/sht3x.rst
index 957c854f5d08..9585fa7c5a5d 100644
--- a/Documentation/hwmon/sht3x.rst
+++ b/Documentation/hwmon/sht3x.rst
@@ -65,6 +65,10 @@ When the temperature and humidity readings move back between the hysteresis
 values, the alert bit is set to 0 and the alert pin on the sensor is set to
 low.
 
+The serial number exposed to debugfs allows for unique identification of the
+sensors. For sts32, sts33 and sht33, the manufacturer provides calibration
+certificates through an API.
+
 sysfs-Interface
 ---------------
 
@@ -99,3 +103,10 @@ repeatability:      write or read repeatability, higher repeatability means
                         - 1: medium repeatability
                         - 2: high repeatability
 =================== ============================================================
+
+debugfs-Interface
+-----------------
+
+=================== ============================================================
+serial_number:      unique serial number of the sensor in decimal
+=================== ============================================================
diff --git a/drivers/hwmon/sht3x.c b/drivers/hwmon/sht3x.c
index 79657910b79e..537365c6213b 100644
--- a/drivers/hwmon/sht3x.c
+++ b/drivers/hwmon/sht3x.c
@@ -10,6 +10,7 @@
 
 #include <asm/page.h>
 #include <linux/crc8.h>
+#include <linux/debugfs.h>
 #include <linux/delay.h>
 #include <linux/err.h>
 #include <linux/hwmon.h>
@@ -41,6 +42,7 @@ static const unsigned char sht3x_cmd_heater_off[]              = { 0x30, 0x66 };
 /* other commands */
 static const unsigned char sht3x_cmd_read_status_reg[]         = { 0xf3, 0x2d };
 static const unsigned char sht3x_cmd_clear_status_reg[]        = { 0x30, 0x41 };
+static const unsigned char sht3x_cmd_read_serial_number[]      = { 0x37, 0x80 };
 
 /* delays for single-shot mode i2c commands, both in us */
 #define SHT3X_SINGLE_WAIT_TIME_HPM  15000
@@ -163,12 +165,14 @@ struct sht3x_data {
 	enum sht3x_chips chip_id;
 	struct mutex i2c_lock; /* lock for sending i2c commands */
 	struct mutex data_lock; /* lock for updating driver data */
+	struct dentry *debugfs;
 
 	u8 mode;
 	const unsigned char *command;
 	u32 wait_time;			/* in us*/
 	unsigned long last_update;	/* last update in periodic mode*/
 	enum sht3x_repeatability repeatability;
+	u32 serial_number;
 
 	/*
 	 * cached values for temperature and humidity and limits
@@ -831,6 +835,50 @@ static int sht3x_write(struct device *dev, enum hwmon_sensor_types type,
 	}
 }
 
+#ifdef CONFIG_DEBUG_FS
+
+static void sht3x_debugfs_init(struct sht3x_data *data)
+{
+	char name[32];
+	struct dentry *sensor_dir;
+
+	data->debugfs = debugfs_lookup("sht3x", NULL);
+	if (IS_ERR_OR_NULL(data->debugfs))
+		data->debugfs = debugfs_create_dir("sht3x", NULL);
+
+	snprintf(name, sizeof(name), "i2c%u-%02x",
+		 data->client->adapter->nr, data->client->addr);
+	sensor_dir = debugfs_create_dir(name, data->debugfs);
+	debugfs_create_u32("serial_number", 0444,
+			   sensor_dir, &data->serial_number);
+}
+
+#else
+
+static void sht3x_debugfs_init(struct sht3x_data *data)
+{
+}
+
+#endif
+
+static int sht3x_serial_number_read(struct sht3x_data *data)
+{
+	int ret;
+	char buffer[SHT3X_RESPONSE_LENGTH];
+	struct i2c_client *client = data->client;
+
+	ret = sht3x_read_from_command(client, data,
+				      sht3x_cmd_read_serial_number,
+				      buffer,
+				      SHT3X_RESPONSE_LENGTH, 0);
+	if (ret)
+		return ret;
+
+	data->serial_number = (buffer[0] << 24) | (buffer[1] << 16) |
+			      (buffer[3] << 8) | buffer[4];
+	return ret;
+}
+
 static const struct hwmon_ops sht3x_ops = {
 	.is_visible = sht3x_is_visible,
 	.read = sht3x_read,
@@ -899,6 +947,13 @@ static int sht3x_probe(struct i2c_client *client)
 	if (ret)
 		return ret;
 
+	ret = sht3x_serial_number_read(data);
+	if (ret) {
+		dev_dbg(dev, "unable to read serial number\n");
+		data->serial_number = 0;
+	}
+	sht3x_debugfs_init(data);
+
 	hwmon_dev = devm_hwmon_device_register_with_info(dev,
 							 client->name,
 							 data,
-- 
2.41.0

Re: [PATCH v2 2/2] hwmon: sht3x: read out sensor serial number

[Guenter Roeck]

On Mon, Dec 04, 2023 at 05:50:04PM +0100, Stefan Gloor wrote:
> The temperature/humidity sensors of the STS3x/SHT3x family are
> calibrated and factory-programmed with a unique serial number.
> For some sensors, this serial number can be used to obtain a calibration
> certificate via an API provided by the manufacturer (Sensirion).
> Expose the serial number via debugfs.
> 
> Tested with: 2x STS31, 1x STS32, 1x SHT31
> 
> Signed-off-by: Stefan Gloor <code@stefan-gloor.ch>
> ---
>  Documentation/hwmon/sht3x.rst | 11 +++++++
>  drivers/hwmon/sht3x.c         | 55 +++++++++++++++++++++++++++++++++++
>  2 files changed, 66 insertions(+)
> 
> diff --git a/Documentation/hwmon/sht3x.rst b/Documentation/hwmon/sht3x.rst
> index 957c854f5d08..9585fa7c5a5d 100644
> --- a/Documentation/hwmon/sht3x.rst
> +++ b/Documentation/hwmon/sht3x.rst
> @@ -65,6 +65,10 @@ When the temperature and humidity readings move back between the hysteresis
>  values, the alert bit is set to 0 and the alert pin on the sensor is set to
>  low.
>  
> +The serial number exposed to debugfs allows for unique identification of the
> +sensors. For sts32, sts33 and sht33, the manufacturer provides calibration
> +certificates through an API.
> +
>  sysfs-Interface
>  ---------------
>  
> @@ -99,3 +103,10 @@ repeatability:      write or read repeatability, higher repeatability means
>                          - 1: medium repeatability
>                          - 2: high repeatability
>  =================== ============================================================
> +
> +debugfs-Interface
> +-----------------
> +
> +=================== ============================================================
> +serial_number:      unique serial number of the sensor in decimal
> +=================== ============================================================
> diff --git a/drivers/hwmon/sht3x.c b/drivers/hwmon/sht3x.c
> index 79657910b79e..537365c6213b 100644
> --- a/drivers/hwmon/sht3x.c
> +++ b/drivers/hwmon/sht3x.c
> @@ -10,6 +10,7 @@
>  
>  #include <asm/page.h>
>  #include <linux/crc8.h>
> +#include <linux/debugfs.h>
>  #include <linux/delay.h>
>  #include <linux/err.h>
>  #include <linux/hwmon.h>
> @@ -41,6 +42,7 @@ static const unsigned char sht3x_cmd_heater_off[]              = { 0x30, 0x66 };
>  /* other commands */
>  static const unsigned char sht3x_cmd_read_status_reg[]         = { 0xf3, 0x2d };
>  static const unsigned char sht3x_cmd_clear_status_reg[]        = { 0x30, 0x41 };
> +static const unsigned char sht3x_cmd_read_serial_number[]      = { 0x37, 0x80 };
>  
>  /* delays for single-shot mode i2c commands, both in us */
>  #define SHT3X_SINGLE_WAIT_TIME_HPM  15000
> @@ -163,12 +165,14 @@ struct sht3x_data {
>  	enum sht3x_chips chip_id;
>  	struct mutex i2c_lock; /* lock for sending i2c commands */
>  	struct mutex data_lock; /* lock for updating driver data */
> +	struct dentry *debugfs;
>  
>  	u8 mode;
>  	const unsigned char *command;
>  	u32 wait_time;			/* in us*/
>  	unsigned long last_update;	/* last update in periodic mode*/
>  	enum sht3x_repeatability repeatability;
> +	u32 serial_number;
>  
>  	/*
>  	 * cached values for temperature and humidity and limits
> @@ -831,6 +835,50 @@ static int sht3x_write(struct device *dev, enum hwmon_sensor_types type,
>  	}
>  }
>  
> +#ifdef CONFIG_DEBUG_FS
> +
> +static void sht3x_debugfs_init(struct sht3x_data *data)
> +{
> +	char name[32];
> +	struct dentry *sensor_dir;
> +
> +	data->debugfs = debugfs_lookup("sht3x", NULL);
> +	if (IS_ERR_OR_NULL(data->debugfs))
> +		data->debugfs = debugfs_create_dir("sht3x", NULL);
> +
> +	snprintf(name, sizeof(name), "i2c%u-%02x",
> +		 data->client->adapter->nr, data->client->addr);
> +	sensor_dir = debugfs_create_dir(name, data->debugfs);
> +	debugfs_create_u32("serial_number", 0444,
> +			   sensor_dir, &data->serial_number);
> +}
> +
> +#else
> +
> +static void sht3x_debugfs_init(struct sht3x_data *data)
> +{
> +}
> +
> +#endif

debugfs doesn't need if/else or error handling.


> +
> +static int sht3x_serial_number_read(struct sht3x_data *data)
> +{
> +	int ret;
> +	char buffer[SHT3X_RESPONSE_LENGTH];
> +	struct i2c_client *client = data->client;
> +
> +	ret = sht3x_read_from_command(client, data,
> +				      sht3x_cmd_read_serial_number,
> +				      buffer,
> +				      SHT3X_RESPONSE_LENGTH, 0);
> +	if (ret)
> +		return ret;
> +
> +	data->serial_number = (buffer[0] << 24) | (buffer[1] << 16) |
> +			      (buffer[3] << 8) | buffer[4];
> +	return ret;
> +}
> +
>  static const struct hwmon_ops sht3x_ops = {
>  	.is_visible = sht3x_is_visible,
>  	.read = sht3x_read,
> @@ -899,6 +947,13 @@ static int sht3x_probe(struct i2c_client *client)
>  	if (ret)
>  		return ret;
>  
> +	ret = sht3x_serial_number_read(data);
> +	if (ret) {
> +		dev_dbg(dev, "unable to read serial number\n");
> +		data->serial_number = 0;
> +	}
> +	sht3x_debugfs_init(data);

The debugfs entry should not be created in the first place if the
serial number can not be read. On top of that, the debugfs entries
are never removed, meaning the system will crash if the driver or device
is unloaded and the no longer referenced debugfs file is accessed.

Guenter

> +
>  	hwmon_dev = devm_hwmon_device_register_with_info(dev,
>  							 client->name,
>  							 data,

Re: [PATCH v2 2/2] hwmon: sht3x: read out sensor serial number

[Stefan Gloor]

On Mon, Dec 11, 2023 at 06:31:27AM -0800, Guenter Roeck wrote:
> On Mon, Dec 04, 2023 at 05:50:04PM +0100, Stefan Gloor wrote:
> >  
> > +#ifdef CONFIG_DEBUG_FS
> > +
> > +static void sht3x_debugfs_init(struct sht3x_data *data)
> > +{
> > +	char name[32];
> > +	struct dentry *sensor_dir;
> > +
> > +	data->debugfs = debugfs_lookup("sht3x", NULL);
> > +	if (IS_ERR_OR_NULL(data->debugfs))
> > +		data->debugfs = debugfs_create_dir("sht3x", NULL);
> > +
> > +	snprintf(name, sizeof(name), "i2c%u-%02x",
> > +		 data->client->adapter->nr, data->client->addr);
> > +	sensor_dir = debugfs_create_dir(name, data->debugfs);
> > +	debugfs_create_u32("serial_number", 0444,
> > +			   sensor_dir, &data->serial_number);
> > +}
> > +
> > +#else
> > +
> > +static void sht3x_debugfs_init(struct sht3x_data *data)
> > +{
> > +}
> > +
> > +#endif
> 
> debugfs doesn't need if/else or error handling.
> 
Do you mean the IS_ERR_OR_NULL? I included that to get rid of the
"debugfs directory already exists" message when using multiple sensors.

Will remove #ifdef in V3.

> 
> > +
> > +static int sht3x_serial_number_read(struct sht3x_data *data)
> > +{
> > +	int ret;
> > +	char buffer[SHT3X_RESPONSE_LENGTH];
> > +	struct i2c_client *client = data->client;
> > +
> > +	ret = sht3x_read_from_command(client, data,
> > +				      sht3x_cmd_read_serial_number,
> > +				      buffer,
> > +				      SHT3X_RESPONSE_LENGTH, 0);
> > +	if (ret)
> > +		return ret;
> > +
> > +	data->serial_number = (buffer[0] << 24) | (buffer[1] << 16) |
> > +			      (buffer[3] << 8) | buffer[4];
> > +	return ret;
> > +}
> > +
> >  static const struct hwmon_ops sht3x_ops = {
> >  	.is_visible = sht3x_is_visible,
> >  	.read = sht3x_read,
> > @@ -899,6 +947,13 @@ static int sht3x_probe(struct i2c_client *client)
> >  	if (ret)
> >  		return ret;
> >  
> > +	ret = sht3x_serial_number_read(data);
> > +	if (ret) {
> > +		dev_dbg(dev, "unable to read serial number\n");
> > +		data->serial_number = 0;
> > +	}
> > +	sht3x_debugfs_init(data);
> 
> The debugfs entry should not be created in the first place if the
> serial number can not be read. On top of that, the debugfs entries
> are never removed, meaning the system will crash if the driver or device
> is unloaded and the no longer referenced debugfs file is accessed.

Got it, will fix in V3.
-- 

Re: [PATCH v2 2/2] hwmon: sht3x: read out sensor serial number

[Guenter Roeck]

On 12/11/23 07:12, Stefan Gloor wrote:
> On Mon, Dec 11, 2023 at 06:31:27AM -0800, Guenter Roeck wrote:
>> On Mon, Dec 04, 2023 at 05:50:04PM +0100, Stefan Gloor wrote:
>>>   
>>> +#ifdef CONFIG_DEBUG_FS
>>> +
>>> +static void sht3x_debugfs_init(struct sht3x_data *data)
>>> +{
>>> +	char name[32];
>>> +	struct dentry *sensor_dir;
>>> +
>>> +	data->debugfs = debugfs_lookup("sht3x", NULL);
>>> +	if (IS_ERR_OR_NULL(data->debugfs))
>>> +		data->debugfs = debugfs_create_dir("sht3x", NULL);
>>> +
>>> +	snprintf(name, sizeof(name), "i2c%u-%02x",
>>> +		 data->client->adapter->nr, data->client->addr);
>>> +	sensor_dir = debugfs_create_dir(name, data->debugfs);
>>> +	debugfs_create_u32("serial_number", 0444,
>>> +			   sensor_dir, &data->serial_number);
>>> +}
>>> +
>>> +#else
>>> +
>>> +static void sht3x_debugfs_init(struct sht3x_data *data)
>>> +{
>>> +}
>>> +
>>> +#endif
>>
>> debugfs doesn't need if/else or error handling.
>>
> Do you mean the IS_ERR_OR_NULL? I included that to get rid of the
> "debugfs directory already exists" message when using multiple sensors.
> 

A much easier way to do that would be to create the directory in the init function,
and to use a static variable to point to it. Note that you'll also need need an
explicit exit function to remove it when the driver is unloaded.

Thanks,
Guenter