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

[Jonathan Cameron <jic23@kernel.org>]

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.