[PATCH 0/5] move buffer allocation into st_sensors_buffer

[PATCH 0/5] move buffer allocation into st_sensors_buffer

[Denis Ciocca]

As proposed by Alexandru, memory allocation and de-allocation can be
performed in a more neat way in st_sensors_buffer.

Denis Ciocca (5):
  iio:common: introduce st_sensors_buffer_preenable/predisable functions
  iio:accel: do not allocate/de-allocate buffer here but link setup_ops
    to st_sensors_buffer
  iio:gyro: do not allocate/de-allocate buffer here but link setup_ops
    to st_sensors_buffer
  iio:magn: do not allocate/de-allocate buffer here but link setup_ops
    to st_sensors_buffer
  iio:pressure: do not allocate/de-allocate buffer here but link
    setup_ops to st_sensors_buffer

 drivers/iio/accel/st_accel_buffer.c           | 14 +++---------
 .../iio/common/st_sensors/st_sensors_buffer.c | 22 +++++++++++++++++++
 drivers/iio/gyro/st_gyro_buffer.c             | 14 +++---------
 drivers/iio/magnetometer/st_magn_buffer.c     | 14 +++---------
 drivers/iio/pressure/st_pressure_buffer.c     | 14 +++---------
 include/linux/iio/common/st_sensors.h         |  2 ++
 6 files changed, 36 insertions(+), 44 deletions(-)

-- 
2.22.0

[PATCH 1/5] iio:common: introduce st_sensors_buffer_preenable/predisable functions

[Denis Ciocca]

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.

Signed-off-by: Denis Ciocca <denis.ciocca@st.com>
---
 .../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@st.com>");
 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
-- 
2.22.0

[PATCH 2/5] iio:accel: do not allocate/de-allocate buffer here but link setup_ops to st_sensors_buffer

[Denis Ciocca]

Setup buffer_setup_ops pre/post enable/disable to use
st_sensor_buffers functions for memory allocation/de-allocation.

Signed-off-by: Denis Ciocca <denis.ciocca@st.com>
---
 drivers/iio/accel/st_accel_buffer.c | 14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)

diff --git a/drivers/iio/accel/st_accel_buffer.c b/drivers/iio/accel/st_accel_buffer.c
index 59dcef02ec19..fc1ba52152ab 100644
--- a/drivers/iio/accel/st_accel_buffer.c
+++ b/drivers/iio/accel/st_accel_buffer.c
@@ -31,17 +31,11 @@ int st_accel_trig_set_state(struct iio_trigger *trig, bool state)
 
 static int st_accel_buffer_postenable(struct iio_dev *indio_dev)
 {
-	struct st_sensor_data *adata = iio_priv(indio_dev);
 	int err;
 
-	adata->buffer_data = kmalloc(indio_dev->scan_bytes,
-				     GFP_DMA | GFP_KERNEL);
-	if (!adata->buffer_data)
-		return -ENOMEM;
-
 	err = iio_triggered_buffer_postenable(indio_dev);
 	if (err < 0)
-		goto st_accel_free_buffer;
+		return err;
 
 	err = st_sensors_set_axis_enable(indio_dev,
 					 (u8)indio_dev->active_scan_mask[0]);
@@ -58,14 +52,11 @@ static int st_accel_buffer_postenable(struct iio_dev *indio_dev)
 	st_sensors_set_axis_enable(indio_dev, ST_SENSORS_ENABLE_ALL_AXIS);
 st_accel_buffer_predisable:
 	iio_triggered_buffer_predisable(indio_dev);
-st_accel_free_buffer:
-	kfree(adata->buffer_data);
 	return err;
 }
 
 static int st_accel_buffer_predisable(struct iio_dev *indio_dev)
 {
-	struct st_sensor_data *adata = iio_priv(indio_dev);
 	int err, err2;
 
 	err = st_sensors_set_enable(indio_dev, false);
@@ -79,13 +70,14 @@ static int st_accel_buffer_predisable(struct iio_dev *indio_dev)
 	if (!err)
 		err = err2;
 
-	kfree(adata->buffer_data);
 	return err;
 }
 
 static const struct iio_buffer_setup_ops st_accel_buffer_setup_ops = {
+	.preenable = &st_sensors_buffer_preenable,
 	.postenable = &st_accel_buffer_postenable,
 	.predisable = &st_accel_buffer_predisable,
+	.postdisable = &st_sensors_buffer_postdisable,
 };
 
 int st_accel_allocate_ring(struct iio_dev *indio_dev)
