On Tue, 27 Sep 2022, Jiri Slaby wrote:
> TX space reads from the control register are performed in various forms
> on 4 places in altera_jtaguart. Unify all those and do the read and
> masking on a single place.
>
> The new helper altera_jtaguart_tx_space() uses FIELD_GET(), so we can
> drop ALTERA_JTAGUART_CONTROL_WSPACE_OFF now.
>
> Cc: Tobias Klauser <tklauser@xxxxxxxxxx>
> Signed-off-by: Jiri Slaby <jslaby@xxxxxxx>
> ---
> drivers/tty/serial/altera_jtaguart.c | 27 ++++++++++++++++-----------
> 1 file changed, 16 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/tty/serial/altera_jtaguart.c b/drivers/tty/serial/altera_jtaguart.c
> index 23f339757894..ac8ce418de36 100644
> --- a/drivers/tty/serial/altera_jtaguart.c
> +++ b/drivers/tty/serial/altera_jtaguart.c
> @@ -9,6 +9,7 @@
> * (C) Copyright 2010, Tobias Klauser <tklauser@xxxxxxxxxx>
> */
>
> +#include <linux/bitfield.h>
> #include <linux/kernel.h>
> #include <linux/init.h>
> #include <linux/interrupt.h>
> @@ -48,7 +49,6 @@
> #define ALTERA_JTAGUART_CONTROL_WI_MSK 0x00000200
> #define ALTERA_JTAGUART_CONTROL_AC_MSK 0x00000400
> #define ALTERA_JTAGUART_CONTROL_WSPACE_MSK 0xFFFF0000
> -#define ALTERA_JTAGUART_CONTROL_WSPACE_OFF 16
>
> /*
> * Local per-uart structure.
> @@ -59,10 +59,19 @@ struct altera_jtaguart {
> unsigned long imr; /* Local IMR mirror */
> };
>
> +static unsigned int altera_jtaguart_tx_space(struct uart_port *port, u32 *ctlp)
> +{
> + u32 ctl = readl(port->membase + ALTERA_JTAGUART_CONTROL_REG);
> +
> + if (ctlp)
> + *ctlp = ctl;
> +
> + return FIELD_GET(ALTERA_JTAGUART_CONTROL_WSPACE_MSK, ctl);
> +}
> +
> static unsigned int altera_jtaguart_tx_empty(struct uart_port *port)
> {
> - return (readl(port->membase + ALTERA_JTAGUART_CONTROL_REG) &
> - ALTERA_JTAGUART_CONTROL_WSPACE_MSK) ? TIOCSER_TEMT : 0;
> + return altera_jtaguart_tx_space(port, NULL) ? TIOCSER_TEMT : 0;
> }
>
> static unsigned int altera_jtaguart_get_mctrl(struct uart_port *port)
> @@ -150,9 +159,7 @@ static void altera_jtaguart_tx_chars(struct altera_jtaguart *pp)
>
> pending = uart_circ_chars_pending(xmit);
> if (pending > 0) {
> - count = (readl(port->membase + ALTERA_JTAGUART_CONTROL_REG) &
> - ALTERA_JTAGUART_CONTROL_WSPACE_MSK) >>
> - ALTERA_JTAGUART_CONTROL_WSPACE_OFF;
> + count = altera_jtaguart_tx_space(port, NULL);
> if (count > pending)
> count = pending;
> if (count > 0) {
> @@ -298,12 +305,11 @@ static struct altera_jtaguart altera_jtaguart_ports[ALTERA_JTAGUART_MAXPORTS];
> #if defined(CONFIG_SERIAL_ALTERA_JTAGUART_CONSOLE_BYPASS)
> static void altera_jtaguart_console_putc(struct uart_port *port, unsigned char c)
> {
> - unsigned long status;
> unsigned long flags;
> + u32 status;
>
> spin_lock_irqsave(&port->lock, flags);
> - while (((status = readl(port->membase + ALTERA_JTAGUART_CONTROL_REG)) &
> - ALTERA_JTAGUART_CONTROL_WSPACE_MSK) == 0) {
> + while (!altera_jtaguart_tx_space(port, &status)) {
> if ((status & ALTERA_JTAGUART_CONTROL_AC_MSK) == 0) {
> spin_unlock_irqrestore(&port->lock, flags);
> return; /* no connection activity */
> @@ -321,8 +327,7 @@ static void altera_jtaguart_console_putc(struct uart_port *port, unsigned char c
> unsigned long flags;
>
> spin_lock_irqsave(&port->lock, flags);
> - while ((readl(port->membase + ALTERA_JTAGUART_CONTROL_REG) &
> - ALTERA_JTAGUART_CONTROL_WSPACE_MSK) == 0) {
> + while (!altera_jtaguart_tx_space(port, NULL)) {
> spin_unlock_irqrestore(&port->lock, flags);
> cpu_relax();
> spin_lock_irqsave(&port->lock, flags);
Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@xxxxxxxxxxxxxxx>
With caveats. This is not incorrect per se but I don't particularly like
that pointer thing done in altera_jtaguart_tx_space(). IMHO,
altera_jtaguart_console_putc() that needs the control register also for
other uses than tx space check should read it locally and
__altera_jtaguart_tx_space() could be added to handle just the
tx space check without read. But this is of course just my opinion,
there's no technical issue things done either way.
--
i.
