? 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 at rock-chips.com> wrote:
>>
>>> From: chenjh <chenjh at 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 at 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 at lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-rockchip
>>
>>
>
>
--
???
E-mail?chenjh at 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
