Re: [PATCH v3 1/1] spi: Using Trigger number to transmit/receive data

[Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>]

Hi Hiep-san,

On Thu, Oct 23, 2014 at 5:14 AM, Cao Minh Hiep <cm-hiep@xxxxxxxxxxx> wrote:
> From: Hiep Cao Minh <cm-hiep@xxxxxxxxxxx>
>
> In order to transmit and receive data when have 32 bytes of data that
> ready has prepared on Transmit/Receive Buffer to transmit or receive.
> Instead transmits/receives a byte data using Transmit/Receive Buffer
> Data Triggering Number will improve the speed of transfer data.

Please accept my apologies for these late review comments.
I had completely forgotten about your patch, until I noticed Mark had
applied it.

> --- a/drivers/spi/spi-rspi.c
> +++ b/drivers/spi/spi-rspi.c
> @@ -614,19 +667,29 @@ static bool rspi_can_dma(struct spi_master *master, struct spi_device *spi,
>         return __rspi_can_dma(rspi, xfer);
>  }
>
> -static int rspi_common_transfer(struct rspi_data *rspi,
> -                               struct spi_transfer *xfer)
> +static int rspi_dma_check_then_transfer(struct rspi_data *rspi,
> +                                        struct spi_transfer *xfer)
>  {
> -       int ret;
> -
>         if (rspi->master->can_dma && __rspi_can_dma(rspi, xfer)) {
>                 /* rx_buf can be NULL on RSPI on SH in TX-only Mode */
> -               ret = rspi_dma_transfer(rspi, &xfer->tx_sg,
> +               int ret = rspi_dma_transfer(rspi, &xfer->tx_sg,
>                                         xfer->rx_buf ? &xfer->rx_sg : NULL);
>                 if (ret != -EAGAIN)
> -                       return ret;
> +                       return 0;

Shouldn't you propagate "ret" here, instead of returning zero?
Else you will turn a failure into a success below (see "here").

>         }
>
> +       return -EAGAIN;
> +}
> +
> +static int rspi_common_transfer(struct rspi_data *rspi,
> +                               struct spi_transfer *xfer)
> +{
> +       int ret;
> +
> +       ret = rspi_dma_check_then_transfer(rspi, xfer);
> +       if (ret != -EAGAIN)
> +               return ret;

... here.

> +
>         ret = rspi_pio_transfer(rspi, xfer->tx_buf, xfer->rx_buf, xfer->len);
>         if (ret < 0)
>                 return ret;
> @@ -666,12 +729,59 @@ static int rspi_rz_transfer_one(struct spi_master *master,
>         return rspi_common_transfer(rspi, xfer);
>  }
>
> +static int qspi_trigger_transfer_out_int(struct rspi_data *rspi, const u8 *tx,

I guess this should be called "qspi_trigger_transfer_out_in" (without "t")?

> +                                       u8 *rx, unsigned int len)

>  static int qspi_transfer_out_in(struct rspi_data *rspi,
>                                 struct spi_transfer *xfer)
>  {
> +       int ret;
> +
>         qspi_receive_init(rspi);
>
> -       return rspi_common_transfer(rspi, xfer);
> +       ret = rspi_dma_check_then_transfer(rspi, xfer);
> +       if (ret != -EAGAIN)
> +               return ret;
> +
> +       ret = qspi_trigger_transfer_out_int(rspi, xfer->tx_buf,
> +                                           xfer->rx_buf, xfer->len);
> +       if (ret < 0)
> +               return ret;
> +
> +       return 0;

You could just write

      return qspi_trigger_transfer_out_int(rspi, xfer->tx_buf,
                                           xfer->rx_buf, xfer->len);

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html