Re: [PATCH] serial: Do not treat the IIR register as a bitfield

[Olliver Schinagl]

Hey Andy,

On 30-03-17 11:56, Andy Shevchenko wrote:
> On Wed, 2017-03-29 at 20:44 +0200, Olliver Schinagl wrote:
> > It seems that at some point, someone made the assumption that the UART
> > Interrupt ID Register was a bitfield and started to check if certain
> > bits where set.
> >
> > Actually however the register contains interrupt ID's where only the
> > MSB
> > seems to be used singular and the rest share at least one bit. Thus
> > doing bitfield operations is wrong.
> >
> > This patch cleans up the serial_reg include file by ordering it and
> > replacing the UART_IIR_ID 'mask' with a proper mask for the register.
> > The OMAP uart appears to have used the two commonly 'reserved' bits 4
> > and 5 and thus get an UART_IIR_EXT_MASK for these two bits.
> >
> > This patch then goes over all UART_IIR_* users and changes the code
> > from
> > bitfield checking, to ID checking instead.
>
>
> Looking to implementation I would rather go with some helper like
>
> int serial_in_IIR(port, [additional mask])
> {
>  return port->serial_in(port, UART_IIR) & (_IIR_MASK [| additional
> mask]);
> }

As I just wrote a simply static inline helper function in serial_core.h, 
I just figured that the helper will only work for some of the calls. All 
interrupt checks in xxx_serial_in() obviously can't rely on this. So do 
you still want this helper function added for the other cases? Or have 
all implementations do the masking manually?

And then, is iir = serial_port_in(up, UART_IIR) & UART_IIR_MASK; 
preferred over splitting it over two lines, like I did?

Finally, why rename it to _IIR_MASK, I assume a typo here?

Olliver

>