[PATCH v4 0/9] spi: add multi-CS and multi-chip aggregation support

[PATCH v4 0/9] spi: add multi-CS and multi-chip aggregation support

[Jonathan Santos]

This series adds support for multi-CS devices and ancillary devices with
lanes, to enable the multi-device setup described in [1], where multiple
AD7768-1 ADCs are wired to the same controller in a synchronized sampling
configuration, and the limitation noted in [2], where no SPI controller 
completely handles logical chip selects beyond the first one.

The first part of the set addresses multi-CS support. Some SPI controllers
can assert multiple chip selects simultaneously, but the existing code
hardcoded CS index 0 in both spi_set_cs() and of_spi_parse_dt(),
preventing this from working. The spi device name pattern is modified to
include all active CS lines in range format, so multi-CS devices are 
distinguishable in sysfs.

The second part introduces spi_new_ancillary_device_with_lane() and its
devm variant, used here to create subdevices that bind the CS with a 
corresponding rx and tx lane from the parent device. With that we can 
handle each device individually when there are separate data lanes.

The last part uses the above infrastructure in the AD7768-1 driver to
support aggregating multiple chips as a single IIO device. The number of
sub-devices is derived from the number of CS lines. IIO channels are
allocated dynamically, buffered capture is restricted to the full-device
scan mask, and SPI offload mode enables SPI_MULTI_LANE_MODE_STRIPE to
interleave samples from all lanes into the DMA stream.

---
v4 summary:
* Included patch to add new spi-cs-composite property to the bindings,
  to allow multi-CS devices.
* Added new patch restricting CS logical values to a contiguous sequence.
* Device name for multi-CS devices is now formatted as range (the 
  contiguous restriction makes it possible) instead of a list.
* Introduced KUnit tests for SPI core CS management, following the SPI DMA
  pattern.
* Addressed other minor suggestions, see individual patches. Please refer
  to "OBS For V4" section before proceeding.
* Link to v3: https://lore.kernel.org/linux-spi/cover.1786973376.git.Jonathan.Santos@analog.com/T/#t

v3 summary:
* Added new patch to document the properties for multidevice support on 
  ad7768-1 bindings.
* Refactored "spi: introduce SPI ancillary device with lanes" patch
  to reduce duplicated code using helper functions.
* Modified channel mask allocation on ad7768_parse_aaf_gain() to get 
  a static value, Since we assume the family is single channel. Added a 
  comment explaining.
* Addressed other minor suggestions, see individual patches. Please refer
  to "OBS For V3" section before proceeding.
* Link to v2: https://lore.kernel.org/linux-spi/cover.1785725359.git.Jonathan.Santos@analog.com/T/#t

v2 summary:
* Removed the per-transfer CS mask patches, the devices will be 
  individually handled by the ancillary device feature.
* Removed dynamic multilane selection patches. The lane is bound to the 
  CS using the new spi_new_ancillary_device_with_lane() feature.
* Included a use case patch modifying the AD7768-1 driver to support 
  multidevice aggregation.
* Link to v1: https://lore.kernel.org/linux-spi/cover.1783729282.git.Jonathan.Santos@analog.com/T/#t

OBS For V4: An alternative to the 'spi-cs-composite' property would be a
separate composite device holding all CS in its cs_index_mask (as suggested
by David), while the main device and the ancillaries are used for
individual access. That approach was dropped because the composite device
claims chip selects already taken by its siblings (the main device and the
ancillaries), and the CS conflict check only skips the parent device, not
siblings. Relaxing that check is not trivial, so the new property instead
lets the main device act as the composite while the ancillaries handle
individual access.

OBS For V3: The ancillary device approach was chosen over per-transfer CS
masks because it has better integration with regmap. Both approaches
require multiple regmap instances, but the CS mask approach would
additionally require custom read/write functions to route transfers to the
correct CS, while ancillary devices handle this more transparently.

David suggested adding an ancillary device field to spi_message, I guess to
enable dynamic CS-lane selection. For now, using one ancillary device per 
chip for individual access and the main SPI device for simultaneous access
seems to be sufficient. The dynamic approach can be implemented in the future
if needed.

---

[1]: https://lore.kernel.org/linux-iio/af0EGv172ZMl%2F6N5@JSANTO12-L01.ad.analog.com/T/#t
[2]: https://lore.kernel.org/all/20250915183725.219473-1-jonas.gorski@gmail.com/

Jonathan Santos (9):
  spi: dt-bindings: Add spi-cs-composite property
  dt-bindings: iio: adc: ad7768-1: Document multidevice support
  spi: support simultaneous assertion of multiple CS
  spi: restrict CS logical values to a contiguous sequence
  spi: expand device name to include enabled CS lines for multi-CS
    devices
  spi: introduce SPI ancillary device with lanes
  spi: add KUnit tests for the multi-CS framework
  spi: spi-engine-ex: Add support for multi-CS devices
  iio: adc: ad7768-1: add support for multiple chip aggregation

 .../bindings/iio/adc/adi,ad7768-1.yaml        |  80 +++-
 .../bindings/spi/spi-peripheral-props.yaml    |   8 +
 drivers/iio/adc/ad7768-1.c                    | 100 ++++-
 drivers/spi/.kunitconfig                      |   1 +
 drivers/spi/Kconfig                           |  11 +
 drivers/spi/Makefile                          |   2 +-
 drivers/spi/internals.h                       |  16 +
 drivers/spi/spi-axi-spi-engine.c              |  21 +-
 drivers/spi/spi.c                             | 172 ++++++--
 drivers/spi/tests/Makefile                    |   3 +-
 drivers/spi/tests/spi-multi-cs-kunit.c        | 374 ++++++++++++++++++
 include/linux/spi/spi.h                       |   8 +
 12 files changed, 751 insertions(+), 45 deletions(-)
 create mode 100644 drivers/spi/tests/spi-multi-cs-kunit.c


base-commit: 43ffda3c7f8456bbe641ae8249a077a361b97880
-- 
2.34.1

[PATCH v4 1/9] spi: dt-bindings: Add spi-cs-composite property

[Jonathan Santos]

Some setups combine several SPI devices into one logical device, with
each chip on its own CS line. All of those CS lines must be asserted at
the same time, so that one transfer reaches every chip.

Add the 'spi-cs-composite' boolean property. When it is set, every chip
select listed in 'reg' is asserted together on each transfer.

Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com>
---
Changes in v4:
* New patch. This has been included because without an explicit property,
  every device with more than one 'reg' entry would have all its chip
  selects asserted together. That breaks devices that use ancillary
  devices for the extra CS lines, so the composite behavior is now opt-in.
---
 .../devicetree/bindings/spi/spi-peripheral-props.yaml     | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
index 4894196c8f9c..456cb9095632 100644
--- a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
+++ b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
@@ -60,6 +60,14 @@ properties:
       Delay in nanoseconds to be introduced by the controller after CS is
       de-asserted.
 
+  spi-cs-composite:
+    description: Some SPI devices can be aggregated into a single logical
+      device by asserting all their chip selects simultaneously. When this
+      property is present, all chip selects listed in the 'reg' property are
+      asserted together on every transfer, treating the group as one composite
+      device.
+    type: boolean
+
   spi-rx-bus-width:
     description:
       Bus width to the SPI bus used for read transfers.
-- 
2.34.1

[PATCH v4 2/9] dt-bindings: iio: adc: ad7768-1: Document multidevice support

[Jonathan Santos]

The AD7768-1 family supports aggregating up to four devices on a single
SPI controller, with each device occupying a dedicated CS line and a
single data lane.

Extend the `reg` property to accept up to 4 entries (one per device).

Add `spi-rx-bus-width` property to declare the per-device lane count.
Since each lane is bound to one CS, the number of entries must match
`reg`. Make it required when `reg` has more than one entry, along with
`spi-cs-composite` property.

Add an example for a four ADAQ7768-1 aggregation, with shared DRDY and
SYNC_IN pin.

Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com>
---
Changes in v4:
* Include spi-cs-composite property to the bindings. The reason for going
  this route is stated in the cover letter.
* We decided to keep the spi-rx-bus-width property because assuming 1-lane
  per CS would break controllers that don't support multiple lanes.

Changes in v3:
* New patch.
---
 .../bindings/iio/adc/adi,ad7768-1.yaml        | 80 ++++++++++++++++++-
 1 file changed, 79 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml
index 7b72d14e104c..64c1cbeaa576 100644
--- a/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml
+++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml
@@ -26,7 +26,10 @@ properties:
       - adi,adaq7769-1
 
   reg:
-    maxItems: 1
+    minItems: 1
+    maxItems: 4
+    description:
+      Chip select number. For multi-device setups, one entry per device.
 
   clocks:
     maxItems: 1
@@ -109,6 +112,12 @@ properties:
   reset-gpios:
     maxItems: 1
 
+  spi-rx-bus-width:
+    description:
+      Bus width per CS. For multi-device setups (multiple reg entries), one
+      entry per device must be provided. Required when reg has more than
+      one entry.
+
   spi-cpol: true
 
   spi-cpha: true
@@ -203,6 +212,16 @@ allOf:
       properties:
         pga-gpios: false
 
+  # Required properties for multi-device setups
+  - if:
+      properties:
+        reg:
+          minItems: 2
+    then:
+      required:
+        - spi-rx-bus-width
+        - spi-cs-composite
+
 unevaluatedProperties: false
 
 examples:
