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

[David Lechner <dlechner@baylibre.com>]

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