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

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

[Gyeyoung Baek]

v4:
 - Ensure buffer is aligned to the cacheline. 
 - Fix coding style overall.

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                 | 307 ++++++++++++++++++
 6 files changed, 359 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/chemical/winsen,mhz19b.yaml
 create mode 100644 drivers/iio/chemical/mhz19b.c

-- 
2.34.1

[PATCH v4 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 v4 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 v4 3/4] iio: chemical: add support for winsen MHZ19B CO2 sensor

[Gyeyoung Baek]

Add support for winsen MHZ19B CO2 sensor.

The datasheet is available at
Link: 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 | 307 ++++++++++++++++++++++++++++++++++
 3 files changed, 318 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..63db82124efa
--- /dev/null
+++ b/drivers/iio/chemical/mhz19b.c
@@ -0,0 +1,307 @@
+// 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/completion.h>
+#include <linux/device.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/regulator/consumer.h>
+#include <linux/serdev.h>
+#include <linux/unaligned.h>
+
+/*
+ * Commands have following format:
+ *
+ * +------+------+-----+------+------+------+------+------+-------+
+ * | 0xFF | 0x01 | cmd | arg0 | arg1 | 0x00 | 0x00 | 0x00 | cksum |
+ * +------+------+-----+------+------+------+------+------+-------+
+ */
+#define MHZ19B_CMD_SIZE 9
+
+/* ABC logic in MHZ19B means auto calibration. */
+#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_SERDEV_TIMEOUT msecs_to_jiffies(100)
+
+struct mhz19b_state {
+	struct serdev_device *serdev;
+
+	/* Must wait until the 'buf' is filled with 9 bytes.*/
+	struct completion buf_ready;
+
+	u8 buf_idx;
+	/*
+	 * Serdev receive buffer.
+	 * When data is received from the MH-Z19B,
+	 * the 'mhz19b_receive_buf' callback function is called and fills this buffer.
+	 */
+	u8 buf[MHZ19B_CMD_SIZE] __aligned(IIO_DMA_MINALIGN);
+};
+
+static u8 mhz19b_get_checksum(u8 *cmd_buf)
+{
+	u8 i, checksum = 0;
+
+/*
+ * +------+------+-----+------+------+------+------+------+-------+
+ * | 0xFF | 0x01 | cmd | arg0 | arg1 | 0x00 | 0x00 | 0x00 | cksum |
+ * +------+------+-----+------+------+------+------+------+-------+
+ *	     i:1    2      3      4      5      6      7
+ *
+ *  Sum all cmd_buf elements from index 1 to 7.
+ */
+	for (i = 1; i < 8; i++)
+		checksum += cmd_buf[i];
+
+	return -checksum;
+}
+
+static int mhz19b_serdev_cmd(struct iio_dev *indio_dev, int cmd, u16 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
+	 */
+	u8 cmd_buf[MHZ19B_CMD_SIZE] = {
+		0xFF, 0x01, cmd,
+	};
+
+	switch (cmd) {
+	case MHZ19B_ABC_LOGIC_CMD:
+		cmd_buf[3] = (arg) ? 0xA0 : 0;
+		break;
+	case MHZ19B_SPAN_POINT_CMD:
+		put_unaligned_be16(arg, &cmd_buf[3]);
+		break;
+	default:
+		break;
+	}
+	cmd_buf[8] = mhz19b_get_checksum(cmd_buf);
+
+	/* Write buf to uart ctrl synchronously */
+	ret = serdev_device_write(serdev, cmd_buf, MHZ19B_CMD_SIZE, 0);
+	if (ret < 0)
+		return ret;
+	if (ret != MHZ19B_CMD_SIZE)
+		return -EIO;
+
+	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;
+
+		if (st->buf[8] != mhz19b_get_checksum(st->buf)) {
+			dev_err(dev, "checksum err");
+			return -EINVAL;
+		}
+
+		return get_unaligned_be16(&st->buf[2]);
+	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;
+
+	ret = mhz19b_serdev_cmd(indio_dev, MHZ19B_READ_CO2_CMD, 0);
+	if (ret < 0)
+		return ret;
+
+	*val = ret;
+	return IIO_VAL_INT;
+}
+
+/*
+ * echo 0 > calibration_auto_enable : ABC logic off
+ * echo 1 > calibration_auto_enable : ABC logic on
+ */
+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;
+
+	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);
+
+/*
+ * echo 0 > calibration_forced_value		 : zero point calibration
+ *	(make sure the sensor has been worked under 400ppm for over 20 minutes.)
+ * echo [1000 1 5000] > calibration_forced_value : span point calibration
+ *	(make sure the sensor has 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);
+	u16 ppm;
+	int cmd, ret;
+
+	ret = kstrtou16(buf, 0, &ppm);
+	if (ret)
+		return ret;
+
+	if (ppm) {
+		if (!in_range(ppm, 1000, 4001)) {
+			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);
+
+	memcpy(st->buf + st->buf_idx, data, len);
+	st->buf_idx += len;
+
+	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;
+
+	serdev_device_set_baudrate(serdev, 9600);
+
+	serdev_device_set_flow_control(serdev, false);
+
+	ret = serdev_device_set_parity(serdev, SERDEV_PARITY_NONE);
+	if (ret)
+		return ret;
+
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
+	if (!indio_dev)
+		return ret;
+	dev_set_drvdata(dev, indio_dev);
+
+	st = iio_priv(indio_dev);
+	st->serdev = serdev;
+
+	init_completion(&st->buf_ready);
+
+	ret = devm_regulator_get_enable(dev, "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 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_AUTHOR("Gyeyoung Baek");
+MODULE_DESCRIPTION("MH-Z19B CO2 sensor driver using serdev interface");
+MODULE_LICENSE("GPL");
-- 
2.34.1

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

[Andy Shevchenko]

On Sun, Apr 20, 2025 at 9:10 PM Gyeyoung Baek <gye976@gmail.com> wrote:
>
> Add support for winsen MHZ19B CO2 sensor.

> The datasheet is available at
> Link: https://www.winsen-sensor.com/d/files/infrared-gas-sensor/mh-z19b-co2-ver1_0.pdf

Instead, just make it Datasheet: tag.

>

Should not be this blank line here.

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

...

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

Preserve order.

...

> +/*
> + * mh-z19b co2 sensor driver

Please, use the marketing name of the chip and we can spell CO₂.

> + * 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/completion.h>
> +#include <linux/device.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/serdev.h>
> +#include <linux/unaligned.h>

Semi-baked list, see below what's missing (actually a lot). I believe
I already pointed that out.

...

> +#define MHZ19B_SERDEV_TIMEOUT msecs_to_jiffies(100)

Missing header for msec_to_jiffies().

...

> +       u8 buf_idx;

Needs types.h

...

> +       switch (cmd) {
> +       case MHZ19B_ABC_LOGIC_CMD:
> +               cmd_buf[3] = (arg) ? 0xA0 : 0;

Unneeded parentheses.

> +               break;
> +       case MHZ19B_SPAN_POINT_CMD:
> +               put_unaligned_be16(arg, &cmd_buf[3]);
> +               break;
> +       default:
> +               break;
> +       }

...

> +               if (st->buf[8] != mhz19b_get_checksum(st->buf)) {
> +                       dev_err(dev, "checksum err");
> +                       return -EINVAL;

Needs errno.h

> +               }

...

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

It's defined in kstrtox.h.

> +       if (ret)
> +               return ret;

...

> +/*
> + * echo 0 > calibration_forced_value            : zero point calibration
> + *     (make sure the sensor has been worked under 400ppm for over 20 minutes.)

working

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

working

It seems you ignored this comment from the previous review.

> + */

...

> +       if (ppm) {
> +               if (!in_range(ppm, 1000, 4001)) {

Missing minmax.h.

The second parameter is length of the range.

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

The above range check doesn't agree with this message.

> +                       return -EINVAL;
> +               }
> +
> +               cmd = MHZ19B_SPAN_POINT_CMD;
> +       } else
> +               cmd = MHZ19B_ZERO_POINT_CMD;

Have you run checkpatch? This needs {} per Coding Style.

...

> +       memcpy(st->buf + st->buf_idx, data, len);

Requires string.h.

...

> +       serdev_device_set_baudrate(serdev, 9600);

> +

Redundant blank line.

> +       serdev_device_set_flow_control(serdev, false);

...

> +       dev_set_drvdata(dev, indio_dev);

Is it really used?

...

> +       indio_dev->num_channels = ARRAY_SIZE(mhz19b_channels);

Needs array_size.h.

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Sun, Apr 20, 2025 at 10:21 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Sun, Apr 20, 2025 at 9:10 PM Gyeyoung Baek <gye976@gmail.com> wrote:

...

> > --- a/drivers/iio/chemical/Makefile
> > +++ b/drivers/iio/chemical/Makefile

> >  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
>
> Preserve order.

Ah, I see it's ordered but by Kconfig, Why do you have WINSEN in the
option and no such thing in the filename? I would drop that from the
config option.

