Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754580AbcDNJmS (ORCPT ); Thu, 14 Apr 2016 05:42:18 -0400 Received: from mail-oi0-f51.google.com ([209.85.218.51]:36261 "EHLO mail-oi0-f51.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754289AbcDNJmO (ORCPT ); Thu, 14 Apr 2016 05:42:14 -0400 MIME-Version: 1.0 In-Reply-To: References: Date: Thu, 14 Apr 2016 11:42:13 +0200 Message-ID: Subject: Re: [PATCH RESEND 1/2] pinctrl: ns2: add pinmux driver support for Broadcom NS2 SoC From: Linus Walleij To: Yendapally Reddy Dhananjaya Reddy Cc: Rob Herring , Pawel Moll , Mark Rutland , Ian Campbell , Kumar Gala , Florian Fainelli , Ray Jui , Scott Branden , Catalin Marinas , Will Deacon , Pramod Kumar , "devicetree@vger.kernel.org" , "linux-kernel@vger.kernel.org" , bcm-kernel-feedback-list , "linux-arm-kernel@lists.infradead.org" , "linux-gpio@vger.kernel.org" , Paul Gortmaker 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: 3781 Lines: 112 On Thu, Apr 14, 2016 at 9:53 AM, Yendapally Reddy Dhananjaya Reddy wrote: > On Wed, Apr 13, 2016 at 6:49 PM, Linus Walleij wrote: >> On Tue, Mar 29, 2016 at 5:22 PM, Yendapally Reddy Dhananjaya Reddy >> wrote: >>> +static const unsigned int gpio_0_1_pins[] = {24, 25}; >>> +static const unsigned int pwm_0_pins[] = {24}; >>> +static const unsigned int pwm_1_pins[] = {25}; >> >> So either the same pins are used for GPIO or PWM. >> And this pattern persists. >> >> Do you have a brewing GPIO driver for this block as well? >> Is it a separate front-end calling to pinctrl using the >> pinctrl_gpio_* calls or do you plan to patch it into this >> driver? >> >> This is more of a question. >> > > This SoC supports group based configuration and there is a top level register > to select groups. Once the gpio_0_1_pins group is selected, there is one more > register to select between gpio_0_1 and pwm (only four pins). The pins > 24 and 25 are shared between nor pins and gpio at the top group level. Once > gpio group is selected, then we can select to be either gpio or pwm. I missed > these two pins to be added to nor_data_pins and will add in the next version. > > static const unsigned nor_data_pins[] = {0, 1, 2, 3, 4, 5, 6, 7, 8, 9, > 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25}; > > NS2_PIN_GROUP(nand, 0, 0, 31, 1, 0), > NS2_PIN_GROUP(nor_data, 0, 0, 31, 1, 1), > NS2_PIN_GROUP(gpio_0_1, 0, 0, 31, 1, 0), > > To select PWM, we need to select gpio and pwm as well. > > gpio: gpio { > function = "gpio"; > groups = "gpio_0_1_grp"; > > pwm: pwm { > function = "pwm"; > groups = "pwm0_grp", "pwm1_grp"; > }; > > Already available gpio driver "pinctrl-iproc-gpio.c" will be the gpio driver > for this soc as well without pin request. Then you are doing something wrong. Look in pinctrl-iproc-gpio.c: /* * Request the Iproc IOMUX pinmux controller to mux individual pins to GPIO */ static int iproc_gpio_request(struct gpio_chip *gc, unsigned offset) { struct iproc_gpio *chip = gpiochip_get_data(gc); unsigned gpio = gc->base + offset; /* not all Iproc GPIO pins can be muxed individually */ if (!chip->pinmux_is_supported) return 0; return pinctrl_request_gpio(gpio); } static void iproc_gpio_free(struct gpio_chip *gc, unsigned offset) { struct iproc_gpio *chip = gpiochip_get_data(gc); unsigned gpio = gc->base + offset; if (!chip->pinmux_is_supported) return; pinctrl_free_gpio(gpio); } So as you see pinctrl_request_gpio() and pinctrl_free_gpio() are being called. These will in turn call pinmux_request_gpio() and pinmux_free_gpio() to make the backing pin controller mux in the pin as GPIO. pinmux_request_gpio() will end up in pin_request() and at this point: if (gpio_range && ops->gpio_request_enable) /* This requests and enables a single GPIO pin */ status = ops->gpio_request_enable(pctldev, gpio_range, pin); As you can see: it will attempt to call the .gpio_request_enable() method of your struct pinmux_ops. But your pinmux ops look like this: +static struct pinmux_ops ns2_pinmux_ops = { + .get_functions_count = ns2_get_functions_count, + .get_function_name = ns2_get_function_name, + .get_function_groups = ns2_get_function_groups, + .set_mux = ns2_pinmux_enable, +}; I.e. there is no way that GPIO can be set up as a GPIO line, and you're relying on some other pin control entries in the device tree to do that, which is unnecessarily complicated. Please consider implementing the .gpio_request_enable callback for this pin multiplexer. Yours, Linus Walleij