[PATCH 0/3] Add Reset and Wakeup support for CCS811

[PATCH 0/3] Add Reset and Wakeup support for CCS811

[mani]

From: Manivannan Sadhasivam <mani@kernel.org>

Hello,

This patchset adds external reset and wakeup support for CCS811 VOC
gas sensor. The nRESET and nWAKE pins available on the device are
utilised to provide these functionalities.

The patchset has been validated using CCS811 connected to STM32MP1 based
board over I2C.

While at it, the devicetree yaml binding and OF match table for this sensor
are also added.

Thanks,
Mani

Manivannan Sadhasivam (3):
  dt-bindings: iio: chemical: Add binding for CCS811 VOC sensor
  iio: chemical: Add support for external Reset and Wakeup in CCS811
  iio: chemical: Add OF match table for CCS811 VOC sensor

 .../bindings/iio/chemical/ams,ccs811.yaml     | 50 ++++++++++
 drivers/iio/chemical/ccs811.c                 | 96 +++++++++++++++++--
 2 files changed, 138 insertions(+), 8 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/iio/chemical/ams,ccs811.yaml

-- 
2.17.1

[PATCH 1/3] dt-bindings: iio: chemical: Add binding for CCS811 VOC sensor

[mani]

From: Manivannan Sadhasivam <mani@kernel.org>

This commit adds devicetree binding for AMS CCS811 VOC sensor.

Signed-off-by: Manivannan Sadhasivam <mani@kernel.org>
---
 .../bindings/iio/chemical/ams,ccs811.yaml     | 50 +++++++++++++++++++
 1 file changed, 50 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/chemical/ams,ccs811.yaml

diff --git a/Documentation/devicetree/bindings/iio/chemical/ams,ccs811.yaml b/Documentation/devicetree/bindings/iio/chemical/ams,ccs811.yaml
new file mode 100644
index 000000000000..564208af7b2b
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/chemical/ams,ccs811.yaml
@@ -0,0 +1,50 @@
+# SPDX-License-Identifier: GPL-2.0
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/chemical/ams,ccs811.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: AMS CCS811 VOC Sensor
+
+maintainers:
+  - Narcisa Vasile <narcisaanamaria12@gmail.com>
+
+description: |
+  Ultra-Low Power Digital Gas Sensor for Monitoring Indoor Air Quality.
+
+properties:
+  compatible:
+    enum:
+      - ams,ccs811
+  reg:
+    maxItems: 1
+
+  reset-gpios:
+    description: GPIO connected to the nRESET line. This is an active low
+                 input to CCS811.
+    maxItems: 1
+
+  wakeup-gpios:
+    description: GPIO connected to the nWAKE line. This is an active low
+                 input to CCS811.
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+
+examples:
+  - |
+    i2c {
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      ccs811@5b {
+        compatible = "ams,ccs811";
+        reg = <0x5b>;
+        reset-gpios = <&gpioa 11 GPIO_ACTIVE_LOW>;
+        wakeup-gpios = <&gpioa 12 GPIO_ACTIVE_LOW>;
+      };
+    };
+
+...
-- 
2.17.1

[PATCH 2/3] iio: chemical: Add support for external Reset and Wakeup in CCS811

[mani]

From: Manivannan Sadhasivam <mani@kernel.org>

CCS811 VOC sensor exposes nRESET and nWAKE pins which can be connected
to GPIO pins of the host controller. These pins can be used to externally
release the device from reset and also to wake it up before any I2C
transaction. The initial driver support assumed that the nRESET pin is not
connected and the nWAKE pin is tied to ground.

This commit improves it by adding support for controlling those two pins
externally using a host controller. For the case of reset, if the hardware
reset is not available, the mechanism to do software reset is also added.

As a side effect of doing this, the IIO device allocation needs to be
slightly moved to top of probe to make use of priv data early.

Signed-off-by: Manivannan Sadhasivam <mani@kernel.org>
---
 drivers/iio/chemical/ccs811.c | 88 +++++++++++++++++++++++++++++++----
 1 file changed, 80 insertions(+), 8 deletions(-)

diff --git a/drivers/iio/chemical/ccs811.c b/drivers/iio/chemical/ccs811.c
index 2ebdfc35bcda..6cd92c49c348 100644
--- a/drivers/iio/chemical/ccs811.c
+++ b/drivers/iio/chemical/ccs811.c
@@ -16,6 +16,7 @@
  */
 
 #include <linux/delay.h>
+#include <linux/gpio/consumer.h>
 #include <linux/i2c.h>
 #include <linux/iio/iio.h>
 #include <linux/iio/buffer.h>
@@ -36,6 +37,7 @@
 #define CCS811_ERR		0xE0
 /* Used to transition from boot to application mode */
 #define CCS811_APP_START	0xF4
+#define CCS811_SW_RESET		0xFF
 
 /* Status register flags */
 #define CCS811_STATUS_ERROR		BIT(0)
@@ -74,6 +76,7 @@ struct ccs811_data {
 	struct mutex lock; /* Protect readings */
 	struct ccs811_reading buffer;
 	struct iio_trigger *drdy_trig;
+	struct gpio_desc *wakeup_gpio;
 	bool drdy_trig_on;
 };
 
@@ -166,10 +169,25 @@ static int ccs811_setup(struct i2c_client *client)
 					 CCS811_MODE_IAQ_1SEC);
 }
 
+static void ccs811_set_wakeup(struct ccs811_data *data, bool enable)
+{
+	if (!data->wakeup_gpio)
+		return;
+
+	gpiod_set_value(data->wakeup_gpio, enable);
+
+	if (enable)
+		usleep_range(50, 60);
+	else
+		usleep_range(20, 30);
+}
+
 static int ccs811_get_measurement(struct ccs811_data *data)
 {
 	int ret, tries = 11;
 
+	ccs811_set_wakeup(data, true);
+
 	/* Maximum waiting time: 1s, as measurements are made every second */
 	while (tries-- > 0) {
 		ret = i2c_smbus_read_byte_data(data->client, CCS811_STATUS);
@@ -183,9 +201,12 @@ static int ccs811_get_measurement(struct ccs811_data *data)
 	if (!(ret & CCS811_STATUS_DATA_READY))
 		return -EIO;
 
-	return i2c_smbus_read_i2c_block_data(data->client,
+	ret = i2c_smbus_read_i2c_block_data(data->client,
 					    CCS811_ALG_RESULT_DATA, 8,
 					    (char *)&data->buffer);
+	ccs811_set_wakeup(data, false);
+
+	return ret;
 }
 
 static int ccs811_read_raw(struct iio_dev *indio_dev,
@@ -336,6 +357,42 @@ static irqreturn_t ccs811_data_rdy_trigger_poll(int irq, void *private)
 	return IRQ_HANDLED;
 }
 
+static int ccs811_reset(struct i2c_client *client)
+{
+	struct gpio_desc *reset_gpio;
+	int ret;
+
+	reset_gpio = devm_gpiod_get_optional(&client->dev, "reset",
+					     GPIOD_OUT_LOW);
+	if (IS_ERR(reset_gpio)) {
+		dev_err(&client->dev, "Failed to acquire reset gpio\n");
+		return -EINVAL;
+	}
+
+	/* Try to reset using nRESET pin if available else do SW reset */
+	if (reset_gpio) {
+		gpiod_set_value(reset_gpio, 1);
+		usleep_range(20, 30);
+		gpiod_set_value(reset_gpio, 0);
+	} else {
+		static const u8 reset_seq[] = {
+			0xFF, 0x11, 0xE5, 0x72, 0x8A,
+		};
+
+		ret = i2c_smbus_write_i2c_block_data(client, CCS811_SW_RESET,
+					     sizeof(reset_seq), reset_seq);
+		if (ret < 0) {
+			dev_err(&client->dev, "Failed to reset sensor\n");
+			return ret;
+		}
+	}
+
+	/* tSTART delay required after reset */
+	usleep_range(1000, 2000);
+
+	return 0;
+}
+
 static int ccs811_probe(struct i2c_client *client,
 			const struct i2c_device_id *id)
 {
@@ -348,6 +405,27 @@ static int ccs811_probe(struct i2c_client *client,
 				     | I2C_FUNC_SMBUS_READ_I2C_BLOCK))
 		return -EOPNOTSUPP;
 
+	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	data = iio_priv(indio_dev);
+	i2c_set_clientdata(client, indio_dev);
+	data->client = client;
+
+	data->wakeup_gpio = devm_gpiod_get_optional(&client->dev, "wakeup",
+						    GPIOD_OUT_HIGH);
+	if (IS_ERR(data->wakeup_gpio)) {
+		dev_err(&client->dev, "Failed to acquire wakeup gpio\n");
+		return -EINVAL;
+	}
+
+	ccs811_set_wakeup(data, true);
+
+	ret = ccs811_reset(client);
+	if (ret)
+		return ret;
+
 	/* Check hardware id (should be 0x81 for this family of devices) */
 	ret = i2c_smbus_read_byte_data(client, CCS811_HW_ID);
 	if (ret < 0)
@@ -367,17 +445,11 @@ static int ccs811_probe(struct i2c_client *client,
 		return -ENODEV;
 	}
 
-	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
-	if (!indio_dev)
-		return -ENOMEM;
-
 	ret = ccs811_setup(client);
 	if (ret < 0)
 		return ret;
 
-	data = iio_priv(indio_dev);
-	i2c_set_clientdata(client, indio_dev);
-	data->client = client;
+	ccs811_set_wakeup(data, false);
 
 	mutex_init(&data->lock);
 
-- 
2.17.1

[PATCH 3/3] iio: chemical: Add OF match table for CCS811 VOC sensor

[mani]

From: Manivannan Sadhasivam <mani@kernel.org>

Add devicetree OF match table support for CCS811 VOC sensor.

Signed-off-by: Manivannan Sadhasivam <mani@kernel.org>
---
 drivers/iio/chemical/ccs811.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/iio/chemical/ccs811.c b/drivers/iio/chemical/ccs811.c
index 6cd92c49c348..313931208f61 100644
--- a/drivers/iio/chemical/ccs811.c
+++ b/drivers/iio/chemical/ccs811.c
@@ -24,6 +24,7 @@
 #include <linux/iio/triggered_buffer.h>
 #include <linux/iio/trigger_consumer.h>
 #include <linux/module.h>
+#include <linux/of.h>
 
 #define CCS811_STATUS		0x00
 #define CCS811_MEAS_MODE	0x01
@@ -538,9 +539,16 @@ static const struct i2c_device_id ccs811_id[] = {
 };
 MODULE_DEVICE_TABLE(i2c, ccs811_id);
 
+static const struct of_device_id ccs811_dt_ids[] = {
+	{ .compatible = "ams,ccs811" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, ccs811_dt_ids);
+
 static struct i2c_driver ccs811_driver = {
 	.driver = {
 		.name = "ccs811",
+		.of_match_table = of_match_ptr(ccs811_dt_ids),
 	},
 	.probe = ccs811_probe,
 	.remove = ccs811_remove,
-- 
2.17.1

Re: [PATCH 3/3] iio: chemical: Add OF match table for CCS811 VOC sensor

[Jonathan Cameron]

On Mon, 13 Apr 2020 00:06:58 +0530
mani@kernel.org wrote:

> From: Manivannan Sadhasivam <mani@kernel.org>
> 
> Add devicetree OF match table support for CCS811 VOC sensor.
> 
> Signed-off-by: Manivannan Sadhasivam <mani@kernel.org>

Hi,

A few small things to clean up inline

Thanks,

Jonathan

> ---
>  drivers/iio/chemical/ccs811.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/iio/chemical/ccs811.c b/drivers/iio/chemical/ccs811.c
> index 6cd92c49c348..313931208f61 100644
> --- a/drivers/iio/chemical/ccs811.c
> +++ b/drivers/iio/chemical/ccs811.c
> @@ -24,6 +24,7 @@
>  #include <linux/iio/triggered_buffer.h>
>  #include <linux/iio/trigger_consumer.h>
>  #include <linux/module.h>
> +#include <linux/of.h>

You are including this just to get things in mod_devicetable.h
so include that directly instead.

>  
>  #define CCS811_STATUS		0x00
>  #define CCS811_MEAS_MODE	0x01
> @@ -538,9 +539,16 @@ static const struct i2c_device_id ccs811_id[] = {
>  };
>  MODULE_DEVICE_TABLE(i2c, ccs811_id);
>  
> +static const struct of_device_id ccs811_dt_ids[] = {
> +	{ .compatible = "ams,ccs811" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, ccs811_dt_ids);
> +
>  static struct i2c_driver ccs811_driver = {
>  	.driver = {
>  		.name = "ccs811",
> +		.of_match_table = of_match_ptr(ccs811_dt_ids),
No need for the of_match_ptr macro.  It has several issues.

1) Blocks PRP001 ACPI magic device types being used to instantiate
this using the device tree binding but under ACPI.
2) Will give warnings about ccs811_dt_ids being unused on no
   device tree builds.

>  	},
>  	.probe = ccs811_probe,
>  	.remove = ccs811_remove,

Re: [PATCH 1/3] dt-bindings: iio: chemical: Add binding for CCS811 VOC sensor

[Jonathan Cameron]

On Mon, 13 Apr 2020 00:06:56 +0530
mani@kernel.org wrote:

> From: Manivannan Sadhasivam <mani@kernel.org>
> 
> This commit adds devicetree binding for AMS CCS811 VOC sensor.
> 
> Signed-off-by: Manivannan Sadhasivam <mani@kernel.org>
Great to see this driver getting some more attention.

A few things inline

Thanks,

Jonathan

> ---
>  .../bindings/iio/chemical/ams,ccs811.yaml     | 50 +++++++++++++++++++
>  1 file changed, 50 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/chemical/ams,ccs811.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/chemical/ams,ccs811.yaml b/Documentation/devicetree/bindings/iio/chemical/ams,ccs811.yaml
> new file mode 100644
> index 000000000000..564208af7b2b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/chemical/ams,ccs811.yaml
> @@ -0,0 +1,50 @@
> +# SPDX-License-Identifier: GPL-2.0

Preferred that all new binding docs are dual licensed with BSD as well
to allow there use by things other than the kernel.

> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/chemical/ams,ccs811.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: AMS CCS811 VOC Sensor
> +
> +maintainers:
> +  - Narcisa Vasile <narcisaanamaria12@gmail.com>
> +
> +description: |
> +  Ultra-Low Power Digital Gas Sensor for Monitoring Indoor Air Quality.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - ams,ccs811
> +  reg:
> +    maxItems: 1
> +
> +  reset-gpios:
> +    description: GPIO connected to the nRESET line. This is an active low
> +                 input to CCS811.
> +    maxItems: 1
> +
> +  wakeup-gpios:
> +    description: GPIO connected to the nWAKE line. This is an active low
> +                 input to CCS811.
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +
> +examples:
> +  - |
> +    i2c {
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +
> +      ccs811@5b {
Hmm. Not sure we have a generic name in the dt spec for voc sensor.
Even so we should try for something appropriate.

Could go with voc@5b?

> +        compatible = "ams,ccs811";
> +        reg = <0x5b>;
> +        reset-gpios = <&gpioa 11 GPIO_ACTIVE_LOW>;
> +        wakeup-gpios = <&gpioa 12 GPIO_ACTIVE_LOW>;
> +      };
> +    };
> +
> +...

Re: [PATCH 2/3] iio: chemical: Add support for external Reset and Wakeup in CCS811

[Jonathan Cameron]

On Mon, 13 Apr 2020 00:06:57 +0530
mani@kernel.org wrote:

> From: Manivannan Sadhasivam <mani@kernel.org>
> 
> CCS811 VOC sensor exposes nRESET and nWAKE pins which can be connected
> to GPIO pins of the host controller. These pins can be used to externally
> release the device from reset and also to wake it up before any I2C
> transaction. The initial driver support assumed that the nRESET pin is not
> connected and the nWAKE pin is tied to ground.
> 
> This commit improves it by adding support for controlling those two pins
> externally using a host controller. For the case of reset, if the hardware
> reset is not available, the mechanism to do software reset is also added.
> 
> As a side effect of doing this, the IIO device allocation needs to be
> slightly moved to top of probe to make use of priv data early.
> 
> Signed-off-by: Manivannan Sadhasivam <mani@kernel.org>

One trivial thing inline to allow things to work if we have deferred probing
and the gpio provider isn't ready yet.  Currently you eat the error code
rather than passing it on.

Otherwise looks good to me.

Thanks,

Jonathan
> ---
>  drivers/iio/chemical/ccs811.c | 88 +++++++++++++++++++++++++++++++----
>  1 file changed, 80 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/iio/chemical/ccs811.c b/drivers/iio/chemical/ccs811.c
> index 2ebdfc35bcda..6cd92c49c348 100644
> --- a/drivers/iio/chemical/ccs811.c
> +++ b/drivers/iio/chemical/ccs811.c
> @@ -16,6 +16,7 @@
>   */
>  
>  #include <linux/delay.h>
> +#include <linux/gpio/consumer.h>
>  #include <linux/i2c.h>
>  #include <linux/iio/iio.h>
>  #include <linux/iio/buffer.h>
> @@ -36,6 +37,7 @@
>  #define CCS811_ERR		0xE0
>  /* Used to transition from boot to application mode */
>  #define CCS811_APP_START	0xF4
> +#define CCS811_SW_RESET		0xFF
>  
>  /* Status register flags */
>  #define CCS811_STATUS_ERROR		BIT(0)
> @@ -74,6 +76,7 @@ struct ccs811_data {
>  	struct mutex lock; /* Protect readings */
>  	struct ccs811_reading buffer;
>  	struct iio_trigger *drdy_trig;
> +	struct gpio_desc *wakeup_gpio;
>  	bool drdy_trig_on;
>  };
>  
> @@ -166,10 +169,25 @@ static int ccs811_setup(struct i2c_client *client)
>  					 CCS811_MODE_IAQ_1SEC);
>  }
>  
> +static void ccs811_set_wakeup(struct ccs811_data *data, bool enable)
> +{
> +	if (!data->wakeup_gpio)
> +		return;
> +
> +	gpiod_set_value(data->wakeup_gpio, enable);
> +
> +	if (enable)
> +		usleep_range(50, 60);
> +	else
> +		usleep_range(20, 30);
> +}
> +
>  static int ccs811_get_measurement(struct ccs811_data *data)
>  {
>  	int ret, tries = 11;
>  
> +	ccs811_set_wakeup(data, true);
> +
>  	/* Maximum waiting time: 1s, as measurements are made every second */
>  	while (tries-- > 0) {
>  		ret = i2c_smbus_read_byte_data(data->client, CCS811_STATUS);
> @@ -183,9 +201,12 @@ static int ccs811_get_measurement(struct ccs811_data *data)
>  	if (!(ret & CCS811_STATUS_DATA_READY))
>  		return -EIO;
>  
> -	return i2c_smbus_read_i2c_block_data(data->client,
> +	ret = i2c_smbus_read_i2c_block_data(data->client,
>  					    CCS811_ALG_RESULT_DATA, 8,
>  					    (char *)&data->buffer);
> +	ccs811_set_wakeup(data, false);
> +
> +	return ret;
>  }
>  
>  static int ccs811_read_raw(struct iio_dev *indio_dev,
> @@ -336,6 +357,42 @@ static irqreturn_t ccs811_data_rdy_trigger_poll(int irq, void *private)
>  	return IRQ_HANDLED;
>  }
>  
> +static int ccs811_reset(struct i2c_client *client)
> +{
> +	struct gpio_desc *reset_gpio;
> +	int ret;
> +
> +	reset_gpio = devm_gpiod_get_optional(&client->dev, "reset",
> +					     GPIOD_OUT_LOW);
> +	if (IS_ERR(reset_gpio)) {
> +		dev_err(&client->dev, "Failed to acquire reset gpio\n");
> +		return -EINVAL;

return PTR_ERR as it may well be a deferred signal to say try again later
after the gpio provide device has been initialized.

> +	}
> +
> +	/* Try to reset using nRESET pin if available else do SW reset */
> +	if (reset_gpio) {
> +		gpiod_set_value(reset_gpio, 1);
> +		usleep_range(20, 30);
> +		gpiod_set_value(reset_gpio, 0);
> +	} else {
> +		static const u8 reset_seq[] = {
> +			0xFF, 0x11, 0xE5, 0x72, 0x8A,
> +		};
> +
> +		ret = i2c_smbus_write_i2c_block_data(client, CCS811_SW_RESET,
> +					     sizeof(reset_seq), reset_seq);
> +		if (ret < 0) {
> +			dev_err(&client->dev, "Failed to reset sensor\n");
> +			return ret;
> +		}
> +	}
> +
> +	/* tSTART delay required after reset */
> +	usleep_range(1000, 2000);
> +
> +	return 0;
> +}
> +
>  static int ccs811_probe(struct i2c_client *client,
>  			const struct i2c_device_id *id)
>  {
> @@ -348,6 +405,27 @@ static int ccs811_probe(struct i2c_client *client,
>  				     | I2C_FUNC_SMBUS_READ_I2C_BLOCK))
>  		return -EOPNOTSUPP;
>  
> +	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	data = iio_priv(indio_dev);
> +	i2c_set_clientdata(client, indio_dev);
> +	data->client = client;
> +
> +	data->wakeup_gpio = devm_gpiod_get_optional(&client->dev, "wakeup",
> +						    GPIOD_OUT_HIGH);
> +	if (IS_ERR(data->wakeup_gpio)) {
> +		dev_err(&client->dev, "Failed to acquire wakeup gpio\n");
> +		return -EINVAL;
> +	}
> +
> +	ccs811_set_wakeup(data, true);
> +
> +	ret = ccs811_reset(client);
> +	if (ret)
> +		return ret;
> +
>  	/* Check hardware id (should be 0x81 for this family of devices) */
>  	ret = i2c_smbus_read_byte_data(client, CCS811_HW_ID);
>  	if (ret < 0)
> @@ -367,17 +445,11 @@ static int ccs811_probe(struct i2c_client *client,
>  		return -ENODEV;
>  	}
>  
> -	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> -	if (!indio_dev)
> -		return -ENOMEM;
> -
>  	ret = ccs811_setup(client);
>  	if (ret < 0)
>  		return ret;
>  
> -	data = iio_priv(indio_dev);
> -	i2c_set_clientdata(client, indio_dev);
> -	data->client = client;
> +	ccs811_set_wakeup(data, false);
>  
>  	mutex_init(&data->lock);
>  

Re: [PATCH 2/3] iio: chemical: Add support for external Reset and Wakeup in CCS811

[Andy Shevchenko]

On Mon, Apr 13, 2020 at 8:34 AM <mani@kernel.org> wrote:
>
> From: Manivannan Sadhasivam <mani@kernel.org>
>
> CCS811 VOC sensor exposes nRESET and nWAKE pins which can be connected
> to GPIO pins of the host controller. These pins can be used to externally
> release the device from reset and also to wake it up before any I2C
> transaction. The initial driver support assumed that the nRESET pin is not
> connected and the nWAKE pin is tied to ground.
>
> This commit improves it by adding support for controlling those two pins
> externally using a host controller. For the case of reset, if the hardware
> reset is not available, the mechanism to do software reset is also added.
>
> As a side effect of doing this, the IIO device allocation needs to be
> slightly moved to top of probe to make use of priv data early.

...

> +#define CCS811_SW_RESET                0xFF


> +       reset_gpio = devm_gpiod_get_optional(&client->dev, "reset",
> +                                            GPIOD_OUT_LOW);
> +       if (IS_ERR(reset_gpio)) {
> +               dev_err(&client->dev, "Failed to acquire reset gpio\n");

> +               return -EINVAL;

Do not shadow actual error code.

> +       }
> +
> +       /* Try to reset using nRESET pin if available else do SW reset */
> +       if (reset_gpio) {
> +               gpiod_set_value(reset_gpio, 1);
> +               usleep_range(20, 30);
> +               gpiod_set_value(reset_gpio, 0);
> +       } else {

> +               static const u8 reset_seq[] = {
> +                       0xFF, 0x11, 0xE5, 0x72, 0x8A,
> +               };

Is 0xFF here is CCS811_SW_RESET? If so, can you put it explicitly?

> +               ret = i2c_smbus_write_i2c_block_data(client, CCS811_SW_RESET,
> +                                            sizeof(reset_seq), reset_seq);
> +               if (ret < 0) {
> +                       dev_err(&client->dev, "Failed to reset sensor\n");
> +                       return ret;
> +               }
> +       }

...

> +       data->wakeup_gpio = devm_gpiod_get_optional(&client->dev, "wakeup",
> +                                                   GPIOD_OUT_HIGH);
> +       if (IS_ERR(data->wakeup_gpio)) {
> +               dev_err(&client->dev, "Failed to acquire wakeup gpio\n");

> +               return -EINVAL;

Ditto.

> +       }

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/3] iio: chemical: Add OF match table for CCS811 VOC sensor

[Andy Shevchenko]

On Mon, Apr 13, 2020 at 10:23 AM <mani@kernel.org> wrote:
>
> From: Manivannan Sadhasivam <mani@kernel.org>
>
> Add devicetree OF match table support for CCS811 VOC sensor.

...

> +#include <linux/of.h>

Why?

...

> +static const struct of_device_id ccs811_dt_ids[] = {
> +       { .compatible = "ams,ccs811" },
> +       { }
> +};
> +MODULE_DEVICE_TABLE(of, ccs811_dt_ids);

Since it has no ugly ifdeffery...

>  static struct i2c_driver ccs811_driver = {
>         .driver = {
>                 .name = "ccs811",
> +               .of_match_table = of_match_ptr(ccs811_dt_ids),

...use of of_match_ptr() brings a compiler warning.

Drop of_match_ptr() for good. And thus drop redundant of.h.

>         },

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/3] iio: chemical: Add OF match table for CCS811 VOC sensor

[Manivannan Sadhasivam]

On Mon, Apr 13, 2020 at 05:50:54PM +0100, Jonathan Cameron wrote:
> On Mon, 13 Apr 2020 00:06:58 +0530
> mani@kernel.org wrote:
> 
> > From: Manivannan Sadhasivam <mani@kernel.org>
> > 
> > Add devicetree OF match table support for CCS811 VOC sensor.
> > 
> > Signed-off-by: Manivannan Sadhasivam <mani@kernel.org>
> 
> Hi,
> 
> A few small things to clean up inline
> 
> Thanks,
> 
> Jonathan
> 
> > ---
> >  drivers/iio/chemical/ccs811.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/drivers/iio/chemical/ccs811.c b/drivers/iio/chemical/ccs811.c
> > index 6cd92c49c348..313931208f61 100644
> > --- a/drivers/iio/chemical/ccs811.c
> > +++ b/drivers/iio/chemical/ccs811.c
> > @@ -24,6 +24,7 @@
> >  #include <linux/iio/triggered_buffer.h>
> >  #include <linux/iio/trigger_consumer.h>
> >  #include <linux/module.h>
> > +#include <linux/of.h>
> 
> You are including this just to get things in mod_devicetable.h
> so include that directly instead.
> 

I added this include for of_match_ptr. Since it is not needed anymore, I'll drop
this.

> >  
> >  #define CCS811_STATUS		0x00
> >  #define CCS811_MEAS_MODE	0x01
> > @@ -538,9 +539,16 @@ static const struct i2c_device_id ccs811_id[] = {
> >  };
> >  MODULE_DEVICE_TABLE(i2c, ccs811_id);
> >  
> > +static const struct of_device_id ccs811_dt_ids[] = {
> > +	{ .compatible = "ams,ccs811" },
> > +	{ }
> > +};
> > +MODULE_DEVICE_TABLE(of, ccs811_dt_ids);
> > +
> >  static struct i2c_driver ccs811_driver = {
> >  	.driver = {
> >  		.name = "ccs811",
> > +		.of_match_table = of_match_ptr(ccs811_dt_ids),
> No need for the of_match_ptr macro.  It has several issues.
> 
> 1) Blocks PRP001 ACPI magic device types being used to instantiate
> this using the device tree binding but under ACPI.
> 2) Will give warnings about ccs811_dt_ids being unused on no
>    device tree builds.
> 

Okay, will use ccs811_dt_ids directly.

Thanks,
Mani

> >  	},
> >  	.probe = ccs811_probe,
> >  	.remove = ccs811_remove,
> 

Re: [PATCH 3/3] iio: chemical: Add OF match table for CCS811 VOC sensor

[Manivannan Sadhasivam]

On Mon, Apr 13, 2020 at 11:23:03PM +0300, Andy Shevchenko wrote:
> On Mon, Apr 13, 2020 at 10:23 AM <mani@kernel.org> wrote:
> >
> > From: Manivannan Sadhasivam <mani@kernel.org>
> >
> > Add devicetree OF match table support for CCS811 VOC sensor.
> 
> ...
> 
> > +#include <linux/of.h>
> 
> Why?
> 

As replied to Jonathan, will drop this and of_match_ptr.

Thanks,
Mani

> ...
> 
> > +static const struct of_device_id ccs811_dt_ids[] = {
> > +       { .compatible = "ams,ccs811" },
> > +       { }
> > +};
> > +MODULE_DEVICE_TABLE(of, ccs811_dt_ids);
> 
> Since it has no ugly ifdeffery...
> 
> >  static struct i2c_driver ccs811_driver = {
> >         .driver = {
> >                 .name = "ccs811",
> > +               .of_match_table = of_match_ptr(ccs811_dt_ids),
> 
> ...use of of_match_ptr() brings a compiler warning.
> 
> Drop of_match_ptr() for good. And thus drop redundant of.h.
> 
> >         },
> 
> -- 
> With Best Regards,
> Andy Shevchenko

Re: [PATCH 2/3] iio: chemical: Add support for external Reset and Wakeup in CCS811

[Manivannan Sadhasivam]

On Mon, Apr 13, 2020 at 11:20:42PM +0300, Andy Shevchenko wrote:
> On Mon, Apr 13, 2020 at 8:34 AM <mani@kernel.org> wrote:
> >
> > From: Manivannan Sadhasivam <mani@kernel.org>
> >
> > CCS811 VOC sensor exposes nRESET and nWAKE pins which can be connected
> > to GPIO pins of the host controller. These pins can be used to externally
> > release the device from reset and also to wake it up before any I2C
> > transaction. The initial driver support assumed that the nRESET pin is not
> > connected and the nWAKE pin is tied to ground.
> >
> > This commit improves it by adding support for controlling those two pins
> > externally using a host controller. For the case of reset, if the hardware
> > reset is not available, the mechanism to do software reset is also added.
> >
> > As a side effect of doing this, the IIO device allocation needs to be
> > slightly moved to top of probe to make use of priv data early.
> 
> ...
> 
> > +#define CCS811_SW_RESET                0xFF
> 
> 
> > +       reset_gpio = devm_gpiod_get_optional(&client->dev, "reset",
> > +                                            GPIOD_OUT_LOW);
> > +       if (IS_ERR(reset_gpio)) {
> > +               dev_err(&client->dev, "Failed to acquire reset gpio\n");
> 
> > +               return -EINVAL;
> 
> Do not shadow actual error code.
> 

Okay. Will change below instance as well.

> > +       }
> > +
> > +       /* Try to reset using nRESET pin if available else do SW reset */
> > +       if (reset_gpio) {
> > +               gpiod_set_value(reset_gpio, 1);
> > +               usleep_range(20, 30);
> > +               gpiod_set_value(reset_gpio, 0);
> > +       } else {
> 
> > +               static const u8 reset_seq[] = {
> > +                       0xFF, 0x11, 0xE5, 0x72, 0x8A,
> > +               };
> 
> Is 0xFF here is CCS811_SW_RESET? If so, can you put it explicitly?
> 

Nope. CCS811_SW_RESET is the register address whereas 0xFF here is the actual
value return to the register. I don't know what these values in array represent.
So will keep them as it is.

Thanks,
Mani

> > +               ret = i2c_smbus_write_i2c_block_data(client, CCS811_SW_RESET,
> > +                                            sizeof(reset_seq), reset_seq);
> > +               if (ret < 0) {
> > +                       dev_err(&client->dev, "Failed to reset sensor\n");
> > +                       return ret;
> > +               }
> > +       }
> 
> ...
> 
> > +       data->wakeup_gpio = devm_gpiod_get_optional(&client->dev, "wakeup",
> > +                                                   GPIOD_OUT_HIGH);
> > +       if (IS_ERR(data->wakeup_gpio)) {
> > +               dev_err(&client->dev, "Failed to acquire wakeup gpio\n");
> 
> > +               return -EINVAL;
> 
> Ditto.
> 
> > +       }
> 
> -- 
> With Best Regards,
> Andy Shevchenko

Re: [PATCH 1/3] dt-bindings: iio: chemical: Add binding for CCS811 VOC sensor

[Manivannan Sadhasivam]

On Mon, Apr 13, 2020 at 05:53:23PM +0100, Jonathan Cameron wrote:
> On Mon, 13 Apr 2020 00:06:56 +0530
> mani@kernel.org wrote:
> 
> > From: Manivannan Sadhasivam <mani@kernel.org>
> > 
> > This commit adds devicetree binding for AMS CCS811 VOC sensor.
> > 
> > Signed-off-by: Manivannan Sadhasivam <mani@kernel.org>
> Great to see this driver getting some more attention.
> 
> A few things inline
> 
> Thanks,
> 
> Jonathan
> 
> > ---
> >  .../bindings/iio/chemical/ams,ccs811.yaml     | 50 +++++++++++++++++++
> >  1 file changed, 50 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/iio/chemical/ams,ccs811.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/iio/chemical/ams,ccs811.yaml b/Documentation/devicetree/bindings/iio/chemical/ams,ccs811.yaml
> > new file mode 100644
> > index 000000000000..564208af7b2b
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/chemical/ams,ccs811.yaml
> > @@ -0,0 +1,50 @@
> > +# SPDX-License-Identifier: GPL-2.0
> 
> Preferred that all new binding docs are dual licensed with BSD as well
> to allow there use by things other than the kernel.
> 

Okay will do.

> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/iio/chemical/ams,ccs811.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: AMS CCS811 VOC Sensor
> > +
> > +maintainers:
> > +  - Narcisa Vasile <narcisaanamaria12@gmail.com>
> > +
> > +description: |
> > +  Ultra-Low Power Digital Gas Sensor for Monitoring Indoor Air Quality.
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - ams,ccs811
> > +  reg:
> > +    maxItems: 1
> > +
> > +  reset-gpios:
> > +    description: GPIO connected to the nRESET line. This is an active low
> > +                 input to CCS811.
> > +    maxItems: 1
> > +
> > +  wakeup-gpios:
> > +    description: GPIO connected to the nWAKE line. This is an active low
> > +                 input to CCS811.
> > +    maxItems: 1
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +
> > +examples:
> > +  - |
> > +    i2c {
> > +      #address-cells = <1>;
> > +      #size-cells = <0>;
> > +
> > +      ccs811@5b {
> Hmm. Not sure we have a generic name in the dt spec for voc sensor.
> Even so we should try for something appropriate.
> 
> Could go with voc@5b?
> 

Sounds good to me.

Thanks,
Mani

> > +        compatible = "ams,ccs811";
> > +        reg = <0x5b>;
> > +        reset-gpios = <&gpioa 11 GPIO_ACTIVE_LOW>;
> > +        wakeup-gpios = <&gpioa 12 GPIO_ACTIVE_LOW>;
> > +      };
> > +    };
> > +
> > +...
> 

Re: [PATCH 1/3] dt-bindings: iio: chemical: Add binding for CCS811 VOC sensor

[Rob Herring]

On Mon, 13 Apr 2020 00:06:56 +0530, mani@kernel.org wrote:
> From: Manivannan Sadhasivam <mani@kernel.org>
> 
> This commit adds devicetree binding for AMS CCS811 VOC sensor.
> 
> Signed-off-by: Manivannan Sadhasivam <mani@kernel.org>
> ---
>  .../bindings/iio/chemical/ams,ccs811.yaml     | 50 +++++++++++++++++++
>  1 file changed, 50 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/chemical/ams,ccs811.yaml
> 

My bot found errors running 'make dt_binding_check' on your patch:

Error: Documentation/devicetree/bindings/iio/chemical/ams,ccs811.example.dts:24.38-39 syntax error
FATAL ERROR: Unable to parse input tree
scripts/Makefile.lib:311: recipe for target 'Documentation/devicetree/bindings/iio/chemical/ams,ccs811.example.dt.yaml' failed
make[1]: *** [Documentation/devicetree/bindings/iio/chemical/ams,ccs811.example.dt.yaml] Error 1
make[1]: *** Waiting for unfinished jobs....
Makefile:1262: recipe for target 'dt_binding_check' failed
make: *** [dt_binding_check] Error 2

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

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

pip3 install git+https://github.com/devicetree-org/dt-schema.git@master --upgrade

Please check and re-submit.

Re: [PATCH 1/3] dt-bindings: iio: chemical: Add binding for CCS811 VOC sensor

[Manivannan Sadhasivam]

On Tue, Apr 14, 2020 at 08:27:17AM -0500, Rob Herring wrote:
> On Mon, 13 Apr 2020 00:06:56 +0530, mani@kernel.org wrote:
> > From: Manivannan Sadhasivam <mani@kernel.org>
> > 
> > This commit adds devicetree binding for AMS CCS811 VOC sensor.
> > 
> > Signed-off-by: Manivannan Sadhasivam <mani@kernel.org>
> > ---
> >  .../bindings/iio/chemical/ams,ccs811.yaml     | 50 +++++++++++++++++++
> >  1 file changed, 50 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/iio/chemical/ams,ccs811.yaml
> > 
> 
> My bot found errors running 'make dt_binding_check' on your patch:
> 
> Error: Documentation/devicetree/bindings/iio/chemical/ams,ccs811.example.dts:24.38-39 syntax error
> FATAL ERROR: Unable to parse input tree
> scripts/Makefile.lib:311: recipe for target 'Documentation/devicetree/bindings/iio/chemical/ams,ccs811.example.dt.yaml' failed
> make[1]: *** [Documentation/devicetree/bindings/iio/chemical/ams,ccs811.example.dt.yaml] Error 1
> make[1]: *** Waiting for unfinished jobs....
> Makefile:1262: recipe for target 'dt_binding_check' failed
> make: *** [dt_binding_check] Error 2
> 
> See https://patchwork.ozlabs.org/patch/1269473
> 
> If you already ran 'make dt_binding_check' and didn't see the above
> error(s), then make sure dt-schema is up to date:
> 

Ah, sorry. I did run 'dtbs_check' but forgot that it won't report errors in the
binding schemas.

Will run `dt_binding_check` hereafter.

Thanks,
Mani

> pip3 install git+https://github.com/devicetree-org/dt-schema.git@master --upgrade
> 
> Please check and re-submit.