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@xxxxxxxxxxxx>");
> > > +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
