[PATCH v2 06/32] ASoC: rt5651: Configure jack-detect source through a device-property

[PATCH v2 06/32] ASoC: rt5651: Configure jack-detect source through a device-property

[Hans de Goede]

Configure the jack-detect source through a device-property which can be
set by code outside of the codec driver. Rather then putting platform
specific DMI quirks inside the generic codec driver.

And move the jack-detect-source quirk for the KIANO SlimNote 14.2, which
was present inside the codec driver to the machine driver, where we can
bundle it together with the other quirks already present for this laptop.

Cc: devicetree@vger.kernel.org
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 Documentation/devicetree/bindings/sound/rt5651.txt |  6 ++++
 include/sound/rt5651.h                             |  4 +++
 sound/soc/codecs/rt5651.c                          | 31 ++++----------------
 sound/soc/intel/boards/bytcr_rt5651.c              | 34 ++++++++++++++++++----
 4 files changed, 44 insertions(+), 31 deletions(-)

diff --git a/Documentation/devicetree/bindings/sound/rt5651.txt b/Documentation/devicetree/bindings/sound/rt5651.txt
index 3875233095f5..1632c0cf6123 100644
--- a/Documentation/devicetree/bindings/sound/rt5651.txt
+++ b/Documentation/devicetree/bindings/sound/rt5651.txt
@@ -16,6 +16,12 @@ Optional properties:
 - realtek,dmic-en
   Boolean. true if dmic is used.
 
+- realtek,jack-detect-source
+  u32. Valid values:
+  1: Use JD1_1 pin for jack-dectect
+  2: Use JD1_2 pin for jack-dectect
+  3: Use JD2 pin for jack-dectect
+
 Pins on the device (for linking into audio routes) for RT5651:
 
   * DMIC L1
diff --git a/include/sound/rt5651.h b/include/sound/rt5651.h
index 1612462bf5ad..725b36c329d0 100644
--- a/include/sound/rt5651.h
+++ b/include/sound/rt5651.h
@@ -11,6 +11,10 @@
 #ifndef __LINUX_SND_RT5651_H
 #define __LINUX_SND_RT5651_H
 
