[PATCH 0/6] iio: pressure: hsc030pa new features

[PATCH 0/6] iio: pressure: hsc030pa new features

[Petre Rodan]

This set of patches covers the following:

 - small cleanup
 - mandatory 2ms delay between readings
 - support for triggered buffer readings
 - support for devices that have "sleep mode" factory option active

Petre Rodan (6):
  dt-bindings: iio: pressure: honeywell,hsc030pa.yaml add spi props
  dt-bindings: iio: pressure: honeywell,hsc030pa.yaml add sleep-mode
  iio: pressure: hsc030pa cleanup
  iio: pressure: hsc030pa add mandatory delay
  iio: pressure: hsc030pa add triggered buffer
  iio: pressure: hsc030pa add sleep mode

 .../iio/pressure/honeywell,hsc030pa.yaml      | 13 +++++
 drivers/iio/pressure/hsc030pa.c               | 48 ++++++++++++++++++-
 drivers/iio/pressure/hsc030pa.h               |  9 +++-
 drivers/iio/pressure/hsc030pa_i2c.c           | 28 ++++++++++-
 drivers/iio/pressure/hsc030pa_spi.c           | 40 ++++++++++++++--
 5 files changed, 131 insertions(+), 7 deletions(-)

--
2.41.0

[PATCH 1/6] dt-bindings: iio: pressure: honeywell,hsc030pa.yaml add spi props

[Petre Rodan]

Add spi-peripheral-props.yaml requirement

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

diff --git a/Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml b/Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml
index 65a24ed67b3c..89977b9f01cf 100644
--- a/Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml
+++ b/Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml
@@ -99,6 +99,9 @@ required:
   - honeywell,transfer-function
   - honeywell,pressure-triplet

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

 dependentSchemas:
--
2.41.0

[PATCH 2/6] dt-bindings: iio: pressure: honeywell,hsc030pa.yaml add sleep-mode

[Petre Rodan]

Add sleep-mode property present in some custom chips.

This flag activates a special wakeup sequence prior to conversion.

Signed-off-by: Petre Rodan <petre.rodan@subdimension.ro>
---
 .../bindings/iio/pressure/honeywell,hsc030pa.yaml      | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml b/Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml
index 89977b9f01cf..350da1d6991b 100644
--- a/Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml
+++ b/Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml
@@ -86,6 +86,15 @@ properties:
       Maximum pressure value the sensor can measure in pascal.
       To be specified only if honeywell,pressure-triplet is set to "NA".

+  honeywell,sleep-mode:
+    description: |
+      'Sleep Mode' is a special factory set mode of the chip that allows the
+      sensor to power down between measurements. It is implemented only on
+      special request, and it is an attribute not present in the HSC/SSC series
+      nomenclature.
+      Set in order to enable the special wakeup sequence prior to conversion.
+    $ref: /schemas/types.yaml#/definitions/flag
+
   vdd-supply:
     description:
       Provide VDD power to the sensor (either 3.3V or 5V depending on the chip)
@@ -140,6 +149,7 @@ examples:
             honeywell,pressure-triplet = "NA";
             honeywell,pmin-pascal = <0>;
             honeywell,pmax-pascal = <200000>;
+            //honeywell,sleep-mode;
         };
     };
 ...
--
2.41.0

[PATCH 3/6] iio: pressure: hsc030pa cleanup

[Petre Rodan]

Use signed type to variable holding the result given by div_s64().

Add includes based on prior reviews from Andy.

Provide bus-specific technical datasheet in the _i2c.c _spi.c headers
instead of the generic one.

Signed-off-by: Petre Rodan <petre.rodan@subdimension.ro>
---
 drivers/iio/pressure/hsc030pa.c     | 2 +-
 drivers/iio/pressure/hsc030pa.h     | 2 ++
 drivers/iio/pressure/hsc030pa_i2c.c | 6 ++++--
 drivers/iio/pressure/hsc030pa_spi.c | 5 ++++-
 4 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/pressure/hsc030pa.c b/drivers/iio/pressure/hsc030pa.c
index d6a51f0c335f..7e3f74d53b47 100644
--- a/drivers/iio/pressure/hsc030pa.c
+++ b/drivers/iio/pressure/hsc030pa.c
@@ -406,7 +406,7 @@ int hsc_common_probe(struct device *dev, hsc_recv_fn recv)
 	struct hsc_data *hsc;
 	struct iio_dev *indio_dev;
 	const char *triplet;
-	u64 tmp;
+	s64 tmp;
 	int ret;

 	indio_dev = devm_iio_device_alloc(dev, sizeof(*hsc));
diff --git a/drivers/iio/pressure/hsc030pa.h b/drivers/iio/pressure/hsc030pa.h
index d20420dba4f6..f1079a70799f 100644
--- a/drivers/iio/pressure/hsc030pa.h
+++ b/drivers/iio/pressure/hsc030pa.h
@@ -10,6 +10,8 @@

 #include <linux/types.h>

+#include <linux/iio/iio.h>
+
 #define HSC_REG_MEASUREMENT_RD_SIZE 4

 struct device;
diff --git a/drivers/iio/pressure/hsc030pa_i2c.c b/drivers/iio/pressure/hsc030pa_i2c.c
index e2b524b36417..b5810bafef40 100644
--- a/drivers/iio/pressure/hsc030pa_i2c.c
+++ b/drivers/iio/pressure/hsc030pa_i2c.c
@@ -4,14 +4,16 @@
  *
  * 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 [hsc]
- * 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 [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
+ * Datasheet: https://prod-edam.honeywell.com/content/dam/honeywell-edam/sps/siot/en-us/products/sensors/pressure-sensors/common/documents/sps-siot-sleep-mode-technical-note-008286-1-en-ciid-155793.pdf
  */

+#include <linux/device.h>
 #include <linux/errno.h>
 #include <linux/i2c.h>
 #include <linux/mod_devicetable.h>
 #include <linux/module.h>
+#include <linux/types.h>

 #include <linux/iio/iio.h>

diff --git a/drivers/iio/pressure/hsc030pa_spi.c b/drivers/iio/pressure/hsc030pa_spi.c
index a719bade8326..8d3441f1dcf9 100644
--- a/drivers/iio/pressure/hsc030pa_spi.c
+++ b/drivers/iio/pressure/hsc030pa_spi.c
@@ -4,13 +4,16 @@
  *
  * 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
+ * Datasheet: https://prod-edam.honeywell.com/content/dam/honeywell-edam/sps/siot/en-us/products/sensors/pressure-sensors/common/documents/sps-siot-spi-comms-digital-ouptu-pressure-sensors-tn-008202-3-en-ciid-45843.pdf
+ * Datasheet: https://prod-edam.honeywell.com/content/dam/honeywell-edam/sps/siot/en-us/products/sensors/pressure-sensors/common/documents/sps-siot-sleep-mode-technical-note-008286-1-en-ciid-155793.pdf
  */

+#include <linux/device.h>
 #include <linux/mod_devicetable.h>
 #include <linux/module.h>
 #include <linux/spi/spi.h>
 #include <linux/stddef.h>
+#include <linux/types.h>

 #include <linux/iio/iio.h>

--
2.41.0

[PATCH 4/6] iio: pressure: hsc030pa add mandatory delay

[Petre Rodan]

Add a mandatory 2ms delay between consecutive chip reads.

I found a new Technical Note pdf that specifies that the measurement cycle
in these chips takes around 1.26ms. By adding this 2ms delay we make sure
that we never get stale measurements.

This feature is also needed by the "iio: pressure: hsc030pa add sleep mode"
patch below.

For more details, please see "Figure 1" in the pdf below:

https://prod-edam.honeywell.com/content/dam/honeywell-edam/sps/siot/en-us/products/sensors/pressure-sensors/common/documents/sps-siot-sleep-mode-technical-note-008286-1-en-ciid-155793.pdf

Signed-off-by: Petre Rodan <petre.rodan@subdimension.ro>
---
 drivers/iio/pressure/hsc030pa.h     | 1 +
 drivers/iio/pressure/hsc030pa_i2c.c | 3 +++
 drivers/iio/pressure/hsc030pa_spi.c | 3 +++
 3 files changed, 7 insertions(+)

diff --git a/drivers/iio/pressure/hsc030pa.h b/drivers/iio/pressure/hsc030pa.h
index f1079a70799f..56dc8e88194b 100644
--- a/drivers/iio/pressure/hsc030pa.h
+++ b/drivers/iio/pressure/hsc030pa.h
@@ -13,6 +13,7 @@
 #include <linux/iio/iio.h>

 #define HSC_REG_MEASUREMENT_RD_SIZE 4
+#define HSC_RESP_TIME_MS            2

 struct device;

diff --git a/drivers/iio/pressure/hsc030pa_i2c.c b/drivers/iio/pressure/hsc030pa_i2c.c
index b5810bafef40..b3fd230e71da 100644
--- a/drivers/iio/pressure/hsc030pa_i2c.c
+++ b/drivers/iio/pressure/hsc030pa_i2c.c
@@ -8,6 +8,7 @@
  * Datasheet: https://prod-edam.honeywell.com/content/dam/honeywell-edam/sps/siot/en-us/products/sensors/pressure-sensors/common/documents/sps-siot-sleep-mode-technical-note-008286-1-en-ciid-155793.pdf
  */

+#include <linux/delay.h>
 #include <linux/device.h>
 #include <linux/errno.h>
 #include <linux/i2c.h>
@@ -25,6 +26,8 @@ static int hsc_i2c_recv(struct hsc_data *data)
 	struct i2c_msg msg;
 	int ret;

+	msleep_interruptible(HSC_RESP_TIME_MS);
+
 	msg.addr = client->addr;
 	msg.flags = client->flags | I2C_M_RD;
 	msg.len = HSC_REG_MEASUREMENT_RD_SIZE;
diff --git a/drivers/iio/pressure/hsc030pa_spi.c b/drivers/iio/pressure/hsc030pa_spi.c
index 8d3441f1dcf9..737197eddff0 100644
--- a/drivers/iio/pressure/hsc030pa_spi.c
+++ b/drivers/iio/pressure/hsc030pa_spi.c
@@ -8,6 +8,7 @@
  * Datasheet: https://prod-edam.honeywell.com/content/dam/honeywell-edam/sps/siot/en-us/products/sensors/pressure-sensors/common/documents/sps-siot-sleep-mode-technical-note-008286-1-en-ciid-155793.pdf
  */

+#include <linux/delay.h>
 #include <linux/device.h>
 #include <linux/mod_devicetable.h>
 #include <linux/module.h>
@@ -28,6 +29,8 @@ static int hsc_spi_recv(struct hsc_data *data)
 		.len = HSC_REG_MEASUREMENT_RD_SIZE,
 	};

+	msleep_interruptible(HSC_RESP_TIME_MS);
+
 	return spi_sync_transfer(spi, &xfer, 1);
 }

--
2.41.0

[PATCH 5/6] iio: pressure: hsc030pa add triggered buffer

[Petre Rodan]

Add triggered buffer feature.

Signed-off-by: Petre Rodan <petre.rodan@subdimension.ro>
---
 drivers/iio/pressure/hsc030pa.c | 42 +++++++++++++++++++++++++++++++++
 drivers/iio/pressure/hsc030pa.h |  2 +-
 2 files changed, 43 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/pressure/hsc030pa.c b/drivers/iio/pressure/hsc030pa.c
index 7e3f74d53b47..3faa0fd42201 100644
--- a/drivers/iio/pressure/hsc030pa.c
+++ b/drivers/iio/pressure/hsc030pa.c
@@ -22,8 +22,11 @@
 #include <linux/types.h>
 #include <linux/units.h>

+#include <linux/iio/buffer.h>
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/iio/triggered_buffer.h>

 #include <asm/unaligned.h>

@@ -297,6 +300,23 @@ static int hsc_get_measurement(struct hsc_data *data)
 	return 0;
 }

+static irqreturn_t hsc_trigger_handler(int irq, void *private)
+{
+	struct iio_poll_func *pf = private;
+	struct iio_dev *indio_dev = pf->indio_dev;
+	struct hsc_data *data = iio_priv(indio_dev);
+	int ret;
+
+	ret = hsc_get_measurement(data);
+	if (!ret) {
+		iio_push_to_buffers_with_timestamp(indio_dev, &data->buffer,
+						   iio_get_time_ns(indio_dev));
+	}
+	iio_trigger_notify_done(indio_dev->trig);
+
+	return IRQ_HANDLED;
+}
+
 /*
  * IIO ABI expects
  * value = (conv + offset) * scale
@@ -382,13 +402,30 @@ static const struct iio_chan_spec hsc_channels[] = {
 		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
 				      BIT(IIO_CHAN_INFO_SCALE) |
 				      BIT(IIO_CHAN_INFO_OFFSET),
+		.scan_index = 0,
+		.scan_type = {
+			.sign = 'u',
+			.realbits = 14,
+			.storagebits = 16,
+			.shift = 0,
+			.endianness = IIO_BE,
+		},
 	},
 	{
 		.type = IIO_TEMP,
 		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
 				      BIT(IIO_CHAN_INFO_SCALE) |
 				      BIT(IIO_CHAN_INFO_OFFSET),
+		.scan_index = 1,
+		.scan_type = {
+			.sign = 'u',
+			.realbits = 11,
+			.storagebits = 16,
+			.shift = 5,
+			.endianness = IIO_BE,
+		},
 	},
+	IIO_CHAN_SOFT_TIMESTAMP(2),
 };

 static const struct iio_info hsc_info = {
@@ -485,6 +522,11 @@ int hsc_common_probe(struct device *dev, hsc_recv_fn recv)
 	indio_dev->channels = hsc->chip->channels;
 	indio_dev->num_channels = hsc->chip->num_channels;

+	ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL,
+					      hsc_trigger_handler, NULL);
+	if (ret)
+		return ret;
+
 	return devm_iio_device_register(dev, indio_dev);
 }
 EXPORT_SYMBOL_NS(hsc_common_probe, IIO_HONEYWELL_HSC030PA);
diff --git a/drivers/iio/pressure/hsc030pa.h b/drivers/iio/pressure/hsc030pa.h
index 56dc8e88194b..6c635c42d85d 100644
--- a/drivers/iio/pressure/hsc030pa.h
+++ b/drivers/iio/pressure/hsc030pa.h
@@ -56,7 +56,7 @@ struct hsc_data {
 	s32 p_scale_dec;
 	s64 p_offset;
 	s32 p_offset_dec;
-	u8 buffer[HSC_REG_MEASUREMENT_RD_SIZE] __aligned(IIO_DMA_MINALIGN);
+	u8 buffer[16] __aligned(IIO_DMA_MINALIGN);
 };

 struct hsc_chip_data {
--
2.41.0

[PATCH 6/6] iio: pressure: hsc030pa add sleep mode

[Petre Rodan]

Some custom chips from this series require a wakeup sequence before the
measurement cycle is started.

Quote from the product datasheet:
"Optional sleep mode available upon special request."

Signed-off-by: Petre Rodan <petre.rodan@subdimension.ro>
---
 drivers/iio/pressure/hsc030pa.c     |  4 ++++
 drivers/iio/pressure/hsc030pa.h     |  4 ++++
 drivers/iio/pressure/hsc030pa_i2c.c | 19 +++++++++++++++++
 drivers/iio/pressure/hsc030pa_spi.c | 32 +++++++++++++++++++++++++++--
 4 files changed, 57 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/pressure/hsc030pa.c b/drivers/iio/pressure/hsc030pa.c
index 3faa0fd42201..9e66fd561801 100644
--- a/drivers/iio/pressure/hsc030pa.c
+++ b/drivers/iio/pressure/hsc030pa.c
@@ -501,6 +501,10 @@ int hsc_common_probe(struct device *dev, hsc_recv_fn recv)
 		return dev_err_probe(dev, -EINVAL,
 				     "pressure limits are invalid\n");

+	ret = device_property_read_bool(dev, "honeywell,sleep-mode");
+	if (ret)
+		hsc->capabilities |= HSC_CAP_SLEEP;
+
 	ret = devm_regulator_get_enable(dev, "vdd");
 	if (ret)
 		return dev_err_probe(dev, ret, "can't get vdd supply\n");
diff --git a/drivers/iio/pressure/hsc030pa.h b/drivers/iio/pressure/hsc030pa.h
index 6c635c42d85d..4e356944d67d 100644
--- a/drivers/iio/pressure/hsc030pa.h
+++ b/drivers/iio/pressure/hsc030pa.h
@@ -15,6 +15,8 @@
 #define HSC_REG_MEASUREMENT_RD_SIZE 4
 #define HSC_RESP_TIME_MS            2

+#define HSC_CAP_SLEEP               0x1
+
 struct device;

 struct iio_chan_spec;
@@ -29,6 +31,7 @@ typedef int (*hsc_recv_fn)(struct hsc_data *);
  * struct hsc_data
  * @dev: current device structure
  * @chip: structure containing chip's channel properties
+ * @capabilities: chip specific attributes
  * @recv_cb: function that implements the chip reads
  * @is_valid: true if last transfer has been validated
  * @pmin: minimum measurable pressure limit
@@ -45,6 +48,7 @@ typedef int (*hsc_recv_fn)(struct hsc_data *);
 struct hsc_data {
 	struct device *dev;
 	const struct hsc_chip_data *chip;
+	u32 capabilities;
 	hsc_recv_fn recv_cb;
 	bool is_valid;
 	s32 pmin;
diff --git a/drivers/iio/pressure/hsc030pa_i2c.c b/drivers/iio/pressure/hsc030pa_i2c.c
index b3fd230e71da..62bdae272012 100644
--- a/drivers/iio/pressure/hsc030pa_i2c.c
+++ b/drivers/iio/pressure/hsc030pa_i2c.c
@@ -24,8 +24,27 @@ static int hsc_i2c_recv(struct hsc_data *data)
 {
 	struct i2c_client *client = to_i2c_client(data->dev);
 	struct i2c_msg msg;
+	u8 buf;
 	int ret;

+	if (data->capabilities & HSC_CAP_SLEEP) {
+		/*
+		 * Send the Full Measurement Request (FMR) command on the CS
+		 * line in order to wake up the sensor as per
+		 * "Sleep Mode for Use with Honeywell Digital Pressure Sensors"
+		 * technical note (consult the datasheet link in the header).
+		 *
+		 * These specifications require a dummy packet comprised only by
+		 * a single byte that contains the 7bit slave address and the
+		 * READ bit followed by a STOP.
+		 * Because the i2c API does not allow packets without a payload,
+		 * the driver sends two bytes in this implementation.
+		 */
+		ret = i2c_master_recv(client, &buf, 1);
+		if (ret < 0)
+			return ret;
+	}
+
 	msleep_interruptible(HSC_RESP_TIME_MS);

 	msg.addr = client->addr;
diff --git a/drivers/iio/pressure/hsc030pa_spi.c b/drivers/iio/pressure/hsc030pa_spi.c
index 737197eddff0..1c139cdfe856 100644
--- a/drivers/iio/pressure/hsc030pa_spi.c
+++ b/drivers/iio/pressure/hsc030pa_spi.c
@@ -25,12 +25,40 @@ static int hsc_spi_recv(struct hsc_data *data)
 	struct spi_device *spi = to_spi_device(data->dev);
 	struct spi_transfer xfer = {
 		.tx_buf = NULL,
-		.rx_buf = data->buffer,
-		.len = HSC_REG_MEASUREMENT_RD_SIZE,
+		.rx_buf = NULL,
+		.len = 0,
 	};
+	u16 orig_cs_setup_value;
+	u8 orig_cs_setup_unit;
+
+	if (data->capabilities & HSC_CAP_SLEEP) {
+		/*
+		 * Send the Full Measurement Request (FMR) command on the CS
+		 * line in order to wake up the sensor as per
+		 * "Sleep Mode for Use with Honeywell Digital Pressure Sensors"
+		 * technical note (consult the datasheet link in the header).
+		 *
+		 * These specifications require the CS line to be held asserted
+		 * for at least 8µs without any payload being generated.
+		 */
+		orig_cs_setup_value = spi->cs_setup.value;
+		orig_cs_setup_unit = spi->cs_setup.unit;
+		spi->cs_setup.value = 8;
+		spi->cs_setup.unit = SPI_DELAY_UNIT_USECS;
+		/*
+		 * Send a dummy 0-size packet so that CS gets toggled.
+		 * Trying to manually call spi->controller->set_cs() instead
+		 * does not work as expected during the second call.
+		 */
+		spi_sync_transfer(spi, &xfer, 1);
+		spi->cs_setup.value = orig_cs_setup_value;
+		spi->cs_setup.unit = orig_cs_setup_unit;
+	}

 	msleep_interruptible(HSC_RESP_TIME_MS);

+	xfer.rx_buf = data->buffer;
+	xfer.len = HSC_REG_MEASUREMENT_RD_SIZE;
 	return spi_sync_transfer(spi, &xfer, 1);
 }

--
2.41.0

Re: [PATCH 1/6] dt-bindings: iio: pressure: honeywell,hsc030pa.yaml add spi props

[Krzysztof Kozlowski]

On 10/01/2024 18:22, Petre Rodan wrote:
> Add spi-peripheral-props.yaml requirement

This we see. What we do not see is: why? Please fix commit msg.


Best regards,
Krzysztof

Re: [PATCH 2/6] dt-bindings: iio: pressure: honeywell,hsc030pa.yaml add sleep-mode

[Krzysztof Kozlowski]

On 10/01/2024 18:22, Petre Rodan wrote:
> Add sleep-mode property present in some custom chips.
> 
> This flag activates a special wakeup sequence prior to conversion.
> 
> Signed-off-by: Petre Rodan <petre.rodan@subdimension.ro>
> ---
>  .../bindings/iio/pressure/honeywell,hsc030pa.yaml      | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml b/Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml
> index 89977b9f01cf..350da1d6991b 100644
> --- a/Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml
> +++ b/Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml
> @@ -86,6 +86,15 @@ properties:
>        Maximum pressure value the sensor can measure in pascal.
>        To be specified only if honeywell,pressure-triplet is set to "NA".
> 
> +  honeywell,sleep-mode:

"Sleep mode" naming suggests there are choices, like mode foo and mode
bar. Probably you want something like "sleep-between-measurements" or
something matching how does it work.


> +    description: |

Do not need '|' unless you need to preserve formatting.

> +      'Sleep Mode' is a special factory set mode of the chip that allows the
> +      sensor to power down between measurements. It is implemented only on
> +      special request, and it is an attribute not present in the HSC/SSC series
> +      nomenclature.
> +      Set in order to enable the special wakeup sequence prior to conversion.
> +    $ref: /schemas/types.yaml#/definitions/flag
> +
>    vdd-supply:
>      description:
>        Provide VDD power to the sensor (either 3.3V or 5V depending on the chip)
> @@ -140,6 +149,7 @@ examples:
>              honeywell,pressure-triplet = "NA";
>              honeywell,pmin-pascal = <0>;
>              honeywell,pmax-pascal = <200000>;
> +            //honeywell,sleep-mode;

Drop comment.

> 2.41.0
> 

Best regards,
Krzysztof

Re: [PATCH 2/6] dt-bindings: iio: pressure: honeywell,hsc030pa.yaml add sleep-mode

[Petre Rodan]

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


Hello Krzysztof,

On Wed, Jan 10, 2024 at 09:48:34PM +0100, Krzysztof Kozlowski wrote:
> On 10/01/2024 18:22, Petre Rodan wrote:
> > Add sleep-mode property present in some custom chips.
> > 
> > This flag activates a special wakeup sequence prior to conversion.
> > 
> > Signed-off-by: Petre Rodan <petre.rodan@subdimension.ro>
> > ---
> >  .../bindings/iio/pressure/honeywell,hsc030pa.yaml      | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml b/Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml
> > index 89977b9f01cf..350da1d6991b 100644
> > --- a/Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml
> > +++ b/Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml
> > @@ -86,6 +86,15 @@ properties:
> >        Maximum pressure value the sensor can measure in pascal.
> >        To be specified only if honeywell,pressure-triplet is set to "NA".
> > 
> > +  honeywell,sleep-mode:
> 
> "Sleep mode" naming suggests there are choices, like mode foo and mode
> bar. Probably you want something like "sleep-between-measurements" or
> something matching how does it work.

"sleep mode" is the terminology used by Honeywell and it defines a chip capability.
it is present in the HSC/SSC and ABP series of ICs.

other such options (capabilities) include temperature output in the ABP series.

the action the driver needs to perform if this option is present is to provide a
wake-up sequence before reading out the conversions.

now regarding a rename of this property, I would vote to leave it as is - for the
users to have a 1:1 equivalence of terms between the driver and the datasheet.

I say that because for instance in circuit design when a part symbol and
footprint is drawn based on a datasheet it is recommended to keep the same pin
notations and the same block diagram as in the datasheet, precisely for this 1:1
equivalence, so there is no uncertainty for the end-user.

cheers,
peter

> 
> > +    description: |
> 
> Do not need '|' unless you need to preserve formatting.
> 
> > +      'Sleep Mode' is a special factory set mode of the chip that allows the
> > +      sensor to power down between measurements. It is implemented only on
> > +      special request, and it is an attribute not present in the HSC/SSC series
> > +      nomenclature.
> > +      Set in order to enable the special wakeup sequence prior to conversion.
> > +    $ref: /schemas/types.yaml#/definitions/flag
> > +
> >    vdd-supply:
> >      description:
> >        Provide VDD power to the sensor (either 3.3V or 5V depending on the chip)
> > @@ -140,6 +149,7 @@ examples:
> >              honeywell,pressure-triplet = "NA";
> >              honeywell,pmin-pascal = <0>;
> >              honeywell,pmax-pascal = <200000>;
> > +            //honeywell,sleep-mode;
> 
> Drop comment.
> 
> > 2.41.0
> > 
> 
> Best regards,
> Krzysztof
> 

-- 
petre rodan

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

Re: [PATCH 5/6] iio: pressure: hsc030pa add triggered buffer

[Jonathan Cameron]

On Wed, 10 Jan 2024 19:22:40 +0200
Petre Rodan <petre.rodan@subdimension.ro> wrote:

> Add triggered buffer feature.
> 
> Signed-off-by: Petre Rodan <petre.rodan@subdimension.ro>
Hi Petre,

A few minor things inline. + you almost certainly need some Kconfig
changes to ensure buffered support is built for the IIO core.

> ---
>  drivers/iio/pressure/hsc030pa.c | 42 +++++++++++++++++++++++++++++++++
>  drivers/iio/pressure/hsc030pa.h |  2 +-
>  2 files changed, 43 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/pressure/hsc030pa.c b/drivers/iio/pressure/hsc030pa.c
> index 7e3f74d53b47..3faa0fd42201 100644
> --- a/drivers/iio/pressure/hsc030pa.c
> +++ b/drivers/iio/pressure/hsc030pa.c
> @@ -22,8 +22,11 @@
>  #include <linux/types.h>
>  #include <linux/units.h>
> 
> +#include <linux/iio/buffer.h>
>  #include <linux/iio/iio.h>
>  #include <linux/iio/sysfs.h>
> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/triggered_buffer.h>
> 
>  #include <asm/unaligned.h>
> 
> @@ -297,6 +300,23 @@ static int hsc_get_measurement(struct hsc_data *data)
>  	return 0;
>  }
> 
> +static irqreturn_t hsc_trigger_handler(int irq, void *private)
> +{
> +	struct iio_poll_func *pf = private;
> +	struct iio_dev *indio_dev = pf->indio_dev;
> +	struct hsc_data *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	ret = hsc_get_measurement(data);
> +	if (!ret) {
Prefer error handling out of line + no {} for single statements

	if (ret)
		goto error;

	iio_push...

error:
	iio_trigger...

> +		iio_push_to_buffers_with_timestamp(indio_dev, &data->buffer,
> +						   iio_get_time_ns(indio_dev));
> +	}
> +	iio_trigger_notify_done(indio_dev->trig);
> +
> +	return IRQ_HANDLED;
> +}
> +
>  /*
>   * IIO ABI expects
>   * value = (conv + offset) * scale
> @@ -382,13 +402,30 @@ static const struct iio_chan_spec hsc_channels[] = {
>  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
>  				      BIT(IIO_CHAN_INFO_SCALE) |
>  				      BIT(IIO_CHAN_INFO_OFFSET),
> +		.scan_index = 0,
> +		.scan_type = {
> +			.sign = 'u',
> +			.realbits = 14,
> +			.storagebits = 16,
> +			.shift = 0,

No need to specify shift if it's zero. That's considered the obvious default
and C will ensure it's set to 0 anyway.

> +			.endianness = IIO_BE,
> +		},
>  	},
>  	{
>  		.type = IIO_TEMP,
>  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
>  				      BIT(IIO_CHAN_INFO_SCALE) |
>  				      BIT(IIO_CHAN_INFO_OFFSET),
> +		.scan_index = 1,
> +		.scan_type = {
> +			.sign = 'u',
> +			.realbits = 11,
> +			.storagebits = 16,
> +			.shift = 5,
> +			.endianness = IIO_BE,
> +		},
>  	},
> +	IIO_CHAN_SOFT_TIMESTAMP(2),
>  };
> 
>  static const struct iio_info hsc_info = {
> @@ -485,6 +522,11 @@ int hsc_common_probe(struct device *dev, hsc_recv_fn recv)
>  	indio_dev->channels = hsc->chip->channels;
>  	indio_dev->num_channels = hsc->chip->num_channels;
> 
> +	ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL,
> +					      hsc_trigger_handler, NULL);
> +	if (ret)
> +		return ret;
> +
>  	return devm_iio_device_register(dev, indio_dev);
>  }
>  EXPORT_SYMBOL_NS(hsc_common_probe, IIO_HONEYWELL_HSC030PA);
> diff --git a/drivers/iio/pressure/hsc030pa.h b/drivers/iio/pressure/hsc030pa.h
> index 56dc8e88194b..6c635c42d85d 100644
> --- a/drivers/iio/pressure/hsc030pa.h
> +++ b/drivers/iio/pressure/hsc030pa.h
> @@ -56,7 +56,7 @@ struct hsc_data {
>  	s32 p_scale_dec;
>  	s64 p_offset;
>  	s32 p_offset_dec;
> -	u8 buffer[HSC_REG_MEASUREMENT_RD_SIZE] __aligned(IIO_DMA_MINALIGN);
> +	u8 buffer[16] __aligned(IIO_DMA_MINALIGN);

Justify that size.  Normal trick is to use a suitable structure definition with
the channels and a timestamp s64 with force alignment (To deal with te annoying 
x86 32bit alignment)

>  };
> 
>  struct hsc_chip_data {
> --
> 2.41.0
> 
> 

Re: [PATCH 6/6] iio: pressure: hsc030pa add sleep mode

[Jonathan Cameron]

On Wed, 10 Jan 2024 19:22:41 +0200
Petre Rodan <petre.rodan@subdimension.ro> wrote:

> Some custom chips from this series require a wakeup sequence before the
> measurement cycle is started.
> 
> Quote from the product datasheet:
> "Optional sleep mode available upon special request."
> 
> Signed-off-by: Petre Rodan <petre.rodan@subdimension.ro>
> ---
>  drivers/iio/pressure/hsc030pa.c     |  4 ++++
>  drivers/iio/pressure/hsc030pa.h     |  4 ++++
>  drivers/iio/pressure/hsc030pa_i2c.c | 19 +++++++++++++++++
>  drivers/iio/pressure/hsc030pa_spi.c | 32 +++++++++++++++++++++++++++--
>  4 files changed, 57 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/pressure/hsc030pa.c b/drivers/iio/pressure/hsc030pa.c
> index 3faa0fd42201..9e66fd561801 100644
> --- a/drivers/iio/pressure/hsc030pa.c
> +++ b/drivers/iio/pressure/hsc030pa.c
> @@ -501,6 +501,10 @@ int hsc_common_probe(struct device *dev, hsc_recv_fn recv)
>  		return dev_err_probe(dev, -EINVAL,
>  				     "pressure limits are invalid\n");
> 
> +	ret = device_property_read_bool(dev, "honeywell,sleep-mode");
> +	if (ret)
> +		hsc->capabilities |= HSC_CAP_SLEEP;
	if (device_property_read_bool())
		hsc->cap...

The return value is not an int so it's inappropriate to stash it in ret.

> +
>  	ret = devm_regulator_get_enable(dev, "vdd");
>  	if (ret)
>  		return dev_err_probe(dev, ret, "can't get vdd supply\n");
> diff --git a/drivers/iio/pressure/hsc030pa.h b/drivers/iio/pressure/hsc030pa.h
> index 6c635c42d85d..4e356944d67d 100644
> --- a/drivers/iio/pressure/hsc030pa.h
> +++ b/drivers/iio/pressure/hsc030pa.h
> @@ -15,6 +15,8 @@
>  #define HSC_REG_MEASUREMENT_RD_SIZE 4
>  #define HSC_RESP_TIME_MS            2
> 
> +#define HSC_CAP_SLEEP               0x1
> +
>  struct device;
> 
>  struct iio_chan_spec;
> @@ -29,6 +31,7 @@ typedef int (*hsc_recv_fn)(struct hsc_data *);
>   * struct hsc_data
>   * @dev: current device structure
>   * @chip: structure containing chip's channel properties
> + * @capabilities: chip specific attributes
>   * @recv_cb: function that implements the chip reads
>   * @is_valid: true if last transfer has been validated
>   * @pmin: minimum measurable pressure limit
> @@ -45,6 +48,7 @@ typedef int (*hsc_recv_fn)(struct hsc_data *);
>  struct hsc_data {
>  	struct device *dev;
>  	const struct hsc_chip_data *chip;
> +	u32 capabilities;
>  	hsc_recv_fn recv_cb;
>  	bool is_valid;
>  	s32 pmin;
> diff --git a/drivers/iio/pressure/hsc030pa_i2c.c b/drivers/iio/pressure/hsc030pa_i2c.c
> index b3fd230e71da..62bdae272012 100644
> --- a/drivers/iio/pressure/hsc030pa_i2c.c
> +++ b/drivers/iio/pressure/hsc030pa_i2c.c
> @@ -24,8 +24,27 @@ static int hsc_i2c_recv(struct hsc_data *data)
>  {
>  	struct i2c_client *client = to_i2c_client(data->dev);
>  	struct i2c_msg msg;
> +	u8 buf;
>  	int ret;
> 
> +	if (data->capabilities & HSC_CAP_SLEEP) {
> +		/*
> +		 * Send the Full Measurement Request (FMR) command on the CS
> +		 * line in order to wake up the sensor as per
> +		 * "Sleep Mode for Use with Honeywell Digital Pressure Sensors"
> +		 * technical note (consult the datasheet link in the header).
> +		 *
> +		 * These specifications require a dummy packet comprised only by
> +		 * a single byte that contains the 7bit slave address and the
> +		 * READ bit followed by a STOP.
> +		 * Because the i2c API does not allow packets without a payload,
> +		 * the driver sends two bytes in this implementation.
> +		 */
> +		ret = i2c_master_recv(client, &buf, 1);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
>  	msleep_interruptible(HSC_RESP_TIME_MS);
> 
>  	msg.addr = client->addr;
> diff --git a/drivers/iio/pressure/hsc030pa_spi.c b/drivers/iio/pressure/hsc030pa_spi.c
> index 737197eddff0..1c139cdfe856 100644
> --- a/drivers/iio/pressure/hsc030pa_spi.c
> +++ b/drivers/iio/pressure/hsc030pa_spi.c
> @@ -25,12 +25,40 @@ static int hsc_spi_recv(struct hsc_data *data)
>  	struct spi_device *spi = to_spi_device(data->dev);
>  	struct spi_transfer xfer = {
>  		.tx_buf = NULL,
> -		.rx_buf = data->buffer,
> -		.len = HSC_REG_MEASUREMENT_RD_SIZE,
> +		.rx_buf = NULL,
> +		.len = 0,
>  	};
> +	u16 orig_cs_setup_value;
> +	u8 orig_cs_setup_unit;
> +
> +	if (data->capabilities & HSC_CAP_SLEEP) {
> +		/*
> +		 * Send the Full Measurement Request (FMR) command on the CS
> +		 * line in order to wake up the sensor as per
> +		 * "Sleep Mode for Use with Honeywell Digital Pressure Sensors"
> +		 * technical note (consult the datasheet link in the header).
> +		 *
> +		 * These specifications require the CS line to be held asserted
> +		 * for at least 8µs without any payload being generated.
> +		 */
> +		orig_cs_setup_value = spi->cs_setup.value;
> +		orig_cs_setup_unit = spi->cs_setup.unit;
> +		spi->cs_setup.value = 8;
> +		spi->cs_setup.unit = SPI_DELAY_UNIT_USECS;
> +		/*
> +		 * Send a dummy 0-size packet so that CS gets toggled.
> +		 * Trying to manually call spi->controller->set_cs() instead
> +		 * does not work as expected during the second call.
> +		 */

Do you have a reference that says the CS must be toggled on 0 length transfer?
If that's not specified in the SPI core somewhere then you will need to send
something...

> +		spi_sync_transfer(spi, &xfer, 1);
> +		spi->cs_setup.value = orig_cs_setup_value;
> +		spi->cs_setup.unit = orig_cs_setup_unit;
> +	}
> 
>  	msleep_interruptible(HSC_RESP_TIME_MS);
> 
> +	xfer.rx_buf = data->buffer;
> +	xfer.len = HSC_REG_MEASUREMENT_RD_SIZE;
>  	return spi_sync_transfer(spi, &xfer, 1);
>  }
> 
> --
> 2.41.0
> 
> 

Re: [PATCH 6/6] iio: pressure: hsc030pa add sleep mode

[Petre Rodan]

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


Hi Jonathan,

On Fri, Jan 12, 2024 at 05:13:56PM +0000, Jonathan Cameron wrote:
> On Wed, 10 Jan 2024 19:22:41 +0200
> Petre Rodan <petre.rodan@subdimension.ro> wrote:
> 
> > Some custom chips from this series require a wakeup sequence before the
> > measurement cycle is started.
> > 
[..]
> > +	if (data->capabilities & HSC_CAP_SLEEP) {
> > +		/*
> > +		 * Send the Full Measurement Request (FMR) command on the CS
> > +		 * line in order to wake up the sensor as per
> > +		 * "Sleep Mode for Use with Honeywell Digital Pressure Sensors"
> > +		 * technical note (consult the datasheet link in the header).
> > +		 *
> > +		 * These specifications require a dummy packet comprised only by
> > +		 * a single byte that contains the 7bit slave address and the
> > +		 * READ bit followed by a STOP.
> > +		 * Because the i2c API does not allow packets without a payload,
> > +		 * the driver sends two bytes in this implementation.
> > +		 */
> > +		ret = i2c_master_recv(client, &buf, 1);
> > +		if (ret < 0)
> > +			return ret;
> > +	}
> > +
[..]
> > diff --git a/drivers/iio/pressure/hsc030pa_spi.c b/drivers/iio/pressure/hsc030pa_spi.c
> > index 737197eddff0..1c139cdfe856 100644
> > --- a/drivers/iio/pressure/hsc030pa_spi.c
> > +++ b/drivers/iio/pressure/hsc030pa_spi.c
> > @@ -25,12 +25,40 @@ static int hsc_spi_recv(struct hsc_data *data)
> >  	struct spi_device *spi = to_spi_device(data->dev);
> >  	struct spi_transfer xfer = {
> >  		.tx_buf = NULL,
> > -		.rx_buf = data->buffer,
> > -		.len = HSC_REG_MEASUREMENT_RD_SIZE,
> > +		.rx_buf = NULL,
> > +		.len = 0,
> >  	};
> > +	u16 orig_cs_setup_value;
> > +	u8 orig_cs_setup_unit;
> > +
> > +	if (data->capabilities & HSC_CAP_SLEEP) {
> > +		/*
> > +		 * Send the Full Measurement Request (FMR) command on the CS
> > +		 * line in order to wake up the sensor as per
> > +		 * "Sleep Mode for Use with Honeywell Digital Pressure Sensors"
> > +		 * technical note (consult the datasheet link in the header).
> > +		 *
> > +		 * These specifications require the CS line to be held asserted
> > +		 * for at least 8µs without any payload being generated.
> > +		 */
> > +		orig_cs_setup_value = spi->cs_setup.value;
> > +		orig_cs_setup_unit = spi->cs_setup.unit;
> > +		spi->cs_setup.value = 8;
> > +		spi->cs_setup.unit = SPI_DELAY_UNIT_USECS;
> > +		/*
> > +		 * Send a dummy 0-size packet so that CS gets toggled.
> > +		 * Trying to manually call spi->controller->set_cs() instead
> > +		 * does not work as expected during the second call.
> > +		 */
>
> Do you have a reference that says the CS must be toggled on 0 length transfer?
> If that's not specified in the SPI core somewhere then you will need to send
> something...
>
> > +		spi_sync_transfer(spi, &xfer, 1);
> > +		spi->cs_setup.value = orig_cs_setup_value;
> > +		spi->cs_setup.unit = orig_cs_setup_unit;
> > +	}

first of all thank you for the review.

I was afraid that this block will not be taken too well since I'm trying to
achieve something that the SPI subsystem was not designed for.

the code does exactly what the datasheet specs require on my SPI controller, but
indeed the API might change at some point making the code non-functional.

by 'sending something' you mean on the SPI bus or are you pushing me toward a
patch to SPI core?

unfortunately this chip feature is a special request only, there is no way for
me to test what happens if the wakeup sequence also contains a payload (in both
i2c and spi cases). the i2c wakeup code was inspired from the abp060mg driver,
but I can't reach its maintainer to ask for details. I also can't seem to reach
Honeywell. oh well.

