| Message ID | 20240710-ens21x-v3-2-4e3fbcf2a7fb@thegoodpenguin.co.uk (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: humidity: Add support for en21x sensor family | expand |
Hi Joshua, kernel test robot noticed the following build warnings: [auto build test WARNING on 1ebab783647a9e3bf357002d5c4ff060c8474a0a] url: https://github.com/intel-lab-lkp/linux/commits/Joshua-Felmeden/dt-bindings-iio-humidity-add-ENS21x-sensor-family/20240711-022826 base: 1ebab783647a9e3bf357002d5c4ff060c8474a0a patch link: https://lore.kernel.org/r/20240710-ens21x-v3-2-4e3fbcf2a7fb%40thegoodpenguin.co.uk patch subject: [PATCH v3 2/2] iio: humidity: Add support for ENS21x config: loongarch-randconfig-r122-20240712 (https://download.01.org/0day-ci/archive/20240712/202407121921.LHla7p7i-lkp@intel.com/config) compiler: loongarch64-linux-gcc (GCC) 14.1.0 reproduce: (https://download.01.org/0day-ci/archive/20240712/202407121921.LHla7p7i-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202407121921.LHla7p7i-lkp@intel.com/ sparse warnings: (new ones prefixed by >>) >> drivers/iio/humidity/ens21x.c:84:23: sparse: sparse: restricted __be32 degrades to integer >> drivers/iio/humidity/ens21x.c:143:26: sparse: sparse: cast to restricted __le32 >> drivers/iio/humidity/ens21x.c:283:19: sparse: sparse: cast to restricted __le16 vim +84 drivers/iio/humidity/ens21x.c 80 81 /* calculate 17-bit crc7 */ 82 static u8 ens21x_crc7(u32 val) 83 { > 84 u32 val_be = (htonl(val & 0x1ffff) >> 0x8); 85 86 return crc7_be(0xde, (u8 *)&val_be, 3) >> 1; 87 } 88 89 static int ens21x_get_measurement(struct iio_dev *indio_dev, bool temp, int *val) 90 { 91 u32 regval, regval_le; 92 int ret, tries; 93 struct ens21x_dev *dev_data = iio_priv(indio_dev); 94 95 /* assert read */ 96 i2c_smbus_write_byte_data(dev_data->client, ENS21X_REG_SENS_START, 97 temp ? ENS21X_SENS_START_T_START : 98 ENS21X_SENS_START_H_START); 99 100 /* wait for conversion to be ready */ 101 switch (dev_data->part_id) { 102 case ENS210: 103 case ENS210A: 104 msleep(ENS210_CONST_CONVERSION_TIME); 105 break; 106 case ENS211: 107 case ENS212: 108 msleep(ENS212_CONST_CONVERSION_TIME); 109 break; 110 case ENS213A: 111 case ENS215: 112 msleep(ENS215_CONST_CONVERSION_TIME); 113 break; 114 default: 115 dev_err(&dev_data->client->dev, "unrecognised device"); 116 return -ENODEV; 117 } 118 119 tries = 10; 120 while (tries-- > 0) { 121 usleep_range(4000, 5000); 122 ret = i2c_smbus_read_byte_data(dev_data->client, 123 ENS21X_REG_SENS_STAT); 124 if (ret < 0) 125 continue; 126 if (!(ret & (temp ? ENS21X_SENS_STAT_T_ACTIVE : 127 ENS21X_SENS_STAT_H_ACTIVE))) 128 break; 129 } 130 if (tries < 0) { 131 dev_err(&indio_dev->dev, "timeout waiting for sensor reading\n"); 132 return -EIO; 133 } 134 135 /* perform read */ 136 ret = i2c_smbus_read_i2c_block_data( 137 dev_data->client, temp ? ENS21X_REG_T_VAL : ENS21X_REG_H_VAL, 3, 138 (u8 *)®val_le); 139 if (ret < 0) { 140 dev_err(&dev_data->client->dev, "failed to read register"); 141 return -EIO; 142 } else if (ret == 3) { > 143 regval = le32_to_cpu(regval_le); 144 if (ens21x_crc7(regval) == ((regval >> 17) & 0x7f)) { 145 *val = regval & 0xffff; 146 return IIO_VAL_INT; 147 } 148 /* crc fail */ 149 dev_err(&indio_dev->dev, "ens invalid crc\n"); 150 return -EIO; 151 } 152 153 dev_err(&indio_dev->dev, "expected 3 bytes, received %d\n", ret); 154 return -EIO; 155 } 156 157 static int ens21x_read_raw(struct iio_dev *indio_dev, 158 struct iio_chan_spec const *channel, int *val, 159 int *val2, long mask) 160 { 161 struct ens21x_dev *dev_data = iio_priv(indio_dev); 162 int ret = -EINVAL; 163 164 switch (mask) { 165 case IIO_CHAN_INFO_RAW: 166 mutex_lock(&dev_data->lock); 167 ret = ens21x_get_measurement( 168 indio_dev, channel->type == IIO_TEMP, val); 169 mutex_unlock(&dev_data->lock); 170 break; 171 case IIO_CHAN_INFO_SCALE: 172 if (channel->type == IIO_TEMP) { 173 *val = ENS21X_CONST_TEMP_SCALE_INT; 174 *val2 = ENS21X_CONST_TEMP_SCALE_DEC; 175 } else { 176 *val = ENS21X_CONST_HUM_SCALE_INT; 177 *val2 = ENS21X_CONST_HUM_SCALE_DEC; 178 } 179 ret = IIO_VAL_INT_PLUS_MICRO; 180 break; 181 case IIO_CHAN_INFO_OFFSET: 182 if (channel->type == IIO_TEMP) { 183 *val = ENS21X_CONST_TEMP_OFFSET_INT; 184 *val2 = ENS21X_CONST_TEMP_OFFSET_DEC; 185 ret = IIO_VAL_INT_PLUS_MICRO; 186 break; 187 } 188 *val = 0; 189 ret = IIO_VAL_INT; 190 break; 191 default: 192 break; 193 } 194 return ret; 195 } 196 197 static const struct iio_chan_spec ens21x_channels[] = { 198 /* Temperature channel */ 199 { 200 .type = IIO_TEMP, 201 .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | 202 BIT(IIO_CHAN_INFO_SCALE) | 203 BIT(IIO_CHAN_INFO_OFFSET), 204 }, 205 /* Humidity channel */ 206 { 207 .type = IIO_HUMIDITYRELATIVE, 208 .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | 209 BIT(IIO_CHAN_INFO_SCALE) | 210 BIT(IIO_CHAN_INFO_OFFSET), 211 } 212 }; 213 214 static const struct iio_info ens21x_info = { 215 .read_raw = ens21x_read_raw, 216 }; 217 218 static int ens21x_probe(struct i2c_client *client) 219 { 220 const struct i2c_device_id *id = i2c_client_get_device_id(client); 221 const struct of_device_id *match; 222 struct ens21x_dev *dev_data; 223 struct iio_dev *indio_dev; 224 uint16_t part_id_le, part_id; 225 int ret, tries; 226 227 if (!i2c_check_functionality(client->adapter, 228 I2C_FUNC_SMBUS_WRITE_BYTE_DATA | 229 I2C_FUNC_SMBUS_WRITE_BYTE | 230 I2C_FUNC_SMBUS_READ_I2C_BLOCK)) { 231 dev_err(&client->dev, 232 "adapter does not support some i2c transactions\n"); 233 return -EOPNOTSUPP; 234 } 235 236 match = i2c_of_match_device(ens21x_of_match, client); 237 if (!match) 238 return -ENODEV; 239 240 indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*dev_data)); 241 if (!indio_dev) 242 return -ENOMEM; 243 244 dev_data = iio_priv(indio_dev); 245 i2c_set_clientdata(client, indio_dev); 246 dev_data->client = client; 247 mutex_init(&dev_data->lock); 248 249 /* reset device */ 250 ret = i2c_smbus_write_byte_data(client, ENS21X_REG_SYS_CTRL, 251 ENS21X_SYS_CTRL_SYS_RESET); 252 if (ret) 253 return ret; 254 255 /* wait for device to become active */ 256 usleep_range(4000, 5000); 257 258 /* disable low power mode */ 259 ret = i2c_smbus_write_byte_data(client, ENS21X_REG_SYS_CTRL, 0x00); 260 if (ret) 261 return ret; 262 263 /* wait for device to become active */ 264 tries = 10; 265 while (tries-- > 0) { 266 msleep(20); 267 ret = i2c_smbus_read_byte_data(client, ENS21X_REG_SYS_STAT); 268 if (ret < 0) 269 return ret; 270 if (ret & ENS21X_SYS_STAT_SYS_ACTIVE) 271 break; 272 } 273 if (tries < 0) { 274 dev_err(&client->dev, 275 "timeout waiting for ens21x to become active\n"); 276 return -EIO; 277 } 278 279 /* get part_id */ 280 part_id_le = i2c_smbus_read_word_data(client, ENS21X_REG_PART_ID); 281 if (part_id_le < 0) 282 return part_id_le; > 283 part_id = le16_to_cpu(part_id_le); 284 285 if (part_id != id->driver_data) { 286 dev_err(&client->dev, 287 "Part ID does not match (0x%04x != 0x%04lx)\n", part_id, 288 id->driver_data); 289 return -ENODEV; 290 } 291 292 /* reenable low power */ 293 ret = i2c_smbus_write_byte_data(client, ENS21X_REG_SYS_CTRL, 294 ENS21X_SYS_CTRL_LOW_POWER_ENABLE); 295 if (ret) 296 return ret; 297 298 dev_data->part_id = part_id; 299 300 indio_dev->name = id->name; 301 indio_dev->modes = INDIO_DIRECT_MODE; 302 indio_dev->channels = ens21x_channels; 303 indio_dev->num_channels = ARRAY_SIZE(ens21x_channels); 304 indio_dev->info = &ens21x_info; 305 306 return devm_iio_device_register(&client->dev, indio_dev); 307 } 308
Hi Joshua,
kernel test robot noticed the following build errors:
[auto build test ERROR on 1ebab783647a9e3bf357002d5c4ff060c8474a0a]
url: https://github.com/intel-lab-lkp/linux/commits/Joshua-Felmeden/dt-bindings-iio-humidity-add-ENS21x-sensor-family/20240711-022826
base: 1ebab783647a9e3bf357002d5c4ff060c8474a0a
patch link: https://lore.kernel.org/r/20240710-ens21x-v3-2-4e3fbcf2a7fb%40thegoodpenguin.co.uk
patch subject: [PATCH v3 2/2] iio: humidity: Add support for ENS21x
config: sparc64-randconfig-r053-20240712 (https://download.01.org/0day-ci/archive/20240713/202407131554.tOb1HnRA-lkp@intel.com/config)
compiler: sparc64-linux-gcc (GCC) 14.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240713/202407131554.tOb1HnRA-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202407131554.tOb1HnRA-lkp@intel.com/
All errors (new ones prefixed by >>):
sparc64-linux-ld: drivers/iio/humidity/ens21x.o: in function `ens21x_get_measurement':
>> ens21x.c:(.text+0x388): undefined reference to `crc7_be'
Le 10/07/2024 à 15:24, Joshua Felmeden a écrit : > Add support for ENS210/ENS210A/ENS211/ENS212/ENS213A/ENS215. > > The ENS21x is a family of temperature and relative humidity sensors with > accuracies tailored to the needs of specific applications. > > Signed-off-by: Joshua Felmeden <jfelmeden-tUaQ5FxYRYX4aQPF92CzsNBc4/FLrbF6@public.gmane.org> > --- > drivers/iio/humidity/Kconfig | 11 ++ > drivers/iio/humidity/Makefile | 1 + > drivers/iio/humidity/ens21x.c | 346 ++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 358 insertions(+) Hi, as kernel test robot complained, there will be a v4. So here are a few nitpicks/questions, in case it helps. ... > +#include <linux/types.h> > +#include <linux/i2c.h> > +#include <linux/delay.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/of_device.h> > +#include <linux/iio/iio.h> > +#include <linux/iio/sysfs.h> > +#include <linux/crc7.h> Nitpick: usually, it is prefered to keep #include alphabetically ordered. ... > + > +/* magic constants */ > +#define ENS21X_CONST_TEMP_SCALE_INT 15 /* integer part of temperature scale (1/64) */ > +#define ENS21X_CONST_TEMP_SCALE_DEC 625000 /* decimal part of temperature scale */ > +#define ENS21X_CONST_HUM_SCALE_INT 1 /* integer part of humidity scale (1/512) */ > +#define ENS21X_CONST_HUM_SCALE_DEC 953125 /* decimal part of humidity scale */ > +#define ENS21X_CONST_TEMP_OFFSET_INT -17481 /* temperature offset (64 * -273.15) */ > +#define ENS21X_CONST_TEMP_OFFSET_DEC 600000 /* decimal part of offset */ > +#define ENS210_CONST_CONVERSION_TIME 130 > +#define ENS212_CONST_CONVERSION_TIME 32 > +#define ENS215_CONST_CONVERSION_TIME 132 Datasheet says 130 for ENS213A and ENS215. Is it a typo? If 132 is intentional, maybe a samll comment explaining why would be welcomed? ... > +static int ens21x_get_measurement(struct iio_dev *indio_dev, bool temp, int *val) > +{ > + u32 regval, regval_le; > + int ret, tries; > + struct ens21x_dev *dev_data = iio_priv(indio_dev); > + > + /* assert read */ > + i2c_smbus_write_byte_data(dev_data->client, ENS21X_REG_SENS_START, > + temp ? ENS21X_SENS_START_T_START : > + ENS21X_SENS_START_H_START); > + > + /* wait for conversion to be ready */ > + switch (dev_data->part_id) { > + case ENS210: > + case ENS210A: > + msleep(ENS210_CONST_CONVERSION_TIME); > + break; > + case ENS211: > + case ENS212: > + msleep(ENS212_CONST_CONVERSION_TIME); > + break; > + case ENS213A: > + case ENS215: > + msleep(ENS215_CONST_CONVERSION_TIME); > + break; > + default: > + dev_err(&dev_data->client->dev, "unrecognised device"); > + return -ENODEV; > + } > + > + tries = 10; > + while (tries-- > 0) { > + usleep_range(4000, 5000); We just msleep()'ed the max expected time for the conversion. So, maybe the code could be re-arranged so that this delay is done only if we retry? > + ret = i2c_smbus_read_byte_data(dev_data->client, > + ENS21X_REG_SENS_STAT); > + if (ret < 0) > + continue; > + if (!(ret & (temp ? ENS21X_SENS_STAT_T_ACTIVE : > + ENS21X_SENS_STAT_H_ACTIVE))) > + break; > + } > + if (tries < 0) { > + dev_err(&indio_dev->dev, "timeout waiting for sensor reading\n"); > + return -EIO; > + } ... > + indio_dev->name = id->name; > + indio_dev->modes = INDIO_DIRECT_MODE; > + indio_dev->channels = ens21x_channels; > + indio_dev->num_channels = ARRAY_SIZE(ens21x_channels); > + indio_dev->info = &ens21x_info; > + > + return devm_iio_device_register(&client->dev, indio_dev); > +} > + > + Nitpick: unneeded 2nd new line. > +static const struct of_device_id ens21x_of_match[] = { > + { .compatible = "sciosense,ens210", .data = (void *)ENS210}, > + { .compatible = "sciosense,ens210a", .data = (void *)ENS210A }, > + { .compatible = "sciosense,ens211", .data = (void *)ENS211}, ... CJ
On Wed, 10 Jul 2024 14:24:05 +0100 Joshua Felmeden <jfelmeden@thegoodpenguin.co.uk> wrote: > Add support for ENS210/ENS210A/ENS211/ENS212/ENS213A/ENS215. > > The ENS21x is a family of temperature and relative humidity sensors with > accuracies tailored to the needs of specific applications. > > Signed-off-by: Joshua Felmeden <jfelmeden@thegoodpenguin.co.uk> Hi Joshua, Various comments inline Jonathan > --- > drivers/iio/humidity/Kconfig | 11 ++ > drivers/iio/humidity/Makefile | 1 + > drivers/iio/humidity/ens21x.c | 346 ++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 358 insertions(+) > > diff --git a/drivers/iio/humidity/Kconfig b/drivers/iio/humidity/Kconfig > index b15b7a3b66d5..ff62abf730d1 100644 > --- a/drivers/iio/humidity/Kconfig > +++ b/drivers/iio/humidity/Kconfig > @@ -25,6 +25,17 @@ config DHT11 > Other sensors should work as well as long as they speak the > same protocol. > > +config ENS21X > + tristate "ENS21X temperature and humidity sensor" > + depends on I2C > + help > + Say yes here to get support for the ScioSense ENS21X family of > + humidity and temperature sensors. > + > + This driver can also be built as a module. If so, the module will be > + called ens21x. > + Keep to local style, so one blank line only. > + > config HDC100X > tristate "TI HDC100x relative humidity and temperature sensor" > depends on I2C > diff --git a/drivers/iio/humidity/ens21x.c b/drivers/iio/humidity/ens21x.c > new file mode 100644 > index 000000000000..7b2e279d1559 > --- /dev/null > +++ b/drivers/iio/humidity/ens21x.c > @@ -0,0 +1,346 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* > + * ens21x.c - Support for ScioSense ens21x > + * temperature & humidity sensor Very short line wrap so odd looking formatting. > + * > + * (7-bit I2C slave address 0x43 ENS210) > + * (7-bit I2C slave address 0x43 ENS210A) > + * (7-bit I2C slave address 0x44 ENS211) > + * (7-bit I2C slave address 0x45 ENS212) > + * (7-bit I2C slave address 0x46 ENS213A) > + * (7-bit I2C slave address 0x47 ENS215) > + * > + * Datasheet: > + * https://www.sciosense.com/wp-content/uploads/2024/04/ENS21x-Datasheet.pdf > + * https://www.sciosense.com/wp-content/uploads/2023/12/ENS210-Datasheet.pdf > + */ > + > +#include <linux/types.h> > +#include <linux/i2c.h> > +#include <linux/delay.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/of_device.h> Fix the various of stuff below and there won't be anything from either of these headers but you should include mod_devicetable.h for the various id tables. > +#include <linux/iio/iio.h> > +#include <linux/iio/sysfs.h> Not seeing this used. It's for custom attributes only. > +#include <linux/crc7.h> > + > +/* register definitions */ > +#define ENS21X_REG_PART_ID 0x00 > +#define ENS21X_REG_DIE_REV 0x02 > +#define ENS21X_REG_UID 0x04 > +#define ENS21X_REG_SYS_CTRL 0x10 > +#define ENS21X_REG_SYS_STAT 0x11 > +#define ENS21X_REG_SENS_RUN 0x21 > +#define ENS21X_REG_SENS_START 0x22 > +#define ENS21X_REG_SENS_STOP 0x23 > +#define ENS21X_REG_SENS_STAT 0x24 > +#define ENS21X_REG_T_VAL 0x30 > +#define ENS21X_REG_H_VAL 0x33 > + > +/* value definitions */ > +#define ENS21X_SENS_START_T_START BIT(0) > +#define ENS21X_SENS_START_H_START BIT(1) > + > +#define ENS21X_SENS_STAT_T_ACTIVE BIT(0) > +#define ENS21X_SENS_STAT_H_ACTIVE BIT(1) > + > +#define ENS21X_SYS_CTRL_LOW_POWER_ENABLE BIT(0) > +#define ENS21X_SYS_CTRL_SYS_RESET BIT(7) > + > +#define ENS21X_SYS_STAT_SYS_ACTIVE BIT(0) > + > +/* magic constants */ > +#define ENS21X_CONST_TEMP_SCALE_INT 15 /* integer part of temperature scale (1/64) */ > +#define ENS21X_CONST_TEMP_SCALE_DEC 625000 /* decimal part of temperature scale */ These defines aren't magic in any sense! They are the actual values. As such, just use them inline in the code where they will also be self documenting (so probably no need for comments unless they are relating them to maths used to find the values). > +#define ENS21X_CONST_HUM_SCALE_INT 1 /* integer part of humidity scale (1/512) */ > +#define ENS21X_CONST_HUM_SCALE_DEC 953125 /* decimal part of humidity scale */ > +#define ENS21X_CONST_TEMP_OFFSET_INT -17481 /* temperature offset (64 * -273.15) */ > +#define ENS21X_CONST_TEMP_OFFSET_DEC 600000 /* decimal part of offset */ > +#define ENS210_CONST_CONVERSION_TIME 130 Conversion times are more reasonable things to have defines for if they turn up in multiple places. Good if the define tells us the units though and no need for it to involve CONST. Anything in capitals is assumed to be a define. ENS210_CONVERSION_TIME_MSECS etc > +#define ENS212_CONST_CONVERSION_TIME 32 > +#define ENS215_CONST_CONVERSION_TIME 132 > + > +static const struct of_device_id ens21x_of_match[]; This won't be needed after changes suggested below. > + > +struct ens21x_dev { > + struct i2c_client *client; > + struct mutex lock; All locks need documentation of what data they are protecting. > + int part_id; I'd expect to see this embedded in the chip_info structure suggested below. > +}; > + > +enum ens21x_partnumber { > + ENS210 = 0x0210, > + ENS210A = 0xa210, > + ENS211 = 0x0211, > + ENS212 = 0x0212, > + ENS213A = 0xa213, > + ENS215 = 0x0215, > +}; > + > +/* calculate 17-bit crc7 */ > +static u8 ens21x_crc7(u32 val) > +{ > + u32 val_be = (htonl(val & 0x1ffff) >> 0x8); Long time since I've seen htonl in kernel code. __be32 val_be = cpu_to_be32(val); I'm suspicious though. You take a le24 below, convert that to CPU then to be24. That's just always byte swapping. If you need to flip the byte order, just do that. > + > + return crc7_be(0xde, (u8 *)&val_be, 3) >> 1; > +} > + > +static int ens21x_get_measurement(struct iio_dev *indio_dev, bool temp, int *val) > +{ > + u32 regval, regval_le; > + int ret, tries; > + struct ens21x_dev *dev_data = iio_priv(indio_dev); dev_data tends to suggest output dev_get_drvdata() or similar. More common in IIO to name it after the part so ens210_data or similar. > + > + /* assert read */ > + i2c_smbus_write_byte_data(dev_data->client, ENS21X_REG_SENS_START, > + temp ? ENS21X_SENS_START_T_START : > + ENS21X_SENS_START_H_START); Check return values from all i2c calls. If it's expected to return an error (that happens for some corner cases) then document why we ignore it. > + > + /* wait for conversion to be ready */ > + switch (dev_data->part_id) { This per chip type data belongs in a chip_info structure (see below) so that this just ebcomes msleep(dev_data->chip_info.conv_time_msec); or something like that. It is almost always preferable to make this sort of stuff picking between data, rather than inline code as it puts all the per device stuff in one place rather than scattered throughout. > + case ENS210: > + case ENS210A: > + msleep(ENS210_CONST_CONVERSION_TIME); > + break; > + case ENS211: > + case ENS212: > + msleep(ENS212_CONST_CONVERSION_TIME); > + break; > + case ENS213A: > + case ENS215: > + msleep(ENS215_CONST_CONVERSION_TIME); > + break; > + default: > + dev_err(&dev_data->client->dev, "unrecognised device"); > + return -ENODEV; > + } > + > + tries = 10; > + while (tries-- > 0) { A retry loop like this needs a comment on why it's needed and why this particular number of retries. > + usleep_range(4000, 5000); > + ret = i2c_smbus_read_byte_data(dev_data->client, > + ENS21X_REG_SENS_STAT); > + if (ret < 0) > + continue; Error expected here? That's nasty if true that the device stops talking i2c right when capturing data. If not, return an error, not carry on, as it's a sign the comms is broken. > + if (!(ret & (temp ? ENS21X_SENS_STAT_T_ACTIVE : > + ENS21X_SENS_STAT_H_ACTIVE))) > + break; > + } > + if (tries < 0) { > + dev_err(&indio_dev->dev, "timeout waiting for sensor reading\n"); > + return -EIO; > + } > + > + /* perform read */ > + ret = i2c_smbus_read_i2c_block_data( > + dev_data->client, temp ? ENS21X_REG_T_VAL : ENS21X_REG_H_VAL, 3, > + (u8 *)®val_le); If it is 3 bytes, read into a u8[3] and use get_unaligned_le24() > + if (ret < 0) { > + dev_err(&dev_data->client->dev, "failed to read register"); > + return -EIO; > + } else if (ret == 3) { > + regval = le32_to_cpu(regval_le); > + if (ens21x_crc7(regval) == ((regval >> 17) & 0x7f)) { pull that bytes from the u8[3] mentioned above > + *val = regval & 0xffff; > + return IIO_VAL_INT; Always prefer the good path to be the inline one for ease of code reading.. So flip the logic. Generally we keep the IIO return values to the boundary code with the IIO core, so I'd return 0 here and check for errors at the claller before returning IIO_VAL_INT. That way we don't need to check all callers of this function handling the unusual return code right. > + }get_measurement > + /* crc fail */ > + dev_err(&indio_dev->dev, "ens invalid crc\n"); > + return -EIO; > + } > + > + dev_err(&indio_dev->dev, "expected 3 bytes, received %d\n", ret); Check that before the crc. As then you can reduce indent of the more complex code. > + return -EIO; > +} > + > +static int ens21x_read_raw(struct iio_dev *indio_dev, > + struct iio_chan_spec const *channel, int *val, > + int *val2, long mask) > +{ > + struct ens21x_dev *dev_data = iio_priv(indio_dev); > + int ret = -EINVAL; > + > + switch (mask) { > + case IIO_CHAN_INFO_RAW: > + mutex_lock(&dev_data->lock); scoped_guard(mutex)(&dev_data->lock) { ret = ens21x_get_measurement(); if (ret) return ret; return IIO_VAL_INT; } > + ret = ens21x_get_measurement( > + indio_dev, channel->type == IIO_TEMP, val); > + mutex_unlock(&dev_data->lock); > + break; > + case IIO_CHAN_INFO_SCALE: > + if (channel->type == IIO_TEMP) { > + *val = ENS21X_CONST_TEMP_SCALE_INT; > + *val2 = ENS21X_CONST_TEMP_SCALE_DEC; > + } else { > + *val = ENS21X_CONST_HUM_SCALE_INT; > + *val2 = ENS21X_CONST_HUM_SCALE_DEC; > + } > + ret = IIO_VAL_INT_PLUS_MICRO; return IIO_VAL_INT_PLUS_MICRO; > + break; > + case IIO_CHAN_INFO_OFFSET: > + if (channel->type == IIO_TEMP) { > + *val = ENS21X_CONST_TEMP_OFFSET_INT; > + *val2 = ENS21X_CONST_TEMP_OFFSET_DEC; > + ret = IIO_VAL_INT_PLUS_MICRO; > + break; > + } > + *val = 0; > + ret = IIO_VAL_INT; Odd spacing + return IIO_VAL_INT; > + break; > + default: > + break; return -EINVAL; > + } > + return ret; get rid of this as after changes above this is unreachable. For code readability reasons, early returns are good. Someone interested in a given path has to look at the minimal amount of code rather than having to chase through to a much later return with nothing actually done on the way there. > +} > + > +static const struct iio_chan_spec ens21x_channels[] = { > + /* Temperature channel */ Obvious from .type, so I'd drop the comments. > + { > + .type = IIO_TEMP, > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | > + BIT(IIO_CHAN_INFO_SCALE) | > + BIT(IIO_CHAN_INFO_OFFSET), > + }, > + /* Humidity channel */ > + { > + .type = IIO_HUMIDITYRELATIVE, > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | > + BIT(IIO_CHAN_INFO_SCALE) | > + BIT(IIO_CHAN_INFO_OFFSET), > + } > +}; > + > +static const struct iio_info ens21x_info = { > + .read_raw = ens21x_read_raw, > +}; > + > +static int ens21x_probe(struct i2c_client *client) > +{ > + const struct i2c_device_id *id = i2c_client_get_device_id(client); > + const struct of_device_id *match; > + struct ens21x_dev *dev_data; > + struct iio_dev *indio_dev; > + uint16_t part_id_le, part_id; > + int ret, tries; > + > + if (!i2c_check_functionality(client->adapter, > + I2C_FUNC_SMBUS_WRITE_BYTE_DATA | > + I2C_FUNC_SMBUS_WRITE_BYTE | > + I2C_FUNC_SMBUS_READ_I2C_BLOCK)) { > + dev_err(&client->dev, > + "adapter does not support some i2c transactions\n"); > + return -EOPNOTSUPP; in probe, nicer to use dev_err_probe() throughout. Doesn't matter much in this case, but good for consistency with where it might. return dev_err_probe(&client->dev, -EOPNOTSUPP, "adapter does not... > + } > + > + match = i2c_of_match_device(ens21x_of_match, client); > + if (!match) > + return -ENODEV; Why? I suspect what you actually want is i2c_get_match_data(client); That gets the data for all supported firmware types including DT. > + > + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*dev_data)); > + if (!indio_dev) > + return -ENOMEM; > + > + dev_data = iio_priv(indio_dev); > + i2c_set_clientdata(client, indio_dev); > + dev_data->client = client; > + mutex_init(&dev_data->lock); > + turn the power on? ret = devm_regulator_get_enabled() or similar > + /* reset device */ > + ret = i2c_smbus_write_byte_data(client, ENS21X_REG_SYS_CTRL, > + ENS21X_SYS_CTRL_SYS_RESET); > + if (ret) > + return ret; > + > + /* wait for device to become active */ > + usleep_range(4000, 5000); > + > + /* disable low power mode */ > + ret = i2c_smbus_write_byte_data(client, ENS21X_REG_SYS_CTRL, 0x00); > + if (ret) > + return ret; > + > + /* wait for device to become active */ Why this long? Reference some power up delay in the datahseet? > + tries = 10; > + while (tries-- > 0) { > + msleep(20); > + ret = i2c_smbus_read_byte_data(client, ENS21X_REG_SYS_STAT); > + if (ret < 0) > + return ret; > + if (ret & ENS21X_SYS_STAT_SYS_ACTIVE) > + break; > + } > + if (tries < 0) { > + dev_err(&client->dev, > + "timeout waiting for ens21x to become active\n"); > + return -EIO; > + } > + > + /* get part_id */ > + part_id_le = i2c_smbus_read_word_data(client, ENS21X_REG_PART_ID); > + if (part_id_le < 0) > + return part_id_le; > + part_id = le16_to_cpu(part_id_le); Smbus has a byte order and the values returned from i2c_smbus_read_word_data() are already cpu endian. So this shouldn't be needed. (that's the reason we have i2c_smbus_read_word_swapped() for when it is in reverse order of the smbus spec - happens annoyingly commonly). > + > + if (part_id != id->driver_data) { > + dev_err(&client->dev, > + "Part ID does not match (0x%04x != 0x%04lx)\n", part_id, dev_info() and no error return only. This hard check is something we used to do, but DT maintainers pointed out a while back that it breaks the use of fallback compatibles to allow old drivers to work out of the box with new hardware. It is useful to print a message though saying we've seen something unexpected. > + id->driver_data); > + return -ENODEV; > + } > + > + /* reenable low power */ > + ret = i2c_smbus_write_byte_data(client, ENS21X_REG_SYS_CTRL, > + ENS21X_SYS_CTRL_LOW_POWER_ENABLE); > + if (ret) > + return ret; > + > + dev_data->part_id = part_id; > + > + indio_dev->name = id->name; This tends to be fragile as we add other firmware types etc. So I'd prefer to see the name inside the chip_info structure suggested below. > + indio_dev->modes = INDIO_DIRECT_MODE; > + indio_dev->channels = ens21x_channels; > + indio_dev->num_channels = ARRAY_SIZE(ens21x_channels); > + indio_dev->info = &ens21x_info; > + > + return devm_iio_device_register(&client->dev, indio_dev); > +} > + > + > +static const struct of_device_id ens21x_of_match[] = { > + { .compatible = "sciosense,ens210", .data = (void *)ENS210}, Putting enums into the data fields has a habit of causing bugs. You've avoided the most common one which is a 0 value, but still I'd prefer these to be a pointer to a ens210_chip_info structure. > + { .compatible = "sciosense,ens210a", .data = (void *)ENS210A }, > + { .compatible = "sciosense,ens211", .data = (void *)ENS211}, > + { .compatible = "sciosense,ens212", .data = (void *)ENS212}, > + { .compatible = "sciosense,ens213a", .data = (void *)ENS213A }, > + { .compatible = "sciosense,ens215", .data = (void *)ENS215}, > + {}, No comma after a 'terminator' like this as we don't want it to be easy for people to put something here. > +}; > +MODULE_DEVICE_TABLE(of, ens21x_of_match); > + > +static const struct i2c_device_id ens21x_id[] = { > + {"ens210", ENS210}, > + {"ens210a", ENS210A}, > + {"ens211", ENS211}, > + {"ens212", ENS212}, > + {"ens213a", ENS213A}, > + {"ens215", ENS215}, > + {} Consistent spacing needed so after { and before } in all cases like htis. > +}; > +MODULE_DEVICE_TABLE(i2c, ens21x_id); > + > +static struct i2c_driver ens21x_driver = { > + .probe = ens21x_probe, > + .id_table = ens21x_id, > + .driver = { > + .name = "ens21x", > + .of_match_table = ens21x_of_match, > + }, > +}; > + > +module_i2c_driver(ens21x_driver); > + > +MODULE_DESCRIPTION("ScioSense ENS21x temperature and humidity sensor driver"); > +MODULE_AUTHOR("Joshua Felmeden <jfelmeden@thegoodpenguin.co.uk>"); > +MODULE_LICENSE("GPL"); > + Unless I'm reading the diff wrong looks like a spare blank line at the end. Trivial but delete that. >
diff --git a/drivers/iio/humidity/Kconfig b/drivers/iio/humidity/Kconfig index b15b7a3b66d5..ff62abf730d1 100644 --- a/drivers/iio/humidity/Kconfig +++ b/drivers/iio/humidity/Kconfig @@ -25,6 +25,17 @@ config DHT11 Other sensors should work as well as long as they speak the same protocol. +config ENS21X + tristate "ENS21X temperature and humidity sensor" + depends on I2C + help + Say yes here to get support for the ScioSense ENS21X family of + humidity and temperature sensors. + + This driver can also be built as a module. If so, the module will be + called ens21x. + + config HDC100X tristate "TI HDC100x relative humidity and temperature sensor" depends on I2C diff --git a/drivers/iio/humidity/Makefile b/drivers/iio/humidity/Makefile index 5fbeef299f61..26590d06d11f 100644 --- a/drivers/iio/humidity/Makefile +++ b/drivers/iio/humidity/Makefile @@ -5,6 +5,7 @@ obj-$(CONFIG_AM2315) += am2315.o obj-$(CONFIG_DHT11) += dht11.o +obj-$(CONFIG_ENS21X) += ens21x.o obj-$(CONFIG_HDC100X) += hdc100x.o obj-$(CONFIG_HDC2010) += hdc2010.o obj-$(CONFIG_HDC3020) += hdc3020.o diff --git a/drivers/iio/humidity/ens21x.c b/drivers/iio/humidity/ens21x.c new file mode 100644 index 000000000000..7b2e279d1559 --- /dev/null +++ b/drivers/iio/humidity/ens21x.c @@ -0,0 +1,346 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * ens21x.c - Support for ScioSense ens21x + * temperature & humidity sensor + * + * (7-bit I2C slave address 0x43 ENS210) + * (7-bit I2C slave address 0x43 ENS210A) + * (7-bit I2C slave address 0x44 ENS211) + * (7-bit I2C slave address 0x45 ENS212) + * (7-bit I2C slave address 0x46 ENS213A) + * (7-bit I2C slave address 0x47 ENS215) + * + * Datasheet: + * https://www.sciosense.com/wp-content/uploads/2024/04/ENS21x-Datasheet.pdf + * https://www.sciosense.com/wp-content/uploads/2023/12/ENS210-Datasheet.pdf + */ + +#include <linux/types.h> +#include <linux/i2c.h> +#include <linux/delay.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/of_device.h> +#include <linux/iio/iio.h> +#include <linux/iio/sysfs.h> +#include <linux/crc7.h> + +/* register definitions */ +#define ENS21X_REG_PART_ID 0x00 +#define ENS21X_REG_DIE_REV 0x02 +#define ENS21X_REG_UID 0x04 +#define ENS21X_REG_SYS_CTRL 0x10 +#define ENS21X_REG_SYS_STAT 0x11 +#define ENS21X_REG_SENS_RUN 0x21 +#define ENS21X_REG_SENS_START 0x22 +#define ENS21X_REG_SENS_STOP 0x23 +#define ENS21X_REG_SENS_STAT 0x24 +#define ENS21X_REG_T_VAL 0x30 +#define ENS21X_REG_H_VAL 0x33 + +/* value definitions */ +#define ENS21X_SENS_START_T_START BIT(0) +#define ENS21X_SENS_START_H_START BIT(1) + +#define ENS21X_SENS_STAT_T_ACTIVE BIT(0) +#define ENS21X_SENS_STAT_H_ACTIVE BIT(1) + +#define ENS21X_SYS_CTRL_LOW_POWER_ENABLE BIT(0) +#define ENS21X_SYS_CTRL_SYS_RESET BIT(7) + +#define ENS21X_SYS_STAT_SYS_ACTIVE BIT(0) + +/* magic constants */ +#define ENS21X_CONST_TEMP_SCALE_INT 15 /* integer part of temperature scale (1/64) */ +#define ENS21X_CONST_TEMP_SCALE_DEC 625000 /* decimal part of temperature scale */ +#define ENS21X_CONST_HUM_SCALE_INT 1 /* integer part of humidity scale (1/512) */ +#define ENS21X_CONST_HUM_SCALE_DEC 953125 /* decimal part of humidity scale */ +#define ENS21X_CONST_TEMP_OFFSET_INT -17481 /* temperature offset (64 * -273.15) */ +#define ENS21X_CONST_TEMP_OFFSET_DEC 600000 /* decimal part of offset */ +#define ENS210_CONST_CONVERSION_TIME 130 +#define ENS212_CONST_CONVERSION_TIME 32 +#define ENS215_CONST_CONVERSION_TIME 132 + +static const struct of_device_id ens21x_of_match[]; + +struct ens21x_dev { + struct i2c_client *client; + struct mutex lock; + int part_id; +}; + +enum ens21x_partnumber { + ENS210 = 0x0210, + ENS210A = 0xa210, + ENS211 = 0x0211, + ENS212 = 0x0212, + ENS213A = 0xa213, + ENS215 = 0x0215, +}; + +/* calculate 17-bit crc7 */ +static u8 ens21x_crc7(u32 val) +{ + u32 val_be = (htonl(val & 0x1ffff) >> 0x8); + + return crc7_be(0xde, (u8 *)&val_be, 3) >> 1; +} + +static int ens21x_get_measurement(struct iio_dev *indio_dev, bool temp, int *val) +{ + u32 regval, regval_le; + int ret, tries; + struct ens21x_dev *dev_data = iio_priv(indio_dev); + + /* assert read */ + i2c_smbus_write_byte_data(dev_data->client, ENS21X_REG_SENS_START, + temp ? ENS21X_SENS_START_T_START : + ENS21X_SENS_START_H_START); + + /* wait for conversion to be ready */ + switch (dev_data->part_id) { + case ENS210: + case ENS210A: + msleep(ENS210_CONST_CONVERSION_TIME); + break; + case ENS211: + case ENS212: + msleep(ENS212_CONST_CONVERSION_TIME); + break; + case ENS213A: + case ENS215: + msleep(ENS215_CONST_CONVERSION_TIME); + break; + default: + dev_err(&dev_data->client->dev, "unrecognised device"); + return -ENODEV; + } + + tries = 10; + while (tries-- > 0) { + usleep_range(4000, 5000); + ret = i2c_smbus_read_byte_data(dev_data->client, + ENS21X_REG_SENS_STAT); + if (ret < 0) + continue; + if (!(ret & (temp ? ENS21X_SENS_STAT_T_ACTIVE : + ENS21X_SENS_STAT_H_ACTIVE))) + break; + } + if (tries < 0) { + dev_err(&indio_dev->dev, "timeout waiting for sensor reading\n"); + return -EIO; + } + + /* perform read */ + ret = i2c_smbus_read_i2c_block_data( + dev_data->client, temp ? ENS21X_REG_T_VAL : ENS21X_REG_H_VAL, 3, + (u8 *)®val_le); + if (ret < 0) { + dev_err(&dev_data->client->dev, "failed to read register"); + return -EIO; + } else if (ret == 3) { + regval = le32_to_cpu(regval_le); + if (ens21x_crc7(regval) == ((regval >> 17) & 0x7f)) { + *val = regval & 0xffff; + return IIO_VAL_INT; + } + /* crc fail */ + dev_err(&indio_dev->dev, "ens invalid crc\n"); + return -EIO; + } + + dev_err(&indio_dev->dev, "expected 3 bytes, received %d\n", ret); + return -EIO; +} + +static int ens21x_read_raw(struct iio_dev *indio_dev, + struct iio_chan_spec const *channel, int *val, + int *val2, long mask) +{ + struct ens21x_dev *dev_data = iio_priv(indio_dev); + int ret = -EINVAL; + + switch (mask) { + case IIO_CHAN_INFO_RAW: + mutex_lock(&dev_data->lock); + ret = ens21x_get_measurement( + indio_dev, channel->type == IIO_TEMP, val); + mutex_unlock(&dev_data->lock); + break; + case IIO_CHAN_INFO_SCALE: + if (channel->type == IIO_TEMP) { + *val = ENS21X_CONST_TEMP_SCALE_INT; + *val2 = ENS21X_CONST_TEMP_SCALE_DEC; + } else { + *val = ENS21X_CONST_HUM_SCALE_INT; + *val2 = ENS21X_CONST_HUM_SCALE_DEC; + } + ret = IIO_VAL_INT_PLUS_MICRO; + break; + case IIO_CHAN_INFO_OFFSET: + if (channel->type == IIO_TEMP) { + *val = ENS21X_CONST_TEMP_OFFSET_INT; + *val2 = ENS21X_CONST_TEMP_OFFSET_DEC; + ret = IIO_VAL_INT_PLUS_MICRO; + break; + } + *val = 0; + ret = IIO_VAL_INT; + break; + default: + break; + } + return ret; +} + +static const struct iio_chan_spec ens21x_channels[] = { + /* Temperature channel */ + { + .type = IIO_TEMP, + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | + BIT(IIO_CHAN_INFO_SCALE) | + BIT(IIO_CHAN_INFO_OFFSET), + }, + /* Humidity channel */ + { + .type = IIO_HUMIDITYRELATIVE, + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | + BIT(IIO_CHAN_INFO_SCALE) | + BIT(IIO_CHAN_INFO_OFFSET), + } +}; + +static const struct iio_info ens21x_info = { + .read_raw = ens21x_read_raw, +}; + +static int ens21x_probe(struct i2c_client *client) +{ + const struct i2c_device_id *id = i2c_client_get_device_id(client); + const struct of_device_id *match; + struct ens21x_dev *dev_data; + struct iio_dev *indio_dev; + uint16_t part_id_le, part_id; + int ret, tries; + + if (!i2c_check_functionality(client->adapter, + I2C_FUNC_SMBUS_WRITE_BYTE_DATA | + I2C_FUNC_SMBUS_WRITE_BYTE | + I2C_FUNC_SMBUS_READ_I2C_BLOCK)) { + dev_err(&client->dev, + "adapter does not support some i2c transactions\n"); + return -EOPNOTSUPP; + } + + match = i2c_of_match_device(ens21x_of_match, client); + if (!match) + return -ENODEV; + + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*dev_data)); + if (!indio_dev) + return -ENOMEM; + + dev_data = iio_priv(indio_dev); + i2c_set_clientdata(client, indio_dev); + dev_data->client = client; + mutex_init(&dev_data->lock); + + /* reset device */ + ret = i2c_smbus_write_byte_data(client, ENS21X_REG_SYS_CTRL, + ENS21X_SYS_CTRL_SYS_RESET); + if (ret) + return ret; + + /* wait for device to become active */ + usleep_range(4000, 5000); + + /* disable low power mode */ + ret = i2c_smbus_write_byte_data(client, ENS21X_REG_SYS_CTRL, 0x00); + if (ret) + return ret; + + /* wait for device to become active */ + tries = 10; + while (tries-- > 0) { + msleep(20); + ret = i2c_smbus_read_byte_data(client, ENS21X_REG_SYS_STAT); + if (ret < 0) + return ret; + if (ret & ENS21X_SYS_STAT_SYS_ACTIVE) + break; + } + if (tries < 0) { + dev_err(&client->dev, + "timeout waiting for ens21x to become active\n"); + return -EIO; + } + + /* get part_id */ + part_id_le = i2c_smbus_read_word_data(client, ENS21X_REG_PART_ID); + if (part_id_le < 0) + return part_id_le; + part_id = le16_to_cpu(part_id_le); + + if (part_id != id->driver_data) { + dev_err(&client->dev, + "Part ID does not match (0x%04x != 0x%04lx)\n", part_id, + id->driver_data); + return -ENODEV; + } + + /* reenable low power */ + ret = i2c_smbus_write_byte_data(client, ENS21X_REG_SYS_CTRL, + ENS21X_SYS_CTRL_LOW_POWER_ENABLE); + if (ret) + return ret; + + dev_data->part_id = part_id; + + indio_dev->name = id->name; + indio_dev->modes = INDIO_DIRECT_MODE; + indio_dev->channels = ens21x_channels; + indio_dev->num_channels = ARRAY_SIZE(ens21x_channels); + indio_dev->info = &ens21x_info; + + return devm_iio_device_register(&client->dev, indio_dev); +} + + +static const struct of_device_id ens21x_of_match[] = { + { .compatible = "sciosense,ens210", .data = (void *)ENS210}, + { .compatible = "sciosense,ens210a", .data = (void *)ENS210A }, + { .compatible = "sciosense,ens211", .data = (void *)ENS211}, + { .compatible = "sciosense,ens212", .data = (void *)ENS212}, + { .compatible = "sciosense,ens213a", .data = (void *)ENS213A }, + { .compatible = "sciosense,ens215", .data = (void *)ENS215}, + {}, +}; +MODULE_DEVICE_TABLE(of, ens21x_of_match); + +static const struct i2c_device_id ens21x_id[] = { + {"ens210", ENS210}, + {"ens210a", ENS210A}, + {"ens211", ENS211}, + {"ens212", ENS212}, + {"ens213a", ENS213A}, + {"ens215", ENS215}, + {} +}; +MODULE_DEVICE_TABLE(i2c, ens21x_id); + +static struct i2c_driver ens21x_driver = { + .probe = ens21x_probe, + .id_table = ens21x_id, + .driver = { + .name = "ens21x", + .of_match_table = ens21x_of_match, + }, +}; + +module_i2c_driver(ens21x_driver); + +MODULE_DESCRIPTION("ScioSense ENS21x temperature and humidity sensor driver"); +MODULE_AUTHOR("Joshua Felmeden <jfelmeden@thegoodpenguin.co.uk>"); +MODULE_LICENSE("GPL"); +
Add support for ENS210/ENS210A/ENS211/ENS212/ENS213A/ENS215. The ENS21x is a family of temperature and relative humidity sensors with accuracies tailored to the needs of specific applications. Signed-off-by: Joshua Felmeden <jfelmeden@thegoodpenguin.co.uk> --- drivers/iio/humidity/Kconfig | 11 ++ drivers/iio/humidity/Makefile | 1 + drivers/iio/humidity/ens21x.c | 346 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 358 insertions(+)