On Thu, Oct 9, 2014 at 10:07 PM, John Crispin <blogic@xxxxxxxxxxx> wrote:
> Add gpio driver for Ralink SoC. This driver makes the gpio core on
> RT2880, RT305x, rt3352, rt3662, rt3883, rt5350 and mt7620 work.
>
> Signed-off-by: John Crispin <blogic@xxxxxxxxxxx>
> Cc: linux-mips@xxxxxxxxxxxxxx
> Cc: linux-gpio@xxxxxxxxxxxxxxx
As mentioned I think this driver should register the GPIO to pin
range using gpiochip_add_pin_range() or
gpiochip_add_pingroup_range().
> +++ b/arch/mips/include/asm/mach-ralink/gpio.h
> @@ -0,0 +1,24 @@
> +/*
> + * Ralink SoC GPIO API support
> + *
> + * Copyright (C) 2008-2009 Gabor Juhos <juhosg@xxxxxxxxxxx>
> + * Copyright (C) 2008 Imre Kaloz <kaloz@xxxxxxxxxxx>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published
> + * by the Free Software Foundation.
> + *
> + */
> +
> +#ifndef __ASM_MACH_RALINK_GPIO_H
> +#define __ASM_MACH_RALINK_GPIO_H
> +
> +#define ARCH_NR_GPIOS 128
The generic definition is 512, do you really need to
modify this?
> +#include <asm-generic/gpio.h>
> +
> +#define gpio_get_value __gpio_get_value
> +#define gpio_set_value __gpio_set_value
> +#define gpio_cansleep __gpio_cansleep
> +#define gpio_to_irq __gpio_to_irq
> +
> +#endif /* __ASM_MACH_RALINK_GPIO_H */
The rest is jus what you get without providing this file
at all, right?
> +++ b/drivers/gpio/gpio-rt2880.c
> @@ -0,0 +1,354 @@
> +/*
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published
> + * by the Free Software Foundation.
> + *
> + * Copyright (C) 2009-2011 Gabor Juhos <juhosg@xxxxxxxxxxx>
> + * Copyright (C) 2013 John Crispin <blogic@xxxxxxxxxxx>
> + */
> +
> +#include <linux/module.h>
> +#include <linux/io.h>
> +#include <linux/gpio.h>
> +#include <linux/spinlock.h>
> +#include <linux/platform_device.h>
> +#include <linux/of_irq.h>
> +#include <linux/irqdomain.h>
If this is a cascaded GPIO chip (one accumulated IRQ line from all the
GPIO lines) you should instead select GPIOLIB_IRQCHIP
and follow the example of the drivers that use this.
> +enum ralink_gpio_reg {
> + GPIO_REG_INT = 0,
> + GPIO_REG_EDGE,
> + GPIO_REG_RENA,
> + GPIO_REG_FENA,
> + GPIO_REG_DATA,
> + GPIO_REG_DIR,
> + GPIO_REG_POL,
> + GPIO_REG_SET,
> + GPIO_REG_RESET,
> + GPIO_REG_TOGGLE,
> + GPIO_REG_MAX
> +};
No that I see why this needs an enumerator and cannot just
be #defined, but whatdoIknow...
> +struct ralink_gpio_chip {
> + struct gpio_chip chip;
> + u8 regs[GPIO_REG_MAX];
> +
> + spinlock_t lock;
> + void __iomem *membase;
> + struct irq_domain *domain;
> + int irq;
Usually you don't need to keep the irq number around.
> +
> + u32 rising;
> + u32 falling;
> +};
Some of this could use some kerneldoc.
> +#define MAP_MAX 4
> +static struct irq_domain *irq_map[MAP_MAX];
> +static int irq_map_count;
> +static atomic_t irq_refcount = ATOMIC_INIT(0);
I don't get why you have a per-device state container and then
add these static locals for the irqdomains that seems unnecessary.
I prefer if all of it goes into the state container.
> +static int ralink_gpio_to_irq(struct gpio_chip *chip, unsigned pin)
> +{
> + struct ralink_gpio_chip *rg = to_ralink_gpio(chip);
> +
> + if (rg->irq < 1)
> + return -1;
> +
> + return irq_create_mapping(rg->domain, pin);
> +}
So this would rather use
irq_find_mapping(chip->irqdomain, pin);
with GPIOLIB_IRQCHIP selected.
> +static void ralink_gpio_irq_handler(unsigned int irq, struct irq_desc *desc)
> +{
> + int i;
> +
> + for (i = 0; i < irq_map_count; i++) {
> + struct irq_domain *domain = irq_map[i];
> + struct ralink_gpio_chip *rg;
> + unsigned long pending;
> + int bit;
> +
> + rg = (struct ralink_gpio_chip *) domain->host_data;
> + pending = rt_gpio_r32(rg, GPIO_REG_INT);
> +
> + for_each_set_bit(bit, &pending, rg->chip.ngpio) {
> + u32 map = irq_find_mapping(domain, bit);
> +
> + generic_handle_irq(map);
> + rt_gpio_w32(rg, GPIO_REG_INT, BIT(bit));
> + }
> + }
> +}
Atleast set the driver state container in irq_get_handler_data() so you don't
need the static locals to find the maps.
If you use GPIOLIB_IRQCHIP you have the gpiochip in the handler datar,
and the irqdomain in the gpiochip.
> +static int gpio_map(struct irq_domain *d, unsigned int irq, irq_hw_number_t hw)
> +{
> + irq_set_chip_and_handler(irq, &ralink_gpio_irq_chip, handle_level_irq);
> + irq_set_handler_data(irq, d);
> +
> + return 0;
> +}
> +
> +static const struct irq_domain_ops irq_domain_ops = {
> + .xlate = irq_domain_xlate_onecell,
> + .map = gpio_map,
> +};
> +
> +static void ralink_gpio_irq_init(struct device_node *np,
> + struct ralink_gpio_chip *rg)
> +{
> + if (irq_map_count >= MAP_MAX)
> + return;
> +
> + rg->irq = irq_of_parse_and_map(np, 0);
> + if (!rg->irq)
> + return;
> +
> + rg->domain = irq_domain_add_linear(np, rg->chip.ngpio,
> + &irq_domain_ops, rg);
> + if (!rg->domain) {
> + dev_err(rg->chip.dev, "irq_domain_add_linear failed\n");
> + return;
> + }
> +
> + irq_map[irq_map_count++] = rg->domain;
> +
> + rt_gpio_w32(rg, GPIO_REG_RENA, 0x0);
> + rt_gpio_w32(rg, GPIO_REG_FENA, 0x0);
> +
> + if (!atomic_read(&irq_refcount))
> + irq_set_chained_handler(rg->irq, ralink_gpio_irq_handler);
> + atomic_inc(&irq_refcount);
> +
> + dev_info(rg->chip.dev, "registering %d irq handlers\n", rg->chip.ngpio);
> +}
Use GPIOLIB_IRQCHIP to centralize all this complex stuff.
> + if (of_property_read_u8_array(np, "ralink,register-map",
> + rg->regs, GPIO_REG_MAX)) {
> + dev_err(&pdev->dev, "failed to read register definition\n");
> + return -EINVAL;
> + }
Is this really wise? Isn't it better to encode the type of block
and then have these register maps statically in the driver here.
> + ngpio = of_get_property(np, "ralink,num-gpios", NULL);
> + if (!ngpio) {
> + dev_err(&pdev->dev, "failed to read number of pins\n");
> + return -EINVAL;
> + }
Also the number of gpios would come from the version ID.
> + gpiobase = of_get_property(np, "ralink,gpio-base", NULL);
> + if (gpiobase)
> + rg->chip.base = be32_to_cpu(*gpiobase);
> + else
> + rg->chip.base = -1;
Over my dead body. We want dynamic GPIO numbers.
Nobody will OK that DT binding anyway.
Just set this to -1 and be happy.
> +subsys_initcall(ralink_gpio_init);
Does it need to be this early, really?
Yours,
Linus Walleij
