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

[Vasileios Amoiridis <vassilisamir@gmail.com>]

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@gmail.com> 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@gmail.com>
> 
> Jonathan