diff mbox series

[5/5] iio: adc: ad7380: use spi_optimize_message()

Message ID 20240212-mainline-spi-precook-message-v1-5-a2373cd72d36@baylibre.com (mailing list archive)
State Superseded
Headers show
Series spi: add support for pre-cooking messages | expand

Commit Message

David Lechner Feb. 12, 2024, 11:26 p.m. UTC
This modifies the ad7380 ADC driver to use spi_optimize_message() to
optimize the SPI message for the buffered read operation. Since buffered
reads reuse the same SPI message for each read, this can improve
performance by reducing the overhead of setting up some parts the SPI
message in each spi_sync() call.

Signed-off-by: David Lechner <dlechner@baylibre.com>
---
 drivers/iio/adc/ad7380.c | 52 +++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 45 insertions(+), 7 deletions(-)

Comments

Nuno Sá Feb. 13, 2024, 9:51 a.m. UTC | #1
On Mon, 2024-02-12 at 17:26 -0600, David Lechner wrote:
> This modifies the ad7380 ADC driver to use spi_optimize_message() to
> optimize the SPI message for the buffered read operation. Since buffered
> reads reuse the same SPI message for each read, this can improve
> performance by reducing the overhead of setting up some parts the SPI
> message in each spi_sync() call.
> 
> Signed-off-by: David Lechner <dlechner@baylibre.com>
> ---
>  drivers/iio/adc/ad7380.c | 52 +++++++++++++++++++++++++++++++++++++++++------
> -
>  1 file changed, 45 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7380.c b/drivers/iio/adc/ad7380.c
> index abd746aef868..5c5d2642a474 100644
> --- a/drivers/iio/adc/ad7380.c
> +++ b/drivers/iio/adc/ad7380.c
> @@ -133,6 +133,7 @@ struct ad7380_state {
>  	struct spi_device *spi;
>  	struct regulator *vref;
>  	struct regmap *regmap;
> +	struct spi_message *msg;
>  	/*
>  	 * DMA (thus cache coherency maintenance) requires the
>  	 * transfer buffers to live in their own cache lines.
> @@ -231,19 +232,55 @@ static int ad7380_debugfs_reg_access(struct iio_dev
> *indio_dev, u32 reg,
>  	return ret;
>  }
>  
> +static int ad7380_buffer_preenable(struct iio_dev *indio_dev)
> +{
> +	struct ad7380_state *st = iio_priv(indio_dev);
> +	struct spi_transfer *xfer;
> +	int ret;
> +
> +	st->msg = spi_message_alloc(1, GFP_KERNEL);
> +	if (!st->msg)
> +		return -ENOMEM;
> +
> +	xfer = list_first_entry(&st->msg->transfers, struct spi_transfer,
> +				transfer_list);
> +
> +	xfer->bits_per_word = st->chip_info->channels[0].scan_type.realbits;
> +	xfer->len = 4;
> +	xfer->rx_buf = st->scan_data.raw;
> +
> +	ret = spi_optimize_message(st->spi, st->msg);
> +	if (ret) {
> +		spi_message_free(st->msg);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int ad7380_buffer_postdisable(struct iio_dev *indio_dev)
> +{
> +	struct ad7380_state *st = iio_priv(indio_dev);
> +
> +	spi_unoptimize_message(st->msg);
> +	spi_message_free(st->msg);
> +
> +	return 0;
> +}
> +

Not such a big deal but unless I'm missing something we could have the
spi_message (+ the transfer) statically allocated in struct ad7380_state and do
the optimize only once at probe (naturally with proper devm action for
unoptimize). Then we would not need to this for every buffer enable + disable. I
know in terms of performance it won't matter but it would be less code I guess.

Am I missing something?

- Nuno Sá
David Lechner Feb. 13, 2024, 3:27 p.m. UTC | #2
On Tue, Feb 13, 2024 at 3:47 AM Nuno Sá <noname.nuno@gmail.com> wrote:
>
> On Mon, 2024-02-12 at 17:26 -0600, David Lechner wrote:
> > This modifies the ad7380 ADC driver to use spi_optimize_message() to
> > optimize the SPI message for the buffered read operation. Since buffered
> > reads reuse the same SPI message for each read, this can improve
> > performance by reducing the overhead of setting up some parts the SPI
> > message in each spi_sync() call.
> >
> > Signed-off-by: David Lechner <dlechner@baylibre.com>
> > ---
> >  drivers/iio/adc/ad7380.c | 52 +++++++++++++++++++++++++++++++++++++++++------
> > -
> >  1 file changed, 45 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/iio/adc/ad7380.c b/drivers/iio/adc/ad7380.c
> > index abd746aef868..5c5d2642a474 100644
> > --- a/drivers/iio/adc/ad7380.c
> > +++ b/drivers/iio/adc/ad7380.c
> > @@ -133,6 +133,7 @@ struct ad7380_state {
> >       struct spi_device *spi;
> >       struct regulator *vref;
> >       struct regmap *regmap;
> > +     struct spi_message *msg;
> >       /*
> >        * DMA (thus cache coherency maintenance) requires the
> >        * transfer buffers to live in their own cache lines.
> > @@ -231,19 +232,55 @@ static int ad7380_debugfs_reg_access(struct iio_dev
> > *indio_dev, u32 reg,
> >       return ret;
> >  }
> >
> > +static int ad7380_buffer_preenable(struct iio_dev *indio_dev)
> > +{
> > +     struct ad7380_state *st = iio_priv(indio_dev);
> > +     struct spi_transfer *xfer;
> > +     int ret;
> > +
> > +     st->msg = spi_message_alloc(1, GFP_KERNEL);
> > +     if (!st->msg)
> > +             return -ENOMEM;
> > +
> > +     xfer = list_first_entry(&st->msg->transfers, struct spi_transfer,
> > +                             transfer_list);
> > +
> > +     xfer->bits_per_word = st->chip_info->channels[0].scan_type.realbits;
> > +     xfer->len = 4;
> > +     xfer->rx_buf = st->scan_data.raw;
> > +
> > +     ret = spi_optimize_message(st->spi, st->msg);
> > +     if (ret) {
> > +             spi_message_free(st->msg);
> > +             return ret;
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +static int ad7380_buffer_postdisable(struct iio_dev *indio_dev)
> > +{
> > +     struct ad7380_state *st = iio_priv(indio_dev);
> > +
> > +     spi_unoptimize_message(st->msg);
> > +     spi_message_free(st->msg);
> > +
> > +     return 0;
> > +}
> > +
>
> Not such a big deal but unless I'm missing something we could have the
> spi_message (+ the transfer) statically allocated in struct ad7380_state and do
> the optimize only once at probe (naturally with proper devm action for
> unoptimize). Then we would not need to this for every buffer enable + disable. I
> know in terms of performance it won't matter but it would be less code I guess.
>
> Am I missing something?

No, your understanding is correct for the current state of everything
in this series. So, we could do as you suggest, but I have a feeling
that future additions to this driver might require that it gets
changed back this way eventually.
Nuno Sá Feb. 13, 2024, 4:08 p.m. UTC | #3
On Tue, 2024-02-13 at 09:27 -0600, David Lechner wrote:
> On Tue, Feb 13, 2024 at 3:47 AM Nuno Sá <noname.nuno@gmail.com> wrote:
> > 
> > On Mon, 2024-02-12 at 17:26 -0600, David Lechner wrote:
> > > This modifies the ad7380 ADC driver to use spi_optimize_message() to
> > > optimize the SPI message for the buffered read operation. Since buffered
> > > reads reuse the same SPI message for each read, this can improve
> > > performance by reducing the overhead of setting up some parts the SPI
> > > message in each spi_sync() call.
> > > 
> > > Signed-off-by: David Lechner <dlechner@baylibre.com>
> > > ---
> > >  drivers/iio/adc/ad7380.c | 52 +++++++++++++++++++++++++++++++++++++++++--
> > > ----
> > > -
> > >  1 file changed, 45 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/drivers/iio/adc/ad7380.c b/drivers/iio/adc/ad7380.c
> > > index abd746aef868..5c5d2642a474 100644
> > > --- a/drivers/iio/adc/ad7380.c
> > > +++ b/drivers/iio/adc/ad7380.c
> > > @@ -133,6 +133,7 @@ struct ad7380_state {
> > >       struct spi_device *spi;
> > >       struct regulator *vref;
> > >       struct regmap *regmap;
> > > +     struct spi_message *msg;
> > >       /*
> > >        * DMA (thus cache coherency maintenance) requires the
> > >        * transfer buffers to live in their own cache lines.
> > > @@ -231,19 +232,55 @@ static int ad7380_debugfs_reg_access(struct iio_dev
> > > *indio_dev, u32 reg,
> > >       return ret;
> > >  }
> > > 
> > > +static int ad7380_buffer_preenable(struct iio_dev *indio_dev)
> > > +{
> > > +     struct ad7380_state *st = iio_priv(indio_dev);
> > > +     struct spi_transfer *xfer;
> > > +     int ret;
> > > +
> > > +     st->msg = spi_message_alloc(1, GFP_KERNEL);
> > > +     if (!st->msg)
> > > +             return -ENOMEM;
> > > +
> > > +     xfer = list_first_entry(&st->msg->transfers, struct spi_transfer,
> > > +                             transfer_list);
> > > +
> > > +     xfer->bits_per_word = st->chip_info->channels[0].scan_type.realbits;
> > > +     xfer->len = 4;
> > > +     xfer->rx_buf = st->scan_data.raw;
> > > +
> > > +     ret = spi_optimize_message(st->spi, st->msg);
> > > +     if (ret) {
> > > +             spi_message_free(st->msg);
> > > +             return ret;
> > > +     }
> > > +
> > > +     return 0;
> > > +}
> > > +
> > > +static int ad7380_buffer_postdisable(struct iio_dev *indio_dev)
> > > +{
> > > +     struct ad7380_state *st = iio_priv(indio_dev);
> > > +
> > > +     spi_unoptimize_message(st->msg);
> > > +     spi_message_free(st->msg);
> > > +
> > > +     return 0;
> > > +}
> > > +
> > 
> > Not such a big deal but unless I'm missing something we could have the
> > spi_message (+ the transfer) statically allocated in struct ad7380_state and
> > do
> > the optimize only once at probe (naturally with proper devm action for
> > unoptimize). Then we would not need to this for every buffer enable +
> > disable. I
> > know in terms of performance it won't matter but it would be less code I
> > guess.
> > 
> > Am I missing something?
> 
> No, your understanding is correct for the current state of everything
> in this series. So, we could do as you suggest, but I have a feeling
> that future additions to this driver might require that it gets
> changed back this way eventually.

Hmm, not really sure about that as chip_info stuff is always our friend :). And
I'm anyways of the opinion of keeping things simpler and start to evolve when
really needed (because often we never really need to evolve). But bah, as I
said... this is really not a big deal.

- Nuno Sá
Jonathan Cameron Feb. 13, 2024, 5:28 p.m. UTC | #4
On Mon, 12 Feb 2024 17:26:45 -0600
David Lechner <dlechner@baylibre.com> wrote:

> This modifies the ad7380 ADC driver to use spi_optimize_message() to
> optimize the SPI message for the buffered read operation. Since buffered
> reads reuse the same SPI message for each read, this can improve
> performance by reducing the overhead of setting up some parts the SPI
> message in each spi_sync() call.
> 
> Signed-off-by: David Lechner <dlechner@baylibre.com>
> ---
>  drivers/iio/adc/ad7380.c | 52 +++++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 45 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7380.c b/drivers/iio/adc/ad7380.c
> index abd746aef868..5c5d2642a474 100644
> --- a/drivers/iio/adc/ad7380.c
> +++ b/drivers/iio/adc/ad7380.c
> @@ -133,6 +133,7 @@ struct ad7380_state {
>  	struct spi_device *spi;
>  	struct regulator *vref;
>  	struct regmap *regmap;
> +	struct spi_message *msg;
>  	/*
>  	 * DMA (thus cache coherency maintenance) requires the
>  	 * transfer buffers to live in their own cache lines.
> @@ -231,19 +232,55 @@ static int ad7380_debugfs_reg_access(struct iio_dev *indio_dev, u32 reg,
>  	return ret;
>  }
>  
> +static int ad7380_buffer_preenable(struct iio_dev *indio_dev)
> +{
> +	struct ad7380_state *st = iio_priv(indio_dev);
> +	struct spi_transfer *xfer;
> +	int ret;
> +
> +	st->msg = spi_message_alloc(1, GFP_KERNEL);

As it only ever has one element, is there a clear advantage over
just embedding the spi_message in the structure rather than
as a separate allocation? You'd need the transfer as well.

	spi_message_init_with_transfers(st->msg, &st->trans, 1);

The transfer is then also available without walking the list (though
obviously you don't walk very far ;).

> +	if (!st->msg)
> +		return -ENOMEM;
> +
> +	xfer = list_first_entry(&st->msg->transfers, struct spi_transfer,
> +				transfer_list);
> +
> +	xfer->bits_per_word = st->chip_info->channels[0].scan_type.realbits;
> +	xfer->len = 4;
> +	xfer->rx_buf = st->scan_data.raw;
> +
> +	ret = spi_optimize_message(st->spi, st->msg);
> +	if (ret) {
> +		spi_message_free(st->msg);
Would avoid freeing explicitly here or later if it was embedded in
struct ad7380_state

Also, this doesn't seem very dynamic in general. Anything stopping this
being done at probe() as a one time thing?

> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int ad7380_buffer_postdisable(struct iio_dev *indio_dev)
> +{
> +	struct ad7380_state *st = iio_priv(indio_dev);
> +
> +	spi_unoptimize_message(st->msg);
> +	spi_message_free(st->msg);
> +
> +	return 0;
> +}
> +
> +static const struct iio_buffer_setup_ops ad7380_buffer_setup_ops = {
> +	.preenable = ad7380_buffer_preenable,
> +	.postdisable = ad7380_buffer_postdisable,
> +};
> +
>  static irqreturn_t ad7380_trigger_handler(int irq, void *p)
>  {
>  	struct iio_poll_func *pf = p;
>  	struct iio_dev *indio_dev = pf->indio_dev;
>  	struct ad7380_state *st = iio_priv(indio_dev);
> -	struct spi_transfer xfer = {
> -		.bits_per_word = st->chip_info->channels[0].scan_type.realbits,
> -		.len = 4,
> -		.rx_buf = st->scan_data.raw,
> -	};
>  	int ret;
>  
> -	ret = spi_sync_transfer(st->spi, &xfer, 1);
> +	ret = spi_sync(st->spi, st->msg);
>  	if (ret)
>  		goto out;
>  
> @@ -420,7 +457,8 @@ static int ad7380_probe(struct spi_device *spi)
>  
>  	ret = devm_iio_triggered_buffer_setup(&spi->dev, indio_dev,
>  					      iio_pollfunc_store_time,
> -					      ad7380_trigger_handler, NULL);
> +					      ad7380_trigger_handler,
> +					      &ad7380_buffer_setup_ops);
>  	if (ret)
>  		return ret;
>  
>
Jonathan Cameron Feb. 13, 2024, 5:31 p.m. UTC | #5
On Tue, 13 Feb 2024 17:08:19 +0100
Nuno Sá <noname.nuno@gmail.com> wrote:

> On Tue, 2024-02-13 at 09:27 -0600, David Lechner wrote:
> > On Tue, Feb 13, 2024 at 3:47 AM Nuno Sá <noname.nuno@gmail.com> wrote:  
> > > 
> > > On Mon, 2024-02-12 at 17:26 -0600, David Lechner wrote:  
> > > > This modifies the ad7380 ADC driver to use spi_optimize_message() to
> > > > optimize the SPI message for the buffered read operation. Since buffered
> > > > reads reuse the same SPI message for each read, this can improve
> > > > performance by reducing the overhead of setting up some parts the SPI
> > > > message in each spi_sync() call.
> > > > 
> > > > Signed-off-by: David Lechner <dlechner@baylibre.com>
> > > > ---
> > > >  drivers/iio/adc/ad7380.c | 52 +++++++++++++++++++++++++++++++++++++++++--
> > > > ----
> > > > -
> > > >  1 file changed, 45 insertions(+), 7 deletions(-)
> > > > 
> > > > diff --git a/drivers/iio/adc/ad7380.c b/drivers/iio/adc/ad7380.c
> > > > index abd746aef868..5c5d2642a474 100644
> > > > --- a/drivers/iio/adc/ad7380.c
> > > > +++ b/drivers/iio/adc/ad7380.c
> > > > @@ -133,6 +133,7 @@ struct ad7380_state {
> > > >       struct spi_device *spi;
> > > >       struct regulator *vref;
> > > >       struct regmap *regmap;
> > > > +     struct spi_message *msg;
> > > >       /*
> > > >        * DMA (thus cache coherency maintenance) requires the
> > > >        * transfer buffers to live in their own cache lines.
> > > > @@ -231,19 +232,55 @@ static int ad7380_debugfs_reg_access(struct iio_dev
> > > > *indio_dev, u32 reg,
> > > >       return ret;
> > > >  }
> > > > 
> > > > +static int ad7380_buffer_preenable(struct iio_dev *indio_dev)
> > > > +{
> > > > +     struct ad7380_state *st = iio_priv(indio_dev);
> > > > +     struct spi_transfer *xfer;
> > > > +     int ret;
> > > > +
> > > > +     st->msg = spi_message_alloc(1, GFP_KERNEL);
> > > > +     if (!st->msg)
> > > > +             return -ENOMEM;
> > > > +
> > > > +     xfer = list_first_entry(&st->msg->transfers, struct spi_transfer,
> > > > +                             transfer_list);
> > > > +
> > > > +     xfer->bits_per_word = st->chip_info->channels[0].scan_type.realbits;
> > > > +     xfer->len = 4;
> > > > +     xfer->rx_buf = st->scan_data.raw;
> > > > +
> > > > +     ret = spi_optimize_message(st->spi, st->msg);
> > > > +     if (ret) {
> > > > +             spi_message_free(st->msg);
> > > > +             return ret;
> > > > +     }
> > > > +
> > > > +     return 0;
> > > > +}
> > > > +
> > > > +static int ad7380_buffer_postdisable(struct iio_dev *indio_dev)
> > > > +{
> > > > +     struct ad7380_state *st = iio_priv(indio_dev);
> > > > +
> > > > +     spi_unoptimize_message(st->msg);
> > > > +     spi_message_free(st->msg);
> > > > +
> > > > +     return 0;
> > > > +}
> > > > +  
> > > 
> > > Not such a big deal but unless I'm missing something we could have the
> > > spi_message (+ the transfer) statically allocated in struct ad7380_state and
> > > do
> > > the optimize only once at probe (naturally with proper devm action for
> > > unoptimize). Then we would not need to this for every buffer enable +
> > > disable. I
> > > know in terms of performance it won't matter but it would be less code I
> > > guess.
> > > 
> > > Am I missing something?  
> > 
> > No, your understanding is correct for the current state of everything
> > in this series. So, we could do as you suggest, but I have a feeling
> > that future additions to this driver might require that it gets
> > changed back this way eventually.  
> 
> Hmm, not really sure about that as chip_info stuff is always our friend :). And
> I'm anyways of the opinion of keeping things simpler and start to evolve when
> really needed (because often we never really need to evolve). But bah, as I
> said... this is really not a big deal.
> 
Oops should have read Nuno's review before replying!

I'd rather we embedded it for now and did the optimization at probe.
Whilst it's a lot of work per transfer it's not enough to worry about delaying
it until preenable().  Easy to make that move and take it dynamic when
driver changes need it.  In meantime, I don't want lots of other drivers
picking up this pattern when they may never need the complexity of
making things more dynamic.

Jonathan

> - Nuno Sá
>
David Lechner Feb. 13, 2024, 6:59 p.m. UTC | #6
On Tue, Feb 13, 2024 at 11:31 AM Jonathan Cameron
<Jonathan.Cameron@huawei.com> wrote:
>
> On Tue, 13 Feb 2024 17:08:19 +0100
> Nuno Sá <noname.nuno@gmail.com> wrote:
>
> > On Tue, 2024-02-13 at 09:27 -0600, David Lechner wrote:
> > > On Tue, Feb 13, 2024 at 3:47 AM Nuno Sá <noname.nuno@gmail.com> wrote:
> > > >

...

> > > > Am I missing something?
> > >
> > > No, your understanding is correct for the current state of everything
> > > in this series. So, we could do as you suggest, but I have a feeling
> > > that future additions to this driver might require that it gets
> > > changed back this way eventually.
> >
> > Hmm, not really sure about that as chip_info stuff is always our friend :). And
> > I'm anyways of the opinion of keeping things simpler and start to evolve when
> > really needed (because often we never really need to evolve). But bah, as I
> > said... this is really not a big deal.
> >
> Oops should have read Nuno's review before replying!
>
> I'd rather we embedded it for now and did the optimization at probe.
> Whilst it's a lot of work per transfer it's not enough to worry about delaying
> it until preenable().  Easy to make that move and take it dynamic when
> driver changes need it.  In meantime, I don't want lots of other drivers
> picking up this pattern when they may never need the complexity of
> making things more dynamic.
>

Noted.
diff mbox series

Patch

diff --git a/drivers/iio/adc/ad7380.c b/drivers/iio/adc/ad7380.c
index abd746aef868..5c5d2642a474 100644
--- a/drivers/iio/adc/ad7380.c
+++ b/drivers/iio/adc/ad7380.c
@@ -133,6 +133,7 @@  struct ad7380_state {
 	struct spi_device *spi;
 	struct regulator *vref;
 	struct regmap *regmap;
+	struct spi_message *msg;
 	/*
 	 * DMA (thus cache coherency maintenance) requires the
 	 * transfer buffers to live in their own cache lines.
@@ -231,19 +232,55 @@  static int ad7380_debugfs_reg_access(struct iio_dev *indio_dev, u32 reg,
 	return ret;
 }
 
+static int ad7380_buffer_preenable(struct iio_dev *indio_dev)
+{
+	struct ad7380_state *st = iio_priv(indio_dev);
+	struct spi_transfer *xfer;
+	int ret;
+
+	st->msg = spi_message_alloc(1, GFP_KERNEL);
+	if (!st->msg)
+		return -ENOMEM;
+
+	xfer = list_first_entry(&st->msg->transfers, struct spi_transfer,
+				transfer_list);
+
+	xfer->bits_per_word = st->chip_info->channels[0].scan_type.realbits;
+	xfer->len = 4;
+	xfer->rx_buf = st->scan_data.raw;
+
+	ret = spi_optimize_message(st->spi, st->msg);
+	if (ret) {
+		spi_message_free(st->msg);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int ad7380_buffer_postdisable(struct iio_dev *indio_dev)
+{
+	struct ad7380_state *st = iio_priv(indio_dev);
+
+	spi_unoptimize_message(st->msg);
+	spi_message_free(st->msg);
+
+	return 0;
+}
+
+static const struct iio_buffer_setup_ops ad7380_buffer_setup_ops = {
+	.preenable = ad7380_buffer_preenable,
+	.postdisable = ad7380_buffer_postdisable,
+};
+
 static irqreturn_t ad7380_trigger_handler(int irq, void *p)
 {
 	struct iio_poll_func *pf = p;
 	struct iio_dev *indio_dev = pf->indio_dev;
 	struct ad7380_state *st = iio_priv(indio_dev);
-	struct spi_transfer xfer = {
-		.bits_per_word = st->chip_info->channels[0].scan_type.realbits,
-		.len = 4,
-		.rx_buf = st->scan_data.raw,
-	};
 	int ret;
 
-	ret = spi_sync_transfer(st->spi, &xfer, 1);
+	ret = spi_sync(st->spi, st->msg);
 	if (ret)
 		goto out;
 
@@ -420,7 +457,8 @@  static int ad7380_probe(struct spi_device *spi)
 
 	ret = devm_iio_triggered_buffer_setup(&spi->dev, indio_dev,
 					      iio_pollfunc_store_time,
-					      ad7380_trigger_handler, NULL);
+					      ad7380_trigger_handler,
+					      &ad7380_buffer_setup_ops);
 	if (ret)
 		return ret;