Messages in this thread Patch in this message | | | Subject | Re: [PATCH v5 0/4] iio: chemical: Add support for Sensirion SCD4x CO2 sensor | From | Roan van Dijk <> | Date | Tue, 19 Oct 2021 09:30:34 +0200 |
| |
On 18-10-2021 19:52, Jonathan Cameron wrote: > On Mon, 18 Oct 2021 10:19:42 +0200 > Roan van Dijk <roan@protonic.nl> wrote: > >> On 14-10-2021 19:19, Jonathan Cameron wrote: >>> On Thu, 14 Oct 2021 10:24:54 +0200 >>> Roan van Dijk <roan@protonic.nl> wrote: >>> >>>> On 13-10-2021 19:38, Jonathan Cameron wrote: >>>>> On Sun, 10 Oct 2021 16:59:19 +0100 >>>>> Jonathan Cameron <jic23@kernel.org> wrote: >>>>> >>>>>> On Fri, 8 Oct 2021 12:17:02 +0200 >>>>>> Roan van Dijk <roan@protonic.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
Okay, it's now a _RAW value with a scale to make the concentration value expressed in percent. This should be the fix then.
diff --git a/drivers/iio/chemical/scd4x.c b/drivers/iio/chemical/scd4x.c index 09b34201c42b..b063b378c7d5 100644 --- a/drivers/iio/chemical/scd4x.c +++ b/drivers/iio/chemical/scd4x.c @@ -352,7 +352,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 +505,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_RAW) | + BIT(IIO_CHAN_INFO_SCALE), .address = SCD4X_CO2, .scan_index = SCD4X_CO2, .scan_type = { Thanks,
Roan
> >> .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 >>>>>>> >>>>>> >>>>> >>> >
| |