Dave,
I'll incorporate the comments and re-submit the patch.
Thanks, Sudhakar
>
> Quick reactions to this version ...
>
> Almost all of these changes flow directly from the interface
> changes which I've excerpted below, which will help make this
> response much shorter than the patch! That's a nice feature
> of a patch being reviewed.
>
> Still ... 40 KBytes is big. This patch *might* split naturally
> into (a) internal restructuring around a "struct edma", (b) the
> interface changes visible to DMA clients, and (c) using those
> changes to work with multiple such structs. Smaller patches
> are easier to test and review; plus, the MMC and ASoC patches
> would depend only on (b).
>
> I'd structure the alloc_channel() a bit differently. You made
> it loop through each channel controller ... but that's only
> needed when it's not allocating a specific channel. Maybe the
> thing to do is more or less
>
> if (search)
> call helper to allocate some channel on some CC;
> else
> allocate specified channel on specified CC;
> if (error)
> return failure code;
> init
> return success
>
> Plus, both your alloc_channel() and alloc_slot() are discarding the
> controller they may have been told to use ... alloc_slot() should
>
> if (slot >= 0) {
> ctlr = EDMA_CTLR(slot);
> slot = EDMA_CHAN(slot);
> }
>
>
>
> On Friday 17 April 2009, Sudhakar Rajashekhara wrote:
> > --- a/arch/arm/mach-davinci/include/mach/edma.h
> > +++ b/arch/arm/mach-davinci/include/mach/edma.h
> > @@ -58,6 +58,11 @@
> > #ifndef EDMA_H_
> > #define EDMA_H_
> >
> > +#define EDMA_MAX_DMACH 64
> > +#define EDMA_MAX_PARAMENTRY 512
> > +#define EDMA_MAX_EVQUE 2 /* FIXME too small */
> > +#define EDMA_MAX_CC 2
> > +
>
> Those shouldn't need to move outside of the dma.c source file;
> there's no reason a DMA client should care about these values.
>
>
> > /* PaRAM slots are laid out like this */
> > struct edmacc_param {
> > unsigned int opt;
> > @@ -171,6 +176,10 @@ enum sync_dimension {
> > ABSYNC = 1
> > };
> >
> > +#define EDMA_CTLR_CHAN(ctlr, chan) (((ctlr) << 16) | (chan))
> > +#define EDMA_CTLR(i) ((i) >> 16)
> > +#define EDMA_CHAN(i) ((i) & 0xffff)
> > +
>
> I can't quite think of a word that can mean "channel or slot"
> and is already used in the DMA context. So all I'll do right
> now is note that "CHAN" will be misleading when these are
> used with slot identifiers, so a better tag would be nice.
>
>
> > #define EDMA_CHANNEL_ANY -1 /* for edma_alloc_channel()
*/
> > #define EDMA_SLOT_ANY -1 /* for edma_alloc_slot() */
> >
> > @@ -181,7 +190,7 @@ int edma_alloc_channel(int channel,
> > void edma_free_channel(unsigned channel);
> >
> > /* alloc/free parameter RAM slots */
> > -int edma_alloc_slot(int slot);
> > +int edma_alloc_slot(unsigned ctlr, int slot);
>
> Right, that's about all that DMA clients should need to see change.
> One function, and those three macros. ;)
>
>
> > void edma_free_slot(unsigned slot);
> >
> > /* calls that operate on part of a parameter RAM slot */
> > @@ -221,9 +230,34 @@ struct edma_soc_info {
> > unsigned n_region;
> > unsigned n_slot;
> > unsigned n_tc;
> > + unsigned n_cc;
> >
> > /* list of channels with no even trigger; terminated by "-1" */
> > const s8 *noevent;
> > };
> >
> > +struct edma {
>
> I had in mind that this structure would be internal to
> the dma.c source file because, again, clients have no
> need to see these details.
>
>
> > + /* how many dma resources of each type */
> > + unsigned num_channels;
> > + unsigned num_region;
> > + unsigned num_slots;
> > + unsigned num_tc;
> > + unsigned num_cc;
> > +
> > + /* list of channels with no even trigger; terminated by "-1" */
> > + const s8 *noevent;
> > +
> > + /* The edma_inuse bit for each PaRAM slot is clear unless the
> > + * channel is in use ... by ARM or DSP, for QDMA, or whatever.
> > + */
> > + DECLARE_BITMAP(edma_inuse, EDMA_MAX_PARAMENTRY);
> > +
> > +
> > + /* The edma_noevent bit for each channel is clear unless
> > + * it doesn't trigger DMA events on this platform. It uses a
> > + * bit of SOC-specific initialization code.
> > + */
> > + DECLARE_BITMAP(edma_noevent, EDMA_MAX_DMACH);
>
> I would think that the interrupt callback data would need to be
> stored per-CC as well. You still have a global-to-the-driver
> "struct interrupt_data { ... } intr_data[EDMA_MAX_DMACH];", so
> there's no way to distinguish e.g. the channel 42 IRQ from two
> different channel controllers ...
>
>
> > +};
> > +
> > #endif
>
>
>
_______________________________________________
Davinci-linux-open-source mailing list
Davinci-linux-open-source@linux.davincidsp.com
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
