Re: [PATCH] gpio: gpio-exar: replace division condition with direct comparison

[Suraj Sonawane <surajsonawane0215@gmail.com>]

On 15/11/24 15:06, Andy Shevchenko wrote:
> + Dan
> 
> I have to comment on this change as it's a bit controversial.
> 
> TL;DR: this patch is not more than a (harmless?) noise.
> 
> On Wed, Nov 13, 2024 at 01:46:59AM +0530, Suraj Sonawane wrote:
>> Fix an issue detected by the Smatch tool:
>>
>> drivers/gpio/gpio-exar.c:52 exar_offset_to_sel_addr() warn:
>> replace divide condition 'pin / 8' with 'pin >= 8'
>> drivers/gpio/gpio-exar.c:62 exar_offset_to_lvl_addr() warn:
>> replace divide condition 'pin / 8' with 'pin >= 8'
> 
> This message does not really explain why.
> 
>> The division 'pin / 8' was used to check if the pin number is 8 or greater,
>> which can be confusing and less readable.
> 
> It's inaccurate description. Everyone who is familiar with GPIO HW is
> also familiar with line grouping in banks. Here is the clear statement
> "get the bank number (where 8 lines per bank), and if it's 0 do this,
> else do that". It might be in the future that (new version of) HW will
> gain more banks and we would return to "division".
> 
>> Replacing it with 'pin >= 8' makes the code clearer by directly
>> comparing the pin number.
> 
> I don't think this statement is fully true. See above.
> 
>> This also removes reliance on integer division,
> 
> On top of that "division" here uses power-of-two divisor, which any
> optimizing (and this code I think won't ever be built without
> optimization turned on) compiler (I think from the very beginning of
> the Linux kernel project) knows how to convert to right shifts on
> the platforms that support that (and how many do not nowadays? 0?).
> 
> Additionally in the cases when we have a / 8; a % 8 type of expressions
> coupled together, the compiler actually may issue an integer division
> assembly instructions on some ISAs where it gives two values in one
> go. Replacing like the above might break that (if the compiler is old
> or not clever enough).
> 
>> which can be harder to understand
> 
> No, "division" by power-of-two numbers is very well understandble.
> 
>> and may introduce subtle bugs in the future.
> 
> What bugs?
> 
> The bottom line is that: I recommend to work with smatch developers
> to amend smatch instead.
> 
> P.S. I wouldn't like to see similar patches to other GPIO drivers,
> especially those that use a / 8; a % 8 type of expressions together.
> 

I understand your points about the familiarity of line grouping in GPIO 
hardware and the optimization behavior of compilers for power-of-two 
division. I initially thought this could be a good fix as I have seen 
similar changes before. Thank you for the feedback—I will keep this in 
mind before submitting such patches in the future.

Best regards,
Suraj Sonawane