+/*
+ * Note these MUST match the values from the DT binding:
+ * Documentation/devicetree/bindings/sound/rt5651.txt
+ */
 enum rt5651_jd_src {
 	RT5651_JD_NULL,
 	RT5651_JD1_1,
diff --git a/sound/soc/codecs/rt5651.c b/sound/soc/codecs/rt5651.c
index 79a839c713ec..7d2bb6fbde4e 100644
--- a/sound/soc/codecs/rt5651.c
+++ b/sound/soc/codecs/rt5651.c
@@ -19,7 +19,6 @@
 #include <linux/platform_device.h>
 #include <linux/spi/spi.h>
 #include <linux/acpi.h>
-#include <linux/dmi.h>
 #include <sound/core.h>
 #include <sound/pcm.h>
 #include <sound/pcm_params.h>
@@ -32,8 +31,6 @@
 #include "rl6231.h"
 #include "rt5651.h"
 
-#define RT5651_JD_MAP(quirk)	((quirk) & GENMASK(7, 0))
-
 #define RT5651_DEVICE_ID_VALUE 0x6281
 
 #define RT5651_PR_RANGE_BASE (0xff + 1)
@@ -41,8 +38,6 @@
 
 #define RT5651_PR_BASE (RT5651_PR_RANGE_BASE + (0 * RT5651_PR_SPACING))
 
-static unsigned long rt5651_quirk;
-
 static const struct regmap_range_cfg rt5651_ranges[] = {
 	{ .name = "PR", .range_min = RT5651_PR_BASE,
 	  .range_max = RT5651_PR_BASE + 0xb4,
@@ -1599,6 +1594,11 @@ static int rt5651_set_jack(struct snd_soc_component *component,
 	struct snd_soc_dapm_context *dapm = snd_soc_component_get_dapm(component);
 	struct rt5651_priv *rt5651 = snd_soc_component_get_drvdata(component);
 	int ret;
+	u32 val;
+
+	if (device_property_read_u32(component->dev,
+				     "realtek,jack-detect-source", &val) == 0)
+		rt5651->jd_src = val;
 
 	if (!rt5651->irq)
 		return -EINVAL;
@@ -1826,24 +1826,6 @@ static const struct i2c_device_id rt5651_i2c_id[] = {
 };
 MODULE_DEVICE_TABLE(i2c, rt5651_i2c_id);
 
-static int rt5651_quirk_cb(const struct dmi_system_id *id)
-{
-	rt5651_quirk = (unsigned long) id->driver_data;
-	return 1;
-}
-
-static const struct dmi_system_id rt5651_quirk_table[] = {
-	{
-		.callback = rt5651_quirk_cb,
-		.matches = {
-			DMI_MATCH(DMI_SYS_VENDOR, "KIANO"),
-			DMI_MATCH(DMI_PRODUCT_NAME, "KIANO SlimNote 14.2"),
-		},
-		.driver_data = (unsigned long *) RT5651_JD1_1,
-	},
-	{}
-};
-
 static int rt5651_jack_detect(struct snd_soc_component *component, int jack_insert)
 {
 	struct snd_soc_dapm_context *dapm = snd_soc_component_get_dapm(component);
@@ -1922,9 +1904,6 @@ static int rt5651_i2c_probe(struct i2c_client *i2c,
 
 	i2c_set_clientdata(i2c, rt5651);
 
-	dmi_check_system(rt5651_quirk_table);
-	rt5651->jd_src = RT5651_JD_MAP(rt5651_quirk);
-
 	rt5651->regmap = devm_regmap_init_i2c(i2c, &rt5651_regmap);
 	if (IS_ERR(rt5651->regmap)) {
 		ret = PTR_ERR(rt5651->regmap);
diff --git a/sound/soc/intel/boards/bytcr_rt5651.c b/sound/soc/intel/boards/bytcr_rt5651.c
index 8008a027cc93..0af855d2df49 100644
--- a/sound/soc/intel/boards/bytcr_rt5651.c
+++ b/sound/soc/intel/boards/bytcr_rt5651.c
@@ -20,6 +20,7 @@
 #include <linux/init.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
+#include <linux/property.h>
 #include <linux/acpi.h>
 #include <linux/clk.h>
 #include <linux/device.h>
@@ -42,10 +43,21 @@ enum {
 	BYT_RT5651_IN3_MAP,
 };
 
-#define BYT_RT5651_MAP(quirk)	((quirk) & GENMASK(7, 0))
-#define BYT_RT5651_DMIC_EN	BIT(16)
-#define BYT_RT5651_MCLK_EN	BIT(17)
-#define BYT_RT5651_MCLK_25MHZ	BIT(18)
+enum {
+	BYT_RT5651_JD_NULL	= (RT5651_JD_NULL << 4),
+	BYT_RT5651_JD1_1	= (RT5651_JD1_1 << 4),
+	BYT_RT5651_JD1_2	= (RT5651_JD1_2 << 4),
+	BYT_RT5651_JD2		= (RT5651_JD2 << 4),
+};
+
+#define BYT_RT5651_MAP(quirk)		((quirk) & GENMASK(3, 0))
+#define BYT_RT5651_JDSRC(quirk)		(((quirk) & GENMASK(7, 4)) >> 4)
+#define BYT_RT5651_DMIC_EN		BIT(16)
+#define BYT_RT5651_MCLK_EN		BIT(17)
+#define BYT_RT5651_MCLK_25MHZ		BIT(18)
+
+/* jack-detect-source + terminating empty entry */
+#define MAX_NO_PROPS 2
 
 struct byt_rt5651_private {
 	struct clk *mclk;
@@ -66,6 +78,9 @@ static void log_quirks(struct device *dev)
 		dev_info(dev, "quirk IN2_MAP enabled");
 	if (BYT_RT5651_MAP(byt_rt5651_quirk) == BYT_RT5651_IN3_MAP)
 		dev_info(dev, "quirk IN3_MAP enabled");
+	if (BYT_RT5651_JDSRC(byt_rt5651_quirk))
+		dev_info(dev, "quirk realtek,jack-detect-source %ld\n",
+			 BYT_RT5651_JDSRC(byt_rt5651_quirk));
 	if (byt_rt5651_quirk & BYT_RT5651_DMIC_EN)
 		dev_info(dev, "quirk DMIC enabled");
 	if (byt_rt5651_quirk & BYT_RT5651_MCLK_EN)
@@ -288,6 +303,7 @@ static const struct dmi_system_id byt_rt5651_quirk_table[] = {
 			DMI_MATCH(DMI_PRODUCT_NAME, "KIANO SlimNote 14.2"),
 		},
 		.driver_data = (void *)(BYT_RT5651_MCLK_EN |
+					BYT_RT5651_JD1_1 |
 					BYT_RT5651_IN1_IN2_MAP),
 	},
 	{}
@@ -298,8 +314,9 @@ static int byt_rt5651_init(struct snd_soc_pcm_runtime *runtime)
 	struct snd_soc_card *card = runtime->card;
 	struct snd_soc_component *codec = runtime->codec_dai->component;
 	struct byt_rt5651_private *priv = snd_soc_card_get_drvdata(card);
+	struct property_entry props[MAX_NO_PROPS] = {};
 	const struct snd_soc_dapm_route *custom_map;
-	int num_routes;
+	int num_routes, cnt = 0;
 	int ret;
 
 	card->dapm.idle_bias_off = true;
@@ -360,6 +377,13 @@ static int byt_rt5651_init(struct snd_soc_pcm_runtime *runtime)
 			dev_err(card->dev, "unable to set MCLK rate\n");
 	}
 
+	props[cnt++] = PROPERTY_ENTRY_U32("realtek,jack-detect-source",
+				BYT_RT5651_JDSRC(byt_rt5651_quirk));
+
+	ret = device_add_properties(codec->dev, props);
+	if (ret)
+		return ret;
+
 	ret = snd_soc_card_jack_new(runtime->card, "Headset",
 				    SND_JACK_HEADSET, &priv->jack,
 				    bytcr_jack_pins, ARRAY_SIZE(bytcr_jack_pins));
-- 
2.14.3

Re: [PATCH v2 06/32] ASoC: rt5651: Configure jack-detect source through a device-property

[Mark Brown]

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

On Sun, Feb 25, 2018 at 11:46:47AM +0100, Hans de Goede wrote:
> Configure the jack-detect source through a device-property which can be
> set by code outside of the codec driver. Rather then putting platform
> specific DMI quirks inside the generic codec driver.

> And move the jack-detect-source quirk for the KIANO SlimNote 14.2, which
> was present inside the codec driver to the machine driver, where we can
> bundle it together with the other quirks already present for this laptop.

Multiple things in a patch again :/  The property itself is fine but...

> @@ -360,6 +377,13 @@ static int byt_rt5651_init(struct snd_soc_pcm_runtime *runtime)
>  			dev_err(card->dev, "unable to set MCLK rate\n");
>  	}
>  
> +	props[cnt++] = PROPERTY_ENTRY_U32("realtek,jack-detect-source",
> +				BYT_RT5651_JDSRC(byt_rt5651_quirk));
> +
> +	ret = device_add_properties(codec->dev, props);
> +	if (ret)
> +		return ret;
> +
>  	ret = snd_soc_card_jack_new(runtime->card, "Headset",

I'm having a hard time geting comfortable with this; it's all a bit
fragile feeling to me - someone deciding to centralise the property
parsing (eg, if they are using a platform where platform data makes
sense or want to cross-validate with some other property on probe) could
easily break things and there's not even a comment in the CODEC driver.
It'll work but it doesn't seem to match expectations for how firmware
data is provided which is what's bugging me here.  It at least needs a
"here be dragons" style comment in the code.

Ideally we'd have a general way to massage the ACPI data at init time
and could have board files that are instantiated from DMI to do that
(like are needed even more for SFI) but that's a long way off.

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

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

Re: [PATCH v2 06/32] ASoC: rt5651: Configure jack-detect source through a device-property

[Hans de Goede]

Hi,

On 01-03-18 20:30, Mark Brown wrote:
> On Sun, Feb 25, 2018 at 11:46:47AM +0100, Hans de Goede wrote:
>> Configure the jack-detect source through a device-property which can be
>> set by code outside of the codec driver. Rather then putting platform
>> specific DMI quirks inside the generic codec driver.
> 
>> And move the jack-detect-source quirk for the KIANO SlimNote 14.2, which
>> was present inside the codec driver to the machine driver, where we can
>> bundle it together with the other quirks already present for this laptop.
> 
> Multiple things in a patch again :/

Yes because the property replaces the quirk, I can split the changes between
the codec and machine driver into 2 patches but then the intermediate state
will be that the jack-detection no longer works.

> The property itself is fine but...
> 
>> @@ -360,6 +377,13 @@ static int byt_rt5651_init(struct snd_soc_pcm_runtime *runtime)
>>   			dev_err(card->dev, "unable to set MCLK rate\n");
>>   	}
>>   
>> +	props[cnt++] = PROPERTY_ENTRY_U32("realtek,jack-detect-source",
>> +				BYT_RT5651_JDSRC(byt_rt5651_quirk));
>> +
>> +	ret = device_add_properties(codec->dev, props);
>> +	if (ret)
>> +		return ret;
>> +
>>   	ret = snd_soc_card_jack_new(runtime->card, "Headset",
> 
> I'm having a hard time geting comfortable with this; it's all a bit
> fragile feeling to me - someone deciding to centralise the property
> parsing (eg, if they are using a platform where platform data makes
> sense or want to cross-validate with some other property on probe) could
> easily break things and there's not even a comment in the CODEC driver.

It is not _that_ fragile, but I agree that it would be good to add
a comment about not moving the property-parsing to the codec driver.

I will rebase and submit a new version with a comment added.
Do you want me to split this patch into separate codec and machine
driver patches while at it?

> It'll work but it doesn't seem to match expectations for how firmware
> data is provided which is what's bugging me here.

The problem here is that the data is not coming from the firmware,
which is a reality we have to deal with leading to some sort of
compromise.

> It at least needs a
> "here be dragons" style comment in the code.

Ack.

> Ideally we'd have a general way to massage the ACPI data at init time
> and could have board files that are instantiated from DMI to do that
> (like are needed even more for SFI) but that's a long way off.

Something like DT overlays for ACPI? With the overlays being triggered
by a DMI match? Interesting concept, but given how much discussion there
still is surrounding DT overlays I don't see this happening anytime soon.

Regards,

Hans

Re: [PATCH v2 06/32] ASoC: rt5651: Configure jack-detect source through a device-property

[Hans de Goede]

Hi,

On 01-03-18 23:35, Hans de Goede wrote:
> Hi,
> 
> On 01-03-18 20:30, Mark Brown wrote:
>> On Sun, Feb 25, 2018 at 11:46:47AM +0100, Hans de Goede wrote:
>>> Configure the jack-detect source through a device-property which can be
>>> set by code outside of the codec driver. Rather then putting platform
>>> specific DMI quirks inside the generic codec driver.
>>
>>> And move the jack-detect-source quirk for the KIANO SlimNote 14.2, which
>>> was present inside the codec driver to the machine driver, where we can
>>> bundle it together with the other quirks already present for this laptop.
>>
>> Multiple things in a patch again :/
> 
> Yes because the property replaces the quirk, I can split the changes between
> the codec and machine driver into 2 patches but then the intermediate state
> will be that the jack-detection no longer works.
> 
>> The property itself is fine but...
>>
>>> @@ -360,6 +377,13 @@ static int byt_rt5651_init(struct snd_soc_pcm_runtime *runtime)
>>>               dev_err(card->dev, "unable to set MCLK rate\n");
>>>       }
>>> +    props[cnt++] = PROPERTY_ENTRY_U32("realtek,jack-detect-source",
>>> +                BYT_RT5651_JDSRC(byt_rt5651_quirk));
>>> +
>>> +    ret = device_add_properties(codec->dev, props);
>>> +    if (ret)
>>> +        return ret;
>>> +
>>>       ret = snd_soc_card_jack_new(runtime->card, "Headset",
>>
>> I'm having a hard time geting comfortable with this; it's all a bit
>> fragile feeling to me - someone deciding to centralise the property
>> parsing (eg, if they are using a platform where platform data makes
>> sense or want to cross-validate with some other property on probe) could
>> easily break things and there's not even a comment in the CODEC driver.
> 
> It is not _that_ fragile, but I agree that it would be good to add
> a comment about not moving the property-parsing to the codec driver.

So one other solution which comes to mind here is to move the
snd_soc_card_jack_new() call into the codec driver's
rt5651_apply_properties() function (conditional on a jack src being
set in the properties).

This would make the machine driver code look like this:

	props[cnt++] = PROPERTY_ENTRY_...
	props[cnt++] = PROPERTY_ENTRY_...
	props[cnt++] = PROPERTY_ENTRY_...
	props[cnt++] = PROPERTY_ENTRY_...

	ret = device_add_properties(codec->dev, props);
	if (ret)
		return ret;

	ret = rt5651_apply_properties(codec);
	if (ret)
		return ret;

Which makes the ordering really clear without needing any
comments.

This will also make the jack-detect device-properties work with
devicetree platforms without any changes to their platform code,
which currently is not the case since the non Intel platform-code
does not call snd_soc_component_set_jack().

Thinking more about this I believe that doing the
snd_soc_card_jack_new() call inside the codec driver based on
device-properties is a better solution then doing it in the
machine driver.

Please let me know if you agree then I will rework the remaining
patches accordingly.

Regards,

Hans

Re: [PATCH v2 06/32] ASoC: rt5651: Configure jack-detect source through a device-property

[Mark Brown]

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

On Fri, Mar 02, 2018 at 10:32:25AM +0100, Hans de Goede wrote:
> On 01-03-18 23:35, Hans de Goede wrote:

> > It is not _that_ fragile, but I agree that it would be good to add
> > a comment about not moving the property-parsing to the codec driver.

> So one other solution which comes to mind here is to move the
> snd_soc_card_jack_new() call into the codec driver's
> rt5651_apply_properties() function (conditional on a jack src being
> set in the properties).

> This would make the machine driver code look like this:
> 
> 	props[cnt++] = PROPERTY_ENTRY_...
> 	props[cnt++] = PROPERTY_ENTRY_...
> 	props[cnt++] = PROPERTY_ENTRY_...
> 	props[cnt++] = PROPERTY_ENTRY_...

> 	ret = device_add_properties(codec->dev, props);
> 	if (ret)
> 		return ret;

> 	ret = rt5651_apply_properties(codec);
> 	if (ret)
> 		return ret;

> Which makes the ordering really clear without needing any
> comments.

It's still unusual to have something outside the driver trigger property
parsing, though the EXPORT_SYMBOL_GPL() would make that apparent.

> This will also make the jack-detect device-properties work with
> devicetree platforms without any changes to their platform code,
> which currently is not the case since the non Intel platform-code
> does not call snd_soc_component_set_jack().

What makes you claim that?  Any board with jack detection wired up will
call that function.  Not all boards have that configured of course but
there's absolutely nothing Intel specific about that interface.

> Thinking more about this I believe that doing the
> snd_soc_card_jack_new() call inside the codec driver based on
> device-properties is a better solution then doing it in the
> machine driver.

No, that's really not a good idea.  Any jacks in the system are part of
the system specific wiring, they're not something that's intrinsic to
the CODEC.  Even with fairly basic setups you've got options like having
a headset jack or separate microphone and headphone jacks.

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

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

Re: [PATCH v2 06/32] ASoC: rt5651: Configure jack-detect source through a device-property

[Hans de Goede]

Hi,

On 02-03-18 13:18, Mark Brown wrote:
> On Fri, Mar 02, 2018 at 10:32:25AM +0100, Hans de Goede wrote:
>> On 01-03-18 23:35, Hans de Goede wrote:
> 
>>> It is not _that_ fragile, but I agree that it would be good to add
>>> a comment about not moving the property-parsing to the codec driver.
> 
>> So one other solution which comes to mind here is to move the
>> snd_soc_card_jack_new() call into the codec driver's
>> rt5651_apply_properties() function (conditional on a jack src being
>> set in the properties).
> 
>> This would make the machine driver code look like this:
>>
>> 	props[cnt++] = PROPERTY_ENTRY_...
>> 	props[cnt++] = PROPERTY_ENTRY_...
>> 	props[cnt++] = PROPERTY_ENTRY_...
>> 	props[cnt++] = PROPERTY_ENTRY_...
> 
>> 	ret = device_add_properties(codec->dev, props);
>> 	if (ret)
>> 		return ret;
> 
>> 	ret = rt5651_apply_properties(codec);
>> 	if (ret)
>> 		return ret;
> 
>> Which makes the ordering really clear without needing any
>> comments.
> 
> It's still unusual to have something outside the driver trigger property
> parsing, though the EXPORT_SYMBOL_GPL() would make that apparent.
> 
>> This will also make the jack-detect device-properties work with
>> devicetree platforms without any changes to their platform code,
>> which currently is not the case since the non Intel platform-code
>> does not call snd_soc_component_set_jack().
> 
> What makes you claim that?  Any board with jack detection wired up will
> call that function.  Not all boards have that configured of course but
> there's absolutely nothing Intel specific about that interface.

Right I'm not claiming the interface is Intel specific, what I'm trying
to say is that the need for some code outside of the codec driver
to create the jack means that adding jack-support to DT platforms
cannot be done through just adding DT properties (once this patch
is merged), it will still require platform code changes.

That is fine, I was just thinking it would be convenient if DT
platforms could lift on the jack-detect work being done for
the rt5651 driver now with just some DT changes and no other
changes required.

>> Thinking more about this I believe that doing the
>> snd_soc_card_jack_new() call inside the codec driver based on
>> device-properties is a better solution then doing it in the
>> machine driver.
> 
> No, that's really not a good idea.  Any jacks in the system are part of
> the system specific wiring, they're not something that's intrinsic to
> the CODEC.  Even with fairly basic setups you've got options like having
> a headset jack or separate microphone and headphone jacks.

OK, so if doing the jack creation in the machine-driver / platform
code is by design and you want to keep things that way, then I
assume that what needs changing for v3 of this patch is:

1) Split the patch in separate codec + machine-drv patches
2) Add comments to make the ordering requirements clear to
both the codec- and machine-driver.

Correct?

Regards,

Hans

Re: [PATCH v2 06/32] ASoC: rt5651: Configure jack-detect source through a device-property

[Mark Brown]

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

On Fri, Mar 02, 2018 at 01:58:53PM +0100, Hans de Goede wrote:
> On 02-03-18 13:18, Mark Brown wrote:

> > What makes you claim that?  Any board with jack detection wired up will
> > call that function.  Not all boards have that configured of course but
> > there's absolutely nothing Intel specific about that interface.

> Right I'm not claiming the interface is Intel specific, what I'm trying
> to say is that the need for some code outside of the codec driver
> to create the jack means that adding jack-support to DT platforms
> cannot be done through just adding DT properties (once this patch
> is merged), it will still require platform code changes.

On DT the situation with generic drivers is much better than it is on
ACPI so we're actually able to create jacks purely from DT with both the
simple and graph cards.  This was what drove the creation of the generic
jack operation rather than device specific function calls, Bard realized
that the device specific calls were all very similar and adding the call
allowed the generic cards to work with jacks.

> > No, that's really not a good idea.  Any jacks in the system are part of
> > the system specific wiring, they're not something that's intrinsic to
> > the CODEC.  Even with fairly basic setups you've got options like having
> > a headset jack or separate microphone and headphone jacks.

> OK, so if doing the jack creation in the machine-driver / platform
> code is by design and you want to keep things that way, then I
> assume that what needs changing for v3 of this patch is:

> 1) Split the patch in separate codec + machine-drv patches
> 2) Add comments to make the ordering requirements clear to
> both the codec- and machine-driver.

> Correct?

Yes, I think so.

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

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

Re: [PATCH v2 06/32] ASoC: rt5651: Configure jack-detect source through a device-property

[Hans de Goede]

Hi,

On 02-03-18 18:41, Mark Brown wrote:
> On Fri, Mar 02, 2018 at 01:58:53PM +0100, Hans de Goede wrote:
>> On 02-03-18 13:18, Mark Brown wrote:
> 
>>> What makes you claim that?  Any board with jack detection wired up will
>>> call that function.  Not all boards have that configured of course but
>>> there's absolutely nothing Intel specific about that interface.
> 
>> Right I'm not claiming the interface is Intel specific, what I'm trying
>> to say is that the need for some code outside of the codec driver
>> to create the jack means that adding jack-support to DT platforms
>> cannot be done through just adding DT properties (once this patch
>> is merged), it will still require platform code changes.
> 
> On DT the situation with generic drivers is much better than it is on
> ACPI so we're actually able to create jacks purely from DT with both the
> simple and graph cards.  This was what drove the creation of the generic
> jack operation rather than device specific function calls, Bard realized
> that the device specific calls were all very similar and adding the call
> allowed the generic cards to work with jacks.
> 
>>> No, that's really not a good idea.  Any jacks in the system are part of
>>> the system specific wiring, they're not something that's intrinsic to
>>> the CODEC.  Even with fairly basic setups you've got options like having
>>> a headset jack or separate microphone and headphone jacks.
> 
>> OK, so if doing the jack creation in the machine-driver / platform
>> code is by design and you want to keep things that way, then I
>> assume that what needs changing for v3 of this patch is:
> 
>> 1) Split the patch in separate codec + machine-drv patches
>> 2) Add comments to make the ordering requirements clear to
>> both the codec- and machine-driver.
> 
>> Correct?
> 
> Yes, I think so.

