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

[PATCH v1 0/5] 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

v1:
 - 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 (5):
  dt-bindings: add winsen to the vendor prefixes
  dt-bindings: add device tree support for winsen MHZ19B CO2 sensor
  ABI: iio: add new ABI doc for mhz19b
  iio: chemical: add support for winsen MHZ19B CO2 sensor
  MAINTAINERS: Add WINSEN MHZ19B

 .../ABI/testing/sysfs-bus-iio-chemical-mhz19b |   7 +
 .../bindings/iio/chemical/winsen,mhz19b.yaml  |  33 ++
 .../devicetree/bindings/vendor-prefixes.yaml  |   2 +
 MAINTAINERS                                   |   7 +
 drivers/iio/chemical/Kconfig                  |  10 +
 drivers/iio/chemical/Makefile                 |   1 +
 drivers/iio/chemical/mhz19b.c                 | 386 ++++++++++++++++++
 7 files changed, 446 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-chemical-mhz19b
 create mode 100644 Documentation/devicetree/bindings/iio/chemical/winsen,mhz19b.yaml
 create mode 100644 drivers/iio/chemical/mhz19b.c

--
2.34.1

[PATCH v1 1/5] 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 v1 1/5] dt-bindings: add winsen to the vendor prefixes

[Krzysztof Kozlowski]

On 03/04/2025 07:32, Gyeyoung Baek wrote:
> Add winsen to the vendor prefixes.
> 
> Signed-off-by: Gyeyoung Baek <gye976@gmail.com>

I am pretty sure you sent v1, so this is v2. And if this is anyhow
confusing, then just use standard tools - b4 - which would do this right.


Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>


---

<form letter>
This is an automated instruction, just in case, because many review tags
are being ignored. If you know the process, you can skip it (please do
not feel offended by me posting it here - no bad intentions intended).
If you do not know the process, here is a short explanation:

Please add Acked-by/Reviewed-by/Tested-by tags when posting new versions
of patchset, under or above your Signed-off-by tag, unless patch changed
significantly (e.g. new properties added to the DT bindings). Tag is
"received", when provided in a message replied to you on the mailing
list. Tools like b4 can help here. However, there's no need to repost
patches *only* to add the tags. The upstream maintainer will do that for
tags received on the version they apply.

Full context and explanation:
https://elixir.bootlin.com/linux/v6.12-rc3/source/Documentation/process/submitting-patches.rst#L577
</form letter>


Best regards,
Krzysztof

Re: [PATCH v1 1/5] dt-bindings: add winsen to the vendor prefixes

[gyeyoung]

Hello Krzysztof, thank you for the review.

On Thu, Apr 3, 2025 at 4:48 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On 03/04/2025 07:32, Gyeyoung Baek wrote:
> > Add winsen to the vendor prefixes.
> >
> > Signed-off-by: Gyeyoung Baek <gye976@gmail.com>
>
> I am pretty sure you sent v1, so this is v2. And if this is anyhow
> confusing, then just use standard tools - b4 - which would do this right.
>
Yes, actually this is v2. I did it manually, so there was a mistake.
Thank you for the tips.

Thanks,
Gyeyoung Baek

[PATCH v1 2/5] 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  | 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..16ec902c906e
--- /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

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

[Krzysztof Kozlowski]

On 03/04/2025 07:32, Gyeyoung Baek wrote:
> Add device tree support for winsen MHZ19B sensor.
> 
> Signed-off-by: Gyeyoung Baek <gye976@gmail.com>
> ---

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>


---

<form letter>
This is an automated instruction, just in case, because many review tags
are being ignored. If you know the process, you can skip it (please do
not feel offended by me posting it here - no bad intentions intended).
If you do not know the process, here is a short explanation:

Please add Acked-by/Reviewed-by/Tested-by tags when posting new versions
of patchset, under or above your Signed-off-by tag, unless patch changed
significantly (e.g. new properties added to the DT bindings). Tag is
"received", when provided in a message replied to you on the mailing
list. Tools like b4 can help here. However, there's no need to repost
patches *only* to add the tags. The upstream maintainer will do that for
tags received on the version they apply.

Full context and explanation:
https://elixir.bootlin.com/linux/v6.12-rc3/source/Documentation/process/submitting-patches.rst#L577
</form letter>

Best regards,
Krzysztof

[PATCH v1 3/5] ABI: iio: add new ABI doc for mhz19b

[Gyeyoung Baek]

Add support for winsen MHZ19B CO2 sensor.

Signed-off-by: Gyeyoung Baek <gye976@gmail.com>
---
 Documentation/ABI/testing/sysfs-bus-iio-chemical-mhz19b | 7 +++++++
 1 file changed, 7 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-chemical-mhz19b

diff --git a/Documentation/ABI/testing/sysfs-bus-iio-chemical-mhz19b b/Documentation/ABI/testing/sysfs-bus-iio-chemical-mhz19b
new file mode 100644
index 000000000000..6cdfd34be016
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-bus-iio-chemical-mhz19b
@@ -0,0 +1,7 @@
+What:		/sys/bus/iio/devices/co2_range
+Date:		April 2025
+KernelVersion:	6.14
+Contact:	Gyeyoung Baek <gye976@gmail.com>
+Description:
+		Writing a value adjust maximum measurable PPM.
+		should be 2000 or 5000.
--
2.34.1

Re: [PATCH v1 3/5] ABI: iio: add new ABI doc for mhz19b

[Krzysztof Kozlowski]

On 03/04/2025 07:32, Gyeyoung Baek wrote:
> Add support for winsen MHZ19B CO2 sensor.
> 
> Signed-off-by: Gyeyoung Baek <gye976@gmail.com>
> ---
>  Documentation/ABI/testing/sysfs-bus-iio-chemical-mhz19b | 7 +++++++
>  1 file changed, 7 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-chemical-mhz19b
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-iio-chemical-mhz19b b/Documentation/ABI/testing/sysfs-bus-iio-chemical-mhz19b
> new file mode 100644
> index 000000000000..6cdfd34be016
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-bus-iio-chemical-mhz19b
> @@ -0,0 +1,7 @@
> +What:		/sys/bus/iio/devices/co2_range

Incomplete path

> +Date:		April 2025

Not possible. This will be probably June.

> +KernelVersion:	6.14

No way you can put ABI into version already released. It's impossible.
That's v6.16.

> +
Best regards,
Krzysztof

Re: [PATCH v1 3/5] ABI: iio: add new ABI doc for mhz19b

[gyeyoung]

On Thu, Apr 3, 2025 at 4:51 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On 03/04/2025 07:32, Gyeyoung Baek wrote:
> > Add support for winsen MHZ19B CO2 sensor.
> >
> > Signed-off-by: Gyeyoung Baek <gye976@gmail.com>
> > ---
> >  Documentation/ABI/testing/sysfs-bus-iio-chemical-mhz19b | 7 +++++++
> >  1 file changed, 7 insertions(+)
> >  create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-chemical-mhz19b
> >
> > diff --git a/Documentation/ABI/testing/sysfs-bus-iio-chemical-mhz19b b/Documentation/ABI/testing/sysfs-bus-iio-chemical-mhz19b
> > new file mode 100644
> > index 000000000000..6cdfd34be016
> > --- /dev/null
> > +++ b/Documentation/ABI/testing/sysfs-bus-iio-chemical-mhz19b
> > @@ -0,0 +1,7 @@
> > +What:                /sys/bus/iio/devices/co2_range
>
> Incomplete path
>
> > +Date:                April 2025
>
> Not possible. This will be probably June.
>
> > +KernelVersion:       6.14
>
> No way you can put ABI into version already released. It's impossible.
> That's v6.16.

sorry, I'll read the kernel doc more carefully.

Thanks,
Gyeyoung

Re: [PATCH v1 3/5] ABI: iio: add new ABI doc for mhz19b

[Jonathan Cameron]

On Thu,  3 Apr 2025 14:32:23 +0900
Gyeyoung Baek <gye976@gmail.com> wrote:

> Add support for winsen MHZ19B CO2 sensor.
> 
> Signed-off-by: Gyeyoung Baek <gye976@gmail.com>
> ---
>  Documentation/ABI/testing/sysfs-bus-iio-chemical-mhz19b | 7 +++++++
>  1 file changed, 7 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-chemical-mhz19b
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-iio-chemical-mhz19b b/Documentation/ABI/testing/sysfs-bus-iio-chemical-mhz19b
> new file mode 100644
> index 000000000000..6cdfd34be016
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-bus-iio-chemical-mhz19b
> @@ -0,0 +1,7 @@
> +What:		/sys/bus/iio/devices/co2_range
> +Date:		April 2025
> +KernelVersion:	6.14
> +Contact:	Gyeyoung Baek <gye976@gmail.com>
> +Description:
> +		Writing a value adjust maximum measurable PPM.
> +		should be 2000 or 5000.

I haven't checked but assume this also results in a scaling of the
measure _raw values?  If so the control should be via the standard
ABI scale.  If you need to be able to establish the range, provide
the _available for the _raw via the read_avail() callback and setting
appropriate bit in info_mask_separate_available

General rule is don't introduce new ABI unless it is impossible to
provide the same information via existing interfaces.  The decision
to use scale rather than range info to control channel scaling was
made a very long time ago and having a mixture of the two would
make for very complex userspace code.

Jonathan

> --
> 2.34.1
> 
> 

Re: [PATCH v1 3/5] ABI: iio: add new ABI doc for mhz19b

[gyeyoung]

Hello Jonathan, thank you for the review.

> > diff --git a/Documentation/ABI/testing/sysfs-bus-iio-chemical-mhz19b b/Documentation/ABI/testing/sysfs-bus-iio-chemical-mhz19b
> > new file mode 100644
> > index 000000000000..6cdfd34be016
> > --- /dev/null
> > +++ b/Documentation/ABI/testing/sysfs-bus-iio-chemical-mhz19b
> > @@ -0,0 +1,7 @@
> > +What:                /sys/bus/iio/devices/co2_range
> > +Date:                April 2025
> > +KernelVersion:       6.14
> > +Contact:     Gyeyoung Baek <gye976@gmail.com>
> > +Description:
> > +             Writing a value adjust maximum measurable PPM.
> > +             should be 2000 or 5000.
>
> I haven't checked but assume this also results in a scaling of the
> measure _raw values?  If so the control should be via the standard
> ABI scale.  If you need to be able to establish the range, provide
> the _available for the _raw via the read_avail() callback and setting
> appropriate bit in info_mask_separate_available
>

In this device, changing the measurement range does not affect the
unit or scaling.
As far as I know, increasing the range just leads to a decrease in accuracy.

> General rule is don't introduce new ABI unless it is impossible to
> provide the same information via existing interfaces.  The decision
> to use scale rather than range info to control channel scaling was
> made a very long time ago and having a mixture of the two would
> make for very complex userspace code.

I’ve reviewed the sysfs-bus-iio documentation, I think there is
no suitable interface for this case. So I'll drop this option.

Thanks,
Gyeyoung

Re: [PATCH v1 3/5] ABI: iio: add new ABI doc for mhz19b

[Jonathan Cameron]

On Sat, 5 Apr 2025 22:47:45 +0900
gyeyoung <gye976@gmail.com> wrote:

> Hello Jonathan, thank you for the review.
> 
> > > diff --git a/Documentation/ABI/testing/sysfs-bus-iio-chemical-mhz19b b/Documentation/ABI/testing/sysfs-bus-iio-chemical-mhz19b
> > > new file mode 100644
> > > index 000000000000..6cdfd34be016
> > > --- /dev/null
> > > +++ b/Documentation/ABI/testing/sysfs-bus-iio-chemical-mhz19b
> > > @@ -0,0 +1,7 @@
> > > +What:                /sys/bus/iio/devices/co2_range
> > > +Date:                April 2025
> > > +KernelVersion:       6.14
> > > +Contact:     Gyeyoung Baek <gye976@gmail.com>
> > > +Description:
> > > +             Writing a value adjust maximum measurable PPM.
> > > +             should be 2000 or 5000.  
> >
> > I haven't checked but assume this also results in a scaling of the
> > measure _raw values?  If so the control should be via the standard
> > ABI scale.  If you need to be able to establish the range, provide
> > the _available for the _raw via the read_avail() callback and setting
> > appropriate bit in info_mask_separate_available
> >  
> 
> In this device, changing the measurement range does not affect the
> unit or scaling.
> As far as I know, increasing the range just leads to a decrease in accuracy.

That's unusual but fair enough.  hardwaregain is perhaps appropriate
as this doesn't really map to calibscale which is the other thing close
to this.

> 
> > General rule is don't introduce new ABI unless it is impossible to
> > provide the same information via existing interfaces.  The decision
> > to use scale rather than range info to control channel scaling was
> > made a very long time ago and having a mixture of the two would
> > make for very complex userspace code.  
> 
> I’ve reviewed the sysfs-bus-iio documentation, I think there is
> no suitable interface for this case. So I'll drop this option.
> 
> Thanks,
> Gyeyoung

Re: [PATCH v1 3/5] ABI: iio: add new ABI doc for mhz19b

[Jonathan Cameron]

On Sun, 6 Apr 2025 12:20:48 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

> On Sat, 5 Apr 2025 22:47:45 +0900
> gyeyoung <gye976@gmail.com> wrote:
> 
> > Hello Jonathan, thank you for the review.
> >   
> > > > diff --git a/Documentation/ABI/testing/sysfs-bus-iio-chemical-mhz19b b/Documentation/ABI/testing/sysfs-bus-iio-chemical-mhz19b
> > > > new file mode 100644
> > > > index 000000000000..6cdfd34be016
> > > > --- /dev/null
> > > > +++ b/Documentation/ABI/testing/sysfs-bus-iio-chemical-mhz19b
> > > > @@ -0,0 +1,7 @@
> > > > +What:                /sys/bus/iio/devices/co2_range
> > > > +Date:                April 2025
> > > > +KernelVersion:       6.14
> > > > +Contact:     Gyeyoung Baek <gye976@gmail.com>
> > > > +Description:
> > > > +             Writing a value adjust maximum measurable PPM.
> > > > +             should be 2000 or 5000.    
> > >
> > > I haven't checked but assume this also results in a scaling of the
> > > measure _raw values?  If so the control should be via the standard
> > > ABI scale.  If you need to be able to establish the range, provide
> > > the _available for the _raw via the read_avail() callback and setting
> > > appropriate bit in info_mask_separate_available
> > >    
> > 
> > In this device, changing the measurement range does not affect the
> > unit or scaling.
> > As far as I know, increasing the range just leads to a decrease in accuracy.  
> 
> That's unusual but fair enough.  hardwaregain is perhaps appropriate
> as this doesn't really map to calibscale which is the other thing close
> to this.
> 
Actually - any idea what the gain is doing?  Is it adjusting a analog
amplfier, or messing with the integration time (I have little idea
how these sensors work!) 

Jonathan


> >   
> > > General rule is don't introduce new ABI unless it is impossible to
> > > provide the same information via existing interfaces.  The decision
> > > to use scale rather than range info to control channel scaling was
> > > made a very long time ago and having a mixture of the two would
> > > make for very complex userspace code.    
> > 
> > I’ve reviewed the sysfs-bus-iio documentation, I think there is
> > no suitable interface for this case. So I'll drop this option.
> > 
> > Thanks,
> > Gyeyoung  
> 

Re: [PATCH v1 3/5] ABI: iio: add new ABI doc for mhz19b

[gyeyoung]

Hello Jonathan,