best regards,
peter

-- 
petre rodan

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

Re: [PATCH 6/6] iio: pressure: hsc030pa add sleep mode

[Jonathan Cameron]

On Sun, 14 Jan 2024 07:17:47 +0200
Petre Rodan <petre.rodan@subdimension.ro> wrote:

> Hi Jonathan,
> 
> On Fri, Jan 12, 2024 at 05:13:56PM +0000, Jonathan Cameron wrote:
> > On Wed, 10 Jan 2024 19:22:41 +0200
> > Petre Rodan <petre.rodan@subdimension.ro> wrote:
> >   
> > > Some custom chips from this series require a wakeup sequence before the
> > > measurement cycle is started.
> > >   
> [..]
> > > +	if (data->capabilities & HSC_CAP_SLEEP) {
> > > +		/*
> > > +		 * Send the Full Measurement Request (FMR) command on the CS
> > > +		 * line in order to wake up the sensor as per
> > > +		 * "Sleep Mode for Use with Honeywell Digital Pressure Sensors"
> > > +		 * technical note (consult the datasheet link in the header).
> > > +		 *
> > > +		 * These specifications require a dummy packet comprised only by
> > > +		 * a single byte that contains the 7bit slave address and the
> > > +		 * READ bit followed by a STOP.
> > > +		 * Because the i2c API does not allow packets without a payload,
> > > +		 * the driver sends two bytes in this implementation.
> > > +		 */
> > > +		ret = i2c_master_recv(client, &buf, 1);
> > > +		if (ret < 0)
> > > +			return ret;
> > > +	}
> > > +  
> [..]
> > > diff --git a/drivers/iio/pressure/hsc030pa_spi.c b/drivers/iio/pressure/hsc030pa_spi.c
> > > index 737197eddff0..1c139cdfe856 100644
> > > --- a/drivers/iio/pressure/hsc030pa_spi.c
> > > +++ b/drivers/iio/pressure/hsc030pa_spi.c
> > > @@ -25,12 +25,40 @@ static int hsc_spi_recv(struct hsc_data *data)
> > >  	struct spi_device *spi = to_spi_device(data->dev);
> > >  	struct spi_transfer xfer = {
> > >  		.tx_buf = NULL,
> > > -		.rx_buf = data->buffer,
> > > -		.len = HSC_REG_MEASUREMENT_RD_SIZE,
> > > +		.rx_buf = NULL,
> > > +		.len = 0,
> > >  	};
> > > +	u16 orig_cs_setup_value;
> > > +	u8 orig_cs_setup_unit;
> > > +
> > > +	if (data->capabilities & HSC_CAP_SLEEP) {
> > > +		/*
> > > +		 * Send the Full Measurement Request (FMR) command on the CS
> > > +		 * line in order to wake up the sensor as per
> > > +		 * "Sleep Mode for Use with Honeywell Digital Pressure Sensors"
> > > +		 * technical note (consult the datasheet link in the header).
> > > +		 *
> > > +		 * These specifications require the CS line to be held asserted
> > > +		 * for at least 8µs without any payload being generated.
> > > +		 */
> > > +		orig_cs_setup_value = spi->cs_setup.value;
> > > +		orig_cs_setup_unit = spi->cs_setup.unit;
> > > +		spi->cs_setup.value = 8;
> > > +		spi->cs_setup.unit = SPI_DELAY_UNIT_USECS;
> > > +		/*
> > > +		 * Send a dummy 0-size packet so that CS gets toggled.
> > > +		 * Trying to manually call spi->controller->set_cs() instead
> > > +		 * does not work as expected during the second call.
> > > +		 */  
> >
> > Do you have a reference that says the CS must be toggled on 0 length transfer?
> > If that's not specified in the SPI core somewhere then you will need to send
> > something...
> >  
> > > +		spi_sync_transfer(spi, &xfer, 1);
> > > +		spi->cs_setup.value = orig_cs_setup_value;
> > > +		spi->cs_setup.unit = orig_cs_setup_unit;
> > > +	}  
> 
> first of all thank you for the review.
> 
> I was afraid that this block will not be taken too well since I'm trying to
> achieve something that the SPI subsystem was not designed for.
> 
> the code does exactly what the datasheet specs require on my SPI controller, but
> indeed the API might change at some point making the code non-functional.
> 
> by 'sending something' you mean on the SPI bus or are you pushing me toward a
> patch to SPI core?
Should have said receive 'something' - that means that we'd clock some data
whether or not the device had any to provide.

> 
> unfortunately this chip feature is a special request only, there is no way for
> me to test what happens if the wakeup sequence also contains a payload (in both
> i2c and spi cases). the i2c wakeup code was inspired from the abp060mg driver,
> but I can't reach its maintainer to ask for details. I also can't seem to reach
> Honeywell. oh well.
> 
+CC Mark.  Discussion is about whether a zero length SPI transfer is guaranteed
to pulse the chip select.

If this is something we need, I'd prefer to see it as something a given SPI
controller would opt in to supporting or some other way of knowing it would
actually happen such as some docs that say it will work for an SPI controller
(which I doubt is the case)

Jonathan





> best regards,
> peter
> 

Re: [PATCH 2/6] dt-bindings: iio: pressure: honeywell,hsc030pa.yaml add sleep-mode

[Rob Herring]

On Fri, Jan 12, 2024 at 09:30:30AM +0200, Petre Rodan wrote:
> 
> Hello Krzysztof,
> 
> On Wed, Jan 10, 2024 at 09:48:34PM +0100, Krzysztof Kozlowski wrote:
> > On 10/01/2024 18:22, Petre Rodan wrote:
> > > Add sleep-mode property present in some custom chips.
> > > 
> > > This flag activates a special wakeup sequence prior to conversion.
> > > 
> > > Signed-off-by: Petre Rodan <petre.rodan@subdimension.ro>
> > > ---
> > >  .../bindings/iio/pressure/honeywell,hsc030pa.yaml      | 10 ++++++++++
> > >  1 file changed, 10 insertions(+)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml b/Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml
> > > index 89977b9f01cf..350da1d6991b 100644
> > > --- a/Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml
> > > +++ b/Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml
> > > @@ -86,6 +86,15 @@ properties:
> > >        Maximum pressure value the sensor can measure in pascal.
> > >        To be specified only if honeywell,pressure-triplet is set to "NA".
> > > 
> > > +  honeywell,sleep-mode:
> > 
> > "Sleep mode" naming suggests there are choices, like mode foo and mode
> > bar. Probably you want something like "sleep-between-measurements" or
> > something matching how does it work.
> 
> "sleep mode" is the terminology used by Honeywell and it defines a chip capability.
> it is present in the HSC/SSC and ABP series of ICs.
> 
> other such options (capabilities) include temperature output in the ABP series.
> 
> the action the driver needs to perform if this option is present is to provide a
> wake-up sequence before reading out the conversions.
> 
> now regarding a rename of this property, I would vote to leave it as is - for the
> users to have a 1:1 equivalence of terms between the driver and the datasheet.
> 
> I say that because for instance in circuit design when a part symbol and
> footprint is drawn based on a datasheet it is recommended to keep the same pin
> notations and the same block diagram as in the datasheet, precisely for this 1:1
> equivalence, so there is no uncertainty for the end-user.

