Re: [PATCH 2/8] mfd: arizona: Remove unneded ret variable

Re: [PATCH 2/8] mfd: arizona: Remove unneded ret variable

[Charles Keepax]

On Tue, Sep 29, 2015 at 01:26:02PM +0200, Javier Martinez Canillas wrote:
> The ret variable is not needed since is not used in the
> function. Remove the variable and just return 0 instead.
> 
> Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
> ---

Acked-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com> 

Thanks,
Charles

[PATCH 0/8] mfd: Simplify return logic

[Javier Martinez Canillas]

Hello Lee,

This series contains trivial patches that simplify the return logic in
a number of functions in mfd drivers. All these were found by running
make coccichek M=drivers/mfd.

The patches don't contain functional changes but makes the functions
simpler and removes unnecessary lines of code.

Best regards,
Javier


Javier Martinez Canillas (8):
  mfd: htc-i2cpld: Remove unneded ret variable
  mfd: arizona: Remove unneded ret variable
  mfd: pcf50633: Remove unneded ret variable
  mfd: kempld-core: Simplify function return logic
  mfd: rtsx: Simplify function return logic
  mfd: da903x: Simplify function return logic
  mfd: da9052: Simplify function return logic
  mfd: lm3533: Simplify function return logic

 drivers/mfd/arizona-core.c |  4 +---
 drivers/mfd/da903x.c       |  6 +-----
 drivers/mfd/da9052-i2c.c   |  6 +-----
 drivers/mfd/da9052-spi.c   |  6 +-----
 drivers/mfd/htc-i2cpld.c   |  3 +--
 drivers/mfd/kempld-core.c  | 14 ++------------
 drivers/mfd/lm3533-core.c  | 12 ++----------
 drivers/mfd/pcf50633-irq.c |  4 ++--
 drivers/mfd/rts5209.c      |  6 +-----
 drivers/mfd/rts5227.c      |  6 +-----
 drivers/mfd/rts5229.c      |  6 +-----
 drivers/mfd/rts5249.c      |  6 +-----
 drivers/mfd/rtsx_pcr.c     |  6 +-----
 13 files changed, 16 insertions(+), 69 deletions(-)

-- 
2.4.3

[PATCH 1/8] mfd: htc-i2cpld: Remove unneded ret variable

[Javier Martinez Canillas]

The ret variable is not needed since is not used in the
function. Remove the variable and just return 0 instead.

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

 drivers/mfd/htc-i2cpld.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/mfd/htc-i2cpld.c b/drivers/mfd/htc-i2cpld.c
index 1bd5b042c8b3..0c6ff727b2ec 100644
--- a/drivers/mfd/htc-i2cpld.c
+++ b/drivers/mfd/htc-i2cpld.c
@@ -318,7 +318,6 @@ static int htcpld_setup_chip_irq(
 	struct htcpld_data *htcpld;
 	struct htcpld_chip *chip;
 	unsigned int irq, irq_end;
-	int ret = 0;
 
 	/* Get the platform and driver data */
 	htcpld = platform_get_drvdata(pdev);
@@ -333,7 +332,7 @@ static int htcpld_setup_chip_irq(
 		irq_clear_status_flags(irq, IRQ_NOREQUEST | IRQ_NOPROBE);
 	}
 
-	return ret;
+	return 0;
 }
 
 static int htcpld_register_chip_i2c(
-- 
2.4.3

[PATCH 2/8] mfd: arizona: Remove unneded ret variable

[Javier Martinez Canillas]

The ret variable is not needed since is not used in the
function. Remove the variable and just return 0 instead.

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

 drivers/mfd/arizona-core.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/mfd/arizona-core.c b/drivers/mfd/arizona-core.c
index 44cfdbb295db..acf354875338 100644
--- a/drivers/mfd/arizona-core.c
+++ b/drivers/mfd/arizona-core.c
@@ -69,8 +69,6 @@ EXPORT_SYMBOL_GPL(arizona_clk32k_enable);
 
 int arizona_clk32k_disable(struct arizona *arizona)
 {
-	int ret = 0;
-
 	mutex_lock(&arizona->clk_lock);
 
 	BUG_ON(arizona->clk32k_ref <= 0);
@@ -90,7 +88,7 @@ int arizona_clk32k_disable(struct arizona *arizona)
 
 	mutex_unlock(&arizona->clk_lock);
 
-	return ret;
+	return 0;
 }
 EXPORT_SYMBOL_GPL(arizona_clk32k_disable);
 
-- 
2.4.3

[PATCH 3/8] mfd: pcf50633: Remove unneded ret variable

[Javier Martinez Canillas]

The ret variable is not needed since is not used in the
function. Remove the variable and just return 0 instead.

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

 drivers/mfd/pcf50633-irq.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/mfd/pcf50633-irq.c b/drivers/mfd/pcf50633-irq.c
index 498286cbb530..71d43edf110c 100644
--- a/drivers/mfd/pcf50633-irq.c
+++ b/drivers/mfd/pcf50633-irq.c
@@ -55,7 +55,7 @@ EXPORT_SYMBOL_GPL(pcf50633_free_irq);
 static int __pcf50633_irq_mask_set(struct pcf50633 *pcf, int irq, u8 mask)
 {
 	u8 reg, bit;
-	int ret = 0, idx;
+	int idx;
 
 	idx = irq >> 3;
 	reg = PCF50633_REG_INT1M + idx;
@@ -72,7 +72,7 @@ static int __pcf50633_irq_mask_set(struct pcf50633 *pcf, int irq, u8 mask)
 
 	mutex_unlock(&pcf->lock);
 
-	return ret;
+	return 0;
 }
 
 int pcf50633_irq_mask(struct pcf50633 *pcf, int irq)
-- 
2.4.3

[PATCH 4/8] mfd: kempld-core: Simplify function return logic

[Javier Martinez Canillas]

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/kempld-core.c | 14 ++------------
 1 file changed, 2 insertions(+), 12 deletions(-)

diff --git a/drivers/mfd/kempld-core.c b/drivers/mfd/kempld-core.c
index 463f4eae20c1..05b924542ee2 100644
--- a/drivers/mfd/kempld-core.c
+++ b/drivers/mfd/kempld-core.c
@@ -448,7 +448,6 @@ static int kempld_probe(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	struct kempld_device_data *pld;
 	struct resource *ioport;
-	int ret;
 
 	pld = devm_kzalloc(dev, sizeof(*pld), GFP_KERNEL);
 	if (!pld)
@@ -471,11 +470,7 @@ static int kempld_probe(struct platform_device *pdev)
 	mutex_init(&pld->lock);
 	platform_set_drvdata(pdev, pld);
 
-	ret = kempld_detect_device(pld);
-	if (ret)
-		return ret;
-
-	return 0;
+	return kempld_detect_device(pld);
 }
 
 static int kempld_remove(struct platform_device *pdev)
@@ -756,7 +751,6 @@ MODULE_DEVICE_TABLE(dmi, kempld_dmi_table);
 static int __init kempld_init(void)
 {
 	const struct dmi_system_id *id;
-	int ret;
 
 	if (force_device_id[0]) {
 		for (id = kempld_dmi_table;
@@ -771,11 +765,7 @@ static int __init kempld_init(void)
 			return -ENODEV;
 	}
 
-	ret = platform_driver_register(&kempld_driver);
-	if (ret)
-		return ret;
-
-	return 0;
+	return platform_driver_register(&kempld_driver);
 }
 
 static void __exit kempld_exit(void)
-- 
2.4.3

[PATCH 5/8] mfd: rtsx: Simplify function return logic

[Javier Martinez Canillas]

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/rts5209.c  | 6 +-----
 drivers/mfd/rts5227.c  | 6 +-----
 drivers/mfd/rts5229.c  | 6 +-----
 drivers/mfd/rts5249.c  | 6 +-----
 drivers/mfd/rtsx_pcr.c | 6 +-----
 5 files changed, 5 insertions(+), 25 deletions(-)

diff --git a/drivers/mfd/rts5209.c b/drivers/mfd/rts5209.c
index 373e253c33df..b95beecf767f 100644
--- a/drivers/mfd/rts5209.c
+++ b/drivers/mfd/rts5209.c
@@ -138,11 +138,7 @@ static int rts5209_card_power_on(struct rtsx_pcr *pcr, int card)
 	rtsx_pci_add_cmd(pcr, WRITE_REG_CMD, CARD_PWR_CTL, pwr_mask, pwr_on);
 	rtsx_pci_add_cmd(pcr, WRITE_REG_CMD, PWR_GATE_CTRL,
 			LDO3318_PWR_MASK, 0x00);
-	err = rtsx_pci_send_cmd(pcr, 100);
-	if (err < 0)
-		return err;
-
-	return 0;
+	return rtsx_pci_send_cmd(pcr, 100);
 }
 
 static int rts5209_card_power_off(struct rtsx_pcr *pcr, int card)
diff --git a/drivers/mfd/rts5227.c b/drivers/mfd/rts5227.c
index ce012d78ce2a..c5a65298c781 100644
--- a/drivers/mfd/rts5227.c
+++ b/drivers/mfd/rts5227.c
@@ -179,11 +179,7 @@ static int rts5227_card_power_on(struct rtsx_pcr *pcr, int card)
 			SD_POWER_MASK, SD_POWER_ON);
 	rtsx_pci_add_cmd(pcr, WRITE_REG_CMD, PWR_GATE_CTRL,
 			LDO3318_PWR_MASK, 0x06);
