Re: [PATCH 3/3] ASoC: atmel: mchp-pdmc: Retain Non-Runtime Controls

[claudiu beznea]

Hi, Andrei,

On 11.09.2024 15:29, Andrei Simion wrote:
> From: Codrin Ciubotariu <codrin.ciubotariu@xxxxxxxxxxxxx>
> 
> Avoid removing these controls, as doing so can cause issues if the stream
> is initiated from another control. Ensure these controls remain intact when
> the stream is started or finished. Instead of removing them, return an
> -EBUSY error code to indicate that the controller is busy, especially when
> the audio filter and the SINC filter are in use.
> 
> [andrei.simion@xxxxxxxxxxxxx: Reword the commit title and the commit
> message. Adapt the code from kernel v6.1 -> v6.6 -> latest kernel version.]
> 
> Signed-off-by: Codrin Ciubotariu <codrin.ciubotariu@xxxxxxxxxxxxx>
> Signed-off-by: Andrei Simion <andrei.simion@xxxxxxxxxxxxx>
> ---
>  sound/soc/atmel/mchp-pdmc.c | 78 ++++++++++++++++---------------------
>  1 file changed, 34 insertions(+), 44 deletions(-)
> 
> diff --git a/sound/soc/atmel/mchp-pdmc.c b/sound/soc/atmel/mchp-pdmc.c
> index d97d153ee375..758b3c550b97 100644
> --- a/sound/soc/atmel/mchp-pdmc.c
> +++ b/sound/soc/atmel/mchp-pdmc.c
> @@ -14,6 +14,7 @@
>  #include <linux/of.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/regmap.h>
> +#include <linux/spinlock.h>
>  
>  #include <sound/core.h>
>  #include <sound/dmaengine_pcm.h>
> @@ -113,6 +114,7 @@ struct mchp_pdmc_chmap {
>  
>  struct mchp_pdmc {
>  	struct mic_map channel_mic_map[MCHP_PDMC_MAX_CHANNELS];
> +	spinlock_t busy_lock;		/* lock protecting busy */
>  	struct device *dev;
>  	struct snd_dmaengine_dai_dma_data addr;
>  	struct regmap *regmap;
> @@ -124,6 +126,7 @@ struct mchp_pdmc {
>  	int mic_no;
>  	int sinc_order;
>  	bool audio_filter_en;
> +	u8 busy:1;

Can the spinlock and busy flag be replaced by an atomic variable?

>  };
>  
>  static const char *const mchp_pdmc_sinc_filter_order_text[] = {
> @@ -167,10 +170,19 @@ static int mchp_pdmc_sinc_order_put(struct snd_kcontrol *kcontrol,
>  		return -EINVAL;
>  
>  	val = snd_soc_enum_item_to_val(e, item[0]) << e->shift_l;
> -	if (val == dd->sinc_order)
> +
> +	spin_lock(&dd->busy_lock);
> +	if (dd->busy) {
> +		spin_unlock((&dd->busy_lock));

You can remove () around (&dd->busy_lock). Valid for the rest of occurrences.

> +		return -EBUSY;
> +	}
> +	if (val == dd->sinc_order) {
> +		spin_unlock((&dd->busy_lock));
>  		return 0;
> +	}
>  
>  	dd->sinc_order = val;
> +	spin_unlock((&dd->busy_lock));
>  
>  	return 1;
>  }
> @@ -193,10 +205,18 @@ static int mchp_pdmc_af_put(struct snd_kcontrol *kcontrol,
>  	struct mchp_pdmc *dd = snd_soc_component_get_drvdata(component);
>  	bool af = uvalue->value.integer.value[0] ? true : false;
>  
> -	if (dd->audio_filter_en == af)
> +	spin_lock(&dd->busy_lock);
> +	if (dd->busy) {
> +		spin_unlock((&dd->busy_lock));
> +		return -EBUSY;
> +	}
> +	if (dd->audio_filter_en == af) {
> +		spin_unlock((&dd->busy_lock));
>  		return 0;
> +	}
>  
>  	dd->audio_filter_en = af;
> +	spin_unlock((&dd->busy_lock));
>  
>  	return 1;
>  }
> @@ -379,52 +399,10 @@ static const struct snd_kcontrol_new mchp_pdmc_snd_controls[] = {
>  	},
>  };
>  
> -static int mchp_pdmc_close(struct snd_soc_component *component,
> -			   struct snd_pcm_substream *substream)
> -{
> -	return snd_soc_add_component_controls(component, mchp_pdmc_snd_controls,
> -					      ARRAY_SIZE(mchp_pdmc_snd_controls));
> -}
> -
> -static int mchp_pdmc_open(struct snd_soc_component *component,
> -			  struct snd_pcm_substream *substream)
> -{
> -	int i;
> -
> -	/* remove controls that can't be changed at runtime */
> -	for (i = 0; i < ARRAY_SIZE(mchp_pdmc_snd_controls); i++) {
> -		const struct snd_kcontrol_new *control = &mchp_pdmc_snd_controls[i];
> -		struct snd_ctl_elem_id id;
> -		int err;
> -
> -		if (component->name_prefix)
> -			snprintf(id.name, sizeof(id.name), "%s %s", component->name_prefix,
> -				 control->name);
> -		else
> -			strscpy(id.name, control->name, sizeof(id.name));
> -
> -		id.numid = 0;
> -		id.iface = control->iface;
> -		id.device = control->device;
> -		id.subdevice = control->subdevice;
> -		id.index = control->index;
> -		err = snd_ctl_remove_id(component->card->snd_card, &id);
> -		if (err < 0)
> -			dev_err(component->dev, "%d: Failed to remove %s\n", err,
> -				control->name);
> -	}
> -
> -	return 0;
> -}
> -
>  static const struct snd_soc_component_driver mchp_pdmc_dai_component = {
>  	.name = "mchp-pdmc",
>  	.controls = mchp_pdmc_snd_controls,
>  	.num_controls = ARRAY_SIZE(mchp_pdmc_snd_controls),
> -	.open = &mchp_pdmc_open,
> -	.close = &mchp_pdmc_close,
> -	.legacy_dai_naming = 1,
> -	.trigger_start = SND_SOC_TRIGGER_ORDER_LDC,
>  };
>  
>  static const unsigned int mchp_pdmc_1mic[] = {1};
> @@ -587,6 +565,13 @@ static int mchp_pdmc_hw_params(struct snd_pcm_substream *substream,
>  			cfgr_val |= MCHP_PDMC_CFGR_BSSEL(i);
>  	}
>  
> +	/*
> +	 * from these point forward, we consider the controller busy, so the
> +	 * audio filter and SINC order can't be changed
> +	 */
> +	spin_lock(&dd->busy_lock);
> +	dd->busy = 1;
> +	spin_unlock((&dd->busy_lock));
>  	for (osr_start = dd->audio_filter_en ? 64 : 8;
>  	     osr_start <= 256 && best_diff_rate; osr_start *= 2) {
>  		long round_rate;
> @@ -1099,6 +1084,7 @@ static int mchp_pdmc_probe(struct platform_device *pdev)
>  	 */
>  	dd->audio_filter_en = true;
>  	dd->sinc_order = 3;
> +	spin_lock_init(&dd->busy_lock);
>  
>  	dd->addr.addr = (dma_addr_t)res->start + MCHP_PDMC_RHR;
>  	platform_set_drvdata(pdev, dd);
> @@ -1143,6 +1129,10 @@ static void mchp_pdmc_remove(struct platform_device *pdev)
>  {
>  	struct mchp_pdmc *dd = platform_get_drvdata(pdev);
>  
> +	spin_lock(&dd->busy_lock);
> +	dd->busy = 0;
> +	spin_unlock((&dd->busy_lock));
> +
>  	if (!pm_runtime_status_suspended(dd->dev))
>  		mchp_pdmc_runtime_suspend(dd->dev);
>