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

[Mark Brown]

On Mon, Mar 21, 2016 at 12:08:27PM -0500, Andreas Dannenberg wrote:

> +static int tas5720_set_dai_sysclk(struct snd_soc_dai *dai, int clk_id,
> +				  unsigned int freq, int dir)
> +{
> +	/*
> +	 * Nothing to configure here for TAS5720. It's a simple codec slave.
> +	 * However we need to keep this function in here otherwise the ASoC
> +	 * platform driver will throw an ENOTSUPP at us when trying to play
> +	 * audio.
> +	 */
> +
> +	return 0;
> +}

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

> +	if (unlikely(!tx_mask)) {

unlikely() is for optimising hot paths, just write the logic clearly
unless there's a reason for it.

> +static irqreturn_t tas5720_irq_handler(int irq, void *_dev)
> +{
> +	/*
> +	 * Immediately disable TAS5720 FAULTZ interrupts inside the low-level
> +	 * handler to prevent the system getting saturated or even overrun
> +	 * by interrupt requests. Normally the fact that we create a threaded
> +	 * interrupt with IRQF_ONESHOT should take care of this as by design
> +	 * it masks interrupts while the thread is processed however testing
> +	 * has shown that at the high frequency the FAULTZ signal triggers
> +	 * (every 300us!) occasionally the system would lock up even with a
> +	 * threaded handler that's completely empty until the Kernel breaks the
> +	 * cycle, disables that interrupt, and reports a "nobody cared" error.
> +	 *
> +	 * Disabling the interrupt here combined with a deferred re-enabling
> +	 * after the thread has run not only prevents this lock condition but
> +	 * also helps to rate-limit the processing of FAULTZ interrupts.
> +	 */
> +	disable_irq_nosync(irq);

No, this is completely broken.  Whatever is going on in your system with
the interrupt core you need to address that at the appropriate level not
by putting a nonsensical bodge in here.  The interrupt is disabled while
the threaded handler is running, if that's not having the desired effect
then whatever causes that needs to be fixed.

> +static int tas5720_dapm_post_event(struct snd_soc_dapm_widget *w,
> +				   struct snd_kcontrol *kcontrol, int event)
> +{
> +	struct snd_soc_codec *codec = snd_soc_dapm_to_codec(w->dapm);
> +	int ret;
> +
> +	switch (event) {
> +	case SND_SOC_DAPM_POST_PMU:
> +		/*
> +		 * Check if the codec is still powered up in which case exit
> +		 * right away also skipping the shutdown-to-active wait time.
> +		 */
> +		ret = snd_soc_test_bits(codec, TAS5720_POWER_CTRL_REG,
> +					TAS5720_SDZ, 0);

I don't understand this.  Why on earth would we be calling the PMU
handler if the widget was not previously powered? 

> +		/*
> +		 * Take TAS5720 out of shutdown mode in preparation for widget
> +		 * power up.
> +		 */
> +		ret = snd_soc_update_bits(codec, TAS5720_POWER_CTRL_REG,
> +					  TAS5720_SDZ, TAS5720_SDZ);
> +		if (ret < 0) {
> +			dev_err(codec->dev, "error waking codec: %d\n", ret);
> +			return ret;
> +		}

This is a _POST_PMU handler not a pre-PMU handler...

> +	/* Events used to control the TAS5720 SHUTDOWN state */
> +	SND_SOC_DAPM_PRE("Pre Event", tas5720_dapm_pre_event),
> +	SND_SOC_DAPM_POST("Post Event", tas5720_dapm_post_event),

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.

Attachment: signature.asc
Description: PGP signature