Re: [PATCH 1/3] iio: chemical: add support for winsen MHZ19B CO2 sensor

[Jonathan Cameron <jic23@kernel.org>]

On Sun, 30 Mar 2025 01:49:03 +0900
Gyeyoung Baek <gye976@gmail.com> wrote:

> Add support for winsen MHZ19B CO2 sensor.
Hi,

Good to add a little more detail here.
> 
Ideally add a
DataSheet tag here so we have a record in the git log on where to find
a datasheet.

Various comments inline.

The big stuff is that you are adding ABI without documentation.
Also that ABI doesn't seem that well aligned with existing calibration related
ABI.

> Signed-off-by: Gyeyoung Baek <gye976@gmail.com>
> ---
>  drivers/iio/chemical/Kconfig  |   6 +
>  drivers/iio/chemical/Makefile |   1 +
>  drivers/iio/chemical/mhz19b.c | 354 ++++++++++++++++++++++++++++++++++
>  3 files changed, 361 insertions(+)
>  create mode 100644 drivers/iio/chemical/mhz19b.c
> 

> diff --git a/drivers/iio/chemical/mhz19b.c b/drivers/iio/chemical/mhz19b.c
> new file mode 100644
> index 000000000000..de900131035b
> --- /dev/null
> +++ b/drivers/iio/chemical/mhz19b.c
> @@ -0,0 +1,354 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * mh-z19b co2 sensor driver
> + *
> + * Copyright (c) 2025 Gyeyoung Baek <gye976@gmail.com>
> + */
> +
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/init.h>
> +#include <linux/serdev.h>
> +#include <linux/of.h>
Shouldn't be needed here. I'd guess you want
mod_devicetable.h

> +#include <linux/device.h>
> +#include <linux/iio/iio.h>
> +#include <linux/mutex.h>
> +#include <linux/cleanup.h>
Alphabetical order preferred for includes.  As it's an IIO driver
you can pull that out to a separate include block at the end if you
like. I'm fine with it in alphabetical order with the other headers
if you prefer that.

> +
> +struct mhz19b_state {
> +	struct serdev_device *serdev;
> +
> +	/* serdev receive buffer */
> +	char buf[9];
> +	int buf_idx;
> +
> +	/* protect access to mhz19b_state */
Be more specific. I'd imagine it's the buffer rather
than the serdev pointer...
> +	struct mutex lock;
> +};
> +
> +/* ABC logig on/off */

If the names of command defines are good then we don't need the comments.
I'd modify them a little to make that true here and drop the comments
unless they are adding something more

