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

[claudiu beznea <claudiu.beznea@tuxon.dev>]

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