Re: [PATCH v4 3/7] can: rcar_canfd: Allow the CAN FD clock to be sourced from fck

[claudiu beznea]

Hi, Mark,

On 10/17/26 23:01, Marc Kleine-Budde wrote:
> > > >    /* Helper functions */
> > > > @@ -2190,13 +2196,20 @@ static int rcar_canfd_probe(struct platform_device *pdev)
> > > >    	 */
> > > >    	gpriv->can_clk = devm_clk_get(dev, "can_clk");
> > > >    	if (IS_ERR(gpriv->can_clk) || (clk_get_rate(gpriv->can_clk) == 0)) {
> > >               ^^^^^^^^^^^^^^^^^^^^^
> > >
> > > As sashiko.dev [1] pointed out here's no check for -EPROBE_DEFER.
> > This looks to me a pre-existing issue.
> >
> > The newly added support does:
> >
> > fcan_freq = clk_get_rate(gpriv->clkp);
> >
> > At the time this is executed the clkp was already checked against any errors
>                                     ^^^^
> > (above) including -EPROBE_DEFER.
> It's not about "clkp".
>
> What's missing is, that "gpriv->can_clk" is checked against IS_ERR():
>
> > > >    	gpriv->can_clk = devm_clk_get(dev, "can_clk");
> > > >    	if (IS_ERR(gpriv->can_clk) || (clk_get_rate(gpriv->can_clk) == 0)) {
> but if it's a -EPROBE_DEFER, IS_ERR() returns true. I think in this case
> you want to return with dev_err_probe().
>
> There's also a difference between "can_clk" not found (e.g. not
> configured in DT), and there was a problem with "can_clk". I don't know
> if this is important for this driver, but if you want to distinguish
> between those, you can use devm_clk_get_optional(). I will return NULL
> if the clock is not found, -EPROBE_DEFER if not available yet, the clock
> or, in case of an error -errno.
>
> Thinking about it: if "can_clk" is not available, I would consider it
> intentional, so the driver should use the other clock. If the clock has
> an error, the driver should not fallback to a probably untested clock
> configuration.

        /* fCAN clock: Pick External clock. If not available fallback to
         * CANFD clock
         */
        gpriv->can_clk = devm_clk_get(dev, "can_clk");
        if (IS_ERR(gpriv->can_clk) || (clk_get_rate(gpriv->can_clk) == 0)) {

Based on the comment on this code "If not available fallback to CANFD clock"
I think the intention with IS_ERR() is to allow probing go forward, falling back 
to CANFD clock, in case of any error of devm_clk_get().

The device trees that I've checked sets the can_clk to a fixed clock with rate = 
0 or have the clock provided directly by the SoC clock provider. I think the 
intention with IS_ERR() was not only for -ENOENT but for any other kind of 
errors. We can return in case of -EPROBE_DEFER, to ensure the expected clock is 
used, when it's available and sane.

I'll switch to something like:

 gpriv->can_clk = devm_clk_get(&pdev->dev, "can_clk");
+if (ERR_PTR(gpriv->can_clk) == -EPROBE_DEFER)
+	return dev_err_ptr(dev, ...);

The devm_clk_get_optional() would involve changing more lines:

-       gpriv->can_clk = devm_clk_get(dev, "can_clk");
-       if (IS_ERR(gpriv->can_clk) || (clk_get_rate(gpriv->can_clk) == 0)) {
+       gpriv->can_clk = devm_clk_get_optional(dev, "can_clk");
+       if (ERR_PTR(gpriv->can_clk) == -EPROBE_DEFER)
+               return dev_err_probe(dev, -EPROBE_DEFER, "cannot get can_clk");
+
+       if (IS_ERR_OR_NULL(gpriv->can_clk) || (clk_get_rate(gpriv->can_clk) == 0)) {
                gpriv->can_clk = devm_clk_get(dev, "canfd");
                if (IS_ERR(gpriv->can_clk))
                        return dev_err_probe(dev, PTR_ERR(gpriv->can_clk),

Please let me know otherwise.

Thank you,
Claudiu