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

Jonathan Cameron
2025-05-05T17:47:13Z

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;
> + }
> +}