[PATCH 0/6] Add support for SCD30 sensor

[PATCH 0/6] Add support for SCD30 sensor

[Tomasz Duszynski]

Following series adds support for Sensirion SCD30 sensor module capable of
measuring carbon dioxide, temperature and relative humidity. CO2 measurements
base on NDIR principle while temperature and relative humidity are measured by
the on board SHT31. As for sensor communication, both I2C and serial interfaces
are supported.

Tomasz Duszynski (6):
  iio: chemical: scd30: add core driver
  iio: chemical: scd30: add I2C interface driver
  iio: chemical: scd30: add serial interface driver
  Documentation: ABI: testing: scd30: document iio attributes
  dt-bindings: iio: scd30: add device binding file
  MAINTAINERS: add myself as a SCD30 driver maintainer

 Documentation/ABI/testing/sysfs-bus-iio-scd30 |  97 +++
 .../iio/chemical/sensirion,scd30.yaml         |  71 ++
 MAINTAINERS                                   |   9 +
 drivers/iio/chemical/Kconfig                  |  33 +
 drivers/iio/chemical/Makefile                 |   3 +
 drivers/iio/chemical/scd30.h                  |  72 ++
 drivers/iio/chemical/scd30_core.c             | 796 ++++++++++++++++++
 drivers/iio/chemical/scd30_i2c.c              | 141 ++++
 drivers/iio/chemical/scd30_serial.c           | 262 ++++++
 9 files changed, 1484 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-scd30
 create mode 100644 Documentation/devicetree/bindings/iio/chemical/sensirion,scd30.yaml
 create mode 100644 drivers/iio/chemical/scd30.h
 create mode 100644 drivers/iio/chemical/scd30_core.c
 create mode 100644 drivers/iio/chemical/scd30_i2c.c
 create mode 100644 drivers/iio/chemical/scd30_serial.c

--
2.26.1

[PATCH 1/6] iio: chemical: scd30: add core driver

[Tomasz Duszynski]

Add Sensirion SCD30 carbon dioxide core driver.

Signed-off-by: Tomasz Duszynski <tomasz.duszynski@octakon.com>
---
 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>
+#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@octakon.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);
+	}
+
+	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]);
+
+	/*
+	 * 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);
+	ret = wait_for_completion_interruptible_timeout(&state->meas_ready,
+							timeout);
+	if (ret > 0)
+		ret = 0;
+	else if (!ret)
+		ret = -ETIMEDOUT;
+
+	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[] = {
+	&iio_dev_attr_pressure_comp.dev_attr.attr,
+	&iio_dev_attr_pressure_comp_available.dev_attr.attr,
+	&iio_dev_attr_meas_interval.dev_attr.attr,
+	&iio_dev_attr_meas_interval_available.dev_attr.attr,
+	&iio_dev_attr_asc.dev_attr.attr,
+	&iio_dev_attr_frc.dev_attr.attr,
+	&iio_dev_attr_frc_available.dev_attr.attr,
+	&iio_dev_attr_temp_offset.dev_attr.attr,
+	&iio_const_attr_temp_offset_available.dev_attr.attr,
+	&iio_dev_attr_reset.dev_attr.attr,
+	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");
+
+	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)) {
+		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)
+		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@octakon.com>");
+MODULE_DESCRIPTION("Sensirion SCD30 carbon dioxide sensor core driver");
+MODULE_LICENSE("GPL v2");
-- 
2.26.1

Re: [PATCH 1/6] iio: chemical: scd30: add core driver

[Andy Shevchenko]

On Wed, Apr 22, 2020 at 5:22 PM Tomasz Duszynski
<tomasz.duszynski@octakon.com> wrote:
>
> Add Sensirion SCD30 carbon dioxide core driver.

And DocLink tar of Datasheet: with a link?

...

> +static SIMPLE_DEV_PM_OPS(scd30_pm_ops, scd30_suspend, scd30_resume);

Would it be used in every module? You will get a compiler warning per
each module that is not using it.

...

> +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));

My gosh.
Please, supply proper structure member in priv or alike.

...

> + * Copyright (c) Tomasz Duszynski <tomasz.duszynski@octakon.com>

Year?

...

> +#include <asm/byteorder.h>

asm goes after linux.

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

Are you sure you need all of them?!

...

> +/* pressure compensation in millibars */
Put the unit as a suffix to each definition and drop useless comment.

> +/* measurement interval in seconds */

Ditto.

> +/* reference CO2 concentration in ppm */

Ditto.

> +enum {
> +       CONC,
> +       TEMP,
> +       HR,
> +};

Way too generic names for anonymous enum.

...

