On Wed 2024-09-18 17:10:53, John Ogness wrote:
> 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.
IMHO, the answer "Yes" to both last questions can't be valid.
The 8250_bcm2835aux driver does:
static int bcm2835aux_serial_probe(struct platform_device *pdev)
{
[...]
up.rs485_start_tx = bcm2835aux_rs485_start_tx;
[...]
}
As a result, the 1st "Yes" was not correct:
up->rs485_start_tx != serial8250_em485_start_tx
and this patch would change the behavior for the 8250_bcm2835aux driver.
Before, start_tx_rs485() called directly:
up->rs485_start_tx(up);
Newly, it would call:
void serial8250_rs485_start_tx(struct uart_8250_port *up)
{
if (!(up->port.rs485.flags & SER_RS485_RX_DURING_TX))
serial8250_stop_rx(&up->port);
up->rs485_start_tx(up);
}
It means that it could call serial8250_stop_rx() even when it was not
called by the original code.
And SER_RS485_RX_DURING_TX seems to be checked even in
drivers/tty/serial/8250/8250_bcm2835aux.c. So, it looks like it
might be (un)set even for this driver.
Or is this code path prevented in start_tx_rs485()? I mean that
em485->tx_stopped could never be true for the 8250_bcm2835aux
driver?
But I see
static int bcm2835aux_serial_probe(struct platform_device *pdev)
{
[...]
up.port.rs485_config = serial8250_em485_config;
up.port.rs485_supported = serial8250_em485_supported;
[...]
}
=> It looks like even bcm2835aux driver could have the em485 thing.
But it obviously wanted to something special in
up->rs485_start_tx().
It looks to me that the change might either cause regression.
Or it would deserve a comment unless the validity is obvious for people
familiar with the code.
Best Regards,
Petr
