[PATCH v2 00/10] changes to mprls0025pa

[PATCH v2 00/10] changes to mprls0025pa

[Petre Rodan]

A number of fixes to the mprls0025pa driver:
 - an off-by-one initially caused by a typo in the bindings file
 - two error fields are never checked during sensor interaction
 - unsafe initialization if the driver is instantiated via sysfs
   and the bindings are missing

Quality of life changes:
 - a refactor that adds a pressure-triplet property which initializes
pmin-pascal and pmax-pascal just like in the hsc030pa driver.
The user only needs to extract a short string from the chip name
instead of looking up the chip in the datasheet, understand the
nomenclature, extract the measurement range and then convert all units
to pascals.

New feature:
 - SPI compatibility for Honeywell MPR sensors that require it.

Both binding and driver are backwards compatible.
Tested in I2C and SPI modes with two different sensors.
The refactor requires property function present in the togreg branch.

Datasheet: 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
Signed-off-by: Petre Rodan <petre.rodan@subdimension.ro>
Signed-off-by: Andreas Klinger <ak@it-klinger.de>
---
Petre Rodan (10):
  dt-bindings: iio: pressure: honeywell,mprls0025pa.yaml fix
  dt-bindings: iio: pressure: honeywell,mprls0025pa.yaml add
    pressure-triplet
  dt-bindings: iio: pressure: honeywell,mprls0025pa.yaml add spi bus
  iio: pressure: mprls0025pa.c fix off-by-one enum
  iio: pressure: mprls0025pa.c fix error flag check
  iio: pressure: mprls0025pa.c remove dangerous defaults
  iio: pressure: mprls0025pa.c whitespace cleanup
  iio: pressure: mprls0025pa.c refactor
  iio: pressure: mprls0025pa.c add triplet property
  iio: pressure: mprls0025pa.c add SPI driver

 .../iio/pressure/honeywell,mprls0025pa.yaml   |  97 ++++--
 MAINTAINERS                                   |   3 +-
 drivers/iio/pressure/Kconfig                  |  14 +-
 drivers/iio/pressure/Makefile                 |   2 +
 drivers/iio/pressure/mprls0025pa.c            | 308 +++++++++---------
 drivers/iio/pressure/mprls0025pa.h            | 100 ++++++
 drivers/iio/pressure/mprls0025pa_i2c.c        |  98 ++++++
 drivers/iio/pressure/mprls0025pa_spi.c        |  91 ++++++
 8 files changed, 539 insertions(+), 174 deletions(-)
 create mode 100644 drivers/iio/pressure/mprls0025pa.h
 create mode 100644 drivers/iio/pressure/mprls0025pa_i2c.c
 create mode 100644 drivers/iio/pressure/mprls0025pa_spi.c

--
2.41.0

[PATCH v2 01/10] dt-bindings: iio: pressure: honeywell,mprls0025pa.yaml fix

[Petre Rodan]

Define enum inside the honeywell,transfer-function property block.

Set the correct irq edge in the example block.
Based on the datasheet, in table 13 on page 11:
"End-of-conversion indicator: This pin is set high when a measurement
and calculation have been completed and the data is ready to be
clocked out"

Add description on End-of-conversion interrupt.

Signed-off-by: Petre Rodan <petre.rodan@subdimension.ro>
Signed-off-by: Andreas Klinger <ak@it-klinger.de>
---
 .../bindings/iio/pressure/honeywell,mprls0025pa.yaml       | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/iio/pressure/honeywell,mprls0025pa.yaml b/Documentation/devicetree/bindings/iio/pressure/honeywell,mprls0025pa.yaml
index d9e903fbfd99..84ced4e5a7da 100644
--- a/Documentation/devicetree/bindings/iio/pressure/honeywell,mprls0025pa.yaml
+++ b/Documentation/devicetree/bindings/iio/pressure/honeywell,mprls0025pa.yaml
@@ -42,6 +42,10 @@ properties:
     maxItems: 1

   interrupts:
+    description:
+      Optional interrupt for indicating End-of-conversion.
+      If not present, the driver loops for a while until the received status
+      byte indicates correct measurement.
     maxItems: 1

   reset-gpios:
@@ -65,6 +69,7 @@ properties:
       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)
+    enum: [1, 2, 3]
     $ref: /schemas/types.yaml#/definitions/uint32

   vdd-supply:
@@ -93,7 +98,7 @@ examples:
             reg = <0x18>;
             reset-gpios = <&gpio3 19 GPIO_ACTIVE_HIGH>;
             interrupt-parent = <&gpio3>;
-            interrupts = <21 IRQ_TYPE_EDGE_FALLING>;
+            interrupts = <21 IRQ_TYPE_EDGE_RISING>;
             honeywell,pmin-pascal = <0>;
             honeywell,pmax-pascal = <172369>;
             honeywell,transfer-function = <1>;
--
2.41.0

[PATCH v2 02/10] dt-bindings: iio: pressure: honeywell,mprls0025pa.yaml add pressure-triplet

[Petre Rodan]

Change order of properties in order for the end user to de-prioritize
pmin-pascal and pmax-pascal which are superseded by pressure-triplet.

Add pressure-triplet property which automatically initializes
pmin-pascal and pmax-pascal inside the driver

Rework honeywell,pmXX-pascal requirements based on feedback from
Jonathan and Conor.

Signed-off-by: Petre Rodan <petre.rodan@subdimension.ro>
Signed-off-by: Andreas Klinger <ak@it-klinger.de>
---
 .../iio/pressure/honeywell,mprls0025pa.yaml   | 64 ++++++++++++++-----
 1 file changed, 47 insertions(+), 17 deletions(-)

diff --git a/Documentation/devicetree/bindings/iio/pressure/honeywell,mprls0025pa.yaml b/Documentation/devicetree/bindings/iio/pressure/honeywell,mprls0025pa.yaml
index 84ced4e5a7da..e4021306d187 100644
--- a/Documentation/devicetree/bindings/iio/pressure/honeywell,mprls0025pa.yaml
+++ b/Documentation/devicetree/bindings/iio/pressure/honeywell,mprls0025pa.yaml
@@ -19,14 +19,17 @@ description: |
   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
+  To support different models one need to specify its pressure triplet as well
+  as the transfer function.
+
+  For custom models the pressure values can alternatively be specified manually.
+  The minimal range value stands for the minimum pressure and the maximum value
+  also for the maximum pressure with linear relation inside the range.
+  Pressure range needs to be converted from the datasheet specified 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.
+  sensor.

   Specifications about the devices can be found at:
     https://prod-edam.honeywell.com/content/dam/honeywell-edam/sps/siot/en-us/
@@ -54,14 +57,6 @@ properties:
       If not present the device is not reset during the probe.
     maxItems: 1

-  honeywell,pmin-pascal:
-    description:
-      Minimum pressure value the sensor can measure in pascal.
-
-  honeywell,pmax-pascal:
-    description:
-      Maximum pressure value the sensor can measure in pascal.
-
   honeywell,transfer-function:
     description: |
       Transfer function which defines the range of valid values delivered by the
@@ -72,17 +67,52 @@ properties:
     enum: [1, 2, 3]
     $ref: /schemas/types.yaml#/definitions/uint32

+  honeywell,pressure-triplet:
+    description: |
+      Case-sensitive five character string that defines pressure range, unit
+      and type as part of the device nomenclature. In the unlikely case of a
+      custom chip, unset and provide pmin-pascal and pmax-pascal instead.
+    enum: [0001BA, 01.6BA, 02.5BA, 0060MG, 0100MG, 0160MG, 0250MG, 0400MG,
+           0600MG, 0001BG, 01.6BG, 02.5BG, 0100KA, 0160KA, 0250KA, 0006KG,
+           0010KG, 0016KG, 0025KG, 0040KG, 0060KG, 0100KG, 0160KG, 0250KG,
+           0015PA, 0025PA, 0030PA, 0001PG, 0005PG, 0015PG, 0030PG, 0300YG]
+    $ref: /schemas/types.yaml#/definitions/string
+
+  honeywell,pmin-pascal:
+    description:
+      Minimum pressure value the sensor can measure in pascal.
+      To be specified only if honeywell,pressure-triplet is not set.
+
+  honeywell,pmax-pascal:
+    description:
+      Maximum pressure value the sensor can measure in pascal.
+      To be specified only if honeywell,pressure-triplet is not set.
+
   vdd-supply:
     description: provide VDD power to the sensor.

 required:
   - compatible
   - reg
-  - honeywell,pmin-pascal
-  - honeywell,pmax-pascal
   - honeywell,transfer-function
   - vdd-supply

+oneOf:
+  - required:
+      - honeywell,pmin-pascal
+      - honeywell,pmax-pascal
+  - required:
+      - honeywell,pressure-triplet
+
+allOf:
+  - if:
+      required:
+        - honeywell,pressure-triplet
+    then:
+      properties:
+        honeywell,pmin-pascal: false
+        honeywell,pmax-pascal: false
+
 additionalProperties: false

 examples:
@@ -99,8 +129,8 @@ examples:
             reset-gpios = <&gpio3 19 GPIO_ACTIVE_HIGH>;
             interrupt-parent = <&gpio3>;
             interrupts = <21 IRQ_TYPE_EDGE_RISING>;
-            honeywell,pmin-pascal = <0>;
-            honeywell,pmax-pascal = <172369>;
+
+            honeywell,pressure-triplet = "0025PA";
             honeywell,transfer-function = <1>;
             vdd-supply = <&vcc_3v3>;
         };
--
2.41.0

[PATCH v2 03/10] dt-bindings: iio: pressure: honeywell,mprls0025pa.yaml add spi bus

[Petre Rodan]

Add spi based example.

Add spi-max-frequency property required by chip specifications.

Add additional maintainer.

Signed-off-by: Petre Rodan <petre.rodan@subdimension.ro>
Signed-off-by: Andreas Klinger <ak@it-klinger.de>
---
 .../iio/pressure/honeywell,mprls0025pa.yaml   | 26 +++++++++++++++++--
 1 file changed, 24 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/iio/pressure/honeywell,mprls0025pa.yaml b/Documentation/devicetree/bindings/iio/pressure/honeywell,mprls0025pa.yaml
index e4021306d187..430496b047c7 100644
--- a/Documentation/devicetree/bindings/iio/pressure/honeywell,mprls0025pa.yaml
+++ b/Documentation/devicetree/bindings/iio/pressure/honeywell,mprls0025pa.yaml
@@ -8,12 +8,12 @@ title: Honeywell mprls0025pa pressure sensor

 maintainers:
   - Andreas Klinger <ak@it-klinger.de>
+  - Petre Rodan <petre.rodan@subdimension.ro>

 description: |
   Honeywell pressure sensor of model mprls0025pa.

-  This sensor has an I2C and SPI interface. Only the I2C interface is
-  implemented.
+  This sensor has an I2C and SPI interface. Both are supported.

   There are many models with different pressure ranges available. The vendor
   calls them "mpr series". All of them have the identical programming model and
@@ -88,6 +88,9 @@ properties:
       Maximum pressure value the sensor can measure in pascal.
       To be specified only if honeywell,pressure-triplet is not set.

+  spi-max-frequency:
+    maximum: 800000
+
   vdd-supply:
     description: provide VDD power to the sensor.

@@ -135,3 +138,22 @@ examples:
             vdd-supply = <&vcc_3v3>;
         };
     };
+  - |
+    spi {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        pressure@0 {
+            compatible = "honeywell,mprls0025pa";
+            reg = <0>;
+            spi-max-frequency = <800000>;
+            reset-gpios = <&gpio1 28 GPIO_ACTIVE_HIGH>;
+            interrupt-parent = <&gpio0>;
+            interrupts = <30 IRQ_TYPE_EDGE_RISING>;
+
+            honeywell,pressure-triplet = "0015PA";
+            honeywell,transfer-function = <1>;
+            vdd-supply = <&vcc_3v3>;
+        };
+    };
+...
--
2.41.0

[PATCH v2 04/10] iio: pressure: mprls0025pa.c fix off-by-one enum

[Petre Rodan]

Fix off-by-one error in transfer-function property.
The honeywell,transfer-function property takes values between 1-3 so
make sure the proper enum gets used.

Signed-off-by: Petre Rodan <petre.rodan@subdimension.ro>
Signed-off-by: Andreas Klinger <ak@it-klinger.de>
---
 drivers/iio/pressure/mprls0025pa.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/pressure/mprls0025pa.c b/drivers/iio/pressure/mprls0025pa.c
index 30fb2de36821..e3f0de020a40 100644
--- a/drivers/iio/pressure/mprls0025pa.c
+++ b/drivers/iio/pressure/mprls0025pa.c
@@ -323,6 +323,7 @@ static int mpr_probe(struct i2c_client *client)
 	struct iio_dev *indio_dev;
 	struct device *dev = &client->dev;
 	s64 scale, offset;
+	u32 func;

 	if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_READ_BYTE))
 		return dev_err_probe(dev, -EOPNOTSUPP,
@@ -362,10 +363,11 @@ static int mpr_probe(struct i2c_client *client)
 			return dev_err_probe(dev, ret,
 				"honeywell,pmax-pascal could not be read\n");
 		ret = device_property_read_u32(dev,
-				"honeywell,transfer-function", &data->function);
+				"honeywell,transfer-function", &func);
 		if (ret)
 			return dev_err_probe(dev, ret,
 				"honeywell,transfer-function could not be read\n");
+		data->function = func - 1;
 		if (data->function > MPR_FUNCTION_C)
 			return dev_err_probe(dev, -EINVAL,
 				"honeywell,transfer-function %d invalid\n",
--
2.41.0

[PATCH v2 05/10] iio: pressure: mprls0025pa.c fix error flag check

[Petre Rodan]

Take into account all 3 error flags while interacting with the sensor.
Based on the datasheet, in table 14 on page 14, the status byte
contains:
    bit 5 busy flag - 1 if device is busy
    bit 2 memory integrity/error flag - 1 if integrity test failed
    bit 0 math saturation - 1 if internal math saturation has occurred

Signed-off-by: Petre Rodan <petre.rodan@subdimension.ro>
Signed-off-by: Andreas Klinger <ak@it-klinger.de>
---
 drivers/iio/pressure/mprls0025pa.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/pressure/mprls0025pa.c b/drivers/iio/pressure/mprls0025pa.c
index e3f0de020a40..233cc1dc38ad 100644
--- a/drivers/iio/pressure/mprls0025pa.c
+++ b/drivers/iio/pressure/mprls0025pa.c
@@ -39,6 +39,8 @@
 #define MPR_I2C_MEMORY	BIT(2)	/* integrity test passed */
 #define MPR_I2C_MATH	BIT(0)	/* internal math saturation */

+#define MPR_I2C_ERR_FLAG  (MPR_I2C_BUSY | MPR_I2C_MEMORY | MPR_I2C_MATH)
+
 /*
  * support _RAW sysfs interface:
  *
@@ -213,7 +215,7 @@ static int mpr_read_pressure(struct mpr_data *data, s32 *press)
 					status);
 				return status;
 			}
-			if (!(status & MPR_I2C_BUSY))
+			if (!(status & MPR_I2C_ERR_FLAG))
 				break;
 		}
 		if (i == nloops) {
@@ -233,7 +235,7 @@ static int mpr_read_pressure(struct mpr_data *data, s32 *press)
 		return -EIO;
 	}

-	if (buf[0] & MPR_I2C_BUSY) {
+	if (buf[0] & MPR_I2C_ERR_FLAG) {
 		/*
 		 * it should never be the case that status still indicates
 		 * business
--
2.41.0

[PATCH v2 06/10] iio: pressure: mprls0025pa.c remove dangerous defaults

[Petre Rodan]

This driver supports 32*3 combinations of fixed ranges and transfer
functions, plus custom ranges.

So statistically a user has more than 99% chance that the provided
default configuration will generate invalid pressure readings if the
bindings are not initialized and the driver is instantiated via sysfs.

The current patch removes this loophole making sure the driver loads
only if the dt has been initialized.

Signed-off-by: Petre Rodan <petre.rodan@subdimension.ro>
Signed-off-by: Andreas Klinger <ak@it-klinger.de>
---
 drivers/iio/pressure/mprls0025pa.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/pressure/mprls0025pa.c b/drivers/iio/pressure/mprls0025pa.c
index 233cc1dc38ad..63c46592956f 100644
--- a/drivers/iio/pressure/mprls0025pa.c
+++ b/drivers/iio/pressure/mprls0025pa.c
@@ -375,11 +375,8 @@ static int mpr_probe(struct i2c_client *client)
 				"honeywell,transfer-function %d invalid\n",
 								data->function);
 	} else {
-		/* when loaded as i2c device we need to use default values */
-		dev_notice(dev, "firmware node not found; using defaults\n");
-		data->pmin = 0;
-		data->pmax = 172369; /* 25 psi */
-		data->function = MPR_FUNCTION_A;
+		return dev_err_probe(dev, -EINVAL,
+				  "driver needs to be initialized in the dt\n");
 	}

 	data->outmin = mpr_func_spec[data->function].output_min;
--
2.41.0

[PATCH v2 07/10] iio: pressure: mprls0025pa.c whitespace cleanup

[Petre Rodan]

Fix indentation and whitespace in code that will not get refactored.

Make URL inside comment copy-paste friendly.

Signed-off-by: Petre Rodan <petre.rodan@subdimension.ro>
Signed-off-by: Andreas Klinger <ak@it-klinger.de>
---
 drivers/iio/pressure/mprls0025pa.c | 38 ++++++++++++++----------------
 1 file changed, 18 insertions(+), 20 deletions(-)

diff --git a/drivers/iio/pressure/mprls0025pa.c b/drivers/iio/pressure/mprls0025pa.c
index 63c46592956f..e14cdee7989f 100644
--- a/drivers/iio/pressure/mprls0025pa.c
+++ b/drivers/iio/pressure/mprls0025pa.c
@@ -5,10 +5,7 @@
  * 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
+ *  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
  */
@@ -84,9 +81,9 @@ struct mpr_func_spec {
 };

 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},
+	[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 {
@@ -273,7 +270,7 @@ static irqreturn_t mpr_trigger_handler(int irq, void *p)
 		goto err;

 	iio_push_to_buffers_with_timestamp(indio_dev, &data->chan,
-						iio_get_time_ns(indio_dev));
+					   iio_get_time_ns(indio_dev));

 err:
 	mutex_unlock(&data->lock);
@@ -355,15 +352,16 @@ static int mpr_probe(struct i2c_client *client)

 	if (dev_fwnode(dev)) {
 		ret = device_property_read_u32(dev, "honeywell,pmin-pascal",
-								&data->pmin);
+					       &data->pmin);
 		if (ret)
 			return dev_err_probe(dev, ret,
-				"honeywell,pmin-pascal could not be read\n");
+				   "honeywell,pmin-pascal could not be read\n");
+
 		ret = device_property_read_u32(dev, "honeywell,pmax-pascal",
-								&data->pmax);
+					       &data->pmax);
 		if (ret)
 			return dev_err_probe(dev, ret,
-				"honeywell,pmax-pascal could not be read\n");
+				   "honeywell,pmax-pascal could not be read\n");
 		ret = device_property_read_u32(dev,
 				"honeywell,transfer-function", &func);
 		if (ret)
@@ -384,14 +382,14 @@ static int mpr_probe(struct i2c_client *client)

 	/* use 64 bit calculation for preserving a reasonable precision */
 	scale = div_s64(((s64)(data->pmax - data->pmin)) * NANO,
-						data->outmax - data->outmin);
+			data->outmax - data->outmin);
 	data->scale = div_s64_rem(scale, NANO, &data->scale2);
 	/*
 	 * multiply with NANO before dividing by scale and later divide by NANO
 	 * again.
 	 */
 	offset = ((-1LL) * (s64)data->outmin) * NANO -
-			div_s64(div_s64((s64)data->pmin * NANO, scale), NANO);
+		  div_s64(div_s64((s64)data->pmin * NANO, scale), NANO);
 	data->offset = div_s64_rem(offset, NANO, &data->offset2);

 	if (data->irq > 0) {
@@ -399,27 +397,27 @@ static int mpr_probe(struct i2c_client *client)
 				IRQF_TRIGGER_RISING, client->name, data);
 		if (ret)
 			return dev_err_probe(dev, ret,
-				"request irq %d failed\n", data->irq);
+					  "request irq %d failed\n", data->irq);
 	}

 	data->gpiod_reset = devm_gpiod_get_optional(dev, "reset",
-							GPIOD_OUT_HIGH);
+						    GPIOD_OUT_HIGH);
 	if (IS_ERR(data->gpiod_reset))
 		return dev_err_probe(dev, PTR_ERR(data->gpiod_reset),
-						"request reset-gpio failed\n");
+				     "request reset-gpio failed\n");

 	mpr_reset(data);

 	ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL,
-						mpr_trigger_handler, NULL);
+					      mpr_trigger_handler, NULL);
 	if (ret)
 		return dev_err_probe(dev, ret,
-					"iio triggered buffer setup failed\n");
+				     "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");
+				     "unable to register iio device\n");

 	return 0;
 }
--
2.41.0

[PATCH v2 08/10] iio: pressure: mprls0025pa.c refactor

[Petre Rodan]

Refactor driver by splitting the code into core and i2c.

Seemingly redundant read/write function parameters are required for
compatibility with the SPI driver.

Signed-off-by: Petre Rodan <petre.rodan@subdimension.ro>
Signed-off-by: Andreas Klinger <ak@it-klinger.de>
---
 MAINTAINERS                            |   3 +-
 drivers/iio/pressure/Kconfig           |   6 +
 drivers/iio/pressure/Makefile          |   1 +
 drivers/iio/pressure/mprls0025pa.c     | 203 ++++++++-----------------
 drivers/iio/pressure/mprls0025pa.h     | 100 ++++++++++++
 drivers/iio/pressure/mprls0025pa_i2c.c |  98 ++++++++++++
 6 files changed, 267 insertions(+), 144 deletions(-)
 create mode 100644 drivers/iio/pressure/mprls0025pa.h
 create mode 100644 drivers/iio/pressure/mprls0025pa_i2c.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 3029841e92a8..cbb163e1b311 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -9725,10 +9725,11 @@ F:	drivers/iio/pressure/hsc030pa*

 HONEYWELL MPRLS0025PA PRESSURE SENSOR SERIES IIO DRIVER
 M:	Andreas Klinger <ak@it-klinger.de>
+M:	Petre Rodan <petre.rodan@subdimension.ro>
 L:	linux-iio@vger.kernel.org
 S:	Maintained
 F:	Documentation/devicetree/bindings/iio/pressure/honeywell,mprls0025pa.yaml
-F:	drivers/iio/pressure/mprls0025pa.c
+F:	drivers/iio/pressure/mprls0025pa*

 HOST AP DRIVER
 L:	linux-wireless@vger.kernel.org
diff --git a/drivers/iio/pressure/Kconfig b/drivers/iio/pressure/Kconfig
index 79adfd059c3a..f03007cfec85 100644
--- a/drivers/iio/pressure/Kconfig
+++ b/drivers/iio/pressure/Kconfig
@@ -182,6 +182,7 @@ config MPL3115
 config MPRLS0025PA
 	tristate "Honeywell MPRLS0025PA (MicroPressure sensors series)"
 	depends on I2C
+	select MPRLS0025PA_I2C if I2C
 	select IIO_BUFFER
 	select IIO_TRIGGERED_BUFFER
 	help
@@ -192,6 +193,11 @@ config MPRLS0025PA
 	  To compile this driver as a module, choose M here: the module will be
 	  called mprls0025pa.

+config MPRLS0025PA_I2C
+	tristate
+	depends on MPRLS0025PA
+	depends on I2C
+
 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 b0f8b94662f2..7754135e190c 100644
--- a/drivers/iio/pressure/Makefile
+++ b/drivers/iio/pressure/Makefile
@@ -24,6 +24,7 @@ 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_MPRLS0025PA_I2C) += mprls0025pa_i2c.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
index e14cdee7989f..cb5d6c0cca7e 100644
--- a/drivers/iio/pressure/mprls0025pa.c
+++ b/drivers/iio/pressure/mprls0025pa.c
@@ -7,12 +7,11 @@
  * 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/delay.h>
-#include <linux/device.h>
-#include <linux/i2c.h>
+#include <linux/array_size.h>
+#include <linux/bitfield.h>
+#include <linux/bits.h>
 #include <linux/math64.h>
 #include <linux/mod_devicetable.h>
 #include <linux/module.h>
@@ -30,13 +29,15 @@

 #include <asm/unaligned.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 */
+#include "mprls0025pa.h"

-#define MPR_I2C_ERR_FLAG  (MPR_I2C_BUSY | MPR_I2C_MEMORY | MPR_I2C_MATH)
+/* bits in status byte */
+#define MPR_ST_POWER  BIT(6) /* device is powered */
+#define MPR_ST_BUSY   BIT(5) /* device is busy */
+#define MPR_ST_MEMORY BIT(2) /* integrity test passed */
+#define MPR_ST_MATH   BIT(0) /* internal math saturation */
+
+#define MPR_ST_ERR_FLAG  (MPR_ST_BUSY | MPR_ST_MEMORY | MPR_ST_MATH)

 /*
  * support _RAW sysfs interface:
@@ -69,12 +70,6 @@
  * 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;
@@ -86,45 +81,6 @@ static const struct mpr_func_spec mpr_func_spec[] = {
 	[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;		/*
-						 * access to device during read
-						 */
-	u32			pmin;		/* minimal pressure in pascal */
-	u32			pmax;		/* maximal pressure in pascal */
-	enum mpr_func_id	function;	/* transfer function */
-	u32			outmin;		/*
-						 * minimal numerical range raw
-						 * value from sensor
-						 */
-	u32			outmax;		/*
-						 * maximal numerical range raw
-						 * value from sensor
-						 */
-	int                     scale;          /* int part of scale */
-	int                     scale2;         /* nano part of scale */
-	int                     offset;         /* int part of offset */
-	int                     offset2;        /* nano part of offset */
-	struct gpio_desc	*gpiod_reset;	/* reset */
-	int			irq;		/*
-						 * end of conversion irq;
-						 * used to distinguish between
-						 * irq mode and reading in a
-						 * loop until data is ready
-						 */
-	struct completion	completion;	/* handshake from irq to read */
-	struct mpr_chan		chan;		/*
-						 * channel values for buffered
-						 * mode
-						 */
-};
-
 static const struct iio_chan_spec mpr_channels[] = {
 	{
 		.type = IIO_PRESSURE,
@@ -152,11 +108,11 @@ static void mpr_reset(struct mpr_data *data)
 }

 /**
- * mpr_read_pressure() - Read pressure value from sensor via I2C
+ * mpr_read_pressure() - Read pressure value from sensor
  * @data: Pointer to private data struct.
  * @press: Output value read from sensor.
  *
- * Reading from the sensor by sending and receiving I2C telegrams.
+ * Reading from the sensor by sending and receiving telegrams.
  *
  * If there is an end of conversion (EOC) interrupt registered the function
  * waits for a maximum of one second for the interrupt.
@@ -169,25 +125,17 @@ static void mpr_reset(struct mpr_data *data)
  */
 static int mpr_read_pressure(struct mpr_data *data, s32 *press)
 {
-	struct device *dev = &data->client->dev;
+	struct device *dev = data->dev;
 	int ret, i;
-	u8 wdata[] = {0xAA, 0x00, 0x00};
-	s32 status;
 	int nloops = 10;
-	u8 buf[4];

 	reinit_completion(&data->completion);

-	ret = i2c_master_send(data->client, wdata, sizeof(wdata));
+	ret = data->ops->write(data, MPR_CMD_SYNC, MPR_PKT_SYNC_LEN);
 	if (ret < 0) {
 		dev_err(dev, "error while writing ret: %d\n", ret);
 		return ret;
 	}
-	if (ret != sizeof(wdata)) {
-		dev_err(dev, "received size doesn't fit - ret: %d / %u\n", ret,
-							(u32)sizeof(wdata));
-		return -EIO;
-	}

 	if (data->irq > 0) {
 		ret = wait_for_completion_timeout(&data->completion, HZ);
@@ -205,14 +153,14 @@ static int mpr_read_pressure(struct mpr_data *data, s32 *press)
 			 *     quite long
 			 */
 			usleep_range(5000, 10000);
-			status = i2c_smbus_read_byte(data->client);
-			if (status < 0) {
+			ret = data->ops->read(data, MPR_CMD_NOP, 1);
+			if (ret < 0) {
 				dev_err(dev,
 					"error while reading, status: %d\n",
-					status);
-				return status;
+					ret);
+				return ret;
 			}
-			if (!(status & MPR_I2C_ERR_FLAG))
+			if (!(data->buffer[0] & MPR_ST_ERR_FLAG))
 				break;
 		}
 		if (i == nloops) {
@@ -221,29 +169,19 @@ static int mpr_read_pressure(struct mpr_data *data, s32 *press)
 		}
 	}

-	ret = i2c_master_recv(data->client, buf, sizeof(buf));
-	if (ret < 0) {
-		dev_err(dev, "error in i2c_master_recv ret: %d\n", ret);
+	ret = data->ops->read(data, MPR_CMD_NOP, MPR_PKT_NOP_LEN);
+	if (ret < 0)
 		return ret;
-	}
-	if (ret != sizeof(buf)) {
-		dev_err(dev, "received size doesn't fit - ret: %d / %u\n", ret,
-							(u32)sizeof(buf));
-		return -EIO;
-	}

-	if (buf[0] & MPR_I2C_ERR_FLAG) {
-		/*
-		 * it should never be the case that status still indicates
-		 * business
-		 */
-		dev_err(dev, "data still not ready: %08x\n", buf[0]);
+	if (data->buffer[0] & MPR_ST_ERR_FLAG) {
+		dev_err(data->dev,
+			"unexpected status byte %02x\n", data->buffer[0]);
 		return -ETIMEDOUT;
 	}

-	*press = get_unaligned_be24(&buf[1]);
+	*press = get_unaligned_be24(&data->buffer[1]);

-	dev_dbg(dev, "received: %*ph cnt: %d\n", ret, buf, *press);
+	dev_dbg(dev, "received: %*ph cnt: %d\n", ret, data->buffer, *press);

 	return 0;
 }
@@ -315,26 +253,22 @@ static const struct iio_info mpr_info = {
 	.read_raw = &mpr_read_raw,
 };

-static int mpr_probe(struct i2c_client *client)
+int mpr_common_probe(struct device *dev, const struct mpr_ops *ops, int irq)
 {
 	int ret;
 	struct mpr_data *data;
 	struct iio_dev *indio_dev;
-	struct device *dev = &client->dev;
 	s64 scale, offset;
 	u32 func;

-	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");
+		return -ENOMEM;

 	data = iio_priv(indio_dev);
-	data->client = client;
-	data->irq = client->irq;
+	data->dev = dev;
+	data->ops = ops;
+	data->irq = irq;

 	mutex_init(&data->lock);
 	init_completion(&data->completion);
@@ -350,32 +284,36 @@ static int mpr_probe(struct i2c_client *client)
 		return dev_err_probe(dev, ret,
 				"can't get and enable vdd supply\n");

-	if (dev_fwnode(dev)) {
-		ret = device_property_read_u32(dev, "honeywell,pmin-pascal",
+	ret = data->ops->init(data->dev);
+	if (ret)
+		return ret;
+
+	ret = device_property_read_u32(dev,
+				       "honeywell,transfer-function", &func);
+	if (ret)
+		return dev_err_probe(dev, ret,
+			     "honeywell,transfer-function could not be read\n");
+	data->function = func - 1;
+	if (data->function > MPR_FUNCTION_C)
+		return dev_err_probe(dev, -EINVAL,
+				     "honeywell,transfer-function %d invalid\n",
+				     data->function);
+
+	ret = device_property_read_u32(dev, "honeywell,pmin-pascal",
 					       &data->pmin);
-		if (ret)
-			return dev_err_probe(dev, ret,
+	if (ret)
+		return dev_err_probe(dev, ret,
 				   "honeywell,pmin-pascal could not be read\n");

-		ret = device_property_read_u32(dev, "honeywell,pmax-pascal",
-					       &data->pmax);
-		if (ret)
-			return dev_err_probe(dev, ret,
+	ret = device_property_read_u32(dev, "honeywell,pmax-pascal",
+				       &data->pmax);
+	if (ret)
+		return dev_err_probe(dev, ret,
 				   "honeywell,pmax-pascal could not be read\n");
-		ret = device_property_read_u32(dev,
-				"honeywell,transfer-function", &func);
-		if (ret)
-			return dev_err_probe(dev, ret,
-				"honeywell,transfer-function could not be read\n");
-		data->function = func - 1;
-		if (data->function > MPR_FUNCTION_C)
-			return dev_err_probe(dev, -EINVAL,
-				"honeywell,transfer-function %d invalid\n",
-								data->function);
-	} else {
+
+	if (data->pmin >= data->pmax)
 		return dev_err_probe(dev, -EINVAL,
-				  "driver needs to be initialized in the dt\n");
-	}
+				     "pressure limits are invalid\n");

 	data->outmin = mpr_func_spec[data->function].output_min;
 	data->outmax = mpr_func_spec[data->function].output_max;
@@ -394,7 +332,7 @@ static int mpr_probe(struct i2c_client *client)

 	if (data->irq > 0) {
 		ret = devm_request_irq(dev, data->irq, mpr_eoc_handler,
-				IRQF_TRIGGER_RISING, client->name, data);
+				      IRQF_TRIGGER_RISING, dev_name(dev), data);
 		if (ret)
 			return dev_err_probe(dev, ret,
 					  "request irq %d failed\n", data->irq);
@@ -421,29 +359,8 @@ static int mpr_probe(struct i2c_client *client)

 	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);
+EXPORT_SYMBOL_NS(mpr_common_probe, IIO_HONEYWELL_MPRLS0025PA);

 MODULE_AUTHOR("Andreas Klinger <ak@it-klinger.de>");
-MODULE_DESCRIPTION("Honeywell MPRLS0025PA I2C driver");
+MODULE_DESCRIPTION("Honeywell MPR pressure sensor core driver");
 MODULE_LICENSE("GPL");
diff --git a/drivers/iio/pressure/mprls0025pa.h b/drivers/iio/pressure/mprls0025pa.h
new file mode 100644
index 000000000000..7d7bd21bda95
--- /dev/null
+++ b/drivers/iio/pressure/mprls0025pa.h
@@ -0,0 +1,100 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * MPRLS0025PA - Honeywell MicroPressure pressure sensor series driver
+ *
+ * Copyright (c) Andreas Klinger <ak@it-klinger.de>
+ *
+ * Data sheet:
+ *  https://prod-edam.honeywell.com/content/dam/honeywell-edam/sps/siot/en-us/products/sensors/pressure-sensors/board-mount-pressure-sensors/micropressure-mpr-series/documents/sps-siot-mpr-series-datasheet-32332628-ciid-172626.pdf
+ */
+
+#ifndef _MPRLS0025PA_H
+#define _MPRLS0025PA_H
+
+#include <linux/completion.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/mutex.h>
+#include <linux/stddef.h>
+#include <linux/types.h>
+
+#define MPR_MEASUREMENT_RD_SIZE 4
+#define MPR_CMD_NOP      0xf0
+#define MPR_CMD_SYNC     0xaa
+#define MPR_PKT_NOP_LEN  MPR_MEASUREMENT_RD_SIZE
+#define MPR_PKT_SYNC_LEN 3
+
+struct device;
+
+struct iio_chan_spec;
+struct iio_dev;
+
+struct mpr_data;
+struct mpr_ops;
+
+/**
+ * struct mpr_chan
+ * @pres: pressure value
+ * @ts: timestamp
+ */
+struct mpr_chan {
+	s32 pres;
+	s64 ts;
+};
+
+enum mpr_func_id {
+	MPR_FUNCTION_A,
+	MPR_FUNCTION_B,
+	MPR_FUNCTION_C,
+};
+
+/**
+ * struct mpr_data
+ * @dev: current device structure
+ * @ops: functions that implement the sensor reads/writes, bus init
+ * @lock: access to device during read
+ * @pmin: minimal pressure in pascal
+ * @pmax: maximal pressure in pascal
+ * @function: transfer function
+ * @outmin: minimum raw pressure in counts (based on transfer function)
+ * @outmax: maximum raw pressure in counts (based on transfer function)
+ * @scale: pressure scale
+ * @scale2: pressure scale, decimal number
+ * @offset: pressure offset
+ * @offset2: pressure offset, decimal number
+ * @gpiod_reset: reset
+ * @irq: end of conversion irq. used to distinguish between irq mode and
+ *       reading in a loop until data is ready
+ * @completion: handshake from irq to read
+ * @chan: channel values for buffered mode
+ * @buffer: raw conversion data
+ */
+struct mpr_data {
+	struct device		*dev;
+	const struct mpr_ops	*ops;
+	struct mutex		lock;
+	u32			pmin;
+	u32			pmax;
+	enum mpr_func_id	function;
+	u32			outmin;
+	u32			outmax;
+	int			scale;
+	int			scale2;
+	int			offset;
+	int			offset2;
+	struct gpio_desc	*gpiod_reset;
+	int			irq;
+	struct completion	completion;
+	struct mpr_chan		chan;
+	u8	    buffer[MPR_MEASUREMENT_RD_SIZE] __aligned(IIO_DMA_MINALIGN);
+};
+
+struct mpr_ops {
+	int (*init)(struct device *);
+	int (*read)(struct mpr_data *, const u8, const u8);
+	int (*write)(struct mpr_data *, const u8, const u8);
+};
+
+int mpr_common_probe(struct device *dev, const struct mpr_ops *ops, int irq);
+
+#endif
diff --git a/drivers/iio/pressure/mprls0025pa_i2c.c b/drivers/iio/pressure/mprls0025pa_i2c.c
new file mode 100644
index 000000000000..89d6a206192b
--- /dev/null
+++ b/drivers/iio/pressure/mprls0025pa_i2c.c
@@ -0,0 +1,98 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * MPRLS0025PA - Honeywell MicroPressure pressure sensor series driver
+ *
+ * Copyright (c) Andreas Klinger <ak@it-klinger.de>
+ *
+ * Data sheet:
+ *  https://prod-edam.honeywell.com/content/dam/honeywell-edam/sps/siot/en-us/products/sensors/pressure-sensors/board-mount-pressure-sensors/micropressure-mpr-series/documents/sps-siot-mpr-series-datasheet-32332628-ciid-172626.pdf
+ */
+
+#include <linux/errno.h>
+#include <linux/i2c.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+
+#include <linux/iio/iio.h>
+
+#include "mprls0025pa.h"
+
+static int mpr_i2c_init(struct device *unused)
+{
+	return 0;
+}
+
+static int mpr_i2c_read(struct mpr_data *data, const u8 unused, const u8 cnt)
+{
+	int ret;
+	struct i2c_client *client = to_i2c_client(data->dev);
+
+	if (cnt > MPR_MEASUREMENT_RD_SIZE)
+		return -EOVERFLOW;
+
+	memset(data->buffer, 0, MPR_MEASUREMENT_RD_SIZE);
+	ret = i2c_master_recv(client, data->buffer, cnt);
+	if (ret != cnt) {
+		return -EIO;
+	}
+
+	return 0;
+}
+
+static int mpr_i2c_write(struct mpr_data *data, const u8 cmd, const u8 unused)
+{
+	int ret;
+	struct i2c_client *client = to_i2c_client(data->dev);
+	u8 wdata[MPR_PKT_SYNC_LEN];
+
+	memset(wdata, 0, sizeof(wdata));
+	wdata[0] = cmd;
+
+	ret = i2c_master_send(client, wdata, MPR_PKT_SYNC_LEN);
+	if (ret != MPR_PKT_SYNC_LEN) {
+		return -EIO;
+	}
+
+	return 0;
+}
+
+static const struct mpr_ops mpr_i2c_ops = {
+	.init = mpr_i2c_init,
+	.read = mpr_i2c_read,
+	.write = mpr_i2c_write,
+};
+
+static int mpr_i2c_probe(struct i2c_client *client)
+{
+	if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_READ_BYTE))
+		return -EOPNOTSUPP;
+
+	return mpr_common_probe(&client->dev, &mpr_i2c_ops, client->irq);
+}
+
+static const struct of_device_id mpr_i2c_match[] = {
+	{ .compatible = "honeywell,mprls0025pa" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, mpr_i2c_match);
+
+static const struct i2c_device_id mpr_i2c_id[] = {
+	{ "mprls0025pa" },
+	{}
+};
+MODULE_DEVICE_TABLE(i2c, mpr_i2c_id);
+
+static struct i2c_driver mpr_i2c_driver = {
+	.probe = mpr_i2c_probe,
+	.id_table = mpr_i2c_id,
+	.driver = {
+		.name = "mprls0025pa",
+		.of_match_table = mpr_i2c_match,
+	},
+};
+module_i2c_driver(mpr_i2c_driver);
+
+MODULE_AUTHOR("Andreas Klinger <ak@it-klinger.de>");
+MODULE_DESCRIPTION("Honeywell MPR pressure sensor i2c driver");
+MODULE_LICENSE("GPL");
+MODULE_IMPORT_NS(IIO_HONEYWELL_MPRLS0025PA);
--
2.41.0

[PATCH v2 09/10] iio: pressure: mprls0025pa.c add triplet property

[Petre Rodan]

Add honeywell,pressure-triplet property that automatically initializes
pmin-pascal, pmax-pascal so that the user is not required to look-up
the chip in the datasheet and convert various units to pascals himself.

Signed-off-by: Petre Rodan <petre.rodan@subdimension.ro>
Signed-off-by: Andreas Klinger <ak@it-klinger.de>
---
 drivers/iio/pressure/mprls0025pa.c | 102 +++++++++++++++++++++++++++--
 1 file changed, 95 insertions(+), 7 deletions(-)

diff --git a/drivers/iio/pressure/mprls0025pa.c b/drivers/iio/pressure/mprls0025pa.c
index cb5d6c0cca7e..90aed290b6c2 100644
--- a/drivers/iio/pressure/mprls0025pa.c
+++ b/drivers/iio/pressure/mprls0025pa.c
@@ -81,6 +81,78 @@ static const struct mpr_func_spec mpr_func_spec[] = {
 	[MPR_FUNCTION_C] = { .output_min = 3355443, .output_max = 13421773 },
 };

+enum mpr_variants {
+	MPR0001BA = 0x00, MPR01_6BA = 0x01, MPR02_5BA = 0x02, MPR0060MG = 0x03,
+	MPR0100MG = 0x04, MPR0160MG = 0x05, MPR0250MG = 0x06, MPR0400MG = 0x07,
+	MPR0600MG = 0x08, MPR0001BG = 0x09, MPR01_6BG = 0x0a, MPR02_5BG = 0x0b,
+	MPR0100KA = 0x0c, MPR0160KA = 0x0d, MPR0250KA = 0x0e, MPR0006KG = 0x0f,
+	MPR0010KG = 0x10, MPR0016KG = 0x11, MPR0025KG = 0x12, MPR0040KG = 0x13,
+	MPR0060KG = 0x14, MPR0100KG = 0x15, MPR0160KG = 0x16, MPR0250KG = 0x17,
+	MPR0015PA = 0x18, MPR0025PA = 0x19, MPR0030PA = 0x1a, MPR0001PG = 0x1b,
+	MPR0005PG = 0x1c, MPR0015PG = 0x1d, MPR0030PG = 0x1e, MPR0300YG = 0x1f,
+	MPR_VARIANTS_MAX
+};
+
+static const char * const mpr_triplet_variants[MPR_VARIANTS_MAX] = {
+	[MPR0001BA] = "0001BA", [MPR01_6BA] = "01.6BA", [MPR02_5BA] = "02.5BA",
+	[MPR0060MG] = "0060MG", [MPR0100MG] = "0100MG", [MPR0160MG] = "0160MG",
+	[MPR0250MG] = "0250MG", [MPR0400MG] = "0400MG", [MPR0600MG] = "0600MG",
+	[MPR0001BG] = "0001BG", [MPR01_6BG] = "01.6BG", [MPR02_5BG] = "02.5BG",
+	[MPR0100KA] = "0100KA", [MPR0160KA] = "0160KA", [MPR0250KA] = "0250KA",
+	[MPR0006KG] = "0006KG", [MPR0010KG] = "0010KG", [MPR0016KG] = "0016KG",
+	[MPR0025KG] = "0025KG", [MPR0040KG] = "0040KG", [MPR0060KG] = "0060KG",
+	[MPR0100KG] = "0100KG", [MPR0160KG] = "0160KG", [MPR0250KG] = "0250KG",
+	[MPR0015PA] = "0015PA", [MPR0025PA] = "0025PA", [MPR0030PA] = "0030PA",
+	[MPR0001PG] = "0001PG", [MPR0005PG] = "0005PG", [MPR0015PG] = "0015PG",
+	[MPR0030PG] = "0030PG", [MPR0300YG] = "0300YG"
+};
+
+/**
+ * struct mpr_range_config - list of pressure ranges based on nomenclature
+ * @pmin: lowest pressure that can be measured
+ * @pmax: highest pressure that can be measured
+ */
+struct mpr_range_config {
+	const s32 pmin;
+	const s32 pmax;
+};
+
+/* All min max limits have been converted to pascals */
+static const struct mpr_range_config mpr_range_config[MPR_VARIANTS_MAX] = {
+	[MPR0001BA] = { .pmin = 0, .pmax = 100000 },
+	[MPR01_6BA] = { .pmin = 0, .pmax = 160000 },
+	[MPR02_5BA] = { .pmin = 0, .pmax = 250000 },
+	[MPR0060MG] = { .pmin = 0, .pmax =   6000 },
+	[MPR0100MG] = { .pmin = 0, .pmax =  10000 },
+	[MPR0160MG] = { .pmin = 0, .pmax =  16000 },
+	[MPR0250MG] = { .pmin = 0, .pmax =  25000 },
+	[MPR0400MG] = { .pmin = 0, .pmax =  40000 },
+	[MPR0600MG] = { .pmin = 0, .pmax =  60000 },
+	[MPR0001BG] = { .pmin = 0, .pmax = 100000 },
+	[MPR01_6BG] = { .pmin = 0, .pmax = 160000 },
+	[MPR02_5BG] = { .pmin = 0, .pmax = 250000 },
+	[MPR0100KA] = { .pmin = 0, .pmax = 100000 },
+	[MPR0160KA] = { .pmin = 0, .pmax = 160000 },
+	[MPR0250KA] = { .pmin = 0, .pmax = 250000 },
+	[MPR0006KG] = { .pmin = 0, .pmax =   6000 },
+	[MPR0010KG] = { .pmin = 0, .pmax =  10000 },
+	[MPR0016KG] = { .pmin = 0, .pmax =  16000 },
+	[MPR0025KG] = { .pmin = 0, .pmax =  25000 },
+	[MPR0040KG] = { .pmin = 0, .pmax =  40000 },
+	[MPR0060KG] = { .pmin = 0, .pmax =  60000 },
+	[MPR0100KG] = { .pmin = 0, .pmax = 100000 },
+	[MPR0160KG] = { .pmin = 0, .pmax = 160000 },
+	[MPR0250KG] = { .pmin = 0, .pmax = 250000 },
+	[MPR0015PA] = { .pmin = 0, .pmax = 103421 },
+	[MPR0025PA] = { .pmin = 0, .pmax = 172369 },
+	[MPR0030PA] = { .pmin = 0, .pmax = 206843 },
+	[MPR0001PG] = { .pmin = 0, .pmax =   6895 },
+	[MPR0005PG] = { .pmin = 0, .pmax =  34474 },
+	[MPR0015PG] = { .pmin = 0, .pmax = 103421 },
+	[MPR0030PG] = { .pmin = 0, .pmax = 206843 },
+	[MPR0300YG] = { .pmin = 0, .pmax =  39997 }
+};
+
 static const struct iio_chan_spec mpr_channels[] = {
 	{
 		.type = IIO_PRESSURE,
@@ -258,6 +330,7 @@ int mpr_common_probe(struct device *dev, const struct mpr_ops *ops, int irq)
 	int ret;
 	struct mpr_data *data;
 	struct iio_dev *indio_dev;
+	const char *triplet;
 	s64 scale, offset;
 	u32 func;

@@ -299,17 +372,32 @@ int mpr_common_probe(struct device *dev, const struct mpr_ops *ops, int irq)
 				     "honeywell,transfer-function %d invalid\n",
 				     data->function);

-	ret = device_property_read_u32(dev, "honeywell,pmin-pascal",
+	ret = device_property_read_string(dev, "honeywell,pressure-triplet",
+					  &triplet);
+	if (ret) {
+		ret = device_property_read_u32(dev, "honeywell,pmin-pascal",
 					       &data->pmin);
-	if (ret)
-		return dev_err_probe(dev, ret,
+		if (ret)
+			return dev_err_probe(dev, ret,
 				   "honeywell,pmin-pascal could not be read\n");

-	ret = device_property_read_u32(dev, "honeywell,pmax-pascal",
-				       &data->pmax);
-	if (ret)
-		return dev_err_probe(dev, ret,
+		ret = device_property_read_u32(dev, "honeywell,pmax-pascal",
+					       &data->pmax);
+		if (ret)
+			return dev_err_probe(dev, ret,
 				   "honeywell,pmax-pascal could not be read\n");
+	} else {
+		ret = device_property_match_property_string(dev,
+						   "honeywell,pressure-triplet",
+						   mpr_triplet_variants,
+						   MPR_VARIANTS_MAX);
+		if (ret < 0)
+			return dev_err_probe(dev, -EINVAL,
+				     "honeywell,pressure-triplet is invalid\n");
+
+		data->pmin = mpr_range_config[ret].pmin;
+		data->pmax = mpr_range_config[ret].pmax;
+	}

 	if (data->pmin >= data->pmax)
 		return dev_err_probe(dev, -EINVAL,
--
2.41.0

[PATCH v2 10/10] iio: pressure: mprls0025pa.c add SPI driver

[Petre Rodan]

Add SPI component of the driver.

Tested with mprls0015pa0000sa in spi mode on BeagleBone Black on
slightly patched 6.7.0-rc6 mainline.

Tested with mprls0025pa in i2c mode on BeagleBone Black with togreg
branch on
git://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git
(tag: iio-for-6.8a)

Signed-off-by: Petre Rodan <petre.rodan@subdimension.ro>
Tested-by: Andreas Klinger <ak@it-klinger.de>
---
 drivers/iio/pressure/Kconfig           |  8 ++-
 drivers/iio/pressure/Makefile          |  1 +
 drivers/iio/pressure/mprls0025pa_spi.c | 91 ++++++++++++++++++++++++++
 3 files changed, 99 insertions(+), 1 deletion(-)
 create mode 100644 drivers/iio/pressure/mprls0025pa_spi.c

diff --git a/drivers/iio/pressure/Kconfig b/drivers/iio/pressure/Kconfig
index f03007cfec85..5da7931dc537 100644
--- a/drivers/iio/pressure/Kconfig
+++ b/drivers/iio/pressure/Kconfig
@@ -181,8 +181,9 @@ config MPL3115

 config MPRLS0025PA
 	tristate "Honeywell MPRLS0025PA (MicroPressure sensors series)"
-	depends on I2C
+	depends on (I2C || SPI_MASTER)
 	select MPRLS0025PA_I2C if I2C
+	select MPRLS0025PA_SPI if SPI_MASTER
 	select IIO_BUFFER
 	select IIO_TRIGGERED_BUFFER
 	help
@@ -198,6 +199,11 @@ config MPRLS0025PA_I2C
 	depends on MPRLS0025PA
 	depends on I2C

+config MPRLS0025PA_SPI
+	tristate
+	depends on MPRLS0025PA
+	depends on SPI_MASTER
+
 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 7754135e190c..a93709e35760 100644
--- a/drivers/iio/pressure/Makefile
+++ b/drivers/iio/pressure/Makefile
@@ -25,6 +25,7 @@ obj-$(CONFIG_MPL115_SPI) += mpl115_spi.o
 obj-$(CONFIG_MPL3115) += mpl3115.o
 obj-$(CONFIG_MPRLS0025PA) += mprls0025pa.o
 obj-$(CONFIG_MPRLS0025PA_I2C) += mprls0025pa_i2c.o
+obj-$(CONFIG_MPRLS0025PA_SPI) += mprls0025pa_spi.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_spi.c b/drivers/iio/pressure/mprls0025pa_spi.c
new file mode 100644
index 000000000000..7ef43a7abc83
--- /dev/null
+++ b/drivers/iio/pressure/mprls0025pa_spi.c
@@ -0,0 +1,91 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * MPRLS0025PA - Honeywell MicroPressure MPR series SPI sensor driver
+ *
+ * Copyright (c) 2024 Petre Rodan <petre.rodan@subdimension.ro>
+ *
+ * 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
+ */
+
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/spi/spi.h>
+#include <linux/stddef.h>
+
+#include <linux/iio/iio.h>
+
+#include "mprls0025pa.h"
+
+struct mpr_spi_buf {
+	u8 tx[MPR_MEASUREMENT_RD_SIZE] __aligned(IIO_DMA_MINALIGN);
+};
+
+static int mpr_spi_init(struct device *dev)
+{
+	struct spi_device *spi = to_spi_device(dev);
+	struct mpr_spi_buf *buf;
+
+	buf = devm_kzalloc(dev, sizeof(*buf), GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+
+	spi_set_drvdata(spi, buf);
+
+	return 0;
+}
+
+static int mpr_spi_xfer(struct mpr_data *data, const u8 cmd, const u8 pkt_len)
+{
+	struct spi_device *spi = to_spi_device(data->dev);
+	struct mpr_spi_buf *buf = spi_get_drvdata(spi);
+	struct spi_transfer xfer;
+
+	if (pkt_len > MPR_MEASUREMENT_RD_SIZE)
+		return -EOVERFLOW;
+
+	buf->tx[0] = cmd;
+	xfer.tx_buf = buf->tx;
+	xfer.rx_buf = data->buffer;
+	xfer.len = pkt_len;
+
+	return spi_sync_transfer(spi, &xfer, 1);
+}
+
+static const struct mpr_ops mpr_spi_ops = {
+	.init = mpr_spi_init,
+	.read = mpr_spi_xfer,
+	.write = mpr_spi_xfer,
+};
+
+static int mpr_spi_probe(struct spi_device *spi)
+{
+	return mpr_common_probe(&spi->dev, &mpr_spi_ops, spi->irq);
+}
+
+static const struct of_device_id mpr_spi_match[] = {
+	{ .compatible = "honeywell,mprls0025pa" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, mpr_spi_match);
+
+static const struct spi_device_id mpr_spi_id[] = {
+	{ "mprls0025pa" },
+	{}
+};
+MODULE_DEVICE_TABLE(spi, mpr_spi_id);
+
+static struct spi_driver mpr_spi_driver = {
+	.driver = {
+		.name = "mprls0025pa",
+		.of_match_table = mpr_spi_match,
+	},
+	.probe = mpr_spi_probe,
+	.id_table = mpr_spi_id,
+};
+module_spi_driver(mpr_spi_driver);
+
+MODULE_AUTHOR("Petre Rodan <petre.rodan@subdimension.ro>");
+MODULE_DESCRIPTION("Honeywell MPR pressure sensor spi driver");
+MODULE_LICENSE("GPL");
+MODULE_IMPORT_NS(IIO_HONEYWELL_MPRLS0025PA);
--
2.41.0

Re: [PATCH v2 01/10] dt-bindings: iio: pressure: honeywell,mprls0025pa.yaml fix

[Krzysztof Kozlowski]

On 24/12/2023 15:34, Petre Rodan wrote:
> Define enum inside the honeywell,transfer-function property block.
> 
> Set the correct irq edge in the example block.
> Based on the datasheet, in table 13 on page 11:
> "End-of-conversion indicator: This pin is set high when a measurement
> and calculation have been completed and the data is ready to be
> clocked out"
> 

Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: [PATCH v2 02/10] dt-bindings: iio: pressure: honeywell,mprls0025pa.yaml add pressure-triplet

[Krzysztof Kozlowski]

On 24/12/2023 15:34, Petre Rodan wrote:
> Change order of properties in order for the end user to de-prioritize
> pmin-pascal and pmax-pascal which are superseded by pressure-triplet.
> 
> Add pressure-triplet property which automatically initializes
> pmin-pascal and pmax-pascal inside the driver
> 
> Rework honeywell,pmXX-pascal requirements based on feedback from
> Jonathan and Conor.
> 
> Signed-off-by: Petre Rodan <petre.rodan@subdimension.ro>
> Signed-off-by: Andreas Klinger <ak@it-klinger.de>
> ---
>  .../iio/pressure/honeywell,mprls0025pa.yaml   | 64 ++++++++++++++-----
>  1 file changed, 47 insertions(+), 17 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/iio/pressure/honeywell,mprls0025pa.yaml b/Documentation/devicetree/bindings/iio/pressure/honeywell,mprls0025pa.yaml
> index 84ced4e5a7da..e4021306d187 100644
> --- a/Documentation/devicetree/bindings/iio/pressure/honeywell,mprls0025pa.yaml
> +++ b/Documentation/devicetree/bindings/iio/pressure/honeywell,mprls0025pa.yaml
> @@ -19,14 +19,17 @@ description: |
>    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
> +  To support different models one need to specify its pressure triplet as well
> +  as the transfer function.
> +
> +  For custom models the pressure values can alternatively be specified manually.
> +  The minimal range value stands for the minimum pressure and the maximum value
> +  also for the maximum pressure with linear relation inside the range.
> +  Pressure range needs to be converted from the datasheet specified 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.
> +  sensor.
> 
>    Specifications about the devices can be found at:
>      https://prod-edam.honeywell.com/content/dam/honeywell-edam/sps/siot/en-us/
> @@ -54,14 +57,6 @@ properties:
>        If not present the device is not reset during the probe.
>      maxItems: 1
> 
> -  honeywell,pmin-pascal:
> -    description:
> -      Minimum pressure value the sensor can measure in pascal.
> -
> -  honeywell,pmax-pascal:
> -    description:
> -      Maximum pressure value the sensor can measure in pascal.
> -
>    honeywell,transfer-function:
>      description: |
>        Transfer function which defines the range of valid values delivered by the
> @@ -72,17 +67,52 @@ properties:
>      enum: [1, 2, 3]
>      $ref: /schemas/types.yaml#/definitions/uint32
> 
> +  honeywell,pressure-triplet:

Why not putting it just before existing properties?

> +    description: |
> +      Case-sensitive five character string that defines pressure range, unit
> +      and type as part of the device nomenclature. In the unlikely case of a
> +      custom chip, unset and provide pmin-pascal and pmax-pascal instead.
> +    enum: [0001BA, 01.6BA, 02.5BA, 0060MG, 0100MG, 0160MG, 0250MG, 0400MG,
> +           0600MG, 0001BG, 01.6BG, 02.5BG, 0100KA, 0160KA, 0250KA, 0006KG,
> +           0010KG, 0016KG, 0025KG, 0040KG, 0060KG, 0100KG, 0160KG, 0250KG,
> +           0015PA, 0025PA, 0030PA, 0001PG, 0005PG, 0015PG, 0030PG, 0300YG]
> +    $ref: /schemas/types.yaml#/definitions/string
> +
> +  honeywell,pmin-pascal:
> +    description:
> +      Minimum pressure value the sensor can measure in pascal.
> +      To be specified only if honeywell,pressure-triplet is not set.

The last sentence is redundant - schema should enforce that.

> +
> +  honeywell,pmax-pascal:
> +    description:
> +      Maximum pressure value the sensor can measure in pascal.
> +      To be specified only if honeywell,pressure-triplet is not set.
> +
>    vdd-supply:
>      description: provide VDD power to the sensor.
> 
>  required:
>    - compatible
>    - reg
> -  - honeywell,pmin-pascal
> -  - honeywell,pmax-pascal
>    - honeywell,transfer-function
>    - vdd-supply
> 
> +oneOf:
> +  - required:
> +      - honeywell,pmin-pascal
> +      - honeywell,pmax-pascal
> +  - required:
> +      - honeywell,pressure-triplet
> +
> +allOf:
> +  - if:
> +      required:
> +        - honeywell,pressure-triplet
> +    then:
> +      properties:
> +        honeywell,pmin-pascal: false
> +        honeywell,pmax-pascal: false

This allOf is not needed.

Best regards,
Krzysztof

Re: [PATCH v2 03/10] dt-bindings: iio: pressure: honeywell,mprls0025pa.yaml add spi bus

[Krzysztof Kozlowski]

On 24/12/2023 15:34, Petre Rodan wrote:
> Add spi based example.
> 
> Add spi-max-frequency property required by chip specifications.
> 
> Add additional maintainer.
> 
> Signed-off-by: Petre Rodan <petre.rodan@subdimension.ro>
> Signed-off-by: Andreas Klinger <ak@it-klinger.de>
> ---
>  .../iio/pressure/honeywell,mprls0025pa.yaml   | 26 +++++++++++++++++--
>  1 file changed, 24 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/iio/pressure/honeywell,mprls0025pa.yaml b/Documentation/devicetree/bindings/iio/pressure/honeywell,mprls0025pa.yaml
> index e4021306d187..430496b047c7 100644
> --- a/Documentation/devicetree/bindings/iio/pressure/honeywell,mprls0025pa.yaml
> +++ b/Documentation/devicetree/bindings/iio/pressure/honeywell,mprls0025pa.yaml
> @@ -8,12 +8,12 @@ title: Honeywell mprls0025pa pressure sensor
> 
>  maintainers:
>    - Andreas Klinger <ak@it-klinger.de>
> +  - Petre Rodan <petre.rodan@subdimension.ro>
> 
>  description: |
>    Honeywell pressure sensor of model mprls0025pa.
> 
> -  This sensor has an I2C and SPI interface. Only the I2C interface is
> -  implemented.
> +  This sensor has an I2C and SPI interface. Both are supported.

Instead drop that sentence. Current driver support should not matter for
the bindings.

> 
>    There are many models with different pressure ranges available. The vendor
>    calls them "mpr series". All of them have the identical programming model and
> @@ -88,6 +88,9 @@ properties:
>        Maximum pressure value the sensor can measure in pascal.
>        To be specified only if honeywell,pressure-triplet is not set.
> 
> +  spi-max-frequency:
> +    maximum: 800000

So you miss allOf: with $ref to spi props.

Best regards,
Krzysztof

Re: [PATCH v2 02/10] dt-bindings: iio: pressure: honeywell,mprls0025pa.yaml add pressure-triplet

[Petre Rodan]

[-- Attachment #1: Type: text/plain, Size: 2918 bytes --]


hello Krzysztof,

On Mon, Dec 25, 2023 at 01:57:39PM +0100, Krzysztof Kozlowski wrote:
> On 24/12/2023 15:34, Petre Rodan wrote:
> > @@ -54,14 +57,6 @@ properties:
> >        If not present the device is not reset during the probe.
> >      maxItems: 1
> > 
> > -  honeywell,pmin-pascal:
> > -    description:
> > -      Minimum pressure value the sensor can measure in pascal.
> > -
> > -  honeywell,pmax-pascal:
> > -    description:
> > -      Maximum pressure value the sensor can measure in pascal.
> > -
> >    honeywell,transfer-function:
> >      description: |
> >        Transfer function which defines the range of valid values delivered by the
> > @@ -72,17 +67,52 @@ properties:
> >      enum: [1, 2, 3]
> >      $ref: /schemas/types.yaml#/definitions/uint32
> > 
> > +  honeywell,pressure-triplet:
> 
> Why not putting it just before existing properties?

I'd like to have pmin-pascal, pmax-pascal as the last two honeywell specific
properties, since they are not to be used unless someone has custom silicon.
so we will still have a block moved just like above.
the most logic order is the one I proposed above:

honeywell,transfer-function:
[..]
honeywell,pressure-triplet:
[..]
honeywell,pmin-pascal:
[..]
honeywell,pmax-pascal:
[..]

since the last 3 are tied together as we will see below.
is there any reason you want this order to change?

> > +  honeywell,pmin-pascal:
> > +    description:
> > +      Minimum pressure value the sensor can measure in pascal.
> > +      To be specified only if honeywell,pressure-triplet is not set.
> 
> The last sentence is redundant - schema should enforce that.

when someone generates the dtbo files via

cpp -nostdinc -I include -I ${LINUX_SRC}/include/ -I arch -undef -x assembler-with-cpp ${file}.dts "${BUILD_DIR}/${file}.dts.preprocessed"
dtc -@ -I dts -O dtb -o "${BUILD_DIR}/${file}.dtbo" "${BUILD_DIR}/${file}.dts.preprocessed"

the schema is not checked in any way.
so unless people can be bothered to understand the yaml intricacies in the
bindings file, I feel they need to see that redundant information there, see below.

> > +oneOf:
> > +  - required:
> > +      - honeywell,pmin-pascal
> > +      - honeywell,pmax-pascal
> > +  - required:
> > +      - honeywell,pressure-triplet
> > +
> > +allOf:
> > +  - if:
> > +      required:
> > +        - honeywell,pressure-triplet
> > +    then:
> > +      properties:
> > +        honeywell,pmin-pascal: false
> > +        honeywell,pmax-pascal: false
> 
> This allOf is not needed.

speaking for intricacies, if the allOf is removed, then a binding containing

honeywell,pmax-pascal = <840000>;
honeywell,pressure-triplet = "0015PA";

would be considered to be correct by the schema, but that would be the incorrect
result. so afaict allOf needs to stay, and so does the redundant text.

best regards,
peter

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 02/10] dt-bindings: iio: pressure: honeywell,mprls0025pa.yaml add pressure-triplet

[Krzysztof Kozlowski]

On 25/12/2023 14:23, Petre Rodan wrote:
> 
> hello Krzysztof,
> 
> On Mon, Dec 25, 2023 at 01:57:39PM +0100, Krzysztof Kozlowski wrote:
>> On 24/12/2023 15:34, Petre Rodan wrote:
>>> @@ -54,14 +57,6 @@ properties:
>>>        If not present the device is not reset during the probe.
>>>      maxItems: 1
>>>
>>> -  honeywell,pmin-pascal:
>>> -    description:
>>> -      Minimum pressure value the sensor can measure in pascal.
>>> -
>>> -  honeywell,pmax-pascal:
>>> -    description:
>>> -      Maximum pressure value the sensor can measure in pascal.
>>> -
>>>    honeywell,transfer-function:
>>>      description: |
>>>        Transfer function which defines the range of valid values delivered by the
>>> @@ -72,17 +67,52 @@ properties:
>>>      enum: [1, 2, 3]
>>>      $ref: /schemas/types.yaml#/definitions/uint32
>>>
>>> +  honeywell,pressure-triplet:
>>
>> Why not putting it just before existing properties?
> 
> I'd like to have pmin-pascal, pmax-pascal as the last two honeywell specific
> properties, since they are not to be used unless someone has custom silicon.
> so we will still have a block moved just like above.
> the most logic order is the one I proposed above:
> 
> honeywell,transfer-function:
> [..]
> honeywell,pressure-triplet:
> [..]
> honeywell,pmin-pascal:
> [..]
> honeywell,pmax-pascal:
> [..]
> 
> since the last 3 are tied together as we will see below.
> is there any reason you want this order to change?

I just don't get why moving the code instead of adding new property next
to them.

The order is often alphabetical.

> 
>>> +  honeywell,pmin-pascal:
>>> +    description:
>>> +      Minimum pressure value the sensor can measure in pascal.
>>> +      To be specified only if honeywell,pressure-triplet is not set.
>>
>> The last sentence is redundant - schema should enforce that.
> 
> when someone generates the dtbo files via
> 
> cpp -nostdinc -I include -I ${LINUX_SRC}/include/ -I arch -undef -x assembler-with-cpp ${file}.dts "${BUILD_DIR}/${file}.dts.preprocessed"
> dtc -@ -I dts -O dtb -o "${BUILD_DIR}/${file}.dtbo" "${BUILD_DIR}/${file}.dts.preprocessed"

And how this command matters? DT overlays are checked, so error is printed.

> 
> the schema is not checked in any way.

When I run `make` the schema is also not checked, so is it an argument
to add anything to the binding? No. Drop redundant text.

> so unless people can be bothered to understand the yaml intricacies in the
> bindings file, I feel they need to see that redundant information there, see below.



> 
>>> +oneOf:
>>> +  - required:
>>> +      - honeywell,pmin-pascal
>>> +      - honeywell,pmax-pascal
>>> +  - required:
>>> +      - honeywell,pressure-triplet
>>> +
>>> +allOf:
>>> +  - if:
>>> +      required:
>>> +        - honeywell,pressure-triplet
>>> +    then:
>>> +      properties:
>>> +        honeywell,pmin-pascal: false
>>> +        honeywell,pmax-pascal: false
>>
>> This allOf is not needed.
> 
> speaking for intricacies, if the allOf is removed, then a binding containing
> 
> honeywell,pmax-pascal = <840000>;
> honeywell,pressure-triplet = "0015PA";
> 
> would be considered to be correct by the schema, but that would be the incorrect
> result. so afaict allOf needs to stay, and so does the redundant text.

Really? Did you test it?

Best regards,
Krzysztof

Re: [PATCH v2 02/10] dt-bindings: iio: pressure: honeywell,mprls0025pa.yaml add pressure-triplet

[Krzysztof Kozlowski]

On 25/12/2023 14:34, Krzysztof Kozlowski wrote:
>>>> +oneOf:
>>>> +  - required:
>>>> +      - honeywell,pmin-pascal
>>>> +      - honeywell,pmax-pascal
>>>> +  - required:
>>>> +      - honeywell,pressure-triplet
>>>> +
>>>> +allOf:
>>>> +  - if:
>>>> +      required:
>>>> +        - honeywell,pressure-triplet
>>>> +    then:
>>>> +      properties:
>>>> +        honeywell,pmin-pascal: false
>>>> +        honeywell,pmax-pascal: false
>>>
>>> This allOf is not needed.
>>
>> speaking for intricacies, if the allOf is removed, then a binding containing
>>
>> honeywell,pmax-pascal = <840000>;
>> honeywell,pressure-triplet = "0015PA";
>>
>> would be considered to be correct by the schema, but that would be the incorrect
>> result. so afaict allOf needs to stay, and so does the redundant text.
> 
> Really? Did you test it?

Hm, indeed, on pmin/pmax would not trigger first required case. OK, then
this part make sense.

Best regards,
Krzysztof

Re: [PATCH v2 02/10] dt-bindings: iio: pressure: honeywell,mprls0025pa.yaml add pressure-triplet

[Petre Rodan]

[-- Attachment #1: Type: text/plain, Size: 4378 bytes --]


hello,

On Mon, Dec 25, 2023 at 02:34:04PM +0100, Krzysztof Kozlowski wrote:
> On 25/12/2023 14:23, Petre Rodan wrote:
> > honeywell,transfer-function:
> > [..]
> > honeywell,pressure-triplet:
> > [..]
> > honeywell,pmin-pascal:
> > [..]
> > honeywell,pmax-pascal:
> > [..]
> > 
> > since the last 3 are tied together as we will see below.
> > is there any reason you want this order to change?
> 
> I just don't get why moving the code instead of adding new property next
> to them.

as I also said in the comments and in my last reply I want the user to not feel
in any way obliged to fill in pmin-pascal, pmax-pascal.
and since a user reads this file from top to bottom, the order in which these
properties are shown to him is important, and it is the one above.

> The order is often alphabetical.

can we please make an exception?

> >>> +  honeywell,pmin-pascal:
> >>> +    description:
> >>> +      Minimum pressure value the sensor can measure in pascal.
> >>> +      To be specified only if honeywell,pressure-triplet is not set.
> >>
> >> The last sentence is redundant - schema should enforce that.
> > 
> > when someone generates the dtbo files via
> > 
> > cpp -nostdinc -I include -I ${LINUX_SRC}/include/ -I arch -undef -x assembler-with-cpp ${file}.dts "${BUILD_DIR}/${file}.dts.preprocessed"
> > dtc -@ -I dts -O dtb -o "${BUILD_DIR}/${file}.dtbo" "${BUILD_DIR}/${file}.dts.preprocessed"
> 
> And how this command matters? DT overlays are checked, so error is printed.
> 
> > 
> > the schema is not checked in any way.
> 
> When I run `make` the schema is also not checked, so is it an argument
> to add anything to the binding? No. Drop redundant text.
> 
> > so unless people can be bothered to understand the yaml intricacies in the
> > bindings file, I feel they need to see that redundant information there, see below.
> 
> 
> 
> > 
> >>> +oneOf:
> >>> +  - required:
> >>> +      - honeywell,pmin-pascal
> >>> +      - honeywell,pmax-pascal
> >>> +  - required:
> >>> +      - honeywell,pressure-triplet
> >>> +
> >>> +allOf:
> >>> +  - if:
> >>> +      required:
> >>> +        - honeywell,pressure-triplet
> >>> +    then:
> >>> +      properties:
> >>> +        honeywell,pmin-pascal: false
> >>> +        honeywell,pmax-pascal: false
> >>
> >> This allOf is not needed.
> > 
> > speaking for intricacies, if the allOf is removed, then a binding containing
> > 
> > honeywell,pmax-pascal = <840000>;
> > honeywell,pressure-triplet = "0015PA";
> > 
> > would be considered to be correct by the schema, but that would be the incorrect
> > result. so afaict allOf needs to stay, and so does the redundant text.
> 
> Really? Did you test it?

for more hours than I would have liked. the allOf was provided with kindness by
Conor in my first revision.

testing it:
 1. invalid yaml with both honeywell,pmax-pascal and honeywell,pressure-triplet
 defined passes the schema check if the allOf is removed:

 # make DT_SCHEMA_FILES=Documentation/devicetree/bindings/iio/pressure/honeywell,mprls0025pa.yaml  DT_CHECKER_FLAGS=-m dt_binding_check
 # echo $?
0

 2. the same invalid yaml but with the allOf not removed issues this output:

 [..]/Documentation/devicetree/bindings/iio/pressure/honeywell,mprls0025pa.example.dtb: pressure@18: honeywell,pmax-pascal: False schema does not allow [[84000]]
         from schema $id: http://devicetree.org/schemas/iio/pressure/honeywell,mprls0025pa.yaml#

which is the expected behaviour. so AFAICT the allOf block is required, as well
as the redundant text for the humans that read the human-readable parts of the
bindings file.

invalid yaml example used above:

    #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_RISING>;

            honeywell,pmax-pascal = <84000>;
            honeywell,pressure-triplet = "0025PA";
            honeywell,transfer-function = <1>;
            vdd-supply = <&vcc_3v3>;
        };
    };

best regards,
peter

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 03/10] dt-bindings: iio: pressure: honeywell,mprls0025pa.yaml add spi bus

[Petre Rodan]

[-- Attachment #1: Type: text/plain, Size: 1041 bytes --]


hello,

On Mon, Dec 25, 2023 at 01:59:43PM +0100, Krzysztof Kozlowski wrote:
> On 24/12/2023 15:34, Petre Rodan wrote:
> > Add spi based example.
> >    There are many models with different pressure ranges available. The vendor
> >    calls them "mpr series". All of them have the identical programming model and
> > @@ -88,6 +88,9 @@ properties:
> >        Maximum pressure value the sensor can measure in pascal.
> >        To be specified only if honeywell,pressure-triplet is not set.
> > 
> > +  spi-max-frequency:
> > +    maximum: 800000
> 
> So you miss allOf: with $ref to spi props.

for simplicity's sake and for compatibility with the i2c devices already in use,
this driver does not have distinct 'compatible' properties for the i2c and spi
implementation.
this is why I just defined spi-max-frequency, used it in the spi example, but
not required it. just like in hsc030pa.yaml .

without a differentiation in the 'compatible' string I don't see how your request
can be implemented.

cheers,
peter

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 03/10] dt-bindings: iio: pressure: honeywell,mprls0025pa.yaml add spi bus

[Krzysztof Kozlowski]

On 25/12/2023 16:13, Petre Rodan wrote:
> 
> hello,
> 
> On Mon, Dec 25, 2023 at 01:59:43PM +0100, Krzysztof Kozlowski wrote:
>> On 24/12/2023 15:34, Petre Rodan wrote:
>>> Add spi based example.
>>>    There are many models with different pressure ranges available. The vendor
>>>    calls them "mpr series". All of them have the identical programming model and
>>> @@ -88,6 +88,9 @@ properties:
>>>        Maximum pressure value the sensor can measure in pascal.
>>>        To be specified only if honeywell,pressure-triplet is not set.
>>>
>>> +  spi-max-frequency:
>>> +    maximum: 800000
>>
>> So you miss allOf: with $ref to spi props.
> 
> for simplicity's sake and for compatibility with the i2c devices already in use,
> this driver does not have distinct 'compatible' properties for the i2c and spi
> implementation.
> this is why I just defined spi-max-frequency, used it in the spi example, but
> not required it. just like in hsc030pa.yaml .
> 
> without a differentiation in the 'compatible' string I don't see how your request
> can be implemented.

You cannot have different compatibles. I did not propose it. I wrote
nothing about compatible. I wrote about missing $ref in top-level for
spi-peripheral-props. Where do you see anything about compatible?

Best regards,
Krzysztof

Re: [PATCH v2 03/10] dt-bindings: iio: pressure: honeywell,mprls0025pa.yaml add spi bus

[Petre Rodan]

[-- Attachment #1: Type: text/plain, Size: 1568 bytes --]

On Mon, Dec 25, 2023 at 07:56:24PM +0100, Krzysztof Kozlowski wrote:
> On 25/12/2023 16:13, Petre Rodan wrote:
> >>> @@ -88,6 +88,9 @@ properties:
> >>>        Maximum pressure value the sensor can measure in pascal.
> >>>        To be specified only if honeywell,pressure-triplet is not set.
> >>>
> >>> +  spi-max-frequency:
> >>> +    maximum: 800000
> >>
> >> So you miss allOf: with $ref to spi props.
> > 
> > for simplicity's sake and for compatibility with the i2c devices already in use,
> > this driver does not have distinct 'compatible' properties for the i2c and spi
> > implementation.
> > this is why I just defined spi-max-frequency, used it in the spi example, but
> > not required it. just like in hsc030pa.yaml .
> > 
> > without a differentiation in the 'compatible' string I don't see how your request
> > can be implemented.
> 
> You cannot have different compatibles. I did not propose it. I wrote
> nothing about compatible. I wrote about missing $ref in top-level for
> spi-peripheral-props. Where do you see anything about compatible?

sorry, for one hot second I thought you want that property to be conditionally
defined, like

allOf:
  - $ref: /schemas/spi/spi-peripheral-props.yaml

  - if:
      properties:
        compatible:
          contains:
            const: honeywell,foo-spi
    then:
      properties:
        spi-max-frequency:
          maximum: 800000
      required:
        - spi-max-frequency

but I guess you only want the first two lines from here.

happy holidays,
peter

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 03/10] dt-bindings: iio: pressure: honeywell,mprls0025pa.yaml add spi bus

[Krzysztof Kozlowski]

On 25/12/2023 21:29, Petre Rodan wrote:
>>> without a differentiation in the 'compatible' string I don't see how your request
>>> can be implemented.
>>
>> You cannot have different compatibles. I did not propose it. I wrote
>> nothing about compatible. I wrote about missing $ref in top-level for
>> spi-peripheral-props. Where do you see anything about compatible?
> 
> sorry, for one hot second I thought you want that property to be conditionally
> defined, like
> 
> allOf:
>   - $ref: /schemas/spi/spi-peripheral-props.yaml
> 

Yes, this one.

Best regards,
Krzysztof

Re: [PATCH v2 01/10] dt-bindings: iio: pressure: honeywell,mprls0025pa.yaml fix

[Jonathan Cameron]

On Sun, 24 Dec 2023 16:34:46 +0200
Petre Rodan <petre.rodan@subdimension.ro> wrote:

> Define enum inside the honeywell,transfer-function property block.
> 
> Set the correct irq edge in the example block.
> Based on the datasheet, in table 13 on page 11:
> "End-of-conversion indicator: This pin is set high when a measurement
> and calculation have been completed and the data is ready to be
> clocked out"
> 
> Add description on End-of-conversion interrupt.
> 
> Signed-off-by: Petre Rodan <petre.rodan@subdimension.ro>
> Signed-off-by: Andreas Klinger <ak@it-klinger.de>
What's the relationship between Andreas and this patch?

Petre seems to have sent it so either Andreas should have a Co-authored-by or
should be the author... Or not there at all

Thanks,

Jonathan

> ---
>  .../bindings/iio/pressure/honeywell,mprls0025pa.yaml       | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/iio/pressure/honeywell,mprls0025pa.yaml b/Documentation/devicetree/bindings/iio/pressure/honeywell,mprls0025pa.yaml
> index d9e903fbfd99..84ced4e5a7da 100644
> --- a/Documentation/devicetree/bindings/iio/pressure/honeywell,mprls0025pa.yaml
> +++ b/Documentation/devicetree/bindings/iio/pressure/honeywell,mprls0025pa.yaml
> @@ -42,6 +42,10 @@ properties:
>      maxItems: 1
> 
>    interrupts:
> +    description:
> +      Optional interrupt for indicating End-of-conversion.
> +      If not present, the driver loops for a while until the received status
> +      byte indicates correct measurement.
>      maxItems: 1
> 
>    reset-gpios:
> @@ -65,6 +69,7 @@ properties:
>        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)
> +    enum: [1, 2, 3]
>      $ref: /schemas/types.yaml#/definitions/uint32
> 
>    vdd-supply:
> @@ -93,7 +98,7 @@ examples:
>              reg = <0x18>;
>              reset-gpios = <&gpio3 19 GPIO_ACTIVE_HIGH>;
>              interrupt-parent = <&gpio3>;
> -            interrupts = <21 IRQ_TYPE_EDGE_FALLING>;
> +            interrupts = <21 IRQ_TYPE_EDGE_RISING>;
>              honeywell,pmin-pascal = <0>;
>              honeywell,pmax-pascal = <172369>;
>              honeywell,transfer-function = <1>;
> --
> 2.41.0
> 

Re: [PATCH v2 01/10] dt-bindings: iio: pressure: honeywell,mprls0025pa.yaml fix

[Jonathan Cameron]

On Tue, 26 Dec 2023 16:28:39 +0000
Jonathan Cameron <jic23@kernel.org> wrote:

> On Sun, 24 Dec 2023 16:34:46 +0200
> Petre Rodan <petre.rodan@subdimension.ro> wrote:
> 
> > Define enum inside the honeywell,transfer-function property block.
> > 
> > Set the correct irq edge in the example block.
> > Based on the datasheet, in table 13 on page 11:
> > "End-of-conversion indicator: This pin is set high when a measurement
> > and calculation have been completed and the data is ready to be
> > clocked out"
> > 
> > Add description on End-of-conversion interrupt.
> > 
> > Signed-off-by: Petre Rodan <petre.rodan@subdimension.ro>
> > Signed-off-by: Andreas Klinger <ak@it-klinger.de>  
> What's the relationship between Andreas and this patch?
> 
> Petre seems to have sent it so either Andreas should have a Co-authored-by or
Co-developed-by: that is.


> should be the author... Or not there at all
> 
> Thanks,
> 
> Jonathan
> 
> > ---
> >  .../bindings/iio/pressure/honeywell,mprls0025pa.yaml       | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/iio/pressure/honeywell,mprls0025pa.yaml b/Documentation/devicetree/bindings/iio/pressure/honeywell,mprls0025pa.yaml
> > index d9e903fbfd99..84ced4e5a7da 100644
> > --- a/Documentation/devicetree/bindings/iio/pressure/honeywell,mprls0025pa.yaml
> > +++ b/Documentation/devicetree/bindings/iio/pressure/honeywell,mprls0025pa.yaml
> > @@ -42,6 +42,10 @@ properties:
> >      maxItems: 1
> > 
> >    interrupts:
> > +    description:
> > +      Optional interrupt for indicating End-of-conversion.
> > +      If not present, the driver loops for a while until the received status
> > +      byte indicates correct measurement.
> >      maxItems: 1
> > 
> >    reset-gpios:
> > @@ -65,6 +69,7 @@ properties:
> >        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)
> > +    enum: [1, 2, 3]
> >      $ref: /schemas/types.yaml#/definitions/uint32
> > 
> >    vdd-supply:
> > @@ -93,7 +98,7 @@ examples:
> >              reg = <0x18>;
> >              reset-gpios = <&gpio3 19 GPIO_ACTIVE_HIGH>;
> >              interrupt-parent = <&gpio3>;
> > -            interrupts = <21 IRQ_TYPE_EDGE_FALLING>;
> > +            interrupts = <21 IRQ_TYPE_EDGE_RISING>;
> >              honeywell,pmin-pascal = <0>;
> >              honeywell,pmax-pascal = <172369>;
> >              honeywell,transfer-function = <1>;
> > --
> > 2.41.0
> >   
> 
> 

Re: [PATCH v2 04/10] iio: pressure: mprls0025pa.c fix off-by-one enum

[Jonathan Cameron]

On Sun, 24 Dec 2023 16:34:49 +0200
Petre Rodan <petre.rodan@subdimension.ro> wrote:

> Fix off-by-one error in transfer-function property.
> The honeywell,transfer-function property takes values between 1-3 so
> make sure the proper enum gets used.
> 
> Signed-off-by: Petre Rodan <petre.rodan@subdimension.ro>
> Signed-off-by: Andreas Klinger <ak@it-klinger.de>
Hi Petre

Same question on what Andreas has to do with this patch and need
for some other tag to explain that.

Needs a fixes tag as well.

> ---
>  drivers/iio/pressure/mprls0025pa.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/pressure/mprls0025pa.c b/drivers/iio/pressure/mprls0025pa.c
> index 30fb2de36821..e3f0de020a40 100644
> --- a/drivers/iio/pressure/mprls0025pa.c
> +++ b/drivers/iio/pressure/mprls0025pa.c
> @@ -323,6 +323,7 @@ static int mpr_probe(struct i2c_client *client)
>  	struct iio_dev *indio_dev;
>  	struct device *dev = &client->dev;
>  	s64 scale, offset;
> +	u32 func;
> 
>  	if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_READ_BYTE))
>  		return dev_err_probe(dev, -EOPNOTSUPP,
> @@ -362,10 +363,11 @@ static int mpr_probe(struct i2c_client *client)
>  			return dev_err_probe(dev, ret,
>  				"honeywell,pmax-pascal could not be read\n");
>  		ret = device_property_read_u32(dev,
> -				"honeywell,transfer-function", &data->function);
> +				"honeywell,transfer-function", &func);
>  		if (ret)
>  			return dev_err_probe(dev, ret,
>  				"honeywell,transfer-function could not be read\n");
> +		data->function = func - 1;
>  		if (data->function > MPR_FUNCTION_C)
>  			return dev_err_probe(dev, -EINVAL,
>  				"honeywell,transfer-function %d invalid\n",
> --
> 2.41.0
> 

Re: [PATCH v2 05/10] iio: pressure: mprls0025pa.c fix error flag check

[Jonathan Cameron]

On Sun, 24 Dec 2023 16:34:50 +0200
Petre Rodan <petre.rodan@subdimension.ro> wrote:

> Take into account all 3 error flags while interacting with the sensor.
> Based on the datasheet, in table 14 on page 14, the status byte
> contains:
>     bit 5 busy flag - 1 if device is busy
>     bit 2 memory integrity/error flag - 1 if integrity test failed
>     bit 0 math saturation - 1 if internal math saturation has occurred
> 
> Signed-off-by: Petre Rodan <petre.rodan@subdimension.ro>
> Signed-off-by: Andreas Klinger <ak@it-klinger.de>
I've not problem with the patch, but the 'fix' in the titles means this should
have a fixes tag. I'm not sure that improving the resilience to device errors
is something we count as a fix however.  Maybe more of a feature or improvement
in which case don't say fix. 
Also, drop the .c in the patch title to be inline with similar patches in IIO.

Thanks,

Jonathan

> ---
>  drivers/iio/pressure/mprls0025pa.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/pressure/mprls0025pa.c b/drivers/iio/pressure/mprls0025pa.c
> index e3f0de020a40..233cc1dc38ad 100644
> --- a/drivers/iio/pressure/mprls0025pa.c
> +++ b/drivers/iio/pressure/mprls0025pa.c
> @@ -39,6 +39,8 @@
>  #define MPR_I2C_MEMORY	BIT(2)	/* integrity test passed */
>  #define MPR_I2C_MATH	BIT(0)	/* internal math saturation */
> 
> +#define MPR_I2C_ERR_FLAG  (MPR_I2C_BUSY | MPR_I2C_MEMORY | MPR_I2C_MATH)
> +
>  /*
>   * support _RAW sysfs interface:
>   *
> @@ -213,7 +215,7 @@ static int mpr_read_pressure(struct mpr_data *data, s32 *press)
>  					status);
>  				return status;
>  			}
> -			if (!(status & MPR_I2C_BUSY))
> +			if (!(status & MPR_I2C_ERR_FLAG))
>  				break;
>  		}
>  		if (i == nloops) {
> @@ -233,7 +235,7 @@ static int mpr_read_pressure(struct mpr_data *data, s32 *press)
>  		return -EIO;
>  	}
> 
> -	if (buf[0] & MPR_I2C_BUSY) {
> +	if (buf[0] & MPR_I2C_ERR_FLAG) {
>  		/*
>  		 * it should never be the case that status still indicates
>  		 * business
> --
> 2.41.0
> 
> 

Re: [PATCH v2 06/10] iio: pressure: mprls0025pa.c remove dangerous defaults

[Jonathan Cameron]

On Sun, 24 Dec 2023 16:34:51 +0200
Petre Rodan <petre.rodan@subdimension.ro> wrote:

> This driver supports 32*3 combinations of fixed ranges and transfer
> functions, plus custom ranges.
> 
> So statistically a user has more than 99% chance that the provided
> default configuration will generate invalid pressure readings if the
> bindings are not initialized and the driver is instantiated via sysfs.

I guess 99% is strong enough that it's unlikely we will break anyone so
fair enough to drop this attempt to guess the values.

> 
> The current patch removes this loophole making sure the driver loads
> only if the dt has been initialized.

Drop the reference to DT. It's correctly using generic firmware accessors
so should work fine with ACPI PRP0001 for example as well as DT.

Usually we just refer to the need for firmware properties.
There are lots of places they could be coming from.

> 
> Signed-off-by: Petre Rodan <petre.rodan@subdimension.ro>
> Signed-off-by: Andreas Klinger <ak@it-klinger.de>
> ---
>  drivers/iio/pressure/mprls0025pa.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/iio/pressure/mprls0025pa.c b/drivers/iio/pressure/mprls0025pa.c
> index 233cc1dc38ad..63c46592956f 100644
> --- a/drivers/iio/pressure/mprls0025pa.c
> +++ b/drivers/iio/pressure/mprls0025pa.c
> @@ -375,11 +375,8 @@ static int mpr_probe(struct i2c_client *client)
>  				"honeywell,transfer-function %d invalid\n",
>  								data->function);
>  	} else {

I'd prefer the condition flipped even though patch will be more noisy.

	if (!dev_fwnode(dev))
		return dev_err_probe()...

	...

As end result will be more readable.

> -		/* when loaded as i2c device we need to use default values */
> -		dev_notice(dev, "firmware node not found; using defaults\n");
> -		data->pmin = 0;
> -		data->pmax = 172369; /* 25 psi */
> -		data->function = MPR_FUNCTION_A;
> +		return dev_err_probe(dev, -EINVAL,
> +				  "driver needs to be initialized in the dt\n");
>  	}
> 
>  	data->outmin = mpr_func_spec[data->function].output_min;
> --
> 2.41.0
> 

Re: [PATCH v2 08/10] iio: pressure: mprls0025pa.c refactor

[Jonathan Cameron]

On Sun, 24 Dec 2023 16:34:53 +0200
Petre Rodan <petre.rodan@subdimension.ro> wrote:

> Refactor driver by splitting the code into core and i2c.
> 
> Seemingly redundant read/write function parameters are required for
> compatibility with the SPI driver.
> 
> Signed-off-by: Petre Rodan <petre.rodan@subdimension.ro>
> Signed-off-by: Andreas Klinger <ak@it-klinger.de>

A few minor comments inline.

Thanks,

Jonathan


> diff --git a/drivers/iio/pressure/mprls0025pa.c b/drivers/iio/pressure/mprls0025pa.c
> index e14cdee7989f..cb5d6c0cca7e 100644
> --- a/drivers/iio/pressure/mprls0025pa.c
> +++ b/drivers/iio/pressure/mprls0025pa.c
...

> -static int mpr_probe(struct i2c_client *client)
> +int mpr_common_probe(struct device *dev, const struct mpr_ops *ops, int irq)
>  {
>  	int ret;
>  	struct mpr_data *data;
>  	struct iio_dev *indio_dev;
> -	struct device *dev = &client->dev;
>  	s64 scale, offset;
>  	u32 func;
> 
> -	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");
> +		return -ENOMEM;
> 
>  	data = iio_priv(indio_dev);
> -	data->client = client;
> -	data->irq = client->irq;
> +	data->dev = dev;
> +	data->ops = ops;
> +	data->irq = irq;
> 
>  	mutex_init(&data->lock);
>  	init_completion(&data->completion);
> @@ -350,32 +284,36 @@ static int mpr_probe(struct i2c_client *client)
>  		return dev_err_probe(dev, ret,
>  				"can't get and enable vdd supply\n");
> 
> -	if (dev_fwnode(dev)) {

So you now rely on device_property_read_u32() failing I guess to cover this
which is fine.  However do that in the earlier patch instead of burying that
change in here.

Should make it easier to tell what changed here as well.


> -		ret = device_property_read_u32(dev, "honeywell,pmin-pascal",
> +	ret = data->ops->init(data->dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = device_property_read_u32(dev,
> +				       "honeywell,transfer-function", &func);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +			     "honeywell,transfer-function could not be read\n");
> +	data->function = func - 1;
> +	if (data->function > MPR_FUNCTION_C)
> +		return dev_err_probe(dev, -EINVAL,
> +				     "honeywell,transfer-function %d invalid\n",
> +				     data->function);
> +
> +	ret = device_property_read_u32(dev, "honeywell,pmin-pascal",
>  					       &data->pmin);
> -		if (ret)
> -			return dev_err_probe(dev, ret,
> +	if (ret)
> +		return dev_err_probe(dev, ret,
>  				   "honeywell,pmin-pascal could not be read\n");
> 
> -		ret = device_property_read_u32(dev, "honeywell,pmax-pascal",
> -					       &data->pmax);
> -		if (ret)
> -			return dev_err_probe(dev, ret,
> +	ret = device_property_read_u32(dev, "honeywell,pmax-pascal",
> +				       &data->pmax);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
>  				   "honeywell,pmax-pascal could not be read\n");
> -		ret = device_property_read_u32(dev,
> -				"honeywell,transfer-function", &func);
> -		if (ret)
> -			return dev_err_probe(dev, ret,
> -				"honeywell,transfer-function could not be read\n");
> -		data->function = func - 1;
> -		if (data->function > MPR_FUNCTION_C)
> -			return dev_err_probe(dev, -EINVAL,
> -				"honeywell,transfer-function %d invalid\n",
> -								data->function);
> -	} else {
> +
> +	if (data->pmin >= data->pmax)
>  		return dev_err_probe(dev, -EINVAL,
> -				  "driver needs to be initialized in the dt\n");
> -	}
> +				     "pressure limits are invalid\n");
> 
>  	data->outmin = mpr_func_spec[data->function].output_min;
>  	data->outmax = mpr_func_spec[data->function].output_max;
> @@ -394,7 +332,7 @@ static int mpr_probe(struct i2c_client *client)
> 
>  	if (data->irq > 0) {
>  		ret = devm_request_irq(dev, data->irq, mpr_eoc_handler,
> -				IRQF_TRIGGER_RISING, client->name, data);
> +				      IRQF_TRIGGER_RISING, dev_name(dev), data);

Even though you'll change it again here, would have been nice to have
the alignment fixed in the earlier patch then the code update here.

>  		if (ret)
>  			return dev_err_probe(dev, ret,
>  					  "request irq %d failed\n", data->irq);
> @@ -421,29 +359,8 @@ static int mpr_probe(struct i2c_client *client)
> 
>  	return 0;
>  }


...


> diff --git a/drivers/iio/pressure/mprls0025pa_i2c.c b/drivers/iio/pressure/mprls0025pa_i2c.c
> new file mode 100644
> index 000000000000..89d6a206192b
> --- /dev/null
> +++ b/drivers/iio/pressure/mprls0025pa_i2c.c
> @@ -0,0 +1,98 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * MPRLS0025PA - Honeywell MicroPressure pressure sensor series driver
> + *
> + * Copyright (c) Andreas Klinger <ak@it-klinger.de>
> + *
> + * Data sheet:
> + *  https://prod-edam.honeywell.com/content/dam/honeywell-edam/sps/siot/en-us/products/sensors/pressure-sensors/board-mount-pressure-sensors/micropressure-mpr-series/documents/sps-siot-mpr-series-datasheet-32332628-ciid-172626.pdf
> + */
> +
> +#include <linux/errno.h>
> +#include <linux/i2c.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +
> +#include <linux/iio/iio.h>

Why include this?  Can't see an IIO specific stuff in here.


> +
> +#include "mprls0025pa.h"
> +
> +static int mpr_i2c_init(struct device *unused)
> +{
> +	return 0;
> +}
> +
> +static int mpr_i2c_read(struct mpr_data *data, const u8 unused, const u8 cnt)
> +{
> +	int ret;
> +	struct i2c_client *client = to_i2c_client(data->dev);
> +
> +	if (cnt > MPR_MEASUREMENT_RD_SIZE)
> +		return -EOVERFLOW;
> +
> +	memset(data->buffer, 0, MPR_MEASUREMENT_RD_SIZE);
> +	ret = i2c_master_recv(client, data->buffer, cnt);
> +	if (ret != cnt) {
> +		return -EIO;
As below.

> +	}
> +
> +	return 0;
> +}
> +
> +static int mpr_i2c_write(struct mpr_data *data, const u8 cmd, const u8 unused)
> +{
> +	int ret;
> +	struct i2c_client *client = to_i2c_client(data->dev);
> +	u8 wdata[MPR_PKT_SYNC_LEN];
> +
> +	memset(wdata, 0, sizeof(wdata));
> +	wdata[0] = cmd;
> +
> +	ret = i2c_master_send(client, wdata, MPR_PKT_SYNC_LEN);
> +	if (ret != MPR_PKT_SYNC_LEN) {

No {} as per Coding Style docs for single statement blocks like this.

Ideally we tend to handle ret < 0 separately from ret != MPR_PKT_SYNC_LEN
as then we don't eat a possible more useful error code.



> +		return -EIO;
> +	}
> +
> +	return 0;
> +}

Re: [PATCH v2 10/10] iio: pressure: mprls0025pa.c add SPI driver

[Jonathan Cameron]

On Sun, 24 Dec 2023 16:34:55 +0200
Petre Rodan <petre.rodan@subdimension.ro> wrote:

> Add SPI component of the driver.
> 
> Tested with mprls0015pa0000sa in spi mode on BeagleBone Black on
> slightly patched 6.7.0-rc6 mainline.
> 
> Tested with mprls0025pa in i2c mode on BeagleBone Black with togreg
> branch on
> git://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git
> (tag: iio-for-6.8a)
> 
> Signed-off-by: Petre Rodan <petre.rodan@subdimension.ro>
> Tested-by: Andreas Klinger <ak@it-klinger.de>
This and previous patch look fine to me.

Thanks,

Jonathan

Re: [PATCH v2 01/10] dt-bindings: iio: pressure: honeywell,mprls0025pa.yaml fix

[Petre Rodan]

[-- Attachment #1: Type: text/plain, Size: 2005 bytes --]


Hello Cameron,

On Tue, Dec 26, 2023 at 04:28:39PM +0000, Jonathan Cameron wrote:
> On Sun, 24 Dec 2023 16:34:46 +0200
> Petre Rodan <petre.rodan@subdimension.ro> wrote:
> 
> > Define enum inside the honeywell,transfer-function property block.
> > 
> > Set the correct irq edge in the example block.
> > Based on the datasheet, in table 13 on page 11:
> > "End-of-conversion indicator: This pin is set high when a measurement
> > and calculation have been completed and the data is ready to be
> > clocked out"
> > 
> > Add description on End-of-conversion interrupt.
> > 
> > Signed-off-by: Petre Rodan <petre.rodan@subdimension.ro>
> > Signed-off-by: Andreas Klinger <ak@it-klinger.de>
> What's the relationship between Andreas and this patch?
> 
> Petre seems to have sent it so either Andreas should have a Co-authored-by or
> should be the author... Or not there at all

Andreas has written this driver as it is in the mainline tree right now and he
is marked as a maintainer for it.
A month back I told him about the enum off-by-one problem and also about my plan
of adding more features to the driver.
He was happy to accept my code and once I sent v1 of this patch to the list has
asked to work together for the v2 you see here. This has helped with cleaning up
the code. He requested the additional 'Signed-off-by' tag, but if you have a more
explicit one I will happily use it. 'Co-developed-by' it is.

He also owns an i2c version of the sensor so he was able to make sure that the
original half of the driver still works after my refactor, hence the 'Tested-by'
tag in the last patch.

please tell me how do the 'fixes'/feature/improvement tags/keywords look like?
are these to be added on the subject line, or should they reside near my
'Signed-off-by' inside the email body? I probably missed the documentation where
these are covered :)

also, should I add a 'Reviewed-by:' you for 09/10 and 10/10 (the last two patches)?

best regards,
peter

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 08/10] iio: pressure: mprls0025pa.c refactor

[Petre Rodan]

[-- Attachment #1: Type: text/plain, Size: 1224 bytes --]


Hi Jonathan,

On Tue, Dec 26, 2023 at 04:49:22PM +0000, Jonathan Cameron wrote:
> >  	if (data->irq > 0) {
> >  		ret = devm_request_irq(dev, data->irq, mpr_eoc_handler,
> > -				IRQF_TRIGGER_RISING, client->name, data);
> > +				      IRQF_TRIGGER_RISING, dev_name(dev), data);
> 
> Even though you'll change it again here, would have been nice to have
> the alignment fixed in the earlier patch then the code update here.

I tried this, but due to the fact that the line has to be right-aligned to
column 80 we will still see a whitespace difference due to the length diff of
the name-related argument.

> > +++ b/drivers/iio/pressure/mprls0025pa_i2c.c
> > +
> > +#include <linux/iio/iio.h>
> 
> Why include this?  Can't see an IIO specific stuff in here.

tried to remove it and

  CC [M]  mprls0025pa_i2c.o
mprls0025pa.h:89:63: error: 'IIO_DMA_MINALIGN' undeclared here (not in a function); did you mean 'ARCH_DMA_MINALIGN'?
   89 |         u8          buffer[MPR_MEASUREMENT_RD_SIZE] __aligned(IIO_DMA_MINALIGN);

I guess it makes more sense to move it to the .h file, where buffer[] is defined.

everything else will be fixed as per your feedback.

thanks,
peter

-- 
petre rodan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 04/10] iio: pressure: mprls0025pa.c fix off-by-one enum

[Andy Shevchenko]

On Sun, Dec 24, 2023 at 04:34:49PM +0200, Petre Rodan wrote:
> Fix off-by-one error in transfer-function property.
> The honeywell,transfer-function property takes values between 1-3 so
> make sure the proper enum gets used.

> Signed-off-by: Petre Rodan <petre.rodan@subdimension.ro>
> Signed-off-by: Andreas Klinger <ak@it-klinger.de>

Submitter's SoB always goes last, this is written in Submitting Patches document.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 07/10] iio: pressure: mprls0025pa.c whitespace cleanup

[Andy Shevchenko]

On Sun, Dec 24, 2023 at 04:34:52PM +0200, Petre Rodan wrote:
> Fix indentation and whitespace in code that will not get refactored.
> 
> Make URL inside comment copy-paste friendly.

>  			return dev_err_probe(dev, ret,
> -				"honeywell,pmin-pascal could not be read\n");
> +				   "honeywell,pmin-pascal could not be read\n");

As done elsewhere, here and in other similar places fix the indentation
by making first character on the latter line to be in the same column as
the first character after the opening parenthesis.

			return dev_err_probe(dev, ret,
					     "honeywell,pmin-pascal could not be read\n");

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 08/10] iio: pressure: mprls0025pa.c refactor

[Andy Shevchenko]

On Wed, Dec 27, 2023 at 04:33:37PM +0200, Petre Rodan wrote:
> On Tue, Dec 26, 2023 at 04:49:22PM +0000, Jonathan Cameron wrote:

,,,

> > >  		ret = devm_request_irq(dev, data->irq, mpr_eoc_handler,
> > > -				IRQF_TRIGGER_RISING, client->name, data);
> > > +				      IRQF_TRIGGER_RISING, dev_name(dev), data);
> > 
> > Even though you'll change it again here, would have been nice to have
> > the alignment fixed in the earlier patch then the code update here.
> 
> I tried this, but due to the fact that the line has to be right-aligned to
> column 80 we will still see a whitespace difference due to the length diff of
> the name-related argument.