> +static int scd30_command(struct scd30_state *state, enum scd30_cmd cmd, u16 arg,
> +                        char *rsp, int size)
> +{

> +       /*
> +        * 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);

Seems like rsp should be void * rather than char *.

> +               rsp += sizeof(u32);
> +               size -= sizeof(u32);
> +       }

NIH of https://elixir.bootlin.com/linux/v5.7-rc2/ident/be32_to_cpu_array ?

> +       if (size)

It can be done before even while loop with an immediate bail out.

> +               *(u16 *)rsp = be16_to_cpup((__be16 *)rsp);
> +
> +       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);

  shift = 23 + exp;
  ... >> shift);

> +       }
> +
> +       /* 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));
> +}

Sounds like a candidate to IIO library or even lib/math/*.c.

...

> +static int scd30_wait_meas_irq(struct scd30_state *state)
> +{
> +       int ret, timeout = msecs_to_jiffies(state->meas_interval * 1250);

Magic number.

> +       reinit_completion(&state->meas_ready);
> +       enable_irq(state->irq);
> +       ret = wait_for_completion_interruptible_timeout(&state->meas_ready,
> +                                                       timeout);
> +       if (ret > 0)
> +               ret = 0;
> +       else if (!ret)
> +               ret = -ETIMEDOUT;
> +
> +       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;

unsigned int tries = ...;

do {
 ...
} while (--tries);
if (!tries)
  return ...;

looks better and I guess less code in asm.

> +       return 0;
> +}

...

> +       if (kstrtou16(buf, 0, &val))
> +               return -EINVAL;

Shadowed error code. Don't do like this.

> +       if (kstrtou16(buf, 0, &val))
> +               return -EINVAL;

Ditto.

> +       if (kstrtou16(buf, 0, &val))
> +               return -EINVAL;

Ditto.

> +       val = !!val;

kstrtobool()?

...

> +       if (kstrtou16(buf, 0, &val))
> +               return -EINVAL;

No shadowed error code, please. Check entire code.

> +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);

Do you need all of them? Doesn't  IIO core provides a tons of helpers for these?
Btw, where is ABI documentation? It's a show stopper.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/6] iio: chemical: scd30: add core driver

[Tomasz Duszynski]

On Wed, Apr 22, 2020 at 10:49:44PM +0300, Andy Shevchenko wrote:
> On Wed, Apr 22, 2020 at 5:22 PM Tomasz Duszynski
> <tomasz.duszynski@octakon.com> wrote:
> >
> > Add Sensirion SCD30 carbon dioxide core driver.
>
> And DocLink tar of Datasheet: with a link?
>

I never do this. These files change their location way too often to be
worthwhile putting here. Nobody has that much time to fallow all this
and keep respective files up to date.

But that doesn't mean I can't drop a link here.
https://developer.sensirion.com/fileadmin/user_upload/customers/sensirion/Dokumente/9.5_CO2/Sensirion_CO2_Sensors_SCD30_Interface_Description.pdf

> ...
>
> > +static SIMPLE_DEV_PM_OPS(scd30_pm_ops, scd30_suspend, scd30_resume);
>
> Would it be used in every module? You will get a compiler warning per
> each module that is not using it.
>

Good point.

> ...
>
> > +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));
>
> My gosh.
> Please, supply proper structure member in priv or alike.
>

Not sure it's worth the fuss. Wrapping all into structure means either
copying respective members or more dereferences later on.

> ...
>
> > + * Copyright (c) Tomasz Duszynski <tomasz.duszynski@octakon.com>
>
> Year?
>

Okay.

> ...
>
> > +#include <asm/byteorder.h>
>
> asm goes after linux.

Right.

>
> > +#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>
>
> Are you sure you need all of them?!
>

Generally each exports something referenced throught the code.

> ...
>
> > +/* pressure compensation in millibars */
> Put the unit as a suffix to each definition and drop useless comment.

Okay.

>
> > +/* measurement interval in seconds */
>
> Ditto.
>
> > +/* reference CO2 concentration in ppm */
>
> Ditto.
>
> > +enum {
> > +       CONC,
> > +       TEMP,
> > +       HR,
> > +};
>
> Way too generic names for anonymous enum.

I'd argue that they are pretty well understood abbreviations in iio generally
and here specifically. But adding some prefix won't harm.

>
> ...
>
> > +static int scd30_command(struct scd30_state *state, enum scd30_cmd cmd, u16 arg,
> > +                        char *rsp, int size)
> > +{
>
> > +       /*
> > +        * 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);
>
> Seems like rsp should be void * rather than char *.
>

Might be. That would save a few casts.

> > +               rsp += sizeof(u32);
> > +               size -= sizeof(u32);
> > +       }
>
> NIH of https://elixir.bootlin.com/linux/v5.7-rc2/ident/be32_to_cpu_array ?

Okay.

>
> > +       if (size)
>
> It can be done before even while loop with an immediate bail out.
>

Okay.

> > +               *(u16 *)rsp = be16_to_cpup((__be16 *)rsp);
> > +
> > +       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);
>
>   shift = 23 + exp;
>   ... >> shift);
>
> > +       }
> > +
> > +       /* 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));
> > +}
>
> Sounds like a candidate to IIO library or even lib/math/*.c.
>

I really doubt it could prove useful to any driver except maybe a few
specific ones which tend to return results as a float (here I mean this
sensors and sps30).

But still with the above reasoning put aside that helper would need
substantial rework to handle rounding errors, precision, etc. before
actual inclusion.

> ...
>
> > +static int scd30_wait_meas_irq(struct scd30_state *state)
> > +{
> > +       int ret, timeout = msecs_to_jiffies(state->meas_interval * 1250);
>
> Magic number.
>

Okay.

> > +       reinit_completion(&state->meas_ready);
> > +       enable_irq(state->irq);
> > +       ret = wait_for_completion_interruptible_timeout(&state->meas_ready,
> > +                                                       timeout);
> > +       if (ret > 0)
> > +               ret = 0;
> > +       else if (!ret)
> > +               ret = -ETIMEDOUT;
> > +
> > +       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;
>
> unsigned int tries = ...;
>
> do {
>  ...
> } while (--tries);
> if (!tries)
>   return ...;
>
> looks better and I guess less code in asm.
>

You mean that one extra branch in case of while? But it comes to code
itself it looks more compact. And I am okay with that.

> > +       return 0;
> > +}
>
> ...
>
> > +       if (kstrtou16(buf, 0, &val))
> > +               return -EINVAL;
>
> Shadowed error code. Don't do like this.
>

Integer parsing either returns EINVAL or ERANGE. Passing the latter to
the user is not worth the trouble, especially because majority of writable attrs
have a fellow _available attr.

> > +       if (kstrtou16(buf, 0, &val))
> > +               return -EINVAL;
>
> Ditto.
>
> > +       if (kstrtou16(buf, 0, &val))
> > +               return -EINVAL;
>
> Ditto.
>
> > +       val = !!val;
>
> kstrtobool()?
>

That would need casting to u16 anyway. So both approaches are more or
less equivalent.

> ...
>
> > +       if (kstrtou16(buf, 0, &val))
> > +               return -EINVAL;
>
> No shadowed error code, please. Check entire code.
>
> > +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);
>
> Do you need all of them? Doesn't  IIO core provides a tons of helpers for these?
> Btw, where is ABI documentation? It's a show stopper.

They are sensor specific and none falls into a category of iio generic
attrs. Maybe, except the measurement interval which could be represented as
a SAMP_FREQ. But given that measurement interval spans from 2s to 1800s
it becomes a little bit awkward to have it in Hz. As for ABI that's in
a separate patch.


Thanks for review.

>
> --
> With Best Regards,
> Andy Shevchenko

Re: [PATCH 1/6] iio: chemical: scd30: add core driver

[Andy Shevchenko]

On Fri, Apr 24, 2020 at 10:05 PM Tomasz Duszynski
<tomasz.duszynski@octakon.com> wrote:
> On Wed, Apr 22, 2020 at 10:49:44PM +0300, Andy Shevchenko wrote:
> > On Wed, Apr 22, 2020 at 5:22 PM Tomasz Duszynski
> > <tomasz.duszynski@octakon.com> wrote:

...

> > > Add Sensirion SCD30 carbon dioxide core driver.
> >
> > And DocLink tar of Datasheet: with a link?
>
> I never do this. These files change their location way too often to be
> worthwhile putting here. Nobody has that much time to fallow all this
> and keep respective files up to date.
>
> But that doesn't mean I can't drop a link here.
> https://developer.sensirion.com/fileadmin/user_upload/customers/sensirion/Dokumente/9.5_CO2/Sensirion_CO2_Sensors_SCD30_Interface_Description.pdf

Yes, just make it a tag

DocLink: ....

...

> > > +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));
> >
> > My gosh.
> > Please, supply proper structure member in priv or alike.
>
> Not sure it's worth the fuss. Wrapping all into structure means either
> copying respective members or more dereferences later on.

At least you may introduce a typedef, because above really hurts my eyes.

...

> > > +enum {
> > > +       CONC,
> > > +       TEMP,
> > > +       HR,
> > > +};
> >
> > Way too generic names for anonymous enum.
>
> I'd argue that they are pretty well understood abbreviations in iio generally
> and here specifically. But adding some prefix won't harm.

Yes, prefix is what I was talking about.

...

> > > +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;
> >
> > unsigned int tries = ...;
> >
> > do {
> >  ...
> > } while (--tries);
> > if (!tries)
> >   return ...;
> >
> > looks better and I guess less code in asm.
> >
>
> You mean that one extra branch in case of while?

There are few things:
a) do {} while notation immediately tells that at least one cycle of
body will be done (unconditionally);
b) it makes a loop variable unsigned and no need to check for specific
negative numbers;
c) it quite likely will generate slightly better assembly code.

>  But it comes to code
> itself it looks more compact. And I am okay with that.
>
> > > +       return 0;
> > > +}

...

> > > +       if (kstrtou16(buf, 0, &val))
> > > +               return -EINVAL;
> >
> > Shadowed error code. Don't do like this.
>
> Integer parsing either returns EINVAL or ERANGE. Passing the latter to
> the user is not worth the trouble, especially because majority of writable attrs
> have a fellow _available attr.

It's simple a bad coding practice. Please, change.

> > > +       if (kstrtou16(buf, 0, &val))
> > > +               return -EINVAL;
> >
> > Ditto.
> >
> > > +       if (kstrtou16(buf, 0, &val))
> > > +               return -EINVAL;
> >
> > Ditto.

...

> > > +       if (kstrtou16(buf, 0, &val))
> > > +               return -EINVAL;
> >
> > No shadowed error code, please. Check entire code.

Same here.

...

> > > +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);
> >
> > Do you need all of them? Doesn't  IIO core provides a tons of helpers for these?
> > Btw, where is ABI documentation? It's a show stopper.
>
> They are sensor specific and none falls into a category of iio generic
> attrs. Maybe, except the measurement interval which could be represented as
> a SAMP_FREQ.

IIO ABI becomes already a big pile of nodes and I hope we will become
stricter about adding new ones.

> But given that measurement interval spans from 2s to 1800s
> it becomes a little bit awkward to have it in Hz.

> As for ABI that's in
> a separate patch.

It's not good from bisectability point of view. If by some reason this
patch or documentation patch gets reverted, the other one will be
dangling.
Please, unify them.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/6] iio: chemical: scd30: add core driver

[Jonathan Cameron]

On Sat, 25 Apr 2020 14:43:35 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Fri, Apr 24, 2020 at 10:05 PM Tomasz Duszynski
> <tomasz.duszynski@octakon.com> wrote:
> > On Wed, Apr 22, 2020 at 10:49:44PM +0300, Andy Shevchenko wrote:  
> > > On Wed, Apr 22, 2020 at 5:22 PM Tomasz Duszynski
> > > <tomasz.duszynski@octakon.com> wrote:  
> 
> ...
> 
> > > > Add Sensirion SCD30 carbon dioxide core driver.  
> > >
> > > And DocLink tar of Datasheet: with a link?  
> >
> > I never do this. These files change their location way too often to be
> > worthwhile putting here. Nobody has that much time to fallow all this
> > and keep respective files up to date.
> >
> > But that doesn't mean I can't drop a link here.
> > https://developer.sensirion.com/fileadmin/user_upload/customers/sensirion/Dokumente/9.5_CO2/Sensirion_CO2_Sensors_SCD30_Interface_Description.pdf  
> 
> Yes, just make it a tag
> 
> DocLink: ....
> 
> ...
> 
> > > > +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));  
> > >
> > > My gosh.
> > > Please, supply proper structure member in priv or alike.  
> >
> > Not sure it's worth the fuss. Wrapping all into structure means either
> > copying respective members or more dereferences later on.  
> 
> At least you may introduce a typedef, because above really hurts my eyes.
> 
> ...
> 
> > > > +enum {
> > > > +       CONC,
> > > > +       TEMP,
> > > > +       HR,
> > > > +};  
> > >
> > > Way too generic names for anonymous enum.  
> >
> > I'd argue that they are pretty well understood abbreviations in iio generally
> > and here specifically. But adding some prefix won't harm.  
> 
> Yes, prefix is what I was talking about.
> 
> ...
> 
> > > > +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;  
> > >
> > > unsigned int tries = ...;
> > >
> > > do {
> > >  ...
> > > } while (--tries);
> > > if (!tries)
> > >   return ...;
> > >
> > > looks better and I guess less code in asm.
> > >  
> >
> > You mean that one extra branch in case of while?  
> 
> There are few things:
> a) do {} while notation immediately tells that at least one cycle of
> body will be done (unconditionally);
> b) it makes a loop variable unsigned and no need to check for specific
> negative numbers;
> c) it quite likely will generate slightly better assembly code.
> 
> >  But it comes to code
> > itself it looks more compact. And I am okay with that.
> >  
> > > > +       return 0;
> > > > +}  
> 
> ...
> 
> > > > +       if (kstrtou16(buf, 0, &val))
> > > > +               return -EINVAL;  
> > >
> > > Shadowed error code. Don't do like this.  
> >
> > Integer parsing either returns EINVAL or ERANGE. Passing the latter to
> > the user is not worth the trouble, especially because majority of writable attrs
> > have a fellow _available attr.  
> 
> It's simple a bad coding practice. Please, change.
> 
> > > > +       if (kstrtou16(buf, 0, &val))
> > > > +               return -EINVAL;  
> > >
> > > Ditto.
> > >  
> > > > +       if (kstrtou16(buf, 0, &val))
> > > > +               return -EINVAL;  
> > >
> > > Ditto.  
> 
> ...
> 
> > > > +       if (kstrtou16(buf, 0, &val))
> > > > +               return -EINVAL;  
> > >
> > > No shadowed error code, please. Check entire code.  
> 
> Same here.
> 
> ...
> 
> > > > +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);  
> > >
> > > Do you need all of them? Doesn't  IIO core provides a tons of helpers for these?
> > > Btw, where is ABI documentation? It's a show stopper.  
> >
> > They are sensor specific and none falls into a category of iio generic
> > attrs. Maybe, except the measurement interval which could be represented as
> > a SAMP_FREQ.  
> 
> IIO ABI becomes already a big pile of nodes and I hope we will become
> stricter about adding new ones.
Yes.  Starting point is they need to be documented or they can't be
properly reviewed.

Documentation/ABI/testing/sysfs-bus-iio-*

> 
> > But given that measurement interval spans from 2s to 1800s
> > it becomes a little bit awkward to have it in Hz.  
> 
> > As for ABI that's in
> > a separate patch.  
> 
> It's not good from bisectability point of view. If by some reason this
> patch or documentation patch gets reverted, the other one will be
> dangling.
> Please, unify them.
> 

Re: [PATCH 1/6] iio: chemical: scd30: add core driver

[Tomasz Duszynski]

On Sat, Apr 25, 2020 at 02:43:35PM +0300, Andy Shevchenko wrote:
> On Fri, Apr 24, 2020 at 10:05 PM Tomasz Duszynski
> <tomasz.duszynski@octakon.com> wrote:
> > On Wed, Apr 22, 2020 at 10:49:44PM +0300, Andy Shevchenko wrote:
> > > On Wed, Apr 22, 2020 at 5:22 PM Tomasz Duszynski
> > > <tomasz.duszynski@octakon.com> wrote:
>
> ...
>
> > > > Add Sensirion SCD30 carbon dioxide core driver.
> > >
> > > And DocLink tar of Datasheet: with a link?
> >
> > I never do this. These files change their location way too often to be
> > worthwhile putting here. Nobody has that much time to fallow all this
> > and keep respective files up to date.
> >
> > But that doesn't mean I can't drop a link here.
> > https://developer.sensirion.com/fileadmin/user_upload/customers/sensirion/Dokumente/9.5_CO2/Sensirion_CO2_Sensors_SCD30_Interface_Description.pdf
>
> Yes, just make it a tag
>
> DocLink: ....
>
> ...
>
> > > > +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));
> > >
> > > My gosh.
> > > Please, supply proper structure member in priv or alike.
> >
> > Not sure it's worth the fuss. Wrapping all into structure means either
> > copying respective members or more dereferences later on.
>
> At least you may introduce a typedef, because above really hurts my eyes.
>

May be.

> ...
>
> > > > +enum {
> > > > +       CONC,
> > > > +       TEMP,
> > > > +       HR,
> > > > +};
> > >
> > > Way too generic names for anonymous enum.
> >
> > I'd argue that they are pretty well understood abbreviations in iio generally
> > and here specifically. But adding some prefix won't harm.
>
> Yes, prefix is what I was talking about.
>
> ...
>
> > > > +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;
> > >
> > > unsigned int tries = ...;
> > >
> > > do {
> > >  ...
> > > } while (--tries);
> > > if (!tries)
> > >   return ...;
> > >
> > > looks better and I guess less code in asm.
> > >
> >
> > You mean that one extra branch in case of while?
>
> There are few things:
> a) do {} while notation immediately tells that at least one cycle of
> body will be done (unconditionally);
> b) it makes a loop variable unsigned and no need to check for specific
> negative numbers;
> c) it quite likely will generate slightly better assembly code.
>
> >  But it comes to code
> > itself it looks more compact. And I am okay with that.
> >
> > > > +       return 0;
> > > > +}
>
> ...
>
> > > > +       if (kstrtou16(buf, 0, &val))
> > > > +               return -EINVAL;
> > >
> > > Shadowed error code. Don't do like this.
> >
> > Integer parsing either returns EINVAL or ERANGE. Passing the latter to
> > the user is not worth the trouble, especially because majority of writable attrs
> > have a fellow _available attr.
>
> It's simple a bad coding practice. Please, change.
>

Fair enough.

> > > > +       if (kstrtou16(buf, 0, &val))
> > > > +               return -EINVAL;
> > >
> > > Ditto.
> > >
> > > > +       if (kstrtou16(buf, 0, &val))
> > > > +               return -EINVAL;
> > >
> > > Ditto.
>
> ...
>
> > > > +       if (kstrtou16(buf, 0, &val))
> > > > +               return -EINVAL;
> > >
> > > No shadowed error code, please. Check entire code.
>
> Same here.
>
> ...
>
> > > > +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);
> > >
> > > Do you need all of them? Doesn't  IIO core provides a tons of helpers for these?
> > > Btw, where is ABI documentation? It's a show stopper.
> >
> > They are sensor specific and none falls into a category of iio generic
> > attrs. Maybe, except the measurement interval which could be represented as
> > a SAMP_FREQ.
>
> IIO ABI becomes already a big pile of nodes and I hope we will become
> stricter about adding new ones.
>

Try persuading vendors to use unified interfaces and problem will
disappear completely :).

> > But given that measurement interval spans from 2s to 1800s
> > it becomes a little bit awkward to have it in Hz.
>
> > As for ABI that's in
> > a separate patch.
>
> It's not good from bisectability point of view. If by some reason this
> patch or documentation patch gets reverted, the other one will be
> dangling.
> Please, unify them.
>

Huh? Reverting core and leaving leftovers would be wrong and pointless.

> --
> With Best Regards,
> Andy Shevchenko

Re: [PATCH 1/6] iio: chemical: scd30: add core driver

[Andy Shevchenko]

On Sat, Apr 25, 2020 at 9:42 PM Tomasz Duszynski
<tomasz.duszynski@octakon.com> wrote:
> On Sat, Apr 25, 2020 at 02:43:35PM +0300, Andy Shevchenko wrote:
> > On Fri, Apr 24, 2020 at 10:05 PM Tomasz Duszynski
> > <tomasz.duszynski@octakon.com> wrote:
> > > On Wed, Apr 22, 2020 at 10:49:44PM +0300, Andy Shevchenko wrote:
> > > > On Wed, Apr 22, 2020 at 5:22 PM Tomasz Duszynski
> > > > <tomasz.duszynski@octakon.com> wrote:

...

> > > As for ABI that's in
> > > a separate patch.
> >
> > It's not good from bisectability point of view. If by some reason this
> > patch or documentation patch gets reverted, the other one will be
> > dangling.
> > Please, unify them.
> >
>
> Huh? Reverting core and leaving leftovers would be wrong and pointless.

Exactly my point why it should be one patch. To secure impossibility
to do pointless reverts.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/6] iio: chemical: scd30: add core driver

[Tomasz Duszynski]

On Sat, Apr 25, 2020 at 09:52:25PM +0300, Andy Shevchenko wrote:
> On Sat, Apr 25, 2020 at 9:42 PM Tomasz Duszynski
> <tomasz.duszynski@octakon.com> wrote:
> > On Sat, Apr 25, 2020 at 02:43:35PM +0300, Andy Shevchenko wrote:
> > > On Fri, Apr 24, 2020 at 10:05 PM Tomasz Duszynski
> > > <tomasz.duszynski@octakon.com> wrote:
> > > > On Wed, Apr 22, 2020 at 10:49:44PM +0300, Andy Shevchenko wrote:
> > > > > On Wed, Apr 22, 2020 at 5:22 PM Tomasz Duszynski
> > > > > <tomasz.duszynski@octakon.com> wrote:
>
> ...
>
> > > > As for ABI that's in
> > > > a separate patch.
> > >
> > > It's not good from bisectability point of view. If by some reason this
> > > patch or documentation patch gets reverted, the other one will be
> > > dangling.
> > > Please, unify them.
> > >
> >
> > Huh? Reverting core and leaving leftovers would be wrong and pointless.
>
> Exactly my point why it should be one patch. To secure impossibility
> to do pointless reverts.
>

But the same applies to other driver parts like i2c or serial
interfaces. I don't buy it.

>
> --
> With Best Regards,
> Andy Shevchenko

Re: [PATCH 1/6] iio: chemical: scd30: add core driver

[Andy Shevchenko]

On Tue, Apr 28, 2020 at 10:57 AM Tomasz Duszynski
<tomasz.duszynski@octakon.com> wrote:
>
> On Sat, Apr 25, 2020 at 09:52:25PM +0300, Andy Shevchenko wrote:
> > On Sat, Apr 25, 2020 at 9:42 PM Tomasz Duszynski
> > <tomasz.duszynski@octakon.com> wrote:
> > > On Sat, Apr 25, 2020 at 02:43:35PM +0300, Andy Shevchenko wrote:
> > > > On Fri, Apr 24, 2020 at 10:05 PM Tomasz Duszynski
> > > > <tomasz.duszynski@octakon.com> wrote:
> > > > > On Wed, Apr 22, 2020 at 10:49:44PM +0300, Andy Shevchenko wrote:
> > > > > > On Wed, Apr 22, 2020 at 5:22 PM Tomasz Duszynski
> > > > > > <tomasz.duszynski@octakon.com> wrote:
> >
> > ...
> >
> > > > > As for ABI that's in
> > > > > a separate patch.
> > > >
> > > > It's not good from bisectability point of view. If by some reason this
> > > > patch or documentation patch gets reverted, the other one will be
> > > > dangling.
> > > > Please, unify them.
> > > >
> > >
> > > Huh? Reverting core and leaving leftovers would be wrong and pointless.
> >
> > Exactly my point why it should be one patch. To secure impossibility
> > to do pointless reverts.
> >
>
> But the same applies to other driver parts like i2c or serial
> interfaces. I don't buy it.

They won't compile without core driver, right? Absence of the
documentation OTOH doesn't prevent build.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/6] iio: chemical: scd30: add core driver

[Tomasz Duszynski]

On Tue, Apr 28, 2020 at 01:16:47PM +0300, Andy Shevchenko wrote:
> On Tue, Apr 28, 2020 at 10:57 AM Tomasz Duszynski
> <tomasz.duszynski@octakon.com> wrote:
> >
> > On Sat, Apr 25, 2020 at 09:52:25PM +0300, Andy Shevchenko wrote:
> > > On Sat, Apr 25, 2020 at 9:42 PM Tomasz Duszynski
> > > <tomasz.duszynski@octakon.com> wrote:
> > > > On Sat, Apr 25, 2020 at 02:43:35PM +0300, Andy Shevchenko wrote:
> > > > > On Fri, Apr 24, 2020 at 10:05 PM Tomasz Duszynski
> > > > > <tomasz.duszynski@octakon.com> wrote:
> > > > > > On Wed, Apr 22, 2020 at 10:49:44PM +0300, Andy Shevchenko wrote:
> > > > > > > On Wed, Apr 22, 2020 at 5:22 PM Tomasz Duszynski
> > > > > > > <tomasz.duszynski@octakon.com> wrote:
> > >
> > > ...
> > >
> > > > > > As for ABI that's in
> > > > > > a separate patch.
> > > > >
> > > > > It's not good from bisectability point of view. If by some reason this
> > > > > patch or documentation patch gets reverted, the other one will be
> > > > > dangling.
> > > > > Please, unify them.
> > > > >
> > > >
> > > > Huh? Reverting core and leaving leftovers would be wrong and pointless.
> > >
> > > Exactly my point why it should be one patch. To secure impossibility
> > > to do pointless reverts.
> > >
> >
> > But the same applies to other driver parts like i2c or serial
> > interfaces. I don't buy it.
>
> They won't compile without core driver, right? Absence of the
> documentation OTOH doesn't prevent build.
>

Fair enough.

>
> --
> With Best Regards,
> Andy Shevchenko

Re: [PATCH 1/6] iio: chemical: scd30: add core driver

[Jonathan Cameron]

On Wed, 22 Apr 2020 16:11:30 +0200
Tomasz Duszynski <tomasz.duszynski@octakon.com> wrote:

> Add Sensirion SCD30 carbon dioxide core driver.
> 
> Signed-off-by: Tomasz Duszynski <tomasz.duszynski@octakon.com>
Hi Tomasz

As you've probably guessed the big questions are around the custom ABI.

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@octakon.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@octakon.com>");
> +MODULE_DESCRIPTION("Sensirion SCD30 carbon dioxide sensor core driver");
> +MODULE_LICENSE("GPL v2");

Re: [PATCH 1/6] iio: chemical: scd30: add core driver

[Jonathan Cameron]

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

> On Wed, 22 Apr 2020 16:11:30 +0200
> Tomasz Duszynski <tomasz.duszynski@octakon.com> wrote:
> 
> > Add Sensirion SCD30 carbon dioxide core driver.
> > 
> > Signed-off-by: Tomasz Duszynski <tomasz.duszynski@octakon.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@octakon.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@octakon.com>");
> > +MODULE_DESCRIPTION("Sensirion SCD30 carbon dioxide sensor core driver");
> > +MODULE_LICENSE("GPL v2");  
> 

Re: [PATCH 1/6] iio: chemical: scd30: add core driver

[Tomasz Duszynski]

On Sat, Apr 25, 2020 at 07:55:34PM +0100, Jonathan Cameron wrote:
> On Wed, 22 Apr 2020 16:11:30 +0200
> Tomasz Duszynski <tomasz.duszynski@octakon.com> wrote:
>
> > Add Sensirion SCD30 carbon dioxide core driver.
> >
> > Signed-off-by: Tomasz Duszynski <tomasz.duszynski@octakon.com>
> Hi Tomasz
>
> As you've probably guessed the big questions are around the custom ABI.
>
> 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.
>

Will drop.

> > +#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@octakon.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.
>

Okay, will rework that.

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

Thanks for reminding me :).

In that case I admit that some float helper in iio would be a good thing to
have. Especially that there will be at least 2 sensors using it.

I'd work on that after this driver makes it into the tree.

How does it sound?

> > +
> > +	/*
> > +	 * 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'?
>

Yes, grab the fresh one. Moreover enabling interrupts only when necessary can
limit pointless buss traffic. Reason being irq is acknowledged by reading data
from sensor.

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

Correct. Timeout should not harm anybody.

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

IRQs can be actually disabled by telling device to stop taking measurements.
There is dedicated command for that. If irq fires off before being disabled
nothing bad should happen as everything necessary is in place already.

Another thing is that without disabling interrupt here we would get warning
about unbalanced irq whilst enabling trigger.

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

Okay.

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

Well, naming might be confusing. The thing is that sensor after being
powered up reverts itself to the much the same state it left.

If we have real regulator then scd30_exit would disable regulator and
that's it. But, in case of a dummy one and sensor starting in
continuous mode we waste power for no real reason (for example 19mA
at 0.5Hz).

So it's explanation for doing 2 things inside early on but not excuse
for unintuitive naming.

> > +		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@octakon.com>");
> > +MODULE_DESCRIPTION("Sensirion SCD30 carbon dioxide sensor core driver");
> > +MODULE_LICENSE("GPL v2");
>

Re: [PATCH 1/6] iio: chemical: scd30: add core driver

[Jonathan Cameron]

On Tue, 28 Apr 2020 09:51:01 +0200
Tomasz Duszynski <tomasz.duszynski@octakon.com> wrote:

> On Sat, Apr 25, 2020 at 07:55:34PM +0100, Jonathan Cameron wrote:
> > On Wed, 22 Apr 2020 16:11:30 +0200
> > Tomasz Duszynski <tomasz.duszynski@octakon.com> wrote:
> >  
> > > Add Sensirion SCD30 carbon dioxide core driver.
> > >
> > > Signed-off-by: Tomasz Duszynski <tomasz.duszynski@octakon.com>  
> > Hi Tomasz
> >
> > As you've probably guessed the big questions are around the custom ABI.
> >
> > Few other things inline.
> >
> > Jonathan
> >  
...

> > > +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.
> >  
> 
> Thanks for reminding me :).
> 
> In that case I admit that some float helper in iio would be a good thing to
> have. Especially that there will be at least 2 sensors using it.
> 
> I'd work on that after this driver makes it into the tree.
> 
> How does it sound?

The problem is that, if we do it in that order we have ABI for this
device that we should really maintain.  We can probably get away
with changing it on the basis the channel type is self describing anyway
but it's not ideal.  

So probably fine but not best practice...

> 
> > > +
> > > +	/*
> > > +	 * 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'?
> >  
> 
> Yes, grab the fresh one. Moreover enabling interrupts only when necessary can
> limit pointless buss traffic. Reason being irq is acknowledged by reading data
> from sensor.
> 

As mentioned below, it seems to me that we should really be starting this
device only when we want a reading.  Hence any interrupt (subject to possible
races) should be valid.  Hence we would not be enabling and disabling the
interrupt controller mask on this line.


> > > +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?
> >  
> 
> IRQs can be actually disabled by telling device to stop taking measurements.
> There is dedicated command for that. If irq fires off before being disabled
> nothing bad should happen as everything necessary is in place already.

Hmm. I wonder if we'd be better off starting it on demand - or only when running
with it as a data ready trigger. That would make the the polled read a case
of starting the sampling for one sample rather than just 'picking' one from
the stream of actual samples.

> 
> Another thing is that without disabling interrupt here we would get warning
> about unbalanced irq whilst enabling trigger.
> 
> > > +
> > > +	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.
> >  
> 
> Okay.
> 
> > > +		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.
> >  
> 
> Well, naming might be confusing. The thing is that sensor after being
> powered up reverts itself to the much the same state it left.
> 
> If we have real regulator then scd30_exit would disable regulator and
> that's it. But, in case of a dummy one and sensor starting in
> continuous mode we waste power for no real reason (for example 19mA
> at 0.5Hz).
> 
> So it's explanation for doing 2 things inside early on but not excuse
> for unintuitive naming.

I'd rather see two devm_add_action_or_reset calls one handling the regulator
and one handling the register write.  Then it will be clear what each
one is doing and that there are no possible races.  Basically it lets
a reviewer not bother thinking which is always good :)

> 
> > > +		return ret;
> > > +
> > > +	ret = scd30_reset(state);
> > > +	if (ret) {
> > > +		dev_err(dev, "failed to reset device: %d\n", ret);
> > > +		return ret;
> > > +	}

Re: [PATCH 1/6] iio: chemical: scd30: add core driver

[Tomasz Duszynski]

On Sat, May 02, 2020 at 05:37:38PM +0100, Jonathan Cameron wrote:
> On Tue, 28 Apr 2020 09:51:01 +0200
> Tomasz Duszynski <tomasz.duszynski@octakon.com> wrote:
>
> > On Sat, Apr 25, 2020 at 07:55:34PM +0100, Jonathan Cameron wrote:
> > > On Wed, 22 Apr 2020 16:11:30 +0200
> > > Tomasz Duszynski <tomasz.duszynski@octakon.com> wrote:
> > >
> > > > Add Sensirion SCD30 carbon dioxide core driver.
> > > >
> > > > Signed-off-by: Tomasz Duszynski <tomasz.duszynski@octakon.com>
> > > Hi Tomasz
> > >
> > > As you've probably guessed the big questions are around the custom ABI.
> > >
> > > Few other things inline.
> > >
> > > Jonathan
> > >
> ...
>
> > > > +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.
> > >
> >
> > Thanks for reminding me :).
> >
> > In that case I admit that some float helper in iio would be a good thing to
> > have. Especially that there will be at least 2 sensors using it.
> >
> > I'd work on that after this driver makes it into the tree.
> >
> > How does it sound?
>
> The problem is that, if we do it in that order we have ABI for this
> device that we should really maintain.  We can probably get away
> with changing it on the basis the channel type is self describing anyway
> but it's not ideal.
>
> So probably fine but not best practice...
>

While I generally agree I can also easily imagine inclusion delay caused
by that change. I need to give some more though to this.

> >
> > > > +
> > > > +	/*
> > > > +	 * 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'?
> > >
> >
> > Yes, grab the fresh one. Moreover enabling interrupts only when necessary can
> > limit pointless buss traffic. Reason being irq is acknowledged by reading data
> > from sensor.
> >
>
> As mentioned below, it seems to me that we should really be starting this
> device only when we want a reading.  Hence any interrupt (subject to possible
> races) should be valid.  Hence we would not be enabling and disabling the
> interrupt controller mask on this line.
>

While it's okay for triggered mode that isn't so ideal for polled mode
because of extra time needed by sensor to actually spin up.

You start measuring and expect new data to arrive within 2 seconds
(given 0.5Hz sampling frequency is set) but they can actually show
up within 8 secs. Not very reliable so to say.

Thus I think sticking to continuous sampling is preferred here.

>
> > > > +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?
> > >
> >
> > IRQs can be actually disabled by telling device to stop taking measurements.
> > There is dedicated command for that. If irq fires off before being disabled
> > nothing bad should happen as everything necessary is in place already.
>
> Hmm. I wonder if we'd be better off starting it on demand - or only when running
> with it as a data ready trigger. That would make the the polled read a case
> of starting the sampling for one sample rather than just 'picking' one from
> the stream of actual samples.
>
> >
> > Another thing is that without disabling interrupt here we would get warning
> > about unbalanced irq whilst enabling trigger.
> >
> > > > +
> > > > +	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.
> > >
> >
> > Okay.
> >
> > > > +		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.
> > >
> >
> > Well, naming might be confusing. The thing is that sensor after being
> > powered up reverts itself to the much the same state it left.
> >
> > If we have real regulator then scd30_exit would disable regulator and
> > that's it. But, in case of a dummy one and sensor starting in
> > continuous mode we waste power for no real reason (for example 19mA
> > at 0.5Hz).
> >
> > So it's explanation for doing 2 things inside early on but not excuse
> > for unintuitive naming.
>
> I'd rather see two devm_add_action_or_reset calls one handling the regulator
> and one handling the register write.  Then it will be clear what each
> one is doing and that there are no possible races.  Basically it lets
> a reviewer not bother thinking which is always good :)
>

Fair enough.

> >
> > > > +		return ret;
> > > > +
> > > > +	ret = scd30_reset(state);
> > > > +	if (ret) {
> > > > +		dev_err(dev, "failed to reset device: %d\n", ret);
> > > > +		return ret;
> > > > +	}

[PATCH 2/6] iio: chemical: scd30: add I2C interface driver

[Tomasz Duszynski]

Add I2C interface driver for the SCD30 sensor.

Signed-off-by: Tomasz Duszynski <tomasz.duszynski@octakon.com>
---
 drivers/iio/chemical/Kconfig     |  11 +++
 drivers/iio/chemical/Makefile    |   1 +
 drivers/iio/chemical/scd30_i2c.c | 141 +++++++++++++++++++++++++++++++
 3 files changed, 153 insertions(+)
 create mode 100644 drivers/iio/chemical/scd30_i2c.c

diff --git a/drivers/iio/chemical/Kconfig b/drivers/iio/chemical/Kconfig
index 55f249333fa2..0c99f37b6bb0 100644
--- a/drivers/iio/chemical/Kconfig
+++ b/drivers/iio/chemical/Kconfig
@@ -85,6 +85,17 @@ config SCD30_CORE
 	  To compile this driver as a module, choose M here: the module will
 	  be called scd30_core.
 
+config SCD30_I2C
+	tristate "SCD30 carbon dioxide sensor I2C driver"
+	depends on SCD30_CORE && I2C
+	select CRC8
+	help
+	  Say Y here to build support for the Sensirion SCD30 I2C interface
+	  driver.
+
+	  To compile this driver as a module, choose M here: the module will
+	  be called scd30_i2c.
+
 config SENSIRION_SGP30
 	tristate "Sensirion SGPxx gas sensors"
 	depends on I2C
diff --git a/drivers/iio/chemical/Makefile b/drivers/iio/chemical/Makefile
index 54abcb641262..f60c8ef358c3 100644
--- a/drivers/iio/chemical/Makefile
+++ b/drivers/iio/chemical/Makefile
@@ -12,6 +12,7 @@ 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_SCD30_I2C) += scd30_i2c.o
 obj-$(CONFIG_SENSIRION_SGP30)	+= sgp30.o
 obj-$(CONFIG_SPS30) += sps30.o
 obj-$(CONFIG_VZ89X)		+= vz89x.o
diff --git a/drivers/iio/chemical/scd30_i2c.c b/drivers/iio/chemical/scd30_i2c.c
new file mode 100644
index 000000000000..c9cf30b05b1a
--- /dev/null
+++ b/drivers/iio/chemical/scd30_i2c.c
@@ -0,0 +1,141 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Sensirion SCD30 carbon dioxide sensor i2c driver
+ *
+ * Copyright (c) Tomasz Duszynski <tomasz.duszynski@octakon.com>
+ *
+ * I2C slave address: 0x61
+ */
+#include <asm/unaligned.h>
+#include <linux/crc8.h>
+#include <linux/device.h>
+#include <linux/errno.h>
+#include <linux/i2c.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/types.h>
+
+#include "scd30.h"
+
+#define SCD30_I2C_MAX_BUF_SIZE 18
+#define SCD30_I2C_CRC8_POLYNOMIAL 0x31
+
+static u16 scd30_i2c_cmd_lookup_tbl[] = {
+	[CMD_START_MEAS] = 0x0010,
+	[CMD_STOP_MEAS] = 0x0104,
+	[CMD_MEAS_INTERVAL] = 0x4600,
+	[CMD_MEAS_READY] = 0x0202,
+	[CMD_READ_MEAS] = 0x0300,
+	[CMD_ASC] = 0x5306,
+	[CMD_FRC] = 0x5204,
+	[CMD_TEMP_OFFSET] = 0x5403,
+	[CMD_FW_VERSION] = 0xd100,
+	[CMD_RESET] = 0xd304,
+};
+
+DECLARE_CRC8_TABLE(scd30_i2c_crc8_tbl);
+
+static int scd30_i2c_xfer(struct scd30_state *state, char *txbuf, int txsize,
+			  char *rxbuf, int rxsize)
+{
+	struct i2c_client *client = to_i2c_client(state->dev);
+	int ret;
+
+	/*
+	 * repeated start is not supported hence instead of sending two i2c
+	 * messages in a row we send one by one
+	 */
+	ret = i2c_master_send(client, txbuf, txsize);
+	if (ret != txsize)
+		return ret < 0 ? ret : -EIO;
+
+	if (!rxbuf)
+		return 0;
+
+	ret = i2c_master_recv(client, rxbuf, rxsize);
+	if (ret != rxsize)
+		return ret < 0 ? ret : -EIO;
+
+	return 0;
+}
+
+static int scd30_i2c_command(struct scd30_state *state, enum scd30_cmd cmd,
+			     u16 arg, char *rsp, int size)
+{
+	char crc, buf[SCD30_I2C_MAX_BUF_SIZE];
+	int i, ret;
+
+	put_unaligned_be16(scd30_i2c_cmd_lookup_tbl[cmd], buf);
+	i = 2;
+
+	if (rsp) {
+		/* each two bytes are followed by a crc8 */
+		size += size / 2;
+	} else {
+		put_unaligned_be16(arg, buf + i);
+		crc = crc8(scd30_i2c_crc8_tbl, buf + i, 2, CRC8_INIT_VALUE);
+		i += 2;
+		buf[i] = crc;
+		i += 1;
+
+		/* commands below don't take an argument */
+		if ((cmd == CMD_STOP_MEAS) || (cmd == CMD_RESET))
+			i -= 3;
+	}
+
+	ret = scd30_i2c_xfer(state, buf, i, buf, size);
+	if (ret)
+		return ret;
+
+	/* validate received data and strip off crc bytes */
+	for (i = 0; i < size; i += 3) {
+		crc = crc8(scd30_i2c_crc8_tbl, buf + i, 2, CRC8_INIT_VALUE);
+		if (crc != buf[i + 2]) {
+			dev_err(state->dev, "data integrity check failed\n");
+			return -EIO;
+		}
+
+		*rsp++ = buf[i];
+		*rsp++ = buf[i + 1];
+	}
+
+	return 0;
+}
+
+static int scd30_i2c_probe(struct i2c_client *client)
+{
+	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
+		return -EOPNOTSUPP;
+
+	crc8_populate_msb(scd30_i2c_crc8_tbl, SCD30_I2C_CRC8_POLYNOMIAL);
+
+	return scd30_probe(&client->dev, client->irq, client->name, NULL,
+			   scd30_i2c_command);
+}
+
+static const struct i2c_device_id scd30_i2c_id[] = {
+	{ "scd30" },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, scd30_i2c_id);
+
+static const struct of_device_id scd30_i2c_of_match[] = {
+	{ .compatible = "sensirion,scd30" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, scd30_i2c_of_match);
+
+static struct i2c_driver scd30_i2c_driver = {
+	.driver = {
+		.name = KBUILD_MODNAME,
+		.of_match_table = scd30_i2c_of_match,
+		.pm = &scd30_pm_ops,
+	},
+	.id_table = scd30_i2c_id,
+	.probe_new = scd30_i2c_probe,
+};
+module_i2c_driver(scd30_i2c_driver);
+
+MODULE_AUTHOR("Tomasz Duszynski <tomasz.duszynski@octakon.com>");
+MODULE_DESCRIPTION("Sensirion SCD30 carbon dioxide sensor i2c driver");
+MODULE_LICENSE("GPL v2");
-- 
2.26.1

[PATCH 3/6] iio: chemical: scd30: add serial interface driver

[Tomasz Duszynski]

Add serial interface driver for the SCD30 sensor.

Signed-off-by: Tomasz Duszynski <tomasz.duszynski@octakon.com>
---
 drivers/iio/chemical/Kconfig        |  11 ++
 drivers/iio/chemical/Makefile       |   1 +
 drivers/iio/chemical/scd30_serial.c | 262 ++++++++++++++++++++++++++++
 3 files changed, 274 insertions(+)
 create mode 100644 drivers/iio/chemical/scd30_serial.c

diff --git a/drivers/iio/chemical/Kconfig b/drivers/iio/chemical/Kconfig
index 0c99f37b6bb0..1b7a58d320e7 100644
--- a/drivers/iio/chemical/Kconfig
+++ b/drivers/iio/chemical/Kconfig
@@ -96,6 +96,17 @@ config SCD30_I2C
 	  To compile this driver as a module, choose M here: the module will
 	  be called scd30_i2c.
 
+config SCD30_SERIAL
+	tristate "SCD30 carbon dioxide sensor serial driver"
+	depends on SCD30_CORE && SERIAL_DEV_BUS
+	select CRC16
+	help
+	  Say Y here to build support for the Sensirion SCD30 serial interface
+	  driver.
+
+	  To compile this driver as a module, choose M here: the module will
+	  be called scd30_serial.
+
 config SENSIRION_SGP30
 	tristate "Sensirion SGPxx gas sensors"
 	depends on I2C
diff --git a/drivers/iio/chemical/Makefile b/drivers/iio/chemical/Makefile
index f60c8ef358c3..1fac1b14f825 100644
--- a/drivers/iio/chemical/Makefile
+++ b/drivers/iio/chemical/Makefile
@@ -13,6 +13,7 @@ obj-$(CONFIG_IAQCORE)		+= ams-iaq-core.o
 obj-$(CONFIG_PMS7003) += pms7003.o
 obj-$(CONFIG_SCD30_CORE) += scd30_core.o
 obj-$(CONFIG_SCD30_I2C) += scd30_i2c.o
+obj-$(CONFIG_SCD30_SERIAL) += scd30_serial.o
 obj-$(CONFIG_SENSIRION_SGP30)	+= sgp30.o
 obj-$(CONFIG_SPS30) += sps30.o
 obj-$(CONFIG_VZ89X)		+= vz89x.o
diff --git a/drivers/iio/chemical/scd30_serial.c b/drivers/iio/chemical/scd30_serial.c
new file mode 100644
index 000000000000..93b98d5f2cc4
--- /dev/null
+++ b/drivers/iio/chemical/scd30_serial.c
@@ -0,0 +1,262 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Sensirion SCD30 carbon dioxide sensor serial driver
+ *
+ * Copyright (c) Tomasz Duszynski <tomasz.duszynski@octakon.com>
+ */
+#include <asm/unaligned.h>
+#include <linux/crc16.h>
+#include <linux/device.h>
+#include <linux/errno.h>
+#include <linux/iio/iio.h>
+#include <linux/jiffies.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/of_irq.h>
+#include <linux/serdev.h>
+#include <linux/string.h>
+#include <linux/types.h>
+
+#include "scd30.h"
+
+#define SCD30_SERDEV_ADDR 0x61
+#define SCD30_SERDEV_WRITE 0x06
+#define SCD30_SERDEV_READ 0x03
+#define SCD30_SERDEV_MAX_BUF_SIZE 17
+#define SCD30_SERDEV_RX_HEADER_SIZE 3
+#define SCD30_SERDEV_CRC_SIZE 2
+#define SCD30_SERDEV_TIMEOUT msecs_to_jiffies(500)
+
+struct scd30_serdev_priv {
+	struct completion meas_ready;
+	char *buf;
+	int num_expected;
+	int num;
+};
+
+static u16 scd30_serdev_cmd_lookup_tbl[] = {
+	[CMD_START_MEAS] = 0x0036,
+	[CMD_STOP_MEAS] = 0x0037,
+	[CMD_MEAS_INTERVAL] = 0x0025,
+	[CMD_MEAS_READY] = 0x0027,
+	[CMD_READ_MEAS] = 0x0028,
+	[CMD_ASC] = 0x003a,
+	[CMD_FRC] = 0x0039,
+	[CMD_TEMP_OFFSET] = 0x003b,
+	[CMD_FW_VERSION] = 0x0020,
+	[CMD_RESET] = 0x0034,
+};
+
+static u16 scd30_serdev_calc_crc(const char *buf, int size)
+{
+	return crc16(0xffff, buf, size);
+}
+
+static int scd30_serdev_xfer(struct scd30_state *state, char *txbuf, int txsize,
+			     char *rxbuf, int rxsize)
+{
+	struct serdev_device *serdev = to_serdev_device(state->dev);
+	struct scd30_serdev_priv *priv = state->priv;
+	int ret;
+
+	priv->buf = rxbuf;
+	priv->num_expected = rxsize;
+	priv->num = 0;
+
+	ret = serdev_device_write(serdev, txbuf, txsize, SCD30_SERDEV_TIMEOUT);
+	if (ret < txsize)
+		return ret < 0 ? ret : -EIO;
+
+	ret = wait_for_completion_interruptible_timeout(&priv->meas_ready,
+							SCD30_SERDEV_TIMEOUT);
+	if (ret > 0)
+		ret = 0;
+	else if (!ret)
+		ret = -ETIMEDOUT;
+
+	return ret;
+}
+
+static int scd30_serdev_command(struct scd30_state *state, enum scd30_cmd cmd,
+				u16 arg, char *rsp, int size)
+{
+	/*
+	 * Communication over serial line is based on modbus protocol (or rather
+	 * its variation called modbus over serial to be precise). Upon
+	 * receiving a request device should reply with response.
+	 *
+	 * Frame below represents a request message. Each field takes
+	 * exactly one byte.
+	 *
+	 * +------+------+-----+-----+-------+-------+-----+-----+
+	 * | dev  | op   | reg | reg | byte1 | byte0 | crc | crc |
+	 * | addr | code | msb | lsb |       |       | lsb | msb |
+	 * +------+------+-----+-----+-------+-------+-----+-----+
+	 *
+	 * The message device replies with depends on the 'op code' field from
+	 * the request. In case it was set to SCD30_SERDEV_WRITE sensor should
+	 * reply with unchanged request. Otherwise 'op code' was set to
+	 * SCD30_SERDEV_READ and response looks like the one below. As with
+	 * request, each field takes one byte.
+	 *
+	 * +------+------+--------+-------+-----+-------+-----+-----+
+	 * | dev  | op   | num of | byte0 | ... | byteN | crc | crc |
+	 * | addr | code | bytes  |       |     |       | lsb | msb |
+	 * +------+------+--------+-------+-----+-------+-----+-----+
+	 */
+	char rxbuf[SCD30_SERDEV_MAX_BUF_SIZE];
+	char txbuf[SCD30_SERDEV_MAX_BUF_SIZE] = { SCD30_SERDEV_ADDR };
+	int ret, rxsize, txsize = 2;
+	u16 crc;
+
+	put_unaligned_be16(scd30_serdev_cmd_lookup_tbl[cmd], txbuf + txsize);
+	txsize += 2;
+
+	if (rsp) {
+		txbuf[1] = SCD30_SERDEV_READ;
+		if (cmd == CMD_READ_MEAS)
+			/* number of u16 words to read */
+			put_unaligned_be16(size / 2, txbuf + txsize);
+		else
+			put_unaligned_be16(0x0001, txbuf + txsize);
+		txsize += 2;
+		crc = scd30_serdev_calc_crc(txbuf, txsize);
+		put_unaligned_le16(crc, txbuf + txsize);
+		txsize += 2;
+		rxsize = SCD30_SERDEV_RX_HEADER_SIZE + size +
+			 SCD30_SERDEV_CRC_SIZE;
+	} else {
+		if ((cmd == CMD_STOP_MEAS) || (cmd == CMD_RESET))
+			arg = 0x0001;
+
+		txbuf[1] = SCD30_SERDEV_WRITE;
+		put_unaligned_be16(arg, txbuf + txsize);
+		txsize += 2;
+		crc = scd30_serdev_calc_crc(txbuf, txsize);
+		put_unaligned_le16(crc, txbuf + txsize);
+		txsize += 2;
+		rxsize = txsize;
+	}
+
+	ret = scd30_serdev_xfer(state, txbuf, txsize, rxbuf, rxsize);
+	if (ret)
+		return ret;
+
+	switch (txbuf[1]) {
+	case SCD30_SERDEV_WRITE:
+		if (memcmp(txbuf, txbuf, txsize)) {
+			dev_err(state->dev, "wrong message received\n");
+			return -EIO;
+		}
+		break;
+	case SCD30_SERDEV_READ:
+		if (rxbuf[2] != (rxsize -
+				 SCD30_SERDEV_RX_HEADER_SIZE -
+				 SCD30_SERDEV_CRC_SIZE)) {
+			dev_err(state->dev,
+				"received data size does not match header\n");
+			return -EIO;
+		}
+
+		rxsize -= SCD30_SERDEV_CRC_SIZE;
+		crc = get_unaligned_le16(rxbuf + rxsize);
+		if (crc != scd30_serdev_calc_crc(rxbuf, rxsize)) {
+			dev_err(state->dev, "data integrity check failed\n");
+			return -EIO;
+		}
+
+		rxsize -= SCD30_SERDEV_RX_HEADER_SIZE;
+		memcpy(rsp, rxbuf + SCD30_SERDEV_RX_HEADER_SIZE, rxsize);
+		break;
+	default:
+		dev_err(state->dev, "received unknown op code\n");
+		return -EIO;
+	}
+
+	return 0;
+}
+
+static int scd30_serdev_receive_buf(struct serdev_device *serdev,
+				    const unsigned char *buf, size_t size)
+{
+	struct iio_dev *indio_dev = dev_get_drvdata(&serdev->dev);
+	struct scd30_state *state = iio_priv(indio_dev);
+	struct scd30_serdev_priv *priv = state->priv;
+	int num;
+
+	/* just in case sensor puts some unexpected bytes on the bus */
+	if (!priv->buf)
+		return 0;
+
+	if (priv->num + size >= priv->num_expected)
+		num = priv->num_expected - priv->num;
+	else
+		num = size;
+
+	memcpy(priv->buf + priv->num, buf, num);
+	priv->num += num;
+
+	if (priv->num == priv->num_expected) {
+		priv->buf = NULL;
+		complete(&priv->meas_ready);
+	}
+
+	return num;
+}
+
+static const struct serdev_device_ops scd30_serdev_ops = {
+	.receive_buf = scd30_serdev_receive_buf,
+	.write_wakeup = serdev_device_write_wakeup,
+};
+
+static int scd30_serdev_probe(struct serdev_device *serdev)
+{
+	struct device *dev = &serdev->dev;
+	struct scd30_serdev_priv *priv;
+	int irq, ret;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	init_completion(&priv->meas_ready);
+	serdev_device_set_client_ops(serdev, &scd30_serdev_ops);
+
+	ret = devm_serdev_device_open(dev, serdev);
+	if (ret)
+		return ret;
+
+	serdev_device_set_baudrate(serdev, 19200);
+	serdev_device_set_flow_control(serdev, false);
+
+	ret = serdev_device_set_parity(serdev, SERDEV_PARITY_NONE);
+	if (ret)
+		return ret;
+
+	irq = of_irq_get(dev->of_node, 0);
+	if (irq <= 0)
+		irq = 0;
+
+	return scd30_probe(dev, irq, KBUILD_MODNAME, priv,
+			   scd30_serdev_command);
+}
+
+static const struct of_device_id scd30_serdev_of_match[] = {
+	{ .compatible = "sensirion,scd30" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, scd30_serdev_of_match);
+
+static struct serdev_device_driver scd30_serdev_driver = {
+	.driver = {
+		.name = KBUILD_MODNAME,
+		.of_match_table = scd30_serdev_of_match,
+		.pm = &scd30_pm_ops,
+	},
+	.probe = scd30_serdev_probe,
+};
+module_serdev_device_driver(scd30_serdev_driver);
+
+MODULE_AUTHOR("Tomasz Duszynski <tomasz.duszynski@octakon.com>");
+MODULE_DESCRIPTION("Sensirion SCD30 carbon dioxide sensor serial driver");
+MODULE_LICENSE("GPL v2");
-- 
2.26.1

Re: [PATCH 3/6] iio: chemical: scd30: add serial interface driver

[Andy Shevchenko]

On Wed, Apr 22, 2020 at 5:22 PM Tomasz Duszynski
<tomasz.duszynski@octakon.com> wrote:
>
> Add serial interface driver for the SCD30 sensor.

...

> +#include <linux/of_irq.h>

Do you need this?

> +static int scd30_serdev_probe(struct serdev_device *serdev)
> +{
> +       struct device *dev = &serdev->dev;
> +       struct scd30_serdev_priv *priv;
> +       int irq, ret;
> +
> +       priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +       if (!priv)
> +               return -ENOMEM;

> +       irq = of_irq_get(dev->of_node, 0);

fwnode_irq_get() ?

> +       if (irq <= 0)
> +               irq = 0;
> +
> +       return scd30_probe(dev, irq, KBUILD_MODNAME, priv,
> +                          scd30_serdev_command);
> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/6] iio: chemical: scd30: add serial interface driver

[Tomasz Duszynski]

On Wed, Apr 22, 2020 at 10:55:05PM +0300, Andy Shevchenko wrote:
> On Wed, Apr 22, 2020 at 5:22 PM Tomasz Duszynski
> <tomasz.duszynski@octakon.com> wrote:
> >
> > Add serial interface driver for the SCD30 sensor.
>
> ...
>
> > +#include <linux/of_irq.h>
>
> Do you need this?
>
> > +static int scd30_serdev_probe(struct serdev_device *serdev)
> > +{
> > +       struct device *dev = &serdev->dev;
> > +       struct scd30_serdev_priv *priv;
> > +       int irq, ret;
> > +
> > +       priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > +       if (!priv)
> > +               return -ENOMEM;
>
> > +       irq = of_irq_get(dev->of_node, 0);
>
> fwnode_irq_get() ?

Okay, that should work equally good.

>
> > +       if (irq <= 0)
> > +               irq = 0;
> > +
> > +       return scd30_probe(dev, irq, KBUILD_MODNAME, priv,
> > +                          scd30_serdev_command);
> > +}
>
> --
> With Best Regards,
> Andy Shevchenko

[PATCH 4/6] Documentation: ABI: testing: scd30: document iio attributes

[Tomasz Duszynski]

Add documentation for sensor specific iio attributes.

Signed-off-by: Tomasz Duszynski <tomasz.duszynski@octakon.com>
---
 Documentation/ABI/testing/sysfs-bus-iio-scd30 | 97 +++++++++++++++++++
 1 file changed, 97 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-scd30

diff --git a/Documentation/ABI/testing/sysfs-bus-iio-scd30 b/Documentation/ABI/testing/sysfs-bus-iio-scd30
new file mode 100644
index 000000000000..0431a718447d
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-bus-iio-scd30
@@ -0,0 +1,97 @@
+What:		/sys/bus/iio/devices/iio:deviceX/pressure_comp
+Date:		April 2020
+KernelVersion:	5.8
+Contact:	linux-iio@vger.kernel.org
+Description:
+		Given that sensor's CO2 measurement chamber has fixed volume
+		pressure changes will affect concentration readings. Writing
+		current ambient pressure here will allow senor to make necessary
+		adjustments. Upon reading previously set value is returned.
+		Units are millibars.
+
+What:		/sys/bus/iio/devices/iio:deviceX/pressure_comp_available
+Date:		April 2020
+KernelVersion:	5.8
+Contact:	linux-iio@vger.kernel.org
+Description:
+		The range of available values in millibars represented as the
+		minimum value, the step and the maximum value, all enclosed in
+		square brackets.
+
+What:		/sys/bus/iio/devices/iio:deviceX/meas_interval
+Date:		January 2020
+KernelVersion:	5.8
+Contact:	linux-iio@vger.kernel.org
+Description:
+		Amount of time between subsequent measurements. Writing this
+		attribute will change measurement interval. Upon reading
+		current measurement interval is returned. Units are seconds.
+
+What:		/sys/bus/iio/devices/iio:deviceX/meas_interval_available
+Date:		April 2020
+KernelVersion:	5.8
+Contact:	linux-iio@vger.kernel.org
+Description:
+		The range of available values in seconds represented as the
+		minimum value, the step and the maximum value, all enclosed in
+		square brackets.
+
+What:		/sys/bus/iio/devices/iio:deviceX/asc
+Date:		April 2020
+KernelVersion:	5.8
+Contact:	linux-iio@vger.kernel.org
+Description:
+		Writing 1 or 0 to this attribute will respectively activate or
+		deactivate automatic self calibration procedure. Upon reading 1
+		is returned if asc is ongoing, 0 otherwise.
+
+What:		/sys/bus/iio/devices/iio:deviceX/frc
+Date:		April 2020
+KernelVersion:	5.8
+Contact:	linux-iio@vger.kernel.org
+Description:
+		Forced recalibration is used to compensate for sensor drifts
+		when a reference value of CO2 concentration in close proximity
+		to the sensor is available. Writing attribute will set frc
+		value. Upon reading current frc is returned. Units are
+		millibars.
+
+What:		/sys/bus/iio/devices/iio:deviceX/frc_available
+Date:		April 2020
+KernelVersion:	5.8
+Contact:	linux-iio@vger.kernel.org
+Description:
+		The range of available values in millibars represented as the
+		minimum value, the step and the maximum value, all enclosed in
+		square brackets.
+
+What:		/sys/bus/iio/devices/iio:deviceX/temp_offset
+Date:		April 2020
+KernelVersion:	5.8
+Contact:	linux-iio@vger.kernel.org
+Description:
+		Sensor readings may be affected by ambient temperature.
+		Writing temperature offset will compensate for unwanted changes.
+		Note that written offset gets multiplied by a factor of 100
+		by a sensor internally.
+
+		For example, writing 10 here will correspond to 0.1 degree
+		Celsius.
+
+What:		/sys/bus/iio/devices/iio:deviceX/temp_offset_available
+Date:		April 2020
+KernelVersion:	5.8
+Contact:	linux-iio@vger.kernel.org
+Description:
+		The range of available values in degrees Celsius represented as
+		the minimum value, the step and the maximum value, all enclosed
+		in square brackets.
+
+What:		/sys/bus/iio/devices/iio:deviceX/reset
+Date:		April 2020
+KernelVersion:	5.8
+Contact:	linux-iio@vger.kernel.org
+Description:
+		Software reset mechanism forces sensor into the same state
+		as after powering up without the need for removing power supply.
+		Writing any value will reset sensor.
-- 
2.26.1

Re: [PATCH 4/6] Documentation: ABI: testing: scd30: document iio attributes

[Peter Meerwald-Stadler]

On Wed, 22 Apr 2020, Tomasz Duszynski wrote:

> Add documentation for sensor specific iio attributes.

minor comments below
 
> Signed-off-by: Tomasz Duszynski <tomasz.duszynski@octakon.com>
> ---
>  Documentation/ABI/testing/sysfs-bus-iio-scd30 | 97 +++++++++++++++++++
>  1 file changed, 97 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-scd30
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-iio-scd30 b/Documentation/ABI/testing/sysfs-bus-iio-scd30
> new file mode 100644
> index 000000000000..0431a718447d
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-bus-iio-scd30
> @@ -0,0 +1,97 @@
> +What:		/sys/bus/iio/devices/iio:deviceX/pressure_comp
> +Date:		April 2020
> +KernelVersion:	5.8
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Given that sensor's CO2 measurement chamber has fixed volume
> +		pressure changes will affect concentration readings. Writing
> +		current ambient pressure here will allow senor to make necessary

sensor

> +		adjustments. Upon reading previously set value is returned.
> +		Units are millibars.

unit for pressure in IIO is kilopascal (e.g. 
/sys/bus/iio/devices/iio:deviceX/in_pressure_raw)

> +
> +What:		/sys/bus/iio/devices/iio:deviceX/pressure_comp_available
> +Date:		April 2020
> +KernelVersion:	5.8
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		The range of available values in millibars represented as the
> +		minimum value, the step and the maximum value, all enclosed in
> +		square brackets.
> +
> +What:		/sys/bus/iio/devices/iio:deviceX/meas_interval
> +Date:		January 2020
> +KernelVersion:	5.8
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Amount of time between subsequent measurements. Writing this
> +		attribute will change measurement interval. Upon reading
> +		current measurement interval is returned. Units are seconds.
> +
> +What:		/sys/bus/iio/devices/iio:deviceX/meas_interval_available
> +Date:		April 2020
> +KernelVersion:	5.8
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		The range of available values in seconds represented as the
> +		minimum value, the step and the maximum value, all enclosed in
> +		square brackets.
> +
> +What:		/sys/bus/iio/devices/iio:deviceX/asc
> +Date:		April 2020
> +KernelVersion:	5.8
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Writing 1 or 0 to this attribute will respectively activate or
> +		deactivate automatic self calibration procedure. Upon reading 1

deactivate automatic self calibration (asc) procedure

> +		is returned if asc is ongoing, 0 otherwise.
> +
> +What:		/sys/bus/iio/devices/iio:deviceX/frc
> +Date:		April 2020
> +KernelVersion:	5.8
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Forced recalibration is used to compensate for sensor drifts
> +		when a reference value of CO2 concentration in close proximity
> +		to the sensor is available. Writing attribute will set frc
> +		value. Upon reading current frc is returned. Units are
> +		millibars.
> +
> +What:		/sys/bus/iio/devices/iio:deviceX/frc_available
> +Date:		April 2020
> +KernelVersion:	5.8
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		The range of available values in millibars represented as the
> +		minimum value, the step and the maximum value, all enclosed in
> +		square brackets.
> +
> +What:		/sys/bus/iio/devices/iio:deviceX/temp_offset
> +Date:		April 2020
> +KernelVersion:	5.8
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Sensor readings may be affected by ambient temperature.
> +		Writing temperature offset will compensate for unwanted changes.
> +		Note that written offset gets multiplied by a factor of 100
> +		by a sensor internally.
> +
> +		For example, writing 10 here will correspond to 0.1 degree
> +		Celsius.
> +
> +What:		/sys/bus/iio/devices/iio:deviceX/temp_offset_available
> +Date:		April 2020
> +KernelVersion:	5.8
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		The range of available values in degrees Celsius represented as
> +		the minimum value, the step and the maximum value, all enclosed
> +		in square brackets.
> +
> +What:		/sys/bus/iio/devices/iio:deviceX/reset
> +Date:		April 2020
> +KernelVersion:	5.8
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Software reset mechanism forces sensor into the same state
> +		as after powering up without the need for removing power supply.
> +		Writing any value will reset sensor.
> 

-- 

Peter Meerwald-Stadler
Mobile: +43 664 24 44 418

Re: [PATCH 4/6] Documentation: ABI: testing: scd30: document iio attributes

[Tomasz Duszynski]

On Wed, Apr 22, 2020 at 06:40:17PM +0200, Peter Meerwald-Stadler wrote:
> On Wed, 22 Apr 2020, Tomasz Duszynski wrote:
>
> > Add documentation for sensor specific iio attributes.
>
> minor comments below

Thanks.

>
> > Signed-off-by: Tomasz Duszynski <tomasz.duszynski@octakon.com>
> > ---
> >  Documentation/ABI/testing/sysfs-bus-iio-scd30 | 97 +++++++++++++++++++
> >  1 file changed, 97 insertions(+)
> >  create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-scd30
> >
> > diff --git a/Documentation/ABI/testing/sysfs-bus-iio-scd30 b/Documentation/ABI/testing/sysfs-bus-iio-scd30
> > new file mode 100644
> > index 000000000000..0431a718447d
> > --- /dev/null
> > +++ b/Documentation/ABI/testing/sysfs-bus-iio-scd30
> > @@ -0,0 +1,97 @@
> > +What:		/sys/bus/iio/devices/iio:deviceX/pressure_comp
> > +Date:		April 2020
> > +KernelVersion:	5.8
> > +Contact:	linux-iio@vger.kernel.org
> > +Description:
> > +		Given that sensor's CO2 measurement chamber has fixed volume
> > +		pressure changes will affect concentration readings. Writing
> > +		current ambient pressure here will allow senor to make necessary
>
> sensor
>

Okay.

> > +		adjustments. Upon reading previously set value is returned.
> > +		Units are millibars.
>
> unit for pressure in IIO is kilopascal (e.g.
> /sys/bus/iio/devices/iio:deviceX/in_pressure_raw)
>

My thinking here was that since these are sensor specific attributes
they don't need to stick to iio conventions and millibars were somewhat
more natural to use. But I guess that's just matter of habit.

So generally I am okay with reworking all attrs to accept values in iio
preferred units.

> > +
> > +What:		/sys/bus/iio/devices/iio:deviceX/pressure_comp_available
> > +Date:		April 2020
> > +KernelVersion:	5.8
> > +Contact:	linux-iio@vger.kernel.org
> > +Description:
> > +		The range of available values in millibars represented as the
> > +		minimum value, the step and the maximum value, all enclosed in
> > +		square brackets.
> > +
> > +What:		/sys/bus/iio/devices/iio:deviceX/meas_interval
> > +Date:		January 2020
> > +KernelVersion:	5.8
> > +Contact:	linux-iio@vger.kernel.org
> > +Description:
> > +		Amount of time between subsequent measurements. Writing this
> > +		attribute will change measurement interval. Upon reading
> > +		current measurement interval is returned. Units are seconds.
> > +
> > +What:		/sys/bus/iio/devices/iio:deviceX/meas_interval_available
> > +Date:		April 2020
> > +KernelVersion:	5.8
> > +Contact:	linux-iio@vger.kernel.org
> > +Description:
> > +		The range of available values in seconds represented as the
> > +		minimum value, the step and the maximum value, all enclosed in
> > +		square brackets.
> > +
> > +What:		/sys/bus/iio/devices/iio:deviceX/asc
> > +Date:		April 2020
> > +KernelVersion:	5.8
> > +Contact:	linux-iio@vger.kernel.org
> > +Description:
> > +		Writing 1 or 0 to this attribute will respectively activate or
> > +		deactivate automatic self calibration procedure. Upon reading 1
>
> deactivate automatic self calibration (asc) procedure
>

That shouldn't be too difficult to realize what asc actually stands for after
reading this short description.

> > +		is returned if asc is ongoing, 0 otherwise.
> > +
> > +What:		/sys/bus/iio/devices/iio:deviceX/frc
> > +Date:		April 2020
> > +KernelVersion:	5.8
> > +Contact:	linux-iio@vger.kernel.org
> > +Description:
> > +		Forced recalibration is used to compensate for sensor drifts
> > +		when a reference value of CO2 concentration in close proximity
> > +		to the sensor is available. Writing attribute will set frc
> > +		value. Upon reading current frc is returned. Units are
> > +		millibars.
> > +
> > +What:		/sys/bus/iio/devices/iio:deviceX/frc_available
> > +Date:		April 2020
> > +KernelVersion:	5.8
> > +Contact:	linux-iio@vger.kernel.org
> > +Description:
> > +		The range of available values in millibars represented as the
> > +		minimum value, the step and the maximum value, all enclosed in
> > +		square brackets.
> > +
> > +What:		/sys/bus/iio/devices/iio:deviceX/temp_offset
> > +Date:		April 2020
> > +KernelVersion:	5.8
> > +Contact:	linux-iio@vger.kernel.org
> > +Description:
> > +		Sensor readings may be affected by ambient temperature.
> > +		Writing temperature offset will compensate for unwanted changes.
> > +		Note that written offset gets multiplied by a factor of 100
> > +		by a sensor internally.
> > +
> > +		For example, writing 10 here will correspond to 0.1 degree
> > +		Celsius.
> > +
> > +What:		/sys/bus/iio/devices/iio:deviceX/temp_offset_available
> > +Date:		April 2020
> > +KernelVersion:	5.8
> > +Contact:	linux-iio@vger.kernel.org
> > +Description:
> > +		The range of available values in degrees Celsius represented as
> > +		the minimum value, the step and the maximum value, all enclosed
> > +		in square brackets.
> > +
> > +What:		/sys/bus/iio/devices/iio:deviceX/reset
> > +Date:		April 2020
> > +KernelVersion:	5.8
> > +Contact:	linux-iio@vger.kernel.org
> > +Description:
> > +		Software reset mechanism forces sensor into the same state
> > +		as after powering up without the need for removing power supply.
> > +		Writing any value will reset sensor.
> >
>
> --
>
> Peter Meerwald-Stadler
> Mobile: +43 664 24 44 418

Re: [PATCH 4/6] Documentation: ABI: testing: scd30: document iio attributes

[Jonathan Cameron]

On Thu, 23 Apr 2020 17:53:17 +0200
Tomasz Duszynski <tomasz.duszynski@octakon.com> wrote:

> On Wed, Apr 22, 2020 at 06:40:17PM +0200, Peter Meerwald-Stadler wrote:
> > On Wed, 22 Apr 2020, Tomasz Duszynski wrote:
> >  
> > > Add documentation for sensor specific iio attributes.  
> >
> > minor comments below  
> 
> Thanks.
> 
> >  
> > > Signed-off-by: Tomasz Duszynski <tomasz.duszynski@octakon.com>
> > > ---
> > >  Documentation/ABI/testing/sysfs-bus-iio-scd30 | 97 +++++++++++++++++++
> > >  1 file changed, 97 insertions(+)
> > >  create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-scd30
> > >
> > > diff --git a/Documentation/ABI/testing/sysfs-bus-iio-scd30 b/Documentation/ABI/testing/sysfs-bus-iio-scd30
> > > new file mode 100644
> > > index 000000000000..0431a718447d
> > > --- /dev/null
> > > +++ b/Documentation/ABI/testing/sysfs-bus-iio-scd30
> > > @@ -0,0 +1,97 @@
> > > +What:		/sys/bus/iio/devices/iio:deviceX/pressure_comp
> > > +Date:		April 2020
> > > +KernelVersion:	5.8
> > > +Contact:	linux-iio@vger.kernel.org
> > > +Description:
> > > +		Given that sensor's CO2 measurement chamber has fixed volume
> > > +		pressure changes will affect concentration readings. Writing
> > > +		current ambient pressure here will allow senor to make necessary  
> >
> > sensor
> >  
> 
> Okay.
> 
> > > +		adjustments. Upon reading previously set value is returned.
> > > +		Units are millibars.  
> >
> > unit for pressure in IIO is kilopascal (e.g.
> > /sys/bus/iio/devices/iio:deviceX/in_pressure_raw)
> >  
> 
> My thinking here was that since these are sensor specific attributes
> they don't need to stick to iio conventions and millibars were somewhat
> more natural to use. But I guess that's just matter of habit.

You absolutely have to stick to standard units.  Userspace programs
aren't going to come read your docs...

For other sensors that take a calibration value like this we've reported
them via an output channel.  For example the atlas-ph sensor has
an 'output temp' channel used for this purpose.

It's not ideal or totally intuitive but it does let us avoid expanding
the overall ABI.  The argument was something along the lines of
1) Imagine your sensor could control the pressure in the measurement space...
2) An output channel would provide the value to set it to.
3) Now instead we provide a means of saying 'what it is'
4) End result is we write a value and the pressure in the chamber is
   that value :)

