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

[Jaakko Koivisto <jmatko@utu.fi>]

On Sat Sep 19, 2026 at 5:09 PM EEST, Andy Shevchenko wrote:
> On Fri, Sep 18, 2026 at 04:40:17PM +0300, Jaakko Koivisto 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.
>
> ...
>
>> +	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));
>> +		return -EIO;
>> +	}
>> +	msleep(1);
>
> Explain this sleep.

It is the maximum time it takes for the chip to execute the command.
I'll add a comment describing it.

In testing this command works if the sleep is omitted, but I would leave
it here anyway to be sure we don't try to read the result before the
chip is ready.

>> +	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");
>> +	}
>> +
>> +	data->serial_number = 0LL | ((u64)be16_to_cpu(res.A) << 32) | ((u64)be16_to_cpu(res.B) << 16) | (u64)be16_to_cpu(res.C);
>
> 0LL ?!
>

I have no excuse for this. Both unnecessary and for u64 should have been
LLU. I will remove it.

>> +	dev_dbg(data->dev, "serial number: %llu", data->serial_number);
>
> Why?!
>

You are right, not really debug information. Will remove.

>
> ...
>
>> -	struct sgp40_tg_measure tg = {.command = {0x26, 0x0F}};
>> +	struct sgp40_command tg = {.command = {0x26, 0x0F}};
>
> While at it, add more spaces.
>

Thanks, I'll fix this and other formatting mistakes.

> ...
>
>> +}
>> +
>
> Unneeded blank line.
>
>> +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,
>> +};
>> +
>> +
>
> Single blank line is enough and we have ATTRIBUTE_GROUPS().
>

Is using ATTRIBUTE_GROUPS() valid here? ATTRIBUTE_GROUPS(sgp40) will
create both attribute_group sgp40_group and
attribute_group *sgp40_groups[], and we only need the first one for iio_info.
The extra *sgp40_groups[] will create unused variable -warning.