Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1234981 > unrolled thread

[PATCH 8/8] mfd: lm3533: Simplify function return logic

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 8/8] mfd: lm3533: Simplify function return logic Javier Martinez Canillas <javier@osg.samsung.com> - 2015-09-29 13:30 +0200
    Re: [PATCH 8/8] mfd: lm3533: Simplify function return logic Johan Hovold <johan@kernel.org> - 2015-09-30 23:10 +0200
      Re: [PATCH 8/8] mfd: lm3533: Simplify function return logic Javier Martinez Canillas <javier@osg.samsung.com> - 2015-09-30 23:50 +0200
        Re: [PATCH 8/8] mfd: lm3533: Simplify function return logic Johan Hovold <johan@kernel.org> - 2015-10-01 00:00 +0200
      Re: [PATCH 8/8] mfd: lm3533: Simplify function return logic Lee Jones <lee.jones@linaro.org> - 2015-10-01 09:20 +0200
        Re: [PATCH 8/8] mfd: lm3533: Simplify function return logic Johan Hovold <johan@kernel.org> - 2015-10-01 20:00 +0200

#1234981 — [PATCH 8/8] mfd: lm3533: Simplify function return logic

The invoked functions already return zero on success or a negative
errno code so there is no need to open code the logic in the caller.

Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>

---

 drivers/mfd/lm3533-core.c | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

diff --git a/drivers/mfd/lm3533-core.c b/drivers/mfd/lm3533-core.c
index 643f3750e830..193ecee1fa7e 100644
--- a/drivers/mfd/lm3533-core.c
+++ b/drivers/mfd/lm3533-core.c
@@ -472,11 +472,7 @@ static int lm3533_device_setup(struct lm3533 *lm3533,
 	if (ret)
 		return ret;
 
-	ret = lm3533_set_boost_ovp(lm3533, pdata->boost_ovp);
-	if (ret)
-		return ret;
-
-	return 0;
+	return lm3533_set_boost_ovp(lm3533, pdata->boost_ovp);
 }
 
 static int lm3533_device_init(struct lm3533 *lm3533)
@@ -613,11 +609,7 @@ static int lm3533_i2c_probe(struct i2c_client *i2c,
 	lm3533->dev = &i2c->dev;
 	lm3533->irq = i2c->irq;
 
-	ret = lm3533_device_init(lm3533);
-	if (ret)
-		return ret;
-
-	return 0;
+	return lm3533_device_init(lm3533);
 }
 
 static int lm3533_i2c_remove(struct i2c_client *i2c)
-- 
2.4.3

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1236762

On Tue, Sep 29, 2015 at 01:26:08PM +0200, Javier Martinez Canillas wrote:
> The invoked functions already return zero on success or a negative
> errno code so there is no need to open code the logic in the caller.
> 
> Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>

I do not consider this an improvement in any way and suggest this patch
is dropped.

