[PATCH] ASoC: Intel: bytcr_5640.c:Refactored if statement and removed buffer

[PATCH] ASoC: Intel: bytcr_5640.c:Refactored if statement and removed buffer

[Nariman]

From: Nariman Etemadi <narimantos@gmail.com>

in function snd_byt_rt5640_mc_probe and removed buffer yt_rt5640_codec_aif_name & byt_rt5640_cpu_dai_name

Signed-off-by: Nariman Etemadi <narimantos@gmail.com>
---
 sound/soc/intel/boards/bytcr_rt5640.c | 26 ++++----------------------
 1 file changed, 4 insertions(+), 22 deletions(-)

diff --git a/sound/soc/intel/boards/bytcr_rt5640.c b/sound/soc/intel/boards/bytcr_rt5640.c
index 940eb27158da..0d91642ca287 100644
--- a/sound/soc/intel/boards/bytcr_rt5640.c
+++ b/sound/soc/intel/boards/bytcr_rt5640.c
@@ -1075,8 +1075,6 @@ static struct snd_soc_dai_link byt_rt5640_dais[] = {
 
 /* SoC card */
 static char byt_rt5640_codec_name[SND_ACPI_I2C_ID_LEN];
-static char byt_rt5640_codec_aif_name[12]; /*  = "rt5640-aif[1|2]" */
-static char byt_rt5640_cpu_dai_name[10]; /*  = "ssp[0|2]-port" */
 static char byt_rt5640_long_name[40]; /* = "bytcr-rt5640-*-spk-*-mic" */
 
 static int byt_rt5640_suspend(struct snd_soc_card *card)
@@ -1268,28 +1266,12 @@ static int snd_byt_rt5640_mc_probe(struct platform_device *pdev)
 	log_quirks(&pdev->dev);
 
 	if ((byt_rt5640_quirk & BYT_RT5640_SSP2_AIF2) ||
-	    (byt_rt5640_quirk & BYT_RT5640_SSP0_AIF2)) {
-
-		/* fixup codec aif name */
-		snprintf(byt_rt5640_codec_aif_name,
-			sizeof(byt_rt5640_codec_aif_name),
-			"%s", "rt5640-aif2");
-
-		byt_rt5640_dais[dai_index].codec_dai_name =
-			byt_rt5640_codec_aif_name;
-	}
+	    (byt_rt5640_quirk & BYT_RT5640_SSP0_AIF2))
+		byt_rt5640_dais[dai_index].codec_dai_name = "rt5640-aif2";
 
 	if ((byt_rt5640_quirk & BYT_RT5640_SSP0_AIF1) ||
-	    (byt_rt5640_quirk & BYT_RT5640_SSP0_AIF2)) {
-
-		/* fixup cpu dai name name */
-		snprintf(byt_rt5640_cpu_dai_name,
-			sizeof(byt_rt5640_cpu_dai_name),
-			"%s", "ssp0-port");
-
-		byt_rt5640_dais[dai_index].cpu_dai_name =
-			byt_rt5640_cpu_dai_name;
-	}
+	    (byt_rt5640_quirk & BYT_RT5640_SSP0_AIF2))
+		byt_rt5640_dais[dai_index].cpu_dai_name = "ssp0-port";
 
 	if (byt_rt5640_quirk & BYT_RT5640_MCLK_EN) {
 		priv->mclk = devm_clk_get(&pdev->dev, "pmc_plt_clk_3");
-- 
2.20.1

[PATCH] ASoC: Intel: cht_bsw_rt5645.c: Remove buffer and snprintf calls

[Nariman]

From: Damian van Soelen <dj.vsoelen@gmail.com>

The snprintf calls filling cht_rt5645_cpu_dai_name / cht_rt5645_codec_aif_name
always fill them with the same string ("ssp0-port" resp "rt5645-aif2") so
instead of keeping these buffers around and making the cpu_dai_name /
codec_aif_name point to this, simply update the foo_dai_name and foo_aif_name pointers to
directly point to a string constant containing the desired string.

Signed-off-by: Damian van Soelen <dj.vsoelen@gmail.com>
---
 sound/soc/intel/boards/cht_bsw_rt5645.c | 26 ++++---------------------
 1 file changed, 4 insertions(+), 22 deletions(-)

diff --git a/sound/soc/intel/boards/cht_bsw_rt5645.c b/sound/soc/intel/boards/cht_bsw_rt5645.c
index cbc2d458483f..b15459e56665 100644
--- a/sound/soc/intel/boards/cht_bsw_rt5645.c
+++ b/sound/soc/intel/boards/cht_bsw_rt5645.c
@@ -506,8 +506,6 @@ static struct cht_acpi_card snd_soc_cards[] = {
 };
 
 static char cht_rt5645_codec_name[SND_ACPI_I2C_ID_LEN];
-static char cht_rt5645_codec_aif_name[12]; /*  = "rt5645-aif[1|2]" */
-static char cht_rt5645_cpu_dai_name[10]; /*  = "ssp[0|2]-port" */
 
 static bool is_valleyview(void)
 {
@@ -641,28 +639,12 @@ static int snd_cht_mc_probe(struct platform_device *pdev)
 	log_quirks(&pdev->dev);
 
 	if ((cht_rt5645_quirk & CHT_RT5645_SSP2_AIF2) ||
-		(cht_rt5645_quirk & CHT_RT5645_SSP0_AIF2)) {
-
-		/* fixup codec aif name */
-		snprintf(cht_rt5645_codec_aif_name,
-			sizeof(cht_rt5645_codec_aif_name),
-			"%s", "rt5645-aif2");
-
-		cht_dailink[dai_index].codec_dai_name =
-			cht_rt5645_codec_aif_name;
-	}
+		(cht_rt5645_quirk & CHT_RT5645_SSP0_AIF2))
+			cht_dailink[dai_index].codec_dai_name = "rt5645-aif2";
 
 	if ((cht_rt5645_quirk & CHT_RT5645_SSP0_AIF1) ||
-		(cht_rt5645_quirk & CHT_RT5645_SSP0_AIF2)) {
-
-		/* fixup cpu dai name name */
-		snprintf(cht_rt5645_cpu_dai_name,
-			sizeof(cht_rt5645_cpu_dai_name),
-			"%s", "ssp0-port");
-
-		cht_dailink[dai_index].cpu_dai_name =
-			cht_rt5645_cpu_dai_name;
-	}
+		(cht_rt5645_quirk & CHT_RT5645_SSP0_AIF2))
+			cht_dailink[dai_index].cpu_dai_name = "ssp0-port";
 
 	/* override plaform name, if required */
 	platform_name = mach->mach_params.platform;
