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

[Jonathan Cameron <Jonathan.Cameron@huawei.com>]

On Thu,  3 Apr 2025 14:32:24 +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.

Various comments inline,

Thanks,

Jonathan

> diff --git a/drivers/iio/chemical/mhz19b.c b/drivers/iio/chemical/mhz19b.c
> new file mode 100644
> index 000000000000..f8f06c01623f
> --- /dev/null
> +++ b/drivers/iio/chemical/mhz19b.c
> @@ -0,0 +1,386 @@

> +struct mhz19b_state {
> +	struct serdev_device *serdev;
> +
> +	/* TO DO, nothing for now.*/
> +	struct regulator *vin_supply;

Unlikely you need this here. Look at the
devm regulator calls.

> +
> +	/* serdev receive buffer.

Fix comment syntax as mentioned below.

> +	 * When data is received from the MH-Z19B,
> +	 * the 'mhz19b_receive_buf' callback function is called and fills this buffer.
> +	 */
> +	char buf[9];
> +	int buf_idx;
> +
> +	/* must wait the 'buf' is filled with 9 bytes.*/
> +	struct completion buf_ready;
> +
> +	/* protect access to mhz19b_state */

Be more specific. What and why?  It's not protecting the regulator.

> +	struct mutex lock;
> +};
> +
> +/*
> + * commands have following format:
> + *
> + * +------+------+-----+------+------+------+------+------+-------+
> + * | 0xFF | 0x01 | cmd | arg0 | arg1 | 0x00 | 0x00 | 0x00 | cksum |
> + * +------+------+-----+------+------+------+------+------+-------+
> + *
> + * The following commands are defined in the datasheet.
> + * https://www.winsen-sensor.com/d/files/infrared-gas-sensor/mh-z19b-co2-ver1_0.pdf
> + */
> +#define MHZ19B_CMD_SIZE 9
> +
> +/* ABC logic in MHZ19B means auto calibration.
One line comment so move the */ up to end of this line.

> + */

