Re: [PATCH v2 3/3] iio: flow: add Sensirion SLF3S liquid flow sensor driver

[Jonathan Cameron]

On Wed, 27 May 2026 20:42:54 +0200
Wadim Mueller <wafgo01@xxxxxxxxx> wrote:

> Add a driver for the Sensirion SLF3S family of digital liquid-flow
> sensors (SLF3S-0600F, SLF3S-1300F and SLF3S-4000B).  The sensors
> communicate over I2C and return a 16-bit signed flow value, a 16-bit
> signed temperature reading and a status word - each protected by a
> CRC-8 byte - in every measurement frame.
> 
> The driver exposes:
> 
>   in_volumeflow_raw     - signed raw counts
>   in_volumeflow_scale   - litres per second per LSB (per variant)
>   in_temp_raw           - signed raw counts
>   in_temp_scale         - millidegrees Celsius per LSB
> 
> The variant is auto-detected from the product-info word read at
> probe time.  All three SLF3S devices are factory-calibrated for both
> water and isopropyl alcohol; the calibration medium is selected via
> the sensirion,medium device property and defaults to water.
> 
> Scale factors are taken from the respective datasheets (Table 16
> for SLF3S-0600F, Table 15 for SLF3S-1300F and SLF3S-4000B):
> 
>   SLF3S-0600F:  10 (ul/min)^-1  -> 1/600 000 000 (l/s)/LSB
>   SLF3S-1300F: 500 (ml/min)^-1  -> 1/30 000 000  (l/s)/LSB
>   SLF3S-4000B:  32 (ml/min)^-1  -> 1/1 920 000   (l/s)/LSB
> 
> Signed-off-by: Wadim Mueller <wafgo01@xxxxxxxxx>
> Cc: Guenter Roeck <linux@xxxxxxxxxxxx>
> Cc: Jean Delvare <jdelvare@xxxxxxxx>
> Cc: Andreas Klinger <ak@xxxxxxxxxxxxx>
> Cc: Lars-Peter Clausen <lars@xxxxxxxxxx>
> Cc: linux-hwmon@xxxxxxxxxxxxxxx
> Cc: Maxwell Doose <m32285159@xxxxxxxxx>

A few fairly minor things inline. Also check out sashiko's review
https://sashiko.dev/#/patchset/20260527184257.141635-1-wafgo01%40gmail.com
(Note the DMA buffers stuff is wrong for I2C so ignore that)

> diff --git a/drivers/iio/flow/slf3s.c b/drivers/iio/flow/slf3s.c
> new file mode 100644
> index 000000000..f971a2dc2
> --- /dev/null
> +++ b/drivers/iio/flow/slf3s.c
> @@ -0,0 +1,345 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Sensirion SLF3S liquid flow sensor driver.
> + *
> + * Supports the SLF3S-0600F, SLF3S-1300F and SLF3S-4000B liquid-flow
> + * sensors over I2C.  Each measurement frame returns a 16-bit signed
> + * flow value, a 16-bit signed temperature value and a status word,
> + * each protected by a CRC-8 byte.
> + *
> + * Datasheet: https://sensirion.com/products/catalog/SLF3S-0600F/
> + *
> + * Copyright (C) 2026 CMBlu Energy GmbH
> + * Author: Wadim Mueller <wafgo01@xxxxxxxxx>
> + */
> +
> +#include <linux/bitops.h>
> +#include <linux/crc8.h>
> +#include <linux/delay.h>

Check these.  I'd expect dev_printk.h for instance.
Follow IWYU (approximately) for includes.

> +#include <linux/i2c.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/property.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/unaligned.h>
> +
> +#include <linux/iio/iio.h>
> +
> +#define SLF3S_CRC8_POLY			0x31
> +#define SLF3S_CRC8_INIT			0xff
> +
> +#define SLF3S_PRODUCT_ID_LEN		18
> +#define SLF3S_PRODUCT_FAMILY_BYTE	1
> +#define SLF3S_PRODUCT_SUBTYPE_BYTE	3
> +#define SLF3S_PRODUCT_FAMILY_ID		0x03
> +
> +#define SLF3S_MEAS_LEN			9

I don't think this constant being a define is that useful. I'd push
the value down to where you set how big the buffer is then use ARRAY_SIZE()
for the loop

> +#define SLF3S_MEAS_START_DELAY_US	12000
> +
> +/*
> + * Temperature LSB equals 1/200 degC.  IIO_TEMP uses milli-degrees,
> + * therefore the scale exposed to userspace is 1000 / 200 = 5.
> + */
> +#define SLF3S_TEMP_SCALE_MILLIC		5

I'd put this value inline with the comment. It's only used in one
place and that is where people will look for information on scale.


