Re: [PATCH v4] pmdomain: renesas: Add R-Car X5H MDLC driver

Hi Ulf,

On Tue, 11 Aug 2026 at 12:16, Ulf Hansson <[email protected]> wrote:
> On Tue, Aug 11, 2026 at 10:58 AM Geert Uytterhoeven
> <[email protected]> wrote:
> > Add a minimal Module Controller driver for the R-Car X5H (R8A78000) SoC.
> > For now this just supports the always-on power domains, and dummy module
> > clocks and resets for the serial console (which is enabled by the boot
> > loader).
> >
> > Signed-off-by: Geert Uytterhoeven <[email protected]>
>
> I had a closer look at the attach part and there is few questions,
> please see below.

Thanks for your comments!

> > --- /dev/null
> > +++ b/drivers/pmdomain/renesas/r8a78000-mdlc.c

> > +static int r8a78000_mdlc_attach_dev(struct generic_pm_domain *domain,
> > +                                   struct device *dev)
> > +{
> > +       struct device_node *np = dev->of_node;
> > +       struct r8a78000_mdlc_priv *priv;
> > +       struct of_phandle_args pd_spec;
> > +       const struct mod_map *map;
> > +       unsigned int id;
> > +       int ret;
> > +
> > +       ret = of_parse_phandle_with_args(np, "power-domains",
> > +                                        "#power-domain-cells", 0, &pd_spec);
> > +       if (ret < 0)
> > +               return ret;
> > +
> > +       if (pd_spec.args_count != 2) {
> > +               of_node_put(pd_spec.np);
> > +               return -EINVAL;
> > +       }
>
> Is the above check really needed?
>
> Assuming r8a78000_genpd_xlate() is used for attaching, then the
> args_count must always be 2, otherwise we return an error code when
> trying to attach the device to its genpd, right?

That is indeed true if power-domains contains a single entry pointing
to an MDLC domain.
This check also protects against the (currently unlikely) case where
there are multiple entries. But for that to work well, it should indeed
validate pd_spec.np, too.

Is this good enough for now, or do you want me to drop the check?

> > +
> > +       scoped_guard(mutex, &r8a78000_mdlc_lock) {
> > +               hlist_for_each_entry(priv, &r8a78000_mdlc_list, link) {
> > +                       if (priv->np == pd_spec.np)
> > +                               break;
> > +               }
> > +       }
> > +
> > +       if (!priv) {
> > +               dev_err(dev, "%s: MDLC %pOF not found\n", __func__, pd_spec.np);
> > +               of_node_put(pd_spec.np);
> > +               return -ENODEV;
> > +       }
> > +
> > +       id = pd_spec.args[1];
> > +       of_node_put(pd_spec.np);
> > +
> > +       map = mod_map_find(priv->mod_map, id);
> > +       if (!map) {
> > +               dev_err(dev, "Unknown module 0x%x\n", id);
> > +               return -ENOENT;
> > +       }
>
> Would it not be better (if possible) to do the above verification as a
> part of the r8a78000_genpd_xlate() instead?

In the current minimal driver, the verification could indeed be done
in the .xlate() callback.
However, subsequent patches will add actual data to the retrieved map
entry.  As the .xlate() callback cannot pass data for later use by the
.attach_dev() callback, the latter has to call mod_map_find() anyway.

> > +
> > +       dev_dbg(dev, "Ignoring HW module 0x%x\n", id);
> > +       return 0;
> > +}

Thanks!

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

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