-- 
2.20.1

[PATCH] ASoC: Intel: bytcr_5640.c: refactored codec_fixup

[Nariman]

From: Erik Bussing <eabbussing@outlook.com>

Remove code duplication in byt_rt5640_codec_fixup

Signed-off-by: Erik Bussing <eabbussing@outlook.com>
---
 sound/soc/intel/boards/bytcr_rt5640.c | 64 ++++++++++-----------------
 1 file changed, 23 insertions(+), 41 deletions(-)

diff --git a/sound/soc/intel/boards/bytcr_rt5640.c b/sound/soc/intel/boards/bytcr_rt5640.c
index a22366ce33c4..679be55418bf 100644
--- a/sound/soc/intel/boards/bytcr_rt5640.c
+++ b/sound/soc/intel/boards/bytcr_rt5640.c
@@ -939,6 +939,7 @@ static int byt_rt5640_codec_fixup(struct snd_soc_pcm_runtime *rtd,
 	struct snd_interval *channels = hw_param_interval(params,
 						SNDRV_PCM_HW_PARAM_CHANNELS);
 	int ret;
+	int bits;
 
 	/* The DSP will covert the FE rate to 48k, stereo */
 	rate->min = rate->max = 48000;
@@ -949,53 +950,34 @@ static int byt_rt5640_codec_fixup(struct snd_soc_pcm_runtime *rtd,
 
 		/* set SSP0 to 16-bit */
 		params_set_format(params, SNDRV_PCM_FORMAT_S16_LE);
-
-		/*
-		 * Default mode for SSP configuration is TDM 4 slot, override config
-		 * with explicit setting to I2S 2ch 16-bit. The word length is set with
-		 * dai_set_tdm_slot() since there is no other API exposed
-		 */
-		ret = snd_soc_dai_set_fmt(rtd->cpu_dai,
-					SND_SOC_DAIFMT_I2S     |
-					SND_SOC_DAIFMT_NB_NF   |
-					SND_SOC_DAIFMT_CBS_CFS
-			);
-		if (ret < 0) {
-			dev_err(rtd->dev, "can't set format to I2S, err %d\n", ret);
-			return ret;
-		}
-
-		ret = snd_soc_dai_set_tdm_slot(rtd->cpu_dai, 0x3, 0x3, 2, 16);
-		if (ret < 0) {
-			dev_err(rtd->dev, "can't set I2S config, err %d\n", ret);
-			return ret;
-		}
-
+	bits = 16;
 	} else {
 
 		/* set SSP2 to 24-bit */
 		params_set_format(params, SNDRV_PCM_FORMAT_S24_LE);
+		bits = 24;
+	}
 
-		/*
-		 * Default mode for SSP configuration is TDM 4 slot, override config
-		 * with explicit setting to I2S 2ch 24-bit. The word length is set with
-		 * dai_set_tdm_slot() since there is no other API exposed
-		 */
-		ret = snd_soc_dai_set_fmt(rtd->cpu_dai,
-					SND_SOC_DAIFMT_I2S     |
-					SND_SOC_DAIFMT_NB_NF   |
-					SND_SOC_DAIFMT_CBS_CFS
-			);
-		if (ret < 0) {
-			dev_err(rtd->dev, "can't set format to I2S, err %d\n", ret);
-			return ret;
-		}
 
-		ret = snd_soc_dai_set_tdm_slot(rtd->cpu_dai, 0x3, 0x3, 2, 24);
-		if (ret < 0) {
-			dev_err(rtd->dev, "can't set I2S config, err %d\n", ret);
-			return ret;
-		}
+	/*
+	 * Default mode for SSP configuration is TDM 4 slot, override config
+	 * with explicit setting to I2S 2ch 24-bit. The word length is set with
+	 * dai_set_tdm_slot() since there is no other API exposed
+	 */
+	ret = snd_soc_dai_set_fmt(rtd->cpu_dai,
+		SND_SOC_DAIFMT_I2S     |
+		SND_SOC_DAIFMT_NB_NF   |
+		SND_SOC_DAIFMT_CBS_CFS
+	);
+	if (ret < 0) {
+		dev_err(rtd->dev, "can't set format to I2S, err %d\n", ret);
+		return ret;
+	}
+
+	ret = snd_soc_dai_set_tdm_slot(rtd->cpu_dai, 0x3, 0x3, 2, bits);
+	if (ret < 0) {
+		dev_err(rtd->dev, "can't set I2S config, err %d\n", ret);
+		return ret;
 	}
 	return 0;
 }
-- 
2.20.1

[PATCH] ASoC: Intel: bytcr_rt5651.c: remove string buffers 'byt_rt5651_cpu_dai_name' and 'byt_rt5651_cpu_dai_name'

[Nariman]

From: Jordy Ubink <jordyubink@hotmail.nl>

