[PATCH v1 1/1] serial: 8520_ce4100: Reuse mem_serial_in() in ce4100_mem_serial_in()

[PATCH v1 1/1] serial: 8520_ce4100: Reuse mem_serial_in() in ce4100_mem_serial_in()

[Andy Shevchenko]

In one place in ce4100_mem_serial_in() the code may be replaced with
mem_serial_in() call. Do it so and collapse two conditionals into one.

Suggested-by: Jiri Slaby <jirislaby@kernel.org>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/tty/serial/8250/8250_ce4100.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_ce4100.c b/drivers/tty/serial/8250/8250_ce4100.c
index 3dd88f372a51..8221b872fd33 100644
--- a/drivers/tty/serial/8250/8250_ce4100.c
+++ b/drivers/tty/serial/8250/8250_ce4100.c
@@ -35,13 +35,8 @@ static u32 ce4100_mem_serial_in(struct uart_port *p, unsigned int offset)
 {
 	u32 ret, ier, lsr;
 
-	if (offset != UART_IIR)
-		return mem_serial_in(p, offset);
-
-	offset <<= p->regshift;
-
-	ret = readl(p->membase + offset);
-	if (!(ret & UART_IIR_NO_INT))
+	ret = mem_serial_in(p, offset);
+	if (!(offset == UART_IIR) && (ret & UART_IIR_NO_INT))
 		return ret;
 
 	/* see if the TX interrupt should have really set */
-- 
2.47.2

Re: [PATCH v1 1/1] serial: 8520_ce4100: Reuse mem_serial_in() in ce4100_mem_serial_in()

[Jiri Slaby]

On 30. 06. 25, 14:54, Andy Shevchenko wrote:
> In one place in ce4100_mem_serial_in() the code may be replaced with
> mem_serial_in() call. Do it so and collapse two conditionals into one.
> 
> Suggested-by: Jiri Slaby <jirislaby@kernel.org>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>   drivers/tty/serial/8250/8250_ce4100.c | 9 ++-------
>   1 file changed, 2 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_ce4100.c b/drivers/tty/serial/8250/8250_ce4100.c
> index 3dd88f372a51..8221b872fd33 100644
> --- a/drivers/tty/serial/8250/8250_ce4100.c
> +++ b/drivers/tty/serial/8250/8250_ce4100.c
> @@ -35,13 +35,8 @@ static u32 ce4100_mem_serial_in(struct uart_port *p, unsigned int offset)
>   {
>   	u32 ret, ier, lsr;
>   
> -	if (offset != UART_IIR)
> -		return mem_serial_in(p, offset);
> -
> -	offset <<= p->regshift;
> -
> -	ret = readl(p->membase + offset);
> -	if (!(ret & UART_IIR_NO_INT))
> +	ret = mem_serial_in(p, offset);
> +	if (!(offset == UART_IIR) && (ret & UART_IIR_NO_INT))

I am in haste, but a misplaced right paren (should be at the end)?

>   		return ret;
>   
>   	/* see if the TX interrupt should have really set */


-- 
js
suse labs

Re: [PATCH v1 1/1] serial: 8520_ce4100: Reuse mem_serial_in() in ce4100_mem_serial_in()

[Andy Shevchenko]

On Mon, Jun 30, 2025 at 03:02:11PM +0200, Jiri Slaby wrote:
> On 30. 06. 25, 14:54, Andy Shevchenko wrote:
> > In one place in ce4100_mem_serial_in() the code may be replaced with
> > mem_serial_in() call. Do it so and collapse two conditionals into one.

...

> >   	u32 ret, ier, lsr;
> > -	if (offset != UART_IIR)
> > -		return mem_serial_in(p, offset);
> > -
> > -	offset <<= p->regshift;
> > -
> > -	ret = readl(p->membase + offset);
> > -	if (!(ret & UART_IIR_NO_INT))
> > +	ret = mem_serial_in(p, offset);
> > +	if (!(offset == UART_IIR) && (ret & UART_IIR_NO_INT))
> 
> I am in haste, but a misplaced right paren (should be at the end)?

Ah, good catch! It's probably better in the original form, i.e.

	if ((offset != UART_IIR) || !(ret & UART_IIR_NO_INT))

What do you think?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/1] serial: 8520_ce4100: Reuse mem_serial_in() in ce4100_mem_serial_in()

[Jiri Slaby]

On 30. 06. 25, 15:30, Andy Shevchenko wrote:
> On Mon, Jun 30, 2025 at 03:02:11PM +0200, Jiri Slaby wrote:
>> On 30. 06. 25, 14:54, Andy Shevchenko wrote:
>>> In one place in ce4100_mem_serial_in() the code may be replaced with
>>> mem_serial_in() call. Do it so and collapse two conditionals into one.
> 
> ...
> 
>>>    	u32 ret, ier, lsr;
>>> -	if (offset != UART_IIR)
>>> -		return mem_serial_in(p, offset);
>>> -
>>> -	offset <<= p->regshift;
>>> -
>>> -	ret = readl(p->membase + offset);
>>> -	if (!(ret & UART_IIR_NO_INT))
>>> +	ret = mem_serial_in(p, offset);
>>> +	if (!(offset == UART_IIR) && (ret & UART_IIR_NO_INT))
>>
>> I am in haste, but a misplaced right paren (should be at the end)?
> 
> Ah, good catch! It's probably better in the original form, i.e.
> 
> 	if ((offset != UART_IIR) || !(ret & UART_IIR_NO_INT))
> 
> What do you think?

Without the parens around the offset check, yes.

-- 
js
suse labs