On 18-10-2021 19:52, Jonathan Cameron wrote:
On Mon, 18 Oct 2021 10:19:42 +0200 Roan van Dijk <roan@xxxxxxxxxxx> wrote:On 14-10-2021 19:19, Jonathan Cameron wrote:On Thu, 14 Oct 2021 10:24:54 +0200 Roan van Dijk <roan@xxxxxxxxxxx> wrote:On 13-10-2021 19:38, Jonathan Cameron wrote:On Sun, 10 Oct 2021 16:59:19 +0100 Jonathan Cameron <jic23@xxxxxxxxxx> wrote:On Fri, 8 Oct 2021 12:17:02 +0200 Roan van Dijk <roan@xxxxxxxxxxx> 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 missedAnd 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, RoanThanks, JonathanSorry for not asking this earlier. Thanks, Roandiff --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, JonathanChanges 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
