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

Sen Wang
2026-04-03T01:16:35Z

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