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

[Jassi Brar]

On Tue, Oct 19, 2010 at 6:35 PM, Mark Brown
<broonie at opensource.wolfsonmicro.com> wrote:
> On Tue, Oct 19, 2010 at 04:07:05PM +0900, Jassi Brar wrote:
>
> This looks good - there's a few comments below but they're pretty small
> things.
>
>>  /**
>>   * struct s3c_audio_pdata - common platform data for audio device drivers
>>   * @cfg_gpio: Callback function to setup mux'ed pins in I2S/PCM/AC97 mode
>>   */
>>  struct s3c_audio_pdata {
>>       int (*cfg_gpio)(struct platform_device *);
>> +     union {
>> +             struct samsung_i2s i2s;
>> +     } type;
>
> 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.


>> +     switch (bfs) {
>> +     case 48:
>> +             mod |= MOD_BCLK_48FS;
>> +             break;
>> +     case 32:
>> +             mod |= MOD_BCLK_32FS;
>> +             break;
>> +     case 24:
>> +             mod |= MOD_BCLK_24FS;
>> +             break;
>> +     default:
>> +             mod |= MOD_BCLK_16FS;
>> +             break;
>
> Especially on the write side I'd be a bit more comfortable if these
> functions didn't silently convert an unspecified setting into a default
> so that we know that users are getting the setting they asked for.

Ok. I'll add a case for 16 and print error in default.



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

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


>> +     /* We don't care about BFS in Slave mode */
>> +     if (is_slave(i2s))
>> +             return 0;
>
> You're skipping more than just rfs setting here.  Probably just a case
> of updating the comment.

Yes, I need to update the comment.


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


>> +#define SAMSUNG_I2S_RATES \
>> +     (SNDRV_PCM_RATE_8000 | SNDRV_PCM_RATE_11025 | SNDRV_PCM_RATE_16000 | \
>> +     SNDRV_PCM_RATE_22050 | SNDRV_PCM_RATE_32000 | SNDRV_PCM_RATE_44100 | \
>> +     SNDRV_PCM_RATE_48000 | SNDRV_PCM_RATE_88200 | SNDRV_PCM_RATE_96000)
>
> SNDRV_PCM_RATE_8000_96000.

of course !

>> +/*
>> + * 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(?)

Thanks.