| Message ID | 1389772636.13585.1.camel@phoenix (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
2014/1/15 Barry Song <Barry.Song@csr.com>: > > >> -----Original Message----- >> From: Axel Lin [mailto:axel.lin@ingics.com] >> Sent: Wednesday, January 15, 2014 3:57 PM >> To: Mark Brown >> Cc: Zhiwu Song; Barry Song; Qipan Li; linux-spi@vger.kernel.org >> Subject: [PATCH] spi: sirf: Avoid duplicate code in various bits_per_word >> cases >> >> Trivial cleanup to avoid duplicate code in various bits_per_word cases. >> >> Signed-off-by: Axel Lin <axel.lin@ingics.com> >> --- >> drivers/spi/spi-sirf.c | 21 ++++++--------------- >> 1 file changed, 6 insertions(+), 15 deletions(-) >> >> diff --git a/drivers/spi/spi-sirf.c b/drivers/spi/spi-sirf.c index >> ed5e501..787683d 100644 >> --- a/drivers/spi/spi-sirf.c >> +++ b/drivers/spi/spi-sirf.c >> @@ -459,11 +459,6 @@ spi_sirfsoc_setup_transfer(struct spi_device *spi, >> struct spi_transfer *t) >> regval |= SIRFSOC_SPI_TRAN_DAT_FORMAT_8; >> sspi->rx_word = spi_sirfsoc_rx_word_u8; >> sspi->tx_word = spi_sirfsoc_tx_word_u8; >> - txfifo_ctrl = SIRFSOC_SPI_FIFO_THD(SIRFSOC_SPI_FIFO_SIZE >> / 2) | >> - SIRFSOC_SPI_FIFO_WIDTH_BYTE; >> - rxfifo_ctrl = SIRFSOC_SPI_FIFO_THD(SIRFSOC_SPI_FIFO_SIZE >> / 2) | >> - SIRFSOC_SPI_FIFO_WIDTH_BYTE; >> - sspi->word_width = 1; >> break; >> case 12: >> case 16: >> @@ -471,26 +466,22 @@ spi_sirfsoc_setup_transfer(struct spi_device *spi, >> struct spi_transfer *t) >> SIRFSOC_SPI_TRAN_DAT_FORMAT_16; >> sspi->rx_word = spi_sirfsoc_rx_word_u16; >> sspi->tx_word = spi_sirfsoc_tx_word_u16; >> - txfifo_ctrl = SIRFSOC_SPI_FIFO_THD(SIRFSOC_SPI_FIFO_SIZE >> / 2) | >> - SIRFSOC_SPI_FIFO_WIDTH_WORD; >> - rxfifo_ctrl = SIRFSOC_SPI_FIFO_THD(SIRFSOC_SPI_FIFO_SIZE >> / 2) | >> - SIRFSOC_SPI_FIFO_WIDTH_WORD; >> - sspi->word_width = 2; >> break; >> case 32: >> regval |= SIRFSOC_SPI_TRAN_DAT_FORMAT_32; >> sspi->rx_word = spi_sirfsoc_rx_word_u32; >> sspi->tx_word = spi_sirfsoc_tx_word_u32; >> - txfifo_ctrl = SIRFSOC_SPI_FIFO_THD(SIRFSOC_SPI_FIFO_SIZE >> / 2) | >> - SIRFSOC_SPI_FIFO_WIDTH_DWORD; >> - rxfifo_ctrl = SIRFSOC_SPI_FIFO_THD(SIRFSOC_SPI_FIFO_SIZE >> / 2) | >> - SIRFSOC_SPI_FIFO_WIDTH_DWORD; >> - sspi->word_width = 4; >> break; >> default: >> BUG(); >> } >> >> + txfifo_ctrl = SIRFSOC_SPI_FIFO_THD(SIRFSOC_SPI_FIFO_SIZE / 2) | >> + SIRFSOC_SPI_FIFO_WIDTH_WORD; >> + rxfifo_ctrl = SIRFSOC_SPI_FIFO_THD(SIRFSOC_SPI_FIFO_SIZE / 2) | >> + SIRFSOC_SPI_FIFO_WIDTH_WORD; >> + sspi->word_width = DIV_ROUND_UP(bits_per_word, 8); >> + Axel, thanks. it is a good cleanup, but it seems it is not right for txfifo_ctrl and rxfifo_ctrl? >> if (!(spi->mode & SPI_CS_HIGH)) >> regval |= SIRFSOC_SPI_CS_IDLE_STAT; >> if (!(spi->mode & SPI_LSB_FIRST)) >> -- >> 1.8.1.2 >> -barry -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2014/1/15 Barry Song <21cnbao@gmail.com>: > 2014/1/15 Barry Song <Barry.Song@csr.com>: >> >> >>> -----Original Message----- >>> From: Axel Lin [mailto:axel.lin@ingics.com] >>> Sent: Wednesday, January 15, 2014 3:57 PM >>> To: Mark Brown >>> Cc: Zhiwu Song; Barry Song; Qipan Li; linux-spi@vger.kernel.org >>> Subject: [PATCH] spi: sirf: Avoid duplicate code in various bits_per_word >>> cases >>> >>> Trivial cleanup to avoid duplicate code in various bits_per_word cases. >>> >>> Signed-off-by: Axel Lin <axel.lin@ingics.com> >>> --- >>> drivers/spi/spi-sirf.c | 21 ++++++--------------- >>> 1 file changed, 6 insertions(+), 15 deletions(-) >>> >>> diff --git a/drivers/spi/spi-sirf.c b/drivers/spi/spi-sirf.c index >>> ed5e501..787683d 100644 >>> --- a/drivers/spi/spi-sirf.c >>> +++ b/drivers/spi/spi-sirf.c >>> @@ -459,11 +459,6 @@ spi_sirfsoc_setup_transfer(struct spi_device *spi, >>> struct spi_transfer *t) >>> regval |= SIRFSOC_SPI_TRAN_DAT_FORMAT_8; >>> sspi->rx_word = spi_sirfsoc_rx_word_u8; >>> sspi->tx_word = spi_sirfsoc_tx_word_u8; >>> - txfifo_ctrl = SIRFSOC_SPI_FIFO_THD(SIRFSOC_SPI_FIFO_SIZE >>> / 2) | >>> - SIRFSOC_SPI_FIFO_WIDTH_BYTE; >>> - rxfifo_ctrl = SIRFSOC_SPI_FIFO_THD(SIRFSOC_SPI_FIFO_SIZE >>> / 2) | >>> - SIRFSOC_SPI_FIFO_WIDTH_BYTE; >>> - sspi->word_width = 1; >>> break; >>> case 12: >>> case 16: >>> @@ -471,26 +466,22 @@ spi_sirfsoc_setup_transfer(struct spi_device *spi, >>> struct spi_transfer *t) >>> SIRFSOC_SPI_TRAN_DAT_FORMAT_16; >>> sspi->rx_word = spi_sirfsoc_rx_word_u16; >>> sspi->tx_word = spi_sirfsoc_tx_word_u16; >>> - txfifo_ctrl = SIRFSOC_SPI_FIFO_THD(SIRFSOC_SPI_FIFO_SIZE >>> / 2) | >>> - SIRFSOC_SPI_FIFO_WIDTH_WORD; >>> - rxfifo_ctrl = SIRFSOC_SPI_FIFO_THD(SIRFSOC_SPI_FIFO_SIZE >>> / 2) | >>> - SIRFSOC_SPI_FIFO_WIDTH_WORD; >>> - sspi->word_width = 2; >>> break; >>> case 32: >>> regval |= SIRFSOC_SPI_TRAN_DAT_FORMAT_32; >>> sspi->rx_word = spi_sirfsoc_rx_word_u32; >>> sspi->tx_word = spi_sirfsoc_tx_word_u32; >>> - txfifo_ctrl = SIRFSOC_SPI_FIFO_THD(SIRFSOC_SPI_FIFO_SIZE >>> / 2) | >>> - SIRFSOC_SPI_FIFO_WIDTH_DWORD; >>> - rxfifo_ctrl = SIRFSOC_SPI_FIFO_THD(SIRFSOC_SPI_FIFO_SIZE >>> / 2) | >>> - SIRFSOC_SPI_FIFO_WIDTH_DWORD; >>> - sspi->word_width = 4; >>> break; >>> default: >>> BUG(); >>> } >>> >>> + txfifo_ctrl = SIRFSOC_SPI_FIFO_THD(SIRFSOC_SPI_FIFO_SIZE / 2) | >>> + SIRFSOC_SPI_FIFO_WIDTH_WORD; >>> + rxfifo_ctrl = SIRFSOC_SPI_FIFO_THD(SIRFSOC_SPI_FIFO_SIZE / 2) | >>> + SIRFSOC_SPI_FIFO_WIDTH_WORD; >>> + sspi->word_width = DIV_ROUND_UP(bits_per_word, 8); >>> + > > Axel, thanks. > it is a good cleanup, but it seems it is not right for txfifo_ctrl and > rxfifo_ctrl? I don't get it. I thought there is no functional change with this patch. Regards, Axel -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2014/1/15 Axel Lin <axel.lin@ingics.com>: > 2014/1/15 Barry Song <21cnbao@gmail.com>: >> 2014/1/15 Barry Song <Barry.Song@csr.com>: >>> >>> >>>> -----Original Message----- >>>> From: Axel Lin [mailto:axel.lin@ingics.com] >>>> Sent: Wednesday, January 15, 2014 3:57 PM >>>> To: Mark Brown >>>> Cc: Zhiwu Song; Barry Song; Qipan Li; linux-spi@vger.kernel.org >>>> Subject: [PATCH] spi: sirf: Avoid duplicate code in various bits_per_word >>>> cases >>>> >>>> Trivial cleanup to avoid duplicate code in various bits_per_word cases. >>>> >>>> Signed-off-by: Axel Lin <axel.lin@ingics.com> >>>> --- >>>> drivers/spi/spi-sirf.c | 21 ++++++--------------- >>>> 1 file changed, 6 insertions(+), 15 deletions(-) >>>> >>>> diff --git a/drivers/spi/spi-sirf.c b/drivers/spi/spi-sirf.c index >>>> ed5e501..787683d 100644 >>>> --- a/drivers/spi/spi-sirf.c >>>> +++ b/drivers/spi/spi-sirf.c >>>> @@ -459,11 +459,6 @@ spi_sirfsoc_setup_transfer(struct spi_device *spi, >>>> struct spi_transfer *t) >>>> regval |= SIRFSOC_SPI_TRAN_DAT_FORMAT_8; >>>> sspi->rx_word = spi_sirfsoc_rx_word_u8; >>>> sspi->tx_word = spi_sirfsoc_tx_word_u8; >>>> - txfifo_ctrl = SIRFSOC_SPI_FIFO_THD(SIRFSOC_SPI_FIFO_SIZE >>>> / 2) | >>>> - SIRFSOC_SPI_FIFO_WIDTH_BYTE; >>>> - rxfifo_ctrl = SIRFSOC_SPI_FIFO_THD(SIRFSOC_SPI_FIFO_SIZE >>>> / 2) | >>>> - SIRFSOC_SPI_FIFO_WIDTH_BYTE; >>>> - sspi->word_width = 1; >>>> break; >>>> case 12: >>>> case 16: >>>> @@ -471,26 +466,22 @@ spi_sirfsoc_setup_transfer(struct spi_device *spi, >>>> struct spi_transfer *t) >>>> SIRFSOC_SPI_TRAN_DAT_FORMAT_16; >>>> sspi->rx_word = spi_sirfsoc_rx_word_u16; >>>> sspi->tx_word = spi_sirfsoc_tx_word_u16; >>>> - txfifo_ctrl = SIRFSOC_SPI_FIFO_THD(SIRFSOC_SPI_FIFO_SIZE >>>> / 2) | >>>> - SIRFSOC_SPI_FIFO_WIDTH_WORD; >>>> - rxfifo_ctrl = SIRFSOC_SPI_FIFO_THD(SIRFSOC_SPI_FIFO_SIZE >>>> / 2) | >>>> - SIRFSOC_SPI_FIFO_WIDTH_WORD; >>>> - sspi->word_width = 2; >>>> break; >>>> case 32: >>>> regval |= SIRFSOC_SPI_TRAN_DAT_FORMAT_32; >>>> sspi->rx_word = spi_sirfsoc_rx_word_u32; >>>> sspi->tx_word = spi_sirfsoc_tx_word_u32; >>>> - txfifo_ctrl = SIRFSOC_SPI_FIFO_THD(SIRFSOC_SPI_FIFO_SIZE >>>> / 2) | >>>> - SIRFSOC_SPI_FIFO_WIDTH_DWORD; >>>> - rxfifo_ctrl = SIRFSOC_SPI_FIFO_THD(SIRFSOC_SPI_FIFO_SIZE >>>> / 2) | >>>> - SIRFSOC_SPI_FIFO_WIDTH_DWORD; >>>> - sspi->word_width = 4; >>>> break; >>>> default: >>>> BUG(); >>>> } >>>> >>>> + txfifo_ctrl = SIRFSOC_SPI_FIFO_THD(SIRFSOC_SPI_FIFO_SIZE / 2) | >>>> + SIRFSOC_SPI_FIFO_WIDTH_WORD; >>>> + rxfifo_ctrl = SIRFSOC_SPI_FIFO_THD(SIRFSOC_SPI_FIFO_SIZE / 2) | >>>> + SIRFSOC_SPI_FIFO_WIDTH_WORD; >>>> + sspi->word_width = DIV_ROUND_UP(bits_per_word, 8); >>>> + >> >> Axel, thanks. >> it is a good cleanup, but it seems it is not right for txfifo_ctrl and >> rxfifo_ctrl? > > I don't get it. > I thought there is no functional change with this patch. you might want to change txfifo_ctrl = SIRFSOC_SPI_FIFO_THD(SIRFSOC_SPI_FIFO_SIZE / 2) | SIRFSOC_SPI_FIFO_WIDTH_WORD; rxfifo_ctrl = SIRFSOC_SPI_FIFO_THD(SIRFSOC_SPI_FIFO_SIZE / 2) | SIRFSOC_SPI_FIFO_WIDTH_WORD; to "SIRFSOC_SPI_FIFO_THD(SIRFSOC_SPI_FIFO_SIZE / 2) I sspi->word_width" ? > > Regards, > Axel -barry -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>>> Axel, thanks. >>> it is a good cleanup, but it seems it is not right for txfifo_ctrl and >>> rxfifo_ctrl? >> >> I don't get it. >> I thought there is no functional change with this patch. > > you might want to change > > txfifo_ctrl = SIRFSOC_SPI_FIFO_THD(SIRFSOC_SPI_FIFO_SIZE / 2) | > SIRFSOC_SPI_FIFO_WIDTH_WORD; > rxfifo_ctrl = SIRFSOC_SPI_FIFO_THD(SIRFSOC_SPI_FIFO_SIZE / 2) | > SIRFSOC_SPI_FIFO_WIDTH_WORD; > > to "SIRFSOC_SPI_FIFO_THD(SIRFSOC_SPI_FIFO_SIZE / 2) I sspi->word_width" ? So it is a bug in original code, right? (I mean a bug before applying this patch.) I'll re-generate the patch and separate it to 2 patches. ( one for bug fix which may need for stable, the other one for the clean up) Thanks, Axel -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2014/1/15 Axel Lin <axel.lin@ingics.com>: >>>> Axel, thanks. >>>> it is a good cleanup, but it seems it is not right for txfifo_ctrl and >>>> rxfifo_ctrl? >>> >>> I don't get it. >>> I thought there is no functional change with this patch. >> >> you might want to change >> >> txfifo_ctrl = SIRFSOC_SPI_FIFO_THD(SIRFSOC_SPI_FIFO_SIZE / 2) | >> SIRFSOC_SPI_FIFO_WIDTH_WORD; >> rxfifo_ctrl = SIRFSOC_SPI_FIFO_THD(SIRFSOC_SPI_FIFO_SIZE / 2) | >> SIRFSOC_SPI_FIFO_WIDTH_WORD; >> >> to "SIRFSOC_SPI_FIFO_THD(SIRFSOC_SPI_FIFO_SIZE / 2) I sspi->word_width" ? > So it is a bug in original code, right? (I mean a bug before applying > this patch.) > I'll re-generate the patch and separate it to 2 patches. > ( one for bug fix which may need for stable, the other one for the clean up) no. the original codes are right. because it does orr with: SIRFSOC_SPI_FIFO_WIDTH_BYTE; SIRFSOC_SPI_FIFO_WIDTH_WORD; SIRFSOC_SPI_FIFO_WIDTH_DWORD; but you move to orr SIRFSOC_SPI_FIFO_WIDTH_WORD actually, the register set is fortunately same with the real width. > > Thanks, > Axel -barry -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2014/1/15 Axel Lin <axel.lin@ingics.com>: >>>> Axel, thanks. >>>> it is a good cleanup, but it seems it is not right for txfifo_ctrl and >>>> rxfifo_ctrl? >>> >>> I don't get it. >>> I thought there is no functional change with this patch. >> >> you might want to change >> >> txfifo_ctrl = SIRFSOC_SPI_FIFO_THD(SIRFSOC_SPI_FIFO_SIZE / 2) | >> SIRFSOC_SPI_FIFO_WIDTH_WORD; >> rxfifo_ctrl = SIRFSOC_SPI_FIFO_THD(SIRFSOC_SPI_FIFO_SIZE / 2) | >> SIRFSOC_SPI_FIFO_WIDTH_WORD; >> >> to "SIRFSOC_SPI_FIFO_THD(SIRFSOC_SPI_FIFO_SIZE / 2) I sspi->word_width" ? > So it is a bug in original code, right? (I mean a bug before applying > this patch.) > I'll re-generate the patch and separate it to 2 patches. > ( one for bug fix which may need for stable, the other one for the clean up) Sorry. Current code is ok. But my patch breaks things. Pls just ignore the patch. Regards, Axel -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2014/1/15 Barry Song <21cnbao@gmail.com>: > 2014/1/15 Axel Lin <axel.lin@ingics.com>: >>>>> Axel, thanks. >>>>> it is a good cleanup, but it seems it is not right for txfifo_ctrl and >>>>> rxfifo_ctrl? >>>> >>>> I don't get it. >>>> I thought there is no functional change with this patch. >>> >>> you might want to change >>> >>> txfifo_ctrl = SIRFSOC_SPI_FIFO_THD(SIRFSOC_SPI_FIFO_SIZE / 2) | >>> SIRFSOC_SPI_FIFO_WIDTH_WORD; >>> rxfifo_ctrl = SIRFSOC_SPI_FIFO_THD(SIRFSOC_SPI_FIFO_SIZE / 2) | >>> SIRFSOC_SPI_FIFO_WIDTH_WORD; >>> >>> to "SIRFSOC_SPI_FIFO_THD(SIRFSOC_SPI_FIFO_SIZE / 2) I sspi->word_width" ? >> So it is a bug in original code, right? (I mean a bug before applying >> this patch.) >> I'll re-generate the patch and separate it to 2 patches. >> ( one for bug fix which may need for stable, the other one for the clean up) > > no. the original codes are right. because it does orr with: > SIRFSOC_SPI_FIFO_WIDTH_BYTE; > SIRFSOC_SPI_FIFO_WIDTH_WORD; > SIRFSOC_SPI_FIFO_WIDTH_DWORD; > > but you move to orr > SIRFSOC_SPI_FIFO_WIDTH_WORD > > actually, the register set is fortunately same with the real width. Yes, I see it now. v2 is on the way. Thanks for the review, Axel -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/spi/spi-sirf.c b/drivers/spi/spi-sirf.c index ed5e501..787683d 100644 --- a/drivers/spi/spi-sirf.c +++ b/drivers/spi/spi-sirf.c @@ -459,11 +459,6 @@ spi_sirfsoc_setup_transfer(struct spi_device *spi, struct spi_transfer *t) regval |= SIRFSOC_SPI_TRAN_DAT_FORMAT_8; sspi->rx_word = spi_sirfsoc_rx_word_u8; sspi->tx_word = spi_sirfsoc_tx_word_u8; - txfifo_ctrl = SIRFSOC_SPI_FIFO_THD(SIRFSOC_SPI_FIFO_SIZE / 2) | - SIRFSOC_SPI_FIFO_WIDTH_BYTE; - rxfifo_ctrl = SIRFSOC_SPI_FIFO_THD(SIRFSOC_SPI_FIFO_SIZE / 2) | - SIRFSOC_SPI_FIFO_WIDTH_BYTE; - sspi->word_width = 1; break; case 12: case 16: @@ -471,26 +466,22 @@ spi_sirfsoc_setup_transfer(struct spi_device *spi, struct spi_transfer *t) SIRFSOC_SPI_TRAN_DAT_FORMAT_16; sspi->rx_word = spi_sirfsoc_rx_word_u16; sspi->tx_word = spi_sirfsoc_tx_word_u16; - txfifo_ctrl = SIRFSOC_SPI_FIFO_THD(SIRFSOC_SPI_FIFO_SIZE / 2) | - SIRFSOC_SPI_FIFO_WIDTH_WORD; - rxfifo_ctrl = SIRFSOC_SPI_FIFO_THD(SIRFSOC_SPI_FIFO_SIZE / 2) | - SIRFSOC_SPI_FIFO_WIDTH_WORD; - sspi->word_width = 2; break; case 32: regval |= SIRFSOC_SPI_TRAN_DAT_FORMAT_32; sspi->rx_word = spi_sirfsoc_rx_word_u32; sspi->tx_word = spi_sirfsoc_tx_word_u32; - txfifo_ctrl = SIRFSOC_SPI_FIFO_THD(SIRFSOC_SPI_FIFO_SIZE / 2) | - SIRFSOC_SPI_FIFO_WIDTH_DWORD; - rxfifo_ctrl = SIRFSOC_SPI_FIFO_THD(SIRFSOC_SPI_FIFO_SIZE / 2) | - SIRFSOC_SPI_FIFO_WIDTH_DWORD; - sspi->word_width = 4; break; default: BUG(); } + txfifo_ctrl = SIRFSOC_SPI_FIFO_THD(SIRFSOC_SPI_FIFO_SIZE / 2) | + SIRFSOC_SPI_FIFO_WIDTH_WORD; + rxfifo_ctrl = SIRFSOC_SPI_FIFO_THD(SIRFSOC_SPI_FIFO_SIZE / 2) | + SIRFSOC_SPI_FIFO_WIDTH_WORD; + sspi->word_width = DIV_ROUND_UP(bits_per_word, 8); + if (!(spi->mode & SPI_CS_HIGH)) regval |= SIRFSOC_SPI_CS_IDLE_STAT; if (!(spi->mode & SPI_LSB_FIRST))
Trivial cleanup to avoid duplicate code in various bits_per_word cases. Signed-off-by: Axel Lin <axel.lin@ingics.com> --- drivers/spi/spi-sirf.c | 21 ++++++--------------- 1 file changed, 6 insertions(+), 15 deletions(-)