[v1] ASoC: mt8188-mt6359: add SOF support

[PATCH 1/3] ASoC: mediatek: mt8188-mt6359: support dynamic pinctrl

Posted by Trevor Wu 2 years, 6 months ago

To avoid power leakage, it is recommended to replace the default pinctrl
state with dynamic pinctrl since certain audio pinmux functions can
remain in a HIGH state even when audio is disabled. Linking pinctrl with
DAPM using SND_SOC_DAPM_PINCTRL will ensure that audio pins remain in
GPIO mode by default and only switch to an audio function when necessary.

Signed-off-by: Trevor Wu <trevor.wu@mediatek.com>
---
 sound/soc/mediatek/mt8188/mt8188-mt6359.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/sound/soc/mediatek/mt8188/mt8188-mt6359.c b/sound/soc/mediatek/mt8188/mt8188-mt6359.c
index 7c9e08e6a4f5..667d79f33bf2 100644
--- a/sound/soc/mediatek/mt8188/mt8188-mt6359.c
+++ b/sound/soc/mediatek/mt8188/mt8188-mt6359.c
@@ -246,6 +246,11 @@ static const struct snd_soc_dapm_widget mt8188_mt6359_widgets[] = {
 	SND_SOC_DAPM_MIC("Headset Mic", NULL),
 	SND_SOC_DAPM_SINK("HDMI"),
 	SND_SOC_DAPM_SINK("DP"),
+
+	/* dynamic pinctrl */
+	SND_SOC_DAPM_PINCTRL("ETDM_SPK_PIN", "aud_etdm_spk_on", "aud_etdm_spk_off"),
+	SND_SOC_DAPM_PINCTRL("ETDM_HP_PIN", "aud_etdm_hp_on", "aud_etdm_hp_off"),
+	SND_SOC_DAPM_PINCTRL("MTKAIF_PIN", "aud_mtkaif_on", "aud_mtkaif_off"),
 };
 
 static const struct snd_kcontrol_new mt8188_mt6359_controls[] = {
@@ -267,6 +272,7 @@ static int mt8188_mt6359_mtkaif_calibration(struct snd_soc_pcm_runtime *rtd)
 		snd_soc_rtdcom_lookup(rtd, AFE_PCM_NAME);
 	struct snd_soc_component *cmpnt_codec =
 		asoc_rtd_to_codec(rtd, 0)->component;
+	struct snd_soc_dapm_widget *pin_w = NULL, *w;
 	struct mtk_base_afe *afe;
 	struct mt8188_afe_private *afe_priv;
 	struct mtkaif_param *param;
@@ -306,6 +312,18 @@ static int mt8188_mt6359_mtkaif_calibration(struct snd_soc_pcm_runtime *rtd)
 		return 0;
 	}
 
+	for_each_card_widgets(rtd->card, w) {
+		if (!strcmp(w->name, "MTKAIF_PIN")) {
+			pin_w = w;
+			break;
+		}
+	}
+
+	if (!pin_w)
+		dev_dbg(afe->dev, "%s(), no pinmux widget, please check if default on\n", __func__);
+	else
+		dapm_pinctrl_event(pin_w, NULL, SND_SOC_DAPM_PRE_PMU);
+
 	pm_runtime_get_sync(afe->dev);
 	mt6359_mtkaif_calibration_enable(cmpnt_codec);
 
@@ -403,6 +421,9 @@ 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];
 
+	if (pin_w)
+		dapm_pinctrl_event(pin_w, NULL, SND_SOC_DAPM_POST_PMD);
+
 	dev_dbg(afe->dev, "%s(), end, calibration ok %d\n",
 		__func__, param->mtkaif_calibration_ok);
 
-- 
2.18.0

Re: [PATCH 1/3] ASoC: mediatek: mt8188-mt6359: support dynamic pinctrl

Posted by AngeloGioacchino Del Regno 2 years, 6 months ago

Il 25/07/23 05:53, Trevor Wu ha scritto:
> To avoid power leakage, it is recommended to replace the default pinctrl
> state with dynamic pinctrl since certain audio pinmux functions can
> remain in a HIGH state even when audio is disabled. Linking pinctrl with
> DAPM using SND_SOC_DAPM_PINCTRL will ensure that audio pins remain in
> GPIO mode by default and only switch to an audio function when necessary.
> 
> Signed-off-by: Trevor Wu <trevor.wu@mediatek.com>

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

