[PATCH v3 0/3] iio: chemical: Add Senseair Sunrise CO2 sensor

[PATCH v3 0/3] iio: chemical: Add Senseair Sunrise CO2 sensor

[Jacopo Mondi]

The driver supports continuous reads of temperature and CO2 concentration
through two dedicated IIO channels. It also supports calibration and error
inspection through the concentration channel ext_info.

Minor changes in v3

v2->v3:
- [1/3]
 - Fix syntax error reported by dt_binding_check
   The device node label in the example cannot contain '-'
 - Add 'Typically' to the gpios polarities description

- [2/3]
 - As suggested by Andy:
   - depends on OF, SYSFS; select REGMAP_I2C
   - Fix style issues:
     - span over 80 cols where appropriate
     - remove , in last entries of all arrays
     - use for_each_set_bit in sunrise_error_status_read()
     - minor style issues (brakets, empty lines, wording)

v1->v2:
- Add ABI documentation in [3/3]
- [1/3]
  - Address Rob's comments on missing maxItem and add device node label
  - Do not change the pin's polarity description as suggested by Andy due to
    conflicting suggestions
- [2/3]
  - Expand Kconfig symbol name and change driver's name as suggested by Andy
  - Use regmap instead of raw smbus calls as suggested by Andy
  - Take into account minor style comments from Andy
  - Install channel's ext_info to support calibration triggering and enumerate
    calibration modes and error status
  - Matt suggested to use sysfs attributes, but I found the per-channel
    attributes more appropriate. Hope this is good as well.

Thanks
   j

Jacopo Mondi (3):
  dt-bindings: iio: chemical: Document senseair,sunrise CO2 sensor
  iio: chemical: Add Sensteair Sunrise 006-0-007 driver
  iio: ABI: docs: Document Senseair Sunrise ABI

 .../sysfs-bus-iio-chemical-sunrise-co2        |  51 ++
 .../iio/chemical/senseair,sunrise.yaml        |  53 +++
 .../devicetree/bindings/vendor-prefixes.yaml  |   2 +
 MAINTAINERS                                   |   6 +
 drivers/iio/chemical/Kconfig                  |  13 +
 drivers/iio/chemical/Makefile                 |   1 +
 drivers/iio/chemical/sunrise_co2.c            | 450 ++++++++++++++++++
 7 files changed, 576 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-chemical-sunrise-co2
 create mode 100644 Documentation/devicetree/bindings/iio/chemical/senseair,sunrise.yaml
 create mode 100644 drivers/iio/chemical/sunrise_co2.c

--
2.32.0

[PATCH v3 1/3] dt-bindings: iio: chemical: Document senseair,sunrise CO2 sensor

[Jacopo Mondi]

Add documentation for the Senseair Sunrise 006-0-0007 CO2 NDIR sensor.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
v2->v3:
- Fix syntax error reported by dt_binding_check
  The device node label in the example cannot contain '-'
- Add 'Typically' to the gpios polarities description

v1->v2:
- Add maxItems to -gpios properties as suggested by Rob
- Add a label to the device node in the example as suggested by Rob
---
 .../iio/chemical/senseair,sunrise.yaml        | 55 +++++++++++++++++++
 .../devicetree/bindings/vendor-prefixes.yaml  |  2 +
 2 files changed, 57 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/chemical/senseair,sunrise.yaml

diff --git a/Documentation/devicetree/bindings/iio/chemical/senseair,sunrise.yaml b/Documentation/devicetree/bindings/iio/chemical/senseair,sunrise.yaml
new file mode 100644
index 000000000000..d05d06b5840e
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/chemical/senseair,sunrise.yaml
@@ -0,0 +1,55 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/chemical/senseair,sunrise.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Senseair Sunrise 006-0-0007 CO2 Sensor
+
+maintainers:
+  - Jacopo Mondi <jacopo@jmondi.org>
+
+description: |
+  Senseair Sunrise 006-0-0007 is a NDIR CO2 sensor. It supports I2C or UART buses
+  for communications and control.
+
+  Datasheets:
+    https://rmtplusstoragesenseair.blob.core.windows.net/docs/Dev/publicerat/PSP11704.pdf
+    https://rmtplusstoragesenseair.blob.core.windows.net/docs/Dev/publicerat/PSH11649.pdf
+    https://rmtplusstoragesenseair.blob.core.windows.net/docs/Dev/publicerat/TDE5531.pdf
+    https://rmtplusstoragesenseair.blob.core.windows.net/docs/Market/publicerat/TDE7318.pdf
+
+properties:
+  compatible:
+    const: senseair,sunrise-006-0-0007
+
+  reg:
+    maxItems: 1
+
+  ndry-gpios:
+    maxItems: 1
+    description:
+      Phandle to the GPIO line connected to the nDRY pin. Typically active low.
+
+  en-gpios:
+    maxItems: 1
+    description:
+      Phandle to the GPIO line connected to the EN pin. Typically active high.
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    i2c {
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      sunrise: co2-sensor@68 {
+        compatible = "senseair,sunrise-006-0-0007";
+        reg = <0x68>;
+      };
+    };
diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml
index 944a14926e02..c60502eb3d36 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
+++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
@@ -1000,6 +1000,8 @@ patternProperties:
     description: Shenzhen SEI Robotics Co., Ltd
   "^semtech,.*":
     description: Semtech Corporation
+  "^senseair,.*":
+    description: Senseair AB
   "^sensirion,.*":
     description: Sensirion AG
   "^sensortek,.*":
--
2.32.0

Re: [PATCH v3 1/3] dt-bindings: iio: chemical: Document senseair,sunrise CO2 sensor

[Rob Herring]

On Sun, 22 Aug 2021 20:49:25 +0200, Jacopo Mondi wrote:
> Add documentation for the Senseair Sunrise 006-0-0007 CO2 NDIR sensor.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
> v2->v3:
> - Fix syntax error reported by dt_binding_check
>   The device node label in the example cannot contain '-'
> - Add 'Typically' to the gpios polarities description
> 
> v1->v2:
> - Add maxItems to -gpios properties as suggested by Rob
> - Add a label to the device node in the example as suggested by Rob
> ---
>  .../iio/chemical/senseair,sunrise.yaml        | 55 +++++++++++++++++++
>  .../devicetree/bindings/vendor-prefixes.yaml  |  2 +
>  2 files changed, 57 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/chemical/senseair,sunrise.yaml
> 

Reviewed-by: Rob Herring <robh@kernel.org>

[PATCH v3 2/3] iio: chemical: Add Sensteair Sunrise 006-0-007 driver

[Jacopo Mondi]

Add support for the Senseair Sunrise 006-0-0007 driver through the
IIO subsystem.

Datasheet: https://rmtplusstoragesenseair.blob.core.windows.net/docs/Dev/publicerat/TDE5531.pdf
Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 MAINTAINERS                        |   6 +
 drivers/iio/chemical/Kconfig       |  13 +
 drivers/iio/chemical/Makefile      |   1 +
 drivers/iio/chemical/sunrise_co2.c | 450 +++++++++++++++++++++++++++++
 4 files changed, 470 insertions(+)
 create mode 100644 drivers/iio/chemical/sunrise_co2.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 90ca9df1d3c3..43f5bba46673 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -16544,6 +16544,12 @@ S:	Maintained
 F:	drivers/misc/phantom.c
 F:	include/uapi/linux/phantom.h
 
+SENSEAIR SUNRISE 006-0-0007
+M:	Jacopo Mondi <jacopo@jmondi.org>
+S:	Maintained
+F:	Documentation/devicetree/bindings/iio/chemical/senseair,sunrise.yaml
+F:	drivers/iio/chemical/sunrise_co2.c
+
 SENSIRION SCD30 CARBON DIOXIDE SENSOR DRIVER
 M:	Tomasz Duszynski <tomasz.duszynski@octakon.com>
 S:	Maintained
diff --git a/drivers/iio/chemical/Kconfig b/drivers/iio/chemical/Kconfig
index 10bb431bc3ce..ee8562949226 100644
--- a/drivers/iio/chemical/Kconfig
+++ b/drivers/iio/chemical/Kconfig
@@ -144,6 +144,19 @@ config SPS30
 	  To compile this driver as a module, choose M here: the module will
 	  be called sps30.
 
+config SENSEAIR_SUNRISE_CO2
+	tristate "Senseair Sunrise 006-0-0007 CO2 sensor"
+	depends on OF
+	depends on I2C
+	depends on SYSFS
+	select REGMAP_I2C
+	help
+	  Say yes here to build support for Senseair Sunrise 006-0-0007 CO2
+	  sensor.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called sunrise_co2.
+
 config VZ89X
 	tristate "SGX Sensortech MiCS VZ89X VOC sensor"
 	depends on I2C
diff --git a/drivers/iio/chemical/Makefile b/drivers/iio/chemical/Makefile
index fef63dd5bf92..d5e2a3331d57 100644
--- a/drivers/iio/chemical/Makefile
+++ b/drivers/iio/chemical/Makefile
@@ -17,4 +17,5 @@ obj-$(CONFIG_SCD30_I2C) += scd30_i2c.o
 obj-$(CONFIG_SCD30_SERIAL) += scd30_serial.o
 obj-$(CONFIG_SENSIRION_SGP30)	+= sgp30.o
 obj-$(CONFIG_SPS30) += sps30.o
+obj-$(CONFIG_SENSEAIR_SUNRISE_CO2) += sunrise_co2.o
 obj-$(CONFIG_VZ89X)		+= vz89x.o
