On Tue, Aug 17, 2021 at 05:49:51PM +0200, Jacopo Mondi wrote:
> Add support for the Senseair Sunrise 006-0-0007 driver through the
> IIO subsystem.
Datasheet tag / link?
...
> +config SUNRISE
> + tristate "Senseair Sunrise 006-0-0007 CO2 sensor"
> + depends on I2C
> + help
> + Say yes here to build support for Senseair Sunrise 006-0-0007 CO2
> + sensor.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called sunrise.
Too generic name for configuration and module,
...
> + * iio/co2/sunrise-006-0-0007.c
> + *
Redundant noise.
...
> + * TODO: Add support for controllable EN pin
> + * TODO: Add support for single-shot operations by using the nDRY pin.
Ditto.
If it's not ready, then make it ready.
...
> +#include <linux/of_device.h>
Why? Perhaps mod_devicetable,h is what you had in mind.
...
> + i2c_smbus_read_byte(client);
Can you use regmap I²C API?
...
> +#define sunrise_readb_client(client, addr) \
> +({ \
> + u8 _val; \
> + sunrise_read_byte(client, addr, &_val); \
> + _val; \
> +})
> +#define sunrise_readb(addr) sunrise_readb_client(client, addr)
Drop ugly macros and use read_poll_timeout() below.
> +#define sunrise_poll_register(reg, val, cond) \
> + readx_poll_timeout(sunrise_readb, reg, val, cond, 100, 0)
...
> +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);
> + struct i2c_client *client = sunrise->client;
+ u16 value;
> + int ret;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
> +
> + mutex_lock(&sunrise->lock);
> +
> + switch (chan->type) {
> + case IIO_CONCENTRATION: {
> + u16 co2;
Reuse value.
> + ret = sunrise_read_co2_filtered(client, &co2);
> + *val = co2;
> + mutex_unlock(&sunrise->lock);
> +
> + return ret ? ret : IIO_VAL_INT;
Can be written as
return ret ?: IIO_VAL_INT;
but up to maintainers.
> + }
> +
> + case IIO_TEMP: {
> + u16 temp;
Reuse value.
> + ret = sunrise_read_word(client,
> + SUNRISE_CHIP_TEMPERATURE_REG,
> + &temp);
> + *val = temp;
> + mutex_unlock(&sunrise->lock);
> +
> + return ret ? ret : IIO_VAL_INT;
> + }
> +
> + default:
> + mutex_unlock(&sunrise->lock);
> + return -EINVAL;
> + }
> +
> + case IIO_CHAN_INFO_SCALE:
> + /* Chip temperature scale = 1/100 */
> + *val = 1;
> + *val2 = 100;
> + return IIO_VAL_FRACTIONAL;
> +
> + default:
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
...
> + return sunrise_poll_register(SUNRISE_CALIBRATION_STATUS_REG, status,
> + (status & BIT(5)));
Too many parentheses.
--
With Best Regards,
Andy Shevchenko
