[PATCH 3/8] spi: spi-ep93xx: always handle transfer specific settings

[PATCH 3/8] spi: spi-ep93xx: always handle transfer specific settings

[H Hartley Sweeten]

__spi_async(), which starts every SPI message transfer, initializes
the bits_per_word and max speed for every transfer in the message.
Since the conditional test in ep93xx_spi_process_transfer() will
always succeed just remove it and always call ep93xx_spi_chip_setup()
to configure the hardware for each transfer in the message.

Remove the redundant ep93xx_spi_chp_setup() in ep93xx_spi_process_transfer()
which just initializes the hardware to the "default" based on the SPI
device.

Signed-off-by: H Hartley Sweeten <hsweeten@visionengravers.com>
Cc: Ryan Mallon <rmallon@gmail.com>
Cc: Mika Westerberg <mika.westerberg@iki.fi>
Cc: Mark Brown <broonie@kernel.org>
Cc: Grant Likely <grant.likely@linaro.org>
---
 drivers/spi/spi-ep93xx.c | 43 ++++++++++---------------------------------
 1 file changed, 10 insertions(+), 33 deletions(-)

diff --git a/drivers/spi/spi-ep93xx.c b/drivers/spi/spi-ep93xx.c
index 93ae7b6..bcfd35a 100644
--- a/drivers/spi/spi-ep93xx.c
+++ b/drivers/spi/spi-ep93xx.c
@@ -684,38 +684,20 @@ static void ep93xx_spi_process_transfer(struct ep93xx_spi *espi,
 					struct spi_transfer *t)
 {
 	struct ep93xx_spi_chip *chip = spi_get_ctldata(msg->spi);
+	int err;
 
 	msg->state = t;
 
-	/*
-	 * Handle any transfer specific settings if needed. We use
-	 * temporary chip settings here and restore original later when
-	 * the transfer is finished.
-	 */
-	if (t->speed_hz || t->bits_per_word) {
-		struct ep93xx_spi_chip tmp_chip = *chip;
-
-		if (t->speed_hz) {
-			int err;
-
-			err = ep93xx_spi_calc_divisors(espi, &tmp_chip,
-						       t->speed_hz);
-			if (err) {
-				dev_err(&espi->pdev->dev,
-					"failed to adjust speed\n");
-				msg->status = err;
-				return;
-			}
-		}
+	err = ep93xx_spi_calc_divisors(espi, chip, t->speed_hz);
+	if (err) {
+		dev_err(&espi->pdev->dev, "failed to adjust speed\n");
+		msg->status = err;
+		return;
+	}
 
-		if (t->bits_per_word)
-			tmp_chip.dss = bits_per_word_to_dss(t->bits_per_word);
+	chip->dss = bits_per_word_to_dss(t->bits_per_word);
 
-		/*
-		 * Set up temporary new hw settings for this transfer.
-		 */
-		ep93xx_spi_chip_setup(espi, &tmp_chip);
-	}
+	ep93xx_spi_chip_setup(espi, chip);
 
 	espi->rx = 0;
 	espi->tx = 0;
@@ -759,9 +741,6 @@ static void ep93xx_spi_process_transfer(struct ep93xx_spi *espi,
 			ep93xx_spi_cs_control(msg->spi, true);
 		}
 	}
-
-	if (t->speed_hz || t->bits_per_word)
-		ep93xx_spi_chip_setup(espi, chip);
 }
 
 /*
@@ -814,10 +793,8 @@ static void ep93xx_spi_process_message(struct ep93xx_spi *espi,
 	espi->fifo_level = 0;
 
 	/*
-	 * Update SPI controller registers according to spi device and assert
-	 * the chipselect.
+	 * Assert the chipselect.
 	 */
-	ep93xx_spi_chip_setup(espi, spi_get_ctldata(msg->spi));
 	ep93xx_spi_cs_control(msg->spi, true);
 
 	list_for_each_entry(t, &msg->transfers, transfer_list) {
-- 
1.8.1.4

Re: [PATCH 3/8] spi: spi-ep93xx: always handle transfer specific settings

[Ryan Mallon]

On 29/06/13 04:43, H Hartley Sweeten wrote:

> __spi_async(), which starts every SPI message transfer, initializes
> the bits_per_word and max speed for every transfer in the message.
> Since the conditional test in ep93xx_spi_process_transfer() will
> always succeed just remove it and always call ep93xx_spi_chip_setup()
> to configure the hardware for each transfer in the message.
> 
> Remove the redundant ep93xx_spi_chp_setup() in ep93xx_spi_process_transfer()
> which just initializes the hardware to the "default" based on the SPI
> device.
> 
> Signed-off-by: H Hartley Sweeten <hsweeten@visionengravers.com>
> Cc: Ryan Mallon <rmallon@gmail.com>
> Cc: Mika Westerberg <mika.westerberg@iki.fi>
> Cc: Mark Brown <broonie@kernel.org>
> Cc: Grant Likely <grant.likely@linaro.org>
> ---


> +	err = ep93xx_spi_calc_divisors(espi, chip, t->speed_hz);
> +	if (err) {
> +		dev_err(&espi->pdev->dev, "failed to adjust speed\n");


Printing out the speed it was trying to set might be useful here?

~Ryan

RE: [PATCH 3/8] spi: spi-ep93xx: always handle transfer specific settings

[H Hartley Sweeten]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1996 bytes --]

On Friday, June 28, 2013 4:18 PM, Ryan Mallon wrote:
> On 29/06/13 04:43, H Hartley Sweeten wrote:
>> __spi_async(), which starts every SPI message transfer, initializes
>> the bits_per_word and max speed for every transfer in the message.
>> Since the conditional test in ep93xx_spi_process_transfer() will
>> always succeed just remove it and always call ep93xx_spi_chip_setup()
>> to configure the hardware for each transfer in the message.
>> 
>> Remove the redundant ep93xx_spi_chp_setup() in ep93xx_spi_process_transfer()
>> which just initializes the hardware to the "default" based on the SPI
>> device.
>> 
>> Signed-off-by: H Hartley Sweeten <hsweeten@visionengravers.com>
>> Cc: Ryan Mallon <rmallon@gmail.com>
>> Cc: Mika Westerberg <mika.westerberg@iki.fi>
>> Cc: Mark Brown <broonie@kernel.org>
>> Cc: Grant Likely <grant.likely@linaro.org>
>> ---
>
>
>> +	err = ep93xx_spi_calc_divisors(espi, chip, t->speed_hz);
>> +	if (err) {
>> +		dev_err(&espi->pdev->dev, "failed to adjust speed\n");
>
>
> Printing out the speed it was trying to set might be useful here?

Technically I don't think this can ever happen.

The minimum and maximum possible speeds are determined during the probe.

	espi->max_rate = clk_get_rate(espi->clk) / 2;
	espi->min_rate = clk_get_rate(espi->clk) / (254 * 256);

Each transfer is validated to make sure the speed is greater than the
minimum.

		if (t->speed_hz < espi->min_rate)
			return -EINVAL;

Then the rate is clamped to the min/max rates.

	rate = clamp(rate, espi->min_rate, espi->max_rate);

The calculations for the div_csr and div_cpsr values needed to produce
the desired rate should always succeed.

Patch 7/8 actually replaces that dev_err() message with a more generic
one. In reality, even the new message, and the error checking, could
probably be removed.

Regards,
Hartley

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

RE: [PATCH 3/8] spi: spi-ep93xx: always handle transfer specific settings

[H Hartley Sweeten]

On Friday, June 28, 2013 4:18 PM, Ryan Mallon wrote:
> On 29/06/13 04:43, H Hartley Sweeten wrote:
>> __spi_async(), which starts every SPI message transfer, initializes
>> the bits_per_word and max speed for every transfer in the message.
>> Since the conditional test in ep93xx_spi_process_transfer() will
>> always succeed just remove it and always call ep93xx_spi_chip_setup()
>> to configure the hardware for each transfer in the message.
>> 
>> Remove the redundant ep93xx_spi_chp_setup() in ep93xx_spi_process_transfer()
>> which just initializes the hardware to the "default" based on the SPI
>> device.
>> 
>> Signed-off-by: H Hartley Sweeten <hsweeten-3FF4nKcrg1dE2c76skzGb0EOCMrvLtNR@public.gmane.org>
>> Cc: Ryan Mallon <rmallon-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> Cc: Mika Westerberg <mika.westerberg-X3B1VOXEql0@public.gmane.org>
>> Cc: Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>> Cc: Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>> ---
>
>
>> +	err = ep93xx_spi_calc_divisors(espi, chip, t->speed_hz);
>> +	if (err) {
>> +		dev_err(&espi->pdev->dev, "failed to adjust speed\n");
>
>
> Printing out the speed it was trying to set might be useful here?

Technically I don't think this can ever happen.

The minimum and maximum possible speeds are determined during the probe.

	espi->max_rate = clk_get_rate(espi->clk) / 2;
	espi->min_rate = clk_get_rate(espi->clk) / (254 * 256);

Each transfer is validated to make sure the speed is greater than the
minimum.

		if (t->speed_hz < espi->min_rate)
			return -EINVAL;

Then the rate is clamped to the min/max rates.

	rate = clamp(rate, espi->min_rate, espi->max_rate);

The calculations for the div_csr and div_cpsr values needed to produce
the desired rate should always succeed.

Patch 7/8 actually replaces that dev_err() message with a more generic
one. In reality, even the new message, and the error checking, could
probably be removed.

Regards,
Hartley

------------------------------------------------------------------------------
This SF.net email is sponsored by Windows:

Build for Windows Store.

http://p.sf.net/sfu/windows-dev2dev

Re: [PATCH 3/8] spi: spi-ep93xx: always handle transfer specific settings

[Mika Westerberg]

On Fri, Jun 28, 2013 at 11:43:34AM -0700, H Hartley Sweeten wrote:
> __spi_async(), which starts every SPI message transfer, initializes
> the bits_per_word and max speed for every transfer in the message.
> Since the conditional test in ep93xx_spi_process_transfer() will
> always succeed just remove it and always call ep93xx_spi_chip_setup()
> to configure the hardware for each transfer in the message.
> 
> Remove the redundant ep93xx_spi_chp_setup() in ep93xx_spi_process_transfer()
> which just initializes the hardware to the "default" based on the SPI
> device.
> 
> Signed-off-by: H Hartley Sweeten <hsweeten@visionengravers.com>
> Cc: Ryan Mallon <rmallon@gmail.com>
> Cc: Mika Westerberg <mika.westerberg@iki.fi>

Acked-by: Mika Westerberg <mika.westerberg@iki.fi>

Re: [PATCH 3/8] spi: spi-ep93xx: always handle transfer specific settings

[Mika Westerberg]

On Fri, Jun 28, 2013 at 11:43:34AM -0700, H Hartley Sweeten wrote:
> __spi_async(), which starts every SPI message transfer, initializes
> the bits_per_word and max speed for every transfer in the message.
> Since the conditional test in ep93xx_spi_process_transfer() will
> always succeed just remove it and always call ep93xx_spi_chip_setup()
> to configure the hardware for each transfer in the message.
> 
> Remove the redundant ep93xx_spi_chp_setup() in ep93xx_spi_process_transfer()
> which just initializes the hardware to the "default" based on the SPI
> device.
> 
> Signed-off-by: H Hartley Sweeten <hsweeten-3FF4nKcrg1dE2c76skzGb0EOCMrvLtNR@public.gmane.org>
> Cc: Ryan Mallon <rmallon-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Cc: Mika Westerberg <mika.westerberg-X3B1VOXEql0@public.gmane.org>

Acked-by: Mika Westerberg <mika.westerberg-X3B1VOXEql0@public.gmane.org>

------------------------------------------------------------------------------
This SF.net email is sponsored by Windows:

Build for Windows Store.

http://p.sf.net/sfu/windows-dev2dev

Re: [PATCH 3/8] spi: spi-ep93xx: always handle transfer specific settings

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 425 bytes --]

On Fri, Jun 28, 2013 at 11:43:34AM -0700, H Hartley Sweeten wrote:
> __spi_async(), which starts every SPI message transfer, initializes
> the bits_per_word and max speed for every transfer in the message.
> Since the conditional test in ep93xx_spi_process_transfer() will
> always succeed just remove it and always call ep93xx_spi_chip_setup()
> to configure the hardware for each transfer in the message.

Applied, thanks.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

RE: [PATCH 3/8] spi: spi-ep93xx: always handle transfer specific settings

[H Hartley Sweeten]

On Monday, July 01, 2013 3:28 AM, Mark Brown wrote:
> On Fri, Jun 28, 2013 at 11:43:34AM -0700, H Hartley Sweeten wrote:
>> __spi_async(), which starts every SPI message transfer, initializes
>> the bits_per_word and max speed for every transfer in the message.
>> Since the conditional test in ep93xx_spi_process_transfer() will
>> always succeed just remove it and always call ep93xx_spi_chip_setup()
>> to configure the hardware for each transfer in the message.
>
> Applied, thanks.

Mark,

I was going to redo this series based on the comments I have received
so far. Did you already apply this one? Should I push it to the front of my
patchset?

Regards,
Hartley

RE: [PATCH 3/8] spi: spi-ep93xx: always handle transfer specific settings

[H Hartley Sweeten]

On Monday, July 01, 2013 3:28 AM, Mark Brown wrote:
> On Fri, Jun 28, 2013 at 11:43:34AM -0700, H Hartley Sweeten wrote:
>> __spi_async(), which starts every SPI message transfer, initializes
>> the bits_per_word and max speed for every transfer in the message.
>> Since the conditional test in ep93xx_spi_process_transfer() will
>> always succeed just remove it and always call ep93xx_spi_chip_setup()
>> to configure the hardware for each transfer in the message.
>
> Applied, thanks.

Mark,

I was going to redo this series based on the comments I have received
so far. Did you already apply this one? Should I push it to the front of my
patchset?

Regards,
Hartley

Re: [PATCH 3/8] spi: spi-ep93xx: always handle transfer specific settings

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 420 bytes --]

On Mon, Jul 01, 2013 at 01:15:40PM -0500, H Hartley Sweeten wrote:
> On Monday, July 01, 2013 3:28 AM, Mark Brown wrote:

> > Applied, thanks.

> I was going to redo this series based on the comments I have received
> so far. Did you already apply this one? Should I push it to the front of my
> patchset?

That's what I said.  You shouldn't resend it at all, you should base
your updates off the relevant topic branch.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 3/8] spi: spi-ep93xx: always handle transfer specific settings

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 420 bytes --]

On Mon, Jul 01, 2013 at 01:15:40PM -0500, H Hartley Sweeten wrote:
> On Monday, July 01, 2013 3:28 AM, Mark Brown wrote:

> > Applied, thanks.

> I was going to redo this series based on the comments I have received
> so far. Did you already apply this one? Should I push it to the front of my
> patchset?

That's what I said.  You shouldn't resend it at all, you should base
your updates off the relevant topic branch.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]