Re: [RFC 09/18] gpio: add driver for Atheros AR5312 SoC GPIO controller

[Linus Walleij <linus.walleij@xxxxxxxxxx>]

On Sun, Sep 14, 2014 at 9:33 PM, Sergey Ryazanov <ryazanov.s.a@xxxxxxxxx> wrote:

> Atheros AR5312 SoC have a builtin GPIO controller, which could be
> accessed via memory mapped registers. This patch adds new driver
> for them.
>
> Signed-off-by: Sergey Ryazanov <ryazanov.s.a@xxxxxxxxx>
> Cc: Linus Walleij <linus.walleij@xxxxxxxxxx>
> Cc: Alexandre Courbot <gnurou@xxxxxxxxx>
> Cc: linux-gpio@xxxxxxxxxxxxxxx
(...)
> -       /* Attempt to jump to the mips reset location - the boot loader
> -        * itself might be able to recover the system */
> +       /* Cold reset does not work on the AR2315/6, use the GPIO reset bits
> +        * a workaround. Give it some time to attempt a gpio based hardware
> +        * reset (atheros reference design workaround) */
> +       gpio_request_one(AR2315_RESET_GPIO, GPIOF_OUT_INIT_LOW, "Reset");
> +       mdelay(100);

Please try to use the new GPIO descriptor API.
See Documentation/gpio/consumer.txt

(...)
> +++ b/drivers/gpio/gpio-ar2315.c

> +static u32 ar2315_gpio_intmask;
> +static u32 ar2315_gpio_intval;
> +static unsigned ar2315_gpio_irq_base;
> +static void __iomem *ar2315_mem;

Get rid of these local variables and put them into an allocated
state container, see
Documentation/driver-model/design-patterns.txt

Get rid of _gpio_irq_base altogether. (See comments below.)

(...)
> +static inline u32 ar2315_gpio_reg_read(unsigned reg)
> +{
> +       return __raw_readl(ar2315_mem + reg);
> +}

Use readl_relaxed()

> +static inline void ar2315_gpio_reg_write(unsigned reg, u32 val)
> +{
> +       __raw_writel(val, ar2315_mem + reg);
> +}

Use writel_relaxed()

> +static void ar2315_gpio_irq_handler(unsigned irq, struct irq_desc *desc)
> +{
> +       u32 pend;
> +       int bit = -1;
> +
> +       /* only do one gpio interrupt at a time */
> +       pend = ar2315_gpio_reg_read(AR2315_GPIO_DI);
> +       pend ^= ar2315_gpio_intval;
> +       pend &= ar2315_gpio_intmask;
> +
> +       if (pend) {
> +               bit = fls(pend) - 1;
> +               pend &= ~(1 << bit);

Do this:

#include <linux/bitops.h>

pend &= ~BIT(bit);

> +       ar2315_gpio_intmask |= (1 << gpio);

|= BIT(gpio);

> +       ar2315_gpio_int_setup(gpio, AR2315_GPIO_INT_TRIG_EDGE);
> +}
> +
> +static void ar2315_gpio_irq_mask(struct irq_data *d)
> +{
> +       unsigned gpio = d->irq - ar2315_gpio_irq_base;

Wait, no. No keeping irq bases around like that please.

unsigned offset = d->hwirq;

And call it offset rather than "gpio" everywhere please, since
it is local to this GPIO controller, not the global GPIO numberspace.

> +       /* Disable interrupt */
> +       ar2315_gpio_intmask &= ~(1 << gpio);

&= ~BIT(gpio);

> +static int ar2315_gpio_get_val(struct gpio_chip *chip, unsigned gpio)
> +{
> +       return (ar2315_gpio_reg_read(AR2315_GPIO_DI) >> gpio) & 1;
> +}

Do this:
return !!(ar2315_gpio_reg_read(AR2315_GPIO_DI) & BIT(gpio));

> +static void ar2315_gpio_set_val(struct gpio_chip *chip, unsigned gpio, int val)
> +{
> +       u32 reg = ar2315_gpio_reg_read(AR2315_GPIO_DO);
> +
> +       reg = val ? reg | (1 << gpio) : reg & ~(1 << gpio);
> +       ar2315_gpio_reg_write(AR2315_GPIO_DO, reg);

Convoluted, I would use an if() else construct rather than the ? operator.

> +static int ar2315_gpio_to_irq(struct gpio_chip *chip, unsigned gpio)
> +{
> +       return ar2315_gpio_irq_base + gpio;
> +}

Don't implement this at all. You're using the gpiolib irqchip helpers!
It will just overrun this implementation.

Yours,
Linus Walleij
--
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