-- 
2.22.0

[PATCH 3/5] iio:gyro: do not allocate/de-allocate buffer here but link setup_ops to st_sensors_buffer

[Denis Ciocca]

Setup buffer_setup_ops pre/post enable/disable to use
st_sensor_buffers functions for memory allocation/de-allocation.

Signed-off-by: Denis Ciocca <denis.ciocca@st.com>
---
 drivers/iio/gyro/st_gyro_buffer.c | 14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)

diff --git a/drivers/iio/gyro/st_gyro_buffer.c b/drivers/iio/gyro/st_gyro_buffer.c
index c6ddfecc1fc3..48811457a943 100644
--- a/drivers/iio/gyro/st_gyro_buffer.c
+++ b/drivers/iio/gyro/st_gyro_buffer.c
@@ -31,17 +31,11 @@ int st_gyro_trig_set_state(struct iio_trigger *trig, bool state)
 
 static int st_gyro_buffer_postenable(struct iio_dev *indio_dev)
 {
-	struct st_sensor_data *gdata = iio_priv(indio_dev);
 	int err;
 
-	gdata->buffer_data = kmalloc(indio_dev->scan_bytes,
-				     GFP_DMA | GFP_KERNEL);
-	if (!gdata->buffer_data)
-		return -ENOMEM;
-
 	err = iio_triggered_buffer_postenable(indio_dev);
 	if (err < 0)
-		goto st_gyro_free_buffer;
+		return err;
 
 	err = st_sensors_set_axis_enable(indio_dev,
 					 (u8)indio_dev->active_scan_mask[0]);
@@ -58,15 +52,12 @@ static int st_gyro_buffer_postenable(struct iio_dev *indio_dev)
 	st_sensors_set_axis_enable(indio_dev, ST_SENSORS_ENABLE_ALL_AXIS);
 st_gyro_buffer_predisable:
 	iio_triggered_buffer_predisable(indio_dev);
-st_gyro_free_buffer:
-	kfree(gdata->buffer_data);
 	return err;
 }
 
 static int st_gyro_buffer_predisable(struct iio_dev *indio_dev)
 {
 	int err, err2;
-	struct st_sensor_data *gdata = iio_priv(indio_dev);
 
 	err = st_sensors_set_enable(indio_dev, false);
 	if (err < 0)
@@ -79,13 +70,14 @@ static int st_gyro_buffer_predisable(struct iio_dev *indio_dev)
 	if (!err)
 		err = err2;
 
-	kfree(gdata->buffer_data);
 	return err;
 }
 
 static const struct iio_buffer_setup_ops st_gyro_buffer_setup_ops = {
+	.preenable = &st_sensors_buffer_preenable,
 	.postenable = &st_gyro_buffer_postenable,
 	.predisable = &st_gyro_buffer_predisable,
+	.postdisable = &st_sensors_buffer_postdisable,
 };
 
 int st_gyro_allocate_ring(struct iio_dev *indio_dev)
-- 
2.22.0

[PATCH 4/5] iio:magn: do not allocate/de-allocate buffer here but link setup_ops to st_sensors_buffer

[Denis Ciocca]

Setup buffer_setup_ops pre/post enable/disable to use
st_sensor_buffers functions for memory allocation/de-allocation.

Signed-off-by: Denis Ciocca <denis.ciocca@st.com>
---
 drivers/iio/magnetometer/st_magn_buffer.c | 14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)

