On Thu, 2023-06-08 at 12:07 +0200, AngeloGioacchino Del Regno wrote:
External email : Please do not click links or open attachments until you have verified the sender or the content. Il 08/06/23 12:03, Trevor Wu (吳文良) ha scritto:
On Thu, 2023-06-08 at 10:47 +0200, AngeloGioacchino Del Regno
wrote:
Replace open coded instances of FIELD_GET() with it, move
register
definitions at the top of the file and also replace magic numbers with register definitions.
While at it, also change a regmap_update_bits() call to regmap_write() because the top 29 bits of AUD_TOP_CFG (31:3) are reserved
(unused).
Signed-off-by: AngeloGioacchino Del Regno < angelogioacchino.delregno@collabora.com>
sound/soc/mediatek/mt8188/mt8188-mt6359.c | 32 ++++++++++++++---
-- 1 file changed, 19 insertions(+), 13 deletions(-)
diff --git a/sound/soc/mediatek/mt8188/mt8188-mt6359.c b/sound/soc/mediatek/mt8188/mt8188-mt6359.c index 5b2660139421..ac69c23e0da1 100644 --- a/sound/soc/mediatek/mt8188/mt8188-mt6359.c +++ b/sound/soc/mediatek/mt8188/mt8188-mt6359.c @@ -6,6 +6,7 @@
- Author: Trevor Wu trevor.wu@mediatek.com
*/
+#include <linux/bitfield.h> #include <linux/input.h> #include <linux/module.h> #include <linux/of_device.h> @@ -19,6 +20,15 @@ #include "../common/mtk-afe-platform-driver.h" #include "../common/mtk-soundcard-driver.h"
+#define CKSYS_AUD_TOP_CFG0x032c
- #define RG_TEST_ONBIT(0)
- #define RG_TEST_TYPEBIT(2)
+#define CKSYS_AUD_TOP_MON0x0330
#define TEST_MISO_COUNT_1GENMASK(3, 0)
#define TEST_MISO_COUNT_2GENMASK(7, 4)
#define TEST_MISO_DONE_1BIT(28)
#define TEST_MISO_DONE_2BIT(29)
#define NAU8825_HS_PRESENTBIT(0)
/*
@@ -251,9 +261,6 @@ static const struct snd_kcontrol_new mt8188_nau8825_controls[] = { SOC_DAPM_PIN_SWITCH("Headphone Jack"), };
-#define CKSYS_AUD_TOP_CFG 0x032c -#define CKSYS_AUD_TOP_MON 0x0330
- static int mt8188_mt6359_mtkaif_calibration(struct
snd_soc_pcm_runtime *rtd) { struct snd_soc_component *cmpnt_afe = @@ -265,13 +272,13 @@ static int mt8188_mt6359_mtkaif_calibration(struct snd_soc_pcm_runtime *rtd) struct mtkaif_param *param; int chosen_phase_1, chosen_phase_2; int prev_cycle_1, prev_cycle_2; -int test_done_1, test_done_2; +u8 test_done_1, test_done_2; int cycle_1, cycle_2; int mtkaif_chosen_phase[MT8188_MTKAIF_MISO_NUM]; int mtkaif_phase_cycle[MT8188_MTKAIF_MISO_NUM]; int mtkaif_calibration_num_phase; bool mtkaif_calibration_ok; -unsigned int monitor = 0; +u32 monitor = 0; int counter; int phase; int i; @@ -303,8 +310,7 @@ static int mt8188_mt6359_mtkaif_calibration(struct snd_soc_pcm_runtime *rtd) mt6359_mtkaif_calibration_enable(cmpnt_codec);
/* set test type to synchronizer pulse */ -regmap_update_bits(afe_priv->topckgen,
- CKSYS_AUD_TOP_CFG, 0xffff, 0x4);
+regmap_write(afe_priv->topckgen, CKSYS_AUD_TOP_CFG, RG_TEST_TYPE);
Hi Angelo,
Because CKSYS_AUD_TOP_CFG is a 32bit register, it should be better
to
use regmap_set_bits instead.
regmap_set_bits(afe_priv->topckgen, CKSYS_AUD_TOP_CFG,
RG_TEST_TYPE);
The previous call to regmap_update_bits() was unsetting RG_TEST_ON and RE_SW_RESET while setting RG_TEST_TYPE: using regmap_write() ensures that we do exactly the same, without the overhead of performing the additional register read and bit swapping operations.
Using the proposed regmap_set_bits() would change the behavior of this flow, which may result in unexpected hardware behavior, as we wouldn't be unsetting the previously mentioned two bits.
Regards, Angelo
It's unclear why the original author chose a mask value of 0xffff, but remap_write() also clears the higher 16 bits which differs from the original logic.
The comment for that line states 'set test type to synchronizer pulse', so I suggest updating only BIT2 here. Additionally, I checked datasheet for other two bits, which have default values of 0.
However, I came across an internal codebase that used regmap_write(), so it should be a safe implementation. If you think regmap_write() is better, it's ok with me.
Thanks, Trevor
Thanks, Trevor
mtkaif_calibration_num_phase = 42;/* mt6359: 0 ~ 42 */ mtkaif_calibration_ok = true;
@@ -314,7 +320,7 @@ static int mt8188_mt6359_mtkaif_calibration(struct snd_soc_pcm_runtime *rtd) mt6359_set_mtkaif_calibration_phase(cmpnt_codec, phase, phase, phase);
-regmap_set_bits(afe_priv->topckgen, CKSYS_AUD_TOP_CFG, 0x1); +regmap_set_bits(afe_priv->topckgen, CKSYS_AUD_TOP_CFG, RG_TEST_ON);
test_done_1 = 0; test_done_2 = 0; @@ -326,14 +332,14 @@ static int mt8188_mt6359_mtkaif_calibration(struct snd_soc_pcm_runtime *rtd) while (!(test_done_1 & test_done_2)) { regmap_read(afe_priv->topckgen, CKSYS_AUD_TOP_MON, &monitor); -test_done_1 = (monitor >> 28) & 0x1; -test_done_2 = (monitor >> 29) & 0x1; +test_done_1 = FIELD_GET(TEST_MISO_DONE_1, monitor); +test_done_2 = FIELD_GET(TEST_MISO_DONE_2, monitor);
if (test_done_1 == 1) -cycle_1 = monitor & 0xf; +cycle_1 = FIELD_GET(TEST_MISO_COUNT_1, monitor);
if (test_done_2 == 1) -cycle_2 = (monitor >> 4) & 0xf; +cycle_2 = FIELD_GET(TEST_MISO_COUNT_2, monitor);
/* handle if never test done */ if (++counter > 10000) { @@ -361,7 +367,7 @@ static int mt8188_mt6359_mtkaif_calibration(struct snd_soc_pcm_runtime *rtd) mtkaif_phase_cycle[MT8188_MTKAIF_MISO_1] = prev_cycle_2; }
-regmap_clear_bits(afe_priv->topckgen, CKSYS_AUD_TOP_CFG, 0x1); +regmap_clear_bits(afe_priv->topckgen, CKSYS_AUD_TOP_CFG, RG_TEST_ON);
if (mtkaif_chosen_phase[MT8188_MTKAIF_MISO_0] >= 0 && mtkaif_chosen_phase[MT8188_MTKAIF_MISO_1] >= 0) -- 2.40.1