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

[Sen Wang]

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