On Tue, Feb 24, 2015 at 3:15 AM, Andrew Bresticker
<abrestic@xxxxxxxxxxxx> wrote:
> Add a driver for the pin controller present on the IMG Pistachio SoC.
> This driver provides pinmux and pinconfig operations as well as GPIO
> and IRQ chips for the GPIO banks.
>
> Signed-off-by: Damien Horsley <Damien.Horsley@xxxxxxxxxx>
> Signed-off-by: Govindraj Raja <govindraj.raja@xxxxxxxxxx>
> Signed-off-by: Andrew Bresticker <abrestic@xxxxxxxxxxxx>
(...)
> +static inline u32 pctl_readl(struct pistachio_pinctrl *pctl, u32 reg)
> +{
> + return readl(pctl->base + reg);
> +}
> +
> +static inline void pctl_writel(struct pistachio_pinctrl *pctl, u32 val, u32 reg)
> +{
> + writel(val, pctl->base + reg);
> +}
> +
> +static inline u32 gpio_readl(struct pistachio_gpio_bank *bank, u32 reg)
> +{
> + return readl(bank->base + reg);
> +}
> +
> +static inline void gpio_writel(struct pistachio_gpio_bank *bank, u32 val,
> + u32 reg)
> +{
> + writel(val, bank->base + reg);
> +}
I don't see the point of these special readl/writel accessors. Just
use readl/writel
directly. Or consider readl/writel_relaxed() if MIPS has this.
> +static inline void gpio_mask_writel(struct pistachio_gpio_bank *bank,
> + u32 reg, unsigned int bit, u32 val)
> +{
> + gpio_writel(bank, (0x10000 | val) << bit, reg);
> +}
Magic mask? Some comment on what is happening here when OR:in
on 0x10000?
(...)
> +static int pistachio_gpio_get_direction(struct gpio_chip *chip, unsigned offset)
> +{
> + struct pistachio_gpio_bank *bank = gc_to_bank(chip);
> +
> + if (gpio_readl(bank, GPIO_OUTPUT_EN) & BIT(offset))
> + return GPIOF_DIR_OUT;
> + return GPIOF_DIR_IN;
> +}
These flags are not for the driver API.
Do this:
return !gpio_readl(bank, GPIO_OUTPUT_EN) & BIT(offset));
> +static void pistachio_gpio_set(struct gpio_chip *chip, unsigned offset,
> + int value)
> +{
> + struct pistachio_gpio_bank *bank = gc_to_bank(chip);
> +
> + gpio_mask_writel(bank, GPIO_OUTPUT, offset, !!value);
> +}
Hm we should clamp value in the core and make the parameter "value"
a bool.... sigh for another day when things are calm.
(...)
> +static void pistachio_gpio_irq_handler(unsigned int irq, struct irq_desc *desc)
> +{
> + struct gpio_chip *gc = irq_get_handler_data(irq);
> + struct pistachio_gpio_bank *bank = gc_to_bank(gc);
> + struct irq_chip *chip = irq_get_chip(irq);
> + unsigned long pending;
> + unsigned int pin, virq;
Don't call it virq, just call it irq. All Linux irq numbers are virtual
so just go with irq.
> +
> + chained_irq_enter(chip, desc);
> + pending = gpio_readl(bank, GPIO_INTERRUPT_STATUS) &
> + gpio_readl(bank, GPIO_INTERRUPT_EN);
> + for_each_set_bit(pin, &pending, 16) {
> + virq = irq_linear_revmap(gc->irqdomain, pin);
> + generic_handle_irq(virq);
> + }
> + chained_irq_exit(chip, desc);
> +}
(...)
> +static int pistachio_gpio_register(struct pistachio_pinctrl *pctl)
> +{
> + struct device_node *child, *node = pctl->dev->of_node;
> + struct pistachio_gpio_bank *bank;
> + unsigned int i = 0;
> + int irq, ret = 0;
> +
> + for_each_child_of_node(node, child) {
> + if (!of_find_property(child, "gpio-controller", NULL))
> + continue;
So why not instead specify "simple-bus" as compatible on the parent node
and have each subnode be its own device (simple-bus will spawn platform
devices for all subnodes).
Overall this composite-device pattern is discouraged if we can instead have
unique devices for each bank.
Apart from these things the driver looks very nice!
Yours,
Linus Walleij
