On Wed, Oct 20, 2010 at 12:13 PM, Mark Brown
<broonie@xxxxxxxxxxxxxxxxxxxxxxxxxxx> 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.
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
