LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Nicolin Chen <nicoleotsuka@gmail.com>
To: Zidan Wang <b50113@freescale.com>
Cc: Zidan Wang <zidan.wang@freescale.com>,
	timur@tabi.org, Xiubo.Lee@gmail.com, lgirdwood@gmail.com,
	broonie@kernel.org, perex@perex.cz, tiwai@suse.de,
	alsa-devel@alsa-project.org, linuxppc-dev@lists.ozlabs.org,
	linux-kernel@vger.kernel.org
Subject: Re: [alsa-devel][PATCH 2/3] ASoC: fsl_sai: Add support for tdm slots operation
Date: Thu, 22 Jan 2015 15:50:20 -0800	[thread overview]
Message-ID: <20150122235019.GA3204@Asurada-GSX> (raw)
In-Reply-To: <20150122062004.GA7046@b50113>

On Thu, Jan 22, 2015 at 02:20:06PM +0800, Zidan Wang wrote:
> > > > > @@ -369,10 +380,25 @@ static int fsl_sai_hw_params(struct snd_pcm_substream *substream,
> > > > >  	u32 word_width = snd_pcm_format_width(params_format(params));
> > > > >  	u32 val_cr4 = 0, val_cr5 = 0;
> > > > >  	int ret;
> > > > > +	u32 bclk;
> > > > > +
> > > > > +	if (channels == 1)
> > > > > +		channels = 2;
> > > > > +
> > > > > +	if (!sai->slots || sai->slots % channels)
> > > > > +		sai->slots = channels;
> > > > > +
> > > > > +	sai->slots = sai->slots / channels;
> > > > > +
> > > > > +	if (sai->slot_width < word_width || sai->is_dsp_mode)
> > > > > +		sai->slot_width = word_width;
> > > > 
> > > > Could you pls explain a bit what's this overriding for?
> > > > Or it might be better to put into a comment.
> > > >
> > > In machine driver, we should use snd_soc_dai_set_tdm_slot to set slots
> > > and slot width. In my opinion, slots in machine driver means the slot
> > > number of all channels. But in sai driver, the slots means the slot number
> > > of one channel. So snd_soc_calc_bclk use fs*sample_size*channels*tdm_slots
> > > to caculate bclk.
> > 
> > Could you pls use the standard way as you described for slot number
> > in machine driver instead of defining some other meanings? That one
> > is really confusing, not to mention a code without any comment.
> > 
> > You can add some extra local variables with extinguished names and
> > calculate the slot number and channels you want without changing
> > the original sai->slots and channels in the hw_params(). As you said,
> > you only need that new slot number and channels for snd_soc_calc_bclk
> > call. So you may also put this code right before the call -- a normal
> > routine doesn't need to recalculate the special slots and channels,
> > right?

> Actually i don't kown which meaning is the standard way. Because i found
> in wm8993 and wm8904 codec driver, they also use slots/channels to set 
> tdm_slots in set_tdm_slot function.

The slots via machine driver should be total slot number per frame.
For I2S, we're passing 2 for slots parameter in our machine drivers.

So why not just following that pattern. You code for calculating slot
per channel is used to calculate bclk. But keeping slot per frame will
also allow you to do that, meanwhile you can drop some code to do the
extra calculation so as to keep it neat.

I suggest you to take a look at fsl_esai.c and to follow that way:
Override slots and slot_width if specified from machine driver.

And another problem of your patch is the configurations for FRSZ and
xMR fields. For monaural case, the channels == 1, the current code
passes it directly to set them while your code overrides it to 2.

I'm not sure if you've tested the monaural case and confirmed it works.
But, apparently, the configurations look pretty different.

Nicolin

  reply	other threads:[~2015-01-22 23:50 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-01-20 12:21 [alsa-devel][PATCH 0/3] Add master mode, tmd and right-j mode support Zidan Wang
2015-01-20 12:21 ` [alsa-devel][PATCH 1/3] SoC: fsl_sai: add sai master " Zidan Wang
2015-01-21  6:07   ` Nicolin Chen
2015-01-21  9:25     ` Zidan Wang
2015-01-21 17:36       ` Nicolin Chen
2015-01-20 12:21 ` [alsa-devel][PATCH 2/3] ASoC: fsl_sai: Add support for tdm slots operation Zidan Wang
2015-01-21 18:08   ` Nicolin Chen
2015-01-22  4:55     ` Zidan Wang
2015-01-22  5:44       ` Nicolin Chen
2015-01-22  6:20         ` Zidan Wang
2015-01-22 23:50           ` Nicolin Chen [this message]
2015-01-23  2:55             ` Zidan Wang
2015-01-23  8:27               ` Nicolin Chen
2015-01-20 12:21 ` [alsa-devel][PATCH 3/3] ASoC: fsl_sai: Add support for Right-J mode Zidan Wang
2015-01-21 18:53   ` Nicolin Chen
2015-01-22  5:13     ` Zidan Wang
2015-01-22  5:46       ` Nicolin Chen
2015-01-21  1:15 ` [alsa-devel] [PATCH 0/3] Add master mode, tmd and right-j mode support Fabio Estevam
2015-01-21  6:29   ` Nicolin Chen

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20150122235019.GA3204@Asurada-GSX \
    --to=nicoleotsuka@gmail.com \
    --cc=Xiubo.Lee@gmail.com \
    --cc=alsa-devel@alsa-project.org \
    --cc=b50113@freescale.com \
    --cc=broonie@kernel.org \
    --cc=lgirdwood@gmail.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linuxppc-dev@lists.ozlabs.org \
    --cc=perex@perex.cz \
    --cc=timur@tabi.org \
    --cc=tiwai@suse.de \
    --cc=zidan.wang@freescale.com \
    --subject='Re: [alsa-devel][PATCH 2/3] ASoC: fsl_sai: Add support for tdm slots operation' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).