devicetree.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 0/3] Support Honeywell mprls0025pa pressure sensor
@ 2023-04-20 19:58 Andreas Klinger
  2023-04-20 20:00 ` [PATCH v3 1/3] dt-bindings: iio: pressure: Support Honeywell mprls0025pa sensor Andreas Klinger
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Andreas Klinger @ 2023-04-20 19:58 UTC (permalink / raw)
  To: linux-iio, devicetree
  Cc: Jonathan Cameron, Lars-Peter Clausen, Rob Herring,
	Krzysztof Kozlowski, Angel Iglesias, linux-kernel

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 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 mpr pressure sensor
  MAINTAINERS: Add Honeywell mpr sensor

 .../iio/pressure/honeywell,mprls0025pa.yaml   |  98 ++++
 MAINTAINERS                                   |   7 +
 drivers/iio/pressure/Kconfig                  |  12 +
 drivers/iio/pressure/Makefile                 |   1 +
 drivers/iio/pressure/mprls0025pa.c            | 429 ++++++++++++++++++
 5 files changed, 547 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/pressure/honeywell,mprls0025pa.yaml
 create mode 100644 drivers/iio/pressure/mprls0025pa.c


base-commit: e0ee50101346ca9cef52da75e3fb4380c27c042a
-- 
2.30.2

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH v3 1/3] dt-bindings: iio: pressure: Support Honeywell mprls0025pa sensor
  2023-04-20 19:58 [PATCH v3 0/3] Support Honeywell mprls0025pa pressure sensor Andreas Klinger
@ 2023-04-20 20:00 ` Andreas Klinger
  2023-04-21  7:21   ` Krzysztof Kozlowski
  2023-04-20 20:00 ` [PATCH v3 2/3] iio: pressure: Honeywell mprls0025pa pressure sensor Andreas Klinger
  2023-04-20 20:01 ` [PATCH v3 3/3] MAINTAINERS: Add Honeywell mprls0025pa sensor Andreas Klinger
  2 siblings, 1 reply; 7+ messages in thread
From: Andreas Klinger @ 2023-04-20 20:00 UTC (permalink / raw)
  To: linux-iio, devicetree
  Cc: Jonathan Cameron, Lars-Peter Clausen, Rob Herring,
	Krzysztof Kozlowski, Angel Iglesias, linux-kernel

Honeywell mpr 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 <ak@it-klinger.de>
---
 .../iio/pressure/honeywell,mprls0025pa.yaml   | 98 +++++++++++++++++++
 1 file changed, 98 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..80ab1beac7f6
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/pressure/honeywell,mprls0025pa.yaml
@@ -0,0 +1,98 @@
+# 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 mpr series pressure sensor
+
+maintainers:
+  - Andreas Klinger <ak@it-klinger.de>
+
+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
+  - reg
+  - honeywell,pmin-pascal
+  - honeywell,pmax-pascal
+  - honeywell,transfer-function
+
+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

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH v3 2/3] iio: pressure: Honeywell mprls0025pa pressure sensor
  2023-04-20 19:58 [PATCH v3 0/3] Support Honeywell mprls0025pa pressure sensor Andreas Klinger
  2023-04-20 20:00 ` [PATCH v3 1/3] dt-bindings: iio: pressure: Support Honeywell mprls0025pa sensor Andreas Klinger
@ 2023-04-20 20:00 ` Andreas Klinger
  2023-04-20 22:04   ` kernel test robot
  2023-04-23 10:30   ` Jonathan Cameron
  2023-04-20 20:01 ` [PATCH v3 3/3] MAINTAINERS: Add Honeywell mprls0025pa sensor Andreas Klinger
  2 siblings, 2 replies; 7+ messages in thread
From: Andreas Klinger @ 2023-04-20 20:00 UTC (permalink / raw)
  To: linux-iio, devicetree
  Cc: Jonathan Cameron, Lars-Peter Clausen, Rob Herring,
	Krzysztof Kozlowski, Angel Iglesias, linux-kernel

Honeywell mprls0025pa is a series 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/mprls0025pa.c | 429 +++++++++++++++++++++++++++++
 3 files changed, 442 insertions(+)
 create mode 100644 drivers/iio/pressure/mprls0025pa.c

diff --git a/drivers/iio/pressure/Kconfig b/drivers/iio/pressure/Kconfig
index c9453389e4f7..c51e53bd54bc 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 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 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..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..3d82f6790e6d
--- /dev/null
+++ b/drivers/iio/pressure/mprls0025pa.c
@@ -0,0 +1,429 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * MPR - MicroPressure pressure sensor series driver
+ *
+ * Copyright (c) Andreas Klinger <ak@it-klinger.de>
+ *
+ * Data sheet:
+ *  https://prod-edam.honeywell.com/content/dam/honeywell-edam/sps/siot/en-us/products/sensors/  \
+ *    pressure-sensors/board-mount-pressure-sensors/micropressure-mpr-series/documents/          \
+ *    sps-siot-mpr-series-datasheet-32332628-ciid-172626.pdf
+ *
+ * 7-bit I2C default slave address: 0x18
+ */
+
+#include <linux/property.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 <linux/regulator/consumer.h>
+#include <linux/math64.h>
+
+#include <asm/unaligned.h>
+
+#include <linux/iio/iio.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 */
+
+#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 interrupt */
+	struct completion	completion;	/* handshake from irq to read */
+	struct mpr_chan		chan;		/* channel values for buffered mode */
+	struct regulator	*vdd;		/* optional external voltage regulator */
+};
+
+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 / %d\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 interrupt\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 / %d\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;
+	}
+
+	return -EINVAL;
+}
+
+static const struct iio_info mpr_info = {
+	.read_raw = &mpr_read_raw,
+};
+
+static void mpr_vdd_disable(void *vdd)
+{
+	regulator_disable(vdd);
+}
+
+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;
+	s64 scale, offset;
+
+	dev_dbg(dev, "client: %s id: %s\n", client->name, id->name);
+
+	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;
+
+	data->vdd = devm_regulator_get_optional(dev, "vdd");
+	if (IS_ERR(data->vdd)) {
+		dev_dbg(dev, "can't get vdd supply err: %pe\n", data->vdd);
+		data->vdd = NULL;
+	} else {
+		ret = regulator_enable(data->vdd);
+		if (ret)
+			return dev_err_probe(dev, ret, "can't enable vdd supply\n");
+		ret = devm_add_action_or_reset(dev, mpr_vdd_disable, data->vdd);
+		if (ret)
+			return dev_err_probe(dev, ret, "can't add vdd disable action\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 missing in DT\n");
+		ret = device_property_read_u32(dev, "honeywell,pmax-pascal", &data->pmax);
+		if (ret)
+			return dev_err_probe(dev, ret, "honeywell,pmax-pascal missing in DT\n");
+		ret = device_property_read_u32(dev, "honeywell,transfer-function", &data->function);
+		if (ret)
+			return dev_err_probe(dev, ret,
+						"honeywell,transfer-function missing in DT\n");
+		if (data->function > MPR_FUNCTION_C)
+			return dev_err_probe(dev, -EINVAL,
+				"honeywell,transfer-function %d not supported\n", data->function);
+
+		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);
+		}
+	} else {
+		/* when loaded as i2c device we need to use default values */
+		dev_notice(dev, "no dt node 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;
+
+	data->gpiod_reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
+	if (IS_ERR(data->gpiod_reset)) {
+		dev_dbg(dev, "didn't get reset-gpios: err=%pe\n", data->gpiod_reset);
+		data->gpiod_reset = NULL;
+	}
+
+	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 = mpr_probe,
+	.id_table = mpr_id,
+	.driver = {
+		.name = "mprls0025pa",
+		.of_match_table = mpr_matches,
+	},
+};
+module_i2c_driver(mpr_driver);
+
+MODULE_AUTHOR("Andreas Klinger <ak@it-klinger.de>");
+MODULE_DESCRIPTION("Honeywell MPR series I2C driver");
+MODULE_LICENSE("GPL");
-- 
2.30.2

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH v3 3/3] MAINTAINERS: Add Honeywell mprls0025pa sensor
  2023-04-20 19:58 [PATCH v3 0/3] Support Honeywell mprls0025pa pressure sensor Andreas Klinger
  2023-04-20 20:00 ` [PATCH v3 1/3] dt-bindings: iio: pressure: Support Honeywell mprls0025pa sensor Andreas Klinger
  2023-04-20 20:00 ` [PATCH v3 2/3] iio: pressure: Honeywell mprls0025pa pressure sensor Andreas Klinger
