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

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

Hi Simon,

On Sun, Sep 10, 2023 at 01:13:24PM -0600, Simon Glass wrote:
> Hi,
> 
> On Thu, 7 Sept 2023 at 22:32, AKASHI Takahiro
> <takahiro.akashi@linaro.org> wrote:
> >
> > 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.
> 
> You could add a post_bind() method to the pinctrl uclass to call
> dm_scan_fdt_dev() as we do with TPM.

Okay, but in that case, defining "compatible" property will become
mandatory, while I invented (B) so that we don't need extra bindings.

Please note that no discussion started about the binding of gpio devices.

-Takahiro Akashi

> >
> > 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.
> 
> 
> OK.
> 
> [..]
> 
> Regards,
> Simon