"Nori, Sekhar" <[email protected]> writes: > On Wed, Mar 17, 2010 at 20:21:21, Kevin Hilman wrote: >> Sekhar Nori <[email protected]> writes: >> >> > From: Sudhakar Rajashekhara <[email protected]> >> > >> > The drivers on da8xx/omapl EVMs do not utilize all the channels >> > and slots provided by EDMA. Some of these are better utilitzed by >> > the DSP on the SoC for speeding up codec operations. >> > >> > Reserve these channels/slots for the DSP. >> > >> > Signed-off-by: Sudhakar Rajashekhara <[email protected]> >> > Signed-off-by: Sekhar Nori <[email protected]> >> > --- >> > arch/arm/mach-davinci/board-da830-evm.c | 32 ++++++++++++++++++- >> > arch/arm/mach-davinci/board-da850-evm.c | 48 >> > +++++++++++++++++++++++++++- >> > arch/arm/mach-davinci/devices-da8xx.c | 21 +++++++----- >> > arch/arm/mach-davinci/include/mach/da8xx.h | 3 +- >> > 4 files changed, 92 insertions(+), 12 deletions(-) >> > >> >> Thanks, I like this one better. Still a small problem though.. >> >> > +int __init da850_register_edma(struct edma_rsv_info *rsv) >> > +{ >> > + if (rsv) { >> > + da850_edma_info[0].rsv = &rsv[0]; >> > + da850_edma_info[1].rsv = &rsv[1]; >> > + } >> > + >> > + return platform_device_register(&da850_edma_device); >> >> What if the caller only has reserved chans/slots for controller 0? >> &rsv[1] will be an undefined pointer. >> >> I think you need some sort of terminator on the list passed in. >> so only edma_rsv_info pointers that are valid are passed along. >> > > Kevin, > > I am assuming since all the other platform data is defined for both > controllers, all the platform data should be consistent and provide > information on both controllers. > > In case the caller does not need reservation for second controller he > can use NULL initialization: > > +static struct edma_rsv_info da850_edma_rsv[] = { > + { > + .rsv_chans = da850_dma0_rsv_chans, > + .rsv_slots = da850_dma0_rsv_slots, > + }, > + { > + }, > +};
> Even currently, if devices-da8xx.c had defined two EDMA instances in resource > structure (da850_edma_resources), but populated da850_edma_info[] for only one > controller, the edma probe would go ahead and access info for second instance. And probably crash. I would call that a bug that needs a fix. > Is it necessary to check for platform data consistency in such a rigorous > manner > (since it is not really a 'user' input)? IMO, it is user input. And if we expect it to be straight forward for anyone to add support for their own board, we should try our best to make the handling of that board-specific data robust. At a minimum, to make this cleaner, the size of the *_edma_info structs (and rsv structs) should use a fixed array size for the max number of controllers for that SoC. Then checking for NULL before they are passed along should be done as well. In addition, after looking at this a little closer, the *_edma_info structs in devices-da8xx.c could be __initdata since they are copied during edma_probe() (should be done as a fix before this patch.) The same for the rsv structs in the board files. Kevin _______________________________________________ Davinci-linux-open-source mailing list [email protected] http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
