Received: by 2002:a05:6a10:8395:0:0:0:0 with SMTP id n21csp252839pxh; Tue, 9 Nov 2021 10:08:57 -0800 (PST) X-Google-Smtp-Source: ABdhPJzNbHyTF6VmpjJCyW+SCqPWL+O+Kjzvgps5LBYlvTSVgKAR18PqffqpoU1AWaBnqkW0Xkru X-Received: by 2002:aa7:d4d4:: with SMTP id t20mr12960965edr.374.1636481337057; Tue, 09 Nov 2021 10:08:57 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1636481337; cv=none; d=google.com; s=arc-20160816; b=D3v17cdJnrDZt9y81Ru2x92vgsoLBYXcUla6/Re7wKDsMXYywCDK50TURxeVNQo0XC k+f4i0Eyc+Qof9sFZ6DBbHaa5MqH//ae3sg+BBJWS4KscnJp/60o8i9jDgb5VLxuIiCt gsFUhrWL1cMk+T6HqztLYHr1hLOeFWUsuhBCcVm5seFQHTfPcgeSmxFLNRWakv0kqGPi bD5IyH2/vbpKV1sx0+BHa54n/zXINjqvHG8yAGv4e8QvKb3L5qtknG+iXgB/ZnrV9Bmk SsunDymHnZS2zI+qFUtNSzvPYki+fDhw/kVAJdkLvnCABCe3jaCa8OrXf7uVMhZS3TiI VHVw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:cc:to:subject:message-id:date:from:in-reply-to :references:mime-version; bh=vCxzbvDJEmmS0S1dn6IErGyixaEwc//p9ymy/wdvxtw=; b=XtItPnstwnjYh8vT9RrF80s/yLWTOahhMUkuIeNr3k29JvB23VHXCUUDpjE0+C26YB 7KtmyefZuQIrKg0JO9MJOaS7xHbv2bOBCqUKBoHatXD/yUwYeLl324fOTFNq+RJ4fN1y 3717gING5/2nuorEeWL2Tj6ouK3ltLlfEy+DjfMIE3TPgEXYqAwuYbBXaI1irYj7QBTE Pw1x9DfY5fG32KxeZcoxnodbHFkg6qYFbG0BCfj4Daza69JdCGAxDu2Gzd0gfg1PSynp 1TqQ/9ZzJZtytv0Wj/NiGCmdA7XaNEWEhnNaCACbr5EMgB0Z7HB+258LS7Z+BMDwADDp VH8g== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [23.128.96.18]) by mx.google.com with ESMTP id g22si33482904edf.94.2021.11.09.10.08.32; Tue, 09 Nov 2021 10:08:57 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S244807AbhKIJYV (ORCPT + 99 others); Tue, 9 Nov 2021 04:24:21 -0500 Received: from mail-pl1-f179.google.com ([209.85.214.179]:37465 "EHLO mail-pl1-f179.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S244806AbhKIJYT (ORCPT ); Tue, 9 Nov 2021 04:24:19 -0500 Received: by mail-pl1-f179.google.com with SMTP id n8so19825754plf.4; Tue, 09 Nov 2021 01:21:33 -0800 (PST) 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=vCxzbvDJEmmS0S1dn6IErGyixaEwc//p9ymy/wdvxtw=; b=jRTqA9CjNbMIaAbN2vA83XYv+AFhETkDyDbVjnWvwsHOo2s86fzQ1SraY29Dozzz1U +OeKQjVl2IAeHxhsyd80SK7Qmb0yr7mHowW1KwwnbjCg6t0hyqrIdzZ6aEKUSjuxCe1b fAPfSqIkrPglwsJEE1JFvdbV7Z1AgDXDuNgZapsMHDiqS9QhdYCf0aCWhen8Fkr5Ni2L rhKbEVzM74q3F6nH482PO/m6u322gY57tMyEyucMF6+liCuLnNxoHma0h31R3/O543+r 68IPjwx1vLRBYjfK/Be6/TNSq9C9UwuMUZxLlGKfRwfnwtNc3xJ2U2a5cFbQ1J4yeY0z 1tTA== X-Gm-Message-State: AOAM531tzjR+rIZRVBb943hg/e06zflSN37cQ1lc4883icJFOnpfrBIi OjXtBj8lkygYkYaWnEGM3VGCwY/ddOT7pUV7CC4= X-Received: by 2002:a17:903:11c5:b0:13f:ef40:e319 with SMTP id q5-20020a17090311c500b0013fef40e319mr5828558plh.33.1636449693539; Tue, 09 Nov 2021 01:21:33 -0800 (PST) MIME-Version: 1.0 References: <20211102161125.1144023-1-kernel@esmil.dk> <20211102161125.1144023-13-kernel@esmil.dk> In-Reply-To: From: Emil Renner Berthing Date: Tue, 9 Nov 2021 10:21:22 +0100 Message-ID: Subject: Re: [PATCH v3 12/16] pinctrl: starfive: Add pinctrl driver for StarFive SoCs To: Linus Walleij Cc: Andy Shevchenko , linux-riscv , devicetree , linux-clk , "open list:GPIO SUBSYSTEM" , "open list:SERIAL DRIVERS" , Palmer Dabbelt , Paul Walmsley , Rob Herring , Michael Turquette , Stephen Boyd , Thomas Gleixner , Marc Zyngier , Philipp Zabel , Greg Kroah-Hartman , Daniel Lezcano , Andy Shevchenko , Jiri Slaby , Maximilian Luz , Sagar Kadam , Drew Fustini , Geert Uytterhoeven , Michael Zhu , Fu Wei , Anup Patel , Atish Patra , Matteo Croce , Linux Kernel Mailing List , Huan Feng Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, 9 Nov 2021 at 02:01, Linus Walleij wrote: > > On Tue, Nov 2, 2021 at 9:08 PM Andy Shevchenko > wrote: > (...) > > > On Tue, Nov 2, 2021 at 6:50 PM Emil Renner Berthing wrote: > > > > ... > > > > > > +static int starfive_pinconf_group_set(struct pinctrl_dev *pctldev, > > > > + unsigned int gsel, > > > > + unsigned long *configs, > > > > + unsigned int num_configs) > > > > +{ > > > > + struct starfive_pinctrl *sfp = pinctrl_dev_get_drvdata(pctldev); > > > > + const struct group_desc *group; > > > > + u16 mask, value; > > > > + int i; > > > > + > > > > + group = pinctrl_generic_get_group(pctldev, gsel); > > > > + if (!group) > > > > + return -EINVAL; > > > > + > > > > + mask = 0; > > > > + value = 0; > > > > + for (i = 0; i < num_configs; i++) { > > > > + int param = pinconf_to_config_param(configs[i]); > > > > + u32 arg = pinconf_to_config_argument(configs[i]); > > > > + > > > > + switch (param) { > > > > + case PIN_CONFIG_BIAS_DISABLE: > > > > + mask |= PAD_BIAS_MASK; > > > > + value = (value & ~PAD_BIAS_MASK) | PAD_BIAS_DISABLE; > > > > + break; > > > > + case PIN_CONFIG_BIAS_PULL_DOWN: > > > > + if (arg == 0) > > > > + return -ENOTSUPP; > > > > + mask |= PAD_BIAS_MASK; > > > > + value = (value & ~PAD_BIAS_MASK) | PAD_BIAS_PULL_DOWN; > > > > + break; > > > > + case PIN_CONFIG_BIAS_PULL_UP: > > > > + if (arg == 0) > > > > + return -ENOTSUPP; > > > > + mask |= PAD_BIAS_MASK; > > > > + value = value & ~PAD_BIAS_MASK; > > > > + break; > > > > + case PIN_CONFIG_DRIVE_STRENGTH: > > > > + mask |= PAD_DRIVE_STRENGTH_MASK; > > > > + value = (value & ~PAD_DRIVE_STRENGTH_MASK) | > > > > + starfive_drive_strength_from_max_mA(arg); > > > > + break; > > > > + case PIN_CONFIG_INPUT_ENABLE: > > > > + mask |= PAD_INPUT_ENABLE; > > > > + if (arg) > > > > + value |= PAD_INPUT_ENABLE; > > > > + else > > > > + value &= ~PAD_INPUT_ENABLE; > > > > + break; > > > > + case PIN_CONFIG_INPUT_SCHMITT_ENABLE: > > > > + mask |= PAD_INPUT_SCHMITT_ENABLE; > > > > + if (arg) > > > > + value |= PAD_INPUT_SCHMITT_ENABLE; > > > > + else > > > > + value &= ~PAD_INPUT_SCHMITT_ENABLE; > > > > + break; > > > > + case PIN_CONFIG_SLEW_RATE: > > > > + mask |= PAD_SLEW_RATE_MASK; > > > > + value = (value & ~PAD_SLEW_RATE_MASK) | > > > > + ((arg << PAD_SLEW_RATE_POS) & PAD_SLEW_RATE_MASK); > > > > + break; > > > > + case PIN_CONFIG_STARFIVE_STRONG_PULL_UP: > > > > + if (arg) { > > > > + mask |= PAD_BIAS_MASK; > > > > + value = (value & ~PAD_BIAS_MASK) | > > > > + PAD_BIAS_STRONG_PULL_UP; > > > > + } else { > > > > + mask |= PAD_BIAS_STRONG_PULL_UP; > > > > + value = value & ~PAD_BIAS_STRONG_PULL_UP; > > > > + } > > > > + break; > > > > + default: > > > > + return -ENOTSUPP; > > > > + } > > > > + } > > > > + > > > > + for (i = 0; i < group->num_pins; i++) > > > > + starfive_padctl_rmw(sfp, group->pins[i], mask, value); > > > > + > > > > + return 0; > > > > +} > > > > Linus any comments on this code (sorry if I missed your reply)? The > > idea behind above is to skip all settings from the same category and > > apply only the last one, e.g. if we have "bias set to X", ..., "bias > > disable", ..., "bias set to Y", the hardware will see only the last > > operation, i.e. "bias set to Y". I think it may not be the best > > approach (theoretically?) since the hardware definitely may behave > > differently on the other side in case of such series of the > > configurations (yes, I have seen some interesting implementations of > > the touchpad / touchscreen GPIOs that may be affected). > > That sounds weird. I think we need to look at how other drivers > deal with this. > > To me it seems more natural that > starfive_padctl_rmw(sfp, group->pins[i], mask, value); > would get called at the end of each iteration of the > for (i = 0; i < num_configs; i++) loop. That would work, but when the loop is done the end result would be exactly the same. The only difference is that the above would rapidly "blink" the different states during the loop until it arrives at the result. This would certainly be different, but it can never be the intended behaviour and only a side-effect on how the pinctrl framework works. The order the different states are blinked depends entirely on how the pinctrl framework parses the device tree. I still think it would be more natural to cleanly go to the end result without this blinking. /Emil