Patchwork [v7,2/2] ASoC: fsl: Add S/PDIF machine driver

login
register
mail settings
Submitter Nicolin Chen
Date Aug. 19, 2013, 8:35 a.m.
Message ID <e7ceef74714bb4ead624b333ee3969225b4c5cb5.1376901081.git.b42378@freescale.com>
Download mbox | patch
Permalink /patch/268129/
State Not Applicable
Headers show

Comments

Nicolin Chen - Aug. 19, 2013, 8:35 a.m.
This patch implements a device-tree-only machine driver for Freescale
i.MX series Soc. It works with spdif_transmitter/spdif_receiver and
fsl_spdif.c drivers.

Signed-off-by: Nicolin Chen <b42378@freescale.com>
---
 .../devicetree/bindings/sound/imx-audio-spdif.txt  |   29 +++++
 sound/soc/fsl/Kconfig                              |   11 ++
 sound/soc/fsl/Makefile                             |    2 +
 sound/soc/fsl/imx-spdif.c                          |  134 ++++++++++++++++++++
 4 files changed, 176 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/sound/imx-audio-spdif.txt
 create mode 100644 sound/soc/fsl/imx-spdif.c
Mark Rutland - Aug. 19, 2013, 9:24 a.m.
On Mon, Aug 19, 2013 at 09:35:22AM +0100, Nicolin Chen wrote:
> This patch implements a device-tree-only machine driver for Freescale
> i.MX series Soc. It works with spdif_transmitter/spdif_receiver and
> fsl_spdif.c drivers.
> 
> Signed-off-by: Nicolin Chen <b42378@freescale.com>
> ---
>  .../devicetree/bindings/sound/imx-audio-spdif.txt  |   29 +++++
>  sound/soc/fsl/Kconfig                              |   11 ++
>  sound/soc/fsl/Makefile                             |    2 +
>  sound/soc/fsl/imx-spdif.c                          |  134 ++++++++++++++++++++
>  4 files changed, 176 insertions(+), 0 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/sound/imx-audio-spdif.txt
>  create mode 100644 sound/soc/fsl/imx-spdif.c
> 
> diff --git a/Documentation/devicetree/bindings/sound/imx-audio-spdif.txt b/Documentation/devicetree/bindings/sound/imx-audio-spdif.txt
> new file mode 100644
> index 0000000..9a3fa26
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/sound/imx-audio-spdif.txt
> @@ -0,0 +1,29 @@
> +Freescale i.MX audio complex with S/PDIF transceiver
> +
> +Required properties:
> +
> +  - compatible : "fsl,imx-audio-spdif"
> +
> +  - model : The user-visible name of this sound complex

Is this used semantically, or is it a completely arbitrary string?  In
either case I don't see why the compatible string doesn't give the
driver enough to have a sensible value.

I'm confused as to why we need this. The phrase "user-visible" in a
device description seems very odd.

> +
> +  - spdif-controller : The phandle of the i.MX S/PDIF controller
> +
> +
> +Optional properties:
> +
> +  - spdif-transmitter : The phandle of the spdif-transmitter dummy codec
> +
> +  - spdif-receiver : The phandle of the spdif-receiver dummy codec
> +
> +* Note: At least one of these two properties should be set in the DT binding.

Are all four units (comlpex,controller,transmitter,receiver) really
separate blocks?

Thanks,
Mark.
Nicolin Chen - Aug. 19, 2013, 9:50 a.m.
Hi,

On Mon, Aug 19, 2013 at 10:24:58AM +0100, Mark Rutland wrote:
> Is this used semantically, or is it a completely arbitrary string?  In
> either case I don't see why the compatible string doesn't give the
> driver enough to have a sensible value.
> 
> I'm confused as to why we need this. The phrase "user-visible" in a
> device description seems very odd.

The string would be in the ALSA device list:
ALSA device list:
  #0: imx-spdif

