Re: [PATCH 13/16] ASoC: codecs: Add AB8500 codec-driver

[Mark Brown <broonie@opensource.wolfsonmicro.com>]

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

On Mon, Mar 19, 2012 at 09:07:16AM +0100, Ola Lilja wrote:
> On 03/17/2012 11:31 PM, Mark Brown wrote:

> > > We need to break chains on very specific positions to be able to
> > > affect only the parts intended. We also have switches (e.g. loopback)
> > > that is not belonging to a specific pin but is a brigde-switch in the
> > > middle of two chains, thus I don't think we can use what you suggest.

> > But if you have a control that actually does something then surely there
> > should be some interaction with the chip when it is changed?

> No, we don't want the control to do anything with the chip before
> playback/capture starts, but rather just indicate that a specific path
> is active, and then when the stream starts the chain gets complete and
> the widgets then do all interaction with the chip in the order that
> our HW requires. There is no bit that we can set prior to the
> execution of the DAPM-chain, without introducing clicks.

In the case of things that go external pins then I'd really expect the
existing solution with pin switches to work, in other cases something
else may be needed but we need to understand what you're trying to do.
The issues with internal paths may need something but please be more
concrete about what the actual register controls are and why you're
doing such unusual things.  The fact that you're using isolated switches
and there's no mixers involved is really surprisng.

> > One of the nice things about open source code is that it's always
> > possible to extend the core if required.

> OK, so do you want us to make it possible to have NOPM for a
> playback/capture-switch? Can you confirm that this is OK and that it
> is possible to accomplish without destroying any mechanism in how it
> works today?

I'm saying that if you're doing things within your driver that go trough
contortions or break the userspace interfaces because the frameworks
didn't support things then you should be fixing the frameworks rather
than doing whatever in the driver.  This sort of thing seems to be at
the root of several of the problems I'm seeing here, it's similar to all
the stuff you're doing with regulators.

> So, what you want is another device/driver pair where our current
> acc.det.-driver adds a driver to the device we add in the ASoC-driver?

I can't parse what you're saying here, sorry.

> We are thinking of modifying so that we put the Android-vibra-functionality in
> userspace, and then it can use vibra through the controls we provide in the
> ASoC-driver.

That'd obviously remove any kernel level problems.

> Regarding the regulators, I was under the impression that everything
> outside the actual codec, we could put in the machine-driver (which I
> think looks pretty good).  This would make a clear distinction between
> all codec-register-handling and usage of those external frameworks
> (regulators and clocks). I have started to move the code location of

No.  That's exactly the sort of stuff we don't want to see.  The fact
that the device needs power to operate isn't something that's specific
to a particular board, it's a property of the silicon.

> regulators/clocks into the codec-file and this results
> in that the machine-driver is basically empty of functionality and the
> codec-file is growing even bigger. Is this really what we want?

Yes, of course!  You need to get away from the idea that the only board
your device will ever be used on is the reference board, things that are
generic to the device should be supported by the device driver for the
device.  If there's a device driver for the part we shouldn't be in the
situation where any new system using the device needs to replicate basic
functionality needed to get the device working.

> I'm also not sure if you have seen that we actually do control ALL regulators
> and clocks via DAPM. The main power-regulator is controlled through a

I've not suggested that.  Look at how other drivers manage their power.

> You are right that a large part of this code is there because it is needed by
> external requirements, but it does not mean that DAPM is not controlling the
> regulators and clocks, just that there is another interface that can also turn
> on/off the power.

I'm not seeing any code in the driver which manages the regulators...

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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