On Wed, Jan 25, 2017 at 7:51 PM, Paul Cercueil <paul@xxxxxxxxxxxxxxx> wrote: > This driver handles the GPIOs of all the Ingenic JZ47xx SoCs > currently supported by the upsteam Linux kernel. > > Signed-off-by: Paul Cercueil <paul@xxxxxxxxxxxxxxx> Looking nice. > +#define JZ4740_GPIO_DATA 0x10 > +#define JZ4740_GPIO_SELECT 0x50 > +#define JZ4740_GPIO_DIR 0x60 > +#define JZ4740_GPIO_TRIG 0x70 > +#define JZ4740_GPIO_FLAG 0x80 > + > +#define JZ4780_GPIO_INT 0x10 > +#define JZ4780_GPIO_PAT1 0x30 > +#define JZ4780_GPIO_PAT0 0x40 > +#define JZ4780_GPIO_FLAG 0x50 > + > +#define REG_SET(x) ((x) + 0x4) > +#define REG_CLEAR(x) ((x) + 0x8) (...) > +enum jz_version { > + ID_JZ4740, > + ID_JZ4780, > +}; (...) > +static inline bool gpio_get_value(struct ingenic_gpio_chip *jzgc, u8 offset) > +{ > + if (jzgc->version >= ID_JZ4780) > + return readl(jzgc->base + GPIO_PIN) & BIT(offset); > + else > + return readl(jzgc->base + JZ4740_GPIO_DATA) & BIT(offset); > +} This works for me, for sure. What some people do, is to put the right virtual address in to the state container. So it would be just: return !!readl(jzgc->datareg) & BIT(offset)); Notice also the double-bang that clamps the value to a bool. I know the core does it too but I like to see it in drivers just to be sure. > +static void gpio_set_value(struct ingenic_gpio_chip *jzgc, u8 offset, int value) > +{ > + u8 reg; > + > + if (jzgc->version >= ID_JZ4780) > + reg = JZ4780_GPIO_PAT0; > + else > + reg = JZ4740_GPIO_DATA; > + > + if (value) > + writel(BIT(offset), jzgc->base + REG_SET(reg)); > + else > + writel(BIT(offset), jzgc->base + REG_CLEAR(reg)); > +} Same comment. What some drivers do when they just get/set a bit in a register to get/set or set the direction of a GPIO, is to select GPIO_GENERIC and just bgpio_init() with the right iomem pointers, then the core will register handlers for get, set, set_direcition callback and get_direction and your driver can just focus on the remainders. > +static void ingenic_gpio_set(struct gpio_chip *gc, > + unsigned int offset, int value) > +{ > + struct ingenic_gpio_chip *jzgc = gpiochip_get_data(gc); > + > + gpio_set_value(jzgc, offset, value); > +} > + > +static int ingenic_gpio_get(struct gpio_chip *gc, unsigned int offset) > +{ > + struct ingenic_gpio_chip *jzgc = gpiochip_get_data(gc); > + > + return (int) gpio_get_value(jzgc, offset); > +} > + > +static int ingenic_gpio_direction_input(struct gpio_chip *gc, > + unsigned int offset) > +{ > + return pinctrl_gpio_direction_input(gc->base + offset); > +} > + > +static int ingenic_gpio_direction_output(struct gpio_chip *gc, > + unsigned int offset, int value) > +{ > + ingenic_gpio_set(gc, offset, value); > + return pinctrl_gpio_direction_output(gc->base + offset); > +} If you're not just replacing these with GPIO_GENERIC, please also include a .get_direction() callback. It's especially nice as it reads out the state at probe and "lsgpio" lists if pins are inputs or outputs. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html