Re: [PATCH 4/4] devicetree: bindings: spi: Introduce SPI bus extensions

[Andrew Davis <afd@ti.com>]

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