Hi Wolfram,
On Mon, Jul 17, 2017 at 11:18 AM, Wolfram Sang <wsa@xxxxxxxxxxxxx> wrote:
>> >> 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@xxxxxxxxxxx>
>> >> Signed-off-by: Yoshihiro Kaneko <ykaneko0929@xxxxxxxxx>
>> >
>> > Reviewed-by: Wolfram Sang <wsa+renesas@xxxxxxxxxxxxxxxxxxxx>
>>
>> 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).
OK.
>> 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?
I do not mean that -EINVAL is correct. Obviously it isn't. But blindly
replacing -EINVAL by zero may not be the right solution neither.
If recalc_rate cannot return an error value, perhaps there is a good reason
for that?
I see there's a similar check in cpg_sd_clock_enable(), so the clock also
cannot be enabled if this condition is met?
When does this happen? If the firmware leaves a invalid/unsupported setting
in the register? If we can't recover from that, perhaps the clock's probe
should just fail instead?
It looks like the only writer of that field is cpg_sd_clock_set_rate(),
which always writes a valid/supported value. Is it guaranteed that this
function is always called first?
If yes, the checks can just be removed instead?
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
