[PATCH v7 0/9] iio: ssp_sensors: improve resource cleanup

[PATCH v7 0/9] 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 embedded 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 and use dev_err_probe in probe path.

Changes in v7:
- first 2 checkpatch patches are applied to iio/testing
- address coding type review comment from Andy at multiple places
- add warning fix in ssp_dev driver
- add new change to cancel refresh work
- add helper API for enable/disable MCU
- Following input from Andy add local variable dev to use with APIs
- v6 series: https://lore.kernel.org/all/20260415050749.3858046-1-sanjayembedded@gmail.com/
Changes in v6:
- 0005: Drop remove() path using devm action with input from David
- 0007: Replace dynamic memory allocation with embedded fixed size struct buffer
- 0004: new: drop duplication in remove()
- 0006: new: Use dev_err_probe in driver probe()
- v5 series: https://lore.kernel.org/all/20260406080852.2727453-1-sanjayembedded@gmail.com/
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.
- v4 series: https://lore.kernel.org/all/20260326081815.925373-1-sanjayembedded@gmail.com/
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
- v3 series: https://lore.kernel.org/all/20260315125509.857195-1-sanjayembedded@gmail.com/

No functional behavior changes are intended.

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

Feedback and reviews are very welcome.

Thanks,
Sanjay Chitroda

Sanjay Chitroda (9):
  iio: ssp_sensors: cleanup codestyle warning
  iio: ssp_sensors: factor out pending list add/remove helper(s)
  iio: ssp_sensors: cancel delayed work_refresh on remove
  iio: ssp_sensors: factor out mcu enable/disable helper(s)
  iio: ssp_sensors: use local struct device
  iio: ssp_sensors: Drop duplicated wdt timer and work cleanup
  iio: ssp_sensors: convert probe and teardown to devm-managed resources
  iio: ssp_sensors: Use dev_err_probe
  iio: ssp_sensors: reuse embedded RX buffer for SPI transfers

 drivers/iio/common/ssp_sensors/ssp.h     |   3 +
 drivers/iio/common/ssp_sensors/ssp_dev.c | 168 ++++++++++++-----------
 drivers/iio/common/ssp_sensors/ssp_spi.c |  78 +++++------
 3 files changed, 128 insertions(+), 121 deletions(-)


base-commit: eade2b843d9b1f668fc1775f15611bb0a1999cd9
-- 
2.34.1

[PATCH v7 1/9] iio: ssp_sensors: cleanup codestyle warning

[Sanjay Chitroda]

From: Sanjay Chitroda <sanjayembeddedse@gmail.com>

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

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

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

diff --git a/drivers/iio/common/ssp_sensors/ssp_dev.c b/drivers/iio/common/ssp_sensors/ssp_dev.c
index da09c9f3ceb6..7d07fae295fd 100644
--- a/drivers/iio/common/ssp_sensors/ssp_dev.c
+++ b/drivers/iio/common/ssp_sensors/ssp_dev.c
@@ -28,7 +28,7 @@ struct ssp_instruction {
 	__le32 a;
 	__le32 b;
 	u8 c;
-} __attribute__((__packed__));
+} __packed__;
 
 static const u8 ssp_magnitude_table[] = {110, 85, 171, 71, 203, 195, 0, 67,
 	208, 56, 175, 244, 206, 213, 0, 92, 250, 0, 55, 48, 189, 252, 171,
-- 
2.34.1

[PATCH v7 2/9] iio: ssp_sensors: factor out pending list add/remove helper(s)

[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 and drop use_no_irq variable to avoid duplication and
simplify transfer flow to follow.

No functional change intended.

Signed-off-by: Sanjay Chitroda <sanjayembeddedse@gmail.com>
Suggested-by: Andy Shevchenko <andriy.shevchenko@intel.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>
---
Changes in v7:
- Following suggestion from Andy, keep helper API definition in single
  line and re-place the comment section
- v6 change: https://lore.kernel.org/all/20260415050749.3858046-4-sanjayembedded@gmail.com/
Changes in v6:
- Include tag for the suggestion of helper functions
- Drop completely use_no_irq variable with review comment from Andy
- v5 change: https://lore.kernel.org/all/20260406080852.2727453-4-sanjayembedded@gmail.com/
---
 drivers/iio/common/ssp_sensors/ssp_spi.c | 58 ++++++++++++++----------
 1 file changed, 33 insertions(+), 25 deletions(-)

diff --git a/drivers/iio/common/ssp_sensors/ssp_spi.c b/drivers/iio/common/ssp_sensors/ssp_spi.c
index 08ed92859be0..870214551f0b 100644
--- a/drivers/iio/common/ssp_sensors/ssp_spi.c
+++ b/drivers/iio/common/ssp_sensors/ssp_spi.c
@@ -174,15 +174,35 @@ 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)
+{
+	/*
+	 * Check if this is a short one way message or the whole transfer has
+	 * second part after an interrupt.
+	 */
+	if (msg->length == 0)
+		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)
+{
+	/* See ssp_pending_add() for transfer length logic */
+	if (msg->length == 0)
+		return;
+
+	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)
 {
 	int status;
-	/*
-	 * check if this is a short one way message or the whole transfer has
-	 * second part after an interrupt
-	 */
-	const bool use_no_irq = msg->length == 0;
 
 	if (data->shut_down)
 		return -EPERM;
@@ -202,35 +222,23 @@ 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);
-	}
+	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);
-		}
+		ssp_pending_del(data, msg);
 		goto _error_locked;
 	}
 
 	mutex_unlock(&data->comm_lock);
 
-	if (!use_no_irq && done)
-		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);
+	if (msg->length && done &&
+	    !wait_for_completion_timeout(done, msecs_to_jiffies(timeout))) {
+		ssp_pending_del(data, msg);
 
-			data->timeout_cnt++;
-			return -ETIMEDOUT;
-		}
+		data->timeout_cnt++;
+		return -ETIMEDOUT;
+	}
 
 	return 0;
 
-- 
2.34.1

[PATCH v7 3/9] iio: ssp_sensors: cancel delayed work_refresh on remove

[Sanjay Chitroda]

From: Sanjay Chitroda <sanjayembeddedse@gmail.com>

The work_refresh may still be pending or running when the device is
removed, cancel the delayed work_refresh in remove path.

