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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <be02b9cd-803c-4aae-9420-ff3bf445efc1@baylibre.com>
Date: Wed, 21 May 2025 08:14:59 -0500
From: David Lechner <dlechner@...libre.com>
To: Krzysztof Kozlowski <krzk@...nel.org>
Cc: Michael Hennerich <michael.hennerich@...log.com>,
 Nuno Sá <nuno.sa@...log.com>,
 Trevor Gamblin <tgamblin@...libre.com>,
 Uwe Kleine-König <ukleinek@...nel.org>,
 Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
 Conor Dooley <conor+dt@...nel.org>, linux-pwm@...r.kernel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/3] dt-bindings: pwm: adi,axi-pwmgen: add external clock

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@...libre.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?

> 
> 
> 
>> +    then:
>> +      properties:
>> +        clocks:
>> +          minItems: 2
>> +    else:
>> +      properties:
>> +        clocks:
>> +          maxItems: 1
>> +
>>  examples:
>>    - |
>>      pwm@...00000 {
>>          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.

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

> 
> Best regards,
> Krzysztof
>