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
