Re: [PATCH] gpio: add Intel WhiskeyCove GPIO driver

[Bin Gao <bin.gao@linux.intel.com>]

On Mon, Jun 13, 2016 at 02:43:16PM +0200, Linus Walleij wrote:
> On Sat, Jun 11, 2016 at 8:01 AM, Bin Gao <bin.gao@linux.intel.com> wrote:
> 
> > This patch introduces a separate GPIO driver for Intel WhiskeyCove PMIC.
> > This driver is based on gpio-crystalcove.c.
> >
> > Signed-off-by: Ajay Thomas <ajay.thomas.david.rajamanickam@intel.com>
> > Signed-off-by: Bin Gao <bin.gao@intel.com>
> 
> It is always good to let Mika and Mathias look at new Intel GPIO
> drivers, so added them to the To: line.
> 
> > +config GPIO_WHISKEY_COVE
> > +       tristate "GPIO support for Whiskey Cove PMIC"
> > +       depends on INTEL_SOC_PMIC
> > +       select GPIOLIB_IRQCHIP
> > +       help
> > +         Support for GPIO pins on Whiskey Cove PMIC.
> > +
> > +         Say Yes if you have a Intel SoC based tablet with Whsikey Cove PMIC
> 
> Speling
> 
> > +#include <linux/interrupt.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/gpio.h>
> 
> No use just #include <linux/gpio/driver.h>
> 
> > +#include <linux/seq_file.h>
> > +#include <linux/bitops.h>
> > +#include <linux/regmap.h>
> > +#include <linux/mfd/intel_soc_pmic.h>
> > +
> > +#define WCOVE_GPIO_NUM 13
> > +#define WCOVE_VGPIO_NUM        94
> > +
> > +#define UPDATE_IRQ_TYPE                BIT(0)
> > +#define UPDATE_IRQ_MASK                BIT(1)
> > +
> > +#define GPIOIRQ0               0x4e0b
> > +#define GPIOIRQ1               0x4e0c
> > +#define MGPIOIRQ0              0x4e19
> > +#define MGPIOIRQ1              0x4e1a
> > +#define GPIO0P0CTLO            0x4e44
> > +#define GPIO0P0CTLI            0x4e51
> > +#define GPIO1P0CTLO            0x4e4b
> > +#define GPIO1P0CTLI            0x4e58
> > +#define GPIO2P0CTLO            0x4e4f
> > +#define GPIO2P0CTLI            0x4e5c
> > +
> > +#define CTLI_INTCNT_DIS                (0)
> > +#define CTLI_INTCNT_NE         (1 << 1)
> > +#define CTLI_INTCNT_PE         (2 << 1)
> > +#define CTLI_INTCNT_BE         (3 << 1)
> > +
> > +#define CTLO_DIR_IN            (0)
> > +#define CTLO_DIR_OUT           (1 << 5)
> > +
> > +#define CTLO_DRV_CMOS          (0)
> > +#define CTLO_DRV_OD            (1 << 4)
> 
> That is likely what we call push-pull and open drain driving.
> Implement .set_single_ended() for this driver so you can handle
> that properly in the driver.
> 
> > +#define CTLO_DRV_REN           (1 << 3)
> > +
> > +#define CTLO_RVAL_2KDW         (0)
> > +#define CTLO_RVAL_2KUP         (1 << 1)
> > +#define CTLO_RVAL_50KDW                (2 << 1)
> > +#define CTLO_RVAL_50KUP                (3 << 1)
> 
> Looks like pull-up settings. That is strictly speaking pin control.
> 
> But OK I am trying to find the right way to abstract this without
> making GPIO too heavyweight.
> 
> > +static inline struct wcove_gpio *to_wg(struct gpio_chip *gc)
> > +{
> > +       return container_of(gc, struct wcove_gpio, chip);
> > +}
> 
> No don't do that. Use devm_gpiochip_add_data() and just
> use gpiochip_get_data() to get the pointer out. Look at any
> other driver in the upstream kernel, I think I converted them all.
> 
> > +static inline int to_reg(int gpio, enum ctrl_register reg_type)
> > +{
> > +       int reg;
> > +
> > +       if (reg_type == CTRL_IN) {
> > +               if (gpio < 7)
> > +                       reg = GPIO0P0CTLI + gpio;
> > +               else if (gpio < 11)
> > +                       reg = GPIO1P0CTLI + (gpio % 7);
> > +               else
> > +                       reg = GPIO2P0CTLI + (gpio % 11);
> > +       } else {
> > +               if (gpio < 7)
> > +                       reg = GPIO0P0CTLO + gpio;
> > +               else if (gpio < 11)
> > +                       reg = GPIO1P0CTLO + (gpio % 7);
> > +               else
> > +                       reg = GPIO2P0CTLO + (gpio % 11);
> > +       }
> > +
> > +       return reg;
> > +}
> 
> You could add a kerneldoc to this function explaining how these
> GPIO registers are laid out.
> 
> > +static void wcove_gpio_set(struct gpio_chip *chip,
> > +                                unsigned int gpio, int value)
> > +{
> > +       struct wcove_gpio *wg = to_wg(chip);
> > +
> > +       if (gpio > WCOVE_VGPIO_NUM)
> > +               return;
> 
> gpiolib already protects against this, you can drop it everywhere.
> 
> > +static irqreturn_t wcove_gpio_irq_handler(int irq, void *data)
> > +{
> > +       struct wcove_gpio *wg = data;
> > +       unsigned int p0, p1, virq;
> > +       int pending, gpio;
> > +
> > +       if (regmap_read(wg->regmap, GPIOIRQ0, &p0) ||
> > +           regmap_read(wg->regmap, GPIOIRQ1, &p1))
> > +               return IRQ_NONE;
> 
> What does this mean? If we fail to read regmaps then
> it was not our IRQ?
> 
> Should be an error message or something at least I think?
> 
> Apart from that it looks all right.
> 
> Yours,
> Linus Walleij

Linus,

Thank you for your review. We'll come up with a version 2.

-Bin