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

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

[Jacopo Mondi]

Hello,
   this is a small driver for the Senseair Sunrise 006-0-0007 CO2
sensor.

The driver supports continuous reads of temperature and CO2 concentration
through two dedicated IIO channels.

While the driver is rather simple I'm not sure calibration is handled in
the correct way. In this version, at probe time, a check on the general
error register is made to verify if a calibration cycle is required.
The calibration takes a time in the order of a few seconds, and currently
can only happen at probe time.

Is there a mechanism available in the IIO framework to expose a trigger to have
userspace decide when the calibration has to happen ? In my understanding IIO
triggers are meant to trigger read events, using them for calibration purpose
seems not the right thing to do, or am I mistaken ?

Thanks
  j

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

 .../iio/chemical/senseair,sunrise.yaml        |  51 +++
 .../devicetree/bindings/vendor-prefixes.yaml  |   2 +
 MAINTAINERS                                   |   6 +
 drivers/iio/chemical/Kconfig                  |  10 +
 drivers/iio/chemical/Makefile                 |   1 +
 drivers/iio/chemical/sunrise.c                | 310 ++++++++++++++++++
 6 files changed, 380 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/chemical/senseair,sunrise.yaml
 create mode 100644 drivers/iio/chemical/sunrise.c

--
2.32.0

[PATCH 1/2] 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>
---
 .../iio/chemical/senseair,sunrise.yaml        | 51 +++++++++++++++++++
 .../devicetree/bindings/vendor-prefixes.yaml  |  2 +
 2 files changed, 53 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..b77196666187
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/chemical/senseair,sunrise.yaml
@@ -0,0 +1,51 @@
+# 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:
+    description: Phandle to the GPIO line connected to the nDRY pin. Active low.
+
+  en-gpios:
+    description: Phandle to the GPIO line connected to the EN pin. Active high.
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    i2c {
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      sunrise@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

[PATCH 2/2] 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.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 MAINTAINERS                    |   6 +
 drivers/iio/chemical/Kconfig   |  10 ++
 drivers/iio/chemical/Makefile  |   1 +
 drivers/iio/chemical/sunrise.c | 310 +++++++++++++++++++++++++++++++++
 4 files changed, 327 insertions(+)
 create mode 100644 drivers/iio/chemical/sunrise.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 90ca9df1d3c3..10a33fd2ccb8 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.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..c308379076ba 100644
--- a/drivers/iio/chemical/Kconfig
+++ b/drivers/iio/chemical/Kconfig
@@ -144,6 +144,16 @@ config SPS30
 	  To compile this driver as a module, choose M here: the module will
 	  be called sps30.
 
+config SUNRISE
+	tristate "Senseair Sunrise 006-0-0007 CO2 sensor"
+	depends on 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.
+
 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..24b8d63d05bd 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_SUNRISE) += sunrise.o
 obj-$(CONFIG_VZ89X)		+= vz89x.o
diff --git a/drivers/iio/chemical/sunrise.c b/drivers/iio/chemical/sunrise.c
new file mode 100644
index 000000000000..582633e86d1d
--- /dev/null
+++ b/drivers/iio/chemical/sunrise.c
@@ -0,0 +1,310 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * iio/co2/sunrise-006-0-0007.c
+ *
+ * Senseair Sunrise 006-0-0007 CO2 sensor driver.
+ *
+ * Copyright (C) 2021 Jacopo Mondi
+ *
+ * The driver supports continuous measurement operation mode only.
+ * TODO: Add support for controllable EN pin
+ * TODO: Add support for single-shot operations by using the nDRY pin.
+ */
+
+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/iio/iio.h>
+#include <linux/iopoll.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/of_device.h>
+
+#define DRIVER_NAME "sunrise"
+
+#define SUNRISE_ERROR_STATUS_REG		0x00
+#define SUNRISE_ERROR_OOR_ERR			BIT(5)
+#define SUNRISE_CO2_FILTERED_COMP_REG		0x06
+#define SUNRISE_CHIP_TEMPERATURE_REG		0x08
+#define SUNRISE_CALIBRATION_STATUS_REG		0x81
+#define SUNRISE_CALIBRATION_COMMAND_REG		0x082
+#define SUNRISE_CALIBRATION_BACKGROUND_CMD	0x7c06
+
+struct sunrise_dev {
+	struct i2c_client *client;
+	struct mutex lock;
+};
+
+static void sunrise_wakeup(struct i2c_client *client)
+{
+	/*
+	 * Ping to wakeup: the chip returns in low power state after 15 msec
+	 * without communications or after a complete read/write sequence.
+	 */
+	i2c_smbus_read_byte(client);
+}
+
+static int sunrise_read_byte(struct i2c_client *client, u8 reg, u8 *val)
+{
+	int ret;
+
+	sunrise_wakeup(client);
+	ret = i2c_smbus_read_byte_data(client, reg);
+	if (ret < 0) {
+		dev_err(&client->dev, "Read byte failed (%d)\n", ret);
+		return ret;
+	}
+
+	*val = ret;
+
+	return 0;
+}
+
+/* Accessors for readx_poll_timeout() function. */
+#define sunrise_readb_client(client, addr)	\
+({						\
+	u8 _val;				\
+	sunrise_read_byte(client, addr, &_val); \
+	_val;					\
+})
+#define sunrise_readb(addr)	sunrise_readb_client(client, addr)
+#define sunrise_poll_register(reg, val, cond)	\
+	readx_poll_timeout(sunrise_readb, reg, val, cond, 100, 0)
+
+static int sunrise_read_word(struct i2c_client *client, u8 reg, u16 *val)
+{
+	int ret;
+
+	sunrise_wakeup(client);
+	ret = i2c_smbus_read_word_data(client, reg);
+	if (ret < 0) {
+		dev_err(&client->dev, "Read word failed (%d)\n", ret);
+		return ret;
+	}
+
+	*val = be16_to_cpu(ret);
+
+	return 0;
+}
+
+static int sunrise_write_byte(struct i2c_client *client, u8 reg, u8 val)
+{
+	int ret;
+
+	sunrise_wakeup(client);
+	ret = i2c_smbus_write_byte_data(client, reg, val);
+	if (ret) {
+		dev_err(&client->dev, "Write byte failed (%d)\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int sunrise_write_word(struct i2c_client *client, u8 reg, u16 data)
+{
+	u16 be_data = cpu_to_be16(data);
+	int ret;
+
+	sunrise_wakeup(client);
+	ret = i2c_smbus_write_word_data(client, reg, be_data);
+	if (ret) {
+		dev_err(&client->dev, "Write word failed (%d)\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+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),
+		.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_co2_filtered(struct i2c_client *client, u16 *co2)
+{
+	int ret;
+
+	ret = sunrise_read_word(client, SUNRISE_CO2_FILTERED_COMP_REG, co2);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+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);
+	struct i2c_client *client = sunrise->client;
+	int ret;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+
+		mutex_lock(&sunrise->lock);
+
+		switch (chan->type) {
+		case IIO_CONCENTRATION: {
+			u16 co2;
+
+			ret = sunrise_read_co2_filtered(client, &co2);
+			*val = co2;
+			mutex_unlock(&sunrise->lock);
+
+			return ret ? ret : IIO_VAL_INT;
+		}
+
+		case IIO_TEMP: {
+			u16 temp;
+
+			ret = sunrise_read_word(client,
+						SUNRISE_CHIP_TEMPERATURE_REG,
+						&temp);
+			*val = temp;
+			mutex_unlock(&sunrise->lock);
+
+			return ret ? 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;
+	}
+
+	return 0;
+}
+
+static const struct iio_info sunrise_info = {
+	.read_raw = sunrise_read_raw,
+};
+
+static int sunrise_calibrate(struct i2c_client *client)
+{
+	u16 status;
+	int ret;
+
+	/* Verify if calibration is required. */
+	ret = sunrise_read_word(client, SUNRISE_ERROR_STATUS_REG, &status);
+	if (ret)
+		return ret;
+	if (!status)
+		return 0;
+
+	/*
+	 * A different error than "Out of range": report it in dmseg but
+	 * do not fail. However operations might be compromised. If OOR error
+	 * is set, proceed with a calibration cycle.
+	 */
+	if (status & ~SUNRISE_ERROR_OOR_ERR)
+		dev_err(&client->dev, "Error status: 0x%2x\n", status);
+	if (!(status & SUNRISE_ERROR_OOR_ERR))
+		return 0;
+
+	/* Run an ADC background calibration cycle. */
+	ret = sunrise_write_byte(client, SUNRISE_CALIBRATION_STATUS_REG, 0x00);
+	if (ret)
+		return ret;
+
+	ret = sunrise_write_word(client, SUNRISE_CALIBRATION_COMMAND_REG,
+				 SUNRISE_CALIBRATION_BACKGROUND_CMD);
+	if (ret)
+		return ret;
+
+	/*
+	 * Perform a read to trigger co2 calibration and poll the status bit
+	 * until calibration is completed.
+	 */
+	dev_dbg(&client->dev, "ABC backgrounf calibration in progress\n");
+
+	ret = sunrise_read_co2_filtered(client, &status);
+	if (ret)
+		return ret;
+
+	return sunrise_poll_register(SUNRISE_CALIBRATION_STATUS_REG, status,
+				     (status & BIT(5)));
+}
+
+static int sunrise_probe(struct i2c_client *client)
+{
+	struct sunrise_dev *sunrise;
+	struct iio_dev *iio_dev;
+	int ret;
+
+	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;
+	mutex_init(&sunrise->lock);
+
+	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;
+
+	ret = sunrise_calibrate(client);
+	if (ret) {
+		dev_err(&client->dev, "Calibration failed (%d)\n", ret);
+		return ret;
+	}
+
+	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 1/2] dt-bindings: iio: chemical: Document senseair,sunrise CO2 sensor

[Andy Shevchenko]

On Tue, Aug 17, 2021 at 05:49:50PM +0200, Jacopo Mondi wrote:
> Add documentation for the Senseair Sunrise 006-0-0007 CO2 NDIR sensor.

> +  ndry-gpios:
> +    description: Phandle to the GPIO line connected to the nDRY pin. Active low.
> +
> +  en-gpios:
> +    description: Phandle to the GPIO line connected to the EN pin. Active high.

Not sure you have to mention polarity. It can be changed on PCB level easily
and this bindings won't satisfy those (valid) changes.

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Tue, Aug 17, 2021 at 05:49:51PM +0200, Jacopo Mondi wrote:
> Add support for the Senseair Sunrise 006-0-0007 driver through the
> IIO subsystem.

Datasheet tag / link?

...

> +config SUNRISE
> +	tristate "Senseair Sunrise 006-0-0007 CO2 sensor"
> +	depends on 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.

Too generic name for configuration and module,

...

> + * iio/co2/sunrise-006-0-0007.c
> + *

Redundant noise.

...

> + * TODO: Add support for controllable EN pin
> + * TODO: Add support for single-shot operations by using the nDRY pin.

Ditto.

If it's not ready, then make it ready.

...


> +#include <linux/of_device.h>

Why? Perhaps mod_devicetable,h is what you had in mind.

...

> +	i2c_smbus_read_byte(client);

Can you use regmap I²C API?

...

> +#define sunrise_readb_client(client, addr)	\
> +({						\
> +	u8 _val;				\
> +	sunrise_read_byte(client, addr, &_val); \
> +	_val;					\
> +})
> +#define sunrise_readb(addr)	sunrise_readb_client(client, addr)

Drop ugly macros and use read_poll_timeout() below.

> +#define sunrise_poll_register(reg, val, cond)	\
> +	readx_poll_timeout(sunrise_readb, reg, val, cond, 100, 0)


...

> +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);
> +	struct i2c_client *client = sunrise->client;

+ u16 value;

> +	int ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +
> +		mutex_lock(&sunrise->lock);
> +
> +		switch (chan->type) {
> +		case IIO_CONCENTRATION: {
> +			u16 co2;

Reuse value.

> +			ret = sunrise_read_co2_filtered(client, &co2);
> +			*val = co2;
> +			mutex_unlock(&sunrise->lock);
> +
> +			return ret ? ret : IIO_VAL_INT;

Can be written as

			return ret ?: IIO_VAL_INT;

but up to maintainers.

> +		}
> +
> +		case IIO_TEMP: {
> +			u16 temp;

Reuse value.

> +			ret = sunrise_read_word(client,
> +						SUNRISE_CHIP_TEMPERATURE_REG,
> +						&temp);
> +			*val = temp;
> +			mutex_unlock(&sunrise->lock);
> +
> +			return ret ? 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;
> +	}
> +
> +	return 0;
> +}

...

> +	return sunrise_poll_register(SUNRISE_CALIBRATION_STATUS_REG, status,
> +				     (status & BIT(5)));

Too many parentheses.

-- 
With Best Regards,
Andy Shevchenko

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

[Matt Ranostay]

On Tue, Aug 17, 2021 at 8:49 AM Jacopo Mondi <jacopo@jmondi.org> wrote:
>
> Hello,
>    this is a small driver for the Senseair Sunrise 006-0-0007 CO2
> sensor.
>
> The driver supports continuous reads of temperature and CO2 concentration
> through two dedicated IIO channels.
>
> While the driver is rather simple I'm not sure calibration is handled in
> the correct way. In this version, at probe time, a check on the general
> error register is made to verify if a calibration cycle is required.
> The calibration takes a time in the order of a few seconds, and currently
> can only happen at probe time.
>
> Is there a mechanism available in the IIO framework to expose a trigger to have
> userspace decide when the calibration has to happen ? In my understanding IIO
> triggers are meant to trigger read events, using them for calibration purpose
> seems not the right thing to do, or am I mistaken ?

For sure you shouldn't use the trigger framework for that.

You should use an attribute group, e.g.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/iio/proximity/as3935.c?h=v5.14-rc6#n168

- Matt

>
> Thanks
>   j
>
> Jacopo Mondi (2):
>   dt-bindings: iio: chemical: Document senseair,sunrise CO2 sensor
>   iio: chemical: Add Senseair Sunrise 006-0-007 driver
>
>  .../iio/chemical/senseair,sunrise.yaml        |  51 +++
>  .../devicetree/bindings/vendor-prefixes.yaml  |   2 +
>  MAINTAINERS                                   |   6 +
>  drivers/iio/chemical/Kconfig                  |  10 +
>  drivers/iio/chemical/Makefile                 |   1 +
>  drivers/iio/chemical/sunrise.c                | 310 ++++++++++++++++++
>  6 files changed, 380 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/chemical/senseair,sunrise.yaml
>  create mode 100644 drivers/iio/chemical/sunrise.c
>
> --
> 2.32.0
>

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

[Jacopo Mondi]

Hi Andy,

On Tue, Aug 17, 2021 at 07:50:46PM +0300, Andy Shevchenko wrote:
> On Tue, Aug 17, 2021 at 05:49:50PM +0200, Jacopo Mondi wrote:
> > Add documentation for the Senseair Sunrise 006-0-0007 CO2 NDIR sensor.
>
> > +  ndry-gpios:
> > +    description: Phandle to the GPIO line connected to the nDRY pin. Active low.
> > +
> > +  en-gpios:
> > +    description: Phandle to the GPIO line connected to the EN pin. Active high.
>
> Not sure you have to mention polarity. It can be changed on PCB level easily
> and this bindings won't satisfy those (valid) changes.

Well, one can indeed invert the signal on the PCB, it's weird I see most of
the bindings reporting the pin's active polarity though...

I'll drop it.

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

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

[Jacopo Mondi]

Hi Andy, thanks for review

On Tue, Aug 17, 2021 at 07:58:38PM +0300, Andy Shevchenko wrote:
> On Tue, Aug 17, 2021 at 05:49:51PM +0200, Jacopo Mondi wrote:
> > Add support for the Senseair Sunrise 006-0-0007 driver through the
> > IIO subsystem.
>
> Datasheet tag / link?

It's reported in the bindings document, I can copy it here

>
> ...
>
> > +config SUNRISE
> > +	tristate "Senseair Sunrise 006-0-0007 CO2 sensor"
> > +	depends on 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.
>
> Too generic name for configuration and module,

I got the same feeling but at the same time SUNRISE_006_0_0007 seems a
bit too long. Would you suggest to go for that one ? Should I also
rename the .c file accordingly ?

>
> ...
>
> > + * iio/co2/sunrise-006-0-0007.c
> > + *
>
> Redundant noise.
>
> ...
>
> > + * TODO: Add support for controllable EN pin
> > + * TODO: Add support for single-shot operations by using the nDRY pin.
>
> Ditto.
>
> If it's not ready, then make it ready.

If I place myself in the perspective of someone interested in
verifying support for this chip in linux, I would first be happy I
found a driver for it, then I would even be happier if I got a clear
notice of what features the driver supports and which ones are missing.
Even more so if I look at bindings and find mention of two GPIO lines
which I don't see handled in the driver. A TODO note would make it
clear that those features are missing.

The "make it ready" it's a little bit a non-sense to me, can you count
how many drivers support -all- the features a device provides ? In the
test board I'm using those lines are not even wired, should I throw
around code I cannot even test ?

>
> ...
>
>
> > +#include <linux/of_device.h>
>
> Why? Perhaps mod_devicetable,h is what you had in mind.
>

Ah yes indeed. I got it wrong in so many places in the past !

> ...
>
> > +	i2c_smbus_read_byte(client);
>
> Can you use regmap I²C API?
>

Do you think it's worth it ?
I admit I never used regmap API so far, but I always got the
impression that for such simple devices it's a bit an overkill. What
benefit would it bring here ?

> ...
>
> > +#define sunrise_readb_client(client, addr)	\
> > +({						\
> > +	u8 _val;				\
> > +	sunrise_read_byte(client, addr, &_val); \
> > +	_val;					\
> > +})
> > +#define sunrise_readb(addr)	sunrise_readb_client(client, addr)
>
> Drop ugly macros and use read_poll_timeout() below.

Ah! Thanks a lot, I missed the fact read_poll_timeout() accepts
a variable number of arguments...
However, the API of the driver i2c read function is:

        int sunrise_read_byte(client, reg, *val);

while read_poll_timeout() expects

        int read(client, reg);

I know the answer already: change your i2c accessor to comply.
As it is only used by read_poll_timeout() I'll do so :)

>
> > +#define sunrise_poll_register(reg, val, cond)	\
> > +	readx_poll_timeout(sunrise_readb, reg, val, cond, 100, 0)
>
>
> ...
>
> > +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);
> > +	struct i2c_client *client = sunrise->client;
>
> + u16 value;
>
> > +	int ret;
> > +
> > +	switch (mask) {
> > +	case IIO_CHAN_INFO_RAW:
> > +
> > +		mutex_lock(&sunrise->lock);
> > +
> > +		switch (chan->type) {
> > +		case IIO_CONCENTRATION: {
> > +			u16 co2;
>
> Reuse value.
>

Ack

> > +			ret = sunrise_read_co2_filtered(client, &co2);
> > +			*val = co2;
> > +			mutex_unlock(&sunrise->lock);
> > +
> > +			return ret ? ret : IIO_VAL_INT;
>
> Can be written as
>
> 			return ret ?: IIO_VAL_INT;
>
> but up to maintainers.
>

You know I never saw this syntax before ? :D
I'll use it!

> > +		}
> > +
> > +		case IIO_TEMP: {
> > +			u16 temp;
>
> Reuse value.
>

ack

> > +			ret = sunrise_read_word(client,
> > +						SUNRISE_CHIP_TEMPERATURE_REG,
> > +						&temp);
> > +			*val = temp;
> > +			mutex_unlock(&sunrise->lock);
> > +
> > +			return ret ? 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;
> > +	}
> > +
> > +	return 0;
> > +}
>
> ...
>
> > +	return sunrise_poll_register(SUNRISE_CALIBRATION_STATUS_REG, status,
> > +				     (status & BIT(5)));
>
> Too many parentheses.

Right!

Thanks for review!

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

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

[Jacopo Mondi]

Hi Matt,

On Tue, Aug 17, 2021 at 10:01:40AM -0700, Matt Ranostay wrote:
> On Tue, Aug 17, 2021 at 8:49 AM Jacopo Mondi <jacopo@jmondi.org> wrote:
> >
> > Hello,
> >    this is a small driver for the Senseair Sunrise 006-0-0007 CO2
> > sensor.
> >
> > The driver supports continuous reads of temperature and CO2 concentration
> > through two dedicated IIO channels.
> >
> > While the driver is rather simple I'm not sure calibration is handled in
> > the correct way. In this version, at probe time, a check on the general
> > error register is made to verify if a calibration cycle is required.
> > The calibration takes a time in the order of a few seconds, and currently
> > can only happen at probe time.
> >
> > Is there a mechanism available in the IIO framework to expose a trigger to have
> > userspace decide when the calibration has to happen ? In my understanding IIO
> > triggers are meant to trigger read events, using them for calibration purpose
> > seems not the right thing to do, or am I mistaken ?
>
> For sure you shouldn't use the trigger framework for that.
>
> You should use an attribute group, e.g.
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/iio/proximity/as3935.c?h=v5.14-rc6#n168

Thanks for the pointer! I'll consider that for v2!

>
> - Matt
>
> >
> > Thanks
> >   j
> >
> > Jacopo Mondi (2):
> >   dt-bindings: iio: chemical: Document senseair,sunrise CO2 sensor
> >   iio: chemical: Add Senseair Sunrise 006-0-007 driver
> >
> >  .../iio/chemical/senseair,sunrise.yaml        |  51 +++
> >  .../devicetree/bindings/vendor-prefixes.yaml  |   2 +
> >  MAINTAINERS                                   |   6 +
> >  drivers/iio/chemical/Kconfig                  |  10 +
> >  drivers/iio/chemical/Makefile                 |   1 +
> >  drivers/iio/chemical/sunrise.c                | 310 ++++++++++++++++++
> >  6 files changed, 380 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/iio/chemical/senseair,sunrise.yaml
> >  create mode 100644 drivers/iio/chemical/sunrise.c
> >
> > --
> > 2.32.0
> >

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

[Andy Shevchenko]

On Wed, Aug 18, 2021 at 10:02:13AM +0200, Jacopo Mondi wrote:
> On Tue, Aug 17, 2021 at 07:58:38PM +0300, Andy Shevchenko wrote:
> > On Tue, Aug 17, 2021 at 05:49:51PM +0200, Jacopo Mondi wrote:
> > > Add support for the Senseair Sunrise 006-0-0007 driver through the
> > > IIO subsystem.
> >
> > Datasheet tag / link?
> 
> It's reported in the bindings document, I can copy it here

Yes, please, as a tag

Datasheet: ...
Signed-of-by: ...

...

> > > +config SUNRISE
> >
> > Too generic name for configuration and module,
> 
> I got the same feeling but at the same time SUNRISE_006_0_0007 seems a
> bit too long. Would you suggest to go for that one ? Should I also
> rename the .c file accordingly ?

I would go with something like sunrise_air_6 or whatever part of the model.
I haven't read datasheet to propose better naming.

Also possible to have something like sunrise_air_meter.c (and corresponding
Kconfig).

...

> > > + * TODO: Add support for controllable EN pin
> > > + * TODO: Add support for single-shot operations by using the nDRY pin.
> >
> > Ditto.
> >
> > If it's not ready, then make it ready.
> 
> If I place myself in the perspective of someone interested in
> verifying support for this chip in linux, I would first be happy I
> found a driver for it, then I would even be happier if I got a clear
> notice of what features the driver supports and which ones are missing.
> Even more so if I look at bindings and find mention of two GPIO lines
> which I don't see handled in the driver. A TODO note would make it
> clear that those features are missing.
> 
> The "make it ready" it's a little bit a non-sense to me, can you count
> how many drivers support -all- the features a device provides ? In the
> test board I'm using those lines are not even wired, should I throw
> around code I cannot even test ?

My point is that TODO means that somebody is working or will work on this.
Is it the case? Then my comment applies, if not, replace this with something more
realistic, like
 * Feature list that driver does NOT support (yet):
 *  1) ...
 *  2) ...

...

> > > +	i2c_smbus_read_byte(client);
> >
> > Can you use regmap I²C API?
> 
> Do you think it's worth it ?
> I admit I never used regmap API so far, but I always got the
> impression that for such simple devices it's a bit an overkill. What
> benefit would it bring here ?

Some of nice features for debugging and tracing are in the generic support for
free. On top if even this will be part of multi-function device than you won;t
need to rewrite the driver. I would definitely go for it.

...

> > > +			return ret ? ret : IIO_VAL_INT;
> >
> > Can be written as
> >
> > 			return ret ?: IIO_VAL_INT;
> >
> > but up to maintainers.
> >
> 
> You know I never saw this syntax before ? :D
> I'll use it!

It's the C standard :-)

-- 
With Best Regards,
Andy Shevchenko

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

[Rob Herring]

On Wed, Aug 18, 2021 at 09:29:43AM +0200, Jacopo Mondi wrote:
> Hi Andy,
> 
> On Tue, Aug 17, 2021 at 07:50:46PM +0300, Andy Shevchenko wrote:
> > On Tue, Aug 17, 2021 at 05:49:50PM +0200, Jacopo Mondi wrote:
> > > Add documentation for the Senseair Sunrise 006-0-0007 CO2 NDIR sensor.
> >
> > > +  ndry-gpios:
> > > +    description: Phandle to the GPIO line connected to the nDRY pin. Active low.
> > > +
> > > +  en-gpios:
> > > +    description: Phandle to the GPIO line connected to the EN pin. Active high.
> >
> > Not sure you have to mention polarity. It can be changed on PCB level easily
> > and this bindings won't satisfy those (valid) changes.
> 
> Well, one can indeed invert the signal on the PCB, it's weird I see most of
> the bindings reporting the pin's active polarity though...
> 
> I'll drop it.

I'd keep it. It's documenting the pin on the h/w. You can prefix with 
'Typically' if you want.

Rob

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

[Rob Herring]

On Tue, Aug 17, 2021 at 05:49:50PM +0200, Jacopo Mondi wrote:
> Add documentation for the Senseair Sunrise 006-0-0007 CO2 NDIR sensor.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  .../iio/chemical/senseair,sunrise.yaml        | 51 +++++++++++++++++++
>  .../devicetree/bindings/vendor-prefixes.yaml  |  2 +
>  2 files changed, 53 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..b77196666187
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/chemical/senseair,sunrise.yaml
> @@ -0,0 +1,51 @@
> +# 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:
> +    description: Phandle to the GPIO line connected to the nDRY pin. Active low.
> +
> +  en-gpios:
> +    description: Phandle to the GPIO line connected to the EN pin. Active high.

You need to define how many (maxItems: 1).

> +
> +required:
> +  - compatible
> +  - reg
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    i2c {
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +
> +      sunrise@68 {

'co2-sensor' perhaps.

> +        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 2/2] iio: chemical: Add Senseair Sunrise 006-0-007 driver

[Jonathan Cameron]

> Ack
> 
> > > +			ret = sunrise_read_co2_filtered(client, &co2);
> > > +			*val = co2;
> > > +			mutex_unlock(&sunrise->lock);
> > > +
> > > +			return ret ? ret : IIO_VAL_INT;  
> >
> > Can be written as
> >
> > 			return ret ?: IIO_VAL_INT;
> >
> > but up to maintainers.
> >  
> 
> You know I never saw this syntax before ? :D
> I'll use it!
I'm rather late to the party, but I'd keep all error handling in driver of the form

	if (ret)
		return ret;

	return IIO_VAL_INT;

That means lazy reviewers like me have one less thing to think about ;)