| Message ID | 20200618233959.160032-1-swboyd@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | spi: spi-geni-qcom: Fixes / perf improvements | expand |
Hi, On Thu, Jun 18, 2020 at 4:40 PM Stephen Boyd <swboyd@chromium.org> wrote: > > The definition of SPI_FULL_DUPLEX (3) is really SPI_TX_ONLY (1) ORed > with SPI_RX_ONLY (2). Let's drop the define and simplify the code here a > bit by collapsing the setting of 'm_cmd' into conditions that are the > same. > > This is a non-functional change, just cleanup to consolidate code. > > Cc: Douglas Anderson <dianders@chromium.org> > Signed-off-by: Stephen Boyd <swboyd@chromium.org> > --- > drivers/spi/spi-geni-qcom.c | 13 ++++--------- > 1 file changed, 4 insertions(+), 9 deletions(-) > > diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c > index 636c3da15db0..670f83793aa4 100644 > --- a/drivers/spi/spi-geni-qcom.c > +++ b/drivers/spi/spi-geni-qcom.c > @@ -51,7 +51,6 @@ > /* M_CMD OP codes for SPI */ > #define SPI_TX_ONLY 1 > #define SPI_RX_ONLY 2 > -#define SPI_FULL_DUPLEX 3 > #define SPI_TX_RX 7 > #define SPI_CS_ASSERT 8 > #define SPI_CS_DEASSERT 9 > @@ -357,12 +356,6 @@ static void setup_fifo_xfer(struct spi_transfer *xfer, > > mas->tx_rem_bytes = 0; > mas->rx_rem_bytes = 0; > - if (xfer->tx_buf && xfer->rx_buf) > - m_cmd = SPI_FULL_DUPLEX; > - else if (xfer->tx_buf) > - m_cmd = SPI_TX_ONLY; > - else if (xfer->rx_buf) > - m_cmd = SPI_RX_ONLY; > > spi_tx_cfg &= ~CS_TOGGLE; > > @@ -373,12 +366,14 @@ static void setup_fifo_xfer(struct spi_transfer *xfer, > len &= TRANS_LEN_MSK; > > mas->cur_xfer = xfer; > - if (m_cmd & SPI_TX_ONLY) { > + if (xfer->tx_buf) { > + m_cmd |= SPI_TX_ONLY; > mas->tx_rem_bytes = xfer->len; > writel(len, se->base + SE_SPI_TX_TRANS_LEN); > } > > - if (m_cmd & SPI_RX_ONLY) { > + if (xfer->rx_buf) { > + m_cmd |= SPI_RX_ONLY; If you're going to touch this, could you change "SPI_TX_ONLY" to "SPI_TX_ENABLED" and "SPI_RX_ONLY" to 'SPI_RX_ENABLED". It felt really weird to me that if you had full duplex you were setting "RX_ONLY" and "TX_ONLY". Other than that, your change is nice and cleans things up a bit, so even if you don't do the extra cleanup: Reviewed-by: Douglas Anderson <dianders@chromium.org> -Doug
On Thu, Jun 18, 2020 at 04:39:58PM -0700, Stephen Boyd wrote: > The definition of SPI_FULL_DUPLEX (3) is really SPI_TX_ONLY (1) ORed > with SPI_RX_ONLY (2). Let's drop the define and simplify the code here a > bit by collapsing the setting of 'm_cmd' into conditions that are the > same. Please don't add extra patches after someone else's series like this, it makes things harder to follow and really confuses tooling which tries to parse serieses off the list. Just send a separate series.
Quoting Doug Anderson (2020-06-18 17:40:59) > On Thu, Jun 18, 2020 at 4:40 PM Stephen Boyd <swboyd@chromium.org> wrote: > > @@ -373,12 +366,14 @@ static void setup_fifo_xfer(struct spi_transfer *xfer, > > len &= TRANS_LEN_MSK; > > > > mas->cur_xfer = xfer; > > - if (m_cmd & SPI_TX_ONLY) { > > + if (xfer->tx_buf) { > > + m_cmd |= SPI_TX_ONLY; > > mas->tx_rem_bytes = xfer->len; > > writel(len, se->base + SE_SPI_TX_TRANS_LEN); > > } > > > > - if (m_cmd & SPI_RX_ONLY) { > > + if (xfer->rx_buf) { > > + m_cmd |= SPI_RX_ONLY; > > If you're going to touch this, could you change "SPI_TX_ONLY" to > "SPI_TX_ENABLED" and "SPI_RX_ONLY" to 'SPI_RX_ENABLED". It felt > really weird to me that if you had full duplex you were setting > "RX_ONLY" and "TX_ONLY". I agree, except in the register documentation it is called "TX only", "RX only" and "Full-Duplex". So I'd rather leave it alone. > > Other than that, your change is nice and cleans things up a bit, so > even if you don't do the extra cleanup: > > Reviewed-by: Douglas Anderson <dianders@chromium.org> > > Thanks.
diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c index 636c3da15db0..670f83793aa4 100644 --- a/drivers/spi/spi-geni-qcom.c +++ b/drivers/spi/spi-geni-qcom.c @@ -51,7 +51,6 @@ /* M_CMD OP codes for SPI */ #define SPI_TX_ONLY 1 #define SPI_RX_ONLY 2 -#define SPI_FULL_DUPLEX 3 #define SPI_TX_RX 7 #define SPI_CS_ASSERT 8 #define SPI_CS_DEASSERT 9 @@ -357,12 +356,6 @@ static void setup_fifo_xfer(struct spi_transfer *xfer, mas->tx_rem_bytes = 0; mas->rx_rem_bytes = 0; - if (xfer->tx_buf && xfer->rx_buf) - m_cmd = SPI_FULL_DUPLEX; - else if (xfer->tx_buf) - m_cmd = SPI_TX_ONLY; - else if (xfer->rx_buf) - m_cmd = SPI_RX_ONLY; spi_tx_cfg &= ~CS_TOGGLE; @@ -373,12 +366,14 @@ static void setup_fifo_xfer(struct spi_transfer *xfer, len &= TRANS_LEN_MSK; mas->cur_xfer = xfer; - if (m_cmd & SPI_TX_ONLY) { + if (xfer->tx_buf) { + m_cmd |= SPI_TX_ONLY; mas->tx_rem_bytes = xfer->len; writel(len, se->base + SE_SPI_TX_TRANS_LEN); } - if (m_cmd & SPI_RX_ONLY) { + if (xfer->rx_buf) { + m_cmd |= SPI_RX_ONLY; writel(len, se->base + SE_SPI_RX_TRANS_LEN); mas->rx_rem_bytes = xfer->len; }
The definition of SPI_FULL_DUPLEX (3) is really SPI_TX_ONLY (1) ORed with SPI_RX_ONLY (2). Let's drop the define and simplify the code here a bit by collapsing the setting of 'm_cmd' into conditions that are the same. This is a non-functional change, just cleanup to consolidate code. Cc: Douglas Anderson <dianders@chromium.org> Signed-off-by: Stephen Boyd <swboyd@chromium.org> --- drivers/spi/spi-geni-qcom.c | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-)