Support Honeywell mprls0025pa pressure sensor.
This patch series adds support for Honeywell mprls0025pa pressure sensor series.
There are a variety of sensors with different pressure ranges supported.
Changes in v4:
- Patch 1: "dt-bindings: iio: pressure: Support Honeywell mprls0025pa sensor"
- change line length to 80 characters
- make vdd-supply mandatory
- Patch 2: "iio: pressure: Honeywell mprls0025pa pressure sensor"
- change line length to 80 characters
- change regulator vcc to devm_regulator_get_enable()
- switch to probe_new
- many changes from the review
- Patch 3: "MAINTAINERS: Add Honeywell mprls0025pa sensor"
- no changes
Changes in v3:
- Patch 1: "dt-bindings: iio: pressure: Support Honeywell mprls0025pa sensor"
- fix errors while doing dt_binding_check
- add vdd-supply
- Patch 2: "iio: pressure: Honeywell mpr pressure sensor"
- change to _RAW interface
- add transfer function
- add regulator
- move to device_property_xxx functions
- many more changes from the feedbacks
- Patch 3: "MAINTAINERS: Add Honeywell mpr sensor"
- change file names
Changes in v2:
- Patch 1: "dt-bindings: iio: pressure: Support Honeywell mprls0025pa sensor"
- change the global sensor decription of mpr to the specific sensor
mprls0025pa
- change compatible string
- rename the file to honeywell,mprls0025pa.yaml
- honeywell,pmin-pascal and honeywell,pmax-pascal: add unit pascal to property
names
- add new property honeywell,transfer-function
- Patch 2: "iio: pressure: Honeywell mpr pressure sensor"
- no change so far
- will be changed and send out as new version when the dt definition is
settled down
- Patch 3: "MAINTAINERS: Add Honeywell mpr sensor"
- no change so far
Andreas Klinger (3):
dt-bindings: iio: pressure: Support Honeywell mprls0025pa sensor
iio: pressure: Honeywell mprls0025pa pressure sensor
MAINTAINERS: Add Honeywell mprls0025pa sensor
.../iio/pressure/honeywell,mprls0025pa.yaml | 104 +++++
MAINTAINERS | 7 +
drivers/iio/pressure/Kconfig | 13 +
drivers/iio/pressure/Makefile | 1 +
drivers/iio/pressure/mprls0025pa.c | 441 ++++++++++++++++++
5 files changed, 566 insertions(+)
create mode 100644 Documentation/devicetree/bindings/iio/pressure/honeywell,mprls0025pa.yaml
create mode 100644 drivers/iio/pressure/mprls0025pa.c
base-commit: 457391b0380335d5e9a5babdec90ac53928b23b4
--
2.30.2
Add myself as a maintainer for Honeywell mprls0025pa sensor.
Signed-off-by: Andreas Klinger <[email protected]>
---
MAINTAINERS | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index c6545eb54104..7b68ec3fba88 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -9361,6 +9361,13 @@ F: lib/test_hmm*
F: mm/hmm*
F: tools/testing/selftests/mm/*hmm*
+HONEYWELL MPRLS0025PA PRESSURE SENSOR SERIES IIO DRIVER
+M: Andreas Klinger <[email protected]>
+L: [email protected]
+S: Maintained
+F: Documentation/devicetree/bindings/iio/pressure/honeywell,mprls0025pa.yaml
+F: drivers/iio/pressure/mprls0025pa.c
+
HOST AP DRIVER
M: Jouni Malinen <[email protected]>
L: [email protected]
--
2.30.2
Honeywell mprls0025pa is a pressure sensor series. There are many
different models with different pressure ranges, units and transfer
functions.
The range and transfer function need to be set up in the dt. Therefore
new properties honeywell,pmin-pascal, honeywell,pmax-pascal,
honeywell,transfer-function are introduced.
Add dt-bindings.
Signed-off-by: Andreas Klinger <[email protected]>
---
.../iio/pressure/honeywell,mprls0025pa.yaml | 104 ++++++++++++++++++
1 file changed, 104 insertions(+)
create mode 100644 Documentation/devicetree/bindings/iio/pressure/honeywell,mprls0025pa.yaml
diff --git a/Documentation/devicetree/bindings/iio/pressure/honeywell,mprls0025pa.yaml b/Documentation/devicetree/bindings/iio/pressure/honeywell,mprls0025pa.yaml
new file mode 100644
index 000000000000..84a61721b597
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/pressure/honeywell,mprls0025pa.yaml
@@ -0,0 +1,104 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/pressure/honeywell,mprls0025pa.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Honeywell mprls0025pa pressure sensor
+
+maintainers:
+ - Andreas Klinger <[email protected]>
+
+description: |
+ Honeywell pressure sensor of model mprls0025pa.
+
+ This sensor has an I2C and SPI interface. Only the I2C interface is
+ implemented.
+
+ There are many models with different pressure ranges available. The vendor
+ calls them "mpr series". All of them have the identical programming model and
+ differ in the pressure range, unit and transfer function.
+
+ To support different models one need to specify the pressure range as well as
+ the transfer function. Pressure range needs to be converted from its unit to
+ pascal.
+
+ The transfer function defines the ranges of numerical values delivered by the
+ sensor. The minimal range value stands for the minimum pressure and the
+ maximum value also for the maximum pressure with linear relation inside the
+ range.
+
+ Specifications about the devices can be found at:
+ 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
+
+properties:
+ compatible:
+ const: honeywell,mprls0025pa
+
+ reg:
+ maxItems: 1
+
+ interrupts:
+ maxItems: 1
+
+ reset-gpios:
+ description:
+ Optional GPIO for resetting the device.
+ If not present the device is not resetted during the probe.
+ maxItems: 1
+
+ honeywell,pmin-pascal:
+ description:
+ Minimum pressure value the sensor can measure in pascal.
+ $ref: /schemas/types.yaml#/definitions/uint32
+
+ honeywell,pmax-pascal:
+ description:
+ Maximum pressure value the sensor can measure in pascal.
+ $ref: /schemas/types.yaml#/definitions/uint32
+
+ honeywell,transfer-function:
+ description:
+ Transfer function which defines the range of valid values delivered by the
+ sensor.
+ 1 - A, 10% to 90% of 2^24 (1677722 .. 15099494)
+ 2 - B, 2.5% to 22.5% of 2^24 (419430 .. 3774874)
+ 3 - C, 20% to 80% of 2^24 (3355443 .. 13421773)
+ $ref: /schemas/types.yaml#/definitions/uint32
+
+ vdd-supply:
+ description: provide VDD power to the sensor.
+
+required:
+ - compatible
+ - honeywell,pmax-pascal
+ - honeywell,pmin-pascal
+ - honeywell,transfer-function
+ - reg
+ - vdd-supply
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/gpio/gpio.h>
+ #include <dt-bindings/interrupt-controller/irq.h>
+ i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ pressure@18 {
+ compatible = "honeywell,mprls0025pa";
+ reg = <0x18>;
+ reset-gpios = <&gpio3 19 GPIO_ACTIVE_HIGH>;
+ interrupt-parent = <&gpio3>;
+ interrupts = <21 IRQ_TYPE_EDGE_FALLING>;
+ honeywell,pmin-pascal = <0>;
+ honeywell,pmax-pascal = <172369>;
+ honeywell,transfer-function = <1>;
+ vdd-supply = <&vcc_3v3>;
+ };
+ };
--
2.30.2
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 <[email protected]>
---
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 <[email protected]>
+ *
+ * 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 <[email protected]>");
+MODULE_DESCRIPTION("Honeywell MPRLS0025PA I2C driver");
+MODULE_LICENSE("GPL");
--
2.30.2
On 05/05/2023 15:21, Andreas Klinger wrote:
> Honeywell mprls0025pa is a pressure sensor series. There are many
> different models with different pressure ranges, units and transfer
> functions.
>
> The range and transfer function need to be set up in the dt. Therefore
> new properties honeywell,pmin-pascal, honeywell,pmax-pascal,
> honeywell,transfer-function are introduced.
>
> Add dt-bindings.
>
> Signed-off-by: Andreas Klinger <[email protected]>
> ---
> .../iio/pressure/honeywell,mprls0025pa.yaml | 104 ++++++++++++++++++
> 1 file changed, 104 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/iio/pressure/honeywell,mprls0025pa.yaml
>
> diff --git a/Documentation/devicetree/bindings/iio/pressure/honeywell,mprls0025pa.yaml b/Documentation/devicetree/bindings/iio/pressure/honeywell,mprls0025pa.yaml
> new file mode 100644
> index 000000000000..84a61721b597
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/pressure/honeywell,mprls0025pa.yaml
> @@ -0,0 +1,104 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/pressure/honeywell,mprls0025pa.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Honeywell mprls0025pa pressure sensor
> +
> +maintainers:
> + - Andreas Klinger <[email protected]>
> +
> +description: |
> + Honeywell pressure sensor of model mprls0025pa.
> +
> + This sensor has an I2C and SPI interface. Only the I2C interface is
> + implemented.
> +
> + There are many models with different pressure ranges available. The vendor
> + calls them "mpr series". All of them have the identical programming model and
> + differ in the pressure range, unit and transfer function.
> +
> + To support different models one need to specify the pressure range as well as
> + the transfer function. Pressure range needs to be converted from its unit to
> + pascal.
> +
> + The transfer function defines the ranges of numerical values delivered by the
> + sensor. The minimal range value stands for the minimum pressure and the
> + maximum value also for the maximum pressure with linear relation inside the
> + range.
> +
> + Specifications about the devices can be found at:
> + 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
> +
> +properties:
> + compatible:
> + const: honeywell,mprls0025pa
> +
> + reg:
> + maxItems: 1
> +
> + interrupts:
> + maxItems: 1
> +
> + reset-gpios:
> + description:
> + Optional GPIO for resetting the device.
> + If not present the device is not resetted during the probe.
> + maxItems: 1
> +
> + honeywell,pmin-pascal:
> + description:
> + Minimum pressure value the sensor can measure in pascal.
> + $ref: /schemas/types.yaml#/definitions/uint32
> +
> + honeywell,pmax-pascal:
> + description:
> + Maximum pressure value the sensor can measure in pascal.
> + $ref: /schemas/types.yaml#/definitions/uint32
> +
> + honeywell,transfer-function:
> + description:
Here you need | to preserve formatting.
> + Transfer function which defines the range of valid values delivered by the
> + sensor.
> + 1 - A, 10% to 90% of 2^24 (1677722 .. 15099494)
> + 2 - B, 2.5% to 22.5% of 2^24 (419430 .. 3774874)
> + 3 - C, 20% to 80% of 2^24 (3355443 .. 13421773)
> + $ref: /schemas/types.yaml#/definitions/uint32
> +
> + vdd-supply:
> + description: provide VDD power to the sensor.
> +
> +required:
> + - compatible
> + - honeywell,pmax-pascal
> + - honeywell,pmin-pascal
> + - honeywell,transfer-function
> + - reg
> + - vdd-supply
Keep the same order as they appear in properties:.
Rest looks good, so with these fixes:
Reviewed-by: Krzysztof Kozlowski <[email protected]>
---
This is an automated instruction, just in case, because many review tags
are being ignored. If you do not know the process, here is a short
explanation:
Please add Acked-by/Reviewed-by/Tested-by tags when posting new
versions, under or above your Signed-off-by tag. Tools like b4 can help
here. However, there's no need to repost patches *only* to add the tags.
The upstream maintainer will do that for acks received on the version
they apply.
https://elixir.bootlin.com/linux/v5.17/source/Documentation/process/submitting-patches.rst#L540
Best regards,
Krzysztof
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/[email protected]/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 <[email protected]>
| Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/
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
On Fri, 5 May 2023 15:22:07 +0200
Andreas Klinger <[email protected]> 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 <[email protected]>
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 <[email protected]>
> + *
> + * 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 <[email protected]> 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 <[email protected]> wrote:
> Hi Jonathan,
>
> thanks for your review. I have one comment.
>
> Jonathan Cameron <[email protected]> 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