diff mbox series

[11/18] tty: serial: samsung: don't compare with zero an if (bitwise expression)

Message ID 20240110102102.61587-12-tudor.ambarus@linaro.org (mailing list archive)
State New
Headers show
Series serial: samsung: gs101 updates and winter cleanup | expand

Commit Message

Tudor Ambarus Jan. 10, 2024, 10:20 a.m. UTC
Since an if tests the numeric value of an expression, certain coding
shortcuts can be used. The most obvious one is writing
    if (expression)
instead of
    if (expression != 0)

Since our case is a bitwise expression, it's more natural and clear to
use the ``if (expression)`` shortcut.

Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
---
 drivers/tty/serial/samsung_tty.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

Comments

Sam Protsenko Jan. 16, 2024, 6:38 p.m. UTC | #1
On Wed, Jan 10, 2024 at 4:24 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>
> Since an if tests the numeric value of an expression, certain coding
> shortcuts can be used. The most obvious one is writing
>     if (expression)
> instead of
>     if (expression != 0)
>
> Since our case is a bitwise expression, it's more natural and clear to
> use the ``if (expression)`` shortcut.

Maybe the author of this code:

    (ufstat & info->tx_fifomask) != 0

just wanted to outline (logically) that the result of this bitwise
operation produces FIFO length, which he checks to have non-zero
length? Mechanically of course it doesn't matter much, and I guess
everyone can understand what's going on there even without '!= 0'
part. But it looks quite intentional to me, because in the same 'if'
block the author uses this as well:

    (ufstat & info->tx_fifofull)

without any comparison operators.

