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

Sanjay Chitroda
2026-05-03T12:04:53Z

On 26 April 2026 7:38:30 pm IST, Jonathan Cameron <jic23@kernel.org> wrote:
>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
>
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.
>