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

[Eric Miao]

On Mon, Jun 8, 2009 at 8:12 PM, pHilipp Zabel<philipp.zabel at gmail.com> wrote:
> On Sat, Jun 6, 2009 at 10:12 PM, Mark
> Brown<broonie at opensource.wolfsonmicro.com> wrote:
>> On Wed, Jun 03, 2009 at 08:33:42PM +0800, Eric Miao wrote:
>>
>>> +/* frame size definitions for I2S and Left_J formats - default is
>>> + * 32fs, other possibilities are 48fs, 64fs and 96fs
>>> + */
>>> +#define PXA_SSP_FRM_32FS     (0 << 16)
>>> +#define PXA_SSP_FRM_48FS     (1 << 16)
>>> +#define PXA_SSP_FRM_64FS     (2 << 16)
>>> +#define PXA_SSP_FRM_96FS     (3 << 16)
>>> +#define PXA_SSP_FRM_WIDTH(x) (((((x) >> 16) & 0x3) + 2) << 4)
>>> +
>>
>> I still haven't checked this on Zylonite but I wanted to mention these
>> new DAI format bits just now - as previously discussed I'm not
>> enthusiastic about this.
>
> Agreed. Also, if I had to use this to configure magician with DSP_A
> (right now it says I2S and LEFT_J only, but why limit it to that?),
> I'd need an additional PXA_SSP_FRM_16FS.
>

That shouldn't be a problem - it can just be added.

>> As well as the previous issues I raised with
>> this approach I also meant to mention is that the use of a bitfield will
>> inevitably restrict what can be expressed, causing real problems for
>> some systems.  For example, the WM9081 supports systems using up to 3
>> stereo TDM slots and up to 32 bit samples so could be used in a system
>> which needs a 192fs bit clock.
>>
>> If we did decide to adopt this approach then these defines should be in
>> the ASoC headers rather than private to this driver - apart from
>> anything else, there's every chance that additions to the standard bits
>> could end up colliding with this.
>>
>> I'm still not sure what the best way to handle this is due to the
>> interaction with TDM mode.  The fact that TDM mode really wants to
>> always explicitly specify the frame width in order to allow the sample
>> size in each slot to vary suggests that one option would be to add a
>> sample width parameter to set_tdm_slot() - given the very small number
>> of in tree users it'd cause little disruption.  A set_sample_width()
>> operation could also be added.

Well, the real problem is that it's quite difficult to abstract all
the different formats into a single structure. I'd prefer to have
something specific at the begining.

>
> set_sample_width or set_frame_width?
> I'd prefer a separate operation over a parameter to set_tdm_slot because
> in my setup I just need to force the (SSP) frame width to 16 bits.
> Enabling network mode and calling set_tdm_slot(..,1,1) wouldn't be
> necessary at all, then.

As said, if we can invent something like a single structure
making all formats happy, it will be good. However, before
that's possible, I'd really prefer less API for the formats being
further introduced, so we go no further from that goal.

>
> regards
> Philipp
>



-- 
Cheers
- eric