Messages in this thread | | | Subject | Re: [PATCH] iio: adc: fsl-imx25-gcq: fix the right check andsimplify code | From | tangbin <> | Date | Mon, 2 Aug 2021 19:50:26 +0800 |
| |
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 >> >> >> >>
| |