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

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

[Gyeyoung Baek]

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

v3:
 - Add vin supply regulator.
 - Drop custom ABI.
 - Drop unnecessary mutex.

v2:
 - Add ABI doc.
 - Add complete struct to receive UART transmission successfully.
 - Add undersigned as a maintainer for the WINSEN MHZ19B.
 - Modify to comply with the IIO subsystem ABI as much as possible.
 - Revise the coding style overall.

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

 .../bindings/iio/chemical/winsen,mhz19b.yaml  |  33 ++
 .../devicetree/bindings/vendor-prefixes.yaml  |   2 +
 MAINTAINERS                                   |   6 +
 drivers/iio/chemical/Kconfig                  |  10 +
 drivers/iio/chemical/Makefile                 |   1 +
 drivers/iio/chemical/mhz19b.c                 | 347 ++++++++++++++++++
 6 files changed, 399 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/chemical/winsen,mhz19b.yaml
 create mode 100644 drivers/iio/chemical/mhz19b.c

--
2.34.1

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

[Gyeyoung Baek]

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

v3:
 - Add vin supply regulator.
 - Drop custom ABI.
 - Drop unnecessary mutex.

v2:
 - Add ABI doc.
 - Add complete struct to receive UART transmission successfully.
 - Add undersigned as a maintainer for the WINSEN MHZ19B.
 - Modify to comply with the IIO subsystem ABI as much as possible.
 - Revise the coding style overall.

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

 .../bindings/iio/chemical/winsen,mhz19b.yaml  |  33 ++
 .../devicetree/bindings/vendor-prefixes.yaml  |   2 +
 MAINTAINERS                                   |   6 +
 drivers/iio/chemical/Kconfig                  |  10 +
 drivers/iio/chemical/Makefile                 |   1 +
 drivers/iio/chemical/mhz19b.c                 | 347 ++++++++++++++++++
 6 files changed, 399 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/chemical/winsen,mhz19b.yaml
 create mode 100644 drivers/iio/chemical/mhz19b.c

--
2.34.1

[PATCH v3 1/4] dt-bindings: add winsen to the vendor prefixes

[Gyeyoung Baek]

Add winsen to the vendor prefixes.

Signed-off-by: Gyeyoung Baek <gye976@gmail.com>
Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 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 86f6a19b28ae..6d35549d2e4b 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
+++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
@@ -1689,6 +1689,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

[PATCH v3 2/4] 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>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 .../bindings/iio/chemical/winsen,mhz19b.yaml  | 33 +++++++++++++++++++
 1 file changed, 33 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..2a6ddb33f163
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/chemical/winsen,mhz19b.yaml
@@ -0,0 +1,33 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/chemical/winsen,mhz19b.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: MHZ19B CO2 sensor
+
+maintainers:
+  - Gyeyoung Baek <gye976@gmail.com>
+
+properties:
+  compatible:
+    const: winsen,mhz19b
+
+  vin-supply:
+    description: Regulator that provides power to the sensor
+
+required:
+  - compatible
+  - vin-supply
+
+additionalProperties: false
+
+examples:
+  - |
+    serial {
+      co2-sensor {
+        compatible = "winsen,mhz19b";
+        vin-supply = <&vdd>;
+      };
+    };
+...
--
2.34.1

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

[Gyeyoung Baek]

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>
---
 drivers/iio/chemical/Kconfig  |  10 +
 drivers/iio/chemical/Makefile |   1 +
 drivers/iio/chemical/mhz19b.c | 347 ++++++++++++++++++++++++++++++++++
 3 files changed, 358 insertions(+)
 create mode 100644 drivers/iio/chemical/mhz19b.c

diff --git a/drivers/iio/chemical/Kconfig b/drivers/iio/chemical/Kconfig
index 330fe0af946f..641bf9b35915 100644
--- a/drivers/iio/chemical/Kconfig
+++ b/drivers/iio/chemical/Kconfig
@@ -237,4 +237,14 @@ config VZ89X
 	  Sensortech MiCS VZ89X VOC (Volatile Organic Compounds)
 	  sensors

