[PATCH 0/5] ASoC: SOF/Intel: fix cppcheck warnings

[PATCH 0/5] ASoC: SOF/Intel: fix cppcheck warnings

[Pierre-Louis Bossart]

A small set of fixes to reduce the number of warnings.

Pierre-Louis Bossart (5):
  ASOC: SOF: Intel: hda-codec: move unused label to correct position
  ASoC: SOF: Intel: hda-codec: move variable used conditionally
  ASoC: Intel: rename shadowed variable for all broadwell boards
  ASoC: Intel: bytcht_cx2072x: simplify return handling
  ASoC: Intel: sof_sdw: clarify operator precedence

 sound/soc/intel/boards/bdw-rt5650.c     | 10 +++++-----
 sound/soc/intel/boards/bdw-rt5677.c     |  8 ++++----
 sound/soc/intel/boards/broadwell.c      |  8 ++++----
 sound/soc/intel/boards/bytcht_cx2072x.c |  2 +-
 sound/soc/intel/boards/sof_sdw.c        |  2 +-
 sound/soc/sof/intel/hda-codec.c         |  7 ++++++-
 6 files changed, 21 insertions(+), 16 deletions(-)

-- 
2.25.1

[PATCH 1/5] ASOC: SOF: Intel: hda-codec: move unused label to correct position

[Pierre-Louis Bossart]

Cppcheck reports the following warning:

sound/soc/sof/intel/hda-codec.c:191:1: style: Label 'error' is not
used. [unusedLabel]

This label is indeed only used conditionally, move it where it's
actually used.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 sound/soc/sof/intel/hda-codec.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/sound/soc/sof/intel/hda-codec.c b/sound/soc/sof/intel/hda-codec.c
index 2c5c451fa19d..119aa9ffcc66 100644
--- a/sound/soc/sof/intel/hda-codec.c
+++ b/sound/soc/sof/intel/hda-codec.c
@@ -178,6 +178,11 @@ static int hda_codec_probe(struct snd_sof_dev *sdev, int address,
 	}
 
 	return ret;
+
+error:
+	snd_hdac_ext_bus_device_exit(hdev);
+	return -ENOENT;
+
 #else
 	hdev = devm_kzalloc(sdev->dev, sizeof(*hdev), GFP_KERNEL);
 	if (!hdev)
-- 
2.25.1

Re: [PATCH 1/5] ASOC: SOF: Intel: hda-codec: move unused label to correct position

[Nathan Chancellor]

On Thu, Aug 13, 2020 at 12:58:35PM -0500, Pierre-Louis Bossart wrote:
> Cppcheck reports the following warning:
> 
> sound/soc/sof/intel/hda-codec.c:191:1: style: Label 'error' is not
> used. [unusedLabel]
> 
> This label is indeed only used conditionally, move it where it's
> actually used.
> 
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> ---
>  sound/soc/sof/intel/hda-codec.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/sound/soc/sof/intel/hda-codec.c b/sound/soc/sof/intel/hda-codec.c
> index 2c5c451fa19d..119aa9ffcc66 100644
> --- a/sound/soc/sof/intel/hda-codec.c
> +++ b/sound/soc/sof/intel/hda-codec.c
> @@ -178,6 +178,11 @@ static int hda_codec_probe(struct snd_sof_dev *sdev, int address,
>  	}
>  
>  	return ret;
> +
> +error:
> +	snd_hdac_ext_bus_device_exit(hdev);
> +	return -ENOENT;
> +
>  #else
>  	hdev = devm_kzalloc(sdev->dev, sizeof(*hdev), GFP_KERNEL);
>  	if (!hdev)
> -- 
> 2.25.1
> 

I don't get this patch because there is no moving of a label, it just
introduces it, where it is actually completely unused in the function as
far as I can tell in both v5.9-rc1 and next-20200821. When building with
clang in certain configurations, this introduces the same type of
warning:

sound/soc/sof/intel/hda-codec.c:182:1: warning: unused label 'error'
[-Wunused-label]
error:
^~~~~~
1 warning generated.

It seems like this should be reverted as it does not actually do
anything.

Cheers,
Nathan

Re: [PATCH 1/5] ASOC: SOF: Intel: hda-codec: move unused label to correct position

[Pierre-Louis Bossart]

> I don't get this patch because there is no moving of a label, it just
> introduces it, where it is actually completely unused in the function as
> far as I can tell in both v5.9-rc1 and next-20200821. When building with
> clang in certain configurations, this introduces the same type of
> warning:
> 
> sound/soc/sof/intel/hda-codec.c:182:1: warning: unused label 'error'
> [-Wunused-label]
> error:
> ^~~~~~
> 1 warning generated.
> 
> It seems like this should be reverted as it does not actually do
> anything.

I must have made a mistake with these cppcheck patches, the patch that 
needed to be fixed is not upstream but in the SOF tree. I will send it 
later today, sorry about the noise.

[PATCH 2/5] ASoC: SOF: Intel: hda-codec: move variable used conditionally

[Pierre-Louis Bossart]

Cppcheck reports the following warning:

sound/soc/sof/intel/hda-codec.c:122:20: style: Unused variable: codec
[unusedVariable]
 struct hda_codec *codec;
                   ^

