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
