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

[Tudor Ambarus]

On 1/16/24 18:38, Sam Protsenko wrote:
> On Wed, Jan 10, 2024 at 4:24 AM Tudor Ambarus <tudor.ambarus@xxxxxxxxxx> 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@xxxxxxxxxx>
>> ---
>>  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