Re: [v2 03/17] ASoC: mediatek: mt8186: support adda in platform driver

[AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>]

Il 05/03/22 11:49, 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 adda dai driver
>>>
>>> Signed-off-by: Jiaxin Yu <jiaxin.yu@mediatek.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@mediatek.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?

https://www.kernel.org/doc/html/latest/process/coding-style.html

Besides, the 80 columns rule was made for old terminals (like VT100 and such)
that supported displaying a maximum of 80 cols... in the modern era, that's
not the case anymore.
It's somewhat preferred if something can fit in 80 columns, but if it does
impact on readability, then you can go over that, up to 100 columns.

scripts/checkpatch.pl was also changed to complain about 100 columns and not
80 anymore...

my $max_line_length = 100;

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/scripts/checkpatch.pl?h=v5.17-rc7#n59

Cheers,
Angelo