Re: [EXT] Re: [PATCH v4 4/7] dt-bindings: usb: ci-hdrc-usb2: add restrictions for reg, interrupts, clock and clock-names properties

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 31/01/2024 09:24, Xu Yang wrote:
> Hi Krzysztof,
> 
>>
>> On 19/01/2024 08:19, Xu Yang wrote:
>>> Change reg, interrupts, clock and clock-names as common properties and add
>>> restrictions on them for different compatibles.
>>>
>>> Signed-off-by: Xu Yang <xu.yang_2@xxxxxxx>
>>>
>>> ---
>>> Changes in v4:
>>>  - new patch since v3's discussion
>>>  - split the reg, interrupts, clock and clock-names properties into
>>>    common part and device-specific
>>> ---
>>>  .../devicetree/bindings/usb/ci-hdrc-usb2.yaml | 118 +++++++++++++++---
>>>  1 file changed, 102 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.yaml b/Documentation/devicetree/bindings/usb/ci-
>> hdrc-usb2.yaml
>>> index b7e664f7395b..78e30ca0a8ca 100644
>>> --- a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.yaml
>>> +++ b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.yaml
>>> @@ -73,22 +73,10 @@ properties:
>>>                - nuvoton,npcm845-udc
>>>            - const: nuvoton,npcm750-udc
>>>
>>> -  reg:
>>> -    minItems: 1
>>> -    maxItems: 2
>>> -
>>> -  interrupts:
>>> -    minItems: 1
>>> -    maxItems: 2
>>> -
>>> -  clocks:
>>> -    minItems: 1
>>> -    maxItems: 3
>>> -
>>> -  clock-names:
>>> -    minItems: 1
>>> -    maxItems: 3
>>
>> Why all these are gone? They are supposed to be here. Your if:then: only
>> customizes them.
> 
> I have also concerns of whether to make this part common.
> I will revert this later.

Revert? No. This patch must be correct.
> 
>>
>>> -
>>> +  reg: true
>>> +  interrupts: true
>>> +  clocks: true
>>> +  clock-names: true
>>
>> No. These are not booleans on other variants.
> 
> Okay.
> 
>>
>>>    dr_mode: true
>>>
>>>    power-domains:
>>> @@ -412,6 +400,104 @@ allOf:
>>>          samsung,picophy-pre-emp-curr-control: false
>>>          samsung,picophy-dc-vol-level-adjust: false
>>>
>>> +  - if:
>>> +      properties:
>>> +        compatible:
>>> +          oneOf:
>>> +            - items:
>>> +                - const: fsl,imx27-usb
>>
>> No, the syntax you need is contains:.
>>
>> Look at existing code - there is no single binding with oneOf: in if: block.
> 
> I wonder why 'make dt_binding_check' does not report this issue if the syntax
> is not correct?

I did not say syntax is incorrect.


> 
> So I need to add contains as below, right?
> 
>   - if:
>       properties:
>         compatible:
>           contains:
>             oneOf:
>               - items:
>                   - const: fsl,imx27-usb
>               - items:
>                   - enum:
>                       - fsl,imx25-usb
>                       - fsl,imx35-usb
>                   - const: fsl,imx27-usb
> 
> The purpose of this code is to match:
> 
>   - compatible = "fsl,imx27-usb";
>   - compatible = "fsl,imx25-usb", "fsl,imx27-usb";
>   - compatible = "fsl,imx35-usb", "fsl,imx27-usb";
> 
> but should not match:
> 
>   - compatible = "fsl,imx7d-usb", "fsl,imx27-usb";
> 
> Is this feasible?

So maybe they are not compatible? Your patch creates some unusual
constraints for all the variants, which is probably result of huge one
binding for all implementations of re-used IP block. I don't think that
this huge if: you add here and further in the patch helps. Just like for
other re-used IP blocks, this should have common part and
per-device/per-family/per-implementation binding.

Best regards,
Krzysztof





[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux