Message-ID: <7c64bdb5-64ee-5a98-965c-bfe59dde615e@linux.intel.com>
Date:   Mon, 16 Jan 2017 09:10:32 -0600
From:   Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>
To:     Shrirang Bagul <shrirang.bagul@...onical.com>,
        alsa-devel@...a-project.org
Cc:     Jeeja KP <jeeja.kp@...el.com>, Arnd Bergmann <arnd@...db.de>,
        Irina Tirdea <irina.tirdea@...el.com>,
        Vinod Koul <vinod.koul@...el.com>,
        linux-kernel@...r.kernel.org, Takashi Iwai <tiwai@...e.com>,
        Ramesh Babu <ramesh.babu@...el.com>,
        Liam Girdwood <lgirdwood@...il.com>,
        Mark Brown <broonie@...nel.org>,
        John Keeping <john@...anate.com>,
        Sathyanarayana Nujella <sathyanarayana.nujella@...el.com>,
        Colin Ian King <colin.king@...onical.com>,
        Wei Yongjun <weiyongjun1@...wei.com>
Subject: Re: [alsa-devel] [PATCH v2 3/4] ASoC: Intel: Support rt5660 codec for
 Baytrail

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@...onical.com>
>>> ---
>>>  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@...a-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>

Powered by blists - more mailing lists