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

[Sen Wang <sen@ti.com>]

On 4/1/26 11:07, Mark Brown wrote:
> On Tue, Mar 31, 2026 at 09:42:07PM -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.
> 
>> +static int tas675x_dapm_sleep_event(struct snd_soc_dapm_widget *widget,
>> +				    struct snd_kcontrol *kcontrol, int event)
>> +{
>> +	struct snd_soc_component *component = snd_soc_dapm_to_component(widget->dapm);
>> +	int ret = 0;
>> +
>> +	switch (event) {
>> +	case SND_SOC_DAPM_POST_PMU:
>> +		ret = pm_runtime_resume_and_get(component->dev);
>> +		break;
>> +	case SND_SOC_DAPM_PRE_PMD:
>> +		pm_runtime_mark_last_busy(component->dev);
>> +		pm_runtime_put_autosuspend(component->dev);
>> +		break;
>> +	}
>> +	return ret < 0 ? ret : 0;
>> +}
> 
> I'm confused what's going on here, there's runtime PM management in the
> ASoC core - isn't this just duplicating what's there?
> 

Hi Mark, thanks for the review.

Yes this function is redundant, it was a residue that I neglected to 
clean up, let me remove this function.

I was experimenting with DAPM events and see if this would a better way 
to systematically clear fault status and set the prerequisite registers 
for LLP (DAI1). But ultimately reverted for the sake of simplicity.

>> +static int tas675x_runtime_resume(struct device *dev)
>> +{
> 
>> +       if (!to_i2c_client(dev)->irq)
>> +               schedule_delayed_work(&tas->fault_check_work,
>> +                                     msecs_to_jiffies(TAS675X_FAULT_CHECK_INTERVAL_MS));
> 
> This is the only place where we start the fault checking but runtime PM
> can be disabled in configuration and we also start runtime PM in the
> active state so potentially might never suspend and resume.  probe()
> should kick off the work as well.
> 

Yes, I need to add work at probe time as well. Didn't consider the 
scenario when PM is disabled in configuration.

I originally wanted detection to only kick off after the first playback.
But scheduling work after the power_on which clears faults status
essentially does the same thing.

>> +static int tas675x_init_device(struct tas675x_priv *tas)
>> +{
>> +	struct regmap *regmap = tas->regmap;
>> +	unsigned int val;
>> +	int ret, i;
>> +
>> +	/* Clear POR fault flag to prevent IRQ storm */
>> +	regmap_read(regmap, TAS675X_POWER_FAULT_LATCHED_REG, &val);
> 
> 
>> +static int tas675x_i2c_probe(struct i2c_client *client)
>> +{
> 
>> +	if (client->irq) {
>> +		ret = devm_request_threaded_irq(tas->dev, client->irq, NULL,
>> +						tas675x_irq_handler,
>> +						IRQF_ONESHOT | IRQF_TRIGGER_FALLING,
>> +						"tas675x-fault", tas);
>> +		if (ret)
>> +			return dev_err_probe(tas->dev, ret, "Failed to request IRQ\n");
>> +	}
> 
> We request the interrupt here...
> 
>> +	INIT_DELAYED_WORK(&tas->fault_check_work, tas675x_fault_check_work);
> 
>> +	ret = tas675x_power_on(tas);
>> +	if (ret)
>> +		return ret;
> 
> ...before we do _power_on() which is what calls _hw_init() and clears any
> faults that were latched in the registers.  This means that if there is
> something there we'll handle it through the normal interrupt handling
> flow and the rest won't buy is much, probably better to reorder the init
> vs the interrupt request.

Absolutely, will reorder the IRQ request to after power_on(), so the 
handler only starts once the fault reg gets cleared and in a known 
state. Along with scheduling work at probe that I've mentioned above.

Thanks a lot for the catch! Will post a V2 addressing these concerns.

Best,
Sen Wang