| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20211018185225.11396bb0@jic23-huawei>
Date: Mon, 18 Oct 2021 18:52:25 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Roan van Dijk <roan@...tonic.nl>
Cc: Rob Herring <robh+dt@...nel.org>,
Tomasz Duszynski <tomasz.duszynski@...akon.com>,
linux-iio@...r.kernel.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, david@...tonic.nl,
Lars-Peter Clausen <lars@...afoo.de>
Subject: Re: [PATCH v5 0/4] iio: chemical: Add support for Sensirion SCD4x
CO2 sensor
On Mon, 18 Oct 2021 10:19:42 +0200
Roan van Dijk <roan@...tonic.nl> wrote:
> On 14-10-2021 19:19, Jonathan Cameron wrote:
> > On Thu, 14 Oct 2021 10:24:54 +0200
> > Roan van Dijk <roan@...tonic.nl> wrote:
> >
> >> On 13-10-2021 19:38, Jonathan Cameron wrote:
> >>> On Sun, 10 Oct 2021 16:59:19 +0100
> >>> Jonathan Cameron <jic23@...nel.org> wrote:
> >>>
> >>>> On Fri, 8 Oct 2021 12:17:02 +0200
> >>>> Roan van Dijk <roan@...tonic.nl> wrote:
> >>>>
> >>>>> This series adds support for the Sensirion SCD4x sensor.
> >>>>>
> >>>>> The driver supports continuous reads of temperature, relative humdity and CO2
> >>>>> concentration. There is an interval of 5 seconds between readings. During
> >>>>> this interval the drivers checks if the sensor has new data available.
> >>>>>
> >>>>> The driver is based on the scd30 driver. However, The scd4x has become too
> >>>>> different to just expand the scd30 driver. I made a new driver instead of
> >>>>> expanding the scd30 driver. I hope I made the right choice by doing so?
> >>>>
> >>>> Applied to the togreg branch of iio.git with the issues Randy mentioned tidied
> >>>> up. Pushed out as testing for 0-day to see if it can find anything we missed
> >>>
> >>> And indeed - I missed a bunch of places where explicit __be16 types should have
> >>> been used.
> >>>
> >>> I've applied the following fixup, shout if it's wrong.
> >>>
> >> Thank you Jonathan for applying this fixup. No need to shout :) Your
> >> changes should fix the issue.
> >>
> >> However, I have a question about something else. The co2 concentration
> >> is an IIO_CHAN_INFO_RAW, but doesn't have a scale or offset at this
> >> moment. Is an _scale always required for an _raw in the ABI? I could not
> >> find anything in the documentation if there is a rule for this. Someone
> >> mentioned this to me, so I want to check if I did this right.
> >>
> >> The sensor returns the actual co2 value upon reading, like 450 ppm. We
> >> can set an offset of this co2 value with the calibration_forced_value
> >> through the ABI, but this offset is handled internally by the sensor. So
> >> there isn't anything with scaling or an offset needed at the driver side.
> >
> > Ah. We could have mapped this to calibbias, though here it's made more
> > complex by other calibrations existing that don't use the value so let's
> > leave it as it is.
> >
> >>
> >> Was I right by making it of type RAW? If needed we could make it more
> >> like the scd30 driver, keeping it of type RAW but with scale = 1. What
> >> should I do or is it fine as it is?
> >
> > Hmm. Interesting corner case in the ABI. A _raw value without a scale
> > normally means we don't know it for some reason. The most common case
> > of this is light sensors where several _raw intensity values are combined
> > in some (typically non linear) transform to form a single measure of illuminance.
> > Those intensity_raw channels don't have an meaningful units, but devices
> > often have threshold events on them so we have to expose them.
> >
> > I would say make it a processed value, but there is a quirk.
> > concentrations in IIO are expressed in percent not per million, so you need
> > a scale anyway, I guess 10000? See Documentation/ABI/testing/sysfs-bus-iio
> >
> >
> > No need to do a new driver version, just send a patch tidying up this corner.
> >
>
> Hi Jonathan,
>
> As you suggested, these are my fixes for the concentration reading.
>
> The co2 reading is now a processed value and has a scale. I also added
> the information in sysfs-bus-iio documentation, because this type of
> processed value is new in the ABI.
>
> diff --git a/Documentation/ABI/testing/sysfs-bus-iio
> b/Documentation/ABI/testing/sysfs-bus-iio
> index c27347d3608e..66a17f4c831e 100644
> --- a/Documentation/ABI/testing/sysfs-bus-iio
> +++ b/Documentation/ABI/testing/sysfs-bus-iio
> @@ -1716,6 +1716,7 @@ Description:
>
> What: /sys/bus/iio/devices/iio:deviceX/in_concentration_raw
> What: /sys/bus/iio/devices/iio:deviceX/in_concentrationX_raw
> +What: /sys/bus/iio/devices/iio:deviceX/in_concentration_co2_input
> What: /sys/bus/iio/devices/iio:deviceX/in_concentration_co2_raw
> What: /sys/bus/iio/devices/iio:deviceX/in_concentrationX_co2_raw
> What:
> /sys/bus/iio/devices/iio:deviceX/in_concentration_ethanol_raw
> diff --git a/drivers/iio/chemical/scd4x.c b/drivers/iio/chemical/scd4x.c
> index 09b34201c42b..bc1c6676029d 100644
> --- a/drivers/iio/chemical/scd4x.c
> +++ b/drivers/iio/chemical/scd4x.c
> @@ -337,6 +337,7 @@ static int scd4x_read_raw(struct iio_dev *indio_dev,
>
> switch (mask) {
> case IIO_CHAN_INFO_RAW:
> + case IIO_CHAN_INFO_PROCESSED:
> ret = iio_device_claim_direct_mode(indio_dev);
> if (ret)
> return ret;
> @@ -352,7 +353,11 @@ static int scd4x_read_raw(struct iio_dev *indio_dev,
> *val = ret;
> return IIO_VAL_INT;
> case IIO_CHAN_INFO_SCALE:
> - if (chan->type == IIO_TEMP) {
> + if (chan->type == IIO_CONCENTRATION) {
> + *val = 0;
> + *val2 = 100;
> + return IIO_VAL_INT_PLUS_MICRO;
> + } else if (chan->type == IIO_TEMP) {
> *val = 175000;
> *val2 = 65536;
> return IIO_VAL_FRACTIONAL;
> @@ -501,7 +506,8 @@ static const struct iio_chan_spec scd4x_channels[] = {
> .type = IIO_CONCENTRATION,
> .channel2 = IIO_MOD_CO2,
> .modified = 1,
> - .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |
> + BIT(IIO_CHAN_INFO_SCALE),
You shouldn't have scale and processed. If we need a scale, then it should
be _RAW.
Jonathan
> .address = SCD4X_CO2,
> .scan_index = SCD4X_CO2,
> .scan_type = {
>
> Thanks,
>
> Roan
>
> > Thanks,
> >
> > Jonathan
> >
> >
> >> Sorry for not asking this earlier.
> >>
> >> Thanks,
> >>
> >> Roan
> >>
> >>> diff --git a/drivers/iio/chemical/scd4x.c b/drivers/iio/chemical/scd4x.c
> >>> index 09b34201c42b..ebebcb117ba2 100644
> >>> --- a/drivers/iio/chemical/scd4x.c
> >>> +++ b/drivers/iio/chemical/scd4x.c
> >>> @@ -263,7 +263,7 @@ static int scd4x_write_and_fetch(struct scd4x_state *state, enum scd4x_cmd cmd,
> >>> static int scd4x_read_meas(struct scd4x_state *state, uint16_t *meas)
> >>> {
> >>> int i, ret;
> >>> - uint16_t buf[3];
> >>> + __be16 buf[3];
> >>>
> >>> ret = scd4x_read(state, CMD_READ_MEAS, buf, sizeof(buf));
> >>> if (ret)
> >>> @@ -282,12 +282,13 @@ static int scd4x_wait_meas_poll(struct scd4x_state *state)
> >>> int ret;
> >>>
> >>> do {
> >>> + __be16 bval;
> >>> uint16_t val;
> >>>
> >>> - ret = scd4x_read(state, CMD_GET_DATA_READY, &val, sizeof(val));
> >>> + ret = scd4x_read(state, CMD_GET_DATA_READY, &bval, sizeof(bval));
> >>> if (ret)
> >>> return -EIO;
> >>> - val = be16_to_cpu(val);
> >>> + val = be16_to_cpu(bval);
> >>>
> >>> /* new measurement available */
> >>> if (val & 0x7FF)
> >>> @@ -333,7 +334,7 @@ static int scd4x_read_raw(struct iio_dev *indio_dev,
> >>> {
> >>> struct scd4x_state *state = iio_priv(indio_dev);
> >>> int ret;
> >>> - uint16_t tmp;
> >>> + __be16 tmp;
> >>>
> >>> switch (mask) {
> >>> case IIO_CHAN_INFO_RAW:
> >>> @@ -405,17 +406,18 @@ static ssize_t calibration_auto_enable_show(struct device *dev,
> >>> struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> >>> struct scd4x_state *state = iio_priv(indio_dev);
> >>> int ret;
> >>> - uint16_t val;
> >>> + __be16 bval;
> >>> + u16 val;
> >>>
> >>> mutex_lock(&state->lock);
> >>> - ret = scd4x_read(state, CMD_GET_ASC, &val, sizeof(val));
> >>> + ret = scd4x_read(state, CMD_GET_ASC, &bval, sizeof(bval));
> >>> mutex_unlock(&state->lock);
> >>> if (ret) {
> >>> dev_err(dev, "failed to read automatic calibration");
> >>> return ret;
> >>> }
> >>>
> >>> - val = (be16_to_cpu(val) & SCD4X_READY_MASK) ? 1 : 0;
> >>> + val = (be16_to_cpu(bval) & SCD4X_READY_MASK) ? 1 : 0;
> >>>
> >>> return sprintf(buf, "%d\n", val);
> >>> }
> >>>
> >>>
> >>>>
> >>>> Thanks,
> >>>>
> >>>> Jonathan
> >>>>
> >>>>>
> >>>>> Changes since v5:
> >>>>> scd4x.c:
> >>>>> - Fix bug in trigger_handler
> >>>>>
> >>>>> Changes since v4:
> >>>>> scd4x.c:
> >>>>> - Minor fixes in documentation
> >>>>> - Reorder trigger_handler so memcpy is not needed anymore
> >>>>> Documentation:
> >>>>> - Change information about the KernelVersion for the
> >>>>> calibration_forced_value_available
> >>>>>
> >>>>> Changes since v3:
> >>>>> scd4x.c
> >>>>> - Change read and write_and_fetch function parameter. CRC byte is now
> >>>>> hidden inside the function.
> >>>>> - Fix minor style issues
> >>>>> - Add calibration_forced_value_available attribute to the driver
> >>>>> - Remove including BUFFER_TRIGGERED
> >>>>> - Change calibbias to raw ADC readings rather than converting it to
> >>>>> milli degrees C.
> >>>>> Documentation:
> >>>>> - Change description of driver attributes
> >>>>> - Add calibration_forced_value_available documentation
> >>>>>
> >>>>> Changes since v2:
> >>>>> scd4x.c:
> >>>>> - Change boolean operations
> >>>>> - Document scope of lock
> >>>>> - Remove device *dev from struct
> >>>>> - Add goto block for errror handling
> >>>>> - Add function to read value per channel in read_raw
> >>>>> - Fix bug with lock in error paths
> >>>>> - Remove conversion of humidity and temperature values
> >>>>> - Add scale and offset to temperature channel
> >>>>> - Add scale to humidity channel
> >>>>> - Move memset out of locked section
> >>>>> - Remove unused irq functions
> >>>>> - Move device register at end of probe function
> >>>>> Documentation:
> >>>>> - Copy content of sysfs-bus-iio-scd30 to sysfs-bus-iio
> >>>>> - Remove Documentation/ABI/testing/sysfs-bus-iio-scd30
> >>>>>
> >>>>> Changes since v1:
> >>>>> dt-bindings:
> >>>>> - Separated compatible string for each sensor type
> >>>>> scd4x.c:
> >>>>> - Changed probe, resume and suspend functions to static
> >>>>> - Added SIMPLE_DEV_PM_OPS function call for power management
> >>>>> operations.
> >>>>>
> >>>>> Roan van Dijk (4):
> >>>>> dt-bindings: iio: chemical: sensirion,scd4x: Add yaml description
> >>>>> MAINTAINERS: Add myself as maintainer of the scd4x driver
> >>>>> drivers: iio: chemical: Add support for Sensirion SCD4x CO2 sensor
> >>>>> iio: documentation: Document scd4x calibration use
> >>>>>
> >>>>> Documentation/ABI/testing/sysfs-bus-iio | 41 ++
> >>>>> Documentation/ABI/testing/sysfs-bus-iio-scd30 | 34 -
> >>>>> .../iio/chemical/sensirion,scd4x.yaml | 46 ++
> >>>>> MAINTAINERS | 6 +
> >>>>> drivers/iio/chemical/Kconfig | 13 +
> >>>>> drivers/iio/chemical/Makefile | 1 +
> >>>>> drivers/iio/chemical/scd4x.c | 689 ++++++++++++++++++
> >>>>> 7 files changed, 796 insertions(+), 34 deletions(-)
> >>>>> delete mode 100644 Documentation/ABI/testing/sysfs-bus-iio-scd30
> >>>>> create mode 100644 Documentation/devicetree/bindings/iio/chemical/sensirion,scd4x.yaml
> >>>>> create mode 100644 drivers/iio/chemical/scd4x.c
> >>>>>
> >>>>
> >>>
> >
Powered by blists - more mailing lists