Re: [alsa-devel] [PATCH v2 3/4] ASoC: Intel: Support rt5660 codec for Baytrail
From: Pierre-Louis Bossart
Date: Mon Jan 16 2017 - 10:10:43 EST
On 1/16/17 1:45 AM, Shrirang Bagul wrote:
On Thu, 2017-01-12 at 08:40 -0600, Pierre-Louis Bossart wrote:
On 1/12/17 6:01 AM, Shrirang Bagul wrote:
rt5660 and rt5640 are similar codecs so reuse the bytcr_rt5640 driver.
RT5660 codec is used on Dell Edge IoT Gateways with ACPI ID 10EC3277.
These devices sport only Line-In and Line-Out jacks.
While it would be nice to avoid copy/pasting everytime we add a new
codec and refactor the code, I am not comfortable with a series of
changes below.
Also if we do this refactoring then we might as well do it for rt5651
which is similar and only relies on I2S. other machine drivers enable
TDM mode when possible.
And last this change has a lot of impact on how we deal with UCM files.
The name of the card should reflect which codec is used, and the quirks
be added to the long name. If you lump everything with a single name
then you will make it really hard for userspace to figure out which
controls need to be set.
So nice idea but too early to merge. NAK.
Thank you for the review, will address these comments in the next version. When
you it be appropriate to re-submit? Are we waiting for any patches which are
queued to be merged soon?
I have a set of 5640/5651 corrections that will be sent out very soon
(fixes for existing platforms).
I am ok with factorizing some of the common code, however the quirk
management can't really be fused and the platform names need to remain
separate. The dailinks and dapm routes need to remain separate, same for
the platform clock control. In the end, I am not sure if it's better to
have a single file with all the codecs, or if we should keep separate
files and use a common set of helper functions. The latter might be
wiser if we want to add another codec, it'd avoid complicated if/else
cases in the middle of the common code.
Signed-off-by: Shrirang Bagul <shrirang.bagul@xxxxxxxxxxxxx>
---
sound/soc/intel/Kconfig | 11 +--
sound/soc/intel/atom/sst/sst_acpi.c | 2 +
sound/soc/intel/boards/bytcr_rt5640.c | 156 ++++++++++++++++++++++++++++++-
---
3 files changed, 147 insertions(+), 22 deletions(-)
diff --git a/sound/soc/intel/Kconfig b/sound/soc/intel/Kconfig
index fd5d1e0..0b43b6a 100644
--- a/sound/soc/intel/Kconfig
+++ b/sound/soc/intel/Kconfig
@@ -147,17 +147,18 @@ config SND_SOC_INTEL_BROADWELL_MACH
If unsure select "N".
config SND_SOC_INTEL_BYTCR_RT5640_MACH
- tristate "ASoC Audio driver for Intel Baytrail and Baytrail-CR with
RT5640 codec"
+ tristate "ASoC Audio driver for Intel Baytrail and Baytrail-CR with
RT5640/5660 codec"
depends on X86 && I2C && ACPI
select SND_SOC_RT5640
+ select SND_SOC_RT5660
select SND_SST_MFLD_PLATFORM
select SND_SST_IPC_ACPI
select SND_SOC_INTEL_SST_MATCH if ACPI
help
- This adds support for ASoC machine driver for Intel(R) Baytrail
and Baytrail-CR
- platforms with RT5640 audio codec.
- Say Y if you have such a device.
- If unsure select "N".
+ This adds support for ASoC machine driver for Intel(R) Baytrail
and Baytrail-CR
+ platforms with RT5640, RT5460 audio codec.
+ Say Y if you have such a device.
+ If unsure select "N".
config SND_SOC_INTEL_BYTCR_RT5651_MACH
tristate "ASoC Audio driver for Intel Baytrail and Baytrail-CR with
RT5651 codec"
diff --git a/sound/soc/intel/atom/sst/sst_acpi.c
b/sound/soc/intel/atom/sst/sst_acpi.c
index f4d92bb..d401457f 100644
--- a/sound/soc/intel/atom/sst/sst_acpi.c
+++ b/sound/soc/intel/atom/sst/sst_acpi.c
@@ -441,6 +441,8 @@ static struct sst_acpi_mach sst_acpi_bytcr[] = {
&byt_rvp_platform_data },
{"10EC5642", "bytcr_rt5640", "intel/fw_sst_0f28.bin",
"bytcr_rt5640", NULL,
&byt_rvp_platform_data },
+ {"10EC3277", "bytcr_rt5640", "intel/fw_sst_0f28.bin",
"bytcr_rt5640", NULL,
+ &byt_rvp_platform_data },
so right there you add an HID in the platform driver and you need the
same in the platform driver to determine which codec type this is...
{"INTCCFFD", "bytcr_rt5640", "intel/fw_sst_0f28.bin",
"bytcr_rt5640", NULL,
&byt_rvp_platform_data },
{"10EC5651", "bytcr_rt5651", "intel/fw_sst_0f28.bin",
"bytcr_rt5651", NULL,
diff --git a/sound/soc/intel/boards/bytcr_rt5640.c
b/sound/soc/intel/boards/bytcr_rt5640.c
index f6fd397..e8c9a01 100644
--- a/sound/soc/intel/boards/bytcr_rt5640.c
+++ b/sound/soc/intel/boards/bytcr_rt5640.c
@@ -32,11 +32,17 @@
#include <sound/soc.h>
#include <sound/jack.h>
#include "../../codecs/rt5640.h"
+#include "../../codecs/rt5660.h"
#include "../atom/sst-atom-controls.h"
#include "../common/sst-acpi.h"
#include "../common/sst-dsp.h"
enum {
+ CODEC_TYPE_RT5640,
+ CODEC_TYPE_RT5660,
+};
+
+enum {
BYT_RT5640_DMIC1_MAP,
BYT_RT5640_DMIC2_MAP,
BYT_RT5640_IN1_MAP,
@@ -60,8 +66,16 @@ enum {
PLL1_MCLK,
};
+struct byt_acpi_card {
+ char *codec_id;
+ int codec_type;
+ struct snd_soc_card *soc_card;
+};
+
struct byt_rt5640_private {
+ struct byt_acpi_card *acpi_card;
struct clk *mclk;
+ char codec_name[16];
int *clks;
};
@@ -72,6 +86,13 @@ static int byt_rt5640_clks[] = {
RT5640_PLL1_S_MCLK
};
+static int byt_rt5660_clks[] = {
+ RT5660_SCLK_S_PLL1,
+ RT5660_SCLK_S_RCCLK,
+ RT5660_PLL1_S_BCLK,
+ RT5660_PLL1_S_MCLK
+};
+
static unsigned long byt_rt5640_quirk = BYT_RT5640_MCLK_EN;
the quirks would need to be isolated and made dependent on codec type
Will fix in next version of the patch.
static void log_quirks(struct device *dev)
@@ -105,6 +126,7 @@ static void log_quirks(struct device *dev)
#define BYT_CODEC_DAI1 "rt5640-aif1"
#define BYT_CODEC_DAI2 "rt5640-aif2"
+#define BYT_CODEC_DAI3 "rt5660-aif1"
static inline struct snd_soc_dai *byt_get_codec_dai(struct snd_soc_card
*card)
{
@@ -117,6 +139,9 @@ static inline struct snd_soc_dai
*byt_get_codec_dai(struct snd_soc_card *card)
if (!strncmp(rtd->codec_dai->name, BYT_CODEC_DAI2,
strlen(BYT_CODEC_DAI2)))
return rtd->codec_dai;
+ if (!strncmp(rtd->codec_dai->name, BYT_CODEC_DAI3,
+ strlen(BYT_CODEC_DAI3)))
+ return rtd->codec_dai;
not very good to go look for a DAI that doesn't exist for a specific
codec. this would need to be dependent on codec type.
Understood, will fix this in next version.
}
return NULL;
@@ -269,6 +294,29 @@ static const struct snd_kcontrol_new
byt_rt5640_controls[] = {
SOC_DAPM_PIN_SWITCH("Speaker"),
};
+static const struct snd_soc_dapm_widget byt_rt5660_widgets[] = {
+ SND_SOC_DAPM_MIC("Line In", NULL),
+ SND_SOC_DAPM_LINE("Line Out", NULL),
+ SND_SOC_DAPM_SUPPLY("Platform Clock", SND_SOC_NOPM, 0, 0,
+ platform_clock_control, SND_SOC_DAPM_PRE_PMU |
+ SND_SOC_DAPM_POST_PMD),
+};
+
+static const struct snd_soc_dapm_route byt_rt5660_audio_map[] = {
+ {"IN1P", NULL, "Line In"},
+ {"IN2P", NULL, "Line In"},
+ {"Line Out", NULL, "LOUTR"},
+ {"Line Out", NULL, "LOUTL"},
+
+ {"Line In", NULL, "Platform Clock"},
+ {"Line Out", NULL, "Platform Clock"},
+};
+
+static const struct snd_kcontrol_new byt_rt5660_controls[] = {
+ SOC_DAPM_PIN_SWITCH("Line In"),
+ SOC_DAPM_PIN_SWITCH("Line Out"),
+};
+
static int byt_rt5640_aif1_hw_params(struct snd_pcm_substream *substream,
struct snd_pcm_hw_params *params)
{
@@ -422,11 +470,8 @@ static int byt_rt5640_init(struct snd_soc_pcm_runtime
*runtime)
struct snd_soc_codec *codec = runtime->codec;
struct snd_soc_card *card = runtime->card;
const struct snd_soc_dapm_route *custom_map;
- struct byt_rt5640_private *priv = snd_soc_card_get_drvdata(card);
int num_routes;
- card->dapm.idle_bias_off = true;
-
rt5640_sel_asrc_clk_src(codec,
RT5640_DA_STEREO_FILTER |
RT5640_DA_MONO_L_FILTER |
@@ -511,6 +556,39 @@ static int byt_rt5640_init(struct snd_soc_pcm_runtime
*runtime)
snd_soc_dapm_ignore_suspend(&card->dapm, "Headphone");
snd_soc_dapm_ignore_suspend(&card->dapm, "Speaker");
+ return ret;
+}
+
+static int byt_rt5660_init(struct snd_soc_pcm_runtime *runtime)
+{
+ int ret;
+ struct snd_soc_card *card = runtime->card;
+
+ ret = snd_soc_dapm_add_routes(&card->dapm,
+ byt_rt5640_ssp2_aif1_map,
+ ARRAY_SIZE(byt_rt5640_ssp2_aif1_map));
+ if (ret)
+ return ret;
+
+ snd_soc_dapm_enable_pin(&card->dapm, "Line In");
+ snd_soc_dapm_enable_pin(&card->dapm, "Line Out");
+
+ return ret;
+}
+
+static int byt_rt56x0_init(struct snd_soc_pcm_runtime *runtime)
+{
+ int ret;
+ struct snd_soc_card *card = runtime->card;
+ struct byt_rt5640_private *priv = snd_soc_card_get_drvdata(card);
+
+ card->dapm.idle_bias_off = true;
+
+ if (priv->acpi_card->codec_type == CODEC_TYPE_RT5640)
+ ret = byt_rt5640_init(runtime);
+ else
+ ret = byt_rt5660_init(runtime);
+
if ((byt_rt5640_quirk & BYT_RT5640_MCLK_EN) && priv->mclk) {
/*
* The firmware might enable the clock at
@@ -679,7 +757,7 @@ static struct snd_soc_dai_link byt_rt5640_dais[] = {
.ignore_suspend = 1,
.dpcm_playback = 1,
.dpcm_capture = 1,
- .init = byt_rt5640_init,
+ .init = byt_rt56x0_init,
.ops = &byt_rt5640_be_ssp2_ops,
},
};
@@ -697,6 +775,25 @@ static struct snd_soc_card byt_rt5640_card = {
.fully_routed = true,
};
+static struct snd_soc_card byt_rt5660_card = {
+ .name = "bytcr-rt5660",
+ .owner = THIS_MODULE,
+ .dai_link = byt_rt5640_dais,
+ .num_links = ARRAY_SIZE(byt_rt5640_dais),
+ .controls = byt_rt5660_controls,
+ .num_controls = ARRAY_SIZE(byt_rt5660_controls),
+ .dapm_widgets = byt_rt5660_widgets,
+ .num_dapm_widgets = ARRAY_SIZE(byt_rt5660_widgets),
+ .dapm_routes = byt_rt5660_audio_map,
+ .num_dapm_routes = ARRAY_SIZE(byt_rt5660_audio_map),
+ .fully_routed = true,
+};
+
+static struct byt_acpi_card snd_soc_cards[] = {
+ {"10EC5640", CODEC_TYPE_RT5640, &byt_rt5640_card},
+ {"10EC3277", CODEC_TYPE_RT5660, &byt_rt5660_card},
+};
I know this is what's used for rt5645/50 but I don't like it and don't
think it should be the baseline for how we deal with codecs. This forces
the addition of additional quirks.
Is there a better baseline to start from or none exists and we ought to come-up
with one?
+
static char byt_rt5640_codec_name[16]; /* i2c-<HID>:00 with HID being 8
chars */
static char byt_rt5640_codec_aif_name[12]; /* = "rt5640-aif[1|2]" */
static char byt_rt5640_cpu_dai_name[10]; /* = "ssp[0|2]-port" */
@@ -721,41 +818,51 @@ struct acpi_chan_package { /* ACPICA seems to
require 64 bit integers */
static int snd_byt_rt5640_mc_probe(struct platform_device *pdev)
{
int ret_val = 0;
- struct sst_acpi_mach *mach;
- const char *i2c_name = NULL;
int i;
- int dai_index;
struct byt_rt5640_private *priv;
+ struct snd_soc_card *card = snd_soc_cards[0].soc_card;
+ struct sst_acpi_mach *mach;
+ const char *i2c_name = NULL;
+ int dai_index = 0;
bool is_bytcr = false;
priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_ATOMIC);
if (!priv)
return -ENOMEM;
+ for (i = 0; i < ARRAY_SIZE(snd_soc_cards); i++) {
+ if (acpi_dev_found(snd_soc_cards[i].codec_id)) {
+ dev_dbg(&pdev->dev,
+ "found codec %s\n",
snd_soc_cards[i].codec_id);
+ card = snd_soc_cards[i].soc_card;
+ priv->acpi_card = &snd_soc_cards[i];
+ break;
+ }
+ }
+
/* register the soc card */
priv->clks = byt_rt5640_clks;
- byt_rt5640_card.dev = &pdev->dev;
- mach = byt_rt5640_card.dev->platform_data;
- snd_soc_card_set_drvdata(&byt_rt5640_card, priv);
+ card->dev = &pdev->dev;
+ mach = card->dev->platform_data;
+ sprintf(priv->codec_name, "i2c-%s:00", priv->acpi_card->codec_id);
/* fix index of codec dai */
- dai_index = MERR_DPCM_COMPR + 1;
- for (i = 0; i < ARRAY_SIZE(byt_rt5640_dais); i++) {
+ for (i = 0; i < ARRAY_SIZE(byt_rt5640_dais); i++)
if (!strcmp(byt_rt5640_dais[i].codec_name, "i2c-
10EC5640:00")) {
+ card->dai_link[i].codec_name = priv->codec_name;
dai_index = i;
- break;
}
- }
/* fixup codec name based on HID */
i2c_name = sst_acpi_find_name_from_hid(mach->id);
if (i2c_name != NULL) {
snprintf(byt_rt5640_codec_name,
sizeof(byt_rt5640_codec_name),
"%s%s", "i2c-", i2c_name);
-
byt_rt5640_dais[dai_index].codec_name =
byt_rt5640_codec_name;
}
+ snd_soc_card_set_drvdata(card, priv);
+
/*
* swap SSP0 if bytcr is detected
* (will be overridden if DMI quirk is detected)
@@ -821,6 +928,21 @@ static int snd_byt_rt5640_mc_probe(struct
platform_device *pdev)
BYT_RT5640_DMIC_EN);
}
+ if (priv->acpi_card->codec_type == CODEC_TYPE_RT5660) {
+ priv->clks = byt_rt5660_clks;
+
+ /* fixup codec aif name for rt5660 */
+ snprintf(byt_rt5640_codec_aif_name,
+ sizeof(byt_rt5640_codec_aif_name),
+ "%s", "rt5660-aif1");
+
+ byt_rt5640_dais[dai_index].codec_dai_name =
+ byt_rt5640_codec_aif_name;
+
+ /* setup codec quirks for rt5660 */
+ byt_rt5640_quirk = BYT_RT5640_MCLK_EN;
+ }
+
/* check quirks before creating card */
dmi_check_system(byt_rt5640_quirk_table);
the quirks have to be separate.
log_quirks(&pdev->dev);
@@ -869,14 +991,14 @@ static int snd_byt_rt5640_mc_probe(struct
platform_device *pdev)
}
}
- ret_val = devm_snd_soc_register_card(&pdev->dev, &byt_rt5640_card);
+ ret_val = devm_snd_soc_register_card(&pdev->dev, card);
if (ret_val) {
dev_err(&pdev->dev, "devm_snd_soc_register_card failed
%d\n",
ret_val);
return ret_val;
}
- platform_set_drvdata(pdev, &byt_rt5640_card);
+ platform_set_drvdata(pdev, card);
return ret_val;
}
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel