[PATCH 1/3] dt-bindings: iio: pressure: Support Honeywell mpr sensors

[PATCH 1/3] dt-bindings: iio: pressure: Support Honeywell mpr sensors

[Andreas Klinger]

Honeywell mpr is a pressure sensor family. There are many different
types with different pressure ranges. The range needs to be set up in
the dt. Therefore new properties honeywell,pmin and honeywell,pmax are
introduced.

Add dt-bindings.

Signed-off-by: Andreas Klinger <ak@it-klinger.de>
---
 .../bindings/iio/pressure/honeywell,mpr.yaml  | 74 +++++++++++++++++++
 1 file changed, 74 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/pressure/honeywell,mpr.yaml

diff --git a/Documentation/devicetree/bindings/iio/pressure/honeywell,mpr.yaml b/Documentation/devicetree/bindings/iio/pressure/honeywell,mpr.yaml
new file mode 100644
index 000000000000..d6fad6f841cf
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/pressure/honeywell,mpr.yaml
@@ -0,0 +1,74 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/pressure/honeywell,mpr.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Honeywell mpr pressure sensor
+
+maintainers:
+  - Andreas Klinger <ak@it-klinger.de>
+
+description: |
+  Honeywell pressure sensor of type mpr. This sensor has an I2C and SPI interface. Only the I2C
+  interface is implemented.
+
+  There are many subtypes with different pressure ranges available. Therefore the minimum and
+  maximum pressure values of the specific sensor needs to be specified in Pascal.
+
+  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,mpr
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  reset-gpios:
+    description:
+      Optional GPIO for resetting the device. If not present the device is not resetted.
+    maxItems: 1
+
+  honeywell,pmin:
+    description:
+      Minimum pressure value the sensor can measure in pascal.
+    $ref: /schemas/types.yaml#/definitions/uint32
+
+  honeywell,pmax:
+    description:
+      Maximum pressure value the sensor can measure in pascal.
+    $ref: /schemas/types.yaml#/definitions/uint32
+
+required:
+  - compatible
+  - reg
+  - honeywell,pmin
+  - honeywell,pmax
+
+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,mpr";
+            reg = <0x18>;
+            reset-gpios = <&gpio3 19 GPIO_ACTIVE_HIGH>;
+            interrupt-parent = <&gpio3>;
+            interrupts = <21 IRQ_TYPE_EDGE_FALLING>;
+            honeywell,pmin = <0>;
+            honeywell,pmax = <172369>;
+        };
+    };
-- 
2.30.2

[PATCH 2/3] iio: pressure: Honeywell mpr pressure sensor

[Andreas Klinger]

Honeywell mpr is a familiy of pressure sensors.

Add initial I2C support.

Note:
- Buffered mode is supported
- SPI mode is not supported

Signed-off-by: Andreas Klinger <ak@it-klinger.de>
---
 drivers/iio/pressure/Kconfig  |  12 ++
 drivers/iio/pressure/Makefile |   1 +
 drivers/iio/pressure/mpr.c    | 307 ++++++++++++++++++++++++++++++++++
 3 files changed, 320 insertions(+)
 create mode 100644 drivers/iio/pressure/mpr.c

diff --git a/drivers/iio/pressure/Kconfig b/drivers/iio/pressure/Kconfig
index c9453389e4f7..e3ec94036e3c 100644
--- a/drivers/iio/pressure/Kconfig
+++ b/drivers/iio/pressure/Kconfig
@@ -148,6 +148,18 @@ config MPL3115
 	  To compile this driver as a module, choose M here: the module
 	  will be called mpl3115.
 
+config MPR
+	tristate "Honeywell MPR (MicroPressure sensors)"
+	depends on I2C
+	select IIO_BUFFER
+	select IIO_TRIGGERED_BUFFER
+	help
+	  Say Y here to build support for the Honeywell MicroPressure pressure sensors MPR.
+	  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 mpr.
+
 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..b701d9c7187d 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_MPR) += mpr.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/mpr.c b/drivers/iio/pressure/mpr.c
