From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751504AbdAPHpf (ORCPT ); Mon, 16 Jan 2017 02:45:35 -0500 Received: from mail-pg0-f53.google.com ([74.125.83.53]:36233 "EHLO mail-pg0-f53.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750855AbdAPHpa (ORCPT ); Mon, 16 Jan 2017 02:45:30 -0500 Message-ID: <1484552722.4496.2.camel@canonical.com> Subject: Re: [alsa-devel] [PATCH v2 3/4] ASoC: Intel: Support rt5660 codec for Baytrail From: Shrirang Bagul To: Pierre-Louis Bossart , alsa-devel@alsa-project.org Cc: Arnd Bergmann , Liam Girdwood , Vinod Koul , linux-kernel@vger.kernel.org, Irina Tirdea , Takashi Iwai , Ramesh Babu , Mark Brown , John Keeping , Sathyanarayana Nujella , Wei Yongjun , Colin Ian King , Jeeja KP Date: Mon, 16 Jan 2017 15:45:22 +0800 In-Reply-To: References: <20170112120101.16933-1-shrirang.bagul@canonical.com> <20170112120101.16933-4-shrirang.bagul@canonical.com> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.22.1-0ubuntu2 Mime-Version: 1.0 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > > --- > >  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 > >  #include > >  #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-: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; > >  } > > > > > >