[alsa-devel] [PATCH] ASoC: rt5645: add fallback case for jack detection support
Commit 78f5605c0329 ("ASoC: rt5645: cleanup DMI matching code") did a lot of useful cleanups, but the logic is a bit flawed: if there is no DMI match on an ACPI machine, the code reads non-existent properties.
Re-add previous code that checked for the existence of those properties and a fallback mode to enable jd-mode3 if there is no pdata, device property or quirk. This will help avoid having to add new DMI quirks when only the jd-mode is needed for headset playback.
Cc: Hans de Goede hdegoede@redhat.com Cc: Bard Liao bardliao@realtek.com Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- sound/soc/codecs/rt5645.c | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-)
diff --git a/sound/soc/codecs/rt5645.c b/sound/soc/codecs/rt5645.c index ebc4f5970f24..476020445f65 100644 --- a/sound/soc/codecs/rt5645.c +++ b/sound/soc/codecs/rt5645.c @@ -3687,6 +3687,17 @@ static const struct dmi_system_id dmi_platform_data[] = { { } };
+static bool rt5645_check_dp(struct device *dev) +{ + if (device_property_present(dev, "realtek,in2-differential") || + device_property_present(dev, "realtek,dmic1-data-pin") || + device_property_present(dev, "realtek,dmic2-data-pin") || + device_property_present(dev, "realtek,jd-mode")) + return true; + + return false; +} + static int rt5645_parse_dt(struct rt5645_priv *rt5645, struct device *dev) { rt5645->pdata.in2_diff = device_property_read_bool(dev, @@ -3727,8 +3738,10 @@ static int rt5645_i2c_probe(struct i2c_client *i2c,
if (pdata) rt5645->pdata = *pdata; - else + else if (rt5645_check_dp(&i2c->dev)) rt5645_parse_dt(rt5645, &i2c->dev); + else + rt5645->pdata = jd_mode3_platform_data;
if (quirk != -1) { rt5645->pdata.in2_diff = QUIRK_IN2_DIFF(quirk);
Hi,
On 03-01-18 19:42, Pierre-Louis Bossart wrote:
Commit 78f5605c0329 ("ASoC: rt5645: cleanup DMI matching code") did a lot of useful cleanups, but the logic is a bit flawed: if there is no DMI match on an ACPI machine, the code reads non-existent properties.
Reading non existent properties is not a problem, notice that the check only checks if one of the properties is present, the others may still be unset, which is fine, then the code reading the properties will leave everything 0 / false.
Re-add previous code that checked for the existence of those properties and a fallback mode to enable jd-mode3 if there is no pdata, device property or quirk. This will help avoid having to add new DMI quirks when only the jd-mode is needed for headset playback.
Note I do think that defaulting to jd_mode = 3 is a good idea, I just believe that the commit message wrongly give the impression that reading non-existent props is a problem.
Cc: Hans de Goede hdegoede@redhat.com Cc: Bard Liao bardliao@realtek.com Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
sound/soc/codecs/rt5645.c | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-)
diff --git a/sound/soc/codecs/rt5645.c b/sound/soc/codecs/rt5645.c index ebc4f5970f24..476020445f65 100644 --- a/sound/soc/codecs/rt5645.c +++ b/sound/soc/codecs/rt5645.c @@ -3687,6 +3687,17 @@ static const struct dmi_system_id dmi_platform_data[] = { { } };
+static bool rt5645_check_dp(struct device *dev) +{
- if (device_property_present(dev, "realtek,in2-differential") ||
device_property_present(dev, "realtek,dmic1-data-pin") ||device_property_present(dev, "realtek,dmic2-data-pin") ||device_property_present(dev, "realtek,jd-mode"))return true;- return false;
+}
- static int rt5645_parse_dt(struct rt5645_priv *rt5645, struct device *dev) { rt5645->pdata.in2_diff = device_property_read_bool(dev,
@@ -3727,8 +3738,10 @@ static int rt5645_i2c_probe(struct i2c_client *i2c,
if (pdata) rt5645->pdata = *pdata;
- else
- else if (rt5645_check_dp(&i2c->dev)) rt5645_parse_dt(rt5645, &i2c->dev);
- else
rt5645->pdata = jd_mode3_platform_data;
How about:
rt5645->pdata.jd_mode = 3;
instead and remove jd_mode3_platform_data and the 2 dmi table entries pointing to it ?
Regards,
Hans
if (quirk != -1) { rt5645->pdata.in2_diff = QUIRK_IN2_DIFF(quirk);
On Wed, Jan 03, 2018 at 11:20:38PM +0100, Hans de Goede wrote:
- else if (rt5645_check_dp(&i2c->dev)) rt5645_parse_dt(rt5645, &i2c->dev);
- else
rt5645->pdata = jd_mode3_platform_data;
How about:
rt5645->pdata.jd_mode = 3;
instead and remove jd_mode3_platform_data and the 2 dmi table entries pointing to it ?
I'm not sure it's best practice to remove the entries that have been explicitly added; on the other hand if the default is jd_mode = 3 then it's not like we'll be adding any more.
On 1/4/18 5:59 AM, Mark Brown wrote:
On Wed, Jan 03, 2018 at 11:20:38PM +0100, Hans de Goede wrote:
- else if (rt5645_check_dp(&i2c->dev)) rt5645_parse_dt(rt5645, &i2c->dev);
- else
rt5645->pdata = jd_mode3_platform_data;How about:
rt5645->pdata.jd_mode = 3;instead and remove jd_mode3_platform_data and the 2 dmi table entries pointing to it ?
I'm not sure it's best practice to remove the entries that have been explicitly added; on the other hand if the default is jd_mode = 3 then it's not like we'll be adding any more.
Yes, I didn't want to remove existing quirks since I can't test for non-regressions on those devices. We will most likely have to add new quirks for the capture part, there is a large variability in how the analog inputs are handled. The jd_mode3 default will help mostly identify if the device is functional out-of-the-box on the playback side (the speaker part relies on a separate amplifier so it's hit or miss).
I will resubmit a patch with a clearer commit message, indeed there is no harm in reading non-existent properties. What I meant is that their absence can be used as a signal to use the default. Thanks for the quick feedback. -Pierre
The patch
ASoC: rt5645: add fallback case for jack detection support
has been applied to the asoc tree at
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git
All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying to this mail.
Thanks, Mark
From 5954c4a1455c3bc42acb2c286744eae1aaa00918 Mon Sep 17 00:00:00 2001
From: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Date: Thu, 11 Jan 2018 14:05:11 -0600 Subject: [PATCH] ASoC: rt5645: add fallback case for jack detection support
Commit 78f5605c0329 ("ASoC: rt5645: cleanup DMI matching code") did a lot of useful cleanups. This patch adds a default case to enable jack detection if there is no pdata, device property or quirk.
The chosen jd-mode3 is the most common and should limit the addition of new DMI-based quirks. Existing DMI quirks which only set this mode are left as is and not removed.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Reviewed-by: Hans de Goede hdegoede@redhat.com Signed-off-by: Mark Brown broonie@kernel.org --- sound/soc/codecs/rt5645.c | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-)
diff --git a/sound/soc/codecs/rt5645.c b/sound/soc/codecs/rt5645.c index fbaf36aeb587..5292fca2f54f 100644 --- a/sound/soc/codecs/rt5645.c +++ b/sound/soc/codecs/rt5645.c @@ -3739,6 +3739,17 @@ static const struct dmi_system_id dmi_platform_data[] = { { } };
+static bool rt5645_check_dp(struct device *dev) +{ + if (device_property_present(dev, "realtek,in2-differential") || + device_property_present(dev, "realtek,dmic1-data-pin") || + device_property_present(dev, "realtek,dmic2-data-pin") || + device_property_present(dev, "realtek,jd-mode")) + return true; + + return false; +} + static int rt5645_parse_dt(struct rt5645_priv *rt5645, struct device *dev) { rt5645->pdata.in2_diff = device_property_read_bool(dev, @@ -3779,8 +3790,10 @@ static int rt5645_i2c_probe(struct i2c_client *i2c,
if (pdata) rt5645->pdata = *pdata; - else + else if (rt5645_check_dp(&i2c->dev)) rt5645_parse_dt(rt5645, &i2c->dev); + else + rt5645->pdata = jd_mode3_platform_data;
if (quirk != -1) { rt5645->pdata.in2_diff = QUIRK_IN2_DIFF(quirk);
participants (3)
-
Hans de Goede -
Mark Brown -
Pierre-Louis Bossart