Re: [PATCH 04/10] can: grcan: Add clock handling

[Arun Muthusamy <arun.muthusamy@gaisler.com>]

Hi Krzysztof,

Thank you for your thorough review. I wanted to get your guidance 
regarding the clock property in the DT binding.

In the binding, I included the clocks property with maxItems: 1 to 
indicate that a clock should be described. The driver calls:

clk = devm_clk_get(dev, NULL);

Since we pass NULL, the driver always requests the first (and only) 
clock from the clocks property.

I want to ensure the binding is fully compliant with the Linux DT ABI. 
Could you advise the preferred way to document the clocks and 
clock-names properties in this scenario? Specifically:

Do we still need a clock-names entry even if the driver never uses it by 
name?
For LEON systems, the driver relies on the "freq" property, while NOEL 
systems use a standard "clocks" binding. Given this dual approach, 
should the clocks property be marked as optional or required in the 
binding?

Thank you for your time and help.

-- 
BR,

Arun Muthusamy
Software Engineer
Frontgrade Gaisler
T : +46 (0) 700 558 528
arun.muthusamy@gaisler.com

Frontgrade Gaisler AB, Kungsgatan 12, SE-411 19 GÖTEBORG, Sweden.
+46 (0) 31 775 8650, www.gaisler.com

On 18.11.2025 12:01, Krzysztof Kozlowski wrote:
> On 18/11/2025 10:21, Arun Muthusamy wrote:
>> From: Daniel Hellstrom <daniel@gaisler.com>
>> 
>> Add clock handling and add error messages for missing 'freq' DT 
>> property.
>> 
>> Signed-off-by: Arun Muthusamy <arun.muthusamy@gaisler.com>
>> Signed-off-by: Daniel Hellstrom <daniel@gaisler.com>
>> ---
>>  drivers/net/can/grcan.c | 19 ++++++++++++++++---
>>  1 file changed, 16 insertions(+), 3 deletions(-)
>> 
>> diff --git a/drivers/net/can/grcan.c b/drivers/net/can/grcan.c
>> index 3b1b09943436..538a9b4f82ab 100644
>> --- a/drivers/net/can/grcan.c
>> +++ b/drivers/net/can/grcan.c
>> @@ -34,7 +34,7 @@
>>  #include <linux/spinlock.h>
>>  #include <linux/of.h>
>>  #include <linux/of_irq.h>
>> -
>> +#include <linux/clk.h>
>>  #include <linux/dma-mapping.h>
>> 
>>  #define DRV_NAME	"grcan"
>> @@ -1644,6 +1644,7 @@ static int grcan_probe(struct platform_device 
>> *ofdev)
>>  {
>>  	struct device_node *np = ofdev->dev.of_node;
>>  	struct device_node *sysid_parent;
>> +	struct clk *clk;
>>  	u32 sysid, ambafreq;
>>  	int irq, err;
>>  	void __iomem *base;
>> @@ -1663,8 +1664,20 @@ static int grcan_probe(struct platform_device 
>> *ofdev)
>> 
>>  	err = of_property_read_u32(np, "freq", &ambafreq);
>>  	if (err) {
>> -		dev_err(&ofdev->dev, "unable to fetch \"freq\" property\n");
>> -		goto exit_error;
>> +		clk = devm_clk_get(&ofdev->dev, NULL);
> 
> Nope, your binding said there is no clock... you cannot add 
> undocumented
> ABI.
> 
> 
> Best regards,
> Krzysztof