Fixes: 50dd64d57eee ("iio: common: ssp_sensors: Add sensorhub driver")
Signed-off-by: Sanjay Chitroda <sanjayembeddedse@gmail.com>
---
 drivers/iio/common/ssp_sensors/ssp_dev.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/iio/common/ssp_sensors/ssp_dev.c b/drivers/iio/common/ssp_sensors/ssp_dev.c
index 7d07fae295fd..51730dae5871 100644
--- a/drivers/iio/common/ssp_sensors/ssp_dev.c
+++ b/drivers/iio/common/ssp_sensors/ssp_dev.c
@@ -590,6 +590,7 @@ static void ssp_remove(struct spi_device *spi)
 	ssp_clean_pending_list(data);
 
 	free_irq(data->spi->irq, data);
+	cancel_delayed_work_sync(&data->work_refresh);
 
 	timer_delete_sync(&data->wdt_timer);
 	cancel_work_sync(&data->work_wdt);
-- 
2.34.1

[PATCH v7 4/9] iio: ssp_sensors: factor out mcu enable/disable helper(s)

[Sanjay Chitroda]

From: Sanjay Chitroda <sanjayembeddedse@gmail.com>

Re-factor the enable and disable MCU logic into small helper functions
this simplify transfer flow to follow.

No functional change intended.

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

diff --git a/drivers/iio/common/ssp_sensors/ssp_dev.c b/drivers/iio/common/ssp_sensors/ssp_dev.c
index 51730dae5871..9b8642193176 100644
--- a/drivers/iio/common/ssp_sensors/ssp_dev.c
+++ b/drivers/iio/common/ssp_sensors/ssp_dev.c
@@ -89,7 +89,7 @@ static void ssp_sync_available_sensors(struct ssp_data *data)
 			"SSP_MSG2SSP_AP_MCU_SET_DUMPMODE failed\n");
 }
 
-static void ssp_enable_mcu(struct ssp_data *data, bool enable)
+static void ssp_set_mcu(struct ssp_data *data, bool enable)
 {
 	dev_info(&data->spi->dev, "current shutdown = %d, old = %d\n", enable,
 		 data->shut_down);
@@ -108,6 +108,16 @@ static void ssp_enable_mcu(struct ssp_data *data, bool enable)
 	}
 }
 
