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

[Maxwell Doose]

Hi Joshua,

Sorry about the bugs, this is my first driver submission.

On Sat, Jun 20, 2026 at 2:43 AM Joshua Crofts <joshua.crofts1@xxxxxxxxx> wrote:
>
> 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@xxxxxxxxx> 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@xxxxxxxxx>
>
> Nit, but this is probably unnecessary since you already have
> the copyright statement above.
>

Fair, will remove.

> > + */
> > +
>
> You're missing array_size.h, as used in probe.
>

Nice catch, I didn't see that in my final compilation.

> > +#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.
>

Will do.

> > +#include <linux/i2c.h>
> > +#include <linux/kernel.h>
>
> Don't use kernel.h for new entries, you should include what

Righty oh, will remove. I guess that's why it compiled fine on my end
because it's a catch-all.

> you actually use instead of relying on this. (there is a tool
> that can help with this, it's called `iwyu-tool`).
>

I've heard of iwyu-tool, I just don't know how to configure it :(

> > +#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.
>

Fair point, I'll see about renaming it and removing the comment in v2.

> > +#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.
>

Alright then.

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

I suppose that we can do sizeof(buf) - 1 since we know the last byte
in the buffer has to be the checksum.

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

Sounds good.

> > +     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?
>

Yea, I accidentally mixed up the commands, I saw this on sashiko when
I submitted it last night. It seems like a five minute fix in any case
though.

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

Alright. Can also explain here:
Datasheet dictates that maximum soft reset time is 1.5ms or 1500us. We
fsleep() here to ensure that the device is ready before we continue.

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

Since this is read_raw() the warning might help the user resolve the
error rather than just getting "Invalid argument".

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

At this point I agree, it's probably a good idea to put our constants
at the top of the file as macros and put the formula there :/

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

Right. When I was first writing up the driver I'd ended up using
i2c_match_id() but after removing it I'd forgotten to put it back at
the bottom. Will fix (alongside the named initializers).

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

Also saw that from sashiko, and will fix.

-- 
best regards,
max