Linux-IIO Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] iio: dma-buffer: Cleanup buffer.h/buffer_impl.h includes
@ 2020-03-24 14:16 Lars-Peter Clausen
  2020-03-24 14:36 ` Ardelean, Alexandru
  2020-03-25  8:26 ` Sa, Nuno
  0 siblings, 2 replies; 6+ messages in thread
From: Lars-Peter Clausen @ 2020-03-24 14:16 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: Hartmut Knaack, Peter Meerwald-Stadler, Alexandru Ardelean,
	linux-iio, Lars-Peter Clausen

The IIO DMA buffer is a DMA buffer implementation. As such it should
include buffer_impl.h rather than buffer.h.

The include to buffer.h in buffer-dma.h should be buffer_impl.h so it has
access to the struct iio_buffer definition. The code currently only works
because all places that use buffer-dma.h include buffer_impl.h before it.

The include to buffer.h in industrialio-buffer-dma.c and
industrialio-buffer-dmaengine.c can be removed since those files don't
reference any of buffer consumer functions.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
 drivers/iio/buffer/industrialio-buffer-dma.c       | 1 -
 drivers/iio/buffer/industrialio-buffer-dmaengine.c | 1 -
 include/linux/iio/buffer-dma.h                     | 2 +-
 3 files changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/iio/buffer/industrialio-buffer-dma.c b/drivers/iio/buffer/industrialio-buffer-dma.c
index a74bd9c0587c..d348af8b9705 100644
--- a/drivers/iio/buffer/industrialio-buffer-dma.c
+++ b/drivers/iio/buffer/industrialio-buffer-dma.c
@@ -12,7 +12,6 @@
 #include <linux/mutex.h>
 #include <linux/sched.h>
 #include <linux/poll.h>
-#include <linux/iio/buffer.h>
 #include <linux/iio/buffer_impl.h>
 #include <linux/iio/buffer-dma.h>
 #include <linux/dma-mapping.h>
diff --git a/drivers/iio/buffer/industrialio-buffer-dmaengine.c b/drivers/iio/buffer/industrialio-buffer-dmaengine.c
index b129693af0fd..8b60dff527c8 100644
--- a/drivers/iio/buffer/industrialio-buffer-dmaengine.c
+++ b/drivers/iio/buffer/industrialio-buffer-dmaengine.c
@@ -14,7 +14,6 @@
 
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
-#include <linux/iio/buffer.h>
 #include <linux/iio/buffer_impl.h>
 #include <linux/iio/buffer-dma.h>
 #include <linux/iio/buffer-dmaengine.h>
diff --git a/include/linux/iio/buffer-dma.h b/include/linux/iio/buffer-dma.h
index 016d8a068353..ff15c61bf319 100644
--- a/include/linux/iio/buffer-dma.h
+++ b/include/linux/iio/buffer-dma.h
@@ -11,7 +11,7 @@
 #include <linux/kref.h>
 #include <linux/spinlock.h>
 #include <linux/mutex.h>
-#include <linux/iio/buffer.h>
+#include <linux/iio/buffer_impl.h>
 
 struct iio_dma_buffer_queue;
 struct iio_dma_buffer_ops;
-- 
2.20.1


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] iio: dma-buffer: Cleanup buffer.h/buffer_impl.h includes
  2020-03-24 14:16 [PATCH] iio: dma-buffer: Cleanup buffer.h/buffer_impl.h includes Lars-Peter Clausen
@ 2020-03-24 14:36 ` Ardelean, Alexandru
  2020-03-28 16:43   ` Jonathan Cameron
  2020-03-25  8:26 ` Sa, Nuno
  1 sibling, 1 reply; 6+ messages in thread
From: Ardelean, Alexandru @ 2020-03-24 14:36 UTC (permalink / raw)
  To: jic23, lars; +Cc: knaack.h, pmeerw, linux-iio

On Tue, 2020-03-24 at 15:16 +0100, Lars-Peter Clausen wrote:
> [External]
> 
> The IIO DMA buffer is a DMA buffer implementation. As such it should
> include buffer_impl.h rather than buffer.h.
> 
> The include to buffer.h in buffer-dma.h should be buffer_impl.h so it has
> access to the struct iio_buffer definition. The code currently only works
> because all places that use buffer-dma.h include buffer_impl.h before it.
> 
> The include to buffer.h in industrialio-buffer-dma.c and
> industrialio-buffer-dmaengine.c can be removed since those files don't
> reference any of buffer consumer functions.

Reviewed-by: Alexandru Ardelean <alexandru.ardelean@analog.com>

> 
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> ---
>  drivers/iio/buffer/industrialio-buffer-dma.c       | 1 -
>  drivers/iio/buffer/industrialio-buffer-dmaengine.c | 1 -
>  include/linux/iio/buffer-dma.h                     | 2 +-
>  3 files changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/buffer/industrialio-buffer-dma.c
> b/drivers/iio/buffer/industrialio-buffer-dma.c
> index a74bd9c0587c..d348af8b9705 100644
> --- a/drivers/iio/buffer/industrialio-buffer-dma.c
> +++ b/drivers/iio/buffer/industrialio-buffer-dma.c
> @@ -12,7 +12,6 @@
>  #include <linux/mutex.h>
>  #include <linux/sched.h>
>  #include <linux/poll.h>
> -#include <linux/iio/buffer.h>
>  #include <linux/iio/buffer_impl.h>
>  #include <linux/iio/buffer-dma.h>
>  #include <linux/dma-mapping.h>
> diff --git a/drivers/iio/buffer/industrialio-buffer-dmaengine.c
> b/drivers/iio/buffer/industrialio-buffer-dmaengine.c
> index b129693af0fd..8b60dff527c8 100644
> --- a/drivers/iio/buffer/industrialio-buffer-dmaengine.c
> +++ b/drivers/iio/buffer/industrialio-buffer-dmaengine.c
> @@ -14,7 +14,6 @@
>  
>  #include <linux/iio/iio.h>
>  #include <linux/iio/sysfs.h>
> -#include <linux/iio/buffer.h>
>  #include <linux/iio/buffer_impl.h>
>  #include <linux/iio/buffer-dma.h>
>  #include <linux/iio/buffer-dmaengine.h>
> diff --git a/include/linux/iio/buffer-dma.h b/include/linux/iio/buffer-dma.h
> index 016d8a068353..ff15c61bf319 100644
> --- a/include/linux/iio/buffer-dma.h
> +++ b/include/linux/iio/buffer-dma.h
> @@ -11,7 +11,7 @@
>  #include <linux/kref.h>
>  #include <linux/spinlock.h>
>  #include <linux/mutex.h>
> -#include <linux/iio/buffer.h>
> +#include <linux/iio/buffer_impl.h>
>  
>  struct iio_dma_buffer_queue;
>  struct iio_dma_buffer_ops;

^ permalink raw reply	[flat|nested] 6+ messages in thread

* RE: [PATCH] iio: dma-buffer: Cleanup buffer.h/buffer_impl.h includes
  2020-03-24 14:16 [PATCH] iio: dma-buffer: Cleanup buffer.h/buffer_impl.h includes Lars-Peter Clausen
  2020-03-24 14:36 ` Ardelean, Alexandru
@ 2020-03-25  8:26 ` Sa, Nuno
  2020-03-25  9:01   ` Lars-Peter Clausen
  1 sibling, 1 reply; 6+ messages in thread
From: Sa, Nuno @ 2020-03-25  8:26 UTC (permalink / raw)
  To: Lars-Peter Clausen, Jonathan Cameron
  Cc: Hartmut Knaack, Peter Meerwald-Stadler, Ardelean, Alexandru, linux-iio

Hi Lars,
> -----Original Message-----
> From: linux-iio-owner@vger.kernel.org <linux-iio-owner@vger.kernel.org> On
> Behalf Of Lars-Peter Clausen
> Sent: Dienstag, 24. März 2020 15:16
> To: Jonathan Cameron <jic23@kernel.org>
> Cc: Hartmut Knaack <knaack.h@gmx.de>; Peter Meerwald-Stadler
> <pmeerw@pmeerw.net>; Ardelean, Alexandru
> <alexandru.Ardelean@analog.com>; linux-iio@vger.kernel.org; Lars-Peter
> Clausen <lars@metafoo.de>
> Subject: [PATCH] iio: dma-buffer: Cleanup buffer.h/buffer_impl.h includes
> 
> The IIO DMA buffer is a DMA buffer implementation. As such it should
> include buffer_impl.h rather than buffer.h.
> 
> The include to buffer.h in buffer-dma.h should be buffer_impl.h so it has
> access to the struct iio_buffer definition. The code currently only works
> because all places that use buffer-dma.h include buffer_impl.h before it.
> 
> The include to buffer.h in industrialio-buffer-dma.c and
> industrialio-buffer-dmaengine.c can be removed since those files don't
> reference any of buffer consumer functions.
> 

I also came across with this in ADI internal tree. Did you considered to also split buffer_dma.h into a public
and an impl header? Hence, users of buffer_dma do not get access to the internals of buffer.h? That was the
approach I suggested in our tree since the split of buffer and buffer_impl is exactly for users not to
know about the internals...

Or do you think that it's not worth it to go over this split in buffer_dma?

- Nuno Sá 




^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] iio: dma-buffer: Cleanup buffer.h/buffer_impl.h includes
  2020-03-25  8:26 ` Sa, Nuno
@ 2020-03-25  9:01   ` Lars-Peter Clausen
  0 siblings, 0 replies; 6+ messages in thread
From: Lars-Peter Clausen @ 2020-03-25  9:01 UTC (permalink / raw)
  To: Sa, Nuno, Jonathan Cameron
  Cc: Hartmut Knaack, Peter Meerwald-Stadler, Ardelean, Alexandru, linux-iio

On 3/25/20 9:26 AM, Sa, Nuno wrote:
> I also came across with this in ADI internal tree. Did you considered to also split buffer_dma.h into a public
> and an impl header? Hence, users of buffer_dma do not get access to the internals of buffer.h? That was the
> approach I suggested in our tree since the split of buffer and buffer_impl is exactly for users not to
> know about the internals...
>
> Or do you think that it's not

At the moment I think there are no users of buffer-dma.h that are not 
implementations of a buffer. At least in upstream.

There are a few drivers in the ADI tree, which include buffer-dma.h. But 
this is because they provide their own overloaded implementation for 
some of the callbacks. In a sense that makes them a buffer 
implementation. Most of them use the same simple implementation for the 
overloaded operations. It should be possible to factor this out and use 
it as a default. Then the include to buffer_dma.h can be removed.

- Lars


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] iio: dma-buffer: Cleanup buffer.h/buffer_impl.h includes
  2020-03-24 14:36 ` Ardelean, Alexandru
@ 2020-03-28 16:43   ` Jonathan Cameron
  2020-03-29 10:08     ` Jonathan Cameron
  0 siblings, 1 reply; 6+ messages in thread
From: Jonathan Cameron @ 2020-03-28 16:43 UTC (permalink / raw)
  To: Ardelean, Alexandru; +Cc: lars, knaack.h, pmeerw, linux-iio

On Tue, 24 Mar 2020 14:36:17 +0000
"Ardelean, Alexandru" <alexandru.Ardelean@analog.com> wrote:

> On Tue, 2020-03-24 at 15:16 +0100, Lars-Peter Clausen wrote:
> > [External]
> > 
> > The IIO DMA buffer is a DMA buffer implementation. As such it should
> > include buffer_impl.h rather than buffer.h.
> > 
> > The include to buffer.h in buffer-dma.h should be buffer_impl.h so it has
> > access to the struct iio_buffer definition. The code currently only works
> > because all places that use buffer-dma.h include buffer_impl.h before it.
> > 
> > The include to buffer.h in industrialio-buffer-dma.c and
> > industrialio-buffer-dmaengine.c can be removed since those files don't
> > reference any of buffer consumer functions.  
> 
> Reviewed-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> 
Applied to the togreg branch of iio.git and pushed out as testing.

Thanks

Jonathan

> > 
> > Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> > ---
> >  drivers/iio/buffer/industrialio-buffer-dma.c       | 1 -
> >  drivers/iio/buffer/industrialio-buffer-dmaengine.c | 1 -
> >  include/linux/iio/buffer-dma.h                     | 2 +-
> >  3 files changed, 1 insertion(+), 3 deletions(-)
> > 
> > diff --git a/drivers/iio/buffer/industrialio-buffer-dma.c
> > b/drivers/iio/buffer/industrialio-buffer-dma.c
> > index a74bd9c0587c..d348af8b9705 100644
> > --- a/drivers/iio/buffer/industrialio-buffer-dma.c
> > +++ b/drivers/iio/buffer/industrialio-buffer-dma.c
> > @@ -12,7 +12,6 @@
> >  #include <linux/mutex.h>
> >  #include <linux/sched.h>
> >  #include <linux/poll.h>
> > -#include <linux/iio/buffer.h>
> >  #include <linux/iio/buffer_impl.h>
> >  #include <linux/iio/buffer-dma.h>
> >  #include <linux/dma-mapping.h>
> > diff --git a/drivers/iio/buffer/industrialio-buffer-dmaengine.c
> > b/drivers/iio/buffer/industrialio-buffer-dmaengine.c
> > index b129693af0fd..8b60dff527c8 100644
> > --- a/drivers/iio/buffer/industrialio-buffer-dmaengine.c
> > +++ b/drivers/iio/buffer/industrialio-buffer-dmaengine.c
> > @@ -14,7 +14,6 @@
> >  
> >  #include <linux/iio/iio.h>
> >  #include <linux/iio/sysfs.h>
> > -#include <linux/iio/buffer.h>
> >  #include <linux/iio/buffer_impl.h>
> >  #include <linux/iio/buffer-dma.h>
> >  #include <linux/iio/buffer-dmaengine.h>
> > diff --git a/include/linux/iio/buffer-dma.h b/include/linux/iio/buffer-dma.h
> > index 016d8a068353..ff15c61bf319 100644
> > --- a/include/linux/iio/buffer-dma.h
> > +++ b/include/linux/iio/buffer-dma.h
> > @@ -11,7 +11,7 @@
> >  #include <linux/kref.h>
> >  #include <linux/spinlock.h>
> >  #include <linux/mutex.h>
> > -#include <linux/iio/buffer.h>
> > +#include <linux/iio/buffer_impl.h>
> >  
> >  struct iio_dma_buffer_queue;
> >  struct iio_dma_buffer_ops;  


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] iio: dma-buffer: Cleanup buffer.h/buffer_impl.h includes
  2020-03-28 16:43   ` Jonathan Cameron
@ 2020-03-29 10:08     ` Jonathan Cameron
  0 siblings, 0 replies; 6+ messages in thread
From: Jonathan Cameron @ 2020-03-29 10:08 UTC (permalink / raw)
  To: Ardelean, Alexandru; +Cc: lars, knaack.h, pmeerw, linux-iio

On Sat, 28 Mar 2020 16:43:35 +0000
Jonathan Cameron <jic23@kernel.org> wrote:

> On Tue, 24 Mar 2020 14:36:17 +0000
> "Ardelean, Alexandru" <alexandru.Ardelean@analog.com> wrote:
> 
> > On Tue, 2020-03-24 at 15:16 +0100, Lars-Peter Clausen wrote:  
> > > [External]
> > > 
> > > The IIO DMA buffer is a DMA buffer implementation. As such it should
> > > include buffer_impl.h rather than buffer.h.
> > > 
> > > The include to buffer.h in buffer-dma.h should be buffer_impl.h so it has
> > > access to the struct iio_buffer definition. The code currently only works
> > > because all places that use buffer-dma.h include buffer_impl.h before it.
> > > 
> > > The include to buffer.h in industrialio-buffer-dma.c and
> > > industrialio-buffer-dmaengine.c can be removed since those files don't
> > > reference any of buffer consumer functions.    
> > 
> > Reviewed-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> >   
> Applied to the togreg branch of iio.git and pushed out as testing.
And reverted again.

The usecase that was introduced in another path meant this finally
got built in my tests and this patch broke it :(

Seems we use iio_buffer_set_attrs in the dmaengine buffer
and that's in buffer.h.

I haven't looked to see how to fix this (e.g. can we just
move that definition to buffer_impl).

Jonathan

> 
> Thanks
> 
> Jonathan
> 
> > > 
> > > Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> > > ---
> > >  drivers/iio/buffer/industrialio-buffer-dma.c       | 1 -
> > >  drivers/iio/buffer/industrialio-buffer-dmaengine.c | 1 -
> > >  include/linux/iio/buffer-dma.h                     | 2 +-
> > >  3 files changed, 1 insertion(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/iio/buffer/industrialio-buffer-dma.c
> > > b/drivers/iio/buffer/industrialio-buffer-dma.c
> > > index a74bd9c0587c..d348af8b9705 100644
> > > --- a/drivers/iio/buffer/industrialio-buffer-dma.c
> > > +++ b/drivers/iio/buffer/industrialio-buffer-dma.c
> > > @@ -12,7 +12,6 @@
> > >  #include <linux/mutex.h>
> > >  #include <linux/sched.h>
> > >  #include <linux/poll.h>
> > > -#include <linux/iio/buffer.h>
> > >  #include <linux/iio/buffer_impl.h>
> > >  #include <linux/iio/buffer-dma.h>
> > >  #include <linux/dma-mapping.h>
> > > diff --git a/drivers/iio/buffer/industrialio-buffer-dmaengine.c
> > > b/drivers/iio/buffer/industrialio-buffer-dmaengine.c
> > > index b129693af0fd..8b60dff527c8 100644
> > > --- a/drivers/iio/buffer/industrialio-buffer-dmaengine.c
> > > +++ b/drivers/iio/buffer/industrialio-buffer-dmaengine.c
> > > @@ -14,7 +14,6 @@
> > >  
> > >  #include <linux/iio/iio.h>
> > >  #include <linux/iio/sysfs.h>
> > > -#include <linux/iio/buffer.h>
> > >  #include <linux/iio/buffer_impl.h>
> > >  #include <linux/iio/buffer-dma.h>
> > >  #include <linux/iio/buffer-dmaengine.h>
> > > diff --git a/include/linux/iio/buffer-dma.h b/include/linux/iio/buffer-dma.h
> > > index 016d8a068353..ff15c61bf319 100644
> > > --- a/include/linux/iio/buffer-dma.h
> > > +++ b/include/linux/iio/buffer-dma.h
> > > @@ -11,7 +11,7 @@
> > >  #include <linux/kref.h>
> > >  #include <linux/spinlock.h>
> > >  #include <linux/mutex.h>
> > > -#include <linux/iio/buffer.h>
> > > +#include <linux/iio/buffer_impl.h>
> > >  
> > >  struct iio_dma_buffer_queue;
> > >  struct iio_dma_buffer_ops;    
> 


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, back to index

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-24 14:16 [PATCH] iio: dma-buffer: Cleanup buffer.h/buffer_impl.h includes Lars-Peter Clausen
2020-03-24 14:36 ` Ardelean, Alexandru
2020-03-28 16:43   ` Jonathan Cameron
2020-03-29 10:08     ` Jonathan Cameron
2020-03-25  8:26 ` Sa, Nuno
2020-03-25  9:01   ` Lars-Peter Clausen

Linux-IIO Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-iio/0 linux-iio/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-iio linux-iio/ https://lore.kernel.org/linux-iio \
		linux-iio@vger.kernel.org
	public-inbox-index linux-iio

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-iio


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git