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

[Jonathan Cameron <jic23@kernel.org>]

On Sat, 30 May 2026 22:54:32 +0200
Wadim Mueller <wafgo01@gmail.com> wrote:

> Add a driver for the Sensirion SLF3S family of digital
> liquid-flow sensors on I2C.  Currently supported variants are
> SLF3S-0600F, SLF3S-1300F and SLF3S-4000B; they share the same
> register map and differ only in flow-scale factor and calibrated
> measurement range.  The variant (and therefore the scale) is
> auto-detected from the product-information register at probe time.
> 
> Each measurement frame returns a 16-bit signed flow value, a
> 16-bit signed temperature reading and a status word, each
> protected by a CRC-8 byte.  The driver exposes the flow rate as
> IIO_VOLUMEFLOW and the temperature as IIO_TEMP via the standard
> IIO read_raw / read_scale interface.
> 
> The active calibration medium can be switched at runtime between
> the factory-calibrated water and isopropyl-alcohol modes via the
> in_volumeflow_medium sysfs attribute; the sensor starts in water
> mode after probe.
> 
> This driver also creates the drivers/iio/flow/ subdirectory and
> the corresponding Kconfig/Makefile glue.
> 
> Signed-off-by: Wadim Mueller <wafgo01@gmail.com>
Hi Wadim

A few things inline.  Biggest one is don't use direct mode claims
to do serialization of things that lie entirely in your driver.
That there is a lock in that call is an implementation detail you
should not be relying on.

Jonathan

> diff --git a/drivers/iio/flow/slf3s.c b/drivers/iio/flow/slf3s.c
> new file mode 100644
> index 000000000..497a56f59
> --- /dev/null
> +++ b/drivers/iio/flow/slf3s.c

> +
> +/*
> + * Read the product-info block and pick the matching variant.  The
> + * sub-type byte returned by the sensor is the source of truth; a
> + * DT-supplied compatible only seeds an initial guess and is overridden
> + * on mismatch (with an informational message so misconfigured device
> + * trees are easy to spot).

Wrap to 80 chars.  Also if following suggestion to 'warn' on mismatch
make sure to update the comment as well.


> + *
> + * Bus / CRC failures are real errors and fail probe.  An unknown
> + * sub-type byte fails probe too: we cannot publish a meaningful scale
> + * without a matching entry in slf3s_variants[].
> + */
> +static int slf3s_detect_variant(struct slf3s_data *sf)
> +{


> +
> +static int slf3s_set_medium(struct iio_dev *indio_dev,
> +			    const struct iio_chan_spec *chan, unsigned int mode)
> +{
> +	struct slf3s_data *sf = iio_priv(indio_dev);
> +	const u8 *start_cmd;
> +	int ret;
> +
> +	if (!iio_device_claim_direct(indio_dev))

It makes no sense to claim direct mode in a driver that doesn't
do any other modes.  I think you are just using this to serialize
commands, all of which occur in direct mode.  Don't do that, use
your own local lock. 

> +		return -EBUSY;
> +
> +	ret = slf3s_send_cmd(sf->client, slf3s_cmd_stop_meas);
> +	if (ret)
> +		goto out;
> +
> +	start_cmd = (mode == SLF3S_MEDIUM_IPA) ? slf3s_cmd_start_ipa
> +					       : slf3s_cmd_start_water;
> +
> +	ret = slf3s_send_cmd(sf->client, start_cmd);
> +	if (ret)
> +		goto out;
> +
> +	fsleep(SLF3S_MEAS_START_DELAY_US);
> +	sf->medium = mode;
> +out:
> +	iio_device_release_direct(indio_dev);
> +
> +	return ret;
> +}


> +static const struct i2c_device_id slf3s_id[] = {
> +	{ .name = "slf3s-0600f",
> +	  .driver_data = (kernel_ulong_t)&slf3s_variants[0] },

	{
		.name = "slf3s-0600f",
		.driver_data = (kernel_ulong_t)&slf3s_variants[0],
	},

> +	{ .name = "slf3s-1300f",
> +	  .driver_data = (kernel_ulong_t)&slf3s_variants[1] },
> +	{ .name = "slf3s-4000b",
> +	  .driver_data = (kernel_ulong_t)&slf3s_variants[2] },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, slf3s_id);