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, ®);
> > + 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, ®);
> > + 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
��.n��������+%������w��{.n����z�{��ܨ}���Ơz�j:+v�����w����ޙ��&�)ߡ�a����z�ޗ���ݢj��w�f