I think it can be a sort of arbitrary as long as users know which this 
device exactly is when they catch the name by 'aplay -l' or 'arecord -l'

The phrase "user-visible" is being used in many current docs, I don't 
dare to change it unless a sage gives me a suggestion.

> > +
> > +  - spdif-controller : The phandle of the i.MX S/PDIF controller
> > +
> > +
> > +Optional properties:
> > +
> > +  - spdif-transmitter : The phandle of the spdif-transmitter dummy codec
> > +
> > +  - spdif-receiver : The phandle of the spdif-receiver dummy codec
> > +
> > +* Note: At least one of these two properties should be set in the DT binding.
> 
> Are all four units (comlpex,controller,transmitter,receiver) really
> separate blocks?

At least they are separate drivers as I mentioned in the commit comments.

Thank you,
Nicolin
Mark Rutland - Aug. 19, 2013, 10:01 a.m.
On Mon, Aug 19, 2013 at 10:50:43AM +0100, Nicolin Chen wrote:
> Hi,
> 
> On Mon, Aug 19, 2013 at 10:24:58AM +0100, Mark Rutland wrote:
> > Is this used semantically, or is it a completely arbitrary string?  In
> > either case I don't see why the compatible string doesn't give the
> > driver enough to have a sensible value.
> > 
> > I'm confused as to why we need this. The phrase "user-visible" in a
> > device description seems very odd.
> 
> The string would be in the ALSA device list:
> ALSA device list:
>   #0: imx-spdif
> 
> I think it can be a sort of arbitrary as long as users know which this 
> device exactly is when they catch the name by 'aplay -l' or 'arecord -l'
> 
> The phrase "user-visible" is being used in many current docs, I don't 
> dare to change it unless a sage gives me a suggestion.

I can see that there is entrenched usage, but this really seems to be
embedding Linux-specific implementation details into the dt. I don't see
why the driver cannot select a sensible name, but perhaps I'm missing
something.

Mark, is there any reason we need to handle the user-visible name of the
device this way?

> 
> > > +
> > > +  - spdif-controller : The phandle of the i.MX S/PDIF controller
> > > +
> > > +
> > > +Optional properties:
> > > +
> > > +  - spdif-transmitter : The phandle of the spdif-transmitter dummy codec
> > > +
> > > +  - spdif-receiver : The phandle of the spdif-receiver dummy codec
> > > +
> > > +* Note: At least one of these two properties should be set in the DT binding.
> > 
> > Are all four units (comlpex,controller,transmitter,receiver) really
> > separate blocks?
> 
> At least they are separate drivers as I mentioned in the commit comments.

I'm not sure that the boundary of Linux drivers should necessarily
determine the way we carve up the description of IP blocks, though
presumably it's a pretty sensible way of carving it up or we wouldn't
have done it.

Is there any public documentation on the i.MX S/PDIF hardware block(s)?

Thanks,
Mark.
Nicolin Chen - Aug. 19, 2013, 10:21 a.m.
On Mon, Aug 19, 2013 at 11:01:43AM +0100, Mark Rutland wrote:
> > At least they are separate drivers as I mentioned in the commit comments.
> 
> I'm not sure that the boundary of Linux drivers should necessarily
> determine the way we carve up the description of IP blocks, though
> presumably it's a pretty sensible way of carving it up or we wouldn't
> have done it.

I'm not sure if I understand your point correctly. But in fact the IP driver
is not being carved up. The spdif-transmitter and spdif-receiver are basically
dummy codec drivers. All the IP-related codes are implemented in fsl_spdif.c,
the PATCH 1/2 you just reviewed.

> Is there any public documentation on the i.MX S/PDIF hardware block(s)?

http://cache.freescale.com/files/32bit/doc/ref_manual/IMX6DQRM.pdf

