Re: [PATCH 1/5] spi: add spi_optimize_message() APIs

[David Lechner]

On Tue, Feb 13, 2024 at 11:25 AM Jonathan Cameron
<Jonathan.Cameron@xxxxxxxxxx> wrote:
>
>
> I thought about suggesting splitting this into an initial patch that just does
> the bits without the controller callbacks. Maybe it would work better that way
> with that introduced after the validate and splitting of transfers (so most
> of patches 1 and 2) as a patch 3 prior to the stm32 additions?

Unless anyone else feels the same way, I'm inclined to avoid the extra
work of splitting it up.


> > +static void __spi_unoptimize_message(struct spi_message *msg)
> > +{
> > +     struct spi_controller *ctlr = msg->spi->controller;
> > +
> > +     if (ctlr->unoptimize_message)
> > +             ctlr->unoptimize_message(msg);
> > +
> > +     msg->optimized = false;
> > +     msg->opt_state = NULL;
> > +}
>
> Seems misbalanced that this doesn't take a pre_optimized flag in but
> __spi_optimize does. I'd move handling that to outside the call in both cases.
>
>

Agreed.


> > @@ -4331,10 +4463,15 @@ static int __spi_sync(struct spi_device *spi, struct spi_message *message)
> >               return -ESHUTDOWN;
> >       }
> >
> > -     status = __spi_validate(spi, message);
> > -     if (status != 0)
> > +     status = spi_maybe_optimize_message(spi, message);
> > +     if (status)
> >               return status;
> >
> > +     /*
> > +      * NB: all return paths after this point must ensure that
> > +      * spi_finalize_current_message() is called to avoid leaking resources.
>
> I'm not sure a catch all like that makes sense. Not sufficient to call
> the finer grained spi_maybe_unoptimize_message()  ?

Hmm... this is my bias from a previous fix showing through. Maybe this
comment doesn't belong in this patch. The short answer to your
question is "it's complicated".