On Sun, Apr 6, 2025 at 8:26 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Sun, 6 Apr 2025 12:20:48 +0100
> Jonathan Cameron <jic23@kernel.org> wrote:
>
> > On Sat, 5 Apr 2025 22:47:45 +0900
> > gyeyoung <gye976@gmail.com> wrote:
> >
> > > Hello Jonathan, thank you for the review.
> > >
> > > > > diff --git a/Documentation/ABI/testing/sysfs-bus-iio-chemical-mhz19b b/Documentation/ABI/testing/sysfs-bus-iio-chemical-mhz19b
> > > > > new file mode 100644
> > > > > index 000000000000..6cdfd34be016
> > > > > --- /dev/null
> > > > > +++ b/Documentation/ABI/testing/sysfs-bus-iio-chemical-mhz19b
> > > > > @@ -0,0 +1,7 @@
> > > > > +What:                /sys/bus/iio/devices/co2_range
> > > > > +Date:                April 2025
> > > > > +KernelVersion:       6.14
> > > > > +Contact:     Gyeyoung Baek <gye976@gmail.com>
> > > > > +Description:
> > > > > +             Writing a value adjust maximum measurable PPM.
> > > > > +             should be 2000 or 5000.
> > > >
> > > > I haven't checked but assume this also results in a scaling of the
> > > > measure _raw values?  If so the control should be via the standard
> > > > ABI scale.  If you need to be able to establish the range, provide
> > > > the _available for the _raw via the read_avail() callback and setting
> > > > appropriate bit in info_mask_separate_available
> > > >
> > >
> > > In this device, changing the measurement range does not affect the
> > > unit or scaling.
> > > As far as I know, increasing the range just leads to a decrease in accuracy.
> >
> > That's unusual but fair enough.  hardwaregain is perhaps appropriate
> > as this doesn't really map to calibscale which is the other thing close
> > to this.
> >
> Actually - any idea what the gain is doing?  Is it adjusting a analog
> amplfier, or messing with the integration time (I have little idea
> how these sensors work!)
>
sorry, I'm not affiliated with the vendor,
so I do not have any information other than the datasheet.
And the datasheet doesn't specify any trade-offs related to adjusting the range.
(only "± (50ppm+3% reading value)" is here)

I think this range setting would be better implemented in
some other way (like module parameter) rather than IIO subsystem.
For now I'll just drop this range setting code..

Regards,
Gyeyoung

Re: [PATCH v1 3/5] ABI: iio: add new ABI doc for mhz19b

[Jonathan Cameron]

On Sun, 6 Apr 2025 22:31:43 +0900
gyeyoung <gye976@gmail.com> wrote:

> Hello Jonathan,
> 
> On Sun, Apr 6, 2025 at 8:26 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Sun, 6 Apr 2025 12:20:48 +0100
> > Jonathan Cameron <jic23@kernel.org> wrote:
> >  
> > > On Sat, 5 Apr 2025 22:47:45 +0900
> > > gyeyoung <gye976@gmail.com> wrote:
> > >  
> > > > Hello Jonathan, thank you for the review.
> > > >  
> > > > > > diff --git a/Documentation/ABI/testing/sysfs-bus-iio-chemical-mhz19b b/Documentation/ABI/testing/sysfs-bus-iio-chemical-mhz19b
> > > > > > new file mode 100644
> > > > > > index 000000000000..6cdfd34be016
> > > > > > --- /dev/null
> > > > > > +++ b/Documentation/ABI/testing/sysfs-bus-iio-chemical-mhz19b
> > > > > > @@ -0,0 +1,7 @@
> > > > > > +What:                /sys/bus/iio/devices/co2_range
> > > > > > +Date:                April 2025
> > > > > > +KernelVersion:       6.14
> > > > > > +Contact:     Gyeyoung Baek <gye976@gmail.com>
> > > > > > +Description:
> > > > > > +             Writing a value adjust maximum measurable PPM.
> > > > > > +             should be 2000 or 5000.  
> > > > >
> > > > > I haven't checked but assume this also results in a scaling of the
> > > > > measure _raw values?  If so the control should be via the standard
> > > > > ABI scale.  If you need to be able to establish the range, provide
> > > > > the _available for the _raw via the read_avail() callback and setting
> > > > > appropriate bit in info_mask_separate_available
> > > > >  
> > > >
> > > > In this device, changing the measurement range does not affect the
> > > > unit or scaling.
> > > > As far as I know, increasing the range just leads to a decrease in accuracy.  
> > >
> > > That's unusual but fair enough.  hardwaregain is perhaps appropriate
> > > as this doesn't really map to calibscale which is the other thing close
> > > to this.
> > >  
> > Actually - any idea what the gain is doing?  Is it adjusting a analog
> > amplfier, or messing with the integration time (I have little idea
> > how these sensors work!)
> >  
> sorry, I'm not affiliated with the vendor,
> so I do not have any information other than the datasheet.
> And the datasheet doesn't specify any trade-offs related to adjusting the range.
> (only "± (50ppm+3% reading value)" is here)
> 
> I think this range setting would be better implemented in
> some other way (like module parameter) rather than IIO subsystem.

I'm not in general keen on module parameters effecting policy (and noise
vs range is definitely a policy thing) so I think we would want to figure out
a suitable sysfs ABI.

> For now I'll just drop this range setting code..
Makes sense  - can revisit when needed.
> 
> Regards,
> Gyeyoung

Re: [PATCH v1 3/5] ABI: iio: add new ABI doc for mhz19b

[gyeyoung]

Hello Jonathan,


> > > Actually - any idea what the gain is doing?  Is it adjusting a analog
> > > amplfier, or messing with the integration time (I have little idea
> > > how these sensors work!)
> > >
> > sorry, I'm not affiliated with the vendor,
> > so I do not have any information other than the datasheet.
> > And the datasheet doesn't specify any trade-offs related to adjusting the range.
> > (only "± (50ppm+3% reading value)" is here)
> >
> > I think this range setting would be better implemented in
> > some other way (like module parameter) rather than IIO subsystem.
>
> I'm not in general keen on module parameters effecting policy (and noise
> vs range is definitely a policy thing) so I think we would want to figure out
> a suitable sysfs ABI.

Or, how about adding IIO_CHAN_INFO_RANGE to the
iio_chan_info_postfix[] array for range setting?
I think there might be examples of devices that expand the range
without affecting scaling,
like this device (though I haven’t found a case yet...)

If this is a valid approach, would it be okay if I submit a patch for it?

Regards,
Gyeyoung

Re: [PATCH v1 3/5] ABI: iio: add new ABI doc for mhz19b

[Jonathan Cameron]

On Mon, 7 Apr 2025 11:10:21 +0900
gyeyoung <gye976@gmail.com> wrote:

> Hello Jonathan,
> 
> 
> > > > Actually - any idea what the gain is doing?  Is it adjusting a analog
> > > > amplfier, or messing with the integration time (I have little idea
> > > > how these sensors work!)
> > > >  
> > > sorry, I'm not affiliated with the vendor,
> > > so I do not have any information other than the datasheet.
> > > And the datasheet doesn't specify any trade-offs related to adjusting the range.
> > > (only "± (50ppm+3% reading value)" is here)
> > >
> > > I think this range setting would be better implemented in
> > > some other way (like module parameter) rather than IIO subsystem.  
> >
> > I'm not in general keen on module parameters effecting policy (and noise
> > vs range is definitely a policy thing) so I think we would want to figure out
> > a suitable sysfs ABI.  
> 
> Or, how about adding IIO_CHAN_INFO_RANGE to the
> iio_chan_info_postfix[] array for range setting?
> I think there might be examples of devices that expand the range
> without affecting scaling,
> like this device (though I haven’t found a case yet...)
> 
> If this is a valid approach, would it be okay if I submit a patch for it?

No. The reason isn't technical, but rather that if we provide that
we'll spend years explaining to people that _scale is the right
choice if the _scale is 1/_range (or similar).

Even without making it easy a lot of drivers try to do that!

I think leave it as an open question for the future for now.

Jonathan

> 
> Regards,
> Gyeyoung
> 

[PATCH v1 4/5] 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 | 386 ++++++++++++++++++++++++++++++++++
 3 files changed, 397 insertions(+)
 create mode 100644 drivers/iio/chemical/mhz19b.c

diff --git a/drivers/iio/chemical/Kconfig b/drivers/iio/chemical/Kconfig
index 330fe0af946f..bb4dfe3986f6 100644
--- a/drivers/iio/chemical/Kconfig
+++ b/drivers/iio/chemical/Kconfig
@@ -108,6 +108,16 @@ 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 Serdev binterface support for the MHZ19B
+	  CO2 sensor.
+
+	  To compile this driver as a module, choose M here: the module will
+	  be called mhz19b.
+
 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..f8f06c01623f