Actually I've come up with a nicer way to deal with the ordering issues
around setting the device-properties from the machine-driver.

If we attach the properties in the machine-driver *before* calling
snd_soc_register_card() and check them from the codec/component driver's
probe function (rather then from the i2c_driver.probe function), then
there is no need for a codec private apply_properties() function to
get the ordering right, since the codec/component driver's probe
function is called from snd_soc_register_card().

This still needs some comments in both the codec and machine driver
to make sure that future changes don't cause issues, but it gets rid
of the ugly apply_properties() function call from the machine-driver.

I'm currently preparing a v3 with this change + other requested changes.
I need to re-run a bunch of tests before posting, so I hope to post the
v3 series tomorrow.

Regards,

Hans

Re: [PATCH v2 06/32] ASoC: rt5651: Configure jack-detect source through a device-property

[Rob Herring]

On Fri, Mar 02, 2018 at 01:58:53PM +0100, Hans de Goede wrote:
> Hi,
> 
> On 02-03-18 13:18, Mark Brown wrote:
> > On Fri, Mar 02, 2018 at 10:32:25AM +0100, Hans de Goede wrote:
> > > On 01-03-18 23:35, Hans de Goede wrote:
> > 
> > > > It is not _that_ fragile, but I agree that it would be good to add
> > > > a comment about not moving the property-parsing to the codec driver.
> > 
> > > So one other solution which comes to mind here is to move the
> > > snd_soc_card_jack_new() call into the codec driver's
> > > rt5651_apply_properties() function (conditional on a jack src being
> > > set in the properties).
> > 
> > > This would make the machine driver code look like this:
> > > 
> > > 	props[cnt++] = PROPERTY_ENTRY_...
> > > 	props[cnt++] = PROPERTY_ENTRY_...
> > > 	props[cnt++] = PROPERTY_ENTRY_...
> > > 	props[cnt++] = PROPERTY_ENTRY_...
> > 
> > > 	ret = device_add_properties(codec->dev, props);
> > > 	if (ret)
> > > 		return ret;
> > 
> > > 	ret = rt5651_apply_properties(codec);
> > > 	if (ret)
> > > 		return ret;
> > 
> > > Which makes the ordering really clear without needing any
> > > comments.
> > 
> > It's still unusual to have something outside the driver trigger property
> > parsing, though the EXPORT_SYMBOL_GPL() would make that apparent.
> > 
> > > This will also make the jack-detect device-properties work with
> > > devicetree platforms without any changes to their platform code,
> > > which currently is not the case since the non Intel platform-code
> > > does not call snd_soc_component_set_jack().
> > 
> > What makes you claim that?  Any board with jack detection wired up will
> > call that function.  Not all boards have that configured of course but
> > there's absolutely nothing Intel specific about that interface.
> 
> Right I'm not claiming the interface is Intel specific, what I'm trying
> to say is that the need for some code outside of the codec driver
> to create the jack means that adding jack-support to DT platforms
> cannot be done through just adding DT properties (once this patch
> is merged), it will still require platform code changes.
> 
> That is fine, I was just thinking it would be convenient if DT
> platforms could lift on the jack-detect work being done for
> the rt5651 driver now with just some DT changes and no other
> changes required.
> 
> > > Thinking more about this I believe that doing the
> > > snd_soc_card_jack_new() call inside the codec driver based on
> > > device-properties is a better solution then doing it in the
> > > machine driver.
> > 
> > No, that's really not a good idea.  Any jacks in the system are part of
> > the system specific wiring, they're not something that's intrinsic to
> > the CODEC.  Even with fairly basic setups you've got options like having
> > a headset jack or separate microphone and headphone jacks.
> 
> OK, so if doing the jack creation in the machine-driver / platform
> code is by design and you want to keep things that way, then I
> assume that what needs changing for v3 of this patch is:
> 
> 1) Split the patch in separate codec + machine-drv patches
> 2) Add comments to make the ordering requirements clear to
> both the codec- and machine-driver.
> 
> Correct?

