[PATCH] iio: dma-buffer: Cleanup buffer.h/buffer_impl.h includes

[PATCH] iio: dma-buffer: Cleanup buffer.h/buffer_impl.h includes

[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

Re: [PATCH] iio: dma-buffer: Cleanup buffer.h/buffer_impl.h includes

[Ardelean, Alexandru]

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;

RE: [PATCH] iio: dma-buffer: Cleanup buffer.h/buffer_impl.h includes

[Sa, Nuno]

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á 

Re: [PATCH] iio: dma-buffer: Cleanup buffer.h/buffer_impl.h includes

[Lars-Peter Clausen]

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

Re: [PATCH] iio: dma-buffer: Cleanup buffer.h/buffer_impl.h includes

[Jonathan Cameron]

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;  

Re: [PATCH] iio: dma-buffer: Cleanup buffer.h/buffer_impl.h includes

[Jonathan Cameron]

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