Re: [PATCH 2/3] dt-bindings: pwm: adi,axi-pwmgen: add external clock

[Krzysztof Kozlowski <krzk@kernel.org>]

On 21/05/2025 15:14, David Lechner wrote:
> On 5/21/25 5:09 AM, Krzysztof Kozlowski wrote:
>> On Tue, May 20, 2025 at 04:00:45PM GMT, David Lechner wrote:
>>> Add external clock to the schema.
>>>
>>> The AXI PWMGEN IP block has a compile option ASYNC_CLK_EN that allows
>>> the use of an external clock for the PWM output separate from the AXI
>>> clock that runs the peripheral.
>>>
>>> In these cases, we should specify both clocks in the device tree. The
>>> intention here is that if you specify both clocks, then you include the
>>> clock-names property and if you don't have an external clock, then you
>>> omit the clock-names property.
>>>
>>> There can't be more than one allOf: in the top level of the schema, so
>>> it is stolen from $ref since it isn't needed there and used for the
>>> more typical case of the if statement (even though technically it isn't
>>> needed there either at this time).
>>>
>>> Signed-off-by: David Lechner <dlechner@baylibre.com>
>>> ---
>>>  .../devicetree/bindings/pwm/adi,axi-pwmgen.yaml    | 26 ++++++++++++++++++----
>>>  1 file changed, 22 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/pwm/adi,axi-pwmgen.yaml b/Documentation/devicetree/bindings/pwm/adi,axi-pwmgen.yaml
>>> index bc44381692054f647a160a6573dae4cff2ee3f31..90f702a5cd80bd7d62e2436b2eed44314ab4fd53 100644
>>> --- a/Documentation/devicetree/bindings/pwm/adi,axi-pwmgen.yaml
>>> +++ b/Documentation/devicetree/bindings/pwm/adi,axi-pwmgen.yaml
>>> @@ -16,8 +16,7 @@ description:
>>>  
>>>    https://analogdevicesinc.github.io/hdl/library/axi_pwm_gen/index.html
>>>  
>>> -allOf:
>>> -  - $ref: pwm.yaml#
>>> +$ref: pwm.yaml#
>>>  
>>>  properties:
>>>    compatible:
>>> @@ -30,7 +29,13 @@ properties:
>>>      const: 3
>>>  
>>>    clocks:
>>> -    maxItems: 1
>>> +    minItems: 1
>>> +    maxItems: 2
>>> +
>>> +  clock-names:
>>> +    items:
>>> +      - const: axi
>>> +      - const: ext
>>>  
>>>  required:
>>>    - reg
>>> @@ -38,11 +43,24 @@ required:
>>>  
>>>  unevaluatedProperties: false
>>>  
>>> +allOf:
>>> +  - if:
>>> +      required: [clock-names]
>>
>>
>> No, don't do that. If you want clock-names, just add them for both
>> cases. Otherwise, just describe items in clocks and no need for
>> clock-names.
> 
> Would it be OK then to make clock-names required and just let the
> driver still handle one clocks, no clock-names for backwards compatibility?

So just don't make it required.

> 
>>
>>
>>
>>> +    then:
>>> +      properties:
>>> +        clocks:
>>> +          minItems: 2
>>> +    else:
>>> +      properties:
>>> +        clocks:
>>> +          maxItems: 1
>>> +
>>>  examples:
>>>    - |
>>>      pwm@44b00000 {
>>>          compatible = "adi,axi-pwmgen-2.00.a";
>>>          reg = <0x44b00000 0x1000>;
>>> -        clocks = <&spi_clk>;
>>> +        clocks = <&fpga_clk>, <&spi_clk>;
>>
>> What was the clock[0] before? Axi, right, so SPI_CLK. Now FPGA is the
>> AXI_CLK? This feels like clock order reversed.
> 
> The problem being fixed here is that since there was only one clock in
> the binding, existing .dts files have either have the spi_clock or
> the FPGA/AXI clock. So the one clock could be either and there are
> existing .dtbs out in the world with both cases.

No problem like that was explained in commit msg. Nevertheless driver
assumed the first clock is the SPI, didn't it? So that's your ABI, even
if binding was not conclusive here.


> 
> But we could consider reversing this so that if someone uses the new
> bindings with an old kernel, then it would still work.

You cannot use new bindings with old kernel. How would that work? Put
YAML file there? Nothing would change.

Binding is supposed to be complete for exactly this reason. You cannot
change it afterwards without breaking users.

Best regards,
Krzysztof