[PATCH v5 0/5] iio: ssp_sensors: improve resource cleanup

[PATCH v5 0/5] iio: ssp_sensors: improve resource cleanup

[Sanjay Chitroda]

From: Sanjay Chitroda <sanjayembeddedse@gmail.com>

Hi all,

This patch series improves resource cleanup and error handling in the
SSP IIO SPI driver by adopting the recently introduced cleanup
helpers.

The changes focus on making probe/remove paths more robust and easier
to reason about by reducing manual unwind logic and ensuring that locks
and dynamically allocated resources are released consistently across
all exit paths.

Key highlights of this series:

- Reuse preallocate rx buffer for SPI transfer instead of allocating new
  DMA memory in interrupt context for write transaction between AP <-> HUB.
- Replace dynamic resource lifetime management with device‑managed
  (devm_) APIs to simplify cleanup and error paths.
- Refactor common helper API, improving readability and reducing
  duplicatation.
- Address minor codestyle warnings introduced or exposed by the cleanup
  refactoring.

Changes in v5:
- Drop usage of guard() helpers to avoid mixing mutex_lock() with
  guard()(), based on reviewer feedback.
- 0003: Refactor shared helper API, reducing duplication.
- 0004: Convert resource allocation and teardown to devm_ managed APIs to
  simplify error handling and probe/remove paths.
Changes in v4:
- Update sequence and make checkpatch style fix in base change
- Split WARNING and CHECK change with input of Andy as 0001 and 0002
- 0003: Use guard() instead of scoped_guard()
- 0004: Use preallocated buffer to stash memory allocation

No functional behavior changes are intended.

Testing:
  - Compiled with W=1
  - Build-tested on QEMU x86_64

Based on:
  <linux-v7.0-rc5>

Feedback and reviews are very welcome.

Thanks,
Sanjay Chitroda

Sanjay Chitroda (5):
  iio: ssp_sensors: cleanup codestyle warning
  iio: ssp_sensors: cleanup codestyle check
  iio: ssp_sensors: factor out pending list add/remove helpers
  iio: ssp_sensors: use devm APIs for mutex and IRQ resources
  iio: ssp_sensors: reuse preallocated RX buffer for SPI transfers

 drivers/iio/common/ssp_sensors/ssp.h     |  5 ++
 drivers/iio/common/ssp_sensors/ssp_dev.c | 48 +++++++++++--------
 drivers/iio/common/ssp_sensors/ssp_spi.c | 61 +++++++++++-------------
 3 files changed, 62 insertions(+), 52 deletions(-)

-- 
2.34.1

[PATCH v5 1/5] iio: ssp_sensors: cleanup codestyle warning

[Sanjay Chitroda]

From: Sanjay Chitroda <sanjayembeddedse@gmail.com>

Reported by checkpatch:
FILE: drivers/iio/common/ssp_sensors/ssp_spi.c

WARNING: Prefer __packed over __attribute__((__packed__))
+} __attribute__((__packed__));

Signed-off-by: Sanjay Chitroda <sanjayembeddedse@gmail.com>
---
 drivers/iio/common/ssp_sensors/ssp_spi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/common/ssp_sensors/ssp_spi.c b/drivers/iio/common/ssp_sensors/ssp_spi.c
index 6c81c0385fb5..8c1e15d61db7 100644
--- a/drivers/iio/common/ssp_sensors/ssp_spi.c
+++ b/drivers/iio/common/ssp_sensors/ssp_spi.c
@@ -29,7 +29,7 @@ struct ssp_msg_header {
 	__le16 length;
 	__le16 options;
 	__le32 data;
-} __attribute__((__packed__));
+} __packed;
 
 struct ssp_msg {
 	u16 length;
-- 
2.34.1

Re: [PATCH v5 1/5] iio: ssp_sensors: cleanup codestyle warning

[Andy Shevchenko]

On Mon, Apr 06, 2026 at 01:38:48PM +0530, Sanjay Chitroda wrote:
> From: Sanjay Chitroda <sanjayembeddedse@gmail.com>
> 
> Reported by checkpatch:
> FILE: drivers/iio/common/ssp_sensors/ssp_spi.c
> 
> WARNING: Prefer __packed over __attribute__((__packed__))
> +} __attribute__((__packed__));

Do we need __packed to be there in the first place? Isn't it naturally aligned?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 1/5] iio: ssp_sensors: cleanup codestyle warning

[Sanjay Chitroda]

On 7 April 2026 12:03:39 am IST, Andy Shevchenko <andriy.shevchenko@intel.com> wrote:
>On Mon, Apr 06, 2026 at 01:38:48PM +0530, Sanjay Chitroda wrote:
>> From: Sanjay Chitroda <sanjayembeddedse@gmail.com>
>> 
>> Reported by checkpatch:
>> FILE: drivers/iio/common/ssp_sensors/ssp_spi.c
>> 
>> WARNING: Prefer __packed over __attribute__((__packed__))
>> +} __attribute__((__packed__));
>
>Do we need __packed to be there in the first place? Isn't it naturally aligned?
>

This structure isn't naturally aligned as in header structure u8 is followed by __le16 and __le32 fields.
Without packing, compiler inserts padding to satisfy alignment requirement.

Re: [PATCH v5 1/5] iio: ssp_sensors: cleanup codestyle warning

[Andy Shevchenko]