--- /dev/null
+++ b/drivers/iio/chemical/mhz19b.c
@@ -0,0 +1,386 @@
+// 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
+ *
+ * TODO:
+ *  - vin supply regulator
+ */
+
+#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/mutex.h>
+#include <linux/regulator/consumer.h>
+#include <linux/serdev.h>
+#include <linux/unaligned.h>
+
+struct mhz19b_state {
+	struct serdev_device *serdev;
+
+	/* TO DO, nothing for now.*/
+	struct regulator *vin_supply;
+
+	/* 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;
+
+	/* protect access to mhz19b_state */
+	struct mutex lock;
+};
+
+/*
+ * commands have following format:
+ *
+ * +------+------+-----+------+------+------+------+------+-------+
+ * | 0xFF | 0x01 | cmd | arg0 | arg1 | 0x00 | 0x00 | 0x00 | cksum |
+ * +------+------+-----+------+------+------+------+------+-------+
+ *
+ * The following commands are defined in the datasheet.
+ * https://www.winsen-sensor.com/d/files/infrared-gas-sensor/mh-z19b-co2-ver1_0.pdf
+ */
+#define MHZ19B_CMD_SIZE 9
+
+/* ABC logic in MHZ19B means auto calibration.
+ */
+#define MHZ19B_ABC_LOGIC_CMD		0x79
+#define MHZ19B_READ_CO2_CMD		0x86
+#define MHZ19B_ZERO_POINT_CMD		0x87
+#define MHZ19B_SPAN_POINT_CMD		0x88
+#define MHZ19B_DETECTION_RANGE_CMD	0x99
+
+#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)
+{
+	int ret = 0;
+	struct mhz19b_state *st = iio_priv(indio_dev);
+	struct serdev_device *serdev = st->serdev;
+	struct device *dev = &indio_dev->dev;
+
+	/* commands format is described above. */
+	uint8_t cmd_buf[MHZ19B_CMD_SIZE] = {
+		0xFF, 0x01, cmd,
+	};
+
+	switch (cmd) {
+	case MHZ19B_ABC_LOGIC_CMD: {
+		bool enable = *((bool *)arg);
+
+		cmd_buf[3] = enable ? 0xA0 : 0x00;
+		break;
+	} case MHZ19B_SPAN_POINT_CMD: {
+		uint16_t ppm = *((uint16_t *)arg);
+
+		put_unaligned_be16(ppm, &cmd_buf[3]);
+		break;
+	} case MHZ19B_DETECTION_RANGE_CMD: {
+		uint16_t range = *((uint16_t *)arg);
+
+		put_unaligned_be16(range, &cmd_buf[3]);
+		break;
+	} default:
+		break;
+	}
+	cmd_buf[MHZ19B_CMD_SIZE - 1] = mhz19b_get_checksum(cmd_buf);
+
+	scoped_guard(mutex, &st->lock) {
+		/* write buf to uart ctrl syncronously */
+		ret = serdev_device_write(serdev, cmd_buf, MHZ19B_CMD_SIZE, 0);
+		if (ret != MHZ19B_CMD_SIZE) {
+			dev_err(dev, "write err, %d bytes written", ret);
+			return -EINVAL;
+		}
+
+		switch (cmd) {
+		case MHZ19B_READ_CO2_CMD:
+			ret = wait_for_completion_interruptible_timeout(&st->buf_ready,
+				MHZ19B_SERDEV_TIMEOUT);
+			if (ret < 0)
+				return ret;
+			if (!ret)
+				return -ETIMEDOUT;
+
+			ret = mhz19b_get_checksum(st->buf);
+			if (st->buf[MHZ19B_CMD_SIZE - 1] != mhz19b_get_checksum(st->buf)) {
+				dev_err(dev, "checksum err");
+				return -EINVAL;
+			}
+
+			ret = get_unaligned_be16(&st->buf[2]);
+			return 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. */
+
+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);
+
+/* write 0		: zero point calibration_auto_enable
+ *	(make sure the sensor had been worked under 400ppm for over 20 minutes.)
+ *
+ * write 1000-5000	: span point calibration:
+ *	(make sure the sensor had been worked under a certain level co2 for over 20 minutes.)
+ */
+static ssize_t calibration_forced_value_store(struct device *dev,
+	struct device_attribute *attr,
+	const char *buf, size_t len)
+{
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	uint16_t ppm;
+	int cmd;
+
+	int 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_CONST_ATTR(calibration_forced_value_available,
+	"0 1000-5000");
+static IIO_DEVICE_ATTR_WO(calibration_forced_value, 0);
+
+/* MH-Z19B supports a measurement range adjustment feature.
+ * It can measure up to 2000 ppm or up to 5000 ppm.
+ */
+static ssize_t co2_range_store(struct device *dev,
+	struct device_attribute *attr,
+	const char *buf, size_t len)
+{
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	int ret;
+	uint16_t range;
+
+	ret = kstrtou16(buf, 10, &range);
+	if (ret)
+		return ret;
+
+	/* Detection Range should be 2000 or 5000 */
+	if (!(range == 2000 || range == 5000)) {
+		dev_dbg(&indio_dev->dev, "detection range should be 2000 or 5000");
+		return -EINVAL;
+	}
+
+
+	ret = mhz19b_serdev_cmd(indio_dev, MHZ19B_DETECTION_RANGE_CMD, &range);
+	if (ret < 0)
+		return ret;
+
+	return len;
+}
+static IIO_CONST_ATTR(co2_range_available,
+	"2000 5000");
+static IIO_DEVICE_ATTR_WO(co2_range, 0);
+
+static struct attribute *mhz19b_attrs[] = {
+	&iio_const_attr_calibration_forced_value_available.dev_attr.attr,
+	&iio_const_attr_co2_range_available.dev_attr.attr,
+	&iio_dev_attr_calibration_auto_enable.dev_attr.attr,
+	&iio_dev_attr_calibration_forced_value.dev_attr.attr,
+	&iio_dev_attr_co2_range.dev_attr.attr,
+	NULL
+};
+
+static const struct 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;
+}
+
+/* The 'serdev_device_write' function returns -EINVAL if the 'write_wakeup' member is NULL,
+ * so it must be mandatory.
+ */
+static void mhz19b_write_wakeup(struct serdev_device *serdev)
+{
+	struct iio_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 = &serdev->dev;
+	struct iio_dev *indio_dev;
+	struct mhz19b_state *st;
+
+	serdev_device_set_client_ops(serdev, &mhz19b_ops);
+
+	ret = devm_serdev_device_open(dev, serdev);
+	if (ret)
+		return ret;
+
+	ret = serdev_device_set_baudrate(serdev, 9600);
+	if (ret < 0)
+		return ret;
+
+	/* void type func, no return */
+	serdev_device_set_flow_control(serdev, false);
+
+	ret = serdev_device_set_parity(serdev, SERDEV_PARITY_NONE);
+	if (ret < 0)
+		return ret;
+
+	indio_dev = devm_iio_device_alloc(dev, sizeof(struct mhz19b_state));
+	if (indio_dev == NULL)
+		return ret;
+	dev_set_drvdata(dev, indio_dev);
+
+	st = iio_priv(indio_dev);
+	st->serdev = serdev;
+
+	init_completion(&st->buf_ready);
+	ret = devm_mutex_init(dev, &st->lock);
+	if (ret)
+		return ret;
+
+	/* TO DO:
+	 *  - vin supply
+	 */
+
+	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 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 v2");
--
2.34.1

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

[Krzysztof Kozlowski]

On 03/04/2025 07:32, Gyeyoung Baek 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>
> ---
>  drivers/iio/chemical/Kconfig  |  10 +
>  drivers/iio/chemical/Makefile |   1 +
>  drivers/iio/chemical/mhz19b.c | 386 ++++++++++++++++++++++++++++++++++
>  3 files changed, 397 insertions(+)
>  create mode 100644 drivers/iio/chemical/mhz19b.c
> 
> diff --git a/drivers/iio/chemical/Kconfig b/drivers/iio/chemical/Kconfig
> index 330fe0af946f..bb4dfe3986f6 100644
> --- a/drivers/iio/chemical/Kconfig
> +++ b/drivers/iio/chemical/Kconfig
> @@ -108,6 +108,16 @@ config IAQCORE
>  	  iAQ-Core Continuous/Pulsed VOC (Volatile Organic Compounds)
>  	  sensors
> 
> +config MHZ19B

probably WINSEN_MHZ19B

> +	tristate "MHZ19B CO2 sensor"

Similarly here

> +	depends on SERIAL_DEV_BUS
> +	help
> +	  Say Y here to build Serdev binterface support for the MHZ19B

Typo, interface

> +	  CO2 sensor.
> +
> +	  To compile this driver as a module, choose M here: the module will
> +	  be called mhz19b.
> +
>  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..f8f06c01623f
> --- /dev/null
> +++ b/drivers/iio/chemical/mhz19b.c
> @@ -0,0 +1,386 @@
> +// 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
> + *
> + * TODO:
> + *  - vin supply regulator
> + */
> +
> +#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/mutex.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/serdev.h>
> +#include <linux/unaligned.h>
> +
> +struct mhz19b_state {
> +	struct serdev_device *serdev;
> +
> +	/* TO DO, nothing for now.*/
> +	struct regulator *vin_supply;

Drop. Don't add dead code.

> +
> +	/* 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;
> +
> +	/* protect access to mhz19b_state */
> +	struct mutex lock;

mhz19b_receive_buf() does not need any locking?


Best regards,
Krzysztof

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

[gyeyoung]

Hello Krzysztof, Thank you for the review.

> > +     /* 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;
> > +
> > +     /* protect access to mhz19b_state */
> > +     struct mutex lock;
>
> mhz19b_receive_buf() does not need any locking?
>

As far as I know, receive_buf() starts from flush_to_ldisc() first.
But this already has locking, so I think it's fine.

Thanks,
Gyeyoung

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

[Jonathan Cameron]

On Thu,  3 Apr 2025 14:32:24 +0900
Gyeyoung Baek <gye976@gmail.com> wrote:

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

Various comments inline,

Thanks,

Jonathan

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

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

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

> +
> +	/* serdev receive buffer.

Fix comment syntax as mentioned below.

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

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

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

> + */

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

> +	struct mhz19b_state *st = iio_priv(indio_dev);
> +	struct serdev_device *serdev = st->serdev;
> +	struct device *dev = &indio_dev->dev;
> +
> +	/* commands format is described above. */
> +	uint8_t cmd_buf[MHZ19B_CMD_SIZE] = {
> +		0xFF, 0x01, cmd,
> +	};
> +
> +	switch (cmd) {
> +	case MHZ19B_ABC_LOGIC_CMD: {
> +		bool enable = *((bool *)arg);
> +
> +		cmd_buf[3] = enable ? 0xA0 : 0x00;
> +		break;
> +	} case MHZ19B_SPAN_POINT_CMD: {
> +		uint16_t ppm = *((uint16_t *)arg);
> +
> +		put_unaligned_be16(ppm, &cmd_buf[3]);
> +		break;
> +	} case MHZ19B_DETECTION_RANGE_CMD: {
> +		uint16_t range = *((uint16_t *)arg);
> +
> +		put_unaligned_be16(range, &cmd_buf[3]);
> +		break;
> +	} default:
> +		break;
> +	}
> +	cmd_buf[MHZ19B_CMD_SIZE - 1] = mhz19b_get_checksum(cmd_buf);
> +
> +	scoped_guard(mutex, &st->lock) {
> +		/* write buf to uart ctrl syncronously */
> +		ret = serdev_device_write(serdev, cmd_buf, MHZ19B_CMD_SIZE, 0);
> +		if (ret != MHZ19B_CMD_SIZE) {
> +			dev_err(dev, "write err, %d bytes written", ret);
> +			return -EINVAL;
> +		}
> +
> +		switch (cmd) {
> +		case MHZ19B_READ_CO2_CMD:
> +			ret = wait_for_completion_interruptible_timeout(&st->buf_ready,
> +				MHZ19B_SERDEV_TIMEOUT);
> +			if (ret < 0)
> +				return ret;
> +			if (!ret)
> +				return -ETIMEDOUT;
> +
> +			ret = mhz19b_get_checksum(st->buf);
> +			if (st->buf[MHZ19B_CMD_SIZE - 1] != mhz19b_get_checksum(st->buf)) {
> +				dev_err(dev, "checksum err");
> +				return -EINVAL;
> +			}
> +
> +			ret = get_unaligned_be16(&st->buf[2]);
			return get_unaligned_be16()

> +			return ret;
> +		default:
> +			/* no response commands. */
> +			return 0;
> +		}
> +	}
> +}


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

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


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

	ret = kstrou16()

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

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

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

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

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

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

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

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

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

> +	},
> +};

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

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


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

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

> +
> +	dev_dbg(&indio_dev->dev, "mhz19b_write_wakeup");
> +}
> +
> +static const struct serdev_device_ops mhz19b_ops = {
> +	.receive_buf = mhz19b_receive_buf,
> +	.write_wakeup = mhz19b_write_wakeup,
> +};
> +
> +static int mhz19b_probe(struct serdev_device *serdev)
> +{
> +	int ret;
> +	struct device *dev = &serdev->dev;
> +	struct iio_dev *indio_dev;
> +	struct mhz19b_state *st;
> +
> +	serdev_device_set_client_ops(serdev, &mhz19b_ops);
> +
> +	ret = devm_serdev_device_open(dev, serdev);
> +	if (ret)
> +		return ret;
> +
> +	ret = serdev_device_set_baudrate(serdev, 9600);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* void type func, no return */

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

> +	serdev_device_set_flow_control(serdev, false);
> +
> +	ret = serdev_device_set_parity(serdev, SERDEV_PARITY_NONE);
> +	if (ret < 0)
> +		return ret;
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(struct mhz19b_state));
> +	if (indio_dev == NULL)

