Re: [PATCH v3 2/2] tty/serial: Add rx-tx-swap OF option to stm32-usart

[Martin DEVERA <devik@xxxxxxxxxx>]

On 3/2/21 6:44 PM, Fabrice Gasnier wrote:
> On 3/2/21 2:15 PM, Martin Devera wrote:
> > STM32 F7/H7 usarts supports RX & TX pin swapping.
> > Add option to turn it on.
> > Tested on STM32MP157.
> >
> > Signed-off-by: Martin Devera <devik@xxxxxxxxxx>
> > ---
> >   drivers/tty/serial/stm32-usart.c | 11 ++++++++++-
> >   drivers/tty/serial/stm32-usart.h |  5 +++++
> >   2 files changed, 15 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/tty/serial/stm32-usart.c b/drivers/tty/serial/stm32-usart.c
> > index b3675cf25a69..d390f7da1441 100644
> > --- a/drivers/tty/serial/stm32-usart.c
> > +++ b/drivers/tty/serial/stm32-usart.c
> > @@ -644,6 +644,12 @@ static int stm32_usart_startup(struct uart_port *port)
> >   	if (ret)
> >   		return ret;
> >   
> > +	if (stm32_port->swap) {
> > +		val = readl_relaxed(port->membase + ofs->cr2);
> > +		val |= USART_CR2_SWAP;
> > +		writel_relaxed(val, port->membase + ofs->cr2);
> > +	}
> > +
> >   	/* RX FIFO Flush */
> >   	if (ofs->rqr != UNDEF_REG)
> >   		stm32_usart_set_bits(port, ofs->rqr, USART_RQR_RXFRQ);
> > @@ -758,7 +764,7 @@ static void stm32_usart_set_termios(struct uart_port *port,
> >   	cr1 = USART_CR1_TE | USART_CR1_RE;
> >   	if (stm32_port->fifoen)
> >   		cr1 |= USART_CR1_FIFOEN;
> > -	cr2 = 0;
> > +	cr2 = stm32_port->swap ? USART_CR2_SWAP : 0;
> >   	cr3 = readl_relaxed(port->membase + ofs->cr3);
> >   	cr3 &= USART_CR3_TXFTIE | USART_CR3_RXFTCFG_MASK | USART_CR3_RXFTIE
> >   		| USART_CR3_TXFTCFG_MASK;
> > @@ -1006,6 +1012,9 @@ static int stm32_usart_init_port(struct stm32_port *stm32port,
> >   			return stm32port->wakeirq ? : -ENODEV;
> >   	}
> >   
> > +	stm32port->swap = stm32port->info->cfg.has_swap &&
> > +		of_property_read_bool(pdev->dev.of_node, "rx-tx-swap");
> > +
> >   	stm32port->fifoen = stm32port->info->cfg.has_fifo;
> >   
> >   	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > diff --git a/drivers/tty/serial/stm32-usart.h b/drivers/tty/serial/stm32-usart.h
> > index cb4f327c46db..bd18dd1c1bcd 100644
> > --- a/drivers/tty/serial/stm32-usart.h
> > +++ b/drivers/tty/serial/stm32-usart.h
> > @@ -25,6 +25,7 @@ struct stm32_usart_offsets {
> >   struct stm32_usart_config {
> >   	u8 uart_enable_bit; /* USART_CR1_UE */
> >   	bool has_7bits_data;
> > +	bool has_swap;
> >   	bool has_wakeup;
> >   	bool has_fifo;
> >   	int fifosize;
> > @@ -55,6 +56,7 @@ struct stm32_usart_info stm32f4_info = {
> >   	.cfg = {
> >   		.uart_enable_bit = 13,
> >   		.has_7bits_data = false,
> > +		.has_swap = false,
> Hi Martin,
>
> Only one minor comment from me here. No need to add a false (zero)
> initialization in this struct. I'm not sure why this is the case for the
> has_7bits_data here...
>
> With that fixed, you can add my:
> Acked-by: Fabrice Gasnier <fabrice.gasnier@xxxxxxxxxxx>
The has_7bits_data was what made me a bit unsure. Ok fixed now.
Thank you for your review.

Martin