Hi, I have a few comments.
On Thu, Jun 06, 2013 at 12:01:25PM +0100, Chander Kashyap wrote: > Extend the local timer interrupt support for handling four local timers. Is this the maximum number of CPUs the MCT could theoretically support? > > Signed-off-by: Chander Kashyap <[email protected]> > --- > drivers/clocksource/exynos_mct.c | 33 ++++++++++++++++++++++++++++++--- > 1 file changed, 30 insertions(+), 3 deletions(-) > > diff --git a/drivers/clocksource/exynos_mct.c > b/drivers/clocksource/exynos_mct.c > index 662fcc0..6af17d4 100644 > --- a/drivers/clocksource/exynos_mct.c > +++ b/drivers/clocksource/exynos_mct.c > @@ -412,6 +412,18 @@ static struct irqaction mct_tick1_event_irq = { > .handler = exynos4_mct_tick_isr, > }; > > +static struct irqaction mct_tick2_event_irq = { > + .name = "mct_tick2_irq", > + .flags = IRQF_TIMER | IRQF_NOBALANCING, > + .handler = exynos4_mct_tick_isr, > +}; > + > +static struct irqaction mct_tick3_event_irq = { > + .name = "mct_tick3_irq", > + .flags = IRQF_TIMER | IRQF_NOBALANCING, > + .handler = exynos4_mct_tick_isr, > +}; > + Is there any reason you can't use {request,free}_irq? > static int __cpuinit exynos4_local_timer_setup(struct clock_event_device > *evt) > { > struct mct_clock_event_device *mevt; > @@ -439,11 +451,21 @@ static int __cpuinit exynos4_local_timer_setup(struct > clock_event_device *evt) > mct_tick0_event_irq.dev_id = mevt; > evt->irq = mct_irqs[MCT_L0_IRQ]; > setup_irq(evt->irq, &mct_tick0_event_irq); > - } else { > + } else if (cpu == 1) { > mct_tick1_event_irq.dev_id = mevt; > evt->irq = mct_irqs[MCT_L1_IRQ]; > setup_irq(evt->irq, &mct_tick1_event_irq); > irq_set_affinity(evt->irq, cpumask_of(1)); > + } else if (cpu == 2) { > + mct_tick2_event_irq.dev_id = mevt; > + evt->irq = mct_irqs[MCT_L2_IRQ]; > + setup_irq(evt->irq, &mct_tick2_event_irq); > + irq_set_affinity(evt->irq, cpumask_of(2)); > + } else if (cpu == 3) { > + mct_tick3_event_irq.dev_id = mevt; > + evt->irq = mct_irqs[MCT_L3_IRQ]; > + setup_irq(evt->irq, &mct_tick3_event_irq); > + irq_set_affinity(evt->irq, cpumask_of(3)); This doesn't seem good to me. You're duplicating the logic for each CPU. Can you not figure out which values you need based on the smp_processor_id (or even better, the *evt) without requiring a separate branch for each CPU? > } > } else { > enable_percpu_irq(mct_irqs[MCT_L0_IRQ], 0); > @@ -456,11 +478,16 @@ static void exynos4_local_timer_stop(struct > clock_event_device *evt) > { > unsigned int cpu = smp_processor_id(); > evt->set_mode(CLOCK_EVT_MODE_UNUSED, evt); > - if (mct_int_type == MCT_INT_SPI) > + if (mct_int_type == MCT_INT_SPI) { > if (cpu == 0) > remove_irq(evt->irq, &mct_tick0_event_irq); > - else > + else if (cpu == 1) > remove_irq(evt->irq, &mct_tick1_event_irq); > + else if (cpu == 2) > + remove_irq(evt->irq, &mct_tick2_event_irq); > + else if (cpu == 3) > + remove_irq(evt->irq, &mct_tick3_event_irq); > + } Again, I don't think each CPU should be special-cased. If you used {request,free}_irq this would be simpler. Thanks, Mark. -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
