Re: [PATCH] mfd: asic3: Delete redundant variable definition

Re: [PATCH] mfd: asic3: Delete redundant variable definition

[Markus Elfring]

> In this function, 'ret' is always assigned, even if 'pdata->leds'
> don't carry out,

I notice possibilities again to improve such a commit message.


> it has already been assigned a value in the above code, including '0',

The variable assignment will eventually be performed only in if branches.


> so it's redundant.

I suggest to reconsider this interpretation of the source code here.
Would you like to move the mentioned statement into an else branch
at the end?

Regards,
Markus

Re: [PATCH] mfd: asic3: Delete redundant variable definition

[Lee Jones]

On Fri, 03 Apr 2020, Markus Elfring wrote:

> > In this function, 'ret' is always assigned, even if 'pdata->leds'
> > don't carry out,
> 
> I notice possibilities again to improve such a commit message.
> 
> 
> > it has already been assigned a value in the above code, including '0',
> 
> The variable assignment will eventually be performed only in if branches.
> 
> 
> > so it's redundant.
> 
> I suggest to reconsider this interpretation of the source code here.
> Would you like to move the mentioned statement into an else branch
> at the end?

Why wasn't this sent 'in reply to'?  Something wrong with your mailer?

Now this messages is orphaned.  Floating about in my inbox on its own.

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: mfd: asic3: Delete redundant variable definition

[Markus Elfring]

> Why wasn't this sent 'in reply to'?  Something wrong with your mailer?

It seems that my mail program version contains still open issues for
the preservation of this information after a click on a web link.
mailto:tangbin%40cmss.chinamobile.com?In-Reply-To=%3C20200403042020.17452-1-tangbin@cmss.chinamobile.com%3E&Cc=lee.jones%40linaro.org%2Clinux-kernel%40vger.kernel.org&Subject=Re%3A%20%5BPATCH%5D%20mfd%3Aasic3%3ADelete%20redundant%20variable%20definition


> Now this messages is orphaned.  Floating about in my inbox on its own.

This effect can be unfortunate.


Regards,
Markus

[PATCH] mfd:asic3:Delete redundant variable definition

[Tang Bin]

In this function, 'ret' is always assigned, even if 'pdata->leds'
don't carry out,it has already been assigned a value in the above
code, including '0',so it's redundant.

Signed-off-by: Tang Bin <tangbin@cmss.chinamobile.com>
---
 drivers/mfd/asic3.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/mfd/asic3.c b/drivers/mfd/asic3.c
index a6bd2134c..cee7454b3 100644
--- a/drivers/mfd/asic3.c
+++ b/drivers/mfd/asic3.c
@@ -924,7 +924,6 @@ static int __init asic3_mfd_probe(struct platform_device *pdev,
 			goto out;
 	}
 
-	ret = 0;
 	if (pdata->leds) {
 		int i;
 
-- 
2.20.1.windows.1

Re: [PATCH] mfd:asic3:Delete redundant variable definition

[Lee Jones]

On Fri, 03 Apr 2020, Tang Bin wrote:

> In this function, 'ret' is always assigned, even if 'pdata->leds'
> don't carry out,it has already been assigned a value in the above
> code, including '0',so it's redundant.

Which line initialises/assigns 'ret' before this one?

> Signed-off-by: Tang Bin <tangbin@cmss.chinamobile.com>
> ---
>  drivers/mfd/asic3.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/mfd/asic3.c b/drivers/mfd/asic3.c
> index a6bd2134c..cee7454b3 100644
> --- a/drivers/mfd/asic3.c
> +++ b/drivers/mfd/asic3.c
> @@ -924,7 +924,6 @@ static int __init asic3_mfd_probe(struct platform_device *pdev,
>  			goto out;
>  	}
>  
> -	ret = 0;
>  	if (pdata->leds) {
>  		int i;
>  

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH] mfd:asic3:Delete redundant variable definition

[Tang Bin]

Hi, Lee:

On 2020/4/16 15:08, Lee Jones wrote:
> On Fri, 03 Apr 2020, Tang Bin wrote:
>
>> In this function, 'ret' is always assigned, even if 'pdata->leds'
>> don't carry out,it has already been assigned a value in the above
>> code, including '0',so it's redundant.
> Which line initialises/assigns 'ret' before this one?

     I think it maybe my fault before, because I treat get resource and 
irq succeed. But now I have two questions  to ask you:

     Q1: About asic3_mfd_probe()?

           In the function asic3_mfd_probe(), if get resource or irq 
failed, the value returned just detected and dev_dbg() error message, 
but there were no error return. What I think the modify should be as 
follows:

     mem_sdio = platform_get_resource(pdev, IORESOURCE_MEM, 1);
     if (!mem_sdio) {
         dev_dbg(asic->dev, "no SDIO MEM resource\n");
         ret = -EINVAL;
         goto out;
     }

     irq = platform_get_irq(pdev, 1);
     if (irq < 0) {
         dev_dbg(asic->dev, "no SDIO IRQ resource\n");
         ret = irq;
         goto out;
     }

     If the function do like this, the 'ret = 0' in line 993 maybe 
redundant.


    Q2: About asic3_probe()?

           In the line 995, if the function asic3_irq_probe() failed, it 
will print error message by the internally called function 
platform_get_irq(), so the dev_err() in the line 997 is redundant, 
should be delete.


     I'll wait actively, and submit the corresponding patch according to 
your reply.


Thanks,

Tang Bin

Re: [PATCH] mfd:asic3:Delete redundant variable definition

[Lee Jones]

On Thu, 16 Apr 2020, Tang Bin wrote:

> Hi, Lee:
> 
> On 2020/4/16 15:08, Lee Jones wrote:
> > On Fri, 03 Apr 2020, Tang Bin wrote:
> > 
> > > In this function, 'ret' is always assigned, even if 'pdata->leds'
> > > don't carry out,it has already been assigned a value in the above
> > > code, including '0',so it's redundant.
> > Which line initialises/assigns 'ret' before this one?
> 
>     I think it maybe my fault before, because I treat get resource and irq
> succeed. But now I have two questions  to ask you:
> 
>     Q1: About asic3_mfd_probe()?
> 
>           In the function asic3_mfd_probe(), if get resource or irq failed,
> the value returned just detected and dev_dbg() error message, but there were
> no error return. What I think the modify should be as follows:
> 
>     mem_sdio = platform_get_resource(pdev, IORESOURCE_MEM, 1);
>     if (!mem_sdio) {
>         dev_dbg(asic->dev, "no SDIO MEM resource\n");
>         ret = -EINVAL;
>         goto out;
>     }
> 
>     irq = platform_get_irq(pdev, 1);
>     if (irq < 0) {
>         dev_dbg(asic->dev, "no SDIO IRQ resource\n");
>         ret = irq;
>         goto out;
>     }
> 
>     If the function do like this, the 'ret = 0' in line 993 maybe redundant.
> 
> 
>    Q2: About asic3_probe()?
> 
>           In the line 995, if the function asic3_irq_probe() failed, it will
> print error message by the internally called function platform_get_irq(), so
> the dev_err() in the line 997 is redundant, should be delete.
> 
> 
>     I'll wait actively, and submit the corresponding patch according to your
> reply.

Just submit the patch and we can discuss it there.

These kind of messages always require a lot of extra faff to process.

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog