[PATCH 0/3] ASoC: da7218: Adjustments for two function implementations

[PATCH 0/3] ASoC: da7218: Adjustments for two function implementations

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 23 Nov 2017 21:00:12 +0100

Three update suggestions were taken into account
from static source code analysis.

Markus Elfring (3):
  Delete two error messages for a failed memory allocation
    in da7218_of_to_pdata()
  Use common error handling code in da7218_of_to_pdata()
  Improve a size determination in da7218_i2c_probe()

 sound/soc/codecs/da7218.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

-- 
2.15.0

[PATCH 1/3] ASoC: da7218: Delete two error messages for a failed memory allocation in da7218_of_to_pdata()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 23 Nov 2017 20:42:20 +0100

Omit extra messages for a memory allocation failure in this function.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 sound/soc/codecs/da7218.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/sound/soc/codecs/da7218.c b/sound/soc/codecs/da7218.c
index 56564ce90cb6..25ab7443d803 100644
--- a/sound/soc/codecs/da7218.c
+++ b/sound/soc/codecs/da7218.c
@@ -2455,10 +2455,8 @@ static struct da7218_pdata *da7218_of_to_pdata(struct snd_soc_codec *codec)
 	u32 of_val32;
 
 	pdata = devm_kzalloc(codec->dev, sizeof(*pdata), GFP_KERNEL);
-	if (!pdata) {
-		dev_warn(codec->dev, "Failed to allocate memory for pdata\n");
+	if (!pdata)
 		return NULL;
-	}
 
 	if (of_property_read_u32(np, "dlg,micbias1-lvl-millivolt", &of_val32) >= 0)
 		pdata->micbias1_lvl = da7218_of_micbias_lvl(codec, of_val32);
@@ -2527,8 +2525,6 @@ static struct da7218_pdata *da7218_of_to_pdata(struct snd_soc_codec *codec)
 		hpldet_pdata = devm_kzalloc(codec->dev, sizeof(*hpldet_pdata),
 					    GFP_KERNEL);
 		if (!hpldet_pdata) {
-			dev_warn(codec->dev,
-				 "Failed to allocate memory for hpldet pdata\n");
 			of_node_put(hpldet_np);
 			return pdata;
 		}
-- 
2.15.0

[PATCH 2/3] ASoC: da7218: Use common error handling code in da7218_of_to_pdata()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 23 Nov 2017 20:48:05 +0100

Add a jump target so that a bit of exception handling can be better reused
in an if branch of this function.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 sound/soc/codecs/da7218.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/sound/soc/codecs/da7218.c b/sound/soc/codecs/da7218.c
index 25ab7443d803..93a0cb741751 100644
--- a/sound/soc/codecs/da7218.c
+++ b/sound/soc/codecs/da7218.c
@@ -2524,10 +2524,9 @@ static struct da7218_pdata *da7218_of_to_pdata(struct snd_soc_codec *codec)
 
 		hpldet_pdata = devm_kzalloc(codec->dev, sizeof(*hpldet_pdata),
 					    GFP_KERNEL);
