Re: [PATCH v3 18/25] ASoC: renesas: rz-ssi: Issue software reset in hw_params API

[Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>]

On Wed, Nov 13, 2024 at 2:36 PM Claudiu <claudiu.beznea@xxxxxxxxx> wrote:
> From: Claudiu Beznea <claudiu.beznea.uj@xxxxxxxxxxxxxx>
>
> The code initially issued software reset on SNDRV_PCM_TRIGGER_START
> action only before starting the first stream. This can be easily moved to
> hw_params() as the action is similar to setting the clocks. Moreover,
> according to the hardware manual (Table 35.7 Bits Initialized by Software
> Reset of the SSIFCR.SSIRST Bit) the software reset action acts also on the
> clock dividers bits. Due to this issue the software reset in hw_params()
> before configuring the clock dividers. This also simplifies the code in
> trigger API.
>
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@xxxxxxxxxxxxxx>

Reviewed-by: Geert Uytterhoeven <geert+renesas@xxxxxxxxx>

> --- a/sound/soc/renesas/rz-ssi.c
> +++ b/sound/soc/renesas/rz-ssi.c
> @@ -388,6 +388,15 @@ static int rz_ssi_start(struct rz_ssi_priv *ssi, struct rz_ssi_stream *strm)
>         return 0;
>  }
>
> +static int rz_ssi_swreset(struct rz_ssi_priv *ssi)
> +{
> +       u32 tmp;
> +
> +       rz_ssi_reg_mask_setl(ssi, SSIFCR, SSIFCR_SSIRST, SSIFCR_SSIRST);

Nit: no need to clear SSIFCR_SSIRST first:

    rz_ssi_reg_mask_setl(ssi, SSIFCR, 0, SSIFCR_SSIRST);

cfr. what the original code did below.

> +       rz_ssi_reg_mask_setl(ssi, SSIFCR, SSIFCR_SSIRST, 0);
> +       return readl_poll_timeout_atomic(ssi->base + SSIFCR, tmp, !(tmp & SSIFCR_SSIRST), 1, 5);
> +}
> +
>  static int rz_ssi_stop(struct rz_ssi_priv *ssi, struct rz_ssi_stream *strm)
>  {
>         strm->running = 0;
> @@ -782,14 +791,6 @@ static int rz_ssi_dai_trigger(struct snd_pcm_substream *substream, int cmd,
>
>         switch (cmd) {
>         case SNDRV_PCM_TRIGGER_START:
> -               /* Soft Reset */
> -               if (!rz_ssi_is_stream_running(&ssi->playback) &&
> -                   !rz_ssi_is_stream_running(&ssi->capture)) {
> -                       rz_ssi_reg_mask_setl(ssi, SSIFCR, 0, SSIFCR_SSIRST);
> -                       rz_ssi_reg_mask_setl(ssi, SSIFCR, SSIFCR_SSIRST, 0);
> -                       udelay(5);
> -               }
> -
>                 rz_ssi_stream_init(strm, substream);
>
>                 if (ssi->dma_rt) {

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds