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