As I said not ideal but the best we can do without having to define a lot
of ABI just to deal with compensation factors.

This is a rare case where I would document the 'standard' ABI in here
to make the point that it is actually providing an estimate of the pressure
not controlling it...

> 
> So generally I am okay with reworking all attrs to accept values in iio
> preferred units.
> 
> > > +
> > > +What:		/sys/bus/iio/devices/iio:deviceX/pressure_comp_available
> > > +Date:		April 2020
> > > +KernelVersion:	5.8
> > > +Contact:	linux-iio@vger.kernel.org
> > > +Description:
> > > +		The range of available values in millibars represented as the
> > > +		minimum value, the step and the maximum value, all enclosed in
> > > +		square brackets.
> > > +
> > > +What:		/sys/bus/iio/devices/iio:deviceX/meas_interval
> > > +Date:		January 2020
> > > +KernelVersion:	5.8
> > > +Contact:	linux-iio@vger.kernel.org
> > > +Description:
> > > +		Amount of time between subsequent measurements. Writing this
> > > +		attribute will change measurement interval. Upon reading
> > > +		current measurement interval is returned. Units are seconds.

Use the existing ABI sampling frequency which is sort of the inverse of this.

> > > +
> > > +What:		/sys/bus/iio/devices/iio:deviceX/meas_interval_available
> > > +Date:		April 2020
> > > +KernelVersion:	5.8
> > > +Contact:	linux-iio@vger.kernel.org
> > > +Description:
> > > +		The range of available values in seconds represented as the
> > > +		minimum value, the step and the maximum value, all enclosed in
> > > +		square brackets.
> > > +
> > > +What:		/sys/bus/iio/devices/iio:deviceX/asc
Spends some characters to easy of understanding ;)

