| Message ID | 1327517363-29698-1-git-send-email-leon@leon.nu |
|---|---|
| State | Superseded, archived |
| Headers | show |
Leon Romanovsky wrote at Wednesday, January 25, 2012 11:49 AM: > This patch adds initial device tree support of ALC5632 sound codec and > machine driver for PAZ00 board. The implementation is based on the WM8903 codec. > .../devicetree/bindings/sound/alc5632.txt | 24 ++++++ > .../bindings/sound/tegra-audio-alc5632.txt | 55 ++++++++++++++ > arch/arm/boot/dts/tegra-paz00.dts | 29 ++++++-- > sound/soc/tegra/tegra_alc5632.c | 78 ++++++++++++++++++-- I worry this patch does a little too much at once; perhaps it should be split up a little? > diff --git a/Documentation/devicetree/bindings/sound/alc5632.txt ... > +Required properties: > + > + - compatible : "realtek,alc5632" You should update Documentation/devicetree/bindings/vendor-prefixes.txt too. Separately from this patch is probably fine. > diff --git a/Documentation/devicetree/bindings/sound/tegra-audio-alc5632.txt > b/Documentation/devicetree/bindings/sound/tegra-audio-alc5632.txt > new file mode 100644 > index 0000000..315213f > --- /dev/null > +++ b/Documentation/devicetree/bindings/sound/tegra-audio-alc5632.txt > @@ -0,0 +1,55 @@ > +NVIDIA Tegra audio complex > + > +Required properties: > +- compatible : "nvidia,tegra-audio-alc5632" > +- nvidia,model : The user-visible name of this sound complex. > +- nvidia,audio-routing : A list of the connections between audio components. > + Each entry is a pair of strings, the first being the connection's sink, > + the second being the connection's source. Valid names for sources and > + sinks are the ALC5632's pins: > + > + ALC5632 pins: > + > + * SPKOUT > + * SPKOUTN > + * HPL > + * HPR > + * AUXOUT My copy of the ALC5632 datasheet indicates there are both AUX_OUT_P and AUX_OUTN pins. Are they always used together such that it makes sense to group them together in the device tree binding? > + * LINEINL > + * LINEINR > + * PHONEP > + * PHONEP PHONEN > + * MIC1 > + * MIC2 Same as above; the datasheet lists MIC1_P, MIC1_N, MIC2_P, MIC2_N. > + * MICBIAS1 > + * MICBIAS2 I only see MICBIAS1 in the datasheet, not MICBIAS2. > + > + Board connectors: > + > + * Headset Stereophone > + * Int Spk > + * MIC1 > + * MICBIAS1 Those last two are codec pins, not board connectors. Don't you need "Headset Mic" in the list too? ... > + nvidia,audio-routing = > + "Int Spk", "SPKOUT", > + "Int Spk", "SPKOUTN", > + "MIC1", "MICBIAS1", > + "MICBIAS1", "Headset Mic", I think those last two lines should read: "Headset Mic", "MICBIAS1", "MIC1", " Headset Mic", The DAPM route table in the driver probably needs updating to say the same thing too. (all the comments on the example above apply to the copy in the .dts file too) ... > diff --git a/arch/arm/boot/dts/tegra-paz00.dts b/arch/arm/boot/dts/tegra-paz00.dts > + sound { > + compatible = "nvidia,tegra-audio-alc5632-paz00", > + "nvidia,tegra-audio-alc5632"; That last line needs some extra indent. > diff --git a/sound/soc/tegra/tegra_alc5632.c b/sound/soc/tegra/tegra_alc5632.c > @@ -163,26 +164,80 @@ static __devinit int tegra_alc5632_probe(struct platform_device *pdev) ... > + alc5632->pcm_dev = ERR_PTR(-EINVAL); > + > + if (!(pdev->dev.of_node)) { > + dev_err(&pdev->dev, "No platform data supplied\n"); I think I'd phrase that more like "Must be instantiated using device tree". > + tegra_alc5632_dai.codec_name = NULL; Given you're only supporting instantiation using device tree now, why not just not initialize that field, and remove that assignment? > + tegra_alc5632_dai.codec_of_node = of_parse_phandle( > + pdev->dev.of_node, "nvidia,audio-codec", 0); Indent that last line a bit more. > + tegra_alc5632_dai.cpu_dai_name = NULL; Same comment; you can remove that. > + tegra_alc5632_dai.cpu_dai_of_node = of_parse_phandle( > + pdev->dev.of_node, "nvidia,i2s-controller", 0); Indent that last line a bit more. Overall, this looks like the same structure as the Tegra+WM8903 bindings, so it works for me.
On Thu, Jan 26, 2012 at 00:21, Stephen Warren <swarren@nvidia.com> wrote: > Leon Romanovsky wrote at Wednesday, January 25, 2012 11:49 AM: >> This patch adds initial device tree support of ALC5632 sound codec and >> machine driver for PAZ00 board. The implementation is based on the WM8903 codec. > >> +++ b/Documentation/devicetree/bindings/sound/tegra-audio-alc5632.txt >> @@ -0,0 +1,55 @@ >> +NVIDIA Tegra audio complex >> + >> +Required properties: >> +- compatible : "nvidia,tegra-audio-alc5632" >> +- nvidia,model : The user-visible name of this sound complex. >> +- nvidia,audio-routing : A list of the connections between audio components. >> + Each entry is a pair of strings, the first being the connection's sink, >> + the second being the connection's source. Valid names for sources and >> + sinks are the ALC5632's pins: >> + >> + ALC5632 pins: >> + >> + * SPKOUT >> + * SPKOUTN >> + * HPL >> + * HPR >> + * AUXOUT > > My copy of the ALC5632 datasheet indicates there are both AUX_OUT_P and > AUX_OUTN pins. Are they always used together such that it makes sense to > group them together in the device tree binding? You are right, it must be the same as SPKOUT > >> + * LINEINL >> + * LINEINR >> + * PHONEP >> + * PHONEP > > PHONEN > >> + * MIC1 >> + * MIC2 > > Same as above; the datasheet lists MIC1_P, MIC1_N, MIC2_P, MIC2_N. > >> + * MICBIAS1 >> + * MICBIAS2 > > I only see MICBIAS1 in the datasheet, not MICBIAS2. You are right if you are looking on function block only (page 3), but in register description you can find reference to MICBIAS2 (reg 22h, microphone control). > >> + >> + Board connectors: >> + >> + * Headset Stereophone >> + * Int Spk >> + * MIC1 >> + * MICBIAS1 > > Those last two are codec pins, not board connectors. > > Don't you need "Headset Mic" in the list too? > > ... >> + nvidia,audio-routing = >> + "Int Spk", "SPKOUT", >> + "Int Spk", "SPKOUTN", >> + "MIC1", "MICBIAS1", >> + "MICBIAS1", "Headset Mic", > > I think those last two lines should read: > > "Headset Mic", "MICBIAS1", > "MIC1", " Headset Mic", > > The DAPM route table in the driver probably needs updating to say the > same thing too. > > (all the comments on the example above apply to the copy in the .dts > file too) Microphone is not tested at all, so you probably right. I prefer to be close as possible to previous board implementation and provide followup patches to clean the microphone path. > Overall, this looks like the same structure as the Tegra+WM8903 bindings, > so it works for me. > -- > nvpublic >
Leon Romanovsky wrote at Friday, January 27, 2012 9:02 AM: > On Thu, Jan 26, 2012 at 00:21, Stephen Warren <swarren@nvidia.com> wrote: > > Leon Romanovsky wrote at Wednesday, January 25, 2012 11:49 AM: > >> This patch adds initial device tree support of ALC5632 sound codec and > >> machine driver for PAZ00 board. The implementation is based on the WM8903 codec. > > > >> +++ b/Documentation/devicetree/bindings/sound/tegra-audio-alc5632.txt > >> @@ -0,0 +1,55 @@ > >> +NVIDIA Tegra audio complex > >> + > >> +Required properties: > >> +- compatible : "nvidia,tegra-audio-alc5632" > >> +- nvidia,model : The user-visible name of this sound complex. > >> +- nvidia,audio-routing : A list of the connections between audio components. > >> + Each entry is a pair of strings, the first being the connection's sink, > >> + the second being the connection's source. Valid names for sources and > >> + sinks are the ALC5632's pins: > >> + > >> + ALC5632 pins: > >> + > >> + * SPKOUT > >> + * SPKOUTN > >> + * HPL > >> + * HPR > >> + * AUXOUT > > > > My copy of the ALC5632 datasheet indicates there are both AUX_OUT_P and > > AUX_OUTN pins. Are they always used together such that it makes sense to > > group them together in the device tree binding? > > You are right, it must be the same as SPKOUT I think the opposite; AUXOUT and SPKOUT aren't the same pin. I meant that AUX_OUT_P and AUX_OUT_N are different pins, so I'm asking why they're a single pin in the above pin list (and in the ASoC codec driver). In contrast, SPKOUT is represented explicitly as two pins which seems to make more sense. > >> + * LINEINL > >> + * LINEINR > >> + * PHONEP > >> + * PHONEP > > > > PHONEN > > > >> + * MIC1 > >> + * MIC2 > > > > Same as above; the datasheet lists MIC1_P, MIC1_N, MIC2_P, MIC2_N. > > > >> + * MICBIAS1 > >> + * MICBIAS2 > > > > I only see MICBIAS1 in the datasheet, not MICBIAS2. > > You are right if you are looking on function block only (page 3), but > in register description you can find reference to MICBIAS2 (reg 22h, > microphone control). I do see that register bit, but the pinout doesn't include it in the codec's own datasheet nor the AC100 schematics. I suspect that there really is no MICBIAS2 feature and the register documentation is simply incorrect, perhaps a carry-over from some other codec? Still, I suppose that just in case the signal really is routed to one of the pins labeled "NC" in the datasheet, there isn't too much harm including it in the driver, and this binding. > >> + Board connectors: ... > > Don't you need "Headset Mic" in the list too? ... > >> + nvidia,audio-routing = ... > >> + "MIC1", "MICBIAS1", > >> + "MICBIAS1", "Headset Mic", > > > > I think those last two lines should read: > > > > "Headset Mic", "MICBIAS1", > > "MIC1", " Headset Mic", > > > > The DAPM route table in the driver probably needs updating to say the > > same thing too. > > Microphone is not tested at all, so you probably right. I prefer to be > close as possible to previous board implementation and provide > followup patches to clean the microphone path. I guess that doesn't actually affect the structure of the binding, just the data contained in the properties, so it's not a big deal if it goes in like this. Mark might have a different opinion though, since it's a simple fix... -- nvpublic
On Fri, Jan 27, 2012 at 18:45, Stephen Warren <swarren@nvidia.com> wrote: > Leon Romanovsky wrote at Friday, January 27, 2012 9:02 AM: >> On Thu, Jan 26, 2012 at 00:21, Stephen Warren <swarren@nvidia.com> wrote: >> > Leon Romanovsky wrote at Wednesday, January 25, 2012 11:49 AM: >> >> This patch adds initial device tree support of ALC5632 sound codec and >> >> machine driver for PAZ00 board. The implementation is based on the WM8903 codec. >> > >> >> +++ b/Documentation/devicetree/bindings/sound/tegra-audio-alc5632.txt >> >> @@ -0,0 +1,55 @@ >> >> +NVIDIA Tegra audio complex >> >> + >> >> +Required properties: >> >> +- compatible : "nvidia,tegra-audio-alc5632" >> >> +- nvidia,model : The user-visible name of this sound complex. >> >> +- nvidia,audio-routing : A list of the connections between audio components. >> >> + Each entry is a pair of strings, the first being the connection's sink, >> >> + the second being the connection's source. Valid names for sources and >> >> + sinks are the ALC5632's pins: >> >> + >> >> + ALC5632 pins: >> >> + >> >> + * SPKOUT >> >> + * SPKOUTN >> >> + * HPL >> >> + * HPR >> >> + * AUXOUT >> > >> > My copy of the ALC5632 datasheet indicates there are both AUX_OUT_P and >> > AUX_OUTN pins. Are they always used together such that it makes sense to >> > group them together in the device tree binding? >> >> You are right, it must be the same as SPKOUT > > I think the opposite; AUXOUT and SPKOUT aren't the same pin. I meant > that AUX_OUT_P and AUX_OUT_N are different pins, so I'm asking why they're > a single pin in the above pin list (and in the ASoC codec driver). In > contrast, SPKOUT is represented explicitly as two pins which seems to make > more sense. Sorry for the misunderstanding, This is what i wanted to say. > >> >> + * LINEINL >> >> + * LINEINR >> >> + * PHONEP >> >> + * PHONEP >> > >> > PHONEN >> > >> >> + * MIC1 >> >> + * MIC2 >> > >> > Same as above; the datasheet lists MIC1_P, MIC1_N, MIC2_P, MIC2_N. >> > >> >> + * MICBIAS1 >> >> + * MICBIAS2 >> > >> > I only see MICBIAS1 in the datasheet, not MICBIAS2. >> >> You are right if you are looking on function block only (page 3), but >> in register description you can find reference to MICBIAS2 (reg 22h, >> microphone control). > > I do see that register bit, but the pinout doesn't include it in the > codec's own datasheet nor the AC100 schematics. I suspect that there > really is no MICBIAS2 feature and the register documentation is simply > incorrect, perhaps a carry-over from some other codec? Still, I suppose > that just in case the signal really is routed to one of the pins labeled > "NC" in the datasheet, there isn't too much harm including it in the > driver, and this binding. Sure > >> >> + Board connectors: > ... >> > Don't you need "Headset Mic" in the list too? > ... >> >> + nvidia,audio-routing = > ... >> >> + "MIC1", "MICBIAS1", >> >> + "MICBIAS1", "Headset Mic", >> > >> > I think those last two lines should read: >> > >> > "Headset Mic", "MICBIAS1", >> > "MIC1", " Headset Mic", >> > >> > The DAPM route table in the driver probably needs updating to say the >> > same thing too. >> >> Microphone is not tested at all, so you probably right. I prefer to be >> close as possible to previous board implementation and provide >> followup patches to clean the microphone path. > > I guess that doesn't actually affect the structure of the binding, just > the data contained in the properties, so it's not a big deal if it goes > in like this. Mark might have a different opinion though, since it's > a simple fix... > > -- > nvpublic >
diff --git a/Documentation/devicetree/bindings/sound/alc5632.txt b/Documentation/devicetree/bindings/sound/alc5632.txt new file mode 100644 index 0000000..8608f74 --- /dev/null +++ b/Documentation/devicetree/bindings/sound/alc5632.txt @@ -0,0 +1,24 @@ +ALC5632 audio CODEC + +This device supports I2C only. + +Required properties: + + - compatible : "realtek,alc5632" + + - reg : the I2C address of the device. + + - gpio-controller : Indicates this device is a GPIO controller. + + - #gpio-cells : Should be two. The first cell is the pin number and the + second cell is used to specify optional parameters (currently unused). + +Example: + +alc5632: alc5632@1e { + compatible = "realtek,alc5632"; + reg = <0x1a>; + + gpio-controller; + #gpio-cells = <2>; +}; diff --git a/Documentation/devicetree/bindings/sound/tegra-audio-alc5632.txt b/Documentation/devicetree/bindings/sound/tegra-audio-alc5632.txt new file mode 100644 index 0000000..315213f --- /dev/null +++ b/Documentation/devicetree/bindings/sound/tegra-audio-alc5632.txt @@ -0,0 +1,55 @@ +NVIDIA Tegra audio complex + +Required properties: +- compatible : "nvidia,tegra-audio-alc5632" +- nvidia,model : The user-visible name of this sound complex. +- nvidia,audio-routing : A list of the connections between audio components. + Each entry is a pair of strings, the first being the connection's sink, + the second being the connection's source. Valid names for sources and + sinks are the ALC5632's pins: + + ALC5632 pins: + + * SPKOUT + * SPKOUTN + * HPL + * HPR + * AUXOUT + * LINEINL + * LINEINR + * PHONEP + * PHONEP + * MIC1 + * MIC2 + * MICBIAS1 + * MICBIAS2 + + Board connectors: + + * Headset Stereophone + * Int Spk + * MIC1 + * MICBIAS1 + +- nvidia,i2s-controller : The phandle of the Tegra I2S controller +- nvidia,audio-codec : The phandle of the ALC5632 audio codec + +Example: + +sound { + compatible = "nvidia,tegra-audio-alc5632-paz00", + "nvidia,tegra-audio-alc5632"; + + nvidia,model = "Compal PAZ00"; + + nvidia,audio-routing = + "Int Spk", "SPKOUT", + "Int Spk", "SPKOUTN", + "MIC1", "MICBIAS1", + "MICBIAS1", "Headset Mic", + "Headset Stereophone", "HPR", + "Headset Stereophone", "HPL"; + + nvidia,i2s-controller = <&tegra_i2s1>; + nvidia,audio-codec = <&alc5632>; +}; diff --git a/arch/arm/boot/dts/tegra-paz00.dts b/arch/arm/boot/dts/tegra-paz00.dts index 4d1bcdc..692d20c 100644 --- a/arch/arm/boot/dts/tegra-paz00.dts +++ b/arch/arm/boot/dts/tegra-paz00.dts @@ -12,6 +12,13 @@ i2c@7000c000 { clock-frequency = <400000>; + + alc5632: alc5632@1e { + compatible = "realtek,alc5632"; + reg = <0x1e>; + gpio-controller; + #gpio-cells = <2>; + }; }; i2c@7000c400 { @@ -37,16 +44,26 @@ clock-frequency = <400000>; }; - i2s@70002800 { - status = "disable"; - }; - i2s@70002a00 { status = "disable"; }; - das@70000c00 { - status = "disable"; + sound { + compatible = "nvidia,tegra-audio-alc5632-paz00", + "nvidia,tegra-audio-alc5632"; + + nvidia,model = "Compal PAZ00"; + + nvidia,audio-routing = + "Int Spk", "SPKOUT", + "Int Spk", "SPKOUTN", + "MIC1", "MICBIAS1", + "MICBIAS1", "Headset Mic", + "Headset Stereophone", "HPR", + "Headset Stereophone", "HPL"; + + nvidia,audio-codec = <&alc5632>; + nvidia,i2s-controller = <&tegra_i2s1>; }; serial@70006000 { diff --git a/sound/soc/tegra/tegra_alc5632.c b/sound/soc/tegra/tegra_alc5632.c index 4a0e805..22348e8 100644 --- a/sound/soc/tegra/tegra_alc5632.c +++ b/sound/soc/tegra/tegra_alc5632.c @@ -36,6 +36,7 @@ struct tegra_alc5632 { struct tegra_asoc_utils_data util_data; + struct platform_device *pcm_dev; }; static int tegra_alc5632_asoc_hw_params(struct snd_pcm_substream *substream, @@ -163,26 +164,80 @@ static __devinit int tegra_alc5632_probe(struct platform_device *pdev) sizeof(struct tegra_alc5632), GFP_KERNEL); if (!alc5632) { dev_err(&pdev->dev, "Can't allocate tegra_alc5632\n"); - return -ENOMEM; + ret = -ENOMEM; + goto err; } - ret = tegra_asoc_utils_init(&alc5632->util_data, &pdev->dev); - if (ret) - return ret; - card->dev = &pdev->dev; platform_set_drvdata(pdev, card); snd_soc_card_set_drvdata(card, alc5632); + alc5632->pcm_dev = ERR_PTR(-EINVAL); + + if (!(pdev->dev.of_node)) { + dev_err(&pdev->dev, "No platform data supplied\n"); + ret = -EINVAL; + goto err; + } + + ret = snd_soc_of_parse_card_name(card, "nvidia,model"); + if (ret) + goto err; + + ret = snd_soc_of_parse_audio_routing(card, "nvidia,audio-routing"); + if (ret) + goto err; + + tegra_alc5632_dai.codec_name = NULL; + tegra_alc5632_dai.codec_of_node = of_parse_phandle( + pdev->dev.of_node, "nvidia,audio-codec", 0); + + if (!tegra_alc5632_dai.codec_of_node) { + dev_err(&pdev->dev, + "Property 'nvidia,audio-codec' missing or invalid\n"); + ret = -EINVAL; + goto err; + } + + tegra_alc5632_dai.cpu_dai_name = NULL; + tegra_alc5632_dai.cpu_dai_of_node = of_parse_phandle( + pdev->dev.of_node, "nvidia,i2s-controller", 0); + if (!tegra_alc5632_dai.cpu_dai_of_node) { + dev_err(&pdev->dev, + "Property 'nvidia,i2s-controller' missing or invalid\n"); + ret = -EINVAL; + goto err; + } + + alc5632->pcm_dev = platform_device_register_simple( + "tegra-pcm-audio", -1, NULL, 0); + if (IS_ERR(alc5632->pcm_dev)) { + dev_err(&pdev->dev, + "Can't instantiate tegra-pcm-audio\n"); + ret = PTR_ERR(alc5632->pcm_dev); + goto err; + } + + ret = tegra_asoc_utils_init(&alc5632->util_data, &pdev->dev); + if (ret) + goto err_unregister; + ret = snd_soc_register_card(card); if (ret) { dev_err(&pdev->dev, "snd_soc_register_card failed (%d)\n", ret); - tegra_asoc_utils_fini(&alc5632->util_data); - return ret; + goto err_fini_utils; } return 0; + +err_fini_utils: + tegra_asoc_utils_fini(&alc5632->util_data); +err_unregister: + if (!IS_ERR(alc5632->pcm_dev)) + platform_device_unregister(alc5632->pcm_dev); +err: + return ret; } static int __devexit tegra_alc5632_remove(struct platform_device *pdev) @@ -193,15 +248,23 @@ static int __devexit tegra_alc5632_remove(struct platform_device *pdev) snd_soc_unregister_card(card); tegra_asoc_utils_fini(&alc5632->util_data); + if (!IS_ERR(alc5632->pcm_dev)) + platform_device_unregister(alc5632->pcm_dev); return 0; } +static const struct of_device_id tegra_alc5632_of_match[] __devinitconst = { + { .compatible = "nvidia,tegra-audio-alc5632", }, + {}, +}; + static struct platform_driver tegra_alc5632_driver = { .driver = { .name = DRV_NAME, .owner = THIS_MODULE, .pm = &snd_soc_pm_ops, + .of_match_table = tegra_alc5632_of_match, }, .probe = tegra_alc5632_probe, .remove = __devexit_p(tegra_alc5632_remove), @@ -212,3 +275,4 @@ MODULE_AUTHOR("Leon Romanovsky <leon@leon.nu>"); MODULE_DESCRIPTION("Tegra+ALC5632 machine ASoC driver"); MODULE_LICENSE("GPL"); MODULE_ALIAS("platform:" DRV_NAME); +MODULE_DEVICE_TABLE(of, tegra_alc5632_of_match);