On Thu, 2009-04-09 at 12:07 +0200, Manuel Lauss wrote:
> Hi Kevin!
>
> Some major nits:
>
> > --- /dev/null
> > +++ b/arch/mips/alchemy/common/gpio_int.c
> > +static struct irq_chip gpio_int_irq_type = {
> > + .name = "Au GPIO/INT",
> > + .ack = gpio_int_ack,
> > + .mask = gpio_int_mask,
> > + .unmask = gpio_int_unmask,
> > + .mask_ack = gpio_int_mask_ack
> > +void __init arch_init_irq(void)
> > +{
> [...]
> > + for (i = 0; i < nr_basic_irqs; ++i) {
> > + printk(KERN_DEBUG "Initializing IRQ %d\n",
> > + basic_irqs[i].number);
> > + set_pin_cfg(&basic_irqs[i]);
> > + if (basic_irqs[i].intcfg == LEVEL_LOW)
> > + set_irq_chip_and_handler_name(
> > + basic_irqs[i].number + GPINT_LINUX_IRQ_OFFSET,
> > + &gpio_int_irq_type,
> > + handle_level_irq,
> > + "lowlevel");
> > + else if (basic_irqs[i].intcfg == LEVEL_HIGH)
> > + set_irq_chip_and_handler_name(
> > + basic_irqs[i].number + GPINT_LINUX_IRQ_OFFSET,
> > + &gpio_int_irq_type,
> > + handle_level_irq,
> > + "highlevel");
> > + else if (basic_irqs[i].intcfg == FALLING)
> > + set_irq_chip_and_handler_name(
> > + basic_irqs[i].number + GPINT_LINUX_IRQ_OFFSET,
> > + &gpio_int_irq_type,
> > + handle_edge_irq,
> > + "fallingedge");
> > + else if (basic_irqs[i].intcfg == RISING)
> > + set_irq_chip_and_handler_name(
> > + basic_irqs[i].number + GPINT_LINUX_IRQ_OFFSET,
> > + &gpio_int_irq_type,
> > + handle_edge_irq,
> > + "risingedge");
> > + else if (basic_irqs[i].intcfg == ANY_CHANGE)
> > + set_irq_chip_and_handler_name(
> > + basic_irqs[i].number + GPINT_LINUX_IRQ_OFFSET,
> > + &gpio_int_irq_type,
> > + handle_edge_irq,
> > + "bothedge");
> > + else
> > + set_irq_chip(
> > + basic_irqs[i].number + GPINT_LINUX_IRQ_OFFSET,
> > + &gpio_int_irq_type);
> > + }
>
> Please please move this to a irq_chip.set_irq_type hook, and replace
> LEVEL_LOW and friends with linux' IRQ_TYPE_LEVEL_LOW... constants
> (So one can pass IRQ flags via platform resource information).
>
>
> > diff --git a/arch/mips/alchemy/devboards/cascade_irq.c b/arch/mips/alchemy/devboards/cascade_irq.c
> > + * The following must be declared/defined in an included file:
> > + * - volatile struct bcsr_regs (declared)
> > + * (which much include fields int_status, intset_mask, intclr_mask, intset,
> > + * and intclr)
> > + * - volatile struct bcsr_regs *const bcsr (defined)
> > + * - CASCADE_IRQ_MIN
> > + * - CASCADE_IRQ_MAX
> > + * - CASCADE_IRQ_TYPE_STRING
> > + * - CASCADE_IRQ (System IRQ to which the cascade is connected)
> > + */
> > +
> > +void __init board_init_irq(void);
> > +
> > +irqreturn_t cascade_handler(int irq, void *dev_id)
> > +{
> > + u16 int_status = au_ioread16(&db_bcsr->int_status);
> > + int irq_in_service;
> > +
> > + au_iowrite16(int_status, &db_bcsr->int_status);
> > + for ( ; int_status; int_status &= int_status - 1) {
> > + irq_in_service = CASCADE_IRQ_MIN + __ffs(int_status);
> > + db_set_hex((u8)(irq_in_service));
> > + do_IRQ(irq_in_service);
>
> generic_handle_irq() please,
>
>
> > +static unsigned int cascade_startup(unsigned int irq)
> > +{
> > + int retval = 0;
> > +
> > + mutex_lock(&cascade_use_count_mutex);
> > + ++cascade_use_count;
> > + if (cascade_use_count == 1)
> > + retval = request_irq(CASCADE_IRQ,
> > + &cascade_handler, 0, "Cascade",
> > + &cascade_handler);
>
> Use "set_irq_chained_handler" after registering the cascades and the
> startup/shutdown hooks can go away.
>
> Also, "cascade" is too generic, maybe call it "db1300cascade". The
> Db1200 too has a cascade but works slightly different (I even doubt it
> makes sense to place this code outside the directory. As it currently
> is, it's not generic enough to be usable by the Db1200/Pb1200; and
> the code is rather tiny anyway).
>
To address all of your comments, I wrote this code some time ago and
have not had time to digest some of the changes that have been posted to
the DB1200 interrupt code since then. I have every intention of finding
some time down the line to revisit this and merge it with the DB1200
cascade code. The controllers are very similar - in fact in my local
tree I am using this same code on both platforms.
In the interest of time and schedule, I was hoping to have this version
included in 2.6.30 so that I had something to build on for future
updates and something to release other driver and peripheral patches
against.
> (Off topic: why the 2-stage mask/unmask system [the 'enable' and
> 'mask' bits] I didn't make sense to me on the DB1200 either...)
>
I'm not really sure either but I can ask the folks responsible for the
CPLD. My guess is that it is just because traditionally interrupt
controllers have both enables and masks and that's how it was written.
>
> > +/*
> > + * Set the GPIO to the specified value. The value must be 0 or 1. Any other
> > + * value results in a no-op.
> > + *
> > + * This call will implicitly reconfigure the pin to be a GPIO if it is
> > + * configured as a device pin.
> > + */
> > +void set_gpio(u8 gpio, u8 value);
> > +
> > +/*
> > + * Get the value of any GPIO pin (including those controlled by devices).
> > + *
> > + * This will not change the pin configuration
> > + */
> > +u8 get_gpio(u8 gpio);
> > +
> > +#endif /* _GPIO_INT_H */
> > +
>
> no support for linux' GPIO framework?
Again, at the time I was unaware of any GPIO standards. I will probably
revise this to use gpiolib or whaterver down the line.
>
--
Kevin Hickey
Alchemy Solutions
RMI Corporation
khickey@xxxxxxxxxxx
P: 512.691.8044
