Re: [RFC] ASoC: sgtl5000: Remove cache support

[Eric Nelson <eric.nelson@boundarydevices.com>]

Hi Mark,

On 05/08/2013 03:31 AM, Mark Brown wrote:
> On Tue, May 07, 2013 at 03:06:39PM -0500, Matt Sealey wrote:
>> On Mon, May 6, 2013 at 5:06 PM, Mark Brown <broonie@kernel.org> wrote:
 >
 > ...
 >
>> Because that's what Freescale did in their code as per their comments
>> - if only because they needed to do this kind of setup before the
>> cache/defaults were populated. I assume they populate the cache from
>> the chip at probe because the defaults change per revision and the
>> presence of VDDD or not may change some of the registers in the cache.
>
> We just have no idea why they're doing what they're doing; this is all
> guesswork.  I've not seen anything that indicates that anyone who's sent
> any code has any understanding of what they're changing.
>
>> It may also be on boards like i.MX6 Sabrelite that the "defaults" are
>> not the "defaults" over a warm reboot, since the chip never powers off
>> (just "optionally" loses clock depending on whether CLKO is
>> reconfigured or not at some point) and probably retains everything you
>> ever set.

That's exactly right for SABRE Lite. Hitting the reset button or
invoking a processor reset doesn't reset the SGTL5000, so when
Linux boots, the registers are in an unknown state.

>
> This is why most drivers do a hardware reset of the device on gaining
> control of it, in order to ensure that the hardware is in a known state.
> I see  that the driver doesn't do that except through the regulators.  I
> see this driver doesn't do that which isn't terribly clever, though it's
> not clear that the chip has suitable support.
>
>> We want to do two possible things...
>> * At probe, dump some known good defaults into the chip and use those as
 >> a cache
>> * At probe, read out all the registers assuming they're already known
>> good defaults into a cache
>
> Clearly the first option is more sane since otherwise you've no idea if
> the chip is configured sensibly.
>

I agree. It seems that the 'cache' is currently mixing two things:
     1.) **presumed** power-on defaults for the chip, and
     2.) cached values of the registers

Since we can't ensure that we're starting with power-on-reset, the
entire initial cache is potentially wrong.

>> The intent being in the first case to make for damn sure the codec is
>> in a well known state, and in the second case JUST to reduce i2c
>> writes further down the line. Freescale seem to like the second one,
>
> This isn't the sole function of the register cache, it's also used to
> implement most of suspend and resume and to allow the device to be
> powered off when idle but still present controls.
>
>> Mainline is doing the first set, although it's definitely possible
>> those defaults are not the true defaults if they were derived from a
>> board like the Sabrelite where a warm reboot might retain all the
>> state from previous boots.. the precedence of that list of defaults is
>> basically unknown.. and I recall Freescale updating them in the BSP a
>> lot.
>
> I'd *really* hope that the defaults come from the chip datasheet.  If
> they don't that's extremely disappointing.
>
>> So the problem here might be some quirky board designs, a random
>> implementation of an erratum workaround with no documentation, a bunch
>
> My impression based on what I'm seeing here, including things like your
> comments about lots of register default changes, is that people working
> on the driver haven't had much understanding of what they're doing and
> have got confused as a result.  Sounds like a good first step is getting
> the register defaults sane...
>

An initial pass of writing all of the default register values with a
sort of 'cache flush' appears to be the right thing.

Then it doesn't even matter if the values match the datasheet, or if
the spec changes over time. If the values are reasonable, the device
will function properly.