[PATCH 0/5] ASoC: SOF/Intel: fix cppcheck warnings
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(-)
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)
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
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.
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;
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);
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,
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);
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.
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.
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.
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!
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
participants (3)
-
Mark Brown -
Nathan Chancellor -
Pierre-Louis Bossart