lkml.org 
[lkml]   [2023]   [Jan]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [PATCH v4 05/13] ASoC: mediatek: mt8188: support etdm in platform driver
    Date
    On Mon, 2023-01-02 at 15:04 +0100, AngeloGioacchino Del Regno wrote:
    > Il 30/12/22 06:54, Trevor Wu ha scritto:
    > > Add mt8188 etdm dai driver support.
    > >
    > > Signed-off-by: Trevor Wu <trevor.wu@mediatek.com>
    > > ---
    > > sound/soc/mediatek/mt8188/mt8188-dai-etdm.c | 2591
    > > +++++++++++++++++++
    > > 1 file changed, 2591 insertions(+)
    > > create mode 100644 sound/soc/mediatek/mt8188/mt8188-dai-etdm.c
    > >
    > > diff --git a/sound/soc/mediatek/mt8188/mt8188-dai-etdm.c
    > > b/sound/soc/mediatek/mt8188/mt8188-dai-etdm.c
    > > new file mode 100644
    > > index 000000000000..f8e69d655706
    > > --- /dev/null
    > > +++ b/sound/soc/mediatek/mt8188/mt8188-dai-etdm.c
    > > @@ -0,0 +1,2591 @@
    > > +// SPDX-License-Identifier: GPL-2.0
    > > +/*
    > > + * MediaTek ALSA SoC Audio DAI eTDM Control
    > > + *
    > > + * Copyright (c) 2022 MediaTek Inc.
    > > + * Author: Bicycle Tsai <bicycle.tsai@mediatek.com>
    > > + * Trevor Wu <trevor.wu@mediatek.com>
    > > + * Chun-Chia Chiu <chun-chia.chiu@mediatek.com>
    > > + */
    > > +
    >
    > ..snip..
    >
    > > +
    > > +static int mtk_dai_etdm_get_clkdiv_id_by_dai_id(int dai_id)
    > > +{
    > > + int clk_id = -1;
    > > +
    > > + switch (dai_id) {
    > > + case MT8188_AFE_IO_DPTX:
    > > + clk_id = MT8188_CLK_TOP_APLL12_DIV9;
    >
    > return MT8188_CLK_TOP_APLL12_DIV_9;
    >
    > > + break;
    > > + case MT8188_AFE_IO_ETDM1_IN:
    > > + clk_id = MT8188_CLK_TOP_APLL12_DIV0;
    >
    > return MT8188_CLK_TOP_APLL12_DIV0;
    >
    > > + break;
    > > + case MT8188_AFE_IO_ETDM2_IN:
    > > + clk_id = MT8188_CLK_TOP_APLL12_DIV1;
    >
    > return .....
    >
    > > + break;
    > > + case MT8188_AFE_IO_ETDM1_OUT:
    > > + clk_id = MT8188_CLK_TOP_APLL12_DIV2;
    > > + break;
    > > + case MT8188_AFE_IO_ETDM2_OUT:
    > > + clk_id = MT8188_CLK_TOP_APLL12_DIV3;
    > > + break;
    > > + case MT8188_AFE_IO_ETDM3_OUT:
    > > + default:
    > > + break;
    > > + }
    > > +
    > > + return clk_id;
    >
    > return -EINVAL;
    >
    > ...so you don't need clk_id at all :-)
    >
    > > +}
    > > +
    >
    > ..snip..
    >
    > > +
    > > +static int mtk_dai_etdm_mclk_configure(struct mtk_base_afe *afe,
    > > int dai_id)
    > > +{
    > > + struct mt8188_afe_private *afe_priv = afe->platform_priv;
    > > + struct mtk_dai_etdm_priv *etdm_data;
    > > + int clk_id = mtk_dai_etdm_get_clk_id_by_dai_id(dai_id);
    > > + int clkdiv_id = mtk_dai_etdm_get_clkdiv_id_by_dai_id(dai_id);
    > > + int apll;
    > > + int apll_clk_id;
    > > + struct etdm_con_reg etdm_reg;
    > > + int ret = 0;
    > > +
    > > + if (clk_id < 0 || clkdiv_id < 0)
    > > + return 0;
    > > +
    > > + if (!is_valid_etdm_dai(dai_id))
    > > + return -EINVAL;
    > > + etdm_data = afe_priv->dai_priv[dai_id];
    > > +
    > > + ret = get_etdm_reg(dai_id, &etdm_reg);
    > > + if (ret < 0)
    > > + return ret;
    > > +
    > > + if (etdm_data->mclk_dir == SND_SOC_CLOCK_OUT)
    > > + regmap_set_bits(afe->regmap, etdm_reg.con1,
    > > + ETDM_CON1_MCLK_OUTPUT);
    > > + else
    > > + regmap_clear_bits(afe->regmap, etdm_reg.con1,
    > > + ETDM_CON1_MCLK_OUTPUT);
    > > +
    > > + if (etdm_data->mclk_freq) {
    > > + apll = etdm_data->mclk_apll;
    > > + apll_clk_id = mt8188_afe_get_mclk_source_clk_id(apll);
    > > + if (apll_clk_id < 0)
    > > + return apll_clk_id;
    > > +
    > > + /* select apll */
    > > + ret = mt8188_afe_set_clk_parent(afe, afe_priv-
    > > >clk[clk_id],
    > > + afe_priv-
    > > >clk[apll_clk_id]);
    > > + if (ret)
    > > + return ret;
    > > +
    > > + /* set rate */
    > > + ret = mt8188_afe_set_clk_rate(afe, afe_priv-
    > > >clk[clkdiv_id],
    > > + etdm_data->mclk_freq);
    >
    > Check ret here...
    >
    > > + } else {
    > > + if (etdm_data->mclk_dir == SND_SOC_CLOCK_OUT)
    > > + dev_dbg(afe->dev, "%s mclk freq = 0\n",
    > > __func__);
    > > + }
    > > +
    >
    > ...and return 0 here.
    >
    > > + return ret;
    > > +}
    > > +
    >
    > ..snip..
    >
    > > +
    > > +static int mtk_dai_etdm_hw_params(struct snd_pcm_substream
    > > *substream,
    > > + struct snd_pcm_hw_params *params,
    > > + struct snd_soc_dai *dai)
    > > +{
    > > + int ret = 0;
    > > + unsigned int rate = params_rate(params);
    > > + unsigned int bit_width = params_width(params);
    > > + unsigned int channels = params_channels(params);
    > > + struct mtk_base_afe *afe = snd_soc_dai_get_drvdata(dai);
    > > + struct mt8188_afe_private *afe_priv = afe->platform_priv;
    > > + struct mtk_dai_etdm_priv *mst_etdm_data;
    > > + int mst_dai_id;
    > > + int slv_dai_id;
    > > + int i;
    > > +
    > > + dev_dbg(afe->dev, "%s '%s' period %u-%u\n",
    > > + __func__, snd_pcm_stream_str(substream),
    > > + params_period_size(params), params_periods(params));
    > > +
    > > + if (is_cowork_mode(dai)) {
    > > + mst_dai_id = get_etdm_cowork_master_id(dai);
    > > + if (!is_valid_etdm_dai(mst_dai_id))
    > > + return -EINVAL;
    > > +
    > > + ret = mtk_dai_etdm_mclk_configure(afe, mst_dai_id);
    > > + if (ret)
    > > + return ret;
    > > +
    > > + ret = mtk_dai_etdm_configure(afe, rate, channels,
    > > + bit_width, mst_dai_id);
    > > + if (ret)
    > > + return ret;
    > > +
    > > + mst_etdm_data = afe_priv->dai_priv[mst_dai_id];
    > > + for (i = 0; i < mst_etdm_data->cowork_slv_count; i++) {
    > > + slv_dai_id = mst_etdm_data->cowork_slv_id[i];
    > > + ret = mtk_dai_etdm_configure(afe, rate,
    > > channels,
    > > + bit_width,
    > > slv_dai_id);
    > > + if (ret)
    > > + return ret;
    > > +
    > > + ret = mt8188_etdm_sync_mode_configure(afe,
    > > slv_dai_id);
    > > + if (ret)
    > > + return ret;
    > > + }
    > > + } else {
    > > + ret = mtk_dai_etdm_mclk_configure(afe, dai->id);
    > > + if (ret)
    > > + return ret;
    > > +
    > > + ret = mtk_dai_etdm_configure(afe, rate, channels,
    > > + bit_width, dai->id);
    >
    > Check return value here...
    >
    > > + }
    > > +
    >
    > ...and return 0 here.
    >
    > > + return ret;
    > > +}
    > > +
    > > +static int mtk_dai_etdm_prepare(struct snd_pcm_substream
    > > *substream,
    > > + struct snd_soc_dai *dai)
    > > +{
    > > + struct mtk_base_afe *afe = snd_soc_dai_get_drvdata(dai);
    > > + struct mt8188_afe_private *afe_priv = afe->platform_priv;
    > > + struct mtk_dai_etdm_priv *mst_etdm_data;
    > > + int mst_dai_id;
    > > + int slv_dai_id;
    > > + int i;
    > > + int ret = 0;
    > > +
    > > + if (!is_valid_etdm_dai(dai->id))
    > > + return -EINVAL;
    > > + mst_etdm_data = afe_priv->dai_priv[dai->id];
    > > +
    > > + dev_dbg(afe->dev, "%s(), dai id %d, prepared %d\n", __func__,
    > > dai->id,
    > > + mst_etdm_data->is_prepared);
    > > +
    > > + if (mst_etdm_data->is_prepared)
    > > + return 0;
    > > +
    > > + mst_etdm_data->is_prepared = true;
    > > +
    > > + if (is_cowork_mode(dai)) {
    > > + mst_dai_id = get_etdm_cowork_master_id(dai);
    > > + if (!is_valid_etdm_dai(mst_dai_id))
    > > + return -EINVAL;
    > > + mst_etdm_data = afe_priv->dai_priv[mst_dai_id];
    > > +
    > > + for (i = 0; i < mst_etdm_data->cowork_slv_count; i++) {
    > > + slv_dai_id = mst_etdm_data->cowork_slv_id[i];
    > > + ret |= mt8188_afe_enable_etdm(afe, slv_dai_id);
    > > + }
    > > +
    > > + ret |= mt8188_afe_enable_etdm(afe, mst_dai_id);
    >
    > OR'ing return values is not an acceptable practice: you should check
    > for error
    > right after the call and immediately take action if anything wrong
    > happened.
    >
    > > + } else {
    > > + ret = mt8188_afe_enable_etdm(afe, dai->id);
    >
    > Same comment as the previous function: check ret here.
    >
    > > + }
    > > +
    > > + return ret;
    > > +}
    > > +
    >
    > ..snip..
    >
    > > +
    > > +static int mtk_dai_hdmitx_dptx_prepare(struct snd_pcm_substream
    > > *substream,
    > > + struct snd_soc_dai *dai)
    > > +{
    > > + struct mtk_base_afe *afe = snd_soc_dai_get_drvdata(dai);
    > > + struct mt8188_afe_private *afe_priv = afe->platform_priv;
    > > + struct mtk_dai_etdm_priv *etdm_data;
    > > + int ret;
    > > +
    > > + if (!is_valid_etdm_dai(dai->id))
    > > + return -EINVAL;
    > > + etdm_data = afe_priv->dai_priv[dai->id];
    > > +
    > > + dev_dbg(afe->dev, "%s(), dai id %d, prepared %d\n", __func__,
    > > dai->id,
    > > + etdm_data->is_prepared);
    > > +
    > > + if (etdm_data->is_prepared)
    > > + return 0;
    > > +
    > > + etdm_data->is_prepared = true;
    > > +
    > > + /* enable dptx interface */
    > > + if (dai->id == MT8188_AFE_IO_DPTX)
    > > + regmap_set_bits(afe->regmap, AFE_DPTX_CON,
    > > AFE_DPTX_CON_ON);
    > > +
    > > + /* enable etdm_out3 */
    > > + ret = mt8188_afe_enable_etdm(afe, dai->id);
    >
    > You don't need the `ret` variable here...
    >
    > return mt8188_afe_enable_etdm(afe, dai->id);
    >
    > > +
    > > + return ret;
    > > +}
    > > +
    >
    > Apart from this, looks good.
    >
    > Regards,
    > Angelo
    >

    Hi Angelo,

    Thanks for your review.
    I'll adopt all suggestions in v5.

    Thanks,
    Trevor
    \
     
     \ /
      Last update: 2023-03-26 23:25    [W:2.297 / U:0.080 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site