diff mbox

[v5,01/10] ASoC: phycore-ac97: Add DT support

Message ID 1366814199-14827-2-git-send-email-mpa@pengutronix.de (mailing list archive)
State New, archived
Headers show

Commit Message

Markus Pargmann April 24, 2013, 2:36 p.m. UTC
Add devicetree support for this audio soc fabric driver.

DT initialized drivers do not have real device ids, so
imx-pcm-fiq-audio.0 is used without '.0'.

This patch also simplifies the way audmux ports are configured. Before
there was knowledge about the used board. As the audmux driver knows the
used board, we know use the returned errorcodes to correctly set the
audmux port. So this driver does not have to know which board it is
running on.

Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
---

Notes:
    Changes in v4:
     - New property phytec,audmux to check which audmux setup should be
       executed. Checking for fsl,imx21-audmux and fsl,imx31-audmux.
    
    Changes in v3:
     - Add some more information in the commit message.
    
    Changes in v2:
     - Simplify the driver, by combining audmux port configurations. The
       audmux driver actually knows on which platform he is running and
       will return the appropriate error code if we use functions for
       another platform. So we don't need to have the knowledge about it
       in phycore-ac97 and can try both functions. This removes the need
       of different compatibilities and renames it to phycore-ac97.
     - Use a phandle for the cpu_dai link.
     - Add devicetree binding documentation.
     - Rename binding to phytec,phycore-ac97 and fsl,ssi to phytec,ssi
    
    Changes in v2:
     - Simplify the driver, by combining audmux port configurations. The
       audmux driver actually knows on which platform he is running and
       will return the appropriate error code if we use functions for
       another platform. So we don't need to have the knowledge about it
       in phycore-ac97 and can try both functions. This removes the need
       of different compatibilities and renames it to imx27-ac97.
     - Use a phandle for the cpu_dai link.
     - Add devicetree binding documentation.
     - Rename binding to phytec,phycore-ac97 and fsl,ssi to phytec,ssi

 .../bindings/sound/phytec,phycore-ac97.txt         |  14 ++
 sound/soc/fsl/phycore-ac97.c                       | 166 ++++++++++++++++++---
 2 files changed, 158 insertions(+), 22 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/sound/phytec,phycore-ac97.txt

Comments

Sascha Hauer April 24, 2013, 7:45 p.m. UTC | #1
Hi Markus,

On Wed, Apr 24, 2013 at 04:36:30PM +0200, Markus Pargmann wrote:
> Add devicetree support for this audio soc fabric driver.
> 
> @@ -32,8 +35,12 @@ static struct snd_soc_dai_link imx_phycore_dai_ac97[] = {
>  		.stream_name	= "HiFi",
>  		.codec_dai_name		= "wm9712-hifi",
>  		.codec_name	= "wm9712-codec",
> +#ifdef CONFIG_MACH_IMX27_DT
> +		.platform_name	= "imx-fiq-pcm-audio",
> +#else
>  		.cpu_dai_name	= "imx-ssi.0",
>  		.platform_name	= "imx-fiq-pcm-audio.0",
> +#endif

This doesn't work properly. Compiling the kernel with IMX27_DT enabled
does not necessarily mean that it also started with dt support.

Personally I am fine with removing platform based support for this
driver if it's too much effort to fix it properly, but we shouldn't
start working with ifdefs here.

Sascha
Shawn Guo April 25, 2013, 3:35 a.m. UTC | #2
On Wed, Apr 24, 2013 at 09:45:40PM +0200, Sascha Hauer wrote:
> Hi Markus,
> 
> On Wed, Apr 24, 2013 at 04:36:30PM +0200, Markus Pargmann wrote:
> > Add devicetree support for this audio soc fabric driver.
> > 
> > @@ -32,8 +35,12 @@ static struct snd_soc_dai_link imx_phycore_dai_ac97[] = {
> >  		.stream_name	= "HiFi",
> >  		.codec_dai_name		= "wm9712-hifi",
> >  		.codec_name	= "wm9712-codec",
> > +#ifdef CONFIG_MACH_IMX27_DT
> > +		.platform_name	= "imx-fiq-pcm-audio",
> > +#else
> >  		.cpu_dai_name	= "imx-ssi.0",
> >  		.platform_name	= "imx-fiq-pcm-audio.0",
> > +#endif
> 
> This doesn't work properly. Compiling the kernel with IMX27_DT enabled
> does not necessarily mean that it also started with dt support.
> 
Right, we should run-time check pdev->dev.of_node to see if it's a DT
boot and set .platform_of_node rather than .platform_name if it is.  But
it's only possible with the cleanup series "ASoC: fsl: imx-pcm driver
cleanup" I just sent being the base.  I hope you can rebase your series
on that :)

Shawn

> Personally I am fine with removing platform based support for this
> driver if it's too much effort to fix it properly, but we shouldn't
> start working with ifdefs here.
> 
> Sascha
> 
> 
> -- 
> Pengutronix e.K.                           |                             |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
Markus Pargmann April 25, 2013, 11:02 a.m. UTC | #3
On Thu, Apr 25, 2013 at 11:35:20AM +0800, Shawn Guo wrote:
> On Wed, Apr 24, 2013 at 09:45:40PM +0200, Sascha Hauer wrote:
> > Hi Markus,
> > 
> > On Wed, Apr 24, 2013 at 04:36:30PM +0200, Markus Pargmann wrote:
> > > Add devicetree support for this audio soc fabric driver.
> > > 
> > > @@ -32,8 +35,12 @@ static struct snd_soc_dai_link imx_phycore_dai_ac97[] = {
> > >  		.stream_name	= "HiFi",
> > >  		.codec_dai_name		= "wm9712-hifi",
> > >  		.codec_name	= "wm9712-codec",
> > > +#ifdef CONFIG_MACH_IMX27_DT
> > > +		.platform_name	= "imx-fiq-pcm-audio",
> > > +#else
> > >  		.cpu_dai_name	= "imx-ssi.0",
> > >  		.platform_name	= "imx-fiq-pcm-audio.0",
> > > +#endif
> > 
> > This doesn't work properly. Compiling the kernel with IMX27_DT enabled
> > does not necessarily mean that it also started with dt support.
> > 
> Right, we should run-time check pdev->dev.of_node to see if it's a DT
> boot and set .platform_of_node rather than .platform_name if it is.  But
> it's only possible with the cleanup series "ASoC: fsl: imx-pcm driver
> cleanup" I just sent being the base.  I hope you can rebase your series
> on that :)

The current version of phycore-ac97.c is not a driver, it is just a late
init call. So run-time check is not possible unless I modify the old
part to be a driver and add a device to all board init functions using
ac97. (I would prefer dropping the non-DT part in this case ;) )

Yes I rebased this series onto your cleanups, looks good so far and
platform_of_node works finally.

Thanks,

Markus

> 
> Shawn
> 
> > Personally I am fine with removing platform based support for this
> > driver if it's too much effort to fix it properly, but we shouldn't
> > start working with ifdefs here.
> > 
> > Sascha
> > 
> > 
> > -- 
> > Pengutronix e.K.                           |                             |
> > Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> > Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> > Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
> 
>
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/sound/phytec,phycore-ac97.txt b/Documentation/devicetree/bindings/sound/phytec,phycore-ac97.txt
new file mode 100644
index 0000000..41201ff
--- /dev/null
+++ b/Documentation/devicetree/bindings/sound/phytec,phycore-ac97.txt
@@ -0,0 +1,14 @@ 
+Phytec phycore AC97
+
+Required properties:
+- compatible: "phytec,phycore-ac97"
+- phytec,ssi: A phandle to the ssi device that is connected to ac97.
+- phytec,audmux: A phandle to the audmux device.
+
+Example:
+
+sound {
+	compatible = "phytec,phycore-ac97";
+	phytec,ssi = <&ssi1>;
+	phytec,audmux = <&audmux>;
+};
diff --git a/sound/soc/fsl/phycore-ac97.c b/sound/soc/fsl/phycore-ac97.c
index f8da6dd..d9f3ac7 100644
--- a/sound/soc/fsl/phycore-ac97.c
+++ b/sound/soc/fsl/phycore-ac97.c
@@ -21,7 +21,10 @@ 
 
 #include "imx-audmux.h"
 
+#define DRV_NAME "phycore-audio-fabric"
+
 static struct snd_soc_card imx_phycore;
+static struct device_node *phycore_dai_cpu_node;
 
 static struct snd_soc_ops imx_phycore_hifi_ops = {
 };
@@ -32,8 +35,12 @@  static struct snd_soc_dai_link imx_phycore_dai_ac97[] = {
 		.stream_name	= "HiFi",
 		.codec_dai_name		= "wm9712-hifi",
 		.codec_name	= "wm9712-codec",
+#ifdef CONFIG_MACH_IMX27_DT
+		.platform_name	= "imx-fiq-pcm-audio",
+#else
 		.cpu_dai_name	= "imx-ssi.0",
 		.platform_name	= "imx-fiq-pcm-audio.0",
+#endif
 		.ops		= &imx_phycore_hifi_ops,
 	},
 };
@@ -45,35 +52,48 @@  static struct snd_soc_card imx_phycore = {
 	.num_links	= ARRAY_SIZE(imx_phycore_dai_ac97),
 };
 
-static struct platform_device *imx_phycore_snd_ac97_device;
+static void phycore_ac97_imx21_audmux(void)
+{
+	imx_audmux_v1_configure_port(MX27_AUDMUX_HPCR1_SSI0,
+		IMX_AUDMUX_V1_PCR_SYN | /* 4wire mode */
+		IMX_AUDMUX_V1_PCR_TFCSEL(3) |
+		IMX_AUDMUX_V1_PCR_TCLKDIR | /* clock is output */
+		IMX_AUDMUX_V1_PCR_RXDSEL(3));
+	imx_audmux_v1_configure_port(3,
+		IMX_AUDMUX_V1_PCR_SYN | /* 4wire mode */
+		IMX_AUDMUX_V1_PCR_TFCSEL(0) |
+		IMX_AUDMUX_V1_PCR_TFSDIR |
+		IMX_AUDMUX_V1_PCR_RXDSEL(0));
+}
+
+static void phycore_ac97_imx31_audmux(void)
+{
+	imx_audmux_v2_configure_port(3,
+		IMX_AUDMUX_V2_PTCR_SYN | /* 4wire mode */
+		IMX_AUDMUX_V2_PTCR_TFSEL(0) |
+		IMX_AUDMUX_V2_PTCR_TFSDIR,
+		IMX_AUDMUX_V2_PDCR_RXDSEL(0));
+	imx_audmux_v2_configure_port(0,
+		IMX_AUDMUX_V2_PTCR_SYN | /* 4wire mode */
+		IMX_AUDMUX_V2_PTCR_TCSEL(3) |
+		IMX_AUDMUX_V2_PTCR_TCLKDIR, /* clock is output */
+		IMX_AUDMUX_V2_PDCR_RXDSEL(3));
+}
+
 static struct platform_device *imx_phycore_snd_device;
 