The snprintf calls filling byt_rt5651_cpu_dai_name / byt_rt5651_cpu_dai_name always fill them with the same string (ssp0-port" resp "rt5651-aif2"). So instead of keeping these buffers around and making the cpu_dai_name / codec_dai_name point to this, simply update the foo_dai_name pointers to directly point to a string constant containing the desired string.

Signed-off-by: Jordy Ubink <jordyubink@hotmail.nl>
---
 sound/soc/intel/boards/bytcr_rt5651.c | 24 ++++--------------------
 1 file changed, 4 insertions(+), 20 deletions(-)

diff --git a/sound/soc/intel/boards/bytcr_rt5651.c b/sound/soc/intel/boards/bytcr_rt5651.c
index e528995668b7..2e1bf43820d8 100644
--- a/sound/soc/intel/boards/bytcr_rt5651.c
+++ b/sound/soc/intel/boards/bytcr_rt5651.c
@@ -730,8 +730,6 @@ static struct snd_soc_dai_link byt_rt5651_dais[] = {
 
 /* SoC card */
 static char byt_rt5651_codec_name[SND_ACPI_I2C_ID_LEN];
-static char byt_rt5651_codec_aif_name[12]; /*  = "rt5651-aif[1|2]" */
-static char byt_rt5651_cpu_dai_name[10]; /*  = "ssp[0|2]-port" */
 static char byt_rt5651_long_name[50]; /* = "bytcr-rt5651-*-spk-*-mic[-swapped-hp]" */
 
 static int byt_rt5651_suspend(struct snd_soc_card *card)
@@ -1009,26 +1007,12 @@ static int snd_byt_rt5651_mc_probe(struct platform_device *pdev)
 	log_quirks(&pdev->dev);
 
 	if ((byt_rt5651_quirk & BYT_RT5651_SSP2_AIF2) ||
-	    (byt_rt5651_quirk & BYT_RT5651_SSP0_AIF2)) {
-		/* fixup codec aif name */
-		snprintf(byt_rt5651_codec_aif_name,
-			sizeof(byt_rt5651_codec_aif_name),
-			"%s", "rt5651-aif2");
-
-		byt_rt5651_dais[dai_index].codec_dai_name =
-			byt_rt5651_codec_aif_name;
-	}
+	    (byt_rt5651_quirk & BYT_RT5651_SSP0_AIF2))
+		byt_rt5651_dais[dai_index].codec_dai_name = "rt5651-aif2";
 
 	if ((byt_rt5651_quirk & BYT_RT5651_SSP0_AIF1) ||
-	    (byt_rt5651_quirk & BYT_RT5651_SSP0_AIF2)) {
-		/* fixup cpu dai name name */
-		snprintf(byt_rt5651_cpu_dai_name,
-			sizeof(byt_rt5651_cpu_dai_name),
-			"%s", "ssp0-port");
-
-		byt_rt5651_dais[dai_index].cpu_dai_name =
-			byt_rt5651_cpu_dai_name;
-	}
+	    (byt_rt5651_quirk & BYT_RT5651_SSP0_AIF2))
+		byt_rt5651_dais[dai_index].cpu_dai_name = "ssp0-port";
 
 	if (byt_rt5651_quirk & BYT_RT5651_MCLK_EN) {
 		priv->mclk = devm_clk_get(&pdev->dev, "pmc_plt_clk_3");
-- 
2.20.1

Re: [PATCH] ASoC: Intel: bytcr_rt5651.c: remove string buffers 'byt_rt5651_cpu_dai_name' and 'byt_rt5651_cpu_dai_name'

[Takashi Iwai]

On Sat, 04 May 2019 17:16:52 +0200,
Nariman wrote:
> 
> From: Jordy Ubink <jordyubink@hotmail.nl>
> 
> The snprintf calls filling byt_rt5651_cpu_dai_name / byt_rt5651_cpu_dai_name always fill them with the same string (ssp0-port" resp "rt5651-aif2"). So instead of keeping these buffers around and making the cpu_dai_name / codec_dai_name point to this, simply update the foo_dai_name pointers to directly point to a string constant containing the desired string.
> 
> Signed-off-by: Jordy Ubink <jordyubink@hotmail.nl>

If you submit a patch, please give your own sign-off as well as the
author's one, even if the patch is not written by you.  This is a
legal requirement.


thanks,

Takashi

Re: [alsa-devel] [PATCH] ASoC: Intel: bytcr_5640.c:Refactored if statement and removed buffer

[Pierre-Louis Bossart]

>   static int byt_rt5640_suspend(struct snd_soc_card *card)
> @@ -1268,28 +1266,12 @@ static int snd_byt_rt5640_mc_probe(struct platform_device *pdev)
>   	log_quirks(&pdev->dev);
>   
>   	if ((byt_rt5640_quirk & BYT_RT5640_SSP2_AIF2) ||
> -	    (byt_rt5640_quirk & BYT_RT5640_SSP0_AIF2)) {
> -
> -		/* fixup codec aif name */
> -		snprintf(byt_rt5640_codec_aif_name,
> -			sizeof(byt_rt5640_codec_aif_name),
> -			"%s", "rt5640-aif2");
> -
> -		byt_rt5640_dais[dai_index].codec_dai_name =
> -			byt_rt5640_codec_aif_name;
> -	}
> +	    (byt_rt5640_quirk & BYT_RT5640_SSP0_AIF2))
> +		byt_rt5640_dais[dai_index].codec_dai_name = "rt5640-aif2";

This is not equivalent, you don't deal with the (byt_rt5640_quirk & 
BYT_RT5640_SSP2_AIF2) case. The default is SSP_AIF1

>   
>   	if ((byt_rt5640_quirk & BYT_RT5640_SSP0_AIF1) ||
> -	    (byt_rt5640_quirk & BYT_RT5640_SSP0_AIF2)) {
> -
> -		/* fixup cpu dai name name */
> -		snprintf(byt_rt5640_cpu_dai_name,
> -			sizeof(byt_rt5640_cpu_dai_name),
> -			"%s", "ssp0-port");
> -
> -		byt_rt5640_dais[dai_index].cpu_dai_name =
> -			byt_rt5640_cpu_dai_name;
> -	}
> +	    (byt_rt5640_quirk & BYT_RT5640_SSP0_AIF2))
> +		byt_rt5640_dais[dai_index].cpu_dai_name = "ssp0-port";

Same here, this is not equivalent. the SSP0_AIF1 case is not handled.
it's fine to remove the intermediate buffers, but you can't remove 
support for 2 out of the 4 combinations supported.

Re: [alsa-devel] [PATCH] ASoC: Intel: cht_bsw_rt5645.c: Remove buffer and snprintf calls

[Pierre-Louis Bossart]

On 5/4/19 10:16 AM, Nariman wrote:
> From: Damian van Soelen <dj.vsoelen@gmail.com>
> 
> The snprintf calls filling cht_rt5645_cpu_dai_name / cht_rt5645_codec_aif_name
> always fill them with the same string ("ssp0-port" resp "rt5645-aif2") so
> instead of keeping these buffers around and making the cpu_dai_name /
> codec_aif_name point to this, simply update the foo_dai_name and foo_aif_name pointers to
> directly point to a string constant containing the desired string.
> 
> Signed-off-by: Damian van Soelen <dj.vsoelen@gmail.com>

Need Nariman's Signoff-of-by tag here.

> ---
>   sound/soc/intel/boards/cht_bsw_rt5645.c | 26 ++++---------------------
>   1 file changed, 4 insertions(+), 22 deletions(-)
> 
> diff --git a/sound/soc/intel/boards/cht_bsw_rt5645.c b/sound/soc/intel/boards/cht_bsw_rt5645.c
> index cbc2d458483f..b15459e56665 100644
> --- a/sound/soc/intel/boards/cht_bsw_rt5645.c
> +++ b/sound/soc/intel/boards/cht_bsw_rt5645.c
> @@ -506,8 +506,6 @@ static struct cht_acpi_card snd_soc_cards[] = {
>   };
>   
>   static char cht_rt5645_codec_name[SND_ACPI_I2C_ID_LEN];
> -static char cht_rt5645_codec_aif_name[12]; /*  = "rt5645-aif[1|2]" */
> -static char cht_rt5645_cpu_dai_name[10]; /*  = "ssp[0|2]-port" */
>   
>   static bool is_valleyview(void)
>   {
> @@ -641,28 +639,12 @@ static int snd_cht_mc_probe(struct platform_device *pdev)
>   	log_quirks(&pdev->dev);
>   
>   	if ((cht_rt5645_quirk & CHT_RT5645_SSP2_AIF2) ||
> -		(cht_rt5645_quirk & CHT_RT5645_SSP0_AIF2)) {
> -
> -		/* fixup codec aif name */
> -		snprintf(cht_rt5645_codec_aif_name,
> -			sizeof(cht_rt5645_codec_aif_name),
> -			"%s", "rt5645-aif2");
> -
> -		cht_dailink[dai_index].codec_dai_name =
> -			cht_rt5645_codec_aif_name;
> -	}
> +		(cht_rt5645_quirk & CHT_RT5645_SSP0_AIF2))
> +			cht_dailink[dai_index].codec_dai_name = "rt5645-aif2";

same, not equivalent. SSP2_AIF2 is not handled.

>   
>   	if ((cht_rt5645_quirk & CHT_RT5645_SSP0_AIF1) ||
> -		(cht_rt5645_quirk & CHT_RT5645_SSP0_AIF2)) {
> -
> -		/* fixup cpu dai name name */
> -		snprintf(cht_rt5645_cpu_dai_name,
> -			sizeof(cht_rt5645_cpu_dai_name),
> -			"%s", "ssp0-port");
> -
> -		cht_dailink[dai_index].cpu_dai_name =
> -			cht_rt5645_cpu_dai_name;
> -	}
> +		(cht_rt5645_quirk & CHT_RT5645_SSP0_AIF2))
> +			cht_dailink[dai_index].cpu_dai_name = "ssp0-port";

and same here, SSP0_AIF1 will no longer work.

>   
>   	/* override plaform name, if required */
>   	platform_name = mach->mach_params.platform;
> 

Re: [PATCH] ASoC: Intel: bytcr_rt5651.c: remove string buffers 'byt_rt5651_cpu_dai_name' and 'byt_rt5651_cpu_dai_name'

[Pierre-Louis Bossart]

On 5/4/19 10:16 AM, Nariman wrote:
> From: Jordy Ubink <jordyubink@hotmail.nl>
> 
> The snprintf calls filling byt_rt5651_cpu_dai_name / byt_rt5651_cpu_dai_name always fill them with the same string (ssp0-port" resp "rt5651-aif2"). So instead of keeping these buffers around and making the cpu_dai_name / codec_dai_name point to this, simply update the foo_dai_name pointers to directly point to a string constant containing the desired string.
> 
> Signed-off-by: Jordy Ubink <jordyubink@hotmail.nl>
> ---
>   sound/soc/intel/boards/bytcr_rt5651.c | 24 ++++--------------------
>   1 file changed, 4 insertions(+), 20 deletions(-)
> 
> diff --git a/sound/soc/intel/boards/bytcr_rt5651.c b/sound/soc/intel/boards/bytcr_rt5651.c
> index e528995668b7..2e1bf43820d8 100644
> --- a/sound/soc/intel/boards/bytcr_rt5651.c
> +++ b/sound/soc/intel/boards/bytcr_rt5651.c
> @@ -730,8 +730,6 @@ static struct snd_soc_dai_link byt_rt5651_dais[] = {
>   
>   /* SoC card */
>   static char byt_rt5651_codec_name[SND_ACPI_I2C_ID_LEN];
> -static char byt_rt5651_codec_aif_name[12]; /*  = "rt5651-aif[1|2]" */
> -static char byt_rt5651_cpu_dai_name[10]; /*  = "ssp[0|2]-port" */
>   static char byt_rt5651_long_name[50]; /* = "bytcr-rt5651-*-spk-*-mic[-swapped-hp]" */
>   
>   static int byt_rt5651_suspend(struct snd_soc_card *card)
> @@ -1009,26 +1007,12 @@ static int snd_byt_rt5651_mc_probe(struct platform_device *pdev)
>   	log_quirks(&pdev->dev);
>   
>   	if ((byt_rt5651_quirk & BYT_RT5651_SSP2_AIF2) ||
> -	    (byt_rt5651_quirk & BYT_RT5651_SSP0_AIF2)) {
> -		/* fixup codec aif name */
> -		snprintf(byt_rt5651_codec_aif_name,
> -			sizeof(byt_rt5651_codec_aif_name),
> -			"%s", "rt5651-aif2");
> -
> -		byt_rt5651_dais[dai_index].codec_dai_name =
> -			byt_rt5651_codec_aif_name;
> -	}
> +	    (byt_rt5651_quirk & BYT_RT5651_SSP0_AIF2))
> +		byt_rt5651_dais[dai_index].codec_dai_name = "rt5651-aif2";
>   
>   	if ((byt_rt5651_quirk & BYT_RT5651_SSP0_AIF1) ||
> -	    (byt_rt5651_quirk & BYT_RT5651_SSP0_AIF2)) {
> -		/* fixup cpu dai name name */
> -		snprintf(byt_rt5651_cpu_dai_name,
> -			sizeof(byt_rt5651_cpu_dai_name),
> -			"%s", "ssp0-port");
> -
> -		byt_rt5651_dais[dai_index].cpu_dai_name =
> -			byt_rt5651_cpu_dai_name;
> -	}
> +	    (byt_rt5651_quirk & BYT_RT5651_SSP0_AIF2))
> +		byt_rt5651_dais[dai_index].cpu_dai_name = "ssp0-port";

same issues with Signed-off-by and missing quirks.

>   
>   	if (byt_rt5651_quirk & BYT_RT5651_MCLK_EN) {
>   		priv->mclk = devm_clk_get(&pdev->dev, "pmc_plt_clk_3");
> 

Re: [PATCH] ASoC: Intel: bytcr_rt5651.c: remove string buffers 'byt_rt5651_cpu_dai_name' and 'byt_rt5651_cpu_dai_name'

[Hans de Goede]

Hi,

On 05-05-19 09:51, Takashi Iwai wrote:
> On Sat, 04 May 2019 17:16:52 +0200,
> Nariman wrote:
>>
>> From: Jordy Ubink <jordyubink@hotmail.nl>
>>
>> The snprintf calls filling byt_rt5651_cpu_dai_name / byt_rt5651_cpu_dai_name always fill them with the same string (ssp0-port" resp "rt5651-aif2"). So instead of keeping these buffers around and making the cpu_dai_name / codec_dai_name point to this, simply update the foo_dai_name pointers to directly point to a string constant containing the desired string.
>>
>> Signed-off-by: Jordy Ubink <jordyubink@hotmail.nl>
> 
> If you submit a patch, please give your own sign-off as well as the
> author's one, even if the patch is not written by you.  This is a
> legal requirement.

Sorry, that is my bad, Nariman and the author authors of the patches
are a group of students doing some kernel work for me and this is
a warm-up assignment for them to get used to the kernel development
process.

I forgot to point out to Nariman that since he is sending
out the entire series for all 4 of them, he needs to add his
S-o-b.

Regards,

Hans

Re: [alsa-devel] [PATCH] ASoC: Intel: bytcr_5640.c:Refactored if statement and removed buffer

[Hans de Goede]

Hi Pierre-Louis,

Nariman and the author authors of these patches are a group of students doing
some kernel work for me and this is a warm-up assignment for them to get used
to the kernel development process.

On 06-05-19 17:21, Pierre-Louis Bossart wrote:
> 
>>   static int byt_rt5640_suspend(struct snd_soc_card *card)
>> @@ -1268,28 +1266,12 @@ static int snd_byt_rt5640_mc_probe(struct platform_device *pdev)
>>       log_quirks(&pdev->dev);
>>       if ((byt_rt5640_quirk & BYT_RT5640_SSP2_AIF2) ||
>> -        (byt_rt5640_quirk & BYT_RT5640_SSP0_AIF2)) {
>> -
>> -        /* fixup codec aif name */
>> -        snprintf(byt_rt5640_codec_aif_name,
>> -            sizeof(byt_rt5640_codec_aif_name),
>> -            "%s", "rt5640-aif2");
>> -
>> -        byt_rt5640_dais[dai_index].codec_dai_name =
>> -            byt_rt5640_codec_aif_name;
>> -    }
>> +        (byt_rt5640_quirk & BYT_RT5640_SSP0_AIF2))
>> +        byt_rt5640_dais[dai_index].codec_dai_name = "rt5640-aif2";
> 
> This is not equivalent, you don't deal with the (byt_rt5640_quirk & BYT_RT5640_SSP2_AIF2) case. The default is SSP_AIF1

I might be mistaken here, but look closer, the original:
	if ((byt_rt5640_quirk & BYT_RT5640_SSP2_AIF2) ||

Line is kept, so the new code block is:

	if ((byt_rt5640_quirk & BYT_RT5640_SSP2_AIF2) ||
	    (byt_rt5640_quirk & BYT_RT5640_SSP0_AIF2))
		byt_rt5640_dais[dai_index].codec_dai_name = "rt5640-aif2";

Which does take the BYT_RT5640_SSP2_AIF2 into account.

>>       if ((byt_rt5640_quirk & BYT_RT5640_SSP0_AIF1) ||
>> -        (byt_rt5640_quirk & BYT_RT5640_SSP0_AIF2)) {
>> -
>> -        /* fixup cpu dai name name */
>> -        snprintf(byt_rt5640_cpu_dai_name,
>> -            sizeof(byt_rt5640_cpu_dai_name),
>> -            "%s", "ssp0-port");
>> -
>> -        byt_rt5640_dais[dai_index].cpu_dai_name =
>> -            byt_rt5640_cpu_dai_name;
>> -    }
>> +        (byt_rt5640_quirk & BYT_RT5640_SSP0_AIF2))
>> +        byt_rt5640_dais[dai_index].cpu_dai_name = "ssp0-port";
> 
> Same here, this is not equivalent. the SSP0_AIF1 case is not handled.
> it's fine to remove the intermediate buffers, but you can't remove support for 2 out of the 4 combinations supported.

Same remark here from me too :)

Regards,

Hans

Re: [alsa-devel] [PATCH] ASoC: Intel: cht_bsw_rt5645.c: Remove buffer and snprintf calls

[Hans de Goede]

Hi,

On 06-05-19 17:24, Pierre-Louis Bossart wrote:
> On 5/4/19 10:16 AM, Nariman wrote:
>> From: Damian van Soelen <dj.vsoelen@gmail.com>
>>
>> The snprintf calls filling cht_rt5645_cpu_dai_name / cht_rt5645_codec_aif_name
>> always fill them with the same string ("ssp0-port" resp "rt5645-aif2") so
>> instead of keeping these buffers around and making the cpu_dai_name /
>> codec_aif_name point to this, simply update the foo_dai_name and foo_aif_name pointers to
>> directly point to a string constant containing the desired string.
>>
>> Signed-off-by: Damian van Soelen <dj.vsoelen@gmail.com>
> 
> Need Nariman's Signoff-of-by tag here.

Yes, Nariman can you resend with your S-o-b added once the other part
of this discussion is resolved?

Note you need to pass

--subject-prefix="PATCH v2"

To git send-email when sending out the new version with your S-o-b added
to the 3 patches which you are not the author of.




>> ---
>>   sound/soc/intel/boards/cht_bsw_rt5645.c | 26 ++++---------------------
>>   1 file changed, 4 insertions(+), 22 deletions(-)
>>
>> diff --git a/sound/soc/intel/boards/cht_bsw_rt5645.c b/sound/soc/intel/boards/cht_bsw_rt5645.c
>> index cbc2d458483f..b15459e56665 100644
>> --- a/sound/soc/intel/boards/cht_bsw_rt5645.c
>> +++ b/sound/soc/intel/boards/cht_bsw_rt5645.c
>> @@ -506,8 +506,6 @@ static struct cht_acpi_card snd_soc_cards[] = {
>>   };
>>   static char cht_rt5645_codec_name[SND_ACPI_I2C_ID_LEN];
>> -static char cht_rt5645_codec_aif_name[12]; /*  = "rt5645-aif[1|2]" */
>> -static char cht_rt5645_cpu_dai_name[10]; /*  = "ssp[0|2]-port" */
>>   static bool is_valleyview(void)
>>   {
>> @@ -641,28 +639,12 @@ static int snd_cht_mc_probe(struct platform_device *pdev)
>>       log_quirks(&pdev->dev);
>>       if ((cht_rt5645_quirk & CHT_RT5645_SSP2_AIF2) ||
>> -        (cht_rt5645_quirk & CHT_RT5645_SSP0_AIF2)) {
>> -
>> -        /* fixup codec aif name */
>> -        snprintf(cht_rt5645_codec_aif_name,
>> -            sizeof(cht_rt5645_codec_aif_name),
>> -            "%s", "rt5645-aif2");
>> -
>> -        cht_dailink[dai_index].codec_dai_name =
>> -            cht_rt5645_codec_aif_name;
>> -    }
>> +        (cht_rt5645_quirk & CHT_RT5645_SSP0_AIF2))
>> +            cht_dailink[dai_index].codec_dai_name = "rt5645-aif2";
> 
> same, not equivalent. SSP2_AIF2 is not handled.

Same remark from me, and also the same for the 3th patch you've
responded to.

Regards,

Hans


> 
>>       if ((cht_rt5645_quirk & CHT_RT5645_SSP0_AIF1) ||
>> -        (cht_rt5645_quirk & CHT_RT5645_SSP0_AIF2)) {
>> -
>> -        /* fixup cpu dai name name */
>> -        snprintf(cht_rt5645_cpu_dai_name,
>> -            sizeof(cht_rt5645_cpu_dai_name),
>> -            "%s", "ssp0-port");
>> -
>> -        cht_dailink[dai_index].cpu_dai_name =
>> -            cht_rt5645_cpu_dai_name;
>> -    }
>> +        (cht_rt5645_quirk & CHT_RT5645_SSP0_AIF2))
>> +            cht_dailink[dai_index].cpu_dai_name = "ssp0-port";
> 
> and same here, SSP0_AIF1 will no longer work.
> 
>>       /* override plaform name, if required */
>>       platform_name = mach->mach_params.platform;
>>
> 

Re: [alsa-devel] [PATCH] ASoC: Intel: bytcr_5640.c:Refactored if statement and removed buffer

[Pierre-Louis Bossart]

