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

[Jonathan Cameron <jic23@kernel.org>]

On Wed,  9 Apr 2025 11:43:10 +0900
Gyeyoung Baek <gye976@gmail.com> wrote:

> Add support for winsen MHZ19B CO2 sensor.
> 
> Datasheet:
> https://www.winsen-sensor.com/d/files/infrared-gas-sensor/mh-z19b-co2-ver1_0.pdf
> 
> Signed-off-by: Gyeyoung Baek <gye976@gmail.com>

Hi Gyeyoung,

A few additional comments inline.

Jonathan

> diff --git a/drivers/iio/chemical/mhz19b.c b/drivers/iio/chemical/mhz19b.c
> new file mode 100644
> index 000000000000..d9a16e022b36
> --- /dev/null
> +++ b/drivers/iio/chemical/mhz19b.c
> @@ -0,0 +1,347 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * mh-z19b co2 sensor driver
> + *
> + * Copyright (c) 2025 Gyeyoung Baek <gye976@gmail.com>
> + *
> + * Datasheet:
> + * https://www.winsen-sensor.com/d/files/infrared-gas-sensor/mh-z19b-co2-ver1_0.pdf
> + */
> +
> +#include <linux/cleanup.h>

Do you use this?

> +#include <linux/completion.h>
> +#include <linux/device.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/serdev.h>
> +#include <linux/unaligned.h>
> +
> +struct mhz19b_state {
> +	struct serdev_device *serdev;
> +	struct regulator *vin;

As inline, you shouldn't need to keep this around if you use
the devm based management for the regulator.

> +
> +	/*
> +	 * serdev receive buffer.
> +	 * When data is received from the MH-Z19B,
> +	 * the 'mhz19b_receive_buf' callback function is called and fills this buffer.
> +	 */
> +	char buf[9];

Use the CMD_SIZE define below for this 9 (having moved it up).

> +	int buf_idx;
> +
> +	/* must wait the 'buf' is filled with 9 bytes.*/
> +	struct completion buf_ready;
> +};
> +
> +/*
> + * commands have following format:
> + *
> + * +------+------+-----+------+------+------+------+------+-------+
> + * | 0xFF | 0x01 | cmd | arg0 | arg1 | 0x00 | 0x00 | 0x00 | cksum |
> + * +------+------+-----+------+------+------+------+------+-------+
> + */
> +#define MHZ19B_CMD_SIZE 9
> +
> +#define MHZ19B_ABC_LOGIC_CMD		0x79
> +#define MHZ19B_READ_CO2_CMD		0x86
> +#define MHZ19B_SPAN_POINT_CMD		0x88
> +#define MHZ19B_ZERO_POINT_CMD		0x87
> +
> +#define MHZ19B_ABC_LOGIC_OFF_CKSUM	0x86
> +#define MHZ19B_ABC_LOGIC_ON_CKSUM	0xE6
> +#define MHZ19B_READ_CO2_CKSUM		0x79
> +#define MHZ19B_ZERO_POINT_CKSUM	0x78
Can we not just calculate these from the buffer contents?

