On Thu, 2022-03-03 at 16:08 +0100, AngeloGioacchino Del Regno wrote:
> Il 03/03/22 15:10, Jiaxin Yu ha scritto:
> > On Fri, 2022-02-18 at 15:54 +0100, AngeloGioacchino Del Regno
> > wrote:
> > > Il 17/02/22 14:41, Jiaxin Yu ha scritto:
> > > > This patch adds mt8186 tdm dai driver.
> > > >
> > > > Signed-off-by: Jiaxin Yu <jiaxin.yu@xxxxxxxxxxxx>
> > > > ---
> > > > sound/soc/mediatek/mt8186/mt8186-dai-tdm.c | 713
> > > > +++++++++++++++++++++
> > > > 1 file changed, 713 insertions(+)
> > > > create mode 100644 sound/soc/mediatek/mt8186/mt8186-dai-
> > > > tdm.c
> > > >
> > > > diff --git a/sound/soc/mediatek/mt8186/mt8186-dai-tdm.c
> > > > b/sound/soc/mediatek/mt8186/mt8186-dai-tdm.c
> > > > new file mode 100644
> > > > index 000000000000..28dd3661f0e0
> > > > --- /dev/null
> > > > +++ b/sound/soc/mediatek/mt8186/mt8186-dai-tdm.c
> > > > @@ -0,0 +1,713 @@
> > > > +// SPDX-License-Identifier: GPL-2.0
> > > > +//
> > > > +// MediaTek ALSA SoC Audio DAI TDM Control
> > > > +//
> > > > +// Copyright (c) 2022 MediaTek Inc.
> > > > +// Author: Jiaxin Yu <jiaxin.yu@xxxxxxxxxxxx>
> > > > +
>
> ..snip..
>
> > > > +
> > > > +static int mtk_dai_tdm_hw_params(struct snd_pcm_substream
> > > > *substream,
> > > > + struct snd_pcm_hw_params
> > > > *params,
> > > > + struct snd_soc_dai *dai)
> > > > +{
> > > > + struct mtk_base_afe *afe =
> > > > snd_soc_dai_get_drvdata(dai);
> > > > + struct mt8186_afe_private *afe_priv = afe-
> > > > >platform_priv;
> > > > + int tdm_id = dai->id;
> > > > + struct mtk_afe_tdm_priv *tdm_priv = afe_priv-
> > > > >dai_priv[tdm_id];
> > > > + unsigned int tdm_mode = tdm_priv->tdm_mode;
> > > > + unsigned int data_mode = tdm_priv->data_mode;
> > > > + unsigned int rate = params_rate(params);
> > > > + unsigned int channels = params_channels(params);
> > > > + snd_pcm_format_t format = params_format(params);
> > > > + unsigned int bit_width =
> > > > + snd_pcm_format_physical_width(format);
> > > > + unsigned int tdm_channels = (data_mode ==
> > > > TDM_DATA_ONE_PIN) ?
> > > > + get_tdm_ch_per_sdata(tdm_mode, channels) : 2;
> > > > + unsigned int lrck_width =
> > > > + get_tdm_lrck_width(format, tdm_mode);
> > > > + unsigned int tdm_con = 0;
> > > > + bool slave_mode = tdm_priv->slave_mode;
> > > > + bool lrck_inv = tdm_priv->lck_invert;
> > > > + bool bck_inv = tdm_priv->bck_invert;
> > > > + unsigned int ctrl_reg;
> > > > + unsigned int ctrl_mask;
> > > > + unsigned int tran_rate;
> > > > + unsigned int tran_relatch_rate;
> > > > +
> > > > + if (tdm_priv)
> > > > + tdm_priv->rate = rate;
> > > > + else
> > > > + dev_info(afe->dev, "%s(), tdm_priv == NULL",
> > > > __func__);
> > > > +
> > > > + tran_rate = mt8186_rate_transform(afe->dev, rate, dai-
> > > > >id);
> > > > + tran_relatch_rate =
> > > > mt8186_tdm_relatch_rate_transform(afe->dev,
> > > > rate);
> > > > +
> > > > + /* calculate mclk_rate, if not set explicitly */
> > > > + if (!tdm_priv->mclk_rate) {
> > > > + tdm_priv->mclk_rate = rate * tdm_priv-
> > > > >mclk_multiple;
> > > > + mtk_dai_tdm_cal_mclk(afe,
> > > > + tdm_priv,
> > > > + tdm_priv->mclk_rate);
> > > > + }
> > > > +
> > > > + /* ETDM_IN1_CON0 */
> > > > + tdm_con |= slave_mode <<
> > > > ETDM_IN1_CON0_REG_SLAVE_MODE_SFT;
> > > > + tdm_con |= tdm_mode << ETDM_IN1_CON0_REG_FMT_SFT;
> > > > + tdm_con |= (bit_width - 1) <<
> > > > ETDM_IN1_CON0_REG_BIT_LENGTH_SFT;
> > > > + tdm_con |= (bit_width - 1) <<
> > > > ETDM_IN1_CON0_REG_WORD_LENGTH_SFT;
> > > > + tdm_con |= (tdm_channels - 1) <<
> > > > ETDM_IN1_CON0_REG_CH_NUM_SFT;
> > > > + /* default disable sync mode */
> > > > + tdm_con |= 0 << ETDM_IN1_CON0_REG_SYNC_MODE_SFT;
> > >
> > > 0 << (anything) == 0
> > >
> > > (number |= 0) == number
> > >
> > > Is this a mistake, or are you really doing nothing here?
> > >
> >
> > No, this is just to emphasize the need to set this bit to 0.
> > It really do nothing here, just link a reminder.
> > Can I keep this sentence?
>
> If, in your judgement, it is very important to have a reminder about
> that
> bit having to be unset, then add a comment in the code saying so.
> Don't simply comment out the statement as it is.
>
> A good way would be something like
> /* sync mode bit has to be unset because this that reason, otherwise
> X happens */
I see, thanks for your kind advise.
>
> > >
> > > > + /* relatch fix to h26m */
> > > > + tdm_con |= 0 <<
> > > > ETDM_IN1_CON0_REG_RELATCH_1X_EN_SEL_DOMAIN_SFT;
> > > > +
> > > > + ctrl_reg = ETDM_IN1_CON0;
> > > > + ctrl_mask = ETDM_IN_CON0_CTRL_MASK;
> > > > + regmap_update_bits(afe->regmap, ctrl_reg, ctrl_mask,
> > > > tdm_con);
> > > > +
> > > > + /* ETDM_IN1_CON1 */
> > > > + tdm_con = 0;
> > > > + tdm_con |= 0 << ETDM_IN1_CON1_REG_LRCK_AUTO_MODE_SFT;
> > > > + tdm_con |= 1 << ETDM_IN1_CON1_PINMUX_MCLK_CTRL_OE_SFT;
> > > > + tdm_con |= (lrck_width - 1) <<
> > > > ETDM_IN1_CON1_REG_LRCK_WIDTH_SFT;
> > > > +
> > > > + ctrl_reg = ETDM_IN1_CON1;
> > > > + ctrl_mask = ETDM_IN_CON1_CTRL_MASK;
> > > > + regmap_update_bits(afe->regmap, ctrl_reg, ctrl_mask,
> > > > tdm_con);
> > >
> > > You don't need the ctrl_reg, nor ctrl_mask variables...
> >
> > I was trying to avoid a line of more than 80 words, so I shortened
> > the
> > number of words through variables.
> >
>
> Yes, I know, I did understand what you were trying to do...
> ...but it's fine to go past 80: in this case this would be 88
> columns,
> which is still ok to have!
>
> And note, this is the case with all of similar calls present in this
> function,
> that's why I said that you don't need these two variables! :)
>
> Thank you,
> Angelo
Ok, I got it. This function will be corrected in the next version.
Thank you.
Jiaxin.Yu
>
>