@ 2023-04-20 20:01 ` Andreas Klinger
  2 siblings, 0 replies; 7+ messages in thread
From: Andreas Klinger @ 2023-04-20 20:01 UTC (permalink / raw)
  To: linux-iio, devicetree
  Cc: Jonathan Cameron, Lars-Peter Clausen, Rob Herring,
	Krzysztof Kozlowski, Angel Iglesias, linux-kernel

Add myself as a maintainer for Honeywell mprls0025pa sensor.

Signed-off-by: Andreas Klinger <ak@it-klinger.de>
---
 MAINTAINERS | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index f77188f30210..0f42f88f8959 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -9475,6 +9475,13 @@ F:	lib/test_hmm*
 F:	mm/hmm*
 F:	tools/testing/selftests/vm/*hmm*
 
+HONEYWELL MPRLS0025PA PRESSURE SENSOR SERIES IIO DRIVER
+M:	Andreas Klinger <ak@it-klinger.de>
+L:	linux-iio@vger.kernel.org
+S:	Maintained
+F:	Documentation/devicetree/bindings/iio/pressure/honeywell,mprls0025pa.yaml
+F:	drivers/iio/pressure/mprls0025pa.c
+
 HOST AP DRIVER
 M:	Jouni Malinen <j@w1.fi>
 L:	linux-wireless@vger.kernel.org
-- 
2.30.2

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH v3 2/3] iio: pressure: Honeywell mprls0025pa pressure sensor
  2023-04-20 20:00 ` [PATCH v3 2/3] iio: pressure: Honeywell mprls0025pa pressure sensor Andreas Klinger
@ 2023-04-20 22:04   ` kernel test robot
  2023-04-23 10:30   ` Jonathan Cameron
  1 sibling, 0 replies; 7+ messages in thread
From: kernel test robot @ 2023-04-20 22:04 UTC (permalink / raw)
  To: Andreas Klinger, linux-iio, devicetree
  Cc: oe-kbuild-all, Jonathan Cameron, Lars-Peter Clausen, Rob Herring,
	Krzysztof Kozlowski, Angel Iglesias, linux-kernel

Hi Andreas,

kernel test robot noticed the following build warnings:

[auto build test WARNING on e0ee50101346ca9cef52da75e3fb4380c27c042a]