+config WINSEN_MHZ19B
+	tristate "Winsen MHZ19B CO2 sensor"
+	depends on SERIAL_DEV_BUS
+	help
+	  Say Y here to build Serdev interface support for the Winsen
+	  MHZ19B CO2 sensor.
+
+	  To compile this driver as a module, choose M here: the module will
+	  be called mhz19b.
+
 endmenu
diff --git a/drivers/iio/chemical/Makefile b/drivers/iio/chemical/Makefile
index 4866db06bdc9..deed437dd396 100644
--- a/drivers/iio/chemical/Makefile
+++ b/drivers/iio/chemical/Makefile
@@ -27,3 +27,4 @@ obj-$(CONFIG_SPS30) += sps30.o
 obj-$(CONFIG_SPS30_I2C) += sps30_i2c.o
 obj-$(CONFIG_SPS30_SERIAL) += sps30_serial.o
 obj-$(CONFIG_VZ89X)		+= vz89x.o
+obj-$(CONFIG_WINSEN_MHZ19B) += mhz19b.o
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>
+#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;
+
+	/*
+	 * 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];
+	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
+
+/* 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);
+
+		if (enable) {
+			cmd_buf[3] = 0xA0;
+			cmd_buf[8] = MHZ19B_ABC_LOGIC_ON_CKSUM;
+		} 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);
+
+		put_unaligned_be16(ppm, &cmd_buf[3]);
+		cmd_buf[MHZ19B_CMD_SIZE - 1] = mhz19b_get_checksum(cmd_buf);
+		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 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)
+{
+	int ret = mhz19b_serdev_cmd(indio_dev, MHZ19B_READ_CO2_CMD, NULL);
+
+	if (ret < 0)
+		return ret;
+
+	*val = ret;
+	return IIO_VAL_INT;
+}
+
+/*
+ * MHZ19B only supports writing configuration values.
+ *
+ * echo 0 > calibration_auto_enable : ABC logic off
+ * echo 1 > calibration_auto_enable : ABC logic on
+ *
+ * echo 0 > calibration_forced_value : zero point calibration
+ *	(make sure the sensor had been worked under 400ppm for over 20 minutes.)
+ * echo [1000 1 5000] > calibration_forced_value : span point calibration
+ *	(make sure the sensor had been worked under a certain level co2 for over 20 minutes.)
+ */
+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);
+
+	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);
+
+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, ret;
+
+	ret = kstrtou16(buf, 10, &ppm);
+	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_DEVICE_ATTR_WO(calibration_forced_value, 0);
+
+static struct attribute *mhz19b_attrs[] = {
+	&iio_dev_attr_calibration_auto_enable.dev_attr.attr,
+	&iio_dev_attr_calibration_forced_value.dev_attr.attr,
+	NULL
+};
+
+static const struct attribute_group mhz19b_attr_group = {
+	.attrs = mhz19b_attrs,
+};
+
+static const struct iio_info mhz19b_info = {
+	.attrs = &mhz19b_attr_group,
+	.read_raw = mhz19b_read_raw,
+};
+
+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),
+	},
+};
+
+static size_t mhz19b_receive_buf(struct serdev_device *serdev,
+	const u8 *data, size_t len)
+{
+	struct iio_dev *indio_dev = dev_get_drvdata(&serdev->dev);
+	struct mhz19b_state *st = iio_priv(indio_dev);
+
+	for (int i = 0; i < len; i++)
+		st->buf[st->buf_idx++] = data[i];
+
+	if (st->buf_idx == MHZ19B_CMD_SIZE) {
+		st->buf_idx = 0;
+		complete(&st->buf_ready);
+	}
+
+	return len;
+}
+
+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));
+	if (!indio_dev)
+		return ret;
+	dev_set_drvdata(dev, indio_dev);
+
+	st = iio_priv(indio_dev);
+	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);
+	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);
+}
+
+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,
+	.remove = mhz19b_remove,
+};
+module_serdev_device_driver(mhz19b_driver);
+
+MODULE_AUTHOR("Gyeyoung Baek");
+MODULE_DESCRIPTION("MH-Z19B CO2 sensor driver using serdev interface");
+MODULE_LICENSE("GPL");
--
2.34.1

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

