Re: [PATCH 1/2] ASoC: mediatek: mt8195: add machine driver with mt6359, rt1011 and rt5682

[Trevor Wu <trevor.wu@mediatek.com>]

Hi Pierre-Louis,

On Mon, 2021-09-13 at 18:24 +0800, Trevor Wu wrote:
> On Fri, 2021-09-10 at 11:47 -0500, Pierre-Louis Bossart wrote:
> > > 
> 
> > > +
> > > +	param->mtkaif_calibration_ok = false;
> > > +	for (i = 0; i < MT8195_MTKAIF_MISO_NUM; i++) {
> > > +		param->mtkaif_chosen_phase[i] = -1;
> > > +		param->mtkaif_phase_cycle[i] = 0;
> > > +		mtkaif_chosen_phase[i] = -1;
> > > +		mtkaif_phase_cycle[i] = 0;
> > > +	}
> > > +
> > > +	if (IS_ERR(afe_priv->topckgen)) {
> > > +		dev_info(afe->dev, "%s() Cannot find topckgen
> > > controller\n",
> > > +			 __func__);
> > > +		return 0;
> > 
> > is this not an error? Why not dev_err() and return -EINVAL or
> > something?
> > 
> 
> Should I still return an error, even if the caller didn't check it?
> 
> Based on my understanding, the calibration function is used to make
> the
> signal more stable. 
> Most of the time, mtkaif still works, even though the calibration
> fails.
> I guess that's why the caller(I refered to the implementation of
> mt8192.) didn't check the return value of calibration function.
> 
> 
> > > +	}
> > > +
> > > +	pm_runtime_get_sync(afe->dev);
> > 
> > test if this worked?
> > 
> 
> Yes, if I didn't add pm_runtime_get_sync here, the calibration
> failed.
> 
> > > +	mt6359_mtkaif_calibration_enable(cmpnt_codec);
> > > +
> > > 
[...]
> > > +	mt6359_set_mtkaif_calibration_phase(cmpnt_codec,
> > > +					    chosen_phase_1,
> > > +					    chosen_phase_2,
> > > +					    chosen_phase_3);
> > > +
> > > +	mt6359_mtkaif_calibration_disable(cmpnt_codec);
> > > +	pm_runtime_put(afe->dev);
> > > +
> > > +	param->mtkaif_calibration_ok = mtkaif_calibration_ok;
> > > +	param->mtkaif_chosen_phase[MT8195_MTKAIF_MISO_0] =
> > > chosen_phase_1;
> > > +	param->mtkaif_chosen_phase[MT8195_MTKAIF_MISO_1] =
> > > chosen_phase_2;
> > > +	param->mtkaif_chosen_phase[MT8195_MTKAIF_MISO_2] =
> > > chosen_phase_3;
> > > +	for (i = 0; i < MT8195_MTKAIF_MISO_NUM; i++)
> > > +		param->mtkaif_phase_cycle[i] = mtkaif_phase_cycle[i];
> > > +
> > > +	dev_info(afe->dev, "%s(), end, calibration ok %d\n",
> > > +		 __func__, param->mtkaif_calibration_ok);
> > 
> > dev_dbg?
> > 
> 
> Because we don't regard calibration failure as an error, it is a hint
> to show the calibration result.
> I prefer to keep dev_info here.
> Is it OK?
> 
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int mt8195_hdmitx_dptx_startup(struct snd_pcm_substream
> > > *substream)
> > > +{
> > > +	static const unsigned int rates[] = {
> > > +		48000
> > > +	};
> > > +	static const unsigned int channels[] = {
> > > +		2, 4, 6, 8
> > > +	};
> > > +	static const struct snd_pcm_hw_constraint_list
> > > constraints_rates = {
> > > +		.count = ARRAY_SIZE(rates),
> > > +		.list  = rates,
> > > +		.mask = 0,
> > > +	};
> > > +	static const struct snd_pcm_hw_constraint_list
> > > constraints_channels = {
> > > +		.count = ARRAY_SIZE(channels),
> > > +		.list  = channels,
> > > +		.mask = 0,
> > > +	};
> > 
> > you use the same const tables several times, move to a higher scope
> > and
> > reuse?
> > 
> 
> There is little difference in channels between these startup ops.
> 
> > > +	struct snd_soc_pcm_runtime *rtd =
> > > asoc_substream_to_rtd(substream);
> > > +	struct snd_pcm_runtime *runtime = substream->runtime;
> > > +	int ret;
> > > +
> > > +	ret = snd_pcm_hw_constraint_list(runtime, 0,
> > > +					 SNDRV_PCM_HW_PARAM_RATE,
> > > +					 &constraints_rates);
> > > +	if (ret < 0) {
> > > +		dev_err(rtd->dev, "hw_constraint_list rate failed\n");
> > > +		return ret;
> > > +	}
> > > +
> > > +	ret = snd_pcm_hw_constraint_list(runtime, 0,
> > > +					 SNDRV_PCM_HW_PARAM_CHANNELS,
> > > +					 &constraints_channels);
> > > +	if (ret < 0) {
> > > +		dev_err(rtd->dev, "hw_constraint_list channel
> > > failed\n");
> > > +		return ret;
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > 
> > > +
> > > +static struct platform_driver mt8195_mt6359_rt1011_rt5682_driver
> > > =
> > > {
> > > +	.driver = {
> > > +		.name = "mt8195_mt6359_rt1011_rt5682",
> > > +#ifdef CONFIG_OF
> > > +		.of_match_table = mt8195_mt6359_rt1011_rt5682_dt_match,
> > > +#endif
> > > +		.pm = &mt8195_mt6359_rt1011_rt5682_pm_ops,
> > > +	},
> > > +	.probe = mt8195_mt6359_rt1011_rt5682_dev_probe,
> > > +};
> > > +
> > > +module_platform_driver(mt8195_mt6359_rt1011_rt5682_driver);
> > > +
> > > +/* Module information */
> > > +MODULE_DESCRIPTION("MT8195-MT6359-RT1011-RT5682 ALSA SoC machine
> > > driver");
> > > +MODULE_AUTHOR("Trevor Wu <trevor.wu@mediatek.com>");
> > > +MODULE_LICENSE("GPL v2");
> > 
> > "GPL" is enough
> > 
> 
> I see many projects use GPL v2 here, and all mediatek projects use
> GPL
> v2, too.
> I'm not sure which one is better.
> Do I need to modify this?
> 

> 
> > > +MODULE_ALIAS("mt8195_mt6359_rt1011_rt5682 soc card");
> > > 

Gentle ping.

Thanks,
Trevor