Message ID | 20231006115147.18559-1-brgl@bgdev.pl |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:a888:0:b0:403:3b70:6f57 with SMTP id x8csp261037vqo; Fri, 6 Oct 2023 04:52:26 -0700 (PDT) X-Google-Smtp-Source: AGHT+IH0l9DfXZ0iWl8dvEoTVPgOorh8iShd3z/6e0VZ+dA+vVCDLgo0QZ8z3eKJBROyoF0ttPcI X-Received: by 2002:a05:6358:885:b0:143:91a2:8e0a with SMTP id m5-20020a056358088500b0014391a28e0amr8558740rwj.4.1696593145801; Fri, 06 Oct 2023 04:52:25 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1696593145; cv=none; d=google.com; s=arc-20160816; b=eczkkEnszKhN27M9+FcyNqdgPixL2F2yVZ6kxUD5PokSYeJxHW8AkbfsmMhDjylioY JKdGjtcFZHS4e8ypFwNfsjP7Da+9SXiAshVU7kQ7ipZHYPa9+fPvelBOvKtGpcXeZ/Ha ZqBexdzarRMlj9MIslnEwme8b0Sh3PEysocoI62+OgF0aG5qIqueNgi1ffuqZfBNmdaI IWK5nnMyjL5KTs3urhlxKkGvi2F2dj+sZAMgdWzLXTurl5jm2x0OnG0t1jGyxIjMYQQd zP7rxOj7tksdCYnVPUv5JB0ktWDUZAJoh7pfyndNQ8Tdkg3L/Iu+EYuPqRFfYrrhjPZs 1PhQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :message-id:date:subject:cc:to:from:dkim-signature; bh=C9Fu9pZwHvbe3yOhUi4vBaHVRbGW0hT6b2oIce4vCFM=; fh=vv3EsjwVz5pzmqJ+4niI1G2GNVisLD44RGDJMTl647o=; b=mrSsxaSy1lPiYKen486wtKGMywaLVgIEc/uvkAR6UnfO4kv7LTsGAolqKRTpRHWDHr j0buHmySlCvyYcfH/IrP8QKXF+SBqX9zlrk6ohFqkRQWcghJwyvIyYMX67Z5Fm2kudbt wwxIYDeTYJvdPr0ZU0rU+6jj3XTHCioPgb4FttAvF45F0RANztWsjOoYYAoEHYahiEFE htdaYzKTXfj2ORUz57UJYAMeRhenY7mbMvkCJ11Qi0Zx/65J4TXLJ3S3HsvDBW8uRkIc 1HOz+T1jpojCLOOHaMUl/8UgdqfTBRJiPI/lR2xS18ypH1RA+k1AFA6mMd4dUdOxV05d E+qg== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@bgdev-pl.20230601.gappssmtp.com header.s=20230601 header.b=j25lHW8t; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:4 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: from howler.vger.email (howler.vger.email. [2620:137:e000::3:4]) by mx.google.com with ESMTPS id u21-20020a63b555000000b005897bfc2ecbsi2553821pgo.205.2023.10.06.04.52.25 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 06 Oct 2023 04:52:25 -0700 (PDT) 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; Authentication-Results: mx.google.com; dkim=pass header.i=@bgdev-pl.20230601.gappssmtp.com header.s=20230601 header.b=j25lHW8t; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:4 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: from out1.vger.email (depot.vger.email [IPv6:2620:137:e000::3:0]) by howler.vger.email (Postfix) with ESMTP id ACBDA822AE44; Fri, 6 Oct 2023 04:52:23 -0700 (PDT) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.103.10 at howler.vger.email Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232127AbjJFLwB (ORCPT <rfc822;ezelljr.billy@gmail.com> + 18 others); Fri, 6 Oct 2023 07:52:01 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:50324 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232093AbjJFLv7 (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Fri, 6 Oct 2023 07:51:59 -0400 Received: from mail-lf1-x131.google.com (mail-lf1-x131.google.com [IPv6:2a00:1450:4864:20::131]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id C8BD7EA for <linux-kernel@vger.kernel.org>; Fri, 6 Oct 2023 04:51:57 -0700 (PDT) Received: by mail-lf1-x131.google.com with SMTP id 2adb3069b0e04-5033918c09eso2597568e87.2 for <linux-kernel@vger.kernel.org>; Fri, 06 Oct 2023 04:51:57 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bgdev-pl.20230601.gappssmtp.com; s=20230601; t=1696593116; x=1697197916; darn=vger.kernel.org; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:from:to:cc:subject:date:message-id:reply-to; bh=C9Fu9pZwHvbe3yOhUi4vBaHVRbGW0hT6b2oIce4vCFM=; b=j25lHW8tVld5f1597X5nkczjlroK+lMkJQ+S1vZSb16XXofGD3q5Fg6UF/vUaMIsQ1 5kt3eQ7EsMY2h+YEpj1JriY5s6qPzcmf7OmW0ZTVfD2whR8J9A1kMo8GDz9lJ/NDZOOa P8sKxKWcss1OAGHKNLnir0TM46VUOAy+O6ByzUpOVDrpcf4wyIOrjwhTKDpbC09Zcwn+ d5NnmbxTcdQNXtJMIkX7reqGGQK/PkLMYZikvnXpwom8kSICGsEDNaX3M+NIcSsCS3ry J54BAD1K5SoWEp1dAG1NJMBmTynj0m+HPdaU/V3A4GMheiF70KEdl6eYv3KqaDYm/vRK dFCg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1696593116; x=1697197916; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=C9Fu9pZwHvbe3yOhUi4vBaHVRbGW0hT6b2oIce4vCFM=; b=t55fUPAQGqZZnzXAjdRgX3nJrUk/CnWkRiQZH8RpwOPThlxLAW9Eadww6Nr6u5cdzg ywjlft6anGQz6dktUYIWxoWS+e60Jc91+hbyJW9QuyLxEB2KQs/n3qy/7E6vKHJr1N0F cWr7dakT7R+nAcLArcSPbjQ8D570Brcqb+OV5Pi2p4wsTUID+ctn9dl5rGbXaLvkMLBz mOaqofx3PiUgKk27+7l4jIrRD9e0wXdN5m24HQaBRHHRrLYMwn9OgSZCap6e1gW1Sr6E TfsdDn9DfclJdzhcCLGia582HTPyiXXhavxH+E8Iu/ZhCmVP/KvTa+uWKbJ/zHfqHnDH zW8w== X-Gm-Message-State: AOJu0YwOKGxQbhyJb8i8bcII7s+ei+Yoh2qQZVExFsJbIOhyV8kvKQMs ydEo65T9PLsZBmzJ2Vu0B5eK9hK+IC+Ps6YBCrE= X-Received: by 2002:a05:6512:3488:b0:503:a76:4eeb with SMTP id v8-20020a056512348800b005030a764eebmr6317894lfr.16.1696593115805; Fri, 06 Oct 2023 04:51:55 -0700 (PDT) Received: from brgl-uxlite.home ([2a01:cb1d:334:ac00:8160:8fdc:c61c:a0a6]) by smtp.gmail.com with ESMTPSA id i2-20020a05600c290200b004063d8b43e7sm5834057wmd.48.2023.10.06.04.51.55 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 06 Oct 2023 04:51:55 -0700 (PDT) From: Bartosz Golaszewski <brgl@bgdev.pl> To: Linus Walleij <linus.walleij@linaro.org>, Andy Shevchenko <andy@kernel.org> Cc: linux-gpio@vger.kernel.org, linux-kernel@vger.kernel.org, Dipen Patel <dipenp@nvidia.com>, Bartosz Golaszewski <bartosz.golaszewski@linaro.org> Subject: [RFC/RFT PATCH] gpiolib: reverse-assign the fwnode to struct gpio_chip Date: Fri, 6 Oct 2023 13:51:47 +0200 Message-Id: <20231006115147.18559-1-brgl@bgdev.pl> X-Mailer: git-send-email 2.39.2 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=2.8 required=5.0 tests=DKIM_SIGNED,DKIM_VALID, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,RCVD_IN_SBL_CSS, SPF_HELO_NONE,SPF_PASS autolearn=no autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on howler.vger.email Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (howler.vger.email [0.0.0.0]); Fri, 06 Oct 2023 04:52:23 -0700 (PDT) X-Spam-Level: ** X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1779006854484439729 X-GMAIL-MSGID: 1779006854484439729 |
Series |
[RFC/RFT] gpiolib: reverse-assign the fwnode to struct gpio_chip
|
|
Commit Message
Bartosz Golaszewski
Oct. 6, 2023, 11:51 a.m. UTC
From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> 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> --- This is something that Dipen reported with one of the tegra drivers where a GPIO lookup by fwnode does not work because the fwnode pointer in struct gpio_chip is NULL. This patch addresses this use-case. drivers/gpio/gpiolib.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)
Comments
On Fri, Oct 06, 2023 at 01:51:47PM +0200, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > 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. That's expected behaviour. > 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. I don't think this is a good change. We paper over the real issue where we and callers need to understand what they are looking for. ... > This is something that Dipen reported with one of the tegra drivers where > a GPIO lookup by fwnode does not work because the fwnode pointer in struct > gpio_chip is NULL. This patch addresses this use-case. I am not sure I understand the problem here. All these should have been addressed already, no? So, the GPIOLIB should use dev_fwnode(&gdev->dev) inside it, outside it the GPIO drivers are free to use gc->fwnode as long as they understand the lifetime of the respective object.
On Fri, Oct 06, 2023 at 01:51:47PM +0200, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > 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. ... > + gc->fwnode = parent_fwnode; Ah, this is basically reverts my commit, the whole idea of which was to go towards constant struct gpio_chip object that is supplied by a provider.
On Fri, Oct 6, 2023 at 3:24 PM Andy Shevchenko <andy@kernel.org> wrote: > > On Fri, Oct 06, 2023 at 01:51:47PM +0200, Bartosz Golaszewski wrote: > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > 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. > > ... > > > + gc->fwnode = parent_fwnode; > > Ah, this is basically reverts my commit, the whole idea of which was to go > towards constant struct gpio_chip object that is supplied by a provider. > Then this idea was wrong in the first place and that goal will never be achieved. Whether that's a correct approach is questionable but struct gpio_chip has become so much more than a simple config structure and - given how ubiquitous GPIO providers are throughout the different subsystems of the kernel - it'll stay that way unless we're ready to rebuild every GPIO provider in linux. The best we can do now is at least make its usage safe. Meaning: it's a structure with which providers will interact using GPIOLIB callbacks which will in turn assure that during the execution of any function taking struct gpio_chip as argument, it will remain alive and protected from concurrent access. The providers however will continue to use gpio_chip for many purposes. One of such purposes is matching the GPIO device BY its backing gpio_chip structure. It not having the same fwnode in this particular case is an inconsistency rather than design IMO. I don't see any good reason for it not having the fwnode assigned. User calling gpio_device_find() will have to jump through hoops in order to match the device by fwnode (include gpiolib.h and dereference gpiodev?) but it could be very easily facilitated by just assigning it at registration-time - just like we assign a whole bunch of other pointers and data structures. Bart > -- > With Best Regards, > Andy Shevchenko > >
On Fri, Oct 6, 2023 at 3:15 PM Andy Shevchenko <andy@kernel.org> wrote: > > On Fri, Oct 06, 2023 at 01:51:47PM +0200, Bartosz Golaszewski wrote: > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > 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. > > That's expected behaviour. > Is it though? We now have a GPIO device that represents a piece of physical hardware that has an fwnode assigned and the associated GPIO chip (tied to that device) that has none. How is that logical? It's not coherent. I'm not surprised users of that code will be confused - like Dipen in this case. Bart > > 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. > > I don't think this is a good change. We paper over the real issue where > we and callers need to understand what they are looking for. > > ... > > > This is something that Dipen reported with one of the tegra drivers where > > a GPIO lookup by fwnode does not work because the fwnode pointer in struct > > gpio_chip is NULL. This patch addresses this use-case. > > I am not sure I understand the problem here. All these should have been > addressed already, no? > > So, the GPIOLIB should use dev_fwnode(&gdev->dev) inside it, outside it > the GPIO drivers are free to use gc->fwnode as long as they understand > the lifetime of the respective object. > > > -- > With Best Regards, > Andy Shevchenko > >
On Fri, Oct 6, 2023 at 1:51 PM 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 = dev_fwnode(gc->parent); > + device_set_node(&gdev->dev, parent_fwnode); > + gc->fwnode = 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
On Fri, Oct 6, 2023 at 9:08 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > I don't see any good reason for it not having the fwnode assigned. > User calling gpio_device_find() will have to jump through hoops in > order to match the device by fwnode Yeah I would add struct fwnode_handle *gpiochip_get_fwnode(struct gpio_chip *gc) { return dev_fwnode(&gc->gpiodev->dev); } so it's easy for external users to get the fwnode if they really need it. This and a few more changes and we can drop gc->fwnode altogether can't we? Yours, Linus Walleij
On Sat, Oct 7, 2023 at 1:14 AM Linus Walleij <linus.walleij@linaro.org> wrote: > On Fri, Oct 6, 2023 at 1:51 PM 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) > > 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. No, we don't. We have plenty of drivers that have gc->fwnode == NULL, which means that it is shared with the parent device. ... > 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? That would work I think. But I'm definitely against this change. It is the way to nowhere. We should really be quite strict about fwnode and do NOT assign the gc one behind the provider's back. If something is not working in this scenario, that should be fixed and not with a hack like this.
On Fri, Oct 06, 2023 at 09:07:49PM +0200, Bartosz Golaszewski wrote: > On Fri, Oct 6, 2023 at 3:24 PM Andy Shevchenko <andy@kernel.org> wrote: > > > > On Fri, Oct 06, 2023 at 01:51:47PM +0200, Bartosz Golaszewski wrote: > > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > > > 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. ... > > > + gc->fwnode = parent_fwnode; > > > > Ah, this is basically reverts my commit, the whole idea of which was to go > > towards constant struct gpio_chip object that is supplied by a provider. > > Then this idea was wrong in the first place and that goal will never > be achieved. Why not? You always can have internal opaque data structure that takes constant object from the provider. > Whether that's a correct approach is questionable but > struct gpio_chip has become so much more than a simple config > structure and - given how ubiquitous GPIO providers are throughout the > different subsystems of the kernel - it'll stay that way unless we're > ready to rebuild every GPIO provider in linux. > The best we can do now is at least make its usage safe. Meaning: it's > a structure with which providers will interact using GPIOLIB callbacks > which will in turn assure that during the execution of any function > taking struct gpio_chip as argument, it will remain alive and > protected from concurrent access. > > The providers however will continue to use gpio_chip for many > purposes. One of such purposes is matching the GPIO device BY its > backing gpio_chip structure. It not having the same fwnode in this > particular case is an inconsistency rather than design IMO. > > I don't see any good reason for it not having the fwnode assigned. I see it clearly that we don't need to go this dead end. The fwnode used by GPIO devices is not semantically the same as fwnode that is supplied by the provider. Moreover, your patch will bring a clear layering violation since it changes the member behind the owner's back. > User calling gpio_device_find() will have to jump through hoops in > order to match the device by fwnode (include gpiolib.h and dereference > gpiodev?) but it could be very easily facilitated by just assigning it > at registration-time - just like we assign a whole bunch of other > pointers and data structures. No, please do not do this hack with fwnode.
On Sat, Oct 07, 2023 at 12:22:01AM +0200, Linus Walleij wrote: > On Fri, Oct 6, 2023 at 9:08 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > I don't see any good reason for it not having the fwnode assigned. > > User calling gpio_device_find() will have to jump through hoops in > > order to match the device by fwnode > > Yeah I would add > > struct fwnode_handle *gpiochip_get_fwnode(struct gpio_chip *gc) > { > return dev_fwnode(&gc->gpiodev->dev); > } > > so it's easy for external users to get the fwnode if they really need it. > This and a few more changes and we can drop gc->fwnode altogether > can't we? This would work, but the problem here is to understand which fwnode (semantically) the caller wants to use. One is the GPIO device's, and the other is what provider explicitly assigned. Currently the latter case is transparent in a sense that GPIO device will get the same fwnode as GPIO chip submitted by the provider. Internally GPIOLIB must use GPIO device fwnode and rely only on it. Externally it depends. Basically it's provider's business to know if it is safe to use gc->fwnode or not and when.
On Fri, Oct 06, 2023 at 09:07:54PM +0200, Bartosz Golaszewski wrote: > On Fri, Oct 6, 2023 at 3:15 PM Andy Shevchenko <andy@kernel.org> wrote: > > > > On Fri, Oct 06, 2023 at 01:51:47PM +0200, Bartosz Golaszewski wrote: > > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > > > 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. > > > > That's expected behaviour. > > Is it though? We now have a GPIO device that represents a piece of > physical hardware that has an fwnode assigned and the associated GPIO > chip (tied to that device) that has none. How is that logical? It's > not coherent. To me it is pretty much logical, yes. The providers decide themselves if they want to have any specific device node for the chip or inherit it from the physical hardware. Note, there are two types of the FW descriptions of the GPIO controller, when it's 1:1 to the banks and when it's one device with list of children, one per bank. Due to this differences we have this field in the GPIO chip to begin with. > I'm not surprised users of that code will be confused - > like Dipen in this case. Which case? I'm still unsure you pictured the issue here. Where can I read about it? > > > 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. > > > > I don't think this is a good change. We paper over the real issue where > > we and callers need to understand what they are looking for. > > > > ... > > > > > This is something that Dipen reported with one of the tegra drivers where > > > a GPIO lookup by fwnode does not work because the fwnode pointer in struct > > > gpio_chip is NULL. This patch addresses this use-case. > > > > I am not sure I understand the problem here. All these should have been > > addressed already, no? > > > > So, the GPIOLIB should use dev_fwnode(&gdev->dev) inside it, outside it > > the GPIO drivers are free to use gc->fwnode as long as they understand > > the lifetime of the respective object. > > > > > > -- > > With Best Regards, > > Andy Shevchenko > > > >
On Sat, Oct 7, 2023 at 9:45 AM Andy Shevchenko <andy@kernel.org> wrote: > The providers decide themselves > if they want to have any specific device node for the chip or inherit > it from the physical hardware. Note, there are two types of the FW descriptions > of the GPIO controller, when it's 1:1 to the banks and when it's one device > with list of children, one per bank. Due to this differences we have > this field in the GPIO chip to begin with. So this is the real problem right, for example a pin controller with six sub-nodes that are GPIO chips but not necessarily devices. In DT terms: pinctrl@0 { compatible = "..."; (...) gpio@0 { (...) }; gpio@1 { (....) }; (...) gpio@5 { (....) }; }; And now each of these subnodes spawn a gpio_chip and a gpio_device and now the big question is what fwnode (in this example of node) to use for each. I was under the impression that despite the main device does not spawn subdevices in the Linux device model, the subdevices always have their individual fwnode, and when we create the gpio_chip for it, that creates a gpio_device and that *is* the device for that fwnode, if there is no corresponding say platform_device or similar. So e.g. assigning the fwnode of the parent (the pin controller in this example) to these gpio_chips flattens the hierarchy and that would be a *bug* in my mind (but maybe not in everyone else's mind). But IIUC what you are saying is that sometimes there are drivers that do this? Yours, Linus Walleij
On Sat, Oct 7, 2023 at 9:45 AM Andy Shevchenko <andy@kernel.org> wrote: > > On Fri, Oct 06, 2023 at 09:07:54PM +0200, Bartosz Golaszewski wrote: > > On Fri, Oct 6, 2023 at 3:15 PM Andy Shevchenko <andy@kernel.org> wrote: > > > > > > On Fri, Oct 06, 2023 at 01:51:47PM +0200, Bartosz Golaszewski wrote: > > > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > > > > > 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. > > > > > > That's expected behaviour. > > > > Is it though? We now have a GPIO device that represents a piece of > > physical hardware that has an fwnode assigned and the associated GPIO > > chip (tied to that device) that has none. How is that logical? It's > > not coherent. > > To me it is pretty much logical, yes. The providers decide themselves > if they want to have any specific device node for the chip or inherit > it from the physical hardware. Note, there are two types of the FW descriptions > of the GPIO controller, when it's 1:1 to the banks and when it's one device > with list of children, one per bank. Due to this differences we have > this field in the GPIO chip to begin with. > This is irrelevant for this discussion. The tegra driver in question knows which fwnode it's using - the one from the parent device. It's just that when the HTE driver tries to find the chip using either gpiochip_find() or gpio_device_find(), it fails and I'm pretty sure that if Dipen bisected it, it would point to commit daecca4b8433 ("gpiolib: Do not alter GPIO chip fwnode member"). IMO the GPIO subsystem should take a phandle to the HTE engine it uses for timestamping and that would allow us to not do the lookup at all but that's a different discussion. Anyway, I think Linus' suggestion is better than this patch. Bart > > > I'm not surprised users of that code will be confused - > > like Dipen in this case. > > Which case? I'm still unsure you pictured the issue here. > Where can I read about it? > > > > > 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. > > > > > > I don't think this is a good change. We paper over the real issue where > > > we and callers need to understand what they are looking for. > > > > > > ... > > > > > > > This is something that Dipen reported with one of the tegra drivers where > > > > a GPIO lookup by fwnode does not work because the fwnode pointer in struct > > > > gpio_chip is NULL. This patch addresses this use-case. > > > > > > I am not sure I understand the problem here. All these should have been > > > addressed already, no? > > > > > > So, the GPIOLIB should use dev_fwnode(&gdev->dev) inside it, outside it > > > the GPIO drivers are free to use gc->fwnode as long as they understand > > > the lifetime of the respective object. > > > > > > > > > -- > > > With Best Regards, > > > Andy Shevchenko > > > > > > > > -- > With Best Regards, > Andy Shevchenko > >
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index 191f9c87b4d0..a0e3d255fb73 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -782,6 +782,7 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data, struct lock_class_key *lock_key, struct lock_class_key *request_key) { + struct fwnode_handle *parent_fwnode; struct gpio_device *gdev; unsigned long flags; unsigned int i; @@ -806,10 +807,13 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data, * If the calling driver did not initialize firmware node, * do it here using the parent device, if any. */ - 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 = dev_fwnode(gc->parent); + device_set_node(&gdev->dev, parent_fwnode); + gc->fwnode = parent_fwnode; + } gdev->id = ida_alloc(&gpio_ida, GFP_KERNEL); if (gdev->id < 0) {