[v3,1/1] hwmon: (sht3x) read out sensor serial number

  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         | 56 ++++++++++++++++++++++++++++++++++-
 2 files changed, 66 insertions(+), 1 deletion(-)
  

On Wed, Dec 27, 2023 at 08:00:36PM +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         | 56 ++++++++++++++++++++++++++++++++++-
>  2 files changed, 66 insertions(+), 1 deletion(-)
> 
> 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..e016e0d9a6c4 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,9 @@ 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 };
> +
> +static struct dentry *debugfs;
>  
>  /* delays for single-shot mode i2c commands, both in us */
>  #define SHT3X_SINGLE_WAIT_TIME_HPM  15000
> @@ -169,6 +173,7 @@ struct sht3x_data {
>  	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 +836,36 @@ static int sht3x_write(struct device *dev, enum hwmon_sensor_types type,
>  	}
>  }
>  
> +static void sht3x_debugfs_init(struct sht3x_data *data)
> +{
> +	char name[32];
> +	struct dentry *sensor_dir;
> +
> +	snprintf(name, sizeof(name), "i2c%u-%02x",
> +		 data->client->adapter->nr, data->client->addr);
> +	sensor_dir = debugfs_create_dir(name, debugfs);
> +	debugfs_create_u32("serial_number", 0444,
> +			   sensor_dir, &data->serial_number);

This creates i2c<bus>-<address>/serial_number when the device is instantiated.
That debugfs entry is not removed when the device is removed, only when the
driver is unloaded. This means that de-instantiating the device will leave
stray debugfs directories and files behind until the driver is unloaded.

We had this before, and I understand that you claimed that this doesn't happen.
To get me to believe you, you'll have to provide a log of

- instantiating the driver 
- Showing the debufs tree
- de-instantiating the driver
- Showing the debugfs tree

... but even then I'll want to be able to test it myself. Not sure if I
have an eval board, but either case that will take some time. Frankly,
I don't understand why you refuse to remove
i2c<bus>-<address>/serial_number on device removal.

Guenter

> +}
> +
> +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 +934,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");
> +	} else {
> +		sht3x_debugfs_init(data);
> +	}
> +
>  	hwmon_dev = devm_hwmon_device_register_with_info(dev,
>  							 client->name,
>  							 data,
> @@ -917,7 +959,19 @@ static struct i2c_driver sht3x_i2c_driver = {
>  	.id_table    = sht3x_ids,
>  };
>  
> -module_i2c_driver(sht3x_i2c_driver);
> +static int __init sht3x_init(void)
> +{
> +	debugfs = debugfs_create_dir("sht3x", NULL);
> +	return i2c_add_driver(&sht3x_i2c_driver);
> +}
> +module_init(sht3x_init);
> +
> +static void __exit sht3x_cleanup(void)
> +{
> +	debugfs_remove_recursive(debugfs);
> +	i2c_del_driver(&sht3x_i2c_driver);
> +}
> +module_exit(sht3x_cleanup);
>  
>  MODULE_AUTHOR("David Frey <david.frey@sensirion.com>");
>  MODULE_AUTHOR("Pascal Sachs <pascal.sachs@sensirion.com>");
  

On Sun, Dec 31, 2023 at 10:32:56AM -0800, Guenter Roeck wrote:
> This creates i2c<bus>-<address>/serial_number when the device is instantiated.
> That debugfs entry is not removed when the device is removed, only when the
> driver is unloaded. This means that de-instantiating the device will leave
> stray debugfs directories and files behind until the driver is unloaded.
> 
> We had this before, and I understand that you claimed that this doesn't happen.
> To get me to believe you, you'll have to provide a log of
> 
> - instantiating the driver 
> - Showing the debufs tree
> - de-instantiating the driver
> - Showing the debugfs tree
> 
> ... but even then I'll want to be able to test it myself. Not sure if I
> have an eval board, but either case that will take some time. Frankly,
> I don't understand why you refuse to remove
> i2c<bus>-<address>/serial_number on device removal.
> 
> Guenter
> 

Hi Guenter,

Thank you for your patience. As this is my first patch set for Linux I still
need to learn a lot.

You are right. I was confused about driver instantiation and driver
loading/unloading. The i2cX-XX directory needs to be removed explicitly.

If I understood correctly, the following changes should achieve this:

+static void sht3x_remove(struct i2c_client *client)
+{
+	struct sht3x_data *data;
+
+	data = dev_get_drvdata(&client->dev);
+	debugfs_remove_recursive(data->sensor_dir);
+}
+
 static struct i2c_driver sht3x_i2c_driver = {
 	.driver.name = "sht3x",
 	.probe       = sht3x_probe,
+	.remove      = sht3x_remove,
 	.id_table    = sht3x_ids,
 };

Of course data->sensor_dir needs to be set to the i2X-XX directory when it is
created.

If there is nothing obviously wrong with it I'll submit v4 shortly.

Best,
Stefan
  

On Mon, Jan 01, 2024 at 11:18:21PM +0100, Stefan Gloor wrote:
> On Sun, Dec 31, 2023 at 10:32:56AM -0800, Guenter Roeck wrote:
> > This creates i2c<bus>-<address>/serial_number when the device is instantiated.
> > That debugfs entry is not removed when the device is removed, only when the
> > driver is unloaded. This means that de-instantiating the device will leave
> > stray debugfs directories and files behind until the driver is unloaded.
> > 
> > We had this before, and I understand that you claimed that this doesn't happen.
> > To get me to believe you, you'll have to provide a log of
> > 
> > - instantiating the driver 
> > - Showing the debufs tree
> > - de-instantiating the driver
> > - Showing the debugfs tree
> > 
> > ... but even then I'll want to be able to test it myself. Not sure if I
> > have an eval board, but either case that will take some time. Frankly,
> > I don't understand why you refuse to remove
> > i2c<bus>-<address>/serial_number on device removal.
> > 
> > Guenter
> > 
> 
> Hi Guenter,
> 
> Thank you for your patience. As this is my first patch set for Linux I still
> need to learn a lot.
> 
> You are right. I was confused about driver instantiation and driver
> loading/unloading. The i2cX-XX directory needs to be removed explicitly.
> 
> If I understood correctly, the following changes should achieve this:
> 
> +static void sht3x_remove(struct i2c_client *client)
> +{
> +	struct sht3x_data *data;
> +
> +	data = dev_get_drvdata(&client->dev);
> +	debugfs_remove_recursive(data->sensor_dir);
> +}
> +
>  static struct i2c_driver sht3x_i2c_driver = {
>  	.driver.name = "sht3x",
>  	.probe       = sht3x_probe,
> +	.remove      = sht3x_remove,
>  	.id_table    = sht3x_ids,
>  };
> 
> Of course data->sensor_dir needs to be set to the i2X-XX directory when it is
> created.
> 
> If there is nothing obviously wrong with it I'll submit v4 shortly.
> 

This is correct. I personally would prefer the use of devm_add_action_or_reset()
instead of a remove function, but the above works as well. Make sure though
that debugfs_remove_recursive() is also called if hwmon registration fails.

Thanks,
Guenter