From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755445AbcKXHNS (ORCPT ); Thu, 24 Nov 2016 02:13:18 -0500 Received: from hqemgate15.nvidia.com ([216.228.121.64]:10466 "EHLO hqemgate15.nvidia.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751097AbcKXHNQ (ORCPT ); Thu, 24 Nov 2016 02:13:16 -0500 X-PGP-Universal: processed; by hqnvupgp07.nvidia.com on Wed, 23 Nov 2016 11:09:20 -0800 Message-ID: <58368E84.6040104@nvidia.com> Date: Thu, 24 Nov 2016 12:23:56 +0530 From: Laxman Dewangan User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.4.0 MIME-Version: 1.0 To: Thierry Reding , Linus Walleij , Stephen Warren , "Suresh Mangipudi" CC: Alexandre Courbot , , , Subject: Re: [PATCH] gpio: Add Tegra186 support References: <20161122173042.GA3239@ulmo.ba.sec> <20161122175539.3897-1-thierry.reding@gmail.com> In-Reply-To: <20161122175539.3897-1-thierry.reding@gmail.com> X-Originating-IP: [10.19.65.30] X-ClientProxiedBy: DRBGMAIL101.nvidia.com (10.18.16.20) To bgmail102.nvidia.com (10.25.59.11) Content-Type: text/plain; charset="ISO-8859-1"; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tuesday 22 November 2016 11:25 PM, Thierry Reding wrote: > +static inline struct tegra_gpio *to_tegra_gpio(struct gpio_chip *chip) > +{ > + return container_of(chip, struct tegra_gpio, gpio); > +} You dont need this as gpiochip_get_data(chip); can provide the required driver specific data. > + > +static const struct tegra_gpio_port * > +tegra186_gpio_get_port(struct tegra_gpio *gpio, unsigned int *pin) > +{ > + unsigned int start = 0, i; > + > + for (i = 0; i < gpio->soc->num_ports; i++) { > + const struct tegra_gpio_port *port = &gpio->soc->ports[i]; > + > + if (*pin >= start && *pin < start + port->pins) { > + *pin -= start; > + return port; > + } > + > + start += port->pins; > + } > + Why not get the port from pins and then calculate with fixed offset. We will not need the loop if we know the port number. > > + > +static int tegra186_gpio_direction_output(struct gpio_chip *chip, > + unsigned int offset, int level) > +{ > + struct tegra_gpio *gpio = to_tegra_gpio(chip); > + void __iomem *base; > + u32 value; > + > + /* configure output level first */ > + chip->set(chip, offset, level); We can directly call the apis instead of function pointer at this point. > > + > +static struct lock_class_key tegra186_gpio_lock_class; We will have two instance of the driver (normal and AON) and so this will be shared between them. Do we really support multiple instance with same variable? > + > + gpio->gpio.label = "tegra186-gpio"; Two instance will have same name. Better to say tegra186-gpio and tegra186-gpio-aon. > + gpio->gpio.parent = &pdev->dev; > + > + gpio->gpio.get_direction = tegra186_gpio_get_direction; > + gpio->gpio.direction_input = tegra186_gpio_direction_input; > + gpio->gpio.direction_output = tegra186_gpio_direction_output; > + gpio->gpio.get = tegra186_gpio_get, > + gpio->gpio.set = tegra186_gpio_set; > + gpio->gpio.to_irq = tegra186_gpio_to_irq; > + > + gpio->gpio.base = -1; > + > + for (i = 0; i < gpio->soc->num_ports; i++) > + gpio->gpio.ngpio += gpio->soc->ports[i].pins; > + Our DT binding does not say this. We assume that we have 8 gpios per port. so this will not work at all. > > + > +static const struct tegra_gpio_port tegra186_main_ports[] = { > + [TEGRA_MAIN_GPIO_PORT_A] = { 0x2000, 7 }, Use C99 style initialization which is like .offset = .pins = >