Re: [PATCH v3 4/6] serial: 8250: Adjust misleading LSR related comment

[Andy Shevchenko]

On Tue, Jun 7, 2022 at 11:16 AM Ilpo Järvinen
<ilpo.jarvinen@xxxxxxxxxxxxxxx> wrote:
>
> serial8250_rx_chars() has max_count based character limit. If it
> triggers, the function returns old LSR value (and it has never returned

the old

> only flags which were not handled). Adjust the comment to match
> behavior and warn about which flags can be depended on.
>
> While I'd have moved LSR read before LSR read and used serial_lsr_in()
> also here but I came across this old discussion about the topic:

>   https://www.spinics.net/lists/linux-serial/msg20555.html

Can it be transformed to lore.kernel.org link? and maybe even moved as
BugLink tag?

> ...so I left it as it is (it works as long as the callers only use
> a subset of the LSR flags which holds true today).

With comments addressed,
Reviewed-by: Andy Shevchenko <andy.shevchenko@xxxxxxxxx>

> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@xxxxxxxxxxxxxxx>
> ---
>  drivers/tty/serial/8250/8250_port.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> index a0ea048eb2ad..686891f1b2ca 100644
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -1782,9 +1782,12 @@ void serial8250_read_char(struct uart_8250_port *up, unsigned char lsr)
>  EXPORT_SYMBOL_GPL(serial8250_read_char);
>
>  /*
> - * serial8250_rx_chars: processes according to the passed in LSR
> - * value, and returns the remaining LSR bits not handled
> - * by this Rx routine.
> + * serial8250_rx_chars: Read characters. The first LSR value must be passed
> + * in.

While at it, I would do the following:
1) keep on one line;
2) replace : with -.

The idea behind is to easily convert to kernel doc in the future if
needed, or at least be consistent with kernel doc format.

> + *
> + * Returns LSR bits. The caller should rely only non-rx related LSR bits

rely only on

non-Rx

> + * (such as THRE) because the LSR value might come from an already consumed
> + * character.
>   */


-- 
With Best Regards,
Andy Shevchenko