Hi Jonathan,
Your solution seems to be more reasonable.
Not clear about the number of bytes anyway, I should check better but my worst case:
6 bytes for data (3 axis, 2 byte each) + 8 bytes timestamp = 14 bytes
Am I missing something?
Denis
> -----Original Message-----
> From: Jonathan Cameron <jic23@xxxxxxxxxxxxxxxxxxxxx>
> Sent: Monday, August 5, 2019 8:36 AM
> To: Ardelean, Alexandru <alexandru.Ardelean@xxxxxxxxxx>
> Cc: Denis CIOCCA <denis.ciocca@xxxxxx>; linux-iio@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH 1/5] iio:common: introduce
> st_sensors_buffer_preenable/predisable functions
>
> On Mon, 5 Aug 2019 16:21:35 +0100
> Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
>
> > On Thu, 1 Aug 2019 08:24:10 +0000
> > "Ardelean, Alexandru" <alexandru.Ardelean@xxxxxxxxxx> wrote:
> >
> > > On Wed, 2019-07-31 at 14:52 -0700, Denis Ciocca wrote:
> > > > [External]
> > > >
> > > > This patch is introducing preenable/postdisable in the common
> > > > st_sensors_buffer code in order to remove the memory allocation /
> > > > de-allocation from each single st driver.
> > > >
> > >
> > > Reviewed-by: Alexandru Ardelean <alexandru.ardelean@xxxxxxxxxx>
> > >
> > > > Signed-off-by: Denis Ciocca <denis.ciocca@xxxxxx>
> >
> > As a rework, this is clearly reasonable, however, if we are going to
> > touch this code at all, there are a few things I would like to tidy up
> > about it.
> >
> > Firstly it's one of relatively few drivers that actually touch
> > scan_bytes in the first place. That is supposed to be internal state
> > to the core subsystem and not used by drivers (see INTERN marking in
> iio.h).
> > It bled across the boundary in too many places where I wasn't looking.
> >
> > Secondly these allocations are small. You would be better off just
> > making them part of the main state structure and not dynamically
> > allocated at all.
> >
> > So move buffer_data to the end of struct st_sensor_data and make it
> > whatever the maximum size needed is - I'm thinking probably 32 bytes
> > but haven't checked.
> Maths escapes me today, probably only 16 bytes as 3 channel devices mostly
> 16 bits max + timestamp.
>
> J
> >
> > You call the bulk regmap API against it so you also need to ensure
> > it's in it's own cacheline. Use the __cacheline_aligned magic to
> > enforce that. The iio_priv region is always aligned appropriately to
> > allow iio_priv accessed structures to pull this trick.
> >
> > That way we don't need to do any memory handling on demand at all.
> > We may or many not save memory as will depend on exactly how big that
> > structure already is and what mood the allocator is in.
> >
> > I don't think I'm missing a reason we can't take the approach of
> > embedding the buffer and it definitely makes the code simpler.
> >
> > Thanks,
> >
> > Jonathan
> >
> >
> > > > ---
> > > > .../iio/common/st_sensors/st_sensors_buffer.c | 22
> +++++++++++++++++++
> > > > include/linux/iio/common/st_sensors.h | 2 ++
> > > > 2 files changed, 24 insertions(+)
> > > >
> > > > diff --git a/drivers/iio/common/st_sensors/st_sensors_buffer.c
> > > > b/drivers/iio/common/st_sensors/st_sensors_buffer.c
> > > > index eee30130ae23..9da1c8104883 100644
> > > > --- a/drivers/iio/common/st_sensors/st_sensors_buffer.c
> > > > +++ b/drivers/iio/common/st_sensors/st_sensors_buffer.c
> > > > @@ -81,6 +81,28 @@ irqreturn_t st_sensors_trigger_handler(int irq,
> > > > void *p) } EXPORT_SYMBOL(st_sensors_trigger_handler);
> > > >
> > > > +int st_sensors_buffer_preenable(struct iio_dev *indio_dev) {
> > > > + struct st_sensor_data *sdata = iio_priv(indio_dev);
> > > > +
> > > > + sdata->buffer_data = kmalloc(indio_dev->scan_bytes,
> > > > + GFP_DMA | GFP_KERNEL);
> > > > + if (!sdata->buffer_data)
> > > > + return -ENOMEM;
> > > > +
> > > > + return 0;
> > > > +}
> > > > +EXPORT_SYMBOL(st_sensors_buffer_preenable);
> > > > +
> > > > +int st_sensors_buffer_postdisable(struct iio_dev *indio_dev) {
> > > > + struct st_sensor_data *sdata = iio_priv(indio_dev);
> > > > +
> > > > + kfree(sdata->buffer_data);
> > > > + return 0;
> > > > +}
> > > > +EXPORT_SYMBOL(st_sensors_buffer_postdisable);
> > > > +
> > > > MODULE_AUTHOR("Denis Ciocca <denis.ciocca@xxxxxx>");
> > > > MODULE_DESCRIPTION("STMicroelectronics ST-sensors buffer");
> > > > MODULE_LICENSE("GPL v2"); diff --git
> > > > a/include/linux/iio/common/st_sensors.h
> > > > b/include/linux/iio/common/st_sensors.h
> > > > index 28fc1f9fa7d5..c66ebb236a15 100644
> > > > --- a/include/linux/iio/common/st_sensors.h
> > > > +++ b/include/linux/iio/common/st_sensors.h
> > > > @@ -254,6 +254,8 @@ struct st_sensor_data {
> > > >
> > > > #ifdef CONFIG_IIO_BUFFER
> > > > irqreturn_t st_sensors_trigger_handler(int irq, void *p);
> > > > +int st_sensors_buffer_preenable(struct iio_dev *indio_dev); int
> > > > +st_sensors_buffer_postdisable(struct iio_dev *indio_dev);
> > > > #endif
> > > >
> > > > #ifdef CONFIG_IIO_TRIGGER
> >
