On Wed, Mar 17, 2010 at 22:48:46, Kevin Hilman wrote: > "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.
I think you mean max possible overall (EDMA_MAX_CC). Without that you would not be able to catch the number of resources vs size of platform data mismatches we talked about earlier. > Then checking for NULL > before they are passed along should be done as well. Okay, I will pass the *_edma_info structures as array of pointers. So, it is easy to bailout if number of info structures passed do not match the number of resources. > > 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. Okay, actually the whole EDMA platform device needs to be __initdata. Thanks, Sekhar _______________________________________________ Davinci-linux-open-source mailing list [email protected] http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
