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

[Wang, Sen]

On 4/8/2026 10:41 AM, Mark Brown wrote:
> On Wed, Apr 08, 2026 at 12:31:46AM -0500, Sen Wang wrote:
>
> > +static int tas675x_dsp_mem_write(struct tas675x_priv *tas, u8 page, u8 reg, u32 val)
> > +{
>
> > +out:
> > +	__tas675x_select_book(tas, TAS675X_BOOK_DEFAULT);
> > +	mutex_unlock(&tas->io_lock);
>
> Do we really need to restore the book here?  The book select register is
> marked as volatile so regmap will figure things out if it's the next
> thing to write, and anything else will need to set whatever it wants
> anyway.  Alternatively if the book register were cached (which wouldn't
> be a bad idea) then we'd need to restore whatever the cache has or
> invalidate the cache.
Hi Mark, Thanks for the review!

The book restore was needed because only the page register is listed as 
the selector_reg, and the hardware also requires page 0 before switching 
books, which kind of prevents cache and including book management into 
the ranges config alongside page selection.

All other regmap accesses in the driver implicitly assume DEFAULT book, 
and therefore I can get away with simple and explicit switching of the 
books with help of mutex in a few places.

Would you suggest a regmap/cache mechanism better suited for this 
two-level hierarchy? Happy to rework it if there's a cleaner approach.

> > +static int tas675x_dsp_mem_read(struct tas675x_priv *tas, u8 page, u8 reg, u32 *val)
> > +{
>
> > +out:
> > +	__tas675x_select_book(tas, TAS675X_BOOK_DEFAULT);
> > +	mutex_unlock(&tas->io_lock);
>
> Same here
>
> > +static int tas675x_rtldg_thresh_info(struct snd_kcontrol *kcontrol,
> > +				     struct snd_ctl_elem_info *uinfo)
> > +{
> > +	uinfo->type = SNDRV_CTL_ELEM_TYPE_INTEGER;
> > +	uinfo->count = 1;
> > +	uinfo->value.integer.min = 0;
> > +	/* Accepts 32-bit values, even though 8bit MSB is ignored */
> > +	uinfo->value.integer.max = 0xFFFFFFFF;
>
> This is going to break on 32 bit architectures since long is a 32 bit
> signed value.  You want LONG_MAX, or to restrict the value (which would
> be more friendly to mixer-test!).

Right, I should restrict the max value to be just 24bit 0x00FFFFFF to 
make it friendly to mixer-test. And I can also lift the explicit 
zero-padding in the dsp_mem_write funcs which is not necessary anymore.

> > +static int tas675x_set_dcldg_trigger(struct snd_kcontrol *kcontrol,
> > +				     struct snd_ctl_elem_value *ucontrol)
> > +{
>
> > +	/* Wait for LOAD_DIAG to exit */
> > +	regmap_read_poll_timeout(tas->regmap, TAS675X_STATE_REPORT_CH1_CH2_REG,
> > +				 state, (state & 0x0F) != TAS675X_STATE_LOAD_DIAG &&
> > +					(state >> 4) != TAS675X_STATE_LOAD_DIAG,
> > +				 TAS675X_POLL_INTERVAL_US,
> > +				 TAS675X_STATE_TRANSITION_TIMEOUT_US);
> > +	regmap_read_poll_timeout(tas->regmap, TAS675X_STATE_REPORT_CH3_CH4_REG,
> > +				 state34, (state34 & 0x0F) != TAS675X_STATE_LOAD_DIAG &&
> > +					  (state34 >> 4) != TAS675X_STATE_LOAD_DIAG,
> > +				 TAS675X_POLL_INTERVAL_US,
> > +				 TAS675X_STATE_TRANSITION_TIMEOUT_US);
>
> Don't know how likely it is in practice but we ignore any timeout
> failures in this function.

Will add in the complete error checking for this function.

> > +static irqreturn_t tas675x_irq_handler(int irq, void *data)
> > +{
> > +	struct tas675x_priv *tas = data;
> > +
> > +	if (!tas675x_check_faults(tas))
> > +		return IRQ_NONE;
> > +
> > +	regmap_write(tas->regmap, TAS675X_RESET_REG, TAS675X_FAULT_CLEAR);
> > +	return IRQ_HANDLED;
> > +}
>
> This probably ought to take a runtime PM reference to ensure the I2C
> controller is woken up, and in case in future you get regulator support.
> Even if the device itself shouldn't be generating interrupts while it's
> idle the interrupt might be shared or something might fire for some
> other reason.

Absolutely, let me add the runtime PM reference (mark_busy & 
put_autosuspend) on this one as well. Better be safe than sorry.

Best Regards,
Sen Wang