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

[claudiu beznea]

On 10/19/26 12:58, claudiu beznea wrote:
> 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, ...);

On the other hand, returning -EPROBE_DEFER here means the driver may never probe 
if the can_clk provider fails to probe, even though the fallback clock is 
available. That would go against what I understand to be the intent of the 
IS_ERR() check: letting the probe continue when can_clk can't be obtained.

Please let me know how you would like me to proceed.

Thank you,
Claudiu