diff mbox

[RFC,v02,02/15] dmaengine: core: Move and merge the code paths using private_candidate

Message ID 1448891145-10766-3-git-send-email-peter.ujfalusi@ti.com (mailing list archive)
State New, archived
Headers show

Commit Message

Peter Ujfalusi Nov. 30, 2015, 1:45 p.m. UTC
Channel matching with private_candidate() is used in two paths, the error
checking is slightly different in them and they are duplicating code also.
Move the code under dma_get_channel() to provide consistent execution and
going to allow us to reuse this mode of channel lookup later.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 drivers/dma/dmaengine.c | 81 +++++++++++++++++++++++++------------------------
 1 file changed, 42 insertions(+), 39 deletions(-)

Comments

Andy Shevchenko Nov. 30, 2015, 2:42 p.m. UTC | #1
On Mon, Nov 30, 2015 at 3:45 PM, Peter Ujfalusi <peter.ujfalusi@ti.com> wrote:
> Channel matching with private_candidate() is used in two paths, the error
> checking is slightly different in them and they are duplicating code also.
> Move the code under dma_get_channel() to provide consistent execution and
> going to allow us to reuse this mode of channel lookup later.
>
> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
> ---
>  drivers/dma/dmaengine.c | 81 +++++++++++++++++++++++++------------------------
>  1 file changed, 42 insertions(+), 39 deletions(-)
>
> diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c
> index 52c3eee48e2e..1249165fb4b2 100644
> --- a/drivers/dma/dmaengine.c
> +++ b/drivers/dma/dmaengine.c
> @@ -549,6 +549,42 @@ static struct dma_chan *private_candidate(const dma_cap_mask_t *mask,
>         return NULL;
>  }
>
> +static struct dma_chan *dma_get_channel(struct dma_device *device,

Naming scheme inside dmaengine.c looks like a mess.

Since it's static function that utilizes private_candidate() may I
propose the name like find_candidate() ?

> +                                       const dma_cap_mask_t *mask,
> +                                       dma_filter_fn fn, void *fn_param)
> +{
> +       struct dma_chan *chan = private_candidate(mask, device, fn, fn_param);
> +       int err;
> +
> +       if (chan) {
> +               /* Found a suitable channel, try to grab, prep, and return it.
> +                * We first set DMA_PRIVATE to disable balance_ref_count as this
> +                * channel will not be published in the general-purpose
> +                * allocator
> +                */
> +               dma_cap_set(DMA_PRIVATE, device->cap_mask);
> +               device->privatecnt++;
> +               err = dma_chan_get(chan);
> +
> +               if (err) {
> +                       if (err == -ENODEV) {
> +                               pr_debug("%s: %s module removed\n", __func__,
> +                                        dma_chan_name(chan));
> +                               list_del_rcu(&device->global_node);
> +                       } else
> +                               pr_debug("%s: failed to get %s: (%d)\n",
> +                                        __func__, dma_chan_name(chan), err);
> +
> +                       if (--device->privatecnt == 0)
> +                               dma_cap_clear(DMA_PRIVATE, device->cap_mask);
> +
> +                       chan = ERR_PTR(err);
> +               }
> +       }
> +
> +       return chan ? chan : ERR_PTR(-EPROBE_DEFER);
> +}
> +
>  /**
>   * dma_get_slave_channel - try to get specific channel exclusively
>   * @chan: target channel
> @@ -587,7 +623,6 @@ struct dma_chan *dma_get_any_slave_channel(struct dma_device *device)
>  {
>         dma_cap_mask_t mask;
>         struct dma_chan *chan;
> -       int err;
>
>         dma_cap_zero(mask);
>         dma_cap_set(DMA_SLAVE, mask);
> @@ -595,23 +630,11 @@ struct dma_chan *dma_get_any_slave_channel(struct dma_device *device)
>         /* lock against __dma_request_channel */
>         mutex_lock(&dma_list_mutex);
>
> -       chan = private_candidate(&mask, device, NULL, NULL);
> -       if (chan) {
> -               dma_cap_set(DMA_PRIVATE, device->cap_mask);
> -               device->privatecnt++;
> -               err = dma_chan_get(chan);
> -               if (err) {
> -                       pr_debug("%s: failed to get %s: (%d)\n",
> -                               __func__, dma_chan_name(chan), err);
> -                       chan = NULL;
> -                       if (--device->privatecnt == 0)
> -                               dma_cap_clear(DMA_PRIVATE, device->cap_mask);
> -               }
> -       }
> +       chan = dma_get_channel(device, &mask, NULL, NULL);
>
>         mutex_unlock(&dma_list_mutex);
>
> -       return chan;
> +       return IS_ERR(chan) ? NULL : chan;
>  }
>  EXPORT_SYMBOL_GPL(dma_get_any_slave_channel);
>
> @@ -628,35 +651,15 @@ struct dma_chan *__dma_request_channel(const dma_cap_mask_t *mask,
>  {
>         struct dma_device *device, *_d;
>         struct dma_chan *chan = NULL;
> -       int err;
>
>         /* Find a channel */
>         mutex_lock(&dma_list_mutex);
>         list_for_each_entry_safe(device, _d, &dma_device_list, global_node) {
> -               chan = private_candidate(mask, device, fn, fn_param);
> -               if (chan) {
> -                       /* Found a suitable channel, try to grab, prep, and
> -                        * return it.  We first set DMA_PRIVATE to disable
> -                        * balance_ref_count as this channel will not be
> -                        * published in the general-purpose allocator
> -                        */
> -                       dma_cap_set(DMA_PRIVATE, device->cap_mask);
> -                       device->privatecnt++;
> -                       err = dma_chan_get(chan);
> +               chan = dma_get_channel(device, mask, fn, fn_param);
> +               if (!IS_ERR(chan))
> +                       break;
>
> -                       if (err == -ENODEV) {
> -                               pr_debug("%s: %s module removed\n",
> -                                        __func__, dma_chan_name(chan));
> -                               list_del_rcu(&device->global_node);
> -                       } else if (err)
> -                               pr_debug("%s: failed to get %s: (%d)\n",
> -                                        __func__, dma_chan_name(chan), err);
> -                       else
> -                               break;
> -                       if (--device->privatecnt == 0)
> -                               dma_cap_clear(DMA_PRIVATE, device->cap_mask);
> -                       chan = NULL;
> -               }
> +               chan = NULL;
>         }
>         mutex_unlock(&dma_list_mutex);
>
> --
> 2.6.3
>
Peter Ujfalusi Dec. 1, 2015, 8:41 a.m. UTC | #2
On 11/30/2015 04:42 PM, Andy Shevchenko wrote:
> On Mon, Nov 30, 2015 at 3:45 PM, Peter Ujfalusi <peter.ujfalusi@ti.com> wrote:
>> Channel matching with private_candidate() is used in two paths, the error
>> checking is slightly different in them and they are duplicating code also.
>> Move the code under dma_get_channel() to provide consistent execution and
>> going to allow us to reuse this mode of channel lookup later.
>>
>> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
>> ---
>>  drivers/dma/dmaengine.c | 81 +++++++++++++++++++++++++------------------------
>>  1 file changed, 42 insertions(+), 39 deletions(-)
>>
>> diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c
>> index 52c3eee48e2e..1249165fb4b2 100644
>> --- a/drivers/dma/dmaengine.c
>> +++ b/drivers/dma/dmaengine.c
>> @@ -549,6 +549,42 @@ static struct dma_chan *private_candidate(const dma_cap_mask_t *mask,
>>         return NULL;
>>  }
>>
>> +static struct dma_chan *dma_get_channel(struct dma_device *device,
> 
> Naming scheme inside dmaengine.c looks like a mess.

Yes, I agree.

> 
> Since it's static function that utilizes private_candidate() may I
> propose the name like find_candidate() ?

I had __* version as well, but the find_candidate() sounds better.

Thanks,
Péter
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c
index 52c3eee48e2e..1249165fb4b2 100644
--- a/drivers/dma/dmaengine.c
+++ b/drivers/dma/dmaengine.c
@@ -549,6 +549,42 @@  static struct dma_chan *private_candidate(const dma_cap_mask_t *mask,
 	return NULL;
 }
 
+static struct dma_chan *dma_get_channel(struct dma_device *device,
+					const dma_cap_mask_t *mask,
+					dma_filter_fn fn, void *fn_param)
+{
+	struct dma_chan *chan = private_candidate(mask, device, fn, fn_param);
+	int err;
+
+	if (chan) {
+		/* Found a suitable channel, try to grab, prep, and return it.
+		 * We first set DMA_PRIVATE to disable balance_ref_count as this
+		 * channel will not be published in the general-purpose
+		 * allocator
+		 */
+		dma_cap_set(DMA_PRIVATE, device->cap_mask);
+		device->privatecnt++;
+		err = dma_chan_get(chan);
+
+		if (err) {
+			if (err == -ENODEV) {
+				pr_debug("%s: %s module removed\n", __func__,
+					 dma_chan_name(chan));
+				list_del_rcu(&device->global_node);
+			} else
+				pr_debug("%s: failed to get %s: (%d)\n",
+					 __func__, dma_chan_name(chan), err);
+
+			if (--device->privatecnt == 0)
+				dma_cap_clear(DMA_PRIVATE, device->cap_mask);
+
+			chan = ERR_PTR(err);
+		}
+	}
+
+	return chan ? chan : ERR_PTR(-EPROBE_DEFER);
+}
+
 /**
  * dma_get_slave_channel - try to get specific channel exclusively
  * @chan: target channel
@@ -587,7 +623,6 @@  struct dma_chan *dma_get_any_slave_channel(struct dma_device *device)
 {
 	dma_cap_mask_t mask;
 	struct dma_chan *chan;
-	int err;
 
 	dma_cap_zero(mask);
 	dma_cap_set(DMA_SLAVE, mask);
@@ -595,23 +630,11 @@  struct dma_chan *dma_get_any_slave_channel(struct dma_device *device)
 	/* lock against __dma_request_channel */
 	mutex_lock(&dma_list_mutex);
 
-	chan = private_candidate(&mask, device, NULL, NULL);
-	if (chan) {
-		dma_cap_set(DMA_PRIVATE, device->cap_mask);
-		device->privatecnt++;
-		err = dma_chan_get(chan);
-		if (err) {
-			pr_debug("%s: failed to get %s: (%d)\n",
-				__func__, dma_chan_name(chan), err);
-			chan = NULL;
-			if (--device->privatecnt == 0)
-				dma_cap_clear(DMA_PRIVATE, device->cap_mask);
-		}
-	}
+	chan = dma_get_channel(device, &mask, NULL, NULL);
 
 	mutex_unlock(&dma_list_mutex);
 
-	return chan;
+	return IS_ERR(chan) ? NULL : chan;
 }
 EXPORT_SYMBOL_GPL(dma_get_any_slave_channel);
 
@@ -628,35 +651,15 @@  struct dma_chan *__dma_request_channel(const dma_cap_mask_t *mask,
 {
 	struct dma_device *device, *_d;
 	struct dma_chan *chan = NULL;
-	int err;
 
 	/* Find a channel */
 	mutex_lock(&dma_list_mutex);
 	list_for_each_entry_safe(device, _d, &dma_device_list, global_node) {
-		chan = private_candidate(mask, device, fn, fn_param);
-		if (chan) {
-			/* Found a suitable channel, try to grab, prep, and
-			 * return it.  We first set DMA_PRIVATE to disable
-			 * balance_ref_count as this channel will not be
-			 * published in the general-purpose allocator
-			 */
-			dma_cap_set(DMA_PRIVATE, device->cap_mask);
-			device->privatecnt++;
-			err = dma_chan_get(chan);
+		chan = dma_get_channel(device, mask, fn, fn_param);
+		if (!IS_ERR(chan))
+			break;
 
-			if (err == -ENODEV) {
-				pr_debug("%s: %s module removed\n",
-					 __func__, dma_chan_name(chan));
-				list_del_rcu(&device->global_node);
-			} else if (err)
-				pr_debug("%s: failed to get %s: (%d)\n",
-					 __func__, dma_chan_name(chan), err);
-			else
-				break;
-			if (--device->privatecnt == 0)
-				dma_cap_clear(DMA_PRIVATE, device->cap_mask);
-			chan = NULL;
-		}
+		chan = NULL;
 	}
 	mutex_unlock(&dma_list_mutex);