And split bindings to separate patch please.

Rob

[PATCH v2 17/32] ASoC: rt5651: Allow specifying over-current thresholds through device-properties

[Hans de Goede]

OVer-Current-Detection (OVCD) for the micbias current is used to detect if
an inserted jack is a headset or headphones (mic shorted to ground).

Some boards may need different values for the OVCD threshold because of a
resistor on the board in serial with or parallel to the jack mic contact.

This commit adds support for the sofar unset OVCD scale-factor register
values and adds support for specifying non-default values for both the
current threshold and the scale-factor through device-properties.

Cc: devicetree@vger.kernel.org
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 Documentation/devicetree/bindings/sound/rt5651.txt | 11 ++++++
 include/sound/rt5651.h                             | 11 ++++++
 sound/soc/codecs/rt5651.c                          | 42 +++++++++++++++++++++-
 sound/soc/codecs/rt5651.h                          | 10 ++++++
 4 files changed, 73 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/sound/rt5651.txt b/Documentation/devicetree/bindings/sound/rt5651.txt
index 1632c0cf6123..8cb7ee8e79cf 100644
--- a/Documentation/devicetree/bindings/sound/rt5651.txt
+++ b/Documentation/devicetree/bindings/sound/rt5651.txt
@@ -22,6 +22,17 @@ Optional properties:
   2: Use JD1_2 pin for jack-dectect
   3: Use JD2 pin for jack-dectect
 