diff --git a/drivers/iio/magnetometer/st_magn_buffer.c b/drivers/iio/magnetometer/st_magn_buffer.c
index 658d627dad8e..592972f6450e 100644
--- a/drivers/iio/magnetometer/st_magn_buffer.c
+++ b/drivers/iio/magnetometer/st_magn_buffer.c
@@ -31,17 +31,11 @@ int st_magn_trig_set_state(struct iio_trigger *trig, bool state)
 
 static int st_magn_buffer_postenable(struct iio_dev *indio_dev)
 {
-	struct st_sensor_data *mdata = iio_priv(indio_dev);
 	int err;
 
-	mdata->buffer_data = kmalloc(indio_dev->scan_bytes,
-				     GFP_DMA | GFP_KERNEL);
-	if (!mdata->buffer_data)
-		return -ENOMEM;
-
 	err = iio_triggered_buffer_postenable(indio_dev);
 	if (err < 0)
-		goto st_magn_free_buffer;
+		return err;
 
 	err = st_sensors_set_enable(indio_dev, true);
 	if (err < 0)
@@ -51,14 +45,11 @@ static int st_magn_buffer_postenable(struct iio_dev *indio_dev)
 
 st_magn_buffer_predisable:
 	iio_triggered_buffer_predisable(indio_dev);
-st_magn_free_buffer:
-	kfree(mdata->buffer_data);
 	return err;
 }
 
 static int st_magn_buffer_predisable(struct iio_dev *indio_dev)
 {
-	struct st_sensor_data *mdata = iio_priv(indio_dev);
 	int err, err2;
 
 	err = st_sensors_set_enable(indio_dev, false);
@@ -67,13 +58,14 @@ static int st_magn_buffer_predisable(struct iio_dev *indio_dev)
 	if (!err)
 		err = err2;
 
-	kfree(mdata->buffer_data);
 	return err;
 }
 
 static const struct iio_buffer_setup_ops st_magn_buffer_setup_ops = {
+	.preenable = &st_sensors_buffer_preenable,
 	.postenable = &st_magn_buffer_postenable,
 	.predisable = &st_magn_buffer_predisable,
+	.postdisable = &st_sensors_buffer_postdisable,
 };
 
 int st_magn_allocate_ring(struct iio_dev *indio_dev)
-- 
2.22.0

[PATCH 5/5] iio:pressure: do not allocate/de-allocate buffer here but link setup_ops to st_sensors_buffer

[Denis Ciocca]

Setup buffer_setup_ops pre/post enable/disable to use
st_sensor_buffers functions for memory allocation/de-allocation.

Signed-off-by: Denis Ciocca <denis.ciocca@st.com>
---
 drivers/iio/pressure/st_pressure_buffer.c | 14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)

diff --git a/drivers/iio/pressure/st_pressure_buffer.c b/drivers/iio/pressure/st_pressure_buffer.c
index 77cb2d862f19..4306541953d0 100644
--- a/drivers/iio/pressure/st_pressure_buffer.c
+++ b/drivers/iio/pressure/st_pressure_buffer.c
@@ -31,17 +31,11 @@ int st_press_trig_set_state(struct iio_trigger *trig, bool state)
 
 static int st_press_buffer_postenable(struct iio_dev *indio_dev)
 {
-	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)
-		return -ENOMEM;
-
 	err = iio_triggered_buffer_postenable(indio_dev);
 	if (err < 0)
-		goto st_press_free_buffer;
+		return err;
 
 	err = st_sensors_set_enable(indio_dev, true);
 	if (err < 0)
@@ -51,14 +45,11 @@ static int st_press_buffer_postenable(struct iio_dev *indio_dev)
 
 st_press_buffer_predisable:
 	iio_triggered_buffer_predisable(indio_dev);
