Re: [RFC 4/6] gpio: add scmi driver based on pinctrl

[AKASHI Takahiro <takahiro.akashi@linaro.org>]

Hi Simon,

On Thu, Sep 07, 2023 at 06:23:05AM -0600, Simon Glass wrote:
> Hi AKASHI,
> 
> On Tue, 5 Sept 2023 at 20:41, AKASHI Takahiro
> <takahiro.akashi@linaro.org> wrote:
> >
> > This DM-compliant driver deals with SCMI pinctrl protocol and presents
> > gpio devices exposed by SCMI firmware (server).
> >
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > ---
> >  drivers/pinctrl/pinctrl-scmi.c | 544 ++++++++++++++++++++++++++++++++-
> >  1 file changed, 539 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/pinctrl/pinctrl-scmi.c b/drivers/pinctrl/pinctrl-scmi.c
> > index 3ebdad57b86c..73d385bdbfcc 100644
> > --- a/drivers/pinctrl/pinctrl-scmi.c
> > +++ b/drivers/pinctrl/pinctrl-scmi.c
> > @@ -11,21 +11,20 @@
> >  #include <malloc.h>
> >  #include <scmi_agent.h>
> >  #include <scmi_protocols.h>
> > +#include <asm-generic/gpio.h>
> >  #include <dm/device_compat.h>
> > +#include <dm/device-internal.h>
> > +#include <dm/lists.h>
> >  #include <dm/pinctrl.h>
> >
> [..]
> 
> > +
> > +U_BOOT_DRIVER(scmi_gpio) = {
> > +       .name = "scmi_gpio",
> > +       .id = UCLASS_GPIO,
> > +       .of_match = scmi_gpio_ids,
> > +       .of_to_plat = scmi_gpio_probe,
> > +       .ops = &scmi_gpio_ops,
> > +       .plat_auto = sizeof(struct scmi_pinctrl_gpio_plat),
> > +};
> > +
> > +/**
> > + * scmi_gpiochip_register - Create a pinctrl-controlled gpio device
> > + * @parent:    SCMI pinctrl device
> > + *
> > + * Create a pinctrl-controlled gpio device
> > + *
> > + * Return:     0 on success, error code on failure
> > + */
> > +static int scmi_gpiochip_register(struct udevice *parent)
> > +{
> > +       ofnode node;
> > +       struct driver *drv;
> > +       struct udevice *gpio_dev;
> > +       int ret;
> > +
> > +       /* TODO: recovery if failed */
> > +       dev_for_each_subnode(node, parent) {
> > +               if (!ofnode_is_enabled(node))
> > +                       continue;
> 
> Can we not rely on the normal driver model binding to bind these
> devices? Why does it need to be done manually here?

First, please take a look at the cover letter.
In this RFC, I basically assume two patterns of DT bindings,
(A) and (B) in the cover letter (or sandbox's test.dts in patch#5).

In (B), a DT node as a gpio device, which is essentially a child
of pinctrl device, is located *under* a pinctrl device. It need
to be probed manually as there is no implicit method to enumerate
it as a DM device automatically.

On the other hand, in (A), the same node can be put anywhere in a DT
as it contains a "compatible" property to identify itself.
So if we want to only support (A), scmi_gpiochip_register() and
scmi_pinctrl_bind() are not necessary.

Since there is no discussion about bindings for GPIO managed by SCMI
pinctrl device yet on the kernel side, I have left two solutions
in this RFC.

Thanks,
-Takakahiro Akashi


> > +
> > +               if (!ofnode_read_prop(node, "gpio-controller", NULL))
> > +                       /* not a GPIO node */
> > +                       continue;
> > +
> > +               drv = DM_DRIVER_GET(scmi_gpio);
> > +               if (!drv) {
> > +                       dev_err(parent, "No gpio driver?\n");
> > +                       return -ENODEV;
> > +               }
> > +
> > +               ret = device_bind(parent, drv, ofnode_get_name(node), NULL,
> > +                                 node, &gpio_dev);
> > +               if (ret) {
> > +                       dev_err(parent, "failed to bind %s to gpio (%d)\n",
> > +                               ofnode_get_name(node), ret);
> > +                       return -ENODEV;
> > +               }
> > +
> > +               return 0;
> > +       }
> > +
> > +       return -ENODEV;
> > +}
> > +
> 
> Regards,
> Simon