linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] iio: adc: fsl-imx25-gcq: fix the right check and simplify code
@ 2021-07-27 12:52 Tang Bin
  2021-07-31 16:45 ` Jonathan Cameron
  0 siblings, 1 reply; 5+ messages in thread
From: Tang Bin @ 2021-07-27 12:52 UTC (permalink / raw)
  To: jic23, knaack.h, lars, shawnguo, s.hauer, festevam
  Cc: linux-iio, linux-arm-kernel, linux-kernel, Tang Bin, Zhang Shengju

For the function of platform_get_irq(), the example in platform.c is
*		int irq = platform_get_irq(pdev, 0);
*		if (irq < 0)
*			return irq;
So the return value of zero is unnecessary to check. And move it
up to a little bit can simplify the code jump.

Co-developed-by: Zhang Shengju <zhangshengju@cmss.chinamobile.com>
Signed-off-by: Zhang Shengju <zhangshengju@cmss.chinamobile.com>
Signed-off-by: Tang Bin <tangbin@cmss.chinamobile.com>
---
 drivers/iio/adc/fsl-imx25-gcq.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/iio/adc/fsl-imx25-gcq.c b/drivers/iio/adc/fsl-imx25-gcq.c
index 8cb51cf7a..d28976f21 100644
--- a/drivers/iio/adc/fsl-imx25-gcq.c
+++ b/drivers/iio/adc/fsl-imx25-gcq.c
@@ -320,6 +320,10 @@ static int mx25_gcq_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
+	priv->irq = platform_get_irq(pdev, 0);
+	if (priv->irq < 0)
+		return priv->irq;
+
 	for (i = 0; i != 4; ++i) {
 		if (!priv->vref[i])
 			continue;
@@ -336,14 +340,6 @@ static int mx25_gcq_probe(struct platform_device *pdev)
 		goto err_vref_disable;
 	}
 
-	priv->irq = platform_get_irq(pdev, 0);
-	if (priv->irq <= 0) {
-		ret = priv->irq;
-		if (!ret)
-			ret = -ENXIO;
-		goto err_clk_unprepare;
-	}
-
 	ret = request_irq(priv->irq, mx25_gcq_irq, 0, pdev->name, priv);
 	if (ret) {
 		dev_err(dev, "Failed requesting IRQ\n");
-- 
2.20.1.windows.1




^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] iio: adc: fsl-imx25-gcq: fix the right check and simplify code
  2021-07-27 12:52 [PATCH] iio: adc: fsl-imx25-gcq: fix the right check and simplify code Tang Bin
@ 2021-07-31 16:45 ` Jonathan Cameron
  2021-08-02  2:31   ` tangbin
  0 siblings, 1 reply; 5+ messages in thread
From: Jonathan Cameron @ 2021-07-31 16:45 UTC (permalink / raw)
  To: Tang Bin
  Cc: knaack.h, lars, shawnguo, s.hauer, festevam, linux-iio,
	linux-arm-kernel, linux-kernel, Zhang Shengju

On Tue, 27 Jul 2021 20:52:09 +0800
Tang Bin <tangbin@cmss.chinamobile.com> wrote:

> For the function of platform_get_irq(), the example in platform.c is
> *		int irq = platform_get_irq(pdev, 0);
> *		if (irq < 0)
> *			return irq;
> So the return value of zero is unnecessary to check. And move it
> up to a little bit can simplify the code jump.
> 
> Co-developed-by: Zhang Shengju <zhangshengju@cmss.chinamobile.com>
> Signed-off-by: Zhang Shengju <zhangshengju@cmss.chinamobile.com>
> Signed-off-by: Tang Bin <tangbin@cmss.chinamobile.com>

Hi,

Logically it is better to keep the irq handling all together, so 
I would prefer we didn't move it.

Also, platform_get_irq() is documented as never returning 0, so the current
code is not incorrect.  As such, this looks like noise unless there is
some plan to make use of the 0 return value?  What benefit do we get from
this change?

Thanks,

Jonathan

> ---
>  drivers/iio/adc/fsl-imx25-gcq.c | 12 ++++--------
>  1 file changed, 4 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/iio/adc/fsl-imx25-gcq.c b/drivers/iio/adc/fsl-imx25-gcq.c
> index 8cb51cf7a..d28976f21 100644
> --- a/drivers/iio/adc/fsl-imx25-gcq.c
> +++ b/drivers/iio/adc/fsl-imx25-gcq.c
> @@ -320,6 +320,10 @@ static int mx25_gcq_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> +	priv->irq = platform_get_irq(pdev, 0);
> +	if (priv->irq < 0)
> +		return priv->irq;
> +
>  	for (i = 0; i != 4; ++i) {
>  		if (!priv->vref[i])
>  			continue;
> @@ -336,14 +340,6 @@ static int mx25_gcq_probe(struct platform_device *pdev)
>  		goto err_vref_disable;
>  	}
>  
> -	priv->irq = platform_get_irq(pdev, 0);
> -	if (priv->irq <= 0) {
> -		ret = priv->irq;
> -		if (!ret)
> -			ret = -ENXIO;
> -		goto err_clk_unprepare;
> -	}
> -
>  	ret = request_irq(priv->irq, mx25_gcq_irq, 0, pdev->name, priv);
>  	if (ret) {
>  		dev_err(dev, "Failed requesting IRQ\n");


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] iio: adc: fsl-imx25-gcq: fix the right check and simplify code
  2021-07-31 16:45 ` Jonathan Cameron
@ 2021-08-02  2:31   ` tangbin
  2021-08-02 10:16     ` Jonathan Cameron
  0 siblings, 1 reply; 5+ messages in thread
From: tangbin @ 2021-08-02  2:31 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: knaack.h, lars, shawnguo, s.hauer, festevam, linux-iio,
	linux-arm-kernel, linux-kernel

Hi Jonathan:

On 2021/8/1 0:45, Jonathan Cameron wrote:
> On Tue, 27 Jul 2021 20:52:09 +0800
> Tang Bin <tangbin@cmss.chinamobile.com> wrote:
>
>> For the function of platform_get_irq(), the example in platform.c is
>> *		int irq = platform_get_irq(pdev, 0);
>> *		if (irq < 0)
>> *			return irq;
>> So the return value of zero is unnecessary to check. And move it
>> up to a little bit can simplify the code jump.
>>
>> Co-developed-by: Zhang Shengju <zhangshengju@cmss.chinamobile.com>
>> Signed-off-by: Zhang Shengju <zhangshengju@cmss.chinamobile.com>
>> Signed-off-by: Tang Bin <tangbin@cmss.chinamobile.com>
> Hi,
>
> Logically it is better to keep the irq handling all together, so
> I would prefer we didn't move it.
Got it in this place.
>
> Also, platform_get_irq() is documented as never returning 0, so the current
> code is not incorrect.  As such, this looks like noise unless there is
> some plan to make use of the 0 return value?  What benefit do we get from
> this change?

Thanks for your reply, I think the benefit of this change maybe just 
simplify the code.

Because the return value is never equal to 0, so the check in here is 
redundant.

We can make the patch like this:

>> ---
>>   drivers/iio/adc/fsl-imx25-gcq.c | 12 ++++--------
>>   1 file changed, 4 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/iio/adc/fsl-imx25-gcq.c b/drivers/iio/adc/fsl-imx25-gcq.c
>> index 8cb51cf7a..d28976f21 100644
>> --- a/drivers/iio/adc/fsl-imx25-gcq.c
>> +++ b/drivers/iio/adc/fsl-imx25-gcq.c
>> @@ -320,6 +320,10 @@ static int mx25_gcq_probe(struct platform_device *pdev)
>>   	if (ret)
>>   		return ret;
>>   
>> +	priv->irq = platform_get_irq(pdev, 0);
>> +	if (priv->irq < 0)
>> +		return priv->irq;
>> +
>>   	for (i = 0; i != 4; ++i) {
>>   		if (!priv->vref[i])
>>   			continue;
>> @@ -336,14 +340,6 @@ static int mx25_gcq_probe(struct platform_device *pdev)
>>   		goto err_vref_disable;
>>   	}
>>   
>> -	priv->irq = platform_get_irq(pdev, 0);
>> -	if (priv->irq <= 0) {
>> -		ret = priv->irq;
>> -		if (!ret)
>> -			ret = -ENXIO;
>> -		goto err_clk_unprepare;
>> -	}
>> -

	priv->irq = platform_get_irq(pdev, 0);
	if (priv->irq < 0) {
		ret = priv->irq;
		goto err_clk_unprepare;
	}

     If you think this is ok, I will send V2 for you. If you think these 
change is meaningless,

just dropped this.

Thanks

Tang Bin





^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] iio: adc: fsl-imx25-gcq: fix the right check and simplify code
  2021-08-02  2:31   ` tangbin
@ 2021-08-02 10:16     ` Jonathan Cameron
  2021-08-02 11:50       ` [PATCH] iio: adc: fsl-imx25-gcq: fix the right check andsimplify code tangbin
  0 siblings, 1 reply; 5+ messages in thread
From: Jonathan Cameron @ 2021-08-02 10:16 UTC (permalink / raw)
  To: tangbin
  Cc: Jonathan Cameron, knaack.h, lars, shawnguo, s.hauer, festevam,
	linux-iio, linux-arm-kernel, linux-kernel

On Mon, 2 Aug 2021 10:31:58 +0800
tangbin <tangbin@cmss.chinamobile.com> wrote:

> Hi Jonathan:
> 
> On 2021/8/1 0:45, Jonathan Cameron wrote:
> > On Tue, 27 Jul 2021 20:52:09 +0800
> > Tang Bin <tangbin@cmss.chinamobile.com> wrote:
> >  
> >> For the function of platform_get_irq(), the example in platform.c is
> >> *		int irq = platform_get_irq(pdev, 0);
> >> *		if (irq < 0)
> >> *			return irq;
> >> So the return value of zero is unnecessary to check. And move it
> >> up to a little bit can simplify the code jump.
> >>
> >> Co-developed-by: Zhang Shengju <zhangshengju@cmss.chinamobile.com>
> >> Signed-off-by: Zhang Shengju <zhangshengju@cmss.chinamobile.com>
> >> Signed-off-by: Tang Bin <tangbin@cmss.chinamobile.com>  
> > Hi,
> >
> > Logically it is better to keep the irq handling all together, so
> > I would prefer we didn't move it.  
> Got it in this place.
> >
> > Also, platform_get_irq() is documented as never returning 0, so the current
> > code is not incorrect.  As such, this looks like noise unless there is
> > some plan to make use of the 0 return value?  What benefit do we get from
> > this change?  
> 
> Thanks for your reply, I think the benefit of this change maybe just 
> simplify the code.
> 
> Because the return value is never equal to 0, so the check in here is 
> redundant.
> 
> We can make the patch like this:
> 
> >> ---
> >>   drivers/iio/adc/fsl-imx25-gcq.c | 12 ++++--------
> >>   1 file changed, 4 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/drivers/iio/adc/fsl-imx25-gcq.c b/drivers/iio/adc/fsl-imx25-gcq.c
> >> index 8cb51cf7a..d28976f21 100644
> >> --- a/drivers/iio/adc/fsl-imx25-gcq.c
> >> +++ b/drivers/iio/adc/fsl-imx25-gcq.c
> >> @@ -320,6 +320,10 @@ static int mx25_gcq_probe(struct platform_device *pdev)
> >>   	if (ret)
> >>   		return ret;
> >>   
> >> +	priv->irq = platform_get_irq(pdev, 0);
> >> +	if (priv->irq < 0)
> >> +		return priv->irq;
> >> +
> >>   	for (i = 0; i != 4; ++i) {
> >>   		if (!priv->vref[i])
> >>   			continue;
> >> @@ -336,14 +340,6 @@ static int mx25_gcq_probe(struct platform_device *pdev)
> >>   		goto err_vref_disable;
> >>   	}
> >>   
> >> -	priv->irq = platform_get_irq(pdev, 0);
> >> -	if (priv->irq <= 0) {
> >> -		ret = priv->irq;
> >> -		if (!ret)
> >> -			ret = -ENXIO;
> >> -		goto err_clk_unprepare;
> >> -	}
> >> -  
> 
> 	priv->irq = platform_get_irq(pdev, 0);
> 	if (priv->irq < 0) {
> 		ret = priv->irq;
> 		goto err_clk_unprepare;
> 	}
> 
>      If you think this is ok, I will send V2 for you. If you think these 
> change is meaningless,

OK, it's a minor tidy up, so lets go with that, or perhaps this is even tidier?
Assuming types of ret and irq are appropriate (I've not checked!)


	ret = platform_get_irq(pdev, 0);
	if (ret)
		goto err_clk_unprepare;

	priv->irq = ret;


> 
> just dropped this.
> 
> Thanks
> 
> Tang Bin
> 
> 
> 
> 


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] iio: adc: fsl-imx25-gcq: fix the right check andsimplify code
  2021-08-02 10:16     ` Jonathan Cameron
@ 2021-08-02 11:50       ` tangbin
  0 siblings, 0 replies; 5+ messages in thread
From: tangbin @ 2021-08-02 11:50 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: Jonathan Cameron, knaack.h, lars, shawnguo, s.hauer, festevam,
	linux-iio, linux-arm-kernel, linux-kernel

Hi Jonathan:

On 2021/8/2 18:16, Jonathan Cameron wrote:
> On Mon, 2 Aug 2021 10:31:58 +0800
> tangbin <tangbin@cmss.chinamobile.com> wrote:
>
>> Hi Jonathan:
>>
>> On 2021/8/1 0:45, Jonathan Cameron wrote:
>>> On Tue, 27 Jul 2021 20:52:09 +0800
>>> Tang Bin <tangbin@cmss.chinamobile.com> wrote:
>>>   
>>>> For the function of platform_get_irq(), the example in platform.c is
>>>> *		int irq = platform_get_irq(pdev, 0);
>>>> *		if (irq < 0)
>>>> *			return irq;
>>>> So the return value of zero is unnecessary to check. And move it
>>>> up to a little bit can simplify the code jump.
>>>>
>>>> Co-developed-by: Zhang Shengju <zhangshengju@cmss.chinamobile.com>
>>>> Signed-off-by: Zhang Shengju <zhangshengju@cmss.chinamobile.com>
>>>> Signed-off-by: Tang Bin <tangbin@cmss.chinamobile.com>
>>> Hi,
>>>
>>> Logically it is better to keep the irq handling all together, so
>>> I would prefer we didn't move it.
>> Got it in this place.
>>> Also, platform_get_irq() is documented as never returning 0, so the current
>>> code is not incorrect.  As such, this looks like noise unless there is
>>> some plan to make use of the 0 return value?  What benefit do we get from
>>> this change?
>> Thanks for your reply, I think the benefit of this change maybe just
>> simplify the code.
>>
>> Because the return value is never equal to 0, so the check in here is
>> redundant.
>>
>> We can make the patch like this:
>>
>>>> ---
>>>>    drivers/iio/adc/fsl-imx25-gcq.c | 12 ++++--------
>>>>    1 file changed, 4 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/drivers/iio/adc/fsl-imx25-gcq.c b/drivers/iio/adc/fsl-imx25-gcq.c
>>>> index 8cb51cf7a..d28976f21 100644
>>>> --- a/drivers/iio/adc/fsl-imx25-gcq.c
>>>> +++ b/drivers/iio/adc/fsl-imx25-gcq.c
>>>> @@ -320,6 +320,10 @@ static int mx25_gcq_probe(struct platform_device *pdev)
>>>>    	if (ret)
>>>>    		return ret;
>>>>    
>>>> +	priv->irq = platform_get_irq(pdev, 0);
>>>> +	if (priv->irq < 0)
>>>> +		return priv->irq;
>>>> +
>>>>    	for (i = 0; i != 4; ++i) {
>>>>    		if (!priv->vref[i])
>>>>    			continue;
>>>> @@ -336,14 +340,6 @@ static int mx25_gcq_probe(struct platform_device *pdev)
>>>>    		goto err_vref_disable;
>>>>    	}
>>>>    
>>>> -	priv->irq = platform_get_irq(pdev, 0);
>>>> -	if (priv->irq <= 0) {
>>>> -		ret = priv->irq;
>>>> -		if (!ret)
>>>> -			ret = -ENXIO;
>>>> -		goto err_clk_unprepare;
>>>> -	}
>>>> -
>> 	priv->irq = platform_get_irq(pdev, 0);
>> 	if (priv->irq < 0) {
>> 		ret = priv->irq;
>> 		goto err_clk_unprepare;
>> 	}
>>
>>       If you think this is ok, I will send V2 for you. If you think these
>> change is meaningless,
> OK, it's a minor tidy up, so lets go with that, or perhaps this is even tidier?
> Assuming types of ret and irq are appropriate (I've not checked!)
>
>
> 	ret = platform_get_irq(pdev, 0);
> 	if (ret)
> 		goto err_clk_unprepare;
>
> 	priv->irq = ret;
>
Thanks for your reply, ret or irq or priv->irq are all appropriate, and 
the changes of mine maybe traditional.

I will send v2 for you like your changes.

Thank you very much.

Tang Bin


>> just dropped this.
>>
>> Thanks
>>
>> Tang Bin
>>
>>
>>
>>



^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2021-08-02 11:50 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-27 12:52 [PATCH] iio: adc: fsl-imx25-gcq: fix the right check and simplify code Tang Bin
2021-07-31 16:45 ` Jonathan Cameron
2021-08-02  2:31   ` tangbin
2021-08-02 10:16     ` Jonathan Cameron
2021-08-02 11:50       ` [PATCH] iio: adc: fsl-imx25-gcq: fix the right check andsimplify code tangbin

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).