+static inline void ssp_enable_mcu(struct ssp_data *data)
+{
+	ssp_set_mcu(data, true);
+}
+
+static inline void ssp_disable_mcu(struct ssp_data *data)
+{
+	ssp_set_mcu(data, false);
+}
+
 /*
  * This function is the first one which communicates with the mcu so it is
  * possible that the first attempt will fail
@@ -146,10 +156,10 @@ static int ssp_check_fwbl(struct ssp_data *data)
 
 static void ssp_reset_mcu(struct ssp_data *data)
 {
-	ssp_enable_mcu(data, false);
+	ssp_disable_mcu(data);
 	ssp_clean_pending_list(data);
 	ssp_toggle_mcu_reset_gpio(data);
-	ssp_enable_mcu(data, true);
+	ssp_enable_mcu(data);
 }
 
 static void ssp_wdt_work_func(struct work_struct *work)
@@ -584,7 +594,7 @@ static void ssp_remove(struct spi_device *spi)
 		dev_err(&data->spi->dev,
 			"SSP_MSG2SSP_AP_STATUS_SHUTDOWN failed\n");
 
-	ssp_enable_mcu(data, false);
+	ssp_disable_mcu(data);
 	ssp_disable_wdt_timer(data);
 
 	ssp_clean_pending_list(data);
-- 
2.34.1

[PATCH v7 5/9] iio: ssp_sensors: use local struct device

[Sanjay Chitroda]

From: Sanjay Chitroda <sanjayembeddedse@gmail.com>

Introduce a struct device *dev derived from &spi->dev to improve
readability and reorder variable(s) to follow reversed xmas tree style.

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

diff --git a/drivers/iio/common/ssp_sensors/ssp_dev.c b/drivers/iio/common/ssp_sensors/ssp_dev.c
index 9b8642193176..fa47282b39c0 100644
--- a/drivers/iio/common/ssp_sensors/ssp_dev.c
+++ b/drivers/iio/common/ssp_sensors/ssp_dev.c
@@ -492,27 +492,28 @@ EXPORT_SYMBOL_NS(ssp_register_consumer, "IIO_SSP_SENSORS");
 
 static int ssp_probe(struct spi_device *spi)
 {
-	int ret, i;
+	struct device *dev = &spi->dev;
 	struct ssp_data *data;
+	int ret, i;
 
-	data = ssp_parse_dt(&spi->dev);
+	data = ssp_parse_dt(dev);
 	if (!data) {
-		dev_err(&spi->dev, "Failed to find platform data\n");
+		dev_err(dev, "Failed to find platform data\n");
 		return -ENODEV;
 	}
 
-	ret = mfd_add_devices(&spi->dev, PLATFORM_DEVID_NONE,
+	ret = mfd_add_devices(dev, PLATFORM_DEVID_NONE,
 			      sensorhub_sensor_devs,
 			      ARRAY_SIZE(sensorhub_sensor_devs), NULL, 0, NULL);
 	if (ret < 0) {
-		dev_err(&spi->dev, "mfd add devices fail\n");
+		dev_err(dev, "mfd add devices fail\n");
 		return ret;
 	}
 
 	spi->mode = SPI_MODE_1;
 	ret = spi_setup(spi);
 	if (ret < 0) {
-		dev_err(&spi->dev, "Failed to setup spi\n");
+		dev_err(dev, "Failed to setup spi\n");
 		goto err_setup_spi;
 	}
 
@@ -548,7 +549,7 @@ static int ssp_probe(struct spi_device *spi)
 				   IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
 				   "SSP_Int", data);
 	if (ret < 0) {
-		dev_err(&spi->dev, "Irq request fail\n");
+		dev_err(dev, "Irq request fail\n");
 		goto err_setup_irq;
 	}
 
@@ -562,11 +563,11 @@ static int ssp_probe(struct spi_device *spi)
 	if (data->fw_dl_state == SSP_FW_DL_STATE_NONE) {
 		ret = ssp_initialize_mcu(data);
 		if (ret < 0) {
-			dev_err(&spi->dev, "Initialize_mcu failed\n");
+			dev_err(dev, "Initialize_mcu failed\n");
 			goto err_read_reg;
 		}
 	} else {
-		dev_err(&spi->dev, "Firmware version not supported\n");
+		dev_err(dev, "Firmware version not supported\n");
 		ret = -EPERM;
 		goto err_read_reg;
 	}
@@ -579,9 +580,9 @@ static int ssp_probe(struct spi_device *spi)
 	mutex_destroy(&data->pending_lock);
 	mutex_destroy(&data->comm_lock);
 err_setup_spi:
-	mfd_remove_devices(&spi->dev);
+	mfd_remove_devices(dev);
 
-	dev_err(&spi->dev, "Probe failed!\n");
+	dev_err(dev, "Probe failed!\n");
 
 	return ret;
 }
-- 
2.34.1

[PATCH v7 6/9] iio: ssp_sensors: Drop duplicated wdt timer and work cleanup

[Sanjay Chitroda]

From: Sanjay Chitroda <sanjayembeddedse@gmail.com>

The SSP remove path cleans up the watchdog timer and associated work
both via ssp_disable_wdt_timer() and through explicit timer and work
teardown.

ssp_disable_wdt_timer() already performs a synchronous teardown of the
watchdog timer and watchdog work, guaranteeing that no timer callbacks
or watchdog work can be running or requeued once it returns.

In addition, the remove path disables interrupts and frees IRQ handler
using ssp_disable_mcu() and free_irq(). The refresh work is also
cancelled, preventing wdt_timer being re-armed before teardown. This
ensures that no new refresh or watchdog activity can be triggered from
the IRQ thread and refresh workqueue.

As a result, the additional timer and work cancellation is redundant
and does not provide additional ordering or race protection. Remove the
duplicated cleanup and rely on the centralized watchdog disable helper.

Signed-off-by: Sanjay Chitroda <sanjayembeddedse@gmail.com>
---
Changes in v7:
- Following comment from Andy, Study the timer, work and IRQ
  relationship and how it would work internally for ssp_sensors and
  changes looks correct; updated the commit message to explain the
  race condition and information on resource release during teardown.
- v6 change: https://lore.kernel.org/all/20260415050749.3858046-5-sanjayembedded@gmail.com/
---
 drivers/iio/common/ssp_sensors/ssp_dev.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/iio/common/ssp_sensors/ssp_dev.c b/drivers/iio/common/ssp_sensors/ssp_dev.c
index fa47282b39c0..29b17118ba2a 100644
--- a/drivers/iio/common/ssp_sensors/ssp_dev.c
+++ b/drivers/iio/common/ssp_sensors/ssp_dev.c
@@ -596,15 +596,12 @@ static void ssp_remove(struct spi_device *spi)
 			"SSP_MSG2SSP_AP_STATUS_SHUTDOWN failed\n");
 
 	ssp_disable_mcu(data);
-	ssp_disable_wdt_timer(data);
-
 	ssp_clean_pending_list(data);
 
 	free_irq(data->spi->irq, data);
 	cancel_delayed_work_sync(&data->work_refresh);
 
-	timer_delete_sync(&data->wdt_timer);
-	cancel_work_sync(&data->work_wdt);
+	ssp_disable_wdt_timer(data);
 
 	mutex_destroy(&data->comm_lock);
 	mutex_destroy(&data->pending_lock);
-- 
2.34.1

[PATCH v7 7/9] iio: ssp_sensors: convert probe and teardown to devm-managed resources

[Sanjay Chitroda]

From: Sanjay Chitroda <sanjayembeddedse@gmail.com>

Convert SSP driver resource management to use devm-managed helpers
and devm actions, tying the lifetime of all resources to the device.

Mutex initialization, IRQ registration, work items, timers, and MFD
resource are now managed using devm APIs. Cleanup logic previously
handled explicitly in probe error paths and the remove callback is
replaced with devm_add_action_or_reset(), ensuring correct teardown
ordering and consistent behaviour on probe failure and device unbind.

This simplifies the probe path by removing goto-based error handling,
eliminates the remove callback entirely.

No functional change intended.

Signed-off-by: Sanjay Chitroda <sanjayembeddedse@gmail.com>
---
Changes in v7:
- Following input from Jonathan, added comment to describe the managed action
- Keep it single line for devm_add_..() API definition
- v6 change: https://lore.kernel.org/all/20260415050749.3858046-6-sanjayembedded@gmail.com/
Changes in v6:
- Drop remove path completely using devm_action with review comment from David
- Convert MFD device to manage resource along with mutex and IRQ
- v5 change: https://lore.kernel.org/all/20260406080852.2727453-5-sanjayembedded@gmail.com/
---
 drivers/iio/common/ssp_sensors/ssp_dev.c | 117 +++++++++++++----------
 1 file changed, 65 insertions(+), 52 deletions(-)

diff --git a/drivers/iio/common/ssp_sensors/ssp_dev.c b/drivers/iio/common/ssp_sensors/ssp_dev.c
index 29b17118ba2a..5415efc7bf48 100644
--- a/drivers/iio/common/ssp_sensors/ssp_dev.c
+++ b/drivers/iio/common/ssp_sensors/ssp_dev.c
@@ -204,6 +204,26 @@ static void ssp_disable_wdt_timer(struct ssp_data *data)
 	cancel_work_sync(&data->work_wdt);
 }
 
+static void ssp_disable_work_timer(void *ptr)
+{
+	struct ssp_data *data = ptr;
+
+	cancel_delayed_work_sync(&data->work_refresh);
+	ssp_disable_wdt_timer(data);
+}
+
+static void ssp_shutdown_action(void *ptr)
+{
+	struct ssp_data *data = ptr;
+
+	if (ssp_command(data, SSP_MSG2SSP_AP_STATUS_SHUTDOWN, 0) < 0)
+		dev_err(&data->spi->dev,
+			"SSP_MSG2SSP_AP_STATUS_SHUTDOWN failed\n");
+
+	ssp_disable_mcu(data);
+	ssp_clean_pending_list(data);
+}
+
 /**
  * ssp_get_sensor_delay() - gets sensor data acquisition period
  * @data:	sensorhub structure
@@ -502,9 +522,9 @@ static int ssp_probe(struct spi_device *spi)
 		return -ENODEV;
 	}
 
-	ret = mfd_add_devices(dev, PLATFORM_DEVID_NONE,
-			      sensorhub_sensor_devs,
-			      ARRAY_SIZE(sensorhub_sensor_devs), NULL, 0, NULL);
+	ret = devm_mfd_add_devices(dev, PLATFORM_DEVID_NONE,
+				   sensorhub_sensor_devs,
+				   ARRAY_SIZE(sensorhub_sensor_devs), NULL, 0, NULL);
 	if (ret < 0) {
 		dev_err(dev, "mfd add devices fail\n");
 		return ret;
@@ -514,14 +534,16 @@ static int ssp_probe(struct spi_device *spi)
 	ret = spi_setup(spi);
 	if (ret < 0) {
 		dev_err(dev, "Failed to setup spi\n");
-		goto err_setup_spi;
+		return ret;
 	}
 
 	data->fw_dl_state = SSP_FW_DL_STATE_NONE;
 	data->spi = spi;
 	spi_set_drvdata(spi, data);
 
-	mutex_init(&data->comm_lock);
+	ret = devm_mutex_init(dev, &data->comm_lock);
+	if (ret < 0)
+		return ret;
 
 	for (i = 0; i < SSP_SENSOR_MAX; ++i) {
 		data->delay_buf[i] = SSP_DEFAULT_POLLING_DELAY;
@@ -534,7 +556,9 @@ static int ssp_probe(struct spi_device *spi)
 
 	data->time_syncing = true;
 
-	mutex_init(&data->pending_lock);
+	ret = devm_mutex_init(dev, &data->pending_lock);
+	if (ret < 0)
+		return ret;
 	INIT_LIST_HEAD(&data->pending_list);
 
 	atomic_set(&data->enable_refcount, 0);
@@ -544,14 +568,23 @@ 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);
-	if (ret < 0) {
-		dev_err(dev, "Irq request fail\n");
-		goto err_setup_irq;
-	}
+	/*
+	 * Register deferred callback which disable timer and work after
+	 * module unloaded.
+	 *
+	 * driver should cancel delayed refresh work, watchdog work and delete
+	 * watchdog timer once interrupt is disabled and IRQ is freed.
+	 */
+	ret = devm_add_action_or_reset(dev, ssp_disable_work_timer, data);
+	if (ret < 0)
+		return ret;
+
+	ret = devm_request_threaded_irq(dev, data->spi->irq, NULL,
+					ssp_irq_thread_fn,
+					IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
+					"SSP_Int", data);
+	if (ret < 0)
+		return ret;
 
 	/* Let's start with enabled one so irq balance could be ok */
 	data->shut_down = false;