Thank you,
Nicolin
Philipp Zabel - Aug. 19, 2013, 10:27 a.m.
Am Montag, den 19.08.2013, 11:01 +0100 schrieb Mark Rutland:
> On Mon, Aug 19, 2013 at 10:50:43AM +0100, Nicolin Chen wrote:
> > Hi,
> > 
> > On Mon, Aug 19, 2013 at 10:24:58AM +0100, Mark Rutland wrote:
> > > Is this used semantically, or is it a completely arbitrary string?  In
> > > either case I don't see why the compatible string doesn't give the
> > > driver enough to have a sensible value.
> > > 
> > > I'm confused as to why we need this. The phrase "user-visible" in a
> > > device description seems very odd.
> > 
> > The string would be in the ALSA device list:
> > ALSA device list:
> >   #0: imx-spdif
> > 
> > I think it can be a sort of arbitrary as long as users know which this 
> > device exactly is when they catch the name by 'aplay -l' or 'arecord -l'
> > 
> > The phrase "user-visible" is being used in many current docs, I don't 
> > dare to change it unless a sage gives me a suggestion.
> 
> I can see that there is entrenched usage, but this really seems to be
> embedding Linux-specific implementation details into the dt. I don't see
> why the driver cannot select a sensible name, but perhaps I'm missing
> something.
> 
> Mark, is there any reason we need to handle the user-visible name of the
> device this way?
> 
> > 
> > > > +
> > > > +  - spdif-controller : The phandle of the i.MX S/PDIF controller
> > > > +
> > > > +
> > > > +Optional properties:
> > > > +
> > > > +  - spdif-transmitter : The phandle of the spdif-transmitter dummy codec
> > > > +
> > > > +  - spdif-receiver : The phandle of the spdif-receiver dummy codec
> > > > +
> > > > +* Note: At least one of these two properties should be set in the DT binding.
> > > 
> > > Are all four units (comlpex,controller,transmitter,receiver) really
> > > separate blocks?
> > 
> > At least they are separate drivers as I mentioned in the commit comments.
> 
> I'm not sure that the boundary of Linux drivers should necessarily
> determine the way we carve up the description of IP blocks, though
> presumably it's a pretty sensible way of carving it up or we wouldn't
> have done it.

The transmitter and receiver can be real external codec devices with
S/PDIF input or output pads connected to the i.MX S/PDIF. One example
would be the Analog Devices ADV7612 HDMI receiver, which can output
audio taken from HDMI input via S/PDIF (just as via I2S).

The dummy codec devices are just needed if the S/PDIF pads are directly
routed to externally accessible connectors.

regards
Philipp
Mark Brown - Aug. 19, 2013, 10:52 a.m.
On Mon, Aug 19, 2013 at 11:01:43AM +0100, Mark Rutland wrote:
> On Mon, Aug 19, 2013 at 10:50:43AM +0100, Nicolin Chen wrote:

> > The phrase "user-visible" is being used in many current docs, I don't 
> > dare to change it unless a sage gives me a suggestion.

> I can see that there is entrenched usage, but this really seems to be
> embedding Linux-specific implementation details into the dt. I don't see
> why the driver cannot select a sensible name, but perhaps I'm missing
> something.

> Mark, is there any reason we need to handle the user-visible name of the
> device this way?

This is intended to allow userspace to distinguish between systems that
are electrically identical but physically distinct, for example when
multiple systems are derived from the same reference design.
Mark Rutland - Aug. 19, 2013, 11:15 a.m.
On Mon, Aug 19, 2013 at 11:21:06AM +0100, Nicolin Chen wrote:
> On Mon, Aug 19, 2013 at 11:01:43AM +0100, Mark Rutland wrote:
> > > At least they are separate drivers as I mentioned in the commit comments.
> > 
> > I'm not sure that the boundary of Linux drivers should necessarily
> > determine the way we carve up the description of IP blocks, though
> > presumably it's a pretty sensible way of carving it up or we wouldn't
> > have done it.
> 
> I'm not sure if I understand your point correctly. But in fact the IP driver
> is not being carved up. The spdif-transmitter and spdif-receiver are basically
> dummy codec drivers. All the IP-related codes are implemented in fsl_spdif.c,
> the PATCH 1/2 you just reviewed.

I see. Sorry for the noise there, that's an artifact of my not full
understanding of the ASoC bindings.

> 
> > Is there any public documentation on the i.MX S/PDIF hardware block(s)?
> 
> http://cache.freescale.com/files/32bit/doc/ref_manual/IMX6DQRM.pdf
> 

Cheers!

Mark.
Mark Rutland - Aug. 19, 2013, 11:31 a.m.
On Mon, Aug 19, 2013 at 11:52:01AM +0100, Mark Brown wrote:
> On Mon, Aug 19, 2013 at 11:01:43AM +0100, Mark Rutland wrote:
> > On Mon, Aug 19, 2013 at 10:50:43AM +0100, Nicolin Chen wrote:
> 
> > > The phrase "user-visible" is being used in many current docs, I don't 
> > > dare to change it unless a sage gives me a suggestion.
> 
> > I can see that there is entrenched usage, but this really seems to be
> > embedding Linux-specific implementation details into the dt. I don't see
> > why the driver cannot select a sensible name, but perhaps I'm missing
> > something.
> 
> > Mark, is there any reason we need to handle the user-visible name of the
> > device this way?
> 
> This is intended to allow userspace to distinguish between systems that
> are electrically identical but physically distinct, for example when
> multiple systems are derived from the same reference design.

I see. Surely if there's some meaning imparted to userspace by the model
name, there's a contract there that we should document (the set of valid
model names and what they correspond to)?

I'm not sure I understand why userspace needs to know what the system is
if we've adequately described how it's wired and what its capabilities
are. However, I'm not at all familiar with the way we handle audio, so
please forgive my naivety there :)

Thanks,
Mark.
Mark Brown - Aug. 19, 2013, 11:45 a.m.
On Mon, Aug 19, 2013 at 12:31:21PM +0100, Mark Rutland wrote:
> On Mon, Aug 19, 2013 at 11:52:01AM +0100, Mark Brown wrote:

> > This is intended to allow userspace to distinguish between systems that
> > are electrically identical but physically distinct, for example when
> > multiple systems are derived from the same reference design.

> I see. Surely if there's some meaning imparted to userspace by the model
> name, there's a contract there that we should document (the set of valid
> model names and what they correspond to)?

In theory I guess.  In practice I doubt many people will bother and I'd
expect zero productive result from those that do.

> I'm not sure I understand why userspace needs to know what the system is
> if we've adequately described how it's wired and what its capabilities
> are. However, I'm not at all familiar with the way we handle audio, so
> please forgive my naivety there :)

The physical form of the system can have a substantial impact on how
people want to configure it at runtime even if there is no software
visible difference.  Coefficients can be chosen to work with plastics,
or volumes adjusted for expected user positioning with respect to the
hardware for example.

Patch

diff --git a/Documentation/devicetree/bindings/sound/imx-audio-spdif.txt b/Documentation/devicetree/bindings/sound/imx-audio-spdif.txt
new file mode 100644
index 0000000..9a3fa26
--- /dev/null
+++ b/Documentation/devicetree/bindings/sound/imx-audio-spdif.txt
@@ -0,0 +1,29 @@ 
+Freescale i.MX audio complex with S/PDIF transceiver
+
+Required properties:
+
+  - compatible : "fsl,imx-audio-spdif"
+
+  - model : The user-visible name of this sound complex
+
+  - spdif-controller : The phandle of the i.MX S/PDIF controller
+
+
+Optional properties:
+
+  - spdif-transmitter : The phandle of the spdif-transmitter dummy codec
+
+  - spdif-receiver : The phandle of the spdif-receiver dummy codec
+
+* Note: At least one of these two properties should be set in the DT binding.
+
+
+Example:
+
+sound-spdif {
+	compatible = "fsl,imx-audio-spdif";
+	model = "imx-spdif";
+	spdif-controller = <&spdif>;
+	spdif-transmitter = <&spdif_tx_codec>;
+	spdif-receiver = <&spdif_rx_codec>;
+};
diff --git a/sound/soc/fsl/Kconfig b/sound/soc/fsl/Kconfig
index cd088cc..a708380 100644
--- a/sound/soc/fsl/Kconfig
+++ b/sound/soc/fsl/Kconfig
@@ -193,6 +193,17 @@  config SND_SOC_IMX_SGTL5000
 	  Say Y if you want to add support for SoC audio on an i.MX board with
 	  a sgtl5000 codec.
 
