Re: [PATCH v9 4/6] iio: adc: ad4691: add SPI offload support

[Jonathan Cameron <jic23@kernel.org>]

> > +		/* TX: address phase, CS stays asserted into data phase */
> > +		st->scan_xfers[2 * k].tx_buf = offload->tx_cmd[k];
> > +		st->scan_xfers[2 * k].len = sizeof(offload->tx_cmd[k]);
> > +		st->scan_xfers[2 * k].bits_per_word = bpw;  
> 
> "When bits_per_word is greater than 8 (like bpw = 16 here), the SPI framework
> treats tx_buf as an array of native 16-bit words.
> On little-endian architectures, the controller will byte-swap the data before
> transmitting it. Will using a u8 array and put_unaligned_be16() result in the
> command bytes being reversed on the wire?"
> 
> Switched to cpu_to_be16() assigned directly into __be16 scan_tx[],
> matching the non-offload path. This makes the intended wire format
> self-evident and sidesteps the byte-ordering question entirely.

This confuses me a bit because the SPI controller should work with
native endian and from that generate the expected big endian on the wire.

So on a little endian host byte order in address space is LH but it will
write top bit of H first thus the ADC channel address needs to be in the
second byte.
On a big endian host despite the ordering in memory being HL, the top
bit of H is still written first thus in needs to be in the first byte.


If you using cpu_to_be16() to assign a 16 bit value swapping only on little endian
and start with the cmd in L on little endian you'll end up with LH swapped to
HL and on big endian HL but the little endian SPI controller should then swap
it again sending what it thinks is the high byte first (L) whereas the big endian
system will send H.

Upshot. I think the field should be native endian. If a byte swap is needed
it should be unconditional and not rely on endianness of the host.

> 
> > +
> > +		/* RX: data phase, CS toggles after to delimit the next register
> > op */
> > +		st->scan_xfers[2 * k + 1].len = sizeof(offload->tx_cmd[k]);
> > +		st->scan_xfers[2 * k + 1].bits_per_word = bpw;
> > +		st->scan_xfers[2 * k + 1].offload_flags =
> > SPI_OFFLOAD_XFER_RX_STREAM;
> > +		st->scan_xfers[2 * k + 1].cs_change = 1;
> > +		k++;
> > +	}
> > +
> > +	/* State reset to re-arm DATA_READY for the next scan. */
> > +	put_unaligned_be16(AD4691_STATE_RESET_REG, offload->tx_reset);
> > +	offload->tx_reset[2] = AD4691_STATE_RESET_ALL;
> > +
> > +	st->scan_xfers[2 * k].tx_buf = offload->tx_reset;
> > +	st->scan_xfers[2 * k].len = sizeof(offload->tx_cmd[k]);
> > +	st->scan_xfers[2 * k].bits_per_word = bpw;
> > +
> > +	st->scan_xfers[2 * k + 1].tx_buf = &offload->tx_reset[2];
> > +	st->scan_xfers[2 * k + 1].len = sizeof(offload->tx_cmd[k]);  
> 
> "Will passing &offload->tx_reset[2] directly as tx_buf cause DMA mapping
> issues, since it is only 2-byte aligned?"
> 
> Addressed by the second concern's fix: no sub-aligned pointer into the middle
> of a DMA buffer — the full 4-byte scan_tx_reset[] is passed as tx_buf
> from its own cache-line-aligned start address.
>