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

[Sen Wang <sen@ti.com>]

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