Hi,
On Tue, Jul 26, 2011 at 01:33:35PM +0300, Tero Kristo wrote:
> > > + while (1) {
> > > + unsigned int virtirq;
> > > +
> > > + chip->irq_ack(&desc->irq_data);
> > > +
> > > + memset(pending, 0, sizeof(pending));
> > > + irq_setup->pending_events(pending);
> > > +
> > > + /* No bit set, then all IRQs are handled */
> > > + if (find_first_bit(pending, OMAP_PRCM_NR_IRQS)
> > > + >= OMAP_PRCM_NR_IRQS) {
> > > + chip->irq_unmask(&desc->irq_data);
> > > + break;
> > > + }
> > > +
> > > + /*
> > > + * Loop on all currently pending irqs so that new irqs
> > > + * cannot starve previously pending irqs
> > > + */
> > > + for_each_set_bit(virtirq, pending, OMAP_PRCM_NR_IRQS)
> > > + generic_handle_irq(irq_setup->base_irq + virtirq);
> > > +
> > > + chip->irq_unmask(&desc->irq_data);
> >
> > can't the IRQ subsystem handle this for you ? I was expecting it would
> > call irq_ack() and irq_unmask() automatically and you wouldn't have to
> > do it yourself. Maybe Thomas can clear this out ? Thomas, should we call
> > ->irq_ack() ->irq_mask ourselves here ?
>
> These are needed, as we are using a handle and a level interrupt. If you
> leave out the ack, we will return to the handler immediately as nobody
> else acks it. If you leave out the unmask, we will only ever get 1
> interrupt and no more.
but that's the thing, I guess IRQ subsystem can handle this
automatically. At least from what I remember. That's how I converted the
retu driver, for instance, and I checked that IRQ subsystem calls
->irq_ack() and ->irq_mask/unmask() for me...
Maybe the irq_gc stuff is different, dunno.
> > if you make this a platform_driver, there would be no need for this
> > trickery. You could pass this as driver data. Something like:
> >
> >
> > struct omap_prcm_irq_setup omap3_prcm_irq_setup = {
> > .ack = (u32)OMAP3430_PRM_IRQSTATUS_MPU,
> > .mask = (u32)OMAP3430_PRM_IRQENABLE_MPU,
> > .pending_events = omap3_prm_pending_events,
> > .irq = INT_34XX_PRCM_MPU_IRQ,
> > };
> >
> > struct const struct platform_device_id prcm_id_table[] __devinitconst =
> > {
> > {
> > .name = "omap3-prcm",
> > .driver_data = &omap3_prcm_irq_setup,
> > },
> > {
> > .name = "omap4-prcm",
> > .driver_data = &omap4_prcm_irq_setup,
> > },
> > };
> > MODULE_DEVICE_TABLE(platform, prcm_id_table);
> >
> > static struct platform_driver prcm_driver = {
> > .probe = prcm_probe,
> > .remove = __devexit_p(prcm_remove),
> > .driver = {
> > .name = "prcm",
> > .pm = DEV_PM_OPS,
> > },
> > .id_table = &prcm_id_table,
> > };
> >
> > or something similar. Then on probe you can make a copy of irq_setup to
> > your driver's context structure, or only use temporarily to initialize
> > some fields and so on...
> >
>
> Hmm, this might be useful, however you would still need a way to
> register the driver from somewhere, and you would essentially end up
> with omap_chip version check somewhere. The reason for all this trickery
> is that the omap chip version detection is heavily frowned upon right
> now... it would be much cleaner if there was an accepted way of doing
> this already in place.
wouldn't hwmod be able to handle this for you ? I mean, it's just the
driver name that has to change, rather than exporting a few functions.
--
balbi
Attachment:
signature.asc
Description: Digital signature