On 5/6/19 10:43 AM, Hans de Goede wrote:
> Hi Pierre-Louis,
> 
> Nariman and the author authors of these patches are a group of students 
> doing
> some kernel work for me and this is a warm-up assignment for them to get 
> used
> to the kernel development process.
> 
> On 06-05-19 17:21, Pierre-Louis Bossart wrote:
>>
>>>   static int byt_rt5640_suspend(struct snd_soc_card *card)
>>> @@ -1268,28 +1266,12 @@ static int snd_byt_rt5640_mc_probe(struct 
>>> platform_device *pdev)
>>>       log_quirks(&pdev->dev);
>>>       if ((byt_rt5640_quirk & BYT_RT5640_SSP2_AIF2) ||
>>> -        (byt_rt5640_quirk & BYT_RT5640_SSP0_AIF2)) {
>>> -
>>> -        /* fixup codec aif name */
>>> -        snprintf(byt_rt5640_codec_aif_name,
>>> -            sizeof(byt_rt5640_codec_aif_name),
>>> -            "%s", "rt5640-aif2");
>>> -
>>> -        byt_rt5640_dais[dai_index].codec_dai_name =
>>> -            byt_rt5640_codec_aif_name;
>>> -    }
>>> +        (byt_rt5640_quirk & BYT_RT5640_SSP0_AIF2))
>>> +        byt_rt5640_dais[dai_index].codec_dai_name = "rt5640-aif2";
>>
>> This is not equivalent, you don't deal with the (byt_rt5640_quirk & 
>> BYT_RT5640_SSP2_AIF2) case. The default is SSP_AIF1
> 
> I might be mistaken here, but look closer, the original:
>      if ((byt_rt5640_quirk & BYT_RT5640_SSP2_AIF2) ||
> 
> Line is kept, so the new code block is:
> 
>      if ((byt_rt5640_quirk & BYT_RT5640_SSP2_AIF2) ||
>          (byt_rt5640_quirk & BYT_RT5640_SSP0_AIF2))
>          byt_rt5640_dais[dai_index].codec_dai_name = "rt5640-aif2";
> 
> Which does take the BYT_RT5640_SSP2_AIF2 into account.

Ah yes, my mistake. Looks good then.

> 
>>>       if ((byt_rt5640_quirk & BYT_RT5640_SSP0_AIF1) ||
>>> -        (byt_rt5640_quirk & BYT_RT5640_SSP0_AIF2)) {
>>> -
>>> -        /* fixup cpu dai name name */
>>> -        snprintf(byt_rt5640_cpu_dai_name,
>>> -            sizeof(byt_rt5640_cpu_dai_name),
>>> -            "%s", "ssp0-port");
>>> -
>>> -        byt_rt5640_dais[dai_index].cpu_dai_name =
>>> -            byt_rt5640_cpu_dai_name;
>>> -    }
>>> +        (byt_rt5640_quirk & BYT_RT5640_SSP0_AIF2))
>>> +        byt_rt5640_dais[dai_index].cpu_dai_name = "ssp0-port";
>>
>> Same here, this is not equivalent. the SSP0_AIF1 case is not handled.
>> it's fine to remove the intermediate buffers, but you can't remove 
>> support for 2 out of the 4 combinations supported.
> 
> Same remark here from me too :)
> 
> Regards,
> 
> Hans
> 
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] ASoC: Intel: bytcr_rt5651.c: remove string buffers 'byt_rt5651_cpu_dai_name' and 'byt_rt5651_cpu_dai_name'

[Pierre-Louis Bossart]

