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

[Marc Kleine-Budde]

On 19.10.2026 12:58:24, 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().

From my point of view it's not consistent to continue probing if the
"can_clk" has been configured in the DT, and it fails for some reason.

> 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.

Using a clock rate of 0 to signal something to the driver sounds like a
hack to me :)

> 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.

I was thinking of not ignoring errors:

       gpriv->can_clk = devm_clk_get_optional(dev, "can_clk");
       if (ERR_PTR(gpriv->can_clk))
               return dev_err_probe(dev, ERR_PTR(gpriv->can_clk), "cannot get can_clk");

       if (!gpriv->can_clk || (clk_get_rate(gpriv->can_clk) == 0)) {

If you plan to keep the current logic and always fall back to "canfd",
please:
- add -EPROBE_DEFER handling
- add a comment that all other errors result in fallback to "canfd" clock
- add a comment that a "can_clk" rate of "0" means don't use the clock

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

Attachment: signature.asc
Description: PGP signature