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
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");
--
2.30.2On 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
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
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
© 2016 - 2026 Red Hat, Inc.