| Message ID | 20240328-iio-backend-axi-dac-v1-1-afc808b3fde3@analog.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: dac: support IIO backends on the output direction | expand |
On Thu, 28 Mar 2024 14:22:25 +0100 Nuno Sa via B4 Relay <devnull+nuno.sa.analog.com@kernel.org> wrote: > From: Nuno Sa <nuno.sa@analog.com> > > Simple helper for setting the buffer direction when it's allocated using > iio_dmaengine_buffer_alloc(). > > Signed-off-by: Nuno Sa <nuno.sa@analog.com> I wonder if we should align with the approach for triggered-buffers with and _ext form of the registration function that takes a direction. It seems odd to allocate one then change the direction. Jonathan > --- > drivers/iio/industrialio-buffer.c | 7 +++++++ > include/linux/iio/buffer.h | 3 +++ > 2 files changed, 10 insertions(+) > > diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c > index 1d950a3e153b..4b1ca6ad86ee 100644 > --- a/drivers/iio/industrialio-buffer.c > +++ b/drivers/iio/industrialio-buffer.c > @@ -1956,6 +1956,13 @@ void iio_buffer_put(struct iio_buffer *buffer) > } > EXPORT_SYMBOL_GPL(iio_buffer_put); > > +void iio_buffer_set_dir(struct iio_buffer *buffer, > + enum iio_buffer_direction dir) > +{ > + buffer->direction = dir; > +} > +EXPORT_SYMBOL_GPL(iio_buffer_set_dir); > + > /** > * iio_device_attach_buffer - Attach a buffer to a IIO device > * @indio_dev: The device the buffer should be attached to > diff --git a/include/linux/iio/buffer.h b/include/linux/iio/buffer.h > index 418b1307d3f2..7e70bb5adc01 100644 > --- a/include/linux/iio/buffer.h > +++ b/include/linux/iio/buffer.h > @@ -55,4 +55,7 @@ bool iio_validate_scan_mask_onehot(struct iio_dev *indio_dev, > int iio_device_attach_buffer(struct iio_dev *indio_dev, > struct iio_buffer *buffer); > > +void iio_buffer_set_dir(struct iio_buffer *buffer, > + enum iio_buffer_direction dir); > + > #endif /* _IIO_BUFFER_GENERIC_H_ */ >
On Thu, 2024-03-28 at 14:36 +0000, Jonathan Cameron wrote: > On Thu, 28 Mar 2024 14:22:25 +0100 > Nuno Sa via B4 Relay <devnull+nuno.sa.analog.com@kernel.org> wrote: > > > From: Nuno Sa <nuno.sa@analog.com> > > > > Simple helper for setting the buffer direction when it's allocated using > > iio_dmaengine_buffer_alloc(). > > > > Signed-off-by: Nuno Sa <nuno.sa@analog.com> > I wonder if we should align with the approach for triggered-buffers with and _ext > form of the registration function that takes a direction. It seems odd to allocate > one then change the direction. > I agree it feels odd but I did not wanted to include buffer_impl.h in places that should not have it :) This patchseries adds the direction to devm_iio_dmaengine_buffer_setup(). Maybe what we need is to have a non devm variant iio_dmaengine_buffer_setup() and turn iio_dmaengine_buffer_alloc() static again. Maybe that would make things a bit more consistent. In fact looking closer into that file, I would get rid of: devm_iio_dmaengine_buffer_alloc() and __devm_iio_dmaengine_buffer_free() and have: devm_iio_dmaengine_buffer_setup() and iio_dmaengine_buffer_setup() that make use of iio_dmaengine_buffer_free() and iio_dmaengine_buffer_alloc(). I think it would make more sense like the above. Thoughts? - Nuno Sá
On Thu, 28 Mar 2024 16:18:04 +0100 Nuno Sá <noname.nuno@gmail.com> wrote: > On Thu, 2024-03-28 at 14:36 +0000, Jonathan Cameron wrote: > > On Thu, 28 Mar 2024 14:22:25 +0100 > > Nuno Sa via B4 Relay <devnull+nuno.sa.analog.com@kernel.org> wrote: > > > > > From: Nuno Sa <nuno.sa@analog.com> > > > > > > Simple helper for setting the buffer direction when it's allocated using > > > iio_dmaengine_buffer_alloc(). > > > > > > Signed-off-by: Nuno Sa <nuno.sa@analog.com> > > I wonder if we should align with the approach for triggered-buffers with and _ext > > form of the registration function that takes a direction. It seems odd to allocate > > one then change the direction. > > > > I agree it feels odd but I did not wanted to include buffer_impl.h in places that > should not have it :) > > This patchseries adds the direction to devm_iio_dmaengine_buffer_setup(). Maybe what > we need is to have a non devm variant iio_dmaengine_buffer_setup() and turn > iio_dmaengine_buffer_alloc() static again. Maybe that would make things a bit more > consistent. In fact looking closer into that file, I would get rid of: > > devm_iio_dmaengine_buffer_alloc() and __devm_iio_dmaengine_buffer_free() > > and have: > > devm_iio_dmaengine_buffer_setup() and iio_dmaengine_buffer_setup() that make use of > iio_dmaengine_buffer_free() and iio_dmaengine_buffer_alloc(). > > I think it would make more sense like the above. Thoughts? Sounds reasonable to me Jonathan > > - Nuno Sá >
diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c index 1d950a3e153b..4b1ca6ad86ee 100644 --- a/drivers/iio/industrialio-buffer.c +++ b/drivers/iio/industrialio-buffer.c @@ -1956,6 +1956,13 @@ void iio_buffer_put(struct iio_buffer *buffer) } EXPORT_SYMBOL_GPL(iio_buffer_put); +void iio_buffer_set_dir(struct iio_buffer *buffer, + enum iio_buffer_direction dir) +{ + buffer->direction = dir; +} +EXPORT_SYMBOL_GPL(iio_buffer_set_dir); + /** * iio_device_attach_buffer - Attach a buffer to a IIO device * @indio_dev: The device the buffer should be attached to diff --git a/include/linux/iio/buffer.h b/include/linux/iio/buffer.h index 418b1307d3f2..7e70bb5adc01 100644 --- a/include/linux/iio/buffer.h +++ b/include/linux/iio/buffer.h @@ -55,4 +55,7 @@ bool iio_validate_scan_mask_onehot(struct iio_dev *indio_dev, int iio_device_attach_buffer(struct iio_dev *indio_dev, struct iio_buffer *buffer); +void iio_buffer_set_dir(struct iio_buffer *buffer, + enum iio_buffer_direction dir); + #endif /* _IIO_BUFFER_GENERIC_H_ */