> +
> +/* ABC logic in MHZ19B means auto calibration. */
> +
> +#define MHZ19B_SERDEV_TIMEOUT	msecs_to_jiffies(100)
> +
> +static uint8_t mhz19b_get_checksum(uint8_t *packet)
> +{
> +	uint8_t i, checksum = 0;
> +
> +	for (i = 1; i < 8; i++)
> +		checksum += packet[i];
> +
> +	checksum = 0xff - checksum;
> +	checksum += 1;
> +
> +	return checksum;
> +}
> +
> +static int mhz19b_serdev_cmd(struct iio_dev *indio_dev,
> +	int cmd, void *arg)
> +{
> +	struct mhz19b_state *st = iio_priv(indio_dev);
> +	struct serdev_device *serdev = st->serdev;
> +	struct device *dev = &indio_dev->dev;
> +	int ret;
> +
> +	/*
> +	 * cmd_buf[3,4] : arg0,1
> +	 * cmd_buf[8]	: checksum
> +	 */
> +	uint8_t cmd_buf[MHZ19B_CMD_SIZE] = {
> +		0xFF, 0x01, cmd,
> +	};
> +
> +	switch (cmd) {
> +	case MHZ19B_ABC_LOGIC_CMD: {
> +		bool enable = *((bool *)arg);
Given you could just pass and u16 for the argument and use 0 /1 for
the bool.  The u16 works directly for the ppm value where needed for span
point

> +
> +		if (enable) {
> +			cmd_buf[3] = 0xA0;
> +			cmd_buf[8] = MHZ19B_ABC_LOGIC_ON_CKSUM;
All these checksums should be easy enough to calculate which would be
simpler to follow than writing constants like this.

You are already doing so for the span point one so do that for them
all.

> +		} else {
> +			cmd_buf[3] = 0;
> +			cmd_buf[8] = MHZ19B_ABC_LOGIC_OFF_CKSUM;
> +		}
> +		break;
> +	} case MHZ19B_READ_CO2_CMD: {
> +		cmd_buf[8] = MHZ19B_READ_CO2_CKSUM;
> +		break;
> +	} case MHZ19B_SPAN_POINT_CMD: {
> +		uint16_t ppm = *((uint16_t *)arg);
For kernel internal code use the older types u16 etc

> +
> +		put_unaligned_be16(ppm, &cmd_buf[3]);
> +		cmd_buf[MHZ19B_CMD_SIZE - 1] = mhz19b_get_checksum(cmd_buf);
Using index value of 8 for all these for consistency.
> +		break;
> +	} case MHZ19B_ZERO_POINT_CMD: {
> +		cmd_buf[8] = MHZ19B_ZERO_POINT_CKSUM;
> +		break;
> +	} default:
> +		break;
> +	}
> +
> +	/* write buf to uart ctrl syncronously */
> +	ret = serdev_device_write(serdev, cmd_buf, MHZ19B_CMD_SIZE, 0);
> +	if (ret != MHZ19B_CMD_SIZE) {
> +		dev_err(dev, "write err, %d bytes written", ret);
> +		return -EINVAL;
> +	}
> +
> +	switch (cmd) {
> +	case MHZ19B_READ_CO2_CMD:
> +		ret = wait_for_completion_interruptible_timeout(&st->buf_ready,
> +			MHZ19B_SERDEV_TIMEOUT);
> +		if (ret < 0)
> +			return ret;
> +		if (!ret)
> +			return -ETIMEDOUT;
> +
> +		ret = mhz19b_get_checksum(st->buf);
> +		if (st->buf[MHZ19B_CMD_SIZE - 1] != mhz19b_get_checksum(st->buf)) {
> +			dev_err(dev, "checksum err");
> +			return -EINVAL;
> +		}
> +
> +		ret = get_unaligned_be16(&st->buf[2]);

return get_unaligned_be16()

> +		return ret;
> +	default:
> +		/* no response commands. */
> +		return 0;
> +	}
> +}
> +
> +static int mhz19b_read_raw(struct iio_dev *indio_dev,
> +	struct iio_chan_spec const *chan,
> +	int *val, int *val2, long mask)

Generally, unless we have very long lines align the following sets of parameters
with just after the (
static int mhz19b_read_raw(struct iio_dev *indio_dev,
			   struct iio_chan_spec const *chan,
			   int *val, int *val2, long mask)

Same applies in various other places in the code.

> +{
> +	int 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 serdev_device_ops mhz19b_ops = {
> +	.receive_buf = mhz19b_receive_buf,
> +	.write_wakeup = serdev_device_write_wakeup,
> +};
> +
> +static int mhz19b_probe(struct serdev_device *serdev)
> +{
> +	int ret;
> +	struct device *dev = &serdev->dev;
> +	struct iio_dev *indio_dev;
> +	struct mhz19b_state *st;
> +
> +	serdev_device_set_client_ops(serdev, &mhz19b_ops);
> +
> +	ret = devm_serdev_device_open(dev, serdev);
> +	if (ret)
> +		return ret;
> +
> +	ret = serdev_device_set_baudrate(serdev, 9600);
> +	if (ret < 0)
> +		return ret;
> +
> +	serdev_device_set_flow_control(serdev, false);
> +
> +	ret = serdev_device_set_parity(serdev, SERDEV_PARITY_NONE);
> +	if (ret < 0)
> +		return ret;
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(struct mhz19b_state));

sizeof(*st) is both more compact and obviously correct with out a reader
having to go check if the st in..

> +	if (!indio_dev)
> +		return ret;
> +	dev_set_drvdata(dev, indio_dev);
> +
> +	st = iio_priv(indio_dev);

this line is the same size as we passed to devm_iio_device_alloc()

> +	st->serdev = serdev;
> +
> +	init_completion(&st->buf_ready);
> +
> +	st->vin = devm_regulator_get(dev, "vin");
> +	if (IS_ERR(st->vin))
> +		return PTR_ERR(st->vin);
> +
> +	ret = regulator_enable(st->vin);

Don't mix devm and non devm calls in probe.  In this case you introduced
a race where the power is turned off before we remove the userspace
interfaces which is unlikely to go well...

	ret = devm_regulator_get_enabled(dev, "vin);
	if (ret)
		return ret;

and no need for a remove.

> +	if (ret)
> +		return ret;
> +
> +	indio_dev->name = "mh-z19b";
> +	indio_dev->channels = mhz19b_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(mhz19b_channels);
> +	indio_dev->info = &mhz19b_info;
> +
> +	return devm_iio_device_register(dev, indio_dev);
> +}
> +
> +static void mhz19b_remove(struct serdev_device *serdev)
> +{
> +	struct iio_dev *indio_dev = dev_get_drvdata(&serdev->dev);
> +	struct mhz19b_state *st = iio_priv(indio_dev);
> +
> +	regulator_disable(st->vin);

As above - remove will be unnecessary once the regulator is also
device managed (devm_...)

J