Re: [RFC PATCH v1 3/4] iio: flow: add Sensirion SLF3x liquid flow sensor driver

[Jonathan Cameron <jic23@kernel.org>]

On Sun, 24 May 2026 14:40:51 -0700
Guenter Roeck <linux@roeck-us.net> wrote:

> On 5/24/26 13:49, Wadim Mueller wrote:
> > From: Wadim Mueller <wadim.mueller@cmblu.de>
> > 
> > Add an IIO driver for the Sensirion SLF3S family of digital
> > liquid-flow sensors.  The supported sub-types (SLF3S-0600F,
> > SLF3S-4000B) share the same register map and command set and are
> > distinguished only by the flow scale; the variant is detected at
> > probe time from the product-information register.
> >   
> Sice you are at it, it might make sense to also support SLF3S-1300F.
> 
> 
> > The driver exposes two IIO channels:
> >    - in_volumeflow_raw / in_volumeflow_scale (litres per second)
> >    - in_temp_raw       / in_temp_scale       (milli-degC)
> > 
> > Continuous measurement mode is started in probe and stopped via
> > devm-action; read_raw() fetches the most recent sample on demand.
> > 
> > Signed-off-by: Wadim Mueller <wadim.mueller@cmblu.de>
> > ---
> >   drivers/iio/Kconfig       |   1 +
> >   drivers/iio/Makefile      |   1 +
> >   drivers/iio/flow/Kconfig  |  22 ++++
> >   drivers/iio/flow/Makefile |   7 +
> >   drivers/iio/flow/slf3x.c  | 264 ++++++++++++++++++++++++++++++++++++++  
> 
> What does the "X" refer to ? Why not "S" ?

Absolutely - no wild cards in file names or indeed in variables etc.
Just pick a part to name after.  Wild cards go wrong far too often!

A couple more follow ups below to some of what Guenter noted

> > +static const u8 slf3x_cmd_stop[]	= { 0x3f, 0xf9 };
> > +
> > +DECLARE_CRC8_TABLE(slf3x_crc_table);
> > +
> > +struct slf3x_variant {
> > +	u8 sub_type;
> > +	const char *name;
> > +	/*
> > +	 * Flow scale exposed via IIO_CHAN_INFO_SCALE in litres per second
> > +	 * per LSB, encoded as IIO_VAL_FRACTIONAL (num / den).  The encoding
> > +	 * comes from the Sensirion datasheet's "scale factor" (ticks per
> > +	 * ml/min) combined with the 1 ml/min = 1/60000 l/s conversion.
> > +	 */  
> 
> Not my call to make, but at least the Sensirion sensors all talk about
> flow rate per minute, not per second. A Google search suggests that
> flow rate is normally measured per minute or even per hour, and that
> per-second measurements are typically only used for large-scale engineering,
> rivers, dams, and rapid industrial chemical dosing. Taking SLF3S-0600F
> as example, it measures up to ±2000 µl/min (!). Even Sensirion's gas
> sensors use per-minute flow rates.
> 
> Any special reason to use a per-second rate ?

As I just replied to the cover letter - things in IIO that have relationship
to time (delays, integration times etc) are in seconds. 
Would be very confusing to mix and match. In general for new channel types
we stick to SI units without multipliers.

> 
> > +	int scale_num;
> > +	int scale_den;
> > +};

> > +			dev_err(&client->dev,
> > +				"product-info CRC mismatch at byte %d\n", i);
> > +			return -EIO;
> > +		}
> > +	}
> > +
> > +	if (buf[SLF3X_FAMILY_BYTE] != SLF3X_FAMILY_ID) {
> > +		dev_err(&client->dev,
> > +			"unexpected device family 0x%02x\n",
> > +			buf[SLF3X_FAMILY_BYTE]);
> > +		return -ENODEV;

Similar to below. This can't be an error or we break fallback
compatibles in DT. There are historic drivers that do this that
we haven't fixed up yet - but no new ones please!

> > +	}
> > +
> > +	for (i = 0; i < ARRAY_SIZE(slf3x_variants); i++) {
> > +		if (buf[SLF3X_SUBTYPE_BYTE] == slf3x_variants[i].sub_type) {
> > +			sf->variant = &slf3x_variants[i];
> > +			return 0;
> > +		}
> > +	}
> > +
> > +	dev_err(&client->dev, "unsupported SLF3x sub-type 0x%02x\n",
> > +		buf[SLF3X_SUBTYPE_BYTE]);  
> 
> Not my call to make, but the driver is way too noisy for my liking.
> 
Given the need to support fallback compatibles from device tree we don't
fail at all on mismatches as that prevents use supporting a new fully
backwards compatible (other than IDs) part in older kernels.

Whether a dev_info() or dev_dbg() is appropriate for such a case is
less obvious but definitely dev_err().

> > +	return -ENODEV;
and it's not an error to read an unexpected ID.
> > +}