Re: [PATCH v5 5/6] gpio: Add gpio driver support for ThunderX and OCTEON-TX

[Linus Walleij <linus.walleij@xxxxxxxxxx>]

On Wed, Mar 1, 2017 at 2:48 AM, David Daney <david.daney@xxxxxxxxxx> wrote:

> Cavium ThunderX and OCTEON-TX are arm64 based SoCs.  Add driver for
> the on-chip GPIO pins.
>
> Signed-off-by: David Daney <david.daney@xxxxxxxxxx>

I guess this can't be merged until the corresponding IRQ changes
are merged. It can be applied in the irqchip tree or next merge
window or they can give me an immutable branch to pull to
the GPIO subsystem or whatever works.

Some pretty minor things remain.

> +#include <linux/gpio/driver.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/irq.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/pci.h>
> +#include <linux/spinlock.h>

I think you should explicitly include <linux/bitops.h>

Other than that it is all fine.

> +/*
> + * Interrupts are chained from underlying MSI-X vectors.  We have
> + * these irq_chip functions to be able to handle level triggering
> + * semantics and other acknowledgment tasks associated with the GPIO
> + * mechanism.
> + */
> +static struct irq_chip thunderx_gpio_irq_chip = {
> +       .name                   = "GPIO",
> +       .irq_enable             = thunderx_gpio_irq_enable,
> +       .irq_disable            = thunderx_gpio_irq_disable,
> +       .irq_ack                = thunderx_gpio_irq_ack,
> +       .irq_mask               = thunderx_gpio_irq_mask,
> +       .irq_mask_ack           = thunderx_gpio_irq_mask_ack,
> +       .irq_unmask             = thunderx_gpio_irq_unmask,
> +       .irq_eoi                = irq_chip_eoi_parent,
> +       .irq_set_affinity       = irq_chip_set_affinity_parent,
> +       .irq_set_type           = thunderx_gpio_irq_set_type,
> +
> +       .flags                  = IRQCHIP_SET_TYPE_MASKED
> +};

I think you should implement .irq_request_resources() and
.irq_release_resources() calling
gpiochip_lock_as_irq(chip, d->hwirq)
and gpiochip_unlock_as_irq(chip, d->hwirq) respectively,
much like gpiochip_irq_reqres() and gpiochip_irq_relres()
in the gpiolib.

This helps the kernel knowing that the lines are taken for
interrupts and must be inputs.

> +static int thunderx_gpio_irq_map(struct irq_domain *d, unsigned int irq,
> +                                irq_hw_number_t hwirq)
> +{
> +       irq_set_irq_type(irq, IRQ_TYPE_LEVEL_LOW);

I would prefer that you just write the right state into the hardware
if that is what you want to do. Normally the consumer specifies
trigger type.

But I'm uncertain, maybe this is allright with the IRQ maintainers.

Apart from these nitpicks it is a beautiful driver.

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