Re: [PATCH] dt-bindings: clock: socfpga: convert to yaml

["Gerlach, Matthew" <matthew.gerlach@altera.com>]

On 4/22/2025 6:23 AM, Rob Herring wrote:

> 
> On Thu, Apr 17, 2025 at 09:06:16AM -0700, Matthew Gerlach wrote:
>> Convert the clock device tree bindings to yaml for the Altera SoCFPGA
>> Cyclone5, Arria5, and Arria10 chip families. Since the clock nodes are
>> subnodes to Altera SOCFPGA Clock Manager, the yaml was added to
>> socfpga-clk-manager.yaml.
>>
>> Signed-off-by: Matthew Gerlach <matthew.gerlach@altera.com>
>> ---
>>  .../arm/altera/socfpga-clk-manager.yaml       | 118 +++++++++++++++++-
>>  .../bindings/clock/altr_socfpga.txt           |  30 -----
>>  2 files changed, 117 insertions(+), 31 deletions(-)
>>  delete mode 100644 Documentation/devicetree/bindings/clock/altr_socfpga.txt
>>
>> diff --git a/Documentation/devicetree/bindings/arm/altera/socfpga-clk-manager.yaml b/Documentation/devicetree/bindings/arm/altera/socfpga-clk-manager.yaml
>> index 572381306681..4cda13259530 100644
>> --- a/Documentation/devicetree/bindings/arm/altera/socfpga-clk-manager.yaml
>> +++ b/Documentation/devicetree/bindings/arm/altera/socfpga-clk-manager.yaml
>> @@ -9,17 +9,133 @@ title: Altera SOCFPGA Clock Manager
>>  maintainers:
>>    - Dinh Nguyen <dinguyen@kernel.org>
>>
>> -description: test
>> +description:
>> +  This binding describes the Altera SOCFGPA Clock Manager and its associated
>> +  tree of clocks, pll's, and clock gates for the Cyclone5, Arria5 and Arria10
>> +  chip families.
>>
>>  properties:
>>    compatible:
>>      items:
>>        - const: altr,clk-mgr
>> +
>>    reg:
>>      maxItems: 1
>>
>> +  clocks:
>> +    type: object
>> +    additionalProperties: false
>> +
>> +    properties:
>> +      "#address-cells":
>> +        const: 1
>> +
>> +      "#size-cells":
>> +        const: 0
>> +
>> +    patternProperties:
>> +      "^osc[0-9]$":
>> +        type: object
>> +
>> +      "^[a-z0-9,_]+[clk,pll,clk_gate,clk_divided](@[a-f0-9]+)?$":
> 
> This regex doesn't do what you think it does. You want:
> 
> "^[a-z0-9,_]+(clk|pll|clk_gate|clk_divided)(@[a-f0-9]+)?$"

Yes, the above is the correct regex.

> 
> However, I don't see clk_gate or clk_divided used anywhere, so I would
> simplify to:

ddr_dqs_clk_gate, ddr_2x_dqs_clk_gate, ddr_dq_clk_gate, and 
sdmmc_clk_divided are all names of nodes in 
arch/arm/boot/dts/intel/socfpga/socfpga.dtsi; so I don't think I can 
simplify as you suggest.

> 
> "(clk|pll)(@[a-f0-9]+)?$"
> 
> 
>> +        type: object
>> +        additionalProperties: false
>> +
>> +        properties:
>> +
>> +          compatible:
>> +            enum:
>> +              - altr,socfpga-pll-clock
>> +              - altr,socfpga-perip-clk
>> +              - altr,socfpga-gate-clk
>> +              - altr,socfpga-a10-pll-clock
>> +              - altr,socfpga-a10-perip-clk
>> +              - altr,socfpga-a10-gate-clk
>> +              - fixed-clock
>> +
>> +          clocks:
>> +            $ref: /schemas/types.yaml#/definitions/phandle-array
> 
> clocks already has a type.

I will remove redundant type.

> 
>> +            description: one or more phandles to input clock
> 
> I assume there is some max, so make this constraints:
> 
> minItems: 1
> maxItems: ?
Yes, adding min/maxItems is appropriate.

> 
>> +
>> +          "#address-cells":
>> +            const: 1
>> +
>> +          "#clock-cells":
>> +            const: 0
>> +
>> +          "#size-cells":
>> +            const: 0
>> +
>> +          clk-gate:
>> +            $ref: /schemas/types.yaml#/definitions/uint32-array
>> +            items:
>> +              - description: gating register offset
>> +              - description: bit index
>> +
>> +          div-reg:
>> +            $ref: /schemas/types.yaml#/definitions/uint32-array
>> +            items:
>> +              - description: divider register offset
>> +              - description: bit shift
>> +              - description: bit width
>> +
>> +          fixed-divider:
>> +            $ref: /schemas/types.yaml#/definitions/uint32
>> +
>> +          reg:
>> +            maxItems: 1
>> +
>> +        patternProperties:
>> +          "^[a-z0-9,_]+[clk,pll](@[a-f0-9]+)?$":
> 
> Similar issues here.

Yes, I will fix this regex too.

> 
>> +            type: object
>> +            additionalProperties: false
>> +
>> +            properties:
>> +              compatible:
>> +                enum:
>> +                  - altr,socfpga-perip-clk
>> +                  - altr,socfpga-gate-clk
>> +                  - altr,socfpga-a10-perip-clk
>> +                  - altr,socfpga-a10-gate-clk
>> +
>> +              "#clock-cells":
>> +                const: 0
>> +
>> +              clocks:
>> +                $ref: /schemas/types.yaml#/definitions/phandle-array
>> +                description: one or more phandles to input clock
>> +
>> +              clk-gate:
>> +                $ref: /schemas/types.yaml#/definitions/uint32-array
>> +                items:
>> +                  - description: gating register offset
>> +                  - description: bit index
>> +
>> +              div-reg:
>> +                $ref: /schemas/types.yaml#/definitions/uint32-array
>> +                items:
>> +                  - description: divider register offset
>> +                  - description: bit shift
>> +                  - description: bit width
>> +
>> +              fixed-divider:
>> +                $ref: /schemas/types.yaml#/definitions/uint32
> 
> As these properties are all just repeated, put them all under '$defs'
> and reference that in both places.

I will put the repeated properties under '$defs' and reference in both 
places.

> 
>> +
>> +              reg:
>> +                maxItems: 1
> 
> 'reg' goes after compatible.

Thanks for the reminder.

> 
>> +
>> +            required:
>> +              - compatible
>> +              - clocks
>> +              - "#clock-cells"
>> +
>> +        required:
>> +          - compatible
>> +          - "#clock-cells"
>> +
>>  required:
>>    - compatible
>> +  - reg
>>
>>  additionalProperties: false
>>
Thank you for the review,
Matthew Gerlach