by Trevor Wu (吳文良)

[permalink] [raw]

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 <[email protected]>
> > ---
> > 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 <[email protected]>
> > + * Trevor Wu <[email protected]>
> > + * Chun-Chia Chiu <[email protected]>
> > + */
> > +
>
> ..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

2023-01-03 07:28:50

by Trevor Wu (吳文良)

[permalink] [raw]

Subject: Re: [PATCH v4 13/13] dt-bindings: mediatek: mt8188: add mt8188-mt6359 document

On Mon, 2023-01-02 at 17:32 +0100, Krzysztof Kozlowski wrote:
> On 30/12/2022 06:54, Trevor Wu wrote:
> > Add document for mt8188 board with mt6359.
>
> Use subject prefixes matching the subsystem (which you can get for
> example with `git log --oneline -- DIRECTORY_OR_FILE` on the
> directory
> your patch is touching).
>
Hi Krzysztof,

I'm sorry, and I'll add "ASoC" prefix in v5.

> >
> > Signed-off-by: Trevor Wu <[email protected]>
> > ---
> > .../sound/mediatek,mt8188-mt6359.yaml | 93
> > +++++++++++++++++++
> > 1 file changed, 93 insertions(+)
> > create mode 100644
> > Documentation/devicetree/bindings/sound/mediatek,mt8188-mt6359.yaml
> >
> > diff --git
> > a/Documentation/devicetree/bindings/sound/mediatek,mt8188-
> > mt6359.yaml
> > b/Documentation/devicetree/bindings/sound/mediatek,mt8188-
> > mt6359.yaml
> > new file mode 100644
> > index 000000000000..5754c1d460db
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/sound/mediatek,mt8188-
> > mt6359.yaml
> > @@ -0,0 +1,93 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id:
> > https://urldefense.com/v3/__http://devicetree.org/schemas/sound/mediatek,mt8188-mt6359.yaml*__;Iw!!CTRNKA9wMg0ARbw!nY7vxevqo8ILGHUn3JesshmMbBP7WhMOLpwDruvd_r_YINu99BUXrajVZU9xY2zEGFUXxtmrBOofaabZqwt22P8zOMZtkA$
> >
> > +$schema:
> > https://urldefense.com/v3/__http://devicetree.org/meta-schemas/core.yaml*__;Iw!!CTRNKA9wMg0ARbw!nY7vxevqo8ILGHUn3JesshmMbBP7WhMOLpwDruvd_r_YINu99BUXrajVZU9xY2zEGFUXxtmrBOofaabZqwt22P9Ehlwj2w$
> >
> > +
> > +title: MediaTek MT8188 ASoC sound card
> > +
> > +maintainers:
> > + - Trevor Wu <[email protected]>
> > +
> > +properties:
> > + compatible:
> > + const: mediatek,mt8188-mt6359-evb
> > +
> > + model:
> > + $ref: /schemas/types.yaml#/definitions/string
> > + description: User specified audio sound card name
> > +
> > + audio-routing:
> > + $ref: /schemas/types.yaml#/definitions/non-unique-string-array
> > + description:
> > + A list of the connections between audio components. Each
> > entry is a
> > + sink/source pair of strings. Valid names could be the input
> > or output
> > + widgets of audio components, power supplies, MicBias of
> > codec and the
> > + software switch.
> > +
> > + mediatek,platform:
> > + $ref: /schemas/types.yaml#/definitions/phandle
> > + description: The phandle of MT8188 ASoC platform.
> > +
> > +patternProperties:
> > + "^dai-link-[0-9]+$":
> > + type: object
>
> On this level:
> additionalProperties: false
>
OK. I'll add it.

> > + description: |
> > + Container for dai-link level properties and CODEC sub-nodes.
> > +
> > + properties:
> > + dai-link-name:
>
> Instead:
> link-name

OK. I'll replace it in v5.

> > + description: |
> > + This property corresponds to the name of the BE dai-link
> > to which
> > + we are going to update parameters in this node.
> > + items:
> > + enum:
> > + - ADDA_BE
> > + - DPTX_BE
> > + - ETDM1_IN_BE
> > + - ETDM2_IN_BE
> > + - ETDM1_OUT_BE
> > + - ETDM2_OUT_BE
> > + - ETDM3_OUT_BE
> > + - PCM1_BE
> > +
> > + codec:
> > + description: Holds subnode which indicates codec dai.
> > + type: object
> > + additionalProperties: false
> > + properties:
> > + sound-dai:
> > + minItems: 1
> > + maxItems: 2
>
> required sound-dai
>
Thanks, I will add it in v5.

> > +
> > + required:
> > + - dai-link-name
> > + - codec
> > +
> > +additionalProperties: false
> > +
> > +required:
> > + - compatible
> > + - mediatek,platform
> > +
>
> Your example looks pretty straightforward. Maybe you can use
> simple-card.yaml?
>

There is a major difference between simple card and MediaTek sound
card.
In MediaTek sound card, all supported dai-links are listed in the
machine driver, so the dai-link property is only used to introduce the
connected codec nodes.
On the other hand, we hope to inherit the implementation of mt8195 in
which one machine driver supports both sof and non-sof cards, so we
make use of "mediatek, platform" and "mediatek,adsp" properties.
If I don't misunderstand the implementation of simple card, I have to
specify every platform node by dai-link array in dts file and it's more
complex than the current implementation.

Thanks,
Trevor