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

[Wadim Mueller <wafgo01@gmail.com>]

On Tue, 26 May 2026 17:35:31 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

> Hopefully I haven't overlapped too much with the review Guenter did.

No overlap problem - replying to all the inline points below.

> > +	  Say yes here to build support for the Sensirion SLF3S family of
> > +	  digital liquid-flow sensors (SLF3S-0600F, SLF3S-4000B, ...).  
> 
> We try to have full listings of supported parts in the help text as it
> gets searched by folk looking for a driver. To minimize the churn consider
> a bulleted list with one per line.

Done in v2 - bulleted list with all currently supported parts
(SLF3S-0600F, SLF3S-1300F, SLF3S-4000B).

> > +#include <linux/unaligned.h>  
> The gap before this header and others in other drivers is an historical oddity ...
> No need to separate it.

Done in v2.

> > +static const struct slf3x_variant slf3x_variants[] = {
> > +	{ .sub_type = 0x03, .name = "slf3s-0600f",
> > +	  .scale_num = 1, .scale_den = 6000000 },
> > +	{ .sub_type = 0x05, .name = "slf3s-4000b",
> > +	  .scale_num = 1, .scale_den = 1666680000 },  
> 
> Maybe format this as one per line.

Done in v2.  One variant entry per line; the list grew by
SLF3S-1300F as Guenter asked.

> > +static int slf3x_write_cmd(struct i2c_client *client, const u8 *cmd)  
> 
> Might be good to use
> const u8 cmd[at_least 2])
> to let the compiler know the constraints.

Done in v2 - declared as `const u8 cmd[static 2]`.

> > +	for (i = 0; i < SLF3X_PRODUCT_ID_LEN; i += 3) {
> > +		if (slf3x_verify_crc(&buf[i])) {
> > +			dev_err(&client->dev,
> > +				"product-info CRC mismatch at byte %d\n", i);
> > +			return -EIO;  
> For all returns in stuff only called from probe() use dev_err_probe().

Done in v2.  All probe-path log+return pairs use dev_err_probe()
now; the remaining dev_err() calls are in the runtime read_raw
path.

> > +	for (i = 0; i < SLF3X_MEAS_LEN; i += 3) {  
> Fine to do
> 	for (unsigned int i = 0; i < ..

Done in v2.

> > +	*flow = (s16)get_unaligned_be16(&buf[0]);  
> 
> Prefer to make this a little more self documenting as
> 
> 	*flow = sign_extend32(get_unaligned_be16(&buf[0]), 15);

Done in v2 for both flow and temp.

> > +	ret = devm_regulator_get_enable_optional(dev, "vdd");  
> 
> No need to turn this on until we are somewhere nearer ready to chat
> to the device.  Normally we do it just before first access.
> I'd move it down a little. It's also not optional unless
> there is some other way of powering the device.
> Just let the regulator framework provide a stub regulator if it's not
> in the firmware description.

Done in v2.  Switched to devm_regulator_get_enable() and moved it
down so it runs just before the first I2C access (the product-info
read).

> > +	ret = slf3x_read_product_info(sf);
> > +	if (ret)
> > +		return dev_err_probe(dev, ret, "product info read failed\n");  
> As in the other review thread, this shouldn't be an error a we want
> those fallback compatibles to work in future.

Done in v2.  Unknown sub-type is dev_dbg() now and falls back to
the variant from the DT compatible (or the of_device_id .data for
the generic fallback).  CRC/family mismatches stay real errors.

> > +	usleep_range(SLF3X_MEAS_DELAY_US, SLF3X_MEAS_DELAY_US + 1000);  
> 
> fsleep() for fuzzy sleeps like this - it applies a standard amount of 'slack' time
> and means we don't need to reason about whether that is a good value or not.

Done in v2.

> > +static const struct i2c_device_id slf3x_id[] = {
> > +	{ "slf3s" },  
> 
> Please use a named initializer here like you do for the of_device_id.
> Uwe is working on making sure all drivers do this and I just took a patch
> that updated most of the IIO ones to do so.

Done in v2 - `{ .name = "slf3s-0600f" }` etc.  The i2c id table
mirrors the of_device_id list, with .driver_data pointing at the
matching slf3s_variant.

Thanks,
Wadim