> +#define MHZ19B_ABC_LOGIC_CMD		0x79
> +/* read CO2 concentration */
> +#define MHZ19B_READ_CO2_CMD		0x86
> +/* calibrate Zero Point */
> +#define MHZ19B_ZERO_POINT_CMD		0x87
> +/* calibrate Span Point */
> +#define MHZ19B_SPAN_POINT_CMD		0x88
> +/* set sensor detection range */
> +#define MHZ19B_DETECTION_RANGE_CMD	0x99
}
> +
> +static int mhz19b_serdev_cmd(struct iio_dev *indio_dev, int cmd, const char *str)
> +{
> +	int ret = 0;
> +	struct serdev_device *serdev;
> +	struct mhz19b_state *mhz19b;
> +	struct device *dev;
> +
> +	mhz19b = iio_priv(indio_dev);
> +	serdev = mhz19b->serdev;
> +	dev = &indio_dev->dev;
These can all be combined with declarations to save a few lines of code.

> +
> +	/*
> +	 * commands have following format:
> +	 *
> +	 * +------+------+-----+------+------+------+------+------+-------+
> +	 * | 0xFF | 0x01 | cmd | arg0 | arg1 | 0x00 | 0x00 | 0x00 | cksum |
> +	 * +------+------+-----+------+------+------+------+------+-------+
> +	 */
> +	uint8_t cmd_buf[MHZ19B_CMD_SIZE] = {
> +		0xFF, 0x01, cmd,
> +	};
> +
> +	switch (cmd) {
> +	case MHZ19B_ABC_LOGIC_CMD:
> +	{

I'd move the { to the line above.


> +		bool enable;
> +
> +		ret = kstrtobool(str, &enable);
> +		if (ret)
> +			return ret;
> +
> +		cmd_buf[3] = enable ? 0xA0 : 0x00;
> +		break;
> +	}
> +	case MHZ19B_SPAN_POINT_CMD:
> +	{
> +		uint16_t ppm;
> +
> +		ret = kstrtou16(str, 10, &ppm);
> +		if (ret)
> +			return ret;
> +
> +		/* at least 1000ppm */
> +		if (ppm < 1000 || ppm > 5000) {
> +			dev_dbg(&indio_dev->dev, "span point ppm should be 1000~5000");
> +			return -EINVAL;
> +		}
> +
> +		cmd_buf[3] = ppm / 256;
> +		cmd_buf[4] = ppm % 256;

That's an elaborate way of doing
		unaligned_put_be16()
so use that instead as it's also clearly documenting what is going on.

> +		break;
> +	}
> +	case MHZ19B_DETECTION_RANGE_CMD:
> +	{
> +		uint16_t range;
> +
> +		ret = kstrtou16(str, 10, &range);
> +		if (ret)
> +			return ret;
> +
> +		/* Detection Range should be 2000 or 5000 */
> +		if (!(range == 2000 || range == 5000)) {
> +			dev_dbg(&indio_dev->dev, "detection range should be 2000 or 5000");
> +			return -EINVAL;
> +		}
> +
> +		cmd_buf[3] = range / 256;
> +		cmd_buf[4] = range % 256;
Same as above.
> +		break;
> +	}
> +	default:
> +		break;
> +	}
> +	cmd_buf[MHZ19B_CMD_SIZE - 1] = mhz19b_get_checksum(cmd_buf);
> +
> +	scoped_guard(mutex, &mhz19b->lock) {
> +		ret = serdev_device_write(serdev, cmd_buf, MHZ19B_CMD_SIZE, 0);
> +		mhz19b->buf_idx = 0;
> +
> +		if (ret != MHZ19B_CMD_SIZE) {
> +			dev_err(dev, "write err, %d bytes written", ret);
> +			return -EINVAL;
> +		}
> +
> +		switch (cmd) {
> +		case MHZ19B_READ_CO2_CMD:
> +			if (mhz19b->buf[MHZ19B_CMD_SIZE - 1] != mhz19b_get_checksum(mhz19b->buf)) {
> +				dev_err(dev, "checksum err");
> +				return -EINVAL;
> +			}
> +
> +			ret = (mhz19b->buf[2] << 8) + mhz19b->buf[3];

That's an unaligned_get_be16() I think. If so use that instead of opencoding.

> +			break;
> +		default:
> +			/* no response commands. */
Might as well return early in each of these cases.

> +			ret = 0;
> +			break;
> +		}
> +	}
> +
> +	return ret;
> +}
> +
> +static int mhz19b_read_raw(struct iio_dev *indio_dev, struct iio_chan_spec const *chan,
Trivial but for IIO I'd prefer we try to keep under 80 chars still when it does
not effect readability.  Here adding a wrap after indio_dev doesn't make it harder
to read.

> +	int *val, int *val2, long mask)
Align after ( 

> +{
> +	struct mhz19b_state *mhz19b;
> +	int ret;
> +
> +	mhz19b = iio_priv(indio_dev);
	struct mhz19b_state *mhz19b = iio_priv(indio_dev);
at the point of declaration above.

> +
> +	ret = mhz19b_serdev_cmd(indio_dev, MHZ19B_READ_CO2_CMD, NULL);
> +	if (ret < 0)
> +		return ret;
> +
> +	*val = ret;
> +	return IIO_VAL_INT;
> +}
> +
> +static const struct iio_info mhz19b_info = {
> +	.read_raw = mhz19b_read_raw,
> +};

> +
> +struct iio_chan_spec_ext_info mhz19b_co2_ext_info[] = {
> +	{
> +		.name = "zero_point",

This is custom ABI.  Before we consider that in detail we
need documentation in
Documentation/ABI/testing/sysfs-bus-iio-mhz19b
It is much easier to review ABI with docs.
All 3 are direct commands to the device, so I've no idea from
what we have here on what they do.

Superficially this one looks like a calibration control.
There is existing ABI for that.


> +		.write = mhz19b_zero_point_write,
> +	},
> +	{
> +		.name = "span_point",
> +		.write = mhz19b_span_point_write,
Also looks like calibration.  See if you can come
up with ABI that matches with what we already have for calibration
of ADCs etc.
> +	},
> +	{
> +		.name = "abc_logic",

Definitely not good logic. ABC is a term they made up as far
as i can tell.  See if you can find existing ABI for this.
I think we have other controls for autonomous calibration cycles.

> +		.write = mhz19b_abc_logic_write,
> +	},
> +	{}
	{ }
preferred for IIO code.

> +};
> +
> +static const struct iio_chan_spec mhz19b_channels[] = {
> +	{
> +		.type = IIO_CONCENTRATION,
> +		.channel2 = IIO_MOD_CO2,
> +		.modified = 1,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
> +
> +		.ext_info = mhz19b_co2_ext_info,
> +	},
> +};
> +
> +static int mhz19b_core_probe(struct device *dev)

As below.  This function isn't sufficiently complex to justify
a separate function.

> +{
> +	int ret;
> +
> +	struct serdev_device *serdev;
> +	struct mhz19b_state *mhz19b;
> +	struct iio_dev *indio_dev;
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(struct mhz19b_state));

sizeof(*mhz19b));

> +	if (indio_dev == NULL)
> +		return ret;
> +
> +	dev_set_drvdata(dev, indio_dev);
> +
> +	mhz19b = iio_priv(indio_dev);
> +
> +	mhz19b->buf_idx = 0;
No need to explicitly zero as it is allocated by kzalloc.  Fine to
keep it though if you think it adds benefit as 'documentation'.
> +	ret = devm_mutex_init(dev, &mhz19b->lock);
> +	if (ret)
> +		return ret;
> +
> +	serdev = container_of(dev, struct serdev_device, dev);

breaking out the _core_probe() makes this more complex as in the
caller serdev is already available.

> +
> +	mhz19b->serdev = serdev;
> +
> +	indio_dev->name = "mh-z19b";
> +	indio_dev->channels = mhz19b_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(mhz19b_channels);
> +	indio_dev->info = &mhz19b_info;
> +
> +	ret = devm_iio_device_register(dev, indio_dev);

return devm_iio_device_register()

> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static size_t mhz19b_receive_buf(struct serdev_device *serdev, const u8 *data, size_t len)
> +{
> +	struct iio_dev *indio_dev;
	struct iio_dev *indio_dev = dev_get_drvdata(&serdev->dev);
	struct mhz19b_state *mhz19b = iio_priv(indio_dev);

to save a few lines.


> +	struct mhz19b_state *mhz19b;
> +
> +	indio_dev = dev_get_drvdata(&serdev->dev);
> +	mhz19b = iio_priv(indio_dev);
> +
> +	for (int i = 0; i < len; i++)
> +		mhz19b->buf[mhz19b->buf_idx++] = data[i];
> +
> +	return len;
> +}
> +
> +static void mhz19b_write_wakeup(struct serdev_device *serdev)
> +{
> +	struct iio_dev *indio_dev;
> +
> +	indio_dev = dev_get_drvdata(&serdev->dev);
> +
> +	dev_dbg(&indio_dev->dev, "mhz19b_write_wakeup");

This doesn't do anything which makes me suspicious. Would
using serdev_device_write_wakeup() as the callback make
sense?  I'm not that familiar with serial drivers but I can
see that a number of other drivers do that.

> +}
> +
> +static const struct serdev_device_ops mhz19b_ops = {
> +	.receive_buf = mhz19b_receive_buf,
> +	.write_wakeup = mhz19b_write_wakeup,
> +};
> +
> +static int mhz19b_probe(struct serdev_device *serdev)
> +{
> +	int ret;
> +
> +	struct device *dev;
> +
> +	dev = &serdev->dev;
> +	serdev_device_set_client_ops(serdev, &mhz19b_ops);
> +
> +	ret = devm_serdev_device_open(dev, serdev);
> +	if (ret)
> +		return ret;
> +
> +	serdev_device_set_baudrate(serdev, 9600);
> +	serdev_device_set_flow_control(serdev, false);
> +	ret = serdev_device_set_parity(serdev, SERDEV_PARITY_NONE);
> +	if (ret < 0)
> +		return ret;

Why check return value from this call but not the previous two?
I'm not immediately able to see a reason this is more likely to fail.

> +
> +	ret = mhz19b_core_probe(dev);

Having a separate _core_probe() seems unnecessary. I'd jut have a single
probe function and put all that code inline here.

> +	if (ret)
> +		return ret;
> +

return mhz19b_core_probe();

> +	return 0;
> +}
> +
> +static const struct of_device_id mhz19b_of_match[] = {
> +	{ .compatible = "winsen,mhz19b", },
> +	{}

Trivial: I'm trying to standardize formatting of these in IIO
and made the random choice of
	{ }
as the terminating entry style.

> +};

Similar to below, it is common practice to have no blank line
between this array of structs and the MODULE_DEVICE_TABLE
to reflect how tightly they are coupled.

> +
> +MODULE_DEVICE_TABLE(of, mhz19b_of_match);
> +
> +static struct serdev_device_driver mhz19b_driver = {
> +	.driver = {
> +		.name = "mhz19b",
> +		.of_match_table = mhz19b_of_match,
> +	},
> +	.probe = mhz19b_probe,
> +};
> +

Typical style for these module* lines is to couple them
closely with the struct. That is done by having no blank line here.

> +module_serdev_device_driver(mhz19b_driver);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Gyeyoung Baek");
> +MODULE_DESCRIPTION("MH-Z19B CO2 sensor driver using serdev interface");