On Thu, 28 May 2015 17:55:26 +0200, bo.svangard@xxxxxxxxxxxxxx wrote:
> From: Bo Svangård <bo.svangard@xxxxxxxxxxxxxx>
>
> Calls to regmap_raw_read/write needed register rewrite in a
> similar way as function calls to regmap_read/write already had.
> This enables reading/writing the serial datastream to the device.
>
> Signed-off-by: Bo Svangård <bo.svangard@xxxxxxxxxxxxxx>
> ---
Sorry for overreacting. I should be more professional/supportive ;)
Comments below.
> drivers/tty/serial/sc16is7xx.c | 36 ++++++++++++++++++++++++++++--------
> 1 file changed, 28 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
> index 468354e..2de40c6 100644
> --- a/drivers/tty/serial/sc16is7xx.c
> +++ b/drivers/tty/serial/sc16is7xx.c
> @@ -340,6 +340,30 @@ static void sc16is7xx_port_write(struct uart_port *port, u8 reg, u8 val)
> (reg << SC16IS7XX_REG_SHIFT) | port->line, val);
> }
>
> +static u8 sc16is7xx_raw_read(struct uart_port *port, u8 reg)
Maybe rename it to sc16is7xx_read_fifo? It does more things that just
being a pure wrapper and is used only for fifo reading.
> +{
> + struct sc16is7xx_port *s = dev_get_drvdata(port->dev);
> + unsigned int rxlen = 0;
Now this line is very wrong.
> +
> + regcache_cache_bypass(s->regmap, true);
> + regmap_raw_read(s->regmap,
> + (reg << SC16IS7XX_REG_SHIFT) | port->line,
Because you only use this function to read fifo I see little reason to
pass the register name as parameter. You can always use
SC16IS7XX_RHR_REG and perhaps add a local variable (addr?) to hold the
converted address so this function call doesn't have to take four lines?
> + s->buf, rxlen);
> + regcache_cache_bypass(s->regmap, false);
> + return rxlen;
> +}
> +
> +static void sc16is7xx_raw_write(struct uart_port *port, u8 reg, u8 to_send)
Same for the name...
> +{
> + struct sc16is7xx_port *s = dev_get_drvdata(port->dev);
> +
> + regcache_cache_bypass(s->regmap, true);
> + regmap_raw_write(s->regmap,
> + (reg << SC16IS7XX_REG_SHIFT) | port->line,
> + s->buf, to_send);
... and no need to pass reg as parameter + local variable.
> + regcache_cache_bypass(s->regmap, false);
> +}
> +
> static void sc16is7xx_port_update(struct uart_port *port, u8 reg,
> u8 mask, u8 val)
> {
> @@ -494,11 +518,8 @@ static void sc16is7xx_handle_rx(struct uart_port *port, unsigned int rxlen,
> s->buf[0] = sc16is7xx_port_read(port, SC16IS7XX_RHR_REG);
> bytes_read = 1;
> } else {
> - regcache_cache_bypass(s->regmap, true);
> - regmap_raw_read(s->regmap, SC16IS7XX_RHR_REG,
> - s->buf, rxlen);
> - regcache_cache_bypass(s->regmap, false);
> - bytes_read = rxlen;
You can actually keep this line and make fifo_read not return anything.
> + bytes_read = sc16is7xx_raw_read(port,
> + SC16IS7XX_RHR_REG);
> }
>
> lsr &= SC16IS7XX_LSR_BRK_ERROR_MASK;
> @@ -577,9 +598,8 @@ static void sc16is7xx_handle_tx(struct uart_port *port)
> s->buf[i] = xmit->buf[xmit->tail];
> xmit->tail = (xmit->tail + 1) & (UART_XMIT_SIZE - 1);
> }
> - regcache_cache_bypass(s->regmap, true);
> - regmap_raw_write(s->regmap, SC16IS7XX_THR_REG, s->buf, to_send);
> - regcache_cache_bypass(s->regmap, false);
> +
> + sc16is7xx_raw_write(port, SC16IS7XX_THR_REG, to_send);
> }
>
> if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
When you send new version please make sure you base it on Greg's
tty-next branch (tty.git tree).
Thanks, Kuba
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
