Device tree support for Atmel AC97 controller on AT91SAM9263

Device tree support for Atmel AC97 controller on AT91SAM9263

[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.

Regards,
Alexander

Device tree support for Atmel AC97 controller on AT91SAM9263

[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.

Regards,
Alexander

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

[Alexander Stein]

THis reduces some boilerplate code.

Signed-off-by: Alexander Stein <alexanders83@web.de>
---
 sound/atmel/ac97c.c | 15 ++-------------
 1 file changed, 2 insertions(+), 13 deletions(-)

diff --git a/sound/atmel/ac97c.c b/sound/atmel/ac97c.c
index c5f0ddd..9a13875 100644
--- a/sound/atmel/ac97c.c
+++ b/sound/atmel/ac97c.c
@@ -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");
-- 
1.9.2

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

[Alexander Stein]

THis reduces some boilerplate code.

Signed-off-by: Alexander Stein <alexanders83@web.de>
---
 sound/atmel/ac97c.c | 15 ++-------------
 1 file changed, 2 insertions(+), 13 deletions(-)

diff --git a/sound/atmel/ac97c.c b/sound/atmel/ac97c.c
index c5f0ddd..9a13875 100644
--- a/sound/atmel/ac97c.c
+++ b/sound/atmel/ac97c.c
@@ -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");
-- 
1.9.2

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

[Alexander Stein]

This platform data member is unsed, so remove it.

Signed-off-by: Alexander Stein <alexanders83@web.de>
---
 include/sound/atmel-ac97c.h | 2 --
 1 file changed, 2 deletions(-)

diff --git a/include/sound/atmel-ac97c.h b/include/sound/atmel-ac97c.h
index e6aabdb..00e6c289 100644
--- a/include/sound/atmel-ac97c.h
+++ b/include/sound/atmel-ac97c.h
@@ -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;
 };
 
-- 
1.9.2

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

[Alexander Stein]

This platform data member is unsed, so remove it.

Signed-off-by: Alexander Stein <alexanders83@web.de>
---
 include/sound/atmel-ac97c.h | 2 --
 1 file changed, 2 deletions(-)

diff --git a/include/sound/atmel-ac97c.h b/include/sound/atmel-ac97c.h
index e6aabdb..00e6c289 100644
--- a/include/sound/atmel-ac97c.h
+++ b/include/sound/atmel-ac97c.h
@@ -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;
 };
 
-- 
1.9.2

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

[Alexander Stein]

Signed-off-by: Alexander Stein <alexanders83@web.de>
---
 .../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

diff --git a/Documentation/devicetree/bindings/sound/atmel_ac97c.txt b/Documentation/devicetree/bindings/sound/atmel_ac97c.txt
new file mode 100644
index 0000000..9839403
--- /dev/null
+++ b/Documentation/devicetree/bindings/sound/atmel_ac97c.txt
@@ -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@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>;
+};
diff --git a/sound/atmel/ac97c.c b/sound/atmel/ac97c.c
index 9a13875..b84432b 100644
--- a/sound/atmel/ac97c.c
+++ b/sound/atmel/ac97c.c
@@ -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;
+
+	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);
+	}
+
+	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);
-- 
1.9.2

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

[Alexander Stein]

Signed-off-by: Alexander Stein <alexanders83@web.de>
---
 .../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

diff --git a/Documentation/devicetree/bindings/sound/atmel_ac97c.txt b/Documentation/devicetree/bindings/sound/atmel_ac97c.txt
new file mode 100644
index 0000000..9839403
--- /dev/null
+++ b/Documentation/devicetree/bindings/sound/atmel_ac97c.txt
@@ -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>;
+};
diff --git a/sound/atmel/ac97c.c b/sound/atmel/ac97c.c
index 9a13875..b84432b 100644
--- a/sound/atmel/ac97c.c
+++ b/sound/atmel/ac97c.c
@@ -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;
+
+	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);
+	}
+
+	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);
-- 
1.9.2

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

[Alexander Stein]

Signed-off-by: Alexander Stein <alexanders83@web.de>
---
 arch/arm/boot/dts/at91sam9263.dtsi | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/arch/arm/boot/dts/at91sam9263.dtsi b/arch/arm/boot/dts/at91sam9263.dtsi