On Tue, Apr 07, 2026 at 11:12:25PM +0530, Sanjay Chitroda wrote:
> On 7 April 2026 12:03:39 am IST, Andy Shevchenko <andriy.shevchenko@intel.com> wrote:
> >On Mon, Apr 06, 2026 at 01:38:48PM +0530, Sanjay Chitroda wrote:

> >> Reported by checkpatch:
> >> FILE: drivers/iio/common/ssp_sensors/ssp_spi.c
> >> 
> >> WARNING: Prefer __packed over __attribute__((__packed__))
> >> +} __attribute__((__packed__));
> >
> >Do we need __packed to be there in the first place? Isn't it naturally aligned?
> 
> This structure isn't naturally aligned as in header structure u8 is followed
> by __le16 and __le32 fields.  Without packing, compiler inserts padding to
> satisfy alignment requirement.

Thank you for confirming this.
Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>

-- 
With Best Regards,
Andy Shevchenko

[PATCH v5 2/5] iio: ssp_sensors: cleanup codestyle check

[Sanjay Chitroda]

From: Sanjay Chitroda <sanjayembeddedse@gmail.com>

Reported by checkpatch:
FILE: drivers/iio/common/ssp_sensors/ssp_spi.c

CHECK: Macro argument '...' may be better as '(...)'
to avoid precedence issues

Signed-off-by: Sanjay Chitroda <sanjayembeddedse@gmail.com>
---
 drivers/iio/common/ssp_sensors/ssp_spi.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/common/ssp_sensors/ssp_spi.c b/drivers/iio/common/ssp_sensors/ssp_spi.c
index 8c1e15d61db7..08ed92859be0 100644
--- a/drivers/iio/common/ssp_sensors/ssp_spi.c
+++ b/drivers/iio/common/ssp_sensors/ssp_spi.c
@@ -6,7 +6,7 @@
 #include "ssp.h"
 
 #define SSP_DEV (&data->spi->dev)
