On 14.02.2018 13:10, Ajay Singh wrote:
> Refactor spi_cmd_complete() to fix the line over 80 char issues reported
> by checkpatch.pl script.
>
> Signed-off-by: Ajay Singh <ajay.kat...@microchip.com>
> ---
> drivers/staging/wilc1000/wilc_spi.c | 250
> ++++++++++++++++++------------------
> 1 file changed, 124 insertions(+), 126 deletions(-)
>
> diff --git a/drivers/staging/wilc1000/wilc_spi.c
> b/drivers/staging/wilc1000/wilc_spi.c
> index 66b6aea..30a9a61 100644
> --- a/drivers/staging/wilc1000/wilc_spi.c
> +++ b/drivers/staging/wilc1000/wilc_spi.c
> @@ -287,17 +287,19 @@ static int spi_cmd_complete(struct wilc *wilc, u8 cmd,
> u32 adr, u8 *b, u32 sz,
> u8 rsp;
> int len = 0;
> int result = N_OK;
> + int retry;
> + u8 crc[2];
>
> wb[0] = cmd;
> switch (cmd) {
> - case CMD_SINGLE_READ: /* single word (4
> bytes) read */
> + case CMD_SINGLE_READ: /* single word (4 bytes) read */
Ok with this but I think would be nicer to have them documented the place were
they are defined.
> wb[1] = (u8)(adr >> 16);
> wb[2] = (u8)(adr >> 8);
> wb[3] = (u8)adr;
> len = 5;
> break;
>
> - case CMD_INTERNAL_READ: /* internal register read */
> + case CMD_INTERNAL_READ: /* internal register read */
Ditto
> wb[1] = (u8)(adr >> 8);
> if (clockless == 1)
> wb[1] |= BIT(7);
> @@ -306,29 +308,29 @@ static int spi_cmd_complete(struct wilc *wilc, u8 cmd,
> u32 adr, u8 *b, u32 sz,
> len = 5;
> break;
>
> - case CMD_TERMINATE: /* termination
> */
> + case CMD_TERMINATE:
Ditto
> wb[1] = 0x00;
> wb[2] = 0x00;
> wb[3] = 0x00;
> len = 5;
> break;
>
> - case CMD_REPEAT: /*
> repeat */
> + case CMD_REPEAT:
Ditto
> wb[1] = 0x00;
> wb[2] = 0x00;
> wb[3] = 0x00;
> len = 5;
> break;
>
> - case CMD_RESET: /*
> reset */
> + case CMD_RESET:
Ditto
> wb[1] = 0xff;
> wb[2] = 0xff;
> wb[3] = 0xff;
> len = 5;
> break;
>
> - case CMD_DMA_WRITE: /* dma write */
> - case CMD_DMA_READ: /* dma read */
> + case CMD_DMA_WRITE: /* dma write */
> + case CMD_DMA_READ: /* dma read */
Ditto
> wb[1] = (u8)(adr >> 16);
> wb[2] = (u8)(adr >> 8);
> wb[3] = (u8)adr;
> @@ -337,8 +339,8 @@ static int spi_cmd_complete(struct wilc *wilc, u8 cmd,
> u32 adr, u8 *b, u32 sz,
> len = 7;
> break;
>
> - case CMD_DMA_EXT_WRITE: /* dma extended write */
> - case CMD_DMA_EXT_READ: /* dma extended read */
> + case CMD_DMA_EXT_WRITE: /* dma extended write */
> + case CMD_DMA_EXT_READ: /* dma extended read */
Ditto
> wb[1] = (u8)(adr >> 16);
> wb[2] = (u8)(adr >> 8);
> wb[3] = (u8)adr;
> @@ -348,7 +350,7 @@ static int spi_cmd_complete(struct wilc *wilc, u8 cmd,
> u32 adr, u8 *b, u32 sz,
> len = 8;
> break;
>
> - case CMD_INTERNAL_WRITE: /* internal register write */
> + case CMD_INTERNAL_WRITE: /* internal register write */
Ditto
> wb[1] = (u8)(adr >> 8);
> if (clockless == 1)
> wb[1] |= BIT(7);
> @@ -360,7 +362,7 @@ static int spi_cmd_complete(struct wilc *wilc, u8 cmd,
> u32 adr, u8 *b, u32 sz,
> len = 8;
> break;
>
> - case CMD_SINGLE_WRITE: /* single word write */
> + case CMD_SINGLE_WRITE: /* single word write */
Ditto
> wb[1] = (u8)(adr >> 16);
> wb[2] = (u8)(adr >> 8);
> wb[3] = (u8)(adr);
> @@ -395,13 +397,12 @@ static int spi_cmd_complete(struct wilc *wilc, u8 cmd,
> u32 adr, u8 *b, u32 sz,
> cmd == CMD_REPEAT) {
> len2 = len + (NUM_SKIP_BYTES + NUM_RSP_BYTES + NUM_DUMMY_BYTES);
> } else if (cmd == CMD_INTERNAL_READ || cmd == CMD_SINGLE_READ) {
> - if (!g_spi.crc_off) {
> - len2 = len + (NUM_RSP_BYTES + NUM_DATA_HDR_BYTES +
> NUM_DATA_BYTES
> - + NUM_CRC_BYTES + NUM_DUMMY_BYTES);
> - } else {
> - len2 = len + (NUM_RSP_BYTES + NUM_DATA_HDR_BYTES +
> NUM_DATA_BYTES
> - + NUM_DUMMY_BYTES);
> - }
> + int tmp = NUM_RSP_BYTES + NUM_DATA_HDR_BYTES + NUM_DATA_BYTES
> + + NUM_DUMMY_BYTES;
> + if (!g_spi.crc_off)
> + len2 = len + tmp + NUM_CRC_BYTES;
> + else
> + len2 = len + tmp;
> } else {
> len2 = len + (NUM_RSP_BYTES + NUM_DUMMY_BYTES);
> }
> @@ -425,11 +426,8 @@ static int spi_cmd_complete(struct wilc *wilc, u8 cmd,
> u32 adr, u8 *b, u32 sz,
> /*
> * Command/Control response
> */
> - if (cmd == CMD_RESET ||
> - cmd == CMD_TERMINATE ||
> - cmd == CMD_REPEAT) {
> - rix++; /* skip 1 byte */
> - }
> + if (cmd == CMD_RESET || cmd == CMD_TERMINATE || cmd == CMD_REPEAT)
> + rix++; /* skip 1 byte */
>
> rsp = rb[rix++];
>
> @@ -452,8 +450,6 @@ static int spi_cmd_complete(struct wilc *wilc, u8 cmd,
> u32 adr, u8 *b, u32 sz,
>
> if (cmd == CMD_INTERNAL_READ || cmd == CMD_SINGLE_READ ||
> cmd == CMD_DMA_READ || cmd == CMD_DMA_EXT_READ) {
> - int retry;
> - u8 crc[2];
> /*
> * Data Respnose header
> */
> @@ -478,132 +474,134 @@ static int spi_cmd_complete(struct wilc *wilc, u8
> cmd, u32 adr, u8 *b, u32 sz,
> "Error, data read response (%02x)\n", rsp);
> return N_RESET;
> }
> + }
> +
> + if (cmd == CMD_INTERNAL_READ || cmd == CMD_SINGLE_READ) {
> + /*
> + * Read bytes
> + */
> + if ((rix + 3) < len2) {
> + b[0] = rb[rix++];
> + b[1] = rb[rix++];
> + b[2] = rb[rix++];
> + b[3] = rb[rix++];
> + } else {
> + dev_err(&spi->dev,
> + "buffer overrun when reading data.\n");
> + return N_FAIL;
> + }
>
> - if (cmd == CMD_INTERNAL_READ || cmd == CMD_SINGLE_READ) {
> + if (!g_spi.crc_off) {
> /*
> - * Read bytes
> + * Read Crc
> */
> - if ((rix + 3) < len2) {
> - b[0] = rb[rix++];
> - b[1] = rb[rix++];
> - b[2] = rb[rix++];
> - b[3] = rb[rix++];
> + if ((rix + 1) < len2) {
> + crc[0] = rb[rix++];
> + crc[1] = rb[rix++];
> } else {
> dev_err(&spi->dev,
> - "buffer overrun when reading data.\n");
> + "buffer overrun when reading crc.\n");
Since you refactor, you may try to have a consistent way of returning in case of
errors. I'm thinking that this kind of block:
{
dev_err(&spi->dev, "msg");
return N_FAIL;
}
could be replaced with:
- at the beginning of function:
const char *errmsg = NULL;
- and in case of error you could add this block:
{
errmsg = "you error string";
result = N_FAIL;
goto _error_;
}
and for _error_ label:
_error_:
if (errmsg)
dev_err(&spi->dev, errmsg);
return result;
And use this pattern wherever an error needs to be thrown, in a consistent way.
This could remain as is in this patch, for the moment, but further re-factors
might be needed for this function to look better, maybe should be added to
backlog.
I want to say that might be nice to have a consistent way of returning with
errors, some paths in this function just uses "return error;" other paths are
using:
result = error;
goto _error_;
and the result is the same.
> return N_FAIL;
> }
> + }
> + } else if ((cmd == CMD_DMA_READ) || (cmd == CMD_DMA_EXT_READ)) {
> + int ix;
>
> - if (!g_spi.crc_off) {
> - /*
> - * Read Crc
> - */
> - if ((rix + 1) < len2) {
> - crc[0] = rb[rix++];
> - crc[1] = rb[rix++];
> - } else {
> - dev_err(&spi->dev, "buffer overrun when
> reading crc.\n");
> - return N_FAIL;
> - }
> - }
> - } else if ((cmd == CMD_DMA_READ) || (cmd == CMD_DMA_EXT_READ)) {
> - int ix;
> -
> - /* some data may be read in response to dummy bytes. */
> - for (ix = 0; (rix < len2) && (ix < sz); )
> - b[ix++] = rb[rix++];
> -
> - sz -= ix;
> -
> - if (sz > 0) {
> - int nbytes;
> + /* some data may be read in response to dummy bytes. */
> + for (ix = 0; (rix < len2) && (ix < sz); )
> + b[ix++] = rb[rix++];
>
> - if (sz <= (DATA_PKT_SZ - ix))
> - nbytes = sz;
> - else
> - nbytes = DATA_PKT_SZ - ix;
> + sz -= ix;
>
> - /*
> - * Read bytes
> - */
> - if (wilc_spi_rx(wilc, &b[ix], nbytes)) {
> - dev_err(&spi->dev, "Failed data block
> read, bus error...\n");
> - result = N_FAIL;
> - goto _error_;
> - }
> + if (sz > 0) {
> + int nbytes;
>
> - /*
> - * Read Crc
> - */
> - if (!g_spi.crc_off) {
> - if (wilc_spi_rx(wilc, crc, 2)) {
> - dev_err(&spi->dev, "Failed data
> block crc read, bus error...\n");
> - result = N_FAIL;
> - goto _error_;
> - }
> - }
> + if (sz <= (DATA_PKT_SZ - ix))
> + nbytes = sz;
> + else
> + nbytes = DATA_PKT_SZ - ix;
>
> - ix += nbytes;
> - sz -= nbytes;
> + /*
> + * Read bytes
> + */
> + if (wilc_spi_rx(wilc, &b[ix], nbytes)) {
> + dev_err(&spi->dev,
> + "Failed block read, bus err\n");
> + result = N_FAIL;
> + goto _error_;
> }
>
> /*
> - * if any data in left unread,
> - * then read the rest using normal DMA code.
> + * Read Crc
> */
> - while (sz > 0) {
> - int nbytes;
> + if (!g_spi.crc_off && wilc_spi_rx(wilc, crc, 2)) {
> + dev_err(&spi->dev,
> + "Failed block crc read, bus err\n");
> + result = N_FAIL;
> + goto _error_;
> + }
>
> - if (sz <= DATA_PKT_SZ)
> - nbytes = sz;
> - else
> - nbytes = DATA_PKT_SZ;
> + ix += nbytes;
> + sz -= nbytes;
> + }
>
> - /*
> - * read data response only on the next DMA
> cycles not
> - * the first DMA since data response header is
> already
> - * handled above for the first DMA.
> - */
> - /*
> - * Data Respnose header
> - */
> - retry = 10;
> - do {
> - if (wilc_spi_rx(wilc, &rsp, 1)) {
> - dev_err(&spi->dev, "Failed data
> response read, bus error...\n");
> - result = N_FAIL;
> - break;
> - }
> - if (((rsp >> 4) & 0xf) == 0xf)
> - break;
> - } while (retry--);
> -
> - if (result == N_FAIL)
> - break;
> + /*
> + * if any data in left unread,
> + * then read the rest using normal DMA code.
> + */
> + while (sz > 0) {
> + int nbytes;
>
> - /*
> - * Read bytes
> - */
> - if (wilc_spi_rx(wilc, &b[ix], nbytes)) {
> - dev_err(&spi->dev, "Failed data block
> read, bus error...\n");
> + if (sz <= DATA_PKT_SZ)
> + nbytes = sz;
> + else
> + nbytes = DATA_PKT_SZ;
> +
> + /*
> + * read data response only on the next DMA cycles not
> + * the first DMA since data response header is already
> + * handled above for the first DMA.
> + */
> + /*
> + * Data Respnose header
> + */
> + retry = 10;
> + do {
> + if (wilc_spi_rx(wilc, &rsp, 1)) {
> + dev_err(&spi->dev,
> + "Failed resp read, bus err\n");
> result = N_FAIL;
> break;
Here, also, the proposed solution could be used. Or just return as in the other
cases or just goto _error_ instead of break and then check for result == N_FAIL.
> }
> + if (((rsp >> 4) & 0xf) == 0xf)
> + break;
> + } while (retry--);
>
> - /*
> - * Read Crc
> - */
> - if (!g_spi.crc_off) {
> - if (wilc_spi_rx(wilc, crc, 2)) {
> - dev_err(&spi->dev, "Failed data
> block crc read, bus error...\n");
> - result = N_FAIL;
> - break;
> - }
> - }
> + if (result == N_FAIL)
> + break;
>
> - ix += nbytes;
> - sz -= nbytes;
> + /*
> + * Read bytes
> + */
> + if (wilc_spi_rx(wilc, &b[ix], nbytes)) {
> + dev_err(&spi->dev,
> + "Failed block read, bus err\n");
> + result = N_FAIL;
> + break;
> }
> +
> + /*
> + * Read Crc
> + */
> + if (!g_spi.crc_off && wilc_spi_rx(wilc, crc, 2)) {
> + dev_err(&spi->dev,
> + "Failed block crc read, bus err\n");
> + result = N_FAIL;
> + break;
> + }
> +
> + ix += nbytes;
> + sz -= nbytes;
> }
> }
> _error_:
>
_______________________________________________
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
