[alsa-devel] [PATCH 11/25] ASoC: Samsung: Add common I2S driver

[Jassi Brar]

On Wed, Oct 20, 2010 at 12:13 PM, Mark Brown
<broonie at opensource.wolfsonmicro.com> wrote:

>> 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.

Ok, will do it.


>> > 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.

Let me see if the space saved is worth the complication.


>> >> +     /* 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.

Ok, I'll add a comment.
Btw, isn't it the standard way? Don't other I2S CPU drivers do the same thing ?


>> 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?

For example, please look at [Patch 23/25]

+               /* Secondary is at offset MAX_I2S from Primary */
+               str = (char *)smdk_dai[SEC_PLAYBACK].cpu_dai_name;
+               str[strlen(str) - 1] = '0' + MAX_I2S;


Thanks.