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 Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 3B48BC7EE23 for ; Fri, 12 May 2023 17:08:15 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S237968AbjELRIO (ORCPT ); Fri, 12 May 2023 13:08:14 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:55762 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S236982AbjELRIL (ORCPT ); Fri, 12 May 2023 13:08:11 -0400 Received: from fgw23-7.mail.saunalahti.fi (fgw23-7.mail.saunalahti.fi [62.142.5.84]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 84462DDA7 for ; Fri, 12 May 2023 10:08:03 -0700 (PDT) Received: from localhost (88-113-26-95.elisa-laajakaista.fi [88.113.26.95]) by fgw23.mail.saunalahti.fi (Halon) with ESMTP id 8bd276d0-f0e7-11ed-b972-005056bdfda7; Fri, 12 May 2023 20:08:00 +0300 (EEST) From: andy.shevchenko@gmail.com Date: Fri, 12 May 2023 20:07:59 +0300 To: Esteban Blanc Cc: linus.walleij@linaro.org, lgirdwood@gmail.com, broonie@kernel.org, a.zummo@towertech.it, alexandre.belloni@bootlin.com, linux-kernel@vger.kernel.org, linux-gpio@vger.kernel.org, linux-rtc@vger.kernel.org, jpanis@baylibre.com, jneanne@baylibre.com, aseketeli@baylibre.com, sterzik@ti.com, u-kumar1@ti.com Subject: Re: [PATCH v4 2/3] pinctrl: tps6594: Add driver for TPS6594 pinctrl and GPIOs Message-ID: References: <20230512141755.1712358-1-eblanc@baylibre.com> <20230512141755.1712358-3-eblanc@baylibre.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20230512141755.1712358-3-eblanc@baylibre.com> Precedence: bulk List-ID: X-Mailing-List: linux-gpio@vger.kernel.org Fri, May 12, 2023 at 04:17:54PM +0200, Esteban Blanc kirjoitti: > TI TPS6594 PMIC has 11 GPIOs which can be used > for different functions. > > This patch adds a pinctrl and GPIO drivers in > order to use those functions. ... > +config PINCTRL_THUNDERBAY Is it correct name? To me sounds not. The problem is that you use platform name for the non-platform-wide pin control, i.e. for PMIC exclusively. Did I miss anything? > + tristate "Generic pinctrl and GPIO driver for Intel Thunder Bay SoC" > + depends on ARCH_THUNDERBAY || (ARM64 && COMPILE_TEST) This doesn't look correct, but I remember some Kconfig options that are using this way of dependency. > + depends on HAS_IOMEM > + select PINMUX > + select PINCONF > + select GENERIC_PINCONF > + select GENERIC_PINCTRL_GROUPS > + select GENERIC_PINMUX_FUNCTIONS > + select GPIOLIB > + select GPIOLIB_IRQCHIP > + select GPIO_GENERIC > + help > + This selects pin control driver for the Intel Thunder Bay SoC. > + It provides pin config functions such as pull-up, pull-down, > + interrupt, drive strength, sec lock, Schmitt trigger, slew > + rate control and direction control. This module will be > + called as pinctrl-thunderbay. Ah, the above simply a mistake. right? Why is it in this patch? > +config PINCTRL_TPS6594 > + tristate "Pinctrl and GPIO driver for TI TPS6594 PMIC" > + depends on MFD_TPS6594 > + default MFD_TPS6594 > + select PINMUX > + select GPIOLIB > + select REGMAP > + select GPIO_REGMAP > + help > + This driver supports GPIOs and pinmuxing for the TPS6594 > + PMICs chip family. Module name? ... > +obj-$(CONFIG_PINCTRL_THUNDERBAY) += pinctrl-thunderbay.o Huh?! > +obj-$(CONFIG_PINCTRL_TPS6594) += pinctrl-tps6594.o ... > +#include > +#include > +#include > +#include > +#include Ordered? ... > +static const char *groups_name[TPS6594_PINCTRL_PINS_NB] = { > + "GPIO0", "GPIO1", "GPIO2", "GPIO3", "GPIO4", "GPIO5", > + "GPIO6", "GPIO7", "GPIO8", "GPIO9", "GPIO10" Leave trailing comma even for known size. > +}; ... > +struct tps6594_pinctrl_function { > + const char *name; > + u8 muxval; > + const char **groups; > + unsigned long ngroups; We have struct pinfunction. Use it here (as embedded). > +}; ... > +static const struct tps6594_pinctrl_function pinctrl_functions[] = { > + { "gpio", TPS6594_PINCTRL_GPIO_FUNCTION, groups_name, > + TPS6594_PINCTRL_PINS_NB }, Here and further use PINCTRL_PINFUNCTION() macro. > +}; ... > +static int tps6594_group_pins(struct pinctrl_dev *pctldev, > + unsigned int selector, const unsigned int **pins, > + unsigned int *num_pins) > +{ > + struct tps6594_pinctrl *pinctrl = pinctrl_dev_get_drvdata(pctldev); > + > + *pins = (unsigned int *)&pinctrl->pins[selector]; Why casting? > + *num_pins = 1; > + > + return 0; > +} ... > + pinctrl->pctl_dev = > + devm_pinctrl_register(&pdev->dev, pctrl_desc, pinctrl); One line? > + if (IS_ERR(pinctrl->pctl_dev)) { > + dev_err(&pdev->dev, "Couldn't register pinctrl driver\n"); > + return PTR_ERR(pinctrl->pctl_dev); return dev_err_probe(...); > + } ... > + pinctrl->gpio_regmap = devm_gpio_regmap_register(&pdev->dev, &config); > + if (IS_ERR(pinctrl->gpio_regmap)) { > + dev_err(&pdev->dev, "Couldn't register gpio_regmap driver\n"); > + return PTR_ERR(pinctrl->pctl_dev); Ditto. > + } > + > + return 0; > +} ... > -#define TPS6594_REG_GPIOX_CONF(gpio_inst) (0x31 + (gpio_inst)) > +#define TPS6594_REG_GPIO1_CONF 0x31 > +#define TPS6594_REG_GPIOX_CONF(gpio_inst) (TPS6594_REG_GPIO1_CONF + (gpio_inst)) Why? The original code with parameter 0 will issue the same. -- With Best Regards, Andy Shevchenko