linux-iio.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/4] iio: chemical: Add support for Sensirion SCD4x CO2 sensor
@ 2021-09-13  8:00 Roan van Dijk
  2021-09-13  8:00 ` [PATCH v2 1/4] dt-bindings: iio: chemical: sensirion,scd4x: Add yaml description Roan van Dijk
                   ` (3 more replies)
  0 siblings, 4 replies; 7+ messages in thread
From: Roan van Dijk @ 2021-09-13  8:00 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: Rob Herring, Tomasz Duszynski, linux-iio, devicetree,
	linux-kernel, david, Lars-Peter Clausen, Roan van Dijk

This series adds support for the Sensirion SCD4x sensor.

The driver supports continuous reads of temperature, relative humdity and CO2
concentration. There is an interval of 5 seconds between readings. During
this interval the drivers checks if the sensor has new data available.

The driver is based on the scd30 driver. However, The scd4x has become too
different to just expand the scd30 driver. I made a new driver instead of
expanding the scd30 driver. I hope I made the right choice by doing so?

Changes since v2:
scd4x.c:
  - Change boolean operations
  - Document scope of lock
  - Remove device *dev from struct
  - Add goto block for errror handling
  - Add function to read value per channel in read_raw
  - Fix bug with lock in error paths
  - Remove conversion of humidity and temperature values
  - Add scale and offset to temperature channel
  - Add scale to humidity channel
  - Move memset out of locked section
  - Remove unused irq functions
  - Move device register at end of probe function
Documentation:
  - Copy content of sysfs-bus-iio-scd30 to sysfs-bus-iio
  - Remove Documentation/ABI/testing/sysfs-bus-iio-scd30

Changes since v1:
dt-bindings:
  - Separated compatible string for each sensor type
scd4x.c:
  - Changed probe, resume and suspend functions to static
  - Added SIMPLE_DEV_PM_OPS function call for power management
    operations.

Roan van Dijk (4):
  dt-bindings: iio: chemical: sensirion,scd4x: Add yaml description
  MAINTAINERS: Add myself as maintainer of the scd4x driver
  drivers: iio: chemical: Add support for Sensirion SCD4x CO2 sensor
  iio: documentation: Document scd4x calibration use

 Documentation/ABI/testing/sysfs-bus-iio       |  35 +
 Documentation/ABI/testing/sysfs-bus-iio-scd30 |  34 -
 .../iio/chemical/sensirion,scd4x.yaml         |  46 ++
 MAINTAINERS                                   |   6 +
 drivers/iio/chemical/Kconfig                  |  13 +
 drivers/iio/chemical/Makefile                 |   1 +
 drivers/iio/chemical/scd4x.c                  | 682 ++++++++++++++++++
 7 files changed, 783 insertions(+), 34 deletions(-)
 delete mode 100644 Documentation/ABI/testing/sysfs-bus-iio-scd30
 create mode 100644 Documentation/devicetree/bindings/iio/chemical/sensirion,scd4x.yaml
 create mode 100644 drivers/iio/chemical/scd4x.c

-- 
2.30.2


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH v2 1/4] dt-bindings: iio: chemical: sensirion,scd4x: Add yaml description
  2021-09-13  8:00 [PATCH v2 0/4] iio: chemical: Add support for Sensirion SCD4x CO2 sensor Roan van Dijk
@ 2021-09-13  8:00 ` Roan van Dijk
  2021-09-13  8:00 ` [PATCH v2 2/4] MAINTAINERS: Add myself as maintainer of the scd4x driver Roan van Dijk
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 7+ messages in thread
From: Roan van Dijk @ 2021-09-13  8:00 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: Rob Herring, Tomasz Duszynski, linux-iio, devicetree,
	linux-kernel, david, Lars-Peter Clausen, Roan van Dijk,
	Rob Herring

Add documentation for the SCD4x carbon dioxide sensor from Sensirion.

Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Roan van Dijk <roan@protonic.nl>
---
 .../iio/chemical/sensirion,scd4x.yaml         | 46 +++++++++++++++++++
 1 file changed, 46 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/chemical/sensirion,scd4x.yaml

diff --git a/Documentation/devicetree/bindings/iio/chemical/sensirion,scd4x.yaml b/Documentation/devicetree/bindings/iio/chemical/sensirion,scd4x.yaml
new file mode 100644
index 000000000000..798f48d05279
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/chemical/sensirion,scd4x.yaml
@@ -0,0 +1,46 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/chemical/sensirion,scd4x.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Sensirion SCD4X carbon dioxide sensor
+
+maintainers:
+  - Roan van Dijk <roan@protonic.nl>
+
+description: |
+  Air quality sensor capable of measuring co2 concentration, temperature
+  and relative humidity.
+
+properties:
+  compatible:
+    enum:
+      - sensirion,scd40
+      - sensirion,scd41
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  vdd-supply: true
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    i2c {
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      co2-sensor@62 {
+        compatible = "sensirion,scd41";
+        reg = <0x62>;
+      };
+    };
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH v2 2/4] MAINTAINERS: Add myself as maintainer of the scd4x driver
  2021-09-13  8:00 [PATCH v2 0/4] iio: chemical: Add support for Sensirion SCD4x CO2 sensor Roan van Dijk
  2021-09-13  8:00 ` [PATCH v2 1/4] dt-bindings: iio: chemical: sensirion,scd4x: Add yaml description Roan van Dijk
@ 2021-09-13  8:00 ` Roan van Dijk
  2021-09-13  8:00 ` [PATCH v2 3/4] drivers: iio: chemical: Add support for Sensirion SCD4x CO2 sensor Roan van Dijk
  2021-09-13  8:00 ` [PATCH v2 4/4] iio: documentation: Document scd4x calibration use Roan van Dijk
  3 siblings, 0 replies; 7+ messages in thread
From: Roan van Dijk @ 2021-09-13  8:00 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: Rob Herring, Tomasz Duszynski, linux-iio, devicetree,
	linux-kernel, david, Lars-Peter Clausen, Roan van Dijk

Signed-off-by: Roan van Dijk <roan@protonic.nl>
---
 MAINTAINERS | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index eeb4c70b3d5b..d524b7fd7d5c 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -16880,6 +16880,12 @@ F:	drivers/iio/chemical/scd30_core.c
 F:	drivers/iio/chemical/scd30_i2c.c
 F:	drivers/iio/chemical/scd30_serial.c
 
+SENSIRION SCD4X CARBON DIOXIDE SENSOR DRIVER
+M:	Roan van Dijk <roan@protonic.nl>
+S:	Maintained
+F:	Documentation/devicetree/bindings/iio/chemical/sensirion,scd4x.yaml
+F:	drivers/iio/chemical/scd4x.c
+
 SENSIRION SGP40 GAS SENSOR DRIVER
 M:	Andreas Klinger <ak@it-klinger.de>
 S:	Maintained
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH v2 3/4] drivers: iio: chemical: Add support for Sensirion SCD4x CO2 sensor
  2021-09-13  8:00 [PATCH v2 0/4] iio: chemical: Add support for Sensirion SCD4x CO2 sensor Roan van Dijk
  2021-09-13  8:00 ` [PATCH v2 1/4] dt-bindings: iio: chemical: sensirion,scd4x: Add yaml description Roan van Dijk
  2021-09-13  8:00 ` [PATCH v2 2/4] MAINTAINERS: Add myself as maintainer of the scd4x driver Roan van Dijk
