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

[Mark Brown]

On Wed, Dec 02, 2015 at 08:25:08AM +0100, Martin Sperl wrote:
> > On 01.12.2015, at 22:29, Mark Brown <broonie at kernel.org> wrote:
> >> On Mon, Nov 30, 2015 at 01:04:53PM +0000, kernel at martin.sperl.org wrote:

Please use normal formatting for e-mails with blank lines between
paragraphs, it makes things much easier to read.

> >> +/* 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...

> It is done the same way as devm_kalloc uses struct devres:
> the magic devres struct is hidden by the devres framework. 
> Only the payload data pointer is returned, which is what this
> structure describes.

> You want it as a more explicit member instead?

If it's not actually a resource and is really just a structure that
happens to be managed with a resource then describe it as such, I'm
complaining about the confusing naming here.

> >> +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;

> > 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.

> Essentially this will handle count spi-transfers that have all identical data
> (Except for transfer_list) initially - especially Len and tx/rx buf.
> The first of those transfers is set its length to shift 
> (no need to modify rx/tx_buf)
> The others are reduced by that length and the pointers are shifted up,
> So that they are now starting at the address after the first transfer and
> Transfer the correct number of bytes.

I'm sorry but the above is still very opaque - why would we have some
list of transfers that all have identical data?  I *think* the intention
here is to do some realignment and that shift is really an offset to
move things by?

> Rx/tx_dma is handled because it is there and it's address needs also to be
> accurately Shifted by that offset. I know that it is a depreciated interface,
> but as it is still there it needs to be supported...

It's really quite broken already for most users.
-------------- 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/20151203/dc62fc55/attachment.sig>