Re: [PATCH v6 08/12] gpio: Add GPIO driver for the RK805 PMIC

[Jianhong Chen <chenjh@rock-chips.com>]

在 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 <chenjh@rock-chips.com> wrote:
>>
>>> From: chenjh <chenjh@rock-chips.com>
>> 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 <chenjh@rock-chips.com>
>> 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 <linux/i2c.h>
>>> +#include <linux/gpio.h>
>> Only use:
>> #include <linux/gpio/driver.h>
>>
>>> +/*
>>> + * @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