| Message ID | 20240110102102.61587-12-tudor.ambarus@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | serial: samsung: gs101 updates and winter cleanup | expand |
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 > >
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
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 --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;
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(-)