Re: [RFC PATCH 2/3] iio: temperature: Add STS30 temperature sensor driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The return value of `sts30_reset()` is ignored during probe, allowing phantom device registration on failure.
- [High] The driver implements a Clock Stretching Disabled I2C sequence but uses Clock Stretching Enabled command codes, resulting in a protocol mismatch.
--

commit 1a183c37bbba5fe87dfcc6019f964e51526fec2b
Author: Maxwell Doose <m32285159@gmail.com>

iio: temperature: Add STS30 temperature sensor driver

Add a driver for the Sensirion STS30 family of temperature sensor
drivers over I2C. The driver supports single-shot non-clock stretched
readings using a specified delay based on the repeatability/delay.

> diff --git a/drivers/iio/temperature/sts30.c b/drivers/iio/temperature/sts30.c
> new file mode 100644
> index 0000000000000..b49deb1d62c21
> --- /dev/null
> +++ b/drivers/iio/temperature/sts30.c

[ ... ]

> +#define STS30_MEAS_SIZE 3
> +
> +#define STS30_COMMAND_READ_HIGH_REPEAT 0x2C06
> +#define STS30_COMMAND_READ_MED_REPEAT 0x2C0D
> +#define STS30_COMMAND_READ_LOW_REPEAT 0x2C10

[Severity: High]
The commit message mentions using non-clock stretched readings, but are
these the correct commands? According to the STS3x datasheet, the 0x2Cxx
series commands are for "Clock Stretching Enabled", which require a
Repeated START condition between the write and read phases.

[ ... ]

> +static int sts30_read(struct sts30_data *data, u16 command, u16 *val)
> +{
> +	u8 tmp[2];
> +	u8 buf[STS30_MEAS_SIZE];
> +	int ret;
> +
> +	put_unaligned_be16(command, tmp);
> +
> +	ret = i2c_master_send(data->client, tmp, sizeof(tmp));
> +	if (ret < 0)
> +		return ret;
> +	if (ret != sizeof(tmp))
> +		return -EIO;
> +
> +	fsleep(data->delay);

[Severity: High]
Will sending a STOP condition here abort the measurement? Since
i2c_master_send() issues a STOP condition rather than a Repeated START,
this sequence violates the protocol for the Clock Stretching Enabled
commands (0x2Cxx) defined above.

Should this use the "Clock Stretching Disabled" commands (e.g., 0x2400)
instead, or alternatively use a single i2c_transfer() with a Repeated START?

[ ... ]

> +static int sts30_probe(struct i2c_client *client)
> +{
> +	struct iio_dev *indio_dev;
> +	struct sts30_data *data;
> +	int ret;

[ ... ]

> +	ret = sts30_reset(data);
> +
> +	return devm_iio_device_register(&client->dev, indio_dev);
> +}

[Severity: High]
Is it intentional to ignore the return value of sts30_reset() here?

If the reset fails due to the hardware being missing or a broken I2C bus,
this will unconditionally register a phantom device to userspace. Should we
check 'ret' and return the error code instead?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260620044010.1082621-1-m32285159@gmail.com?part=2