Re: [PATCH v3 2/2] iio: pressure: add Sensirion SDP31 driver

[Jonathan Cameron]

On Sat, 26 Sep 2026 20:41:18 +0500
Muhammad Abu Bakar <m.abubakar365@xxxxxxxxx> wrote:

> Add an IIO driver for the Sensirion SDP31 differential pressure sensor.
> The device is accessed over I2C and reports differential pressure and
> temperature. Each measurement is validated using the sensor's CRC-8
> checksum.
> 
> Tested on an SDP31 connected to a Raspberry Pi 4 I2C bus.
> 
> Signed-off-by: Muhammad Abu Bakar <m.abubakar365@xxxxxxxxx>
Hi. 

Given you are going to probably need to changes stuff in the dt binding
and so do a v4, various comments inline.  Mostly optimization suggestions.

Jonathan

>  st_pressure-y := st_pressure_core.o
> diff --git a/drivers/iio/pressure/sdp31.c b/drivers/iio/pressure/sdp31.c
> new file mode 100644
> index 000000000..fd7cac027
> --- /dev/null
> +++ b/drivers/iio/pressure/sdp31.c

> +static int sdp31_measure(struct sdp31_data *data, struct sdp31_reading *out)
> +{
> +	u8 rx[9];
> +	int ret;
> +
> +	guard(mutex)(&data->lock);
> +
> +	ret = sdp31_send_cmd(data->client, SDP31_CMD_TRIG_DP);
> +	if (ret)
> +		return ret;
> +
> +	msleep(SDP31_MEAS_DELAY_MS);
> +
> +	ret = i2c_master_recv(data->client, rx, sizeof(rx));

Given there is only a one time read of scale and that i2c is a rather
slow bus, can we do a short read when we only want the temperature
and pressure?  The datasheet mentions Nack + stop is
sufficient to stop the read out sequence and that is IIRC the
normal end of an i2c read sequence. So it 'should' be fine to just
read fewer bytes.

> +	if (ret < 0)
> +		return ret;
> +	if (ret != sizeof(rx))
> +		return -EIO;
> +
> +	if (sdp31_check_crc(&rx[0]) ||
> +	    sdp31_check_crc(&rx[3]) ||
> +	    sdp31_check_crc(&rx[6]))
> +		return -EIO;
> +
> +	out->pressure = (s16)get_unaligned_be16(&rx[0]);
> +	out->temp = (s16)get_unaligned_be16(&rx[3]);
> +	out->scale = get_unaligned_be16(&rx[6]);
> +
> +	return 0;
> +}

> +
> +static int sdp31_probe(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	struct iio_dev *indio_dev;
> +	struct sdp31_data *data;
> +	struct sdp31_reading r;
> +	int ret;
> +
> +	ret = devm_regulator_get_enable(dev, "vdd");
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to enable regulator\n");
> +
> +	/* Wait for the sensor to be ready after power-up (datasheet t_PU). */
> +	msleep(SDP31_POWERUP_TIME_MS);
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	data = iio_priv(indio_dev);
> +	data->client = client;
> +
> +	ret = devm_mutex_init(dev, &data->lock);
> +	if (ret)
> +		return ret;
> +
> +	/* The CRC table is shared by all instances; initialise it once. */

I would argue that is obvious, so no comment needed.

> +	DO_ONCE(crc8_populate_msb, sdp31_crc8_table, SDP31_CRC8_POLY);
> +
> +	/* Confirm the sensor is present and learn its scale factor. */

If you end up with with a specific call to get the scale factor this
comment will become excessive (see below).

> +	ret = sdp31_measure(data, &r);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to read from sensor\n");
> +	if (!r.scale)
> +		return dev_err_probe(dev, -EINVAL, "invalid scale factor\n");

See above. This is the only time we actually read the scale - so perhaps
we can save some traffic on every other access. Most likely that would give
you a different command for this scale read back.

> +	data->dp_scale = r.scale;
> +
> +	indio_dev->name = "sdp31";
> +	indio_dev->info = &sdp31_info;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->channels = sdp31_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(sdp31_channels);
> +
> +	return devm_iio_device_register(dev, indio_dev);
> +}