[PATCH 2/3] spi: add initial set of spi_transfer transformation methods

[Mark Brown]

On Mon, Nov 30, 2015 at 01:04:53PM +0000, kernel at martin.sperl.org wrote:
> From: Martin Sperl <kernel at martin.sperl.org>
> 
> added the following spi_transformation methods:
> * spi_split_transfers_first_page_len_not_aligned -
>   this splits spi_transfers that are not aligned on rx_buf/tx_buf
>   this will create 2 or 3 transfers, where the first 1/2 transfers are
>   just there to allow the last transfer to be fully aligned. These first
>   transfers will have a length less than alignment.
> * spi_split_transfers_maxsize
>   this splits the spi_transfer into multiple independent spi_transfers
>   all of which will be of size max_size or smaller.

Please split this up for review, as I'm fairly sure has been discussed
before smaller, more focused patches with clear changelogs describing
what they are suppose to do are much easier to review.  Start by adding
the framework changes, then add one transformation at once.  This is a
very big patch full of fiddly, unexplained code.

> To start these shall get used by the individual drivers in prepare_message,
> but some may get moved into spi-core with the correct parametrization in
> spi_master.

As discussed I'm not a big fan of this approach.

> +/* the spi_resource structure used */
> +struct spi_res_replaced_transfers {
> +	spi_res_release_t release;
> +	struct list_head replaced_transfers;
> +	int inserted;
> +	struct spi_transfer xfers[];
> +};

This quite clearly isn't a struct spi_resource, nor does it contain
one...

> +static void __spi_split_transfers_fixup_transfer_addr_and_len(
> +	struct spi_transfer *xfer, int count, int shift)
> +{
> +	int i;
> +
> +	/* fix up transfer length */
> +	xfer[0].len = shift;
> +
> +	/* shift all the addresses arround */
> +	for (i = 1; i < count; i++) {
> +		xfer[i].len -= shift;
> +		xfer[i].tx_buf += xfer[i].tx_buf ? shift : 0;
> +		xfer[i].rx_buf += xfer[i].rx_buf ? shift : 0;
> +		xfer[i].tx_dma += xfer[i].tx_dma ? shift : 0;
> +		xfer[i].rx_dma += xfer[i].rx_dma ? shift : 0;
> +	}
> +}

It's really not clear what this is supposed to do.  Among other things
"shift" appears to be misnamed and anything using tx_dma and rx_dma is
worrying.

I did read a bit further but like I said at the top this is a lot of
complicated code that's not split up enough to be clear.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-rpi-kernel/attachments/20151201/11c41538/attachment.sig>