@@ -564,49 +597,30 @@ static int ssp_probe(struct spi_device *spi)
 		ret = ssp_initialize_mcu(data);
 		if (ret < 0) {
 			dev_err(dev, "Initialize_mcu failed\n");
-			goto err_read_reg;
+			return ret;
 		}
 	} else {
 		dev_err(dev, "Firmware version not supported\n");
-		ret = -EPERM;
-		goto err_read_reg;
+		return -EPERM;
 	}
 
-	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(dev);
-
-	dev_err(dev, "Probe failed!\n");
-
-	return ret;
-}
-
-static void ssp_remove(struct spi_device *spi)
-{
-	struct ssp_data *data = spi_get_drvdata(spi);
-
-	if (ssp_command(data, SSP_MSG2SSP_AP_STATUS_SHUTDOWN, 0) < 0)
-		dev_err(&data->spi->dev,
-			"SSP_MSG2SSP_AP_STATUS_SHUTDOWN failed\n");
-
-	ssp_disable_mcu(data);
-	ssp_clean_pending_list(data);
-
-	free_irq(data->spi->irq, data);
-	cancel_delayed_work_sync(&data->work_refresh);
-
-	ssp_disable_wdt_timer(data);
-
-	mutex_destroy(&data->comm_lock);
-	mutex_destroy(&data->pending_lock);
+	/*
+	 * Managed shutdown action for the SSP device lifecycle.
+	 *
+	 * This action unwinds state by:
+	 *  - notifying the SSP firmware of AP shutdown,
+	 *  - disabling the MCU to prevent further IRQ activity,
+	 *  - cleaning up any pending command state.
+	 *
+	 * The action is registered once the MCU has been initialized so that
+	 * both driver removal and probe failure after this point leave the
+	 * device in a quiescent state.
+	 */
+	ret = devm_add_action_or_reset(dev, ssp_shutdown_action, data);
+	if (ret < 0)
+		return ret;
 
-	mfd_remove_devices(&spi->dev);
+	return 0;
 }
 
 static int ssp_suspend(struct device *dev)