Move declaration inside a conditionally-compiled block.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 sound/soc/sof/intel/hda-codec.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/soc/sof/intel/hda-codec.c b/sound/soc/sof/intel/hda-codec.c
index 119aa9ffcc66..55811b99e47a 100644
--- a/sound/soc/sof/intel/hda-codec.c
+++ b/sound/soc/sof/intel/hda-codec.c
@@ -116,10 +116,10 @@ static int hda_codec_probe(struct snd_sof_dev *sdev, int address,
 {
 #if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA_AUDIO_CODEC)
 	struct hdac_hda_priv *hda_priv;
+	struct hda_codec *codec;
 #endif
 	struct hda_bus *hbus = sof_to_hbus(sdev);
 	struct hdac_device *hdev;
-	struct hda_codec *codec;
 	u32 hda_cmd = (address << 28) | (AC_NODE_ROOT << 20) |
 		(AC_VERB_PARAMETERS << 8) | AC_PAR_VENDOR_ID;
 	u32 resp = -1;
-- 
2.25.1

[PATCH 3/5] ASoC: Intel: rename shadowed variable for all broadwell boards

[Pierre-Louis Bossart]

Fix cppcheck warnings:

sound/soc/intel/boards/bdw-rt5650.c:91:23: style: Local variable
'channels' shadows outer variable [shadowVariable]

sound/soc/intel/boards/bdw-rt5677.c:144:23: style: Local variable
'channels' shadows outer variable [shadowVariable]

sound/soc/intel/boards/broadwell.c:91:23: style: Local variable
'channels' shadows outer variable [shadowVariable]

This was fixed earlier in other machine drivers but keeps coming back
with copy/paste.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 sound/soc/intel/boards/bdw-rt5650.c | 10 +++++-----
 sound/soc/intel/boards/bdw-rt5677.c |  8 ++++----
 sound/soc/intel/boards/broadwell.c  |  8 ++++----
 3 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/sound/soc/intel/boards/bdw-rt5650.c b/sound/soc/intel/boards/bdw-rt5650.c
index ce7320916b22..1412a9941ed4 100644
--- a/sound/soc/intel/boards/bdw-rt5650.c
+++ b/sound/soc/intel/boards/bdw-rt5650.c
@@ -87,14 +87,14 @@ static int broadwell_ssp0_fixup(struct snd_soc_pcm_runtime *rtd,
 			struct snd_pcm_hw_params *params)
 {
 	struct snd_interval *rate = hw_param_interval(params,
-			SNDRV_PCM_HW_PARAM_RATE);
-	struct snd_interval *channels = hw_param_interval(params,
-						SNDRV_PCM_HW_PARAM_CHANNELS);
+						      SNDRV_PCM_HW_PARAM_RATE);
+	struct snd_interval *chan = hw_param_interval(params,
+						      SNDRV_PCM_HW_PARAM_CHANNELS);
 
 	/* The ADSP will covert the FE rate to 48k, max 4-channels */
 	rate->min = rate->max = 48000;
-	channels->min = 2;
-	channels->max = 4;
+	chan->min = 2;
+	chan->max = 4;
 
 	/* set SSP0 to 24 bit */
 	snd_mask_set_format(hw_param_mask(params, SNDRV_PCM_HW_PARAM_FORMAT),
diff --git a/sound/soc/intel/boards/bdw-rt5677.c b/sound/soc/intel/boards/bdw-rt5677.c
index 86e427e3822f..297871bcaf5d 100644
--- a/sound/soc/intel/boards/bdw-rt5677.c
+++ b/sound/soc/intel/boards/bdw-rt5677.c
@@ -140,13 +140,13 @@ static int broadwell_ssp0_fixup(struct snd_soc_pcm_runtime *rtd,
 			struct snd_pcm_hw_params *params)
 {
 	struct snd_interval *rate = hw_param_interval(params,
-			SNDRV_PCM_HW_PARAM_RATE);
-	struct snd_interval *channels = hw_param_interval(params,
-						SNDRV_PCM_HW_PARAM_CHANNELS);
+						      SNDRV_PCM_HW_PARAM_RATE);
+	struct snd_interval *chan = hw_param_interval(params,
+						      SNDRV_PCM_HW_PARAM_CHANNELS);
 
 	/* The ADSP will covert the FE rate to 48k, stereo */
 	rate->min = rate->max = 48000;
-	channels->min = channels->max = 2;
+	chan->min = chan->max = 2;
 
 	/* set SSP0 to 16 bit */
 	params_set_format(params, SNDRV_PCM_FORMAT_S16_LE);
diff --git a/sound/soc/intel/boards/broadwell.c b/sound/soc/intel/boards/broadwell.c
index f6399077d291..56972af13b6f 100644
--- a/sound/soc/intel/boards/broadwell.c
+++ b/sound/soc/intel/boards/broadwell.c
@@ -87,13 +87,13 @@ static int broadwell_ssp0_fixup(struct snd_soc_pcm_runtime *rtd,
 			struct snd_pcm_hw_params *params)
 {
 	struct snd_interval *rate = hw_param_interval(params,
-			SNDRV_PCM_HW_PARAM_RATE);
-	struct snd_interval *channels = hw_param_interval(params,
-						SNDRV_PCM_HW_PARAM_CHANNELS);
+						      SNDRV_PCM_HW_PARAM_RATE);
+	struct snd_interval *chan = hw_param_interval(params,
+						      SNDRV_PCM_HW_PARAM_CHANNELS);
 
 	/* The ADSP will covert the FE rate to 48k, stereo */
 	rate->min = rate->max = 48000;
-	channels->min = channels->max = 2;
+	chan->min = chan->max = 2;
 
 	/* set SSP0 to 16 bit */
 	params_set_format(params, SNDRV_PCM_FORMAT_S16_LE);
-- 
2.25.1

[PATCH 4/5] ASoC: Intel: bytcht_cx2072x: simplify return handling

[Pierre-Louis Bossart]

Fix cppcheck warning:

sound/soc/intel/boards/bytcht_cx2072x.c:102:9: warning: Identical
condition and return expression 'ret', return value is always 0
[identicalConditionAfterEarlyExit]

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 sound/soc/intel/boards/bytcht_cx2072x.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/soc/intel/boards/bytcht_cx2072x.c b/sound/soc/intel/boards/bytcht_cx2072x.c
index 9cb42ba40c07..0b50b3646d55 100644
--- a/sound/soc/intel/boards/bytcht_cx2072x.c
+++ b/sound/soc/intel/boards/bytcht_cx2072x.c
@@ -99,7 +99,7 @@ static int byt_cht_cx2072x_init(struct snd_soc_pcm_runtime *rtd)
 
 	snd_soc_dai_set_bclk_ratio(asoc_rtd_to_codec(rtd, 0), 50);
 
-	return ret;
+	return 0;
 }
 
 static int byt_cht_cx2072x_fixup(struct snd_soc_pcm_runtime *rtd,
-- 
2.25.1

[PATCH 5/5] ASoC: Intel: sof_sdw: clarify operator precedence

[Pierre-Louis Bossart]

Fix cppcheck warning:

sound/soc/intel/boards/sof_sdw.c:739:46: style: Clarify calculation
precedence for '&' and '?'. [clarifyCalculation]
 hdmi_num = sof_sdw_quirk & SOF_SDW_TGL_HDMI ?
                                             ^

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 sound/soc/intel/boards/sof_sdw.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/soc/intel/boards/sof_sdw.c b/sound/soc/intel/boards/sof_sdw.c
index 2463d432bf4d..baff9e99f626 100644
--- a/sound/soc/intel/boards/sof_sdw.c
+++ b/sound/soc/intel/boards/sof_sdw.c
@@ -728,7 +728,7 @@ static int sof_card_dai_links_create(struct device *dev,
 	for (i = 0; i < ARRAY_SIZE(codec_info_list); i++)
 		codec_info_list[i].amp_num = 0;
 
-	hdmi_num = sof_sdw_quirk & SOF_SDW_TGL_HDMI ?
+	hdmi_num = (sof_sdw_quirk & SOF_SDW_TGL_HDMI) ?
 				SOF_TGL_HDMI_COUNT : SOF_PRE_TGL_HDMI_COUNT;
 
 	ssp_mask = SOF_SSP_GET_PORT(sof_sdw_quirk);
-- 
2.25.1

Re: [PATCH 5/5] ASoC: Intel: sof_sdw: clarify operator precedence

[Mark Brown]

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

On Thu, Aug 13, 2020 at 12:58:39PM -0500, Pierre-Louis Bossart wrote:

> -	hdmi_num = sof_sdw_quirk & SOF_SDW_TGL_HDMI ?
> +	hdmi_num = (sof_sdw_quirk & SOF_SDW_TGL_HDMI) ?
>  				SOF_TGL_HDMI_COUNT : SOF_PRE_TGL_HDMI_COUNT;

Or better yet, just don't abuse the ternery operator like this and write
normal conditional statements.

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

Re: [PATCH 5/5] ASoC: Intel: sof_sdw: clarify operator precedence

[Pierre-Louis Bossart]

On 8/13/20 1:45 PM, Mark Brown wrote:
> On Thu, Aug 13, 2020 at 12:58:39PM -0500, Pierre-Louis Bossart wrote:
> 
>> -	hdmi_num = sof_sdw_quirk & SOF_SDW_TGL_HDMI ?
>> +	hdmi_num = (sof_sdw_quirk & SOF_SDW_TGL_HDMI) ?
>>   				SOF_TGL_HDMI_COUNT : SOF_PRE_TGL_HDMI_COUNT;
> 
> Or better yet, just don't abuse the ternery operator like this and write
> normal conditional statements.

I count 795 uses of the ternary operator in sound/soc and 68776 in my 
local kernel branch.
Can you clarify in what way this is an abuse? I don't mind changing 
this, I wasn't aware this is frowned upon.

Re: [PATCH 5/5] ASoC: Intel: sof_sdw: clarify operator precedence

[Mark Brown]

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

On Thu, Aug 13, 2020 at 02:43:50PM -0500, Pierre-Louis Bossart wrote:
> On 8/13/20 1:45 PM, Mark Brown wrote:
> > On Thu, Aug 13, 2020 at 12:58:39PM -0500, Pierre-Louis Bossart wrote:

> > > -	hdmi_num = sof_sdw_quirk & SOF_SDW_TGL_HDMI ?
> > > +	hdmi_num = (sof_sdw_quirk & SOF_SDW_TGL_HDMI) ?
> > >   				SOF_TGL_HDMI_COUNT : SOF_PRE_TGL_HDMI_COUNT;

> > Or better yet, just don't abuse the ternery operator like this and write
> > normal conditional statements.

> I count 795 uses of the ternary operator in sound/soc and 68776 in my local
> kernel branch.
> Can you clarify in what way this is an abuse? I don't mind changing this, I
> wasn't aware this is frowned upon.

If you write a normal conditional statement then not only is the
precedence clear but it's just generally easier to read.  There are
cases where it can help make things clearer (eg, avoiding the use of
scratch variables to hold results) but this is most definitely not one
of them and I don't understand everyone's enthusiasm for trying to put
them in.

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

Re: [PATCH 5/5] ASoC: Intel: sof_sdw: clarify operator precedence

[Pierre-Louis Bossart]

On 8/13/20 2:49 PM, Mark Brown wrote:
> On Thu, Aug 13, 2020 at 02:43:50PM -0500, Pierre-Louis Bossart wrote:
>> On 8/13/20 1:45 PM, Mark Brown wrote:
>>> On Thu, Aug 13, 2020 at 12:58:39PM -0500, Pierre-Louis Bossart wrote:
> 
>>>> -	hdmi_num = sof_sdw_quirk & SOF_SDW_TGL_HDMI ?
>>>> +	hdmi_num = (sof_sdw_quirk & SOF_SDW_TGL_HDMI) ?
>>>>    				SOF_TGL_HDMI_COUNT : SOF_PRE_TGL_HDMI_COUNT;
> 
>>> Or better yet, just don't abuse the ternery operator like this and write
>>> normal conditional statements.
> 
>> I count 795 uses of the ternary operator in sound/soc and 68776 in my local
>> kernel branch.
>> Can you clarify in what way this is an abuse? I don't mind changing this, I
>> wasn't aware this is frowned upon.
> 
> If you write a normal conditional statement then not only is the
> precedence clear but it's just generally easier to read.  There are
> cases where it can help make things clearer (eg, avoiding the use of
> scratch variables to hold results) but this is most definitely not one
> of them and I don't understand everyone's enthusiasm for trying to put
> them in.

That's fair, I am not a big fan either.
Please drop this patch and we'll rework this machine driver. There's a 
set of updates planned anyways and we can add this cleanup in a separate 
set. Thanks!

Re: [PATCH 0/5] ASoC: SOF/Intel: fix cppcheck warnings

[Mark Brown]

On Thu, 13 Aug 2020 12:58:34 -0500, Pierre-Louis Bossart wrote:
> A small set of fixes to reduce the number of warnings.
> 
> Pierre-Louis Bossart (5):
>   ASOC: SOF: Intel: hda-codec: move unused label to correct position
>   ASoC: SOF: Intel: hda-codec: move variable used conditionally
>   ASoC: Intel: rename shadowed variable for all broadwell boards
>   ASoC: Intel: bytcht_cx2072x: simplify return handling
>   ASoC: Intel: sof_sdw: clarify operator precedence
> 
> [...]

Applied to

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

Thanks!

[1/5] ASOC: SOF: Intel: hda-codec: move unused label to correct position
      commit: 11ec0edc6408a739dffca34ebbbe921817c3b10e
[2/5] ASoC: SOF: Intel: hda-codec: move variable used conditionally
      commit: 2e3e0bc378f205370fc4c6dbd9374d66e803ce53
[3/5] ASoC: Intel: rename shadowed variable for all broadwell boards
      commit: 1e6444271c667d56f3a793cfc295b72a1f8007da
[4/5] ASoC: Intel: bytcht_cx2072x: simplify return handling
      commit: 9c7deb0576d7fe4370a23f4e127b2a69325f7ce9
[5/5] ASoC: Intel: sof_sdw: clarify operator precedence
      (no commit info)

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