You can split in the previous patch accordingly, so data comes to a new line.

...

> > > +#include <linux/iio/iio.h>
> > 
> > Why include this?  Can't see an IIO specific stuff in here.
> 
> tried to remove it and
> 
>   CC [M]  mprls0025pa_i2c.o
> mprls0025pa.h:89:63: error: 'IIO_DMA_MINALIGN' undeclared here (not in a function); did you mean 'ARCH_DMA_MINALIGN'?
>    89 |         u8          buffer[MPR_MEASUREMENT_RD_SIZE] __aligned(IIO_DMA_MINALIGN);

> I guess it makes more sense to move it to the .h file, where buffer[] is defined.

Yes, C-code and especially headers should follow IWYI principle. The real user
of that definition is _the header_ file, and not C in this case.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 07/10] iio: pressure: mprls0025pa.c whitespace cleanup

[Petre Rodan]

[-- Attachment #1: Type: text/plain, Size: 1259 bytes --]

On Wed, Dec 27, 2023 at 06:34:25PM +0200, Andy Shevchenko wrote:
> On Sun, Dec 24, 2023 at 04:34:52PM +0200, Petre Rodan wrote:
> > Fix indentation and whitespace in code that will not get refactored.
> > 
> > Make URL inside comment copy-paste friendly.
> 
> >  			return dev_err_probe(dev, ret,
> > -				"honeywell,pmin-pascal could not be read\n");
> > +				   "honeywell,pmin-pascal could not be read\n");
> 
> As done elsewhere, here and in other similar places fix the indentation
> by making first character on the latter line to be in the same column as
> the first character after the opening parenthesis.