-	err = rtsx_pci_send_cmd(pcr, 100);
-	if (err < 0)
-		return err;
-
-	return 0;
+	return rtsx_pci_send_cmd(pcr, 100);
 }
 
 static int rts5227_card_power_off(struct rtsx_pcr *pcr, int card)
diff --git a/drivers/mfd/rts5229.c b/drivers/mfd/rts5229.c
index ace45384ec8b..9ed9dc84eac8 100644
--- a/drivers/mfd/rts5229.c
+++ b/drivers/mfd/rts5229.c
@@ -129,11 +129,7 @@ static int rts5229_card_power_on(struct rtsx_pcr *pcr, int card)
 			SD_POWER_MASK, SD_POWER_ON);
 	rtsx_pci_add_cmd(pcr, WRITE_REG_CMD, PWR_GATE_CTRL,
 			LDO3318_PWR_MASK, 0x06);
-	err = rtsx_pci_send_cmd(pcr, 100);
-	if (err < 0)
-		return err;
-
-	return 0;
+	return rtsx_pci_send_cmd(pcr, 100);
 }
 
 static int rts5229_card_power_off(struct rtsx_pcr *pcr, int card)
diff --git a/drivers/mfd/rts5249.c b/drivers/mfd/rts5249.c
index eb2d5866f719..40f8bb14fc59 100644
--- a/drivers/mfd/rts5249.c
+++ b/drivers/mfd/rts5249.c
@@ -234,11 +234,7 @@ static int rtsx_base_card_power_on(struct rtsx_pcr *pcr, int card)
 			SD_POWER_MASK, SD_VCC_POWER_ON);
 	rtsx_pci_add_cmd(pcr, WRITE_REG_CMD, PWR_GATE_CTRL,
 			LDO3318_PWR_MASK, 0x06);
-	err = rtsx_pci_send_cmd(pcr, 100);
-	if (err < 0)
-		return err;
-
-	return 0;
+	return rtsx_pci_send_cmd(pcr, 100);
 }
 
 static int rtsx_base_card_power_off(struct rtsx_pcr *pcr, int card)
diff --git a/drivers/mfd/rtsx_pcr.c b/drivers/mfd/rtsx_pcr.c
index a66540a49079..b98cf1de0a55 100644
--- a/drivers/mfd/rtsx_pcr.c
+++ b/drivers/mfd/rtsx_pcr.c
@@ -571,11 +571,7 @@ static int rtsx_pci_set_pull_ctl(struct rtsx_pcr *pcr, const u32 *tbl)
 		tbl++;
 	}
 
-	err = rtsx_pci_send_cmd(pcr, 100);
-	if (err < 0)
-		return err;
-
-	return 0;
+	return rtsx_pci_send_cmd(pcr, 100);
 }
 
 int rtsx_pci_card_pull_ctl_enable(struct rtsx_pcr *pcr, int card)
-- 
2.4.3

[PATCH 6/8] mfd: da903x: Simplify function return logic

[Javier Martinez Canillas]

The invoked function already returns 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/da903x.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/mfd/da903x.c b/drivers/mfd/da903x.c
index ef7fe2ae2fa4..37e4426ef061 100644
--- a/drivers/mfd/da903x.c
+++ b/drivers/mfd/da903x.c
@@ -532,11 +532,7 @@ static int da903x_probe(struct i2c_client *client,
 		return ret;
 	}
 
