Re: [PATCH 04/12] ASoC: mediatek: mt8188: support adda in platform driver

[Trevor Wu (吳文良)]

On Tue, 2022-10-04 at 11:37 +0200, AngeloGioacchino Del Regno wrote:
> Il 30/09/22 16:56, Trevor Wu ha scritto:
> > 
..snip..
> 
> > +
> > +static int mt8188_adda_mtkaif_init(struct mtk_base_afe *afe)
> > +{
> > +	struct mt8188_afe_private *afe_priv = afe->platform_priv;
> > +	struct mtkaif_param *param = &afe_priv->mtkaif_params;
> > +	int delay_data;
> > +	int delay_cycle;
> > +	unsigned int mask = 0;
> > +	unsigned int val = 0;
> > +
> > +	/* set rx protocol 2 & mtkaif_rxif_clkinv_adc inverse */
> > +	mask = (MTKAIF_RXIF_CLKINV_ADC | MTKAIF_RXIF_PROTOCOL2);
> > +	val = (MTKAIF_RXIF_CLKINV_ADC | MTKAIF_RXIF_PROTOCOL2);
> > +
> > +	regmap_update_bits(afe->regmap, AFE_ADDA_MTKAIF_CFG0, mask,
> > val);
> 
> This should be
> 	regmap_set_bits(afe->regmap, AFE_ADDA_MTKAIF_CFG0,
> 			MTKAIF_RXIF_CLKINV_ADC |
> MTKAIF_RXIF_PROTOCOL2);

OK. I will replace all similar usages in V2.

> > +
> > +	mask = RG_RX_PROTOCOL2;
> > +	val = RG_RX_PROTOCOL2;
> > +	regmap_update_bits(afe->regmap, AFE_AUD_PAD_TOP, mask, val);
> 
> regmap_set_bits() again
> 

> > +
> > +	if (!param->mtkaif_calibration_ok) {
> > +		dev_info(afe->dev, "%s(), calibration
> > fail\n",  __func__);
> > +		return 0;
> > +	}
> > +
> > +	/* set delay for ch1, ch2 */
> > +	if (param->mtkaif_phase_cycle[MT8188_MTKAIF_MISO_0] >=
> > +	    param->mtkaif_phase_cycle[MT8188_MTKAIF_MISO_1]) {
> > +		delay_data = DELAY_DATA_MISO1;
> > +		delay_cycle =
> > +			param->mtkaif_phase_cycle[MT8188_MTKAIF_MISO_0] 
> > -
> > +			param-
> > >mtkaif_phase_cycle[MT8188_MTKAIF_MISO_1];
> > +	} else {
> > +		delay_data = DELAY_DATA_MISO0;
> > +		delay_cycle =
> > +			param->mtkaif_phase_cycle[MT8188_MTKAIF_MISO_1] 
> > -
> > +			param-
> > >mtkaif_phase_cycle[MT8188_MTKAIF_MISO_0];
> > +	}
> > +
> > +	val = 0;
> > +	mask = (MTKAIF_RXIF_DELAY_DATA | MTKAIF_RXIF_DELAY_CYCLE_MASK);
> > +	val |= MTKAIF_RXIF_DELAY_CYCLE(delay_cycle) &
> > +	       MTKAIF_RXIF_DELAY_CYCLE_MASK;
> 
> 	val = FIELD_PREP(MTKAIF_RXIF_DELAY_CYCLE_MASK, delay_cycle);
> 
> > +	val |= delay_data << MTKAIF_RXIF_DELAY_DATA_SHIFT;
> 
> 	val |= FIELD_PREP(MTKAIF_RXIF_DELAY_DATA, delay_data);
> 
> Can you please use bitfield access macros across the entire file (and
> the others)?
> This will both increase human readability and add compile-time checks
> on register
> fields.
> 

Thanks for your suggestion.
compile-time checks are helpful to find some unexpected errors.
I will update it in V2.

> > +	regmap_update_bits(afe->regmap, AFE_ADDA_MTKAIF_RX_CFG2, mask,
> > val);
> > +
> > +	return 0;
> > +}
> > +
> > +static int mtk_adda_mtkaif_cfg_event(struct snd_soc_dapm_widget
> > *w,
> > +				     struct snd_kcontrol *kcontrol,
> > +				     int event)
> > +{
> > +	struct snd_soc_component *cmpnt = snd_soc_dapm_to_component(w-
> > >dapm);
> > +	struct mtk_base_afe *afe =
> > snd_soc_component_get_drvdata(cmpnt);
> > +
> > +	dev_dbg(afe->dev, "%s(), name %s, event 0x%x\n",
> > +		__func__, w->name, event);
> > +
> > +	switch (event) {
> > +	case SND_SOC_DAPM_PRE_PMU:
> > +		mt8188_adda_mtkaif_init(afe);
> > +		break;
> > +	default:
> > +		break;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int mtk_adda_dl_event(struct snd_soc_dapm_widget *w,
> > +			     struct snd_kcontrol *kcontrol,
> > +			     int event)
> > +{
> > +	struct snd_soc_component *cmpnt = snd_soc_dapm_to_component(w-
> > >dapm);
> > +	struct mtk_base_afe *afe =
> > snd_soc_component_get_drvdata(cmpnt);
> > +
> > +	dev_dbg(afe->dev, "%s(), name %s, event 0x%x\n",
> > +		__func__, w->name, event);
> > +
> > +	switch (event) {
> > +	case SND_SOC_DAPM_POST_PMD:
> > +		/* should delayed 1/fs(smallest is 8k) = 125us before
> > afe off */
> > +		usleep_range(125, 135);
> > +		break;
> > +	default:
> > +		break;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static void mtk_adda_ul_mictype(struct mtk_base_afe *afe, bool
> > dmic)
> > +{
> > +	unsigned int reg = AFE_ADDA_UL_SRC_CON0;
> > +	unsigned int val = 0;
> > +	unsigned int mask;
> > +
> > +	mask = (UL_SDM3_LEVEL_CTL | UL_MODE_3P25M_CH1_CTL |
> > +		UL_MODE_3P25M_CH2_CTL);
> 
> 	val = (UL_SDM3_LEVEL_CTL | UL_MODE_3P25M_CH1_CTL |
> UL_MODE_3P25M_CH2_CTL);
> 
> > +
> > +	/* turn on dmic, ch1, ch2 */
> > +	if (dmic)
> 
> 		regmap_set_bits(afe->regmap, reg, val);
> 	else
> 		regmap_clear_bits(afe->regmap, reg, val);
> 
> 
OK. I will update this part in V2.

> > +		val = mask;
> > +
> > +	regmap_update_bits(afe->regmap, reg, mask, val);
> > +}
> > +
> 
> ..snip..
> 
> > +
> > +static int mtk_afe_adc_hires_connect(struct snd_soc_dapm_widget
> > *source,
> > +				     struct snd_soc_dapm_widget *sink)
> > +{
> > +	struct snd_soc_dapm_widget *w = source;
> > +	struct snd_soc_component *cmpnt = snd_soc_dapm_to_component(w-
> > >dapm);
> > +	struct mtk_base_afe *afe =
> > snd_soc_component_get_drvdata(cmpnt);
> > +	struct mt8188_afe_private *afe_priv = afe->platform_priv;
> > +	struct mtk_dai_adda_priv *adda_priv;
> > +
> > +	adda_priv = afe_priv->dai_priv[MT8188_AFE_IO_ADDA];
> > +
> > +	if (!adda_priv) {
> > +		dev_err(afe->dev, "%s adda_priv == NULL", __func__);
> > +		return 0;
> 
> 		return -EINVAL?
> 
dapm_supply_check_power doesn't handled error return value, so it seems
to be better to keep return 0 here.

> > +	}
> > +
> > +	return (adda_priv->ul_rate > ADDA_HIRES_THRES) ? 1 : 0;
> 
> 	return !!(adda_priv->ul_rate > ADDA_HIRES_THRES);
> 
OK. I will update it in V2.

> > +}
> > +
> > +static int mtk_afe_dac_hires_connect(struct snd_soc_dapm_widget
> > *source,
> > +				     struct snd_soc_dapm_widget *sink)
> > +{
> > +	struct snd_soc_dapm_widget *w = source;
> > +	struct snd_soc_component *cmpnt = snd_soc_dapm_to_component(w-
> > >dapm);
> > +	struct mtk_base_afe *afe =
> > snd_soc_component_get_drvdata(cmpnt);
> > +	struct mt8188_afe_private *afe_priv = afe->platform_priv;
> > +	struct mtk_dai_adda_priv *adda_priv;
> > +
> > +	adda_priv = afe_priv->dai_priv[MT8188_AFE_IO_ADDA];
> > +
> > +	if (!adda_priv) {
> > +		dev_err(afe->dev, "%s adda_priv == NULL", __func__);
> > +		return 0;
> 
> same here
> 
> > +	}
> > +
> > +	return (adda_priv->dl_rate > ADDA_HIRES_THRES) ? 1 : 0;
> > +}
> > +
> 
> ..snip..
> 
> Regards,
> Angelo
> 
>