index fece866..34db6d8 100644
--- a/arch/arm/boot/dts/at91sam9263.dtsi
+++ b/arch/arm/boot/dts/at91sam9263.dtsi
@@ -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@fffff200 {
 					compatible = "atmel,at91rm9200-gpio";
 					reg = <0xfffff200 0x200>;
@@ -506,6 +516,15 @@
 				status = "disabled";
 			};
 
+			ac97: sound@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@fffbc000 {
 				compatible = "cdns,at32ap7000-macb", "cdns,macb";
 				reg = <0xfffbc000 0x100>;
-- 
1.9.2

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

[Alexander Stein]

Signed-off-by: Alexander Stein <alexanders83@web.de>
---
 arch/arm/boot/dts/at91sam9263.dtsi | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/arch/arm/boot/dts/at91sam9263.dtsi b/arch/arm/boot/dts/at91sam9263.dtsi
index fece866..34db6d8 100644
--- a/arch/arm/boot/dts/at91sam9263.dtsi
+++ b/arch/arm/boot/dts/at91sam9263.dtsi
@@ -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>;
-- 
1.9.2

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

[Alexander Shiyan]

Sat, 12 Apr 2014 11:08:26 +0200 от Alexander Stein <alexanders83@web.de>:
> Signed-off-by: Alexander Stein <alexanders83@web.de>
> ---
>  .../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
> 
> diff --git a/Documentation/devicetree/bindings/sound/atmel_ac97c.txt b/Documentation/devicetree/bindings/sound/atmel_ac97c.txt
> new file mode 100644
> index 0000000..9839403
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/sound/atmel_ac97c.txt
> @@ -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?

---

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

[Alexander Shiyan]

Sat, 12 Apr 2014 11:08:26 +0200 от Alexander Stein <alexanders83@web.de>:
> Signed-off-by: Alexander Stein <alexanders83@web.de>
> ---
>  .../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
> 
> diff --git a/Documentation/devicetree/bindings/sound/atmel_ac97c.txt b/Documentation/devicetree/bindings/sound/atmel_ac97c.txt
> new file mode 100644
> index 0000000..9839403
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/sound/atmel_ac97c.txt
> @@ -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?

---

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

[Alexander Shiyan]

Sat, 12 Apr 2014 11:08:26 +0200 от Alexander Stein <alexanders83@web.de>:
> Signed-off-by: Alexander Stein <alexanders83@web.de>
> ---
>  .../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
> 
> diff --git a/Documentation/devicetree/bindings/sound/atmel_ac97c.txt b/Documentation/devicetree/bindings/sound/atmel_ac97c.txt
> new file mode 100644
> index 0000000..9839403
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/sound/atmel_ac97c.txt
> @@ -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?

---

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

[Alexander Shiyan]

Sat, 12 Apr 2014 11:08:26 +0200 ?? Alexander Stein <alexanders83@web.de>:
> Signed-off-by: Alexander Stein <alexanders83@web.de>
> ---
>  .../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
> 
> diff --git a/Documentation/devicetree/bindings/sound/atmel_ac97c.txt b/Documentation/devicetree/bindings/sound/atmel_ac97c.txt
> new file mode 100644
> index 0000000..9839403
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/sound/atmel_ac97c.txt
> @@ -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?

---

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

[Alexander Stein]

On Saturday 12 April 2014, 14:31:28 wrote Alexander Shiyan:
> Sat, 12 Apr 2014 11:08:26 +0200 от Alexander Stein <alexanders83@web.de>:
> > Signed-off-by: Alexander Stein <alexanders83@web.de>
> > ---
> >  .../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
> > 
> > diff --git a/Documentation/devicetree/bindings/sound/atmel_ac97c.txt b/Documentation/devicetree/bindings/sound/atmel_ac97c.txt
> > new file mode 100644
> > index 0000000..9839403
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/sound/atmel_ac97c.txt
> > @@ -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


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

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

[Alexander Stein]

On Saturday 12 April 2014, 14:31:28 wrote Alexander Shiyan:
> Sat, 12 Apr 2014 11:08:26 +0200 ?? Alexander Stein <alexanders83@web.de>:
> > Signed-off-by: Alexander Stein <alexanders83@web.de>
> > ---
> >  .../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
> > 
> > diff --git a/Documentation/devicetree/bindings/sound/atmel_ac97c.txt b/Documentation/devicetree/bindings/sound/atmel_ac97c.txt
> > new file mode 100644
> > index 0000000..9839403
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/sound/atmel_ac97c.txt
> > @@ -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

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

[Alexander Shiyan]

Sat, 12 Apr 2014 12:42:18 +0200 от Alexander Stein <alexanders83@web.de>:
> On Saturday 12 April 2014, 14:31:28 wrote Alexander Shiyan:
> > Sat, 12 Apr 2014 11:08:26 +0200 от Alexander Stein <alexanders83@web.de>:
> > > Signed-off-by: Alexander Stein <alexanders83@web.de>
> > > ---
> > >  .../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
> > > 
> > > diff --git a/Documentation/devicetree/bindings/sound/atmel_ac97c.txt b/Documentation/devicetree/bindings/sound/atmel_ac97c.txt
> > > new file mode 100644
> > > index 0000000..9839403
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/sound/atmel_ac97c.txt
> > > @@ -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.

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

---

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

[Alexander Shiyan]

Sat, 12 Apr 2014 12:42:18 +0200 ?? Alexander Stein <alexanders83@web.de>:
> On Saturday 12 April 2014, 14:31:28 wrote Alexander Shiyan:
> > Sat, 12 Apr 2014 11:08:26 +0200 ?? Alexander Stein <alexanders83@web.de>:
> > > Signed-off-by: Alexander Stein <alexanders83@web.de>
> > > ---
> > >  .../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
> > > 
> > > diff --git a/Documentation/devicetree/bindings/sound/atmel_ac97c.txt b/Documentation/devicetree/bindings/sound/atmel_ac97c.txt
> > > new file mode 100644
> > > index 0000000..9839403
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/sound/atmel_ac97c.txt
> > > @@ -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.

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

---

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

[Alexander Stein]

On Saturday 12 April 2014, 14:48:17 wrote Alexander Shiyan:
> Sat, 12 Apr 2014 12:42:18 +0200 от Alexander Stein <alexanders83@web.de>:
> > On Saturday 12 April 2014, 14:31:28 wrote Alexander Shiyan:
> > > Sat, 12 Apr 2014 11:08:26 +0200 от Alexander Stein <alexanders83@web.de>:
> > > > Signed-off-by: Alexander Stein <alexanders83@web.de>
> > > > ---
> > > >  .../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
> > > > 
> > > > diff --git a/Documentation/devicetree/bindings/sound/atmel_ac97c.txt b/Documentation/devicetree/bindings/sound/atmel_ac97c.txt
> > > > new file mode 100644
> > > > index 0000000..9839403
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/sound/atmel_ac97c.txt
> > > > @@ -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.
> 
> 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

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

[Alexander Stein]

On Saturday 12 April 2014, 14:48:17 wrote Alexander Shiyan:
> Sat, 12 Apr 2014 12:42:18 +0200 ?? Alexander Stein <alexanders83@web.de>:
> > On Saturday 12 April 2014, 14:31:28 wrote Alexander Shiyan:
> > > Sat, 12 Apr 2014 11:08:26 +0200 ?? Alexander Stein <alexanders83@web.de>:
> > > > Signed-off-by: Alexander Stein <alexanders83@web.de>
> > > > ---
> > > >  .../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
> > > > 
> > > > diff --git a/Documentation/devicetree/bindings/sound/atmel_ac97c.txt b/Documentation/devicetree/bindings/sound/atmel_ac97c.txt
> > > > new file mode 100644
> > > > index 0000000..9839403
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/sound/atmel_ac97c.txt
> > > > @@ -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.
> 
> 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

Re: Device tree support for Atmel AC97 controller on AT91SAM9263

[Bo Shen]

Hi Alexander,

On 04/12/2014 05:08 PM, Alexander Stein wrote:
> 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>

> Regards,
> Alexander

Best Regards,
Bo Shen

Device tree support for Atmel AC97 controller on AT91SAM9263

[Bo Shen]

Hi Alexander,

On 04/12/2014 05:08 PM, Alexander Stein wrote:
> 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>

> Regards,
> Alexander

Best Regards,
Bo Shen

Re: Device tree support for Atmel AC97 controller on AT91SAM9263

[Takashi Iwai]

At Sat, 12 Apr 2014 11:08:23 +0200,
Alexander Stein wrote:
> 
> 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

Device tree support for Atmel AC97 controller on AT91SAM9263

[Takashi Iwai]

At Sat, 12 Apr 2014 11:08:23 +0200,
Alexander Stein wrote:
> 
> 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

Re: Re: Device tree support for Atmel AC97 controller on AT91SAM9263

[Alexander Stein]

Hello Bo,

On Monday 14 April 2014, 16:39:43 wrote Bo Shen:
> On 04/12/2014 05:08 PM, Alexander Stein wrote:
> > 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>

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

Device tree support for Atmel AC97 controller on AT91SAM9263

[Alexander Stein]

Hello Bo,

On Monday 14 April 2014, 16:39:43 wrote Bo Shen:
> On 04/12/2014 05:08 PM, Alexander Stein wrote:
> > 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>

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

Re: Re: Device tree support for Atmel AC97 controller on AT91SAM9263

[Alexander Stein]

Hello Takashi,

On Monday 14 April 2014, 10:42:06 wrote Takashi Iwai:
> At Sat, 12 Apr 2014 11:08:23 +0200,
> Alexander Stein wrote:
> > 
> > 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.

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

Device tree support for Atmel AC97 controller on AT91SAM9263

[Alexander Stein]

Hello Takashi,

On Monday 14 April 2014, 10:42:06 wrote Takashi Iwai:
> At Sat, 12 Apr 2014 11:08:23 +0200,
> Alexander Stein wrote:
> > 
> > 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.

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

Re: Re: Device tree support for Atmel AC97 controller on AT91SAM9263

[Alexandre Belloni]

On 14/04/2014 at 20:43:02 +0200, Alexander Stein wrote :
> Hello Takashi,
> 
> On Monday 14 April 2014, 10:42:06 wrote Takashi Iwai:
> > At Sat, 12 Apr 2014 11:08:23 +0200,
> > Alexander Stein wrote:
> > > 
> > > 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.
> 
> 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 !

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

Device tree support for Atmel AC97 controller on AT91SAM9263

[Alexandre Belloni]

On 14/04/2014 at 20:43:02 +0200, Alexander Stein wrote :
> Hello Takashi,
> 
> On Monday 14 April 2014, 10:42:06 wrote Takashi Iwai:
> > At Sat, 12 Apr 2014 11:08:23 +0200,
> > Alexander Stein wrote:
> > > 
> > > 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.
> 
> 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 !

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

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

[Alexandre Belloni]

On 12/04/2014 at 11:08:24 +0200, Alexander Stein wrote :
> THis reduces some boilerplate code.
   ^
   small typo
> 
> Signed-off-by: Alexander Stein <alexanders83@web.de>
else
Acked-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
> ---
>  sound/atmel/ac97c.c | 15 ++-------------
>  1 file changed, 2 insertions(+), 13 deletions(-)
> 
> diff --git a/sound/atmel/ac97c.c b/sound/atmel/ac97c.c
> index c5f0ddd..9a13875 100644
> --- a/sound/atmel/ac97c.c
> +++ b/sound/atmel/ac97c.c
> @@ -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");
> -- 
> 1.9.2
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

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

[Alexandre Belloni]

On 12/04/2014 at 11:08:24 +0200, Alexander Stein wrote :
> THis reduces some boilerplate code.
   ^
   small typo
> 
> Signed-off-by: Alexander Stein <alexanders83@web.de>
else
Acked-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
> ---
>  sound/atmel/ac97c.c | 15 ++-------------
>  1 file changed, 2 insertions(+), 13 deletions(-)
> 
> diff --git a/sound/atmel/ac97c.c b/sound/atmel/ac97c.c
> index c5f0ddd..9a13875 100644
> --- a/sound/atmel/ac97c.c
> +++ b/sound/atmel/ac97c.c
> @@ -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");
> -- 
> 1.9.2
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

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

[Alexandre Belloni]

On 12/04/2014 at 11:08:25 +0200, Alexander Stein wrote :
> This platform data member is unsed, so remove it.
                                 ^
                                 another typo :)
