Message-ID: <20181204184725.GA3334@arch>
Date:   Tue, 4 Dec 2018 19:47:29 +0100
From:   Tomasz Duszynski <tduszyns@...il.com>
To:     Andreas Brauchli <a.brauchli@...mentarea.net>
Cc:     Jonathan Cameron <jic23@...23.retrosnub.co.uk>,
        Tomasz Duszynski <tduszyns@...il.com>,
        andreas.brauchli@...sirion.com, linux-iio@...r.kernel.org,
        linux-kernel@...r.kernel.org, devicetree@...r.kernel.org
Subject: Re: [PATCH 2/3] iio: chemical: add support for Sensirion SPS30 sensor

On Mon, Dec 03, 2018 at 11:30:45AM +0100, Andreas Brauchli wrote:
> From: Andreas Brauchli <a.brauchli@...mentarea.net>
>
> On Sat, 1 Dec 2018 15:58:57 +0000, Jonathan Cameron wrote:
> > On Sun, 25 Nov 2018 20:05:09 +0100
> > Tomasz Duszynski <tduszyns@...il.com> wrote:
> >
> > > On Sun, Nov 25, 2018 at 08:56:59AM +0000, Jonathan Cameron wrote:
> > > > On Sat, 24 Nov 2018 23:14:14 +0100
> > > > Tomasz Duszynski <tduszyns@...il.com> wrote:
> > > >
> > > > > Add support for Sensirion SPS30 particulate matter sensor.
>
> Thanks a lot for your work! A few comments inline, I'm also happy to assist with
> further clarifications.
>
> Since I work for Sensirion, feel free to add me on the Acked-by list. (The
> company server molests emails..)
>
> Acked-by: Andreas Brauchli <andreas.brauchli@...sirion.com>
>
> > > > >
> > > > > Signed-off-by: Tomasz Duszynski <tduszyns@...il.com>
> > > > A few things inline.
> > > >
> > > > I'm particularly curious as to why we are ignoring two of the particle
> > > > sizes that the sensor seems to capture?
> > > >
> > >
> > > I was thinking about adding first the most common ones i.e PM2.5 and
> > > PM10 and the rest of them later on if there's a particular need.
> > >
> > > On the other hand I can add PM1.0 and PM4.0 now so that we have
> > > everything in place once new dust sensors appear on the market.
> >
> > I would.  I do hope there don't turn out to be 100s of different
> > views of what these values should be though.  The various standards
> > should prevent that even if people tuck in one or two extra
> > just because they could.
>
> PM1.0 is definitely used in other sensors (e.g. Plantower), but on a quick
> search I couldn't find another PM4.0 one.
>
> Our rationale for the value is that, according to medical studies, anything
> below PM4.0 can affect the lungs.
>

Fair enough.

