[PATCH 1/4] ASoC: wm9713: add binding for WM9713 codec

[PATCH 1/4] ASoC: wm9713: add binding for WM9713 codec

[Robert Jarzmik]

This adds a binding for the Wolfson WM9713 audio codec.

Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
---
 Documentation/devicetree/bindings/sound/wm9713.txt | 14 ++++++++++++++
 1 file changed, 14 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/sound/wm9713.txt

diff --git a/Documentation/devicetree/bindings/sound/wm9713.txt b/Documentation/devicetree/bindings/sound/wm9713.txt
new file mode 100644
index 000000000000..761001bc3201
--- /dev/null
+++ b/Documentation/devicetree/bindings/sound/wm9713.txt
@@ -0,0 +1,14 @@
+WM9713 audio CODEC
+
+This devices supports I2C.
+
+Required properties:
+  - compatible :
+	"wlf,wm9713"
+
+Example:
+	wm9713: wm9713@0 {
+		compatible = "wlf,wm9713";
+		#sound-dai-cells = <1>;
+		status = "okay";
+	};
-- 
2.1.4

[PATCH 2/4] ASoC: wm9713: add device tree support

[Robert Jarzmik]

Add the code to be able to use this codec in a devicetree platform.
This is tested with the zylonite pxa310 board.

Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
---
 sound/soc/codecs/wm9713.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/sound/soc/codecs/wm9713.c b/sound/soc/codecs/wm9713.c
index 79e143625ac3..4e522302bb8a 100644
--- a/sound/soc/codecs/wm9713.c
+++ b/sound/soc/codecs/wm9713.c
@@ -1272,9 +1272,19 @@ static int wm9713_remove(struct platform_device *pdev)
 	return 0;
 }
 
+#ifdef CONFIG_OF
+static const struct of_device_id wm9713_dt_ids[] = {
+	{ .compatible = "wlf,wm9713", },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, wm9713_dt_ids);
+
+#endif
+
 static struct platform_driver wm9713_codec_driver = {
 	.driver = {
 			.name = "wm9713-codec",
+			.of_match_table = of_match_ptr(wm9713_dt_ids),
 	},
 
 	.probe = wm9713_probe,
-- 
2.1.4

[PATCH 3/4] ASoC: pxa: add binding for pxa2xx-ac97 audio complex

[Robert Jarzmik]

This adds a binding for the Marvell PXA audio complex, available in
pxa2xx and pxa3xx variants.

Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
---
 .../bindings/sound/marvell,pxa2xx-ac97.txt         | 25 ++++++++++++++++++++++
 1 file changed, 25 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/sound/marvell,pxa2xx-ac97.txt

diff --git a/Documentation/devicetree/bindings/sound/marvell,pxa2xx-ac97.txt b/Documentation/devicetree/bindings/sound/marvell,pxa2xx-ac97.txt
new file mode 100644
index 000000000000..b3f2882d9c7d
--- /dev/null
+++ b/Documentation/devicetree/bindings/sound/marvell,pxa2xx-ac97.txt
@@ -0,0 +1,25 @@
+Marvell PXA2xx audio complex
+
+This descriptions matches the AC97 controller found in pxa2xx and pxa3xx series.
+
+Required properties:
+  - compatible: "marvell,pxa2xx-ac97"
+  - reg: device MMIO address space
+  - interrupts: single interrupt generated by AC97 IP
+  - clocks: input clock of the AC97 IP, refer to clock-bindings.txt
+
+Optional properties:
+  - pinctrl-names, pinctrl-0: refer to pinctrl-bindings.txt
+  - reset-gpio: gpio used for AC97 reset, refer to gpio.txt
+
+Example:
+	ac97: sound@40500000 {
+	      	compatible = "marvell,pxa2xx-ac97";
+		reg = < 0x40500000 0x1000 >;
+		interrupts = <14>;
+		reset-gpio = <&gpio 113 GPIO_ACTIVE_HIGH>;
+		#sound-dai-cells = <1>;
+		pinctrl-names = "default";
+		pinctrl-0 = < &pmux_ac97_default >;
+		status = "okay";
+	};
-- 
2.1.4

[PATCH 4/4] ASoC: pxa: add pxa2xx-ac97 devicetree support

[Robert Jarzmik]

Add the devicetree support, so that the driver can be used in a
devicetree platform.

Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
---
 sound/arm/pxa2xx-ac97-lib.c | 11 +++++++++++
 sound/soc/pxa/pxa2xx-ac97.c | 10 ++++++++++
 2 files changed, 21 insertions(+)

diff --git a/sound/arm/pxa2xx-ac97-lib.c b/sound/arm/pxa2xx-ac97-lib.c
index 39c3969ac1c7..37fe7e0cbcf1 100644
--- a/sound/arm/pxa2xx-ac97-lib.c
+++ b/sound/arm/pxa2xx-ac97-lib.c
@@ -19,6 +19,7 @@
 #include <linux/module.h>
 #include <linux/io.h>
 #include <linux/gpio.h>
+#include <linux/of_gpio.h>
 
 #include <sound/ac97_codec.h>
 #include <sound/pxa2xx-lib.h>
@@ -332,6 +333,16 @@ int pxa2xx_ac97_hw_probe(struct platform_device *dev)
 			dev_err(&dev->dev, "Invalid reset GPIO %d\n",
 				pdata->reset_gpio);
 		}
+	} else if (!pdata && dev->dev.of_node) {
+		pdata = devm_kzalloc(&dev->dev, sizeof(*pdata), GFP_KERNEL);
+		if (!pdata)
+			return -ENOMEM;
+		pdata->reset_gpio = of_get_named_gpio(dev->dev.of_node,
+						      "reset-gpio", 0);
+		if (pdata->reset_gpio == -ENOENT)
+			pdata->reset_gpio = -1;
+		else if (pdata->reset_gpio < 0)
+			return pdata->reset_gpio;
 	} else {
 		if (cpu_is_pxa27x())
 			reset_gpio = 113;
diff --git a/sound/soc/pxa/pxa2xx-ac97.c b/sound/soc/pxa/pxa2xx-ac97.c
index f3de615aacd7..13e3ab9224b9 100644
--- a/sound/soc/pxa/pxa2xx-ac97.c
+++ b/sound/soc/pxa/pxa2xx-ac97.c
@@ -221,6 +221,15 @@ static const struct snd_soc_component_driver pxa_ac97_component = {
 	.name		= "pxa-ac97",
 };
 
+#ifdef CONFIG_OF
+static const struct of_device_id pxa2xx_ac97_dt_ids[] = {
+	{ .compatible = "marvell,pxa2xx-ac97", },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, pxa2xx_ac97_dt_ids);
+
+#endif
+
 static int pxa2xx_ac97_dev_probe(struct platform_device *pdev)
 {
 	int ret;
@@ -279,6 +288,7 @@ static struct platform_driver pxa2xx_ac97_driver = {
 #ifdef CONFIG_PM_SLEEP
 		.pm	= &pxa2xx_ac97_pm_ops,
 #endif
+		.of_match_table = of_match_ptr(pxa2xx_ac97_dt_ids),
 	},
 };
 
-- 
2.1.4

Re: [PATCH 1/4] ASoC: wm9713: add binding for WM9713 codec

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 381 bytes --]

On Sat, Feb 20, 2016 at 03:37:56PM +0100, Robert Jarzmik wrote:

> +WM9713 audio CODEC
> +
> +This devices supports I2C.

