[PATCH 0/3] pwm: axi-pwmgen: add external clock

[PATCH 0/3] pwm: axi-pwmgen: add external clock

[David Lechner]

When we created the driver for the AXI PWMGEN IP block, we overlooked
the fact that it can optionally be configured to use an external clock
in addition to the AXI bus clock. This is easy to miss in testing
because the bus clock is always on because it is driving other
peripherals as well.

Up to now, users were specifying the external clock if there was one and
the AXI bus clock otherwise. But the proper way to do this is to would
be to always specify the bus clock and only specify the external clock
if the IP block has been configured to use it.

To support this, we extend the bindings to allow 1 or 2 clocks and
modify the driver to handle both cases.

---
David Lechner (3):
      dt-bindings: pwm: adi,axi-pwmgen: update documentation link
      dt-bindings: pwm: adi,axi-pwmgen: add external clock
      pwm: axi-pwmgen: add support for external clock

 .../devicetree/bindings/pwm/adi,axi-pwmgen.yaml    | 28 ++++++++++++++++++----
 drivers/pwm/pwm-axi-pwmgen.c                       | 23 +++++++++++++++---
 2 files changed, 43 insertions(+), 8 deletions(-)
---
base-commit: 484803582c77061b470ac64a634f25f89715be3f
change-id: 20250515-pwm-axi-pwmgen-add-external-clock-0364fbdf809b

Best regards,
-- 
David Lechner <dlechner@baylibre.com>

[PATCH 1/3] dt-bindings: pwm: adi,axi-pwmgen: update documentation link

[David Lechner]

Change the documentation link to point to the location with the most
up-to-date information.

Signed-off-by: David Lechner <dlechner@baylibre.com>
---
 Documentation/devicetree/bindings/pwm/adi,axi-pwmgen.yaml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/pwm/adi,axi-pwmgen.yaml b/Documentation/devicetree/bindings/pwm/adi,axi-pwmgen.yaml
index 45e112d0efb4663bc7fbb3a25a12d66aa8b7492d..bc44381692054f647a160a6573dae4cff2ee3f31 100644
--- a/Documentation/devicetree/bindings/pwm/adi,axi-pwmgen.yaml
+++ b/Documentation/devicetree/bindings/pwm/adi,axi-pwmgen.yaml
@@ -14,7 +14,7 @@ description:
   The Analog Devices AXI PWM generator can generate PWM signals
   with variable pulse width and period.
 
-  https://wiki.analog.com/resources/fpga/docs/axi_pwm_gen
+  https://analogdevicesinc.github.io/hdl/library/axi_pwm_gen/index.html
 
 allOf:
   - $ref: pwm.yaml#

-- 
2.43.0

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

[David Lechner]

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]
+    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>;
+        clock-names = "axi", "ext";
         #pwm-cells = <3>;
     };

-- 
2.43.0

[PATCH 3/3] pwm: axi-pwmgen: add support for external clock

[David Lechner]

Add support for external clock to the AXI PWM generator driver.

In most cases, there is a separate external clock that drives the PWM
output separate from the peripheral clock. This allows enabling both
clocks.

Signed-off-by: David Lechner <dlechner@baylibre.com>
---
 drivers/pwm/pwm-axi-pwmgen.c | 23 ++++++++++++++++++++---
 1 file changed, 20 insertions(+), 3 deletions(-)

diff --git a/drivers/pwm/pwm-axi-pwmgen.c b/drivers/pwm/pwm-axi-pwmgen.c
index 4337c8f5acf055fc87dc134f2a70b99b0cb5ede6..67992a7561ec0440b1c1fa327f844a0602872771 100644
--- a/drivers/pwm/pwm-axi-pwmgen.c
+++ b/drivers/pwm/pwm-axi-pwmgen.c
@@ -280,9 +280,26 @@ static int axi_pwmgen_probe(struct platform_device *pdev)
 	ddata = pwmchip_get_drvdata(chip);
 	ddata->regmap = regmap;
 
-	clk = devm_clk_get_enabled(dev, NULL);
-	if (IS_ERR(clk))
-		return dev_err_probe(dev, PTR_ERR(clk), "failed to get clock\n");
+	/* When clock-names is present, there is a separate ext clock. */
+	if (device_property_present(dev, "clock-names")) {
+		struct clk *axi_clk;
+
+		axi_clk = devm_clk_get_enabled(dev, "axi");
+		if (IS_ERR(axi_clk))
+			return dev_err_probe(dev, PTR_ERR(axi_clk),
+					     "failed to get axi clock\n");
+
+		clk = devm_clk_get_enabled(dev, "ext");
+		if (IS_ERR(clk))
+			return dev_err_probe(dev, PTR_ERR(clk),
+					     "failed to get ext clock\n");
+	} else {
+		/* Otherwise, a single clock does everything. */
+		clk = devm_clk_get_enabled(dev, NULL);
+		if (IS_ERR(clk))
+			return dev_err_probe(dev, PTR_ERR(clk),
+					     "failed to get clock\n");
+	}
 
 	ret = devm_clk_rate_exclusive_get(dev, clk);
 	if (ret)

-- 
2.43.0

Re: [PATCH 3/3] pwm: axi-pwmgen: add support for external clock

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 2634 bytes --]

On Tue, May 20, 2025 at 04:00:46PM -0500, David Lechner wrote:
> Add support for external clock to the AXI PWM generator driver.
> 
> In most cases, there is a separate external clock that drives the PWM
> output separate from the peripheral clock. This allows enabling both
> clocks.
> 
> Signed-off-by: David Lechner <dlechner@baylibre.com>
> ---
>  drivers/pwm/pwm-axi-pwmgen.c | 23 ++++++++++++++++++++---
>  1 file changed, 20 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-axi-pwmgen.c b/drivers/pwm/pwm-axi-pwmgen.c
> index 4337c8f5acf055fc87dc134f2a70b99b0cb5ede6..67992a7561ec0440b1c1fa327f844a0602872771 100644
> --- a/drivers/pwm/pwm-axi-pwmgen.c
> +++ b/drivers/pwm/pwm-axi-pwmgen.c
> @@ -280,9 +280,26 @@ static int axi_pwmgen_probe(struct platform_device *pdev)
>  	ddata = pwmchip_get_drvdata(chip);
>  	ddata->regmap = regmap;
>  
> -	clk = devm_clk_get_enabled(dev, NULL);
> -	if (IS_ERR(clk))
> -		return dev_err_probe(dev, PTR_ERR(clk), "failed to get clock\n");
> +	/* When clock-names is present, there is a separate ext clock. */
> +	if (device_property_present(dev, "clock-names")) {
> +		struct clk *axi_clk;
> +
> +		axi_clk = devm_clk_get_enabled(dev, "axi");
> +		if (IS_ERR(axi_clk))
> +			return dev_err_probe(dev, PTR_ERR(axi_clk),
> +					     "failed to get axi clock\n");
> +
> +		clk = devm_clk_get_enabled(dev, "ext");
> +		if (IS_ERR(clk))
> +			return dev_err_probe(dev, PTR_ERR(clk),
> +					     "failed to get ext clock\n");
> +	} else {
> +		/* Otherwise, a single clock does everything. */
> +		clk = devm_clk_get_enabled(dev, NULL);
> +		if (IS_ERR(clk))
> +			return dev_err_probe(dev, PTR_ERR(clk),
> +					     "failed to get clock\n");
> +	}

Can you achieve the same effect with the (IMHO slightly nicer but
hand-crafted) following patch:

 	ddata = pwmchip_get_drvdata(chip);
 	ddata->regmap = regmap;
 
-	clk = devm_clk_get_enabled(dev, NULL);
-	if (IS_ERR(clk))
-		return dev_err_probe(dev, PTR_ERR(clk), "failed to get clock\n");
+	axi_clk = devm_clk_get_enabled(dev, "axi");
+	if (IS_ERR(axi_clk))
+		return dev_err_probe(dev, PTR_ERR(axi_clk), "failed to get axi clock\n");

+	clk = devm_clk_get_enabled_optional(dev, "ext");
+	if (IS_ERR(clk))
+		return dev_err_probe(dev, PTR_ERR(clk), "failed to get ext clock\n");
+	}

 	ret = devm_clk_rate_exclusive_get(dev, clk);
 	if (ret)

with the only side effect that for machines with a single clk we get
axi_clk == clk and it's enabled twice.

Another upside is that

	clock-names = "axi";
	clocks = <...>;

still works.

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Krzysztof Kozlowski]

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.



> +    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.

Best regards,
Krzysztof

Re: [PATCH 3/3] pwm: axi-pwmgen: add support for external clock

[Krzysztof Kozlowski]

On Tue, May 20, 2025 at 04:00:46PM GMT, David Lechner wrote:
> Add support for external clock to the AXI PWM generator driver.
> 
> In most cases, there is a separate external clock that drives the PWM
> output separate from the peripheral clock. This allows enabling both
> clocks.
> 
> Signed-off-by: David Lechner <dlechner@baylibre.com>
> ---
>  drivers/pwm/pwm-axi-pwmgen.c | 23 ++++++++++++++++++++---
>  1 file changed, 20 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-axi-pwmgen.c b/drivers/pwm/pwm-axi-pwmgen.c
> index 4337c8f5acf055fc87dc134f2a70b99b0cb5ede6..67992a7561ec0440b1c1fa327f844a0602872771 100644
> --- a/drivers/pwm/pwm-axi-pwmgen.c
> +++ b/drivers/pwm/pwm-axi-pwmgen.c
> @@ -280,9 +280,26 @@ static int axi_pwmgen_probe(struct platform_device *pdev)
>  	ddata = pwmchip_get_drvdata(chip);
>  	ddata->regmap = regmap;
>  
> -	clk = devm_clk_get_enabled(dev, NULL);
> -	if (IS_ERR(clk))
> -		return dev_err_probe(dev, PTR_ERR(clk), "failed to get clock\n");
> +	/* When clock-names is present, there is a separate ext clock. */
> +	if (device_property_present(dev, "clock-names")) {

No. List is ordered, you do not need such dance at all.

> +		struct clk *axi_clk;
> +
> +		axi_clk = devm_clk_get_enabled(dev, "axi");
> +		if (IS_ERR(axi_clk))
> +			return dev_err_probe(dev, PTR_ERR(axi_clk),
> +					     "failed to get axi clock\n");
> +
> +		clk = devm_clk_get_enabled(dev, "ext");

So that's messing the order, which confirms my question from the
binding.

No. List has a strict order, you cannot change it just because you want
to add optional clock.

Best regards,
Krzysztof

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

[David Lechner]

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?

> 
> 
> 
>> +    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.

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
> 

Re: [PATCH 3/3] pwm: axi-pwmgen: add support for external clock

[David Lechner]

On 5/21/25 4:22 AM, Uwe Kleine-König wrote:
> On Tue, May 20, 2025 at 04:00:46PM -0500, David Lechner wrote:
>> Add support for external clock to the AXI PWM generator driver.
>>
>> In most cases, there is a separate external clock that drives the PWM
>> output separate from the peripheral clock. This allows enabling both
>> clocks.
>>
>> Signed-off-by: David Lechner <dlechner@baylibre.com>
>> ---
>>  drivers/pwm/pwm-axi-pwmgen.c | 23 ++++++++++++++++++++---
>>  1 file changed, 20 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/pwm/pwm-axi-pwmgen.c b/drivers/pwm/pwm-axi-pwmgen.c
>> index 4337c8f5acf055fc87dc134f2a70b99b0cb5ede6..67992a7561ec0440b1c1fa327f844a0602872771 100644
>> --- a/drivers/pwm/pwm-axi-pwmgen.c
>> +++ b/drivers/pwm/pwm-axi-pwmgen.c
>> @@ -280,9 +280,26 @@ static int axi_pwmgen_probe(struct platform_device *pdev)
>>  	ddata = pwmchip_get_drvdata(chip);
>>  	ddata->regmap = regmap;
>>  
>> -	clk = devm_clk_get_enabled(dev, NULL);
>> -	if (IS_ERR(clk))
>> -		return dev_err_probe(dev, PTR_ERR(clk), "failed to get clock\n");
>> +	/* When clock-names is present, there is a separate ext clock. */
>> +	if (device_property_present(dev, "clock-names")) {
>> +		struct clk *axi_clk;
>> +
>> +		axi_clk = devm_clk_get_enabled(dev, "axi");
>> +		if (IS_ERR(axi_clk))
>> +			return dev_err_probe(dev, PTR_ERR(axi_clk),
>> +					     "failed to get axi clock\n");
>> +
>> +		clk = devm_clk_get_enabled(dev, "ext");
>> +		if (IS_ERR(clk))
>> +			return dev_err_probe(dev, PTR_ERR(clk),
>> +					     "failed to get ext clock\n");
>> +	} else {
>> +		/* Otherwise, a single clock does everything. */
>> +		clk = devm_clk_get_enabled(dev, NULL);
>> +		if (IS_ERR(clk))
>> +			return dev_err_probe(dev, PTR_ERR(clk),
>> +					     "failed to get clock\n");
>> +	}
> 
> Can you achieve the same effect with the (IMHO slightly nicer but
> hand-crafted) following patch:
> 
>  	ddata = pwmchip_get_drvdata(chip);
>  	ddata->regmap = regmap;
>  
> -	clk = devm_clk_get_enabled(dev, NULL);
> -	if (IS_ERR(clk))
> -		return dev_err_probe(dev, PTR_ERR(clk), "failed to get clock\n");
> +	axi_clk = devm_clk_get_enabled(dev, "axi");
> +	if (IS_ERR(axi_clk))
> +		return dev_err_probe(dev, PTR_ERR(axi_clk), "failed to get axi clock\n");
> 
> +	clk = devm_clk_get_enabled_optional(dev, "ext");
> +	if (IS_ERR(clk))
> +		return dev_err_probe(dev, PTR_ERR(clk), "failed to get ext clock\n");
> +	}

The trouble with this is that it would not work with existing .dtbs
that don't have clock-names set. I think it would need to be more like this:


	axi_clk = devm_clk_get_enabled(dev, NULL);
	if (IS_ERR(axi_clk))
		return dev_err_probe(dev, PTR_ERR(axi_clk), "failed to get axi clock\n");

	clk = devm_clk_get_enabled_optional(dev, "ext");
	if (IS_ERR(clk))
		return dev_err_probe(dev, PTR_ERR(clk), "failed to get ext clock\n");

	if (!clk)
		clk = axi_clk


> 
>  	ret = devm_clk_rate_exclusive_get(dev, clk);
>  	if (ret)
> 
> with the only side effect that for machines with a single clk we get
> axi_clk == clk and it's enabled twice.
> 
> Another upside is that
> 
> 	clock-names = "axi";
> 	clocks = <...>;
> 
> still works.
> 
> Best regards
> Uwe

Re: [PATCH 3/3] pwm: axi-pwmgen: add support for external clock

[David Lechner]

On 5/21/25 5:10 AM, Krzysztof Kozlowski wrote:
> On Tue, May 20, 2025 at 04:00:46PM GMT, David Lechner wrote:
>> Add support for external clock to the AXI PWM generator driver.
>>
>> In most cases, there is a separate external clock that drives the PWM
>> output separate from the peripheral clock. This allows enabling both
>> clocks.
>>
>> Signed-off-by: David Lechner <dlechner@baylibre.com>
>> ---
>>  drivers/pwm/pwm-axi-pwmgen.c | 23 ++++++++++++++++++++---
>>  1 file changed, 20 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/pwm/pwm-axi-pwmgen.c b/drivers/pwm/pwm-axi-pwmgen.c
>> index 4337c8f5acf055fc87dc134f2a70b99b0cb5ede6..67992a7561ec0440b1c1fa327f844a0602872771 100644
>> --- a/drivers/pwm/pwm-axi-pwmgen.c
>> +++ b/drivers/pwm/pwm-axi-pwmgen.c
>> @@ -280,9 +280,26 @@ static int axi_pwmgen_probe(struct platform_device *pdev)
>>  	ddata = pwmchip_get_drvdata(chip);
>>  	ddata->regmap = regmap;
>>  
>> -	clk = devm_clk_get_enabled(dev, NULL);
>> -	if (IS_ERR(clk))
>> -		return dev_err_probe(dev, PTR_ERR(clk), "failed to get clock\n");
>> +	/* When clock-names is present, there is a separate ext clock. */
>> +	if (device_property_present(dev, "clock-names")) {
> 
> No. List is ordered, you do not need such dance at all.

I should have added more to the comment here. This is also needed for
backwards compatibility where only what should be the "ext" clock is
given as clocks = <&spi_clk>; and the AXI clock was missing.

> 
>> +		struct clk *axi_clk;
>> +
>> +		axi_clk = devm_clk_get_enabled(dev, "axi");
>> +		if (IS_ERR(axi_clk))
>> +			return dev_err_probe(dev, PTR_ERR(axi_clk),
>> +					     "failed to get axi clock\n");
>> +
>> +		clk = devm_clk_get_enabled(dev, "ext");
> 
> So that's messing the order, which confirms my question from the
> binding.
> 
> No. List has a strict order, you cannot change it just because you want
> to add optional clock.
> 
> Best regards,
> Krzysztof
> 

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

[Krzysztof Kozlowski]

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

Re: [PATCH 3/3] pwm: axi-pwmgen: add support for external clock

[Krzysztof Kozlowski]

On 21/05/2025 15:23, David Lechner wrote:
> On 5/21/25 5:10 AM, Krzysztof Kozlowski wrote:
>> On Tue, May 20, 2025 at 04:00:46PM GMT, David Lechner wrote:
>>> Add support for external clock to the AXI PWM generator driver.
>>>
>>> In most cases, there is a separate external clock that drives the PWM
>>> output separate from the peripheral clock. This allows enabling both
>>> clocks.
>>>
>>> Signed-off-by: David Lechner <dlechner@baylibre.com>
>>> ---
>>>  drivers/pwm/pwm-axi-pwmgen.c | 23 ++++++++++++++++++++---
>>>  1 file changed, 20 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/pwm/pwm-axi-pwmgen.c b/drivers/pwm/pwm-axi-pwmgen.c
>>> index 4337c8f5acf055fc87dc134f2a70b99b0cb5ede6..67992a7561ec0440b1c1fa327f844a0602872771 100644
>>> --- a/drivers/pwm/pwm-axi-pwmgen.c
>>> +++ b/drivers/pwm/pwm-axi-pwmgen.c
>>> @@ -280,9 +280,26 @@ static int axi_pwmgen_probe(struct platform_device *pdev)
>>>  	ddata = pwmchip_get_drvdata(chip);
>>>  	ddata->regmap = regmap;
>>>  
>>> -	clk = devm_clk_get_enabled(dev, NULL);
>>> -	if (IS_ERR(clk))
>>> -		return dev_err_probe(dev, PTR_ERR(clk), "failed to get clock\n");
>>> +	/* When clock-names is present, there is a separate ext clock. */
>>> +	if (device_property_present(dev, "clock-names")) {
>>
>> No. List is ordered, you do not need such dance at all.
> 
> I should have added more to the comment here. This is also needed for
> backwards compatibility where only what should be the "ext" clock is
> given as clocks = <&spi_clk>; and the AXI clock was missing.

I do not see this needed at all. That's already handled by old code. You
only need to take new optional, second clock - axi clk. You take it by
index.

Best regards,
Krzysztof

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

[David Lechner]

On 5/21/25 8:28 AM, Krzysztof Kozlowski wrote:
> 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

You are right. I explained it in the cover letter, but failed to repeat
that in this commit message.

> assumed the first clock is the SPI, didn't it? So that's your ABI, even
> if binding was not conclusive here.

Not quite. The driver (before this series) assumes that the clock
is the SPI clock ("ext") if the HDL was compiled with ASYNC_CLK_EN=1
or the AXI clock if the HDL was compiled with ASYNC_CLK_EN=0 (in this
case, there is no "ext"/SPI clock).

>>
>> 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

Re: [PATCH 3/3] pwm: axi-pwmgen: add support for external clock

[David Lechner]

On 5/21/25 8:30 AM, Krzysztof Kozlowski wrote:
> On 21/05/2025 15:23, David Lechner wrote:
>> On 5/21/25 5:10 AM, Krzysztof Kozlowski wrote:
>>> On Tue, May 20, 2025 at 04:00:46PM GMT, David Lechner wrote:
>>>> Add support for external clock to the AXI PWM generator driver.
>>>>
>>>> In most cases, there is a separate external clock that drives the PWM
>>>> output separate from the peripheral clock. This allows enabling both
>>>> clocks.
>>>>
>>>> Signed-off-by: David Lechner <dlechner@baylibre.com>
>>>> ---
>>>>  drivers/pwm/pwm-axi-pwmgen.c | 23 ++++++++++++++++++++---
>>>>  1 file changed, 20 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/pwm/pwm-axi-pwmgen.c b/drivers/pwm/pwm-axi-pwmgen.c
>>>> index 4337c8f5acf055fc87dc134f2a70b99b0cb5ede6..67992a7561ec0440b1c1fa327f844a0602872771 100644
>>>> --- a/drivers/pwm/pwm-axi-pwmgen.c
>>>> +++ b/drivers/pwm/pwm-axi-pwmgen.c
>>>> @@ -280,9 +280,26 @@ static int axi_pwmgen_probe(struct platform_device *pdev)
>>>>  	ddata = pwmchip_get_drvdata(chip);
>>>>  	ddata->regmap = regmap;
>>>>  
>>>> -	clk = devm_clk_get_enabled(dev, NULL);
>>>> -	if (IS_ERR(clk))
>>>> -		return dev_err_probe(dev, PTR_ERR(clk), "failed to get clock\n");
>>>> +	/* When clock-names is present, there is a separate ext clock. */
>>>> +	if (device_property_present(dev, "clock-names")) {
>>>
>>> No. List is ordered, you do not need such dance at all.
>>
>> I should have added more to the comment here. This is also needed for
>> backwards compatibility where only what should be the "ext" clock is
>> given as clocks = <&spi_clk>; and the AXI clock was missing.
> 
> I do not see this needed at all. That's already handled by old code. You
> only need to take new optional, second clock - axi clk. You take it by
> index.

Except that it is the "ext" clock that is supposed to be optional and
the "axi" clock is supposed to be required.

> 
> Best regards,
> Krzysztof

Re: [PATCH 3/3] pwm: axi-pwmgen: add support for external clock

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1514 bytes --]

Hello David,

On Wed, May 21, 2025 at 08:19:51AM -0500, David Lechner wrote:
> On 5/21/25 4:22 AM, Uwe Kleine-König wrote:
> > Can you achieve the same effect with the (IMHO slightly nicer but
> > hand-crafted) following patch:
> > 
> >  	ddata = pwmchip_get_drvdata(chip);
> >  	ddata->regmap = regmap;
> >  
> > -	clk = devm_clk_get_enabled(dev, NULL);
> > -	if (IS_ERR(clk))
> > -		return dev_err_probe(dev, PTR_ERR(clk), "failed to get clock\n");
> > +	axi_clk = devm_clk_get_enabled(dev, "axi");
> > +	if (IS_ERR(axi_clk))
> > +		return dev_err_probe(dev, PTR_ERR(axi_clk), "failed to get axi clock\n");
> > 
> > +	clk = devm_clk_get_enabled_optional(dev, "ext");
> > +	if (IS_ERR(clk))
> > +		return dev_err_probe(dev, PTR_ERR(clk), "failed to get ext clock\n");
> > +	}
> 
> The trouble with this is that it would not work with existing .dtbs
> that don't have clock-names set. I think it would need to be more like this:
> 
> 
> 	axi_clk = devm_clk_get_enabled(dev, NULL);
> 	if (IS_ERR(axi_clk))
> 		return dev_err_probe(dev, PTR_ERR(axi_clk), "failed to get axi clock\n");
> 
> 	clk = devm_clk_get_enabled_optional(dev, "ext");
> 	if (IS_ERR(clk))
> 		return dev_err_probe(dev, PTR_ERR(clk), "failed to get ext clock\n");
> 
> 	if (!clk)
> 		clk = axi_clk
> 

If there are no clock-names, the parameter is ignored. (I didn't test,
only quickly checked the code.) So passing "axi" instead of NULL should
work and yield a more robust solution.

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 3/3] pwm: axi-pwmgen: add support for external clock

[David Lechner]

On 5/21/25 8:54 AM, Uwe Kleine-König wrote:
> Hello David,
> 
> On Wed, May 21, 2025 at 08:19:51AM -0500, David Lechner wrote:
>> On 5/21/25 4:22 AM, Uwe Kleine-König wrote:
>>> Can you achieve the same effect with the (IMHO slightly nicer but
>>> hand-crafted) following patch:
>>>
>>>  	ddata = pwmchip_get_drvdata(chip);
>>>  	ddata->regmap = regmap;
>>>  
>>> -	clk = devm_clk_get_enabled(dev, NULL);
>>> -	if (IS_ERR(clk))
>>> -		return dev_err_probe(dev, PTR_ERR(clk), "failed to get clock\n");
>>> +	axi_clk = devm_clk_get_enabled(dev, "axi");
>>> +	if (IS_ERR(axi_clk))
>>> +		return dev_err_probe(dev, PTR_ERR(axi_clk), "failed to get axi clock\n");
>>>
>>> +	clk = devm_clk_get_enabled_optional(dev, "ext");
>>> +	if (IS_ERR(clk))
>>> +		return dev_err_probe(dev, PTR_ERR(clk), "failed to get ext clock\n");
>>> +	}
>>
>> The trouble with this is that it would not work with existing .dtbs
>> that don't have clock-names set. I think it would need to be more like this:
>>
>>
>> 	axi_clk = devm_clk_get_enabled(dev, NULL);
>> 	if (IS_ERR(axi_clk))
>> 		return dev_err_probe(dev, PTR_ERR(axi_clk), "failed to get axi clock\n");
>>
>> 	clk = devm_clk_get_enabled_optional(dev, "ext");
>> 	if (IS_ERR(clk))
>> 		return dev_err_probe(dev, PTR_ERR(clk), "failed to get ext clock\n");
>>
>> 	if (!clk)
>> 		clk = axi_clk
>>
> 
> If there are no clock-names, the parameter is ignored. (I didn't test,
> only quickly checked the code.) So passing "axi" instead of NULL should
> work and yield a more robust solution.
> 
> Best regards
> Uwe


I didn't know that. So with your suggestion, I guess we would get/enable
the same clock twice. I guess that doesn't hurt anything. I will try it.

Re: [PATCH 3/3] pwm: axi-pwmgen: add support for external clock

[Nuno Sá]

On Wed, 2025-05-21 at 15:54 +0200, Uwe Kleine-König wrote:
> Hello David,
> 
> On Wed, May 21, 2025 at 08:19:51AM -0500, David Lechner wrote:
> > On 5/21/25 4:22 AM, Uwe Kleine-König wrote:
> > > Can you achieve the same effect with the (IMHO slightly nicer but
> > > hand-crafted) following patch:
> > > 
> > >  	ddata = pwmchip_get_drvdata(chip);
> > >  	ddata->regmap = regmap;
> > >  
> > > -	clk = devm_clk_get_enabled(dev, NULL);
> > > -	if (IS_ERR(clk))
> > > -		return dev_err_probe(dev, PTR_ERR(clk), "failed to get
> > > clock\n");
> > > +	axi_clk = devm_clk_get_enabled(dev, "axi");
> > > +	if (IS_ERR(axi_clk))
> > > +		return dev_err_probe(dev, PTR_ERR(axi_clk), "failed to
> > > get axi clock\n");
> > > 
> > > +	clk = devm_clk_get_enabled_optional(dev, "ext");
> > > +	if (IS_ERR(clk))
> > > +		return dev_err_probe(dev, PTR_ERR(clk), "failed to get
> > > ext clock\n");
> > > +	}
> > 
> > The trouble with this is that it would not work with existing .dtbs
> > that don't have clock-names set. I think it would need to be more like this:
> > 
> > 
> > 	axi_clk = devm_clk_get_enabled(dev, NULL);
> > 	if (IS_ERR(axi_clk))
> > 		return dev_err_probe(dev, PTR_ERR(axi_clk), "failed to get
> > axi clock\n");
> > 
> > 	clk = devm_clk_get_enabled_optional(dev, "ext");
> > 	if (IS_ERR(clk))
> > 		return dev_err_probe(dev, PTR_ERR(clk), "failed to get ext
> > clock\n");
> > 
> > 	if (!clk)
> > 		clk = axi_clk
> > 
> 
> If there are no clock-names, the parameter is ignored. (I didn't test,
> only quickly checked the code.) So passing "axi" instead of NULL should
> work and yield a more robust solution.
> 
> 

Are you sure? If there are no clock-names and you pass an id, you should get an
error back:

https://elixir.bootlin.com/linux/v6.14.7/source/drivers/clk/clk.c#L5198


I know it's not exactly the same we're discussing but of_property_match_string()
return -EINVAL if the property is not found which leads to an index < 0 and thus
of_parse_phandle_with_args() also returns an error back.

I think I'm not missing anything but it's always a possibility.

- Nuno Sá

Re: [PATCH 3/3] pwm: axi-pwmgen: add support for external clock

[Nuno Sá]

On Wed, 2025-05-21 at 09:12 -0500, David Lechner wrote:
> On 5/21/25 8:54 AM, Uwe Kleine-König wrote:
> > Hello David,
> > 
> > On Wed, May 21, 2025 at 08:19:51AM -0500, David Lechner wrote:
> > > On 5/21/25 4:22 AM, Uwe Kleine-König wrote:
> > > > Can you achieve the same effect with the (IMHO slightly nicer but
> > > > hand-crafted) following patch:
> > > > 
> > > >  	ddata = pwmchip_get_drvdata(chip);
> > > >  	ddata->regmap = regmap;
> > > >  
> > > > -	clk = devm_clk_get_enabled(dev, NULL);
> > > > -	if (IS_ERR(clk))
> > > > -		return dev_err_probe(dev, PTR_ERR(clk), "failed to get
> > > > clock\n");
> > > > +	axi_clk = devm_clk_get_enabled(dev, "axi");
> > > > +	if (IS_ERR(axi_clk))
> > > > +		return dev_err_probe(dev, PTR_ERR(axi_clk), "failed to
> > > > get axi clock\n");
> > > > 
> > > > +	clk = devm_clk_get_enabled_optional(dev, "ext");
> > > > +	if (IS_ERR(clk))
> > > > +		return dev_err_probe(dev, PTR_ERR(clk), "failed to get
> > > > ext clock\n");
> > > > +	}
> > > 
> > > The trouble with this is that it would not work with existing .dtbs
> > > that don't have clock-names set. I think it would need to be more like
> > > this:
> > > 
> > > 
> > > 	axi_clk = devm_clk_get_enabled(dev, NULL);
> > > 	if (IS_ERR(axi_clk))
> > > 		return dev_err_probe(dev, PTR_ERR(axi_clk), "failed to
> > > get axi clock\n");
> > > 
> > > 	clk = devm_clk_get_enabled_optional(dev, "ext");
> > > 	if (IS_ERR(clk))
> > > 		return dev_err_probe(dev, PTR_ERR(clk), "failed to get
> > > ext clock\n");
> > > 
> > > 	if (!clk)
> > > 		clk = axi_clk
> > > 
> > 
> > If there are no clock-names, the parameter is ignored. (I didn't test,
> > only quickly checked the code.) So passing "axi" instead of NULL should
> > work and yield a more robust solution.
> > 
> > Best regards
> > Uwe
> 
> 
> I didn't know that. So with your suggestion, I guess we would get/enable
> the same clock twice. I guess that doesn't hurt anything. I will try it.

So, in the axi-dac we ended up doing this if you recall:

https://elixir.bootlin.com/linux/v6.15-rc7/source/drivers/iio/dac/adi-axi-dac.c#L837

But I do not think you need that here. I that what you suggested (with the first
call having id as NULL) should work fine.

I'm starting to think that always having clock-names just makes things easier
and more extendable (though the recommendation is to not have it in case there's
only one clock). Not the first time I see this (or go through this kind of
stuff). I had a similar situation in the axi-clkgen IP and IIRC, me and Conor
just agreed in making clock-names required and to handle backward compatibility
in the driver:
https://elixir.bootlin.com/linux/v6.14.7/source/drivers/clk/clk-axi-clkgen.c#L534

I'm actually now re-testing because I think I actually have a bug in there :)

- Nuno Sá

Re: [PATCH 3/3] pwm: axi-pwmgen: add support for external clock

[David Lechner]

On 5/21/25 9:22 AM, Nuno Sá wrote:
> On Wed, 2025-05-21 at 15:54 +0200, Uwe Kleine-König wrote:
>> Hello David,
>>
>> On Wed, May 21, 2025 at 08:19:51AM -0500, David Lechner wrote:
>>> On 5/21/25 4:22 AM, Uwe Kleine-König wrote:
>>>> Can you achieve the same effect with the (IMHO slightly nicer but
>>>> hand-crafted) following patch:
>>>>
>>>>  	ddata = pwmchip_get_drvdata(chip);
>>>>  	ddata->regmap = regmap;
>>>>  
>>>> -	clk = devm_clk_get_enabled(dev, NULL);
>>>> -	if (IS_ERR(clk))
>>>> -		return dev_err_probe(dev, PTR_ERR(clk), "failed to get
>>>> clock\n");
>>>> +	axi_clk = devm_clk_get_enabled(dev, "axi");
>>>> +	if (IS_ERR(axi_clk))
>>>> +		return dev_err_probe(dev, PTR_ERR(axi_clk), "failed to
>>>> get axi clock\n");
>>>>
>>>> +	clk = devm_clk_get_enabled_optional(dev, "ext");
>>>> +	if (IS_ERR(clk))
>>>> +		return dev_err_probe(dev, PTR_ERR(clk), "failed to get
>>>> ext clock\n");
>>>> +	}
>>>
>>> The trouble with this is that it would not work with existing .dtbs
>>> that don't have clock-names set. I think it would need to be more like this:
>>>
>>>
>>> 	axi_clk = devm_clk_get_enabled(dev, NULL);
>>> 	if (IS_ERR(axi_clk))
>>> 		return dev_err_probe(dev, PTR_ERR(axi_clk), "failed to get
>>> axi clock\n");
>>>
>>> 	clk = devm_clk_get_enabled_optional(dev, "ext");
>>> 	if (IS_ERR(clk))
>>> 		return dev_err_probe(dev, PTR_ERR(clk), "failed to get ext
>>> clock\n");
>>>
>>> 	if (!clk)
>>> 		clk = axi_clk
>>>
>>
>> If there are no clock-names, the parameter is ignored. (I didn't test,
>> only quickly checked the code.) So passing "axi" instead of NULL should
>> work and yield a more robust solution.
>>
>>
> 
> Are you sure? If there are no clock-names and you pass an id, you should get an
> error back:
> 
> https://elixir.bootlin.com/linux/v6.14.7/source/drivers/clk/clk.c#L5198
> 
> 
> I know it's not exactly the same we're discussing but of_property_match_string()
> return -EINVAL if the property is not found which leads to an index < 0 and thus
> of_parse_phandle_with_args() also returns an error back.
> 
> I think I'm not missing anything but it's always a possibility.
> 
> - Nuno Sá

Testing agrees:

Given:

	clocks = <&some_clock>;
	/delete-property/clock-names;

And:

	axi_clk = devm_clk_get_enabled(dev, "axi");

We get:

[    1.190040] axi-pwmgen 44b00000.pwm: error -ENOENT: failed to get axi clock

Re: [PATCH 3/3] pwm: axi-pwmgen: add support for external clock

[Nuno Sá]

On Wed, 2025-05-21 at 10:05 -0500, David Lechner wrote:
> On 5/21/25 9:22 AM, Nuno Sá wrote:
> > On Wed, 2025-05-21 at 15:54 +0200, Uwe Kleine-König wrote:
> > > Hello David,
> > > 
> > > On Wed, May 21, 2025 at 08:19:51AM -0500, David Lechner wrote:
> > > > On 5/21/25 4:22 AM, Uwe Kleine-König wrote:
> > > > > Can you achieve the same effect with the (IMHO slightly nicer but
> > > > > hand-crafted) following patch:
> > > > > 
> > > > >  	ddata = pwmchip_get_drvdata(chip);
> > > > >  	ddata->regmap = regmap;
> > > > >  
> > > > > -	clk = devm_clk_get_enabled(dev, NULL);
> > > > > -	if (IS_ERR(clk))
> > > > > -		return dev_err_probe(dev, PTR_ERR(clk), "failed to
> > > > > get
> > > > > clock\n");
> > > > > +	axi_clk = devm_clk_get_enabled(dev, "axi");
> > > > > +	if (IS_ERR(axi_clk))
> > > > > +		return dev_err_probe(dev, PTR_ERR(axi_clk), "failed
> > > > > to
> > > > > get axi clock\n");
> > > > > 
> > > > > +	clk = devm_clk_get_enabled_optional(dev, "ext");
> > > > > +	if (IS_ERR(clk))
> > > > > +		return dev_err_probe(dev, PTR_ERR(clk), "failed to
> > > > > get
> > > > > ext clock\n");
> > > > > +	}
> > > > 
> > > > The trouble with this is that it would not work with existing .dtbs
> > > > that don't have clock-names set. I think it would need to be more like
> > > > this:
> > > > 
> > > > 
> > > > 	axi_clk = devm_clk_get_enabled(dev, NULL);
> > > > 	if (IS_ERR(axi_clk))
> > > > 		return dev_err_probe(dev, PTR_ERR(axi_clk), "failed to
> > > > get
> > > > axi clock\n");
> > > > 
> > > > 	clk = devm_clk_get_enabled_optional(dev, "ext");
> > > > 	if (IS_ERR(clk))
> > > > 		return dev_err_probe(dev, PTR_ERR(clk), "failed to get
> > > > ext
> > > > clock\n");
> > > > 
> > > > 	if (!clk)
> > > > 		clk = axi_clk
> > > > 
> > > 
> > > If there are no clock-names, the parameter is ignored. (I didn't test,
> > > only quickly checked the code.) So passing "axi" instead of NULL should
> > > work and yield a more robust solution.
> > > 
> > > 
> > 
> > Are you sure? If there are no clock-names and you pass an id, you should get
> > an
> > error back:
> > 
> > https://elixir.bootlin.com/linux/v6.14.7/source/drivers/clk/clk.c#L5198
> > 
> > 
> > I know it's not exactly the same we're discussing but
> > of_property_match_string()
> > return -EINVAL if the property is not found which leads to an index < 0 and
> > thus
> > of_parse_phandle_with_args() also returns an error back.
> > 
> > I think I'm not missing anything but it's always a possibility.
> > 
> > - Nuno Sá
> 
> Testing agrees:
> 
> Given:
> 
> 	clocks = <&some_clock>;
> 	/delete-property/clock-names;
> 
> And:
> 
> 	axi_clk = devm_clk_get_enabled(dev, "axi");
> 
> We get:
> 
> [    1.190040] axi-pwmgen 44b00000.pwm: error -ENOENT: failed to get axi clock

Hmm, so it seems I have no bug (in the other link I pasted in the other reply).
Looking at the code I would expect -EINVAL to be returned...

I guess it's because we still try  __clk_get_sys().

- Nuno Sá