Hi! Subject looks wrong, shouldn't it be 006-0-0007? On 2021-09-09 11:45, Jacopo Mondi wrote: > Add support for the Senseair Sunrise 006-0-0007 driver through the > IIO subsystem. > > Datasheet: https://rmtplusstoragesenseair.blob.core.windows.net/docs/Dev/publicerat/TDE5531.pdf > Signed-off-by: Jacopo Mondi <jacopo@xxxxxxxxxx> > --- > MAINTAINERS | 7 + > drivers/iio/chemical/Kconfig | 10 + > drivers/iio/chemical/Makefile | 1 + > drivers/iio/chemical/sunrise_co2.c | 526 +++++++++++++++++++++++++++++ > 4 files changed, 544 insertions(+) > create mode 100644 drivers/iio/chemical/sunrise_co2.c > *snip* > + * the i2c bus. > + */ > +static int sunrise_read_byte(struct sunrise_dev *sunrise, u8 reg) > +{ > + const struct i2c_client *client = sunrise->client; > + const struct device *dev = &client->dev; > + unsigned int val; > + int ret; > + > + i2c_lock_bus(client->adapter, I2C_LOCK_SEGMENT); > + ret = regmap_read(sunrise->regmap, reg, &val); > + i2c_unlock_bus(client->adapter, I2C_LOCK_SEGMENT); > + if (ret) { > + dev_err(dev, "Read byte failed: reg 0x%2x (%d)\n", reg, ret); Nitpick, %02x looks better methinks ("reg 0x04" vs. "reg 0x 4"). [more instances below] > + return ret; > + } > + > + return val; > +} > + > +static int sunrise_read_word(struct sunrise_dev *sunrise, u8 reg, u16 *val) > +{ > + const struct i2c_client *client = sunrise->client; > + const struct device *dev = &client->dev; > + __be16 be_val; > + int ret; > + > + i2c_lock_bus(client->adapter, I2C_LOCK_SEGMENT); > + ret = regmap_bulk_read(sunrise->regmap, reg, &be_val, sizeof(be_val)); > + i2c_unlock_bus(client->adapter, I2C_LOCK_SEGMENT); > + if (ret) { > + dev_err(dev, "Read word failed: reg 0x%2x (%d)\n", reg, ret); > + return ret; > + } > + > + *val = be16_to_cpu(be_val); > + > + return 0; > +} > + > +static int sunrise_write_byte(struct sunrise_dev *sunrise, u8 reg, u8 val) > +{ > + const struct i2c_client *client = sunrise->client; > + const struct device *dev = &client->dev; > + int ret; > + > + i2c_lock_bus(client->adapter, I2C_LOCK_SEGMENT); > + ret = regmap_write(sunrise->regmap, reg, val); > + i2c_unlock_bus(client->adapter, I2C_LOCK_SEGMENT); > + if (ret) > + dev_err(dev, "Write byte failed: reg 0x%2x (%d)\n", reg, ret); > + > + return ret; > +} > + > +static int sunrise_write_word(struct sunrise_dev *sunrise, u8 reg, u16 data) > +{ > + const struct i2c_client *client = sunrise->client; > + const struct device *dev = &client->dev; > + __be16 be_data = cpu_to_be16(data); > + int ret; > + > + i2c_lock_bus(client->adapter, I2C_LOCK_SEGMENT); > + ret = regmap_bulk_write(sunrise->regmap, reg, &be_data, sizeof(be_data)); > + i2c_unlock_bus(client->adapter, I2C_LOCK_SEGMENT); > + if (ret) > + dev_err(dev, "Write word failed: reg 0x%2x (%d)\n", reg, ret); > + > + return ret; > +} *snip* > + > +static const struct regmap_bus sunrise_regmap_bus = { > + .read = sunrise_regmap_read, > + .write = sunrise_regmap_write, > +}; > + > +static const struct regmap_config sunrise_regmap_config = { > + .reg_bits = 8, > + .val_bits = 8, > +}; > + > +static int sunrise_probe(struct i2c_client *client) > +{ > + struct sunrise_dev *sunrise; > + struct iio_dev *iio_dev; > + > + iio_dev = devm_iio_device_alloc(&client->dev, sizeof(*sunrise)); > + if (!iio_dev) > + return -ENOMEM; > + > + sunrise = iio_priv(iio_dev); > + sunrise->client = client; You should check if the I2C adapter supports the needed operations. if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_BLOCK_DATA | I2C_FUNC_SMBUS_BYTE_DATA | I2C_FUNC_SMBUS_QUICK | I2C_FUNC_PROTOCOL_MANGLING)) ... And, I suspect that protocol mangling might not be the first thing simple SMBus-only adapters support?? Maybe you don't lose all that many adapters by not using the restart-after-wakeup method, when odd things are required anyway? That is, if the device gets enough time to wake up properly using that method? (see the message from Andy in the v3 discussion) SMBus quick isn't universal either... But hmmm, maybe you don't *really* need protocol mangling when you don't check the return value of the wakeup call? At the same time, without I2C_M_IGNORE_NAK, you'll perhaps end up with boring entries in the log or the adapter doing retries or some other inconvenient crap? You could perhaps set I2C_M_IGNORE_NAK only if the adapter supports protocol mangling? Any maybe you don't care about losers who don't have a nice enough adapter. :-) Cheers, Peter > + mutex_init(&sunrise->lock); > + > + sunrise->regmap = devm_regmap_init(&client->dev, &sunrise_regmap_bus, > + client, &sunrise_regmap_config); > + if (IS_ERR(sunrise->regmap)) { > + dev_err(&client->dev, "Failed to initialize regmap\n"); > + return PTR_ERR(sunrise->regmap); > + } > + > + iio_dev->info = &sunrise_info; > + iio_dev->name = DRIVER_NAME; > + iio_dev->channels = sunrise_channels; > + iio_dev->num_channels = ARRAY_SIZE(sunrise_channels); > + iio_dev->modes = INDIO_DIRECT_MODE; > + > + return devm_iio_device_register(&client->dev, iio_dev); > +} > + > +static const struct of_device_id sunrise_of_match[] = { > + { .compatible = "senseair,sunrise-006-0-0007" }, > + {} > +}; > +MODULE_DEVICE_TABLE(of, sunrise_of_match); > + > +static struct i2c_driver sunrise_driver = { > + .driver = { > + .name = DRIVER_NAME, > + .of_match_table = sunrise_of_match, > + }, > + .probe_new = sunrise_probe, > +}; > +module_i2c_driver(sunrise_driver); > + > +MODULE_AUTHOR("Jacopo Mondi <jacopo@xxxxxxxxxx>"); > +MODULE_DESCRIPTION("Senseair Sunrise 006-0-0007 CO2 sensor IIO driver"); > +MODULE_LICENSE("GPL v2"); > -- > 2.32.0 >