-#define SSP_GET_MESSAGE_TYPE(data) (data & (3 << SSP_RW))
+#define SSP_GET_MESSAGE_TYPE(data) ((data) & (3 << SSP_RW))
 
 /*
  * SSP -> AP Instruction
@@ -119,9 +119,9 @@ static inline void ssp_get_buffer(struct ssp_msg *m, unsigned int offset,
 }
 
 #define SSP_GET_BUFFER_AT_INDEX(m, index) \
-	(m->buffer[SSP_HEADER_SIZE_ALIGNED + index])
+	((m)->buffer[SSP_HEADER_SIZE_ALIGNED + (index)])
 #define SSP_SET_BUFFER_AT_INDEX(m, index, val) \
-	(m->buffer[SSP_HEADER_SIZE_ALIGNED + index] = val)
+	((m)->buffer[SSP_HEADER_SIZE_ALIGNED + (index)] = val)
 
 static void ssp_clean_msg(struct ssp_msg *m)
 {
-- 
2.34.1

[PATCH v5 3/5] iio: ssp_sensors: factor out pending list add/remove helpers

[Sanjay Chitroda]

From: Sanjay Chitroda <sanjayembeddedse@gmail.com>

The SSP SPI transfer path manipulates the pending message list in
multiple places, each time open-coding the same locking and list
operations.

Re-factor the pending list add and delete logic into small helper
functions to avoid duplication and simplify transfer flow to follow.

No functional change intended.

Signed-off-by: Sanjay Chitroda <sanjayembeddedse@gmail.com>
---
 drivers/iio/common/ssp_sensors/ssp_spi.c | 34 +++++++++++++++---------
 1 file changed, 21 insertions(+), 13 deletions(-)

diff --git a/drivers/iio/common/ssp_sensors/ssp_spi.c b/drivers/iio/common/ssp_sensors/ssp_spi.c
index 08ed92859be0..7c1780e15acf 100644
--- a/drivers/iio/common/ssp_sensors/ssp_spi.c
+++ b/drivers/iio/common/ssp_sensors/ssp_spi.c
@@ -174,6 +174,22 @@ static int ssp_check_lines(struct ssp_data *data, bool state)
 	return 0;
 }
 
+static inline void ssp_pending_add(struct ssp_data *data,
+				   struct ssp_msg *msg)
+{
+	mutex_lock(&data->pending_lock);
+	list_add_tail(&msg->list, &data->pending_list);
+	mutex_unlock(&data->pending_lock);
+}
+
+static inline void ssp_pending_del(struct ssp_data *data,
+				   struct ssp_msg *msg)
+{
+	mutex_lock(&data->pending_lock);
+	list_del(&msg->list);
+	mutex_unlock(&data->pending_lock);
+}
+
 static int ssp_do_transfer(struct ssp_data *data, struct ssp_msg *msg,
 			   struct completion *done, int timeout)
 {
@@ -202,19 +218,13 @@ static int ssp_do_transfer(struct ssp_data *data, struct ssp_msg *msg,
 		goto _error_locked;
 	}
 
-	if (!use_no_irq) {
-		mutex_lock(&data->pending_lock);
-		list_add_tail(&msg->list, &data->pending_list);
-		mutex_unlock(&data->pending_lock);
-	}
+	if (!use_no_irq)
+		ssp_pending_add(data, msg);
 
 	status = ssp_check_lines(data, true);
 	if (status < 0) {
-		if (!use_no_irq) {
-			mutex_lock(&data->pending_lock);
-			list_del(&msg->list);
-			mutex_unlock(&data->pending_lock);
-		}
+		if (!use_no_irq)
+			ssp_pending_del(data, msg);
 		goto _error_locked;
 	}
 
@@ -224,9 +234,7 @@ static int ssp_do_transfer(struct ssp_data *data, struct ssp_msg *msg,
 		if (wait_for_completion_timeout(done,
 						msecs_to_jiffies(timeout)) ==
 		    0) {
-			mutex_lock(&data->pending_lock);
-			list_del(&msg->list);
-			mutex_unlock(&data->pending_lock);
+			ssp_pending_del(data, msg);
 
 			data->timeout_cnt++;
 			return -ETIMEDOUT;
-- 
2.34.1

Re: [PATCH v5 3/5] iio: ssp_sensors: factor out pending list add/remove helpers

[Andy Shevchenko]

On Mon, Apr 06, 2026 at 01:38:50PM +0530, Sanjay Chitroda wrote:

> The SSP SPI transfer path manipulates the pending message list in
> multiple places, each time open-coding the same locking and list
> operations.
> 
> Re-factor the pending list add and delete logic into small helper
> functions to avoid duplication and simplify transfer flow to follow.
> 
> No functional change intended.

Suggested-by?

...

> +static inline void ssp_pending_add(struct ssp_data *data,
> +				   struct ssp_msg *msg)

Make it rather:

static inline void ssp_pending_add(struct ssp_data *data, struct ssp_msg *msg)
{
	if (msg->length)
		return;
	...
}

> +{
> +	mutex_lock(&data->pending_lock);
> +	list_add_tail(&msg->list, &data->pending_list);
> +	mutex_unlock(&data->pending_lock);
> +}
> +
> +static inline void ssp_pending_del(struct ssp_data *data,
> +				   struct ssp_msg *msg)
> +{
> +	mutex_lock(&data->pending_lock);
> +	list_del(&msg->list);
> +	mutex_unlock(&data->pending_lock);
> +}

(in the same manner)

...

> -	if (!use_no_irq) {
> -		mutex_lock(&data->pending_lock);
> -		list_add_tail(&msg->list, &data->pending_list);
> -		mutex_unlock(&data->pending_lock);
> -	}
> +	if (!use_no_irq)
> +		ssp_pending_add(data, msg);

...and then it will become

	ssp_pending_add(data, msg);

>  	status = ssp_check_lines(data, true);
>  	if (status < 0) {
> -		if (!use_no_irq) {
> -			mutex_lock(&data->pending_lock);
> -			list_del(&msg->list);
> -			mutex_unlock(&data->pending_lock);
> -		}
> +		if (!use_no_irq)
> +			ssp_pending_del(data, msg);
>  		goto _error_locked;
>  	}

...

>  		if (wait_for_completion_timeout(done,
>  						msecs_to_jiffies(timeout)) ==
>  		    0) {
> -			mutex_lock(&data->pending_lock);
> -			list_del(&msg->list);
> -			mutex_unlock(&data->pending_lock);
> +			ssp_pending_del(data, msg);
>  
>  			data->timeout_cnt++;
>  			return -ETIMEDOUT;

Also rewrite this to follow the style (missing {}, better indentation):

	if (msg->length && done &&
	    !wait_for_completion_timeout(done, msecs_to_jiffies(timeout))) {
		ssp_pending_del(data, msg);

		data->timeout_cnt++;
		return -ETIMEDOUT;
	}

And kill that use_no_urq altogether. Move the comment rather to a helper(s).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 3/5] iio: ssp_sensors: factor out pending list add/remove helpers

[Sanjay Chitroda]

On 7 April 2026 1:34:41 am IST, Andy Shevchenko <andriy.shevchenko@intel.com> wrote:
>On Mon, Apr 06, 2026 at 01:38:50PM +0530, Sanjay Chitroda wrote:
>
>> The SSP SPI transfer path manipulates the pending message list in
>> multiple places, each time open-coding the same locking and list
>> operations.
>> 
>> Re-factor the pending list add and delete logic into small helper
>> functions to avoid duplication and simplify transfer flow to follow.
>> 
>> No functional change intended.
>
>Suggested-by?
>

Requesting your input on which email is recommended to tag with your name.

Shall I use intel.com ?

>...
>
>> +static inline void ssp_pending_add(struct ssp_data *data,
>> +				   struct ssp_msg *msg)
>
>Make it rather:
>
>static inline void ssp_pending_add(struct ssp_data *data, struct ssp_msg *msg)
>{
>	if (msg->length)
>		return;
>	...
>}

It should be not, right ?

{
if (!msg->length)
        return;
...
}

 const bool use_no_irq = msg->length == 0;
 if (!use_no_irq) {add/del}

If message length is 0 then no interrupt

>
>> +{
>> +	mutex_lock(&data->pending_lock);
>> +	list_add_tail(&msg->list, &data->pending_list);
>> +	mutex_unlock(&data->pending_lock);
>> +}
>> +
>> +static inline void ssp_pending_del(struct ssp_data *data,
>> +				   struct ssp_msg *msg)
>> +{
>> +	mutex_lock(&data->pending_lock);
>> +	list_del(&msg->list);
>> +	mutex_unlock(&data->pending_lock);
>> +}
>
>(in the same manner)
>
>...
>
>> -	if (!use_no_irq) {
>> -		mutex_lock(&data->pending_lock);
>> -		list_add_tail(&msg->list, &data->pending_list);
>> -		mutex_unlock(&data->pending_lock);
>> -	}
>> +	if (!use_no_irq)
>> +		ssp_pending_add(data, msg);
>
>...and then it will become
>
>	ssp_pending_add(data, msg);
>
>>  	status = ssp_check_lines(data, true);
>>  	if (status < 0) {
>> -		if (!use_no_irq) {
>> -			mutex_lock(&data->pending_lock);
>> -			list_del(&msg->list);
>> -			mutex_unlock(&data->pending_lock);
>> -		}
>> +		if (!use_no_irq)
>> +			ssp_pending_del(data, msg);
>>  		goto _error_locked;
>>  	}
>
>...
>
>>  		if (wait_for_completion_timeout(done,
>>  						msecs_to_jiffies(timeout)) ==
>>  		    0) {
>> -			mutex_lock(&data->pending_lock);
>> -			list_del(&msg->list);
>> -			mutex_unlock(&data->pending_lock);
>> +			ssp_pending_del(data, msg);
>>  
>>  			data->timeout_cnt++;
>>  			return -ETIMEDOUT;
>
>Also rewrite this to follow the style (missing {}, better indentation):
>
>	if (msg->length && done &&
>	    !wait_for_completion_timeout(done, msecs_to_jiffies(timeout))) {
>		ssp_pending_del(data, msg);
>
>		data->timeout_cnt++;
>		return -ETIMEDOUT;
>	}
>
>And kill that use_no_urq altogether. Move the comment rather to a helper(s).

Understood, will follow and drop this variable in next series.

>

Re: [PATCH v5 3/5] iio: ssp_sensors: factor out pending list add/remove helpers

[Andy Shevchenko]

On Wed, Apr 08, 2026 at 07:32:48AM +0000, Sanjay Embedded_SE wrote:
> On 7 April 2026 1:34:41 am IST, Andy Shevchenko <andriy.shevchenko@intel.com> wrote:
> >On Mon, Apr 06, 2026 at 01:38:50PM +0530, Sanjay Chitroda wrote:

...

> >Suggested-by?
> 
> Requesting your input on which email is recommended to tag with your name.
> 
> Shall I use intel.com ?

The one that was used in the message which suggested this change. I don't
remember which one I have used.

...

> >> +static inline void ssp_pending_add(struct ssp_data *data,
> >> +                               struct ssp_msg *msg)
> >
> >Make it rather:
> >
> >static inline void ssp_pending_add(struct ssp_data *data, struct ssp_msg *msg)
> >{
> >       if (msg->length)
> >               return;
> >       ...
> >}

> It should be not, right ?

Yes, too many negations, perhaps even better to spell it as msg->length == 0.

> {
> if (!msg->length)
>         return;
> ...
> }
> 
>  const bool use_no_irq = msg->length == 0;
>  if (!use_no_irq) {add/del}

> If message length is 0 then no interrupt

-- 
With Best Regards,
Andy Shevchenko

[PATCH v5 4/5] iio: ssp_sensors: use devm APIs for mutex and IRQ resources

[Sanjay Chitroda]

From: Sanjay Chitroda <sanjayembeddedse@gmail.com>

Convert mutex initialization and IRQ registration to use devm-managed
helpers, tying their lifetime to the device.

This removes the need for explicit cleanup in the error and remove
paths, as the resources are automatically released on probe failure
or device unbind.

Signed-off-by: Sanjay Chitroda <sanjayembeddedse@gmail.com>
---
 drivers/iio/common/ssp_sensors/ssp_dev.c | 36 +++++++++++-------------
 1 file changed, 17 insertions(+), 19 deletions(-)

diff --git a/drivers/iio/common/ssp_sensors/ssp_dev.c b/drivers/iio/common/ssp_sensors/ssp_dev.c
index da09c9f3ceb6..f4fc869fc770 100644
--- a/drivers/iio/common/ssp_sensors/ssp_dev.c
+++ b/drivers/iio/common/ssp_sensors/ssp_dev.c
@@ -510,7 +510,11 @@ static int ssp_probe(struct spi_device *spi)
 	data->spi = spi;
 	spi_set_drvdata(spi, data);
 
-	mutex_init(&data->comm_lock);
+	ret = devm_mutex_init(&spi->dev, &data->comm_lock);
+	if (ret < 0) {
+		dev_err(&spi->dev, "Failed to init comm_lock mutex\n");
+		goto err_setup_spi;
+	}
 
 	for (i = 0; i < SSP_SENSOR_MAX; ++i) {
 		data->delay_buf[i] = SSP_DEFAULT_POLLING_DELAY;
@@ -523,7 +527,11 @@ static int ssp_probe(struct spi_device *spi)
 
 	data->time_syncing = true;
 
-	mutex_init(&data->pending_lock);
+	ret = devm_mutex_init(&spi->dev, &data->pending_lock);
+	if (ret < 0) {
+		dev_err(&spi->dev, "Failed to init pending_lock mutex\n");
+		goto err_setup_spi;
+	}
 	INIT_LIST_HEAD(&data->pending_list);
 
 	atomic_set(&data->enable_refcount, 0);
@@ -533,13 +541,13 @@ static int ssp_probe(struct spi_device *spi)
 
 	timer_setup(&data->wdt_timer, ssp_wdt_timer_func, 0);
 
-	ret = request_threaded_irq(data->spi->irq, NULL,
-				   ssp_irq_thread_fn,
-				   IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
-				   "SSP_Int", data);
+	ret = devm_request_threaded_irq(&spi->dev, data->spi->irq, NULL,
+					ssp_irq_thread_fn,
+					IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
+					"SSP_Int", data);
 	if (ret < 0) {
 		dev_err(&spi->dev, "Irq request fail\n");
-		goto err_setup_irq;
+		goto err_setup_spi;
 	}
 
 	/* Let's start with enabled one so irq balance could be ok */
