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:

quoted

quoted

quoted

quoted

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

quoted

quoted

+	}
+
+	pm_runtime_get_sync(afe->dev);

test if this worked?

Yes, if I didn't add pm_runtime_get_sync here, the calibration
failed.

quoted

quoted

+	mt6359_mtkaif_calibration_enable(cmpnt_codec);
+

[...]

quoted

quoted

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

quoted

quoted

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

quoted

quoted

+	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 [off-list ref]");
+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?

quoted

quoted

+MODULE_ALIAS("mt8195_mt6359_rt1011_rt5682 soc card");

Gentle ping.

Thanks,
Trevor


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel