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

[Jonathan Cameron <jic23@kernel.org>]

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");
>