diff --git a/drivers/iio/chemical/sunrise_co2.c b/drivers/iio/chemical/sunrise_co2.c
new file mode 100644
index 000000000000..1dcfe8e1a94a
--- /dev/null
+++ b/drivers/iio/chemical/sunrise_co2.c
@@ -0,0 +1,450 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Senseair Sunrise 006-0-0007 CO2 sensor driver.
+ *
+ * Copyright (C) 2021 Jacopo Mondi
+ *
+ * List of features not yet supported by the driver:
+ * - controllable EN pin
+ * - single-shot operations using the nDRY pin.
+ * - ABC/target calibration
+ */
+
+#include <linux/bitops.h>
+#include <linux/i2c.h>
+#include <linux/kernel.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/regmap.h>
+#include <linux/time64.h>
+
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+
+#define DRIVER_NAME "sunrise"
+
+#define SUNRISE_ERROR_STATUS_REG		0x00
+#define SUNRISE_CO2_FILTERED_COMP_REG		0x06
+#define SUNRISE_CHIP_TEMPERATURE_REG		0x08
+#define SUNRISE_CALIBRATION_STATUS_REG		0x81
+#define SUNRISE_CALIBRATION_COMMAND_REG		0x82
+#define SUNRISE_CALIBRATION_FACTORY_CMD		0x7c02
+#define SUNRISE_CALIBRATION_BACKGROUND_CMD	0x7c06
+/*
+ * The calibration timeout is not characterized in the datasheet.
+ * Use 30 seconds as a reasonable upper limit.
+ */
+#define SUNRISE_CALIBRATION_TIMEOUT_US		(30 * USEC_PER_SEC)
+
+enum sunrise_calib {
+	SUNRISE_CALIBRATION_FACTORY,
+	SUNRISE_CALIBRATION_BACKGROUND,
+};
+
+struct sunrise_dev {
+	struct device *dev;
+	struct i2c_client *client;
+	struct regmap *regmap;
+	struct mutex lock;
+	enum sunrise_calib calibration;
+};
+
+static void sunrise_wakeup(struct sunrise_dev *sunrise)
+{
+	struct i2c_client *client = sunrise->client;
+
+	/*
+	 * Wake up sensor by sending sensor address: START, sensor address,
+	 * STOP. Sensor will not ACK this byte.
+	 *
+	 * The chip returns in low power state after 15msec without
+	 * communications or after a complete read/write sequence.
+	 */
+	i2c_smbus_xfer(client->adapter, client->addr, I2C_M_IGNORE_NAK,
+		       I2C_SMBUS_WRITE, 0, I2C_SMBUS_QUICK, NULL);
+}
+
+static int sunrise_read_word(struct sunrise_dev *sunrise, u8 reg, u16 *val)
+{
+	__be16 be_val;
+	int ret;
+
+	sunrise_wakeup(sunrise);
+	ret = regmap_bulk_read(sunrise->regmap, reg, &be_val, 2);
+	if (ret) {
+		dev_err(sunrise->dev, "Read word failed: reg 0x%2x (%d)\n", reg, ret);
+		return ret;
+	}
+
+	*val = be16_to_cpu(be_val);
+
+	return 0;
+}
+
+static int sunrise_write_byte(struct sunrise_dev *sunrise, u8 reg, u8 val)
+{
+	int ret;
+
+	sunrise_wakeup(sunrise);
+	ret = regmap_write(sunrise->regmap, reg, val);
+	if (ret) {
+		dev_err(sunrise->dev, "Write byte failed: reg 0x%2x (%d)\n", reg, ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int sunrise_write_word(struct sunrise_dev *sunrise, u8 reg, u16 data)
+{
+	__be16 be_data = cpu_to_be16(data);
+	int ret;
+
+	sunrise_wakeup(sunrise);
+	ret = regmap_bulk_write(sunrise->regmap, reg, &be_data, 2);
+	if (ret) {
+		dev_err(sunrise->dev, "Write word failed: reg 0x%2x (%d)\n", reg, ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+/*
+ *  --- Calibration ---
+ *
+ *  Enumerate and select calibration modes, trigger a calibration cycle.
+ */
+static const char * const sunrise_calibration_modes[] = {
+	[SUNRISE_CALIBRATION_FACTORY] = "factory_calibration",
+	[SUNRISE_CALIBRATION_BACKGROUND] = "background_calibration"
+};
+
+static const struct sunrise_calibration_data {
+	u16 calibration_cmd;
+	u8 calibration_bit;
+} sunrise_calibrations[] = {
+	[SUNRISE_CALIBRATION_FACTORY] = {
+		SUNRISE_CALIBRATION_FACTORY_CMD,
+		BIT(2)
+	},
+	[SUNRISE_CALIBRATION_BACKGROUND] = {
+		SUNRISE_CALIBRATION_BACKGROUND_CMD,
+		BIT(5)
+	}
+};
+
+static int sunrise_calibrate(struct sunrise_dev *sunrise)
+{
+	const struct sunrise_calibration_data *data;
+	unsigned int status;
+	int ret;
+
+	/* Reset the calibration status reg. */
+	ret = sunrise_write_byte(sunrise, SUNRISE_CALIBRATION_STATUS_REG, 0x00);
+	if (ret)
+		return ret;
+
+	/* Write a calibration command and poll the calibration status bit. */
+	data = &sunrise_calibrations[sunrise->calibration];
+	ret = sunrise_write_word(sunrise, SUNRISE_CALIBRATION_COMMAND_REG,
+				 data->calibration_cmd);
+	if (ret)
+		return ret;
+
+	dev_dbg(sunrise->dev, "%s in progress\n",
+		sunrise_calibration_modes[sunrise->calibration]);
+
+	return regmap_read_poll_timeout(sunrise->regmap,
+					SUNRISE_CALIBRATION_STATUS_REG,
+					status, status & data->calibration_bit,
+					100, SUNRISE_CALIBRATION_TIMEOUT_US);
+}
+
+static ssize_t sunrise_calibration_write(struct iio_dev *iiodev,
+					 uintptr_t private,
+					 const struct iio_chan_spec *chan,
+					 const char *buf, size_t len)
+{
+	struct sunrise_dev *sunrise = iio_priv(iiodev);
+	bool calibrate;
+	int ret;
+
+	ret = kstrtobool(buf, &calibrate);
+	if (ret)
+		return ret;
+
+	if (!calibrate)
+		return 0;
+
+	ret = sunrise_calibrate(sunrise);
+	if (ret)
+		return ret;
+
+	return len;
+}
+
+static int sunrise_set_calibration_mode(struct iio_dev *iiodev,
+					const struct iio_chan_spec *chan,
+					unsigned int mode)
+{
+	struct sunrise_dev *sunrise = iio_priv(iiodev);
+
+	mutex_lock(&sunrise->lock);
+	sunrise->calibration = mode;
+	mutex_unlock(&sunrise->lock);
+
+	return 0;
+}
+
+static int sunrise_get_calibration_mode(struct iio_dev *iiodev,
+					const struct iio_chan_spec *chan)
+{
+	struct sunrise_dev *sunrise = iio_priv(iiodev);
+	int mode;
+
+	mutex_lock(&sunrise->lock);
+	mode = sunrise->calibration;
+	mutex_unlock(&sunrise->lock);
+
+	return mode;
+}
+
+static const struct iio_enum sunrise_calibration_modes_enum = {
+	.items = sunrise_calibration_modes,
+	.num_items = ARRAY_SIZE(sunrise_calibration_modes),
+	.set = sunrise_set_calibration_mode,
+	.get = sunrise_get_calibration_mode,
+};
+
+/* --- Error status---
+ *
+ * Enumerate and retrieve the chip error status.
+ */
+enum {
+	SUNRISE_ERROR_FATAL,
+	SUNRISE_ERROR_I2C,
+	SUNRISE_ERROR_ALGORITHM,
+	SUNRISE_ERROR_CALIBRATION,
+	SUNRISE_ERROR_SELF_DIAGNOSTIC,
+	SUNRISE_ERROR_OUT_OF_RANGE,
+	SUNRISE_ERROR_MEMORY,
+	SUNRISE_ERROR_NO_MEASUREMENT,
+	SUNRISE_ERROR_LOW_VOLTAGE,
+	SUNRISE_ERROR_MEASUREMENT_TIMEOUT
+};
+
+static const char * const sunrise_error_statuses[] = {
+	[SUNRISE_ERROR_FATAL] = "error_fatal",
+	[SUNRISE_ERROR_I2C] = "error_i2c",
+	[SUNRISE_ERROR_ALGORITHM] = "error_algorithm",
+	[SUNRISE_ERROR_CALIBRATION] = "error_calibration",
+	[SUNRISE_ERROR_SELF_DIAGNOSTIC] = "error_self_diagnostic",
+	[SUNRISE_ERROR_OUT_OF_RANGE] = "error_out_of_range",
+	[SUNRISE_ERROR_MEMORY] = "error_memory",
+	[SUNRISE_ERROR_NO_MEASUREMENT] = "error_no_measurement",
+	[SUNRISE_ERROR_LOW_VOLTAGE] = "error_low_voltage",
+	[SUNRISE_ERROR_MEASUREMENT_TIMEOUT] = "error_measurement_timeout"
+};
+
+static const u8 error_codes[] = {
+	SUNRISE_ERROR_FATAL,
+	SUNRISE_ERROR_I2C,
+	SUNRISE_ERROR_ALGORITHM,
+	SUNRISE_ERROR_CALIBRATION,
+	SUNRISE_ERROR_SELF_DIAGNOSTIC,
+	SUNRISE_ERROR_OUT_OF_RANGE,
+	SUNRISE_ERROR_MEMORY,
+	SUNRISE_ERROR_NO_MEASUREMENT,
+	SUNRISE_ERROR_LOW_VOLTAGE,
+	SUNRISE_ERROR_MEASUREMENT_TIMEOUT
+};
+
+static const struct iio_enum sunrise_error_statuses_enum = {
+	.items = sunrise_error_statuses,
+	.num_items = ARRAY_SIZE(sunrise_error_statuses)
+};
+
+static ssize_t sunrise_error_status_read(struct iio_dev *iiodev,
+					 uintptr_t private,
+					 const struct iio_chan_spec *chan,
+					 char *buf)
+{
+	struct sunrise_dev *sunrise = iio_priv(iiodev);
+	const unsigned long *errors;
+	ssize_t len = 0;
+	u16 value;
+	int ret;
+	u8 i;
+
+	ret = sunrise_read_word(sunrise, SUNRISE_ERROR_STATUS_REG, &value);
+	if (ret)
+		return -EINVAL;
+
+pr_err("%s:%d0x%x\n", __func__, __LINE__, value);
+
+	errors = (const unsigned long *)&value;
+	for_each_set_bit(i, errors, ARRAY_SIZE(error_codes))
+		len += sysfs_emit_at(buf, len, "%s ", sunrise_error_statuses[i]);
+
+	if (len)
+		buf[len - 1] = '\n';
+
+	return len;
+}
+
+static const struct iio_chan_spec_ext_info sunrise_concentration_ext_info[] = {
+	/* Calibration modes and calibration trigger. */
+	{
+		.name = "calibration",
+		.write = sunrise_calibration_write,
+		.shared = IIO_SEPARATE
+	},
+	IIO_ENUM("calibration_mode", IIO_SEPARATE,
+		 &sunrise_calibration_modes_enum),
+	IIO_ENUM_AVAILABLE("calibration_mode",
+			   &sunrise_calibration_modes_enum),
+
+	/* Error statuses. */
+	{
+		.name = "error_status",
+		.read = sunrise_error_status_read,
+		.shared = IIO_SEPARATE
+	},
+	IIO_ENUM_AVAILABLE("error_status", &sunrise_error_statuses_enum),
+	{}
+};
+
+static const struct iio_chan_spec sunrise_channels[] = {
+	{
+		.type = IIO_CONCENTRATION,
+		.modified = 1,
+		.channel2 = IIO_MOD_CO2,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+		.ext_info = sunrise_concentration_ext_info,
+		.scan_index = 0,
+		.scan_type =  {
+			.sign = 's',
+			.realbits = 16,
+			.storagebits = 16,
+			.endianness = IIO_CPU,
+		}
+	},
+	{
+		.type = IIO_TEMP,
+		.info_mask_separate =  BIT(IIO_CHAN_INFO_RAW) |
+				       BIT(IIO_CHAN_INFO_SCALE),
+		.scan_index = 1,
+		.scan_type =  {
+			.sign = 's',
+			.realbits = 16,
+			.storagebits = 16,
+			.endianness = IIO_CPU,
+		}
+	}
+};
+
+static int sunrise_read_raw(struct iio_dev *iio_dev,
+			    const struct iio_chan_spec *chan,
+			    int *val, int *val2, long mask)
+{
+	struct sunrise_dev *sunrise = iio_priv(iio_dev);
+	u16 value;
+	int ret;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		mutex_lock(&sunrise->lock);
+
+		switch (chan->type) {
+		case IIO_CONCENTRATION:
+			ret = sunrise_read_word(sunrise, SUNRISE_CO2_FILTERED_COMP_REG,
+						&value);
+			*val = value;
+			mutex_unlock(&sunrise->lock);
+
+			return ret ?: IIO_VAL_INT;
+
+		case IIO_TEMP:
+			ret = sunrise_read_word(sunrise, SUNRISE_CHIP_TEMPERATURE_REG,
+						&value);
+			*val = value;
+			mutex_unlock(&sunrise->lock);
+
+			return ret ?: IIO_VAL_INT;
+
+		default:
+			mutex_unlock(&sunrise->lock);
+			return -EINVAL;
+		}
+
+	case IIO_CHAN_INFO_SCALE:
+		/* Chip temperature scale = 1/100 */
+		*val = 1;
+		*val2 = 100;
+		return IIO_VAL_FRACTIONAL;
+
+	default:
+		return -EINVAL;
+	}
+}
+
+static const struct iio_info sunrise_info = {
+	.read_raw = sunrise_read_raw
+};
+
+static struct regmap_config sunrise_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8
+};
+
+static int sunrise_probe(struct i2c_client *client)
+{
+	struct sunrise_dev *sunrise;
+	struct iio_dev *iio_dev;
+
+	iio_dev = devm_iio_device_alloc(&client->dev, sizeof(*sunrise));
+	if (!iio_dev)
+		return -ENOMEM;
+
:q
:q

+	i2c_set_clientdata(client, iio_dev);
+
+	sunrise = iio_priv(iio_dev);
+	sunrise->client = client;
+	sunrise->dev = &client->dev;
+	mutex_init(&sunrise->lock);
+
+	sunrise->regmap = devm_regmap_init_i2c(client, &sunrise_regmap_config);
+	if (IS_ERR(sunrise->regmap)) {
+		dev_err(&client->dev, "Failed to initialize regmap\n");
+		return PTR_ERR(sunrise->regmap);
+	}
+
+	iio_dev->info = &sunrise_info;
+	iio_dev->name = DRIVER_NAME;
+	iio_dev->channels = sunrise_channels;
+	iio_dev->num_channels = ARRAY_SIZE(sunrise_channels);
+	iio_dev->modes = INDIO_DIRECT_MODE;
+
+	return devm_iio_device_register(&client->dev, iio_dev);
+}
+
+static const struct of_device_id sunrise_of_match[] = {
+	{ .compatible = "senseair,sunrise-006-0-0007" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, sunrise_of_match);
+
+static struct i2c_driver sunrise_driver = {
+	.driver = {
+		.name = DRIVER_NAME,
+		.of_match_table = sunrise_of_match
+	},
+	.probe_new = sunrise_probe
+};
+module_i2c_driver(sunrise_driver);
+
+MODULE_AUTHOR("Jacopo Mondi <jacopo@jmondi.org>");
+MODULE_DESCRIPTION("Senseair Sunrise 006-0-0007 CO2 sensor IIO driver");
+MODULE_LICENSE("GPL v2");
-- 
2.32.0

Re: [PATCH v3 2/3] iio: chemical: Add Sensteair Sunrise 006-0-007 driver

[Andy Shevchenko]

On Sun, Aug 22, 2021 at 9:49 PM Jacopo Mondi <jacopo@jmondi.org> wrote:
>
> Add support for the Senseair Sunrise 006-0-0007 driver through the
> IIO subsystem.

Thanks for an update, my comments below.

> Datasheet: https://rmtplusstoragesenseair.blob.core.windows.net/docs/Dev/publicerat/TDE5531.pdf
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  MAINTAINERS                        |   6 +
>  drivers/iio/chemical/Kconfig       |  13 +
>  drivers/iio/chemical/Makefile      |   1 +
>  drivers/iio/chemical/sunrise_co2.c | 450 +++++++++++++++++++++++++++++
>  4 files changed, 470 insertions(+)
>  create mode 100644 drivers/iio/chemical/sunrise_co2.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 90ca9df1d3c3..43f5bba46673 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -16544,6 +16544,12 @@ S:     Maintained
>  F:     drivers/misc/phantom.c
>  F:     include/uapi/linux/phantom.h
>
> +SENSEAIR SUNRISE 006-0-0007
> +M:     Jacopo Mondi <jacopo@jmondi.org>
> +S:     Maintained
> +F:     Documentation/devicetree/bindings/iio/chemical/senseair,sunrise.yaml
> +F:     drivers/iio/chemical/sunrise_co2.c
> +
>  SENSIRION SCD30 CARBON DIOXIDE SENSOR DRIVER
>  M:     Tomasz Duszynski <tomasz.duszynski@octakon.com>
>  S:     Maintained
> diff --git a/drivers/iio/chemical/Kconfig b/drivers/iio/chemical/Kconfig
> index 10bb431bc3ce..ee8562949226 100644
> --- a/drivers/iio/chemical/Kconfig
> +++ b/drivers/iio/chemical/Kconfig
> @@ -144,6 +144,19 @@ config SPS30
>           To compile this driver as a module, choose M here: the module will
>           be called sps30.
>
> +config SENSEAIR_SUNRISE_CO2
> +       tristate "Senseair Sunrise 006-0-0007 CO2 sensor"
> +       depends on OF
> +       depends on I2C
> +       depends on SYSFS
> +       select REGMAP_I2C
> +       help
> +         Say yes here to build support for Senseair Sunrise 006-0-0007 CO2
> +         sensor.
> +
> +         To compile this driver as a module, choose M here: the
> +         module will be called sunrise_co2.
> +
>  config VZ89X
>         tristate "SGX Sensortech MiCS VZ89X VOC sensor"
>         depends on I2C
> diff --git a/drivers/iio/chemical/Makefile b/drivers/iio/chemical/Makefile
> index fef63dd5bf92..d5e2a3331d57 100644
> --- a/drivers/iio/chemical/Makefile
> +++ b/drivers/iio/chemical/Makefile
> @@ -17,4 +17,5 @@ obj-$(CONFIG_SCD30_I2C) += scd30_i2c.o
>  obj-$(CONFIG_SCD30_SERIAL) += scd30_serial.o
>  obj-$(CONFIG_SENSIRION_SGP30)  += sgp30.o
>  obj-$(CONFIG_SPS30) += sps30.o
> +obj-$(CONFIG_SENSEAIR_SUNRISE_CO2) += sunrise_co2.o
>  obj-$(CONFIG_VZ89X)            += vz89x.o
> diff --git a/drivers/iio/chemical/sunrise_co2.c b/drivers/iio/chemical/sunrise_co2.c
> new file mode 100644
> index 000000000000..1dcfe8e1a94a
> --- /dev/null
> +++ b/drivers/iio/chemical/sunrise_co2.c
> @@ -0,0 +1,450 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Senseair Sunrise 006-0-0007 CO2 sensor driver.
> + *
> + * Copyright (C) 2021 Jacopo Mondi
> + *
> + * List of features not yet supported by the driver:
> + * - controllable EN pin
> + * - single-shot operations using the nDRY pin.
> + * - ABC/target calibration
> + */
> +
> +#include <linux/bitops.h>
> +#include <linux/i2c.h>
> +#include <linux/kernel.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/regmap.h>
> +#include <linux/time64.h>
> +
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +
> +#define DRIVER_NAME "sunrise"
> +
> +#define SUNRISE_ERROR_STATUS_REG               0x00
> +#define SUNRISE_CO2_FILTERED_COMP_REG          0x06
> +#define SUNRISE_CHIP_TEMPERATURE_REG           0x08
> +#define SUNRISE_CALIBRATION_STATUS_REG         0x81
> +#define SUNRISE_CALIBRATION_COMMAND_REG                0x82
> +#define SUNRISE_CALIBRATION_FACTORY_CMD                0x7c02
> +#define SUNRISE_CALIBRATION_BACKGROUND_CMD     0x7c06
> +/*
> + * The calibration timeout is not characterized in the datasheet.
> + * Use 30 seconds as a reasonable upper limit.
> + */
> +#define SUNRISE_CALIBRATION_TIMEOUT_US         (30 * USEC_PER_SEC)
> +
> +enum sunrise_calib {
> +       SUNRISE_CALIBRATION_FACTORY,
> +       SUNRISE_CALIBRATION_BACKGROUND,
> +};
> +
> +struct sunrise_dev {

> +       struct device *dev;
> +       struct i2c_client *client;

Is the *dev is different to &client->dev?

> +       struct regmap *regmap;
> +       struct mutex lock;
> +       enum sunrise_calib calibration;
> +};
> +
> +static void sunrise_wakeup(struct sunrise_dev *sunrise)
> +{
> +       struct i2c_client *client = sunrise->client;
> +
> +       /*
> +        * Wake up sensor by sending sensor address: START, sensor address,
> +        * STOP. Sensor will not ACK this byte.
> +        *
> +        * The chip returns in low power state after 15msec without
> +        * communications or after a complete read/write sequence.
> +        */
> +       i2c_smbus_xfer(client->adapter, client->addr, I2C_M_IGNORE_NAK,
> +                      I2C_SMBUS_WRITE, 0, I2C_SMBUS_QUICK, NULL);
> +}
> +
> +static int sunrise_read_word(struct sunrise_dev *sunrise, u8 reg, u16 *val)
> +{
> +       __be16 be_val;
> +       int ret;
> +
> +       sunrise_wakeup(sunrise);
> +       ret = regmap_bulk_read(sunrise->regmap, reg, &be_val, 2);
> +       if (ret) {
> +               dev_err(sunrise->dev, "Read word failed: reg 0x%2x (%d)\n", reg, ret);
> +               return ret;
> +       }
> +
> +       *val = be16_to_cpu(be_val);
> +
> +       return 0;
> +}
> +
> +static int sunrise_write_byte(struct sunrise_dev *sunrise, u8 reg, u8 val)
> +{
> +       int ret;
> +
> +       sunrise_wakeup(sunrise);
> +       ret = regmap_write(sunrise->regmap, reg, val);
> +       if (ret) {
> +               dev_err(sunrise->dev, "Write byte failed: reg 0x%2x (%d)\n", reg, ret);
> +               return ret;
> +       }
> +
> +       return 0;
> +}
> +
> +static int sunrise_write_word(struct sunrise_dev *sunrise, u8 reg, u16 data)
> +{
> +       __be16 be_data = cpu_to_be16(data);
> +       int ret;
> +
> +       sunrise_wakeup(sunrise);
> +       ret = regmap_bulk_write(sunrise->regmap, reg, &be_data, 2);
> +       if (ret) {
> +               dev_err(sunrise->dev, "Write word failed: reg 0x%2x (%d)\n", reg, ret);
> +               return ret;
> +       }
> +
> +       return 0;
> +}
> +
> +/*
> + *  --- Calibration ---
> + *
> + *  Enumerate and select calibration modes, trigger a calibration cycle.
> + */
> +static const char * const sunrise_calibration_modes[] = {
> +       [SUNRISE_CALIBRATION_FACTORY] = "factory_calibration",
> +       [SUNRISE_CALIBRATION_BACKGROUND] = "background_calibration"
> +};
> +
> +static const struct sunrise_calibration_data {
> +       u16 calibration_cmd;
> +       u8 calibration_bit;
> +} sunrise_calibrations[] = {
> +       [SUNRISE_CALIBRATION_FACTORY] = {
> +               SUNRISE_CALIBRATION_FACTORY_CMD,
> +               BIT(2)

+ Comma

> +       },
> +       [SUNRISE_CALIBRATION_BACKGROUND] = {
> +               SUNRISE_CALIBRATION_BACKGROUND_CMD,
> +               BIT(5)

Ditto.

> +};
> +
> +static int sunrise_calibrate(struct sunrise_dev *sunrise)
> +{
> +       const struct sunrise_calibration_data *data;
> +       unsigned int status;
> +       int ret;
> +
> +       /* Reset the calibration status reg. */
> +       ret = sunrise_write_byte(sunrise, SUNRISE_CALIBRATION_STATUS_REG, 0x00);
> +       if (ret)
> +               return ret;
> +
> +       /* Write a calibration command and poll the calibration status bit. */
> +       data = &sunrise_calibrations[sunrise->calibration];
> +       ret = sunrise_write_word(sunrise, SUNRISE_CALIBRATION_COMMAND_REG,
> +                                data->calibration_cmd);
> +       if (ret)
> +               return ret;
> +
> +       dev_dbg(sunrise->dev, "%s in progress\n",
> +               sunrise_calibration_modes[sunrise->calibration]);
> +
> +       return regmap_read_poll_timeout(sunrise->regmap,
> +                                       SUNRISE_CALIBRATION_STATUS_REG,
> +                                       status, status & data->calibration_bit,
> +                                       100, SUNRISE_CALIBRATION_TIMEOUT_US);
> +}
> +
> +static ssize_t sunrise_calibration_write(struct iio_dev *iiodev,
> +                                        uintptr_t private,
> +                                        const struct iio_chan_spec *chan,
> +                                        const char *buf, size_t len)
> +{
> +       struct sunrise_dev *sunrise = iio_priv(iiodev);
> +       bool calibrate;
> +       int ret;
> +
> +       ret = kstrtobool(buf, &calibrate);
> +       if (ret)
> +               return ret;
> +
> +       if (!calibrate)
> +               return 0;
> +
> +       ret = sunrise_calibrate(sunrise);
> +       if (ret)
> +               return ret;
> +
> +       return len;
> +}
> +
> +static int sunrise_set_calibration_mode(struct iio_dev *iiodev,
> +                                       const struct iio_chan_spec *chan,
> +                                       unsigned int mode)
> +{
> +       struct sunrise_dev *sunrise = iio_priv(iiodev);
> +
> +       mutex_lock(&sunrise->lock);
> +       sunrise->calibration = mode;
> +       mutex_unlock(&sunrise->lock);
> +
> +       return 0;
> +}
> +
> +static int sunrise_get_calibration_mode(struct iio_dev *iiodev,
> +                                       const struct iio_chan_spec *chan)
> +{
> +       struct sunrise_dev *sunrise = iio_priv(iiodev);
> +       int mode;
> +
> +       mutex_lock(&sunrise->lock);
> +       mode = sunrise->calibration;
> +       mutex_unlock(&sunrise->lock);
> +
> +       return mode;
> +}
> +
> +static const struct iio_enum sunrise_calibration_modes_enum = {
> +       .items = sunrise_calibration_modes,
> +       .num_items = ARRAY_SIZE(sunrise_calibration_modes),
> +       .set = sunrise_set_calibration_mode,
> +       .get = sunrise_get_calibration_mode,
> +};
> +
> +/* --- Error status---
> + *
> + * Enumerate and retrieve the chip error status.
> + */
> +enum {
> +       SUNRISE_ERROR_FATAL,
> +       SUNRISE_ERROR_I2C,
> +       SUNRISE_ERROR_ALGORITHM,
> +       SUNRISE_ERROR_CALIBRATION,
> +       SUNRISE_ERROR_SELF_DIAGNOSTIC,
> +       SUNRISE_ERROR_OUT_OF_RANGE,
> +       SUNRISE_ERROR_MEMORY,
> +       SUNRISE_ERROR_NO_MEASUREMENT,
> +       SUNRISE_ERROR_LOW_VOLTAGE,
> +       SUNRISE_ERROR_MEASUREMENT_TIMEOUT

+ Comma

> +};
> +
> +static const char * const sunrise_error_statuses[] = {
> +       [SUNRISE_ERROR_FATAL] = "error_fatal",
> +       [SUNRISE_ERROR_I2C] = "error_i2c",
> +       [SUNRISE_ERROR_ALGORITHM] = "error_algorithm",
> +       [SUNRISE_ERROR_CALIBRATION] = "error_calibration",
> +       [SUNRISE_ERROR_SELF_DIAGNOSTIC] = "error_self_diagnostic",
> +       [SUNRISE_ERROR_OUT_OF_RANGE] = "error_out_of_range",
> +       [SUNRISE_ERROR_MEMORY] = "error_memory",
> +       [SUNRISE_ERROR_NO_MEASUREMENT] = "error_no_measurement",
> +       [SUNRISE_ERROR_LOW_VOLTAGE] = "error_low_voltage",
> +       [SUNRISE_ERROR_MEASUREMENT_TIMEOUT] = "error_measurement_timeout"

Ditto.

> +};
> +
> +static const u8 error_codes[] = {
> +       SUNRISE_ERROR_FATAL,
> +       SUNRISE_ERROR_I2C,
> +       SUNRISE_ERROR_ALGORITHM,
> +       SUNRISE_ERROR_CALIBRATION,
> +       SUNRISE_ERROR_SELF_DIAGNOSTIC,
> +       SUNRISE_ERROR_OUT_OF_RANGE,
> +       SUNRISE_ERROR_MEMORY,
> +       SUNRISE_ERROR_NO_MEASUREMENT,
> +       SUNRISE_ERROR_LOW_VOLTAGE,
> +       SUNRISE_ERROR_MEASUREMENT_TIMEOUT

Ditto.

> +};
> +
> +static const struct iio_enum sunrise_error_statuses_enum = {
> +       .items = sunrise_error_statuses,
> +       .num_items = ARRAY_SIZE(sunrise_error_statuses)

Ditto.


> +};
> +
> +static ssize_t sunrise_error_status_read(struct iio_dev *iiodev,
> +                                        uintptr_t private,
> +                                        const struct iio_chan_spec *chan,
> +                                        char *buf)
> +{
> +       struct sunrise_dev *sunrise = iio_priv(iiodev);
> +       const unsigned long *errors;
> +       ssize_t len = 0;
> +       u16 value;
> +       int ret;
> +       u8 i;
> +
> +       ret = sunrise_read_word(sunrise, SUNRISE_ERROR_STATUS_REG, &value);
> +       if (ret)
> +               return -EINVAL;
> +

> +pr_err("%s:%d0x%x\n", __func__, __LINE__, value);

What's this?

> +       errors = (const unsigned long *)&value;
> +       for_each_set_bit(i, errors, ARRAY_SIZE(error_codes))
> +               len += sysfs_emit_at(buf, len, "%s ", sunrise_error_statuses[i]);
> +
> +       if (len)
> +               buf[len - 1] = '\n';
> +
> +       return len;
> +}
> +
> +static const struct iio_chan_spec_ext_info sunrise_concentration_ext_info[] = {
> +       /* Calibration modes and calibration trigger. */
> +       {
> +               .name = "calibration",
> +               .write = sunrise_calibration_write,
> +               .shared = IIO_SEPARATE

+ Comma

> +       },
> +       IIO_ENUM("calibration_mode", IIO_SEPARATE,
> +                &sunrise_calibration_modes_enum),
> +       IIO_ENUM_AVAILABLE("calibration_mode",
> +                          &sunrise_calibration_modes_enum),
> +
> +       /* Error statuses. */
> +       {
> +               .name = "error_status",
> +               .read = sunrise_error_status_read,
> +               .shared = IIO_SEPARATE
> +       },
> +       IIO_ENUM_AVAILABLE("error_status", &sunrise_error_statuses_enum),
> +       {}
> +};
> +
> +static const struct iio_chan_spec sunrise_channels[] = {
> +       {
> +               .type = IIO_CONCENTRATION,
> +               .modified = 1,
> +               .channel2 = IIO_MOD_CO2,
> +               .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> +               .ext_info = sunrise_concentration_ext_info,
> +               .scan_index = 0,
> +               .scan_type =  {
> +                       .sign = 's',
> +                       .realbits = 16,
> +                       .storagebits = 16,
> +                       .endianness = IIO_CPU,
> +               }
> +       },
> +       {
> +               .type = IIO_TEMP,
> +               .info_mask_separate =  BIT(IIO_CHAN_INFO_RAW) |
> +                                      BIT(IIO_CHAN_INFO_SCALE),
> +               .scan_index = 1,
> +               .scan_type =  {
> +                       .sign = 's',
> +                       .realbits = 16,
> +                       .storagebits = 16,
> +                       .endianness = IIO_CPU,
> +               }
> +       }

+ Comma

> +};
> +
> +static int sunrise_read_raw(struct iio_dev *iio_dev,
> +                           const struct iio_chan_spec *chan,
> +                           int *val, int *val2, long mask)
> +{
> +       struct sunrise_dev *sunrise = iio_priv(iio_dev);
> +       u16 value;
> +       int ret;
> +
> +       switch (mask) {
> +       case IIO_CHAN_INFO_RAW:
> +               mutex_lock(&sunrise->lock);
> +
> +               switch (chan->type) {
> +               case IIO_CONCENTRATION:
> +                       ret = sunrise_read_word(sunrise, SUNRISE_CO2_FILTERED_COMP_REG,
> +                                               &value);
> +                       *val = value;
> +                       mutex_unlock(&sunrise->lock);
> +
> +                       return ret ?: IIO_VAL_INT;
> +
> +               case IIO_TEMP:
> +                       ret = sunrise_read_word(sunrise, SUNRISE_CHIP_TEMPERATURE_REG,
> +                                               &value);
> +                       *val = value;
> +                       mutex_unlock(&sunrise->lock);
> +
> +                       return ret ?: IIO_VAL_INT;
> +
> +               default:
> +                       mutex_unlock(&sunrise->lock);
> +                       return -EINVAL;
> +               }
> +
> +       case IIO_CHAN_INFO_SCALE:
> +               /* Chip temperature scale = 1/100 */
> +               *val = 1;
> +               *val2 = 100;
> +               return IIO_VAL_FRACTIONAL;
> +
> +       default:
> +               return -EINVAL;
> +       }
> +}
> +
> +static const struct iio_info sunrise_info = {
> +       .read_raw = sunrise_read_raw

+ Comma

> +};
> +
> +static struct regmap_config sunrise_regmap_config = {
> +       .reg_bits = 8,
> +       .val_bits = 8

+ Comma

Do you need a regmap lock?

> +};
> +
> +static int sunrise_probe(struct i2c_client *client)
> +{
> +       struct sunrise_dev *sunrise;
> +       struct iio_dev *iio_dev;
> +
> +       iio_dev = devm_iio_device_alloc(&client->dev, sizeof(*sunrise));
> +       if (!iio_dev)
> +               return -ENOMEM;

> +
> :q
> :q

Not sure what this is and how it comes to here. Have you tested your patches?

> +       i2c_set_clientdata(client, iio_dev);
> +
> +       sunrise = iio_priv(iio_dev);
> +       sunrise->client = client;
> +       sunrise->dev = &client->dev;
> +       mutex_init(&sunrise->lock);
> +
> +       sunrise->regmap = devm_regmap_init_i2c(client, &sunrise_regmap_config);
> +       if (IS_ERR(sunrise->regmap)) {
> +               dev_err(&client->dev, "Failed to initialize regmap\n");
> +               return PTR_ERR(sunrise->regmap);
> +       }
> +
> +       iio_dev->info = &sunrise_info;
> +       iio_dev->name = DRIVER_NAME;
> +       iio_dev->channels = sunrise_channels;
> +       iio_dev->num_channels = ARRAY_SIZE(sunrise_channels);
> +       iio_dev->modes = INDIO_DIRECT_MODE;
> +
> +       return devm_iio_device_register(&client->dev, iio_dev);
> +}
> +
> +static const struct of_device_id sunrise_of_match[] = {
> +       { .compatible = "senseair,sunrise-006-0-0007" },
> +       {}
> +};
> +MODULE_DEVICE_TABLE(of, sunrise_of_match);
> +
> +static struct i2c_driver sunrise_driver = {
> +       .driver = {
> +               .name = DRIVER_NAME,
> +               .of_match_table = sunrise_of_match

+ Comma

> +       },
> +       .probe_new = sunrise_probe

+ Comma

> +};

I think you didn't get my comment about commas. Terminator line is one
which is used as a marker of the end of an array. In such cases a
comma is not needed, otherwise always add it.

--
With Best Regards,
Andy Shevchenko

[PATCH v3.1 2/3] iio: chemical: Add Senseair Sunrise 006-0-007 driver

[Jacopo Mondi]

Add support for the Senseair Sunrise 006-0-0007 driver through the
IIO subsystem.

Datasheet: https://rmtplusstoragesenseair.blob.core.windows.net/docs/Dev/publicerat/TDE5531.pdf
Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
v3->v3.1
- Remove debug leftover
- Re-add commas at the end of arrays declarations
---
 MAINTAINERS                        |   6 +
 drivers/iio/chemical/Kconfig       |  13 +
 drivers/iio/chemical/Makefile      |   1 +
 drivers/iio/chemical/sunrise_co2.c | 448 +++++++++++++++++++++++++++++
 4 files changed, 468 insertions(+)
 create mode 100644 drivers/iio/chemical/sunrise_co2.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 90ca9df1d3c3..43f5bba46673 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -16544,6 +16544,12 @@ S:	Maintained
 F:	drivers/misc/phantom.c
 F:	include/uapi/linux/phantom.h

+SENSEAIR SUNRISE 006-0-0007
+M:	Jacopo Mondi <jacopo@jmondi.org>
+S:	Maintained
+F:	Documentation/devicetree/bindings/iio/chemical/senseair,sunrise.yaml
+F:	drivers/iio/chemical/sunrise_co2.c
+
 SENSIRION SCD30 CARBON DIOXIDE SENSOR DRIVER
 M:	Tomasz Duszynski <tomasz.duszynski@octakon.com>
 S:	Maintained
diff --git a/drivers/iio/chemical/Kconfig b/drivers/iio/chemical/Kconfig
index 10bb431bc3ce..ee8562949226 100644
--- a/drivers/iio/chemical/Kconfig
+++ b/drivers/iio/chemical/Kconfig
@@ -144,6 +144,19 @@ config SPS30
 	  To compile this driver as a module, choose M here: the module will
 	  be called sps30.

+config SENSEAIR_SUNRISE_CO2
+	tristate "Senseair Sunrise 006-0-0007 CO2 sensor"
+	depends on OF
+	depends on I2C
+	depends on SYSFS
+	select REGMAP_I2C
+	help
+	  Say yes here to build support for Senseair Sunrise 006-0-0007 CO2
+	  sensor.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called sunrise_co2.
+
 config VZ89X
 	tristate "SGX Sensortech MiCS VZ89X VOC sensor"
 	depends on I2C
diff --git a/drivers/iio/chemical/Makefile b/drivers/iio/chemical/Makefile
index fef63dd5bf92..d5e2a3331d57 100644
--- a/drivers/iio/chemical/Makefile
+++ b/drivers/iio/chemical/Makefile
@@ -17,4 +17,5 @@ obj-$(CONFIG_SCD30_I2C) += scd30_i2c.o
 obj-$(CONFIG_SCD30_SERIAL) += scd30_serial.o
 obj-$(CONFIG_SENSIRION_SGP30)	+= sgp30.o
 obj-$(CONFIG_SPS30) += sps30.o
+obj-$(CONFIG_SENSEAIR_SUNRISE_CO2) += sunrise_co2.o
 obj-$(CONFIG_VZ89X)		+= vz89x.o
diff --git a/drivers/iio/chemical/sunrise_co2.c b/drivers/iio/chemical/sunrise_co2.c
new file mode 100644
index 000000000000..84f19df6fc00
--- /dev/null
+++ b/drivers/iio/chemical/sunrise_co2.c
@@ -0,0 +1,448 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Senseair Sunrise 006-0-0007 CO2 sensor driver.
+ *
+ * Copyright (C) 2021 Jacopo Mondi
+ *
+ * List of features not yet supported by the driver:
+ * - controllable EN pin
+ * - single-shot operations using the nDRY pin.
+ * - ABC/target calibration
+ */
+
+#include <linux/bitops.h>
+#include <linux/i2c.h>
+#include <linux/kernel.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/regmap.h>
+#include <linux/time64.h>
+
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+
+#define DRIVER_NAME "sunrise"
+
+#define SUNRISE_ERROR_STATUS_REG		0x00
+#define SUNRISE_CO2_FILTERED_COMP_REG		0x06
+#define SUNRISE_CHIP_TEMPERATURE_REG		0x08
+#define SUNRISE_CALIBRATION_STATUS_REG		0x81
+#define SUNRISE_CALIBRATION_COMMAND_REG		0x82
+#define SUNRISE_CALIBRATION_FACTORY_CMD		0x7c02
+#define SUNRISE_CALIBRATION_BACKGROUND_CMD	0x7c06
+/*
+ * The calibration timeout is not characterized in the datasheet.
+ * Use 30 seconds as a reasonable upper limit.
+ */
+#define SUNRISE_CALIBRATION_TIMEOUT_US		(30 * USEC_PER_SEC)
+
+enum sunrise_calib {
+	SUNRISE_CALIBRATION_FACTORY,
+	SUNRISE_CALIBRATION_BACKGROUND,
+};
+
+struct sunrise_dev {
+	struct device *dev;
+	struct i2c_client *client;
+	struct regmap *regmap;
+	struct mutex lock;
+	enum sunrise_calib calibration;
+};
+
+static void sunrise_wakeup(struct sunrise_dev *sunrise)
+{
+	struct i2c_client *client = sunrise->client;
+
+	/*
+	 * Wake up sensor by sending sensor address: START, sensor address,
+	 * STOP. Sensor will not ACK this byte.
+	 *
+	 * The chip returns in low power state after 15msec without
+	 * communications or after a complete read/write sequence.
+	 */
+	i2c_smbus_xfer(client->adapter, client->addr, I2C_M_IGNORE_NAK,
+		       I2C_SMBUS_WRITE, 0, I2C_SMBUS_QUICK, NULL);
+}
+
+static int sunrise_read_word(struct sunrise_dev *sunrise, u8 reg, u16 *val)
+{
+	__be16 be_val;
+	int ret;
+
+	sunrise_wakeup(sunrise);
+	ret = regmap_bulk_read(sunrise->regmap, reg, &be_val, 2);
+	if (ret) {
+		dev_err(sunrise->dev, "Read word failed: reg 0x%2x (%d)\n", reg, ret);
+		return ret;
+	}
+
+	*val = be16_to_cpu(be_val);
+
+	return 0;
+}
+
+static int sunrise_write_byte(struct sunrise_dev *sunrise, u8 reg, u8 val)
+{
+	int ret;
+
+	sunrise_wakeup(sunrise);
+	ret = regmap_write(sunrise->regmap, reg, val);
+	if (ret) {
+		dev_err(sunrise->dev, "Write byte failed: reg 0x%2x (%d)\n", reg, ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int sunrise_write_word(struct sunrise_dev *sunrise, u8 reg, u16 data)
+{
+	__be16 be_data = cpu_to_be16(data);
+	int ret;
+
+	sunrise_wakeup(sunrise);
+	ret = regmap_bulk_write(sunrise->regmap, reg, &be_data, 2);
+	if (ret) {
+		dev_err(sunrise->dev, "Write word failed: reg 0x%2x (%d)\n", reg, ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+/*
+ *  --- Calibration ---
+ *
+ *  Enumerate and select calibration modes, trigger a calibration cycle.
+ */
+static const char * const sunrise_calibration_modes[] = {
+	[SUNRISE_CALIBRATION_FACTORY] = "factory_calibration",
+	[SUNRISE_CALIBRATION_BACKGROUND] = "background_calibration",
+};
+
+static const struct sunrise_calibration_data {
+	u16 calibration_cmd;
+	u8 calibration_bit;
+} sunrise_calibrations[] = {
+	[SUNRISE_CALIBRATION_FACTORY] = {
+		SUNRISE_CALIBRATION_FACTORY_CMD,
+		BIT(2),
+	},
+	[SUNRISE_CALIBRATION_BACKGROUND] = {
+		SUNRISE_CALIBRATION_BACKGROUND_CMD,
+		BIT(5),
+	},
+};
+
+static int sunrise_calibrate(struct sunrise_dev *sunrise)
+{
+	const struct sunrise_calibration_data *data;
+	unsigned int status;
+	int ret;
+
+	/* Reset the calibration status reg. */
+	ret = sunrise_write_byte(sunrise, SUNRISE_CALIBRATION_STATUS_REG, 0x00);
+	if (ret)
+		return ret;
+
+	/* Write a calibration command and poll the calibration status bit. */
+	data = &sunrise_calibrations[sunrise->calibration];
+	ret = sunrise_write_word(sunrise, SUNRISE_CALIBRATION_COMMAND_REG,
+				 data->calibration_cmd);
+	if (ret)
+		return ret;
+
+	dev_dbg(sunrise->dev, "%s in progress\n",
+		sunrise_calibration_modes[sunrise->calibration]);
+
+	return regmap_read_poll_timeout(sunrise->regmap,
+					SUNRISE_CALIBRATION_STATUS_REG,
+					status, status & data->calibration_bit,
+					100, SUNRISE_CALIBRATION_TIMEOUT_US);
+}
+
+static ssize_t sunrise_calibration_write(struct iio_dev *iiodev,
+					 uintptr_t private,
+					 const struct iio_chan_spec *chan,
+					 const char *buf, size_t len)
+{
+	struct sunrise_dev *sunrise = iio_priv(iiodev);
+	bool calibrate;
+	int ret;
+
+	ret = kstrtobool(buf, &calibrate);
+	if (ret)
+		return ret;
+
+	if (!calibrate)
+		return 0;
+
+	ret = sunrise_calibrate(sunrise);
+	if (ret)
+		return ret;
+
+	return len;
+}
+
+static int sunrise_set_calibration_mode(struct iio_dev *iiodev,
+					const struct iio_chan_spec *chan,
+					unsigned int mode)
+{
+	struct sunrise_dev *sunrise = iio_priv(iiodev);
+
+	mutex_lock(&sunrise->lock);
+	sunrise->calibration = mode;
+	mutex_unlock(&sunrise->lock);
+
+	return 0;
+}
+
+static int sunrise_get_calibration_mode(struct iio_dev *iiodev,
+					const struct iio_chan_spec *chan)
+{
+	struct sunrise_dev *sunrise = iio_priv(iiodev);
+	int mode;
+
+	mutex_lock(&sunrise->lock);
+	mode = sunrise->calibration;
+	mutex_unlock(&sunrise->lock);
+
+	return mode;
+}
+
+static const struct iio_enum sunrise_calibration_modes_enum = {
+	.items = sunrise_calibration_modes,
+	.num_items = ARRAY_SIZE(sunrise_calibration_modes),
+	.set = sunrise_set_calibration_mode,
+	.get = sunrise_get_calibration_mode,
+};
+
+/* --- Error status---
+ *
+ * Enumerate and retrieve the chip error status.
+ */
+enum {
+	SUNRISE_ERROR_FATAL,
+	SUNRISE_ERROR_I2C,
+	SUNRISE_ERROR_ALGORITHM,
+	SUNRISE_ERROR_CALIBRATION,
+	SUNRISE_ERROR_SELF_DIAGNOSTIC,
+	SUNRISE_ERROR_OUT_OF_RANGE,
+	SUNRISE_ERROR_MEMORY,
+	SUNRISE_ERROR_NO_MEASUREMENT,
+	SUNRISE_ERROR_LOW_VOLTAGE,
+	SUNRISE_ERROR_MEASUREMENT_TIMEOUT,
+};
+
+static const char * const sunrise_error_statuses[] = {
+	[SUNRISE_ERROR_FATAL] = "error_fatal",
+	[SUNRISE_ERROR_I2C] = "error_i2c",
+	[SUNRISE_ERROR_ALGORITHM] = "error_algorithm",
+	[SUNRISE_ERROR_CALIBRATION] = "error_calibration",
+	[SUNRISE_ERROR_SELF_DIAGNOSTIC] = "error_self_diagnostic",
+	[SUNRISE_ERROR_OUT_OF_RANGE] = "error_out_of_range",
+	[SUNRISE_ERROR_MEMORY] = "error_memory",
+	[SUNRISE_ERROR_NO_MEASUREMENT] = "error_no_measurement",
+	[SUNRISE_ERROR_LOW_VOLTAGE] = "error_low_voltage",
+	[SUNRISE_ERROR_MEASUREMENT_TIMEOUT] = "error_measurement_timeout",
+};
+
+static const u8 error_codes[] = {
+	SUNRISE_ERROR_FATAL,
+	SUNRISE_ERROR_I2C,
+	SUNRISE_ERROR_ALGORITHM,
+	SUNRISE_ERROR_CALIBRATION,
+	SUNRISE_ERROR_SELF_DIAGNOSTIC,
+	SUNRISE_ERROR_OUT_OF_RANGE,
+	SUNRISE_ERROR_MEMORY,
+	SUNRISE_ERROR_NO_MEASUREMENT,
+	SUNRISE_ERROR_LOW_VOLTAGE,
+	SUNRISE_ERROR_MEASUREMENT_TIMEOUT,
+};
+
+static const struct iio_enum sunrise_error_statuses_enum = {
+	.items = sunrise_error_statuses,
+	.num_items = ARRAY_SIZE(sunrise_error_statuses),
+};
+
+static ssize_t sunrise_error_status_read(struct iio_dev *iiodev,
+					 uintptr_t private,
+					 const struct iio_chan_spec *chan,
+					 char *buf)
+{
+	struct sunrise_dev *sunrise = iio_priv(iiodev);
+	const unsigned long *errors;
+	ssize_t len = 0;
+	u16 value;
+	int ret;
+	u8 i;
+
+	ret = sunrise_read_word(sunrise, SUNRISE_ERROR_STATUS_REG, &value);
+	if (ret)
+		return -EINVAL;
+
+	errors = (const unsigned long *)&value;
+	for_each_set_bit(i, errors, ARRAY_SIZE(error_codes))
+		len += sysfs_emit_at(buf, len, "%s ", sunrise_error_statuses[i]);
+
+	if (len)
+		buf[len - 1] = '\n';
+
+	return len;
+}
+
+static const struct iio_chan_spec_ext_info sunrise_concentration_ext_info[] = {
+	/* Calibration modes and calibration trigger. */
+	{
+		.name = "calibration",
+		.write = sunrise_calibration_write,
+		.shared = IIO_SEPARATE,
+	},
+	IIO_ENUM("calibration_mode", IIO_SEPARATE,
+		 &sunrise_calibration_modes_enum),
+	IIO_ENUM_AVAILABLE("calibration_mode",
+			   &sunrise_calibration_modes_enum),
+
+	/* Error statuses. */
+	{
+		.name = "error_status",
+		.read = sunrise_error_status_read,
+		.shared = IIO_SEPARATE
+	},
+	IIO_ENUM_AVAILABLE("error_status", &sunrise_error_statuses_enum),
+	{}
+};
+
+static const struct iio_chan_spec sunrise_channels[] = {
+	{
+		.type = IIO_CONCENTRATION,
+		.modified = 1,
+		.channel2 = IIO_MOD_CO2,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+		.ext_info = sunrise_concentration_ext_info,
+		.scan_index = 0,
+		.scan_type =  {
+			.sign = 's',
+			.realbits = 16,
+			.storagebits = 16,
+			.endianness = IIO_CPU,
+		},
+	},
+	{
+		.type = IIO_TEMP,
+		.info_mask_separate =  BIT(IIO_CHAN_INFO_RAW) |
+				       BIT(IIO_CHAN_INFO_SCALE),
+		.scan_index = 1,
+		.scan_type =  {
+			.sign = 's',
+			.realbits = 16,
+			.storagebits = 16,
+			.endianness = IIO_CPU,
+		},
+	},
+};
+
+static int sunrise_read_raw(struct iio_dev *iio_dev,
+			    const struct iio_chan_spec *chan,
+			    int *val, int *val2, long mask)
+{
+	struct sunrise_dev *sunrise = iio_priv(iio_dev);
+	u16 value;
+	int ret;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		mutex_lock(&sunrise->lock);
+
+		switch (chan->type) {
+		case IIO_CONCENTRATION:
+			ret = sunrise_read_word(sunrise, SUNRISE_CO2_FILTERED_COMP_REG,
+						&value);
+			*val = value;
+			mutex_unlock(&sunrise->lock);
+
+			return ret ?: IIO_VAL_INT;
+
+		case IIO_TEMP:
+			ret = sunrise_read_word(sunrise, SUNRISE_CHIP_TEMPERATURE_REG,
+						&value);
+			*val = value;
+			mutex_unlock(&sunrise->lock);
+
+			return ret ?: IIO_VAL_INT;
+
+		default:
+			mutex_unlock(&sunrise->lock);
+			return -EINVAL;
+		}
+
+	case IIO_CHAN_INFO_SCALE:
+		/* Chip temperature scale = 1/100 */
+		*val = 1;
+		*val2 = 100;
+		return IIO_VAL_FRACTIONAL;
+
+	default:
+		return -EINVAL;
+	}
+}
+
+static const struct iio_info sunrise_info = {
+	.read_raw = sunrise_read_raw,
+};
+
+static struct regmap_config sunrise_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+};
+
+static int sunrise_probe(struct i2c_client *client)
+{
+	struct sunrise_dev *sunrise;
+	struct iio_dev *iio_dev;
+
+	iio_dev = devm_iio_device_alloc(&client->dev, sizeof(*sunrise));
+	if (!iio_dev)
+		return -ENOMEM;
+
+	i2c_set_clientdata(client, iio_dev);
+
+	sunrise = iio_priv(iio_dev);
+	sunrise->client = client;
+	sunrise->dev = &client->dev;
+	mutex_init(&sunrise->lock);
+
+	sunrise->regmap = devm_regmap_init_i2c(client, &sunrise_regmap_config);
+	if (IS_ERR(sunrise->regmap)) {
+		dev_err(&client->dev, "Failed to initialize regmap\n");
+		return PTR_ERR(sunrise->regmap);
+	}
+
+	iio_dev->info = &sunrise_info;
+	iio_dev->name = DRIVER_NAME;
+	iio_dev->channels = sunrise_channels;
+	iio_dev->num_channels = ARRAY_SIZE(sunrise_channels);
+	iio_dev->modes = INDIO_DIRECT_MODE;
+
+	return devm_iio_device_register(&client->dev, iio_dev);
+}
+
+static const struct of_device_id sunrise_of_match[] = {
+	{ .compatible = "senseair,sunrise-006-0-0007" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, sunrise_of_match);
+
+static struct i2c_driver sunrise_driver = {
+	.driver = {
+		.name = DRIVER_NAME,
+		.of_match_table = sunrise_of_match,
+	},
+	.probe_new = sunrise_probe,
+};
+module_i2c_driver(sunrise_driver);
+
+MODULE_AUTHOR("Jacopo Mondi <jacopo@jmondi.org>");
+MODULE_DESCRIPTION("Senseair Sunrise 006-0-0007 CO2 sensor IIO driver");
+MODULE_LICENSE("GPL v2");
--
2.32.0

Re: [PATCH v3.1 2/3] iio: chemical: Add Senseair Sunrise 006-0-007 driver

[Andy Shevchenko]

On Mon, Aug 23, 2021 at 09:36:39AM +0200, Jacopo Mondi wrote:
> Add support for the Senseair Sunrise 006-0-0007 driver through the
> IIO subsystem.

Thanks for this intermediate update, looks much better.
So, there are a few comments below and we are almost ready.

> Datasheet: https://rmtplusstoragesenseair.blob.core.windows.net/docs/Dev/publicerat/TDE5531.pdf
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
> v3->v3.1
> - Remove debug leftover
> - Re-add commas at the end of arrays declarations
> ---
>  MAINTAINERS                        |   6 +
>  drivers/iio/chemical/Kconfig       |  13 +
>  drivers/iio/chemical/Makefile      |   1 +
>  drivers/iio/chemical/sunrise_co2.c | 448 +++++++++++++++++++++++++++++
>  4 files changed, 468 insertions(+)
>  create mode 100644 drivers/iio/chemical/sunrise_co2.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 90ca9df1d3c3..43f5bba46673 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -16544,6 +16544,12 @@ S:	Maintained
>  F:	drivers/misc/phantom.c
>  F:	include/uapi/linux/phantom.h
> 
> +SENSEAIR SUNRISE 006-0-0007
> +M:	Jacopo Mondi <jacopo@jmondi.org>
> +S:	Maintained
> +F:	Documentation/devicetree/bindings/iio/chemical/senseair,sunrise.yaml
> +F:	drivers/iio/chemical/sunrise_co2.c
> +
>  SENSIRION SCD30 CARBON DIOXIDE SENSOR DRIVER
>  M:	Tomasz Duszynski <tomasz.duszynski@octakon.com>
>  S:	Maintained
> diff --git a/drivers/iio/chemical/Kconfig b/drivers/iio/chemical/Kconfig
> index 10bb431bc3ce..ee8562949226 100644
> --- a/drivers/iio/chemical/Kconfig
> +++ b/drivers/iio/chemical/Kconfig
> @@ -144,6 +144,19 @@ config SPS30
>  	  To compile this driver as a module, choose M here: the module will
>  	  be called sps30.
> 
> +config SENSEAIR_SUNRISE_CO2
> +	tristate "Senseair Sunrise 006-0-0007 CO2 sensor"
> +	depends on OF
> +	depends on I2C
> +	depends on SYSFS
> +	select REGMAP_I2C
> +	help
> +	  Say yes here to build support for Senseair Sunrise 006-0-0007 CO2
> +	  sensor.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called sunrise_co2.
> +
>  config VZ89X
>  	tristate "SGX Sensortech MiCS VZ89X VOC sensor"
>  	depends on I2C
> diff --git a/drivers/iio/chemical/Makefile b/drivers/iio/chemical/Makefile
> index fef63dd5bf92..d5e2a3331d57 100644
> --- a/drivers/iio/chemical/Makefile
> +++ b/drivers/iio/chemical/Makefile
> @@ -17,4 +17,5 @@ obj-$(CONFIG_SCD30_I2C) += scd30_i2c.o
>  obj-$(CONFIG_SCD30_SERIAL) += scd30_serial.o
>  obj-$(CONFIG_SENSIRION_SGP30)	+= sgp30.o
>  obj-$(CONFIG_SPS30) += sps30.o
> +obj-$(CONFIG_SENSEAIR_SUNRISE_CO2) += sunrise_co2.o
>  obj-$(CONFIG_VZ89X)		+= vz89x.o
> diff --git a/drivers/iio/chemical/sunrise_co2.c b/drivers/iio/chemical/sunrise_co2.c
> new file mode 100644
> index 000000000000..84f19df6fc00
> --- /dev/null
> +++ b/drivers/iio/chemical/sunrise_co2.c
> @@ -0,0 +1,448 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Senseair Sunrise 006-0-0007 CO2 sensor driver.
> + *
> + * Copyright (C) 2021 Jacopo Mondi
> + *
> + * List of features not yet supported by the driver:
> + * - controllable EN pin
> + * - single-shot operations using the nDRY pin.
> + * - ABC/target calibration
> + */
> +
> +#include <linux/bitops.h>
> +#include <linux/i2c.h>
> +#include <linux/kernel.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/regmap.h>
> +#include <linux/time64.h>
> +
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>

> +#define DRIVER_NAME "sunrise"

I would expect "sunrise_co2" here.

Also, since it's one time use, please drop the definition completely and use
literal in-place.

> +#define SUNRISE_ERROR_STATUS_REG		0x00
> +#define SUNRISE_CO2_FILTERED_COMP_REG		0x06
> +#define SUNRISE_CHIP_TEMPERATURE_REG		0x08
> +#define SUNRISE_CALIBRATION_STATUS_REG		0x81
> +#define SUNRISE_CALIBRATION_COMMAND_REG		0x82
> +#define SUNRISE_CALIBRATION_FACTORY_CMD		0x7c02
> +#define SUNRISE_CALIBRATION_BACKGROUND_CMD	0x7c06
> +/*
> + * The calibration timeout is not characterized in the datasheet.
> + * Use 30 seconds as a reasonable upper limit.
> + */
> +#define SUNRISE_CALIBRATION_TIMEOUT_US		(30 * USEC_PER_SEC)
> +
> +enum sunrise_calib {
> +	SUNRISE_CALIBRATION_FACTORY,
> +	SUNRISE_CALIBRATION_BACKGROUND,
> +};
> +
> +struct sunrise_dev {
> +	struct device *dev;
> +	struct i2c_client *client;
> +	struct regmap *regmap;
> +	struct mutex lock;
> +	enum sunrise_calib calibration;
> +};
> +
> +static void sunrise_wakeup(struct sunrise_dev *sunrise)
> +{
> +	struct i2c_client *client = sunrise->client;
> +
> +	/*
> +	 * Wake up sensor by sending sensor address: START, sensor address,
> +	 * STOP. Sensor will not ACK this byte.
> +	 *
> +	 * The chip returns in low power state after 15msec without
> +	 * communications or after a complete read/write sequence.
> +	 */
> +	i2c_smbus_xfer(client->adapter, client->addr, I2C_M_IGNORE_NAK,
> +		       I2C_SMBUS_WRITE, 0, I2C_SMBUS_QUICK, NULL);
> +}
> +
> +static int sunrise_read_word(struct sunrise_dev *sunrise, u8 reg, u16 *val)
> +{
> +	__be16 be_val;
> +	int ret;
> +
> +	sunrise_wakeup(sunrise);
> +	ret = regmap_bulk_read(sunrise->regmap, reg, &be_val, 2);
> +	if (ret) {
> +		dev_err(sunrise->dev, "Read word failed: reg 0x%2x (%d)\n", reg, ret);
> +		return ret;
> +	}
> +
> +	*val = be16_to_cpu(be_val);
> +
> +	return 0;
> +}
> +
> +static int sunrise_write_byte(struct sunrise_dev *sunrise, u8 reg, u8 val)
> +{
> +	int ret;
> +
> +	sunrise_wakeup(sunrise);
> +	ret = regmap_write(sunrise->regmap, reg, val);
> +	if (ret) {
> +		dev_err(sunrise->dev, "Write byte failed: reg 0x%2x (%d)\n", reg, ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int sunrise_write_word(struct sunrise_dev *sunrise, u8 reg, u16 data)
> +{
> +	__be16 be_data = cpu_to_be16(data);
> +	int ret;
> +
> +	sunrise_wakeup(sunrise);
> +	ret = regmap_bulk_write(sunrise->regmap, reg, &be_data, 2);
> +	if (ret) {
> +		dev_err(sunrise->dev, "Write word failed: reg 0x%2x (%d)\n", reg, ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +/*
> + *  --- Calibration ---
> + *
> + *  Enumerate and select calibration modes, trigger a calibration cycle.
> + */
> +static const char * const sunrise_calibration_modes[] = {
> +	[SUNRISE_CALIBRATION_FACTORY] = "factory_calibration",
> +	[SUNRISE_CALIBRATION_BACKGROUND] = "background_calibration",
> +};
> +
> +static const struct sunrise_calibration_data {
> +	u16 calibration_cmd;
> +	u8 calibration_bit;
> +} sunrise_calibrations[] = {
> +	[SUNRISE_CALIBRATION_FACTORY] = {
> +		SUNRISE_CALIBRATION_FACTORY_CMD,
> +		BIT(2),
> +	},
> +	[SUNRISE_CALIBRATION_BACKGROUND] = {
> +		SUNRISE_CALIBRATION_BACKGROUND_CMD,
> +		BIT(5),
> +	},
> +};
> +
> +static int sunrise_calibrate(struct sunrise_dev *sunrise)
> +{
> +	const struct sunrise_calibration_data *data;
> +	unsigned int status;
> +	int ret;
> +
> +	/* Reset the calibration status reg. */
> +	ret = sunrise_write_byte(sunrise, SUNRISE_CALIBRATION_STATUS_REG, 0x00);
> +	if (ret)
> +		return ret;
> +
> +	/* Write a calibration command and poll the calibration status bit. */
> +	data = &sunrise_calibrations[sunrise->calibration];
> +	ret = sunrise_write_word(sunrise, SUNRISE_CALIBRATION_COMMAND_REG,
> +				 data->calibration_cmd);
> +	if (ret)
> +		return ret;
> +
> +	dev_dbg(sunrise->dev, "%s in progress\n",
> +		sunrise_calibration_modes[sunrise->calibration]);
> +
> +	return regmap_read_poll_timeout(sunrise->regmap,
> +					SUNRISE_CALIBRATION_STATUS_REG,
> +					status, status & data->calibration_bit,
> +					100, SUNRISE_CALIBRATION_TIMEOUT_US);
> +}
> +
> +static ssize_t sunrise_calibration_write(struct iio_dev *iiodev,
> +					 uintptr_t private,
> +					 const struct iio_chan_spec *chan,
> +					 const char *buf, size_t len)
> +{
> +	struct sunrise_dev *sunrise = iio_priv(iiodev);
> +	bool calibrate;
> +	int ret;
> +
> +	ret = kstrtobool(buf, &calibrate);
> +	if (ret)
> +		return ret;
> +
> +	if (!calibrate)
> +		return 0;
> +
> +	ret = sunrise_calibrate(sunrise);
> +	if (ret)
> +		return ret;
> +
> +	return len;
> +}
> +
> +static int sunrise_set_calibration_mode(struct iio_dev *iiodev,
> +					const struct iio_chan_spec *chan,
> +					unsigned int mode)
> +{
> +	struct sunrise_dev *sunrise = iio_priv(iiodev);
> +
> +	mutex_lock(&sunrise->lock);
> +	sunrise->calibration = mode;
> +	mutex_unlock(&sunrise->lock);
> +
> +	return 0;
> +}
> +
> +static int sunrise_get_calibration_mode(struct iio_dev *iiodev,
> +					const struct iio_chan_spec *chan)
> +{
> +	struct sunrise_dev *sunrise = iio_priv(iiodev);
> +	int mode;
> +
> +	mutex_lock(&sunrise->lock);
> +	mode = sunrise->calibration;
> +	mutex_unlock(&sunrise->lock);
> +
> +	return mode;
> +}
> +
> +static const struct iio_enum sunrise_calibration_modes_enum = {
> +	.items = sunrise_calibration_modes,
> +	.num_items = ARRAY_SIZE(sunrise_calibration_modes),
> +	.set = sunrise_set_calibration_mode,
> +	.get = sunrise_get_calibration_mode,
> +};
> +
> +/* --- Error status---
> + *
> + * Enumerate and retrieve the chip error status.
> + */
> +enum {
> +	SUNRISE_ERROR_FATAL,
> +	SUNRISE_ERROR_I2C,
> +	SUNRISE_ERROR_ALGORITHM,
> +	SUNRISE_ERROR_CALIBRATION,
> +	SUNRISE_ERROR_SELF_DIAGNOSTIC,
> +	SUNRISE_ERROR_OUT_OF_RANGE,
> +	SUNRISE_ERROR_MEMORY,
> +	SUNRISE_ERROR_NO_MEASUREMENT,
> +	SUNRISE_ERROR_LOW_VOLTAGE,
> +	SUNRISE_ERROR_MEASUREMENT_TIMEOUT,
> +};
> +
> +static const char * const sunrise_error_statuses[] = {
> +	[SUNRISE_ERROR_FATAL] = "error_fatal",
> +	[SUNRISE_ERROR_I2C] = "error_i2c",
> +	[SUNRISE_ERROR_ALGORITHM] = "error_algorithm",
> +	[SUNRISE_ERROR_CALIBRATION] = "error_calibration",
> +	[SUNRISE_ERROR_SELF_DIAGNOSTIC] = "error_self_diagnostic",
> +	[SUNRISE_ERROR_OUT_OF_RANGE] = "error_out_of_range",
> +	[SUNRISE_ERROR_MEMORY] = "error_memory",
> +	[SUNRISE_ERROR_NO_MEASUREMENT] = "error_no_measurement",
> +	[SUNRISE_ERROR_LOW_VOLTAGE] = "error_low_voltage",
> +	[SUNRISE_ERROR_MEASUREMENT_TIMEOUT] = "error_measurement_timeout",
> +};
> +
> +static const u8 error_codes[] = {
> +	SUNRISE_ERROR_FATAL,
> +	SUNRISE_ERROR_I2C,
> +	SUNRISE_ERROR_ALGORITHM,
> +	SUNRISE_ERROR_CALIBRATION,
> +	SUNRISE_ERROR_SELF_DIAGNOSTIC,
> +	SUNRISE_ERROR_OUT_OF_RANGE,
> +	SUNRISE_ERROR_MEMORY,
> +	SUNRISE_ERROR_NO_MEASUREMENT,
> +	SUNRISE_ERROR_LOW_VOLTAGE,
> +	SUNRISE_ERROR_MEASUREMENT_TIMEOUT,
> +};
> +
> +static const struct iio_enum sunrise_error_statuses_enum = {
> +	.items = sunrise_error_statuses,
> +	.num_items = ARRAY_SIZE(sunrise_error_statuses),
> +};
> +
> +static ssize_t sunrise_error_status_read(struct iio_dev *iiodev,
> +					 uintptr_t private,
> +					 const struct iio_chan_spec *chan,
> +					 char *buf)
> +{
> +	struct sunrise_dev *sunrise = iio_priv(iiodev);
> +	const unsigned long *errors;
> +	ssize_t len = 0;
> +	u16 value;
> +	int ret;
> +	u8 i;
> +
> +	ret = sunrise_read_word(sunrise, SUNRISE_ERROR_STATUS_REG, &value);
> +	if (ret)
> +		return -EINVAL;
> +
> +	errors = (const unsigned long *)&value;

Yes, it takes a pointer, but in your case it will oops the kernel quite likely.

	unsigned long errors;

	...

	errors = value;
	for_each_set_bit(..., &errors, ...)

> +	for_each_set_bit(i, errors, ARRAY_SIZE(error_codes))
> +		len += sysfs_emit_at(buf, len, "%s ", sunrise_error_statuses[i]);
> +
> +	if (len)
> +		buf[len - 1] = '\n';
> +
> +	return len;
> +}
> +
> +static const struct iio_chan_spec_ext_info sunrise_concentration_ext_info[] = {
> +	/* Calibration modes and calibration trigger. */
> +	{
> +		.name = "calibration",
> +		.write = sunrise_calibration_write,
> +		.shared = IIO_SEPARATE,
> +	},
> +	IIO_ENUM("calibration_mode", IIO_SEPARATE,
> +		 &sunrise_calibration_modes_enum),
> +	IIO_ENUM_AVAILABLE("calibration_mode",
> +			   &sunrise_calibration_modes_enum),
> +
> +	/* Error statuses. */
> +	{
> +		.name = "error_status",
> +		.read = sunrise_error_status_read,
> +		.shared = IIO_SEPARATE
> +	},
> +	IIO_ENUM_AVAILABLE("error_status", &sunrise_error_statuses_enum),
> +	{}
> +};
> +
> +static const struct iio_chan_spec sunrise_channels[] = {
> +	{
> +		.type = IIO_CONCENTRATION,
> +		.modified = 1,
> +		.channel2 = IIO_MOD_CO2,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> +		.ext_info = sunrise_concentration_ext_info,
> +		.scan_index = 0,
> +		.scan_type =  {
> +			.sign = 's',
> +			.realbits = 16,
> +			.storagebits = 16,
> +			.endianness = IIO_CPU,
> +		},
> +	},
> +	{
> +		.type = IIO_TEMP,
> +		.info_mask_separate =  BIT(IIO_CHAN_INFO_RAW) |
> +				       BIT(IIO_CHAN_INFO_SCALE),
> +		.scan_index = 1,
> +		.scan_type =  {
> +			.sign = 's',
> +			.realbits = 16,
> +			.storagebits = 16,
> +			.endianness = IIO_CPU,
> +		},
> +	},
> +};
> +
> +static int sunrise_read_raw(struct iio_dev *iio_dev,
> +			    const struct iio_chan_spec *chan,
> +			    int *val, int *val2, long mask)
> +{
> +	struct sunrise_dev *sunrise = iio_priv(iio_dev);
> +	u16 value;
> +	int ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		mutex_lock(&sunrise->lock);
> +
> +		switch (chan->type) {
> +		case IIO_CONCENTRATION:
> +			ret = sunrise_read_word(sunrise, SUNRISE_CO2_FILTERED_COMP_REG,
> +						&value);
> +			*val = value;
> +			mutex_unlock(&sunrise->lock);
> +
> +			return ret ?: IIO_VAL_INT;
> +
> +		case IIO_TEMP:
> +			ret = sunrise_read_word(sunrise, SUNRISE_CHIP_TEMPERATURE_REG,
> +						&value);
> +			*val = value;
> +			mutex_unlock(&sunrise->lock);
> +
> +			return ret ?: IIO_VAL_INT;
> +
> +		default:
> +			mutex_unlock(&sunrise->lock);
> +			return -EINVAL;
> +		}
> +
> +	case IIO_CHAN_INFO_SCALE:
> +		/* Chip temperature scale = 1/100 */
> +		*val = 1;
> +		*val2 = 100;
> +		return IIO_VAL_FRACTIONAL;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static const struct iio_info sunrise_info = {
> +	.read_raw = sunrise_read_raw,
> +};
> +
> +static struct regmap_config sunrise_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,

Does it need a lock?
(I think it does, but I would like to confirm)

> +};
> +
> +static int sunrise_probe(struct i2c_client *client)
> +{
> +	struct sunrise_dev *sunrise;
> +	struct iio_dev *iio_dev;
> +
> +	iio_dev = devm_iio_device_alloc(&client->dev, sizeof(*sunrise));
> +	if (!iio_dev)
> +		return -ENOMEM;
> +
> +	i2c_set_clientdata(client, iio_dev);
> +
> +	sunrise = iio_priv(iio_dev);
> +	sunrise->client = client;
> +	sunrise->dev = &client->dev;
> +	mutex_init(&sunrise->lock);
> +
> +	sunrise->regmap = devm_regmap_init_i2c(client, &sunrise_regmap_config);
> +	if (IS_ERR(sunrise->regmap)) {
> +		dev_err(&client->dev, "Failed to initialize regmap\n");
> +		return PTR_ERR(sunrise->regmap);
> +	}
> +
> +	iio_dev->info = &sunrise_info;
> +	iio_dev->name = DRIVER_NAME;
> +	iio_dev->channels = sunrise_channels;
> +	iio_dev->num_channels = ARRAY_SIZE(sunrise_channels);
> +	iio_dev->modes = INDIO_DIRECT_MODE;
> +
> +	return devm_iio_device_register(&client->dev, iio_dev);
> +}
> +
> +static const struct of_device_id sunrise_of_match[] = {
> +	{ .compatible = "senseair,sunrise-006-0-0007" },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, sunrise_of_match);
> +
> +static struct i2c_driver sunrise_driver = {
> +	.driver = {
> +		.name = DRIVER_NAME,
> +		.of_match_table = sunrise_of_match,
> +	},
> +	.probe_new = sunrise_probe,
> +};
> +module_i2c_driver(sunrise_driver);
> +
> +MODULE_AUTHOR("Jacopo Mondi <jacopo@jmondi.org>");
> +MODULE_DESCRIPTION("Senseair Sunrise 006-0-0007 CO2 sensor IIO driver");
> +MODULE_LICENSE("GPL v2");
> --
> 2.32.0
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3.1 2/3] iio: chemical: Add Senseair Sunrise 006-0-007 driver

[Jacopo Mondi]

Hi Andy,
   thanks for the immediate review :)

On Mon, Aug 23, 2021 at 11:35:23AM +0300, Andy Shevchenko wrote:
> On Mon, Aug 23, 2021 at 09:36:39AM +0200, Jacopo Mondi wrote:
> > Add support for the Senseair Sunrise 006-0-0007 driver through the
> > IIO subsystem.
>
> Thanks for this intermediate update, looks much better.
> So, there are a few comments below and we are almost ready.

Thanks, I would also like feedback on the usage of channel's
ext_info to handle calibration instead of using raw attributes as
suggested by Matt

>
> > Datasheet: https://rmtplusstoragesenseair.blob.core.windows.net/docs/Dev/publicerat/TDE5531.pdf
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> > v3->v3.1
> > - Remove debug leftover
> > - Re-add commas at the end of arrays declarations
> > ---
> >  MAINTAINERS                        |   6 +
> >  drivers/iio/chemical/Kconfig       |  13 +
> >  drivers/iio/chemical/Makefile      |   1 +
> >  drivers/iio/chemical/sunrise_co2.c | 448 +++++++++++++++++++++++++++++
> >  4 files changed, 468 insertions(+)
> >  create mode 100644 drivers/iio/chemical/sunrise_co2.c
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 90ca9df1d3c3..43f5bba46673 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -16544,6 +16544,12 @@ S:	Maintained
> >  F:	drivers/misc/phantom.c
> >  F:	include/uapi/linux/phantom.h
> >
> > +SENSEAIR SUNRISE 006-0-0007
> > +M:	Jacopo Mondi <jacopo@jmondi.org>
> > +S:	Maintained
> > +F:	Documentation/devicetree/bindings/iio/chemical/senseair,sunrise.yaml
> > +F:	drivers/iio/chemical/sunrise_co2.c
> > +
> >  SENSIRION SCD30 CARBON DIOXIDE SENSOR DRIVER
> >  M:	Tomasz Duszynski <tomasz.duszynski@octakon.com>
> >  S:	Maintained
> > diff --git a/drivers/iio/chemical/Kconfig b/drivers/iio/chemical/Kconfig
> > index 10bb431bc3ce..ee8562949226 100644
> > --- a/drivers/iio/chemical/Kconfig
> > +++ b/drivers/iio/chemical/Kconfig
> > @@ -144,6 +144,19 @@ config SPS30
> >  	  To compile this driver as a module, choose M here: the module will
> >  	  be called sps30.
> >
> > +config SENSEAIR_SUNRISE_CO2
> > +	tristate "Senseair Sunrise 006-0-0007 CO2 sensor"
> > +	depends on OF
> > +	depends on I2C
> > +	depends on SYSFS

Do you think I need this ? The driver includes iio/sysfs.h but I found
no symbol nor dependency for that

> > +	select REGMAP_I2C
> > +	help
> > +	  Say yes here to build support for Senseair Sunrise 006-0-0007 CO2
> > +	  sensor.
> > +
> > +	  To compile this driver as a module, choose M here: the
> > +	  module will be called sunrise_co2.
> > +
> >  config VZ89X
> >  	tristate "SGX Sensortech MiCS VZ89X VOC sensor"
> >  	depends on I2C
> > diff --git a/drivers/iio/chemical/Makefile b/drivers/iio/chemical/Makefile
> > index fef63dd5bf92..d5e2a3331d57 100644
> > --- a/drivers/iio/chemical/Makefile
> > +++ b/drivers/iio/chemical/Makefile
> > @@ -17,4 +17,5 @@ obj-$(CONFIG_SCD30_I2C) += scd30_i2c.o
> >  obj-$(CONFIG_SCD30_SERIAL) += scd30_serial.o
> >  obj-$(CONFIG_SENSIRION_SGP30)	+= sgp30.o
> >  obj-$(CONFIG_SPS30) += sps30.o
> > +obj-$(CONFIG_SENSEAIR_SUNRISE_CO2) += sunrise_co2.o
> >  obj-$(CONFIG_VZ89X)		+= vz89x.o
> > diff --git a/drivers/iio/chemical/sunrise_co2.c b/drivers/iio/chemical/sunrise_co2.c
> > new file mode 100644
> > index 000000000000..84f19df6fc00
> > --- /dev/null
> > +++ b/drivers/iio/chemical/sunrise_co2.c
> > @@ -0,0 +1,448 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Senseair Sunrise 006-0-0007 CO2 sensor driver.
> > + *
> > + * Copyright (C) 2021 Jacopo Mondi
> > + *
> > + * List of features not yet supported by the driver:
> > + * - controllable EN pin
> > + * - single-shot operations using the nDRY pin.
> > + * - ABC/target calibration
> > + */
> > +
> > +#include <linux/bitops.h>
> > +#include <linux/i2c.h>
> > +#include <linux/kernel.h>
> > +#include <linux/mod_devicetable.h>
> > +#include <linux/module.h>
> > +#include <linux/mutex.h>
> > +#include <linux/regmap.h>
> > +#include <linux/time64.h>
> > +
> > +#include <linux/iio/iio.h>
> > +#include <linux/iio/sysfs.h>
>
> > +#define DRIVER_NAME "sunrise"
>
> I would expect "sunrise_co2" here.
>

Yes, better as the driver has been updated

> Also, since it's one time use, please drop the definition completely and use
> literal in-place.
>

I got two usages
        ...
	iio_dev->name = DRIVER_NAME;

        ...
	.driver = {
		.name = DRIVER_NAME,

Is it ok to keep it ?

> > +#define SUNRISE_ERROR_STATUS_REG		0x00
> > +#define SUNRISE_CO2_FILTERED_COMP_REG		0x06
> > +#define SUNRISE_CHIP_TEMPERATURE_REG		0x08
> > +#define SUNRISE_CALIBRATION_STATUS_REG		0x81
> > +#define SUNRISE_CALIBRATION_COMMAND_REG		0x82
> > +#define SUNRISE_CALIBRATION_FACTORY_CMD		0x7c02
> > +#define SUNRISE_CALIBRATION_BACKGROUND_CMD	0x7c06
> > +/*
> > + * The calibration timeout is not characterized in the datasheet.
> > + * Use 30 seconds as a reasonable upper limit.
> > + */
> > +#define SUNRISE_CALIBRATION_TIMEOUT_US		(30 * USEC_PER_SEC)
> > +
> > +enum sunrise_calib {
> > +	SUNRISE_CALIBRATION_FACTORY,
> > +	SUNRISE_CALIBRATION_BACKGROUND,
> > +};
> > +
> > +struct sunrise_dev {
> > +	struct device *dev;
> > +	struct i2c_client *client;
> > +	struct regmap *regmap;
> > +	struct mutex lock;
> > +	enum sunrise_calib calibration;
> > +};
> > +
> > +static void sunrise_wakeup(struct sunrise_dev *sunrise)
> > +{
> > +	struct i2c_client *client = sunrise->client;
> > +
> > +	/*
> > +	 * Wake up sensor by sending sensor address: START, sensor address,
> > +	 * STOP. Sensor will not ACK this byte.
> > +	 *
> > +	 * The chip returns in low power state after 15msec without
> > +	 * communications or after a complete read/write sequence.
> > +	 */
> > +	i2c_smbus_xfer(client->adapter, client->addr, I2C_M_IGNORE_NAK,
> > +		       I2C_SMBUS_WRITE, 0, I2C_SMBUS_QUICK, NULL);
> > +}
> > +
> > +static int sunrise_read_word(struct sunrise_dev *sunrise, u8 reg, u16 *val)
> > +{
> > +	__be16 be_val;
> > +	int ret;
> > +
> > +	sunrise_wakeup(sunrise);
> > +	ret = regmap_bulk_read(sunrise->regmap, reg, &be_val, 2);
> > +	if (ret) {
> > +		dev_err(sunrise->dev, "Read word failed: reg 0x%2x (%d)\n", reg, ret);
> > +		return ret;
> > +	}
> > +
> > +	*val = be16_to_cpu(be_val);
> > +
> > +	return 0;
> > +}
> > +
> > +static int sunrise_write_byte(struct sunrise_dev *sunrise, u8 reg, u8 val)
> > +{
> > +	int ret;
> > +
> > +	sunrise_wakeup(sunrise);
> > +	ret = regmap_write(sunrise->regmap, reg, val);
> > +	if (ret) {
> > +		dev_err(sunrise->dev, "Write byte failed: reg 0x%2x (%d)\n", reg, ret);
> > +		return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int sunrise_write_word(struct sunrise_dev *sunrise, u8 reg, u16 data)
> > +{
> > +	__be16 be_data = cpu_to_be16(data);
> > +	int ret;
> > +
> > +	sunrise_wakeup(sunrise);
> > +	ret = regmap_bulk_write(sunrise->regmap, reg, &be_data, 2);
> > +	if (ret) {
> > +		dev_err(sunrise->dev, "Write word failed: reg 0x%2x (%d)\n", reg, ret);
> > +		return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +/*
> > + *  --- Calibration ---
> > + *
> > + *  Enumerate and select calibration modes, trigger a calibration cycle.
> > + */
> > +static const char * const sunrise_calibration_modes[] = {
> > +	[SUNRISE_CALIBRATION_FACTORY] = "factory_calibration",
> > +	[SUNRISE_CALIBRATION_BACKGROUND] = "background_calibration",
> > +};
> > +
> > +static const struct sunrise_calibration_data {
> > +	u16 calibration_cmd;
> > +	u8 calibration_bit;
> > +} sunrise_calibrations[] = {
> > +	[SUNRISE_CALIBRATION_FACTORY] = {
> > +		SUNRISE_CALIBRATION_FACTORY_CMD,
> > +		BIT(2),
> > +	},
> > +	[SUNRISE_CALIBRATION_BACKGROUND] = {
> > +		SUNRISE_CALIBRATION_BACKGROUND_CMD,
> > +		BIT(5),
> > +	},
> > +};
> > +
> > +static int sunrise_calibrate(struct sunrise_dev *sunrise)
> > +{
> > +	const struct sunrise_calibration_data *data;
> > +	unsigned int status;
> > +	int ret;
> > +
> > +	/* Reset the calibration status reg. */
> > +	ret = sunrise_write_byte(sunrise, SUNRISE_CALIBRATION_STATUS_REG, 0x00);
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* Write a calibration command and poll the calibration status bit. */
> > +	data = &sunrise_calibrations[sunrise->calibration];
> > +	ret = sunrise_write_word(sunrise, SUNRISE_CALIBRATION_COMMAND_REG,
> > +				 data->calibration_cmd);
> > +	if (ret)
> > +		return ret;
> > +
> > +	dev_dbg(sunrise->dev, "%s in progress\n",
> > +		sunrise_calibration_modes[sunrise->calibration]);
> > +
> > +	return regmap_read_poll_timeout(sunrise->regmap,
> > +					SUNRISE_CALIBRATION_STATUS_REG,
> > +					status, status & data->calibration_bit,
> > +					100, SUNRISE_CALIBRATION_TIMEOUT_US);
> > +}
> > +
> > +static ssize_t sunrise_calibration_write(struct iio_dev *iiodev,
> > +					 uintptr_t private,
> > +					 const struct iio_chan_spec *chan,
> > +					 const char *buf, size_t len)
> > +{
> > +	struct sunrise_dev *sunrise = iio_priv(iiodev);
> > +	bool calibrate;
> > +	int ret;
> > +
> > +	ret = kstrtobool(buf, &calibrate);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (!calibrate)
> > +		return 0;
> > +
> > +	ret = sunrise_calibrate(sunrise);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return len;
> > +}
> > +
> > +static int sunrise_set_calibration_mode(struct iio_dev *iiodev,
> > +					const struct iio_chan_spec *chan,
> > +					unsigned int mode)
> > +{
> > +	struct sunrise_dev *sunrise = iio_priv(iiodev);
> > +
> > +	mutex_lock(&sunrise->lock);
> > +	sunrise->calibration = mode;
> > +	mutex_unlock(&sunrise->lock);
> > +
> > +	return 0;
> > +}
> > +
> > +static int sunrise_get_calibration_mode(struct iio_dev *iiodev,
> > +					const struct iio_chan_spec *chan)
> > +{
> > +	struct sunrise_dev *sunrise = iio_priv(iiodev);
> > +	int mode;
> > +
> > +	mutex_lock(&sunrise->lock);
> > +	mode = sunrise->calibration;
> > +	mutex_unlock(&sunrise->lock);
> > +
> > +	return mode;
> > +}
> > +
> > +static const struct iio_enum sunrise_calibration_modes_enum = {
> > +	.items = sunrise_calibration_modes,
> > +	.num_items = ARRAY_SIZE(sunrise_calibration_modes),
> > +	.set = sunrise_set_calibration_mode,
> > +	.get = sunrise_get_calibration_mode,
> > +};
> > +
> > +/* --- Error status---
> > + *
> > + * Enumerate and retrieve the chip error status.
> > + */
> > +enum {
> > +	SUNRISE_ERROR_FATAL,
> > +	SUNRISE_ERROR_I2C,
> > +	SUNRISE_ERROR_ALGORITHM,
> > +	SUNRISE_ERROR_CALIBRATION,
> > +	SUNRISE_ERROR_SELF_DIAGNOSTIC,
> > +	SUNRISE_ERROR_OUT_OF_RANGE,
> > +	SUNRISE_ERROR_MEMORY,
> > +	SUNRISE_ERROR_NO_MEASUREMENT,
> > +	SUNRISE_ERROR_LOW_VOLTAGE,
> > +	SUNRISE_ERROR_MEASUREMENT_TIMEOUT,
> > +};
> > +
> > +static const char * const sunrise_error_statuses[] = {
> > +	[SUNRISE_ERROR_FATAL] = "error_fatal",
> > +	[SUNRISE_ERROR_I2C] = "error_i2c",
> > +	[SUNRISE_ERROR_ALGORITHM] = "error_algorithm",
> > +	[SUNRISE_ERROR_CALIBRATION] = "error_calibration",
> > +	[SUNRISE_ERROR_SELF_DIAGNOSTIC] = "error_self_diagnostic",
> > +	[SUNRISE_ERROR_OUT_OF_RANGE] = "error_out_of_range",
> > +	[SUNRISE_ERROR_MEMORY] = "error_memory",
> > +	[SUNRISE_ERROR_NO_MEASUREMENT] = "error_no_measurement",
> > +	[SUNRISE_ERROR_LOW_VOLTAGE] = "error_low_voltage",
> > +	[SUNRISE_ERROR_MEASUREMENT_TIMEOUT] = "error_measurement_timeout",
> > +};
> > +
> > +static const u8 error_codes[] = {
> > +	SUNRISE_ERROR_FATAL,
> > +	SUNRISE_ERROR_I2C,
> > +	SUNRISE_ERROR_ALGORITHM,
> > +	SUNRISE_ERROR_CALIBRATION,
> > +	SUNRISE_ERROR_SELF_DIAGNOSTIC,
> > +	SUNRISE_ERROR_OUT_OF_RANGE,
> > +	SUNRISE_ERROR_MEMORY,
> > +	SUNRISE_ERROR_NO_MEASUREMENT,
> > +	SUNRISE_ERROR_LOW_VOLTAGE,
> > +	SUNRISE_ERROR_MEASUREMENT_TIMEOUT,
> > +};
> > +
> > +static const struct iio_enum sunrise_error_statuses_enum = {
> > +	.items = sunrise_error_statuses,
> > +	.num_items = ARRAY_SIZE(sunrise_error_statuses),
> > +};
> > +
> > +static ssize_t sunrise_error_status_read(struct iio_dev *iiodev,
> > +					 uintptr_t private,
> > +					 const struct iio_chan_spec *chan,
> > +					 char *buf)
> > +{
> > +	struct sunrise_dev *sunrise = iio_priv(iiodev);
> > +	const unsigned long *errors;
> > +	ssize_t len = 0;
> > +	u16 value;
> > +	int ret;
> > +	u8 i;
> > +
> > +	ret = sunrise_read_word(sunrise, SUNRISE_ERROR_STATUS_REG, &value);
> > +	if (ret)
> > +		return -EINVAL;
> > +
> > +	errors = (const unsigned long *)&value;
>
> Yes, it takes a pointer, but in your case it will oops the kernel quite likely.

The usage of a pointer kind of puzzled me in first place, and I found
no pattern to copy in the existing code base. I have probbaly not
looked hard enough ?

>
> 	unsigned long errors;
>
> 	...
>
> 	errors = value;
> 	for_each_set_bit(..., &errors, ...)

I can do so but, for my education mostly, why do you think it would
oops ? '*errors' is a pointer to a variable allocated on the stack as
much as 'errors' you suggested is a local stack variable. I might be a
bit slow today, but I'm missing the difference. FWIW I tested the
function, even if there were no error bits set at the moment I tested, but
the for_each_set_bit() macro was indeed run.

>
> > +	for_each_set_bit(i, errors, ARRAY_SIZE(error_codes))
> > +		len += sysfs_emit_at(buf, len, "%s ", sunrise_error_statuses[i]);
> > +
> > +	if (len)
> > +		buf[len - 1] = '\n';
> > +
> > +	return len;
> > +}
> > +
> > +static const struct iio_chan_spec_ext_info sunrise_concentration_ext_info[] = {
> > +	/* Calibration modes and calibration trigger. */
> > +	{
> > +		.name = "calibration",
> > +		.write = sunrise_calibration_write,
> > +		.shared = IIO_SEPARATE,
> > +	},
> > +	IIO_ENUM("calibration_mode", IIO_SEPARATE,
> > +		 &sunrise_calibration_modes_enum),
> > +	IIO_ENUM_AVAILABLE("calibration_mode",
> > +			   &sunrise_calibration_modes_enum),
> > +
> > +	/* Error statuses. */
> > +	{
> > +		.name = "error_status",
> > +		.read = sunrise_error_status_read,
> > +		.shared = IIO_SEPARATE
> > +	},
> > +	IIO_ENUM_AVAILABLE("error_status", &sunrise_error_statuses_enum),
> > +	{}
> > +};
> > +
> > +static const struct iio_chan_spec sunrise_channels[] = {
> > +	{
> > +		.type = IIO_CONCENTRATION,
> > +		.modified = 1,
> > +		.channel2 = IIO_MOD_CO2,
> > +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> > +		.ext_info = sunrise_concentration_ext_info,
> > +		.scan_index = 0,
> > +		.scan_type =  {
> > +			.sign = 's',
> > +			.realbits = 16,
> > +			.storagebits = 16,
> > +			.endianness = IIO_CPU,
> > +		},
> > +	},
> > +	{
> > +		.type = IIO_TEMP,
> > +		.info_mask_separate =  BIT(IIO_CHAN_INFO_RAW) |
> > +				       BIT(IIO_CHAN_INFO_SCALE),
> > +		.scan_index = 1,
> > +		.scan_type =  {
> > +			.sign = 's',
> > +			.realbits = 16,
> > +			.storagebits = 16,
> > +			.endianness = IIO_CPU,
> > +		},
> > +	},
> > +};
> > +
> > +static int sunrise_read_raw(struct iio_dev *iio_dev,
> > +			    const struct iio_chan_spec *chan,
> > +			    int *val, int *val2, long mask)
> > +{
> > +	struct sunrise_dev *sunrise = iio_priv(iio_dev);
> > +	u16 value;
> > +	int ret;
> > +
> > +	switch (mask) {
> > +	case IIO_CHAN_INFO_RAW:
> > +		mutex_lock(&sunrise->lock);
> > +
> > +		switch (chan->type) {
> > +		case IIO_CONCENTRATION:
> > +			ret = sunrise_read_word(sunrise, SUNRISE_CO2_FILTERED_COMP_REG,
> > +						&value);
> > +			*val = value;
> > +			mutex_unlock(&sunrise->lock);
> > +
> > +			return ret ?: IIO_VAL_INT;
> > +
> > +		case IIO_TEMP:
> > +			ret = sunrise_read_word(sunrise, SUNRISE_CHIP_TEMPERATURE_REG,
> > +						&value);
> > +			*val = value;
> > +			mutex_unlock(&sunrise->lock);
> > +
> > +			return ret ?: IIO_VAL_INT;
> > +
> > +		default:
> > +			mutex_unlock(&sunrise->lock);
> > +			return -EINVAL;
> > +		}
> > +
> > +	case IIO_CHAN_INFO_SCALE:
> > +		/* Chip temperature scale = 1/100 */
> > +		*val = 1;
> > +		*val2 = 100;
> > +		return IIO_VAL_FRACTIONAL;
> > +
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +}
> > +
> > +static const struct iio_info sunrise_info = {
> > +	.read_raw = sunrise_read_raw,
> > +};
> > +
> > +static struct regmap_config sunrise_regmap_config = {
> > +	.reg_bits = 8,
> > +	.val_bits = 8,
>
> Does it need a lock?
> (I think it does, but I would like to confirm)
>

You know, I had the same doubt, but the description of a few fields of
struct regmap_config lead me to think there's a locking mechanism
already inplace

 * @disable_locking: This regmap is either protected by external means or
 *                   is guaranteed not to be accessed from multiple threads.
 *                   Don't use any locking mechanisms.
 * @lock:	  Optional lock callback (overrides regmap's default lock
 *		  function, based on spinlock or mutex).

As you can see 'lock' is option and is said to override regmap's default
lock functions. Locking seems to have be disabled explicitely
through 'disable_locking' too. I was then expecting a reference to a
locally declared mutex/spinlock to be passed through regmap_config
if the regmap's locking mechanism requires driver-allocated locking
primitives. This suggests to me there's an internal locking.

In facts, regmap's core seems to have an internal mutex and a built-in
locking mechanism, as shown by __regmap_init(), which selects the
opportune locking primitive according to how regmap_config is
initialized. In our case it seems it relies on the usage of the
regmap_lock_mutex() and regmap_unlock_mutex() functions, which use
struct regmap.mutex.

I think we're then safe locking-wise, do you agree ?

That said, as I'm not pushing to have the driver accepted for this
merge window, for which we might be late already, I would wait for
comments on the usage of the ext_channel abstraction from IIO people
before submitting a new version if that's fine with everyone.

Thanks again for the quick comments!


> > +};
> > +
> > +static int sunrise_probe(struct i2c_client *client)
> > +{
> > +	struct sunrise_dev *sunrise;
> > +	struct iio_dev *iio_dev;
> > +
> > +	iio_dev = devm_iio_device_alloc(&client->dev, sizeof(*sunrise));
> > +	if (!iio_dev)
> > +		return -ENOMEM;
> > +
> > +	i2c_set_clientdata(client, iio_dev);
> > +
> > +	sunrise = iio_priv(iio_dev);
> > +	sunrise->client = client;
> > +	sunrise->dev = &client->dev;
> > +	mutex_init(&sunrise->lock);
> > +
> > +	sunrise->regmap = devm_regmap_init_i2c(client, &sunrise_regmap_config);
> > +	if (IS_ERR(sunrise->regmap)) {
> > +		dev_err(&client->dev, "Failed to initialize regmap\n");
> > +		return PTR_ERR(sunrise->regmap);
> > +	}
> > +
> > +	iio_dev->info = &sunrise_info;
> > +	iio_dev->name = DRIVER_NAME;
> > +	iio_dev->channels = sunrise_channels;
> > +	iio_dev->num_channels = ARRAY_SIZE(sunrise_channels);
> > +	iio_dev->modes = INDIO_DIRECT_MODE;
> > +
> > +	return devm_iio_device_register(&client->dev, iio_dev);
> > +}
> > +
> > +static const struct of_device_id sunrise_of_match[] = {
> > +	{ .compatible = "senseair,sunrise-006-0-0007" },
> > +	{}
> > +};
> > +MODULE_DEVICE_TABLE(of, sunrise_of_match);
> > +
> > +static struct i2c_driver sunrise_driver = {
> > +	.driver = {
> > +		.name = DRIVER_NAME,
> > +		.of_match_table = sunrise_of_match,
> > +	},
> > +	.probe_new = sunrise_probe,
> > +};
> > +module_i2c_driver(sunrise_driver);
> > +
> > +MODULE_AUTHOR("Jacopo Mondi <jacopo@jmondi.org>");
> > +MODULE_DESCRIPTION("Senseair Sunrise 006-0-0007 CO2 sensor IIO driver");
> > +MODULE_LICENSE("GPL v2");
> > --
> > 2.32.0
> >
>
> --
> With Best Regards,
> Andy Shevchenko
>
>

Re: [PATCH v3.1 2/3] iio: chemical: Add Senseair Sunrise 006-0-007 driver

[Andy Shevchenko]

On Mon, Aug 23, 2021 at 11:06:10AM +0200, Jacopo Mondi wrote:
> On Mon, Aug 23, 2021 at 11:35:23AM +0300, Andy Shevchenko wrote:
> > On Mon, Aug 23, 2021 at 09:36:39AM +0200, Jacopo Mondi wrote:

...

> > Thanks for this intermediate update, looks much better.
> > So, there are a few comments below and we are almost ready.
> 
> Thanks, I would also like feedback on the usage of channel's
> ext_info to handle calibration instead of using raw attributes as
> suggested by Matt

Better to wait for Jonathan.

...

> > > +	depends on SYSFS
> 
> Do you think I need this ? The driver includes iio/sysfs.h but I found
> no symbol nor dependency for that

Ditto.

...

> > Also, since it's one time use, please drop the definition completely and use
> > literal in-place.

> I got two usages
>         ...
> 	iio_dev->name = DRIVER_NAME;
> 
>         ...
> 	.driver = {
> 		.name = DRIVER_NAME,
> 
> Is it ok to keep it ?

Ah, okay then!

...

> > > +	errors = (const unsigned long *)&value;
> >
> > Yes, it takes a pointer, but in your case it will oops the kernel quite likely.
> 
> The usage of a pointer kind of puzzled me in first place, and I found
> no pattern to copy in the existing code base. I have probbaly not
> looked hard enough ?
> 
> > 	unsigned long errors;
> >
> > 	...
> >
> > 	errors = value;
> > 	for_each_set_bit(..., &errors, ...)
> 
> I can do so but, for my education mostly, why do you think it would
> oops ? '*errors' is a pointer to a variable allocated on the stack as
> much as 'errors' you suggested is a local stack variable. I might be a
> bit slow today, but I'm missing the difference. FWIW I tested the
> function, even if there were no error bits set at the moment I tested, but
> the for_each_set_bit() macro was indeed run.

Because you theoretically access bytes behind 16-bit. That castings just ugly
and compiler should warn you about if it is clever enough.

If you found it in the existing code, please, fix it there, so quite bad
pattern won't spread.

> > > +	for_each_set_bit(i, errors, ARRAY_SIZE(error_codes))
> > > +		len += sysfs_emit_at(buf, len, "%s ", sunrise_error_statuses[i]);

...

> > > +static struct regmap_config sunrise_regmap_config = {
> > > +	.reg_bits = 8,
> > > +	.val_bits = 8,
> >
> > Does it need a lock?
> > (I think it does, but I would like to confirm)
> 
> You know, I had the same doubt, but the description of a few fields of
> struct regmap_config lead me to think there's a locking mechanism
> already inplace

Exactly! But see below.

>  * @disable_locking: This regmap is either protected by external means or
>  *                   is guaranteed not to be accessed from multiple threads.
>  *                   Don't use any locking mechanisms.
>  * @lock:	  Optional lock callback (overrides regmap's default lock
>  *		  function, based on spinlock or mutex).
> 
> As you can see 'lock' is option and is said to override regmap's default
> lock functions. Locking seems to have be disabled explicitely
> through 'disable_locking' too. I was then expecting a reference to a
> locally declared mutex/spinlock to be passed through regmap_config
> if the regmap's locking mechanism requires driver-allocated locking
> primitives. This suggests to me there's an internal locking.
> 
> In facts, regmap's core seems to have an internal mutex and a built-in
> locking mechanism, as shown by __regmap_init(), which selects the
> opportune locking primitive according to how regmap_config is
> initialized. In our case it seems it relies on the usage of the
> regmap_lock_mutex() and regmap_unlock_mutex() functions, which use
> struct regmap.mutex.
> 
> I think we're then safe locking-wise, do you agree ?

My point is do you need regmap locking mechanism to be used or not?

> That said, as I'm not pushing to have the driver accepted for this
> merge window, for which we might be late already, I would wait for
> comments on the usage of the ext_channel abstraction from IIO people
> before submitting a new version if that's fine with everyone.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3.1 2/3] iio: chemical: Add Senseair Sunrise 006-0-007 driver

[Jacopo Mondi]

Hello,

On Mon, Aug 23, 2021 at 12:40:00PM +0300, Andy Shevchenko wrote:
> On Mon, Aug 23, 2021 at 11:06:10AM +0200, Jacopo Mondi wrote:
> > On Mon, Aug 23, 2021 at 11:35:23AM +0300, Andy Shevchenko wrote:
> > > On Mon, Aug 23, 2021 at 09:36:39AM +0200, Jacopo Mondi wrote:
>
> ...
>
> > > Thanks for this intermediate update, looks much better.
> > > So, there are a few comments below and we are almost ready.
> >
> > Thanks, I would also like feedback on the usage of channel's
> > ext_info to handle calibration instead of using raw attributes as
> > suggested by Matt
>
> Better to wait for Jonathan.
>
> ...
>
> > > > +	depends on SYSFS
> >
> > Do you think I need this ? The driver includes iio/sysfs.h but I found
> > no symbol nor dependency for that
>
> Ditto.
>
> ...
>
> > > Also, since it's one time use, please drop the definition completely and use
> > > literal in-place.
>
> > I got two usages
> >         ...
> > 	iio_dev->name = DRIVER_NAME;
> >
> >         ...
> > 	.driver = {
> > 		.name = DRIVER_NAME,
> >
> > Is it ok to keep it ?
>
> Ah, okay then!
>
> ...
>
> > > > +	errors = (const unsigned long *)&value;
> > >
> > > Yes, it takes a pointer, but in your case it will oops the kernel quite likely.
> >
> > The usage of a pointer kind of puzzled me in first place, and I found
> > no pattern to copy in the existing code base. I have probbaly not
> > looked hard enough ?
> >
> > > 	unsigned long errors;
> > >
> > > 	...
> > >
> > > 	errors = value;
> > > 	for_each_set_bit(..., &errors, ...)
> >
> > I can do so but, for my education mostly, why do you think it would
> > oops ? '*errors' is a pointer to a variable allocated on the stack as
> > much as 'errors' you suggested is a local stack variable. I might be a
> > bit slow today, but I'm missing the difference. FWIW I tested the
> > function, even if there were no error bits set at the moment I tested, but
> > the for_each_set_bit() macro was indeed run.
>
> Because you theoretically access bytes behind 16-bit. That castings just ugly
> and compiler should warn you about if it is clever enough.

I don't think there's such a risk as I limit the search space to
ARRAY_SIZE(error_codes), but I agree the cast is ugly and I'll change
to what you suggested

>
> If you found it in the existing code, please, fix it there, so quite bad
> pattern won't spread.
>

My point was that I was not able to find any pattern to copy from :)

> > > > +	for_each_set_bit(i, errors, ARRAY_SIZE(error_codes))
> > > > +		len += sysfs_emit_at(buf, len, "%s ", sunrise_error_statuses[i]);
>
> ...
>
> > > > +static struct regmap_config sunrise_regmap_config = {
> > > > +	.reg_bits = 8,
> > > > +	.val_bits = 8,
> > >
> > > Does it need a lock?
> > > (I think it does, but I would like to confirm)
> >
> > You know, I had the same doubt, but the description of a few fields of
> > struct regmap_config lead me to think there's a locking mechanism
> > already inplace
>
> Exactly! But see below.
>
> >  * @disable_locking: This regmap is either protected by external means or
> >  *                   is guaranteed not to be accessed from multiple threads.
> >  *                   Don't use any locking mechanisms.
> >  * @lock:	  Optional lock callback (overrides regmap's default lock
> >  *		  function, based on spinlock or mutex).
> >
> > As you can see 'lock' is option and is said to override regmap's default
> > lock functions. Locking seems to have be disabled explicitely
> > through 'disable_locking' too. I was then expecting a reference to a
> > locally declared mutex/spinlock to be passed through regmap_config
> > if the regmap's locking mechanism requires driver-allocated locking
> > primitives. This suggests to me there's an internal locking.
> >
> > In facts, regmap's core seems to have an internal mutex and a built-in
> > locking mechanism, as shown by __regmap_init(), which selects the
> > opportune locking primitive according to how regmap_config is
> > initialized. In our case it seems it relies on the usage of the
> > regmap_lock_mutex() and regmap_unlock_mutex() functions, which use
> > struct regmap.mutex.
> >
> > I think we're then safe locking-wise, do you agree ?
>
> My point is do you need regmap locking mechanism to be used or not?
>

Oh I see, sorry for the useless digression, you were asking if I
should not disable locking, not the other way around!

Anyway, there's a risk for a concurrent read/write when in example
reading the error status and triggering a calibration. There's no
locking at the driver level in those functions as they do not access
shared driver's fields, but on the wire there might be conflicting
transactions, so I think I should keep locking in place.

Thanks
   j

> > That said, as I'm not pushing to have the driver accepted for this
> > merge window, for which we might be late already, I would wait for
> > comments on the usage of the ext_channel abstraction from IIO people
> > before submitting a new version if that's fine with everyone.
>
> --
> With Best Regards,
> Andy Shevchenko
>
>

Re: [PATCH v3.1 2/3] iio: chemical: Add Senseair Sunrise 006-0-007 driver

[Andy Shevchenko]

On Mon, Aug 23, 2021 at 1:20 PM Jacopo Mondi <jacopo@jmondi.org> wrote:
> On Mon, Aug 23, 2021 at 12:40:00PM +0300, Andy Shevchenko wrote:
> > On Mon, Aug 23, 2021 at 11:06:10AM +0200, Jacopo Mondi wrote:
> > > On Mon, Aug 23, 2021 at 11:35:23AM +0300, Andy Shevchenko wrote:
> > > > On Mon, Aug 23, 2021 at 09:36:39AM +0200, Jacopo Mondi wrote:

...

> > > > > +       errors = (const unsigned long *)&value;
> > > >
> > > > Yes, it takes a pointer, but in your case it will oops the kernel quite likely.
> > >
> > > The usage of a pointer kind of puzzled me in first place, and I found
> > > no pattern to copy in the existing code base. I have probbaly not
> > > looked hard enough ?
> > >
> > > >   unsigned long errors;
> > > >
> > > >   ...
> > > >
> > > >   errors = value;
> > > >   for_each_set_bit(..., &errors, ...)
> > >
> > > I can do so but, for my education mostly, why do you think it would
> > > oops ? '*errors' is a pointer to a variable allocated on the stack as
> > > much as 'errors' you suggested is a local stack variable. I might be a
> > > bit slow today, but I'm missing the difference. FWIW I tested the
> > > function, even if there were no error bits set at the moment I tested, but
> > > the for_each_set_bit() macro was indeed run.
> >
> > Because you theoretically access bytes behind 16-bit. That castings just ugly
> > and compiler should warn you about if it is clever enough.
>
> I don't think there's such a risk as I limit the search space to
> ARRAY_SIZE(error_codes), but I agree the cast is ugly and I'll change
> to what you suggested

More for the sake of education. Actually it's not theoretical. Some
architectures may not access longs on unaligned (16-bit) addresses.
So, yes, oops is probable.
Another example is reading the long to the register. This can cross
the page boundary and produce fault (again) oops.

> > If you found it in the existing code, please, fix it there, so quite bad
> > pattern won't spread.
>
> My point was that I was not able to find any pattern to copy from :)
>
> > > > > +       for_each_set_bit(i, errors, ARRAY_SIZE(error_codes))
> > > > > +               len += sysfs_emit_at(buf, len, "%s ", sunrise_error_statuses[i]);

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3.1 2/3] iio: chemical: Add Senseair Sunrise 006-0-007 driver

[Jonathan Cameron]

On Mon, 23 Aug 2021 14:09:21 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Mon, Aug 23, 2021 at 1:20 PM Jacopo Mondi <jacopo@jmondi.org> wrote:
> > On Mon, Aug 23, 2021 at 12:40:00PM +0300, Andy Shevchenko wrote:  
> > > On Mon, Aug 23, 2021 at 11:06:10AM +0200, Jacopo Mondi wrote:  
> > > > On Mon, Aug 23, 2021 at 11:35:23AM +0300, Andy Shevchenko wrote:  
> > > > > On Mon, Aug 23, 2021 at 09:36:39AM +0200, Jacopo Mondi wrote:  
> 
> ...
> 
> > > > > > +       errors = (const unsigned long *)&value;  
> > > > >
> > > > > Yes, it takes a pointer, but in your case it will oops the kernel quite likely.  
> > > >
> > > > The usage of a pointer kind of puzzled me in first place, and I found
> > > > no pattern to copy in the existing code base. I have probbaly not
> > > > looked hard enough ?
> > > >  
> > > > >   unsigned long errors;
> > > > >
> > > > >   ...
> > > > >
> > > > >   errors = value;
> > > > >   for_each_set_bit(..., &errors, ...)  
> > > >
> > > > I can do so but, for my education mostly, why do you think it would
> > > > oops ? '*errors' is a pointer to a variable allocated on the stack as
> > > > much as 'errors' you suggested is a local stack variable. I might be a
> > > > bit slow today, but I'm missing the difference. FWIW I tested the
> > > > function, even if there were no error bits set at the moment I tested, but
> > > > the for_each_set_bit() macro was indeed run.  
> > >
> > > Because you theoretically access bytes behind 16-bit. That castings just ugly
> > > and compiler should warn you about if it is clever enough.  
> >
> > I don't think there's such a risk as I limit the search space to
> > ARRAY_SIZE(error_codes), but I agree the cast is ugly and I'll change
> > to what you suggested  
> 
> More for the sake of education. Actually it's not theoretical. Some
> architectures may not access longs on unaligned (16-bit) addresses.
> So, yes, oops is probable.
> Another example is reading the long to the register. This can cross
> the page boundary and produce fault (again) oops.

For extra giggles, (and I'm half asleep today so may have this backwards)
what it the platform is big endian and you do this?  I'm fairly sure it will
walk the bits from low to high and so the first access will be off the end
of your shorter type being as it will be at addr + sizeof (long) - 1 bit 7.

> 
> > > If you found it in the existing code, please, fix it there, so quite bad
> > > pattern won't spread.  
> >
> > My point was that I was not able to find any pattern to copy from :)
> >  
> > > > > > +       for_each_set_bit(i, errors, ARRAY_SIZE(error_codes))
> > > > > > +               len += sysfs_emit_at(buf, len, "%s ", sunrise_error_statuses[i]);  
> 

Re: [PATCH v3.1 2/3] iio: chemical: Add Senseair Sunrise 006-0-007 driver

[Andy Shevchenko]

On Sun, Aug 29, 2021 at 7:18 PM Jonathan Cameron <jic23@kernel.org> wrote:
> On Mon, 23 Aug 2021 14:09:21 +0300
> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > On Mon, Aug 23, 2021 at 1:20 PM Jacopo Mondi <jacopo@jmondi.org> wrote:
> > > On Mon, Aug 23, 2021 at 12:40:00PM +0300, Andy Shevchenko wrote:
> > > > On Mon, Aug 23, 2021 at 11:06:10AM +0200, Jacopo Mondi wrote:
> > > > > On Mon, Aug 23, 2021 at 11:35:23AM +0300, Andy Shevchenko wrote:
> > > > > > On Mon, Aug 23, 2021 at 09:36:39AM +0200, Jacopo Mondi wrote:

...

> > > > > > > +       errors = (const unsigned long *)&value;
> > > > > >
> > > > > > Yes, it takes a pointer, but in your case it will oops the kernel quite likely.
> > > > >
> > > > > The usage of a pointer kind of puzzled me in first place, and I found
> > > > > no pattern to copy in the existing code base. I have probbaly not
> > > > > looked hard enough ?
> > > > >
> > > > > >   unsigned long errors;
> > > > > >
> > > > > >   ...
> > > > > >
> > > > > >   errors = value;
> > > > > >   for_each_set_bit(..., &errors, ...)
> > > > >
> > > > > I can do so but, for my education mostly, why do you think it would
> > > > > oops ? '*errors' is a pointer to a variable allocated on the stack as
> > > > > much as 'errors' you suggested is a local stack variable. I might be a
> > > > > bit slow today, but I'm missing the difference. FWIW I tested the
> > > > > function, even if there were no error bits set at the moment I tested, but
> > > > > the for_each_set_bit() macro was indeed run.
> > > >
> > > > Because you theoretically access bytes behind 16-bit. That castings just ugly
> > > > and compiler should warn you about if it is clever enough.
> > >
> > > I don't think there's such a risk as I limit the search space to
> > > ARRAY_SIZE(error_codes), but I agree the cast is ugly and I'll change
> > > to what you suggested
> >
> > More for the sake of education. Actually it's not theoretical. Some
> > architectures may not access longs on unaligned (16-bit) addresses.
> > So, yes, oops is probable.
> > Another example is reading the long to the register. This can cross
> > the page boundary and produce fault (again) oops.
>
> For extra giggles, (and I'm half asleep today so may have this backwards)
> what it the platform is big endian and you do this?  I'm fairly sure it will
> walk the bits from low to high and so the first access will be off the end
> of your shorter type being as it will be at addr + sizeof (long) - 1 bit 7.

Thanks, I forgot to think about this case.
Maybe being half asleep is not a bad thing after all? :-)

> > > > If you found it in the existing code, please, fix it there, so quite bad
> > > > pattern won't spread.
> > >
> > > My point was that I was not able to find any pattern to copy from :)
> > >
> > > > > > > +       for_each_set_bit(i, errors, ARRAY_SIZE(error_codes))
> > > > > > > +               len += sysfs_emit_at(buf, len, "%s ", sunrise_error_statuses[i]);


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3.1 2/3] iio: chemical: Add Senseair Sunrise 006-0-007 driver

[Jonathan Cameron]

On Mon, 23 Aug 2021 09:36:39 +0200
Jacopo Mondi <jacopo@jmondi.org> wrote:

> Add support for the Senseair Sunrise 006-0-0007 driver through the
> IIO subsystem.
> 
> Datasheet: https://rmtplusstoragesenseair.blob.core.windows.net/docs/Dev/publicerat/TDE5531.pdf
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
> v3->v3.1

Always do a new version of the whole series.  The automated tools that maintainers
mostly use these days (e.g. b4) are pointed out a whole series when picking it up.

This means we have to do it manually, one patch at a time I think which is annoying.


> - Remove debug leftover
> - Re-add commas at the end of arrays declarations
> ---
>  MAINTAINERS                        |   6 +
>  drivers/iio/chemical/Kconfig       |  13 +
>  drivers/iio/chemical/Makefile      |   1 +
>  drivers/iio/chemical/sunrise_co2.c | 448 +++++++++++++++++++++++++++++
>  4 files changed, 468 insertions(+)
>  create mode 100644 drivers/iio/chemical/sunrise_co2.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 90ca9df1d3c3..43f5bba46673 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -16544,6 +16544,12 @@ S:	Maintained
>  F:	drivers/misc/phantom.c
>  F:	include/uapi/linux/phantom.h
> 
> +SENSEAIR SUNRISE 006-0-0007
> +M:	Jacopo Mondi <jacopo@jmondi.org>
> +S:	Maintained
> +F:	Documentation/devicetree/bindings/iio/chemical/senseair,sunrise.yaml
> +F:	drivers/iio/chemical/sunrise_co2.c
> +
>  SENSIRION SCD30 CARBON DIOXIDE SENSOR DRIVER
>  M:	Tomasz Duszynski <tomasz.duszynski@octakon.com>
>  S:	Maintained
> diff --git a/drivers/iio/chemical/Kconfig b/drivers/iio/chemical/Kconfig
> index 10bb431bc3ce..ee8562949226 100644
> --- a/drivers/iio/chemical/Kconfig
> +++ b/drivers/iio/chemical/Kconfig
> @@ -144,6 +144,19 @@ config SPS30
>  	  To compile this driver as a module, choose M here: the module will
>  	  be called sps30.
> 
> +config SENSEAIR_SUNRISE_CO2
> +	tristate "Senseair Sunrise 006-0-0007 CO2 sensor"
> +	depends on OF

Not needed.

> +	depends on I2C

regmap_i2c select should bring that in.

> +	depends on SYSFS

I'd be surprised if this necessary...   Everything should be stubbed appropriately if
its' not there.
> +	select REGMAP_I2C
> +	help
> +	  Say yes here to build support for Senseair Sunrise 006-0-0007 CO2
> +	  sensor.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called sunrise_co2.
> +
>  config VZ89X
>  	tristate "SGX Sensortech MiCS VZ89X VOC sensor"
>  	depends on I2C
> diff --git a/drivers/iio/chemical/Makefile b/drivers/iio/chemical/Makefile
> index fef63dd5bf92..d5e2a3331d57 100644
> --- a/drivers/iio/chemical/Makefile
> +++ b/drivers/iio/chemical/Makefile
> @@ -17,4 +17,5 @@ obj-$(CONFIG_SCD30_I2C) += scd30_i2c.o
>  obj-$(CONFIG_SCD30_SERIAL) += scd30_serial.o
>  obj-$(CONFIG_SENSIRION_SGP30)	+= sgp30.o
>  obj-$(CONFIG_SPS30) += sps30.o
> +obj-$(CONFIG_SENSEAIR_SUNRISE_CO2) += sunrise_co2.o
>  obj-$(CONFIG_VZ89X)		+= vz89x.o
> diff --git a/drivers/iio/chemical/sunrise_co2.c b/drivers/iio/chemical/sunrise_co2.c
> new file mode 100644
> index 000000000000..84f19df6fc00
> --- /dev/null
> +++ b/drivers/iio/chemical/sunrise_co2.c
> @@ -0,0 +1,448 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Senseair Sunrise 006-0-0007 CO2 sensor driver.
> + *
> + * Copyright (C) 2021 Jacopo Mondi
> + *
> + * List of features not yet supported by the driver:
> + * - controllable EN pin
> + * - single-shot operations using the nDRY pin.
> + * - ABC/target calibration
> + */
> +
> +#include <linux/bitops.h>
> +#include <linux/i2c.h>
> +#include <linux/kernel.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/regmap.h>
> +#include <linux/time64.h>
> +
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>

What are you using from this header?

> +
> +#define DRIVER_NAME "sunrise"
> +
> +#define SUNRISE_ERROR_STATUS_REG		0x00
> +#define SUNRISE_CO2_FILTERED_COMP_REG		0x06
> +#define SUNRISE_CHIP_TEMPERATURE_REG		0x08
> +#define SUNRISE_CALIBRATION_STATUS_REG		0x81
> +#define SUNRISE_CALIBRATION_COMMAND_REG		0x82
> +#define SUNRISE_CALIBRATION_FACTORY_CMD		0x7c02
> +#define SUNRISE_CALIBRATION_BACKGROUND_CMD	0x7c06
> +/*
> + * The calibration timeout is not characterized in the datasheet.
> + * Use 30 seconds as a reasonable upper limit.
> + */
> +#define SUNRISE_CALIBRATION_TIMEOUT_US		(30 * USEC_PER_SEC)
> +
> +enum sunrise_calib {
> +	SUNRISE_CALIBRATION_FACTORY,
> +	SUNRISE_CALIBRATION_BACKGROUND,
> +};
> +
> +struct sunrise_dev {
> +	struct device *dev;
> +	struct i2c_client *client;
> +	struct regmap *regmap;
> +	struct mutex lock;
> +	enum sunrise_calib calibration;
> +};
> +
> +static void sunrise_wakeup(struct sunrise_dev *sunrise)
> +{
> +	struct i2c_client *client = sunrise->client;
> +
> +	/*
> +	 * Wake up sensor by sending sensor address: START, sensor address,
> +	 * STOP. Sensor will not ACK this byte.
> +	 *
> +	 * The chip returns in low power state after 15msec without
> +	 * communications or after a complete read/write sequence.
> +	 */
> +	i2c_smbus_xfer(client->adapter, client->addr, I2C_M_IGNORE_NAK,
> +		       I2C_SMBUS_WRITE, 0, I2C_SMBUS_QUICK, NULL);
> +}
> +
> +static int sunrise_read_word(struct sunrise_dev *sunrise, u8 reg, u16 *val)
> +{
> +	__be16 be_val;
> +	int ret;
> +
> +	sunrise_wakeup(sunrise);
> +	ret = regmap_bulk_read(sunrise->regmap, reg, &be_val, 2);
> +	if (ret) {
> +		dev_err(sunrise->dev, "Read word failed: reg 0x%2x (%d)\n", reg, ret);
> +		return ret;
> +	}
> +
> +	*val = be16_to_cpu(be_val);
> +
> +	return 0;
> +}
> +
> +static int sunrise_write_byte(struct sunrise_dev *sunrise, u8 reg, u8 val)
> +{
> +	int ret;
> +
> +	sunrise_wakeup(sunrise);
> +	ret = regmap_write(sunrise->regmap, reg, val);
> +	if (ret) {
> +		dev_err(sunrise->dev, "Write byte failed: reg 0x%2x (%d)\n", reg, ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int sunrise_write_word(struct sunrise_dev *sunrise, u8 reg, u16 data)
> +{
> +	__be16 be_data = cpu_to_be16(data);
> +	int ret;
> +
> +	sunrise_wakeup(sunrise);

Hmm. Technically there isn't anything stopping another user of the i2c bus sneaking in
between the wakeup and the following command.  That would make the device going back
to sleep a lot more likely.  I can't off the top of my head remember if regmap lets
you lock the bus.  If not, you'll have to use the underlying i2c bus locking functions.
https://elixir.bootlin.com/linux/latest/source/drivers/iio/temperature/mlx90614.c#L432
gives an example.

Perhaps worth a look is the regmap-sccb implementation which has a dance that looks
a tiny bit like what you have to do here (be it for a different reason).
It might be nice to do something similar here and have a custom regmap bus which
has the necessary wakeups in the relevant places.

Note I haven't thought it through in depth, so it might not work!

> +	ret = regmap_bulk_write(sunrise->regmap, reg, &be_data, 2);
> +	if (ret) {
> +		dev_err(sunrise->dev, "Write word failed: reg 0x%2x (%d)\n", reg, ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +/*
> + *  --- Calibration ---
> + *
> + *  Enumerate and select calibration modes, trigger a calibration cycle.
> + */
> +static const char * const sunrise_calibration_modes[] = {
> +	[SUNRISE_CALIBRATION_FACTORY] = "factory_calibration",
> +	[SUNRISE_CALIBRATION_BACKGROUND] = "background_calibration",
> +};
> +
> +static const struct sunrise_calibration_data {
> +	u16 calibration_cmd;
> +	u8 calibration_bit;
> +} sunrise_calibrations[] = {
> +	[SUNRISE_CALIBRATION_FACTORY] = {
> +		SUNRISE_CALIBRATION_FACTORY_CMD,
> +		BIT(2),
> +	},
> +	[SUNRISE_CALIBRATION_BACKGROUND] = {
> +		SUNRISE_CALIBRATION_BACKGROUND_CMD,
> +		BIT(5),
> +	},
> +};
> +
> +static int sunrise_calibrate(struct sunrise_dev *sunrise)
> +{
> +	const struct sunrise_calibration_data *data;
> +	unsigned int status;
> +	int ret;
> +
> +	/* Reset the calibration status reg. */

I was kind of assuming the locking around calibration mode was to avoid races
with this.  Hence, why no lock here?

> +	ret = sunrise_write_byte(sunrise, SUNRISE_CALIBRATION_STATUS_REG, 0x00);
> +	if (ret)
> +		return ret;
> +
> +	/* Write a calibration command and poll the calibration status bit. */
> +	data = &sunrise_calibrations[sunrise->calibration];
> +	ret = sunrise_write_word(sunrise, SUNRISE_CALIBRATION_COMMAND_REG,
> +				 data->calibration_cmd);
> +	if (ret)
> +		return ret;
> +
> +	dev_dbg(sunrise->dev, "%s in progress\n",
> +		sunrise_calibration_modes[sunrise->calibration]);
> +
> +	return regmap_read_poll_timeout(sunrise->regmap,
> +					SUNRISE_CALIBRATION_STATUS_REG,
> +					status, status & data->calibration_bit,
> +					100, SUNRISE_CALIBRATION_TIMEOUT_US);
> +}
> +
> +static ssize_t sunrise_calibration_write(struct iio_dev *iiodev,
> +					 uintptr_t private,
> +					 const struct iio_chan_spec *chan,
> +					 const char *buf, size_t len)
> +{
> +	struct sunrise_dev *sunrise = iio_priv(iiodev);
> +	bool calibrate;
> +	int ret;
> +
> +	ret = kstrtobool(buf, &calibrate);
> +	if (ret)
> +		return ret;
> +
> +	if (!calibrate)
> +		return 0;

return len or an error code.  Not 0,

> +
> +	ret = sunrise_calibrate(sunrise);
> +	if (ret)
> +		return ret;
> +
> +	return len;
> +}
> +
> +static int sunrise_set_calibration_mode(struct iio_dev *iiodev,
> +					const struct iio_chan_spec *chan,
> +					unsigned int mode)
> +{
> +	struct sunrise_dev *sunrise = iio_priv(iiodev);
> +
> +	mutex_lock(&sunrise->lock);
> +	sunrise->calibration = mode;
> +	mutex_unlock(&sunrise->lock);
> +
> +	return 0;
> +}
> +
> +static int sunrise_get_calibration_mode(struct iio_dev *iiodev,
> +					const struct iio_chan_spec *chan)
> +{
> +	struct sunrise_dev *sunrise = iio_priv(iiodev);
> +	int mode;
> +
> +	mutex_lock(&sunrise->lock);
> +	mode = sunrise->calibration;
> +	mutex_unlock(&sunrise->lock);
> +
> +	return mode;
> +}
> +
> +static const struct iio_enum sunrise_calibration_modes_enum = {
> +	.items = sunrise_calibration_modes,
> +	.num_items = ARRAY_SIZE(sunrise_calibration_modes),
> +	.set = sunrise_set_calibration_mode,
> +	.get = sunrise_get_calibration_mode,
> +};
> +
> +/* --- Error status---
/*
 * --- Error status --- 

If you really want to do the heading.  I'm not sure it adds anything over
the fairly short description that follows, so I'd just have

/* Enumerate and retrieve the chip error status */

> + *
> + * Enumerate and retrieve the chip error status.
> + */
> +enum {
> +	SUNRISE_ERROR_FATAL,
> +	SUNRISE_ERROR_I2C,
> +	SUNRISE_ERROR_ALGORITHM,
> +	SUNRISE_ERROR_CALIBRATION,
> +	SUNRISE_ERROR_SELF_DIAGNOSTIC,
> +	SUNRISE_ERROR_OUT_OF_RANGE,
> +	SUNRISE_ERROR_MEMORY,
> +	SUNRISE_ERROR_NO_MEASUREMENT,
> +	SUNRISE_ERROR_LOW_VOLTAGE,
> +	SUNRISE_ERROR_MEASUREMENT_TIMEOUT,
> +};
> +
> +static const char * const sunrise_error_statuses[] = {
> +	[SUNRISE_ERROR_FATAL] = "error_fatal",
> +	[SUNRISE_ERROR_I2C] = "error_i2c",
> +	[SUNRISE_ERROR_ALGORITHM] = "error_algorithm",
> +	[SUNRISE_ERROR_CALIBRATION] = "error_calibration",
> +	[SUNRISE_ERROR_SELF_DIAGNOSTIC] = "error_self_diagnostic",
> +	[SUNRISE_ERROR_OUT_OF_RANGE] = "error_out_of_range",
> +	[SUNRISE_ERROR_MEMORY] = "error_memory",
> +	[SUNRISE_ERROR_NO_MEASUREMENT] = "error_no_measurement",
> +	[SUNRISE_ERROR_LOW_VOLTAGE] = "error_low_voltage",
> +	[SUNRISE_ERROR_MEASUREMENT_TIMEOUT] = "error_measurement_timeout",
> +};
> +
> +static const u8 error_codes[] = {
> +	SUNRISE_ERROR_FATAL,
> +	SUNRISE_ERROR_I2C,
> +	SUNRISE_ERROR_ALGORITHM,
> +	SUNRISE_ERROR_CALIBRATION,
> +	SUNRISE_ERROR_SELF_DIAGNOSTIC,
> +	SUNRISE_ERROR_OUT_OF_RANGE,
> +	SUNRISE_ERROR_MEMORY,
> +	SUNRISE_ERROR_NO_MEASUREMENT,
> +	SUNRISE_ERROR_LOW_VOLTAGE,
> +	SUNRISE_ERROR_MEASUREMENT_TIMEOUT,
> +};
> +
> +static const struct iio_enum sunrise_error_statuses_enum = {
> +	.items = sunrise_error_statuses,
> +	.num_items = ARRAY_SIZE(sunrise_error_statuses),
> +};
> +
> +static ssize_t sunrise_error_status_read(struct iio_dev *iiodev,
> +					 uintptr_t private,
> +					 const struct iio_chan_spec *chan,
> +					 char *buf)
> +{
> +	struct sunrise_dev *sunrise = iio_priv(iiodev);
> +	const unsigned long *errors;
> +	ssize_t len = 0;
> +	u16 value;
> +	int ret;
> +	u8 i;
> +
> +	ret = sunrise_read_word(sunrise, SUNRISE_ERROR_STATUS_REG, &value);
> +	if (ret)
> +		return -EINVAL;
> +
> +	errors = (const unsigned long *)&value;

Copy it to an unsigned long as discussed in other branch of this thread.

> +	for_each_set_bit(i, errors, ARRAY_SIZE(error_codes))

Unless I'm going crazy, ARRAY_SIZE(sunrise_error_statuses) == ARRAY_SIZE(error_codes)
and so there isn't any point in having the error_codes array.

> +		len += sysfs_emit_at(buf, len, "%s ", sunrise_error_statuses[i]);
> +
> +	if (len)
> +		buf[len - 1] = '\n';
> +
> +	return len;
> +}
> +
> +static const struct iio_chan_spec_ext_info sunrise_concentration_ext_info[] = {
> +	/* Calibration modes and calibration trigger. */
> +	{
> +		.name = "calibration",
> +		.write = sunrise_calibration_write,
> +		.shared = IIO_SEPARATE,
> +	},
> +	IIO_ENUM("calibration_mode", IIO_SEPARATE,
> +		 &sunrise_calibration_modes_enum),

I'll comment on this ABI in the docs patch rather than here.
Given you asked somewhere about ext_info vs explicit attrs.
In theory we always prefer ext_info because it provides a way to access them from
in kernel consumers + enforces naming etc.  However as we have a massive number
of legacy attributes I haven't yet started insisting on it, even for new drivers.

Good to see it here though!


> +	IIO_ENUM_AVAILABLE("calibration_mode",
> +			   &sunrise_calibration_modes_enum),
> +
> +	/* Error statuses. */
> +	{
> +		.name = "error_status",
> +		.read = sunrise_error_status_read,
> +		.shared = IIO_SEPARATE
> +	},
> +	IIO_ENUM_AVAILABLE("error_status", &sunrise_error_statuses_enum),
> +	{}
> +};
> +
> +static const struct iio_chan_spec sunrise_channels[] = {
> +	{
> +		.type = IIO_CONCENTRATION,
> +		.modified = 1,
> +		.channel2 = IIO_MOD_CO2,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> +		.ext_info = sunrise_concentration_ext_info,
> +		.scan_index = 0,
> +		.scan_type =  {
> +			.sign = 's',
> +			.realbits = 16,
> +			.storagebits = 16,
> +			.endianness = IIO_CPU,
> +		},
> +	},
> +	{
> +		.type = IIO_TEMP,
> +		.info_mask_separate =  BIT(IIO_CHAN_INFO_RAW) |
> +				       BIT(IIO_CHAN_INFO_SCALE),
> +		.scan_index = 1,
> +		.scan_type =  {
> +			.sign = 's',
> +			.realbits = 16,
> +			.storagebits = 16,
> +			.endianness = IIO_CPU,
> +		},
> +	},
> +};
> +
> +static int sunrise_read_raw(struct iio_dev *iio_dev,
> +			    const struct iio_chan_spec *chan,
> +			    int *val, int *val2, long mask)
> +{
> +	struct sunrise_dev *sunrise = iio_priv(iio_dev);
> +	u16 value;
> +	int ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		mutex_lock(&sunrise->lock);
> +
> +		switch (chan->type) {
> +		case IIO_CONCENTRATION:
> +			ret = sunrise_read_word(sunrise, SUNRISE_CO2_FILTERED_COMP_REG,
> +						&value);
> +			*val = value;
> +			mutex_unlock(&sunrise->lock);
> +
> +			return ret ?: IIO_VAL_INT;

I mentioned in a late response to an earlier one that I'm not overly keen on this form, but
I can live with it if you prefer it.

> +
> +		case IIO_TEMP:
> +			ret = sunrise_read_word(sunrise, SUNRISE_CHIP_TEMPERATURE_REG,
> +						&value);
> +			*val = value;
> +			mutex_unlock(&sunrise->lock);
> +
> +			return ret ?: IIO_VAL_INT;
> +
> +		default:
> +			mutex_unlock(&sunrise->lock);

Move the locks into the two case statements, then you won't have to unlock here which
will be cleaner.

> +			return -EINVAL;
> +		}
> +
> +	case IIO_CHAN_INFO_SCALE:
> +		/* Chip temperature scale = 1/100 */

IIO temperatures are measured in milli degrees.  1lsb = 1/100*1000 degrees centigrade seems very accurate
for a device like this!  I'm guessing this should be 10.

> +		*val = 1;
> +		*val2 = 100;
> +		return IIO_VAL_FRACTIONAL;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static const struct iio_info sunrise_info = {
> +	.read_raw = sunrise_read_raw,
> +};
> +
> +static struct regmap_config sunrise_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +};
> +
> +static int sunrise_probe(struct i2c_client *client)
> +{
> +	struct sunrise_dev *sunrise;
> +	struct iio_dev *iio_dev;
> +
> +	iio_dev = devm_iio_device_alloc(&client->dev, sizeof(*sunrise));
> +	if (!iio_dev)
> +		return -ENOMEM;
> +
> +	i2c_set_clientdata(client, iio_dev);

Why?  I'm not immediately spotting where this is used.

> +
> +	sunrise = iio_priv(iio_dev);
> +	sunrise->client = client;
> +	sunrise->dev = &client->dev;

Why carry this around when you can get it from client->dev?

> +	mutex_init(&sunrise->lock);
> +
> +	sunrise->regmap = devm_regmap_init_i2c(client, &sunrise_regmap_config);
> +	if (IS_ERR(sunrise->regmap)) {
> +		dev_err(&client->dev, "Failed to initialize regmap\n");
> +		return PTR_ERR(sunrise->regmap);
> +	}
> +
> +	iio_dev->info = &sunrise_info;
> +	iio_dev->name = DRIVER_NAME;
> +	iio_dev->channels = sunrise_channels;
> +	iio_dev->num_channels = ARRAY_SIZE(sunrise_channels);
> +	iio_dev->modes = INDIO_DIRECT_MODE;
> +
> +	return devm_iio_device_register(&client->dev, iio_dev);
> +}
> +
> +static const struct of_device_id sunrise_of_match[] = {
> +	{ .compatible = "senseair,sunrise-006-0-0007" },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, sunrise_of_match);
> +
> +static struct i2c_driver sunrise_driver = {
> +	.driver = {
> +		.name = DRIVER_NAME,
> +		.of_match_table = sunrise_of_match,
> +	},
> +	.probe_new = sunrise_probe,
> +};
> +module_i2c_driver(sunrise_driver);
> +
> +MODULE_AUTHOR("Jacopo Mondi <jacopo@jmondi.org>");
> +MODULE_DESCRIPTION("Senseair Sunrise 006-0-0007 CO2 sensor IIO driver");
> +MODULE_LICENSE("GPL v2");
> --
> 2.32.0
> 

Re: [PATCH v3.1 2/3] iio: chemical: Add Senseair Sunrise 006-0-007 driver

[Jacopo Mondi]

Hi Jonathan,
   thanks for review

On Sun, Aug 29, 2021 at 05:54:13PM +0100, Jonathan Cameron wrote:
> On Mon, 23 Aug 2021 09:36:39 +0200
> Jacopo Mondi <jacopo@jmondi.org> wrote:
>
> > Add support for the Senseair Sunrise 006-0-0007 driver through the
> > IIO subsystem.
> >
> > Datasheet: https://rmtplusstoragesenseair.blob.core.windows.net/docs/Dev/publicerat/TDE5531.pdf
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> > v3->v3.1
>
> Always do a new version of the whole series.  The automated tools that maintainers
> mostly use these days (e.g. b4) are pointed out a whole series when picking it up.
>
> This means we have to do it manually, one patch at a time I think which is annoying.
>

Right, sorry, it's pretty common in other subsystems for minor
updates, but I understand it's more work on your side! Sorry about
that!

>
> > - Remove debug leftover
> > - Re-add commas at the end of arrays declarations
> > ---
> >  MAINTAINERS                        |   6 +
> >  drivers/iio/chemical/Kconfig       |  13 +
> >  drivers/iio/chemical/Makefile      |   1 +
> >  drivers/iio/chemical/sunrise_co2.c | 448 +++++++++++++++++++++++++++++
> >  4 files changed, 468 insertions(+)
> >  create mode 100644 drivers/iio/chemical/sunrise_co2.c
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 90ca9df1d3c3..43f5bba46673 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -16544,6 +16544,12 @@ S:	Maintained
> >  F:	drivers/misc/phantom.c
> >  F:	include/uapi/linux/phantom.h
> >
> > +SENSEAIR SUNRISE 006-0-0007
> > +M:	Jacopo Mondi <jacopo@jmondi.org>
> > +S:	Maintained
> > +F:	Documentation/devicetree/bindings/iio/chemical/senseair,sunrise.yaml
> > +F:	drivers/iio/chemical/sunrise_co2.c
> > +
> >  SENSIRION SCD30 CARBON DIOXIDE SENSOR DRIVER
> >  M:	Tomasz Duszynski <tomasz.duszynski@octakon.com>
> >  S:	Maintained
> > diff --git a/drivers/iio/chemical/Kconfig b/drivers/iio/chemical/Kconfig
> > index 10bb431bc3ce..ee8562949226 100644
> > --- a/drivers/iio/chemical/Kconfig
> > +++ b/drivers/iio/chemical/Kconfig
> > @@ -144,6 +144,19 @@ config SPS30
> >  	  To compile this driver as a module, choose M here: the module will
> >  	  be called sps30.
> >
> > +config SENSEAIR_SUNRISE_CO2
> > +	tristate "Senseair Sunrise 006-0-0007 CO2 sensor"
> > +	depends on OF
>
> Not needed.
>

Well, the driver won't be probed as it doesn't have an i2c id table.
But I guess it compiles fine
> > +	depends on I2C
>
> regmap_i2c select should bring that in.
>
> > +	depends on SYSFS
>
> I'd be surprised if this necessary...   Everything should be stubbed appropriately if
> its' not there.

I asked the same question as I didn't find any symbol related to
iio/sysfs to depend on. I should have guessed it is not needed

> > +	select REGMAP_I2C
> > +	help
> > +	  Say yes here to build support for Senseair Sunrise 006-0-0007 CO2
> > +	  sensor.
> > +
> > +	  To compile this driver as a module, choose M here: the
> > +	  module will be called sunrise_co2.
> > +
> >  config VZ89X
> >  	tristate "SGX Sensortech MiCS VZ89X VOC sensor"
> >  	depends on I2C
> > diff --git a/drivers/iio/chemical/Makefile b/drivers/iio/chemical/Makefile
> > index fef63dd5bf92..d5e2a3331d57 100644
> > --- a/drivers/iio/chemical/Makefile
> > +++ b/drivers/iio/chemical/Makefile
> > @@ -17,4 +17,5 @@ obj-$(CONFIG_SCD30_I2C) += scd30_i2c.o
> >  obj-$(CONFIG_SCD30_SERIAL) += scd30_serial.o
> >  obj-$(CONFIG_SENSIRION_SGP30)	+= sgp30.o
> >  obj-$(CONFIG_SPS30) += sps30.o
> > +obj-$(CONFIG_SENSEAIR_SUNRISE_CO2) += sunrise_co2.o
> >  obj-$(CONFIG_VZ89X)		+= vz89x.o
> > diff --git a/drivers/iio/chemical/sunrise_co2.c b/drivers/iio/chemical/sunrise_co2.c
> > new file mode 100644
> > index 000000000000..84f19df6fc00
> > --- /dev/null
> > +++ b/drivers/iio/chemical/sunrise_co2.c
> > @@ -0,0 +1,448 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Senseair Sunrise 006-0-0007 CO2 sensor driver.
> > + *
> > + * Copyright (C) 2021 Jacopo Mondi
> > + *
> > + * List of features not yet supported by the driver:
> > + * - controllable EN pin
> > + * - single-shot operations using the nDRY pin.
> > + * - ABC/target calibration
> > + */
> > +
> > +#include <linux/bitops.h>
> > +#include <linux/i2c.h>
> > +#include <linux/kernel.h>
> > +#include <linux/mod_devicetable.h>
> > +#include <linux/module.h>
> > +#include <linux/mutex.h>
> > +#include <linux/regmap.h>
> > +#include <linux/time64.h>
> > +
> > +#include <linux/iio/iio.h>
> > +#include <linux/iio/sysfs.h>
>
> What are you using from this header?
>

Leftover from when I used raw attributes. I'll drop

> > +
> > +#define DRIVER_NAME "sunrise"
> > +
> > +#define SUNRISE_ERROR_STATUS_REG		0x00
> > +#define SUNRISE_CO2_FILTERED_COMP_REG		0x06
> > +#define SUNRISE_CHIP_TEMPERATURE_REG		0x08
> > +#define SUNRISE_CALIBRATION_STATUS_REG		0x81
> > +#define SUNRISE_CALIBRATION_COMMAND_REG		0x82
> > +#define SUNRISE_CALIBRATION_FACTORY_CMD		0x7c02
> > +#define SUNRISE_CALIBRATION_BACKGROUND_CMD	0x7c06
> > +/*
> > + * The calibration timeout is not characterized in the datasheet.
> > + * Use 30 seconds as a reasonable upper limit.
> > + */
> > +#define SUNRISE_CALIBRATION_TIMEOUT_US		(30 * USEC_PER_SEC)
> > +
> > +enum sunrise_calib {
> > +	SUNRISE_CALIBRATION_FACTORY,
> > +	SUNRISE_CALIBRATION_BACKGROUND,
> > +};
> > +
> > +struct sunrise_dev {
> > +	struct device *dev;
> > +	struct i2c_client *client;
> > +	struct regmap *regmap;
> > +	struct mutex lock;
> > +	enum sunrise_calib calibration;
> > +};
> > +
> > +static void sunrise_wakeup(struct sunrise_dev *sunrise)
> > +{
> > +	struct i2c_client *client = sunrise->client;
> > +
> > +	/*
> > +	 * Wake up sensor by sending sensor address: START, sensor address,
> > +	 * STOP. Sensor will not ACK this byte.
> > +	 *
> > +	 * The chip returns in low power state after 15msec without
> > +	 * communications or after a complete read/write sequence.
> > +	 */
> > +	i2c_smbus_xfer(client->adapter, client->addr, I2C_M_IGNORE_NAK,
> > +		       I2C_SMBUS_WRITE, 0, I2C_SMBUS_QUICK, NULL);
> > +}
> > +
> > +static int sunrise_read_word(struct sunrise_dev *sunrise, u8 reg, u16 *val)
> > +{
> > +	__be16 be_val;
> > +	int ret;
> > +
> > +	sunrise_wakeup(sunrise);
> > +	ret = regmap_bulk_read(sunrise->regmap, reg, &be_val, 2);
> > +	if (ret) {
> > +		dev_err(sunrise->dev, "Read word failed: reg 0x%2x (%d)\n", reg, ret);
> > +		return ret;
> > +	}
> > +
> > +	*val = be16_to_cpu(be_val);
> > +
> > +	return 0;
> > +}
> > +
> > +static int sunrise_write_byte(struct sunrise_dev *sunrise, u8 reg, u8 val)
> > +{
> > +	int ret;
> > +
> > +	sunrise_wakeup(sunrise);
> > +	ret = regmap_write(sunrise->regmap, reg, val);
> > +	if (ret) {
> > +		dev_err(sunrise->dev, "Write byte failed: reg 0x%2x (%d)\n", reg, ret);
> > +		return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int sunrise_write_word(struct sunrise_dev *sunrise, u8 reg, u16 data)
> > +{
> > +	__be16 be_data = cpu_to_be16(data);
> > +	int ret;
> > +
> > +	sunrise_wakeup(sunrise);
>
> Hmm. Technically there isn't anything stopping another user of the i2c bus sneaking in
> between the wakeup and the following command.  That would make the device going back
> to sleep a lot more likely.  I can't off the top of my head remember if regmap lets
> you lock the bus.  If not, you'll have to use the underlying i2c bus locking functions.
> https://elixir.bootlin.com/linux/latest/source/drivers/iio/temperature/mlx90614.c#L432
> gives an example.

Right, there might be another call stealing the wakeup session!

I should lock the underlying i2c bus, probably not root adapter like
mlx90614 does but only the segment.

>
> Perhaps worth a look is the regmap-sccb implementation which has a dance that looks
> a tiny bit like what you have to do here (be it for a different reason).
> It might be nice to do something similar here and have a custom regmap bus which
> has the necessary wakeups in the relevant places.
>
> Note I haven't thought it through in depth, so it might not work!

the dance is similar if not regmap-sccb tranfers a byte instead of
sending only the R/W bit (notice the usage of I2C_SMBUS_QUICK here and
I2C_SMBUS_BYTE in regmap-sccb). Practically speaking it makes no
difference as the sensor nacks the first message, so the underlying
bus implementation bails out, but that's a bit of work-by-accident
thing, isn't it ?

If fine with you, I would stick to this implementation and hold the
segment locked between the wakup and the actual messages.

>
> > +	ret = regmap_bulk_write(sunrise->regmap, reg, &be_data, 2);
> > +	if (ret) {
> > +		dev_err(sunrise->dev, "Write word failed: reg 0x%2x (%d)\n", reg, ret);
> > +		return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +/*
> > + *  --- Calibration ---
> > + *
> > + *  Enumerate and select calibration modes, trigger a calibration cycle.
> > + */
> > +static const char * const sunrise_calibration_modes[] = {
> > +	[SUNRISE_CALIBRATION_FACTORY] = "factory_calibration",
> > +	[SUNRISE_CALIBRATION_BACKGROUND] = "background_calibration",
> > +};
> > +
> > +static const struct sunrise_calibration_data {
> > +	u16 calibration_cmd;
> > +	u8 calibration_bit;
> > +} sunrise_calibrations[] = {
> > +	[SUNRISE_CALIBRATION_FACTORY] = {
> > +		SUNRISE_CALIBRATION_FACTORY_CMD,
> > +		BIT(2),
> > +	},
> > +	[SUNRISE_CALIBRATION_BACKGROUND] = {
> > +		SUNRISE_CALIBRATION_BACKGROUND_CMD,
> > +		BIT(5),
> > +	},
> > +};
> > +
> > +static int sunrise_calibrate(struct sunrise_dev *sunrise)
> > +{
> > +	const struct sunrise_calibration_data *data;
> > +	unsigned int status;
> > +	int ret;
> > +
> > +	/* Reset the calibration status reg. */
>
> I was kind of assuming the locking around calibration mode was to avoid races
> with this.  Hence, why no lock here?
>

As I assumed that as long as the write on the sysfs file does not
return all other attempts would have to wait. And this function only
returns when calibration is completed. However one can open a file
with O_NONBLOCK, does this apply to a syfs attribute as well ? In that
case yes, I should lock here.

> > +	ret = sunrise_write_byte(sunrise, SUNRISE_CALIBRATION_STATUS_REG, 0x00);
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* Write a calibration command and poll the calibration status bit. */
> > +	data = &sunrise_calibrations[sunrise->calibration];
> > +	ret = sunrise_write_word(sunrise, SUNRISE_CALIBRATION_COMMAND_REG,
> > +				 data->calibration_cmd);
> > +	if (ret)
> > +		return ret;
> > +
> > +	dev_dbg(sunrise->dev, "%s in progress\n",
> > +		sunrise_calibration_modes[sunrise->calibration]);
> > +
> > +	return regmap_read_poll_timeout(sunrise->regmap,
> > +					SUNRISE_CALIBRATION_STATUS_REG,
> > +					status, status & data->calibration_bit,
> > +					100, SUNRISE_CALIBRATION_TIMEOUT_US);
> > +}
> > +
> > +static ssize_t sunrise_calibration_write(struct iio_dev *iiodev,
> > +					 uintptr_t private,
> > +					 const struct iio_chan_spec *chan,
> > +					 const char *buf, size_t len)
> > +{
> > +	struct sunrise_dev *sunrise = iio_priv(iiodev);
> > +	bool calibrate;
> > +	int ret;
> > +
> > +	ret = kstrtobool(buf, &calibrate);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (!calibrate)
> > +		return 0;
>
> return len or an error code.  Not 0,
>

Ack

> > +
> > +	ret = sunrise_calibrate(sunrise);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return len;
> > +}
> > +
> > +static int sunrise_set_calibration_mode(struct iio_dev *iiodev,
> > +					const struct iio_chan_spec *chan,
> > +					unsigned int mode)
> > +{
> > +	struct sunrise_dev *sunrise = iio_priv(iiodev);
> > +
> > +	mutex_lock(&sunrise->lock);
> > +	sunrise->calibration = mode;
> > +	mutex_unlock(&sunrise->lock);
> > +
> > +	return 0;
> > +}
> > +
> > +static int sunrise_get_calibration_mode(struct iio_dev *iiodev,
> > +					const struct iio_chan_spec *chan)
> > +{
> > +	struct sunrise_dev *sunrise = iio_priv(iiodev);
> > +	int mode;
> > +
> > +	mutex_lock(&sunrise->lock);
> > +	mode = sunrise->calibration;
> > +	mutex_unlock(&sunrise->lock);
> > +
> > +	return mode;
> > +}
> > +
> > +static const struct iio_enum sunrise_calibration_modes_enum = {
> > +	.items = sunrise_calibration_modes,
> > +	.num_items = ARRAY_SIZE(sunrise_calibration_modes),
> > +	.set = sunrise_set_calibration_mode,
> > +	.get = sunrise_get_calibration_mode,
> > +};
> > +
> > +/* --- Error status---
> /*
>  * --- Error status ---
>
> If you really want to do the heading.  I'm not sure it adds anything over
> the fairly short description that follows, so I'd just have
>
> /* Enumerate and retrieve the chip error status */
>

Ok, I'll keep my comment headers style for me next time :)

> > + *
> > + * Enumerate and retrieve the chip error status.
> > + */
> > +enum {
> > +	SUNRISE_ERROR_FATAL,
> > +	SUNRISE_ERROR_I2C,
> > +	SUNRISE_ERROR_ALGORITHM,
> > +	SUNRISE_ERROR_CALIBRATION,
> > +	SUNRISE_ERROR_SELF_DIAGNOSTIC,
> > +	SUNRISE_ERROR_OUT_OF_RANGE,
> > +	SUNRISE_ERROR_MEMORY,
> > +	SUNRISE_ERROR_NO_MEASUREMENT,
> > +	SUNRISE_ERROR_LOW_VOLTAGE,
> > +	SUNRISE_ERROR_MEASUREMENT_TIMEOUT,
> > +};
> > +
> > +static const char * const sunrise_error_statuses[] = {
> > +	[SUNRISE_ERROR_FATAL] = "error_fatal",
> > +	[SUNRISE_ERROR_I2C] = "error_i2c",
> > +	[SUNRISE_ERROR_ALGORITHM] = "error_algorithm",
> > +	[SUNRISE_ERROR_CALIBRATION] = "error_calibration",
> > +	[SUNRISE_ERROR_SELF_DIAGNOSTIC] = "error_self_diagnostic",
> > +	[SUNRISE_ERROR_OUT_OF_RANGE] = "error_out_of_range",
> > +	[SUNRISE_ERROR_MEMORY] = "error_memory",
> > +	[SUNRISE_ERROR_NO_MEASUREMENT] = "error_no_measurement",
> > +	[SUNRISE_ERROR_LOW_VOLTAGE] = "error_low_voltage",
> > +	[SUNRISE_ERROR_MEASUREMENT_TIMEOUT] = "error_measurement_timeout",
> > +};
> > +
> > +static const u8 error_codes[] = {
> > +	SUNRISE_ERROR_FATAL,
> > +	SUNRISE_ERROR_I2C,
> > +	SUNRISE_ERROR_ALGORITHM,
> > +	SUNRISE_ERROR_CALIBRATION,
> > +	SUNRISE_ERROR_SELF_DIAGNOSTIC,
> > +	SUNRISE_ERROR_OUT_OF_RANGE,
> > +	SUNRISE_ERROR_MEMORY,
> > +	SUNRISE_ERROR_NO_MEASUREMENT,
> > +	SUNRISE_ERROR_LOW_VOLTAGE,
> > +	SUNRISE_ERROR_MEASUREMENT_TIMEOUT,
> > +};
> > +
> > +static const struct iio_enum sunrise_error_statuses_enum = {
> > +	.items = sunrise_error_statuses,
> > +	.num_items = ARRAY_SIZE(sunrise_error_statuses),
> > +};
> > +
> > +static ssize_t sunrise_error_status_read(struct iio_dev *iiodev,
> > +					 uintptr_t private,
> > +					 const struct iio_chan_spec *chan,
> > +					 char *buf)
> > +{
> > +	struct sunrise_dev *sunrise = iio_priv(iiodev);
> > +	const unsigned long *errors;
> > +	ssize_t len = 0;
> > +	u16 value;
> > +	int ret;
> > +	u8 i;
> > +
> > +	ret = sunrise_read_word(sunrise, SUNRISE_ERROR_STATUS_REG, &value);
> > +	if (ret)
> > +		return -EINVAL;
> > +
> > +	errors = (const unsigned long *)&value;
>
> Copy it to an unsigned long as discussed in other branch of this thread.
>

Ack, done in v3.1

> > +	for_each_set_bit(i, errors, ARRAY_SIZE(error_codes))
>
> Unless I'm going crazy, ARRAY_SIZE(sunrise_error_statuses) == ARRAY_SIZE(error_codes)
> and so there isn't any point in having the error_codes array.
>

Uh, you're right. I thought I had to layout error bits in an array to
cycle on them, but what I care about is the size only, so
sunrise_error_statuses will do just fine. I'll drop

> > +		len += sysfs_emit_at(buf, len, "%s ", sunrise_error_statuses[i]);
> > +
> > +	if (len)
> > +		buf[len - 1] = '\n';
> > +
> > +	return len;
> > +}
> > +
> > +static const struct iio_chan_spec_ext_info sunrise_concentration_ext_info[] = {
> > +	/* Calibration modes and calibration trigger. */
> > +	{
> > +		.name = "calibration",
> > +		.write = sunrise_calibration_write,
> > +		.shared = IIO_SEPARATE,
> > +	},
> > +	IIO_ENUM("calibration_mode", IIO_SEPARATE,
> > +		 &sunrise_calibration_modes_enum),
>
> I'll comment on this ABI in the docs patch rather than here.
> Given you asked somewhere about ext_info vs explicit attrs.
> In theory we always prefer ext_info because it provides a way to access them from
> in kernel consumers + enforces naming etc.  However as we have a massive number
> of legacy attributes I haven't yet started insisting on it, even for new drivers.
>
> Good to see it here though!
>

Good! I'll keep using ext_info and update the ABI as you suggested.
Be aware though that the chip supports up to 5 calibration modes, and
we'll have one attribute for each of them, that's why I thought having
2 only was better. With an ack to potentially have 5 attrs, I'll
change the ABI

>
> > +	IIO_ENUM_AVAILABLE("calibration_mode",
> > +			   &sunrise_calibration_modes_enum),
> > +
> > +	/* Error statuses. */
> > +	{
> > +		.name = "error_status",
> > +		.read = sunrise_error_status_read,
> > +		.shared = IIO_SEPARATE
> > +	},
> > +	IIO_ENUM_AVAILABLE("error_status", &sunrise_error_statuses_enum),
> > +	{}
> > +};
> > +
> > +static const struct iio_chan_spec sunrise_channels[] = {
> > +	{
> > +		.type = IIO_CONCENTRATION,
> > +		.modified = 1,
> > +		.channel2 = IIO_MOD_CO2,
> > +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> > +		.ext_info = sunrise_concentration_ext_info,
> > +		.scan_index = 0,
> > +		.scan_type =  {
> > +			.sign = 's',
> > +			.realbits = 16,
> > +			.storagebits = 16,
> > +			.endianness = IIO_CPU,
> > +		},
> > +	},
> > +	{
> > +		.type = IIO_TEMP,
> > +		.info_mask_separate =  BIT(IIO_CHAN_INFO_RAW) |
> > +				       BIT(IIO_CHAN_INFO_SCALE),
> > +		.scan_index = 1,
> > +		.scan_type =  {
> > +			.sign = 's',
> > +			.realbits = 16,
> > +			.storagebits = 16,
> > +			.endianness = IIO_CPU,
> > +		},
> > +	},
> > +};
> > +
> > +static int sunrise_read_raw(struct iio_dev *iio_dev,
> > +			    const struct iio_chan_spec *chan,
> > +			    int *val, int *val2, long mask)
> > +{
> > +	struct sunrise_dev *sunrise = iio_priv(iio_dev);
> > +	u16 value;
> > +	int ret;
> > +
> > +	switch (mask) {
> > +	case IIO_CHAN_INFO_RAW:
> > +		mutex_lock(&sunrise->lock);
> > +
> > +		switch (chan->type) {
> > +		case IIO_CONCENTRATION:
> > +			ret = sunrise_read_word(sunrise, SUNRISE_CO2_FILTERED_COMP_REG,
> > +						&value);
> > +			*val = value;
> > +			mutex_unlock(&sunrise->lock);
> > +
> > +			return ret ?: IIO_VAL_INT;
>
> I mentioned in a late response to an earlier one that I'm not overly keen on this form, but
> I can live with it if you prefer it.
>

Whatever, really, I've done a few back and forth already. I'm more
accustomed to the canonical if (ret) return ret; as well fwiw
> > +
> > +		case IIO_TEMP:
> > +			ret = sunrise_read_word(sunrise, SUNRISE_CHIP_TEMPERATURE_REG,
> > +						&value);
> > +			*val = value;
> > +			mutex_unlock(&sunrise->lock);
> > +
> > +			return ret ?: IIO_VAL_INT;
> > +
> > +		default:
> > +			mutex_unlock(&sunrise->lock);
>
> Move the locks into the two case statements, then you won't have to unlock here which
> will be cleaner.

Ack

>
> > +			return -EINVAL;
> > +		}
> > +
> > +	case IIO_CHAN_INFO_SCALE:
> > +		/* Chip temperature scale = 1/100 */
>
> IIO temperatures are measured in milli degrees.  1lsb = 1/100*1000 degrees centigrade seems very accurate
> for a device like this!  I'm guessing this should be 10.

Ah yes, I thought it had to be given in the chip's native format,
which is 1/100 degree.

I guess I should then multiply by 10 the temperature raw read and
return IIO_VAL_INT here.

>
> > +		*val = 1;
> > +		*val2 = 100;
> > +		return IIO_VAL_FRACTIONAL;
> > +
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +}
> > +
> > +static const struct iio_info sunrise_info = {
> > +	.read_raw = sunrise_read_raw,
> > +};
> > +
> > +static struct regmap_config sunrise_regmap_config = {
> > +	.reg_bits = 8,
> > +	.val_bits = 8,
> > +};
> > +
> > +static int sunrise_probe(struct i2c_client *client)
> > +{
> > +	struct sunrise_dev *sunrise;
> > +	struct iio_dev *iio_dev;
> > +
> > +	iio_dev = devm_iio_device_alloc(&client->dev, sizeof(*sunrise));
> > +	if (!iio_dev)
> > +		return -ENOMEM;
> > +
> > +	i2c_set_clientdata(client, iio_dev);
>
> Why?  I'm not immediately spotting where this is used.
>

Probably a leftover since when I used raw attrs ?

> > +
> > +	sunrise = iio_priv(iio_dev);
> > +	sunrise->client = client;
> > +	sunrise->dev = &client->dev;
>
> Why carry this around when you can get it from client->dev?
>

Andy had the same comment. I think it's very cheap and makes the error
printing more compact. But if it bothers both of you I can drop it.

Thanks
   j

> > +	mutex_init(&sunrise->lock);
> > +
> > +	sunrise->regmap = devm_regmap_init_i2c(client, &sunrise_regmap_config);
> > +	if (IS_ERR(sunrise->regmap)) {
> > +		dev_err(&client->dev, "Failed to initialize regmap\n");
> > +		return PTR_ERR(sunrise->regmap);
> > +	}
> > +
> > +	iio_dev->info = &sunrise_info;
> > +	iio_dev->name = DRIVER_NAME;
> > +	iio_dev->channels = sunrise_channels;
> > +	iio_dev->num_channels = ARRAY_SIZE(sunrise_channels);
> > +	iio_dev->modes = INDIO_DIRECT_MODE;
> > +
> > +	return devm_iio_device_register(&client->dev, iio_dev);
> > +}
> > +
> > +static const struct of_device_id sunrise_of_match[] = {
> > +	{ .compatible = "senseair,sunrise-006-0-0007" },
> > +	{}
> > +};
> > +MODULE_DEVICE_TABLE(of, sunrise_of_match);
> > +
> > +static struct i2c_driver sunrise_driver = {
> > +	.driver = {
> > +		.name = DRIVER_NAME,
> > +		.of_match_table = sunrise_of_match,
> > +	},
> > +	.probe_new = sunrise_probe,
> > +};
> > +module_i2c_driver(sunrise_driver);
> > +
> > +MODULE_AUTHOR("Jacopo Mondi <jacopo@jmondi.org>");
> > +MODULE_DESCRIPTION("Senseair Sunrise 006-0-0007 CO2 sensor IIO driver");
> > +MODULE_LICENSE("GPL v2");
> > --
> > 2.32.0
> >
>

Re: [PATCH v3.1 2/3] iio: chemical: Add Senseair Sunrise 006-0-007 driver

[Jacopo Mondi]

On Mon, Aug 30, 2021 at 06:20:55PM +0200, Jacopo Mondi wrote:
> > > +	case IIO_CHAN_INFO_SCALE:
> > > +		/* Chip temperature scale = 1/100 */
> >
> > IIO temperatures are measured in milli degrees.  1lsb = 1/100*1000 degrees centigrade seems very accurate
> > for a device like this!  I'm guessing this should be 10.
>
> Ah yes, I thought it had to be given in the chip's native format,
> which is 1/100 degree.
>
> I guess I should then multiply by 10 the temperature raw read and
> return IIO_VAL_INT here.

Of course I don't have to multiply by 10 the raw value, and that's
what the _scale attribute is for.

Fingers faster than brain it seems. And I'm a slow typer.

Re: [PATCH v3.1 2/3] iio: chemical: Add Senseair Sunrise 006-0-007 driver

[Jonathan Cameron]

On Mon, 30 Aug 2021 18:20:51 +0200
Jacopo Mondi <jacopo@jmondi.org> wrote:

> Hi Jonathan,
>    thanks for review
> 
> On Sun, Aug 29, 2021 at 05:54:13PM +0100, Jonathan Cameron wrote:
> > On Mon, 23 Aug 2021 09:36:39 +0200
> > Jacopo Mondi <jacopo@jmondi.org> wrote:
> >  
> > > Add support for the Senseair Sunrise 006-0-0007 driver through the
> > > IIO subsystem.
> > >
> > > Datasheet: https://rmtplusstoragesenseair.blob.core.windows.net/docs/Dev/publicerat/TDE5531.pdf
> > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > > ---
> > > v3->v3.1  
> >
> > Always do a new version of the whole series.  The automated tools that maintainers
> > mostly use these days (e.g. b4) are pointed out a whole series when picking it up.
> >
> > This means we have to do it manually, one patch at a time I think which is annoying.
> >  
> 
> Right, sorry, it's pretty common in other subsystems for minor
> updates, but I understand it's more work on your side! Sorry about
> that!
> 
> >  
> > > - Remove debug leftover
> > > - Re-add commas at the end of arrays declarations
> > > ---
> > >  MAINTAINERS                        |   6 +
> > >  drivers/iio/chemical/Kconfig       |  13 +
> > >  drivers/iio/chemical/Makefile      |   1 +
> > >  drivers/iio/chemical/sunrise_co2.c | 448 +++++++++++++++++++++++++++++
> > >  4 files changed, 468 insertions(+)
> > >  create mode 100644 drivers/iio/chemical/sunrise_co2.c
> > >
> > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > index 90ca9df1d3c3..43f5bba46673 100644
> > > --- a/MAINTAINERS
> > > +++ b/MAINTAINERS
> > > @@ -16544,6 +16544,12 @@ S:	Maintained
> > >  F:	drivers/misc/phantom.c
> > >  F:	include/uapi/linux/phantom.h
> > >
> > > +SENSEAIR SUNRISE 006-0-0007
> > > +M:	Jacopo Mondi <jacopo@jmondi.org>
> > > +S:	Maintained
> > > +F:	Documentation/devicetree/bindings/iio/chemical/senseair,sunrise.yaml
> > > +F:	drivers/iio/chemical/sunrise_co2.c
> > > +
> > >  SENSIRION SCD30 CARBON DIOXIDE SENSOR DRIVER
> > >  M:	Tomasz Duszynski <tomasz.duszynski@octakon.com>
> > >  S:	Maintained
> > > diff --git a/drivers/iio/chemical/Kconfig b/drivers/iio/chemical/Kconfig
> > > index 10bb431bc3ce..ee8562949226 100644
> > > --- a/drivers/iio/chemical/Kconfig
> > > +++ b/drivers/iio/chemical/Kconfig
> > > @@ -144,6 +144,19 @@ config SPS30
> > >  	  To compile this driver as a module, choose M here: the module will
> > >  	  be called sps30.
> > >
> > > +config SENSEAIR_SUNRISE_CO2
> > > +	tristate "Senseair Sunrise 006-0-0007 CO2 sensor"
> > > +	depends on OF  
> >
> > Not needed.
> >  
> 
> Well, the driver won't be probed as it doesn't have an i2c id table.
> But I guess it compiles fine

So you'd think.  But let me introduce you to ACPI PRP0001 which oddly uses
the of_device_id table to bind to a device in an ACPI DSDT table ;)

> > > +	depends on I2C  
> >
> > regmap_i2c select should bring that in.
> >  
> > > +	depends on SYSFS  
> >
> > I'd be surprised if this necessary...   Everything should be stubbed appropriately if
> > its' not there.  
> 
> I asked the same question as I didn't find any symbol related to
> iio/sysfs to depend on. I should have guessed it is not needed
> 
> > > +	select REGMAP_I2C
> > > +	help
> > > +	  Say yes here to build support for Senseair Sunrise 006-0-0007 CO2
> > > +	  sensor.
> > > +
> > > +	  To compile this driver as a module, choose M here: the
> > > +	  module will be called sunrise_co2.
> > > +
> > >  config VZ89X
> > >  	tristate "SGX Sensortech MiCS VZ89X VOC sensor"
> > >  	depends on I2C
> > > diff --git a/drivers/iio/chemical/Makefile b/drivers/iio/chemical/Makefile
> > > index fef63dd5bf92..d5e2a3331d57 100644
> > > --- a/drivers/iio/chemical/Makefile
> > > +++ b/drivers/iio/chemical/Makefile
> > > @@ -17,4 +17,5 @@ obj-$(CONFIG_SCD30_I2C) += scd30_i2c.o
> > >  obj-$(CONFIG_SCD30_SERIAL) += scd30_serial.o
> > >  obj-$(CONFIG_SENSIRION_SGP30)	+= sgp30.o
> > >  obj-$(CONFIG_SPS30) += sps30.o
> > > +obj-$(CONFIG_SENSEAIR_SUNRISE_CO2) += sunrise_co2.o
> > >  obj-$(CONFIG_VZ89X)		+= vz89x.o
> > > diff --git a/drivers/iio/chemical/sunrise_co2.c b/drivers/iio/chemical/sunrise_co2.c
> > > new file mode 100644
> > > index 000000000000..84f19df6fc00
> > > --- /dev/null
> > > +++ b/drivers/iio/chemical/sunrise_co2.c
> > > @@ -0,0 +1,448 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * Senseair Sunrise 006-0-0007 CO2 sensor driver.
> > > + *
> > > + * Copyright (C) 2021 Jacopo Mondi
> > > + *
> > > + * List of features not yet supported by the driver:
> > > + * - controllable EN pin
> > > + * - single-shot operations using the nDRY pin.
> > > + * - ABC/target calibration
> > > + */
> > > +
> > > +#include <linux/bitops.h>
> > > +#include <linux/i2c.h>
> > > +#include <linux/kernel.h>
> > > +#include <linux/mod_devicetable.h>
> > > +#include <linux/module.h>
> > > +#include <linux/mutex.h>
> > > +#include <linux/regmap.h>
> > > +#include <linux/time64.h>
> > > +
> > > +#include <linux/iio/iio.h>
> > > +#include <linux/iio/sysfs.h>  
> >
> > What are you using from this header?
> >  
> 
> Leftover from when I used raw attributes. I'll drop
> 
> > > +
> > > +#define DRIVER_NAME "sunrise"
> > > +
> > > +#define SUNRISE_ERROR_STATUS_REG		0x00
> > > +#define SUNRISE_CO2_FILTERED_COMP_REG		0x06
> > > +#define SUNRISE_CHIP_TEMPERATURE_REG		0x08
> > > +#define SUNRISE_CALIBRATION_STATUS_REG		0x81
> > > +#define SUNRISE_CALIBRATION_COMMAND_REG		0x82
> > > +#define SUNRISE_CALIBRATION_FACTORY_CMD		0x7c02
> > > +#define SUNRISE_CALIBRATION_BACKGROUND_CMD	0x7c06
> > > +/*
> > > + * The calibration timeout is not characterized in the datasheet.
> > > + * Use 30 seconds as a reasonable upper limit.
> > > + */
> > > +#define SUNRISE_CALIBRATION_TIMEOUT_US		(30 * USEC_PER_SEC)
> > > +
> > > +enum sunrise_calib {
> > > +	SUNRISE_CALIBRATION_FACTORY,
> > > +	SUNRISE_CALIBRATION_BACKGROUND,
> > > +};
> > > +
> > > +struct sunrise_dev {
> > > +	struct device *dev;
> > > +	struct i2c_client *client;
> > > +	struct regmap *regmap;
> > > +	struct mutex lock;
> > > +	enum sunrise_calib calibration;
> > > +};
> > > +
> > > +static void sunrise_wakeup(struct sunrise_dev *sunrise)
> > > +{
> > > +	struct i2c_client *client = sunrise->client;
> > > +
> > > +	/*
> > > +	 * Wake up sensor by sending sensor address: START, sensor address,
> > > +	 * STOP. Sensor will not ACK this byte.
> > > +	 *
> > > +	 * The chip returns in low power state after 15msec without
> > > +	 * communications or after a complete read/write sequence.
> > > +	 */
> > > +	i2c_smbus_xfer(client->adapter, client->addr, I2C_M_IGNORE_NAK,
> > > +		       I2C_SMBUS_WRITE, 0, I2C_SMBUS_QUICK, NULL);
> > > +}
> > > +
> > > +static int sunrise_read_word(struct sunrise_dev *sunrise, u8 reg, u16 *val)
> > > +{
> > > +	__be16 be_val;
> > > +	int ret;
> > > +
> > > +	sunrise_wakeup(sunrise);
> > > +	ret = regmap_bulk_read(sunrise->regmap, reg, &be_val, 2);
> > > +	if (ret) {
> > > +		dev_err(sunrise->dev, "Read word failed: reg 0x%2x (%d)\n", reg, ret);
> > > +		return ret;
> > > +	}
> > > +
> > > +	*val = be16_to_cpu(be_val);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int sunrise_write_byte(struct sunrise_dev *sunrise, u8 reg, u8 val)
> > > +{
> > > +	int ret;
> > > +
> > > +	sunrise_wakeup(sunrise);
> > > +	ret = regmap_write(sunrise->regmap, reg, val);
> > > +	if (ret) {
> > > +		dev_err(sunrise->dev, "Write byte failed: reg 0x%2x (%d)\n", reg, ret);
> > > +		return ret;
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int sunrise_write_word(struct sunrise_dev *sunrise, u8 reg, u16 data)
> > > +{
> > > +	__be16 be_data = cpu_to_be16(data);
> > > +	int ret;
> > > +
> > > +	sunrise_wakeup(sunrise);  
> >
> > Hmm. Technically there isn't anything stopping another user of the i2c bus sneaking in
> > between the wakeup and the following command.  That would make the device going back
> > to sleep a lot more likely.  I can't off the top of my head remember if regmap lets
> > you lock the bus.  If not, you'll have to use the underlying i2c bus locking functions.
> > https://elixir.bootlin.com/linux/latest/source/drivers/iio/temperature/mlx90614.c#L432
> > gives an example.  
> 
> Right, there might be another call stealing the wakeup session!
> 
> I should lock the underlying i2c bus, probably not root adapter like
> mlx90614 does but only the segment.

Ideally only segment as you say as could be some muxes involved.

> 
> >
> > Perhaps worth a look is the regmap-sccb implementation which has a dance that looks
> > a tiny bit like what you have to do here (be it for a different reason).
> > It might be nice to do something similar here and have a custom regmap bus which
> > has the necessary wakeups in the relevant places.
> >
> > Note I haven't thought it through in depth, so it might not work!  
> 
> the dance is similar if not regmap-sccb tranfers a byte instead of
> sending only the R/W bit (notice the usage of I2C_SMBUS_QUICK here and
> I2C_SMBUS_BYTE in regmap-sccb). Practically speaking it makes no
> difference as the sensor nacks the first message, so the underlying
> bus implementation bails out, but that's a bit of work-by-accident
> thing, isn't it ?
> 
> If fine with you, I would stick to this implementation and hold the
> segment locked between the wakup and the actual messages.

That's fine.  I was just thinking you could hid the magic in a custom regmap then
the rest of the driver would not have to be aware of it.  Slightly neater than
wrapping regmap functions with this extra call in the wrapper.

> 
> >  
> > > +	ret = regmap_bulk_write(sunrise->regmap, reg, &be_data, 2);
> > > +	if (ret) {
> > > +		dev_err(sunrise->dev, "Write word failed: reg 0x%2x (%d)\n", reg, ret);
> > > +		return ret;
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +/*
> > > + *  --- Calibration ---
> > > + *
> > > + *  Enumerate and select calibration modes, trigger a calibration cycle.
> > > + */
> > > +static const char * const sunrise_calibration_modes[] = {
> > > +	[SUNRISE_CALIBRATION_FACTORY] = "factory_calibration",
> > > +	[SUNRISE_CALIBRATION_BACKGROUND] = "background_calibration",
> > > +};
> > > +
> > > +static const struct sunrise_calibration_data {
> > > +	u16 calibration_cmd;
> > > +	u8 calibration_bit;
> > > +} sunrise_calibrations[] = {
> > > +	[SUNRISE_CALIBRATION_FACTORY] = {
> > > +		SUNRISE_CALIBRATION_FACTORY_CMD,
> > > +		BIT(2),
> > > +	},
> > > +	[SUNRISE_CALIBRATION_BACKGROUND] = {
> > > +		SUNRISE_CALIBRATION_BACKGROUND_CMD,
> > > +		BIT(5),
> > > +	},
> > > +};
> > > +
> > > +static int sunrise_calibrate(struct sunrise_dev *sunrise)
> > > +{
> > > +	const struct sunrise_calibration_data *data;
> > > +	unsigned int status;
> > > +	int ret;
> > > +
> > > +	/* Reset the calibration status reg. */  
> >
> > I was kind of assuming the locking around calibration mode was to avoid races
> > with this.  Hence, why no lock here?
> >  
> 
> As I assumed that as long as the write on the sysfs file does not
> return all other attempts would have to wait. And this function only
> returns when calibration is completed. However one can open a file
> with O_NONBLOCK, does this apply to a syfs attribute as well ? In that
> case yes, I should lock here.

IIRC sysfs doesn't have any such protections, particularly not if multiple
files are involved.

> 
> > > +	ret = sunrise_write_byte(sunrise, SUNRISE_CALIBRATION_STATUS_REG, 0x00);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	/* Write a calibration command and poll the calibration status bit. */
> > > +	data = &sunrise_calibrations[sunrise->calibration];
> > > +	ret = sunrise_write_word(sunrise, SUNRISE_CALIBRATION_COMMAND_REG,
> > > +				 data->calibration_cmd);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	dev_dbg(sunrise->dev, "%s in progress\n",
> > > +		sunrise_calibration_modes[sunrise->calibration]);
> > > +
> > > +	return regmap_read_poll_timeout(sunrise->regmap,
> > > +					SUNRISE_CALIBRATION_STATUS_REG,
> > > +					status, status & data->calibration_bit,
> > > +					100, SUNRISE_CALIBRATION_TIMEOUT_US);
> > > +}
> > > +
> > > +static ssize_t sunrise_calibration_write(struct iio_dev *iiodev,
> > > +					 uintptr_t private,
> > > +					 const struct iio_chan_spec *chan,
> > > +					 const char *buf, size_t len)
> > > +{
> > > +	struct sunrise_dev *sunrise = iio_priv(iiodev);
> > > +	bool calibrate;
> > > +	int ret;
> > > +
> > > +	ret = kstrtobool(buf, &calibrate);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	if (!calibrate)
> > > +		return 0;  
> >
> > return len or an error code.  Not 0,
> >  
> 
> Ack
> 
> > > +
> > > +	ret = sunrise_calibrate(sunrise);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	return len;
> > > +}
> > > +
> > > +static int sunrise_set_calibration_mode(struct iio_dev *iiodev,
> > > +					const struct iio_chan_spec *chan,
> > > +					unsigned int mode)
> > > +{
> > > +	struct sunrise_dev *sunrise = iio_priv(iiodev);
> > > +
> > > +	mutex_lock(&sunrise->lock);
> > > +	sunrise->calibration = mode;
> > > +	mutex_unlock(&sunrise->lock);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int sunrise_get_calibration_mode(struct iio_dev *iiodev,
> > > +					const struct iio_chan_spec *chan)
> > > +{
> > > +	struct sunrise_dev *sunrise = iio_priv(iiodev);
> > > +	int mode;
> > > +
> > > +	mutex_lock(&sunrise->lock);
> > > +	mode = sunrise->calibration;
> > > +	mutex_unlock(&sunrise->lock);
> > > +
> > > +	return mode;
> > > +}
> > > +
> > > +static const struct iio_enum sunrise_calibration_modes_enum = {
> > > +	.items = sunrise_calibration_modes,
> > > +	.num_items = ARRAY_SIZE(sunrise_calibration_modes),
> > > +	.set = sunrise_set_calibration_mode,
> > > +	.get = sunrise_get_calibration_mode,
> > > +};
> > > +
> > > +/* --- Error status---  
> > /*
> >  * --- Error status ---
> >
> > If you really want to do the heading.  I'm not sure it adds anything over
> > the fairly short description that follows, so I'd just have
> >
> > /* Enumerate and retrieve the chip error status */
> >  
> 
> Ok, I'll keep my comment headers style for me next time :)
> 
> > > + *
> > > + * Enumerate and retrieve the chip error status.
> > > + */
> > > +enum {
> > > +	SUNRISE_ERROR_FATAL,
> > > +	SUNRISE_ERROR_I2C,
> > > +	SUNRISE_ERROR_ALGORITHM,
> > > +	SUNRISE_ERROR_CALIBRATION,
> > > +	SUNRISE_ERROR_SELF_DIAGNOSTIC,
> > > +	SUNRISE_ERROR_OUT_OF_RANGE,
> > > +	SUNRISE_ERROR_MEMORY,
> > > +	SUNRISE_ERROR_NO_MEASUREMENT,
> > > +	SUNRISE_ERROR_LOW_VOLTAGE,
> > > +	SUNRISE_ERROR_MEASUREMENT_TIMEOUT,
> > > +};
> > > +
> > > +static const char * const sunrise_error_statuses[] = {
> > > +	[SUNRISE_ERROR_FATAL] = "error_fatal",
> > > +	[SUNRISE_ERROR_I2C] = "error_i2c",
> > > +	[SUNRISE_ERROR_ALGORITHM] = "error_algorithm",
> > > +	[SUNRISE_ERROR_CALIBRATION] = "error_calibration",
> > > +	[SUNRISE_ERROR_SELF_DIAGNOSTIC] = "error_self_diagnostic",
> > > +	[SUNRISE_ERROR_OUT_OF_RANGE] = "error_out_of_range",
> > > +	[SUNRISE_ERROR_MEMORY] = "error_memory",
> > > +	[SUNRISE_ERROR_NO_MEASUREMENT] = "error_no_measurement",
> > > +	[SUNRISE_ERROR_LOW_VOLTAGE] = "error_low_voltage",
> > > +	[SUNRISE_ERROR_MEASUREMENT_TIMEOUT] = "error_measurement_timeout",
> > > +};
> > > +
> > > +static const u8 error_codes[] = {
> > > +	SUNRISE_ERROR_FATAL,
> > > +	SUNRISE_ERROR_I2C,
> > > +	SUNRISE_ERROR_ALGORITHM,
> > > +	SUNRISE_ERROR_CALIBRATION,
> > > +	SUNRISE_ERROR_SELF_DIAGNOSTIC,
> > > +	SUNRISE_ERROR_OUT_OF_RANGE,
> > > +	SUNRISE_ERROR_MEMORY,
> > > +	SUNRISE_ERROR_NO_MEASUREMENT,
> > > +	SUNRISE_ERROR_LOW_VOLTAGE,
> > > +	SUNRISE_ERROR_MEASUREMENT_TIMEOUT,
> > > +};
> > > +
> > > +static const struct iio_enum sunrise_error_statuses_enum = {
> > > +	.items = sunrise_error_statuses,
> > > +	.num_items = ARRAY_SIZE(sunrise_error_statuses),
> > > +};
> > > +
> > > +static ssize_t sunrise_error_status_read(struct iio_dev *iiodev,
> > > +					 uintptr_t private,
> > > +					 const struct iio_chan_spec *chan,
> > > +					 char *buf)
> > > +{
> > > +	struct sunrise_dev *sunrise = iio_priv(iiodev);
> > > +	const unsigned long *errors;
> > > +	ssize_t len = 0;
> > > +	u16 value;
> > > +	int ret;
> > > +	u8 i;
> > > +
> > > +	ret = sunrise_read_word(sunrise, SUNRISE_ERROR_STATUS_REG, &value);
> > > +	if (ret)
> > > +		return -EINVAL;
> > > +
> > > +	errors = (const unsigned long *)&value;  
> >
> > Copy it to an unsigned long as discussed in other branch of this thread.
> >  
> 
> Ack, done in v3.1
> 
> > > +	for_each_set_bit(i, errors, ARRAY_SIZE(error_codes))  
> >
> > Unless I'm going crazy, ARRAY_SIZE(sunrise_error_statuses) == ARRAY_SIZE(error_codes)
> > and so there isn't any point in having the error_codes array.
> >  
> 
> Uh, you're right. I thought I had to layout error bits in an array to
> cycle on them, but what I care about is the size only, so
> sunrise_error_statuses will do just fine. I'll drop
> 
> > > +		len += sysfs_emit_at(buf, len, "%s ", sunrise_error_statuses[i]);
> > > +
> > > +	if (len)
> > > +		buf[len - 1] = '\n';
> > > +
> > > +	return len;
> > > +}
> > > +
> > > +static const struct iio_chan_spec_ext_info sunrise_concentration_ext_info[] = {
> > > +	/* Calibration modes and calibration trigger. */
> > > +	{
> > > +		.name = "calibration",
> > > +		.write = sunrise_calibration_write,
> > > +		.shared = IIO_SEPARATE,
> > > +	},
> > > +	IIO_ENUM("calibration_mode", IIO_SEPARATE,
> > > +		 &sunrise_calibration_modes_enum),  
> >
> > I'll comment on this ABI in the docs patch rather than here.
> > Given you asked somewhere about ext_info vs explicit attrs.
> > In theory we always prefer ext_info because it provides a way to access them from
> > in kernel consumers + enforces naming etc.  However as we have a massive number
> > of legacy attributes I haven't yet started insisting on it, even for new drivers.
> >
> > Good to see it here though!
> >  
> 
> Good! I'll keep using ext_info and update the ABI as you suggested.
> Be aware though that the chip supports up to 5 calibration modes, and
> we'll have one attribute for each of them, that's why I thought having
> 2 only was better. With an ack to potentially have 5 attrs, I'll
> change the ABI
5 clearly defined atts is fine.  If it was 100s I might have a different opinion!

> 
> >  
> > > +	IIO_ENUM_AVAILABLE("calibration_mode",
> > > +			   &sunrise_calibration_modes_enum),
> > > +
> > > +	/* Error statuses. */
> > > +	{
> > > +		.name = "error_status",
> > > +		.read = sunrise_error_status_read,
> > > +		.shared = IIO_SEPARATE
> > > +	},
> > > +	IIO_ENUM_AVAILABLE("error_status", &sunrise_error_statuses_enum),
> > > +	{}
> > > +};
> > > +
> > > +static const struct iio_chan_spec sunrise_channels[] = {
> > > +	{
> > > +		.type = IIO_CONCENTRATION,
> > > +		.modified = 1,
> > > +		.channel2 = IIO_MOD_CO2,
> > > +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> > > +		.ext_info = sunrise_concentration_ext_info,
> > > +		.scan_index = 0,
> > > +		.scan_type =  {
> > > +			.sign = 's',
> > > +			.realbits = 16,
> > > +			.storagebits = 16,
> > > +			.endianness = IIO_CPU,
> > > +		},
> > > +	},
> > > +	{
> > > +		.type = IIO_TEMP,
> > > +		.info_mask_separate =  BIT(IIO_CHAN_INFO_RAW) |
> > > +				       BIT(IIO_CHAN_INFO_SCALE),
> > > +		.scan_index = 1,
> > > +		.scan_type =  {
> > > +			.sign = 's',
> > > +			.realbits = 16,
> > > +			.storagebits = 16,
> > > +			.endianness = IIO_CPU,
> > > +		},
> > > +	},
> > > +};
> > > +
> > > +static int sunrise_read_raw(struct iio_dev *iio_dev,
> > > +			    const struct iio_chan_spec *chan,
> > > +			    int *val, int *val2, long mask)
> > > +{
> > > +	struct sunrise_dev *sunrise = iio_priv(iio_dev);
> > > +	u16 value;
> > > +	int ret;
> > > +
> > > +	switch (mask) {
> > > +	case IIO_CHAN_INFO_RAW:
> > > +		mutex_lock(&sunrise->lock);
> > > +
> > > +		switch (chan->type) {
> > > +		case IIO_CONCENTRATION:
> > > +			ret = sunrise_read_word(sunrise, SUNRISE_CO2_FILTERED_COMP_REG,
> > > +						&value);
> > > +			*val = value;
> > > +			mutex_unlock(&sunrise->lock);
> > > +
> > > +			return ret ?: IIO_VAL_INT;  
> >
> > I mentioned in a late response to an earlier one that I'm not overly keen on this form, but
> > I can live with it if you prefer it.
> >  
> 
> Whatever, really, I've done a few back and forth already. I'm more
> accustomed to the canonical if (ret) return ret; as well fwiw
> > > +
> > > +		case IIO_TEMP:
> > > +			ret = sunrise_read_word(sunrise, SUNRISE_CHIP_TEMPERATURE_REG,
> > > +						&value);
> > > +			*val = value;
> > > +			mutex_unlock(&sunrise->lock);
> > > +
> > > +			return ret ?: IIO_VAL_INT;
> > > +
> > > +		default:
> > > +			mutex_unlock(&sunrise->lock);  
> >
> > Move the locks into the two case statements, then you won't have to unlock here which
> > will be cleaner.  
> 
> Ack
> 
> >  
> > > +			return -EINVAL;
> > > +		}
> > > +
> > > +	case IIO_CHAN_INFO_SCALE:
> > > +		/* Chip temperature scale = 1/100 */  
> >
> > IIO temperatures are measured in milli degrees.  1lsb = 1/100*1000 degrees centigrade seems very accurate
> > for a device like this!  I'm guessing this should be 10.  
> 
> Ah yes, I thought it had to be given in the chip's native format,
> which is 1/100 degree.
> 
> I guess I should then multiply by 10 the temperature raw read and
> return IIO_VAL_INT here.

You could do that, but can cause a mess if buffered support comes along later as
it is then not a whole number of bits for putting in the buffer.

> 
> >  
> > > +		*val = 1;
> > > +		*val2 = 100;
> > > +		return IIO_VAL_FRACTIONAL;
> > > +
> > > +	default:
> > > +		return -EINVAL;
> > > +	}
> > > +}
> > > +
> > > +static const struct iio_info sunrise_info = {
> > > +	.read_raw = sunrise_read_raw,
> > > +};
> > > +
> > > +static struct regmap_config sunrise_regmap_config = {
> > > +	.reg_bits = 8,
> > > +	.val_bits = 8,
> > > +};
> > > +
> > > +static int sunrise_probe(struct i2c_client *client)
> > > +{
> > > +	struct sunrise_dev *sunrise;
> > > +	struct iio_dev *iio_dev;
> > > +
> > > +	iio_dev = devm_iio_device_alloc(&client->dev, sizeof(*sunrise));
> > > +	if (!iio_dev)
> > > +		return -ENOMEM;
> > > +
> > > +	i2c_set_clientdata(client, iio_dev);  
> >
> > Why?  I'm not immediately spotting where this is used.
> >  
> 
> Probably a leftover since when I used raw attrs ?
> 
> > > +
> > > +	sunrise = iio_priv(iio_dev);
> > > +	sunrise->client = client;
> > > +	sunrise->dev = &client->dev;  
> >
> > Why carry this around when you can get it from client->dev?
> >  
> 
> Andy had the same comment. I think it's very cheap and makes the error
> printing more compact. But if it bothers both of you I can drop it.

I'd drop it, but if you have a particular function with multiple prints
then have a local struct device *dev in that function.  The compiler will
tidy that up so it makes no difference and it will be even neater at
call sites.

> 
> Thanks
>    j
> 
> > > +	mutex_init(&sunrise->lock);
> > > +
> > > +	sunrise->regmap = devm_regmap_init_i2c(client, &sunrise_regmap_config);
> > > +	if (IS_ERR(sunrise->regmap)) {
> > > +		dev_err(&client->dev, "Failed to initialize regmap\n");
> > > +		return PTR_ERR(sunrise->regmap);
> > > +	}
> > > +
> > > +	iio_dev->info = &sunrise_info;
> > > +	iio_dev->name = DRIVER_NAME;
> > > +	iio_dev->channels = sunrise_channels;
> > > +	iio_dev->num_channels = ARRAY_SIZE(sunrise_channels);
> > > +	iio_dev->modes = INDIO_DIRECT_MODE;
> > > +
> > > +	return devm_iio_device_register(&client->dev, iio_dev);
> > > +}
> > > +
> > > +static const struct of_device_id sunrise_of_match[] = {
> > > +	{ .compatible = "senseair,sunrise-006-0-0007" },
> > > +	{}
> > > +};
> > > +MODULE_DEVICE_TABLE(of, sunrise_of_match);
> > > +
> > > +static struct i2c_driver sunrise_driver = {
> > > +	.driver = {
> > > +		.name = DRIVER_NAME,
> > > +		.of_match_table = sunrise_of_match,
> > > +	},
> > > +	.probe_new = sunrise_probe,
> > > +};
> > > +module_i2c_driver(sunrise_driver);
> > > +
> > > +MODULE_AUTHOR("Jacopo Mondi <jacopo@jmondi.org>");
> > > +MODULE_DESCRIPTION("Senseair Sunrise 006-0-0007 CO2 sensor IIO driver");
> > > +MODULE_LICENSE("GPL v2");
> > > --
> > > 2.32.0
> > >  
> >  

Re: [PATCH v3.1 2/3] iio: chemical: Add Senseair Sunrise 006-0-007 driver

[Jacopo Mondi]

Hi Jonathan,
   two more clarification requests, sorry to bother :)

On Mon, Aug 30, 2021 at 06:11:17PM +0100, Jonathan Cameron wrote:
> On Mon, 30 Aug 2021 18:20:51 +0200
> > > > +static int sunrise_write_word(struct sunrise_dev *sunrise, u8 reg, u16 data)
> > > > +{
> > > > +	__be16 be_data = cpu_to_be16(data);
> > > > +	int ret;
> > > > +
> > > > +	sunrise_wakeup(sunrise);
> > >
> > > Hmm. Technically there isn't anything stopping another user of the i2c bus sneaking in
> > > between the wakeup and the following command.  That would make the device going back
> > > to sleep a lot more likely.  I can't off the top of my head remember if regmap lets
> > > you lock the bus.  If not, you'll have to use the underlying i2c bus locking functions.
> > > https://elixir.bootlin.com/linux/latest/source/drivers/iio/temperature/mlx90614.c#L432
> > > gives an example.
> >
> > Right, there might be another call stealing the wakeup session!
> >
> > I should lock the underlying i2c bus, probably not root adapter like
> > mlx90614 does but only the segment.
>
> Ideally only segment as you say as could be some muxes involved.

If not that i2c_smbus_xfer() which is called by my wakeup and by the
regmap_smbus_* calls tries to lock the segment as well, so we deadlock :)

And actually, locking the underlying i2c segment seems even too
strict, what we have to guarantee is that no other read/write function
call from this driver interrupts a [wakeup-trasactions] sequence.

Wouldn't it be better if I handle that with a dedicated mutex ?

>
> >
> > >
> > > Perhaps worth a look is the regmap-sccb implementation which has a dance that looks
> > > a tiny bit like what you have to do here (be it for a different reason).
> > > It might be nice to do something similar here and have a custom regmap bus which
> > > has the necessary wakeups in the relevant places.
> > >
> > > Note I haven't thought it through in depth, so it might not work!
> >
> > the dance is similar if not regmap-sccb tranfers a byte instead of
> > sending only the R/W bit (notice the usage of I2C_SMBUS_QUICK here and
> > I2C_SMBUS_BYTE in regmap-sccb). Practically speaking it makes no
> > difference as the sensor nacks the first message, so the underlying
> > bus implementation bails out, but that's a bit of work-by-accident
> > thing, isn't it ?
> >
> > If fine with you, I would stick to this implementation and hold the
> > segment locked between the wakup and the actual messages.
>
> That's fine.  I was just thinking you could hid the magic in a custom regmap then
> the rest of the driver would not have to be aware of it.  Slightly neater than
> wrapping regmap functions with this extra call in the wrapper.
>

[snip]

> > > > +		}
> > > > +
> > > > +	case IIO_CHAN_INFO_SCALE:
> > > > +		/* Chip temperature scale = 1/100 */
> > >
> > > IIO temperatures are measured in milli degrees.  1lsb = 1/100*1000 degrees centigrade seems very accurate
> > > for a device like this!  I'm guessing this should be 10.
> >
> > Ah yes, I thought it had to be given in the chip's native format,
> > which is 1/100 degree.
> >
> > I guess I should then multiply by 10 the temperature raw read and
> > return IIO_VAL_INT here.
>
> You could do that, but can cause a mess if buffered support comes along later as
> it is then not a whole number of bits for putting in the buffer.
>

Care to clarify ? What shouldn't I do here ? Multiply by 10 the raw
value (which I think is wrong as pointed out in a later reply) or
return IIO_VAL_INT ? Sorry I didn't get the connection with the number
of bits to put in the buffer :)

Thanks
   j

Re: [PATCH v3.1 2/3] iio: chemical: Add Senseair Sunrise 006-0-007 driver

[Jonathan Cameron]

On Tue, 31 Aug 2021 09:40:11 +0200
Jacopo Mondi <jacopo@jmondi.org> wrote:

> Hi Jonathan,
>    two more clarification requests, sorry to bother :)
No problem.  First one: No idea +CC wolfram and i2c list.
> 
> On Mon, Aug 30, 2021 at 06:11:17PM +0100, Jonathan Cameron wrote:
> > On Mon, 30 Aug 2021 18:20:51 +0200  
> > > > > +static int sunrise_write_word(struct sunrise_dev *sunrise, u8 reg, u16 data)
> > > > > +{
> > > > > +	__be16 be_data = cpu_to_be16(data);
> > > > > +	int ret;
> > > > > +
> > > > > +	sunrise_wakeup(sunrise);  
> > > >
> > > > Hmm. Technically there isn't anything stopping another user of the i2c bus sneaking in
> > > > between the wakeup and the following command.  That would make the device going back
> > > > to sleep a lot more likely.  I can't off the top of my head remember if regmap lets
> > > > you lock the bus.  If not, you'll have to use the underlying i2c bus locking functions.
> > > > https://elixir.bootlin.com/linux/latest/source/drivers/iio/temperature/mlx90614.c#L432
> > > > gives an example.  
> > >
> > > Right, there might be another call stealing the wakeup session!
> > >
> > > I should lock the underlying i2c bus, probably not root adapter like
> > > mlx90614 does but only the segment.  
> >
> > Ideally only segment as you say as could be some muxes involved.  
> 
> If not that i2c_smbus_xfer() which is called by my wakeup and by the
> regmap_smbus_* calls tries to lock the segment as well, so we deadlock :)
> 
> And actually, locking the underlying i2c segment seems even too
> strict, what we have to guarantee is that no other read/write function
> call from this driver interrupts a [wakeup-trasactions] sequence.
> 
> Wouldn't it be better if I handle that with a dedicated mutex ?

I'm not sure what best route is. +CC Wolfram, Peter and linux-i2c.

Short story here is we have a device which autonomously goes to sleep.
Datasheet suggests waking it up with a failed xfer followed by what we
actually want to do (sufficiently quickly).

Obviously we can't actually guarantee that will ever happen but it's a lot
more likely to succeed if we briefly stop anything else using he i2c bus.

How should we handle this?




> 
> >  
> > >  
> > > >
> > > > Perhaps worth a look is the regmap-sccb implementation which has a dance that looks
> > > > a tiny bit like what you have to do here (be it for a different reason).
> > > > It might be nice to do something similar here and have a custom regmap bus which
> > > > has the necessary wakeups in the relevant places.
> > > >
> > > > Note I haven't thought it through in depth, so it might not work!  
> > >
> > > the dance is similar if not regmap-sccb tranfers a byte instead of
> > > sending only the R/W bit (notice the usage of I2C_SMBUS_QUICK here and
> > > I2C_SMBUS_BYTE in regmap-sccb). Practically speaking it makes no
> > > difference as the sensor nacks the first message, so the underlying
> > > bus implementation bails out, but that's a bit of work-by-accident
> > > thing, isn't it ?
> > >
> > > If fine with you, I would stick to this implementation and hold the
> > > segment locked between the wakup and the actual messages.  
> >
> > That's fine.  I was just thinking you could hid the magic in a custom regmap then
> > the rest of the driver would not have to be aware of it.  Slightly neater than
> > wrapping regmap functions with this extra call in the wrapper.
> >  
> 
> [snip]
> 
> > > > > +		}
> > > > > +
> > > > > +	case IIO_CHAN_INFO_SCALE:
> > > > > +		/* Chip temperature scale = 1/100 */  
> > > >
> > > > IIO temperatures are measured in milli degrees.  1lsb = 1/100*1000 degrees centigrade seems very accurate
> > > > for a device like this!  I'm guessing this should be 10.  
> > >
> > > Ah yes, I thought it had to be given in the chip's native format,
> > > which is 1/100 degree.
> > >
> > > I guess I should then multiply by 10 the temperature raw read and
> > > return IIO_VAL_INT here.  
> >
> > You could do that, but can cause a mess if buffered support comes along later as
> > it is then not a whole number of bits for putting in the buffer.
> >  
> 
> Care to clarify ? What shouldn't I do here ? Multiply by 10 the raw
> value (which I think is wrong as pointed out in a later reply) or
> return IIO_VAL_INT ? Sorry I didn't get the connection with the number
> of bits to put in the buffer :)

So.  If you stick to output of _raw and _scale in the buffer the data
would be whatever you read from the register.  That is typically a whole number of bits.
If you were to multiply by 10 you end up something that may be say 12 or 13 bits depending
on the value.  That's a bit ugly. We can handle it of course, but I'd rather not if it's
as simple as not doing the *10 in kernel for the sysfs path.

So not critical but things end up more elegant / standard if we don't do the *10 and
instead make that a problem for userspace.

Jonathan


> 
> Thanks
>    j

Re: [PATCH v3.1 2/3] iio: chemical: Add Senseair Sunrise 006-0-007 driver

[Peter Rosin]

On 2021-09-05 12:04, Jonathan Cameron wrote:
> On Tue, 31 Aug 2021 09:40:11 +0200
> Jacopo Mondi <jacopo@jmondi.org> wrote:
> 
>> Hi Jonathan,
>>    two more clarification requests, sorry to bother :)
> No problem.  First one: No idea +CC wolfram and i2c list.
>>
>> On Mon, Aug 30, 2021 at 06:11:17PM +0100, Jonathan Cameron wrote:
>>> On Mon, 30 Aug 2021 18:20:51 +0200  
>>>>>> +static int sunrise_write_word(struct sunrise_dev *sunrise, u8 reg, u16 data)
>>>>>> +{
>>>>>> +	__be16 be_data = cpu_to_be16(data);
>>>>>> +	int ret;
>>>>>> +
>>>>>> +	sunrise_wakeup(sunrise);  
>>>>>
>>>>> Hmm. Technically there isn't anything stopping another user of the i2c bus sneaking in
>>>>> between the wakeup and the following command.  That would make the device going back
>>>>> to sleep a lot more likely.  I can't off the top of my head remember if regmap lets
>>>>> you lock the bus.  If not, you'll have to use the underlying i2c bus locking functions.
>>>>> https://elixir.bootlin.com/linux/latest/source/drivers/iio/temperature/mlx90614.c#L432
>>>>> gives an example.  
>>>>
>>>> Right, there might be another call stealing the wakeup session!
>>>>
>>>> I should lock the underlying i2c bus, probably not root adapter like
>>>> mlx90614 does but only the segment.  
>>>
>>> Ideally only segment as you say as could be some muxes involved.  
>>
>> If not that i2c_smbus_xfer() which is called by my wakeup and by the
>> regmap_smbus_* calls tries to lock the segment as well, so we deadlock :)
>>
>> And actually, locking the underlying i2c segment seems even too
>> strict, what we have to guarantee is that no other read/write function
>> call from this driver interrupts a [wakeup-trasactions] sequence.
>>
>> Wouldn't it be better if I handle that with a dedicated mutex ?
> 
> I'm not sure what best route is. +CC Wolfram, Peter and linux-i2c.
> 
> Short story here is we have a device which autonomously goes to sleep.
> Datasheet suggests waking it up with a failed xfer followed by what we
> actually want to do (sufficiently quickly).
> 
> Obviously we can't actually guarantee that will ever happen but it's a lot
> more likely to succeed if we briefly stop anything else using he i2c bus.
> 
> How should we handle this?

The way I read this is that interactions with other I2C devices that squeeze
in are not a fundamental problem. Not unless there are so many of these 3rd
party xfers that the device times out again. If my assessment is correct,
then I would suggest handling this in the driver by somehow making sure that
it doesn't clobber its own pairs of wakeup+work interactions. But see below.

Because there really is no way to protect against those extra I2C accesses.
With a parent-locked mux you can (ignoring arbitrators, where another
system may possibly take over the bus if too much time is spent between
two xfers that were supposed to be adjacent). But if there's a mux-locked
mux in the path it's simply not possible to lock out all other xfers on
the root adapter.

With a parent-locked I2C tree, "locking the segment" is equivalent to
locking everything all the way to the root adapter. But the whole point
of mux-locked muxes is that they can't operate if you do that. Mux-locked
muxes are allowed to depend on other (ignorant) drivers using other parts
of the I2C tree while the segment is locked. If you lock the root adapter
when there is a mux-locked mux involved, you simply kill that property
and sign up for a deadlock. Which also means that you cannot prevent 3rd
party xfers to other parts of the I2C tree.

However, if you worry about 3rd party xfers causing the wakeup to timeout
again and that only handling it in the driver as suggested above is
insufficient, then it's an option to lock the segment. For parent-locked I2C
trees, this should prevent (most) 3rd party actions and minimize the delay.
In the odd case that there are mux-locked muxes involved, there will simply
be a higher risk of failure, but there is little to do about that.

See Documentation/i2c/i2c-topology.rst for some discussion on the details
of mux-locked and parent-locked muxes.

I hope I make at least some sense...

Cheers,
Peter

Re: [PATCH v3.1 2/3] iio: chemical: Add Senseair Sunrise 006-0-007 driver

[Peter Rosin]

On 2021-09-06 01:03, Peter Rosin wrote:
> On 2021-09-05 12:04, Jonathan Cameron wrote:
>> On Tue, 31 Aug 2021 09:40:11 +0200
>> Jacopo Mondi <jacopo@jmondi.org> wrote:
>>
>>> Hi Jonathan,
>>>    two more clarification requests, sorry to bother :)
>> No problem.  First one: No idea +CC wolfram and i2c list.
>>>
>>> On Mon, Aug 30, 2021 at 06:11:17PM +0100, Jonathan Cameron wrote:
>>>> On Mon, 30 Aug 2021 18:20:51 +0200  
>>>>>>> +static int sunrise_write_word(struct sunrise_dev *sunrise, u8 reg, u16 data)
>>>>>>> +{
>>>>>>> +	__be16 be_data = cpu_to_be16(data);
>>>>>>> +	int ret;
>>>>>>> +
>>>>>>> +	sunrise_wakeup(sunrise);  
>>>>>>
>>>>>> Hmm. Technically there isn't anything stopping another user of the i2c bus sneaking in
>>>>>> between the wakeup and the following command.  That would make the device going back
>>>>>> to sleep a lot more likely.  I can't off the top of my head remember if regmap lets
>>>>>> you lock the bus.  If not, you'll have to use the underlying i2c bus locking functions.
>>>>>> https://elixir.bootlin.com/linux/latest/source/drivers/iio/temperature/mlx90614.c#L432
>>>>>> gives an example.

Forgot to mention, regmap does let you do that. See e.g.
drivers/media/dvb-frontends/rtl2830.c which wraps regmap_update_bits,
regmap_bulk_write and regmap_bulk_read within a local I2C segment
lock along with a special regmap_bus that does unlocked I2C trasfers.

I think the driver does this because it has an I2C gate that needs to
be opened with a regmap interaction that in turn needs to be back to
back with the "real" regmap access, or else the gate closes again.

Cheers,
Peter

Re: [PATCH v3.1 2/3] iio: chemical: Add Senseair Sunrise 006-0-007 driver

[Jacopo Mondi]

Hi Peter,
   thanks for your detailed answer

On Mon, Sep 06, 2021 at 01:03:52AM +0200, Peter Rosin wrote:
> On 2021-09-05 12:04, Jonathan Cameron wrote:
> > On Tue, 31 Aug 2021 09:40:11 +0200
> > Jacopo Mondi <jacopo@jmondi.org> wrote:
> >
> >> Hi Jonathan,
> >>    two more clarification requests, sorry to bother :)
> > No problem.  First one: No idea +CC wolfram and i2c list.
> >>
> >> On Mon, Aug 30, 2021 at 06:11:17PM +0100, Jonathan Cameron wrote:
> >>> On Mon, 30 Aug 2021 18:20:51 +0200
> >>>>>> +static int sunrise_write_word(struct sunrise_dev *sunrise, u8 reg, u16 data)
> >>>>>> +{
> >>>>>> +	__be16 be_data = cpu_to_be16(data);
> >>>>>> +	int ret;
> >>>>>> +
> >>>>>> +	sunrise_wakeup(sunrise);
> >>>>>
> >>>>> Hmm. Technically there isn't anything stopping another user of the i2c bus sneaking in
> >>>>> between the wakeup and the following command.  That would make the device going back
> >>>>> to sleep a lot more likely.  I can't off the top of my head remember if regmap lets
> >>>>> you lock the bus.  If not, you'll have to use the underlying i2c bus locking functions.
> >>>>> https://elixir.bootlin.com/linux/latest/source/drivers/iio/temperature/mlx90614.c#L432
> >>>>> gives an example.
> >>>>
> >>>> Right, there might be another call stealing the wakeup session!
> >>>>
> >>>> I should lock the underlying i2c bus, probably not root adapter like
> >>>> mlx90614 does but only the segment.
> >>>
> >>> Ideally only segment as you say as could be some muxes involved.
> >>
> >> If not that i2c_smbus_xfer() which is called by my wakeup and by the
> >> regmap_smbus_* calls tries to lock the segment as well, so we deadlock :)
> >>
> >> And actually, locking the underlying i2c segment seems even too
> >> strict, what we have to guarantee is that no other read/write function
> >> call from this driver interrupts a [wakeup-trasactions] sequence.
> >>
> >> Wouldn't it be better if I handle that with a dedicated mutex ?
> >
> > I'm not sure what best route is. +CC Wolfram, Peter and linux-i2c.
> >
> > Short story here is we have a device which autonomously goes to sleep.
> > Datasheet suggests waking it up with a failed xfer followed by what we
> > actually want to do (sufficiently quickly).
> >
> > Obviously we can't actually guarantee that will ever happen but it's a lot
> > more likely to succeed if we briefly stop anything else using he i2c bus.
> >
> > How should we handle this?
>
> The way I read this is that interactions with other I2C devices that squeeze
> in are not a fundamental problem. Not unless there are so many of these 3rd
> party xfers that the device times out again. If my assessment is correct,
> then I would suggest handling this in the driver by somehow making sure that
> it doesn't clobber its own pairs of wakeup+work interactions. But see below.

So, I tested by sending a double wakeup signal, to verify if the chip
goes back to sleep after -any- kind of transaction after the first
wakeup. It seems it does from what I see.

I also tested by inserting a spurious i2c transaction to a
non-existing address between the wakeup and the actual transaction,
but I cannot say if that proves anything as I'm not sure if messages
directed to non-registered addresses are actually put on the bus.

Anyway, quoting the chip manual:

------------------------------------------------------------------------------
Senseair Sunrise spend most of its time in deep sleep mode to minimize
power consumption, this have the effect that it is necessary to wake
up the sensor before it is possible to communicate with it.  Sensor
will wake up on a falling edge on SDA, it is recommended to send
sensors address to wake it up. When sensors address is used to wake up
the sensor the sensor will not acknowledge this byte.

Communication sequence:
1) Wake up sensor by sending sensor address (START, sensor address, STOP).
   Sensor will not ACK this byte.

2) Normal I2C read/write operations. I2C communication must be started
   within 15ms after the wake-up byte, each byte sent to or from the
   sensor sets the timeout to 15 ms. After a complete read or write
   sequence sensor will enter sleep mode immediately.
------------------------------------------------------------------------------

I think you're correct assuming the only way 3rd parties could
interfere with the wakeup session is by issuing so many transactions
that the bus is not available to the device for such a long time
(15msec) that the wakeup session expires.

Other messages, not directed to the chip, doesn't seem to interfere.

So locking in the driver should be good enough I think.

>
> Because there really is no way to protect against those extra I2C accesses.
> With a parent-locked mux you can (ignoring arbitrators, where another
> system may possibly take over the bus if too much time is spent between
> two xfers that were supposed to be adjacent). But if there's a mux-locked
> mux in the path it's simply not possible to lock out all other xfers on
> the root adapter.
>
> With a parent-locked I2C tree, "locking the segment" is equivalent to
> locking everything all the way to the root adapter. But the whole point
> of mux-locked muxes is that they can't operate if you do that. Mux-locked
> muxes are allowed to depend on other (ignorant) drivers using other parts
> of the I2C tree while the segment is locked. If you lock the root adapter
> when there is a mux-locked mux involved, you simply kill that property
> and sign up for a deadlock. Which also means that you cannot prevent 3rd
> party xfers to other parts of the I2C tree.
>
> However, if you worry about 3rd party xfers causing the wakeup to timeout
> again and that only handling it in the driver as suggested above is
> insufficient, then it's an option to lock the segment. For parent-locked I2C
> trees, this should prevent (most) 3rd party actions and minimize the delay.
> In the odd case that there are mux-locked muxes involved, there will simply
> be a higher risk of failure, but there is little to do about that.

It doesn't feel to me this is required, but I let Jonathan and Andy
which have discussed this with me in the past express an opinion as
well.

In case I need to go for this solution am I correct assuming I should
lock the bus for the whole wakeup-work session and override the
regmap_bus operations to perform unlocked access to the i2c bus?

In my mind this could be realized as

int wakeup()
{

        /* Unlocked wakup ping */
        __i2c_smbus_xfer()
}

int regmap_bus_read()
{
        i2c_lock_bus();

        wakeup();
        /* Unlocked i2c read transaction */
        __i2c_transfer();

        i2c_unlock_bus();
}


struct regmap_bus regmap_bus = {
        .read = regmap_bus_read,
        ...
}

int probe()
{

        regmap_init(..., regmap_bus, ..).
}

But I somehow feel like I could have locking and wakeup handled by a mux's
select/deselect and have a simpler read function. However even if I
think I could have the driver register a mux even if there's actually
no muxed bus behind the chip, I'm missing how that would work if not
by exposing this in the DT bindings or by registering an
i2c_board_info, but this feels already too complicated to be worth it,
right ?

Thanks
   j

>
> See Documentation/i2c/i2c-topology.rst for some discussion on the details
> of mux-locked and parent-locked muxes.
>
> I hope I make at least some sense...
>
> Cheers,
> Peter

Re: [PATCH v3.1 2/3] iio: chemical: Add Senseair Sunrise 006-0-007 driver

[Peter Rosin]

On 2021-09-08 13:00, Jacopo Mondi wrote:
> Hi Peter,
>    thanks for your detailed answer
> 
> On Mon, Sep 06, 2021 at 01:03:52AM +0200, Peter Rosin wrote:
>> The way I read this is that interactions with other I2C devices that squeeze
>> in are not a fundamental problem. Not unless there are so many of these 3rd
>> party xfers that the device times out again. If my assessment is correct,
>> then I would suggest handling this in the driver by somehow making sure that
>> it doesn't clobber its own pairs of wakeup+work interactions. But see below.
> 
> So, I tested by sending a double wakeup signal, to verify if the chip
> goes back to sleep after -any- kind of transaction after the first
> wakeup. It seems it does from what I see.
> 
> I also tested by inserting a spurious i2c transaction to a
> non-existing address between the wakeup and the actual transaction,
> but I cannot say if that proves anything as I'm not sure if messages
> directed to non-registered addresses are actually put on the bus.
> 
> Anyway, quoting the chip manual:
> 
> ------------------------------------------------------------------------------
> Senseair Sunrise spend most of its time in deep sleep mode to minimize
> power consumption, this have the effect that it is necessary to wake
> up the sensor before it is possible to communicate with it.  Sensor
> will wake up on a falling edge on SDA, it is recommended to send
> sensors address to wake it up. When sensors address is used to wake up
> the sensor the sensor will not acknowledge this byte.
> 
> Communication sequence:
> 1) Wake up sensor by sending sensor address (START, sensor address, STOP).
>    Sensor will not ACK this byte.
> 
> 2) Normal I2C read/write operations. I2C communication must be started
>    within 15ms after the wake-up byte, each byte sent to or from the
>    sensor sets the timeout to 15 ms. After a complete read or write
>    sequence sensor will enter sleep mode immediately.
> ------------------------------------------------------------------------------
> 
> I think you're correct assuming the only way 3rd parties could
> interfere with the wakeup session is by issuing so many transactions
> that the bus is not available to the device for such a long time
> (15msec) that the wakeup session expires.
> 
> Other messages, not directed to the chip, doesn't seem to interfere.
> 
> So locking in the driver should be good enough I think.

I think so too. Even if 15ms is kind of short... I mean, locking the
I2C segment can certainly exclude (some) 3rd party xfers on the bus and
that may help, but there is so much else that could potentially cause
a 15ms stall, especially on smallish single-cpu devices.

>>
>> Because there really is no way to protect against those extra I2C accesses.
>> With a parent-locked mux you can (ignoring arbitrators, where another
>> system may possibly take over the bus if too much time is spent between
>> two xfers that were supposed to be adjacent). But if there's a mux-locked
>> mux in the path it's simply not possible to lock out all other xfers on
>> the root adapter.
>>
>> With a parent-locked I2C tree, "locking the segment" is equivalent to
>> locking everything all the way to the root adapter. But the whole point
>> of mux-locked muxes is that they can't operate if you do that. Mux-locked
>> muxes are allowed to depend on other (ignorant) drivers using other parts
>> of the I2C tree while the segment is locked. If you lock the root adapter
>> when there is a mux-locked mux involved, you simply kill that property
>> and sign up for a deadlock. Which also means that you cannot prevent 3rd
>> party xfers to other parts of the I2C tree.
>>
>> However, if you worry about 3rd party xfers causing the wakeup to timeout
>> again and that only handling it in the driver as suggested above is
>> insufficient, then it's an option to lock the segment. For parent-locked I2C
>> trees, this should prevent (most) 3rd party actions and minimize the delay.
>> In the odd case that there are mux-locked muxes involved, there will simply
>> be a higher risk of failure, but there is little to do about that.
> 
> It doesn't feel to me this is required, but I let Jonathan and Andy
> which have discussed this with me in the past express an opinion as
> well.
> 
> In case I need to go for this solution am I correct assuming I should
> lock the bus for the whole wakeup-work session and override the
> regmap_bus operations to perform unlocked access to the i2c bus?
> 
> In my mind this could be realized as
> 
> int wakeup()
> {
> 
>         /* Unlocked wakup ping */
>         __i2c_smbus_xfer()
> }
> 
> int regmap_bus_read()
> {
>         i2c_lock_bus();
> 
>         wakeup();
>         /* Unlocked i2c read transaction */
>         __i2c_transfer();
> 
>         i2c_unlock_bus();
> }
> 
> 
> struct regmap_bus regmap_bus = {
>         .read = regmap_bus_read,
>         ...
> }
> 
> int probe()
> {
> 
>         regmap_init(..., regmap_bus, ..).
> }

Yep, that looks like the right direction from here as well, should you take
that path.

> But I somehow feel like I could have locking and wakeup handled by a mux's
> select/deselect and have a simpler read function. However even if I
> think I could have the driver register a mux even if there's actually
> no muxed bus behind the chip, I'm missing how that would work if not
> by exposing this in the DT bindings or by registering an
> i2c_board_info, but this feels already too complicated to be worth it,
> right ?

This basically is exactly what is otherwise called an I2C gate. You have
to do <something> to get I2C xfers safely past the gate, in this case
wake the device up. So, that model isn't wrong.

However, since the device wakes up on *any* action on SDA, would it not
also work to make special I2C xfers, with a restart instead of a full
stop-start after the "wakeup call". That is, if you can assume that the
I2C adapter is flexible enough...

I.e. something like this:

static int
sunrise_foo(struct sunrise_context *ctx)
{
	unsigned char reg = 0x17;
	unsigned char buf[17];
	struct i2c_msg msg[3] = {
		{	/* wakeup */
			.addr = 0x68,
			.flags = I2C_M_RD | I2C_M_IGNORE_NAK,
			.len = 0,
		}, {	/* write register number */
			.addr = 0x68,
			.flags = 0,
			.len = 1,
			.buf = &reg,
		}, {	/* read register contents */
			.addr = 0x68,
			.flags = I2C_M_RD,
			.len = 17,
			.buf = buf,
		},
	};
	int ret;

	ret = i2c_transfer(ctx->adapter, msg, 3);

	...

	return ret;
}

Cheers,
Peter

> Thanks
>    j
> 
>>
>> See Documentation/i2c/i2c-topology.rst for some discussion on the details
>> of mux-locked and parent-locked muxes.
>>
>> I hope I make at least some sense...
>>
>> Cheers,
>> Peter

Re: [PATCH v3.1 2/3] iio: chemical: Add Senseair Sunrise 006-0-007 driver

[Andy Shevchenko]

On Wed, Sep 8, 2021 at 6:29 PM Peter Rosin <peda@axentia.se> wrote:
> On 2021-09-08 13:00, Jacopo Mondi wrote:
> > On Mon, Sep 06, 2021 at 01:03:52AM +0200, Peter Rosin wrote:

...

>         struct i2c_msg msg[3] = {
>                 {       /* wakeup */
>                         .addr = 0x68,
>                         .flags = I2C_M_RD | I2C_M_IGNORE_NAK,
>                         .len = 0,
>                 }, {    /* write register number */

I'm wondering if device will have enough time in between to actualle
be woken up. I believe the waking up latency must be considered as
well as known 15ms suspending one.

>                         .addr = 0x68,
>                         .flags = 0,
>                         .len = 1,
>                         .buf = &reg,
>                 }, {    /* read register contents */
>                         .addr = 0x68,
>                         .flags = I2C_M_RD,
>                         .len = 17,
>                         .buf = buf,
>                 },
>         };

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3.1 2/3] iio: chemical: Add Senseair Sunrise 006-0-007 driver

[Jacopo Mondi]

Hi Peter,

On Wed, Sep 08, 2021 at 05:29:02PM +0200, Peter Rosin wrote:
> On 2021-09-08 13:00, Jacopo Mondi wrote:
> > Hi Peter,
> >    thanks for your detailed answer
> >
> > On Mon, Sep 06, 2021 at 01:03:52AM +0200, Peter Rosin wrote:
> >> The way I read this is that interactions with other I2C devices that squeeze
> >> in are not a fundamental problem. Not unless there are so many of these 3rd
> >> party xfers that the device times out again. If my assessment is correct,
> >> then I would suggest handling this in the driver by somehow making sure that
> >> it doesn't clobber its own pairs of wakeup+work interactions. But see below.
> >
> > So, I tested by sending a double wakeup signal, to verify if the chip
> > goes back to sleep after -any- kind of transaction after the first
> > wakeup. It seems it does from what I see.
> >
> > I also tested by inserting a spurious i2c transaction to a
> > non-existing address between the wakeup and the actual transaction,
> > but I cannot say if that proves anything as I'm not sure if messages
> > directed to non-registered addresses are actually put on the bus.
> >
> > Anyway, quoting the chip manual:
> >
> > ------------------------------------------------------------------------------
> > Senseair Sunrise spend most of its time in deep sleep mode to minimize
> > power consumption, this have the effect that it is necessary to wake
> > up the sensor before it is possible to communicate with it.  Sensor
> > will wake up on a falling edge on SDA, it is recommended to send
> > sensors address to wake it up. When sensors address is used to wake up
> > the sensor the sensor will not acknowledge this byte.
> >
> > Communication sequence:
> > 1) Wake up sensor by sending sensor address (START, sensor address, STOP).
> >    Sensor will not ACK this byte.
> >
> > 2) Normal I2C read/write operations. I2C communication must be started
> >    within 15ms after the wake-up byte, each byte sent to or from the
> >    sensor sets the timeout to 15 ms. After a complete read or write
> >    sequence sensor will enter sleep mode immediately.
> > ------------------------------------------------------------------------------
> >
> > I think you're correct assuming the only way 3rd parties could
> > interfere with the wakeup session is by issuing so many transactions
> > that the bus is not available to the device for such a long time
> > (15msec) that the wakeup session expires.
> >
> > Other messages, not directed to the chip, doesn't seem to interfere.
> >
> > So locking in the driver should be good enough I think.
>
> I think so too. Even if 15ms is kind of short... I mean, locking the
> I2C segment can certainly exclude (some) 3rd party xfers on the bus and
> that may help, but there is so much else that could potentially cause
> a 15ms stall, especially on smallish single-cpu devices.
>
> >>
> >> Because there really is no way to protect against those extra I2C accesses.
> >> With a parent-locked mux you can (ignoring arbitrators, where another
> >> system may possibly take over the bus if too much time is spent between
> >> two xfers that were supposed to be adjacent). But if there's a mux-locked
> >> mux in the path it's simply not possible to lock out all other xfers on
> >> the root adapter.
> >>
> >> With a parent-locked I2C tree, "locking the segment" is equivalent to
> >> locking everything all the way to the root adapter. But the whole point
> >> of mux-locked muxes is that they can't operate if you do that. Mux-locked
> >> muxes are allowed to depend on other (ignorant) drivers using other parts
> >> of the I2C tree while the segment is locked. If you lock the root adapter
> >> when there is a mux-locked mux involved, you simply kill that property
> >> and sign up for a deadlock. Which also means that you cannot prevent 3rd
> >> party xfers to other parts of the I2C tree.
> >>
> >> However, if you worry about 3rd party xfers causing the wakeup to timeout
> >> again and that only handling it in the driver as suggested above is
> >> insufficient, then it's an option to lock the segment. For parent-locked I2C
> >> trees, this should prevent (most) 3rd party actions and minimize the delay.
> >> In the odd case that there are mux-locked muxes involved, there will simply
> >> be a higher risk of failure, but there is little to do about that.
> >
> > It doesn't feel to me this is required, but I let Jonathan and Andy
> > which have discussed this with me in the past express an opinion as
> > well.
> >
> > In case I need to go for this solution am I correct assuming I should
> > lock the bus for the whole wakeup-work session and override the
> > regmap_bus operations to perform unlocked access to the i2c bus?
> >
> > In my mind this could be realized as
> >
> > int wakeup()
> > {
> >
> >         /* Unlocked wakup ping */
> >         __i2c_smbus_xfer()
> > }
> >
> > int regmap_bus_read()
> > {
> >         i2c_lock_bus();
> >
> >         wakeup();
> >         /* Unlocked i2c read transaction */
> >         __i2c_transfer();
> >
> >         i2c_unlock_bus();
> > }
> >
> >
> > struct regmap_bus regmap_bus = {
> >         .read = regmap_bus_read,
> >         ...
> > }
> >
> > int probe()
> > {
> >
> >         regmap_init(..., regmap_bus, ..).
> > }
>
> Yep, that looks like the right direction from here as well, should you take
> that path.

In facts, I have just implemented this version using unlocked
__i2c_smbus_ functions in the regmap read/write overloads:

static int sunrise_regmap_read(void *context, const void *reg_buf,
			       size_t reg_size, void *val_buf, size_t val_size)
{
	struct i2c_client *client = context;
	union i2c_smbus_data data;
	int ret;

	memset(&data, 0, sizeof(data));
	data.block[0] = val_size;

	/*
	 * Wake up sensor by sending sensor address: START, sensor address,
	 * STOP. Sensor will not ACK this byte.
	 *
	 * The chip enters a low power state after 15msec without
	 * communications or after a complete read/write sequence.
	 */
	__i2c_smbus_xfer(client->adapter, client->addr, I2C_M_IGNORE_NAK,
			 I2C_SMBUS_WRITE, 0, I2C_SMBUS_QUICK, NULL);

	ret = __i2c_smbus_xfer(client->adapter, client->addr, client->flags,
			       I2C_SMBUS_READ, ((u8 *)reg_buf)[0],
			       I2C_SMBUS_I2C_BLOCK_DATA, &data);
	if (ret < 0)
		return ret;

	memcpy(val_buf, &data.block[1], data.block[0]);

	return 0;
}

static int sunrise_regmap_write(void *context, const void *val_buf, size_t count)
{
	struct i2c_client *client = context;
	union i2c_smbus_data data;

	/* Discard reg address from values count. */
	if (count < 1)
		return -EINVAL;
	count--;

	memset(&data, 0, sizeof(data));
	data.block[0] = count;
	memcpy(&data.block[1], (u8 *)val_buf + 1, count);

	__i2c_smbus_xfer(client->adapter, client->addr, I2C_M_IGNORE_NAK,
			 I2C_SMBUS_WRITE, 0, I2C_SMBUS_QUICK, NULL);

	return __i2c_smbus_xfer(client->adapter, client->addr, client->flags,
				I2C_SMBUS_WRITE, ((u8 *)val_buf)[0],
				I2C_SMBUS_I2C_BLOCK_DATA, &data);
}


Those will be wrapped by a segment lock in the driver's read/write
functions.

>
> > But I somehow feel like I could have locking and wakeup handled by a mux's
> > select/deselect and have a simpler read function. However even if I
> > think I could have the driver register a mux even if there's actually
> > no muxed bus behind the chip, I'm missing how that would work if not
> > by exposing this in the DT bindings or by registering an
> > i2c_board_info, but this feels already too complicated to be worth it,
> > right ?
>
> This basically is exactly what is otherwise called an I2C gate. You have
> to do <something> to get I2C xfers safely past the gate, in this case
> wake the device up. So, that model isn't wrong.
>
> However, since the device wakes up on *any* action on SDA, would it not
> also work to make special I2C xfers, with a restart instead of a full
> stop-start after the "wakeup call". That is, if you can assume that the
> I2C adapter is flexible enough...
>
> I.e. something like this:
>
> static int
> sunrise_foo(struct sunrise_context *ctx)
> {
> 	unsigned char reg = 0x17;
> 	unsigned char buf[17];
> 	struct i2c_msg msg[3] = {
> 		{	/* wakeup */
> 			.addr = 0x68,
> 			.flags = I2C_M_RD | I2C_M_IGNORE_NAK,
> 			.len = 0,
> 		}, {	/* write register number */
> 			.addr = 0x68,
> 			.flags = 0,
> 			.len = 1,
> 			.buf = &reg,
> 		}, {	/* read register contents */
> 			.addr = 0x68,
> 			.flags = I2C_M_RD,
> 			.len = 17,
> 			.buf = buf,
> 		},
> 	};
> 	int ret;
>
> 	ret = i2c_transfer(ctx->adapter, msg, 3);

But this is interesting as well, as it seems the chip is flexible
enough to support repeated starts so this might works too.

Assuming this version works, which one is preferred in terms of
compatibility with the largest possible number of adapters (if it
makes any difference at all) ?

Documentation/i2c/smbus-protocol.rst seems to suggest smbus command
should be preferred:

If you write a driver for some I2C device, please try to use the SMBus
commands if at all possible (if the device uses only that subset of the
I2C protocol). This makes it possible to use the device driver on both
SMBus adapters and I2C adapters (the SMBus command set is automatically
translated to I2C on I2C adapters, but plain I2C commands can not be
handled at all on most pure SMBus adapters).

Thanks
   j

>
> 	...
>
> 	return ret;
> }
>
> Cheers,
> Peter
>
> > Thanks
> >    j
> >
> >>
> >> See Documentation/i2c/i2c-topology.rst for some discussion on the details
> >> of mux-locked and parent-locked muxes.
> >>
> >> I hope I make at least some sense...
> >>
> >> Cheers,
> >> Peter

[PATCH v3 3/3] iio: ABI: docs: Document Senseair Sunrise ABI

[Jacopo Mondi]

Add documentation for the sysfs attributes of the sunrise_co2 driver.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 .../sysfs-bus-iio-chemical-sunrise-co2        | 51 +++++++++++++++++++
 1 file changed, 51 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-chemical-sunrise-co2

diff --git a/Documentation/ABI/testing/sysfs-bus-iio-chemical-sunrise-co2 b/Documentation/ABI/testing/sysfs-bus-iio-chemical-sunrise-co2
new file mode 100644
index 000000000000..1a252f616652
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-bus-iio-chemical-sunrise-co2
@@ -0,0 +1,51 @@
+What:		/sys/bus/iio/devices/iio:deviceX/in_concentration_calibration_mode_available
+Date:		August 2021
+KernelVersion:
+Contact:	Jacopo Mondi <jacopo@jmondi.org>
+Description:
+		Reading returns the list of the possible calibration modes.
+		Available options:
+		- 'factory_calibration': Restore factory calibration
+		- 'background_calibration': Calibration using target value
+
+What:		/sys/bus/iio/devices/iio:deviceX/in_concentration_co2_calibration_mode
+Date:		August 2021
+KernelVersion:
+Contact:	Jacopo Mondi <jacopo@jmondi.org>
+Description:
+		Reading returns the currently selected calibration mode.
+		Writing sets the desired calibration mode to one of the values
+		returned by 'in_concentration_calibration_mode_available'
+
+What:		/sys/bus/iio/devices/iio:deviceX/in_concentration_co2_calibration
+Date:		August 2021
+KernelVersion:
+Contact:	Jacopo Mondi <jacopo@jmondi.org>
+Description:
+		Writing '1' triggers a calibration cycle according to the mode
+		set int 'in_concentration_co2_calibration_mode'.
+
+What:		/sys/bus/iio/devices/iio:deviceX/in_concentration_error_status_available
+Date:		August 2021
+KernelVersion:
+Contact:	Jacopo Mondi <jacopo@jmondi.org>
+Description:
+		Reading returns the list of possible chip error status.
+		Available options are:
+		- 'error_fatal': Analog front-end initialization error
+		- 'error_i2c': Read/write to non-existing register
+		- 'error_algorithm': Corrupted parameters
+		- 'error_calibration': Calibration has failed
+		- 'error_self_diagnostic': Internal interface failure
+		- 'error_out_of_range': Measured concentration out of scale
+		- 'error_memory': Error during memory operations
+		- 'error_no_measurement': Cleared at first measurement
+		- 'error_low_voltage': Sensor regulated voltage too low
+		- 'error_measurement_timeout': Unable to complete measurement
+
+What:		/sys/bus/iio/devices/iio:deviceX/in_concentration_co2_error_status
+Date:		August 2021
+KernelVersion:
+Contact:	Jacopo Mondi <jacopo@jmondi.org>
+Description:
+		Reading returns the current chip error status.
-- 
2.32.0

Re: [PATCH v3 3/3] iio: ABI: docs: Document Senseair Sunrise ABI

[Jonathan Cameron]

On Sun, 22 Aug 2021 20:49:27 +0200
Jacopo Mondi <jacopo@jmondi.org> wrote:

> Add documentation for the sysfs attributes of the sunrise_co2 driver.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  .../sysfs-bus-iio-chemical-sunrise-co2        | 51 +++++++++++++++++++
>  1 file changed, 51 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-chemical-sunrise-co2
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-iio-chemical-sunrise-co2 b/Documentation/ABI/testing/sysfs-bus-iio-chemical-sunrise-co2
> new file mode 100644
> index 000000000000..1a252f616652
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-bus-iio-chemical-sunrise-co2
> @@ -0,0 +1,51 @@
> +What:		/sys/bus/iio/devices/iio:deviceX/in_concentration_calibration_mode_available
> +Date:		August 2021
> +KernelVersion:
> +Contact:	Jacopo Mondi <jacopo@jmondi.org>
> +Description:
> +		Reading returns the list of the possible calibration modes.
> +		Available options:
> +		- 'factory_calibration': Restore factory calibration
> +		- 'background_calibration': Calibration using target value
> +
> +What:		/sys/bus/iio/devices/iio:deviceX/in_concentration_co2_calibration_mode
> +Date:		August 2021
> +KernelVersion:
> +Contact:	Jacopo Mondi <jacopo@jmondi.org>
> +Description:
> +		Reading returns the currently selected calibration mode.
> +		Writing sets the desired calibration mode to one of the values
> +		returned by 'in_concentration_calibration_mode_available'
> +
> +What:		/sys/bus/iio/devices/iio:deviceX/in_concentration_co2_calibration
> +Date:		August 2021
> +KernelVersion:
> +Contact:	Jacopo Mondi <jacopo@jmondi.org>
> +Description:
> +		Writing '1' triggers a calibration cycle according to the mode
> +		set int 'in_concentration_co2_calibration_mode'.
Why not just have two attributes:

	in_concentration_co2_calibration_factory
	in_concentration_co2_calibration_background
and have writing 1 to the appropriate one start that type of calibration?

Feels like that would be a simpler interface.

> +
> +What:		/sys/bus/iio/devices/iio:deviceX/in_concentration_error_status_available
> +Date:		August 2021
> +KernelVersion:
> +Contact:	Jacopo Mondi <jacopo@jmondi.org>
> +Description:
> +		Reading returns the list of possible chip error status.
> +		Available options are:
> +		- 'error_fatal': Analog front-end initialization error
> +		- 'error_i2c': Read/write to non-existing register
> +		- 'error_algorithm': Corrupted parameters
> +		- 'error_calibration': Calibration has failed
> +		- 'error_self_diagnostic': Internal interface failure
> +		- 'error_out_of_range': Measured concentration out of scale
> +		- 'error_memory': Error during memory operations
> +		- 'error_no_measurement': Cleared at first measurement
> +		- 'error_low_voltage': Sensor regulated voltage too low
> +		- 'error_measurement_timeout': Unable to complete measurement
> +
> +What:		/sys/bus/iio/devices/iio:deviceX/in_concentration_co2_error_status

Some of these are not 'technically' channels specific, so I'd argue this should be shared_by_all
and hence error_status only.

One day we will have a nice general way of reporting such errors, but that's not an IIO question
as such so we can probably cope with this.


> +Date:		August 2021
> +KernelVersion:
> +Contact:	Jacopo Mondi <jacopo@jmondi.org>
> +Description:
> +		Reading returns the current chip error status.

Re: [PATCH v3 3/3] iio: ABI: docs: Document Senseair Sunrise ABI

[Jacopo Mondi]

Hi Jonathan,

On Sun, Aug 29, 2021 at 05:57:48PM +0100, Jonathan Cameron wrote:
> On Sun, 22 Aug 2021 20:49:27 +0200
> Jacopo Mondi <jacopo@jmondi.org> wrote:
>
> > Add documentation for the sysfs attributes of the sunrise_co2 driver.
> >
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >  .../sysfs-bus-iio-chemical-sunrise-co2        | 51 +++++++++++++++++++
> >  1 file changed, 51 insertions(+)
> >  create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-chemical-sunrise-co2
> >
> > diff --git a/Documentation/ABI/testing/sysfs-bus-iio-chemical-sunrise-co2 b/Documentation/ABI/testing/sysfs-bus-iio-chemical-sunrise-co2
> > new file mode 100644
> > index 000000000000..1a252f616652
> > --- /dev/null
> > +++ b/Documentation/ABI/testing/sysfs-bus-iio-chemical-sunrise-co2
> > @@ -0,0 +1,51 @@
> > +What:		/sys/bus/iio/devices/iio:deviceX/in_concentration_calibration_mode_available
> > +Date:		August 2021
> > +KernelVersion:
> > +Contact:	Jacopo Mondi <jacopo@jmondi.org>
> > +Description:
> > +		Reading returns the list of the possible calibration modes.
> > +		Available options:
> > +		- 'factory_calibration': Restore factory calibration
> > +		- 'background_calibration': Calibration using target value
> > +
> > +What:		/sys/bus/iio/devices/iio:deviceX/in_concentration_co2_calibration_mode
> > +Date:		August 2021
> > +KernelVersion:
> > +Contact:	Jacopo Mondi <jacopo@jmondi.org>
> > +Description:
> > +		Reading returns the currently selected calibration mode.
> > +		Writing sets the desired calibration mode to one of the values
> > +		returned by 'in_concentration_calibration_mode_available'
> > +
> > +What:		/sys/bus/iio/devices/iio:deviceX/in_concentration_co2_calibration
> > +Date:		August 2021
> > +KernelVersion:
> > +Contact:	Jacopo Mondi <jacopo@jmondi.org>
> > +Description:
> > +		Writing '1' triggers a calibration cycle according to the mode
> > +		set int 'in_concentration_co2_calibration_mode'.
> Why not just have two attributes:
>
> 	in_concentration_co2_calibration_factory
> 	in_concentration_co2_calibration_background
> and have writing 1 to the appropriate one start that type of calibration?
>
> Feels like that would be a simpler interface.

Please see my reply in the driver's patch.
With an ack to the fact the chip supports 5 calibration modes and we
might end up with one attribute for each, I'll change.

>
> > +
> > +What:		/sys/bus/iio/devices/iio:deviceX/in_concentration_error_status_available
> > +Date:		August 2021
> > +KernelVersion:
> > +Contact:	Jacopo Mondi <jacopo@jmondi.org>
> > +Description:
> > +		Reading returns the list of possible chip error status.
> > +		Available options are:
> > +		- 'error_fatal': Analog front-end initialization error
> > +		- 'error_i2c': Read/write to non-existing register
> > +		- 'error_algorithm': Corrupted parameters
> > +		- 'error_calibration': Calibration has failed
> > +		- 'error_self_diagnostic': Internal interface failure
> > +		- 'error_out_of_range': Measured concentration out of scale
> > +		- 'error_memory': Error during memory operations
> > +		- 'error_no_measurement': Cleared at first measurement
> > +		- 'error_low_voltage': Sensor regulated voltage too low
> > +		- 'error_measurement_timeout': Unable to complete measurement
> > +
> > +What:		/sys/bus/iio/devices/iio:deviceX/in_concentration_co2_error_status
>
> Some of these are not 'technically' channels specific, so I'd argue this should be shared_by_all
> and hence error_status only.

You know, it's kind of a mixed bag of errors.

I thought as most apply to calibration/concentration it made sense to
have them separate, but I can change this to be shared indeed.

>
> One day we will have a nice general way of reporting such errors, but that's not an IIO question
> as such so we can probably cope with this.
>
>
> > +Date:		August 2021
> > +KernelVersion:
> > +Contact:	Jacopo Mondi <jacopo@jmondi.org>
> > +Description:
> > +		Reading returns the current chip error status.
>

Re: [PATCH v3 3/3] iio: ABI: docs: Document Senseair Sunrise ABI

[Jonathan Cameron]

On Mon, 30 Aug 2021 18:24:11 +0200
Jacopo Mondi <jacopo@jmondi.org> wrote:

> Hi Jonathan,
> 
> On Sun, Aug 29, 2021 at 05:57:48PM +0100, Jonathan Cameron wrote:
> > On Sun, 22 Aug 2021 20:49:27 +0200
> > Jacopo Mondi <jacopo@jmondi.org> wrote:
> >  
> > > Add documentation for the sysfs attributes of the sunrise_co2 driver.
> > >
> > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > > ---
> > >  .../sysfs-bus-iio-chemical-sunrise-co2        | 51 +++++++++++++++++++
> > >  1 file changed, 51 insertions(+)
> > >  create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-chemical-sunrise-co2
> > >
> > > diff --git a/Documentation/ABI/testing/sysfs-bus-iio-chemical-sunrise-co2 b/Documentation/ABI/testing/sysfs-bus-iio-chemical-sunrise-co2
> > > new file mode 100644
> > > index 000000000000..1a252f616652
> > > --- /dev/null
> > > +++ b/Documentation/ABI/testing/sysfs-bus-iio-chemical-sunrise-co2
> > > @@ -0,0 +1,51 @@
> > > +What:		/sys/bus/iio/devices/iio:deviceX/in_concentration_calibration_mode_available
> > > +Date:		August 2021
> > > +KernelVersion:
> > > +Contact:	Jacopo Mondi <jacopo@jmondi.org>
> > > +Description:
> > > +		Reading returns the list of the possible calibration modes.
> > > +		Available options:
> > > +		- 'factory_calibration': Restore factory calibration
> > > +		- 'background_calibration': Calibration using target value
> > > +
> > > +What:		/sys/bus/iio/devices/iio:deviceX/in_concentration_co2_calibration_mode
> > > +Date:		August 2021
> > > +KernelVersion:
> > > +Contact:	Jacopo Mondi <jacopo@jmondi.org>
> > > +Description:
> > > +		Reading returns the currently selected calibration mode.
> > > +		Writing sets the desired calibration mode to one of the values
> > > +		returned by 'in_concentration_calibration_mode_available'
> > > +
> > > +What:		/sys/bus/iio/devices/iio:deviceX/in_concentration_co2_calibration
> > > +Date:		August 2021
> > > +KernelVersion:
> > > +Contact:	Jacopo Mondi <jacopo@jmondi.org>
> > > +Description:
> > > +		Writing '1' triggers a calibration cycle according to the mode
> > > +		set int 'in_concentration_co2_calibration_mode'.  
> > Why not just have two attributes:
> >
> > 	in_concentration_co2_calibration_factory
> > 	in_concentration_co2_calibration_background
> > and have writing 1 to the appropriate one start that type of calibration?
> >
> > Feels like that would be a simpler interface.  
> 
> Please see my reply in the driver's patch.
> With an ack to the fact the chip supports 5 calibration modes and we
> might end up with one attribute for each, I'll change.

5 attrs is fine.

> 
> >  
> > > +
> > > +What:		/sys/bus/iio/devices/iio:deviceX/in_concentration_error_status_available
> > > +Date:		August 2021
> > > +KernelVersion:
> > > +Contact:	Jacopo Mondi <jacopo@jmondi.org>
> > > +Description:
> > > +		Reading returns the list of possible chip error status.
> > > +		Available options are:
> > > +		- 'error_fatal': Analog front-end initialization error
> > > +		- 'error_i2c': Read/write to non-existing register
> > > +		- 'error_algorithm': Corrupted parameters
> > > +		- 'error_calibration': Calibration has failed
> > > +		- 'error_self_diagnostic': Internal interface failure
> > > +		- 'error_out_of_range': Measured concentration out of scale
> > > +		- 'error_memory': Error during memory operations
> > > +		- 'error_no_measurement': Cleared at first measurement
> > > +		- 'error_low_voltage': Sensor regulated voltage too low
> > > +		- 'error_measurement_timeout': Unable to complete measurement
> > > +
> > > +What:		/sys/bus/iio/devices/iio:deviceX/in_concentration_co2_error_status  
> >
> > Some of these are not 'technically' channels specific, so I'd argue this should be shared_by_all
> > and hence error_status only.  
> 
> You know, it's kind of a mixed bag of errors.
> 
> I thought as most apply to calibration/concentration it made sense to
> have them separate, but I can change this to be shared indeed.

It's obscure and custom so I don't really care.  Up to you.
> 
> >
> > One day we will have a nice general way of reporting such errors, but that's not an IIO question
> > as such so we can probably cope with this.
> >
> >  
> > > +Date:		August 2021
> > > +KernelVersion:
> > > +Contact:	Jacopo Mondi <jacopo@jmondi.org>
> > > +Description:
> > > +		Reading returns the current chip error status.  
> >  

Re: [PATCH v3 0/3] iio: chemical: Add Senseair Sunrise CO2 sensor

[Andy Shevchenko]

On Sun, Aug 22, 2021 at 9:49 PM Jacopo Mondi <jacopo@jmondi.org> wrote:
>
> The driver supports continuous reads of temperature and CO2 concentration
> through two dedicated IIO channels. It also supports calibration and error
> inspection through the concentration channel ext_info.
>
> Minor changes in v3

Not sure, I have found bigger issues. See my comments in patch 2.
So, since it's obvious you haven't tested the patch and we are at rc7
I think you can take a few weeks of time to have a look and carefully
address all comments and to test.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 0/3] iio: chemical: Add Senseair Sunrise CO2 sensor

[Jacopo Mondi]

Andy,

On Sun, Aug 22, 2021 at 11:11:59PM +0300, Andy Shevchenko wrote:
> On Sun, Aug 22, 2021 at 9:49 PM Jacopo Mondi <jacopo@jmondi.org> wrote:
> >
> > The driver supports continuous reads of temperature and CO2 concentration
> > through two dedicated IIO channels. It also supports calibration and error
> > inspection through the concentration channel ext_info.
> >
> > Minor changes in v3
>
> Not sure, I have found bigger issues. See my comments in patch 2.
> So, since it's obvious you haven't tested the patch and we are at rc7
> I think you can take a few weeks of time to have a look and carefully
> address all comments and to test.

I'm sorry if the patches I sent out contain a leftover debug (the
pr_err) and I cannot explain how the :q commands ended up in the final
patch, most probably they come from me inspecting the generated patch
and being sloppy with vim on a sunday night, as they are not in the
git tree. They won't have even compiled otherwise, and it's obvious
where they come from if you've ever used vim.

Aprt from that, this is a v3, not v7, I've tested it several times, there's
no need to paternalize me as the only thing to fix is to re-add back
the ending commas in arrays declarations as a result of a comment from
you which I interpreted as a request from removing them. To me commas
at the end of an array declaration is mostly nit picking, which I
accept given the context and given I don't know the subsystem rules,
but please consider the last version of the patch mostly fixes minor
style issue which are questionable (lines that can span over 80 cols,
terminating commas, empty line at the beginning of the switch which I
liked but you didn't, 'Typically' in the bindings etc)).

So please consider that it took me a sunday
afternoon to please your preferences. If a debug printout leftover has
escaped I'm sorry, I've been sloppy. The ':q' in the final patch are
another obvious stupid mistake but the assumption that I've not tested
the patches it's really not appropriate.

As I appreciate your effort in review and I've silently gone over all
your comments, even the questionable ones, I don't think it's fair to
crucify someone which has sent a fully working driver with no major issues
(none that has been found so far at least) for two stupid leftovers.

I'll send v4 re-adding back the commas at the end of arrays.
>
> --
> With Best Regards,
> Andy Shevchenko

Re: [PATCH v3 0/3] iio: chemical: Add Senseair Sunrise CO2 sensor

[Andy Shevchenko]

On Mon, Aug 23, 2021 at 09:16:22AM +0200, Jacopo Mondi wrote:
> On Sun, Aug 22, 2021 at 11:11:59PM +0300, Andy Shevchenko wrote:
> > On Sun, Aug 22, 2021 at 9:49 PM Jacopo Mondi <jacopo@jmondi.org> wrote:
> > >
> > > The driver supports continuous reads of temperature and CO2 concentration
> > > through two dedicated IIO channels. It also supports calibration and error
> > > inspection through the concentration channel ext_info.
> > >
> > > Minor changes in v3
> >
> > Not sure, I have found bigger issues. See my comments in patch 2.
> > So, since it's obvious you haven't tested the patch and we are at rc7
> > I think you can take a few weeks of time to have a look and carefully
> > address all comments and to test.
> 
> I'm sorry if the patches I sent out contain a leftover debug (the
> pr_err) and I cannot explain how the :q commands ended up in the final
> patch, most probably they come from me inspecting the generated patch
> and being sloppy with vim on a sunday night, as they are not in the
> git tree. They won't have even compiled otherwise, and it's obvious
> where they come from if you've ever used vim.
> 
> Aprt from that, this is a v3, not v7, I've tested it several times, there's
> no need to paternalize me as the only thing to fix is to re-add back
> the ending commas in arrays declarations as a result of a comment from
> you which I interpreted as a request from removing them. To me commas
> at the end of an array declaration is mostly nit picking, which I
> accept given the context and given I don't know the subsystem rules,
> but please consider the last version of the patch mostly fixes minor
> style issue which are questionable (lines that can span over 80 cols,
> terminating commas, empty line at the beginning of the switch which I
> liked but you didn't, 'Typically' in the bindings etc)).
> 
> So please consider that it took me a sunday
> afternoon to please your preferences. If a debug printout leftover has
> escaped I'm sorry, I've been sloppy. The ':q' in the final patch are
> another obvious stupid mistake but the assumption that I've not tested
> the patches it's really not appropriate.
> 
> As I appreciate your effort in review and I've silently gone over all
> your comments, even the questionable ones, I don't think it's fair to
> crucify someone which has sent a fully working driver with no major issues
> (none that has been found so far at least) for two stupid leftovers.
> 
> I'll send v4 re-adding back the commas at the end of arrays.

Please, do.

-- 
With Best Regards,
Andy Shevchenko