@ 2021-09-13  8:00 ` Roan van Dijk
  2021-09-19 16:41   ` Jonathan Cameron
  2021-09-13  8:00 ` [PATCH v2 4/4] iio: documentation: Document scd4x calibration use Roan van Dijk
  3 siblings, 1 reply; 7+ messages in thread
From: Roan van Dijk @ 2021-09-13  8:00 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: Rob Herring, Tomasz Duszynski, linux-iio, devicetree,
	linux-kernel, david, Lars-Peter Clausen, Roan van Dijk

This is a driver for the SCD4x CO2 sensor from Sensirion. The sensor is
able to measure CO2 concentration, temperature and relative humdity.
The sensor uses a photoacoustic principle for measuring CO2 concentration.
An I2C interface is supported by this driver in order to communicate with
the sensor.

Reviewed-by: Jonathan Cameron <jic23@kernel.org>
Signed-off-by: Roan van Dijk <roan@protonic.nl>
---
 drivers/iio/chemical/Kconfig  |  13 +
 drivers/iio/chemical/Makefile |   1 +
 drivers/iio/chemical/scd4x.c  | 682 ++++++++++++++++++++++++++++++++++
 3 files changed, 696 insertions(+)
 create mode 100644 drivers/iio/chemical/scd4x.c

diff --git a/drivers/iio/chemical/Kconfig b/drivers/iio/chemical/Kconfig
index c03667e62732..f4d2fcb1b9ac 100644
--- a/drivers/iio/chemical/Kconfig
+++ b/drivers/iio/chemical/Kconfig
@@ -118,6 +118,19 @@ config SCD30_SERIAL
 	  To compile this driver as a module, choose M here: the module will
 	  be called scd30_serial.
 
+config SCD4X
+	tristate "SCD4X carbon dioxide sensor driver"
+	select IIO_BUFFER
+	select IIO_TRIGGERED_BUFFER
+	depends on I2C
+	select CRC8
+	help
+	  Say Y here to build support for the Sensirion SCD4X sensor with cabon
+	  dioxide, relative humidity and temperature sensing capabilities
+
+	  To compile this driver as a module, choose M here: the module will
+	  be called scd4x.
+
 config SENSIRION_SGP30
 	tristate "Sensirion SGPxx gas sensors"
 	depends on I2C
diff --git a/drivers/iio/chemical/Makefile b/drivers/iio/chemical/Makefile
index d07af581f234..81b29d9eb6a2 100644
--- a/drivers/iio/chemical/Makefile
+++ b/drivers/iio/chemical/Makefile
@@ -15,6 +15,7 @@ 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_SCD4X) += scd4x.o
 obj-$(CONFIG_SENSIRION_SGP30)	+= sgp30.o
 obj-$(CONFIG_SENSIRION_SGP40)	+= sgp40.o
 obj-$(CONFIG_SPS30) += sps30.o
diff --git a/drivers/iio/chemical/scd4x.c b/drivers/iio/chemical/scd4x.c
new file mode 100644
index 000000000000..8f795c972a93
--- /dev/null
+++ b/drivers/iio/chemical/scd4x.c
@@ -0,0 +1,682 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Sensirion SCD4X carbon dioxide sensor i2c driver
+ *
+ * Copyright (C) 2021 Protonic Holland
+ * Author: Roan van Dijk <roan@protonic.nl>
+ *
+ * I2C slave address: 0x62
+ *
+ * Datasheets:
+ * https://www.sensirion.com/file/datasheet_scd4x
+ */
+
+#include <asm/unaligned.h>
+#include <linux/crc8.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/i2c.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/kernel.h>
+#include <linux/mutex.h>
+#include <linux/string.h>
+#include <linux/sysfs.h>
+#include <linux/types.h>
+
+#define SCD4X_CRC8_POLYNOMIAL 0x31
+#define SCD4X_TIMEOUT_ERR 1000
+#define SCD4X_READ_BUF_SIZE 9
+#define SCD4X_COMMAND_BUF_SIZE 2
+#define SCD4X_WRITE_BUF_SIZE 5
+#define SCD4X_FRC_MIN_PPM 400
+#define SCD4X_FRC_MAX_PPM 2000
+#define SCD4X_READY_MASK 0x01
+
+/*Commands SCD4X*/
+enum scd4x_cmd {
+	CMD_START_MEAS          = 0x21b1,
+	CMD_READ_MEAS           = 0xec05,
+	CMD_STOP_MEAS           = 0x3f86,
+	CMD_SET_TEMP_OFFSET     = 0x241d,
+	CMD_GET_TEMP_OFFSET     = 0x2318,
+	CMD_FRC                 = 0x362f,
+	CMD_SET_ASC             = 0x2416,
+	CMD_GET_ASC             = 0x2313,
+	CMD_GET_DATA_READY      = 0xe4b8,
+};
+
+enum scd4x_channel_idx {
+	SCD4X_CO2,
+	SCD4X_TEMP,
+	SCD4X_HR,
+};
+
+struct scd4x_state {
+	struct i2c_client *client;
+	/* maintain access to device, to prevent concurrent reads/writes */
+	struct mutex lock;
+	struct regulator *vdd;
+};
+
+DECLARE_CRC8_TABLE(scd4x_crc8_table);
+
+static int scd4x_i2c_xfer(struct scd4x_state *state, char *txbuf, int txsize,
+				char *rxbuf, int rxsize)
+{
+	struct i2c_client *client = state->client;
+	int ret;
+
+	ret = i2c_master_send(client, txbuf, txsize);
+
+	if (ret < 0)
+		return ret;
+	if (ret != txsize)
+		return -EIO;
+
+	if (rxsize == 0)
+		return 0;
+
+	ret = i2c_master_recv(client, rxbuf, rxsize);
+	if (ret < 0)
+		return ret;
+	if (ret != rxsize)
+		return -EIO;
+
+	return 0;
+}
+
+static int scd4x_send_command(struct scd4x_state *state, enum scd4x_cmd cmd)
+{
+	char buf[SCD4X_COMMAND_BUF_SIZE];
+	int ret;
+
+	/*
+	 * Measurement needs to be stopped before sending commands.
+	 * Except stop and start command.
+	 */
+	if ((cmd != CMD_STOP_MEAS) && (cmd != CMD_START_MEAS)) {
+
+		ret = scd4x_send_command(state, CMD_STOP_MEAS);
+		if (ret)
+			return ret;
+
+		/* execution time for stopping measurement */
+		msleep_interruptible(500);
+	}
+
+	put_unaligned_be16(cmd, buf);
+	ret = scd4x_i2c_xfer(state, buf, 2, buf, 0);
+	if (ret)
+		return ret;
+
+	if ((cmd != CMD_STOP_MEAS) && (cmd != CMD_START_MEAS)) {
+		ret = scd4x_send_command(state, CMD_START_MEAS);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+static int scd4x_read(struct scd4x_state *state, enum scd4x_cmd cmd,
+			void *response, int byte_cnt)
+{
+	struct i2c_client *client = state->client;
+	char buf[SCD4X_READ_BUF_SIZE];
+	char *rsp = response;
+	int i, ret;
+	char crc;
+
+	/*
+	 * Measurement needs to be stopped before sending commands.
+	 * Except for reading measurement and data ready command.
+	 */
+	if ((cmd != CMD_GET_DATA_READY) && (cmd != CMD_READ_MEAS)) {
+		ret = scd4x_send_command(state, CMD_STOP_MEAS);
+		if (ret)
+			return ret;
+
+		/* execution time for stopping measurement */
+		msleep_interruptible(500);
+	}
+
+	put_unaligned_be16(cmd, buf);
+	ret = scd4x_i2c_xfer(state, buf, 2, buf, byte_cnt);
+	if (ret)
+		return ret;
+
+	for (i = 0; i < byte_cnt; i += 3) {
+		crc = crc8(scd4x_crc8_table, buf + i, 2, CRC8_INIT_VALUE);
+		if (crc != buf[i + 2]) {
+			dev_err(&client->dev, "CRC error\n");
+			return -EIO;
+		}
+
+		*rsp++ = buf[i];
+		*rsp++ = buf[i + 1];
+	}
+
+	/* start measurement */
+	if ((cmd != CMD_GET_DATA_READY) && (cmd != CMD_READ_MEAS)) {
+		ret = scd4x_send_command(state, CMD_START_MEAS);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+static int scd4x_write(struct scd4x_state *state, enum scd4x_cmd cmd, uint16_t arg)
+{
+	char buf[SCD4X_WRITE_BUF_SIZE];
+	int ret;
+	char crc;
+
+	put_unaligned_be16(cmd, buf);
+	put_unaligned_be16(arg, buf + 2);
+
+	crc = crc8(scd4x_crc8_table, buf + 2, 2, CRC8_INIT_VALUE);
+	buf[4] = crc;
+
+	/* measurement needs to be stopped before sending commands */
+	ret = scd4x_send_command(state, CMD_STOP_MEAS);
+	if (ret)
+		return ret;
+
+	/* execution time */
+	msleep_interruptible(500);
+
+	ret = scd4x_i2c_xfer(state, buf, SCD4X_WRITE_BUF_SIZE, buf, 0);
+	if (ret)
+		return ret;
+
+	/* start measurement, except for forced calibration command */
+	if (cmd != CMD_FRC) {
+		ret = scd4x_send_command(state, CMD_START_MEAS);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+static int scd4x_write_and_fetch(struct scd4x_state *state, enum scd4x_cmd cmd,
+				uint16_t arg, void *response, int byte_cnt)
+{
+	struct i2c_client *client = state->client;
+	char buf[SCD4X_READ_BUF_SIZE];
+	char *rsp = response;
+	int i, ret;
+	char crc;
+
+	ret = scd4x_write(state, CMD_FRC, arg);
+	if (ret)
+		goto err;
+
+	/* Execution time */
+	msleep_interruptible(400);
+
+	ret = i2c_master_recv(client, buf, byte_cnt);
+	if (ret < 0)
+		goto err;
+	if (ret != byte_cnt) {
+		ret = -EIO;
+		goto err;
+	}
+
+	for (i = 0; i < byte_cnt; i += 3) {
+		crc = crc8(scd4x_crc8_table, buf + i, 2, CRC8_INIT_VALUE);
+		if (crc != buf[i + 2]) {
+			dev_err(&client->dev, "CRC error\n");
+			ret = -EIO;
+			goto err;
+		}
+
+		*rsp++ = buf[i];
+		*rsp++ = buf[i + 1];
+	}
+
+	return scd4x_send_command(state, CMD_START_MEAS);
+
+err:
+	/*
+	 * on error try to start the measurement,
+	 * puts sensor back into continuous measurement
+	 */
+	scd4x_send_command(state, CMD_START_MEAS);
+
+	return ret;
+}
+
+static int scd4x_read_meas(struct scd4x_state *state, uint16_t *meas)
+{
+	int i, ret;
+	uint16_t buf[3];
+
+	ret = scd4x_read(state, CMD_READ_MEAS, buf, 9);
+	if (ret)
+		return ret;
+
+	for (i = 0; i < ARRAY_SIZE(buf); i++)
+		meas[i] = be16_to_cpu(buf[i]);
+
+	return 0;
+}
+
+static int scd4x_wait_meas_poll(struct scd4x_state *state)
+{
+	struct i2c_client *client = state->client;
+	int tries = 6;
+	int ret;
+
+	do {
+		uint16_t val;
+
+		ret = scd4x_read(state, CMD_GET_DATA_READY, &val, 3);
+		if (ret)
+			return -EIO;
+		val = be16_to_cpu(val);
+
+		/* new measurement available */
+		if (val & 0x7FF)
+			return 0;
+
+		msleep_interruptible(1000);
+	} while (--tries);
+
+	/* try to start sensor on timeout */
+	ret = scd4x_send_command(state, CMD_START_MEAS);
+	if (ret)
+		dev_err(&client->dev, "failed to start measurement: %d\n", ret);
+
+	return -ETIMEDOUT;
+}
+
+static int scd4x_read_poll(struct scd4x_state *state, uint16_t *buf)
+{
+	int ret;
+
+	ret = scd4x_wait_meas_poll(state);
+	if (ret)
+		return ret;
+
+	return scd4x_read_meas(state, buf);
+}
+
+static int scd4x_read_channel(struct scd4x_state *state, int chan)
+{
+	int ret;
+	uint16_t buf[3];
+
+	ret = scd4x_read_poll(state, buf);
+	if (ret)
+		return ret;
+
+	return buf[chan];
+}
+static int scd4x_read_raw(struct iio_dev *indio_dev,
+			struct iio_chan_spec const *chan, int *val,
+			int *val2, long mask)
+{
+	struct scd4x_state *state = iio_priv(indio_dev);
+	int ret;
+	uint16_t tmp;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		ret = iio_device_claim_direct_mode(indio_dev);
+		if (ret)
+			return ret;
+
+		mutex_lock(&state->lock);
+		ret = scd4x_read_channel(state, chan->address);
+		mutex_unlock(&state->lock);
+
+		iio_device_release_direct_mode(indio_dev);
+		if (ret < 0)
+			return ret;
+
+		*val = ret;
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_SCALE:
+		if (chan->type == IIO_TEMP) {
+			*val = 175000;
+			*val2 = 65536;
+			return IIO_VAL_FRACTIONAL;
+		} else if (chan->type == IIO_HUMIDITYRELATIVE) {
+			*val = 100000;
+			*val2 = 65536;
+			return IIO_VAL_FRACTIONAL;
+		}
+		return -EINVAL;
+	case IIO_CHAN_INFO_OFFSET:
+		*val = -16852;
+		*val2 = 114286;
+		return IIO_VAL_INT_PLUS_MICRO;
+	case IIO_CHAN_INFO_CALIBBIAS:
+		mutex_lock(&state->lock);
+		ret = scd4x_read(state, CMD_GET_TEMP_OFFSET, &tmp, 3);
+		mutex_unlock(&state->lock);
+		if (ret)
+			return ret;
+
+		*val = 175000 * be16_to_cpu(tmp) / 65536;
+
+		return IIO_VAL_INT;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int scd4x_write_raw(struct iio_dev *indio_dev, struct iio_chan_spec const *chan,
+				int val, int val2, long mask)
+{
+	struct scd4x_state *state = iio_priv(indio_dev);
+	int ret = 0;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_CALIBBIAS:
+		val = val * 65536 / 175000;
+
+		mutex_lock(&state->lock);
+		ret = scd4x_write(state, CMD_SET_TEMP_OFFSET, val);
+		mutex_unlock(&state->lock);
+
+		return ret;
+	default:
+		return -EINVAL;
+	}
+}
+
+static ssize_t calibration_auto_enable_show(struct device *dev,
+			struct device_attribute *attr, char *buf)
+{
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct scd4x_state *state = iio_priv(indio_dev);
+	int ret;
+	uint16_t val;
+
+	mutex_lock(&state->lock);
+	ret = scd4x_read(state, CMD_GET_ASC, &val, 3);
+	mutex_unlock(&state->lock);
+	if (ret) {
+		dev_err(dev, "failed to read automatic calibration");
+		return ret;
+	}
+
+	val = be16_to_cpu(val) & SCD4X_READY_MASK;
+
+	return sprintf(buf, "%d\n", val);
+}
+
+static ssize_t calibration_auto_enable_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 scd4x_state *state = iio_priv(indio_dev);
+	bool val;
+	int ret;
+	uint16_t value;
+
+	ret = kstrtobool(buf, &val);
+	if (ret)
+		return ret;
+
+	value = val;
+
+	mutex_lock(&state->lock);
+	ret = scd4x_write(state, CMD_SET_ASC, value);
+	mutex_unlock(&state->lock);
+	if (ret)
+		dev_err(dev, "failed to set automatic calibration");
+
+	return ret ?: len;
+}
+
+static ssize_t calibration_forced_value_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 scd4x_state *state = iio_priv(indio_dev);
+	uint16_t val, arg;
+	int ret;
+
+	ret = kstrtou16(buf, 0, &arg);
+	if (ret)
+		return ret;
+
+	if (arg < SCD4X_FRC_MIN_PPM || arg > SCD4X_FRC_MAX_PPM)
+		return -EINVAL;
+
+	mutex_lock(&state->lock);
+	ret = scd4x_write_and_fetch(state, CMD_FRC, arg, &val, 3);
+	mutex_unlock(&state->lock);
+
+	if (val == 0xff) {
+		dev_err(dev, "forced calibration has failed");
+		return -EINVAL;
+	}
+
+	return ret ?: len;
+}
+
+static IIO_DEVICE_ATTR_RW(calibration_auto_enable, 0);
+static IIO_DEVICE_ATTR_WO(calibration_forced_value, 0);
+
+static struct attribute *scd4x_attrs[] = {
+	&iio_dev_attr_calibration_auto_enable.dev_attr.attr,
+	&iio_dev_attr_calibration_forced_value.dev_attr.attr,
+	NULL
+};
+
+static const struct attribute_group scd4x_attr_group = {
+	.attrs = scd4x_attrs,
+};
+static const struct iio_info scd4x_info = {
+	.attrs = &scd4x_attr_group,
+	.read_raw = scd4x_read_raw,
+	.write_raw = scd4x_write_raw,
+};
+
+static const struct iio_chan_spec scd4x_channels[] = {
+	{
+		.type = IIO_CONCENTRATION,
+		.channel2 = IIO_MOD_CO2,
+		.modified = 1,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+		.address = SCD4X_CO2,
+		.scan_index = SCD4X_CO2,
+		.scan_type = {
+			.sign = 'u',
+			.realbits = 16,
+			.storagebits = 16,
+			.endianness = IIO_BE,
+		},
+	},
+	{
+		.type = IIO_TEMP,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+					BIT(IIO_CHAN_INFO_SCALE) |
+					BIT(IIO_CHAN_INFO_OFFSET) |
+					BIT(IIO_CHAN_INFO_CALIBBIAS),
+		.address = SCD4X_TEMP,
+		.scan_index = SCD4X_TEMP,
+		.scan_type = {
+			.sign = 'u',
+			.realbits = 16,
+			.storagebits = 16,
+			.endianness = IIO_BE,
+		},
+	},
+	{
+		.type = IIO_HUMIDITYRELATIVE,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+					BIT(IIO_CHAN_INFO_SCALE),
+		.address = SCD4X_HR,
+		.scan_index = SCD4X_HR,
+		.scan_type = {
+			.sign = 'u',
+			.realbits = 16,
+			.storagebits = 16,
+			.endianness = IIO_BE,
+		},
+	},
+};
+
+static int __maybe_unused scd4x_suspend(struct device *dev)
+{
+	struct iio_dev *indio_dev = dev_get_drvdata(dev);
+	struct scd4x_state *state  = iio_priv(indio_dev);
+	int ret;
+
+	ret = scd4x_send_command(state, CMD_STOP_MEAS);
+	if (ret)
+		return ret;
+
+	return regulator_disable(state->vdd);
+}
+
+static int __maybe_unused scd4x_resume(struct device *dev)
+{
+	struct iio_dev *indio_dev = dev_get_drvdata(dev);
+	struct scd4x_state *state = iio_priv(indio_dev);
+	int ret;
+
+	ret = regulator_enable(state->vdd);
+	if (ret)
+		return ret;
+
+	return scd4x_send_command(state, CMD_START_MEAS);
+}
+
+static __maybe_unused SIMPLE_DEV_PM_OPS(scd4x_pm_ops, scd4x_suspend, scd4x_resume);
+
+static void scd4x_stop_meas(void *state)
+{
+	scd4x_send_command(state, CMD_STOP_MEAS);
+}
+
+static void scd4x_disable_regulator(void *data)
+{
+	struct scd4x_state *state = data;
+
+	regulator_disable(state->vdd);
+}
+
+static irqreturn_t scd4x_trigger_handler(int irq, void *p)
+{
+	struct iio_poll_func *pf = p;
+	struct iio_dev *indio_dev = pf->indio_dev;
+	struct scd4x_state *state = iio_priv(indio_dev);
+	struct {
+		uint16_t data[3];
+		int64_t ts __aligned(8);
+	} scan;
+	int ret;
+	uint16_t buf[3];
+
+	mutex_lock(&state->lock);
+	ret = scd4x_read_poll(state, buf);
+	mutex_unlock(&state->lock);
+	if (ret)
+		goto out;
+
+	memset(&scan, 0, sizeof(scan));
+	memcpy(scan.data, buf, sizeof(buf));
+
+	iio_push_to_buffers_with_timestamp(indio_dev, &scan, iio_get_time_ns(indio_dev));
+out:
+	iio_trigger_notify_done(indio_dev->trig);
+	return IRQ_HANDLED;
+}
+
+static int scd4x_probe(struct i2c_client *client, const struct i2c_device_id *id)
+{
+	static const unsigned long scd4x_scan_masks[] = { 0x07, 0x00 };
+	struct device *dev = &client->dev;
+	struct iio_dev *indio_dev;
+	struct scd4x_state *state;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*state));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	state = iio_priv(indio_dev);
+	mutex_init(&state->lock);
+	state->client = client;
+	crc8_populate_msb(scd4x_crc8_table, SCD4X_CRC8_POLYNOMIAL);
+
+	indio_dev->info = &scd4x_info;
+	indio_dev->name = client->name;
+	indio_dev->channels = scd4x_channels;
+	indio_dev->num_channels = ARRAY_SIZE(scd4x_channels);
+	indio_dev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_TRIGGERED;
+	indio_dev->available_scan_masks = scd4x_scan_masks;
+
+	state->vdd = devm_regulator_get(dev, "vdd");
+	if (IS_ERR(state->vdd))
+		return dev_err_probe(dev, PTR_ERR(state->vdd), "failed to get regulator\n");
+
+	ret = regulator_enable(state->vdd);
+	if (ret)
+		return ret;
+
+	ret = devm_add_action_or_reset(dev, scd4x_disable_regulator, state);
+	if (ret)
+		return ret;
+
+	ret = scd4x_send_command(state, CMD_STOP_MEAS);
+	if (ret) {
+		dev_err(dev, "failed to stop measurement: %d\n", ret);
+		return ret;
+	}
+
+	/* execution time */
+	msleep_interruptible(500);
+
+	ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL, scd4x_trigger_handler, NULL);
+	if (ret)
+		return ret;
+
+	ret = scd4x_send_command(state, CMD_START_MEAS);
+	if (ret) {
+		dev_err(dev, "failed to start measurement: %d\n", ret);
+		return ret;
+	}
+
+	ret = devm_add_action_or_reset(dev, scd4x_stop_meas, state);
+	if (ret)
+		return ret;
+
+	return devm_iio_device_register(dev, indio_dev);
+}
+
+static const struct of_device_id scd4x_dt_ids[] = {
+	{ .compatible = "sensirion,scd40" },
+	{ .compatible = "sensirion,scd41" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, scd4x_dt_ids);
+
+static struct i2c_driver scd4x_i2c_driver = {
+	.driver = {
+		.name = KBUILD_MODNAME,
+		.of_match_table = scd4x_dt_ids,
+		.pm = &scd4x_pm_ops
+	},
+	.probe = scd4x_probe,
+};
+module_i2c_driver(scd4x_i2c_driver);
+
+MODULE_AUTHOR("Roan van Dijk <roan@protonic.nl>");
+MODULE_DESCRIPTION("Sensirion SCD4X carbon dioxide sensor core driver");
+MODULE_LICENSE("GPL v2");
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH v2 4/4] iio: documentation: Document scd4x calibration use
  2021-09-13  8:00 [PATCH v2 0/4] iio: chemical: Add support for Sensirion SCD4x CO2 sensor Roan van Dijk
                   ` (2 preceding siblings ...)
  2021-09-13  8:00 ` [PATCH v2 3/4] drivers: iio: chemical: Add support for Sensirion SCD4x CO2 sensor Roan van Dijk
@ 2021-09-13  8:00 ` Roan van Dijk
  2021-09-19 16:51   ` Jonathan Cameron
  3 siblings, 1 reply; 7+ messages in thread
From: Roan van Dijk @ 2021-09-13  8:00 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: Rob Herring, Tomasz Duszynski, linux-iio, devicetree,
	linux-kernel, david, Lars-Peter Clausen, Roan van Dijk

Add entries from Documentation/ABI/testing/sysfs-bus-iio-scd30
to Documentation/ABI/testing/sysfs-bus-iio. The attributes of the scd4x
and scd30 are common.

Remove Documentation/ABI/testing/sysfs-bus-iio-scd30.

Signed-off-by: Roan van Dijk <roan@protonic.nl>
---
 Documentation/ABI/testing/sysfs-bus-iio       | 35 +++++++++++++++++++
 Documentation/ABI/testing/sysfs-bus-iio-scd30 | 34 ------------------
 2 files changed, 35 insertions(+), 34 deletions(-)
 delete mode 100644 Documentation/ABI/testing/sysfs-bus-iio-scd30

diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
index 6ad47a67521c..56492c564f72 100644
--- a/Documentation/ABI/testing/sysfs-bus-iio
+++ b/Documentation/ABI/testing/sysfs-bus-iio
@@ -1957,3 +1957,38 @@ Description:
 		Specify the percent for light sensor relative to the channel
 		absolute value that a data field should change before an event
 		is generated. Units are a percentage of the prior reading.
+
+What:		/sys/bus/iio/devices/iio:deviceX/calibration_auto_enable
+Date:		June 2020
+KernelVersion:	5.8
+Contact:	linux-iio@vger.kernel.org
+Description:
+		Contaminants build-up in the measurement chamber or optical
+		elements deterioration leads to sensor drift.
+
+		One can compensate for sensor drift by using automatic self
+		calibration procedure (asc).
+
+		Writing 1 or 0 to this attribute will respectively activate or
+		deactivate asc.
+
+		Upon reading current asc status is returned.
+
+What:		/sys/bus/iio/devices/iio:deviceX/calibration_forced_value
+Date:		June 2020
+KernelVersion:	5.8
+Contact:	linux-iio@vger.kernel.org
+Description:
+		Contaminants build-up in the measurement chamber or optical
+		elements deterioration leads to sensor drift.
+
+		One can compensate for sensor drift by using forced
+		recalibration (frc). This is useful in case there's known
+		co2 reference available nearby the sensor.
+
+		Picking value from the range [400 1 2000] and writing it to the
+		sensor will set frc.
+
+		Upon reading current frc value is returned. Note that after
+		power cycling default value (i.e 400) is returned even though
+		internally sensor had recalibrated itself.
diff --git a/Documentation/ABI/testing/sysfs-bus-iio-scd30 b/Documentation/ABI/testing/sysfs-bus-iio-scd30
deleted file mode 100644
index b9712f390bec..000000000000
--- a/Documentation/ABI/testing/sysfs-bus-iio-scd30
+++ /dev/null
@@ -1,34 +0,0 @@
-What:		/sys/bus/iio/devices/iio:deviceX/calibration_auto_enable
-Date:		June 2020
-KernelVersion:	5.8
-Contact:	linux-iio@vger.kernel.org
-Description:
-		Contaminants build-up in the measurement chamber or optical
-		elements deterioration leads to sensor drift.
-
-		One can compensate for sensor drift by using automatic self
-		calibration procedure (asc).
-
-		Writing 1 or 0 to this attribute will respectively activate or
-		deactivate asc.
-
-		Upon reading current asc status is returned.
-
-What:		/sys/bus/iio/devices/iio:deviceX/calibration_forced_value
-Date:		June 2020
-KernelVersion:	5.8
-Contact:	linux-iio@vger.kernel.org
-Description:
-		Contaminants build-up in the measurement chamber or optical
-		elements deterioration leads to sensor drift.
-
-		One can compensate for sensor drift by using forced
-		recalibration (frc). This is useful in case there's known
-		co2 reference available nearby the sensor.
-
-		Picking value from the range [400 1 2000] and writing it to the
-		sensor will set frc.
-
-		Upon reading current frc value is returned. Note that after
-		power cycling default value (i.e 400) is returned even though
-		internally sensor had recalibrated itself.
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH v2 3/4] drivers: iio: chemical: Add support for Sensirion SCD4x CO2 sensor
  2021-09-13  8:00 ` [PATCH v2 3/4] drivers: iio: chemical: Add support for Sensirion SCD4x CO2 sensor Roan van Dijk
@ 2021-09-19 16:41   ` Jonathan Cameron
  0 siblings, 0 replies; 7+ messages in thread
From: Jonathan Cameron @ 2021-09-19 16:41 UTC (permalink / raw)
  To: Roan van Dijk
  Cc: Rob Herring, Tomasz Duszynski, linux-iio, devicetree,
	linux-kernel, david, Lars-Peter Clausen

On Mon, 13 Sep 2021 10:00:19 +0200
Roan van Dijk <roan@protonic.nl> wrote:

> This is a driver for the SCD4x CO2 sensor from Sensirion. The sensor is
> able to measure CO2 concentration, temperature and relative humdity.
> The sensor uses a photoacoustic principle for measuring CO2 concentration.
> An I2C interface is supported by this driver in order to communicate with
> the sensor.
> 
> Reviewed-by: Jonathan Cameron <jic23@kernel.org>
A small process thing.

Don't add a tag from someone else.  They must explicitly give the tag
on a version of the series.  Given I sign off on IIO patches, I never
give a reviewed-by for anything that will go through the IIO tree
(making this easy to spot :)

In the past there have been discussions about whether it's acceptable
for maintainers taking a patch to add reviewed-by tags to reflect the
review work done. General consensus now is that it is not acceptable and
if anyone wishes to do this they must confirm with the person before doing
so.


> Signed-off-by: Roan van Dijk <roan@protonic.nl>

I took another look at this (was hoping I could just fix anything trivial
and apply) but I think there is just enough to be worth asking you to do a v3
to tidy up a last few corners.  In particular I'm not sure what the right
approach to take for calibbias scaling is.

Thanks,

Jonathan

> ---
>  drivers/iio/chemical/Kconfig  |  13 +
>  drivers/iio/chemical/Makefile |   1 +
>  drivers/iio/chemical/scd4x.c  | 682 ++++++++++++++++++++++++++++++++++
>  3 files changed, 696 insertions(+)
>  create mode 100644 drivers/iio/chemical/scd4x.c
...
> +static int scd4x_read(struct scd4x_state *state, enum scd4x_cmd cmd,
> +			void *response, int byte_cnt)

As for the equivalent write_and_fetch, change the parameter to
reflect the size of response and scale it to the byte_cnt inside
this function.  That is a complexity we want to keep hidden from
as much of the driver as possible.

> +{
> +	struct i2c_client *client = state->client;
> +	char buf[SCD4X_READ_BUF_SIZE];
> +	char *rsp = response;
> +	int i, ret;
> +	char crc;
> +
> +	/*
> +	 * Measurement needs to be stopped before sending commands.
> +	 * Except for reading measurement and data ready command.
> +	 */
> +	if ((cmd != CMD_GET_DATA_READY) && (cmd != CMD_READ_MEAS)) {
> +		ret = scd4x_send_command(state, CMD_STOP_MEAS);
> +		if (ret)
> +			return ret;
> +
> +		/* execution time for stopping measurement */
> +		msleep_interruptible(500);
> +	}
> +
> +	put_unaligned_be16(cmd, buf);
> +	ret = scd4x_i2c_xfer(state, buf, 2, buf, byte_cnt);
> +	if (ret)
> +		return ret;
> +
> +	for (i = 0; i < byte_cnt; i += 3) {
> +		crc = crc8(scd4x_crc8_table, buf + i, 2, CRC8_INIT_VALUE);
> +		if (crc != buf[i + 2]) {
> +			dev_err(&client->dev, "CRC error\n");
> +			return -EIO;
> +		}
> +
> +		*rsp++ = buf[i];
> +		*rsp++ = buf[i + 1];
> +	}
> +
> +	/* start measurement */
> +	if ((cmd != CMD_GET_DATA_READY) && (cmd != CMD_READ_MEAS)) {
> +		ret = scd4x_send_command(state, CMD_START_MEAS);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int scd4x_write(struct scd4x_state *state, enum scd4x_cmd cmd, uint16_t arg)
> +{
> +	char buf[SCD4X_WRITE_BUF_SIZE];
> +	int ret;
> +	char crc;
> +
> +	put_unaligned_be16(cmd, buf);
> +	put_unaligned_be16(arg, buf + 2);
> +
> +	crc = crc8(scd4x_crc8_table, buf + 2, 2, CRC8_INIT_VALUE);
> +	buf[4] = crc;
> +
> +	/* measurement needs to be stopped before sending commands */
> +	ret = scd4x_send_command(state, CMD_STOP_MEAS);
> +	if (ret)
> +		return ret;
> +
> +	/* execution time */
> +	msleep_interruptible(500);
> +
> +	ret = scd4x_i2c_xfer(state, buf, SCD4X_WRITE_BUF_SIZE, buf, 0);
> +	if (ret)
> +		return ret;
> +
> +	/* start measurement, except for forced calibration command */
> +	if (cmd != CMD_FRC) {
> +		ret = scd4x_send_command(state, CMD_START_MEAS);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int scd4x_write_and_fetch(struct scd4x_state *state, enum scd4x_cmd cmd,
> +				uint16_t arg, void *response, int byte_cnt)
> +{
> +	struct i2c_client *client = state->client;
> +	char buf[SCD4X_READ_BUF_SIZE];
> +	char *rsp = response;
> +	int i, ret;
> +	char crc;
> +
> +	ret = scd4x_write(state, CMD_FRC, arg);
> +	if (ret)
> +		goto err;
> +
> +	/* Execution time */
> +	msleep_interruptible(400);
> +
> +	ret = i2c_master_recv(client, buf, byte_cnt);
> +	if (ret < 0)
> +		goto err;
> +	if (ret != byte_cnt) {
> +		ret = -EIO;
> +		goto err;
> +	}
> +
> +	for (i = 0; i < byte_cnt; i += 3) {

Given the relationship between the size of the response buffer and the
actual bytes sent is simply (I think) 3/2* sizeof(response),
I would pass instead int response_sz so that at the callers you can use
a sizeof() call to get that value correct rather that just having it
as a fairly random seeming parameter.

> +		crc = crc8(scd4x_crc8_table, buf + i, 2, CRC8_INIT_VALUE);
> +		if (crc != buf[i + 2]) {
> +			dev_err(&client->dev, "CRC error\n");
> +			ret = -EIO;
> +			goto err;
> +		}
> +
> +		*rsp++ = buf[i];
> +		*rsp++ = buf[i + 1];
> +	}
> +
> +	return scd4x_send_command(state, CMD_START_MEAS);
> +
> +err:
> +	/*
> +	 * on error try to start the measurement,
> +	 * puts sensor back into continuous measurement
> +	 */
> +	scd4x_send_command(state, CMD_START_MEAS);
> +
> +	return ret;
> +}
> +
> +static int scd4x_read_meas(struct scd4x_state *state, uint16_t *meas)
> +{
> +	int i, ret;
> +	uint16_t buf[3];
> +
> +	ret = scd4x_read(state, CMD_READ_MEAS, buf, 9);

See above, this is sufficiently odd that I'd try to hide
the existence of the CRC bytes from anything outside of the
scd4x_read() function.

> +	if (ret)
> +		return ret;
> +
> +	for (i = 0; i < ARRAY_SIZE(buf); i++)
> +		meas[i] = be16_to_cpu(buf[i]);
> +
> +	return 0;
> +}
> +
> +static int scd4x_wait_meas_poll(struct scd4x_state *state)
> +{
> +	struct i2c_client *client = state->client;
> +	int tries = 6;
> +	int ret;
> +
> +	do {
> +		uint16_t val;
> +
> +		ret = scd4x_read(state, CMD_GET_DATA_READY, &val, 3);
> +		if (ret)
> +			return -EIO;
> +		val = be16_to_cpu(val);
> +
> +		/* new measurement available */
> +		if (val & 0x7FF)
> +			return 0;
> +
> +		msleep_interruptible(1000);
> +	} while (--tries);
> +
> +	/* try to start sensor on timeout */
> +	ret = scd4x_send_command(state, CMD_START_MEAS);
> +	if (ret)
> +		dev_err(&client->dev, "failed to start measurement: %d\n", ret);
> +
> +	return -ETIMEDOUT;
> +}
> +
> +static int scd4x_read_poll(struct scd4x_state *state, uint16_t *buf)
> +{
> +	int ret;
> +
> +	ret = scd4x_wait_meas_poll(state);
> +	if (ret)
> +		return ret;
> +
> +	return scd4x_read_meas(state, buf);
> +}
> +
> +static int scd4x_read_channel(struct scd4x_state *state, int chan)
> +{
> +	int ret;
> +	uint16_t buf[3];
> +
> +	ret = scd4x_read_poll(state, buf);
> +	if (ret)
> +		return ret;
> +
> +	return buf[chan];
> +}

Blank line here.  Please check for similar places.  These only make a small
difference to readability, but it's worth having.

> +static int scd4x_read_raw(struct iio_dev *indio_dev,
> +			struct iio_chan_spec const *chan, int *val,
> +			int *val2, long mask)
> +{
> +	struct scd4x_state *state = iio_priv(indio_dev);
> +	int ret;
> +	uint16_t tmp;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		ret = iio_device_claim_direct_mode(indio_dev);
> +		if (ret)
> +			return ret;
> +
> +		mutex_lock(&state->lock);
> +		ret = scd4x_read_channel(state, chan->address);
> +		mutex_unlock(&state->lock);
> +
> +		iio_device_release_direct_mode(indio_dev);
> +		if (ret < 0)
> +			return ret;
> +
> +		*val = ret;
> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_SCALE:
> +		if (chan->type == IIO_TEMP) {
> +			*val = 175000;
> +			*val2 = 65536;
> +			return IIO_VAL_FRACTIONAL;
> +		} else if (chan->type == IIO_HUMIDITYRELATIVE) {
> +			*val = 100000;
> +			*val2 = 65536;
> +			return IIO_VAL_FRACTIONAL;

You could use IIO_VAL_FACTIONAL_LOG2;  However, as it's expressed this
way in the datasheet, I don't mind if you leave it as you have it now.

> +		}
> +		return -EINVAL;
> +	case IIO_CHAN_INFO_OFFSET:
> +		*val = -16852;
> +		*val2 = 114286;
> +		return IIO_VAL_INT_PLUS_MICRO;
> +	case IIO_CHAN_INFO_CALIBBIAS:
> +		mutex_lock(&state->lock);
> +		ret = scd4x_read(state, CMD_GET_TEMP_OFFSET, &tmp, 3);
> +		mutex_unlock(&state->lock);
> +		if (ret)
> +			return ret;
> +
> +		*val = 175000 * be16_to_cpu(tmp) / 65536;

Calibbias often applies to some random tuning DAC deep inside a device and that
sometimes has a value that isn't related the actual sensor reading any any sensible
fashion.  As such, we don't put any rules on what it's 'units' are.

Here, (I think) it's a digital offset applied to the raw ADC reading.  As such,
I would probably have chosen to keep it in those units (ADC) counts rather than
converting to milli degrees C. 

Do you have a strong reason to make the conversion as you have done?
If so, it might be worth adding a special case to the docs for this in sysfs-bus-iio
(there is one there already for the icm42600 which is similar to what you have
here in that the units expressed are real world ones).

> +
> +		return IIO_VAL_INT;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int scd4x_write_raw(struct iio_dev *indio_dev, struct iio_chan_spec const *chan,
> +				int val, int val2, long mask)
> +{
> +	struct scd4x_state *state = iio_priv(indio_dev);
> +	int ret = 0;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_CALIBBIAS:
> +		val = val * 65536 / 175000;
> +
> +		mutex_lock(&state->lock);
> +		ret = scd4x_write(state, CMD_SET_TEMP_OFFSET, val);
> +		mutex_unlock(&state->lock);
> +
> +		return ret;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static ssize_t calibration_auto_enable_show(struct device *dev,
> +			struct device_attribute *attr, char *buf)
> +{
> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +	struct scd4x_state *state = iio_priv(indio_dev);
> +	int ret;
> +	uint16_t val;
> +
> +	mutex_lock(&state->lock);
> +	ret = scd4x_read(state, CMD_GET_ASC, &val, 3);
> +	mutex_unlock(&state->lock);
> +	if (ret) {
> +		dev_err(dev, "failed to read automatic calibration");
> +		return ret;
> +	}
> +
> +	val = be16_to_cpu(val) & SCD4X_READY_MASK;
Whilst is the same thing because SCD4X_READ_MASK == 0x1, this would become
more obviously correct if you forced the value to 0,1 here.

	val = (be16_to_cpu(val) & SCD4X_READY_MASK) ? 1 : 0;
> +
> +	return sprintf(buf, "%d\n", val);
> +}
> +
> +static ssize_t calibration_auto_enable_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 scd4x_state *state = iio_priv(indio_dev);
> +	bool val;
> +	int ret;
> +	uint16_t value;
> +
> +	ret = kstrtobool(buf, &val);
> +	if (ret)
> +		return ret;
> +
> +	value = val;
> +
> +	mutex_lock(&state->lock);
> +	ret = scd4x_write(state, CMD_SET_ASC, value);
> +	mutex_unlock(&state->lock);
> +	if (ret)
> +		dev_err(dev, "failed to set automatic calibration");
> +
> +	return ret ?: len;
> +}
> +
> +static ssize_t calibration_forced_value_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 scd4x_state *state = iio_priv(indio_dev);
> +	uint16_t val, arg;
> +	int ret;
> +
> +	ret = kstrtou16(buf, 0, &arg);
> +	if (ret)
> +		return ret;
> +
> +	if (arg < SCD4X_FRC_MIN_PPM || arg > SCD4X_FRC_MAX_PPM)
> +		return -EINVAL;

Ideally add an _available attribute to provide the range as

[FRC_MIN_PPM 1 SCD4X_FRC_MAX_PPM]
the _available attribute will also need documentation.

> +
> +	mutex_lock(&state->lock);
> +	ret = scd4x_write_and_fetch(state, CMD_FRC, arg, &val, 3);
> +	mutex_unlock(&state->lock);
> +
> +	if (val == 0xff) {
> +		dev_err(dev, "forced calibration has failed");
> +		return -EINVAL;
> +	}
> +
> +	return ret ?: len;
> +}
> +
> +static IIO_DEVICE_ATTR_RW(calibration_auto_enable, 0);
> +static IIO_DEVICE_ATTR_WO(calibration_forced_value, 0);
> +
> +static struct attribute *scd4x_attrs[] = {
> +	&iio_dev_attr_calibration_auto_enable.dev_attr.attr,
> +	&iio_dev_attr_calibration_forced_value.dev_attr.attr,
> +	NULL
> +};
> +
> +static const struct attribute_group scd4x_attr_group = {
> +	.attrs = scd4x_attrs,
> +};

Looks a bit odd without a blank line here.

> +static const struct iio_info scd4x_info = {
> +	.attrs = &scd4x_attr_group,
> +	.read_raw = scd4x_read_raw,
> +	.write_raw = scd4x_write_raw,
> +};
> +
> +static const struct iio_chan_spec scd4x_channels[] = {
> +	{
> +		.type = IIO_CONCENTRATION,
> +		.channel2 = IIO_MOD_CO2,
> +		.modified = 1,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> +		.address = SCD4X_CO2,
> +		.scan_index = SCD4X_CO2,
> +		.scan_type = {
> +			.sign = 'u',
> +			.realbits = 16,
> +			.storagebits = 16,
> +			.endianness = IIO_BE,
> +		},
> +	},
> +	{
> +		.type = IIO_TEMP,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> +					BIT(IIO_CHAN_INFO_SCALE) |
> +					BIT(IIO_CHAN_INFO_OFFSET) |
> +					BIT(IIO_CHAN_INFO_CALIBBIAS),
> +		.address = SCD4X_TEMP,
> +		.scan_index = SCD4X_TEMP,
> +		.scan_type = {
> +			.sign = 'u',
> +			.realbits = 16,
> +			.storagebits = 16,
> +			.endianness = IIO_BE,
> +		},
> +	},
> +	{
> +		.type = IIO_HUMIDITYRELATIVE,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> +					BIT(IIO_CHAN_INFO_SCALE),
> +		.address = SCD4X_HR,
> +		.scan_index = SCD4X_HR,
> +		.scan_type = {
> +			.sign = 'u',
> +			.realbits = 16,
> +			.storagebits = 16,
> +			.endianness = IIO_BE,
> +		},
> +	},
> +};

...

> +
> +static int scd4x_probe(struct i2c_client *client, const struct i2c_device_id *id)
> +{
> +	static const unsigned long scd4x_scan_masks[] = { 0x07, 0x00 };
> +	struct device *dev = &client->dev;
> +	struct iio_dev *indio_dev;
> +	struct scd4x_state *state;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*state));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	state = iio_priv(indio_dev);
> +	mutex_init(&state->lock);
> +	state->client = client;
> +	crc8_populate_msb(scd4x_crc8_table, SCD4X_CRC8_POLYNOMIAL);
> +
> +	indio_dev->info = &scd4x_info;
> +	indio_dev->name = client->name;
> +	indio_dev->channels = scd4x_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(scd4x_channels);
> +	indio_dev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_TRIGGERED;

Not need to include BUFFER_TRIGGERED.  It is done inside
devm_iio_triggered_buffer_setup() as calling that function always means it should
be set.
 
> +	indio_dev->available_scan_masks = scd4x_scan_masks;
> +
> +	state->vdd = devm_regulator_get(dev, "vdd");
> +	if (IS_ERR(state->vdd))
> +		return dev_err_probe(dev, PTR_ERR(state->vdd), "failed to get regulator\n");
> +
> +	ret = regulator_enable(state->vdd);
> +	if (ret)
> +		return ret;
> +
> +	ret = devm_add_action_or_reset(dev, scd4x_disable_regulator, state);
> +	if (ret)
> +		return ret;
> +
> +	ret = scd4x_send_command(state, CMD_STOP_MEAS);
> +	if (ret) {
> +		dev_err(dev, "failed to stop measurement: %d\n", ret);
> +		return ret;
> +	}
> +
> +	/* execution time */
> +	msleep_interruptible(500);
> +
> +	ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL, scd4x_trigger_handler, NULL);
> +	if (ret)
> +		return ret;
> +
> +	ret = scd4x_send_command(state, CMD_START_MEAS);
> +	if (ret) {
> +		dev_err(dev, "failed to start measurement: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = devm_add_action_or_reset(dev, scd4x_stop_meas, state);
> +	if (ret)
> +		return ret;
> +
> +	return devm_iio_device_register(dev, indio_dev);
> +}
> +
> +static const struct of_device_id scd4x_dt_ids[] = {
> +	{ .compatible = "sensirion,scd40" },
> +	{ .compatible = "sensirion,scd41" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, scd4x_dt_ids);
> +
> +static struct i2c_driver scd4x_i2c_driver = {
> +	.driver = {
> +		.name = KBUILD_MODNAME,
> +		.of_match_table = scd4x_dt_ids,
> +		.pm = &scd4x_pm_ops
> +	},
> +	.probe = scd4x_probe,
> +};
> +module_i2c_driver(scd4x_i2c_driver);
> +
> +MODULE_AUTHOR("Roan van Dijk <roan@protonic.nl>");
> +MODULE_DESCRIPTION("Sensirion SCD4X carbon dioxide sensor core driver");
> +MODULE_LICENSE("GPL v2");


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v2 4/4] iio: documentation: Document scd4x calibration use
  2021-09-13  8:00 ` [PATCH v2 4/4] iio: documentation: Document scd4x calibration use Roan van Dijk
