diff mbox series

[7/8] dt-bindings: arm: rockchip: Add Relfor Saib

Message ID 20240823153528.3863993-8-karthikeyan@linumiz.com (mailing list archive)
State New, archived
Headers show
Series Add support Relfor Saib board which is based on Rockchip RV1109 SoC | expand

Commit Message

karthikeyan Aug. 23, 2024, 3:35 p.m. UTC
Add devicetree binding documentation for Relfor Saib
board which uses Rockchip RV1109 SoC

Signed-off-by: Karthikeyan Krishnasamy <karthikeyan@linumiz.com>
---
 Documentation/devicetree/bindings/arm/rockchip.yaml | 6 ++++++
 1 file changed, 6 insertions(+)

Comments

Conor Dooley Aug. 23, 2024, 4:21 p.m. UTC | #1
On Fri, Aug 23, 2024 at 09:05:27PM +0530, Karthikeyan Krishnasamy wrote:
> Add devicetree binding documentation for Relfor Saib
> board which uses Rockchip RV1109 SoC
> 
> Signed-off-by: Karthikeyan Krishnasamy <karthikeyan@linumiz.com>
> ---
>  Documentation/devicetree/bindings/arm/rockchip.yaml | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/arm/rockchip.yaml b/Documentation/devicetree/bindings/arm/rockchip.yaml
> index 1ef09fbfdfaf..29f7e09ae443 100644
> --- a/Documentation/devicetree/bindings/arm/rockchip.yaml
> +++ b/Documentation/devicetree/bindings/arm/rockchip.yaml
> @@ -848,6 +848,12 @@ properties:
>                - radxa,zero-3w
>            - const: rockchip,rk3566
>  
> +      - description: Relfor SAIB board
> +        items:
> +          - const: relfor,saib
> +          - enum:
> +              - rockchip,rv1109

This does not make sense to me. Why do you have an enum for the SoC
model, implying that this SAIB board would have more than one possible 
SoC? I'd expect to see - const: rockvhip,rv1109

> +
>        - description: Rikomagic MK808 v1
>          items:
>            - const: rikomagic,mk808
> -- 
> 2.39.2
>
karthikeyan Aug. 24, 2024, 12:48 p.m. UTC | #2
On 8/23/24 21:51, Conor Dooley wrote:
> On Fri, Aug 23, 2024 at 09:05:27PM +0530, Karthikeyan Krishnasamy wrote:
>> Add devicetree binding documentation for Relfor Saib
>> board which uses Rockchip RV1109 SoC
>>
>> Signed-off-by: Karthikeyan Krishnasamy <karthikeyan@linumiz.com>
>> ---
>>   Documentation/devicetree/bindings/arm/rockchip.yaml | 6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/arm/rockchip.yaml b/Documentation/devicetree/bindings/arm/rockchip.yaml
>> index 1ef09fbfdfaf..29f7e09ae443 100644
>> --- a/Documentation/devicetree/bindings/arm/rockchip.yaml
>> +++ b/Documentation/devicetree/bindings/arm/rockchip.yaml
>> @@ -848,6 +848,12 @@ properties:
>>                 - radxa,zero-3w
>>             - const: rockchip,rk3566
>>   
>> +      - description: Relfor SAIB board
>> +        items:
>> +          - const: relfor,saib
>> +          - enum:
>> +              - rockchip,rv1109
> 
> This does not make sense to me. Why do you have an enum for the SoC
> model, implying that this SAIB board would have more than one possible
> SoC? I'd expect to see - const: rockvhip,rv1109
> 
There is an upcoming version of SAIB board based on Rockchip RV1103.
>> +
>>         - description: Rikomagic MK808 v1
>>           items:
>>             - const: rikomagic,mk808
>> -- 
>> 2.39.2
>>
Krzysztof Kozlowski Aug. 26, 2024, 8:33 a.m. UTC | #3
On 24/08/2024 14:48, karthikeyan wrote:
> On 8/23/24 21:51, Conor Dooley wrote:
>> On Fri, Aug 23, 2024 at 09:05:27PM +0530, Karthikeyan Krishnasamy wrote:
>>> Add devicetree binding documentation for Relfor Saib
>>> board which uses Rockchip RV1109 SoC
>>>
>>> Signed-off-by: Karthikeyan Krishnasamy <karthikeyan@linumiz.com>
>>> ---
>>>   Documentation/devicetree/bindings/arm/rockchip.yaml | 6 ++++++
>>>   1 file changed, 6 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/arm/rockchip.yaml b/Documentation/devicetree/bindings/arm/rockchip.yaml
>>> index 1ef09fbfdfaf..29f7e09ae443 100644
>>> --- a/Documentation/devicetree/bindings/arm/rockchip.yaml
>>> +++ b/Documentation/devicetree/bindings/arm/rockchip.yaml
>>> @@ -848,6 +848,12 @@ properties:
>>>                 - radxa,zero-3w
>>>             - const: rockchip,rk3566
>>>   
>>> +      - description: Relfor SAIB board
>>> +        items:
>>> +          - const: relfor,saib
>>> +          - enum:
>>> +              - rockchip,rv1109
>>
>> This does not make sense to me. Why do you have an enum for the SoC
>> model, implying that this SAIB board would have more than one possible
>> SoC? I'd expect to see - const: rockvhip,rv1109
>>
> There is an upcoming version of SAIB board based on Rockchip RV1103.

Still wrong form multiple points of view:
1. Not logical, we never expect such entry in top level bindings,
2. Same board or different? If same, how is it possible to have two
different SoCs (not modules!) in the same board? These are different
boards. Or maybe this uses some SoM, but your commit msg explained
nothing about this.

You have entire commit msg to explain the hardware. Use it, so you don't
get such questions.

The code above: NAK

Best regards,
Krzysztof
karthikeyan Aug. 28, 2024, 9:23 a.m. UTC | #4
On 8/26/24 14:03, Krzysztof Kozlowski wrote:
> On 24/08/2024 14:48, karthikeyan wrote:
>> On 8/23/24 21:51, Conor Dooley wrote:
>>> On Fri, Aug 23, 2024 at 09:05:27PM +0530, Karthikeyan Krishnasamy wrote:
>>>> Add devicetree binding documentation for Relfor Saib
>>>> board which uses Rockchip RV1109 SoC
>>>>
>>>> Signed-off-by: Karthikeyan Krishnasamy <karthikeyan@linumiz.com>
>>>> ---
>>>>    Documentation/devicetree/bindings/arm/rockchip.yaml | 6 ++++++
>>>>    1 file changed, 6 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/arm/rockchip.yaml b/Documentation/devicetree/bindings/arm/rockchip.yaml
>>>> index 1ef09fbfdfaf..29f7e09ae443 100644
>>>> --- a/Documentation/devicetree/bindings/arm/rockchip.yaml
>>>> +++ b/Documentation/devicetree/bindings/arm/rockchip.yaml
>>>> @@ -848,6 +848,12 @@ properties:
>>>>                  - radxa,zero-3w
>>>>              - const: rockchip,rk3566
>>>>    
>>>> +      - description: Relfor SAIB board
>>>> +        items:
>>>> +          - const: relfor,saib
>>>> +          - enum:
>>>> +              - rockchip,rv1109
>>>
>>> This does not make sense to me. Why do you have an enum for the SoC
>>> model, implying that this SAIB board would have more than one possible
>>> SoC? I'd expect to see - const: rockvhip,rv1109
>>>
>> There is an upcoming version of SAIB board based on Rockchip RV1103.
> 
> Still wrong form multiple points of view:
> 1. Not logical, we never expect such entry in top level bindings,
> 2. Same board or different? If same, how is it possible to have two
> different SoCs (not modules!) in the same board? These are different
> boards. Or maybe this uses some SoM, but your commit msg explained
> nothing about this.
> 
> You have entire commit msg to explain the hardware. Use it, so you don't
> get such questions.
> 
> The code above: NAK
It's my mistake, previously my understanding about this binding was 
wrong. I will change it in v2 patch.>
> Best regards,
> Krzysztof
> 
Thanks,
Karthikeyan
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/arm/rockchip.yaml b/Documentation/devicetree/bindings/arm/rockchip.yaml
index 1ef09fbfdfaf..29f7e09ae443 100644
--- a/Documentation/devicetree/bindings/arm/rockchip.yaml
+++ b/Documentation/devicetree/bindings/arm/rockchip.yaml
@@ -848,6 +848,12 @@  properties:
               - radxa,zero-3w
           - const: rockchip,rk3566
 
+      - description: Relfor SAIB board
+        items:
+          - const: relfor,saib
+          - enum:
+              - rockchip,rv1109
+
       - description: Rikomagic MK808 v1
         items:
           - const: rikomagic,mk808