Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:2 as permitted sender) client-ip=2620:137:e000::3:2;
MIME-Version: 1.0
References: <20231005025843.508689-1-takahiro.akashi@linaro.org>
 <20231005025843.508689-6-takahiro.akashi@linaro.org> <20231006132346.GA3426353-robh@kernel.org>
 <CACRpkdaLsfSBEG-h9ZNT2_Lm8tW8AZO7tedDVNeuZoQAqSkyjw@mail.gmail.com>
 <ZSTgTC4cFFpofYAk@octopus> <CACRpkdYD6pkccYoy90AfzV3KT7oYkBPD2_4ZW-AXzT1eUVpchA@mail.gmail.com>
 <ZS3yK/f12Mxw9rXe@octopus>
In-Reply-To: <ZS3yK/f12Mxw9rXe@octopus>
From:   Linus Walleij <linus.walleij@linaro.org>
Date:   Mon, 23 Oct 2023 10:12:21 +0200
Message-ID: <CACRpkdarDrVkPmyDawhZ+H94S4F=dtDSDVuKegi-eNfQNDY3rg@mail.gmail.com>
Subject: Re: [RFC v2 5/5] dt-bindings: gpio: Add bindings for pinctrl based
 generic gpio driver
To:     AKASHI Takahiro <takahiro.akashi@linaro.org>,
        Linus Walleij <linus.walleij@linaro.org>,
        Rob Herring <robh@kernel.org>, sudeep.holla@arm.com,
        cristian.marussi@arm.com, krzysztof.kozlowski+dt@linaro.org,
        conor+dt@kernel.org, Oleksii_Moisieiev@epam.com,
        linux-arm-kernel@lists.infradead.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-gpio@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hi Takashi,

sorry for slow response :(

On Tue, Oct 17, 2023 at 4:32=E2=80=AFAM AKASHI Takahiro
<takahiro.akashi@linaro.org> 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.

> 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 =3D ...;
>             ...
>             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).

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.

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

> 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 =3D <&scmi_pinctrl ... >;
>         }
>     }
> }
>
> I tried:
>     gpio-ranges =3D <0 ...>; // dtc passed, but '0' might be illegal by s=
pec.
>     gpio-ranges =3D <(-1) ...>; // dtc passed, but ...
>     gpio-ranges =3D <&{..} ...>; // dtc error because it's not a full pat=
h.
>
> 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?

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.

Yours,
Linus Walleij