[PATCH -next] serial: imx: remove set but not used variable 'rtsirq'

[PATCH -next] serial: imx: remove set but not used variable 'rtsirq'

[YueHaibing]

Fixes gcc '-Wunused-but-set-variable' warning:

drivers/tty/serial/imx.c: In function 'imx_uart_probe':
drivers/tty/serial/imx.c:2198:20: warning:
 variable 'rtsirq' set but not used [-Wunused-but-set-variable]

Signed-off-by: YueHaibing <yuehaibing@huawei.com>
---
 drivers/tty/serial/imx.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 4341589..1df7d23 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -2195,7 +2195,7 @@ static int imx_uart_probe(struct platform_device *pdev)
 	int ret = 0;
 	u32 ucr1;
 	struct resource *res;
-	int txirq, rxirq, rtsirq;
+	int txirq, rxirq;
 
 	sport = devm_kzalloc(&pdev->dev, sizeof(*sport), GFP_KERNEL);
 	if (!sport)
@@ -2220,7 +2220,6 @@ static int imx_uart_probe(struct platform_device *pdev)
 
 	rxirq = platform_get_irq(pdev, 0);
 	txirq = platform_get_irq(pdev, 1);
-	rtsirq = platform_get_irq(pdev, 2);
 
 	sport->port.dev = &pdev->dev;
 	sport->port.mapbase = res->start;

Re: [PATCH -next] serial: imx: remove set but not used variable 'rtsirq'

[Uwe Kleine-König]

On Thu, Sep 20, 2018 at 01:58:45AM +0000, YueHaibing wrote:
> Fixes gcc '-Wunused-but-set-variable' warning:
> 
> drivers/tty/serial/imx.c: In function 'imx_uart_probe':
> drivers/tty/serial/imx.c:2198:20: warning:
>  variable 'rtsirq' set but not used [-Wunused-but-set-variable]
> 
> Signed-off-by: YueHaibing <yuehaibing@huawei.com>

Wow, this variable is write-only since 

	afe9cbb1a6ad ("serial: imx: drop support for IRDA")

which is over three years old. The last hunk of this patch is wrong
however, which means that nobody uses handshaking on imx1 with a kernel
newer than 4.1-rc1. (Well, or they fixed it and didn't made the effort
to tell.)

I suggest to break rx and tx on imx1, too, and if nobody reports a
regression within the next three years, we rip out imx1 support
completely. :-)

Otherwise we need:

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 4e853570ea80..554a69db1bca 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -2350,6 +2350,14 @@ static int imx_uart_probe(struct platform_device *pdev)
 				ret);
 			return ret;
 		}
+
+		ret = devm_request_irq(&pdev->dev, rtsirq, imx_uart_rtsint, 0,
+				       dev_name(&pdev->dev), sport);
+		if (ret) {
+			dev_err(&pdev->dev, "failed to request rts irq: %d\n",
+				ret);
+			return ret;
+		}
 	} else {
 		ret = devm_request_irq(&pdev->dev, rxirq, imx_uart_int, 0,
 				       dev_name(&pdev->dev), sport);

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH] serial: imx: restore handshaking irq for imx1

[Uwe Kleine-König]

Back in 2015 when irda was dropped from the driver imx1 was broken. This
change reintroduces the support for the third interrupt of the UART.

Fixes: afe9cbb1a6ad ("serial: imx: drop support for IRDA")
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/tty/serial/imx.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 4e853570ea80..554a69db1bca 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -2350,6 +2350,14 @@ static int imx_uart_probe(struct platform_device *pdev)
 				ret);
 			return ret;
 		}
+
+		ret = devm_request_irq(&pdev->dev, rtsirq, imx_uart_rtsint, 0,
+				       dev_name(&pdev->dev), sport);
+		if (ret) {
+			dev_err(&pdev->dev, "failed to request rts irq: %d\n",
+				ret);
+			return ret;
+		}
 	} else {
 		ret = devm_request_irq(&pdev->dev, rxirq, imx_uart_int, 0,
 				       dev_name(&pdev->dev), sport);
-- 
2.18.0

Re: [PATCH] serial: imx: restore handshaking irq for imx1

[Leonard Crestez]

On Thu, 2018-09-20 at 14:11 +0200, Uwe Kleine-König wrote:
> Back in 2015 when irda was dropped from the driver imx1 was broken. This
> change reintroduces the support for the third interrupt of the UART.
> 
> Fixes: afe9cbb1a6ad ("serial: imx: drop support for IRDA")
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Reviewed-by: Leonard Crestez <leonard.crestez@nxp.com>

I'm not sure if anyone has imx1 hardware handy can is willing to test
this CTS/RTS but the fix is pretty obviously correct.

RE: [PATCH] serial: imx: restore handshaking irq for imx1

[Andy Duan]

From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> Sent: 2018年9月20日 20:11
> Back in 2015 when irda was dropped from the driver imx1 was broken.
> This change reintroduces the support for the third interrupt of the UART.
> 
> Fixes: afe9cbb1a6ad ("serial: imx: drop support for IRDA")
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/tty/serial/imx.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c index
> 4e853570ea80..554a69db1bca 100644
> --- a/drivers/tty/serial/imx.c
> +++ b/drivers/tty/serial/imx.c
> @@ -2350,6 +2350,14 @@ static int imx_uart_probe(struct
> platform_device *pdev)
>  				ret);
>  			return ret;
>  		}
> +
> +		ret = devm_request_irq(&pdev->dev, rtsirq, imx_uart_rtsint, 0,
> +				       dev_name(&pdev->dev), sport);
> +		if (ret) {
> +			dev_err(&pdev->dev, "failed to request rts irq: %d\n",
> +				ret);
> +			return ret;
> +		}
>  	} else {
>  		ret = devm_request_irq(&pdev->dev, rxirq, imx_uart_int, 0,
>  				       dev_name(&pdev->dev), sport);
> --
> 2.18.0

Reviewed-by: Fugang Duan <fugang.duan@nxp.com>

Re: [PATCH -next] serial: imx: remove set but not used variable 'rtsirq'

[Leonard Crestez]

On Thu, 2018-09-20 at 08:45 +0200, Jiri Slaby wrote:
> On 09/20/2018, 03:58 AM, YueHaibing wrote:
> > Fixes gcc '-Wunused-but-set-variable' warning:
> > 
> > drivers/tty/serial/imx.c: In function 'imx_uart_probe':
> > drivers/tty/serial/imx.c:2198:20: warning:
> >  variable 'rtsirq' set but not used [-Wunused-but-set-variable]
> > 
> > diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> > @@ -2220,7 +2220,6 @@ static int imx_uart_probe(struct platform_device *pdev)
> >  
> >  	rxirq = platform_get_irq(pdev, 0);
> >  	txirq = platform_get_irq(pdev, 1);
> > -	rtsirq = platform_get_irq(pdev, 2);
> 
> I am not sure this is correct. platform_get_irq has side effects (like
> enabling the IRQ). Are you sure this won't change the behaviour (this is
> question mostly to IMX fellows)?