+- realtek,over-current-threshold
+  u32, micbias over-current detection threshold in µA, valid values are
+  600, 1500 and 2000µA.
+
+- realtek,over-current-scale-factor
+  u32, micbias over-current detection scale-factor, valid values are:
+  0: Scale current by 0.5
+  1: Scale current by 0.75
+  2: Scale current by 1.0
+  3: Scale current by 1.5
+
 Pins on the device (for linking into audio routes) for RT5651:
 
   * DMIC L1
diff --git a/include/sound/rt5651.h b/include/sound/rt5651.h
index 725b36c329d0..6403b862fb9a 100644
--- a/include/sound/rt5651.h
+++ b/include/sound/rt5651.h
@@ -22,4 +22,15 @@ enum rt5651_jd_src {
 	RT5651_JD2,
 };
 
+/*
+ * Note these MUST match the values from the DT binding:
+ * Documentation/devicetree/bindings/sound/rt5651.txt
+ */
+enum rt5651_ovcd_sf {
+	RT5651_OVCD_SF_0P5,
+	RT5651_OVCD_SF_0P75,
+	RT5651_OVCD_SF_1P0,
+	RT5651_OVCD_SF_1P5,
+};
+
 #endif
diff --git a/sound/soc/codecs/rt5651.c b/sound/soc/codecs/rt5651.c
index dfdc8244d308..38d2e12fe701 100644
--- a/sound/soc/codecs/rt5651.c
+++ b/sound/soc/codecs/rt5651.c
@@ -1592,6 +1592,13 @@ static int rt5651_set_jack(struct snd_soc_component *component,
 			   struct snd_soc_jack *hp_jack, void *data)
 {
 	struct rt5651_priv *rt5651 = snd_soc_component_get_drvdata(component);
+	/*
+	 * Testing on various boards has shown that good defaults for the OVCD
+	 * threshold and scale-factor are 2000µA and 0.75. For an effective
+	 * limit of 1500µA, this seems to be more reliable then 1500µA and 1.0.
+	 */
+	unsigned int ovcd_th = RT5651_MIC1_OVTH_2000UA;
+	unsigned int ovcd_sf = RT5651_MIC_OVCD_SF_0P75;
 	int ret;
 	u32 val;
 
@@ -1599,6 +1606,35 @@ static int rt5651_set_jack(struct snd_soc_component *component,
 				     "realtek,jack-detect-source", &val) == 0)
 		rt5651->jd_src = val;
 