-	ret = da903x_add_subdevs(chip, pdata);
-	if (ret)
-		return ret;
-
-	return 0;
+	return da903x_add_subdevs(chip, pdata);
 }
 
 static int da903x_remove(struct i2c_client *client)
-- 
2.4.3

[PATCH 7/8] mfd: da9052: Simplify function return logic

[Javier Martinez Canillas]

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/da9052-i2c.c | 6 +-----
 drivers/mfd/da9052-spi.c | 6 +-----
 2 files changed, 2 insertions(+), 10 deletions(-)

diff --git a/drivers/mfd/da9052-i2c.c b/drivers/mfd/da9052-i2c.c
index 02887001e800..2697ffb08009 100644
--- a/drivers/mfd/da9052-i2c.c
+++ b/drivers/mfd/da9052-i2c.c
@@ -174,11 +174,7 @@ static int da9052_i2c_probe(struct i2c_client *client,
 		return ret;
 	}
 
-	ret = da9052_device_init(da9052, id->driver_data);
-	if (ret != 0)
-		return ret;
-
-	return 0;
+	return da9052_device_init(da9052, id->driver_data);
 }
 
 static int da9052_i2c_remove(struct i2c_client *client)
diff --git a/drivers/mfd/da9052-spi.c b/drivers/mfd/da9052-spi.c
index b5de8a6856c0..0f5e2c2858c1 100644
--- a/drivers/mfd/da9052-spi.c
+++ b/drivers/mfd/da9052-spi.c
@@ -56,11 +56,7 @@ static int da9052_spi_probe(struct spi_device *spi)
 		return ret;
 	}
 
-	ret = da9052_device_init(da9052, id->driver_data);
-	if (ret != 0)
-		return ret;
-
-	return 0;
+	return da9052_device_init(da9052, id->driver_data);
 }
 
 static int da9052_spi_remove(struct spi_device *spi)
-- 
2.4.3

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

[Javier Martinez Canillas]

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

RE: [PATCH 6/8] mfd: da903x: Simplify function return logic

[Opensource [Adam Thomson]]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 500 bytes --]

On September 29, 2015 12:26, Javier Martinez Canillas wrote:

> The invoked function already returns 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>
> ---

Acked-by: Adam Thomson <Adam.Thomson.Opensource@diasemi.com>
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

RE: [PATCH 7/8] mfd: da9052: Simplify function return logic

[Opensource [Adam Thomson]]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 500 bytes --]

On September 29, 2015 12:26, 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>
> ---

Acked-by: Adam Thomson <Adam.Thomson.Opensource@diasemi.com>
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

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

[Johan Hovold]

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

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

[Javier Martinez Canillas]

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

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

[Johan Hovold]

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

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

[Lee Jones]

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

Re: [PATCH 1/8] mfd: htc-i2cpld: Remove unneded ret variable

[Lee Jones]

On Tue, 29 Sep 2015, Javier Martinez Canillas wrote:

> The ret variable is not needed since is not used in the
> function. Remove the variable and just return 0 instead.
> 
> Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
> ---
> 
>  drivers/mfd/htc-i2cpld.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)

Applied, thanks.

> diff --git a/drivers/mfd/htc-i2cpld.c b/drivers/mfd/htc-i2cpld.c
> index 1bd5b042c8b3..0c6ff727b2ec 100644
> --- a/drivers/mfd/htc-i2cpld.c
> +++ b/drivers/mfd/htc-i2cpld.c
> @@ -318,7 +318,6 @@ static int htcpld_setup_chip_irq(
>  	struct htcpld_data *htcpld;
>  	struct htcpld_chip *chip;
>  	unsigned int irq, irq_end;
> -	int ret = 0;
>  
>  	/* Get the platform and driver data */
>  	htcpld = platform_get_drvdata(pdev);
> @@ -333,7 +332,7 @@ static int htcpld_setup_chip_irq(
>  		irq_clear_status_flags(irq, IRQ_NOREQUEST | IRQ_NOPROBE);
>  	}
>  
> -	return ret;
> +	return 0;
>  }
>  
>  static int htcpld_register_chip_i2c(

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 2/8] mfd: arizona: Remove unneded ret variable

[Lee Jones]

On Tue, 29 Sep 2015, Javier Martinez Canillas wrote:

> The ret variable is not needed since is not used in the
> function. Remove the variable and just return 0 instead.
> 
> Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
> ---
> 
>  drivers/mfd/arizona-core.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)

Applied, thanks.

> diff --git a/drivers/mfd/arizona-core.c b/drivers/mfd/arizona-core.c
> index 44cfdbb295db..acf354875338 100644
> --- a/drivers/mfd/arizona-core.c
> +++ b/drivers/mfd/arizona-core.c
> @@ -69,8 +69,6 @@ EXPORT_SYMBOL_GPL(arizona_clk32k_enable);
>  
>  int arizona_clk32k_disable(struct arizona *arizona)
>  {
> -	int ret = 0;
> -
>  	mutex_lock(&arizona->clk_lock);
>  
>  	BUG_ON(arizona->clk32k_ref <= 0);
> @@ -90,7 +88,7 @@ int arizona_clk32k_disable(struct arizona *arizona)
>  
>  	mutex_unlock(&arizona->clk_lock);
>  
> -	return ret;
> +	return 0;
>  }
>  EXPORT_SYMBOL_GPL(arizona_clk32k_disable);
>  

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 3/8] mfd: pcf50633: Remove unneded ret variable

[Lee Jones]

On Tue, 29 Sep 2015, Javier Martinez Canillas wrote:

> The ret variable is not needed since is not used in the
> function. Remove the variable and just return 0 instead.
> 
> Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
> ---
> 
>  drivers/mfd/pcf50633-irq.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Applied, thanks.

> diff --git a/drivers/mfd/pcf50633-irq.c b/drivers/mfd/pcf50633-irq.c
> index 498286cbb530..71d43edf110c 100644
> --- a/drivers/mfd/pcf50633-irq.c
> +++ b/drivers/mfd/pcf50633-irq.c
> @@ -55,7 +55,7 @@ EXPORT_SYMBOL_GPL(pcf50633_free_irq);
>  static int __pcf50633_irq_mask_set(struct pcf50633 *pcf, int irq, u8 mask)
>  {
>  	u8 reg, bit;
> -	int ret = 0, idx;
> +	int idx;
>  
>  	idx = irq >> 3;
>  	reg = PCF50633_REG_INT1M + idx;
> @@ -72,7 +72,7 @@ static int __pcf50633_irq_mask_set(struct pcf50633 *pcf, int irq, u8 mask)
>  
>  	mutex_unlock(&pcf->lock);
>  
> -	return ret;
> +	return 0;
>  }
>  
>  int pcf50633_irq_mask(struct pcf50633 *pcf, int irq)

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 4/8] mfd: kempld-core: Simplify function return logic

[Lee Jones]

On Tue, 29 Sep 2015, 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>
> ---
> 
>  drivers/mfd/kempld-core.c | 14 ++------------
>  1 file changed, 2 insertions(+), 12 deletions(-)

Applied, thanks.

> diff --git a/drivers/mfd/kempld-core.c b/drivers/mfd/kempld-core.c
> index 463f4eae20c1..05b924542ee2 100644
> --- a/drivers/mfd/kempld-core.c
> +++ b/drivers/mfd/kempld-core.c
> @@ -448,7 +448,6 @@ static int kempld_probe(struct platform_device *pdev)
>  	struct device *dev = &pdev->dev;
>  	struct kempld_device_data *pld;
>  	struct resource *ioport;
> -	int ret;
>  
>  	pld = devm_kzalloc(dev, sizeof(*pld), GFP_KERNEL);
>  	if (!pld)
> @@ -471,11 +470,7 @@ static int kempld_probe(struct platform_device *pdev)
>  	mutex_init(&pld->lock);
>  	platform_set_drvdata(pdev, pld);
>  
> -	ret = kempld_detect_device(pld);
> -	if (ret)
> -		return ret;
> -
> -	return 0;
> +	return kempld_detect_device(pld);
>  }
>  
>  static int kempld_remove(struct platform_device *pdev)
> @@ -756,7 +751,6 @@ MODULE_DEVICE_TABLE(dmi, kempld_dmi_table);
>  static int __init kempld_init(void)
>  {
>  	const struct dmi_system_id *id;
> -	int ret;
>  
>  	if (force_device_id[0]) {
>  		for (id = kempld_dmi_table;
> @@ -771,11 +765,7 @@ static int __init kempld_init(void)
>  			return -ENODEV;
>  	}
>  
> -	ret = platform_driver_register(&kempld_driver);
> -	if (ret)
> -		return ret;
> -
> -	return 0;
> +	return platform_driver_register(&kempld_driver);
>  }
>  
>  static void __exit kempld_exit(void)

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 5/8] mfd: rtsx: Simplify function return logic