+config SND_SOC_IMX_SPDIF
+	tristate "SoC Audio support for i.MX boards with S/PDIF"
+	select SND_SOC_IMX_PCM_DMA
+	select SND_SOC_FSL_SPDIF
+	select SND_SOC_FSL_UTILS
+	select SND_SOC_SPDIF
+	help
+	  SoC Audio support for i.MX boards with S/PDIF
+	  Say Y if you want to add support for SoC audio on an i.MX board with
+	  a S/DPDIF.
+
 config SND_SOC_IMX_MC13783
 	tristate "SoC Audio support for I.MX boards with mc13783"
 	depends on MFD_MC13783 && ARM
diff --git a/sound/soc/fsl/Makefile b/sound/soc/fsl/Makefile
index 4b5970e..e2aaff7 100644
--- a/sound/soc/fsl/Makefile
+++ b/sound/soc/fsl/Makefile
@@ -45,6 +45,7 @@  snd-soc-mx27vis-aic32x4-objs := mx27vis-aic32x4.o
 snd-soc-wm1133-ev1-objs := wm1133-ev1.o
 snd-soc-imx-sgtl5000-objs := imx-sgtl5000.o
 snd-soc-imx-wm8962-objs := imx-wm8962.o
+snd-soc-imx-spdif-objs :=imx-spdif.o
 snd-soc-imx-mc13783-objs := imx-mc13783.o
 
 obj-$(CONFIG_SND_SOC_EUKREA_TLV320) += snd-soc-eukrea-tlv320.o
@@ -53,4 +54,5 @@  obj-$(CONFIG_SND_SOC_MX27VIS_AIC32X4) += snd-soc-mx27vis-aic32x4.o
 obj-$(CONFIG_SND_MXC_SOC_WM1133_EV1) += snd-soc-wm1133-ev1.o
 obj-$(CONFIG_SND_SOC_IMX_SGTL5000) += snd-soc-imx-sgtl5000.o
 obj-$(CONFIG_SND_SOC_IMX_WM8962) += snd-soc-imx-wm8962.o
+obj-$(CONFIG_SND_SOC_IMX_SPDIF) += snd-soc-imx-spdif.o
 obj-$(CONFIG_SND_SOC_IMX_MC13783) += snd-soc-imx-mc13783.o
