From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id AAD3CC433EF for ; Wed, 1 Dec 2021 13:54:31 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231828AbhLAN5r (ORCPT ); Wed, 1 Dec 2021 08:57:47 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:37056 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S243993AbhLAN5Q (ORCPT ); Wed, 1 Dec 2021 08:57:16 -0500 Received: from mail-ed1-x52b.google.com (mail-ed1-x52b.google.com [IPv6:2a00:1450:4864:20::52b]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 7CF5CC061759 for ; Wed, 1 Dec 2021 05:53:54 -0800 (PST) Received: by mail-ed1-x52b.google.com with SMTP id y12so102048998eda.12 for ; Wed, 01 Dec 2021 05:53:54 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bgdev-pl.20210112.gappssmtp.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=4deMsVbOnJ3WN8CLL6wclTeFqPAZPUEWcJx/Fop1Tyo=; b=1l3lnEtuA8NrfBhwXmTUJrdk7Oemc/pyml50HxaN70UALtlVNqS/FBDnUVC7ONlL/U 0mkYW5ZoouJf3JnG/o77BM4sfzCkQLxpP1Fi8occwNuQ64wWZeounUscVrblhDB2cJkS OFqk323TvUF4e4TQ7bV5Fd6U76FH2Vh4WytR9Os5ZKcPC0HC7aHuKRMsSg7rukyfHluk fBH/c7u7YnxTFx/oL4sHzXNjIcNftnTaAKpFZLVtGaahBiderqY+hbB4owgEMwzoIpUd XInsHlVVxSgyGQ5qE1QNrFRlZLtycXhguSdZNUopMhFBYnY2Gw7Ge3aQaia4wGVlnb1l b7Jw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=4deMsVbOnJ3WN8CLL6wclTeFqPAZPUEWcJx/Fop1Tyo=; b=8I3Pdu1c+FKP17/zPdtGW/dFMbzsU98SmFZ0rrqcxpFgMqI1OndmGUUKHQ70vinvu5 YtOGe1uZhs5vVvPA+GrAZqe/UXpxsfUQXCtzu+GRJq2/vDIq04nWXUsO0pF5J896zeEd V8P3l6oqi2+Rh/z0sPwNV6G7Ulpb3bdxofEjmd+T3i3EDB7wXmLn7aWZYkhK8RXrPfP0 jEGio0asm+J2B7jTe9nTDvzdN35tSl5kBjMZ8fAjiyDT+G9xHfShqRh6ejntuwOMnUa1 WepC6SePi7mnGf/hY6icMpTTRvr+q85JP2ybt8w7BlUdYUyYbkGp2faZlVE6chCPNa9y dHpQ== X-Gm-Message-State: AOAM531s8P1BvazVT74MFK5W+Qn7f5Y76w/43qezrDtS8TUWIX2Xz+jR u3HcMk1eJ0v/PLVHwU7gCBcFilTso7DJq+yY7W7BO1koxnRAYQ== X-Google-Smtp-Source: ABdhPJyqZibQyod4rRaPKH46bY4KCTTr61Gqj21JVMYOSoBhV1RUk41ThnrGC0919q5EKf6+TNKmvWK5ti4obl53xsk= X-Received: by 2002:a05:6402:2744:: with SMTP id z4mr8861558edd.310.1638366832980; Wed, 01 Dec 2021 05:53:52 -0800 (PST) MIME-Version: 1.0 References: <20211130154127.12272-1-brgl@bgdev.pl> <20211130154127.12272-3-brgl@bgdev.pl> In-Reply-To: From: Bartosz Golaszewski Date: Wed, 1 Dec 2021 14:53:42 +0100 Message-ID: Subject: Re: [PATCH v11 2/6] gpiolib: allow to specify the firmware node in struct gpio_chip To: Andy Shevchenko Cc: Kent Gibson , Linus Walleij , Shuah Khan , Geert Uytterhoeven , "open list:GPIO SUBSYSTEM" , Linux Kernel Mailing List , linux-kselftest@vger.kernel.org Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-gpio@vger.kernel.org On Wed, Dec 1, 2021 at 2:40 PM Andy Shevchenko wrote: > > On Wed, Dec 01, 2021 at 02:11:28PM +0100, Bartosz Golaszewski wrote: > > On Tue, Nov 30, 2021 at 10:04 PM Bartosz Golaszewski wrote: > > > > > > On Tue, Nov 30, 2021 at 10:00 PM Andy Shevchenko > > > wrote: > > > > > > > > On Tue, Nov 30, 2021 at 09:25:35PM +0100, Bartosz Golaszewski wrote: > > > > > On Tue, Nov 30, 2021 at 5:15 PM Andy Shevchenko > > > > > wrote: > > > > > > > > > > > > On Tue, Nov 30, 2021 at 04:41:23PM +0100, Bartosz Golaszewski wrote: > > > > > > > Software nodes allow us to represent hierarchies for device components > > > > > > > that don't have their struct device representation yet - for instance: > > > > > > > banks of GPIOs under a common GPIO expander. The core gpiolib core > > > > > > > > > > > > core .. core ?! > > > > > > > > > > > > > however doesn't offer any way of passing this information from the > > > > > > > drivers. > > > > > > > > > > > > > > This extends struct gpio_chip with a pointer to fwnode that can be set > > > > > > > by the driver and used to pass device properties for child nodes. > > > > > > > > > > > > > > This is similar to how we handle device-tree sub-nodes with > > > > > > > CONFIG_OF_GPIO enabled. > > > > > > > > > > > > Not sure I understand the proposal. Can you provide couple of (simplest) > > > > > > examples? > > > > > > > > > > > > And also it sounds like reinventing a wheel. What problem do you have that you > > > > > > need to solve this way? > > > > > > > > > > > > ... > > > > > > > > > > > > > +#if IS_ENABLED(CONFIG_OF_GPIO) > > > > > > > + if (gc->of_node && gc->fwnode) { > > > > > > > + pr_err("%s: tried to set both the of_node and fwnode in gpio_chip\n", > > > > > > > + __func__); > > > > > > > + return -EINVAL; > > > > > > > + } > > > > > > > +#endif /* CONFIG_OF_GPIO */ > > > > > > > > > > > > I don't like this. It seems like a hack right now. > > > > > > > > > > > > Is it possible to convert all GPIO controller drivers to provide an fwnode > > > > > > rather than doing this? (I believe in most of the drivers we can drop > > > > > > completely the of_node assignment). > > > > > > > > > > > > > > > > Yes, it's definitely a good idea but I would be careful with just > > > > > dropping the of_node assignments as callbacks may depend on them > > > > > later. > > > > > > > > GPIO library does it for us among these lines: > > > > > > > > struct fwnode_handle *fwnode = gc->parent ? dev_fwnode(gc->parent) : NULL; > > > > > > > > of_gpio_dev_init(gc, gdev); <<< HERE! > > > > acpi_gpio_dev_init(gc, gdev); > > > > > > > > /* > > > > * Assign fwnode depending on the result of the previous calls, > > > > * if none of them succeed, assign it to the parent's one. > > > > */ > > > > gdev->dev.fwnode = dev_fwnode(&gdev->dev) ?: fwnode; > > > > > > > > > > Except that it doesn't and I noticed that when working on the > > > subsequent patch. The child gpiochipX devices all had the parent's > > > fwnode assigned as their primary fwnode and no secondary fwnode. > > > > > > Note that this driver doesn't use neither OF nor ACPI in which case > > > gdev->dev has no fwnode and the parent's one is used. This patch > > > addresses it. If you have a better idea, let me know. > > > > > > Bart > > > > Let me maybe rephrase the problem: currently, for GPIO devices > > instantiating multiple banks created outside of the OF or ACPI > > frameworks (e.g. instantiated manually and configured using a > > hierarchy of software nodes with a single parent swnode and a number > > of child swnodes representing the children), it is impossible to > > assign firmware nodes other than the one representing the top GPIO > > device to the gpiochip child devices. > > > > In fact if we want to drop the OF APIs entirely from gpiolib - this > > would be the right first step as for gpio-sim it actually replaces the > > gc->of_node = some_of_node; assignment that OF-based drivers do for > > sub-nodes defining banks and it does work with device-tree (I verified > > that too) thanks to the fwnode abstraction layer. > > I still don't see how you set up hierarchy of primary/secondary fwnodes. > > And I don't like this change. It seems it band-aids some issue with fwnode > usage. What the easiest way to reproduce the issue with your series applied > (without this change)? > Drop this patch and drop the line where the fwnode is assigned in gpio-sim.c. Then probe the device and print the addresses of the parent and child swnodes. See how they are the same and don't match the swnode hierarchy we created. You can then apply this patch and see how it becomes correct. Bart