Device tree support for Atmel AC97 controller on AT91SAM9263

---
 sound/atmel/ac97c.c | 15 ++-------------
 1 file changed, 2 insertions(+), 13 deletions(-)

sound/atmel/ac97c.c +2 ~~-13~~

index c5f0ddd..9a13875 100644
@@ -1202,6 +1202,7 @@ static int atmel_ac97c_remove(struct platform_device *pdev)
 }
 
 static struct platform_driver atmel_ac97c_driver = {
+	.probe		= atmel_ac97c_probe,
 	.remove		= atmel_ac97c_remove,
 	.driver		= {
 		.name	= "atmel_ac97c",

@@ -1209,19 +1210,7 @@ static struct platform_driver atmel_ac97c_driver = {
 		.pm	= ATMEL_AC97C_PM_OPS,
 	},
 };
-
-static int __init atmel_ac97c_init(void)
-{
-	return platform_driver_probe(&atmel_ac97c_driver,
-			atmel_ac97c_probe);
-}
-module_init(atmel_ac97c_init);
-
-static void __exit atmel_ac97c_exit(void)
-{
-	platform_driver_unregister(&atmel_ac97c_driver);
-}
-module_exit(atmel_ac97c_exit);
+module_platform_driver(atmel_ac97c_driver);
 
 MODULE_LICENSE("GPL");
 MODULE_DESCRIPTION("Driver for Atmel AC97 controller");

Signature · 1 line

1.9.2

Alexandre Belloni

2014-04-14 22:48 UTC #2 lore raw

PATCH 1/4 ALSA: sound/atmel/ac97c.c: Convert to module_platform_driver

Quoted 1 line · Alexander Stein

THis reduces some boilerplate code.

^
small typo

Quoted 2 lines

Signed-off-by Alexander Stein <[email protected]>

else

Quoted 45 lines

---
 sound/atmel/ac97c.c | 15 ++-------------
 1 file changed, 2 insertions(+), 13 deletions(-)

sound/atmel/ac97c.c +2 ~~-13~~

index c5f0ddd..9a13875 100644
@@ -1202,6 +1202,7 @@ static int atmel_ac97c_remove(struct platform_device *pdev)
 }
 
 static struct platform_driver atmel_ac97c_driver = {
+	.probe		= atmel_ac97c_probe,
 	.remove		= atmel_ac97c_remove,
 	.driver		= {
 		.name	= "atmel_ac97c",

@@ -1209,19 +1210,7 @@ static struct platform_driver atmel_ac97c_driver = {
 		.pm	= ATMEL_AC97C_PM_OPS,
 	},
 };
-
-static int __init atmel_ac97c_init(void)
-{
-	return platform_driver_probe(&atmel_ac97c_driver,
-			atmel_ac97c_probe);
-}
-module_init(atmel_ac97c_init);
-
-static void __exit atmel_ac97c_exit(void)
-{
-	platform_driver_unregister(&atmel_ac97c_driver);
-}
-module_exit(atmel_ac97c_exit);
+module_platform_driver(atmel_ac97c_driver);
 
 MODULE_LICENSE("GPL");
 MODULE_DESCRIPTION("Driver for Atmel AC97 controller");

Signature · 7 lines

1.9.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Signature · 3 lines

Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Alexander Stein

author

2014-04-12 09:08 UTC #3 lore raw

PATCH 2/4 ALSA: sound/atmel-ac97c.h: Remove unused flags from platform data

This platform data member is unsed, so remove it.

---
 include/sound/atmel-ac97c.h | 2 --
 1 file changed, 2 deletions(-)

include/sound/atmel-ac97c.h +0 -2

index e6aabdb..00e6c289 100644
@@ -23,7 +23,6 @@
  * @reset_pin: GPIO pin wired to the reset input on the external AC97 codec,
  *             optional to use, set to -ENODEV if not in use. AC97 layer will
  *             try to do a software reset of the external codec anyway.
- * @flags: Flags for which directions should be enabled.
  *
  * If the user do not want to use a DMA channel for playback or capture, i.e.
  * only one feature is required on the board. The slave for playback or capture

@@ -33,7 +32,6 @@
 struct ac97c_platform_data {
 	struct dw_dma_slave	rx_dws;
 	struct dw_dma_slave	tx_dws;
-	unsigned int 		flags;
 	int			reset_pin;
 };

Signature · 1 line

1.9.2

Alexandre Belloni

2014-04-14 22:51 UTC #4 lore raw

PATCH 2/4 ALSA: sound/atmel-ac97c.h: Remove unused flags from platform data

Quoted 1 line · Alexander Stein

This platform data member is unsed, so remove it.

^
another typo :)

Quoted 2 lines

Signed-off-by Alexander Stein <[email protected]>

I would say that it doesn't matter anymore but no code is better tahn dead code.

Quoted 32 lines

---
 include/sound/atmel-ac97c.h | 2 --
 1 file changed, 2 deletions(-)

include/sound/atmel-ac97c.h +0 -2

index e6aabdb..00e6c289 100644
@@ -23,7 +23,6 @@
  * @reset_pin: GPIO pin wired to the reset input on the external AC97 codec,
  *             optional to use, set to -ENODEV if not in use. AC97 layer will
  *             try to do a software reset of the external codec anyway.
- * @flags: Flags for which directions should be enabled.
  *
  * If the user do not want to use a DMA channel for playback or capture, i.e.
  * only one feature is required on the board. The slave for playback or capture