No, it clearly doesn't...  The problem with doing this is that since
AC'97 is an enumerable bus we really shouldn't need to list AC'97 CODECs
in the device tree.  Instead we should be probing at runtime (as the
non-ASoC AC'97 code does) or something similar.

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

Re: [PATCH 1/4] ASoC: wm9713: add binding for WM9713 codec

[Robert Jarzmik]

Mark Brown <broonie@kernel.org> writes:

> On Sat, Feb 20, 2016 at 03:37:56PM +0100, Robert Jarzmik wrote:
>
>> +WM9713 audio CODEC
>> +
>> +This devices supports I2C.
>
> No, it clearly doesn't...
Right, it supports AC97.

> The problem with doing this is that since AC'97 is an enumerable bus we really
> shouldn't need to list AC'97 CODECs in the device tree.
Ok, I understand that.

> Instead we should be probing at runtime (as the non-ASoC AC'97 code does) or
> something similar.
When you say "non-ASoC AC'97 code", which file are you referring to ? Is it
sound/pci/ac97/ac97_codec.c ?

If so is there already a table of tuples (AC97_VENDOR_ID1, AC97_VENDOR_ID2) ->
(platform device, platform device data) and a matching mechanism already
available to the ASoC drivers ?

Cheers.

-- 
Robert

Re: [PATCH 1/4] ASoC: wm9713: add binding for WM9713 codec

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 876 bytes --]

On Sat, Feb 20, 2016 at 07:22:04PM +0100, Robert Jarzmik wrote:
> Mark Brown <broonie@kernel.org> writes:

> > Instead we should be probing at runtime (as the non-ASoC AC'97 code does) or
> > something similar.

> When you say "non-ASoC AC'97 code", which file are you referring to ? Is it
> sound/pci/ac97/ac97_codec.c ?

Yes.

> If so is there already a table of tuples (AC97_VENDOR_ID1, AC97_VENDOR_ID2) ->
> (platform device, platform device data) and a matching mechanism already
> available to the ASoC drivers ?

ASoC doesn't really support the enumeration very well, you can use
ac97.c as the CODEC but that's about it.  There is a generic AC'97 PXA
driver in sound/arm, if your system can use that that'd be a better
route to DT integration for it I think.  Did you try that, if there are
problems with that perhaps we can improve that driver, it should  be
simpler.

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

Re: [PATCH 1/4] ASoC: wm9713: add binding for WM9713 codec

[Robert Jarzmik]

Mark Brown <broonie@kernel.org> writes:

> On Sat, Feb 20, 2016 at 07:22:04PM +0100, Robert Jarzmik wrote:
>> Mark Brown <broonie@kernel.org> writes:
Removed DT people from this conversation.

>> > Instead we should be probing at runtime (as the non-ASoC AC'97 code does) or
>> > something similar.
>
>> When you say "non-ASoC AC'97 code", which file are you referring to ? Is it
>> sound/pci/ac97/ac97_codec.c ?
>
> Yes.
>
>> If so is there already a table of tuples (AC97_VENDOR_ID1, AC97_VENDOR_ID2) ->
>> (platform device, platform device data) and a matching mechanism already
>> available to the ASoC drivers ?
>
> ASoC doesn't really support the enumeration very well, you can use
> ac97.c as the CODEC but that's about it.

> There is a generic AC'97 PXA driver in sound/arm, if your system can use that
> that'd be a better route to DT integration for it I think.
I'm open on the topic.

Historically, I use sound/soc/pxa/pxa2xx-ac97.c since 2008. I know it works, but
if you think I should examine sound/arm/pxa2xx-ac97.c, let's do that.

> Did you try that, if there are problems with that perhaps we can improve that
> driver, it should be simpler.
I will. By now I fail to see how this will help in the wm9713 probing and
detection ...

Until I make the try, here is what I have as a device-tree extract in [1], which
is my candidate for sound/soc/pxa/zylonite.c replacement.. If we conclude that
wm9713 shouldn't be in device-tree, then I'm curious how the DAI bindings
(simple-audio-card,dai-link*) should be handled.

Cheers.

-- 
Robert

[1] Zylonite DT extract
	ssp3: ssp@41900000 {
		compatible = "mrvl,pxa3xx-ssp";
		reg = <0x41900000 0x40>;
		interrupts = <0>;
		clocks = < &clks CLK_SSP3 >;
		dmas = <&pdma 66 3
			&pdma 67 3>;
		dma-names = "rx", "tx";
		pinctrl-names = "default";
		pinctrl-0 = < &pmux_ssp3_low_default
			      &pmux_ssp3_float_default >;
		status = "okay";
	};

	ssp_dai0: ssp_dai@0 {
		compatible = "mrvl,pxa-ssp-dai";
		port = <&ssp3>;
		#sound-dai-cells = <0>;
	};
	ac97: sound@40500000 {
	      	compatible = "marvell,pxa2xx-ac97";
		reg = < 0x40500000 0x1000 >;
		interrupts = <14>;
		reset-gpio = <&gpio 113 GPIO_ACTIVE_HIGH>;
		#sound-dai-cells = <1>;
		pinctrl-names = "default";
		pinctrl-0 = < &pmux_ac97_default >;
		status = "okay";
	};

	pxa_pcm_audio: snd_soc_pxa_audio {
		compatible = "mrvl,pxa-pcm-audio";
		#sound-dai-cells = <1>;
	};
	pxa_ssp_dai: snd_soc_pxa_audio {
		compatible = "mrvl,pxa-pcm-audio";
		#sound-dai-cells = <1>;
	};

	wm9713: wm9713@0 {
		compatible = "wlf,wm9713";
		#sound-dai-cells = <1>;
		pinctrl-names = "default";
		pinctrl-0 = < &pmux_wm9713_default >;
		status = "okay";
	};

	sound {
		compatible = "simple-audio-card";
		simple-audio-card,name = "Zylonite-Sound-Card";
		simple-audio-card,format = "ac97";
		simple-audio-card,widgets =
			"Headphone", "Headphone",
			"Microphone", "Headset Microphone",
			"Microphone", "Handset Microphone",
			"Speaker", "Multiactor",
			"Speaker", "Headset Earpiece";
		simple-audio-card,routing =
			"Headphone", "HPL",		/* Headphone output */
			"Headphone", "HPR",		/* connected to HPL/HPR */
			"Headset Earpiece", "OUT3",	/* On-board earpiece */
			"MIC2A", "Mic Bias",			/* Headphone mic */
			"Mic Bias", "Headset Microphone",	/* Headphone mic */
			"MIC1", "Mic Bias",			/* On-board mic */
			"Mic Bias", "Handset Microphone",	/* On-board mic */
			"Multiactor", "SPKL",		/* Multiactor differentially */
			"Multiactor", "SPKR";		/* connected over SPKL/SPKR */
	
		simple-audio-card,dai-link@0 {		/* AC97 */
			format = "ac97";
			cpu {
				sound-dai = <&ac97 0>;
			};
			codec {
				sound-dai = <&wm9713 0>;
			};
			plat {
				sound-dai = <&pxa_pcm_audio 0>;
			};
		};
		simple-audio-card,dai-link@1 {		/* AC97 Aux */
			format = "ac97";
			cpu {
				sound-dai = <&ac97 1>;
			};
			codec {
				sound-dai = <&wm9713 1>;
			};
			plat {
				sound-dai = <&pxa_pcm_audio 0>;
			};
		};
		simple-audio-card,dai-link@2 {		/* AC97 Voice */
			format = "i2s";
			cpu {
				sound-dai = <&ssp_dai0>;
			};
			codec {
				sound-dai = <&wm9713 2>;
			};
			plat {
				sound-dai = <&pxa_pcm_audio 0>;
			};
		};
	};

Re: [PATCH 1/4] ASoC: wm9713: add binding for WM9713 codec

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1460 bytes --]

On Sat, Feb 20, 2016 at 09:32:58PM +0100, Robert Jarzmik wrote:
> Mark Brown <broonie@kernel.org> writes:
> > On Sat, Feb 20, 2016 at 07:22:04PM +0100, Robert Jarzmik wrote:

Please fix your mail client to word wrap within paragraphs at something
substantially less than 80 columns.  Doing this makes your messages much
easier to read and reply to.

> > There is a generic AC'97 PXA driver in sound/arm, if your system can use that
> > that'd be a better route to DT integration for it I think.
> I'm open on the topic.

> Historically, I use sound/soc/pxa/pxa2xx-ac97.c since 2008. I know it works, but
> if you think I should examine sound/arm/pxa2xx-ac97.c, let's do that.
> 
> > Did you try that, if there are problems with that perhaps we can improve that
> > driver, it should be simpler.

> I will. By now I fail to see how this will help in the wm9713 probing and
> detection ...

It will eumerate the AC'97 bus by itself and does not need the CODEC to
be described.

> Until I make the try, here is what I have as a device-tree extract in [1], which
> is my candidate for sound/soc/pxa/zylonite.c replacement.. If we conclude that
> wm9713 shouldn't be in device-tree, then I'm curious how the DAI bindings
> (simple-audio-card,dai-link*) should be handled.

They should be created as a function of enumerating the CODEC.  If you
use the genric AC'97 stuff it doesn't use ASoC at all and this happens
as a side effect.

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

Re: [PATCH 1/4] ASoC: wm9713: add binding for WM9713 codec

[Robert Jarzmik]

Mark Brown <broonie@kernel.org> writes:

> On Sat, Feb 20, 2016 at 09:32:58PM +0100, Robert Jarzmik wrote:
>> Mark Brown <broonie@kernel.org> writes:
>> > On Sat, Feb 20, 2016 at 07:22:04PM +0100, Robert Jarzmik wrote:
>> I will. By now I fail to see how this will help in the wm9713 probing and
>> detection ...
>
> It will eumerate the AC'97 bus by itself and does not need the CODEC to
> be described.
I think I still don't get it.

So let's rephrase it another way : how will the function wm9713_probe() be
called, ie. what is the possible function backtrace leading to that call ?

>> Until I make the try, here is what I have as a device-tree extract in [1],
>> which is my candidate for sound/soc/pxa/zylonite.c replacement.. If we
>> conclude that wm9713 shouldn't be in device-tree, then I'm curious how the
>> DAI bindings (simple-audio-card,dai-link*) should be handled.
>
> They should be created as a function of enumerating the CODEC.  If you
> use the genric AC'97 stuff it doesn't use ASoC at all and this happens
> as a side effect.
I don't get that either. For me sound/soc/pci/ac97/ac97_codec.c is PCI specific,
not generic, so what is "generic AC'97 stuff" ? I will never be able to use it
as on my platforms CONFIG_PCI=n.

Do you have a devicetree example somewhere, with (ac97 host, audio codec) pair I
can have a look at to understand ?

Cheers.

-- 
Robert

Re: [PATCH 1/4] ASoC: wm9713: add binding for WM9713 codec

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1328 bytes --]

On Sat, Feb 20, 2016 at 11:24:02PM +0100, Robert Jarzmik wrote:

Please fix your mail client to word wrap within paragraphs at something
substantially less than 80 columns.  Doing this makes your messages much
easier to read and reply to.

> Mark Brown <broonie@kernel.org> writes:
> > On Sat, Feb 20, 2016 at 09:32:58PM +0100, Robert Jarzmik wrote:

> > It will eumerate the AC'97 bus by itself and does not need the CODEC to
> > be described.

> I think I still don't get it.

> So let's rephrase it another way : how will the function wm9713_probe() be
> called, ie. what is the possible function backtrace leading to that call ?

It will not be called, the generic AC'97 code will be used.

> > They should be created as a function of enumerating the CODEC.  If you
> > use the genric AC'97 stuff it doesn't use ASoC at all and this happens
> > as a side effect.

> I don't get that either. For me sound/soc/pci/ac97/ac97_codec.c is PCI specific,
> not generic, so what is "generic AC'97 stuff" ? I will never be able to use it
> as on my platforms CONFIG_PCI=n.

That is the generic code, there is no PCI dependency.

> Do you have a devicetree example somewhere, with (ac97 host, audio codec) pair I
> can have a look at to understand ?

Some Atmel boards do this IIRC, as does the AACI driver (via AMBA but
same effect).

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

Re: [PATCH 3/4] ASoC: pxa: add binding for pxa2xx-ac97 audio complex

[Rob Herring]

On Sat, Feb 20, 2016 at 03:37:58PM +0100, Robert Jarzmik wrote:
> This adds a binding for the Marvell PXA audio complex, available in
> pxa2xx and pxa3xx variants.
> 
> Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
> ---
>  .../bindings/sound/marvell,pxa2xx-ac97.txt         | 25 ++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/sound/marvell,pxa2xx-ac97.txt
> 
> diff --git a/Documentation/devicetree/bindings/sound/marvell,pxa2xx-ac97.txt b/Documentation/devicetree/bindings/sound/marvell,pxa2xx-ac97.txt
> new file mode 100644
> index 000000000000..b3f2882d9c7d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/sound/marvell,pxa2xx-ac97.txt
> @@ -0,0 +1,25 @@
> +Marvell PXA2xx audio complex
> +
> +This descriptions matches the AC97 controller found in pxa2xx and pxa3xx series.
> +
> +Required properties:
> +  - compatible: "marvell,pxa2xx-ac97"
> +  - reg: device MMIO address space
> +  - interrupts: single interrupt generated by AC97 IP
> +  - clocks: input clock of the AC97 IP, refer to clock-bindings.txt
> +
> +Optional properties:
> +  - pinctrl-names, pinctrl-0: refer to pinctrl-bindings.txt
> +  - reset-gpio: gpio used for AC97 reset, refer to gpio.txt

reset-gpios

> +
> +Example:
> +	ac97: sound@40500000 {
> +	      	compatible = "marvell,pxa2xx-ac97";
> +		reg = < 0x40500000 0x1000 >;
> +		interrupts = <14>;
> +		reset-gpio = <&gpio 113 GPIO_ACTIVE_HIGH>;
> +		#sound-dai-cells = <1>;
> +		pinctrl-names = "default";
> +		pinctrl-0 = < &pmux_ac97_default >;
> +		status = "okay";

No clocks prop, yet required.

> +	};
> -- 
> 2.1.4
> 

Re: [PATCH 1/4] ASoC: wm9713: add binding for WM9713 codec

[Robert Jarzmik]

Mark Brown <broonie@kernel.org> writes:

>> > It will eumerate the AC'97 bus by itself and does not need the CODEC to
>> > be described.
>
>> I think I still don't get it.
>
>> So let's rephrase it another way : how will the function wm9713_probe() be
>> called, ie. what is the possible function backtrace leading to that call ?
>
> It will not be called, the generic AC'97 code will be used.

Ok, if it's not called no code in sound/soc/codecs/wm9713.c will be used, right
?
In that case wm9713_set_dai_clkdiv() will never be used, nor will the
wm9713_audio_map or wm9713_dapm_widgets be created, which will break all
userspace programs relying on these mixers and DAPM routes.

Or am I missing something ?

>> Do you have a devicetree example somewhere, with (ac97 host, audio codec) pair I
>> can have a look at to understand ?
>
> Some Atmel boards do this IIRC, as does the AACI driver (via AMBA but
> same effect).
I suppose you mean sound/arm/aaci.c, which is more a platform_data like driver
(if I understood the integrator code correctly).

I suppose we can achieve comparable result with sound/arm/pxa2xx-ac97.c, but as
to know if the functionality will be comparable to sound/soc/pxa/pxa2xx-ac97.c,
it's hard to say. If I count the DMA requestors, I see 5 in the sound/soc
version, and 2 in sound/arm.

That makes me believe the sound/arm version is inferior.

Cheers.

-- 
Robert

Re: [PATCH 1/4] ASoC: wm9713: add binding for WM9713 codec

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1237 bytes --]

