| Message ID | 1422029330-10971-12-git-send-email-ricardo.ribalda@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Jan 23, 2015 at 05:08:43PM +0100, Ricardo Ribalda Delgado wrote: > + switch (xspi->bits_per_word) { > + case 8: > + *(u8 *)(xspi->rx_ptr) = data; > + break; > + case 16: > + *(u16 *)(xspi->rx_ptr) = data; > + break; > + case 32: > *(u32 *)(xspi->rx_ptr) = data; > - xspi->rx_ptr += 4; > + break; > } Perhaps I'm missing something here but we only seem to be incrementing rx_ptr for the 32 bit case here... > + xspi->rx_ptr += xspi->bits_per_word/8; ...which looks to duplicate this which handles all cases. Also there's a coding style thing - spaces around the / please.
Hello Mark > Perhaps I'm missing something here but we only seem to be incrementing > rx_ptr for the 32 bit case here... The unified diff is a bit confusing this time. This is how the function looks after the patch. All the modes are handled in a generic way. Which I believe it is right: static void xilinx_spi_rx(struct xilinx_spi *xspi) { u32 data = xspi->read_fn(xspi->regs + XSPI_RXD_OFFSET); if (!xspi->rx_ptr) return; switch (xspi->bits_per_word) { case 8: *(u8 *)(xspi->rx_ptr) = data; break; case 16: *(u16 *)(xspi->rx_ptr) = data; break; case 32: *(u32 *)(xspi->rx_ptr) = data; break; } xspi->rx_ptr += xspi->bits_per_word/8; } > >> + xspi->rx_ptr += xspi->bits_per_word/8; > > ...which looks to duplicate this which handles all cases. Also there's > a coding style thing - spaces around the / please. I am fixing this on the next version. (and xspi->tx_ptr += xspi->bits_per_word/8;) I am also cc: the maintainers of checkpatch, because for whatever reason, this was not found by the script ricardo@neopili:~/curro/qtec/linux-upstream$ scripts/checkpatch.pl xilinx-spi2/0011-spi-xilinx-Remove-rx_fn-and-tx_fn-pointer.patch total: 0 errors, 0 warnings, 109 lines checked xilinx-spi2/0011-spi-xilinx-Remove-rx_fn-and-tx_fn-pointer.patch has no obvious style problems and is ready for submission. Thanks!
On Tue, 2015-01-27 at 11:00 +0100, Ricardo Ribalda Delgado wrote: > >> + xspi->rx_ptr += xspi->bits_per_word/8; > > Also there's > > a coding style thing - spaces around the / please. [] > I am also cc: the maintainers of checkpatch, because for whatever > reason, this was not found by the script Because it's personal preference. checkpatch would note if a space was used inconsistently (before but not after the /, or the other way 'round)
Hello Joe On Tue, Jan 27, 2015 at 4:42 PM, Joe Perches <joe@perches.com> wrote: > > Because it's personal preference. > > checkpatch would note if a space was used > inconsistently (before but not after the /, > or the other way 'round) Thanks for the explanation (and the great script :) ) Regards!
diff --git a/drivers/spi/spi-xilinx.c b/drivers/spi/spi-xilinx.c index 29edf1b..6b6c658 100644 --- a/drivers/spi/spi-xilinx.c +++ b/drivers/spi/spi-xilinx.c @@ -92,8 +92,6 @@ struct xilinx_spi { u32 cs_inactive; /* Level of the CS pins when inactive*/ unsigned int (*read_fn)(void __iomem *); void (*write_fn)(u32, void __iomem *); - void (*tx_fn)(struct xilinx_spi *); - void (*rx_fn)(struct xilinx_spi *); }; static void xspi_write32(u32 val, void __iomem *addr) @@ -116,49 +114,32 @@ static unsigned int xspi_read32_be(void __iomem *addr) return ioread32be(addr); } -static void xspi_tx8(struct xilinx_spi *xspi) -{ - xspi->write_fn(*xspi->tx_ptr, xspi->regs + XSPI_TXD_OFFSET); - xspi->tx_ptr++; -} - -static void xspi_tx16(struct xilinx_spi *xspi) -{ - xspi->write_fn(*(u16 *)(xspi->tx_ptr), xspi->regs + XSPI_TXD_OFFSET); - xspi->tx_ptr += 2; -} - -static void xspi_tx32(struct xilinx_spi *xspi) +static void xilinx_spi_tx(struct xilinx_spi *xspi) { xspi->write_fn(*(u32 *)(xspi->tx_ptr), xspi->regs + XSPI_TXD_OFFSET); - xspi->tx_ptr += 4; + xspi->tx_ptr += xspi->bits_per_word/8; } -static void xspi_rx8(struct xilinx_spi *xspi) +static void xilinx_spi_rx(struct xilinx_spi *xspi) { u32 data = xspi->read_fn(xspi->regs + XSPI_RXD_OFFSET); - if (xspi->rx_ptr) { - *xspi->rx_ptr = data & 0xff; - xspi->rx_ptr++; - } -} -static void xspi_rx16(struct xilinx_spi *xspi) -{ - u32 data = xspi->read_fn(xspi->regs + XSPI_RXD_OFFSET); - if (xspi->rx_ptr) { - *(u16 *)(xspi->rx_ptr) = data & 0xffff; - xspi->rx_ptr += 2; - } -} + if (!xspi->rx_ptr) + return; -static void xspi_rx32(struct xilinx_spi *xspi) -{ - u32 data = xspi->read_fn(xspi->regs + XSPI_RXD_OFFSET); - if (xspi->rx_ptr) { + switch (xspi->bits_per_word) { + case 8: + *(u8 *)(xspi->rx_ptr) = data; + break; + case 16: + *(u16 *)(xspi->rx_ptr) = data; + break; + case 32: *(u32 *)(xspi->rx_ptr) = data; - xspi->rx_ptr += 4; + break; } + + xspi->rx_ptr += xspi->bits_per_word/8; } static void xspi_init_hw(struct xilinx_spi *xspi) @@ -250,7 +231,7 @@ static void xilinx_spi_fill_tx_fifo(struct xilinx_spi *xspi, int n_words) while (n_words--) if (xspi->tx_ptr) - xspi->tx_fn(xspi); + xilinx_spi_tx(xspi); else xspi->write_fn(0, xspi->regs + XSPI_TXD_OFFSET); return; @@ -301,7 +282,7 @@ static int xilinx_spi_txrx_bufs(struct spi_device *spi, struct spi_transfer *t) /* Read out all the data from the Rx FIFO */ while (n_words--) - xspi->rx_fn(xspi); + xilinx_spi_rx(xspi); } return t->len - xspi->remaining_bytes; @@ -423,20 +404,6 @@ static int xilinx_spi_probe(struct platform_device *pdev) master->bits_per_word_mask = SPI_BPW_MASK(bits_per_word); xspi->bits_per_word = bits_per_word; - if (xspi->bits_per_word == 8) { - xspi->tx_fn = xspi_tx8; - xspi->rx_fn = xspi_rx8; - } else if (xspi->bits_per_word == 16) { - xspi->tx_fn = xspi_tx16; - xspi->rx_fn = xspi_rx16; - } else if (xspi->bits_per_word == 32) { - xspi->tx_fn = xspi_tx32; - xspi->rx_fn = xspi_rx32; - } else { - ret = -EINVAL; - goto put_master; - } - xspi->buffer_size = xilinx_spi_find_buffer_size(xspi); xspi->irq = platform_get_irq(pdev, 0);
Simplify the code by removing the tx and and rx function pointers and substitute them by a single function. Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> --- drivers/spi/spi-xilinx.c | 69 +++++++++++++----------------------------------- 1 file changed, 18 insertions(+), 51 deletions(-)