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=-11.4 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH, MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=unavailable 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 63D27C43461 for ; Sun, 13 Sep 2020 19:29:48 +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 04E39208C7 for ; Sun, 13 Sep 2020 19:29:47 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="kcw1mG0L"; dkim=fail reason="signature verification failed" (2048-bit key) header.d=microchip.com header.i=@microchip.com header.b="ll3f1/JV" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 04E39208C7 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=eXoGbz3bKJCJVtvh0XuXu46we5qUhJ1gX1r4h9O1CWU=; b=kcw1mG0LUjVfyigNj1RXLWQLt tKdRzDw3nAJwsF8u2dXBacSJ2UmVBHBZF+B0M/gJdonci/5k1ISLv81T+Nz54hV3facXcJkMwTJrx 1PNRfQlkrQnEYJTSqzQhMmFof4ufZFz9uwSvrose0SYBGvw2fyaL3lv75z65ApUEyef9hyVuPvvRV Pp3o6JvDFW3BhkS4Rf/4eZ6T6DAnrjx+Q8A0+t9Jo/6ykriFVQPJDFyw3Yix4HKfrcHxKFBmcn/Az IRnxX2/9chsVlVQfSKXBM6SchJyxBx0WgKyHocuWKs49Zh/H/B4CFf5BQ7XWhy9MdjfSqGRw/hJ2D P9+8W6Kjg==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1kHXfa-0004vv-HX; Sun, 13 Sep 2020 19:28:14 +0000 Received: from esa4.microchip.iphmx.com ([68.232.154.123]) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1kHXfY-0004vB-GM for linux-arm-kernel@lists.infradead.org; Sun, 13 Sep 2020 19:28:13 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=microchip.com; i=@microchip.com; q=dns/txt; s=mchp; t=1600025292; x=1631561292; h=references:from:to:cc:subject:in-reply-to:date: message-id:mime-version; bh=KfSuhuBWdbKfZmkY1vicW1hLi4TcaPsoMTVGkC57pPs=; b=ll3f1/JVuj9T4jUMioaV2NnKzcE1lSedwktfsd2gFPENprTUtbRjlFEC 7SF7rDM5arS+XmHlpkW6ddWAYvhXenW/GNps7uHbUU2nzRNo+IVGKxhyN qVDPlt8LYdD7v/0uJgE0lxxYdQjmQvkceWrXs4sgnGAB1hwZh0YilADaJ cN7DMZGqIS4UBZPQXCqzuhHigfxenhBxvwHH0ltbMDNdShyyAVfgX9GCE vvqhCCH+zxRPXSY2xV3ztPjessTWR4sLe6DTgqF8awXRRpxIAVl/Du65y Hy1M5lW8bP+L62FtUI1WjEa2h48XuuhrwirscgB8f18nVnR9QgKQG+MO/ w==; IronPort-SDR: L82gV9CfNkoIkGKRwnqtrKBDp7LdIqqC/CrEwCU+ixN+HcX7AAxQWpFR61EAETH9NANuK2IYjS OeiKzxOc9vA1Qid8peOriEYe1B7kyFDSMRcvrlSYTolv7SrpXgryg4wHiQNBuH6zlvKKt9KjyC /UFlO27ChtoyHjBPRwyTsg4uEfAnQc1U4NcaWXv2W0SNxAZxb8WUk1Ba2EjxyATz9ludGMcgZG UQNeDTjg6Shfinn6pPDGNUzJXBB4FKpM6GxpgTG0bhkyYNSTlrGC74GYXYGPLWUCFnJyPCKAcG j4U= X-IronPort-AV: E=Sophos;i="5.76,423,1592895600"; d="scan'208";a="86711584" Received: from smtpout.microchip.com (HELO email.microchip.com) ([198.175.253.82]) by esa4.microchip.iphmx.com with ESMTP/TLS/AES256-SHA256; 13 Sep 2020 12:28:09 -0700 Received: from chn-vm-ex03.mchp-main.com (10.10.85.151) by chn-vm-ex02.mchp-main.com (10.10.85.144) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.1979.3; Sun, 13 Sep 2020 12:28:08 -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; Sun, 13 Sep 2020 12:28:02 -0700 References: <20200903133528.8595-1-lars.povlsen@microchip.com> <20200903133528.8595-3-lars.povlsen@microchip.com> From: Lars Povlsen To: Linus Walleij Subject: Re: [PATCH v2 2/3] pinctrl: pinctrl-mchp-sgpio: Add pinctrl driver for Microsemi Serial GPIO In-Reply-To: Date: Sun, 13 Sep 2020 21:28:05 +0200 Message-ID: <87pn6pwk6y.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-20200913_152812_672803_A1E8DAE9 X-CRM114-Status: GOOD ( 32.63 ) 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: Kate Stewart , "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: > On Thu, Sep 3, 2020 at 3:35 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 > (...) > >> diff --git a/drivers/pinctrl/pinctrl-mchp-sgpio.c b/drivers/pinctrl/pinctrl-mchp-sgpio.c > > Can we just spell it out > pinctrl-microchip-sgpio.c ? > > The abbreviation seems arbitrary and unnecessary. Well, not completely arbitrary, but maybe unnecessary... I'll change that. I'll also change that for any symbols/defines off course. > > I do see that this chip is using the pinctrl abstractions (very nicely) > and should be under drivers/pinctrl/*. > > Sadly it doesn't mean the bindings need to be in pinctrl ... unless you > plan to add pinctrl bindings later. > >> +config PINCTRL_MCHP_SGPIO >> + bool "Pinctrl driver for Microsemi/Microchip Serial GPIO" >> + depends on OF >> + depends on HAS_IOMEM >> + select GPIOLIB >> + select GENERIC_PINCONF >> + select GENERIC_PINCTRL_GROUPS >> + select GENERIC_PINMUX_FUNCTIONS > > Nice use of these abstractions! Thanks! > >> +// SPDX-License-Identifier: (GPL-2.0 OR MIT) > > What's up with this OR MIT? I'd like Kate to OK this. > >> +#define MCHP_SGPIOS_PER_BANK 32 >> +#define MCHP_SGPIO_BANK_DEPTH 4 >> + >> +#define PIN_NAM_SZ (sizeof("SGPIO_D_pXXbY")+1) >> + >> +enum { >> + REG_INPUT_DATA, >> + REG_PORT_CONFIG, >> + REG_PORT_ENABLE, >> + REG_SIO_CONFIG, >> + REG_SIO_CLOCK, >> + MAXREG >> +}; >> + >> +struct mchp_sgpio_props { > > Just call it struct microchip_gpio_variant it is easier to read and > does not abbreviate randomly, also it is a per-variant set of properties > so calling it variant is more to the point. > Fine. >> +struct mchp_sgpio_priv { > > I would just spell it out struct microchip_sgpio, it is implicit that > the struct is private since it is defined in a c file. > Fine. >> +struct mchp_sgpio_port_addr { > > struct microchip_sgpio_port_addr > > (Admittedly this is a bit about taste.) > >> +static inline void sgpio_writel(struct mchp_sgpio_priv *priv, >> + u32 val, u32 rno, u32 off) >> +{ >> + u32 __iomem *reg = &priv->regs[priv->props->regoff[rno] + off]; >> + >> + writel(val, reg); >> +} >> + >> +static inline void sgpio_clrsetbits(struct mchp_sgpio_priv *priv, >> + u32 rno, u32 off, u32 clear, u32 set) >> +{ >> + u32 __iomem *reg = &priv->regs[priv->props->regoff[rno] + off]; >> + u32 val = readl(reg); >> + >> + val &= ~clear; >> + val |= set; >> + >> + writel(val, reg); >> +} > > This looks like a reimplementation of regmap_update_bits for a bit, > have you considered just using regmap? It's pretty simple. > Well, the registers are not in a regmap, so I did not consider that. The utility function also serves to abstract the variant register address. >> +static int mchp_sgpio_direction_input(struct gpio_chip *gc, unsigned int gpio) >> +{ >> + struct mchp_sgpio_priv *priv = gpiochip_get_data(gc); >> + >> + /* Fixed-position function */ >> + return sgpio_is_input(priv, gpio) ? 0 : -EINVAL; >> +} >> + >> +static int mchp_sgpio_direction_output(struct gpio_chip *gc, >> + unsigned int gpio, int value) >> +{ >> + struct mchp_sgpio_priv *priv = gpiochip_get_data(gc); >> + struct mchp_sgpio_port_addr addr; >> + >> + sgpio_pin_to_addr(priv, gpio, &addr); >> + >> + /* Fixed-position function */ >> + if (addr.input) >> + return -EINVAL; >> + >> + sgpio_output_set(priv, &addr, value); >> + >> + return 0; >> +} > > This looks like the right way to handle this! I'm glad you think so... > >> +static int mchp_sgpio_of_xlate(struct gpio_chip *gc, >> + const struct of_phandle_args *gpiospec, >> + u32 *flags) >> +{ >> + struct mchp_sgpio_priv *priv = gpiochip_get_data(gc); >> + int pin, base; >> + >> + if (gpiospec->args[0] > MCHP_SGPIOS_PER_BANK || >> + gpiospec->args[1] > priv->bitcount) >> + return -EINVAL; >> + base = priv->bitcount * gpiospec->args[0]; >> + pin = base + gpiospec->args[1]; >> + /* Add to 2nd half of output range if output */ >> + if (gpiospec->args[2] == PIN_OUTPUT) >> + pin += (priv->ngpios / 2); >> + >> + if (pin > gc->ngpio) >> + return -EINVAL; >> + >> + if (flags) >> + *flags = gpiospec->args[3]; >> + >> + return pin; >> +} > > I don't like this. I would certainly prefer the driver to just use standard > GPIO bindings. I do not understand why this is necessary. > > If for nothing else, there should be a big comment explaining this. > > The only real problem I have with the driver is this extra flag tagged onto > all the GPIOs, this seems unnecessary, and something the hardware > driver should already know from the compatible string. I hope my previous comments have cleared this up. > > Yours, > Linus Walleij Thank you for your time and comments! ---Lars -- Lars Povlsen, Microchip _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel