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

[AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>]

On 9/15/26 08:55, Bui Duc Phuc wrote:
> Hi Mark , Angelo
> 
> Thanks both for the review.
> 
>>
>>>>   Though practically speaking the error
>>>> handling ends up being the same as if they couldn't fail since if
>>>> something goes wrong it's generally catastrophic stuff like locking the
>>>> core up completely so the end result is the same.
>>
>>> That was also an implicit point (that should've been explicit from me): if anything
>>> goes horribly wrong here, it means that it already went horribly wrong "some
>>> function calls ago", and the platform likely already locked up as you suggested.
>>
>>> In any case, I'm not against doing error checking, it's just about not doing it
>>> when it's really useless (I'm sure you understand my reasons), and I believe this
>>> specific case is one of those.
>>
>>> That said, should you prefer having error checks in such places... it's not a
>>> performance path, so I don't really have strong opinions really.
>>
>> Yeah, I think it's fine and sensible to skip the error checks in cases
>> where we can't really do anything constructive about the error.
> 
> As far as I understand it, regmap_update_bits() doesn't go straight to MMIO.
> It first passes through the regmap core, which has checks independent of
> the underlying bus:
> 
> https://elixir.bootlin.com/linux/v7.3-rc2/source/drivers/base/regmap/regmap.c#L2834
> 
> ----------------------------------------------
> if (map->cache_only)
>          return -EBUSY;
> 
> if (!regmap_readable(map, reg))
>          return -EIO;
> ----------------------------------------------
> 
> So, in my understanding, checking ret here isn't just unnecessary
> defensive programming
> for an MMIO write, it guards an assumption about PM/cache ordering, namely that
> the regmap is already out of cache_only mode by the time this runs.

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().

> 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? :-)

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

Cheers,
Angelo

> rather than the driver silently assuming the enable succeeded when it didn't.
> 
> Best regards,
> Phuc