Re: [PATCH 02/11] ASoC: rt5651: Remove is_sys_clk_from_pll, it has ordering issues

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1.1: Type: text/plain, Size: 2438 bytes --]

On Wed, Feb 21, 2018 at 08:38:01PM +0100, Hans de Goede wrote:
> On 21-02-18 12:18, Mark Brown wrote:
> > On Tue, Feb 20, 2018 at 11:15:02PM +0100, Hans de Goede wrote:

> > Only if jack detection is enabled, if jack detection is not in use for
> > some reason then the PLL isn't required and should be powered off - this
> > is normally handled by having the jack detection code force enable
> > things.

> As the commit message tries to explain, the code this removes is fundamentally
> broken and this is not jack-detection related:

>    "dapm_power_widgets() first builds a list of which widgets to power-up
>     before actually powering any of them up. For dapm-supply widgets their
>     connected method, in our case is_sys_clk_from_pll() get called at this
>     point.

>     Before this commit is_sys_clk_from_pll() was looking at the actually
>     selected clock in the RT5651_GBL_CLK register. But the sysclk itself is
>     selected by another dapm-supply (the "Platform Clock" supply in the
>     machine driver) and any changes to that supply part of the same power-
>     transition get executed after building the list, causing
>     is_sys_clk_from_pll() to return the wrong value."

Right, but there's a couple of jumps in your reasoning with the actual
solution.

> As for that the PLL should be powered-off when not needed, I agree but that
> is controlled by the machine-driver through the "Platform Clock" dapm supply
> and if that leaves things on for some reason then that is the problem which
> we actually need to fix, e.g. this will also leave the MCLK input clk enabled
> needlessly.

I don't follow this bit, sorry.  If there's a DAPM supply that's being
enabled when it's not needed then we should just disable it.

> I really don't see how configuring a broken sysclk (GBL_CLK points to PLL1,
> but PLL-bit in PWR_ANLG2 is OFF) is ever a good thing, instead we should make
> sure we always switch to the RCCLK when we don't need the SYSCLK.

> Last force-enabling the PLL bit on all machines where we can do jack-detection
> (not only over-current, but the JD irq in general needs a valid sysclk) would
> in practice mean always force-enabling the PLL bit since almost all boards have
> jack-detection once we add the necessary quirks, so this really is not a solution.

A bit confused here as well, sorry - I think you're assuming I'm
suggesting some particular solution but I'm not sure what that is.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]