[PATCH 0/5] ASoC: mt8188-mt6359: Cleanups

[PATCH 0/5] ASoC: mt8188-mt6359: Cleanups

[AngeloGioacchino Del Regno]

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

-- 
2.40.1

[PATCH 0/5] ASoC: mt8188-mt6359: Cleanups

[AngeloGioacchino Del Regno]

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

-- 
2.40.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 1/5] ASoC: mediatek: mt8188-mt6359: Compress of_device_id entries

[AngeloGioacchino Del Regno]

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);
 
-- 
2.40.1

[PATCH 1/5] ASoC: mediatek: mt8188-mt6359: Compress of_device_id entries

[AngeloGioacchino Del Regno]

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);
 
-- 
2.40.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/5] ASoC: mediatek: mt8188-mt6359: clean up a return in codec_init

[AngeloGioacchino Del Regno]

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

[PATCH 2/5] ASoC: mediatek: mt8188-mt6359: clean up a return in codec_init

[AngeloGioacchino Del Regno]

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 3/5] ASoC: mediatek: mt8188-mt6359: Cleanup return 0 disguised as return ret

[AngeloGioacchino Del Regno]

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 = {
-- 
2.40.1

[PATCH 3/5] ASoC: mediatek: mt8188-mt6359: Cleanup return 0 disguised as return ret

[AngeloGioacchino Del Regno]

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 = {
-- 
2.40.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 4/5] ASoC: mediatek: mt8188-mt6359: Clean up log levels

[AngeloGioacchino Del Regno]

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;
 	}
 
-- 
2.40.1

[PATCH 4/5] ASoC: mediatek: mt8188-mt6359: Clean up log levels

[AngeloGioacchino Del Regno]

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;
 	}
 
-- 
2.40.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[AngeloGioacchino Del Regno]

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

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

[AngeloGioacchino Del Regno]

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/5] ASoC: mediatek: mt8188-mt6359: Compress of_device_id entries

[Alexandre Mergnat]

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>

-- 
Regards,
Alexandre

Re: [PATCH 1/5] ASoC: mediatek: mt8188-mt6359: Compress of_device_id entries

[Alexandre Mergnat]

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>

-- 
Regards,
Alexandre


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/5] ASoC: mediatek: mt8188-mt6359: clean up a return in codec_init

[Alexandre Mergnat]

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>

-- 
Regards,
Alexandre

Re: [PATCH 2/5] ASoC: mediatek: mt8188-mt6359: clean up a return in codec_init

[Alexandre Mergnat]

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>

-- 
Regards,
Alexandre


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/5] ASoC: mediatek: mt8188-mt6359: Cleanup return 0 disguised as return ret

[Alexandre Mergnat]

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>

-- 
Regards,
Alexandre

Re: [PATCH 3/5] ASoC: mediatek: mt8188-mt6359: Cleanup return 0 disguised as return ret

[Alexandre Mergnat]

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>

-- 
Regards,
Alexandre


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 4/5] ASoC: mediatek: mt8188-mt6359: Clean up log levels

[Alexandre Mergnat]

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>

-- 
Regards,
Alexandre

Re: [PATCH 4/5] ASoC: mediatek: mt8188-mt6359: Clean up log levels

[Alexandre Mergnat]

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>

-- 
Regards,
Alexandre


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Alexandre Mergnat]

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>

-- 
Regards,
Alexandre

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

[Alexandre Mergnat]

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>

-- 
Regards,
Alexandre


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[AngeloGioacchino Del Regno]

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

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

[AngeloGioacchino Del Regno]

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Alexandre Mergnat]

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.

-- 
Regards,
Alexandre

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

[Alexandre Mergnat]

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.

-- 
Regards,
Alexandre


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Trevor Wu (吳文良)]

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

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

[Trevor Wu (吳文良)]

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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[AngeloGioacchino Del Regno]

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

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

[AngeloGioacchino Del Regno]

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




_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Trevor Wu (吳文良)]

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

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

[Trevor Wu (吳文良)]

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
> 
> 
> 
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[AngeloGioacchino Del Regno]

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

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

[AngeloGioacchino Del Regno]

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/5] ASoC: mt8188-mt6359: Cleanups

[Mark Brown]

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

Re: [PATCH 0/5] ASoC: mt8188-mt6359: Cleanups

[Mark Brown]

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/5] ASoC: mediatek: mt8188-mt6359: Compress of_device_id entries

[Matthias Brugger]

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

Re: [PATCH 1/5] ASoC: mediatek: mt8188-mt6359: Compress of_device_id entries

[Matthias Brugger]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/5] ASoC: mediatek: mt8188-mt6359: clean up a return in codec_init

[Matthias Brugger]

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,

Re: [PATCH 2/5] ASoC: mediatek: mt8188-mt6359: clean up a return in codec_init

[Matthias Brugger]

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,

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/5] ASoC: mediatek: mt8188-mt6359: Cleanup return 0 disguised as return ret

[Matthias Brugger]

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

Re: [PATCH 3/5] ASoC: mediatek: mt8188-mt6359: Cleanup return 0 disguised as return ret

[Matthias Brugger]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel