linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 0/4] iio: chemical: Add support for Sensirion SCD4x CO2 sensor
@ 2021-09-22 10:39 Roan van Dijk
  2021-09-22 10:39 ` [PATCH v3 1/4] dt-bindings: iio: chemical: sensirion,scd4x: Add yaml description Roan van Dijk
                   ` (3 more replies)
  0 siblings, 4 replies; 8+ messages in thread
From: Roan van Dijk @ 2021-09-22 10:39 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 v3:
scd4x.c
  - Change read and write_and_fetch function parameter. CRC byte is now
    hidden inside the function.
  - Fix minor style issues
  - Add calibration_forced_value_available attribute to the driver
  - Remove including BUFFER_TRIGGERED
  - Change calibbias to raw ADC readings rather than converting it to
    milli degrees C.
Documentation:
  - Change description of driver attributes
  - Add calibration_forced_value_available documentation

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       |  41 ++
 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                  | 694 ++++++++++++++++++
 7 files changed, 801 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] 8+ messages in thread

* [PATCH v3 1/4] dt-bindings: iio: chemical: sensirion,scd4x: Add yaml description
  2021-09-22 10:39 [PATCH v3 0/4] iio: chemical: Add support for Sensirion SCD4x CO2 sensor Roan van Dijk
@ 2021-09-22 10:39 ` Roan van Dijk
  2021-09-22 10:39 ` [PATCH v3 2/4] MAINTAINERS: Add myself as maintainer of the scd4x driver Roan van Dijk
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 8+ messages in thread
From: Roan van Dijk @ 2021-09-22 10:39 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] 8+ messages in thread

* [PATCH v3 2/4] MAINTAINERS: Add myself as maintainer of the scd4x driver
  2021-09-22 10:39 [PATCH v3 0/4] iio: chemical: Add support for Sensirion SCD4x CO2 sensor Roan van Dijk
  2021-09-22 10:39 ` [PATCH v3 1/4] dt-bindings: iio: chemical: sensirion,scd4x: Add yaml description Roan van Dijk
@ 2021-09-22 10:39 ` Roan van Dijk
  2021-09-22 10:39 ` [PATCH v3 3/4] drivers: iio: chemical: Add support for Sensirion SCD4x CO2 sensor Roan van Dijk
  2021-09-22 10:39 ` [PATCH v3 4/4] iio: documentation: Document scd4x calibration use Roan van Dijk
  3 siblings, 0 replies; 8+ messages in thread
From: Roan van Dijk @ 2021-09-22 10:39 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 ca6d6fde85cf..8ace6bd1280b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -16881,6 +16881,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] 8+ messages in thread

* [PATCH v3 3/4] drivers: iio: chemical: Add support for Sensirion SCD4x CO2 sensor
  2021-09-22 10:39 [PATCH v3 0/4] iio: chemical: Add support for Sensirion SCD4x CO2 sensor Roan van Dijk
  2021-09-22 10:39 ` [PATCH v3 1/4] dt-bindings: iio: chemical: sensirion,scd4x: Add yaml description Roan van Dijk
  2021-09-22 10:39 ` [PATCH v3 2/4] MAINTAINERS: Add myself as maintainer of the scd4x driver Roan van Dijk
