Message-ID: <b31f9e88-a078-47f4-8d6c-359a0394ef7e@baylibre.com>
Date: Mon, 8 Dec 2025 10:14:36 -0600
From: David Lechner <dlechner@...libre.com>
To: Rob Herring <robh@...nel.org>
Cc: Marcelo Schmitt <marcelo.schmitt1@...il.com>,
 Mark Brown <broonie@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
 Conor Dooley <conor+dt@...nel.org>,
 Marcelo Schmitt <marcelo.schmitt@...log.com>,
 Michael Hennerich <michael.hennerich@...log.com>,
 Nuno Sá <nuno.sa@...log.com>,
 Jonathan Cameron <jic23@...nel.org>, Andy Shevchenko <andy@...nel.org>,
 Sean Anderson <sean.anderson@...ux.dev>, linux-spi@...r.kernel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-iio@...r.kernel.org
Subject: Re: [PATCH v3 7/7] dt-bindings: iio: adc: adi,ad4030: add data-lanes
 property

On 12/5/25 6:47 PM, Rob Herring wrote:
> On Fri, Dec 05, 2025 at 05:43:31PM -0600, David Lechner wrote:
>> On 12/5/25 3:33 PM, David Lechner wrote:
>>> On 12/5/25 3:12 PM, Marcelo Schmitt wrote:
>>>> On 12/04, Rob Herring wrote:
>>>>> On Mon, Dec 01, 2025 at 08:20:45PM -0600, David Lechner wrote:
>>>>>> Add data-lanes property to specify the number of data lanes used on the
>>>>>> ad463x chips that support reading two samples at the same time using
>>>>>> two data lanes with a capable SPI controller.
>>>>>>
>>>>>> Signed-off-by: David Lechner <dlechner@...libre.com>
>>>>>> ---
>>>>>> v3 changes: new patch
>>>>>>
>>>>>> I added this one to give a real-world use case where spi-rx-bus-width
>>>>>> was not sufficient to fully describe the hardware configuration.
>>>>>>
>>>>>> spi-rx-bus-width = <4>; alone could be be interpreted as either:
>>>>>>
>>>>>> +--------------+    +----------+
>>>>>> | SPI          |    | AD4630   |
>>>>>> | Controller   |    | ADC      |
>>>>>> |              |    |          |
>>>>>> |        SDIA0 |<---| SDOA0    |
>>>>>> |        SDIA1 |<---| SDOA1    |
>>>>>> |        SDIA2 |<---| SDOA2    |
>>>>>> |        SDIA3 |<---| SDOA3    |
>>>>>> |              |    |          |
>>>>>> |        SDIB0 |x   | SDOB0    |
>>>>>> |        SDIB1 |x   | SDOB1    |
>>>>>> |        SDIB2 |x   | SDOB2    |
>>>>>> |        SDIB3 |x   | SDOB3    |
>>>>>> |              |    |          |
>>>>>> +--------------+     +---------+
>>>>>>
>>>>>> or
>>>>>>
>>>>>> +--------------+    +----------+
>>>>>> | SPI          |    | AD4630   |
>>>>>> | Controller   |    | ADC      |
>>>>>> |              |    |          |
>>>>>> |        SDIA0 |<---| SDOA0    |
>>>>>> |        SDIA1 |<---| SDOA1    |
>>>>>> |        SDIA2 |x   | SDOA2    |
>>>>>> |        SDIA3 |x   | SDOA3    |
>>>>>> |              |    |          |
>>>>>> |        SDIB0 |<---| SDOB0    |
>>>>>> |        SDIB1 |<---| SDOB1    |
>>>>>> |        SDIB2 |x   | SDOB2    |
>>>>>> |        SDIB3 |x   | SDOB3    |
>>>>>> |              |    |          |
>>>>>> +--------------+     +---------+
>>>>>>
>>>>>> Now, with data-lanes having a default value of [0] (inherited from
>>>>>> spi-peripheral-props.yaml), specifying:
>>>>>>
>>>>>>     spi-rx-bus-width = <4>;
>>>>>>
>>>>>> is unambiguously the first case and the example given in the binding
>>>>>> documentation is the second case:
>>>>>>
>>>>>>     spi-rx-bus-width = <2>;
>>>>>>     data-lanes = <0>, <1>;
>>>>>
>>>>> I just reviewed this and all, but what if you just did:
>>>>>
>>>>> spi-rx-bus-width = <2>, <2>;
>>>>>
>>>>> So *-bus-width becomes equal to the number of serializers/channels.
>>>>
>>>> Unless I'm missing something, I think that would also describe the currently
>>>> possible use cases as well. To me, it actually seems even more accurate than
>>>> data-lanes. The data-lanes property only describes the SPI controller input
>>>> lines/lanes, no info is given about the output lanes.
>>>
>>> It describes both directions.
>>>
>>>> Well yeah, that would only> be a problem for a device with multiple input serializers and multiple output
>>>> serializers. Still, the *-bus-width = <N>, <N>, ... <N>; notation looks clearer,
>>>> IMHO.
>>>>
>>>>>
>>>>> Rob
>>>>>
>>>
>>> It think it complicates Sean's use case though where such
>>> a controller is being used as basically two separate SPI
>>> buses.
>>>
>>> For that case, we want to be able to do:
>>>
>>> spi {
>>> 	...
>>>
>>> 	thing@0 {
>>> 		compatible = ...;
>>> 		reg = <0>;
>>> 		/* (implicit) data-lanes = <0>; */
>>> 	};
>>>
>>> 	thing@1 {
>>> 		compatible = ...;
>>> 		reg = <1>;
>>> 		data-lanes = <1>;
>>> 	};
>>> };
>>>
>>> Meaning:
>>>
>>> +--------------+    +----------+
>>> | SPI          |    | Thing 1  |
>>> | Controller   |    |          |
>>> |              |    |          |
>>> |          CS0 |--->| CS       |
>>> |         SDI0 |<---| SDO      |
>>> |         SDO0 |--->| SDI      |
>>> |        SCLK0 |--->| SCLK     |
>>> |              |    |          |
>>> |              |    +----------+
>>> |              |                
>>> |              |    +----------+
>>> |              |    | Thing 2  |
>>> |              |    |          |
>>> |          CS1 |--->| CS       |
>>> |         SDI1 |<---| SDO      |
>>> |         SDO1 |--->| SDI      |
>>> |        SCLK1 |--->| SCLK     |
>>> |              |    |          |
>>> +--------------+    +----------+
>>>
>>> (I don't remember if SCLKs are shared or separate, but I don't
>>> think that is relevant anyway).
>>>
>>>
>>> I guess we could write it like this?
>>>
>>> spi {
>>> 	...
>>>
>>> 	thing@0 {
>>> 		compatible = ...;
>>> 		reg = <0>;
>>> 	};
>>>
>>> 	thing@1 {
>>> 		compatible = ...;
>>> 		reg = <1>;
>>> 		spi-tx-bus-width = <0>, <1>;
>>> 		spi-rx-bus-width = <0>, <1>;
>>> 	};
>>> };
> 
> I forget the details on that, but just looking at the above I think 
> something like that should have 2 SPI bus nodes under the controller. 
> Unless CS0 and CS1 can't be asserted at the same time and they aren't 
> really independent.