[Jonathan Cameron]

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

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

[gyeyoung]

Hello Jonathan, thank you for taking the time to review my patch.

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

Sorry, It's not used, I missed that.

---

> > +#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?

Yes, we can calculate it simply, (actually previous patches did it in that way.)
I'll revert to that way for readability.

---

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

I used bool type for readability, but on second thought
The consistency of using u16 type seems more important than readability.
I'll edit this.

---

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

Sorry, I missed the existence of the devm_regulator_get_enable().
I'll edit this.

Thanks,
Gyeyoung

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

[Andy Shevchenko]

On Wed, Apr 9, 2025 at 5:45 AM 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

Make the above one line...

>

...and remove this blank line, hence this becomes a tag in the commit message.

> Signed-off-by: Gyeyoung Baek <gye976@gmail.com>

...

> +#include <linux/kernel.h>

No usual driver has a business to include the  kernel.h. Just follow
the IWYU principle and make sure what you include is what you use
here.

...

> +struct mhz19b_state {
> +       struct serdev_device *serdev;
> +       struct regulator *vin;
> +
> +       /*
> +        * 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];

Should it be DMA-safe?

> +       int buf_idx;
> +
> +       /* must wait the 'buf' is filled with 9 bytes.*/

...wait until the...

> +       struct completion buf_ready;
> +};

Have you run `pahole` to check if this is the best layout?

...

> +/*
> + * commands have following format:

Commands

> + *
> + * +------+------+-----+------+------+------+------+------+-------+
> + * | 0xFF | 0x01 | cmd | arg0 | arg1 | 0x00 | 0x00 | 0x00 | cksum |
> + * +------+------+-----+------+------+------+------+------+-------+
> + */

...

> +static uint8_t mhz19b_get_checksum(uint8_t *packet)

What's wrong with u8 here and everywhere else?

> +{
> +       uint8_t i, checksum = 0;
> +
> +       for (i = 1; i < 8; i++)

Shouldn't you use sizeof here? It seems related to that 9 above.

> +               checksum += packet[i];

> +       checksum = 0xff - checksum;
> +       checksum += 1;
> +
> +       return checksum;

Looks to me like these lines can be simplified to

  return -checksum;

> +}

...

> +       case MHZ19B_ABC_LOGIC_CMD: {
> +               bool enable = *((bool *)arg);

Oh, no. The boolean type is a tricky one and here you probably break
all the possible implementation defined behaviours esp. on bigendian
systems.

> +               if (enable) {
> +                       cmd_buf[3] = 0xA0;
> +                       cmd_buf[8] = MHZ19B_ABC_LOGIC_ON_CKSUM;
> +               } else {
> +                       cmd_buf[3] = 0;
> +                       cmd_buf[8] = MHZ19B_ABC_LOGIC_OFF_CKSUM;
> +               }
> +               break;

> +       } case MHZ19B_SPAN_POINT_CMD: {

Please, split the leading } from the case (everywhere). This is an
unusual style.

> +               uint16_t ppm = *((uint16_t *)arg);

Do you guarantee the alignment?

> +               put_unaligned_be16(ppm, &cmd_buf[3]);
> +               cmd_buf[MHZ19B_CMD_SIZE - 1] = mhz19b_get_checksum(cmd_buf);
> +               break;
> +       } case MHZ19B_ZERO_POINT_CMD: {
> +               cmd_buf[8] = MHZ19B_ZERO_POINT_CKSUM;
> +               break;
> +       } default:
> +               break;
> +       }

...

> +       /* write buf to uart ctrl syncronously */

UART
synchronously

Perhaps a spellchecker to run?

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

Can ret be negative? In such a case, please propagate the actual error code.

> +       }

...

> +               ret = mhz19b_get_checksum(st->buf);

How ret is being used? And if it may hold an error code, it should be
return to the upper layers, otherwise the ret variable is a bad choice
to keep this locally.

> +               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 ret;

...

