Received: by 2002:ad5:474a:0:0:0:0:0 with SMTP id i10csp4834592imu; Sun, 25 Nov 2018 11:06:24 -0800 (PST) X-Google-Smtp-Source: AFSGD/W9RTNnTDLf7gjvDjo/TyWITZ/XHmNPoWM4b3LY9/cDOxS4xqisbJLK4pMPs6RPfMiqJgJF X-Received: by 2002:a63:ec13:: with SMTP id j19mr18357818pgh.6.1543172784531; Sun, 25 Nov 2018 11:06:24 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1543172784; cv=none; d=google.com; s=arc-20160816; b=MttEXbRZn/B/RKHdK1VX/s+H1WGwm+NSQn6nLJH6NQ0Hx0pwD/erd5duyPAHKMpBUI uajSYpnf++uRXXx4HV5a/JTfEEpgrqNORjBwasAEA+yG4jsWMRrt2JqHNRas6YSWUKr8 jcNvOL05ngKp+shZsdi4S+rNrqdDZyfRCwhgXVkoBWbhJP2CEevGWABSE3H3TWQTtX/a Ao8/u+l+AntvpxCgFyN3IWK7crMJktyfEp4q/G5je0EQRkFgHuD+Q7PNwpiRDIm4e6gD lHNWYzA3W4Vw8yfNctBBNWsa0zUbSZ/62vItYxWffdC1pFHyksopNpl2m65jOW4jvET4 iQ3A== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:user-agent:in-reply-to :content-disposition:mime-version:references:message-id:subject:cc :to:from:date:dkim-signature; bh=4u+PhV3Pc2a0CXY1OF9PyajcD4HtwwmgLGPVLzh2+t0=; b=qojsGAyP15kx1VpZLVD1qJvxiQHYtXSCh20ma/7HA1+RFZQza8D7w2wIu4maCg9wnP xGQhjIiGC4ks7ZVVpKOyYcZ5z8UJmnqSGmMxRuRiUUTcw6Bz5p6amIb3RDVvLd9sJzzL i41P7cYtjqkWSTgCuPYLyIyVuMpvBBWH+oN6+3cuTBdAYfx0AqS3PD1yB1PFrOMYnd4A dUV5R9X7EeqnHbQ/Jyvbc99WklH+tz5R7UWEaLRqsG0VsC5lHfIhU/IfPkBxcF87Xf4R dBhS2K3Dx5RnGnV5h84Aw8IQLf01RVl0vjlQWx9UFJDYo2IrOolAe0Hj0/9qormd2YVU D1JA== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@gmail.com header.s=20161025 header.b=KReGik+q; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id a2si47378216pgd.461.2018.11.25.11.06.09; Sun, 25 Nov 2018 11:06:24 -0800 (PST) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; dkim=pass header.i=@gmail.com header.s=20161025 header.b=KReGik+q; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726624AbeKZF5S (ORCPT + 99 others); Mon, 26 Nov 2018 00:57:18 -0500 Received: from mail-lf1-f67.google.com ([209.85.167.67]:43656 "EHLO mail-lf1-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725945AbeKZF5S (ORCPT ); Mon, 26 Nov 2018 00:57:18 -0500 Received: by mail-lf1-f67.google.com with SMTP id u18so11909104lff.10; Sun, 25 Nov 2018 11:05:30 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=4u+PhV3Pc2a0CXY1OF9PyajcD4HtwwmgLGPVLzh2+t0=; b=KReGik+q5IxaYxaJyqxgSc5lbK0gc6Fia6zE4vzw2vJqirBhjDk2HBU6RspzEtZCKg AIhm1zuV2rRoA47wOUxrNbEQl6LWAo+OHBnNlClX8cblxf5WCKeZ3Wo04dsZAEYGleJd yFTjU9UluqamQFbX4wzCJ/TOBOVIE6Tkf/MIMNa1DGDOYN5cokwNUpveEMNhQsYuiOuh cSdFtscPC/BVppEy0SH1yNtdkQh7HzWzfd2JFLII9j/XIDsGsiSGj700VwQwlKvxmn9t +++SpgZl/yto3v97rHkhRXh2ycokr+hToSD/Fo9uJiLnkPRlIA4BHiW0O6FQdYpXMt2h ncMQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=4u+PhV3Pc2a0CXY1OF9PyajcD4HtwwmgLGPVLzh2+t0=; b=ZekUm8pKm0HuOrUgU21duGc98gCFHM3NjESDBNb/XjlYgx0aBekUsCQ6x51xU12eQ4 xPT99GjwNEA96XqLoATO+oFdYBsMgiQnzWQu7qoYlpYMQjq6PDSjkjhx5Z2amYk4jptJ jbNZJorzZCP3mEnv82KhxYGfCHA7MUfV2t8niJu84Cr7WgeoJe6WKsNksyVzOsHruZop /Z1tEaI18ZyF8XWsrFmZy2uzg61rBslBDtBH5Gujh4QlfmU/WJZ9rHu/rXuEXt32eCnw djP+tpkhqIi+k8RRjdFH6eKis7Vi/04C2tQsHCTbIsRmRTD/dU7CTWy7ip9Uutxab4Gz dg1A== X-Gm-Message-State: AGRZ1gLUxxy+Yq1YvPon21iCQGueoCqQVVLNiC7f5MYRAWWtSe1eiJpV KJ2XaTcZY7DyN5LrNKp7qJw= X-Received: by 2002:a19:8fce:: with SMTP id s75mr13555299lfk.151.1543172729576; Sun, 25 Nov 2018 11:05:29 -0800 (PST) Received: from localhost (89-70-226-70.dynamic.chello.pl. [89.70.226.70]) by smtp.gmail.com with ESMTPSA id s3-v6sm9154529lje.73.2018.11.25.11.05.28 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Sun, 25 Nov 2018 11:05:29 -0800 (PST) Date: Sun, 25 Nov 2018 20:05:09 +0100 From: Tomasz Duszynski To: Jonathan Cameron Cc: Tomasz Duszynski , linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org, devicetree@vger.kernel.org Subject: Re: [PATCH 2/3] iio: chemical: add support for Sensirion SPS30 sensor Message-ID: <20181125190508.GD5053@arch> References: <20181124221415.10081-1-tduszyns@gmail.com> <20181124221415.10081-3-tduszyns@gmail.com> <20181125085659.114b0950@archlinux> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20181125085659.114b0950@archlinux> User-Agent: Mutt/1.10.1 (2018-07-13) Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Sun, Nov 25, 2018 at 08:56:59AM +0000, Jonathan Cameron wrote: > On Sat, 24 Nov 2018 23:14:14 +0100 > Tomasz Duszynski wrote: > > > Add support for Sensirion SPS30 particulate matter sensor. > > > > Signed-off-by: Tomasz Duszynski > 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. 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 > > + * > > + * 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 > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > + > > +#define SPS30_CRC8_POLYNOMIAL 0x31 > > + > > +/* SPS30 commands */ > > +#define SPS30_START_MEAS 0x0010 > > +#define SPS30_STOP_MEAS 0x0104 > > +#define SPS30_RESET 0xd304 > > +#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. So having 12/32 bit setting, especially 32 storagebits protects against future changes in datasheet (which is btw preliminary) i.e updating measurement range. But, I guess that 16 for real and storage bits should be more that enough. > > 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. > > > > + .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. > > + 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. > > + * 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? 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.. > > + */ > > + *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. > > + */ > > + 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. > > + > > + 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 "); > > +MODULE_DESCRIPTION("Sensirion SPS30 particulate matter sensor driver"); > > +MODULE_LICENSE("GPL v2"); >