Re: [RFC v2 5/5] dt-bindings: gpio: Add bindings for pinctrl based generic gpio driver

[AKASHI Takahiro <takahiro.akashi@xxxxxxxxxx>]

Hi Linus,

On Mon, Oct 23, 2023 at 10:12:21AM +0200, Linus Walleij wrote:
> Hi Takashi,
> 
> sorry for slow response :(

Thank you for your feedback.

> On Tue, Oct 17, 2023 at 4:32???AM AKASHI Takahiro
> <takahiro.akashi@xxxxxxxxxx> wrote:
> 
> > > > > We can probably mandate that this has to be inside a pin controller
> > > > > since it is a first.
> > > >
> > > > Yeah, my U-Boot implementation tentatively supports both (inside and
> > > > outside pin controller). But it is not a user's choice, but we should
> > > > decide which way to go.
> > >
> > > OK I have decided we are going to put it inside the pin control node,
> > > as a subnode. (I don't expect anyone to object.)
> >
> > While I'm still thinking of how I can modify my current implementation
> > to fit into 'inside' syntax, there are a couple of concerns:
> >
> > 1) invoke gpiochip_add_data() at probe function
> > Probably we no longer need "compatible" property,
> 
> The DT binding people made it clear to me that they really
> like compatibles for this kind of stuff so we should probably
> keep it.

Okay, I will assume this in the following discussion.

> > but instead we need to
> > call gpiochip_add_data() explicitly in SCMI pin controller's probe
> > as follows:
> >
> > scmi_pinctrl_probe()
> >     ...
> >     devm_pinctrl_register_and_init(dev, ..., pctrldev);
> >     pinctrl_enable(pctrldev);
> >
> >     device_for_each_child_node(dev, fwnode)
> >         if (fwnode contains "gpio-controller") {
> >             /* what pin_control_gpio_probe() does */
> >             gc->get_direction = ...;
> >             ...
> >             devm_gpiochip_data_add(dev, gc, ...);
> >         }
> 
> I think it is better of the pin controller just parse and add any
> subdevices (GPIO or other) using of_platform_default_populate()
> (just grep for this function and you will see how many device
> drivers use that).

IICU, then, we will have to add a "compatible" to pinctrl node
to make of_platform_default_populate() work as expected. That is:

scmi {
    ...
    protocol@19 {
        compatible = "simple-bus"; // <- added
        reg = <0x19>;

        ... // a couple of pinconf nodes

        scmi_gpio {
            compatible = "pin-control-gpio";
            ...
        }
    }
}

Is this what you meant?
In this case, however, "protocol@19" has a mixture of sub-nodes,
most are pinconf definitions which are the properties of the pin
controller, while "scmi_gpio" is a separate device.

The code will work, but is it sane from DT binding pov?

> What is good with this approach is that if you place this call
> last in the probe() we know the GPIO driver has all resources
> it needs when it probes so it won't defer.
> 
> > 2) gpio-by-pinctrl.c
> > While this file is SCMI-independent now, due to a change at (1),
> > it would be better to move the whole content inside SCMI pin controller
> > driver (because there is no other user for now).
> 
> That works, too. I have no strong opinion on this subject.

I assumed that "compatible" had been removed here.
If we retain "compatible" property, I don't care either way.

> > 3) Then, pin-control-gpio.yaml may also be put into SCMI binding
> > (i.e. firmware/arm,scmi.yaml). Can we leave the gpio binding outside?
> 
> There is no clear pattern whether to put subdevice bindings into
> the parent device binding or not. Maybe? A lot of MFD devices does
> this for sure.

The same as above.

> > 4) phandle in "gpio-ranges" property
> > (As you mentioned)
> > The first element in a tuple of "gpio-ranges" is a phandle to a pin
> > controller node. Now that the gpio node is a sub node of pin controller,
> > the phandle is trivial. But there is no easier way to represent it
> > than using an explicit label:
> > (My U-Boot implementation does this.)
> >
> > scmi {
> >     ...
> >     scmi_pinctrl: protocol@19 {
> >         ...
> >         gpio {
> >             gpio-controller;
> >             ...
> >             gpio-ranges = <&scmi_pinctrl ... >;
> >         }
> >     }
> > }
> >
> > I tried:
> >     gpio-ranges = <0 ...>; // dtc passed, but '0' might be illegal by spec.
> >     gpio-ranges = <(-1) ...>; // dtc passed, but ...
> >     gpio-ranges = <&{..} ...>; // dtc error because it's not a full path.
> >
> > Do you have any other idea? Otherwise, I will modify my RFC
> > with the changes above.
> 
> If you have the GPIO node inside the pin controller node
> and have all the details of the existing ranges available, there
> is no need to put that into the device tree at all, just omit it?

Then, of_gpiochip_add_data() (hence, of_gpiochip_add()/gpiochip_add_data())
won't work because it always assume phandle + 3 arguments. Right?

In this case, "gpio-ranges" here may have different semantics from
the other pinctrl-based gpio drivers. Doesn't matter from DT pov?

> Instead just call gpiochip_add_pin_range() directly in Linux
> after adding the pin controller and gpio_chip.
> C.f. drivers/pinctrl/pinctrl-sx150x.c for an example of a driver
> doing this. In this case the SX150X is hot-plugged (on a slow
> bus) so it needs to figure out all ranges at runtime anyway.

Are you suggesting implementing a custom function for parsing "gpio-ranges"
and calling it in pin_control_gpio_probe() instead of a generic helper?

Or do you want to always map all the pin controller's pins to
gpio pins as sx150x does?

-Takahiro Akashi

> Yours,
> Linus Walleij