> +
> +static const struct slf3s_variant slf3s_variants[] = {
> +	[0] = {
> +		.sub_type	= 0x03,
> +		.name		= "slf3s-0600f",
> +		.scale_num	= 1,
> +		.scale_den	= 600000000,

Probably more readable with 600 * MICRO 

> +	},
> +	[1] = {
> +		.sub_type	= 0x02,
> +		.name		= "slf3s-1300f",
> +		.scale_num	= 1,
> +		.scale_den	= 30000000,

30 * MICRO

> +	},
> +	[2] = {
> +		.sub_type	= 0x05,
> +		.name		= "slf3s-4000b",
> +		.scale_num	= 1,
> +		.scale_den	= 1920000,

1920 * MILLI or leave this one as is. 4 zeros are easy to count ;)

> +	},
> +};
> +
> +/**
> + * struct slf3s_data - per-device state
> + * @client:	I2C client this instance is bound to
> + * @variant:	pointer into @slf3s_variants for the detected device
> + * @crc_table:	pre-computed CRC-8 lookup table for SLF3S_CRC8_POLY
> + */
> +struct slf3s_data {
> +	struct i2c_client *client;
> +	const struct slf3s_variant *variant;
> +	u8 crc_table[CRC8_TABLE_SIZE];
> +};
> +
> +static bool slf3s_crc_valid(const struct slf3s_data *sf, const u8 *block)

I'd be consistent and have const u8 block[at_least 3]
the make it clear how big that is expected to be.

> +{
> +	return crc8(sf->crc_table, block, 2, SLF3S_CRC8_INIT) == block[2];
> +}
> +
> +static int slf3s_send_cmd(struct i2c_client *client, const u8 cmd[static 2])

Use at_least, not static for kernel code as it makes it much more obvious what
is going on.

Note the sashiko report on this memory potentially being used for DMA is wrong.
The i2c subsystem only does that if you opt in.

> +{
> +	int ret = i2c_master_send(client, cmd, 2);
> +
> +	if (ret == 2)
> +		return 0;
> +	return ret < 0 ? ret : -EIO;
> +}
> +
> +/*
> + * Read the product-info block and update @sf->variant.  The kernel

wrap at 80 chars. This is a bit short in places.

> + * trusts the DT compatible (or i2c id_table .data) above all else; the
> + * sub-type byte is a sanity hint.  This means:
> + *
> + *   - bus / CRC failures are real errors and must fail probe;
> + *   - if the caller already picked a variant (specific compatible), the
> + *     PID is logged for diagnostics but mismatches do not fail probe;
> + *   - if the caller has no variant (generic "sensirion,slf3s" fallback),
> + *     the sub-type byte is used to pick one; unknown sub-type fails.
This is interesting approach, but lets discuss it in the dt-binding  thread.

> + */
> +static int slf3s_detect_variant(struct slf3s_data *sf)
> +{
> +	struct i2c_client *client = sf->client;
> +	u8 buf[SLF3S_PRODUCT_ID_LEN];
> +	int ret;
> +
> +	ret = slf3s_send_cmd(client, slf3s_cmd_prep_pid);
> +	if (ret)
> +		return ret;
> +
> +	ret = slf3s_send_cmd(client, slf3s_cmd_read_pid);
> +	if (ret)
> +		return ret;
> +
> +	ret = i2c_master_recv(client, buf, sizeof(buf));
> +	if (ret < 0)
> +		return ret;
> +	if (ret != sizeof(buf))
> +		return -EIO;
> +
> +	for (unsigned int i = 0; i < SLF3S_PRODUCT_ID_LEN; i += 3) {
> +		if (!slf3s_crc_valid(sf, &buf[i]))
> +			return -EIO;
> +	}
> +
> +	if (buf[SLF3S_PRODUCT_FAMILY_BYTE] != SLF3S_PRODUCT_FAMILY_ID)
> +		dev_dbg(&client->dev,
> +			"unexpected family byte 0x%02x (expected 0x%02x)\n",
> +			buf[SLF3S_PRODUCT_FAMILY_BYTE],
> +			SLF3S_PRODUCT_FAMILY_ID);
> +
> +	for (unsigned int i = 0; i < ARRAY_SIZE(slf3s_variants); i++) {
> +		if (buf[SLF3S_PRODUCT_SUBTYPE_BYTE] !=
> +		    slf3s_variants[i].sub_type)
I'd go slightly long and have that one line.
		if (buf[SLF3S_PRODUCT_SUBTYPE_BYTE] != slf3s_variants[i].sub_type)
We are a bit flexible on the 80 chars where it helps readabilty.


> +			continue;
> +
> +		if (sf->variant && sf->variant != &slf3s_variants[i])
> +			dev_dbg(&client->dev,
> +				"DT compatible says %s but sub-type 0x%02x suggests %s\n",
> +				sf->variant->name,
> +				buf[SLF3S_PRODUCT_SUBTYPE_BYTE],
> +				slf3s_variants[i].name);
> +		else if (!sf->variant)

If this happens I'd go with the detection over the dt provided. And dev_info
for the mismatch as we want people to know wrong sensor is described.

> +			sf->variant = &slf3s_variants[i];
> +		return 0;
> +	}
> +
> +	if (sf->variant) {
> +		dev_dbg(&client->dev,
> +			"unknown SLF3S sub-type 0x%02x, trusting DT compatible %s\n",
> +			buf[SLF3S_PRODUCT_SUBTYPE_BYTE], sf->variant->name);
> +		return 0;
> +	}
> +
> +	dev_dbg(&client->dev, "unknown SLF3S sub-type 0x%02x\n",
> +		buf[SLF3S_PRODUCT_SUBTYPE_BYTE]);
As below. I'd have a blank line here

> +	return -ENODEV;
> +}
> +
> +static int slf3s_read_sample(struct slf3s_data *sf, int *flow, int *temp)
> +{
> +	u8 buf[SLF3S_MEAS_LEN];
> +	int ret;
> +
> +	ret = i2c_master_recv(sf->client, buf, sizeof(buf));
> +	if (ret < 0)
> +		return ret;
> +	if (ret != sizeof(buf))
> +		return -EIO;
> +
> +	for (unsigned int i = 0; i < SLF3S_MEAS_LEN; i += 3) {
> +		if (!slf3s_crc_valid(sf, &buf[i]))
> +			return -EIO;
> +	}
> +
> +	*flow = sign_extend32(get_unaligned_be16(&buf[0]), 15);
> +	*temp = sign_extend32(get_unaligned_be16(&buf[3]), 15);

Trivial but good to have a blank line before simple return statements like
this. Just makes the code a tiny bit more readable.

> +	return 0;
> +}

