Message-ID: <c65d26d0-51b8-4131-a755-6c72b7dea549@kernel.org>
Date: Tue, 29 Jul 2025 14:52:00 +0200
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Ayush Singh <ayush@...gleboard.org>, Mark Brown <broonie@...nel.org>,
 herve.codina@...tlin.com, luca.ceresoli@...tlin.com, conor+dt@...nel.org,
 Jason Kridner <jkridner@...gleboard.org>,
 Deepak Khatri <lorforlinux@...gleboard.org>, Dhruva Gole <d-gole@...com>,
 Robert Nelson <robertcnelson@...gleboard.org>, Andrew Davis <afd@...com>,
 Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>
Cc: linux-spi@...r.kernel.org, linux-kernel@...r.kernel.org,
 devicetree@...r.kernel.org
Subject: Re: [PATCH 4/4] devicetree: bindings: spi: Introduce SPI bus
 extensions

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@...d0000 {
>       compatible = "xyz,foo";
>       spi-bus-extension@0 {
>           spi-bus = <&spi_ctrl>;
>       };
>       ...
>   };
> 
>   spi5: spi@...e0000 {
>       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@...d0000 {
>       compatible = "xyz,foo";
>       spi-bus-extension@0 {
>           spi-bus = <&spi_ctrl>;
>       };
>       ...
>   };
> 
>   spi5: spi@...e0000 {
>       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@...d0000) 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@...gleboard.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

Powered by blists - more mailing lists