[PATCH 1/2] spi/bfin_spi: only request GPIO on first load

[PATCH 1/2] spi/bfin_spi: only request GPIO on first load

[Mike Frysinger]

From: Michael Hennerich <michael.hennerich-OyLXuOCK7orQT0dZR+AlfA@public.gmane.org>

The gpiolib code does not allow people to do gpio_request() on a GPIO
once it has already been requested.  So make sure we only request the
pin on the first setup of a SPI device.  Otherwise, if you attempts to
reconfigure a SPI device on the fly (like change bit sizes), the setup
function incorrectly fails.

Signed-off-by: Michael Hennerich <michael.hennerich-OyLXuOCK7orQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Mike Frysinger <vapier-aBrp7R+bbdUdnm+yROfE0A@public.gmane.org>
---
 drivers/spi/spi_bfin5xx.c |   13 ++++++++-----
 1 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/spi/spi_bfin5xx.c b/drivers/spi/spi_bfin5xx.c
index ab483a0..a8f276d 100644
--- a/drivers/spi/spi_bfin5xx.c
+++ b/drivers/spi/spi_bfin5xx.c
@@ -1099,12 +1099,15 @@ static int bfin_spi_setup(struct spi_device *spi)
 	}
 
 	if (chip->chip_select_num >= MAX_CTRL_CS) {
-		ret = gpio_request(chip->cs_gpio, spi->modalias);
-		if (ret) {
-			dev_err(&spi->dev, "gpio_request() error\n");
-			goto pin_error;
+		/* Only request on first setup */
+		if (spi_get_ctldata(spi) == NULL) {
+			ret = gpio_request(chip->cs_gpio, spi->modalias);
+			if (ret) {
+				dev_err(&spi->dev, "gpio_request() error\n");
+				goto pin_error;
+			}
+			gpio_direction_output(chip->cs_gpio, 1);
 		}
-		gpio_direction_output(chip->cs_gpio, 1);
 	}
 
 	dev_dbg(&spi->dev, "setup spi chip %s, width is %d, dma is %d\n",
-- 
1.7.3.1

[PATCH 2/2] spi/bfin_spi: handle error/status changes after data interrupts

[Mike Frysinger]

From: Michael Hennerich <michael.hennerich-OyLXuOCK7orQT0dZR+AlfA@public.gmane.org>

The error interrupt on the BF537 SIC cannot be enabled on a
per-peripheral basis.  Once the error interrupt is enabled
for one peripheral, it is automatically enabled for all.

So in the Blackfin on-chip SPI driver, we need to clear out
these known errors in the data interrupt once we've successfully
finished processing all of the pending data.

Signed-off-by: Michael Hennerich <michael.hennerich-OyLXuOCK7orQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Mike Frysinger <vapier-aBrp7R+bbdUdnm+yROfE0A@public.gmane.org>
---
 drivers/spi/spi_bfin5xx.c |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/drivers/spi/spi_bfin5xx.c b/drivers/spi/spi_bfin5xx.c
index a8f276d..3f22351 100644
--- a/drivers/spi/spi_bfin5xx.c
+++ b/drivers/spi/spi_bfin5xx.c
@@ -504,6 +504,15 @@ static irqreturn_t bfin_spi_dma_irq_handler(int irq, void *dev_id)
 		"in dma_irq_handler dmastat:0x%x spistat:0x%x\n",
 		dmastat, spistat);
 
+	if (drv_data->rx != NULL) {
+		u16 cr = read_CTRL(drv_data);
+		/* discard old RX data and clear RXS */
+		bfin_spi_dummy_read(drv_data);
+		write_CTRL(drv_data, cr & ~BIT_CTL_ENABLE); /* Disable SPI */
+		write_CTRL(drv_data, cr & ~BIT_CTL_TIMOD); /* Restore State */
+		write_STAT(drv_data, BIT_STAT_CLR); /* Clear Status */
+	}
+
 	clear_dma_irqstat(drv_data->dma_channel);
 
 	/*
-- 
1.7.3.1


------------------------------------------------------------------------------
Nokia and AT&T present the 2010 Calling All Innovators-North America contest
Create new apps & games for the Nokia N8 for consumers in  U.S. and Canada
$10 million total in prizes - $4M cash, 500 devices, nearly $6M in marketing
Develop with Nokia Qt SDK, Web Runtime, or Java and Publish to Ovi Store 
http://p.sf.net/sfu/nokia-dev2dev

Re: [PATCH 1/2] spi/bfin_spi: only request GPIO on first load

[Grant Likely]

On Fri, Oct 22, 2010 at 02:01:47AM -0400, Mike Frysinger wrote:
> From: Michael Hennerich <michael.hennerich-OyLXuOCK7orQT0dZR+AlfA@public.gmane.org>
> 
> The gpiolib code does not allow people to do gpio_request() on a GPIO
> once it has already been requested.  So make sure we only request the
> pin on the first setup of a SPI device.  Otherwise, if you attempts to
> reconfigure a SPI device on the fly (like change bit sizes), the setup
> function incorrectly fails.
> 
> Signed-off-by: Michael Hennerich <michael.hennerich-OyLXuOCK7orQT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Mike Frysinger <vapier-aBrp7R+bbdUdnm+yROfE0A@public.gmane.org>

Doesn't that mean that the gpio pin be requested at device probe time
instead of in a function that can be repeatedly called?

g.

> ---
>  drivers/spi/spi_bfin5xx.c |   13 ++++++++-----
>  1 files changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/spi/spi_bfin5xx.c b/drivers/spi/spi_bfin5xx.c
> index ab483a0..a8f276d 100644
> --- a/drivers/spi/spi_bfin5xx.c
> +++ b/drivers/spi/spi_bfin5xx.c
> @@ -1099,12 +1099,15 @@ static int bfin_spi_setup(struct spi_device *spi)
>  	}
>  
>  	if (chip->chip_select_num >= MAX_CTRL_CS) {
> -		ret = gpio_request(chip->cs_gpio, spi->modalias);
> -		if (ret) {
> -			dev_err(&spi->dev, "gpio_request() error\n");
> -			goto pin_error;
> +		/* Only request on first setup */
> +		if (spi_get_ctldata(spi) == NULL) {
> +			ret = gpio_request(chip->cs_gpio, spi->modalias);
> +			if (ret) {
> +				dev_err(&spi->dev, "gpio_request() error\n");
> +				goto pin_error;
> +			}
> +			gpio_direction_output(chip->cs_gpio, 1);
>  		}
> -		gpio_direction_output(chip->cs_gpio, 1);
>  	}
>  
>  	dev_dbg(&spi->dev, "setup spi chip %s, width is %d, dma is %d\n",
> -- 
> 1.7.3.1
> 

Re: [PATCH 1/2] spi/bfin_spi: only request GPIO on first load

[Mike Frysinger]

On Fri, Oct 22, 2010 at 02:30, Grant Likely wrote:
> On Fri, Oct 22, 2010 at 02:01:47AM -0400, Mike Frysinger wrote:
>> From: Michael Hennerich <michael.hennerich@analog.com>
>>
>> The gpiolib code does not allow people to do gpio_request() on a GPIO
>> once it has already been requested.  So make sure we only request the
>> pin on the first setup of a SPI device.  Otherwise, if you attempts to
>> reconfigure a SPI device on the fly (like change bit sizes), the setup
>> function incorrectly fails.
>
> Doesn't that mean that the gpio pin be requested at device probe time
> instead of in a function that can be repeatedly called?

not sure what you mean.  this is a pin specific to a spi slave device.
 there is no "spi probe" function for spi slave devices, only the "spi
setup" function.

if there's something specific you're referring to, you'll have to elaborate ...
-mike
_______________________________________________
Uclinux-dist-devel mailing list
Uclinux-dist-devel@blackfin.uclinux.org
https://blackfin.uclinux.org/mailman/listinfo/uclinux-dist-devel

Re: [Uclinux-dist-devel] [PATCH 1/2] spi/bfin_spi: only request GPIO on first load

[Grant Likely]

On Fri, Oct 22, 2010 at 02:41:12AM -0400, Mike Frysinger wrote:
> On Fri, Oct 22, 2010 at 02:30, Grant Likely wrote:
> > On Fri, Oct 22, 2010 at 02:01:47AM -0400, Mike Frysinger wrote:
> >> From: Michael Hennerich <michael.hennerich-OyLXuOCK7orQT0dZR+AlfA@public.gmane.org>
> >>
> >> The gpiolib code does not allow people to do gpio_request() on a GPIO
> >> once it has already been requested.  So make sure we only request the
> >> pin on the first setup of a SPI device.  Otherwise, if you attempts to
> >> reconfigure a SPI device on the fly (like change bit sizes), the setup
> >> function incorrectly fails.
> >
> > Doesn't that mean that the gpio pin be requested at device probe time
> > instead of in a function that can be repeatedly called?
> 
> not sure what you mean.  this is a pin specific to a spi slave device.
>  there is no "spi probe" function for spi slave devices, only the "spi
> setup" function.

The spi master device is responsible for managing its own cs lines,
and it should know what they are at bfin_spi_probe time.  Looking at
commit d3cc71f7 which reworked the cs_gpio bits, I see that the
blackfin driver places the gpio information with the slave instance.

One could be argue that the spi_slave can be considered the owner of
spi ss lines, but that runs counter to current conventions, and
counter to how I want to implement common infrastructure for handling
gpio slave select lines.

I see that this patch is a bug fix, and the driver already implements
bad behaviour, so I have little choice but to pick it up.  However, it
is only a band-aid solution and in the long term it should be
refactored to store the gpio slave-select lines with the spi_master
device (probably after some common gpio-slave-select infrastructure is
put in place).

g.

------------------------------------------------------------------------------
Nokia and AT&T present the 2010 Calling All Innovators-North America contest
Create new apps & games for the Nokia N8 for consumers in  U.S. and Canada
$10 million total in prizes - $4M cash, 500 devices, nearly $6M in marketing
Develop with Nokia Qt SDK, Web Runtime, or Java and Publish to Ovi Store 
http://p.sf.net/sfu/nokia-dev2dev

Re: [PATCH 1/2] spi/bfin_spi: only request GPIO on first load

[Grant Likely]

On Fri, Oct 22, 2010 at 02:01:47AM -0400, Mike Frysinger wrote:
> From: Michael Hennerich <michael.hennerich-OyLXuOCK7orQT0dZR+AlfA@public.gmane.org>
> 
> The gpiolib code does not allow people to do gpio_request() on a GPIO
> once it has already been requested.  So make sure we only request the
> pin on the first setup of a SPI device.  Otherwise, if you attempts to
> reconfigure a SPI device on the fly (like change bit sizes), the setup
> function incorrectly fails.
> 
> Signed-off-by: Michael Hennerich <michael.hennerich-OyLXuOCK7orQT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Mike Frysinger <vapier-aBrp7R+bbdUdnm+yROfE0A@public.gmane.org>

merged, thanks.

g.

> ---
>  drivers/spi/spi_bfin5xx.c |   13 ++++++++-----
>  1 files changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/spi/spi_bfin5xx.c b/drivers/spi/spi_bfin5xx.c
> index ab483a0..a8f276d 100644
> --- a/drivers/spi/spi_bfin5xx.c
> +++ b/drivers/spi/spi_bfin5xx.c
> @@ -1099,12 +1099,15 @@ static int bfin_spi_setup(struct spi_device *spi)
>  	}
>  
>  	if (chip->chip_select_num >= MAX_CTRL_CS) {
> -		ret = gpio_request(chip->cs_gpio, spi->modalias);
> -		if (ret) {
> -			dev_err(&spi->dev, "gpio_request() error\n");
> -			goto pin_error;
> +		/* Only request on first setup */
> +		if (spi_get_ctldata(spi) == NULL) {
> +			ret = gpio_request(chip->cs_gpio, spi->modalias);
> +			if (ret) {
> +				dev_err(&spi->dev, "gpio_request() error\n");
> +				goto pin_error;
> +			}
> +			gpio_direction_output(chip->cs_gpio, 1);
>  		}
> -		gpio_direction_output(chip->cs_gpio, 1);
>  	}
>  
>  	dev_dbg(&spi->dev, "setup spi chip %s, width is %d, dma is %d\n",
> -- 
> 1.7.3.1
> 

Re: [PATCH 2/2] spi/bfin_spi: handle error/status changes after data interrupts

[Grant Likely]

On Fri, Oct 22, 2010 at 02:01:48AM -0400, Mike Frysinger wrote:
> From: Michael Hennerich <michael.hennerich-OyLXuOCK7orQT0dZR+AlfA@public.gmane.org>
> 
> The error interrupt on the BF537 SIC cannot be enabled on a
> per-peripheral basis.  Once the error interrupt is enabled
> for one peripheral, it is automatically enabled for all.
> 
> So in the Blackfin on-chip SPI driver, we need to clear out
> these known errors in the data interrupt once we've successfully
> finished processing all of the pending data.
> 
> Signed-off-by: Michael Hennerich <michael.hennerich-OyLXuOCK7orQT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Mike Frysinger <vapier-aBrp7R+bbdUdnm+yROfE0A@public.gmane.org>
> ---

merged, thanks.

g.

>  drivers/spi/spi_bfin5xx.c |    9 +++++++++
>  1 files changed, 9 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/spi/spi_bfin5xx.c b/drivers/spi/spi_bfin5xx.c
> index a8f276d..3f22351 100644
> --- a/drivers/spi/spi_bfin5xx.c
> +++ b/drivers/spi/spi_bfin5xx.c
> @@ -504,6 +504,15 @@ static irqreturn_t bfin_spi_dma_irq_handler(int irq, void *dev_id)
>  		"in dma_irq_handler dmastat:0x%x spistat:0x%x\n",
>  		dmastat, spistat);
>  
> +	if (drv_data->rx != NULL) {
> +		u16 cr = read_CTRL(drv_data);
> +		/* discard old RX data and clear RXS */
> +		bfin_spi_dummy_read(drv_data);
> +		write_CTRL(drv_data, cr & ~BIT_CTL_ENABLE); /* Disable SPI */
> +		write_CTRL(drv_data, cr & ~BIT_CTL_TIMOD); /* Restore State */
> +		write_STAT(drv_data, BIT_STAT_CLR); /* Clear Status */
> +	}
> +
>  	clear_dma_irqstat(drv_data->dma_channel);
>  
>  	/*
> -- 
> 1.7.3.1
> 

Re: [PATCH 1/2] spi/bfin_spi: only request GPIO on first load

[Mike Frysinger]

On Fri, Oct 22, 2010 at 03:25, Grant Likely wrote:
> On Fri, Oct 22, 2010 at 02:41:12AM -0400, Mike Frysinger wrote:
>> On Fri, Oct 22, 2010 at 02:30, Grant Likely wrote:
>> > On Fri, Oct 22, 2010 at 02:01:47AM -0400, Mike Frysinger wrote:
>> >> From: Michael Hennerich <michael.hennerich@analog.com>
>> >>
>> >> The gpiolib code does not allow people to do gpio_request() on a GPIO
>> >> once it has already been requested.  So make sure we only request the
>> >> pin on the first setup of a SPI device.  Otherwise, if you attempts to
>> >> reconfigure a SPI device on the fly (like change bit sizes), the setup
>> >> function incorrectly fails.
>> >
>> > Doesn't that mean that the gpio pin be requested at device probe time
>> > instead of in a function that can be repeatedly called?
>>
>> not sure what you mean.  this is a pin specific to a spi slave device.
>>  there is no "spi probe" function for spi slave devices, only the "spi
>> setup" function.
>
> The spi master device is responsible for managing its own cs lines,
> and it should know what they are at bfin_spi_probe time.  Looking at
> commit d3cc71f7 which reworked the cs_gpio bits, I see that the
> blackfin driver places the gpio information with the slave instance.
>
> One could be argue that the spi_slave can be considered the owner of
> spi ss lines, but that runs counter to current conventions, and
> counter to how I want to implement common infrastructure for handling
> gpio slave select lines.
>
> I see that this patch is a bug fix, and the driver already implements
> bad behaviour, so I have little choice but to pick it up.  However, it
> is only a band-aid solution and in the long term it should be
> refactored to store the gpio slave-select lines with the spi_master
> device (probably after some common gpio-slave-select infrastructure is
> put in place).

i disagree.  if any GPIO at all may be used as a CS, how could the SPI
master driver have any idea at all what GPIO a slave might choose to
use ?  or who is to say that the GPIO in question is available when
the SPI bus master registers itself if the GPIO sits on a GPIO
expander board (e.g. i2c) and not the SOC ?

this is information that only the SPI client knows about and that the
slave device is binding to.  putting slave information into the master
resources makes no sense to me.
-mike
_______________________________________________
Uclinux-dist-devel mailing list
Uclinux-dist-devel@blackfin.uclinux.org
https://blackfin.uclinux.org/mailman/listinfo/uclinux-dist-devel

Re: [PATCH 1/2] spi/bfin_spi: only request GPIO on first load

[Grant Likely]

On Fri, Oct 22, 2010 at 3:33 AM, Mike Frysinger <vapier-aBrp7R+bbdUdnm+yROfE0A@public.gmane.org> wrote:
> On Fri, Oct 22, 2010 at 03:25, Grant Likely wrote:
>> On Fri, Oct 22, 2010 at 02:41:12AM -0400, Mike Frysinger wrote:
>>> On Fri, Oct 22, 2010 at 02:30, Grant Likely wrote:
>>> > On Fri, Oct 22, 2010 at 02:01:47AM -0400, Mike Frysinger wrote:
>>> >> From: Michael Hennerich <michael.hennerich-OyLXuOCK7orQT0dZR+AlfA@public.gmane.org>
>>> >>
>>> >> The gpiolib code does not allow people to do gpio_request() on a GPIO
>>> >> once it has already been requested.  So make sure we only request the
>>> >> pin on the first setup of a SPI device.  Otherwise, if you attempts to
>>> >> reconfigure a SPI device on the fly (like change bit sizes), the setup
>>> >> function incorrectly fails.
>>> >
>>> > Doesn't that mean that the gpio pin be requested at device probe time
>>> > instead of in a function that can be repeatedly called?
>>>
>>> not sure what you mean.  this is a pin specific to a spi slave device.
>>>  there is no "spi probe" function for spi slave devices, only the "spi
>>> setup" function.
>>
>> The spi master device is responsible for managing its own cs lines,
>> and it should know what they are at bfin_spi_probe time.  Looking at
>> commit d3cc71f7 which reworked the cs_gpio bits, I see that the
>> blackfin driver places the gpio information with the slave instance.
>>
>> One could be argue that the spi_slave can be considered the owner of
>> spi ss lines, but that runs counter to current conventions, and
>> counter to how I want to implement common infrastructure for handling
>> gpio slave select lines.
>>
>> I see that this patch is a bug fix, and the driver already implements
>> bad behaviour, so I have little choice but to pick it up.  However, it
>> is only a band-aid solution and in the long term it should be
>> refactored to store the gpio slave-select lines with the spi_master
>> device (probably after some common gpio-slave-select infrastructure is
>> put in place).
>
> i disagree.  if any GPIO at all may be used as a CS, how could the SPI
> master driver have any idea at all what GPIO a slave might choose to
> use ?  or who is to say that the GPIO in question is available when
> the SPI bus master registers itself if the GPIO sits on a GPIO
> expander board (e.g. i2c) and not the SOC ?
>
> this is information that only the SPI client knows about and that the
> slave device is binding to.  putting slave information into the master
> resources makes no sense to me.

The arguement can be made in either direction.  From one perspective,
the GPIO is a property of the SPI controller because that is what is
responsible for fiddling with the bit (evidenced by the fact that the
GPIO number is being stored in controller data).  From another
perspective it could be seen as a property of the spi_device.
However, at some point a convention needs to be chosen, and the SPI
layer has tended towards SS lines being a property of the SPI
controller; regardless of whether or not it happens to be a GPIO line.

It isn't a critical issue now, but it will become more important when
the GPIO SS line handling is generalized so that the individual
drivers can be simplified.

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.