> +
> +static int slf3s_read_raw(struct iio_dev *indio_dev,
> +			  struct iio_chan_spec const *chan, int *val,
> +			  int *val2, long mask)
> +{
> +	struct slf3s_data *sf = iio_priv(indio_dev);
> +	int flow, temp, ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		if (!iio_device_claim_direct(indio_dev))
> +			return -EBUSY;
> +
> +		ret = slf3s_read_sample(sf, &flow, &temp);
> +		iio_device_release_direct(indio_dev);
> +		if (ret)
> +			return ret;
> +
> +		*val = (chan->type == IIO_VOLUMEFLOW) ? flow : temp;
> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_SCALE:
> +		if (chan->type == IIO_VOLUMEFLOW) {
> +			*val = sf->variant->scale_num;
> +			*val2 = sf->variant->scale_den;

Sashiko (probably correctly) identifies that the formatting that the IIO core
does for an IIO_VAL_FRACTIONAL only goes to 9 decimal places.  That means
in some cases the scale is truncated to 1 digit losing much of its usefulness.
https://sashiko.dev/#/patchset/20260527184257.141635-1-wafgo01%40gmail.com

As such we need to increase the number of digits or maybe switch to using
https://lore.kernel.org/all/20260524-adf41513-iio-driver-v14-6-06824d9c15f4@xxxxxxxxxx/

Which defines IIO_VAL_DECIMAL64_PICO

+CC Rodrigo


> +			return IIO_VAL_FRACTIONAL;
> +		}
> +		*val = SLF3S_TEMP_SCALE_MILLIC;
> +		return IIO_VAL_INT;
> +	default:
> +		return -EINVAL;
> +	}
> +}

> +
> +static int slf3s_probe(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	struct iio_dev *indio_dev;
> +	struct slf3s_data *sf;
> +	const u8 *start_cmd = slf3s_cmd_start_water;
> +	const char *medium;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*sf));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	sf = iio_priv(indio_dev);
> +	sf->client = client;
> +	sf->variant = i2c_get_match_data(client);
> +	crc8_populate_msb(sf->crc_table, SLF3S_CRC8_POLY);
> +
> +	ret = devm_regulator_get_enable(dev, "vdd");
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to enable vdd supply\n");

More than likely we need some level of sleep here for the device to wake up.
Is there anything in the datasheet?

> +
> +	ret = slf3s_detect_variant(sf);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "product info read failed\n");

> +
> +static const struct of_device_id slf3s_of_match[] = {
> +	{ .compatible = "sensirion,slf3s-0600f", .data = &slf3s_variants[0] },
> +	{ .compatible = "sensirion,slf3s-1300f", .data = &slf3s_variants[1] },
> +	{ .compatible = "sensirion,slf3s-4000b", .data = &slf3s_variants[2] },
> +	{ .compatible = "sensirion,slf3s" },

This came up in the dt review, so I'll assume you'll address it there. How useful
is the generic compatible?

> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, slf3s_of_match);
> +
> +static struct i2c_driver slf3s_driver = {
> +	.driver = {
> +		.name		= "slf3s",
> +		.of_match_table	= slf3s_of_match,
> +	},
> +	.probe		= slf3s_probe,
> +	.id_table	= slf3s_id,
> +};
> +module_i2c_driver(slf3s_driver);
> +
> +MODULE_AUTHOR("Wadim Mueller <wafgo01@xxxxxxxxx>");
> +MODULE_DESCRIPTION("Sensirion SLF3S liquid flow sensor driver");
> +MODULE_LICENSE("GPL");