[PATCH 0/3] add support for winsen MHZ19B CO2 sensor

[PATCH 0/3] add support for winsen MHZ19B CO2 sensor

[Gyeyoung Baek]

This patch series adds support for winsen MHZ19B CO2 sensor.

Datasheet:
https://www.winsen-sensor.com/d/files/infrared-gas-sensor/mh-z19b-co2-ver1_0.pdf

Gyeyoung Baek (3):
  iio: chemical: add support for winsen MHZ19B CO2 sensor
  dt-bindings: add device tree support for winsen MHZ19B CO2 sensor
  dt-bindings: add winsen to the vendor prefixes

 .../bindings/iio/chemical/winsen,mhz19b.yaml  |  31 ++
 .../devicetree/bindings/vendor-prefixes.yaml  |   2 +
 drivers/iio/chemical/Kconfig                  |   6 +
 drivers/iio/chemical/Makefile                 |   1 +
 drivers/iio/chemical/mhz19b.c                 | 354 ++++++++++++++++++
 5 files changed, 394 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/chemical/winsen,mhz19b.yaml
 create mode 100644 drivers/iio/chemical/mhz19b.c

-- 
2.34.1

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

[Gyeyoung Baek]

Add support for winsen MHZ19B CO2 sensor.

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/Kconfig b/drivers/iio/chemical/Kconfig
index 330fe0af946f..6b09a9c52f32 100644
--- a/drivers/iio/chemical/Kconfig
+++ b/drivers/iio/chemical/Kconfig
@@ -108,6 +108,12 @@ config IAQCORE
 	  iAQ-Core Continuous/Pulsed VOC (Volatile Organic Compounds)
 	  sensors
 
+config MHZ19B
+	tristate "MHZ19B CO2 sensor"
+	depends on SERIAL_DEV_BUS
+	help
+	  Say Y here to build support for the MHZ19B CO2 sensor.
+
+         To compile this driver as a module, choose M here: the module will
+         be called scd4x.
+
 config PMS7003
 	tristate "Plantower PMS7003 particulate matter sensor"
 	depends on SERIAL_DEV_BUS
diff --git a/drivers/iio/chemical/Makefile b/drivers/iio/chemical/Makefile
index 4866db06bdc9..c63daebf39ac 100644
--- a/drivers/iio/chemical/Makefile
+++ b/drivers/iio/chemical/Makefile
@@ -15,6 +15,7 @@ obj-$(CONFIG_ENS160) += ens160_core.o
 obj-$(CONFIG_ENS160_I2C) += ens160_i2c.o
 obj-$(CONFIG_ENS160_SPI) += ens160_spi.o
 obj-$(CONFIG_IAQCORE)		+= ams-iaq-core.o
