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

[Maxwell Doose]

Hi Jonathan,

On Fri, 3 Jul 2026 01:05:32 +0100
Jonathan Cameron <jic23@xxxxxxxxxx> wrote:

> On Sat, 20 Jun 2026 19:46:24 -0500
> Maxwell Doose <m32285159@xxxxxxxxx> wrote:
> 
> > Add a driver for the Sensirion STS30 family of temperature sensor
> > drivers over I2C. The STS30 family of sensors includes the STS30, STS31,
> > and STS35, all of which are supported by this driver, since they all
> > share the same commands, etc. and only differ in accuracy and tolerance.
> > 
> > The driver currently supports single-shot non-clock stretched readings,
> > by using a specified delay based on the repeatability/delay specified
> > by the user. The repeatability/delay can be changed at any time through
> > sysfs.
> > 
> > Additionally add Kconfig and Makefile entries for the driver as well as
> > a MAINTAINERS entry.
> > 
> > Signed-off-by: Maxwell Doose <m32285159@xxxxxxxxx>  
> 
> Big question first.  Why IIO? These are fairly basic temperature sensors
> which typically means hwmon is more appropriate.  What does it need
> that hwmon doesn't provide?
> 

The datasheet says that the STS30 supports continuous reads which means
that we can read it into a triggered buffer (which hwmon doesn't
support) and at some point I'd like to implement that.

Additionally I don't have much experience (or any for that matter) in
hwmon.

> A few other things inline.
> 
> > diff --git a/drivers/iio/temperature/sts30.c b/drivers/iio/temperature/sts30.c
> > new file mode 100644
> > index 000000000000..dcfe3435ae5a
> > --- /dev/null
> > +++ b/drivers/iio/temperature/sts30.c  
> 
> > +
> > +/* Size of the temperature measurement data received after a read command */
> > +#define STS30_TEMP_MEAS_SIZE 2
> > +
> > +#define STS30_COMMAND_READ_HIGH_REPEAT 0x2400
> > +#define STS30_COMMAND_READ_MED_REPEAT 0x240B
> > +#define STS30_COMMAND_READ_LOW_REPEAT 0x2416  
> That smells like two fields in one value.
> 
> > +
> > +#define STS30_COMMAND_RESET 0x30A2  
> 
> > +enum sts30_read_delays {   
> 
> Name it to indicate unit.
> 
> > +	STS30_REPEAT_LOW = 4500,
> > +	STS30_REPEAT_MED = 6000,
> > +	STS30_REPEAT_HIGH = 15000
> > +};
> > +  
> 
> > +static int sts30_read(struct sts30_data *data, u16 command, u16 *val)
> > +{
> > +	u8 buf[STS30_MEAS_SIZE];
> > +	u8 tmp[2];  
> Might as well make it __be16  then it's aligned.

Makes sense.

>
> > +	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);
> > +
> > +	ret = i2c_master_recv(data->client, buf, sizeof(buf));
> > +	if (ret < 0)
> > +		return ret;
> > +	if (ret != sizeof(buf))
> > +		return -EIO;
> > +
> > +	*val = get_unaligned_be16(buf);
> > +
> > +	ret = sts30_verify_crc8(data, buf);  
> 
> return sts30_...
> 

D'oh, don't know how I missed this.

