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

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

[Jacopo Mondi]

v2 of the driver for Senseair Sunrise 006-0-0007 CO2 sensor.

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.

Changelog:

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                  |  10 +
 drivers/iio/chemical/Makefile                 |   1 +
 drivers/iio/chemical/sunrise_co2.c            | 458 ++++++++++++++++++
 7 files changed, 581 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 v2 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>
---
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        | 53 +++++++++++++++++++
 .../devicetree/bindings/vendor-prefixes.yaml  |  2 +
 2 files changed, 55 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..d776adcb0117
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/chemical/senseair,sunrise.yaml
@@ -0,0 +1,53 @@
+# 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. Active low.
+
+  en-gpios:
+    maxItems: 1
+    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>;
+
+      co2-sensor: 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

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

[Andy Shevchenko]

On Fri, Aug 20, 2021 at 4:38 PM Jacopo Mondi <jacopo@jmondi.org> wrote:
>
> Add documentation for the Senseair Sunrise 006-0-0007 CO2 NDIR sensor.

...

> +    description: Phandle to the GPIO line connected to the nDRY pin. Active low.

> +    description: Phandle to the GPIO line connected to the EN pin. Active high.

As Rob suggested, adding the word "typically" would resolve my claim.

-- 
With Best Regards,
Andy Shevchenko

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

[Rob Herring]

On Fri, 20 Aug 2021 15:38:19 +0200, Jacopo Mondi wrote:
> Add documentation for the Senseair Sunrise 006-0-0007 CO2 NDIR sensor.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
> 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        | 53 +++++++++++++++++++
>  .../devicetree/bindings/vendor-prefixes.yaml  |  2 +
>  2 files changed, 55 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/chemical/senseair,sunrise.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Error: Documentation/devicetree/bindings/iio/chemical/senseair,sunrise.example.dts:23.21-22 syntax error
FATAL ERROR: Unable to parse input tree
make[1]: *** [scripts/Makefile.lib:380: Documentation/devicetree/bindings/iio/chemical/senseair,sunrise.example.dt.yaml] Error 1
make[1]: *** Waiting for unfinished jobs....
make: *** [Makefile:1419: dt_binding_check] Error 2

doc reference errors (make refcheckdocs):

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

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

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

pip3 install dtschema --upgrade

Please check and re-submit.

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

[Jacopo Mondi]

On Fri, Aug 20, 2021 at 02:46:12PM -0500, Rob Herring wrote:
> On Fri, 20 Aug 2021 15:38:19 +0200, Jacopo Mondi wrote:
> > Add documentation for the Senseair Sunrise 006-0-0007 CO2 NDIR sensor.
> >
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> > 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
> >

Labels cannot contain '-'.

I'll make this

        sunrise: co2-sensor@68

My bad and sorry for not running dt_binding_check, I thought the
changes were trivial enough.

Thanks
   j

> > ---
> >  .../iio/chemical/senseair,sunrise.yaml        | 53 +++++++++++++++++++
> >  .../devicetree/bindings/vendor-prefixes.yaml  |  2 +
> >  2 files changed, 55 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/iio/chemical/senseair,sunrise.yaml
> >
>
> My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
> on your patch (DT_CHECKER_FLAGS is new in v5.13):
>
> yamllint warnings/errors:
>
> dtschema/dtc warnings/errors:
> Error: Documentation/devicetree/bindings/iio/chemical/senseair,sunrise.example.dts:23.21-22 syntax error
> FATAL ERROR: Unable to parse input tree
> make[1]: *** [scripts/Makefile.lib:380: Documentation/devicetree/bindings/iio/chemical/senseair,sunrise.example.dt.yaml] Error 1
> make[1]: *** Waiting for unfinished jobs....
> make: *** [Makefile:1419: dt_binding_check] Error 2
>
> doc reference errors (make refcheckdocs):
>
> See https://patchwork.ozlabs.org/patch/1519042
>
> This check can fail if there are any dependencies. The base for a patch
> series is generally the most recent rc1.
>
> If you already ran 'make dt_binding_check' and didn't see the above
> error(s), then make sure 'yamllint' is installed and dt-schema is up to
> date:
>
> pip3 install dtschema --upgrade
>
> Please check and re-submit.
>

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

