Re: [PATCH v2 2/2] iio: chemical: Add driver for SEN0322

[Jonathan Cameron <jic23@kernel.org>]

On Mon, 05 May 2025 09:52:59 +0200
Tóth János via B4 Relay <devnull+gomba007.gmail.com@kernel.org> wrote:

> From: Tóth János <gomba007@gmail.com>
> 
> Add support for the DFRobot SEN0322 oxygen sensor.
> 
> Datasheet:
> 	https://wiki.dfrobot.com/Gravity_I2C_Oxygen_Sensor_SKU_SEN0322

Checkpatch is lagging behind the times, but it is fine to use this
as a formal tag in the tag block..
> 
> To instantiate (assuming device is connected to I2C-2):
> 	echo 'sen0322 0x73' > /sys/class/i2c-dev/i2c-2/device/new_device
> 
> To get the oxygen concentration (assuming device is iio:device0) multiply
> the values read from:
> 	/sys/bus/iio/devices/iio:device0/in_concentration_raw
> 	/sys/bus/iio/devices/iio:device0/in_concentration_scale
> 
Datasheet: https://wiki.dfrobot.com/Gravity_I2C_Oxygen_Sensor_SKU_SEN0322

> Signed-off-by: Tóth János <gomba007@gmail.com>
A few other little things inline.

Nice little driver :)

Jonathan

> diff --git a/drivers/iio/chemical/sen0322.c b/drivers/iio/chemical/sen0322.c
> new file mode 100644
> index 000000000000..c2dfb0ff7f40
> --- /dev/null
> +++ b/drivers/iio/chemical/sen0322.c
> @@ -0,0 +1,167 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Driver for the DFRobot SEN0322 oxygen sensor.
> + *
> + * Datasheet:
> + *	https://wiki.dfrobot.com/Gravity_I2C_Oxygen_Sensor_SKU_SEN0322
> + *
> + * Possible I2C slave addresses:
> + *	0x70
> + *	0x71
> + *	0x72
> + *	0x73
> + *
> + * Copyright (C) 2025 Tóth János <gomba007@gmail.com>
> + */
> +
> +#include <linux/i2c.h>
> +#include <linux/regmap.h>
> +
> +#include <linux/iio/iio.h>
> +
> +#define SEN0322_REG_DATA	0x03
> +#define SEN0322_REG_COEFF	0x0A
> +
> +struct sen0322 {
> +	struct regmap	*regmap;
> +};
> +
> +static int sen0322_read_scale(struct sen0322 *sen0322, int *num, int *den)
> +{
> +	u32 val;
> +	int ret;
> +
> +	ret = regmap_read(sen0322->regmap, SEN0322_REG_COEFF, &val);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (val) {
> +		*num = val;
> +		*den = 100000;
> +	} else {
> +		*num = 209;
> +		*den = 120000;

This is odd enough, that perhaps we could add a comment on why, or at least
a cross reference to where these numbers come from?
What is the special meaning of 0?

> +	}
> +
> +	dev_dbg(regmap_get_device(sen0322->regmap), "scale: %d/%d\n",
> +		*num, *den);
> +
> +	return 0;
> +}
> +
> +static int sen0322_read_data(struct sen0322 *sen0322)
> +{
> +	u8 data[4] = { 0 };

If you are only read 3 bytes, why is this 4 long?

> +	int ret;
> +
> +	ret = regmap_bulk_read(sen0322->regmap, SEN0322_REG_DATA, data, 3);

Having shortened above, use sizeof(data) for that 3 to avoid
any potential future mismatch in sizes.

> +	if (ret < 0)
> +		return ret;
> +
> +	ret = data[0] * 100 + data[1] * 10 + data[2];
> +
> +	dev_dbg(regmap_get_device(sen0322->regmap), "data: %d\n", ret);

Given you more or less directly provide this to userspace now I'd drop
the dev_dbg() as not adding any value for debugging.

> +
> +	return ret;
> +}
> +
> +static int sen0322_read_raw(struct iio_dev *iio_dev,
> +			    const struct iio_chan_spec *chan,
> +			    int *val, int *val2, long mask)
> +{
> +	struct sen0322 *sen0322 = iio_priv(iio_dev);
> +	int ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		switch (chan->type) {
> +		case IIO_CONCENTRATION:
As the sensor only does concentration, you could either drop this
check on basis we can't get here without it or if you want 
a strong sanity check do it outside the switch statement as
	if (chan->type != IIO_CONCENTRATION)
		return -EINVAL;


> +			ret = sen0322_read_data(sen0322);
> +			if (ret < 0)
> +				return ret;
> +
> +			*val = ret;
> +			return IIO_VAL_INT;
> +
> +		default:
> +			return -EINVAL;
> +		}
> +
> +	case IIO_CHAN_INFO_SCALE:
> +		switch (chan->type) {
> +		case IIO_CONCENTRATION:
> +			ret = sen0322_read_scale(sen0322, val, val2);
> +			if (ret < 0)
> +				return ret;
> +
> +			return IIO_VAL_FRACTIONAL;
> +
> +		default:
> +			return -EINVAL;
> +		}
> +
> +	default:
> +		return -EINVAL;
> +	}
> +}