Re: [alsa-devel] [PATCH v2 3/4] ASoC: Intel: Support rt5660 codec for Baytrail

[Shrirang Bagul]

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?
> 
> > 
> > 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;
> > Â}
> > 
> > 
> 
>