| Message ID | 20210823073639.13688-1-jacopo@jmondi.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | None | expand |
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. > Datasheet: https://rmtplusstoragesenseair.blob.core.windows.net/docs/Dev/publicerat/TDE5531.pdf > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > 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@jmondi.org> > +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@octakon.com> > 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 > + 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. Also, since it's one time use, please drop the definition completely and use literal in-place. > +#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. unsigned long errors; ... errors = value; for_each_set_bit(..., &errors, ...) > + 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) > +}; > + > +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@jmondi.org>"); > +MODULE_DESCRIPTION("Senseair Sunrise 006-0-0007 CO2 sensor IIO driver"); > +MODULE_LICENSE("GPL v2"); > -- > 2.32.0 >
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@jmondi.org> > > --- > > 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@jmondi.org> > > +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@octakon.com> > > 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@jmondi.org>"); > > +MODULE_DESCRIPTION("Senseair Sunrise 006-0-0007 CO2 sensor IIO driver"); > > +MODULE_LICENSE("GPL v2"); > > -- > > 2.32.0 > > > > -- > With Best Regards, > Andy Shevchenko > >
On Mon, Aug 23, 2021 at 11:06:10AM +0200, Jacopo Mondi wrote: > 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: ... > > 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 Better to wait for Jonathan. ... > > > + depends on SYSFS > > Do you think I need this ? The driver includes iio/sysfs.h but I found > no symbol nor dependency for that Ditto. ... > > 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 ? Ah, okay then! ... > > > + 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. Because you theoretically access bytes behind 16-bit. That castings just ugly and compiler should warn you about if it is clever enough. If you found it in the existing code, please, fix it there, so quite bad pattern won't spread. > > > + for_each_set_bit(i, errors, ARRAY_SIZE(error_codes)) > > > + len += sysfs_emit_at(buf, len, "%s ", sunrise_error_statuses[i]); ... > > > +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 Exactly! But see below. > * @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 ? My point is do you need regmap locking mechanism to be used or not? > 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.
Hello, On Mon, Aug 23, 2021 at 12:40:00PM +0300, Andy Shevchenko wrote: > On Mon, Aug 23, 2021 at 11:06:10AM +0200, Jacopo Mondi wrote: > > 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: > > ... > > > > 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 > > Better to wait for Jonathan. > > ... > > > > > + depends on SYSFS > > > > Do you think I need this ? The driver includes iio/sysfs.h but I found > > no symbol nor dependency for that > > Ditto. > > ... > > > > 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 ? > > Ah, okay then! > > ... > > > > > + 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. > > Because you theoretically access bytes behind 16-bit. That castings just ugly > and compiler should warn you about if it is clever enough. I don't think there's such a risk as I limit the search space to ARRAY_SIZE(error_codes), but I agree the cast is ugly and I'll change to what you suggested > > If you found it in the existing code, please, fix it there, so quite bad > pattern won't spread. > My point was that I was not able to find any pattern to copy from :) > > > > + for_each_set_bit(i, errors, ARRAY_SIZE(error_codes)) > > > > + len += sysfs_emit_at(buf, len, "%s ", sunrise_error_statuses[i]); > > ... > > > > > +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 > > Exactly! But see below. > > > * @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 ? > > My point is do you need regmap locking mechanism to be used or not? > Oh I see, sorry for the useless digression, you were asking if I should not disable locking, not the other way around! Anyway, there's a risk for a concurrent read/write when in example reading the error status and triggering a calibration. There's no locking at the driver level in those functions as they do not access shared driver's fields, but on the wire there might be conflicting transactions, so I think I should keep locking in place. Thanks j > > 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. > > -- > With Best Regards, > Andy Shevchenko > >
On Mon, Aug 23, 2021 at 1:20 PM Jacopo Mondi <jacopo@jmondi.org> wrote: > On Mon, Aug 23, 2021 at 12:40:00PM +0300, Andy Shevchenko wrote: > > On Mon, Aug 23, 2021 at 11:06:10AM +0200, Jacopo Mondi wrote: > > > 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: ... > > > > > + 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. > > > > Because you theoretically access bytes behind 16-bit. That castings just ugly > > and compiler should warn you about if it is clever enough. > > I don't think there's such a risk as I limit the search space to > ARRAY_SIZE(error_codes), but I agree the cast is ugly and I'll change > to what you suggested More for the sake of education. Actually it's not theoretical. Some architectures may not access longs on unaligned (16-bit) addresses. So, yes, oops is probable. Another example is reading the long to the register. This can cross the page boundary and produce fault (again) oops. > > If you found it in the existing code, please, fix it there, so quite bad > > pattern won't spread. > > My point was that I was not able to find any pattern to copy from :) > > > > > > + for_each_set_bit(i, errors, ARRAY_SIZE(error_codes)) > > > > > + len += sysfs_emit_at(buf, len, "%s ", sunrise_error_statuses[i]);
On Mon, 23 Aug 2021 14:09:21 +0300 Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Mon, Aug 23, 2021 at 1:20 PM Jacopo Mondi <jacopo@jmondi.org> wrote: > > On Mon, Aug 23, 2021 at 12:40:00PM +0300, Andy Shevchenko wrote: > > > On Mon, Aug 23, 2021 at 11:06:10AM +0200, Jacopo Mondi wrote: > > > > 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: > > ... > > > > > > > + 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. > > > > > > Because you theoretically access bytes behind 16-bit. That castings just ugly > > > and compiler should warn you about if it is clever enough. > > > > I don't think there's such a risk as I limit the search space to > > ARRAY_SIZE(error_codes), but I agree the cast is ugly and I'll change > > to what you suggested > > More for the sake of education. Actually it's not theoretical. Some > architectures may not access longs on unaligned (16-bit) addresses. > So, yes, oops is probable. > Another example is reading the long to the register. This can cross > the page boundary and produce fault (again) oops. For extra giggles, (and I'm half asleep today so may have this backwards) what it the platform is big endian and you do this? I'm fairly sure it will walk the bits from low to high and so the first access will be off the end of your shorter type being as it will be at addr + sizeof (long) - 1 bit 7. > > > > If you found it in the existing code, please, fix it there, so quite bad > > > pattern won't spread. > > > > My point was that I was not able to find any pattern to copy from :) > > > > > > > > + for_each_set_bit(i, errors, ARRAY_SIZE(error_codes)) > > > > > > + len += sysfs_emit_at(buf, len, "%s ", sunrise_error_statuses[i]); >
On Mon, 23 Aug 2021 09:36:39 +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> > --- > v3->v3.1 Always do a new version of the whole series. The automated tools that maintainers mostly use these days (e.g. b4) are pointed out a whole series when picking it up. This means we have to do it manually, one patch at a time I think which is annoying. > - 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@jmondi.org> > +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@octakon.com> > 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 Not needed. > + depends on I2C regmap_i2c select should bring that in. > + depends on SYSFS I'd be surprised if this necessary... Everything should be stubbed appropriately if its' not there. > + 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> What are you using from this header? > + > +#define DRIVER_NAME "sunrise" > + > +#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); Hmm. Technically there isn't anything stopping another user of the i2c bus sneaking in between the wakeup and the following command. That would make the device going back to sleep a lot more likely. I can't off the top of my head remember if regmap lets you lock the bus. If not, you'll have to use the underlying i2c bus locking functions. https://elixir.bootlin.com/linux/latest/source/drivers/iio/temperature/mlx90614.c#L432 gives an example. Perhaps worth a look is the regmap-sccb implementation which has a dance that looks a tiny bit like what you have to do here (be it for a different reason). It might be nice to do something similar here and have a custom regmap bus which has the necessary wakeups in the relevant places. Note I haven't thought it through in depth, so it might not work! > + 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. */ I was kind of assuming the locking around calibration mode was to avoid races with this. Hence, why no lock here? > + 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; return len or an error code. Not 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--- /* * --- Error status --- If you really want to do the heading. I'm not sure it adds anything over the fairly short description that follows, so I'd just have /* Enumerate and retrieve the chip 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; Copy it to an unsigned long as discussed in other branch of this thread. > + for_each_set_bit(i, errors, ARRAY_SIZE(error_codes)) Unless I'm going crazy, ARRAY_SIZE(sunrise_error_statuses) == ARRAY_SIZE(error_codes) and so there isn't any point in having the error_codes array. > + 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), I'll comment on this ABI in the docs patch rather than here. Given you asked somewhere about ext_info vs explicit attrs. In theory we always prefer ext_info because it provides a way to access them from in kernel consumers + enforces naming etc. However as we have a massive number of legacy attributes I haven't yet started insisting on it, even for new drivers. Good to see it here though! > + 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; I mentioned in a late response to an earlier one that I'm not overly keen on this form, but I can live with it if you prefer it. > + > + 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); Move the locks into the two case statements, then you won't have to unlock here which will be cleaner. > + return -EINVAL; > + } > + > + case IIO_CHAN_INFO_SCALE: > + /* Chip temperature scale = 1/100 */ IIO temperatures are measured in milli degrees. 1lsb = 1/100*1000 degrees centigrade seems very accurate for a device like this! I'm guessing this should be 10. > + *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, > +}; > + > +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); Why? I'm not immediately spotting where this is used. > + > + sunrise = iio_priv(iio_dev); > + sunrise->client = client; > + sunrise->dev = &client->dev; Why carry this around when you can get it from 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@jmondi.org>"); > +MODULE_DESCRIPTION("Senseair Sunrise 006-0-0007 CO2 sensor IIO driver"); > +MODULE_LICENSE("GPL v2"); > -- > 2.32.0 >
On Sun, Aug 29, 2021 at 7:18 PM Jonathan Cameron <jic23@kernel.org> wrote: > On Mon, 23 Aug 2021 14:09:21 +0300 > Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Mon, Aug 23, 2021 at 1:20 PM Jacopo Mondi <jacopo@jmondi.org> wrote: > > > On Mon, Aug 23, 2021 at 12:40:00PM +0300, Andy Shevchenko wrote: > > > > On Mon, Aug 23, 2021 at 11:06:10AM +0200, Jacopo Mondi wrote: > > > > > 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: ... > > > > > > > + 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. > > > > > > > > Because you theoretically access bytes behind 16-bit. That castings just ugly > > > > and compiler should warn you about if it is clever enough. > > > > > > I don't think there's such a risk as I limit the search space to > > > ARRAY_SIZE(error_codes), but I agree the cast is ugly and I'll change > > > to what you suggested > > > > More for the sake of education. Actually it's not theoretical. Some > > architectures may not access longs on unaligned (16-bit) addresses. > > So, yes, oops is probable. > > Another example is reading the long to the register. This can cross > > the page boundary and produce fault (again) oops. > > For extra giggles, (and I'm half asleep today so may have this backwards) > what it the platform is big endian and you do this? I'm fairly sure it will > walk the bits from low to high and so the first access will be off the end > of your shorter type being as it will be at addr + sizeof (long) - 1 bit 7. Thanks, I forgot to think about this case. Maybe being half asleep is not a bad thing after all? :-) > > > > If you found it in the existing code, please, fix it there, so quite bad > > > > pattern won't spread. > > > > > > My point was that I was not able to find any pattern to copy from :) > > > > > > > > > > + for_each_set_bit(i, errors, ARRAY_SIZE(error_codes)) > > > > > > > + len += sysfs_emit_at(buf, len, "%s ", sunrise_error_statuses[i]);
Hi Jonathan, thanks for review On Sun, Aug 29, 2021 at 05:54:13PM +0100, Jonathan Cameron wrote: > On Mon, 23 Aug 2021 09:36:39 +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> > > --- > > v3->v3.1 > > Always do a new version of the whole series. The automated tools that maintainers > mostly use these days (e.g. b4) are pointed out a whole series when picking it up. > > This means we have to do it manually, one patch at a time I think which is annoying. > Right, sorry, it's pretty common in other subsystems for minor updates, but I understand it's more work on your side! Sorry about that! > > > - 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@jmondi.org> > > +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@octakon.com> > > 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 > > Not needed. > Well, the driver won't be probed as it doesn't have an i2c id table. But I guess it compiles fine > > + depends on I2C > > regmap_i2c select should bring that in. > > > + depends on SYSFS > > I'd be surprised if this necessary... Everything should be stubbed appropriately if > its' not there. I asked the same question as I didn't find any symbol related to iio/sysfs to depend on. I should have guessed it is not needed > > + 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> > > What are you using from this header? > Leftover from when I used raw attributes. I'll drop > > + > > +#define DRIVER_NAME "sunrise" > > + > > +#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); > > Hmm. Technically there isn't anything stopping another user of the i2c bus sneaking in > between the wakeup and the following command. That would make the device going back > to sleep a lot more likely. I can't off the top of my head remember if regmap lets > you lock the bus. If not, you'll have to use the underlying i2c bus locking functions. > https://elixir.bootlin.com/linux/latest/source/drivers/iio/temperature/mlx90614.c#L432 > gives an example. Right, there might be another call stealing the wakeup session! I should lock the underlying i2c bus, probably not root adapter like mlx90614 does but only the segment. > > Perhaps worth a look is the regmap-sccb implementation which has a dance that looks > a tiny bit like what you have to do here (be it for a different reason). > It might be nice to do something similar here and have a custom regmap bus which > has the necessary wakeups in the relevant places. > > Note I haven't thought it through in depth, so it might not work! the dance is similar if not regmap-sccb tranfers a byte instead of sending only the R/W bit (notice the usage of I2C_SMBUS_QUICK here and I2C_SMBUS_BYTE in regmap-sccb). Practically speaking it makes no difference as the sensor nacks the first message, so the underlying bus implementation bails out, but that's a bit of work-by-accident thing, isn't it ? If fine with you, I would stick to this implementation and hold the segment locked between the wakup and the actual messages. > > > + 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. */ > > I was kind of assuming the locking around calibration mode was to avoid races > with this. Hence, why no lock here? > As I assumed that as long as the write on the sysfs file does not return all other attempts would have to wait. And this function only returns when calibration is completed. However one can open a file with O_NONBLOCK, does this apply to a syfs attribute as well ? In that case yes, I should lock here. > > + 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; > > return len or an error code. Not 0, > Ack > > + > > + 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--- > /* > * --- Error status --- > > If you really want to do the heading. I'm not sure it adds anything over > the fairly short description that follows, so I'd just have > > /* Enumerate and retrieve the chip error status */ > Ok, I'll keep my comment headers style for me next time :) > > + * > > + * 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; > > Copy it to an unsigned long as discussed in other branch of this thread. > Ack, done in v3.1 > > + for_each_set_bit(i, errors, ARRAY_SIZE(error_codes)) > > Unless I'm going crazy, ARRAY_SIZE(sunrise_error_statuses) == ARRAY_SIZE(error_codes) > and so there isn't any point in having the error_codes array. > Uh, you're right. I thought I had to layout error bits in an array to cycle on them, but what I care about is the size only, so sunrise_error_statuses will do just fine. I'll drop > > + 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), > > I'll comment on this ABI in the docs patch rather than here. > Given you asked somewhere about ext_info vs explicit attrs. > In theory we always prefer ext_info because it provides a way to access them from > in kernel consumers + enforces naming etc. However as we have a massive number > of legacy attributes I haven't yet started insisting on it, even for new drivers. > > Good to see it here though! > Good! I'll keep using ext_info and update the ABI as you suggested. Be aware though that the chip supports up to 5 calibration modes, and we'll have one attribute for each of them, that's why I thought having 2 only was better. With an ack to potentially have 5 attrs, I'll change the ABI > > > + 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; > > I mentioned in a late response to an earlier one that I'm not overly keen on this form, but > I can live with it if you prefer it. > Whatever, really, I've done a few back and forth already. I'm more accustomed to the canonical if (ret) return ret; as well fwiw > > + > > + 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); > > Move the locks into the two case statements, then you won't have to unlock here which > will be cleaner. Ack > > > + return -EINVAL; > > + } > > + > > + case IIO_CHAN_INFO_SCALE: > > + /* Chip temperature scale = 1/100 */ > > IIO temperatures are measured in milli degrees. 1lsb = 1/100*1000 degrees centigrade seems very accurate > for a device like this! I'm guessing this should be 10. Ah yes, I thought it had to be given in the chip's native format, which is 1/100 degree. I guess I should then multiply by 10 the temperature raw read and return IIO_VAL_INT here. > > > + *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, > > +}; > > + > > +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); > > Why? I'm not immediately spotting where this is used. > Probably a leftover since when I used raw attrs ? > > + > > + sunrise = iio_priv(iio_dev); > > + sunrise->client = client; > > + sunrise->dev = &client->dev; > > Why carry this around when you can get it from client->dev? > Andy had the same comment. I think it's very cheap and makes the error printing more compact. But if it bothers both of you I can drop it. Thanks j > > + 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@jmondi.org>"); > > +MODULE_DESCRIPTION("Senseair Sunrise 006-0-0007 CO2 sensor IIO driver"); > > +MODULE_LICENSE("GPL v2"); > > -- > > 2.32.0 > > >
On Mon, Aug 30, 2021 at 06:20:55PM +0200, Jacopo Mondi wrote: > > > + case IIO_CHAN_INFO_SCALE: > > > + /* Chip temperature scale = 1/100 */ > > > > IIO temperatures are measured in milli degrees. 1lsb = 1/100*1000 degrees centigrade seems very accurate > > for a device like this! I'm guessing this should be 10. > > Ah yes, I thought it had to be given in the chip's native format, > which is 1/100 degree. > > I guess I should then multiply by 10 the temperature raw read and > return IIO_VAL_INT here. Of course I don't have to multiply by 10 the raw value, and that's what the _scale attribute is for. Fingers faster than brain it seems. And I'm a slow typer.
On Mon, 30 Aug 2021 18:20:51 +0200 Jacopo Mondi <jacopo@jmondi.org> wrote: > Hi Jonathan, > thanks for review > > On Sun, Aug 29, 2021 at 05:54:13PM +0100, Jonathan Cameron wrote: > > On Mon, 23 Aug 2021 09:36:39 +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> > > > --- > > > v3->v3.1 > > > > Always do a new version of the whole series. The automated tools that maintainers > > mostly use these days (e.g. b4) are pointed out a whole series when picking it up. > > > > This means we have to do it manually, one patch at a time I think which is annoying. > > > > Right, sorry, it's pretty common in other subsystems for minor > updates, but I understand it's more work on your side! Sorry about > that! > > > > > > - 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@jmondi.org> > > > +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@octakon.com> > > > 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 > > > > Not needed. > > > > Well, the driver won't be probed as it doesn't have an i2c id table. > But I guess it compiles fine So you'd think. But let me introduce you to ACPI PRP0001 which oddly uses the of_device_id table to bind to a device in an ACPI DSDT table ;) > > > + depends on I2C > > > > regmap_i2c select should bring that in. > > > > > + depends on SYSFS > > > > I'd be surprised if this necessary... Everything should be stubbed appropriately if > > its' not there. > > I asked the same question as I didn't find any symbol related to > iio/sysfs to depend on. I should have guessed it is not needed > > > > + 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> > > > > What are you using from this header? > > > > Leftover from when I used raw attributes. I'll drop > > > > + > > > +#define DRIVER_NAME "sunrise" > > > + > > > +#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); > > > > Hmm. Technically there isn't anything stopping another user of the i2c bus sneaking in > > between the wakeup and the following command. That would make the device going back > > to sleep a lot more likely. I can't off the top of my head remember if regmap lets > > you lock the bus. If not, you'll have to use the underlying i2c bus locking functions. > > https://elixir.bootlin.com/linux/latest/source/drivers/iio/temperature/mlx90614.c#L432 > > gives an example. > > Right, there might be another call stealing the wakeup session! > > I should lock the underlying i2c bus, probably not root adapter like > mlx90614 does but only the segment. Ideally only segment as you say as could be some muxes involved. > > > > > Perhaps worth a look is the regmap-sccb implementation which has a dance that looks > > a tiny bit like what you have to do here (be it for a different reason). > > It might be nice to do something similar here and have a custom regmap bus which > > has the necessary wakeups in the relevant places. > > > > Note I haven't thought it through in depth, so it might not work! > > the dance is similar if not regmap-sccb tranfers a byte instead of > sending only the R/W bit (notice the usage of I2C_SMBUS_QUICK here and > I2C_SMBUS_BYTE in regmap-sccb). Practically speaking it makes no > difference as the sensor nacks the first message, so the underlying > bus implementation bails out, but that's a bit of work-by-accident > thing, isn't it ? > > If fine with you, I would stick to this implementation and hold the > segment locked between the wakup and the actual messages. That's fine. I was just thinking you could hid the magic in a custom regmap then the rest of the driver would not have to be aware of it. Slightly neater than wrapping regmap functions with this extra call in the wrapper. > > > > > > + 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. */ > > > > I was kind of assuming the locking around calibration mode was to avoid races > > with this. Hence, why no lock here? > > > > As I assumed that as long as the write on the sysfs file does not > return all other attempts would have to wait. And this function only > returns when calibration is completed. However one can open a file > with O_NONBLOCK, does this apply to a syfs attribute as well ? In that > case yes, I should lock here. IIRC sysfs doesn't have any such protections, particularly not if multiple files are involved. > > > > + 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; > > > > return len or an error code. Not 0, > > > > Ack > > > > + > > > + 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--- > > /* > > * --- Error status --- > > > > If you really want to do the heading. I'm not sure it adds anything over > > the fairly short description that follows, so I'd just have > > > > /* Enumerate and retrieve the chip error status */ > > > > Ok, I'll keep my comment headers style for me next time :) > > > > + * > > > + * 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; > > > > Copy it to an unsigned long as discussed in other branch of this thread. > > > > Ack, done in v3.1 > > > > + for_each_set_bit(i, errors, ARRAY_SIZE(error_codes)) > > > > Unless I'm going crazy, ARRAY_SIZE(sunrise_error_statuses) == ARRAY_SIZE(error_codes) > > and so there isn't any point in having the error_codes array. > > > > Uh, you're right. I thought I had to layout error bits in an array to > cycle on them, but what I care about is the size only, so > sunrise_error_statuses will do just fine. I'll drop > > > > + 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), > > > > I'll comment on this ABI in the docs patch rather than here. > > Given you asked somewhere about ext_info vs explicit attrs. > > In theory we always prefer ext_info because it provides a way to access them from > > in kernel consumers + enforces naming etc. However as we have a massive number > > of legacy attributes I haven't yet started insisting on it, even for new drivers. > > > > Good to see it here though! > > > > Good! I'll keep using ext_info and update the ABI as you suggested. > Be aware though that the chip supports up to 5 calibration modes, and > we'll have one attribute for each of them, that's why I thought having > 2 only was better. With an ack to potentially have 5 attrs, I'll > change the ABI 5 clearly defined atts is fine. If it was 100s I might have a different opinion! > > > > > > + 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; > > > > I mentioned in a late response to an earlier one that I'm not overly keen on this form, but > > I can live with it if you prefer it. > > > > Whatever, really, I've done a few back and forth already. I'm more > accustomed to the canonical if (ret) return ret; as well fwiw > > > + > > > + 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); > > > > Move the locks into the two case statements, then you won't have to unlock here which > > will be cleaner. > > Ack > > > > > > + return -EINVAL; > > > + } > > > + > > > + case IIO_CHAN_INFO_SCALE: > > > + /* Chip temperature scale = 1/100 */ > > > > IIO temperatures are measured in milli degrees. 1lsb = 1/100*1000 degrees centigrade seems very accurate > > for a device like this! I'm guessing this should be 10. > > Ah yes, I thought it had to be given in the chip's native format, > which is 1/100 degree. > > I guess I should then multiply by 10 the temperature raw read and > return IIO_VAL_INT here. You could do that, but can cause a mess if buffered support comes along later as it is then not a whole number of bits for putting in the buffer. > > > > > > + *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, > > > +}; > > > + > > > +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); > > > > Why? I'm not immediately spotting where this is used. > > > > Probably a leftover since when I used raw attrs ? > > > > + > > > + sunrise = iio_priv(iio_dev); > > > + sunrise->client = client; > > > + sunrise->dev = &client->dev; > > > > Why carry this around when you can get it from client->dev? > > > > Andy had the same comment. I think it's very cheap and makes the error > printing more compact. But if it bothers both of you I can drop it. I'd drop it, but if you have a particular function with multiple prints then have a local struct device *dev in that function. The compiler will tidy that up so it makes no difference and it will be even neater at call sites. > > Thanks > j > > > > + 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@jmondi.org>"); > > > +MODULE_DESCRIPTION("Senseair Sunrise 006-0-0007 CO2 sensor IIO driver"); > > > +MODULE_LICENSE("GPL v2"); > > > -- > > > 2.32.0 > > > > >
Hi Jonathan, two more clarification requests, sorry to bother :) On Mon, Aug 30, 2021 at 06:11:17PM +0100, Jonathan Cameron wrote: > On Mon, 30 Aug 2021 18:20:51 +0200 > > > > +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); > > > > > > Hmm. Technically there isn't anything stopping another user of the i2c bus sneaking in > > > between the wakeup and the following command. That would make the device going back > > > to sleep a lot more likely. I can't off the top of my head remember if regmap lets > > > you lock the bus. If not, you'll have to use the underlying i2c bus locking functions. > > > https://elixir.bootlin.com/linux/latest/source/drivers/iio/temperature/mlx90614.c#L432 > > > gives an example. > > > > Right, there might be another call stealing the wakeup session! > > > > I should lock the underlying i2c bus, probably not root adapter like > > mlx90614 does but only the segment. > > Ideally only segment as you say as could be some muxes involved. If not that i2c_smbus_xfer() which is called by my wakeup and by the regmap_smbus_* calls tries to lock the segment as well, so we deadlock :) And actually, locking the underlying i2c segment seems even too strict, what we have to guarantee is that no other read/write function call from this driver interrupts a [wakeup-trasactions] sequence. Wouldn't it be better if I handle that with a dedicated mutex ? > > > > > > > > > Perhaps worth a look is the regmap-sccb implementation which has a dance that looks > > > a tiny bit like what you have to do here (be it for a different reason). > > > It might be nice to do something similar here and have a custom regmap bus which > > > has the necessary wakeups in the relevant places. > > > > > > Note I haven't thought it through in depth, so it might not work! > > > > the dance is similar if not regmap-sccb tranfers a byte instead of > > sending only the R/W bit (notice the usage of I2C_SMBUS_QUICK here and > > I2C_SMBUS_BYTE in regmap-sccb). Practically speaking it makes no > > difference as the sensor nacks the first message, so the underlying > > bus implementation bails out, but that's a bit of work-by-accident > > thing, isn't it ? > > > > If fine with you, I would stick to this implementation and hold the > > segment locked between the wakup and the actual messages. > > That's fine. I was just thinking you could hid the magic in a custom regmap then > the rest of the driver would not have to be aware of it. Slightly neater than > wrapping regmap functions with this extra call in the wrapper. > [snip] > > > > + } > > > > + > > > > + case IIO_CHAN_INFO_SCALE: > > > > + /* Chip temperature scale = 1/100 */ > > > > > > IIO temperatures are measured in milli degrees. 1lsb = 1/100*1000 degrees centigrade seems very accurate > > > for a device like this! I'm guessing this should be 10. > > > > Ah yes, I thought it had to be given in the chip's native format, > > which is 1/100 degree. > > > > I guess I should then multiply by 10 the temperature raw read and > > return IIO_VAL_INT here. > > You could do that, but can cause a mess if buffered support comes along later as > it is then not a whole number of bits for putting in the buffer. > Care to clarify ? What shouldn't I do here ? Multiply by 10 the raw value (which I think is wrong as pointed out in a later reply) or return IIO_VAL_INT ? Sorry I didn't get the connection with the number of bits to put in the buffer :) Thanks j
On Tue, 31 Aug 2021 09:40:11 +0200 Jacopo Mondi <jacopo@jmondi.org> wrote: > Hi Jonathan, > two more clarification requests, sorry to bother :) No problem. First one: No idea +CC wolfram and i2c list. > > On Mon, Aug 30, 2021 at 06:11:17PM +0100, Jonathan Cameron wrote: > > On Mon, 30 Aug 2021 18:20:51 +0200 > > > > > +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); > > > > > > > > Hmm. Technically there isn't anything stopping another user of the i2c bus sneaking in > > > > between the wakeup and the following command. That would make the device going back > > > > to sleep a lot more likely. I can't off the top of my head remember if regmap lets > > > > you lock the bus. If not, you'll have to use the underlying i2c bus locking functions. > > > > https://elixir.bootlin.com/linux/latest/source/drivers/iio/temperature/mlx90614.c#L432 > > > > gives an example. > > > > > > Right, there might be another call stealing the wakeup session! > > > > > > I should lock the underlying i2c bus, probably not root adapter like > > > mlx90614 does but only the segment. > > > > Ideally only segment as you say as could be some muxes involved. > > If not that i2c_smbus_xfer() which is called by my wakeup and by the > regmap_smbus_* calls tries to lock the segment as well, so we deadlock :) > > And actually, locking the underlying i2c segment seems even too > strict, what we have to guarantee is that no other read/write function > call from this driver interrupts a [wakeup-trasactions] sequence. > > Wouldn't it be better if I handle that with a dedicated mutex ? I'm not sure what best route is. +CC Wolfram, Peter and linux-i2c. Short story here is we have a device which autonomously goes to sleep. Datasheet suggests waking it up with a failed xfer followed by what we actually want to do (sufficiently quickly). Obviously we can't actually guarantee that will ever happen but it's a lot more likely to succeed if we briefly stop anything else using he i2c bus. How should we handle this? > > > > > > > > > > > > > > Perhaps worth a look is the regmap-sccb implementation which has a dance that looks > > > > a tiny bit like what you have to do here (be it for a different reason). > > > > It might be nice to do something similar here and have a custom regmap bus which > > > > has the necessary wakeups in the relevant places. > > > > > > > > Note I haven't thought it through in depth, so it might not work! > > > > > > the dance is similar if not regmap-sccb tranfers a byte instead of > > > sending only the R/W bit (notice the usage of I2C_SMBUS_QUICK here and > > > I2C_SMBUS_BYTE in regmap-sccb). Practically speaking it makes no > > > difference as the sensor nacks the first message, so the underlying > > > bus implementation bails out, but that's a bit of work-by-accident > > > thing, isn't it ? > > > > > > If fine with you, I would stick to this implementation and hold the > > > segment locked between the wakup and the actual messages. > > > > That's fine. I was just thinking you could hid the magic in a custom regmap then > > the rest of the driver would not have to be aware of it. Slightly neater than > > wrapping regmap functions with this extra call in the wrapper. > > > > [snip] > > > > > > + } > > > > > + > > > > > + case IIO_CHAN_INFO_SCALE: > > > > > + /* Chip temperature scale = 1/100 */ > > > > > > > > IIO temperatures are measured in milli degrees. 1lsb = 1/100*1000 degrees centigrade seems very accurate > > > > for a device like this! I'm guessing this should be 10. > > > > > > Ah yes, I thought it had to be given in the chip's native format, > > > which is 1/100 degree. > > > > > > I guess I should then multiply by 10 the temperature raw read and > > > return IIO_VAL_INT here. > > > > You could do that, but can cause a mess if buffered support comes along later as > > it is then not a whole number of bits for putting in the buffer. > > > > Care to clarify ? What shouldn't I do here ? Multiply by 10 the raw > value (which I think is wrong as pointed out in a later reply) or > return IIO_VAL_INT ? Sorry I didn't get the connection with the number > of bits to put in the buffer :) So. If you stick to output of _raw and _scale in the buffer the data would be whatever you read from the register. That is typically a whole number of bits. If you were to multiply by 10 you end up something that may be say 12 or 13 bits depending on the value. That's a bit ugly. We can handle it of course, but I'd rather not if it's as simple as not doing the *10 in kernel for the sysfs path. So not critical but things end up more elegant / standard if we don't do the *10 and instead make that a problem for userspace. Jonathan > > Thanks > j
On 2021-09-05 12:04, Jonathan Cameron wrote: > On Tue, 31 Aug 2021 09:40:11 +0200 > Jacopo Mondi <jacopo@jmondi.org> wrote: > >> Hi Jonathan, >> two more clarification requests, sorry to bother :) > No problem. First one: No idea +CC wolfram and i2c list. >> >> On Mon, Aug 30, 2021 at 06:11:17PM +0100, Jonathan Cameron wrote: >>> On Mon, 30 Aug 2021 18:20:51 +0200 >>>>>> +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); >>>>> >>>>> Hmm. Technically there isn't anything stopping another user of the i2c bus sneaking in >>>>> between the wakeup and the following command. That would make the device going back >>>>> to sleep a lot more likely. I can't off the top of my head remember if regmap lets >>>>> you lock the bus. If not, you'll have to use the underlying i2c bus locking functions. >>>>> https://elixir.bootlin.com/linux/latest/source/drivers/iio/temperature/mlx90614.c#L432 >>>>> gives an example. >>>> >>>> Right, there might be another call stealing the wakeup session! >>>> >>>> I should lock the underlying i2c bus, probably not root adapter like >>>> mlx90614 does but only the segment. >>> >>> Ideally only segment as you say as could be some muxes involved. >> >> If not that i2c_smbus_xfer() which is called by my wakeup and by the >> regmap_smbus_* calls tries to lock the segment as well, so we deadlock :) >> >> And actually, locking the underlying i2c segment seems even too >> strict, what we have to guarantee is that no other read/write function >> call from this driver interrupts a [wakeup-trasactions] sequence. >> >> Wouldn't it be better if I handle that with a dedicated mutex ? > > I'm not sure what best route is. +CC Wolfram, Peter and linux-i2c. > > Short story here is we have a device which autonomously goes to sleep. > Datasheet suggests waking it up with a failed xfer followed by what we > actually want to do (sufficiently quickly). > > Obviously we can't actually guarantee that will ever happen but it's a lot > more likely to succeed if we briefly stop anything else using he i2c bus. > > How should we handle this? The way I read this is that interactions with other I2C devices that squeeze in are not a fundamental problem. Not unless there are so many of these 3rd party xfers that the device times out again. If my assessment is correct, then I would suggest handling this in the driver by somehow making sure that it doesn't clobber its own pairs of wakeup+work interactions. But see below. Because there really is no way to protect against those extra I2C accesses. With a parent-locked mux you can (ignoring arbitrators, where another system may possibly take over the bus if too much time is spent between two xfers that were supposed to be adjacent). But if there's a mux-locked mux in the path it's simply not possible to lock out all other xfers on the root adapter. With a parent-locked I2C tree, "locking the segment" is equivalent to locking everything all the way to the root adapter. But the whole point of mux-locked muxes is that they can't operate if you do that. Mux-locked muxes are allowed to depend on other (ignorant) drivers using other parts of the I2C tree while the segment is locked. If you lock the root adapter when there is a mux-locked mux involved, you simply kill that property and sign up for a deadlock. Which also means that you cannot prevent 3rd party xfers to other parts of the I2C tree. However, if you worry about 3rd party xfers causing the wakeup to timeout again and that only handling it in the driver as suggested above is insufficient, then it's an option to lock the segment. For parent-locked I2C trees, this should prevent (most) 3rd party actions and minimize the delay. In the odd case that there are mux-locked muxes involved, there will simply be a higher risk of failure, but there is little to do about that. See Documentation/i2c/i2c-topology.rst for some discussion on the details of mux-locked and parent-locked muxes. I hope I make at least some sense... Cheers, Peter
On 2021-09-06 01:03, Peter Rosin wrote: > On 2021-09-05 12:04, Jonathan Cameron wrote: >> On Tue, 31 Aug 2021 09:40:11 +0200 >> Jacopo Mondi <jacopo@jmondi.org> wrote: >> >>> Hi Jonathan, >>> two more clarification requests, sorry to bother :) >> No problem. First one: No idea +CC wolfram and i2c list. >>> >>> On Mon, Aug 30, 2021 at 06:11:17PM +0100, Jonathan Cameron wrote: >>>> On Mon, 30 Aug 2021 18:20:51 +0200 >>>>>>> +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); >>>>>> >>>>>> Hmm. Technically there isn't anything stopping another user of the i2c bus sneaking in >>>>>> between the wakeup and the following command. That would make the device going back >>>>>> to sleep a lot more likely. I can't off the top of my head remember if regmap lets >>>>>> you lock the bus. If not, you'll have to use the underlying i2c bus locking functions. >>>>>> https://elixir.bootlin.com/linux/latest/source/drivers/iio/temperature/mlx90614.c#L432 >>>>>> gives an example. Forgot to mention, regmap does let you do that. See e.g. drivers/media/dvb-frontends/rtl2830.c which wraps regmap_update_bits, regmap_bulk_write and regmap_bulk_read within a local I2C segment lock along with a special regmap_bus that does unlocked I2C trasfers. I think the driver does this because it has an I2C gate that needs to be opened with a regmap interaction that in turn needs to be back to back with the "real" regmap access, or else the gate closes again. Cheers, Peter
Hi Peter, thanks for your detailed answer On Mon, Sep 06, 2021 at 01:03:52AM +0200, Peter Rosin wrote: > On 2021-09-05 12:04, Jonathan Cameron wrote: > > On Tue, 31 Aug 2021 09:40:11 +0200 > > Jacopo Mondi <jacopo@jmondi.org> wrote: > > > >> Hi Jonathan, > >> two more clarification requests, sorry to bother :) > > No problem. First one: No idea +CC wolfram and i2c list. > >> > >> On Mon, Aug 30, 2021 at 06:11:17PM +0100, Jonathan Cameron wrote: > >>> On Mon, 30 Aug 2021 18:20:51 +0200 > >>>>>> +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); > >>>>> > >>>>> Hmm. Technically there isn't anything stopping another user of the i2c bus sneaking in > >>>>> between the wakeup and the following command. That would make the device going back > >>>>> to sleep a lot more likely. I can't off the top of my head remember if regmap lets > >>>>> you lock the bus. If not, you'll have to use the underlying i2c bus locking functions. > >>>>> https://elixir.bootlin.com/linux/latest/source/drivers/iio/temperature/mlx90614.c#L432 > >>>>> gives an example. > >>>> > >>>> Right, there might be another call stealing the wakeup session! > >>>> > >>>> I should lock the underlying i2c bus, probably not root adapter like > >>>> mlx90614 does but only the segment. > >>> > >>> Ideally only segment as you say as could be some muxes involved. > >> > >> If not that i2c_smbus_xfer() which is called by my wakeup and by the > >> regmap_smbus_* calls tries to lock the segment as well, so we deadlock :) > >> > >> And actually, locking the underlying i2c segment seems even too > >> strict, what we have to guarantee is that no other read/write function > >> call from this driver interrupts a [wakeup-trasactions] sequence. > >> > >> Wouldn't it be better if I handle that with a dedicated mutex ? > > > > I'm not sure what best route is. +CC Wolfram, Peter and linux-i2c. > > > > Short story here is we have a device which autonomously goes to sleep. > > Datasheet suggests waking it up with a failed xfer followed by what we > > actually want to do (sufficiently quickly). > > > > Obviously we can't actually guarantee that will ever happen but it's a lot > > more likely to succeed if we briefly stop anything else using he i2c bus. > > > > How should we handle this? > > The way I read this is that interactions with other I2C devices that squeeze > in are not a fundamental problem. Not unless there are so many of these 3rd > party xfers that the device times out again. If my assessment is correct, > then I would suggest handling this in the driver by somehow making sure that > it doesn't clobber its own pairs of wakeup+work interactions. But see below. So, I tested by sending a double wakeup signal, to verify if the chip goes back to sleep after -any- kind of transaction after the first wakeup. It seems it does from what I see. I also tested by inserting a spurious i2c transaction to a non-existing address between the wakeup and the actual transaction, but I cannot say if that proves anything as I'm not sure if messages directed to non-registered addresses are actually put on the bus. Anyway, quoting the chip manual: ------------------------------------------------------------------------------ Senseair Sunrise spend most of its time in deep sleep mode to minimize power consumption, this have the effect that it is necessary to wake up the sensor before it is possible to communicate with it. Sensor will wake up on a falling edge on SDA, it is recommended to send sensors address to wake it up. When sensors address is used to wake up the sensor the sensor will not acknowledge this byte. Communication sequence: 1) Wake up sensor by sending sensor address (START, sensor address, STOP). Sensor will not ACK this byte. 2) Normal I2C read/write operations. I2C communication must be started within 15ms after the wake-up byte, each byte sent to or from the sensor sets the timeout to 15 ms. After a complete read or write sequence sensor will enter sleep mode immediately. ------------------------------------------------------------------------------ I think you're correct assuming the only way 3rd parties could interfere with the wakeup session is by issuing so many transactions that the bus is not available to the device for such a long time (15msec) that the wakeup session expires. Other messages, not directed to the chip, doesn't seem to interfere. So locking in the driver should be good enough I think. > > Because there really is no way to protect against those extra I2C accesses. > With a parent-locked mux you can (ignoring arbitrators, where another > system may possibly take over the bus if too much time is spent between > two xfers that were supposed to be adjacent). But if there's a mux-locked > mux in the path it's simply not possible to lock out all other xfers on > the root adapter. > > With a parent-locked I2C tree, "locking the segment" is equivalent to > locking everything all the way to the root adapter. But the whole point > of mux-locked muxes is that they can't operate if you do that. Mux-locked > muxes are allowed to depend on other (ignorant) drivers using other parts > of the I2C tree while the segment is locked. If you lock the root adapter > when there is a mux-locked mux involved, you simply kill that property > and sign up for a deadlock. Which also means that you cannot prevent 3rd > party xfers to other parts of the I2C tree. > > However, if you worry about 3rd party xfers causing the wakeup to timeout > again and that only handling it in the driver as suggested above is > insufficient, then it's an option to lock the segment. For parent-locked I2C > trees, this should prevent (most) 3rd party actions and minimize the delay. > In the odd case that there are mux-locked muxes involved, there will simply > be a higher risk of failure, but there is little to do about that. It doesn't feel to me this is required, but I let Jonathan and Andy which have discussed this with me in the past express an opinion as well. In case I need to go for this solution am I correct assuming I should lock the bus for the whole wakeup-work session and override the regmap_bus operations to perform unlocked access to the i2c bus? In my mind this could be realized as int wakeup() { /* Unlocked wakup ping */ __i2c_smbus_xfer() } int regmap_bus_read() { i2c_lock_bus(); wakeup(); /* Unlocked i2c read transaction */ __i2c_transfer(); i2c_unlock_bus(); } struct regmap_bus regmap_bus = { .read = regmap_bus_read, ... } int probe() { regmap_init(..., regmap_bus, ..). } But I somehow feel like I could have locking and wakeup handled by a mux's select/deselect and have a simpler read function. However even if I think I could have the driver register a mux even if there's actually no muxed bus behind the chip, I'm missing how that would work if not by exposing this in the DT bindings or by registering an i2c_board_info, but this feels already too complicated to be worth it, right ? Thanks j > > See Documentation/i2c/i2c-topology.rst for some discussion on the details > of mux-locked and parent-locked muxes. > > I hope I make at least some sense... > > Cheers, > Peter
On 2021-09-08 13:00, Jacopo Mondi wrote: > Hi Peter, > thanks for your detailed answer > > On Mon, Sep 06, 2021 at 01:03:52AM +0200, Peter Rosin wrote: >> The way I read this is that interactions with other I2C devices that squeeze >> in are not a fundamental problem. Not unless there are so many of these 3rd >> party xfers that the device times out again. If my assessment is correct, >> then I would suggest handling this in the driver by somehow making sure that >> it doesn't clobber its own pairs of wakeup+work interactions. But see below. > > So, I tested by sending a double wakeup signal, to verify if the chip > goes back to sleep after -any- kind of transaction after the first > wakeup. It seems it does from what I see. > > I also tested by inserting a spurious i2c transaction to a > non-existing address between the wakeup and the actual transaction, > but I cannot say if that proves anything as I'm not sure if messages > directed to non-registered addresses are actually put on the bus. > > Anyway, quoting the chip manual: > > ------------------------------------------------------------------------------ > Senseair Sunrise spend most of its time in deep sleep mode to minimize > power consumption, this have the effect that it is necessary to wake > up the sensor before it is possible to communicate with it. Sensor > will wake up on a falling edge on SDA, it is recommended to send > sensors address to wake it up. When sensors address is used to wake up > the sensor the sensor will not acknowledge this byte. > > Communication sequence: > 1) Wake up sensor by sending sensor address (START, sensor address, STOP). > Sensor will not ACK this byte. > > 2) Normal I2C read/write operations. I2C communication must be started > within 15ms after the wake-up byte, each byte sent to or from the > sensor sets the timeout to 15 ms. After a complete read or write > sequence sensor will enter sleep mode immediately. > ------------------------------------------------------------------------------ > > I think you're correct assuming the only way 3rd parties could > interfere with the wakeup session is by issuing so many transactions > that the bus is not available to the device for such a long time > (15msec) that the wakeup session expires. > > Other messages, not directed to the chip, doesn't seem to interfere. > > So locking in the driver should be good enough I think. I think so too. Even if 15ms is kind of short... I mean, locking the I2C segment can certainly exclude (some) 3rd party xfers on the bus and that may help, but there is so much else that could potentially cause a 15ms stall, especially on smallish single-cpu devices. >> >> Because there really is no way to protect against those extra I2C accesses. >> With a parent-locked mux you can (ignoring arbitrators, where another >> system may possibly take over the bus if too much time is spent between >> two xfers that were supposed to be adjacent). But if there's a mux-locked >> mux in the path it's simply not possible to lock out all other xfers on >> the root adapter. >> >> With a parent-locked I2C tree, "locking the segment" is equivalent to >> locking everything all the way to the root adapter. But the whole point >> of mux-locked muxes is that they can't operate if you do that. Mux-locked >> muxes are allowed to depend on other (ignorant) drivers using other parts >> of the I2C tree while the segment is locked. If you lock the root adapter >> when there is a mux-locked mux involved, you simply kill that property >> and sign up for a deadlock. Which also means that you cannot prevent 3rd >> party xfers to other parts of the I2C tree. >> >> However, if you worry about 3rd party xfers causing the wakeup to timeout >> again and that only handling it in the driver as suggested above is >> insufficient, then it's an option to lock the segment. For parent-locked I2C >> trees, this should prevent (most) 3rd party actions and minimize the delay. >> In the odd case that there are mux-locked muxes involved, there will simply >> be a higher risk of failure, but there is little to do about that. > > It doesn't feel to me this is required, but I let Jonathan and Andy > which have discussed this with me in the past express an opinion as > well. > > In case I need to go for this solution am I correct assuming I should > lock the bus for the whole wakeup-work session and override the > regmap_bus operations to perform unlocked access to the i2c bus? > > In my mind this could be realized as > > int wakeup() > { > > /* Unlocked wakup ping */ > __i2c_smbus_xfer() > } > > int regmap_bus_read() > { > i2c_lock_bus(); > > wakeup(); > /* Unlocked i2c read transaction */ > __i2c_transfer(); > > i2c_unlock_bus(); > } > > > struct regmap_bus regmap_bus = { > .read = regmap_bus_read, > ... > } > > int probe() > { > > regmap_init(..., regmap_bus, ..). > } Yep, that looks like the right direction from here as well, should you take that path. > But I somehow feel like I could have locking and wakeup handled by a mux's > select/deselect and have a simpler read function. However even if I > think I could have the driver register a mux even if there's actually > no muxed bus behind the chip, I'm missing how that would work if not > by exposing this in the DT bindings or by registering an > i2c_board_info, but this feels already too complicated to be worth it, > right ? This basically is exactly what is otherwise called an I2C gate. You have to do <something> to get I2C xfers safely past the gate, in this case wake the device up. So, that model isn't wrong. However, since the device wakes up on *any* action on SDA, would it not also work to make special I2C xfers, with a restart instead of a full stop-start after the "wakeup call". That is, if you can assume that the I2C adapter is flexible enough... I.e. something like this: static int sunrise_foo(struct sunrise_context *ctx) { unsigned char reg = 0x17; unsigned char buf[17]; struct i2c_msg msg[3] = { { /* wakeup */ .addr = 0x68, .flags = I2C_M_RD | I2C_M_IGNORE_NAK, .len = 0, }, { /* write register number */ .addr = 0x68, .flags = 0, .len = 1, .buf = ®, }, { /* read register contents */ .addr = 0x68, .flags = I2C_M_RD, .len = 17, .buf = buf, }, }; int ret; ret = i2c_transfer(ctx->adapter, msg, 3); ... return ret; } Cheers, Peter > Thanks > j > >> >> See Documentation/i2c/i2c-topology.rst for some discussion on the details >> of mux-locked and parent-locked muxes. >> >> I hope I make at least some sense... >> >> Cheers, >> Peter
On Wed, Sep 8, 2021 at 6:29 PM Peter Rosin <peda@axentia.se> wrote: > On 2021-09-08 13:00, Jacopo Mondi wrote: > > On Mon, Sep 06, 2021 at 01:03:52AM +0200, Peter Rosin wrote: ... > struct i2c_msg msg[3] = { > { /* wakeup */ > .addr = 0x68, > .flags = I2C_M_RD | I2C_M_IGNORE_NAK, > .len = 0, > }, { /* write register number */ I'm wondering if device will have enough time in between to actualle be woken up. I believe the waking up latency must be considered as well as known 15ms suspending one. > .addr = 0x68, > .flags = 0, > .len = 1, > .buf = ®, > }, { /* read register contents */ > .addr = 0x68, > .flags = I2C_M_RD, > .len = 17, > .buf = buf, > }, > };
Hi Peter, On Wed, Sep 08, 2021 at 05:29:02PM +0200, Peter Rosin wrote: > On 2021-09-08 13:00, Jacopo Mondi wrote: > > Hi Peter, > > thanks for your detailed answer > > > > On Mon, Sep 06, 2021 at 01:03:52AM +0200, Peter Rosin wrote: > >> The way I read this is that interactions with other I2C devices that squeeze > >> in are not a fundamental problem. Not unless there are so many of these 3rd > >> party xfers that the device times out again. If my assessment is correct, > >> then I would suggest handling this in the driver by somehow making sure that > >> it doesn't clobber its own pairs of wakeup+work interactions. But see below. > > > > So, I tested by sending a double wakeup signal, to verify if the chip > > goes back to sleep after -any- kind of transaction after the first > > wakeup. It seems it does from what I see. > > > > I also tested by inserting a spurious i2c transaction to a > > non-existing address between the wakeup and the actual transaction, > > but I cannot say if that proves anything as I'm not sure if messages > > directed to non-registered addresses are actually put on the bus. > > > > Anyway, quoting the chip manual: > > > > ------------------------------------------------------------------------------ > > Senseair Sunrise spend most of its time in deep sleep mode to minimize > > power consumption, this have the effect that it is necessary to wake > > up the sensor before it is possible to communicate with it. Sensor > > will wake up on a falling edge on SDA, it is recommended to send > > sensors address to wake it up. When sensors address is used to wake up > > the sensor the sensor will not acknowledge this byte. > > > > Communication sequence: > > 1) Wake up sensor by sending sensor address (START, sensor address, STOP). > > Sensor will not ACK this byte. > > > > 2) Normal I2C read/write operations. I2C communication must be started > > within 15ms after the wake-up byte, each byte sent to or from the > > sensor sets the timeout to 15 ms. After a complete read or write > > sequence sensor will enter sleep mode immediately. > > ------------------------------------------------------------------------------ > > > > I think you're correct assuming the only way 3rd parties could > > interfere with the wakeup session is by issuing so many transactions > > that the bus is not available to the device for such a long time > > (15msec) that the wakeup session expires. > > > > Other messages, not directed to the chip, doesn't seem to interfere. > > > > So locking in the driver should be good enough I think. > > I think so too. Even if 15ms is kind of short... I mean, locking the > I2C segment can certainly exclude (some) 3rd party xfers on the bus and > that may help, but there is so much else that could potentially cause > a 15ms stall, especially on smallish single-cpu devices. > > >> > >> Because there really is no way to protect against those extra I2C accesses. > >> With a parent-locked mux you can (ignoring arbitrators, where another > >> system may possibly take over the bus if too much time is spent between > >> two xfers that were supposed to be adjacent). But if there's a mux-locked > >> mux in the path it's simply not possible to lock out all other xfers on > >> the root adapter. > >> > >> With a parent-locked I2C tree, "locking the segment" is equivalent to > >> locking everything all the way to the root adapter. But the whole point > >> of mux-locked muxes is that they can't operate if you do that. Mux-locked > >> muxes are allowed to depend on other (ignorant) drivers using other parts > >> of the I2C tree while the segment is locked. If you lock the root adapter > >> when there is a mux-locked mux involved, you simply kill that property > >> and sign up for a deadlock. Which also means that you cannot prevent 3rd > >> party xfers to other parts of the I2C tree. > >> > >> However, if you worry about 3rd party xfers causing the wakeup to timeout > >> again and that only handling it in the driver as suggested above is > >> insufficient, then it's an option to lock the segment. For parent-locked I2C > >> trees, this should prevent (most) 3rd party actions and minimize the delay. > >> In the odd case that there are mux-locked muxes involved, there will simply > >> be a higher risk of failure, but there is little to do about that. > > > > It doesn't feel to me this is required, but I let Jonathan and Andy > > which have discussed this with me in the past express an opinion as > > well. > > > > In case I need to go for this solution am I correct assuming I should > > lock the bus for the whole wakeup-work session and override the > > regmap_bus operations to perform unlocked access to the i2c bus? > > > > In my mind this could be realized as > > > > int wakeup() > > { > > > > /* Unlocked wakup ping */ > > __i2c_smbus_xfer() > > } > > > > int regmap_bus_read() > > { > > i2c_lock_bus(); > > > > wakeup(); > > /* Unlocked i2c read transaction */ > > __i2c_transfer(); > > > > i2c_unlock_bus(); > > } > > > > > > struct regmap_bus regmap_bus = { > > .read = regmap_bus_read, > > ... > > } > > > > int probe() > > { > > > > regmap_init(..., regmap_bus, ..). > > } > > Yep, that looks like the right direction from here as well, should you take > that path. In facts, I have just implemented this version using unlocked __i2c_smbus_ functions in the regmap read/write overloads: 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); } Those will be wrapped by a segment lock in the driver's read/write functions. > > > But I somehow feel like I could have locking and wakeup handled by a mux's > > select/deselect and have a simpler read function. However even if I > > think I could have the driver register a mux even if there's actually > > no muxed bus behind the chip, I'm missing how that would work if not > > by exposing this in the DT bindings or by registering an > > i2c_board_info, but this feels already too complicated to be worth it, > > right ? > > This basically is exactly what is otherwise called an I2C gate. You have > to do <something> to get I2C xfers safely past the gate, in this case > wake the device up. So, that model isn't wrong. > > However, since the device wakes up on *any* action on SDA, would it not > also work to make special I2C xfers, with a restart instead of a full > stop-start after the "wakeup call". That is, if you can assume that the > I2C adapter is flexible enough... > > I.e. something like this: > > static int > sunrise_foo(struct sunrise_context *ctx) > { > unsigned char reg = 0x17; > unsigned char buf[17]; > struct i2c_msg msg[3] = { > { /* wakeup */ > .addr = 0x68, > .flags = I2C_M_RD | I2C_M_IGNORE_NAK, > .len = 0, > }, { /* write register number */ > .addr = 0x68, > .flags = 0, > .len = 1, > .buf = ®, > }, { /* read register contents */ > .addr = 0x68, > .flags = I2C_M_RD, > .len = 17, > .buf = buf, > }, > }; > int ret; > > ret = i2c_transfer(ctx->adapter, msg, 3); But this is interesting as well, as it seems the chip is flexible enough to support repeated starts so this might works too. Assuming this version works, which one is preferred in terms of compatibility with the largest possible number of adapters (if it makes any difference at all) ? Documentation/i2c/smbus-protocol.rst seems to suggest smbus command should be preferred: If you write a driver for some I2C device, please try to use the SMBus commands if at all possible (if the device uses only that subset of the I2C protocol). This makes it possible to use the device driver on both SMBus adapters and I2C adapters (the SMBus command set is automatically translated to I2C on I2C adapters, but plain I2C commands can not be handled at all on most pure SMBus adapters). Thanks j > > ... > > return ret; > } > > Cheers, > Peter > > > Thanks > > j > > > >> > >> See Documentation/i2c/i2c-topology.rst for some discussion on the details > >> of mux-locked and parent-locked muxes. > >> > >> I hope I make at least some sense... > >> > >> Cheers, > >> Peter
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@jmondi.org> +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@octakon.com> 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 + 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" + +#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; + 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, +}; + +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@jmondi.org>"); +MODULE_DESCRIPTION("Senseair Sunrise 006-0-0007 CO2 sensor IIO driver"); +MODULE_LICENSE("GPL v2");
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> --- 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 -- 2.32.0