At least add a '-en' suffix so it is clear this property enables the 
mode. We have both flavors (enables and disables). 

Low power modes between samples is pretty common on these devices. We 
should consider if this should be a common property. Jonathan?

Rob

Re: [PATCH 2/6] dt-bindings: iio: pressure: honeywell,hsc030pa.yaml add sleep-mode

[Jonathan Cameron]

On Tue, 16 Jan 2024 11:30:23 -0600
Rob Herring <robh@kernel.org> wrote:

> On Fri, Jan 12, 2024 at 09:30:30AM +0200, Petre Rodan wrote:
> > 
> > Hello Krzysztof,
> > 
> > On Wed, Jan 10, 2024 at 09:48:34PM +0100, Krzysztof Kozlowski wrote:  
> > > On 10/01/2024 18:22, Petre Rodan wrote:  
> > > > Add sleep-mode property present in some custom chips.
> > > > 
> > > > This flag activates a special wakeup sequence prior to conversion.
> > > > 
> > > > Signed-off-by: Petre Rodan <petre.rodan@subdimension.ro>
> > > > ---
> > > >  .../bindings/iio/pressure/honeywell,hsc030pa.yaml      | 10 ++++++++++
> > > >  1 file changed, 10 insertions(+)
> > > > 
> > > > diff --git a/Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml b/Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml
> > > > index 89977b9f01cf..350da1d6991b 100644
> > > > --- a/Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml
> > > > +++ b/Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml
> > > > @@ -86,6 +86,15 @@ properties:
> > > >        Maximum pressure value the sensor can measure in pascal.
> > > >        To be specified only if honeywell,pressure-triplet is set to "NA".
> > > > 
> > > > +  honeywell,sleep-mode:  
> > > 
> > > "Sleep mode" naming suggests there are choices, like mode foo and mode
> > > bar. Probably you want something like "sleep-between-measurements" or
> > > something matching how does it work.  
> > 
> > "sleep mode" is the terminology used by Honeywell and it defines a chip capability.
> > it is present in the HSC/SSC and ABP series of ICs.
> > 
> > other such options (capabilities) include temperature output in the ABP series.
> > 
> > the action the driver needs to perform if this option is present is to provide a
> > wake-up sequence before reading out the conversions.
> > 
> > now regarding a rename of this property, I would vote to leave it as is - for the
> > users to have a 1:1 equivalence of terms between the driver and the datasheet.
> > 
> > I say that because for instance in circuit design when a part symbol and
> > footprint is drawn based on a datasheet it is recommended to keep the same pin
> > notations and the same block diagram as in the datasheet, precisely for this 1:1
> > equivalence, so there is no uncertainty for the end-user.  
> 
> At least add a '-en' suffix so it is clear this property enables the 
> mode. We have both flavors (enables and disables). 
> 
> Low power modes between samples is pretty common on these devices. We 
> should consider if this should be a common property. Jonathan?

Normally it's a controllable things so we make it dependent on userspace
interaction (runtime-pm or whether buffered capture is enabled).
Policy thing so not appropriate in DT.

Here it's different because it's a particular device variant that must work in this
fashion. Other device variants don't support it at all.

If it weren't for the obscene number of variants this would normally be
derived from the compatible rather than being in DT at all.

So it's odd and I don't think appropriate for a common property.

Jonathan

> 
> Rob
> 

Re: [PATCH 2/6] dt-bindings: iio: pressure: honeywell,hsc030pa.yaml add sleep-mode

[Rob Herring]

On Wed, Jan 17, 2024 at 04:50:01PM +0000, Jonathan Cameron wrote:
> On Tue, 16 Jan 2024 11:30:23 -0600
> Rob Herring <robh@kernel.org> wrote:
> 
> > On Fri, Jan 12, 2024 at 09:30:30AM +0200, Petre Rodan wrote:
> > > 
> > > Hello Krzysztof,
> > > 
> > > On Wed, Jan 10, 2024 at 09:48:34PM +0100, Krzysztof Kozlowski wrote:  
> > > > On 10/01/2024 18:22, Petre Rodan wrote:  
> > > > > Add sleep-mode property present in some custom chips.
> > > > > 
> > > > > This flag activates a special wakeup sequence prior to conversion.
> > > > > 
> > > > > Signed-off-by: Petre Rodan <petre.rodan@subdimension.ro>
> > > > > ---
> > > > >  .../bindings/iio/pressure/honeywell,hsc030pa.yaml      | 10 ++++++++++
> > > > >  1 file changed, 10 insertions(+)
> > > > > 
> > > > > diff --git a/Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml b/Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml
> > > > > index 89977b9f01cf..350da1d6991b 100644
> > > > > --- a/Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml
> > > > > +++ b/Documentation/devicetree/bindings/iio/pressure/honeywell,hsc030pa.yaml
> > > > > @@ -86,6 +86,15 @@ properties:
> > > > >        Maximum pressure value the sensor can measure in pascal.
> > > > >        To be specified only if honeywell,pressure-triplet is set to "NA".
> > > > > 
> > > > > +  honeywell,sleep-mode:  
> > > > 
> > > > "Sleep mode" naming suggests there are choices, like mode foo and mode
> > > > bar. Probably you want something like "sleep-between-measurements" or
> > > > something matching how does it work.  
> > > 
> > > "sleep mode" is the terminology used by Honeywell and it defines a chip capability.
> > > it is present in the HSC/SSC and ABP series of ICs.
> > > 
> > > other such options (capabilities) include temperature output in the ABP series.
> > > 
> > > the action the driver needs to perform if this option is present is to provide a
> > > wake-up sequence before reading out the conversions.
> > > 
> > > now regarding a rename of this property, I would vote to leave it as is - for the
> > > users to have a 1:1 equivalence of terms between the driver and the datasheet.
> > > 
> > > I say that because for instance in circuit design when a part symbol and
> > > footprint is drawn based on a datasheet it is recommended to keep the same pin
> > > notations and the same block diagram as in the datasheet, precisely for this 1:1
> > > equivalence, so there is no uncertainty for the end-user.  
> > 
> > At least add a '-en' suffix so it is clear this property enables the 
> > mode. We have both flavors (enables and disables). 
> > 
> > Low power modes between samples is pretty common on these devices. We 
> > should consider if this should be a common property. Jonathan?
> 
> Normally it's a controllable things so we make it dependent on userspace
> interaction (runtime-pm or whether buffered capture is enabled).
> Policy thing so not appropriate in DT.
> 
> Here it's different because it's a particular device variant that must work in this
> fashion. Other device variants don't support it at all.
> 
> If it weren't for the obscene number of variants this would normally be
> derived from the compatible rather than being in DT at all.
> 
> So it's odd and I don't think appropriate for a common property.

All good details missing from the description and commit msg. Given 
that, I retract my suggestion to use '-en' as it not a case of enabling 
the feature. Probably worth just keeping the name as-is than discussing 
further.

Rob