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@xxxxxxxxxxxxxxx>
> ---
>  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,