Messages in this thread | | | From | Olliver Schinagl <> | Subject | Re: [PATCH] serial: Do not treat the IIR register as a bitfield | Date | Fri, 31 Mar 2017 15:54:52 +0200 |
| |
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
>
| |