devicetree.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/4] spi: Introduce spi bus extensions
@ 2025-07-29  9:50 Ayush Singh
  2025-07-29  9:51 ` [PATCH 1/4] spi: Follow spi-parent when retrieving a controller from node Ayush Singh
                   ` (3 more replies)
  0 siblings, 4 replies; 12+ messages in thread
From: Ayush Singh @ 2025-07-29  9:50 UTC (permalink / raw)
  To: Mark Brown, herve.codina, luca.ceresoli, conor+dt, Jason Kridner,
	Deepak Khatri, Dhruva Gole, Robert Nelson, Andrew Davis,
	Rob Herring, Krzysztof Kozlowski
  Cc: linux-spi, linux-kernel, devicetree, Ayush Singh

This patch series is basically an SPI version of i2c-bus-extension patch
series [0].

The big picture behind this RFC series is to support a Linux device
with a connector to physically add and remove an add-on to/from the
main device to augment its features at runtime, adding devices on
non-discoverable busses, using device tree overlays.

The related big picture has been already presented in
  - the 'Add support for GE SUNH hot-pluggable connector' series [0]
  - the 'Runtime hotplug on non-discoverable busses with device tree
    overlays' talk at Linux Plumbers Conference 2024 [1].

This series focuses on the SPI related part.

An spi bus is wired to the connector and allows an add-on board to
connect additional spi devices to this bus.

When device tree nodes are added, the SPI core tries to probe client
devices based on the classic DT structure:

  spi@abcd0000 {
      some-client@42 { compatible = "xyz,blah"; ... };
  };

However for hotplug connectors described via device tree overlays [1]
there is additional level of indirection, which is needed to decouple
the overlay and the base tree:

  --- base device tree ---

  spi1: spi@abcd0000 {
      compatible = "xyz,spi-ctrl";
      spi-bus-extension@0 {
          spi-bus = <&spi_ctrl>;
      };
      ...
  };

  spi5: spi@cafe0000 {
      compatible = "xyz,spi-ctrl";
      spi-bus-extension@0 {
          spi-bus = <&spi-sensors>;
      };
      ...
  };

  connector {
      spi_ctrl: spi-ctrl {
          spi-parent = <&spi1>;
          #address-cells = <1>;
          #size-cells = <0>;
      };

      spi-sensors {
          spi-parent = <&spi5>;
          #address-cells = <1>;
          #size-cells = <0>;
      };
  };

  --- device tree overlay ---

  ...
  // This node will overlay on the spi-ctrl node of the base tree
  spi-ctrl {
      device@1 { compatible = "xyz,foo"; ... };
  };
  ...

  --- resulting device tree ---

  spi1: spi@abcd0000 {
      compatible = "xyz,spi-ctrl";
      spi-bus-extension@0 {
          spi-bus = <&spi_ctrl>;
      };
      ...
  };

  spi5: spi@cafe0000 {
      compatible = "xyz,spi-ctrl";
      spi-bus-extension@0 {
          spi-bus = <&spi-sensors>;
      };
      ...
  };

  connector {
      spi-ctrl {
          spi-parent = <&spi1>;
          #address-cells = <1>;
          #size-cells = <0>;

          device@1 { compatible = "xyz,foo"; ... };
      };

      spi-sensors {
          spi-parent = <&spi5>;
          #address-cells = <1>;
          #size-cells = <0>;
      };
  };

Here spi-ctrl (same goes for spi-sensors) represent the part of SPI bus
that is on the hot-pluggable add-on. On hot-plugging it will physically
connect to the SPI adapter on the base board. Let's call the 'spi-ctrl'
node an "extension node".

In order to decouple the overlay from the base tree, the SPI adapter
(spi@abcd0000) and the extension node (spi-ctrl) are separate nodes.
Rightfully, only the former will probe into an SPI adapter, and it will
do that perhaps during boot, long before overlay insertion or after the
overlay has been inserted for instance if the SPI adapter is remove and
re-probed for whatever reason after the overlay insertion.

The extension node won't probe into an SPI adapter or any other device
or bus, so its subnodes ('device@1') won't be interpreted as SPI
clients by current SPI core code.

The extension node is linked to the adapter node in two ways. The first
one with the spi-bus-extension adapter sub-node and the second one with
the spi-parent property in the extension node itself.

The purpose of those two links is to handle device probing in several
cases.

- First case: Adapter already probed when add-on devices are added

When devices are added by the overlay, an OF change notification is
triggered so that busses can support those new devices.

Going from a newly added device node, the spi-parent property allows to
find the corresponding SPI adapter and register the new SPI client with
this adapter.

The patch 1 in this series proposes modification to handle this case
and, by the nature of the modification, all cases where a phandle refers
an extension node instead of the adapter node itself.

- Second case: Add-on devices already present in device-tree when
  adapter is probed

In this case, everything is already described in the device-tree and
then the adapter is probed.

OF change notifications don't play a role in this case either because
they were never triggered (the overlay was applied by the bootloader)
or they were triggered before the adapter is probed and so were
missed/ignored.

The adapter probe process registers device already described at the
adapter node level (current code) and, thanks to spi-bus-extension
adapter sub-node and its spi-bus property, it can also follow the
extension and registers devices described in those extension nodes.

The patch 2 and 3 in this series proposes modifications to handle this
case.

The patch 4 provides the device-tree bindings for spi-bus-extension and
spi-parent.

I also have a prototype driver with addon-board overlays to see how
combining all the relevant patches looks for support Beagle capes for
PocketBeagle 2 [3]. The tree can be found here [4]. To be more specific,
the base board overlay can be found here [5] and the addon board
(Techlab Cape [6]) overlay can be found here [7].

Best Regards,
Ayush Singh

[0]: https://lore.kernel.org/all/20250205173918.600037-1-herve.codina@bootlin.com/
[1]: https://lore.kernel.org/all/20240917-hotplug-drm-bridge-v4-0-bc4dfee61be6@bootlin.com/
[2]: https://lpc.events/event/18/contributions/1696/
[3]: https://www.beagleboard.org/boards/pocketbeagle-2
[4]: https://github.com/Ayush1325/linux/commits/beagle-cape-v1/
[5]: https://github.com/Ayush1325/BeagleBoard-DeviceTrees/commit/0d919e3fca9bc134b8593db16d1da9d73bdb794f
[6]: https://www.beagleboard.org/boards/techlab
[7]: https://github.com/Ayush1325/linux/commit/7a6728e35b4b82e94c24a0a9464ab849b8485812

Signed-off-by: Ayush Singh <ayush@beagleboard.org>
---
Ayush Singh (4):
      spi: Follow spi-parent when retrieving a controller from node
      spi: Move children registration in a dedicated function
      spi: Handle spi bus extension
      devicetree: bindings: spi: Introduce SPI bus extensions

 .../devicetree/bindings/spi/spi-controller.yaml    | 66 +++++++++++++++++++++-
 drivers/spi/spi.c                                  | 63 +++++++++++++++++----
 2 files changed, 116 insertions(+), 13 deletions(-)
---
base-commit: d7af19298454ed155f5cf67201a70f5cf836c842
change-id: 20250728-spi-bus-extension-121de5d93b47

Best regards,
-- 
Ayush Singh <ayush@beagleboard.org>


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

* [PATCH 1/4] spi: Follow spi-parent when retrieving a controller from node
  2025-07-29  9:50 [PATCH 0/4] spi: Introduce spi bus extensions Ayush Singh
@ 2025-07-29  9:51 ` Ayush Singh
  2025-07-29  9:51 ` [PATCH 2/4] spi: Move children registration in a dedicated function Ayush Singh
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 12+ messages in thread
From: Ayush Singh @ 2025-07-29  9:51 UTC (permalink / raw)
  To: Mark Brown, herve.codina, luca.ceresoli, conor+dt, Jason Kridner,
	Deepak Khatri, Dhruva Gole, Robert Nelson, Andrew Davis,
	Rob Herring, Krzysztof Kozlowski
  Cc: linux-spi, linux-kernel, devicetree, Ayush Singh

spi bus extension were introduced to decouple spi busses when they are
wired to connectors. Combined with devicetree overlays, they introduce
an additional level of indirection, which is needed to decouple the
overlay (describing the hardware available on addon baord) and the base
tree (describing resources provided to the addon board).

For instance, the following devicetree fragment, available once
overlays are applied, is legit:

```
spi1: spi@abcd0000 {
    compatible = "xyz,spi-ctrl";
    spi-bus-extension@0 {
        reg = <0>;
        spi-bus = <&spi-ctrl>;
    };
    ...
};

connector {
    spi-ctrl {
        spi-parent = <&spi1>;
        #address-cells = <1>;
        #size-cells = <0>;

        spi-bus-extension@0 {
            reg = <0>;
            spi-bus = <&spi-other-connector>;
        };

        device@1 {
            compatible = "xyz,foo";
            reg = <1>;
        };
    };

    devices {
        other-connector {
            spi-at-other-connector {
                spi-parent = <&spi-ctrl>;
                #address-cells = <1>;
                #size-cells = <0>;

                device@2 {
                   compatible = "xyz,bar";
                   reg = <2>;
                };
            };
        };
    };
};
```

Current implementation of of_find_spi_controller_by_node() supposes that
the node parameter correspond to the adapter.

This assumption is no more valid with spi bus extensions. Indeed, the
node parameter can reference an spi bus extension node and not the
related adapter.

In the example, spi-ctrl and spi-at-other-connector nodes are chained
bus extensions and those node can be passed to
of_find_spi_controller_by_node() in order to get the related adapter (i.e
the adapter handling the bus and its extensions: spi@abcd0000).

Extend of_find_spi_controller_by_node() to perform the walking from the
given node through spi-parent references up to the adapter.

Signed-off-by: Ayush Singh <ayush@beagleboard.org>
---
 drivers/spi/spi.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index a388f372b27a7f29d18f1dd5e862902811016fc6..0030e0be0d9b2f9e2b0c4a1d806b42bdb4ecb5d2 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -4776,11 +4776,18 @@ static struct spi_device *of_find_spi_device_by_node(struct device_node *node)
 /* The spi controllers are not using spi_bus, so we find it with another way */
 static struct spi_controller *of_find_spi_controller_by_node(struct device_node *node)
 {
+	struct device_node *ctlr_node = node;
 	struct device *dev;
 
-	dev = class_find_device_by_of_node(&spi_controller_class, node);
+	while (of_property_present(ctlr_node, "spi-parent")) {
+		ctlr_node = of_parse_phandle(ctlr_node, "spi-parent", 0);
+		if (!ctlr_node)
+			return NULL;
+	}
+
+	dev = class_find_device_by_of_node(&spi_controller_class, ctlr_node);
 	if (!dev && IS_ENABLED(CONFIG_SPI_SLAVE))
-		dev = class_find_device_by_of_node(&spi_target_class, node);
+		dev = class_find_device_by_of_node(&spi_target_class, ctlr_node);
 	if (!dev)
 		return NULL;
 

-- 
2.50.1


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

* [PATCH 2/4] spi: Move children registration in a dedicated function
  2025-07-29  9:50 [PATCH 0/4] spi: Introduce spi bus extensions Ayush Singh
  2025-07-29  9:51 ` [PATCH 1/4] spi: Follow spi-parent when retrieving a controller from node Ayush Singh
@ 2025-07-29  9:51 ` Ayush Singh
  2025-07-29  9:51 ` [PATCH 3/4] spi: Handle spi bus extension Ayush Singh
  2025-07-29  9:51 ` [PATCH 4/4] devicetree: bindings: spi: Introduce SPI bus extensions Ayush Singh
  3 siblings, 0 replies; 12+ messages in thread
From: Ayush Singh @ 2025-07-29  9:51 UTC (permalink / raw)
  To: Mark Brown, herve.codina, luca.ceresoli, conor+dt, Jason Kridner,
	Deepak Khatri, Dhruva Gole, Robert Nelson, Andrew Davis,
	Rob Herring, Krzysztof Kozlowski
  Cc: linux-spi, linux-kernel, devicetree, Ayush Singh

of_register_spi_devices() perform the loop for registering child devices.

In order to prepare the support for spi bus extensions, extract this
registration loop and move it in a dedicated function.

Signed-off-by: Ayush Singh <ayush@beagleboard.org>
---
 drivers/spi/spi.c | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 0030e0be0d9b2f9e2b0c4a1d806b42bdb4ecb5d2..ea271e37c72d3dc099c5147ec404050ee0bbf046 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -2524,21 +2524,16 @@ of_register_spi_device(struct spi_controller *ctlr, struct device_node *nc)
 	return ERR_PTR(rc);
 }
 
