RE: [PATCH] ASoC: Intel: bxt-da7219-max98357a: support MAX98390 speaker amp

2020-07-01 Thread Lu, Brent
> 
> this doesn't look too good, the only difference is the addition of
> MAX98090 which should be added in
> SND_SOC_INTEL_DA7219_MAX98357A_GENERIC
> above.

Will do

> 
> i don't think you need a different top-level config, just extend the existing
> one to say MAX98357A or MAX98390.
> 
> [...]
> 

OK. I'll modify the description for MAX98390

> 
> these look like unrelated changes?
> 
> 

Will revert those changes.

> 
> That looks just wrong?
> 
> It would be really odd to list two devices as prerequisites for loading a 
> driver,
> when in practice they are mutually exclusive? Something's broken in
> coreboot if both are present.
> 
> see below what we used for JSL:
> 
> see static struct snd_soc_acpi_codecs jsl_7219_98373_codecs = {
>   .num_codecs = 1,
>   .codecs = {"MX98373"}
> };
> 
> static struct snd_soc_acpi_codecs mx98360a_spk = {
>   .num_codecs = 1,
>   .codecs = {"MX98360A"}
> };

Err... I misread the snd_soc_acpi_codec_list function and made this error. Will
fix it in the V2 patch. I will also fix the Chrome v4.19 branch. Thank you for 
the
review.



Regards,
Brent




Re: [PATCH] ASoC: Intel: bxt-da7219-max98357a: support MAX98390 speaker amp

2020-06-30 Thread Pierre-Louis Bossart




diff --git a/sound/soc/intel/boards/Kconfig
b/sound/soc/intel/boards/Kconfig index 3d820e1..b3b863e 100644
--- a/sound/soc/intel/boards/Kconfig
+++ b/sound/soc/intel/boards/Kconfig
@@ -291,9 +291,17 @@ config
SND_SOC_INTEL_DA7219_MAX98357A_GENERIC
select SND_SOC_DMIC
select SND_SOC_HDAC_HDMI

+config SND_SOC_INTEL_DA7219_MAX98390_GENERIC
+   tristate
+   select SND_SOC_DA7219
+   select SND_SOC_MAX98390
+   select SND_SOC_DMIC
+   select SND_SOC_HDAC_HDMI
+
  config SND_SOC_INTEL_BXT_DA7219_MAX98357A_COMMON
tristate
select SND_SOC_INTEL_DA7219_MAX98357A_GENERIC
+   select SND_SOC_INTEL_DA7219_MAX98390_GENERIC


this doesn't look too good, the only difference is the addition of 
MAX98090 which should be added in SND_SOC_INTEL_DA7219_MAX98357A_GENERIC 
above.




  if SND_SOC_INTEL_APL

@@ -309,6 +317,18 @@ config
SND_SOC_INTEL_BXT_DA7219_MAX98357A_MACH
   Say Y or m if you have such a device. This is a recommended option.
   If unsure select "N".

+config SND_SOC_INTEL_BXT_DA7219_MAX98390_MACH
+   tristate "Broxton with DA7219 and MAX98390 in I2S Mode"
+   depends on I2C && ACPI && GPIOLIB
+   depends on MFD_INTEL_LPSS || COMPILE_TEST
+   depends on SND_HDA_CODEC_HDMI
+   select SND_SOC_INTEL_BXT_DA7219_MAX98357A_COMMON
+   help
+  This adds support for ASoC machine driver for Broxton-P platforms
+  with DA7219 + MAX98390 I2S audio codec.
+  Say Y or m if you have such a device. This is a recommended option.
+  If unsure select "N".
+


i don't think you need a different top-level config, just extend the 
existing one to say MAX98357A or MAX98390.


[...]


