Dave,
Thanks for review comments.
>
> On Friday 24 April 2009, Sudhakar Rajashekhara wrote:
> > +struct edma {
> > + /* 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);
> > +
> > + unsigned irq_start[EDMA_MAX_CC];
> > + unsigned irq_end[EDMA_MAX_CC];
>
> I don't quite see why those are arras... surely they should
> be single values?
I agree with you. I overlooked them when I moved these arrays
into the structure. They should be single values.
> And I'd have to look in more detail, to
> answer this, but *why* have irq_end when we know that it's
> going to be "irq_start + num_channels"?
I am using "irq_end" to know the last interrupt number which is
being used for EDMA (CCERR_INT is this case). irq_start and
irq_end are required inside the interrupt handler to know the CC
instance for which the interrupt has come.
>
>
> > +};
> > +
> > +struct edma edma_info[EDMA_MAX_CC];
>
> Normally I'd expect that should be an array of *pointers* that
> get allocated in probe(), but that may not be a big deal if
> it's just the difference between one and two such structs.
But I feel it's good to implement this. Will incorporate in the
next set of patches.
>
>
> > static struct dma_interrupt_data {
> > void (*callback)(unsigned channel, unsigned short ch_status,
> void *data);
> > void *data;
> > } intr_data[EDMA_MAX_DMACH];
>
> And I recall pointing out before that this is incorrect.
> EDMA_MAX_DMACH will be 64 (per channel controller), so
> if there are two such controllers ... that's not enough.
>
> Either that array should be part of the "struct edma" or
> it should be size "EDMA_MAX_DMACH * EDMA_MAX_CC". I'd
> tend to go for the former.
OK. Will take care of it when I submit the next set.
Regards, Sudhakar
_______________________________________________
Davinci-linux-open-source mailing list
[email protected]
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
