Re: [RFC PATCH 1/2] ASoC: simple-card: add support for bclk_ratio

[Russell King - ARM Linux admin <linux@armlinux.org.uk>]

On Tue, Feb 26, 2019 at 09:35:47AM +0900, Kuninori Morimoto wrote:
> > @@ -97,7 +98,7 @@ static int asoc_simple_card_hw_params(struct snd_pcm_substream *substream,
> >  	struct simple_card_data *priv = snd_soc_card_get_drvdata(rtd->card);
> >  	struct simple_dai_props *dai_props =
> >  		simple_priv_to_props(priv, rtd->num);
> > -	unsigned int mclk, mclk_fs = 0;
> > +	unsigned int mclk, bclk_ratio, mclk_fs = 0, bclk_slot_ratio = 0;
> >  	int ret = 0;
> >  
> >  	if (dai_props->mclk_fs)
> (snip)
> > +	if (bclk_slot_ratio) {
> > +		/* FIXME do we need to care about TDM slots here ? */
> > +		bclk_ratio = snd_soc_calc_frame_size(bclk_slot_ratio,
> > +				params_channels(params), 1);
> > +
> > +		ret = snd_soc_dai_set_bclk_ratio(codec_dai, bclk_ratio);
> > +		if (ret && ret != -ENOTSUPP)
> > +			goto err;
> > +
> > +		ret = snd_soc_dai_set_bclk_ratio(cpu_dai, bclk_ratio);
> > +		if (ret && ret != -ENOTSUPP)
> > +			goto err;
> > +	}
> 
> Not a big deal, but "bclk_ratio" is used only here.
> We can define it here

This code actually requires a lot more thought - while it may solve
Sven's issue, it isn't generic.  So far, I have this table put
together from various sources of information:

			bclk_ratio
sample width	current	mcasp   fslssi  kirkwood
16		32	32	64	64
24		48	48	64	64
32		64	64	64	64

Let's also consider whether it should depend on the number of channels.
I2S uses a WS/LRCLK signal to differentiate each channel and to demark
where the MSB bit is.

If userspace negotiates one channel, what happens - if WS becomes
static, then there is no signal indicating where the MSB bit is in
the stream, so there is no way for a receiver to synchronise.  So,
it is highly unlikely that bclk_ratio = channels * sample_width.

If the signal continues toggling, what does it do for the inactive
channel - is that just one BCLK period long or does it assume there
is still the second channel?  If the former, it means we could end
up with bclk_ratios of 17, 25, 33 which imho is unlikely, and
would mess up TDA998x's CTS measurement.

What about setups where we have multiple I2S data signals (to support
multi-channel audio) - if we have four channels transmitted on two
data lines (as would be required by the TDA998x), that doesn't mean
BCLK becomes sample_width * 4.

So, I don't think the number of channels comes into the bclk_ratio
calculation at all.  Given the above code, it effectively means we'd
always specify channels = 2 to snd_soc_calc_frame_size().

Given that it is normal to talk about I2S being clocked at "64fs",
"32fs" etc, wouldn't it just be much neater to specify _this_ value
in DT, rather than half that value?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel