On Thu, Nov 28, 2013 at 4:15 AM, <thloh@xxxxxxxxxx> wrote:
> From: Tien Hock Loh <thloh@xxxxxxxxxx>
>
> Add driver support for Altera GPIO soft IP, including interrupts and I/O.
> Tested on Altera CV SoC board using dipsw and LED using LED framework.
>
> Signed-off-by: Tien Hock Loh <thloh@xxxxxxxxxx>
Overall it looks good (some stuff to fix below).
I want an ACK from some device tree maintainer on the bindings,
ideally.
> +- altr,interrupt_trigger: Specifies the interrupt trigger type the GPIO
> + hardware is synthesized. This field is required if the Altera GPIO controller
> + used has IRQ enabled as the interrupt type is not software controlled,
> + but hardware synthesized. Required if GPIO is used as an interrupt
> + controller. The value is defined in <dt-bindings/interrupt-controller/irq.h>
> + Only the following flags are supported:
> + IRQ_TYPE_EDGE_RISING
> + IRQ_TYPE_EDGE_FALLING
> + IRQ_TYPE_EDGE_BOTH
> + IRQ_TYPE_LEVEL_HIGH
I remember discussing this, and it's OK from my side.
> +/**
> +* struct altera_gpio_chip
> +* @mmchip : memory mapped chip structure.
> +* @irq : irq domain that this driver is registered to.
> +* @gpio_lock : synchronization lock so that new irq/set/get requests
> + will be blocked until the current one completes.
> +* @interrupt_trigger : specifies the hardware configured IRQ trigger type
> + (rising, falling, both, high)
> +* @mapped_irq : kernel mapped irq number.
> +*/
> +struct altera_gpio_chip {
> + struct of_mm_gpio_chip mmchip;
> + struct irq_domain *irq;
Argh that's a bad member name for an irqdomain, please just
call this "domain" like the other drivers do.
> + spinlock_t gpio_lock;
> + int interrupt_trigger;
> + int edge_type;
> + int mapped_irq;
Why are you keeping this around? Well I guess I might figure out...
> +static int altera_gpio_irq_set_type(struct irq_data *d,
> + unsigned int type)
> +{
> + struct altera_gpio_chip *altera_gc = irq_data_get_irq_chip_data(d);
> +
> + if (type == IRQ_TYPE_NONE)
> + return 0;
> +
> + if (type == IRQ_TYPE_LEVEL_HIGH &&
> + altera_gc->interrupt_trigger == IRQ_TYPE_LEVEL_HIGH) {
> + return 0;
> + } else {
> + if (type == IRQ_TYPE_EDGE_RISING &&
> + altera_gc->interrupt_trigger == IRQ_TYPE_EDGE_RISING)
> + return 0;
> + else if (type == IRQ_TYPE_EDGE_FALLING &&
> + altera_gc->interrupt_trigger == IRQ_TYPE_EDGE_FALLING)
> + return 0;
> + else if (type == IRQ_TYPE_EDGE_BOTH &&
> + altera_gc->interrupt_trigger == IRQ_TYPE_EDGE_BOTH)
> + return 0;
> + }
There is something very convoluted about this non-special else clause.
It can be just a else if like all the others, right?
> +static struct irq_chip altera_irq_chip = {
> + .name = "altera-gpio",
> + .irq_mask = altera_gpio_irq_mask,
> + .irq_unmask = altera_gpio_irq_unmask,
> + .irq_set_type = altera_gpio_irq_set_type,
> +};
We have added new APIs to flag GPIO lines as IRQs.
Please implement this in the .irq_startup()/.irq_shutdown()
callbacks in accordance with the style of
e.g. this patch:
http://marc.info/?l=linux-gpio&m=138571851304612&w=2
Note that you need to call unmask/mask from these callbacks.
> +static int altera_gpio_get(struct gpio_chip *gc, unsigned offset)
> +{
> + struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
> +
> + return (readl(mm_gc->regs + ALTERA_GPIO_DATA) >> offset) & 1;
Use this style to clamp a bit to {0,1}:
return !!(readl(mm_gc->regs + ALTERA_GPIO_DATA) & BIT(offset));
> +static void altera_gpio_set(struct gpio_chip *gc, unsigned offset, int value)
> +{
> + struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
> + struct altera_gpio_chip *chip = container_of(mm_gc,
> + struct altera_gpio_chip, mmchip);
> + unsigned long flags;
> + unsigned int data_reg;
> +
> + spin_lock_irqsave(&chip->gpio_lock, flags);
> + data_reg = readl(mm_gc->regs + ALTERA_GPIO_DATA);
> + data_reg = (data_reg & ~BIT(offset)) | (value << offset);
This has the same arithmetic effect but is easier to read:
if (value)
data_reg |= BIT(offset);
else
data_reg &= ~BIT(offset);
> +static int altera_gpio_direction_output(struct gpio_chip *gc,
> + unsigned offset, int value)
> +{
> + struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
> + struct altera_gpio_chip *chip = container_of(mm_gc,
> + struct altera_gpio_chip, mmchip);
> + unsigned long flags;
> + unsigned int data_reg, gpio_ddr;
> +
> + spin_lock_irqsave(&chip->gpio_lock, flags);
> + /* Sets the GPIO value */
> + data_reg = readl(mm_gc->regs + ALTERA_GPIO_DATA);
> + data_reg = (data_reg & ~BIT(offset)) | (value << offset);
Same comment. As for set().
> +static int altera_gpio_to_irq(struct gpio_chip *gc, unsigned offset)
> +{
> + struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
> + struct altera_gpio_chip *altera_gc = container_of(mm_gc,
> + struct altera_gpio_chip, mmchip);
> +
> + if (!altera_gc->irq)
> + return -ENXIO;
> + if (offset < altera_gc->mmchip.gc.ngpio)
> + return irq_create_mapping(altera_gc->irq, offset);
Recently we have established that you should call irq_create_mapping()
for all valid IRQs in probe() and just use irq_find_mapping() here,
as it is valid semantics to use an IRQ in an irq_chip without converting
it from a GPIO line first.
This is because in DT use cases you may use an interrupt from an
interrupt-controller without first calling gpio_to_irq() and then this
explodes.
So please fix this...
> +static void altera_gpio_irq_handler(unsigned int irq, struct irq_desc *desc)
> +{
> + struct altera_gpio_chip *altera_gc = irq_desc_get_handler_data(desc);
> + struct irq_chip *chip = irq_desc_get_chip(desc);
> + struct of_mm_gpio_chip *mm_gc = &altera_gc->mmchip;
> + unsigned long status;
> +
> + int i;
> + chip->irq_mask(&desc->irq_data);
Why do you call mask()/unmask()?
All interrupt handlers are called with interrupts disabled, I bet
you can just delete these two calls.
> +
> + /* Handling for level trigger and edge trigger is different */
> + if (altera_gc->interrupt_trigger == IRQ_TYPE_LEVEL_HIGH) {
> + status = readl_relaxed(mm_gc->regs + ALTERA_GPIO_DATA);
> + status &= readl_relaxed(mm_gc->regs + ALTERA_GPIO_IRQ_MASK);
> +
> + for (i = 0; i < mm_gc->gc.ngpio; i++) {
> + if (BIT(i) & status) {
> + generic_handle_irq(irq_linear_revmap(
> + altera_gc->irq, i));
Why do you do it like that?
What us wrong with this:
generic_handle_irq(irq_find_mapping(altera_gc->domain, i))
?
> + }
> + }
> + } else {
> + while ((status =
> + (readl_relaxed(mm_gc->regs + ALTERA_GPIO_EDGE_CAP) &
> + readl_relaxed(mm_gc->regs + ALTERA_GPIO_IRQ_MASK)))) {
> + writel_relaxed(status,
> + mm_gc->regs + ALTERA_GPIO_EDGE_CAP);
> + for (i = 0; i < mm_gc->gc.ngpio; i++) {
> + if (BIT(i) & status) {
> + generic_handle_irq(irq_linear_revmap(
> + altera_gc->irq, i));
Dito.
> + }
> + }
> + }
> + }
> +
> + chip->irq_eoi(irq_desc_get_irq_data(desc));
I don't think you should call that directly either. Why do you
do this?
> + chip->irq_unmask(&desc->irq_data);
See above about mask/unmask.
> +static int altera_gpio_irq_map(struct irq_domain *h, unsigned int virq,
> + irq_hw_number_t hw_irq_num)
> +{
> + irq_set_chip_data(virq, h->host_data);
> + irq_set_chip_and_handler(virq, &altera_irq_chip, handle_level_irq);
> + irq_set_irq_type(virq, IRQ_TYPE_NONE);
> +
> + return 0;
> +}
Rename the argument "virq" to just irq. These IRQs are not
really virtual any more than any other Linux IRQ.
(...)
> + altera_gc->irq = irq_domain_add_linear(node, altera_gc->mmchip.gc.ngpio,
> + &altera_gpio_irq_ops, altera_gc);
Here you should call irq_create_map() for all valid IRQs.
> +static int altera_gpio_remove(struct platform_device *pdev)
> +{
> + unsigned int irq, i;
> + int status;
> + struct altera_gpio_chip *altera_gc = platform_get_drvdata(pdev);
> +
> + status = gpiochip_remove(&altera_gc->mmchip.gc);
> +
> + if (status < 0)
> + return status;
> +
> + if (altera_gc->irq) {
> + irq_dispose_mapping(altera_gc->mapped_irq);
> +
> + for (i = 0; i < altera_gc->mmchip.gc.ngpio; i++) {
> + irq = irq_find_mapping(altera_gc->irq, i);
> + if (irq > 0)
> + irq_dispose_mapping(irq);
> + }
Look, you're already disposing all mappings properly :-)
Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