I triple-checked that I am following the max 80 column rule, the parenthesis
rule and the 'do not split printk messages' rules in all my code in these 10 patches.
precisely so I don't get feedback like this one.
if the parenthesis rule makes the line longer then 80 chars I right-align to
column 80 as seen above.
that is what I understand from the latest coding style document and that is what
I will follow.

in this particular case if I were to ignore the 80 column rule we would end up on
column 90 if I were to follow your feedback (open parenthesis is at column 45
and the error takes 45 chars more).

peter


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 01/10] dt-bindings: iio: pressure: honeywell,mprls0025pa.yaml fix

[Jonathan Cameron]

On Wed, 27 Dec 2023 09:11:35 +0200
Petre Rodan <petre.rodan@subdimension.ro> wrote:

> Hello Cameron,
> 
> On Tue, Dec 26, 2023 at 04:28:39PM +0000, Jonathan Cameron wrote:
> > On Sun, 24 Dec 2023 16:34:46 +0200
> > Petre Rodan <petre.rodan@subdimension.ro> wrote:
> >   
> > > Define enum inside the honeywell,transfer-function property block.
> > > 
> > > Set the correct irq edge in the example block.
> > > Based on the datasheet, in table 13 on page 11:
> > > "End-of-conversion indicator: This pin is set high when a measurement
> > > and calculation have been completed and the data is ready to be
> > > clocked out"
> > > 
> > > Add description on End-of-conversion interrupt.
> > > 
> > > Signed-off-by: Petre Rodan <petre.rodan@subdimension.ro>
> > > Signed-off-by: Andreas Klinger <ak@it-klinger.de>  
> > What's the relationship between Andreas and this patch?
> > 
> > Petre seems to have sent it so either Andreas should have a Co-authored-by or
> > should be the author... Or not there at all  
> 
> Andreas has written this driver as it is in the mainline tree right now and he
> is marked as a maintainer for it.
> A month back I told him about the enum off-by-one problem and also about my plan
> of adding more features to the driver.
> He was happy to accept my code and once I sent v1 of this patch to the list has
> asked to work together for the v2 you see here. This has helped with cleaning up
> the code. He requested the additional 'Signed-off-by' tag, but if you have a more
> explicit one I will happily use it. 'Co-developed-by' it is.