auto_calib_proc_enable maybe?  Or can we get away with the 'somewhat standard
calibration (it's used in at least one other driver IIRC)

> > > +Date:		April 2020
> > > +KernelVersion:	5.8
> > > +Contact:	linux-iio@vger.kernel.org
> > > +Description:
> > > +		Writing 1 or 0 to this attribute will respectively activate or
> > > +		deactivate automatic self calibration procedure. Upon reading 1  
> >
> > deactivate automatic self calibration (asc) procedure
> >  
> 
> That shouldn't be too difficult to realize what asc actually stands for after
> reading this short description.
> 
> > > +		is returned if asc is ongoing, 0 otherwise.
> > > +
> > > +What:		/sys/bus/iio/devices/iio:deviceX/frc
> > > +Date:		April 2020
> > > +KernelVersion:	5.8
> > > +Contact:	linux-iio@vger.kernel.org
> > > +Description:
> > > +		Forced recalibration is used to compensate for sensor drifts
> > > +		when a reference value of CO2 concentration in close proximity
> > > +		to the sensor is available. Writing attribute will set frc
> > > +		value. Upon reading current frc is returned. Units are
> > > +		millibars.

Could we implement this by just writing to the main channel value?
Bit of a clunky ABI but sort of logically fits in my head given we are basically
forcing the value we read to be this one?

> > > +
> > > +What:		/sys/bus/iio/devices/iio:deviceX/frc_available
> > > +Date:		April 2020
> > > +KernelVersion:	5.8
> > > +Contact:	linux-iio@vger.kernel.org
> > > +Description:
> > > +		The range of available values in millibars represented as the
> > > +		minimum value, the step and the maximum value, all enclosed in
> > > +		square brackets.
> > > +
> > > +What:		/sys/bus/iio/devices/iio:deviceX/temp_offset
> > > +Date:		April 2020
> > > +KernelVersion:	5.8
> > > +Contact:	linux-iio@vger.kernel.org
> > > +Description:
> > > +		Sensor readings may be affected by ambient temperature.
> > > +		Writing temperature offset will compensate for unwanted changes.
> > > +		Note that written offset gets multiplied by a factor of 100
> > > +		by a sensor internally.
> > > +
> > > +		For example, writing 10 here will correspond to 0.1 degree
> > > +		Celsius.

This sounds like a calibbias to me which is standard ABI.

> > > +
> > > +What:		/sys/bus/iio/devices/iio:deviceX/temp_offset_available
> > > +Date:		April 2020
> > > +KernelVersion:	5.8
> > > +Contact:	linux-iio@vger.kernel.org
> > > +Description:
> > > +		The range of available values in degrees Celsius represented as
> > > +		the minimum value, the step and the maximum value, all enclosed
> > > +		in square brackets.

Wrong units for temperature (which is an odd one as we
lifted them from hwmon before learning the error of our ways and starting to use
SI units as the base).


> > > +
> > > +What:		/sys/bus/iio/devices/iio:deviceX/reset
> > > +Date:		April 2020
> > > +KernelVersion:	5.8
> > > +Contact:	linux-iio@vger.kernel.org
> > > +Description:
> > > +		Software reset mechanism forces sensor into the same state
> > > +		as after powering up without the need for removing power supply.
> > > +		Writing any value will reset sensor.

Not seeing an argument here for why you might want to do that other than on
power up or module probe to get the driver into a known state.
So currently it's a no to this one - just don't expose it to userspace.


> > >  
> >
> > --
> >
> > Peter Meerwald-Stadler
> > Mobile: +43 664 24 44 418  

Re: [PATCH 4/6] Documentation: ABI: testing: scd30: document iio attributes

[Tomasz Duszynski]

On Sat, Apr 25, 2020 at 08:20:57PM +0100, Jonathan Cameron wrote:
> On Thu, 23 Apr 2020 17:53:17 +0200
> Tomasz Duszynski <tomasz.duszynski@octakon.com> wrote:
>
> > On Wed, Apr 22, 2020 at 06:40:17PM +0200, Peter Meerwald-Stadler wrote:
> > > On Wed, 22 Apr 2020, Tomasz Duszynski wrote:
> > >
> > > > Add documentation for sensor specific iio attributes.
> > >
> > > minor comments below
> >
> > Thanks.
> >
> > >
> > > > Signed-off-by: Tomasz Duszynski <tomasz.duszynski@octakon.com>
> > > > ---
> > > >  Documentation/ABI/testing/sysfs-bus-iio-scd30 | 97 +++++++++++++++++++
> > > >  1 file changed, 97 insertions(+)
> > > >  create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-scd30
> > > >
> > > > diff --git a/Documentation/ABI/testing/sysfs-bus-iio-scd30 b/Documentation/ABI/testing/sysfs-bus-iio-scd30
> > > > new file mode 100644
> > > > index 000000000000..0431a718447d
> > > > --- /dev/null
> > > > +++ b/Documentation/ABI/testing/sysfs-bus-iio-scd30
> > > > @@ -0,0 +1,97 @@
> > > > +What:		/sys/bus/iio/devices/iio:deviceX/pressure_comp
> > > > +Date:		April 2020
> > > > +KernelVersion:	5.8
> > > > +Contact:	linux-iio@vger.kernel.org
> > > > +Description:
> > > > +		Given that sensor's CO2 measurement chamber has fixed volume
> > > > +		pressure changes will affect concentration readings. Writing
> > > > +		current ambient pressure here will allow senor to make necessary
> > >
> > > sensor
> > >
> >
> > Okay.
> >
> > > > +		adjustments. Upon reading previously set value is returned.
> > > > +		Units are millibars.
> > >
> > > unit for pressure in IIO is kilopascal (e.g.
> > > /sys/bus/iio/devices/iio:deviceX/in_pressure_raw)
> > >
> >
> > My thinking here was that since these are sensor specific attributes
> > they don't need to stick to iio conventions and millibars were somewhat
> > more natural to use. But I guess that's just matter of habit.
>
> You absolutely have to stick to standard units.  Userspace programs
> aren't going to come read your docs...
>
> For other sensors that take a calibration value like this we've reported
> them via an output channel.  For example the atlas-ph sensor has
> an 'output temp' channel used for this purpose.
>

Fair enough.

> It's not ideal or totally intuitive but it does let us avoid expanding
> the overall ABI.  The argument was something along the lines of
> 1) Imagine your sensor could control the pressure in the measurement space...
> 2) An output channel would provide the value to set it to.
> 3) Now instead we provide a means of saying 'what it is'
> 4) End result is we write a value and the pressure in the chamber is
>    that value :)
>
> As I said not ideal but the best we can do without having to define a lot
> of ABI just to deal with compensation factors.
>
> This is a rare case where I would document the 'standard' ABI in here
> to make the point that it is actually providing an estimate of the pressure
> not controlling it...
>
> >
> > So generally I am okay with reworking all attrs to accept values in iio
> > preferred units.
> >
> > > > +
> > > > +What:		/sys/bus/iio/devices/iio:deviceX/pressure_comp_available
> > > > +Date:		April 2020
> > > > +KernelVersion:	5.8
> > > > +Contact:	linux-iio@vger.kernel.org
> > > > +Description:
> > > > +		The range of available values in millibars represented as the
> > > > +		minimum value, the step and the maximum value, all enclosed in
> > > > +		square brackets.
> > > > +
> > > > +What:		/sys/bus/iio/devices/iio:deviceX/meas_interval
> > > > +Date:		January 2020
> > > > +KernelVersion:	5.8
> > > > +Contact:	linux-iio@vger.kernel.org
> > > > +Description:
> > > > +		Amount of time between subsequent measurements. Writing this
> > > > +		attribute will change measurement interval. Upon reading
> > > > +		current measurement interval is returned. Units are seconds.
>
> Use the existing ABI sampling frequency which is sort of the inverse of this.
>

