Re: [PATCH v2 5/6] iio: light: ROHM BU27034 Ambient Light Sensor

[Matti Vaittinen <mazziesaccount@gmail.com>]

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