[alsa-devel] [PATCH v2] ASoC: imx-wm8958: add imx-wm8958 machine driver

[Nicolin Chen]

It looks like you replied me off the maillist. So I just add
the maillist and related TO/CCs back.

On Tue, Mar 01, 2016 at 05:43:22AM +0000, Zidan Wang wrote:
> On Fri, Feb 26, 2016 at 11:42:42AM +0800, Zidan Wang wrote:
> > This is the initial imx-wm8958 device-tree-only machine driver working 
> > with fsl_sai driver. This sound card has three dai link, HIFI, VOICE 
> > and BT dai. HIFI dai link will support codec master and slave mode.
> > VOICE an BT dai link have dummy cpu dai, and just support codec master 
> > mode.
> 
> Why VOICE and BT are using dummy cpu dai? Where can I find the schematics
> for wm8958 from NXP website?
> [Wang Zidan-B50113] I have attached the schematics for wm8958. In our
> release, we can't support VOICE and BT, and just using AIF1. But in
> order to upstream, I add the voice and bt dai.

The way to support the other two AIFs are not very graceful -- looks
like you will have to hard code the CPU DAIs in the driver.

I personally suggest you to use fsl-asoc-card. Since the Codec IC
provides three DAIs. And in the schematics, you do have three DAI
links. It sounds plausible to me that we might treat these three
DAI links as three different cards. Then use different compatible
names: "imx-audio-wm8958-aif1" and so on. And each one can also
has ASRC back-end support.

If you really have big trouble to integrate it into fsl-asoc-card
(not 100% sure if the 3 cards methodology is totally feasible),  I
will be okay with a dedicated driver for wm8958. But you will need
to refine the DT binding to support three CPU DAIs in my opinion.

> > +static u32 imx_wm8958_rates[] = { 8000, 16000, 32000, 48000 };
> > +
> > +static struct snd_pcm_hw_constraint_list imx_wm8958_rate_constraints = {
> > +	.count = ARRAY_SIZE(imx_wm8958_rates),
> > +	.list = imx_wm8958_rates,
> > +};
> 
> Where are these rate constraints coming from?
> [Wang Zidan-B50113] these rates are restricted by the SAI MCLK.

If the SAI MCLK runs at a different rate, it would be invalid.
So there should be at least two constraints depending on the
MCLK rate.

And I think it might be better to have this in the SAI driver
as a refinement since the current driver only errors out for
those unsupported rates.

> > +	if (id == HIFI_DAI) {
> > +		/*
> > +		 * Set GPIO1 pin function to reserve, so that DAC1 and ADC1
> > +		 * using shared LRCLK from DACLRCK1.
> > +		 */
> > +		snd_soc_update_bits(codec, WM8994_GPIO_1, 0x1f, 0x2);
> 
> I don't see any reverse operation against this GPIO configuration here.
> So is it really an operation that has to be done every hw_params()?
> 
> > +	} else if (id == VOICE_DAI) {
> > +		/*
> > +		 * Set GPIO6 pin function to reserve, so that DAC2 and ADC2
> > +		 * using shared LRCLK from DACLRCK2.
> > +		 */
> > +		snd_soc_update_bits(codec, WM8994_GPIO_6, 0x1f, 0x2);
> 
> Same here.

Actually I just took a look at the DT binding doc of WM8994.
It supports GPIO configurations in the codec DT nodes. So
you may not need to do these over here at all.

> > +static int imx_wm8958_set_bias_level_post(struct snd_soc_card *card,
> > +				     struct snd_soc_dapm_context *dapm,
> > +				     enum snd_soc_bias_level level) {
> > +	switch (level) {
> > +	case SND_SOC_BIAS_OFF:
> > +		if (card->dapm.bias_level == SND_SOC_BIAS_STANDBY)
> > +			for (i = 0; i < WM8958_MCLK_MAX; i++) {
> > +				if (!IS_ERR(data->mclk[i]))
> > +					clk_disable_unprepare(data->mclk[i]);
> 
> The set_bias() funcs are merely en/disabling mclk.
> Would be possible to do it in the codec driver?
> [Wang Zidan-B50113] It can be moved to codec driver,
> but the wm8994 driver can support wm8958 wm8994 wm1811 codec,
> it's hard for me to do like that, and I also can't test them.

Sending a patch to the maillist is a way to let people be aware
of the change and then test it. The driver maintainer will review
it and give you feedback. If it really causes compatible issues,
you can then limit the code for wm8958 only.

> > +static struct snd_soc_dai_link imx_wm8958_dai_link[] = {
> > +	[BT_DAI] =	{
> > +			.name = "Bluetooth",
> > +			.stream_name = "Bluetooth",
> > +			.cpu_dai_name = "snd-soc-dummy-dai",
> > +			.codec_name = "wm8994-codec",
> > +			.codec_dai_name = "wm8994-aif3",
> > +			.platform_name = "snd-soc-dummy",
> > +			.ignore_pmdown_time = 1,
> > +	},
> 
> A little curious...how is the BT gonna work for you?
> [Wang Zidan-B50113] I have not test the BT dai. The BT is directly
> connect to AIF3, when AIF3 receive date from BT, it will route the
> date to AIF1 or AIF2. And the date from AIF1 and AIF2 can also route
> to AIF3 to BT. So we just need to set the amixer setting to open the
> audio route.

Sounds interesting. So the Codec totally becomes a router. If
this design can not be supported by using three cards via the
fsl-asoc-card, you may continue this driver as I can see the
situation here. But I encourage you to try that first so as to
avoid duplicated code/driver.