Re: [PATCH 3/3] spi: qcom: geni: handle timeout for gpi mode

[Doug Anderson <dianders@xxxxxxxxxxxx>]

Hi,

On Wed, Nov 17, 2021 at 5:31 AM Vinod Koul <vkoul@xxxxxxxxxx> wrote:
>
> We missed adding handle_err for gpi mode, so add a new function
> spi_geni_handle_err() which would call handle_fifo_timeout() or newly
> added handle_gpi_timeout() based on mode
>
> Signed-off-by: Vinod Koul <vkoul@xxxxxxxxxx>
> ---
>  drivers/spi/spi-geni-qcom.c | 26 +++++++++++++++++++++++++-
>  1 file changed, 25 insertions(+), 1 deletion(-)

Fixes: b59c122484ec ("spi: spi-geni-qcom: Add support for GPI dma")
Reported-by: Douglas Anderson <dianders@xxxxxxxxxxxx>
Reviewed-by: Douglas Anderson <dianders@xxxxxxxxxxxx>


> diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c
> index b9769de1f5f0..5b6e9a6643d8 100644
> --- a/drivers/spi/spi-geni-qcom.c
> +++ b/drivers/spi/spi-geni-qcom.c
> @@ -164,6 +164,30 @@ static void handle_fifo_timeout(struct spi_master *spi,
>         }
>  }
>
> +static void handle_gpi_timeout(struct spi_master *spi, struct spi_message *msg)
> +{
> +       struct spi_geni_master *mas = spi_master_get_devdata(spi);
> +
> +       dmaengine_terminate_sync(mas->tx);
> +       dmaengine_terminate_sync(mas->rx);
> +}
> +
> +static void spi_geni_handle_err(struct spi_master *spi, struct spi_message *msg)
> +{
> +       struct spi_geni_master *mas = spi_master_get_devdata(spi);
> +
> +       switch (mas->cur_xfer_mode) {
> +       case GENI_SE_FIFO:
> +               handle_fifo_timeout(spi, msg);
> +               break;
> +       case GENI_GPI_DMA:
> +               handle_gpi_timeout(spi, msg);

Slight nit that maybe you should call it handle_gpi_err() instead of
handle_gpi_timeout(). As I understand it this function will get called
for _all_ errors, including errors reported by
spi_gsi_callback_result(). So basically if you have residue then
you'll immediately finalize the transfer with -EIO in the status and
then spi_geni_handle_err() will get called. It seems a little strange
that it then goes and calls a function whose name makes it sound as if
it's only called for "timeout". (For the FIFO case we actually only
hit this for timeouts since we don't currently terminate transfers
early for errors, so the FIFO name is actually OK).

-Doug