* [PATCH] ASoC: Intel: cht_bsw_rt5645.c: Remove buffer and snprintf calls
2019-05-04 15:16 [PATCH] ASoC: Intel: bytcr_5640.c:Refactored if statement and removed buffer Nariman
@ 2019-05-04 15:16 ` Nariman
2019-05-06 15:24 ` [alsa-devel] " Pierre-Louis Bossart
2019-05-04 15:16 ` [PATCH] ASoC: Intel: bytcr_5640.c: refactored codec_fixup Nariman
` (2 subsequent siblings)
3 siblings, 1 reply; 14+ messages in thread
From: Nariman @ 2019-05-04 15:16 UTC (permalink / raw)
To: linux-kernel
Cc: hdegoede, pierre-louis.bossart, liam.r.girdwood, yang.jie,
broonie, perex, tiwai, alsa-devel, Damian van Soelen
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
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [alsa-devel] [PATCH] ASoC: Intel: cht_bsw_rt5645.c: Remove buffer and snprintf calls
2019-05-04 15:16 ` [PATCH] ASoC: Intel: cht_bsw_rt5645.c: Remove buffer and snprintf calls Nariman
@ 2019-05-06 15:24 ` Pierre-Louis Bossart
2019-05-06 15:48 ` Hans de Goede
0 siblings, 1 reply; 14+ messages in thread
From: Pierre-Louis Bossart @ 2019-05-06 15:24 UTC (permalink / raw)
To: Nariman, linux-kernel
Cc: alsa-devel, tiwai, yang.jie, liam.r.girdwood, hdegoede, broonie,
Damian van Soelen
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;
>
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [alsa-devel] [PATCH] ASoC: Intel: cht_bsw_rt5645.c: Remove buffer and snprintf calls
2019-05-06 15:24 ` [alsa-devel] " Pierre-Louis Bossart
@ 2019-05-06 15:48 ` Hans de Goede
0 siblings, 0 replies; 14+ messages in thread
From: Hans de Goede @ 2019-05-06 15:48 UTC (permalink / raw)
To: Pierre-Louis Bossart, Nariman, linux-kernel
Cc: alsa-devel, tiwai, yang.jie, liam.r.girdwood, broonie, Damian van Soelen
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;
>>
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH] ASoC: Intel: bytcr_5640.c: refactored codec_fixup
2019-05-04 15:16 [PATCH] ASoC: Intel: bytcr_5640.c:Refactored if statement and removed buffer Nariman
2019-05-04 15:16 ` [PATCH] ASoC: Intel: cht_bsw_rt5645.c: Remove buffer and snprintf calls Nariman
@ 2019-05-04 15:16 ` Nariman
2019-05-04 15:16 ` [PATCH] ASoC: Intel: bytcr_rt5651.c: remove string buffers 'byt_rt5651_cpu_dai_name' and 'byt_rt5651_cpu_dai_name' Nariman
2019-05-06 15:21 ` [alsa-devel] [PATCH] ASoC: Intel: bytcr_5640.c:Refactored if statement and removed buffer Pierre-Louis Bossart
3 siblings, 0 replies; 14+ messages in thread
From: Nariman @ 2019-05-04 15:16 UTC (permalink / raw)
To: linux-kernel
Cc: hdegoede, pierre-louis.bossart, liam.r.girdwood, yang.jie,
broonie, perex, tiwai, alsa-devel, Erik Bussing
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
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH] ASoC: Intel: bytcr_rt5651.c: remove string buffers 'byt_rt5651_cpu_dai_name' and 'byt_rt5651_cpu_dai_name'
2019-05-04 15:16 [PATCH] ASoC: Intel: bytcr_5640.c:Refactored if statement and removed buffer Nariman
2019-05-04 15:16 ` [PATCH] ASoC: Intel: cht_bsw_rt5645.c: Remove buffer and snprintf calls Nariman
2019-05-04 15:16 ` [PATCH] ASoC: Intel: bytcr_5640.c: refactored codec_fixup Nariman
@ 2019-05-04 15:16 ` Nariman
2019-05-05 7:51 ` Takashi Iwai
2019-05-06 15:24 ` Pierre-Louis Bossart
2019-05-06 15:21 ` [alsa-devel] [PATCH] ASoC: Intel: bytcr_5640.c:Refactored if statement and removed buffer Pierre-Louis Bossart
3 siblings, 2 replies; 14+ messages in thread
From: Nariman @ 2019-05-04 15:16 UTC (permalink / raw)
To: linux-kernel
Cc: hdegoede, pierre-louis.bossart, liam.r.girdwood, yang.jie,
broonie, perex, tiwai, alsa-devel, Jordy Ubink
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
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH] ASoC: Intel: bytcr_rt5651.c: remove string buffers 'byt_rt5651_cpu_dai_name' and 'byt_rt5651_cpu_dai_name'
2019-05-04 15:16 ` [PATCH] ASoC: Intel: bytcr_rt5651.c: remove string buffers 'byt_rt5651_cpu_dai_name' and 'byt_rt5651_cpu_dai_name' Nariman
@ 2019-05-05 7:51 ` Takashi Iwai
2019-05-06 15:40 ` Hans de Goede
2019-05-06 15:24 ` Pierre-Louis Bossart
1 sibling, 1 reply; 14+ messages in thread
From: Takashi Iwai @ 2019-05-05 7:51 UTC (permalink / raw)
To: Nariman
Cc: linux-kernel, alsa-devel, Jordy Ubink, broonie, liam.r.girdwood,
pierre-louis.bossart, yang.jie, perex, hdegoede
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] ASoC: Intel: bytcr_rt5651.c: remove string buffers 'byt_rt5651_cpu_dai_name' and 'byt_rt5651_cpu_dai_name'
2019-05-05 7:51 ` Takashi Iwai
@ 2019-05-06 15:40 ` Hans de Goede
2019-05-06 15:50 ` Pierre-Louis Bossart
0 siblings, 1 reply; 14+ messages in thread
From: Hans de Goede @ 2019-05-06 15:40 UTC (permalink / raw)
To: Takashi Iwai, Nariman
Cc: linux-kernel, alsa-devel, Jordy Ubink, broonie, liam.r.girdwood,
pierre-louis.bossart, yang.jie, perex
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] ASoC: Intel: bytcr_rt5651.c: remove string buffers 'byt_rt5651_cpu_dai_name' and 'byt_rt5651_cpu_dai_name'
2019-05-06 15:40 ` Hans de Goede
@ 2019-05-06 15:50 ` Pierre-Louis Bossart
0 siblings, 0 replies; 14+ messages in thread
From: Pierre-Louis Bossart @ 2019-05-06 15:50 UTC (permalink / raw)
To: Hans de Goede, Takashi Iwai, Nariman
Cc: linux-kernel, alsa-devel, Jordy Ubink, broonie, liam.r.girdwood,
yang.jie, perex
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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] ASoC: Intel: bytcr_rt5651.c: remove string buffers 'byt_rt5651_cpu_dai_name' and 'byt_rt5651_cpu_dai_name'
2019-05-04 15:16 ` [PATCH] ASoC: Intel: bytcr_rt5651.c: remove string buffers 'byt_rt5651_cpu_dai_name' and 'byt_rt5651_cpu_dai_name' Nariman
2019-05-05 7:51 ` Takashi Iwai
@ 2019-05-06 15:24 ` Pierre-Louis Bossart
1 sibling, 0 replies; 14+ messages in thread
From: Pierre-Louis Bossart @ 2019-05-06 15:24 UTC (permalink / raw)
To: Nariman, linux-kernel
Cc: hdegoede, liam.r.girdwood, yang.jie, broonie, perex, tiwai,
alsa-devel, Jordy Ubink
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");
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [alsa-devel] [PATCH] ASoC: Intel: bytcr_5640.c:Refactored if statement and removed buffer
2019-05-04 15:16 [PATCH] ASoC: Intel: bytcr_5640.c:Refactored if statement and removed buffer Nariman
` (2 preceding siblings ...)
2019-05-04 15:16 ` [PATCH] ASoC: Intel: bytcr_rt5651.c: remove string buffers 'byt_rt5651_cpu_dai_name' and 'byt_rt5651_cpu_dai_name' Nariman
@ 2019-05-06 15:21 ` Pierre-Louis Bossart
2019-05-06 15:43 ` Hans de Goede
3 siblings, 1 reply; 14+ messages in thread
From: Pierre-Louis Bossart @ 2019-05-06 15:21 UTC (permalink / raw)
To: Nariman, linux-kernel
Cc: alsa-devel, tiwai, yang.jie, liam.r.girdwood, hdegoede, broonie
> 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.
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [alsa-devel] [PATCH] ASoC: Intel: bytcr_5640.c:Refactored if statement and removed buffer
2019-05-06 15:21 ` [alsa-devel] [PATCH] ASoC: Intel: bytcr_5640.c:Refactored if statement and removed buffer Pierre-Louis Bossart
@ 2019-05-06 15:43 ` Hans de Goede
2019-05-06 15:50 ` Pierre-Louis Bossart
0 siblings, 1 reply; 14+ messages in thread
From: Hans de Goede @ 2019-05-06 15:43 UTC (permalink / raw)
To: Pierre-Louis Bossart, Nariman, linux-kernel
Cc: alsa-devel, tiwai, yang.jie, liam.r.girdwood, broonie
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
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [alsa-devel] [PATCH] ASoC: Intel: bytcr_5640.c:Refactored if statement and removed buffer
2019-05-06 15:43 ` Hans de Goede
@ 2019-05-06 15:50 ` Pierre-Louis Bossart
0 siblings, 0 replies; 14+ messages in thread
From: Pierre-Louis Bossart @ 2019-05-06 15:50 UTC (permalink / raw)
To: Hans de Goede, Nariman, linux-kernel
Cc: liam.r.girdwood, alsa-devel, broonie, yang.jie, tiwai
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
^ permalink raw reply [flat|nested] 14+ messages in thread