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

login
register
mail settings
Submitter Nicolin Chen
Date Aug. 21, 2013, 3:13 a.m.
Message ID <e9cf5e37f858dea78319720f007f66c8c063885f.1377054540.git.b42378@freescale.com>
Download mbox | patch
Permalink /patch/268678/
State Not Applicable
Headers show

Comments

Nicolin Chen - Aug. 21, 2013, 3:13 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
Stephen Warren - Aug. 21, 2013, 6:30 p.m.
On 08/20/2013 09:13 PM, 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.

> diff --git a/Documentation/devicetree/bindings/sound/imx-audio-spdif.txt b/Documentation/devicetree/bindings/sound/imx-audio-spdif.txt

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

I still don't think those two properties are correct.

Exactly what node will those phandles point at?

There definitely should not be a DT node for any "dummy CODEC",
irrespective of whether this binding calls the other node a "CODEC" or a
"dummy CODEC".

If these properties are to contain phandles, it would be acceptable for
the referenced node to be:

* A node representing the physical connector/jack on the board.

* A node representing some other IP block on the board, such as an HDMI
encoder/display-controller

I think those options are unlikely in general, so I think instead these
properties should just be Boolean indicating that "something" is
connector to the S/PDIF RX/TX, without specifying what that "something"
is. It doesn't matter what at least in the connector/jack case, although
perhaps it does in the HDMI encoder/display-controller?
Tomasz Figa - Aug. 21, 2013, 6:54 p.m.
On Wednesday 21 of August 2013 12:30:59 Stephen Warren wrote:
> On 08/20/2013 09:13 PM, 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.
> > 
> > diff --git
> > a/Documentation/devicetree/bindings/sound/imx-audio-spdif.txt
> > b/Documentation/devicetree/bindings/sound/imx-audio-spdif.txt
> > 
> > +Optional properties:
> > +
> > +  - spdif-transmitter : The phandle of the spdif-transmitter codec
> > +
> > +  - spdif-receiver : The phandle of the spdif-receiver codec
> > +
> > +* Note: At least one of these two properties should be set in the DT
> > binding.
> I still don't think those two properties are correct.
> 
> Exactly what node will those phandles point at?

Imagine following setup:

  ________              ________________
 |        |     RX     | Microphone DSP |   Analog mic input
 | S/PDIF | <--------< |________________| <-------------------
 |        |             ________________
 |  DAI   | >--------> | Amplifier      | >-------------------
 |________|     TX     |________________|   Speakers output

As you see in the diagram, the S/PDIF interface of the SoC can be 
connected to some external devices that can perform sound processing or 
simply handle the physical layer.

I'd say that normally both RX and TX lines would be connected to a single 
codec chip that has multiple blocks inside, like sound processing, 
amplifier, mixer, etc., but nothing stops you from making a crazy setup, 
when RX and TX lines are connected to different chips.

> There definitely should not be a DT node for any "dummy CODEC",
> irrespective of whether this binding calls the other node a "CODEC" or a
> "dummy CODEC".

I agree. Instead if no chip connected to particular line is specified in 
device tree, it's responsibility of Linux sound core to handle this 
properly by adding a dummy codec or whatever.

> If these properties are to contain phandles, it would be acceptable for
> the referenced node to be:
> 
> * A node representing the physical connector/jack on the board.
> 
> * A node representing some other IP block on the board, such as an HDMI
> encoder/display-controller
> 
> I think those options are unlikely in general

Why? You usually codec SoC DAIs to some external chips.

Best regards,
Tomasz
Stephen Warren - Aug. 21, 2013, 10:14 p.m.
On 08/21/2013 12:54 PM, Tomasz Figa wrote:
> On Wednesday 21 of August 2013 12:30:59 Stephen Warren wrote:
>> On 08/20/2013 09:13 PM, 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.
>>>
>>> diff --git
>>> a/Documentation/devicetree/bindings/sound/imx-audio-spdif.txt
>>> b/Documentation/devicetree/bindings/sound/imx-audio-spdif.txt
>>>
>>> +Optional properties:
>>> +
>>> +  - spdif-transmitter : The phandle of the spdif-transmitter codec
>>> +
>>> +  - spdif-receiver : The phandle of the spdif-receiver codec
>>> +
>>> +* Note: At least one of these two properties should be set in the DT
>>> binding.
>> I still don't think those two properties are correct.
>>
>> Exactly what node will those phandles point at?
> 
> Imagine following setup:
> 
>   ________              ________________
>  |        |     RX     | Microphone DSP |   Analog mic input
>  | S/PDIF | <--------< |________________| <-------------------
>  |        |             ________________
>  |  DAI   | >--------> | Amplifier      | >-------------------
>  |________|     TX     |________________|   Speakers output
> 
> As you see in the diagram, the S/PDIF interface of the SoC can be 
> connected to some external devices that can perform sound processing or 
> simply handle the physical layer.
> 
> I'd say that normally both RX and TX lines would be connected to a single 
> codec chip that has multiple blocks inside, like sound processing, 
> amplifier, mixer, etc., but nothing stops you from making a crazy setup, 
> when RX and TX lines are connected to different chips.

That's much rarer with S/PDIF than I2S though right? Usually I'd expect
the S/PDIF controller to simply be routed out to a jack/connector on the
board, or perhaps to an internal HDMI encoder.

But the point of my question was more that the binding should fully
describe the type of object/node that the phandle should reference. The
type of the node would then imply what kind of operations could be
performed on that other node's driver by this node's driver. If the set
of operations is undefined, that's bad. If the set of operations is
NULL, there's probably no need to reference the node.

>> There definitely should not be a DT node for any "dummy CODEC",
>> irrespective of whether this binding calls the other node a "CODEC" or a
>> "dummy CODEC".
> 
> I agree. Instead if no chip connected to particular line is specified in 
> device tree, it's responsibility of Linux sound core to handle this 
> properly by adding a dummy codec or whatever.
> 
>> If these properties are to contain phandles, it would be acceptable for
>> the referenced node to be:
>>
>> * A node representing the physical connector/jack on the board.
>>
>> * A node representing some other IP block on the board, such as an HDMI
>> encoder/display-controller
>>
>> I think those options are unlikely in general
> 
> Why? You usually codec SoC DAIs to some external chips.

It's unlikely the phandle would reference a connector/jack since we
(thus far at least) haven't created DT nodes for them. If we do put
jacks/connectors into DT, then referencing them is fine.

In the "other IP block" case, it may be worth referencing the device,
although as I mentioned above, we need to describe exactly what is
expected from that other device interface-wise.
Nicolin Chen - Aug. 22, 2013, 11:40 a.m.
Hi Stephen,

On Wed, Aug 21, 2013 at 12:30:59PM -0600, Stephen Warren wrote:
> I still don't think those two properties are correct.
> 
> Exactly what node will those phandles point at?
> 
> There definitely should not be a DT node for any "dummy CODEC",
> irrespective of whether this binding calls the other node a "CODEC" or a
> "dummy CODEC".
> 
> If these properties are to contain phandles, it would be acceptable for
> the referenced node to be:
> 
> * A node representing the physical connector/jack on the board.
> 
> * A node representing some other IP block on the board, such as an HDMI
> encoder/display-controller
> 
> I think those options are unlikely in general, so I think instead these
> properties should just be Boolean indicating that "something" is
> connector to the S/PDIF RX/TX, without specifying what that "something"
> is. It doesn't matter what at least in the connector/jack case, although
> perhaps it does in the HDMI encoder/display-controller?

Documentation/devicetree/bindings/sound/spdif-receiver.txt
If I understand correctly, this doc for the dummy codec should be invalid?

But this patch, the spdif machine driver, is based on this codec driver,
pls check the following code:

164 +       codec_rx_np = of_parse_phandle(np, "spdif-receiver", 0);
165 +       if (codec_rx_np) {
169 +               data->dai[num_links].codec_of_node = codec_rx_np;
173 +       }

Accordingly, the binding I planned to add in DT:

27 +       spdif_rx_codec: spdif-receiver {
28 +               compatible = "linux,spdif-dir";
29 +       };
30 +
31 +       sound-spdif {
32 +               compatible = "fsl,imx-audio-spdif",
33 +                          "fsl,imx-sabreauto-spdif";
34 +               model = "imx-spdif";
35 +               spdif-controller = <&spdif>;
37 +               spdif-receiver = <&spdif_rx_codec>;
38 +       };


So if the DT can't allow me to include this codec node, how could I
handle it in the current baseline.

Could you please directly provide me a nicer means? Or maybe just an
eclectic way for everyone, since it doesn't look like we have a perfect
solution right now.

Thank you so much.
Nicolin
Stephen Warren - Aug. 22, 2013, 7:56 p.m.
On 08/22/2013 05:40 AM, Nicolin Chen wrote:
> Hi Stephen,
> 
> On Wed, Aug 21, 2013 at 12:30:59PM -0600, Stephen Warren wrote:
>> I still don't think those two properties are correct.
>>
>> Exactly what node will those phandles point at?
>>
>> There definitely should not be a DT node for any "dummy CODEC",
>> irrespective of whether this binding calls the other node a "CODEC" or a
>> "dummy CODEC".
>>
>> If these properties are to contain phandles, it would be acceptable for
>> the referenced node to be:
>>
>> * A node representing the physical connector/jack on the board.
>>
>> * A node representing some other IP block on the board, such as an HDMI
>> encoder/display-controller
>>
>> I think those options are unlikely in general, so I think instead these
>> properties should just be Boolean indicating that "something" is
>> connector to the S/PDIF RX/TX, without specifying what that "something"
>> is. It doesn't matter what at least in the connector/jack case, although
>> perhaps it does in the HDMI encoder/display-controller?
> 
> Documentation/devicetree/bindings/sound/spdif-receiver.txt
> If I understand correctly, this doc for the dummy codec should be invalid?

Yes, I'm not convinced that binding is a good idea; it describes
something that often doesn't actually exist in HW. (Sometimes there's a
real S/PDIF receiving device on board, but sometimes there's nothing
except a jack/connector).

It'd be useful if other DT binding maintainers could weigh in on this to
confirm/deny my thoughts.

> But this patch, the spdif machine driver, is based on this codec driver,
> pls check the following code:
> 
> 164 +       codec_rx_np = of_parse_phandle(np, "spdif-receiver", 0);
> 165 +       if (codec_rx_np) {
> 169 +               data->dai[num_links].codec_of_node = codec_rx_np;
> 173 +       }
> 
> Accordingly, the binding I planned to add in DT:
> 
> 27 +       spdif_rx_codec: spdif-receiver {
> 28 +               compatible = "linux,spdif-dir";
> 29 +       };
> 30 +
> 31 +       sound-spdif {
> 32 +               compatible = "fsl,imx-audio-spdif",
> 33 +                          "fsl,imx-sabreauto-spdif";
> 34 +               model = "imx-spdif";
> 35 +               spdif-controller = <&spdif>;
> 37 +               spdif-receiver = <&spdif_rx_codec>;
> 38 +       };
> 
> So if the DT can't allow me to include this codec node, how could I
> handle it in the current baseline.

I would expect the machine driver's probe routine to create the dummy
S/PDIF receiver object itself, and register it by calling
snd_soc_register_codec(). That way, only the machine driver code need
know it (dummy CODEC) exists.
Mark Brown - Aug. 22, 2013, 8:05 p.m.
On Thu, Aug 22, 2013 at 01:56:32PM -0600, Stephen Warren wrote:
> On 08/22/2013 05:40 AM, Nicolin Chen wrote:

> > Documentation/devicetree/bindings/sound/spdif-receiver.txt
> > If I understand correctly, this doc for the dummy codec should be invalid?

> Yes, I'm not convinced that binding is a good idea; it describes
> something that often doesn't actually exist in HW. (Sometimes there's a
> real S/PDIF receiving device on board, but sometimes there's nothing
> except a jack/connector).

> It'd be useful if other DT binding maintainers could weigh in on this to
> confirm/deny my thoughts.

I think the binding should be changed to replace the word "dummy" with
"generic" and perhaps some verbiage about not requiring software
configuration.  I think given the unidirectional nature of S/PDIF it's
reasonable to represent a jack like this - the hardware can't generally
tell if there's anything at the other end of the link anyway, for all
pratical purposes the transmit end just has to blindly send.

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..08a129a
--- /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 codec
+
+  - spdif-receiver : The phandle of the spdif-receiver 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");