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

[Joshua Crofts <joshua.crofts1@gmail.com>]

Hi Max,

comments inline, some nits, some more serious, some Sashiko reviews.

Josh

On Fri, 19 Jun 2026 23:40:06 -0500
Maxwell Doose <m32285159@gmail.com> wrote:
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2026 Maxwell Doose
> + *
> + * Sensirion STS30 temperature sensor driver
> + *
> + * Datasheet: https://sensirion.com/media/documents/1DA31AFD/65D613A8/Datasheet_STS3x_DIS.pdf
> + *
> + * Author: Maxwell Doose <m32285159@gmail.com>

Nit, but this is probably unnecessary since you already have
the copyright statement above.

> + */
> +

You're missing array_size.h, as used in probe.

> +#include <linux/bits.h>
> +#include <linux/cleanup.h>
> +#include <linux/crc8.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/errno.h>
> +#include <linux/export.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/types.h>

Move the IIO specific headers below the generic linux headers
and group them separately.

> +#include <linux/i2c.h>
> +#include <linux/kernel.h>

Don't use kernel.h for new entries, you should include what
you actually use instead of relying on this. (there is a tool
that can help with this, it's called `iwyu-tool`).

> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/types.h>
> +#include <linux/unaligned.h>
> +
> +/* Amount of bytes received from the STS30 after a read command */
> +#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
> +
> +/* Soft reset command */

No point in having a comment if your macro is sensibly named.

> +#define STS30_COMMAND_RESET 0x30A2
> +
> +/*
> + * sts30 includes a CRC8 checksum at the end of its i2c responses. The polynomial
> + * is used to generate the CRC8 table and the seed is the starting value.
> + */
> +#define STS30_CRC8_POLYNOMIAL 0x31
> +#define STS30_CRC8_SEED 0xFF
> +
> +DECLARE_CRC8_TABLE(sts30_crc_table);
> +
> +enum sts30_read_delays {
> +	STS30_REPEAT_LOW = 4500,
> +	STS30_REPEAT_MED = 6000,
> +	STS30_REPEAT_HIGH = 15000
> +};
> +
> +/**
> + * struct sts30_data - data structure for STS30 driver
> + *
> + * @client: underlying i2c client data structure
> + * @lock: mutex for serialized communication on the i2c bus
> + * @delay: measurement duration for the current repeatability mode
> + */

I'd remove this comment altogether and just add a comment on why
you need a mutex.

> +struct sts30_data {
> +	struct i2c_client *client;
> +	struct mutex lock;
> +	/*
> +	 * sts30 has three potential repeatability/measurement durations. We need to
> +	 * account for them while reading the i2c bus.
> +	 *
> +	 * See section 2.2 in the datasheet for more info on processing times.
> +	 */
> +	enum sts30_read_delays delay;
> +};
> +
> +static int sts30_verify_crc8(struct sts30_data *data, u8 buf[STS30_MEAS_SIZE])
> +{
> +	int crc;
> +
> +	crc = crc8(sts30_crc_table, buf, 2, STS30_CRC8_SEED);

Please use sizeof() instead of hard coding the buffer length.

> +	if (crc != buf[2]) {
> +		dev_err(&data->client->dev, "Expected CRC8 value of 0x%02x, got 0x%02x\n",
> +			buf[2], crc);
> +		return -EIO;
> +	}
> +
> +	return 0;
> +}
> +
> +static int sts30_read(struct sts30_data *data, u16 command, u16 *val)
> +{
> +	u8 tmp[2];
> +	u8 buf[STS30_MEAS_SIZE];
	
Reverse christmas tree order.

> +	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);

Adding Sashiko's comment:

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?

> +
> +	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);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}

...

> +static int sts30_reset(struct sts30_data *data)
> +{
> +	int ret;
> +
> +	guard(mutex)(&data->lock);
> +
> +	ret = sts30_write(data, STS30_COMMAND_RESET);
> +	if (ret)
> +		return ret;
> +
> +	fsleep(1500);

Add a comment or change this to a macro to explain why 1500
specifically.

> +
> +	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);
> +
> +	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);

Do we need this warning? Returning -EINVAL is sufficient
enough.

> +			return -EINVAL;
> +		}
> +
> +		if (ret)
> +			return ret;
> +
> +		*val = tmp;
> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_OFFSET:
> +		/*
> +		 * We use this constant -16852 as calculated using the formula
> +		 * in the datasheet. See section 4.12 in the data sheet for more
> +		 * info.
> +		 */
> +		*val = -16852;

Okay, this would definitely be better if it were a macro. I'd even
be tempted to add the formula in a comment for safe keeping.
 
...

> +static const struct i2c_device_id sts30_id[] = {
> +	{ "sts30" },
> +	{ "sts31" },
> +	{ "sts35" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, sts30_id);

Move the struct i2c_device_id struct after probe(). Additionally,
use the `.name` named initializer when defining the IDs (this reflects
on Uwe Kleine-Konig's effort in IIO).

> +
> +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;
> +	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);
> +
> +	ret = sts30_reset(data);

You're not checking ret here.

> +
> +	return devm_iio_device_register(&client->dev, indio_dev);
> +}
> +

-- 
Kind regards

CJD