On Mon, May 06, 2013 at 03:19:27PM -0500, Matt Sealey wrote:

I have asked before and I will ask again; does anyone have any insight

Not on the list you haven't as far as I can tell... this is the first I've heard of any concern about this code, I'd not be surprised if others were similarly unaware of any concerns.

The patch came from out of nowhere, and the comments seem to say that the LDO gets enabled on every chip higher than a specific version. Now, this is totally against the design documentation for the SGTL5000 which specifically states that using the internal LDO is quite possibly the *worst* power consuming case and NOT best practise for putting the codec on a design, with the sole benefit being reduced external component count.

Are you sure it's purely about power consumption? A common application for LDOs in this situation is to clean up noise on the incoming power supply, improving rejection of noise. This is mostly irrelevant for digital supplies but can be desirable for analogue ones. Of course this looks like it's being used as a digital supply so...

I have to say your explanation is a little curious, though - normally an LDO would require an external capacitor and so would either increase the component count (adding the capactior) or have no effect (if one is already present for decoupling). What is the supply for this LDO and where does the output go?

What happens if we just take out the internal LDO setup conditional on version, and only initialize it if VDDD is missing? Do boards that

If the LDO is enabled and connected to a supply which is also driven externally that's not a clever idea, the two regulators will both be trying to regulate the same supply.

I am not entirely sure how well the commit linked by Fabio would work on mainline since if it uses hw_read/write it should also be updating the cache with those values to be sure they can later be modified... the BSP code does that then fills the cache with the existing values afterwards.

That commit is just bogus in the way it interacts with the cache, half of it looks awfully like it's trying to work around some breakage in the way the cache was being set up.

So maybe we don't disable the cache, or it gets ported to regmap, but the order should be enable regulators in a non-cached way then fill the cache AFTERWARDS - the code at ToT mainline doesn't fix the cache

I'm sorry, this makes no sense to me. If we know the physical defaults for the device and we can get the device into that state then we should have no need to read from the cache. If we don't know what they are then they shouldn't be listed.

after it does regulator/power regs setup, I assume because cache filling is now somehow automated in ASoC (and if this is the case,

There is no cache filling to automate, if (as is the case here) register defaults are provided they're used.

then the cache io setup needs to be done after regulator/power setup, the defaults list/cache needs to be updated for those registers before any register caching io is done..). And maybe setting up the regulator is kind of pointless anyway (meaning that whole caching/not-caching thing just falls away?)

Again this makes no sense, it seems like you're very confused here. The whole purpose of the register defaults is to reflect the default values of the registers. The infrastructure relies on this being the case, otherwise it will get confused for example during cache sync when it decides if a write to the device is required.

I think the point that you lost me was your assumption that the regulator enable needs to be done without the cache. What makes you believe that this is the case?

Note that as I said in another post the driver is currently totally broken with regard to anything that actually powers off the device, there's never any attempt made to restore the register cache.