> >
> > >
> > > It's seems reasonable to assume they will measure the very same
> > > subset of particulates.
> > >
> > > > Also, a 'potential' race in remove that I'd like us to make
> > > > 'obviously' correct rather than requiring hard thought on whether
> > > > it would be a problem.
> > > >
> > > > Thanks,
> > > >
> > > > Jonathan
> > > >
> > > > > ---
> > > > >  drivers/iio/chemical/Kconfig  |  11 ++
> > > > >  drivers/iio/chemical/Makefile |   1 +
> > > > >  drivers/iio/chemical/sps30.c  | 359 ++++++++++++++++++++++++++++++++++
> > > > >  3 files changed, 371 insertions(+)
> > > > >  create mode 100644 drivers/iio/chemical/sps30.c
> > > > >
> > > > > diff --git a/drivers/iio/chemical/Kconfig b/drivers/iio/chemical/Kconfig
> > > > > index b8e005be4f87..40057ecf8130 100644
> > > > > --- a/drivers/iio/chemical/Kconfig
> > > > > +++ b/drivers/iio/chemical/Kconfig
> > > > > @@ -61,6 +61,17 @@ config IAQCORE
> > > > >  	  iAQ-Core Continuous/Pulsed VOC (Volatile Organic Compounds)
> > > > >  	  sensors
> > > > >
> > > > > +config SPS30
> > > > > +	tristate "SPS30 Particulate Matter sensor"
> > > > > +	depends on I2C
> > > > > +	select CRC8
> > > > > +	help
> > > > > +	  Say Y here to build support for the Sensirion SPS30 Particulate
> > > > > +	  Matter sensor.
> > > > > +
> > > > > +	  To compile this driver as a module, choose M here: the module will
> > > > > +	  be called sps30.
> > > > > +
> > > > >  config VZ89X
> > > > >  	tristate "SGX Sensortech MiCS VZ89X VOC sensor"
> > > > >  	depends on I2C
> > > > > diff --git a/drivers/iio/chemical/Makefile b/drivers/iio/chemical/Makefile
> > > > > index 2f4c4ba4d781..9f42f4252151 100644
> > > > > --- a/drivers/iio/chemical/Makefile
> > > > > +++ b/drivers/iio/chemical/Makefile
> > > > > @@ -9,4 +9,5 @@ obj-$(CONFIG_BME680_I2C) += bme680_i2c.o
> > > > >  obj-$(CONFIG_BME680_SPI) += bme680_spi.o
> > > > >  obj-$(CONFIG_CCS811)		+= ccs811.o
> > > > >  obj-$(CONFIG_IAQCORE)		+= ams-iaq-core.o
> > > > > +obj-$(CONFIG_SPS30) += sps30.o
> > > > >  obj-$(CONFIG_VZ89X)		+= vz89x.o
> > > > > diff --git a/drivers/iio/chemical/sps30.c b/drivers/iio/chemical/sps30.c
> > > > > new file mode 100644
> > > > > index 000000000000..bf802621ae7f
> > > > > --- /dev/null
> > > > > +++ b/drivers/iio/chemical/sps30.c
> > > > > @@ -0,0 +1,359 @@
> > > > > +// SPDX-License-Identifier: GPL-2.0
> > > > > +/*
> > > > > + * Sensirion SPS30 Particulate Matter sensor driver
> > > > > + *
> > > > > + * Copyright (c) Tomasz Duszynski <tduszyns@...il.com>
> > > > > + *
> > > > > + * I2C slave address: 0x69
> > > > > + *
> > > > > + * TODO:
> > > > > + *  - support for turning on fan cleaning
> > > > > + *  - support for reading/setting auto cleaning interval
> > > > > + */
> > > > > +
> > > > > +#define pr_fmt(fmt) "sps30: " fmt
> > > > > +
> > > > > +#include <linux/crc8.h>
> > > > > +#include <linux/delay.h>
> > > > > +#include <linux/i2c.h>
> > > > > +#include <linux/iio/buffer.h>
> > > > > +#include <linux/iio/iio.h>
> > > > > +#include <linux/iio/sysfs.h>
> > > > > +#include <linux/iio/trigger_consumer.h>
> > > > > +#include <linux/iio/triggered_buffer.h>
> > > > > +#include <linux/module.h>
> > > > > +
> > > > > +#define SPS30_CRC8_POLYNOMIAL 0x31
> > > > > +
> > > > > +/* SPS30 commands */
> > > > > +#define SPS30_START_MEAS 0x0010
> > > > > +#define SPS30_STOP_MEAS 0x0104
> > > > > +#define SPS30_RESET 0xd304
>
> lower case d vs upper case in SPS30_READ_SERIAL
>

Good catch.

