Re: [PATCH v3 07/10] ASoC: renesas: fsi: refactor clock initialization

Hi all,

First of all, I appreciate receiving review comments from the AI bot.
I think it is quite helpful for learning and improving patches.

However, I have a few questions regarding the expected workflow:

1. I noticed that the bot reply only CCs a subset of the original recipient=
s.
    Is there a specific reason or convention behind that behavior?
2. When receiving review comments from the bot, is it generally
    expected that contributors reply to those comments?
3. If replying is recommended, should I keep the same CC list as the
    original patch submission, or only reply to the bot thread participants=
?

Sorry if these are basic questions. I just want to better understand the
usual mailing list workflow.

Best Regards,
Phuc

On Tue, May 12, 2026 at 6:47=E2=80=AFAM <[email protected]> wrote:
>
> Thank you for your contribution! Sashiko AI review found 1 potential issu=
e(s) to consider:
> - [Critical] The return value of `fsi_clk_init()` is ignored in `fsi_prob=
e()`, which breaks probe deferral and leads to a kernel panic from derefere=
ncing error pointers.
> --
>
> commit 055b3b167e655ece1592450b8baaea9abe771d10
> Author: bui duc phuc <[email protected]>
>
> ASoC: renesas: fsi: refactor clock initialization
>
> This commit moves fsi_clk_init() to probe and uses devm_clk_get_optional(=
)
> for optional clocks. It allows probe to succeed when optional clocks are
> missing, while set_rate() validates clock availability.
>
> > diff --git a/sound/soc/renesas/fsi.c b/sound/soc/renesas/fsi.c
> > index 55a11f1fe8aa1..8c46d68069581 100644
> > --- a/sound/soc/renesas/fsi.c
> > +++ b/sound/soc/renesas/fsi.c
> [ ... ]
> > @@ -973,35 +983,42 @@ static int fsi_clk_set_rate_cpg(struct device *de=
v,
> >       return ret;
> >  }
> >
> > -static int fsi_clk_init(struct device *dev,
> > -                     struct fsi_priv *fsi,
> > -                     int xck,
> > -                     int ick,
> > -                     int div,
> > -                     int (*set_rate)(struct device *dev,
> > -                                     struct fsi_priv *fsi))
> > +static int fsi_clk_init(struct device *dev, struct fsi_priv *fsi)
> >  {
> [ ... ]
> >       clock->own =3D devm_clk_get(dev, NULL);
> >       if (IS_ERR(clock->own))
> >               return -EINVAL;
>
> Does returning -EINVAL here mask probe deferral? If devm_clk_get() return=
s
> -EPROBE_DEFER, returning -EINVAL will permanently fail the driver probe
> rather than deferring it.
>
> [ ... ]
> > @@ -1997,6 +2001,7 @@ static int fsi_probe(struct platform_device *pdev=
)
> >       fsi->running_streams =3D 0;
> >       fsi_port_info_init(fsi, &info.port_a);
> >       fsi_handler_init(fsi, &info.port_a);
> > +     fsi_clk_init(&pdev->dev, fsi);
>
> Is it safe to ignore the return value of fsi_clk_init() here?
>
> If devm_clk_get_optional() fails and returns an error pointer like
> -EPROBE_DEFER, those error pointers will be stored in the clock fields
> instead of NULL.
>
> Later, when fsi_clk_set_rate_external() checks if the clocks are missing:
>
>     if (!xck || !ick) {
>
> This check will pass because the error pointers are not NULL. The error
> pointers will then be passed to common clock framework APIs like
> clk_get_rate(), which will dereference them and cause a crash.
>
> Should the return value of fsi_clk_init() be checked and propagated?
>
> >       ret =3D fsi_stream_probe(fsi, &pdev->dev);
> >       if (ret < 0) {
> >               dev_err(&pdev->dev, "FSIA stream probe failed\n");
> > @@ -2011,6 +2016,7 @@ static int fsi_probe(struct platform_device *pdev=
)
> >       fsi->running_streams =3D 0;
> >       fsi_port_info_init(fsi, &info.port_b);
> >       fsi_handler_init(fsi, &info.port_b);
> > +     fsi_clk_init(&pdev->dev, fsi);
>
> Would the same return value check be needed here as well?
>
> >       ret =3D fsi_stream_probe(fsi, &pdev->dev);
> >       if (ret < 0) {
> >               dev_err(&pdev->dev, "FSIB stream probe failed\n");
>
> --
> Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/20260510084303.12=
[email protected]?part=3D7