> > +static int rt1015_set_tdm_slot(struct snd_soc_dai *dai,
> > + unsigned int tx_mask, unsigned int rx_mask, int slots, int
> > +slot_width) {
> > + struct snd_soc_component *component = dai->component;
> > + unsigned int val = 0, rx_slotnum, tx_slotnum;
> > + int ret = 0, first_bit;
> > +
> > + switch (slots) {
> > + case 4:
> > + val |= RT1015_I2S_TX_4CH;
> > + break;
> > + case 6:
> > + val |= RT1015_I2S_TX_6CH;
> > + break;
> > + case 8:
> > + val |= RT1015_I2S_TX_8CH;
> > + break;
> > + case 2:
> > + break;
>
> nit-pick: I would put the case 2 first to keep the order. I thought for a second
> this was an error case due to the discontinuity.
OK, will fix that.
> > + default:
> > + ret = -EINVAL;
> > + goto _set_tdm_err_;
> > + }
> > +
> > + switch (slot_width) {
> > + case 20:
> > + val |= RT1015_I2S_CH_TX_LEN_20B;
> > + break;
> > + case 24:
> > + val |= RT1015_I2S_CH_TX_LEN_24B;
> > + break;
> > + case 32:
> > + val |= RT1015_I2S_CH_TX_LEN_32B;
> > + break;
> > + case 16:
> > + break;
>
> nit-pick: same here, I would put 16 first.
>
> > + default:
> > + ret = -EINVAL;
> > + goto _set_tdm_err_;
> > + }
> > +
> > + /* Rx slot configuration */
> > + rx_slotnum = hweight_long(rx_mask);
> > + if (rx_slotnum > 1 || !rx_slotnum) {
>
> I am confused here, is this saying you can only have a single channel on RX?
>
> If yes should this be simplified as if (rx_slotnum != 1) ?
Yes, RT1015 is a mono amplifier, so only a single channel will be configured.
I will fix the check condition as your suggestion. Thanks.
> > + ret = -EINVAL;
> > + dev_err(component->dev, "too many rx slots or zero slot\n");
> > + goto _set_tdm_err_;
> > + }
> > +
> > + first_bit = __ffs(rx_mask);
> > + switch (first_bit) {
> > + case 0:
> > + case 2:
> > + case 4:
> > + case 6:
> > + snd_soc_component_update_bits(component,
> > + RT1015_PAD_DRV2, RT1015_MONO_LR_SEL_MASK,
> > + RT1015_MONO_L_CHANNEL);
> > + snd_soc_component_update_bits(component,
> > + RT1015_TDM1_4,
> > + RT1015_TDM_I2S_TX_L_DAC1_1_MASK |
> > + RT1015_TDM_I2S_TX_R_DAC1_1_MASK,
> > + (first_bit << RT1015_TDM_I2S_TX_L_DAC1_1_SFT) |
> > + ((first_bit+1) << RT1015_TDM_I2S_TX_R_DAC1_1_SFT));
>
> looks like there's an assumption that the rx mask has contiguous bits set?
> Maybe add a comment to explain how the RX path works?
Yes. In the normal case, the system will send the stereo audio to the codec.
I assume that the stereo audio is placed in slot 0/2/4/6.
I will add a comment above this switch case.
> > + break;
> > + case 1:
> > + case 3:
> > + case 5:
> > + case 7:
> > + snd_soc_component_update_bits(component,
> > + RT1015_PAD_DRV2, RT1015_MONO_LR_SEL_MASK,
> > + RT1015_MONO_R_CHANNEL);
> > + snd_soc_component_update_bits(component,
> > + RT1015_TDM1_4,
> > + RT1015_TDM_I2S_TX_L_DAC1_1_MASK |
> > + RT1015_TDM_I2S_TX_R_DAC1_1_MASK,
> > + ((first_bit-1) << RT1015_TDM_I2S_TX_L_DAC1_1_SFT) |
> > + (first_bit << RT1015_TDM_I2S_TX_R_DAC1_1_SFT));
> > + break;
