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

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

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

On Mon, Mar 19, 2012 at 03:54:19PM +0100, Ola Lilja wrote:

> We have a routing situation that could be rendered something like this:
> 
>                             D----->Speaker
>                             |
> I2S (playback)------>A----->B----->C----->Headset

> In this case, let's say that the bit at B needs to be set in HW before C is set
> to avoid pop/click noises.

This looks very standard, you've got a PGA at B and either further PGAs
or output drivers at C and D.

> If we just use a normal playback-switch that is associated with the bit at C,
> then this switch would directly set the C bit before the stream is active and
> then later when the stream is activated, the other widgets are enabled thus B
> would be set (i.e. after C was set which is not the desired order).
> Our solution to handle this is to introduce a virtual switch (in the form of an
> enum_virt) between B and Headset.

It sounds like this non-specific register bit is just the power control
for C in which case it should just be a DAPM widget and machines should
be using a pin switch or jack pins for the outputs.

> I2S (playback)------>A----->B----->C----->Headset
>                             ^
>                             |
>          LineIn----->D------E----->F----->I2S (capture)

> As you can see neither of the endpoints "belongs" to the bypass-path (E->B) and
> therefore I can't see how it should be possible to use a simple pin switch here
> as it would always affect other paths.

Again, this looks *very* standard - it's just a totally normal
bypass/sidetone path as far as I can see.  B is a mixer here and
presumably there's some control on B which turns on and off the path?

Depending on how the path is used you might want to mark the route as a
weak route to suppress power up from that path alone but really as with
the first example this doesn't seem at all unusual unless there's more
going on than your description.

> >> 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.

> OK, I was trying to understand what you meant with these comments:
> "breaking the device model" and "just embed a trivial input
> driver in the CODEC for the vibra if it's small enough."
> Could you explain this abit more?

The device model is this whole idea that we have devices which are
matched up automatically by the core.  "Embedding" means "putting in" -
see for example wm8962 which has a tiny little input driver in it.

> > 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.

> I agree that the fact that it needs power is not specific to the board, but we
> could have different regulators feeding the codec-chip. The regulators could be

You've completely failed to understand how requesting supplies in the
regulator API works, a regulator API like you describe would mean that
no chip driver was ever able to request a supply for itself.  The whole
point of the machine interface is to provide a mechanism for drivers to
talk about their supplies without having to know the details of how
they're wired up on an individual board.

Since your CODEC driver is a driver for a chip your CODEC driver should
be requesting whatever the supplies the chip has.

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

> I have looked alot at other drivers, but in many cases I've found that our
> situation looks pretty different and often more complex.
> See the last comment below for more info on our regulator-usage.

I'm sorry but really I don't see anything in the text below which sounds
in the least bit unusual.  Could you be specific about what you think is
special about your device and/or system?

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

> This is all our regulator-widgets, currently located in the machine-driver and
> connected to chains located in the codec-driver:

Things that are in the machine driver are not in the CODEC driver and
need to be cut and pasted into individual machine drivers.  If it's
something that's genuinely machine specific that's fine but we're
talking about basic core device supplies here.

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

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