[v1] Add support for the DFRobot SEN0322 oxygen sensor

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

Posted by Tóth János via B4 Relay 9 months, 2 weeks ago

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

To instantiate (assuming device is connected to I2C-2):
	echo 'sen0322 0x73' > /sys/class/i2c-dev/i2c-2/device/new_device

To read the oxygen concentration (assuming device is iio:device0):
	cat /sys/bus/iio/devices/iio:device0/in_concentration_input

Signed-off-by: Tóth János <gomba007@gmail.com>
---
 MAINTAINERS                    |   6 ++
 drivers/iio/chemical/Kconfig   |  10 ++
 drivers/iio/chemical/Makefile  |   1 +
 drivers/iio/chemical/sen0322.c | 238 +++++++++++++++++++++++++++++++++++++++++
 4 files changed, 255 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 3cbf9ac0d83f..6fda7a2f1248 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -6852,6 +6852,12 @@ L:	linux-rtc@vger.kernel.org
 S:	Maintained
 F:	drivers/rtc/rtc-sd2405al.c
 
+DFROBOT SEN0322 DRIVER
+M:	Tóth János <gomba007@gmail.com>
+L:	linux-iio@vger.kernel.org
+S:	Maintained
+F:	drivers/iio/chemical/sen0322.c
+
 DH ELECTRONICS DHSOM SOM AND BOARD SUPPORT
 M:	Christoph Niedermaier <cniedermaier@dh-electronics.com>
 M:	Marek Vasut <marex@denx.de>
diff --git a/drivers/iio/chemical/Kconfig b/drivers/iio/chemical/Kconfig
index 330fe0af946f..60a81863d123 100644
--- a/drivers/iio/chemical/Kconfig
+++ b/drivers/iio/chemical/Kconfig
@@ -166,6 +166,16 @@ config SCD4X
 	  To compile this driver as a module, choose M here: the module will
 	  be called scd4x.
 
+config SEN0322
+	tristate "SEN0322 oxygen sensor"
+	depends on I2C
+	select REGMAP_I2C
+	help
+	  Say Y here to build support for the DFRobot SEN0322 oxygen sensor.
+
+	  To compile this driver as a module, choose M here: the module will
+	  be called sen0322.
+
 config SENSIRION_SGP30
 	tristate "Sensirion SGPxx gas sensors"
 	depends on I2C
diff --git a/drivers/iio/chemical/Makefile b/drivers/iio/chemical/Makefile
index 4866db06bdc9..deeff0e4e6f7 100644
--- a/drivers/iio/chemical/Makefile
+++ b/drivers/iio/chemical/Makefile
@@ -20,6 +20,7 @@ obj-$(CONFIG_SCD30_CORE) += scd30_core.o
 obj-$(CONFIG_SCD30_I2C) += scd30_i2c.o
 obj-$(CONFIG_SCD30_SERIAL) += scd30_serial.o
 obj-$(CONFIG_SCD4X) += scd4x.o
+obj-$(CONFIG_SEN0322)	+= sen0322.o
 obj-$(CONFIG_SENSEAIR_SUNRISE_CO2) += sunrise_co2.o
 obj-$(CONFIG_SENSIRION_SGP30)	+= sgp30.o
 obj-$(CONFIG_SENSIRION_SGP40)	+= sgp40.o
