[PATCH] CHROMIUM: ASoC: amd: acp: Add tdm support for codecs in machine driver
From: Venkata Prasad Potturu <venkataprasad.potturu@amd.com> Add tdm support for rt5682, rt5682s, rt1019 and nau8825 codec in machine driver. Signed-off-by: Venkata Prasad Potturu <venkataprasad.potturu@amd.com> --- sound/soc/amd/acp/acp-mach-common.c | 255 ++++++++++++++++++++++++++-- 1 file changed, 243 insertions(+), 12 deletions(-) diff --git a/sound/soc/amd/acp/acp-mach-common.c b/sound/soc/amd/acp/acp-mach-common.c index a78cf29387a7..bce3d8ed48ec 100644 --- a/sound/soc/amd/acp/acp-mach-common.c +++ b/sound/soc/amd/acp/acp-mach-common.c @@ -27,6 +27,21 @@ #include "../../codecs/nau8825.h" #include "acp-mach.h" +static int tdm_mode = 0; +module_param_named(tdm_mode, tdm_mode, int, 0444); +MODULE_PARM_DESC(tdm_mode, "Set 1 for tdm mode, set 0 for i2s mode"); + +#define TDM_CHANNELS 8 +#define BIT_WIDTH 16 +#define SLOT0 BIT(0) +#define SLOT1 BIT(1) +#define SLOT2 BIT(2) +#define SLOT3 BIT(3) +#define SLOT4 BIT(4) +#define SLOT5 BIT(5) +#define SLOT6 BIT(6) +#define SLOT7 BIT(7) + #define PCO_PLAT_CLK 48000000 #define RT5682_PLL_FREQ (48000 * 512) #define DUAL_CHANNEL 2 @@ -80,13 +95,19 @@ static int acp_card_rt5682_init(struct snd_soc_pcm_runtime *rtd) struct snd_soc_dai *codec_dai = asoc_rtd_to_codec(rtd, 0); struct snd_soc_component *component = codec_dai->component; int ret; + unsigned int fmt; dev_info(rtd->dev, "codec dai name = %s\n", codec_dai->name); if (drvdata->hs_codec_id != RT5682) return -EINVAL; - ret = snd_soc_dai_set_fmt(codec_dai, SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_NB_NF + if (tdm_mode) + fmt = SND_SOC_DAIFMT_DSP_A; + else + fmt = SND_SOC_DAIFMT_I2S; + + ret = snd_soc_dai_set_fmt(codec_dai, fmt | SND_SOC_DAIFMT_NB_NF | SND_SOC_DAIFMT_CBP_CFP); if (ret < 0) { dev_err(rtd->card->dev, "Failed to set dai fmt: %d\n", ret); @@ -151,10 +172,15 @@ static int acp_card_hs_startup(struct snd_pcm_substream *substream) int ret; unsigned int fmt; + if (tdm_mode) + fmt = SND_SOC_DAIFMT_DSP_A; + else + fmt = SND_SOC_DAIFMT_I2S; + if (drvdata->soc_mclk) - fmt = SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_NB_NF | SND_SOC_DAIFMT_CBC_CFC; + fmt = fmt | SND_SOC_DAIFMT_NB_NF | SND_SOC_DAIFMT_CBC_CFC; else - fmt = SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_NB_NF | SND_SOC_DAIFMT_CBP_CFP; + fmt = fmt | SND_SOC_DAIFMT_NB_NF | SND_SOC_DAIFMT_CBP_CFP; ret = snd_soc_dai_set_fmt(codec_dai, fmt); if (ret < 0) { @@ -191,9 +217,65 @@ static void acp_card_shutdown(struct snd_pcm_substream *substream) clk_disable_unprepare(drvdata->wclk); } +static int acp_card_rt5682x_hw_params(struct snd_pcm_substream *substream, + struct snd_pcm_hw_params *params) +{ + struct snd_soc_pcm_runtime *rtd = substream->private_data; + struct snd_soc_dai *codec_dai = asoc_rtd_to_codec(rtd, 0); + struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(rtd, 0); + int ret; + unsigned int fmt; + + if (tdm_mode) + fmt = SND_SOC_DAIFMT_DSP_A; + else + fmt = SND_SOC_DAIFMT_I2S; + + ret = snd_soc_dai_set_fmt(cpu_dai, fmt | SND_SOC_DAIFMT_NB_NF + | SND_SOC_DAIFMT_CBP_CFP); + if (ret && ret != -ENOTSUPP) { + dev_err(rtd->dev, "Failed to set dai fmt: %d\n", ret); + return ret; + } + + if (tdm_mode) { + /** + * As codec supports slot 0 and slot 1 for playback and capture. + */ + ret = snd_soc_dai_set_tdm_slot(cpu_dai, SLOT0 | SLOT1, SLOT0 | SLOT1, + TDM_CHANNELS, BIT_WIDTH); + if (ret && ret != -ENOTSUPP) { + dev_err(rtd->dev, "set TDM slot err: %d\n", ret); + return ret; + } + } + + ret = snd_soc_dai_set_fmt(codec_dai, fmt | SND_SOC_DAIFMT_NB_NF + | SND_SOC_DAIFMT_CBC_CFC); + if (ret < 0) { + dev_err(rtd->card->dev, "Failed to set dai fmt: %d\n", ret); + return ret; + } + + if (tdm_mode) { + /** + * As codec supports slot 0 and slot 1 for playback and capture. + */ + ret = snd_soc_dai_set_tdm_slot(codec_dai, SLOT0 | SLOT1, SLOT0 | SLOT1, + TDM_CHANNELS, BIT_WIDTH); + if (ret < 0) { + dev_warn(rtd->dev, "set TDM slot err:%d\n", ret); + return ret; + } + } + + return 0; +} + static const struct snd_soc_ops acp_card_rt5682_ops = { .startup = acp_card_hs_startup, .shutdown = acp_card_shutdown, + .hw_params = acp_card_rt5682x_hw_params, }; /* Define RT5682S CODEC component*/ @@ -220,10 +302,15 @@ static int acp_card_rt5682s_init(struct snd_soc_pcm_runtime *rtd) if (drvdata->hs_codec_id != RT5682S) return -EINVAL; + if (tdm_mode) + fmt = SND_SOC_DAIFMT_DSP_A; + else + fmt = SND_SOC_DAIFMT_I2S; + if (drvdata->soc_mclk) - fmt = SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_NB_NF | SND_SOC_DAIFMT_CBC_CFC; + fmt = fmt | SND_SOC_DAIFMT_NB_NF | SND_SOC_DAIFMT_CBC_CFC; else - fmt = SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_NB_NF | SND_SOC_DAIFMT_CBP_CFP; + fmt = fmt | SND_SOC_DAIFMT_NB_NF | SND_SOC_DAIFMT_CBP_CFP; ret = snd_soc_dai_set_fmt(codec_dai, fmt); if (ret < 0) { @@ -283,6 +370,7 @@ static int acp_card_rt5682s_init(struct snd_soc_pcm_runtime *rtd) static const struct snd_soc_ops acp_card_rt5682s_ops = { .startup = acp_card_hs_startup, + .hw_params = acp_card_rt5682x_hw_params, }; static const unsigned int dmic_channels[] = { @@ -351,19 +439,48 @@ static int acp_card_rt1019_hw_params(struct snd_pcm_substream *substream, struct snd_soc_card *card = rtd->card; struct acp_card_drvdata *drvdata = card->drvdata; struct snd_soc_dai *codec_dai; + struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(rtd, 0); int srate, i, ret = 0; + unsigned int fmt; srate = params_rate(params); if (drvdata->amp_codec_id != RT1019) return -EINVAL; + if (tdm_mode) + fmt = SND_SOC_DAIFMT_DSP_A; + else + fmt = SND_SOC_DAIFMT_I2S; + + ret = snd_soc_dai_set_fmt(cpu_dai, fmt | SND_SOC_DAIFMT_NB_NF + | SND_SOC_DAIFMT_CBP_CFP); + if (ret && ret != -ENOTSUPP) { + dev_err(rtd->dev, "Failed to set dai fmt: %d\n", ret); + return ret; + } + + if (tdm_mode) { + /** + * As codec supports slot 2 and slot 3 for playback. + */ + ret = snd_soc_dai_set_tdm_slot(cpu_dai, SLOT2 | SLOT3, 0, TDM_CHANNELS, BIT_WIDTH); + if (ret && ret != -ENOTSUPP) { + dev_err(rtd->dev, "set TDM slot err: %d\n", ret); + return ret; + } + } for_each_rtd_codec_dais(rtd, i, codec_dai) { if (strcmp(codec_dai->name, "rt1019-aif")) continue; - ret = snd_soc_dai_set_pll(codec_dai, 0, RT1019_PLL_S_BCLK, - 64 * srate, 256 * srate); + if (tdm_mode) + ret = snd_soc_dai_set_pll(codec_dai, 0, RT1019_PLL_S_BCLK, + 128 * srate, 256 * srate); + else + ret = snd_soc_dai_set_pll(codec_dai, 0, RT1019_PLL_S_BCLK, + 64 * srate, 256 * srate); + if (ret < 0) return ret; @@ -371,8 +488,36 @@ static int acp_card_rt1019_hw_params(struct snd_pcm_substream *substream, 256 * srate, SND_SOC_CLOCK_IN); if (ret < 0) return ret; - } + if (tdm_mode) { + ret = snd_soc_dai_set_fmt(codec_dai, SND_SOC_DAIFMT_DSP_A + | SND_SOC_DAIFMT_NB_NF); + if (ret < 0) { + dev_err(rtd->card->dev, "Failed to set dai fmt: %d\n", ret); + return ret; + } + + /** + * As codec supports slot 2 for left channel playback. + */ + if (!strcmp(codec_dai->component->name, "i2c-10EC1019:00")) { + ret = snd_soc_dai_set_tdm_slot(codec_dai, SLOT2, SLOT2, + TDM_CHANNELS, BIT_WIDTH); + if (ret < 0) + break; + } + + /** + * As codec supports slot 3 for right channel playback. + */ + if (!strcmp(codec_dai->component->name, "i2c-10EC1019:01")) { + ret = snd_soc_dai_set_tdm_slot(codec_dai, SLOT3, SLOT3, + TDM_CHANNELS, BIT_WIDTH); + if (ret < 0) + break; + } + } + } return 0; } @@ -426,9 +571,43 @@ static int acp_card_maxim_init(struct snd_soc_pcm_runtime *rtd) ARRAY_SIZE(max98360a_map)); } +static int acp_card_maxim_hw_params(struct snd_pcm_substream *substream, + struct snd_pcm_hw_params *params) +{ + struct snd_soc_pcm_runtime *rtd = substream->private_data; + struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(rtd, 0); + unsigned int fmt; + int ret; + + if (tdm_mode) + fmt = SND_SOC_DAIFMT_DSP_A; + else + fmt = SND_SOC_DAIFMT_I2S; + + ret = snd_soc_dai_set_fmt(cpu_dai, fmt | SND_SOC_DAIFMT_NB_NF + | SND_SOC_DAIFMT_CBP_CFP); + if (ret && ret != -ENOTSUPP) { + dev_err(rtd->dev, "Failed to set dai fmt: %d\n", ret); + return ret; + } + + if (tdm_mode) { + /** + * As codec supports slot 2 and slot 3 for playback. + */ + ret = snd_soc_dai_set_tdm_slot(cpu_dai, SLOT2 | SLOT3, 0, TDM_CHANNELS, BIT_WIDTH); + if (ret && ret != -ENOTSUPP) { + dev_err(rtd->dev, "set TDM slot err: %d\n", ret); + return ret; + } + } + return 0; +} + static const struct snd_soc_ops acp_card_maxim_ops = { .startup = acp_card_amp_startup, .shutdown = acp_card_shutdown, + .hw_params = acp_card_maxim_hw_params, }; /* Declare nau8825 codec components */ @@ -454,10 +633,15 @@ static int acp_card_nau8825_init(struct snd_soc_pcm_runtime *rtd) if (drvdata->hs_codec_id != NAU8825) return -EINVAL; + if (tdm_mode) + fmt = SND_SOC_DAIFMT_DSP_A; + else + fmt = SND_SOC_DAIFMT_I2S; + if (drvdata->soc_mclk) - fmt = SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_NB_NF | SND_SOC_DAIFMT_CBC_CFC; + fmt = fmt | SND_SOC_DAIFMT_NB_NF | SND_SOC_DAIFMT_CBC_CFC; else - fmt = SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_NB_NF | SND_SOC_DAIFMT_CBP_CFP; + fmt = fmt | SND_SOC_DAIFMT_NB_NF | SND_SOC_DAIFMT_CBP_CFP; ret = snd_soc_dai_set_fmt(codec_dai, fmt); if (ret < 0) { @@ -493,8 +677,34 @@ static int acp_nau8825_hw_params(struct snd_pcm_substream *substream, { struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream); struct snd_soc_dai *codec_dai = asoc_rtd_to_codec(rtd, 0); + struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(rtd, 0); int ret; + unsigned int fmt; + if (tdm_mode) + fmt = SND_SOC_DAIFMT_DSP_A; + else + fmt = SND_SOC_DAIFMT_I2S; + + ret = snd_soc_dai_set_fmt(cpu_dai, fmt | SND_SOC_DAIFMT_NB_NF + | SND_SOC_DAIFMT_CBP_CFP); + if (ret && ret != -ENOTSUPP) { + dev_err(rtd->dev, "Failed to set dai fmt: %d\n", ret); + return ret; + } + + if (tdm_mode) { + /** + * As codec supports slot 4 and slot 5 for playback + * and slot 6 and slot 7 for capture. + */ + ret = snd_soc_dai_set_tdm_slot(cpu_dai, SLOT4 | SLOT5, SLOT6 | SLOT7, + TDM_CHANNELS, BIT_WIDTH); + if (ret && ret != -ENOTSUPP) { + dev_err(rtd->dev, "set TDM slot err: %d\n", ret); + return ret; + } + } ret = snd_soc_dai_set_sysclk(codec_dai, NAU8825_CLK_FLL_FS, (48000 * 256), SND_SOC_CLOCK_IN); if (ret < 0) @@ -507,6 +717,25 @@ static int acp_nau8825_hw_params(struct snd_pcm_substream *substream, return ret; } + ret = snd_soc_dai_set_fmt(codec_dai, fmt | SND_SOC_DAIFMT_NB_NF + | SND_SOC_DAIFMT_CBC_CFC); + if (ret < 0) { + dev_err(rtd->card->dev, "Failed to set dai fmt: %d\n", ret); + return ret; + } + + if (tdm_mode) { + /** + * As codec supports slot 4 and slot 5 for playback and slot 6 for capture. + */ + ret = snd_soc_dai_set_tdm_slot(codec_dai, SLOT6, SLOT4 | SLOT5, + TDM_CHANNELS, BIT_WIDTH); + if (ret < 0) { + dev_warn(rtd->dev, "set TDM slot err:%d\n", ret); + return ret; + } + } + return ret; } @@ -567,6 +796,8 @@ SND_SOC_DAILINK_DEF(sof_sp, DAILINK_COMP_ARRAY(COMP_CPU("acp-sof-sp"))); SND_SOC_DAILINK_DEF(sof_hs, DAILINK_COMP_ARRAY(COMP_CPU("acp-sof-hs"))); +SND_SOC_DAILINK_DEF(sof_hs_virtual, + DAILINK_COMP_ARRAY(COMP_CPU("acp-sof-hs-virtual"))); SND_SOC_DAILINK_DEF(sof_dmic, DAILINK_COMP_ARRAY(COMP_CPU("acp-sof-dmic"))); SND_SOC_DAILINK_DEF(pdm_dmic, @@ -733,8 +964,8 @@ int acp_sofdsp_dai_links_create(struct snd_soc_card *card) if (drv_data->amp_cpu_id == I2S_HS) { links[i].name = "acp-amp-codec"; links[i].id = AMP_BE_ID; - links[i].cpus = sof_hs; - links[i].num_cpus = ARRAY_SIZE(sof_hs); + links[i].cpus = sof_hs_virtual; + links[i].num_cpus = ARRAY_SIZE(sof_hs_virtual); links[i].platforms = sof_component; links[i].num_platforms = ARRAY_SIZE(sof_component); links[i].dpcm_playback = 1; -- 2.25.1
On Fri, Oct 28, 2022 at 04:04:41PM +0530, Venkata Prasad Potturu wrote:
+static int tdm_mode = 0; +module_param_named(tdm_mode, tdm_mode, int, 0444); +MODULE_PARM_DESC(tdm_mode, "Set 1 for tdm mode, set 0 for i2s mode");
Why is this a module parameter - how would a user decide to set this? Is it something that someone might want to change at runtime? Please submit patches using subject lines reflecting the style for the subsystem, this makes it easier for people to identify relevant patches. Look at what existing commits in the area you're changing are doing and make sure your subject lines visually resemble what they're doing. There's no need to resubmit to fix this alone.
Thanks for your time. On 10/28/22 16:28, Mark Brown wrote:
+static int tdm_mode = 0; +module_param_named(tdm_mode, tdm_mode, int, 0444); +MODULE_PARM_DESC(tdm_mode, "Set 1 for tdm mode, set 0 for i2s mode"); Why is this a module parameter - how would a user decide to set this? Is it something that someone might want to change at runtime?
While inserting this module we need to pass tdm_mode variable as 0 or 1 like below. sudo insmod /lib/modules/$(uname -r)/kernel/sound/soc/amd/acp/snd-acp-mach.ko *tdm_mode=1*
Please submit patches using subject lines reflecting the style for the subsystem, this makes it easier for people to identify relevant patches. Look at what existing commits in the area you're changing are doing and make sure your subject lines visually resemble what they're doing. There's no need to resubmit to fix this alone.
Sorry for the mistake, we will not repeat this. Thanks.
On Tue, Nov 01, 2022 at 03:15:08PM +0530, Venkata Prasad Potturu wrote:
On 10/28/22 16:28, Mark Brown wrote:
+static int tdm_mode = 0; +module_param_named(tdm_mode, tdm_mode, int, 0444); +MODULE_PARM_DESC(tdm_mode, "Set 1 for tdm mode, set 0 for i2s mode"); Why is this a module parameter - how would a user decide to set this? Is it something that someone might want to change at runtime?
While inserting this module we need to pass tdm_mode variable as 0 or 1 like below.
sudo insmod /lib/modules/$(uname -r)/kernel/sound/soc/amd/acp/snd-acp-mach.ko *tdm_mode=1*
Right, that's what the code does but why is this something that should be controlled in this fashion?
Thanks, On 11/1/22 20:01, Mark Brown wrote:
On Tue, Nov 01, 2022 at 03:15:08PM +0530, Venkata Prasad Potturu wrote:
On 10/28/22 16:28, Mark Brown wrote:
+static int tdm_mode = 0; +module_param_named(tdm_mode, tdm_mode, int, 0444); +MODULE_PARM_DESC(tdm_mode, "Set 1 for tdm mode, set 0 for i2s mode"); Why is this a module parameter - how would a user decide to set this? Is it something that someone might want to change at runtime? While inserting this module we need to pass tdm_mode variable as 0 or 1 like below. sudo insmod/lib/modules/$(uname -r)/kernel/sound/soc/amd/acp/snd-acp-mach.ko *tdm_mode=1* Right, that's what the code does but why is this something that should be controlled in this fashion?
This machine driver is common for TDM mode and I2S mode, user can select TDM mode or I2S mode. Based on tdm_mode parameter we are configuring tdm/i2s format and tdm slot configuration like below. if (tdm_mode) fmt = SND_SOC_DAIFMT_DSP_A; else fmt = SND_SOC_DAIFMT_I2S; ret = snd_soc_dai_set_fmt(cpu_dai, fmt | SND_SOC_DAIFMT_NB_NF | SND_SOC_DAIFMT_CBP_CFP); if (tdm_mode) { /** * As codec supports slot 4 and slot 5 for playback * and slot 6 and slot 7 for capture. */ ret = snd_soc_dai_set_tdm_slot(cpu_dai, SLOT4 | SLOT5, SLOT6 | SLOT7, TDM_CHANNELS, BIT_WIDTH); if (ret && ret != -ENOTSUPP) { dev_err(rtd->dev, "set TDM slot err: %d\n", ret); return ret; } } if (tdm_mode) { /** * As codec supports slot 4 and slot 5 for playback and slot 6 for capture. */ ret = snd_soc_dai_set_tdm_slot(codec_dai, SLOT6, SLOT4 | SLOT5, TDM_CHANNELS, BIT_WIDTH); if (ret < 0) { dev_warn(rtd->dev, "set TDM slot err:%d\n", ret); return ret; } }
On Wed, Nov 02, 2022 at 10:59:07AM +0530, Venkata Prasad Potturu wrote:
On 11/1/22 20:01, Mark Brown wrote:
On Tue, Nov 01, 2022 at 03:15:08PM +0530, Venkata Prasad Potturu wrote:
Right, that's what the code does but why is this something that should be controlled in this fashion?
This machine driver is common for TDM mode and I2S mode, user can select TDM mode or I2S mode.
Why would the user choose one value or the other, and why would this choice be something that only changes at module load time? If this is user controllable I'd really expect it to be runtime controllable. You're not explaining why this is a module parameter.
On 11/2/22 17:02, Mark Brown wrote:
On 11/1/22 20:01, Mark Brown wrote:
On Tue, Nov 01, 2022 at 03:15:08PM +0530, Venkata Prasad Potturu wrote: Right, that's what the code does but why is this something that should be controlled in this fashion? This machine driver is common for TDM mode and I2S mode, user can select TDM mode or I2S mode. Why would the user choose one value or the other, and why would this choice be something that only changes at module load time? If this is user controllable I'd really expect it to be runtime controllable. You're not explaining why this is a module parameter.
Different vendors/OEM's use the same hardware as one need I2S mode and other need TDM mode, using common driver to support I2S and TDM mode with this parameter. static int tdm_mode = 0; module_param_named(tdm_mode, tdm_mode, int, 0444); And this can be runtime controllable by setting permissions as 0644, we will change and send next version patch.
On 11/7/22 04:34, Venkata Prasad Potturu wrote:
On 11/2/22 17:02, Mark Brown wrote:
On 11/1/22 20:01, Mark Brown wrote:
On Tue, Nov 01, 2022 at 03:15:08PM +0530, Venkata Prasad Potturu wrote: Right, that's what the code does but why is this something that should be controlled in this fashion? This machine driver is common for TDM mode and I2S mode, user can select TDM mode or I2S mode. Why would the user choose one value or the other, and why would this choice be something that only changes at module load time? If this is user controllable I'd really expect it to be runtime controllable. You're not explaining why this is a module parameter.
Different vendors/OEM's use the same hardware as one need I2S mode and other need TDM mode, using common driver to support I2S and TDM mode with this parameter.
static int tdm_mode = 0; module_param_named(tdm_mode, tdm_mode, int, 0444);
And this can be runtime controllable by setting permissions as 0644, we will change and send next version patch.
kernel parameters are difficult to manage for distributions using a single-build. Either all platforms use the kernel parameter or none of them do. That would not allow a per-platform choice of parameters. Using DMI quirks or ACPI identifiers would be a lot less problematic, no?
On 11/7/22 19:44, Pierre-Louis Bossart wrote:
Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
On 11/2/22 17:02, Mark Brown wrote:
On 11/1/22 20:01, Mark Brown wrote:
On Tue, Nov 01, 2022 at 03:15:08PM +0530, Venkata Prasad Potturu wrote: Right, that's what the code does but why is this something that should be controlled in this fashion? This machine driver is common for TDM mode and I2S mode, user can select TDM mode or I2S mode. Why would the user choose one value or the other, and why would this choice be something that only changes at module load time? If this is user controllable I'd really expect it to be runtime controllable. You're not explaining why this is a module parameter. Different vendors/OEM's use the same hardware as one need I2S mode and other need TDM mode, using common driver to support I2S and TDM mode with this parameter.
static int tdm_mode = 0; module_param_named(tdm_mode, tdm_mode, int, 0444);
And this can be runtime controllable by setting permissions as 0644, we will change and send next version patch. kernel parameters are difficult to manage for distributions using a single-build. Either all platforms use the kernel parameter or none of
On 11/7/22 04:34, Venkata Prasad Potturu wrote: them do. That would not allow a per-platform choice of parameters. Using DMI quirks or ACPI identifiers would be a lot less problematic, no?
All platforms use the kernel parameter to select the I2S/TDM mode. Runtime parameters are not required here, by default it is in I2S mode and when the platform needs to enable TDM mode then pass tdm_mode module parameter as 1.
On 11/7/22 09:02, Venkata Prasad Potturu wrote:
On 11/7/22 19:44, Pierre-Louis Bossart wrote:
Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
On 11/2/22 17:02, Mark Brown wrote:
On 11/1/22 20:01, Mark Brown wrote:
On Tue, Nov 01, 2022 at 03:15:08PM +0530, Venkata Prasad Potturu wrote: Right, that's what the code does but why is this something that should be controlled in this fashion? This machine driver is common for TDM mode and I2S mode, user can select TDM mode or I2S mode. Why would the user choose one value or the other, and why would this choice be something that only changes at module load time? If this is user controllable I'd really expect it to be runtime controllable. You're not explaining why this is a module parameter. Different vendors/OEM's use the same hardware as one need I2S mode and other need TDM mode, using common driver to support I2S and TDM mode with this parameter.
static int tdm_mode = 0; module_param_named(tdm_mode, tdm_mode, int, 0444);
And this can be runtime controllable by setting permissions as 0644, we will change and send next version patch. kernel parameters are difficult to manage for distributions using a single-build. Either all platforms use the kernel parameter or none of
On 11/7/22 04:34, Venkata Prasad Potturu wrote: them do. That would not allow a per-platform choice of parameters. Using DMI quirks or ACPI identifiers would be a lot less problematic, no?
All platforms use the kernel parameter to select the I2S/TDM mode. Runtime parameters are not required here, by default it is in I2S mode and when the platform needs to enable TDM mode then pass tdm_mode module parameter as 1.
How would a distribution decide to set this kernel parameter, what criteria would be used to determine that the TDM mode is required? You've got to think of who uses that parameter. This may work for a Chrome solution given that there are per-product overlays but won't work in the general case IMHO.
On 11/7/22 20:34, Pierre-Louis Bossart wrote:
Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
On 11/7/22 09:02, Venkata Prasad Potturu wrote:
Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
On 11/2/22 17:02, Mark Brown wrote:
On 11/1/22 20:01, Mark Brown wrote: > On Tue, Nov 01, 2022 at 03:15:08PM +0530, Venkata Prasad Potturu > wrote: > Right, that's what the code does but why is this something that > should > be controlled in this fashion? This machine driver is common for TDM mode and I2S mode, user can select TDM mode or I2S mode. Why would the user choose one value or the other, and why would this choice be something that only changes at module load time? If this is user controllable I'd really expect it to be runtime controllable. You're not explaining why this is a module parameter. Different vendors/OEM's use the same hardware as one need I2S mode and other need TDM mode, using common driver to support I2S and TDM mode with this parameter.
static int tdm_mode = 0; module_param_named(tdm_mode, tdm_mode, int, 0444);
And this can be runtime controllable by setting permissions as 0644, we will change and send next version patch. kernel parameters are difficult to manage for distributions using a single-build. Either all platforms use the kernel parameter or none of
On 11/7/22 04:34, Venkata Prasad Potturu wrote: them do. That would not allow a per-platform choice of parameters. Using DMI quirks or ACPI identifiers would be a lot less problematic, no? All platforms use the kernel parameter to select the I2S/TDM mode. Runtime parameters are not required here, by default it is in I2S mode and when the platform needs to enable TDM mode then pass tdm_mode module
On 11/7/22 19:44, Pierre-Louis Bossart wrote: parameter as 1. How would a distribution decide to set this kernel parameter, what criteria would be used to determine that the TDM mode is required? You've got to think of who uses that parameter. This may work for a Chrome solution given that there are per-product overlays but won't work in the general case IMHO.
Thanks for your time and suggestion. We will come back with DMI quirks.
On Mon, Nov 07, 2022 at 04:04:40PM +0530, Venkata Prasad Potturu wrote:
On 11/2/22 17:02, Mark Brown wrote:
Why would the user choose one value or the other, and why would this choice be something that only changes at module load time? If this is user controllable I'd really expect it to be runtime controllable. You're not explaining why this is a module parameter.
Different vendors/OEM's use the same hardware as one need I2S mode and other need TDM mode, using common driver to support I2S and TDM mode with this parameter.
If a given board needs a specific configuration we should be configuring based on identifying the board, not hoping that the user somehow knows that this configuration is required and can work out how to do it. If this is purely a software setting depending on the software stack running on the device then it should be selected at runtime by that software as part of the use case management.
participants (4)
-
Mark Brown -
Pierre-Louis Bossart -
Venkata Prasad Potturu -
Venkata Prasad Potturu