lkml.org 
[lkml]   [2016]   [Nov]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRE: [PATCH V2 4/4] gpio: pv88080: Add GPIO function support
    Date
    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@diasemi.com> wrote:
    >
    > > From: Eric Jeong <eric.jeong.opensource@diasemi.com>
    > >
    > > 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@diasemi.com>
    > (...)
    >
    > > +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
    \
     
     \ /
      Last update: 2016-11-08 07:16    [W:3.981 / U:0.068 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site