[U-Boot] [PATCH] e1000: fix bugs from recent commits

[Mike Frysinger]

On Fri, Oct 28, 2011 at 22:19, Wolfgang Denk wrote:
> Mike Frysinger wrote:
>> On Fri, Oct 28, 2011 at 07:49, Wolfgang Denk wrote:
>> > --- a/drivers/net/e1000.h
>> > +++ b/drivers/net/e1000.h
>> >
>> >  #define E1000_WRITE_FLUSH(a) \
>> > -       do { uint32_t x = E1000_READ_REG(a, STATUS); } while (0)
>> > +       E1000_READ_REG(a, STATUS)
>>
>> i think we want the do{}while as this is a write command and we don't
>> want people accidentally trying to check the return value
>
> I don't see that this is a write command.  I'm seeing only reading of
> the status register here.

yes, the actual implementation is a read.  however, the API implied by
the naming (xxx_WRITE_xxx) is a write command.  sometimes that means a
read of the status register (if they're R1C bits), or a write (if
they're W1C bits), but the forward facing API is still a write.

> And I don't understand the "accidentally trying to check the return
> value" argument either. Why would one do that - and if one does
> (probably after checking the implementation), what would be wrong
> about it?

imo, using do{}while is simply "good programming".  it falls under the
same argument of why we respect type warnings with gcc.  this is
supposed to be a write operation with void return, so we should make
sure the implementation enforces that to keep the user from
accidentally trying to do things otherwise.
-mike