@@ -33,7 +32,6 @@
 struct ac97c_platform_data {
 	struct dw_dma_slave	rx_dws;
 	struct dw_dma_slave	tx_dws;
-	unsigned int 		flags;
 	int			reset_pin;
 };

Signature · 7 lines

1.9.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Signature · 3 lines

Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Alexander Stein

author

2014-04-12 09:08 UTC #5 lore raw

PATCH 3/4 ALSA: sound/atmel/ac97c.c: Add device tree support

---
 .../devicetree/bindings/sound/atmel_ac97c.txt      | 20 +++++++++
 sound/atmel/ac97c.c                                | 52 ++++++++++++++++++++--
 2 files changed, 69 insertions(+), 3 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/sound/atmel_ac97c.txt

Documentation/devicetree/bindings/sound/atmel_ac97c.txt new file +20 -0

new file mode 100644
index 0000000..9839403
@@ -0,0 +1,20 @@
+* Atmel AC97 controller
+
+Required properties:
+  - compatible: "atmel,atmel_ac97c"
+  - reg: Address and length of the register set for the device
+  - interrupts: Should contain AC97 interrupt
+  - atmel,reset-pin: GPIO for resetting the codec
+Optional properties:
+  - pinctrl-names, pinctrl-0: Please refer to pinctrl-bindings.txt
+
+Example:
+sound at fffa0000 {
+	compatible = "atmel,atmel_ac97c";
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_ac97>;
+	reg = <0xfffa0000 0x4000>;
+	interrupts = <18 IRQ_TYPE_LEVEL_HIGH 5>;
+
+	atmel,reset-pin = <&pioC 29 GPIO_ACTIVE_LOW>;
+};

sound/atmel/ac97c.c +49 -3

77 lines not on this page · raw message

Signature · 1 line

1.9.2

Alexander Shiyan

2014-04-12 10:31 UTC #6 lore raw

Re: [PATCH 3/4] ALSA: sound/atmel/ac97c.c: Add device tree support

Sat, 12 Apr 2014 11:08:26 +0200 ?? Alexander Stein <[email protected]>:

Quoted 13 lines

---
 .../devicetree/bindings/sound/atmel_ac97c.txt      | 20 +++++++++
 sound/atmel/ac97c.c                                | 52 ++++++++++++++++++++--
 2 files changed, 69 insertions(+), 3 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/sound/atmel_ac97c.txt

Documentation/devicetree/bindings/sound/atmel_ac97c.txt new file +0 -0

new file mode 100644
index 0000000..9839403

Documentation/devicetree/bindings/sound/atmel_ac97c.txt line 7

Quoted 7 lines

Documentation/devicetree/bindings/sound/atmel_ac97c.txt new file +7 -0

@@ -0,0 +1,20 @@
+* Atmel AC97 controller
+
+Required properties:
+  - compatible: "atmel,atmel_ac97c"
+  - reg: Address and length of the register set for the device
+  - interrupts: Should contain AC97 interrupt
+  - atmel,reset-pin: GPIO for resetting the codec

Why standard "gpios" property cannot be used here?

---

Alexander Stein

author

2014-04-12 10:42 UTC #7 lore raw

PATCH 3/4 ALSA: sound/atmel/ac97c.c: Add device tree support

On Saturday 12 April 2014, 14:31:28 wrote Alexander Shiyan:

Quoted 8 lines

Sat, 12 Apr 2014 11:08:26 +0200 ?? Alexander Stein <[email protected]>:
Quoted 7 lines
Signed-off-by Alexander Stein <[email protected]>
---
 .../devicetree/bindings/sound/atmel_ac97c.txt      | 20 +++++++++
 sound/atmel/ac97c.c                                | 52 ++++++++++++++++++++--
 2 files changed, 69 insertions(+), 3 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/sound/atmel_ac97c.txt

Quoted 15 lines

Documentation/devicetree/bindings/sound/atmel_ac97c.txt line 7
Quoted 13 lines
Documentation/devicetree/bindings/sound/atmel_ac97c.txt new file +7 -0
new file mode 100644
index 0000000..9839403
@@ -0,0 +1,20 @@
+* Atmel AC97 controller
+
+Required properties:
+  - compatible: "atmel,atmel_ac97c"
+  - reg: Address and length of the register set for the device
+  - interrupts: Should contain AC97 interrupt
+  - atmel,reset-pin: GPIO for resetting the codec
Why standard "gpios" property cannot be used here?

I guess they can. I have no experience defining a devicetree binding, so I just stuck to the platformdata member name. If this is the recommended way, I'll gladly change that.

Alexander

Alexander Shiyan

2014-04-12 10:48 UTC #8 lore raw

Re: [PATCH 3/4] ALSA: sound/atmel/ac97c.c: Add device tree support

Sat, 12 Apr 2014 12:42:18 +0200 ?? Alexander Stein <[email protected]>:

Quoted 25 lines

On Saturday 12 April 2014, 14:31:28 wrote Alexander Shiyan:
Quoted 8 lines
Sat, 12 Apr 2014 11:08:26 +0200 ?? Alexander Stein <[email protected]>:
Quoted 7 lines
Signed-off-by Alexander Stein <[email protected]>
---
 .../devicetree/bindings/sound/atmel_ac97c.txt      | 20 +++++++++
 sound/atmel/ac97c.c                                | 52 ++++++++++++++++++++--
 2 files changed, 69 insertions(+), 3 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/sound/atmel_ac97c.txt