On Fri, Feb 26, 2016 at 02:33:49AM +0100, Robert Jarzmik wrote:
> Mark Brown <broonie@kernel.org> writes:

> > It will not be called, the generic AC'97 code will be used.

> Ok, if it's not called no code in sound/soc/codecs/wm9713.c will be used, right
> ?
> In that case wm9713_set_dai_clkdiv() will never be used, nor will the
> wm9713_audio_map or wm9713_dapm_widgets be created, which will break all
> userspace programs relying on these mixers and DAPM routes.

> Or am I missing something ?

No, you're not missing anything - that'll be what happens.  If you need
to preserve the userspace ABI on your board you'd need a much bigger
(but very welcome) refactoring of the AC'97 code to be less hacky and
use the device model more directly, or at least define a generic AC'97
binding somehow.  All the AC'97 support has never really been properly
moved over to the device model and unfortunately nobody's yet cared
about it for device tree except in the simple cases supported by the
generic AC'97 code.  I appreciate that this isn't very helpful, it's
an unfortunate consequence of DT as an ABI.

We probably want to end up with something like what the Intel folks have
been doing recently for HDA to get that working within ASoC.

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

Re: [PATCH 1/4] ASoC: wm9713: add binding for WM9713 codec

[Robert Jarzmik]

Mark Brown <broonie@kernel.org> writes:

