Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752365AbbEFSpK (ORCPT ); Wed, 6 May 2015 14:45:10 -0400 Received: from mail-qk0-f181.google.com ([209.85.220.181]:35905 "EHLO mail-qk0-f181.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751330AbbEFSpE (ORCPT ); Wed, 6 May 2015 14:45:04 -0400 MIME-Version: 1.0 In-Reply-To: References: <1430269982-24129-1-git-send-email-abrestic@chromium.org> <1430269982-24129-3-git-send-email-abrestic@chromium.org> Date: Wed, 6 May 2015 11:45:03 -0700 X-Google-Sender-Auth: RVKcLUbajUp5Vw_fCMdavAEqv6k Message-ID: Subject: Re: [PATCH V4 2/2] pinctrl: Add Pistachio SoC pin control driver From: Andrew Bresticker To: Linus Walleij Cc: Ralf Baechle , "linux-gpio@vger.kernel.org" , "devicetree@vger.kernel.org" , Linux MIPS , "linux-kernel@vger.kernel.org" , Ezequiel Garcia , James Hartley , James Hogan , Damien Horsley , Govindraj Raja , Kevin Cernekee Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5092 Lines: 112 Hi Linus, On Wed, May 6, 2015 at 12:14 AM, Linus Walleij wrote: > Hi Andrew and sorry for a slow review process, I've been > overloaded :( > > On Wed, Apr 29, 2015 at 3:13 AM, Andrew Bresticker > wrote: > >> Add a driver for the pin controller present on the IMG Pistachio SoC. >> This driver provides pinmux and pinconfig operations as well as GPIO >> and IRQ chips for the GPIO banks. >> >> Signed-off-by: Damien Horsley >> Signed-off-by: Govindraj Raja >> Signed-off-by: Ezequiel Garcia >> Signed-off-by: Kevin Cernekee >> Signed-off-by: Andrew Bresticker >> --- >> Changes from v3: >> - Addressed review comments from Ezequiel. > > Overall this is a very very nice looking driver so expect it to > be merged after addressing or answering my last few > concerns. > >> +config PINCTRL_PISTACHIO >> + def_bool y if MACH_PISTACHIO >> + select PINMUX >> + select GENERIC_PINCONF >> + select GPIOLIB_IRQCHIP > > I think you also need > depends on / select GPIOLIB > select OF_GPIO > > x86_64 allmodconfig is usually the best way to test > if your GPIOs and pin control fragments are correctly Kconfig:ed. Right, will fix. >> +#define GPIO_BANK(_bank, _pin_base, _npins) \ >> + { \ >> + .gpio_chip = { \ >> + .label = "GPIO" #_bank, \ >> + .request = pistachio_gpio_request, \ >> + .free = pistachio_gpio_free, \ >> + .get_direction = pistachio_gpio_get_direction, \ >> + .direction_input = pistachio_gpio_direction_input, \ >> + .direction_output = pistachio_gpio_direction_output, \ >> + .get = pistachio_gpio_get, \ >> + .set = pistachio_gpio_set, \ >> + .base = _pin_base, \ >> + .ngpio = _npins, \ >> + }, \ >> + .irq_chip = { \ >> + .name = "GPIO" #_bank, \ >> + .irq_startup = pistachio_gpio_irq_startup, \ >> + .irq_ack = pistachio_gpio_irq_ack, \ >> + .irq_mask = pistachio_gpio_irq_mask, \ >> + .irq_unmask = pistachio_gpio_irq_unmask, \ >> + .irq_set_type = pistachio_gpio_irq_set_type, \ >> + }, \ >> + .gpio_range = { \ >> + .name = "GPIO" #_bank, \ >> + .id = _bank, \ >> + .base = _pin_base, \ >> + .pin_base = _pin_base, \ >> + .npins = _npins, \ >> + }, \ >> + } > > This -gpio_range is the only thing that bothers me a little, combined with > this: > >> + bank->gpio_range.gc = &bank->gpio_chip; >> + pinctrl_add_gpio_range(pctl->pctldev, &bank->gpio_range); > > Because it adds the ranges from the pinctrl side instead of > doing it from the gpiochip side using > gpiochip_add_pin_range() or gpiochip_add_pingroup_range(). > > Have you tried using those (from instead? > > They have the upside that .base is taken from the gpio_chip > meaning it is unnecessary to define .base for the gpiochip > too and you can just go for what gpiolib dynamically assigns > for you. Yup, switching to gpiochip_add_pin_range() works just fine here. >> +static int __init pistachio_pinctrl_register(void) >> +{ >> + return platform_driver_register(&pistachio_pinctrl_driver); >> +} >> +arch_initcall(pistachio_pinctrl_register); > > Is it necessary to have it registered so early? No, nothing depends on it being initialized this early. When I was asked to remove the module stuff, I just picked arch_initcall since that's what most other pinctrl drivers not using module_platform_driver() use. It also has the benefit of reducing the number of probe deferrals. Thanks, Andrew -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/