On 3/6/23 13:21, Andy Shevchenko wrote:
On Sun, Mar 05, 2023 at 03:10:38PM +0200, Matti Vaittinen wrote:
On 3/4/23 22:17, Jonathan Cameron wrote:
On Thu, 2 Mar 2023 12:58:59 +0200
Matti Vaittinen <mazziesaccount@xxxxxxxxx> wrote:
As per other branch of the thread.
ch0 = max(1, le16_to_cpu(res[0]);
> would be cleaner.
I tried this out. Comparing u16 to literal 1 results comparison of values
with different sizes:
./include/linux/minmax.h:20:28: warning: comparison of distinct pointer
types lacks a cast
(!!(sizeof((typeof(x) *)1 == (typeof(y) *)1)))
^
./include/linux/minmax.h:26:4: note: in expansion of macro ‘__typecheck’
(__typecheck(x, y) && __no_side_effects(x, y))
^~~~~~~~~~~
./include/linux/minmax.h:36:24: note: in expansion of macro ‘__safe_cmp’
__builtin_choose_expr(__safe_cmp(x, y), \
^~~~~~~~~~
./include/linux/minmax.h:74:19: note: in expansion of macro ‘__careful_cmp’
#define max(x, y) __careful_cmp(x, y, >)
^~~~~~~~~~~~~
drivers/iio/light/rohm-bu27034.c:1057:8: note: in expansion of macro ‘max’
ch0 = max(1, ch0);
I could work around this by doing:
const u16 min_ch_val = 1;
...
ch0 = max(min_ch_val, le16_to_cpu(res[0]));
but I think that would really be obfuscating the meaning. I assume
ch0 = max((u16)1, le16_to_cpu(res[0]));
might work too - but to me it's pretty ugly.
That's why we have max_t() and clamp_val().
And you know that.
Thanks for pointing out the max_t(). I really missed that. I didn't
think of the clamp_val() either. clamp_val() would "explain" better what
we do here - but as we don't have the upper limit using that would (IMO)
be slightly more confusing.
The more I am looking at this, the stronger I feel we should really just
write this as it was. Check if res[0] contains the only unsafe data
"!res[0]" - and if yes, set it to 1. The comment above it will clarify it to
a reader wondering what happens.
I will leave it like it was in v2 for v3. If you still feel strong about it
then we need to continue rubbing it.
You need to convert bit ordering first, then check for 0. It would at least
make more sense. (Today is 0 you are comparing with, tomorrow it might be
0xfffe, which is different to 0x7fff).
I don't think being prepared for any other check but the check for 0 is
meaningful here. Every other value except 0 is valid for this sensor and
can be accepted as such. Preparing for something that may never realize
(new sensor with less than 16bits of data) is likely to just make things
more complicated.
Well, as both You and Jonathan do think we should not just do
if(!res[0]) - I'll try using the max_t() as you suggested. I still
really dislike it because it makes it harder (for me) to capture what
happens here. Well, I try to survive with this and hopefully just read
the comment above if puzzled when I need to get back to this code.
Yours,
-- Matti
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
