[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAMknhBHc5LnaOOYoeCvtDvUcSXzPnHUtkheYMt73Uv5512dJVg@mail.gmail.com>
Date: Tue, 13 Feb 2024 13:20:31 -0600
From: David Lechner <dlechner@...libre.com>
To: Jonathan Cameron <Jonathan.Cameron@...wei.com>
Cc: Mark Brown <broonie@...nel.org>, Martin Sperl <kernel@...tin.sperl.org>,
David Jander <david@...tonic.nl>, Jonathan Cameron <jic23@...nel.org>,
Michael Hennerich <michael.hennerich@...log.com>, Nuno Sá <nuno.sa@...log.com>,
Alain Volmat <alain.volmat@...s.st.com>, Maxime Coquelin <mcoquelin.stm32@...il.com>,
Alexandre Torgue <alexandre.torgue@...s.st.com>, linux-spi@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-stm32@...md-mailman.stormreply.com,
linux-arm-kernel@...ts.infradead.org, linux-iio@...r.kernel.org
Subject: Re: [PATCH 1/5] spi: add spi_optimize_message() APIs
On Tue, Feb 13, 2024 at 11:25 AM Jonathan Cameron
<Jonathan.Cameron@...wei.com> 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".
Powered by blists - more mailing lists