On 03/28/2017 09:59 AM, Arnd Bergmann wrote:
On Tue, Mar 28, 2017 at 4:08 PM, Gary R Hook <ghook@xxxxxxx> wrote:
In fact, the use of bit fields in hardware defined data structures is
not portable to start with, so until all these bit fields get replaced
by something else, the driver cannot work on big-endian machines, and
I'm adding an annotation here to prevent it from being selected.
This is a driver that talks to hardware, a device which, AFAIK, has no
plan to be implemented in a big endian flavor. I clearly need to be more
diligent in building with various checkers enabled. I'd prefer my fix
over your suggested refusal to compile, if that's okay.
It's hard to predict the future. If this device ever makes it into an
ARM based chip, the chances are relatively high that someone
will eventually run a big-endian kernel on it. As long as it's guaranteed
to be x86-only, the risk of anyone running into the bug is close to
zero, but we normally still try to write device drivers in portable C
code to prevent it from getting copied incorrectly into another driver.
Understood, and I had surmised as such. Totally agree.
The CCPv3 code seems to not suffer from this problem, only v5 uses
bitfields.
Yes, I took a different approach when I wrote the code. IMO (arguably)
more readable. Same result: words full of hardware-dependent bit patterns.
Please help me understand what I could do better.
The rule for portable drivers is that you must not use bitfields in
structures
that can be accessed by the hardware. I think you can do this in a more
readable way by removing the CCP5_CMD_* macros etc completely
and just accessing the members of the structure as __le32 words.
The main advantage for readability here is that you can grep for the
struct members and see where they are used without following the
macros. If it helps, you can also encapsulate the generation of the
word inside of an inline function, like:
Please see my follow-on patch.
--
This is my day job. Follow me at:
IG/Twitter/Facebook: @grhookphoto
IG/Twitter/Facebook: @grhphotographer
