Re: [RFC PATCH 2/3] ASoC: tegra: Update ASRC ratio controls

[Thierry Reding]

On Mon, Sep 21, 2026 at 08:57:03AM +0000, Sheetal wrote:
> Replace the separate integer and fractional ASRC ratio controls with a
> single two-value control for each stream. This is a userspace-visible
> control ABI change, but:
> 
> 1. Keeping writable compatibility controls would preserve the non-atomic
>    update path that can expose a transient mixed ratio to hardware.
> 2. This is limited to only the Tegra ASRC driver.
> 
> Validate both values before programming hardware. The fractional field
> keeps the existing 32-bit maximum, while the integer field is rejected
> when it exceeds the hardware field width.
> 
> ALSA exposes one min and max range for all values in an integer-array
> control, so the paired ratio controls advertise the fractional field's
> full 32-bit range. The put callback validates the integer part separately
> against the hardware field width.
> 
> Serialize ratio-source changes, stream setup, and ratio updates with a
> driver mutex so cached ratio state and paired hardware programming remain
> consistent.
> 
> Unlock and re-lock the ASRC stream before programming a software ratio
> when the new ratio differs from the cached ratio by more than 20%.
> Large ratio jumps require the hardware lock to be refreshed around the
> paired integer/fractional update.
> 
> Roll back the previous hardware pair and lock state if a paired ratio
> update fails after partially programming the hardware.
> 
> Signed-off-by: Sheetal <sheetal@xxxxxxxxxx>
> ---
>  sound/soc/tegra/tegra186_asrc.c | 443 ++++++++++++++++++++------------
>  sound/soc/tegra/tegra186_asrc.h |   4 +
>  2 files changed, 276 insertions(+), 171 deletions(-)
> 
> diff --git a/sound/soc/tegra/tegra186_asrc.c b/sound/soc/tegra/tegra186_asrc.c
> index d8ae5d997615..2b67d23f5fa7 100644
> --- a/sound/soc/tegra/tegra186_asrc.c
> +++ b/sound/soc/tegra/tegra186_asrc.c
> @@ -19,6 +19,9 @@
>  #include "tegra186_asrc.h"
>  #include "tegra_cif.h"
>  
> +#define TEGRA186_ASRC_RATIO_PERCENT_SCALE		100
> +#define TEGRA186_ASRC_RATIO_UNLOCK_THRESHOLD_PERCENT	20
> +
>  #define ASRC_STREAM_SOURCE_SELECT(id)					       \
>  	(TEGRA186_ASRC_CFG + ((id) * TEGRA186_ASRC_STREAM_STRIDE))
>  
> @@ -65,13 +68,125 @@ static const struct reg_default tegra186_asrc_reg_defaults[] = {
>  	{ TEGRA186_ASRC_CYA, 0x0},
>  };
>  
> -static void tegra186_asrc_lock_stream(struct tegra186_asrc *asrc,
> -				      unsigned int id)
> +static int tegra186_asrc_set_stream_lock(struct tegra186_asrc *asrc,
> +					 unsigned int id, bool lock)
>  {
> -	regmap_write(asrc->regmap,
> -		     ASRC_STREAM_REG(TEGRA186_ASRC_RATIO_LOCK_STATUS,
> -				     id),
> -		     1);
> +	return regmap_write(asrc->regmap,
> +			    ASRC_STREAM_REG(TEGRA186_ASRC_RATIO_LOCK_STATUS,
> +					    id),
> +			    lock);
> +}
> +
> +static int tegra186_asrc_write_ratio_pair(struct tegra186_asrc *asrc,
> +					  unsigned int id,
> +					  unsigned int int_part,
> +					  unsigned int frac_part)
> +{
> +	int ret;
> +
> +	ret = regmap_write(asrc->regmap,
> +			   ASRC_STREAM_REG(TEGRA186_ASRC_RATIO_INT_PART, id),
> +			   int_part);
> +	if (ret)
> +		return ret;
> +
> +	return regmap_write(asrc->regmap,
> +			    ASRC_STREAM_REG(TEGRA186_ASRC_RATIO_FRAC_PART, id),
> +			    frac_part);
> +}
> +
> +static int tegra186_asrc_read_ratio_pair(struct tegra186_asrc *asrc,
> +					 unsigned int id, unsigned int *int_part,
> +					 unsigned int *frac_part)
> +{
> +	int ret;
> +
> +	ret = regmap_read(asrc->regmap,
> +			  ASRC_STREAM_REG(TEGRA186_ASRC_RATIO_INT_PART, id),
> +			  int_part);
> +	if (ret)
> +		return ret;
> +
> +	return regmap_read(asrc->regmap,
> +			   ASRC_STREAM_REG(TEGRA186_ASRC_RATIO_FRAC_PART, id),
> +			   frac_part);
> +}
> +
> +static void tegra186_asrc_cache_ratio(struct tegra186_asrc *asrc,
> +				      unsigned int id, unsigned int int_part,
> +				      unsigned int frac_part)
> +{
> +	asrc->lane[id].int_part = int_part;
> +	asrc->lane[id].frac_part = frac_part;
> +}
> +
> +static int tegra186_asrc_apply_ratio(struct tegra186_asrc *asrc,
> +				     unsigned int id, unsigned int int_part,
> +				     unsigned int frac_part, bool unlock)
> +{
> +	unsigned int old_int_part, old_frac_part, old_lock;
> +	int ret, restore_ret;
> +
> +	ret = tegra186_asrc_read_ratio_pair(asrc, id, &old_int_part,
> +					    &old_frac_part);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_read(asrc->regmap,
> +			  ASRC_STREAM_REG(TEGRA186_ASRC_RATIO_LOCK_STATUS, id),
> +			  &old_lock);
> +	if (ret)
> +		return ret;
> +
> +	if (unlock) {
> +		ret = tegra186_asrc_set_stream_lock(asrc, id, false);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	ret = tegra186_asrc_write_ratio_pair(asrc, id, int_part, frac_part);
> +	if (ret)
> +		goto restore_ratio;
> +
> +	ret = tegra186_asrc_set_stream_lock(asrc, id, true);
> +	if (!ret)
> +		return 0;
> +
> +restore_ratio:
> +	restore_ret = tegra186_asrc_write_ratio_pair(asrc, id,
> +						     old_int_part,
> +						     old_frac_part);
> +	if (!restore_ret) {
> +		if (old_lock)
> +			restore_ret = tegra186_asrc_set_stream_lock(asrc, id,
> +								    true);
> +		else
> +			restore_ret = tegra186_asrc_set_stream_lock(asrc, id,
> +								    false);
> +	}
> +
> +	return ret ?: restore_ret;
> +}
> +
> +static int tegra186_asrc_apply_cached_ratio(struct tegra186_asrc *asrc,
> +					    unsigned int id)
> +{
> +	return tegra186_asrc_apply_ratio(asrc, id, asrc->lane[id].int_part,
> +					asrc->lane[id].frac_part, false);
> +}
> +
> +static bool tegra186_asrc_need_unlock(u64 old_ratio, u64 new_ratio)
> +{
> +	u64 ratio_diff;
> +
> +	if (!old_ratio || !new_ratio)
> +		return false;
> +
> +	ratio_diff = (old_ratio > new_ratio) ?
> +		     (old_ratio - new_ratio) : (new_ratio - old_ratio);
> +
> +	return ((ratio_diff * TEGRA186_ASRC_RATIO_PERCENT_SCALE) >
> +		(old_ratio * TEGRA186_ASRC_RATIO_UNLOCK_THRESHOLD_PERCENT));
>  }

There's a fair bit of complication introduced by the concept of locking
here, so I wonder what the impact of this is. The function name implies
that sometimes unlocking is needed and sometimes it isn't.

What if we unlock unconditionally? That doesn't get rid of all the
locking and unlocking, but at least it would avoid the need to track the
current locking status.

The unlock operation doesn't seem overly expensive, so maybe it isn't
worth trying to avoid it?

[...]
> diff --git a/sound/soc/tegra/tegra186_asrc.h b/sound/soc/tegra/tegra186_asrc.h
> index 0c98e26d5e72..9cc8cec6c204 100644
> --- a/sound/soc/tegra/tegra186_asrc.h
> +++ b/sound/soc/tegra/tegra186_asrc.h
> @@ -7,6 +7,8 @@
>  #ifndef __TEGRA186_ASRC_H__
>  #define __TEGRA186_ASRC_H__
>  
> +#include <linux/mutex.h>
> +
>  /* ASRC stream related offset */
>  #define TEGRA186_ASRC_CFG				0x0
>  #define TEGRA186_ASRC_RATIO_INT_PART			0x4
> @@ -111,6 +113,8 @@ struct tegra186_asrc {
>  	const struct tegra_asrc_soc_data *soc_data;
>  	struct tegra186_asrc_lane lane[TEGRA186_ASRC_STREAM_MAX];
>  	struct regmap *regmap;
> +	/* Serializes ratio source, cache, and paired register updates. */
> +	struct mutex ratio_lock;

All of the operations that are serialized by this lock seem to be non-
sleeping register or cache updates. Could the lock be turned into a
spinlock instead?

Thierry

Attachment: signature.asc
Description: PGP signature