Re: [PATCH v2 2/4] ASoC: codecs: Add TAS675x quad-channel audio amplifier driver

[Sen Wang]

On 4/2/26 12:10, Mark Brown wrote:
> On Wed, Apr 01, 2026 at 05:28:43PM -0500, Sen Wang wrote:
> > The TAS675x (TAS6754, TAS67524) are quad-channel, digital-input
> > Class-D amplifiers with an integrated DSP, controlled over I2C.
> > They support I2S and TDM serial audio interfaces.
>
> One issue that didn't get noticed last time, sorry:

The more the merrier :)

> > +static int tas675x_runtime_suspend(struct device *dev)
> > +{
> > +	struct tas675x_priv *tas = dev_get_drvdata(dev);
> > +
> > +	cancel_delayed_work_sync(&tas->fault_check_work);
> > +	tas675x_set_state_all(tas, TAS675X_STATE_SLEEP_BOTH);
> > +
> > +	return 0;
> > +}
>
> This cancels the work, completing any that's already running, but...
>
> > +static void tas675x_fault_check_work(struct work_struct *work)
> > +{
> > +	struct tas675x_priv *tas = container_of(work, struct tas675x_priv,
> > +						fault_check_work.work);
> > +
> > +	if (tas675x_check_faults(tas))
> > +		regmap_write(tas->regmap, TAS675X_RESET_REG, TAS675X_FAULT_CLEAR);
> > +
> > +	schedule_delayed_work(&tas->fault_check_work,
> > +			      msecs_to_jiffies(TAS675X_FAULT_CHECK_INTERVAL_MS));
> > +}
>
> ...the work unconditionally rearms itself so we might race and requeue
> (we cancel *then* wait) with the device powered off.  There's the
> disable_delayed_work_sync() API which should be a better fit.

Right the APIs themselves aren't atomic, disable would be a much less 
error-prone approach. Looks like TAS5720 & TAS6424 has a similar issue 
too, where they have cancel work at DAPM events instead of runtime 
suspend. I can test and send these changes in a separate patch.

> > +static irqreturn_t tas675x_irq_handler(int irq, void *data)
> > +{
> > +	struct tas675x_priv *tas = data;
> > +
> > +	tas675x_check_faults(tas);
> > +
> > +	/* Clear the FAULT pin latch as something latched */
> > +	regmap_write(tas->regmap, TAS675X_RESET_REG, TAS675X_FAULT_CLEAR);
> > +
> > +	return IRQ_HANDLED;
> > +}
>
> Also, this should return IRQ_NONE if no faults were seen (to allow for
> interrupt sharing and the genirq core's handling of hardware faults).

Understood, thank you for the context, didn't know IRQ_NONE provides 
additional heuristics to irq core. Will post a follow-up V3 addressing 
the issues.

Best,
Sen Wang