[Jonathan Cameron]

On Sun, 22 Aug 2021 18:04:44 +0200
Jacopo Mondi <jacopo@jmondi.org> wrote:

> On Fri, Aug 20, 2021 at 02:46:12PM -0500, Rob Herring wrote:
> > On Fri, 20 Aug 2021 15:38:19 +0200, Jacopo Mondi wrote:  
> > > Add documentation for the Senseair Sunrise 006-0-0007 CO2 NDIR sensor.
> > >
> > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > > ---
> > > 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
> > >  
> 
> Labels cannot contain '-'.
> 
> I'll make this
> 
>         sunrise: co2-sensor@68

Why label at all?  Might be handy on a real board, but example doesn't need it.

Jonathan

> 
> My bad and sorry for not running dt_binding_check, I thought the
> changes were trivial enough.
> 
> Thanks
>    j
> 
> > > ---
> > >  .../iio/chemical/senseair,sunrise.yaml        | 53 +++++++++++++++++++
> > >  .../devicetree/bindings/vendor-prefixes.yaml  |  2 +
> > >  2 files changed, 55 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/iio/chemical/senseair,sunrise.yaml
> > >  
> >
> > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
> > on your patch (DT_CHECKER_FLAGS is new in v5.13):
> >
> > yamllint warnings/errors:
> >
> > dtschema/dtc warnings/errors:
> > Error: Documentation/devicetree/bindings/iio/chemical/senseair,sunrise.example.dts:23.21-22 syntax error
> > FATAL ERROR: Unable to parse input tree
> > make[1]: *** [scripts/Makefile.lib:380: Documentation/devicetree/bindings/iio/chemical/senseair,sunrise.example.dt.yaml] Error 1
> > make[1]: *** Waiting for unfinished jobs....
> > make: *** [Makefile:1419: dt_binding_check] Error 2
> >
> > doc reference errors (make refcheckdocs):
> >
> > See https://patchwork.ozlabs.org/patch/1519042
> >
> > This check can fail if there are any dependencies. The base for a patch
> > series is generally the most recent rc1.
> >
> > If you already ran 'make dt_binding_check' and didn't see the above
> > error(s), then make sure 'yamllint' is installed and dt-schema is up to
> > date:
> >
> > pip3 install dtschema --upgrade
> >
> > Please check and re-submit.
> >  

[PATCH v2 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       |  10 +
 drivers/iio/chemical/Makefile      |   1 +
 drivers/iio/chemical/sunrise_co2.c | 458 +++++++++++++++++++++++++++++
 4 files changed, 475 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..2c7f526dfa2d 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 SENSEAIR_SUNRISE_CO2
+	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_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..39ed8fae0963
--- /dev/null
+++ b/drivers/iio/chemical/sunrise_co2.c
@@ -0,0 +1,458 @@
+// 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:
+ * - support for controllable EN pin
+ * - support for single-shot operations using the nDRY pin.
+ * - ABC/target calibration
+ */
+
+#include <linux/i2c.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+#include <linux/kernel.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/regmap.h>
+#include <linux/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		30000000
+
+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 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 -EINVAL;
+
+	if (!calibrate)
+		return 0;
+
+	ret = sunrise_calibrate(sunrise);
+
+	return ret ?: 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);
+	ssize_t len = 0;
+	u16 value;
+	int ret;
+	u8 i;
+
+	ret = sunrise_read_word(sunrise, SUNRISE_ERROR_STATUS_REG, &value);
+	if (ret)
+		return -EINVAL;
+
+	for (i = 0; i < ARRAY_SIZE(error_codes); ++i) {
+		if (!(value & BIT(error_codes[i])))
+			continue;
+
+		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;
+	}
+
+	return 0;
+}
+
+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 v2 2/3] iio: chemical: Add Sensteair Sunrise 006-0-007 driver

[Andy Shevchenko]