Quoted 15 lines
Documentation/devicetree/bindings/sound/atmel_ac97c.txt line 7
Quoted 13 lines
Documentation/devicetree/bindings/sound/atmel_ac97c.txt new file +7 -0
new file mode 100644
index 0000000..9839403
@@ -0,0 +1,20 @@
+* Atmel AC97 controller
+
+Required properties:
+  - compatible: "atmel,atmel_ac97c"
+  - reg: Address and length of the register set for the device
+  - interrupts: Should contain AC97 interrupt
+  - atmel,reset-pin: GPIO for resetting the codec
Why standard "gpios" property cannot be used here?

Quoted 2 lines

I guess they can. I have no experience defining a devicetree binding, so I just stuck to the platformdata member name. If this is the recommended way, I'll gladly change that.

Another question is whether this property should refer to the controller, as it is clear that it refers to the codec ....

---

Alexander Stein

author

2014-04-13 08:32 UTC #9 lore raw

PATCH 3/4 ALSA: sound/atmel/ac97c.c: Add device tree support

On Saturday 12 April 2014, 14:48:17 wrote Alexander Shiyan:

Quoted 26 lines

Sat, 12 Apr 2014 12:42:18 +0200 ?? Alexander Stein <[email protected]>:
Quoted 25 lines
On Saturday 12 April 2014, 14:31:28 wrote Alexander Shiyan:
Quoted 23 lines
Sat, 12 Apr 2014 11:08:26 +0200 ?? Alexander Stein <[email protected]>:
Quoted 13 lines
Signed-off-by Alexander Stein <[email protected]>
---
 .../devicetree/bindings/sound/atmel_ac97c.txt      | 20 +++++++++
 sound/atmel/ac97c.c                                | 52 ++++++++++++++++++++--
 2 files changed, 69 insertions(+), 3 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/sound/atmel_ac97c.txt
Documentation/devicetree/bindings/sound/atmel_ac97c.txt new file +0 -0
new file mode 100644
index 0000000..9839403
Documentation/devicetree/bindings/sound/atmel_ac97c.txt line 7
Quoted 7 lines
Documentation/devicetree/bindings/sound/atmel_ac97c.txt new file +7 -0
@@ -0,0 +1,20 @@
+* Atmel AC97 controller
+
+Required properties:
+  - compatible: "atmel,atmel_ac97c"
+  - reg: Address and length of the register set for the device
+  - interrupts: Should contain AC97 interrupt
+  - atmel,reset-pin: GPIO for resetting the codec
Why standard "gpios" property cannot be used here?

Quoted 5 lines

Quoted 2 lines
I guess they can. I have no experience defining a devicetree binding, so I just stuck to the platformdata member name. If this is the recommended way, I'll gladly change that.
Another question is whether this property should refer to the controller, as it is clear that it refers to the codec ....

AFAICT the codec itself has no representation in the devicetree. It gets detected by the controller which uses the reset GPIO.

Alexander

Alexandre Belloni

2014-04-14 23:21 UTC #10 lore raw

PATCH 3/4 ALSA: sound/atmel/ac97c.c: Add device tree support

Quoted 27 lines · Alexander Stein

On Saturday 12 April 2014, 14:48:17 wrote Alexander Shiyan:
Quoted 26 lines
Sat, 12 Apr 2014 12:42:18 +0200 ?? Alexander Stein <[email protected]>:
Quoted 25 lines
On Saturday 12 April 2014, 14:31:28 wrote Alexander Shiyan:
Quoted 23 lines
Sat, 12 Apr 2014 11:08:26 +0200 ?? Alexander Stein <[email protected]>:
Quoted 13 lines
Signed-off-by Alexander Stein <[email protected]>
---
 .../devicetree/bindings/sound/atmel_ac97c.txt      | 20 +++++++++
 sound/atmel/ac97c.c                                | 52 ++++++++++++++++++++--
 2 files changed, 69 insertions(+), 3 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/sound/atmel_ac97c.txt
Documentation/devicetree/bindings/sound/atmel_ac97c.txt new file +0 -0
new file mode 100644
index 0000000..9839403
Documentation/devicetree/bindings/sound/atmel_ac97c.txt line 7
Quoted 7 lines
Documentation/devicetree/bindings/sound/atmel_ac97c.txt new file +7 -0
@@ -0,0 +1,20 @@
+* Atmel AC97 controller
+
+Required properties:
+  - compatible: "atmel,atmel_ac97c"
+  - reg: Address and length of the register set for the device
+  - interrupts: Should contain AC97 interrupt
+  - atmel,reset-pin: GPIO for resetting the codec
Why standard "gpios" property cannot be used here?

Quoted 8 lines

Quoted 5 lines
Quoted 2 lines
I guess they can. I have no experience defining a devicetree binding, so I just stuck to the platformdata member name. If this is the recommended way, I'll gladly change that.
Another question is whether this property should refer to the controller, as it is clear that it refers to the codec ....
AFAICT the codec itself has no representation in the devicetree. It gets detected by the controller which uses the reset GPIO.

Yeah, maybe we can already go for ac97-gpios, there. It actually takes a list of gpios: ac97-sync, ac97-sdata and ac97-reset. You may want to have a look at Documentation/devicetree/bindings/sound/soc-ac97link.txt and snd_soc_set_ac97_ops_of_reset(), snd_soc_ac97_parse_pinctl() in sound/soc/core.c

The goal would be to be able to keep the same bindings when switching over to ASoC.

Signature · 3 lines

Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Alexander Stein

author

2014-04-15 19:17 UTC #11 lore raw

