Re: clk: renesas: rcar-gen3: Fix error return code in cpg_sd_clock_recalc_rate

[Wolfram Sang <wsa@the-dreams.de>]

[-- Attachment #1: Type: text/plain, Size: 1408 bytes --]

Hi Geert,

> >> In .recalc_rate of struct clk_ops, it is desirable to return 0 if an
> >> error occurs, but -EINVAL is returned. This patch fixes it.
> >>
> >> Fixes: 5b1defde7054 ("clk: renesas: cpg-mssr: Extract common R-Car Gen3 support code")
> >> Signed-off-by: Takeshi Kihara <takeshi.kihara.df@renesas.com>
> >> Signed-off-by: Yoshihiro Kaneko <ykaneko0929@gmail.com>
> >
> > Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> 
> Why is it desirable to return 0 if an error occurs? Because the return type is
> unsigned long?

Yes, I'd think so. Instead of an arbitrary high, kind-of-random, number, just
return 0 which also indicates that something unexpected happened? Also, all
other drivers return zero in an error case (did some quick coccinelle
grepping before).

> 
> Is this routine allowed to fail? I don't see any handling of zero by
> its callers.

From clk-provider.h:

 * @recalc_rate Recalculate the rate of this clock, by querying hardware. The
 *              parent rate is an input parameter.  It is up to the caller to
 *              ensure that the prepare_mutex is held across this call.
 *              Returns the calculated rate.  Optional, but recommended - if
 *              this op is not set then clock rate will be initialized to 0.

What would be the benefit of keeping -EINVAL in an unsigned long?

Regards,

   Wolfram

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]