All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/2] dt-bindings: iio: pressure: add honeywell,hsc030
@ 2023-11-17 16:42 Petre Rodan
  2023-11-17 16:42 ` [PATCH 2/2] iio: pressure: driver for Honeywell HSC/SSC series pressure sensors Petre Rodan
                   ` (2 more replies)
  0 siblings, 3 replies; 24+ messages in thread
From: Petre Rodan @ 2023-11-17 16:42 UTC (permalink / raw)
  To: petre.rodan, linux-kernel, linux-iio, devicetree
  Cc: Conor Dooley, Lars-Peter Clausen, Rob Herring,
	Krzysztof Kozlowski, linux-kernel-mentees, Jonathan Cameron

Adds binding for digital Honeywell TruStability HSC and SSC series
pressure and temperature sensors.

Datasheet:
 [HSC] https://prod-edam.honeywell.com/content/dam/honeywell-edam/sps/siot/en-us/products/sensors/pressure-sensors/board-mount-pressure-sensors/trustability-hsc-series/documents/sps-siot-trustability-hsc-series-high-accuracy-board-mount-pressure-sensors-50099148-a-en-ciid-151133.pdf
 [SSC] https://prod-edam.honeywell.com/content/dam/honeywell-edam/sps/siot/en-us/products/sensors/pressure-sensors/board-mount-pressure-sensors/trustability-ssc-series/documents/sps-siot-trustability-ssc-series-standard-accuracy-board-mount-pressure-sensors-50099533-a-en-ciid-151134.pdf

Signed-off-by: Petre Rodan <petre.rodan@subdimension.ro>
---
 .../iio/pressure/honeywell,hsc030pa.yaml     | 166 ++++++++++++++++++
 1 file changed, 166 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/pressure/honeywell,hsc0030pa.yaml

diff --git a/Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml b/Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml
new file mode 100644
index 000000000000..777710790696
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml
@@ -0,0 +1,166 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/pressure/honeywell,hsc030pa.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Honeywell TruStability HSC and SSC pressure sensor families
+
+maintainers:
+  - Petre Rodan <petre.rodan@subdimension.ro>
+
+description: |
+  support for Honeywell TruStability HSC and SSC digital pressure sensor
+  families.
+
+  These sensors have either an I2C, an SPI or an analog interface. Only the
+  digital versions are supported by this driver.
+
+  There are 118 models with different pressure ranges available in each family.
+  The vendor calls them "HSC series" and "SSC series". All of them have an
+  identical programming model but differ in 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 can either be provided via range_str or
+  in case it's a custom chip via numerical range limits converted to pascals.
+
+  The transfer function defines the ranges of raw conversion values delivered
+  by the sensor. pmin-pascal and pmax-pascal corespond to the minimum and
+  maximum pressure that can be measured.
+
+  Specifications about the devices can be found at:
+  https://prod-edam.honeywell.com/content/dam/honeywell-edam/sps/siot/en-us/products/sensors/pressure-sensors/board-mount-pressure-sensors/trustability-hsc-series/documents/sps-siot-trustability-hsc-series-high-accuracy-board-mount-pressure-sensors-50099148-a-en-ciid-151133.pdf
+  https://prod-edam.honeywell.com/content/dam/honeywell-edam/sps/siot/en-us/products/sensors/pressure-sensors/board-mount-pressure-sensors/trustability-ssc-series/documents/sps-siot-trustability-ssc-series-standard-accuracy-board-mount-pressure-sensors-50099533-a-en-ciid-151134.pdf
+
+properties:
+  compatible:
+    enum:
+      - honeywell,hsc
+      - honeywell,ssc
+
+  reg:
+    maxItems: 1
+
+  honeywell,transfer-function:
+    description: |
+      Transfer function which defines the range of valid values delivered by
+      the sensor.
+      0 - A, 10% to 90% of 2^14
+      1 - B, 5% to 95% of 2^14
+      2 - C, 5% to 85% of 2^14
+      3 - F, 4% to 94% of 2^14
+    $ref: /schemas/types.yaml#/definitions/uint32
+
+  honeywell,range_str:
+    description: |
+      Five character string that defines "pressure range, unit and type"
+      as part of the device nomenclature. In the unlikely case of a custom
+      chip, set to "NA" and provide honeywell,pmin-pascal honeywell,pmax-pascal
+    enum: [ "001BA", "1.6BA", "2.5BA", "004BA", "006BA", "010BA", "1.6MD",
+            "2.5MD", "004MD", "006MD", "010MD", "016MD", "025MD", "040MD",
+            "060MD", "100MD", "160MD", "250MD", "400MD", "600MD", "001BD",
+            "1.6BD", "2.5BD", "004BD", "2.5MG", "004MG", "006MG", "010MG",
+            "016MG", "025MG", "040MG", "060MG", "100MG", "160MG", "250MG",
+            "400MG", "600MG", "001BG", "1.6BG", "2.5BG", "004BG", "006BG",
+            "010BG", "100KA", "160KA", "250KA", "400KA", "600KA", "001GA",
+            "160LD", "250LD", "400LD", "600LD", "001KD", "1.6KD", "2.5KD",
+            "004KD", "006KD", "010KD", "016KD", "025KD", "040KD", "060KD",
+            "100KD", "160KD", "250KD", "400KD", "250LG", "400LG", "600LG",
+            "001KG", "1.6KG", "2.5KG", "004KG", "006KG", "010KG", "016KG",
+            "025KG", "040KG", "060KG", "100KG", "160KG", "250KG", "400KG",
+            "600KG", "001GG", "015PA", "030PA", "060PA", "100PA", "150PA",
+            "0.5ND", "001ND", "002ND", "004ND", "005ND", "010ND", "020ND",
+            "030ND", "001PD", "005PD", "015PD", "030PD", "060PD", "001NG",
+            "002NG", "004NG", "005NG", "010NG", "020NG", "030NG", "001PG",
+            "005PG", "015PG", "030PG", "060PG", "100PG", "150PG", "NA" ]
+    $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,range_str is set to "NA".
+    $ref: /schemas/types.yaml#/definitions/int32
+
+  honeywell,pmax-pascal:
+    description: |
+      Maximum pressure value the sensor can measure in pascal.
+      To be specified only if honeywell,range_str is set to "NA".
+    $ref: /schemas/types.yaml#/definitions/int32
+
+  vdd-supply:
+    description: |
+      Provide VDD power to the sensor (either 3.3V or 5V depending on the chip).
+      Optional, activate only if required by the target board.
+
+  spi-max-frequency:
+    description: SPI clock to be kept between 50 and 800kHz
+
+  clock-frequency:
+    description: i2c clock to be kept between 100 and 400kHz
+
+required:
+  - compatible
+  - reg
+  - honeywell,transfer-function
+  - honeywell,range_str
+  - clock-frequency
+  - spi-max-frequency
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+    i2c {
+        status = "okay";
+        clock-frequency = <400000>;
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        HSCMRNN030PA2A3@28 {
+          status = "okay";
+          compatible = "honeywell,hsc";
+          reg = <0x28>;
+          honeywell,transfer-function = <0>;
+          honeywell,range_str = "030PA";
+          honeywell,pmin-pascal = <0>;
+          honeywell,pmax-pascal = <206850>;
+
+		      //vdd-supply = <&foo>;
+        };
+    };
+
+    spi {
+        # note that MOSI is not required by this sensor
+        status = "okay";
+        #address-cells = <1>;
+        #size-cells = <0>;
+        pinctrl-names = "default";
+        pinctrl-0 = <&spi0_pins>;
+
+        channel@0{
+          status = "disabled";
+          reg = <0>;
+        };
+
+        HSCMLNN100PASA3@0 {
+          status = "okay";
+          compatible = "honeywell,hsc";
+		      reg = <0>;
+		      spi-max-frequency = <800000>;
+
+		      honeywell,transfer-function = <0>;
+		      honeywell,range_str = "100PA";
+
+		      // in case of a custom range, use NA as range_str
+		      // and populate pmin-pascal and pmax-pascal
+		      // with the range limits converted into pascals
+		      //honeywell,range_str = "NA";
+		      //honeywell,pmin-pascal = <0>;
+		      //honeywell,pmax-pascal = <206850>;
+
+		      //vdd-supply = <&foo>;
+	      };
+    };
+
-- 
2.41.0


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

* [PATCH 2/2] iio: pressure: driver for Honeywell HSC/SSC series pressure sensors
  2023-11-17 16:42 [PATCH 1/2] dt-bindings: iio: pressure: add honeywell,hsc030 Petre Rodan