+obj-$(CONFIG_MHZ19B) += mhz19b.o
 obj-$(CONFIG_PMS7003) += pms7003.o
 obj-$(CONFIG_SCD30_CORE) += scd30_core.o
 obj-$(CONFIG_SCD30_I2C) += scd30_i2c.o
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>
+#include <linux/device.h>
+#include <linux/iio/iio.h>
+#include <linux/mutex.h>
+#include <linux/cleanup.h>
+
+struct mhz19b_state {
+	struct serdev_device *serdev;
+
+	/* serdev receive buffer */
+	char buf[9];
+	int buf_idx;
+
+	/* protect access to mhz19b_state */
+	struct mutex lock;
+};
+
+/* ABC logig on/off */
+#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
+
+#define MHZ19B_CMD_SIZE 9
+
+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, 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;
+
+	/*
+	 * 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:
+	{
+		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;
+		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;
+		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];
+			break;
+		default:
+			/* no response commands. */
+			ret = 0;
+			break;
+		}
+	}
+
+	return ret;
+}
+
+static int mhz19b_read_raw(struct iio_dev *indio_dev, struct iio_chan_spec const *chan,
+	int *val, int *val2, long mask)
+{
+	struct mhz19b_state *mhz19b;
+	int ret;
+
+	mhz19b = iio_priv(indio_dev);
+
+	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,
+};
+
+static ssize_t mhz19b_zero_point_write(struct iio_dev *iiodev,
+	uintptr_t private, const struct iio_chan_spec *chan,
+	const char *buf, size_t len)
+{
+	int ret;
+
+	ret = mhz19b_serdev_cmd(iiodev, MHZ19B_ZERO_POINT_CMD, buf);
+	if (ret < 0)
+		return ret;
+
+	return len;
+}
+
+static ssize_t mhz19b_span_point_write(struct iio_dev *iiodev,
+	uintptr_t private, const struct iio_chan_spec *chan,
+	const char *buf, size_t len)
+{
+	int ret;
+
+	ret = mhz19b_serdev_cmd(iiodev, MHZ19B_SPAN_POINT_CMD, buf);
+	if (ret < 0)
+		return ret;
+
+	return len;
+}
+
+static ssize_t mhz19b_abc_logic_write(struct iio_dev *iiodev,
+	uintptr_t private, const struct iio_chan_spec *chan,
+	const char *buf, size_t len)
+{
+	int ret;
+
+	ret = mhz19b_serdev_cmd(iiodev, MHZ19B_ABC_LOGIC_CMD, buf);
+	if (ret < 0)
+		return ret;
+
+	return len;
+}
+
+struct iio_chan_spec_ext_info mhz19b_co2_ext_info[] = {
+	{
+		.name = "zero_point",
+		.write = mhz19b_zero_point_write,
+	},
+	{
+		.name = "span_point",
+		.write = mhz19b_span_point_write,
+	},
+	{
+		.name = "abc_logic",
+		.write = mhz19b_abc_logic_write,
+	},
+	{}
+};
+
+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)
+{
+	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));
+	if (indio_dev == NULL)
+		return ret;
+
+	dev_set_drvdata(dev, indio_dev);
+
+	mhz19b = iio_priv(indio_dev);
+
+	mhz19b->buf_idx = 0;
+	ret = devm_mutex_init(dev, &mhz19b->lock);
+	if (ret)
+		return ret;
+
+	serdev = container_of(dev, struct serdev_device, dev);
+
+	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);
+	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 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");
+}
+
+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;
+
+	ret = mhz19b_core_probe(dev);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static const struct of_device_id mhz19b_of_match[] = {
+	{ .compatible = "winsen,mhz19b", },
+	{}
+};
+
+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,
+};
+
+module_serdev_device_driver(mhz19b_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Gyeyoung Baek");
+MODULE_DESCRIPTION("MH-Z19B CO2 sensor driver using serdev interface");
-- 
2.34.1

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

[Jonathan Cameron]

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

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

[gyeyoung]

Hello Jonathan, thank you for the review.
Sorry for the late response.

> > +
> > +             /* 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.

Since I couldn't find a function like 'unaligned_put_be16',
but I found a function like 'be16_to_cpu', so I will use that.




> > +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.

 I did it arbitrarily. I will refer to the documentation
and rewrite it to be as compatible as possible with the existing ABI, thanks.




>
> > +
> > +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.
>

'serdev_device_write_wakeup' member function is mandatory.
If this function is not set and remains NULL, the
'serdev_device_write' function will just return -EINVAL.

The following is a part of serdev_device_write().
------------
ssize_t serdev_device_write(struct serdev_device *serdev, const u8 *buf,
   size_t count, long timeout)
{
struct serdev_controller *ctrl = serdev->ctrl;
size_t written = 0;
ssize_t ret;

if (!ctrl || !ctrl->ops->write_buf || !serdev->ops->write_wakeup)
return -EINVAL;
.
.
.
------------

> > +}
> > +
> > +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.

'serdev_device_set_flow_control' is a void function.
and as far as I know, 'serdev_device_set_baudrate' does not return an error.
but I'll check again.

I'll revise it considering your overall coding style guide.

Thanks,
Gyeyoung Baek

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

[Jonathan Cameron]

On Mon, 31 Mar 2025 23:36:17 +0900
gyeyoung <gye976@gmail.com> wrote:

> Hello Jonathan, thank you for the review.
> Sorry for the late response.
> 
> > > +
> > > +             /* 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.  
> 
> Since I couldn't find a function like 'unaligned_put_be16',
> but I found a function like 'be16_to_cpu', so I will use that.

You can't. An array of u8 is not guaranteed to be aligned so in
some cases be16_to_cpu() will fail.

I was half asleep :(
put_unaligned_be16() is what you are looking for.
https://elixir.bootlin.com/linux/v6.13.7/source/include/linux/unaligned.h#L61


> > > +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.
> >  
> 
> 'serdev_device_write_wakeup' member function is mandatory.
> If this function is not set and remains NULL, the
> 'serdev_device_write' function will just return -EINVAL.
> 
> The following is a part of serdev_device_write().
> ------------
> ssize_t serdev_device_write(struct serdev_device *serdev, const u8 *buf,
>    size_t count, long timeout)
> {
> struct serdev_controller *ctrl = serdev->ctrl;
> size_t written = 0;
> ssize_t ret;
> 
> if (!ctrl || !ctrl->ops->write_buf || !serdev->ops->write_wakeup)
> return -EINVAL;
> .
> .
> .
> ------------

Ok. So why not serdev_device_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;
> > > +
> > > +     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.  
> 
> 'serdev_device_set_flow_control' is a void function.
> and as far as I know, 'serdev_device_set_baudrate' does not return an error.
> but I'll check again.

Ah I missed that. No problem not checking it.

Jonathan

> 
> I'll revise it considering your overall coding style guide.
> 
> Thanks,
> Gyeyoung Baek
> 
> 

[PATCH 2/3] dt-bindings: add device tree support for winsen MHZ19B CO2 sensor

[Gyeyoung Baek]

Add device tree support for winsen MHZ19B sensor.

Signed-off-by: Gyeyoung Baek <gye976@gmail.com>
---
 .../bindings/iio/chemical/winsen,mhz19b.yaml  | 31 +++++++++++++++++++
 1 file changed, 31 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/chemical/winsen,mhz19b.yaml

diff --git a/Documentation/devicetree/bindings/iio/chemical/winsen,mhz19b.yaml b/Documentation/devicetree/bindings/iio/chemical/winsen,mhz19b.yaml
new file mode 100644
index 000000000000..c08681e43281
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/chemical/winsen,mhz19b.yaml
@@ -0,0 +1,31 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/chemical/dht11.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: MHZ19B CO2 sensor
+
+maintainers:
+  - Gyeyoung Baek <gye976@gmail.com>
+
+description: |
+   CO2 sensor using UART serdev bus interface.
+
+properties:
+  compatible:
+    const: winsen,mhz19b
+
+required:
+  - compatible
+
+additionalProperties: false
+
+examples:
+  - |
+    serial {
+      mhz19b-co2-sensor {
+        compatible = "winsen,mhz19b";
+      };
+    };
+...
-- 
2.34.1

Re: [PATCH 2/3] dt-bindings: add device tree support for winsen MHZ19B CO2 sensor

[Krzysztof Kozlowski]

On 29/03/2025 17:49, Gyeyoung Baek wrote:
> Add device tree support for winsen MHZ19B sensor.
> 
> Signed-off-by: Gyeyoung Baek <gye976@gmail.com>
> ---
>  .../bindings/iio/chemical/winsen,mhz19b.yaml  | 31 +++++++++++++++++++
>  1 file changed, 31 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/chemical/winsen,mhz19b.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/chemical/winsen,mhz19b.yaml b/Documentation/devicetree/bindings/iio/chemical/winsen,mhz19b.yaml
> new file mode 100644
> index 000000000000..c08681e43281
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/chemical/winsen,mhz19b.yaml
> @@ -0,0 +1,31 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/chemical/dht11.yaml#

Never tested.

> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: MHZ19B CO2 sensor
> +
> +maintainers:
> +  - Gyeyoung Baek <gye976@gmail.com>
> +
> +description: |
> +   CO2 sensor using UART serdev bus interface.

serdev is Linux thing. Just drop description.

> +
> +properties:
> +  compatible:
> +    const: winsen,mhz19b
> +
> +required:
> +  - compatible
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    serial {
> +      mhz19b-co2-sensor {

Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation



Best regards,
Krzysztof

Re: [PATCH 2/3] dt-bindings: add device tree support for winsen MHZ19B CO2 sensor

[gyeyoung]

On Sun, Mar 30, 2025 at 6:39 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On 29/03/2025 17:49, Gyeyoung Baek wrote:
> > Add device tree support for winsen MHZ19B sensor.
> >
> > Signed-off-by: Gyeyoung Baek <gye976@gmail.com>
> > ---
> >  .../bindings/iio/chemical/winsen,mhz19b.yaml  | 31 +++++++++++++++++++
> >  1 file changed, 31 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/iio/chemical/winsen,mhz19b.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/iio/chemical/winsen,mhz19b.yaml b/Documentation/devicetree/bindings/iio/chemical/winsen,mhz19b.yaml
> > new file mode 100644
> > index 000000000000..c08681e43281
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/chemical/winsen,mhz19b.yaml
> > @@ -0,0 +1,31 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/iio/chemical/dht11.yaml#
>
> Never tested.

sorry, I missed that, my mistake.

> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: MHZ19B CO2 sensor
> > +
> > +maintainers:
> > +  - Gyeyoung Baek <gye976@gmail.com>
> > +
> > +description: |
> > +   CO2 sensor using UART serdev bus interface.
>
> serdev is Linux thing. Just drop description.

oh, I see that a hardware-specific description is needed here.
I now understand the documentation style. thanks.

> > +
> > +properties:
> > +  compatible:
> > +    const: winsen,mhz19b
> > +
> > +required:
> > +  - compatible
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    serial {
> > +      mhz19b-co2-sensor {
>
> Node names should be generic. See also an explanation and list of
> examples (not exhaustive) in DT specification:
> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
>
>
>
> Best regards,
> Krzysztof

Thanks,
Gyeyoung Baek

Re: [PATCH 2/3] dt-bindings: add device tree support for winsen MHZ19B CO2 sensor

[Jonathan Cameron]

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

> Add device tree support for winsen MHZ19B sensor.
> 
> Signed-off-by: Gyeyoung Baek <gye976@gmail.com>
> ---
>  .../bindings/iio/chemical/winsen,mhz19b.yaml  | 31 +++++++++++++++++++
>  1 file changed, 31 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/chemical/winsen,mhz19b.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/chemical/winsen,mhz19b.yaml b/Documentation/devicetree/bindings/iio/chemical/winsen,mhz19b.yaml
> new file mode 100644
> index 000000000000..c08681e43281
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/chemical/winsen,mhz19b.yaml
> @@ -0,0 +1,31 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/chemical/dht11.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: MHZ19B CO2 sensor
> +
> +maintainers:
> +  - Gyeyoung Baek <gye976@gmail.com>
> +
> +description: |
> +   CO2 sensor using UART serdev bus interface.
> +
> +properties:
> +  compatible:
> +    const: winsen,mhz19b
Hi, 

Welcome to IIO. 

Just one additional comment from me.

Bindings should be as complete as possible and also reflect
the existence of things like power supplies even if your particular
board is relying in them being always turned on.

So you need 
vin-supply: true;

and add it to the required items.




> +
> +required:
> +  - compatible
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    serial {
> +      mhz19b-co2-sensor {
> +        compatible = "winsen,mhz19b";
> +      };
> +    };
> +...

Re: [PATCH 2/3] dt-bindings: add device tree support for winsen MHZ19B CO2 sensor

[gyeyoung]

Hello Jonathan, thank you for the review.


On Sun, Mar 30, 2025 at 9:50 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Sun, 30 Mar 2025 01:49:04 +0900
> Gyeyoung Baek <gye976@gmail.com> wrote:
>
> > Add device tree support for winsen MHZ19B sensor.
> >
> > Signed-off-by: Gyeyoung Baek <gye976@gmail.com>
> > ---
> >  .../bindings/iio/chemical/winsen,mhz19b.yaml  | 31 +++++++++++++++++++
> >  1 file changed, 31 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/iio/chemical/winsen,mhz19b.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/iio/chemical/winsen,mhz19b.yaml b/Documentation/devicetree/bindings/iio/chemical/winsen,mhz19b.yaml
> > new file mode 100644
> > index 000000000000..c08681e43281
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/chemical/winsen,mhz19b.yaml
> > @@ -0,0 +1,31 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/iio/chemical/dht11.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: MHZ19B CO2 sensor
> > +
> > +maintainers:
> > +  - Gyeyoung Baek <gye976@gmail.com>
> > +
> > +description: |
> > +   CO2 sensor using UART serdev bus interface.
> > +
> > +properties:
> > +  compatible:
> > +    const: winsen,mhz19b
> Hi,
>
> Welcome to IIO.
>
> Just one additional comment from me.
>
> Bindings should be as complete as possible and also reflect
> the existence of things like power supplies even if your particular
> board is relying in them being always turned on.
>
> So you need
> vin-supply: true;
>
> and add it to the required items.
>

Yes, I will add that by referring to other IIO code. thanks.

>
>
> > +
> > +required:
> > +  - compatible
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    serial {
> > +      mhz19b-co2-sensor {
> > +        compatible = "winsen,mhz19b";
> > +      };
> > +    };
> > +...
>

Thanks,
Gyeyoung Baek

[PATCH 3/3] dt-bindings: add winsen to the vendor prefixes

[Gyeyoung Baek]

Add winsen to the vendor prefixes.

Signed-off-by: Gyeyoung Baek <gye976@gmail.com>
---
 Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml
index 5079ca6ce1d1..ee7f6100c432 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
+++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
@@ -1679,6 +1679,8 @@ patternProperties:
     description: Wingtech Technology Co., Ltd.
   "^winlink,.*":
     description: WinLink Co., Ltd
+  "^winsen,.*":
+    description: Winsen Corp.
   "^winstar,.*":
     description: Winstar Display Corp.
   "^wirelesstag,.*":
-- 
2.34.1

Re: [PATCH 3/3] dt-bindings: add winsen to the vendor prefixes

[Krzysztof Kozlowski]

On 29/03/2025 17:49, Gyeyoung Baek wrote:
> Add winsen to the vendor prefixes.
> 
> Signed-off-by: Gyeyoung Baek <gye976@gmail.com>
> ---
Your patches are ordered entirely in opposite order. First you use the
binding, then you document. First you use prefix, then you document it.

It's not bisectable. Reverse everything (see also DT submitting patches).

Best regards,
Krzysztof

Re: [PATCH 3/3] dt-bindings: add winsen to the vendor prefixes

[gyeyoung]

Hello Krzysztof, thank you for the review.


On Sun, Mar 30, 2025 at 6:37 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On 29/03/2025 17:49, Gyeyoung Baek wrote:
> > Add winsen to the vendor prefixes.
> >
> > Signed-off-by: Gyeyoung Baek <gye976@gmail.com>
> > ---
> Your patches are ordered entirely in opposite order. First you use the
> binding, then you document. First you use prefix, then you document it.
>
> It's not bisectable. Reverse everything (see also DT submitting patches).

On second thought, reverse order makes more sense. thanks.

> Best regards,
> Krzysztof

Thanks,
Gyeyoung Baek