[PATCH 0/5] ASoC: mt8188-mt6359: Cleanups
This series performs some cleanups to the mt8188-mt6359 driver, including usage of bitfield macros, adding definitions of register fields and some others for readability and consistency.
AngeloGioacchino Del Regno (4): ASoC: mediatek: mt8188-mt6359: Compress of_device_id entries ASoC: mediatek: mt8188-mt6359: Cleanup return 0 disguised as return ret ASoC: mediatek: mt8188-mt6359: Clean up log levels ASoC: mediatek: mt8188-mt6359: Use bitfield macros for registers
Dan Carpenter (1): ASoC: mediatek: mt8188-mt6359: clean up a return in codec_init
sound/soc/mediatek/mt8188/mt8188-mt6359.c | 87 ++++++++++++----------- 1 file changed, 45 insertions(+), 42 deletions(-)
Those entries fit in one line: compress them to reduce line count. While at it, also add the sentinel comment to the last entry.
Signed-off-by: AngeloGioacchino Del Regno angelogioacchino.delregno@collabora.com --- sound/soc/mediatek/mt8188/mt8188-mt6359.c | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-)
diff --git a/sound/soc/mediatek/mt8188/mt8188-mt6359.c b/sound/soc/mediatek/mt8188/mt8188-mt6359.c index bc4b74970a46..643a7a12a96b 100644 --- a/sound/soc/mediatek/mt8188/mt8188-mt6359.c +++ b/sound/soc/mediatek/mt8188/mt8188-mt6359.c @@ -1117,15 +1117,9 @@ static struct mt8188_card_data mt8188_nau8825_card = { };
static const struct of_device_id mt8188_mt6359_dt_match[] = { - { - .compatible = "mediatek,mt8188-mt6359-evb", - .data = &mt8188_evb_card, - }, - { - .compatible = "mediatek,mt8188-nau8825", - .data = &mt8188_nau8825_card, - }, - {}, + { .compatible = "mediatek,mt8188-mt6359-evb", .data = &mt8188_evb_card, }, + { .compatible = "mediatek,mt8188-nau8825", .data = &mt8188_nau8825_card, }, + { /* sentinel */ }, }; MODULE_DEVICE_TABLE(of, mt8188_mt6359_dt_match);
On 08/06/2023 10:47, AngeloGioacchino Del Regno wrote:
Those entries fit in one line: compress them to reduce line count. While at it, also add the sentinel comment to the last entry.
Reviewed-by: Alexandre Mergnat amergnat@baylibre.com
On 08/06/2023 10:47, AngeloGioacchino Del Regno wrote:
Those entries fit in one line: compress them to reduce line count. While at it, also add the sentinel comment to the last entry.
Signed-off-by: AngeloGioacchino Del Regno angelogioacchino.delregno@collabora.com
Reviewed-by: Matthias Brugger matthias.bgg@gmail.com
sound/soc/mediatek/mt8188/mt8188-mt6359.c | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-)
diff --git a/sound/soc/mediatek/mt8188/mt8188-mt6359.c b/sound/soc/mediatek/mt8188/mt8188-mt6359.c index bc4b74970a46..643a7a12a96b 100644 --- a/sound/soc/mediatek/mt8188/mt8188-mt6359.c +++ b/sound/soc/mediatek/mt8188/mt8188-mt6359.c @@ -1117,15 +1117,9 @@ static struct mt8188_card_data mt8188_nau8825_card = { };
static const struct of_device_id mt8188_mt6359_dt_match[] = {
- {
.compatible = "mediatek,mt8188-mt6359-evb",.data = &mt8188_evb_card,- },
- {
.compatible = "mediatek,mt8188-nau8825",.data = &mt8188_nau8825_card,- },
- {},
- { .compatible = "mediatek,mt8188-mt6359-evb", .data = &mt8188_evb_card, },
- { .compatible = "mediatek,mt8188-nau8825", .data = &mt8188_nau8825_card, },
- { /* sentinel */ }, }; MODULE_DEVICE_TABLE(of, mt8188_mt6359_dt_match);
From: Dan Carpenter dan.carpenter@linaro.org
This code triggers a Smatch static checker warning and does sort of look like an error path.
sound/soc/mediatek/mt8188/mt8188-mt6359.c:597 mt8188_max98390_codec_init() warn: missing error code? 'ret'
However, returning 0 is intentional. Make that explicit.
Signed-off-by: Dan Carpenter dan.carpenter@linaro.org Signed-off-by: AngeloGioacchino Del Regno angelogioacchino.delregno@collabora.com --- sound/soc/mediatek/mt8188/mt8188-mt6359.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/mediatek/mt8188/mt8188-mt6359.c b/sound/soc/mediatek/mt8188/mt8188-mt6359.c index 643a7a12a96b..b2735496d140 100644 --- a/sound/soc/mediatek/mt8188/mt8188-mt6359.c +++ b/sound/soc/mediatek/mt8188/mt8188-mt6359.c @@ -594,7 +594,7 @@ static int mt8188_max98390_codec_init(struct snd_soc_pcm_runtime *rtd) }
if (rtd->dai_link->num_codecs <= 2) - return ret; + return 0;
/* add widgets/controls/dapm for rear speakers */ ret = snd_soc_dapm_new_controls(&card->dapm, mt8188_rear_spk_widgets,
On 08/06/2023 10:47, AngeloGioacchino Del Regno wrote:
This code triggers a Smatch static checker warning and does sort of look like an error path.
sound/soc/mediatek/mt8188/mt8188-mt6359.c:597 mt8188_max98390_codec_init() warn: missing error code? 'ret'
However, returning 0 is intentional. Make that explicit.
Reviewed-by: Alexandre Mergnat amergnat@baylibre.com
On 08/06/2023 10:47, AngeloGioacchino Del Regno wrote:
From: Dan Carpenter dan.carpenter@linaro.org
This code triggers a Smatch static checker warning and does sort of look like an error path.
sound/soc/mediatek/mt8188/mt8188-mt6359.c:597 mt8188_max98390_codec_init() warn: missing error code? 'ret'
However, returning 0 is intentional. Make that explicit.
Signed-off-by: Dan Carpenter dan.carpenter@linaro.org Signed-off-by: AngeloGioacchino Del Regno angelogioacchino.delregno@collabora.com
Reviewed-by: Matthias Brugger matthias.bgg@gmail.com
sound/soc/mediatek/mt8188/mt8188-mt6359.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/mediatek/mt8188/mt8188-mt6359.c b/sound/soc/mediatek/mt8188/mt8188-mt6359.c index 643a7a12a96b..b2735496d140 100644 --- a/sound/soc/mediatek/mt8188/mt8188-mt6359.c +++ b/sound/soc/mediatek/mt8188/mt8188-mt6359.c @@ -594,7 +594,7 @@ static int mt8188_max98390_codec_init(struct snd_soc_pcm_runtime *rtd) }
if (rtd->dai_link->num_codecs <= 2)
return ret;
return 0;/* add widgets/controls/dapm for rear speakers */ ret = snd_soc_dapm_new_controls(&card->dapm, mt8188_rear_spk_widgets,
Change all instances of `return ret` to `return 0` at the end of functions where ret is always zero and also change functions mt8188_{hdmi,dptx}_codec_init to be consistent with how other functions are returning errors
Signed-off-by: AngeloGioacchino Del Regno angelogioacchino.delregno@collabora.com --- sound/soc/mediatek/mt8188/mt8188-mt6359.c | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-)
diff --git a/sound/soc/mediatek/mt8188/mt8188-mt6359.c b/sound/soc/mediatek/mt8188/mt8188-mt6359.c index b2735496d140..260cace408b9 100644 --- a/sound/soc/mediatek/mt8188/mt8188-mt6359.c +++ b/sound/soc/mediatek/mt8188/mt8188-mt6359.c @@ -491,11 +491,13 @@ static int mt8188_hdmi_codec_init(struct snd_soc_pcm_runtime *rtd) }
ret = snd_soc_component_set_jack(component, &priv->hdmi_jack, NULL); - if (ret) + if (ret) { dev_info(rtd->dev, "%s, set jack failed on %s (ret=%d)\n", __func__, component->name, ret); + return ret; + }
- return ret; + return 0; }
static int mt8188_dptx_codec_init(struct snd_soc_pcm_runtime *rtd) @@ -513,11 +515,13 @@ static int mt8188_dptx_codec_init(struct snd_soc_pcm_runtime *rtd) }
ret = snd_soc_component_set_jack(component, &priv->dp_jack, NULL); - if (ret) + if (ret) { dev_info(rtd->dev, "%s, set jack failed on %s (ret=%d)\n", __func__, component->name, ret); + return ret; + }
- return ret; + return 0; }
static int mt8188_dumb_amp_init(struct snd_soc_pcm_runtime *rtd) @@ -539,7 +543,7 @@ static int mt8188_dumb_amp_init(struct snd_soc_pcm_runtime *rtd) return ret; }
- return ret; + return 0; }
static int mt8188_max98390_hw_params(struct snd_pcm_substream *substream, @@ -612,7 +616,7 @@ static int mt8188_max98390_codec_init(struct snd_soc_pcm_runtime *rtd) return ret; }
- return ret; + return 0; }
static int mt8188_nau8825_codec_init(struct snd_soc_pcm_runtime *rtd) @@ -660,7 +664,7 @@ static int mt8188_nau8825_codec_init(struct snd_soc_pcm_runtime *rtd) return ret; }
- return ret; + return 0; };
static void mt8188_nau8825_codec_exit(struct snd_soc_pcm_runtime *rtd) @@ -697,7 +701,7 @@ static int mt8188_nau8825_hw_params(struct snd_pcm_substream *substream, return ret; }
- return ret; + return 0; }
static const struct snd_soc_ops mt8188_nau8825_ops = {
On 08/06/2023 10:47, AngeloGioacchino Del Regno wrote:
Change all instances of `return ret` to `return 0` at the end of functions where ret is always zero and also change functions mt8188_{hdmi,dptx}_codec_init to be consistent with how other functions are returning errors
Reviewed-by: Alexandre Mergnat amergnat@baylibre.com
On 08/06/2023 10:47, AngeloGioacchino Del Regno wrote:
Change all instances of `return ret` to `return 0` at the end of functions where ret is always zero and also change functions mt8188_{hdmi,dptx}_codec_init to be consistent with how other functions are returning errors
Signed-off-by: AngeloGioacchino Del Regno angelogioacchino.delregno@collabora.com
Reviewed-by: Matthias Brugger matthias.bgg@gmail.com
sound/soc/mediatek/mt8188/mt8188-mt6359.c | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-)
diff --git a/sound/soc/mediatek/mt8188/mt8188-mt6359.c b/sound/soc/mediatek/mt8188/mt8188-mt6359.c index b2735496d140..260cace408b9 100644 --- a/sound/soc/mediatek/mt8188/mt8188-mt6359.c +++ b/sound/soc/mediatek/mt8188/mt8188-mt6359.c @@ -491,11 +491,13 @@ static int mt8188_hdmi_codec_init(struct snd_soc_pcm_runtime *rtd) }
ret = snd_soc_component_set_jack(component, &priv->hdmi_jack, NULL);
- if (ret)
- if (ret) { dev_info(rtd->dev, "%s, set jack failed on %s (ret=%d)\n", __func__, component->name, ret);
return ret;- }
- return ret;
return 0; }
static int mt8188_dptx_codec_init(struct snd_soc_pcm_runtime *rtd)
@@ -513,11 +515,13 @@ static int mt8188_dptx_codec_init(struct snd_soc_pcm_runtime *rtd) }
ret = snd_soc_component_set_jack(component, &priv->dp_jack, NULL);
- if (ret)
- if (ret) { dev_info(rtd->dev, "%s, set jack failed on %s (ret=%d)\n", __func__, component->name, ret);
return ret;- }
- return ret;
return 0; }
static int mt8188_dumb_amp_init(struct snd_soc_pcm_runtime *rtd)
@@ -539,7 +543,7 @@ static int mt8188_dumb_amp_init(struct snd_soc_pcm_runtime *rtd) return ret; }
- return ret;
return 0; }
static int mt8188_max98390_hw_params(struct snd_pcm_substream *substream,
@@ -612,7 +616,7 @@ static int mt8188_max98390_codec_init(struct snd_soc_pcm_runtime *rtd) return ret; }
- return ret;
return 0; }
static int mt8188_nau8825_codec_init(struct snd_soc_pcm_runtime *rtd)
@@ -660,7 +664,7 @@ static int mt8188_nau8825_codec_init(struct snd_soc_pcm_runtime *rtd) return ret; }
- return ret;
return 0; };
static void mt8188_nau8825_codec_exit(struct snd_soc_pcm_runtime *rtd)
@@ -697,7 +701,7 @@ static int mt8188_nau8825_hw_params(struct snd_pcm_substream *substream, return ret; }
- return ret;
return 0; }
static const struct snd_soc_ops mt8188_nau8825_ops = {
Change some dev_info prints to dev_err() and some to dev_dbg(), depending on the actual severity of them.
Signed-off-by: AngeloGioacchino Del Regno angelogioacchino.delregno@collabora.com --- sound/soc/mediatek/mt8188/mt8188-mt6359.c | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-)
diff --git a/sound/soc/mediatek/mt8188/mt8188-mt6359.c b/sound/soc/mediatek/mt8188/mt8188-mt6359.c index 260cace408b9..5b2660139421 100644 --- a/sound/soc/mediatek/mt8188/mt8188-mt6359.c +++ b/sound/soc/mediatek/mt8188/mt8188-mt6359.c @@ -337,9 +337,8 @@ static int mt8188_mt6359_mtkaif_calibration(struct snd_soc_pcm_runtime *rtd)
/* handle if never test done */ if (++counter > 10000) { - dev_info(afe->dev, "%s(), test fail, cycle_1 %d, cycle_2 %d, monitor 0x%x\n", - __func__, - cycle_1, cycle_2, monitor); + dev_err(afe->dev, "%s(), test fail, cycle_1 %d, cycle_2 %d, monitor 0x%x\n", + __func__, cycle_1, cycle_2, monitor); mtkaif_calibration_ok = false; break; } @@ -398,8 +397,8 @@ static int mt8188_mt6359_mtkaif_calibration(struct snd_soc_pcm_runtime *rtd) for (i = 0; i < MT8188_MTKAIF_MISO_NUM; i++) param->mtkaif_phase_cycle[i] = mtkaif_phase_cycle[i];
- dev_info(afe->dev, "%s(), end, calibration ok %d\n", - __func__, param->mtkaif_calibration_ok); + dev_dbg(afe->dev, "%s(), end, calibration ok %d\n", + __func__, param->mtkaif_calibration_ok);
return 0; } @@ -486,14 +485,14 @@ static int mt8188_hdmi_codec_init(struct snd_soc_pcm_runtime *rtd) mt8188_hdmi_jack_pins, ARRAY_SIZE(mt8188_hdmi_jack_pins)); if (ret) { - dev_info(rtd->dev, "%s, new jack failed: %d\n", __func__, ret); + dev_err(rtd->dev, "%s, new jack failed: %d\n", __func__, ret); return ret; }
ret = snd_soc_component_set_jack(component, &priv->hdmi_jack, NULL); if (ret) { - dev_info(rtd->dev, "%s, set jack failed on %s (ret=%d)\n", - __func__, component->name, ret); + dev_err(rtd->dev, "%s, set jack failed on %s (ret=%d)\n", + __func__, component->name, ret); return ret; }
@@ -510,14 +509,14 @@ static int mt8188_dptx_codec_init(struct snd_soc_pcm_runtime *rtd) &priv->dp_jack, mt8188_dp_jack_pins, ARRAY_SIZE(mt8188_dp_jack_pins)); if (ret) { - dev_info(rtd->dev, "%s, new jack failed: %d\n", __func__, ret); + dev_err(rtd->dev, "%s, new jack failed: %d\n", __func__, ret); return ret; }
ret = snd_soc_component_set_jack(component, &priv->dp_jack, NULL); if (ret) { - dev_info(rtd->dev, "%s, set jack failed on %s (ret=%d)\n", - __func__, component->name, ret); + dev_err(rtd->dev, "%s, set jack failed on %s (ret=%d)\n", + __func__, component->name, ret); return ret; }
On 08/06/2023 10:47, AngeloGioacchino Del Regno wrote:
Change some dev_info prints to dev_err() and some to dev_dbg(), depending on the actual severity of them.
Reviewed-by: Alexandre Mergnat amergnat@baylibre.com
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_CFG 0x032c + #define RG_TEST_ON BIT(0) + #define RG_TEST_TYPE BIT(2) +#define CKSYS_AUD_TOP_MON 0x0330 + #define TEST_MISO_COUNT_1 GENMASK(3, 0) + #define TEST_MISO_COUNT_2 GENMASK(7, 4) + #define TEST_MISO_DONE_1 BIT(28) + #define TEST_MISO_DONE_2 BIT(29) + #define NAU8825_HS_PRESENT BIT(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); 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)
On 08/06/2023 10:47, 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_CFG 0x032c
- #define RG_TEST_ON BIT(0)
- #define RG_TEST_TYPE BIT(2)
+#define CKSYS_AUD_TOP_MON 0x0330
#define TEST_MISO_COUNT_1 GENMASK(3, 0)
#define TEST_MISO_COUNT_2 GENMASK(7, 4)
#define TEST_MISO_DONE_1 BIT(28)
#define TEST_MISO_DONE_2 BIT(29)
#define NAU8825_HS_PRESENT BIT(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;
Shouldn't be unsigned too ?
I'm wondering if it would be better (probably in another patch) to change the data type of the other variables too, to match their use-case. (maybe it's already the case, I'm just wondering)
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); 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)
After that:
Reviewed-by: Alexandre Mergnat amergnat@baylibre.com
Il 08/06/23 11:50, Alexandre Mergnat ha scritto:
On 08/06/2023 10:47, 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_CFG 0x032c
- #define RG_TEST_ON BIT(0)
- #define RG_TEST_TYPE BIT(2)
+#define CKSYS_AUD_TOP_MON 0x0330
- #define TEST_MISO_COUNT_1 GENMASK(3, 0)
- #define TEST_MISO_COUNT_2 GENMASK(7, 4)
- #define TEST_MISO_DONE_1 BIT(28)
- #define TEST_MISO_DONE_2 BIT(29)
#define NAU8825_HS_PRESENT BIT(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;
Shouldn't be unsigned too ?
I'm wondering if it would be better (probably in another patch) to change the data type of the other variables too, to match their use-case. (maybe it's already the case, I'm just wondering)
In theory, yes, cycle_1 and 2 should be unsigned, but the signedness of this variable is getting used in the calibration logic later, as in the for loop they are both being reinitialized to -1 ... so I couldn't just switch 'em to unsigned.
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); 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)
After that:
Reviewed-by: Alexandre Mergnat amergnat@baylibre.com
On 08/06/2023 11:57, AngeloGioacchino Del Regno wrote:
- int test_done_1, test_done_2; + u8 test_done_1, test_done_2; int cycle_1, cycle_2;
Shouldn't be unsigned too ?
I'm wondering if it would be better (probably in another patch) to change the data type of the other variables too, to match their use-case. (maybe it's already the case, I'm just wondering)
In theory, yes, cycle_1 and 2 should be unsigned, but the signedness of this variable is getting used in the calibration logic later, as in the for loop they are both being reinitialized to -1 ... so I couldn't just switch 'em to unsigned.
Understood, thanks for your explanation.
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_CFG 0x032c
- #define RG_TEST_ON BIT(0)
- #define RG_TEST_TYPE BIT(2)
+#define CKSYS_AUD_TOP_MON 0x0330
- #define TEST_MISO_COUNT_1 GENMASK(3, 0)
- #define TEST_MISO_COUNT_2 GENMASK(7, 4)
- #define TEST_MISO_DONE_1 BIT(28)
- #define TEST_MISO_DONE_2 BIT(29)
#define NAU8825_HS_PRESENT BIT(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);
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
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_CFG 0x032c
- #define RG_TEST_ON BIT(0)
- #define RG_TEST_TYPE BIT(2)
+#define CKSYS_AUD_TOP_MON 0x0330
#define TEST_MISO_COUNT_1 GENMASK(3, 0)
#define TEST_MISO_COUNT_2 GENMASK(7, 4)
#define TEST_MISO_DONE_1 BIT(28)
#define TEST_MISO_DONE_2 BIT(29)
#define NAU8825_HS_PRESENT BIT(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
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
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
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
On Thu, 08 Jun 2023 10:47:22 +0200, AngeloGioacchino Del Regno wrote:
This series performs some cleanups to the mt8188-mt6359 driver, including usage of bitfield macros, adding definitions of register fields and some others for readability and consistency.
AngeloGioacchino Del Regno (4): ASoC: mediatek: mt8188-mt6359: Compress of_device_id entries ASoC: mediatek: mt8188-mt6359: Cleanup return 0 disguised as return ret ASoC: mediatek: mt8188-mt6359: Clean up log levels ASoC: mediatek: mt8188-mt6359: Use bitfield macros for registers
[...]
Applied to
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next
Thanks!
[1/5] ASoC: mediatek: mt8188-mt6359: Compress of_device_id entries commit: 22628e92d76a403181916f7bac7848dd2326d750 [2/5] ASoC: mediatek: mt8188-mt6359: clean up a return in codec_init commit: 1148b42257e2bf30093708398db2c4570ae9fe97 [3/5] ASoC: mediatek: mt8188-mt6359: Cleanup return 0 disguised as return ret commit: 4882ef44f51bbb759b8a738b747fdbcbad38e51b [4/5] ASoC: mediatek: mt8188-mt6359: Clean up log levels commit: acb43baf8b7e75acdb14920de29881e3f70c6819 [5/5] ASoC: mediatek: mt8188-mt6359: Use bitfield macros for registers commit: b0e2e4fb8a5467f4f64bcf64d1454d18cb665cc8
All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying to this mail.
Thanks, Mark
participants (5)
-
Alexandre Mergnat -
AngeloGioacchino Del Regno -
Mark Brown -
Matthias Brugger -
Trevor Wu (吳文良)