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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [ALSA User]     [Linux Audio Users]     [Kernel Archive]     [Asterisk PBX]     [Photo Sharing]     [Linux Sound]     [Video 4 Linux]     [Gimp]     [Yosemite News]

  Powered by Linux