在 2017/6/9 20:17, Heiko Stuebner 写道:

Hi,

Am Freitag, 9. Juni 2017, 13:37:26 CEST schrieb Linus Walleij:

quoted

Heiko, can you please look at this patch.

On Thu, Jun 8, 2017 at 9:30 AM, Jianhong Chen [off-list ref] wrote:

quoted

From: chenjh <redacted>

Full name please.

git config --global user.name "John Doe"

might do the and make this permanent for all your commits :-)

quoted

quoted

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 <redacted>

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.

quoted

quoted

+#include <linux/i2c.h>
+#include <linux/gpio.h>

Only use:
#include <linux/gpio/driver.h>

quoted

+/*
+ * @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.

quoted

quoted

+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)

quoted

+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/* ?

quoted

quoted

+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()

quoted

+       .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