| Message ID | 20250403053225.298308-4-gye976@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | add support for winsen MHZ19B CO2 sensor | expand |
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
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
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 > >
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
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
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 >
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
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
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
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.
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 -- 2.34.1