Re: [PATCHv6 01/11] omap: prcm: switch to a chained IRQ handler mechanism

[Felipe Balbi <balbi@ti.com>]

[-- Attachment #1: Type: text/plain, Size: 3396 bytes --]

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 #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]