| Message ID | 20200324141624.30597-1-lars@metafoo.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iio: dma-buffer: Cleanup buffer.h/buffer_impl.h includes | expand |
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;
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á
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
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;
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; >
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;
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(-)