PATCH 3/4 ALSA: sound/atmel/ac97c.c: Add device tree support

On Tuesday 15 April 2014, 01:21:34 wrote Alexandre Belloni:

Quoted 37 lines · Alexander Stein

Quoted 27 lines
On Saturday 12 April 2014, 14:48:17 wrote Alexander Shiyan:
Quoted 26 lines
Sat, 12 Apr 2014 12:42:18 +0200 ?? Alexander Stein <[email protected]>:
Quoted 25 lines
On Saturday 12 April 2014, 14:31:28 wrote Alexander Shiyan:
Quoted 23 lines
Sat, 12 Apr 2014 11:08:26 +0200 ?? Alexander Stein <[email protected]>:
Quoted 13 lines
Signed-off-by Alexander Stein <[email protected]>
---
 .../devicetree/bindings/sound/atmel_ac97c.txt      | 20 +++++++++
 sound/atmel/ac97c.c                                | 52 ++++++++++++++++++++--
 2 files changed, 69 insertions(+), 3 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/sound/atmel_ac97c.txt
Documentation/devicetree/bindings/sound/atmel_ac97c.txt new file +0 -0
new file mode 100644
index 0000000..9839403
Documentation/devicetree/bindings/sound/atmel_ac97c.txt line 7
Quoted 7 lines
Documentation/devicetree/bindings/sound/atmel_ac97c.txt new file +7 -0
@@ -0,0 +1,20 @@
+* Atmel AC97 controller
+
+Required properties:
+  - compatible: "atmel,atmel_ac97c"
+  - reg: Address and length of the register set for the device
+  - interrupts: Should contain AC97 interrupt
+  - atmel,reset-pin: GPIO for resetting the codec
Why standard "gpios" property cannot be used here?
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
I guess they can. I have no experience defining a devicetree binding, so I just stuck to the platformdata member name. If this is the recommended way, I'll gladly change that.
Another question is whether this property should refer to the controller, as it is clear that it refers to the codec ....
AFAICT the codec itself has no representation in the devicetree. It gets detected by the controller which uses the reset GPIO.

Quoted 8 lines

Yeah, maybe we can already go for ac97-gpios, there. It actually takes a list of gpios: ac97-sync, ac97-sdata and ac97-reset. You may want to have a look at Documentation/devicetree/bindings/sound/soc-ac97link.txt and snd_soc_set_ac97_ops_of_reset(), snd_soc_ac97_parse_pinctl() in sound/soc/core.c
The goal would be to be able to keep the same bindings when switching over to ASoC.

Those bindings are only for the description an AC97 codec attached, right? I'm just wondering what is ac97-sdata actually?
The datasheet to my microcontroller (atmel ac97c) has 4 signals:
* AC97CK - this would be omitted in the description as it's fixed 12,288MHz
* AC97RX - sdata_in
* AC97FS - is this ac97-sync?
* AC97TX - sdata_out

ac97-reset would be board-specific. Is ac97-reset one of those AC97TX or AC97RX?

Best regards,
Alexander

PS: I splitted Patch 3 to separate the device-tree binding and removed the device match as you suggested.

A message that is not in the archive (sent off-list or not yet received): 1397293707-26890-4-git-send-email-alexanders83-S0/[email protected]

Alexander Shiyan

2014-04-12 10:31 UTC #13 lore raw

Re: [PATCH 3/4] ALSA: sound/atmel/ac97c.c: Add device tree support

U2F0LCAxMiBBcHIgMjAxNCAxMTowODoyNiArMDIwMCDQvtGCIEFsZXhhbmRlciBTdGVpbiA8YWxl eGFuZGVyczgzQHdlYi5kZT46Cj4gU2lnbmVkLW9mZi1ieTogQWxleGFuZGVyIFN0ZWluIDxhbGV4 YW5kZXJzODNAd2ViLmRlPgo+IC0tLQo+ICAuLi4vZGV2aWNldHJlZS9iaW5kaW5ncy9zb3VuZC9h dG1lbF9hYzk3Yy50eHQgICAgICB8IDIwICsrKysrKysrKwo+ICBzb3VuZC9hdG1lbC9hYzk3Yy5j ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICB8IDUyICsrKysrKysrKysrKysrKysrKysr LS0KPiAgMiBmaWxlcyBjaGFuZ2VkLCA2OSBpbnNlcnRpb25zKCspLCAzIGRlbGV0aW9ucygtKQo+ ICBjcmVhdGUgbW9kZSAxMDA2NDQgRG9jdW1lbnRhdGlvbi9kZXZpY2V0cmVlL2JpbmRpbmdzL3Nv dW5kL2F0bWVsX2FjOTdjLnR4dAo+IAo+IGRpZmYgLS1naXQgYS9Eb2N1bWVudGF0aW9uL2Rldmlj ZXRyZWUvYmluZGluZ3Mvc291bmQvYXRtZWxfYWM5N2MudHh0IGIvRG9jdW1lbnRhdGlvbi9kZXZp Y2V0cmVlL2JpbmRpbmdzL3NvdW5kL2F0bWVsX2FjOTdjLnR4dAo+IG5ldyBmaWxlIG1vZGUgMTAw NjQ0Cj4gaW5kZXggMDAwMDAwMC4uOTgzOTQwMwo+IC0tLSAvZGV2L251bGwKPiArKysgYi9Eb2N1 bWVudGF0aW9uL2RldmljZXRyZWUvYmluZGluZ3Mvc291bmQvYXRtZWxfYWM5N2MudHh0Cj4gQEAg LTAsMCArMSwyMCBAQAo+ICsqIEF0bWVsIEFDOTcgY29udHJvbGxlcgo+ICsKPiArUmVxdWlyZWQg cHJvcGVydGllczoKPiArICAtIGNvbXBhdGlibGU6ICJhdG1lbCxhdG1lbF9hYzk3YyIKPiArICAt IHJlZzogQWRkcmVzcyBhbmQgbGVuZ3RoIG9mIHRoZSByZWdpc3RlciBzZXQgZm9yIHRoZSBkZXZp Y2UKPiArICAtIGludGVycnVwdHM6IFNob3VsZCBjb250YWluIEFDOTcgaW50ZXJydXB0Cj4gKyAg LSBhdG1lbCxyZXNldC1waW46IEdQSU8gZm9yIHJlc2V0dGluZyB0aGUgY29kZWMKCldoeSBzdGFu ZGFyZCAiZ3Bpb3MiIHByb3BlcnR5IGNhbm5vdCBiZSB1c2VkIGhlcmU/CgotLS0KCg== --
To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Alexander Shiyan

