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 X-Spam-Level: X-Spam-Status: No, score=-8.3 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY, SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 5CDC0CA9EAF for ; Mon, 21 Oct 2019 23:15:05 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 0FA162086D for ; Mon, 21 Oct 2019 23:15:05 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="OmXYtLVH" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1730399AbfJUXPE (ORCPT ); Mon, 21 Oct 2019 19:15:04 -0400 Received: from mail-pf1-f194.google.com ([209.85.210.194]:35503 "EHLO mail-pf1-f194.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1730069AbfJUXPE (ORCPT ); Mon, 21 Oct 2019 19:15:04 -0400 Received: by mail-pf1-f194.google.com with SMTP id 205so9386425pfw.2 for ; Mon, 21 Oct 2019 16:15:04 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:content-transfer-encoding:in-reply-to :user-agent; bh=lP4fCISmX7zqlrNgoaVi59fb0dh+amjr7KKzwNExP2s=; b=OmXYtLVHvJr1SRDojHKHLgnluhgfNYFxFV7JHwqL/Hlhyg9xMBQywTX+o7sI/pgkXP 7ObJ9+wbQTHsdTu3zATzKTyYvx/P5sK1yUoFZTCl5BRYh9oqpgne2JXeItNIGz8lhztz lNciOOXUl4BOcorddicFxPOa347VBfKFIG3UtQdEKuysd2eSrdXzOrRx1Gs2elJKjeVa Pmqh8yNEdCRBRpuMZ8QCbNS2O2Bo6tHoUQvxgPBh/1z0z+VedOYEhctq9b90LsETrr8b s968pVMckX7Fbi5swYFv990rrmIZEDH1E43Yk46TlIjsLwyEffq952FzAXRuONW/zEB6 kjvA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:content-transfer-encoding :in-reply-to:user-agent; bh=lP4fCISmX7zqlrNgoaVi59fb0dh+amjr7KKzwNExP2s=; b=aSupzYaj9WL6gVOXag04vkpk1I97+Y5aHGA+IWbIbPI42fcpxZ+K6YIQHILpjWgz2M tBoOHwXZ4wN1PoA7MzL6mBtuHv1pRh5VYhzai3rfBEHMCHW93J211+xs4jj9zF1Hq/a2 ttKgOd03uox10gLjeTD/tnmdgpTMJ+yeCj0ucU2zKCkh9ZhUQI+/kwr/10blbp3K61iL fu2SbeSqWWdIDRcL3B9K/rNRiB866Yy8310QefHE/Hvm4VaWg/ZbQx0wc7PVWQgCt3uF YJrCMHZW9+Ezz3zxkuNX5T8o/rEQ2eBh+PYnsX8OH6KgkLHyHAJgzy3qxmAB+id4vaW7 8XvQ== X-Gm-Message-State: APjAAAWaJAWgpeDBTU0MbytshoLS/LkI9yyqcqcX6tadAWlCbGc2WyLT CzAIKc8hJiTFFMCtoakkLRE4mwDeHkPstA== X-Google-Smtp-Source: APXvYqz0egbFASiBcvJ+zt8dATLiaT4yRs5OL3dy9iEXaB2kgA5FLSUPO7eb1zd6G6HvBrohaueB3w== X-Received: by 2002:a63:1403:: with SMTP id u3mr368820pgl.85.1571699703048; Mon, 21 Oct 2019 16:15:03 -0700 (PDT) Received: from sol (220-235-109-115.dyn.iinet.net.au. [220.235.109.115]) by smtp.gmail.com with ESMTPSA id y15sm24014132pfp.111.2019.10.21.16.14.59 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Mon, 21 Oct 2019 16:15:01 -0700 (PDT) Date: Tue, 22 Oct 2019 07:14:56 +0800 From: Kent Gibson To: Bartosz Golaszewski Cc: "open list:GPIO SUBSYSTEM" , Linus Walleij , Bamvor Jian Zhang , Drew Fustini Subject: Re: [PATCH v2 5/6] gpiolib: disable bias on inputs when pull up/down are both set Message-ID: <20191021231456.GA3374@sol> References: <20191014130425.GC28012@sol> <20191015005849.GA7970@sol> <20191016010104.GA8083@sol> <20191017050647.GA21551@sol> <20191018101306.GB16347@sol> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: User-Agent: Mutt/1.9.4 (2018-02-28) Sender: linux-gpio-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-gpio@vger.kernel.org On Mon, Oct 21, 2019 at 04:57:01PM +0200, Bartosz Golaszewski wrote: > pt., 18 paź 2019 o 12:13 Kent Gibson napisał(a): > > > > On Fri, Oct 18, 2019 at 10:03:17AM +0200, Bartosz Golaszewski wrote: > > > czw., 17 paź 2019 o 07:06 Kent Gibson napisał(a): > > > > > > > > On Wed, Oct 16, 2019 at 09:01:04AM +0800, Kent Gibson wrote: > > > > > On Tue, Oct 15, 2019 at 02:51:18PM +0200, Bartosz Golaszewski wrote: > > > > > > wt., 15 paź 2019 o 02:58 Kent Gibson napisał(a): > > > > > > > > > > > > > > On Mon, Oct 14, 2019 at 06:50:41PM +0200, Bartosz Golaszewski wrote: > > > > > > > > pon., 14 paź 2019 o 15:04 Kent Gibson napisał(a): > > > > > > > > > > > > > > > > > > On Mon, Oct 14, 2019 at 02:43:54PM +0200, Bartosz Golaszewski wrote: > > > > > > > > > > sob., 12 paź 2019 o 03:57 Kent Gibson napisał(a): > > > > > > > > > > > > > > > > > > > > > > This patch allows pull up/down bias to be disabled, allowing > > > > > > > > > > > the line to float or to be biased only by external circuitry. > > > > > > > > > > > Use case is for where the bias has been applied previously, > > > > > > > > > > > either by default or by the user, but that setting may > > > > > > > > > > > conflict with the current use of the line. > > > > > > > > > > > > > > > > > > > > > > Signed-off-by: Kent Gibson > > > > > > > > > > > --- > > > > > > > > > > > drivers/gpio/gpiolib.c | 22 +++++++--------------- > > > > > > > > > > > 1 file changed, 7 insertions(+), 15 deletions(-) > > > > > > > > > > > > > > > > > > > > > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c > > > > > > > > > > > index 647334f53622..f90b20d548b9 100644 > > > > > > > > > > > --- a/drivers/gpio/gpiolib.c > > > > > > > > > > > +++ b/drivers/gpio/gpiolib.c > > > > > > > > > > > @@ -539,11 +539,6 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip) > > > > > > > > > > > (lflags & GPIOHANDLE_REQUEST_OUTPUT)) > > > > > > > > > > > return -EINVAL; > > > > > > > > > > > > > > > > > > > > > > - /* Same with pull-up and pull-down. */ > > > > > > > > > > > - if ((lflags & GPIOHANDLE_REQUEST_PULL_UP) && > > > > > > > > > > > - (lflags & GPIOHANDLE_REQUEST_PULL_DOWN)) > > > > > > > > > > > - return -EINVAL; > > > > > > > > > > > - > > > > > > > > > > > /* > > > > > > > > > > > * Do not allow OPEN_SOURCE & OPEN_DRAIN flags in a single request. If > > > > > > > > > > > * the hardware actually supports enabling both at the same time the > > > > > > > > > > > @@ -935,14 +930,6 @@ static int lineevent_create(struct gpio_device *gdev, void __user *ip) > > > > > > > > > > > (lflags & GPIOHANDLE_REQUEST_PULL_DOWN))) > > > > > > > > > > > return -EINVAL; > > > > > > > > > > > > > > > > > > > > > > - /* > > > > > > > > > > > - * Do not allow both pull-up and pull-down flags to be set as they > > > > > > > > > > > - * are contradictory. > > > > > > > > > > > - */ > > > > > > > > > > > - if ((lflags & GPIOHANDLE_REQUEST_PULL_UP) && > > > > > > > > > > > - (lflags & GPIOHANDLE_REQUEST_PULL_DOWN)) > > > > > > > > > > > - return -EINVAL; > > > > > > > > > > > - > > > > > > > > > > > le = kzalloc(sizeof(*le), GFP_KERNEL); > > > > > > > > > > > if (!le) > > > > > > > > > > > return -ENOMEM; > > > > > > > > > > > @@ -2931,6 +2918,7 @@ static int gpio_set_config(struct gpio_chip *gc, unsigned offset, > > > > > > > > > > > unsigned arg; > > > > > > > > > > > > > > > > > > > > > > switch (mode) { > > > > > > > > > > > + case PIN_CONFIG_BIAS_DISABLE: > > > > > > > > > > > case PIN_CONFIG_BIAS_PULL_DOWN: > > > > > > > > > > > case PIN_CONFIG_BIAS_PULL_UP: > > > > > > > > > > > arg = 1; > > > > > > > > > > > @@ -2991,7 +2979,11 @@ int gpiod_direction_input(struct gpio_desc *desc) > > > > > > > > > > > if (ret == 0) > > > > > > > > > > > clear_bit(FLAG_IS_OUT, &desc->flags); > > > > > > > > > > > > > > > > > > > > > > - if (test_bit(FLAG_PULL_UP, &desc->flags)) > > > > > > > > > > > + if (test_bit(FLAG_PULL_UP, &desc->flags) && > > > > > > > > > > > + test_bit(FLAG_PULL_DOWN, &desc->flags)) > > > > > > > > > > > + gpio_set_config(chip, gpio_chip_hwgpio(desc), > > > > > > > > > > > + PIN_CONFIG_BIAS_DISABLE); > > > > > > > > > > > + else if (test_bit(FLAG_PULL_UP, &desc->flags)) > > > > > > > > > > > > > > > > > > > > From looking at the code: user-space can disable bias when setting > > > > > > > > > > both PULL_UP and PULL_DOWN flags. I don't understand why it's done in > > > > > > > > > > this implicit way? Why not a separate flag? > > > > > > > > > > > > > > > > > > An extra flag would waste a bit and add nothing but more sanity checking. > > > > > > > > > > > > > > > > > > > > > > > > > I disagree. The user API needs to be very explicit. Sanity checking is > > > > > > > > alright - if there'll be too many ifdefs, we can start thinking about > > > > > > > > adding some core library helpers for sanitizing conflicting flags, I'm > > > > > > > > sure other frameworks could use something like this as well. > > > > > > > > > > > > > > > > Especially in this context: setting PULL_UP and PULL_DOWN together > > > > > > > > disables bias - this doesn't make sense logically. > > > > > > > > > > > > > > > In a way it does make a weird kind of sense - they cancel. Physically. > > > > > > > > > > > > > > > > > > > Yes, on some devices we set both bits to disable bias, but on others > > > > > > the pull-up and pull-down bits need to be cleared and yet others have > > > > > > a dedicated bit for that. It's not standardized and the pinctrl > > > > > > framework defines all three values as separate bits to expose a common > > > > > > programming interface. > > > > > > > > > > > > > > Is there any documentation on this? The pinctrl docs stay pretty high > > > > level and doesn't touch on this. And from the pinconf-generic.h > > > > documentation, I'd consider drivers that require both pull-up and > > > > pull-down set to disable bias to be non-compliant with the API - for > > > > BIAS_DISABLE it says "this setting disables all biasing", so you'd think > > > > the driver would support that and do any mapping (setting both pulls > > > > high or low or whatever) internally. > > > > > > > > So no answer for this one? > > > > I find it unsettling that we will have a user space API that doesn't > > provide a definitive way to disable bias, independent of the underlying > > hardware. I thought the kernel was all about hardware abstraction? > > > > If you expose three options to user-space: PULL-UP, PULL-DOWN and > DISABLE and then that gets translated by the driver to whatever the > driver understands - how is this not abstraction? > Ok, I misunderstood what you were saying. > > > > > Ok. And, since gpiolib has no knowledge of what combinations are > > > > > appropriate for a given chip, we can't provide a higher level > > > > > abstraction and have no option but to expose that pinconf > > > > > complexity in the GPIO uapi? > > > > > > > > > > In fact, pinconf doesn't just define 3 bias bits - it defines 6: > > > > > > > > > > enum pin_config_param { > > > > > PIN_CONFIG_BIAS_BUS_HOLD, > > > > > PIN_CONFIG_BIAS_DISABLE, > > > > > PIN_CONFIG_BIAS_HIGH_IMPEDANCE, > > > > > PIN_CONFIG_BIAS_PULL_DOWN, > > > > > PIN_CONFIG_BIAS_PULL_PIN_DEFAULT, > > > > > PIN_CONFIG_BIAS_PULL_UP, > > > > > > > > > > Do we need to support any of the remaining 3 in the GPIO uapi, either > > > > > now or possibly in the future? > > > > > > > > > > And what about the other PIN_CONFIG flags? Some of these might be > > > > > candidates for controlling via SET_CONFIG_IOCTL, if not in the request > > > > > itself? (again this is contemplating the future, not suggesting being part > > > > > of this patch) > > > > > > > > > > > > Did you read the cover letter? The problem, as I see it, > > > > > > > is that we're stuck using a flag field to encode a two bit enum. > > > > > > > That fact the we only have a flag field to play with can't be > > > > > > > changed due to ABI. > > > > > > > > > > > > For some reason I haven't received the cover letter on my inbox. I'm > > > > > > only now seeing it on linux-gpio archives. > > > > > > > > > > > And for some reason I didn't get 0001, yet all 7 parts made it to the mailing > > > > > list. Spam filters kicking in? Though it isn't in my spam folder either. > > > > > Something odd going on. > > > > > > > > > > > Anyway: I don't understand why you insist on using two instead of > > > > > > three bits. You have 32 bits in total that can be used and only 5 are > > > > > > used so far. There's plenty left. > > > > > > > > > > > Cos it makes no sense to me to encode 4 values into 3 bits when 2 will > > > > > do. But if you want to expose part of the pinconf API within the GPIO > > > > > uapi then that goes out the window - it's not 4 values anymore. > > > > > > > > > > And partly cos I'm frustrated that I'd asked questions regarding how the > > > > > API should look earlier and got no reply. This is the sort of thing I > > > > > usually deal with in the design stage, not review. > > > > > > > > > > I realise you guys are busy, but a little time spent clarifying design > > > > > would save a whole lot more time in coding, testing and review. > > > > > > > > > > > I'd prefer to see: > > > > > > > > > > > > GPIOHANDLE_REQUEST_PULL_UP > > > > > > GPIOHANDLE_REQUEST_PULL_DOWN > > > > > > GPIOHANDLE_REQUEST_PULL_DISABLED > > > > > > > > > > > > or maybe even > > > > > > > > > > > > GPIOHANDLE_REQUEST_BIAS_PULL_UP > > > > > > GPIOHANDLE_REQUEST_BIAS_PULL_DOWN > > > > > > GPIOHANDLE_REQUEST_BIAS_DISABLED > > > > > > > > > > > > to stay consistent with the pinctrl flags. No bit set among these > > > > > > three would mean AS_IS. > > > > > > > > > > > That makes sense, if we are exposing the pinctrl API here. > > > > > > > > > > > > > Looking at going with the naming including BIAS... > > > > What to do with constants defined in headers prior to this patch that > > > > don't include the BIAS? e.g. FLAG_PULL_UP and FLAG_PULL_DOWN in gpiolib.h? > > > > > > But this has nothing to do with user-space. This was added so that > > > GPIO expanders can use this without pulling in the pinctrl framework. > > > > > > > Safe to assume they can't be renamed? > > > > > > What for? > > > > > > > For consistency and clarity. I need to add a flag into the gpio_desc > > flags, which are usedf throughout gpiolib.c and aredefined in gpiolib.h, > > and those FLAG_PULL_UP and FLAG_PULL_DOWN are already there. > > > > To be consistent I'd be dropping the GPIOHANDLE_REQUEST_BIAS_ from your > > preferred names when determining the name for the new flag, but then it > > would be called FLAG_DISABLE, which is obviously too vague. > > > > I intend to use FLAG_BIAS_DISABLE, and for consistency it would be nice > > to rename FLAG_PULL_UP to FLAG_BIAS_PULL_UP, and similarly PULL_DOWN, > > but that may break things and so be unacceptable, right? > > > > Yeah, not for device tree, no. It sounds fine for core gpiolib but I > wouldn't change it in this series though personally - let's try to > limit its scope and we can come back to it later. > Agreed, and coded that way in v3. > > > > So ok to stay with the unBIASed names for both old (cos they are there) > > > > and also the new (to be consistent with the old)? > > > > > > There's no need for perfect naming consistency between user and kernel > > > space declarations. The difference is: you need to be sure to get the > > > user-space flags right the first time - unlike the kernel APIs, they > > > cannot be renamed later. > > > > > > > So it may be acceptable to change the gpiolib.h flag names? > > > > As I said above - yes, but I'd wait until this series is at least in > Linus' or my for-next branch. > > > This is one of those cases where I'd rather ask than guess and not find > > out until the patch gets rejected. > > > > > > > > > > Also, while the DT interface (gpiod_configure_flags) has GPIO_PULL_UP > > > > and GPIO_PULL_DOWN, it doesn't support DISABLE, and it explicitly rejects > > > > both having both PULL_UP and PULL_DOWN set. Should we be extending the > > > > DISABLE support to the DT interface, and should the API behaviour also > > > > mirror the pinctrl behaviour you describe above? > > > > > > Is someone needing it? Adding new features without users is frowned > > > upon for good reasons. > > > > > > > Not that I am aware of, so we can always add it later. > > And that is why I asked - my first instinct is to keep the APIs > > aligned, especially where we now have the user space API containing > > functionality not available to DT, but I am also aware that might be > > considered unimportant or even unacceptable. > > > > In general: we're adding new interfaces when someone needs them, not > "just in case" someone needs them in the future. > Ok. That is that way I went with v3 - leaving the DT as it was. > > > > > > > > And are there any combinations that are guaranteed to be invalid, > > > > and so should be rejected, like DISABLE + PULL_UP?? In fact are there > > > > any combinations that are valid other then PULL_UP + PULL_DOWN? > > > > > > You mean invalid? You just said PULL_UP + PULL_DOWN is rejected. > > > > > > > No, I my example was DISABLE + PULL_UP, which I assume is an invalid combo. > > And then on reflection the only valid combo I could think of was > > PULL_UP + PULL_DOWN, and even then only because you said we need it to > > disable bias on some chips. > > I think there's some misunderstanding. I said that on some chips there > are bits in control registers that are used to control the bias and > sometimes it's three (pull-up, pull-down and disable) and sometimes > it's two (pull-up, pull-down and both set = disabled) but that we > should only have a single interface in user-space where the > BIAS_DISABLE bit would be translated to whatever the hardware > understands by the driver. So in this case: only one of the bits could > be set. > Ok - I misinterpreted what you were saying. That answers one of the outstanding issues I mention in the v3 cover letter. For v4 I will add sanity checks that only one bit bias is set. > I understand that this sounds like a task for an enum, but introducing > an enum in a separate field would modify the ABI while simply adding > new mutually-exclusive flags would not. Not to mention that encoding > this enum on two bits of a field that's normally used for flags and is > even called "flags" would introduce a lot of confusion. > Considering the vast majority of users will use a library, not the uAPI directly, I doubt it would introduce that much confusion. Though if we want to expose any of the other modes at a later date the flags do work better. And its coded as separate flags now, so what the hell. > I got your new patches - I'll take a look at them shortly. > I've also got the corresponding updates for libgpiod in a branch on github now, though not structured as a patch series yet. I'll tidy them up and submit them once this series is in. The updated C and CXX tests pass. The one problem I have is getting the Python tests running - they can't import gpiomockup, though I've built and installed everything. Not sure what I'm missing there. Cheers, Kent.