| Message ID | 20231026-mbly-uart-v1-0-9258eea297d3@bootlin.com |
|---|---|
| Headers | show |
| Series | Cleanup AMBA PL011 driver | expand |
On Thu, 26 Oct 2023, Théo Lebrun wrote: > pl011_console_get_options() gets called to retrieve currently configured > options from the registers. Previously, LCRH_TX.WLEN was being parsed > as either 7 or 8 (fallback). Hardware supports values from 5 to 8 > inclusive, which pl011_set_termios() exploits for example. > > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> > --- > drivers/tty/serial/amba-pl011.c | 5 +---- > 1 file changed, 1 insertion(+), 4 deletions(-) > > diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c > index 5774d48c7f16..b2062e4cbbab 100644 > --- a/drivers/tty/serial/amba-pl011.c > +++ b/drivers/tty/serial/amba-pl011.c > @@ -2384,10 +2384,7 @@ static void pl011_console_get_options(struct uart_amba_port *uap, int *baud, > *parity = 'o'; > } > > - if ((lcr_h & 0x60) == UART01x_LCRH_WLEN_7) > - *bits = 7; > - else > - *bits = 8; > + *bits = FIELD_GET(0x60, lcr_h) + 5; /* from 5 to 8 inclusive */ 0x60 needs to be replaced with a named define!
Hi, On Thu Oct 26, 2023 at 1:13 PM CEST, Ilpo Järvinen wrote: > On Thu, 26 Oct 2023, Théo Lebrun wrote: > > > > diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c > > index 5774d48c7f16..b2062e4cbbab 100644 > > --- a/drivers/tty/serial/amba-pl011.c > > +++ b/drivers/tty/serial/amba-pl011.c > > @@ -2384,10 +2384,7 @@ static void pl011_console_get_options(struct uart_amba_port *uap, int *baud, > > *parity = 'o'; > > } > > > > - if ((lcr_h & 0x60) == UART01x_LCRH_WLEN_7) > > - *bits = 7; > > - else > > - *bits = 8; > > + *bits = FIELD_GET(0x60, lcr_h) + 5; /* from 5 to 8 inclusive */ > > 0x60 needs to be replaced with a named define! Fixed locally for the next revision, thanks! -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
Hello, On Thu Oct 26, 2023 at 3:48 PM CEST, Linus Walleij wrote: > On Thu, Oct 26, 2023 at 12:41 PM Théo Lebrun <theo.lebrun@bootlin.com> wrote: > > > pl011_console_get_options() gets called to retrieve currently configured > > options from the registers. Previously, LCRH_TX.WLEN was being parsed > > as either 7 or 8 (fallback). Hardware supports values from 5 to 8 > > inclusive, which pl011_set_termios() exploits for example. > > > > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> > > With Ilpo's comment fixed: > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> It's been fixed locally. Thank you for your review Linus! Regards, -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
Hello, On Thu Oct 26, 2023 at 4:24 PM CEST, Hugo Villeneuve wrote: > On Thu, 26 Oct 2023 12:41:21 +0200 > Théo Lebrun <theo.lebrun@bootlin.com> wrote: > > The driver uses two TIOCMBIT macros inside pl011_{get,set}_mctrl to > > simplify the logic. Those look scary to checkpatch because they contain > > ifs without do-while loops. > > > > Avoid the macros by creating small equivalent static functions; that > > lets the compiler do its type checking & avoids checkpatch errors. > > > > For the second instance __assign_bit is not usable because it deals with > > unsigned long pointers whereas we have an unsigned int in > > pl011_set_mctrl. > > > > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> > > --- > > drivers/tty/serial/amba-pl011.c | 46 +++++++++++++++++++++-------------------- > > 1 file changed, 24 insertions(+), 22 deletions(-) > > > > diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c > > index 0d53973374de..bb3082c4d35c 100644 > > --- a/drivers/tty/serial/amba-pl011.c > > +++ b/drivers/tty/serial/amba-pl011.c > > @@ -1087,7 +1087,6 @@ static void pl011_dma_rx_poll(struct timer_list *t) > > */ > > if (jiffies_to_msecs(jiffies - dmarx->last_jiffies) > > > uap->dmarx.poll_timeout) { > > - > > This should go into a separate patch, or simply be merged with one > of your other coding style/whitespace cleanup patches. Indeed, added to "tty: serial: amba-pl011: cleanup driver". Thanks. Regards, -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
Hi, While adding upstream support to a new platform (Mobileye EyeQ5[1]) that uses the AMBA PL011 driver, I took some time to look at the PL011 driver and ended up with a few patches that cleanup parts of it. The line-diff is big mostly because of the checkpatch-fixing commits. The driver hadn't received any love for quite some time. A single commit changes the code's behavior: see "tty: serial: amba-pl011: Parse bits option as 5, 6, 7 or 8 in _get_options". See commit messages for more information. [1]: https://lore.kernel.org/all/202310050726.GDpZbMDO-lkp@intel.com/T/ Have a nice day, Théo Lebrun Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> --- Théo Lebrun (6): tty: serial: amba: cleanup whitespace tty: serial: amba: Use BIT() macro for constant declarations tty: serial: amba-pl011: cleanup driver tty: serial: amba-pl011: replace TIOCMBIT macros by static functions tty: serial: amba-pl011: unindent pl011_console_get_options function body tty: serial: amba-pl011: Parse bits option as 5, 6, 7 or 8 in _get_options drivers/tty/serial/amba-pl011.c | 238 ++++++++++++++++++++-------------------- include/linux/amba/serial.h | 192 ++++++++++++++++---------------- 2 files changed, 214 insertions(+), 216 deletions(-) --- base-commit: ad582615776e62e365ab2dfa7a7a3806ada28b30 change-id: 20231023-mbly-uart-afcacbb98f8b Best regards,