> On Fri, Feb 26, 2016 at 02:33:49AM +0100, Robert Jarzmik wrote:
>> Mark Brown <broonie@kernel.org> writes:
>
>> > It will not be called, the generic AC'97 code will be used.
>
>> Ok, if it's not called no code in sound/soc/codecs/wm9713.c will be used, right
>> ?
>> In that case wm9713_set_dai_clkdiv() will never be used, nor will the
>> wm9713_audio_map or wm9713_dapm_widgets be created, which will break all
>> userspace programs relying on these mixers and DAPM routes.
>
>> Or am I missing something ?
>
> No, you're not missing anything - that'll be what happens.  If you need
> to preserve the userspace ABI on your board you'd need a much bigger
> (but very welcome) refactoring of the AC'97 code to be less hacky and
> use the device model more directly, or at least define a generic AC'97
> binding somehow.  All the AC'97 support has never really been properly
> moved over to the device model and unfortunately nobody's yet cared
> about it for device tree except in the simple cases supported by the
> generic AC'97 code.  I appreciate that this isn't very helpful, it's
> an unfortunate consequence of DT as an ABI.
>
> We probably want to end up with something like what the Intel folks have
> been doing recently for HDA to get that working within ASoC.

Ok, let me think about it and propose something, an approach.

I must admit I like the structure I saw in drivers/amba/bus.c, ie. to have
something like :
 - a bus driver core (sound/ac97/bus.c ?)
   Split between this and sound/pci/ac97_codec.c I don't know yet.
   => an instance of this bus will be instanciated from each snd_ac97_bus() call
      just as now
   => bus_register(&ac97_bustype)
   => ac97_bus_probe/remove(), match/uevent/dev_attrs
   => ac97_driver_register(struct ac97_driver *drv)

 - a ac97 driver structure (struct ac97_driver) with :
   => u32 vendor_id (vendor_id = lambda(vendor_id1, vendor_id2))
   => u32 vendor_id_mask (mask of bits to match)
   ...

 - each ac97 controller will call snd_ac97_bus().
   In my case, that's sound/arm/pxa2xx-ac97.c or sound/soc/pxa2xx-ac97.c,
   whatever.

 - each ac97 codec will subscribe to the bus
   ac97_driver_register(struct ac97_driver *drv, u32 vendor_id, u32 vendor_mask)
   For example wm9713.c will call :
     ac97_driver_register(drv, AC97_VENDOR(0x...., 0x....), 0xffffffff);

