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 [off-list ref] wrote:

quoted

From: chenjh <redacted>

Full name please.

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

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

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

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?

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?

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