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

Jonathan Cameron
2026-05-05T16:39:09Z

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

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