[PATCH] lib: utils: Fix shakti uart implementation

[Anup Patel]

On Tue, Dec 8, 2020 at 9:38 AM Vijai Kumar K <vijai at behindbytes.com> wrote:
>
> Hi Jessica,
>
> Thank you for reviewing the code. Please see the inline responses.
>
>
> ---- On Mon, 07 Dec 2020 22:44:06 +0530 Jessica Clarke <jrtc27 at jrtc27.com> wrote ----
>
>  > > On 7 Dec 2020, at 16:54, Vijai Kumar K <vijai at behindbytes.com> wrote:
>  > >
>  > > Fix uart_putc implementation.
>  > > Due to a bug in the IP, this went unnoticed.
>  > > Use macros instead of magic numbers to make the code
>  > > more readable.
>  >
>  > Yeah, using macros is good, but this patch isn't doing anything other
>  > than adding macros; there's no change of behaviour? Either you've
>  > missed something or your commit message needs to be fixed.
>
> If you see we are dropping the "==0" comparison. That UART_TX_FULL
> bit is set when TX is full. So that comparison is wrong and we are dropping
> it.
>
>  >
>  > Also see below for a style comment.
>  >
>  > Jess
>  >
>  > > Signed-off-by: Vijai Kumar K <vijai at behindbytes.com>
>  > > ---
>  > > lib/utils/serial/shakti-uart.c | 7 +++++--
>  > > 1 file changed, 5 insertions(+), 2 deletions(-)
>  > >
>  > > diff --git a/lib/utils/serial/shakti-uart.c b/lib/utils/serial/shakti-uart.c
>  > > index 493edcf..b2deb70 100644
>  > > --- a/lib/utils/serial/shakti-uart.c
>  > > +++ b/lib/utils/serial/shakti-uart.c
>  > > @@ -18,18 +18,21 @@
>  > > #define REG_IQ_CYCLES    0x1C
>  > > #define REG_RX_THRES    0x20
>  > >
>  > > +#define UART_TX_FULL  0x2
>  > > +#define UART_RX_FULL  0x8
>  > > +
>  > > static volatile void *uart_base;
>  > >
>  > > void shakti_uart_putc(char ch)
>  > > {
>  > > -    while((readw(uart_base + REG_STATUS) & 0x2) == 0);
>  > > +    while((readw(uart_base + REG_STATUS) & UART_TX_FULL));
>  >
>  > You should have a space after the while. Also, the existing style is to
>  > put the semicolon on a new line so it's not accidentally overlooked.
>
> Got it. Will add the space. However, I don't see the style recommendation
> for putting the semicolon in a new line. Have I overlooked it, or is it
> just an undocumented practice or a personal taste?

Yes, style documentation is not available yet.

We are largely following Linux and U-Boot coding style.

I will wait for your revised patch.

Regards,
Anup