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