[v2] iio: st_sensors: drop temporary kmalloc buffer and reuse buffer_data

[PATCH v2] iio: st_sensors: drop temporary kmalloc buffer and reuse buffer_data

Posted by Sanjay Chitroda 3 weeks, 5 days ago

From: Sanjay Chitroda <sanjayembeddedse@gmail.com>

Replace the per-call kmalloc() scratch buffer with the statically allocated
buffer_data[] field present in struct st_sensor_data. The existing buffer
is DMA-aligned and sufficiently sized for all channel widths, so using it
avoids unnecessary dynamic memory allocation on each read.

This simplifies the code, removes redundent code and allocation.
No functional change intended.

Signed-off-by: Sanjay Chitroda <sanjayembeddedse@gmail.com>
---
Changes in v2:
- split series to individual patch
- address review comment from David Lechner and reuse exising buffer instead of allocation
- Link to v1 https://lore.kernel.org/all/20260310200513.2162018-4-sanjayembedded@gmail.com/
---
 drivers/iio/common/st_sensors/st_sensors_core.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/drivers/iio/common/st_sensors/st_sensors_core.c b/drivers/iio/common/st_sensors/st_sensors_core.c
index dac593be5695..488e2bf6d117 100644
--- a/drivers/iio/common/st_sensors/st_sensors_core.c
+++ b/drivers/iio/common/st_sensors/st_sensors_core.c
@@ -495,20 +495,17 @@ static int st_sensors_read_axis_data(struct iio_dev *indio_dev,
 				     struct iio_chan_spec const *ch, int *data)
 {
 	int err;
-	u8 *outdata;
 	struct st_sensor_data *sdata = iio_priv(indio_dev);
 	unsigned int byte_for_channel;
 
 	byte_for_channel = DIV_ROUND_UP(ch->scan_type.realbits +
 					ch->scan_type.shift, 8);
-	outdata = kmalloc(byte_for_channel, GFP_DMA | GFP_KERNEL);
-	if (!outdata)
-		return -ENOMEM;
+	u8 *outdata = (u8 *)sdata->buffer_data;
 
 	err = regmap_bulk_read(sdata->regmap, ch->address,
 			       outdata, byte_for_channel);
 	if (err < 0)
-		goto st_sensors_free_memory;
+		return err;
 
 	if (byte_for_channel == 1)
 		*data = (s8)*outdata;
@@ -517,10 +514,7 @@ static int st_sensors_read_axis_data(struct iio_dev *indio_dev,
 	else if (byte_for_channel == 3)
 		*data = (s32)sign_extend32(get_unaligned_le24(outdata), 23);
 
-st_sensors_free_memory:
-	kfree(outdata);
-
-	return err;
+	return 0;
 }
 
 int st_sensors_read_info_raw(struct iio_dev *indio_dev,
-- 
2.34.1

Re: [PATCH v2] iio: st_sensors: drop temporary kmalloc buffer and reuse buffer_data

Posted by Jonathan Cameron 3 weeks, 1 day ago

On Wed, 11 Mar 2026 23:50:50 +0530
Sanjay Chitroda <sanjayembeddedse@gmail.com> wrote:

> From: Sanjay Chitroda <sanjayembeddedse@gmail.com>
> 
> Replace the per-call kmalloc() scratch buffer with the statically allocated
> buffer_data[] field present in struct st_sensor_data. The existing buffer
> is DMA-aligned and sufficiently sized for all channel widths, so using it
> avoids unnecessary dynamic memory allocation on each read.
> 
> This simplifies the code, removes redundent code and allocation.
> No functional change intended.
> 
> Signed-off-by: Sanjay Chitroda <sanjayembeddedse@gmail.com>
> ---
> Changes in v2:
> - split series to individual patch
> - address review comment from David Lechner and reuse exising buffer instead of allocation
> - Link to v1 https://lore.kernel.org/all/20260310200513.2162018-4-sanjayembedded@gmail.com/
> ---
>  drivers/iio/common/st_sensors/st_sensors_core.c | 12 +++---------
>  1 file changed, 3 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/iio/common/st_sensors/st_sensors_core.c b/drivers/iio/common/st_sensors/st_sensors_core.c
> index dac593be5695..488e2bf6d117 100644
> --- a/drivers/iio/common/st_sensors/st_sensors_core.c
> +++ b/drivers/iio/common/st_sensors/st_sensors_core.c
> @@ -495,20 +495,17 @@ static int st_sensors_read_axis_data(struct iio_dev *indio_dev,
>  				     struct iio_chan_spec const *ch, int *data)
>  {
>  	int err;
> -	u8 *outdata;
>  	struct st_sensor_data *sdata = iio_priv(indio_dev);
>  	unsigned int byte_for_channel;
>  
>  	byte_for_channel = DIV_ROUND_UP(ch->scan_type.realbits +
>  					ch->scan_type.shift, 8);
> -	outdata = kmalloc(byte_for_channel, GFP_DMA | GFP_KERNEL);
> -	if (!outdata)
> -		return -ENOMEM;
> +	u8 *outdata = (u8 *)sdata->buffer_data;
Why is buffer_data a char array?  It's already implicitly cast to a u8
in one place. I'd see if it actually wants to be a char anywhere in the driver.
If not fix that as a precursor patch to this one.

I'm not sure I'd bother having a local variable for outdata given
it is only used a few times but I don't mind it much (beyond what Andy
pointed out about declaring it here).

Jonathan

>  
>  	err = regmap_bulk_read(sdata->regmap, ch->address,
>  			       outdata, byte_for_channel);
>  	if (err < 0)
> -		goto st_sensors_free_memory;
> +		return err;
>  
>  	if (byte_for_channel == 1)
>  		*data = (s8)*outdata;
> @@ -517,10 +514,7 @@ static int st_sensors_read_axis_data(struct iio_dev *indio_dev,
>  	else if (byte_for_channel == 3)
>  		*data = (s32)sign_extend32(get_unaligned_le24(outdata), 23);
>  
> -st_sensors_free_memory:
> -	kfree(outdata);
> -
> -	return err;
> +	return 0;
>  }
>  
>  int st_sensors_read_info_raw(struct iio_dev *indio_dev,

Re: [PATCH v2] iio: st_sensors: drop temporary kmalloc buffer and reuse buffer_data

Posted by Andy Shevchenko 3 weeks, 5 days ago

On Wed, Mar 11, 2026 at 8:21 PM Sanjay Chitroda
<sanjayembeddedse@gmail.com> wrote:
>
> From: Sanjay Chitroda <sanjayembeddedse@gmail.com>
>
> Replace the per-call kmalloc() scratch buffer with the statically allocated
> buffer_data[] field present in struct st_sensor_data. The existing buffer
> is DMA-aligned and sufficiently sized for all channel widths, so using it
> avoids unnecessary dynamic memory allocation on each read.
>
> This simplifies the code, removes redundent code and allocation.

Spell check your commit messages.

> No functional change intended.

...

>         int err;
> -       u8 *outdata;
>         struct st_sensor_data *sdata = iio_priv(indio_dev);
>         unsigned int byte_for_channel;
>
>         byte_for_channel = DIV_ROUND_UP(ch->scan_type.realbits +
>                                         ch->scan_type.shift, 8);

> +       u8 *outdata = (u8 *)sdata->buffer_data;

No. We don't mess up with C99 definitions. The only exceptions are
loop iterators and scoped variables which are none in this case.



-- 
With Best Regards,
Andy Shevchenko