Re: [PATCH 1/2] iio: chemical: sgpxx: Support Sensirion SGPxx sensors

2018-03-11 Thread Jonathan Cameron
On Sat, 10 Mar 2018 23:02:17 +0100
Andreas Brauchli  wrote:

> Dear Peter, Jonathan,
> 
> Thanks for the thourough and speedy review and apologies for the delayed 
> reply.
> 
> Many of your comments are integrated in the v2 patch series - details below.
> 
...
> > >   
> > > > +
> > > > +2.2. Indoor Air Quality (IAQ) concentrations
> > > > +
> > > > +* tVOC (in_concentration_voc_input) at ppb precision (1e-9)
> > > > +* CO2eq (in_concentration_co2_input) at ppm precision (1e-6) -- SGP30 
> > > > only
> > > > +
> > > > +2.2.1. IAQ Initialization
> > > > +Before Indoor Air Quality (IAQ) values can be read, the IAQ mode must 
> > > > be
> > > > +initialized by writing a non-empty value to out_iaq_init:
> > > > +
> > > > +$ echo init > out_iaq_init
> > > 
> > > can't this be done on probe()?  
> 
> Yes, v2 now uses this approach and removes iaq_init entirely.
> A semi-replacement is provided in the form of a set_iaq_preheat_seconds
> interface that sets the pre-heat duration, on the low-power SGPC3. All uses of
> iaq_init are now purely internal and called as needed E.g. running the 
> selftest
> can mess up the state on the SGP30. So we're also re-initializing after a
> selftest and after set_baseline.
> 
> > It very much needs to be - userspace code for this sort of sensor
> > should be generic and hence not need to know about how to initialize
> > your particular sensor.  It will assume if the driver is there, the
> > device is on - or will be dynamically enabled when it wants to talk
> > to it.  
> > > 
> > > in any case, private API should be documented under
> > > Documentation/ABI/testing/sysfs-bus-iio-*  
> 
> amended in v2, please check if the format is as expected.
> 
> > >   
> > > > +After initializing IAQ, at least one IAQ signal must be read out every 
> > > > second
> > > > +(SGP30) / every two seconds (SGPC3) for the sensor to correctly 
> > > > maintain its
> > > > +internal baseline:
> > > 
> > > shouldn't the driver do this?  
> > 
> > Absolutely!
> > 
> > As I understand it, the requirement is also to prevent the hardware being 
> > read
> > more often than this so it needs to be under control of the driver.  
> 
> fixed with a kernel thread in v2 - the IAQ thread is now the only accessor of
> the chip's iaq_{init,measure,set_baseline} functions. A separate dedicated
> IAQ-values buffer is used for caching.
> 
> > >   
> > > > +
> > > > +SGP30:
> > > > +$ watch -n1 cat in_concentration_voc_input
> > > > +
> > > > +SGPC3:
> > > > +$ watch -n2 cat in_concentration_voc_input
> > > > +
> > > > +For the first 15s of operation after writing to out_iaq_init, default 
> > > > values are
> > > > +retured by the sensor.
> > 
> > In which case it should return -EBUSY from the read function and not garbage
> > data (userspace in general has no way of knowing these chip specific
> > things).  
> 
> fixed in v2. For the low-power/pulsed SGPC3, the time is dependent on the
> pre-heating duration and whether the initialization is performed with a 
> restored
> baseline. This is reflected in the code (sgpc3_iaq_init). A query of default
> values now results in -EBUSY.
> 
> Consequently, in_iaq_baseline now also returns -EBUSY instead of "" which 
> is
> considered invalid.
> 
> > > typo: returned  
> 
> fixed in v2
> 
> > > > +
> > > > +2.2.2. Pausing and Resuming IAQ
> > > > +
> > > > +For best performance and faster startup times, the baseline should be 
> > > > saved
> > > > +once every hour, after 12h of operation. The baseline is restored by 
> > > > writing a
> > > > +non-empty value to out_iaq_init, followed by writing an unmodified 
> > > > retrieved
> > > > +baseline value from in_iaq_baseline to out_iaq_baseline.
> > > 
> > > the out_ prefix seems inappropriate here, the sensors doesn't output CO2 
> > > :)  
> 
> renamed to set_iaq_baseline in v2. Should in_iaq_baseline also be renamed to
> get_iaq_baseline?
Nope, just iaq_baseline for both.