Jonathan, what do you think about this? Which order (in case of
misalignment between module name and configuration option) should be
taken?

-- 
With Best Regards,
Andy Shevchenko

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

[Jonathan Cameron]

On Sun, 20 Apr 2025 22:23:57 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Sun, Apr 20, 2025 at 10:21 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > On Sun, Apr 20, 2025 at 9:10 PM Gyeyoung Baek <gye976@gmail.com> wrote:  
> 
> ...
> 
> > > --- a/drivers/iio/chemical/Makefile
> > > +++ b/drivers/iio/chemical/Makefile  
> 
> > >  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  
> >
> > Preserve order.  
> 
> Ah, I see it's ordered but by Kconfig, Why do you have WINSEN in the
> option and no such thing in the filename? I would drop that from the
> config option.
> 
> Jonathan, what do you think about this? Which order (in case of
> misalignment between module name and configuration option) should be
> taken?
> 

Hmm.. No idea.  It think just dropping it from the CONFIG name
is the right way to avoid the question ;)

Jonathan

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

[Gyeyoung Baek]

Hello Jonathan,

On Mon, Apr 21, 2025 at 10:25 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Sun, 20 Apr 2025 22:23:57 +0300
> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>
> > On Sun, Apr 20, 2025 at 10:21 PM Andy Shevchenko
> > <andy.shevchenko@gmail.com> wrote:
> > > On Sun, Apr 20, 2025 at 9:10 PM Gyeyoung Baek <gye976@gmail.com> wrote:
> >
> > ...
> >
> > > > --- a/drivers/iio/chemical/Makefile
> > > > +++ b/drivers/iio/chemical/Makefile
> >
> > > >  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
> > >
> > > Preserve order.
> >
> > Ah, I see it's ordered but by Kconfig, Why do you have WINSEN in the
> > option and no such thing in the filename? I would drop that from the
> > config option.
> >
> > Jonathan, what do you think about this? Which order (in case of
> > misalignment between module name and configuration option) should be
> > taken?
> >
>
> Hmm.. No idea.  It think just dropping it from the CONFIG name
> is the right way to avoid the question ;)

Thanks for your suggestion. I'll just drop that.

Thanks,
Gyeyoung

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

[Gyeyoung Baek]

Hello Andy,

On Mon, Apr 21, 2025 at 4:24 AM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Sun, Apr 20, 2025 at 10:21 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > On Sun, Apr 20, 2025 at 9:10 PM Gyeyoung Baek <gye976@gmail.com> wrote:
>
> ...
>
> > > --- a/drivers/iio/chemical/Makefile
> > > +++ b/drivers/iio/chemical/Makefile
>
> > >  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
> >
> > Preserve order.
>
> Ah, I see it's ordered but by Kconfig, Why do you have WINSEN in the
> option and no such thing in the filename? I would drop that from the
> config option.

I followed the 'vendor_device' naming pattern seen in examples like
'CONFIG_SENSEAIR_SUNRISE_CO2'
But I'll drop the vendor prefix in the next patch, thanks.

--
Regards,
Gyeyoung

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

[Andy Shevchenko]

On Tue, Apr 22, 2025 at 05:38:56PM +0900, Gyeyoung Baek wrote:
> On Mon, Apr 21, 2025 at 4:24 AM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > On Sun, Apr 20, 2025 at 10:21 PM Andy Shevchenko
> > <andy.shevchenko@gmail.com> wrote:
> > > On Sun, Apr 20, 2025 at 9:10 PM Gyeyoung Baek <gye976@gmail.com> wrote:

...

> > > > --- a/drivers/iio/chemical/Makefile
> > > > +++ b/drivers/iio/chemical/Makefile
> >
> > > >  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
> > >
> > > Preserve order.
> >
> > Ah, I see it's ordered but by Kconfig, Why do you have WINSEN in the
> > option and no such thing in the filename? I would drop that from the
> > config option.
> 
> I followed the 'vendor_device' naming pattern seen in examples like
> 'CONFIG_SENSEAIR_SUNRISE_CO2'

Interesting... Now I understand why the above was pulled without any comment,
because the vendor and device names start with the same letter 'S'!

> But I'll drop the vendor prefix in the next patch, thanks.

-- 
With Best Regards,
Andy Shevchenko

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

[Jonathan Cameron]

