Re: [PATCH next v2 1/4] serial: 8250: Split out IER from rs485_start_tx()

[John Ogness <john.ogness@linutronix.de>]

On 2024-09-17, Petr Mladek <pmladek@suse.com> 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