-		if (!hpldet_pdata) {
-			of_node_put(hpldet_np);
-			return pdata;
-		}
+		if (!hpldet_pdata)
+			goto put_node;
+
 		pdata->hpldet_pdata = hpldet_pdata;
 
 		if (of_property_read_u32(hpldet_np, "dlg,jack-rate-us",
@@ -2561,6 +2560,7 @@ static struct da7218_pdata *da7218_of_to_pdata(struct snd_soc_codec *codec)
 		if (of_property_read_bool(hpldet_np, "dlg,discharge"))
 			hpldet_pdata->discharge = true;
 
+put_node:
 		of_node_put(hpldet_np);
 	}
 
-- 
2.15.0

[PATCH 3/3] ASoC: da7218: Improve a size determination in da7218_i2c_probe()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 23 Nov 2017 20:50:44 +0100

Replace the specification of a data structure by a pointer dereference
as the parameter for the operator "sizeof" to make the corresponding size
determination a bit safer according to the Linux coding style convention.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 sound/soc/codecs/da7218.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/sound/soc/codecs/da7218.c b/sound/soc/codecs/da7218.c
index 93a0cb741751..0c1933a98995 100644
--- a/sound/soc/codecs/da7218.c
+++ b/sound/soc/codecs/da7218.c
@@ -3269,8 +3269,7 @@ static int da7218_i2c_probe(struct i2c_client *i2c,
 	struct da7218_priv *da7218;
 	int ret;
 
-	da7218 = devm_kzalloc(&i2c->dev, sizeof(struct da7218_priv),
-			      GFP_KERNEL);
+	da7218 = devm_kzalloc(&i2c->dev, sizeof(*da7218), GFP_KERNEL);
 	if (!da7218)
 		return -ENOMEM;
 
-- 
2.15.0

RE: [PATCH 1/3] ASoC: da7218: Delete two error messages for a failed memory allocation in da7218_of_to_pdata()

[Adam Thomson]

On 23 November 2017 20:05, SF Markus Elfring wrote:

> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Thu, 23 Nov 2017 20:42:20 +0100
> 
> Omit extra messages for a memory allocation failure in this function.
> 
> This issue was detected by using the Coccinelle software.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>

Acked-by: Adam Thomson <Adam.Thomson.Opensource@diasemi.com>

> ---
>  sound/soc/codecs/da7218.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/sound/soc/codecs/da7218.c b/sound/soc/codecs/da7218.c
> index 56564ce90cb6..25ab7443d803 100644
> --- a/sound/soc/codecs/da7218.c
> +++ b/sound/soc/codecs/da7218.c
> @@ -2455,10 +2455,8 @@ static struct da7218_pdata *da7218_of_to_pdata(struct
> snd_soc_codec *codec)
>  	u32 of_val32;
> 
>  	pdata = devm_kzalloc(codec->dev, sizeof(*pdata), GFP_KERNEL);
> -	if (!pdata) {
> -		dev_warn(codec->dev, "Failed to allocate memory for pdata\n");
> +	if (!pdata)
>  		return NULL;
> -	}
> 
>  	if (of_property_read_u32(np, "dlg,micbias1-lvl-millivolt", &of_val32) >= 0)
>  		pdata->micbias1_lvl = da7218_of_micbias_lvl(codec, of_val32);
> @@ -2527,8 +2525,6 @@ static struct da7218_pdata *da7218_of_to_pdata(struct
> snd_soc_codec *codec)
>  		hpldet_pdata = devm_kzalloc(codec->dev, sizeof(*hpldet_pdata),
>  					    GFP_KERNEL);
>  		if (!hpldet_pdata) {
> -			dev_warn(codec->dev,
> -				 "Failed to allocate memory for hpldet pdata\n");
>  			of_node_put(hpldet_np);
>  			return pdata;
>  		}
> --
> 2.15.0

RE: [PATCH 3/3] ASoC: da7218: Improve a size determination in da7218_i2c_probe()

[Adam Thomson]

On 23 November 2017 20:08, SF Markus Elfring wrote:

> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Thu, 23 Nov 2017 20:50:44 +0100
> 
> Replace the specification of a data structure by a pointer dereference
> as the parameter for the operator "sizeof" to make the corresponding size
> determination a bit safer according to the Linux coding style convention.
> 
> This issue was detected by using the Coccinelle software.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>

Acked-by: Adam Thomson <Adam.Thomson.Opensource@diasemi.com>

> ---
>  sound/soc/codecs/da7218.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/sound/soc/codecs/da7218.c b/sound/soc/codecs/da7218.c
> index 93a0cb741751..0c1933a98995 100644
> --- a/sound/soc/codecs/da7218.c
> +++ b/sound/soc/codecs/da7218.c
> @@ -3269,8 +3269,7 @@ static int da7218_i2c_probe(struct i2c_client *i2c,
>  	struct da7218_priv *da7218;
>  	int ret;
> 
> -	da7218 = devm_kzalloc(&i2c->dev, sizeof(struct da7218_priv),
> -			      GFP_KERNEL);
> +	da7218 = devm_kzalloc(&i2c->dev, sizeof(*da7218), GFP_KERNEL);
>  	if (!da7218)
>  		return -ENOMEM;
> 
> --
> 2.15.0

RE: [PATCH 2/3] ASoC: da7218: Use common error handling code in da7218_of_to_pdata()

[Adam Thomson]

On 23 November 2017 20:06, SF Markus Elfring wrote:

> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Thu, 23 Nov 2017 20:48:05 +0100
> 
> Add a jump target so that a bit of exception handling can be better reused
> in an if branch of this function.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>

Hmm. Doesn't really gain an awful lot this. Would understand if there were
multiple return paths, but in that case I'd have implemented something like
this anyway. Also your patch description isn't really correct. You're re-using
code from the sunny day scenario to handle an exception.

> ---
>  sound/soc/codecs/da7218.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/sound/soc/codecs/da7218.c b/sound/soc/codecs/da7218.c
> index 25ab7443d803..93a0cb741751 100644
> --- a/sound/soc/codecs/da7218.c
> +++ b/sound/soc/codecs/da7218.c
> @@ -2524,10 +2524,9 @@ static struct da7218_pdata *da7218_of_to_pdata(struct
> snd_soc_codec *codec)
> 
>  		hpldet_pdata = devm_kzalloc(codec->dev, sizeof(*hpldet_pdata),
>  					    GFP_KERNEL);
> -		if (!hpldet_pdata) {
> -			of_node_put(hpldet_np);
> -			return pdata;
> -		}
> +		if (!hpldet_pdata)
> +			goto put_node;
> +
>  		pdata->hpldet_pdata = hpldet_pdata;
> 
>  		if (of_property_read_u32(hpldet_np, "dlg,jack-rate-us",
> @@ -2561,6 +2560,7 @@ static struct da7218_pdata *da7218_of_to_pdata(struct
> snd_soc_codec *codec)
>  		if (of_property_read_bool(hpldet_np, "dlg,discharge"))
>  			hpldet_pdata->discharge = true;
> 
> +put_node:
>  		of_node_put(hpldet_np);
>  	}
> 
> --
> 2.15.0

Re: ASoC: da7218: Use common error handling code in da7218_of_to_pdata()

[SF Markus Elfring]

>> Add a jump target so that a bit of exception handling can be better reused
>> in an if branch of this function.
…
> Hmm. Doesn't really gain an awful lot this.

I show just another small change possibility.


> Would understand if there were multiple return paths,
> but in that case I'd have implemented something like this anyway.

Where?

Can the suggested software refactoring become useful also for this
function implementation?


> Also your patch description isn't really correct.

Which wording would you find more appropriate?


> You're re-using code from the sunny day scenario to handle an exception.

Can this detail be better?

Regards,
Markus

Re: [PATCH 2/3] ASoC: da7218: Use common error handling code in da7218_of_to_pdata()

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 657 bytes --]

On Fri, Nov 24, 2017 at 12:03:32PM +0000, Adam Thomson wrote:
> On 23 November 2017 20:06, SF Markus Elfring wrote:

> > Add a jump target so that a bit of exception handling can be better reused
> > in an if branch of this function.

> Hmm. Doesn't really gain an awful lot this. Would understand if there were
> multiple return paths, but in that case I'd have implemented something like
> this anyway. Also your patch description isn't really correct. You're re-using
> code from the sunny day scenario to handle an exception.

Other people have given him similar feedback on his other patches with
this pattern.  I'm not intending to apply any of them.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: ASoC: da7218: Use common error handling code in da7218_of_to_pdata()

[SF Markus Elfring]

>>> Add a jump target so that a bit of exception handling can be better reused
>>> in an if branch of this function.
> 
>> Hmm. Doesn't really gain an awful lot this. Would understand if there were
>> multiple return paths, but in that case I'd have implemented something like
>> this anyway. Also your patch description isn't really correct. You're re-using
>> code from the sunny day scenario to handle an exception.
> 
> Other people have given him similar feedback on his other patches with
> this pattern.  I'm not intending to apply any of them.

I can offer another bit of information for a software development discussion. 💭

The affected source file can be compiled for the processor architecture “x86_64”
by a tool like “GCC 6.4.1+r251631-1.3” from the software distribution
“openSUSE Tumbleweed” with the following command example.

my_cc=/usr/bin/gcc-6 \
&& my_module=sound/soc/codecs/da7218.o \
&& for XYZ in 0 s 3; do echo "   _____ $XYZ _____" \
&& my_extra="-O$XYZ" \
&& git checkout ':/^ASoC: da7218: Delete two error messages for a failed memory allocation in da7218_of_to_pdata' \
&& make -j4 CC="${my_cc}" HOSTCC="${my_cc}" EXTRA_CFLAGS="${my_extra}" allmodconfig "${my_module}" \
&& size "${my_module}" \
&& git checkout ':/^ASoC: da7218: Use common error handling code in da7218_of_to_pdata' \
&& make -j4 CC="${my_cc}" HOSTCC="${my_cc}" EXTRA_CFLAGS="${my_extra}" allmodconfig "${my_module}" \
&& size "${my_module}"; done


🔮 Do you find the following differences worth for further clarification?

╔═════════╤══════╗
║ setting │ text ║
╠═════════╪══════╣
║   O0    │ -25  ║
║   Os    │ -14  ║
║   O3    │ +11  ║
╚═════════╧══════╝


Regards,
Markus

Applied "ASoC: da7218: Improve a size determination in da7218_i2c_probe()" to the asoc tree

[Mark Brown]

The patch

   ASoC: da7218: Improve a size determination in da7218_i2c_probe()

has been applied to the asoc tree at

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git 

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

>From 392b79e20b41cfdc174d31bd4b004bbd874de4d9 Mon Sep 17 00:00:00 2001
From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 23 Nov 2017 20:50:44 +0100
Subject: [PATCH] ASoC: da7218: Improve a size determination in
 da7218_i2c_probe()

Replace the specification of a data structure by a pointer dereference
as the parameter for the operator "sizeof" to make the corresponding size
determination a bit safer according to the Linux coding style convention.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
Acked-by: Adam Thomson <Adam.Thomson.Opensource@diasemi.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 sound/soc/codecs/da7218.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/sound/soc/codecs/da7218.c b/sound/soc/codecs/da7218.c
index 25ab7443d803..96c644a15b11 100644
--- a/sound/soc/codecs/da7218.c
+++ b/sound/soc/codecs/da7218.c
@@ -3269,8 +3269,7 @@ static int da7218_i2c_probe(struct i2c_client *i2c,
 	struct da7218_priv *da7218;
 	int ret;
 
-	da7218 = devm_kzalloc(&i2c->dev, sizeof(struct da7218_priv),
-			      GFP_KERNEL);
+	da7218 = devm_kzalloc(&i2c->dev, sizeof(*da7218), GFP_KERNEL);
 	if (!da7218)
 		return -ENOMEM;
 
-- 
2.15.0

Applied "ASoC: da7218: Delete two error messages for a failed memory allocation in da7218_of_to_pdata()" to the asoc tree

[Mark Brown]

The patch

   ASoC: da7218: Delete two error messages for a failed memory allocation in da7218_of_to_pdata()

has been applied to the asoc tree at

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git 

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

>From 14a07f1d8c4c64af29566316df0415052e8bdfe4 Mon Sep 17 00:00:00 2001
From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 23 Nov 2017 20:42:20 +0100
Subject: [PATCH] ASoC: da7218: Delete two error messages for a failed memory
 allocation in da7218_of_to_pdata()

Omit extra messages for a memory allocation failure in this function.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
Acked-by: Adam Thomson <Adam.Thomson.Opensource@diasemi.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 sound/soc/codecs/da7218.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/sound/soc/codecs/da7218.c b/sound/soc/codecs/da7218.c
index 56564ce90cb6..25ab7443d803 100644
--- a/sound/soc/codecs/da7218.c
+++ b/sound/soc/codecs/da7218.c
@@ -2455,10 +2455,8 @@ static struct da7218_pdata *da7218_of_to_pdata(struct snd_soc_codec *codec)
 	u32 of_val32;
 
 	pdata = devm_kzalloc(codec->dev, sizeof(*pdata), GFP_KERNEL);
-	if (!pdata) {
-		dev_warn(codec->dev, "Failed to allocate memory for pdata\n");
+	if (!pdata)
 		return NULL;
-	}
 
 	if (of_property_read_u32(np, "dlg,micbias1-lvl-millivolt", &of_val32) >= 0)
 		pdata->micbias1_lvl = da7218_of_micbias_lvl(codec, of_val32);
@@ -2527,8 +2525,6 @@ static struct da7218_pdata *da7218_of_to_pdata(struct snd_soc_codec *codec)
 		hpldet_pdata = devm_kzalloc(codec->dev, sizeof(*hpldet_pdata),
 					    GFP_KERNEL);
 		if (!hpldet_pdata) {
-			dev_warn(codec->dev,
-				 "Failed to allocate memory for hpldet pdata\n");
 			of_node_put(hpldet_np);
 			return pdata;
 		}
-- 
2.15.0