On Wed, Oct 06, 2021 at 11:37:24AM -0500, Pierre-Louis Bossart wrote:
> On 10/6/21 11:23 AM, Andy Shevchenko wrote:
> > On Wed, Oct 06, 2021 at 10:51:52AM -0500, Pierre-Louis Bossart wrote:
> >> On 10/6/21 10:04 AM, Andy Shevchenko wrote:
...
> >>> if (SND_SOC_DAPM_EVENT_ON(event)) {
> >>> - if (byt_rt5651_quirk & BYT_RT5651_MCLK_EN) {
> >>> - ret = clk_prepare_enable(priv->mclk);
> >>> - if (ret < 0) {
> >>> - dev_err(card->dev,
> >>> - "could not configure MCLK state");
> >>> - return ret;
> >>> - }
> >>> + ret = clk_prepare_enable(priv->mclk);
> >>> + if (ret < 0) {
> >>> + dev_err(card->dev, "could not configure MCLK state");
> >>> + return ret;
> >>> }
> >>
> >> I don't get why you removed the test on the BYT_RT5651_MCLK_EN quirk,
> >> see below it was designed as a fall-back mode. We don't want to return
> >> an error when we know the clock is not present/desired.
> >
> > Why should we do a unneeded test? When we switch to the optional, there
> > will be no error from these CCF APIs. Besides that it drops indentation
> > level and makes code neat.
>
> By looking at this code only one cannot really visualize that it's a
> no-op. I personally prefer to see explicit intent rather than have to
> dig hundreds of lines below what this clock is optional.
>
> I am also not even sure that in real products this clock is actually
> optional,
The code tells that it's optional. If it's not the case, the code has
to be fixed accordingly.
> the default is to make use of it:
>
> #define BYT_RT5651_DEFAULT_QUIRKS (BYT_RT5651_MCLK_EN | \
>
> and the only platform without this clock is "Minnowboard Max B3" -
> probably not used by anyone. I fried mine a long time ago.
>
> We'd need to Hans to comment on this since he's really the only one
> maintaining this code.
Additional pair of eyes and a brain is always good to have!
...
> >> that part in the probe looks fine, but the changes above are controversial.
> >
> > I didn't get. How controversial? Why? The whole point of _optional is to get
> > rid of unneeded checks (since they are _anyway_ be called).
>
> It's inconsistent since you kept the following part but no longer use it:
It's used in some cases, but not with CLK APIs where it's redundant.
Otherwise I would remove it completely (actually it's possible to replace
it with the !priv->mclk test.
> + /*
> + * Fall back to bit clock usage when clock is not
> + * available likely due to missing dependencies.
> + */
> + if (!priv->mclk)
> + byt_rt5651_quirk &= ~BYT_RT5651_MCLK_EN;
--
With Best Regards,
Andy Shevchenko
