| Message ID | e7ceef74714bb4ead624b333ee3969225b4c5cb5.1376901081.git.b42378@freescale.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
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.
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
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.
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
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
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.
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.
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.
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.
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");
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