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

Jonathan Cameron
2026-04-26T14:14:02Z

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);