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

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

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@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.
> 

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
>>
>>
>>