> 
> > > 
> > > handling calibration data in a generic way is difficult  
> > 
> > We can do better than this though.  Init is automatic - if you are updating 
> > the
> > baseline and it needs to call it again, then do that in the write of the 
> > baseline
> > not a separate init write.  
> 
> Yes, in v2, iaq_init is now always performed automatically by the driver.
> 
> > > > +
> > > > +Saving the baseline:
> > > > +$ baseline=$(cat in_iaq_baseline)
> > > > +
> > > > +Restoring the baseline:
> > > > +$ echo init > out_iaq_init
> > > > +$ echo -n $baseline > out_iaq_baseline
> > > > +
> > > > +2.3. Gas Concentration Signals
> > > > +
> > > > +* Ethanol (in_concentration_ethanol_raw)
> > > 
> > > we'd need a IIO_MOD_ETHANOL?  
> > 
> > Cool a new sensor type ;)  
> 
> Added in v2 - On a side note, I can imagine quite a few more gases to be added
> down the road.. maybe half a dozen if you don't ask a chemist.
> 
Sure, it's fine to add them as they come up.
 
> > 

Re: [PATCH 1/2] iio: chemical: sgpxx: Support Sensirion SGPxx sensors

2018-03-10 Thread Andreas Brauchli
Dear Peter, Jonathan,

Thanks for the thourough and speedy review and apologies for the delayed reply.

Many of your comments are integrated in the v2 patch series - details below.

On Sam, 2017-11-25 at 17:41 +, Jonathan Cameron wrote:
> On Tue, 21 Nov 2017 22:46:07 +0100 (CET)
> Peter Meerwald-Stadler  wrote:
> 
> > Hello,
> > 
> > some quick comments on this driver below
> 
> A few additional bits from me but I think Peter got most of the key
> stuff.
> 
> > 
> > I think documentation is missing and the ABI is a bit problematic and 
> > unusual 

In v2, the interface is definitely more user-friendly and with that,
hopefully, less exotic.

> > 
> > > Support Sensirion SGP30 and SGPC3 multi-pixel I2C gas sensors  
> > 
> > generally, we tend to avoid wildcard driver names; sgp30 would be 
> > preferred over sgpxx

renamed to sgp30 in v2

> >  
> > > Supported Features:
> > > 
> > > * Indoor Air Quality (IAQ) concentrations for
> > >   SGP30 and SGPC3:
> > >   - tVOC (in_concentration_voc_input)
> > >   SGP30 only:
> > >   - CO2eq (in_concentration_co2_input)
> > >   IAQ must first be initialized by writing a non-empty value to
> > >   out_iaq_init. After initializing IAQ, at least one IAQ signal must
> > >   be read out every second (SGP30) / every two seconds (SGPC3) for the
> > >   sensor to correctly maintain its internal baseline
> > > * Baseline support for IAQ (in_iaq_baseline, out_iaq_baseline)
> > > * Gas concentration signals for
> > >   SGP30 and SGPC3:
> > >   - Ethanol (in_concentration_ethanol_raw)
> > >   SGP30 only:
> > >   - H2 (in_concentration_h2_raw)
> > > * On-chip self test (in_selftest)
> > >   The self test interferes with IAQ operations. If needed, first
> > >   retrieve the current baseline, then reset it after the self test
> > > * Sensor interface version (in_feature_set_version)
> > > * Sensor serial number (in_serial_id)
> > > * Humidity compensation for SGP30
> > >   With the help of a humidity signal, the gas signals can be
> > >   humidity-compensated.
> > > * Checksummed I2C communication  
> > 
> > 
> >  
> > > For all features, refer to the data sheet or the documentation in
> > > Documentation/iio/chemical/sgpxx.txt for more details.  
> > 
> > may some brief TODOs; heat controller?

The heater interface is not publicly accessible and not part of the datasheet.
We obviously can't recommend playing with it.

> >  
> > > Signed-off-by: Andreas Brauchli 
> > > ---
> > >  Documentation/iio/chemical/sgpxx.txt | 112 +
> > >  drivers/iio/chemical/Kconfig |  13 +
> > >  drivers/iio/chemical/Makefile|   1 +
> > >  drivers/iio/chemical/sgpxx.c | 894 
> > > +++
> > >  4 files changed, 1020 insertions(+)
> > >  create mode 100644 Documentation/iio/chemical/sgpxx.txt
> > >  create mode 100644 drivers/iio/chemical/sgpxx.c
> > > 
> > > diff --git a/Documentation/iio/chemical/sgpxx.txt 
> > > b/Documentation/iio/chemical/sgpxx.txt
> > > new file mode 100644
> > > index ..f49b2f365df3
> > > --- /dev/null
> > > +++ b/Documentation/iio/chemical/sgpxx.txt
> > > @@ -0,0 +1,112 @@
> > > +sgpxx: Industrial IO driver for Sensirion i2c Multi-Pixel Gas Sensors
> > > +
> > > +1. Overview
> > > +
> > > +The sgpxx driver supports the Sensirion SGP30 and SGPC3 multi-pixel gas 
> > > sensors.
> > > +
> > > +Datasheets:
> > > +https://www.sensirion.com/fileadmin/user_upload/customers/sensirion/Dokumente/9_Gas_Sensors/Sensirion_Gas_Sensors_SGP30_Datasheet_EN.pdf
> > > +https://www.sensirion.com/fileadmin/user_upload/customers/sensirion/Dokumente/9_Gas_Sensors/Sensirion_Gas_Sensors_SGPC3_Datasheet_EN.pdf
> 
> Put this stuff in the driver itself.