new file mode 100644
index 000000000000..1728b42bee7e
--- /dev/null
+++ b/drivers/iio/pressure/mpr.c
@@ -0,0 +1,307 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * MPR - MicroPressure pressure sensor 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 <linux/of.h>
+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/mod_devicetable.h>
+
+#include <linux/device.h>
+#include <linux/gpio/consumer.h>
+
+#include <asm/unaligned.h>
+
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+#include <linux/iio/buffer.h>
+#include <linux/iio/triggered_buffer.h>
+#include <linux/iio/trigger_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 */
+
+struct mpr_data {
+	struct device		*dev;
+	void			*client;
+	struct mutex		lock;
+	s32			pmin;
+	s32			pmax;
+	struct gpio_desc	*gpiod_reset;
+	int			irq;
+	struct completion	completion;
+	s64			channel[2] __aligned(8);
+};
+
+static int mpr_read_raw(struct iio_dev *indio_dev, struct iio_chan_spec const *chan, int *val,
+									int *val2, long mask);
+
+static const struct iio_chan_spec mpr_channels[] = {
+	{
+		.type = IIO_PRESSURE,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
+		.scan_index = 0,
+		.scan_type = {
+			.sign = 's',
+			.realbits = 64,
+			.storagebits = 64,
+			.endianness = IIO_CPU,
+		},
+	},
+	IIO_CHAN_SOFT_TIMESTAMP(1),
+};
+
+static const struct iio_info mpr_info = {
+	.read_raw = &mpr_read_raw,
+};
+
+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);
+	}
+}
+
+static int mpr_read_pressure(struct mpr_data *data, s64 *press)
+{
+	int ret, i;
+	u8 wdata[] = {0xAA, 0x00, 0x00};
+	s32 status;
+	int nloops = 10;
+	char buf[5];
+	s64 press_cnt;
+	s64 outputmin = 1677722;
+	s64 outputmax = 15099494;
+
+	reinit_completion(&data->completion);
+
+	ret = i2c_master_send(data->client, wdata, sizeof(wdata));
+	if (ret < 0) {
+		dev_err(data->dev, "error while writing ret: %d\n", ret);
+		return ret;
+	}
+
+	if (data->irq > 0) {
+		ret = wait_for_completion_timeout(&data->completion, HZ);
+		if (!ret) {
+			dev_err(data->dev, "timeout while waiting for eoc interrupt\n");
+			return -ETIMEDOUT;
+		}
+	} else {
+		/* wait until status indicates data is ready */
+		for (i = 0; i < nloops; i++) {
+			usleep_range(5000, 10000);
+			status = i2c_smbus_read_byte(data->client);
+			if (status < 0) {
+				dev_err(data->dev, "error while reading, status: %d\n", status);
+				return status;
+			}
+			if (!(status & MPR_I2C_BUSY))
+				break;
+		}
+		if (i == nloops) {
+			dev_err(data->dev, "timeout while reading\n");
+			return -ETIMEDOUT;
+		}
+	}
+
+	ret = i2c_master_recv(data->client, buf, sizeof(buf));
+	if (ret < 0) {
+		dev_err(data->dev, "error in i2c_master_recv ret: %d\n", ret);
+		return ret;
+	}
+
+	if (buf[0] & MPR_I2C_BUSY) {
+		/* it should never be the case that status still indicates business */
+		dev_err(data->dev, "data still not ready: %08x\n", buf[0]);
+		return -ETIMEDOUT;
+	}
+
+	press_cnt = buf[3] + buf[2] * 256 + buf[1] * 65536;
+
+	*press = ((press_cnt - outputmin) * (s64)(data->pmax - data->pmin))
+					/ (outputmax - outputmin) + (s64)data->pmin;
+
+	dev_dbg(data->dev, "received: %*ph cnt: %lld press: %lld\n", ret, buf, press_cnt, *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->channel[0]);
+	if (ret < 0) {
+		mutex_unlock(&data->lock);
+		goto err;
+	}
+
+	iio_push_to_buffers_with_timestamp(indio_dev, data->channel, 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;
+	s64 pressure;
+	struct mpr_data *data = iio_priv(indio_dev);
+
+	if (mask == IIO_CHAN_INFO_PROCESSED) {
+		mutex_lock(&data->lock);
+		ret = mpr_read_pressure(data, &pressure);
+		mutex_unlock(&data->lock);
+		if (ret < 0)
+			return ret;
+
+		if (chan->type == IIO_PRESSURE) {
+			*val = (s32)clamp(pressure, 0LL, 2147483648LL);
+			return IIO_VAL_INT;
+		}
+	}
+
+	return -EINVAL;
+}
+
+static int mpr_probe(struct i2c_client *client, const struct i2c_device_id *id)
+{
+	int ret;
+	struct mpr_data *data;
+	struct iio_dev *indio_dev;
+	struct device *dev = &client->dev;
+	struct device_node *np = dev->of_node;
+
+	if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_READ_BYTE))
+		return -EOPNOTSUPP;
+
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	data = iio_priv(indio_dev);
+	data->dev = &client->dev;
+	i2c_set_clientdata(client, indio_dev);
+	data->client = client;
+	data->irq = client->irq;
+
+	mutex_init(&data->lock);
+	init_completion(&data->completion);
+
+	indio_dev->name = client->name;
+	indio_dev->info = &mpr_info;
+	indio_dev->channels = mpr_channels;
+	indio_dev->num_channels = ARRAY_SIZE(mpr_channels);
+	indio_dev->modes = INDIO_DIRECT_MODE;
+
+	if (np) {
+		if (of_property_read_s32(np, "honeywell,pmin", &data->pmin)) {
+			dev_err(dev, "honeywell,pmin missing in DT\n");
+			return -EINVAL;
+		}
+		if (of_property_read_s32(np, "honeywell,pmax", &data->pmax)) {
+			dev_err(dev, "honeywell,pmax missing in DT\n");
+			return -EINVAL;
+		}
+
+		data->gpiod_reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
+		if (IS_ERR(data->gpiod_reset)) {
+			dev_err(dev, "failed to get reset-gpios: err=%pe\n",
+								data->gpiod_reset);
+			data->gpiod_reset = NULL;
+		}
+
+		if (data->irq > 0) {
+			ret = devm_request_irq(dev, data->irq, mpr_eoc_handler,
+							IRQF_TRIGGER_RISING, client->name, data);
+			if (ret) {
+				dev_err(dev, "request irq %d failed\n", data->irq);
+				return ret;
+			}
+		}
+	} else {
+		/* when loaded as i2c device we need to use default values */
+		dev_warn(dev, "no dt node found; using defaults\n");
+		data->pmin = 0;
+		data->pmax = 172369; /* 25 psi */
+		data->gpiod_reset = NULL;
+	}
+
+	mpr_reset(data);
+
+	ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL, mpr_trigger_handler, NULL);
+	if (ret < 0) {
+		dev_err(dev, "iio triggered buffer setup failed\n");
+		return ret;
+	}
+
+	ret = devm_iio_device_register(dev, indio_dev);
+	if (ret < 0) {
+		dev_err(dev, "unable to register iio device\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+static const struct of_device_id mpr_matches[] = {
+	{ .compatible = "honeywell,mpr", .data = 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, mpr_matches);
+
+static const struct i2c_device_id mpr_id[] = {
+	{ "honeywell,mpr", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, mpr_id);
+
+static struct i2c_driver mpr_driver = {
+	.probe = mpr_probe,
+	.id_table = mpr_id,
+	.driver = {
+		.name = "mpr",
+		.of_match_table = mpr_matches,
+	},
+};
+module_i2c_driver(mpr_driver);
+
+MODULE_AUTHOR("Andreas Klinger <ak@it-klinger.de>");
+MODULE_DESCRIPTION("MPR I2C driver");
+MODULE_LICENSE("GPL");
+MODULE_IMPORT_NS(IIO_MPR);
-- 
2.30.2

Re: [PATCH 1/3] dt-bindings: iio: pressure: Support Honeywell mpr sensors

[Krzysztof Kozlowski]

On 01/04/2023 11:09, Andreas Klinger wrote:
> Honeywell mpr is a pressure sensor family. There are many different
> types with different pressure ranges. The range needs to be set up in
> the dt. Therefore new properties honeywell,pmin and honeywell,pmax are
> introduced.
> 
> Add dt-bindings.
> 
> Signed-off-by: Andreas Klinger <ak@it-klinger.de>
> ---
>  .../bindings/iio/pressure/honeywell,mpr.yaml  | 74 +++++++++++++++++++
>  1 file changed, 74 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/pressure/honeywell,mpr.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/pressure/honeywell,mpr.yaml b/Documentation/devicetree/bindings/iio/pressure/honeywell,mpr.yaml
> new file mode 100644
> index 000000000000..d6fad6f841cf
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/pressure/honeywell,mpr.yaml
> @@ -0,0 +1,74 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/pressure/honeywell,mpr.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Honeywell mpr pressure sensor
> +
> +maintainers:
> +  - Andreas Klinger <ak@it-klinger.de>
> +
> +description: |
> +  Honeywell pressure sensor of type mpr. This sensor has an I2C and SPI interface. Only the I2C

Doesn't look wrapped according to Linux coding style (see Coding style).

> +  interface is implemented.
> +
> +  There are many subtypes with different pressure ranges available. Therefore the minimum and
> +  maximum pressure values of the specific sensor needs to be specified in Pascal.
> +
> +  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

Lines are not continued, so drop \

> +
> +properties:
> +  compatible:
> +    const: honeywell,mpr

You need device specific compatible, not some generic one. Rename also
then the filename (should match the compatible).

> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  reset-gpios:
> +    description:
> +      Optional GPIO for resetting the device. If not present the device is not resetted.

Are you sure it is wrapped properly?

> +    maxItems: 1
> +
> +  honeywell,pmin:
> +    description:
> +      Minimum pressure value the sensor can measure in pascal.

Use standard unit suffix:
https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/property-units.yaml

> +    $ref: /schemas/types.yaml#/definitions/uint32
> +
> +  honeywell,pmax:
> +    description:
> +      Maximum pressure value the sensor can measure in pascal.
> +    $ref: /schemas/types.yaml#/definitions/uint32

Same.

Why these values are suitable for DT? Does it depend on type of sensor
(thus it is implied from compatible) or on system setup?

> +
> +required:
> +  - compatible
> +  - reg
> +  - honeywell,pmin
> +  - honeywell,pmax
> +


Best regards,
Krzysztof

Re: [PATCH 2/3] iio: pressure: Honeywell mpr pressure sensor

[kernel test robot]

Hi Andreas,

I love your patch! Yet something to improve:

[auto build test ERROR on jic23-iio/togreg]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Andreas-Klinger/dt-bindings-iio-pressure-Support-Honeywell-mpr-sensors/20230401-171226
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
patch link:    https://lore.kernel.org/r/ZCf085W4XL2PtQf6%40arbad
patch subject: [PATCH 2/3] iio: pressure: Honeywell mpr pressure sensor
config: m68k-randconfig-r023-20230401 (https://download.01.org/0day-ci/archive/20230401/202304011957.pKSWQHyS-lkp@intel.com/config)
compiler: m68k-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/6a49dae45811d8a644c56dc18b6cdbc6ea67df98
        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-mpr-sensors/20230401-171226
        git checkout 6a49dae45811d8a644c56dc18b6cdbc6ea67df98
        # 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=m68k olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=m68k SHELL=/bin/bash

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/202304011957.pKSWQHyS-lkp@intel.com/

All errors (new ones prefixed by >>, old ones prefixed by <<):

>> ERROR: modpost: "__divdi3" [drivers/iio/pressure/mpr.ko] undefined!

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

Re: [PATCH 1/3] dt-bindings: iio: pressure: Support Honeywell mpr sensors

[Jonathan Cameron]

On Sat, 1 Apr 2023 11:09:41 +0200
Andreas Klinger <ak@it-klinger.de> wrote:

> Honeywell mpr is a pressure sensor family. There are many different
> types with different pressure ranges. The range needs to be set up in
> the dt. Therefore new properties honeywell,pmin and honeywell,pmax are
> introduced.
> 
> Add dt-bindings.
> 
> Signed-off-by: Andreas Klinger <ak@it-klinger.de>
> ---
>  .../bindings/iio/pressure/honeywell,mpr.yaml  | 74 +++++++++++++++++++
>  1 file changed, 74 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/pressure/honeywell,mpr.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/pressure/honeywell,mpr.yaml b/Documentation/devicetree/bindings/iio/pressure/honeywell,mpr.yaml
> new file mode 100644
> index 000000000000..d6fad6f841cf
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/pressure/honeywell,mpr.yaml
> @@ -0,0 +1,74 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/pressure/honeywell,mpr.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Honeywell mpr pressure sensor
> +
> +maintainers:
> +  - Andreas Klinger <ak@it-klinger.de>
> +
> +description: |
> +  Honeywell pressure sensor of type mpr. This sensor has an I2C and SPI interface. Only the I2C
> +  interface is implemented.
> +
> +  There are many subtypes with different pressure ranges available. Therefore the minimum and
> +  maximum pressure values of the specific sensor needs to be specified in Pascal.
> +
> +  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

Ouch. Anyone want to suggest to honeywell that they add some short paths for this stuff!

Please add regulator for VDD.  May well be a fixed reg on the boards you care about, but
it's still good document it's existence.

> +
> +properties:
> +  compatible:
> +    const: honeywell,mpr
> +

Re: [PATCH 1/3] dt-bindings: iio: pressure: Support Honeywell mpr sensors

[Jonathan Cameron]

On Sat, 1 Apr 2023 11:42:15 +0200
Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:

> On 01/04/2023 11:09, Andreas Klinger wrote:
> > Honeywell mpr is a pressure sensor family. There are many different
> > types with different pressure ranges. The range needs to be set up in
> > the dt. Therefore new properties honeywell,pmin and honeywell,pmax are
> > introduced.
> > 
> > Add dt-bindings.
> > 
> > Signed-off-by: Andreas Klinger <ak@it-klinger.de>
> > ---
> >  .../bindings/iio/pressure/honeywell,mpr.yaml  | 74 +++++++++++++++++++
> >  1 file changed, 74 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/iio/pressure/honeywell,mpr.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/iio/pressure/honeywell,mpr.yaml b/Documentation/devicetree/bindings/iio/pressure/honeywell,mpr.yaml
> > new file mode 100644
> > index 000000000000..d6fad6f841cf
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/pressure/honeywell,mpr.yaml
> > @@ -0,0 +1,74 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/iio/pressure/honeywell,mpr.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Honeywell mpr pressure sensor
> > +
> > +maintainers:
> > +  - Andreas Klinger <ak@it-klinger.de>
> > +
> > +description: |
> > +  Honeywell pressure sensor of type mpr. This sensor has an I2C and SPI interface. Only the I2C  
> 
> Doesn't look wrapped according to Linux coding style (see Coding style).
> 
> > +  interface is implemented.
> > +
> > +  There are many subtypes with different pressure ranges available. Therefore the minimum and
> > +  maximum pressure values of the specific sensor needs to be specified in Pascal.
> > +
> > +  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  
> 
> Lines are not continued, so drop \
> 
> > +
> > +properties:
> > +  compatible:
> > +    const: honeywell,mpr  
> 
> You need device specific compatible, not some generic one. Rename also
> then the filename (should match the compatible).
> 
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +  reset-gpios:
> > +    description:
> > +      Optional GPIO for resetting the device. If not present the device is not resetted.  
> 
> Are you sure it is wrapped properly?
> 
> > +    maxItems: 1
> > +
> > +  honeywell,pmin:
> > +    description:
> > +      Minimum pressure value the sensor can measure in pascal.  
> 
> Use standard unit suffix:
> https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/property-units.yaml
> 
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +
> > +  honeywell,pmax:
> > +    description:
> > +      Maximum pressure value the sensor can measure in pascal.
> > +    $ref: /schemas/types.yaml#/definitions/uint32  
> 
> Same.
> 
> Why these values are suitable for DT? Does it depend on type of sensor
> (thus it is implied from compatible) or on system setup?

I think we'll end up with a lot of compatibles, but that's still better
than free form description.  May still need these as well though given
the datasheet helpfully adds a foot note.

1. Custom pressure ranges are available.

Might not be worth including all the details though but unhelpfully the
bits we care about are after details like is the gel food grade or the port long.
Definitely can ignore the encoding of i2c address / spi in the last few bits but
may need the transfer function.


mpr-0025GA-A maybe as a form?

> 
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - honeywell,pmin
> > +  - honeywell,pmax
> > +  
> 
> 
> Best regards,
> Krzysztof
> 

Re: [PATCH 2/3] iio: pressure: Honeywell mpr pressure sensor

[Jonathan Cameron]

On Sat, 1 Apr 2023 11:10:11 +0200
Andreas Klinger <ak@it-klinger.de> wrote:
Hi Andreas,

Various comments inline.

> Honeywell mpr is a familiy of pressure sensors.

Spell check.

> 
> Add initial I2C support.
> 
> Note:
> - Buffered mode is supported
> - SPI mode is not supported
> 
> Signed-off-by: Andreas Klinger <ak@it-klinger.de>
> ---
>  drivers/iio/pressure/Kconfig  |  12 ++
>  drivers/iio/pressure/Makefile |   1 +
>  drivers/iio/pressure/mpr.c    | 307 ++++++++++++++++++++++++++++++++++
>  3 files changed, 320 insertions(+)
>  create mode 100644 drivers/iio/pressure/mpr.c
> 
> diff --git a/drivers/iio/pressure/Kconfig b/drivers/iio/pressure/Kconfig
> index c9453389e4f7..e3ec94036e3c 100644
> --- a/drivers/iio/pressure/Kconfig
> +++ b/drivers/iio/pressure/Kconfig
> @@ -148,6 +148,18 @@ config MPL3115
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called mpl3115.
>  
> +config MPR
> +	tristate "Honeywell MPR (MicroPressure sensors)"
> +	depends on I2C
> +	select IIO_BUFFER
> +	select IIO_TRIGGERED_BUFFER
> +	help
> +	  Say Y here to build support for the Honeywell MicroPressure pressure sensors MPR.
> +	  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 mpr.
> +
>  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..b701d9c7187d 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_MPR) += mpr.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/mpr.c b/drivers/iio/pressure/mpr.c
> new file mode 100644
> index 000000000000..1728b42bee7e
> --- /dev/null
> +++ b/drivers/iio/pressure/mpr.c
> @@ -0,0 +1,307 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * MPR - MicroPressure pressure sensor 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
> + *
Trivial but this blank line doesn't add anything useful to my eye
> + */
> +
> +#include <linux/of.h>
As below, should use stuff in linux/property.h not of.h

> +#include <linux/delay.h>
> +#include <linux/i2c.h>
> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
> +
> +#include <linux/device.h>
> +#include <linux/gpio/consumer.h>
> +
> +#include <asm/unaligned.h>
> +
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>

Not seeing any custom attributes (which is pretty much only reason you'd
need iio/sysfs.h)

> +#include <linux/iio/buffer.h>
> +#include <linux/iio/triggered_buffer.h>
> +#include <linux/iio/trigger_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 */
> +
> +struct mpr_data {
> +	struct device		*dev;

Given it's easy to get this dev from client->dev, don't bother holding
both in here.

> +	void			*client;
> +	struct mutex		lock;

All locks should have a comment saying what exactly they protect.

> +	s32			pmin;
> +	s32			pmax;
> +	struct gpio_desc	*gpiod_reset;
> +	int			irq;
> +	struct completion	completion;
> +	s64			channel[2] __aligned(8);

Why?  Good to call out the explicit purpose of this.  Usually easiest
option is to use a structure where you can call out the two elements
with names that tell reader what is going on.


> +};
> +
> +static int mpr_read_raw(struct iio_dev *indio_dev, struct iio_chan_spec const *chan, int *val,
> +									int *val2, long mask);

Should be possible to reorder code so this isn't needed.

> +
> +static const struct iio_chan_spec mpr_channels[] = {
> +	{
> +		.type = IIO_PRESSURE,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
> +		.scan_index = 0,
> +		.scan_type = {
> +			.sign = 's',
> +			.realbits = 64,
> +			.storagebits = 64,
> +			.endianness = IIO_CPU,
> +		},
> +	},
> +	IIO_CHAN_SOFT_TIMESTAMP(1),

> +};
> +
> +static const struct iio_info mpr_info = {
> +	.read_raw = &mpr_read_raw,
> +};
> +
> +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);
> +	}

If there isn't a reset signal, I'd like to see an attempt at least to write
all configuration registers to a known value (same as the one you'd
get after reset).  

> +}
> +
> +static int mpr_read_pressure(struct mpr_data *data, s64 *press)
> +{
> +	int ret, i;
> +	u8 wdata[] = {0xAA, 0x00, 0x00};
> +	s32 status;
> +	int nloops = 10;
> +	char buf[5];
> +	s64 press_cnt;
> +	s64 outputmin = 1677722;
> +	s64 outputmax = 15099494;
> +
> +	reinit_completion(&data->completion);
> +
> +	ret = i2c_master_send(data->client, wdata, sizeof(wdata));
> +	if (ret < 0) {
> +		dev_err(data->dev, "error while writing ret: %d\n", ret);
> +		return ret;
> +	}
> +
> +	if (data->irq > 0) {
> +		ret = wait_for_completion_timeout(&data->completion, HZ);
> +		if (!ret) {
> +			dev_err(data->dev, "timeout while waiting for eoc interrupt\n");
> +			return -ETIMEDOUT;
> +		}
> +	} else {
> +		/* wait until status indicates data is ready */
> +		for (i = 0; i < nloops; i++) {
> +			usleep_range(5000, 10000);

Add a comment on why this particular timing makes sense (reference to datasheet probably)

> +			status = i2c_smbus_read_byte(data->client);
> +			if (status < 0) {
> +				dev_err(data->dev, "error while reading, status: %d\n", status);
> +				return status;
> +			}
> +			if (!(status & MPR_I2C_BUSY))
> +				break;
> +		}
> +		if (i == nloops) {
> +			dev_err(data->dev, "timeout while reading\n");
> +			return -ETIMEDOUT;
> +		}
> +	}
> +
> +	ret = i2c_master_recv(data->client, buf, sizeof(buf));
> +	if (ret < 0) {
> +		dev_err(data->dev, "error in i2c_master_recv ret: %d\n", ret);
> +		return ret;
> +	}
> +
> +	if (buf[0] & MPR_I2C_BUSY) {
> +		/* it should never be the case that status still indicates business */
> +		dev_err(data->dev, "data still not ready: %08x\n", buf[0]);
> +		return -ETIMEDOUT;
> +	}
> +
> +	press_cnt = buf[3] + buf[2] * 256 + buf[1] * 65536;

Looks like a get_unaligned_le24() open coded?  Use the standard function for
this as that makes it easier to see what is going on.

> +
> +	*press = ((press_cnt - outputmin) * (s64)(data->pmax - data->pmin))
> +					/ (outputmax - outputmin) + (s64)data->pmin;

Some defines for the value of outputmin and outputmax would be better than
using local variables that happen to be constant.

Looks to me like this could be broken into 
(raw + offset) * scale in which case you could use a _RAW interface with _OFFSET and
_SCALE allow you to make this into a 24 bit (well 32 bit) slot of a buffer should
you add buffered support later.


> +
> +	dev_dbg(data->dev, "received: %*ph cnt: %lld press: %lld\n", ret, buf, press_cnt, *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->channel[0]);
> +	if (ret < 0) {
> +		mutex_unlock(&data->lock);
> +		goto err;
> +	}
> +
> +	iio_push_to_buffers_with_timestamp(indio_dev, data->channel, 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;
> +	s64 pressure;
> +	struct mpr_data *data = iio_priv(indio_dev);
> +
> +	if (mask == IIO_CHAN_INFO_PROCESSED) {

Given you only support one option neater to error out early if it's not a match even if you
have two do that twice.

	if (mask != IIO_CHAN_INFO_PROCESSED)
		return -EINVAL;


> +		mutex_lock(&data->lock);
> +		ret = mpr_read_pressure(data, &pressure);
> +		mutex_unlock(&data->lock);
> +		if (ret < 0)
> +			return ret;
> +
> +		if (chan->type == IIO_PRESSURE) {

Check that earlier. Not a lot of point in reading the data if channel type
is wrong.

> +			*val = (s32)clamp(pressure, 0LL, 2147483648LL);

What is special about that constant?  If it's a device specific value then
given it a name.

> +			return IIO_VAL_INT;
> +		}
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static int mpr_probe(struct i2c_client *client, const struct i2c_device_id *id)
> +{
> +	int ret;
> +	struct mpr_data *data;
> +	struct iio_dev *indio_dev;
> +	struct device *dev = &client->dev;
> +	struct device_node *np = dev->of_node;

As below, switch to non of specific firmware handlign.

> +
> +	if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_READ_BYTE))
> +		return -EOPNOTSUPP;
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	data = iio_priv(indio_dev);
> +	data->dev = &client->dev;
> +	i2c_set_clientdata(client, indio_dev);

I can't see where this is used.

> +	data->client = client;
> +	data->irq = client->irq;
> +
> +	mutex_init(&data->lock);
> +	init_completion(&data->completion);
> +
> +	indio_dev->name = client->name;

Should be part number.  As a string provided here (or more likely in a
device type specific structure that you are going to add).

> +	indio_dev->info = &mpr_info;
> +	indio_dev->channels = mpr_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(mpr_channels);
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +
> +	if (np) {

Don't use of specific interfaces.  Use the ones from linux/property.h instead.
Lets other firmware types work 'for free' so we are trying to avoid introducing
any of specific handling into new IIO drivers (and slowly scrubbing the old ones
for this)·

if (dev_fwnode()) etc


> +		if (of_property_read_s32(np, "honeywell,pmin", &data->pmin)) {

all these need to converting to the device_property_xxx equivalents.

> +			dev_err(dev, "honeywell,pmin missing in DT\n");
> +			return -EINVAL;
> +		}
> +		if (of_property_read_s32(np, "honeywell,pmax", &data->pmax)) {
> +			dev_err(dev, "honeywell,pmax missing in DT\n");
> +			return -EINVAL;
> +		}
> +
> +		data->gpiod_reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);

Whilst these are sort of fw node dependent they don't need the protection.
So drop them out of this if (np) block.

devm_gpiod_get_optional() given you support not having it.


> +		if (IS_ERR(data->gpiod_reset)) {
> +			dev_err(dev, "failed to get reset-gpios: err=%pe\n",
> +								data->gpiod_reset);

Not an error so dev_dbg() probably right thing to report.

> +			data->gpiod_reset = NULL;
> +		}
> +
> +		if (data->irq > 0) {
> +			ret = devm_request_irq(dev, data->irq, mpr_eoc_handler,
> +							IRQF_TRIGGER_RISING, client->name, data);
> +			if (ret) {
> +				dev_err(dev, "request irq %d failed\n", data->irq);
> +				return ret;
> +			}
> +		}
> +	} else {
> +		/* when loaded as i2c device we need to use default values */
> +		dev_warn(dev, "no dt node found; using defaults\n");

A strong argument in favour of not handling all devices via one 'compatible'
(or the the i2c_device_id version of that anyway).

> +		data->pmin = 0;
> +		data->pmax = 172369; /* 25 psi */

Perhaps worth defining the maths for that conversion so that you can just use

	psi_to_milibar(25); or whatever makes sense.

> +		data->gpiod_reset = NULL;

Should be no need to set that as it will be NULL anyway in this path.

> +	}
> +
> +	mpr_reset(data);
> +
> +	ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL, mpr_trigger_handler, NULL);
> +	if (ret < 0) {
> +		dev_err(dev, "iio triggered buffer setup failed\n");
> +		return ret;
> +	}
> +
> +	ret = devm_iio_device_register(dev, indio_dev);
> +	if (ret < 0) {
> +		dev_err(dev, "unable to register iio device\n");

In general use
		return dev_err_probe(dev, ret, ...)
for all errors that only occur on probe.  That wraps up some stuff around
deferred probing but is in general a good thing to do even if a given
call can't defer.

> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id mpr_matches[] = {
> +	{ .compatible = "honeywell,mpr", .data = 0 },

No point in filling data either here or in the i2c_device_id
entries until something useful is done with it.
When you do (which is likely after request to use compatibles
to indicate a lot of the per device type variability) then you should look
for having the data as a pointer to a const structure that describes what those
per device type details.

> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, mpr_matches);
> +
> +static const struct i2c_device_id mpr_id[] = {
> +	{ "honeywell,mpr", 0 },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, mpr_id);
> +
> +static struct i2c_driver mpr_driver = {
> +	.probe = mpr_probe,
> +	.id_table = mpr_id,
> +	.driver = {
> +		.name = "mpr",
> +		.of_match_table = mpr_matches,
> +	},
> +};
> +module_i2c_driver(mpr_driver);
> +
> +MODULE_AUTHOR("Andreas Klinger <ak@it-klinger.de>");
> +MODULE_DESCRIPTION("MPR I2C driver");
> +MODULE_LICENSE("GPL");
> +MODULE_IMPORT_NS(IIO_MPR);

Why?  You aren't exporting any symbols in that namespace that I can see.

Re: [PATCH 2/3] iio: pressure: Honeywell mpr pressure sensor

[Lars-Peter Clausen]

Hi,

Looks pretty good. Jonathan already covered most of it, a few additional 
comments.

On 4/1/23 02:10, Andreas Klinger wrote:
> [...]
> +struct mpr_data {
> +	struct device		*dev;
> +	void			*client;

Any reason not to use `struct i2c_client` for the type?

> +	struct mutex		lock;
> +	s32			pmin;
> +	s32			pmax;
> +	struct gpio_desc	*gpiod_reset;
> +	int			irq;
> +	struct completion	completion;
> +	s64			channel[2] __aligned(8);
> +};
> +
> [...]
> +static int mpr_read_pressure(struct mpr_data *data, s64 *press)
> +{
> +	int ret, i;
> +	u8 wdata[] = {0xAA, 0x00, 0x00};
> +	s32 status;
> +	int nloops = 10;
> +	char buf[5];
The tx buffer is `u8`, the rx buffer is `char`. This should be consistent.
> +	s64 press_cnt;
> +	s64 outputmin = 1677722;
> +	s64 outputmax = 15099494;
> +
> +	reinit_completion(&data->completion);
> +
> +	ret = i2c_master_send(data->client, wdata, sizeof(wdata));

The i2c family of transfer functions returns the number of bytes 
transferred, this can be less than what you expect if you get an early 
NACK. Its always good to check that all the data was transferred. E.g.

if (ret >= 0 && ret != sizeof(wdata))

    ret = -EIO;

Same for the receive later on.

[...]

Re: [PATCH 2/3] iio: pressure: Honeywell mpr pressure sensor

[kernel test robot]

Hi Andreas,

I love your patch! Yet something to improve:

[auto build test ERROR on jic23-iio/togreg]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Andreas-Klinger/dt-bindings-iio-pressure-Support-Honeywell-mpr-sensors/20230401-171226
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
patch link:    https://lore.kernel.org/r/ZCf085W4XL2PtQf6%40arbad
patch subject: [PATCH 2/3] iio: pressure: Honeywell mpr pressure sensor
config: arm-buildonly-randconfig-r004-20230401 (https://download.01.org/0day-ci/archive/20230402/202304021013.6NytoSFn-lkp@intel.com/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project 67409911353323ca5edf2049ef0df54132fa1ca7)
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
        # install arm cross compiling tool for clang build
        # apt-get install binutils-arm-linux-gnueabi
        # https://github.com/intel-lab-lkp/linux/commit/6a49dae45811d8a644c56dc18b6cdbc6ea67df98
        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-mpr-sensors/20230401-171226
        git checkout 6a49dae45811d8a644c56dc18b6cdbc6ea67df98
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm SHELL=/bin/bash

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/202304021013.6NytoSFn-lkp@intel.com/

All errors (new ones prefixed by >>, old ones prefixed by <<):

>> ERROR: modpost: "__aeabi_ldivmod" [drivers/iio/pressure/mpr.ko] undefined!

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

Re: [PATCH 2/3] iio: pressure: Honeywell mpr pressure sensor

[Andreas Klinger]

Hi Jonathan,

thanks for the extensive review. Most of it is clear but one questions remain.
See below.

Jonathan Cameron <jic23@kernel.org> schrieb am Sa, 01. Apr 18:57:
> > +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);
> > +	}
> 
> If there isn't a reset signal, I'd like to see an attempt at least to write
> all configuration registers to a known value (same as the one you'd
> get after reset).  

There is no configuration register in the sensor I could write to. But maybe I
didn't comprehend your point.

Andreas

Re: [PATCH 1/3] dt-bindings: iio: pressure: Support Honeywell mpr sensors

[Andreas Klinger]

Hi,

thanks to Krzysztof, Lars-Peter and Jonathan for the review and suggestions. I
have one thing to clarify. See below.

Jonathan Cameron <jic23@kernel.org> schrieb am Sa, 01. Apr 16:27:
> On Sat, 1 Apr 2023 11:42:15 +0200
> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
> 
> > On 01/04/2023 11:09, Andreas Klinger wrote:
[...]
> > > +  honeywell,pmin:
> > > +    description:
> > > +      Minimum pressure value the sensor can measure in pascal.  
> > 
> > Use standard unit suffix:
> > https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/property-units.yaml
> > 
> > > +    $ref: /schemas/types.yaml#/definitions/uint32
> > > +
> > > +  honeywell,pmax:
> > > +    description:
> > > +      Maximum pressure value the sensor can measure in pascal.
> > > +    $ref: /schemas/types.yaml#/definitions/uint32  
> > 
> > Same.
> > 
> > Why these values are suitable for DT? Does it depend on type of sensor
> > (thus it is implied from compatible) or on system setup?
> 
> I think we'll end up with a lot of compatibles, but that's still better
> than free form description.  May still need these as well though given
> the datasheet helpfully adds a foot note.
> 
> 1. Custom pressure ranges are available.
> 
> Might not be worth including all the details though but unhelpfully the
> bits we care about are after details like is the gel food grade or the port long.
> Definitely can ignore the encoding of i2c address / spi in the last few bits but
> may need the transfer function.
> 
> 
> mpr-0025GA-A maybe as a form?

Just to clarify: There are 32 different pressure ranges and 3 transfer functions
which means we'll end up with 96 compatibles and 96 I2C ids.

Would it be an option to have only one dt compatible and to add the pressure
range as dt property?
e. g.: honeywell,range = "0025PA";

But because of "Custom pressure ranges" we still need the DT properties. In this
case there's another "mpr-custom" compatible, right?

Andreas

Re: [PATCH 1/3] dt-bindings: iio: pressure: Support Honeywell mpr sensors

[Krzysztof Kozlowski]

On 06/04/2023 22:15, Andreas Klinger wrote:
> Hi,
> 
> thanks to Krzysztof, Lars-Peter and Jonathan for the review and suggestions. I
> have one thing to clarify. See below.
> 
> Jonathan Cameron <jic23@kernel.org> schrieb am Sa, 01. Apr 16:27:
>> On Sat, 1 Apr 2023 11:42:15 +0200
>> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
>>
>>> On 01/04/2023 11:09, Andreas Klinger wrote:
> [...]
>>>> +  honeywell,pmin:
>>>> +    description:
>>>> +      Minimum pressure value the sensor can measure in pascal.  
>>>
>>> Use standard unit suffix:
>>> https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/property-units.yaml
>>>
>>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>>> +
>>>> +  honeywell,pmax:
>>>> +    description:
>>>> +      Maximum pressure value the sensor can measure in pascal.
>>>> +    $ref: /schemas/types.yaml#/definitions/uint32  
>>>
>>> Same.
>>>
>>> Why these values are suitable for DT? Does it depend on type of sensor
>>> (thus it is implied from compatible) or on system setup?
>>
>> I think we'll end up with a lot of compatibles, but that's still better
>> than free form description.  May still need these as well though given
>> the datasheet helpfully adds a foot note.
>>
>> 1. Custom pressure ranges are available.
>>
>> Might not be worth including all the details though but unhelpfully the
>> bits we care about are after details like is the gel food grade or the port long.
>> Definitely can ignore the encoding of i2c address / spi in the last few bits but
>> may need the transfer function.
>>
>>
>> mpr-0025GA-A maybe as a form?
> 
> Just to clarify: There are 32 different pressure ranges and 3 transfer functions
> which means we'll end up with 96 compatibles and 96 I2C ids.

You anyway need compatibles per devices, don't you?
https://elixir.bootlin.com/linux/v6.1-rc1/source/Documentation/devicetree/bindings/writing-bindings.rst#L42

I already commented on this.

> 
> Would it be an option to have only one dt compatible and to add the pressure
> range as dt property?
> e. g.: honeywell,range = "0025PA";

Did you just decided to ignore my comment?

> 
> But because of "Custom pressure ranges" we still need the DT properties. In this
> case there's another "mpr-custom" compatible, right?

Please go to my email and respond to comments.

Best regards,
Krzysztof

Re: [PATCH 2/3] iio: pressure: Honeywell mpr pressure sensor

[Jonathan Cameron]

On Thu, 6 Apr 2023 21:43:57 +0200
Andreas Klinger <ak@it-klinger.de> wrote:

> Hi Jonathan,
> 
> thanks for the extensive review. Most of it is clear but one questions remain.
> See below.
> 
> Jonathan Cameron <jic23@kernel.org> schrieb am Sa, 01. Apr 18:57:
> > > +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);
> > > +	}  
> > 
> > If there isn't a reset signal, I'd like to see an attempt at least to write
> > all configuration registers to a known value (same as the one you'd
> > get after reset).    
> 
> There is no configuration register in the sensor I could write to. But maybe I
> didn't comprehend your point.