Was thinking about it but long periods in Hz simply don't look appealing :).

No other strong opinions so I'll rework that.

> > > > +
> > > > +What:		/sys/bus/iio/devices/iio:deviceX/meas_interval_available
> > > > +Date:		April 2020
> > > > +KernelVersion:	5.8
> > > > +Contact:	linux-iio@vger.kernel.org
> > > > +Description:
> > > > +		The range of available values in seconds represented as the
> > > > +		minimum value, the step and the maximum value, all enclosed in
> > > > +		square brackets.
> > > > +
> > > > +What:		/sys/bus/iio/devices/iio:deviceX/asc
> Spends some characters to easy of understanding ;)
>
> auto_calib_proc_enable maybe?  Or can we get away with the 'somewhat standard
> calibration (it's used in at least one other driver IIRC)
>

Just self_calibration would do?

> > > > +Date:		April 2020
> > > > +KernelVersion:	5.8
> > > > +Contact:	linux-iio@vger.kernel.org
> > > > +Description:
> > > > +		Writing 1 or 0 to this attribute will respectively activate or
> > > > +		deactivate automatic self calibration procedure. Upon reading 1
> > >
> > > deactivate automatic self calibration (asc) procedure
> > >
> >
> > That shouldn't be too difficult to realize what asc actually stands for after
> > reading this short description.
> >
> > > > +		is returned if asc is ongoing, 0 otherwise.
> > > > +
> > > > +What:		/sys/bus/iio/devices/iio:deviceX/frc
> > > > +Date:		April 2020
> > > > +KernelVersion:	5.8
> > > > +Contact:	linux-iio@vger.kernel.org
> > > > +Description:
> > > > +		Forced recalibration is used to compensate for sensor drifts
> > > > +		when a reference value of CO2 concentration in close proximity
> > > > +		to the sensor is available. Writing attribute will set frc
> > > > +		value. Upon reading current frc is returned. Units are
> > > > +		millibars.
>
> Could we implement this by just writing to the main channel value?
> Bit of a clunky ABI but sort of logically fits in my head given we are basically
> forcing the value we read to be this one?
>

So the similar to the pressure compensation. Okay.

> > > > +
> > > > +What:		/sys/bus/iio/devices/iio:deviceX/frc_available
> > > > +Date:		April 2020
> > > > +KernelVersion:	5.8
> > > > +Contact:	linux-iio@vger.kernel.org
> > > > +Description:
> > > > +		The range of available values in millibars represented as the
> > > > +		minimum value, the step and the maximum value, all enclosed in
> > > > +		square brackets.
> > > > +
> > > > +What:		/sys/bus/iio/devices/iio:deviceX/temp_offset
> > > > +Date:		April 2020
> > > > +KernelVersion:	5.8
> > > > +Contact:	linux-iio@vger.kernel.org
> > > > +Description:
> > > > +		Sensor readings may be affected by ambient temperature.
> > > > +		Writing temperature offset will compensate for unwanted changes.
> > > > +		Note that written offset gets multiplied by a factor of 100
> > > > +		by a sensor internally.
> > > > +
> > > > +		For example, writing 10 here will correspond to 0.1 degree
> > > > +		Celsius.
>
> This sounds like a calibbias to me which is standard ABI.
>

Right, that could work.

> > > > +
> > > > +What:		/sys/bus/iio/devices/iio:deviceX/temp_offset_available
> > > > +Date:		April 2020
> > > > +KernelVersion:	5.8
> > > > +Contact:	linux-iio@vger.kernel.org
> > > > +Description:
> > > > +		The range of available values in degrees Celsius represented as
> > > > +		the minimum value, the step and the maximum value, all enclosed
> > > > +		in square brackets.
>
> Wrong units for temperature (which is an odd one as we
> lifted them from hwmon before learning the error of our ways and starting to use
> SI units as the base).
>

Does calibbias have _available counterpart?

>
> > > > +
> > > > +What:		/sys/bus/iio/devices/iio:deviceX/reset
> > > > +Date:		April 2020
> > > > +KernelVersion:	5.8
> > > > +Contact:	linux-iio@vger.kernel.org
> > > > +Description:
> > > > +		Software reset mechanism forces sensor into the same state
> > > > +		as after powering up without the need for removing power supply.
> > > > +		Writing any value will reset sensor.
>
> Not seeing an argument here for why you might want to do that other than on
> power up or module probe to get the driver into a known state.
> So currently it's a no to this one - just don't expose it to userspace.
>

If one writes some odd configuration (though allowed) into sensor, for example
out of sheer curiosity and then writes the sane values back sensor needs some
time to recover (i.e start reporting valid measurements again).

So rationale here was that after reset sensor recovers immediately. I'd
say that reset is sometimes useful. Perhaps that could be exported by
means of iio debug api?

>
> > > >
> > >
> > > --
> > >
> > > Peter Meerwald-Stadler
> > > Mobile: +43 664 24 44 418
>

Re: [PATCH 4/6] Documentation: ABI: testing: scd30: document iio attributes

[Jonathan Cameron]

Hi Tomasz,

Replies inline.

On Sun, 26 Apr 2020 13:11:04 +0200
Tomasz Duszynski <tomasz.duszynski@octakon.com> wrote:

> On Sat, Apr 25, 2020 at 08:20:57PM +0100, Jonathan Cameron wrote:
> > On Thu, 23 Apr 2020 17:53:17 +0200
> > Tomasz Duszynski <tomasz.duszynski@octakon.com> wrote:
> >  
> > > On Wed, Apr 22, 2020 at 06:40:17PM +0200, Peter Meerwald-Stadler wrote:  
> > > > On Wed, 22 Apr 2020, Tomasz Duszynski wrote:
> > > >  
> > > > > Add documentation for sensor specific iio attributes.  
> > > >
> > > > minor comments below  
> > >
> > > Thanks.
> > >  
> > > >  
> > > > > Signed-off-by: Tomasz Duszynski <tomasz.duszynski@octakon.com>
> > > > > ---
> > > > >  Documentation/ABI/testing/sysfs-bus-iio-scd30 | 97 +++++++++++++++++++
> > > > >  1 file changed, 97 insertions(+)
> > > > >  create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-scd30
> > > > >
> > > > > diff --git a/Documentation/ABI/testing/sysfs-bus-iio-scd30 b/Documentation/ABI/testing/sysfs-bus-iio-scd30
> > > > > new file mode 100644
> > > > > index 000000000000..0431a718447d
> > > > > --- /dev/null
> > > > > +++ b/Documentation/ABI/testing/sysfs-bus-iio-scd30
> > > > > @@ -0,0 +1,97 @@
> > > > > +What:		/sys/bus/iio/devices/iio:deviceX/pressure_comp
> > > > > +Date:		April 2020
> > > > > +KernelVersion:	5.8
> > > > > +Contact:	linux-iio@vger.kernel.org
> > > > > +Description:
> > > > > +		Given that sensor's CO2 measurement chamber has fixed volume
> > > > > +		pressure changes will affect concentration readings. Writing
> > > > > +		current ambient pressure here will allow senor to make necessary  
> > > >
> > > > sensor
> > > >  
> > >
> > > Okay.
> > >  
> > > > > +		adjustments. Upon reading previously set value is returned.
> > > > > +		Units are millibars.  
> > > >
> > > > unit for pressure in IIO is kilopascal (e.g.
> > > > /sys/bus/iio/devices/iio:deviceX/in_pressure_raw)
> > > >  
> > >
> > > My thinking here was that since these are sensor specific attributes
> > > they don't need to stick to iio conventions and millibars were somewhat
> > > more natural to use. But I guess that's just matter of habit.  
> >
> > You absolutely have to stick to standard units.  Userspace programs
> > aren't going to come read your docs...
> >
> > For other sensors that take a calibration value like this we've reported
> > them via an output channel.  For example the atlas-ph sensor has
> > an 'output temp' channel used for this purpose.
> >  
> 
> Fair enough.
> 
> > It's not ideal or totally intuitive but it does let us avoid expanding
> > the overall ABI.  The argument was something along the lines of
> > 1) Imagine your sensor could control the pressure in the measurement space...
> > 2) An output channel would provide the value to set it to.
> > 3) Now instead we provide a means of saying 'what it is'
> > 4) End result is we write a value and the pressure in the chamber is
> >    that value :)
> >
> > As I said not ideal but the best we can do without having to define a lot
> > of ABI just to deal with compensation factors.
> >
> > This is a rare case where I would document the 'standard' ABI in here
> > to make the point that it is actually providing an estimate of the pressure
> > not controlling it...
> >  
> > >
> > > So generally I am okay with reworking all attrs to accept values in iio
> > > preferred units.
> > >  
> > > > > +
> > > > > +What:		/sys/bus/iio/devices/iio:deviceX/pressure_comp_available
> > > > > +Date:		April 2020
> > > > > +KernelVersion:	5.8
> > > > > +Contact:	linux-iio@vger.kernel.org
> > > > > +Description:
> > > > > +		The range of available values in millibars represented as the
> > > > > +		minimum value, the step and the maximum value, all enclosed in
> > > > > +		square brackets.
> > > > > +
> > > > > +What:		/sys/bus/iio/devices/iio:deviceX/meas_interval
> > > > > +Date:		January 2020
> > > > > +KernelVersion:	5.8
> > > > > +Contact:	linux-iio@vger.kernel.org
> > > > > +Description:
> > > > > +		Amount of time between subsequent measurements. Writing this
> > > > > +		attribute will change measurement interval. Upon reading
> > > > > +		current measurement interval is returned. Units are seconds.  
> >
> > Use the existing ABI sampling frequency which is sort of the inverse of this.
> >  
> 
> Was thinking about it but long periods in Hz simply don't look appealing :).
> 
> No other strong opinions so I'll rework that.

