Re: [PATCH] spi: pxa2xx_spi introduce chip select gpio to simplify the common cases

Re: [PATCH] spi: pxa2xx_spi introduce chip select gpio to simplify the common cases

[David Brownell]

On Thursday 04 September 2008, Eric Miao wrote:
> As Russell suggested, updated with gpio_is_valid().

He beat me to that comment.  ;)

I like, but that doesn't apply on top of the bugfix patch
which I just cc'd you on...

Related:

 - It's probably worth removing support for the SSFRM
   mechanism and requiring gpio_cs or (at least as a
   transitional scheme) the cs_control() thing.

   I suggest issuing a strong warning for all devices
   which assume the SSFRM mechanism (single device
   on the bus segment too) ... and maybe a weaker one
   if they use cs_control, saying "use gpio_cs".

 - Remove the gpio_cs_inverted thing ... that's exactly
   what SPI_CS_HIGH is there to handle (in spi->mode).

The SSFRM issue is, briefly, that it forces deselect
of the chip between transfer segments even when the
driver doesn't want that.  If it's not disabled, the
transfer() routine needs integrity checks to make
sure it rejects all messages that don't expect that
deselection (unless cs_control/gpio_cs is used for
that particular spi_device).

The cs_control issue is more just simplification:  as
I recall, all users are just wrapping GPIOS, and giving
up potential SPI_CS_HIGH support while doing it.

- Dave


============
From: "Eric Miao" <eric.y.miao-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

Most SPI peripherals use GPIOs as their chip select, introduce .gpio_cs
for this, to avoid 

Signed-off-by: Eric Miao <eric.miao-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>
---
 arch/arm/mach-pxa/include/mach/pxa2xx_spi.h |    1 +
 drivers/spi/pxa2xx_spi.c                    |   92 ++++++++++++++++++++++-----
 2 files changed, 78 insertions(+), 15 deletions(-)

diff --git a/arch/arm/mach-pxa/include/mach/pxa2xx_spi.h
b/arch/arm/mach-pxa/include/mach/pxa2xx_spi.h
index 2206cb6..b87cecd 100644
--- a/arch/arm/mach-pxa/include/mach/pxa2xx_spi.h
+++ b/arch/arm/mach-pxa/include/mach/pxa2xx_spi.h
@@ -38,6 +38,7 @@ struct pxa2xx_spi_chip {
 	u8 dma_burst_size;
 	u32 timeout;
 	u8 enable_loopback;
+	int gpio_cs;
 	void (*cs_control)(u32 command);
 };

diff --git a/drivers/spi/pxa2xx_spi.c b/drivers/spi/pxa2xx_spi.c
index 34c7c98..98d2338 100644
--- a/drivers/spi/pxa2xx_spi.c
+++ b/drivers/spi/pxa2xx_spi.c
@@ -28,6 +28,7 @@
 #include <linux/workqueue.h>
 #include <linux/delay.h>
 #include <linux/clk.h>
+#include <linux/gpio.h>

 #include <asm/io.h>
 #include <asm/irq.h>
@@ -164,6 +165,8 @@ struct chip_data {
 	u8 enable_dma;
 	u8 bits_per_word;
 	u32 speed_hz;
+	int gpio_cs;
+	unsigned gpio_cs_inverted : 1;
 	int (*write)(struct driver_data *drv_data);
 	int (*read)(struct driver_data *drv_data);
 	void (*cs_control)(u32 command);
@@ -171,6 +174,32 @@ struct chip_data {

 static void pump_messages(struct work_struct *work);

+static void cs_assert(struct driver_data *drv_data)
+{
+	struct chip_data *chip = drv_data->cur_chip;
+
+	if (chip->cs_control) {
+		chip->cs_control(PXA2XX_CS_ASSERT);
+		return;
+	}
+
+	if (gpio_is_valid(chip->gpio_cs))
+		gpio_set_value(chip->gpio_cs, chip->gpio_cs_inverted);
+}
+
+static void cs_deassert(struct driver_data *drv_data)
+{
+	struct chip_data *chip = drv_data->cur_chip;
+
+	if (chip->cs_control) {
+		chip->cs_control(PXA2XX_CS_DEASSERT);
+		return;
+	}
+
+	if (gpio_is_valid(chip->gpio_cs))
+		gpio_set_value(chip->gpio_cs, !chip->gpio_cs_inverted);
+}
+
 static int flush(struct driver_data *drv_data)
 {
 	unsigned long limit = loops_per_jiffy << 1;
@@ -187,10 +216,6 @@ static int flush(struct driver_data *drv_data)
 	return limit;
 }

-static void null_cs_control(u32 command)
-{
-}
-
 static int null_writer(struct driver_data *drv_data)
 {
 	void __iomem *reg = drv_data->ioaddr;
@@ -398,7 +423,6 @@ static void giveback(struct driver_data *drv_data)
 	msg = drv_data->cur_msg;
 	drv_data->cur_msg = NULL;
 	drv_data->cur_transfer = NULL;
-	drv_data->cur_chip = NULL;
 	queue_work(drv_data->workqueue, &drv_data->pump_messages);
 	spin_unlock_irqrestore(&drv_data->lock, flags);

@@ -407,7 +431,9 @@ static void giveback(struct driver_data *drv_data)
 					transfer_list);

 	if (!last_transfer->cs_change)
-		drv_data->cs_control(PXA2XX_CS_DEASSERT);
+		cs_deassert(drv_data);
+
+	drv_data->cur_chip = NULL;

 	msg->state = NULL;
 	if (msg->complete)
@@ -493,7 +519,7 @@ static void dma_transfer_complete(struct driver_data *drv_data)
 	/* Release chip select if requested, transfer delays are
 	 * handled in pump_transfers */
 	if (drv_data->cs_change)
-		drv_data->cs_control(PXA2XX_CS_DEASSERT);
+		cs_deassert(drv_data);

 	/* Move to next transfer */
 	msg->state = next_transfer(drv_data);
@@ -605,7 +631,7 @@ static void int_transfer_complete(struct driver_data *drv_data)
 	/* Release chip select if requested, transfer delays are
 	 * handled in pump_transfers */
 	if (drv_data->cs_change)
-		drv_data->cs_control(PXA2XX_CS_DEASSERT);
+		cs_deassert(drv_data);

 	/* Move to next transfer */
 	drv_data->cur_msg->state = next_transfer(drv_data);
@@ -868,7 +894,6 @@ static void pump_transfers(unsigned long data)
 	}
 	drv_data->n_bytes = chip->n_bytes;
 	drv_data->dma_width = chip->dma_width;
-	drv_data->cs_control = chip->cs_control;
 	drv_data->tx = (void *)transfer->tx_buf;
 	drv_data->tx_end = drv_data->tx + transfer->len;
 	drv_data->rx = transfer->rx_buf;
@@ -1020,7 +1045,7 @@ static void pump_transfers(unsigned long data)
 	 * this driver uses struct pxa2xx_spi_chip.cs_control to
 	 * specify a CS handling function, and it ignores most
 	 * struct spi_device.mode[s], including SPI_CS_HIGH */
-	drv_data->cs_control(PXA2XX_CS_ASSERT);
+	cs_assert(drv_data);

 	/* after chip select, release the data by enabling service
 	 * requests and interrupts, without changing any mode bits */
@@ -1098,6 +1123,43 @@ static int transfer(struct spi_device *spi, struct spi_message *msg)
 /* the spi->mode bits understood by this driver: */
 #define MODEBITS (SPI_CPOL | SPI_CPHA)

+static int setup_cs(struct spi_device *spi, struct chip_data *chip,
+		    struct pxa2xx_spi_chip *chip_info)
+{
+	int err = 0;
+	
+	if (chip_info == NULL)
+		return 0;
+
+	/* NOTE: setup() can be called multiple times, possibly with
+	 * different chip_info, previously requested GPIO shall be
+	 * released, stumped :(
+	 */
+	if (gpio_is_valid(chip->gpio_cs))
+		gpio_free(chip->gpio_cs);
+
+	if (chip_info->cs_control) {
+		chip->cs_control = chip_info->cs_control;
+		return 0;
+	}
+
+	if (gpio_is_valid(chip_info->gpio_cs)) {
+		err = gpio_request(chip_info->gpio_cs, "SPI_CS");
+		if (err) {
+			dev_err(&spi->dev, "failed to request CS GPIO%d\n",
+					chip_info->gpio_cs);
+			return err;
+		}
+
+		chip->gpio_cs = chip_info->gpio_cs;
+		chip->gpio_cs_inverted = spi->mode & SPI_CS_HIGH;
+
+		gpio_direction_output(chip->gpio_cs, !chip->gpio_cs_inverted);
+	}
+
+	return err;
+}
+
 static int setup(struct spi_device *spi)
 {
 	struct pxa2xx_spi_chip *chip_info = NULL;
@@ -1141,7 +1203,7 @@ static int setup(struct spi_device *spi)
 			return -ENOMEM;
 		}

-		chip->cs_control = null_cs_control;
+		chip->gpio_cs = -1;
 		chip->enable_dma = 0;
 		chip->timeout = 1000;
 		chip->threshold = SSCR1_RxTresh(1) | SSCR1_TxTresh(1);
@@ -1156,9 +1218,6 @@ static int setup(struct spi_device *spi)
 	/* chip_info isn't always needed */
 	chip->cr1 = 0;
 	if (chip_info) {
-		if (chip_info->cs_control)
-			chip->cs_control = chip_info->cs_control;
-
 		chip->timeout = chip_info->timeout;

 		chip->threshold = (SSCR1_RxTresh(chip_info->rx_threshold) &
@@ -1238,13 +1297,16 @@ static int setup(struct spi_device *spi)

 	spi_set_ctldata(spi, chip);

-	return 0;
+	return setup_cs(spi, chip, chip_info);
 }

 static void cleanup(struct spi_device *spi)
 {
 	struct chip_data *chip = spi_get_ctldata(spi);

+	if (gpio_is_valid(chip->gpio_cs))
+		gpio_free(chip->gpio_cs);
+
 	kfree(chip);
 }

-- 
1.5.4.3


-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/

Re: [PATCH] spi: pxa2xx_spi introduce chip select gpio to simplify the common cases

[Ned Forrester]

David Brownell wrote:
> On Thursday 04 September 2008, Eric Miao wrote:
> ============
> From: "Eric Miao" <eric.y.miao-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> 
> Most SPI peripherals use GPIOs as their chip select, introduce .gpio_cs
> for this, to avoid 
> 
> Signed-off-by: Eric Miao <eric.miao-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>

[snip]

> +static int setup_cs(struct spi_device *spi, struct chip_data *chip,
> +		    struct pxa2xx_spi_chip *chip_info)

My understanding is that it is legal to call setup without a defined
pointer to a struct pxa2xx_spi_chip in spi_dev->controller_data, if the
chip is happy with the defaults (only works for a single chip bus that
needs no CS, a degenerate case, but still legal).  Thus you should allow
for that by always checking for existence (chip_info not NULL) before use.

> +{
> +	int err = 0;
> +	
> +	if (chip_info == NULL)
> +		return 0;
> +
> +	/* NOTE: setup() can be called multiple times, possibly with
> +	 * different chip_info, previously requested GPIO shall be
> +	 * released, stumped :(
> +	 */

That is why the GPIO allocation function belongs in the controller
driver, and not in the master driver.  The master serves many
controllers, and only the controller drivers know when they are loaded
and unloaded, and thus when they need to allocate and initialize a GPIO
pin and when to release it.  Additionally, on a multi-chip bus (which
may have chips with CS assertion of differing polarity) it may be that
only the board init code can configure all the CS pins to an idle state
to get anything on the bus to work at all.  That is why cs_control
shifts the burden of configuring CS pins outside of the master driver.
I don't think that this function can work as written; perhaps this can
be converted to a non-static helper function to be called by either
board init or controller drivers.

> +	if (gpio_is_valid(chip->gpio_cs))
> +		gpio_free(chip->gpio_cs);
> +
> +	if (chip_info->cs_control) {
> +		chip->cs_control = chip_info->cs_control;
> +		return 0;
> +	}
> +
> +	if (gpio_is_valid(chip_info->gpio_cs)) {
> +		err = gpio_request(chip_info->gpio_cs, "SPI_CS");
> +		if (err) {
> +			dev_err(&spi->dev, "failed to request CS GPIO%d\n",
> +					chip_info->gpio_cs);

I do not agree that this is an error, though David may argue that it is.
 Feel free to reduce this to KERN_INFO and eliminate the following
return.  It is not illegal to omit the chip select on a one-chip bus,
when the chip does not use CS; there is no need to allocate and waste a
pin if it is not connected to anything.

> +			return err;
> +		}
> +
> +		chip->gpio_cs = chip_info->gpio_cs;
> +		chip->gpio_cs_inverted = spi->mode & SPI_CS_HIGH;
> +
> +		gpio_direction_output(chip->gpio_cs, !chip->gpio_cs_inverted);
> +	}
> +
> +	return err;
> +}
> +
>  static int setup(struct spi_device *spi)
>  {
>  	struct pxa2xx_spi_chip *chip_info = NULL;
> @@ -1141,7 +1203,7 @@ static int setup(struct spi_device *spi)
>  			return -ENOMEM;
>  		}
> 
> -		chip->cs_control = null_cs_control;

Based on cs_assert/cs_deassert, above, chip->cs_control needs to be
initialized to 0 here.  It's initialization should not be removed.

> +		chip->gpio_cs = -1;
>  		chip->enable_dma = 0;
>  		chip->timeout = 1000;
>  		chip->threshold = SSCR1_RxTresh(1) | SSCR1_TxTresh(1);
> @@ -1156,9 +1218,6 @@ static int setup(struct spi_device *spi)
>  	/* chip_info isn't always needed */
>  	chip->cr1 = 0;
>  	if (chip_info) {
> -		if (chip_info->cs_control)
> -			chip->cs_control = chip_info->cs_control;
> -

This should not be removed.  This is where legacy drivers establish the
pointer for a cs_control routine.

>  		chip->timeout = chip_info->timeout;
> 
>  		chip->threshold = (SSCR1_RxTresh(chip_info->rx_threshold) &
> @@ -1238,13 +1297,16 @@ static int setup(struct spi_device *spi)
> 
>  	spi_set_ctldata(spi, chip);
> 
> -	return 0;
> +	return setup_cs(spi, chip, chip_info);
>  }
> 
>  static void cleanup(struct spi_device *spi)
>  {
>  	struct chip_data *chip = spi_get_ctldata(spi);
> 
> +	if (gpio_is_valid(chip->gpio_cs))
> +		gpio_free(chip->gpio_cs);

Again, this does not belong in the master driver, IMHO; the resource
should have been allocated outside the master.

> +
>  	kfree(chip);
>  }
> 


-- 
Ned Forrester                                       nforrester-/d+BM93fTQY@public.gmane.org
Oceanographic Systems Lab                                  508-289-2226
Applied Ocean Physics and Engineering Dept.
Woods Hole Oceanographic Institution          Woods Hole, MA 02543, USA
http://www.whoi.edu/sbl/liteSite.do?litesiteid=7212
http://www.whoi.edu/hpb/Site.do?id=1532
http://www.whoi.edu/page.do?pid=10079


-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/

Re: [PATCH] spi: pxa2xx_spi introduce chip select gpio to simplify the common cases

[Eric Miao]

>> +static int setup_cs(struct spi_device *spi, struct chip_data *chip,
>> +                 struct pxa2xx_spi_chip *chip_info)
>
> My understanding is that it is legal to call setup without a defined
> pointer to a struct pxa2xx_spi_chip in spi_dev->controller_data, if the
> chip is happy with the defaults (only works for a single chip bus that
> needs no CS, a degenerate case, but still legal).  Thus you should allow
> for that by always checking for existence (chip_info not NULL) before use.
>

OK.

>> +{
>> +     int err = 0;
>> +
>> +     if (chip_info == NULL)
>> +             return 0;
>> +
>> +     /* NOTE: setup() can be called multiple times, possibly with
>> +      * different chip_info, previously requested GPIO shall be
>> +      * released, stumped :(
>> +      */
>
> That is why the GPIO allocation function belongs in the controller
> driver, and not in the master driver.  The master serves many
> controllers, and only the controller drivers know when they are loaded
> and unloaded, and thus when they need to allocate and initialize a GPIO
> pin and when to release it.  Additionally, on a multi-chip bus (which
> may have chips with CS assertion of differing polarity) it may be that
> only the board init code can configure all the CS pins to an idle state
> to get anything on the bus to work at all.  That is why cs_control
> shifts the burden of configuring CS pins outside of the master driver.
> I don't think that this function can work as written; perhaps this can
> be converted to a non-static helper function to be called by either
> board init or controller drivers.
>

This is because chip selects are usually done by GPIO in this case,
what about those SPI masters with a couple of CS(s) controlled
completely by the master registers?

>> +     if (gpio_is_valid(chip->gpio_cs))
>> +             gpio_free(chip->gpio_cs);
>> +
>> +     if (chip_info->cs_control) {
>> +             chip->cs_control = chip_info->cs_control;
>> +             return 0;
>> +     }
>> +
>> +     if (gpio_is_valid(chip_info->gpio_cs)) {
>> +             err = gpio_request(chip_info->gpio_cs, "SPI_CS");
>> +             if (err) {
>> +                     dev_err(&spi->dev, "failed to request CS GPIO%d\n",
>> +                                     chip_info->gpio_cs);
>
> I do not agree that this is an error, though David may argue that it is.
>  Feel free to reduce this to KERN_INFO and eliminate the following
> return.  It is not illegal to omit the chip select on a one-chip bus,
> when the chip does not use CS; there is no need to allocate and waste a
> pin if it is not connected to anything.

Well, if chip->gpio_cs is specified and not able to be requested, this
is obviously a serious error. If someone wants to use NULL CS,
chances are he will not make this "gpio_cs" valid.

>
>> +                     return err;
>> +             }
>> +
>> +             chip->gpio_cs = chip_info->gpio_cs;
>> +             chip->gpio_cs_inverted = spi->mode & SPI_CS_HIGH;
>> +
>> +             gpio_direction_output(chip->gpio_cs, !chip->gpio_cs_inverted);
>> +     }
>> +
>> +     return err;
>> +}
>> +
>>  static int setup(struct spi_device *spi)
>>  {
>>       struct pxa2xx_spi_chip *chip_info = NULL;
>> @@ -1141,7 +1203,7 @@ static int setup(struct spi_device *spi)
>>                       return -ENOMEM;
>>               }
>>
>> -             chip->cs_control = null_cs_control;
>
> Based on cs_assert/cs_deassert, above, chip->cs_control needs to be
> initialized to 0 here.  It's initialization should not be removed.

It's already been initialized to 0 by kzalloc(), explicitly.

>
>> +             chip->gpio_cs = -1;
>>               chip->enable_dma = 0;
>>               chip->timeout = 1000;
>>               chip->threshold = SSCR1_RxTresh(1) | SSCR1_TxTresh(1);
>> @@ -1156,9 +1218,6 @@ static int setup(struct spi_device *spi)
>>       /* chip_info isn't always needed */
>>       chip->cr1 = 0;
>>       if (chip_info) {
>> -             if (chip_info->cs_control)
>> -                     chip->cs_control = chip_info->cs_control;
>> -
>
> This should not be removed.  This is where legacy drivers establish the
> pointer for a cs_control routine.

This is done by setup_cs(), FYI.

-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/

Re: [PATCH] spi: pxa2xx_spi introduce chip select gpio to simplify the common cases

[Eric Miao]

On Tue, Sep 9, 2008 at 9:42 AM, Eric Miao <eric.y.miao-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>> +static int setup_cs(struct spi_device *spi, struct chip_data *chip,
>>> +                 struct pxa2xx_spi_chip *chip_info)
>>
>> My understanding is that it is legal to call setup without a defined
>> pointer to a struct pxa2xx_spi_chip in spi_dev->controller_data, if the
>> chip is happy with the defaults (only works for a single chip bus that
>> needs no CS, a degenerate case, but still legal).  Thus you should allow
>> for that by always checking for existence (chip_info not NULL) before use.
>>

Ah, checked again with the source, I don't think setup_cs()
shall continue if (chip_info == NULL), which implies a
null_cs_control(), and is now handled by cs_assert() and
cs_deassert().

-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/

Re: [PATCH] spi: pxa2xx_spi introduce chip select gpio to simplify the common cases

[Ned Forrester]

Eric Miao wrote:
>>> +static int setup_cs(struct spi_device *spi, struct chip_data *chip,
>>> +                 struct pxa2xx_spi_chip *chip_info)
>> My understanding is that it is legal to call setup without a defined
>> pointer to a struct pxa2xx_spi_chip in spi_dev->controller_data, if the
>> chip is happy with the defaults (only works for a single chip bus that
>> needs no CS, a degenerate case, but still legal).  Thus you should allow
>> for that by always checking for existence (chip_info not NULL) before use.
>>
> 
> OK.
> 
>>> +{
>>> +     int err = 0;
>>> +
>>> +     if (chip_info == NULL)
>>> +             return 0;
>>> +
>>> +     /* NOTE: setup() can be called multiple times, possibly with
>>> +      * different chip_info, previously requested GPIO shall be
>>> +      * released, stumped :(
>>> +      */
>> That is why the GPIO allocation function belongs in the controller
>> driver, and not in the master driver.  The master serves many
>> controllers, and only the controller drivers know when they are loaded
>> and unloaded, and thus when they need to allocate and initialize a GPIO
>> pin and when to release it.  Additionally, on a multi-chip bus (which
>> may have chips with CS assertion of differing polarity) it may be that
>> only the board init code can configure all the CS pins to an idle state
>> to get anything on the bus to work at all.  That is why cs_control
>> shifts the burden of configuring CS pins outside of the master driver.
>> I don't think that this function can work as written; perhaps this can
>> be converted to a non-static helper function to be called by either
>> board init or controller drivers.
>>
> 
> This is because chip selects are usually done by GPIO in this case,
> what about those SPI masters with a couple of CS(s) controlled
> completely by the master registers?

ON MORE CAREFUL THOUGHT (sorry): I missed something; I was thrown off
track by your original "stumped" comment, above.  I thought your
original concern was that you could only maintain one gpio_cs at a time,
and that is what cannot work.  I forgot that you are maintaining one
gpio_cs per chip, which is right, and is the way cs_control is managed.

In light of that, it seems like your original "stumped" is easily dealt
with by only freeing chip->gpio_cs if it does not match the new GPIO value:

	if (chip->gpio_cs != chip_info->gpio_cs
		|| !chip_info->cs_control)
			if (gpio_is_valid(chip->gpio_cs))
				gpio_free(chip->gpio_cs);

to replace the next two lines.  Of course, the two ifs can be combined
if the call to gpio_is_valid is not expensive.

By the way, I am not familiar with these gpio_ calls.  Is gpio_is_valid
really the right call here, or is there something like
gpio_is_allocated.  Since you call gpio_is_valid on an unallocated GPIO,
below, the call would succeed on an unallocated GPIO, and I wonder if
freeing an unallocated GPIO is legal.

>>> +     if (gpio_is_valid(chip->gpio_cs))
>>> +             gpio_free(chip->gpio_cs);
>>> +
>>> +     if (chip_info->cs_control) {
>>> +             chip->cs_control = chip_info->cs_control;
>>> +             return 0;
>>> +     }
>>> +
>>> +     if (gpio_is_valid(chip_info->gpio_cs)) {
>>> +             err = gpio_request(chip_info->gpio_cs, "SPI_CS");
>>> +             if (err) {
>>> +                     dev_err(&spi->dev, "failed to request CS GPIO%d\n",
>>> +                                     chip_info->gpio_cs);
>> I do not agree that this is an error, though David may argue that it is.
>>  Feel free to reduce this to KERN_INFO and eliminate the following
>> return.  It is not illegal to omit the chip select on a one-chip bus,
>> when the chip does not use CS; there is no need to allocate and waste a
>> pin if it is not connected to anything.
> 
> Well, if chip->gpio_cs is specified and not able to be requested, this
> is obviously a serious error. If someone wants to use NULL CS,
> chances are he will not make this "gpio_cs" valid.

Oops.  What I was thinking is that it is not an error for
chip_info->gpio_cs to remain in its initialized state of -1.  You are
right, of course, that to request a specific pin that can't be allocated
is an error.  And it is too late at night, so I missed the logic that
err remains zero if gpio_is_valid fails.  Sorry.

>>> +                     return err;
>>> +             }
>>> +
>>> +             chip->gpio_cs = chip_info->gpio_cs;
>>> +             chip->gpio_cs_inverted = spi->mode & SPI_CS_HIGH;
>>> +
>>> +             gpio_direction_output(chip->gpio_cs, !chip->gpio_cs_inverted);
>>> +     }
>>> +
>>> +     return err;
>>> +}
>>> +
>>>  static int setup(struct spi_device *spi)
>>>  {
>>>       struct pxa2xx_spi_chip *chip_info = NULL;
>>> @@ -1141,7 +1203,7 @@ static int setup(struct spi_device *spi)
>>>                       return -ENOMEM;
>>>               }
>>>
>>> -             chip->cs_control = null_cs_control;
>> Based on cs_assert/cs_deassert, above, chip->cs_control needs to be
>> initialized to 0 here.  It's initialization should not be removed.
> 
> It's already been initialized to 0 by kzalloc(), explicitly.

Oops, I keep forgetting that.  I was looking at the auto declaration.

> 
>>> +             chip->gpio_cs = -1;
>>>               chip->enable_dma = 0;
>>>               chip->timeout = 1000;
>>>               chip->threshold = SSCR1_RxTresh(1) | SSCR1_TxTresh(1);
>>> @@ -1156,9 +1218,6 @@ static int setup(struct spi_device *spi)
>>>       /* chip_info isn't always needed */
>>>       chip->cr1 = 0;
>>>       if (chip_info) {
>>> -             if (chip_info->cs_control)
>>> -                     chip->cs_control = chip_info->cs_control;
>>> -
>> This should not be removed.  This is where legacy drivers establish the
>> pointer for a cs_control routine.
> 
> This is done by setup_cs(), FYI.

I guess that is right, too.  I did miss that, but in the back of my mind
I was thinking that you will not end up calling setup_cs() from the
setup() routine (because I was arguing that GPIO allocation does not
belong here).

So now I am whittled down to a suggestion for your "stumped" comment.

Clearly, I should have left this post for tomorrow. :-)

-- 
Ned Forrester                                       nforrester-/d+BM93fTQY@public.gmane.org
Oceanographic Systems Lab                                  508-289-2226
Applied Ocean Physics and Engineering Dept.
Woods Hole Oceanographic Institution          Woods Hole, MA 02543, USA
http://www.whoi.edu/sbl/liteSite.do?litesiteid=7212
http://www.whoi.edu/hpb/Site.do?id=1532
http://www.whoi.edu/page.do?pid=10079


-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/

Re: [PATCH] spi: pxa2xx_spi introduce chip select gpio to simplify the common cases

[Ned Forrester]

Eric Miao wrote:
> On Tue, Sep 9, 2008 at 9:42 AM, Eric Miao <eric.y.miao-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>>> +static int setup_cs(struct spi_device *spi, struct chip_data *chip,
>>>> +                 struct pxa2xx_spi_chip *chip_info)
>>> My understanding is that it is legal to call setup without a defined
>>> pointer to a struct pxa2xx_spi_chip in spi_dev->controller_data, if the
>>> chip is happy with the defaults (only works for a single chip bus that
>>> needs no CS, a degenerate case, but still legal).  Thus you should allow
>>> for that by always checking for existence (chip_info not NULL) before use.
>>>
> 
> Ah, checked again with the source, I don't think setup_cs()
> shall continue if (chip_info == NULL), which implies a
> null_cs_control(), and is now handled by cs_assert() and
> cs_deassert().

Yea, I already noticed that.  Oh well.

-- 
Ned Forrester                                       nforrester-/d+BM93fTQY@public.gmane.org
Oceanographic Systems Lab                                  508-289-2226
Applied Ocean Physics and Engineering Dept.
Woods Hole Oceanographic Institution          Woods Hole, MA 02543, USA
http://www.whoi.edu/sbl/liteSite.do?litesiteid=7212
http://www.whoi.edu/hpb/Site.do?id=1532
http://www.whoi.edu/page.do?pid=10079


-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/

Re: [PATCH] spi: pxa2xx_spi introduce chip select gpio to simplify the common cases

[Eric Miao]

On Tue, Sep 9, 2008 at 10:50 AM, Ned Forrester <nforrester-/d+BM93fTQY@public.gmane.org> wrote:
> Eric Miao wrote:
>>>> +static int setup_cs(struct spi_device *spi, struct chip_data *chip,
>>>> +                 struct pxa2xx_spi_chip *chip_info)
>>> My understanding is that it is legal to call setup without a defined
>>> pointer to a struct pxa2xx_spi_chip in spi_dev->controller_data, if the
>>> chip is happy with the defaults (only works for a single chip bus that
>>> needs no CS, a degenerate case, but still legal).  Thus you should allow
>>> for that by always checking for existence (chip_info not NULL) before use.
>>>
>>
>> OK.
>>
>>>> +{
>>>> +     int err = 0;
>>>> +
>>>> +     if (chip_info == NULL)
>>>> +             return 0;
>>>> +
>>>> +     /* NOTE: setup() can be called multiple times, possibly with
>>>> +      * different chip_info, previously requested GPIO shall be
>>>> +      * released, stumped :(
>>>> +      */
>>> That is why the GPIO allocation function belongs in the controller
>>> driver, and not in the master driver.  The master serves many
>>> controllers, and only the controller drivers know when they are loaded
>>> and unloaded, and thus when they need to allocate and initialize a GPIO
>>> pin and when to release it.  Additionally, on a multi-chip bus (which
>>> may have chips with CS assertion of differing polarity) it may be that
>>> only the board init code can configure all the CS pins to an idle state
>>> to get anything on the bus to work at all.  That is why cs_control
>>> shifts the burden of configuring CS pins outside of the master driver.
>>> I don't think that this function can work as written; perhaps this can
>>> be converted to a non-static helper function to be called by either
>>> board init or controller drivers.
>>>
>>
>> This is because chip selects are usually done by GPIO in this case,
>> what about those SPI masters with a couple of CS(s) controlled
>> completely by the master registers?
>
> ON MORE CAREFUL THOUGHT (sorry): I missed something; I was thrown off
> track by your original "stumped" comment, above.  I thought your
> original concern was that you could only maintain one gpio_cs at a time,
> and that is what cannot work.  I forgot that you are maintaining one
> gpio_cs per chip, which is right, and is the way cs_control is managed.
>
> In light of that, it seems like your original "stumped" is easily dealt
> with by only freeing chip->gpio_cs if it does not match the new GPIO value:
>
>        if (chip->gpio_cs != chip_info->gpio_cs
>                || !chip_info->cs_control)
>                        if (gpio_is_valid(chip->gpio_cs))
>                                gpio_free(chip->gpio_cs);
>
> to replace the next two lines.  Of course, the two ifs can be combined
> if the call to gpio_is_valid is not expensive.
>

Yes, this will probably save another several calls to gpio_request()
and gpio_free() if the GPIO hasn't changed between two setup().

My original patch is going to free the previously requested GPIO
anyway, which I think is acceptable since no existing drivers is
going to call setup() many times during its lifetime.

> By the way, I am not familiar with these gpio_ calls.  Is gpio_is_valid
> really the right call here, or is there something like
> gpio_is_allocated.  Since you call gpio_is_valid on an unallocated GPIO,
> below, the call would succeed on an unallocated GPIO, and I wonder if
> freeing an unallocated GPIO is legal.
>

I'd really like to use if (chip->gpio_cs) then ...., but unfortunately no,
simply because "0" is a valid GPIO :(

The gpio_is_valid() takes care of this (hopefully :), and the reason
gpio_is_allocated() or gpio_is_requested() is not used here because
we need to make sure the GPIO is valid _before_ requesting it.

>>>> +     if (gpio_is_valid(chip->gpio_cs))
>>>> +             gpio_free(chip->gpio_cs);
>>>> +
>>>> +     if (chip_info->cs_control) {
>>>> +             chip->cs_control = chip_info->cs_control;
>>>> +             return 0;
>>>> +     }
>>>> +
>>>> +     if (gpio_is_valid(chip_info->gpio_cs)) {
>>>> +             err = gpio_request(chip_info->gpio_cs, "SPI_CS");
>>>> +             if (err) {
>>>> +                     dev_err(&spi->dev, "failed to request CS GPIO%d\n",
>>>> +                                     chip_info->gpio_cs);
>>> I do not agree that this is an error, though David may argue that it is.
>>>  Feel free to reduce this to KERN_INFO and eliminate the following
>>> return.  It is not illegal to omit the chip select on a one-chip bus,
>>> when the chip does not use CS; there is no need to allocate and waste a
>>> pin if it is not connected to anything.
>>
>> Well, if chip->gpio_cs is specified and not able to be requested, this
>> is obviously a serious error. If someone wants to use NULL CS,
>> chances are he will not make this "gpio_cs" valid.
>
> Oops.  What I was thinking is that it is not an error for
> chip_info->gpio_cs to remain in its initialized state of -1.  You are
> right, of course, that to request a specific pin that can't be allocated
> is an error.  And it is too late at night, so I missed the logic that
> err remains zero if gpio_is_valid fails.  Sorry.
>
>>>> +                     return err;
>>>> +             }
>>>> +
>>>> +             chip->gpio_cs = chip_info->gpio_cs;
>>>> +             chip->gpio_cs_inverted = spi->mode & SPI_CS_HIGH;
>>>> +
>>>> +             gpio_direction_output(chip->gpio_cs, !chip->gpio_cs_inverted);
>>>> +     }
>>>> +
>>>> +     return err;
>>>> +}
>>>> +
>>>>  static int setup(struct spi_device *spi)
>>>>  {
>>>>       struct pxa2xx_spi_chip *chip_info = NULL;
>>>> @@ -1141,7 +1203,7 @@ static int setup(struct spi_device *spi)
>>>>                       return -ENOMEM;
>>>>               }
>>>>
>>>> -             chip->cs_control = null_cs_control;
>>> Based on cs_assert/cs_deassert, above, chip->cs_control needs to be
>>> initialized to 0 here.  It's initialization should not be removed.
>>
>> It's already been initialized to 0 by kzalloc(), explicitly.
>
> Oops, I keep forgetting that.  I was looking at the auto declaration.
>
>>
>>>> +             chip->gpio_cs = -1;
>>>>               chip->enable_dma = 0;
>>>>               chip->timeout = 1000;
>>>>               chip->threshold = SSCR1_RxTresh(1) | SSCR1_TxTresh(1);
>>>> @@ -1156,9 +1218,6 @@ static int setup(struct spi_device *spi)
>>>>       /* chip_info isn't always needed */
>>>>       chip->cr1 = 0;
>>>>       if (chip_info) {
>>>> -             if (chip_info->cs_control)
>>>> -                     chip->cs_control = chip_info->cs_control;
>>>> -
>>> This should not be removed.  This is where legacy drivers establish the
>>> pointer for a cs_control routine.
>>
>> This is done by setup_cs(), FYI.
>
> I guess that is right, too.  I did miss that, but in the back of my mind
> I was thinking that you will not end up calling setup_cs() from the
> setup() routine (because I was arguing that GPIO allocation does not
> belong here).

Yes, I expect there will be structural change to the SPI core code
itself, to have a fine-grained separation of one-time configurations
and run-time ones (which could be changed multiple times).

>
> So now I am whittled down to a suggestion for your "stumped" comment.
>
> Clearly, I should have left this post for tomorrow. :-)

Well, comments are always welcome. Others usually
see what I cannot :)

-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/

Re: [PATCH] spi: pxa2xx_spi introduce chip select gpio to simplify the common cases

[Jonathan Cameron]

Dear David,
> Related:
> 
>  - It's probably worth removing support for the SSFRM
>    mechanism and requiring gpio_cs or (at least as a
>    transitional scheme) the cs_control() thing.
I disagree strongly with this.

By all means issue a warning that cs_change will be
effectively 1 in all cases. However, with many real setups
this is absolutely fine. Leave getting this right to a
combination of the board config writers and some good
documentation!

>    I suggest issuing a strong warning for all devices
>    which assume the SSFRM mechanism (single device
>    on the bus segment too) ... and maybe a weaker one
>    if they use cs_control, saying "use gpio_cs".
> 
>  - Remove the gpio_cs_inverted thing ... that's exactly
>    what SPI_CS_HIGH is there to handle (in spi->mode).
> 
> The SSFRM issue is, briefly, that it forces deselect
> of the chip between transfer segments even when the
> driver doesn't want that.  If it's not disabled, the
> transfer() routine needs integrity checks to make
> sure it rejects all messages that don't expect that
> deselection (unless cs_control/gpio_cs is used for
> that particular spi_device).
> 
> The cs_control issue is more just simplification:  as
> I recall, all users are just wrapping GPIOS, and giving
> up potential SPI_CS_HIGH support while doing it.
Whilst no one may currently be doing anything other than
wrapping a gpio, are there really no foreseeable reasons
to do it another way?

One classic possibility that comes to mind is some chips are
quite happy not to have the chip select switched at all. 
Obviously this is only sensible on single chip connected
to the bus situations, but then for high performance stuff
that's all that ever seems to happen anyway!

--
Jonathan Cameron
> 
> 
> ============
> From: "Eric Miao" <eric.y.miao-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> 
> Most SPI peripherals use GPIOs as their chip select, introduce .gpio_cs
> for this, to avoid 
> 
> Signed-off-by: Eric Miao <eric.miao-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>
> ---
>  arch/arm/mach-pxa/include/mach/pxa2xx_spi.h |    1 +
>  drivers/spi/pxa2xx_spi.c                    |   92 ++++++++++++++++++++++-----
>  2 files changed, 78 insertions(+), 15 deletions(-)
> 
> diff --git a/arch/arm/mach-pxa/include/mach/pxa2xx_spi.h
> b/arch/arm/mach-pxa/include/mach/pxa2xx_spi.h
> index 2206cb6..b87cecd 100644
> --- a/arch/arm/mach-pxa/include/mach/pxa2xx_spi.h
> +++ b/arch/arm/mach-pxa/include/mach/pxa2xx_spi.h
> @@ -38,6 +38,7 @@ struct pxa2xx_spi_chip {
>  	u8 dma_burst_size;
>  	u32 timeout;
>  	u8 enable_loopback;
> +	int gpio_cs;
>  	void (*cs_control)(u32 command);
>  };
> 
> diff --git a/drivers/spi/pxa2xx_spi.c b/drivers/spi/pxa2xx_spi.c
> index 34c7c98..98d2338 100644
> --- a/drivers/spi/pxa2xx_spi.c
> +++ b/drivers/spi/pxa2xx_spi.c
> @@ -28,6 +28,7 @@
>  #include <linux/workqueue.h>
>  #include <linux/delay.h>
>  #include <linux/clk.h>
> +#include <linux/gpio.h>
> 
>  #include <asm/io.h>
>  #include <asm/irq.h>
> @@ -164,6 +165,8 @@ struct chip_data {
>  	u8 enable_dma;
>  	u8 bits_per_word;
>  	u32 speed_hz;
> +	int gpio_cs;
> +	unsigned gpio_cs_inverted : 1;
>  	int (*write)(struct driver_data *drv_data);
>  	int (*read)(struct driver_data *drv_data);
>  	void (*cs_control)(u32 command);
> @@ -171,6 +174,32 @@ struct chip_data {
> 
>  static void pump_messages(struct work_struct *work);
> 
> +static void cs_assert(struct driver_data *drv_data)
> +{
> +	struct chip_data *chip = drv_data->cur_chip;
> +
> +	if (chip->cs_control) {
> +		chip->cs_control(PXA2XX_CS_ASSERT);
> +		return;
> +	}
> +
> +	if (gpio_is_valid(chip->gpio_cs))
> +		gpio_set_value(chip->gpio_cs, chip->gpio_cs_inverted);
> +}
> +
> +static void cs_deassert(struct driver_data *drv_data)
> +{
> +	struct chip_data *chip = drv_data->cur_chip;
> +
> +	if (chip->cs_control) {
> +		chip->cs_control(PXA2XX_CS_DEASSERT);
> +		return;
> +	}
> +
> +	if (gpio_is_valid(chip->gpio_cs))
> +		gpio_set_value(chip->gpio_cs, !chip->gpio_cs_inverted);
> +}
> +
>  static int flush(struct driver_data *drv_data)
>  {
>  	unsigned long limit = loops_per_jiffy << 1;
> @@ -187,10 +216,6 @@ static int flush(struct driver_data *drv_data)
>  	return limit;
>  }
> 
> -static void null_cs_control(u32 command)
> -{
> -}
> -
>  static int null_writer(struct driver_data *drv_data)
>  {
>  	void __iomem *reg = drv_data->ioaddr;
> @@ -398,7 +423,6 @@ static void giveback(struct driver_data *drv_data)
>  	msg = drv_data->cur_msg;
>  	drv_data->cur_msg = NULL;
>  	drv_data->cur_transfer = NULL;
> -	drv_data->cur_chip = NULL;
>  	queue_work(drv_data->workqueue, &drv_data->pump_messages);
>  	spin_unlock_irqrestore(&drv_data->lock, flags);
> 
> @@ -407,7 +431,9 @@ static void giveback(struct driver_data *drv_data)
>  					transfer_list);
> 
>  	if (!last_transfer->cs_change)
> -		drv_data->cs_control(PXA2XX_CS_DEASSERT);
> +		cs_deassert(drv_data);
> +
> +	drv_data->cur_chip = NULL;
> 
>  	msg->state = NULL;
>  	if (msg->complete)
> @@ -493,7 +519,7 @@ static void dma_transfer_complete(struct driver_data *drv_data)
>  	/* Release chip select if requested, transfer delays are
>  	 * handled in pump_transfers */
>  	if (drv_data->cs_change)
> -		drv_data->cs_control(PXA2XX_CS_DEASSERT);
> +		cs_deassert(drv_data);
> 
>  	/* Move to next transfer */
>  	msg->state = next_transfer(drv_data);
> @@ -605,7 +631,7 @@ static void int_transfer_complete(struct driver_data *drv_data)
>  	/* Release chip select if requested, transfer delays are
>  	 * handled in pump_transfers */
>  	if (drv_data->cs_change)
> -		drv_data->cs_control(PXA2XX_CS_DEASSERT);
> +		cs_deassert(drv_data);
> 
>  	/* Move to next transfer */
>  	drv_data->cur_msg->state = next_transfer(drv_data);
> @@ -868,7 +894,6 @@ static void pump_transfers(unsigned long data)
>  	}
>  	drv_data->n_bytes = chip->n_bytes;
>  	drv_data->dma_width = chip->dma_width;
> -	drv_data->cs_control = chip->cs_control;
>  	drv_data->tx = (void *)transfer->tx_buf;
>  	drv_data->tx_end = drv_data->tx + transfer->len;
>  	drv_data->rx = transfer->rx_buf;
> @@ -1020,7 +1045,7 @@ static void pump_transfers(unsigned long data)
>  	 * this driver uses struct pxa2xx_spi_chip.cs_control to
>  	 * specify a CS handling function, and it ignores most
>  	 * struct spi_device.mode[s], including SPI_CS_HIGH */
> -	drv_data->cs_control(PXA2XX_CS_ASSERT);
> +	cs_assert(drv_data);
> 
>  	/* after chip select, release the data by enabling service
>  	 * requests and interrupts, without changing any mode bits */
> @@ -1098,6 +1123,43 @@ static int transfer(struct spi_device *spi, struct spi_message *msg)
>  /* the spi->mode bits understood by this driver: */
>  #define MODEBITS (SPI_CPOL | SPI_CPHA)
> 
> +static int setup_cs(struct spi_device *spi, struct chip_data *chip,
> +		    struct pxa2xx_spi_chip *chip_info)
> +{
> +	int err = 0;
> +	
> +	if (chip_info == NULL)
> +		return 0;
> +
> +	/* NOTE: setup() can be called multiple times, possibly with
> +	 * different chip_info, previously requested GPIO shall be
> +	 * released, stumped :(
> +	 */
> +	if (gpio_is_valid(chip->gpio_cs))
> +		gpio_free(chip->gpio_cs);
> +
> +	if (chip_info->cs_control) {
> +		chip->cs_control = chip_info->cs_control;
> +		return 0;
> +	}
> +
> +	if (gpio_is_valid(chip_info->gpio_cs)) {
> +		err = gpio_request(chip_info->gpio_cs, "SPI_CS");
> +		if (err) {
> +			dev_err(&spi->dev, "failed to request CS GPIO%d\n",
> +					chip_info->gpio_cs);
> +			return err;
> +		}
> +
> +		chip->gpio_cs = chip_info->gpio_cs;
> +		chip->gpio_cs_inverted = spi->mode & SPI_CS_HIGH;
> +
> +		gpio_direction_output(chip->gpio_cs, !chip->gpio_cs_inverted);
> +	}
> +
> +	return err;
> +}
> +
>  static int setup(struct spi_device *spi)
>  {
>  	struct pxa2xx_spi_chip *chip_info = NULL;
> @@ -1141,7 +1203,7 @@ static int setup(struct spi_device *spi)
>  			return -ENOMEM;
>  		}
> 
> -		chip->cs_control = null_cs_control;
> +		chip->gpio_cs = -1;
>  		chip->enable_dma = 0;
>  		chip->timeout = 1000;
>  		chip->threshold = SSCR1_RxTresh(1) | SSCR1_TxTresh(1);
> @@ -1156,9 +1218,6 @@ static int setup(struct spi_device *spi)
>  	/* chip_info isn't always needed */
>  	chip->cr1 = 0;
>  	if (chip_info) {
> -		if (chip_info->cs_control)
> -			chip->cs_control = chip_info->cs_control;
> -
>  		chip->timeout = chip_info->timeout;
> 
>  		chip->threshold = (SSCR1_RxTresh(chip_info->rx_threshold) &
> @@ -1238,13 +1297,16 @@ static int setup(struct spi_device *spi)
> 
>  	spi_set_ctldata(spi, chip);
> 
> -	return 0;
> +	return setup_cs(spi, chip, chip_info);
>  }
> 
>  static void cleanup(struct spi_device *spi)
>  {
>  	struct chip_data *chip = spi_get_ctldata(spi);
> 
> +	if (gpio_is_valid(chip->gpio_cs))
> +		gpio_free(chip->gpio_cs);
> +
>  	kfree(chip);
>  }
> 


-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/

Re: [PATCH] spi: pxa2xx_spi introduce chip select gpio to simplify the common cases

[David Brownell]

> > +     /* NOTE: setup() can be called multiple times, possibly with
> > +      * different chip_info, previously requested GPIO shall be
> > +      * released, stumped :(
> > +      */

No; chip_info should be regarded as immutable between the time the
initializing setup() call is invoked and, should it ever happen,
the cleanup() is invoked.  Anyone mucking around with that controller
data deserves the chaos they will have started.


> > +     if (gpio_is_valid(chip_info->gpio_cs)) {
> > +             err = gpio_request(chip_info->gpio_cs, "SPI_CS");

I'd actually pass dev_name(&spi->dev) not "SPI_CS", since I
happen to like seeing /sys/kernel/debug/gpio be informative.

Seeing half a dozen "SPI_CS" (uppercase, yeech!) labels is
not informative; seeing "spi1.0", "spi1.1", "spi2.5" etc is
more useful ... you can probably cross-check them with the
board schematics to detect errors, instead of just omissions.


> > +             if (err) {
> > +                     dev_err(&spi->dev, "failed to request CS GPIO%d\n",
> > +                                     chip_info->gpio_cs);
> 
> I do not agree that this is an error, though David may argue that it is.

It's an error all right.  If that signal is in use by some
other driver, this one should never touch it.


>  Feel free to reduce this to KERN_INFO and eliminate the following
> return.  It is not illegal to omit the chip select on a one-chip bus,
> when the chip does not use CS; there is no need to allocate and waste a
> pin if it is not connected to anything.

But such "not illegal" cases wouldn't hit this "if (err) ... " branch...


> > +     if (gpio_is_valid(chip->gpio_cs))
> > +             gpio_free(chip->gpio_cs);
>
> Again, this does not belong in the master driver, IMHO; the resource
> should have been allocated outside the master.

No; drivers do request_resource(), and gpio_request() is the same
kind of thing.  And they need to clean up the resources which they
requested on various paths -- even when those resources are GPIOs.

Of course, if the cs_control() route is used, then pxa2xx_spi isn't
going to be allocating any underlying GPIOs, so it won't have to
free them either.

- Dave


-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/

Re: [PATCH] spi: pxa2xx_spi introduce chip select gpio to simplify the common cases

[David Brownell]

On Monday 08 September 2008, Eric Miao wrote:
> My original patch is going to free the previously requested GPIO
> anyway, which I think is acceptable since no existing drivers is
> going to call setup() many times during its lifetime.

That seems like a bad assumption to make.  :)



-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/

Re: [PATCH] spi: pxa2xx_spi introduce chip select gpio to simplify the common cases

[Eric Miao]

>> > +     if (gpio_is_valid(chip_info->gpio_cs)) {
>> > +             err = gpio_request(chip_info->gpio_cs, "SPI_CS");
>
> I'd actually pass dev_name(&spi->dev) not "SPI_CS", since I
> happen to like seeing /sys/kernel/debug/gpio be informative.
>

Good suggestion, indeed. I'll include this in the next submission.

-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/

Re: [PATCH] spi: pxa2xx_spi introduce chip select gpio to simplify the common cases

[David Brownell]

On Tuesday 09 September 2008, Jonathan Cameron wrote:
> 
> >  - It's probably worth removing support for the SSFRM
> >    mechanism and requiring gpio_cs or (at least as a
> >    transitional scheme) the cs_control() thing.
>
> I disagree strongly with this.
> 
> By all means issue a warning that cs_change will be
> effectively 1 in all cases. However, with many real setups
> this is absolutely fine. Leave getting this right to a
> combination of the board config writers and some good
> documentation!

My logic is as follows:

  (a) it's superfluous given that the pin used for
      SSFRM can be managed as a GPIO;
  (b) it's less capable than managing it as a GPIO,
      examples being lack of SPI_CS_HIGH support
      and broken cs_change;
  (c) given (b) and what you noted, it's error prone;
  (d) given (a-c) it's confusing

In short, removing this mechanism would improve quality
and maintainability.  Needing *more* documentation for
core functionality is a red flag.

Notice I'm not saying it can't (or doesn't) work, within
its constraints (b).  I'm just saying that if it's removed,
the driver will be better and won't lose *ANY* functionality.

- Dave


-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/

Re: [PATCH] spi: pxa2xx_spi introduce chip select gpio to simplify the common cases

[Ned Forrester]

David Brownell wrote:
> On Tuesday 09 September 2008, Jonathan Cameron wrote:
>>>  - It's probably worth removing support for the SSFRM
>>>    mechanism and requiring gpio_cs or (at least as a
>>>    transitional scheme) the cs_control() thing.
>> I disagree strongly with this.
>>
>> By all means issue a warning that cs_change will be
>> effectively 1 in all cases. However, with many real setups
>> this is absolutely fine. Leave getting this right to a
>> combination of the board config writers and some good
>> documentation!
> 
> My logic is as follows:
> 
>   (a) it's superfluous given that the pin used for
>       SSFRM can be managed as a GPIO;
>   (b) it's less capable than managing it as a GPIO,
>       examples being lack of SPI_CS_HIGH support
>       and broken cs_change;
>   (c) given (b) and what you noted, it's error prone;
>   (d) given (a-c) it's confusing
> 
> In short, removing this mechanism would improve quality
> and maintainability.  Needing *more* documentation for
> core functionality is a red flag.
> 
> Notice I'm not saying it can't (or doesn't) work, within
> its constraints (b).  I'm just saying that if it's removed,
> the driver will be better and won't lose *ANY* functionality.

I'm puzzled.  What is there about the pxa2xx_spi.c that would be
simplified by doing anything with respect to SSPFRM?  As I said in a
previous email: there is no "support" for SSPFRM in the driver, other
than quietly doing nothing if no CS action has been specified.  Maybe
you are thinking simplification elsewhere, like in board init code.

I agree that SSPFRM can be controlled as a GPIO line via the gpio_cs or
cs_control methods, but I don't see any "cost" associated with leaving
things as they are.  If use of gpio_cs were enforced, an existing
application that uses the SSPFRM function of the PXA2xx chip would have
to be carefully checked for proper use of cs_change (cs_change would
have been previously ineffective).  Like I said recently, I have a
narrow view of all this, so likely I am missing something obvious. :-)

On a related aspect:

By "broken" cs_change, are you referring to the lack of support for
SPI_CS_HIGH?  That could easily be handled the same way that Eric is
proposing for cs_assert/cs_deassert: by passing SPI_CS_HIGH, either
directly or indirectly, to cs_control (or, to avoid breaking existing
cs_control routines, to a new routine that users would be encouraged to
use).

I see little difference between the proposed gpio_cs and the existing
cs_control mechanisms, except that gpio_cs constrains the user to using
a gpio pin, while the cs_control callback allows the user to implement
any scheme for chip select.  While I know of no one doing this, it is
quite conceivable that a board could use an external expansion chip to
provide the physical pins used for chip select; in that case, cs_control
allows whatever code is required to to be executed to effect a CS
change, while the gpio_cs scheme does not.

Certainly, the gpio_cs mechanism reduces the burden of GPIO setup to
passing a single integer (the pin number, I assume), and this will be
sufficient for most boards, but it seems unnecessarily restrictive, if
it is the only mechanism.

-- 
Ned Forrester                                       nforrester-/d+BM93fTQY@public.gmane.org
Oceanographic Systems Lab                                  508-289-2226
Applied Ocean Physics and Engineering Dept.
Woods Hole Oceanographic Institution          Woods Hole, MA 02543, USA
http://www.whoi.edu/sbl/liteSite.do?litesiteid=7212
http://www.whoi.edu/hpb/Site.do?id=1532
http://www.whoi.edu/page.do?pid=10079


-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/

Re: [PATCH] spi: pxa2xx_spi introduce chip select gpio to simplify the common cases

[David Brownell]

On Thursday 11 September 2008, Ned Forrester wrote:

> I'm puzzled.  What is there about the pxa2xx_spi.c that would be
> simplified by doing anything with respect to SSPFRM?

The usage model ... callers must understand that option,
evaluate whether it's even usable on their board, and
then have an extra set of possibilities to debug.  And
it's not a model needed with other SPI controllers...

The code ... has to support that more complex model.

Developers ... have to answer questions about that
mechanism when they could be doing more useful things.


> If use of gpio_cs were enforced, an existing 
> application that uses the SSPFRM function of the PXA2xx chip would have
> to be carefully checked for proper use of cs_change (cs_change would
> have been previously ineffective). 

Needing to fix latent bugs is a weak argument.  They'd
need to be fixed when moving to other hardware in any
case.  Note that needing such fixage is another cost
to allowing this mechanism...


> On a related aspect:
> 
> By "broken" cs_change, are you referring to the lack of support for
> SPI_CS_HIGH?

No; spi_transfer.cs_change is what controls whether chipselect
should be deactivated (briefly) between transfers.  It's never
supposed to be deactivated except between messages ... unless
that flag is set.

The brokenness is that when using the SSP frame signal as
chipselect, it's *always* disabled between transfers.

- Dave

-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/

Re: [PATCH] spi: pxa2xx_spi introduce chip select gpio to simplify the common cases

[Jonathan Cameron]

David Brownell wrote:
> On Thursday 11 September 2008, Ned Forrester wrote:
> 
>> I'm puzzled.  What is there about the pxa2xx_spi.c that would be
>> simplified by doing anything with respect to SSPFRM?
> 
> The usage model ... callers must understand that option,
> evaluate whether it's even usable on their board, and
> then have an extra set of possibilities to debug.  And
> it's not a model needed with other SPI controllers...
> 
> The code ... has to support that more complex model.
> 
> Developers ... have to answer questions about that
> mechanism when they could be doing more useful things.

Having converted the boards I use over to a gpio model
(was all sfrm based before) I agree that this is probably 
the way to go.

There are a few use cases though where using the hardware
support (though broken as it is) would give higher performance.

For some devices it is necessary to deselect them for
between transfers for an amount of time equal to say 
4 times the SCK period.  Clearly the only way to
make this happen on various spi controllers is to 
set mdelay_usecs to the best value you can manage.
(at high speed this is somewhat tedious, and in most cases
the time to turn the gpio off and on again is sufficient)

However, there is always a fixed min delay between the 
transfers due to the passing of interrupts back to the
driver etc so in many cases the use of SFRM allows
this to be harnessed as it switches much closer to the
actual transmission than a gpio based control. 

As an example, take the vti sca3000 range of accelerometers.
The specs say that they must be deselected for 4 * the SCK
period.  As some of these devices go at 325kHz this comes
to 12.3 microsecs and a typical time I've observed for gpio
deselecting is only 7 microsecs this would be a device that
would benefit from such strategy (as long as the board
config had the option of specifying a minimum delay between
SRFM being deselected and control hitting the delay_usecs
code in pxa2xx_spi.c.

Still, despite the above being the case, I'm inclined
to agree that at the very least people should be strongly
discouraged from using SRFM!

Jonathan

-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/