| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <98252c70856919ef1b5b6716d9fa88d9c0a24c35.camel@mediatek.com>
Date: Tue, 3 Jan 2023 04:06:29 +0000
From: Trevor Wu (吳文良) <Trevor.Wu@...iatek.com>
To: "robh+dt@...nel.org" <robh+dt@...nel.org>,
"broonie@...nel.org" <broonie@...nel.org>,
"p.zabel@...gutronix.de" <p.zabel@...gutronix.de>,
"tiwai@...e.com" <tiwai@...e.com>,
"lgirdwood@...il.com" <lgirdwood@...il.com>,
"krzysztof.kozlowski+dt@...aro.org"
<krzysztof.kozlowski+dt@...aro.org>,
"matthias.bgg@...il.com" <matthias.bgg@...il.com>,
"perex@...ex.cz" <perex@...ex.cz>,
"angelogioacchino.delregno@...labora.com"
<angelogioacchino.delregno@...labora.com>
CC: "linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-mediatek@...ts.infradead.org"
<linux-mediatek@...ts.infradead.org>,
"alsa-devel@...a-project.org" <alsa-devel@...a-project.org>,
Project_Global_Chrome_Upstream_Group
<Project_Global_Chrome_Upstream_Group@...iatek.com>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>
Subject: Re: [PATCH v4 05/13] ASoC: mediatek: mt8188: support etdm in platform
driver
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@...iatek.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@...iatek.com>
> > + * Trevor Wu <trevor.wu@...iatek.com>
> > + * Chun-Chia Chiu <chun-chia.chiu@...iatek.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
Powered by blists - more mailing lists