already there, so removed from doc in v2.

> > > +
> > > +2. Modes of Operation
> > > +
> > > +2.1. Driver Instantiation
> > > +
> > > +The sgpxx driver must be instantiated on the corresponding i2c bus with 
> > > the
> > > +product name (sgp30 or sgpc3) and i2c address (0x58).
> > > +
> > > +Example instantiation of an sgp30 on i2c bus 1 (i2c-1):
> > > +
> > > +$ echo sgp30 0x58 | sudo tee /sys/bus/i2c/devices/i2c-1/new_device
> > > +
> > > +Using the wrong product name results in an instantiation error. Check 
> > > dmesg.  
> > 
> > I'd rather drop this section, the only specific information is the I2C 
> > address

dropped in v2

> Which belongs in the device tree bindings not here.
> 
> > 
> > > +
> > > +2.2. Indoor Air Quality (IAQ) concentrations
> > > +
> > > +* tVOC (in_concentration_voc_input) at ppb precision (1e-9)
> > > +* CO2eq (in_concentration_co2_input) at ppm precision (1e-6) -- SGP30 
> > > only
> > > +
> > > +2.2.1. IAQ Initialization
> > > +Before Indoor Air Quality (IAQ) values can be read, the IAQ mode must be
> > > +initialized by writing a non-empty value to out_iaq_init:
> > > +
> > > +$ echo init > out_iaq_init  
> > 
> > can't this be done on probe()?

Yes, v2 now uses this approach and removes 

Re: [PATCH 1/2] iio: chemical: sgpxx: Support Sensirion SGPxx sensors

2017-11-25 Thread Jonathan Cameron
On Tue, 21 Nov 2017 22:46:07 +0100 (CET)
Peter Meerwald-Stadler  wrote:

> Hello,
> 
> some quick comments on this driver below
A few additional bits from me but I think Peter got most of the key
stuff.

> 
> I think documentation is missing and the ABI is a bit problematic and 
> unusual 
> 
> > Support Sensirion SGP30 and SGPC3 multi-pixel I2C gas sensors  
> 
> generally, we tend to avoid wildcard driver names; sgp30 would be 
> preferred over sgpxx
>  
> > Supported Features:
> > 
> > * Indoor Air Quality (IAQ) concentrations for
> >   SGP30 and SGPC3:
> >   - tVOC (in_concentration_voc_input)
> >   SGP30 only:
> >   - CO2eq (in_concentration_co2_input)
> >   IAQ must first be initialized by writing a non-empty value to
> >   out_iaq_init. After initializing IAQ, at least one IAQ signal must
> >   be read out every second (SGP30) / every two seconds (SGPC3) for the
> >   sensor to correctly maintain its internal baseline
> > * Baseline support for IAQ (in_iaq_baseline, out_iaq_baseline)
> > * Gas concentration signals for
> >   SGP30 and SGPC3:
> >   - Ethanol (in_concentration_ethanol_raw)
> >   SGP30 only:
> >   - H2 (in_concentration_h2_raw)
> > * On-chip self test (in_selftest)
> >   The self test interferes with IAQ operations. If needed, first
> >   retrieve the current baseline, then reset it after the self test
> > * Sensor interface version (in_feature_set_version)
> > * Sensor serial number (in_serial_id)
> > * Humidity compensation for SGP30
> >   With the help of a humidity signal, the gas signals can be
> >   humidity-compensated.
> > * Checksummed I2C communication  
> 
> 
>  
> > For all features, refer to the data sheet or the documentation in
> > Documentation/iio/chemical/sgpxx.txt for more details.  
> 
> may some brief TODOs; heat controller?
>  
> > Signed-off-by: Andreas Brauchli 
> > ---
> >  Documentation/iio/chemical/sgpxx.txt | 112 +
> >  drivers/iio/chemical/Kconfig |  13 +
> >  drivers/iio/chemical/Makefile|   1 +
> >  drivers/iio/chemical/sgpxx.c | 894 
> > +++
> >  4 files changed, 1020 insertions(+)
> >  create mode 100644 Documentation/iio/chemical/sgpxx.txt
> >  create mode 100644 drivers/iio/chemical/sgpxx.c
> > 
> > diff --git a/Documentation/iio/chemical/sgpxx.txt 
> > b/Documentation/iio/chemical/sgpxx.txt
> > new file mode 100644
> > index ..f49b2f365df3
> > --- /dev/null
> > +++ b/Documentation/iio/chemical/sgpxx.txt
> > @@ -0,0 +1,112 @@
> > +sgpxx: Industrial IO driver for Sensirion i2c Multi-Pixel Gas Sensors
> > +
> > +1. Overview
> > +
> > +The sgpxx driver supports the Sensirion SGP30 and SGPC3 multi-pixel gas 
> > sensors.
> > +
> > +Datasheets:
> > +https://www.sensirion.com/fileadmin/user_upload/customers/sensirion/Dokumente/9_Gas_Sensors/Sensirion_Gas_Sensors_SGP30_Datasheet_EN.pdf
> > +https://www.sensirion.com/fileadmin/user_upload/customers/sensirion/Dokumente/9_Gas_Sensors/Sensirion_Gas_Sensors_SGPC3_Datasheet_EN.pdf

