| Message ID | 1393947831-26116-1-git-send-email-jsarha@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Mark Brown |
| Headers | show |
On Tue, Mar 04, 2014 at 05:43:51PM +0200, Jyri Sarha wrote: > Add machine driver support for AM43xx-ePOS-EVM and update associated > device tree binding document. Don't send new patches in replies to the middle of threads, that is just confusing and hard to follow. Send a new series. > +/* Logic for EVMs with an aic31xx */ > +static int evm_aic31xx_init(struct snd_soc_pcm_runtime *rtd) > +{ > + struct snd_soc_codec *codec = rtd->codec; > + struct snd_soc_card *card = rtd->card; > + struct snd_soc_dapm_context *dapm = &card->dapm; > + struct device_node *np = card->dev->of_node; > + int ret; > + > + snd_soc_dapm_new_controls(dapm, aic31xx_dapm_widgets, > + ARRAY_SIZE(aic31xx_dapm_widgets)); > + > + if (np) { > + ret = snd_soc_of_parse_audio_routing(codec->card, > + "ti,audio-routing"); > + if (ret) > + return ret; > + } Why not add the DAPM widget table to... > + { > + .compatible = "ti,am43xx-epos-evm-audio", > + .data = &evm_dai_tlv320aic3111, > + }, ...the data here? Or alternatively should support for the widgets binding that was recently added by added to the kernel - that way the binding becomes more general and doesnn't need individual boards adding? Indeed ideally the simple-card binding (which that was added for) could just be used for these boards.
On 03/05/2014 03:42 AM, Mark Brown wrote: > On Tue, Mar 04, 2014 at 05:43:51PM +0200, Jyri Sarha wrote: >> Add machine driver support for AM43xx-ePOS-EVM and update associated >> device tree binding document. > > Don't send new patches in replies to the middle of threads, that is just > confusing and hard to follow. Send a new series. > Ok, won't do that anymore. >> +/* Logic for EVMs with an aic31xx */ >> +static int evm_aic31xx_init(struct snd_soc_pcm_runtime *rtd) >> +{ >> + struct snd_soc_codec *codec = rtd->codec; >> + struct snd_soc_card *card = rtd->card; >> + struct snd_soc_dapm_context *dapm = &card->dapm; >> + struct device_node *np = card->dev->of_node; >> + int ret; >> + >> + snd_soc_dapm_new_controls(dapm, aic31xx_dapm_widgets, >> + ARRAY_SIZE(aic31xx_dapm_widgets)); >> + >> + if (np) { >> + ret = snd_soc_of_parse_audio_routing(codec->card, >> + "ti,audio-routing"); >> + if (ret) >> + return ret; >> + } > > Why not add the DAPM widget table to... > >> + { >> + .compatible = "ti,am43xx-epos-evm-audio", >> + .data = &evm_dai_tlv320aic3111, >> + }, > > ...the data here? > > Or alternatively should support for the widgets binding that was > recently added by added to the kernel - that way the binding becomes > more general and doesnn't need individual boards adding? Indeed ideally > the simple-card binding (which that was added for) could just be used > for these boards. > I got am43x-epos-evm audio working on simple-card directly. I guess we can forget about this patch. However, I think there is a problem with simple-card. It does not invert bitclock-master and frame-master values when converting them to CB[SM] and CF[SM] for cpu_dai as I think it should. I can get around it by setting the both cpu-dai and the codec as bclk and frame masters in the sound node. I volunteer to fix this if you agree. Best regards, Jyri
On Fri, Mar 07, 2014 at 02:45:27PM +0200, Jyri Sarha wrote: > However, I think there is a problem with simple-card. It does not > invert bitclock-master and frame-master values when converting them > to CB[SM] and CF[SM] for cpu_dai as I think it should. I can get > around it by setting the both cpu-dai and the codec as bclk and > frame masters in the sound node. I volunteer to fix this if you > agree. No, it shouldn't do any inversion. If inversion is required one of the drivers is buggy, they are CODEC bit master and CPU bit master so for a CPU driver the sense should be inverted when parsing.
On 03/09/2014 10:11 AM, Mark Brown wrote: > On Fri, Mar 07, 2014 at 02:45:27PM +0200, Jyri Sarha wrote: > >> >However, I think there is a problem with simple-card. It does not >> >invert bitclock-master and frame-master values when converting them >> >to CB[SM] and CF[SM] for cpu_dai as I think it should. I can get >> >around it by setting the both cpu-dai and the codec as bclk and >> >frame masters in the sound node. I volunteer to fix this if you >> >agree. > No, it shouldn't do any inversion. If inversion is required one of the > drivers is buggy, they are CODEC bit master and CPU bit master so for a > CPU driver the sense should be inverted when parsing. Yes, that is the problem. The same code in simple-card parses the codec node and cpu-dai node and they produce the same SND_SOC_DAIFMT_C??_C?? flags for both codec and cpu-dai drivers. For example: simple-audio-card,cpu { sound-dai = <&mcasp1>; }; causes SND_SOC_DAIFMT_CBS_CFS flags to be set to mcasp driver in set_dai_fmt() call. So omitting the bitclock-master and frame-master parameters from cpu-dai node strangely indicates that the cpu-dai should be the bitclock and frame master. Can this be right? If you do not want to have the inversion, could we at least call the parameters codec-bitclock-master and codec-frame-master. Best regards, Jyri
On Mon, Mar 10, 2014 at 12:49:56PM +0200, Jyri Sarha wrote: > On 03/09/2014 10:11 AM, Mark Brown wrote: > >No, it shouldn't do any inversion. If inversion is required one of the > >drivers is buggy, they are CODEC bit master and CPU bit master so for a > >CPU driver the sense should be inverted when parsing. > Yes, that is the problem. The same code in simple-card parses the > codec node and cpu-dai node and they produce the same > SND_SOC_DAIFMT_C??_C?? flags for both codec and cpu-dai drivers. Sorry, thinko above - the master flags are specified in terms of the CODEC. Anything interpreting them that isn't a CODEC needs to be inverting the sense.
On 03/10/2014 01:09 PM, Mark Brown wrote: > On Mon, Mar 10, 2014 at 12:49:56PM +0200, Jyri Sarha wrote: >> On 03/09/2014 10:11 AM, Mark Brown wrote: > >>> No, it shouldn't do any inversion. If inversion is required one of the >>> drivers is buggy, they are CODEC bit master and CPU bit master so for a >>> CPU driver the sense should be inverted when parsing. > >> Yes, that is the problem. The same code in simple-card parses the >> codec node and cpu-dai node and they produce the same >> SND_SOC_DAIFMT_C??_C?? flags for both codec and cpu-dai drivers. > > Sorry, thinko above - the master flags are specified in terms of the > CODEC. Anything interpreting them that isn't a CODEC needs to be > inverting the sense. Exactly, CBM_CFM means that the codec is the master of both clocks. Codec is configured as master and the cpu side is configured as slave. But the issue is that with simple card (when you want to have the codec as master for both clocks): simple-audio-card,codec { sound-dai = <&aic3106>; bitclock-master; frame-master; }; simple-audio-card,cpu { sound-dai = <&mcasp1 0>; }; The codec will get CBM_CFM, however the cpu_dai will end up having CBS_CFS (since *-master is not specified in the dts). So when the simple card parses the master/slave configuration it has to invert the cpu_dai settings it got back from snd_soc_of_parse_daifmt() to get it right. Since: simple-audio-card,codec { sound-dai = <&aic3106>; }; simple-audio-card,cpu { sound-dai = <&mcasp1 0>; bitclock-master; frame-master; }; Will end up as: codec is CBS_CFS and cpu_dai as CBM_CFM which means that both cpu and codec is supposed to be slave...
On Mon, Mar 10, 2014 at 01:31:15PM +0200, Peter Ujfalusi wrote: > Exactly, CBM_CFM means that the codec is the master of both clocks. Codec is > configured as master and the cpu side is configured as slave. > But the issue is that with simple card (when you want to have the codec as > master for both clocks): > simple-audio-card,codec { > sound-dai = <&aic3106>; > bitclock-master; > frame-master; > }; > simple-audio-card,cpu { > sound-dai = <&mcasp1 0>; > }; > The codec will get CBM_CFM, however the cpu_dai will end up having CBS_CFS > (since *-master is not specified in the dts). What I would expect the code to be doing here is coming up with the same setting for both ends of the link rather than trying to parse the two ends of the link independently - though that *is* more flexible it's unlikely to actually work in a system. The properties on each end of the link aren't independent of each other. > So when the simple card parses the master/slave configuration it has to invert > the cpu_dai settings it got back from snd_soc_of_parse_daifmt() to get it right. Or there should just be one unified parse. When reading Jiri's mail the lack of any reference to the code it really sounded like he was asking for the result of a single parse to inverted between passing to the CODEC and CPU sides. Needs looking at.
diff --git a/Documentation/devicetree/bindings/sound/davinci-evm-audio.txt b/Documentation/devicetree/bindings/sound/davinci-evm-audio.txt index 865178d..356cba1 100644 --- a/Documentation/devicetree/bindings/sound/davinci-evm-audio.txt +++ b/Documentation/devicetree/bindings/sound/davinci-evm-audio.txt @@ -2,8 +2,10 @@ Required properties: - compatible : "ti,da830-evm-audio" : forDM365/DA8xx/OMAPL1x/AM33xx + : "ti,am43xx-epos-evm-audio" : for am43xx-epos-evm - ti,model : The user-visible name of this sound complex. -- ti,audio-codec : The phandle of the TLV320AIC3x audio codec +- ti,audio-codec : The phandle of the TLV320AIC3x audio codec, + or the TLV320AIC31xx audio codec. - ti,mcasp-controller : The phandle of the McASP controller - ti,codec-clock-rate : The Codec Clock rate (in Hz) applied to the Codec - ti,audio-routing : A list of the connections between audio components. @@ -14,9 +16,10 @@ Required properties: Board connectors: * Headphone Jack - * Line Out + * Line Out - "ti,da830-evm-audio" only * Mic Jack - * Line In + * Line In - "ti,da830-evm-audio" only + * Speaker - "ti,am43xx-epos-evm-audio" only Example: diff --git a/sound/soc/davinci/davinci-evm.c b/sound/soc/davinci/davinci-evm.c index 5e3bc3c..75a6165 100644 --- a/sound/soc/davinci/davinci-evm.c +++ b/sound/soc/davinci/davinci-evm.c @@ -128,6 +128,34 @@ static int evm_aic3x_init(struct snd_soc_pcm_runtime *rtd) return 0; } +static const struct snd_soc_dapm_widget aic31xx_dapm_widgets[] = { + SND_SOC_DAPM_HP("Headphone Jack", NULL), + SND_SOC_DAPM_SPK("Speaker", NULL), + SND_SOC_DAPM_MIC("Mic Jack", NULL), +}; + +/* Logic for EVMs with an aic31xx */ +static int evm_aic31xx_init(struct snd_soc_pcm_runtime *rtd) +{ + struct snd_soc_codec *codec = rtd->codec; + struct snd_soc_card *card = rtd->card; + struct snd_soc_dapm_context *dapm = &card->dapm; + struct device_node *np = card->dev->of_node; + int ret; + + snd_soc_dapm_new_controls(dapm, aic31xx_dapm_widgets, + ARRAY_SIZE(aic31xx_dapm_widgets)); + + if (np) { + ret = snd_soc_of_parse_audio_routing(codec->card, + "ti,audio-routing"); + if (ret) + return ret; + } + + return 0; +} + /* davinci-evm digital audio interface glue - connects codec <--> CPU */ static struct snd_soc_dai_link dm6446_evm_dai = { .name = "TLV320AIC3X", @@ -326,11 +354,25 @@ static struct snd_soc_dai_link evm_dai_tlv320aic3x = { SND_SOC_DAIFMT_IB_NF, }; +static struct snd_soc_dai_link evm_dai_tlv320aic3111 = { + .name = "TLV320AIC3111", + .stream_name = "AIC3111", + .codec_dai_name = "tlv320aic31xx-hifi", + .ops = &evm_ops, + .init = evm_aic31xx_init, + .dai_fmt = (SND_SOC_DAIFMT_CBM_CFM | SND_SOC_DAIFMT_DSP_B | + SND_SOC_DAIFMT_IB_NF), +}; + static const struct of_device_id davinci_evm_dt_ids[] = { { .compatible = "ti,da830-evm-audio", .data = (void *) &evm_dai_tlv320aic3x, }, + { + .compatible = "ti,am43xx-epos-evm-audio", + .data = &evm_dai_tlv320aic3111, + }, { /* sentinel */ } }; MODULE_DEVICE_TABLE(of, davinci_evm_dt_ids);
Add machine driver support for AM43xx-ePOS-EVM and update associated device tree binding document. Signed-off-by: Jyri Sarha <jsarha@ti.com> --- .../bindings/sound/davinci-evm-audio.txt | 9 +++-- sound/soc/davinci/davinci-evm.c | 42 ++++++++++++++++++++ 2 files changed, 48 insertions(+), 3 deletions(-)