[PATCH v2 3/3] i2c: rk3x: Add check for clk_enable()

[Andi Shyti]

Hi Doug,

On Tue, Nov 05, 2024 at 01:29:40PM -0800, Doug Anderson wrote:
> On Tue, Nov 5, 2024 at 8:18 AM Jiasheng Jiang
> > Add check for the return value of clk_enable() in order to catch the
> > potential exception. Moreover, convert the return type of
> > rk3x_i2c_adapt_div() into int and add the check.
> >
> > Signed-off-by: Jiasheng Jiang <jiashengjiangcool at gmail.com>
> > ---
> > Changelog:
> >
> > v1 -> v2:
> >
> > 1. Remove the Fixes tag.
> > 2. Use dev_err_probe to simplify error handling.
> > ---
> >  drivers/i2c/busses/i2c-rk3x.c | 51 +++++++++++++++++++++++++----------
> >  1 file changed, 37 insertions(+), 14 deletions(-)
> 
> Wow, this is a whole lot of code to add to check for an error that
> can't really happen as far as I'm aware. Turning on a clock is just
> some MMIO writes and can't fail, right? Is this really worth it? Maybe
> just wrap clk_enable() and spam an error to the logs if it fails? If
> we ever see that error we can figure out what's going on and if
> there's a sensible reason it could fail we could add the more complex
> code.

We've had this discussion several times. I'm of the school that
if a function can return an error, that error should be checked.
It's not spam, we do it everywhere.

On the other hand, there is another school, bigger than mine,
that doesn't want such a check.

I decided not to bother. If someone adds the check, I'm going to
accept it. If someone doesn't add the check, I won't bother
asking for it.

Said that, MMIO reads and writes can fail: in other drivers I'm
seeing bogus reads due to some firmware failures during device
reset.

I agree with you with the rest of the comments and thanks for
checking this out.

Andi