@ 2021-09-19 16:51   ` Jonathan Cameron
  0 siblings, 0 replies; 7+ messages in thread
From: Jonathan Cameron @ 2021-09-19 16:51 UTC (permalink / raw)
  To: Roan van Dijk
  Cc: Rob Herring, Tomasz Duszynski, linux-iio, devicetree,
	linux-kernel, david, Lars-Peter Clausen

On Mon, 13 Sep 2021 10:00:20 +0200
Roan van Dijk <roan@protonic.nl> wrote:

> Add entries from Documentation/ABI/testing/sysfs-bus-iio-scd30
> to Documentation/ABI/testing/sysfs-bus-iio. The attributes of the scd4x
> and scd30 are common.
> 
> Remove Documentation/ABI/testing/sysfs-bus-iio-scd30.
> 
> Signed-off-by: Roan van Dijk <roan@protonic.nl>
Hi Roan,

Great to move these, but there is a bit too much 'specific' detail in here
reflecting the scd30.  We either need to make the descriptions more generic
or state some parts are only true for that device.

Some suggestions inline.  Make sure they also apply to your new driver.

Thanks,

Jonathan

> ---
>  Documentation/ABI/testing/sysfs-bus-iio       | 35 +++++++++++++++++++
>  Documentation/ABI/testing/sysfs-bus-iio-scd30 | 34 ------------------
>  2 files changed, 35 insertions(+), 34 deletions(-)
>  delete mode 100644 Documentation/ABI/testing/sysfs-bus-iio-scd30
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
> index 6ad47a67521c..56492c564f72 100644
> --- a/Documentation/ABI/testing/sysfs-bus-iio
> +++ b/Documentation/ABI/testing/sysfs-bus-iio
> @@ -1957,3 +1957,38 @@ Description:
>  		Specify the percent for light sensor relative to the channel
>  		absolute value that a data field should change before an event
>  		is generated. Units are a percentage of the prior reading.
> +
> +What:		/sys/bus/iio/devices/iio:deviceX/calibration_auto_enable
> +Date:		June 2020
> +KernelVersion:	5.8
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Contaminants build-up in the measurement chamber or optical
> +		elements deterioration leads to sensor drift.

Given this is a top level ABI doc, we don't know it is sensor with a measurement
chamber or that it's optical.  This might be an ADC.

We could make this an example.

		Some sensors have the ability to apply auto calibration at
		runtime.  For example, it may be necessary to compensate for
		contaminant build-up in a measurement chamber or optical
		element deterioration that would otherwise lead to sensor drift.

		Writing 1 or 0 to this attribute will respectively activate or
		deactivate this auto calibration function.

		Upon reading, the current status is returned.
> +
> +		One can compensate for sensor drift by using automatic self
> +		calibration procedure (asc).
> +
> +		Writing 1 or 0 to this attribute will respectively activate or
> +		deactivate asc.
> +
> +		Upon reading current asc status is returned.
> +
> +What:		/sys/bus/iio/devices/iio:deviceX/calibration_forced_value
> +Date:		June 2020
> +KernelVersion:	5.8
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Contaminants build-up in the measurement chamber or optical
> +		elements deterioration leads to sensor drift.
> +
> +		One can compensate for sensor drift by using forced
> +		recalibration (frc). This is useful in case there's known
> +		co2 reference available nearby the sensor.

Remove the c02 specific references from here and have something like

		Some sensors have the ability to apply a manual calibration using
		a known measurement value, perhaps obtained from an external
		reference device. 

		Writing a value to this function will force such a calibration
		change. For the scd30 the value should be from the range
		[400 1 2000].

		Note that a valid value may only be obtained once it is has been
		written.  Until then any read back of this value should be ignored.

Hmm. That last bit is rather ugly.  I'd rather we returned an error code if the
value had not yet been forced.  We should clean that up at some stage and also
add an appropriate _available so we don't need to document the range in this file.

		
> +
> +		Picking value from the range [400 1 2000] and writing it to the
> +		sensor will set frc.
> +
> +		Upon reading current frc value is returned. Note that after
> +		power cycling default value (i.e 400) is returned even though
> +		internally sensor had recalibrated itself.
> diff --git a/Documentation/ABI/testing/sysfs-bus-iio-scd30 b/Documentation/ABI/testing/sysfs-bus-iio-scd30
> deleted file mode 100644
> index b9712f390bec..000000000000
> --- a/Documentation/ABI/testing/sysfs-bus-iio-scd30
> +++ /dev/null
> @@ -1,34 +0,0 @@
> -What:		/sys/bus/iio/devices/iio:deviceX/calibration_auto_enable
> -Date:		June 2020
> -KernelVersion:	5.8
> -Contact:	linux-iio@vger.kernel.org
> -Description:
> -		Contaminants build-up in the measurement chamber or optical
> -		elements deterioration leads to sensor drift.
> -
> -		One can compensate for sensor drift by using automatic self
> -		calibration procedure (asc).
> -
> -		Writing 1 or 0 to this attribute will respectively activate or
> -		deactivate asc.
> -
> -		Upon reading current asc status is returned.
> -
> -What:		/sys/bus/iio/devices/iio:deviceX/calibration_forced_value
> -Date:		June 2020
> -KernelVersion:	5.8
> -Contact:	linux-iio@vger.kernel.org
> -Description:
> -		Contaminants build-up in the measurement chamber or optical
> -		elements deterioration leads to sensor drift.
> -
> -		One can compensate for sensor drift by using forced
> -		recalibration (frc). This is useful in case there's known
> -		co2 reference available nearby the sensor.
> -
> -		Picking value from the range [400 1 2000] and writing it to the
> -		sensor will set frc.
> -
> -		Upon reading current frc value is returned. Note that after
> -		power cycling default value (i.e 400) is returned even though
> -		internally sensor had recalibrated itself.


^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2021-09-19 16:47 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-13  8:00 [PATCH v2 0/4] iio: chemical: Add support for Sensirion SCD4x CO2 sensor Roan van Dijk
2021-09-13  8:00 ` [PATCH v2 1/4] dt-bindings: iio: chemical: sensirion,scd4x: Add yaml description Roan van Dijk
2021-09-13  8:00 ` [PATCH v2 2/4] MAINTAINERS: Add myself as maintainer of the scd4x driver Roan van Dijk
2021-09-13  8:00 ` [PATCH v2 3/4] drivers: iio: chemical: Add support for Sensirion SCD4x CO2 sensor Roan van Dijk
2021-09-19 16:41   ` Jonathan Cameron
2021-09-13  8:00 ` [PATCH v2 4/4] iio: documentation: Document scd4x calibration use Roan van Dijk
2021-09-19 16:51   ` Jonathan Cameron

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).