* Re: [PATCH v3 2/2] iio: humidity: Add support for ENS21x
2024-07-10 13:24 ` [PATCH v3 2/2] iio: humidity: Add support for ENS21x Joshua Felmeden
@ 2024-07-12 11:50 ` kernel test robot
2024-07-13 7:42 ` kernel test robot
` (2 subsequent siblings)
3 siblings, 0 replies; 9+ messages in thread
From: kernel test robot @ 2024-07-12 11:50 UTC (permalink / raw)
To: Joshua Felmeden, Jonathan Cameron, Lars-Peter Clausen,
Rob Herring, Krzysztof Kozlowski, Conor Dooley
Cc: oe-kbuild-all, linux-iio, devicetree, linux-kernel
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
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v3 2/2] iio: humidity: Add support for ENS21x
2024-07-10 13:24 ` [PATCH v3 2/2] iio: humidity: Add support for ENS21x Joshua Felmeden
2024-07-12 11:50 ` kernel test robot
2024-07-13 7:42 ` kernel test robot
@ 2024-07-13 10:44 ` Christophe JAILLET
2024-07-13 11:47 ` Jonathan Cameron
3 siblings, 0 replies; 9+ messages in thread
From: Christophe JAILLET @ 2024-07-13 10:44 UTC (permalink / raw)
To: jfelmeden
Cc: conor+dt, devicetree, jic23, krzk+dt, lars, linux-iio,
linux-kernel, robh
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
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v3 2/2] iio: humidity: Add support for ENS21x
2024-07-10 13:24 ` [PATCH v3 2/2] iio: humidity: Add support for ENS21x Joshua Felmeden
` (2 preceding siblings ...)
2024-07-13 10:44 ` Christophe JAILLET
@ 2024-07-13 11:47 ` Jonathan Cameron
3 siblings, 0 replies; 9+ messages in thread
From: Jonathan Cameron @ 2024-07-13 11:47 UTC (permalink / raw)
To: Joshua Felmeden
Cc: Lars-Peter Clausen, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, linux-iio, devicetree, linux-kernel
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.
>
^ permalink raw reply [flat|nested] 9+ messages in thread