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

[Eric Hyeung Dong Jeong]

On Friday, October 28, 2016 9:11 PM, Linux Walleij wrote:

> 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

Thank you for the comments and recommendation.
I will try to updated the code like your recommendation and make it simple.  Then, I will send patch again.

Regards
Eric