On Fri, 5 May 2023 15:22:07 +0200 Andreas Klinger <ak@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx> > + * > + * 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; > +}