Yup, needs combination of Co-developed-by and a Sign off for this case as in
effect both of you wrote the code.

> 
> He also owns an i2c version of the sensor so he was able to make sure that the
> original half of the driver still works after my refactor, hence the 'Tested-by'
> tag in the last patch.

That's fine.

> 
> please tell me how do the 'fixes'/feature/improvement tags/keywords look like?
> are these to be added on the subject line, or should they reside near my
> 'Signed-off-by' inside the email body? I probably missed the documentation where
> these are covered :)
See https://elixir.bootlin.com/linux/latest/source/Documentation/process/submitting-patches.rst
for lots of detail.  The fixes tag has a particular format and goes in the main
tag block. Lots of examples in tree so can just take a look at the git history for
how to apply these rules in practice.

> 
> also, should I add a 'Reviewed-by:' you for 09/10 and 10/10 (the last two patches)?
No. Tags should be explicitly given.  I tend not to give RB for stuff I'll pick up
because they will have my SoB anyway as the person who applies the patches
and that includes reviewing.

In this case, Andreas asked for a tag to be included whereas I have not.

Jonathan


> 
> best regards,
> peter

Re: [PATCH v2 07/10] iio: pressure: mprls0025pa.c whitespace cleanup

[Jonathan Cameron]

On Wed, 27 Dec 2023 19:39:28 +0200
Petre Rodan <petre.rodan@subdimension.ro> wrote:

> On Wed, Dec 27, 2023 at 06:34:25PM +0200, Andy Shevchenko wrote:
> > On Sun, Dec 24, 2023 at 04:34:52PM +0200, Petre Rodan wrote:  
> > > Fix indentation and whitespace in code that will not get refactored.
> > > 
> > > Make URL inside comment copy-paste friendly.  
> >   
> > >  			return dev_err_probe(dev, ret,
> > > -				"honeywell,pmin-pascal could not be read\n");
> > > +				   "honeywell,pmin-pascal could not be read\n");  
> > 
> > As done elsewhere, here and in other similar places fix the indentation
> > by making first character on the latter line to be in the same column as
> > the first character after the opening parenthesis.  
> 
> I triple-checked that I am following the max 80 column rule, the parenthesis
> rule and the 'do not split printk messages' rules in all my code in these 10 patches.
> precisely so I don't get feedback like this one.
> if the parenthesis rule makes the line longer then 80 chars I right-align to
> column 80 as seen above.

I'm not aware of (and can't immediately see) anything about right aligning to 80
columns.  It's fine to align it less if line length is long but normally people
go with aligning to one tab more than the start of the block.

> that is what I understand from the latest coding style document and that is what
> I will follow.
> 
> in this particular case if I were to ignore the 80 column rule we would end up on
> column 90 if I were to follow your feedback (open parenthesis is at column 45
> and the error takes 45 chars more).

