Re: [PATCH] added API: spi_message_compile/optimize/hint/...

[Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 3062 bytes --]

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]