[Lee Jones]

On Tue, 29 Sep 2015, 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>
> ---
> 
>  drivers/mfd/rts5209.c  | 6 +-----
>  drivers/mfd/rts5227.c  | 6 +-----
>  drivers/mfd/rts5229.c  | 6 +-----
>  drivers/mfd/rts5249.c  | 6 +-----
>  drivers/mfd/rtsx_pcr.c | 6 +-----
>  5 files changed, 5 insertions(+), 25 deletions(-)

Applied, thanks.

> diff --git a/drivers/mfd/rts5209.c b/drivers/mfd/rts5209.c
> index 373e253c33df..b95beecf767f 100644
> --- a/drivers/mfd/rts5209.c
> +++ b/drivers/mfd/rts5209.c
> @@ -138,11 +138,7 @@ static int rts5209_card_power_on(struct rtsx_pcr *pcr, int card)
>  	rtsx_pci_add_cmd(pcr, WRITE_REG_CMD, CARD_PWR_CTL, pwr_mask, pwr_on);
>  	rtsx_pci_add_cmd(pcr, WRITE_REG_CMD, PWR_GATE_CTRL,
>  			LDO3318_PWR_MASK, 0x00);
> -	err = rtsx_pci_send_cmd(pcr, 100);
> -	if (err < 0)
> -		return err;
> -
> -	return 0;
> +	return rtsx_pci_send_cmd(pcr, 100);
>  }
>  
>  static int rts5209_card_power_off(struct rtsx_pcr *pcr, int card)
> diff --git a/drivers/mfd/rts5227.c b/drivers/mfd/rts5227.c
> index ce012d78ce2a..c5a65298c781 100644
> --- a/drivers/mfd/rts5227.c
> +++ b/drivers/mfd/rts5227.c
> @@ -179,11 +179,7 @@ static int rts5227_card_power_on(struct rtsx_pcr *pcr, int card)
>  			SD_POWER_MASK, SD_POWER_ON);
>  	rtsx_pci_add_cmd(pcr, WRITE_REG_CMD, PWR_GATE_CTRL,
>  			LDO3318_PWR_MASK, 0x06);
> -	err = rtsx_pci_send_cmd(pcr, 100);
> -	if (err < 0)
> -		return err;
> -
> -	return 0;
> +	return rtsx_pci_send_cmd(pcr, 100);
>  }
>  
>  static int rts5227_card_power_off(struct rtsx_pcr *pcr, int card)
> diff --git a/drivers/mfd/rts5229.c b/drivers/mfd/rts5229.c
> index ace45384ec8b..9ed9dc84eac8 100644
> --- a/drivers/mfd/rts5229.c
> +++ b/drivers/mfd/rts5229.c
> @@ -129,11 +129,7 @@ static int rts5229_card_power_on(struct rtsx_pcr *pcr, int card)
>  			SD_POWER_MASK, SD_POWER_ON);
>  	rtsx_pci_add_cmd(pcr, WRITE_REG_CMD, PWR_GATE_CTRL,
>  			LDO3318_PWR_MASK, 0x06);
> -	err = rtsx_pci_send_cmd(pcr, 100);
> -	if (err < 0)
> -		return err;
> -
> -	return 0;
> +	return rtsx_pci_send_cmd(pcr, 100);
>  }
>  
>  static int rts5229_card_power_off(struct rtsx_pcr *pcr, int card)
> diff --git a/drivers/mfd/rts5249.c b/drivers/mfd/rts5249.c
> index eb2d5866f719..40f8bb14fc59 100644
> --- a/drivers/mfd/rts5249.c
> +++ b/drivers/mfd/rts5249.c
> @@ -234,11 +234,7 @@ static int rtsx_base_card_power_on(struct rtsx_pcr *pcr, int card)
>  			SD_POWER_MASK, SD_VCC_POWER_ON);
>  	rtsx_pci_add_cmd(pcr, WRITE_REG_CMD, PWR_GATE_CTRL,
>  			LDO3318_PWR_MASK, 0x06);
> -	err = rtsx_pci_send_cmd(pcr, 100);
> -	if (err < 0)
> -		return err;
> -
> -	return 0;
> +	return rtsx_pci_send_cmd(pcr, 100);
>  }
>  
>  static int rtsx_base_card_power_off(struct rtsx_pcr *pcr, int card)
> diff --git a/drivers/mfd/rtsx_pcr.c b/drivers/mfd/rtsx_pcr.c
> index a66540a49079..b98cf1de0a55 100644
> --- a/drivers/mfd/rtsx_pcr.c
> +++ b/drivers/mfd/rtsx_pcr.c
> @@ -571,11 +571,7 @@ static int rtsx_pci_set_pull_ctl(struct rtsx_pcr *pcr, const u32 *tbl)
>  		tbl++;
>  	}
>  
> -	err = rtsx_pci_send_cmd(pcr, 100);
> -	if (err < 0)
> -		return err;
> -
> -	return 0;
> +	return rtsx_pci_send_cmd(pcr, 100);
>  }
>  
>  int rtsx_pci_card_pull_ctl_enable(struct rtsx_pcr *pcr, int card)

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 6/8] mfd: da903x: Simplify function return logic

