Date:   Sat, 25 Apr 2020 20:00:47 +0100
From:   Jonathan Cameron <jic23@...23.retrosnub.co.uk>
To:     Tomasz Duszynski <tomasz.duszynski@...akon.com>
Cc:     <linux-iio@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <devicetree@...r.kernel.org>, <robh+dt@...nel.org>
Subject: Re: [PATCH 1/6] iio: chemical: scd30: add core driver

On Sat, 25 Apr 2020 19:55:34 +0100
Jonathan Cameron <jic23@...nel.org> wrote:

> On Wed, 22 Apr 2020 16:11:30 +0200
> Tomasz Duszynski <tomasz.duszynski@...akon.com> wrote:
> 
> > Add Sensirion SCD30 carbon dioxide core driver.
> > 
> > Signed-off-by: Tomasz Duszynski <tomasz.duszynski@...akon.com>  
> Hi Tomasz
> 
> As you've probably guessed the big questions are around the custom ABI.
Doh. You documented it later in the series. Sorry missed that for some reason
at first glance!

Jonathan

> 
> Few other things inline.
> 
> Jonathan
> 
> > ---
> >  drivers/iio/chemical/Kconfig      |  11 +
> >  drivers/iio/chemical/Makefile     |   1 +
> >  drivers/iio/chemical/scd30.h      |  72 +++
> >  drivers/iio/chemical/scd30_core.c | 796 ++++++++++++++++++++++++++++++
> >  4 files changed, 880 insertions(+)
> >  create mode 100644 drivers/iio/chemical/scd30.h
> >  create mode 100644 drivers/iio/chemical/scd30_core.c
> > 
> > diff --git a/drivers/iio/chemical/Kconfig b/drivers/iio/chemical/Kconfig
> > index 0b91de4df8f4..55f249333fa2 100644
> > --- a/drivers/iio/chemical/Kconfig
> > +++ b/drivers/iio/chemical/Kconfig
> > @@ -74,6 +74,17 @@ config PMS7003
> >  	  To compile this driver as a module, choose M here: the module will
> >  	  be called pms7003.
> >  
> > +config SCD30_CORE
> > +	tristate "SCD30 carbon dioxide sensor driver"
> > +	select IIO_BUFFER
> > +	select IIO_TRIGGERED_BUFFER
> > +	help
> > +	  Say Y here to build support for the Sensirion SCD30 sensor with carbon
> > +	  dioxide, relative humidity and temperature sensing capabilities.
> > +
> > +	  To compile this driver as a module, choose M here: the module will
> > +	  be called scd30_core.
> > +
> >  config SENSIRION_SGP30
> >  	tristate "Sensirion SGPxx gas sensors"
> >  	depends on I2C
> > diff --git a/drivers/iio/chemical/Makefile b/drivers/iio/chemical/Makefile
> > index 33d3a595dda9..54abcb641262 100644
> > --- a/drivers/iio/chemical/Makefile
> > +++ b/drivers/iio/chemical/Makefile
> > @@ -11,6 +11,7 @@ obj-$(CONFIG_BME680_SPI) += bme680_spi.o
> >  obj-$(CONFIG_CCS811)		+= ccs811.o
> >  obj-$(CONFIG_IAQCORE)		+= ams-iaq-core.o
> >  obj-$(CONFIG_PMS7003) += pms7003.o
> > +obj-$(CONFIG_SCD30_CORE) += scd30_core.o
> >  obj-$(CONFIG_SENSIRION_SGP30)	+= sgp30.o
> >  obj-$(CONFIG_SPS30) += sps30.o
> >  obj-$(CONFIG_VZ89X)		+= vz89x.o
> > diff --git a/drivers/iio/chemical/scd30.h b/drivers/iio/chemical/scd30.h
> > new file mode 100644
> > index 000000000000..814782f5e71a
> > --- /dev/null
> > +++ b/drivers/iio/chemical/scd30.h
> > @@ -0,0 +1,72 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +#ifndef _SCD30_H
> > +#define _SCD30_H
> > +
> > +#include <linux/completion.h>
> > +#include <linux/device.h>
> > +#include <linux/i2c.h>  
> 
> Doesn't make much sense to have an i2c header included here.
> 
> > +#include <linux/mutex.h>
> > +#include <linux/pm.h>
> > +#include <linux/regulator/consumer.h>
> > +#include <linux/types.h>
> > +
> > +enum scd30_cmd {
> > +	/* start continuous measurement with pressure compensation */
> > +	CMD_START_MEAS,
> > +	/* stop continuous measurement */
> > +	CMD_STOP_MEAS,
> > +	/* set/get measurement interval */
> > +	CMD_MEAS_INTERVAL,
> > +	/* check whether new measurement is ready */
> > +	CMD_MEAS_READY,
> > +	/* get measurement */
> > +	CMD_READ_MEAS,
> > +	/* turn on/off automatic self calibration */
> > +	CMD_ASC,
> > +	/* set/get forced recalibration value */
> > +	CMD_FRC,
> > +	/* set/get temperature offset */
> > +	CMD_TEMP_OFFSET,
> > +	/* get firmware version */
> > +	CMD_FW_VERSION,
> > +	/* reset sensor */
> > +	CMD_RESET,
> > +	/*
> > +	 * Command for altitude compensation was omitted intentionally because
> > +	 * the same can be achieved by means of CMD_START_MEAS which takes
> > +	 * pressure above the sea level as an argument.
> > +	 */
> > +};
> > +
> > +#define SCD30_MEAS_COUNT 3
> > +
> > +struct scd30_state {
> > +	/* serialize access to the device */
> > +	struct mutex lock;
> > +	struct device *dev;
> > +	struct regulator *vdd;
> > +	struct completion meas_ready;
> > +	void *priv;
> > +	int irq;
> > +	/*
> > +	 * no way to retrieve current ambient pressure compensation value from
> > +	 * the sensor so keep one around
> > +	 */
> > +	u16 pressure_comp;
> > +	u16 meas_interval;
> > +	int meas[SCD30_MEAS_COUNT];
> > +
> > +	int (*command)(struct scd30_state *state, enum scd30_cmd cmd, u16 arg,
> > +		       char *rsp, int size);
> > +};
> > +
> > +int scd30_suspend(struct device *dev);
> > +int scd30_resume(struct device *dev);
> > +
> > +static SIMPLE_DEV_PM_OPS(scd30_pm_ops, scd30_suspend, scd30_resume);
> > +
> > +int scd30_probe(struct device *dev, int irq, const char *name, void *priv,
> > +		int (*command)(struct scd30_state *state, enum scd30_cmd cmd,
> > +			       u16 arg, char *rsp, int size));
> > +
> > +#endif
> > diff --git a/drivers/iio/chemical/scd30_core.c b/drivers/iio/chemical/scd30_core.c
> > new file mode 100644
> > index 000000000000..4dc7e8f9a4f1
> > --- /dev/null
> > +++ b/drivers/iio/chemical/scd30_core.c
> > @@ -0,0 +1,796 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Sensirion SCD30 carbon dioxide sensor core driver
> > + *
> > + * Copyright (c) Tomasz Duszynski <tomasz.duszynski@...akon.com>
> > + */
> > +#include <asm/byteorder.h>
> > +#include <linux/bits.h>
> > +#include <linux/compiler.h>
> > +#include <linux/completion.h>
> > +#include <linux/delay.h>
> > +#include <linux/device.h>
> > +#include <linux/errno.h>
> > +#include <linux/export.h>
> > +#include <linux/iio/buffer.h>
> > +#include <linux/iio/iio.h>
> > +#include <linux/iio/sysfs.h>
> > +#include <linux/iio/trigger.h>
> > +#include <linux/iio/trigger_consumer.h>
> > +#include <linux/iio/triggered_buffer.h>
> > +#include <linux/iio/types.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/irqreturn.h>
> > +#include <linux/jiffies.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/mutex.h>
> > +#include <linux/regulator/consumer.h>
> > +#include <linux/string.h>
> > +#include <linux/sysfs.h>
> > +#include <linux/types.h>
> > +
> > +#include "scd30.h"
> > +
> > +/* pressure compensation in millibars */
> > +#define SCD30_PRESSURE_COMP_MIN 700
> > +#define SCD30_PRESSURE_COMP_MAX 1400
> > +#define SCD30_PRESSURE_COMP_DEFAULT 1013
> > +/* measurement interval in seconds */
> > +#define SCD30_MEAS_INTERVAL_MIN 2
> > +#define SCD30_MEAS_INTERVAL_MAX 1800
> > +#define SCD30_MEAS_INTERVAL_DEFAULT SCD30_MEAS_INTERVAL_MIN
> > +/* reference CO2 concentration in ppm */
> > +#define SCD30_FRC_MIN 400
> > +#define SCD30_FRC_MAX 2000
> > +
> > +enum {
> > +	CONC,
> > +	TEMP,
> > +	HR,
> > +};
> > +
> > +static int scd30_command(struct scd30_state *state, enum scd30_cmd cmd, u16 arg,
> > +			 char *rsp, int size)
> > +{
> > +	int ret;
> > +
> > +	ret = state->command(state, cmd, arg, rsp, size);
> > +	if (ret)
> > +		return ret;
> > +
> > +	/*
> > +	 * assumption holds that response buffer pointer has been already
> > +	 * properly aligned so casts are safe
> > +	 */
> > +	while (size >= sizeof(u32)) {
> > +		*(u32 *)rsp = be32_to_cpup((__be32 *)rsp);
> > +		rsp += sizeof(u32);
> > +		size -= sizeof(u32);
> > +	}
> > +  
> 
> It's more than a little nasty to rely on the readout either being
> a set of __be32s or a single __be16. 
> 
> I would break this function into two options and then you can have
> the relevant sized pointer for rsp and drop the various casts.
> 
> Alternatively just do the endian conversions where they are needed
> and call the state->command directly.
> 
> > +	if (size)
> > +		*(u16 *)rsp = be16_to_cpup((__be16 *)rsp);
> > +
> > +	return 0;
> > +}
> > +
> > +static int scd30_reset(struct scd30_state *state)
> > +{
> > +	int ret;
> > +	u16 val;
> > +
> > +	ret = scd30_command(state, CMD_RESET, 0, NULL, 0);
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* sensor boots up within 2 secs */
> > +	msleep(2000);
> > +	/*
> > +	 * Power-on-reset causes sensor to produce some glitch on i2c bus and
> > +	 * some controllers end up in error state. Try to recover by placing
> > +	 * any data on the bus.
> > +	 */
> > +	scd30_command(state, CMD_MEAS_READY, 0, (char *)&val, sizeof(val));
> > +
> > +	return 0;
> > +}
> > +
> > +/* simplified float to fixed point conversion with a scaling factor of 0.01 */
> > +static int scd30_float_to_fp(int float32)
> > +{
> > +	int fraction, shift,
> > +	    mantissa = float32 & GENMASK(22, 0),
> > +	    sign = float32 & BIT(31) ? -1 : 1,
> > +	    exp = (float32 & ~BIT(31)) >> 23;
> > +
> > +	/* special case 0 */
> > +	if (!exp && !mantissa)
> > +		return 0;
> > +
> > +	exp -= 127;
> > +	if (exp < 0) {
> > +		exp = -exp;
> > +		/* return values ranging from 1 to 99 */
> > +		return sign * ((((BIT(23) + mantissa) * 100) >> 23) >> exp);
> > +	}
> > +
> > +	/* return values starting at 100 */
> > +	shift = 23 - exp;
> > +	float32 = BIT(exp) + (mantissa >> shift);
> > +	fraction = mantissa & GENMASK(shift - 1, 0);
> > +
> > +	return sign * (float32 * 100 + ((fraction * 100) >> shift));
> > +}
> > +
> > +static int scd30_read_meas(struct scd30_state *state)
> > +{
> > +	int i, ret;
> > +
> > +	ret = scd30_command(state, CMD_READ_MEAS, 0, (char *)state->meas,
> > +			    sizeof(state->meas));
> > +	if (ret)
> > +		return ret;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(state->meas); i++)
> > +		state->meas[i] = scd30_float_to_fp(state->meas[i]);  
> 
> We have previously discussed proving direct floating point channel types
> for the rare devices that actually provide floating point data in
> a standard format.
> 
> I'm happy to revisit that if you would like to.
> 
> > +
> > +	/*
> > +	 * Accuracy within calibrated operating range is
> > +	 * +-(30ppm + 3% measurement) so fractional part does
> > +	 * not add real value. Moreover, ppm is an integer.
> > +	 */
> > +	state->meas[CONC] /= 100;
> > +
> > +	return 0;
> > +}
> > +
> > +static int scd30_wait_meas_irq(struct scd30_state *state)
> > +{
> > +	int ret, timeout = msecs_to_jiffies(state->meas_interval * 1250);
> > +
> > +	reinit_completion(&state->meas_ready);
> > +	enable_irq(state->irq);  
> 
> So this is just 'grab the next one'?
> 
> > +	ret = wait_for_completion_interruptible_timeout(&state->meas_ready,
> > +							timeout);
> > +	if (ret > 0)
> > +		ret = 0;
> > +	else if (!ret)
> > +		ret = -ETIMEDOUT;
> > +  
> 
> I suppose a race here doesn't matter?  Additional interrupt is safe if not
> efficient?
> 
> > +	disable_irq(state->irq);
> > +
> > +	return ret;
> > +}
> > +
> > +static int scd30_wait_meas_poll(struct scd30_state *state)
> > +{
> > +	int tries = 5;
> > +
> > +	while (tries--) {
> > +		int ret;
> > +		u16 val;
> > +
> > +		ret = scd30_command(state, CMD_MEAS_READY, 0, (char *)&val,
> > +				    sizeof(val));
> > +		if (ret)
> > +			return -EIO;
> > +
> > +		/* new measurement available */
> > +		if (val)
> > +			break;
> > +
> > +		msleep_interruptible(state->meas_interval * 250);
> > +	}
> > +
> > +	if (tries == -1)
> > +		return -ETIMEDOUT;
> > +
> > +	return 0;
> > +}
> > +
> > +static int scd30_read_poll(struct scd30_state *state)
> > +{
> > +	int ret;
> > +
> > +	ret = scd30_wait_meas_poll(state);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return scd30_read_meas(state);
> > +}
> > +
> > +static int scd30_read(struct scd30_state *state)
> > +{
> > +	if (state->irq > 0)
> > +		return scd30_wait_meas_irq(state);
> > +
> > +	return scd30_read_poll(state);
> > +}
> > +
> > +static int scd30_read_raw(struct iio_dev *indio_dev,
> > +			  struct iio_chan_spec const *chan,
> > +			  int *val, int *val2, long mask)
> > +{
> > +	struct scd30_state *state = iio_priv(indio_dev);
> > +	int ret, meas[SCD30_MEAS_COUNT];
> > +
> > +	switch (mask) {
> > +	case IIO_CHAN_INFO_PROCESSED:
> > +		ret = iio_device_claim_direct_mode(indio_dev);
> > +		if (ret)
> > +			return ret;
> > +
> > +		mutex_lock(&state->lock);
> > +		ret = scd30_read(state);
> > +		memcpy(meas, state->meas, sizeof(meas));
> > +		mutex_unlock(&state->lock);
> > +		iio_device_release_direct_mode(indio_dev);
> > +		if (ret)
> > +			return ret;
> > +
> > +		switch (chan->type) {
> > +		case IIO_CONCENTRATION:
> > +			*val = meas[chan->address] / 10000;
> > +			*val2 = (meas[chan->address] % 10000) * 100;
> > +			return IIO_VAL_INT_PLUS_MICRO;
> > +		case IIO_TEMP:
> > +		case IIO_HUMIDITYRELATIVE:
> > +			*val = meas[chan->address] * 10;
> > +			return IIO_VAL_INT;
> > +		default:
> > +			return -EINVAL;
> > +		}
> > +	case IIO_CHAN_INFO_SCALE:
> > +		switch (chan->type) {
> > +		case IIO_CONCENTRATION:
> > +			*val = 0;
> > +			*val2 = 100;
> > +			return IIO_VAL_INT_PLUS_MICRO;
> > +		case IIO_TEMP:
> > +		case IIO_HUMIDITYRELATIVE:
> > +			*val = 10;
> > +			return IIO_VAL_INT;
> > +		default:
> > +			return -EINVAL;
> > +		}
> > +	}
> > +
> > +	return -EINVAL;
> > +}
> > +
> > +static ssize_t pressure_comp_show(struct device *dev,
> > +				  struct device_attribute *attr, char *buf)
> > +{
> > +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> > +	struct scd30_state *state = iio_priv(indio_dev);
> > +	int ret;
> > +
> > +	mutex_lock(&state->lock);
> > +	ret = sprintf(buf, "%d\n", state->pressure_comp);
> > +	mutex_unlock(&state->lock);
> > +
> > +	return ret;
> > +}
> > +
> > +static ssize_t pressure_comp_store(struct device *dev,
> > +				   struct device_attribute *attr,
> > +				   const char *buf, size_t len)
> > +{
> > +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> > +	struct scd30_state *state = iio_priv(indio_dev);
> > +	int ret;
> > +	u16 val;
> > +
> > +	if (kstrtou16(buf, 0, &val))
> > +		return -EINVAL;
> > +
> > +	if ((val < SCD30_PRESSURE_COMP_MIN) || (val > SCD30_PRESSURE_COMP_MAX))
> > +		return -EINVAL;
> > +
> > +	mutex_lock(&state->lock);
> > +	ret = scd30_command(state, CMD_START_MEAS, val, NULL, 0);
> > +	if (ret)
> > +		goto out;
> > +
> > +	state->pressure_comp = val;
> > +out:
> > +	mutex_unlock(&state->lock);
> > +
> > +	return ret ?: len;
> > +}
> > +
> > +static ssize_t pressure_comp_available_show(struct device *dev,
> > +					    struct device_attribute *attr,
> > +					    char *buf)
> > +{
> > +	return sprintf(buf, "[%d %d %d]\n", SCD30_PRESSURE_COMP_MIN, 1,
> > +		       SCD30_PRESSURE_COMP_MAX);
> > +}
> > +
> > +static ssize_t meas_interval_show(struct device *dev,
> > +				  struct device_attribute *attr, char *buf)
> > +{
> > +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> > +	struct scd30_state *state = iio_priv(indio_dev);
> > +	int ret;
> > +	u16 val;
> > +
> > +	mutex_lock(&state->lock);
> > +	ret = scd30_command(state, CMD_MEAS_INTERVAL, 0, (char *)&val,
> > +			    sizeof(val));
> > +	mutex_unlock(&state->lock);
> > +
> > +	return ret ?: sprintf(buf, "%d\n", val);
> > +}
> > +
> > +static ssize_t meas_interval_store(struct device *dev,
> > +				   struct device_attribute *attr,
> > +				   const char *buf, size_t len)
> > +{
> > +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> > +	struct scd30_state *state = iio_priv(indio_dev);
> > +	int ret;
> > +	u16 val;
> > +
> > +	if (kstrtou16(buf, 0, &val))
> > +		return -EINVAL;
> > +
> > +	if ((val < SCD30_MEAS_INTERVAL_MIN) || (val > SCD30_MEAS_INTERVAL_MAX))
> > +		return -EINVAL;
> > +
> > +	mutex_lock(&state->lock);
> > +	ret = scd30_command(state, CMD_MEAS_INTERVAL, val, NULL, 0);
> > +	if (ret)
> > +		goto out;
> > +
> > +	state->meas_interval = val;
> > +out:
> > +	mutex_unlock(&state->lock);
> > +
> > +	return ret ?: len;
> > +}
> > +
> > +static ssize_t meas_interval_available_show(struct device *dev,
> > +					    struct device_attribute *attr,
> > +					    char *buf)
> > +{
> > +	return sprintf(buf, "[%d %d %d]\n", SCD30_MEAS_INTERVAL_MIN, 1,
> > +		       SCD30_MEAS_INTERVAL_MAX);
> > +}
> > +
> > +static ssize_t asc_show(struct device *dev, struct device_attribute *attr,
> > +			char *buf)
> > +{
> > +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> > +	struct scd30_state *state = iio_priv(indio_dev);
> > +	int ret;
> > +	u16 val;
> > +
> > +	mutex_lock(&state->lock);
> > +	ret = scd30_command(state, CMD_ASC, 0, (char *)&val, sizeof(val));
> > +	mutex_unlock(&state->lock);
> > +
> > +	return ret ?: sprintf(buf, "%d\n", val);
> > +}
> > +
> > +static ssize_t asc_store(struct device *dev, struct device_attribute *attr,
> > +			 const char *buf, size_t len)
> > +{
> > +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> > +	struct scd30_state *state = iio_priv(indio_dev);
> > +	int ret;
> > +	u16 val;
> > +
> > +	if (kstrtou16(buf, 0, &val))
> > +		return -EINVAL;
> > +
> > +	val = !!val;
> > +	mutex_lock(&state->lock);
> > +	ret = scd30_command(state, CMD_ASC, val, NULL, 0);
> > +	mutex_unlock(&state->lock);
> > +
> > +	return ret ?: len;
> > +}
> > +
> > +static ssize_t frc_show(struct device *dev, struct device_attribute *attr,
> > +			char *buf)
> > +{
> > +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> > +	struct scd30_state *state = iio_priv(indio_dev);
> > +	u16 val;
> > +	int ret;
> > +
> > +	mutex_lock(&state->lock);
> > +	ret = scd30_command(state, CMD_FRC, 0, (char *)&val, sizeof(val));
> > +	mutex_unlock(&state->lock);
> > +
> > +	return ret ?: sprintf(buf, "%d\n", val);
> > +}
> > +
> > +static ssize_t frc_store(struct device *dev, struct device_attribute *attr,
> > +			 const char *buf, size_t len)
> > +{
> > +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> > +	struct scd30_state *state = iio_priv(indio_dev);
> > +	int ret;
> > +	u16 val;
> > +
> > +	if (kstrtou16(buf, 0, &val))
> > +		return -EINVAL;
> > +
> > +	if ((val < SCD30_FRC_MIN) || (val > SCD30_FRC_MAX))
> > +		return -EINVAL;
> > +
> > +	mutex_lock(&state->lock);
> > +	ret = scd30_command(state, CMD_FRC, val, NULL, 0);
> > +	mutex_unlock(&state->lock);
> > +
> > +	return ret ?: len;
> > +}
> > +
> > +static ssize_t frc_available_show(struct device *dev,
> > +				  struct device_attribute *attr, char *buf)
> > +{
> > +	return sprintf(buf, "[%d %d %d]\n", SCD30_FRC_MIN, 1, SCD30_FRC_MAX);
> > +}
> > +
> > +static ssize_t temp_offset_show(struct device *dev,
> > +				struct device_attribute *attr, char *buf)
> > +{
> > +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> > +	struct scd30_state *state = iio_priv(indio_dev);
> > +	int ret;
> > +	u16 val;
> > +
> > +	mutex_lock(&state->lock);
> > +	ret = scd30_command(state, CMD_TEMP_OFFSET, 0, (char *)&val,
> > +			    sizeof(val));
> > +	mutex_unlock(&state->lock);
> > +
> > +	return ret ?: sprintf(buf, "%d\n", val);
> > +}
> > +
> > +static ssize_t temp_offset_store(struct device *dev,
> > +				 struct device_attribute *attr, const char *buf,
> > +				 size_t len)
> > +{
> > +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> > +	struct scd30_state *state = iio_priv(indio_dev);
> > +	int ret;
> > +	u16 val;
> > +
> > +	if (kstrtou16(buf, 0, &val))
> > +		return -EINVAL;
> > +
> > +	/*
> > +	 * Manufacturer does not explicitly specify min/max sensible values
> > +	 * hence check is omitted for simplicity.
> > +	 */
> > +	mutex_lock(&state->lock);
> > +	ret = scd30_command(state, CMD_TEMP_OFFSET, val, NULL, 0);
> > +	mutex_unlock(&state->lock);
> > +
> > +	return ret ?: len;
> > +}
> > +
> > +static ssize_t reset_store(struct device *dev, struct device_attribute *attr,
> > +			   const char *buf, size_t len)
> > +{
> > +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> > +	struct scd30_state *state = iio_priv(indio_dev);
> > +	int ret;
> > +
> > +	mutex_lock(&state->lock);
> > +	/* after reset previous sensor state will be restored automatically */
> > +	ret = scd30_reset(state);
> > +	mutex_unlock(&state->lock);
> > +
> > +	return ret ?: len;
> > +}
> > +
> > +static IIO_DEVICE_ATTR_RW(pressure_comp, 0);
> > +static IIO_DEVICE_ATTR_RO(pressure_comp_available, 0);
> > +static IIO_DEVICE_ATTR_RW(meas_interval, 0);
> > +static IIO_DEVICE_ATTR_RO(meas_interval_available, 0);
> > +static IIO_DEVICE_ATTR_RW(asc, 0);
> > +static IIO_DEVICE_ATTR_RW(frc, 0);
> > +static IIO_DEVICE_ATTR_RO(frc_available, 0);
> > +static IIO_DEVICE_ATTR_RW(temp_offset, 0);
> > +static IIO_CONST_ATTR(temp_offset_available, "[0 1 65535]");
> > +static IIO_DEVICE_ATTR_WO(reset, 0);
> > +
> > +static struct attribute *scd30_attrs[] = {  
> 
> As mentioned previously all of these need documentation.
> I'll take a guess at what they are and offer some quick comments though
> 
> > +	&iio_dev_attr_pressure_comp.dev_attr.attr,
> > +	&iio_dev_attr_pressure_comp_available.dev_attr.attr,  
> These look to be pressure values to allow for compensation?
> Hmm. There is some similar ABI in a few drivers but I'm not sure anything
> exactly matches that one.  We could do it as an output channel.
> 
> > +	&iio_dev_attr_meas_interval.dev_attr.attr,
> > +	&iio_dev_attr_meas_interval_available.dev_attr.attr,  
> 
> Interval is inverse of sampling freqency?
> Do the maths to use that instead.
> 
> > +	&iio_dev_attr_asc.dev_attr.attr,  
> This is very device specific so may needs special ABI. However
> definitely needs to be written out long hand rather than an acronym
> that will have people reaching for the manual.
> 
> > +	&iio_dev_attr_frc.dev_attr.attr,
> > +	&iio_dev_attr_frc_available.dev_attr.attr,  
> 
> > +	&iio_dev_attr_temp_offset.dev_attr.attr,  
> This one looks like a calibration parameter on the temperature
> measurement. We have standard ABI for that.
> > +	&iio_const_attr_temp_offset_available.dev_attr.attr,
> > +	&iio_dev_attr_reset.dev_attr.attr,  
> 
> Need a strong reason to support reset as a userspace ABI.
> Normally we restrict that to device startup.
> 
> 
> > +	NULL
> > +};
> > +
> > +static const struct attribute_group scd30_attr_group = {
> > +	.attrs = scd30_attrs,
> > +};
> > +
> > +static const struct iio_info scd30_info = {
> > +	.attrs = &scd30_attr_group,
> > +	.read_raw = scd30_read_raw,
> > +};
> > +
> > +#define SCD30_CHAN(_type, _index) \
> > +	.type = _type, \
> > +	.address = _index, \
> > +	.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED), \
> > +	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \
> > +	.scan_index = _index
> > +
> > +#define SCD30_CHAN_SCAN_TYPE(_sign, _realbits) .scan_type = { \
> > +	.sign = _sign, \
> > +	.realbits = _realbits, \
> > +	.storagebits = 32, \
> > +	.endianness = IIO_CPU, \
> > +}
> > +
> > +static const struct iio_chan_spec scd30_channels[] = {
> > +	{
> > +		SCD30_CHAN(IIO_CONCENTRATION, CONC),
> > +		SCD30_CHAN_SCAN_TYPE('u', 16),
> > +	},
> > +	{
> > +		SCD30_CHAN(IIO_TEMP, TEMP),
> > +		SCD30_CHAN_SCAN_TYPE('s', 14),
> > +	},
> > +	{
> > +		SCD30_CHAN(IIO_HUMIDITYRELATIVE, HR),
> > +		SCD30_CHAN_SCAN_TYPE('u', 14),
> > +	},
> > +	IIO_CHAN_SOFT_TIMESTAMP(3),
> > +};
> > +
> > +int __maybe_unused scd30_suspend(struct device *dev)
> > +{
> > +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> > +	struct scd30_state *state  = iio_priv(indio_dev);
> > +	int ret;
> > +
> > +	ret = scd30_command(state, CMD_STOP_MEAS, 0, NULL, 0);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return regulator_disable(state->vdd);
> > +}
> > +EXPORT_SYMBOL(scd30_suspend);
> > +
> > +int __maybe_unused scd30_resume(struct device *dev)
> > +{
> > +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> > +	struct scd30_state *state = iio_priv(indio_dev);
> > +	int ret;
> > +
> > +	ret = regulator_enable(state->vdd);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return scd30_command(state, CMD_START_MEAS, state->pressure_comp,
> > +			     NULL, 0);
> > +}
> > +EXPORT_SYMBOL(scd30_resume);
> > +
> > +static void scd30_exit(void *data)
> > +{
> > +	struct scd30_state *state = data;
> > +
> > +	scd30_command(state, CMD_STOP_MEAS, 0, NULL, 0);
> > +	regulator_disable(state->vdd);
> > +}
> > +
> > +static irqreturn_t scd30_irq_handler(int irq, void *priv)
> > +{
> > +	struct iio_dev *indio_dev = priv;
> > +
> > +	if (iio_buffer_enabled(indio_dev)) {
> > +		iio_trigger_poll(indio_dev->trig);
> > +
> > +		return IRQ_HANDLED;
> > +	}
> > +
> > +	return IRQ_WAKE_THREAD;
> > +}
> > +
> > +static irqreturn_t scd30_irq_thread_handler(int irq, void *priv)
> > +{
> > +	struct iio_dev *indio_dev = priv;
> > +	struct scd30_state *state = iio_priv(indio_dev);
> > +	int ret;
> > +
> > +	ret = scd30_read_meas(state);
> > +	if (ret)
> > +		goto out;
> > +
> > +	complete_all(&state->meas_ready);
> > +out:
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +static irqreturn_t scd30_trigger_handler(int irq, void *p)
> > +{
> > +	struct iio_poll_func *pf = p;
> > +	struct iio_dev *indio_dev = pf->indio_dev;
> > +	struct scd30_state *state = iio_priv(indio_dev);
> > +	/* co2 concentration, temperature, rh, padding, timestamp */
> > +	int data[SCD30_MEAS_COUNT + 1 + 2], ret = 0;
> > +
> > +	mutex_lock(&state->lock);
> > +	if (!iio_trigger_using_own(indio_dev))
> > +		ret = scd30_read_poll(state);
> > +	else
> > +		ret = scd30_read_meas(state);
> > +	memcpy(data, state->meas, sizeof(state->meas));
> > +	mutex_unlock(&state->lock);
> > +	if (ret)
> > +		goto out;
> > +
> > +	iio_push_to_buffers_with_timestamp(indio_dev, data,
> > +					   iio_get_time_ns(indio_dev));
> > +out:
> > +	iio_trigger_notify_done(indio_dev->trig);
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +static int scd30_set_trigger_state(struct iio_trigger *trig, bool state)
> > +{
> > +	struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
> > +	struct scd30_state *st = iio_priv(indio_dev);
> > +
> > +	if (state)
> > +		enable_irq(st->irq);
> > +	else
> > +		disable_irq(st->irq);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct iio_trigger_ops scd30_trigger_ops = {
> > +	.set_trigger_state = scd30_set_trigger_state,
> > +};
> > +
> > +static int scd30_setup_trigger(struct iio_dev *indio_dev)
> > +{
> > +	struct scd30_state *state = iio_priv(indio_dev);
> > +	struct device *dev = indio_dev->dev.parent;
> > +	struct iio_trigger *trig;
> > +	int ret;
> > +
> > +	trig = devm_iio_trigger_alloc(dev, "%s-dev%d", indio_dev->name,
> > +				      indio_dev->id);
> > +	if (!trig) {
> > +		dev_err(dev, "failed to allocate trigger\n");
> > +		return -ENOMEM;
> > +	}
> > +
> > +	trig->dev.parent = dev;
> > +	trig->ops = &scd30_trigger_ops;
> > +	iio_trigger_set_drvdata(trig, indio_dev);
> > +
> > +	ret = devm_iio_trigger_register(dev, trig);
> > +	if (ret)
> > +		return ret;
> > +
> > +	indio_dev->trig = iio_trigger_get(trig);
> > +
> > +	ret = devm_request_threaded_irq(dev, state->irq, scd30_irq_handler,
> > +					scd30_irq_thread_handler,
> > +					IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
> > +					indio_dev->name, indio_dev);
> > +	if (ret)
> > +		dev_err(dev, "failed to request irq\n");  
> 
> I'm guessing this is a device without any means to disable the interrupt
> being generated?  In which case are you safe against a race before you
> disable here?
> 
> > +
> > +	disable_irq(state->irq);
> > +
> > +	return ret;
> > +}
> > +
> > +int scd30_probe(struct device *dev, int irq, const char *name, void *priv,
> > +		int (*command)(struct scd30_state *state, enum scd30_cmd cmd,
> > +			       u16 arg, char *rsp, int size))
> > +{
> > +	static const unsigned long scd30_scan_masks[] = { 0x07, 0x00 };
> > +	struct scd30_state *state;
> > +	struct iio_dev *indio_dev;
> > +	int ret;
> > +	u16 val;
> > +
> > +	indio_dev = devm_iio_device_alloc(dev, sizeof(*state));
> > +	if (!indio_dev)
> > +		return -ENOMEM;
> > +
> > +	dev_set_drvdata(dev, indio_dev);
> > +
> > +	state = iio_priv(indio_dev);
> > +	state->dev = dev;
> > +	state->priv = priv;
> > +	state->irq = irq;
> > +	state->pressure_comp = SCD30_PRESSURE_COMP_DEFAULT;
> > +	state->meas_interval = SCD30_MEAS_INTERVAL_DEFAULT;
> > +	state->command = command;
> > +	mutex_init(&state->lock);
> > +	init_completion(&state->meas_ready);
> > +
> > +	indio_dev->dev.parent = dev;
> > +	indio_dev->info = &scd30_info;
> > +	indio_dev->name = name;
> > +	indio_dev->channels = scd30_channels;
> > +	indio_dev->num_channels = ARRAY_SIZE(scd30_channels);
> > +	indio_dev->modes = INDIO_DIRECT_MODE;
> > +	indio_dev->available_scan_masks = scd30_scan_masks;
> > +
> > +	state->vdd = devm_regulator_get(dev, "vdd");
> > +	if (IS_ERR(state->vdd)) {  
> 
> This is very noisy if we have deferred probing going on.
> Either explicitly check for that case or just don't bother
> with an error message in this path.
> 
> > +		dev_err(dev, "failed to get vdd regulator\n");
> > +		return PTR_ERR(state->vdd);
> > +	}
> > +
> > +	ret = regulator_enable(state->vdd);
> > +	if (ret) {
> > +		dev_err(dev, "failed to enable vdd regulator\n");
> > +		return ret;
> > +	}
> > +
> > +	ret = devm_add_action_or_reset(dev, scd30_exit, state);
> > +	if (ret)  
> 
> This should match exactly against the item above it. Whilst stop
> measurement may be safe from here on, it is not easy to review
> unless we can clearly see where the equivalent start is.
> 
> > +		return ret;
> > +
> > +	ret = scd30_reset(state);
> > +	if (ret) {
> > +		dev_err(dev, "failed to reset device: %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	if (state->irq > 0) {
> > +		ret = scd30_setup_trigger(indio_dev);
> > +		if (ret) {
> > +			dev_err(dev, "failed to setup trigger: %d\n", ret);
> > +			return ret;
> > +		}
> > +	}
> > +
> > +	ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL,
> > +					      scd30_trigger_handler, NULL);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = scd30_command(state, CMD_FW_VERSION, 0, (char *)&val,
> > +			    sizeof(val));
> > +	if (ret) {
> > +		dev_err(dev, "failed to read firmware version: %d\n", ret);
> > +		return ret;
> > +	}
> > +	dev_info(dev, "firmware version: %d.%d\n", val >> 8, (char)val);
> > +
> > +	ret = scd30_command(state, CMD_MEAS_INTERVAL, state->meas_interval,
> > +			    NULL, 0);
> > +	if (ret) {
> > +		dev_err(dev, "failed to set measurement interval: %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	ret = scd30_command(state, CMD_START_MEAS, state->pressure_comp,
> > +			    NULL, 0);
> > +	if (ret) {
> > +		dev_err(dev, "failed to start measurement: %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	return devm_iio_device_register(dev, indio_dev);
> > +}
> > +EXPORT_SYMBOL(scd30_probe);
> > +
> > +MODULE_AUTHOR("Tomasz Duszynski <tomasz.duszynski@...akon.com>");
> > +MODULE_DESCRIPTION("Sensirion SCD30 carbon dioxide sensor core driver");
> > +MODULE_LICENSE("GPL v2");  
> 

Powered by blists - more mailing lists