@@ -553,21 +561,16 @@ static int ssp_probe(struct spi_device *spi)
 		ret = ssp_initialize_mcu(data);
 		if (ret < 0) {
 			dev_err(&spi->dev, "Initialize_mcu failed\n");
-			goto err_read_reg;
+			goto err_setup_spi;
 		}
 	} else {
 		dev_err(&spi->dev, "Firmware version not supported\n");
 		ret = -EPERM;
-		goto err_read_reg;
+		goto err_setup_spi;
 	}
 
 	return 0;
 
-err_read_reg:
-	free_irq(data->spi->irq, data);
-err_setup_irq:
-	mutex_destroy(&data->pending_lock);
-	mutex_destroy(&data->comm_lock);
 err_setup_spi:
 	mfd_remove_devices(&spi->dev);
 
@@ -589,14 +592,9 @@ static void ssp_remove(struct spi_device *spi)
 
 	ssp_clean_pending_list(data);
 
-	free_irq(data->spi->irq, data);
-
 	timer_delete_sync(&data->wdt_timer);
 	cancel_work_sync(&data->work_wdt);
 
-	mutex_destroy(&data->comm_lock);
-	mutex_destroy(&data->pending_lock);
-
 	mfd_remove_devices(&spi->dev);
 }
 
-- 
2.34.1

Re: [PATCH v5 4/5] iio: ssp_sensors: use devm APIs for mutex and IRQ resources

[David Lechner]

On 4/6/26 3:08 AM, Sanjay Chitroda wrote:
> From: Sanjay Chitroda <sanjayembeddedse@gmail.com>
> 
> Convert mutex initialization and IRQ registration to use devm-managed
> helpers, tying their lifetime to the device.
> 
> This removes the need for explicit cleanup in the error and remove
> paths, as the resources are automatically released on probe failure
> or device unbind.
> 
> Signed-off-by: Sanjay Chitroda <sanjayembeddedse@gmail.com>
> ---
>  drivers/iio/common/ssp_sensors/ssp_dev.c | 36 +++++++++++-------------
>  1 file changed, 17 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/iio/common/ssp_sensors/ssp_dev.c b/drivers/iio/common/ssp_sensors/ssp_dev.c
> index da09c9f3ceb6..f4fc869fc770 100644
> --- a/drivers/iio/common/ssp_sensors/ssp_dev.c
> +++ b/drivers/iio/common/ssp_sensors/ssp_dev.c
> @@ -510,7 +510,11 @@ static int ssp_probe(struct spi_device *spi)
>  	data->spi = spi;
>  	spi_set_drvdata(spi, data);
>  
> -	mutex_init(&data->comm_lock);
> +	ret = devm_mutex_init(&spi->dev, &data->comm_lock);
> +	if (ret < 0) {
> +		dev_err(&spi->dev, "Failed to init comm_lock mutex\n");
> +		goto err_setup_spi;
> +	}
>  
>  	for (i = 0; i < SSP_SENSOR_MAX; ++i) {
>  		data->delay_buf[i] = SSP_DEFAULT_POLLING_DELAY;
> @@ -523,7 +527,11 @@ static int ssp_probe(struct spi_device *spi)
>  
>  	data->time_syncing = true;
>  
> -	mutex_init(&data->pending_lock);
> +	ret = devm_mutex_init(&spi->dev, &data->pending_lock);
> +	if (ret < 0) {
> +		dev_err(&spi->dev, "Failed to init pending_lock mutex\n");
> +		goto err_setup_spi;
> +	}
>  	INIT_LIST_HEAD(&data->pending_list);
>  
>  	atomic_set(&data->enable_refcount, 0);
> @@ -533,13 +541,13 @@ static int ssp_probe(struct spi_device *spi)
>  
>  	timer_setup(&data->wdt_timer, ssp_wdt_timer_func, 0);
>  
> -	ret = request_threaded_irq(data->spi->irq, NULL,
> -				   ssp_irq_thread_fn,
> -				   IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> -				   "SSP_Int", data);
> +	ret = devm_request_threaded_irq(&spi->dev, data->spi->irq, NULL,
> +					ssp_irq_thread_fn,
> +					IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> +					"SSP_Int", data);
>  	if (ret < 0) {
>  		dev_err(&spi->dev, "Irq request fail\n");
> -		goto err_setup_irq;
> +		goto err_setup_spi;
>  	}
>  
>  	/* Let's start with enabled one so irq balance could be ok */
> @@ -553,21 +561,16 @@ static int ssp_probe(struct spi_device *spi)
>  		ret = ssp_initialize_mcu(data);
>  		if (ret < 0) {
>  			dev_err(&spi->dev, "Initialize_mcu failed\n");
> -			goto err_read_reg;
> +			goto err_setup_spi;
>  		}
>  	} else {
>  		dev_err(&spi->dev, "Firmware version not supported\n");
>  		ret = -EPERM;
> -		goto err_read_reg;
> +		goto err_setup_spi;
>  	}
>  
>  	return 0;
>  
> -err_read_reg:
> -	free_irq(data->spi->irq, data);
> -err_setup_irq:
> -	mutex_destroy(&data->pending_lock);
> -	mutex_destroy(&data->comm_lock);
>  err_setup_spi:
>  	mfd_remove_devices(&spi->dev);
>  
> @@ -589,14 +592,9 @@ static void ssp_remove(struct spi_device *spi)
>  
>  	ssp_clean_pending_list(data);
>  
> -	free_irq(data->spi->irq, data);

Do we need to replace this with an irq_disable() so make sure
no more timers/work is scheduled after we delete them below?

If it is safe the way it is, add an explanation to the commit
message.

Alternative would be to use devm_add_action_or_reset() to
handle all of the other cleanup functions in the correct order
still and get rid of the remove function entirely.

> -
>  	timer_delete_sync(&data->wdt_timer);
>  	cancel_work_sync(&data->work_wdt);
>  
> -	mutex_destroy(&data->comm_lock);
> -	mutex_destroy(&data->pending_lock);
> -
>  	mfd_remove_devices(&spi->dev);
>  }
>  

Re: [PATCH v5 4/5] iio: ssp_sensors: use devm APIs for mutex and IRQ resources

[Andy Shevchenko]

On Mon, Apr 06, 2026 at 01:38:51PM +0530, Sanjay Chitroda wrote:

> Convert mutex initialization and IRQ registration to use devm-managed
> helpers, tying their lifetime to the device.
> 
> This removes the need for explicit cleanup in the error and remove
> paths, as the resources are automatically released on probe failure
> or device unbind.

...

> -	mutex_init(&data->comm_lock);
> +	ret = devm_mutex_init(&spi->dev, &data->comm_lock);
> +	if (ret < 0) {
> +		dev_err(&spi->dev, "Failed to init comm_lock mutex\n");

The message is not needed here.

> +		goto err_setup_spi;

This is half-baked solution. When devm is in use there is shouldn't be goto.
after it.

> +	}

...

> -	ret = request_threaded_irq(data->spi->irq, NULL,
> -				   ssp_irq_thread_fn,
> -				   IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> -				   "SSP_Int", data);
> +	ret = devm_request_threaded_irq(&spi->dev, data->spi->irq, NULL,
> +					ssp_irq_thread_fn,
> +					IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> +					"SSP_Int", data);
>  	if (ret < 0) {

>  		dev_err(&spi->dev, "Irq request fail\n");

Now you should also remove this dup message. devm APIs in IRQ core print
a message for you.

> -		goto err_setup_irq;
> +		goto err_setup_spi;
>  	}

-- 
With Best Regards,
Andy Shevchenko

[PATCH v5 5/5] iio: ssp_sensors: reuse preallocated RX buffer for SPI transfers

[Sanjay Chitroda]

From: Sanjay Chitroda <sanjayembeddedse@gmail.com>

Avoid allocating a temporary DMA buffer in the interrupt context when
handling hub-to-AP and AP-to-hub SPI write messages.

Preallocate RX buffer during probe and reuse it for SPI receive
operations. This removes repeated kzalloc() calls from the IRQ
path, reduces allocation overhead, and avoids potential allocation
failures under memory pressure.

The RX buffer size is tracked and allocated using devm_kzalloc(), ensuring
proper lifetime management tied to the device.

No functional change intended; this is an internal optimization and
robustness improvement.

Signed-off-by: Sanjay Chitroda <sanjayembeddedse@gmail.com>
---
Changes in v5:
- Rebase change on top of latest v5 patch series.
Changes in v4:
- Use preallocated buffer and stash a buffer that gets reused each time instead of a fresh allocation.
- Link to v3: https://lore.kernel.org/all/20260315125509.857195-3-sanjayembedded@gmail.com/
Changes in v3:
- prepare series to have all respective cleanup API support for the ssp_sensors following input from Andy Shevchenko
- Link to v2 https://lore.kernel.org/all/20260311174151.3441429-1-sanjayembedded@gmail.com/
Changes in v2:
- split series to individual patch
- address review comment from Andy Shevchenko
- Link to v1 https://lore.kernel.org/all/20260310200513.2162018-3-sanjayembedded@gmail.com/
---
 drivers/iio/common/ssp_sensors/ssp.h     |  5 +++++
 drivers/iio/common/ssp_sensors/ssp_dev.c | 12 ++++++++++++
 drivers/iio/common/ssp_sensors/ssp_spi.c | 19 +++----------------
 3 files changed, 20 insertions(+), 16 deletions(-)

