| 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: <5aca64866e5441d21aaf0293a1e48286acfdd978.camel@mediatek.com>
Date: Sat, 5 Mar 2022 18:49:33 +0800
From: Jiaxin Yu <jiaxin.yu@...iatek.com>
To: AngeloGioacchino Del Regno
<angelogioacchino.delregno@...labora.com>, <broonie@...nel.org>
CC: <lgirdwood@...il.com>, <tiwai@...e.com>, <robh+dt@...nel.org>,
<matthias.bgg@...il.com>, <perex@...ex.cz>,
<p.zabel@...gutronix.de>, <geert+renesas@...der.be>,
<trevor.wu@...iatek.com>, <tzungbi@...gle.com>,
<aaronyu@...gle.com>, <zhangqilong3@...wei.com>,
<alsa-devel@...a-project.org>, <devicetree@...r.kernel.org>,
<linux-arm-kernel@...ts.infradead.org>,
<linux-mediatek@...ts.infradead.org>,
<linux-kernel@...r.kernel.org>
Subject: Re: [v2 03/17] ASoC: mediatek: mt8186: support adda in platform
driver
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 adda dai driver
> >
> > Signed-off-by: Jiaxin Yu <jiaxin.yu@...iatek.com>
> > ---
> > sound/soc/mediatek/mt8186/mt8186-dai-adda.c | 891
> > ++++++++++++++++++++
> > 1 file changed, 891 insertions(+)
> > create mode 100644 sound/soc/mediatek/mt8186/mt8186-dai-adda.c
> >
> > diff --git a/sound/soc/mediatek/mt8186/mt8186-dai-adda.c
> > b/sound/soc/mediatek/mt8186/mt8186-dai-adda.c
> > new file mode 100644
> > index 000000000000..6d7dd1533da0
> > --- /dev/null
> > +++ b/sound/soc/mediatek/mt8186/mt8186-dai-adda.c
> > @@ -0,0 +1,891 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +//
> > +// MediaTek ALSA SoC Audio DAI ADDA Control
> > +//
> > +// Copyright (c) 2022 MediaTek Inc.
> > +// Author: Jiaxin Yu <jiaxin.yu@...iatek.com>
> > +
> > +#include <linux/regmap.h>
> > +#include <linux/delay.h>
> > +#include "mt8186-afe-clk.h"
> > +#include "mt8186-afe-common.h"
> > +#include "mt8186-afe-gpio.h"
> > +#include "mt8186-interconnection.h"
> > +
...snip...
> >
> > +/* dai ops */
> > +static int mtk_dai_adda_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;
> > + unsigned int rate = params_rate(params);
> > + int id = dai->id;
> > + struct mtk_afe_adda_priv *adda_priv = afe_priv->dai_priv[id];
> > +
> > + dev_info(afe->dev, "%s(), id %d, stream %d, rate %d\n",
> > + __func__,
> > + id,
> > + substream->stream,
> > + rate);
> > +
> > + if (!adda_priv) {
> > + dev_info(afe->dev, "%s(), adda_priv == NULL",
> > __func__);
> > + return -EINVAL;
> > + }
> > +
> > + if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
> > + unsigned int dl_src2_con0 = 0;
> > + unsigned int dl_src2_con1 = 0;
>
> This initialization is redundant: you're never using these variables
> before initializing them later, so initializing them to zero is not
> needed here.
Yes, got it. Thank you.
>
> > +
> > + adda_priv->dl_rate = rate;
> > +
> > + /* set sampling rate */
> > + dl_src2_con0 = adda_dl_rate_transform(afe, rate) <<
> > + DL_2_INPUT_MODE_CTL_SFT;
> > +
> > + /* set output mode, UP_SAMPLING_RATE_X8 */
> > + dl_src2_con0 |= (0x3 << DL_2_OUTPUT_SEL_CTL_SFT);
> > +
> > + /* turn off mute function */
> > + dl_src2_con0 |= (0x01 <<
> > DL_2_MUTE_CH2_OFF_CTL_PRE_SFT);
>
> BIT() macro, please
>
> > + dl_src2_con0 |= (0x01 <<
> > DL_2_MUTE_CH1_OFF_CTL_PRE_SFT);
> > +
> > + /* set voice input data if input sample rate is 8k or
> > 16k */
> > + if (rate == 8000 || rate == 16000)
> > + dl_src2_con0 |= 0x01 <<
> > DL_2_VOICE_MODE_CTL_PRE_SFT;
> > +
> > + /* SA suggest apply -0.3db to audio/speech path */
> > + dl_src2_con1 = MTK_AFE_ADDA_DL_GAIN_NORMAL <<
> > + DL_2_GAIN_CTL_PRE_SFT;
> > +
> > + /* turn on down-link gain */
> > + dl_src2_con0 |= (0x01 << DL_2_GAIN_ON_CTL_PRE_SFT);
> > +
> > + if (id == MT8186_DAI_ADDA) {
> > + /* clean predistortion */
> > + regmap_write(afe->regmap, AFE_ADDA_PREDIS_CON0,
> > 0);
> > + regmap_write(afe->regmap, AFE_ADDA_PREDIS_CON1,
> > 0);
> > +
> > + regmap_write(afe->regmap,
> > + AFE_ADDA_DL_SRC2_CON0,
> > dl_src2_con0);
> > + regmap_write(afe->regmap,
> > + AFE_ADDA_DL_SRC2_CON1,
> > dl_src2_con1);
> > +
> > + /* set sdm gain */
> > + regmap_update_bits(afe->regmap,
> > + AFE_ADDA_DL_SDM_DCCOMP_CON,
> > + ATTGAIN_CTL_MASK_SFT,
> > + AUDIO_SDM_LEVEL_NORMAL <<
> > + ATTGAIN_CTL_SFT);
> > +
> > + /* Use new 2nd sdm */
> > + regmap_update_bits(afe->regmap,
> > + AFE_ADDA_DL_SDM_DITHER_CON,
> > + AFE_DL_SDM_DITHER_64TAP_EN_M
> > ASK_SFT,
> > + 0x1 <<
> > AFE_DL_SDM_DITHER_64TAP_EN_SFT);
>
> BIT(AFE_DL_SDM_DITHER_64TAP_EN_SFT)
>
> > + regmap_update_bits(afe->regmap,
> > + AFE_ADDA_DL_SDM_AUTO_RESET_C
> > ON,
> > + AFE_DL_USE_NEW_2ND_SDM_MASK_
> > SFT,
> > + 0x1 <<
> > AFE_DL_USE_NEW_2ND_SDM_SFT);
>
> BIT(AFE_DL_USE_NEW_2ND_SDM_SFT)
>
> > + regmap_update_bits(afe->regmap,
> > + AFE_ADDA_DL_SDM_DCCOMP_CON,
> > + USE_3RD_SDM_MASK_SFT,
> > + AUDIO_SDM_2ND <<
> > USE_3RD_SDM_SFT);
> > +
> > + /* sdm auto reset */
> > + regmap_write(afe->regmap,
> > + AFE_ADDA_DL_SDM_AUTO_RESET_CON,
> > + SDM_AUTO_RESET_THRESHOLD);
> > + regmap_update_bits(afe->regmap,
> > + AFE_ADDA_DL_SDM_AUTO_RESET_C
> > ON,
> > + SDM_AUTO_RESET_TEST_ON_MASK_
> > SFT,
> > + 0x1 <<
> > SDM_AUTO_RESET_TEST_ON_SFT);
>
> BIT(SDM_AUTO_RESET_TEST_ON_SFT)
>
> > + }
> > + } else {
> > + unsigned int voice_mode = 0;
>
> what about...
> unsigned int ul_src_con0 = 0; /* default value */
> unsigned int voice_mode = adda_ul_rate_transform(afe,
> rate);
Agree with you.
> > + unsigned int ul_src_con0 = 0; /* default value */
> > +
> > + adda_priv->ul_rate = rate;
> > +
> > + voice_mode = adda_ul_rate_transform(afe, rate);
> > +
> > + ul_src_con0 |= (voice_mode << 17) & (0x7 << 17);
> > +
> > + /* enable iir */
> > + ul_src_con0 |= (1 << UL_IIR_ON_TMP_CTL_SFT) &
> > + UL_IIR_ON_TMP_CTL_MASK_SFT;
> > + ul_src_con0 |= (UL_IIR_SW << UL_IIRMODE_CTL_SFT) &
> > + UL_IIRMODE_CTL_MASK_SFT;
> > + switch (id) {
> > + case MT8186_DAI_ADDA:
> > + case MT8186_DAI_AP_DMIC:
> > + /* 35Hz @ 48k */
> > + regmap_write(afe->regmap,
> > + AFE_ADDA_IIR_COEF_02_01,
> > 0x00000000);
>
> Please drop leading zeroes:
>
> regmap_write(afe->regmap, AFE_ADDA_IIR_COEF_02_01, 0);
>
> > + regmap_write(afe->regmap,
> > + AFE_ADDA_IIR_COEF_04_03,
> > 0x00003FB8);
>
> ... and also please write hex in lower-case:
>
Got it.
> regmap_write(afe->regmap,
> AFE_ADDA_IIR_COEF_04_03, 0x03fb8);
>
> > + regmap_write(afe->regmap,
> > + AFE_ADDA_IIR_COEF_06_05,
> > 0x3FB80000);
> > + regmap_write(afe->regmap,
> > + AFE_ADDA_IIR_COEF_08_07,
> > 0x3FB80000);
> > + regmap_write(afe->regmap,
> > + AFE_ADDA_IIR_COEF_10_09,
> > 0x0000C048);
> > +
> > + regmap_write(afe->regmap,
> > + AFE_ADDA_UL_SRC_CON0,
> > ul_src_con0);
> > +
> > + /* Using Internal ADC */
> > + regmap_update_bits(afe->regmap,
> > + AFE_ADDA_TOP_CON0,
> > + 0x1 << 0,
> > + 0x0 << 0);
>
> Please use the BIT() macro:
>
> regmap_update_bits(afe->regmap, AFE_ADDA_TOP_CON0, BIT(0), 0);
>
> P.S.: 87 columns is ok
How can I judge whether it can exceed 80 lines?
>
> > +
> > + /* mtkaif_rxif_data_mode = 0, amic */
> > + regmap_update_bits(afe->regmap,
> > + AFE_ADDA_MTKAIF_RX_CFG0,
> > + 0x1 << 0,
> > + 0x0 << 0);
>
> same here.
>
> > + break;
> > + default:
> > + break;
> > + }
> > +
> > + /* ap dmic */
> > + switch (id) {
> > + case MT8186_DAI_AP_DMIC:
> > + mtk_adda_ul_src_dmic(afe, id);
> > + break;
> > + default:
> > + break;
> > + }
> > + }
> > +
> > + return 0;
> > +}
> > +
>
> Regards,
> Angelo
>
Powered by blists - more mailing lists