-/**
- * of_register_spi_devices() - Register child devices onto the SPI bus
- * @ctlr:	Pointer to spi_controller device
- *
- * Registers an spi_device for each child node of controller node which
- * represents a valid SPI target device.
- */
-static void of_register_spi_devices(struct spi_controller *ctlr)
+static void of_register_spi_children(struct spi_controller *ctlr,
+				     struct device_node *node)
 {
 	struct spi_device *spi;
 	struct device_node *nc;
 
-	for_each_available_child_of_node(ctlr->dev.of_node, nc) {
+	for_each_available_child_of_node(node, nc) {
 		if (of_node_test_and_set_flag(nc, OF_POPULATED))
 			continue;
+
 		spi = of_register_spi_device(ctlr, nc);
 		if (IS_ERR(spi)) {
 			dev_warn(&ctlr->dev,
@@ -2547,6 +2542,18 @@ static void of_register_spi_devices(struct spi_controller *ctlr)
 		}
 	}
 }
+
+/**
+ * of_register_spi_devices() - Register child devices onto the SPI bus
+ * @ctlr:	Pointer to spi_controller device
+ *
+ * Registers an spi_device for each child node of controller node which
+ * represents a valid SPI target device.
+ */
+static void of_register_spi_devices(struct spi_controller *ctlr)
+{
+	of_register_spi_children(ctlr, ctlr->dev.of_node);
+}
 #else
 static void of_register_spi_devices(struct spi_controller *ctlr) { }
 #endif

-- 
2.50.1


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

* [PATCH 3/4] spi: Handle spi bus extension
  2025-07-29  9:50 [PATCH 0/4] spi: Introduce spi bus extensions Ayush Singh
  2025-07-29  9:51 ` [PATCH 1/4] spi: Follow spi-parent when retrieving a controller from node Ayush Singh
  2025-07-29  9:51 ` [PATCH 2/4] spi: Move children registration in a dedicated function Ayush Singh
@ 2025-07-29  9:51 ` Ayush Singh
  2025-07-29 12:46   ` Krzysztof Kozlowski
  2025-07-29  9:51 ` [PATCH 4/4] devicetree: bindings: spi: Introduce SPI bus extensions Ayush Singh
  3 siblings, 1 reply; 12+ messages in thread
From: Ayush Singh @ 2025-07-29  9:51 UTC (permalink / raw)
  To: Mark Brown, herve.codina, luca.ceresoli, conor+dt, Jason Kridner,
	Deepak Khatri, Dhruva Gole, Robert Nelson, Andrew Davis,
	Rob Herring, Krzysztof Kozlowski
  Cc: linux-spi, linux-kernel, devicetree, Ayush Singh

spi bus extensions were introduced to decouple spi busses when they are
wired to connectors. Combined with devicetree overlays, they introduce
an additional level of indirection, which is needed to decouple the
overlay (describing the hardware available on addon baord) and the base
tree (describing resources provided to the addon board).

For instance, the following devicetree fragment, available once
overlays are applied, is legit:

```
spi1: spi@abcd0000 {
    compatible = "xyz,spi-ctrl";
    spi-bus-extension@0 {
        reg = <0>;
        spi-bus = <&spi-ctrl>;
    };
    ...
};

connector {
    spi-ctrl {
        spi-parent = <&spi1>;
        #address-cells = <1>;
        #size-cells = <0>;

        spi-bus-extension@0 {
            reg = <0>;
            spi-bus = <&spi-other-connector>;
        };

        device@1 {
            compatible = "xyz,foo";
            reg = <1>;
        };
    };

    devices {
        other-connector {
            spi-at-other-connector {
                spi-parent = <&spi-ctrl>;
                #address-cells = <1>;
                #size-cells = <0>;

                device@2 {
                   compatible = "xyz,bar";
                   reg = <2>;
                };
            };
        };
    };
};
```

Current processing done when a spi adapter is registered registers
spi clients described at the adapter node level.

With spi bus extensions, the process needs to look also at
extensions to register devices described in those extensions and so
connected to the adapter.

Extend of_spi_register_children() to look recursively at those
spi bus extensions.

Signed-off-by: Ayush Singh <ayush@beagleboard.org>
---
 drivers/spi/spi.c | 27 ++++++++++++++++++++++++++-
 1 file changed, 26 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index ea271e37c72d3dc099c5147ec404050ee0bbf046..015f86c6f3228a8746dc517112d466051b50e3db 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -2528,9 +2528,17 @@ static void of_register_spi_children(struct spi_controller *ctlr,
 				     struct device_node *node)
 {
 	struct spi_device *spi;
-	struct device_node *nc;
+	struct device_node *nc, *extension;
 
+	/*
+	 * Register device directly described in this bus node before looking
+	 * at extensions.
+	 */
 	for_each_available_child_of_node(node, nc) {
+		/* Filter out extension node */
+		if (of_node_name_eq(nc, "spi-bus-extension"))
+			continue;
+
 		if (of_node_test_and_set_flag(nc, OF_POPULATED))
 			continue;
 
@@ -2541,6 +2549,23 @@ static void of_register_spi_children(struct spi_controller *ctlr,
 			of_node_clear_flag(nc, OF_POPULATED);
 		}
 	}
+
+	/* Look at extensions */
+	for_each_available_child_of_node(node, nc) {
+		if (!of_node_name_eq(nc, "spi-bus-extension"))
+			continue;
+
+		extension = of_parse_phandle(nc, "spi-bus", 0);
+		if (!extension)
+			continue;
+
+		/*
+		 * Register children available at this extension possibly
+		 * walking other chained extensions.
+		 */
+		of_register_spi_children(ctlr, extension);
+		of_node_put(extension);
+	}
 }
 
 /**

-- 
2.50.1


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

* [PATCH 4/4] devicetree: bindings: spi: Introduce SPI bus extensions
  2025-07-29  9:50 [PATCH 0/4] spi: Introduce spi bus extensions Ayush Singh
                   ` (2 preceding siblings ...)
  2025-07-29  9:51 ` [PATCH 3/4] spi: Handle spi bus extension Ayush Singh
@ 2025-07-29  9:51 ` Ayush Singh
  2025-07-29 12:52   ` Krzysztof Kozlowski
  2025-07-29 13:12   ` Rob Herring (Arm)
  3 siblings, 2 replies; 12+ messages in thread
From: Ayush Singh @ 2025-07-29  9:51 UTC (permalink / raw)
  To: Mark Brown, herve.codina, luca.ceresoli, conor+dt, Jason Kridner,
	Deepak Khatri, Dhruva Gole, Robert Nelson, Andrew Davis,
	Rob Herring, Krzysztof Kozlowski
  Cc: linux-spi, linux-kernel, devicetree, Ayush Singh

An SPI bus can be wired to the connector and allows an add-on board to
connect additional SPI devices to this bus.

Those additional SPI devices could be described as sub-nodes of the SPI
bus controller node however for hotplug connectors described via device
tree overlays there is additional level of indirection, which is needed
to decouple the overlay and the base tree:

  --- base device tree ---

  spi1: spi@abcd0000 {
      compatible = "xyz,foo";
      spi-bus-extension@0 {
          spi-bus = <&spi_ctrl>;
      };
      ...
  };

  spi5: spi@cafe0000 {
      compatible = "xyz,bar";
      spi-bus-extension@0 {
          spi-bus = <&spi_sensors>;
      };
      ...
  };

  connector {
      spi_ctrl: spi-ctrl {
          spi-parent = <&spi1>;
          #address-cells = <1>;
          #size-cells = <0>;
      };

      spi_sensors: spi-sensors {
          spi-parent = <&spi5>;
          #address-cells = <1>;
          #size-cells = <0>;
      };
  };

  --- device tree overlay ---

  ...
  // This node will overlay on the spi-ctrl node of the base tree
  spi-ctrl {
      eeprom@50 { compatible = "atmel,24c64"; ... };
  };
  ...

  --- resulting device tree ---

  spi1: spi@abcd0000 {
      compatible = "xyz,foo";
      spi-bus-extension@0 {
          spi-bus = <&spi_ctrl>;
      };
      ...
  };

  spi5: spi@cafe0000 {
      compatible = "xyz,bar";
      spi-bus-extension@0 {
          spi-bus = <&spi_sensors>;
      };
      ...
  };

  connector {
      spi_ctrl: spi-ctrl {
          spi-parent = <&spi1>;
          #address-cells = <1>;
          #size-cells = <0>;

          device@1 { compatible = "xyz,foo"; ... };
      };

      spi_sensors: spi-sensors {
          spi-parent = <&spi5>;
          #address-cells = <1>;
          #size-cells = <0>;
      };
  };

Here spi-ctrl (same goes for spi-sensors) represent the part of SPI bus
that is on the hot-pluggable add-on. On hot-plugging it will physically
connect to the SPI adapter on the base board. Let's call the 'spi-ctrl'
node an "extension node".

In order to decouple the overlay from the base tree, the SPI adapter
(spi@abcd0000) and the extension node (spi-ctrl) are separate nodes.

The extension node is linked to the SPI bus controller in two ways. The
first one with the spi-bus-extension available in SPI controller
sub-node and the second one with the spi-parent property available in
the extension node itself.

The purpose of those two links is to provide the link in both direction
from the SPI controller to the SPI extension and from the SPI extension
to the SPI controller.

Signed-off-by: Ayush Singh <ayush@beagleboard.org>
---
 .../devicetree/bindings/spi/spi-controller.yaml    | 66 +++++++++++++++++++++-
 1 file changed, 65 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/spi/spi-controller.yaml b/Documentation/devicetree/bindings/spi/spi-controller.yaml
index 82d051f7bd6e09dab9809c85ff13475d2b118efd..9b44ce4542f9552c94cb0658ffe3f6d3f29bc434 100644
--- a/Documentation/devicetree/bindings/spi/spi-controller.yaml
+++ b/Documentation/devicetree/bindings/spi/spi-controller.yaml
@@ -25,6 +25,13 @@ properties:
   "#size-cells":
     const: 0
 
+  spi-parent:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description:
+      In case of an SPI bus extension, reference to the SPI bus controller
+      this extension is connected to. In other word, reference the SPI bus
+      controller on the fixed side that drives the bus extension.
+
   cs-gpios:
     description: |
       GPIOs used as chip selects.
@@ -111,7 +118,26 @@ properties:
       - compatible
 
 patternProperties:
-  "^.*@[0-9a-f]+$":
+  'spi-bus-extension@[0-9a-f]+$':
+    type: object
+    description:
+      An SPI bus extension connected to an SPI bus. Those extensions allow to
+      decouple SPI busses when they are wired to connectors.
+
+    properties:
+      reg:
+        maxItems: 1
+
+      spi-bus:
+        $ref: /schemas/types.yaml#/definitions/phandle
+        description:
+          Reference to the extension bus.
+
+    required:
+      - reg
+      - spi-bus
+
+  "^(?!spi-bus-extension@).*@[0-9a-f]+$":
     type: object
     $ref: spi-peripheral-props.yaml
     additionalProperties: true
@@ -214,3 +240,41 @@ examples:
             spi-cs-high;
         };
     };
+
+  # SPI bus extension example involving an SPI bus controller and a connector.
+  #
+  #  +--------------+     +-------------+     +-------------+
+  #  | spi@abcd0000 |     |  Connector  |     | Addon board |
+  #  |    (spi1)    +-----+ (spi-addon) +-----+ (device@10) |
+  #  |              |     |             |     |             |
+  #  +--------------+     +-------------+     +-------------+
+  #
+  # The spi1 SPI bus is wired from a SPI controller to a connector. It is
+  # identified at connector level as spi-addon bus.
+  # An addon board can be connected to this connector and connects a device
+  # (device@10) to this spi-addon extension bus.
+  - |
+    spi1: spi@abcd0000 {
+        compatible = "brcm,bcm2835-spi";
+        reg = <0xabcd0000 0x100>;
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        spi-bus-extension@0 {
+            reg = <0>;
+            spi-bus = <&spi_addon>;
+        };
+    };
+
+    connector {
+        spi_addon: spi-addon {
+            spi-parent = <&spi1>;
+            #address-cells = <1>;
+            #size-cells = <0>;
+
+            device@2 {
+                compatible = "xyz,foo";
+                reg = <0x02>;
+            };
+        };
+    };

-- 
2.50.1


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

* Re: [PATCH 3/4] spi: Handle spi bus extension
  2025-07-29  9:51 ` [PATCH 3/4] spi: Handle spi bus extension Ayush Singh
@ 2025-07-29 12:46   ` Krzysztof Kozlowski
  2025-07-29 12:52     ` Ayush Singh
  0 siblings, 1 reply; 12+ messages in thread
From: Krzysztof Kozlowski @ 2025-07-29 12:46 UTC (permalink / raw)
  To: Ayush Singh, Mark Brown, herve.codina, luca.ceresoli, conor+dt,
	Jason Kridner, Deepak Khatri, Dhruva Gole, Robert Nelson,
	Andrew Davis, Rob Herring, Krzysztof Kozlowski
  Cc: linux-spi, linux-kernel, devicetree

On 29/07/2025 11:51, Ayush Singh wrote:
>  	for_each_available_child_of_node(node, nc) {
> +		/* Filter out extension node */
> +		if (of_node_name_eq(nc, "spi-bus-extension"))
> +			continue;
> +
>  		if (of_node_test_and_set_flag(nc, OF_POPULATED))
>  			continue;
>  
> @@ -2541,6 +2549,23 @@ static void of_register_spi_children(struct spi_controller *ctlr,
>  			of_node_clear_flag(nc, OF_POPULATED);
>  		}
>  	}
> +
> +	/* Look at extensions */
> +	for_each_available_child_of_node(node, nc) {
> +		if (!of_node_name_eq(nc, "spi-bus-extension"))

Where did you document the new ABI? There is no DT bindings patch with it.


Best regards,
Krzysztof

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

* Re: [PATCH 4/4] devicetree: bindings: spi: Introduce SPI bus extensions
  2025-07-29  9:51 ` [PATCH 4/4] devicetree: bindings: spi: Introduce SPI bus extensions Ayush Singh