@ 2021-09-22 10:39 ` Roan van Dijk
  2021-09-25 16:23   ` Jonathan Cameron
  2021-09-22 10:39 ` [PATCH v3 4/4] iio: documentation: Document scd4x calibration use Roan van Dijk
  3 siblings, 1 reply; 8+ messages in thread
From: Roan van Dijk @ 2021-09-22 10:39 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.

Signed-off-by: Roan van Dijk <roan@protonic.nl>
---
 drivers/iio/chemical/Kconfig  |  13 +
 drivers/iio/chemical/Makefile |   1 +
 drivers/iio/chemical/scd4x.c  | 694 ++++++++++++++++++++++++++++++++++
 3 files changed, 708 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..1ecc770249be
--- /dev/null
+++ b/drivers/iio/chemical/scd4x.c
@@ -0,0 +1,694 @@
+// 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 0
+#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 response_sz)
+{
+	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);
+	}
+
+	/*CRC byte for every 2 bytes of data */
+	response_sz += response_sz / 2;
+
+	put_unaligned_be16(cmd, buf);
+	ret = scd4x_i2c_xfer(state, buf, 2, buf, response_sz);
+	if (ret)
+		return ret;
+
+	for (i = 0; i < response_sz; 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 response_sz)
+{
+	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);
+
+	/*CRC byte for every 2 bytes of data */
+	response_sz += response_sz / 2;
+
+	ret = i2c_master_recv(client, buf, response_sz);
+	if (ret < 0)
+		goto err;
+	if (ret != response_sz) {
+		ret = -EIO;
+		goto err;
+	}
+
+	for (i = 0; i < response_sz; 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, sizeof(buf));
+	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, sizeof(val));
+		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, sizeof(tmp));
+		mutex_unlock(&state->lock);
+		if (ret)
+			return ret;
+
+		*val = be16_to_cpu(tmp);
+
+		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;
+
+		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, sizeof(val));
+	mutex_unlock(&state->lock);
+	if (ret) {
+		dev_err(dev, "failed to read automatic calibration");
+		return ret;
+	}
+
+	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;
+
+	mutex_lock(&state->lock);
+	ret = scd4x_write_and_fetch(state, CMD_FRC, arg, &val, sizeof(val));
+	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 IIO_CONST_ATTR(calibration_forced_value_available,
+	       __stringify(SCD4X_FRC_MIN_PPM 1 SCD4X_FRC_MAX_PPM));
+
+static struct attribute *scd4x_attrs[] = {
+	&iio_dev_attr_calibration_auto_enable.dev_attr.attr,
+	&iio_dev_attr_calibration_forced_value.dev_attr.attr,
+	&iio_const_attr_calibration_forced_value_available.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_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] 8+ messages in thread

* [PATCH v3 4/4] iio: documentation: Document scd4x calibration use
  2021-09-22 10:39 [PATCH v3 0/4] iio: chemical: Add support for Sensirion SCD4x CO2 sensor Roan van Dijk
                   ` (2 preceding siblings ...)
  2021-09-22 10:39 ` [PATCH v3 3/4] drivers: iio: chemical: Add support for Sensirion SCD4x CO2 sensor Roan van Dijk
@ 2021-09-22 10:39 ` Roan van Dijk
  3 siblings, 0 replies; 8+ messages in thread
From: Roan van Dijk @ 2021-09-22 10:39 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       | 41 +++++++++++++++++++
 Documentation/ABI/testing/sysfs-bus-iio-scd30 | 34 ---------------
 2 files changed, 41 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..07522b96d980 100644
--- a/Documentation/ABI/testing/sysfs-bus-iio
+++ b/Documentation/ABI/testing/sysfs-bus-iio
@@ -1957,3 +1957,44 @@ 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:
+		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.
+
+What:		/sys/bus/iio/devices/iio:deviceX/calibration_forced_value
+Date:		June 2020
+KernelVersion:	5.8
+Contact:	linux-iio@vger.kernel.org
+Description:
+		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 for the scd30 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. As for the scd4x an error will be returned
+		immediately if the manual calibration has failed.
+
+What:		/sys/bus/iio/devices/iio:deviceX/calibration_forced_value_available
+KernelVersion:  5.8
+Contact:        linux-iio@vger.kernel.org
+Description:
+		Available range for the forced calibration value, expressed as:
+
+		- a range specified as "[min step max]"
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] 8+ messages in thread

* Re: [PATCH v3 3/4] drivers: iio: chemical: Add support for Sensirion SCD4x CO2 sensor
  2021-09-22 10:39 ` [PATCH v3 3/4] drivers: iio: chemical: Add support for Sensirion SCD4x CO2 sensor Roan van Dijk
@ 2021-09-25 16:23   ` Jonathan Cameron
  2021-09-27 12:28     ` Roan van Dijk
  0 siblings, 1 reply; 8+ messages in thread
From: Jonathan Cameron @ 2021-09-25 16:23 UTC (permalink / raw)
  To: Roan van Dijk
  Cc: Rob Herring, Tomasz Duszynski, linux-iio, devicetree,
	linux-kernel, david, Lars-Peter Clausen

On Wed, 22 Sep 2021 12:39:24 +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.
> 
> Signed-off-by: Roan van Dijk <roan@protonic.nl>

Hi Roan,

Only thing in here of significance is that the format for available attribute
is wrong + it needs adding to the ABI docs.

Given we are going to have a v4, I noted a few other minor things to tidy up.

Thanks,

Jonathan


> +static int scd4x_read(struct scd4x_state *state, enum scd4x_cmd cmd,
> +			void *response, int response_sz)
> +{
> +	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);
> +	}
> +
> +	/*CRC byte for every 2 bytes of data */

/* CRC..

Please check for similar as otherwise we'll get 'cleanup' patches the moment various
scripts hit this new code and it'll waste our time!

> +	response_sz += response_sz / 2;
> +
> +	put_unaligned_be16(cmd, buf);
> +	ret = scd4x_i2c_xfer(state, buf, 2, buf, response_sz);
> +	if (ret)
> +		return ret;
> +
> +	for (i = 0; i < response_sz; 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 IIO_DEVICE_ATTR_RW(calibration_auto_enable, 0);
> +static IIO_DEVICE_ATTR_WO(calibration_forced_value, 0);
> +
> +static IIO_CONST_ATTR(calibration_forced_value_available,
> +	       __stringify(SCD4X_FRC_MIN_PPM 1 SCD4X_FRC_MAX_PPM));

Ah, I wasn't completely clear on this.  See the main ABI doc for
_available

Format for this needs to include brackets to indicate it's a range
rather than 3 numbers 
"[MIN 1  MAX]"


Having added this it also needs to be in the ABI documentation.
Whilst somewhat trivial, all ABI should be documented there.

> +
> +static struct attribute *scd4x_attrs[] = {
> +	&iio_dev_attr_calibration_auto_enable.dev_attr.attr,
> +	&iio_dev_attr_calibration_forced_value.dev_attr.attr,
> +	&iio_const_attr_calibration_forced_value_available.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 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));

I missed this before, but why not do the scd4x_read_poll() directly into scan->data after
you've done the memset?  That way you avoid the need for a memcpy.

i.e.

	memset(&scan, 0, sizeof(scan));
	mutex_lock(&state->lock)
	ret = scd4x_read_poll(state, scan->data);
	mutex_unlock(&state->lock);
	if (ret)
	...


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

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

* Re: [PATCH v3 3/4] drivers: iio: chemical: Add support for Sensirion SCD4x CO2 sensor
  2021-09-25 16:23   ` Jonathan Cameron
