Re: [PATCH v5 0/4] iio: chemical: Add support for Sensirion SCD4x CO2 sensor

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, 19 Oct 2021 09:30:34 +0200
Roan van Dijk <roan@xxxxxxxxxxx> wrote:

> 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 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.

Looks good to me.  Please send as a new patch with Signed-off-by: etc
Technically it's not a 'fix' but I'll probably queue it up as one anyway
as better this definitely makes the same cycle as the driver.

Thanks,

Jonathan

> 
> 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
> >>>>>>>            
> >>>>>>        
> >>>>>         
> >>>      
> >   




[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Input]     [Linux Kernel]     [Linux SCSI]     [X.org]

  Powered by Linux