Re: [PATCH v2 2/5] ASoC: samsung: i2s: add support for FSD I2S

[Krzysztof Kozlowski]

On 03/01/2023 05:56, Padmanabhan Rajanbabu wrote:
> Add support for enabling I2S controller on FSD platform.
> 
> FSD I2S controller is based on Exynos7 I2S controller, supporting
> 2CH playback/capture in I2S mode and 7.1CH playback/capture in TDM
> mode.
> 
> Signed-off-by: Padmanabhan Rajanbabu <p.rajanbabu@xxxxxxxxxxx>
> ---
>  sound/soc/samsung/i2s-regs.h |  1 +
>  sound/soc/samsung/i2s.c      | 57 ++++++++++++++++++++++++++++++++++++
>  2 files changed, 58 insertions(+)
> 
> diff --git a/sound/soc/samsung/i2s-regs.h b/sound/soc/samsung/i2s-regs.h
> index b4b5d6053503..4444c857d0c0 100644
> --- a/sound/soc/samsung/i2s-regs.h
> +++ b/sound/soc/samsung/i2s-regs.h
> @@ -132,6 +132,7 @@
>  #define EXYNOS7_MOD_RCLK_192FS	7
>  
>  #define PSR_PSREN		(1 << 15)
> +#define PSR_PSVAL(x)		(((x - 1) << 8) & 0x3f00)
>  
>  #define FIC_TX2COUNT(x)		(((x) >>  24) & 0xf)
>  #define FIC_TX1COUNT(x)		(((x) >>  16) & 0xf)
> diff --git a/sound/soc/samsung/i2s.c b/sound/soc/samsung/i2s.c
> index 9505200f3d11..dcb5c438cb2f 100644
> --- a/sound/soc/samsung/i2s.c
> +++ b/sound/soc/samsung/i2s.c
> @@ -50,6 +50,10 @@ struct samsung_i2s_dai_data {
>  	u32 quirks;
>  	unsigned int pcm_rates;
>  	const struct samsung_i2s_variant_regs *i2s_variant_regs;
> +	void (*fixup_early)(struct snd_pcm_substream *substream,
> +					struct snd_soc_dai *dai);
> +	void (*fixup_late)(struct snd_pcm_substream *substream,
> +					struct snd_soc_dai *dai);
>  };
>  
>  struct i2s_dai {
> @@ -111,6 +115,10 @@ struct samsung_i2s_priv {
>  	u32 suspend_i2spsr;
>  
>  	const struct samsung_i2s_variant_regs *variant_regs;
> +	void (*fixup_early)(struct snd_pcm_substream *substream,
> +						struct snd_soc_dai *dai);
> +	void (*fixup_late)(struct snd_pcm_substream *substream,
> +						struct snd_soc_dai *dai);
>  	u32 quirks;
>  
>  	/* The clock provider's data */
> @@ -940,6 +948,10 @@ static int i2s_trigger(struct snd_pcm_substream *substream,
>  	case SNDRV_PCM_TRIGGER_RESUME:
>  	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
>  		pm_runtime_get_sync(dai->dev);
> +
> +		if (priv->fixup_early)
> +			priv->fixup_early(substream, dai);
> +
>  		spin_lock_irqsave(&priv->lock, flags);
>  
>  		if (config_setup(i2s)) {
> @@ -947,6 +959,13 @@ static int i2s_trigger(struct snd_pcm_substream *substream,
>  			return -EINVAL;
>  		}
>  

Except several warnings this patch generates, this is a bit surprising:

> +		spin_unlock_irqrestore(&priv->lock, flags);

You have critical section which you now break into two. You cannot do
this usually. How the synchronization is now kept?

> +
> +		if (priv->fixup_late)
> +			priv->fixup_late(substream, dai);
> +
> +		spin_lock_irqsave(&priv->lock, flags);
> +
>  		if (capture)
>  			i2s_rxctrl(i2s, 1);
>  		else

Best regards,
Krzysztof