@@ -662,7 +676,6 @@ static DEFINE_SIMPLE_DEV_PM_OPS(ssp_pm_ops, ssp_suspend, ssp_resume);
 
 static struct spi_driver ssp_driver = {
 	.probe = ssp_probe,
-	.remove = ssp_remove,
 	.driver = {
 		.pm = pm_sleep_ptr(&ssp_pm_ops),
 		.of_match_table = ssp_of_match,
-- 
2.34.1

[PATCH v7 8/9] iio: ssp_sensors: Use dev_err_probe

[Sanjay Chitroda]

From: Sanjay Chitroda <sanjayembeddedse@gmail.com>

dev_err_probe() makes error code handling simpler and handle
deferred probe nicely (avoid spamming logs).

Signed-off-by: Sanjay Chitroda <sanjayembeddedse@gmail.com>
---
changes in v7:
- Drop redundant else with refactor as guided by Andy
- Add new change in series for local dev variable with input from Andy
- v6 change: https://lore.kernel.org/all/20260415050749.3858046-7-sanjayembedded@gmail.com/
---
 drivers/iio/common/ssp_sensors/ssp_dev.c | 38 +++++++++---------------
 1 file changed, 14 insertions(+), 24 deletions(-)

diff --git a/drivers/iio/common/ssp_sensors/ssp_dev.c b/drivers/iio/common/ssp_sensors/ssp_dev.c
index 5415efc7bf48..22d26980baa2 100644
--- a/drivers/iio/common/ssp_sensors/ssp_dev.c
+++ b/drivers/iio/common/ssp_sensors/ssp_dev.c
@@ -517,25 +517,19 @@ static int ssp_probe(struct spi_device *spi)
 	int ret, i;
 
 	data = ssp_parse_dt(dev);
-	if (!data) {
-		dev_err(dev, "Failed to find platform data\n");
-		return -ENODEV;
-	}
+	if (!data)
+		return dev_err_probe(dev, -ENODEV, "Failed to find platform data\n");
 
-	ret = devm_mfd_add_devices(dev, PLATFORM_DEVID_NONE,
-				   sensorhub_sensor_devs,
+	ret = devm_mfd_add_devices(dev, PLATFORM_DEVID_NONE, sensorhub_sensor_devs,
 				   ARRAY_SIZE(sensorhub_sensor_devs), NULL, 0, NULL);
-	if (ret < 0) {
-		dev_err(dev, "mfd add devices fail\n");
-		return ret;
-	}
+
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "mfd add devices fail\n");
 
 	spi->mode = SPI_MODE_1;
 	ret = spi_setup(spi);
-	if (ret < 0) {
-		dev_err(dev, "Failed to setup spi\n");
-		return ret;
-	}
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "Failed to setup spi\n");
 
 	data->fw_dl_state = SSP_FW_DL_STATE_NONE;
 	data->spi = spi;
@@ -593,16 +587,12 @@ static int ssp_probe(struct spi_device *spi)
 	enable_irq_wake(data->spi->irq);
 
 	data->fw_dl_state = ssp_check_fwbl(data);
-	if (data->fw_dl_state == SSP_FW_DL_STATE_NONE) {
-		ret = ssp_initialize_mcu(data);
-		if (ret < 0) {
-			dev_err(dev, "Initialize_mcu failed\n");
-			return ret;
-		}
-	} else {
-		dev_err(dev, "Firmware version not supported\n");
-		return -EPERM;
-	}
+	if (data->fw_dl_state != SSP_FW_DL_STATE_NONE)
+		return dev_err_probe(dev, -EPERM, "Firmware version not supported\n");
+
+	ret = ssp_initialize_mcu(data);
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "Initialize_mcu failed\n");
 
 	/*
 	 * Managed shutdown action for the SSP device lifecycle.
-- 
2.34.1

[PATCH v7 9/9] iio: ssp_sensors: reuse embedded 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.

Replace the dynamically allocated RX buffer with a fixed-size,
preallocated buffer embedded in the driver structure and reused for all
SPI receive operations. This removes memory allocation from the
IRQ path, simplifies lifetime management.

Signed-off-by: Sanjay Chitroda <sanjayembeddedse@gmail.com>
---
Changes in v6:
- Replace dynamically allocated RX buffer with embedded fixed-size buffer
- Fix struct layout to satisfy DMA alignment constraints comment from David
- v5 change: https://lore.kernel.org/all/20260406080852.2727453-6-sanjayembedded@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.
- v3 change: 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
- v2 change: 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
- v1 change: https://lore.kernel.org/all/20260310200513.2162018-3-sanjayembedded@gmail.com/
---
 drivers/iio/common/ssp_sensors/ssp.h     |  3 +++
 drivers/iio/common/ssp_sensors/ssp_spi.c | 20 ++------------------
 2 files changed, 5 insertions(+), 18 deletions(-)

diff --git a/drivers/iio/common/ssp_sensors/ssp.h b/drivers/iio/common/ssp_sensors/ssp.h
index f649cdecc277..8295bb7062a3 100644
--- a/drivers/iio/common/ssp_sensors/ssp.h
+++ b/drivers/iio/common/ssp_sensors/ssp.h
@@ -174,6 +174,7 @@ struct ssp_sensorhub_info {
  * @pending_list:	pending list for messages queued to be sent/read
  * @sensor_devs:	registered IIO devices table
  * @enable_refcount:	enable reference count for wdt (watchdog timer)
+ * @rx_buf:		buffer to receive SPI data
  * @header_buffer:	cache aligned buffer for packet header
  */
 struct ssp_data {
@@ -221,6 +222,8 @@ struct ssp_data {
 	struct iio_dev *sensor_devs[SSP_SENSOR_MAX];
 	atomic_t enable_refcount;
 
+	u8 rx_buf[SSP_DATA_PACKET_SIZE];
+
 	__le16 header_buffer[SSP_HEADER_BUFFER_SIZE / sizeof(__le16)] __aligned(IIO_DMA_MINALIGN);
 };
 
diff --git a/drivers/iio/common/ssp_sensors/ssp_spi.c b/drivers/iio/common/ssp_sensors/ssp_spi.c
index 870214551f0b..e41da88bf96d 100644
--- a/drivers/iio/common/ssp_sensors/ssp_spi.c
+++ b/drivers/iio/common/ssp_sensors/ssp_spi.c
@@ -339,7 +339,7 @@ static int ssp_parse_dataframe(struct ssp_data *data, char *dataframe, int len)
 /* threaded irq */
 int ssp_irq_msg(struct ssp_data *data)
 {
-	char *buffer;
+	char *buffer = data->rx_buf;
 	u8 msg_type;
 	int ret;
 	u16 length, msg_options;
@@ -383,19 +383,12 @@ 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;
-			}
 
 			/* 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 +421,13 @@ 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;
-
 		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 v7 1/9] iio: ssp_sensors: cleanup codestyle warning

[Jonathan Cameron]

On Sun, 26 Apr 2026 14:47:02 +0530
Sanjay Chitroda <sanjayembeddedse@gmail.com> wrote:

> From: Sanjay Chitroda <sanjayembeddedse@gmail.com>
> 
> Reported by checkpatch:
> FILE: drivers/iio/common/ssp_sensors/ssp_dev.c
> 
> WARNING: Prefer __packed over __attribute__((__packed__))
> +} __attribute__((__packed__));
> 
> Signed-off-by: Sanjay Chitroda <sanjayembeddedse@gmail.com>
Applied to the testing branch of iio.git.

I'm trying to pick up some of the patches on list just
to reduce how many are getting sent in new revisions and
the time it takes people to look at them each time.

Not sure how far I'll get in your series so look out for
when I stop saying I've applied them.

thanks,

Jonathan

> ---
>  drivers/iio/common/ssp_sensors/ssp_dev.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/common/ssp_sensors/ssp_dev.c b/drivers/iio/common/ssp_sensors/ssp_dev.c
> index da09c9f3ceb6..7d07fae295fd 100644
> --- a/drivers/iio/common/ssp_sensors/ssp_dev.c
> +++ b/drivers/iio/common/ssp_sensors/ssp_dev.c
> @@ -28,7 +28,7 @@ struct ssp_instruction {
>  	__le32 a;
>  	__le32 b;
>  	u8 c;
> -} __attribute__((__packed__));
> +} __packed__;
>  
>  static const u8 ssp_magnitude_table[] = {110, 85, 171, 71, 203, 195, 0, 67,
>  	208, 56, 175, 244, 206, 213, 0, 92, 250, 0, 55, 48, 189, 252, 171,

Re: [PATCH v7 2/9] iio: ssp_sensors: factor out pending list add/remove helper(s)

[Jonathan Cameron]

On Sun, 26 Apr 2026 14:47:03 +0530
Sanjay Chitroda <sanjayembeddedse@gmail.com> wrote:

> 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 and drop use_no_irq variable to avoid duplication and
> simplify transfer flow to follow.
> 
> No functional change intended.
> 
> Signed-off-by: Sanjay Chitroda <sanjayembeddedse@gmail.com>
> Suggested-by: Andy Shevchenko <andriy.shevchenko@intel.com>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>
> ---
> Changes in v7:
> - Following suggestion from Andy, keep helper API definition in single
>   line and re-place the comment section
> - v6 change: https://lore.kernel.org/all/20260415050749.3858046-4-sanjayembedded@gmail.com/
> Changes in v6:
> - Include tag for the suggestion of helper functions
> - Drop completely use_no_irq variable with review comment from Andy
> - v5 change: https://lore.kernel.org/all/20260406080852.2727453-4-sanjayembedded@gmail.com/
> ---
>  drivers/iio/common/ssp_sensors/ssp_spi.c | 58 ++++++++++++++----------
>  1 file changed, 33 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/iio/common/ssp_sensors/ssp_spi.c b/drivers/iio/common/ssp_sensors/ssp_spi.c
> index 08ed92859be0..870214551f0b 100644
> --- a/drivers/iio/common/ssp_sensors/ssp_spi.c
> +++ b/drivers/iio/common/ssp_sensors/ssp_spi.c
> @@ -174,15 +174,35 @@ 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)
> +{
> +	/*
> +	 * Check if this is a short one way message or the whole transfer has
> +	 * second part after an interrupt.
> +	 */
> +	if (msg->length == 0)
> +		return;

I know Andy suggested your bring these into the helpers, but to me
it's obscuring flow as it looks at the caller like it was added
to the pending list when it wasn't..  And we end up with multiple
checks on msg_length where we had one before.

One option would be to have it return a bool to indicate whether
it was added to the pending list or not.

Andy, would that work for you?

> +
> +	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)
> +{
> +	/* See ssp_pending_add() for transfer length logic */
> +	if (msg->length == 0)
Not useful to know if this happened at caller, so no need to return
bool from this.
> +		return;
> +
> +	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)
>  {
>  	int status;
> -	/*
> -	 * check if this is a short one way message or the whole transfer has
> -	 * second part after an interrupt
> -	 */
> -	const bool use_no_irq = msg->length == 0;
>  
>  	if (data->shut_down)
>  		return -EPERM;
> @@ -202,35 +222,23 @@ 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);
> -	}
> +	ssp_pending_add(data, msg);

