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

[Jaakko Koivisto <jmatko@utu.fi>]

On Fri Sep 18, 2026 at 5:26 PM EEST, Maxwell Doose wrote:
> Hi there Jaakko,
>
> Firstly a (rather important) question I have is what's the point? I'm
> not sure that anyone will need the serial number on the fly (assuming a
> business would be using this, they will likely keep records of their
> parts). Though maybe I could be wrong (so feel free to prove me wrong).

I agree that everybody should keep accurate records. Whether or not they
do, and those records are fast and easy to access, is another question.
Stuff like this gets lost in company mergers, or the records are
buried somewhere in manufacturing department archives.
I don't see the downside in having fast and easy way to, for example,
check that the records are in fact correct.

You are probably correct that it won't be called for often. Saving the
serial number to driver data is probably overkill, and better to just
read it when requested. Would remove one variable and less code in
probe()-function.

>
> On Fri Sep 18, 2026 at 8:40 AM CDT
> Jaakko Koivisto <jmatko@utu.fi> wrote:
>
>> -Retrieve the chip serial number.
>> -Present the serial number to userspace as device attribute.
>> -Rename the tg_measure -struct now that is is used for multiple
>> commands.
>>
>
> Last one should be left out and put in a separate patch.
>

Probably best to leave it out, not strictly necessary.

>
> Also, commit message needs a bit of work, something like:
> 	"Add support to the SGP40 driver to enable retrieval of the
> 	serial number from the chip and add new sysfs attribute to
> 	expose the serial number to userspace."
>

Thanks, I'll improve it for v2.

>> Signed-off-by: Jaakko Koivisto <jmatko@utu.fi>
>> ---
>>  drivers/iio/chemical/sgp40.c | 78 +++++++++++++++++++++++++++++++++++-
>>  1 file changed, 76 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/iio/chemical/sgp40.c b/drivers/iio/chemical/sgp40.c
>> index b2b5e32a9eb2..c1e2a992ec2a 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>
>>  
>>  /*
>>   * floating point calculation of voc is done as integer
>> @@ -53,11 +54,12 @@ struct sgp40_data {
>>  	int			rht;
>>  	int			temp;
>>  	int			res_calibbias;
>> +	u64			serial_number;
>>  	/* Prevent concurrent access to rht, tmp, calibbias */
>>  	struct mutex		lock;
>>  };
>>  
>> -struct sgp40_tg_measure {
>> +struct sgp40_command {
>>  	u8	command[2];
>>  	__be16	rht_ticks;
>>  	u8	rht_crc;
>> @@ -70,6 +72,18 @@ struct sgp40_tg_result {
>>  	u8	res_crc;
>>  } __packed;
>>  
>
> Name change should be put in a different patch or just left out
> entirely.
>

I'll leave it out.

>
>> +/*
>> + * 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;
>> +
>
> Perhaps this but maybe this could be implemented as an
> annonymous struct instead.
>

This I would like to leave as it is in order to have all the command
and result structs follow the same pattern as the already existing
sgp40_tg_result and sgp40_tg_measure.

>>  static const struct iio_chan_spec sgp40_channels[] = {
>>  	{
>>  		.type = IIO_CONCENTRATION,
>> @@ -162,6 +176,40 @@ 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)
>> +{
>> +	int ret;
>> +	struct i2c_client *client = data->client;
>> +	struct sgp40_command get_sn = {.command = {0x36, 0x82}};
>> +	struct sgp40_serial_number_result res;
>> +
>> +	ret = i2c_master_send(client, (char*)&get_sn, sizeof(get_sn.command));
>> +	if (ret != sizeof(get_sn.command)) {
>> +		dev_err(data->dev, "i2c_master_send ret: %d, expected %zu", ret, sizeof(get_sn.command));
>
> Missing '\n' here (sashiko).

I'll add this and other missing '\n's.

>> +		return -EIO;
>> +	}
>> +	msleep(1);
>> +	ret = i2c_master_recv(client, (char*)&res, sizeof(res));
>> +	if (ret < 0)
>> +		return ret;
>> +	if (ret != sizeof(res)) {
>> +		dev_err(data->dev, "i2c_master_recv ret: %d, expected: %zu", ret, sizeof(res));
>> +		return -EIO;
>> +	}
>> +
>> +	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)
>> +	{
>> +		dev_warn(data->dev, "CRC error in get_serial_number");
>
> Probably should return either -EIO or -EREMOTEIO here (sashiko).
>

Will change for v2.

>> +	}
>> +
>> +	data->serial_number = 0LL | ((u64)be16_to_cpu(res.A) << 32) | ((u64)be16_to_cpu(res.B) << 16) | (u64)be16_to_cpu(res.C);
>> +	dev_dbg(data->dev, "serial number: %llu", data->serial_number);
>> +
>> +	return 0;
>> +}
>> +
>>  static int sgp40_measure_resistance_raw(struct sgp40_data *data, u16 *resistance_raw)
>>  {
>>  	int ret;
>> @@ -169,7 +217,7 @@ static int sgp40_measure_resistance_raw(struct sgp40_data *data, u16 *resistance
>>  	u32 ticks;
>>  	u16 ticks16;
>>  	u8 crc;
>> -	struct sgp40_tg_measure tg = {.command = {0x26, 0x0F}};
>> +	struct sgp40_command tg = {.command = {0x26, 0x0F}};
>>  	struct sgp40_tg_result tgres;
>>  
>>  	mutex_lock(&data->lock);
>> @@ -311,9 +359,31 @@ static int sgp40_write_raw(struct iio_dev *indio_dev,
>>  	return -EINVAL;
>>  }
>>  
>> +static ssize_t serial_number_show(struct device *dev,
>> +				  struct device_attribute *attr,
>> +				  char *buf)
>> +{
>> +	struct sgp40_data *data = iio_priv(dev_to_iio_dev(dev));
>> +
>> +	return sysfs_emit_at(buf, 0, "%llu\n", data->serial_number);
>> +}
>> +
>> +static IIO_DEVICE_ATTR_RO(serial_number, 0);
>> +
>> +static struct attribute *sgp40_attributes[] = {
>> +	&iio_dev_attr_serial_number.dev_attr.attr,
>> +	NULL
>> +};
>> +
>> +static struct attribute_group sgp40_attribute_group = {
>> +	.attrs = sgp40_attributes,
>> +};
>> +
>> +
>>  static const struct iio_info sgp40_info = {
>>  	.read_raw	= sgp40_read_raw,
>>  	.write_raw	= sgp40_write_raw,
>> +	.attrs		= &sgp40_attribute_group,
>>  };
>>  
>>  static int sgp40_probe(struct i2c_client *client)
>> @@ -347,6 +417,10 @@ static int sgp40_probe(struct i2c_client *client)
>>  	indio_dev->channels = sgp40_channels;
>>  	indio_dev->num_channels = ARRAY_SIZE(sgp40_channels);
>>  
>> +	ret = sgp40_get_serial_number(data);
>> +	if (ret)
>> +		dev_warn(dev, "failed to retrieve device serial number\n");
>> +
>>  	ret = devm_iio_device_register(dev, indio_dev);
>>  	if (ret)
>>  		dev_err(dev, "failed to register iio device\n");