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
