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

[David Lechner <dlechner@baylibre.com>]

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