Re: [PATCH 2/4] soc-dai: add bitfields for hardware I2S formats

[Daniel Mack <daniel@caiaq.de>]

Hi Mark,

On Wed, Mar 04, 2009 at 10:30:58PM +0000, Mark Brown wrote:
> On Wed, Mar 04, 2009 at 09:16:58PM +0100, Daniel Mack wrote:
> > This patch adds bitfields for I2S serial formats that differ from the
> > amount of data actually sent on the line. Some codecs (namely the
> > cs4270) require 64 sysclks being sent during each frame period, even
> > though the number of acutal data bits might be less.
> 
> Hrm.  This is normally handled via the clock divider interface - it's
> often much more straightforward to work with when dealing with devices
> with very flexible clocking, especially if there are more than two
> devices on the link.  Have you considered handling this through that,
> perhaps through adding a virtual thing to configure (eg, a
> PXA_SSP_FRAME_CLOCKS)?

I thought about that but then I condidered it's actually the wrong place
for such a setting. I see two statements the board file has to make:

1) "We need to have 64 bits per frame for the codec"
2) "The sample data you'll be finding in the FIFO is 16 bits"

It could be possible to express that in clk division rations, but it is
actually a lot more straight forward to make that setting once rather
then changing it over and over again.

> Normally this would be a network mode but it
> seems clear that that has some issues on this hardware and an
> alternative solution is required.
> 
> The other downside of this approach is that it makes all existing
> drivers theoretically instabuggy in that they don't reject invalid
> configurations, although that's not such a big issue since it really
> only makes life easier when writing board drivers.

Well, it won't break things - the worst thing that can happen is that
things are silently ignored. That bitfield was meant to be extended,
wasn't it ;)

> I guess cs4270.c ought to be enforcing this if it's added...

cs4270.c would need that flag to be set or fail otherwise. Don't know if
that really makes life easier for board support files that are not
mainline. Want me to do that?

> > +#define SND_SOC_DAIFMT_FF_SAMPLE	(0 << 16)
> > +#define SND_SOC_DAIFMT_FF_I2S_16	(1 << 16)
> > +#define SND_SOC_DAIFMT_FF_I2S_32	(2 << 16)
> 
> FF_SAMPLE should probably be FF_UNSPEC or something to preserve the
> existing semantics.

Could do that, yes.

> FF_I2S should probably be something else since something might need this
> for non-I2S devices - _BIT, perhaps?

For non-I2S devices, we could re-use the same bit space as they will
never appear in the same format word anyway, right?

Anyway, I'm totally open to any other approach, this one just seems most
straight forward :)

Thanks,
Daniel