> +static int mhz19b_read_raw(struct iio_dev *indio_dev,
> +       struct iio_chan_spec const *chan,
> +       int *val, int *val2, long mask)
> +{
> +       int ret = mhz19b_serdev_cmd(indio_dev, MHZ19B_READ_CO2_CMD, NULL);
> +
> +       if (ret < 0)
> +               return ret;

It's better to use the form of

   int ret;

   ret = ...
   if (ret)
      ...

Also why do you have ' < 0' parts? Please, double check that you use
this form of the errorcheck if and only if the callee may return a
positive value.

> +       *val = ret;
> +       return IIO_VAL_INT;
> +}

...

> +/*
> + * MHZ19B only supports writing configuration values.
> + *
> + * echo 0 > calibration_auto_enable : ABC logic off
> + * echo 1 > calibration_auto_enable : ABC logic on
> + *
> + * echo 0 > calibration_forced_value : zero point calibration
> + *     (make sure the sensor had been worked under 400ppm for over 20 minutes.)

has been working / had been working ?

> + * echo [1000 1 5000] > calibration_forced_value : span point calibration
> + *     (make sure the sensor had been worked under a certain level co2 for over 20 minutes.)
> + */

...

> +{
> +       struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +       bool enable;

> +       int ret = kstrtobool(buf, &enable);
> +
> +       if (ret)
> +               return ret;

Same comment to everywhere where you assign in the definition block
and check that in the code later on, this form is harder to maintain.

> +
> +       ret = mhz19b_serdev_cmd(indio_dev, MHZ19B_ABC_LOGIC_CMD, &enable);
> +       if (ret < 0)
> +               return ret;
> +
> +       return len;
> +}

...

> +       uint16_t ppm;

u16. Please, use kernel types everywhere.

...

> +       ret = kstrtou16(buf, 10, &ppm);

No hex?

> +       if (ret)
> +               return ret;

...

> +               if (ppm < 1000 || ppm > 5000) {

in_range()

> +                       dev_dbg(&indio_dev->dev,
> +                               "span point ppm should be 1000~5000");
> +                       return -EINVAL;
> +               }

...

> +static const struct attribute_group mhz19b_attr_group = {
> +       .attrs = mhz19b_attrs,
> +};

ATTRIBUTE_GROUP() ?

...

> +static size_t mhz19b_receive_buf(struct serdev_device *serdev,
> +       const u8 *data, size_t len)
> +{
> +       struct iio_dev *indio_dev = dev_get_drvdata(&serdev->dev);
> +       struct mhz19b_state *st = iio_priv(indio_dev);
> +
> +       for (int i = 0; i < len; i++)

Why signed?

> +               st->buf[st->buf_idx++] = data[i];
> +
> +       if (st->buf_idx == MHZ19B_CMD_SIZE) {
> +               st->buf_idx = 0;
> +               complete(&st->buf_ready);
> +       }
> +
> +       return len;
> +}

...

> +       ret = serdev_device_set_baudrate(serdev, 9600);
> +       if (ret < 0)

Why < 0?

> +               return ret;
> +
> +       serdev_device_set_flow_control(serdev, false);
> +
> +       ret = serdev_device_set_parity(serdev, SERDEV_PARITY_NONE);
> +       if (ret < 0)

Why < 0?

> +               return ret;

-- 
With Best Regards,
Andy Shevchenko

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

[gyeyoung]

Hello Andy, thank you for taking the time to review my patch.

> > +#include <linux/kernel.h>
>
> No usual driver has a business to include the  kernel.h. Just follow
> the IWYU principle and make sure what you include is what you use
> here.

I thought "linux/kernel.h" was a globally essential header.
Thank you for the information.

---

