On Wed, Oct 20, 2010 at 11:22:26AM +0900, Jassi Brar wrote:
On Tue, Oct 19, 2010 at 6:35 PM, Mark Brown
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.
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.
- 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
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.
- /* 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.
+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.
It helps make the code correspond to the expected patterns for selecting one of many clocks which is especially helpful when doing a big picture scan.
+/*
- 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(?)
How would the index be used with multi-component?