With suggestion above this would become

	use_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);
> -		}
> +		ssp_pending_del(data, msg);
>  		goto _error_locked;
>  	}
>  
>  	mutex_unlock(&data->comm_lock);
>  
> -	if (!use_no_irq && done)
> -		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);
> +	if (msg->length && done &&
then
	if (use_irq && done &&
	    !wait_for_completion_timeout()
> +	    !wait_for_completion_timeout(done, msecs_to_jiffies(timeout))) {
> +		ssp_pending_del(data, msg);
>  
> -			data->timeout_cnt++;
> -			return -ETIMEDOUT;
> -		}
> +		data->timeout_cnt++;
> +		return -ETIMEDOUT;
> +	}
>  
>  	return 0;
>  
The mix of using a goto error handling block and not in here is not elegant but
it's would take quite a bit of reorganizing to tidy that up.  One option would be to
factor out this bit
	mutex_lock(&data->comm_lock);

	status = ssp_check_lines(data, false);
	if (status < 0)
		goto _error_locked;

	status = spi_write(data->spi, msg->buffer, SSP_HEADER_SIZE);
	if (status < 0) {
		gpiod_set_value_cansleep(data->ap_mcu_gpiod, 1);
		dev_err(SSP_DEV, "%s spi_write fail\n", __func__);
		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);
	}

	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);
		}
		goto _error_locked;
	}

	mutex_unlock(&data->comm_lock);

into a helper, use guard() for the outer mutex and then direct returns.

Then we only have a simple check on return value from that to decide
to increment the counter and exit on error.

Re: [PATCH v7 3/9] iio: ssp_sensors: cancel delayed work_refresh on remove

[Jonathan Cameron]

On Sun, 26 Apr 2026 14:47:04 +0530
Sanjay Chitroda <sanjayembeddedse@gmail.com> wrote:

> From: Sanjay Chitroda <sanjayembeddedse@gmail.com>
> 
> The work_refresh may still be pending or running when the device is
> removed, cancel the delayed work_refresh in remove path.
> 
> Fixes: 50dd64d57eee ("iio: common: ssp_sensors: Add sensorhub driver")

Fixes at start of patch series not after some refactors of unrelated
code.

> Signed-off-by: Sanjay Chitroda <sanjayembeddedse@gmail.com>
> ---
>  drivers/iio/common/ssp_sensors/ssp_dev.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/iio/common/ssp_sensors/ssp_dev.c b/drivers/iio/common/ssp_sensors/ssp_dev.c
> index 7d07fae295fd..51730dae5871 100644
> --- a/drivers/iio/common/ssp_sensors/ssp_dev.c
> +++ b/drivers/iio/common/ssp_sensors/ssp_dev.c
> @@ -590,6 +590,7 @@ static void ssp_remove(struct spi_device *spi)
>  	ssp_clean_pending_list(data);
>  
>  	free_irq(data->spi->irq, data);
> +	cancel_delayed_work_sync(&data->work_refresh);
>  
>  	timer_delete_sync(&data->wdt_timer);
>  	cancel_work_sync(&data->work_wdt);

Re: [PATCH v7 4/9] iio: ssp_sensors: factor out mcu enable/disable helper(s)

[Jonathan Cameron]

On Sun, 26 Apr 2026 14:47:05 +0530
Sanjay Chitroda <sanjayembeddedse@gmail.com> wrote:

> From: Sanjay Chitroda <sanjayembeddedse@gmail.com>
> 
> Re-factor the enable and disable MCU logic into small helper functions
> this simplify transfer flow to follow.
> 
> No functional change intended.
> 
> Signed-off-by: Sanjay Chitroda <sanjayembeddedse@gmail.com>
> ---
>  drivers/iio/common/ssp_sensors/ssp_dev.c | 18 ++++++++++++++----
>  1 file changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/common/ssp_sensors/ssp_dev.c b/drivers/iio/common/ssp_sensors/ssp_dev.c
> index 51730dae5871..9b8642193176 100644
> --- a/drivers/iio/common/ssp_sensors/ssp_dev.c
> +++ b/drivers/iio/common/ssp_sensors/ssp_dev.c
> @@ -89,7 +89,7 @@ static void ssp_sync_available_sensors(struct ssp_data *data)
>  			"SSP_MSG2SSP_AP_MCU_SET_DUMPMODE failed\n");
>  }
>  
> -static void ssp_enable_mcu(struct ssp_data *data, bool enable)
> +static void ssp_set_mcu(struct ssp_data *data, bool enable)
>  {
>  	dev_info(&data->spi->dev, "current shutdown = %d, old = %d\n", enable,
>  		 data->shut_down);
> @@ -108,6 +108,16 @@ static void ssp_enable_mcu(struct ssp_data *data, bool enable)
>  	}
>  }
>  
> +static inline void ssp_enable_mcu(struct ssp_data *data)
> +{
> +	ssp_set_mcu(data, true);
> +}
> +
> +static inline void ssp_disable_mcu(struct ssp_data *data)
> +{
> +	ssp_set_mcu(data, false);
> +}
There is no significant shared code in ssp_set_mcu() for true vs false. Also never add
inlines in c files without very strong perf data to back them up.
So something like:

static void ssp_enable_mcu(struct ssp_data *data)
{
	dev_info(&data->spi->dev, "current shutdown = %d, old = %d\n", enable,
		 data->shut_down);

	if (!data->shutdown) {
		dev_warn(&data->spi->dev, "current shutdown = %d, old = %d\n",
			 enable, data->shut_down);

// I think this is debug only. So I'd drop the print as something that shouldn't
// be in code after development is done.

		return;
	}
	data->shut_down = false;
	enable_irq(data->spi->irq);
	enable_irq_wake(data->spi->irq);
}

and similar for disable_mcu()

FWIW I don't like having a dev_info() print there. So demote that to dev_dbg().

