[v3,02/41] dmaengine: at_hdmac: convert callback to helper function

Commit Message

Dave Jiang July 18, 2016, 9:47 p.m. UTC

Convert driver to use the new helper function for callback

Signed-off-by: Dave Jiang <[e-mail]>
Cc: Nicolas Ferre <[e-mail]>
---
 drivers/dma/at_hdmac.c  |   13 +++++--------
 drivers/dma/dmaengine.h |    6 ++++++
 2 files changed, 11 insertions(+), 8 deletions(-)


--
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Ludovic Desroches July 19, 2016, 8:41 a.m. UTC | #1

On Mon, Jul 18, 2016 at 02:47:56PM -0700, Dave Jiang wrote:
> Convert driver to use the new helper function for callback
> 
> Signed-off-by: Dave Jiang <[e-mail]>

Tested-by: Ludovic Desroches <[e-mail]>

> Cc: Nicolas Ferre <[e-mail]>
> ---
>  drivers/dma/at_hdmac.c  |   13 +++++--------
>  drivers/dma/dmaengine.h |    6 ++++++
>  2 files changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/dma/at_hdmac.c b/drivers/dma/at_hdmac.c
> index 53d22eb..11203fb 100644
> --- a/drivers/dma/at_hdmac.c
> +++ b/drivers/dma/at_hdmac.c
> @@ -473,15 +473,13 @@ atc_chain_complete(struct at_dma_chan *atchan, struct at_desc *desc)
>  	/* for cyclic transfers,
>  	 * no need to replay callback function while stopping */
>  	if (!atc_chan_is_cyclic(atchan)) {
> -		dma_async_tx_callback	callback = txd->callback;
> -		void			*param = txd->callback_param;
> +		struct dmaengine_desc_callback	cb;
>  
>  		/*
>  		 * The API requires that no submissions are done from a
>  		 * callback, so we don't need to drop the lock here
>  		 */
> -		if (callback)
> -			callback(param);
> +		dmaengine_desc_get_callback_invoke(txd, &cb, NULL);
>  	}
>  
>  	dma_run_dependencies(txd);
> @@ -598,15 +596,14 @@ static void atc_handle_cyclic(struct at_dma_chan *atchan)
>  {
>  	struct at_desc			*first = atc_first_active(atchan);
>  	struct dma_async_tx_descriptor	*txd = &first->txd;
> -	dma_async_tx_callback		callback = txd->callback;
> -	void				*param = txd->callback_param;
> +	struct dmaengine_desc_callback	cb;
>  
>  	dev_vdbg(chan2dev(&atchan->chan_common),
>  			"new cyclic period llp 0x%08x\n",
>  			channel_readl(atchan, DSCR));
>  
> -	if (callback)
> -		callback(param);
> +	dmaengine_desc_get_callback(txd, &cb);
> +	dmaengine_desc_callback_invoke(&cb, NULL);
>  }
>  
>  /*--  IRQ & Tasklet  ---------------------------------------------------*/
> diff --git a/drivers/dma/dmaengine.h b/drivers/dma/dmaengine.h
> index 6fb5edc..e64d27a 100644
> --- a/drivers/dma/dmaengine.h
> +++ b/drivers/dma/dmaengine.h
> @@ -116,4 +116,10 @@ dmaengine_desc_get_callback_invoke(struct dma_async_tx_descriptor *tx,
>  	dmaengine_desc_callback_invoke(cb, result);
>  }
>  
> +static inline bool
> +dmaengine_desc_callback_valid(struct dmaengine_desc_callback *cb)
> +{
> +	return (cb->callback) ? true : false;
> +}
> +
>  #endif
> 
> --
> To unsubscribe from this list: send the line "unsubscribe dmaengine" in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Lars-Peter Clausen July 19, 2016, 6:15 p.m. UTC | #2

On 07/18/2016 11:47 PM, Dave Jiang wrote:
> Convert driver to use the new helper function for callback

I'd still like to see a proper commit message for the transformation
patches. You know what this is about, I know what this is about, somebody
reading the log in 2-3 years will have no idea.

>  /*--  IRQ & Tasklet  ---------------------------------------------------*/
> diff --git a/drivers/dma/dmaengine.h b/drivers/dma/dmaengine.h
> index 6fb5edc..e64d27a 100644
> --- a/drivers/dma/dmaengine.h
> +++ b/drivers/dma/dmaengine.h
> @@ -116,4 +116,10 @@ dmaengine_desc_get_callback_invoke(struct dma_async_tx_descriptor *tx,
>  	dmaengine_desc_callback_invoke(cb, result);
>  }
>  
> +static inline bool
> +dmaengine_desc_callback_valid(struct dmaengine_desc_callback *cb)
> +{
> +	return (cb->callback) ? true : false;
> +}