2014-04-12 10:31 UTC #14 lore raw

Re: [PATCH 3/4] ALSA: sound/atmel/ac97c.c: Add device tree support

Sat, 12 Apr 2014 11:08:26 +0200 от Alexander Stein <[email protected]>:

Quoted 13 lines

---
 .../devicetree/bindings/sound/atmel_ac97c.txt      | 20 +++++++++
 sound/atmel/ac97c.c                                | 52 ++++++++++++++++++++--
 2 files changed, 69 insertions(+), 3 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/sound/atmel_ac97c.txt

Documentation/devicetree/bindings/sound/atmel_ac97c.txt new file +0 -0

new file mode 100644
index 0000000..9839403

Documentation/devicetree/bindings/sound/atmel_ac97c.txt line 7

Quoted 7 lines

Documentation/devicetree/bindings/sound/atmel_ac97c.txt new file +7 -0

@@ -0,0 +1,20 @@
+* Atmel AC97 controller
+
+Required properties:
+  - compatible: "atmel,atmel_ac97c"
+  - reg: Address and length of the register set for the device
+  - interrupts: Should contain AC97 interrupt
+  - atmel,reset-pin: GPIO for resetting the codec

Why standard "gpios" property cannot be used here?

---

Alexandre Belloni

2014-04-14 23:07 UTC #15 lore raw

PATCH 3/4 ALSA: sound/atmel/ac97c.c: Add device tree support

On 12/04/2014 at 11:08:26 +0200, Alexander Stein wrote :

Even if dead simple, please try to include a commit log.

Quoted 4 lines

---
 .../devicetree/bindings/sound/atmel_ac97c.txt      | 20 +++++++++
 sound/atmel/ac97c.c                                | 52 ++++++++++++++++++++--

You should separate the documentation from the rest of the patch. The patch adding documentation is actually the only one of the series that should go to devicetree at vger.kernel.org and the devicetree maintainers.

Quoted 60 lines

 2 files changed, 69 insertions(+), 3 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/sound/atmel_ac97c.txt

Documentation/devicetree/bindings/sound/atmel_ac97c.txt new file +20 -0

new file mode 100644
index 0000000..9839403
@@ -0,0 +1,20 @@
+* Atmel AC97 controller
+
+Required properties:
+  - compatible: "atmel,atmel_ac97c"
+  - reg: Address and length of the register set for the device
+  - interrupts: Should contain AC97 interrupt
+  - atmel,reset-pin: GPIO for resetting the codec
+Optional properties:
+  - pinctrl-names, pinctrl-0: Please refer to pinctrl-bindings.txt
+
+Example:
+sound at fffa0000 {
+	compatible = "atmel,atmel_ac97c";
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_ac97>;
+	reg = <0xfffa0000 0x4000>;
+	interrupts = <18 IRQ_TYPE_LEVEL_HIGH 5>;
+
+	atmel,reset-pin = <&pioC 29 GPIO_ACTIVE_LOW>;
+};

sound/atmel/ac97c.c +16 -0

index 9a13875..b84432b 100644
@@ -22,6 +22,9 @@
 #include <linux/gpio.h>
 #include <linux/types.h>
 #include <linux/io.h>
+#include <linux/of.h>
+#include <linux/of_gpio.h>
+#include <linux/of_device.h>
 
 #include <sound/core.h>
 #include <sound/initval.h>

@@ -901,6 +904,47 @@ static void atmel_ac97c_reset(struct atmel_ac97c *chip)
 	}
 }
 
+#ifdef CONFIG_OF
+static const struct of_device_id atmel_ac97c_dt_ids[] = {
+	{ .compatible = "atmel,atmel_ac97c", },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, atmel_ac97c_dt_ids);
+
+static struct ac97c_platform_data *atmel_ac97c_probe_dt(struct device *dev)
+{
+	struct ac97c_platform_data *pdata;
+	struct device_node *node = dev->of_node;
+	const struct of_device_id *match;
+

sound/atmel/ac97c.c static void atmel_ac97c_reset(struct atmel_ac97c *chip) · line 929

Quoted 10 lines

sound/atmel/ac97c.c +10 -0

+	if (!node) {
+		dev_err(dev, "Device does not have associated DT data\n");
+		return ERR_PTR(-EINVAL);
+	}
+
+	match = of_match_device(atmel_ac97c_dt_ids, dev);
+	if (!match) {
+		dev_err(dev, "Unknown device model\n");
+		return ERR_PTR(-EINVAL);
+	}

You probably don't need that, it will always match. You would need it if you were actually using match (for example to get the .data member).

Quoted 52 lines

sound/atmel/ac97c.c +23 -3

+
+	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
+	if (!pdata)
+		return ERR_PTR(-ENOMEM);
+
+	pdata->reset_pin = of_get_named_gpio(dev->of_node,
+					     "atmel,reset-pin", 0);
+
+	return pdata;
+}
+#else
+static struct ac97c_platform_data *atmel_ac97c_probe_dt(struct device *dev)
+{
+	dev_err(dev, "no platform data defined\n");
+	return ERR_PTR(-ENXIO);
+}
+#endif
+
 static int atmel_ac97c_probe(struct platform_device *pdev)
 {
 	struct snd_card			*card;

@@ -921,10 +965,11 @@ static int atmel_ac97c_probe(struct platform_device *pdev)
 		return -ENXIO;
 	}
 
-	pdata = pdev->dev.platform_data;
+	pdata = dev_get_platdata(&pdev->dev);
 	if (!pdata) {
-		dev_dbg(&pdev->dev, "no platform data\n");
-		return -ENXIO;
+		pdata = atmel_ac97c_probe_dt(&pdev->dev);
+		if (IS_ERR(pdata))
+			return PTR_ERR(pdata);
 	}
 
 	irq = platform_get_irq(pdev, 0);

@@ -1208,6 +1253,7 @@ static struct platform_driver atmel_ac97c_driver = {
 		.name	= "atmel_ac97c",
 		.owner	= THIS_MODULE,
 		.pm	= ATMEL_AC97C_PM_OPS,
+		.of_match_table = of_match_ptr(atmel_ac97c_dt_ids),
 	},
 };
 module_platform_driver(atmel_ac97c_driver);

Signature · 7 lines

1.9.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Signature · 3 lines

Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Alexander Stein

author

2014-04-12 09:08 UTC #16 lore raw

PATCH 4/4 ARM: at91/dt: sam9263: Add ac97 device node

---
 arch/arm/boot/dts/at91sam9263.dtsi | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

arch/arm/boot/dts/at91sam9263.dtsi +19 -0

index fece866..34db6d8 100644
@@ -395,6 +395,16 @@
 					};
 				};
 
+				ac97 {
+					pinctrl_ac97: ac97-0 {
+						atmel,pins =
+							<AT91_PIOB 0 AT91_PERIPH_A AT91_PINCTRL_NONE	/* PB12 periph A AC97FS pin */
+							 AT91_PIOB 1 AT91_PERIPH_A AT91_PINCTRL_NONE	/* PB13 periph A AC97CK pin */
+							 AT91_PIOB 2 AT91_PERIPH_A AT91_PINCTRL_NONE	/* PB14 periph A AC97TX pin */
+							 AT91_PIOB 3 AT91_PERIPH_A AT91_PINCTRL_NONE>;	/* PB14 periph A AC97RX pin */
+					};
+				};
+
 				pioA: gpio at fffff200 {
 					compatible = "atmel,at91rm9200-gpio";
 					reg = <0xfffff200 0x200>;

@@ -506,6 +516,15 @@
 				status = "disabled";
 			};
 
+			ac97: sound at fffa0000 {
+				compatible = "atmel,atmel_ac97c";
+				reg = <0xfffa0000 0x4000>;
+				interrupts = <18 IRQ_TYPE_LEVEL_HIGH 5>;
+				pinctrl-names = "default";
+				pinctrl-0 = <&pinctrl_ac97>;
+				status = "disabled";
+			};
+
 			macb0: ethernet at fffbc000 {
 				compatible = "cdns,at32ap7000-macb", "cdns,macb";
 				reg = <0xfffbc000 0x100>;

Signature · 1 line

1.9.2

Bo Shen

2014-04-14 08:39 UTC #17 lore raw

Hi Alexander,

Quoted 4 lines · Alexander Stein

This set of patches add device tree support for the AC97 controller found on AT91SAM9263.
The first two patches are minor cleanup, while the last ones add the actual support.

I just test the whole patch series, and find issues to work with device tree kernel.
When we use dt kernel, no where assign the device id (so take it as "-1"), so when register devices, it will be /dev/snd/pcmC0D-1p and /dev/snd/pcmC0D-1c. So, the sound won't work.

The root cause is that when call "snd_pcm_new", it will pass "chip->pdev->id" as parameter.

So, I am thinking whether this should be fix in <sound/atmel/ac97c.c> or in <sound/core/pcm.c>

Quoted 2 lines

Regards,
Alexander

Best Regards,
Bo Shen

Alexander Stein

author

2014-04-14 18:34 UTC #18 lore raw

Hello Bo,

On Monday 14 April 2014, 16:39:43 wrote Bo Shen:

Quoted 12 lines · Alexander Stein

Quoted 4 lines
This set of patches add device tree support for the AC97 controller found on AT91SAM9263.
The first two patches are minor cleanup, while the last ones add the actual support.
I just test the whole patch series, and find issues to work with device tree kernel.
When we use dt kernel, no where assign the device id (so take it as "-1"), so when register devices, it will be /dev/snd/pcmC0D-1p and /dev/snd/pcmC0D-1c. So, the sound won't work.

Quoted 5 lines

The root cause is that when call "snd_pcm_new", it will pass "chip->pdev->id" as parameter.
So, I am thinking whether this should be fix in <sound/atmel/ac97c.c> or in <sound/core/pcm.c>

Ah. thanks for figuring out. I also noticed later that e.g. aplay actually doesn't work, but could not figure out why. pcmC0D-1c looked valid at first glance I wasn't aware that this includes a -1 :) I was already happy that the controller found the codec.

Best regards,
Alexander

Takashi Iwai

2014-04-14 08:42 UTC #19 lore raw

At Sat, 12 Apr 2014 11:08:23 +0200,

Quoted 5 lines · Alexander Stein

This set of patches add device tree support for the AC97 controller found on AT91SAM9263.
The first two patches are minor cleanup, while the last ones add the actual support.

Do you really need work on this driver? The sound/atmel/* stuff is an old one before ASoC was introduced, mostly obsoleted nowadays.

Takashi

Alexander Stein

author

2014-04-14 18:43 UTC #20 lore raw

Hello Takashi,

On Monday 14 April 2014, 10:42:06 wrote Takashi Iwai:

Quoted 6 lines

At Sat, 12 Apr 2014 11:08:23 +0200,
Quoted 4 lines · Alexander Stein
This set of patches add device tree support for the AC97 controller found on AT91SAM9263.
The first two patches are minor cleanup, while the last ones add the

actual

Quoted 4 lines

Quoted 1 line
support.
Do you really need work on this driver? The sound/atmel/* stuff is an old one before ASoC was introduced, mostly obsoleted nowadays.

Ah, ok. I wasn't aware of that. But is there a replacement for the ac97c driver? All I found in sound/soc/atmel is using the SSC peripheral. Not something I can use.
Regards,
Alexander

Alexandre Belloni

2014-04-14 22:47 UTC #21 lore raw

Quoted 10 lines · Alexander Stein

Hello Takashi,
On Monday 14 April 2014, 10:42:06 wrote Takashi Iwai:
Quoted 6 lines
At Sat, 12 Apr 2014 11:08:23 +0200,
Quoted 4 lines · Alexander Stein
This set of patches add device tree support for the AC97 controller found on AT91SAM9263.
The first two patches are minor cleanup, while the last ones add the
actual

Quoted 8 lines

Quoted 4 lines
Quoted 1 line
support.
Do you really need work on this driver? The sound/atmel/* stuff is an old one before ASoC was introduced, mostly obsoleted nowadays.
Ah, ok. I wasn't aware of that. But is there a replacement for the ac97c driver? All I found in sound/soc/atmel is using the SSC peripheral. Not something I can use.

I guess what Takashi wanted to say was: shouldn't we rewrite the driver completely to take advantage of the ASoC infrastructure ?

I'm actually quite interesting in getting it to work as this is the la peripheral that doesn't have DT support on the SAM9RL.

I'll make some comments on your patches but I'm not sure whether we should keep that driver or move to ASoC. Maybe that could be an intermediate step. I'll try to have a look this week.

Thank you for your work !

Signature · 3 lines

Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Takashi Iwai

2014-04-15 05:59 UTC #22 lore raw

At Tue, 15 Apr 2014 00:47:11 +0200,

Quoted 17 lines · Alexandre Belloni

Quoted 15 lines · Alexander Stein
Hello Takashi,
On Monday 14 April 2014, 10:42:06 wrote Takashi Iwai:
Quoted 6 lines
At Sat, 12 Apr 2014 11:08:23 +0200,
Quoted 4 lines · Alexander Stein
This set of patches add device tree support for the AC97 controller found on AT91SAM9263.
The first two patches are minor cleanup, while the last ones add the
actual
Quoted 4 lines
Quoted 1 line
support.
Do you really need work on this driver? The sound/atmel/* stuff is an old one before ASoC was introduced, mostly obsoleted nowadays.

Quoted 6 lines

Quoted 3 lines
Ah, ok. I wasn't aware of that. But is there a replacement for the ac97c driver? All I found in sound/soc/atmel is using the SSC peripheral. Not something I can use.
I guess what Takashi wanted to say was: shouldn't we rewrite the driver completely to take advantage of the ASoC infrastructure ?

Yep, exactly. Thanks for my sparse word completion :)

Quoted 6 lines

I'm actually quite interesting in getting it to work as this is the la peripheral that doesn't have DT support on the SAM9RL.
I'll make some comments on your patches but I'm not sure whether we should keep that driver or move to ASoC. Maybe that could be an intermediate step. I'll try to have a look this week.

OK, if people think it's still useful, I can take the patches without problems. The changes are small and easy enough. But, the only condition is that it's assured that the changes / binding is applicable to ASoC (in future, if any). That said, it'd be great if anyone can work on the proper integration to ASoC at the same time.

thanks,

Takashi

Alexander Stein

author

2014-04-15 17:39 UTC #23 lore raw

PATCH v2 1/2 ALSA: sound/atmel/ac97c.c: Convert to module_platform_driver

This reduces some boilerplate code.

---
 sound/atmel/ac97c.c | 15 ++-------------
 1 file changed, 2 insertions(+), 13 deletions(-)

sound/atmel/ac97c.c +2 ~~-13~~

index 05ec049..a04d2317 100644
@@ -1198,6 +1198,7 @@ static int atmel_ac97c_remove(struct platform_device *pdev)
 }
 
 static struct platform_driver atmel_ac97c_driver = {
+	.probe		= atmel_ac97c_probe,
 	.remove		= atmel_ac97c_remove,
 	.driver		= {
 		.name	= "atmel_ac97c",

@@ -1205,19 +1206,7 @@ static struct platform_driver atmel_ac97c_driver = {
 		.pm	= ATMEL_AC97C_PM_OPS,
 	},
 };
-
-static int __init atmel_ac97c_init(void)
-{
-	return platform_driver_probe(&atmel_ac97c_driver,
-			atmel_ac97c_probe);
-}
-module_init(atmel_ac97c_init);
-
-static void __exit atmel_ac97c_exit(void)
-{
-	platform_driver_unregister(&atmel_ac97c_driver);
-}
-module_exit(atmel_ac97c_exit);
+module_platform_driver(atmel_ac97c_driver);
 
 MODULE_LICENSE("GPL");
 MODULE_DESCRIPTION("Driver for Atmel AC97 controller");

Signature · 1 line

1.9.2

Alexander Stein

author

2014-04-15 17:39 UTC #24 lore raw

PATCH v2 2/2 ALSA: sound/atmel-ac97c.h: Remove unused flags from platform data

This platform data member is unused, so remove it.

---
 include/sound/atmel-ac97c.h | 2 --
 1 file changed, 2 deletions(-)

include/sound/atmel-ac97c.h +0 -2

index e6aabdb..00e6c289 100644
@@ -23,7 +23,6 @@
  * @reset_pin: GPIO pin wired to the reset input on the external AC97 codec,
  *             optional to use, set to -ENODEV if not in use. AC97 layer will
  *             try to do a software reset of the external codec anyway.
- * @flags: Flags for which directions should be enabled.
  *
  * If the user do not want to use a DMA channel for playback or capture, i.e.
  * only one feature is required on the board. The slave for playback or capture

@@ -33,7 +32,6 @@
 struct ac97c_platform_data {
 	struct dw_dma_slave	rx_dws;
 	struct dw_dma_slave	tx_dws;
-	unsigned int 		flags;
 	int			reset_pin;
 };

Signature · 1 line

1.9.2

Nicolas Ferre

Acked-by on v2 2/2

2014-04-16 07:39 UTC #25 lore raw

Re: [PATCH v2 2/2] ALSA: sound/atmel-ac97c.h: Remove unused flags from platform data

On 15/04/2014 19:38, Alexander Stein :

Quoted 4 lines

This platform data member is unused, so remove it.
Signed-off-by Alexander Stein <[email protected]>
Acked-by Alexandre Belloni <[email protected]>

Acked-by Nicolas Ferre <[email protected]>

Quoted 25 lines

---
 include/sound/atmel-ac97c.h | 2 --
 1 file changed, 2 deletions(-)

include/sound/atmel-ac97c.h +0 -2

index e6aabdb..00e6c289 100644
@@ -23,7 +23,6 @@
  * @reset_pin: GPIO pin wired to the reset input on the external AC97 codec,
  *             optional to use, set to -ENODEV if not in use. AC97 layer will
  *             try to do a software reset of the external codec anyway.
- * @flags: Flags for which directions should be enabled.
  *
  * If the user do not want to use a DMA channel for playback or capture, i.e.
  * only one feature is required on the board. The slave for playback or capture

@@ -33,7 +32,6 @@
 struct ac97c_platform_data {
 	struct dw_dma_slave	rx_dws;
 	struct dw_dma_slave	tx_dws;
-	unsigned int 		flags;
 	int			reset_pin;
 };

Signature · 1 line

Nicolas Ferre

Nicolas Ferre

Acked-by on v2 1/2

2014-04-16 07:39 UTC #26 lore raw

Re: [PATCH v2 1/2] ALSA: sound/atmel/ac97c.c: Convert to module_platform_driver

On 15/04/2014 19:38, Alexander Stein :

Quoted 4 lines

This reduces some boilerplate code.
Signed-off-by Alexander Stein <[email protected]>
Acked-by Alexandre Belloni <[email protected]>

Acked-by Nicolas Ferre <[email protected]>

Thanks

Quoted 38 lines

---
 sound/atmel/ac97c.c | 15 ++-------------
 1 file changed, 2 insertions(+), 13 deletions(-)

sound/atmel/ac97c.c +2 ~~-13~~

index 05ec049..a04d2317 100644
@@ -1198,6 +1198,7 @@ static int atmel_ac97c_remove(struct platform_device *pdev)
 }
 
 static struct platform_driver atmel_ac97c_driver = {
+	.probe		= atmel_ac97c_probe,
 	.remove		= atmel_ac97c_remove,
 	.driver		= {
 		.name	= "atmel_ac97c",

@@ -1205,19 +1206,7 @@ static struct platform_driver atmel_ac97c_driver = {
 		.pm	= ATMEL_AC97C_PM_OPS,
 	},
 };
-
-static int __init atmel_ac97c_init(void)
-{
-	return platform_driver_probe(&atmel_ac97c_driver,
-			atmel_ac97c_probe);
-}
-module_init(atmel_ac97c_init);
-
-static void __exit atmel_ac97c_exit(void)
-{
-	platform_driver_unregister(&atmel_ac97c_driver);
-}
-module_exit(atmel_ac97c_exit);
+module_platform_driver(atmel_ac97c_driver);
 
 MODULE_LICENSE("GPL");
 MODULE_DESCRIPTION("Driver for Atmel AC97 controller");

Signature · 1 line

Nicolas Ferre

Takashi Iwai

Applied

2014-04-16 08:23 UTC #27 lore raw

Re: [PATCH v2 1/2] ALSA: sound/atmel/ac97c.c: Convert to module_platform_driver