Re: [PATCH 1/4] ASoC: pxa-ssp: enhance I2S and add Left_J support

From: Daniel Ribeiro <drwyrm@gmail.com>
To: Mark Brown <broonie@opensource.wolfsonmicro.com>
Cc: alsa-devel@alsa-project.org, Eric Miao <eric.y.miao@gmail.com>,
	linux-arm-kernel <linux-arm-kernel@lists.arm.linux.org.uk>,
	pHilipp Zabel <philipp.zabel@gmail.com>
Subject: Re: [PATCH 1/4] ASoC: pxa-ssp: enhance I2S and add Left_J support
Date: Mon, 08 Jun 2009 14:26:32 -0300	[thread overview]
Message-ID: <1244481992.17327.22.camel@brutus> (raw)
In-Reply-To: <20090608165328.GB14026@rakim.wolfsonmicro.main>

[-- Attachment #1.1: Type: text/plain, Size: 2624 bytes --]

Em Seg, 2009-06-08 às 17:53 +0100, Mark Brown escreveu:
> > But sample wouldn't cover all cases. eg, magician and ezx uses the same
> > sample size, but magician needs 16bit frames and ezx 32bits frames.
> 
> See my reply to Philip - even with frame size his case is going to be
> very hard to cover in a standard fashion since it's not clear what to do
> when your frame clock is run too fast relative to your bit clock.  On
> the receive side a lot of devices are just going to discard incomplete
> stereo frames.

I don't see why. If we provide an API to setup the frame size this is
all magician needs.

If we assume that the standard is frame_size = sample_size * channels,
then there is no need for a frame size API at all. But then magician
case will not be supported correctly.

> Network mode is just a detail of the implementation of the PXA here - it
> should not be visible outside the pxa-ssp driver.  Or to put it another
> way setting a tdm_slot of 1, 1 ought to result in the same behaviour as
> disabling TDM as far as the user is concerned.

But for pxa-ssp this is not the current behaviour.

Currently, the code uses frame_size = sample_size unless the machine
driver explicitly calls set_tdm_slots.

I believe that the correct behaviour would be frame_size = channels *
sample size.

Regarding tdm_slot(1, 1), it does not disable network mode, it setups
DMA to 16bits regardless of 2 channels.

> > Using set_tdm_slot to setup this also seems awkward, because the dma
> > configuration to use is also affected by the frame size.
> 
> I'd not expect it to be?

If we want to support magician, then set_frame_size(16) would also setup
DMA for 16bits even for stereo audio.

> > My suggestion here is to make a clear distinction between normal mode
> > and network mode. We should not abuse SSCR0_MOD when we don't need it.
> > We should not require the machine driver to call set_tdm_slot when it
> > doesn't need to. And for frame sizes that really need SSCR0_MOD to be
> > set we should issue a warning telling that network mode was
> > "automatically" enabled.
> 
> I'd like to see all these details handled within the driver - knowing if
> and when network mode are to be set up is the sort of thing that users
> ought to be able to rely on the driver for.

I can cook a patch.. All I need to know is:

Does it needs to support magician non-standard format? Or this will be
handled by userspace?

I think we can support magician case too, we just need to provide the
set_frame_size api that was initially proposed. :)

-- 
Daniel Ribeiro

[-- Attachment #1.2: Esta é uma parte de mensagem assinada digitalmente --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel