[PATCH 5/5] ASoC: mediatek: mt8188-mt6359: Use bitfield macros for registers
AngeloGioacchino Del Regno
angelogioacchino.delregno at collabora.com
Thu Jun 8 04:20:36 PDT 2023
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 at 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 at 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.
>
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
>>
>>
>>
More information about the linux-arm-kernel
mailing list