url:    https://github.com/intel-lab-lkp/linux/commits/Andreas-Klinger/dt-bindings-iio-pressure-Support-Honeywell-mprls0025pa-sensor/20230421-040254
base:   e0ee50101346ca9cef52da75e3fb4380c27c042a
patch link:    https://lore.kernel.org/r/ZEGZ7VMrqaPNzhwj%40arbad
patch subject: [PATCH v3 2/3] iio: pressure: Honeywell mprls0025pa pressure sensor
config: sparc-allyesconfig (https://download.01.org/0day-ci/archive/20230421/202304210512.5qdqDRgd-lkp@intel.com/config)
compiler: sparc64-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/3d6a7eae49611392fb6f7563dcb71b74f12a0a78
        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/20230421-040254
        git checkout 3d6a7eae49611392fb6f7563dcb71b74f12a0a78
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=sparc olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=sparc SHELL=/bin/bash drivers/iio/pressure/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202304210512.5qdqDRgd-lkp@intel.com/

All warnings (new ones prefixed by >>):

   In file included from include/linux/device.h:15,
                    from include/linux/acpi.h:15,
                    from include/linux/i2c.h:13,
                    from drivers/iio/pressure/mprls0025pa.c:17:
   drivers/iio/pressure/mprls0025pa.c: In function 'mpr_read_pressure':
>> drivers/iio/pressure/mprls0025pa.c:169:30: warning: format '%d' expects argument of type 'int', but argument 4 has type 'long unsigned int' [-Wformat=]
     169 |                 dev_err(dev, "received size doesn't fit - ret: %d / %d\n", ret, sizeof(wdata));
         |                              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   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:169:17: note: in expansion of macro 'dev_err'
     169 |                 dev_err(dev, "received size doesn't fit - ret: %d / %d\n", ret, sizeof(wdata));
         |                 ^~~~~~~
   drivers/iio/pressure/mprls0025pa.c:169:70: note: format string is defined here
     169 |                 dev_err(dev, "received size doesn't fit - ret: %d / %d\n", ret, sizeof(wdata));
         |                                                                     ~^
         |                                                                      |
         |                                                                      int
         |                                                                     %ld
   drivers/iio/pressure/mprls0025pa.c:208:30: warning: format '%d' expects argument of type 'int', but argument 4 has type 'long unsigned int' [-Wformat=]
     208 |                 dev_err(dev, "received size doesn't fit - ret: %d / %d\n", ret, sizeof(buf));
         |                              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   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:208:17: note: in expansion of macro 'dev_err'
     208 |                 dev_err(dev, "received size doesn't fit - ret: %d / %d\n", ret, sizeof(buf));
         |                 ^~~~~~~
   drivers/iio/pressure/mprls0025pa.c:208:70: note: format string is defined here
     208 |                 dev_err(dev, "received size doesn't fit - ret: %d / %d\n", ret, sizeof(buf));
         |                                                                     ~^
         |                                                                      |
         |                                                                      int
         |                                                                     %ld


vim +169 drivers/iio/pressure/mprls0025pa.c

  > 17	#include <linux/i2c.h>
    18	#include <linux/module.h>
    19	#include <linux/mod_devicetable.h>
    20	#include <linux/device.h>
    21	#include <linux/gpio/consumer.h>
    22	#include <linux/regulator/consumer.h>
    23	#include <linux/math64.h>
    24	
    25	#include <asm/unaligned.h>
    26	
    27	#include <linux/iio/iio.h>
    28	#include <linux/iio/buffer.h>
    29	#include <linux/iio/triggered_buffer.h>
    30	#include <linux/iio/trigger_consumer.h>
    31	
    32	/* bits in i2c status byte */
    33	#define MPR_I2C_POWER	BIT(6)	/* device is powered */
    34	#define MPR_I2C_BUSY	BIT(5)	/* device is busy */
    35	#define MPR_I2C_MEMORY	BIT(2)	/* integrity test passed */
    36	#define MPR_I2C_MATH	BIT(0)	/* internal math saturation */
    37	
    38	#define MPR_NANO_PART	1000000000LL
    39	
    40	/*
    41	 * _INPUT interface:
    42	 * Calculation formular from the datasheet:
    43	 * pressure = (press_cnt - outputmin) * scale + pmin
    44	 * with:
    45	 * * pressure	- measured pressure in Pascal
    46	 * * press_cnt	- raw value read from sensor
    47	 * * pmin	- minimum pressure range value of sensor (data->pmin)
    48	 * * pmax	- maximum pressure range value of sensor (data->pmax)
    49	 * * outputmin	- minimum numerical range raw value delivered by sensor (MPR_OUT_MIN)
    50	 * * outputmax	- maximum numerical range raw value delivered by sensor (MPR_OUT_MAX)
    51	 * * scale	- (pmax - pmin) / (outputmax - outputmin)
    52	 *
    53	 * _RAW interface:
    54	 * pressure = (raw + offset) * scale
    55	 * --> need to adjust offset for fitting into _RAW interface
    56	 * Values for _RAW interface:
    57	 * * raw	- press_cnt
    58	 * * scale	- (pmax - pmin) / (outputmax - outputmin)
    59	 * * offset	- (-1 * outputmin) - pmin / scale
    60	 *                note: With all sensors from the datasheet pmin = 0 which reduces the offset to
    61	 *                (-1 * outputmin)
    62	 */
    63	
    64	/*
    65	 * transfer function A: 10%   to 90%   of 2^24
    66	 * transfer function B:  2.5% to 22.5% of 2^24
    67	 * transfer function C: 20%   to 80%   of 2^24
    68	 */
    69	enum mpr_func_id {
    70		MPR_FUNCTION_A,
    71		MPR_FUNCTION_B,
    72		MPR_FUNCTION_C,
    73	};
    74	
    75	struct mpr_func_spec {
    76		u32			output_min;
    77		u32			output_max;
    78	};
    79	
    80	static const struct mpr_func_spec mpr_func_spec[] = {
    81		[MPR_FUNCTION_A] = {.output_min = 1677722, .output_max = 15099494},
    82		[MPR_FUNCTION_B] = {.output_min =  419430, .output_max =  3774874},
    83		[MPR_FUNCTION_C] = {.output_min = 3355443, .output_max = 13421773},
    84	};
    85	
    86	struct mpr_chan {
    87		s32			pres;		/* pressure value */
    88		s64			ts;		/* timestamp */
    89	};
    90	
    91	struct mpr_data {
    92		struct i2c_client	*client;
    93		struct mutex		lock;		/* i2c transactions */
    94		u32			pmin;		/* minimal pressure in pascal */
    95		u32			pmax;		/* maximal pressure in pascal */
    96		u32			function;	/* transfer function */
    97		u32			outmin;		/* minimal numerical range raw value from sensor */
    98		u32			outmax;		/* maximal numerical range raw value from sensor */
    99		int                     scale;          /* int part of scale */
   100		int                     scale2;         /* nano part of scale */
   101		int                     offset;         /* int part of offset */
   102		int                     offset2;        /* nano part of offset */
   103		struct gpio_desc	*gpiod_reset;	/* reset */
   104		int			irq;		/* end of conversion interrupt */
   105		struct completion	completion;	/* handshake from irq to read */
   106		struct mpr_chan		chan;		/* channel values for buffered mode */
   107		struct regulator	*vdd;		/* optional external voltage regulator */
   108	};
   109	
   110	static const struct iio_chan_spec mpr_channels[] = {
   111		{
   112			.type = IIO_PRESSURE,
   113			.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
   114						BIT(IIO_CHAN_INFO_SCALE) |
   115						BIT(IIO_CHAN_INFO_OFFSET),
   116			.scan_index = 0,
   117			.scan_type = {
   118				.sign = 's',
   119				.realbits = 32,
   120				.storagebits = 32,
   121				.endianness = IIO_CPU,
   122			},
   123		},
   124		IIO_CHAN_SOFT_TIMESTAMP(1),
   125	};
   126	
   127	static void mpr_reset(struct mpr_data *data)
   128	{
   129		if (data->gpiod_reset) {
   130			gpiod_set_value(data->gpiod_reset, 0);
   131			udelay(10);
   132			gpiod_set_value(data->gpiod_reset, 1);
   133		}
   134	}
   135	
   136	/**
   137	 * mpr_read_pressure() - Read pressure value from sensor via I2C
   138	 * @data: Pointer to private data struct.
   139	 * @press: Output value read from sensor.
   140	 *
   141	 * Reading from the sensor by sending and receiving I2C telegrams.
   142	 *
   143	 * If there is an end of conversion (EOC) interrupt registered the function waits for a maximum of
   144	 * one second for the interrupt.
   145	 *
   146	 * Context: The function can sleep and data->lock should be held when calling it.
   147	 * Return:
   148	 * * 0		- OK, the pressure value could be read
   149	 * * -ETIMEDOUT	- Timeout while waiting for the EOC interrupt or busy flag is still set after nloops
   150	 *		  attempts of reading
   151	 */
   152	static int mpr_read_pressure(struct mpr_data *data, s32 *press)
   153	{
   154		struct device *dev = &data->client->dev;
   155		int ret, i;
   156		u8 wdata[] = {0xAA, 0x00, 0x00};
   157		s32 status;
   158		int nloops = 10;
   159		u8 buf[5];
   160	
   161		reinit_completion(&data->completion);
   162	
   163		ret = i2c_master_send(data->client, wdata, sizeof(wdata));
   164		if (ret < 0) {
   165			dev_err(dev, "error while writing ret: %d\n", ret);
   166			return ret;
   167		}
   168		if (ret != sizeof(wdata)) {
 > 169			dev_err(dev, "received size doesn't fit - ret: %d / %d\n", ret, sizeof(wdata));
   170			return -EIO;
   171		}
   172	
   173		if (data->irq > 0) {
   174			ret = wait_for_completion_timeout(&data->completion, HZ);
   175			if (!ret) {
   176				dev_err(dev, "timeout while waiting for eoc interrupt\n");
   177				return -ETIMEDOUT;
   178			}
   179		} else {
   180			/* wait until status indicates data is ready */
   181			for (i = 0; i < nloops; i++) {
   182				/*
   183				 * datasheet only says to wait at least 5 ms for the data but leave the
   184				 * maximum response time open
   185				 * --> let's try it nloops (10) times which seems to be quite long
   186				 */
   187				usleep_range(5000, 10000);
   188				status = i2c_smbus_read_byte(data->client);
   189				if (status < 0) {
   190					dev_err(dev, "error while reading, status: %d\n", status);
   191					return status;
   192				}
   193				if (!(status & MPR_I2C_BUSY))
   194					break;
   195			}
   196			if (i == nloops) {
   197				dev_err(dev, "timeout while reading\n");
   198				return -ETIMEDOUT;
   199			}
   200		}
   201	
   202		ret = i2c_master_recv(data->client, buf, sizeof(buf));
   203		if (ret < 0) {
   204			dev_err(dev, "error in i2c_master_recv ret: %d\n", ret);
   205			return ret;
   206		}
   207		if (ret != sizeof(buf)) {
   208			dev_err(dev, "received size doesn't fit - ret: %d / %d\n", ret, sizeof(buf));
   209			return -EIO;
   210		}
   211	
   212		if (buf[0] & MPR_I2C_BUSY) {
   213			/* it should never be the case that status still indicates business */
   214			dev_err(dev, "data still not ready: %08x\n", buf[0]);
   215			return -ETIMEDOUT;
   216		}
   217	
   218		*press = get_unaligned_be24(&buf[1]);
   219	
   220		dev_dbg(dev, "received: %*ph cnt: %d\n", ret, buf, *press);
   221	
   222		return 0;
   223	}
   224	

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v3 1/3] dt-bindings: iio: pressure: Support Honeywell mprls0025pa sensor
  2023-04-20 20:00 ` [PATCH v3 1/3] dt-bindings: iio: pressure: Support Honeywell mprls0025pa sensor Andreas Klinger
@ 2023-04-21  7:21   ` Krzysztof Kozlowski
  0 siblings, 0 replies; 7+ messages in thread
From: Krzysztof Kozlowski @ 2023-04-21  7:21 UTC (permalink / raw)
  To: Andreas Klinger, linux-iio, devicetree
  Cc: Jonathan Cameron, Lars-Peter Clausen, Rob Herring,
	Krzysztof Kozlowski, Angel Iglesias, linux-kernel

On 20/04/2023 22:00, Andreas Klinger wrote:
> Honeywell mpr 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 <ak@it-klinger.de>
> ---
>  .../iio/pressure/honeywell,mprls0025pa.yaml   | 98 +++++++++++++++++++
>  1 file changed, 98 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..80ab1beac7f6
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/pressure/honeywell,mprls0025pa.yaml
> @@ -0,0 +1,98 @@
> +# 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 mpr series pressure sensor
> +
> +maintainers:
> +  - Andreas Klinger <ak@it-klinger.de>
> +
> +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

This is a friendly reminder during the review process.

It seems my previous comments were not fully addressed. Maybe my
feedback got lost between the quotes, maybe you just forgot to apply it.
Please go back to the previous discussion and either implement all
requested changes or keep discussing them.

Thank you.


Best regards,
Krzysztof


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v3 2/3] iio: pressure: Honeywell mprls0025pa pressure sensor
  2023-04-20 20:00 ` [PATCH v3 2/3] iio: pressure: Honeywell mprls0025pa pressure sensor Andreas Klinger
  2023-04-20 22:04   ` kernel test robot
