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

[Jonathan Cameron]

On Sun, 03 May 2026 16:53:27 +0530
Sanjay Chitroda <sanjayembeddedse@xxxxxxxxx> wrote:

> 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?
Pass in any additional parameters you need. If it becomes too long then 
this was a bad idea and you should ignore me ;)

J
> 
> 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.
> >