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@xxxxxxxxxxxxxxx> 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@xxxxxxxxx> > > Signed-off-by: Bin Gao <bin.gao@xxxxxxxxx> > > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html