@ 2023-11-17 16:42 ` Petre Rodan
  2023-11-18  5:21   ` [PATCH v2 " kernel test robot
  2023-11-20 12:35   ` [PATCH " Andy Shevchenko
  2023-11-17 17:12 ` [PATCH 1/2] dt-bindings: iio: pressure: add honeywell,hsc030 Rob Herring
  2023-11-17 19:22 ` [PATCH v2 " Petre Rodan
  2 siblings, 2 replies; 24+ messages in thread
From: Petre Rodan @ 2023-11-17 16:42 UTC (permalink / raw)
  To: petre.rodan, linux-kernel, linux-iio
  Cc: Jonathan Cameron, Lars-Peter Clausen, Andy Shevchenko,
	Angel Iglesias, Matti Vaittinen, Andreas Klinger, Rob Herring,
	Krzysztof Kozlowski

Adds driver for Honeywell TruStability HSC and SSC series pressure and
temperature sensors.

Covers i2c and spi-based interfaces. For both series it supports all the
combinations of four transfer functions and all 118 pressure ranges.
In case of a special chip not covered by the nomenclature a custom range
can be specified.

Devices tested:
 HSCMLNN100PASA3 (SPI sensor)
 HSCMRNN030PA2A3 (i2c sensor)

Datasheet:
 [HSC] https://prod-edam.honeywell.com/content/dam/honeywell-edam/sps/siot/en-us/products/sensors/pressure-sensors/board-mount-pressure-sensors/trustability-hsc-series/documents/sps-siot-trustability-hsc-series-high-accuracy-board-mount-pressure-sensors-50099148-a-en-ciid-151133.pdf
 [SSC] https://prod-edam.honeywell.com/content/dam/honeywell-edam/sps/siot/en-us/products/sensors/pressure-sensors/board-mount-pressure-sensors/trustability-ssc-series/documents/sps-siot-trustability-ssc-series-standard-accuracy-board-mount-pressure-sensors-50099533-a-en-ciid-151134.pdf
 [i2c comms] https://prod-edam.honeywell.com/content/dam/honeywell-edam/sps/siot/en-us/products/sensors/pressure-sensors/board-mount-pressure-sensors/common/documents/sps-siot-i2c-comms-digital-output-pressure-sensors-tn-008201-3-en-ciid-45841.pdf

Signed-off-by: Petre Rodan <petre.rodan@subdimension.ro>
---
 MAINTAINERS                         |   7 +
 drivers/iio/pressure/Kconfig        |  22 ++
 drivers/iio/pressure/Makefile       |   3 +
 drivers/iio/pressure/hsc030pa.c     | 402 ++++++++++++++++++++++++++++
 drivers/iio/pressure/hsc030pa.h     |  59 ++++
 drivers/iio/pressure/hsc030pa_i2c.c | 136 ++++++++++
 drivers/iio/pressure/hsc030pa_spi.c | 129 +++++++++
 7 files changed, 758 insertions(+)
 create mode 100644 drivers/iio/pressure/hsc030pa.c
 create mode 100644 drivers/iio/pressure/hsc030pa.h
 create mode 100644 drivers/iio/pressure/hsc030pa_i2c.c
 create mode 100644 drivers/iio/pressure/hsc030pa_spi.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 482d428472e7..cba0d34c504a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -9708,6 +9708,13 @@ S:	Maintained
 F:	Documentation/devicetree/bindings/iio/pressure/honeywell,mprls0025pa.yaml
 F:	drivers/iio/pressure/mprls0025pa.c
 
+HONEYWELL HSC, SSC PRESSURE SENSOR SERIES IIO DRIVER
+M:	Petre Rodan <petre.rodan@subdimension.ro>
+L:	linux-iio@vger.kernel.org
+S:	Maintained
+F:	Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml
+F:	drivers/iio/pressure/hsc030pa*
+
 HOST AP DRIVER
 L:	linux-wireless@vger.kernel.org
 S:	Obsolete
diff --git a/drivers/iio/pressure/Kconfig b/drivers/iio/pressure/Kconfig
index 95efa32e4289..266688802fb3 100644
--- a/drivers/iio/pressure/Kconfig
+++ b/drivers/iio/pressure/Kconfig
@@ -109,6 +109,28 @@ config HP03
 	  To compile this driver as a module, choose M here: the module
 	  will be called hp03.
 
+config HSC030PA
+	tristate "Honeywell HSC/SSC (TruStability pressure sensors series)"
+	depends on (I2C || SPI_MASTER)
+	select HSC030PA_I2C if (I2C)
+	select HSC030PA_SPI if (SPI_MASTER)
+	help
+	  Say Y here to build support for the Honeywell HSC and SSC TruStability
+      pressure and temperature sensor series.
+
+	  To compile this driver as a module, choose M here: the module will be
+	  called hsc030pa.
+
+config HSC030PA_I2C
+	tristate
+	depends on HSC030PA
+	depends on I2C
+
+config HSC030PA_SPI
+	tristate
+	depends on HSC030PA
+	depends on SPI_MASTER
+
 config ICP10100
 	tristate "InvenSense ICP-101xx pressure and temperature sensor"
 	depends on I2C
diff --git a/drivers/iio/pressure/Makefile b/drivers/iio/pressure/Makefile
index 436aec7e65f3..b0f8b94662f2 100644
--- a/drivers/iio/pressure/Makefile
+++ b/drivers/iio/pressure/Makefile
@@ -15,6 +15,9 @@ obj-$(CONFIG_DPS310) += dps310.o
 obj-$(CONFIG_IIO_CROS_EC_BARO) += cros_ec_baro.o
 obj-$(CONFIG_HID_SENSOR_PRESS)   += hid-sensor-press.o
 obj-$(CONFIG_HP03) += hp03.o
+obj-$(CONFIG_HSC030PA) += hsc030pa.o
+obj-$(CONFIG_HSC030PA_I2C) += hsc030pa_i2c.o
+obj-$(CONFIG_HSC030PA_SPI) += hsc030pa_spi.o
 obj-$(CONFIG_ICP10100) += icp10100.o
 obj-$(CONFIG_MPL115) += mpl115.o
 obj-$(CONFIG_MPL115_I2C) += mpl115_i2c.o
diff --git a/drivers/iio/pressure/hsc030pa.c b/drivers/iio/pressure/hsc030pa.c
new file mode 100644
index 000000000000..b6ddfef7ee28
--- /dev/null
+++ b/drivers/iio/pressure/hsc030pa.c
@@ -0,0 +1,402 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Honeywell TruStability HSC Series pressure/temperature sensor
+ *
+ * Copyright (c) 2023 Petre Rodan <petre.rodan@subdimension.ro>
+ *
+ * Datasheet: https://prod-edam.honeywell.com/content/dam/honeywell-edam/sps/siot/en-us/products/sensors/pressure-sensors/board-mount-pressure-sensors/trustability-hsc-series/documents/sps-siot-trustability-hsc-series-high-accuracy-board-mount-pressure-sensors-50099148-a-en-ciid-151133.pdf?download=false
+ */
+
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/init.h>
+#include <linux/math64.h>
+#include <linux/units.h>
+#include <linux/mod_devicetable.h>
+#include <linux/printk.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+#include "hsc030pa.h"
+
+struct hsc_func_spec {
+	u32 output_min;
+	u32 output_max;
+};
+
+static const struct hsc_func_spec hsc_func_spec[] = {
+	[HSC_FUNCTION_A] = {.output_min = 1638, .output_max = 14746}, // 10% - 90% of 2^14
+	[HSC_FUNCTION_B] = {.output_min =  819, .output_max = 15565}, //  5% - 95% of 2^14
+	[HSC_FUNCTION_C] = {.output_min =  819, .output_max = 13926}, //  5% - 85% of 2^14
+	[HSC_FUNCTION_F] = {.output_min =  655, .output_max = 15401}, //  4% - 94% of 2^14
+};
+
+// pressure range for current chip based on the nomenclature
+struct hsc_range_config {
+	char name[HSC_RANGE_STR_LEN];	// 5-char string that defines the range - ie "030PA"
+	s32 pmin;		// minimal pressure in pascals
+	s32 pmax;		// maximum pressure in pascals
+};
+
+// all min max limits have been converted to pascals
+// code generated by scripts/parse_variants_table.sh
+static const struct hsc_range_config hsc_range_config[] = {
+	{.name = "001BA", .pmin =       0, .pmax =  100000 },
+	{.name = "1.6BA", .pmin =       0, .pmax =  160000 },
+	{.name = "2.5BA", .pmin =       0, .pmax =  250000 },
+	{.name = "004BA", .pmin =       0, .pmax =  400000 },
+	{.name = "006BA", .pmin =       0, .pmax =  600000 },
+	{.name = "010BA", .pmin =       0, .pmax = 1000000 },
+	{.name = "1.6MD", .pmin =    -160, .pmax =     160 },
+	{.name = "2.5MD", .pmin =    -250, .pmax =     250 },
+	{.name = "004MD", .pmin =    -400, .pmax =     400 },
+	{.name = "006MD", .pmin =    -600, .pmax =     600 },
+	{.name = "010MD", .pmin =   -1000, .pmax =    1000 },
+	{.name = "016MD", .pmin =   -1600, .pmax =    1600 },
+	{.name = "025MD", .pmin =   -2500, .pmax =    2500 },
+	{.name = "040MD", .pmin =   -4000, .pmax =    4000 },
+	{.name = "060MD", .pmin =   -6000, .pmax =    6000 },
+	{.name = "100MD", .pmin =  -10000, .pmax =   10000 },
+	{.name = "160MD", .pmin =  -16000, .pmax =   16000 },
+	{.name = "250MD", .pmin =  -25000, .pmax =   25000 },
+	{.name = "400MD", .pmin =  -40000, .pmax =   40000 },
+	{.name = "600MD", .pmin =  -60000, .pmax =   60000 },
+	{.name = "001BD", .pmin = -100000, .pmax =  100000 },
+	{.name = "1.6BD", .pmin = -160000, .pmax =  160000 },
+	{.name = "2.5BD", .pmin = -250000, .pmax =  250000 },
+	{.name = "004BD", .pmin = -400000, .pmax =  400000 },
+	{.name = "2.5MG", .pmin =       0, .pmax =     250 },
+	{.name = "004MG", .pmin =       0, .pmax =     400 },
+	{.name = "006MG", .pmin =       0, .pmax =     600 },
+	{.name = "010MG", .pmin =       0, .pmax =    1000 },
+	{.name = "016MG", .pmin =       0, .pmax =    1600 },
+	{.name = "025MG", .pmin =       0, .pmax =    2500 },
+	{.name = "040MG", .pmin =       0, .pmax =    4000 },
+	{.name = "060MG", .pmin =       0, .pmax =    6000 },
+	{.name = "100MG", .pmin =       0, .pmax =   10000 },
+	{.name = "160MG", .pmin =       0, .pmax =   16000 },
+	{.name = "250MG", .pmin =       0, .pmax =   25000 },
+	{.name = "400MG", .pmin =       0, .pmax =   40000 },
+	{.name = "600MG", .pmin =       0, .pmax =   60000 },
+	{.name = "001BG", .pmin =       0, .pmax =  100000 },
+	{.name = "1.6BG", .pmin =       0, .pmax =  160000 },
+	{.name = "2.5BG", .pmin =       0, .pmax =  250000 },
+	{.name = "004BG", .pmin =       0, .pmax =  400000 },
+	{.name = "006BG", .pmin =       0, .pmax =  600000 },
+	{.name = "010BG", .pmin =       0, .pmax = 1000000 },
+	{.name = "100KA", .pmin =       0, .pmax =  100000 },
+	{.name = "160KA", .pmin =       0, .pmax =  160000 },
+	{.name = "250KA", .pmin =       0, .pmax =  250000 },
+	{.name = "400KA", .pmin =       0, .pmax =  400000 },
+	{.name = "600KA", .pmin =       0, .pmax =  600000 },
+	{.name = "001GA", .pmin =       0, .pmax = 1000000 },
+	{.name = "160LD", .pmin =    -160, .pmax =     160 },
+	{.name = "250LD", .pmin =    -250, .pmax =     250 },
+	{.name = "400LD", .pmin =    -400, .pmax =     400 },
+	{.name = "600LD", .pmin =    -600, .pmax =     600 },
+	{.name = "001KD", .pmin =   -1000, .pmax =    1000 },
+	{.name = "1.6KD", .pmin =   -1600, .pmax =    1600 },
+	{.name = "2.5KD", .pmin =   -2500, .pmax =    2500 },
+	{.name = "004KD", .pmin =   -4000, .pmax =    4000 },
+	{.name = "006KD", .pmin =   -6000, .pmax =    6000 },
+	{.name = "010KD", .pmin =  -10000, .pmax =   10000 },
+	{.name = "016KD", .pmin =  -16000, .pmax =   16000 },
+	{.name = "025KD", .pmin =  -25000, .pmax =   25000 },
+	{.name = "040KD", .pmin =  -40000, .pmax =   40000 },
+	{.name = "060KD", .pmin =  -60000, .pmax =   60000 },
+	{.name = "100KD", .pmin = -100000, .pmax =  100000 },
+	{.name = "160KD", .pmin = -160000, .pmax =  160000 },
+	{.name = "250KD", .pmin = -250000, .pmax =  250000 },
+	{.name = "400KD", .pmin = -400000, .pmax =  400000 },
+	{.name = "250LG", .pmin =       0, .pmax =     250 },
+	{.name = "400LG", .pmin =       0, .pmax =     400 },
+	{.name = "600LG", .pmin =       0, .pmax =     600 },
+	{.name = "001KG", .pmin =       0, .pmax =    1000 },
+	{.name = "1.6KG", .pmin =       0, .pmax =    1600 },
+	{.name = "2.5KG", .pmin =       0, .pmax =    2500 },
+	{.name = "004KG", .pmin =       0, .pmax =    4000 },
+	{.name = "006KG", .pmin =       0, .pmax =    6000 },
+	{.name = "010KG", .pmin =       0, .pmax =   10000 },
+	{.name = "016KG", .pmin =       0, .pmax =   16000 },
+	{.name = "025KG", .pmin =       0, .pmax =   25000 },
+	{.name = "040KG", .pmin =       0, .pmax =   40000 },
+	{.name = "060KG", .pmin =       0, .pmax =   60000 },
+	{.name = "100KG", .pmin =       0, .pmax =  100000 },
+	{.name = "160KG", .pmin =       0, .pmax =  160000 },
+	{.name = "250KG", .pmin =       0, .pmax =  250000 },
+	{.name = "400KG", .pmin =       0, .pmax =  400000 },
+	{.name = "600KG", .pmin =       0, .pmax =  600000 },
+	{.name = "001GG", .pmin =       0, .pmax = 1000000 },
+	{.name = "015PA", .pmin =       0, .pmax =  103425 },
+	{.name = "030PA", .pmin =       0, .pmax =  206850 },
+	{.name = "060PA", .pmin =       0, .pmax =  413700 },
+	{.name = "100PA", .pmin =       0, .pmax =  689500 },
+	{.name = "150PA", .pmin =       0, .pmax = 1034250 },
+	{.name = "0.5ND", .pmin =    -125, .pmax =     125 },
+	{.name = "001ND", .pmin =    -249, .pmax =     249 },
+	{.name = "002ND", .pmin =    -498, .pmax =     498 },
+	{.name = "004ND", .pmin =    -996, .pmax =     996 },
+	{.name = "005ND", .pmin =   -1245, .pmax =    1245 },
+	{.name = "010ND", .pmin =   -2491, .pmax =    2491 },
+	{.name = "020ND", .pmin =   -4982, .pmax =    4982 },
+	{.name = "030ND", .pmin =   -7473, .pmax =    7473 },
+	{.name = "001PD", .pmin =   -6895, .pmax =    6895 },
+	{.name = "005PD", .pmin =  -34475, .pmax =   34475 },
+	{.name = "015PD", .pmin = -103425, .pmax =  103425 },
+	{.name = "030PD", .pmin = -206850, .pmax =  206850 },
+	{.name = "060PD", .pmin = -413700, .pmax =  413700 },
+	{.name = "001NG", .pmin =       0, .pmax =     249 },
+	{.name = "002NG", .pmin =       0, .pmax =     498 },
+	{.name = "004NG", .pmin =       0, .pmax =     996 },
+	{.name = "005NG", .pmin =       0, .pmax =    1245 },
+	{.name = "010NG", .pmin =       0, .pmax =    2491 },
+	{.name = "020NG", .pmin =       0, .pmax =    4982 },
+	{.name = "030NG", .pmin =       0, .pmax =    7473 },
+	{.name = "001PG", .pmin =       0, .pmax =    6895 },
+	{.name = "005PG", .pmin =       0, .pmax =   34475 },
+	{.name = "015PG", .pmin =       0, .pmax =  103425 },
+	{.name = "030PG", .pmin =       0, .pmax =  206850 },
+	{.name = "060PG", .pmin =       0, .pmax =  413700 },
+	{.name = "100PG", .pmin =       0, .pmax =  689500 },
+	{.name = "150PG", .pmin =       0, .pmax = 1034250 }
+};
+
+/*
+ * the first two bits from the first byte contain a status code
+ *
+ * 00 - normal operation, valid data
+ * 01 - device in hidden factory command mode
+ * 10 - stale data
+ * 11 - diagnostic condition
+ *
+ * function returns 1 only if both bits are zero
+ */
+static bool hsc_measurement_is_valid(struct hsc_data *data)
+{
+	if (data->buffer[0] & 0xc0)
+		return 0;
+
+	return 1;
+}
+
+static int hsc_get_measurement(struct hsc_data *data)
+{
+	const struct hsc_chip_data *chip = data->chip;
+	int ret;
+
+	/* don't bother sensor more than once a second */
+	if (!time_after(jiffies, data->last_update + HZ))
+		return data->is_valid ? 0 : -EAGAIN;
+
+	data->is_valid = false;
+	data->last_update = jiffies;
+
+	ret = data->xfer(data);
+
+	if (ret < 0)
+		return ret;
+
+	ret = chip->valid(data);
+	if (!ret)
+		return -EAGAIN;
+
+	data->is_valid = true;
+
+	return 0;
+}
+
+/*
+ * 4 bytes are read, the dissection looks like
+ *
+ * .  0  .  1  .  2  .  3  .  4  .  5  .  6  .  7  .
+ * byte 0:
+ * |  s1 |  s0 | b13 | b12 | b11 | b10 |  b9 |  b8 |
+ * | status    | bridge data (pressure) MSB        |
+ * byte 1:
+ * |  b7 |  b6 |  b5 |  b4 |  b3 |  b2 |  b1 |  b0 |
+ * | bridge data (pressure) LSB                    |
+ * byte 2:
+ * | t10 |  t9 |  t8 |  t7 |  t6 |  t5 |  t4 |  t3 |
+ * | temperature data MSB                          |
+ * byte 3:
+ * |  t2 |  t1 |  t0 |  X  |  X  |  X  |  X  |  X  |
+ * | temperature LSB | ignore                      |
+ *
+ * .  0  .  1  .  2  .  3  .  4  .  5  .  6  .  7  .
+ */
+
+static int hsc_read_raw(struct iio_dev *indio_dev,
+			struct iio_chan_spec const *channel, int *val,
+			int *val2, long mask)
+{
+	struct hsc_data *data = iio_priv(indio_dev);
+	int ret = -EINVAL;
+
+	switch (mask) {
+
+	case IIO_CHAN_INFO_RAW:
+		mutex_lock(&data->lock);
+		ret = hsc_get_measurement(data);
+		mutex_unlock(&data->lock);
+
+		if (ret)
+			return ret;
+
+		switch (channel->type) {
+		case IIO_PRESSURE:
+			*val =
+			    ((data->buffer[0] & 0x3f) << 8) + data->buffer[1];
+			return IIO_VAL_INT;
+		case IIO_TEMP:
+			*val =
+			    (data->buffer[2] << 3) +
+			    ((data->buffer[3] & 0xe0) >> 5);
+			ret = 0;
+			if (!ret)
+				return IIO_VAL_INT;
+			break;
+		default:
+			return -EINVAL;
+		}
+		break;
+
+/**
+ *	IIO ABI expects
+ *	value = (conv + offset) * scale
+ *
+ *	datasheet provides the following formula for determining the temperature
+ *	temp[C] = conv * a + b
+ *        where a = 200/2047; b = -50
+ *
+ *	temp[C] = (conv + (b/a)) * a * (1000)
+ *      =>
+ *	scale = a * 1000 = .097703957 * 1000 = 97.703957
+ *	offset = b/a = -50 / .097703957 = -50000000 / 97704
+ *
+ *	based on the datasheet
+ *	pressure = (conv - HSC_OUTPUT_MIN) * Q + Pmin =
+ *	           ((conv - HSC_OUTPUT_MIN) + Pmin/Q) * Q
+ *	=>
+ *	scale = Q = (Pmax - Pmin) / (HSC_OUTPUT_MAX - HSC_OUTPUT_MIN)
+ *	offset = Pmin/Q = Pmin * (HSC_OUTPUT_MAX - HSC_OUTPUT_MIN) / (Pmax - Pmin)
+ */
+
+	case IIO_CHAN_INFO_SCALE:
+		switch (channel->type) {
+		case IIO_TEMP:
+			*val = 97;
+			*val2 = 703957;
+			return IIO_VAL_INT_PLUS_MICRO;
+		case IIO_PRESSURE:
+			*val = data->p_scale;
+			*val2 = data->p_scale_nano;
+			return IIO_VAL_INT_PLUS_NANO;
+		default:
+			return -EINVAL;
+		}
+		break;
+
+	case IIO_CHAN_INFO_OFFSET:
+		switch (channel->type) {
+		case IIO_TEMP:
+			*val = -50000000;
+			*val2 = 97704;
+			return IIO_VAL_FRACTIONAL;
+		case IIO_PRESSURE:
+			*val = data->p_offset;
+			*val2 = data->p_offset_nano;
+			return IIO_VAL_INT_PLUS_NANO;
+		default:
+			return -EINVAL;
+		}
+	}
+
+	return ret;
+}
+
+static const struct iio_chan_spec hsc_channels[] = {
+	{
+	 .type = IIO_PRESSURE,
+	 .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+	 BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_OFFSET)
+	 },
+	{
+	 .type = IIO_TEMP,
+	 .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+	 BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_OFFSET)
+	 },
+};
+
+static const struct iio_info hsc_info = {
+	.read_raw = hsc_read_raw,
+};
+
+static const struct hsc_chip_data hsc_chip = {
+	.valid = hsc_measurement_is_valid,
+	.channels = hsc_channels,
+	.num_channels = ARRAY_SIZE(hsc_channels),
+};
+
+int hsc_probe(struct iio_dev *indio_dev, struct device *dev,
+	      const char *name, int type)
+{
+	struct hsc_data *hsc;
+	u64 tmp;
+	int index;
+	int found = 0;
+
+	hsc = iio_priv(indio_dev);
+
+	hsc->last_update = jiffies - HZ;
+	hsc->chip = &hsc_chip;
+
+	if (strcasecmp(hsc->range_str, "na") != 0) {
+		// chip should be defined in the nomenclature
+		for (index = 0; index < ARRAY_SIZE(hsc_range_config); index++) {
+			if (strcasecmp
+			    (hsc_range_config[index].name,
+			     hsc->range_str) == 0) {
+				hsc->pmin = hsc_range_config[index].pmin;
+				hsc->pmax = hsc_range_config[index].pmax;
+				found = 1;
+				break;
+			}
+		}
+		if (hsc->pmin == hsc->pmax || !found)
+			return dev_err_probe(dev, -EINVAL,
+					     "honeywell,range_str is invalid\n");
+	}
+
+	hsc->outmin = hsc_func_spec[hsc->function].output_min;
+	hsc->outmax = hsc_func_spec[hsc->function].output_max;
+
+	// multiply with MICRO and then divide by NANO since the output needs
+	// to be in KPa as per IIO ABI requirement
+	tmp = div_s64(((s64) (hsc->pmax - hsc->pmin)) * MICRO,
+		      (hsc->outmax - hsc->outmin));
+	hsc->p_scale = div_s64_rem(tmp, NANO, &hsc->p_scale_nano);
+	tmp =
+	    div_s64(((s64) hsc->pmin * (s64) (hsc->outmax - hsc->outmin)) *
+		    MICRO, hsc->pmax - hsc->pmin);
+	hsc->p_offset =
+	    div_s64_rem(tmp, NANO, &hsc->p_offset_nano) - hsc->outmin;
+
+	mutex_init(&hsc->lock);
+	indio_dev->name = name;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->info = &hsc_info;
+	indio_dev->channels = hsc->chip->channels;
+	indio_dev->num_channels = hsc->chip->num_channels;
+
+	return devm_iio_device_register(dev, indio_dev);
+}
+EXPORT_SYMBOL_NS(hsc_probe, IIO_HONEYWELL_HSC);
+
+void hsc_remove(struct iio_dev *indio_dev)
+{
+	iio_device_unregister(indio_dev);
+}
+EXPORT_SYMBOL_NS(hsc_remove, IIO_HONEYWELL_HSC);
+
+MODULE_AUTHOR("Petre Rodan <petre.rodan@subdimension.ro>");
+MODULE_DESCRIPTION("Honeywell HSC and SSC pressure sensor core driver");
+MODULE_LICENSE("GPL");
diff --git a/drivers/iio/pressure/hsc030pa.h b/drivers/iio/pressure/hsc030pa.h
new file mode 100644
index 000000000000..bc2a4877465b
--- /dev/null
+++ b/drivers/iio/pressure/hsc030pa.h
@@ -0,0 +1,59 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Honeywell TruStability HSC Series pressure/temperature sensor
+ *
+ * Copyright (c) 2023 Petre Rodan <petre.rodan@subdimension.ro>
+ *
+ */
+
+#ifndef _HSC030PA_H
+#define _HSC030PA_H
+
+#define  HSC_REG_MEASUREMENT_RD_SIZE  4         // get all conversions in one go since transfers are not address-based
+#define            HSC_RANGE_STR_LEN  6
+
+struct hsc_chip_data;
+
+struct hsc_data {
+	void *client;                           // either i2c or spi kernel interface struct for current dev
+	const struct hsc_chip_data *chip;
+	struct mutex lock;                      // lock protecting chip reads
+	int (*xfer)(struct hsc_data *data);    // function that implements the chip reads
+	bool is_valid;                          // false if last transfer has failed
+	unsigned long last_update;              // time of last successful conversion
+	u8 buffer[HSC_REG_MEASUREMENT_RD_SIZE]; // raw conversion data
+	char range_str[HSC_RANGE_STR_LEN];	// range as defined by the chip nomenclature - ie "030PA" or "NA"
+	s32 pmin;                               // min pressure limit
+	s32 pmax;                               // max pressure limit
+	u32 outmin;                             // minimum raw pressure in counts (based on transfer function)
+	u32 outmax;                             // maximum raw pressure in counts (based on transfer function)
+	u32 function;                           // transfer function
+	s64 p_scale;                            // pressure scale
+	s32 p_scale_nano;                       // pressure scale, decimal places
+	s64 p_offset;                           // pressure offset
+	s32 p_offset_nano;                      // pressure offset, decimal places
+};
+
+struct hsc_chip_data {
+	bool (*valid)(struct hsc_data *data);  // function that checks the two status bits
+	const struct iio_chan_spec *channels;   // channel specifications
+	u8 num_channels;                        // pressure and temperature channels
+};
+
+enum hsc_func_id {
+	HSC_FUNCTION_A,
+	HSC_FUNCTION_B,
+	HSC_FUNCTION_C,
+	HSC_FUNCTION_F
+};
+
+enum hsc_variant {
+	HSC,
+	SSC
+};
+
+int hsc_probe(struct iio_dev *indio_dev, struct device *dev,
+	      const char *name, int type);
+void hsc_remove(struct iio_dev *indio_dev);
+
+#endif
diff --git a/drivers/iio/pressure/hsc030pa_i2c.c b/drivers/iio/pressure/hsc030pa_i2c.c
new file mode 100644
index 000000000000..8d3fb174285a
--- /dev/null
+++ b/drivers/iio/pressure/hsc030pa_i2c.c
@@ -0,0 +1,136 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Honeywell TruStability HSC Series pressure/temperature sensor
+ *
+ * Copyright (c) 2023 Petre Rodan <petre.rodan@subdimension.ro>
+ *
+ * Datasheet: https://prod-edam.honeywell.com/content/dam/honeywell-edam/sps/siot/en-us/products/sensors/pressure-sensors/board-mount-pressure-sensors/trustability-hsc-series/documents/sps-siot-trustability-hsc-series-high-accuracy-board-mount-pressure-sensors-50099148-a-en-ciid-151133.pdf
+ * i2c-related datasheet: https://prod-edam.honeywell.com/content/dam/honeywell-edam/sps/siot/en-us/products/sensors/pressure-sensors/board-mount-pressure-sensors/common/documents/sps-siot-i2c-comms-digital-output-pressure-sensors-tn-008201-3-en-ciid-45841.pdf
+ */
+
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/mod_devicetable.h>
+#include <linux/regulator/consumer.h>
+#include <linux/iio/iio.h>
+#include "hsc030pa.h"
+
+static int hsc_i2c_xfer(struct hsc_data *data)
+{
+	struct i2c_client *client = data->client;
+	struct i2c_msg msg;
+	int ret;
+
+	msg.addr = client->addr;
+	msg.flags = client->flags | I2C_M_RD;
+	msg.len = HSC_REG_MEASUREMENT_RD_SIZE;
+	msg.buf = (char *)&data->buffer;
+
+	ret = i2c_transfer(client->adapter, &msg, 1);
+
+	return (ret == 2) ? 0 : ret;
+}
+
+static int hsc_i2c_probe(struct i2c_client *client,
+			 const struct i2c_device_id *id)
+{
+	struct device *dev = &client->dev;
+	struct iio_dev *indio_dev;
+	struct hsc_data *hsc;
+	const char *range_nom;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*hsc));
+	if (!indio_dev) {
+		dev_err(&client->dev, "Failed to allocate device\n");
+		return -ENOMEM;
+	}
+
+	hsc = iio_priv(indio_dev);
+
+	if (i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
+		hsc->xfer = hsc_i2c_xfer;
+	else
+		return -EOPNOTSUPP;
+
+	ret = devm_regulator_get_enable_optional(dev, "vdd");
+	if (ret == -EPROBE_DEFER)
+		return -EPROBE_DEFER;
+
+	if (!dev_fwnode(dev))
+		return -EOPNOTSUPP;
+
+	ret = device_property_read_u32(dev,
+				       "honeywell,transfer-function",
+				       &hsc->function);
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "honeywell,transfer-function could not be read\n");
+	if (hsc->function > HSC_FUNCTION_F)
+		return dev_err_probe(dev, -EINVAL,
+				     "honeywell,transfer-function %d invalid\n",
+				     hsc->function);
+
+	ret = device_property_read_string(dev,
+					  "honeywell,range_str", &range_nom);
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "honeywell,range_str not defined\n");
+
+	memcpy(hsc->range_str, range_nom, HSC_RANGE_STR_LEN - 1);
+	hsc->range_str[HSC_RANGE_STR_LEN - 1] = 0;
+
+	if (strcasecmp(hsc->range_str, "na") == 0) {
+		// "not available"
+		// we got a custom-range chip not covered by the nomenclature
+		ret = device_property_read_u32(dev,
+					     "honeywell,pmin-pascal",
+					     &hsc->pmin);
+		if (ret)
+			return dev_err_probe(dev, ret,
+					     "honeywell,pmin-pascal could not be read\n");
+		ret = device_property_read_u32(dev,
+					     "honeywell,pmax-pascal",
+					     &hsc->pmax);
+		if (ret)
+			return dev_err_probe(dev, ret,
+					     "honeywell,pmax-pascal could not be read\n");
+	}
+
+	i2c_set_clientdata(client, indio_dev);
+	hsc->client = client;
+
+	return hsc_probe(indio_dev, &client->dev, id->name, id->driver_data);
+}
+
+static const struct of_device_id hsc_i2c_match[] = {
+	{.compatible = "honeywell,hsc",},
+	{.compatible = "honeywell,ssc",},
+	{},
+};
+
+MODULE_DEVICE_TABLE(of, hsc_i2c_match);
+
+static const struct i2c_device_id hsc_i2c_id[] = {
+	{"hsc", HSC},
+	{"ssc", SSC},
+	{}
+};
+
+MODULE_DEVICE_TABLE(i2c, hsc_i2c_id);
+
+static struct i2c_driver hsc_i2c_driver = {
+	.driver = {
+		   .name = "honeywell_hsc",
+		   .of_match_table = hsc_i2c_match,
+		   },
+	.probe = hsc_i2c_probe,
+	.id_table = hsc_i2c_id,
+};
+
+module_i2c_driver(hsc_i2c_driver);
+
+MODULE_AUTHOR("Petre Rodan <petre.rodan@subdimension.ro>");
+MODULE_DESCRIPTION("Honeywell HSC and SSC pressure sensor i2c driver");
+MODULE_LICENSE("GPL");
+MODULE_IMPORT_NS(IIO_HONEYWELL_HSC);
diff --git a/drivers/iio/pressure/hsc030pa_spi.c b/drivers/iio/pressure/hsc030pa_spi.c
new file mode 100644
index 000000000000..e7a9b64ac84b
--- /dev/null
+++ b/drivers/iio/pressure/hsc030pa_spi.c
@@ -0,0 +1,129 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Honeywell TruStability HSC Series pressure/temperature sensor
+ *
+ * Copyright (c) 2023 Petre Rodan <petre.rodan@subdimension.ro>
+ *
+ * Datasheet: https://prod-edam.honeywell.com/content/dam/honeywell-edam/sps/siot/en-us/products/sensors/pressure-sensors/board-mount-pressure-sensors/trustability-hsc-series/documents/sps-siot-trustability-hsc-series-high-accuracy-board-mount-pressure-sensors-50099148-a-en-ciid-151133.pdf
+ */
+
+#include <linux/module.h>
+#include <linux/spi/spi.h>
+#include <linux/iio/iio.h>
+#include <linux/regulator/consumer.h>
+#include "hsc030pa.h"
+
+static int hsc_spi_xfer(struct hsc_data *data)
+{
+	struct spi_transfer xfer = {
+		.tx_buf = NULL,
+		.rx_buf = (char *)&data->buffer,
+		.len = HSC_REG_MEASUREMENT_RD_SIZE,
+	};
+	int ret;
+
+	ret = spi_sync_transfer(data->client, &xfer, 1);
+
+	return ret;
+}
+
+static int hsc_spi_probe(struct spi_device *spi)
+{
+	struct iio_dev *indio_dev;
+	struct hsc_data *hsc;
+	const char *range_nom;
+	int ret;
+	struct device *dev = &spi->dev;
+
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*hsc));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	spi_set_drvdata(spi, indio_dev);
+
+	spi->mode = SPI_MODE_0;
+	spi->max_speed_hz = min(spi->max_speed_hz, 800000U);
+	spi->bits_per_word = 8;
+	ret = spi_setup(spi);
+	if (ret < 0)
+		return ret;
+
+	hsc = iio_priv(indio_dev);
+	hsc->xfer = hsc_spi_xfer;
+	hsc->client = spi;
+
+	ret = devm_regulator_get_enable_optional(dev, "vdd");
+	if (ret == -EPROBE_DEFER)
+		return -EPROBE_DEFER;
+
+	ret = device_property_read_u32(dev,
+				       "honeywell,transfer-function",
+				       &hsc->function);
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "honeywell,transfer-function could not be read\n");
+	if (hsc->function > HSC_FUNCTION_F)
+		return dev_err_probe(dev, -EINVAL,
+				     "honeywell,transfer-function %d invalid\n",
+				     hsc->function);
+
+	ret =
+	    device_property_read_string(dev, "honeywell,range_str", &range_nom);
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "honeywell,range_str not defined\n");
+
+	// minimal input sanitization
+	memcpy(hsc->range_str, range_nom, HSC_RANGE_STR_LEN - 1);
+	hsc->range_str[HSC_RANGE_STR_LEN - 1] = 0;
+
+	if (strcasecmp(hsc->range_str, "na") == 0) {
+		// range string "not available"
+		// we got a custom chip not covered by the nomenclature with a custom range
+		ret = device_property_read_u32(dev, "honeywell,pmin-pascal",
+					       &hsc->pmin);
+		if (ret)
+			return dev_err_probe(dev, ret,
+					     "honeywell,pmin-pascal could not be read\n");
+		ret = device_property_read_u32(dev, "honeywell,pmax-pascal",
+					       &hsc->pmax);
+		if (ret)
+			return dev_err_probe(dev, ret,
+					     "honeywell,pmax-pascal could not be read\n");
+	}
+
+	return hsc_probe(indio_dev, &spi->dev, spi_get_device_id(spi)->name,
+			 spi_get_device_id(spi)->driver_data);
+}
+
+static const struct of_device_id hsc_spi_match[] = {
+	{.compatible = "honeywell,hsc",},
+	{.compatible = "honeywell,ssc",},
+	{},
+};
+
+MODULE_DEVICE_TABLE(of, hsc_spi_match);
+
+static const struct spi_device_id hsc_spi_id[] = {
+	{"hsc", HSC},
+	{"ssc", SSC},
+	{}
+};
+
+MODULE_DEVICE_TABLE(spi, hsc_spi_id);
+
+static struct spi_driver hsc_spi_driver = {
+	.driver = {
+		   .name = "honeywell_hsc",
+		   .of_match_table = hsc_spi_match,
+		   },
+	.probe = hsc_spi_probe,
+	.id_table = hsc_spi_id,
+};
+
+module_spi_driver(hsc_spi_driver);
+
+MODULE_AUTHOR("Petre Rodan <petre.rodan@subdimension.ro>");
+MODULE_DESCRIPTION("Honeywell HSC and SSC pressure sensor spi driver");
+MODULE_LICENSE("GPL");
+MODULE_IMPORT_NS(IIO_HONEYWELL_HSC);
-- 
2.41.0


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

* Re: [PATCH 1/2] dt-bindings: iio: pressure: add honeywell,hsc030
  2023-11-17 16:42 [PATCH 1/2] dt-bindings: iio: pressure: add honeywell,hsc030 Petre Rodan
  2023-11-17 16:42 ` [PATCH 2/2] iio: pressure: driver for Honeywell HSC/SSC series pressure sensors Petre Rodan
@ 2023-11-17 17:12 ` Rob Herring
  2023-11-17 19:22 ` [PATCH v2 " Petre Rodan
  2 siblings, 0 replies; 24+ messages in thread
From: Rob Herring @ 2023-11-17 17:12 UTC (permalink / raw)
  To: Petre Rodan
  Cc: Conor Dooley, devicetree, linux-kernel, linux-kernel-mentees,
	linux-iio, Jonathan Cameron, Rob Herring, Krzysztof Kozlowski,
	Lars-Peter Clausen


On Fri, 17 Nov 2023 18:42:05 +0200, Petre Rodan wrote:
> Adds binding for digital Honeywell TruStability HSC and SSC series
> pressure and temperature sensors.
> 
> Datasheet:
>  [HSC] https://prod-edam.honeywell.com/content/dam/honeywell-edam/sps/siot/en-us/products/sensors/pressure-sensors/board-mount-pressure-sensors/trustability-hsc-series/documents/sps-siot-trustability-hsc-series-high-accuracy-board-mount-pressure-sensors-50099148-a-en-ciid-151133.pdf
>  [SSC] https://prod-edam.honeywell.com/content/dam/honeywell-edam/sps/siot/en-us/products/sensors/pressure-sensors/board-mount-pressure-sensors/trustability-ssc-series/documents/sps-siot-trustability-ssc-series-standard-accuracy-board-mount-pressure-sensors-50099533-a-en-ciid-151134.pdf
> 
> Signed-off-by: Petre Rodan <petre.rodan@subdimension.ro>
> ---
>  .../iio/pressure/honeywell,hsc030pa.yaml     | 166 ++++++++++++++++++
>  1 file changed, 166 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/pressure/honeywell,hsc0030pa.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:60:13: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:60:22: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:60:31: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:60:40: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:60:49: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:60:58: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:60:67: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:61:13: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:61:22: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:61:31: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:61:40: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:61:49: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:61:58: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:61:67: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:62:13: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:62:22: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:62:31: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:62:40: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:62:49: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:62:58: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:62:67: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:63:13: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:63:22: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:63:31: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:63:40: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:63:49: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:63:58: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:63:67: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:64:13: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:64:22: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:64:31: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:64:40: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:64:49: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:64:58: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:64:67: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:65:13: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:65:22: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:65:31: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:65:40: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:65:49: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:65:58: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:65:67: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:66:13: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:66:22: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:66:31: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:66:40: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:66:49: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:66:58: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:66:67: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:67:13: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:67:22: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:67:31: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:67:40: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:67:49: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:67:58: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:67:67: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:68:13: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:68:22: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:68:31: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:68:40: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:68:49: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:68:58: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:68:67: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:69:13: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:69:22: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:69:31: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:69:40: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:69:49: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:69:58: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:69:67: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:70:13: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:70:22: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:70:31: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:70:40: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:70:49: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:70:58: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:70:67: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:71:13: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:71:22: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:71:31: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:71:40: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:71:49: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:71:58: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:71:67: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:72:13: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:72:22: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:72:31: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:72:40: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:72:49: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:72:58: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:72:67: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:73:13: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:73:22: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:73:31: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:73:40: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:73:49: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:73:58: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:73:67: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:74:13: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:74:22: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:74:31: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:74:40: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:74:49: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:74:58: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:74:67: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:75:13: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:75:22: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:75:31: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:75:40: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:75:49: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:75:58: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:75:67: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:76:13: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:76:22: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:76:31: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:76:40: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:76:49: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:76:58: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:76:67: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:130:1: [error] syntax error: found character '\t' that cannot start any token (syntax)

dtschema/dtc warnings/errors:
make[2]: *** Deleting file 'Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.example.dts'
Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:130:1: found a tab character where an indentation space is expected
make[2]: *** [Documentation/devicetree/bindings/Makefile:26: Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.example.dts] Error 1
make[2]: *** Waiting for unfinished jobs....
./Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml:130:1: found a tab character where an indentation space is expected
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml: ignoring, error parsing file
make[1]: *** [/builds/robherring/dt-review-ci/linux/Makefile:1424: dt_binding_check] Error 2
make: *** [Makefile:234: __sub-make] Error 2

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20231117164232.8474-1-petre.rodan@subdimension.ro

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.


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

* [PATCH v2 1/2] dt-bindings: iio: pressure: add honeywell,hsc030
  2023-11-17 16:42 [PATCH 1/2] dt-bindings: iio: pressure: add honeywell,hsc030 Petre Rodan
  2023-11-17 16:42 ` [PATCH 2/2] iio: pressure: driver for Honeywell HSC/SSC series pressure sensors Petre Rodan
  2023-11-17 17:12 ` [PATCH 1/2] dt-bindings: iio: pressure: add honeywell,hsc030 Rob Herring
@ 2023-11-17 19:22 ` Petre Rodan
  2023-11-17 20:13   ` Rob Herring
                     ` (3 more replies)
  2 siblings, 4 replies; 24+ messages in thread
From: Petre Rodan @ 2023-11-17 19:22 UTC (permalink / raw)
  To: linux-kernel, linux-iio, devicetree
  Cc: Petre Rodan, Conor Dooley, Lars-Peter Clausen, Rob Herring,
	Krzysztof Kozlowski, linux-kernel-mentees, Jonathan Cameron

Adds binding for digital Honeywell TruStability HSC and SSC series pressure 
and temperature sensors.

Datasheet:
 [HSC] https://prod-edam.honeywell.com/content/dam/honeywell-edam/sps/siot/en-us/products/sensors/pressure-sensors/board-mount-pressure-sensors/trustability-hsc-series/documents/sps-siot-trustability-hsc-series-high-accuracy-board-mount-pressure-sensors-50099148-a-en-ciid-151133.pdf
 [SSC] https://prod-edam.honeywell.com/content/dam/honeywell-edam/sps/siot/en-us/products/sensors/pressure-sensors/board-mount-pressure-sensors/trustability-ssc-series/documents/sps-siot-trustability-ssc-series-standard-accuracy-board-mount-pressure-sensors-50099533-a-en-ciid-151134.pdf

Signed-off-by: Petre Rodan <petre.rodan@subdimension.ro>
---

Changes for v2:
- Removed redundant quotations reported by robh's bot
- Fixed yamllint warnings

I'm failing to run 'make DT_CHECKER_FLAGS=-m dt_binding_check' due to
python errors and exceptions
---
 .../iio/pressure/honeywell,hsc030pa.yaml      | 156 ++++++++++++++++++
 1 file changed, 156 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml

diff --git a/Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml b/Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml
new file mode 100644
index 000000000000..c7e5d3bd5ef4
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml
@@ -0,0 +1,156 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/pressure/honeywell,hsc030pa.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Honeywell TruStability HSC and SSC pressure sensor families
+
+description: |
+  support for Honeywell TruStability HSC and SSC digital pressure sensor
+  families.
+
+  These sensors have either an I2C, an SPI or an analog interface. Only the
+  digital versions are supported by this driver.
+
+  There are 118 models with different pressure ranges available in each family.
+  The vendor calls them "HSC series" and "SSC series". All of them have an
+  identical programming model but differ in 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 can either be provided via range_str or
+  in case it's a custom chip via numerical range limits converted to pascals.
+
+  The transfer function defines the ranges of raw conversion values delivered
+  by the sensor. pmin-pascal and pmax-pascal corespond to the minimum and
+  maximum pressure that can be measured.
+
+  Specifications about the devices can be found at:
+  https://prod-edam.honeywell.com/content/dam/honeywell-edam/sps/siot/en-us/products/sensors/pressure-sensors/board-mount-pressure-sensors/trustability-hsc-series/documents/sps-siot-trustability-hsc-series-high-accuracy-board-mount-pressure-sensors-50099148-a-en-ciid-151133.pdf
+  https://prod-edam.honeywell.com/content/dam/honeywell-edam/sps/siot/en-us/products/sensors/pressure-sensors/board-mount-pressure-sensors/trustability-ssc-series/documents/sps-siot-trustability-ssc-series-standard-accuracy-board-mount-pressure-sensors-50099533-a-en-ciid-151134.pdf
+
+maintainers:
+  - Petre Rodan <petre.rodan@subdimension.ro>
+
+properties:
+  compatible:
+    enum:
+      - honeywell,hsc
+      - honeywell,ssc
+
+  reg:
+    maxItems: 1
+
+  honeywell,transfer-function:
+    description: |
+      Transfer function which defines the range of valid values delivered by
+      the sensor.
+      0 - A, 10% to 90% of 2^14
+      1 - B, 5% to 95% of 2^14
+      2 - C, 5% to 85% of 2^14
+      3 - F, 4% to 94% of 2^14
+    $ref: /schemas/types.yaml#/definitions/uint32
+
+  honeywell,range_str:
+    description: |
+      Five character string that defines "pressure range, unit and type"
+      as part of the device nomenclature. In the unlikely case of a custom
+      chip, set to "NA" and provide honeywell,pmin-pascal honeywell,pmax-pascal
+    enum: [001BA, 1.6BA, 2.5BA, 004BA, 006BA, 010BA, 1.6MD, 2.5MD, 004MD,
+           006MD, 010MD, 016MD, 025MD, 040MD, 060MD, 100MD, 160MD, 250MD,
+           400MD, 600MD, 001BD, 1.6BD, 2.5BD, 004BD, 2.5MG, 004MG, 006MG,
+           010MG, 016MG, 025MG, 040MG, 060MG, 100MG, 160MG, 250MG, 400MG,
+           600MG, 001BG, 1.6BG, 2.5BG, 004BG, 006BG, 010BG, 100KA, 160KA,
+           250KA, 400KA, 600KA, 001GA, 160LD, 250LD, 400LD, 600LD, 001KD,
+           1.6KD, 2.5KD, 004KD, 006KD, 010KD, 016KD, 025KD, 040KD, 060KD,
+           100KD, 160KD, 250KD, 400KD, 250LG, 400LG, 600LG, 001KG, 1.6KG,
+           2.5KG, 004KG, 006KG, 010KG, 016KG, 025KG, 040KG, 060KG, 100KG,
+           160KG, 250KG, 400KG, 600KG, 001GG, 015PA, 030PA, 060PA, 100PA,
+           150PA, 0.5ND, 001ND, 002ND, 004ND, 005ND, 010ND, 020ND, 030ND,
+           001PD, 005PD, 015PD, 030PD, 060PD, 001NG, 002NG, 004NG, 005NG,
+           010NG, 020NG, 030NG, 001PG, 005PG, 015PG, 030PG, 060PG, 100PG,
+           150PG, NA]
+    $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,range_str is set to "NA".
+    $ref: /schemas/types.yaml#/definitions/int32
+
+  honeywell,pmax-pascal:
+    description: |
+      Maximum pressure value the sensor can measure in pascal.
+      To be specified only if honeywell,range_str is set to "NA".
+    $ref: /schemas/types.yaml#/definitions/int32
+
+  vdd-supply:
+    description: |
+      Provide VDD power to the sensor (either 3.3V or 5V depending on the chip).
+      Optional, activate only if required by the target board.
+
+  spi-max-frequency:
+    description: SPI clock to be kept between 50 and 800kHz
+
+  clock-frequency:
+    description: i2c clock to be kept between 100 and 400kHz
+
+required:
+  - compatible
+  - reg
+  - honeywell,transfer-function
+  - honeywell,range_str
+  - clock-frequency
+  - spi-max-frequency
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+    i2c {
+        status = "okay";
+        clock-frequency = <400000>;
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        HSCMRNN030PA2A3@28 {
+          status = "okay";
+          compatible = "honeywell,hsc";
+          reg = <0x28>;
+
+          honeywell,transfer-function = <0>;
+          honeywell,range_str = "030PA";
+        };
+    };
+
+    spi {
+        # note that MOSI is not required by this sensor
+        status = "okay";
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        HSCMLNN100PASA3@0 {
+          status = "okay";
+          compatible = "honeywell,hsc";
+          reg = <0>;
+          spi-max-frequency = <800000>;
+
+          honeywell,transfer-function = <0>;
+          honeywell,range_str = "100PA";
+        };
+
+        HSC_CUSTOM_CHIP@0 {
+          status = "okay";
+          compatible = "honeywell,hsc";
+          reg = <1>;
+          spi-max-frequency = <800000>;
+
+          honeywell,transfer-function = <0>;
+          honeywell,range_str = "NA";
+          honeywell,pmin-pascal = <0>;
+          honeywell,pmax-pascal = <206850>;
+        };
+
+    };
-- 
2.41.0


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

* Re: [PATCH v2 1/2] dt-bindings: iio: pressure: add honeywell,hsc030
  2023-11-17 19:22 ` [PATCH v2 " Petre Rodan
@ 2023-11-17 20:13   ` Rob Herring
  2023-11-19 13:49   ` Rob Herring
                     ` (2 subsequent siblings)
  3 siblings, 0 replies; 24+ messages in thread
From: Rob Herring @ 2023-11-17 20:13 UTC (permalink / raw)
  To: Petre Rodan
  Cc: Lars-Peter Clausen, linux-kernel-mentees, Jonathan Cameron,
	linux-kernel, devicetree, linux-iio, Conor Dooley,
	Krzysztof Kozlowski, Rob Herring


On Fri, 17 Nov 2023 21:22:57 +0200, Petre Rodan wrote:
> Adds binding for digital Honeywell TruStability HSC and SSC series pressure
> and temperature sensors.
> 
> Datasheet:
>  [HSC] https://prod-edam.honeywell.com/content/dam/honeywell-edam/sps/siot/en-us/products/sensors/pressure-sensors/board-mount-pressure-sensors/trustability-hsc-series/documents/sps-siot-trustability-hsc-series-high-accuracy-board-mount-pressure-sensors-50099148-a-en-ciid-151133.pdf
>  [SSC] https://prod-edam.honeywell.com/content/dam/honeywell-edam/sps/siot/en-us/products/sensors/pressure-sensors/board-mount-pressure-sensors/trustability-ssc-series/documents/sps-siot-trustability-ssc-series-standard-accuracy-board-mount-pressure-sensors-50099533-a-en-ciid-151134.pdf
> 
> Signed-off-by: Petre Rodan <petre.rodan@subdimension.ro>
> ---
> 
> Changes for v2:
> - Removed redundant quotations reported by robh's bot
> - Fixed yamllint warnings
> 
> I'm failing to run 'make DT_CHECKER_FLAGS=-m dt_binding_check' due to
> python errors and exceptions
> ---
>  .../iio/pressure/honeywell,hsc030pa.yaml      | 156 ++++++++++++++++++
>  1 file changed, 156 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Error: Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.example.dts:36.15-16 syntax error
FATAL ERROR: Unable to parse input tree
make[2]: *** [scripts/Makefile.lib:419: Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.example.dtb] Error 1
make[2]: *** Waiting for unfinished jobs....
make[1]: *** [/builds/robherring/dt-review-ci/linux/Makefile:1424: dt_binding_check] Error 2
make: *** [Makefile:234: __sub-make] Error 2

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20231117192305.17612-1-petre.rodan@subdimension.ro

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.


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

* Re: [PATCH v2 2/2] iio: pressure: driver for Honeywell HSC/SSC series pressure sensors
  2023-11-17 16:42 ` [PATCH 2/2] iio: pressure: driver for Honeywell HSC/SSC series pressure sensors Petre Rodan
@ 2023-11-18  5:21   ` kernel test robot
  2023-11-20 12:35   ` [PATCH " Andy Shevchenko
  1 sibling, 0 replies; 24+ messages in thread
From: kernel test robot @ 2023-11-18  5:21 UTC (permalink / raw)
  To: Petre Rodan, linux-kernel, linux-iio
  Cc: oe-kbuild-all, Jonathan Cameron, Lars-Peter Clausen,
	Andy Shevchenko, Angel Iglesias, Matti Vaittinen,
	Matti Vaittinen, Andreas Klinger, Rob Herring,
	Krzysztof Kozlowski

Hi Petre,

kernel test robot noticed the following build errors:

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

url:    https://github.com/intel-lab-lkp/linux/commits/Petre-Rodan/iio-pressure-driver-for-Honeywell-HSC-SSC-series-pressure-sensors/20231118-072654
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
patch link:    https://lore.kernel.org/r/20231117164232.8474-2-petre.rodan%40subdimension.ro
patch subject: [PATCH v2 2/2] iio: pressure: driver for Honeywell HSC/SSC series pressure sensors
config: riscv-rv32_defconfig (attached as .config)
compiler: riscv32-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231118/202311181316.z2BmTZmP-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202311181316.z2BmTZmP-lkp@intel.com/

All errors (new ones prefixed by >>):

>> drivers/iio/pressure/Kconfig:120: syntax error
   drivers/iio/pressure/Kconfig:119:warning: ignoring unsupported character '.'
   drivers/iio/pressure/Kconfig:119: unknown statement "pressure"
   drivers/iio/pressure/Kconfig:121:warning: ignoring unsupported character ','
   drivers/iio/pressure/Kconfig:121:warning: ignoring unsupported character ':'
   drivers/iio/pressure/Kconfig:121: unknown statement "To"
   drivers/iio/pressure/Kconfig:122:warning: ignoring unsupported character '.'
   drivers/iio/pressure/Kconfig:122: unknown statement "called"
   make[3]: *** [scripts/kconfig/Makefile:77: oldconfig] Error 1
   make[2]: *** [Makefile:685: oldconfig] Error 2
   make[1]: *** [Makefile:234: __sub-make] Error 2
   make[1]: Target 'oldconfig' not remade because of errors.
   make: *** [Makefile:234: __sub-make] Error 2
   make: Target 'oldconfig' not remade because of errors.
--
>> drivers/iio/pressure/Kconfig:120: syntax error
   drivers/iio/pressure/Kconfig:119:warning: ignoring unsupported character '.'
   drivers/iio/pressure/Kconfig:119: unknown statement "pressure"
   drivers/iio/pressure/Kconfig:121:warning: ignoring unsupported character ','
   drivers/iio/pressure/Kconfig:121:warning: ignoring unsupported character ':'
   drivers/iio/pressure/Kconfig:121: unknown statement "To"
   drivers/iio/pressure/Kconfig:122:warning: ignoring unsupported character '.'
   drivers/iio/pressure/Kconfig:122: unknown statement "called"
   make[3]: *** [scripts/kconfig/Makefile:77: olddefconfig] Error 1
   make[2]: *** [Makefile:685: olddefconfig] Error 2
   make[1]: *** [Makefile:234: __sub-make] Error 2
   make[1]: Target 'olddefconfig' not remade because of errors.
   make: *** [Makefile:234: __sub-make] Error 2
   make: Target 'olddefconfig' not remade because of errors.
--
>> drivers/iio/pressure/Kconfig:120: syntax error
   drivers/iio/pressure/Kconfig:119:warning: ignoring unsupported character '.'
   drivers/iio/pressure/Kconfig:119: unknown statement "pressure"
   drivers/iio/pressure/Kconfig:121:warning: ignoring unsupported character ','
   drivers/iio/pressure/Kconfig:121:warning: ignoring unsupported character ':'
   drivers/iio/pressure/Kconfig:121: unknown statement "To"
   drivers/iio/pressure/Kconfig:122:warning: ignoring unsupported character '.'
   drivers/iio/pressure/Kconfig:122: unknown statement "called"
   make[5]: *** [scripts/kconfig/Makefile:87: defconfig] Error 1
   make[4]: *** [Makefile:685: defconfig] Error 2
   make[3]: *** [Makefile:350: __build_one_by_one] Error 2
   make[3]: Target 'defconfig' not remade because of errors.
   make[3]: Target '32-bit.config' not remade because of errors.
   make[2]: *** [arch/riscv/Makefile:190: rv32_defconfig] Error 2
   make[1]: *** [Makefile:234: __sub-make] Error 2
   make[1]: Target 'rv32_defconfig' not remade because of errors.
   make: *** [Makefile:234: __sub-make] Error 2
   make: Target 'rv32_defconfig' not remade because of errors.


vim +120 drivers/iio/pressure/Kconfig

     8	
     9	config ABP060MG
    10		tristate "Honeywell ABP pressure sensor driver"
    11		depends on I2C
    12		help
    13		  Say yes here to build support for the Honeywell ABP pressure
    14		  sensors.
    15	
    16		  To compile this driver as a module, choose M here: the module
    17		  will be called abp060mg.
    18	
    19	config ROHM_BM1390
    20		tristate "ROHM BM1390GLV-Z pressure sensor driver"
    21		depends on I2C
    22		help
    23		  Support for the ROHM BM1390 pressure sensor. The BM1390GLV-Z
    24		  can measure pressures ranging from 300 hPa to 1300 hPa with
    25		  configurable measurement averaging and internal FIFO. The
    26		  sensor does also provide temperature measurements.
    27	
    28	config BMP280
    29		tristate "Bosch Sensortec BMP180/BMP280/BMP380/BMP580 pressure sensor driver"
    30		depends on (I2C || SPI_MASTER)
    31		select REGMAP
    32		select BMP280_I2C if (I2C)
    33		select BMP280_SPI if (SPI_MASTER)
    34		help
    35		  Say yes here to build support for Bosch Sensortec BMP180, BMP280, BMP380
    36		  and BMP580 pressure and temperature sensors. Also supports the BME280 with
    37		  an additional humidity sensor channel.
    38	
    39		  To compile this driver as a module, choose M here: the core module
    40		  will be called bmp280 and you will also get bmp280-i2c for I2C
    41		  and/or bmp280-spi for SPI support.
    42	
    43	config BMP280_I2C
    44		tristate
    45		depends on BMP280
    46		depends on I2C
    47		select REGMAP_I2C
    48	
    49	config BMP280_SPI
    50		tristate
    51		depends on BMP280
    52		depends on SPI_MASTER
    53		select REGMAP
    54	
    55	config IIO_CROS_EC_BARO
    56		tristate "ChromeOS EC Barometer Sensor"
    57		depends on IIO_CROS_EC_SENSORS_CORE
    58		help
    59		  Say yes here to build support for the Barometer sensor when
    60		  presented by the ChromeOS EC Sensor hub.
    61	
    62		  To compile this driver as a module, choose M here: the module
    63		  will be called cros_ec_baro.
    64	
    65	config DLHL60D
    66		tristate "All Sensors DLHL60D and DLHL60G low voltage digital pressure sensors"
    67		depends on I2C
    68		select IIO_BUFFER
    69		select IIO_TRIGGERED_BUFFER
    70		help
    71		  Say yes here to build support for the All Sensors DLH series
    72		  pressure sensors driver.
    73	
    74		  To compile this driver as a module, choose M here: the module
    75		  will be called dlhl60d.
    76	
    77	config DPS310
    78		tristate "Infineon DPS310 pressure and temperature sensor"
    79		depends on I2C
    80		select REGMAP_I2C
    81		help
    82		  Support for the Infineon DPS310 digital barometric pressure sensor.
    83		  It can be accessed over I2C bus.
    84	
    85		  This driver can also be built as a module.  If so, the module will be
    86		  called dps310.
    87	
    88	config HID_SENSOR_PRESS
    89		depends on HID_SENSOR_HUB
    90		select IIO_BUFFER
    91		select HID_SENSOR_IIO_COMMON
    92		select HID_SENSOR_IIO_TRIGGER
    93		tristate "HID PRESS"
    94		help
    95		  Say yes here to build support for the HID SENSOR
    96		  Pressure driver
    97	
    98		  To compile this driver as a module, choose M here: the module
    99		  will be called hid-sensor-press.
   100	
   101	config HP03
   102		tristate "Hope RF HP03 temperature and pressure sensor driver"
   103		depends on I2C
   104		select REGMAP_I2C
   105		help
   106		  Say yes here to build support for Hope RF HP03 pressure and
   107		  temperature sensor.
   108	
   109		  To compile this driver as a module, choose M here: the module
   110		  will be called hp03.
   111	
   112	config HSC030PA
   113		tristate "Honeywell HSC/SSC (TruStability pressure sensors series)"
   114		depends on (I2C || SPI_MASTER)
   115		select HSC030PA_I2C if (I2C)
   116		select HSC030PA_SPI if (SPI_MASTER)
   117		help
   118		  Say Y here to build support for the Honeywell HSC and SSC TruStability
   119	      pressure and temperature sensor series.
 > 120	
   121		  To compile this driver as a module, choose M here: the module will be
   122		  called hsc030pa.
   123	

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

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

* Re: [PATCH v2 1/2] dt-bindings: iio: pressure: add honeywell,hsc030
  2023-11-17 19:22 ` [PATCH v2 " Petre Rodan
  2023-11-17 20:13   ` Rob Herring
@ 2023-11-19 13:49   ` Rob Herring
  2023-11-19 20:14     ` Petre Rodan
  2023-11-20 10:21   ` Krzysztof Kozlowski
  2023-11-25 19:19   ` Jonathan Cameron
  3 siblings, 1 reply; 24+ messages in thread
From: Rob Herring @ 2023-11-19 13:49 UTC (permalink / raw)
  To: Petre Rodan
  Cc: linux-kernel, linux-iio, devicetree, Conor Dooley,
	Lars-Peter Clausen, Krzysztof Kozlowski, linux-kernel-mentees,
	Jonathan Cameron

On Fri, Nov 17, 2023 at 09:22:57PM +0200, Petre Rodan wrote:
> Adds binding for digital Honeywell TruStability HSC and SSC series pressure 
> and temperature sensors.
> 
> Datasheet:
>  [HSC] https://prod-edam.honeywell.com/content/dam/honeywell-edam/sps/siot/en-us/products/sensors/pressure-sensors/board-mount-pressure-sensors/trustability-hsc-series/documents/sps-siot-trustability-hsc-series-high-accuracy-board-mount-pressure-sensors-50099148-a-en-ciid-151133.pdf
>  [SSC] https://prod-edam.honeywell.com/content/dam/honeywell-edam/sps/siot/en-us/products/sensors/pressure-sensors/board-mount-pressure-sensors/trustability-ssc-series/documents/sps-siot-trustability-ssc-series-standard-accuracy-board-mount-pressure-sensors-50099533-a-en-ciid-151134.pdf
> 
> Signed-off-by: Petre Rodan <petre.rodan@subdimension.ro>
> ---
> 
> Changes for v2:
> - Removed redundant quotations reported by robh's bot
> - Fixed yamllint warnings
> 
> I'm failing to run 'make DT_CHECKER_FLAGS=-m dt_binding_check' due to
> python errors and exceptions

What exceptions?


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

* Re: [PATCH v2 1/2] dt-bindings: iio: pressure: add honeywell,hsc030
  2023-11-19 13:49   ` Rob Herring
@ 2023-11-19 20:14     ` Petre Rodan
  2023-11-20 10:15       ` Krzysztof Kozlowski
  2023-11-20 17:19       ` Rob Herring
  0 siblings, 2 replies; 24+ messages in thread
From: Petre Rodan @ 2023-11-19 20:14 UTC (permalink / raw)
  To: Rob Herring
  Cc: linux-kernel, linux-iio, devicetree, Conor Dooley,
	Lars-Peter Clausen, Krzysztof Kozlowski, linux-kernel-mentees,
	Jonathan Cameron


Good morning!

On Sun, Nov 19, 2023 at 07:49:39AM -0600, Rob Herring wrote:
> On Fri, Nov 17, 2023 at 09:22:57PM +0200, Petre Rodan wrote:
> > Adds binding for digital Honeywell TruStability HSC and SSC series pressure 
> > and temperature sensors.
> > 
[..]
> > Changes for v2:
> > - Removed redundant quotations reported by robh's bot
> > - Fixed yamllint warnings
> > 
> > I'm failing to run 'make DT_CHECKER_FLAGS=-m dt_binding_check' due to
> > python errors and exceptions
> 
> What exceptions?

thanks for asking.

first off, installed packages. the first 4 are not part of the official Gentoo repo, so I might have prepared them with missing options if any where not included by default.
I know nothing about python.

$ equery l dtschema pylibfdt ruamel-yaml yamllint jsonschema python 
[I-O] [  ] dev-python/dtschema-2023.9:0
[I-O] [  ] dev-python/pylibfdt-1.7.0_p1:0
[I-O] [  ] dev-python/ruamel-yaml-0.18.5:0
[I-O] [  ] dev-python/yamllint-1.33.0:0
[IP-] [  ] dev-python/jsonschema-4.19.1:0
[IP-] [  ] dev-lang/python-2.7.18_p16-r1:2.7
[IP-] [  ] dev-lang/python-3.10.13:3.10
[IP-] [  ] dev-lang/python-3.11.5:3.11
prodan@sunspire /usr/src/linux-upstream $ python --version
Python 3.11.5

# binding check
prodan@sunspire /usr/src/linux-upstream $ make DT_SCHEMA_FILES=Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml  DT_CHECKER_FLAGS=-m dt_binding_check
Traceback (most recent call last):
  File "/usr/lib/python3.11/site-packages/jsonschema/validators.py", line 1152, in resolve_fragment
    document = document[part]
               ~~~~~~~~^^^^^^
KeyError: 'definitions'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/usr/lib/python-exec/python3.11/dt-doc-validate", line 64, in <module>
    ret |= check_doc(f)
           ^^^^^^^^^^^^
  File "/usr/lib/python-exec/python3.11/dt-doc-validate", line 32, in check_doc
    for error in sorted(dtsch.iter_errors(), key=lambda e: e.linecol):
                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.11/site-packages/dtschema/schema.py", line 123, in iter_errors
    for error in self.validator.iter_errors(self):
  File "/usr/lib/python3.11/site-packages/jsonschema/validators.py", line 368, in iter_errors
    for error in errors:
  File "/usr/lib/python3.11/site-packages/jsonschema/_keywords.py", line 335, in allOf
    yield from validator.descend(instance, subschema, schema_path=index)
  File "/usr/lib/python3.11/site-packages/jsonschema/validators.py", line 416, in descend
    for error in errors:
  File "/usr/lib/python3.11/site-packages/jsonschema/_keywords.py", line 284, in ref
    yield from validator._validate_reference(ref=ref, instance=instance)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.11/site-packages/jsonschema/validators.py", line 465, in _validate_reference
    return list(self.descend(instance, resolved))
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.11/site-packages/jsonschema/validators.py", line 416, in descend
    for error in errors:
  File "/usr/lib/python3.11/site-packages/jsonschema/_keywords.py", line 335, in allOf
    yield from validator.descend(instance, subschema, schema_path=index)
  File "/usr/lib/python3.11/site-packages/jsonschema/validators.py", line 416, in descend
    for error in errors:
  File "/usr/lib/python3.11/site-packages/jsonschema/_keywords.py", line 284, in ref
    yield from validator._validate_reference(ref=ref, instance=instance)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.11/site-packages/jsonschema/validators.py", line 465, in _validate_reference
    return list(self.descend(instance, resolved))
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.11/site-packages/jsonschema/validators.py", line 416, in descend
    for error in errors:
  File "/usr/lib/python3.11/site-packages/jsonschema/_keywords.py", line 305, in properties
    yield from validator.descend(
  File "/usr/lib/python3.11/site-packages/jsonschema/validators.py", line 416, in descend
    for error in errors:
  File "/usr/lib/python3.11/site-packages/jsonschema/_keywords.py", line 34, in propertyNames
    yield from validator.descend(instance=property, schema=propertyNames)
  File "/usr/lib/python3.11/site-packages/jsonschema/validators.py", line 416, in descend
    for error in errors:
  File "/usr/lib/python3.11/site-packages/jsonschema/_keywords.py", line 335, in allOf
    yield from validator.descend(instance, subschema, schema_path=index)
  File "/usr/lib/python3.11/site-packages/jsonschema/validators.py", line 416, in descend
    for error in errors:
  File "/usr/lib/python3.11/site-packages/jsonschema/_keywords.py", line 378, in not_
    if validator.evolve(schema=not_schema).is_valid(instance):
       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.11/site-packages/jsonschema/validators.py", line 483, in is_valid
    error = next(self.iter_errors(instance), None)
            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.11/site-packages/jsonschema/validators.py", line 368, in iter_errors
    for error in errors:
  File "/usr/lib/python3.11/site-packages/jsonschema/_keywords.py", line 284, in ref
    yield from validator._validate_reference(ref=ref, instance=instance)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.11/site-packages/jsonschema/validators.py", line 461, in _validate_reference
    scope, resolved = resolve(ref)
                      ^^^^^^^^^^^^
  File "/usr/lib/python3.11/site-packages/jsonschema/validators.py", line 1086, in resolve
    return url, self._remote_cache(url)
                ^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.11/site-packages/jsonschema/validators.py", line 1104, in resolve_from_url
    return self.resolve_fragment(document, fragment)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.11/site-packages/jsonschema/validators.py", line 1154, in resolve_fragment
    raise exceptions._RefResolutionError(
jsonschema.exceptions._RefResolutionError: Unresolvable JSON pointer: 'definitions/json-schema-prop-names'
Error: Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.example.dts:36.15-16 syntax error
FATAL ERROR: Unable to parse input tree
make[2]: *** [scripts/Makefile.lib:419: Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.example.dtb] Error 1
make[1]: *** [/usr/src/linux-upstream/Makefile:1424: dt_binding_check] Error 2
make: *** [Makefile:234: __sub-make] Error 2

best regards,
peter

-- 
petre rodan

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

* Re: [PATCH v2 1/2] dt-bindings: iio: pressure: add honeywell,hsc030
  2023-11-19 20:14     ` Petre Rodan
@ 2023-11-20 10:15       ` Krzysztof Kozlowski
  2023-11-20 17:19       ` Rob Herring
  1 sibling, 0 replies; 24+ messages in thread
From: Krzysztof Kozlowski @ 2023-11-20 10:15 UTC (permalink / raw)
  To: Petre Rodan, Rob Herring
  Cc: linux-kernel, linux-iio, devicetree, Conor Dooley,
	Lars-Peter Clausen, Krzysztof Kozlowski, linux-kernel-mentees,
	Jonathan Cameron

On 19/11/2023 21:14, Petre Rodan wrote:
> 
> Good morning!
> 
> On Sun, Nov 19, 2023 at 07:49:39AM -0600, Rob Herring wrote:
>> On Fri, Nov 17, 2023 at 09:22:57PM +0200, Petre Rodan wrote:
>>> Adds binding for digital Honeywell TruStability HSC and SSC series pressure 
>>> and temperature sensors.
>>>
> [..]
>>> Changes for v2:
>>> - Removed redundant quotations reported by robh's bot
>>> - Fixed yamllint warnings
>>>
>>> I'm failing to run 'make DT_CHECKER_FLAGS=-m dt_binding_check' due to
>>> python errors and exceptions
>>
>> What exceptions?
> 
> thanks for asking.
> 
> first off, installed packages. the first 4 are not part of the official Gentoo repo, so I might have prepared them with missing options if any where not included by default.
> I know nothing about python.

The easiest is to install them with pip. You might get old versions from
your distro. Although in your case, it looks you got most recent.

> 
> $ equery l dtschema pylibfdt ruamel-yaml yamllint jsonschema python 
> [I-O] [  ] dev-python/dtschema-2023.9:0
> [I-O] [  ] dev-python/pylibfdt-1.7.0_p1:0
> [I-O] [  ] dev-python/ruamel-yaml-0.18.5:0
> [I-O] [  ] dev-python/yamllint-1.33.0:0
> [IP-] [  ] dev-python/jsonschema-4.19.1:0

I guess this is the problem. The dtschema explicitly asks for:
    "jsonschema>=4.1.2,<4.18",
because newer jsonschema does not work well for us.

Best regards,
Krzysztof


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

* Re: [PATCH v2 1/2] dt-bindings: iio: pressure: add honeywell,hsc030
  2023-11-17 19:22 ` [PATCH v2 " Petre Rodan
  2023-11-17 20:13   ` Rob Herring
  2023-11-19 13:49   ` Rob Herring
@ 2023-11-20 10:21   ` Krzysztof Kozlowski
  2023-11-20 13:42     ` Petre Rodan
  2023-11-25 19:19   ` Jonathan Cameron
  3 siblings, 1 reply; 24+ messages in thread
From: Krzysztof Kozlowski @ 2023-11-20 10:21 UTC (permalink / raw)
  To: Petre Rodan, linux-kernel, linux-iio, devicetree
  Cc: Conor Dooley, Lars-Peter Clausen, Rob Herring,
	Krzysztof Kozlowski, linux-kernel-mentees, Jonathan Cameron

On 17/11/2023 20:22, Petre Rodan wrote:
> Adds binding for digital Honeywell TruStability HSC and SSC series pressure 
> and temperature sensors.
> 
> Datasheet:
>  [HSC] https://prod-edam.honeywell.com/content/dam/honeywell-edam/sps/siot/en-us/products/sensors/pressure-sensors/board-mount-pressure-sensors/trustability-hsc-series/documents/sps-siot-trustability-hsc-series-high-accuracy-board-mount-pressure-sensors-50099148-a-en-ciid-151133.pdf
>  [SSC] https://prod-edam.honeywell.com/content/dam/honeywell-edam/sps/siot/en-us/products/sensors/pressure-sensors/board-mount-pressure-sensors/trustability-ssc-series/documents/sps-siot-trustability-ssc-series-standard-accuracy-board-mount-pressure-sensors-50099533-a-en-ciid-151134.pdf
> 
> Signed-off-by: Petre Rodan <petre.rodan@subdimension.ro>
> ---
> 
> Changes for v2:
> - Removed redundant quotations reported by robh's bot
> - Fixed yamllint warnings
> 
> I'm failing to run 'make DT_CHECKER_FLAGS=-m dt_binding_check' due to
> python errors and exceptions
> ---
>  .../iio/pressure/honeywell,hsc030pa.yaml      | 156 ++++++++++++++++++
>  1 file changed, 156 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml b/Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml
> new file mode 100644
> index 000000000000..c7e5d3bd5ef4
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml
> @@ -0,0 +1,156 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/pressure/honeywell,hsc030pa.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Honeywell TruStability HSC and SSC pressure sensor families
> +
> +description: |
> +  support for Honeywell TruStability HSC and SSC digital pressure sensor
> +  families.
> +
> +  These sensors have either an I2C, an SPI or an analog interface. Only the
> +  digital versions are supported by this driver.
> +
> +  There are 118 models with different pressure ranges available in each family.
> +  The vendor calls them "HSC series" and "SSC series". All of them have an
> +  identical programming model but differ in 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 can either be provided via range_str or
> +  in case it's a custom chip via numerical range limits converted to pascals.
> +
> +  The transfer function defines the ranges of raw conversion values delivered
> +  by the sensor. pmin-pascal and pmax-pascal corespond to the minimum and
> +  maximum pressure that can be measured.
> +
> +  Specifications about the devices can be found at:
> +  https://prod-edam.honeywell.com/content/dam/honeywell-edam/sps/siot/en-us/products/sensors/pressure-sensors/board-mount-pressure-sensors/trustability-hsc-series/documents/sps-siot-trustability-hsc-series-high-accuracy-board-mount-pressure-sensors-50099148-a-en-ciid-151133.pdf
> +  https://prod-edam.honeywell.com/content/dam/honeywell-edam/sps/siot/en-us/products/sensors/pressure-sensors/board-mount-pressure-sensors/trustability-ssc-series/documents/sps-siot-trustability-ssc-series-standard-accuracy-board-mount-pressure-sensors-50099533-a-en-ciid-151134.pdf
> +
> +maintainers:
> +  - Petre Rodan <petre.rodan@subdimension.ro>
> +
> +properties:
> +  compatible:
> +    enum:
> +      - honeywell,hsc

Way too generic

> +      - honeywell,ssc

Way too generic

> +
> +  reg:
> +    maxItems: 1
> +
> +  honeywell,transfer-function:
> +    description: |
> +      Transfer function which defines the range of valid values delivered by
> +      the sensor.
> +      0 - A, 10% to 90% of 2^14
> +      1 - B, 5% to 95% of 2^14
> +      2 - C, 5% to 85% of 2^14
> +      3 - F, 4% to 94% of 2^14
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +
> +  honeywell,range_str:

No underscores in property names.

"str" is redundant. Instead say what is it, because "range" is way too
vague.

> +    description: |
> +      Five character string that defines "pressure range, unit and type"
> +      as part of the device nomenclature. In the unlikely case of a custom
> +      chip, set to "NA" and provide honeywell,pmin-pascal honeywell,pmax-pascal
> +    enum: [001BA, 1.6BA, 2.5BA, 004BA, 006BA, 010BA, 1.6MD, 2.5MD, 004MD,
> +           006MD, 010MD, 016MD, 025MD, 040MD, 060MD, 100MD, 160MD, 250MD,
> +           400MD, 600MD, 001BD, 1.6BD, 2.5BD, 004BD, 2.5MG, 004MG, 006MG,
> +           010MG, 016MG, 025MG, 040MG, 060MG, 100MG, 160MG, 250MG, 400MG,
> +           600MG, 001BG, 1.6BG, 2.5BG, 004BG, 006BG, 010BG, 100KA, 160KA,
> +           250KA, 400KA, 600KA, 001GA, 160LD, 250LD, 400LD, 600LD, 001KD,
> +           1.6KD, 2.5KD, 004KD, 006KD, 010KD, 016KD, 025KD, 040KD, 060KD,
> +           100KD, 160KD, 250KD, 400KD, 250LG, 400LG, 600LG, 001KG, 1.6KG,
> +           2.5KG, 004KG, 006KG, 010KG, 016KG, 025KG, 040KG, 060KG, 100KG,
> +           160KG, 250KG, 400KG, 600KG, 001GG, 015PA, 030PA, 060PA, 100PA,
> +           150PA, 0.5ND, 001ND, 002ND, 004ND, 005ND, 010ND, 020ND, 030ND,
> +           001PD, 005PD, 015PD, 030PD, 060PD, 001NG, 002NG, 004NG, 005NG,
> +           010NG, 020NG, 030NG, 001PG, 005PG, 015PG, 030PG, 060PG, 100PG,
> +           150PG, NA]
> +    $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,range_str is set to "NA".
> +    $ref: /schemas/types.yaml#/definitions/int32

That's uint32. Why do you need negative values?

> +
> +  honeywell,pmax-pascal:
> +    description: |
> +      Maximum pressure value the sensor can measure in pascal.
> +      To be specified only if honeywell,range_str is set to "NA".
> +    $ref: /schemas/types.yaml#/definitions/int32

Ditto

> +
> +  vdd-supply:
> +    description: |
> +      Provide VDD power to the sensor (either 3.3V or 5V depending on the chip).
> +      Optional, activate only if required by the target board.

Drop the last sentence. The supplies are required not by target board
but by hardware. I also do not understand what "activate" means in terms
of bindings and DTS.

> +
> +  spi-max-frequency:
> +    description: SPI clock to be kept between 50 and 800kHz

Drop description, add minimum/maximum constraints if worth.

> +
> +  clock-frequency:
> +    description: i2c clock to be kept between 100 and 400kHz

Drop, that's not really an I2C device property. Your driver must use
common clock framework.

> +
> +required:
> +  - compatible
> +  - reg
> +  - honeywell,transfer-function
> +  - honeywell,range_str
> +  - clock-frequency

Why?

> +  - spi-max-frequency
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +    i2c {
> +        status = "okay";

?!? Drop

> +        clock-frequency = <400000>;

Drop

> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        HSCMRNN030PA2A3@28 {

Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation

Plus, upper case is not allowed...

> +          status = "okay";

Drop. BTW status never comes first!

> +          compatible = "honeywell,hsc";
> +          reg = <0x28>;
> +
> +          honeywell,transfer-function = <0>;
> +          honeywell,range_str = "030PA";
> +        };
> +    };
> +
> +    spi {
> +        # note that MOSI is not required by this sensor

This should be then part of description, not example.

> +        status = "okay";

Drop

> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        HSCMLNN100PASA3@0 {

Eh...

> +          status = "okay";

Drop

> +          compatible = "honeywell,hsc";
> +          reg = <0>;
> +          spi-max-frequency = <800000>;
> +
> +          honeywell,transfer-function = <0>;
> +          honeywell,range_str = "100PA";
> +        };
> +
> +        HSC_CUSTOM_CHIP@0 {

Drop, not needed. One example is enough.

> +          status = "okay";
> +          compatible = "honeywell,hsc";
> +          reg = <1>;
> +          spi-max-frequency = <800000>;

Also, your indentation is broken.

Use 4 spaces for example indentation.

> +
> +          honeywell,transfer-function = <0>;
> +          honeywell,range_str = "NA";
> +          honeywell,pmin-pascal = <0>;
> +          honeywell,pmax-pascal = <206850>;
> +        };
> +

No stray blank lines.

Best regards,
Krzysztof


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

* Re: [PATCH 2/2] iio: pressure: driver for Honeywell HSC/SSC series pressure sensors
  2023-11-17 16:42 ` [PATCH 2/2] iio: pressure: driver for Honeywell HSC/SSC series pressure sensors Petre Rodan
  2023-11-18  5:21   ` [PATCH v2 " kernel test robot
@ 2023-11-20 12:35   ` Andy Shevchenko
  2023-11-22  6:08     ` Petre Rodan
  1 sibling, 1 reply; 24+ messages in thread
From: Andy Shevchenko @ 2023-11-20 12:35 UTC (permalink / raw)
  To: Petre Rodan
  Cc: linux-kernel, linux-iio, Jonathan Cameron, Lars-Peter Clausen,
	Angel Iglesias, Matti Vaittinen, Andreas Klinger, Rob Herring,
	Krzysztof Kozlowski

On Fri, Nov 17, 2023 at 06:42:06PM +0200, Petre Rodan wrote:
> Adds driver for Honeywell TruStability HSC and SSC series pressure and
> temperature sensors.
> 
> Covers i2c and spi-based interfaces. For both series it supports all the
> combinations of four transfer functions and all 118 pressure ranges.
> In case of a special chip not covered by the nomenclature a custom range
> can be specified.
> 
> Devices tested:
>  HSCMLNN100PASA3 (SPI sensor)
>  HSCMRNN030PA2A3 (i2c sensor)

> Datasheet:
>  [HSC] https://prod-edam.honeywell.com/content/dam/honeywell-edam/sps/siot/en-us/products/sensors/pressure-sensors/board-mount-pressure-sensors/trustability-hsc-series/documents/sps-siot-trustability-hsc-series-high-accuracy-board-mount-pressure-sensors-50099148-a-en-ciid-151133.pdf
>  [SSC] https://prod-edam.honeywell.com/content/dam/honeywell-edam/sps/siot/en-us/products/sensors/pressure-sensors/board-mount-pressure-sensors/trustability-ssc-series/documents/sps-siot-trustability-ssc-series-standard-accuracy-board-mount-pressure-sensors-50099533-a-en-ciid-151134.pdf
>  [i2c comms] https://prod-edam.honeywell.com/content/dam/honeywell-edam/sps/siot/en-us/products/sensors/pressure-sensors/board-mount-pressure-sensors/common/documents/sps-siot-i2c-comms-digital-output-pressure-sensors-tn-008201-3-en-ciid-45841.pdf

Make it a single tag per one URL as
Datasheet: URL_#1 [xxx]
Datasheet: URL_#2 [yyy]


> 

No blank line in tags block.

> Signed-off-by: Petre Rodan <petre.rodan@subdimension.ro>

...

> +config HSC030PA
> +	tristate "Honeywell HSC/SSC (TruStability pressure sensors series)"
> +	depends on (I2C || SPI_MASTER)

> +	select HSC030PA_I2C if (I2C)
> +	select HSC030PA_SPI if (SPI_MASTER)

Unneeded parentheses

> +	help
> +	  Say Y here to build support for the Honeywell HSC and SSC TruStability
> +      pressure and temperature sensor series.
> +
> +	  To compile this driver as a module, choose M here: the module will be
> +	  called hsc030pa.

Yeah besides indentation issues the LKP complain about this.

...

> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/init.h>
> +#include <linux/math64.h>
> +#include <linux/units.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/printk.h>

Keep them sorted alphabetically.
Also there are missing at least these ones: array_size.h, types.h.

+ blank line

> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>

+ blank line.

> +#include "hsc030pa.h"

...

> +// pressure range for current chip based on the nomenclature
> +struct hsc_range_config {
> +	char name[HSC_RANGE_STR_LEN];	// 5-char string that defines the range - ie "030PA"
> +	s32 pmin;		// minimal pressure in pascals
> +	s32 pmax;		// maximum pressure in pascals
> +};

Can you utilize linear ranges data types and APIs? (linear_range.h)

...

> +/*
> + * the first two bits from the first byte contain a status code
> + *
> + * 00 - normal operation, valid data
> + * 01 - device in hidden factory command mode
> + * 10 - stale data
> + * 11 - diagnostic condition
> + *
> + * function returns 1 only if both bits are zero
> + */

You really need to be consistent with style of multi-line comments.
And also C++/C variants. Besides that, respect English grammar and
punctuation.

...

> +static bool hsc_measurement_is_valid(struct hsc_data *data)
> +{
> +	if (data->buffer[0] & 0xc0)
> +		return 0;
> +
> +	return 1;

You use bool and return integers.

Besides, it can be just a oneliner.

	return !(buffer[0] & GENMASK(3, 2));

(Note, you will need bits.h for this.)

> +}

...

> +static int hsc_get_measurement(struct hsc_data *data)
> +{
> +	const struct hsc_chip_data *chip = data->chip;
> +	int ret;
> +
> +	/* don't bother sensor more than once a second */
> +	if (!time_after(jiffies, data->last_update + HZ))
> +		return data->is_valid ? 0 : -EAGAIN;
> +
> +	data->is_valid = false;
> +	data->last_update = jiffies;
> +
> +	ret = data->xfer(data);

> +

Redundant blank line.

> +	if (ret < 0)
> +		return ret;

> +	ret = chip->valid(data);
> +	if (!ret)
> +		return -EAGAIN;
> +
> +	data->is_valid = true;

Can this be like

	bool is_valid;

	is_valid = chip->valid(...);
	if (!is_valid)
		return ...


	data->is_valid = is_valid;

	// Depending on the flow you can even use that field directly

> +	return 0;
> +}

> +static int hsc_read_raw(struct iio_dev *indio_dev,
> +			struct iio_chan_spec const *channel, int *val,
> +			int *val2, long mask)
> +{
> +	struct hsc_data *data = iio_priv(indio_dev);

> +	int ret = -EINVAL;

Just use value directly, no need to have this assignment.

> +
> +	switch (mask) {

> +

Redundant blank line.

> +	case IIO_CHAN_INFO_RAW:
> +		mutex_lock(&data->lock);
> +		ret = hsc_get_measurement(data);
> +		mutex_unlock(&data->lock);

Use guard() operator from cleanup.h.

> +		if (ret)
> +			return ret;
> +
> +		switch (channel->type) {
> +		case IIO_PRESSURE:
> +			*val =
> +			    ((data->buffer[0] & 0x3f) << 8) + data->buffer[1];
> +			return IIO_VAL_INT;
> +		case IIO_TEMP:
> +			*val =
> +			    (data->buffer[2] << 3) +
> +			    ((data->buffer[3] & 0xe0) >> 5);

Is this some endianess / sign extension? Please convert using proper APIs.

> +			ret = 0;
> +			if (!ret)

lol

> +				return IIO_VAL_INT;
> +			break;
> +		default:
> +			return -EINVAL;
> +		}
> +		break;
> +
> +/**
> + *	IIO ABI expects
> + *	value = (conv + offset) * scale
> + *
> + *	datasheet provides the following formula for determining the temperature
> + *	temp[C] = conv * a + b
> + *        where a = 200/2047; b = -50
> + *
> + *	temp[C] = (conv + (b/a)) * a * (1000)
> + *      =>
> + *	scale = a * 1000 = .097703957 * 1000 = 97.703957
> + *	offset = b/a = -50 / .097703957 = -50000000 / 97704
> + *
> + *	based on the datasheet
> + *	pressure = (conv - HSC_OUTPUT_MIN) * Q + Pmin =
> + *	           ((conv - HSC_OUTPUT_MIN) + Pmin/Q) * Q
> + *	=>
> + *	scale = Q = (Pmax - Pmin) / (HSC_OUTPUT_MAX - HSC_OUTPUT_MIN)
> + *	offset = Pmin/Q = Pmin * (HSC_OUTPUT_MAX - HSC_OUTPUT_MIN) / (Pmax - Pmin)
> + */
> +
> +	case IIO_CHAN_INFO_SCALE:
> +		switch (channel->type) {
> +		case IIO_TEMP:
> +			*val = 97;
> +			*val2 = 703957;
> +			return IIO_VAL_INT_PLUS_MICRO;
> +		case IIO_PRESSURE:
> +			*val = data->p_scale;
> +			*val2 = data->p_scale_nano;
> +			return IIO_VAL_INT_PLUS_NANO;
> +		default:
> +			return -EINVAL;
> +		}

> +		break;
> +

Dead code?

> +	case IIO_CHAN_INFO_OFFSET:
> +		switch (channel->type) {
> +		case IIO_TEMP:
> +			*val = -50000000;
> +			*val2 = 97704;
> +			return IIO_VAL_FRACTIONAL;
> +		case IIO_PRESSURE:
> +			*val = data->p_offset;
> +			*val2 = data->p_offset_nano;
> +			return IIO_VAL_INT_PLUS_NANO;
> +		default:
> +			return -EINVAL;
> +		}
> +	}

> +	return ret;

Use default with explicit error code.

> +}

...

> +static const struct iio_chan_spec hsc_channels[] = {
> +	{
> +	 .type = IIO_PRESSURE,
> +	 .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> +	 BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_OFFSET)
> +	 },
> +	{
> +	 .type = IIO_TEMP,
> +	 .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> +	 BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_OFFSET)
> +	 },

Strange indentation of }:s...

> +};

...

> +int hsc_probe(struct iio_dev *indio_dev, struct device *dev,
> +	      const char *name, int type)
> +{
> +	struct hsc_data *hsc;
> +	u64 tmp;
> +	int index;
> +	int found = 0;
> +
> +	hsc = iio_priv(indio_dev);
> +
> +	hsc->last_update = jiffies - HZ;
> +	hsc->chip = &hsc_chip;
> +
> +	if (strcasecmp(hsc->range_str, "na") != 0) {
> +		// chip should be defined in the nomenclature
> +		for (index = 0; index < ARRAY_SIZE(hsc_range_config); index++) {
> +			if (strcasecmp
> +			    (hsc_range_config[index].name,
> +			     hsc->range_str) == 0) {
> +				hsc->pmin = hsc_range_config[index].pmin;
> +				hsc->pmax = hsc_range_config[index].pmax;
> +				found = 1;
> +				break;
> +			}
> +		}

Reinventing match_string() / sysfs_match_string() ?

> +		if (hsc->pmin == hsc->pmax || !found)
> +			return dev_err_probe(dev, -EINVAL,
> +					     "honeywell,range_str is invalid\n");
> +	}
> +
> +	hsc->outmin = hsc_func_spec[hsc->function].output_min;
> +	hsc->outmax = hsc_func_spec[hsc->function].output_max;
> +
> +	// multiply with MICRO and then divide by NANO since the output needs
> +	// to be in KPa as per IIO ABI requirement
> +	tmp = div_s64(((s64) (hsc->pmax - hsc->pmin)) * MICRO,
> +		      (hsc->outmax - hsc->outmin));
> +	hsc->p_scale = div_s64_rem(tmp, NANO, &hsc->p_scale_nano);
> +	tmp =
> +	    div_s64(((s64) hsc->pmin * (s64) (hsc->outmax - hsc->outmin)) *
> +		    MICRO, hsc->pmax - hsc->pmin);

No need to have space after castings

> +	hsc->p_offset =
> +	    div_s64_rem(tmp, NANO, &hsc->p_offset_nano) - hsc->outmin;
> +
> +	mutex_init(&hsc->lock);
> +	indio_dev->name = name;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->info = &hsc_info;
> +	indio_dev->channels = hsc->chip->channels;
> +	indio_dev->num_channels = hsc->chip->num_channels;
> +
> +	return devm_iio_device_register(dev, indio_dev);
> +}

This one devm wrapped...

> +void hsc_remove(struct iio_dev *indio_dev)
> +{
> +	iio_device_unregister(indio_dev);
> +}

...but not this. Why do you even need remove if the above is using devm always?

...

> + * Copyright (c) 2023 Petre Rodan <petre.rodan@subdimension.ro>

> + *

Redundant blank line.

...

> +#ifndef _HSC030PA_H
> +#define _HSC030PA_H

This header is using a lot and you are missing to include even a single
provider. :-(

At first glance:

mutex.h
types.h

A few forward declarations:

struct device;

struct iio_chan_spec;
struct iio_dev;

struct hsc_chip_data;

(Note blank lines in between.)

...

> +struct hsc_data {
> +	void *client;                           // either i2c or spi kernel interface struct for current dev
> +	const struct hsc_chip_data *chip;
> +	struct mutex lock;                      // lock protecting chip reads
> +	int (*xfer)(struct hsc_data *data);    // function that implements the chip reads
> +	bool is_valid;                          // false if last transfer has failed
> +	unsigned long last_update;              // time of last successful conversion
> +	u8 buffer[HSC_REG_MEASUREMENT_RD_SIZE]; // raw conversion data
> +	char range_str[HSC_RANGE_STR_LEN];	// range as defined by the chip nomenclature - ie "030PA" or "NA"
> +	s32 pmin;                               // min pressure limit
> +	s32 pmax;                               // max pressure limit
> +	u32 outmin;                             // minimum raw pressure in counts (based on transfer function)
> +	u32 outmax;                             // maximum raw pressure in counts (based on transfer function)
> +	u32 function;                           // transfer function
> +	s64 p_scale;                            // pressure scale
> +	s32 p_scale_nano;                       // pressure scale, decimal places
> +	s64 p_offset;                           // pressure offset
> +	s32 p_offset_nano;                      // pressure offset, decimal places
> +};
> +
> +struct hsc_chip_data {
> +	bool (*valid)(struct hsc_data *data);  // function that checks the two status bits
> +	const struct iio_chan_spec *channels;   // channel specifications
> +	u8 num_channels;                        // pressure and temperature channels
> +};

Convert comments to kernel-doc format.

...

> +enum hsc_func_id {
> +	HSC_FUNCTION_A,
> +	HSC_FUNCTION_B,
> +	HSC_FUNCTION_C,
> +	HSC_FUNCTION_F

Leave trailing comma. It make code slightly better to maintain.

> +};
> +
> +enum hsc_variant {
> +	HSC,
> +	SSC

Ditto.

> +};

...

> +static int hsc_i2c_xfer(struct hsc_data *data)
> +{
> +	struct i2c_client *client = data->client;
> +	struct i2c_msg msg;
> +	int ret;
> +
> +	msg.addr = client->addr;
> +	msg.flags = client->flags | I2C_M_RD;
> +	msg.len = HSC_REG_MEASUREMENT_RD_SIZE;
> +	msg.buf = (char *)&data->buffer;
> +
> +	ret = i2c_transfer(client->adapter, &msg, 1);
> +
> +	return (ret == 2) ? 0 : ret;
> +}

Can you use regmap I2C?

...

> +static int hsc_i2c_probe(struct i2c_client *client,
> +			 const struct i2c_device_id *id)

No use of this function prototype, we have a new one.

...

> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*hsc));
> +	if (!indio_dev) {

> +		dev_err(&client->dev, "Failed to allocate device\n");
> +		return -ENOMEM;

First of all, use

		return dev_err_probe();

Second, since it's ENOMEM, we do not issue an errors like this, error
code is already enough.

> +	}
> +
> +	hsc = iio_priv(indio_dev);

> +	if (i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
> +		hsc->xfer = hsc_i2c_xfer;
> +	else

Redundant 'else', see below.

> +		return -EOPNOTSUPP;

Use traditional pattern, i.e. checking for errors first:

	if (...)
		return ...

...

> +	ret = devm_regulator_get_enable_optional(dev, "vdd");
> +	if (ret == -EPROBE_DEFER)
> +		return -EPROBE_DEFER;

Oh, boy, this should check for ENODEV or so, yeah, regulator APIs a bit
interesting.

...

> +	if (!dev_fwnode(dev))
> +		return -EOPNOTSUPP;

Why do you need this?
And why this error code?


> +	ret = device_property_read_u32(dev,
> +				       "honeywell,transfer-function",
> +				       &hsc->function);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "honeywell,transfer-function could not be read\n");

...

> +	ret = device_property_read_string(dev,
> +					  "honeywell,range_str", &range_nom);

One line.

> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "honeywell,range_str not defined\n");

...

> +	memcpy(hsc->range_str, range_nom, HSC_RANGE_STR_LEN - 1);
> +	hsc->range_str[HSC_RANGE_STR_LEN - 1] = 0;

Oh, why do you need this all and can use the property value directly?
(Besides the fact the interesting operations are being used for strings.)

> +	if (strcasecmp(hsc->range_str, "na") == 0) {
> +		// "not available"
> +		// we got a custom-range chip not covered by the nomenclature
> +		ret = device_property_read_u32(dev,
> +					     "honeywell,pmin-pascal",
> +					     &hsc->pmin);
> +		if (ret)
> +			return dev_err_probe(dev, ret,
> +					     "honeywell,pmin-pascal could not be read\n");
> +		ret = device_property_read_u32(dev,
> +					     "honeywell,pmax-pascal",
> +					     &hsc->pmax);
> +		if (ret)
> +			return dev_err_probe(dev, ret,
> +					     "honeywell,pmax-pascal could not be read\n");
> +	}

...

> +	i2c_set_clientdata(client, indio_dev);

How is this being used?

> +	hsc->client = client;
> +
> +	return hsc_probe(indio_dev, &client->dev, id->name, id->driver_data);
> +}

...

> +static const struct of_device_id hsc_i2c_match[] = {
> +	{.compatible = "honeywell,hsc",},
> +	{.compatible = "honeywell,ssc",},

Inner commas are redundant.

> +	{},

Drop the comma in the terminator entry.

> +};

> +

Redundant blank line.

> +MODULE_DEVICE_TABLE(of, hsc_i2c_match);
> +
> +static const struct i2c_device_id hsc_i2c_id[] = {
> +	{"hsc", HSC},
> +	{"ssc", SSC},

Both ID tables should use pointers in driver data and respective API to get
that.

> +	{}
> +};

> +

Redundant blank line.

> +MODULE_DEVICE_TABLE(i2c, hsc_i2c_id);

...

> +static struct i2c_driver hsc_i2c_driver = {
> +	.driver = {
> +		   .name = "honeywell_hsc",
> +		   .of_match_table = hsc_i2c_match,

> +		   },

Indentation level can be dropped a bit.

> +	.probe = hsc_i2c_probe,
> +	.id_table = hsc_i2c_id,
> +};

> +

Redundant blank line.

> +module_i2c_driver(hsc_i2c_driver);

...

> +++ b/drivers/iio/pressure/hsc030pa_spi.c

...

> +	int ret;
> +
> +	ret = spi_sync_transfer(data->client, &xfer, 1);
> +
> +	return ret;

So, why ret is needed?

...

> +	spi_set_drvdata(spi, indio_dev);

How is this being used?

> +	spi->mode = SPI_MODE_0;
> +	spi->max_speed_hz = min(spi->max_speed_hz, 800000U);
> +	spi->bits_per_word = 8;
> +	ret = spi_setup(spi);
> +	if (ret < 0)
> +		return ret;

Why the firmware can't provide the correct information to begin with?

...

> +	ret = devm_regulator_get_enable_optional(dev, "vdd");
> +	if (ret == -EPROBE_DEFER)
> +		return -EPROBE_DEFER;

As per I2C driver.

But why is not in the main ->probe()?

...

> +	ret = device_property_read_u32(dev,
> +				       "honeywell,transfer-function",
> +				       &hsc->function);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "honeywell,transfer-function could not be read\n");
> +	if (hsc->function > HSC_FUNCTION_F)
> +		return dev_err_probe(dev, -EINVAL,
> +				     "honeywell,transfer-function %d invalid\n",
> +				     hsc->function);
> +
> +	ret =
> +	    device_property_read_string(dev, "honeywell,range_str", &range_nom);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "honeywell,range_str not defined\n");
> +
> +	// minimal input sanitization
> +	memcpy(hsc->range_str, range_nom, HSC_RANGE_STR_LEN - 1);
> +	hsc->range_str[HSC_RANGE_STR_LEN - 1] = 0;
> +
> +	if (strcasecmp(hsc->range_str, "na") == 0) {
> +		// range string "not available"
> +		// we got a custom chip not covered by the nomenclature with a custom range
> +		ret = device_property_read_u32(dev, "honeywell,pmin-pascal",
> +					       &hsc->pmin);
> +		if (ret)
> +			return dev_err_probe(dev, ret,
> +					     "honeywell,pmin-pascal could not be read\n");
> +		ret = device_property_read_u32(dev, "honeywell,pmax-pascal",
> +					       &hsc->pmax);
> +		if (ret)
> +			return dev_err_probe(dev, ret,
> +					     "honeywell,pmax-pascal could not be read\n");
> +	}

Ditto. Why is this duplication?

...

> +static const struct of_device_id hsc_spi_match[] = {
> +	{.compatible = "honeywell,hsc",},
> +	{.compatible = "honeywell,ssc",},
> +	{},
> +};
> +
> +MODULE_DEVICE_TABLE(of, hsc_spi_match);
> +
> +static const struct spi_device_id hsc_spi_id[] = {
> +	{"hsc", HSC},
> +	{"ssc", SSC},
> +	{}
> +};
> +
> +MODULE_DEVICE_TABLE(spi, hsc_spi_id);
> +
> +static struct spi_driver hsc_spi_driver = {
> +	.driver = {
> +		   .name = "honeywell_hsc",
> +		   .of_match_table = hsc_spi_match,
> +		   },
> +	.probe = hsc_spi_probe,
> +	.id_table = hsc_spi_id,
> +};
> +
> +module_spi_driver(hsc_spi_driver);

Same comments as per I2C driver above.

-- 
With Best Regards,
Andy Shevchenko



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

* Re: [PATCH v2 1/2] dt-bindings: iio: pressure: add honeywell,hsc030
  2023-11-20 10:21   ` Krzysztof Kozlowski
@ 2023-11-20 13:42     ` Petre Rodan
  2023-11-20 14:04       ` Krzysztof Kozlowski
  0 siblings, 1 reply; 24+ messages in thread
From: Petre Rodan @ 2023-11-20 13:42 UTC (permalink / raw)
  To: Krzysztof Kozlowski
  Cc: linux-kernel, linux-iio, devicetree, Conor Dooley,
	Lars-Peter Clausen, Rob Herring, Krzysztof Kozlowski,
	linux-kernel-mentees, Jonathan Cameron


Hello Krzysztof,

thanks for the pointer regarding the version requirement for jsonschema.
installing an older version fixed all python exceptions.


On Mon, Nov 20, 2023 at 11:21:42AM +0100, Krzysztof Kozlowski wrote:
> On 17/11/2023 20:22, Petre Rodan wrote:
> > Adds binding for digital Honeywell TruStability HSC and SSC series pressure 
> > and temperature sensors.
> > 
> > Datasheet:
> >  [HSC] https://prod-edam.honeywell.com/content/dam/honeywell-edam/sps/siot/en-us/products/sensors/pressure-sensors/board-mount-pressure-sensors/trustability-hsc-series/documents/sps-siot-trustability-hsc-series-high-accuracy-board-mount-pressure-sensors-50099148-a-en-ciid-151133.pdf
> >  [SSC] https://prod-edam.honeywell.com/content/dam/honeywell-edam/sps/siot/en-us/products/sensors/pressure-sensors/board-mount-pressure-sensors/trustability-ssc-series/documents/sps-siot-trustability-ssc-series-standard-accuracy-board-mount-pressure-sensors-50099533-a-en-ciid-151134.pdf

> > +properties:
> > +  compatible:
> > +    enum:
> > +      - honeywell,hsc
> 
> Way too generic

I'm new to this, please excuse my ignorance.
my driver covers all Honeywell pressure sensors under the "TruStability board mount HSC/SSC" moniker.
that is why my intention was to provide a rather generic name for the driver itself.
are you afraid that they will come up with a different device that they will call "hsc" in the future?
in this case honeywell,trustability-hsc would be fine?

as I see you prefer to target a particular chip, but I am a bit afraid that the end-user will be confused by needing to set up something like

pressure@28 {
	compatible = "honeywell,hsc030pa";
	reg = <0x28>;
	honeywell,transfer-function = <0>;
	honeywell,pressure-range = "250MD";
};

ie. specifying "hsc030pa" as driver while his chip is not in the 030PA range, but 250MD.

so do you prefer
 honeywell,trustability-hsc  OR
 honeywell,hsc030pa

> > +  honeywell,range_str:
> 
> No underscores in property names.
> 
> "str" is redundant. Instead say what is it, because "range" is way too
> vague.

will rename to honeywell,pressure-range if that is ok with you.

> > +    description: |
> > +      Five character string that defines "pressure range, unit and type"
> > +      as part of the device nomenclature. In the unlikely case of a custom
> > +      chip, set to "NA" and provide honeywell,pmin-pascal honeywell,pmax-pascal
> > +    enum: [001BA, 1.6BA, 2.5BA, 004BA, 006BA, 010BA, 1.6MD, 2.5MD, 004MD,
> > +           006MD, 010MD, 016MD, 025MD, 040MD, 060MD, 100MD, 160MD, 250MD,
> > +           400MD, 600MD, 001BD, 1.6BD, 2.5BD, 004BD, 2.5MG, 004MG, 006MG,
> > +           010MG, 016MG, 025MG, 040MG, 060MG, 100MG, 160MG, 250MG, 400MG,
> > +           600MG, 001BG, 1.6BG, 2.5BG, 004BG, 006BG, 010BG, 100KA, 160KA,
> > +           250KA, 400KA, 600KA, 001GA, 160LD, 250LD, 400LD, 600LD, 001KD,
> > +           1.6KD, 2.5KD, 004KD, 006KD, 010KD, 016KD, 025KD, 040KD, 060KD,
> > +           100KD, 160KD, 250KD, 400KD, 250LG, 400LG, 600LG, 001KG, 1.6KG,
> > +           2.5KG, 004KG, 006KG, 010KG, 016KG, 025KG, 040KG, 060KG, 100KG,
> > +           160KG, 250KG, 400KG, 600KG, 001GG, 015PA, 030PA, 060PA, 100PA,
> > +           150PA, 0.5ND, 001ND, 002ND, 004ND, 005ND, 010ND, 020ND, 030ND,
> > +           001PD, 005PD, 015PD, 030PD, 060PD, 001NG, 002NG, 004NG, 005NG,
> > +           010NG, 020NG, 030NG, 001PG, 005PG, 015PG, 030PG, 060PG, 100PG,
> > +           150PG, NA]
> > +    $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,range_str is set to "NA".
> > +    $ref: /schemas/types.yaml#/definitions/int32
> 
> That's uint32. Why do you need negative values?

signed int32 is intentional. some chips have two physical input ports and measure a pressure differential in which case pmin is negative.
see either of the pdfs at page 14, table 8, column 2, row 7+

> > +  honeywell,pmax-pascal:
> > +    description: |
> > +      Maximum pressure value the sensor can measure in pascal.
> > +      To be specified only if honeywell,range_str is set to "NA".
> > +    $ref: /schemas/types.yaml#/definitions/int32
> 
> Ditto

well, since we saw pmin needs to be signed should we have pmax unsigned?

> > +  vdd-supply:
> > +    description: |
> > +      Provide VDD power to the sensor (either 3.3V or 5V depending on the chip).
> > +      Optional, activate only if required by the target board.
> 
> Drop the last sentence. The supplies are required not by target board
> but by hardware. I also do not understand what "activate" means in terms
> of bindings and DTS.

ok, ignore rambling.

> > +
> > +  spi-max-frequency:
> > +    description: SPI clock to be kept between 50 and 800kHz
> 
> Drop description, add minimum/maximum constraints if worth.

will replace block with

  spi-max-frequency:
    maximum: 800000

as I saw in other yaml files
 
> > +  clock-frequency:
> > +    description: i2c clock to be kept between 100 and 400kHz
> 
> Drop, that's not really an I2C device property. Your driver must use
> common clock framework.

ack

> > +required:
> > +  - compatible
> > +  - reg
> > +  - honeywell,transfer-function
> > +  - honeywell,range_str
> > +  - clock-frequency
> 
> Why?

dropped clock-frequency

everything else below will be as you asked.

I will provide a new set of patches after I get your inpyt.

my very best regards,
peter

> > +  - spi-max-frequency
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/gpio/gpio.h>
> > +    i2c {
> > +        status = "okay";
> 
> ?!? Drop
> 
> > +        clock-frequency = <400000>;
> 
> Drop
> 
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        HSCMRNN030PA2A3@28 {
> 
> Node names should be generic. See also an explanation and list of
> examples (not exhaustive) in DT specification:
> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
> 
> Plus, upper case is not allowed...
> 
> > +          status = "okay";
> 
> Drop. BTW status never comes first!
> 
> > +          compatible = "honeywell,hsc";
> > +          reg = <0x28>;
> > +
> > +          honeywell,transfer-function = <0>;
> > +          honeywell,range_str = "030PA";
> > +        };
> > +    };
> > +
> > +    spi {
> > +        # note that MOSI is not required by this sensor
> 
> This should be then part of description, not example.
> 
> > +        status = "okay";
> 
> Drop
> 
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        HSCMLNN100PASA3@0 {
> 
> Eh...
> 
> > +          status = "okay";
> 
> Drop
> 
> > +          compatible = "honeywell,hsc";
> > +          reg = <0>;
> > +          spi-max-frequency = <800000>;
> > +
> > +          honeywell,transfer-function = <0>;
> > +          honeywell,range_str = "100PA";
> > +        };
> > +
> > +        HSC_CUSTOM_CHIP@0 {
> 
> Drop, not needed. One example is enough.
> 
> > +          status = "okay";
> > +          compatible = "honeywell,hsc";
> > +          reg = <1>;
> > +          spi-max-frequency = <800000>;
> 
> Also, your indentation is broken.
> 
> Use 4 spaces for example indentation.
> 
> > +
> > +          honeywell,transfer-function = <0>;
> > +          honeywell,range_str = "NA";
> > +          honeywell,pmin-pascal = <0>;
> > +          honeywell,pmax-pascal = <206850>;
> > +        };
> > +
> 
> No stray blank lines.
> 
> Best regards,
> Krzysztof
> 
> 

-- 
petre rodan

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

* Re: [PATCH v2 1/2] dt-bindings: iio: pressure: add honeywell,hsc030
  2023-11-20 13:42     ` Petre Rodan
@ 2023-11-20 14:04       ` Krzysztof Kozlowski
  2023-11-20 14:40         ` Petre Rodan
  0 siblings, 1 reply; 24+ messages in thread
From: Krzysztof Kozlowski @ 2023-11-20 14:04 UTC (permalink / raw)
  To: Petre Rodan
  Cc: linux-kernel, linux-iio, devicetree, Conor Dooley,
	Lars-Peter Clausen, Rob Herring, Krzysztof Kozlowski,
	linux-kernel-mentees, Jonathan Cameron

On 20/11/2023 14:42, Petre Rodan wrote:

>>> +properties:
>>> +  compatible:
>>> +    enum:
>>> +      - honeywell,hsc
>>
>> Way too generic
> 
> I'm new to this, please excuse my ignorance.
> my driver covers all Honeywell pressure sensors under the "TruStability board mount HSC/SSC" moniker.

We talk here about bindings, not driver. For the driver you can use
whatever name is approved by reviewers of your driver.

> that is why my intention was to provide a rather generic name for the driver itself.
> are you afraid that they will come up with a different device that they will call "hsc" in the future?
> in this case honeywell,trustability-hsc would be fine?
> 
> as I see you prefer to target a particular chip, but I am a bit afraid that the end-user will be confused by needing to set up something like
> 
> pressure@28 {
> 	compatible = "honeywell,hsc030pa";

The compatible should be specific, thus for example match exact model
number.

If you can guarantee that all devices from given family are the same in
respect of programming model and hardware requirements (e.g. supplies),
then you could go with family name. However such guarantees are rarely
given. Therefore for mprls0025pa I agreed for using one specific model
for entire family.

https://lore.kernel.org/all/d577bc44-780f-f25d-29c6-ed1d353b540c@linaro.org/


> 	reg = <0x28>;
> 	honeywell,transfer-function = <0>;
> 	honeywell,pressure-range = "250MD";
> };
> 
> ie. specifying "hsc030pa" as driver while his chip is not in the 030PA range, but 250MD.
> 
> so do you prefer
>  honeywell,trustability-hsc  OR
>  honeywell,hsc030pa

I think the latter, just like we did for mprls0025pa. How many devices
do you have there?


> 
>>> +  honeywell,range_str:
>>
>> No underscores in property names.
>>
>> "str" is redundant. Instead say what is it, because "range" is way too
>> vague.
> 
> will rename to honeywell,pressure-range if that is ok with you.

Yes

> 
>>> +    description: |
>>> +      Five character string that defines "pressure range, unit and type"
>>> +      as part of the device nomenclature. In the unlikely case of a custom
>>> +      chip, set to "NA" and provide honeywell,pmin-pascal honeywell,pmax-pascal
>>> +    enum: [001BA, 1.6BA, 2.5BA, 004BA, 006BA, 010BA, 1.6MD, 2.5MD, 004MD,
>>> +           006MD, 010MD, 016MD, 025MD, 040MD, 060MD, 100MD, 160MD, 250MD,
>>> +           400MD, 600MD, 001BD, 1.6BD, 2.5BD, 004BD, 2.5MG, 004MG, 006MG,
>>> +           010MG, 016MG, 025MG, 040MG, 060MG, 100MG, 160MG, 250MG, 400MG,
>>> +           600MG, 001BG, 1.6BG, 2.5BG, 004BG, 006BG, 010BG, 100KA, 160KA,
>>> +           250KA, 400KA, 600KA, 001GA, 160LD, 250LD, 400LD, 600LD, 001KD,
>>> +           1.6KD, 2.5KD, 004KD, 006KD, 010KD, 016KD, 025KD, 040KD, 060KD,
>>> +           100KD, 160KD, 250KD, 400KD, 250LG, 400LG, 600LG, 001KG, 1.6KG,
>>> +           2.5KG, 004KG, 006KG, 010KG, 016KG, 025KG, 040KG, 060KG, 100KG,
>>> +           160KG, 250KG, 400KG, 600KG, 001GG, 015PA, 030PA, 060PA, 100PA,
>>> +           150PA, 0.5ND, 001ND, 002ND, 004ND, 005ND, 010ND, 020ND, 030ND,
>>> +           001PD, 005PD, 015PD, 030PD, 060PD, 001NG, 002NG, 004NG, 005NG,
>>> +           010NG, 020NG, 030NG, 001PG, 005PG, 015PG, 030PG, 060PG, 100PG,
>>> +           150PG, NA]
>>> +    $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,range_str is set to "NA".
>>> +    $ref: /schemas/types.yaml#/definitions/int32
>>
>> That's uint32. Why do you need negative values?
> 
> signed int32 is intentional. some chips have two physical input ports and measure a pressure differential in which case pmin is negative.
> see either of the pdfs at page 14, table 8, column 2, row 7+

Then the best would be to change the type in other bindings to have
int32 everywhere... but dtschema also requires unt32:
https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/property-units.yaml

I think pressure can be negative (e.g. when device measures pressure in
relation to atmospheric pressure), thus maybe we need to fix dtschema first.

> 
>>> +  honeywell,pmax-pascal:
>>> +    description: |
>>> +      Maximum pressure value the sensor can measure in pascal.
>>> +      To be specified only if honeywell,range_str is set to "NA".
>>> +    $ref: /schemas/types.yaml#/definitions/int32
>>
>> Ditto
> 
> well, since we saw pmin needs to be signed should we have pmax unsigned?

I guess this could stay uint32, although it depends on final units for
pascal.

Best regards,
Krzysztof


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

* Re: [PATCH v2 1/2] dt-bindings: iio: pressure: add honeywell,hsc030
  2023-11-20 14:04       ` Krzysztof Kozlowski
@ 2023-11-20 14:40         ` Petre Rodan
  2023-11-20 17:39           ` Jonathan Cameron
  0 siblings, 1 reply; 24+ messages in thread
From: Petre Rodan @ 2023-11-20 14:40 UTC (permalink / raw)
  To: Krzysztof Kozlowski
  Cc: linux-kernel, linux-iio, devicetree, Conor Dooley,
	Lars-Peter Clausen, Rob Herring, Krzysztof Kozlowski,
	linux-kernel-mentees, Jonathan Cameron


Hello!

On Mon, Nov 20, 2023 at 03:04:07PM +0100, Krzysztof Kozlowski wrote:
> On 20/11/2023 14:42, Petre Rodan wrote:
> 
> >>> +properties:
> >>> +  compatible:
> >>> +    enum:
> >>> +      - honeywell,hsc
> >>
> >> Way too generic
> > 
> > I'm new to this, please excuse my ignorance.
> > my driver covers all Honeywell pressure sensors under the "TruStability board mount HSC/SSC" moniker.
> 
> We talk here about bindings, not driver. For the driver you can use
> whatever name is approved by reviewers of your driver.
> 
> > that is why my intention was to provide a rather generic name for the driver itself.
> > are you afraid that they will come up with a different device that they will call "hsc" in the future?
> > in this case honeywell,trustability-hsc would be fine?
> > 
> > as I see you prefer to target a particular chip, but I am a bit afraid that the end-user will be confused by needing to set up something like
> > 
> > pressure@28 {
> > 	compatible = "honeywell,hsc030pa";
> 
> The compatible should be specific, thus for example match exact model
> number.

there are an infinite number of combinations of 4 transfer functions and 118 ranges + one custom range, so providing an array with all specific chips that could end up as compatible is out of the question.
I was aiming at providing a generic name for the binding and get the transfer function and the pressure range as required parameters.

> If you can guarantee that all devices from given family are the same in
> respect of programming model and hardware requirements (e.g. supplies),
> then you could go with family name. However such guarantees are rarely
> given.

I see your point.

> Therefore for mprls0025pa I agreed for using one specific model
> for entire family.
> 
> https://lore.kernel.org/all/d577bc44-780f-f25d-29c6-ed1d353b540c@linaro.org/
> 
> 
> > 	reg = <0x28>;
> > 	honeywell,transfer-function = <0>;
> > 	honeywell,pressure-range = "250MD";
> > };
> > 
> > ie. specifying "hsc030pa" as driver while his chip is not in the 030PA range, but 250MD.
> > 
> > so do you prefer
> >  honeywell,trustability-hsc  OR
> >  honeywell,hsc030pa
> 
> I think the latter, just like we did for mprls0025pa. How many devices
> do you have there?

both hsc and ssc have 118 ranges, 4 transfer functions and both can be requested from the manufacturer with custom measurement ranges.

ok,I will rename hsc->hsc030pa in the code as you requested.

> >>> +  honeywell,pmin-pascal:
> >>> +    description: |
> >>> +      Minimum pressure value the sensor can measure in pascal.
> >>> +      To be specified only if honeywell,range_str is set to "NA".
> >>> +    $ref: /schemas/types.yaml#/definitions/int32
> >>
> >> That's uint32. Why do you need negative values?
> > 
> > signed int32 is intentional. some chips have two physical input ports and measure a pressure differential in which case pmin is negative.
> > see either of the pdfs at page 14, table 8, column 2, row 7+
> 
> Then the best would be to change the type in other bindings to have
> int32 everywhere... but dtschema also requires unt32:
> https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/property-units.yaml

yeah, and also '-kpascal' might be too coarse of a unit when we talk about sensors that have a -125 .. 125 pascal measurement range as '0.5ND' has.

cheers,
peter

-- 
petre rodan

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

* Re: [PATCH v2 1/2] dt-bindings: iio: pressure: add honeywell,hsc030
  2023-11-19 20:14     ` Petre Rodan
  2023-11-20 10:15       ` Krzysztof Kozlowski
@ 2023-11-20 17:19       ` Rob Herring
  2023-11-20 18:09         ` Petre Rodan
  1 sibling, 1 reply; 24+ messages in thread
From: Rob Herring @ 2023-11-20 17:19 UTC (permalink / raw)
  To: Petre Rodan
  Cc: linux-kernel, linux-iio, devicetree, Conor Dooley,
	Lars-Peter Clausen, Krzysztof Kozlowski, linux-kernel-mentees,
	Jonathan Cameron

On Sun, Nov 19, 2023 at 10:14:58PM +0200, Petre Rodan wrote:
> 
> Good morning!
> 
> On Sun, Nov 19, 2023 at 07:49:39AM -0600, Rob Herring wrote:
> > On Fri, Nov 17, 2023 at 09:22:57PM +0200, Petre Rodan wrote:
> > > Adds binding for digital Honeywell TruStability HSC and SSC series pressure 
> > > and temperature sensors.
> > > 
> [..]
> > > Changes for v2:
> > > - Removed redundant quotations reported by robh's bot
> > > - Fixed yamllint warnings
> > > 
> > > I'm failing to run 'make DT_CHECKER_FLAGS=-m dt_binding_check' due to
> > > python errors and exceptions
> > 
> > What exceptions?
> 
> thanks for asking.
> 
> first off, installed packages. the first 4 are not part of the 
> official Gentoo repo, so I might have prepared them with missing 
> options if any where not included by default.
> I know nothing about python.
> 
> $ equery l dtschema pylibfdt ruamel-yaml yamllint jsonschema python 
> [I-O] [  ] dev-python/dtschema-2023.9:0
> [I-O] [  ] dev-python/pylibfdt-1.7.0_p1:0
> [I-O] [  ] dev-python/ruamel-yaml-0.18.5:0
> [I-O] [  ] dev-python/yamllint-1.33.0:0
> [IP-] [  ] dev-python/jsonschema-4.19.1:0

4.18 and later are not supported.

Apparently behavior we relied on in pre-4.18 was "wrong" usage... 4.18 
also makes rust a hard dependency. That's a problem for any arch without 
LLVM support.

Installing via pip will check this dependency.

Rob

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

* Re: [PATCH v2 1/2] dt-bindings: iio: pressure: add honeywell,hsc030
  2023-11-20 14:40         ` Petre Rodan
@ 2023-11-20 17:39           ` Jonathan Cameron
  2023-11-20 18:25             ` Petre Rodan
  0 siblings, 1 reply; 24+ messages in thread
From: Jonathan Cameron @ 2023-11-20 17:39 UTC (permalink / raw)
  To: Petre Rodan
  Cc: Krzysztof Kozlowski, linux-kernel, linux-iio, devicetree,
	Conor Dooley, Lars-Peter Clausen, Rob Herring,
	Krzysztof Kozlowski, linux-kernel-mentees, Jonathan Cameron

On Mon, 20 Nov 2023 16:40:51 +0200
Petre Rodan <petre.rodan@subdimension.ro> wrote:

> Hello!
> 
> On Mon, Nov 20, 2023 at 03:04:07PM +0100, Krzysztof Kozlowski wrote:
> > On 20/11/2023 14:42, Petre Rodan wrote:
> >   
> > >>> +properties:
> > >>> +  compatible:
> > >>> +    enum:
> > >>> +      - honeywell,hsc  
> > >>
> > >> Way too generic  
> > > 
> > > I'm new to this, please excuse my ignorance.
> > > my driver covers all Honeywell pressure sensors under the "TruStability board mount HSC/SSC" moniker.  
> > 
> > We talk here about bindings, not driver. For the driver you can use
> > whatever name is approved by reviewers of your driver.
> >   
> > > that is why my intention was to provide a rather generic name for the driver itself.
> > > are you afraid that they will come up with a different device that they will call "hsc" in the future?
> > > in this case honeywell,trustability-hsc would be fine?
> > > 
> > > as I see you prefer to target a particular chip, but I am a bit afraid that the end-user will be confused by needing to set up something like
> > > 
> > > pressure@28 {
> > > 	compatible = "honeywell,hsc030pa";  
> > 
> > The compatible should be specific, thus for example match exact model
> > number.  
> 
> there are an infinite number of combinations of 4 transfer functions and 118 ranges + one custom range, so providing an array with all specific chips that could end up as compatible is out of the question.
> I was aiming at providing a generic name for the binding and get the transfer function and the pressure range as required parameters.
> 
> > If you can guarantee that all devices from given family are the same in
> > respect of programming model and hardware requirements (e.g. supplies),
> > then you could go with family name. However such guarantees are rarely
> > given.  
> 
> I see your point.
> 
> > Therefore for mprls0025pa I agreed for using one specific model
> > for entire family.
> > 
> > https://lore.kernel.org/all/d577bc44-780f-f25d-29c6-ed1d353b540c@linaro.org/
> > 
> >   
> > > 	reg = <0x28>;
> > > 	honeywell,transfer-function = <0>;
> > > 	honeywell,pressure-range = "250MD";
> > > };
> > > 
> > > ie. specifying "hsc030pa" as driver while his chip is not in the 030PA range, but 250MD.
> > > 
> > > so do you prefer
> > >  honeywell,trustability-hsc  OR
> > >  honeywell,hsc030pa  
> > 
> > I think the latter, just like we did for mprls0025pa. How many devices
> > do you have there?  
> 
> both hsc and ssc have 118 ranges, 4 transfer functions and both can be requested from the manufacturer with custom measurement ranges.
> 
> ok,I will rename hsc->hsc030pa in the code as you requested.

Where does pa come from? 

If we are going generic, feels like trustability-ssc etc are more representative
and matches the datasheet cover page.


> 
> > >>> +  honeywell,pmin-pascal:
> > >>> +    description: |
> > >>> +      Minimum pressure value the sensor can measure in pascal.
> > >>> +      To be specified only if honeywell,range_str is set to "NA".
> > >>> +    $ref: /schemas/types.yaml#/definitions/int32  
> > >>
> > >> That's uint32. Why do you need negative values?  
> > > 
> > > signed int32 is intentional. some chips have two physical input ports and measure a pressure differential in which case pmin is negative.
> > > see either of the pdfs at page 14, table 8, column 2, row 7+  
> > 
> > Then the best would be to change the type in other bindings to have
> > int32 everywhere... but dtschema also requires unt32:
> > https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/property-units.yaml  
> 
> yeah, and also '-kpascal' might be too coarse of a unit when we talk about sensors that have a -125 .. 125 pascal measurement range as '0.5ND' has.
> 
> cheers,
> peter
> 


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

* Re: [PATCH v2 1/2] dt-bindings: iio: pressure: add honeywell,hsc030
  2023-11-20 17:19       ` Rob Herring
@ 2023-11-20 18:09         ` Petre Rodan
  0 siblings, 0 replies; 24+ messages in thread
From: Petre Rodan @ 2023-11-20 18:09 UTC (permalink / raw)
  To: Rob Herring
  Cc: linux-kernel, linux-iio, devicetree, Conor Dooley,
	Lars-Peter Clausen, Krzysztof Kozlowski, linux-kernel-mentees,
	Jonathan Cameron


hello!

On Mon, Nov 20, 2023 at 10:19:03AM -0700, Rob Herring wrote:
> > first off, installed packages. the first 4 are not part of the 
> > official Gentoo repo, so I might have prepared them with missing 
> > options if any where not included by default.
> > I know nothing about python.
> > 
> > $ equery l dtschema pylibfdt ruamel-yaml yamllint jsonschema python 
[..]
> > [IP-] [  ] dev-python/jsonschema-4.19.1:0
> 
> 4.18 and later are not supported.
> 
> Apparently behavior we relied on in pre-4.18 was "wrong" usage... 4.18 
> also makes rust a hard dependency. That's a problem for any arch without 
> LLVM support.
> 
> Installing via pip will check this dependency.

I confirm that installing ver 4.17 of jsonschema fixed all the exceptions.

thanks.
peter

-- 
petre rodan

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

* Re: [PATCH v2 1/2] dt-bindings: iio: pressure: add honeywell,hsc030
  2023-11-20 17:39           ` Jonathan Cameron
@ 2023-11-20 18:25             ` Petre Rodan
  2023-11-25 19:21               ` Jonathan Cameron
  0 siblings, 1 reply; 24+ messages in thread
From: Petre Rodan @ 2023-11-20 18:25 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: Krzysztof Kozlowski, linux-kernel, linux-iio, devicetree,
	Conor Dooley, Lars-Peter Clausen, Rob Herring,
	Krzysztof Kozlowski, linux-kernel-mentees, Jonathan Cameron


Hello!

On Mon, Nov 20, 2023 at 05:39:29PM +0000, Jonathan Cameron wrote:
> On Mon, 20 Nov 2023 16:40:51 +0200
> Petre Rodan <petre.rodan@subdimension.ro> wrote:
> 
> > Hello!
> > 
> > On Mon, Nov 20, 2023 at 03:04:07PM +0100, Krzysztof Kozlowski wrote:
> > > On 20/11/2023 14:42, Petre Rodan wrote:
> > >   
> > > >>> +properties:
> > > >>> +  compatible:
> > > >>> +    enum:
> > > >>> +      - honeywell,hsc  
> > > >>
> > > >> Way too generic  
> > > > 
> > > > I'm new to this, please excuse my ignorance.
> > > > my driver covers all Honeywell pressure sensors under the "TruStability board mount HSC/SSC" moniker.  
> > > 
> > > We talk here about bindings, not driver. For the driver you can use
> > > whatever name is approved by reviewers of your driver.
> > >   
> > > > that is why my intention was to provide a rather generic name for the driver itself.
> > > > are you afraid that they will come up with a different device that they will call "hsc" in the future?
> > > > in this case honeywell,trustability-hsc would be fine?
> > > > 
> > > > as I see you prefer to target a particular chip, but I am a bit afraid that the end-user will be confused by needing to set up something like
> > > > 
> > > > pressure@28 {
> > > > 	compatible = "honeywell,hsc030pa";  
> > > 
> > > The compatible should be specific, thus for example match exact model
> > > number.  
> > 
> > there are an infinite number of combinations of 4 transfer functions and 118 ranges + one custom range, so providing an array with all specific chips that could end up as compatible is out of the question.
> > I was aiming at providing a generic name for the binding and get the transfer function and the pressure range as required parameters.
> > 
> > > If you can guarantee that all devices from given family are the same in
> > > respect of programming model and hardware requirements (e.g. supplies),
> > > then you could go with family name. However such guarantees are rarely
> > > given.  
> > 
> > I see your point.
> > 
> > > Therefore for mprls0025pa I agreed for using one specific model
> > > for entire family.
> > > 
> > > https://lore.kernel.org/all/d577bc44-780f-f25d-29c6-ed1d353b540c@linaro.org/
> > > 
> > >   
> > > > 	reg = <0x28>;
> > > > 	honeywell,transfer-function = <0>;
> > > > 	honeywell,pressure-range = "250MD";
> > > > };
> > > > 
> > > > ie. specifying "hsc030pa" as driver while his chip is not in the 030PA range, but 250MD.
> > > > 
> > > > so do you prefer
> > > >  honeywell,trustability-hsc  OR
> > > >  honeywell,hsc030pa  
> > > 
> > > I think the latter, just like we did for mprls0025pa. How many devices
> > > do you have there?  
> > 
> > both hsc and ssc have 118 ranges, 4 transfer functions and both can be requested from the manufacturer with custom measurement ranges.
> > 
> > ok,I will rename hsc->hsc030pa in the code as you requested.
> 
> Where does pa come from? 

honeywell,hsc030pa was provided as an equivalent to honeywell,mprls0025pa (which is already in the repo).

'030PA' and '0025PA' define the pressure range (0-30, 0-25), the unit of measure (Psi) and the measurement type (Absolute) for a particular chip in the honeywell catalog. (please ignore the psi part, we convert everything to pascals).
but both my driver and Andreas Klinger's mprls0025pa actually provide a generic abstraction layer for entire series of sensors.

> If we are going generic, feels like trustability-ssc etc are more representative
> and matches the datasheet cover page.

Krzysztof voted for non-generic, honeywell,mprls0025pa is already set up non-generic, my intent was to go generic.

I'll rewrite the code to whatever you guys feel is best.

peter


-- 
petre rodan

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

* Re: [PATCH 2/2] iio: pressure: driver for Honeywell HSC/SSC series pressure sensors
  2023-11-20 12:35   ` [PATCH " Andy Shevchenko
@ 2023-11-22  6:08     ` Petre Rodan
  2023-11-22 10:45       ` Andy Shevchenko
  2023-11-25 19:13       ` Jonathan Cameron
  0 siblings, 2 replies; 24+ messages in thread
From: Petre Rodan @ 2023-11-22  6:08 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: linux-kernel, linux-iio, Jonathan Cameron, Lars-Peter Clausen,
	Angel Iglesias, Matti Vaittinen, Andreas Klinger, Rob Herring,
	Krzysztof Kozlowski


hello,

first of all, thank you for the code review.
in the interest of brevity I will skip all comments where I simply remove the block, blankline, or fix indentation.

On Mon, Nov 20, 2023 at 02:35:39PM +0200, Andy Shevchenko wrote:
> > +	select HSC030PA_I2C if (I2C)
> > +	select HSC030PA_SPI if (SPI_MASTER)
> 
> Unneeded parentheses

ack

> > +	help
> > +	  Say Y here to build support for the Honeywell HSC and SSC TruStability
> > +      pressure and temperature sensor series.
> > +
> > +	  To compile this driver as a module, choose M here: the module will be
> > +	  called hsc030pa.
> 
> Yeah besides indentation issues the LKP complain about this.

fixed indentation and now it compiles fine with git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
sorry, what is 'LKP' in this context and how do I reproduce?

> > +#include <linux/module.h>
> > +#include <linux/mutex.h>
> > +#include <linux/init.h>
> > +#include <linux/math64.h>
> > +#include <linux/units.h>
> > +#include <linux/mod_devicetable.h>
> > +#include <linux/printk.h>
> 
> Keep them sorted alphabetically.
> Also there are missing at least these ones: array_size.h, types.h.

'#include <linux/array_size.h>' is a weird one.
$ cd /usr/src/linux/drivers
$ grep -r ARRAY_SIZE * | grep '\.c:' |  wc -l
 32396
$ grep -r 'include.*array_size\.h' * | grep -E '\.[ch]:' | wc -l
11
$ grep -r 'include.*array_size\.h' * | grep -E '\.[ch]:' | grep -v '^pinctrl' | wc -l
0

plus on a 6.1 version kernel, `make modules` actually reports that the header can't be found if I include it. can't comprehend that.
so I'll be skipping that particular include.

> > +// pressure range for current chip based on the nomenclature
> > +struct hsc_range_config {
> > +	char name[HSC_RANGE_STR_LEN];	// 5-char string that defines the range - ie "030PA"
> > +	s32 pmin;		// minimal pressure in pascals
> > +	s32 pmax;		// maximum pressure in pascals
> > +};
> 
> Can you utilize linear ranges data types and APIs? (linear_range.h)

not fit for this purpose, sorry.

> > +/*
> > + * the first two bits from the first byte contain a status code
> > + *
> > + * 00 - normal operation, valid data
> > + * 01 - device in hidden factory command mode
> > + * 10 - stale data
> > + * 11 - diagnostic condition
> > + *
> > + * function returns 1 only if both bits are zero
> > + */
> 
> You really need to be consistent with style of multi-line comments.
> And also C++/C variants. Besides that, respect English grammar and
> punctuation.

ok, I changed all comments to /* */.
this particular block was rewritten (the legend is taken from honeywell's i2c-related datasheet).

> > +static bool hsc_measurement_is_valid(struct hsc_data *data)
> > +{
> > +	if (data->buffer[0] & 0xc0)
> > +		return 0;
> > +
> > +	return 1;
> 
> You use bool and return integers.
> 
> Besides, it can be just a oneliner.

rewritten as a one-liner, without GENMASK.

> 	return !(buffer[0] & GENMASK(3, 2));
> 
> (Note, you will need bits.h for this.)
> 
> > +}
> 
...
> > +	ret = chip->valid(data);
> > +	if (!ret)
> > +		return -EAGAIN;
> > +
> > +	data->is_valid = true;
> 
> Can this be like
> 
> 	bool is_valid;
> 
> 	is_valid = chip->valid(...);
> 	if (!is_valid)
> 		return ...
> 
> 
> 	data->is_valid = is_valid;
> 
> 	// Depending on the flow you can even use that field directly

ack

> > +	case IIO_CHAN_INFO_RAW:
> > +		mutex_lock(&data->lock);
> > +		ret = hsc_get_measurement(data);
> > +		mutex_unlock(&data->lock);
> 
> Use guard() operator from cleanup.h.

I'm not familiar with that, for the time being I'll stick to mutex_lock/unlock if you don't mind.

> > +		switch (channel->type) {
> > +		case IIO_PRESSURE:
> > +			*val =
> > +			    ((data->buffer[0] & 0x3f) << 8) + data->buffer[1];
> > +			return IIO_VAL_INT;
> > +		case IIO_TEMP:
> > +			*val =
> > +			    (data->buffer[2] << 3) +
> > +			    ((data->buffer[3] & 0xe0) >> 5);
> 
> Is this some endianess / sign extension? Please convert using proper APIs.

the raw conversion data is spread over 4 bytes and interlaced with other info (see comment above the function).
I'm just cherry-picking the bits I'm interested in, in a way my brain can understand what is going on.

> > +			ret = 0;
> > +			if (!ret)
> 
> lol

I should leave that in for comic relief. missed it after a lot of code changes.

> > +	case IIO_CHAN_INFO_OFFSET:
> > +		switch (channel->type) {
> > +		case IIO_TEMP:
> > +			*val = -50000000;
> > +			*val2 = 97704;
> > +			return IIO_VAL_FRACTIONAL;
> > +		case IIO_PRESSURE:
> > +			*val = data->p_offset;
> > +			*val2 = data->p_offset_nano;
> > +			return IIO_VAL_INT_PLUS_NANO;
> > +		default:
> > +			return -EINVAL;
> > +		}
> > +	}
> 
> > +	return ret;
> 
> Use default with explicit error code.

ack.

> > +static const struct iio_chan_spec hsc_channels[] = {
> > +	{
> > +	 .type = IIO_PRESSURE,
> > +	 .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> > +	 BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_OFFSET)
> > +	 },
> > +	{
> > +	 .type = IIO_TEMP,
> > +	 .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> > +	 BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_OFFSET)
> > +	 },
> 
> Strange indentation of }:s...

I blame `indent -linux --line-length 80` for these and weirdly-spaced pointer declarations.
are you using something else?

> > +int hsc_probe(struct iio_dev *indio_dev, struct device *dev,
> > +	      const char *name, int type)
> > +{
> > +	struct hsc_data *hsc;
> > +	u64 tmp;
> > +	int index;
> > +	int found = 0;
> > +
> > +	hsc = iio_priv(indio_dev);
> > +
> > +	hsc->last_update = jiffies - HZ;
> > +	hsc->chip = &hsc_chip;
> > +
> > +	if (strcasecmp(hsc->range_str, "na") != 0) {
> > +		// chip should be defined in the nomenclature
> > +		for (index = 0; index < ARRAY_SIZE(hsc_range_config); index++) {
> > +			if (strcasecmp
> > +			    (hsc_range_config[index].name,
> > +			     hsc->range_str) == 0) {
> > +				hsc->pmin = hsc_range_config[index].pmin;
> > +				hsc->pmax = hsc_range_config[index].pmax;
> > +				found = 1;
> > +				break;
> > +			}
> > +		}
> 
> Reinventing match_string() / sysfs_match_string() ?

match_string() is case-sensitive and operates on string arrays, so unfit for this purpose.

> > +struct hsc_data {
> > +	void *client;                           // either i2c or spi kernel interface struct for current dev
> > +	const struct hsc_chip_data *chip;
> > +	struct mutex lock;                      // lock protecting chip reads
> > +	int (*xfer)(struct hsc_data *data);    // function that implements the chip reads
> > +	bool is_valid;                          // false if last transfer has failed
> > +	unsigned long last_update;              // time of last successful conversion
> > +	u8 buffer[HSC_REG_MEASUREMENT_RD_SIZE]; // raw conversion data
> > +	char range_str[HSC_RANGE_STR_LEN];	// range as defined by the chip nomenclature - ie "030PA" or "NA"
> > +	s32 pmin;                               // min pressure limit
> > +	s32 pmax;                               // max pressure limit
> > +	u32 outmin;                             // minimum raw pressure in counts (based on transfer function)
> > +	u32 outmax;                             // maximum raw pressure in counts (based on transfer function)
> > +	u32 function;                           // transfer function
> > +	s64 p_scale;                            // pressure scale
> > +	s32 p_scale_nano;                       // pressure scale, decimal places
> > +	s64 p_offset;                           // pressure offset
> > +	s32 p_offset_nano;                      // pressure offset, decimal places
> > +};
> > +
> > +struct hsc_chip_data {
> > +	bool (*valid)(struct hsc_data *data);  // function that checks the two status bits
> > +	const struct iio_chan_spec *channels;   // channel specifications
> > +	u8 num_channels;                        // pressure and temperature channels
> > +};
> 
> Convert comments to kernel-doc format.

ack. switched to kernel-doc format in multiple places.

> > +enum hsc_func_id {
> > +	HSC_FUNCTION_A,
> > +	HSC_FUNCTION_B,
> > +	HSC_FUNCTION_C,
> > +	HSC_FUNCTION_F
> 
> Leave trailing comma. It make code slightly better to maintain.

ack

> > +static int hsc_i2c_xfer(struct hsc_data *data)
> > +{
> > +	struct i2c_client *client = data->client;
> > +	struct i2c_msg msg;
> > +	int ret;
> > +
> > +	msg.addr = client->addr;
> > +	msg.flags = client->flags | I2C_M_RD;
> > +	msg.len = HSC_REG_MEASUREMENT_RD_SIZE;
> > +	msg.buf = (char *)&data->buffer;
> > +
> > +	ret = i2c_transfer(client->adapter, &msg, 1);
> > +
> > +	return (ret == 2) ? 0 : ret;
> > +}
> 
> Can you use regmap I2C?

the communication is one-way as in the sensors do not expect anything except 4 bytes-worth of clock signals per 'packet' for both the i2c and spi versions.
regmap is suited to sensors with an actual memory map.

> > +static int hsc_i2c_probe(struct i2c_client *client,
> > +			 const struct i2c_device_id *id)
> 
> No use of this function prototype, we have a new one.

oops, I was hoping my 6.1.38 kernel is using the same API as 6.7.0
fixed.

> > +	indio_dev = devm_iio_device_alloc(dev, sizeof(*hsc));
> > +	if (!indio_dev) {
> 
> > +		dev_err(&client->dev, "Failed to allocate device\n");
> > +		return -ENOMEM;
> 
> First of all, use
> 
> 		return dev_err_probe();
> 
> Second, since it's ENOMEM, we do not issue an errors like this, error
> code is already enough.

ack

> 
> > +	}
> > +
> > +	hsc = iio_priv(indio_dev);
> 
> > +	if (i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
> > +		hsc->xfer = hsc_i2c_xfer;
> > +	else
> 
> Redundant 'else', see below.
> 
> > +		return -EOPNOTSUPP;
> 
> Use traditional pattern, i.e. checking for errors first:
> 
> 	if (...)
> 		return ...

ack

> > +	ret = devm_regulator_get_enable_optional(dev, "vdd");
> > +	if (ret == -EPROBE_DEFER)
> > +		return -EPROBE_DEFER;
> 
> Oh, boy, this should check for ENODEV or so, yeah, regulator APIs a bit
> interesting.

since I'm unable to test this I'd rather remove the block altogether.
if I go the ENODEV route my module will never load since I can't see any vdd-supply support on my devboard.

> > +	if (!dev_fwnode(dev))
> > +		return -EOPNOTSUPP;
> 
> Why do you need this?
> And why this error code?

it's intentional.
this module has a hard requirement on the correct parameters (transfer function and pressure range) being provided in the devicetree.
without those I don't want to provide any measurements since there can't be a default transfer function and pressure range for a generic driver that supports an infinite combination of those.

echo hsc030pa 0x28 > /sys/bus/i2c/devices/i2c-0/new_device
I want iio_info to detect 0 devices.

> > +	memcpy(hsc->range_str, range_nom, HSC_RANGE_STR_LEN - 1);
> > +	hsc->range_str[HSC_RANGE_STR_LEN - 1] = 0;
> 
> Oh, why do you need this all and can use the property value directly?
> (Besides the fact the interesting operations are being used for strings.)

using directly and moved to main probe() file.

> > +MODULE_DEVICE_TABLE(of, hsc_i2c_match);
> > +
> > +static const struct i2c_device_id hsc_i2c_id[] = {
> > +	{"hsc", HSC},
> > +	{"ssc", SSC},
> 
> Both ID tables should use pointers in driver data and respective API to get
> that.

re-written based on bindings thread.

> > +	spi_set_drvdata(spi, indio_dev);
> 
> How is this being used?

removed.

> > +	spi->mode = SPI_MODE_0;
> > +	spi->max_speed_hz = min(spi->max_speed_hz, 800000U);
> > +	spi->bits_per_word = 8;
> > +	ret = spi_setup(spi);
> > +	if (ret < 0)
> > +		return ret;
> 
> Why the firmware can't provide the correct information to begin with?

moved 800kHz max requirement to the bindings file.

> > +	ret = device_property_read_u32(dev,
> > +				       "honeywell,transfer-function",
> > +				       &hsc->function);
..
> > +		if (ret)
> > +			return dev_err_probe(dev, ret,
> > +					     "honeywell,pmax-pascal could not be read\n");
> > +	}
> 
> Ditto. Why is this duplication?

you're right, moved to main probe()

> -- 
> With Best Regards,
> Andy Shevchenko

patches are ready, but awaiting any aditional feedback to this message.

thanks again,
peter

-- 
petre rodan

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

* Re: [PATCH 2/2] iio: pressure: driver for Honeywell HSC/SSC series pressure sensors
  2023-11-22  6:08     ` Petre Rodan
@ 2023-11-22 10:45       ` Andy Shevchenko
  2023-11-25 19:15         ` Jonathan Cameron
  2023-11-25 19:13       ` Jonathan Cameron
  1 sibling, 1 reply; 24+ messages in thread
From: Andy Shevchenko @ 2023-11-22 10:45 UTC (permalink / raw)
  To: Petre Rodan
  Cc: linux-kernel, linux-iio, Jonathan Cameron, Lars-Peter Clausen,
	Angel Iglesias, Matti Vaittinen, Andreas Klinger, Rob Herring,
	Krzysztof Kozlowski

On Wed, Nov 22, 2023 at 08:08:27AM +0200, Petre Rodan wrote:
> On Mon, Nov 20, 2023 at 02:35:39PM +0200, Andy Shevchenko wrote:

...

> sorry, what is 'LKP' in this context and how do I reproduce?

It's an acronym for CI system run by Intel. You should have had an email in
your mailbox with complains. It also duplicates them to a mailing list which
address I don't know by heart.

...

> > Also there are missing at least these ones: array_size.h, types.h.
> 
> '#include <linux/array_size.h>' is a weird one.

Why?

> $ cd /usr/src/linux/drivers
> $ grep -r ARRAY_SIZE * | grep '\.c:' |  wc -l
>  32396
> $ grep -r 'include.*array_size\.h' * | grep -E '\.[ch]:' | wc -l
> 11
> $ grep -r 'include.*array_size\.h' * | grep -E '\.[ch]:' | grep -v '^pinctrl' | wc -l
> 0

Hint, use `git grep ...` which much, much faster against the Git indexed data.

> plus on a 6.1 version kernel, `make modules` actually reports that the header
> can't be found if I include it. can't comprehend that. so I'll be skipping
> that particular include.

No, the new code is always should be submitted against latest release cycle,
v6.7-rcX as of today. There is the header. Please, use it.

...

> > Can you utilize linear ranges data types and APIs? (linear_range.h)
> 
> not fit for this purpose, sorry.

NP.

...

> > > +	if (data->buffer[0] & 0xc0)
> > > +		return 0;
> > > +
> > > +	return 1;
> > 
> > You use bool and return integers.
> > 
> > Besides, it can be just a oneliner.
> 
> rewritten as a one-liner, without GENMASK.
> 
> > 	return !(buffer[0] & GENMASK(3, 2));
> > 
> > (Note, you will need bits.h for this.)
> > 
> > > +}

Why no GENMASK() ? What the meaning of the 0xc0?
Ideally it should be

#define ...meaningful name...  GENMASK()

...

> > > +		mutex_lock(&data->lock);
> > > +		ret = hsc_get_measurement(data);
> > > +		mutex_unlock(&data->lock);
> > 
> > Use guard() operator from cleanup.h.
> 
> I'm not familiar with that, for the time being I'll stick to
> mutex_lock/unlock if you don't mind.

I do mind. RAII is a method to make code more robust against forgotten
unlock/free calls.

...

> > > +		case IIO_PRESSURE:
> > > +			*val =
> > > +			    ((data->buffer[0] & 0x3f) << 8) + data->buffer[1];
> > > +			return IIO_VAL_INT;
> > > +		case IIO_TEMP:
> > > +			*val =
> > > +			    (data->buffer[2] << 3) +
> > > +			    ((data->buffer[3] & 0xe0) >> 5);
> > 
> > Is this some endianess / sign extension? Please convert using proper APIs.
> 
> the raw conversion data is spread over 4 bytes and interlaced with other info
> (see comment above the function).  I'm just cherry-picking the bits I'm
> interested in, in a way my brain can understand what is going on.

So, perhaps you need to use get_unaligned_.e32() and then FIELD_*() from
bitfield.h. This will be much better in terms of understanding the semantics
of these magic bit shifts and masks.

...

> > > +			ret = 0;
> > > +			if (!ret)
> > 
> > lol
> 
> I should leave that in for comic relief. missed it after a lot of code
> changes.

I understand, that's why no shame on you, just fun code to see :-)

...

> > Strange indentation of }:s...
> 
> I blame `indent -linux --line-length 80` for these and weirdly-spaced pointer
> declarations.  are you using something else?

Some maintainers suggest to use clang-format. I find it weird in some corner
cases. So, I would suggest to use it and reread the code and fix some
strangenesses.

...

> > > +	if (strcasecmp(hsc->range_str, "na") != 0) {
> > > +		// chip should be defined in the nomenclature
> > > +		for (index = 0; index < ARRAY_SIZE(hsc_range_config); index++) {
> > > +			if (strcasecmp
> > > +			    (hsc_range_config[index].name,
> > > +			     hsc->range_str) == 0) {
> > > +				hsc->pmin = hsc_range_config[index].pmin;
> > > +				hsc->pmax = hsc_range_config[index].pmax;
> > > +				found = 1;
> > > +				break;
> > > +			}
> > > +		}
> > 
> > Reinventing match_string() / sysfs_match_string() ?
> 
> match_string() is case-sensitive and operates on string arrays, so unfit for
> this purpose.

Let's put it this way: Why do you care of the relaxed case?
I.o.w. why can we be slightly stricter?

...

> > Can you use regmap I2C?
> 
> the communication is one-way as in the sensors do not expect anything except
> 4 bytes-worth of clock signals per 'packet' for both the i2c and spi
> versions.  regmap is suited to sensors with an actual memory map.

If not yet, worse to add in the comment area of the patch
(after the cutter '---' line).

...

> > No use of this function prototype, we have a new one.
> 
> oops, I was hoping my 6.1.38 kernel is using the same API as 6.7.0
> fixed.

Same way with a (new) header :-)

...

> > > +	ret = devm_regulator_get_enable_optional(dev, "vdd");
> > > +	if (ret == -EPROBE_DEFER)
> > > +		return -EPROBE_DEFER;
> > 
> > Oh, boy, this should check for ENODEV or so, yeah, regulator APIs a bit
> > interesting.
> 
> since I'm unable to test this I'd rather remove the block altogether.
> if I go the ENODEV route my module will never load since I can't see any
> vdd-supply support on my devboard.

No, what I meant is to have something like

	if (ret) {
		if (ret != -ENODEV)
			return ret;
		...regulator is not present...
	}

This is how it's being used in dozens of places in the kernel. Just utilize
`git grep ...` which should be a top-10 tool for the Linux kernel developer.

Q: ...
A: Try `git grep ...` to find your answer in the existing code.

...

> > > +	if (!dev_fwnode(dev))
> > > +		return -EOPNOTSUPP;
> > 
> > Why do you need this?
> > And why this error code?
> 
> it's intentional.
> this module has a hard requirement on the correct parameters (transfer
> function and pressure range) being provided in the devicetree.  without those
> I don't want to provide any measurements since there can't be a default
> transfer function and pressure range for a generic driver that supports an
> infinite combination of those.
> 
> echo hsc030pa 0x28 > /sys/bus/i2c/devices/i2c-0/new_device
> I want iio_info to detect 0 devices.

So, fine, but the very first mandatory property check will fail as it has
the very same check inside. So, why do you need a double check?

-- 
With Best Regards,
Andy Shevchenko



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

* Re: [PATCH 2/2] iio: pressure: driver for Honeywell HSC/SSC series pressure sensors
  2023-11-22  6:08     ` Petre Rodan
  2023-11-22 10:45       ` Andy Shevchenko
@ 2023-11-25 19:13       ` Jonathan Cameron
  1 sibling, 0 replies; 24+ messages in thread
From: Jonathan Cameron @ 2023-11-25 19:13 UTC (permalink / raw)
  To: Petre Rodan
  Cc: Andy Shevchenko, linux-kernel, linux-iio, Lars-Peter Clausen,
	Angel Iglesias, Matti Vaittinen, Andreas Klinger, Rob Herring,
	Krzysztof Kozlowski

On Wed, 22 Nov 2023 08:08:27 +0200
Petre Rodan <petre.rodan@subdimension.ro> wrote:

> hello,
> 
> first of all, thank you for the code review.
> in the interest of brevity I will skip all comments where I simply remove the block, blankline, or fix indentation.
> 
> On Mon, Nov 20, 2023 at 02:35:39PM +0200, Andy Shevchenko wrote:
> > > +	select HSC030PA_I2C if (I2C)
> > > +	select HSC030PA_SPI if (SPI_MASTER)  
> > 
> > Unneeded parentheses  
> 
> ack
Where you agree, just crop it out. Saves on scrolling!

> > > +	case IIO_CHAN_INFO_RAW:
> > > +		mutex_lock(&data->lock);
> > > +		ret = hsc_get_measurement(data);
> > > +		mutex_unlock(&data->lock);  
> > 
> > Use guard() operator from cleanup.h.  
> 
> I'm not familiar with that, for the time being I'll stick to mutex_lock/unlock if you don't mind.
> 

It's simple and worth taking a look for new drivers as it makes some error paths much much simpler.
I'm sitting on a big set that applies it to quite few IIO drivers.



> > > +	ret = devm_regulator_get_enable_optional(dev, "vdd");
> > > +	if (ret == -EPROBE_DEFER)
> > > +		return -EPROBE_DEFER;  
> > 
> > Oh, boy, this should check for ENODEV or so, yeah, regulator APIs a bit
> > interesting.  
> 
> since I'm unable to test this I'd rather remove the block altogether.
> if I go the ENODEV route my module will never load since I can't see any vdd-supply support on my devboard.
Problem here is why do you think that regulator is optional? Does your device
work with out power?  What is optional is whether the regulator is fixed and
on and hence doesn't need to be in DT or whether it is specified there.
That's unconnected to the enabling in driver.

The call you have here is for when the power supply really is optional.
That is the driver does something different if nothing is supplied on the pin.
Typically this is used when we have option of either an internal reference voltage
or supplying an external one. The absence on an external one means we fallback
to only enabling the internal one.


Jonathan

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

* Re: [PATCH 2/2] iio: pressure: driver for Honeywell HSC/SSC series pressure sensors
  2023-11-22 10:45       ` Andy Shevchenko
@ 2023-11-25 19:15         ` Jonathan Cameron
  0 siblings, 0 replies; 24+ messages in thread
From: Jonathan Cameron @ 2023-11-25 19:15 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Petre Rodan, linux-kernel, linux-iio, Lars-Peter Clausen,
	Angel Iglesias, Matti Vaittinen, Andreas Klinger, Rob Herring,
	Krzysztof Kozlowski

> 
> > > > +	ret = devm_regulator_get_enable_optional(dev, "vdd");
> > > > +	if (ret == -EPROBE_DEFER)
> > > > +		return -EPROBE_DEFER;  
> > > 
> > > Oh, boy, this should check for ENODEV or so, yeah, regulator APIs a bit
> > > interesting.  
> > 
> > since I'm unable to test this I'd rather remove the block altogether.
> > if I go the ENODEV route my module will never load since I can't see any
> > vdd-supply support on my devboard.  
> 
> No, what I meant is to have something like
> 
> 	if (ret) {
> 		if (ret != -ENODEV)
> 			return ret;
> 		...regulator is not present...
> 	}
> 
> This is how it's being used in dozens of places in the kernel. Just utilize
> `git grep ...` which should be a top-10 tool for the Linux kernel developer.

As per my very late reply to previous email. Nope. This regulator is never
not present. It's just a question of whether the firmware tells us what
it is, or it is supplied with a stub regulator.


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

* Re: [PATCH v2 1/2] dt-bindings: iio: pressure: add honeywell,hsc030
  2023-11-17 19:22 ` [PATCH v2 " Petre Rodan
                     ` (2 preceding siblings ...)
  2023-11-20 10:21   ` Krzysztof Kozlowski
@ 2023-11-25 19:19   ` Jonathan Cameron
  3 siblings, 0 replies; 24+ messages in thread
From: Jonathan Cameron @ 2023-11-25 19:19 UTC (permalink / raw)
  To: Petre Rodan
  Cc: linux-kernel, linux-iio, devicetree, Conor Dooley,
	Lars-Peter Clausen, Rob Herring, Krzysztof Kozlowski,
	linux-kernel-mentees

On Fri, 17 Nov 2023 21:22:57 +0200
Petre Rodan <petre.rodan@subdimension.ro> wrote:

> Adds binding for digital Honeywell TruStability HSC and SSC series pressure 
> and temperature sensors.
> 
> Datasheet:
>  [HSC] https://prod-edam.honeywell.com/content/dam/honeywell-edam/sps/siot/en-us/products/sensors/pressure-sensors/board-mount-pressure-sensors/trustability-hsc-series/documents/sps-siot-trustability-hsc-series-high-accuracy-board-mount-pressure-sensors-50099148-a-en-ciid-151133.pdf
>  [SSC] https://prod-edam.honeywell.com/content/dam/honeywell-edam/sps/siot/en-us/products/sensors/pressure-sensors/board-mount-pressure-sensors/trustability-ssc-series/documents/sps-siot-trustability-ssc-series-standard-accuracy-board-mount-pressure-sensors-50099533-a-en-ciid-151134.pdf
> 
> Signed-off-by: Petre Rodan <petre.rodan@subdimension.ro>

Hi Petre,

Please resend whole series when you do a new version.  I know that some
areas of the kernel do minor tweaks by reply to an earlier version but
in IIO we rely heavily on patchwork for tracking and it makes it very
hard to find the email.

Also, don't make that a reply to earlier version. The nesting of
remotely complex threads makes that impossible track once we have
a few versions posted.

Jonathan

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

* Re: [PATCH v2 1/2] dt-bindings: iio: pressure: add honeywell,hsc030
  2023-11-20 18:25             ` Petre Rodan
@ 2023-11-25 19:21               ` Jonathan Cameron
  0 siblings, 0 replies; 24+ messages in thread
From: Jonathan Cameron @ 2023-11-25 19:21 UTC (permalink / raw)
  To: Petre Rodan
  Cc: Jonathan Cameron, Krzysztof Kozlowski, linux-kernel, linux-iio,
	devicetree, Conor Dooley, Lars-Peter Clausen, Rob Herring,
	Krzysztof Kozlowski, linux-kernel-mentees

On Mon, 20 Nov 2023 20:25:08 +0200
Petre Rodan <petre.rodan@subdimension.ro> wrote:

> Hello!
> 
> On Mon, Nov 20, 2023 at 05:39:29PM +0000, Jonathan Cameron wrote:
> > On Mon, 20 Nov 2023 16:40:51 +0200
> > Petre Rodan <petre.rodan@subdimension.ro> wrote:
> >   
> > > Hello!
> > > 
> > > On Mon, Nov 20, 2023 at 03:04:07PM +0100, Krzysztof Kozlowski wrote:  
> > > > On 20/11/2023 14:42, Petre Rodan wrote:
> > > >     
> > > > >>> +properties:
> > > > >>> +  compatible:
> > > > >>> +    enum:
> > > > >>> +      - honeywell,hsc    
> > > > >>
> > > > >> Way too generic    
> > > > > 
> > > > > I'm new to this, please excuse my ignorance.
> > > > > my driver covers all Honeywell pressure sensors under the "TruStability board mount HSC/SSC" moniker.    
> > > > 
> > > > We talk here about bindings, not driver. For the driver you can use
> > > > whatever name is approved by reviewers of your driver.
> > > >     
> > > > > that is why my intention was to provide a rather generic name for the driver itself.
> > > > > are you afraid that they will come up with a different device that they will call "hsc" in the future?
> > > > > in this case honeywell,trustability-hsc would be fine?
> > > > > 
> > > > > as I see you prefer to target a particular chip, but I am a bit afraid that the end-user will be confused by needing to set up something like
> > > > > 
> > > > > pressure@28 {
> > > > > 	compatible = "honeywell,hsc030pa";    
> > > > 
> > > > The compatible should be specific, thus for example match exact model
> > > > number.    
> > > 
> > > there are an infinite number of combinations of 4 transfer functions and 118 ranges + one custom range, so providing an array with all specific chips that could end up as compatible is out of the question.
> > > I was aiming at providing a generic name for the binding and get the transfer function and the pressure range as required parameters.
> > >   
> > > > If you can guarantee that all devices from given family are the same in
> > > > respect of programming model and hardware requirements (e.g. supplies),
> > > > then you could go with family name. However such guarantees are rarely
> > > > given.    
> > > 
> > > I see your point.
> > >   
> > > > Therefore for mprls0025pa I agreed for using one specific model
> > > > for entire family.
> > > > 
> > > > https://lore.kernel.org/all/d577bc44-780f-f25d-29c6-ed1d353b540c@linaro.org/
> > > > 
> > > >     
> > > > > 	reg = <0x28>;
> > > > > 	honeywell,transfer-function = <0>;
> > > > > 	honeywell,pressure-range = "250MD";
> > > > > };
> > > > > 
> > > > > ie. specifying "hsc030pa" as driver while his chip is not in the 030PA range, but 250MD.
> > > > > 
> > > > > so do you prefer
> > > > >  honeywell,trustability-hsc  OR
> > > > >  honeywell,hsc030pa    
> > > > 
> > > > I think the latter, just like we did for mprls0025pa. How many devices
> > > > do you have there?    
> > > 
> > > both hsc and ssc have 118 ranges, 4 transfer functions and both can be requested from the manufacturer with custom measurement ranges.
> > > 
> > > ok,I will rename hsc->hsc030pa in the code as you requested.  
> > 
> > Where does pa come from?   
> 
> honeywell,hsc030pa was provided as an equivalent to honeywell,mprls0025pa (which is already in the repo).
> 
> '030PA' and '0025PA' define the pressure range (0-30, 0-25), the unit of measure (Psi) and the measurement type (Absolute) for a particular chip in the honeywell catalog. (please ignore the psi part, we convert everything to pascals).
> but both my driver and Andreas Klinger's mprls0025pa actually provide a generic abstraction layer for entire series of sensors.

ah ok. That's fine then - searching the datasheet I found didn't include that particular
string, so I was rather confused.

I'm fine with specific now you've explained where it came from!

Jonathan

> 
> > If we are going generic, feels like trustability-ssc etc are more representative
> > and matches the datasheet cover page.  
> 
> Krzysztof voted for non-generic, honeywell,mprls0025pa is already set up non-generic, my intent was to go generic.
> 
> I'll rewrite the code to whatever you guys feel is best.
> 
> peter
> 
> 


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

end of thread, other threads:[~2023-11-25 19:21 UTC | newest]

Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-11-17 16:42 [PATCH 1/2] dt-bindings: iio: pressure: add honeywell,hsc030 Petre Rodan
2023-11-17 16:42 ` [PATCH 2/2] iio: pressure: driver for Honeywell HSC/SSC series pressure sensors Petre Rodan
2023-11-18  5:21   ` [PATCH v2 " kernel test robot
2023-11-20 12:35   ` [PATCH " Andy Shevchenko
2023-11-22  6:08     ` Petre Rodan
2023-11-22 10:45       ` Andy Shevchenko
2023-11-25 19:15         ` Jonathan Cameron
2023-11-25 19:13       ` Jonathan Cameron
2023-11-17 17:12 ` [PATCH 1/2] dt-bindings: iio: pressure: add honeywell,hsc030 Rob Herring
2023-11-17 19:22 ` [PATCH v2 " Petre Rodan
2023-11-17 20:13   ` Rob Herring
2023-11-19 13:49   ` Rob Herring
2023-11-19 20:14     ` Petre Rodan
2023-11-20 10:15       ` Krzysztof Kozlowski
2023-11-20 17:19       ` Rob Herring
2023-11-20 18:09         ` Petre Rodan
2023-11-20 10:21   ` Krzysztof Kozlowski
2023-11-20 13:42     ` Petre Rodan
2023-11-20 14:04       ` Krzysztof Kozlowski
2023-11-20 14:40         ` Petre Rodan
2023-11-20 17:39           ` Jonathan Cameron
2023-11-20 18:25             ` Petre Rodan
2023-11-25 19:21               ` Jonathan Cameron
2023-11-25 19:19   ` Jonathan Cameron

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.