Re: [PATCH 1/2] ASoC: codecs: add support for ES8326

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2075 bytes --]

On Thu, Jul 14, 2022 at 10:26:17AM +0800, Zhu Ning wrote:

> >> +     snd_soc_component_write(comp, ES8326_ANA_MICBIAS_1B, 0x7c);

> >this ES8326_ANA_MICBIAS_1B register is also modified in the workqueue,
> >could this lead to invalid configurations?

> In es8326_irq, MICBIAS is turned on to detect headphone from headset. When an unpluged
> event is detected, MICBIAS is turned off to minimize pop noise. Maybe a comment/macro for this?

The issue is that both the workqueue and interrupt are modifying the
register - what happens if they race with each other?

Generally it's better to manage the MICBIAS as a DAPM widget and then
force it on when needed for jack detection (search other drivers for
examples) and then let DAPM worry about refcounting.

> >> +     dev_dbg(comp->dev, "gpio flag %#04x", iface);
> >> +     if ((iface & ES8326_HPINSERT_FLAG) == 0) {
> >> +             dev_dbg(comp->dev, "No headset detected");
> >> +             snd_soc_jack_report(es8326->jack, 0, SND_JACK_HEADSET);

> >should you check if es8326->jack is set?
> >in the 8316 driver you have a check for a spurious interrupt before
> >set_jack() is called

> I haven't seen other codecs (rt5640) handle spurious irq.

A lot of devices have a chip level reset register which they use to get
the device into a known state during probe, generally the detection
logic is turned off when the device is reset which helps here.

> >it's rather odd that there's a resume but no suspend?

> Since the codec loses power on suspend. It's also odd to write to the registers when they 
> are going to be cleared during suspend. The lost registers need to be written during 
> resume though. 

You shouldn't rely on the device loosing power over suspend, a lot of
systems will do that but it depends on the physical design (perhaps the
power is shared with something needed to detect wakeup).  It's best
practice to get the device into the lowest power possible state before
power is cut, things that don't impact power can generally be left alone
though.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]