Re: [PATCH v3 3/9] ASoC: mediatek: mt8192: support i2s in platform driver

[Jiaxin Yu <jiaxin.yu@mediatek.com>]

On Fri, 2020-10-30 at 14:37 +0000, Mark Brown wrote:
> On Sat, Oct 24, 2020 at 03:58:53PM +0800, Jiaxin Yu wrote:
> 
> > +static const struct soc_enum mt8192_i2s_enum[] = {
> > +	SOC_ENUM_SINGLE_EXT(ARRAY_SIZE(mt8192_i2s_hd_str),
> > +			    mt8192_i2s_hd_str),
> > +};
> 
> Why is this declared as a single element array?  It just makes all the
> usages look odd for no obvious gain.
> 
> > +static int mtk_i2s_en_event(struct snd_soc_dapm_widget *w,
> > +			    struct snd_kcontrol *kcontrol,
> > +			    int event)
> 
> > +	dev_info(cmpnt->dev, "%s(), name %s, event 0x%x\n",
> > +		 __func__, w->name, event);
> 
> This should be dev_dbg() at most, _info() will be too noisy in the logs.
> Same for a lot of functions, including the stream callbacks.
> 
> > +static int mtk_i2s_hd_en_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);
> > +
> > +	dev_info(cmpnt->dev, "%s(), name %s, event 0x%x\n",
> > +		 __func__, w->name, event);
> > +
> > +	return 0;
> > +}
> 
> This should just be removed entirely, there's trace in the core if you
> need logging in production systems - the tracepoints in particular are
> good for just leaving on all the time without adding overhead.
> 
> > +	return (i2s_need_apll == cur_apll) ? 1 : 0;
> 
> Please write normal conditional statements to improve legiblity.
> 
> > +	if (rate == 44100)
> > +		regmap_write(afe->regmap, AFE_ASRC_2CH_CON3, 0x001B9000);
> > +	else if (rate == 32000)
> > +		regmap_write(afe->regmap, AFE_ASRC_2CH_CON3, 0x140000);
> > +	else
> > +		regmap_write(afe->regmap, AFE_ASRC_2CH_CON3, 0x001E0000);
> 
> This would be better written as a switch statement.
> 
> > +	/* Calibration setting */
> > +	regmap_write(afe->regmap, AFE_ASRC_2CH_CON4, 0x00140000);
> > +	regmap_write(afe->regmap, AFE_ASRC_2CH_CON9, 0x00036000);
> > +	regmap_write(afe->regmap, AFE_ASRC_2CH_CON10, 0x0002FC00);
> > +	regmap_write(afe->regmap, AFE_ASRC_2CH_CON6, 0x00007EF4);
> > +	regmap_write(afe->regmap, AFE_ASRC_2CH_CON5, 0x00FF5986);
> 
> Are you sure this isn't system dependant?
Hi Mark,
Yes, this is a system independent setting. And I fixed other comments
you pointed out then send "PATCH v4".