On Fri, Aug 20, 2021 at 4:38 PM Jacopo Mondi <jacopo@jmondi.org> wrote:
>
> Add support for the Senseair Sunrise 006-0-0007 driver through the
> IIO subsystem.

...

> +config SENSEAIR_SUNRISE_CO2
> +       tristate "Senseair Sunrise 006-0-0007 CO2 sensor"

> +       depends on I2C

Actually

select REGMAP_I2C

...

> + * List of features not yet supported by the driver:
> + * - support for controllable EN pin

To avoid tautology

* - controllable EN pin


> + * - support for single-shot operations using the nDRY pin.

Ditto.

* - single-shot operations using the nDRY pin.

> + * - ABC/target calibration

...

> +#include <linux/i2c.h>

> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>

Can you move this as a separate group...

> +#include <linux/kernel.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/regmap.h>
> +#include <linux/sysfs.h>

...here ?

...

> +#define SUNRISE_CALIBRATION_TIMEOUT_US         30000000

30 * USEC_PER_SEC ?

...

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

I'm wondering if there is a better way to perform this.

> +       i2c_smbus_xfer(client->adapter, client->addr, I2C_M_IGNORE_NAK,
> +                      I2C_SMBUS_WRITE, 0, I2C_SMBUS_QUICK, NULL);
> +}

...

> +               dev_err(sunrise->dev, "Read word failed: reg 0x%2x (%d)\n",
> +                       reg, ret);

One line?

...

> +               dev_err(sunrise->dev, "Write byte failed: reg 0x%2x (%d)\n",
> +                       reg, ret);

One line?

...

> +               dev_err(sunrise->dev, "Write word failed: reg 0x%2x (%d)\n",
> +                       reg, ret);

One line?

...

> +       /* Write calibration command and poll the calibration status bit. */

Write a calibration

...

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

Shadowed return code.

> +       if (!calibrate)
> +               return 0;
> +
> +       ret = sunrise_calibrate(sunrise);
> +
> +       return ret ?: len;

In this case

  if (ret)
    return ret;

return len;

will look more natural.

> +}

...

> +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);
> +       ssize_t len = 0;
> +       u16 value;
> +       int ret;
> +       u8 i;
> +
> +       ret = sunrise_read_word(sunrise, SUNRISE_ERROR_STATUS_REG, &value);
> +       if (ret)
> +               return -EINVAL;

> +       for (i = 0; i < ARRAY_SIZE(error_codes); ++i) {
> +               if (!(value & BIT(error_codes[i])))
> +                       continue;

for_each_set_bit()

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

One line?

> +       }
> +
> +       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),

One line?

> +       IIO_ENUM_AVAILABLE("calibration_mode",
> +                          &sunrise_calibration_modes_enum),

One line?

> +       /* Error statuses. */
> +       {
> +               .name = "error_status",
> +               .read = sunrise_error_status_read,
> +               .shared = IIO_SEPARATE,
> +       },
> +       IIO_ENUM_AVAILABLE("error_status", &sunrise_error_statuses_enum),

> +       {},

No comma for terminator entries.

> +};

...

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

> +

Redundant blank line.

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

You don't need {} anymore.

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

Ditto.

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

Dead code?

> +}


-- 
With Best Regards,
Andy Shevchenko

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

[Jacopo Mondi]

Hi Andy,
   thanks for the quick review

On Fri, Aug 20, 2021 at 05:21:30PM +0300, Andy Shevchenko wrote:
> On Fri, Aug 20, 2021 at 4:38 PM Jacopo Mondi <jacopo@jmondi.org> wrote:
> >
> > Add support for the Senseair Sunrise 006-0-0007 driver through the
> > IIO subsystem.
>
> ...
>
> > +config SENSEAIR_SUNRISE_CO2
> > +       tristate "Senseair Sunrise 006-0-0007 CO2 sensor"
>
> > +       depends on I2C
>
> Actually
>
> select REGMAP_I2C

Ugh, thanks

>
> ...
>
> > + * List of features not yet supported by the driver:
> > + * - support for controllable EN pin
>
> To avoid tautology
>
> * - controllable EN pin
>
>
> > + * - support for single-shot operations using the nDRY pin.
>
> Ditto.
>
> * - single-shot operations using the nDRY pin.

Right :)

>
> > + * - ABC/target calibration
>
> ...
>
> > +#include <linux/i2c.h>
>
> > +#include <linux/iio/iio.h>
> > +#include <linux/iio/sysfs.h>
>
> Can you move this as a separate group...
>
> > +#include <linux/kernel.h>
> > +#include <linux/mod_devicetable.h>
> > +#include <linux/module.h>
> > +#include <linux/mutex.h>
> > +#include <linux/regmap.h>
> > +#include <linux/sysfs.h>
>
> ...here ?

Ah, is this customary in the subsystem ?

>
> ...
>
> > +#define SUNRISE_CALIBRATION_TIMEOUT_US         30000000
>
> 30 * USEC_PER_SEC ?
>

ack

> ...
>
> > +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.
> > +        */
>
> I'm wondering if there is a better way to perform this.
>

Do you mean using a different API instead of i2c_smbus_xfer()?

In v1 I had smbus_read_byte(), which expects a reply. The sensor sends
a NACK so the communication is interrupted and the effect is actually
the same but it seemed a little abuse to me.

The i2c documentation describes

SMBus Quick Command
===================

This sends a single bit to the device, at the place of the Rd/Wr bit::

  S Addr Rd/Wr [A] P

Functionality flag: I2C_FUNC_SMBUS_QUICK

Which is exactly what I want, but there's no API (or I have found
none) to perform that (I haven't looked at logs, but I suspect it has
been removed?). So I went and call i2c_smbus_xfer() by hand with the
opportune flags. It feels kind of a layering violation, as ideally
this should go through an i2c_smbus_send_bit() or something, but if
it's not there there might be a reason ?


> > +       i2c_smbus_xfer(client->adapter, client->addr, I2C_M_IGNORE_NAK,
> > +                      I2C_SMBUS_WRITE, 0, I2C_SMBUS_QUICK, NULL);
> > +}
>
> ...
>
> > +               dev_err(sunrise->dev, "Read word failed: reg 0x%2x (%d)\n",
> > +                       reg, ret);
>
> One line?

Goes over 80, as all the other identical comments below.

>
> ...
>
> > +               dev_err(sunrise->dev, "Write byte failed: reg 0x%2x (%d)\n",
> > +                       reg, ret);
>
> One line?
>
> ...
>
> > +               dev_err(sunrise->dev, "Write word failed: reg 0x%2x (%d)\n",
> > +                       reg, ret);
>
> One line?
>
> ...
>
> > +       /* Write calibration command and poll the calibration status bit. */
>
> Write a calibration

Ack

>
> ...
>
> > +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 -EINVAL;
>
> Shadowed return code.
>

ok

> > +       if (!calibrate)
> > +               return 0;
> > +
> > +       ret = sunrise_calibrate(sunrise);
> > +
> > +       return ret ?: len;
>
> In this case
>
>   if (ret)
>     return ret;
>
> return len;
>
> will look more natural.

I had this and I switched before sending. Matter of tastes I guess.
I actually changed this as I thought it would have pleased you :)


>
> > +}
>
> ...
>
> > +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);
> > +       ssize_t len = 0;
> > +       u16 value;
> > +       int ret;
> > +       u8 i;
> > +
> > +       ret = sunrise_read_word(sunrise, SUNRISE_ERROR_STATUS_REG, &value);
> > +       if (ret)
> > +               return -EINVAL;
>
> > +       for (i = 0; i < ARRAY_SIZE(error_codes); ++i) {
> > +               if (!(value & BIT(error_codes[i])))
> > +                       continue;
>
> for_each_set_bit()

only 12 bits are valid, the top 4 are undocumented. They -should- be
zeroed but who knows.

>
> > +               len += sysfs_emit_at(buf, len, "%s ",
> > +                                    sunrise_error_statuses[i]);
>
> One line?

way more than 80 cols!
I know there were discussions on dropping this as an hard limit, but
when possible shouldn't we strive to respect it ?

>
> > +       }
> > +
> > +       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),
>
> One line?
>
> > +       IIO_ENUM_AVAILABLE("calibration_mode",
> > +                          &sunrise_calibration_modes_enum),
>
> One line?
>
> > +       /* Error statuses. */
> > +       {
> > +               .name = "error_status",
> > +               .read = sunrise_error_status_read,
> > +               .shared = IIO_SEPARATE,
> > +       },
> > +       IIO_ENUM_AVAILABLE("error_status", &sunrise_error_statuses_enum),
>
> > +       {},
>
> No comma for terminator entries.
>
> > +};
>
> ...
>
> > +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:
>
> > +
>
> Redundant blank line.
>
> > +               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;
> > +               }
>
> You don't need {} anymore.

Correct!

>
> > +               case IIO_TEMP: {
> > +                       ret = sunrise_read_word(sunrise,
> > +                                               SUNRISE_CHIP_TEMPERATURE_REG,
> > +                                               &value);
> > +                       *val = value;
> > +                       mutex_unlock(&sunrise->lock);
> > +
> > +                       return ret ?: IIO_VAL_INT;
> > +               }
>
> Ditto.
>
> > +               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;
>
> Dead code?

It is, but it seems nicer :) Should I drop it ? I recall I've been
asked to add it to the end of switch() in other drivers in this
subsystem...

Thanks again for the review
    j

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

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

[Andy Shevchenko]

On Fri, Aug 20, 2021 at 5:42 PM Jacopo Mondi <jacopo@jmondi.org> wrote:

...

> > > +#include <linux/i2c.h>
> >
> > > +#include <linux/iio/iio.h>
> > > +#include <linux/iio/sysfs.h>
> >
> > Can you move this as a separate group...
> >
> > > +#include <linux/kernel.h>
> > > +#include <linux/mod_devicetable.h>
> > > +#include <linux/module.h>
> > > +#include <linux/mutex.h>
> > > +#include <linux/regmap.h>
> > > +#include <linux/sysfs.h>
> >
> > ...here ?
>
> Ah, is this customary in the subsystem ?

When it's a group of headers for a certain subsystem it makes it clear
that the driver belongs to it.

...

> > > +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.
> > > +        */
> >
> > I'm wondering if there is a better way to perform this.
>
> Do you mean using a different API instead of i2c_smbus_xfer()?

I meant on hw level.

...

> > > +               dev_err(sunrise->dev, "Read word failed: reg 0x%2x (%d)\n",
> > > +                       reg, ret);
> >
> > One line?
>
> Goes over 80, as all the other identical comments below.

It's still less than 90, so go for it!

...

> > > +       ret = sunrise_calibrate(sunrise);
> > > +
> > > +       return ret ?: len;
> >
> > In this case
> >
> >   if (ret)
> >     return ret;
> >
> > return len;
> >
> > will look more natural.
>
> I had this and I switched before sending. Matter of tastes I guess.
> I actually changed this as I thought it would have pleased you :)

Use your common sense. Not every place where you can put it really needs it.
Either way you choose is okay because of style, but to be sure ask the
maintainer :-)

...

> > > +       for (i = 0; i < ARRAY_SIZE(error_codes); ++i) {
> > > +               if (!(value & BIT(error_codes[i])))
> > > +                       continue;
> >
> > for_each_set_bit()
>
> only 12 bits are valid, the top 4 are undocumented. They -should- be
> zeroed but who knows.

I didn't get your answer. The limit for the for-loop in both cases
will be the same, but for_each_set_bit() is what you open-coded. Why
do you need the open-coded variant, what is the benefit?

...

> > > +       /* Error statuses. */
> > > +       {
> > > +               .name = "error_status",
> > > +               .read = sunrise_error_status_read,
> > > +               .shared = IIO_SEPARATE,
> > > +       },
> > > +       IIO_ENUM_AVAILABLE("error_status", &sunrise_error_statuses_enum),
> >
> > > +       {},
> >
> > No comma for terminator entries.

I assume non-commented ones are agreed on.

...

> > > +
> > > +       return 0;
> >
> > Dead code?
>
> It is, but it seems nicer :) Should I drop it ?

We don't need dead code in the kernel.

-- 
With Best Regards,
Andy Shevchenko

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

[Jacopo Mondi]

Hi Andy,

On Fri, Aug 20, 2021 at 05:56:48PM +0300, Andy Shevchenko wrote:
> On Fri, Aug 20, 2021 at 5:42 PM Jacopo Mondi <jacopo@jmondi.org> wrote:
>
> ...
>
> > > > +#include <linux/i2c.h>
> > >
> > > > +#include <linux/iio/iio.h>
> > > > +#include <linux/iio/sysfs.h>
> > >
> > > Can you move this as a separate group...
> > >
> > > > +#include <linux/kernel.h>
> > > > +#include <linux/mod_devicetable.h>
> > > > +#include <linux/module.h>
> > > > +#include <linux/mutex.h>
> > > > +#include <linux/regmap.h>
> > > > +#include <linux/sysfs.h>
> > >
> > > ...here ?
> >
> > Ah, is this customary in the subsystem ?
>
> When it's a group of headers for a certain subsystem it makes it clear
> that the driver belongs to it.

Ack, I'll move

>
> ...
>
> > > > +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.
> > > > +        */
> > >
> > > I'm wondering if there is a better way to perform this.
> >
> > Do you mean using a different API instead of i2c_smbus_xfer()?
>
> I meant on hw level.

That doesn't seem to be possible, I'm afraid.

The chip manual clearly reports that the chip needs to be waken up as
it enters low power state automatically after 15ms without
communications or after a completed transaction

Quoting TDE5531 which is now referenced in the commit message:

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

> ...
>
> > > > +               dev_err(sunrise->dev, "Read word failed: reg 0x%2x (%d)\n",
> > > > +                       reg, ret);
> > >
> > > One line?
> >
> > Goes over 80, as all the other identical comments below.
>
> It's still less than 90, so go for it!

Where does the 90 cols limit comes from ? :)

Anyway, I guess that's up to the subsystem, so I'll re-arrange lines
close to 80 cols

>
> ...
>
> > > > +       ret = sunrise_calibrate(sunrise);
> > > > +
> > > > +       return ret ?: len;
> > >
> > > In this case
> > >
> > >   if (ret)
> > >     return ret;
> > >
> > > return len;
> > >
> > > will look more natural.
> >
> > I had this and I switched before sending. Matter of tastes I guess.
> > I actually changed this as I thought it would have pleased you :)
>
> Use your common sense. Not every place where you can put it really needs it.
> Either way you choose is okay because of style, but to be sure ask the
> maintainer :-)

It's a really minor detail, and I also prefer the style you suggested
so I'll go for it.

>
> ...
>
> > > > +       for (i = 0; i < ARRAY_SIZE(error_codes); ++i) {
> > > > +               if (!(value & BIT(error_codes[i])))
> > > > +                       continue;
> > >
> > > for_each_set_bit()
> >
> > only 12 bits are valid, the top 4 are undocumented. They -should- be
> > zeroed but who knows.
>
> I didn't get your answer. The limit for the for-loop in both cases
> will be the same, but for_each_set_bit() is what you open-coded. Why
> do you need the open-coded variant, what is the benefit?
>

I should have looked at the for_each_set_bit() definition to find out
that it supports a size argument. I assumed it ranged on the full size
of the variable to inspect. Sorry for being sloppy ;)

> ...
>
> > > > +       /* Error statuses. */
> > > > +       {
> > > > +               .name = "error_status",
> > > > +               .read = sunrise_error_status_read,
> > > > +               .shared = IIO_SEPARATE,
> > > > +       },
> > > > +       IIO_ENUM_AVAILABLE("error_status", &sunrise_error_statuses_enum),
> > >
> > > > +       {},
> > >
> > > No comma for terminator entries.
>
> I assume non-commented ones are agreed on.
>

Ack

> ...
>
> > > > +
> > > > +       return 0;
> > >
> > > Dead code?
> >
> > It is, but it seems nicer :) Should I drop it ?
>
> We don't need dead code in the kernel.

Nobody does actually, so I'll drop this.

Thanks
   j

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

[PATCH v2 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