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=-6.8 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=no 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 00653C43467 for ; Thu, 8 Oct 2020 10:59:27 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 6E932215A4 for ; Thu, 8 Oct 2020 10:59:26 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="Eggwc0kz"; dkim=fail reason="signature verification failed" (2048-bit key) header.d=microchip.com header.i=@microchip.com header.b="Kpcose/z" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 6E932215A4 Authentication-Results: mail.kernel.org; dmarc=fail (p=quarantine dis=none) header.from=microchip.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=merlin.20170209; h=Sender:Content-Transfer-Encoding: Content-Type:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:MIME-Version:Message-ID:Date:In-Reply-To:Subject:To: From:References:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=Q5+pf48ICu9EwFrVf3euXevkJzE7FSOocWnFlnlq36A=; b=Eggwc0kzu+apKWjmqx15pPp/0 M6DniApo8vbP6Mxc3fuG7TZmTCNjSOPoWNFVXexTPeFopsTaYmZQOLgjnCDfvPpDSytlHaapo8+hQ F+lAkP77tCwIdXgODpojx3+nkklVik7deYJTDxRpFknD1/F5uM2J23nYFG+H39QTsemFTcv2gb0R9 wg7oT6fVqN1Jsa4RTJxuF1B3z/9sBNGLw8vW38CF5UUZ1PTphLBO00OdfztjfdFvmusIda4oLJ6Jj s/X1Rp/SaWIEHt3N4zr2HixxV9AQttZeAvWibLsB17UcH/a3lcXiS/2Jq1rCID3vsT7gXsoQnGjlf frZlhAwjg==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1kQTcM-0006Ew-IY; Thu, 08 Oct 2020 10:57:50 +0000 Received: from esa6.microchip.iphmx.com ([216.71.154.253]) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1kQTcI-0006E1-Iu for linux-arm-kernel@lists.infradead.org; Thu, 08 Oct 2020 10:57:47 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=microchip.com; i=@microchip.com; q=dns/txt; s=mchp; t=1602154666; x=1633690666; h=references:from:to:cc:subject:in-reply-to:date: message-id:mime-version; bh=sbsYblhM7HBECp1Iw7gGZrJTSXmYJPEc0TVl2NmfuLg=; b=Kpcose/z8BJkVJ9phtpq3IxvDse6qF6kbWTKbZjqIt+9ZHUuFLX6Fe88 iZZQjIDxcp9Se1F2wcQ0dMZRm9fY871ZUNRgkZyX2iOMWd5+IDZIY7VX9 1NPADKoqWmNVMAxRRbP6H6OMjSETY+nRdINCNMQwTzf+0TtWvapyUs0xp Vf7jJ0lPHl81U8QG8NoFW/PyIO3W2gNgvN+C4Dm6v7s9yKC9fM7d3uVJw oymRmFs08BGIJuDgbm7e1UDfp7jPsW7zyN2khtW500oaZryW8V03nVq5y 6WCNHoFH16MxDX5jh6+nqdezmVEiT9Z4DG4qmEyKNYllMFM/ydGYl8LO1 Q==; IronPort-SDR: W1L25IG2YRzeJ5cgA1BK2uWtRzxQrvEmH+oYQPDzJnJ/1O1H5EFRAOenV5rsudJBysL0AIYeBE gS3mDqkS0LjTZ31CANbCIAiHVaeY3p2O+RLJB0wp8ab2heD3isdSnIzIIG/mFag1xll++laeIr hMeW2fiW2Y7CRltm0XgQYo/jM3whPNoT9KN3ukc+jPOL22CBvt2ZyRmAW5nGct9VBDH6ZD2MW1 lZ2CKm7r/6CPAoZ8TJcNNftGE4lS7JAh+PGtuWIaE8HdEhyd1hKkBpPfYhJ9xapry388n3bgVU /Bk= X-IronPort-AV: E=Sophos;i="5.77,350,1596524400"; d="scan'208";a="29188573" Received: from smtpout.microchip.com (HELO email.microchip.com) ([198.175.253.82]) by esa6.microchip.iphmx.com with ESMTP/TLS/AES256-SHA256; 08 Oct 2020 03:57:43 -0700 Received: from chn-vm-ex03.mchp-main.com (10.10.85.151) by chn-vm-ex01.mchp-main.com (10.10.85.143) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.1979.3; Thu, 8 Oct 2020 03:57:18 -0700 Received: from soft-dev15.microsemi.net.microchip.com (10.10.115.15) by chn-vm-ex03.mchp-main.com (10.10.85.151) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.1979.3 via Frontend Transport; Thu, 8 Oct 2020 03:57:41 -0700 References: <20201006142532.2247515-1-lars.povlsen@microchip.com> <20201006142532.2247515-3-lars.povlsen@microchip.com> From: Lars Povlsen To: Linus Walleij Subject: Re: [RESEND PATCH v3 2/3] pinctrl: pinctrl-mchp-sgpio: Add pinctrl driver for Microsemi Serial GPIO In-Reply-To: Date: Thu, 8 Oct 2020 12:57:39 +0200 Message-ID: <87ft6px9wc.fsf@soft-dev15.microsemi.net> MIME-Version: 1.0 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20201008_065746_749592_B2427A5D X-CRM114-Status: GOOD ( 34.42 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" , Alexandre Belloni , "linux-kernel@vger.kernel.org" , Microchip Linux Driver Support , "open list:GPIO SUBSYSTEM" , Lars Povlsen , Linux ARM Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org Linus Walleij writes: > Hi Lars! > > Thanks for the update, this looks much improved! Glad you like it! It's been a difficult birth... > > Some further hints at things I saw here: > > On Tue, Oct 6, 2020 at 4:25 PM Lars Povlsen wrote: > >> This adds a pinctrl driver for the Microsemi/Microchip Serial GPIO >> (SGPIO) device used in various SoC's. >> >> Signed-off-by: Lars Povlsen > >> + /* 2 banks: IN/OUT */ >> + struct { >> + struct gpio_chip gpio; >> + struct pinctrl_desc pctl_desc; >> + struct pinctrl_dev *pctl_dev; >> + } bank[2]; > > Is it really good to use index [0,1] to refer to these? > Isnt it easier to do something like: > > struct sgpio_bank { > struct gpio_chip gpio; > struct pinctrl_desc pctl_desc; > struct pinctrl_dev *pctl_dev; > }; > > struct sgpio_priv { > (...) > struct sgpio_bank in; > struct sgpio_bank out; > (...) > }; > > This way you I think the code becomes clearer. > I have done the change as suggested, and I think your right - looks better. Also helped loose the "is_input" helper functions. >> +static inline bool sgpio_pctl_is_input(struct sgpio_priv *priv, >> + struct pinctrl_dev *pctl) >> +{ >> + /* 1st index is input */ >> + return pctl == priv->bank[0].pctl_dev; >> +} >> + >> +static inline bool sgpio_gpio_is_input(struct sgpio_priv *priv, >> + struct gpio_chip *gc) >> +{ >> + /* 1st index is input */ >> + return gc == &priv->bank[0].gpio; >> +} > > Isn't it easier to just add a > > bool is_input; > > to the struct gpio_bank? Yes, did that. > >> +static inline u32 sgpio_readl(struct sgpio_priv *priv, u32 rno, u32 off) >> +{ >> + u32 __iomem *reg = &priv->regs[priv->properties->regoff[rno] + off]; >> + >> + return readl(reg); >> +} >> + >> +static inline void sgpio_writel(struct sgpio_priv *priv, >> + u32 val, u32 rno, u32 off) >> +{ >> + u32 __iomem *reg = &priv->regs[priv->properties->regoff[rno] + off]; >> + >> + writel(val, reg); >> +} >> + >> +static inline void sgpio_clrsetbits(struct sgpio_priv *priv, >> + u32 rno, u32 off, u32 clear, u32 set) >> +{ >> + u32 __iomem *reg = &priv->regs[priv->properties->regoff[rno] + off]; >> + u32 val = readl(reg); >> + >> + val &= ~clear; >> + val |= set; >> + >> + writel(val, reg); >> +} > > These accessors are somewhat re-implementing regmap-mmio, especially > the clrsetbits. I would consider just creating a regmap for each > struct sgpio_bank and manipulate the register through that. > (Not any requirement just a suggestion.) > Humm, not sure if these few functions warrants using regmap. I'll have a closer look. >> +static void sgpio_output_set(struct sgpio_priv *priv, >> + struct sgpio_port_addr *addr, >> + int value) >> +{ >> + u32 mask = 3 << (3 * addr->bit); >> + value = (value & 3) << (3 * addr->bit); > > I would spell it out a bit so it becomes clear what is going in > and use the bits helpers. > > value & 3 doesn't make much sense since value here is always > 0 or 1. Thus if value is 1 it in effect becomes value = 1 << (3 * addr->bit) > so with the above helper bit: > > unsigned int bit = 3 * addr->bit; > u32 mask = GENMASK(bit+1, bit); > u32 val = BIT(bit); > > This way it becomes clear that you set the output usin the lower > of the two bits. > > Also note that I use val rather than value to avoid overwriting > the parameter: it is legal but confusing. Let the compiler optimize > register use. > I will change as suggested. >> +static int sgpio_output_get(struct sgpio_priv *priv, >> + struct sgpio_port_addr *addr) >> +{ >> + u32 portval = sgpio_readl(priv, REG_PORT_CONFIG, addr->port); >> + int ret; >> + >> + ret = SGPIO_X_PORT_CFG_BIT_SOURCE(priv, portval); >> + ret = !!(ret & (3 << (3 * addr->bit))); > > Is the output direction really controlled by both bits? > > ret = !!(ret & (BIT(3 * addr->bit))); ? > The 3 bits are actually "mode" not direction. The direction is fixed as described earlier. 0: Forced 0 1: Forced 1 2: Blink mode 0 3: Blink mode 1 4: Link activity blink mode 0 5: Link activity blink mode 1 But you are still right the (forced) _value_ can still be read using just the first bit. I will change to using just the first bit. >> +static int microchip_sgpio_get_direction(struct gpio_chip *gc, unsigned int gpio) >> +{ >> + struct sgpio_priv *priv = gpiochip_get_data(gc); >> + >> + return sgpio_gpio_is_input(priv, gc); /* 0=out, 1=in */ > > You can use the #defines from if you want to be explicit: > > return sgpio_gpio_is_input(priv, gc) ? GPIO_LINE_DIRECTION_IN : > GPIO_LINE_DIRECTION_OUT; > Yes, good suggestion. Also using bank->is_input now. >> +static int microchip_sgpio_of_xlate(struct gpio_chip *gc, >> + const struct of_phandle_args *gpiospec, >> + u32 *flags) >> +{ >> + struct sgpio_priv *priv = gpiochip_get_data(gc); >> + int pin; >> + >> + if (gpiospec->args[0] > SGPIO_BITS_PER_WORD || >> + gpiospec->args[1] > priv->bitcount) >> + return -EINVAL; >> + >> + pin = gpiospec->args[1] + (gpiospec->args[0] * priv->bitcount); >> + >> + if (pin > gc->ngpio) >> + return -EINVAL; >> + >> + if (flags) >> + *flags = gpiospec->args[2]; >> + >> + return pin; >> +} > > I'm still not convinced that you need the flags in args[2]. > > Just using the default of_gpio_simple_xlate() with one flag > should be fine. But I will go and review the bindings to figure > this out. > Ok, I just assumed the std GPIO flags in args[2] were kind of mandatory, I mean polarity would be needed? F.ex. a GPIO (led) looks like this now: led@0 { label = "eth60:yellow"; gpios = <&sgpio_out1 28 1 GPIO_ACTIVE_LOW>; default-state = "off"; }; >> + gc->of_xlate = microchip_sgpio_of_xlate; >> + gc->of_gpio_n_cells = 3; > > So I'm sceptical to this. > > Why can't you just use the pin index in cell 0 directly > and avoid cell 1? > You scepticism has surfaced before :-). The (now) 2 indices relates to how the hardware address signals. Each signal/pin is addressed by port, bit number and direction. We now have the direction encoded in the bank/phandle. While it would be possible to just produce a linear range of (32 * width), hardware designs and documentation use pXXbY (as "p28b1" above), making the cross reference much better using the 2 indices when specifying a pin (as opposed to using f.ex. value "60" in the example). Hope this helps. Thank you for your comments, I will refresh later today. ---Lars > Yours, > Linus Walleij -- Lars Povlsen, Microchip _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel