Re: [PATCH v2 3/5] iio: chemical: sgp40: Implement get_serial_number-command

[Joshua Crofts <crofts@kernel.org>]

On Mon, 2 Nov 2026 11:12:56 +0200
Jaakko Koivisto <jmatko@utu.fi> wrote:

> Add functionality for retrieving SGP40 sensor serial number
> and present it to the userspace using serialnumber ABI.
> 
> Signed-off-by: Jaakko Koivisto <jmatko@utu.fi>
> ---
> Currently only the bno055 driver uses the serialnumber ABI, and presents
> the SN in hex. Documentation gives hex output as example format, not hard
> requirement, but should we use the same format here anyway?
> 
>  drivers/iio/chemical/sgp40.c | 76 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 76 insertions(+)
> 
> diff --git a/drivers/iio/chemical/sgp40.c b/drivers/iio/chemical/sgp40.c
> index 080d51f9f53c..e0a300720fce 100644
> --- a/drivers/iio/chemical/sgp40.c
> +++ b/drivers/iio/chemical/sgp40.c
> @@ -35,6 +35,7 @@
>  #include <linux/mutex.h>
>  #include <linux/i2c.h>
>  #include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>

Could you add an additional patch to your series adding any
missing includes? Just looking at this patch I can see that
types.h, asm/byteorder.h and delay.h are missing.

>  
>  /*
>   * floating point calculation of voc is done as integer
> @@ -70,6 +71,18 @@ struct sgp40_tg_result {
>  	u8	res_crc;
>  } __packed;
>  
> +/*
> + * Datasheet table 16. Serial number is given as 48-bit value 0xAAAABBBBCCCC.
> + */
> +struct sgp40_serial_number_result {
> +	__be16	A;
> +	u8	A_crc;
> +	__be16	B;
> +	u8	B_crc;
> +	__be16	C;
> +	u8	C_crc;
> +} __packed;
> +
>  static const struct iio_chan_spec sgp40_channels[] = {
>  	{
>  		.type = IIO_CONCENTRATION,
> @@ -161,6 +174,41 @@ static int sgp40_calc_voc(struct sgp40_data *data, u16 resistance_raw, int *voc)
>  	return 0;
>  }
>  
> +static int sgp40_get_serial_number(struct sgp40_data *data, u64 *serial_number)
> +{
> +	int ret;
> +	struct i2c_client *client = data->client;
> +	struct sgp40_tg_measure get_sn = { .command = {0x36, 0x82} };
> +	struct sgp40_serial_number_result res;

Reverse xmas tree order.

> +
> +	guard(mutex)(&data->lock);
> +
> +	ret = i2c_master_send(client, (char *)&get_sn, sizeof(get_sn.command));
> +	if (ret < 0)
> +		return ret;
> +	if (ret != sizeof(get_sn.command))
> +		return dev_err_probe(data->dev, -EIO,

Sashiko points out that this function isn't called from probe(),
therefore it shouldn't use dev_err_probe().

Also, you can use a local variable for dev.

> +		       "i2c_master_send ret: %d, expected %zu\n", ret, sizeof(get_sn.command));
> +
> +	fsleep(1000); /* Max duration for get_serial_number command. Datasheet table 8. */
> +
> +	ret = i2c_master_recv(client, (char *)&res, sizeof(res));
> +	if (ret < 0)
> +		return ret;
> +	if (ret != sizeof(res))
> +		return dev_err_probe(data->dev, -EIO,
> +		       "i2c_master_recv ret: %d, expected: %zu\n", ret, sizeof(res));
> +
> +	if (crc8(sgp40_crc8_table, (u8 *)&res.A, 2, SGP40_CRC8_INIT) != res.A_crc ||
> +	    crc8(sgp40_crc8_table, (u8 *)&res.B, 2, SGP40_CRC8_INIT) != res.B_crc ||
> +	    crc8(sgp40_crc8_table, (u8 *)&res.C, 2, SGP40_CRC8_INIT) != res.C_crc)
> +		return dev_err_probe(data->dev, -EIO, "CRC error in get_serial_number\n");
> +
> +	*serial_number = ((u64)be16_to_cpu(res.A) << 32) | ((u64)be16_to_cpu(res.B) << 16) | (u64)be16_to_cpu(res.C);
> +	return 0;
> +}
> +
>  static int sgp40_measure_resistance_raw(struct sgp40_data *data, u16 *resistance_raw)
>  {
>  	int ret;
> @@ -306,9 +354,37 @@ static int sgp40_write_raw(struct iio_dev *indio_dev,
>  	return -EINVAL;
>  }
>  
> +static ssize_t serialnumber_show(struct device *dev,
> +				  struct device_attribute *attr,
> +				  char *buf)
> +{
> +	int ret;
> +	u64 serial_number;
> +	struct sgp40_data *data = iio_priv(dev_to_iio_dev(dev));

Reverse xmas tree.

...


-- 
Kind regards,
Joshua Crofts