It is the case that they aren't really independent. Only one "bus"
can operate at a time.

> 
> But would be good to wait for Sean's comments here.
> 
>>
>> I started down this road. Before I do the working of changing the
>> whole series, this is what it will probably look like. Is this really
>> what we want?
>>
>> There is one issue I see with this. If we allow <0> to mean that a lane
>> isn't wired up on the controller, then we can't constrain the length of
>> the array in peripheral bindings. For example, the ad403x chips can only
>> have one lane and the ad463x chips can have one or two lanes. But I
>> don't see a way to express that in the binding if <0> at any index
>> doesn't count towards the number of lanes that are actually wired up.
> 
> That's fine I think. How many entries is primarily a controller 
> property. We set the length in the controller binding. The device just 
> sets the maximum width per channel.
> 
>>
>> This is e.g. why the binding in sitronix,st7789v.yaml is
>>
>> 	items:
>> 	  enum: [0, 1]
>>
>> rather than
>>
>> 	items:
>> 	  - enum: [0, 1]
>>
>> ---
>> commit 049b9508b1b0190f87a4b35fe3ed8a9f3d0d3c50
>> Author: David Lechner <dlechner@...libre.com>
>> Date:   Fri Dec 5 16:09:08 2025 -0600
>>
>>     spi: dt-bindings: change spi-{rx,tx}-bus-width to arrays
>>     
>>     Change spi-rx-bus-width and spi-tx-bus-width properties from single
>>     uint32 values to arrays of uint32 values. This allows describing SPI
>>     peripherals connected to controllers that have multiple data lanes for
>>     receiving or transmitting two or more words at the same time.
>>     
>>     Bindings that make use of this property are updated in the same commit
>>     to avoid validation errors. Bindings that used minimum/maximum are
>>     changed to use enum instead to be consistent with the base property
>>     definition.
>>     
>>     The adi,ad4030 binding has an example added now that we can fully
>>     describe the peripheral's capabilities.
>>     
>>     Converting from single uint32 to array of uint32 does not break .dts/
>>     .dtb files since there is no difference between specifying a single
>>     uint32 value and an array with a single uint32 value in devicetree.
>>     
>>     Signed-off-by: David Lechner <dlechner@...libre.com>
>> ---   
>>
>> diff --git a/Documentation/devicetree/bindings/display/panel/sitronix,st7789v.yaml b/Documentation/devicetree/bindings/display/panel/sitronix,st7789v.yaml
>> index 0ce2ea13583d..23b33dcd5ed4 100644
>> --- a/Documentation/devicetree/bindings/display/panel/sitronix,st7789v.yaml
>> +++ b/Documentation/devicetree/bindings/display/panel/sitronix,st7789v.yaml
>> @@ -34,8 +34,8 @@ properties:
>>    spi-cpol: true
>>  
>>    spi-rx-bus-width:
>> -    minimum: 0
>> -    maximum: 1
>> +    items:
>> +      enum: [0, 1]
>>  
>>    dc-gpios:
>>      maxItems: 1
>> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad4030.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad4030.yaml
>> index 54e7349317b7..6052a44b04de 100644
>> --- a/Documentation/devicetree/bindings/iio/adc/adi,ad4030.yaml
>> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad4030.yaml
>> @@ -37,7 +37,8 @@ properties:
>>      maximum: 102040816
>>  
>>    spi-rx-bus-width:
>> -    enum: [1, 2, 4]
>> +    items:
>> +      enum: [1, 2, 4]
> 
> We'd need to allow 0 here, right?

To avoid binding check failures, yes, I suppose so. All of the
`const: 1` would need to be changed to `enum: [0, 1]` as well.

Although since the controller also could have limitations maybe
we should have the controller use `enum` and have the peripheral
use `maximum`?

Then, if we have a controller with:

patternProperties:
  "@[0-9a-f]+$":
      # controller has 2 lanes with 2 lines per lane
      spi-rx-bus-width:
	maxItems: 2
        items:
          enum: [0, 1, 2]

And a peripheral with:

properties:
  spi-rx-bus-width:
    items:
      maximum: 4

The controller limit would be in effect and cause a binding check error
if attempting to use bus width of 4.

But if the controller was enum: [0, 1, 2, 4, 8], then the peripheral
maximum would be the limiting factor if attempting to use bus width of 8.

> 
> What we really want to say is there is exactly 1 entry of 1, 2, or 4. I

Not sure this is what we want. For the ADC cases, we want 2 or 4 items
in the array to be the same value.
 
> can't think of a concise way to say that. The closest is something like 
> this:
> 
> uniqueItems: true
> items:
>   enum: [0, 1, 2, 4]
> contains:
>   enum: [1, 2, 4]
> 
> That implicitly limits the length to 4, but does allow [0, 1, 2, 4] and 
> other ordering. More generally, if the device supports fewer channels 
> than the host, then we can't constrain that. Oh well, we can't check 
> everything (we hardly check values within reg, interrupts, clocks, and 
> on and on). But most controllers are going to limit the length to 1 
> entry, so it should end up with the same constraints most of the time.
> 
> Are these updates all of them or just a sampling.

This is everything currently in linux-next.

> If the latter and 
> there's a lot more, then we may want to split spi-controller.yaml into 
> 2 (3 really with a common part). Make spi-controller.yaml define single 
> channel SPI controllers which keeps the length of spi-[rt]x-bus-width at 
> 1 entry and then define a spi-multi-chan-controller.yaml which doesn't 
> constrain it. 
> 
> Rob

Powered by blists - more mailing lists