>
> > +	if (ret)
> > +		return ret;
> > +
> > +	return 0;
> > +}  
> 
> > +
> > +static int sts30_read_raw(struct iio_dev *indio_dev,
> > +			  struct iio_chan_spec const *chan, int *val, int *val2,
> > +			  long mask)
> > +{
> > +	struct sts30_data *data = iio_priv(indio_dev);
> > +	int ret;
> > +	u16 tmp;
> > +
> > +	guard(mutex)(&data->lock);  
> 
> I'd move this into appropriate scope as the lock isn't needed for
> all the const data cases.
> 

Will do.

>
> > +
> > +	switch (mask) {
> > +	case IIO_CHAN_INFO_RAW:
> > +		switch (data->delay) {
> > +		case STS30_REPEAT_LOW:
> > +			ret = sts30_read(data, STS30_COMMAND_READ_LOW_REPEAT, &tmp);
> > +			break;
> > +		case STS30_REPEAT_MED:
> > +			ret = sts30_read(data, STS30_COMMAND_READ_MED_REPEAT, &tmp);
> > +			break;
> > +		case STS30_REPEAT_HIGH:
> > +			ret = sts30_read(data, STS30_COMMAND_READ_HIGH_REPEAT, &tmp);
> > +			break;
> > +		default:
> > +			dev_warn(&data->client->dev, "Repeatability state corrupted, got: %d\n",
> > +				 data->delay);  
> 
> Any realistic way this can happen?  If not drop the print.
>

Probably not (unless of course a cosmic ray flips a bit or two).

>
> > +			return -EINVAL;
> > +		}
> > +
> > +		if (ret)
> > +			return ret;
> > +
> > +		*val = tmp;
> > +		return IIO_VAL_INT;
> > +	case IIO_CHAN_INFO_OFFSET:
> > +		*val = STS30_TEMP_OFFSET;  
> 
> These constant cases don't need the lock.
> > +		return IIO_VAL_INT;
> > +	case IIO_CHAN_INFO_SCALE:
> > +		*val = 175000;
> > +		*val2 = 65535;
> > +		return IIO_VAL_FRACTIONAL;
> > +	case IIO_CHAN_INFO_INT_TIME:
> > +		*val = 0;
> > +		*val2 = data->delay;  
> 
> Might need the guard - I haven't checked.
>

I'm tempted to lock that read since it's shared, but I'll take a look
into it.

>
> > +		return IIO_VAL_INT_PLUS_MICRO;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +}
> > +
> > +static int sts30_write_raw(struct iio_dev *indio_dev,
> > +			   struct iio_chan_spec const *chan, int val, int val2,
> > +			   long mask)
> > +{
> > +	struct sts30_data *data = iio_priv(indio_dev);
> > +
> > +	if (val)
> > +		return -EINVAL;  
> 
> That is going to be IIO_* specific in the switch - so move it into
> the case block.
> 

Good point.

>
> > +
> > +	switch (mask) {
> > +	case IIO_CHAN_INFO_INT_TIME: {
> > +		guard(mutex)(&data->lock);  
> 
> Given it is taken in all non error paths, I'd lift the guard() up to outside
> the switch.
>

Will do.

>
> > +
> > +		switch (val2) {
> > +		case STS30_REPEAT_LOW:
> > +			data->delay = STS30_REPEAT_LOW;
> > +			break;  
> 
> Given you are done in each of these, return instead of break.
>

D'oh, also don't know how I missed this.

>
> > +		case STS30_REPEAT_MED:
> > +			data->delay = STS30_REPEAT_MED;
> > +			break;
> > +		case STS30_REPEAT_HIGH:
> > +			data->delay = STS30_REPEAT_HIGH;
> > +			break;
> > +		default:
> > +			return -EINVAL;
> > +		}
> > +
> > +		return 0;
> > +	}
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +}
> > +
> > +static const struct iio_info sts30_info = {
> > +	.read_raw = sts30_read_raw,
> > +	.write_raw = sts30_write_raw  
> Missing comma.
> 
> Basic rule of these is you can only skip the comma if doing so doesn't create additional
> churn when adding new stuff after it.  Two cases are common.
> 1) Nothing can be added there - true of terminators  {} etc.
> 2) It's one line anyway so any change will result in that line changing
>    and hence no advantage in having the comma.
>

Ah. Will fix.

>
> > +};
> > +
> > +static const struct iio_chan_spec sts30_channels[] = {
> > +	{
> > +		.type = IIO_TEMP,
> > +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_OFFSET) |
> > +				      BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_INT_TIME)  
> 
> No obvious gain in going longer than 80 chars here.  Just have one per line.
> Also the , is missing
>

Will fix up.

>
> > +	},
> > +};
> > +
> > +static int sts30_probe(struct i2c_client *client)
> > +{
> > +	struct iio_dev *indio_dev;
> > +	struct sts30_data *data;
> > +	int ret;
> > +
> > +	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> > +	if (!indio_dev)
> > +		return -ENOMEM;
> > +
> > +	indio_dev->name = client->name;  
> 
> I'd prefer to see that done via device specific chip_info structures.
> IIRC there are some paths in which client->name might not be set appropriately.
> For ACPI PRP0001 (the route that uses compatible) it is set to the vendor stripped
> name so that is fine, but should we ever add 'proper' ACPI support it will be the
> _HID which isn't what we want.
> 
> Anyhow, normally we avoid thinking about this by getting from the data
> rather than the ->name via i2c_get_match_data() and appropriate structures.
> 

Alright then, will do.

>
> > +	indio_dev->info = &sts30_info;
> > +	indio_dev->channels = sts30_channels;
> > +	indio_dev->num_channels = ARRAY_SIZE(sts30_channels);
> > +	indio_dev->modes = INDIO_DIRECT_MODE;
> > +
> > +	data = iio_priv(indio_dev);
> > +	data->client = client;
> > +	data->delay = STS30_REPEAT_HIGH;
> > +
> > +	ret = devm_mutex_init(&client->dev, &data->lock);
> > +	if (ret)
> > +		return ret;
> > +
> > +	i2c_set_clientdata(client, indio_dev);  
> 
> Why? I'm not seeing it being used.
> 

Will just get rid of it then.

I'm probably a numpty for submitting this without the hardware :( I'll
probably just withhold the v3 until I can get hardware (or at least
submit as an RFC).

-- 
best regards,
max