Re: [PATCH v1 1/2] spi: npcm-pspi: add full duplex support

[Mark Brown <broonie@kernel.org>]

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

On Thu, Jul 21, 2022 at 01:15:55PM +0300, Tomer Maimon wrote:

> The NPCM PSPI handler, on TX-buffer not null, would perform a dummy read
> but did not save the rx-data, this was valid only for half duplex.

> This patch adds full duplex support for NPCM PSPI driver by storing all
> rx-data when the Rx-buffer is defined also for TX-buffer handling.

This doesn't seem to entirely correspond to what the patch does, nor to
what the driver currently does?  I can't see any dummy read code in the
current driver.

>  static void npcm_pspi_send(struct npcm_pspi *priv)
>  {
>  	int wsize;
> -	u16 val;
> +	u16 val = 0;
>  
>  	wsize = min(bytes_per_word(priv->bits_per_word), priv->tx_bytes);
>  	priv->tx_bytes -= wsize;
>  
> -	if (!priv->tx_buf)
> -		return;
> -
>  	switch (wsize) {
>  	case 1:
> -		val = *priv->tx_buf++;
> +		if (priv->tx_buf)
> +			val = *priv->tx_buf++;
>  		iowrite8(val, NPCM_PSPI_DATA + priv->base);
>  		break;

These changes appaear to be trying to ensure that when _send() is called
we now always write something out, even if there was no transmit buffer.
Since the device has been supporting half duplex transfers it is not
clear why we'd want to do that, it's adding overhead to the PIO which
isn't great.  This also isn't what the changelog said, the changelog
said we were adding reading of data when there's a transmit buffer.
Similar issues apply on the read side.

AFAICT the bulk of what the change is doing is trying make the driver
unconditionally do both read and writes to the hardware when it would
previously have only read or written data if there was a buffer
provided.  That's basically open coding SPI_CONTROLLER_MUST_TX and
SPI_CONTROLLER_MUST_RX, if that's what the hardware needs then you
should just set those flags and let the core fix things up.

> +       /*
> +        * first we do the read since if we do the write we previous read might
> +        * be lost (indeed low chances)
> +        */

This reordering sounds like it might be needed but should have been
mentioned in the changelog and is a separate patch.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]