Re: [PATCH v8 08/10] ASoC: mediatek: mt8196: add platform driver

[Cyril Chao (钞悦)]

Thank you for your assistance in reviewing. Could you please also
review the modifications in the diff? If everything is okay, I will
include them in v9 in the next update.


diff --git a/sound/soc/mediatek/mt8196/mt8196-afe-pcm.c
b/sound/soc/mediatek/mt8196/mt8196-afe-pcm.c
index 3d3174cd8efb..ff7aa89e4779 100644
--- a/sound/soc/mediatek/mt8196/mt8196-afe-pcm.c
+++ b/sound/soc/mediatek/mt8196/mt8196-afe-pcm.c
@@ -90,9 +90,20 @@ static int mt8196_set_cm(struct mtk_base_afe *afe,
int id,
 	struct mt8196_afe_private *afe_priv = afe->platform_priv;
 	unsigned int rate = afe_priv->cm_rate[id];
 	unsigned int rate_val = mt8196_rate_transform(afe->dev, rate);
-	unsigned int update_val = update ? ((((26000000 / rate) - 10) /
(ch / 2)) - 1) : 0x64;
+	unsigned int ch_pair = ch / 2;
+	unsigned int update_val;
 	int reg = AFE_CM0_CON0 + 0x10 * id;
 
+	if (update) {
+		if (ch_pair == 0) {
+			dev_err(afe->dev, "CM%d: invalid channel count
%u\n", id, ch);
+			return -EINVAL;
+		}
+		update_val = (26000000 / rate - 10) / ch_pair - 1;
+	} else {
+		update_val = 0x64;
+	}
+
 	dev_dbg(afe->dev, "CM%d, rate %d, update %d, swap %d, ch %d\n",
 		id, rate, update, swap, ch);
 
@@ -471,6 +482,7 @@ static int ul_cm0_event(struct snd_soc_dapm_widget
*w,
 	struct mtk_base_afe *afe =
snd_soc_component_get_drvdata(cmpnt);
 	struct mt8196_afe_private *afe_priv = afe->platform_priv;
 	unsigned int channels = afe_priv->cm_channels;
+	int ret;
 
 	dev_dbg(afe->dev, "event 0x%x, name %s, channels %u\n",
 		event, w->name, channels);
@@ -478,7 +490,9 @@ static int ul_cm0_event(struct snd_soc_dapm_widget
*w,
 	switch (event) {
 	case SND_SOC_DAPM_PRE_PMU:
 		mt8196_enable_cm_bypass(afe, CM0, false);
-		mt8196_set_cm(afe, CM0, true, false, channels);
+		ret = mt8196_set_cm(afe, CM0, true, false, channels);
+		if (ret)
+			return ret;
 		regmap_update_bits(afe->regmap, AUDIO_TOP_CON0,
 				   PDN_CM0_MASK_SFT, 0 << PDN_CM0_SFT);
 		break;
@@ -502,6 +516,7 @@ static int ul_cm1_event(struct snd_soc_dapm_widget
*w,
 	struct mtk_base_afe *afe =
snd_soc_component_get_drvdata(cmpnt);
 	struct mt8196_afe_private *afe_priv = afe->platform_priv;
 	unsigned int channels = afe_priv->cm_channels;
+	int ret;
 
 	dev_dbg(afe->dev, "event 0x%x, name %s, channels %u\n",
 		event, w->name, channels);
@@ -509,7 +524,9 @@ static int ul_cm1_event(struct snd_soc_dapm_widget
*w,
 	switch (event) {
 	case SND_SOC_DAPM_PRE_PMU:
 		mt8196_enable_cm_bypass(afe, CM1, false);
-		mt8196_set_cm(afe, CM1, true, false, channels);
+		ret = mt8196_set_cm(afe, CM1, true, false, channels);
+		if (ret)
+			return ret;
 		regmap_update_bits(afe->regmap, AUDIO_TOP_CON0,
 				   PDN_CM1_MASK_SFT, 0 << PDN_CM1_SFT);
 		break;
@@ -533,6 +550,7 @@ static int ul_cm2_event(struct snd_soc_dapm_widget
*w,
 	struct mtk_base_afe *afe =
snd_soc_component_get_drvdata(cmpnt);
 	struct mt8196_afe_private *afe_priv = afe->platform_priv;
 	unsigned int channels = afe_priv->cm_channels;
+	int ret;
 
 	dev_dbg(afe->dev, "event 0x%x, name %s, channels %u\n",
 		event, w->name, channels);
@@ -540,7 +558,9 @@ static int ul_cm2_event(struct snd_soc_dapm_widget
*w,
 	switch (event) {
 	case SND_SOC_DAPM_PRE_PMU:
 		mt8196_enable_cm_bypass(afe, CM2, false);
-		mt8196_set_cm(afe, CM2, true, false, channels);
+		ret = mt8196_set_cm(afe, CM2, true, false, channels);
+		if (ret)
+			return ret;
 		regmap_update_bits(afe->regmap, AUDIO_TOP_CON0,
 				   PDN_CM2_MASK_SFT, 0 << PDN_CM2_SFT);
 		break;

Best Regards
Cyril Chao


On Fri, 2026-04-03 at 15:07 +0100, Mark Brown wrote:
> On Tue, Mar 24, 2026 at 09:56:49AM +0800, Cyril Chao wrote:
> 
> > +static int mt8196_set_cm(struct mtk_base_afe *afe, int id,
> > +			 bool update, bool swap, unsigned int ch)
> > +{
> > +	struct mt8196_afe_private *afe_priv = afe->platform_priv;
> > +	unsigned int rate = afe_priv->cm_rate[id];
> > +	unsigned int rate_val = mt8196_rate_transform(afe->dev, rate);
> > +	unsigned int update_val = update ? ((((26000000 / rate) - 10) /
> > (ch / 2)) - 1) : 0x64;
> > +	int reg = AFE_CM0_CON0 + 0x10 * id;
> 
> The driver looks like it supports mono so won't this trigger divide
> by
> zero?
> 
> Also please write normal conditional statements, it's much more
> leigible.