On 2024-09-17, Petr Mladek <pmladek@xxxxxxxx> wrote:
> Sigh, I am trying to review this patch but I am not familiar with the
> code. Feel free to ignore me when the questions are completely off.
I appreciate you researching where the code came from. I made my changes
based on what I see the code doing now.
>> --- a/drivers/tty/serial/8250/8250_port.c
>> +++ b/drivers/tty/serial/8250/8250_port.c
>> void serial8250_em485_start_tx(struct uart_8250_port *up)
>> {
>> unsigned char mcr = serial8250_in_MCR(up);
>>
>> + /*
>> + * Some chips set the UART_LSR_DR bit even when UART_IER_RDI is
>> + * disabled, so explicitly mask it.
>> + */
>> if (!(up->port.rs485.flags & SER_RS485_RX_DURING_TX))
>> - serial8250_stop_rx(&up->port);
>> + up->port.read_status_mask &= ~UART_LSR_DR;
>
> This change is related to disabling UART_IER_RDI but we do not longer
> disable it in this code path.
Correct. It will be disabled in the new wrapper
serial8250_em485_start_tx(). For the console write() callback, RDI is
already being disabled (IER is cleared). It will not use the wrapper.
> Why do we need to do it here, please?
Because the console write() callback also needs to clear LSR_DR. That
part of the callback needs to stay.
> Why is it needed only in the em485-specific path, please?
Only RS485 deals with controlling TX/RX directions.
> On one hand, the comment talks about UART_LSR_DR and UART_IER_RDI
> so seems to be relater.
I do not know if the LSR_DR modify is strictly necessary. I am just
preserving the existing behavior (and related comment). The disabling of
IER_RDI will still happen (via wrapper or explicitly as in the console
write() callback).
>> static bool start_tx_rs485(struct uart_port *port)
>> {
>> @@ -1585,7 +1600,7 @@ static bool start_tx_rs485(struct uart_port *port)
>> if (em485->tx_stopped) {
>> em485->tx_stopped = false;
>>
>> - up->rs485_start_tx(up);
>> + serial8250_rs485_start_tx(up);
>
> If I get this correctly then this keeps the existing behavior when
>
> up->rs485_start_tx == serial8250_em485_start_tx
Correct.
> Is this always the case, please?
Yes.
> Can start_tx_rs485() be called for the 8250_bcm2835aux.c driver?
Yes.
> Will it still work as expected?
Yes, but it does perform an extra read. And since someone added a
comment just to mention that, I assume it was important for some use
case.
John