> +
> +static int mhz19b_serdev_cmd(struct iio_dev *indio_dev,
> +	int cmd, void *arg)
> +{
> +	int ret = 0;
Move declaraton to the scope where it is used.
Then it will be obcious that it is alaway set.

> +	struct mhz19b_state *st = iio_priv(indio_dev);
> +	struct serdev_device *serdev = st->serdev;
> +	struct device *dev = &indio_dev->dev;
> +
> +	/* commands format is described above. */
> +	uint8_t cmd_buf[MHZ19B_CMD_SIZE] = {
> +		0xFF, 0x01, cmd,
> +	};
> +
> +	switch (cmd) {
> +	case MHZ19B_ABC_LOGIC_CMD: {
> +		bool enable = *((bool *)arg);
> +
> +		cmd_buf[3] = enable ? 0xA0 : 0x00;
> +		break;
> +	} case MHZ19B_SPAN_POINT_CMD: {
> +		uint16_t ppm = *((uint16_t *)arg);
> +
> +		put_unaligned_be16(ppm, &cmd_buf[3]);
> +		break;
> +	} case MHZ19B_DETECTION_RANGE_CMD: {
> +		uint16_t range = *((uint16_t *)arg);
> +
> +		put_unaligned_be16(range, &cmd_buf[3]);
> +		break;
> +	} default:
> +		break;
> +	}
> +	cmd_buf[MHZ19B_CMD_SIZE - 1] = mhz19b_get_checksum(cmd_buf);
> +
> +	scoped_guard(mutex, &st->lock) {
> +		/* 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;
> +		}
> +	}
> +}


Cache the ones that aren't a one time thing then (such as this one).

> +
> +static ssize_t calibration_auto_enable_store(struct device *dev,
> +	struct device_attribute *attr,
> +	const char *buf, size_t len)
> +{
> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +	bool enable;
> +
> +	int ret = kstrtobool(buf, &enable);
As below. Either have this in the declaration block or not. This
sort of half and half with extra blank lines is unusual.


> +
> +	if (ret)
> +		return ret;
> +
> +	ret = mhz19b_serdev_cmd(indio_dev, MHZ19B_ABC_LOGIC_CMD, &enable);
> +	if (ret < 0)
> +		return ret;
> +
> +	return len;
> +}
> +static IIO_DEVICE_ATTR_WO(calibration_auto_enable, 0);
> +
> +/* write 0		: zero point calibration_auto_enable
> + *	(make sure the sensor had been worked under 400ppm for over 20 minutes.)
> + *
> + * write 1000-5000	: span point calibration:
> + *	(make sure the sensor had been worked under a certain level co2 for over 20 minutes.)
> + */
> +static ssize_t calibration_forced_value_store(struct device *dev,
> +	struct device_attribute *attr,
> +	const char *buf, size_t len)
> +{
> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +	uint16_t ppm;
> +	int cmd;
The line break here is odd.  I'd define
	int ret, cmd;

	ret = kstrou16()

> +
> +	int ret = kstrtou16(buf, 10, &ppm);
> +
This blank line is also then unnecessary.

> +	if (ret)
> +		return ret;
> +
> +	/* at least 1000ppm */
> +	if (ppm) {
> +		if (ppm < 1000 || ppm > 5000) {
> +			dev_dbg(&indio_dev->dev,
> +				"span point ppm should be 1000~5000");
> +			return -EINVAL;
> +		}
> +
> +		cmd = MHZ19B_SPAN_POINT_CMD;
> +	} else {
> +		cmd = MHZ19B_ZERO_POINT_CMD;
> +	}
> +
> +	ret = mhz19b_serdev_cmd(indio_dev, cmd, &ppm);
> +	if (ret < 0)
> +		return ret;
> +
> +	return len;
> +}
> +static IIO_CONST_ATTR(calibration_forced_value_available,
> +	"0 1000-5000");

Check the syntax for available attributes. We don't have a good
way to describe this particular set of ranges. So best option
may unfortunately be to not describe it at all.
Anyone calibrating this device is going to be reading the datasheet
anyway so should know what is possible.

> +static IIO_DEVICE_ATTR_WO(calibration_forced_value, 0);
> +
> +/* MH-Z19B supports a measurement range adjustment feature.
/*
 * MH-Z...

> + * It can measure up to 2000 ppm or up to 5000 ppm.
> + */
> +static ssize_t co2_range_store(struct device *dev,
As per comments on ABI docs patch, I don't think we need custom ABI for this.

> +	struct device_attribute *attr,
> +	const char *buf, size_t len)
Except on very long lines, align parameters with just after ( rather.

> +{
> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +	int ret;
> +	uint16_t range;
> +
> +	ret = kstrtou16(buf, 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;
> +	}
> +
No need for more than one blank line.
> +
> +	ret = mhz19b_serdev_cmd(indio_dev, MHZ19B_DETECTION_RANGE_CMD, &range);
> +	if (ret < 0)
> +		return ret;
> +
> +	return len;
> +}
> +static IIO_CONST_ATTR(co2_range_available,
> +	"2000 5000");
> +static IIO_DEVICE_ATTR_WO(co2_range, 0);
> +
> +static struct attribute *mhz19b_attrs[] = {
> +	&iio_const_attr_calibration_forced_value_available.dev_attr.attr,
> +	&iio_const_attr_co2_range_available.dev_attr.attr,
> +	&iio_dev_attr_calibration_auto_enable.dev_attr.attr,
> +	&iio_dev_attr_calibration_forced_value.dev_attr.attr,
> +	&iio_dev_attr_co2_range.dev_attr.attr,
> +	NULL
> +};

> +
> +static const struct iio_chan_spec mhz19b_channels[] = {
> +	{
> +		.type = IIO_CONCENTRATION,
> +		.channel2 = IIO_MOD_CO2,
> +		.modified = 1,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),

I'm curious. We have a range control in your custom ABI, but no scale.
So what units is this in?

> +	},
> +};

> +
> +/* The 'serdev_device_write' function returns -EINVAL if the 'write_wakeup' member is NULL,
> + * so it must be mandatory.

Answering that in review was fine. No need for comment
(which is formatted wrong for IIO that uses
/*
 * The ...


> + */
> +static void mhz19b_write_wakeup(struct serdev_device *serdev)
> +{
> +	struct iio_dev *indio_dev = dev_get_drvdata(&serdev->dev);

There was still an open question on that where I asked
if a standard existing callback was the correct minimal thing to do.

> +
> +	dev_dbg(&indio_dev->dev, "mhz19b_write_wakeup");
> +}
> +
> +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 = &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;
> +
> +	/* void type func, no return */

No need for comment then! I was wrong in previous review.

> +	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));
> +	if (indio_dev == NULL)

For simple pointer validity checks.

	if (!indio_dev) is probably more common than checking if NUL.

> +		return ret;
> +	dev_set_drvdata(dev, indio_dev);
> +
> +	st = iio_priv(indio_dev);
> +	st->serdev = serdev;
> +
> +	init_completion(&st->buf_ready);
> +	ret = devm_mutex_init(dev, &st->lock);
> +	if (ret)
> +		return ret;
> +
> +	/* TO DO:
> +	 *  - vin supply

The code is very simple to just turn on the power supply
at driver load and off at unload. Simpler to do that from the start than
end up with it being added later.

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

>