Re: [PATCH v2] serial: amba-pl011: add RS485 support

[Lino Sanfilippo]

Hi,

On 24.06.21 at 14:55, Greg KH wrote:
>>
>> +static int pl011_rs485_tx_stop(struct uart_amba_port *uap)
>> +{
>> +	struct uart_port *port = &uap->port;
>> +	u32 cr;
>> +
>> +	/* Wait until hardware tx queue is empty */
>> +	while (!pl011_tx_empty(port))
>> +		udelay(uap->rs485_tx_drain_interval);
>
> No way out if the hardware doesn't ever empty?  Shouldn't you have an
> "upper bound" on this loop somehow?

Yes, indeed. I will fix this.

>
>> +
>> +	if (port->rs485.delay_rts_after_send)
>> +		mdelay(port->rs485.delay_rts_after_send);
>> +
>> +	cr = pl011_read(uap, REG_CR);
>> +
>> +	if (port->rs485.flags & SER_RS485_RTS_AFTER_SEND)
>> +		cr &= ~UART011_CR_RTS;
>> +	else
>> +		cr |= UART011_CR_RTS;
>
> Blank line here please.

Ok.

>
>> +	/* Disable the transmitter and reenable the transceiver */
>> +	cr &= ~UART011_CR_TXE;
>> +	cr |= UART011_CR_RXE;
>> +	pl011_write(cr, uap, REG_CR);
>> +
>> +	uap->rs485_tx_started = false;
>> +
>> +	return 0;
>
> Why does this function return a value if it can not fail and you do not
> check the return value of it?

>> +}
>> +
>>  static void pl011_stop_tx(struct uart_port *port)
>>  {
>>  	struct uart_amba_port *uap =
>> @@ -1290,6 +1322,9 @@ 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) && uap->rs485_tx_started)
>> +		pl011_rs485_tx_stop(uap);
>
> So, no check :(
>

Ah, right. The return value is a leftover from an earlier version of the function. I will
correct this in the next patch version.

>
>>  }
>>
>>  static bool pl011_tx_chars(struct uart_amba_port *uap, bool from_irq);
>> @@ -1380,6 +1415,31 @@ static bool pl011_tx_char(struct uart_amba_port *uap, unsigned char c,
>>  	return true;
>>  }
>>
>> +static void pl011_rs485_tx_start(struct uart_amba_port *uap)
>> +{
>> +	struct uart_port *port = &uap->port;
>> +	u32 cr;
>> +
>> +	/* Enable transmitter */
>> +	cr = pl011_read(uap, REG_CR);
>> +	cr |= UART011_CR_TXE;
>
> Blank line please.
>

Ok.

>> +
>>  	spin_lock_irqsave(&port->lock, flags);
>>
>>  	/*
>>  	 * Update the per-port timeout.
>>  	 */
>>  	uart_update_timeout(port, termios->c_cflag, baud);
>
> Blank line
>

Ok.

>>
>> +static int pl011_rs485_config(struct uart_port *port,
>> +			      struct serial_rs485 *rs485)
>> +{
>> +	struct uart_amba_port *uap =
>> +		container_of(port, struct uart_amba_port, port);
>> +
>> +	/* pick sane settings if the user hasn't */
>> +	if (!!(rs485->flags & SER_RS485_RTS_ON_SEND) ==
>
> Why the !! in an if statement?
>
>> +	    !!(rs485->flags & SER_RS485_RTS_AFTER_SEND)) {
>
> Same here, why?
>

This was copied from serial8250_em485_config(). But I think we can simply use

	if (rs485->flags & SER_RS485_RTS_AFTER_SEND)
		rs485->flags &= ~SER_RS485_RTS_ON_SEND;
	else
		rs485->flags |= SER_RS485_RTS_ON_SEND;

instead. I will adjust the code accordingly.

>> +
>> +	if (port->rs485.flags & SER_RS485_ENABLED)
>> +		pl011_rs485_tx_stop(uap);
>> +
>> +	/* Set new configuration */
>> +	port->rs485 = *rs485;
>
> Blank line please.
>

Ok.

>
> thanks,
>
> greg k-h
>

Thank you for the review!

Regards,
Lino