Messages in this thread | | | Date | Tue, 22 Nov 2022 08:37:58 +0100 | From | Jiri Slaby <> | Subject | Re: [PATCH v5 09/14] serial: liteuart: fix rx loop variable types |
| |
On 21. 11. 22, 14:55, Gabriel L. Somlo wrote: > Hi Jiri, > > Thanks for the feedback! > > On Mon, Nov 21, 2022 at 09:45:05AM +0100, Jiri Slaby wrote: >> On 21. 11. 22, 9:37, Jiri Slaby wrote: >>> On 18. 11. 22, 15:55, Gabriel Somlo wrote: >>>> Update variable types to match the signature of uart_insert_char() >>>> which consumes them. >>>> >>>> Signed-off-by: Gabriel Somlo <gsomlo@gmail.com> >>>> Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> >>>> --- >>>> drivers/tty/serial/liteuart.c | 3 +-- >>>> 1 file changed, 1 insertion(+), 2 deletions(-) >>>> >>>> diff --git a/drivers/tty/serial/liteuart.c >>>> b/drivers/tty/serial/liteuart.c >>>> index 81aa7c1da73c..42ac9aee050a 100644 >>>> --- a/drivers/tty/serial/liteuart.c >>>> +++ b/drivers/tty/serial/liteuart.c >>>> @@ -73,8 +73,7 @@ static void liteuart_timer(struct timer_list *t) >>>> struct liteuart_port *uart = from_timer(uart, t, timer); >>>> struct uart_port *port = &uart->port; >>>> unsigned char __iomem *membase = port->membase; >>>> - int ch; >>>> - unsigned long status; >>>> + unsigned int status, ch; >>> >>> These should be u8 after all, right? > > OK, so: > > - I can hard-code `status` as `1`, like so: > > while(!litex_read8(membase + OFF_RXEMPTY) { > ... > if (!(uart_handle_sysrq_char(port, ch))) > uart_insert_char(port, 1, 0, ch, TTY_NORMAL); > > ... since `status` would always be `1` inside the loop. So I'm > basically going to get rid of it altogether.
Yes, I had that in my mind. Except passing 1 to uart_insert_char() when overflow is hardwired to 0 makes no sense IMO :).
> - `ch` is indeed *produced* by `litex_read8()`, which would make it > `u8`. It is subsequently *consumed* by `uart_handle_sysrq_char()` > and `uart_insert_char()`, which both expect `unsigned int`.
Ignore uart_handle_sysrq_char and uart_insert_char. They should be fixed one day. It should really be u8. All down the call chain (it magically turns into int in the sysrq handlers, then char is expected).
> If you think it's better to go with the type when the value is > produced (as opposed to when it's consumed), I'm OK with that for > the upcoming v6 of the series... :)
Yes, please. We should slowly convert _all_ of them.
>> And can you change membase to u8 * too 8-)? > > Hmmm, in `struct uart_port` (in include/linux/serial_core.h), the > `membase` field is declared as: > > unsigned char __iomem *membase; > > which is why I'm thinking we should leave it as-is? Unless there are > plans (or a pending patch I'm unaware of) to switch the field in > include/linux/serial_core.h to `u8` as well? -- Please advise.
Ah, then keep it. I somehow thought it's void *. And yes, even this should be u8 __iomem *, eventually.
thanks, -- js suse labs
| |