Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:4 as permitted sender) client-ip=2620:137:e000::3:4;
MIME-Version: 1.0
References: <20231006115147.18559-1-brgl@bgdev.pl>
In-Reply-To: <20231006115147.18559-1-brgl@bgdev.pl>
From:   Linus Walleij <linus.walleij@linaro.org>
Date:   Sat, 7 Oct 2023 00:14:14 +0200
Message-ID: <CACRpkdZP_nOgj77iek_V28Ny8Pb03Xyy-=ho+WqzMHzXajtfqA@mail.gmail.com>
Subject: Re: [RFC/RFT PATCH] gpiolib: reverse-assign the fwnode to struct gpio_chip
To:     Bartosz Golaszewski <brgl@bgdev.pl>
Cc:     Andy Shevchenko <andy@kernel.org>, linux-gpio@vger.kernel.org,
        linux-kernel@vger.kernel.org, Dipen Patel <dipenp@nvidia.com>,
        Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Fri, Oct 6, 2023 at 1:51=E2=80=AFPM Bartosz Golaszewski <brgl@bgdev.pl> =
wrote:

> struct gpio_chip is not only used to carry the information needed to
> set-up a GPIO device but is also used in all GPIOLIB callbacks and is
> passed to the matching functions of lookup helpers.
>
> In that last case, it is currently impossible to match a GPIO device by
> fwnode unless it was explicitly assigned to the chip in the provider
> code. If the fwnode is taken from the parent device, the pointer in
> struct gpio_chip will remain NULL.
>
> If we have a parent device but gc->fwnode was not assigned by the
> provider, let's assign it ourselves so that lookup by fwnode can work in
> all cases.
>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Acked-by: Linus Walleij <linus.walleij@linaro.org>

because we want the code to work (rough consensus and running code)

> -       if (gc->fwnode)
> +       if (gc->fwnode) {
>                 device_set_node(&gdev->dev, gc->fwnode);
> -       else if (gc->parent)
> -               device_set_node(&gdev->dev, dev_fwnode(gc->parent));
> +       } else if (gc->parent) {
> +               parent_fwnode =3D dev_fwnode(gc->parent);
> +               device_set_node(&gdev->dev, parent_fwnode);
> +               gc->fwnode =3D parent_fwnode;

The core of the crux is that we have
information duplication with a reference to the fwnode in two
places. One in gdev->dev and one in gc->fwnode.

gc->of_node was the same duplicated before.

A gdev is created for each gpio_chip so in my naive brain we could
get rid of gc->fwnode and only have the one inside gdev->dev?
+/- some helpful getters/setters if need be.

Or what am I thinking wrong here?

Yours,
Linus Walleij