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

[Arun Muthusamy]

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

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@xxxxxxxxxxx>
> >
> > Add clock handling and add error messages for missing 'freq' DT 
> > property.
> >
> > Signed-off-by: Arun Muthusamy <arun.muthusamy@xxxxxxxxxxx>
> > Signed-off-by: Daniel Hellstrom <daniel@xxxxxxxxxxx>
> > ---
> >  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