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

From: Linus Walleij
Date: Fri Oct 28 2016 - 08:10:53 EST


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