Re: [PATCH 5/5] ASoC: mediatek: mt8188-mt6359: Use bitfield macros for registers

AngeloGioacchino Del Regno <angelogioacchino.delregno@xxxxxxxxxxxxx> · Thu, 8 Jun 2023 13:20:36 +0200

Il 08/06/23 13:02, Trevor Wu (吳文良) ha scritto:
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@xxxxxxxxxxxxx>
---
   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@xxxxxxxxxxxx>
    */
   
+#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.


Yes, using regmap_write() here is better.

Thanks,
Angelo

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










