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
>
