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

On Tue, Aug 11, 2026 at 1:52 PM Geert Uytterhoeven <[email protected]> wrote:
>
> 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?

Let's drop the check then, as it's redundant.

>
> > > +
> > > +       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.

Okay, thanks for confirming.

> 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.

I see. In that case, let's keep the r8a78000_mdlc_attach_dev().

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

Kind regards
Uffe