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

Linus Walleij
2017-06-09T11:37:31Z

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.

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

> +#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?

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

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