Re: [PATCH 2/3] iio: chemical: sgp40: Implement execute_self_test-command

[Jaakko Koivisto <jmatko@utu.fi>]

On Sun Sep 20, 2026 at 8:48 PM EEST, Jonathan Cameron wrote:
> On Fri, 18 Sep 2026 16:40:18 +0300
> Jaakko Koivisto <jmatko@utu.fi> wrote:
>
>> -Run the chip self test routine testing heater and MOX -material.
>> -Log error if the test fails.
>> 
>> Datasheet does not clearly state if the chip is completely unusable if
>> the test fails. Because of this the driver will try to use the chip
>> normally even if the self-test has failed.
>> 
>> Signed-off-by: Jaakko Koivisto <jmatko@utu.fi>
> Hi Jaakko,
>
> A few things inline,
>
> Thanks,
>
> Jonathan
>
>> ---
>>  drivers/iio/chemical/sgp40.c | 53 ++++++++++++++++++++++++++++++++++++
>>  1 file changed, 53 insertions(+)
>> 
>> diff --git a/drivers/iio/chemical/sgp40.c b/drivers/iio/chemical/sgp40.c
>> index c1e2a992ec2a..28d5e737d1dc 100644
>> --- a/drivers/iio/chemical/sgp40.c
>> +++ b/drivers/iio/chemical/sgp40.c
>> @@ -84,6 +84,19 @@ struct sgp40_serial_number_result {
>>  	u8	C_crc;
>>  } __packed;
>>  
>> +/*
>> + * Datasheet table 13.
>> + */
>> +#define SGP40_SELF_TEST_PASS 0xD4
>> +#define SGP40_SELF_TEST_FAIL 0x4B
>> +
>> +struct sgp40_self_test_result {
>> +	u8	data;
>> +	u8	_ignore;
>> +	u8	crc;
>> +} __packed;
>> +
>> +
>>  static const struct iio_chan_spec sgp40_channels[] = {
>>  	{
>>  		.type = IIO_CONCENTRATION,
>> @@ -210,6 +223,42 @@ static int sgp40_get_serial_number(struct sgp40_data *data)
>>  	return 0;
>>  }
>>  
>> +static int sgp40_execute_self_test(struct sgp40_data *data)
>> +{
>> +	int ret;
>> +	struct i2c_client *client = data->client;
>> +	struct sgp40_command run_test = {.command = {0x28, 0x0E}};
>> +	struct sgp40_self_test_result res;
>> +
>> +	ret = i2c_master_send(client, (char*)&run_test, sizeof(run_test.command));
>> +	if (ret != sizeof(run_test.command)) {
>> +		dev_err(data->dev, "i2c_master_send ret: %d, expected %zu", ret, sizeof(run_test.command));
>> +		return -EIO;
> Only called from problem so prefer use of
>
> 		return dev_err_probe().
>
> Also don't eat the error if it returns on (rather than a short access).
>

I will change it, thanks.

>
>> +	}
>> +	msleep(320);
>
> Spec reference needed for any sleep times.
>

It's the maximum time the chip takes to execute the self-test. All
command times are found in datasheet table 8. I'll add a comment here.

>> +
>> +	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;
>
> return dev_err_probe() and don't eat the error return if there is one.

Will change.

>> +	}
>> +
>> +	if (crc8(sgp40_crc8_table, (u8*)&res, 2, SGP40_CRC8_INIT) != res.crc) {
>> +		dev_warn(data->dev, "CRC error in execute_self_test");
>
> CRC fail indicates broken comms. I'd fail hard on this one.
>
Ditto.

>> +	}
>> +
>> +	dev_dbg(data->dev, "self test result: 0x%x", res.data);
>> +	switch (res.data) {
>> +		case SGP40_SELF_TEST_PASS:
>> +			return 0;
>> +		case SGP40_SELF_TEST_FAIL:
>> +		default:
>> +			return res.data;
>> +	}
>> +}
>> +
>>  static int sgp40_measure_resistance_raw(struct sgp40_data *data, u16 *resistance_raw)
>>  {
>>  	int ret;
>> @@ -421,6 +470,10 @@ static int sgp40_probe(struct i2c_client *client)
>>  	if (ret)
>>  		dev_warn(dev, "failed to retrieve device serial number\n");
>>  
>> +	ret = sgp40_execute_self_test(data);
>> +	if (ret)
>> +		dev_warn(dev, "device self test failed: ret 0x%x", ret);
>
> There should be a distinction between the test failed to run and failed to pass.
> For failing to run (e.g. comms error) then fail the probe.  Failing to pass
> the test is what you might choose to carry on from.

I'll change it to include failing to run the test.

>
>> +
>>  	ret = devm_iio_device_register(dev, indio_dev);
>>  	if (ret)
>>  		dev_err(dev, "failed to register iio device\n");