@ 2025-07-29 12:52   ` Krzysztof Kozlowski
  2025-07-30 15:45     ` Luca Ceresoli
  2025-07-29 13:12   ` Rob Herring (Arm)
  1 sibling, 1 reply; 12+ messages in thread
From: Krzysztof Kozlowski @ 2025-07-29 12:52 UTC (permalink / raw)
  To: Ayush Singh, Mark Brown, herve.codina, luca.ceresoli, conor+dt,
	Jason Kridner, Deepak Khatri, Dhruva Gole, Robert Nelson,
	Andrew Davis, Rob Herring, Krzysztof Kozlowski
  Cc: linux-spi, linux-kernel, devicetree

On 29/07/2025 11:51, Ayush Singh wrote:
> An SPI bus can be wired to the connector and allows an add-on board to
> connect additional SPI devices to this bus.
> 

... so I found the binding. Not marked by my filters due to non-standard
subject.

Please use subject prefixes matching the subsystem. You can get them for
example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
your patch is touching. For bindings, the preferred subjects are
explained here:
https://www.kernel.org/doc/html/latest/devicetree/bindings/submitting-patches.html#i-for-patch-submitters

> Those additional SPI devices could be described as sub-nodes of the SPI
> bus controller node however for hotplug connectors described via device
> tree overlays there is additional level of indirection, which is needed
> to decouple the overlay and the base tree:
> 
>   --- base device tree ---
> 
>   spi1: spi@abcd0000 {
>       compatible = "xyz,foo";
>       spi-bus-extension@0 {
>           spi-bus = <&spi_ctrl>;
>       };
>       ...
>   };
> 
>   spi5: spi@cafe0000 {
>       compatible = "xyz,bar";
>       spi-bus-extension@0 {
>           spi-bus = <&spi_sensors>;
>       };
>       ...
>   };
> 
>   connector {
>       spi_ctrl: spi-ctrl {
>           spi-parent = <&spi1>;
>           #address-cells = <1>;
>           #size-cells = <0>;
>       };
> 
>       spi_sensors: spi-sensors {
>           spi-parent = <&spi5>;
>           #address-cells = <1>;
>           #size-cells = <0>;
>       };
>   };

It looks like you are re-doing I2C work. Please wait till I2C discussion
finishes, so we won't have to comment on the same in multiple places.

> 
>   --- device tree overlay ---
> 
>   ...
>   // This node will overlay on the spi-ctrl node of the base tree
>   spi-ctrl {
>       eeprom@50 { compatible = "atmel,24c64"; ... };
>   };
>   ...
> 
>   --- resulting device tree ---
> 
>   spi1: spi@abcd0000 {
>       compatible = "xyz,foo";
>       spi-bus-extension@0 {
>           spi-bus = <&spi_ctrl>;
>       };
>       ...
>   };
> 
>   spi5: spi@cafe0000 {
>       compatible = "xyz,bar";
>       spi-bus-extension@0 {
>           spi-bus = <&spi_sensors>;
>       };
>       ...
>   };
> 
>   connector {
>       spi_ctrl: spi-ctrl {
>           spi-parent = <&spi1>;
>           #address-cells = <1>;
>           #size-cells = <0>;
> 
>           device@1 { compatible = "xyz,foo"; ... };
>       };
> 
>       spi_sensors: spi-sensors {
>           spi-parent = <&spi5>;
>           #address-cells = <1>;
>           #size-cells = <0>;
>       };
>   };
> 
> Here spi-ctrl (same goes for spi-sensors) represent the part of SPI bus
> that is on the hot-pluggable add-on. On hot-plugging it will physically
> connect to the SPI adapter on the base board. Let's call the 'spi-ctrl'
> node an "extension node".
> 
> In order to decouple the overlay from the base tree, the SPI adapter
> (spi@abcd0000) and the extension node (spi-ctrl) are separate nodes.
> 
> The extension node is linked to the SPI bus controller in two ways. The
> first one with the spi-bus-extension available in SPI controller
> sub-node and the second one with the spi-parent property available in
> the extension node itself.
> 
> The purpose of those two links is to provide the link in both direction
> from the SPI controller to the SPI extension and from the SPI extension
> to the SPI controller.
> 
> Signed-off-by: Ayush Singh <ayush@beagleboard.org>
> ---
>  .../devicetree/bindings/spi/spi-controller.yaml    | 66 +++++++++++++++++++++-
>  1 file changed, 65 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/spi/spi-controller.yaml b/Documentation/devicetree/bindings/spi/spi-controller.yaml
> index 82d051f7bd6e09dab9809c85ff13475d2b118efd..9b44ce4542f9552c94cb0658ffe3f6d3f29bc434 100644
> --- a/Documentation/devicetree/bindings/spi/spi-controller.yaml
> +++ b/Documentation/devicetree/bindings/spi/spi-controller.yaml
> @@ -25,6 +25,13 @@ properties:
>    "#size-cells":
>      const: 0
>  
> +  spi-parent:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description:
> +      In case of an SPI bus extension, reference to the SPI bus controller
> +      this extension is connected to. In other word, reference the SPI bus
> +      controller on the fixed side that drives the bus extension.
> +
>    cs-gpios:
>      description: |
>        GPIOs used as chip selects.
> @@ -111,7 +118,26 @@ properties:
>        - compatible
>  
>  patternProperties:
> -  "^.*@[0-9a-f]+$":
> +  'spi-bus-extension@[0-9a-f]+$':
> +    type: object
> +    description:
> +      An SPI bus extension connected to an SPI bus. Those extensions allow to
> +      decouple SPI busses when they are wired to connectors.

I really do not get why you need separate two-way phandles for marking
parent child relationship. IOW, if you need two way, then why not graphs?

Or why not just making the device@2 a child of SPI, since it is coming
from overlay.


Best regards,
Krzysztof

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

* Re: [PATCH 3/4] spi: Handle spi bus extension
  2025-07-29 12:46   ` Krzysztof Kozlowski
@ 2025-07-29 12:52     ` Ayush Singh
  2025-07-29 13:09       ` Krzysztof Kozlowski
  0 siblings, 1 reply; 12+ messages in thread
From: Ayush Singh @ 2025-07-29 12:52 UTC (permalink / raw)
  To: Krzysztof Kozlowski, Mark Brown, herve.codina, luca.ceresoli,
	conor+dt, Jason Kridner, Deepak Khatri, Dhruva Gole,
	Robert Nelson, Andrew Davis, Rob Herring, Krzysztof Kozlowski
  Cc: linux-spi, linux-kernel, devicetree

On 7/29/25 18:16, Krzysztof Kozlowski wrote:

> On 29/07/2025 11:51, Ayush Singh wrote:
>>   	for_each_available_child_of_node(node, nc) {
>> +		/* Filter out extension node */
>> +		if (of_node_name_eq(nc, "spi-bus-extension"))
>> +			continue;
>> +
>>   		if (of_node_test_and_set_flag(nc, OF_POPULATED))
>>   			continue;
>>   
>> @@ -2541,6 +2549,23 @@ static void of_register_spi_children(struct spi_controller *ctlr,
>>   			of_node_clear_flag(nc, OF_POPULATED);
>>   		}
>>   	}
>> +
>> +	/* Look at extensions */
>> +	for_each_available_child_of_node(node, nc) {
>> +		if (!of_node_name_eq(nc, "spi-bus-extension"))
> Where did you document the new ABI? There is no DT bindings patch with it.

Patch 4 is the dt bindings patch. I will reorder the patches in any 
future to make the dt bindings patch 1.

Here is a direct link in case it got lost in mail: 
https://lore.kernel.org/all/20250729-spi-bus-extension-v1-4-b20c73f2161a@beagleboard.org/


>
>
> Best regards,
> Krzysztof


Best Regards,

Ayush Singh


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

* Re: [PATCH 3/4] spi: Handle spi bus extension
  2025-07-29 12:52     ` Ayush Singh
@ 2025-07-29 13:09       ` Krzysztof Kozlowski
  0 siblings, 0 replies; 12+ messages in thread
From: Krzysztof Kozlowski @ 2025-07-29 13:09 UTC (permalink / raw)
  To: Ayush Singh, Mark Brown, herve.codina, luca.ceresoli, conor+dt,
	Jason Kridner, Deepak Khatri, Dhruva Gole, Robert Nelson,
	Andrew Davis, Rob Herring, Krzysztof Kozlowski
  Cc: linux-spi, linux-kernel, devicetree

On 29/07/2025 14:52, Ayush Singh wrote:
> On 7/29/25 18:16, Krzysztof Kozlowski wrote:
> 
>> On 29/07/2025 11:51, Ayush Singh wrote:
>>>   	for_each_available_child_of_node(node, nc) {
>>> +		/* Filter out extension node */
>>> +		if (of_node_name_eq(nc, "spi-bus-extension"))
>>> +			continue;
>>> +
>>>   		if (of_node_test_and_set_flag(nc, OF_POPULATED))
>>>   			continue;
>>>   
>>> @@ -2541,6 +2549,23 @@ static void of_register_spi_children(struct spi_controller *ctlr,
>>>   			of_node_clear_flag(nc, OF_POPULATED);
>>>   		}
>>>   	}
>>> +
>>> +	/* Look at extensions */
>>> +	for_each_available_child_of_node(node, nc) {
>>> +		if (!of_node_name_eq(nc, "spi-bus-extension"))
>> Where did you document the new ABI? There is no DT bindings patch with it.
> 
> Patch 4 is the dt bindings patch. I will reorder the patches in any 
> future to make the dt bindings patch 1.
> 
> Here is a direct link in case it got lost in mail: 
> https://lore.kernel.org/all/20250729-spi-bus-extension-v1-4-b20c73f2161a@beagleboard.org/


I found it, just did not spot as a binding because my filters expect
certain subjects (see submitting patches in DT subdir).

Best regards,
Krzysztof

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

* Re: [PATCH 4/4] devicetree: bindings: spi: Introduce SPI bus extensions
  2025-07-29  9:51 ` [PATCH 4/4] devicetree: bindings: spi: Introduce SPI bus extensions Ayush Singh
  2025-07-29 12:52   ` Krzysztof Kozlowski
@ 2025-07-29 13:12   ` Rob Herring (Arm)
  1 sibling, 0 replies; 12+ messages in thread
From: Rob Herring (Arm) @ 2025-07-29 13:12 UTC (permalink / raw)
  To: Ayush Singh
  Cc: Jason Kridner, luca.ceresoli, Krzysztof Kozlowski, linux-kernel,
	conor+dt, Mark Brown, Deepak Khatri, Andrew Davis, Dhruva Gole,
	linux-spi, herve.codina, devicetree, Robert Nelson


On Tue, 29 Jul 2025 15:21:03 +0530, Ayush Singh wrote:
> An SPI bus can be wired to the connector and allows an add-on board to
> connect additional SPI devices to this bus.
> 
> Those additional SPI devices could be described as sub-nodes of the SPI
> bus controller node however for hotplug connectors described via device
> tree overlays there is additional level of indirection, which is needed
> to decouple the overlay and the base tree:
> 
>   --- base device tree ---
> 
>   spi1: spi@abcd0000 {
>       compatible = "xyz,foo";
>       spi-bus-extension@0 {
>           spi-bus = <&spi_ctrl>;
>       };
>       ...
>   };
> 
>   spi5: spi@cafe0000 {
>       compatible = "xyz,bar";
>       spi-bus-extension@0 {
>           spi-bus = <&spi_sensors>;
>       };
>       ...
>   };
> 
>   connector {
>       spi_ctrl: spi-ctrl {
>           spi-parent = <&spi1>;
>           #address-cells = <1>;
>           #size-cells = <0>;
>       };
> 
>       spi_sensors: spi-sensors {
>           spi-parent = <&spi5>;
>           #address-cells = <1>;
>           #size-cells = <0>;
>       };
>   };
> 
>   --- device tree overlay ---
> 
>   ...
>   // This node will overlay on the spi-ctrl node of the base tree
>   spi-ctrl {
>       eeprom@50 { compatible = "atmel,24c64"; ... };
>   };
>   ...
> 
>   --- resulting device tree ---
> 
>   spi1: spi@abcd0000 {
>       compatible = "xyz,foo";
>       spi-bus-extension@0 {
>           spi-bus = <&spi_ctrl>;
>       };
>       ...
>   };
> 
>   spi5: spi@cafe0000 {
>       compatible = "xyz,bar";
>       spi-bus-extension@0 {
>           spi-bus = <&spi_sensors>;
>       };
>       ...
>   };
> 
>   connector {
>       spi_ctrl: spi-ctrl {
>           spi-parent = <&spi1>;
>           #address-cells = <1>;
>           #size-cells = <0>;
> 
>           device@1 { compatible = "xyz,foo"; ... };
>       };
> 
>       spi_sensors: spi-sensors {
>           spi-parent = <&spi5>;
>           #address-cells = <1>;
>           #size-cells = <0>;
>       };
>   };
> 
> Here spi-ctrl (same goes for spi-sensors) represent the part of SPI bus
> that is on the hot-pluggable add-on. On hot-plugging it will physically
> connect to the SPI adapter on the base board. Let's call the 'spi-ctrl'
> node an "extension node".
> 
> In order to decouple the overlay from the base tree, the SPI adapter
> (spi@abcd0000) and the extension node (spi-ctrl) are separate nodes.
> 
> The extension node is linked to the SPI bus controller in two ways. The
> first one with the spi-bus-extension available in SPI controller
> sub-node and the second one with the spi-parent property available in
> the extension node itself.
> 
> The purpose of those two links is to provide the link in both direction
> from the SPI controller to the SPI extension and from the SPI extension
> to the SPI controller.
> 
> Signed-off-by: Ayush Singh <ayush@beagleboard.org>
> ---
>  .../devicetree/bindings/spi/spi-controller.yaml    | 66 +++++++++++++++++++++-
>  1 file changed, 65 insertions(+), 1 deletion(-)
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/spi/spi-controller.example.dtb: spi@abcd0000 (brcm,bcm2835-spi): 'oneOf' conditional failed, one must be fixed:
	'interrupts' is a required property
	'interrupts-extended' is a required property
	from schema $id: http://devicetree.org/schemas/spi/brcm,bcm2835-spi.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/spi/spi-controller.example.dtb: spi@abcd0000 (brcm,bcm2835-spi): 'clocks' is a required property
	from schema $id: http://devicetree.org/schemas/spi/brcm,bcm2835-spi.yaml#
Documentation/devicetree/bindings/spi/spi-controller.example.dtb: /example-2/connector/spi-addon/device@2: failed to match any schema with compatible: ['xyz,foo']

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20250729-spi-bus-extension-v1-4-b20c73f2161a@beagleboard.org

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

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

pip3 install dtschema --upgrade

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


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

* Re: [PATCH 4/4] devicetree: bindings: spi: Introduce SPI bus extensions
  2025-07-29 12:52   ` Krzysztof Kozlowski
@ 2025-07-30 15:45     ` Luca Ceresoli
  2025-07-30 18:30       ` Andrew Davis
  0 siblings, 1 reply; 12+ messages in thread
From: Luca Ceresoli @ 2025-07-30 15:45 UTC (permalink / raw)
  To: Krzysztof Kozlowski
  Cc: Ayush Singh, Mark Brown, herve.codina, conor+dt, Jason Kridner,
	Deepak Khatri, Dhruva Gole, Robert Nelson, Andrew Davis,
	Rob Herring, Krzysztof Kozlowski, linux-spi, linux-kernel,
	devicetree

On Tue, 29 Jul 2025 14:52:00 +0200
Krzysztof Kozlowski <krzk@kernel.org> wrote:

> On 29/07/2025 11:51, Ayush Singh wrote:
> > An SPI bus can be wired to the connector and allows an add-on board to
> > connect additional SPI devices to this bus.
> >   
> 
> ... so I found the binding. Not marked by my filters due to non-standard
> subject.
> 
> Please use subject prefixes matching the subsystem. You can get them for
> example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
> your patch is touching. For bindings, the preferred subjects are
> explained here:
> https://www.kernel.org/doc/html/latest/devicetree/bindings/submitting-patches.html#i-for-patch-submitters
> 
> > Those additional SPI devices could be described as sub-nodes of the SPI
> > bus controller node however for hotplug connectors described via device
> > tree overlays there is additional level of indirection, which is needed
> > to decouple the overlay and the base tree:
> > 
> >   --- base device tree ---
> > 
> >   spi1: spi@abcd0000 {
> >       compatible = "xyz,foo";
> >       spi-bus-extension@0 {
> >           spi-bus = <&spi_ctrl>;
> >       };
> >       ...
> >   };
> > 
> >   spi5: spi@cafe0000 {
> >       compatible = "xyz,bar";
> >       spi-bus-extension@0 {
> >           spi-bus = <&spi_sensors>;
> >       };
> >       ...
> >   };
> > 
> >   connector {
> >       spi_ctrl: spi-ctrl {
> >           spi-parent = <&spi1>;
> >           #address-cells = <1>;
> >           #size-cells = <0>;
> >       };
> > 
> >       spi_sensors: spi-sensors {
> >           spi-parent = <&spi5>;
> >           #address-cells = <1>;
> >           #size-cells = <0>;
> >       };
> >   };  
> 
> It looks like you are re-doing I2C work. Please wait till I2C discussion
> finishes, so we won't have to comment on the same in multiple places.
> 
> > 
> >   --- device tree overlay ---
> > 
> >   ...
> >   // This node will overlay on the spi-ctrl node of the base tree
> >   spi-ctrl {
> >       eeprom@50 { compatible = "atmel,24c64"; ... };
> >   };
> >   ...
> > 
> >   --- resulting device tree ---
> > 
> >   spi1: spi@abcd0000 {
> >       compatible = "xyz,foo";
> >       spi-bus-extension@0 {
> >           spi-bus = <&spi_ctrl>;
> >       };
> >       ...
> >   };
> > 
> >   spi5: spi@cafe0000 {
> >       compatible = "xyz,bar";
> >       spi-bus-extension@0 {
> >           spi-bus = <&spi_sensors>;
> >       };
> >       ...
> >   };
> > 
> >   connector {
> >       spi_ctrl: spi-ctrl {
> >           spi-parent = <&spi1>;
> >           #address-cells = <1>;
> >           #size-cells = <0>;
> > 
> >           device@1 { compatible = "xyz,foo"; ... };
> >       };
> > 
> >       spi_sensors: spi-sensors {
> >           spi-parent = <&spi5>;
> >           #address-cells = <1>;
> >           #size-cells = <0>;
> >       };
> >   };
> > 
> > Here spi-ctrl (same goes for spi-sensors) represent the part of SPI bus
> > that is on the hot-pluggable add-on. On hot-plugging it will physically
> > connect to the SPI adapter on the base board. Let's call the 'spi-ctrl'
> > node an "extension node".
> > 
> > In order to decouple the overlay from the base tree, the SPI adapter
> > (spi@abcd0000) and the extension node (spi-ctrl) are separate nodes.
> > 
> > The extension node is linked to the SPI bus controller in two ways. The
> > first one with the spi-bus-extension available in SPI controller
> > sub-node and the second one with the spi-parent property available in
> > the extension node itself.
> > 
> > The purpose of those two links is to provide the link in both direction
> > from the SPI controller to the SPI extension and from the SPI extension
> > to the SPI controller.
> > 
> > Signed-off-by: Ayush Singh <ayush@beagleboard.org>
> > ---
> >  .../devicetree/bindings/spi/spi-controller.yaml    | 66 +++++++++++++++++++++-
> >  1 file changed, 65 insertions(+), 1 deletion(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/spi/spi-controller.yaml b/Documentation/devicetree/bindings/spi/spi-controller.yaml
> > index 82d051f7bd6e09dab9809c85ff13475d2b118efd..9b44ce4542f9552c94cb0658ffe3f6d3f29bc434 100644
> > --- a/Documentation/devicetree/bindings/spi/spi-controller.yaml
> > +++ b/Documentation/devicetree/bindings/spi/spi-controller.yaml
> > @@ -25,6 +25,13 @@ properties:
> >    "#size-cells":
> >      const: 0
> >  
> > +  spi-parent:
> > +    $ref: /schemas/types.yaml#/definitions/phandle
> > +    description:
> > +      In case of an SPI bus extension, reference to the SPI bus controller
> > +      this extension is connected to. In other word, reference the SPI bus
> > +      controller on the fixed side that drives the bus extension.
> > +
> >    cs-gpios:
> >      description: |
> >        GPIOs used as chip selects.
> > @@ -111,7 +118,26 @@ properties:
> >        - compatible
> >  
> >  patternProperties:
> > -  "^.*@[0-9a-f]+$":
> > +  'spi-bus-extension@[0-9a-f]+$':
> > +    type: object
> > +    description:
> > +      An SPI bus extension connected to an SPI bus. Those extensions allow to
> > +      decouple SPI busses when they are wired to connectors.  
> 
> I really do not get why you need separate two-way phandles for marking
> parent child relationship. IOW, if you need two way, then why not graphs?
> 
> Or why not just making the device@2 a child of SPI, since it is coming
> from overlay.

For the same reason as in I2C (and the proposed solution is the same).

As you wrote above, Ayush could wait for the I2C discussion to finish.
Problem is, the I2C discussion is not moving forward: Hervé is sending
proposals but there is no feedback on the core of the proposal, and no
feedback at all on the latest iteration. In case your filters missed it:
https://lore.kernel.org/all/20250618082313.549140-1-herve.codina@bootlin.com/

I think Ayush's series is useful anyway in that it shows the approach
Hervé proposed works unmodified for another subsystem and is useful for
a different use case.

Luca

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

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

* Re: [PATCH 4/4] devicetree: bindings: spi: Introduce SPI bus extensions
  2025-07-30 15:45     ` Luca Ceresoli
@ 2025-07-30 18:30       ` Andrew Davis
  0 siblings, 0 replies; 12+ messages in thread
From: Andrew Davis @ 2025-07-30 18:30 UTC (permalink / raw)
  To: Luca Ceresoli, Krzysztof Kozlowski
  Cc: Ayush Singh, Mark Brown, herve.codina, conor+dt, Jason Kridner,
	Deepak Khatri, Dhruva Gole, Robert Nelson, Rob Herring,
	Krzysztof Kozlowski, linux-spi, linux-kernel, devicetree

On 7/30/25 10:45 AM, Luca Ceresoli wrote:
> On Tue, 29 Jul 2025 14:52:00 +0200
> Krzysztof Kozlowski <krzk@kernel.org> wrote:
> 
>> On 29/07/2025 11:51, Ayush Singh wrote:
>>> An SPI bus can be wired to the connector and allows an add-on board to
>>> connect additional SPI devices to this bus.
>>>    
>>
>> ... so I found the binding. Not marked by my filters due to non-standard
>> subject.
>>
>> Please use subject prefixes matching the subsystem. You can get them for
>> example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
>> your patch is touching. For bindings, the preferred subjects are
>> explained here:
>> https://www.kernel.org/doc/html/latest/devicetree/bindings/submitting-patches.html#i-for-patch-submitters
>>
>>> Those additional SPI devices could be described as sub-nodes of the SPI
>>> bus controller node however for hotplug connectors described via device
>>> tree overlays there is additional level of indirection, which is needed
>>> to decouple the overlay and the base tree:
>>>
>>>    --- base device tree ---
>>>
>>>    spi1: spi@abcd0000 {
>>>        compatible = "xyz,foo";
>>>        spi-bus-extension@0 {

Could we make this more generic so it doesn't need to be specific to each
type of node/bus? The core goal IIUC is to keep some parts of the node
out in another node, but have the contents act as if they were part of the
first node. So how about "node-extension", or if we only need one direction
links from the second node, "node-extends", something like this:


--- base device tree ---

spi1: spi@abcd0000 {
     compatible = "xyz,foo";
     #address-cells = <1>;
     #size-cells = <0>;
     node-extensions = <&spi_sensors>;
}

connector {
     spi_sensors: spi-sensors {
         eeprom@50 { compatible = "atmel,24c64"; ... };
     };
};

Or if we want to keep the base device node untouched (this would be my
preference as otherwise we would have to add modifications in more places):

--- base device tree ---

spi1: spi@abcd0000 {
     compatible = "xyz,foo";
     #address-cells = <1>;
     #size-cells = <0>;
}

connector {
     spi_sensors: spi-sensors {
         node-extends = <&spi1>;
         eeprom@50 { compatible = "atmel,24c64"; ... };
     };
};

In either case, the content of the "spi-sensors" node (i.e. eeprom@50)
acts in all ways as if that was directly inside the "spi1" node.

This should provide the separation you are looking for and be generic for
*any* node's use. The base OF support could be extended so that functions
like for_each_available_child_of_node() and property fetching handle
this transparently. That way you don't need to go make modifications
to each and every bus and driver that accesses the contents of nodes
to handle these node extensions.

>>>            spi-bus = <&spi_ctrl>;
>>>        };
>>>        ...
>>>    };
>>>
>>>    spi5: spi@cafe0000 {
>>>        compatible = "xyz,bar";
>>>        spi-bus-extension@0 {
>>>            spi-bus = <&spi_sensors>;
>>>        };
>>>        ...
>>>    };
>>>
>>>    connector {
>>>        spi_ctrl: spi-ctrl {
>>>            spi-parent = <&spi1>;
>>>            #address-cells = <1>;
>>>            #size-cells = <0>;
>>>        };
>>>
>>>        spi_sensors: spi-sensors {
>>>            spi-parent = <&spi5>;
>>>            #address-cells = <1>;
>>>            #size-cells = <0>;
>>>        };
>>>    };
>>
>> It looks like you are re-doing I2C work. Please wait till I2C discussion
>> finishes, so we won't have to comment on the same in multiple places.
>>
>>>
>>>    --- device tree overlay ---
>>>
>>>    ...
>>>    // This node will overlay on the spi-ctrl node of the base tree
>>>    spi-ctrl {
>>>        eeprom@50 { compatible = "atmel,24c64"; ... };
>>>    };
>>>    ...
>>>
>>>    --- resulting device tree ---
>>>
>>>    spi1: spi@abcd0000 {
>>>        compatible = "xyz,foo";
>>>        spi-bus-extension@0 {
>>>            spi-bus = <&spi_ctrl>;
>>>        };
>>>        ...
>>>    };
>>>
>>>    spi5: spi@cafe0000 {
>>>        compatible = "xyz,bar";
>>>        spi-bus-extension@0 {
>>>            spi-bus = <&spi_sensors>;
>>>        };
>>>        ...
>>>    };
>>>
>>>    connector {
>>>        spi_ctrl: spi-ctrl {
>>>            spi-parent = <&spi1>;
>>>            #address-cells = <1>;
>>>            #size-cells = <0>;
>>>
>>>            device@1 { compatible = "xyz,foo"; ... };
>>>        };
>>>
>>>        spi_sensors: spi-sensors {
>>>            spi-parent = <&spi5>;
>>>            #address-cells = <1>;
>>>            #size-cells = <0>;
>>>        };
>>>    };
>>>
>>> Here spi-ctrl (same goes for spi-sensors) represent the part of SPI bus
>>> that is on the hot-pluggable add-on. On hot-plugging it will physically
>>> connect to the SPI adapter on the base board. Let's call the 'spi-ctrl'
>>> node an "extension node".
>>>
>>> In order to decouple the overlay from the base tree, the SPI adapter
>>> (spi@abcd0000) and the extension node (spi-ctrl) are separate nodes.
>>>
>>> The extension node is linked to the SPI bus controller in two ways. The
>>> first one with the spi-bus-extension available in SPI controller
>>> sub-node and the second one with the spi-parent property available in
>>> the extension node itself.
>>>
>>> The purpose of those two links is to provide the link in both direction
>>> from the SPI controller to the SPI extension and from the SPI extension
>>> to the SPI controller.
>>>
>>> Signed-off-by: Ayush Singh <ayush@beagleboard.org>
>>> ---
>>>   .../devicetree/bindings/spi/spi-controller.yaml    | 66 +++++++++++++++++++++-
>>>   1 file changed, 65 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/spi/spi-controller.yaml b/Documentation/devicetree/bindings/spi/spi-controller.yaml
>>> index 82d051f7bd6e09dab9809c85ff13475d2b118efd..9b44ce4542f9552c94cb0658ffe3f6d3f29bc434 100644
>>> --- a/Documentation/devicetree/bindings/spi/spi-controller.yaml
>>> +++ b/Documentation/devicetree/bindings/spi/spi-controller.yaml
>>> @@ -25,6 +25,13 @@ properties:
>>>     "#size-cells":
>>>       const: 0
>>>   
>>> +  spi-parent:
>>> +    $ref: /schemas/types.yaml#/definitions/phandle
>>> +    description:
>>> +      In case of an SPI bus extension, reference to the SPI bus controller
>>> +      this extension is connected to. In other word, reference the SPI bus
>>> +      controller on the fixed side that drives the bus extension.
>>> +
>>>     cs-gpios:
>>>       description: |
>>>         GPIOs used as chip selects.
>>> @@ -111,7 +118,26 @@ properties:
>>>         - compatible
>>>   
>>>   patternProperties:
>>> -  "^.*@[0-9a-f]+$":
>>> +  'spi-bus-extension@[0-9a-f]+$':
>>> +    type: object
>>> +    description:
>>> +      An SPI bus extension connected to an SPI bus. Those extensions allow to
>>> +      decouple SPI busses when they are wired to connectors.
>>
>> I really do not get why you need separate two-way phandles for marking
>> parent child relationship. IOW, if you need two way, then why not graphs?
>>
>> Or why not just making the device@2 a child of SPI, since it is coming
>> from overlay.
> 
> For the same reason as in I2C (and the proposed solution is the same).
> 
> As you wrote above, Ayush could wait for the I2C discussion to finish.
> Problem is, the I2C discussion is not moving forward: Hervé is sending
> proposals but there is no feedback on the core of the proposal, and no
> feedback at all on the latest iteration. In case your filters missed it:
> https://lore.kernel.org/all/20250618082313.549140-1-herve.codina@bootlin.com/
> 

That series didn't end up in my mailbox for some reason, so replying
here, but my response above should apply to both I2C and this SPI series
just the same.

Andrew


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

end of thread, other threads:[~2025-07-30 18:58 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-07-29  9:50 [PATCH 0/4] spi: Introduce spi bus extensions Ayush Singh
2025-07-29  9:51 ` [PATCH 1/4] spi: Follow spi-parent when retrieving a controller from node Ayush Singh
2025-07-29  9:51 ` [PATCH 2/4] spi: Move children registration in a dedicated function Ayush Singh
2025-07-29  9:51 ` [PATCH 3/4] spi: Handle spi bus extension Ayush Singh
2025-07-29 12:46   ` Krzysztof Kozlowski
2025-07-29 12:52     ` Ayush Singh
2025-07-29 13:09       ` Krzysztof Kozlowski
2025-07-29  9:51 ` [PATCH 4/4] devicetree: bindings: spi: Introduce SPI bus extensions Ayush Singh
2025-07-29 12:52   ` Krzysztof Kozlowski
2025-07-30 15:45     ` Luca Ceresoli
2025-07-30 18:30       ` Andrew Davis
2025-07-29 13:12   ` Rob Herring (Arm)

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