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 linux-mmc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
