* [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 related [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: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
* 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
end of thread, other threads:[~2020-03-29 10:08 UTC | newest]
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
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).