> 
> Signed-off-by: Alexander Stein <alexanders83@web.de>
I would say that it doesn't matter anymore but no code is better tahn
dead code.

Acked-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
> ---
>  include/sound/atmel-ac97c.h | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/include/sound/atmel-ac97c.h b/include/sound/atmel-ac97c.h
> index e6aabdb..00e6c289 100644
> --- a/include/sound/atmel-ac97c.h
> +++ b/include/sound/atmel-ac97c.h
> @@ -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;
>  };
>  
> -- 
> 1.9.2
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

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

[Alexandre Belloni]

On 12/04/2014 at 11:08:25 +0200, Alexander Stein wrote :
> This platform data member is unsed, so remove it.
                                 ^
                                 another typo :)
> 
> Signed-off-by: Alexander Stein <alexanders83@web.de>
I would say that it doesn't matter anymore but no code is better tahn
dead code.

Acked-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
> ---
>  include/sound/atmel-ac97c.h | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/include/sound/atmel-ac97c.h b/include/sound/atmel-ac97c.h
> index e6aabdb..00e6c289 100644
> --- a/include/sound/atmel-ac97c.h
> +++ b/include/sound/atmel-ac97c.h
> @@ -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;
>  };
>  
> -- 
> 1.9.2
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

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

[Alexandre Belloni]

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

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