diff --git a/drivers/iio/chemical/sen0322.c b/drivers/iio/chemical/sen0322.c
new file mode 100644
index 000000000000..5f1f4528401e
--- /dev/null
+++ b/drivers/iio/chemical/sen0322.c
@@ -0,0 +1,238 @@
+// 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/math64.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+
+#include <linux/iio/iio.h>
+
+#define DRIVER_NAME "sen0322"
+
+#define SEN0322_REG_DATA	0x03
+#define SEN0322_REG_COEFF	0x0A
+
+#define FIXED_FRAC_BITS		18
+#define FIXED_INT(x)		((fixed_t)((x) << FIXED_FRAC_BITS))
+
+typedef u32 fixed_t;
+
+struct sen0322 {
+	struct i2c_client	*client;
+	struct regmap		*regmap;
+	fixed_t			coeff;
+};
+
+static fixed_t fixed_mul(fixed_t a, fixed_t b)
+{
+	u64 tmp;
+
+	tmp = (u64)a * (u64)b;
+	tmp = (tmp >> FIXED_FRAC_BITS) + ((tmp >> FIXED_FRAC_BITS) & 1);
+
+	if (tmp > U32_MAX)
+		return (fixed_t)U32_MAX;
+	else
+		return (fixed_t)tmp;
+}
+
+static fixed_t fixed_div(fixed_t a, fixed_t b)
+{
+	u64 tmp;
+
+	tmp = (uint64_t)a << FIXED_FRAC_BITS;
+	tmp += (b >> 1);
+
+	return (fixed_t)(div_u64(tmp, b));
+}
+
+static int sen0322_read_coeff(struct sen0322 *sen0322)
+{
+	u32 val;
+	int ret;
+
+	ret = regmap_read(sen0322->regmap, SEN0322_REG_COEFF, &val);
+	if (ret < 0)
+		return ret;
+
+	if (val)
+		sen0322->coeff = fixed_div(FIXED_INT(val), FIXED_INT(1000));
+	else
+		sen0322->coeff = fixed_div(FIXED_INT(209), FIXED_INT(1200));
+
+	dev_dbg(&sen0322->client->dev, "coeff: %08X\n", sen0322->coeff);
+
+	return 0;
+}
+
+static int sen0322_read_data(struct sen0322 *sen0322)
+{
+	u8 data[4] = { 0 };
+	int ret;
+
+	ret = regmap_bulk_read(sen0322->regmap, SEN0322_REG_DATA, data, 3);
+	if (ret < 0)
+		return ret;
+
+	ret = data[0] * 100 +  data[1] * 10 + data[2];
+
+	dev_dbg(&sen0322->client->dev, "raw data: %d\n", ret);
+
+	return ret;
+}
+
+static int sen0322_read_prep_data(struct sen0322 *sen0322)
+{
+	fixed_t val;
+	int ret;
+
+	if (!sen0322->coeff) {
+		ret = sen0322_read_coeff(sen0322);
+		if (ret < 0)
+			return ret;
+	}
+
+	ret = sen0322_read_data(sen0322);
+	if (ret < 0)
+		return ret;
+
+	val = fixed_mul(sen0322->coeff, FIXED_INT(ret));
+
+	dev_dbg(&sen0322->client->dev, "prep data: %08X\n", val);
+
+	return val >> FIXED_FRAC_BITS;
+}
+
+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:
+			ret = sen0322_read_data(sen0322);
+			if (ret < 0)
+				return ret;
+
+			*val = ret;
+			return IIO_VAL_INT;
+
+		default:
+			return -EINVAL;
+		}
+
+	case IIO_CHAN_INFO_PROCESSED:
+		switch (chan->type) {
+		case IIO_CONCENTRATION:
+			ret = sen0322_read_prep_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:
+			*val = 1;
+			*val2 = 100;
+			return IIO_VAL_FRACTIONAL;
+
+		default:
+			return -EINVAL;
+		}
+
+	default:
+		return -EINVAL;
+	}
+}
+
+static const struct iio_info sen0322_info = {
+	.read_raw = sen0322_read_raw,
+};
+
+static const struct regmap_config sen0322_regmap_conf = {
+	.reg_bits = 8,
+	.val_bits = 8,
+};
+
+static const struct iio_chan_spec sen0322_channels[] = {
+	{
+		.type = IIO_CONCENTRATION,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+				      BIT(IIO_CHAN_INFO_PROCESSED) |
+				      BIT(IIO_CHAN_INFO_SCALE),
+	},
+};
+
+static int sen0322_probe(struct i2c_client *client)
+{
+	struct sen0322 *sen0322;
+	struct iio_dev *iio_dev;
+
+	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
+		return -ENODEV;
+
+	iio_dev = devm_iio_device_alloc(&client->dev, sizeof(*sen0322));
+	if (!iio_dev)
+		return -ENOMEM;
+
+	sen0322 = iio_priv(iio_dev);
+	sen0322->client = client;
+	sen0322->coeff = 0;
+
+	sen0322->regmap = devm_regmap_init_i2c(client, &sen0322_regmap_conf);
+	if (IS_ERR(sen0322->regmap))
+		return PTR_ERR(sen0322->regmap);
+
+	i2c_set_clientdata(client, sen0322);
+
+	iio_dev->info = &sen0322_info;
+	iio_dev->name = DRIVER_NAME;
+	iio_dev->channels = sen0322_channels;
+	iio_dev->num_channels = ARRAY_SIZE(sen0322_channels);
+	iio_dev->modes = INDIO_DIRECT_MODE;
+
+	return devm_iio_device_register(&client->dev, iio_dev);
+}
+
+static const struct of_device_id sen0322_of_match[] = {
+	{ .compatible = "dfrobot,sen0322" },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, sen0322_of_match);
+
+static struct i2c_driver sen0322_driver = {
+	.driver = {
+		.name = DRIVER_NAME,
+		.of_match_table = sen0322_of_match,
+	},
+	.probe = sen0322_probe,
+};
+module_i2c_driver(sen0322_driver);
+
+MODULE_AUTHOR("Tóth János <gomba007@gmail.com>");
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("SEN0322 oxygen sensor driver");