This should be part of patch 1.

--
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Dave Jiang July 19, 2016, 6:19 p.m. UTC | #3

> -----Original Message-----

> From: Lars-Peter Clausen [mailto:[e-mail]]

> Sent: Tuesday, July 19, 2016 11:16 AM

> To: Jiang, Dave <[e-mail]>; Koul, Vinod <[e-mail]>

> Cc: [e-mail]; Williams, Dan J <[e-mail]>; Nicolas Ferre <[e-mail]>;

> [e-mail]

> Subject: Re: [PATCH v3 02/41] dmaengine: at_hdmac: convert callback to helper function

> 

> On 07/18/2016 11:47 PM, Dave Jiang wrote:

> > Convert driver to use the new helper function for callback

> 

> I'd still like to see a proper commit message for the transformation

> patches. You know what this is about, I know what this is about, somebody

> reading the log in 2-3 years will have no idea.


Ok I'll fix that up.

> 

> >  /*--  IRQ & Tasklet  ---------------------------------------------------*/

> > diff --git a/drivers/dma/dmaengine.h b/drivers/dma/dmaengine.h

> > index 6fb5edc..e64d27a 100644

> > --- a/drivers/dma/dmaengine.h

> > +++ b/drivers/dma/dmaengine.h

> > @@ -116,4 +116,10 @@ dmaengine_desc_get_callback_invoke(struct dma_async_tx_descriptor *tx,

> >  	dmaengine_desc_callback_invoke(cb, result);

> >  }

> >

> > +static inline bool

> > +dmaengine_desc_callback_valid(struct dmaengine_desc_callback *cb)

> > +{

> > +	return (cb->callback) ? true : false;

> > +}

> 

> This should be part of patch 1.


Uck. stgit user error. Will fix.

Dave Jiang July 19, 2016, 6:42 p.m. UTC | #4

> -----Original Message-----

> From: [e-mail] [mailto:[e-mail]] On Behalf Of Jiang, Dave

> Sent: Tuesday, July 19, 2016 11:20 AM

> To: Lars-Peter Clausen <[e-mail]>; Koul, Vinod <[e-mail]>

> Cc: [e-mail]; Williams, Dan J <[e-mail]>; Nicolas Ferre <[e-mail]>;

> [e-mail]

> Subject: RE: [PATCH v3 02/41] dmaengine: at_hdmac: convert callback to helper function

> 

> > -----Original Message-----

> > From: Lars-Peter Clausen [mailto:[e-mail]]

> > Sent: Tuesday, July 19, 2016 11:16 AM

> > To: Jiang, Dave <[e-mail]>; Koul, Vinod <[e-mail]>

> > Cc: [e-mail]; Williams, Dan J <[e-mail]>; Nicolas Ferre <[e-mail]>;

> > [e-mail]

> > Subject: Re: [PATCH v3 02/41] dmaengine: at_hdmac: convert callback to helper function

> >

> > On 07/18/2016 11:47 PM, Dave Jiang wrote:

> > > Convert driver to use the new helper function for callback

> >

> > I'd still like to see a proper commit message for the transformation

> > patches. You know what this is about, I know what this is about, somebody

> > reading the log in 2-3 years will have no idea.

> 

> Ok I'll fix that up.


You want a more verbose commit message for patch 1/41 and 36/41 for the two transformation patches and not all the drivers patches correct? Just want to clarify. 

> 

> >

> > >  /*--  IRQ & Tasklet  ---------------------------------------------------*/

> > > diff --git a/drivers/dma/dmaengine.h b/drivers/dma/dmaengine.h

> > > index 6fb5edc..e64d27a 100644

> > > --- a/drivers/dma/dmaengine.h

> > > +++ b/drivers/dma/dmaengine.h

> > > @@ -116,4 +116,10 @@ dmaengine_desc_get_callback_invoke(struct dma_async_tx_descriptor *tx,

> > >  	dmaengine_desc_callback_invoke(cb, result);

> > >  }

> > >

> > > +static inline bool

> > > +dmaengine_desc_callback_valid(struct dmaengine_desc_callback *cb)

> > > +{

> > > +	return (cb->callback) ? true : false;

> > > +}

> >

> > This should be part of patch 1.

> 

> Uck. stgit user error. Will fix.

> �{.n�+�������+%��lzwm��b�맲��r��yٚzx"�觶

> ��ܨ}���Ơz�&j:+v���
����zZ+��+zf���h���~����i���z��w���?����&�)ߢf

Lars-Peter Clausen July 19, 2016, 7:57 p.m. UTC | #5