> > +struct mhz19b_state {
> > +       struct serdev_device *serdev;
> > +       struct regulator *vin;
> > +
> > +       /*
> > +        * 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];
>
> Should it be DMA-safe?
>
> > +       int buf_idx;

I'm not sure if I understood your point correctly,
This code isn't DMA-safe. I'm currently understanding why DMA-safe is necessary.
(but actually other drivers implementing 'serdev ops' use non-DMA-safe buffers.)
I will verify this part and then send the next patch.

---

> > +       struct completion buf_ready;
> > +};
>
> Have you run `pahole` to check if this is the best layout?

No, I haven't run 'pahole' yet, but I'll verify the structure layout
using the tool from now on.
Thank you for the information.

---

> > +       case MHZ19B_ABC_LOGIC_CMD: {
> > +               bool enable = *((bool *)arg);
>
> Oh, no. The boolean type is a tricky one and here you probably break
> all the possible implementation defined behaviours esp. on bigendian
> systems.

> > +               uint16_t ppm = *((uint16_t *)arg);
>
> Do you guarantee the alignment?

So far, the arg has been the address of a u16 type stack variable, so
there was no error.
But I'll edit this by referring to the alignment documentation.

---

> > +       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;
>
> Can ret be negative? In such a case, please propagate the actual error code.

Yes, ret can be negative. and I'll propagate the actual error code from now on.

> > +       }
>
> ...
>
> > +               ret = mhz19b_get_checksum(st->buf);
>
> How ret is being used? And if it may hold an error code, it should be
> return to the upper layers, otherwise the ret variable is a bad choice
> to keep this locally.

Sorry, this "ret = mhz19b_get_checksum(st->buf);" line should be dropped,
 I missed that.

---

> > +static int mhz19b_read_raw(struct iio_dev *indio_dev,
> > +       struct iio_chan_spec const *chan,
> > +       int *val, int *val2, long mask)
> > +{
> > +       int ret = mhz19b_serdev_cmd(indio_dev, MHZ19B_READ_CO2_CMD, NULL);
> > +
> > +       if (ret < 0)
> > +               return ret;
>
> It's better to use the form of
>
>    int ret;
>
>    ret = ...
>    if (ret)
>       ...
>
> Also why do you have ' < 0' parts? Please, double check that you use
> this form of the errorcheck if and only if the callee may return a
> positive value.

Yes, this function returns either a positive value(0 ~ 2000),
depending on the cmd argument.
So 'if (ret < 0)' is more appropriate than 'if (ret)' in this case.

---

>
> > +static const struct attribute_group mhz19b_attr_group = {
> > +       .attrs = mhz19b_attrs,
> > +};
>
> ATTRIBUTE_GROUP() ?

I looked into the API and found only ATTRIBUTE_GROUPS(),
But using ATTRIBUTE_GROUPS() requires the attribute_group value to be
declared as an array ending with a NULL entry. Would this be OK?
then i'll use ATTRIBUTE_GROUPS().

---

> > +static size_t mhz19b_receive_buf(struct serdev_device *serdev,
> > +       const u8 *data, size_t len)
> > +{
> > +       struct iio_dev *indio_dev = dev_get_drvdata(&serdev->dev);
> > +       struct mhz19b_state *st = iio_priv(indio_dev);
> > +
> > +       for (int i = 0; i < len; i++)
>
> Why signed?

Would it be better to declare 'i' as 'size_t' to match the type of 'len'?
then I'll change 'int i' to 'size_t i'.

---

> > +       ret = serdev_device_set_baudrate(serdev, 9600);
> > +       if (ret < 0)
>
> Why < 0?

First I assumed there was an error, but after checking the
serdev_device_set_baudrate() function,
I found that it doesn't return any error. so I'll remove the error check.

---

> > +               return ret;
> > +
> > +       serdev_device_set_flow_control(serdev, false);
> > +
> > +       ret = serdev_device_set_parity(serdev, SERDEV_PARITY_NONE);
> > +       if (ret < 0)
>
> Why < 0?

I'll simply change it to 'if (ret)'.

---

I'll revise the overall coding style by referring to other reviews.

Thanks,
Gyeyoung

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

[Andy Shevchenko]

On Mon, Apr 14, 2025 at 6:49 PM gyeyoung <gye976@gmail.com> wrote:

...

> > > +#include <linux/kernel.h>
> >
> > No usual driver has a business to include the  kernel.h. Just follow
> > the IWYU principle and make sure what you include is what you use
> > here.
>
> I thought "linux/kernel.h" was a globally essential header.

Not at all, it's a big mess which no driver should use.
And on top of that (to any header) you should not use 'proxy' headers.
It's a bad style and practice with the real consequences as build time
and headache for the others who want to clean up header dependencies
in the future.

...

> > > +       /*
> > > +        * 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];
> >
> > Should it be DMA-safe?
>
> I'm not sure if I understood your point correctly,
> This code isn't DMA-safe. I'm currently understanding why DMA-safe is necessary.
> (but actually other drivers implementing 'serdev ops' use non-DMA-safe buffers.)
> I will verify this part and then send the next patch.

Because some of the UART drivers may enable DMA by default if it's
available and your code won't work on them, right? But double check if
serdev makes it DMA-safe before use.

...

> > > +       case MHZ19B_ABC_LOGIC_CMD: {
> > > +               bool enable = *((bool *)arg);
> >
> > Oh, no. The boolean type is a tricky one and here you probably break
> > all the possible implementation defined behaviours esp. on bigendian
> > systems.
>
> > > +               uint16_t ppm = *((uint16_t *)arg);
> >
> > Do you guarantee the alignment?
>
> So far, the arg has been the address of a u16 type stack variable, so
> there was no error.
> But I'll edit this by referring to the alignment documentation.

Easier to use the proper accessor, i.e. one of get_unaligned*() calls here.

...

> > Also why do you have ' < 0' parts? Please, double check that you use
> > this form of the errorcheck if and only if the callee may return a
> > positive value.
>
> Yes, this function returns either a positive value(0 ~ 2000),
> depending on the cmd argument.
> So 'if (ret < 0)' is more appropriate than 'if (ret)' in this case.

I see, so make sure that only those that may return positive values
will have the  < 0 filter, and others (that return 0 or negative error
code) use the regular form.

...

> > > +static const struct attribute_group mhz19b_attr_group = {
> > > +       .attrs = mhz19b_attrs,
> > > +};
> >
> > ATTRIBUTE_GROUP() ?
>
> I looked into the API and found only ATTRIBUTE_GROUPS(),
> But using ATTRIBUTE_GROUPS() requires the attribute_group value to be
> declared as an array ending with a NULL entry. Would this be OK?
> then i'll use ATTRIBUTE_GROUPS().

Indeed, only if you use that array, otherwise the original code is OK.

...

> > > +       for (int i = 0; i < len; i++)
> >
> > Why signed?
>
> Would it be better to declare 'i' as 'size_t' to match the type of 'len'?
> then I'll change 'int i' to 'size_t i'.

It will be consistent.

-- 
With Best Regards,
Andy Shevchenko

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

[gyeyoung]

On Tue, Apr 15, 2025 at 2:21 AM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Mon, Apr 14, 2025 at 6:49 PM gyeyoung <gye976@gmail.com> wrote:
>
> ...
>
> > > > +#include <linux/kernel.h>
> > >
> > > No usual driver has a business to include the  kernel.h. Just follow
> > > the IWYU principle and make sure what you include is what you use
> > > here.
> >
> > I thought "linux/kernel.h" was a globally essential header.
>
> Not at all, it's a big mess which no driver should use.
> And on top of that (to any header) you should not use 'proxy' headers.
> It's a bad style and practice with the real consequences as build time
> and headache for the others who want to clean up header dependencies
> in the future.

Now I see. I had been referring to some legacy examples, but looking
at it again,
It makes more sense to explicitly include only what's needed.
Thank you for pointing out the good practice.

Thanks,
Gyeyoung

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

[gyeyoung]

Hello Andy,

On Tue, Apr 15, 2025 at 2:21 AM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> > > > +       /*
> > > > +        * 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];
> > >
> > > Should it be DMA-safe?
> >
> > I'm not sure if I understood your point correctly,
> > This code isn't DMA-safe. I'm currently understanding why DMA-safe is necessary.
> > (but actually other drivers implementing 'serdev ops' use non-DMA-safe buffers.)
> > I will verify this part and then send the next patch.
>
> Because some of the UART drivers may enable DMA by default if it's
> available and your code won't work on them, right? But double check if
> serdev makes it DMA-safe before use.

It seems that the serdev buf doesn't need to be DMA-safe. I looked
into the PL011 driver as an example,
which uses DMA, and found that the data received via DMA is firstly
stored in the buffer within the 'uart_amba_port' structure, and then
copied into the 'tty_bufhead' within the tty_port. Later, in serdev's
receive_buf(), it simply copies from the tty_bufhead to into serdev
buf. So I think there's no need to consider DMA-safe in the serdev buf
itself.
would this make sense? If so, I think there is no need to change the
code related to the buffer.

Regards,
Gyeyoung

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

[Andy Shevchenko]

On Thu, Apr 17, 2025 at 11:03:12PM +0900, gyeyoung wrote:
> On Tue, Apr 15, 2025 at 2:21 AM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:

...

> > > > > +       /*
> > > > > +        * 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];
> > > >
> > > > Should it be DMA-safe?
> > >
> > > I'm not sure if I understood your point correctly,
> > > This code isn't DMA-safe. I'm currently understanding why DMA-safe is necessary.
> > > (but actually other drivers implementing 'serdev ops' use non-DMA-safe buffers.)
> > > I will verify this part and then send the next patch.
> >
> > Because some of the UART drivers may enable DMA by default if it's
> > available and your code won't work on them, right? But double check if
> > serdev makes it DMA-safe before use.
> 
> It seems that the serdev buf doesn't need to be DMA-safe. I looked
> into the PL011 driver as an example,
> which uses DMA, and found that the data received via DMA is firstly
> stored in the buffer within the 'uart_amba_port' structure, and then
> copied into the 'tty_bufhead' within the tty_port. Later, in serdev's
> receive_buf(), it simply copies from the tty_bufhead to into serdev
> buf. So I think there's no need to consider DMA-safe in the serdev buf
> itself.

But who will give those guarantees (note, the code is most likely may be run on
different UART controllers (PL011 is just one of many), have you checked all
supported drivers for DMA?

> would this make sense? If so, I think there is no need to change the
> code related to the buffer.

The bare minimum is to make sure this buffer occupies the cacheline.
Read about DMA safety for the cache coherency.

-- 
With Best Regards,
Andy Shevchenko

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

[Gyeyoung Baek]

> > It seems that the serdev buf doesn't need to be DMA-safe. I looked
> > into the PL011 driver as an example,
> > which uses DMA, and found that the data received via DMA is firstly
> > stored in the buffer within the 'uart_amba_port' structure, and then
> > copied into the 'tty_bufhead' within the tty_port. Later, in serdev's
> > receive_buf(), it simply copies from the tty_bufhead to into serdev
> > buf. So I think there's no need to consider DMA-safe in the serdev buf
> > itself.

> But who will give those guarantees (note, the code is most likely may be run on
> different UART controllers (PL011 is just one of many), have you checked all
> supported drivers for DMA?

No, I just checked one. 
Sorry for generalizing even though I'm not sure.

> > would this make sense? If so, I think there is no need to change the
> > code related to the buffer.

> The bare minimum is to make sure this buffer occupies the cacheline.
> Read about DMA safety for the cache coherency.

It seems appropriate to use '__aligned(IIO_DMA_MINALIGN)' to ensure cache alignment.
I'll use that, thanks.

--
Regards,
Gyeyoung

[PATCH v3 4/4] MAINTAINERS: Add WINSEN MHZ19B

[Gyeyoung Baek]

Add undersigned as a maintainer for the WINSEN MHZ19B.

Signed-off-by: Gyeyoung Baek <gye976@gmail.com>
---
 MAINTAINERS | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 030d90d38341..8135d1bbda68 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -26008,6 +26008,12 @@ M:	David Härdeman <david@hardeman.nu>
 S:	Maintained
 F:	drivers/media/rc/winbond-cir.c

+WINSEN MHZ19B
+M:	Gyeyoung Baek <gye976@gmail.com>
+S:	Maintained
+F:	Documentation/devicetree/bindings/iio/chemical/winsen,mhz19b.yaml
+F:	drivers/iio/chemical/mhz19b.c
+
 WINSYSTEMS EBC-C384 WATCHDOG DRIVER
 L:	linux-watchdog@vger.kernel.org
 S:	Orphan
--
2.34.1