> ---
> 
>  drivers/mfd/lm3533-core.c | 12 ++----------
>  1 file changed, 2 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/mfd/lm3533-core.c b/drivers/mfd/lm3533-core.c
> index 643f3750e830..193ecee1fa7e 100644
> --- a/drivers/mfd/lm3533-core.c
> +++ b/drivers/mfd/lm3533-core.c
> @@ -472,11 +472,7 @@ static int lm3533_device_setup(struct lm3533 *lm3533,
>  	if (ret)
>  		return ret;
>  
> -	ret = lm3533_set_boost_ovp(lm3533, pdata->boost_ovp);
> -	if (ret)
> -		return ret;
> -
> -	return 0;
> +	return lm3533_set_boost_ovp(lm3533, pdata->boost_ovp);

You're saving a few lines of code but instead introduce asymmetries and
obscure the fact that the function returns zero on success.

Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1236799

Hello Johan,

On 09/30/2015 11:04 PM, Johan Hovold wrote:
> On Tue, Sep 29, 2015 at 01:26:08PM +0200, Javier Martinez Canillas wrote:
>> The invoked functions already return zero on success or a negative
>> errno code so there is no need to open code the logic in the caller.
>>
>> Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
> 
> I do not consider this an improvement in any way and suggest this patch
> is dropped.
>

Since I posted the patch I obviously disagree but I don't really mind
if the patch is dropped.

>> ---
>>
>>  drivers/mfd/lm3533-core.c | 12 ++----------
>>  1 file changed, 2 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/mfd/lm3533-core.c b/drivers/mfd/lm3533-core.c
>> index 643f3750e830..193ecee1fa7e 100644
>> --- a/drivers/mfd/lm3533-core.c
>> +++ b/drivers/mfd/lm3533-core.c
>> @@ -472,11 +472,7 @@ static int lm3533_device_setup(struct lm3533 *lm3533,
>>  	if (ret)
>>  		return ret;
>>  
>> -	ret = lm3533_set_boost_ovp(lm3533, pdata->boost_ovp);
>> -	if (ret)
>> -		return ret;
>> -
>> -	return 0;
>> +	return lm3533_set_boost_ovp(lm3533, pdata->boost_ovp);
> 
> You're saving a few lines of code but instead introduce asymmetries and
> obscure the fact that the function returns zero on success.
>

I don't think the change makes the code more obscure tbh, the return foo()
construct is very common in the kernel and most functions return 0 on
success and a negative errno code on failure.

Also, we have a coccinelle semantic patch to find this pattern [0] so if
you think that is not worth it, please add a comment to the code. Otherwise
another developer could attempt to post the same patch since make coccicheck
will always complain about this file.

> Johan
> 

[0]: http://lxr.free-electrons.com/source/scripts/coccinelle/misc/simple_return.cocci

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1236812

On Wed, Sep 30, 2015 at 11:41:26PM +0200, Javier Martinez Canillas wrote:
> On 09/30/2015 11:04 PM, Johan Hovold wrote:
> > On Tue, Sep 29, 2015 at 01:26:08PM +0200, Javier Martinez Canillas wrote:

> >> diff --git a/drivers/mfd/lm3533-core.c b/drivers/mfd/lm3533-core.c
> >> index 643f3750e830..193ecee1fa7e 100644
> >> --- a/drivers/mfd/lm3533-core.c
> >> +++ b/drivers/mfd/lm3533-core.c
> >> @@ -472,11 +472,7 @@ static int lm3533_device_setup(struct lm3533 *lm3533,
> >>  	if (ret)
> >>  		return ret;
> >>  
> >> -	ret = lm3533_set_boost_ovp(lm3533, pdata->boost_ovp);
> >> -	if (ret)
> >> -		return ret;
> >> -
> >> -	return 0;
> >> +	return lm3533_set_boost_ovp(lm3533, pdata->boost_ovp);
> > 
> > You're saving a few lines of code but instead introduce asymmetries and
> > obscure the fact that the function returns zero on success.
> 
> I don't think the change makes the code more obscure tbh, the return foo()
> construct is very common in the kernel and most functions return 0 on
> success and a negative errno code on failure.

But it was perfectly obvious from just looking at the function before
your change.
 
> Also, we have a coccinelle semantic patch to find this pattern [0] so if
> you think that is not worth it, please add a comment to the code. Otherwise
> another developer could attempt to post the same patch since make coccicheck
> will always complain about this file.

Yes, I've NAKed similar so called clean up patches based on that pattern
for USB-serial and would be very glad to see that semantic patch removed.

Coccinelle can be very useful to detect and fix real bugs, but this
return-value exercise is just pointless at best.

Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1237076

On Wed, 30 Sep 2015, Johan Hovold wrote:

> On Tue, Sep 29, 2015 at 01:26:08PM +0200, Javier Martinez Canillas wrote:
> > The invoked functions already return zero on success or a negative
> > errno code so there is no need to open code the logic in the caller.
> > 
> > Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
> 
> I do not consider this an improvement in any way and suggest this patch
> is dropped.

Sorry Johan, but I disagree.

> > ---
> >  drivers/mfd/lm3533-core.c | 12 ++----------
> >  1 file changed, 2 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/mfd/lm3533-core.c b/drivers/mfd/lm3533-core.c
> > index 643f3750e830..193ecee1fa7e 100644
> > --- a/drivers/mfd/lm3533-core.c
> > +++ b/drivers/mfd/lm3533-core.c
> > @@ -472,11 +472,7 @@ static int lm3533_device_setup(struct lm3533 *lm3533,
> >  	if (ret)
> >  		return ret;
> >  
> > -	ret = lm3533_set_boost_ovp(lm3533, pdata->boost_ovp);
> > -	if (ret)
> > -		return ret;
> > -
> > -	return 0;
> > +	return lm3533_set_boost_ovp(lm3533, pdata->boost_ovp);
> 
> You're saving a few lines of code but instead introduce asymmetries and
> obscure the fact that the function returns zero on success.

There is no obfuscation here.  Functions normally return zero on
success and !zero on failure, it's what's expected.

I'm going to apply the patch.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1237605

On Thu, Oct 01, 2015 at 08:17:00AM +0100, Lee Jones wrote:
> On Wed, 30 Sep 2015, Johan Hovold wrote:
> 
> > On Tue, Sep 29, 2015 at 01:26:08PM +0200, Javier Martinez Canillas wrote:
> > > The invoked functions already return zero on success or a negative
> > > errno code so there is no need to open code the logic in the caller.
> > > 
> > > Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
> > 
> > I do not consider this an improvement in any way and suggest this patch
> > is dropped.
> 
> Sorry Johan, but I disagree.

It's your call now, but I really think this is something that should be
up to the author of the code (which in this case happens to be me).

Neither style is incorrect, but there are reasons for preferring the
current style in this case.

> > > ---
> > >  drivers/mfd/lm3533-core.c | 12 ++----------
> > >  1 file changed, 2 insertions(+), 10 deletions(-)
> > > 
> > > diff --git a/drivers/mfd/lm3533-core.c b/drivers/mfd/lm3533-core.c
> > > index 643f3750e830..193ecee1fa7e 100644
> > > --- a/drivers/mfd/lm3533-core.c
> > > +++ b/drivers/mfd/lm3533-core.c
> > > @@ -472,11 +472,7 @@ static int lm3533_device_setup(struct lm3533 *lm3533,
> > >  	if (ret)
> > >  		return ret;
> > >  
> > > -	ret = lm3533_set_boost_ovp(lm3533, pdata->boost_ovp);
> > > -	if (ret)
> > > -		return ret;
> > > -
> > > -	return 0;
> > > +	return lm3533_set_boost_ovp(lm3533, pdata->boost_ovp);
> > 
> > You're saving a few lines of code but instead introduce asymmetries and
> > obscure the fact that the function returns zero on success.
> 
> There is no obfuscation here.  Functions normally return zero on
> success and !zero on failure, it's what's expected.

We have functions returning boolean true or a positive integer on
success.

In summary (rehashing what I wrote in a different thread) I do think
that:

{
        int ret;

        ret = init_a(...);
        if (ret)
                return ret;

        ret = init_b(...);
        if (ret)
                return ret;

        return 0;
}

is (at least to me) preferred over:

{
        int ret;

        ret = init_a(...);
        if (ret)
                return ret;

        return init_b(...);
}

for symmetry and readability reasons (e.g. I don't have to look at
init_b to figure out what the functions returns). And with a long
parameter list to init_b with line breaks, this would look even worse.

But either way, it should be up to the author of the code to decide what
style to use.

> I'm going to apply the patch.

Fair enough. I've asked for the warnings to be removed from coccinelle
so hopefully we can get on to fix real issues. ;)

Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web