> -----Original Message-----
> From: Ardelean, Alexandru <alexandru.Ardelean@xxxxxxxxxx>
> Sent: Tuesday, July 30, 2019 11:59 PM
> To: jic23@xxxxxxxxxx; Denis CIOCCA <denis.ciocca@xxxxxx>; linux-
> iio@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH 4/4] iio:pressure: preenable/postenable/predisable
> fixup for ST press buffer
>
> On Tue, 2019-07-30 at 16:08 +0000, Denis CIOCCA wrote:
> > [External]
> >
> > Hi Alexandru,
> >
> > > -----Original Message-----
> > > From: Ardelean, Alexandru <alexandru.Ardelean@xxxxxxxxxx>
> > > Sent: Tuesday, July 30, 2019 2:02 AM
> > > To: jic23@xxxxxxxxxx; Denis CIOCCA <denis.ciocca@xxxxxx>; linux-
> > > iio@xxxxxxxxxxxxxxx
> > > Subject: Re: [PATCH 4/4] iio:pressure:
> > > preenable/postenable/predisable fixup for ST press buffer
> > >
> > > On Mon, 2019-07-29 at 17:03 -0700, Denis Ciocca wrote:
> > > > [External]
> > > >
> > > > This patch is trying to cleanup for good the buffers operation functions.
> > > > There is no need of using preenable, all can be done into postenable.
> > > > Let's also use logical sequence of operations as already done in
> > > > accel driver.
> > > > Finally also rename the goto label using operation to perform and
> > > > not where it fails.
> > > >
> > >
> > > Reviewed-by: Alexandru Ardelean <alexandru.ardelean@xxxxxxxxxx>
> > >
> > >
> > > Unrelated to this patch, I was thinking that it would be a neat idea
> > > to move the `buffer_data` allocation in
> > > `drivers/iio/common/st_sensors/st_sensors_buffer.c`
> > >
> > > This would remove some duplication of this alloc + free in drivers.
> > >
> > > Maybe in st_sensors_trigger_handler() something like:
> > >
> > > if (!sdata->buffer_data) {
> > > sdata->buffer_data = devm_kmalloc()
> > > if (!sdata->buffer_data) {
> > > dev_err(indio_dev->dev, "Failed to allocate buffer data\n");
> > > goto st_sensors_get_buffer_element_error;
> > > }
> > > }
> > >
> > > Using devm_kmalloc() or a similar devm_ variant would be neat, since
> > > it gets free'd when the device gets removed.
> >
> > Not sure about the solution proposed.
> > Memory allocation is actually related to how many channels are
> > enabled, one possibility could be to allocate the maximum but not sure it's
> worth...
> > Moreover the memory allocation there could let driver miss the first
> sample I guess.
> >
>
> I see.
> Would [then] moving the allocation in preenable work? and the free in
> postdisable?
Yeah, I think will work just fine. I've tested a preliminary version, I'll share soon.
>
> Thing is: when iio_triggered_buffer_postenable() and
> iio_triggered_buffer_predisable() get removed (and logic moved into core),
> then the order changes, and the attach pollfunc gets called before the
> allocation.
>
>
> The final destination for these changes is this commit:
> https://github.com/analogdevicesinc/linux/commit/eee97d12665fef8c#diff-
> 0a87744ce945d2c1c89ea19f21fb35bbR722
> [this an older form]
> In this commit, the attach/detach of the poll_func is moved into core, since
> that is all that the 2 functions do
> (iio_triggered_buffer_postenable() and iio_triggered_buffer_predisable())
>
> When the move happens [for the ST drivers in particular] the poll_func is
> attached, and then the allocation is done, and the free is done first, and then
> the poll_func is detached.
>
>
> >
> > > Thanks
> > > Alex
> > >
> > > > Signed-off-by: Denis Ciocca <denis.ciocca@xxxxxx>
> > > > ---
> > > > drivers/iio/pressure/st_pressure_buffer.c | 32
> > > > ++++++++---------------
> > > > 1 file changed, 11 insertions(+), 21 deletions(-)
> > > >
> > > > diff --git a/drivers/iio/pressure/st_pressure_buffer.c
> > > > b/drivers/iio/pressure/st_pressure_buffer.c
> > > > index f21b630abaa0..54823cfcfab5 100644
> > > > --- a/drivers/iio/pressure/st_pressure_buffer.c
> > > > +++ b/drivers/iio/pressure/st_pressure_buffer.c
> > > > @@ -29,53 +29,43 @@ int st_press_trig_set_state(struct iio_trigger
> > > > *trig,
> > > bool state)
> > > > return st_sensors_set_dataready_irq(indio_dev, state); }
> > > >
> > > > -static int st_press_buffer_preenable(struct iio_dev *indio_dev) -{
> > > > - return st_sensors_set_enable(indio_dev, true);
> > > > -}
> > > > -
> > > > static int st_press_buffer_postenable(struct iio_dev *indio_dev) {
> > > > - int err;
> > > > struct st_sensor_data *press_data = iio_priv(indio_dev);
> > > > + int err;
> > > >
> > > > press_data->buffer_data = kmalloc(indio_dev->scan_bytes,
> > > > GFP_DMA | GFP_KERNEL);
> > > > - if (press_data->buffer_data == NULL) {
> > > > - err = -ENOMEM;
> > > > - goto allocate_memory_error;
> > > > - }
> > > > + if (!press_data->buffer_data)
> > > > + return -ENOMEM;
> > > >
> > > > err = iio_triggered_buffer_postenable(indio_dev);
> > > > if (err < 0)
> > > > - goto st_press_buffer_postenable_error;
> > > > + goto st_press_free_buffer;
> > > >
> > > > - return err;
> > > > + return st_sensors_set_enable(indio_dev, true);
> > > >
> > > > -st_press_buffer_postenable_error:
> > > > +st_press_free_buffer:
> > > > kfree(press_data->buffer_data);
> > > > -allocate_memory_error:
> > > > return err;
> > > > }
> > > >
> > > > static int st_press_buffer_predisable(struct iio_dev *indio_dev)
> > > > {
> > > > - int err;
> > > > struct st_sensor_data *press_data = iio_priv(indio_dev);
> > > > -
> > > > - err = iio_triggered_buffer_predisable(indio_dev);
> > > > - if (err < 0)
> > > > - goto st_press_buffer_predisable_error;
> > > > + int err, err2;
> > > >
> > > > err = st_sensors_set_enable(indio_dev, false);
> > > >
> > > > -st_press_buffer_predisable_error:
> > > > + err2 = iio_triggered_buffer_predisable(indio_dev);
> > > > + if (!err)
> > > > + err = err2;
> > > > +
> > > > kfree(press_data->buffer_data);
> > > > return err;
> > > > }
> > > >
> > > > static const struct iio_buffer_setup_ops st_press_buffer_setup_ops =
> {
> > > > - .preenable = &st_press_buffer_preenable,
> > > > .postenable = &st_press_buffer_postenable,
> > > > .predisable = &st_press_buffer_predisable, };