diff --git a/sound/soc/fsl/imx-spdif.c b/sound/soc/fsl/imx-spdif.c
new file mode 100644
index 0000000..893f3d1
--- /dev/null
+++ b/sound/soc/fsl/imx-spdif.c
@@ -0,0 +1,134 @@ 
+/*
+ * Copyright (C) 2013 Freescale Semiconductor, Inc.
+ *
+ * The code contained herein is licensed under the GNU General Public
+ * License. You may obtain a copy of the GNU General Public License
+ * Version 2 or later at the following locations:
+ *
+ * http://www.opensource.org/licenses/gpl-license.html
+ * http://www.gnu.org/copyleft/gpl.html
+ */
+
+#include <linux/module.h>
+#include <linux/of_platform.h>
+#include <sound/soc.h>
+
+struct imx_spdif_data {
+	struct snd_soc_dai_link dai[2];
+	struct snd_soc_card card;
+};
+
+static int imx_spdif_audio_probe(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	struct device_node *spdif_np, *codec_tx_np, *codec_rx_np;
+	struct platform_device *spdif_pdev;
+	struct imx_spdif_data *data;
+	int ret = 0, num_links = 0;
+
+	spdif_np = of_parse_phandle(np, "spdif-controller", 0);
+	if (!spdif_np) {
+		dev_err(&pdev->dev, "failed to find spdif-controller\n");
+		ret = -EINVAL;
+		goto fail;
+	}
+
+	spdif_pdev = of_find_device_by_node(spdif_np);
+	if (!spdif_pdev) {
+		dev_err(&pdev->dev, "failed to find S/PDIF device\n");
+		ret = -EINVAL;
+		goto fail;
+	}
+
+	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
+	if (!data) {
+		dev_err(&pdev->dev, "failed to allocate memory\n");
+		ret = -ENOMEM;
+		goto fail;
+	}
+
+	codec_tx_np = of_parse_phandle(np, "spdif-transmitter", 0);
+	if (codec_tx_np) {
+		data->dai[num_links].name = "S/PDIF TX";
+		data->dai[num_links].stream_name = "S/PDIF PCM Playback";
+		data->dai[num_links].codec_dai_name = "dit-hifi";
+		data->dai[num_links].codec_of_node = codec_tx_np;
+		data->dai[num_links].cpu_of_node = spdif_np;
+		data->dai[num_links].platform_of_node = spdif_np;
+		num_links++;
+	}
+
+	codec_rx_np = of_parse_phandle(np, "spdif-receiver", 0);
+	if (codec_rx_np) {
+		data->dai[num_links].name = "S/PDIF RX";
+		data->dai[num_links].stream_name = "S/PDIF PCM Capture";
+		data->dai[num_links].codec_dai_name = "dir-hifi";
+		data->dai[num_links].codec_of_node = codec_rx_np;
+		data->dai[num_links].cpu_of_node = spdif_np;
+		data->dai[num_links].platform_of_node = spdif_np;
+		num_links++;
+	}
+
+	if (!num_links) {
+		dev_err(&pdev->dev, "no enabled S/PDIF DAI link\n");
+		goto fail;
+	}
+
+	data->card.dev = &pdev->dev;
+	data->card.num_links = num_links;
+	data->card.dai_link = data->dai;
+
+	ret = snd_soc_of_parse_card_name(&data->card, "model");
+	if (ret)
+		goto fail;
+
+	ret = snd_soc_register_card(&data->card);
+	if (ret) {
+		dev_err(&pdev->dev, "snd_soc_register_card failed (%d)\n", ret);
+		goto fail;
+	}
+
+	platform_set_drvdata(pdev, data);
+
+fail:
+	if (codec_tx_np)
+		of_node_put(codec_tx_np);
+	if (codec_rx_np)
+		of_node_put(codec_rx_np);
+	if (spdif_np)
+		of_node_put(spdif_np);
+
+	return ret;
+}
+
+static int imx_spdif_audio_remove(struct platform_device *pdev)
+{
+	struct imx_spdif_data *data = platform_get_drvdata(pdev);
+
+	snd_soc_unregister_card(&data->card);
+
+	return 0;
+}
+
+static const struct of_device_id imx_spdif_dt_ids[] = {
+	{ .compatible = "fsl,imx-audio-spdif", },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, imx_spdif_dt_ids);
+
+static struct platform_driver imx_spdif_driver = {
+	.driver = {
+		.name = "imx-spdif",
+		.owner = THIS_MODULE,
+		.of_match_table = imx_spdif_dt_ids,
+	},
+	.probe = imx_spdif_audio_probe,
+	.remove = imx_spdif_audio_remove,
+};
+
+module_platform_driver(imx_spdif_driver);
+
+MODULE_AUTHOR("Freescale Semiconductor, Inc.");
+MODULE_DESCRIPTION("Freescale i.MX S/PDIF machine driver");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:imx-spdif");