-- 
2.34.1

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

Posted by Jonathan Cameron 9 months, 1 week ago

On Mon, 28 Apr 2025 12:50:14 +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
> 
> To instantiate (assuming device is connected to I2C-2):
> 	echo 'sen0322 0x73' > /sys/class/i2c-dev/i2c-2/device/new_device
> 
> To read the oxygen concentration (assuming device is iio:device0):
> 	cat /sys/bus/iio/devices/iio:device0/in_concentration_input
> 
> Signed-off-by: Tóth János <gomba007@gmail.com>

Hi Tóth

Nice little driver.  Main questions are around the userspace ABI and why
we have both _RAW and _PROCESSED reported. There are few reasons we
let drivers do that and I don't see what reason applies here.

Mostly it just confuses userspace by providing multiple ways to read the
same thing.

Jonathan

> diff --git a/drivers/iio/chemical/sen0322.c b/drivers/iio/chemical/sen0322.c
> new file mode 100644
> index 000000000000..5f1f4528401e
> --- /dev/null
> +++ b/drivers/iio/chemical/sen0322.c
> @@ -0,0 +1,238 @@
> +// 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/math64.h>
> +#include <linux/module.h>
> +#include <linux/regmap.h>
> +
> +#include <linux/iio/iio.h>
> +
> +#define DRIVER_NAME "sen0322"
> +
> +#define SEN0322_REG_DATA	0x03
> +#define SEN0322_REG_COEFF	0x0A
> +
> +#define FIXED_FRAC_BITS		18
> +#define FIXED_INT(x)		((fixed_t)((x) << FIXED_FRAC_BITS))
> +
> +typedef u32 fixed_t;
> +
> +struct sen0322 {
> +	struct i2c_client	*client;
What do you need client for after probe?

There is a function to get the struct device from the regmap.

> +	struct regmap		*regmap;
> +	fixed_t			coeff;
> +};
> +
> +static fixed_t fixed_mul(fixed_t a, fixed_t b)
> +{
> +	u64 tmp;
> +
> +	tmp = (u64)a * (u64)b;
> +	tmp = (tmp >> FIXED_FRAC_BITS) + ((tmp >> FIXED_FRAC_BITS) & 1);

These need some comments.  It's moderately fiddly fixed point maths
and there are many ways to do that.

> +
> +	if (tmp > U32_MAX)
> +		return (fixed_t)U32_MAX;
> +	else
> +		return (fixed_t)tmp;
> +}
> +
> +static fixed_t fixed_div(fixed_t a, fixed_t b)
> +{
> +	u64 tmp;
> +
> +	tmp = (uint64_t)a << FIXED_FRAC_BITS;
> +	tmp += (b >> 1);
> +
> +	return (fixed_t)(div_u64(tmp, b));
> +}
> +
> +static int sen0322_read_coeff(struct sen0322 *sen0322)
> +{
> +	u32 val;
> +	int ret;
> +
> +	ret = regmap_read(sen0322->regmap, SEN0322_REG_COEFF, &val);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (val)
> +		sen0322->coeff = fixed_div(FIXED_INT(val), FIXED_INT(1000));
> +	else
> +		sen0322->coeff = fixed_div(FIXED_INT(209), FIXED_INT(1200));

This second one is just a number. Why not just put the constant here?

> +
> +	dev_dbg(&sen0322->client->dev, "coeff: %08X\n", sen0322->coeff);
> +
> +	return 0;
> +}
> +
> +static int sen0322_read_data(struct sen0322 *sen0322)
> +{
> +	u8 data[4] = { 0 };
> +	int ret;
> +
> +	ret = regmap_bulk_read(sen0322->regmap, SEN0322_REG_DATA, data, 3);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = data[0] * 100 +  data[1] * 10 + data[2];
> +
> +	dev_dbg(&sen0322->client->dev, "raw data: %d\n", ret);
> +
> +	return ret;
> +}
> +
> +static int sen0322_read_prep_data(struct sen0322 *sen0322)
> +{
> +	fixed_t val;
> +	int ret;
> +
> +	if (!sen0322->coeff) {
> +		ret = sen0322_read_coeff(sen0322);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	ret = sen0322_read_data(sen0322);
> +	if (ret < 0)
> +		return ret;
> +
> +	val = fixed_mul(sen0322->coeff, FIXED_INT(ret));
Superficially looks like you could compute a correct _SCALE and
make this maths a userspace problem?

> +
> +	dev_dbg(&sen0322->client->dev, "prep data: %08X\n", val);
> +
> +	return val >> FIXED_FRAC_BITS;
> +}
> +
> +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:

You need a strong reason to provide both _RAW and _PROCESSED.
What was your thinking here? 

As a general rule, if the conversion is linear, then we provide
_RAW and _SCALE. If it's non linear then _PROCESSED.

The _RAW + _SCALE thing is for 2 reasons.
1 - userspace is better at maths as it has floating point easily
    available.
2 - if we ever add buffered capture then _RAW tends to be of a defined
    number of bits whereas processed is more complex.

> +			ret = sen0322_read_data(sen0322);
> +			if (ret < 0)
> +				return ret;
> +
> +			*val = ret;
> +			return IIO_VAL_INT;
> +
> +		default:
> +			return -EINVAL;
> +		}
> +
> +	case IIO_CHAN_INFO_PROCESSED:
> +		switch (chan->type) {
> +		case IIO_CONCENTRATION:
> +			ret = sen0322_read_prep_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:
> +			*val = 1;
> +			*val2 = 100;

Given above you use the coeff in the calculation of processed
I don't understand what this scale is indicating.
Scale only applies to _RAW channels.

> +			return IIO_VAL_FRACTIONAL;
> +
> +		default:
> +			return -EINVAL;
> +		}
> +
> +	default:
> +		return -EINVAL;
> +	}
> +}

> +static const struct iio_chan_spec sen0322_channels[] = {
> +	{
> +		.type = IIO_CONCENTRATION,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> +				      BIT(IIO_CHAN_INFO_PROCESSED) |
> +				      BIT(IIO_CHAN_INFO_SCALE),
> +	},
> +};
This doesn't need to be an array. Can just use one structure
and pass the address plus an explicit 1 for the number of channels
below.   Quite a few drivers do it like this though and I don't mind
much.

> +
> +static int sen0322_probe(struct i2c_client *client)
> +{
> +	struct sen0322 *sen0322;
> +	struct iio_dev *iio_dev;
> +
> +	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
> +		return -ENODEV;
> +
> +	iio_dev = devm_iio_device_alloc(&client->dev, sizeof(*sen0322));
> +	if (!iio_dev)
> +		return -ENOMEM;
> +
> +	sen0322 = iio_priv(iio_dev);
> +	sen0322->client = client;
> +	sen0322->coeff = 0;
> +
> +	sen0322->regmap = devm_regmap_init_i2c(client, &sen0322_regmap_conf);
> +	if (IS_ERR(sen0322->regmap))
> +		return PTR_ERR(sen0322->regmap);
> +
> +	i2c_set_clientdata(client, sen0322);

I don't immediately see where this is used. If it's not then drop setting it.

> +
> +	iio_dev->info = &sen0322_info;
> +	iio_dev->name = DRIVER_NAME;

As below. I'd rather see the name here as a string.

> +	iio_dev->channels = sen0322_channels;
> +	iio_dev->num_channels = ARRAY_SIZE(sen0322_channels);
> +	iio_dev->modes = INDIO_DIRECT_MODE;
> +
> +	return devm_iio_device_register(&client->dev, iio_dev);
> +}
> +
> +static const struct of_device_id sen0322_of_match[] = {
> +	{ .compatible = "dfrobot,sen0322" },
> +	{ /* sentinel */ }

No real need for the comment.

> +};
> +MODULE_DEVICE_TABLE(of, sen0322_of_match);
> +
> +static struct i2c_driver sen0322_driver = {
> +	.driver = {

> +		.name = DRIVER_NAME,
I'd rather see the string directly here.  There is no reason
why the iio_dev->name above would always match this so in general
it is easier to just see the strings in each place rather than
under a define.

> +		.of_match_table = sen0322_of_match,
> +	},
> +	.probe = sen0322_probe,
> +};
> +module_i2c_driver(sen0322_driver);
> +
> +MODULE_AUTHOR("Tóth János <gomba007@gmail.com>");
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("SEN0322 oxygen sensor driver");
>

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

Posted by Tóth János 9 months, 1 week ago

Hi Jonathan!

Thank you for the review and the explanation! I've misunderstood the purpose of
_SCALE. I'll rewrite the driver to use only _RAW and _SCALE.

Regards,
János

[PATCH 1/2] dt-bindings: iio: chemical: Document SEN0322
[PATCH 2/2] iio: chemical: Add driver for SEN0322