Ok stupid question, you were considering the align already.
Denis
> -----Original Message-----
> From: Denis CIOCCA
> Sent: Monday, August 5, 2019 10:19 AM
> To: Jonathan Cameron <jic23@xxxxxxxxxxxxxxxxxxxxx>; Ardelean, Alexandru
> <alexandru.Ardelean@xxxxxxxxxx>
> Cc: linux-iio@xxxxxxxxxxxxxxx
> Subject: RE: [PATCH 1/5] iio:common: introduce
> st_sensors_buffer_preenable/predisable functions
>
> 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
> > >