[Lee Jones]

On Tue, 29 Sep 2015, Javier Martinez Canillas wrote:

> The invoked function already returns 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/da903x.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)

Applied, thanks.

> diff --git a/drivers/mfd/da903x.c b/drivers/mfd/da903x.c
> index ef7fe2ae2fa4..37e4426ef061 100644
> --- a/drivers/mfd/da903x.c
> +++ b/drivers/mfd/da903x.c
> @@ -532,11 +532,7 @@ static int da903x_probe(struct i2c_client *client,
>  		return ret;
>  	}
>  
> -	ret = da903x_add_subdevs(chip, pdata);
> -	if (ret)
> -		return ret;
> -
> -	return 0;
> +	return da903x_add_subdevs(chip, pdata);
>  }
>  
>  static int da903x_remove(struct i2c_client *client)

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 7/8] mfd: da9052: Simplify function return logic

[Lee Jones]

On Tue, 29 Sep 2015, 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>
> ---
> 
>  drivers/mfd/da9052-i2c.c | 6 +-----
>  drivers/mfd/da9052-spi.c | 6 +-----
>  2 files changed, 2 insertions(+), 10 deletions(-)

Applied, thanks.

> diff --git a/drivers/mfd/da9052-i2c.c b/drivers/mfd/da9052-i2c.c
> index 02887001e800..2697ffb08009 100644
> --- a/drivers/mfd/da9052-i2c.c
> +++ b/drivers/mfd/da9052-i2c.c
> @@ -174,11 +174,7 @@ static int da9052_i2c_probe(struct i2c_client *client,
>  		return ret;
>  	}
>  
> -	ret = da9052_device_init(da9052, id->driver_data);
> -	if (ret != 0)
> -		return ret;
> -
> -	return 0;
> +	return da9052_device_init(da9052, id->driver_data);
>  }
>  
>  static int da9052_i2c_remove(struct i2c_client *client)
> diff --git a/drivers/mfd/da9052-spi.c b/drivers/mfd/da9052-spi.c
> index b5de8a6856c0..0f5e2c2858c1 100644
> --- a/drivers/mfd/da9052-spi.c
> +++ b/drivers/mfd/da9052-spi.c
> @@ -56,11 +56,7 @@ static int da9052_spi_probe(struct spi_device *spi)
>  		return ret;
>  	}
>  
> -	ret = da9052_device_init(da9052, id->driver_data);
> -	if (ret != 0)
> -		return ret;
> -
> -	return 0;
> +	return da9052_device_init(da9052, id->driver_data);
>  }
>  
>  static int da9052_spi_remove(struct spi_device *spi)

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

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

[Lee Jones]

On Tue, 29 Sep 2015, 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>
> 
> ---
> 
>  drivers/mfd/lm3533-core.c | 12 ++----------
>  1 file changed, 2 insertions(+), 10 deletions(-)

Applied, thanks.

> 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)

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

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

[Johan Hovold]

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