Add Reset and Wakeup support for CCS811

From: Manivannan Sadhasivam <[email protected]>

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 <[email protected]>

---
 drivers/iio/chemical/ccs811.c | 88 +++++++++++++++++++++++++++++++----
 1 file changed, 80 insertions(+), 8 deletions(-)

drivers/iio/chemical/ccs811.c +18 -0

index 2ebdfc35bcda..951358710f64 100644
@@ -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);

drivers/iio/chemical/ccs811.c +47 -0

+
+	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 PTR_ERR(reset_gpio);
+	}
+
+	/* 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",

drivers/iio/chemical/ccs811.c +3 -7

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

Quoted 1 line · On Tue, 14 Apr 2020, [email protected] wrote:

From: Manivannan Sadhasivam <[email protected]>

comments below

Quoted 76 lines

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 <[email protected]>

---
 drivers/iio/chemical/ccs811.c | 88 +++++++++++++++++++++++++++++++----
 1 file changed, 80 insertions(+), 8 deletions(-)

drivers/iio/chemical/ccs811.c +18 -0

index 2ebdfc35bcda..951358710f64 100644
@@ -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;
 

drivers/iio/chemical/ccs811.c static int ccs811_get_measurement(struct ccs811_data *data) · line 207

Quoted 5 lines

drivers/iio/chemical/ccs811.c +2 -1

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

shouldn't the ccs811_set_wakeup(data, false) call be added to all potential paths leaving the function?
in particular when there is a timeout or reading the status fails?

Quoted 61 lines

drivers/iio/chemical/ccs811.c +47 -0

+
+	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 PTR_ERR(reset_gpio);
+	}
+
+	/* 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",

drivers/iio/chemical/ccs811.c static int ccs811_probe(struct i2c_client *client, · line 426

Quoted 10 lines

drivers/iio/chemical/ccs811.c +10 -0

+						    GPIOD_OUT_HIGH);
+	if (IS_ERR(data->wakeup_gpio)) {
+		dev_err(&client->dev, "Failed to acquire wakeup gpio\n");
+		return PTR_ERR(data->wakeup_gpio);
+	}
+
+	ccs811_set_wakeup(data, true);
+
+	ret = ccs811_reset(client);
+	if (ret)

ccs811_set_wakeup(data, false) missing here? and in other error paths?

Quoted 25 lines

drivers/iio/chemical/ccs811.c +3 -7

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

Signature · 2 lines

Peter Meerwald-Stadler
Mobile: +43 664 24 44 418

From: Manivannan Sadhasivam <[email protected]>

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.

(quoted diff) +2 -0

+               return PTR_ERR(reset_gpio);
+       }

drivers/iio/chemical/ccs811.c +2 -0

+               return PTR_ERR(data->wakeup_gpio);
+       }

Quoted 9 lines · On Tue, Apr 14, 2020 at 6:34 PM <[email protected]> wrote:

From: Manivannan Sadhasivam <[email protected]>

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.

Quoted 6 lines

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.

Thank you for an update, my comments below.

...

drivers/iio/chemical/ccs811.c static irqreturn_t ccs811_data_rdy_trigger_poll(int irq, void *private) · line 368

Quoted 5 lines

drivers/iio/chemical/ccs811.c +4 -0

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

If it's a deferred probe, it would spam the log.

Quoted 2 lines

(quoted diff) +2 -0

+               return PTR_ERR(reset_gpio);
+       }

...

drivers/iio/chemical/ccs811.c static irqreturn_t ccs811_data_rdy_trigger_poll(int irq, void *private) · line 380

Quoted 3 lines

drivers/iio/chemical/ccs811.c +3 -0

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

I would suggest to comment above from where you got this and the meaning of the numbers.

...

drivers/iio/chemical/ccs811.c static int ccs811_probe(struct i2c_client *client, · line 419

Quoted 5 lines

drivers/iio/chemical/ccs811.c +4 -0

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

Ditto.

Quoted 2 lines

drivers/iio/chemical/ccs811.c +2 -0

+               return PTR_ERR(data->wakeup_gpio);
+       }

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Quoted 1 line · Andy Shevchenko

On Tue, Apr 14, 2020 at 6:34 PM <[email protected]> wrote:

Quoted 8 lines

drivers/iio/chemical/ccs811.c static irqreturn_t ccs811_data_rdy_trigger_poll(int irq, void *private) · line 368

Quoted 5 lines

drivers/iio/chemical/ccs811.c +4 -0

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

If it's a deferred probe, it would spam the log.

Hmm. But error is an error isn't it? Would you recommend doing a debug print or completely removing the logging?

Quoted 9 lines

Quoted 2 lines

(quoted diff) +2 -0

+               return PTR_ERR(reset_gpio);
+       }

...

Quoted 3 lines

drivers/iio/chemical/ccs811.c +3 -0

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

Quoted 3 lines

I would suggest to comment above from where you got this and the meaning of the numbers.

The datasheet doesn't specify the meaning of these values. But will add a comment.

Btw, just noticed that 0xFF is not needed and only 4 values are sufficient for SW reset.

Quoted 2 lines · Manivannan Sadhasivam

Quoted 1 line · Andy Shevchenko

On Tue, Apr 14, 2020 at 6:34 PM <[email protected]> wrote:

...

Quoted 9 lines

Quoted 8 lines

drivers/iio/chemical/ccs811.c static irqreturn_t ccs811_data_rdy_trigger_poll(int irq, void *private) · line 368

Quoted 5 lines

drivers/iio/chemical/ccs811.c +4 -0

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

If it's a deferred probe, it would spam the log.

Quoted 2 lines

Hmm. But error is an error isn't it? Would you recommend doing a debug print or completely removing the logging?

I would remove completely, but better to wait for Jonathan to comment. Maybe he prefers something like
if (err != EPROBE_DEFER)

    dev_err()

Quoted 10 lines

Quoted 10 lines

Quoted 3 lines

(quoted diff) +2 -0

+               return PTR_ERR(reset_gpio);
+       }  

...

Quoted 3 lines

drivers/iio/chemical/ccs811.c +3 -0

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

Quoted 6 lines

Quoted 3 lines

I would suggest to comment above from where you got this and the meaning of the numbers.

The datasheet doesn't specify the meaning of these values. But will add a comment.

Thanks!

Quoted 2 lines

Btw, just noticed that 0xFF is not needed and only 4 values are sufficient for SW reset.

Better to do exactly what datasheet suggests (in case it's not clear or deductible what is going on).

Quoted 18 lines · On Tue, 14 Apr 2020 20:54:49 +0300 Andy Shevchenko <[email protected]>…

Quoted 2 lines · Manivannan Sadhasivam

Quoted 1 line · Andy Shevchenko

On Tue, Apr 14, 2020 at 6:34 PM <[email protected]> wrote:

...

Quoted 11 lines

Quoted 8 lines

drivers/iio/chemical/ccs811.c static irqreturn_t ccs811_data_rdy_trigger_poll(int irq, void *private) · line 368

Quoted 5 lines

drivers/iio/chemical/ccs811.c +4 -0

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

If it's a deferred probe, it would spam the log.

Hmm. But error is an error isn't it? Would you recommend doing a debug print or completely removing the logging?

Quoted 4 lines

I would remove completely, but better to wait for Jonathan to comment. Maybe he prefers something like
if (err != EPROBE_DEFER)

    dev_err()

Not fussed. Either drop it or do the check for defer - one I leave up to the author.

Quoted 25 lines

Quoted 10 lines

Quoted 10 lines

Quoted 3 lines

(quoted diff) +2 -0

+               return PTR_ERR(reset_gpio);
+       }  

...

Quoted 3 lines

drivers/iio/chemical/ccs811.c +3 -0

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

Quoted 6 lines

Quoted 3 lines

I would suggest to comment above from where you got this and the meaning of the numbers.

The datasheet doesn't specify the meaning of these values. But will add a comment.

Thanks!

Quoted 2 lines

Btw, just noticed that 0xFF is not needed and only 4 values are sufficient for SW reset.

Better to do exactly what datasheet suggests (in case it's not clear or deductible what is going on).

From: Manivannan Sadhasivam <[email protected]>

Add devicetree OF match table support for CCS811 VOC sensor.

Signed-off-by Manivannan Sadhasivam <[email protected]>

---
 drivers/iio/chemical/ccs811.c | 7 +++++++
 1 file changed, 7 insertions(+)

drivers/iio/chemical/ccs811.c +7 -1

index 951358710f64..310e9dedc9ba 100644
@@ -538,9 +538,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 = ccs811_dt_ids,
        },
        .probe = ccs811_probe,
        .remove = ccs811_remove,
--

2.17.1