@@ -244,4 +263,63 @@ examples:
             };
         };
     };
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+    #include <dt-bindings/gpio/gpio.h>
+    #include <dt-bindings/iio/adc/adi,ad7768-1.h>
+    /*
+     * Four ADAQ7768-1 devices aggregated on a single SPI controller.
+     * Each device occupies one CS and one data lane; DRDY and SYNC_IN
+     * pins are wired together across all four devices.
+     */
+    spi {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        adc@0 {
+            compatible = "adi,adaq7768-1";
+            reg = <0>, <1>, <2>, <3>;
+            spi-rx-bus-width = <1>, <1>, <1>, <1>;
+            spi-cs-composite;
+            spi-max-frequency = <2000000>;
+            spi-cpol;
+            spi-cpha;
+            vref-supply = <&adc_vref>;
+            pga-gpios = <&gpio 10 GPIO_ACTIVE_HIGH>,
+                        <&gpio 11 GPIO_ACTIVE_HIGH>,
+                        <&gpio 12 GPIO_ACTIVE_HIGH>;
+            interrupts = <25 IRQ_TYPE_EDGE_RISING>;
+            interrupt-parent = <&gpio>;
+            #trigger-source-cells = <1>;
+            adi,sync-in-gpios = <&gpio 22 GPIO_ACTIVE_LOW>;
+            reset-gpios = <&gpio 27 GPIO_ACTIVE_LOW>;
+            clocks = <&ad7768_mclk>;
+            clock-names = "mclk";
+            gpio-controller;
+            #gpio-cells = <2>;
+            #io-channel-cells = <1>;
+            #address-cells = <1>;
+            #size-cells = <0>;
+
+            channel@0 {
+                reg = <0>;
+                label = "channel_0";
+            };
+
+            channel@1 {
+                reg = <1>;
+                label = "channel_1";
+            };
+
+            channel@2 {
+                reg = <2>;
+                label = "channel_2";
+            };
+
+            channel@3 {
+                reg = <3>;
+                label = "channel_3";
+            };
+        };
+    };
 ...
-- 
2.34.1

[PATCH v4 3/9] spi: support simultaneous assertion of multiple CS

[Jonathan Santos]

Some SPI controllers allow multiple CS lines to be toggled at the same
time. The existing code always used CS index 0 when tracking the last
active CS in spi_set_cs(), and unconditionally set cs_index_mask to
BIT(0) when parsing DT, both forcing the single CS usage.

Modify spi_set_cs() to iterate last_cs[] using each logical CS index
instead of always reading index 0. Modify of_spi_parse_dt() to build
cs_index_mask from all parsed CS entries when 'spi-cs-composite' is set,
so the controller identifies which CS lines belong to a device when
asserting them simultaneously as a composite device.

Board info, ACPI, and ancillary device paths are not updated here.
Board info would require an API change to accept an array of CS values
and is left for a follow-up when we have a use case for this. Ancillary
devices are by design single-CS, so multi-CS is not a current use case for
them. ACPI represents the CS as a 64-bit integer with no established
convention for encoding multiple CS indices yet, so any extension there
would require a separate specification effort.

Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com>
---
Changes in v4:
* use spi-cs-composite to determine whether to assert all CS lines
  together, rather than always asserting them together.
* Replaced the for-loop to assign the spi->cs_index_mask with 
  "GENMASK(spi->num_chipselect - 1, 0)".

Changes in v3:
* None.

Changes in v2:
* Include Summary describing why the other SPI paths were not addressed
  here.
---
 drivers/spi/spi.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 5b5b3bc5f0d8..7a0406ec200e 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -1278,7 +1278,7 @@ static void spi_set_cs(struct spi_device *spi, bool enable, bool force)
 	spi->controller->last_cs_index_mask = spi->cs_index_mask;
 	for (idx = 0; idx < SPI_DEVICE_CS_CNT_MAX; idx++) {
 		if (enable && idx < spi->num_chipselect)
-			spi->controller->last_cs[idx] = spi_get_chipselect(spi, 0);
+			spi->controller->last_cs[idx] = spi_get_chipselect(spi, idx);
 		else
 			spi->controller->last_cs[idx] = SPI_INVALID_CS;
 	}
@@ -2794,8 +2794,19 @@ static int of_spi_parse_dt(struct spi_controller *ctlr, struct spi_device *spi,
 	/*
 	 * By default spi->chip_select[0] will hold the physical CS number,
 	 * so set bit 0 in spi->cs_index_mask.
+	 *
+	 * If "spi-cs-composite" is present, assert all CS lines at once
+	 * so the device acts as a single composite target.
 	 */
-	spi->cs_index_mask = BIT(0);
+	if (of_property_read_bool(nc, "spi-cs-composite")) {
+		if (!(ctlr->flags & SPI_CONTROLLER_MULTI_CS)) {
+			dev_err(&ctlr->dev, "SPI controller doesn't support multi CS\n");
+			return -EINVAL;
+		}
+		spi->cs_index_mask = GENMASK(spi->num_chipselect - 1, 0);
+	} else {
+		spi->cs_index_mask = BIT(0);
+	}
 
 	/* Device speed */
 	if (!of_property_read_u32(nc, "spi-max-frequency", &value))
-- 
2.34.1

[PATCH v4 4/9] spi: restrict CS logical values to a contiguous sequence

[Jonathan Santos]

When a device uses more than one chip select, require the logical CS
numbers to be consecutive (for example 2, 3, 4). If they aren't,
__spi_add_device() now fails with -EINVAL and logs which CS breaks
the sequence.

Suggested-by: Andy Shevchenko <andriy.shevchenko@intel.com>
Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com>
---
Changes in v4:
* New patch, suggested by Andy. I assume this might have impacts on
  existing users. Should we make something else to complement this?
---
 drivers/spi/spi.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 7a0406ec200e..2b2eeaa4c15d 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -902,6 +902,16 @@ static int __spi_add_device(struct spi_device *spi, struct spi_device *parent)
 	for (idx = spi->num_chipselect; idx < SPI_DEVICE_CS_CNT_MAX; idx++)
 		spi_set_chipselect(spi, idx, SPI_INVALID_CS);
 
+	/* Restrict CS logical values to a contiguous sequence */
+	for (idx = 1; idx < spi->num_chipselect; idx++) {
+		if (spi_get_chipselect(spi, idx) != spi_get_chipselect(spi, idx - 1) + 1) {
+			dev_err(dev, "cs%d is not consecutive with cs%d\n",
+				spi_get_chipselect(spi, idx),
+				spi_get_chipselect(spi, idx - 1));
+			return -EINVAL;
+		}
+	}
+
 	/* Set the bus ID string */
 	spi_dev_set_name(spi);
 
-- 
2.34.1

[PATCH v4 5/9] spi: expand device name to include enabled CS lines for multi-CS devices

[Jonathan Santos]

The device name for SPI devices was formatted as <controller>.<cs0>,
always using only the first chip-select index, ignoring the remaining CS
lines.

Change the naming format to <controller>.<cs0>-<csN> for multi-cs
devices so that the range of active chip-selects is reflected in the
device name. Since we are not expecting sparse values for logical CS
values, adopt the range format.

Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com>
---
Changes in v4:
* Opted for the range format, since we decide to limit logical CS values
  to a contiguous sequence.

Changes in v3:
* replaced `int idx = 1` by `unsigned int idx = 1` in the for loop.

Changes in v2:
* New patch.
---
 drivers/spi/spi.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 2b2eeaa4c15d..53d6ab4ce8a3 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -782,6 +782,7 @@ static void spi_dev_set_name(struct spi_device *spi)
 {
 	struct device *dev = &spi->dev;
 	struct fwnode_handle *fwnode = dev_fwnode(dev);
+	u8 num_enabled_cs = hweight32(spi->cs_index_mask);
 
 	if (is_acpi_device_node(fwnode)) {
 		dev_set_name(dev, "spi-%s", acpi_dev_name(to_acpi_device_node(fwnode)));
@@ -793,6 +794,19 @@ static void spi_dev_set_name(struct spi_device *spi)
 		return;
 	}
 
+	/*
+	 * In multi-CS arrangements, use num_enabled_cs (derived from
+	 * cs_index_mask) to determine which CS lines the device is actively
+	 * using. num_chipselect only reflects how many CS lines are assigned
+	 * to the device, not how many are actually enabled.
+	 */
+	if (num_enabled_cs > 1) {
+		dev_set_name(&spi->dev, "%s.%u-%u", dev_name(&spi->controller->dev),
+			     spi_get_chipselect(spi, 0),
+			     spi_get_chipselect(spi, num_enabled_cs - 1));
+		return;
+	}
+
 	dev_set_name(&spi->dev, "%s.%u", dev_name(&spi->controller->dev),
 		     spi_get_chipselect(spi, 0));
 }
-- 
2.34.1

[PATCH v4 6/9] spi: introduce SPI ancillary device with lanes

[Jonathan Santos]

The existing spi_new_ancillary_device() creates an ancillary SPI device
but does not consider the parent's lane map. In multi-device setups where
each sub-device's chip-select is bound to a dedicated data lane, there is
no way to bind an ancillary device to an arbitrary lane index.

Introduce spi_new_ancillary_device_with_lane() and
devm_spi_new_ancillary_device_with_lane(), which accept rx_lane_idx and
tx_lane_idx parameters to select a specific lane from the parent's
rx_lane_map and tx_lane_map respectively. The resulting ancillary device
is registered with a single RX and TX lane, keeping it independent from
the other sub-devices sharing the same controller.

While at it, fix the spi_new_ancillary_device() documentation, which
claimed it returns 0 on success instead of a pointer.

Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com>
---
Changes in v4:
* Don't use extern for the new declarations of the ancillary functions from
  spi.h.

Changes in v3:
* Refactored code using helper function to reduce duplicated code.

Changes in v2:
* New patch.
---
 drivers/spi/spi.c       | 116 +++++++++++++++++++++++++++++++++++-----
 include/linux/spi/spi.h |   8 +++
 2 files changed, 110 insertions(+), 14 deletions(-)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 53d6ab4ce8a3..30103bffad33 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -2918,20 +2918,12 @@ static void of_register_spi_devices(struct spi_controller *ctlr)
 static void of_register_spi_devices(struct spi_controller *ctlr) { }
 #endif
 
-/**
- * spi_new_ancillary_device() - Register ancillary SPI device
- * @spi:         Pointer to the main SPI device registering the ancillary device
- * @chip_select: Chip Select of the ancillary device
- *
- * Register an ancillary SPI device; for example some chips have a chip-select
- * for normal device usage and another one for setup/firmware upload.
- *
- * This may only be called from main SPI device's probe routine.
- *
- * Return: 0 on success; negative errno on failure
- */
-struct spi_device *spi_new_ancillary_device(struct spi_device *spi,
-					     u8 chip_select)
+#define SPI_UNDEFINED_LANE	-1
+
+static struct spi_device *__spi_new_ancillary_device(struct spi_device *spi,
+						     u8 chip_select,
+						     int rx_lane_idx,
+						     int tx_lane_idx)
 {
 	struct spi_controller *ctlr = spi->controller;
 	struct spi_device *ancillary;
@@ -2958,6 +2950,21 @@ struct spi_device *spi_new_ancillary_device(struct spi_device *spi,
 	 */
 	ancillary->cs_index_mask = BIT(0);
 
+	if (rx_lane_idx >= spi->num_rx_lanes || tx_lane_idx >= spi->num_tx_lanes) {
+		rc = -EINVAL;
+		goto err_out;
+	}
+
+	if (rx_lane_idx != SPI_UNDEFINED_LANE) {
+		ancillary->rx_lane_map[0] = spi->rx_lane_map[rx_lane_idx];
+		ancillary->num_rx_lanes = 1;
+	}
+
+	if (tx_lane_idx != SPI_UNDEFINED_LANE) {
+		ancillary->tx_lane_map[0] = spi->tx_lane_map[tx_lane_idx];
+		ancillary->num_tx_lanes = 1;
+	}
+
 	WARN_ON(!mutex_is_locked(&ctlr->add_lock));
 
 	/* Register the new device, passing the parent to skip CS conflict check */
@@ -2973,8 +2980,53 @@ struct spi_device *spi_new_ancillary_device(struct spi_device *spi,
 	spi_dev_put(ancillary);
 	return ERR_PTR(rc);
 }
+
+/**
+ * spi_new_ancillary_device() - Register ancillary SPI device
+ * @spi:         Pointer to the main SPI device registering the ancillary device
+ * @chip_select: Chip Select of the ancillary device
+ *
+ * Register an ancillary SPI device; for example some chips have a chip-select
+ * for normal device usage and another one for setup/firmware upload.
+ *
+ * This may only be called from main SPI device's probe routine.
+ *
+ * Return: Pointer to new ancillary device on success; ERR_PTR on failure
+ */
+struct spi_device *spi_new_ancillary_device(struct spi_device *spi,
+					    u8 chip_select)
+{
+	return __spi_new_ancillary_device(spi, chip_select,
+					  SPI_UNDEFINED_LANE,
+					  SPI_UNDEFINED_LANE);
+}
 EXPORT_SYMBOL_GPL(spi_new_ancillary_device);
 
+/**
+ * spi_new_ancillary_device_with_lane() - Register ancillary SPI device bound to specific lane
+ * @spi:          Pointer to the main SPI device registering the ancillary device
+ * @chip_select:  Chip Select of the ancillary device
+ * @rx_lane_idx:  Lane index within the parent's rx_lane_map
+ * @tx_lane_idx:  Lane index within the parent's tx_lane_map
+ *
+ * Like spi_new_ancillary_device(), but additionally binds the ancillary device
+ * to a single lane from the parent's lane map. Use this in multi-device setups
+ * where each sub-device is physically wired to a dedicated CS and a dedicated
+ * data lane.
+ *
+ * This may only be called from main SPI device's probe routine.
+ *
+ * Return: Pointer to new ancillary device on success; ERR_PTR on failure
+ */
+struct spi_device *spi_new_ancillary_device_with_lane(struct spi_device *spi,
+						      u8 chip_select,
+						      unsigned int rx_lane_idx,
+						      unsigned int tx_lane_idx)
+{
+	return __spi_new_ancillary_device(spi, chip_select, rx_lane_idx, tx_lane_idx);
+}
+EXPORT_SYMBOL_GPL(spi_new_ancillary_device_with_lane);
+
 static void devm_spi_unregister_device(void *spi)
 {
 	spi_unregister_device(spi);
@@ -3015,6 +3067,42 @@ struct spi_device *devm_spi_new_ancillary_device(struct spi_device *spi,
 }
 EXPORT_SYMBOL_GPL(devm_spi_new_ancillary_device);
 
+/**
+ * devm_spi_new_ancillary_device_with_lane() - Register managed ancillary SPI device bound to a lane
+ * @spi:         Pointer to the main SPI device registering the ancillary device
+ * @chip_select: Chip Select of the ancillary device
+ * @rx_lane_idx: Per-device lane index within the parent's rx_lane_map
+ * @tx_lane_idx: Per-device lane index within the parent's tx_lane_map
+ *
+ * Managed version of spi_new_ancillary_device_with_lane(). The ancillary device
+ * will be unregistered automatically when the parent SPI device is unregistered.
+ *
+ * This may only be called from main SPI device's probe routine.
+ *
+ * Return: Pointer to new ancillary device on success; ERR_PTR on failure
+ */
+struct spi_device *devm_spi_new_ancillary_device_with_lane(struct spi_device *spi,
+							   u8 chip_select,
+							   unsigned int rx_lane_idx,
+							   unsigned int tx_lane_idx)
+{
+	struct spi_device *ancillary;
+	int ret;
+
+	ancillary = spi_new_ancillary_device_with_lane(spi, chip_select,
+						       rx_lane_idx, tx_lane_idx);
+	if (IS_ERR(ancillary))
+		return ancillary;
+
+	ret = devm_add_action_or_reset(&spi->dev, devm_spi_unregister_device,
+				       ancillary);
+	if (ret)
+		return ERR_PTR(ret);
+
+	return ancillary;
+}
+EXPORT_SYMBOL_GPL(devm_spi_new_ancillary_device_with_lane);
+
 #ifdef CONFIG_ACPI
 struct acpi_spi_lookup {
 	struct spi_controller 	*ctlr;
diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
index 88d17fce02dc..9bc7fb9ec69a 100644
--- a/include/linux/spi/spi.h
+++ b/include/linux/spi/spi.h
@@ -391,6 +391,14 @@ static inline void spi_unregister_driver(struct spi_driver *sdrv)
 
 extern struct spi_device *spi_new_ancillary_device(struct spi_device *spi, u8 chip_select);
 extern struct spi_device *devm_spi_new_ancillary_device(struct spi_device *spi, u8 chip_select);
+struct spi_device *spi_new_ancillary_device_with_lane(struct spi_device *spi,
+						      u8 chip_select,
+						      unsigned int rx_lane_idx,
+						      unsigned int tx_lane_idx);
+struct spi_device *devm_spi_new_ancillary_device_with_lane(struct spi_device *spi,
+							   u8 chip_select,
+							   unsigned int rx_lane_idx,
+							   unsigned int tx_lane_idx);
 
 /* Use a define to avoid include chaining to get THIS_MODULE */
 #define spi_register_driver(driver) \
-- 
2.34.1

[PATCH v4 7/9] spi: add KUnit tests for the multi-CS framework

[Jonathan Santos]

Add a KUnit suite for the multi-CS feature in the SPI core, covering:
the CS suffix of the device name, considering the contiguous values
restriction, the last-asserted CS tracking for single and multi-CS devices,
and CS array validation rules (overflow, out of range, duplicates,
non-contiguous and descending sequences) and filling of unused slots with
SPI_INVALID_CS, implemented by a test-local spi_validate_cs_config()
helper.

Move SPI_INVALID_CS to internals.h so the tests can use it. Mark
spi_dev_set_name() and spi_is_last_cs() as VISIBLE_IF_KUNIT and expose
them through internals.h.

Place the test in drivers/spi/tests/, next to the existing DMA KUnit
suite, and add CONFIG_SPI_MULTI_CS_KUNIT_TEST to the subsystem
.kunitconfig so it is exercised by default. Since tests/ now hosts more
than one suite, descend into it unconditionally and let tests/Makefile
gate each one on its own Kconfig symbol.

Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com>
---
Changes in v4:
* New patch, as requested to cover the changes in the SPI CS management.
---
 drivers/spi/.kunitconfig               |   1 +
 drivers/spi/Kconfig                    |  11 +
 drivers/spi/Makefile                   |   2 +-
 drivers/spi/internals.h                |  16 ++
 drivers/spi/spi.c                      |  17 +-
 drivers/spi/tests/Makefile             |   3 +-
 drivers/spi/tests/spi-multi-cs-kunit.c | 374 +++++++++++++++++++++++++
 7 files changed, 409 insertions(+), 15 deletions(-)
 create mode 100644 drivers/spi/tests/spi-multi-cs-kunit.c

diff --git a/drivers/spi/.kunitconfig b/drivers/spi/.kunitconfig
index 07fa092c8fd0..eb4c6e693203 100644
--- a/drivers/spi/.kunitconfig
+++ b/drivers/spi/.kunitconfig
@@ -2,3 +2,4 @@ CONFIG_KUNIT=y
 CONFIG_SPI=y
 CONFIG_SPI_MASTER=y
 CONFIG_SPI_DMA_KUNIT_TEST=y
+CONFIG_SPI_MULTI_CS_KUNIT_TEST=y
diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index a0f37e2df716..7077f2e78c0f 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -1383,6 +1383,17 @@ config SPI_DMA_KUNIT_TEST
 
 	  If unsure say N.
 
+config SPI_MULTI_CS_KUNIT_TEST
+	tristate "KUnit tests for SPI multi-CS framework" if !KUNIT_ALL_TESTS
+	depends on KUNIT
+	default KUNIT_ALL_TESTS
+	help
+	  Build KUnit tests for the SPI multi-CS framework. The tests cover
+	  device naming (spi_dev_set_name), last-CS tracking (spi_is_last_cs),
+	  and CS array validation rules.
+
+	  If unsure say N.
+
 config SPI_LOOPBACK_TEST
 	tristate "spi loopback test framework support"
 	depends on m
diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
index a18814fbde93..c549cf471bd2 100644
--- a/drivers/spi/Makefile
+++ b/drivers/spi/Makefile
@@ -13,7 +13,7 @@ obj-$(CONFIG_SPI_MUX)			+= spi-mux.o
 obj-$(CONFIG_SPI_OFFLOAD)		+= spi-offload.o
 obj-$(CONFIG_SPI_SPIDEV)		+= spidev.o
 obj-$(CONFIG_SPI_LOOPBACK_TEST)		+= spi-loopback-test.o
-obj-$(CONFIG_SPI_DMA_KUNIT_TEST)	+= tests/
+obj-y += tests/
 
 # SPI master controller drivers (bus)
 obj-$(CONFIG_SPI_AIROHA_SNFI)		+= spi-airoha-snfi.o
diff --git a/drivers/spi/internals.h b/drivers/spi/internals.h
index c56c190b02e9..ef227d17b33f 100644
--- a/drivers/spi/internals.h
+++ b/drivers/spi/internals.h
@@ -19,6 +19,22 @@
 
 void spi_flush_queue(struct spi_controller *ctrl);
 
+/*
+ * Zero(0) is a valid physical CS value and can be located at any
+ * logical CS in the spi->chip_select[]. If all the physical CS
+ * are initialized to 0 then It would be difficult to differentiate
+ * between a valid physical CS 0 & an unused logical CS whose physical
+ * CS can be 0. As a solution to this issue initialize all the CS to -1.
+ * Now all the unused logical CS will have -1 physical CS value & can be
+ * ignored while performing physical CS validity checks.
+ */
+#define SPI_INVALID_CS		((s8)-1)
+
+#if IS_ENABLED(CONFIG_KUNIT)
+bool spi_is_last_cs(struct spi_device *spi);
+void spi_dev_set_name(struct spi_device *spi);
+#endif
+
 #ifdef CONFIG_HAS_DMA
 #if IS_ENABLED(CONFIG_KUNIT)
 int __spi_map_msg(struct spi_controller *ctlr, struct spi_message *msg);
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 30103bffad33..287e81048063 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -778,7 +778,7 @@ struct spi_device *spi_alloc_device(struct spi_controller *ctlr)
 }
 EXPORT_SYMBOL_GPL(spi_alloc_device);
 
-static void spi_dev_set_name(struct spi_device *spi)
+VISIBLE_IF_KUNIT void spi_dev_set_name(struct spi_device *spi)
 {
 	struct device *dev = &spi->dev;
 	struct fwnode_handle *fwnode = dev_fwnode(dev);
@@ -810,17 +810,7 @@ static void spi_dev_set_name(struct spi_device *spi)
 	dev_set_name(&spi->dev, "%s.%u", dev_name(&spi->controller->dev),
 		     spi_get_chipselect(spi, 0));
 }
-
-/*
- * Zero(0) is a valid physical CS value and can be located at any
- * logical CS in the spi->chip_select[]. If all the physical CS
- * are initialized to 0 then It would be difficult to differentiate
- * between a valid physical CS 0 & an unused logical CS whose physical
- * CS can be 0. As a solution to this issue initialize all the CS to -1.
- * Now all the unused logical CS will have -1 physical CS value & can be
- * ignored while performing physical CS validity checks.
- */
-#define SPI_INVALID_CS		((s8)-1)
+EXPORT_SYMBOL_IF_KUNIT(spi_dev_set_name);
 
 static inline int spi_dev_check_cs(struct device *dev,
 				   struct spi_device *spi, u8 idx,
@@ -1247,7 +1237,7 @@ static void spi_res_release(struct spi_controller *ctlr, struct spi_message *mes
 	for (idx = 0; idx < spi->num_chipselect; idx++)		\
 		if (!(spi->cs_index_mask & BIT(idx))) {} else
 
-static inline bool spi_is_last_cs(struct spi_device *spi)
+VISIBLE_IF_KUNIT bool spi_is_last_cs(struct spi_device *spi)
 {
 	u8 idx;
 	bool last = false;
@@ -1258,6 +1248,7 @@ static inline bool spi_is_last_cs(struct spi_device *spi)
 	}
 	return last;
 }
+EXPORT_SYMBOL_IF_KUNIT(spi_is_last_cs);
 
 static void spi_toggle_csgpiod(struct spi_device *spi, u8 idx, bool enable, bool activate)
 {
diff --git a/drivers/spi/tests/Makefile b/drivers/spi/tests/Makefile
index 26689e0cb28a..0711fdd7ce33 100644
--- a/drivers/spi/tests/Makefile
+++ b/drivers/spi/tests/Makefile
@@ -1,3 +1,4 @@
 # SPDX-License-Identifier: GPL-2.0-only
 
-obj-$(CONFIG_SPI_DMA_KUNIT_TEST) += spi-dma-kunit.o
+obj-$(CONFIG_SPI_DMA_KUNIT_TEST)	+= spi-dma-kunit.o
+obj-$(CONFIG_SPI_MULTI_CS_KUNIT_TEST)	+= spi-multi-cs-kunit.o
diff --git a/drivers/spi/tests/spi-multi-cs-kunit.c b/drivers/spi/tests/spi-multi-cs-kunit.c
new file mode 100644
index 000000000000..314de7dc8606
--- /dev/null
+++ b/drivers/spi/tests/spi-multi-cs-kunit.c
@@ -0,0 +1,374 @@
+// SPDX-License-Identifier: GPL-2.0
+// KUnit tests for the SPI multi-CS framework covering CS suffix formatting in
+// device names, last-asserted CS tracking, and CS array configuration
+// validation.
+
+#include <kunit/test.h>
+#include <kunit/device.h>
+#include <linux/spi/spi.h>
+
+#include "../internals.h"
+
+MODULE_IMPORT_NS("EXPORTED_FOR_KUNIT_TESTING");
+KUNIT_DEFINE_ACTION_WRAPPER(spi_dev_put_wrapper, spi_dev_put,
+			    struct spi_device *);
+
+/*
+ * Allocate an SPI device on a controller with @ctlr_num_cs chip selects,
+ * using the core allocators so both are real, refcounted devices. The
+ * controller is parented to a KUnit device and released through devres
+ * when that device is unregistered at the end of the test. the SPI device
+ * reference is dropped by a deferred action registered after it.
+ */
+static struct spi_device *alloc_spi(struct kunit *test, const u8 *cs,
+				    u8 num_cs, u16 ctlr_num_cs)
+{
+	struct spi_controller *ctlr;
+	struct spi_device *spi;
+	struct device *dev;
+	u8 i;
+
+	dev = kunit_device_register(test, "spi-multi-cs-test");
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, dev);
+
+	ctlr = devm_spi_alloc_host(dev, 0);
+	KUNIT_ASSERT_NOT_NULL(test, ctlr);
+	ctlr->num_chipselect = ctlr_num_cs;
+
+	spi = spi_alloc_device(ctlr);
+	KUNIT_ASSERT_NOT_NULL(test, spi);
+	KUNIT_ASSERT_EQ(test,
+			kunit_add_action_or_reset(test, spi_dev_put_wrapper, spi),
+			0);
+
+	spi->num_chipselect = num_cs;
+	for (i = 0; i < num_cs; i++) {
+		spi_set_chipselect(spi, i, cs[i]);
+		spi->cs_index_mask |= BIT(i);
+	}
+
+	return spi;
+}
+
+/*
+ * Name @spi through spi_dev_set_name() under a controller named "spi0". Both
+ * names are freed by the kobject release when the devices are put.
+ */
+static const char *spi_name(struct kunit *test, struct spi_device *spi)
+{
+	KUNIT_ASSERT_EQ(test, dev_set_name(&spi->controller->dev, "spi0"), 0);
+
+	spi_dev_set_name(spi);
+	KUNIT_ASSERT_NOT_NULL(test, dev_name(&spi->dev));
+
+	return dev_name(&spi->dev);
+}
+
+/* single CS 0 -> "spi0.0". */
+static void dev_name_single_zero(struct kunit *test)
+{
+	const u8 cs[] = { 0 };
+	struct spi_device *spi = alloc_spi(test, cs, ARRAY_SIZE(cs), 8);
+
+	KUNIT_EXPECT_STREQ(test, spi_name(test, spi), "spi0.0");
+}
+
+/* single CS 7 -> "spi0.7". */
+static void dev_name_single_nonzero(struct kunit *test)
+{
+	const u8 cs[] = { 7 };
+	struct spi_device *spi = alloc_spi(test, cs, ARRAY_SIZE(cs), 8);
+
+	KUNIT_EXPECT_STREQ(test, spi_name(test, spi), "spi0.7");
+}
+
+/* two CS 0,1 -> "spi0.0-1". */
+static void dev_name_two_cs(struct kunit *test)
+{
+	const u8 cs[] = { 0, 1 };
+	struct spi_device *spi = alloc_spi(test, cs, ARRAY_SIZE(cs), 8);
+
+	KUNIT_EXPECT_STREQ(test, spi_name(test, spi), "spi0.0-1");
+}
+
+/* four CS 1,2,3,4 -> "spi0.1-4". */
+static void dev_name_four_cs(struct kunit *test)
+{
+	const u8 cs[] = { 1, 2, 3, 4 };
+	struct spi_device *spi = alloc_spi(test, cs, ARRAY_SIZE(cs), 8);
+
+	KUNIT_EXPECT_STREQ(test, spi_name(test, spi), "spi0.1-4");
+}
+
+/*
+ * cs_index_mask only has bit 0 set even though num_chipselect=2:
+ * only the enabled CS is reflected in the name -> "spi0.0".
+ */
+static void dev_name_index_mask_single(struct kunit *test)
+{
+	const u8 cs[] = { 0, 1 };
+	struct spi_device *spi = alloc_spi(test, cs, ARRAY_SIZE(cs), 8);
+
+	spi->cs_index_mask = BIT(0);
+
+	KUNIT_EXPECT_STREQ(test, spi_name(test, spi), "spi0.0");
+}
+
+static struct kunit_case spi_dev_name_cases[] = {
+	KUNIT_CASE(dev_name_single_zero),
+	KUNIT_CASE(dev_name_single_nonzero),
+	KUNIT_CASE(dev_name_two_cs),
+	KUNIT_CASE(dev_name_four_cs),
+	KUNIT_CASE(dev_name_index_mask_single),
+	{}
+};
+
+static struct kunit_suite spi_dev_name_suite = {
+	.name = "spi_dev_name",
+	.test_cases = spi_dev_name_cases,
+};
+
+/* single CS: last_cs[0] matches. */
+static void last_cs_single_match(struct kunit *test)
+{
+	const u8 cs[] = { 3 };
+	struct spi_device *spi = alloc_spi(test, cs, ARRAY_SIZE(cs), 8);
+
+	spi->controller->last_cs[0] = 3;
+
+	KUNIT_EXPECT_TRUE(test, spi_is_last_cs(spi));
+}
+
+/* single CS: last_cs[0] is SPI_INVALID_CS. */
+static void last_cs_single_invalid(struct kunit *test)
+{
+	const u8 cs[] = { 3 };
+	struct spi_device *spi = alloc_spi(test, cs, ARRAY_SIZE(cs), 8);
+
+	spi->controller->last_cs[0] = SPI_INVALID_CS;
+
+	KUNIT_EXPECT_FALSE(test, spi_is_last_cs(spi));
+}
+
+/* single CS: last_cs[0] holds a different physical CS. */
+static void last_cs_single_mismatch(struct kunit *test)
+{
+	const u8 cs[] = { 3 };
+	struct spi_device *spi = alloc_spi(test, cs, ARRAY_SIZE(cs), 8);
+
+	spi->controller->last_cs[0] = 4;
+
+	KUNIT_EXPECT_FALSE(test, spi_is_last_cs(spi));
+}
+
+/* multi-CS: both slots match. */
+static void last_cs_multi_both_match(struct kunit *test)
+{
+	const u8 cs[] = { 0, 1 };
+	struct spi_device *spi = alloc_spi(test, cs, ARRAY_SIZE(cs), 8);
+
+	spi->controller->last_cs[0] = 0;
+	spi->controller->last_cs[1] = 1;
+
+	KUNIT_EXPECT_TRUE(test, spi_is_last_cs(spi));
+}
+
+/* multi-CS: only second slot matches. */
+static void last_cs_multi_partial_match(struct kunit *test)
+{
+	const u8 cs[] = { 0, 1 };
+	struct spi_device *spi = alloc_spi(test, cs, ARRAY_SIZE(cs), 8);
+
+	spi->controller->last_cs[0] = SPI_INVALID_CS;
+	spi->controller->last_cs[1] = 1;
+
+	KUNIT_EXPECT_TRUE(test, spi_is_last_cs(spi));
+}
+
+/* multi-CS: all slots invalid. */
+static void last_cs_multi_all_invalid(struct kunit *test)
+{
+	const u8 cs[] = { 0, 1 };
+	struct spi_device *spi = alloc_spi(test, cs, ARRAY_SIZE(cs), 8);
+
+	spi->controller->last_cs[0] = SPI_INVALID_CS;
+	spi->controller->last_cs[1] = SPI_INVALID_CS;
+
+	KUNIT_EXPECT_FALSE(test, spi_is_last_cs(spi));
+}
+
+/*
+ * cs_index_mask only has bit 0 set even though num_chipselect=2:
+ * slot 1 is skipped by spi_for_each_valid_cs, only slot 0 is checked.
+ * last_cs[0] matches.
+ */
+static void last_cs_index_mask_filters_slots(struct kunit *test)
+{
+	const u8 cs[] = { 0, 1 };
+	struct spi_device *spi = alloc_spi(test, cs, ARRAY_SIZE(cs), 8);
+
+	/* override mask: only slot 0 active. */
+	spi->cs_index_mask = BIT(0);
+	spi->controller->last_cs[0] = 0;
+	spi->controller->last_cs[1] = SPI_INVALID_CS;
+
+	KUNIT_EXPECT_TRUE(test, spi_is_last_cs(spi));
+}
+
+static struct kunit_case spi_is_last_cases[] = {
+	KUNIT_CASE(last_cs_single_match),
+	KUNIT_CASE(last_cs_single_invalid),
+	KUNIT_CASE(last_cs_single_mismatch),
+	KUNIT_CASE(last_cs_multi_both_match),
+	KUNIT_CASE(last_cs_multi_partial_match),
+	KUNIT_CASE(last_cs_multi_all_invalid),
+	KUNIT_CASE(last_cs_index_mask_filters_slots),
+	{}
+};
+
+static struct kunit_suite spi_is_last_suite = {
+	.name = "spi_is_last_cs",
+	.test_cases = spi_is_last_cases,
+};
+
+/*
+ * Validate and normalise the CS configuration of @spi against the controller's
+ * @num_chipselect limit. Arithmetic Analysis only, no device-model calls or bus
+ * scan.
+ *
+ * On success the unused chip_select[] slots are filled with SPI_INVALID_CS.
+ */
+static int spi_validate_cs_config(struct spi_device *spi,
+				  u16 ctlr_num_chipselect)
+{
+	int idx;
+
+	if (spi->num_chipselect > SPI_DEVICE_CS_CNT_MAX)
+		return -EOVERFLOW;
+
+	for (idx = 0; idx < spi->num_chipselect; idx++) {
+		if (spi_get_chipselect(spi, idx) >= ctlr_num_chipselect)
+			return -EINVAL;
+	}
+
+	/* No duplicate physical CS values within the same device. */
+	for (idx = 0; idx < spi->num_chipselect; idx++) {
+		for (int j = idx + 1; j < spi->num_chipselect; j++) {
+			if (spi_get_chipselect(spi, idx) == spi_get_chipselect(spi, j))
+				return -EBUSY;
+		}
+	}
+
+	/* Fill unused slots before the contiguity check modifies chip_select[]. */
+	for (idx = spi->num_chipselect; idx < SPI_DEVICE_CS_CNT_MAX; idx++)
+		spi_set_chipselect(spi, idx, SPI_INVALID_CS);
+
+	/* CS values must form a ascending contiguous sequence. */
+	for (idx = 1; idx < spi->num_chipselect; idx++) {
+		if (spi_get_chipselect(spi, idx) != spi_get_chipselect(spi, idx - 1) + 1)
+			return -EINVAL;
+	}
+
+	return 0;
+}
+
+/* valid single CS within controller range. */
+static void cs_cfg_single_valid(struct kunit *test)
+{
+	const u8 cs[] = { 3 };
+	struct spi_device *spi = alloc_spi(test, cs, ARRAY_SIZE(cs), 8);
+
+	KUNIT_EXPECT_EQ(test, spi_validate_cs_config(spi, 8), 0);
+	/* unused slots filled with SPI_INVALID_CS. */
+	KUNIT_EXPECT_EQ(test, (s8)spi_get_chipselect(spi, 1), SPI_INVALID_CS);
+	KUNIT_EXPECT_EQ(test, (s8)spi_get_chipselect(spi, 2), SPI_INVALID_CS);
+	KUNIT_EXPECT_EQ(test, (s8)spi_get_chipselect(spi, 3), SPI_INVALID_CS);
+}
+
+/* valid multi-CS contiguous range. */
+static void cs_cfg_multi_valid(struct kunit *test)
+{
+	const u8 cs[] = { 1, 2 };
+	struct spi_device *spi = alloc_spi(test, cs, ARRAY_SIZE(cs), 8);
+
+	KUNIT_EXPECT_EQ(test, spi_validate_cs_config(spi, 8), 0);
+	KUNIT_EXPECT_EQ(test, (s8)spi_get_chipselect(spi, 2), SPI_INVALID_CS);
+	KUNIT_EXPECT_EQ(test, (s8)spi_get_chipselect(spi, 3), SPI_INVALID_CS);
+}
+
+/* num_chipselect > SPI_DEVICE_CS_CNT_MAX. */
+static void cs_cfg_overflow(struct kunit *test)
+{
+	struct spi_device *spi = alloc_spi(test, NULL, 0, 8);
+
+	spi->num_chipselect = SPI_DEVICE_CS_CNT_MAX + 1;
+
+	KUNIT_EXPECT_EQ(test, spi_validate_cs_config(spi, 8), -EOVERFLOW);
+}
+
+/* physical CS >= controller num_chipselect. */
+static void cs_cfg_out_of_range(struct kunit *test)
+{
+	const u8 cs[] = { 8 };
+	struct spi_device *spi = alloc_spi(test, cs, ARRAY_SIZE(cs), 8);
+
+	KUNIT_EXPECT_EQ(test, spi_validate_cs_config(spi, 8), -EINVAL);
+}
+
+/* duplicate physical CS values. */
+static void cs_cfg_duplicate(struct kunit *test)
+{
+	const u8 cs[] = { 2, 2 };
+	struct spi_device *spi = alloc_spi(test, cs, ARRAY_SIZE(cs), 8);
+
+	KUNIT_EXPECT_EQ(test, spi_validate_cs_config(spi, 8), -EBUSY);
+}
+
+/* non-contiguous CS (gap). */
+static void cs_cfg_gap(struct kunit *test)
+{
+	const u8 cs[] = { 0, 2 };
+	struct spi_device *spi = alloc_spi(test, cs, ARRAY_SIZE(cs), 8);
+
+	KUNIT_EXPECT_EQ(test, spi_validate_cs_config(spi, 8), -EINVAL);
+}
+
+/* descending CS values. */
+static void cs_cfg_descending(struct kunit *test)
+{
+	const u8 cs[] = { 3, 2 };
+	struct spi_device *spi = alloc_spi(test, cs, ARRAY_SIZE(cs), 8);
+
+	KUNIT_EXPECT_EQ(test, spi_validate_cs_config(spi, 8), -EINVAL);
+}
+
+/* max 4 CS, all valid and contiguous. */
+static void cs_cfg_max_cs(struct kunit *test)
+{
+	const u8 cs[] = { 0, 1, 2, 3 };
+	struct spi_device *spi = alloc_spi(test, cs, ARRAY_SIZE(cs), 8);
+
+	KUNIT_EXPECT_EQ(test, spi_validate_cs_config(spi, 8), 0);
+}
+
+static struct kunit_case spi_cs_config_cases[] = {
+	KUNIT_CASE(cs_cfg_single_valid),
+	KUNIT_CASE(cs_cfg_multi_valid),
+	KUNIT_CASE(cs_cfg_overflow),
+	KUNIT_CASE(cs_cfg_out_of_range),
+	KUNIT_CASE(cs_cfg_duplicate),
+	KUNIT_CASE(cs_cfg_gap),
+	KUNIT_CASE(cs_cfg_descending),
+	KUNIT_CASE(cs_cfg_max_cs),
+	{}
+};
+
+static struct kunit_suite spi_cs_config_suite = {
+	.name = "spi_cs_config",
+	.test_cases = spi_cs_config_cases,
+};
+
+kunit_test_suites(&spi_dev_name_suite, &spi_is_last_suite, &spi_cs_config_suite);
+
+MODULE_DESCRIPTION("KUnit tests for SPI multi-CS framework");
+MODULE_LICENSE("GPL");
-- 
2.34.1

[PATCH v4 8/9] spi: spi-engine-ex: Add support for multi-CS devices

[Jonathan Santos]

The SPI Engine controller only handled the first chip select when
asserting CS lines and configuring CS polarity inversion, ignoring
additional CS indices present in cs_index_mask.

Update spi_engine_gen_cs() to iterate over all set bits in
cs_index_mask and toggle each corresponding CS line in the assert mask.
Update spi_engine_setup() likewise so that SPI_CS_HIGH polarity is
applied to every active CS index rather than only index 0.

Set SPI_CONTROLLER_MULTI_CS in the controller flags to reflect the
capability to the SPI core.

Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com>
---
Changes in v4:
* Added the spi_engine_get_chipselect_bits() helper to reduce duplicated
  code.

Changes in v3:
* None.

Changes in v2:
* Adaptation of the patch
  "spi: spi-engine-ex: Add support for multi-CS devices" from v1,
  discarding the per transfer CS mask and keeping the Multi-CS support
  only.
---
 drivers/spi/spi-axi-spi-engine.c | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/drivers/spi/spi-axi-spi-engine.c b/drivers/spi/spi-axi-spi-engine.c
index 02bbc5d0cfc5..e4c40059e583 100644
--- a/drivers/spi/spi-axi-spi-engine.c
+++ b/drivers/spi/spi-axi-spi-engine.c
@@ -278,13 +278,25 @@ static void spi_engine_gen_sleep(struct spi_engine_program *p, bool dry,
 	}
 }
 
+static unsigned long spi_engine_get_chipselect_bits(struct spi_device *spi,
+						    unsigned long index_mask)
+{
+	unsigned long bits = 0;
+	unsigned int idx;
+
+	for_each_set_bit(idx, &index_mask, SPI_DEVICE_CS_CNT_MAX)
+		bits |= BIT(spi_get_chipselect(spi, idx));
+
+	return bits;
+}
+
 static void spi_engine_gen_cs(struct spi_engine_program *p, bool dry,
 		struct spi_device *spi, bool assert)
 {
 	unsigned int mask = 0xff;
 
 	if (assert)
-		mask ^= BIT(spi_get_chipselect(spi, 0));
+		mask ^= spi_engine_get_chipselect_bits(spi, spi->cs_index_mask);
 
 	spi_engine_program_add_cmd(p, dry, SPI_ENGINE_CMD_ASSERT(0, mask));
 }
@@ -886,12 +898,14 @@ static int spi_engine_setup(struct spi_device *device)
 {
 	struct spi_controller *host = device->controller;
 	struct spi_engine *spi_engine = spi_controller_get_devdata(host);
+	unsigned long cs_bits;
 	unsigned int reg;
 
+	cs_bits = spi_engine_get_chipselect_bits(device, device->cs_index_mask);
 	if (device->mode & SPI_CS_HIGH)
-		spi_engine->cs_inv |= BIT(spi_get_chipselect(device, 0));
+		spi_engine->cs_inv |= cs_bits;
 	else
-		spi_engine->cs_inv &= ~BIT(spi_get_chipselect(device, 0));
+		spi_engine->cs_inv &= ~cs_bits;
 
 	writel_relaxed(SPI_ENGINE_CMD_SYNC(0),
 		       spi_engine->base + SPI_ENGINE_REG_CMD_FIFO);
@@ -1222,6 +1236,7 @@ static int spi_engine_probe(struct platform_device *pdev)
 	host->unoptimize_message = spi_engine_unoptimize_message;
 	host->get_offload = spi_engine_get_offload;
 	host->put_offload = spi_engine_put_offload;
+	host->flags |= SPI_CONTROLLER_MULTI_CS;
 	host->num_chipselect = 8;
 
 	if (adi_axi_pcore_ver_gteq(version, 1, 2)) {
-- 
2.34.1

[PATCH v4 9/9] iio: adc: ad7768-1: add support for multiple chip aggregation

[Jonathan Santos]

The AD7768-1 family is a single-channel ADC, but it is designed to allow
connecting multiple devices to the same SPI controller, each on a
dedicated CS and data lane, clocked synchronously, and sharing SDO and
SCLK. The number of aggregated devices is derived from the number of
CS.

The DRDY pins are combined to trigger the data interrupt when all
devices are ready, and since the synchronization pins are tied, they
stay in synchrony.

To reflect the multidevice setup, IIO channels are dynamically set
based on the number of devices. Restrict buffered capture to the
all-channels scan mask, since all devices sample in lockstep. Register
an ancillary SPI device per lane for individual direct reads, and enable
SPI_MULTI_LANE_MODE_STRIPE in offload mode to interleave samples from all
lanes into the DMA stream.

Since all devices must be in sync, all configurations that affect the
sampling rate are unified, so they always have the same sampling
frequency.

Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com>
---
Changes in v4:
* Fixed the st->offload_xfer.len calculation to account for multiple 
  devices.
* The number of aggregated devices is now defined by the number of CS
  instead of number of lanes.
* Used devm_bitmap_zalloc() and bitmap_set() for the masks allocation
  and definition, as requested.

Changes in v3:
* Modified channel mask allocation on ad7768_parse_aaf_gain() to get 
  a static value, Since we assume the family is single channel. Added a 
  comment explaining.

Changes in v2:
* New patch.
---
 drivers/iio/adc/ad7768-1.c | 100 +++++++++++++++++++++++++++++++++----
 1 file changed, 90 insertions(+), 10 deletions(-)

diff --git a/drivers/iio/adc/ad7768-1.c b/drivers/iio/adc/ad7768-1.c
index 3972821c295a..064b4b394a2e 100644
--- a/drivers/iio/adc/ad7768-1.c
+++ b/drivers/iio/adc/ad7768-1.c
@@ -6,6 +6,7 @@
  */
 #include <linux/array_size.h>
 #include <linux/bitfield.h>
+#include <linux/bitmap.h>
 #include <linux/cleanup.h>
 #include <linux/clk.h>
 #include <linux/completion.h>
@@ -20,6 +21,7 @@
 #include <linux/minmax.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
+#include <linux/overflow.h>
 #include <linux/rational.h>
 #include <linux/regmap.h>
 #include <linux/regulator/consumer.h>
@@ -120,7 +122,8 @@
 
 #define AD7768_TRIGGER_SOURCE_SYNC_IDX 0
 
-#define AD7768_MAX_CHANNELS 1
+#define AD7768_MAX_CHANNELS	1
+#define AD7768_MAX_AGGR_DEVICES	4
 
 #define ADAQ7768_PGA_PINS 3
 
@@ -300,10 +303,12 @@ struct ad7768_chip_info {
 
 struct ad7768_state {
 	struct spi_device *spi;
+	struct spi_device *spi_anc[AD7768_MAX_AGGR_DEVICES];
 	struct spi_offload *offload;
 	struct spi_offload_trigger *offload_trigger;
 	struct regmap *regmap;
 	struct regmap *regmap24;
+	struct regmap *regmap24_anc[AD7768_MAX_AGGR_DEVICES];
 	int vref_uv;
 	struct regulator_dev *vcm_rdev;
 	unsigned int vcm_output_sel;
@@ -322,11 +327,12 @@ struct ad7768_state {
 	struct gpio_descs *pga_gpios;
 	struct gpio_desc *gpio_sync_in;
 	struct gpio_desc *gpio_reset;
-	const char *labels[AD7768_MAX_CHANNELS];
+	const char *labels[AD7768_MAX_CHANNELS * AD7768_MAX_AGGR_DEVICES];
 	struct gpio_chip gpiochip;
 	struct spi_transfer offload_xfer;
 	struct spi_message offload_msg;
 	const struct ad7768_chip_info *chip;
+	u8 num_devices;
 	bool en_spi_sync;
 	struct mutex pga_lock; /* protect device internal state (PGA) */
 	/*
@@ -335,7 +341,7 @@ struct ad7768_state {
 	 */
 	union {
 		struct {
-			__be32 chan;
+			__be32 chan[AD7768_MAX_AGGR_DEVICES];
 			aligned_s64 timestamp;
 		} scan;
 		__be32 d32;
@@ -478,9 +484,10 @@ static int ad7768_set_mode(struct ad7768_state *st,
 				 AD7768_CONV_MODE_MSK, AD7768_CONV_MODE(mode));
 }
 
-static int ad7768_scan_direct(struct iio_dev *indio_dev)
+static int ad7768_scan_direct(struct iio_dev *indio_dev, unsigned int chan)
 {
 	struct ad7768_state *st = iio_priv(indio_dev);
+	struct regmap *regmap;
 	int readval, ret;
 
 	reinit_completion(&st->completion);
@@ -492,7 +499,8 @@ static int ad7768_scan_direct(struct iio_dev *indio_dev)
 	if (!ret)
 		return -ETIMEDOUT;
 
-	ret = regmap_read(st->regmap24, AD7768_REG24_ADC_DATA, &readval);
+	regmap = st->num_devices > 1 ? st->regmap24_anc[chan] : st->regmap24;
+	ret = regmap_read(regmap, AD7768_REG24_ADC_DATA, &readval);
 	if (ret)
 		return ret;
 
@@ -989,7 +997,7 @@ static int ad7768_read_raw(struct iio_dev *indio_dev,
 		if (!iio_device_claim_direct(indio_dev))
 			return -EBUSY;
 
-		ret = ad7768_scan_direct(indio_dev);
+		ret = ad7768_scan_direct(indio_dev, chan->channel);
 
 		iio_device_release_direct(indio_dev);
 		if (ret < 0)
@@ -1388,9 +1396,11 @@ static int ad7768_offload_buffer_postenable(struct iio_dev *indio_dev)
 	if (IS_ERR(scan_type))
 		return PTR_ERR(scan_type);
 
-	st->offload_xfer.len = spi_bpw_to_bytes(scan_type->realbits);
+	st->offload_xfer.len = spi_bpw_to_bytes(scan_type->realbits) * st->num_devices;
 	st->offload_xfer.bits_per_word = scan_type->realbits;
 	st->offload_xfer.offload_flags = SPI_OFFLOAD_XFER_RX_STREAM;
+	if (st->num_devices > 1)
+		st->offload_xfer.multi_lane_mode = SPI_MULTI_LANE_MODE_STRIPE;
 
 	spi_message_init_with_transfers(&st->offload_msg, &st->offload_xfer, 1);
 	st->offload_msg.offload = st->offload;
@@ -1694,6 +1704,63 @@ static int ad7768_parse_aaf_gain(struct device *dev, struct ad7768_state *st)
 	return 0;
 }
 
+static int ad7768_probe_multidevices(struct iio_dev *indio_dev)
+{
+	struct ad7768_state *st = iio_priv(indio_dev);
+	struct device *dev = indio_dev->dev.parent;
+	struct iio_chan_spec *channels;
+	unsigned int mask_longs;
+	unsigned long *masks;
+	size_t num_chans;
+	unsigned int i;
+	u8 cs;
+
+	num_chans = size_mul(st->num_devices, st->chip->num_channels);
+	channels = devm_kcalloc(dev, num_chans, sizeof(*channels), GFP_KERNEL);
+	if (!channels)
+		return -ENOMEM;
+
+	indio_dev->num_channels = num_chans;
+	for (i = 0; i < num_chans; i++) {
+		struct iio_chan_spec *chan = &channels[i];
+
+		*chan = *st->chip->channel_spec;
+		chan->channel = i;
+		chan->scan_index = i;
+	}
+
+	indio_dev->channels = channels;
+
+	/*
+	 * One scan mask with all devices enabled, plus a zero terminator. Each
+	 * mask spans BITS_TO_LONGS(masklength) longs.
+	 */
+	mask_longs = BITS_TO_LONGS(indio_dev->num_channels);
+	masks = devm_bitmap_zalloc(dev, 2 * mask_longs * BITS_PER_LONG, GFP_KERNEL);
+	if (!masks)
+		return -ENOMEM;
+
+	bitmap_set(masks, 0, num_chans);
+	indio_dev->available_scan_masks = masks;
+
+	/* Setup ancillary SPI devices for single device access */
+	for (i = 0; i < st->num_devices; i++) {
+		cs = spi_get_chipselect(st->spi, i);
+		st->spi_anc[i] = devm_spi_new_ancillary_device_with_lane(st->spi,
+									 cs, i, 0);
+		if (IS_ERR(st->spi_anc[i]))
+			return dev_err_probe(dev, PTR_ERR(st->spi_anc[i]),
+					     "failed to register ancillary device\n");
+
+		st->regmap24_anc[i] = devm_regmap_init_spi(st->spi_anc[i],
+							   &ad7768_regmap24_config);
+		if (IS_ERR(st->regmap24_anc[i]))
+			return PTR_ERR(st->regmap24_anc[i]);
+	}
+
+	return 0;
+}
+
 static bool ad7768_offload_trigger_match(struct spi_offload_trigger *trigger,
 					 enum spi_offload_trigger_type type,
 					 u64 *args, u32 nargs)
@@ -1833,6 +1900,12 @@ static int ad7768_probe(struct spi_device *spi)
 		return -ENODATA;
 
 	st->spi = spi;
+	/* Infer the number of devices from the number of CS */
+	st->num_devices = spi->num_chipselect;
+	if (st->num_devices > AD7768_MAX_AGGR_DEVICES)
+		return dev_err_probe(&spi->dev, -EINVAL,
+				     "Too many devices (%u), max %d supported\n",
+				     st->num_devices, AD7768_MAX_AGGR_DEVICES);
 
 	st->regmap = devm_regmap_init_spi(spi, &ad7768_regmap_config);
 	if (IS_ERR(st->regmap))
@@ -1856,11 +1929,18 @@ static int ad7768_probe(struct spi_device *spi)
 
 	st->mclk_freq = clk_get_rate(st->mclk);
 
-	indio_dev->channels = st->chip->channel_spec;
-	indio_dev->num_channels = st->chip->num_channels;
 	indio_dev->name = st->chip->name;
 	indio_dev->info = &ad7768_info;
 	indio_dev->modes = INDIO_DIRECT_MODE;
+	if (st->num_devices > 1) {
+		ret = ad7768_probe_multidevices(indio_dev);
+		if (ret)
+			return dev_err_probe(&spi->dev, ret,
+					     "Failed to configure multidevice\n");
+	} else {
+		indio_dev->channels = st->chip->channel_spec;
+		indio_dev->num_channels = st->chip->num_channels;
+	}
 
 	/* Register VCM output regulator */
 	if (st->chip->has_vcm_regulator) {
@@ -1892,7 +1972,7 @@ static int ad7768_probe(struct spi_device *spi)
 			return ret;
 	}
 
-	ret = ad7768_set_channel_label(indio_dev, st->chip->num_channels);
+	ret = ad7768_set_channel_label(indio_dev, st->num_devices);
 	if (ret)
 		return ret;
 
-- 
2.34.1

Re: [PATCH v4 7/9] spi: add KUnit tests for the multi-CS framework

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] spi: tests: spi_cs_config suite only tests local spi_validate_cs_config()
- [Medium] spi: tests: last_cs_index_mask_filters_slots() does not test mask filtering
--

commit c0274670e3ec066a2b9eb903dcadd09d2e349d14
Author: Jonathan Santos <Jonathan.Santos@analog.com>

spi: add KUnit tests for the multi-CS framework

This commit adds KUnit test suites for the SPI multi-CS framework covering
device naming, last-asserted CS tracking, and CS array validation. It also
exposes SPI_INVALID_CS, spi_dev_set_name(), and spi_is_last_cs() in
drivers/spi/internals.h for KUnit testing.

> diff --git a/drivers/spi/tests/spi-multi-cs-kunit.c b/drivers/spi/tests/spi-multi-cs-kunit.c
> new file mode 100644
> index 0000000000000..314de7dc86064
> --- /dev/null
> +++ b/drivers/spi/tests/spi-multi-cs-kunit.c

[ ... ]

> +/*
> + * cs_index_mask only has bit 0 set even though num_chipselect=2:
> + * slot 1 is skipped by spi_for_each_valid_cs, only slot 0 is checked.
> + * last_cs[0] matches.
> + */
> +static void last_cs_index_mask_filters_slots(struct kunit *test)
> +{
> +	const u8 cs[] = { 0, 1 };
> +	struct spi_device *spi = alloc_spi(test, cs, ARRAY_SIZE(cs), 8);
> +
> +	/* override mask: only slot 0 active. */
> +	spi->cs_index_mask = BIT(0);
> +	spi->controller->last_cs[0] = 0;
> +	spi->controller->last_cs[1] = SPI_INVALID_CS;
> +
> +	KUNIT_EXPECT_TRUE(test, spi_is_last_cs(spi));
> +}

[Severity: Medium]
In drivers/spi/spi.c, spi_is_last_cs() sets last = true whenever any active
slot matches, and never clears last on a non-matching slot:

drivers/spi/spi.c:spi_is_last_cs() {
	u8 idx;
	bool last = false;

	spi_for_each_valid_cs(spi, idx) {
		if (spi->controller->last_cs[idx] == spi_get_chipselect(spi, idx))
			last = true;
	}
	return last;
}

In last_cs_index_mask_filters_slots(), slot 0 is set to match
(last_cs[0] = 0 with cs[0] = 0) and slot 1 is set not to match
(last_cs[1] = SPI_INVALID_CS with cs[1] = 1) while cs_index_mask is BIT(0).
Because slot 0 matches, spi_is_last_cs() will return true even if
cs_index_mask is ignored and slot 1 is also evaluated.

To verify that cs_index_mask actually filters out inactive slots, shouldn't
this test set the active slot 0 to a non-matching value (such as
SPI_INVALID_CS), set the masked-out slot 1 to match (last_cs[1] = 1), and
expect spi_is_last_cs(spi) to return false?

[ ... ]

> +/*
> + * Validate and normalise the CS configuration of @spi against the controller's
> + * @num_chipselect limit. Arithmetic Analysis only, no device-model calls or bus
> + * scan.
> + *
> + * On success the unused chip_select[] slots are filled with SPI_INVALID_CS.
> + */
> +static int spi_validate_cs_config(struct spi_device *spi,
> +				  u16 ctlr_num_chipselect)
> +{
> +	int idx;
> +
> +	if (spi->num_chipselect > SPI_DEVICE_CS_CNT_MAX)
> +		return -EOVERFLOW;
> +
> +	for (idx = 0; idx < spi->num_chipselect; idx++) {
> +		if (spi_get_chipselect(spi, idx) >= ctlr_num_chipselect)
> +			return -EINVAL;
> +	}
> +
> +	/* No duplicate physical CS values within the same device. */
> +	for (idx = 0; idx < spi->num_chipselect; idx++) {
> +		for (int j = idx + 1; j < spi->num_chipselect; j++) {
> +			if (spi_get_chipselect(spi, idx) == spi_get_chipselect(spi, j))
> +				return -EBUSY;
> +		}
> +	}
> +
> +	/* Fill unused slots before the contiguity check modifies chip_select[]. */
> +	for (idx = spi->num_chipselect; idx < SPI_DEVICE_CS_CNT_MAX; idx++)
> +		spi_set_chipselect(spi, idx, SPI_INVALID_CS);
> +
> +	/* CS values must form a ascending contiguous sequence. */
> +	for (idx = 1; idx < spi->num_chipselect; idx++) {
> +		if (spi_get_chipselect(spi, idx) != spi_get_chipselect(spi, idx - 1) + 1)
> +			return -EINVAL;
> +	}
> +
> +	return 0;
> +}

[Severity: Medium]
All of the test cases in spi_cs_config_suite (cs_cfg_single_valid(),
cs_cfg_multi_valid(), cs_cfg_overflow(), cs_cfg_out_of_range(),
cs_cfg_duplicate(), cs_cfg_gap(), cs_cfg_descending(), and cs_cfg_max_cs())
call this test-local spi_validate_cs_config() helper rather than exercising
the SPI core validation logic in __spi_add_device() in drivers/spi/spi.c:

drivers/spi/spi.c:__spi_add_device() {
	...
	if (spi->num_chipselect > SPI_DEVICE_CS_CNT_MAX) {
		dev_err(dev, "num_cs %d > max %d\n", spi->num_chipselect,
			SPI_DEVICE_CS_CNT_MAX);
		return -EOVERFLOW;
	}
	...
	/* Restrict CS logical values to a contiguous sequence */
	for (idx = 1; idx < spi->num_chipselect; idx++) {
		if (spi_get_chipselect(spi, idx) != spi_get_chipselect(spi, idx - 1) + 1) {
			...
			return -EINVAL;
		}
	}
	...
}

In addition, alloc_spi() initializes ctlr->num_chipselect = ctlr_num_cs,
which is never read because spi_validate_cs_config() takes a separate
ctlr_num_chipselect argument instead of checking
spi->controller->num_chipselect.

Could the CS validation logic in __spi_add_device() be factored out into a
helper in drivers/spi/spi.c and exposed via VISIBLE_IF_KUNIT and
EXPORT_SYMBOL_IF_KUNIT in drivers/spi/internals.h so that spi_cs_config_suite
tests the actual SPI core implementation?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1792435248.git.Jonathan.Santos@analog.com?part=7

Re: [PATCH v4 2/9] dt-bindings: iio: adc: ad7768-1: Document multidevice support

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] dt-bindings: iio: adc: ad7768-1: missing schema constraints on spi-rx-bus-width
--

commit 6ac23a15419b8807e67f7b54e0a33ca605e1e6c8
Author: Jonathan Santos <Jonathan.Santos@analog.com>

dt-bindings: iio: adc: ad7768-1: Document multidevice support

This commit updates the adi,ad7768-1 device tree binding to support
aggregating up to four devices on a single spi controller using multiple
reg entries, spi-rx-bus-width, and spi-cs-composite.

> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml
> index 7b72d14e104c2..64c1cbeaa5762 100644
> --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml
> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml
> @@ -26,7 +26,10 @@ properties:
>        - adi,adaq7769-1
>  
>    reg:
> -    maxItems: 1
> +    minItems: 1
> +    maxItems: 4
> +    description:
> +      Chip select number. For multi-device setups, one entry per device.
>  
>    clocks:
>      maxItems: 1
> @@ -109,6 +112,12 @@ properties:
>    reset-gpios:
>      maxItems: 1
>  
> +  spi-rx-bus-width:
> +    description:
> +      Bus width per CS. For multi-device setups (multiple reg entries), one
> +      entry per device must be provided. Required when reg has more than
> +      one entry.
> +
>    spi-cpol: true
>  
>    spi-cpha: true
> @@ -203,6 +212,16 @@ allOf:
>        properties:
>          pga-gpios: false
>  
> +  # Required properties for multi-device setups
> +  - if:
> +      properties:
> +        reg:
> +          minItems: 2
> +    then:
> +      required:
> +        - spi-rx-bus-width
> +        - spi-cs-composite
> +
>  unevaluatedProperties: false

[Severity: Low]
In Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml,
properties:spi-rx-bus-width only adds a description without constraining
minItems, maxItems, or the per-item value. Because this binding references
spi-peripheral-props.yaml, spi-rx-bus-width inherits an unbounded
uint32-array allowing values from [0, 1, 2, 4, 8] even though each ad7768-1
device uses a single 1-bit data lane and at most four devices can be
aggregated.

Also, in the allOf conditional block for reg with minItems: 2, the then
branch marks spi-rx-bus-width and spi-cs-composite as required, but does
not require minItems: 2 on spi-rx-bus-width or disallow spi-cs-composite
when reg has only one entry.

If a device tree node for adi,ad7768-1 specifies multiple reg entries with
only a single spi-rx-bus-width entry, or specifies unsupported bus widths
or more than four lanes, it will pass dt_binding_check and then fail at
probe time in devm_spi_new_ancillary_device_with_lane() when rx_lane_idx >=
spi->num_rx_lanes.

Should properties:spi-rx-bus-width specify minItems: 1, maxItems: 4, and
items: const: 1 (similar to
Documentation/devicetree/bindings/iio/adc/adi,ad7380.yaml), and should the
allOf entry enforce minItems: 2 on spi-rx-bus-width when reg has
minItems: 2?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1792435248.git.Jonathan.Santos@analog.com?part=2

Re: [PATCH v4 4/9] spi: restrict CS logical values to a contiguous sequence

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] spi: __spi_add_device() contiguous CS check breaks non-consecutive multi-CS
--

commit fa819c18d6f789f30ff9ec769f0b81bb8ee9ca8a
Author: Jonathan Santos <Jonathan.Santos@analog.com>

spi: restrict CS logical values to a contiguous sequence

This commit adds a check in __spi_add_device() requiring chip-select values
for multi-CS SPI devices to form a consecutive ascending sequence, returning
-EINVAL otherwise.

> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> index 7a0406ec200e1..2b2eeaa4c15d4 100644
> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -902,6 +902,16 @@ static int __spi_add_device(struct spi_device *spi, struct spi_device *parent)
>  	for (idx = spi->num_chipselect; idx < SPI_DEVICE_CS_CNT_MAX; idx++)
>  		spi_set_chipselect(spi, idx, SPI_INVALID_CS);
>  
> +	/* Restrict CS logical values to a contiguous sequence */
> +	for (idx = 1; idx < spi->num_chipselect; idx++) {
> +		if (spi_get_chipselect(spi, idx) != spi_get_chipselect(spi, idx - 1) + 1) {
> +			dev_err(dev, "cs%d is not consecutive with cs%d\n",
> +				spi_get_chipselect(spi, idx),
> +				spi_get_chipselect(spi, idx - 1));
> +			return -EINVAL;
> +		}
> +	}
> +

[Severity: High]
In drivers/spi/spi.c, idx (0 .. spi->num_chipselect - 1) is the logical CS
index, whereas spi_get_chipselect(spi, idx) returns spi->chip_select[idx],
which is the physical controller CS number mapped to that logical index, as
noted above SPI_INVALID_CS:

/*
 * Zero(0) is a valid physical CS value and can be located at any
 * logical CS in the spi->chip_select[].

During DeviceTree registration, of_register_spi_devices() ->
of_register_spi_device() -> of_spi_parse_dt() populates spi->chip_select[]
from the DT reg array before calling spi_add_device() -> __spi_add_device():

	spi->num_chipselect = rc;
	for (idx = 0; idx < rc; idx++)
		spi_set_chipselect(spi, idx, cs[idx]);

By checking spi_get_chipselect(spi, idx) != spi_get_chipselect(spi, idx - 1)
+ 1, __spi_add_device() requires the physical CS numbers in
spi->chip_select[] to be strictly consecutive and ascending.

For example, arch/arm/boot/dts/nxp/imx/imx53-ppd.dts defines spidev0 with
descending physical chip selects:

	spidev0: spi@1 {
		compatible = "ge,achc", "nxp,kinetis-k20";
		reg = <1>, <0>;
		vdd-supply = <&reg_3v3>;
		vdda-supply = <&reg_3v3>;
		clocks = <&achc_24M>;
		reset-gpios = <&gpio3 6 GPIO_ACTIVE_LOW>;
	};

Per Documentation/devicetree/bindings/misc/ge-achc.yaml, reg entry 0 is the
control interface (on CS 1) and reg entry 1 is the firmware programming
interface (on CS 0), which drivers/misc/gehc-achc.c registers in
gehc_achc_probe() via spi_new_ancillary_device():

	ret = of_property_read_u32_index(spi->dev.of_node, "reg", 1, &ezport_reg);
	if (ret)
		return dev_err_probe(&spi->dev, ret, "missing second reg entry!\n");

	achc->ezport = spi_new_ancillary_device(spi, ezport_reg);

With this check in __spi_add_device(), spi_get_chipselect(spi, 1) is 0 while
spi_get_chipselect(spi, 0) + 1 is 2, causing __spi_add_device() to fail with
-EINVAL before gehc_achc_probe() can run.

Won't this cause a regression for imx53-ppd.dts as well as any other
multi-CS SPI peripheral wired to non-consecutive or descending physical
controller chip-select lines (such as reg = <1>, <0> or reg = <0>, <2>)?

>  	/* Set the bus ID string */
>  	spi_dev_set_name(spi);
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1792435248.git.Jonathan.Santos@analog.com?part=4

Re: [PATCH v4 9/9] iio: adc: ad7768-1: add support for multiple chip aggregation

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] iio: adc: ad7768-1: incomplete multi-device read in ad7768_trigger_handler()
--

commit 093b4a79a74e19badb41a08df17fd753e9cf37ec
Author: Jonathan Santos <Jonathan.Santos@analog.com>

iio: adc: ad7768-1: add support for multiple chip aggregation

This commit adds support for aggregating multiple ad7768-1 devices connected
to the same spi controller using dedicated chip selects and data lanes. It
dynamically configures the channels, registers per-lane ancillary devices for
direct reads, and enables striped multi-lane transfers in offload mode.

> diff --git a/drivers/iio/adc/ad7768-1.c b/drivers/iio/adc/ad7768-1.c
> index 3972821c295a1..667c09326f92f 100644
> --- a/drivers/iio/adc/ad7768-1.c
> +++ b/drivers/iio/adc/ad7768-1.c
[ ... ]
> @@ -335,7 +341,7 @@ struct ad7768_state {
>  	 */
>  	union {
>  		struct {
> -			__be32 chan;
> +			__be32 chan[AD7768_MAX_AGGR_DEVICES];
>  			aligned_s64 timestamp;
>  		} scan;
>  		__be32 d32;

[Severity: High]
While struct ad7768_state expands st->data.scan.chan[] for non-offload
triggered buffer capture, does ad7768_trigger_handler() in
drivers/iio/adc/ad7768-1.c also need to be updated when multiple devices are
aggregated?

When st->num_devices > 1 and spi offload is not present, ad7768_probe() still
falls back to registering the triggered buffer path via
ad7768_triggered_buffer_alloc():

drivers/iio/adc/ad7768-1.c:ad7768_probe() {
	...
	st->offload = devm_spi_offload_get(dev, spi, &ad7768_spi_offload_config);
	ret = PTR_ERR_OR_ZERO(st->offload);
	if (ret == -ENODEV) {
		/* If not using SPI offload, fall back to low speed usage */
		ret = ad7768_triggered_buffer_alloc(indio_dev);
	...
}

However, when buffered capture is enabled and the interrupt fires,
ad7768_trigger_handler() still performs a single-lane, single-sample read:

drivers/iio/adc/ad7768-1.c:ad7768_trigger_handler() {
	...
	ret = spi_read(st->spi, &st->data.scan.chan,
		       BITS_TO_BYTES(scan_type->realbits));
	if (ret < 0)
		goto out;

	iio_push_to_buffers_with_ts(indio_dev, &st->data.scan,
				    sizeof(st->data.scan),
				    iio_get_time_ns(indio_dev));
	...
}

Because spi_read() only reads a single sample of scan_type->realbits bits on
one lane into the start of st->data.scan.chan[0] while asserting all chip
selects on st->spi, the samples from devices 1 through st->num_devices - 1
are clocked out and lost on the bus, pushing unpopulated channel data to the
buffer.

Furthermore, with multiple channels enabled, a 3-byte read in 24-bit mode
(realbits = 24, storagebits = 32) or a 2-byte read in 16-bit mode
(st->oversampling_ratio == 8, realbits = 16, storagebits = 16) does not match
the 4-byte __be32 element stride of st->data.scan.chan[].

Should ad7768_trigger_handler() be updated to read and pack all
st->num_devices channels according to scan_type->storagebits, or is
non-offload buffered capture unsupported when st->num_devices > 1?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1792435248.git.Jonathan.Santos@analog.com?part=9