Re: [PATCH 8/9] spi: amd: Refactor to overcome 70 bytes per CS limitation

[Mark Brown]

On Tue, Aug 24, 2021 at 11:40:40AM +0100, Lucas Tanure wrote:
> AMD SPI controller has 70 bytes for its FIFO and it has an
> automatic way of controlling it`s internal CS, which can
> only be activated during the time that the FIFO is being
> transfered.

> SPI_MASTER_HALF_DUPLEX here means that it can only read
> RX bytes after TX bytes were written, and RX+TX must be
> less than 70. If you write 4 bytes the first byte of read
> is in position 5 of the FIFO.
> 
> All of that means that for devices that require an address
> for reads and writes, the 2 transfers must be put in the same
> FIFO so the CS can be hold for address and data, otherwise
> the data would lose it`s meaning.

This commit message is confusing, it's hard to tell what the refactoring
is.  It also doesn't seem at all joined up with the rest of the series
or the contents of the patch.  The rest of this series adds a new
interface which says that the controller is only capable of doing a
single transfer which means that the core should ensure that the
controller never sees a message with more than one transfer in it but
this patch appears to be attempting to parse multiple transfers and pack
them together due to controller limitations in some way.  That is never
going to do anything useful, if anything is paying attention to the flag
the controller will never see messages like that.  Indeed code that pays
attention to the flag and needs this is likely to refuse to work with
the hardware at all since the device is half duplex so anything that
needs messages mixing reads and writes just won't work at all.

It also looks like the code is adding support for a new revision of the
hardware which isn't mentioned anywhere in the commit message at all and
really should be, it should most likely be a separate commit.

As far as I can tell what you're trying to do here is better expressed
as saying that the controller has a limit on the maximum message size
then having the transfer_one_message() pack those down into whatever the
controller needs so it can send them without bouncing chip select.  The
new flag is not needed for this device and indeed looks like it will
actively work against what's needed to make the controller useful in
these applications.

Please also use normal apostrophies.

> +	amd_spi_set_tx_count(amd_spi, tx1_len + tx2_len);
> +	ret = amd_spi_execute_opcode(amd_spi);
> +
> +	return ret ? ret : tx1_len + 1 + tx2_len;

Please write normal conditional statements so people can read the code
more easily.

> +static const struct amd_spi_devtype_data spi_v1 = {
> +       .exec_op        = amd_spi_execute_opcode_v1,
> +       .set_op         = amd_spi_set_opcode_v1,
> +};
> +
> +static const struct amd_spi_devtype_data spi_v2 = {
> +       .version        = 1,

v2 sets the version to 1 and v1 doesn't set the version at all and the
only use of the version field AFAICT is that we should try to call
amd_spi_clear_chip() after a transfer.  This isn't entirely clear.  If
it's just a flag for the need to do that clear make it an explicit flag
for that.

Attachment: signature.asc
Description: PGP signature