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

[Mark Brown <broonie@kernel.org>]

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

On Tue, Mar 29, 2016 at 09:53:18PM -0500, Andreas Dannenberg wrote:
> On Mon, Mar 28, 2016 at 08:01:43PM +0100, Mark Brown wrote:
> > On Mon, Mar 21, 2016 at 12:08:27PM -0500, Andreas Dannenberg wrote:

> > Remove empty funnctions, -ENOTSUPP is expected behaviour for anything
> > that isn't explicitly supported by a driver.

> Ok will double-check. Very early during my driver development I was not
> able to play audio through aplay if this function was not provided. I
> don't recall what specific Kernel version that was but it may have been
> something like 4.1.

This would be a bug in your machine driver then.

> As explained in the code comment even with a boiled-down test code that
> has an empty threaded handler the system would come to a grinding halt
> when bombarded with interrupts every 300us which I found odd but not
> completely unexpected (from my MCU background POV that is). And while
> digging I had seen that the interrupts do get disabled just like you
> mention during threaded handling to operate in a more graceful manner.
> But I wasn't sure at this point if the additional (high priority, I
> suppose) overhead of creating/starting the thread (even an empty one)
> every 300us was just too much for my poor single-core SoC to handle so
> my assumption was that it never got cycles to process stuff other than
> interrupts, and disabling interrupts in the low-level handler fixed just
> that. But I'm going to spend some extra cycles trying to re-digest the
> realtime behavior of my particular SoC/setup to understand why exactly
> this is happening.

If your device is constantly retriggering the same interrupt that
suggests there is a problem with how you are handling your device,
perhaps you need to disable the interrupts at source if it's truly
broken beyond repair.

> > Oh, we're using _PRE() and _POST() events...  this almost certainly
> > indicates a problem, there are very few circumstances where these are a
> > good idea and I'm not seeing anything in this driver which indicates
> > that this is going on.  Please just use normal DAPM widgets (I'm
> > guessing a PGA) to represent the device and work within DAPM, don't
> > shoehorn some bodge around the side.

> I'm currently using these handlers to essentially tame the TAS5720 error
> reporting. Only when the device is in shutdown mode it will seize
> bombarding the host with 300us-spaced FAULT interrupts (that will come
> as soon as the SAIF stream stops).  Unfortunately that's the way the
> TAS5720 was designed and I've already provided feedback internally that
> this makes an elegant / low-overhead SW implementation quite
> challenging. Anyways I did see several places where this shutdown mode
> handling could get added so I simply picked the one that was not directly
> associated with the audio stream itself to make it more explicit what
> this is about but this can certainly be changed.

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.  What is the value in implementing it?

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