diff mbox series

[v5,3/4] iio: chemical: Add Senseair Sunrise 006-0-007 driver

Message ID 20210909094537.218064-4-jacopo@jmondi.org (mailing list archive)
State Changes Requested
Headers show
Series iio: chemical: Add Senseair Sunrise CO2 sensor | expand

Commit Message

Jacopo Mondi Sept. 9, 2021, 9:45 a.m. UTC
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                        |   7 +
 drivers/iio/chemical/Kconfig       |  10 +
 drivers/iio/chemical/Makefile      |   1 +
 drivers/iio/chemical/sunrise_co2.c | 526 +++++++++++++++++++++++++++++
 4 files changed, 544 insertions(+)
 create mode 100644 drivers/iio/chemical/sunrise_co2.c

--
2.32.0

Comments

Peter Rosin Sept. 9, 2021, 12:04 p.m. UTC | #1
Hi!

Subject looks wrong, shouldn't it be 006-0-0007?

On 2021-09-09 11:45, Jacopo Mondi wrote:
> 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                        |   7 +
>  drivers/iio/chemical/Kconfig       |  10 +
>  drivers/iio/chemical/Makefile      |   1 +
>  drivers/iio/chemical/sunrise_co2.c | 526 +++++++++++++++++++++++++++++
>  4 files changed, 544 insertions(+)
>  create mode 100644 drivers/iio/chemical/sunrise_co2.c
> 

*snip*

> + * the i2c bus.
> + */
> +static int sunrise_read_byte(struct sunrise_dev *sunrise, u8 reg)
> +{
> +	const struct i2c_client *client = sunrise->client;
> +	const struct device *dev = &client->dev;
> +	unsigned int val;
> +	int ret;
> +
> +	i2c_lock_bus(client->adapter, I2C_LOCK_SEGMENT);
> +	ret = regmap_read(sunrise->regmap, reg, &val);
> +	i2c_unlock_bus(client->adapter, I2C_LOCK_SEGMENT);
> +	if (ret) {
> +		dev_err(dev, "Read byte failed: reg 0x%2x (%d)\n", reg, ret);

Nitpick, %02x looks better methinks ("reg 0x04" vs. "reg 0x 4").

[more instances below]

> +		return ret;
> +	}
> +
> +	return val;
> +}
> +
> +static int sunrise_read_word(struct sunrise_dev *sunrise, u8 reg, u16 *val)
> +{
> +	const struct i2c_client *client = sunrise->client;
> +	const struct device *dev = &client->dev;
> +	__be16 be_val;
> +	int ret;
> +
> +	i2c_lock_bus(client->adapter, I2C_LOCK_SEGMENT);
> +	ret = regmap_bulk_read(sunrise->regmap, reg, &be_val, sizeof(be_val));
> +	i2c_unlock_bus(client->adapter, I2C_LOCK_SEGMENT);
> +	if (ret) {
> +		dev_err(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)
> +{
> +	const struct i2c_client *client = sunrise->client;
> +	const struct device *dev = &client->dev;
> +	int ret;
> +
> +	i2c_lock_bus(client->adapter, I2C_LOCK_SEGMENT);
> +	ret = regmap_write(sunrise->regmap, reg, val);
> +	i2c_unlock_bus(client->adapter, I2C_LOCK_SEGMENT);
> +	if (ret)
> +		dev_err(dev, "Write byte failed: reg 0x%2x (%d)\n", reg, ret);
> +
> +	return ret;
> +}
> +
> +static int sunrise_write_word(struct sunrise_dev *sunrise, u8 reg, u16 data)
> +{
> +	const struct i2c_client *client = sunrise->client;
> +	const struct device *dev = &client->dev;
> +	__be16 be_data = cpu_to_be16(data);
> +	int ret;
> +
> +	i2c_lock_bus(client->adapter, I2C_LOCK_SEGMENT);
> +	ret = regmap_bulk_write(sunrise->regmap, reg, &be_data, sizeof(be_data));
> +	i2c_unlock_bus(client->adapter, I2C_LOCK_SEGMENT);
> +	if (ret)
> +		dev_err(dev, "Write word failed: reg 0x%2x (%d)\n", reg, ret);
> +
> +	return ret;
> +}

*snip*

> +
> +static const struct regmap_bus sunrise_regmap_bus = {
> +	.read = sunrise_regmap_read,
> +	.write = sunrise_regmap_write,
> +};
> +
> +static const 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;
> +
> +	sunrise = iio_priv(iio_dev);
> +	sunrise->client = client;

You should check if the I2C adapter supports the needed operations.

	if (!i2c_check_functionality(client->adapter,
				     I2C_FUNC_SMBUS_BLOCK_DATA |
				     I2C_FUNC_SMBUS_BYTE_DATA |
				     I2C_FUNC_SMBUS_QUICK |
				     I2C_FUNC_PROTOCOL_MANGLING))
		...

And, I suspect that protocol mangling might not be the first thing simple
SMBus-only adapters support?? Maybe you don't lose all that many adapters
by not using the restart-after-wakeup method, when odd things are required
anyway? That is, if the device gets enough time to wake up properly using
that method? (see the message from Andy in the v3 discussion)

SMBus quick isn't universal either...

But hmmm, maybe you don't *really* need protocol mangling when you don't
check the return value of the wakeup call? At the same time, without
I2C_M_IGNORE_NAK, you'll perhaps end up with boring entries in the log
or the adapter doing retries or some other inconvenient crap?

You could perhaps set I2C_M_IGNORE_NAK only if the adapter supports
protocol mangling? Any maybe you don't care about losers who don't have
a nice enough adapter. :-)

Cheers,
Peter

> +	mutex_init(&sunrise->lock);
> +
> +	sunrise->regmap = devm_regmap_init(&client->dev, &sunrise_regmap_bus,
> +					   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
>
Andy Shevchenko Sept. 9, 2021, 1:01 p.m. UTC | #2
On Thu, Sep 09, 2021 at 11:45:36AM +0200, Jacopo Mondi wrote:
> Add support for the Senseair Sunrise 006-0-0007 driver through the
> IIO subsystem.

...

> +	/*
> +	 * Wake up sensor by sending sensor address: START, sensor address,
> +	 * STOP. Sensor will not ACK this byte.
> +	 *
> +	 * The chip enters a low power state after 15msec without

msec --> ms (everybody understands 'ms' unit)

> +	 * communications or after a complete read/write sequence.
> +	 */

...

> +	struct i2c_client *client = context;
> +	union i2c_smbus_data data;
> +
> +	/* Discard reg address from values count. */
> +	if (count < 1)
> +		return -EINVAL;
> +	count--;

Wouldn't be more natural to decrement and then check against 0?

...

> +	memcpy(&data.block[1], (u8 *)val_buf + 1, count);

Not sure I understand why you need an explicit casting here.


...

> +	mutex_lock(&sunrise->lock);
> +	ret = sunrise_read_word(sunrise, SUNRISE_ERROR_STATUS_REG, &value);
> +	if (ret) {
> +		mutex_unlock(&sunrise->lock);

> +		return -EINVAL;

Why shadowing an actual error code?

> +	}

...

> +			/*
> +			 * / 10^4 to comply with IIO scale for CO2 (percentage).

"1 / 10^4"

> +			 * The chip CO2 reading range is [400 - 5000] ppm
> +			 * which corresponds to [0,004 - 0,5] %.
> +			 */
Andy Shevchenko Sept. 9, 2021, 1:03 p.m. UTC | #3
On Thu, Sep 09, 2021 at 04:01:48PM +0300, Andy Shevchenko wrote:
> On Thu, Sep 09, 2021 at 11:45:36AM +0200, Jacopo Mondi wrote:

> > +	struct i2c_client *client = context;
> > +	union i2c_smbus_data data;
> > +
> > +	/* Discard reg address from values count. */
> > +	if (count < 1)
> > +		return -EINVAL;
> > +	count--;
> 
> Wouldn't be more natural to decrement and then check against 0?

Ah, count is of size_t. Then comparison < 1 is equal to !count.
Jonathan Cameron Sept. 11, 2021, 3:12 p.m. UTC | #4
On Thu, 9 Sep 2021 16:01:48 +0300
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Thu, Sep 09, 2021 at 11:45:36AM +0200, Jacopo Mondi wrote:
> > Add support for the Senseair Sunrise 006-0-0007 driver through the
> > IIO subsystem.  
> 
> ...
> 
> > +	/*
> > +	 * Wake up sensor by sending sensor address: START, sensor address,
> > +	 * STOP. Sensor will not ACK this byte.
> > +	 *
> > +	 * The chip enters a low power state after 15msec without  
> 
> msec --> ms (everybody understands 'ms' unit)

yup. millisiemens :)
(couldn't resist)

Context is fine either way here.

> 
> > +	 * communications or after a complete read/write sequence.
> > +	 */  
> 
> ...
> 
> > +	struct i2c_client *client = context;
> > +	union i2c_smbus_data data;
> > +
> > +	/* Discard reg address from values count. */
> > +	if (count < 1)
> > +		return -EINVAL;
> > +	count--;  
> 
> Wouldn't be more natural to decrement and then check against 0?
> 
> ...
> 
> > +	memcpy(&data.block[1], (u8 *)val_buf + 1, count);  
> 
> Not sure I understand why you need an explicit casting here.

C doesn't allow pointer arithmetic on void * (gcc has it as an extension
though so it's probably fine without the cast)



> 
> 
> ...
> 
> > +	mutex_lock(&sunrise->lock);
> > +	ret = sunrise_read_word(sunrise, SUNRISE_ERROR_STATUS_REG, &value);
> > +	if (ret) {
> > +		mutex_unlock(&sunrise->lock);  
> 
> > +		return -EINVAL;  
> 
> Why shadowing an actual error code?
> 
> > +	}  
> 
> ...
> 
> > +			/*
> > +			 * / 10^4 to comply with IIO scale for CO2 (percentage).  
> 
> "1 / 10^4"
> 
> > +			 * The chip CO2 reading range is [400 - 5000] ppm
> > +			 * which corresponds to [0,004 - 0,5] %.
> > +			 */  
>
Jonathan Cameron Sept. 11, 2021, 3:29 p.m. UTC | #5
On Thu,  9 Sep 2021 11:45:36 +0200
Jacopo Mondi <jacopo@jmondi.org> wrote:

> 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>
Given you will be doing a v6 anyway, I noticed one small place inline where you can reduce
the scope of the lock and hence simplify error paths a bit (it occurs a few times in similar functions).

Regmap stuff looks acceptable to me, though if a better way comes from Peter's comments even
better!

Thanks,

Jonathan

> +
> +/* Custom regmap read/write operations: perform unlocked access to the i2c bus. */
> +
> +static int sunrise_regmap_read(void *context, const void *reg_buf,
> +			       size_t reg_size, void *val_buf, size_t val_size)
> +{
> +	struct i2c_client *client = context;
> +	union i2c_smbus_data data;
> +	int ret;
> +
> +	memset(&data, 0, sizeof(data));
> +	data.block[0] = val_size;
> +
> +	/*
> +	 * Wake up sensor by sending sensor address: START, sensor address,
> +	 * STOP. Sensor will not ACK this byte.
> +	 *
> +	 * The chip enters a 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);
> +
> +	ret = __i2c_smbus_xfer(client->adapter, client->addr, client->flags,
> +			       I2C_SMBUS_READ, ((u8 *)reg_buf)[0],
> +			       I2C_SMBUS_I2C_BLOCK_DATA, &data);
> +	if (ret < 0)
> +		return ret;
> +
> +	memcpy(val_buf, &data.block[1], data.block[0]);
> +
> +	return 0;
> +}
> +
> +static int sunrise_regmap_write(void *context, const void *val_buf, size_t count)
> +{
> +	struct i2c_client *client = context;
> +	union i2c_smbus_data data;
> +
> +	/* Discard reg address from values count. */
> +	if (count < 1)
> +		return -EINVAL;
> +	count--;
> +
> +	memset(&data, 0, sizeof(data));
> +	data.block[0] = count;
> +	memcpy(&data.block[1], (u8 *)val_buf + 1, count);
> +
> +	__i2c_smbus_xfer(client->adapter, client->addr, I2C_M_IGNORE_NAK,
> +			 I2C_SMBUS_WRITE, 0, I2C_SMBUS_QUICK, NULL);
> +
> +	return __i2c_smbus_xfer(client->adapter, client->addr, client->flags,
> +				I2C_SMBUS_WRITE, ((u8 *)val_buf)[0],
> +				I2C_SMBUS_I2C_BLOCK_DATA, &data);
> +}

This didn't end up looking too bad :)


...

> +
> +static ssize_t sunrise_cal_background_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 enable;
> +	int ret;
> +
> +	mutex_lock(&sunrise->lock);
> +	ret = kstrtobool(buf, &enable);
> +	if (ret)
> +		goto out_unlock;
> +
> +	if (!enable)
> +		goto out_unlock;

Trivial: Move these outside the lock (as only use local variables) and
then return directly from them.  That gets rid of the need for the label as well.

Same for other cases that look like this...

> +
> +	ret = sunrise_calibrate(sunrise, &calib_data[SUNRISE_CALIBRATION_BACKGROUND]);
> +
> +out_unlock:
> +	mutex_unlock(&sunrise->lock);
> +	if (ret)
> +		return ret;
> +
> +	return len;
> +}
> +

Thanks,

Jonathan
Jacopo Mondi Sept. 20, 2021, 1:02 p.m. UTC | #6
Hi Peter,

On Thu, Sep 09, 2021 at 02:04:31PM +0200, Peter Rosin wrote:
> Hi!
>
> Subject looks wrong, shouldn't it be 006-0-0007?
>
> On 2021-09-09 11:45, Jacopo Mondi wrote:
> > 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                        |   7 +
> >  drivers/iio/chemical/Kconfig       |  10 +
> >  drivers/iio/chemical/Makefile      |   1 +
> >  drivers/iio/chemical/sunrise_co2.c | 526 +++++++++++++++++++++++++++++
> >  4 files changed, 544 insertions(+)
> >  create mode 100644 drivers/iio/chemical/sunrise_co2.c
> >
>
> *snip*
>
> > + * the i2c bus.
> > + */
> > +static int sunrise_read_byte(struct sunrise_dev *sunrise, u8 reg)
> > +{
> > +	const struct i2c_client *client = sunrise->client;
> > +	const struct device *dev = &client->dev;
> > +	unsigned int val;
> > +	int ret;
> > +
> > +	i2c_lock_bus(client->adapter, I2C_LOCK_SEGMENT);
> > +	ret = regmap_read(sunrise->regmap, reg, &val);
> > +	i2c_unlock_bus(client->adapter, I2C_LOCK_SEGMENT);
> > +	if (ret) {
> > +		dev_err(dev, "Read byte failed: reg 0x%2x (%d)\n", reg, ret);
>
> Nitpick, %02x looks better methinks ("reg 0x04" vs. "reg 0x 4").
>
> [more instances below]
>

Sure!

> > +		return ret;
> > +	}
> > +
> > +	return val;
> > +}
> > +
> > +static int sunrise_read_word(struct sunrise_dev *sunrise, u8 reg, u16 *val)
> > +{
> > +	const struct i2c_client *client = sunrise->client;
> > +	const struct device *dev = &client->dev;
> > +	__be16 be_val;
> > +	int ret;
> > +
> > +	i2c_lock_bus(client->adapter, I2C_LOCK_SEGMENT);
> > +	ret = regmap_bulk_read(sunrise->regmap, reg, &be_val, sizeof(be_val));
> > +	i2c_unlock_bus(client->adapter, I2C_LOCK_SEGMENT);
> > +	if (ret) {
> > +		dev_err(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)
> > +{
> > +	const struct i2c_client *client = sunrise->client;
> > +	const struct device *dev = &client->dev;
> > +	int ret;
> > +
> > +	i2c_lock_bus(client->adapter, I2C_LOCK_SEGMENT);
> > +	ret = regmap_write(sunrise->regmap, reg, val);
> > +	i2c_unlock_bus(client->adapter, I2C_LOCK_SEGMENT);
> > +	if (ret)
> > +		dev_err(dev, "Write byte failed: reg 0x%2x (%d)\n", reg, ret);
> > +
> > +	return ret;
> > +}
> > +
> > +static int sunrise_write_word(struct sunrise_dev *sunrise, u8 reg, u16 data)
> > +{
> > +	const struct i2c_client *client = sunrise->client;
> > +	const struct device *dev = &client->dev;
> > +	__be16 be_data = cpu_to_be16(data);
> > +	int ret;
> > +
> > +	i2c_lock_bus(client->adapter, I2C_LOCK_SEGMENT);
> > +	ret = regmap_bulk_write(sunrise->regmap, reg, &be_data, sizeof(be_data));
> > +	i2c_unlock_bus(client->adapter, I2C_LOCK_SEGMENT);
> > +	if (ret)
> > +		dev_err(dev, "Write word failed: reg 0x%2x (%d)\n", reg, ret);
> > +
> > +	return ret;
> > +}
>
> *snip*
>
> > +
> > +static const struct regmap_bus sunrise_regmap_bus = {
> > +	.read = sunrise_regmap_read,
> > +	.write = sunrise_regmap_write,
> > +};
> > +
> > +static const 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;
> > +
> > +	sunrise = iio_priv(iio_dev);
> > +	sunrise->client = client;
>
> You should check if the I2C adapter supports the needed operations.
>
> 	if (!i2c_check_functionality(client->adapter,
> 				     I2C_FUNC_SMBUS_BLOCK_DATA |
> 				     I2C_FUNC_SMBUS_BYTE_DATA |
> 				     I2C_FUNC_SMBUS_QUICK |
> 				     I2C_FUNC_PROTOCOL_MANGLING))
> 		...
>
> And, I suspect that protocol mangling might not be the first thing simple
> SMBus-only adapters support?? Maybe you don't lose all that many adapters
> by not using the restart-after-wakeup method, when odd things are required
> anyway? That is, if the device gets enough time to wake up properly using
> that method? (see the message from Andy in the v3 discussion)

So, I've tried to apply the change you suggested in v3

static int sunrise_regmap_read(void *context, const void *reg_buf,
			       size_t reg_size, void *val_buf, size_t val_size)
{
	struct i2c_client *client = context;
	unsigned char reg =  ((char *)reg_buf)[0];
	unsigned char databuf[2];
        struct i2c_msg msg[3] = {
                {       /* wakeup */
                        .addr = client->addr,
                        .flags = I2C_M_RD | I2C_M_IGNORE_NAK,
                        .len = 0,
                }, {    /* write register number */
                        .addr = client->addr,
                        .flags = 0,
                        .len = reg_size,
                        .buf = &reg,
                }, {    /* read register contents */
                        .addr = client->addr,
                        .flags = I2C_M_RD,
                        .len = val_size,
                        .buf = databuf,
                },
        };
	int ret;

	ret = __i2c_transfer(client->adapter, msg, 3);
	if (ret != 3)
		return -EIO;

	memcpy(val_buf, databuf, val_size);

	return 0;

And I get a rather unstable system, ie communications -sometimes-
fail.

This becomes more evident when trying to calibrate, due to register
polling, but also simply reading one of the channels fails randomly.

I blame the chip wakeup time requirement between the wakeup message
and the rest. So unfortunately this version cannot replace the two
separate __i2c_smbu_xfer() calls I have now.

As the wakeup time is not characterized, I'll give it 1 msec even when
using raw __i2c_smbus_xfer() as suggested by Andy.

>
> SMBus quick isn't universal either...

This seems problematic, as I see few adapters support this (might be
the reason why there's no i2c_smbus_quick() helper ?).

I tried using I2C_SMBUS_WRITE in place of I2C_SMBUS_QUICK and the chip
seems to work properly (please not my adapater supports protocol
mangling though)

>
> But hmmm, maybe you don't *really* need protocol mangling when you don't
> check the return value of the wakeup call? At the same time, without
> I2C_M_IGNORE_NAK, you'll perhaps end up with boring entries in the log
> or the adapter doing retries or some other inconvenient crap?

Possibly, but I think it's fine as long as I don't check the error
code ?

>
> You could perhaps set I2C_M_IGNORE_NAK only if the adapter supports
> protocol mangling? Any maybe you don't care about losers who don't have
> a nice enough adapter. :-)

:) I'm on i2c-gpio, so it's all but fancy I guess..

Considering the above in v6 I will:

- Keep the two __i2c_smbu_xfer() calls
- Optional protocol mangling
- usleep_range(500, 1500) between wakup and actual message
- Use I2C_SMBUS_WRITE in place of I2C_SMBUS_QUICK for the wakeup
  message not to rule out too many adapters

Anything wrong with this in your opinion ?

Thanks
  j

>
> Cheers,
> Peter
>
> > +	mutex_init(&sunrise->lock);
> > +
> > +	sunrise->regmap = devm_regmap_init(&client->dev, &sunrise_regmap_bus,
> > +					   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
> >
diff mbox series

Patch

diff --git a/MAINTAINERS b/MAINTAINERS
index d7b4f32875a9..4f39e0d65e6d 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -16723,6 +16723,13 @@  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/ABI/testing/sysfs-bus-iio-chemical-sunrise-co2
+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 a4920646e9be..5155ab2caed4 100644
--- a/drivers/iio/chemical/Kconfig
+++ b/drivers/iio/chemical/Kconfig
@@ -159,6 +159,16 @@  config SPS30_SERIAL
 	  To compile this driver as a module, choose M here: the module will
 	  be called sps30_serial.

+config SENSEAIR_SUNRISE_CO2
+	tristate "Senseair Sunrise 006-0-0007 CO2 sensor"
+	select REGMAP_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 4898690cc155..61e8749a84f3 100644
--- a/drivers/iio/chemical/Makefile
+++ b/drivers/iio/chemical/Makefile
@@ -15,6 +15,7 @@  obj-$(CONFIG_PMS7003) += pms7003.o
 obj-$(CONFIG_SCD30_CORE) += scd30_core.o
 obj-$(CONFIG_SCD30_I2C) += scd30_i2c.o
 obj-$(CONFIG_SCD30_SERIAL) += scd30_serial.o
+obj-$(CONFIG_SENSEAIR_SUNRISE_CO2) += sunrise_co2.o
 obj-$(CONFIG_SENSIRION_SGP30)	+= sgp30.o
 obj-$(CONFIG_SPS30) += sps30.o
 obj-$(CONFIG_SPS30_I2C) += sps30_i2c.o
diff --git a/drivers/iio/chemical/sunrise_co2.c b/drivers/iio/chemical/sunrise_co2.c
new file mode 100644
index 000000000000..af845b0cb938
--- /dev/null
+++ b/drivers/iio/chemical/sunrise_co2.c
@@ -0,0 +1,526 @@ 
+// 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:
+ * - controllable EN pin
+ * - single-shot operations using the nDRY pin.
+ * - ABC/target calibration
+ */
+
+#include <linux/bitops.h>
+#include <linux/i2c.h>
+#include <linux/kernel.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/regmap.h>
+#include <linux/time64.h>
+
+#include <linux/iio/iio.h>
+
+#define DRIVER_NAME "sunrise_co2"
+
+#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		(30 * USEC_PER_SEC)
+
+struct sunrise_dev {
+	struct i2c_client *client;
+	struct regmap *regmap;
+	/* Protects access to IIO attributes. */
+	struct mutex lock;
+};
+
+/* Custom regmap read/write operations: perform unlocked access to the i2c bus. */
+
+static int sunrise_regmap_read(void *context, const void *reg_buf,
+			       size_t reg_size, void *val_buf, size_t val_size)
+{
+	struct i2c_client *client = context;
+	union i2c_smbus_data data;
+	int ret;
+
+	memset(&data, 0, sizeof(data));
+	data.block[0] = val_size;
+
+	/*
+	 * Wake up sensor by sending sensor address: START, sensor address,
+	 * STOP. Sensor will not ACK this byte.
+	 *
+	 * The chip enters a 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);
+
+	ret = __i2c_smbus_xfer(client->adapter, client->addr, client->flags,
+			       I2C_SMBUS_READ, ((u8 *)reg_buf)[0],
+			       I2C_SMBUS_I2C_BLOCK_DATA, &data);
+	if (ret < 0)
+		return ret;
+
+	memcpy(val_buf, &data.block[1], data.block[0]);
+
+	return 0;
+}
+
+static int sunrise_regmap_write(void *context, const void *val_buf, size_t count)
+{
+	struct i2c_client *client = context;
+	union i2c_smbus_data data;
+
+	/* Discard reg address from values count. */
+	if (count < 1)
+		return -EINVAL;
+	count--;
+
+	memset(&data, 0, sizeof(data));
+	data.block[0] = count;
+	memcpy(&data.block[1], (u8 *)val_buf + 1, count);
+
+	__i2c_smbus_xfer(client->adapter, client->addr, I2C_M_IGNORE_NAK,
+			 I2C_SMBUS_WRITE, 0, I2C_SMBUS_QUICK, NULL);
+
+	return __i2c_smbus_xfer(client->adapter, client->addr, client->flags,
+				I2C_SMBUS_WRITE, ((u8 *)val_buf)[0],
+				I2C_SMBUS_I2C_BLOCK_DATA, &data);
+}
+
+/*
+ * Sunrise i2c read/write operations: lock the i2c segment to avoid losing the
+ * wake up session. Use custom regmap operations that perform unlocked access to
+ * the i2c bus.
+ */
+static int sunrise_read_byte(struct sunrise_dev *sunrise, u8 reg)
+{
+	const struct i2c_client *client = sunrise->client;
+	const struct device *dev = &client->dev;
+	unsigned int val;
+	int ret;
+
+	i2c_lock_bus(client->adapter, I2C_LOCK_SEGMENT);
+	ret = regmap_read(sunrise->regmap, reg, &val);
+	i2c_unlock_bus(client->adapter, I2C_LOCK_SEGMENT);
+	if (ret) {
+		dev_err(dev, "Read byte failed: reg 0x%2x (%d)\n", reg, ret);
+		return ret;
+	}
+
+	return val;
+}
+
+static int sunrise_read_word(struct sunrise_dev *sunrise, u8 reg, u16 *val)
+{
+	const struct i2c_client *client = sunrise->client;
+	const struct device *dev = &client->dev;
+	__be16 be_val;
+	int ret;
+
+	i2c_lock_bus(client->adapter, I2C_LOCK_SEGMENT);
+	ret = regmap_bulk_read(sunrise->regmap, reg, &be_val, sizeof(be_val));
+	i2c_unlock_bus(client->adapter, I2C_LOCK_SEGMENT);
+	if (ret) {
+		dev_err(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)
+{
+	const struct i2c_client *client = sunrise->client;
+	const struct device *dev = &client->dev;
+	int ret;
+
+	i2c_lock_bus(client->adapter, I2C_LOCK_SEGMENT);
+	ret = regmap_write(sunrise->regmap, reg, val);
+	i2c_unlock_bus(client->adapter, I2C_LOCK_SEGMENT);
+	if (ret)
+		dev_err(dev, "Write byte failed: reg 0x%2x (%d)\n", reg, ret);
+
+	return ret;
+}
+
+static int sunrise_write_word(struct sunrise_dev *sunrise, u8 reg, u16 data)
+{
+	const struct i2c_client *client = sunrise->client;
+	const struct device *dev = &client->dev;
+	__be16 be_data = cpu_to_be16(data);
+	int ret;
+
+	i2c_lock_bus(client->adapter, I2C_LOCK_SEGMENT);
+	ret = regmap_bulk_write(sunrise->regmap, reg, &be_data, sizeof(be_data));
+	i2c_unlock_bus(client->adapter, I2C_LOCK_SEGMENT);
+	if (ret)
+		dev_err(dev, "Write word failed: reg 0x%2x (%d)\n", reg, ret);
+
+	return ret;
+}
+
+/* Trigger a calibration cycle. */
+
+enum {
+	SUNRISE_CALIBRATION_FACTORY,
+	SUNRISE_CALIBRATION_BACKGROUND,
+};
+
+static const struct sunrise_calib_data {
+	u16 cmd;
+	u8 bit;
+	const char * const name;
+} calib_data[] = {
+	[SUNRISE_CALIBRATION_FACTORY] = {
+		SUNRISE_CALIBRATION_FACTORY_CMD,
+		BIT(2),
+		"factory_calibration",
+	},
+	[SUNRISE_CALIBRATION_BACKGROUND] = {
+		SUNRISE_CALIBRATION_BACKGROUND_CMD,
+		BIT(5),
+		"background_calibration",
+	},
+};
+
+static int sunrise_calibrate(struct sunrise_dev *sunrise,
+			     const struct sunrise_calib_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 a calibration command and poll the calibration status bit. */
+	ret = sunrise_write_word(sunrise, SUNRISE_CALIBRATION_COMMAND_REG, data->cmd);
+	if (ret)
+		return ret;
+
+	dev_dbg(&sunrise->client->dev, "%s in progress\n", data->name);
+
+	/*
+	 * Calibration takes several seconds, so the sleep time between reads
+	 * can be pretty relaxed.
+	 */
+	return read_poll_timeout(sunrise_read_byte, status, status & data->bit,
+				 200000, SUNRISE_CALIBRATION_TIMEOUT_US, false,
+				 sunrise, SUNRISE_CALIBRATION_STATUS_REG);
+}
+
+static ssize_t sunrise_cal_factory_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 enable;
+	int ret;
+
+	mutex_lock(&sunrise->lock);
+	ret = kstrtobool(buf, &enable);
+	if (ret)
+		goto out_unlock;
+
+	if (!enable)
+		goto out_unlock;
+
+	ret = sunrise_calibrate(sunrise, &calib_data[SUNRISE_CALIBRATION_FACTORY]);
+
+out_unlock:
+	mutex_unlock(&sunrise->lock);
+	if (ret)
+		return ret;
+
+	return len;
+}
+
+static ssize_t sunrise_cal_background_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 enable;
+	int ret;
+
+	mutex_lock(&sunrise->lock);
+	ret = kstrtobool(buf, &enable);
+	if (ret)
+		goto out_unlock;
+
+	if (!enable)
+		goto out_unlock;
+
+	ret = sunrise_calibrate(sunrise, &calib_data[SUNRISE_CALIBRATION_BACKGROUND]);
+
+out_unlock:
+	mutex_unlock(&sunrise->lock);
+	if (ret)
+		return ret;
+
+	return len;
+}
+
+ /* 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 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);
+	unsigned long errors;
+	ssize_t len = 0;
+	u16 value;
+	int ret;
+	u8 i;
+
+	mutex_lock(&sunrise->lock);
+	ret = sunrise_read_word(sunrise, SUNRISE_ERROR_STATUS_REG, &value);
+	if (ret) {
+		mutex_unlock(&sunrise->lock);
+		return -EINVAL;
+	}
+
+	errors = value;
+	for_each_set_bit(i, &errors, ARRAY_SIZE(sunrise_error_statuses))
+		len += sysfs_emit_at(buf, len, "%s ", sunrise_error_statuses[i]);
+
+	if (len)
+		buf[len - 1] = '\n';
+
+	mutex_unlock(&sunrise->lock);
+
+	return len;
+}
+
+static const struct iio_chan_spec_ext_info sunrise_concentration_ext_info[] = {
+	/* Calibration triggers. */
+	{
+		.name = "calibration_factory",
+		.write = sunrise_cal_factory_write,
+		.shared = IIO_SEPARATE,
+	},
+	{
+		.name = "calibration_background",
+		.write = sunrise_cal_background_write,
+		.shared = IIO_SEPARATE,
+	},
+
+	/* Error statuses. */
+	{
+		.name = "error_status",
+		.read = sunrise_error_status_read,
+		.shared = IIO_SHARED_BY_ALL,
+	},
+	{
+		.name = "error_status_available",
+		.shared = IIO_SHARED_BY_ALL,
+		.read = iio_enum_available_read,
+		.private = (uintptr_t)&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) |
+				      BIT(IIO_CHAN_INFO_SCALE),
+		.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:
+		switch (chan->type) {
+		case IIO_CONCENTRATION:
+			mutex_lock(&sunrise->lock);
+			ret = sunrise_read_word(sunrise, SUNRISE_CO2_FILTERED_COMP_REG,
+						&value);
+			*val = value;
+			mutex_unlock(&sunrise->lock);
+
+			if (ret)
+				return ret;
+
+			return IIO_VAL_INT;
+
+		case IIO_TEMP:
+			mutex_lock(&sunrise->lock);
+			ret = sunrise_read_word(sunrise, SUNRISE_CHIP_TEMPERATURE_REG,
+						&value);
+			*val = value;
+			mutex_unlock(&sunrise->lock);
+
+			if (ret)
+				return ret;
+
+			return IIO_VAL_INT;
+
+		default:
+			return -EINVAL;
+		}
+
+	case IIO_CHAN_INFO_SCALE:
+		switch (chan->type) {
+		case IIO_CONCENTRATION:
+			/*
+			 * / 10^4 to comply with IIO scale for CO2 (percentage).
+			 * The chip CO2 reading range is [400 - 5000] ppm
+			 * which corresponds to [0,004 - 0,5] %.
+			 */
+			*val = 1;
+			*val2 = 10000;
+			return IIO_VAL_FRACTIONAL;
+
+		case IIO_TEMP:
+			/* x10 to comply with IIO scale (millidegrees celsius). */
+			*val = 10;
+			return IIO_VAL_INT;
+
+		default:
+			return -EINVAL;
+		}
+
+	default:
+		return -EINVAL;
+	}
+}
+
+static const struct iio_info sunrise_info = {
+	.read_raw = sunrise_read_raw,
+};
+
+static const struct regmap_bus sunrise_regmap_bus = {
+	.read = sunrise_regmap_read,
+	.write = sunrise_regmap_write,
+};
+
+static const 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;
+
+	sunrise = iio_priv(iio_dev);
+	sunrise->client = client;
+	mutex_init(&sunrise->lock);
+
+	sunrise->regmap = devm_regmap_init(&client->dev, &sunrise_regmap_bus,
+					   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");