> Signed-off-by: Alexander Stein <alexanders83@web.de>
> ---
>  .../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@vger.kernel.org and the devicetree maintainers.

>  2 files changed, 69 insertions(+), 3 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/sound/atmel_ac97c.txt
> 
> diff --git a/Documentation/devicetree/bindings/sound/atmel_ac97c.txt b/Documentation/devicetree/bindings/sound/atmel_ac97c.txt
> new file mode 100644
> index 0000000..9839403
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/sound/atmel_ac97c.txt
> @@ -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@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>;
> +};
> diff --git a/sound/atmel/ac97c.c b/sound/atmel/ac97c.c
> index 9a13875..b84432b 100644
> --- a/sound/atmel/ac97c.c
> +++ b/sound/atmel/ac97c.c
> @@ -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;
> +
> +	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).

> +
> +	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);
> -- 
> 1.9.2
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

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

[Alexandre Belloni]

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

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

> Signed-off-by: Alexander Stein <alexanders83@web.de>
> ---
>  .../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.

>  2 files changed, 69 insertions(+), 3 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/sound/atmel_ac97c.txt
> 
> diff --git a/Documentation/devicetree/bindings/sound/atmel_ac97c.txt b/Documentation/devicetree/bindings/sound/atmel_ac97c.txt
> new file mode 100644
> index 0000000..9839403
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/sound/atmel_ac97c.txt
> @@ -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>;
> +};
> diff --git a/sound/atmel/ac97c.c b/sound/atmel/ac97c.c
> index 9a13875..b84432b 100644
> --- a/sound/atmel/ac97c.c
> +++ b/sound/atmel/ac97c.c
> @@ -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;
> +
> +	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).

> +
> +	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);
> -- 
> 1.9.2
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

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

[Alexandre Belloni]

On 13/04/2014 at 10:32:09 +0200, Alexander Stein wrote :
> On Saturday 12 April 2014, 14:48:17 wrote Alexander Shiyan:
> > Sat, 12 Apr 2014 12:42:18 +0200 от Alexander Stein <alexanders83@web.de>:
> > > On Saturday 12 April 2014, 14:31:28 wrote Alexander Shiyan:
> > > > Sat, 12 Apr 2014 11:08:26 +0200 от Alexander Stein <alexanders83@web.de>:
> > > > > Signed-off-by: Alexander Stein <alexanders83@web.de>
> > > > > ---
> > > > >  .../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
> > > > > 
> > > > > diff --git a/Documentation/devicetree/bindings/sound/atmel_ac97c.txt b/Documentation/devicetree/bindings/sound/atmel_ac97c.txt
> > > > > new file mode 100644
> > > > > index 0000000..9839403
> > > > > --- /dev/null
> > > > > +++ b/Documentation/devicetree/bindings/sound/atmel_ac97c.txt
> > > > > @@ -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.
> > 
> > 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.

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

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

[Alexandre Belloni]

On 13/04/2014 at 10:32:09 +0200, Alexander Stein wrote :
> On Saturday 12 April 2014, 14:48:17 wrote Alexander Shiyan:
> > Sat, 12 Apr 2014 12:42:18 +0200 ?? Alexander Stein <alexanders83@web.de>:
> > > On Saturday 12 April 2014, 14:31:28 wrote Alexander Shiyan:
> > > > Sat, 12 Apr 2014 11:08:26 +0200 ?? Alexander Stein <alexanders83@web.de>:
> > > > > Signed-off-by: Alexander Stein <alexanders83@web.de>
> > > > > ---
> > > > >  .../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
> > > > > 
> > > > > diff --git a/Documentation/devicetree/bindings/sound/atmel_ac97c.txt b/Documentation/devicetree/bindings/sound/atmel_ac97c.txt
> > > > > new file mode 100644
> > > > > index 0000000..9839403
> > > > > --- /dev/null
> > > > > +++ b/Documentation/devicetree/bindings/sound/atmel_ac97c.txt
> > > > > @@ -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.
> > 
> > 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.

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

Re: Device tree support for Atmel AC97 controller on AT91SAM9263

[Takashi Iwai]

At Tue, 15 Apr 2014 00:47:11 +0200,
Alexandre Belloni wrote:
> 
> On 14/04/2014 at 20:43:02 +0200, Alexander Stein wrote :
> > Hello Takashi,
> > 
> > On Monday 14 April 2014, 10:42:06 wrote Takashi Iwai:
> > > At Sat, 12 Apr 2014 11:08:23 +0200,
> > > Alexander Stein wrote:
> > > > 
> > > > 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.
> > 
> > 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 :)

> 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

Device tree support for Atmel AC97 controller on AT91SAM9263

[Takashi Iwai]

At Tue, 15 Apr 2014 00:47:11 +0200,
Alexandre Belloni wrote:
> 
> On 14/04/2014 at 20:43:02 +0200, Alexander Stein wrote :
> > Hello Takashi,
> > 
> > On Monday 14 April 2014, 10:42:06 wrote Takashi Iwai:
> > > At Sat, 12 Apr 2014 11:08:23 +0200,
> > > Alexander Stein wrote:
> > > > 
> > > > 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.
> > 
> > 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 :)

> 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

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

[Alexander Stein]

This reduces some boilerplate code.

Signed-off-by: Alexander Stein <alexanders83@web.de>
Acked-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
---
 sound/atmel/ac97c.c | 15 ++-------------
 1 file changed, 2 insertions(+), 13 deletions(-)

diff --git a/sound/atmel/ac97c.c b/sound/atmel/ac97c.c
index 05ec049..a04d2317 100644
--- a/sound/atmel/ac97c.c
+++ b/sound/atmel/ac97c.c
@@ -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");
-- 
1.9.2

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

[Alexander Stein]

This platform data member is unused, so remove it.

Signed-off-by: Alexander Stein <alexanders83@web.de>
Acked-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
---
 include/sound/atmel-ac97c.h | 2 --
 1 file changed, 2 deletions(-)

diff --git a/include/sound/atmel-ac97c.h b/include/sound/atmel-ac97c.h
index e6aabdb..00e6c289 100644
--- a/include/sound/atmel-ac97c.h
+++ b/include/sound/atmel-ac97c.h
@@ -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;
 };
 
-- 
1.9.2

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

[Alexander Stein]