As far as I can tell there was a request_irq call for rtsirq which was
removed by mistake in commit afe9cbb1a6ad ("serial: imx: drop support
for IRDA"):

-               /* do not use RTS IRQ on IrDA */
-               if (!USE_IRDA(sport)) {
-                       ret = devm_request_irq(&pdev->dev, rtsirq,
-                                              imx_rtsint, 0,
-                                              dev_name(&pdev->dev), sport);
-                       if (ret)
-                               return ret;
-               }

This should have just removed the IRDA check and request rtsirq
unconditionally. Nobody noticed this by testing RTS on imx1, this is an
old chip and later variants have a single combined irq.

The correct fix for the warning would be to restore that request_irq.

--
Regards,
Leonard

RE: [PATCH -next] serial: imx: remove set but not used variable 'rtsirq'

[Andy Duan]

From: Leonard Crestez Sent: 2018年9月20日 16:51
> On Thu, 2018-09-20 at 08:45 +0200, Jiri Slaby wrote:
> > On 09/20/2018, 03:58 AM, YueHaibing wrote:
> > > Fixes gcc '-Wunused-but-set-variable' warning:
> > >
> > > drivers/tty/serial/imx.c: In function 'imx_uart_probe':
> > > drivers/tty/serial/imx.c:2198:20: warning:
> > >  variable 'rtsirq' set but not used [-Wunused-but-set-variable]
> > >
> > > diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c @@
> > > -2220,7 +2220,6 @@ static int imx_uart_probe(struct
> platform_device
> > > *pdev)
> > >
> > >  	rxirq = platform_get_irq(pdev, 0);
> > >  	txirq = platform_get_irq(pdev, 1);
> > > -	rtsirq = platform_get_irq(pdev, 2);
> >
> > I am not sure this is correct. platform_get_irq has side effects (like
> > enabling the IRQ). Are you sure this won't change the behaviour (this
> > is question mostly to IMX fellows)?
> 
> As far as I can tell there was a request_irq call for rtsirq which was
> removed by mistake in commit afe9cbb1a6ad ("serial: imx: drop support
> for IRDA"):
> 
> -               /* do not use RTS IRQ on IrDA */
> -               if (!USE_IRDA(sport)) {
> -                       ret = devm_request_irq(&pdev->dev, rtsirq,
> -                                              imx_rtsint, 0,
> -
> dev_name(&pdev->dev), sport);
> -                       if (ret)
> -                               return ret;
> -               }
> 
> This should have just removed the IRDA check and request rtsirq
> unconditionally. Nobody noticed this by testing RTS on imx1, this is an old
> chip and later variants have a single combined irq.
> 
> The correct fix for the warning would be to restore that request_irq.
> 
> --
> Regards,
> Leonard

Yes, your explain is very correct! Thanks for your comment.
We should restore rtsirq request that for i.MX1.

Regards,
Andy Duan

Re: [PATCH -next] serial: imx: remove set but not used variable 'rtsirq'

[YueHaibing]

On 2018/9/20 17:41, Andy Duan wrote:
> From: Leonard Crestez Sent: 2018年9月20日 16:51
>> On Thu, 2018-09-20 at 08:45 +0200, Jiri Slaby wrote:
>>> On 09/20/2018, 03:58 AM, YueHaibing wrote:
>>>> Fixes gcc '-Wunused-but-set-variable' warning:
>>>>
>>>> drivers/tty/serial/imx.c: In function 'imx_uart_probe':
>>>> drivers/tty/serial/imx.c:2198:20: warning:
>>>>  variable 'rtsirq' set but not used [-Wunused-but-set-variable]
>>>>
>>>> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c @@
>>>> -2220,7 +2220,6 @@ static int imx_uart_probe(struct
>> platform_device
>>>> *pdev)
>>>>
>>>>  	rxirq = platform_get_irq(pdev, 0);
>>>>  	txirq = platform_get_irq(pdev, 1);
>>>> -	rtsirq = platform_get_irq(pdev, 2);
>>>
>>> I am not sure this is correct. platform_get_irq has side effects (like
>>> enabling the IRQ). Are you sure this won't change the behaviour (this
>>> is question mostly to IMX fellows)?
>>
>> As far as I can tell there was a request_irq call for rtsirq which was
>> removed by mistake in commit afe9cbb1a6ad ("serial: imx: drop support
>> for IRDA"):
>>
>> -               /* do not use RTS IRQ on IrDA */
>> -               if (!USE_IRDA(sport)) {
>> -                       ret = devm_request_irq(&pdev->dev, rtsirq,
>> -                                              imx_rtsint, 0,
>> -
>> dev_name(&pdev->dev), sport);
>> -                       if (ret)
>> -                               return ret;
>> -               }
>>
>> This should have just removed the IRDA check and request rtsirq
>> unconditionally. Nobody noticed this by testing RTS on imx1, this is an old
>> chip and later variants have a single combined irq.
>>
>> The correct fix for the warning would be to restore that request_irq.
>>
>> --
>> Regards,
>> Leonard
> 
> Yes, your explain is very correct! Thanks for your comment.
> We should restore rtsirq request that for i.MX1.

Ok, I will post a new fix patch as Leonard and suggested.

> 
> Regards,
> Andy Duan
>