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