Well, if I'm totally mistaken, tell me. If not it will take me a bit of time to
write is down properly in a Documentation/ file.

Cheers.

-- 
Robert

Re: [PATCH 3/4] ASoC: pxa: add binding for pxa2xx-ac97 audio complex

[Robert Jarzmik]

Rob Herring <robh@kernel.org> writes:

>> +Example:
>> +	ac97: sound@40500000 {
>> +	      	compatible = "marvell,pxa2xx-ac97";
>> +		reg = < 0x40500000 0x1000 >;
>> +		interrupts = <14>;
>> +		reset-gpio = <&gpio 113 GPIO_ACTIVE_HIGH>;
>> +		#sound-dai-cells = <1>;
>> +		pinctrl-names = "default";
>> +		pinctrl-0 = < &pmux_ac97_default >;
>> +		status = "okay";
>
> No clocks prop, yet required.
Ah yeah, that's a good catch, thanks.

I'll resubmit once my discussion with Mark on ac97 will be finished, which might
takes monthes given all I have to do lately.

Cheers.

-- 
Robert

Re: [PATCH 1/4] ASoC: wm9713: add binding for WM9713 codec

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 773 bytes --]

On Fri, Feb 26, 2016 at 10:04:15PM +0100, Robert Jarzmik wrote:

> Ok, let me think about it and propose something, an approach.

> I must admit I like the structure I saw in drivers/amba/bus.c, ie. to have
> something like :

...

> Well, if I'm totally mistaken, tell me. If not it will take me a bit of time to
> write is down properly in a Documentation/ file.

That seems pretty reasonable at first pass I think, the main issue is
just managing the transition safely for all the users.  There's a lot of
fragility in the AC'97 hardware and software so people have been very
reluctant to touch it.  Equally I think a lot of the more problematic
users probably aren't in use any more so you can probably get away with
more now than would have been the case in the past.

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