diff mbox series

[v1] spi: dt-bindings: spi-rockchip: restrict num-cs

Message ID acc4ff4b-811a-4a6d-8f58-9d8da3be40bb@gmail.com
State New
Headers show
Series [v1] spi: dt-bindings: spi-rockchip: restrict num-cs | expand

Commit Message

Johan Jonker Jan. 22, 2024, 10:59 p.m. UTC
In the driver spi-rockchip.c max_native_cs is limited to 4 and the
default num-cs property is 1. Restrict num-cs in spi-rockchip.yaml.

Signed-off-by: Johan Jonker <jbx6244@gmail.com>
---
 Documentation/devicetree/bindings/spi/spi-rockchip.yaml | 5 +++++
 1 file changed, 5 insertions(+)

--
2.39.2

Comments

Johan Jonker Jan. 23, 2024, 10:49 a.m. UTC | #1
On 1/23/24 10:17, Luis de Arquer wrote:
> On 1/22/24 23:59, Johan Jonker wrote:
>> In the driver spi-rockchip.c max_native_cs is limited to 4 and the
>> default num-cs property is 1. Restrict num-cs in spi-rockchip.yaml.
>>
> 

> Doesn't num-cs include gpio chip selects too?
> I have a setup with num-cs = <12> which uses non-native cs-gpios just fine

Given that bindings and Linux drivers capabilities are 2 separate things.
However this document has also a purpose that must notify mainline maintainers if users submit bogus DT values.
Currently that limit is set to 4 in the mainline driver.
You are free to submit a real board file/patch serie afterwords as proof for review with 12 spi chips and then adjust this limit and increase ROCKCHIP_SPI_MAX_CS_NUM.

Johan

> 
> Luis
> 
>> Signed-off-by: Johan Jonker <jbx6244@gmail.com>
>> ---
>>   Documentation/devicetree/bindings/spi/spi-rockchip.yaml | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/spi/spi-rockchip.yaml b/Documentation/devicetree/bindings/spi/spi-rockchip.yaml
>> index e4941e9212d1..00d555bcbad3 100644
>> --- a/Documentation/devicetree/bindings/spi/spi-rockchip.yaml
>> +++ b/Documentation/devicetree/bindings/spi/spi-rockchip.yaml
>> @@ -65,6 +65,11 @@ properties:
>>         - const: tx
>>         - const: rx
>>
>> +  num-cs:
>> +    default: 1
>> +    minimum: 1
>> +    maximum: 4
>> +
>>     rx-sample-delay-ns:
>>       default: 0
>>       description:
>> -- 
>> 2.39.2
>>
>>
>> _______________________________________________
>> Linux-rockchip mailing list
>> Linux-rockchip@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-rockchip
>
Luis de Arquer Jan. 23, 2024, 1:51 p.m. UTC | #2
On 1/23/24 11:49, Johan Jonker wrote:
> 
> 
> On 1/23/24 10:17, Luis de Arquer wrote:
>> On 1/22/24 23:59, Johan Jonker wrote:
>>> In the driver spi-rockchip.c max_native_cs is limited to 4 and the
>>> default num-cs property is 1. Restrict num-cs in spi-rockchip.yaml.
>>>
>>
> 
>> Doesn't num-cs include gpio chip selects too?
>> I have a setup with num-cs = <12> which uses non-native cs-gpios just fine
> 
> Given that bindings and Linux drivers capabilities are 2 separate things.
> However this document has also a purpose that must notify mainline maintainers if users submit bogus DT values.
> Currently that limit is set to 4 in the mainline driver.
> You are free to submit a real board file/patch serie afterwords as proof for review with 12 spi chips and then adjust this limit and increase ROCKCHIP_SPI_MAX_CS_NUM.

Hi Johan,

OK to that, I was not aware a driver could limit num-cs (I thought it 
could be extended with as many gpios as needed without involving the 
controller driver). I thought of num-cs as a spi subsystem generic thing.

In fact, I was reviewing the setup I mentioned, and even it defines 12 
cs in the controller with no driver complaints, so far only 3 of them 
are mapped to devices (I'll have to review the driver before going any 
futher!)

Luis