For simple pointer validity checks.

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

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

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

> +	 */
> +
> +	indio_dev->name = "mh-z19b";
> +	indio_dev->channels = mhz19b_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(mhz19b_channels);
> +	indio_dev->info = &mhz19b_info;
> +	ret = devm_iio_device_register(dev, indio_dev);

return devm_iio_device_register();

> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}

> 

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

[gyeyoung]

Hello Jonathan, thank you for the review.

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

I added it at first since the buffer is shared, but there shouldn’t be
any locking issues in this case. I’ll drop it.
>
> > +     struct mutex lock;
> > +};


>
> > +static IIO_CONST_ATTR(calibration_forced_value_available,
> > +     "0 1000-5000");
>
> Check the syntax for available attributes. We don't have a good
> way to describe this particular set of ranges. So best option
> may unfortunately be to not describe it at all.
> Anyone calibrating this device is going to be reading the datasheet
> anyway so should know what is possible.

Yes, it seems better to keep this as a comment only.


> > +}
> > +static IIO_CONST_ATTR(co2_range_available,
> > +     "2000 5000");
> > +static IIO_DEVICE_ATTR_WO(co2_range, 0);
> > +
> > +static struct attribute *mhz19b_attrs[] = {
> > +     &iio_const_attr_calibration_forced_value_available.dev_attr.attr,
> > +     &iio_const_attr_co2_range_available.dev_attr.attr,
> > +     &iio_dev_attr_calibration_auto_enable.dev_attr.attr,
> > +     &iio_dev_attr_calibration_forced_value.dev_attr.attr,
> > +     &iio_dev_attr_co2_range.dev_attr.attr,
> > +     NULL
> > +};
>
> > +
> > +static const struct iio_chan_spec mhz19b_channels[] = {
> > +     {
> > +             .type = IIO_CONCENTRATION,
> > +             .channel2 = IIO_MOD_CO2,
> > +             .modified = 1,
> > +             .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
>
> I'm curious. We have a range control in your custom ABI, but no scale.
> So what units is this in?
>
The unit is ppm. and as far as I understand, co2 input(processed) unit
 is percentage concentration.
so In this device, the scale is a constant value (0.0001), regardless
of the range.
Is my understanding correct?

> > + */
> > +static void mhz19b_write_wakeup(struct serdev_device *serdev)
> > +{
> > +     struct iio_dev *indio_dev = dev_get_drvdata(&serdev->dev);
>
> There was still an open question on that where I asked
> if a standard existing callback was the correct minimal thing to do.
>
ah, serdev_device_write_wakeup() fits perfectly in this case.
I miss that, sorry.

> > +
> > +     init_completion(&st->buf_ready);
> > +     ret = devm_mutex_init(dev, &st->lock);
> > +     if (ret)
> > +             return ret;
> > +
> > +     /* TO DO:
> > +      *  - vin supply
>
> The code is very simple to just turn on the power supply
> at driver load and off at unload. Simpler to do that from the start than
> end up with it being added later.

Yes, I agree with you. I will add some code for it.

Thanks,
Gyeyoung

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

[Jonathan Cameron]

> > > +
> > > +static const struct iio_chan_spec mhz19b_channels[] = {
> > > +     {
> > > +             .type = IIO_CONCENTRATION,
> > > +             .channel2 = IIO_MOD_CO2,
> > > +             .modified = 1,
> > > +             .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),  
> >
> > I'm curious. We have a range control in your custom ABI, but no scale.
> > So what units is this in?
> >  
> The unit is ppm. and as far as I understand, co2 input(processed) unit
>  is percentage concentration.
> so In this device, the scale is a constant value (0.0001), regardless
> of the range.
> Is my understanding correct?

Yes.

[PATCH v1 5/5] MAINTAINERS: Add WINSEN MHZ19B

[Gyeyoung Baek]

Add undersigned as a maintainer for the WINSEN MHZ19B.

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

diff --git a/MAINTAINERS b/MAINTAINERS
index 98e9e318178b..c78c758cd069 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -25615,6 +25615,13 @@ 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/ABI/testing/sysfs-bus-iio-chemical-mhz19b
+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