+#ifndef CONFIG_MACH_IMX27_DT
+
+static struct platform_device *imx_phycore_snd_ac97_device;
+
 static int __init imx_phycore_init(void)
 {
 	int ret;
 
 	if (machine_is_pca100()) {
-		imx_audmux_v1_configure_port(MX27_AUDMUX_HPCR1_SSI0,
-			IMX_AUDMUX_V1_PCR_SYN | /* 4wire mode */
-			IMX_AUDMUX_V1_PCR_TFCSEL(3) |
-			IMX_AUDMUX_V1_PCR_TCLKDIR | /* clock is output */
-			IMX_AUDMUX_V1_PCR_RXDSEL(3));
-		imx_audmux_v1_configure_port(3,
-			IMX_AUDMUX_V1_PCR_SYN | /* 4wire mode */
-			IMX_AUDMUX_V1_PCR_TFCSEL(0) |
-			IMX_AUDMUX_V1_PCR_TFSDIR |
-			IMX_AUDMUX_V1_PCR_RXDSEL(0));
+		phycore_ac97_imx21_audmux();
 	} else if (machine_is_pcm043()) {
-		imx_audmux_v2_configure_port(3,
-			IMX_AUDMUX_V2_PTCR_SYN | /* 4wire mode */
-			IMX_AUDMUX_V2_PTCR_TFSEL(0) |
-			IMX_AUDMUX_V2_PTCR_TFSDIR,
-			IMX_AUDMUX_V2_PDCR_RXDSEL(0));
-		imx_audmux_v2_configure_port(0,
-			IMX_AUDMUX_V2_PTCR_SYN | /* 4wire mode */
-			IMX_AUDMUX_V2_PTCR_TCSEL(3) |
-			IMX_AUDMUX_V2_PTCR_TCLKDIR, /* clock is output */
-			IMX_AUDMUX_V2_PDCR_RXDSEL(3));
+		phycore_ac97_imx31_audmux();
 	} else {
 		/* return happy. We might run on a totally different machine */
 		return 0;
@@ -120,6 +140,108 @@  static void __exit imx_phycore_exit(void)
 late_initcall(imx_phycore_init);
 module_exit(imx_phycore_exit);
 
+#else  /* !CONFIG_MACH_IMX27_DT */
+
+static const struct of_device_id imx_phycore_ac97_of_dev_id[] = {
+	{
+		.compatible = "phytec,phycore-ac97",
+	}, {
+		/* sentinel */
+	}
+};
+MODULE_DEVICE_TABLE(of, imx_phycore_ac97_of_dev_id);
+
+static int phycore_ac97_setup_devices(struct platform_device *pdev)
+{
+	int ret;
+
+	imx_phycore_snd_device = platform_device_alloc("wm9712-codec", -1);
+	if (!imx_phycore_snd_device)
+		return -ENOMEM;
+
+	ret = platform_device_add(imx_phycore_snd_device);
+	if (ret) {
+		dev_err(&pdev->dev, "ASoC: Platform device allocation failed\n");
+		goto fail1;
+	}
+
+	ret = snd_soc_register_card(&imx_phycore);
+	if (ret) {
+		dev_err(&pdev->dev, "ASoC: soc card registration failed\n");
+		goto fail2;
+	}
+	return 0;
+
+fail2:
+	platform_device_del(imx_phycore_snd_device);
+fail1:
+	platform_device_put(imx_phycore_snd_device);
+	return ret;
+}
+
+static int imx_phycore_ac97_probe(struct platform_device *pdev)
+{
+	int ret;
+	struct device_node *ssi_np;
+	struct device_node *audmux_np;
+
+	imx_phycore.dev = &pdev->dev;
+
+	audmux_np = of_parse_phandle(pdev->dev.of_node, "phytec,audmux", 0);
+	if (!audmux_np) {
+		dev_err(&pdev->dev, "Failed to parse phytec,audmux phandle\n");
+		return -EINVAL;
+	}
+
+	if (of_device_is_compatible(audmux_np, "fsl,imx21-audmux")) {
+		phycore_ac97_imx21_audmux();
+	} else if (of_device_is_compatible(audmux_np, "fsl,imx31-audmux")) {
+		phycore_ac97_imx31_audmux();
+	} else {
+		dev_err(&pdev->dev, "Unknown audmux, failed to setup audmux\n");
+		of_node_put(audmux_np);
+		return -EINVAL;
+	}
+	of_node_put(audmux_np);
+
+	ssi_np = of_parse_phandle(pdev->dev.of_node, "phytec,ssi", 0);
+	if (!ssi_np) {
+		dev_err(&pdev->dev, "No valid ssi phandle found\n");
+		return -EINVAL;
+	}
+
+	imx_phycore_dai_ac97[0].cpu_of_node = ssi_np;
+	phycore_dai_cpu_node = ssi_np;
+
+
+	ret = phycore_ac97_setup_devices(pdev);
+	if (ret)
+		of_node_put(phycore_dai_cpu_node);
+
+	return ret;
+}
+
+static int imx_phycore_ac97_remove(struct platform_device *pdev)
+{
+	of_node_put(phycore_dai_cpu_node);
+	platform_device_unregister(imx_phycore_snd_device);
+	return 0;
+}
+
+static struct platform_driver imx_phycore_ac97_driver = {
+	.probe		= imx_phycore_ac97_probe,
+	.remove		= imx_phycore_ac97_remove,
+	.driver		= {
+		.name	= DRV_NAME,
+		.owner	= THIS_MODULE,
+		.of_match_table = imx_phycore_ac97_of_dev_id,
+	},
+};
+
+module_platform_driver(imx_phycore_ac97_driver);
+
+#endif  /* CONFIG_MACH_IMX27_DT */
+
 MODULE_AUTHOR("Sascha Hauer <s.hauer@pengutronix.de>");
-MODULE_DESCRIPTION("PhyCORE ALSA SoC driver");
+MODULE_DESCRIPTION(DRV_NAME ": PhyCORE ALSA SoC fabric driver");
 MODULE_LICENSE("GPL");