Re: [PATCH 2/2] pinctrl: Add Pistachio SoC pin control driver

On Tue, Feb 24, 2015 at 3:15 AM, Andrew Bresticker
[off-list ref] 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 <redacted>
Signed-off-by: Govindraj Raja <redacted>
Signed-off-by: Andrew Bresticker <redacted>

(...)

+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