Ah. Devices is even simpler than I was anticipating. Which makes me wonder.
What does the reset actually do?

I checked the datasheet and reason to bother with this is about powersupplies
that don't come up fast enough.  Fair enough. If someone hasn't wired
the reset I guess they are happy that the power on reset will work.
(4.0 Power support requirement) 

Jonathan

> 
> Andreas
> 

Re: [PATCH 1/3] dt-bindings: iio: pressure: Support Honeywell mpr sensors

[Andreas Klinger]

Hi Krzysztof,

Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> schrieb am Sa, 01. Apr 11:42:
> On 01/04/2023 11:09, Andreas Klinger wrote:
> > Honeywell mpr is a pressure sensor family. There are many different
> > types with different pressure ranges. The range needs to be set up in
> > the dt. Therefore new properties honeywell,pmin and honeywell,pmax are
> > introduced.
> > 
> > Add dt-bindings.
> > 
> > Signed-off-by: Andreas Klinger <ak@it-klinger.de>
> > ---
> >  .../bindings/iio/pressure/honeywell,mpr.yaml  | 74 +++++++++++++++++++
> >  1 file changed, 74 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/iio/pressure/honeywell,mpr.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/iio/pressure/honeywell,mpr.yaml b/Documentation/devicetree/bindings/iio/pressure/honeywell,mpr.yaml
> > new file mode 100644
> > index 000000000000..d6fad6f841cf
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/pressure/honeywell,mpr.yaml
> > @@ -0,0 +1,74 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/iio/pressure/honeywell,mpr.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Honeywell mpr pressure sensor
> > +
> > +maintainers:
> > +  - Andreas Klinger <ak@it-klinger.de>
> > +
> > +description: |
> > +  Honeywell pressure sensor of type mpr. This sensor has an I2C and SPI interface. Only the I2C
> 
> Doesn't look wrapped according to Linux coding style (see Coding style).
> 
> > +  interface is implemented.
> > +
> > +  There are many subtypes with different pressure ranges available. Therefore the minimum and
> > +  maximum pressure values of the specific sensor needs to be specified in Pascal.
> > +
> > +  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
> 
> Lines are not continued, so drop \
> 
> > +
> > +properties:
> > +  compatible:
> > +    const: honeywell,mpr
> 
> You need device specific compatible, not some generic one. Rename also
> then the filename (should match the compatible).
> 
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +  reset-gpios:
> > +    description:
> > +      Optional GPIO for resetting the device. If not present the device is not resetted.
> 
> Are you sure it is wrapped properly?
> 
> > +    maxItems: 1
> > +
> > +  honeywell,pmin:
> > +    description:
> > +      Minimum pressure value the sensor can measure in pascal.
> 
> Use standard unit suffix:
> https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/property-units.yaml

