On Tue, Nov 19, 2013 at 07:33:14PM +0100, Martin Sperl wrote: > Here the patch for a spi_message_compile interface > (as discussed in a separate thread).
As well as the signoff please try to follow the other stuff in SubmittingPatches, both in terms of the formatting of the patch and in terms of things like coding style - checkpatch is a useful tool for checking this stuff. The shape of the patch looks good, but there's a few issues below. > Naming of the API is subject to discussion - instead of "compile" it could > also be "optimize" or "hint" or ... Hrm. I don't really like any of these options but can't think of anything else right now. Perhaps optimize is the best of the options here. > The patch creates: > * 3 additional variables in spi_message > * 2 additional function pointers in spi_master Can you keep the master interface in a separate patch please? I'm not sure if we want to do this or if we want to push more of the work to the core, especially given the desire to factor the generic bits of DMA work out of drivers, but the external interface seems very clear now so we should be able to get that merged more easily (and then things like the mcp2515 can start building on that). For the master interface I think I'd like to at least see a patch adding a driver using it go in along with the core changes. This is probably going to be a bit difficult right now given the situation with the Raspberry Pi kernels unfortunately. > Here some measurements using my "standard" CAN test: > * runs on Raspberry Pi without over-clocking > * optimized mcp2515 driver with 3 Message chunks and callbacks running > in listen only mode Is this device part of the vanilla board out of interest? > * kernel 3.10.16 (foundation based, as the vanilla RPI kernel still misses > dmaengine, so it is easier to develop against this kernel) Patches really need to be sent against the latest code for application - this is going to collide with the factoring of the validation out of the lock that I did too. Normally that should be git://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next though until the merge window is over that'll need to be topic/core. > +/** > + * spi_message_verify - verifies if the spi message is supported > + * by the bus-master > + * @spi: device with which data will be exchanged > + * @message: describes the data transfers, including completion callback > + * Context: any (irqs may be blocked, etc) > + * > + * This call may be used in_irq and other contexts which can't sleep, > + * as well as from task contexts which can sleep. > + */ > +extern int spi_message_verify(struct spi_device *spi, > + struct spi_message *message) This stuff does more than verify, it also initialises the message with defaults - if it were really a verify() then it'd be able to have the message passed as a const. > + if ((verify=spi_message_verify(spi,m))) > + return verify; Assign and check the result separately.
signature.asc
Description: Digital signature
