Re: [PATCH v2 3/5] ASoC: mt6797: add mt6797 platform driver

[Mark Brown <broonie@kernel.org>]

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

On Thu, Apr 19, 2018 at 09:51:13AM +0800, KaiChieh Chuang wrote:
> On Wed, 2018-04-18 at 17:46 +0100, Mark Brown wrote:

> > > +		/* turn of mute function */
> > > +		dl_src2_con0 |= (0x03 << 11);

> > Is there any reason for not implementing this as a mute_stream()
> > callback?  That'll keep the DAI muted during startup and before teardown
> > which will do the most to avoid any noise making it into the output.

> We used to avoid the pop sound from user space by closing hw path from outside to inside.
> The control here is just to avoid the state is leaved in muted state.
> I'll take a look at the mute_stream() though.

Right, that's the goal of mute_stream() too - it's the last thing we do
during startup and first thing we do on tear down so that there's less
chance of any noise making its way out of the DAI.  If it doesn't work
for your system it's fine to do this though.

> > This unconditionally returns IRQ_HANDLED even if we didn't get an
> > interrupt - it's better to return IRQ_NONE unless we actually handled
> > something since that is more robust if something goes wrong, it lets the
> > IRQ core shut up interrupts that get latched on and works with shared
> > interrupts if any future designs do that.

> this IRQ num is not shared with other module, it's only for audio.

In current designs...

> We error handle by clearing the all the irq status again, when
> interrupts is sent but status not matched. we consider this handled?

You should at least complain somewhere if you get an interrupt that the
driver doesn't know how to handle.

> I'm concerned if returing IRQ_NONE, will i no longer be able
> to receive irq from this irq line?

Well, that's kind of the goal in a way - if what's going wrong is
something that's just going to keep flagging the interrupt up then
the system will just get stuck handling the interrupt constantly which
is especially nasty if the interrupt is just silently acknowledged.  If
you return IRQ_NONE eventually the core will decide that the interrupt
is stuck and disable it which contains whatever the problem is.  I'd at
least put some logging in there so if something goes wrong there's an
obvious indication that something's gone wrong with the interrupt.

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

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