Hi Wolfram,
On Mon, Jul 17, 2017 at 11:18 AM, Wolfram Sang <[email protected]> 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 <[email protected]>
>> >> Signed-off-by: Yoshihiro Kaneko <[email protected]>
>> >
>> > Reviewed-by: Wolfram Sang <[email protected]>
>>
>> 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 -- [email protected]
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