> > > > > +#define SPS30_READ_DATA_READY_FLAG 0x0202
> > > > > +#define SPS30_READ_DATA 0x0300
> > > > > +#define SPS30_READ_SERIAL 0xD033
> > > > > +
> > > > > +#define SPS30_CHAN(_index, _mod) { \
> > > > > +	.type = IIO_MASSCONCENTRATION, \
> > > > > +	.modified = 1, \
> > > > > +	.channel2 = IIO_MOD_ ## _mod, \
> > > > > +	.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED), \
> > > > > +	.scan_index = _index, \
> > > > > +	.scan_type = { \
> > > > > +		.sign = 'u', \
> > > > > +		.realbits = 12, \
> > > > > +		.storagebits = 32, \
> > > >
> > > > That is unusual.  Why do we need 32 bits to store a 12 bit value?
> > > > 16 should be plenty.  It'll make a difference to the buffer
> > > > sizes if people just want to stream data and don't care about
> > > > timestamps as it'll halve the memory usage.
> > >
> > > Right, I could have picked up a petter values. But I've got at least one
> > > valid reason for doing that. Sensor datasheet specifies 0-1000
> > > measurement range but simple tests showed that SPS30 can measure way beyond
> > > that. Interestingly, measurements are consistent with third party sensors.
> >
> > I'm guessing there are certification bodies for this sort of thing. They
> > may only have paid for the low end to be certified (or there may be no
> > official test for higher levels?)
> >
> > >
> > > So having 12/32 bit setting, especially 32 storagebits protects against
> > > future changes in datasheet (which is btw preliminary) i.e updating
> > > measurement range.
>
> The final datasheet is expected in Q1 2019. You could add the following link
> which always points to the latest version:
>
> https://www.sensirion.com/file/datasheet_sps30
>

Personally, I dislike the idea of adding datasheet links to the drivers.
Rationale being that they become outdated pretty soon. Anyone interested
in the details can find the doc pretty easily himself.

> >
> > Not really because userspace is going to assume the value is 12 bits and
> > will probably mask it as such, thus any change requires userspace to cope
> > with it. Once you are doing that, might as well just change the padding
> > as well.
> >
> > >
> > > But, I guess that 16 for real and storage bits should be more
> > > that enough.
> >
> > Feels like it to me as well.
> >
> > >
> > > >
> > > > Also, convention has always been to got for next 8,16,32,64 above
> > > > the realbits if there isn't a reason not to (shifting etc)
> > > >
> > > > How did we end up with 12 bits off the floating point conversion
> > > > anyway?  Needs some docs.
> > >
> > > Matter of simple tests. I was able to trigger measurements of over
> > > 3000 ug/m3.
> >
> > Hmm. So potentially it will give readings with more?  If so we should
> > work out a hard justification for that limit.  Userspace will mask
> > against it (as the rest of the bits may be garbage - we don't force
> > them to 0 in other drivers and they often contain meta data if
> > not actual garbage).
>
> The SPS30 can output high values (e.g. 60,000), but the measurement principle
> only applies up to somewhere between 3000-5000 ug/m3. Anything above 3000 is
> highly imprecise.
>

In that case, clamping the measurements to 3000.00 would be fine I
guess.

> >
> > >
> > > >
> > > >
> > > > > +		.endianness = IIO_CPU, \
> > > > > +	}, \
> > > > > +}
> > > > > +
> > > > > +enum {
> > > > > +	PM1p0, /* just a placeholder */
> > > > > +	PM2p5,
> > > > > +	PM4p0, /* just a placeholder */
> > > > > +	PM10,
> > > > > +};
> > > > > +
> > > > > +struct sps30_state {
> > > > > +	struct i2c_client *client;
> > > > > +	/* guards against concurrent access to sensor registers */
> > > > > +	struct mutex lock;
> > > > > +};
> > > > > +
> > > > > +DECLARE_CRC8_TABLE(sps30_crc8_table);
> > > > > +
> > > >
> > > > I think you are fine locking wise, but it would be good to add a note
> > > > on what locks are expected to be held on entering this function.
> > > >
> > > > Without locks it's obviously very racey!
> > > >
> > >
> > > Understood.
> > >
> > > > > +static int sps30_write_then_read(struct sps30_state *state, u8 *buf,
> > > > > +				 int buf_size, u8 *data, int data_size)
> > > > > +{
> > > > > +	/* every two received data bytes are checksummed */
> > > > > +	u8 tmp[data_size + data_size / 2];
> > > > > +	int ret, i;
> > > > > +
> > > > > +	/*
> > > > > +	 * Sensor does not support repeated start so instead of
> > > > > +	 * sending two i2c messages in a row we just send one by one.
> > > > > +	 */
> > > > > +	ret = i2c_master_send(state->client, buf, buf_size);
> > > > > +	if (ret != buf_size)
> > > > > +		return ret < 0 ? ret : -EIO;
> > > > > +
> > > > > +	if (!data)
> > > > > +		return 0;
> > > > > +
> > > > > +	ret = i2c_master_recv(state->client, tmp, sizeof(tmp));
> > > > > +	if (ret != sizeof(tmp))
> > > > > +		return ret < 0 ? ret : -EIO;
> > > > > +
> > > > > +	for (i = 0; i < sizeof(tmp); i += 3) {
> > > > > +		u8 crc = crc8(sps30_crc8_table, &tmp[i], 2, CRC8_INIT_VALUE);
> > > > > +
> > > > > +		if (crc != tmp[i + 2]) {
> > > > > +			dev_err(&state->client->dev,
> > > > > +				"data integrity check failed\n");
> > > > > +			return -EIO;
> > > > > +		}
> > > > > +
> > > > > +		*data++ = tmp[i];
> > > > > +		*data++ = tmp[i + 1];
> > > > > +	}
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > > > +
> > > > > +static int sps30_do_cmd(struct sps30_state *state, u16 cmd, u8 *data, int size)
> > > > > +{
> > > > > +	/* depending on the command up to 3 bytes may be needed for argument */
> > > > > +	u8 buf[sizeof(cmd) + 3] = { cmd >> 8, cmd };
> > > > > +
> > > > > +	switch (cmd) {
> > > > > +	case SPS30_START_MEAS:
> > > > > +		buf[2] = 0x03;
> > > > > +		buf[3] = 0x00;
> > > > > +		buf[4] = 0xac; /* precomputed crc */
> > > >
> > > > Could you put that in code and let the compiler do the pre compute?
> > > > Might be a little more elegant.
> > > >
> > >
> > > Okay.
> > >
> > > > > +		return sps30_write_then_read(state, buf, 5, NULL, 0);
> > > > > +	case SPS30_STOP_MEAS:
> > > > > +	case SPS30_RESET:
> > > > > +		return sps30_write_then_read(state, buf, 2, NULL, 0);
> > > > > +	case SPS30_READ_DATA_READY_FLAG:
> > > > > +	case SPS30_READ_DATA:
> > > > > +	case SPS30_READ_SERIAL:
> > > > > +		return sps30_write_then_read(state, buf, 2, data, size);
> > > > > +	default:
> > > > > +		return -EINVAL;
> > > > > +	};
> > > > > +}
> > > > > +
> > > > > +static int sps30_ieee754_to_int(const u8 *data)
> > > > > +{
> > > > > +	u32 val = ((u32)data[0] << 24) | ((u32)data[1] << 16) |
> > > > > +		  ((u32)data[2] << 8) | (u32)data[3],
> > > > Have a separate
> > > > u32 mantissa = (val << 9) >> 9 line.
> > > > What is this next line actually doing?  Kind of looks like
> > > > a mask?  If so just mask it with & as more readable.
> > > >
> > >
> > > Do you mean mantissa? We want to get rid of sign bit and 8 exponent bits.
> > Got it, but the above is the same as.
> >
> > val & 0x7ffffff; I think...  Shifting like this immediately made me
> > think you were manually sign extending (but it's unsigned so obviously not).
> >
> > It's a mask to extract just the mantissa, so code it as one.
> >
> >
> > >
> > > > > +	    mantissa = (val << 9) >> 9;
> > > > > +	int exp = (val >> 23) - 127;
> > > > > +
> > > > > +	if (!exp && !mantissa)
> > > > > +		return 0;
> > > > > +
> > > > > +	if (exp < 0)
> > > > > +		return 0;
> > > > > +
> > > > > +	return (1 << exp) + (mantissa >> (23 - exp));
> > > > > +}
> > > > > +
> > > > > +static int sps30_do_meas(struct sps30_state *state, int *pm2p5, int *pm10)
> > > > > +{
> > > > > +	/*
> > > > > +	 * Internally sensor stores measurements in a following manner:
> > > > > +	 *
> > > > > +	 * PM1p0: upper two bytes, crc8, lower two bytes, crc8
> > > >
> > > > So there are other particle sizes that this sensor reads?  Why
> > > > are we mapping them down to just the two channel types?
> > > >
> > >
> > > As said before, introducing PM1.0 and PM4.0 readings seems
> > > a reasonable option.
> >
> > Go for it.  Userspace who doesn't care won't read them.
> >
> > >
> > > > > +	 * PM2p5: upper two bytes, crc8, lower two bytes, crc8
> > > > > +	 * PM4p0: upper two bytes, crc8, lower two bytes, crc8
> > > > > +	 * PM10:  upper two bytes, crc8, lower two bytes, crc8
> > > > > +	 *
> > > > > +	 * What follows next are number concentration measurements and
> > > > > +	 * typical particle size measurement.
> > > > > +	 *
> > > > > +	 * Once data is read from sensor crc bytes are stripped off
> > > > > +	 * hence we need 16 bytes of buffer space.
> > > > > +	 */
> > > > > +	int ret, tries = 5;
> > > > > +	u8 buf[16];
> > > > > +
> > > > > +	while (tries--) {
> > > > > +		ret = sps30_do_cmd(state, SPS30_READ_DATA_READY_FLAG, buf, 2);
> > > > > +		if (ret)
> > > > > +			return -EIO;
> > > > > +
> > > > > +		/* new measurements ready to be read */
> > > > > +		if (buf[1] == 1)
> > > > > +			break;
> > > > > +
> > > > > +		usleep_range(300000, 400000);
> > > > > +	}
> > > > > +
> > > > > +	if (!tries)
> > > > > +		return -ETIMEDOUT;
> > > > > +
> > > > > +	ret = sps30_do_cmd(state, SPS30_READ_DATA, buf, sizeof(buf));
> > > > > +	if (ret)
> > > > > +		return ret;
> > > > > +
> > > > > +	/*
> > > > > +	 * All measurements come in IEEE754 single precision floating point
> > > > > +	 * format but sensor itself is not precise enough (-+ 10% error)
> > > > > +	 * to take full advantage of it. Hence converting result to int
> > > > > +	 * to keep things simple.
> > > >
> > > > Interesting.  We have talked about allowing floating point formats for
> > > > sensors the provide them.  Would that be beneficial here?
> > > >
> > >
> > > I recall this was discussed once but unfortunately do not
> > > remember final conclusion. Anyway, in case of this device datasheet
> > > states that readings have maximum error of -+10% in given range which
> > > makes me think that using floats here is an overkill.
> > >
> > > Example, reading of 1000.123412ug/m3 has error of -+100ug/m3.
> > > Does it really matter if care about fractional part?
>
> The datasheet specifies absolute precision, the relative precision is much
> better. The noise level is at ca. 1%. We thus suggest using 16bit values with
> 2bits dedicated to fixed point decimals.
>

Wondering why that was not mentioned in the datasheet in the first place
but good to know anyway. Then I am fine with adding that extra level of
precision i.e two extra decimal places.

> >
> > *laughs*.  It does make you wonder why they ended up as floating point
> > unless it is sensitive at very low levels? Note I know nothing about
> > particle sensing at all!
> >
>
> ..used internally
>
> > >
> > > Just out of curiosity, if IIO gets support for floats would it
> > > be problematic to to add extra channel returning measurements in floats?
> > > Or it rather will not fly..
> >
> > It's hard for userspace to interpret the case of two channels measuring the
> > same thing.  So it could be done (and we have once or twice done this)
> > but it's messy.
> >
> > >
> > > > > +	 */
> > > > > +	*pm2p5 = sps30_ieee754_to_int(&buf[PM2p5 * 4]);
> > > > > +	*pm10 = sps30_ieee754_to_int(&buf[PM10 * 4]);
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > > > +
> > > > > +static irqreturn_t sps30_trigger_handler(int irq, void *p)
> > > > > +{
> > > > > +	struct iio_poll_func *pf = p;
> > > > > +	struct iio_dev *indio_dev = pf->indio_dev;
> > > > > +	struct sps30_state *state = iio_priv(indio_dev);
> > > > > +	u32 buf[4]; /* PM2p5, PM10, timestamp */
> > > > > +	int ret;
> > > > > +
> > > > > +	mutex_lock(&state->lock);
> > > > > +	ret = sps30_do_meas(state, &buf[0], &buf[1]);
> > > > > +	mutex_unlock(&state->lock);
> > > > > +	if (ret < 0)
> > > > > +		goto err;
> > > > > +
> > > > > +	iio_push_to_buffers_with_timestamp(indio_dev, buf,
> > > > > +					   iio_get_time_ns(indio_dev));
> > > > > +err:
> > > > > +	iio_trigger_notify_done(indio_dev->trig);
> > > > > +
> > > > > +	return IRQ_HANDLED;
> > > > > +}
> > > > > +
> > > > > +static int sps30_read_raw(struct iio_dev *indio_dev,
> > > > > +			  struct iio_chan_spec const *chan,
> > > > > +			  int *val, int *val2, long mask)
> > > > > +{
> > > > > +	struct sps30_state *state = iio_priv(indio_dev);
> > > > > +	int ret;
> > > > > +
> > > > > +	switch (mask) {
> > > > > +	case IIO_CHAN_INFO_PROCESSED:
> > > > > +		switch (chan->type) {
> > > > > +		case IIO_MASSCONCENTRATION:
> > > > > +			mutex_lock(&state->lock);
> > > > > +			switch (chan->channel2) {
> > > > > +			case IIO_MOD_PM2p5:
> > > > > +				ret = sps30_do_meas(state, val, val2);
> > > > > +				break;
> > > > > +			case IIO_MOD_PM10:
> > > > > +				ret = sps30_do_meas(state, val2, val);
> > > > > +				break;
> > > > > +			default:
> > > > > +				break;
> > > > > +			}
> > > > > +			mutex_unlock(&state->lock);
> > > > > +			if (ret)
> > > > > +				return ret;
> > > > > +
> > > > > +			return IIO_VAL_INT;
> > > > > +		default:
> > > > > +			return -EINVAL;
> > > > > +		}
> > > > > +		break;
> > > > > +	default:
> > > > You can't get here.  Is this a warning suppression?
> > > > If so just add a comment saying so to prevent it being removed
> > > > by someone else later.
> > > >
> > >
> > > Okay.
> > >
> > > > > +		return -EINVAL;
> > > > > +	}
> > > > > +}
> > > > > +
> > > > > +static const struct iio_info sps30_info = {
> > > > > +	.read_raw = sps30_read_raw,
> > > > > +};
> > > > > +
> > > > From a readability point of view, it would be helpful to pull
> > > > the macro SPS30_CHAN down here in the code so we don't
> > > > need to go jumping around to check what it is doing.
> > > >
> > >
> > > Okay.
> > >
> > > > > +static const struct iio_chan_spec sps30_channels[] = {
> > > > > +	SPS30_CHAN(0, PM2p5),
> > > > > +	SPS30_CHAN(1, PM10),
> > > > > +	IIO_CHAN_SOFT_TIMESTAMP(2),
> > > > > +};
> > > > > +
> > > > > +static const unsigned long sps30_scan_masks[] = { 0x03, 0x00 };
> > > > > +
> > > > > +static int sps30_probe(struct i2c_client *client)
> > > > > +{
> > > > > +	struct iio_dev *indio_dev;
> > > > > +	struct sps30_state *state;
> > > > > +	u8 buf[32] = { };
> > > > > +	int ret;
> > > > > +
> > > > > +	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
> > > > > +		return -EOPNOTSUPP;
> > > > > +
> > > > > +	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*state));
> > > > > +	if (!indio_dev)
> > > > > +		return -ENOMEM;
> > > > > +
> > > > > +	state = iio_priv(indio_dev);
> > > > > +	i2c_set_clientdata(client, indio_dev);
> > > > > +	state->client = client;
> > > > > +	indio_dev->dev.parent = &client->dev;
> > > > > +	indio_dev->info = &sps30_info;
> > > > > +	indio_dev->name = client->name;
> > > > > +	indio_dev->channels = sps30_channels;
> > > > > +	indio_dev->num_channels = ARRAY_SIZE(sps30_channels);
> > > > > +	indio_dev->modes = INDIO_DIRECT_MODE;
> > > > > +	indio_dev->available_scan_masks = sps30_scan_masks;
> > > > > +
> > > > > +	mutex_init(&state->lock);
> > > > > +	crc8_populate_msb(sps30_crc8_table, SPS30_CRC8_POLYNOMIAL);
> > > > > +
> > > > > +	/*
> > > > > +	 * Power-on-reset causes sensor to produce some glitch on i2c bus
> > > > > +	 * and some controllers end up in error state. Recover simply
> > > > > +	 * by placing something on the bus.
> > > > > +	 */
>
> Apologies for that. I talked to the firmware engineer and we'll do our best to
> have it fixed in the next update scheduled for Q1 2019. The cause seems to be
> the pin initialization causing the clock to be pulled down for ca. 2.5us.
>

>From what I see SDA is pulled low as well for the same amount of time and
interestingly in happens always in about ~10ms after reset.

> > > > > +	ret = sps30_do_cmd(state, SPS30_RESET, NULL, 0);
> > > > > +	if (ret) {
> > > > > +		dev_err(&client->dev, "failed to reset device\n");
> > > > > +		return ret;
> > > > > +	}
> > > > > +	usleep_range(2500000, 3500000);
> > > > > +	sps30_do_cmd(state, SPS30_STOP_MEAS, NULL, 0);
> > > >
> > > > Talk me through this logic.  We stop it then pretty much immediately
> > > > start it again?  Needs a comment.
> > > >
> > >
> > > The idea is to send some message after resetting sensor so that
> > > i2c controller recovers from error state. I decided to send STOP_MEAS
> > > as it's a NOP in this case. I'll try to do more testing with other
> > > SPS30s and perhaps different boards.
> >
> > Ah.  Makes sense.  Just add a comment to the code saying this and it's
> > fine.
>
> Not to shift the blame, but the recovery is likely controller specific and might
> be better handled at that level. Consider the case where the faulty device is
> present but the driver is not loaded after the initialization..
>
> >
> > >
> > > > > +
> > > > > +	ret = sps30_do_cmd(state, SPS30_READ_SERIAL, buf, sizeof(buf));
> > > > > +	if (ret) {
> > > > > +		dev_err(&client->dev, "failed to read serial number\n");
> > > > > +		return ret;
> > > > > +	}
> > > > > +	dev_info(&client->dev, "serial number: %s\n", buf);
> > > > > +
> > > > > +	ret = sps30_do_cmd(state, SPS30_START_MEAS, NULL, 0);
> > > > > +	if (ret) {
> > > >
> > > > This is not unwound on error.   See comment on remove race below.
> > > >
> > > > > +		dev_err(&client->dev, "failed to start measurement\n");
> > > > > +		return ret;
> > > > > +	}
> > > > > +
> > > > > +	ret = devm_iio_triggered_buffer_setup(&client->dev, indio_dev, NULL,
> > > > > +					      sps30_trigger_handler, NULL);
> > > > > +	if (ret)
> > > > > +		return ret;
> > > > > +
> > > > > +	return devm_iio_device_register(&client->dev, indio_dev);
> > > > > +}
> > > > > +
> > > > > +static int sps30_remove(struct i2c_client *client)
> > > > > +{
> > > > > +	struct iio_dev *indio_dev = i2c_get_clientdata(client);
> > > > > +	struct sps30_state *state = iio_priv(indio_dev);
> > > > > +
> > > > > +	sps30_do_cmd(state, SPS30_STOP_MEAS, NULL, 0);
> > > > This looks like a race to me.
> > > >
> > >
> > > Right, this needs fixing.
> > >
> > > > You turn off the measurements before removing the interface to them.
> > > > It's certainly not in the 'obviously' right category.
> > > >
> > > > As such I would either use a devm action to do this stop,
> > > > or not use the managed interfaces for everything in probe
> > > > after the equivalent start.
> > > > The devm_add_action_or_reset would be the cleanest option I think..
> > > > Will also fix the cleanup on error in probe.
> > > >
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > > > +
> > > > > +static const struct i2c_device_id sps30_id[] = {
> > > > > +	{ "sps30" },
> > > > > +	{ }
> > > > > +};
> > > > > +MODULE_DEVICE_TABLE(i2c, sps30_id);
> > > > > +
> > > > > +static const struct of_device_id sps30_of_match[] = {
> > > > > +	{ .compatible = "sensirion,sps30" },
> > > > > +	{ }
> > > > > +};
> > > > > +MODULE_DEVICE_TABLE(of, sps30_of_match);
> > > > > +
> > > > > +static struct i2c_driver sps30_driver = {
> > > > > +	.driver = {
> > > > > +		.name = "sps30",
> > > > > +		.of_match_table = sps30_of_match,
> > > > > +	},
> > > > > +	.id_table = sps30_id,
> > > > > +	.probe_new = sps30_probe,
> > > > > +	.remove = sps30_remove,
> > > > > +};
> > > > > +module_i2c_driver(sps30_driver);
> > > > > +
> > > > > +MODULE_AUTHOR("Tomasz Duszynski <tduszyns@...il.com>");
> > > > > +MODULE_DESCRIPTION("Sensirion SPS30 particulate matter sensor driver");
> > > > > +MODULE_LICENSE("GPL v2");
> > > >

Powered by blists - more mailing lists