Agreed it can look a bit odd, but we don't want to have multiple controls for the
same thing so we are stuck with it.

> 
> > > > > +
> > > > > +What:		/sys/bus/iio/devices/iio:deviceX/meas_interval_available
> > > > > +Date:		April 2020
> > > > > +KernelVersion:	5.8
> > > > > +Contact:	linux-iio@vger.kernel.org
> > > > > +Description:
> > > > > +		The range of available values in seconds represented as the
> > > > > +		minimum value, the step and the maximum value, all enclosed in
> > > > > +		square brackets.
> > > > > +
> > > > > +What:		/sys/bus/iio/devices/iio:deviceX/asc  
> > Spends some characters to easy of understanding ;)
> >
> > auto_calib_proc_enable maybe?  Or can we get away with the 'somewhat standard
> > calibration (it's used in at least one other driver IIRC)
> >  
> 
> Just self_calibration would do?

I'll think a bit more on this one but probably fine.

> 
> > > > > +Date:		April 2020
> > > > > +KernelVersion:	5.8
> > > > > +Contact:	linux-iio@vger.kernel.org
> > > > > +Description:
> > > > > +		Writing 1 or 0 to this attribute will respectively activate or
> > > > > +		deactivate automatic self calibration procedure. Upon reading 1  
> > > >
> > > > deactivate automatic self calibration (asc) procedure
> > > >  
> > >
> > > That shouldn't be too difficult to realize what asc actually stands for after
> > > reading this short description.
> > >  
> > > > > +		is returned if asc is ongoing, 0 otherwise.
> > > > > +
> > > > > +What:		/sys/bus/iio/devices/iio:deviceX/frc
> > > > > +Date:		April 2020
> > > > > +KernelVersion:	5.8
> > > > > +Contact:	linux-iio@vger.kernel.org
> > > > > +Description:
> > > > > +		Forced recalibration is used to compensate for sensor drifts
> > > > > +		when a reference value of CO2 concentration in close proximity
> > > > > +		to the sensor is available. Writing attribute will set frc
> > > > > +		value. Upon reading current frc is returned. Units are
> > > > > +		millibars.  
> >
> > Could we implement this by just writing to the main channel value?
> > Bit of a clunky ABI but sort of logically fits in my head given we are basically
> > forcing the value we read to be this one?
> >  
> 
> So the similar to the pressure compensation. Okay.
> 
> > > > > +
> > > > > +What:		/sys/bus/iio/devices/iio:deviceX/frc_available
> > > > > +Date:		April 2020
> > > > > +KernelVersion:	5.8
> > > > > +Contact:	linux-iio@vger.kernel.org
> > > > > +Description:
> > > > > +		The range of available values in millibars represented as the
> > > > > +		minimum value, the step and the maximum value, all enclosed in
> > > > > +		square brackets.
> > > > > +
> > > > > +What:		/sys/bus/iio/devices/iio:deviceX/temp_offset
> > > > > +Date:		April 2020
> > > > > +KernelVersion:	5.8
> > > > > +Contact:	linux-iio@vger.kernel.org
> > > > > +Description:
> > > > > +		Sensor readings may be affected by ambient temperature.
> > > > > +		Writing temperature offset will compensate for unwanted changes.
> > > > > +		Note that written offset gets multiplied by a factor of 100
> > > > > +		by a sensor internally.
> > > > > +
> > > > > +		For example, writing 10 here will correspond to 0.1 degree
> > > > > +		Celsius.  
> >
> > This sounds like a calibbias to me which is standard ABI.
> >  
> 
> Right, that could work.
> 
> > > > > +
> > > > > +What:		/sys/bus/iio/devices/iio:deviceX/temp_offset_available
> > > > > +Date:		April 2020
> > > > > +KernelVersion:	5.8
> > > > > +Contact:	linux-iio@vger.kernel.org
> > > > > +Description:
> > > > > +		The range of available values in degrees Celsius represented as
> > > > > +		the minimum value, the step and the maximum value, all enclosed
> > > > > +		in square brackets.  
> >
> > Wrong units for temperature (which is an odd one as we
> > lifted them from hwmon before learning the error of our ways and starting to use
> > SI units as the base).
> >  
> 
> Does calibbias have _available counterpart?

It might not be documented yet (as not sure it's been used) but any attribute has
an available counterpart.  That's effectively standard ABI.

> 
> >  
> > > > > +
> > > > > +What:		/sys/bus/iio/devices/iio:deviceX/reset
> > > > > +Date:		April 2020
> > > > > +KernelVersion:	5.8
> > > > > +Contact:	linux-iio@vger.kernel.org
> > > > > +Description:
> > > > > +		Software reset mechanism forces sensor into the same state
> > > > > +		as after powering up without the need for removing power supply.
> > > > > +		Writing any value will reset sensor.  
> >
> > Not seeing an argument here for why you might want to do that other than on
> > power up or module probe to get the driver into a known state.
> > So currently it's a no to this one - just don't expose it to userspace.
> >  
> 
> If one writes some odd configuration (though allowed) into sensor, for example
> out of sheer curiosity and then writes the sane values back sensor needs some
> time to recover (i.e start reporting valid measurements again).
> 
> So rationale here was that after reset sensor recovers immediately. I'd
> say that reset is sometimes useful. Perhaps that could be exported by
> means of iio debug api?

Debugfs would be fine

> 
> >  
> > > > >  
> > > >
> > > > --
> > > >
> > > > Peter Meerwald-Stadler
> > > > Mobile: +43 664 24 44 418  
> >  

[PATCH 5/6] dt-bindings: iio: scd30: add device binding file

[Tomasz Duszynski]

Add SCD30 sensor binding file.

Signed-off-by: Tomasz Duszynski <tomasz.duszynski@octakon.com>
---
 .../iio/chemical/sensirion,scd30.yaml         | 71 +++++++++++++++++++
 1 file changed, 71 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/chemical/sensirion,scd30.yaml

diff --git a/Documentation/devicetree/bindings/iio/chemical/sensirion,scd30.yaml b/Documentation/devicetree/bindings/iio/chemical/sensirion,scd30.yaml
new file mode 100644
index 000000000000..b092b2530c76
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/chemical/sensirion,scd30.yaml
@@ -0,0 +1,71 @@
+# SPDX-License-Identifier: GPL-2.0
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/chemical/sensirion,scd30.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Sensirion SCD30 carbon dioxide sensor
+
+maintainers:
+  - Tomasz Duszynski <tomasz.duszynski@octakon.com>
+
+description: |
+  Air quality sensor capable of measuring co2 concentration, temperature
+  and relative humidity.
+
+properties:
+  compatible:
+    enum:
+      - sensirion,scd30
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  vdd-supply: true
+
+  sensirion,sel-gpios:
+    description: GPIO connected to the SEL line
+    maxItems: 1
+
+  sensirion,pwm-gpios:
+    description: GPIO connected to the PWM line
+    maxItems: 1
+
+required:
+  - compatible
+  - vdd-supply
+
+additionalProperties: false
+
+examples:
+  - |
+    # include <dt-bindings/gpio/gpio.h>
+    # include <dt-bindings/interrupt-controller/irq.h>
+    i2c {
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      scd30@61 {
+        compatible = "sensirion,scd30";
+        reg = <0x61>;
+        vdd-supply = <&vdd>;
+        interrupt-parrent = <&gpio0>;
+        interrupts = <0 IRQ_TYPE_LEVEL_HIGH>;
+      };
+    };
+  - |
+    # include <dt-bindings/gpio/gpio.h>
+    # include <dt-bindings/interrupt-controller/irq.h>
+    serial {
+      scd30 {
+        compatible = "sensirion,scd30";
+        vdd-supply = <&vdd>;
+        interrupt-parrent = <&gpio0>;
+        interrupts = <0 IRQ_TYPE_LEVEL_HIGH>;
+      };
+    };
+
+...
-- 
2.26.1

Re: [PATCH 5/6] dt-bindings: iio: scd30: add device binding file

[Jonathan Cameron]

On Wed, 22 Apr 2020 16:11:34 +0200
Tomasz Duszynski <tomasz.duszynski@octakon.com> wrote:

> Add SCD30 sensor binding file.
> 
> Signed-off-by: Tomasz Duszynski <tomasz.duszynski@octakon.com>
> ---
>  .../iio/chemical/sensirion,scd30.yaml         | 71 +++++++++++++++++++
>  1 file changed, 71 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/chemical/sensirion,scd30.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/chemical/sensirion,scd30.yaml b/Documentation/devicetree/bindings/iio/chemical/sensirion,scd30.yaml
> new file mode 100644
> index 000000000000..b092b2530c76
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/chemical/sensirion,scd30.yaml
> @@ -0,0 +1,71 @@
> +# SPDX-License-Identifier: GPL-2.0

Dual license preferred with BSD for bindings.

> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/chemical/sensirion,scd30.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Sensirion SCD30 carbon dioxide sensor
> +
> +maintainers:
> +  - Tomasz Duszynski <tomasz.duszynski@octakon.com>
> +
> +description: |
> +  Air quality sensor capable of measuring co2 concentration, temperature
> +  and relative humidity.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - sensirion,scd30
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  vdd-supply: true
> +
> +  sensirion,sel-gpios:
> +    description: GPIO connected to the SEL line
> +    maxItems: 1
> +
> +  sensirion,pwm-gpios:
> +    description: GPIO connected to the PWM line
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - vdd-supply

Only enable it I think in the driver.  Should be fine with a stub regulator
as provided if we don't specify one in the dt binding because it's not
controllable and we are lazy.

> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    # include <dt-bindings/gpio/gpio.h>
> +    # include <dt-bindings/interrupt-controller/irq.h>
> +    i2c {
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +
> +      scd30@61 {
> +        compatible = "sensirion,scd30";
> +        reg = <0x61>;
> +        vdd-supply = <&vdd>;
> +        interrupt-parrent = <&gpio0>;
> +        interrupts = <0 IRQ_TYPE_LEVEL_HIGH>;
> +      };
> +    };
> +  - |
> +    # include <dt-bindings/gpio/gpio.h>

Why gpio?

> +    # include <dt-bindings/interrupt-controller/irq.h>
> +    serial {
> +      scd30 {
> +        compatible = "sensirion,scd30";
> +        vdd-supply = <&vdd>;
> +        interrupt-parrent = <&gpio0>;
> +        interrupts = <0 IRQ_TYPE_LEVEL_HIGH>;
> +      };
> +    };
> +
> +...

Re: [PATCH 5/6] dt-bindings: iio: scd30: add device binding file

[Tomasz Duszynski]

On Sat, Apr 25, 2020 at 08:23:41PM +0100, Jonathan Cameron wrote:
> On Wed, 22 Apr 2020 16:11:34 +0200
> Tomasz Duszynski <tomasz.duszynski@octakon.com> wrote:
>
> > Add SCD30 sensor binding file.
> >
> > Signed-off-by: Tomasz Duszynski <tomasz.duszynski@octakon.com>
> > ---
> >  .../iio/chemical/sensirion,scd30.yaml         | 71 +++++++++++++++++++
> >  1 file changed, 71 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/iio/chemical/sensirion,scd30.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/iio/chemical/sensirion,scd30.yaml b/Documentation/devicetree/bindings/iio/chemical/sensirion,scd30.yaml
> > new file mode 100644
> > index 000000000000..b092b2530c76
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/chemical/sensirion,scd30.yaml
> > @@ -0,0 +1,71 @@
> > +# SPDX-License-Identifier: GPL-2.0
>
> Dual license preferred with BSD for bindings.
>

Okay.

> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/iio/chemical/sensirion,scd30.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Sensirion SCD30 carbon dioxide sensor
> > +
> > +maintainers:
> > +  - Tomasz Duszynski <tomasz.duszynski@octakon.com>
> > +
> > +description: |
> > +  Air quality sensor capable of measuring co2 concentration, temperature
> > +  and relative humidity.
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - sensirion,scd30
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +  vdd-supply: true
> > +
> > +  sensirion,sel-gpios:
> > +    description: GPIO connected to the SEL line
> > +    maxItems: 1
> > +
> > +  sensirion,pwm-gpios:
> > +    description: GPIO connected to the PWM line
> > +    maxItems: 1
> > +
> > +required:
> > +  - compatible
> > +  - vdd-supply
>
> Only enable it I think in the driver.  Should be fine with a stub regulator
> as provided if we don't specify one in the dt binding because it's not
> controllable and we are lazy.
>
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    # include <dt-bindings/gpio/gpio.h>
> > +    # include <dt-bindings/interrupt-controller/irq.h>
> > +    i2c {
> > +      #address-cells = <1>;
> > +      #size-cells = <0>;
> > +
> > +      scd30@61 {
> > +        compatible = "sensirion,scd30";
> > +        reg = <0x61>;
> > +        vdd-supply = <&vdd>;
> > +        interrupt-parrent = <&gpio0>;
> > +        interrupts = <0 IRQ_TYPE_LEVEL_HIGH>;
> > +      };
> > +    };
> > +  - |
> > +    # include <dt-bindings/gpio/gpio.h>
>
> Why gpio?
>

That shouldn't be here. I have some local changes where I use sel-gpios hence
this include.

> > +    # include <dt-bindings/interrupt-controller/irq.h>
> > +    serial {
> > +      scd30 {
> > +        compatible = "sensirion,scd30";
> > +        vdd-supply = <&vdd>;
> > +        interrupt-parrent = <&gpio0>;
> > +        interrupts = <0 IRQ_TYPE_LEVEL_HIGH>;
> > +      };
> > +    };
> > +
> > +...
>

Re: [PATCH 5/6] dt-bindings: iio: scd30: add device binding file

[Rob Herring]

On Wed, 22 Apr 2020 16:11:34 +0200, Tomasz Duszynski wrote:
> Add SCD30 sensor binding file.
> 
> Signed-off-by: Tomasz Duszynski <tomasz.duszynski@octakon.com>
> ---
>  .../iio/chemical/sensirion,scd30.yaml         | 71 +++++++++++++++++++
>  1 file changed, 71 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/chemical/sensirion,scd30.yaml
> 

My bot found errors running 'make dt_binding_check' on your patch:

/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/iio/chemical/sensirion,scd30.example.dt.yaml: scd30@61: 'interrupt-parrent' does not match any of the regexes: 'pinctrl-[0-9]+'
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/iio/chemical/sensirion,scd30.example.dt.yaml: scd30: 'interrupt-parrent' does not match any of the regexes: 'pinctrl-[0-9]+'

See https://patchwork.ozlabs.org/patch/1275131

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure dt-schema is up to date:

pip3 install git+https://github.com/devicetree-org/dt-schema.git@master --upgrade

Please check and re-submit.

Re: [PATCH 5/6] dt-bindings: iio: scd30: add device binding file

[Tomasz Duszynski]

On Mon, Apr 27, 2020 at 04:28:26PM -0500, Rob Herring wrote:
> On Wed, 22 Apr 2020 16:11:34 +0200, Tomasz Duszynski wrote:
> > Add SCD30 sensor binding file.
> >
> > Signed-off-by: Tomasz Duszynski <tomasz.duszynski@octakon.com>
> > ---
> >  .../iio/chemical/sensirion,scd30.yaml         | 71 +++++++++++++++++++
> >  1 file changed, 71 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/iio/chemical/sensirion,scd30.yaml
> >
>
> My bot found errors running 'make dt_binding_check' on your patch:
>
> /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/iio/chemical/sensirion,scd30.example.dt.yaml: scd30@61: 'interrupt-parrent' does not match any of the regexes: 'pinctrl-[0-9]+'
> /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/iio/chemical/sensirion,scd30.example.dt.yaml: scd30: 'interrupt-parrent' does not match any of the regexes: 'pinctrl-[0-9]+'

Well, a typo here.

>
> See https://patchwork.ozlabs.org/patch/1275131
>
> If you already ran 'make dt_binding_check' and didn't see the above
> error(s), then make sure dt-schema is up to date:
>
> pip3 install git+https://github.com/devicetree-org/dt-schema.git@master --upgrade
>
> Please check and re-submit.

[PATCH 6/6] MAINTAINERS: add myself as a SCD30 driver maintainer

[Tomasz Duszynski]

Add myself as a SCD30 driver maintainer.

Signed-off-by: Tomasz Duszynski <tomasz.duszynski@octakon.com>
---
 MAINTAINERS | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index c900a82f7434..e11c5594199d 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -15001,6 +15001,15 @@ S:	Maintained
 F:	drivers/misc/phantom.c
 F:	include/uapi/linux/phantom.h
 
+SENSIRION SCD30 CARBON DIOXIDE SENSOR DRIVER
+M:	Tomasz Duszynski <tomasz.duszynski@octakon.com>
+S:	Maintained
+F:	drivers/iio/chemical/scd30.h
+F:	drivers/iio/chemical/scd30_core.c
+F:	drivers/iio/chemical/scd30_i2c.c
+F:	drivers/iio/chemical/scd30_serial.c
+F:	Documentation/devicetree/bindings/iio/chemical/sensirion,scd30.yaml
+
 SENSIRION SPS30 AIR POLLUTION SENSOR DRIVER
 M:	Tomasz Duszynski <tduszyns@gmail.com>
 S:	Maintained
-- 
2.26.1

Re: [PATCH 6/6] MAINTAINERS: add myself as a SCD30 driver maintainer

[Andy Shevchenko]

On Wed, Apr 22, 2020 at 5:23 PM Tomasz Duszynski
<tomasz.duszynski@octakon.com> wrote:
>
> Add myself as a SCD30 driver maintainer.

> +SENSIRION SCD30 CARBON DIOXIDE SENSOR DRIVER
> +M:     Tomasz Duszynski <tomasz.duszynski@octakon.com>
> +S:     Maintained
> +F:     drivers/iio/chemical/scd30.h
> +F:     drivers/iio/chemical/scd30_core.c
> +F:     drivers/iio/chemical/scd30_i2c.c
> +F:     drivers/iio/chemical/scd30_serial.c
> +F:     Documentation/devicetree/bindings/iio/chemical/sensirion,scd30.yaml

Broken order. Run
  scripts/parse-maintainers.pl --input=MAINTAINERS --output=MAINTAINERS --order
to fix.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 6/6] MAINTAINERS: add myself as a SCD30 driver maintainer

[Tomasz Duszynski]

On Wed, Apr 22, 2020 at 10:25:03PM +0300, Andy Shevchenko wrote:
> On Wed, Apr 22, 2020 at 5:23 PM Tomasz Duszynski
> <tomasz.duszynski@octakon.com> wrote:
> >
> > Add myself as a SCD30 driver maintainer.
>
> > +SENSIRION SCD30 CARBON DIOXIDE SENSOR DRIVER
> > +M:     Tomasz Duszynski <tomasz.duszynski@octakon.com>
> > +S:     Maintained
> > +F:     drivers/iio/chemical/scd30.h
> > +F:     drivers/iio/chemical/scd30_core.c
> > +F:     drivers/iio/chemical/scd30_i2c.c
> > +F:     drivers/iio/chemical/scd30_serial.c
> > +F:     Documentation/devicetree/bindings/iio/chemical/sensirion,scd30.yaml
>
> Broken order. Run
>   scripts/parse-maintainers.pl --input=MAINTAINERS --output=MAINTAINERS --order
> to fix.

Indeed. That order argument seems to be some fairly recent addition. Thanks for
pointing this out.

>
> --
> With Best Regards,
> Andy Shevchenko