Re: [PATCH v2 02/13] ASoC: mediatek: mt8189: Propagate APLL enable errors

[Bui Duc Phuc]

> You have reasons, of course, but have a wider view and check the full picture:
> apll{1,2}_enable() is called only by mtk_apll_event(), which is a DAPM supply.
>
> That call can only happen if the device is not in suspended state, so can happen
> only after a call to
>
>         regcache_cache_only(afe->regmap, false);
>         regcache_sync(afe->regmap);
>
> is done in mt8189_afe_runtime_resume().
>

I don't think runtime resume always succeeds.
However, with the code as it is now, it can indeed return success as
you described.

> > If that assumption were ever violated by a bug elsewhere (
>
>
> > a PM race,
>
> That would have worst consequences, and your system won't be saved by this error
> check because it would crash way before reaching that...
>
> > wrong resume ordering,
>
> If limited to ASoC, that would break most (if not all) sound drivers, as that'd
> happen at the API level, and would get fixed immediately since that'd affect way
> too many platforms and architectures.
>
> > a register missing from readable_reg, etc.),
>
> ..and that would be a driver bug, which is not the case here: the error check
> though would be useful during development where you definitely want to paranoidly
> check for literally everything, even "useless" things... but this driver definitely
> is not in development stage, is it? :-)
>

Oh, being upstream means it's already past the development stage now? :-)

> > this is where it would
> > get caught and reported,
>
> ...but then you still wouldn't be able to do anything about it during runtime, as
> there would be no way to handle this and fix it: the driver would still act in a
> broken manner.
>

I agree it may not be recoverable at runtime, but returning the error prevents
the driver from continuing as if it succeeded and makes the failure
visible to the caller.

Best regards,
Phuc