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

[Jonathan Cameron <jic23@kernel.org>]

On Sat, 4 Jul 2026 14:45:03 -0500
Maxwell Doose <m32285159@gmail.com> wrote:

> Hi Jonathan,
> 
> On Fri, 3 Jul 2026 01:05:32 +0100
> Jonathan Cameron <jic23@kernel.org> wrote:
> 
> > On Sat, 20 Jun 2026 19:46:24 -0500
> > Maxwell Doose <m32285159@gmail.com> 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@gmail.com>    
> > 
> > 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.

Hmm. For a temperature chip that is often more about the monitors running
continuously than the ability to gather signals fast.  Temperature changes
tend to be fairly slow when it's a physical sensor - gets less obvious when
infrared remote sensors are involved.

Even more relevant - there is an existing driver I think.
See drivers/hwmon/sht3x.c which supports things under the wildcard sts3x as well.
Check if that covers this part.

Thanks,

Jonathan


> 
> 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).
>