Re: [PATCH] gpio: tqmx86: Add GPIO driver for this IO controller

[Andrew Lunn <andrew@xxxxxxx>]

> 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.
> 
> > +#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

Hi Linus

I will look at GPIO_GENERIC, but these are not MMIO, registers, but
Intel IO registered, using inb() and outb().

> > +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 device has 4 GPO and 4 GPI. Only the GPI can generate interrupts.

> > +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?

Humm, i will have to reverse enginner this. I don't have a datasheet,
just the vendor code.

> > +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?

I don't actually know. I also don't know if i have a way to test this,
with the hardware i have. I've not yet even tested interrupts, they
are not too important for my use case.

Thanks for the comment

    Andrew