[alsa-devel] [PATCH] ASoC: nau8825: Add driver for headset chip Nuvoton 8825

[Mark Brown]

On Mon, Aug 10, 2015 at 05:32:32PM -0700, Anatol Pomozov wrote:
> On Tue, Aug 4, 2015 at 2:47 AM, Mark Brown <broonie at kernel.org> wrote:
> > On Mon, Aug 03, 2015 at 08:13:40PM -0700, Anatol Pomozov wrote:

> >> >> +     /* Setup ADC x128 OSR */Moved to
> >> >> +     regmap_write(regmap, NAU8825_REG_ADC_RATE, 0x0002);
> >> >> +     /* Setup DAC x128 OSR */
> >> >> +     regmap_write(regmap, NAU8825_REG_DAC_CTRL1, 0x0082);

> >> > I'd expect this to be user controllable.

> >> The oversampling configuration is important for chip audio quality.
> >> There is audible hissing without these settings.

> >> My understanding that all users need to set these values and it is
> >> better to move it to the driver initialization sequence.

> > That sounds like spectacularly poor quality of implementation, but even
> > if there's a noticeable reduction in audio quality the control should
> > still be given to the user, they may have an application where power is

> Moved it to ALSA controls. Though I still feel a bit uncomfortable
> that the default control value is not usable.
> I have to set "ADC Decimation Rate" and "DAC Oversampling Rate"
> controls in userspace from its defaults to "128".

Setting the default isn't too bad in this case (especially with a
comment explaining why) - that doesn't preclude also providing user
configuration.  

> > If you need to control MCLK you should be using the clock API to get
> > MCLK which you can do in the driver model probe.  Relying on the machine
> > driver to control MCLK for you in this way is at best fragile.

> After discussing it with my teammates who works on Intel+NAU8825
> platform I found that they use MCLK only at playback/capture time. The
> rest of the time VCO is suppose to be sysclk source. Thus using
> get_clk() is not flexible for all cases.

I'm sorry, I don't understand.  What makes you say that "using get_clk()
is not flexible for all cases", I can't quite parse that bit?

> > > +int nau8825_enable_jack_detect(struct snd_soc_codec *codec,
> > > +                             struct snd_soc_jack *jack)
> > > +{
> > > +     struct nau8825 *nau8825 = snd_soc_codec_get_drvdata(codec);


> > > +     snd_jack_set_key(jack->jack, SND_JACK_BTN_0, KEY_MEDIA);
> > > +     snd_jack_set_key(jack->jack, SND_JACK_BTN_1, KEY_VOICECOMMAND);
> > > +     snd_jack_set_key(jack->jack, SND_JACK_BTN_2, KEY_VOLUMEUP);
> > > +     snd_jack_set_key(jack->jack, SND_JACK_BTN_3, KEY_VOLUMEDOWN);

> > The driver shouldn't do this - it's up to the system integration to
> > define how the buttons are mapped.

> I was following code from ts3a227e driver that does the same.

That device is specifically tied to Chromebooks which have a defined
mapping, as opposed to being a generic device which is usable on a range
of devices.

> > > +     nau8825->jack = jack;

> > > +     return 0;
> > > +}
> > > +EXPORT_SYMBOL_GPL(nau8825_enable_jack_detect);

> > This function doesn't appear to affect the hardware.  I would expect
> > that the jack detection hardware is turned off when not in use.

> Disabling jack detection means disable HeadsetCompletion IRQ. But the
> IRQ handler does a bunch of manual stuff like grounding/ungrounding
> jack pins. If IRQ handler is not called then neither playback nor
> capture will not work.

The assumption would be that if the system doesn't have or need jack
detection then the jack pins won't be reconfigurable anyway.

> At this point this function just sets jack structure needed later in
> IRQ handler.

I can see what it's doing, I'd just also expect it to also be starting
and stopping the detection in the hardware.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: <http://mailman.alsa-project.org/pipermail/alsa-devel/attachments/20150811/164c34b3/attachment-0001.sig>