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

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

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

On Wed, Mar 14, 2012 at 02:27:13PM +0100, Ola LILJA2 wrote:

Fix your mailer to word wrap properly, I've reflowed your text for
legibility.

> > > diff --git a/sound/soc/codecs/ab8500_audio.c
> > > b/sound/soc/codecs/ab8500_audio.c

> > Everything else uses -codec.

> Could just see one codec named with "-codec". The reason for appending _audio
> Is that the AB8500 is not only audio and we already have files elsewhere named
> ab8500.c but I probably could rename it to ab8500-codec.c.

The drivers for MFDs pretty much all call themselves -codec internally
(as yours does).

> > Why are you doing this virtual register stuff?  It's making the code a
> > lot more complex and doesn't look at all driver specific.

> There is a chain of events leading up to the decision of having it
> like this.  The HW is designed so that only one coefficient-register
> is present and when we write a value to it the HW increases a HW-index
> so that the next time we write to it the value will set the next
> coefficient and so on.  Also, we don't want to be able to first set
> all FIR/IIR-coefficients with the ALSA-controls And when we are happy
> we write all the coefficients to the HW by committing them using the
> strobe-control.  Furthermore, we cannot read the coefficients from the
> HW, so to be able to provide this to userspace we use the
> SOC_HWDEP_MULTIPLE_1R, and since these values actually is not exposed
> as separate registers we use a set of virtual register. When we commit
> the coefficients we then take the values from the virtual registers
> and write them down to the HW in a loop.

Don't do this, this just makes things a lot more complicated in the
register I/O code and making it much more difficult to work with both
locally and from a framework point of view.  Store the coefficients in
your driver data rather than shoehorning them into the register cache.

> > > +static const char * const enum_ena_dis[] = {"Enabled", "Disabled"};
> > > +static const char * const enum_dis_ena[] = {"Disabled", "Enabled"};

> > Why are the controls using these enums and not Switch controls?  UIs
> > know how to display switches.

> We need to make the switches virtual, as we don't want them to
> actually set any bits, just break or complete a DAPM-chain. That is
> why we made the as MUX:es.

This is completely orthogonal to how the controls are displayed to
userspace, it's an implementation detail of your driver.  Though if your
routing control doesn't actually touch the device one has to wonder what
it actually does...

> > > +/* Earpiece - Mute */
> > > +static const struct snd_kcontrol_new dapm_ear_mute[] = {
> > > +	SOC_DAPM_SINGLE("Playback Switch", REG_MUTECONF,
> > > +REG_MUTECONF_MUTEAR, 1, 1), };

> > This looks like it's just a mute rather than a mixer input enable
> > (there's only one control...) so should be a regular sound control;
> > unless there's a very good reason mutes other than mixer inputs
> > normally don't affect the audio path as you might get pops and you
> > probably don't want to do things like stop clocking just because the
> > output is silenced.  Similar thing applies in several other places.

> Most of the mutes need to be apart of the DAPM-chain to actually prevent
> click-n-pops. So it cannot be used by the user as a normal ALSA-control.
> Muting can be done by setting certain gains to -inf.

This explanation doesn't correspond to what you've actually written -
the code above will result in a user visible control.

> > > +			unsigned int fmt,
> > > +			unsigned int wl,
> > > +			unsigned int delay);
> > > +void ab8500_audio_anc_configure(struct snd_soc_codec *codec,
> > > +			bool apply_fir, bool apply_iir);

> > No, you shouldn't be exporting this stuff - it all looks like basic
> > framework stuff.

> Hmm.. ok... the power_control is needed for reasons explained before
> (vibra-driver and Accessory-detection-magic), but I guess I have to
> remove these features for now and I can then remove this export.

You've not mentioned accessory detection before...  there's certainly
no obvious excuse for doing the power management for accessory detection
outside of DAPM, we've got a bunch of drivers in mainline already which
manage to do this quite successfully, but since you've not explained
what the issue you think you see is it's hard to comment.

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

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