spi: npcm-pspi: add Arbel NPCM8XX and full duplex support

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.

static void npcm_pspi_send(struct npcm_pspi *priv) {

drivers/spi/spi-npcm-pspi.c +1 -1

 	int wsize;
-	u16 val;
+	u16 val = 0;
 
 	wsize = min(bytes_per_word(priv->bits_per_word), priv->tx_bytes);
 	priv->tx_bytes -= wsize;
 

Quoted 2 lines · Tomer Maimon

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.

Quoted 2 lines

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.

Quoted 9 lines

static void npcm_pspi_send(struct npcm_pspi *priv)

 {
      int wsize;

drivers/spi/spi-npcm-pspi.c +1 -1

-     u16 val;
+     u16 val = 0;
 
      wsize = min(bytes_per_word(priv->bits_per_word), priv->tx_bytes);
      priv->tx_bytes -= wsize;
 

drivers/spi/spi-npcm-pspi.c static void npcm_pspi_setup_transfer(struct spi_device *spi, · line 208

Quoted 10 lines

drivers/spi/spi-npcm-pspi.c +2 -4

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

drivers/spi/spi-npcm-pspi.c static irqreturn_t npcm_pspi_handler(int irq, void *dev_id) · line 315

Quoted 5 lines

drivers/spi/spi-npcm-pspi.c +4 -0

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

Quoted 8 lines · Mark Brown

Quoted 2 lines · Tomer Maimon

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.

In the current handler file, in the handler function. static irqreturn_t npcm_pspi_handler(int irq, void *dev_id)

drivers/spi/spi-npcm-pspi.c +0 -3

-       if (priv->tx_buf) {
-               if (stat & NPCM_PSPI_STAT_RBF) {
-                       ioread8(NPCM_PSPI_DATA + priv->base);

the read above doing a dummy read

Quoted 4 lines · Tomer Maimon

Required properties:
- - compatible : "nuvoton,npcm750-pspi" for NPCM7XX BMC
+ - compatible : "nuvoton,npcm750-pspi" for Poleg NPCM7XX.
+ "nuvoton,npcm845-pspi" for Arbel NPCM8XX.

You've not updated the driver to accept this compatible and this doesn't say anything about fallbacks.

Tomer Maimon (2):
spi: npcm-pspi: add full duplex support
dt-binding: spi: npcm-pspi: Add npcm845 compatible

Quoted 3 lines · Tomer Maimon

Tomer Maimon (2):
spi: npcm-pspi: add full duplex support
dt-binding: spi: npcm-pspi: Add npcm845 compatible

It is not obvious why these are a series, they appear to be entirely orthogonal. If there's no relationship between patches it's generally better to send them separately, that way problems with one patch won't hold up unrelated patches and reviewers aren't left wondering about why things are grouped.

Please submit patches using subject lines reflecting the style for the subsystem, this makes it easier for people to identify relevant patches. Look at what existing commits in the area you're changing are doing and make sure your subject lines visually resemble what they're doing. There's no need to resubmit to fix this alone.