Put this stuff in the driver itself.

> > +
> > +2. Modes of Operation
> > +
> > +2.1. Driver Instantiation
> > +
> > +The sgpxx driver must be instantiated on the corresponding i2c bus with the
> > +product name (sgp30 or sgpc3) and i2c address (0x58).
> > +
> > +Example instantiation of an sgp30 on i2c bus 1 (i2c-1):
> > +
> > +$ echo sgp30 0x58 | sudo tee /sys/bus/i2c/devices/i2c-1/new_device
> > +
> > +Using the wrong product name results in an instantiation error. Check 
> > dmesg.  
> 
> I'd rather drop this section, the only specific information is the I2C 
> address

Which belongs in the device tree bindings not here.

> 
> > +
> > +2.2. Indoor Air Quality (IAQ) concentrations
> > +
> > +* tVOC (in_concentration_voc_input) at ppb precision (1e-9)
> > +* CO2eq (in_concentration_co2_input) at ppm precision (1e-6) -- SGP30 only
> > +
> > +2.2.1. IAQ Initialization
> > +Before Indoor Air Quality (IAQ) values can be read, the IAQ mode must be
> > +initialized by writing a non-empty value to out_iaq_init:
> > +
> > +$ echo init > out_iaq_init  
> 
> can't this be done on probe()?

It very much needs to be - userspace code for this sort of sensor
should be generic and hence not need to know about how to initialize
your particular sensor.  It will assume if the driver is there, the
device is on - or will be dynamically enabled when it wants to talk
to it.

> 
> in any case, private API should be documented under
> Documentation/ABI/testing/sysfs-bus-iio-*
> 
> > +After initializing IAQ, at least one IAQ signal must be read out every 
> > second
> > +(SGP30) / every two seconds (SGPC3) for the sensor to correctly maintain 
> > its
> > +internal baseline:  
> 
> shouldn't the driver do this?
Absolutely!

As I understand it, the requirement is also to prevent the hardware being read
more often than this so it needs to be under control of the driver.

> 
> > +
> > +SGP30:
> > +$ watch -n1 cat 

Re: [PATCH 1/2] iio: chemical: sgpxx: Support Sensirion SGPxx sensors

2017-11-21 Thread Peter Meerwald-Stadler
Hello,

some quick comments on this driver below

I think documentation is missing and the ABI is a bit problematic and 
unusual 

> Support Sensirion SGP30 and SGPC3 multi-pixel I2C gas sensors

generally, we tend to avoid wildcard driver names; sgp30 would be 
preferred over sgpxx
 
> Supported Features:
> 
> * Indoor Air Quality (IAQ) concentrations for
>   SGP30 and SGPC3:
>   - tVOC (in_concentration_voc_input)
>   SGP30 only:
>   - CO2eq (in_concentration_co2_input)
>   IAQ must first be initialized by writing a non-empty value to
>   out_iaq_init. After initializing IAQ, at least one IAQ signal must
>   be read out every second (SGP30) / every two seconds (SGPC3) for the
>   sensor to correctly maintain its internal baseline
> * Baseline support for IAQ (in_iaq_baseline, out_iaq_baseline)
> * Gas concentration signals for
>   SGP30 and SGPC3:
>   - Ethanol (in_concentration_ethanol_raw)
>   SGP30 only:
>   - H2 (in_concentration_h2_raw)
> * On-chip self test (in_selftest)
>   The self test interferes with IAQ operations. If needed, first
>   retrieve the current baseline, then reset it after the self test
> * Sensor interface version (in_feature_set_version)
> * Sensor serial number (in_serial_id)
> * Humidity compensation for SGP30
>   With the help of a humidity signal, the gas signals can be
>   humidity-compensated.
> * Checksummed I2C communication


 
> For all features, refer to the data sheet or the documentation in
> Documentation/iio/chemical/sgpxx.txt for more details.

may some brief TODOs; heat controller?
 
> Signed-off-by: Andreas Brauchli 
> ---
>  Documentation/iio/chemical/sgpxx.txt | 112 +
>  drivers/iio/chemical/Kconfig |  13 +
>  drivers/iio/chemical/Makefile|   1 +
>  drivers/iio/chemical/sgpxx.c | 894 
> +++
>  4 files changed, 1020 insertions(+)
>  create mode 100644 Documentation/iio/chemical/sgpxx.txt
>  create mode 100644 drivers/iio/chemical/sgpxx.c
> 
> diff --git a/Documentation/iio/chemical/sgpxx.txt 
> b/Documentation/iio/chemical/sgpxx.txt
> new file mode 100644
> index ..f49b2f365df3
> --- /dev/null
> +++ b/Documentation/iio/chemical/sgpxx.txt
> @@ -0,0 +1,112 @@
> +sgpxx: Industrial IO driver for Sensirion i2c Multi-Pixel Gas Sensors
> +
> +1. Overview
> +
> +The sgpxx driver supports the Sensirion SGP30 and SGPC3 multi-pixel gas 
> sensors.
> +
> +Datasheets:
> +https://www.sensirion.com/fileadmin/user_upload/customers/sensirion/Dokumente/9_Gas_Sensors/Sensirion_Gas_Sensors_SGP30_Datasheet_EN.pdf
> +https://www.sensirion.com/fileadmin/user_upload/customers/sensirion/Dokumente/9_Gas_Sensors/Sensirion_Gas_Sensors_SGPC3_Datasheet_EN.pdf
> +
> +2. Modes of Operation
> +
> +2.1. Driver Instantiation
> +
> +The sgpxx driver must be instantiated on the corresponding i2c bus with the
> +product name (sgp30 or sgpc3) and i2c address (0x58).
> +
> +Example instantiation of an sgp30 on i2c bus 1 (i2c-1):
> +
> +$ echo sgp30 0x58 | sudo tee /sys/bus/i2c/devices/i2c-1/new_device
> +
> +Using the wrong product name results in an instantiation error. Check dmesg.

I'd rather drop this section, the only specific information is the I2C 
address

> +
> +2.2. Indoor Air Quality (IAQ) concentrations
> +
> +* tVOC (in_concentration_voc_input) at ppb precision (1e-9)
> +* CO2eq (in_concentration_co2_input) at ppm precision (1e-6) -- SGP30 only
> +
> +2.2.1. IAQ Initialization
> +Before Indoor Air Quality (IAQ) values can be read, the IAQ mode must be
> +initialized by writing a non-empty value to out_iaq_init:
> +
> +$ echo init > out_iaq_init

can't this be done on probe()?

in any case, private API should be documented under
Documentation/ABI/testing/sysfs-bus-iio-*

> +After initializing IAQ, at least one IAQ signal must be read out every second
> +(SGP30) / every two seconds (SGPC3) for the sensor to correctly maintain its
> +internal baseline:

shouldn't the driver do this?

> +
> +SGP30:
> +$ watch -n1 cat in_concentration_voc_input
> +
> +SGPC3:
> +$ watch -n2 cat in_concentration_voc_input
> +
> +For the first 15s of operation after writing to out_iaq_init, default values 
> are
> +retured by the sensor.

typo: returned

> +
> +2.2.2. Pausing and Resuming IAQ
> +
> +For best performance and faster startup times, the baseline should be saved
> +once every hour, after 12h of operation. The baseline is restored by writing 
> a
> +non-empty value to out_iaq_init, followed by writing an unmodified retrieved
> +baseline value from in_iaq_baseline to out_iaq_baseline.

the out_ prefix seems inappropriate here, the sensors doesn't output CO2 :)

handling calibration data in a generic way is difficult

> +
> +Saving the baseline:
> +$ baseline=$(cat in_iaq_baseline)
> +
> +Restoring the baseline:
> +$ echo init > out_iaq_init
> +$ echo -n $baseline > out_iaq_baseline
> +
> +2.3. Gas Concentration Signals
> +
> +* Ethanol