Hi Andrew! Thanks for your patch!
On Sat, Dec 8, 2018 at 5:18 PM Andrew Lunn <andrew@xxxxxxx> wrote:
> Some TQ-Systems ComExpress modules contain an IO controller with 8
> GPIO lines.
>
> Signed-off-by: Andrew Lunn <andrew@xxxxxxx>
> +config GPIO_TQMX86
> + tristate "TQ-Systems QTMX86 GPIO"
> + depends on MFD_TQMX86
> + select GENERIC_IRQ_CHIP
I'd say probably remove that and add:
select GPIO_GENERIC
select GPIOLIB_IRQCHIP
Becuase I think this driver can use these two abstractions
quite nicely. It will diet the driver down to a very few
lines of code, which is nice. I am almost 100% sure that
you can use GPIO_GENERIC and 80% sure you can
use GPIOLIB_IRQCHIP.
> +#include <linux/gpio.h>
Just
#include <linux/gpio/driver.h>
these days.
> +#include <linux/irq.h>
> +#include <linux/irqdomain.h>
> +#include <linux/interrupt.h>
With the GPIOLIB_IRQCHIP you don't need these.
> +#define TQMX86_NGPIO 8
> +#define TQMX86_DIR_MASK 0xf0 /* 0-3 - output, 4-7 - input */
> +#define TQMX86_GPIODD 0 /* GPIO Data Direction Register */
> +#define TQMX86_GPIOD 1 /* GPIO Data Register */
> +#define TQMX86_GPIIC 3 /* GPI Interrupt Configuration Register */
> +#define TQMX86_GPIIS 4 /* GPI Interrupt Status Register */
This looks like a very straight-forward MMIO GPIO, which means
I think you can just use GPIO_GENERIC. Look at one of the
examples, like drivers/gpio/gpio-ftgpio010.c that show how to
get a compact driver with these.
As a bonus, using the generic driver gives you implementations
of .get_multiple() and .set_multiple() for free.
> +struct tqmx86_gpio_data {
> + struct gpio_chip chip;
> + void __iomem *io_base;
> + struct irq_domain *domain;
> + int irq;
No need for storing IRQs and irq domains if you use
GPIOLIB_IRQCHIP.
> + spinlock_t spinlock;
And GPIO_MMIO contains its own spinlock.
> + u8 irq_type[4];
> + int irqs[4]; /* mapped irqs */
This will likely be handled too.
> +static u8 tqmx86_gpio_read(struct tqmx86_gpio_data *gd, unsigned int reg)
> +static void tqmx86_gpio_write(struct tqmx86_gpio_data *gd, u8 val,
> + unsigned int reg)
> +static int tqmx86_gpio_get(struct gpio_chip *chip, unsigned int offset)
> +static void tqmx86_gpio_set(struct gpio_chip *chip, unsigned int offset,
> + int value)
> +static int tqmx86_gpio_direction_input(struct gpio_chip *chip,
> + unsigned int offset)
> +static int tqmx86_gpio_direction_output(struct gpio_chip *chip,
> + unsigned int offset,
> + int value)
> +static int tqmx86_gpio_get_direction(struct gpio_chip *chip,
> + unsigned int offset)
All of these will be assigned to default implementations after you call
bgpio_init() on your gpiolib so they just go away.
> +static int tqmx86_gpio_to_irq(struct gpio_chip *chip, unsigned int offset)
(...)
> + if (offset < 4 || offset > 7)
> + return -EINVAL;
> + ret = irq_create_mapping(gpio->domain, offset - 4);
> + if (ret > 0)
> + gpio->irqs[offset - 4] = ret;
This "offset of 4" is something I don't quite get and would stop
you from using GPIOLIB_IRQCHIP.
The gpiolib irqchip implementation sort of assumes offset 0..n
> +static void tqmx86_gpio_irq_noop(struct irq_data *data)
> +{
> +}
> +
> +static void tqmx86_gpio_irq_mask(struct irq_data *data)
> +{
> + struct tqmx86_gpio_data *gpio = data->domain->host_data;
> + unsigned long flags, irq_mask = data->mask;
> + int i;
> + u8 gpiic, mask = 0;
> +
> + for_each_set_bit(i, &irq_mask, 4)
> + mask |= 3 << (2 * i);
This looks strange, can we have some comment explaining what
happens here? Something like two bits per IRQ I get it,
but how are the bits arranged in the register?
And why does it start on 4?
> + if (mask) {
> + spin_lock_irqsave(&gpio->spinlock, flags);
> + gpiic = tqmx86_gpio_read(gpio, TQMX86_GPIIC);
> + gpiic &= ~mask;
> + tqmx86_gpio_write(gpio, gpiic, TQMX86_GPIIC);
> + spin_unlock_irqrestore(&gpio->spinlock, flags);
> + }
So you can still use your own irqchip with GPIO_GENERIC,
just use spin_lock(&chip->bgpio_lock); that comes with
GPIO_GENERIC.
> +static int tqmx86_gpio_irq_set_type(struct irq_data *data, unsigned int type)
> +{
> + struct tqmx86_gpio_data *gpio = data->domain->host_data;
> + unsigned int edge_type = type & IRQF_TRIGGER_MASK;
> + unsigned int offset = data->hwirq * 2; /* 2 bits per line */
> + unsigned long flags;
> + u8 new_type, gpiic;
> +
> + if (edge_type == IRQ_TYPE_EDGE_RISING)
> + new_type = TQMX86_GPII_RISING;
> + else if (edge_type == IRQ_TYPE_EDGE_FALLING)
> + new_type = TQMX86_GPII_FALLING;
> + else
> + return -EINVAL; /* not supported */
> +
> + gpio->irq_type[offset] = new_type;
So it doesn't support both egdes at the same time?
Since they appear to be two different bits it seems
like it can?
> + spin_lock_irqsave(&gpio->spinlock, flags);
> + gpiic = tqmx86_gpio_read(gpio, TQMX86_GPIIC);
> + gpiic &= ~(3 << offset);
> + gpiic |= new_type << offset;
> + tqmx86_gpio_write(gpio, gpiic, TQMX86_GPIIC);
> + spin_unlock_irqrestore(&gpio->spinlock, flags);
> + /* TODO: find offset, set GPIIC */
> +
> + irqd_set_trigger_type(data, type);
This is why you need irq_noop, because setting it to
rising/falling edge will call the .ack callback.
Maybe it's fine like this but then add a comment
about this to the noop callback.
> +static irqreturn_t tqmx86_gpio_irq_cascade(int irq, void *data)
I usually just call it irq_handler or something for familiarity.
> +{
> + struct tqmx86_gpio_data *gpio = data;
> + u8 irq_status = tqmx86_gpio_read(gpio, TQMX86_GPIIS);
> + int i;
> + unsigned long irq_bits;
> +
> + if (!irq_status)
> + return IRQ_HANDLED;
> +
> + tqmx86_gpio_write(gpio, irq_status, TQMX86_GPIIS);
> +
> + irq_bits = irq_status;
> + for_each_set_bit(i, &irq_bits, 4)
> + generic_handle_irq(irq_find_mapping(gpio->domain, i));
> +
> + return IRQ_HANDLED;
> +}
So if you can use GPIOLIB_IRQCHIP you just use
chained_irq_enter()/exit() in this handler before calling
generic_handle_irq()
> + chip = &gpio->chip;
> + chip->label = "gpio-tqmx86";
> + chip->owner = THIS_MODULE;
> + chip->can_sleep = false;
> + chip->base = -1;
> + chip->direction_input = tqmx86_gpio_direction_input;
> + chip->direction_output = tqmx86_gpio_direction_output;
> + chip->get_direction = tqmx86_gpio_get_direction;
> + chip->get = tqmx86_gpio_get;
> + chip->set = tqmx86_gpio_set;
So instead of these assignments just call bgpio_init()
to get standard accessors.
> + chip->ngpio = TQMX86_NGPIO;
This will go away too as you send in that it's 8 bits wide.
> + gpio->domain = irq_domain_add_linear(dev->of_node, 4,
> + &irq_generic_chip_ops,
> + gpio);
> + if (!gpio->domain)
> + return -ENOMEM;
> +
> + ret = irq_alloc_domain_generic_chips(gpio->domain, 4, 1,
> + chip->label,
> + handle_simple_irq,
> + 0, 0, 0);
> + if (ret)
> + return ret;
> +
> + gc = gpio->domain->gc->gc[0];
> + gc->private = gpio;
> + gc->chip_types[0].chip.irq_ack = tqmx86_gpio_irq_noop;
> + gc->chip_types[0].chip.irq_mask = tqmx86_gpio_irq_mask;
> + gc->chip_types[0].chip.irq_unmask = tqmx86_gpio_irq_unmask;
> + gc->chip_types[0].chip.irq_set_type = tqmx86_gpio_irq_set_type;
> +
> + chip->to_irq = tqmx86_gpio_to_irq;
As all this complex logic goes away with GPIOLIB_IRQCHIP
I recommend you use that if you can.
Since the struct gpio_irq_chip contains a "valid_mask" you
can (hopefully) just mask off the 4 first IRQs as invalid
and still use offsets 0..n for this GPIO irqchip.
> + ret = gpiochip_add_data(chip, gpio);
Why not devm_gpiochip_add_data()?
> + gpiochip_remove(&gpio->chip);
Then you don't need this.
Yours,
Linus Walleij
Re: [PATCH] gpio: tqmx86: Add GPIO driver for this IO controller
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
- Subject: Re: [PATCH] gpio: tqmx86: Add GPIO driver for this IO controller
- From: Linus Walleij <linus.walleij@xxxxxxxxxx>
- Date: Sun, 9 Dec 2018 12:01:52 +0100
- In-reply-to: <1544285898-24255-1-git-send-email-andrew@lunn.ch>
- References: <1544285898-24255-1-git-send-email-andrew@lunn.ch>
- Follow-Ups:
- Re: [PATCH] gpio: tqmx86: Add GPIO driver for this IO controller
- From: Andrew Lunn
- Re: [PATCH] gpio: tqmx86: Add GPIO driver for this IO controller
- From: Andrew Lunn
- Re: [PATCH] gpio: tqmx86: Add GPIO driver for this IO controller
- References:
- [PATCH] gpio: tqmx86: Add GPIO driver for this IO controller
- From: Andrew Lunn
- [PATCH] gpio: tqmx86: Add GPIO driver for this IO controller
- Prev by Date: Re: [gpio:devel 35/40] ERROR: "irq_chip_retrigger_hierarchy" [drivers/gpio/gpio-lpc18xx.ko] undefined!
- Next by Date: Re: [PATCH v2 2/2] gpiolib-acpi: Remove unnecessary WARN_ON from acpi_gpiochip_free_interrupts
- Previous by thread: [PATCH] gpio: tqmx86: Add GPIO driver for this IO controller
- Next by thread: Re: [PATCH] gpio: tqmx86: Add GPIO driver for this IO controller
- Index(es):
 |