diff --git a/drivers/iio/common/ssp_sensors/ssp.h b/drivers/iio/common/ssp_sensors/ssp.h
index f649cdecc277..aa125fd1bed5 100644
--- a/drivers/iio/common/ssp_sensors/ssp.h
+++ b/drivers/iio/common/ssp_sensors/ssp.h
@@ -175,6 +175,8 @@ struct ssp_sensorhub_info {
  * @sensor_devs:	registered IIO devices table
  * @enable_refcount:	enable reference count for wdt (watchdog timer)
  * @header_buffer:	cache aligned buffer for packet header
+ * @rx_buf:		buffer to receive SPI data
+ * @rx_buf_size:	allocated size of rx_buf
  */
 struct ssp_data {
 	struct spi_device *spi;
@@ -222,6 +224,9 @@ struct ssp_data {
 	atomic_t enable_refcount;
 
 	__le16 header_buffer[SSP_HEADER_BUFFER_SIZE / sizeof(__le16)] __aligned(IIO_DMA_MINALIGN);
+
+	u8 *rx_buf;
+	size_t rx_buf_size;
 };
 
 void ssp_clean_pending_list(struct ssp_data *data);
diff --git a/drivers/iio/common/ssp_sensors/ssp_dev.c b/drivers/iio/common/ssp_sensors/ssp_dev.c
index aab28f2a0f75..2a8d6f040ae4 100644
--- a/drivers/iio/common/ssp_sensors/ssp_dev.c
+++ b/drivers/iio/common/ssp_sensors/ssp_dev.c
@@ -516,6 +516,18 @@ static int ssp_probe(struct spi_device *spi)
 		goto err_setup_spi;
 	}
 
+	data->rx_buf_size = SSP_DATA_PACKET_SIZE;
+	data->rx_buf = devm_kzalloc(&spi->dev,
+				    data->rx_buf_size,
+				    GFP_KERNEL | GFP_DMA);
+
+	if (!data->rx_buf) {
+		dev_err(&spi->dev,
+			"Failed to allocate memory for rx_buf\n");
+		ret = -ENOMEM;
+		goto err_setup_spi;
+	}
+
 	for (i = 0; i < SSP_SENSOR_MAX; ++i) {
 		data->delay_buf[i] = SSP_DEFAULT_POLLING_DELAY;
 		data->batch_latency_buf[i] = 0;
diff --git a/drivers/iio/common/ssp_sensors/ssp_spi.c b/drivers/iio/common/ssp_sensors/ssp_spi.c
index 7c1780e15acf..2f7445e8b4d1 100644
--- a/drivers/iio/common/ssp_sensors/ssp_spi.c
+++ b/drivers/iio/common/ssp_sensors/ssp_spi.c
@@ -383,19 +383,13 @@ int ssp_irq_msg(struct ssp_data *data)
 			 * but the slave should not send such ones - it is to
 			 * check but let's handle this
 			 */
-			buffer = kmalloc(length, GFP_KERNEL | GFP_DMA);
-			if (!buffer) {
-				ret = -ENOMEM;
-				goto _unlock;
-			}
+			buffer = data->rx_buf;
 
 			/* got dead packet so it is always an error */
 			ret = spi_read(data->spi, buffer, length);
 			if (ret >= 0)
 				ret = -EPROTO;
 
-			kfree(buffer);
-
 			dev_err(SSP_DEV, "No match error %x\n",
 				msg_options);
 
@@ -428,22 +422,15 @@ int ssp_irq_msg(struct ssp_data *data)
 		mutex_unlock(&data->pending_lock);
 		break;
 	case SSP_HUB2AP_WRITE:
-		buffer = kzalloc(length, GFP_KERNEL | GFP_DMA);
-		if (!buffer)
-			return -ENOMEM;
+		buffer = data->rx_buf;
 
 		ret = spi_read(data->spi, buffer, length);
 		if (ret < 0) {
 			dev_err(SSP_DEV, "spi read fail\n");
-			kfree(buffer);
 			break;
 		}
 
-		ret = ssp_parse_dataframe(data, buffer, length);
-
-		kfree(buffer);
-		break;
-
+		return ssp_parse_dataframe(data, buffer, length);
 	default:
 		dev_err(SSP_DEV, "unknown msg type\n");
 		return -EPROTO;
-- 
2.34.1

Re: [PATCH v5 5/5] iio: ssp_sensors: reuse preallocated RX buffer for SPI transfers

[David Lechner]

On 4/6/26 3:08 AM, Sanjay Chitroda wrote:
> From: Sanjay Chitroda <sanjayembeddedse@gmail.com>
> 
> Avoid allocating a temporary DMA buffer in the interrupt context when
> handling hub-to-AP and AP-to-hub SPI write messages.
> 
> Preallocate RX buffer during probe and reuse it for SPI receive
> operations. This removes repeated kzalloc() calls from the IRQ
> path, reduces allocation overhead, and avoids potential allocation
> failures under memory pressure.
> 
> The RX buffer size is tracked and allocated using devm_kzalloc(), ensuring
> proper lifetime management tied to the device.
> 
> No functional change intended; this is an internal optimization and
> robustness improvement.

If we are going to claim this is an optimization, we should have
some measurements to back that up.

> 
> Signed-off-by: Sanjay Chitroda <sanjayembeddedse@gmail.com>
> ---
> Changes in v5:
> - Rebase change on top of latest v5 patch series.
> Changes in v4:
> - Use preallocated buffer and stash a buffer that gets reused each time instead of a fresh allocation.
> - Link to v3: https://lore.kernel.org/all/20260315125509.857195-3-sanjayembedded@gmail.com/
> Changes in v3:
> - prepare series to have all respective cleanup API support for the ssp_sensors following input from Andy Shevchenko
> - Link to v2 https://lore.kernel.org/all/20260311174151.3441429-1-sanjayembedded@gmail.com/
> Changes in v2:
> - split series to individual patch
> - address review comment from Andy Shevchenko
> - Link to v1 https://lore.kernel.org/all/20260310200513.2162018-3-sanjayembedded@gmail.com/
> ---
>  drivers/iio/common/ssp_sensors/ssp.h     |  5 +++++
>  drivers/iio/common/ssp_sensors/ssp_dev.c | 12 ++++++++++++
>  drivers/iio/common/ssp_sensors/ssp_spi.c | 19 +++----------------
>  3 files changed, 20 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/iio/common/ssp_sensors/ssp.h b/drivers/iio/common/ssp_sensors/ssp.h
> index f649cdecc277..aa125fd1bed5 100644
> --- a/drivers/iio/common/ssp_sensors/ssp.h
> +++ b/drivers/iio/common/ssp_sensors/ssp.h
> @@ -175,6 +175,8 @@ struct ssp_sensorhub_info {
>   * @sensor_devs:	registered IIO devices table
>   * @enable_refcount:	enable reference count for wdt (watchdog timer)
>   * @header_buffer:	cache aligned buffer for packet header
> + * @rx_buf:		buffer to receive SPI data
> + * @rx_buf_size:	allocated size of rx_buf
>   */
>  struct ssp_data {
>  	struct spi_device *spi;
> @@ -222,6 +224,9 @@ struct ssp_data {
>  	atomic_t enable_refcount;
>  
>  	__le16 header_buffer[SSP_HEADER_BUFFER_SIZE / sizeof(__le16)] __aligned(IIO_DMA_MINALIGN);
> +
> +	u8 *rx_buf;
> +	size_t rx_buf_size;

No, these can't be after _aligned(IIO_DMA_MINALIGN); without causing problems.

What would work here though is:

	u8 rx_buf[SSP_DATA_PACKET_SIZE];

>  };
>  
>  void ssp_clean_pending_list(struct ssp_data *data);
> diff --git a/drivers/iio/common/ssp_sensors/ssp_dev.c b/drivers/iio/common/ssp_sensors/ssp_dev.c
> index aab28f2a0f75..2a8d6f040ae4 100644
> --- a/drivers/iio/common/ssp_sensors/ssp_dev.c
> +++ b/drivers/iio/common/ssp_sensors/ssp_dev.c
> @@ -516,6 +516,18 @@ static int ssp_probe(struct spi_device *spi)
>  		goto err_setup_spi;
>  	}
>  
> +	data->rx_buf_size = SSP_DATA_PACKET_SIZE;
> +	data->rx_buf = devm_kzalloc(&spi->dev,
> +				    data->rx_buf_size,
> +				    GFP_KERNEL | GFP_DMA);

Since this is a fixed size, we don't need a separate alloc here. We can
just embed the array in the data struct.

> +
> +	if (!data->rx_buf) {
> +		dev_err(&spi->dev,
> +			"Failed to allocate memory for rx_buf\n");
> +		ret = -ENOMEM;
> +		goto err_setup_spi;
> +	}
> +
>  	for (i = 0; i < SSP_SENSOR_MAX; ++i) {
>  		data->delay_buf[i] = SSP_DEFAULT_POLLING_DELAY;
>  		data->batch_latency_buf[i] = 0;
> diff --git a/drivers/iio/common/ssp_sensors/ssp_spi.c b/drivers/iio/common/ssp_sensors/ssp_spi.c
> index 7c1780e15acf..2f7445e8b4d1 100644
> --- a/drivers/iio/common/ssp_sensors/ssp_spi.c
> +++ b/drivers/iio/common/ssp_sensors/ssp_spi.c
> @@ -383,19 +383,13 @@ int ssp_irq_msg(struct ssp_data *data)
>  			 * but the slave should not send such ones - it is to
>  			 * check but let's handle this
>  			 */
> -			buffer = kmalloc(length, GFP_KERNEL | GFP_DMA);
> -			if (!buffer) {
> -				ret = -ENOMEM;
> -				goto _unlock;
> -			}
> +			buffer = data->rx_buf;


I don't think it is helpful to keep the buffer local variable.

>  
>  			/* got dead packet so it is always an error */
>  			ret = spi_read(data->spi, buffer, length);
>  			if (ret >= 0)
>  				ret = -EPROTO;
>  
> -			kfree(buffer);
> -
>  			dev_err(SSP_DEV, "No match error %x\n",
>  				msg_options);
>  
> @@ -428,22 +422,15 @@ int ssp_irq_msg(struct ssp_data *data)
>  		mutex_unlock(&data->pending_lock);
>  		break;
>  	case SSP_HUB2AP_WRITE:
> -		buffer = kzalloc(length, GFP_KERNEL | GFP_DMA);
> -		if (!buffer)
> -			return -ENOMEM;
> +		buffer = data->rx_buf;
>  
>  		ret = spi_read(data->spi, buffer, length);
>  		if (ret < 0) {
>  			dev_err(SSP_DEV, "spi read fail\n");
> -			kfree(buffer);
>  			break;
>  		}
>  
> -		ret = ssp_parse_dataframe(data, buffer, length);
> -
> -		kfree(buffer);
> -		break;
> -
> +		return ssp_parse_dataframe(data, buffer, length);
>  	default:
>  		dev_err(SSP_DEV, "unknown msg type\n");
>  		return -EPROTO;