> 
> Johan
> 
>>
>> Luis
>>
>>> Signed-off-by: Johan Jonker <jbx6244@gmail.com>
>>> ---
>>>    Documentation/devicetree/bindings/spi/spi-rockchip.yaml | 5 +++++
>>>    1 file changed, 5 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/spi/spi-rockchip.yaml b/Documentation/devicetree/bindings/spi/spi-rockchip.yaml
>>> index e4941e9212d1..00d555bcbad3 100644
>>> --- a/Documentation/devicetree/bindings/spi/spi-rockchip.yaml
>>> +++ b/Documentation/devicetree/bindings/spi/spi-rockchip.yaml
>>> @@ -65,6 +65,11 @@ properties:
>>>          - const: tx
>>>          - const: rx
>>>
>>> +  num-cs:
>>> +    default: 1
>>> +    minimum: 1
>>> +    maximum: 4
>>> +
>>>      rx-sample-delay-ns:
>>>        default: 0
>>>        description:
>>> -- 
>>> 2.39.2
>>>
>>>
>>> _______________________________________________
>>> Linux-rockchip mailing list
>>> Linux-rockchip@lists.infradead.org
>>> http://lists.infradead.org/mailman/listinfo/linux-rockchip
>>
Robin Murphy Jan. 23, 2024, 5:45 p.m. UTC | #3
On 23/01/2024 10:49 am, Johan Jonker wrote:
> 
> 
> On 1/23/24 10:17, Luis de Arquer wrote:
>> On 1/22/24 23:59, Johan Jonker wrote:
>>> In the driver spi-rockchip.c max_native_cs is limited to 4 and the
>>> default num-cs property is 1. Restrict num-cs in spi-rockchip.yaml.
>>>
>>
> 
>> Doesn't num-cs include gpio chip selects too?
>> I have a setup with num-cs = <12> which uses non-native cs-gpios just fine
> 
> Given that bindings and Linux drivers capabilities are 2 separate things.

Er, that's the whole point - bindings and drivers *are* separate things, 
and bindings do not describe drivers. Not least since the fundamental 
model is to have one canonical binding for multiple different drivers to 
consume.

There seems to be some ambiguity as to whether the common "num-cs" 
property is supposed to describe the number of dedicated hardware 
chipselects or the total number including additional GPIOs, but either 
way this change appears to be objectively wrong - if it's the former 
than the comment in the driver plus a survey of a few TRMs implies that 
the maximum number of hardware lines is 2; if it's the latter then 
obviously it's valid for a platform to wire up 3 or more additional 
GPIOs as desired, and for a DT to accurately describe that, regardless 
of whether any particular consumer happens to support it yet or not. For 
example, AFAICS the U-Boot and FreeBSD drivers for Rockchip SPI appear 
not to support GPIO chipselects at all.

Thanks,
Robin.

> However this document has also a purpose that must notify mainline maintainers if users submit bogus DT values.
> Currently that limit is set to 4 in the mainline driver.
> You are free to submit a real board file/patch serie afterwords as proof for review with 12 spi chips and then adjust this limit and increase ROCKCHIP_SPI_MAX_CS_NUM.
> 
> Johan
> 
>>
>> Luis
>>
>>> Signed-off-by: Johan Jonker <jbx6244@gmail.com>
>>> ---
>>>    Documentation/devicetree/bindings/spi/spi-rockchip.yaml | 5 +++++
>>>    1 file changed, 5 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/spi/spi-rockchip.yaml b/Documentation/devicetree/bindings/spi/spi-rockchip.yaml
>>> index e4941e9212d1..00d555bcbad3 100644
>>> --- a/Documentation/devicetree/bindings/spi/spi-rockchip.yaml
>>> +++ b/Documentation/devicetree/bindings/spi/spi-rockchip.yaml
>>> @@ -65,6 +65,11 @@ properties:
>>>          - const: tx
>>>          - const: rx
>>>
>>> +  num-cs:
>>> +    default: 1
>>> +    minimum: 1
>>> +    maximum: 4
>>> +
>>>      rx-sample-delay-ns:
>>>        default: 0
>>>        description:
>>> -- 
>>> 2.39.2
>>>
>>>
>>> _______________________________________________
>>> Linux-rockchip mailing list
>>> Linux-rockchip@lists.infradead.org
>>> http://lists.infradead.org/mailman/listinfo/linux-rockchip
>>
> 
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/spi/spi-rockchip.yaml b/Documentation/devicetree/bindings/spi/spi-rockchip.yaml
index e4941e9212d1..00d555bcbad3 100644
--- a/Documentation/devicetree/bindings/spi/spi-rockchip.yaml
+++ b/Documentation/devicetree/bindings/spi/spi-rockchip.yaml
@@ -65,6 +65,11 @@  properties:
       - const: tx
       - const: rx

+  num-cs:
+    default: 1
+    minimum: 1
+    maximum: 4
+
   rx-sample-delay-ns:
     default: 0
     description: