[alsa-devel] [PATCH 10/11] ASoC: codecs: Add AB8500 codec-driver

[Mark Brown]

On Wed, May 09, 2012 at 09:48:35AM +0200, Ola Lilja wrote:
> On 05/08/2012 08:27 PM, Mark Brown wrote:
> > On Tue, May 08, 2012 at 03:57:30PM +0200, Ola Lilja wrote:

> > So, you were adding this just for debug...  Let's not do that.  There's
> > already diagnostic infrastructure in the regulator API and at the DAPM
> > level too.  If we need this sort of stuff it's probably not device
> > specific so we should probably improve the core if it's not easy enough
> > to figure out what's going on already.

> Well, I want this in our driver and you told me to use the regulator-widget, so
> now the only way for us to have this information, which I find very useful when
> we debug, is to let the core have some means to provide the information to our
> driver. I don't want to be forced to enable debug prints in a lot of other
> places than our driver. That just makes it harder.

I'm sorry, but this is crazy.  Anyone might want to inspect the state of
any DAPM widget - it's really not sensible for us to go through and open
code this in every single driver and widget which might want them.  Just
adding random ad-hoc logging all over the place isn't helpful.  As I've
said quite a few times now you really need to work with the frameworks
rather than against or around them, pretty much all of the issues I'm
finding come into this category.  Nothing about these widgets seems like
it is different to any other, and your driver isn't terribly unusual in
this area.

There's *lots* of debugging infrastructure in there as standard, there's
debugfs, there's tracepoints (which are specificially designed for easy
post processing) and yes there's debug prints too.  Given how basic the
stuff you're adding here is it doesn't really seem credible that it's
not possible to do it with the existing infrastructure, and you've
certainly not discussed any problems.

> >> +	{"Mic 1a or 1b Select", "Mic 1a", "MIC1A V-AMICx Enable"},
> >> +	{"Mic 1a or 1b Select", "Mic 1b", "MIC1B V-AMICx Enable"},

> > This also looks very odd...  is this the micbias stuff again?

> I'll rename them to "MIC1A Enable" and "MIC1B Enable". They are connected
> connected to the correct regulator-supply from the machine-driver.

So these are the micbiases...  all my previous comments still apply.

> > Same as last time this should be configured by the machine driver.

> I also told you last time that I have a hard time doing this from the
> machine-driver. The switch between these clocks comes from a component in
> user-space getting its information from a DSP running its own life. If we just
> run the ASoC-driver normally without Android this will never change, but our
> whole system-design form Android needs to be able to do this clock-switch in the
> way we do it. I have no means of getting this information inside the linux-kernel.

As I said last time if you want to do this manually from userspace do it
from the machine driver.  The CODEC driver should *not* be doing this
stuff, especially not in a way which is not joined up with the rest of
the kernel.  As I said last time:

| Normally the clocking control is under the control of the machine driver
| and if the machine driver wants to offer any options to userspace it'd
| provide its own control - usually there's way more stuff going on here
| than just selecting a source and much more coordination needed with the
| drivers involved.

Please don't just ignore review and continue to submit the same stuff
unless there's been clear discussion that what's happening is actually
OK.

> >> +	SOC_DOUBLE_R_TLV("Mic Master Gain",
> >> +		AB8500_ADDIGGAIN3, AB8500_ADDIGGAIN4,
> >> +		0, AB8500_ADDIGGAINX_ADXGAIN_MAX, 1, adx_dig_gain_tlv),

> > All volume controls should be "...Volume".

> So what you are saying is that "Gain" is not an accepted term for a volume?!

The control names have meaning to userspace and are parsed by it to ffer
UI to users.  The keyword for a volume control is Volume.  See
ControlNames.txt in the kernel.

> >> +	SOC_ENUM("Digital Interface 0 Bit-clock Switch", soc_enum_fsbitclk0),
> >> +	SOC_ENUM("Digital Interface 1 Bit-clock Switch", soc_enum_fsbitclk1),

> > Hrm?

> In our current Android-design this is also needed to be controlled for the same
> reasons as the switching of clocks.

And *exactly* the same concerns apply - if you need to do this do 

> > This is fairly impenetrable and would usually be done in hte machine
> > driver.  Machines might not use the chip biases for some or all of the
> > mics but it looks like this code assumes they do.

> OK, so it will be fine if I just move the code to the machine-driver?

Should be, yes.

> >> +int ab8500_audio_setup_if1(struct snd_soc_codec *codec,
> >> +			unsigned int fmt,
> >> +			unsigned int wl,
> >> +			unsigned int delay)

> > Why is this not static?

> Because it is called from the machine-driver.

Why?  No other driver does this...
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
Url : http://mailman.alsa-project.org/pipermail/alsa-devel/attachments/20120509/346f6c10/attachment-0001.sig