linux-kernel - Re: [PATCH 6/5] spi: spi-geni-qcom: Simplify setup_fifo

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAD=FV=UwXKQcDPhqb0j+x-+kU1ZdBjVx52aY1JHx6oJRMLUPrg@mail.gmail.com>
Date:   Thu, 18 Jun 2020 17:40:59 -0700
From:   Doug Anderson <dianders@...omium.org>
To:     Stephen Boyd <swboyd@...omium.org>
Cc:     Mark Brown <broonie@...nel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Alok Chauhan <alokc@...eaurora.org>,
        linux-arm-msm <linux-arm-msm@...r.kernel.org>,
        linux-spi <linux-spi@...r.kernel.org>
Subject: Re: [PATCH 6/5] spi: spi-geni-qcom: Simplify setup_fifo_xfer()

Hi,

On Thu, Jun 18, 2020 at 4:40 PM Stephen Boyd <swboyd@...omium.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@...omium.org>
> Signed-off-by: Stephen Boyd <swboyd@...omium.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@...omium.org>



-Doug