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

[Sanjay Chitroda]

On 26 April 2026 7:38:30 pm IST, Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
>On Sun, 26 Apr 2026 14:47:03 +0530
>Sanjay Chitroda <sanjayembeddedse@xxxxxxxxx> wrote:
>
>> From: Sanjay Chitroda <sanjayembeddedse@xxxxxxxxx>
>> 
>> 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@xxxxxxxxx>
>> Suggested-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxx>
>> Reviewed-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxx>
>> ---
>> 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@xxxxxxxxx/
>> 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@xxxxxxxxx/
>> ---
>>  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
>
Thank you for the input.
Agree will include in next series.

>	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.
>
Hi Jonathan,

Thank you for review and point,

If we convert this change to sub-function then suggested use_irq would be shifted to sub-function; then how would you suggest to handle that bool ?
- keep msg->length as it is
- in sub function __ssp_do_transfer(data, msg), should we pass additional parameter to bool for further execution?

Or any better alternative to handle both ?

Thanks,
Sanjay Chitroda

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