Re: [PATCH] spi: dw: Add deferred DMA-channels setup support

[Andy Shevchenko <andy.shevchenko@xxxxxxxxx>]

On Fri, Jun 10, 2022 at 10:50:06AM +0300, Serge Semin wrote:
> Currently if the source DMA device isn't ready to provide the channels
> capable of the SPI DMA transfers, the DW SSI controller will be registered
> with no DMA support. It isn't right since all what the driver needs to do
> is to postpone the probe procedure until the DMA device is ready. Let's
> fix that in the framework of the DWC SSI generic DMA implementation. First
> we need to use the dma_request_chan() method instead of the
> dma_request_slave_channel() function, because the later one is deprecated
> and most importantly doesn't return the failure cause but the
> NULL-pointer. Second we need to stop the DW SSI controller probe procedure
> if the -EPROBE_DEFER error is returned on the DMA initialization. The
> procedure will resume later when the channels are ready to be requested.

One nit-pick below
Reviewed-by: Andy Shevchenko <andy.shevchenko@xxxxxxxxx>

> Signed-off-by: Serge Semin <Sergey.Semin@xxxxxxxxxxxxxxxxxxxx>
> ---
>  drivers/spi/spi-dw-core.c |  5 ++++-
>  drivers/spi/spi-dw-dma.c  | 25 ++++++++++++++++++-------
>  2 files changed, 22 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/spi/spi-dw-core.c b/drivers/spi/spi-dw-core.c
> index ecea471ff42c..911ea9bddbee 100644
> --- a/drivers/spi/spi-dw-core.c
> +++ b/drivers/spi/spi-dw-core.c
> @@ -942,7 +942,9 @@ int dw_spi_add_host(struct device *dev, struct dw_spi *dws)
>  
>  	if (dws->dma_ops && dws->dma_ops->dma_init) {
>  		ret = dws->dma_ops->dma_init(dev, dws);
> -		if (ret) {
> +		if (ret == -EPROBE_DEFER) {
> +			goto err_free_irq;
> +		} else if (ret) {
>  			dev_warn(dev, "DMA init failed\n");
>  		} else {
>  			master->can_dma = dws->dma_ops->can_dma;
> @@ -963,6 +965,7 @@ int dw_spi_add_host(struct device *dev, struct dw_spi *dws)
>  	if (dws->dma_ops && dws->dma_ops->dma_exit)
>  		dws->dma_ops->dma_exit(dws);
>  	dw_spi_enable_chip(dws, 0);
> +err_free_irq:
>  	free_irq(dws->irq, master);
>  err_free_master:
>  	spi_controller_put(master);
> diff --git a/drivers/spi/spi-dw-dma.c b/drivers/spi/spi-dw-dma.c
> index 63e5260100ec..1322b8cce5b7 100644
> --- a/drivers/spi/spi-dw-dma.c
> +++ b/drivers/spi/spi-dw-dma.c
> @@ -139,15 +139,20 @@ static int dw_spi_dma_init_mfld(struct device *dev, struct dw_spi *dws)
>  
>  static int dw_spi_dma_init_generic(struct device *dev, struct dw_spi *dws)
>  {
> -	dws->rxchan = dma_request_slave_channel(dev, "rx");
> -	if (!dws->rxchan)
> -		return -ENODEV;
> +	int ret;
>  
> -	dws->txchan = dma_request_slave_channel(dev, "tx");
> -	if (!dws->txchan) {
> -		dma_release_channel(dws->rxchan);
> +	dws->rxchan = dma_request_chan(dev, "rx");
> +	if (IS_ERR(dws->rxchan)) {
> +		ret = PTR_ERR(dws->rxchan);
>  		dws->rxchan = NULL;

> -		return -ENODEV;
> +		goto err_exit;

This change doesn't bring anything...

> +	}
> +
> +	dws->txchan = dma_request_chan(dev, "tx");
> +	if (IS_ERR(dws->txchan)) {
> +		ret = PTR_ERR(dws->txchan);
> +		dws->txchan = NULL;
> +		goto free_rxchan;
>  	}
>  
>  	dws->master->dma_rx = dws->rxchan;
> @@ -160,6 +165,12 @@ static int dw_spi_dma_init_generic(struct device *dev, struct dw_spi *dws)
>  	dw_spi_dma_sg_burst_init(dws);
>  
>  	return 0;
> +
> +free_rxchan:
> +	dma_release_channel(dws->rxchan);
> +	dws->rxchan = NULL;

> +err_exit:

...and this too.

> +	return ret;
>  }
>  
>  static void dw_spi_dma_exit(struct dw_spi *dws)
> -- 
> 2.35.1
> 

-- 
With Best Regards,
Andy Shevchenko