Re: [PATCH 11/25] ASoC: Samsung: Add common I2S driver

[Mark Brown <broonie@opensource.wolfsonmicro.com>]

On Wed, Oct 20, 2010 at 11:22:26AM +0900, Jassi Brar wrote:
> On Tue, Oct 19, 2010 at 6:35 PM, Mark Brown

> > Might be as easy just to embed a struct s3c_audio_pdata within the
> > struct samsung_i2s?

> struct s3c_audio_pdata is shared among I2S, AC97, SPDIF and PCM.
> Here the idea is that controller drivers add specific structures to the union
> as and when they need it.

Right, what I'm saying is that it'd seem more natural to do things the
other way around and embed the common structure within a device specific
pdata structure instead of having the union.

> >> +             if (is_secondary(i2s)) {
> >> +                     con |= CON_TXSDMA_ACTIVE;
> >> +                     con &= ~CON_TXSDMA_PAUSE;
> >> +             } else {
> >> +                     con |= CON_TXDMA_ACTIVE;
> >> +                     con &= ~CON_TXDMA_PAUSE;

> > Can we do this stuff with a variable storing the mask to use?  It might
> > compress the code a lot but I've not looked at what the bits actually
> > are.

> sorry, I am unable to understand what you suggest

If we have something in the driver data struct specifying the masks to
use then set these at probe time rather than having the if statements -
probably the same mask can be used for playback & record if the
bitfields are lined up similarly.  This would then be:

	con |= data->active_mask;
	con &= ~data->pause_mask;

or similar, possibly with some shifts.

> >> +     /* Allow LRCLK-inverted version of the supported formats */
> >> +     switch (fmt & SND_SOC_DAIFMT_INV_MASK) {
> >> +     case SND_SOC_DAIFMT_NB_NF:
> >> +             break;
> >> +     case SND_SOC_DAIFMT_NB_IF:
> >> +             if (tmp & MOD_LR_RLOW)
> >> +                     tmp &= ~MOD_LR_RLOW;
> >> +             else
> >> +                     tmp |= MOD_LR_RLOW;
> >> +             break;

> > This looks a bit odd - it'll flip MOD_LR_FLOW if the frame is inverted,
> > I'd expect it to want to set a specific value?

> The frame polarity is specified by the Format(I2S, LSB, MSB), so we
> set/clear MOD_LR_RLOW acc to the Format requested.
> The Inversion request works relative to the Format -- if Frame inversion
> is requested, we simply flip it from the value set during Format setting.

Please add a comment explaining that you're inverting the orientation
you set previously - it's really surprising when reading the code.

> >> +static int i2sv2_i2s_set_clkdiv(struct snd_soc_dai *dai,
> >> +     int div_id, int div)
> >> +{
> >> +     struct i2s_dai *i2s = to_info(dai);
> >> +     struct i2s_dai *other = i2s->pri_dai ? : i2s->sec_dai;
> >> +
> >> +     if (div_id == SAMSUNG_I2S_DIV_BCLK) {

> > switch statement please.

> Switch didn't look very pretty with just one case, so I made it if-else
> Anyways, I'll change it to switch.

It helps make the code correspond to the expected patterns for selecting
one of many clocks which is especially helpful when doing a big picture
scan.

> >> +/*
> >> + * Maximum number of I2S blocks that any SoC can have.
> >> + * The secondary interface of a CPU dai(if there exists any),
> >> + * is indexed at [cpu-dai's ID + MAX_I2S]
> >> + */
> >> +#define MAX_I2S      4

> > Can this be kept internal to the driver?  If not it should be
> > namespaced.

> ASoC machine drivers need to index the secondary dai that is
> automatically created and registered by the cpu driver.
> So, it needs to be available outside.
> I'll make it SAMSUNG_I2S_SECOFF -- secondary offset of I2S(?)

How would the index be used with multi-component?