On Tuesday 15 April 2014, 01:21:34 wrote Alexandre Belloni:
> On 13/04/2014 at 10:32:09 +0200, Alexander Stein wrote :
> > On Saturday 12 April 2014, 14:48:17 wrote Alexander Shiyan:
> > > Sat, 12 Apr 2014 12:42:18 +0200 от Alexander Stein <alexanders83@web.de>:
> > > > On Saturday 12 April 2014, 14:31:28 wrote Alexander Shiyan:
> > > > > Sat, 12 Apr 2014 11:08:26 +0200 от Alexander Stein <alexanders83@web.de>:
> > > > > > Signed-off-by: Alexander Stein <alexanders83@web.de>
> > > > > > ---
> > > > > >  .../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
> > > > > > 
> > > > > > diff --git a/Documentation/devicetree/bindings/sound/atmel_ac97c.txt b/Documentation/devicetree/bindings/sound/atmel_ac97c.txt
> > > > > > new file mode 100644
> > > > > > index 0000000..9839403
> > > > > > --- /dev/null
> > > > > > +++ b/Documentation/devicetree/bindings/sound/atmel_ac97c.txt
> > > > > > @@ -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.
> > > 
> > > 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.

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.

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

[Alexander Stein]

On Tuesday 15 April 2014, 01:21:34 wrote Alexandre Belloni:
> On 13/04/2014 at 10:32:09 +0200, Alexander Stein wrote :
> > On Saturday 12 April 2014, 14:48:17 wrote Alexander Shiyan:
> > > Sat, 12 Apr 2014 12:42:18 +0200 ?? Alexander Stein <alexanders83@web.de>:
> > > > On Saturday 12 April 2014, 14:31:28 wrote Alexander Shiyan:
> > > > > Sat, 12 Apr 2014 11:08:26 +0200 ?? Alexander Stein <alexanders83@web.de>:
> > > > > > Signed-off-by: Alexander Stein <alexanders83@web.de>
> > > > > > ---
> > > > > >  .../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
> > > > > > 
> > > > > > diff --git a/Documentation/devicetree/bindings/sound/atmel_ac97c.txt b/Documentation/devicetree/bindings/sound/atmel_ac97c.txt
> > > > > > new file mode 100644
> > > > > > index 0000000..9839403
> > > > > > --- /dev/null
> > > > > > +++ b/Documentation/devicetree/bindings/sound/atmel_ac97c.txt
> > > > > > @@ -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.
> > > 
> > > 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.

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.

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

[Nicolas Ferre]

On 15/04/2014 19:38, Alexander Stein :
> This reduces some boilerplate code.
> 
> Signed-off-by: Alexander Stein <alexanders83@web.de>
> Acked-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>

Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com>

Thanks

> ---
>  sound/atmel/ac97c.c | 15 ++-------------
>  1 file changed, 2 insertions(+), 13 deletions(-)
> 
> diff --git a/sound/atmel/ac97c.c b/sound/atmel/ac97c.c
> index 05ec049..a04d2317 100644
> --- a/sound/atmel/ac97c.c
> +++ b/sound/atmel/ac97c.c
> @@ -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");
> 


-- 
Nicolas Ferre

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

[Nicolas Ferre]

On 15/04/2014 19:38, Alexander Stein :
> This platform data member is unused, so remove it.
> 
> Signed-off-by: Alexander Stein <alexanders83@web.de>
> Acked-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>

Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com>

> ---
>  include/sound/atmel-ac97c.h | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/include/sound/atmel-ac97c.h b/include/sound/atmel-ac97c.h
> index e6aabdb..00e6c289 100644
> --- a/include/sound/atmel-ac97c.h
> +++ b/include/sound/atmel-ac97c.h
> @@ -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;
>  };
>  
> 


-- 
Nicolas Ferre

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

[Takashi Iwai]

At Tue, 15 Apr 2014 19:38:56 +0200,
Alexander Stein wrote:
> 
> This reduces some boilerplate code.
> 
> Signed-off-by: Alexander Stein <alexanders83@web.de>
> Acked-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>

Thanks, applied these two cleanup patches now.


Takashi

> ---
>  sound/atmel/ac97c.c | 15 ++-------------
>  1 file changed, 2 insertions(+), 13 deletions(-)
> 
> diff --git a/sound/atmel/ac97c.c b/sound/atmel/ac97c.c
> index 05ec049..a04d2317 100644
> --- a/sound/atmel/ac97c.c
> +++ b/sound/atmel/ac97c.c
> @@ -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");
> -- 
> 1.9.2
>