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
