On Tue, 14 Apr 2020, mani@xxxxxxxxxx wrote:
> From: Manivannan Sadhasivam <mani@xxxxxxxxxx>
comments below
> 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@xxxxxxxxxx>
> ---
> 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..951358710f64 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);
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?
> +
> + 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",
> + 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?
> + 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);
>
>
--
Peter Meerwald-Stadler
Mobile: +43 664 24 44 418
