Messages in this thread | | | Date | Fri, 5 Sep 2008 20:11:11 +0200 | From | Ingo Molnar <> | Subject | Re: [patch 3/3] x86: io-apic - code style cleaning for setup_IO_APIC_irqs |
| |
* Cyrill Gorcunov <gorcunov@gmail.com> wrote:
> [Ingo Molnar - Fri, Sep 05, 2008 at 10:04:47AM +0200] > | > | * Cyrill Gorcunov <gorcunov@gmail.com> wrote: > | > | > Use a nested level for 'for' cycle and break long lines. > | > For apic_print we should countinue using KERNEL_DEBUG if > | > we have started to. > | > | > @@ -1521,32 +1521,35 @@ static void __init setup_IO_APIC_irqs(vo > | > apic_printk(APIC_VERBOSE, KERN_DEBUG "init IO_APIC IRQs\n"); > | > > | > for (apic = 0; apic < nr_ioapics; apic++) { > | > - for (pin = 0; pin < nr_ioapic_registers[apic]; pin++) { > | > + for (pin = 0; pin < nr_ioapic_registers[apic]; pin++) { > | > > | > + idx = find_irq_entry(apic, pin, mp_INT); > | > + if (idx == -1) { > | > | hm, i dont really like the super-deep nesting we do here. Could you > | please split out the iterator into a separate function? That makes the > | code a lot easier to understand and saves two extra tabs as well for > | those ugly-looking printk lines. > | > | Ingo > | > > You know it seems we use such a 'cycle on every pin on io-apics' > in several places for now: > > io_apic.c > --------- > clear_IO_APIC > save_mask_IO_APIC_setup > restore_IO_APIC_setup > IO_APIC_irq_trigger > setup_IO_APIC_irqs > > I've made a one-line macro for this (like for_all_ioapics_pins) > _but_ it looks much more ugly then this two nested for(;;) :) > > If you meant me to make a separate iterator over the pins I think > it will not help a lot - this function is simple enought so the only > problem is too-long-printk-form - maybe just print them on separated > lines instead of tracking apicids? Or it was made in a sake to not > scroll screen too much?
hm, by iterator i meant the body itself. I.e. something like:
static void __init setup_IO_APIC_irqs(void) { int apic, pin, notcon = 1;
apic_printk(APIC_VERBOSE, KERN_DEBUG "init IO_APIC IRQs\n");
for (apic = 0; apic < nr_ioapics; apic++) for (pin = 0; pin < nr_ioapic_registers[apic]; pin++) notcon = setup_ioapic_irq(apic, pin, notcon);
if (!notcon) apic_printk(APIC_VERBOSE, " not connected.\n"); }
this looks quite a bit cleaner, doesnt it? We lose the 'idx' and 'irq' variables and we lose the curly braces as well. The flow looks a lot more trivial. And the new setup_ioapic_irq() function will be simpler as well - it will only have 'idx' and 'irq' as a local variable, the rest comes in as a parameter. It can 'return notcon' instead of 'continue'. And it will be 2 levels of tabs aligned to the left, as an added bonus.
Hm?
Ingo
| |