Re: [PATCH v5 05/10] iio: pressure: bmp280: Make return values consistent

From: Vasileios Amoiridis
Date: Sun May 05 2024 - 19:08:26 EST


On Sun, May 05, 2024 at 08:08:18PM +0100, Jonathan Cameron wrote:
> On Mon, 29 Apr 2024 21:00:41 +0200
> Vasileios Amoiridis <vassilisamir@xxxxxxxxx> wrote:
>
> > Throughout the driver there are quite a few places were return
> > values are treated as errors if they are negative or not-zero.
> > This commit tries to make the return values of those functions
> > consistent and treat them as errors in case there is a negative
> > value since the vast majority of the functions are returning
> > erorrs coming from regmap_*() functions.
>
> The changes are fine, but that argument isn't correct.
> regmap_*() functions never (that I can recall) return positive
> values, so if (ret) would be valid for those and I'd have expected
> the exact opposite outcome if you are looking at regmap*() return
> values to make the decision.
>
> The if (ret) pattern is sometimes used throughout because it
> makes
> return function()
>
> consistent without needing to do
>
> ret = function();
> if (ret < 0)
> return ret;
>
> return 0;
>
> That pattern isn't particularly common in this driver (there are few cases).
> We also tend not to worry too much about that slight inconsistency though
> in a few cases it has lead to compilers failing to detect that some paths
> are not possible and reporting false warnings.
>
> However, all arguments about which is 'better' aside, key is that consistency
> (either choice) is better than a mix. So I'm fine with ret < 0 on basis
> it's the most common in this driver being your justification. Just don't
> blame regmap*() return values!
>

Hi Jonathan!

Thank you once again for the valueable feedback!

Of course, if (ret) would be valid for the return values of the regmap_*()
functions. I was just trying to understand which of the 2 options is more
widely used in other drivers and I tried to implement that. In general,
the if (ret) is used 65 times while the if (ret < 0) only 20. So, in
terms of noise, changing the if (ret < 0) to if (ret) will create less
noise. I chose the if (ret < 0) because I saw other people using it
and it felt better in my eyes. I could check if if (ret) applies
everywhere and update it in the v6.

> >
> > While at it, add error messages that were not implemented before.
> >
> > Finally, remove any extra error checks that are dead code.
>
> Ideally this would be broken up a little more as, whilst all error
> code related, these aren't all the same thing.
>
> I'd have preferred:
> 1) Dead code removal.
> 2) Message updates.
> 3) Switch to consistent ret handling.
>
> However it isn't that bad as a single patch, so just address the question
> above and I think this will be fine as one patch.
>

Since from your comments in the next patches a v6 is for sure, I could split
this as well!

Cheers,
Vasilis

> >
> > Signed-off-by: Vasileios Amoiridis <vassilisamir@xxxxxxxxx>
>
> Jonathan