Re:Re: [PATCH 1/2] tty/serial: delete break after return
From: "Uwe Kleine-König"
Date: 2020-11-07 22:01:29
To: Bernard Zhao
Cc: Greg Kroah-Hartman ,Jiri Slaby
,Shawn Guo ,Sascha Hauer
,Pengutronix Kernel Team ,Fabio
Estevam ,NXP Linux Team
,linux-kernel@vger.kernel.org,linux-ser...@vger.kernel.org,linux-arm-ker...@lists.infradead.org,opensource.ker...@vivo.com
Subject: Re: [PATCH 1/2] tty/serial: delete break after return>Hello,
>
>the Subject is wrong, it should use a prefix similar to "serial: imx:".
>It's a good idea to check previous patches to the same file to pick a
>suitable prefix. (E.g. git log --oneline drivers/tty/serial/imx.c)
Hi, Uwe:
Thank you for your suggestion, I will make a more accurate subject in my future
patches.
>On Fri, Nov 06, 2020 at 07:29:23PM -0800, Bernard Zhao wrote:
>> Delete break after return, which will never run.
>>
>> Signed-off-by: Bernard Zhao
>> ---
>> drivers/tty/serial/imx.c | 5 -
>> 1 file changed, 5 deletions(-)
>>
>> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
>> index 1731d9728865..09703079db7b 100644
>> --- a/drivers/tty/serial/imx.c
>> +++ b/drivers/tty/serial/imx.c
>> @@ -320,7 +320,6 @@ static u32 imx_uart_readl(struct imx_port *sport, u32
>> offset)
>> switch (offset) {
>> case UCR1:
>> return sport->ucr1;
>> -break;
>> case UCR2:
>> /*
>> * UCR2_SRST is the only bit in the cached registers that might
>> @@ -331,16 +330,12 @@ static u32 imx_uart_readl(struct imx_port *sport, u32
>> offset)
>> if (!(sport->ucr2 & UCR2_SRST))
>> sport->ucr2 = readl(sport->port.membase + offset);
>> return sport->ucr2;
>> -break;
>> case UCR3:
>> return sport->ucr3;
>> -break;
>> case UCR4:
>> return sport->ucr4;
>> -break;
>> case UFCR:
>> return sport->ufcr;
>> -break;
>
>you're the third to send this patch since October 20:
>
> https://lore.kernel.org/r/20201026125142.21105-1-zhangqilo...@huawei.com
> https://lore.kernel.org/r/20201020130709.28096-1-t...@redhat.com
>
>My comment for these was:
>
>> this might be subjective, but I like the break being there for clearity.
>> So I object to make a patch to remove them. In case I'm outvoted I'd at
>> least want empty lines instead.
>
>Zhang Qilong wrote he found the patch opportunity by manual code
>inspection, I would have expected that there is a tool that identifies a
>break after a return. If you had tool support, please mention the tool
>in the commit log (if you really want to keep following the patch's
>idea).
For this part:
I wrote a python script to check if there is a break after a return or goto.
The script currently has some issues in its handling of special characters, the
search result is not precise enough. It is still under debugging.
These are the only places which the script checks out in path: driver/tty.
I also checked other path(like: driver/vme/, driver/video, driver/usb), these
paths also have a certain number of similar issues. And I will try to submit
these patch later.
Thanks!
BR//Bernard
>Best regards
>Uwe
>
>--
>Pengutronix e.K. | Uwe Kleine-König|
>Industrial Linux Solutions | https://www.pengutronix.de/ |
Re: [PATCH 1/2] tty/serial: delete break after return
Hello,
the Subject is wrong, it should use a prefix similar to "serial: imx:".
It's a good idea to check previous patches to the same file to pick a
suitable prefix. (E.g. git log --oneline drivers/tty/serial/imx.c)
On Fri, Nov 06, 2020 at 07:29:23PM -0800, Bernard Zhao wrote:
> Delete break after return, which will never run.
>
> Signed-off-by: Bernard Zhao
> ---
> drivers/tty/serial/imx.c | 5 -
> 1 file changed, 5 deletions(-)
>
> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> index 1731d9728865..09703079db7b 100644
> --- a/drivers/tty/serial/imx.c
> +++ b/drivers/tty/serial/imx.c
> @@ -320,7 +320,6 @@ static u32 imx_uart_readl(struct imx_port *sport, u32
> offset)
> switch (offset) {
> case UCR1:
> return sport->ucr1;
> - break;
> case UCR2:
> /*
>* UCR2_SRST is the only bit in the cached registers that might
> @@ -331,16 +330,12 @@ static u32 imx_uart_readl(struct imx_port *sport, u32
> offset)
> if (!(sport->ucr2 & UCR2_SRST))
> sport->ucr2 = readl(sport->port.membase + offset);
> return sport->ucr2;
> - break;
> case UCR3:
> return sport->ucr3;
> - break;
> case UCR4:
> return sport->ucr4;
> - break;
> case UFCR:
> return sport->ufcr;
> - break;
you're the third to send this patch since October 20:
https://lore.kernel.org/r/20201026125142.21105-1-zhangqilo...@huawei.com
https://lore.kernel.org/r/20201020130709.28096-1-t...@redhat.com
My comment for these was:
> this might be subjective, but I like the break being there for clearity.
> So I object to make a patch to remove them. In case I'm outvoted I'd at
> least want empty lines instead.
Zhang Qilong wrote he found the patch opportunity by manual code
inspection, I would have expected that there is a tool that identifies a
break after a return. If you had tool support, please mention the tool
in the commit log (if you really want to keep following the patch's
idea).
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |
signature.asc
Description: PGP signature
[PATCH 1/2] tty/serial: delete break after return
Delete break after return, which will never run.
Signed-off-by: Bernard Zhao
---
drivers/tty/serial/imx.c | 5 -
1 file changed, 5 deletions(-)
diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 1731d9728865..09703079db7b 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -320,7 +320,6 @@ static u32 imx_uart_readl(struct imx_port *sport, u32
offset)
switch (offset) {
case UCR1:
return sport->ucr1;
- break;
case UCR2:
/*
* UCR2_SRST is the only bit in the cached registers that might
@@ -331,16 +330,12 @@ static u32 imx_uart_readl(struct imx_port *sport, u32
offset)
if (!(sport->ucr2 & UCR2_SRST))
sport->ucr2 = readl(sport->port.membase + offset);
return sport->ucr2;
- break;
case UCR3:
return sport->ucr3;
- break;
case UCR4:
return sport->ucr4;
- break;
case UFCR:
return sport->ufcr;
- break;
default:
return readl(sport->port.membase + offset);
}
--
2.29.0