if (soc_intel_is_glk())
snd_soc_dapm_add_routes(>dapm, gemini_map,
ARRAY_SIZE(gemini_map));
@@ -631,17 +719,17 @@ static int bxt_card_late_probe(struct snd_soc_card
*card)
component = pcm->codec_dai->component;
snprintf(jack_name, sizeof(jack_name),
"HDMI/DP, pcm=%d Jack", pcm->device);
-   err = snd_soc_card_jack_new(card, jack_name,
+   ret = snd_soc_card_jack_new(card, jack_name,
SND_JACK_AVOUT,
_hdmi[i],
NULL, 0);

-   if (err)
-   return err;
+   if (ret)
+   return ret;

-   err = hdac_hdmi_jack_init(pcm->codec_dai, pcm->device,
+   ret = hdac_hdmi_jack_init(pcm->codec_dai, pcm->device,
_hdmi[i]);
-   if (err < 0)
-   return err;
+   if (ret < 0)
+   return ret;


these look like unrelated changes?



--- a/sound/soc/intel/common/soc-acpi-intel-cml-match.c
+++ b/sound/soc/intel/common/soc-acpi-intel-cml-match.c
@@ -15,8 +15,8 @@ static struct snd_soc_acpi_codecs rt1011_spk_codecs =
{  };

  static struct snd_soc_acpi_codecs max98357a_spk_codecs = {
-   .num_codecs = 1,
-   .codecs = {"MX98357A"}
+   .num_codecs = 2,
+   .codecs = {"MX98357A", "MX98390"}


That looks just wrong?

It would be really odd to list two devices as prerequisites for loading 
a driver, when in practice they are mutually exclusive? Something's 
broken in coreboot if both are present.


see below what we used for JSL:

see static struct snd_soc_acpi_codecs jsl_7219_98373_codecs = {
.num_codecs = 1,
.codecs = {"MX98373"}
};

static struct snd_soc_acpi_codecs mx98360a_spk = {
.num_codecs = 1,
.codecs = {"MX98360A"}
};


RE: [PATCH] ASoC: Intel: bxt-da7219-max98357a: support MAX98390 speaker amp

2020-06-30 Thread Lu, Brent
> 
> Support MAX98390 speaker amplifier on cometlake platform. Driver now
> detects amplifier type in the probe function and installs corresponding
> controls and DAPM widgets/routes in the late_probe function.
> 
> Signed-off-by: Brent Lu 

This patch is from Chrome-v4.19 branch to support the combination of DA7219
and MAX98390 on a CML Chromebook. It reuses the topology file
sof-cml-da7219-max98357a.tplg


> ---
>  sound/soc/intel/boards/Kconfig|  20 
>  sound/soc/intel/boards/bxt_da7219_max98357a.c | 129
> --
>  sound/soc/intel/common/soc-acpi-intel-cml-match.c |   4 +-
>  3 files changed, 139 insertions(+), 14 deletions(-)
> 
> diff --git a/sound/soc/intel/boards/Kconfig
> b/sound/soc/intel/boards/Kconfig index 3d820e1..b3b863e 100644
> --- a/sound/soc/intel/boards/Kconfig
> +++ b/sound/soc/intel/boards/Kconfig
> @@ -291,9 +291,17 @@ config
> SND_SOC_INTEL_DA7219_MAX98357A_GENERIC
>   select SND_SOC_DMIC
>   select SND_SOC_HDAC_HDMI
> 
> +config SND_SOC_INTEL_DA7219_MAX98390_GENERIC
> + tristate
> + select SND_SOC_DA7219
> + select SND_SOC_MAX98390
> + select SND_SOC_DMIC
> + select SND_SOC_HDAC_HDMI
> +
>  config SND_SOC_INTEL_BXT_DA7219_MAX98357A_COMMON
>   tristate
>   select SND_SOC_INTEL_DA7219_MAX98357A_GENERIC
> + select SND_SOC_INTEL_DA7219_MAX98390_GENERIC
> 
>  if SND_SOC_INTEL_APL
> 
> @@ -309,6 +317,18 @@ config
> SND_SOC_INTEL_BXT_DA7219_MAX98357A_MACH
>  Say Y or m if you have such a device. This is a recommended option.
>  If unsure select "N".
> 
> +config SND_SOC_INTEL_BXT_DA7219_MAX98390_MACH
> + tristate "Broxton with DA7219 and MAX98390 in I2S Mode"
> + depends on I2C && ACPI && GPIOLIB
> + depends on MFD_INTEL_LPSS || COMPILE_TEST
> + depends on SND_HDA_CODEC_HDMI
> + select SND_SOC_INTEL_BXT_DA7219_MAX98357A_COMMON
> + help
> +This adds support for ASoC machine driver for Broxton-P platforms
> +with DA7219 + MAX98390 I2S audio codec.
> +Say Y or m if you have such a device. This is a recommended option.
> +If unsure select "N".
> +
>  config SND_SOC_INTEL_BXT_RT298_MACH
>   tristate "Broxton with RT298 I2S mode"
>   depends on I2C && ACPI && GPIOLIB
> diff --git a/sound/soc/intel/boards/bxt_da7219_max98357a.c
> b/sound/soc/intel/boards/bxt_da7219_max98357a.c
> index 44016c1..12f07e1 100644
> --- a/sound/soc/intel/boards/bxt_da7219_max98357a.c
> +++ b/sound/soc/intel/boards/bxt_da7219_max98357a.c
> @@ -25,9 +25,14 @@
> 
>  #define BXT_DIALOG_CODEC_DAI "da7219-hifi"
>  #define BXT_MAXIM_CODEC_DAI  "HiFi"
> +#define MAX98390_DEV0_NAME   "i2c-MX98390:00"
> +#define MAX98390_DEV1_NAME   "i2c-MX98390:01"
>  #define DUAL_CHANNEL 2
>  #define QUAD_CHANNEL 4
> 
> +#define SPKAMP_MAX98357A 1
> +#define SPKAMP_MAX98390  2
> +
>  static struct snd_soc_jack broxton_headset;  static struct snd_soc_jack
> broxton_hdmi[3];
> 
> @@ -40,6 +45,7 @@ struct bxt_hdmi_pcm {
>  struct bxt_card_private {
>   struct list_head hdmi_pcm_list;
>   bool common_hdmi_codec_drv;
> + int spkamp;
>  };
> 
>  enum {
> @@ -85,13 +91,20 @@ static int platform_clock_control(struct
> snd_soc_dapm_widget *w,  static const struct snd_kcontrol_new
> broxton_controls[] = {
>   SOC_DAPM_PIN_SWITCH("Headphone Jack"),
>   SOC_DAPM_PIN_SWITCH("Headset Mic"),
> +};
> +
> +static const struct snd_kcontrol_new max98357a_controls[] = {
>   SOC_DAPM_PIN_SWITCH("Spk"),
>  };
> 
> +static const struct snd_kcontrol_new max98390_controls[] = {
> + SOC_DAPM_PIN_SWITCH("Left Spk"),
> + SOC_DAPM_PIN_SWITCH("Right Spk"),
> +};
> +
>  static const struct snd_soc_dapm_widget broxton_widgets[] = {
>   SND_SOC_DAPM_HP("Headphone Jack", NULL),
>   SND_SOC_DAPM_MIC("Headset Mic", NULL),
> - SND_SOC_DAPM_SPK("Spk", NULL),
>   SND_SOC_DAPM_MIC("SoC DMIC", NULL),
>   SND_SOC_DAPM_SPK("HDMI1", NULL),
>   SND_SOC_DAPM_SPK("HDMI2", NULL),
> @@ -100,14 +113,20 @@ static const struct snd_soc_dapm_widget
> broxton_widgets[] = {
>   platform_clock_control,
>   SND_SOC_DAPM_POST_PMD|SND_SOC_DAPM_PRE_PMU),
>  };
> 
> +static const struct snd_soc_dapm_widget max98357a_widgets[] = {
> + SND_SOC_DAPM_SPK("Spk", NULL),
> +};
> +
> +static const struct snd_soc_dapm_widget max98390_widgets[] = {
> + SND_SOC_DAPM_SPK("Left Spk", NULL),
> + SND_SOC_DAPM_SPK("Right Spk", NULL),
> +};
> +
>  static const struct snd_soc_dapm_route audio_map[] = {
>   /* HP jack connectors - unknown if we have jack detection */
>   {"Headphone Jack", NULL, "HPL"},
>   {"Headphone Jack", NULL, "HPR"},
> 
> - /* speaker */
> - {"Spk", NULL, "Speaker"},
> -
>   /* other jacks */
>   {"MIC", NULL, "Headset Mic"},
> 
> @@ -134,6 +153,17 @@ static const struct snd_soc_dapm_route audio_map[]
> = {
>   { "Headset Mic", NULL, "Platform Clock" },  };
> 
> +static const struct