[PATCH 0/2] Intel: sof_rt5682: remove quirk flag
We add a helper funcion to detect amplifier number according to device instance in ACPI table so the SOF_MAX98390_TWEETER_SPEAKER_PRESENT flag and a dmi quirk for 4-amplifier configuration could be safely removed.
Also refactory the max_98390_hw_params() function to use an array to handle the TDM parameter.
Amplifier number detection and TDM parameter are tested on two Chromebooks. One with 2 MAX98390 and one with 4 MAX98390 amplifier.
*** BLURB HERE ***
Brent Lu (2): ASoC: Intel: maxim-common: get codec number from ACPI ASoC: Intel: sof_rt5682: remove SOF_MAX98390_TWEETER_SPEAKER_PRESENT flag
sound/soc/intel/boards/sof_maxim_common.c | 174 +++++++++++++--------- sound/soc/intel/boards/sof_maxim_common.h | 21 ++- sound/soc/intel/boards/sof_rt5682.c | 37 +---- 3 files changed, 115 insertions(+), 117 deletions(-)
Implement a helper function to get number of codecs from ACPI subsystem to remove the need of quirk flag in machine driver.
Signed-off-by: Brent Lu brent.lu@intel.com --- sound/soc/intel/boards/sof_maxim_common.c | 174 +++++++++++++--------- sound/soc/intel/boards/sof_maxim_common.h | 21 ++- 2 files changed, 113 insertions(+), 82 deletions(-)
diff --git a/sound/soc/intel/boards/sof_maxim_common.c b/sound/soc/intel/boards/sof_maxim_common.c index 112e89951da0..f8b44a81fec1 100644 --- a/sound/soc/intel/boards/sof_maxim_common.c +++ b/sound/soc/intel/boards/sof_maxim_common.c @@ -4,6 +4,7 @@ #include <linux/module.h> #include <linux/string.h> #include <sound/pcm.h> +#include <sound/pcm_params.h> #include <sound/soc.h> #include <sound/soc-acpi.h> #include <sound/soc-dai.h> @@ -11,6 +12,21 @@ #include <uapi/sound/asound.h> #include "sof_maxim_common.h"
+/* helper function to get the number of specific codec */ +static int get_num_codecs(const char *hid) +{ + struct acpi_device *adev = NULL; + int dev_num = 0; + + do { + adev = acpi_dev_get_next_match_dev(adev, hid, NULL, -1); + if (adev) + dev_num++; + } while (adev != NULL); + + return dev_num; +} + #define MAX_98373_PIN_NAME 16
const struct snd_soc_dapm_route max_98373_dapm_routes[] = { @@ -168,17 +184,6 @@ static struct snd_soc_codec_conf max_98390_codec_conf[] = { .dlc = COMP_CODEC_CONF(MAX_98390_DEV1_NAME), .name_prefix = "Left", }, -}; - -static struct snd_soc_codec_conf max_98390_4spk_codec_conf[] = { - { - .dlc = COMP_CODEC_CONF(MAX_98390_DEV0_NAME), - .name_prefix = "Right", - }, - { - .dlc = COMP_CODEC_CONF(MAX_98390_DEV1_NAME), - .name_prefix = "Left", - }, { .dlc = COMP_CODEC_CONF(MAX_98390_DEV2_NAME), .name_prefix = "Tweeter Right", @@ -189,19 +194,7 @@ static struct snd_soc_codec_conf max_98390_4spk_codec_conf[] = { }, };
-struct snd_soc_dai_link_component max_98390_components[] = { - { - .name = MAX_98390_DEV0_NAME, - .dai_name = MAX_98390_CODEC_DAI, - }, - { - .name = MAX_98390_DEV1_NAME, - .dai_name = MAX_98390_CODEC_DAI, - }, -}; -EXPORT_SYMBOL_NS(max_98390_components, SND_SOC_INTEL_SOF_MAXIM_COMMON); - -struct snd_soc_dai_link_component max_98390_4spk_components[] = { +static struct snd_soc_dai_link_component max_98390_components[] = { { .name = MAX_98390_DEV0_NAME, .dai_name = MAX_98390_CODEC_DAI, @@ -219,62 +212,56 @@ struct snd_soc_dai_link_component max_98390_4spk_components[] = { .dai_name = MAX_98390_CODEC_DAI, }, }; -EXPORT_SYMBOL_NS(max_98390_4spk_components, SND_SOC_INTEL_SOF_MAXIM_COMMON); + +static const struct { + unsigned int tx; + unsigned int rx; +} max_98390_tdm_mask[] = { + {.tx = 0x01, .rx = 0x3}, + {.tx = 0x02, .rx = 0x3}, + {.tx = 0x04, .rx = 0x3}, + {.tx = 0x08, .rx = 0x3}, +};
static int max_98390_hw_params(struct snd_pcm_substream *substream, struct snd_pcm_hw_params *params) { struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream); struct snd_soc_dai *codec_dai; - int i; + int i, ret = 0;
for_each_rtd_codec_dais(rtd, i, codec_dai) { - if (i >= ARRAY_SIZE(max_98390_4spk_components)) { + if (i >= ARRAY_SIZE(max_98390_tdm_mask)) { dev_err(codec_dai->dev, "invalid codec index %d\n", i); return -ENODEV; }
- if (!strcmp(codec_dai->component->name, MAX_98390_DEV0_NAME)) { - /* DEV0 tdm slot configuration Right */ - snd_soc_dai_set_tdm_slot(codec_dai, 0x01, 3, 4, 32); - } - if (!strcmp(codec_dai->component->name, MAX_98390_DEV1_NAME)) { - /* DEV1 tdm slot configuration Left */ - snd_soc_dai_set_tdm_slot(codec_dai, 0x02, 3, 4, 32); - } - - if (!strcmp(codec_dai->component->name, MAX_98390_DEV2_NAME)) { - /* DEVi2 tdm slot configuration Tweeter Right */ - snd_soc_dai_set_tdm_slot(codec_dai, 0x04, 3, 4, 32); - } - if (!strcmp(codec_dai->component->name, MAX_98390_DEV3_NAME)) { - /* DEV3 tdm slot configuration Tweeter Left */ - snd_soc_dai_set_tdm_slot(codec_dai, 0x08, 3, 4, 32); + ret = snd_soc_dai_set_tdm_slot(codec_dai, max_98390_tdm_mask[i].tx, + max_98390_tdm_mask[i].rx, 4, + params_width(params)); + if (ret < 0) { + dev_err(codec_dai->dev, "fail to set tdm slot, ret %d\n", + ret); + return ret; } } return 0; }
-int max_98390_spk_codec_init(struct snd_soc_pcm_runtime *rtd) +static int max_98390_init(struct snd_soc_pcm_runtime *rtd) { struct snd_soc_card *card = rtd->card; + int num_codecs = get_num_codecs(MAX_98390_ACPI_HID); int ret;
- /* add regular speakers dapm route */ - ret = snd_soc_dapm_add_routes(&card->dapm, max_98390_dapm_routes, - ARRAY_SIZE(max_98390_dapm_routes)); - if (ret) { - dev_err(rtd->dev, "unable to add Left/Right Speaker dapm, ret %d\n", ret); - return ret; - } - - /* add widgets/controls/dapm for tweeter speakers */ - if (acpi_dev_present("MX98390", "3", -1)) { + switch (num_codecs) { + case 4: + /* add widgets/controls/dapm for tweeter speakers */ ret = snd_soc_dapm_new_controls(&card->dapm, max_98390_tt_dapm_widgets, ARRAY_SIZE(max_98390_tt_dapm_widgets)); - if (ret) { - dev_err(rtd->dev, "unable to add tweeter dapm controls, ret %d\n", ret); + dev_err(rtd->dev, "unable to add tweeter dapm widgets, ret %d\n", + ret); /* Don't need to add routes if widget addition failed */ return ret; } @@ -282,33 +269,80 @@ int max_98390_spk_codec_init(struct snd_soc_pcm_runtime *rtd) ret = snd_soc_add_card_controls(card, max_98390_tt_kcontrols, ARRAY_SIZE(max_98390_tt_kcontrols)); if (ret) { - dev_err(rtd->dev, "unable to add tweeter card controls, ret %d\n", ret); + dev_err(rtd->dev, "unable to add tweeter controls, ret %d\n", + ret); return ret; }
ret = snd_soc_dapm_add_routes(&card->dapm, max_98390_tt_dapm_routes, ARRAY_SIZE(max_98390_tt_dapm_routes)); - if (ret) - dev_err(rtd->dev, - "unable to add Tweeter Left/Right Speaker dapm, ret %d\n", ret); + if (ret) { + dev_err(rtd->dev, "unable to add tweeter dapm routes, ret %d\n", + ret); + return ret; + } + + fallthrough; + case 2: + /* add regular speakers dapm route */ + ret = snd_soc_dapm_add_routes(&card->dapm, max_98390_dapm_routes, + ARRAY_SIZE(max_98390_dapm_routes)); + if (ret) { + dev_err(rtd->dev, "unable to add dapm routes, ret %d\n", + ret); + return ret; + } + break; + default: + dev_err(rtd->dev, "invalid codec number %d\n", num_codecs); + ret = -EINVAL; + break; } + return ret; } -EXPORT_SYMBOL_NS(max_98390_spk_codec_init, SND_SOC_INTEL_SOF_MAXIM_COMMON);
-const struct snd_soc_ops max_98390_ops = { +static const struct snd_soc_ops max_98390_ops = { .hw_params = max_98390_hw_params, }; -EXPORT_SYMBOL_NS(max_98390_ops, SND_SOC_INTEL_SOF_MAXIM_COMMON);
-void max_98390_set_codec_conf(struct snd_soc_card *card, int ch) +void max_98390_dai_link(struct snd_soc_dai_link *link) +{ + int num_codecs = get_num_codecs(MAX_98390_ACPI_HID); + + link->codecs = max_98390_components; + + switch (num_codecs) { + case 2: + case 4: + link->num_codecs = num_codecs; + break; + default: + pr_err("invalid codec number %d for %s\n", num_codecs, + MAX_98390_ACPI_HID); + break; + } + + link->init = max_98390_init; + link->ops = &max_98390_ops; +} +EXPORT_SYMBOL_NS(max_98390_dai_link, SND_SOC_INTEL_SOF_MAXIM_COMMON); + +void max_98390_set_codec_conf(struct snd_soc_card *card) { - if (ch == ARRAY_SIZE(max_98390_4spk_codec_conf)) { - card->codec_conf = max_98390_4spk_codec_conf; - card->num_configs = ARRAY_SIZE(max_98390_4spk_codec_conf); - } else { - card->codec_conf = max_98390_codec_conf; - card->num_configs = ARRAY_SIZE(max_98390_codec_conf); + int num_codecs = get_num_codecs(MAX_98390_ACPI_HID); + + card->codec_conf = max_98390_codec_conf; + + switch (num_codecs) { + case 2: + case 4: + card->num_configs = num_codecs; + break; + default: + pr_err("invalid codec number %d for %s\n", num_codecs, + MAX_98390_ACPI_HID); + break; } } EXPORT_SYMBOL_NS(max_98390_set_codec_conf, SND_SOC_INTEL_SOF_MAXIM_COMMON); diff --git a/sound/soc/intel/boards/sof_maxim_common.h b/sound/soc/intel/boards/sof_maxim_common.h index 7a8c53049e4d..a3676d68cc12 100644 --- a/sound/soc/intel/boards/sof_maxim_common.h +++ b/sound/soc/intel/boards/sof_maxim_common.h @@ -27,18 +27,15 @@ int max_98373_trigger(struct snd_pcm_substream *substream, int cmd); /* * Maxim MAX98390 */ -#define MAX_98390_CODEC_DAI "max98390-aif1" -#define MAX_98390_DEV0_NAME "i2c-MX98390:00" -#define MAX_98390_DEV1_NAME "i2c-MX98390:01" -#define MAX_98390_DEV2_NAME "i2c-MX98390:02" -#define MAX_98390_DEV3_NAME "i2c-MX98390:03" - -extern struct snd_soc_dai_link_component max_98390_components[2]; -extern struct snd_soc_dai_link_component max_98390_4spk_components[4]; -extern const struct snd_soc_ops max_98390_ops; - -void max_98390_set_codec_conf(struct snd_soc_card *card, int ch); -int max_98390_spk_codec_init(struct snd_soc_pcm_runtime *rtd); +#define MAX_98390_ACPI_HID "MX98390" +#define MAX_98390_CODEC_DAI "max98390-aif1" +#define MAX_98390_DEV0_NAME "i2c-MX98390:00" +#define MAX_98390_DEV1_NAME "i2c-MX98390:01" +#define MAX_98390_DEV2_NAME "i2c-MX98390:02" +#define MAX_98390_DEV3_NAME "i2c-MX98390:03" + +void max_98390_dai_link(struct snd_soc_dai_link *link); +void max_98390_set_codec_conf(struct snd_soc_card *card);
/* * Maxim MAX98357A/MAX98360A
On 7/20/23 11:26, Brent Lu wrote:
Implement a helper function to get number of codecs from ACPI subsystem to remove the need of quirk flag in machine driver.
Signed-off-by: Brent Lu brent.lu@intel.com
sound/soc/intel/boards/sof_maxim_common.c | 174 +++++++++++++--------- sound/soc/intel/boards/sof_maxim_common.h | 21 ++- 2 files changed, 113 insertions(+), 82 deletions(-)
diff --git a/sound/soc/intel/boards/sof_maxim_common.c b/sound/soc/intel/boards/sof_maxim_common.c index 112e89951da0..f8b44a81fec1 100644 --- a/sound/soc/intel/boards/sof_maxim_common.c +++ b/sound/soc/intel/boards/sof_maxim_common.c @@ -4,6 +4,7 @@ #include <linux/module.h> #include <linux/string.h> #include <sound/pcm.h> +#include <sound/pcm_params.h> #include <sound/soc.h> #include <sound/soc-acpi.h> #include <sound/soc-dai.h> @@ -11,6 +12,21 @@ #include <uapi/sound/asound.h> #include "sof_maxim_common.h"
+/* helper function to get the number of specific codec */ +static int get_num_codecs(const char *hid) +{
- struct acpi_device *adev = NULL;
- int dev_num = 0;
- do {
adev = acpi_dev_get_next_match_dev(adev, hid, NULL, -1);
Humm, I am a bit worried about reference counts.
See https://elixir.bootlin.com/linux/latest/source/drivers/acpi/utils.c#L916, it's not clear to me where the get() is done.
Adding Andy to make sure this is done right.
if (adev)dev_num++;- } while (adev != NULL);
- return dev_num;
+}
On Mon, Jul 24, 2023 at 11:08:17AM +0200, Pierre-Louis Bossart wrote:
On 7/20/23 11:26, Brent Lu wrote:
...
+/* helper function to get the number of specific codec */
...and leak a lot of reference counts...
+static int get_num_codecs(const char *hid) +{
- struct acpi_device *adev = NULL;
- int dev_num = 0;
- do {
adev = acpi_dev_get_next_match_dev(adev, hid, NULL, -1);Humm, I am a bit worried about reference counts.
See https://elixir.bootlin.com/linux/latest/source/drivers/acpi/utils.c#L916, it's not clear to me where the get() is done.
Adding Andy to make sure this is done right.
Thank you for Cc'ing.
Yes, the above code is problematic. One has to use the respective for_each macro (defined nearby the used API).
if (adev)dev_num++;- } while (adev != NULL);
- return dev_num;
+}
+/* helper function to get the number of specific codec */
...and leak a lot of reference counts...
+static int get_num_codecs(const char *hid) {
- struct acpi_device *adev = NULL;
- int dev_num = 0;
- do {
adev = acpi_dev_get_next_match_dev(adev, hid, NULL, -1);Humm, I am a bit worried about reference counts.
See https://elixir.bootlin.com/linux/latest/source/drivers/acpi/utils.c#L9 16, it's not clear to me where the get() is done.
Adding Andy to make sure this is done right.
Thank you for Cc'ing.
Yes, the above code is problematic. One has to use the respective for_each macro (defined nearby the used API).
if (adev)dev_num++;- } while (adev != NULL);
- return dev_num;
+}
-- With Best Regards, Andy Shevchenko
Hi Andy,
Each invocation of acpi_dev_get_next_match_dev() calls acpi_dev_put() to release the adev from previous call. And the last call returns NULL. It seems to me the reference count should be fine. Is my understanding correct?
I saw the macro for_each_acpi_dev_match and re-write the function as follow. Thanks for suggesting using the macro.
/* helper function to get the number of specific codec */ static int get_num_codecs(const char *hid) { struct acpi_device *adev; int dev_num = 0;
for_each_acpi_dev_match(adev, hid, NULL, -1) dev_num++;
return dev_num; }
Will test it in next few days.
Regards, Brent
On Mon, Jul 24, 2023 at 11:06:02AM +0000, Lu, Brent wrote:
+/* helper function to get the number of specific codec */
...and leak a lot of reference counts...
+static int get_num_codecs(const char *hid) {
- struct acpi_device *adev = NULL;
- int dev_num = 0;
- do {
adev = acpi_dev_get_next_match_dev(adev, hid, NULL, -1);Humm, I am a bit worried about reference counts.
See https://elixir.bootlin.com/linux/latest/source/drivers/acpi/utils.c#L9 16, it's not clear to me where the get() is done.
Adding Andy to make sure this is done right.
Thank you for Cc'ing.
Yes, the above code is problematic. One has to use the respective for_each macro (defined nearby the used API).
if (adev)dev_num++;- } while (adev != NULL);
- return dev_num;
+}
Each invocation of acpi_dev_get_next_match_dev() calls acpi_dev_put() to release the adev from previous call. And the last call returns NULL. It seems to me the reference count should be fine. Is my understanding correct?
Ah, right. sorry for the confusion. That's why we have a macro to not think about these details :-)
I saw the macro for_each_acpi_dev_match and re-write the function as follow. Thanks for suggesting using the macro.
/* helper function to get the number of specific codec */ static int get_num_codecs(const char *hid) { struct acpi_device *adev;
int dev_num = 0;
size_t here or at least unsigned int is more correct.
for_each_acpi_dev_match(adev, hid, NULL, -1) dev_num++;
return dev_num; }
Otherwise, yes, that's what I have in mind.
Will test it in next few days.
size_t here or at least unsigned int is more correct.
for_each_acpi_dev_match(adev, hid, NULL, -1) dev_num++;
return dev_num; }
Otherwise, yes, that's what I have in mind.
Will test it in next few days.
-- With Best Regards, Andy Shevchenko
Thanks for the review. I've modified the v2 patch accordingly.
Regards, Brent
On 7/20/2023 5:26 PM, Brent Lu wrote:
Implement a helper function to get number of codecs from ACPI subsystem to remove the need of quirk flag in machine driver.
Signed-off-by: Brent Lu brent.lu@intel.com
sound/soc/intel/boards/sof_maxim_common.c | 174 +++++++++++++--------- sound/soc/intel/boards/sof_maxim_common.h | 21 ++- 2 files changed, 113 insertions(+), 82 deletions(-)
diff --git a/sound/soc/intel/boards/sof_maxim_common.c b/sound/soc/intel/boards/sof_maxim_common.c index 112e89951da0..f8b44a81fec1 100644 --- a/sound/soc/intel/boards/sof_maxim_common.c +++ b/sound/soc/intel/boards/sof_maxim_common.c @@ -4,6 +4,7 @@ #include <linux/module.h> #include <linux/string.h> #include <sound/pcm.h> +#include <sound/pcm_params.h> #include <sound/soc.h> #include <sound/soc-acpi.h> #include <sound/soc-dai.h> @@ -11,6 +12,21 @@ #include <uapi/sound/asound.h> #include "sof_maxim_common.h"
+/* helper function to get the number of specific codec */ +static int get_num_codecs(const char *hid) +{
- struct acpi_device *adev = NULL;
- int dev_num = 0;
- do {
adev = acpi_dev_get_next_match_dev(adev, hid, NULL, -1);if (adev)dev_num++;- } while (adev != NULL);
- return dev_num;
+}
#define MAX_98373_PIN_NAME 16
const struct snd_soc_dapm_route max_98373_dapm_routes[] = {
@@ -168,17 +184,6 @@ static struct snd_soc_codec_conf max_98390_codec_conf[] = { .dlc = COMP_CODEC_CONF(MAX_98390_DEV1_NAME), .name_prefix = "Left", }, -};
-static struct snd_soc_codec_conf max_98390_4spk_codec_conf[] = {
- {
.dlc = COMP_CODEC_CONF(MAX_98390_DEV0_NAME),.name_prefix = "Right",- },
- {
.dlc = COMP_CODEC_CONF(MAX_98390_DEV1_NAME),.name_prefix = "Left",- }, { .dlc = COMP_CODEC_CONF(MAX_98390_DEV2_NAME), .name_prefix = "Tweeter Right",
@@ -189,19 +194,7 @@ static struct snd_soc_codec_conf max_98390_4spk_codec_conf[] = { }, };
-struct snd_soc_dai_link_component max_98390_components[] = {
- {
.name = MAX_98390_DEV0_NAME,.dai_name = MAX_98390_CODEC_DAI,- },
- {
.name = MAX_98390_DEV1_NAME,.dai_name = MAX_98390_CODEC_DAI,- },
-}; -EXPORT_SYMBOL_NS(max_98390_components, SND_SOC_INTEL_SOF_MAXIM_COMMON);
-struct snd_soc_dai_link_component max_98390_4spk_components[] = {
max_98390_components[] and max_98390_4spk_components[] are still used
by sof_rt5682.c, we should remove them in the same patch.
Maybe combine the 2 patches together?
+static struct snd_soc_dai_link_component max_98390_components[] = { { .name = MAX_98390_DEV0_NAME, .dai_name = MAX_98390_CODEC_DAI, @@ -219,62 +212,56 @@ struct snd_soc_dai_link_component max_98390_4spk_components[] = { .dai_name = MAX_98390_CODEC_DAI, }, }; -EXPORT_SYMBOL_NS(max_98390_4spk_components, SND_SOC_INTEL_SOF_MAXIM_COMMON);
+static const struct {
- unsigned int tx;
- unsigned int rx;
+} max_98390_tdm_mask[] = {
- {.tx = 0x01, .rx = 0x3},
- {.tx = 0x02, .rx = 0x3},
- {.tx = 0x04, .rx = 0x3},
- {.tx = 0x08, .rx = 0x3},
+};
static int max_98390_hw_params(struct snd_pcm_substream *substream, struct snd_pcm_hw_params *params) { struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream); struct snd_soc_dai *codec_dai;
- int i;
int i, ret = 0;
for_each_rtd_codec_dais(rtd, i, codec_dai) {
if (i >= ARRAY_SIZE(max_98390_4spk_components)) {
if (i >= ARRAY_SIZE(max_98390_tdm_mask)) { dev_err(codec_dai->dev, "invalid codec index %d\n", i); return -ENODEV;
if (!strcmp(codec_dai->component->name, MAX_98390_DEV0_NAME)) {/* DEV0 tdm slot configuration Right */snd_soc_dai_set_tdm_slot(codec_dai, 0x01, 3, 4, 32);}if (!strcmp(codec_dai->component->name, MAX_98390_DEV1_NAME)) {/* DEV1 tdm slot configuration Left */snd_soc_dai_set_tdm_slot(codec_dai, 0x02, 3, 4, 32);}if (!strcmp(codec_dai->component->name, MAX_98390_DEV2_NAME)) {/* DEVi2 tdm slot configuration Tweeter Right */snd_soc_dai_set_tdm_slot(codec_dai, 0x04, 3, 4, 32);}if (!strcmp(codec_dai->component->name, MAX_98390_DEV3_NAME)) {/* DEV3 tdm slot configuration Tweeter Left */snd_soc_dai_set_tdm_slot(codec_dai, 0x08, 3, 4, 32);
ret = snd_soc_dai_set_tdm_slot(codec_dai, max_98390_tdm_mask[i].tx,max_98390_tdm_mask[i].rx, 4,params_width(params));if (ret < 0) {dev_err(codec_dai->dev, "fail to set tdm slot, ret %d\n",ret);return ret;-int max_98390_spk_codec_init(struct snd_soc_pcm_runtime *rtd) +static int max_98390_init(struct snd_soc_pcm_runtime *rtd) { struct snd_soc_card *card = rtd->card;
- int num_codecs = get_num_codecs(MAX_98390_ACPI_HID); int ret;
- /* add regular speakers dapm route */
- ret = snd_soc_dapm_add_routes(&card->dapm, max_98390_dapm_routes,
ARRAY_SIZE(max_98390_dapm_routes));
Don't we need to add max_98390_dapm_routes for the 4 speakers case?
- if (ret) {
dev_err(rtd->dev, "unable to add Left/Right Speaker dapm, ret %d\n", ret);return ret;- }
- /* add widgets/controls/dapm for tweeter speakers */
- if (acpi_dev_present("MX98390", "3", -1)) {
- switch (num_codecs) {
- case 4:
/* add widgets/controls/dapm for tweeter speakers */
- if (ret) {
dev_err(rtd->dev, "unable to add tweeter dapm controls, ret %d\n", ret);
dev_err(rtd->dev, "unable to add tweeter dapm widgets, ret %d\n",ret); /* Don't need to add routes if widget addition failed */ return ret;@@ -282,33 +269,80 @@ int max_98390_spk_codec_init(struct snd_soc_pcm_runtime *rtd) ret = snd_soc_add_card_controls(card, max_98390_tt_kcontrols, ARRAY_SIZE(max_98390_tt_kcontrols)); if (ret) {
dev_err(rtd->dev, "unable to add tweeter card controls, ret %d\n", ret);
dev_err(rtd->dev, "unable to add tweeter controls, ret %d\n",ret); return ret;}
ret = snd_soc_dapm_add_routes(&card->dapm, max_98390_tt_dapm_routes, ARRAY_SIZE(max_98390_tt_dapm_routes));
if (ret)dev_err(rtd->dev,"unable to add Tweeter Left/Right Speaker dapm, ret %d\n", ret);
if (ret) {dev_err(rtd->dev, "unable to add tweeter dapm routes, ret %d\n",ret);return ret;}fallthrough;- case 2:
/* add regular speakers dapm route */ret = snd_soc_dapm_add_routes(&card->dapm, max_98390_dapm_routes,ARRAY_SIZE(max_98390_dapm_routes));if (ret) {dev_err(rtd->dev, "unable to add dapm routes, ret %d\n",ret);return ret;}break;- default:
dev_err(rtd->dev, "invalid codec number %d\n", num_codecs);ret = -EINVAL;break;- return ret; }
-EXPORT_SYMBOL_NS(max_98390_spk_codec_init, SND_SOC_INTEL_SOF_MAXIM_COMMON);
-const struct snd_soc_ops max_98390_ops = { +static const struct snd_soc_ops max_98390_ops = { .hw_params = max_98390_hw_params, }; -EXPORT_SYMBOL_NS(max_98390_ops, SND_SOC_INTEL_SOF_MAXIM_COMMON);
-void max_98390_set_codec_conf(struct snd_soc_card *card, int ch) +void max_98390_dai_link(struct snd_soc_dai_link *link) +{
- int num_codecs = get_num_codecs(MAX_98390_ACPI_HID);
- link->codecs = max_98390_components;
- switch (num_codecs) {
- case 2:
- case 4:
link->num_codecs = num_codecs;break;- default:
pr_err("invalid codec number %d for %s\n", num_codecs,MAX_98390_ACPI_HID);break;- }
- link->init = max_98390_init;
- link->ops = &max_98390_ops;
+} +EXPORT_SYMBOL_NS(max_98390_dai_link, SND_SOC_INTEL_SOF_MAXIM_COMMON);
+void max_98390_set_codec_conf(struct snd_soc_card *card) {
- if (ch == ARRAY_SIZE(max_98390_4spk_codec_conf)) {
card->codec_conf = max_98390_4spk_codec_conf;card->num_configs = ARRAY_SIZE(max_98390_4spk_codec_conf);- } else {
card->codec_conf = max_98390_codec_conf;card->num_configs = ARRAY_SIZE(max_98390_codec_conf);
- int num_codecs = get_num_codecs(MAX_98390_ACPI_HID);
- card->codec_conf = max_98390_codec_conf;
- switch (num_codecs) {
- case 2:
- case 4:
card->num_configs = num_codecs;break;- default:
pr_err("invalid codec number %d for %s\n", num_codecs,MAX_98390_ACPI_HID);break;diff --git a/sound/soc/intel/boards/sof_maxim_common.h b/sound/soc/intel/boards/sof_maxim_common.h index 7a8c53049e4d..a3676d68cc12 100644 --- a/sound/soc/intel/boards/sof_maxim_common.h +++ b/sound/soc/intel/boards/sof_maxim_common.h @@ -27,18 +27,15 @@ int max_98373_trigger(struct snd_pcm_substream *substream, int cmd); /*
- Maxim MAX98390
*/ -#define MAX_98390_CODEC_DAI "max98390-aif1" -#define MAX_98390_DEV0_NAME "i2c-MX98390:00" -#define MAX_98390_DEV1_NAME "i2c-MX98390:01" -#define MAX_98390_DEV2_NAME "i2c-MX98390:02" -#define MAX_98390_DEV3_NAME "i2c-MX98390:03"
-extern struct snd_soc_dai_link_component max_98390_components[2]; -extern struct snd_soc_dai_link_component max_98390_4spk_components[4]; -extern const struct snd_soc_ops max_98390_ops;
-void max_98390_set_codec_conf(struct snd_soc_card *card, int ch); -int max_98390_spk_codec_init(struct snd_soc_pcm_runtime *rtd); +#define MAX_98390_ACPI_HID "MX98390" +#define MAX_98390_CODEC_DAI "max98390-aif1" +#define MAX_98390_DEV0_NAME "i2c-MX98390:00" +#define MAX_98390_DEV1_NAME "i2c-MX98390:01" +#define MAX_98390_DEV2_NAME "i2c-MX98390:02" +#define MAX_98390_DEV3_NAME "i2c-MX98390:03"
+void max_98390_dai_link(struct snd_soc_dai_link *link); +void max_98390_set_codec_conf(struct snd_soc_card *card);
/*
- Maxim MAX98357A/MAX98360A
On 7/26/2023 4:47 PM, Liao, Bard wrote:
;
- /* add regular speakers dapm route */ - ret = snd_soc_dapm_add_routes(&card->dapm, max_98390_dapm_routes, - ARRAY_SIZE(max_98390_dapm_routes));
Don't we need to add max_98390_dapm_routes for the 4 speakers case?
Please ignore this comment. I didn't notice that it is a fallthrough not
break in the end of case 4.
max_98390_components[] and max_98390_4spk_components[] are still used
by sof_rt5682.c, we should remove them in the same patch.
Maybe combine the 2 patches together?
I've got your point but these two patches are doing two things: one is refactor the code and add a detection feature, the other one is removing the quirk. Not sure if it's proper to merge them.
Regards, Brent
On 7/27/2023 10:14 AM, Lu, Brent wrote:
max_98390_components[] and max_98390_4spk_components[] are still used
by sof_rt5682.c, we should remove them in the same patch.
Maybe combine the 2 patches together?
I've got your point but these two patches are doing two things: one is refactor the code and add a detection feature, the other one is removing the quirk. Not sure if it's proper to merge them.
The point is that if you remove them and they are still used somewhere,
you will break the build. i.e. Kernel will not compile if we apply the
first patch only.
Regards, Brent
On 7/27/23 5:21 AM, Liao, Bard wrote:
On 7/27/2023 10:14 AM, Lu, Brent wrote:
max_98390_components[] and max_98390_4spk_components[] are still used
by sof_rt5682.c, we should remove them in the same patch.
Maybe combine the 2 patches together?
I've got your point but these two patches are doing two things: one is refactor the code and add a detection feature, the other one is removing the quirk. Not sure if it's proper to merge them.
The point is that if you remove them and they are still used somewhere,
you will break the build. i.e. Kernel will not compile if we apply the
first patch only.
IOW git bisect is broken and that's a big no-no.
On Thu, Jul 27, 2023 at 07:41:53AM +0200, Pierre-Louis Bossart wrote:
On 7/27/23 5:21 AM, Liao, Bard wrote:
The point is that if you remove them and they are still used somewhere,
you will break the build. i.e. Kernel will not compile if we apply the
first patch only.
IOW git bisect is broken and that's a big no-no.
Yes, I build test every patch so I'll notice.
The point is that if you remove them and they are still used somewhere,
you will break the build. i.e. Kernel will not compile if we apply the
first patch only.
Thanks for pointing this out. I will merge these two patches to avoid build break in v4 patch.
Regards, Brent
Remove the SOF_MAX98390_TWEETER_SPEAKER_PRESENT flag from driver since the number of amplifiers could be queried from ACPI by counting the device instance.
Signed-off-by: Brent Lu brent.lu@intel.com --- sound/soc/intel/boards/sof_rt5682.c | 37 ++--------------------------- 1 file changed, 2 insertions(+), 35 deletions(-)
diff --git a/sound/soc/intel/boards/sof_rt5682.c b/sound/soc/intel/boards/sof_rt5682.c index b4f07bdcf8b4..9116f4df4752 100644 --- a/sound/soc/intel/boards/sof_rt5682.c +++ b/sound/soc/intel/boards/sof_rt5682.c @@ -59,7 +59,6 @@ #define SOF_SSP_BT_OFFLOAD_PRESENT BIT(22) #define SOF_RT5682S_HEADPHONE_CODEC_PRESENT BIT(23) #define SOF_MAX98390_SPEAKER_AMP_PRESENT BIT(24) -#define SOF_MAX98390_TWEETER_SPEAKER_PRESENT BIT(25) #define SOF_RT1019_SPEAKER_AMP_PRESENT BIT(26) #define SOF_RT5650_HEADPHONE_CODEC_PRESENT BIT(27)
@@ -195,23 +194,6 @@ static const struct dmi_system_id sof_rt5682_quirk_table[] = { SOF_RT5682_SSP_AMP(2) | SOF_RT5682_NUM_HDMIDEV(4)), }, - { - .callback = sof_rt5682_quirk_cb, - .matches = { - DMI_MATCH(DMI_PRODUCT_FAMILY, "Google_Brya"), - DMI_MATCH(DMI_OEM_STRING, "AUDIO-MAX98390_ALC5682I_I2S_4SPK"), - }, - .driver_data = (void *)(SOF_RT5682_MCLK_EN | - SOF_RT5682_SSP_CODEC(0) | - SOF_SPEAKER_AMP_PRESENT | - SOF_MAX98390_SPEAKER_AMP_PRESENT | - SOF_MAX98390_TWEETER_SPEAKER_PRESENT | - SOF_RT5682_SSP_AMP(1) | - SOF_RT5682_NUM_HDMIDEV(4) | - SOF_BT_OFFLOAD_SSP(2) | - SOF_SSP_BT_OFFLOAD_PRESENT), - - }, { .callback = sof_rt5682_quirk_cb, .matches = { @@ -850,17 +832,7 @@ static struct snd_soc_dai_link *sof_card_dai_links_create(struct device *dev, sof_rt1011_dai_link(&links[id]); } else if (sof_rt5682_quirk & SOF_MAX98390_SPEAKER_AMP_PRESENT) { - if (sof_rt5682_quirk & - SOF_MAX98390_TWEETER_SPEAKER_PRESENT) { - links[id].codecs = max_98390_4spk_components; - links[id].num_codecs = ARRAY_SIZE(max_98390_4spk_components); - } else { - links[id].codecs = max_98390_components; - links[id].num_codecs = ARRAY_SIZE(max_98390_components); - } - links[id].init = max_98390_spk_codec_init; - links[id].ops = &max_98390_ops; - + max_98390_dai_link(&links[id]); } else if (sof_rt5682_quirk & SOF_RT5650_HEADPHONE_CODEC_PRESENT) { links[id].codecs = &rt5650_components[1]; links[id].num_codecs = 1; @@ -1019,12 +991,7 @@ static int sof_audio_probe(struct platform_device *pdev) else if (sof_rt5682_quirk & SOF_RT1015P_SPEAKER_AMP_PRESENT) sof_rt1015p_codec_conf(&sof_audio_card_rt5682); else if (sof_rt5682_quirk & SOF_MAX98390_SPEAKER_AMP_PRESENT) { - if (sof_rt5682_quirk & SOF_MAX98390_TWEETER_SPEAKER_PRESENT) - max_98390_set_codec_conf(&sof_audio_card_rt5682, - ARRAY_SIZE(max_98390_4spk_components)); - else - max_98390_set_codec_conf(&sof_audio_card_rt5682, - ARRAY_SIZE(max_98390_components)); + max_98390_set_codec_conf(&sof_audio_card_rt5682); }
if (sof_rt5682_quirk & SOF_SSP_BT_OFFLOAD_PRESENT)
Hi,
On Thu, 20 Jul 2023, Brent Lu wrote:
We add a helper funcion to detect amplifier number according to device instance in ACPI table so the SOF_MAX98390_TWEETER_SPEAKER_PRESENT flag and a dmi quirk for 4-amplifier configuration could be safely removed.
Also refactory the max_98390_hw_params() function to use an array to handle the TDM parameter.
Amplifier number detection and TDM parameter are tested on two Chromebooks. One with 2 MAX98390 and one with 4 MAX98390 amplifier.
*** BLURB HERE ***
this looks like a nice cleanup, thanks Brent. For the series:
Acked-by: Kai Vehmanen kai.vehmanen@linux.intel.com
Minor nit: some spelling erros in cover letter (funcion->function, refactory->refactor, BLURB HERE left), but I think the intent comes across.
Br, Kai
participants (7)
-
Andy Shevchenko -
Brent Lu -
Kai Vehmanen -
Liao, Bard -
Lu, Brent
-
Mark Brown -
Pierre-Louis Bossart