Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752148AbdFNMJ2 (ORCPT ); Wed, 14 Jun 2017 08:09:28 -0400 Received: from regular1.263xmail.com ([211.150.99.138]:59763 "EHLO regular1.263xmail.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751719AbdFNMJ0 (ORCPT ); Wed, 14 Jun 2017 08:09:26 -0400 X-263anti-spam: KSV:0; X-MAIL-GRAY: 0 X-MAIL-DELIVERY: 1 X-KSVirus-check: 0 X-ABS-CHECKED: 4 X-RL-SENDER: chenjh@rock-chips.com X-FST-TO: w.egorov@phytec.de X-SENDER-IP: 58.22.7.114 X-LOGIN-NAME: chenjh@rock-chips.com X-UNIQUE-TAG: <3f560b96d62e8a89c97267fa4342a84a> X-ATTACHMENT-NUM: 0 X-DNS-TYPE: 0 Subject: Re: [PATCH v6 08/12] gpio: Add GPIO driver for the RK805 PMIC To: Heiko Stuebner , linux-rockchip@lists.infradead.org Cc: Linus Walleij , Tao Huang , Alexandre Courbot , zhangqing@rock-chips.com, "devicetree@vger.kernel.org" , Mark Brown , Dmitry Torokhov , "linux-kernel@vger.kernel.org" , Rob Herring , Liam Girdwood , tony.xie@rock-chips.com, "linux-gpio@vger.kernel.org" , Linux Input , Mark Rutland , w.egorov@phytec.de References: <1496905959-29202-1-git-send-email-chenjh@rock-chips.com> <1496907001-27107-1-git-send-email-chenjh@rock-chips.com> <2451251.ri2bNrhWeM@phil> From: Jianhong Chen Message-ID: Date: Wed, 14 Jun 2017 20:11:06 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.1.1 MIME-Version: 1.0 In-Reply-To: <2451251.ri2bNrhWeM@phil> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5335 Lines: 146 在 2017/6/9 20:17, Heiko Stuebner 写道: > Hi, > > Am Freitag, 9. Juni 2017, 13:37:26 CEST schrieb Linus Walleij: >> 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. > git config --global user.name "John Doe" > > might do the and make this permanent for all your commits :-) > >>> 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? > So far, I've only seen the rk808 and rk818. Both do not have any > configurable pins. > > The rk805 which is a sort of variant of the above, does have the two > pins defined below, but in the manual I could also only find them as > output-only and having no other function than being output-pins. > > So I don't really know if all the input- or "gpio-mode"- handling is only > an oversight (copy'n'paste) or if there are yet other rk808 variants around > that can actually be configured as inputs or even non-gpio modes? > > I hope Jianhong will be able to answer that. > > > Heiko This driver is not only for rk805, but also intend for rk816 and furtrue PMICs. The rk816 has one multi function pin(TS/GPIO), when setting as gpio, it can be configured as output or input. Here is simple description from manual: "Thermistor input. Connect a thermistor from this pin to ground. The thermistor is usually inside the battery pack. (multi-function for GPIO) ". At beginning, I want to name it "gpio-rk8xx.c", but I found rk808 and rk818 don't have gpio function. So I abandon this name and use "gpio-rk805.c", but still implement it as a generic driver for furture PMICs to be added. >>> +#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? Yes, as explain above, these "OUTPUT_MODE, INPUTOUT" flags are prepared for RK816 or furture PMICs. Maybe these should be enum are better. >>> +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? Becuase I refered to other PMICs, I see most of their gpio driver is merged into drivers/gpio/*, Only a few are added in drivers/pinctrl/*. So, it is better to merge the driver into drivers/pinctrl/* ? >>> +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 >> >> _______________________________________________ >> Linux-rockchip mailing list >> Linux-rockchip@lists.infradead.org >> http://lists.infradead.org/mailman/listinfo/linux-rockchip >> >> > > -- 陈健洪 E-mail：chenjh@rock-chips.com 福州瑞芯微电子股份有限公司 Fuzhou Rockchip Electronics Co.Ltd 福建省福州市铜盘路软件大道89号软件园A区21号楼 (350003) No. 21 Building, A District, No.89,software Boulevard Fuzhou,Fujian,PRC TEL：0591-83991906/07-8573