[alsa-devel] [PATCH RESEND 0/3] Intel-rt5670 updates
[when I started cleaning-up some patches reviewed by Andy Shevchenko, I realized this initial batch has not been merged, although both Takashi and Vinod reviewed them, see http://mailman.alsa-project.org/pipermail/alsa-devel/2017-September/125331.h... no change except for the addition of Takashi's Reviewed-by and Vinod's Acked-by. I don't mean to be pushy or anything, just checking if something is missing ]
Instead of submitting all my patches at ones, I broke them by platforms/codecs. This series is only for Intel-rt5670 platforms. It fixes a mistake on the Dell Venue 5585 - corrected with the help from Realtek and Dell who kindly provided schematics and data sheet information needed to fix non-working DMIC capture. Since there are still some reports of non-functional audio on some platforms, I added quirks that can be changed from the kernel command line (and later added as DMI-based quirks). The last two patches are minor corrections to fix ACPI HID naming issues and use a more meaningful card name.
Pierre-Louis Bossart (3): ASoC: rt5670: refactor DMI quirks and fix Dell Venue settings ASoC: Intel: cht_bsw_rt5672: use actual HID in suspend/resume ASoC: Intel: cht_bsw_rt5672: fix card name
sound/soc/codecs/rt5670.c | 124 +++++++++++++++++++++++++++----- sound/soc/intel/boards/cht_bsw_rt5672.c | 10 ++- 2 files changed, 112 insertions(+), 22 deletions(-)
Additional checks exposed a mistake in the quirk for the Dell Venue Pro 5855 (Dmic2 instead of Dmic1). Rather than adding quirk tables, merge all quirks in a single table and use flags to differentiate platforms. Also add a parameter override to help support additional platforms using this codec
CC: Bard Liao bardliao@realtek.com Reviewed-by: Takashi Iwai tiwai@suse.de Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- sound/soc/codecs/rt5670.c | 124 +++++++++++++++++++++++++++++++++++++++------- 1 file changed, 105 insertions(+), 19 deletions(-)
diff --git a/sound/soc/codecs/rt5670.c b/sound/soc/codecs/rt5670.c index 9545764ef3eb..be945b345b43 100644 --- a/sound/soc/codecs/rt5670.c +++ b/sound/soc/codecs/rt5670.c @@ -34,6 +34,24 @@ #include "rt5670.h" #include "rt5670-dsp.h"
+#define RT5670_DEV_GPIO BIT(0) +#define RT5670_IN2_DIFF BIT(1) +#define RT5670_DMIC_EN BIT(2) +#define RT5670_DMIC1_IN2P BIT(3) +#define RT5670_DMIC1_GPIO6 BIT(4) +#define RT5670_DMIC1_GPIO7 BIT(5) +#define RT5670_DMIC2_INR BIT(6) +#define RT5670_DMIC2_GPIO8 BIT(7) +#define RT5670_DMIC3_GPIO5 BIT(8) +#define RT5670_JD_MODE1 BIT(9) +#define RT5670_JD_MODE2 BIT(10) +#define RT5670_JD_MODE3 BIT(11) + +static unsigned long rt5670_quirk; +static unsigned int quirk_override; +module_param_named(quirk, quirk_override, uint, 0444); +MODULE_PARM_DESC(quirk, "Board-specific quirk override"); + #define RT5670_DEVICE_ID 0x6271
#define RT5670_PR_RANGE_BASE (0xff + 1) @@ -2808,56 +2826,84 @@ static const struct acpi_device_id rt5670_acpi_match[] = { MODULE_DEVICE_TABLE(acpi, rt5670_acpi_match); #endif
-static const struct dmi_system_id dmi_platform_intel_braswell[] = { +static int rt5670_quirk_cb(const struct dmi_system_id *id) +{ + rt5670_quirk = (unsigned long)id->driver_data; + return 1; +} + +static const struct dmi_system_id dmi_platform_intel_quirks[] = { { + .callback = rt5670_quirk_cb, .ident = "Intel Braswell", .matches = { DMI_MATCH(DMI_SYS_VENDOR, "Intel Corporation"), DMI_MATCH(DMI_BOARD_NAME, "Braswell CRB"), }, + .driver_data = (unsigned long *)(RT5670_DMIC_EN | + RT5670_DMIC1_IN2P | + RT5670_DEV_GPIO | + RT5670_JD_MODE1), }, { + .callback = rt5670_quirk_cb, .ident = "Dell Wyse 3040", .matches = { DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."), DMI_MATCH(DMI_PRODUCT_NAME, "Wyse 3040"), }, + .driver_data = (unsigned long *)(RT5670_DMIC_EN | + RT5670_DMIC1_IN2P | + RT5670_DEV_GPIO | + RT5670_JD_MODE1), }, { + .callback = rt5670_quirk_cb, .ident = "Lenovo Thinkpad Tablet 10", .matches = { DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"), DMI_MATCH(DMI_PRODUCT_VERSION, "ThinkPad 10"), }, + .driver_data = (unsigned long *)(RT5670_DMIC_EN | + RT5670_DMIC1_IN2P | + RT5670_DEV_GPIO | + RT5670_JD_MODE1), }, { + .callback = rt5670_quirk_cb, .ident = "Lenovo Thinkpad Tablet 10", .matches = { DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"), DMI_MATCH(DMI_PRODUCT_VERSION, "ThinkPad Tablet B"), }, + .driver_data = (unsigned long *)(RT5670_DMIC_EN | + RT5670_DMIC1_IN2P | + RT5670_DEV_GPIO | + RT5670_JD_MODE1), }, - {} -}; - -static const struct dmi_system_id dmi_platform_intel_bytcht_jdmode2[] = { { + .callback = rt5670_quirk_cb, .ident = "Lenovo Thinkpad Tablet 10", .matches = { DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"), DMI_MATCH(DMI_PRODUCT_VERSION, "Lenovo Miix 2 10"), }, + .driver_data = (unsigned long *)(RT5670_DMIC_EN | + RT5670_DMIC1_IN2P | + RT5670_DEV_GPIO | + RT5670_JD_MODE2), }, - {} -}; - -static const struct dmi_system_id dmi_platform_intel_bytcht_jdmode3[] = { { + .callback = rt5670_quirk_cb, .ident = "Dell Venue 8 Pro 5855", .matches = { DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."), DMI_MATCH(DMI_PRODUCT_NAME, "Venue 8 Pro 5855"), }, + .driver_data = (unsigned long *)(RT5670_DMIC_EN | + RT5670_DMIC2_INR | + RT5670_DEV_GPIO | + RT5670_JD_MODE3), }, {} }; @@ -2881,21 +2927,61 @@ static int rt5670_i2c_probe(struct i2c_client *i2c, if (pdata) rt5670->pdata = *pdata;
- if (dmi_check_system(dmi_platform_intel_braswell)) { - rt5670->pdata.dmic_en = true; - rt5670->pdata.dmic1_data_pin = RT5670_DMIC_DATA_IN2P; + dmi_check_system(dmi_platform_intel_quirks); + if (quirk_override) { + dev_info(&i2c->dev, "Overriding quirk 0x%x => 0x%x\n", + (unsigned int)rt5670_quirk, quirk_override); + rt5670_quirk = quirk_override; + } + + if (rt5670_quirk & RT5670_DEV_GPIO) { rt5670->pdata.dev_gpio = true; - rt5670->pdata.jd_mode = 1; - } else if (dmi_check_system(dmi_platform_intel_bytcht_jdmode2)) { + dev_info(&i2c->dev, "quirk dev_gpio\n"); + } + if (rt5670_quirk & RT5670_IN2_DIFF) { + rt5670->pdata.in2_diff = true; + dev_info(&i2c->dev, "quirk IN2_DIFF\n"); + } + if (rt5670_quirk & RT5670_DMIC_EN) { rt5670->pdata.dmic_en = true; + dev_info(&i2c->dev, "quirk DMIC enabled\n"); + } + if (rt5670_quirk & RT5670_DMIC1_IN2P) { rt5670->pdata.dmic1_data_pin = RT5670_DMIC_DATA_IN2P; - rt5670->pdata.dev_gpio = true; + dev_info(&i2c->dev, "quirk DMIC1 on IN2P pin\n"); + } + if (rt5670_quirk & RT5670_DMIC1_GPIO6) { + rt5670->pdata.dmic1_data_pin = RT5670_DMIC_DATA_GPIO6; + dev_info(&i2c->dev, "quirk DMIC1 on GPIO6 pin\n"); + } + if (rt5670_quirk & RT5670_DMIC1_GPIO7) { + rt5670->pdata.dmic1_data_pin = RT5670_DMIC_DATA_GPIO7; + dev_info(&i2c->dev, "quirk DMIC1 on GPIO7 pin\n"); + } + if (rt5670_quirk & RT5670_DMIC2_INR) { + rt5670->pdata.dmic2_data_pin = RT5670_DMIC_DATA_IN3N; + dev_info(&i2c->dev, "quirk DMIC2 on INR pin\n"); + } + if (rt5670_quirk & RT5670_DMIC2_GPIO8) { + rt5670->pdata.dmic2_data_pin = RT5670_DMIC_DATA_GPIO8; + dev_info(&i2c->dev, "quirk DMIC2 on GPIO8 pin\n"); + } + if (rt5670_quirk & RT5670_DMIC3_GPIO5) { + rt5670->pdata.dmic3_data_pin = RT5670_DMIC_DATA_GPIO5; + dev_info(&i2c->dev, "quirk DMIC3 on GPIO5 pin\n"); + } + + if (rt5670_quirk & RT5670_JD_MODE1) { + rt5670->pdata.jd_mode = 1; + dev_info(&i2c->dev, "quirk JD mode 1\n"); + } + if (rt5670_quirk & RT5670_JD_MODE2) { rt5670->pdata.jd_mode = 2; - } else if (dmi_check_system(dmi_platform_intel_bytcht_jdmode3)) { - rt5670->pdata.dmic_en = true; - rt5670->pdata.dmic1_data_pin = RT5670_DMIC_DATA_IN2P; - rt5670->pdata.dev_gpio = true; + dev_info(&i2c->dev, "quirk JD mode 2\n"); + } + if (rt5670_quirk & RT5670_JD_MODE3) { rt5670->pdata.jd_mode = 3; + dev_info(&i2c->dev, "quirk JD mode 3\n"); }
rt5670->regmap = devm_regmap_init_i2c(i2c, &rt5670_regmap);
The code scans all components looking for the default name i2c-10EC5670:00, which of course doesn't work in platforms where the BIOS uses a different HID such as Dell 5585
Since we already have the correct information available, just use the actual codec name and length.
Acked-by: Vinod Koul vinod.koul@intel.com Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- sound/soc/intel/boards/cht_bsw_rt5672.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/sound/soc/intel/boards/cht_bsw_rt5672.c b/sound/soc/intel/boards/cht_bsw_rt5672.c index a0e60bc1f84f..5b1e3ee965a7 100644 --- a/sound/soc/intel/boards/cht_bsw_rt5672.c +++ b/sound/soc/intel/boards/cht_bsw_rt5672.c @@ -347,9 +347,11 @@ static struct snd_soc_dai_link cht_dailink[] = { static int cht_suspend_pre(struct snd_soc_card *card) { struct snd_soc_component *component; + struct cht_mc_private *ctx = snd_soc_card_get_drvdata(card);
list_for_each_entry(component, &card->component_dev_list, card_list) { - if (!strcmp(component->name, "i2c-10EC5670:00")) { + if (!strncmp(component->name, + ctx->codec_name, sizeof(ctx->codec_name))) { struct snd_soc_codec *codec = snd_soc_component_to_codec(component);
dev_dbg(codec->dev, "disabling jack detect before going to suspend.\n"); @@ -363,9 +365,11 @@ static int cht_suspend_pre(struct snd_soc_card *card) static int cht_resume_post(struct snd_soc_card *card) { struct snd_soc_component *component; + struct cht_mc_private *ctx = snd_soc_card_get_drvdata(card);
list_for_each_entry(component, &card->component_dev_list, card_list) { - if (!strcmp(component->name, "i2c-10EC5670:00")) { + if (!strncmp(component->name, + ctx->codec_name, sizeof(ctx->codec_name))) { struct snd_soc_codec *codec = snd_soc_component_to_codec(component);
dev_dbg(codec->dev, "enabling jack detect for resume.\n");
For now reason the current card name is a ridiculous 'cherrytrailcraudio'. This isn't very useful or self-explanatory, change to driver name cht-bsw-rt5672.
[Additional comment from Takashi: One bonus by this change would be to avoid inconsistent names by the driver's name string size. Since it's 16 bytes including \0, the name is shortened as cherrytrailcrau or such in some fields. With the new name this shouldn't happen ]
Reviewed-by: Takashi Iwai tiwai@suse.de Acked-by: Vinod Koul vinod.koul@intel.com Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- sound/soc/intel/boards/cht_bsw_rt5672.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/intel/boards/cht_bsw_rt5672.c b/sound/soc/intel/boards/cht_bsw_rt5672.c index 5b1e3ee965a7..f799b76fdedf 100644 --- a/sound/soc/intel/boards/cht_bsw_rt5672.c +++ b/sound/soc/intel/boards/cht_bsw_rt5672.c @@ -383,7 +383,7 @@ static int cht_resume_post(struct snd_soc_card *card)
/* SoC card */ static struct snd_soc_card snd_soc_card_cht = { - .name = "cherrytrailcraudio", + .name = "cht-bsw-rt5672", .owner = THIS_MODULE, .dai_link = cht_dailink, .num_links = ARRAY_SIZE(cht_dailink),
On Tue, Sep 19, 2017 at 07:34:50PM -0500, Pierre-Louis Bossart wrote:
[when I started cleaning-up some patches reviewed by Andy Shevchenko, I realized this initial batch has not been merged, although both Takashi and Vinod reviewed them, see http://mailman.alsa-project.org/pipermail/alsa-devel/2017-September/125331.h...
Please allow a reasonable time for review. People get busy, go on holiday, attend conferences and so on so unless there is some reason for urgency (like critical bug fixes) please allow at least a couple of weeks for review. It's been less than two weeks, including both the merge window and LPC...
On 9/20/17 5:41 AM, Mark Brown wrote:
On Tue, Sep 19, 2017 at 07:34:50PM -0500, Pierre-Louis Bossart wrote:
[when I started cleaning-up some patches reviewed by Andy Shevchenko, I realized this initial batch has not been merged, although both Takashi and Vinod reviewed them, see http://mailman.alsa-project.org/pipermail/alsa-devel/2017-September/125331.h...
Please allow a reasonable time for review. People get busy, go on holiday, attend conferences and so on so unless there is some reason for urgency (like critical bug fixes) please allow at least a couple of weeks for review. It's been less than two weeks, including both the merge window and LPC...
Sure. it's just that you already merged a number of patches sent *after* this batch and they aren't queued in your topic/intel branch either.
On Wed, Sep 20, 2017 at 08:17:09AM -0500, Pierre-Louis Bossart wrote:
On 9/20/17 5:41 AM, Mark Brown wrote:
Please allow a reasonable time for review. People get busy, go on holiday, attend conferences and so on so unless there is some reason for urgency (like critical bug fixes) please allow at least a couple of weeks for review. It's been less than two weeks, including both the merge window and LPC...
Sure. it's just that you already merged a number of patches sent *after* this batch and they aren't queued in your topic/intel branch either.
You really shouldn't expect things to be handled FIFO.
participants (2)
-
Mark Brown -
Pierre-Louis Bossart