Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <03467516-3962-4ff2-23d2-2b3a1d647c5a@kernel.org>
Date:   Thu, 23 Jun 2022 09:41:24 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.10.0
Subject: Re: [PATCH v2 6/6] serial: Consolidate BOTH_EMPTY use
Content-Language: en-US
To:     =?UTF-8?Q?Ilpo_J=c3=a4rvinen?= <ilpo.jarvinen@linux.intel.com>,
        linux-serial@vger.kernel.org, Greg KH <gregkh@linuxfoundation.org>,
        Thomas Bogendoerfer <tsbogend@alpha.franken.de>,
        William Hubbs <w.d.hubbs@gmail.com>,
        Chris Brannon <chris@the-brannons.com>,
        Kirk Reiser <kirk@reisers.ca>,
        Samuel Thibault <samuel.thibault@ens-lyon.org>,
        "David S. Miller" <davem@davemloft.net>,
        linux-mips@vger.kernel.org, linux-kernel@vger.kernel.org,
        speakup@linux-speakup.org, sparclinux@vger.kernel.org
References: <20220621124958.3342-1-ilpo.jarvinen@linux.intel.com>
 <20220621124958.3342-7-ilpo.jarvinen@linux.intel.com>
From:   Jiri Slaby <jirislaby@kernel.org>
In-Reply-To: <20220621124958.3342-7-ilpo.jarvinen@linux.intel.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 21. 06. 22, 14:49, Ilpo Järvinen wrote:
> Per file BOTH_EMPTY defines are littering our source code here and
> there. Define once in serial.h and create helper for the check
> too.
> 
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

Reviewed-by: Jiri Slaby <jirislaby@kernel.org>

> --- a/arch/mips/ath79/early_printk.c
> +++ b/arch/mips/ath79/early_printk.c
> @@ -8,6 +8,7 @@
>   
>   #include <linux/io.h>
>   #include <linux/errno.h>
> +#include <linux/serial.h>
>   #include <linux/serial_reg.h>
>   #include <asm/addrspace.h>
>   #include <asm/setup.h>
> @@ -29,15 +30,15 @@ static inline void prom_putchar_wait(void __iomem *reg, u32 mask, u32 val)
>   	} while (1);
>   }
>   
> -#define BOTH_EMPTY (UART_LSR_TEMT | UART_LSR_THRE)
> -
>   static void prom_putchar_ar71xx(char ch)
>   {
>   	void __iomem *base = (void __iomem *)(KSEG1ADDR(AR71XX_UART_BASE));
>   
> -	prom_putchar_wait(base + UART_LSR * 4, BOTH_EMPTY, BOTH_EMPTY);
> +	prom_putchar_wait(base + UART_LSR * 4, UART_LSR_BOTH_EMPTY,
> +			  UART_LSR_BOTH_EMPTY);
>   	__raw_writel((unsigned char)ch, base + UART_TX * 4);
> -	prom_putchar_wait(base + UART_LSR * 4, BOTH_EMPTY, BOTH_EMPTY);
> +	prom_putchar_wait(base + UART_LSR * 4, UART_LSR_BOTH_EMPTY,
> +			  UART_LSR_BOTH_EMPTY);

Two observations apart from this patch:
* prom_putchar_wait()'s last two parameters are always the same.
   One should be removed, i.e. all this simplified.
* prom_putchar_wait() should be implemented using
   read_poll_timeout_atomic(), incl. failure/timeout handling.

thanks,
-- 
js
suse labs