Received: by 2002:ac0:a5b6:0:0:0:0:0 with SMTP id m51-v6csp216666imm; Mon, 4 Jun 2018 16:10:41 -0700 (PDT) X-Google-Smtp-Source: ADUXVKJn3LyEOTTRm2VQhi9H3BNbxVNYJdtUYgiX6MK9TLeAUs/tc56MPx4UqtcyR3j8qBND4KHh X-Received: by 2002:a63:8f0d:: with SMTP id n13-v6mr19185512pgd.109.1528153841104; Mon, 04 Jun 2018 16:10:41 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1528153841; cv=none; d=google.com; s=arc-20160816; b=o+RqxCQ8fy4F77+NH9SAotMW+AEiKimlL5TZLddXGwJzn65VzTzACe0EbRent4mGXH ZpVYYbSIk5gtmPG/2qYn4o3xYPInjSFSEdqKNMFg163hWaaY7uavaULBY7Vds+dAKojP sTRdkRd+ee9Gzu2PG9VLRASc/HYJ8zg4NtPKetwbE/N19tsIukK7WtOO2sKJjdNjz/y4 EaA0knGF5RKhQ5uFnRIypv1b/7kS1XTQO+HpwfFrKgR9jGNwO3MHx6A7bvPJ67zUheYs MB5FfRCCEv37kcw5AIadCbGWCTjcoaHzqs16Wi9Q1yeaoHNr7yiBbdFy5Jhan48oP/u5 Wk9Q== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:content-transfer-encoding:mime-version :references:in-reply-to:message-id:subject:cc:to:from:date :arc-authentication-results; bh=3SAM4p1naFA3AODUohL+wCcKJvHQostz4VFUdXPDvOw=; b=suSIMGqza4F0/YZ2uIVo92p1QqzUjnFhdpSMT5quy8+dVC/l2BKv90CfCYSAeGxmR1 Hx9MzU0aAjRfVMDE6e9WUlufpERpTwHeiMAGvUvEFqZpt1Yoqov54AZMMmxyyHYWf7v5 xRW9FWno6YZF/TEDt2NjSrVxzPBhfbdOhDUNZHqjG+8CHWeiP0ebhbFvdQRngad+xKiH ksg3j1KWnyWu2GuD1fgaXshFoWtoXJ/DhrmprYRqClVY1b1E478SPmyE7KDnoWKroXev QhrfLYIv0n82HwAHo/PVrsa1h14gJlYjyCXS2Mk+6lJMKbcm+j8pu+/Gcj3IV3UsmZRP iQJQ== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id w190-v6si20924901pgd.5.2018.06.04.16.10.27; Mon, 04 Jun 2018 16:10:41 -0700 (PDT) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752085AbeFDXJq (ORCPT + 99 others); Mon, 4 Jun 2018 19:09:46 -0400 Received: from mail.bootlin.com ([62.4.15.54]:33420 "EHLO mail.bootlin.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751409AbeFDXJp (ORCPT ); Mon, 4 Jun 2018 19:09:45 -0400 Received: by mail.bootlin.com (Postfix, from userid 110) id 4C851203D9; Tue, 5 Jun 2018 01:09:43 +0200 (CEST) X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on mail.bootlin.com X-Spam-Level: X-Spam-Status: No, score=-1.0 required=5.0 tests=ALL_TRUSTED,SHORTCIRCUIT shortcircuit=ham autolearn=disabled version=3.4.0 Received: from bbrezillon (91-160-177-164.subs.proxad.net [91.160.177.164]) by mail.bootlin.com (Postfix) with ESMTPSA id E144620376; Tue, 5 Jun 2018 01:09:32 +0200 (CEST) Date: Tue, 5 Jun 2018 01:09:32 +0200 From: Boris Brezillon To: Janusz Krzysztofik Cc: Richard Weinberger , H Hartley Sweeten , Tony Lindgren , linux-kernel@vger.kernel.org, Marek Vasut , Krzysztof Halasa , Shreeya Patel , Arvind Yadav , Brian Norris , David Woodhouse , linux-mtd@lists.infradead.org Subject: Re: [PATCH 5/6 v2] mtd: rawnand: ams-delta: use GPIO lookup table Message-ID: <20180605010932.5c97b1de@bbrezillon> In-Reply-To: <1716431.jBl9UCSIdH@z50> References: <1582841.uoaVdad1fL@z50> <20180604115543.3be75717@bbrezillon> <1716431.jBl9UCSIdH@z50> X-Mailer: Claws Mail 3.15.0-dirty (GTK+ 2.24.31; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, 04 Jun 2018 18:48:08 +0200 Janusz Krzysztofik wrote: > On Monday, June 4, 2018 11:55:43 AM CEST Boris Brezillon wrote: > > On Wed, 30 May 2018 22:39:03 +0200 > > > > Janusz Krzysztofik wrote: > > > On Wednesday, May 30, 2018 7:52:20 PM CEST Boris Brezillon wrote: > > > > On Wed, 30 May 2018 19:43:09 +0200 > > > > > > > > Janusz Krzysztofik wrote: > > > > > On Wednesday, May 30, 2018 11:05:00 AM CEST Boris Brezillon wrote: > > > > > > Hi Janusz, > > > > > > > > > > Hi Boris, > > > > > > > > > > > On Sat, 26 May 2018 00:20:45 +0200 > > > > > > > > > > > > Janusz Krzysztofik wrote: > > > > > > > ... > > > > > > > Changes since v1: > > > > > > > - fix handling of devm_gpiod_get_optional() return values - thanks > > > > > > > to > > > > > > > > > > > > > > Andy Shevchenko. > > > > > > > > > > > > Can you put the changelog after the "---" separator so that it does > > > > > > not > > > > > > appear in the final commit message? > > > > > > > > > > Yes, sure, sorry for that. > > > > > > > > > > > > +err_gpiod: > > > > > > > + if (err == -ENODEV || err == -ENOENT) > > > > > > > + err = -EPROBE_DEFER; > > > > > > > > > > > > Hm, isn't it better to make gpiod_find() return > > > > > > ERR_PTR(-EPROBE_DEFER) > > > > > > here [1]? At least, ENOENT should not be turned into EPROBE_DEFER, > > > > > > because it's returned when there's no entry matching the requested > > > > > > gpio > > > > > > in the lookup table, and deferring the probe won't solve this > > > > > > problem. > > > > > > > > > > ENOENT is also returned when no matching lookup table is found. That > > > > > may > > > > > happen if consumer dev_name stored in the table differs from dev_name > > > > > assigned to the consumer by its bus, the platform bus in this case. > > > > > For > > > > > that reason I think the consumer dev_name should be initialized in the > > > > > table after the device is registered, when its actual dev_name can be > > > > > obtained. If that device registration happens after the driver is > > > > > already > > > > > registered, e.g., at late_initcall, the device is probed before its > > > > > lookup table is ready. For that reason returning EPROBE_DEFER seems > > > > > better to me even in the ENOENT case. > > > > > > > > Sorry, I don't get it. Aren't GPIO lookup tables supposed to be declared > > > > in board files, especially if the GPIO is used by a platform device? > > > > When would you have a lookup table registered later in the init/boot > > > > process? > > > > > > When e.g. I'd like to register my GPIO consumer platform device at > > > late_initcall for some reason, and I'm not sure what exact dev_name my > > > consomer will be registered with by the platform bus. > > > > You should know the name before the device is registered. > > What if I use PLATFORM_DEVID_AUTO? In this case, the id assigned to the device will be dependent on the device registration order, which in case of C board files should be predictable. Am I missing something? > > For other cases, if bus specific names of devices were supposed to be known > before registration, bus drivers should export functions returning those names > from initialized bus specific device structures or their components while they > don't. Under such circumstances, we end up hardcoding device names based on > our knowledge of bus internals if we need to specify them in advance, and > those internals are not guaranteed to never change. But, when it comes to C board files, device names are known ahead of time, right? And the only use case I see for those gpio_lookup_table is non-DT platforms. I had a quick look at a few call-sites of gpiod_add_lookup_table() and couldn't find an example where the gpio lookup table was registered after the platform device it was meant to be used by. If you have such an example, can you point it to me? > > > > In that case I think I > > > should assign dev_name to the lookup table after the consumer device is > > > registered and its exact dev_name can be obtained, then register the > > > table, > > > > I'm pretty sure it's not supposed to work like that. Resources attached > > to a device should be defined before the device is registered, not > > after, > > What do you mean by resources attached to a device? I don't think we should > consider GPIO lookup tables as consumer device resources. Why? If the device requires a GPIO to be controlled by the SoC, why shouldn't we consider the GPIO as a resource. Sure, it's not part of the struct resource array attached to the platform_device, but it's still a resource that needs to be available for the driver to correctly probe the device. > Those tables are > registered separately from consumer device registration and I know of no > requirement for registering them in advance. Maybe I'm missing something. But that doesn't make sense. Why would you register a device, and only then attach it the description of the GPIOs it needs. Think about the DT case, and imagine you support DT overlays, it's like having an overlay that defines your device, and then another overlay that adds the xx-gpios props to this device on top. > > Let's have a look at regulators. There are no separately registered regulator > lookup tables, instead regulator consumer supply tables are attached to bus > specific device structures of regulator devices, not their consumers, hence > registered together with providers, not consumers. Will you still call those > tables 'resources attached to' consumers? Except the gpio-lookup tables are not here to describe GPIOs, but to give consumer-oriented friendly names so that the consumer can then do devm_gpiod_get(&pdev->dev, "foo", GPIOD_OUT_LOW); Here "foo" is only meaningful to pdev, and not globally exposed, and another driver might pretty well request the exact same GPIO using a different name. > > As far as I can see, regulator_get() never returns -EINVAL, only -ENODEV or - > EPROBE_DEFER. However, gpiod_get() can also return -EINVAL. Maybe it > shouldn't, but it does, and I'm just trying to adopt to that in order to not > break a driver I'm trying to update. Well, IMO it should return EINVAL if no entry in the registered lookup tables is matching the requested "dev_name(dev) + con_id" pair. I'm bringing back my analogy with the DT case. What you're suggesting would be similar to waiting for a new xxx-gpios prop to magically appear in the DT. Note that the GPIO itself is not necessarily ready to be used when the consumer calls devm_gpio_get(), because the GPIO chip might not be probed yet or it might be missing a dependency. And that's all fine, we have EPROBE_DEFER for that. But that's different than saying "we don't have a GPIO description attached to the device, let's wait for someone to create one". > > > simply because when you call platform_device_register(), the > > device might be directly bind to the driver before the > > platform_device_register() calls return, and the driver will fail to > > probe the device if it doesn't find the GPIO it needs. > > That's exactly the case I'm talking about, but my conclusion is different: the > driver should fail softly so the device is probed again later, as long as I'm > not wrong the no requirement exists for registering GPIO lookup tables before > related consumers are registered. How long. Why should we wait only after late init calls? What if the lookup table is created after that? > > If I' missing something or you are still not convinced, I'll try to resolve > issues with the device I see in a different way and submit a new patch that > hopefully matches your requirements. Maybe I'm wrong, but I still think that registering a lookup table after the pdev it will be used by is a bad idea. Regards, Boris