| Message ID | ZFUC/3zBFQRBsYUk@arbad (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Support Honeywell mprls0025pa pressure sensor | expand |
Hi Andreas,
kernel test robot noticed the following build warnings:
[auto build test WARNING on 457391b0380335d5e9a5babdec90ac53928b23b4]
url: https://github.com/intel-lab-lkp/linux/commits/Andreas-Klinger/dt-bindings-iio-pressure-Support-Honeywell-mprls0025pa-sensor/20230505-212836
base: 457391b0380335d5e9a5babdec90ac53928b23b4
patch link: https://lore.kernel.org/r/ZFUC%2F3zBFQRBsYUk%40arbad
patch subject: [PATCH v4 2/3] iio: pressure: Honeywell mprls0025pa pressure sensor
config: sparc-allyesconfig (https://download.01.org/0day-ci/archive/20230506/202305060054.AFXCprUA-lkp@intel.com/config)
compiler: sparc64-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/2e6fb6a53d15af5fb86052a7d5d64c4d343157d0
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Andreas-Klinger/dt-bindings-iio-pressure-Support-Honeywell-mprls0025pa-sensor/20230505-212836
git checkout 2e6fb6a53d15af5fb86052a7d5d64c4d343157d0
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=sparc olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=sparc SHELL=/bin/bash drivers/iio/pressure/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202305060054.AFXCprUA-lkp@intel.com/
All warnings (new ones prefixed by >>):
In file included from include/linux/device.h:15,
from drivers/iio/pressure/mprls0025pa.c:18:
drivers/iio/pressure/mprls0025pa.c: In function 'mpr_read_pressure':
>> drivers/iio/pressure/mprls0025pa.c:178:30: warning: format '%u' expects argument of type 'unsigned int', but argument 4 has type 'long unsigned int' [-Wformat=]
178 | dev_err(dev, "received size doesn't fit - ret: %d / %u\n", ret,
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/dev_printk.h:110:30: note: in definition of macro 'dev_printk_index_wrap'
110 | _p_func(dev, fmt, ##__VA_ARGS__); \
| ^~~
include/linux/dev_printk.h:144:56: note: in expansion of macro 'dev_fmt'
144 | dev_printk_index_wrap(_dev_err, KERN_ERR, dev, dev_fmt(fmt), ##__VA_ARGS__)
| ^~~~~~~
drivers/iio/pressure/mprls0025pa.c:178:17: note: in expansion of macro 'dev_err'
178 | dev_err(dev, "received size doesn't fit - ret: %d / %u\n", ret,
| ^~~~~~~
drivers/iio/pressure/mprls0025pa.c:178:70: note: format string is defined here
178 | dev_err(dev, "received size doesn't fit - ret: %d / %u\n", ret,
| ~^
| |
| unsigned int
| %lu
drivers/iio/pressure/mprls0025pa.c:221:30: warning: format '%u' expects argument of type 'unsigned int', but argument 4 has type 'long unsigned int' [-Wformat=]
221 | dev_err(dev, "received size doesn't fit - ret: %d / %u\n", ret,
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/dev_printk.h:110:30: note: in definition of macro 'dev_printk_index_wrap'
110 | _p_func(dev, fmt, ##__VA_ARGS__); \
| ^~~
include/linux/dev_printk.h:144:56: note: in expansion of macro 'dev_fmt'
144 | dev_printk_index_wrap(_dev_err, KERN_ERR, dev, dev_fmt(fmt), ##__VA_ARGS__)
| ^~~~~~~
drivers/iio/pressure/mprls0025pa.c:221:17: note: in expansion of macro 'dev_err'
221 | dev_err(dev, "received size doesn't fit - ret: %d / %u\n", ret,
| ^~~~~~~
drivers/iio/pressure/mprls0025pa.c:221:70: note: format string is defined here
221 | dev_err(dev, "received size doesn't fit - ret: %d / %u\n", ret,
| ~^
| |
| unsigned int
| %lu
vim +178 drivers/iio/pressure/mprls0025pa.c
144
145 /**
146 * mpr_read_pressure() - Read pressure value from sensor via I2C
147 * @data: Pointer to private data struct.
148 * @press: Output value read from sensor.
149 *
150 * Reading from the sensor by sending and receiving I2C telegrams.
151 *
152 * If there is an end of conversion (EOC) interrupt registered the function
153 * waits for a maximum of one second for the interrupt.
154 *
155 * Context: The function can sleep and data->lock should be held when calling it
156 * Return:
157 * * 0 - OK, the pressure value could be read
158 * * -ETIMEDOUT - Timeout while waiting for the EOC interrupt or busy flag is
159 * still set after nloops attempts of reading
160 */
161 static int mpr_read_pressure(struct mpr_data *data, s32 *press)
162 {
163 struct device *dev = &data->client->dev;
164 int ret, i;
165 u8 wdata[] = {0xAA, 0x00, 0x00};
166 s32 status;
167 int nloops = 10;
168 u8 buf[5];
169
170 reinit_completion(&data->completion);
171
172 ret = i2c_master_send(data->client, wdata, sizeof(wdata));
173 if (ret < 0) {
174 dev_err(dev, "error while writing ret: %d\n", ret);
175 return ret;
176 }
177 if (ret != sizeof(wdata)) {
> 178 dev_err(dev, "received size doesn't fit - ret: %d / %u\n", ret,
179 sizeof(wdata));
180 return -EIO;
181 }
182
183 if (data->irq > 0) {
184 ret = wait_for_completion_timeout(&data->completion, HZ);
185 if (!ret) {
186 dev_err(dev, "timeout while waiting for eoc irq\n");
187 return -ETIMEDOUT;
188 }
189 } else {
190 /* wait until status indicates data is ready */
191 for (i = 0; i < nloops; i++) {
192 /*
193 * datasheet only says to wait at least 5 ms for the
194 * data but leave the maximum response time open
195 * --> let's try it nloops (10) times which seems to be
196 * quite long
197 */
198 usleep_range(5000, 10000);
199 status = i2c_smbus_read_byte(data->client);
200 if (status < 0) {
201 dev_err(dev,
202 "error while reading, status: %d\n",
203 status);
204 return status;
205 }
206 if (!(status & MPR_I2C_BUSY))
207 break;
208 }
209 if (i == nloops) {
210 dev_err(dev, "timeout while reading\n");
211 return -ETIMEDOUT;
212 }
213 }
214
215 ret = i2c_master_recv(data->client, buf, sizeof(buf));
216 if (ret < 0) {
217 dev_err(dev, "error in i2c_master_recv ret: %d\n", ret);
218 return ret;
219 }
220 if (ret != sizeof(buf)) {
221 dev_err(dev, "received size doesn't fit - ret: %d / %u\n", ret,
222 sizeof(buf));
223 return -EIO;
224 }
225
226 if (buf[0] & MPR_I2C_BUSY) {
227 /*
228 * it should never be the case that status still indicates
229 * business
230 */
231 dev_err(dev, "data still not ready: %08x\n", buf[0]);
232 return -ETIMEDOUT;
233 }
234
235 *press = get_unaligned_be24(&buf[1]);
236
237 dev_dbg(dev, "received: %*ph cnt: %d\n", ret, buf, *press);
238
239 return 0;
240 }
241
On Fri, 5 May 2023 15:22:07 +0200 Andreas Klinger <ak@it-klinger.de> wrote: > Honeywell mprls0025pa is a series of pressure sensors. > > Add initial I2C support. > > Note: > - IIO buffered mode is supported > - SPI mode is not supported > > Signed-off-by: Andreas Klinger <ak@it-klinger.de> Hi Andreas, A few maths related questions inline + a few other bits noticed on a fresh read through. Thanks, Jonathan > --- > drivers/iio/pressure/Kconfig | 13 + > drivers/iio/pressure/Makefile | 1 + > drivers/iio/pressure/mprls0025pa.c | 441 +++++++++++++++++++++++++++++ > 3 files changed, 455 insertions(+) > create mode 100644 drivers/iio/pressure/mprls0025pa.c > > diff --git a/drivers/iio/pressure/Kconfig b/drivers/iio/pressure/Kconfig > index c9453389e4f7..43aef35ce778 100644 > --- a/drivers/iio/pressure/Kconfig > +++ b/drivers/iio/pressure/Kconfig > @@ -148,6 +148,19 @@ config MPL3115 > To compile this driver as a module, choose M here: the module > will be called mpl3115. > > +config MPRLS0025PA > + tristate "Honeywell MPRLS0025PA (MicroPressure sensors series)" > + depends on I2C > + select IIO_BUFFER > + select IIO_TRIGGERED_BUFFER > + help > + Say Y here to build support for the Honeywell MicroPressure pressure > + sensor series. There are many different types with different pressure > + range. These ranges can be set up in the device tree. > + > + To compile this driver as a module, choose M here: the module will be > + called mprls0025pa. > + > config MS5611 > tristate "Measurement Specialties MS5611 pressure sensor driver" > select IIO_BUFFER > diff --git a/drivers/iio/pressure/Makefile b/drivers/iio/pressure/Makefile > index 083ae87ff48f..c90f77210e94 100644 > --- a/drivers/iio/pressure/Makefile > +++ b/drivers/iio/pressure/Makefile > @@ -19,6 +19,7 @@ obj-$(CONFIG_MPL115) += mpl115.o > obj-$(CONFIG_MPL115_I2C) += mpl115_i2c.o > obj-$(CONFIG_MPL115_SPI) += mpl115_spi.o > obj-$(CONFIG_MPL3115) += mpl3115.o > +obj-$(CONFIG_MPRLS0025PA) += mprls0025pa.o > obj-$(CONFIG_MS5611) += ms5611_core.o > obj-$(CONFIG_MS5611_I2C) += ms5611_i2c.o > obj-$(CONFIG_MS5611_SPI) += ms5611_spi.o > diff --git a/drivers/iio/pressure/mprls0025pa.c b/drivers/iio/pressure/mprls0025pa.c > new file mode 100644 > index 000000000000..7a8736de6e87 > --- /dev/null > +++ b/drivers/iio/pressure/mprls0025pa.c > @@ -0,0 +1,441 @@ > +// SPDX-License-Identifier: GPL-2.0-or-later > +/* > + * MPRLS0025PA - Honeywell MicroPressure pressure sensor series driver > + * > + * Copyright (c) Andreas Klinger <ak@it-klinger.de> > + * > + * Data sheet: > + * https://prod-edam.honeywell.com/content/dam/honeywell-edam/sps/siot/en-us/ > + * products/sensors/pressure-sensors/board-mount-pressure-sensors/ > + * micropressure-mpr-series/documents/ > + * sps-siot-mpr-series-datasheet-32332628-ciid-172626.pdf > + * > + * 7-bit I2C default slave address: 0x18 > + */ > + > +#include <asm/unaligned.h> Common convention is put the asm stuff after the linux includes as it's more specific. Often you get: General includes Subsystem specific includes asm includes > +#include <linux/delay.h> > +#include <linux/device.h> > +#include <linux/gpio/consumer.h> > +#include <linux/i2c.h> > +#include <linux/iio/buffer.h> > +#include <linux/iio/iio.h> > +#include <linux/iio/trigger_consumer.h> > +#include <linux/iio/triggered_buffer.h> > +#include <linux/math64.h> > +#include <linux/mod_devicetable.h> > +#include <linux/module.h> > +#include <linux/property.h> > +#include <linux/regulator/consumer.h> > + > +/* bits in i2c status byte */ > +#define MPR_I2C_POWER BIT(6) /* device is powered */ > +#define MPR_I2C_BUSY BIT(5) /* device is busy */ > +#define MPR_I2C_MEMORY BIT(2) /* integrity test passed */ > +#define MPR_I2C_MATH BIT(0) /* internal math saturation */ > + > +#define MPR_NANO_PART 1000000000LL NANO from units.h instead of this? > + > +/* > + * _INPUT interface: Why _INPUT? I kind of get what you mean, but I'd just call it sysfs interface and talk about the units the value ends up in rather than referring to a different sysfs interface the driver correctly isn't supplying to userspace. > + * Calculation formular from the datasheet: > + * pressure = (press_cnt - outputmin) * scale + pmin > + * with: > + * * pressure - measured pressure in Pascal > + * * press_cnt - raw value read from sensor > + * * pmin - minimum pressure range value of sensor (data->pmin) > + * * pmax - maximum pressure range value of sensor (data->pmax) > + * * outputmin - minimum numerical range raw value delivered by sensor > + * (MPR_OUT_MIN) > + * * outputmax - maximum numerical range raw value delivered by sensor > + * (MPR_OUT_MAX) > + * * scale - (pmax - pmin) / (outputmax - outputmin) > + * > + * _RAW interface: > + * pressure = (raw + offset) * scale > + * --> need to adjust offset for fitting into _RAW interface > + * Values for _RAW interface: > + * * raw - press_cnt > + * * scale - (pmax - pmin) / (outputmax - outputmin) > + * * offset - (-1 * outputmin) - pmin / scale > + * note: With all sensors from the datasheet pmin = 0 > + * which reduces the offset to (-1 * outputmin) > + */ > + > +/* > + * transfer function A: 10% to 90% of 2^24 > + * transfer function B: 2.5% to 22.5% of 2^24 > + * transfer function C: 20% to 80% of 2^24 > + */ > +enum mpr_func_id { > + MPR_FUNCTION_A, > + MPR_FUNCTION_B, > + MPR_FUNCTION_C, > +}; > + > +struct mpr_func_spec { > + u32 output_min; > + u32 output_max; > +}; > + > +static const struct mpr_func_spec mpr_func_spec[] = { > + [MPR_FUNCTION_A] = {.output_min = 1677722, .output_max = 15099494}, > + [MPR_FUNCTION_B] = {.output_min = 419430, .output_max = 3774874}, > + [MPR_FUNCTION_C] = {.output_min = 3355443, .output_max = 13421773}, > +}; > + > +struct mpr_chan { > + s32 pres; /* pressure value */ > + s64 ts; /* timestamp */ > +}; > + > +struct mpr_data { > + struct i2c_client *client; > + struct mutex lock; /* i2c transactions */ That's a little vague. Key bit here I think is to lock the access to the device when waiting for an interrupt or timeout on a reading, not the transactions themselves. > + u32 pmin; /* minimal pressure in pascal */ > + u32 pmax; /* maximal pressure in pascal */ > + u32 function; /* transfer function */ Why not enum mpr_func_id ? > + u32 outmin; /* > + * minimal numerical range raw > + * value from sensor > + */ > + u32 outmax; /* > + * maximal numerical range raw > + * value from sensor > + */ > + int scale; /* int part of scale */ > + int scale2; /* nano part of scale */ > + int offset; /* int part of offset */ > + int offset2; /* nano part of offset */ > + struct gpio_desc *gpiod_reset; /* reset */ > + int irq; /* end of conversion irq */ Only needed in probe, no need for a copy in here. > + struct completion completion; /* handshake from irq to read */ > + struct mpr_chan chan; /* > + * channel values for buffered > + * mode > + */ > +}; > +/** > + * mpr_read_pressure() - Read pressure value from sensor via I2C > + * @data: Pointer to private data struct. > + * @press: Output value read from sensor. > + * > + * Reading from the sensor by sending and receiving I2C telegrams. > + * > + * If there is an end of conversion (EOC) interrupt registered the function > + * waits for a maximum of one second for the interrupt. > + * > + * Context: The function can sleep and data->lock should be held when calling it > + * Return: > + * * 0 - OK, the pressure value could be read > + * * -ETIMEDOUT - Timeout while waiting for the EOC interrupt or busy flag is > + * still set after nloops attempts of reading > + */ > +static int mpr_read_pressure(struct mpr_data *data, s32 *press) > +{ > + struct device *dev = &data->client->dev; > + int ret, i; > + u8 wdata[] = {0xAA, 0x00, 0x00}; > + s32 status; > + int nloops = 10; > + u8 buf[5]; > + > + reinit_completion(&data->completion); > + > + ret = i2c_master_send(data->client, wdata, sizeof(wdata)); > + if (ret < 0) { > + dev_err(dev, "error while writing ret: %d\n", ret); > + return ret; > + } > + if (ret != sizeof(wdata)) { > + dev_err(dev, "received size doesn't fit - ret: %d / %u\n", ret, > + sizeof(wdata)); > + return -EIO; > + } > + > + if (data->irq > 0) { > + ret = wait_for_completion_timeout(&data->completion, HZ); > + if (!ret) { > + dev_err(dev, "timeout while waiting for eoc irq\n"); > + return -ETIMEDOUT; > + } > + } else { > + /* wait until status indicates data is ready */ > + for (i = 0; i < nloops; i++) { > + /* > + * datasheet only says to wait at least 5 ms for the > + * data but leave the maximum response time open > + * --> let's try it nloops (10) times which seems to be > + * quite long > + */ > + usleep_range(5000, 10000); > + status = i2c_smbus_read_byte(data->client); > + if (status < 0) { > + dev_err(dev, > + "error while reading, status: %d\n", > + status); > + return status; > + } > + if (!(status & MPR_I2C_BUSY)) > + break; > + } > + if (i == nloops) { > + dev_err(dev, "timeout while reading\n"); > + return -ETIMEDOUT; > + } > + } > + > + ret = i2c_master_recv(data->client, buf, sizeof(buf)); > + if (ret < 0) { > + dev_err(dev, "error in i2c_master_recv ret: %d\n", ret); > + return ret; > + } > + if (ret != sizeof(buf)) { > + dev_err(dev, "received size doesn't fit - ret: %d / %u\n", ret, > + sizeof(buf)); > + return -EIO; > + } > + > + if (buf[0] & MPR_I2C_BUSY) { > + /* > + * it should never be the case that status still indicates > + * business > + */ > + dev_err(dev, "data still not ready: %08x\n", buf[0]); > + return -ETIMEDOUT; > + } > + > + *press = get_unaligned_be24(&buf[1]); Is it necessary to read the 5th byte even though it's never touched? A quick galnce at datasheet shows no mention of that byte so I'm a little confused. > + > + dev_dbg(dev, "received: %*ph cnt: %d\n", ret, buf, *press); > + > + return 0; > +} > + > +static irqreturn_t mpr_eoc_handler(int irq, void *p) > +{ > + struct mpr_data *data = (struct mpr_data *)p; You should never need to cast explicitly from a void * (C spec thing) Hence I don't think that cast is necessary. > + > + complete(&data->completion); > + > + return IRQ_HANDLED; > +} > + > +static irqreturn_t mpr_trigger_handler(int irq, void *p) > +{ > + int ret; > + struct iio_poll_func *pf = p; > + struct iio_dev *indio_dev = pf->indio_dev; > + struct mpr_data *data = iio_priv(indio_dev); > + > + mutex_lock(&data->lock); > + ret = mpr_read_pressure(data, &data->chan.pres); > + if (ret < 0) { > + mutex_unlock(&data->lock); Move the err label above the mutex unlock then go to that instead, allowing you to unlock in just one place. > + goto err; > + } > + > + iio_push_to_buffers_with_timestamp(indio_dev, &data->chan, > + iio_get_time_ns(indio_dev)); > + mutex_unlock(&data->lock); > + > +err: > + iio_trigger_notify_done(indio_dev->trig); > + > + return IRQ_HANDLED; > +} > + > +static int mpr_probe(struct i2c_client *client) > +{ > + data->outmin = mpr_func_spec[data->function].output_min; > + data->outmax = mpr_func_spec[data->function].output_max; > + > + scale = div_s64(((s64)(data->pmax - data->pmin)) * MPR_NANO_PART, NANO. > + data->outmax - data->outmin); > + data->scale = div_s64(scale, MPR_NANO_PART); > + data->scale2 = scale - data->scale * MPR_NANO_PART; As below, I'm not seeing why div_s64_rem() isn't appropriate for this as well as making it immediately obvious what is going on. > + > + offset = ((-1LL) * (s64)data->outmin) * MPR_NANO_PART - > + div_s64(div_s64((s64)data->pmin * MPR_NANO_PART, scale), > + MPR_NANO_PART); I'm guessing the multiply by MPR_NANO_PART then divide by it is to maintain precision? If so I'd like a comment here explaining the logic behind it. > + data->offset = div_s64(offset, MPR_NANO_PART); > + data->offset2 = offset - data->offset * MPR_NANO_PART; Is this a round about way of doing offset % NANO? div_s64_rem() appropriate here? > + > + if (data->irq > 0) { > + ret = devm_request_irq(dev, data->irq, mpr_eoc_handler, > + IRQF_TRIGGER_RISING, client->name, data); > + if (ret) > + return dev_err_probe(dev, ret, > + "request irq %d failed\n", data->irq); > + } > + > + data->gpiod_reset = devm_gpiod_get_optional(dev, "reset", > + GPIOD_OUT_HIGH); > + if (IS_ERR(data->gpiod_reset)) > + return dev_err_probe(dev, PTR_ERR(data->gpiod_reset), > + "request reset-gpio failed\n"); > + > + mpr_reset(data); > + > + ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL, > + mpr_trigger_handler, NULL); > + if (ret) > + return dev_err_probe(dev, ret, > + "iio triggered buffer setup failed\n"); > + > + ret = devm_iio_device_register(dev, indio_dev); > + if (ret) > + return dev_err_probe(dev, ret, > + "unable to register iio device\n"); > + > + return 0; > +}
Hi Jonathan, thanks for your review. I have one comment. Jonathan Cameron <jic23@kernel.org> schrieb am Sa, 06. Mai 17:04: > > + int scale; /* int part of scale */ > > + int scale2; /* nano part of scale */ > > + int offset; /* int part of offset */ > > + int offset2; /* nano part of offset */ > > + struct gpio_desc *gpiod_reset; /* reset */ > > + int irq; /* end of conversion irq */ > > Only needed in probe, no need for a copy in here. It's also used in mpr_read_pressure() to distinguish the two possible operation modes: - waiting for an interrupt - reading in a loop until status indicates data ready Andreas
On Tue, 16 May 2023 12:28:18 +0200 Andreas Klinger <ak@it-klinger.de> wrote: > Hi Jonathan, > > thanks for your review. I have one comment. > > Jonathan Cameron <jic23@kernel.org> schrieb am Sa, 06. Mai 17:04: > > > + int scale; /* int part of scale */ > > > + int scale2; /* nano part of scale */ > > > + int offset; /* int part of offset */ > > > + int offset2; /* nano part of offset */ > > > + struct gpio_desc *gpiod_reset; /* reset */ > > > + int irq; /* end of conversion irq */ > > > > Only needed in probe, no need for a copy in here. > > It's also used in mpr_read_pressure() to distinguish the two possible operation > modes: > - waiting for an interrupt > - reading in a loop until status indicates data ready > Oops. Thanks for pointing that out! > Andreas
diff --git a/drivers/iio/pressure/Kconfig b/drivers/iio/pressure/Kconfig index c9453389e4f7..43aef35ce778 100644 --- a/drivers/iio/pressure/Kconfig +++ b/drivers/iio/pressure/Kconfig @@ -148,6 +148,19 @@ config MPL3115 To compile this driver as a module, choose M here: the module will be called mpl3115. +config MPRLS0025PA + tristate "Honeywell MPRLS0025PA (MicroPressure sensors series)" + depends on I2C + select IIO_BUFFER + select IIO_TRIGGERED_BUFFER + help + Say Y here to build support for the Honeywell MicroPressure pressure + sensor series. There are many different types with different pressure + range. These ranges can be set up in the device tree. + + To compile this driver as a module, choose M here: the module will be + called mprls0025pa. + config MS5611 tristate "Measurement Specialties MS5611 pressure sensor driver" select IIO_BUFFER diff --git a/drivers/iio/pressure/Makefile b/drivers/iio/pressure/Makefile index 083ae87ff48f..c90f77210e94 100644 --- a/drivers/iio/pressure/Makefile +++ b/drivers/iio/pressure/Makefile @@ -19,6 +19,7 @@ obj-$(CONFIG_MPL115) += mpl115.o obj-$(CONFIG_MPL115_I2C) += mpl115_i2c.o obj-$(CONFIG_MPL115_SPI) += mpl115_spi.o obj-$(CONFIG_MPL3115) += mpl3115.o +obj-$(CONFIG_MPRLS0025PA) += mprls0025pa.o obj-$(CONFIG_MS5611) += ms5611_core.o obj-$(CONFIG_MS5611_I2C) += ms5611_i2c.o obj-$(CONFIG_MS5611_SPI) += ms5611_spi.o diff --git a/drivers/iio/pressure/mprls0025pa.c b/drivers/iio/pressure/mprls0025pa.c new file mode 100644 index 000000000000..7a8736de6e87 --- /dev/null +++ b/drivers/iio/pressure/mprls0025pa.c @@ -0,0 +1,441 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * MPRLS0025PA - Honeywell MicroPressure pressure sensor series driver + * + * Copyright (c) Andreas Klinger <ak@it-klinger.de> + * + * Data sheet: + * https://prod-edam.honeywell.com/content/dam/honeywell-edam/sps/siot/en-us/ + * products/sensors/pressure-sensors/board-mount-pressure-sensors/ + * micropressure-mpr-series/documents/ + * sps-siot-mpr-series-datasheet-32332628-ciid-172626.pdf + * + * 7-bit I2C default slave address: 0x18 + */ + +#include <asm/unaligned.h> +#include <linux/delay.h> +#include <linux/device.h> +#include <linux/gpio/consumer.h> +#include <linux/i2c.h> +#include <linux/iio/buffer.h> +#include <linux/iio/iio.h> +#include <linux/iio/trigger_consumer.h> +#include <linux/iio/triggered_buffer.h> +#include <linux/math64.h> +#include <linux/mod_devicetable.h> +#include <linux/module.h> +#include <linux/property.h> +#include <linux/regulator/consumer.h> + +/* bits in i2c status byte */ +#define MPR_I2C_POWER BIT(6) /* device is powered */ +#define MPR_I2C_BUSY BIT(5) /* device is busy */ +#define MPR_I2C_MEMORY BIT(2) /* integrity test passed */ +#define MPR_I2C_MATH BIT(0) /* internal math saturation */ + +#define MPR_NANO_PART 1000000000LL + +/* + * _INPUT interface: + * Calculation formular from the datasheet: + * pressure = (press_cnt - outputmin) * scale + pmin + * with: + * * pressure - measured pressure in Pascal + * * press_cnt - raw value read from sensor + * * pmin - minimum pressure range value of sensor (data->pmin) + * * pmax - maximum pressure range value of sensor (data->pmax) + * * outputmin - minimum numerical range raw value delivered by sensor + * (MPR_OUT_MIN) + * * outputmax - maximum numerical range raw value delivered by sensor + * (MPR_OUT_MAX) + * * scale - (pmax - pmin) / (outputmax - outputmin) + * + * _RAW interface: + * pressure = (raw + offset) * scale + * --> need to adjust offset for fitting into _RAW interface + * Values for _RAW interface: + * * raw - press_cnt + * * scale - (pmax - pmin) / (outputmax - outputmin) + * * offset - (-1 * outputmin) - pmin / scale + * note: With all sensors from the datasheet pmin = 0 + * which reduces the offset to (-1 * outputmin) + */ + +/* + * transfer function A: 10% to 90% of 2^24 + * transfer function B: 2.5% to 22.5% of 2^24 + * transfer function C: 20% to 80% of 2^24 + */ +enum mpr_func_id { + MPR_FUNCTION_A, + MPR_FUNCTION_B, + MPR_FUNCTION_C, +}; + +struct mpr_func_spec { + u32 output_min; + u32 output_max; +}; + +static const struct mpr_func_spec mpr_func_spec[] = { + [MPR_FUNCTION_A] = {.output_min = 1677722, .output_max = 15099494}, + [MPR_FUNCTION_B] = {.output_min = 419430, .output_max = 3774874}, + [MPR_FUNCTION_C] = {.output_min = 3355443, .output_max = 13421773}, +}; + +struct mpr_chan { + s32 pres; /* pressure value */ + s64 ts; /* timestamp */ +}; + +struct mpr_data { + struct i2c_client *client; + struct mutex lock; /* i2c transactions */ + u32 pmin; /* minimal pressure in pascal */ + u32 pmax; /* maximal pressure in pascal */ + u32 function; /* transfer function */ + u32 outmin; /* + * minimal numerical range raw + * value from sensor + */ + u32 outmax; /* + * maximal numerical range raw + * value from sensor + */ + int scale; /* int part of scale */ + int scale2; /* nano part of scale */ + int offset; /* int part of offset */ + int offset2; /* nano part of offset */ + struct gpio_desc *gpiod_reset; /* reset */ + int irq; /* end of conversion irq */ + struct completion completion; /* handshake from irq to read */ + struct mpr_chan chan; /* + * channel values for buffered + * mode + */ +}; + +static const struct iio_chan_spec mpr_channels[] = { + { + .type = IIO_PRESSURE, + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | + BIT(IIO_CHAN_INFO_SCALE) | + BIT(IIO_CHAN_INFO_OFFSET), + .scan_index = 0, + .scan_type = { + .sign = 's', + .realbits = 32, + .storagebits = 32, + .endianness = IIO_CPU, + }, + }, + IIO_CHAN_SOFT_TIMESTAMP(1), +}; + +static void mpr_reset(struct mpr_data *data) +{ + if (data->gpiod_reset) { + gpiod_set_value(data->gpiod_reset, 0); + udelay(10); + gpiod_set_value(data->gpiod_reset, 1); + } +} + +/** + * mpr_read_pressure() - Read pressure value from sensor via I2C + * @data: Pointer to private data struct. + * @press: Output value read from sensor. + * + * Reading from the sensor by sending and receiving I2C telegrams. + * + * If there is an end of conversion (EOC) interrupt registered the function + * waits for a maximum of one second for the interrupt. + * + * Context: The function can sleep and data->lock should be held when calling it + * Return: + * * 0 - OK, the pressure value could be read + * * -ETIMEDOUT - Timeout while waiting for the EOC interrupt or busy flag is + * still set after nloops attempts of reading + */ +static int mpr_read_pressure(struct mpr_data *data, s32 *press) +{ + struct device *dev = &data->client->dev; + int ret, i; + u8 wdata[] = {0xAA, 0x00, 0x00}; + s32 status; + int nloops = 10; + u8 buf[5]; + + reinit_completion(&data->completion); + + ret = i2c_master_send(data->client, wdata, sizeof(wdata)); + if (ret < 0) { + dev_err(dev, "error while writing ret: %d\n", ret); + return ret; + } + if (ret != sizeof(wdata)) { + dev_err(dev, "received size doesn't fit - ret: %d / %u\n", ret, + sizeof(wdata)); + return -EIO; + } + + if (data->irq > 0) { + ret = wait_for_completion_timeout(&data->completion, HZ); + if (!ret) { + dev_err(dev, "timeout while waiting for eoc irq\n"); + return -ETIMEDOUT; + } + } else { + /* wait until status indicates data is ready */ + for (i = 0; i < nloops; i++) { + /* + * datasheet only says to wait at least 5 ms for the + * data but leave the maximum response time open + * --> let's try it nloops (10) times which seems to be + * quite long + */ + usleep_range(5000, 10000); + status = i2c_smbus_read_byte(data->client); + if (status < 0) { + dev_err(dev, + "error while reading, status: %d\n", + status); + return status; + } + if (!(status & MPR_I2C_BUSY)) + break; + } + if (i == nloops) { + dev_err(dev, "timeout while reading\n"); + return -ETIMEDOUT; + } + } + + ret = i2c_master_recv(data->client, buf, sizeof(buf)); + if (ret < 0) { + dev_err(dev, "error in i2c_master_recv ret: %d\n", ret); + return ret; + } + if (ret != sizeof(buf)) { + dev_err(dev, "received size doesn't fit - ret: %d / %u\n", ret, + sizeof(buf)); + return -EIO; + } + + if (buf[0] & MPR_I2C_BUSY) { + /* + * it should never be the case that status still indicates + * business + */ + dev_err(dev, "data still not ready: %08x\n", buf[0]); + return -ETIMEDOUT; + } + + *press = get_unaligned_be24(&buf[1]); + + dev_dbg(dev, "received: %*ph cnt: %d\n", ret, buf, *press); + + return 0; +} + +static irqreturn_t mpr_eoc_handler(int irq, void *p) +{ + struct mpr_data *data = (struct mpr_data *)p; + + complete(&data->completion); + + return IRQ_HANDLED; +} + +static irqreturn_t mpr_trigger_handler(int irq, void *p) +{ + int ret; + struct iio_poll_func *pf = p; + struct iio_dev *indio_dev = pf->indio_dev; + struct mpr_data *data = iio_priv(indio_dev); + + mutex_lock(&data->lock); + ret = mpr_read_pressure(data, &data->chan.pres); + if (ret < 0) { + mutex_unlock(&data->lock); + goto err; + } + + iio_push_to_buffers_with_timestamp(indio_dev, &data->chan, + iio_get_time_ns(indio_dev)); + mutex_unlock(&data->lock); + +err: + iio_trigger_notify_done(indio_dev->trig); + + return IRQ_HANDLED; +} + +static int mpr_read_raw(struct iio_dev *indio_dev, + struct iio_chan_spec const *chan, int *val, int *val2, long mask) +{ + int ret; + s32 pressure; + struct mpr_data *data = iio_priv(indio_dev); + + if (chan->type != IIO_PRESSURE) + return -EINVAL; + + switch (mask) { + case IIO_CHAN_INFO_RAW: + mutex_lock(&data->lock); + ret = mpr_read_pressure(data, &pressure); + mutex_unlock(&data->lock); + if (ret < 0) + return ret; + *val = pressure; + return IIO_VAL_INT; + case IIO_CHAN_INFO_SCALE: + *val = data->scale; + *val2 = data->scale2; + return IIO_VAL_INT_PLUS_NANO; + case IIO_CHAN_INFO_OFFSET: + *val = data->offset; + *val2 = data->offset2; + return IIO_VAL_INT_PLUS_NANO; + default: + return -EINVAL; + } +} + +static const struct iio_info mpr_info = { + .read_raw = &mpr_read_raw, +}; + +static int mpr_probe(struct i2c_client *client) +{ + int ret; + struct mpr_data *data; + struct iio_dev *indio_dev; + struct device *dev = &client->dev; + s64 scale, offset; + + if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_READ_BYTE)) + return dev_err_probe(dev, -EOPNOTSUPP, + "I2C functionality not supported\n"); + + indio_dev = devm_iio_device_alloc(dev, sizeof(*data)); + if (!indio_dev) + return dev_err_probe(dev, -ENOMEM, "couldn't get iio_dev\n"); + + data = iio_priv(indio_dev); + data->client = client; + data->irq = client->irq; + + mutex_init(&data->lock); + init_completion(&data->completion); + + indio_dev->name = "mprls0025pa"; + indio_dev->info = &mpr_info; + indio_dev->channels = mpr_channels; + indio_dev->num_channels = ARRAY_SIZE(mpr_channels); + indio_dev->modes = INDIO_DIRECT_MODE; + + ret = devm_regulator_get_enable(dev, "vdd"); + if (ret) + return dev_err_probe(dev, ret, + "can't get and enable vdd supply\n"); + + if (dev_fwnode(dev)) { + ret = device_property_read_u32(dev, "honeywell,pmin-pascal", + &data->pmin); + if (ret) + return dev_err_probe(dev, ret, + "honeywell,pmin-pascal could not be read\n"); + ret = device_property_read_u32(dev, "honeywell,pmax-pascal", + &data->pmax); + if (ret) + return dev_err_probe(dev, ret, + "honeywell,pmax-pascal could not be read\n"); + ret = device_property_read_u32(dev, + "honeywell,transfer-function", &data->function); + if (ret) + return dev_err_probe(dev, ret, + "honeywell,transfer-function could not be read\n"); + if (data->function > MPR_FUNCTION_C) + return dev_err_probe(dev, -EINVAL, + "honeywell,transfer-function %d invalid\n", + data->function); + } else { + /* when loaded as i2c device we need to use default values */ + dev_notice(dev, "firmware node not found; using defaults\n"); + data->pmin = 0; + data->pmax = 172369; /* 25 psi */ + data->function = MPR_FUNCTION_A; + } + + data->outmin = mpr_func_spec[data->function].output_min; + data->outmax = mpr_func_spec[data->function].output_max; + + scale = div_s64(((s64)(data->pmax - data->pmin)) * MPR_NANO_PART, + data->outmax - data->outmin); + data->scale = div_s64(scale, MPR_NANO_PART); + data->scale2 = scale - data->scale * MPR_NANO_PART; + + offset = ((-1LL) * (s64)data->outmin) * MPR_NANO_PART - + div_s64(div_s64((s64)data->pmin * MPR_NANO_PART, scale), + MPR_NANO_PART); + data->offset = div_s64(offset, MPR_NANO_PART); + data->offset2 = offset - data->offset * MPR_NANO_PART; + + if (data->irq > 0) { + ret = devm_request_irq(dev, data->irq, mpr_eoc_handler, + IRQF_TRIGGER_RISING, client->name, data); + if (ret) + return dev_err_probe(dev, ret, + "request irq %d failed\n", data->irq); + } + + data->gpiod_reset = devm_gpiod_get_optional(dev, "reset", + GPIOD_OUT_HIGH); + if (IS_ERR(data->gpiod_reset)) + return dev_err_probe(dev, PTR_ERR(data->gpiod_reset), + "request reset-gpio failed\n"); + + mpr_reset(data); + + ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL, + mpr_trigger_handler, NULL); + if (ret) + return dev_err_probe(dev, ret, + "iio triggered buffer setup failed\n"); + + ret = devm_iio_device_register(dev, indio_dev); + if (ret) + return dev_err_probe(dev, ret, + "unable to register iio device\n"); + + return 0; +} + +static const struct of_device_id mpr_matches[] = { + { .compatible = "honeywell,mprls0025pa" }, + { } +}; +MODULE_DEVICE_TABLE(of, mpr_matches); + +static const struct i2c_device_id mpr_id[] = { + { "mprls0025pa" }, + { } +}; +MODULE_DEVICE_TABLE(i2c, mpr_id); + +static struct i2c_driver mpr_driver = { + .probe_new = mpr_probe, + .id_table = mpr_id, + .driver = { + .name = "mprls0025pa", + .of_match_table = mpr_matches, + }, +}; +module_i2c_driver(mpr_driver); + +MODULE_AUTHOR("Andreas Klinger <ak@it-klinger.de>"); +MODULE_DESCRIPTION("Honeywell MPRLS0025PA I2C driver"); +MODULE_LICENSE("GPL");
Honeywell mprls0025pa is a series of pressure sensors. Add initial I2C support. Note: - IIO buffered mode is supported - SPI mode is not supported Signed-off-by: Andreas Klinger <ak@it-klinger.de> --- drivers/iio/pressure/Kconfig | 13 + drivers/iio/pressure/Makefile | 1 + drivers/iio/pressure/mprls0025pa.c | 441 +++++++++++++++++++++++++++++ 3 files changed, 455 insertions(+) create mode 100644 drivers/iio/pressure/mprls0025pa.c