-st_press_free_buffer:
-	kfree(press_data->buffer_data);
 	return err;
 }
 
 static int st_press_buffer_predisable(struct iio_dev *indio_dev)
 {
-	struct st_sensor_data *press_data = iio_priv(indio_dev);
 	int err, err2;
 
 	err = st_sensors_set_enable(indio_dev, false);
@@ -67,13 +58,14 @@ static int st_press_buffer_predisable(struct iio_dev *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_sensors_buffer_preenable,
 	.postenable = &st_press_buffer_postenable,
 	.predisable = &st_press_buffer_predisable,
+	.postdisable = &st_sensors_buffer_postdisable,
 };
 
 int st_press_allocate_ring(struct iio_dev *indio_dev)
-- 
2.22.0

Re: [PATCH 0/5] move buffer allocation into st_sensors_buffer

[Ardelean, Alexandru]

On Wed, 2019-07-31 at 14:52 -0700, Denis Ciocca wrote:
> [External]
> 
> As proposed by Alexandru, memory allocation and de-allocation can be
> performed in a more neat way in st_sensors_buffer.
> 

Thank you for this series as well.
As a note: I think this depends on the previous series: "[PATCH v2 0/4] preenable/postenable/predisable fixup for ST
drivers"

The changeset looks neat.

Alex

> Denis Ciocca (5):
>   iio:common: introduce st_sensors_buffer_preenable/predisable functions
>   iio:accel: do not allocate/de-allocate buffer here but link setup_ops
>     to st_sensors_buffer
>   iio:gyro: do not allocate/de-allocate buffer here but link setup_ops
>     to st_sensors_buffer
>   iio:magn: do not allocate/de-allocate buffer here but link setup_ops
>     to st_sensors_buffer
>   iio:pressure: do not allocate/de-allocate buffer here but link
>     setup_ops to st_sensors_buffer
> 
>  drivers/iio/accel/st_accel_buffer.c           | 14 +++---------
>  .../iio/common/st_sensors/st_sensors_buffer.c | 22 +++++++++++++++++++
>  drivers/iio/gyro/st_gyro_buffer.c             | 14 +++---------
>  drivers/iio/magnetometer/st_magn_buffer.c     | 14 +++---------
>  drivers/iio/pressure/st_pressure_buffer.c     | 14 +++---------
>  include/linux/iio/common/st_sensors.h         |  2 ++
>  6 files changed, 36 insertions(+), 44 deletions(-)
> 

Re: [PATCH 1/5] iio:common: introduce st_sensors_buffer_preenable/predisable functions

[Ardelean, Alexandru]

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@analog.com>

> Signed-off-by: Denis Ciocca <denis.ciocca@st.com>
> ---
>  .../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@st.com>");
>  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

Re: [PATCH 2/5] iio:accel: do not allocate/de-allocate buffer here but link setup_ops to st_sensors_buffer

[Ardelean, Alexandru]

On Wed, 2019-07-31 at 14:52 -0700, Denis Ciocca wrote:
> [External]
> 
> Setup buffer_setup_ops pre/post enable/disable to use
> st_sensor_buffers functions for memory allocation/de-allocation.
> 

Reviewed-by: Alexandru Ardelean <alexandru.ardelean@analog.com>

> Signed-off-by: Denis Ciocca <denis.ciocca@st.com>
> ---
>  drivers/iio/accel/st_accel_buffer.c | 14 +++-----------
>  1 file changed, 3 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/iio/accel/st_accel_buffer.c b/drivers/iio/accel/st_accel_buffer.c
> index 59dcef02ec19..fc1ba52152ab 100644
> --- a/drivers/iio/accel/st_accel_buffer.c
> +++ b/drivers/iio/accel/st_accel_buffer.c
> @@ -31,17 +31,11 @@ int st_accel_trig_set_state(struct iio_trigger *trig, bool state)
>  
>  static int st_accel_buffer_postenable(struct iio_dev *indio_dev)
>  {
> -	struct st_sensor_data *adata = iio_priv(indio_dev);
>  	int err;
>  
> -	adata->buffer_data = kmalloc(indio_dev->scan_bytes,
> -				     GFP_DMA | GFP_KERNEL);
> -	if (!adata->buffer_data)
> -		return -ENOMEM;
> -
>  	err = iio_triggered_buffer_postenable(indio_dev);
>  	if (err < 0)
> -		goto st_accel_free_buffer;
> +		return err;
>  
>  	err = st_sensors_set_axis_enable(indio_dev,
>  					 (u8)indio_dev->active_scan_mask[0]);
> @@ -58,14 +52,11 @@ static int st_accel_buffer_postenable(struct iio_dev *indio_dev)
>  	st_sensors_set_axis_enable(indio_dev, ST_SENSORS_ENABLE_ALL_AXIS);
>  st_accel_buffer_predisable:
>  	iio_triggered_buffer_predisable(indio_dev);
> -st_accel_free_buffer:
> -	kfree(adata->buffer_data);
>  	return err;
>  }
>  
>  static int st_accel_buffer_predisable(struct iio_dev *indio_dev)
>  {
> -	struct st_sensor_data *adata = iio_priv(indio_dev);
>  	int err, err2;
>  
>  	err = st_sensors_set_enable(indio_dev, false);
> @@ -79,13 +70,14 @@ static int st_accel_buffer_predisable(struct iio_dev *indio_dev)
>  	if (!err)
>  		err = err2;
>  
> -	kfree(adata->buffer_data);
>  	return err;
>  }
>  
>  static const struct iio_buffer_setup_ops st_accel_buffer_setup_ops = {
> +	.preenable = &st_sensors_buffer_preenable,
>  	.postenable = &st_accel_buffer_postenable,
>  	.predisable = &st_accel_buffer_predisable,
> +	.postdisable = &st_sensors_buffer_postdisable,
>  };
>  
>  int st_accel_allocate_ring(struct iio_dev *indio_dev)

Re: [PATCH 3/5] iio:gyro: do not allocate/de-allocate buffer here but link setup_ops to st_sensors_buffer

[Ardelean, Alexandru]

On Wed, 2019-07-31 at 14:52 -0700, Denis Ciocca wrote:
> [External]
> 
> Setup buffer_setup_ops pre/post enable/disable to use
> st_sensor_buffers functions for memory allocation/de-allocation.
> 

Reviewed-by: Alexandru Ardelean <alexandru.ardelean@analog.com>

> Signed-off-by: Denis Ciocca <denis.ciocca@st.com>
> ---
>  drivers/iio/gyro/st_gyro_buffer.c | 14 +++-----------
>  1 file changed, 3 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/iio/gyro/st_gyro_buffer.c b/drivers/iio/gyro/st_gyro_buffer.c
> index c6ddfecc1fc3..48811457a943 100644
> --- a/drivers/iio/gyro/st_gyro_buffer.c
> +++ b/drivers/iio/gyro/st_gyro_buffer.c
> @@ -31,17 +31,11 @@ int st_gyro_trig_set_state(struct iio_trigger *trig, bool state)
>  
>  static int st_gyro_buffer_postenable(struct iio_dev *indio_dev)
>  {
> -	struct st_sensor_data *gdata = iio_priv(indio_dev);
>  	int err;
>  
> -	gdata->buffer_data = kmalloc(indio_dev->scan_bytes,
> -				     GFP_DMA | GFP_KERNEL);
> -	if (!gdata->buffer_data)
> -		return -ENOMEM;
> -
>  	err = iio_triggered_buffer_postenable(indio_dev);
>  	if (err < 0)
> -		goto st_gyro_free_buffer;
> +		return err;
>  
>  	err = st_sensors_set_axis_enable(indio_dev,
>  					 (u8)indio_dev->active_scan_mask[0]);
> @@ -58,15 +52,12 @@ static int st_gyro_buffer_postenable(struct iio_dev *indio_dev)
>  	st_sensors_set_axis_enable(indio_dev, ST_SENSORS_ENABLE_ALL_AXIS);
>  st_gyro_buffer_predisable:
>  	iio_triggered_buffer_predisable(indio_dev);
> -st_gyro_free_buffer:
> -	kfree(gdata->buffer_data);
>  	return err;
>  }
>  
>  static int st_gyro_buffer_predisable(struct iio_dev *indio_dev)
>  {
>  	int err, err2;
> -	struct st_sensor_data *gdata = iio_priv(indio_dev);
>  
>  	err = st_sensors_set_enable(indio_dev, false);
>  	if (err < 0)
> @@ -79,13 +70,14 @@ static int st_gyro_buffer_predisable(struct iio_dev *indio_dev)
>  	if (!err)
>  		err = err2;
>  
> -	kfree(gdata->buffer_data);
>  	return err;
>  }
>  
>  static const struct iio_buffer_setup_ops st_gyro_buffer_setup_ops = {
> +	.preenable = &st_sensors_buffer_preenable,
>  	.postenable = &st_gyro_buffer_postenable,
>  	.predisable = &st_gyro_buffer_predisable,
> +	.postdisable = &st_sensors_buffer_postdisable,
>  };
>  
>  int st_gyro_allocate_ring(struct iio_dev *indio_dev)

Re: [PATCH 4/5] iio:magn: do not allocate/de-allocate buffer here but link setup_ops to st_sensors_buffer

[Ardelean, Alexandru]

On Wed, 2019-07-31 at 14:52 -0700, Denis Ciocca wrote:
> [External]
> 
> Setup buffer_setup_ops pre/post enable/disable to use
> st_sensor_buffers functions for memory allocation/de-allocation.
> 

Reviewed-by: Alexandru Ardelean <alexandru.ardelean@analog.com>

> Signed-off-by: Denis Ciocca <denis.ciocca@st.com>
> ---
>  drivers/iio/magnetometer/st_magn_buffer.c | 14 +++-----------
>  1 file changed, 3 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/iio/magnetometer/st_magn_buffer.c b/drivers/iio/magnetometer/st_magn_buffer.c
> index 658d627dad8e..592972f6450e 100644
> --- a/drivers/iio/magnetometer/st_magn_buffer.c
> +++ b/drivers/iio/magnetometer/st_magn_buffer.c
> @@ -31,17 +31,11 @@ int st_magn_trig_set_state(struct iio_trigger *trig, bool state)
>  
>  static int st_magn_buffer_postenable(struct iio_dev *indio_dev)
>  {
> -	struct st_sensor_data *mdata = iio_priv(indio_dev);
>  	int err;
>  
> -	mdata->buffer_data = kmalloc(indio_dev->scan_bytes,
> -				     GFP_DMA | GFP_KERNEL);
> -	if (!mdata->buffer_data)
> -		return -ENOMEM;
> -
>  	err = iio_triggered_buffer_postenable(indio_dev);
>  	if (err < 0)
> -		goto st_magn_free_buffer;
> +		return err;
>  
>  	err = st_sensors_set_enable(indio_dev, true);
>  	if (err < 0)
> @@ -51,14 +45,11 @@ static int st_magn_buffer_postenable(struct iio_dev *indio_dev)
>  
>  st_magn_buffer_predisable:
>  	iio_triggered_buffer_predisable(indio_dev);
> -st_magn_free_buffer:
> -	kfree(mdata->buffer_data);
>  	return err;
>  }
>  
>  static int st_magn_buffer_predisable(struct iio_dev *indio_dev)
>  {
> -	struct st_sensor_data *mdata = iio_priv(indio_dev);
>  	int err, err2;
>  
>  	err = st_sensors_set_enable(indio_dev, false);
> @@ -67,13 +58,14 @@ static int st_magn_buffer_predisable(struct iio_dev *indio_dev)
>  	if (!err)
>  		err = err2;
>  
> -	kfree(mdata->buffer_data);
>  	return err;
>  }
>  
>  static const struct iio_buffer_setup_ops st_magn_buffer_setup_ops = {
> +	.preenable = &st_sensors_buffer_preenable,
>  	.postenable = &st_magn_buffer_postenable,
>  	.predisable = &st_magn_buffer_predisable,
> +	.postdisable = &st_sensors_buffer_postdisable,
>  };
>  
>  int st_magn_allocate_ring(struct iio_dev *indio_dev)

Re: [PATCH 5/5] iio:pressure: do not allocate/de-allocate buffer here but link setup_ops to st_sensors_buffer

[Ardelean, Alexandru]

On Wed, 2019-07-31 at 14:52 -0700, Denis Ciocca wrote:
> [External]
> 
> Setup buffer_setup_ops pre/post enable/disable to use
> st_sensor_buffers functions for memory allocation/de-allocation.
> 

Reviewed-by: Alexandru Ardelean <alexandru.ardelean@analog.com>

> Signed-off-by: Denis Ciocca <denis.ciocca@st.com>
> ---
>  drivers/iio/pressure/st_pressure_buffer.c | 14 +++-----------
>  1 file changed, 3 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/iio/pressure/st_pressure_buffer.c b/drivers/iio/pressure/st_pressure_buffer.c
> index 77cb2d862f19..4306541953d0 100644
> --- a/drivers/iio/pressure/st_pressure_buffer.c
> +++ b/drivers/iio/pressure/st_pressure_buffer.c
> @@ -31,17 +31,11 @@ int st_press_trig_set_state(struct iio_trigger *trig, bool state)
>  
>  static int st_press_buffer_postenable(struct iio_dev *indio_dev)
>  {
> -	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)
> -		return -ENOMEM;
> -
>  	err = iio_triggered_buffer_postenable(indio_dev);
>  	if (err < 0)
> -		goto st_press_free_buffer;
> +		return err;
>  
>  	err = st_sensors_set_enable(indio_dev, true);
>  	if (err < 0)
> @@ -51,14 +45,11 @@ static int st_press_buffer_postenable(struct iio_dev *indio_dev)
>  
>  st_press_buffer_predisable:
>  	iio_triggered_buffer_predisable(indio_dev);
> -st_press_free_buffer:
> -	kfree(press_data->buffer_data);
>  	return err;
>  }
>  
>  static int st_press_buffer_predisable(struct iio_dev *indio_dev)
>  {
> -	struct st_sensor_data *press_data = iio_priv(indio_dev);
>  	int err, err2;
>  
>  	err = st_sensors_set_enable(indio_dev, false);
> @@ -67,13 +58,14 @@ static int st_press_buffer_predisable(struct iio_dev *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_sensors_buffer_preenable,
>  	.postenable = &st_press_buffer_postenable,
>  	.predisable = &st_press_buffer_predisable,
> +	.postdisable = &st_sensors_buffer_postdisable,
>  };
>  
>  int st_press_allocate_ring(struct iio_dev *indio_dev)

Re: [PATCH 1/5] iio:common: introduce st_sensors_buffer_preenable/predisable functions

[Jonathan Cameron]

On Thu, 1 Aug 2019 08:24:10 +0000
"Ardelean, Alexandru" <alexandru.Ardelean@analog.com> 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@analog.com>
> 
> > Signed-off-by: Denis Ciocca <denis.ciocca@st.com>

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.

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@st.com>");
> >  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  

Re: [PATCH 1/5] iio:common: introduce st_sensors_buffer_preenable/predisable functions

[Jonathan Cameron]

On Mon, 5 Aug 2019 16:21:35 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

> On Thu, 1 Aug 2019 08:24:10 +0000
> "Ardelean, Alexandru" <alexandru.Ardelean@analog.com> 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@analog.com>
> >   
> > > Signed-off-by: Denis Ciocca <denis.ciocca@st.com>  
> 
> 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@st.com>");
> > >  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    
> 

RE: [PATCH 1/5] iio:common: introduce st_sensors_buffer_preenable/predisable functions

[Denis CIOCCA]

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@jic23.retrosnub.co.uk>
> Sent: Monday, August 5, 2019 8:36 AM
> To: Ardelean, Alexandru <alexandru.Ardelean@analog.com>
> Cc: Denis CIOCCA <denis.ciocca@st.com>; linux-iio@vger.kernel.org
> 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@kernel.org> wrote:
> 
> > On Thu, 1 Aug 2019 08:24:10 +0000
> > "Ardelean, Alexandru" <alexandru.Ardelean@analog.com> 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@analog.com>
> > >
> > > > Signed-off-by: Denis Ciocca <denis.ciocca@st.com>
> >
> > 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@st.com>");
> > > > 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
> >

RE: [PATCH 1/5] iio:common: introduce st_sensors_buffer_preenable/predisable functions

[Denis CIOCCA]

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@jic23.retrosnub.co.uk>; Ardelean, Alexandru
> <alexandru.Ardelean@analog.com>
> Cc: linux-iio@vger.kernel.org
> 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@jic23.retrosnub.co.uk>
> > Sent: Monday, August 5, 2019 8:36 AM
> > To: Ardelean, Alexandru <alexandru.Ardelean@analog.com>
> > Cc: Denis CIOCCA <denis.ciocca@st.com>; linux-iio@vger.kernel.org
> > 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@kernel.org> wrote:
> >
> > > On Thu, 1 Aug 2019 08:24:10 +0000
> > > "Ardelean, Alexandru" <alexandru.Ardelean@analog.com> 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@analog.com>
> > > >
> > > > > Signed-off-by: Denis Ciocca <denis.ciocca@st.com>
> > >
> > > 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@st.com>");
> > > > > 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
> > >