Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751611AbdFILhb (ORCPT ); Fri, 9 Jun 2017 07:37:31 -0400 Received: from mail-io0-f181.google.com ([209.85.223.181]:35961 "EHLO mail-io0-f181.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751524AbdFILh2 (ORCPT ); Fri, 9 Jun 2017 07:37:28 -0400 MIME-Version: 1.0 In-Reply-To: <1496907001-27107-1-git-send-email-chenjh@rock-chips.com> References: <1496905959-29202-1-git-send-email-chenjh@rock-chips.com> <1496907001-27107-1-git-send-email-chenjh@rock-chips.com> From: Linus Walleij Date: Fri, 9 Jun 2017 13:37:26 +0200 Message-ID: Subject: Re: [PATCH v6 08/12] gpio: Add GPIO driver for the RK805 PMIC To: Jianhong Chen , =?UTF-8?Q?Heiko_St=C3=BCbner?= Cc: Alexandre Courbot , Dmitry Torokhov , "open list:ARM/Rockchip SoC..." , "linux-kernel@vger.kernel.org" , Tao Huang , tony.xie@rock-chips.com, zhangqing@rock-chips.com, "devicetree@vger.kernel.org" , w.egorov@phytec.de, Liam Girdwood , Mark Brown , Rob Herring , Mark Rutland , "linux-gpio@vger.kernel.org" , Linux Input 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: 2903 Lines: 95 Heiko, can you please look at this patch. On Thu, Jun 8, 2017 at 9:30 AM, Jianhong Chen wrote: > From: chenjh Full name please. > RK805 has two configurable GPIOs that can be used for several > purposes. These are output only. > > This driver is generic for other Rockchip PMICs to be added. > > Signed-off-by: chenjh Dito. Your commit message says they are output-only, yet you implement .direction_input(). So what is is going to be? > +#include > +#include Only use: #include > +/* > + * @mode: supported modes for this gpio, i.e. OUTPUT_MODE, OUTPUT_MODE... Are you saying this should be an enum or a set of flags? > +static int rk805_gpio_get(struct gpio_chip *chip, unsigned offset) > +{ > + int ret, val; > + struct rk805_gpio *gpio = gpiochip_get_data(chip); > + > + ret = regmap_read(gpio->rk808->regmap, gpio->pins[offset].reg, &val); > + if (ret) { > + dev_err(gpio->dev, "gpio%d not support output mode\n", offset); > + return ret; > + } > + > + return (val & gpio->pins[offset].val_msk) ? 1 : 0; Do this: return !!(val & gpio->pins[offset].val_msk) > +static int rk805_gpio_request(struct gpio_chip *chip, unsigned offset) > +{ > + int ret; > + struct rk805_gpio *gpio = gpiochip_get_data(chip); > + > + /* switch to gpio mode */ > + if (gpio->pins[offset].func_mask) { > + ret = regmap_update_bits(gpio->rk808->regmap, > + gpio->pins[offset].reg, > + gpio->pins[offset].func_mask, > + gpio->pins[offset].func_mask); > + if (ret) { > + dev_err(gpio->dev, "set gpio%d func failed\n", offset); > + return ret; > + } > + } > + > + return 0; > +} This is pin control. Why don't you implement a proper pin control driver for this chip? If you don't, this will just come back and haunt you. Why not merge the driver into drivers/pinctrl/* and name it pinctrl-rk805.c to begin with? > +static const struct gpio_chip rk805_chip = { > + .label = "rk805-gpio", > + .owner = THIS_MODULE, > + .direction_input = rk805_gpio_direction_input, > + .direction_output = rk805_gpio_direction_output, Please implement .get_direction() > + .get = rk805_gpio_get, > + .set = rk805_gpio_set, > + .request = rk805_gpio_request, > + .base = -1, > + .ngpio = 2, > + .can_sleep = true, Consider assigning the .names[] array some pin names. Yours, Linus Walleij