Re: [PATCH v3] tty: make pl011 serial port driver support 485 mode

Re: [PATCH v3] tty: make pl011 serial port driver support 485 mode

[Greg KH]

On Thu, Jan 14, 2021 at 08:28:30PM +0800, zhangqiumiao1@huawei.com wrote:
> From: Qiumiao Zhang <zhangqiumiao1@huawei.com>
> 
> make pl011 serial port support 485 mode full duplex communication
> 
> Signed-off-by: Qiumiao Zhang <zhangqiumiao1@huawei.com>
> ---
> Changes in v3:
>   -Fix busy loop forever in pl011_tx_empty
>   -Move the definition of cr into uart_amba_port
>   -run checkpatch with no error or warning
> 
> Changes in v2:
>   -Fix two compilation errors
> 
>  drivers/tty/serial/amba-pl011.c | 32 ++++++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
> 
> diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
> index c255476..9da10a4 100644
> --- a/drivers/tty/serial/amba-pl011.c
> +++ b/drivers/tty/serial/amba-pl011.c
> @@ -44,6 +44,7 @@
> 
>  #include "amba-pl011.h"
> 
> +#define ISEMPTY			1
>  #define UART_NR			14
> 
>  #define SERIAL_AMBA_MAJOR	204
> @@ -264,6 +265,7 @@ struct uart_amba_port {
>  	unsigned int		fifosize;	/* vendor-specific */
>  	unsigned int		old_cr;		/* state during shutdown */
>  	unsigned int		fixed_baud;	/* vendor-set fixed baud rate */
> +	unsigned int		cr;
>  	char			type[12];
>  #ifdef CONFIG_DMA_ENGINE
>  	/* DMA stuff */
> @@ -1284,6 +1286,8 @@ static inline bool pl011_dma_rx_running(struct uart_amba_port *uap)
>  #define pl011_dma_flush_buffer	NULL
>  #endif
> 
> +static unsigned int pl011_tx_empty(struct uart_port *port);
> +
>  static void pl011_stop_tx(struct uart_port *port)
>  {
>  	struct uart_amba_port *uap =
> @@ -1292,6 +1296,17 @@ static void pl011_stop_tx(struct uart_port *port)
>  	uap->im &= ~UART011_TXIM;
>  	pl011_write(uap->im, uap, REG_IMSC);
>  	pl011_dma_tx_stop(uap);
> +	if (port->rs485.flags & SER_RS485_ENABLED) {
> +		while(pl011_tx_empty(port) != ISEMPTY) ;

No busy loops that run forever please.

> +
> +		uap->cr = pl011_read(uap, REG_CR);
> +		if (port->rs485.flags & SER_RS485_RTS_AFTER_SEND) {
> +			uap->cr |= UART011_CR_RTS;
> +		} else {
> +			uap->cr &= ~UART011_CR_RTS;
> +		}

checkpatch did not complain about this?  It should have.

thanks,

greg k-h

Re: [PATCH v3] tty: make pl011 serial port driver support 485 mode

[Greg KH]

On Fri, Jan 15, 2021 at 02:32:39AM +0000, zhangqiumiao wrote:
> On Thu, Jan 14, 2021 at 08:28:30PM +0800, zhangqiumiao1@huawei.com wrote:
> > From: Qiumiao Zhang <zhangqiumiao1@huawei.com>
> > 
> > make pl011 serial port support 485 mode full duplex communication
> > 
> > Signed-off-by: Qiumiao Zhang <zhangqiumiao1@huawei.com>
> > ---
> > Changes in v3:
> >   -Fix busy loop forever in pl011_tx_empty
> >   -Move the definition of cr into uart_amba_port
> >   -run checkpatch with no error or warning
> > 
> > Changes in v2:
> >   -Fix two compilation errors
> > 
> >  drivers/tty/serial/amba-pl011.c | 32 ++++++++++++++++++++++++++++++++
> >  1 file changed, 32 insertions(+)
> > 
> > diff --git a/drivers/tty/serial/amba-pl011.c 
> > b/drivers/tty/serial/amba-pl011.c index c255476..9da10a4 100644
> > --- a/drivers/tty/serial/amba-pl011.c
> > +++ b/drivers/tty/serial/amba-pl011.c
> > @@ -44,6 +44,7 @@
> > 
> >  #include "amba-pl011.h"
> > 
> > +#define ISEMPTY			1
> >  #define UART_NR			14
> > 
> >  #define SERIAL_AMBA_MAJOR	204
> > @@ -264,6 +265,7 @@ struct uart_amba_port {
> >  	unsigned int		fifosize;	/* vendor-specific */
> >  	unsigned int		old_cr;		/* state during shutdown */
> >  	unsigned int		fixed_baud;	/* vendor-set fixed baud rate */
> > +	unsigned int		cr;
> >  	char			type[12];
> >  #ifdef CONFIG_DMA_ENGINE
> >  	/* DMA stuff */
> > @@ -1284,6 +1286,8 @@ static inline bool pl011_dma_rx_running(struct uart_amba_port *uap)
> >  #define pl011_dma_flush_buffer	NULL
> >  #endif
> > 
> > +static unsigned int pl011_tx_empty(struct uart_port *port);
> > +
> >  static void pl011_stop_tx(struct uart_port *port)  {
> >  	struct uart_amba_port *uap =
> > @@ -1292,6 +1296,17 @@ static void pl011_stop_tx(struct uart_port *port)
> >  	uap->im &= ~UART011_TXIM;
> >  	pl011_write(uap->im, uap, REG_IMSC);
> >  	pl011_dma_tx_stop(uap);
> > +	if (port->rs485.flags & SER_RS485_ENABLED) {
> > +		while(pl011_tx_empty(port) != ISEMPTY) ;
> 
> I intend to change this to:
>         while(pl011_tx_empty(port) != ISEMPTY)
> 			cpu_relax();
> Is this ok?

See other comments on this same list (linux-serial) for a patch for a
different driver a few days ago about why doing that would not be a good
idea.  Did you not review that change when it was submitted?

> > +
> > +		uap->cr = pl011_read(uap, REG_CR);
> > +		if (port->rs485.flags & SER_RS485_RTS_AFTER_SEND) {
> > +			uap->cr |= UART011_CR_RTS;
> > +		} else {
> > +			uap->cr &= ~UART011_CR_RTS;
> > +		}
> 
> Checkpatch doesn't find the problem here. Please tell me what's wrong here?

No braces should be needed, have you read the coding style rules?

thanks,

greg k-h

Re: [PATCH v3] tty: make pl011 serial port driver support 485 mode

[zhangqiumiao]

On Thu, Jan 14, 2021 at 08:28:30PM +0800, zhangqiumiao1@huawei.com wrote:
> From: Qiumiao Zhang <zhangqiumiao1@huawei.com>
> 
> make pl011 serial port support 485 mode full duplex communication
> 
> Signed-off-by: Qiumiao Zhang <zhangqiumiao1@huawei.com>
> ---
> Changes in v3:
>   -Fix busy loop forever in pl011_tx_empty
>   -Move the definition of cr into uart_amba_port
>   -run checkpatch with no error or warning
> 
> Changes in v2:
>   -Fix two compilation errors
> 
>  drivers/tty/serial/amba-pl011.c | 32 ++++++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
> 
> diff --git a/drivers/tty/serial/amba-pl011.c 
> b/drivers/tty/serial/amba-pl011.c index c255476..9da10a4 100644
> --- a/drivers/tty/serial/amba-pl011.c
> +++ b/drivers/tty/serial/amba-pl011.c
> @@ -44,6 +44,7 @@
> 
>  #include "amba-pl011.h"
> 
> +#define ISEMPTY			1
>  #define UART_NR			14
> 
>  #define SERIAL_AMBA_MAJOR	204
> @@ -264,6 +265,7 @@ struct uart_amba_port {
>  	unsigned int		fifosize;	/* vendor-specific */
>  	unsigned int		old_cr;		/* state during shutdown */
>  	unsigned int		fixed_baud;	/* vendor-set fixed baud rate */
> +	unsigned int		cr;
>  	char			type[12];
>  #ifdef CONFIG_DMA_ENGINE
>  	/* DMA stuff */
> @@ -1284,6 +1286,8 @@ static inline bool pl011_dma_rx_running(struct uart_amba_port *uap)
>  #define pl011_dma_flush_buffer	NULL
>  #endif
> 
> +static unsigned int pl011_tx_empty(struct uart_port *port);
> +
>  static void pl011_stop_tx(struct uart_port *port)  {
>  	struct uart_amba_port *uap =
> @@ -1292,6 +1296,17 @@ static void pl011_stop_tx(struct uart_port *port)
>  	uap->im &= ~UART011_TXIM;
>  	pl011_write(uap->im, uap, REG_IMSC);
>  	pl011_dma_tx_stop(uap);
> +	if (port->rs485.flags & SER_RS485_ENABLED) {
> +		while(pl011_tx_empty(port) != ISEMPTY) ;

I intend to change this to:
        while(pl011_tx_empty(port) != ISEMPTY)
			cpu_relax();
Is this ok?

> +
> +		uap->cr = pl011_read(uap, REG_CR);
> +		if (port->rs485.flags & SER_RS485_RTS_AFTER_SEND) {
> +			uap->cr |= UART011_CR_RTS;
> +		} else {
> +			uap->cr &= ~UART011_CR_RTS;
> +		}

Checkpatch doesn't find the problem here. Please tell me what's wrong here?

thanks,

Qiumiao Zhang

Re: [PATCH v3] tty: make pl011 serial port driver support 485 mode

[Greg KH]

On Thu, Jan 14, 2021 at 05:07:33PM +0800, zhangqiumiao1@huawei.com wrote:
> From: Qiumiao Zhang <zhangqiumiao1@huawei.com>
> 
> make pl011 serial port support 485 mode full duplex communication
> 
> ---
> Changes in v3:
>   -Fix busy loop forever in pl011_tx_empty
>   -Move the definition of cr into uart_amba_port
>   -run checkpatch with no error or warning
> 
> Changes in v2:
>   -Fix two compilation errors
> 
>  drivers/tty/serial/amba-pl011.c | 32 ++++++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)

Hi,

This is the friendly patch-bot of Greg Kroah-Hartman.  You have sent him
a patch that has triggered this response.  He used to manually respond
to these common problems, but in order to save his sanity (he kept
writing the same thing over and over, yet to different people), I was
created.  Hopefully you will not take offence and will fix the problem
in your patch and resubmit it so that it can be accepted into the Linux
kernel tree.

You are receiving this message because of the following common error(s)
as indicated below:

- Your patch does not have a Signed-off-by: line.  Please read the
  kernel file, Documentation/SubmittingPatches and resend it after
  adding that line.  Note, the line needs to be in the body of the
  email, before the patch, not at the bottom of the patch or in the
  email signature.

If you wish to discuss this problem further, or you have questions about
how to resolve this issue, please feel free to respond to this email and
Greg will reply once he has dug out from the pending patches received
from other developers.

thanks,

greg k-h's patch email bot