Hi Andy, thanks for the immediate review :) On Mon, Aug 23, 2021 at 11:35:23AM +0300, Andy Shevchenko wrote: > On Mon, Aug 23, 2021 at 09:36:39AM +0200, Jacopo Mondi wrote: > > Add support for the Senseair Sunrise 006-0-0007 driver through the > > IIO subsystem. > > Thanks for this intermediate update, looks much better. > So, there are a few comments below and we are almost ready. Thanks, I would also like feedback on the usage of channel's ext_info to handle calibration instead of using raw attributes as suggested by Matt > > > Datasheet: https://rmtplusstoragesenseair.blob.core.windows.net/docs/Dev/publicerat/TDE5531.pdf > > Signed-off-by: Jacopo Mondi <jacopo@xxxxxxxxxx> > > --- > > v3->v3.1 > > - Remove debug leftover > > - Re-add commas at the end of arrays declarations > > --- > > MAINTAINERS | 6 + > > drivers/iio/chemical/Kconfig | 13 + > > drivers/iio/chemical/Makefile | 1 + > > drivers/iio/chemical/sunrise_co2.c | 448 +++++++++++++++++++++++++++++ > > 4 files changed, 468 insertions(+) > > create mode 100644 drivers/iio/chemical/sunrise_co2.c > > > > diff --git a/MAINTAINERS b/MAINTAINERS > > index 90ca9df1d3c3..43f5bba46673 100644 > > --- a/MAINTAINERS > > +++ b/MAINTAINERS > > @@ -16544,6 +16544,12 @@ S: Maintained > > F: drivers/misc/phantom.c > > F: include/uapi/linux/phantom.h > > > > +SENSEAIR SUNRISE 006-0-0007 > > +M: Jacopo Mondi <jacopo@xxxxxxxxxx> > > +S: Maintained > > +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@xxxxxxxxxxx> > > S: Maintained > > diff --git a/drivers/iio/chemical/Kconfig b/drivers/iio/chemical/Kconfig > > index 10bb431bc3ce..ee8562949226 100644 > > --- a/drivers/iio/chemical/Kconfig > > +++ b/drivers/iio/chemical/Kconfig > > @@ -144,6 +144,19 @@ config SPS30 > > To compile this driver as a module, choose M here: the module will > > be called sps30. > > > > +config SENSEAIR_SUNRISE_CO2 > > + tristate "Senseair Sunrise 006-0-0007 CO2 sensor" > > + depends on OF > > + depends on I2C > > + depends on SYSFS Do you think I need this ? The driver includes iio/sysfs.h but I found no symbol nor dependency for that > > + 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 fef63dd5bf92..d5e2a3331d57 100644 > > --- a/drivers/iio/chemical/Makefile > > +++ b/drivers/iio/chemical/Makefile > > @@ -17,4 +17,5 @@ obj-$(CONFIG_SCD30_I2C) += scd30_i2c.o > > obj-$(CONFIG_SCD30_SERIAL) += scd30_serial.o > > obj-$(CONFIG_SENSIRION_SGP30) += sgp30.o > > obj-$(CONFIG_SPS30) += sps30.o > > +obj-$(CONFIG_SENSEAIR_SUNRISE_CO2) += sunrise_co2.o > > obj-$(CONFIG_VZ89X) += vz89x.o > > diff --git a/drivers/iio/chemical/sunrise_co2.c b/drivers/iio/chemical/sunrise_co2.c > > new file mode 100644 > > index 000000000000..84f19df6fc00 > > --- /dev/null > > +++ b/drivers/iio/chemical/sunrise_co2.c > > @@ -0,0 +1,448 @@ > > +// 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> > > +#include <linux/iio/sysfs.h> > > > +#define DRIVER_NAME "sunrise" > > I would expect "sunrise_co2" here. > Yes, better as the driver has been updated > Also, since it's one time use, please drop the definition completely and use > literal in-place. > I got two usages ... iio_dev->name = DRIVER_NAME; ... .driver = { .name = DRIVER_NAME, Is it ok to keep it ? > > +#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) > > + > > +enum sunrise_calib { > > + SUNRISE_CALIBRATION_FACTORY, > > + SUNRISE_CALIBRATION_BACKGROUND, > > +}; > > + > > +struct sunrise_dev { > > + struct device *dev; > > + struct i2c_client *client; > > + struct regmap *regmap; > > + struct mutex lock; > > + enum sunrise_calib calibration; > > +}; > > + > > +static void sunrise_wakeup(struct sunrise_dev *sunrise) > > +{ > > + struct i2c_client *client = sunrise->client; > > + > > + /* > > + * Wake up sensor by sending sensor address: START, sensor address, > > + * STOP. Sensor will not ACK this byte. > > + * > > + * The chip returns in 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); > > +} > > + > > +static int sunrise_read_word(struct sunrise_dev *sunrise, u8 reg, u16 *val) > > +{ > > + __be16 be_val; > > + int ret; > > + > > + sunrise_wakeup(sunrise); > > + ret = regmap_bulk_read(sunrise->regmap, reg, &be_val, 2); > > + if (ret) { > > + dev_err(sunrise->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) > > +{ > > + int ret; > > + > > + sunrise_wakeup(sunrise); > > + ret = regmap_write(sunrise->regmap, reg, val); > > + if (ret) { > > + dev_err(sunrise->dev, "Write byte failed: reg 0x%2x (%d)\n", reg, ret); > > + return ret; > > + } > > + > > + return 0; > > +} > > + > > +static int sunrise_write_word(struct sunrise_dev *sunrise, u8 reg, u16 data) > > +{ > > + __be16 be_data = cpu_to_be16(data); > > + int ret; > > + > > + sunrise_wakeup(sunrise); > > + ret = regmap_bulk_write(sunrise->regmap, reg, &be_data, 2); > > + if (ret) { > > + dev_err(sunrise->dev, "Write word failed: reg 0x%2x (%d)\n", reg, ret); > > + return ret; > > + } > > + > > + return 0; > > +} > > + > > +/* > > + * --- Calibration --- > > + * > > + * Enumerate and select calibration modes, trigger a calibration cycle. > > + */ > > +static const char * const sunrise_calibration_modes[] = { > > + [SUNRISE_CALIBRATION_FACTORY] = "factory_calibration", > > + [SUNRISE_CALIBRATION_BACKGROUND] = "background_calibration", > > +}; > > + > > +static const struct sunrise_calibration_data { > > + u16 calibration_cmd; > > + u8 calibration_bit; > > +} sunrise_calibrations[] = { > > + [SUNRISE_CALIBRATION_FACTORY] = { > > + SUNRISE_CALIBRATION_FACTORY_CMD, > > + BIT(2), > > + }, > > + [SUNRISE_CALIBRATION_BACKGROUND] = { > > + SUNRISE_CALIBRATION_BACKGROUND_CMD, > > + BIT(5), > > + }, > > +}; > > + > > +static int sunrise_calibrate(struct sunrise_dev *sunrise) > > +{ > > + const struct sunrise_calibration_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. */ > > + data = &sunrise_calibrations[sunrise->calibration]; > > + ret = sunrise_write_word(sunrise, SUNRISE_CALIBRATION_COMMAND_REG, > > + data->calibration_cmd); > > + if (ret) > > + return ret; > > + > > + dev_dbg(sunrise->dev, "%s in progress\n", > > + sunrise_calibration_modes[sunrise->calibration]); > > + > > + return regmap_read_poll_timeout(sunrise->regmap, > > + SUNRISE_CALIBRATION_STATUS_REG, > > + status, status & data->calibration_bit, > > + 100, SUNRISE_CALIBRATION_TIMEOUT_US); > > +} > > + > > +static ssize_t sunrise_calibration_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 calibrate; > > + int ret; > > + > > + ret = kstrtobool(buf, &calibrate); > > + if (ret) > > + return ret; > > + > > + if (!calibrate) > > + return 0; > > + > > + ret = sunrise_calibrate(sunrise); > > + if (ret) > > + return ret; > > + > > + return len; > > +} > > + > > +static int sunrise_set_calibration_mode(struct iio_dev *iiodev, > > + const struct iio_chan_spec *chan, > > + unsigned int mode) > > +{ > > + struct sunrise_dev *sunrise = iio_priv(iiodev); > > + > > + mutex_lock(&sunrise->lock); > > + sunrise->calibration = mode; > > + mutex_unlock(&sunrise->lock); > > + > > + return 0; > > +} > > + > > +static int sunrise_get_calibration_mode(struct iio_dev *iiodev, > > + const struct iio_chan_spec *chan) > > +{ > > + struct sunrise_dev *sunrise = iio_priv(iiodev); > > + int mode; > > + > > + mutex_lock(&sunrise->lock); > > + mode = sunrise->calibration; > > + mutex_unlock(&sunrise->lock); > > + > > + return mode; > > +} > > + > > +static const struct iio_enum sunrise_calibration_modes_enum = { > > + .items = sunrise_calibration_modes, > > + .num_items = ARRAY_SIZE(sunrise_calibration_modes), > > + .set = sunrise_set_calibration_mode, > > + .get = sunrise_get_calibration_mode, > > +}; > > + > > +/* --- Error status--- > > + * > > + * 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 u8 error_codes[] = { > > + 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 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); > > + const unsigned long *errors; > > + ssize_t len = 0; > > + u16 value; > > + int ret; > > + u8 i; > > + > > + ret = sunrise_read_word(sunrise, SUNRISE_ERROR_STATUS_REG, &value); > > + if (ret) > > + return -EINVAL; > > + > > + errors = (const unsigned long *)&value; > > Yes, it takes a pointer, but in your case it will oops the kernel quite likely. The usage of a pointer kind of puzzled me in first place, and I found no pattern to copy in the existing code base. I have probbaly not looked hard enough ? > > unsigned long errors; > > ... > > errors = value; > for_each_set_bit(..., &errors, ...) I can do so but, for my education mostly, why do you think it would oops ? '*errors' is a pointer to a variable allocated on the stack as much as 'errors' you suggested is a local stack variable. I might be a bit slow today, but I'm missing the difference. FWIW I tested the function, even if there were no error bits set at the moment I tested, but the for_each_set_bit() macro was indeed run. > > > + for_each_set_bit(i, errors, ARRAY_SIZE(error_codes)) > > + len += sysfs_emit_at(buf, len, "%s ", sunrise_error_statuses[i]); > > + > > + if (len) > > + buf[len - 1] = '\n'; > > + > > + return len; > > +} > > + > > +static const struct iio_chan_spec_ext_info sunrise_concentration_ext_info[] = { > > + /* Calibration modes and calibration trigger. */ > > + { > > + .name = "calibration", > > + .write = sunrise_calibration_write, > > + .shared = IIO_SEPARATE, > > + }, > > + IIO_ENUM("calibration_mode", IIO_SEPARATE, > > + &sunrise_calibration_modes_enum), > > + IIO_ENUM_AVAILABLE("calibration_mode", > > + &sunrise_calibration_modes_enum), > > + > > + /* Error statuses. */ > > + { > > + .name = "error_status", > > + .read = sunrise_error_status_read, > > + .shared = IIO_SEPARATE > > + }, > > + IIO_ENUM_AVAILABLE("error_status", &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), > > + .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: > > + mutex_lock(&sunrise->lock); > > + > > + switch (chan->type) { > > + case IIO_CONCENTRATION: > > + ret = sunrise_read_word(sunrise, SUNRISE_CO2_FILTERED_COMP_REG, > > + &value); > > + *val = value; > > + mutex_unlock(&sunrise->lock); > > + > > + return ret ?: IIO_VAL_INT; > > + > > + case IIO_TEMP: > > + ret = sunrise_read_word(sunrise, SUNRISE_CHIP_TEMPERATURE_REG, > > + &value); > > + *val = value; > > + mutex_unlock(&sunrise->lock); > > + > > + return 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; > > + } > > +} > > + > > +static const struct iio_info sunrise_info = { > > + .read_raw = sunrise_read_raw, > > +}; > > + > > +static struct regmap_config sunrise_regmap_config = { > > + .reg_bits = 8, > > + .val_bits = 8, > > Does it need a lock? > (I think it does, but I would like to confirm) > You know, I had the same doubt, but the description of a few fields of struct regmap_config lead me to think there's a locking mechanism already inplace * @disable_locking: This regmap is either protected by external means or * is guaranteed not to be accessed from multiple threads. * Don't use any locking mechanisms. * @lock: Optional lock callback (overrides regmap's default lock * function, based on spinlock or mutex). As you can see 'lock' is option and is said to override regmap's default lock functions. Locking seems to have be disabled explicitely through 'disable_locking' too. I was then expecting a reference to a locally declared mutex/spinlock to be passed through regmap_config if the regmap's locking mechanism requires driver-allocated locking primitives. This suggests to me there's an internal locking. In facts, regmap's core seems to have an internal mutex and a built-in locking mechanism, as shown by __regmap_init(), which selects the opportune locking primitive according to how regmap_config is initialized. In our case it seems it relies on the usage of the regmap_lock_mutex() and regmap_unlock_mutex() functions, which use struct regmap.mutex. I think we're then safe locking-wise, do you agree ? That said, as I'm not pushing to have the driver accepted for this merge window, for which we might be late already, I would wait for comments on the usage of the ext_channel abstraction from IIO people before submitting a new version if that's fine with everyone. Thanks again for the quick comments! > > +}; > > + > > +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; > > + > > + i2c_set_clientdata(client, iio_dev); > > + > > + sunrise = iio_priv(iio_dev); > > + sunrise->client = client; > > + sunrise->dev = &client->dev; > > + mutex_init(&sunrise->lock); > > + > > + sunrise->regmap = devm_regmap_init_i2c(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@xxxxxxxxxx>"); > > +MODULE_DESCRIPTION("Senseair Sunrise 006-0-0007 CO2 sensor IIO driver"); > > +MODULE_LICENSE("GPL v2"); > > -- > > 2.32.0 > > > > -- > With Best Regards, > Andy Shevchenko > >