>
> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
> ---
>  drivers/tty/serial/samsung_tty.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
> index dbbe6b8e3ceb..f2413da14b1d 100644
> --- a/drivers/tty/serial/samsung_tty.c
> +++ b/drivers/tty/serial/samsung_tty.c
> @@ -988,8 +988,7 @@ static unsigned int s3c24xx_serial_tx_empty(struct uart_port *port)
>         u32 ufcon = rd_regl(port, S3C2410_UFCON);
>
>         if (ufcon & S3C2410_UFCON_FIFOMODE) {
> -               if ((ufstat & info->tx_fifomask) != 0 ||
> -                   (ufstat & info->tx_fifofull))
> +               if ((ufstat & info->tx_fifomask) || (ufstat & info->tx_fifofull))

Does this line fit into 80 characters? If no, please rework it so it
does. I guess it's also possible to get rid of superfluous braces
there, but then the code might look confusing, and I'm not sure if
checkpatch would be ok with that.

>                         return 0;
>
>                 return 1;
> --
> 2.43.0.472.g3155946c3a-goog
>
>
Tudor Ambarus Jan. 17, 2024, 3:41 p.m. UTC | #2
On 1/16/24 18:38, Sam Protsenko wrote:
> On Wed, Jan 10, 2024 at 4:24 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>>
>> Since an if tests the numeric value of an expression, certain coding
>> shortcuts can be used. The most obvious one is writing
>>     if (expression)
>> instead of
>>     if (expression != 0)
>>
>> Since our case is a bitwise expression, it's more natural and clear to
>> use the ``if (expression)`` shortcut.
> 
> Maybe the author of this code:
> 
>     (ufstat & info->tx_fifomask) != 0
> 
> just wanted to outline (logically) that the result of this bitwise
> operation produces FIFO length, which he checks to have non-zero
> length? Mechanically of course it doesn't matter much, and I guess

that's a bitwise AND with the fifo mask to check if the fifo is empty or
not, it doesn't care about the length, just if the fifo is empty. IOW if
any of those bits are set, the fifo is not empty. I think not comparing
with zero explicitly is better. At the same time I'm fine dropping the
patch as well. So please tell me if you want me to reword the commit
message or drop the patch entirely.

> everyone can understand what's going on there even without '!= 0'
> part. But it looks quite intentional to me, because in the same 'if'
> block the author uses this as well:
> 
>     (ufstat & info->tx_fifofull)

tx_fifofull is just a bit in the register, in my case BIT(24). If that
bit is one, the fifo is full. Not comparing with zero is fine here, as
we're interested just in that bit/flag.

> 
> without any comparison operators.
> 
>>
>> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
>> ---
>>  drivers/tty/serial/samsung_tty.c | 3 +--
>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
>> index dbbe6b8e3ceb..f2413da14b1d 100644
>> --- a/drivers/tty/serial/samsung_tty.c
>> +++ b/drivers/tty/serial/samsung_tty.c
>> @@ -988,8 +988,7 @@ static unsigned int s3c24xx_serial_tx_empty(struct uart_port *port)
>>         u32 ufcon = rd_regl(port, S3C2410_UFCON);
>>
>>         if (ufcon & S3C2410_UFCON_FIFOMODE) {
>> -               if ((ufstat & info->tx_fifomask) != 0 ||
>> -                   (ufstat & info->tx_fifofull))
>> +               if ((ufstat & info->tx_fifomask) || (ufstat & info->tx_fifofull))
> 
> Does this line fit into 80 characters? If no, please rework it so it

it fits

> does. I guess it's also possible to get rid of superfluous braces
> there, but then the code might look confusing, and I'm not sure if
> checkpatch would be ok with that.
> 

I find it better with the braces.

Thanks!
ta
Sam Protsenko Jan. 17, 2024, 4:24 p.m. UTC | #3
On Wed, Jan 17, 2024 at 9:41 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>
>
>
> On 1/16/24 18:38, Sam Protsenko wrote:
> > On Wed, Jan 10, 2024 at 4:24 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
> >>
> >> Since an if tests the numeric value of an expression, certain coding
> >> shortcuts can be used. The most obvious one is writing
> >>     if (expression)
> >> instead of
> >>     if (expression != 0)
> >>
> >> Since our case is a bitwise expression, it's more natural and clear to
> >> use the ``if (expression)`` shortcut.
> >
> > Maybe the author of this code:
> >
> >     (ufstat & info->tx_fifomask) != 0
> >
> > just wanted to outline (logically) that the result of this bitwise
> > operation produces FIFO length, which he checks to have non-zero
> > length? Mechanically of course it doesn't matter much, and I guess
>
> that's a bitwise AND with the fifo mask to check if the fifo is empty or
> not, it doesn't care about the length, just if the fifo is empty. IOW if
> any of those bits are set, the fifo is not empty. I think not comparing
> with zero explicitly is better. At the same time I'm fine dropping the
> patch as well. So please tell me if you want me to reword the commit
> message or drop the patch entirely.
>

I'm not opposed to this patch, just don't have any preference in this
case. But the patch is ok with me.

> > everyone can understand what's going on there even without '!= 0'
> > part. But it looks quite intentional to me, because in the same 'if'
> > block the author uses this as well:
> >
> >     (ufstat & info->tx_fifofull)
>
> tx_fifofull is just a bit in the register, in my case BIT(24). If that
> bit is one, the fifo is full. Not comparing with zero is fine here, as
> we're interested just in that bit/flag.
>
> >
> > without any comparison operators.
> >
> >>
> >> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
> >> ---
> >>  drivers/tty/serial/samsung_tty.c | 3 +--
> >>  1 file changed, 1 insertion(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
> >> index dbbe6b8e3ceb..f2413da14b1d 100644
> >> --- a/drivers/tty/serial/samsung_tty.c
> >> +++ b/drivers/tty/serial/samsung_tty.c
> >> @@ -988,8 +988,7 @@ static unsigned int s3c24xx_serial_tx_empty(struct uart_port *port)
> >>         u32 ufcon = rd_regl(port, S3C2410_UFCON);
> >>
> >>         if (ufcon & S3C2410_UFCON_FIFOMODE) {
> >> -               if ((ufstat & info->tx_fifomask) != 0 ||
> >> -                   (ufstat & info->tx_fifofull))
> >> +               if ((ufstat & info->tx_fifomask) || (ufstat & info->tx_fifofull))
> >
> > Does this line fit into 80 characters? If no, please rework it so it
>
> it fits
>

Just checked, and it's 1 character off (so it has length of 81
characters). I know it's not a strong rule in kernel anymore, but I
like it personally. If you are going to fix that, be free to add:

Reviewed-by: Sam Protsenko <semen.protsenko@linaro.org>

> > does. I guess it's also possible to get rid of superfluous braces
> > there, but then the code might look confusing, and I'm not sure if
> > checkpatch would be ok with that.
> >
>
> I find it better with the braces.
>
> Thanks!
> ta
diff mbox series

Patch

diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
index dbbe6b8e3ceb..f2413da14b1d 100644
--- a/drivers/tty/serial/samsung_tty.c
+++ b/drivers/tty/serial/samsung_tty.c
@@ -988,8 +988,7 @@  static unsigned int s3c24xx_serial_tx_empty(struct uart_port *port)
 	u32 ufcon = rd_regl(port, S3C2410_UFCON);
 
 	if (ufcon & S3C2410_UFCON_FIFOMODE) {
-		if ((ufstat & info->tx_fifomask) != 0 ||
-		    (ufstat & info->tx_fifofull))
+		if ((ufstat & info->tx_fifomask) || (ufstat & info->tx_fifofull))
 			return 0;
 
 		return 1;