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