There are only kilopascal as standard unit suffix. But with kilopascal as
integer the accuracy of the driver is very rough. Therefore I would like to use
pascal. E. g.:

honeywell,pmin-pascal

> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +
> > +  honeywell,pmax:
> > +    description:
> > +      Maximum pressure value the sensor can measure in pascal.
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> 
> Same.
> 
> Why these values are suitable for DT?

Technically from the software perspective the sensors are identical with the
only difference of having different pressure ranges, measurement units and
transfer functions.

If we omit the pressure values and transfer function we'll need 96 compatibles
and also 96 I2C ids.

But there are also custom sensor types. For covering them we'll need another
compatible and just for this case the pressure values and transfer function.

> Does it depend on type of sensor (thus it is implied from compatible) or on
> system setup?

For the standard types it can be derived from the type of sensor but for the
custom types it's not possible.

So sum up it'll look like this:

standard types:
96 compatibles, e. g. "honeywell,mpr-0025pa-a"

custom types:
1 compatible: "honeywell,mpr-custom"
honeywell,pmin-pascal
honeywell,pmax-pascal
honeywell,transfer-function


Best regards,

Andreas

Re: [PATCH 1/3] dt-bindings: iio: pressure: Support Honeywell mpr sensors

[Krzysztof Kozlowski]

On 14/04/2023 09:27, Andreas Klinger wrote:
> Hi Krzysztof,
> 
> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> schrieb am Sa, 01. Apr 11:42:
>> On 01/04/2023 11:09, Andreas Klinger wrote:
>>> Honeywell mpr is a pressure sensor family. There are many different
>>> types with different pressure ranges. The range needs to be set up in
>>> the dt. Therefore new properties honeywell,pmin and honeywell,pmax are
>>> introduced.
>>>
>>> Add dt-bindings.
>>>
>>> Signed-off-by: Andreas Klinger <ak@it-klinger.de>
>>> ---
>>>  .../bindings/iio/pressure/honeywell,mpr.yaml  | 74 +++++++++++++++++++
>>>  1 file changed, 74 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/iio/pressure/honeywell,mpr.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/iio/pressure/honeywell,mpr.yaml b/Documentation/devicetree/bindings/iio/pressure/honeywell,mpr.yaml
>>> new file mode 100644
>>> index 000000000000..d6fad6f841cf
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/iio/pressure/honeywell,mpr.yaml
>>> @@ -0,0 +1,74 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/iio/pressure/honeywell,mpr.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: Honeywell mpr pressure sensor
>>> +
>>> +maintainers:
>>> +  - Andreas Klinger <ak@it-klinger.de>
>>> +
>>> +description: |
>>> +  Honeywell pressure sensor of type mpr. This sensor has an I2C and SPI interface. Only the I2C
>>
>> Doesn't look wrapped according to Linux coding style (see Coding style).
>>
>>> +  interface is implemented.
>>> +
>>> +  There are many subtypes with different pressure ranges available. Therefore the minimum and
>>> +  maximum pressure values of the specific sensor needs to be specified in Pascal.
>>> +
>>> +  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
>>
>> Lines are not continued, so drop \
>>
>>> +
>>> +properties:
>>> +  compatible:
>>> +    const: honeywell,mpr
>>
>> You need device specific compatible, not some generic one. Rename also
>> then the filename (should match the compatible).
>>
>>> +
>>> +  reg:
>>> +    maxItems: 1
>>> +
>>> +  interrupts:
>>> +    maxItems: 1
>>> +
>>> +  reset-gpios:
>>> +    description:
>>> +      Optional GPIO for resetting the device. If not present the device is not resetted.
>>
>> Are you sure it is wrapped properly?
>>
>>> +    maxItems: 1
>>> +
>>> +  honeywell,pmin:
>>> +    description:
>>> +      Minimum pressure value the sensor can measure in pascal.
>>
>> Use standard unit suffix:
>> https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/property-units.yaml
> 
> There are only kilopascal as standard unit suffix. But with kilopascal as
> integer the accuracy of the driver is very rough. Therefore I would like to use
> pascal. E. g.:
> 
> honeywell,pmin-pascal
> 
>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>> +
>>> +  honeywell,pmax:
>>> +    description:
>>> +      Maximum pressure value the sensor can measure in pascal.
>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>
>> Same.
>>
>> Why these values are suitable for DT?
> 
> Technically from the software perspective the sensors are identical with the
> only difference of having different pressure ranges, measurement units and
> transfer functions.
> 
> If we omit the pressure values and transfer function we'll need 96 compatibles
> and also 96 I2C ids.

Compatibles are expected to be specific. You used something generic, so
not correct, although I understand the reason behind.

If we go with generic compatible, do you guarantee that all Honeywell
mpr sensors - now and in 50 years - will be 100% compatible with this
set here?

Your description calls it "type mpr", not "model mpr", so I assume you
can have entirely different sensors coming soon which will not be
compatible.

> 
> But there are also custom sensor types. For covering them we'll need another
> compatible and just for this case the pressure values and transfer function.

OK, but isn't this the case in all devices? I could accept family of
identical devices with identical programming model where difference in
supported measurement range is described via this property.

Unfortunately you did not model it that way. Instead it represents all
possible devices even with incompatible programming model.

Best regards,
Krzysztof