> +
>  /*
>   * This function is the first one which communicates with the mcu so it is
>   * possible that the first attempt will fail
> @@ -146,10 +156,10 @@ static int ssp_check_fwbl(struct ssp_data *data)
>  
>  static void ssp_reset_mcu(struct ssp_data *data)
>  {
> -	ssp_enable_mcu(data, false);
> +	ssp_disable_mcu(data);
>  	ssp_clean_pending_list(data);
>  	ssp_toggle_mcu_reset_gpio(data);
> -	ssp_enable_mcu(data, true);
> +	ssp_enable_mcu(data);
>  }
>  
>  static void ssp_wdt_work_func(struct work_struct *work)
> @@ -584,7 +594,7 @@ static void ssp_remove(struct spi_device *spi)
>  		dev_err(&data->spi->dev,
>  			"SSP_MSG2SSP_AP_STATUS_SHUTDOWN failed\n");
>  
> -	ssp_enable_mcu(data, false);
> +	ssp_disable_mcu(data);
>  	ssp_disable_wdt_timer(data);
>  
>  	ssp_clean_pending_list(data);

Re: [PATCH v7 5/9] iio: ssp_sensors: use local struct device

[Jonathan Cameron]

On Sun, 26 Apr 2026 14:47:06 +0530
Sanjay Chitroda <sanjayembeddedse@gmail.com> wrote:

> From: Sanjay Chitroda <sanjayembeddedse@gmail.com>
> 
> Introduce a struct device *dev derived from &spi->dev to improve
> readability and reorder variable(s) to follow reversed xmas tree style.
It's really minor reordering but generally I'd not do that in a patch
that does anything else.  So here I'd have put the new variables
in as close to correct location as possible, then followed with
a reorder if it was worth the churn that results. I'm not sure it is
in this particular case.

> 
> Signed-off-by: Sanjay Chitroda <sanjayembeddedse@gmail.com>

> @@ -579,9 +580,9 @@ static int ssp_probe(struct spi_device *spi)
>  	mutex_destroy(&data->pending_lock);
>  	mutex_destroy(&data->comm_lock);
>  err_setup_spi:
> -	mfd_remove_devices(&spi->dev);
> +	mfd_remove_devices(dev);
>  
> -	dev_err(&spi->dev, "Probe failed!\n");
> +	dev_err(dev, "Probe failed!\n");

Drop this one.  It's easy to find out if a driver probe succeeded
so we don't need a print for it given meaningful errors generated
messages anyway.

>  
>  	return ret;
>  }

Re: [PATCH v7 8/9] iio: ssp_sensors: Use dev_err_probe

[Jonathan Cameron]

On Sun, 26 Apr 2026 14:47:09 +0530
Sanjay Chitroda <sanjayembeddedse@gmail.com> wrote:

> From: Sanjay Chitroda <sanjayembeddedse@gmail.com>
> 
> dev_err_probe() makes error code handling simpler and handle
> deferred probe nicely (avoid spamming logs).
> 
> Signed-off-by: Sanjay Chitroda <sanjayembeddedse@gmail.com>
> ---
> changes in v7:
> - Drop redundant else with refactor as guided by Andy
> - Add new change in series for local dev variable with input from Andy
> - v6 change: https://lore.kernel.org/all/20260415050749.3858046-7-sanjayembedded@gmail.com/
> ---
>  drivers/iio/common/ssp_sensors/ssp_dev.c | 38 +++++++++---------------
>  1 file changed, 14 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/iio/common/ssp_sensors/ssp_dev.c b/drivers/iio/common/ssp_sensors/ssp_dev.c
> index 5415efc7bf48..22d26980baa2 100644
> --- a/drivers/iio/common/ssp_sensors/ssp_dev.c
> +++ b/drivers/iio/common/ssp_sensors/ssp_dev.c
> @@ -517,25 +517,19 @@ static int ssp_probe(struct spi_device *spi)
>  	int ret, i;
>  
>  	data = ssp_parse_dt(dev);
> -	if (!data) {
> -		dev_err(dev, "Failed to find platform data\n");
> -		return -ENODEV;
> -	}
> +	if (!data)
> +		return dev_err_probe(dev, -ENODEV, "Failed to find platform data\n");
>  
> -	ret = devm_mfd_add_devices(dev, PLATFORM_DEVID_NONE,
> -				   sensorhub_sensor_devs,
> +	ret = devm_mfd_add_devices(dev, PLATFORM_DEVID_NONE, sensorhub_sensor_devs,

Unrelated change. Should not be in this patch.

>  				   ARRAY_SIZE(sensorhub_sensor_devs), NULL, 0, NULL);
> -	if (ret < 0) {
> -		dev_err(dev, "mfd add devices fail\n");
> -		return ret;
> -	}
> +

Normal to not have a blank line between call and error check, so why one here?

> +	if (ret < 0)
> +		return dev_err_probe(dev, ret, "mfd add devices fail\n");
>  

Re: [PATCH v7 9/9] iio: ssp_sensors: reuse embedded RX buffer for SPI transfers

[Jonathan Cameron]

On Sun, 26 Apr 2026 14:47:10 +0530
Sanjay Chitroda <sanjayembeddedse@gmail.com> 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.
> 
> Replace the dynamically allocated RX buffer with a fixed-size,
> preallocated buffer embedded in the driver structure and reused for all
> SPI receive operations. This removes memory allocation from the
> IRQ path, simplifies lifetime management.
> 
> Signed-off-by: Sanjay Chitroda <sanjayembeddedse@gmail.com>
> ---
> Changes in v6:
> - Replace dynamically allocated RX buffer with embedded fixed-size buffer
> - Fix struct layout to satisfy DMA alignment constraints comment from David
> - v5 change: https://lore.kernel.org/all/20260406080852.2727453-6-sanjayembedded@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.
> - v3 change: 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
> - v2 change: 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
> - v1 change: https://lore.kernel.org/all/20260310200513.2162018-3-sanjayembedded@gmail.com/
> ---
>  drivers/iio/common/ssp_sensors/ssp.h     |  3 +++
>  drivers/iio/common/ssp_sensors/ssp_spi.c | 20 ++------------------
>  2 files changed, 5 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/iio/common/ssp_sensors/ssp.h b/drivers/iio/common/ssp_sensors/ssp.h
> index f649cdecc277..8295bb7062a3 100644
> --- a/drivers/iio/common/ssp_sensors/ssp.h
> +++ b/drivers/iio/common/ssp_sensors/ssp.h
> @@ -174,6 +174,7 @@ struct ssp_sensorhub_info {
>   * @pending_list:	pending list for messages queued to be sent/read
>   * @sensor_devs:	registered IIO devices table
>   * @enable_refcount:	enable reference count for wdt (watchdog timer)
> + * @rx_buf:		buffer to receive SPI data
>   * @header_buffer:	cache aligned buffer for packet header
>   */
>  struct ssp_data {
> @@ -221,6 +222,8 @@ struct ssp_data {
>  	struct iio_dev *sensor_devs[SSP_SENSOR_MAX];
>  	atomic_t enable_refcount;
>  
> +	u8 rx_buf[SSP_DATA_PACKET_SIZE];

Whilst the define exists for this size, do we have anything to confirm it is right?
It wasn't previously used for anything so is a bit high risk.

Also, why doesn't it need to be DMA safe?  I'd expect it either after the following buffer
or with a __aligned(IIO_DMA_MINALIGN) marking in which case the one below isn't used.


> +
>  	__le16 header_buffer[SSP_HEADER_BUFFER_SIZE / sizeof(__le16)] __aligned(IIO_DMA_MINALIGN);
>  };
>  
> diff --git a/drivers/iio/common/ssp_sensors/ssp_spi.c b/drivers/iio/common/ssp_sensors/ssp_spi.c
> index 870214551f0b..e41da88bf96d 100644
> --- a/drivers/iio/common/ssp_sensors/ssp_spi.c
> +++ b/drivers/iio/common/ssp_sensors/ssp_spi.c
> @@ -339,7 +339,7 @@ static int ssp_parse_dataframe(struct ssp_data *data, char *dataframe, int len)
>  /* threaded irq */
>  int ssp_irq_msg(struct ssp_data *data)
>  {
> -	char *buffer;
> +	char *buffer = data->rx_buf;

Put this in inline. The local variable may reduce code churn but
it also hides the important info that this is not locally allocated.

>  	u8 msg_type;
>  	int ret;
>  	u16 length, msg_options;
> @@ -383,19 +383,12 @@ 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;
> -			}
>  
>  			/* 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 +421,13 @@ 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;
> -
>  		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);

This now makes this path different from the other switch legs.
I would precede this patch with one doing direct returns from
all the legs in here.  Then this just becomes an obvious update
in this patch.

>  	default:
>  		dev_err(SSP_DEV, "unknown msg type\n");
>  		return -EPROTO;

Re: [PATCH v7 9/9] iio: ssp_sensors: reuse embedded RX buffer for SPI transfers

[Jonathan Cameron]

On Sun, 26 Apr 2026 14:47:10 +0530
Sanjay Chitroda <sanjayembeddedse@gmail.com> 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.
> 
> Replace the dynamically allocated RX buffer with a fixed-size,
> preallocated buffer embedded in the driver structure and reused for all
> SPI receive operations. This removes memory allocation from the
> IRQ path, simplifies lifetime management.
> 
> Signed-off-by: Sanjay Chitroda <sanjayembeddedse@gmail.com>

> diff --git a/drivers/iio/common/ssp_sensors/ssp_spi.c b/drivers/iio/common/ssp_sensors/ssp_spi.c
> index 870214551f0b..e41da88bf96d 100644
> --- a/drivers/iio/common/ssp_sensors/ssp_spi.c
> +++ b/drivers/iio/common/ssp_sensors/ssp_spi.c
> @@ -339,7 +339,7 @@ static int ssp_parse_dataframe(struct ssp_data *data, char *dataframe, int len)
>  /* threaded irq */
>  int ssp_irq_msg(struct ssp_data *data)
>  {
> -	char *buffer;
> +	char *buffer = data->rx_buf;
>  	u8 msg_type;
>  	int ret;
>  	u16 length, msg_options;
> @@ -383,19 +383,12 @@ 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);
As bellow.

> -			if (!buffer) {
> -				ret = -ENOMEM;
> -				goto _unlock;
> -			}
>  
>  			/* 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 +421,13 @@ 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);
Is there any chance that GFP_DMA marking is needed?

> -		if (!buffer)
> -			return -ENOMEM;