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

On Wed, Jan 17, 2024 at 9:41 AM Tudor Ambarus [off-list ref] wrote:

On 1/16/24 18:38, Sam Protsenko wrote:

quoted

On Wed, Jan 10, 2024 at 4:24 AM Tudor Ambarus [off-list ref] wrote:

quoted

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.

quoted

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.

quoted

without any comparison operators.

quoted

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>

quoted

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel