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

[Tero Kristo <t-kristo@ti.com>]

Hey Paul,

I've been looking at this now and got one question below. Otherwise your
comments look okay to me and I can work with those.

On Fri, 2011-08-26 at 11:12 +0200, Paul Walmsley wrote:
> Hello Tero,
> 
> a few comments on this patch:
> 
> On Mon, 25 Jul 2011, Tero Kristo wrote:
> 
> > Introduce a chained interrupt handler mechanism for the PRCM
> > interrupt, so that individual PRCM event can cleanly be handled by
> > handlers in separate drivers. We do this by introducing PRCM event
> > names, which are then matched to the particular PRCM interrupt bit
> > depending on the specific OMAP SoC being used.
> > 
> > arch/arm/mach-omap2/prcm.c implements the chained interrupt mechanism
> > itself, with SoC specific support / init structure defined in
> > arch/arm/mach-omap2/prm2xxx_3xxx.c and arch/arm/mach-omap2/prm4xxx.c
> > respectively. At initialization time, the set of PRCM events is filtered
> > against the SoC on which we are running, keeping only the ones that are
> > actually useful. All the logic is written to be generic with regard to
> > OMAP3/OMAP4, even though OMAP3 has single PRCM event registers and OMAP4
> > has two PRCM event registers.
> 
> Looking over this patch, it seems that this functionality should be
> part of a PRM device driver.  That would allow the separation of the
> SoC-specific data from the code, so there wouldn't be a need to embed
> the OMAP_PRCM_IRQ data in the driver code.  Rather, that data could go
> into the dev_attr data for the PRM hwmod.  That avoids putting
> SoC-specific data in driver code, allows the removal of
> omap[34]_prcm_irq_setup(), and should also remove the dependency on
> omap_chip.
> 
> Similarly, OMAP_PRCM_MAX_NR_PENDING_REG and OMAP_PRCM_NR_IRQS should
> be defined somewhere SoC-specific.  I'd suggest defining those in the
> hwmod dev_attr data.  That way that file won't need to be patched if
> those constants need change in the future.  Unfortunately, doing this
> in a clean way will probably mean that the variables that are
> allocated via these constants will need to be allocated and freed
> dynamically.
> 
> What I'd suggest is to create a short series that:
> 
> 1. adds PRM hwmod data for OMAP2430+ platforms

How should this be done? It believe all the data in the hwmods should be
autogenerated somehow... should I just make a temporary hack patch for
one platform that could be then autogenerated by someone for all omap
platforms?


> 
> 2. adds a basic PRM device driver skeleton in a directory such as
>    drivers/power -- (I'm not convinced that this is the right place,
>    in the end; but seems like a good place to start)
> 
> 3. creates the chained interrupt handler in the PRM device driver,
>    and removes the old PRCM interrupt handler from pm34xx.c 
> 
> ...
> 
> A few other relatively minor comments:
> 
> - Probably omap_prcm_irq_cleanup() shouldn't be called from pm34xx.c,
>   since other code outside of pm34xx.c might wish to use the PRCM
>   interrupt, even in the (admittedly unlikely) circumstance that some
>   of the code in pm34xx.c fails?
> 
> - It would be good to document struct omap_prcm_irq via KernelDoc,
>   rather than inline comments
>   (Documentation/kernel-doc-nano-HOWTO.txt).  It would be ideal if the
>   patch's function documentation followed the same standard.
> 
> 
> - Paul



Texas Instruments Oy, Tekniikantie 12, 02150 Espoo. Y-tunnus: 0115040-6. Kotipaikka: Helsinki