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

[Marcelo Schmitt]

Hello Wadim,

Some additional comments. Feel free to disregard any of them if they conflict
with previous reviews.

On 05/30, Wadim Mueller 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@xxxxxxxxx>
> ---
...
> +config SENSIRION_SLF3S
> +	tristate "Sensirion SLF3S liquid flow sensor"
> +	depends on I2C
> +	select CRC8
> +	help
> +	  Say yes here to build support for the Sensirion SLF3S family
> +	  of digital liquid-flow sensors:
> +
> +	    - SLF3S-0600F
> +	    - SLF3S-1300F
> +	    - SLF3S-4000B
> +
> +	  The driver reports the volumetric flow rate and the embedded
> +	  temperature reading via the standard IIO interface.
This level of detail is probably not needed in the Kconfig entry. Though, it's
probably okay to have it anyways.

> +
> +	  To compile this driver as a module, choose M here: the module
> +	  will be called slf3s.
> +
> +endmenu
> diff --git a/drivers/iio/flow/Makefile b/drivers/iio/flow/Makefile
> new file mode 100644
> index 000000000..3cf4ab95c
> --- /dev/null
> +++ b/drivers/iio/flow/Makefile
> @@ -0,0 +1,7 @@
> +# SPDX-License-Identifier: GPL-2.0
> +#
> +# Makefile for industrial I/O flow sensor drivers
> +#
> +
> +# When adding new entries keep the list in alphabetical order
> +obj-$(CONFIG_SENSIRION_SLF3S) += slf3s.o
> 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
> @@ -0,0 +1,406 @@
...
> +static const struct slf3s_variant slf3s_variants[] = {
> +	[0] = {
> +		.sub_type	= 0x03,
> +		.name		= "slf3s-0600f",
> +		.scale_num	= 1,
> +		.scale_den	= 600 * MICRO,
> +	},
> +	[1] = {
> +		.sub_type	= 0x02,
> +		.name		= "slf3s-1300f",
> +		.scale_num	= 1,
> +		.scale_den	= 30 * MICRO,
This one seems to almost zero out with 9 decimal digit precision.
And it doesn't get better if _raw * _scale is to result in m³/s.
I haven't followed it closely but, maybe this driver could use some of the new
IIO_VAL types from ADF41513 series [1].

Ignore my comment if l/s was accepted.

[1]: https://lore.kernel.org/linux-iio/20260531-adf41513-iio-driver-v15-6-da09adf1c0dd@xxxxxxxxxx/

> +	},
...
> +
> +static int slf3s_read_sample(struct slf3s_data *sf, int *flow, int *temp)
> +{
> +	u8 buf[9];
Can buf be 6 bytes long? 2 bytes of flow data + 1 CRC + 2 bytes of temp + 1 CRC.

> +	int ret;
> +
> +	ret = i2c_master_recv(sf->client, buf, ARRAY_SIZE(buf));
> +	if (ret < 0)
> +		return ret;
> +	if (ret != ARRAY_SIZE(buf))
> +		return -EIO;
> +
> +	for (unsigned int i = 0; i < ARRAY_SIZE(buf); 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);
> +
> +	return 0;
> +}
> +
...
> +static const struct iio_chan_spec_ext_info slf3s_ext_info[] = {
> +	IIO_ENUM("medium", IIO_SHARED_BY_TYPE, &slf3s_medium_enum),
> +	IIO_ENUM_AVAILABLE("medium", IIO_SHARED_BY_TYPE, &slf3s_medium_enum),
We should probably document the new 'medium' ABI.
For consolidated ABI, we document it in Documentation/ABI/testing/sysfs-bus-iio.
Since this is new stuf, maybe start with a separate ABI doc
(e.g. Documentation/ABI/testing/sysfs-bus-iio-slf3s) ?

> +	{ }
> +};

With best regards,
Marcelo