It's fine to do this in the interests of readability.

People differ in opinion on what constitutes 'significant readability' and I'd
be happy with either a shorter alignment (single tab more than line above)
or going over 80 chars in this case.

Jonathan

> 
> peter
> 

Re: [PATCH v2 08/10] iio: pressure: mprls0025pa.c refactor

[Jonathan Cameron]

On Wed, 27 Dec 2023 18:37:42 +0200
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Wed, Dec 27, 2023 at 04:33:37PM +0200, Petre Rodan wrote:
> > On Tue, Dec 26, 2023 at 04:49:22PM +0000, Jonathan Cameron wrote:  
> 
> ,,,
> 
> > > >  		ret = devm_request_irq(dev, data->irq, mpr_eoc_handler,
> > > > -				IRQF_TRIGGER_RISING, client->name, data);
> > > > +				      IRQF_TRIGGER_RISING, dev_name(dev), data);  
> > > 
> > > Even though you'll change it again here, would have been nice to have
> > > the alignment fixed in the earlier patch then the code update here.  
> > 
> > I tried this, but due to the fact that the line has to be right-aligned to
> > column 80 we will still see a whitespace difference due to the length diff of
> > the name-related argument.  
> 
> You can split in the previous patch accordingly, so data comes to a new line.
> 
> ...
> 
> > > > +#include <linux/iio/iio.h>  
> > > 
> > > Why include this?  Can't see an IIO specific stuff in here.  
> > 
> > tried to remove it and
> > 
> >   CC [M]  mprls0025pa_i2c.o
> > mprls0025pa.h:89:63: error: 'IIO_DMA_MINALIGN' undeclared here (not in a function); did you mean 'ARCH_DMA_MINALIGN'?
> >    89 |         u8          buffer[MPR_MEASUREMENT_RD_SIZE] __aligned(IIO_DMA_MINALIGN);  
> 
> > I guess it makes more sense to move it to the .h file, where buffer[] is defined.  
> 
> Yes, C-code and especially headers should follow IWYI principle. The real user
> of that definition is _the header_ file, and not C in this case.

Absolutely - it is clear why this should be included from the header file.

Jonathan

> 
> 

Re: [PATCH v2 07/10] iio: pressure: mprls0025pa.c whitespace cleanup

[Andy Shevchenko]

On Wed, Dec 27, 2023 at 07:39:28PM +0200, Petre Rodan wrote:
> On Wed, Dec 27, 2023 at 06:34:25PM +0200, Andy Shevchenko wrote:
> > On Sun, Dec 24, 2023 at 04:34:52PM +0200, Petre Rodan wrote:
> > > Fix indentation and whitespace in code that will not get refactored.
> > > 
> > > Make URL inside comment copy-paste friendly.
> > 
> > >  			return dev_err_probe(dev, ret,
> > > -				"honeywell,pmin-pascal could not be read\n");
> > > +				   "honeywell,pmin-pascal could not be read\n");
> > 
> > As done elsewhere, here and in other similar places fix the indentation
> > by making first character on the latter line to be in the same column as
> > the first character after the opening parenthesis.
> 
> I triple-checked that I am following the max 80 column rule, the parenthesis
> rule and the 'do not split printk messages' rules in all my code in these 10 patches.
> precisely so I don't get feedback like this one.
> if the parenthesis rule makes the line longer then 80 chars I right-align to
> column 80 as seen above.
> that is what I understand from the latest coding style document and that is what
> I will follow.
> 
> in this particular case if I were to ignore the 80 column rule we would end up on
> column 90 if I were to follow your feedback (open parenthesis is at column 45
> and the error takes 45 chars more).

checkpatch has got an exceptional rule _not_ to warn on the long string
literals for 10+ years. It had happened much earlier than 100 relaxation one.

-- 
With Best Regards,
Andy Shevchenko