Re: [PATCH V2 4/4] gpio: pv88080: Add GPIO function support

[Linus Walleij]

On Thu, Oct 27, 2016 at 3:03 AM, Eric Jeong
<eric.jeong.opensource@xxxxxxxxxxx> wrote:

> From: Eric Jeong <eric.jeong.opensource@xxxxxxxxxxx>
>
> This patch adds support for PV88080 PMIC GPIOs.
> PV88080 has two configurable GPIOs.
>
> Kconfig and Makefile are updated to reflect support
> for PV88080 PMIC GPIO.
>
> Signed-off-by: Eric Jeong <eric.jeong.opensource@xxxxxxxxxxx>
(...)

> +static int pv88080_gpio_direction_input(struct gpio_chip *gc,
> +                               unsigned int offset)
> +{
> +       struct pv88080_gpio *priv = gpiochip_get_data(gc);
> +       struct pv88080 *chip = priv->chip;
> +       int ret;
> +
> +       /* Set the initial value */
> +       ret = regmap_update_bits(chip->regmap, priv->gpio_base_reg + offset,
> +                                       PV88080_GPIO_OUTPUT_MASK, 0);
> +       if (ret)
> +               return ret;

So you set the initial value when we change the pin to *input*...

> +
> +       return regmap_update_bits(chip->regmap, priv->gpio_base_reg + offset,
> +                               PV88080_GPIO_DIRECTION_MASK, 0);
> +}
> +
> +static int pv88080_gpio_direction_output(struct gpio_chip *gc,
> +                               unsigned int offset, int value)
> +{
> +       struct pv88080_gpio *priv = gpiochip_get_data(gc);
> +       struct pv88080 *chip = priv->chip;
> +       int ret;
> +
> +       ret = regmap_update_bits(chip->regmap, priv->gpio_base_reg + offset,
> +                       PV88080_GPIO_DIRECTION_MASK,
> +                       PV88080_GPIO_DIRECTION_MASK);

But do nothing when we change the pin to *output*?

It seems like you switched the two function implementations or
something?

> +static int pv88080_gpio_get(struct gpio_chip *gc, unsigned int offset)
> +{
> +       struct pv88080_gpio *priv = gpiochip_get_data(gc);
> +       struct pv88080 *chip = priv->chip;
> +       unsigned int reg = 0, direction;
> +       int ret;
> +
> +       ret = regmap_read(chip->regmap, priv->gpio_base_reg + offset, &reg);
> +       if (ret)
> +               return ret;
> +
> +       direction = (reg & PV88080_GPIO_DIRECTION_MASK);
> +       if (direction == PV88080_PORT_DIRECTION_OUTPUT) {
> +               if (reg & PV88080_GPIO_OUTPUT_EN)
> +                       return 1;
> +               ret = 0;
> +       } else {
> +               ret = regmap_read(chip->regmap, priv->input_reg, &reg);
> +               if (ret < 0)
> +                       return ret;
> +               ret = (reg & (PV88080_GPIO_INPUT_MASK << offset)) >> offset;

Isn't this what you want to do?

#include <linux/bitops.h>

ret = !!(reg & BIT(offset));

The mask is 0x01. No point in making things more complicated than they are.


> +static void pv88080_gpio_set(struct gpio_chip *gc, unsigned int offset,
> +                               int value)
> +{
> +       struct pv88080_gpio *priv = gpiochip_get_data(gc);
> +       struct pv88080 *chip = priv->chip;
> +
> +       if (value)
> +               regmap_update_bits(chip->regmap, priv->gpio_base_reg + offset,
> +                               PV88080_GPIO_OUTPUT_MASK,
> +                               PV88080_GPIO_OUTPUT_EN);
> +       else
> +               regmap_update_bits(chip->regmap, priv->gpio_base_reg + offset,
> +                               PV88080_GPIO_OUTPUT_MASK,
> +                               PV88080_GPIO_OUTPUT_DIS);
> +}

Looks good, output is more complicated.

> +static const struct gpio_chip template_gpio = {
> +       .label = "pv88080-gpio",
> +       .owner = THIS_MODULE,
> +       .get_direction = pv88080_gpio_get_direction,
> +       .direction_input = pv88080_gpio_direction_input,
> +       .direction_output = pv88080_gpio_direction_output,
> +       .get = pv88080_gpio_get,
> +       .set = pv88080_gpio_set,
> +       .base = -1,
> +       .ngpio = DEFAULT_PIN_NUMBER,
> +};

Why even have a #define for DEFAULT_PIN_NUMBER?
Just hardcode it here.

> +       priv->chip = chip;
> +       priv->gpio_chip = template_gpio;
> +       priv->gpio_chip.parent = chip->dev;

I slightly prefer that you fill in the priv->gpio_chip with code (one
assignment per line) rather than
assigning a template like here, but it's your pick.

> +       if (pdata && pdata->gpio_base)
> +               priv->gpio_chip.base = pdata->gpio_base;

Give me any good reason to support this. Please just drop
this platform data. Use -1 like in the template and get
dynamic assignment of GPIO numbers.

Yours,
Linus Walleij