On 07/19/2016 08:42 PM, Jiang, Dave wrote:
>> -----Original Message-----
>> From: [e-mail] [mailto:[e-mail]] On Behalf Of Jiang, Dave
>> Sent: Tuesday, July 19, 2016 11:20 AM
>> To: Lars-Peter Clausen <[e-mail]>; Koul, Vinod <[e-mail]>
>> Cc: [e-mail]; Williams, Dan J <[e-mail]>; Nicolas Ferre <[e-mail]>;
>> [e-mail]
>> Subject: RE: [PATCH v3 02/41] dmaengine: at_hdmac: convert callback to helper function
>>
>>> -----Original Message-----
>>> From: Lars-Peter Clausen [mailto:[e-mail]]
>>> Sent: Tuesday, July 19, 2016 11:16 AM
>>> To: Jiang, Dave <[e-mail]>; Koul, Vinod <[e-mail]>
>>> Cc: [e-mail]; Williams, Dan J <[e-mail]>; Nicolas Ferre <[e-mail]>;
>>> [e-mail]
>>> Subject: Re: [PATCH v3 02/41] dmaengine: at_hdmac: convert callback to helper function
>>>
>>> On 07/18/2016 11:47 PM, Dave Jiang wrote:
>>>> Convert driver to use the new helper function for callback
>>>
>>> I'd still like to see a proper commit message for the transformation
>>> patches. You know what this is about, I know what this is about, somebody
>>> reading the log in 2-3 years will have no idea.
>>
>> Ok I'll fix that up.
> 
> You want a more verbose commit message for patch 1/41 and 36/41 for the two transformation patches and not all the drivers patches correct? Just want to clarify. 

Every patch should have a proper commit message explaining what is done, how
it is done and why it is done. Somebody looking at the commit log should be
able to get a basic understanding what is going on without having to dig
through the mailing list archives to find the original discussions.

E.g. if I read "Convert driver to use the new helper function for callback"
I'd immediately ask "why? What is the advantage of using the helper functions?"

The commit message should provide additional information to the commit. This
is not the case here, it is fairly obvious looking at the change that it
converts the driver to using the helper functions. In addition to that the
same information is also in the commit title, so all in all the commit
message itself is pretty redundant in its current form. What is not obvious
from the change though is why it is done, this is what the commit message
should explain.

- Lars

--
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/dma/at_hdmac.c b/drivers/dma/at_hdmac.c
index 53d22eb..11203fb 100644
--- a/drivers/dma/at_hdmac.c
+++ b/drivers/dma/at_hdmac.c
@@ -473,15 +473,13 @@  atc_chain_complete(struct at_dma_chan *atchan, struct at_desc *desc)
 	/* for cyclic transfers,
 	 * no need to replay callback function while stopping */
 	if (!atc_chan_is_cyclic(atchan)) {
-		dma_async_tx_callback	callback = txd->callback;
-		void			*param = txd->callback_param;
+		struct dmaengine_desc_callback	cb;
 
 		/*
 		 * The API requires that no submissions are done from a
 		 * callback, so we don't need to drop the lock here
 		 */
-		if (callback)
-			callback(param);
+		dmaengine_desc_get_callback_invoke(txd, &cb, NULL);
 	}
 
 	dma_run_dependencies(txd);
@@ -598,15 +596,14 @@  static void atc_handle_cyclic(struct at_dma_chan *atchan)
 {
 	struct at_desc			*first = atc_first_active(atchan);
 	struct dma_async_tx_descriptor	*txd = &first->txd;
-	dma_async_tx_callback		callback = txd->callback;
-	void				*param = txd->callback_param;
+	struct dmaengine_desc_callback	cb;
 
 	dev_vdbg(chan2dev(&atchan->chan_common),
 			"new cyclic period llp 0x%08x\n",
 			channel_readl(atchan, DSCR));
 
-	if (callback)
-		callback(param);
+	dmaengine_desc_get_callback(txd, &cb);
+	dmaengine_desc_callback_invoke(&cb, NULL);
 }
 
 /*--  IRQ & Tasklet  ---------------------------------------------------*/
diff --git a/drivers/dma/dmaengine.h b/drivers/dma/dmaengine.h
index 6fb5edc..e64d27a 100644
--- a/drivers/dma/dmaengine.h
+++ b/drivers/dma/dmaengine.h
@@ -116,4 +116,10 @@  dmaengine_desc_get_callback_invoke(struct dma_async_tx_descriptor *tx,
 	dmaengine_desc_callback_invoke(cb, result);
 }
 
+static inline bool
+dmaengine_desc_callback_valid(struct dmaengine_desc_callback *cb)
+{
+	return (cb->callback) ? true : false;
+}
+
 #endif