| Message ID | 20250406172001.2167607-7-jic23@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | IIO: Introduce iio_push_to_buffers_with_ts() taking an input buffer length argument. | expand |
On Sun, Apr 06, 2025 at 06:19:47PM +0100, Jonathan Cameron wrote: > From: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > It has long been discouraged for drivers to make use of iio_dev->scan_bytes > directly as that is an implementation detail of the core. As such our > example driver should definitely not be doing so. > > A simple anonymous structure definition suffices here as even though > we have a mixture of signed and unsigned channels only the signed ones > use the full storage so the unsigned channels can used signed types as > well. ... > --- > v2: Add a comment about stack buffers not being DMA safe. > - data = kzalloc(indio_dev->scan_bytes, GFP_KERNEL); > - if (!data) > - goto done; > + /* > + * Note that some buses such as SPI require DMA safe buffers which > + * cannot be on the stack. > + */ > + struct { > + s16 data[ARRAY_SIZE(fakedata)]; > + aligned_s64 timestamp; > + } scan; But why not continue using kmemdup() / kzalloc() for this and put ithe comment there, which would make more sense? Then the device driver developer will choose either to use the on-stack or heap one? Or didn't I get the idea behind all this?
On Mon, 2025-04-07 at 12:23 +0300, Andy Shevchenko wrote: > On Sun, Apr 06, 2025 at 06:19:47PM +0100, Jonathan Cameron wrote: > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > > > It has long been discouraged for drivers to make use of iio_dev->scan_bytes > > directly as that is an implementation detail of the core. As such our > > example driver should definitely not be doing so. > > > > A simple anonymous structure definition suffices here as even though > > we have a mixture of signed and unsigned channels only the signed ones > > use the full storage so the unsigned channels can used signed types as > > well. > > ... > > > --- > > v2: Add a comment about stack buffers not being DMA safe. > > > - data = kzalloc(indio_dev->scan_bytes, GFP_KERNEL); > > - if (!data) > > - goto done; > > + /* > > + * Note that some buses such as SPI require DMA safe buffers which > > + * cannot be on the stack. > > + */ > > + struct { > > + s16 data[ARRAY_SIZE(fakedata)]; > > + aligned_s64 timestamp; > > + } scan; > > But why not continue using kmemdup() / kzalloc() for this and put > ithe comment there, which would make more sense? Then the device > driver developer will choose either to use the on-stack or heap one? > > Or didn't I get the idea behind all this? I think Jonathan means that if you need to do any SPI/I2c transfer as part of your trigger handler, then do not use stack allocated variables. Since in this example we have anything like that, things were simplified to use stack variables with a comment on top of it. - Nuno Sá
On Sun, 2025-04-06 at 18:19 +0100, Jonathan Cameron wrote: > From: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > It has long been discouraged for drivers to make use of iio_dev->scan_bytes > directly as that is an implementation detail of the core. As such our > example driver should definitely not be doing so. > > A simple anonymous structure definition suffices here as even though > we have a mixture of signed and unsigned channels only the signed ones > use the full storage so the unsigned channels can used signed types as > well. > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > --- > v2: Add a comment about stack buffers not being DMA safe. > --- Reviewed-by: Nuno Sá <nuno.sa@analog.com> > drivers/iio/dummy/iio_simple_dummy_buffer.c | 20 ++++++++++---------- > 1 file changed, 10 insertions(+), 10 deletions(-) > > diff --git a/drivers/iio/dummy/iio_simple_dummy_buffer.c > b/drivers/iio/dummy/iio_simple_dummy_buffer.c > index 288880346707..eca5f0652d23 100644 > --- a/drivers/iio/dummy/iio_simple_dummy_buffer.c > +++ b/drivers/iio/dummy/iio_simple_dummy_buffer.c > @@ -46,11 +46,14 @@ static irqreturn_t iio_simple_dummy_trigger_h(int irq, > void *p) > struct iio_poll_func *pf = p; > struct iio_dev *indio_dev = pf->indio_dev; > int i = 0, j; > - u16 *data; > - > - data = kzalloc(indio_dev->scan_bytes, GFP_KERNEL); > - if (!data) > - goto done; > + /* > + * Note that some buses such as SPI require DMA safe buffers which > + * cannot be on the stack. > + */ > + struct { > + s16 data[ARRAY_SIZE(fakedata)]; > + aligned_s64 timestamp; > + } scan; > > /* > * Three common options here: > @@ -69,14 +72,11 @@ static irqreturn_t iio_simple_dummy_trigger_h(int irq, > void *p) > * constant table fakedata. > */ > iio_for_each_active_channel(indio_dev, j) > - data[i++] = fakedata[j]; > + scan.data[i++] = fakedata[j]; > > - iio_push_to_buffers_with_timestamp(indio_dev, data, > + iio_push_to_buffers_with_timestamp(indio_dev, &scan, > iio_get_time_ns(indio_dev)); > > - kfree(data); > - > -done: > /* > * Tell the core we are done with this trigger and ready for the > * next one.
diff --git a/drivers/iio/dummy/iio_simple_dummy_buffer.c b/drivers/iio/dummy/iio_simple_dummy_buffer.c index 288880346707..eca5f0652d23 100644 --- a/drivers/iio/dummy/iio_simple_dummy_buffer.c +++ b/drivers/iio/dummy/iio_simple_dummy_buffer.c @@ -46,11 +46,14 @@ static irqreturn_t iio_simple_dummy_trigger_h(int irq, void *p) struct iio_poll_func *pf = p; struct iio_dev *indio_dev = pf->indio_dev; int i = 0, j; - u16 *data; - - data = kzalloc(indio_dev->scan_bytes, GFP_KERNEL); - if (!data) - goto done; + /* + * Note that some buses such as SPI require DMA safe buffers which + * cannot be on the stack. + */ + struct { + s16 data[ARRAY_SIZE(fakedata)]; + aligned_s64 timestamp; + } scan; /* * Three common options here: @@ -69,14 +72,11 @@ static irqreturn_t iio_simple_dummy_trigger_h(int irq, void *p) * constant table fakedata. */ iio_for_each_active_channel(indio_dev, j) - data[i++] = fakedata[j]; + scan.data[i++] = fakedata[j]; - iio_push_to_buffers_with_timestamp(indio_dev, data, + iio_push_to_buffers_with_timestamp(indio_dev, &scan, iio_get_time_ns(indio_dev)); - kfree(data); - -done: /* * Tell the core we are done with this trigger and ready for the * next one.