Re: [PATCH v2 2/6] ASoC: renesas: fsi: Fix hang by enabling SPU clock

[Bui Duc Phuc]

Hi Morimoto-san,

Thank you for your detailed review and feedback.

> 1st, please insert white line between "int ret = 0;" and "/* enable spu
> clock */".
>
> 2nd, besically, FSI already has "lock", and using it for several protecting.
> Please re-use it, and don't add random new-lock. It makes code confusable.
> Then, please use guard().

I will fix the coding style and use
guard(spinlock_irqsave)(&master->lock) in v3.
It’s much better than adding a new lock.

> 3rd, I don't like above count inc/dec, and mutex_unlock() style, because
> the code unnecessarily complicated. It can be...
>
>       int ret = 0;
>
>         if (master->clk_spu) {
>                 guard(spinlock_irqsave)(&master->lock);
>
>                 if (master->spu_count == 0)
>                         ret = clk_prepare_enable(master->clk_spu);
>
>                 master->spu_count++;
>         }
>         if (ret < 0)
>                 return ret;
>
> I'm not 100% sure, but I guess you need to count up spu_count anyway
> regardless of clk_prepare_enable() result ?

Regarding spu_count, I’m not entirely sure, but if we increment it
even on failure,
the counter might become unbalanced and clk_prepare_enable() may not
be retried on the next call.
Would it be better to increment spu_count only on success to keep the
state consistent?

Also, I have a question about the context here.
Since fsi_hw_startup() and fsi_hw_shutdown() are called from fsi_dai_trigger(),
I think this runs in an atomic context, but please correct me if I'm wrong.
If so, is it safe to call clk_prepare_enable() under guard(spinlock_irqsave)?
Since clk_prepare() can sleep, I’m wondering if this could potentially
cause a "scheduling while atomic" issue.
Would it make more sense to move clk_prepare() to init time (in new
fsi_clk_init() ),
and only use clk_enable() / clk_disable() in the trigger path?

Best regards,
Phuc

On Tue, Apr 14, 2026 at 7:27 AM Kuninori Morimoto
<kuninori.morimoto.gx@xxxxxxxxxxx> wrote:
>
>
> Hi
>
> Hi
>
> > Enable/disable the shared SPU clock in hw startup/shutdown. Without this,
> > accessing FSI registers may hang the system.
> >
> > Suggested-by: Kuninori Morimoto <kuninori.morimoto.gx@xxxxxxxxxxx>
> > Signed-off-by: bui duc phuc <phucduc.bui@xxxxxxxxx>
> > ---
> (snip)
> > @@ -1492,6 +1492,18 @@ static int fsi_hw_startup(struct fsi_priv *fsi,
> >                         struct device *dev)
> >  {
> >       u32 data = 0;
> > +     int ret = 0;
> > +     /* enable spu clock */
> > +     mutex_lock(&fsi->master->clk_lock);
> > +     if (fsi->master->clk_spu && fsi->master->spu_count++ == 0) {
> > +             ret = clk_prepare_enable(fsi->master->clk_spu);
> > +             if (ret < 0) {
> > +                     fsi->master->spu_count--;
> > +                     mutex_unlock(&fsi->master->clk_lock);
> > +                     return ret;
> > +             }
> > +     }
> > +     mutex_unlock(&fsi->master->clk_lock);
>
> 1st, please insert white line between "int ret = 0;" and "/* enable spu
> clock */".
>
> 2nd, besically, FSI already has "lock", and using it for several protecting.
> Please re-use it, and don't add random new-lock. It makes code confusable.
> Then, please use guard().
>
> 3rd, I don't like above count inc/dec, and mutex_unlock() style, because
> the code unnecessarily complicated. It can be...
>
>         int ret = 0;
>
>         if (master->clk_spu) {
>                 guard(spinlock_irqsave)(&master->lock);
>
>                 if (master->spu_count == 0)
>                         ret = clk_prepare_enable(master->clk_spu);
>
>                 master->spu_count++;
>         }
>         if (ret < 0)
>                 return ret;
>
> I'm not 100% sure, but I guess you need to count up spu_count anyway
> regardless of clk_prepare_enable() result ?
>
> Thank you for your help !!
>
> Best regards
> ---
> Kuninori Morimoto