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

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2697 bytes --]

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.

> +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!).

> +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.

> +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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]