@ 2023-04-23 10:30   ` Jonathan Cameron
  1 sibling, 0 replies; 7+ messages in thread
From: Jonathan Cameron @ 2023-04-23 10:30 UTC (permalink / raw)
  To: Andreas Klinger
  Cc: linux-iio, devicetree, Lars-Peter Clausen, Rob Herring,
	Krzysztof Kozlowski, Angel Iglesias, linux-kernel

On Thu, 20 Apr 2023 22:00:45 +0200
Andreas Klinger <ak@it-klinger.de> wrote:

> Honeywell mprls0025pa is a series of pressure sensors.
> 
> Add initial I2C support.
> 
> Note:
> - Buffered mode is supported

Maybe clarify this comment more.  You support IIO buffered modes for
example, so I'm not immediately sure what this refers to.


> - SPI mode is not supported
> 
> Signed-off-by: Andreas Klinger <ak@it-klinger.de>

Various other comments inline.

Thanks,

Jonathan

> ---
>  drivers/iio/pressure/Kconfig       |  12 +
>  drivers/iio/pressure/Makefile      |   1 +
>  drivers/iio/pressure/mprls0025pa.c | 429 +++++++++++++++++++++++++++++
>  3 files changed, 442 insertions(+)
>  create mode 100644 drivers/iio/pressure/mprls0025pa.c
> 
> diff --git a/drivers/iio/pressure/Kconfig b/drivers/iio/pressure/Kconfig
> index c9453389e4f7..c51e53bd54bc 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 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 mpr.

Doesn't look like the module will have that name any more.

> +
>  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..3d82f6790e6d
> --- /dev/null
> +++ b/drivers/iio/pressure/mprls0025pa.c
> @@ -0,0 +1,429 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * MPR - MicroPressure pressure sensor series driver
> + *
> + * Copyright (c) Andreas Klinger <ak@it-klinger.de>
> + *
> + * Data sheet:
> + *  https://prod-edam.honeywell.com/content/dam/honeywell-edam/sps/siot/en-us/products/sensors/  \
> + *    pressure-sensors/board-mount-pressure-sensors/micropressure-mpr-series/documents/          \
> + *    sps-siot-mpr-series-datasheet-32332628-ciid-172626.pdf
> + *
> + * 7-bit I2C default slave address: 0x18
> + */
> +
> +#include <linux/property.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 <linux/regulator/consumer.h>
> +#include <linux/math64.h>

Trivial, but good to keep the includes in alphabetical order.

> +
> +#include <asm/unaligned.h>
> +
> +#include <linux/iio/iio.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/triggered_buffer.h>
> +#include <linux/iio/trigger_consumer.h>
> +
>
> +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 interrupt */
> +	struct completion	completion;	/* handshake from irq to read */
> +	struct mpr_chan		chan;		/* channel values for buffered mode */
> +	struct regulator	*vdd;		/* optional external voltage regulator */

It's unusual for something called vdd to be optional... see later.

> +};

> +/**
> + * 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

Very long lines.  Keep to 80 chars unless there is a strong readability reason to go up to < 100 chars.
Over that requires a very very strong reason (rarely done)

> + *		  attempts of reading
> + */

...

> +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;
> +	}
> +

You can't get here so drop this.

> +	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;
> +	s64 scale, offset;
> +
> +	dev_dbg(dev, "client: %s id: %s\n", client->name, id->name);

Unlikely to be much use after initial driver development. drop it.

> +
> +	if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_READ_BYTE))
> +		return dev_err_probe(dev, -EOPNOTSUPP, "I2C functionality not supported\n");

...

> +
> +	data->vdd = devm_regulator_get_optional(dev, "vdd");
> +	if (IS_ERR(data->vdd)) {
> +		dev_dbg(dev, "can't get vdd supply err: %pe\n", data->vdd);

This seems unusual.  Is VDD really optional, or are you trying to handle it
simply being always on or not provided by firmware? If the second case rely
on the stub regulators you'll get from
devm_regulator_get_enabled(dev, "vdd") to deal with that for you.

That should work however the driver was probed (I think anyway!)

> +		data->vdd = NULL;
> +	} else {
> +		ret = regulator_enable(data->vdd);
> +		if (ret)
> +			return dev_err_probe(dev, ret, "can't enable vdd supply\n");
> +		ret = devm_add_action_or_reset(dev, mpr_vdd_disable, data->vdd);
> +		if (ret)
> +			return dev_err_probe(dev, ret, "can't add vdd disable action\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 missing in DT\n");

Don't mention DT. Could be another type of firmware.  Also might not be missing, but instead
wrongly formatted.

"honeywell,pmin-pascal could not be read." perhaps?
similar for the others.


> +		ret = device_property_read_u32(dev, "honeywell,pmax-pascal", &data->pmax);
> +		if (ret)
> +			return dev_err_probe(dev, ret, "honeywell,pmax-pascal missing in DT\n");
> +		ret = device_property_read_u32(dev, "honeywell,transfer-function", &data->function);
> +		if (ret)
> +			return dev_err_probe(dev, ret,
> +						"honeywell,transfer-function missing in DT\n");
> +		if (data->function > MPR_FUNCTION_C)
> +			return dev_err_probe(dev, -EINVAL,
> +				"honeywell,transfer-function %d not supported\n", data->function);
> +
> +		if (data->irq > 0) {

This doesn't need to be in the dev_fwnode() protection. I'd move it out of here.

> +			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);
> +		}
> +	} else {
> +		/* when loaded as i2c device we need to use default values */
> +		dev_notice(dev, "no dt node 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;
> +
> +	data->gpiod_reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> +	if (IS_ERR(data->gpiod_reset)) {
If devm_gpiod_get_optional() returns an error that doesn't mean the gpio wasn't found,
but rather that it didn't work for some reason.

If that happens you should error out and fail the probe.  Something is broken and
carrying on papers over that which is rarely a good idea.

The not wired case is handled fine by data->gpiod_reset being set to NULL anyway.

> +		dev_dbg(dev, "didn't get reset-gpios: err=%pe\n", data->gpiod_reset);
> +		data->gpiod_reset = NULL;
> +	}


^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2023-04-23 10:15 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-04-20 19:58 [PATCH v3 0/3] Support Honeywell mprls0025pa pressure sensor Andreas Klinger
2023-04-20 20:00 ` [PATCH v3 1/3] dt-bindings: iio: pressure: Support Honeywell mprls0025pa sensor Andreas Klinger
2023-04-21  7:21   ` Krzysztof Kozlowski
2023-04-20 20:00 ` [PATCH v3 2/3] iio: pressure: Honeywell mprls0025pa pressure sensor Andreas Klinger
2023-04-20 22:04   ` kernel test robot
2023-04-23 10:30   ` Jonathan Cameron
2023-04-20 20:01 ` [PATCH v3 3/3] MAINTAINERS: Add Honeywell mprls0025pa sensor Andreas Klinger

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).