@ 2021-09-27 12:28     ` Roan van Dijk
  2021-09-30 15:59       ` Jonathan Cameron
  0 siblings, 1 reply; 8+ messages in thread
From: Roan van Dijk @ 2021-09-27 12:28 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: Rob Herring, Tomasz Duszynski, linux-iio, devicetree,
	linux-kernel, david, Lars-Peter Clausen



On 25-09-2021 18:23, Jonathan Cameron wrote:
> On Wed, 22 Sep 2021 12:39:24 +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.
>>
>> Signed-off-by: Roan van Dijk <roan@protonic.nl>
> 
> Hi Roan,
> 
> Only thing in here of significance is that the format for available attribute
> is wrong + it needs adding to the ABI docs.
> 
> Given we are going to have a v4, I noted a few other minor things to tidy up.
> 
> Thanks,
> 
> Jonathan
> 

Thank you for your feedback. I will make a new patch with these fixes.

The documentation for the _available and the fixes you suggested, have 
already been added in patch [PATCH v3 4/4]. Do I still need to add 
something to the ABI documentation or is it fine like this?

However, I will change the KernelVersion to 5.15 of the _available 
description in patch v4. I left this at 5.8 and that's not right, 
because this information is newly added.

Thanks,

Roan

> 
>> +static int scd4x_read(struct scd4x_state *state, enum scd4x_cmd cmd,
>> +			void *response, int response_sz)
>> +{
>> +	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);
>> +	}
>> +
>> +	/*CRC byte for every 2 bytes of data */
> 
> /* CRC..
> 
> Please check for similar as otherwise we'll get 'cleanup' patches the moment various
> scripts hit this new code and it'll waste our time!
> 
>> +	response_sz += response_sz / 2;
>> +
>> +	put_unaligned_be16(cmd, buf);
>> +	ret = scd4x_i2c_xfer(state, buf, 2, buf, response_sz);
>> +	if (ret)
>> +		return ret;
>> +
>> +	for (i = 0; i < response_sz; 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 IIO_DEVICE_ATTR_RW(calibration_auto_enable, 0);
>> +static IIO_DEVICE_ATTR_WO(calibration_forced_value, 0);
>> +
>> +static IIO_CONST_ATTR(calibration_forced_value_available,
>> +	       __stringify(SCD4X_FRC_MIN_PPM 1 SCD4X_FRC_MAX_PPM));
> 
> Ah, I wasn't completely clear on this.  See the main ABI doc for
> _available
> 
> Format for this needs to include brackets to indicate it's a range
> rather than 3 numbers
> "[MIN 1  MAX]"
> 
> 
> Having added this it also needs to be in the ABI documentation.
> Whilst somewhat trivial, all ABI should be documented there.
> 
>> +
>> +static struct attribute *scd4x_attrs[] = {
>> +	&iio_dev_attr_calibration_auto_enable.dev_attr.attr,
>> +	&iio_dev_attr_calibration_forced_value.dev_attr.attr,
>> +	&iio_const_attr_calibration_forced_value_available.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 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));
> 
> I missed this before, but why not do the scd4x_read_poll() directly into scan->data after
> you've done the memset?  That way you avoid the need for a memcpy.
> 
> i.e.
> 
> 	memset(&scan, 0, sizeof(scan));
> 	mutex_lock(&state->lock)
> 	ret = scd4x_read_poll(state, scan->data);
> 	mutex_unlock(&state->lock);
> 	if (ret)
> 	...
> 
> 
>> +
>> +	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;
>> +}
>> +

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

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

On Mon, 27 Sep 2021 14:28:24 +0200
Roan van Dijk <roan@protonic.nl> wrote:

> On 25-09-2021 18:23, Jonathan Cameron wrote:
> > On Wed, 22 Sep 2021 12:39:24 +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.
> >>
> >> Signed-off-by: Roan van Dijk <roan@protonic.nl>  
> > 
> > Hi Roan,
> > 
> > Only thing in here of significance is that the format for available attribute
> > is wrong + it needs adding to the ABI docs.
> > 
> > Given we are going to have a v4, I noted a few other minor things to tidy up.
> > 
> > Thanks,
> > 
> > Jonathan
> >   
> 
> Thank you for your feedback. I will make a new patch with these fixes.
> 
> The documentation for the _available and the fixes you suggested, have 
> already been added in patch [PATCH v3 4/4]. Do I still need to add 
> something to the ABI documentation or is it fine like this?

Ah. I missed _available documentation for some reason. Sorry about that
it's fine as you have it.

> 
> However, I will change the KernelVersion to 5.15 of the _available 
> description in patch v4. I left this at 5.8 and that's not right, 
> because this information is newly added.
Good spot.

J

> 
> Thanks,
> 
> Roan
> 
> >   
> >> +static int scd4x_read(struct scd4x_state *state, enum scd4x_cmd cmd,
> >> +			void *response, int response_sz)
> >> +{
> >> +	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);
> >> +	}
> >> +
> >> +	/*CRC byte for every 2 bytes of data */  
> > 
> > /* CRC..
> > 
> > Please check for similar as otherwise we'll get 'cleanup' patches the moment various
> > scripts hit this new code and it'll waste our time!
> >   
> >> +	response_sz += response_sz / 2;
> >> +
> >> +	put_unaligned_be16(cmd, buf);
> >> +	ret = scd4x_i2c_xfer(state, buf, 2, buf, response_sz);
> >> +	if (ret)
> >> +		return ret;
> >> +
> >> +	for (i = 0; i < response_sz; 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 IIO_DEVICE_ATTR_RW(calibration_auto_enable, 0);
> >> +static IIO_DEVICE_ATTR_WO(calibration_forced_value, 0);
> >> +
> >> +static IIO_CONST_ATTR(calibration_forced_value_available,
> >> +	       __stringify(SCD4X_FRC_MIN_PPM 1 SCD4X_FRC_MAX_PPM));  
> > 
> > Ah, I wasn't completely clear on this.  See the main ABI doc for
> > _available
> > 
> > Format for this needs to include brackets to indicate it's a range
> > rather than 3 numbers
> > "[MIN 1  MAX]"
> > 
> > 
> > Having added this it also needs to be in the ABI documentation.
> > Whilst somewhat trivial, all ABI should be documented there.
> >   
> >> +
> >> +static struct attribute *scd4x_attrs[] = {
> >> +	&iio_dev_attr_calibration_auto_enable.dev_attr.attr,
> >> +	&iio_dev_attr_calibration_forced_value.dev_attr.attr,
> >> +	&iio_const_attr_calibration_forced_value_available.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 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));  
> > 
> > I missed this before, but why not do the scd4x_read_poll() directly into scan->data after
> > you've done the memset?  That way you avoid the need for a memcpy.
> > 
> > i.e.
> > 
> > 	memset(&scan, 0, sizeof(scan));
> > 	mutex_lock(&state->lock)
> > 	ret = scd4x_read_poll(state, scan->data);
> > 	mutex_unlock(&state->lock);
> > 	if (ret)
> > 	...
> > 
> >   
> >> +
> >> +	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;
> >> +}
> >> +  


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

end of thread, other threads:[~2021-09-30 15:55 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-22 10:39 [PATCH v3 0/4] iio: chemical: Add support for Sensirion SCD4x CO2 sensor Roan van Dijk
2021-09-22 10:39 ` [PATCH v3 1/4] dt-bindings: iio: chemical: sensirion,scd4x: Add yaml description Roan van Dijk
2021-09-22 10:39 ` [PATCH v3 2/4] MAINTAINERS: Add myself as maintainer of the scd4x driver Roan van Dijk
2021-09-22 10:39 ` [PATCH v3 3/4] drivers: iio: chemical: Add support for Sensirion SCD4x CO2 sensor Roan van Dijk
2021-09-25 16:23   ` Jonathan Cameron
2021-09-27 12:28     ` Roan van Dijk
2021-09-30 15:59       ` Jonathan Cameron
2021-09-22 10:39 ` [PATCH v3 4/4] iio: documentation: Document scd4x calibration use Roan van Dijk

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