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

[Serge Semin <fancer.lancer@xxxxxxxxx>]

On Fri, Jun 10, 2022 at 02:22:28PM +0300, Andy Shevchenko wrote:
> 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>

Thanks.

> 
> > 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...

Right, but it makes this method looking alike dw_spi_dma_init_mfld().
I even used the same label names. It makes the code a little bit easier
to read.

> 
> > +	}
> > +
> > +	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.

ditto

-Sergey

> 
> > +	return ret;
> >  }
> >  
> >  static void dw_spi_dma_exit(struct dw_spi *dws)
> > -- 
> > 2.35.1
> > 
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
>