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 topckgencontroller\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 channelfailed\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