Heiko, can you please look at this patch.
On Thu, Jun 8, 2017 at 9:30 AM, Jianhong Chen <chenjh@xxxxxxxxxxxxxx> wrote:
> From: chenjh <chenjh@xxxxxxxxxxxxxx>
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@xxxxxxxxxxxxxx>
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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
