On Fri, Sep 27, 2019 at 04:37:37AM +0530, Ravulapati Vishnu vardhan rao wrote:
RAVEN has multiple I2S instances:BT and SP.But only BT is enabled. Now I2S SP instance also gets enabled with this patch.
This is extremely difficult to review as is, the patch is very large and the description very brief so it's hard to tell exactly what issues there are that must be fixed to enable multiple I2S interfaces. My suggestion would be that this should be split into a number of smaller patches, each making one logical change with a clear description of what that specific change is.
A few specific comments below but really I didn't get very far into the code due to the difficulty figuring out what's going on:
@@ -46,10 +28,10 @@ static int acp3x_i2s_set_fmt(struct snd_soc_dai *cpu_dai, unsigned int fmt)
case SND_SOC_DAIFMT_I2S: adata->tdm_mode = false;
break;
- break; case SND_SOC_DAIFMT_DSP_A:
adata->tdm_mode = true;break;
adata->tdm_mode = true;- break; default: return -EINVAL; }
For example this is a pure formatting change (one that moves things away from the normal Linux coding style) and clearly not related to the changelog.
@@ -87,9 +69,16 @@ static int acp3x_i2s_set_tdm_slot(struct snd_soc_dai *cpu_dai, u32 tx_mask, val = rv_readl(adata->acp3x_base + mmACP_BTTDM_IRER); rv_writel((val | 0x2), adata->acp3x_base + mmACP_BTTDM_IRER);
val = rv_readl(adata->acp3x_base + mmACP_I2STDM_ITER);
rv_writel((val | 0x2), adata->acp3x_base + mmACP_I2STDM_ITER);
val = rv_readl(adata->acp3x_base + mmACP_I2STDM_IRER);
rv_writel((val | 0x2), adata->acp3x_base + mmACP_I2STDM_IRER);
val = (FRM_LEN | (slots << 15) | (slot_len << 18)); rv_writel(val, adata->acp3x_base + mmACP_BTTDM_TXFRMT); rv_writel(val, adata->acp3x_base + mmACP_BTTDM_RXFRMT);
rv_writel(val, adata->acp3x_base + mmACP_I2STDM_TXFRMT);
rv_writel(val, adata->acp3x_base + mmACP_I2STDM_RXFRMT);
adata->tdm_fmt = val; return 0;
Won't this configure all the interfaces identically?