Re: [PATCH 1/3] ASoC: mediatek: mt8188-mt6359: support dynamic pinctrl

Posted by AngeloGioacchino Del Regno 2 years, 6 months ago

Il 25/07/23 05:53, Trevor Wu ha scritto:
> To avoid power leakage, it is recommended to replace the default pinctrl
> state with dynamic pinctrl since certain audio pinmux functions can
> remain in a HIGH state even when audio is disabled. Linking pinctrl with
> DAPM using SND_SOC_DAPM_PINCTRL will ensure that audio pins remain in
> GPIO mode by default and only switch to an audio function when necessary.
> 
> Signed-off-by: Trevor Wu <trevor.wu@mediatek.com>
> ---
>   sound/soc/mediatek/mt8188/mt8188-mt6359.c | 21 +++++++++++++++++++++
>   1 file changed, 21 insertions(+)
> 
> diff --git a/sound/soc/mediatek/mt8188/mt8188-mt6359.c b/sound/soc/mediatek/mt8188/mt8188-mt6359.c
> index 7c9e08e6a4f5..667d79f33bf2 100644
> --- a/sound/soc/mediatek/mt8188/mt8188-mt6359.c
> +++ b/sound/soc/mediatek/mt8188/mt8188-mt6359.c
> @@ -246,6 +246,11 @@ static const struct snd_soc_dapm_widget mt8188_mt6359_widgets[] = {
>   	SND_SOC_DAPM_MIC("Headset Mic", NULL),
>   	SND_SOC_DAPM_SINK("HDMI"),
>   	SND_SOC_DAPM_SINK("DP"),
> +
> +	/* dynamic pinctrl */
> +	SND_SOC_DAPM_PINCTRL("ETDM_SPK_PIN", "aud_etdm_spk_on", "aud_etdm_spk_off"),
> +	SND_SOC_DAPM_PINCTRL("ETDM_HP_PIN", "aud_etdm_hp_on", "aud_etdm_hp_off"),
> +	SND_SOC_DAPM_PINCTRL("MTKAIF_PIN", "aud_mtkaif_on", "aud_mtkaif_off"),
>   };
>   
>   static const struct snd_kcontrol_new mt8188_mt6359_controls[] = {
> @@ -267,6 +272,7 @@ static int mt8188_mt6359_mtkaif_calibration(struct snd_soc_pcm_runtime *rtd)
>   		snd_soc_rtdcom_lookup(rtd, AFE_PCM_NAME);
>   	struct snd_soc_component *cmpnt_codec =
>   		asoc_rtd_to_codec(rtd, 0)->component;
> +	struct snd_soc_dapm_widget *pin_w = NULL, *w;
>   	struct mtk_base_afe *afe;
>   	struct mt8188_afe_private *afe_priv;
>   	struct mtkaif_param *param;
> @@ -306,6 +312,18 @@ static int mt8188_mt6359_mtkaif_calibration(struct snd_soc_pcm_runtime *rtd)
>   		return 0;
>   	}
>   
> +	for_each_card_widgets(rtd->card, w) {
> +		if (!strcmp(w->name, "MTKAIF_PIN")) {

if (strncmp(w->name, "MTKAIF_PIN", strlen(w->name) == 0) {
	pin_w = w;
	break;
}

That's safer.

> +			pin_w = w;
> +			break;
> +		}
> +	}
> +
> +	if (!pin_w)

Just a nitpick: you're checking for `if (pin_w)` later in this function, so
to increase readability please do the same here.

if (pin_w)
	dapm_pinctrl_event(...)
else
	dev_dbg(...)

Regards,
Angelo

Re: [PATCH 1/3] ASoC: mediatek: mt8188-mt6359: support dynamic pinctrl

Posted by Trevor Wu (吳文良) 2 years, 6 months ago

On Tue, 2023-07-25 at 09:06 +0200, AngeloGioacchino Del Regno wrote:
> Il 25/07/23 05:53, Trevor Wu ha scritto:
> > To avoid power leakage, it is recommended to replace the default
> > pinctrl
> > state with dynamic pinctrl since certain audio pinmux functions can
> > remain in a HIGH state even when audio is disabled. Linking pinctrl
> > with
> > DAPM using SND_SOC_DAPM_PINCTRL will ensure that audio pins remain
> > in
> > GPIO mode by default and only switch to an audio function when
> > necessary.
> > 
> > Signed-off-by: Trevor Wu <trevor.wu@mediatek.com>
> > ---
> >   sound/soc/mediatek/mt8188/mt8188-mt6359.c | 21
> > +++++++++++++++++++++
> >   1 file changed, 21 insertions(+)
> > 
> > diff --git a/sound/soc/mediatek/mt8188/mt8188-mt6359.c
> > b/sound/soc/mediatek/mt8188/mt8188-mt6359.c
> > index 7c9e08e6a4f5..667d79f33bf2 100644
> > --- a/sound/soc/mediatek/mt8188/mt8188-mt6359.c
> > +++ b/sound/soc/mediatek/mt8188/mt8188-mt6359.c
> > @@ -246,6 +246,11 @@ static const struct snd_soc_dapm_widget
> > mt8188_mt6359_widgets[] = {
> >   	SND_SOC_DAPM_MIC("Headset Mic", NULL),
> >   	SND_SOC_DAPM_SINK("HDMI"),
> >   	SND_SOC_DAPM_SINK("DP"),
> > +
> > +	/* dynamic pinctrl */
> > +	SND_SOC_DAPM_PINCTRL("ETDM_SPK_PIN", "aud_etdm_spk_on",
> > "aud_etdm_spk_off"),
> > +	SND_SOC_DAPM_PINCTRL("ETDM_HP_PIN", "aud_etdm_hp_on",
> > "aud_etdm_hp_off"),
> > +	SND_SOC_DAPM_PINCTRL("MTKAIF_PIN", "aud_mtkaif_on",
> > "aud_mtkaif_off"),
> >   };
> >   
> >   static const struct snd_kcontrol_new mt8188_mt6359_controls[] = {
> > @@ -267,6 +272,7 @@ static int
> > mt8188_mt6359_mtkaif_calibration(struct snd_soc_pcm_runtime *rtd)
> >   		snd_soc_rtdcom_lookup(rtd, AFE_PCM_NAME);
> >   	struct snd_soc_component *cmpnt_codec =
> >   		asoc_rtd_to_codec(rtd, 0)->component;
> > +	struct snd_soc_dapm_widget *pin_w = NULL, *w;
> >   	struct mtk_base_afe *afe;
> >   	struct mt8188_afe_private *afe_priv;
> >   	struct mtkaif_param *param;
> > @@ -306,6 +312,18 @@ static int
> > mt8188_mt6359_mtkaif_calibration(struct snd_soc_pcm_runtime *rtd)
> >   		return 0;
> >   	}
> >   
> > +	for_each_card_widgets(rtd->card, w) {
> > +		if (!strcmp(w->name, "MTKAIF_PIN")) {
> 
> if (strncmp(w->name, "MTKAIF_PIN", strlen(w->name) == 0) {
> 	pin_w = w;
> 	break;
> }
> 
> That's safer.
> 

If w->name is MTKAIF, the strncmp expression will return 0. However,
the result is not expected. I prefer to keep strcmp here.

> > +			pin_w = w;
> > +			break;
> > +		}
> > +	}
> > +
> > +	if (!pin_w)
> 
> Just a nitpick: you're checking for `if (pin_w)` later in this
> function, so
> to increase readability please do the same here.
> 
> if (pin_w)
> 	dapm_pinctrl_event(...)
> else
> 	dev_dbg(...)
> 

OK. I will update it in v2.

Thanks,
Trevor

Re: [PATCH 1/3] ASoC: mediatek: mt8188-mt6359: support dynamic pinctrl

Posted by AngeloGioacchino Del Regno 2 years, 6 months ago

Il 26/07/23 04:19, Trevor Wu (吳文良) ha scritto:
> On Tue, 2023-07-25 at 09:06 +0200, AngeloGioacchino Del Regno wrote:
>> Il 25/07/23 05:53, Trevor Wu ha scritto:
>>> To avoid power leakage, it is recommended to replace the default
>>> pinctrl
>>> state with dynamic pinctrl since certain audio pinmux functions can
>>> remain in a HIGH state even when audio is disabled. Linking pinctrl
>>> with
>>> DAPM using SND_SOC_DAPM_PINCTRL will ensure that audio pins remain
>>> in
>>> GPIO mode by default and only switch to an audio function when
>>> necessary.
>>>
>>> Signed-off-by: Trevor Wu <trevor.wu@mediatek.com>
>>> ---
>>>    sound/soc/mediatek/mt8188/mt8188-mt6359.c | 21
>>> +++++++++++++++++++++
>>>    1 file changed, 21 insertions(+)
>>>
>>> diff --git a/sound/soc/mediatek/mt8188/mt8188-mt6359.c
>>> b/sound/soc/mediatek/mt8188/mt8188-mt6359.c
>>> index 7c9e08e6a4f5..667d79f33bf2 100644
>>> --- a/sound/soc/mediatek/mt8188/mt8188-mt6359.c
>>> +++ b/sound/soc/mediatek/mt8188/mt8188-mt6359.c
>>> @@ -246,6 +246,11 @@ static const struct snd_soc_dapm_widget
>>> mt8188_mt6359_widgets[] = {
>>>    	SND_SOC_DAPM_MIC("Headset Mic", NULL),
>>>    	SND_SOC_DAPM_SINK("HDMI"),
>>>    	SND_SOC_DAPM_SINK("DP"),
>>> +
>>> +	/* dynamic pinctrl */
>>> +	SND_SOC_DAPM_PINCTRL("ETDM_SPK_PIN", "aud_etdm_spk_on",
>>> "aud_etdm_spk_off"),
>>> +	SND_SOC_DAPM_PINCTRL("ETDM_HP_PIN", "aud_etdm_hp_on",
>>> "aud_etdm_hp_off"),
>>> +	SND_SOC_DAPM_PINCTRL("MTKAIF_PIN", "aud_mtkaif_on",
>>> "aud_mtkaif_off"),
>>>    };
>>>    
>>>    static const struct snd_kcontrol_new mt8188_mt6359_controls[] = {
>>> @@ -267,6 +272,7 @@ static int
>>> mt8188_mt6359_mtkaif_calibration(struct snd_soc_pcm_runtime *rtd)
>>>    		snd_soc_rtdcom_lookup(rtd, AFE_PCM_NAME);
>>>    	struct snd_soc_component *cmpnt_codec =
>>>    		asoc_rtd_to_codec(rtd, 0)->component;
>>> +	struct snd_soc_dapm_widget *pin_w = NULL, *w;
>>>    	struct mtk_base_afe *afe;
>>>    	struct mt8188_afe_private *afe_priv;
>>>    	struct mtkaif_param *param;
>>> @@ -306,6 +312,18 @@ static int
>>> mt8188_mt6359_mtkaif_calibration(struct snd_soc_pcm_runtime *rtd)
>>>    		return 0;
>>>    	}
>>>    
>>> +	for_each_card_widgets(rtd->card, w) {
>>> +		if (!strcmp(w->name, "MTKAIF_PIN")) {
>>
>> if (strncmp(w->name, "MTKAIF_PIN", strlen(w->name) == 0) {
>> 	pin_w = w;
>> 	break;
>> }
>>
>> That's safer.
>>
> 
> If w->name is MTKAIF, the strncmp expression will return 0. However,
> the result is not expected. I prefer to keep strcmp here.
> 

You could also do, instead

if (strncmp(w->name, "MTKAIF_PIN", strlen("MTKAIF_PIN") == 0))

...solving your concern.

Regards,
Angelo

>>> +			pin_w = w;
>>> +			break;
>>> +		}
>>> +	}
>>> +
>>> +	if (!pin_w)
>>
>> Just a nitpick: you're checking for `if (pin_w)` later in this
>> function, so
>> to increase readability please do the same here.
>>
>> if (pin_w)
>> 	dapm_pinctrl_event(...)
>> else
>> 	dev_dbg(...)
>>
> 
> OK. I will update it in v2.
> 
> Thanks,
> Trevor

Re: [PATCH 1/3] ASoC: mediatek: mt8188-mt6359: support dynamic pinctrl

Posted by Trevor Wu (吳文良) 2 years, 6 months ago

On Wed, 2023-07-26 at 08:43 +0200, AngeloGioacchino Del Regno wrote:
> Il 26/07/23 04:19, Trevor Wu (吳文良) ha scritto:
> > On Tue, 2023-07-25 at 09:06 +0200, AngeloGioacchino Del Regno
> > wrote:
> > > Il 25/07/23 05:53, Trevor Wu ha scritto:
> > > > To avoid power leakage, it is recommended to replace the
> > > > default
> > > > pinctrl
> > > > state with dynamic pinctrl since certain audio pinmux functions
> > > > can
> > > > remain in a HIGH state even when audio is disabled. Linking
> > > > pinctrl
> > > > with
> > > > DAPM using SND_SOC_DAPM_PINCTRL will ensure that audio pins
> > > > remain
> > > > in
> > > > GPIO mode by default and only switch to an audio function when
> > > > necessary.
> > > > 
> > > > Signed-off-by: Trevor Wu <trevor.wu@mediatek.com>
> > > > ---
> > > >    sound/soc/mediatek/mt8188/mt8188-mt6359.c | 21
> > > > +++++++++++++++++++++
> > > >    1 file changed, 21 insertions(+)
> > > > 
> > > > diff --git a/sound/soc/mediatek/mt8188/mt8188-mt6359.c
> > > > b/sound/soc/mediatek/mt8188/mt8188-mt6359.c
> > > > index 7c9e08e6a4f5..667d79f33bf2 100644
> > > > --- a/sound/soc/mediatek/mt8188/mt8188-mt6359.c
> > > > +++ b/sound/soc/mediatek/mt8188/mt8188-mt6359.c
> > > > @@ -246,6 +246,11 @@ static const struct snd_soc_dapm_widget
> > > > mt8188_mt6359_widgets[] = {
> > > >    	SND_SOC_DAPM_MIC("Headset Mic", NULL),
> > > >    	SND_SOC_DAPM_SINK("HDMI"),
> > > >    	SND_SOC_DAPM_SINK("DP"),
> > > > +
> > > > +	/* dynamic pinctrl */
> > > > +	SND_SOC_DAPM_PINCTRL("ETDM_SPK_PIN", "aud_etdm_spk_on",
> > > > "aud_etdm_spk_off"),
> > > > +	SND_SOC_DAPM_PINCTRL("ETDM_HP_PIN", "aud_etdm_hp_on",
> > > > "aud_etdm_hp_off"),
> > > > +	SND_SOC_DAPM_PINCTRL("MTKAIF_PIN", "aud_mtkaif_on",
> > > > "aud_mtkaif_off"),
> > > >    };
> > > >    
> > > >    static const struct snd_kcontrol_new
> > > > mt8188_mt6359_controls[] = {
> > > > @@ -267,6 +272,7 @@ static int
> > > > mt8188_mt6359_mtkaif_calibration(struct snd_soc_pcm_runtime
> > > > *rtd)
> > > >    		snd_soc_rtdcom_lookup(rtd, AFE_PCM_NAME);
> > > >    	struct snd_soc_component *cmpnt_codec =
> > > >    		asoc_rtd_to_codec(rtd, 0)->component;
> > > > +	struct snd_soc_dapm_widget *pin_w = NULL, *w;
> > > >    	struct mtk_base_afe *afe;
> > > >    	struct mt8188_afe_private *afe_priv;
> > > >    	struct mtkaif_param *param;
> > > > @@ -306,6 +312,18 @@ static int
> > > > mt8188_mt6359_mtkaif_calibration(struct snd_soc_pcm_runtime
> > > > *rtd)
> > > >    		return 0;
> > > >    	}
> > > >    
> > > > +	for_each_card_widgets(rtd->card, w) {
> > > > +		if (!strcmp(w->name, "MTKAIF_PIN")) {
> > > 
> > > if (strncmp(w->name, "MTKAIF_PIN", strlen(w->name) == 0) {
> > > 	pin_w = w;
> > > 	break;
> > > }
> > > 
> > > That's safer.
> > > 
> > 
> > If w->name is MTKAIF, the strncmp expression will return 0.
> > However,
> > the result is not expected. I prefer to keep strcmp here.
> > 
> 
> You could also do, instead
> 
> if (strncmp(w->name, "MTKAIF_PIN", strlen("MTKAIF_PIN") == 0))
> 
> ...solving your concern.
> 
> 

From my understanding, strncmp is utilized to determine a string begins
with a particular prefix while strcmp is used to compare a whole
string. In this scenario, I wish to verify if the widget name is
exactly 'MTKAIF_PIN', so I believe using strcmp would be more
appropriate.

Using either strlen(w->name) or strlen("MTKAIF_PIN") may lead to
incorrect results when w->name is either MTKAIF or MTKAIF_PIN1.

Thanks,
Trevor

Re: [PATCH 1/3] ASoC: mediatek: mt8188-mt6359: support dynamic pinctrl

Posted by AngeloGioacchino Del Regno 2 years, 6 months ago

Il 26/07/23 13:36, Trevor Wu (吳文良) ha scritto:
> On Wed, 2023-07-26 at 08:43 +0200, AngeloGioacchino Del Regno wrote:
>> Il 26/07/23 04:19, Trevor Wu (吳文良) ha scritto:
>>> On Tue, 2023-07-25 at 09:06 +0200, AngeloGioacchino Del Regno
>>> wrote:
>>>> Il 25/07/23 05:53, Trevor Wu ha scritto:
>>>>> To avoid power leakage, it is recommended to replace the
>>>>> default
>>>>> pinctrl
>>>>> state with dynamic pinctrl since certain audio pinmux functions
>>>>> can
>>>>> remain in a HIGH state even when audio is disabled. Linking
>>>>> pinctrl
>>>>> with
>>>>> DAPM using SND_SOC_DAPM_PINCTRL will ensure that audio pins
>>>>> remain
>>>>> in
>>>>> GPIO mode by default and only switch to an audio function when
>>>>> necessary.
>>>>>
>>>>> Signed-off-by: Trevor Wu <trevor.wu@mediatek.com>
>>>>> ---
>>>>>     sound/soc/mediatek/mt8188/mt8188-mt6359.c | 21
>>>>> +++++++++++++++++++++
>>>>>     1 file changed, 21 insertions(+)
>>>>>
>>>>> diff --git a/sound/soc/mediatek/mt8188/mt8188-mt6359.c
>>>>> b/sound/soc/mediatek/mt8188/mt8188-mt6359.c
>>>>> index 7c9e08e6a4f5..667d79f33bf2 100644
>>>>> --- a/sound/soc/mediatek/mt8188/mt8188-mt6359.c
>>>>> +++ b/sound/soc/mediatek/mt8188/mt8188-mt6359.c
>>>>> @@ -246,6 +246,11 @@ static const struct snd_soc_dapm_widget
>>>>> mt8188_mt6359_widgets[] = {
>>>>>     	SND_SOC_DAPM_MIC("Headset Mic", NULL),
>>>>>     	SND_SOC_DAPM_SINK("HDMI"),
>>>>>     	SND_SOC_DAPM_SINK("DP"),
>>>>> +
>>>>> +	/* dynamic pinctrl */
>>>>> +	SND_SOC_DAPM_PINCTRL("ETDM_SPK_PIN", "aud_etdm_spk_on",
>>>>> "aud_etdm_spk_off"),
>>>>> +	SND_SOC_DAPM_PINCTRL("ETDM_HP_PIN", "aud_etdm_hp_on",
>>>>> "aud_etdm_hp_off"),
>>>>> +	SND_SOC_DAPM_PINCTRL("MTKAIF_PIN", "aud_mtkaif_on",
>>>>> "aud_mtkaif_off"),
>>>>>     };
>>>>>     
>>>>>     static const struct snd_kcontrol_new
>>>>> mt8188_mt6359_controls[] = {
>>>>> @@ -267,6 +272,7 @@ static int
>>>>> mt8188_mt6359_mtkaif_calibration(struct snd_soc_pcm_runtime
>>>>> *rtd)
>>>>>     		snd_soc_rtdcom_lookup(rtd, AFE_PCM_NAME);
>>>>>     	struct snd_soc_component *cmpnt_codec =
>>>>>     		asoc_rtd_to_codec(rtd, 0)->component;
>>>>> +	struct snd_soc_dapm_widget *pin_w = NULL, *w;
>>>>>     	struct mtk_base_afe *afe;
>>>>>     	struct mt8188_afe_private *afe_priv;
>>>>>     	struct mtkaif_param *param;
>>>>> @@ -306,6 +312,18 @@ static int
>>>>> mt8188_mt6359_mtkaif_calibration(struct snd_soc_pcm_runtime
>>>>> *rtd)
>>>>>     		return 0;
>>>>>     	}
>>>>>     
>>>>> +	for_each_card_widgets(rtd->card, w) {
>>>>> +		if (!strcmp(w->name, "MTKAIF_PIN")) {
>>>>
>>>> if (strncmp(w->name, "MTKAIF_PIN", strlen(w->name) == 0) {
>>>> 	pin_w = w;
>>>> 	break;
>>>> }
>>>>
>>>> That's safer.
>>>>
>>>
>>> If w->name is MTKAIF, the strncmp expression will return 0.
>>> However,
>>> the result is not expected. I prefer to keep strcmp here.
>>>
>>
>> You could also do, instead
>>
>> if (strncmp(w->name, "MTKAIF_PIN", strlen("MTKAIF_PIN") == 0))
>>
>> ...solving your concern.
>>
>>
> 
>  From my understanding, strncmp is utilized to determine a string begins
> with a particular prefix while strcmp is used to compare a whole
> string. In this scenario, I wish to verify if the widget name is
> exactly 'MTKAIF_PIN', so I believe using strcmp would be more
> appropriate.
> 
> Using either strlen(w->name) or strlen("MTKAIF_PIN") may lead to
> incorrect results when w->name is either MTKAIF or MTKAIF_PIN1.
> 
> Thanks,
> Trevor

strcmp() and strncmp() are the same; except strncmp() compares *at most* `n` bytes,
where `n` is my `strlen("MTKAIF_PIN")`.

 From Linux man pages....
https://www.man7.org/linux/man-pages/man3/strcmp.3.html

Regards,
Angelo

Re: [PATCH 1/3] ASoC: mediatek: mt8188-mt6359: support dynamic pinctrl

Posted by Trevor Wu (吳文良) 2 years, 6 months ago

On Wed, 2023-07-26 at 13:54 +0200, AngeloGioacchino Del Regno wrote:

..snip..

> > > > > > 
> > > > > > @@ -306,6 +312,18 @@ static int
> > > > > > mt8188_mt6359_mtkaif_calibration(struct snd_soc_pcm_runtime
> > > > > > *rtd)
> > > > > >     		return 0;
> > > > > >     	}
> > > > > >     
> > > > > > +	for_each_card_widgets(rtd->card, w) {
> > > > > > +		if (!strcmp(w->name, "MTKAIF_PIN")) {
> > > > > 
> > > > > if (strncmp(w->name, "MTKAIF_PIN", strlen(w->name) == 0) {
> > > > > 	pin_w = w;
> > > > > 	break;
> > > > > }
> > > > > 
> > > > > That's safer.
> > > > > 
> > > > 
> > > > If w->name is MTKAIF, the strncmp expression will return 0.
> > > > However,
> > > > the result is not expected. I prefer to keep strcmp here.
> > > > 
> > > 
> > > You could also do, instead
> > > 
> > > if (strncmp(w->name, "MTKAIF_PIN", strlen("MTKAIF_PIN") == 0))
> > > 
> > > ...solving your concern.
> > > 
> > > 
> > 
> >  From my understanding, strncmp is utilized to determine a string
> > begins
> > with a particular prefix while strcmp is used to compare a whole
> > string. In this scenario, I wish to verify if the widget name is
> > exactly 'MTKAIF_PIN', so I believe using strcmp would be more
> > appropriate.
> > 
> > Using either strlen(w->name) or strlen("MTKAIF_PIN") may lead to
> > incorrect results when w->name is either MTKAIF or MTKAIF_PIN1.
> > 
> > Thanks,
> > Trevor
> 
> strcmp() and strncmp() are the same; except strncmp() compares *at
> most* `n` bytes,
> where `n` is my `strlen("MTKAIF_PIN")`.
> 
>  From Linux man pages....
> 
https://urldefense.com/v3/__https://www.man7.org/linux/man-pages/man3/strcmp.3.html__;!!CTRNKA9wMg0ARbw!n-X-ckbkVYv4cbrhpwb2f7NH6sQkxx1czmCU2-q5BtSOhQU0C68wj9JNcu47YfbYeTpVEzjYQVXGuQb5ulpeWwKjnVMdZpg$ 
>  
> 
> 

Hi Angelo,

My concern is that strncmp() compares at most `n` bytes, where `n` is
the length of the string 'MTKAIF_PIN'. For instance, if both
'MTKAIF_PIN' and 'MTKAIF_PINMUX' exist in the widget list, they will
both enter execute_something() function below when executing this code:
 
if (strncmp(w->name, "MTKAIF_PIN", strlen("MTKAIF_PIN")) == 0) { 	
 execute_something(); 
}. 

This is not my expected scenario. To prevent this problem, we can use
strcmp() instead of strncmp(). strcmp() compares two strings until it
finds a difference or reaches the end of one of them. Therefore, it
will compare the entire string 'MTKAIF_PIN' with w->name and make sure
that only do execute_something() when w->names is the same as
'MTKAIF_PIN'.

Thanks,
Trevor