On Mon, 30 Aug 2021 18:20:51 +0200 Jacopo Mondi <jacopo@xxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxx> > > > --- > > > 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@xxxxxxxxxx> > > > +S: Maintained > > > +F: Documentation/devicetree/bindings/iio/chemical/senseair,sunrise.yaml > > > +F: drivers/iio/chemical/sunrise_co2.c > > > + > > > SENSIRION SCD30 CARBON DIOXIDE SENSOR DRIVER > > > M: Tomasz Duszynski <tomasz.duszynski@xxxxxxxxxxx> > > > S: Maintained > > > diff --git a/drivers/iio/chemical/Kconfig b/drivers/iio/chemical/Kconfig > > > index 10bb431bc3ce..ee8562949226 100644 > > > --- a/drivers/iio/chemical/Kconfig > > > +++ b/drivers/iio/chemical/Kconfig > > > @@ -144,6 +144,19 @@ config SPS30 > > > To compile this driver as a module, choose M here: the module will > > > be called sps30. > > > > > > +config SENSEAIR_SUNRISE_CO2 > > > + tristate "Senseair Sunrise 006-0-0007 CO2 sensor" > > > + depends on OF > > > > 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@xxxxxxxxxx>"); > > > +MODULE_DESCRIPTION("Senseair Sunrise 006-0-0007 CO2 sensor IIO driver"); > > > +MODULE_LICENSE("GPL v2"); > > > -- > > > 2.32.0 > > > > >