On Tue, 22 Apr 2025 20:30:58 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Tue, Apr 22, 2025 at 05:38:56PM +0900, Gyeyoung Baek wrote:
> > On Mon, Apr 21, 2025 at 4:24 AM Andy Shevchenko
> > <andy.shevchenko@gmail.com> wrote:  
> > > On Sun, Apr 20, 2025 at 10:21 PM Andy Shevchenko
> > > <andy.shevchenko@gmail.com> wrote:  
> > > > On Sun, Apr 20, 2025 at 9:10 PM Gyeyoung Baek <gye976@gmail.com> wrote:  
> 
> ...
> 
> > > > > --- a/drivers/iio/chemical/Makefile
> > > > > +++ b/drivers/iio/chemical/Makefile  
> > >  
> > > > >  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  
> > > >
> > > > Preserve order.  
> > >
> > > Ah, I see it's ordered but by Kconfig, Why do you have WINSEN in the
> > > option and no such thing in the filename? I would drop that from the
> > > config option.  
> > 
> > I followed the 'vendor_device' naming pattern seen in examples like
> > 'CONFIG_SENSEAIR_SUNRISE_CO2'  
> 
> Interesting... Now I understand why the above was pulled without any comment,
> because the vendor and device names start with the same letter 'S'!

These vendor + driver things originally (IIRC) snuck in with driver
moves from misc etc a long time ago and I've never really enforced having
them or not properly since then :(

I prefer not having the company in these but it is a fairly random
choice with little real justification either way.

Jonathan

> 
> > But I'll drop the vendor prefix in the next patch, thanks.  
> 

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

[Gyeyoung Baek]

Hello Andy, thanks for the review.

On Mon, Apr 21, 2025 at 4:22 AM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Sun, Apr 20, 2025 at 9:10 PM Gyeyoung Baek <gye976@gmail.com> wrote:
> >
> > Add support for winsen MHZ19B CO2 sensor.
>
> > The datasheet is available at
> > Link: https://www.winsen-sensor.com/d/files/infrared-gas-sensor/mh-z19b-co2-ver1_0.pdf
>
> Instead, just make it Datasheet: tag.
>
> >
>
> Should not be this blank line here.
> > Signed-off-by: Gyeyoung Baek <gye976@gmail.com>
>

Sorry, I now understand what you mean.
('Datasheet:' is a tag, not an explanation. So there should be no
space between the tags.)
However, when I use the 'Datasheet:' tag, I get the following warning
by checkpatch.pl.
Would it be OK? then I'll use 'Datasheet:' tag.

    "WARNING: Unknown link reference 'Datasheet:', use 'Link:' or
'Closes:' instead
     #8:
     Datasheet:
https://www.winsen-sensor.com/d/files/infrared-gas-sensor/mh-z19b-co2-ver1_0.pdf"

---

> > +#include <linux/completion.h>
> > +#include <linux/device.h>
> > +#include <linux/iio/iio.h>
> > +#include <linux/iio/sysfs.h>
> > +#include <linux/mod_devicetable.h>
> > +#include <linux/module.h>
> > +#include <linux/regulator/consumer.h>
> > +#include <linux/serdev.h>
> > +#include <linux/unaligned.h>
>
> Semi-baked list, see below what's missing (actually a lot). I believe
> I already pointed that out.

From the last review, I understood the two main points as:
(1) Avoid using large headers and explicitly include only the necessary ones.
(2) Reduce including headers as much as possible to improve build times.

However, I found that removing headers like 'types.h' didn't cause any
build failure,
So I thought it would be better to remove headers that are unnecessary
to improve build time.
But in the next patch, I'll explicitly include the headers you've pointed out.

---

> > +/*
> > + * echo 0 > calibration_forced_value            : zero point calibration
> > + *     (make sure the sensor has been worked under 400ppm for over 20 minutes.)
>
> working
>
> > + * echo [1000 1 5000] > calibration_forced_value : span point calibration
> > + *     (make sure the sensor has been worked under a certain level co2 for over 20 minutes.)
>
> working
>
> It seems you ignored this comment from the previous review.

Sorry, I only changed 'had -> has'.
I missed changing 'worked -> working' ..

---

> > +       if (ppm) {
> > +               if (!in_range(ppm, 1000, 4001)) {
>
> Missing minmax.h.
>
> The second parameter is length of the range.
>
> > +                       dev_dbg(&indio_dev->dev,
> > +                               "span point ppm should be 1000~5000");
>
> The above range check doesn't agree with this message.

in_range() uses the range 'start <= val && val < (start + len)'.
So, to allow 5000, the 'len' should be 4001, not 4000.
I have tested this.
But would it be correct that I understand your point?

---

> > +                       return -EINVAL;
> > +               }
> > +
> > +               cmd = MHZ19B_SPAN_POINT_CMD;
> > +       } else
> > +               cmd = MHZ19B_ZERO_POINT_CMD;
>
> Have you run checkpatch? This needs {} per Coding Style.

Yes, checkpatch.pl didn't raise any warnings.
And after making the changes as you point out, there were no warnings either.
It seems that checkpatch.pl allows both.
But for consistency, I'll add the { }.

---

> > +       dev_set_drvdata(dev, indio_dev);
>
> Is it really used?

Yes, this 'dev' is the same as '&serdev->dev'.
But I found 'serdev_device_set_drvdata()', so I'll change it to that.

--
Thanks,
Gyeyoung

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

[Andy Shevchenko]

On Mon, Apr 21, 2025 at 03:41:58PM +0900, Gyeyoung Baek wrote:
> On Mon, Apr 21, 2025 at 4:22 AM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > On Sun, Apr 20, 2025 at 9:10 PM Gyeyoung Baek <gye976@gmail.com> wrote:

...

> > > The datasheet is available at
> > > Link: https://www.winsen-sensor.com/d/files/infrared-gas-sensor/mh-z19b-co2-ver1_0.pdf
> >
> > Instead, just make it Datasheet: tag.

> > >

> > Should not be this blank line here.
> > > Signed-off-by: Gyeyoung Baek <gye976@gmail.com>
> 
> Sorry, I now understand what you mean.
> ('Datasheet:' is a tag, not an explanation. So there should be no
> space between the tags.)
> However, when I use the 'Datasheet:' tag, I get the following warning
> by checkpatch.pl.
> Would it be OK? then I'll use 'Datasheet:' tag.
> 
>     "WARNING: Unknown link reference 'Datasheet:', use 'Link:' or
> 'Closes:' instead
>      #8:
>      Datasheet:
> https://www.winsen-sensor.com/d/files/infrared-gas-sensor/mh-z19b-co2-ver1_0.pdf"

checkpatch is recommendation and sometimes has not fully suitable warnings. You
may ignore this one.

...

> > > +#include <linux/completion.h>
> > > +#include <linux/device.h>
> > > +#include <linux/iio/iio.h>
> > > +#include <linux/iio/sysfs.h>
> > > +#include <linux/mod_devicetable.h>
> > > +#include <linux/module.h>
> > > +#include <linux/regulator/consumer.h>
> > > +#include <linux/serdev.h>
> > > +#include <linux/unaligned.h>
> >
> > Semi-baked list, see below what's missing (actually a lot). I believe
> > I already pointed that out.
> 
> From the last review, I understood the two main points as:
> (1) Avoid using large headers and explicitly include only the necessary ones.
> (2) Reduce including headers as much as possible to improve build times.
> 
> However, I found that removing headers like 'types.h' didn't cause any
> build failure,

Because we have other headers that may use it, but you shouldn't do that.

> So I thought it would be better to remove headers that are unnecessary
> to improve build time.
> But in the next patch, I'll explicitly include the headers you've pointed out.

You should follow IWYU principle, i.e. Include What You Use, e.g., types.h.

...

> > > +       if (ppm) {
> > > +               if (!in_range(ppm, 1000, 4001)) {
> >
> > Missing minmax.h.
> >
> > The second parameter is length of the range.
> >
> > > +                       dev_dbg(&indio_dev->dev,
> > > +                               "span point ppm should be 1000~5000");
> >
> > The above range check doesn't agree with this message.
> 
> in_range() uses the range 'start <= val && val < (start + len)'.
> So, to allow 5000, the 'len' should be 4001, not 4000.
> I have tested this.
> But would it be correct that I understand your point?

I see now, I recommend then to define those two:

#define ..._PPM_MIN	1000
#define ..._PPM_MAX	5000

And use here as

               if (!in_range(ppm, ..._MIN, ..._MAX - ..._MIN + 1)) {
                       dev_dbg(&indio_dev->dev,
                               "span point PPM should be in a range [%d-%d]\n",
			       _MIN, _MAX);

Note, you missed \n as well.

> > > +                       return -EINVAL;
> > > +               }

-- 
With Best Regards,
Andy Shevchenko

[PATCH v4 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 01079a189c93..4a0089db6670 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -26015,6 +26015,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