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

[Wadim Mueller <wafgo01@gmail.com>]

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

> Sice you are at it, it might make sense to also support SLF3S-1300F.

Done in v2.  scale_den = 30 000 000 (500 (ml/min)^-1, Table 15 of
the LQ_DS_SLF3S-1300F datasheet) and sub_type byte 0x02 (Table 13,
product number 0x07030202).  Re-checked 0600F (0x07030302 -> 0x03)
and 4000B (0x07030501 -> 0x05) while I was at it, so all three
entries now match the published product numbers.

> > +	{ "sensirion,slf3s-0600f", "slf3s-0600f", ... }
> > drivers/iio/flow/slf3x.c
> What does the "X" refer to ? Why not "S" ?

Done in v2.  Renamed everywhere - slf3x.c -> slf3s.c, SLF3X_* ->
SLF3S_*, Kconfig symbol and MAINTAINERS title too.

> > +static const u8 slf3x_cmd_start_water[]	= { 0x36, 0x08 };  
> 
> I looked at LQ_DS_SLF3S-1300F, LQ_DS_SLF3S-0600F, and LQ_DS_SLF3S-4000B.
> They all also support Isopropyl alcohol (IPA) measurements.
> 
> Would it make sense to also provide support for other liquid types
> besides water ? That could be a sysfs attribute and/or a devicetree
> property.

Picked up in v2.  All three variants are factory-calibrated for
H2O and IPA; the scale factor and temperature interpretation are
identical, only the start command differs (0x3608 vs 0x3615).
v2 adds a "sensirion,medium" DT property (enum "water" / "ipa",
default water) that selects the start command at probe time, and
the binding (patch 2/3) documents it.  Kept this as a probe-time
choice instead of a sysfs attribute since switching mid-run needs
a stop anyway per the datasheet.

> > +static int slf3x_verify_crc(const u8 *block)
> > +{
> > +	return crc8(slf3x_crc_table, block, 2, SLF3X_CRC8_INIT) == block[2] ?
> > +		       0 :
> > +		       -EIO;  
> This returns -EIO on error ...
> > +
> > +	for (i = 0; i < SLF3X_PRODUCT_ID_LEN; i += 3) {
> > +		if (slf3x_verify_crc(&buf[i])) {  
> ... which is then ignored here and replaced by -EIO.
> 
> Why not just have it return a boolean ?

Done in v2.  slf3s_crc_valid() returns bool now; the call sites
translate false into -EIO themselves.

> > +	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.

Toned down in v2:

  - Unknown sub-type / unexpected family byte -> dev_dbg() and
    keep going (also covers Jonathan's fallback-compatibles point).
  - CRC mismatches -> dev_err_probe() in probe / dev_err() in
    read_raw, one line per failure instead of per-byte spam.

Thanks,
Wadim