+	if (device_property_read_u32(component->dev,
+			"realtek,over-current-threshold", &val) == 0) {
+		switch (val) {
+		case 600:
+			ovcd_th = RT5651_MIC1_OVTH_600UA;
+			break;
+		case 1500:
+			ovcd_th = RT5651_MIC1_OVTH_1500UA;
+			break;
+		case 2000:
+			ovcd_th = RT5651_MIC1_OVTH_2000UA;
+			break;
+		default:
+			dev_err(component->dev, "Invalid over-current-threshold value: %d\n",
+				val);
+			return -EINVAL;
+		}
+	}
+
+	if (device_property_read_u32(component->dev,
+			"realtek,over-current-scale-factor", &val) == 0) {
+		if (val > RT5651_OVCD_SF_1P5) {
+			dev_err(component->dev, "Invalid over-current-scale-factor value: %d\n",
+				val);
+			return -EINVAL;
+		}
+		ovcd_sf = val << RT5651_MIC_OVCD_SF_SFT;
+	}
+
 	if (!rt5651->irq)
 		return -EINVAL;
 
@@ -1637,13 +1673,17 @@ static int rt5651_set_jack(struct snd_soc_component *component,
 	snd_soc_component_update_bits(component, RT5651_PWR_ANLG2,
 		RT5651_PWR_JD_M, RT5651_PWR_JD_M);
 
+	/* Set OVCD threshold current and scale-factor */
+	snd_soc_component_write(component, RT5651_PR_BASE + RT5651_BIAS_CUR4,
+				0xa800 | ovcd_sf);
+
 	snd_soc_component_update_bits(component, RT5651_MICBIAS,
 				      RT5651_MIC1_OVCD_MASK |
 				      RT5651_MIC1_OVTH_MASK |
 				      RT5651_PWR_CLK12M_MASK |
 				      RT5651_PWR_MB_MASK,
 				      RT5651_MIC1_OVCD_EN |
-				      RT5651_MIC1_OVTH_600UA |
+				      ovcd_th |
 				      RT5651_PWR_MB_PU |
 				      RT5651_PWR_CLK12M_PU);
 
diff --git a/sound/soc/codecs/rt5651.h b/sound/soc/codecs/rt5651.h
index 7d9d5fa09d6f..5fb25f7e2a8c 100644
--- a/sound/soc/codecs/rt5651.h
+++ b/sound/soc/codecs/rt5651.h
@@ -138,6 +138,7 @@
 /* Index of Codec Private Register definition */
 #define RT5651_BIAS_CUR1			0x12
 #define RT5651_BIAS_CUR3			0x14
+#define RT5651_BIAS_CUR4			0x15
 #define RT5651_CLSD_INT_REG1			0x1c
 #define RT5651_CHPUMP_INT_REG1			0x24
 #define RT5651_MAMP_INT_REG2			0x37
@@ -1966,6 +1967,15 @@
 #define RT5651_D_GATE_EN_SFT			0
 
 /* Codec Private Register definition */
+
+/* MIC Over current threshold scale factor (0x15) */
+#define RT5651_MIC_OVCD_SF_MASK			(0x3 << 8)
+#define RT5651_MIC_OVCD_SF_SFT			8
+#define RT5651_MIC_OVCD_SF_0P5			(0x0 << 8)
+#define RT5651_MIC_OVCD_SF_0P75			(0x1 << 8)
+#define RT5651_MIC_OVCD_SF_1P0			(0x2 << 8)
+#define RT5651_MIC_OVCD_SF_1P5			(0x3 << 8)
+
 /* 3D Speaker Control (0x63) */
 #define RT5651_3D_SPK_MASK			(0x1 << 15)
 #define RT5651_3D_SPK_SFT			15
-- 
2.14.3

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH v2 17/32] ASoC: rt5651: Allow specifying over-current thresholds through device-properties

[Rob Herring]

On Sun, Feb 25, 2018 at 11:46:58AM +0100, Hans de Goede wrote:
> OVer-Current-Detection (OVCD) for the micbias current is used to detect if
> an inserted jack is a headset or headphones (mic shorted to ground).
> 
> Some boards may need different values for the OVCD threshold because of a
> resistor on the board in serial with or parallel to the jack mic contact.
> 
> This commit adds support for the sofar unset OVCD scale-factor register
> values and adds support for specifying non-default values for both the
> current threshold and the scale-factor through device-properties.
> 
> Cc: devicetree@vger.kernel.org
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  Documentation/devicetree/bindings/sound/rt5651.txt | 11 ++++++

Please split to a separate patch.

>  include/sound/rt5651.h                             | 11 ++++++
>  sound/soc/codecs/rt5651.c                          | 42 +++++++++++++++++++++-
>  sound/soc/codecs/rt5651.h                          | 10 ++++++
>  4 files changed, 73 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/sound/rt5651.txt b/Documentation/devicetree/bindings/sound/rt5651.txt
> index 1632c0cf6123..8cb7ee8e79cf 100644
> --- a/Documentation/devicetree/bindings/sound/rt5651.txt
> +++ b/Documentation/devicetree/bindings/sound/rt5651.txt
> @@ -22,6 +22,17 @@ Optional properties:
>    2: Use JD1_2 pin for jack-dectect
>    3: Use JD2 pin for jack-dectect
>  
> +- realtek,over-current-threshold
> +  u32, micbias over-current detection threshold in µA, valid values are
> +  600, 1500 and 2000µA.

Needs a unit suffix as defined in property-units.txt

> +
> +- realtek,over-current-scale-factor
> +  u32, micbias over-current detection scale-factor, valid values are:
> +  0: Scale current by 0.5
> +  1: Scale current by 0.75
> +  2: Scale current by 1.0
> +  3: Scale current by 1.5
> +
>  Pins on the device (for linking into audio routes) for RT5651:
>  
>    * DMIC L1