[PATCH] ASoC: rt5645: add fallback case for jack detection support

[PATCH] ASoC: rt5645: add fallback case for jack detection support

[Pierre-Louis Bossart]

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);
-- 
2.14.1

Re: [PATCH] ASoC: rt5645: add fallback case for jack detection support

[Hans de Goede]

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);
> 

Re: [PATCH] ASoC: rt5645: add fallback case for jack detection support

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 528 bytes --]

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.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] ASoC: rt5645: add fallback case for jack detection support

[Pierre-Louis Bossart]

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

Applied "ASoC: rt5645: add fallback case for jack detection support" to the asoc tree

[Mark Brown]

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);
-- 
2.15.1