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

[Bui Duc Phuc]

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.
If that assumption were ever violated by a bug elsewhere (a PM race,
wrong resume ordering,
a register missing from readable_reg, etc.), this is where it would
get caught and reported,
rather than the driver silently assuming the enable succeeded when it didn't.

Best regards,
Phuc