Re: [PATCH v2 2/2] ASoC: codecs: add support for TAS5720 digital amplifier

From: Mark Brown <broonie@kernel.org>
To: Andreas Dannenberg <dannenberg@ti.com>
Cc: alsa-devel@alsa-project.org, devicetree@vger.kernel.org,
	Liam Girdwood <lgirdwood@gmail.com>,
	Jaroslav Kysela <perex@perex.cz>, Takashi Iwai <tiwai@suse.com>,
	Rob Herring <robh+dt@kernel.org>, Pawel Moll <pawel.moll@arm.com>,
	Mark Rutland <mark.rutland@arm.com>,
	Ian Campbell <ijc+devicetree@hellion.org.uk>,
	Kumar Gala <galak@codeaurora.org>,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 2/2] ASoC: codecs: add support for TAS5720 digital amplifier
Date: Sat, 2 Apr 2016 09:21:33 -0700	[thread overview]
Message-ID: <20160402162133.GA2350@sirena.org.uk> (raw)
In-Reply-To: <20160401211402.GA26370@borg.dal.design.ti.com>

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

On Fri, Apr 01, 2016 at 04:14:02PM -0500, Andreas Dannenberg wrote:
> On Wed, Mar 30, 2016 at 08:38:53AM -0700, Mark Brown wrote:

> > It sounds like this feature is unusably broken...  possibly you could do
> > something in the mute handler but it seems that anything you try to do
> > to use this feature is going to be both fragile and disruptive to the
> > system.

> Agreed, this is not the first time this has come up :( Btw in my quest
> for a solution one of my earlier implementations actually hooked into
> the MUTE handler, but while this worked keeping the TA5720 in shutdown
> most of the time it did not completely solve the interrupt-overrun issue
> (the TAS5720 would still generate SAIF errors for brief periods,
> dead-locking my SoC even with an empty threaded handler). I was also
> concerned that hooking such parasitic code into a MUTE handler would be
> a bit of an abuse and not make me may friends here.

I think this feature is so broken that any attempt to use it is going to
cause problems.  Even if you somehow manage to make something that holds
together in your test system I'm not convinced it's going to be safe for
other users.

> > What is the value in implementing it?

> There is a strong request from one rather large customer to have
> interrupt-driven fault handling. I did have an early implementation of
> the driver that polled for errors (except SAIF) at the beginning and the
> end of the audio playback but this was not good enough.

I really think this is something that the user needs to carry out of
tree, it seems clear that that enbling interrupts is very disruptive.

> But thinking about this some more, what if I do not actually use the
> interrupt signal, but rather during playback use a timer that fires
> every let's say 1s to check the TAS5720 fault register? This way one

That's fine, some other drivers do this for things that don't have
interrupts or don't have usable interrupts.

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