Re: [PATCH v2 10/14] mmc: jz4740: Use dma_request_chan()

[Paul Cercueil <paul@crapouillou.net>]

Le jeu. 15 mars 2018 à 18:12, Ezequiel Garcia 
<ezequiel@collabora.co.uk> a écrit :
> On Thu, 2018-03-15 at 17:59 -0300, Paul Cercueil wrote:
>>  Hi,
>> 
>>  Le lun. 12 mars 2018 à 18:55, Ezequiel Garcia
>>  <ezequiel@vanguardiasur.com.ar> a écrit :
>>  > From: Ezequiel Garcia <ezequiel@collabora.co.uk>
>>  >
>>  > Replace dma_request_channel() with dma_request_chan(),
>>  > which also supports probing from the devicetree.
>>  >
>>  > Tested-by: Mathieu Malaterre <malat@debian.org>
>>  > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.co.uk>
>>  > ---
>>  >  drivers/mmc/host/jz4740_mmc.c | 22 +++++++---------------
>>  >  1 file changed, 7 insertions(+), 15 deletions(-)
>>  >
>>  > diff --git a/drivers/mmc/host/jz4740_mmc.c
>>  > b/drivers/mmc/host/jz4740_mmc.c
>>  > index c3ec8e662706..37183fe32ef8 100644
>>  > --- a/drivers/mmc/host/jz4740_mmc.c
>>  > +++ b/drivers/mmc/host/jz4740_mmc.c
>>  > @@ -225,31 +225,23 @@ static void
>>  > jz4740_mmc_release_dma_channels(struct jz4740_mmc_host *host)
>>  >
>>  >  static int jz4740_mmc_acquire_dma_channels(struct jz4740_mmc_host
>>  > *host)
>>  >  {
>>  > -	dma_cap_mask_t mask;
>>  > -
>>  > -	dma_cap_zero(mask);
>>  > -	dma_cap_set(DMA_SLAVE, mask);
>>  > -
>>  > -	host->dma_tx = dma_request_channel(mask, NULL, host);
>>  > -	if (!host->dma_tx) {
>>  > +	host->dma_tx = dma_request_chan(mmc_dev(host->mmc), "tx");
>>  > +	if (IS_ERR(host->dma_tx)) {
>>  >  		dev_err(mmc_dev(host->mmc), "Failed to get dma_tx
>>  > channel\n");
>>  > -		return -ENODEV;
>>  > +		return PTR_ERR(host->dma_tx);
>>  >  	}
>>  >
>>  > -	host->dma_rx = dma_request_channel(mask, NULL, host);
>>  > -	if (!host->dma_rx) {
>>  > +	host->dma_rx = dma_request_chan(mmc_dev(host->mmc), "rx");
>> 
>>  I suspect this breaks on jz4740... Did you test?
>> 
> 
> No, but code inspecting I was expecting it wouldn't break anything.
> dma_request_channel() searches for a slave channel, via the DMA_SLAVE
> mask that the driver sets.
> 
> dma_request_chan() seems to fallback to do the same.

Alright, I overlooked that. I guess it's fine then.

> struct dma_chan *dma_request_chan(struct device *dev, const char 
> *name)
> {
>         struct dma_device *d, *_d;
>         struct dma_chan *chan = NULL;
> 
>         /* If device-tree is present get slave info from here */
>         if (dev->of_node)
>                 chan = of_dma_request_slave_channel(dev->of_node,
> name);
> 
>         /* ... */
> 
>         if (chan) {
>                 /* Valid channel found or requester need to be 
> deferred
> */
>                 if (!IS_ERR(chan) || PTR_ERR(chan) == -EPROBE_DEFER)
>                         return chan;
>         }
> 
>         /* Try to find the channel via the DMA filter map(s) */
>         mutex_lock(&dma_list_mutex);
>         list_for_each_entry_safe(d, _d, &dma_device_list, global_node)
> {
>                 dma_cap_mask_t mask;
>                 const struct dma_slave_map *map = dma_filter_match(d,
> name, dev);
> 
>                 if (!map)
>                         continue;
> 
>                 dma_cap_zero(mask);
>                 dma_cap_set(DMA_SLAVE, mask);
> 
>                 chan = find_candidate(d, &mask, d->filter.fn, map-
>> param);
>                 if (!IS_ERR(chan))
>                         break;
>         }
>         mutex_unlock(&dma_list_mutex);
> 
>         return chan ? chan : ERR_PTR(-EPROBE_DEFER);
> }
> 
> Unfortunately, I don't have anything but a jz4780 Ci20, so can't 
> really
> test this.

Apart from this patch, your patchset looks sufficiently similar to what 
I
successfully tested on jz4740, so it's safe to assume it doesn't break 
anything.
Unfortunately I won't be able to test it on jz4740 before the end of 
this year.

> Thanks,
> Eze