On 5/6/19 10:40 AM, Hans de Goede wrote:
> Hi,
> 
> On 05-05-19 09:51, Takashi Iwai wrote:
>> On Sat, 04 May 2019 17:16:52 +0200,
>> Nariman wrote:
>>>
>>> From: Jordy Ubink <jordyubink@hotmail.nl>
>>>
>>> The snprintf calls filling byt_rt5651_cpu_dai_name / 
>>> byt_rt5651_cpu_dai_name always fill them with the same string 
>>> (ssp0-port" resp "rt5651-aif2"). So instead of keeping these buffers 
>>> around and making the cpu_dai_name / codec_dai_name point to this, 
>>> simply update the foo_dai_name pointers to directly point to a string 
>>> constant containing the desired string.
>>>
>>> Signed-off-by: Jordy Ubink <jordyubink@hotmail.nl>
>>
>> If you submit a patch, please give your own sign-off as well as the
>> author's one, even if the patch is not written by you.  This is a
>> legal requirement.
> 
> Sorry, that is my bad, Nariman and the author authors of the patches
> are a group of students doing some kernel work for me and this is
> a warm-up assignment for them to get used to the kernel development
> process.
> 
> I forgot to point out to Nariman that since he is sending
> out the entire series for all 4 of them, he needs to add his
> S-o-b.

One suggestion for additional cleanups: can we try and remove all these 
static variables and move them to the context structure? It's been on my 
TODO list for a while.

Re: [alsa-devel] [PATCH] ASoC: Intel: bytcr_5640.c:Refactored if statement and removed buffer

[Pierre-Louis Bossart]

On 5/20/19 3:37 AM, Hans de Goede wrote:
> Hi all,
> 
> On 19-05-19 19:57, nariman wrote:
>> From: Nariman Etemadi <narimantos@gmail.com>
>>
>> in function snd_byt_rt5640_mc_probe and removed buffer 
>> yt_rt5640_codec_aif_name & byt_rt5640_cpu_dai_name
>>
>> Signed-off-by: Nariman Etemadi <narimantos@gmail.com>
> 
> Series (all 4 patches) look good to me:
> 
> Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Thanks for the update (I believe it's the v2, yes?)

the code is fine but can be polished to fix a couple of
alignment issues

+	bits = 16;
  	} else {

and checkpatch.pl --strict is quite verbose here. Some reports look like 
false-alarms but the commit message should be wrapped and alignement fixed.

---------------------------------------------------------
0001-ASoC-Intel-bytcr_5640.c-refactored-codec_fixup.patch
---------------------------------------------------------
CHECK: Alignment should match open parenthesis
#86: FILE: sound/soc/intel/boards/bytcr_rt5640.c:980:
+	ret = snd_soc_dai_set_fmt(rtd->cpu_dai,
+		SND_SOC_DAIFMT_I2S     |

total: 0 errors, 0 warnings, 1 checks, 82 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
       mechanically convert to the typical style using --fix or 
--fix-inplace.

0001-ASoC-Intel-bytcr_5640.c-refactored-codec_fixup.patch has style 
problems, please review.
---------------------------------------------------------------
0002-ASoC-Intel-bytcr_5640.c-Refactored-if-statement-and-.patch
---------------------------------------------------------------
WARNING: Possible unwrapped commit description (prefer a maximum 75 
chars per line)
#7:
in function snd_byt_rt5640_mc_probe and removed buffer 
yt_rt5640_codec_aif_name & byt_rt5640_cpu_dai_name

total: 0 errors, 1 warnings, 0 checks, 40 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
       mechanically convert to the typical style using --fix or 
--fix-inplace.

0002-ASoC-Intel-bytcr_5640.c-Refactored-if-statement-and-.patch has 
style problems, please review.
---------------------------------------------------------------
0003-ASoC-Intel-bytcr_rt5651.c-remove-string-buffers-byt_.patch
---------------------------------------------------------------
WARNING: Possible unwrapped commit description (prefer a maximum 75 
chars per line)
#7:
The snprintf calls filling byt_rt5651_cpu_dai_name / 
byt_rt5651_cpu_dai_name always fill them with the same string 
(ssp0-port" resp "rt5651-aif2"). So instead of keeping these buffers 
around and making the cpu_dai_name / codec_dai_name point to this, 
simply update the foo_dai_name pointers to directly point to a string 
constant containing the desired string.

total: 0 errors, 1 warnings, 0 checks, 38 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
       mechanically convert to the typical style using --fix or 
--fix-inplace.

0003-ASoC-Intel-bytcr_rt5651.c-remove-string-buffers-byt_.patch has 
style problems, please review.
---------------------------------------------------------------
0004-ASoC-Intel-cht_bsw_rt5645.c-Remove-buffer-and-snprin.patch
---------------------------------------------------------------
WARNING: Possible unwrapped commit description (prefer a maximum 75 
chars per line)
#7:
The snprintf calls filling cht_rt5645_cpu_dai_name / 
cht_rt5645_codec_aif_name

WARNING: suspect code indent for conditional statements (8, 24)
#35: FILE: sound/soc/intel/boards/cht_bsw_rt5645.c:642:
  	if ((cht_rt5645_quirk & CHT_RT5645_SSP2_AIF2) ||
[...]
+			cht_dailink[dai_index].codec_dai_name = "rt5645-aif2";

WARNING: suspect code indent for conditional statements (8, 24)
#49: FILE: sound/soc/intel/boards/cht_bsw_rt5645.c:646:
  	if ((cht_rt5645_quirk & CHT_RT5645_SSP0_AIF1) ||
[...]
+			cht_dailink[dai_index].cpu_dai_name = "ssp0-port";

total: 0 errors, 3 warnings, 0 checks, 40 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
       mechanically convert to the typical style using --fix or 
--fix-inplace.

0004-ASoC-Intel-cht_bsw_rt5645.c-Remove-buffer-and-snprin.patch has 
style problems, please review.



> 
> Regards,
> 
> Hans
> 
> 
>> ---
>>   sound/soc/intel/boards/bytcr_rt5640.c | 26 ++++----------------------
>>   1 file changed, 4 insertions(+), 22 deletions(-)
>>
>> diff --git a/sound/soc/intel/boards/bytcr_rt5640.c 
>> b/sound/soc/intel/boards/bytcr_rt5640.c
>> index 940eb27158da..0d91642ca287 100644
>> --- a/sound/soc/intel/boards/bytcr_rt5640.c
>> +++ b/sound/soc/intel/boards/bytcr_rt5640.c
>> @@ -1075,8 +1075,6 @@ static struct snd_soc_dai_link byt_rt5640_dais[] 
>> = {
>>   /* SoC card */
>>   static char byt_rt5640_codec_name[SND_ACPI_I2C_ID_LEN];
>> -static char byt_rt5640_codec_aif_name[12]; /*  = "rt5640-aif[1|2]" */
>> -static char byt_rt5640_cpu_dai_name[10]; /*  = "ssp[0|2]-port" */
>>   static char byt_rt5640_long_name[40]; /* = 
>> "bytcr-rt5640-*-spk-*-mic" */
>>   static int byt_rt5640_suspend(struct snd_soc_card *card)
>> @@ -1268,28 +1266,12 @@ static int snd_byt_rt5640_mc_probe(struct 
>> platform_device *pdev)
>>       log_quirks(&pdev->dev);
>>       if ((byt_rt5640_quirk & BYT_RT5640_SSP2_AIF2) ||
>> -        (byt_rt5640_quirk & BYT_RT5640_SSP0_AIF2)) {
>> -
>> -        /* fixup codec aif name */
>> -        snprintf(byt_rt5640_codec_aif_name,
>> -            sizeof(byt_rt5640_codec_aif_name),
>> -            "%s", "rt5640-aif2");
>> -
>> -        byt_rt5640_dais[dai_index].codec_dai_name =
>> -            byt_rt5640_codec_aif_name;
>> -    }
>> +        (byt_rt5640_quirk & BYT_RT5640_SSP0_AIF2))
>> +        byt_rt5640_dais[dai_index].codec_dai_name = "rt5640-aif2";
>>       if ((byt_rt5640_quirk & BYT_RT5640_SSP0_AIF1) ||
>> -        (byt_rt5640_quirk & BYT_RT5640_SSP0_AIF2)) {
>> -
>> -        /* fixup cpu dai name name */
>> -        snprintf(byt_rt5640_cpu_dai_name,
>> -            sizeof(byt_rt5640_cpu_dai_name),
>> -            "%s", "ssp0-port");
>> -
>> -        byt_rt5640_dais[dai_index].cpu_dai_name =
>> -            byt_rt5640_cpu_dai_name;
>> -    }
>> +        (byt_rt5640_quirk & BYT_RT5640_SSP0_AIF2))
>> +        byt_rt5640_dais[dai_index].cpu_dai_name = "ssp0-port";
>>       if (byt_rt5640_quirk & BYT_RT5640_MCLK_EN) {
>>           priv->mclk = devm_clk_get(&pdev->dev, "pmc_plt_clk_3");
>>
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> https://mailman.alsa-project.org/mailman/listinfo/alsa-devel