diff mbox

[V3,4/5] spi: s3c64xx: Added provision for dedicated cs pin

Message ID 1363157014-9615-5-git-send-email-ks.giri@samsung.com (mailing list archive)
State Superseded, archived
Headers show

Commit Message

girishks2000@gmail.com March 13, 2013, 6:43 a.m. UTC
From: Girish K S <girishks2000@gmail.com>

The existing driver supports gpio based /cs signal.
For controller's that have one device per controller,
the slave device's /cs signal might be internally controlled
by the chip select bit of slave select register. They are not
externally asserted/deasserted using gpio pin.

This patch adds support for controllers with dedicated /cs pin.
if "cs-gpio" property doesnt exist in a spi dts node, the controller
would treat the /cs pin as dedicated.

Signed-off-by: Girish K S <ks.giri@samsung.com>
---
 drivers/spi/spi-s3c64xx.c                 | 27 +++++++++++++++++++--------
 include/linux/platform_data/spi-s3c64xx.h |  3 +++
 2 files changed, 22 insertions(+), 8 deletions(-)

Comments

girishks2000@gmail.com April 8, 2013, 9:51 a.m. UTC | #1
On Mon, Apr 1, 2013 at 6:27 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Wed, Mar 13, 2013 at 12:13:33PM +0530, Girish K S wrote:
>> From: Girish K S <girishks2000@gmail.com>
>>
>> The existing driver supports gpio based /cs signal.
>> For controller's that have one device per controller,
>> the slave device's /cs signal might be internally controlled
>> by the chip select bit of slave select register. They are not
>> externally asserted/deasserted using gpio pin.
>
> Applying this patch breaks my S3C64xx based system (Cragganmore, a
> non-DT platform).  It'll break all existing non-DT platforms since...

sure will rebase and take care of the non-dt case

>
>> + * @cs_gpio: CS line status, 'true' if CS line is asserted by gpio.
>> + *        'false' if asserted by internal dedicated pin.
>>   * @line: Custom 'identity' of the CS line.
>>   *
>>   * This is per SPI-Slave Chipselect information.
>> @@ -25,6 +27,7 @@ struct platform_device;
>>   */
>>  struct s3c64xx_spi_csinfo {
>>       u8 fb_delay;
>> +     bool cs_gpio;
>>       unsigned line;
>>  };
>
> ...you've added this new cs_gpio field to the platform data but not
> updated any of the existing users (including Cragganmore).  It would
> seem better to make the default behaviour stay as per the current
> default and make the new behaviour optional but at a minimum all
> existing in-tree users need to be updated.
>
> It's also a bit odd that we end up checking cs_gpio and then using line
> in the code, it'd be more idiomatic if cs_gpio were the GPIO number.

In the original driver it was assumed that the cs line is always a gpio pin.
But the current controller that i am working on has no gpio pin for cs
selection.
All the lines to the device are internally connected. There is no
option to select
the cs signal. So cs-gpio property parsing has to skipped for this
controller, that means
cs_gpio cannot be a GPIO number. If it has to be a number then it has
to be < 0 to say
it is not gpio. Any >= 0 number implies it is a valid gpio (in reality
for this controller it is not.)

So i introduced a new member cs_gpio. I will checkout the possibility
to avoid this member.

------------------------------------------------------------------------------
Minimize network downtime and maximize team effectiveness.
Reduce network management and security costs.Learn how to hire 
the most talented Cisco Certified professionals. Visit the 
Employer Resources Portal
http://www.cisco.com/web/learning/employer_resources/index.html
girishks2000@gmail.com April 8, 2013, 11:45 a.m. UTC | #2
On Mon, Apr 8, 2013 at 3:45 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Mon, Apr 08, 2013 at 03:21:03PM +0530, Girish KS wrote:
>> On Mon, Apr 1, 2013 at 6:27 PM, Mark Brown
>
>> > It's also a bit odd that we end up checking cs_gpio and then using line
>> > in the code, it'd be more idiomatic if cs_gpio were the GPIO number.
>
>> In the original driver it was assumed that the cs line is always a gpio pin.
>> But the current controller that i am working on has no gpio pin for cs
>> selection.
>> All the lines to the device are internally connected. There is no
>> option to select
>> the cs signal. So cs-gpio property parsing has to skipped for this
>> controller, that means
>> cs_gpio cannot be a GPIO number. If it has to be a number then it has
>> to be < 0 to say
>> it is not gpio. Any >= 0 number implies it is a valid gpio (in reality
>> for this controller it is not.)
>
> Two options here, one is to just assume nobody will use GPIO 0 and the
> other is to set the number appopriately during probe so that only probe
> needs to worry about the issue.

regarding the first option, may be others also should agree to it (in
case if somebody is
using the gpio 0).
In the second option if the gpio number is set in the probe, then we
are overwriting the
actual gpio number assigned in the platform data.
If I move the cs_gpio from the platform data to controller private
data then the dependency
on other platforms can be removed, but still the check (true or false)
before setting the gpio
line will remain. If agreed upon this i can go ahead and post the patch

------------------------------------------------------------------------------
Minimize network downtime and maximize team effectiveness.
Reduce network management and security costs.Learn how to hire 
the most talented Cisco Certified professionals. Visit the 
Employer Resources Portal
http://www.cisco.com/web/learning/employer_resources/index.html
girishks2000@gmail.com April 8, 2013, 11:52 a.m. UTC | #3
On Mon, Apr 8, 2013 at 5:15 PM, Girish KS <girishks2000@gmail.com> wrote:
> On Mon, Apr 8, 2013 at 3:45 PM, Mark Brown
> <broonie@opensource.wolfsonmicro.com> wrote:
>> On Mon, Apr 08, 2013 at 03:21:03PM +0530, Girish KS wrote:
>>> On Mon, Apr 1, 2013 at 6:27 PM, Mark Brown
>>
>>> > It's also a bit odd that we end up checking cs_gpio and then using line
>>> > in the code, it'd be more idiomatic if cs_gpio were the GPIO number.
>>
>>> In the original driver it was assumed that the cs line is always a gpio pin.
>>> But the current controller that i am working on has no gpio pin for cs
>>> selection.
>>> All the lines to the device are internally connected. There is no
>>> option to select
>>> the cs signal. So cs-gpio property parsing has to skipped for this
>>> controller, that means
>>> cs_gpio cannot be a GPIO number. If it has to be a number then it has
>>> to be < 0 to say
>>> it is not gpio. Any >= 0 number implies it is a valid gpio (in reality
>>> for this controller it is not.)
>>
>> Two options here, one is to just assume nobody will use GPIO 0 and the
>> other is to set the number appopriately during probe so that only probe
>> needs to worry about the issue.
>
> regarding the first option, may be others also should agree to it (in
> case if somebody is
> using the gpio 0).
> In the second option if the gpio number is set in the probe, then we
> are overwriting the
> actual gpio number assigned in the platform data.
> If I move the cs_gpio from the platform data to controller private
> data then the dependency
> on other platforms can be removed, but still the check (true or false)
> before setting the gpio
> line will remain. If agreed upon this i can go ahead and post the patch

Sorry for the allignment. there is some issue with my interface

------------------------------------------------------------------------------
Minimize network downtime and maximize team effectiveness.
Reduce network management and security costs.Learn how to hire 
the most talented Cisco Certified professionals. Visit the 
Employer Resources Portal
http://www.cisco.com/web/learning/employer_resources/index.html
girishks2000@gmail.com April 8, 2013, 1:49 p.m. UTC | #4
On Mon, Apr 8, 2013 at 5:50 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Mon, Apr 08, 2013 at 05:15:26PM +0530, Girish KS wrote:
>> On Mon, Apr 8, 2013 at 3:45 PM, Mark Brown
>
>> > Two options here, one is to just assume nobody will use GPIO 0 and the
>> > other is to set the number appopriately during probe so that only probe
>> > needs to worry about the issue.
>>
>> In the second option if the gpio number is set in the probe, then we
>> are overwriting the
>> actual gpio number assigned in the platform data.
>> If I move the cs_gpio from the platform data to controller private
>> data then the dependency
>> on other platforms can be removed, but still the check (true or false)
>> before setting the gpio
>> line will remain. If agreed upon this i can go ahead and post the patch
>
> I think logic in probe should be fine, it just felt really cumbersome
> having this on every single use but doing it once on probe ought to be
> OK.

I will move the populating cs_gpio logic to probe. But the enable_cs
and disable_cs will have the
check before calling the gpio_set_value api (because it takes cs-line
as a parameter),

------------------------------------------------------------------------------
Minimize network downtime and maximize team effectiveness.
Reduce network management and security costs.Learn how to hire 
the most talented Cisco Certified professionals. Visit the 
Employer Resources Portal
http://www.cisco.com/web/learning/employer_resources/index.html
diff mbox

Patch

diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index 94716d1..ac557f8 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -414,14 +414,16 @@  static inline void enable_cs(struct s3c64xx_spi_driver_data *sdd,
 		if (sdd->tgl_spi != spi) { /* if last mssg on diff device */
 			/* Deselect the last toggled device */
 			cs = sdd->tgl_spi->controller_data;
-			gpio_set_value(cs->line,
-				spi->mode & SPI_CS_HIGH ? 0 : 1);
+			if (cs->cs_gpio)
+				gpio_set_value(cs->line,
+					spi->mode & SPI_CS_HIGH ? 0 : 1);
 		}
 		sdd->tgl_spi = NULL;
 	}
 
 	cs = spi->controller_data;
-	gpio_set_value(cs->line, spi->mode & SPI_CS_HIGH ? 1 : 0);
+	if (cs->cs_gpio)
+		gpio_set_value(cs->line, spi->mode & SPI_CS_HIGH ? 1 : 0);
 
 	/* Start the signals */
 	writel(0, sdd->regs + S3C64XX_SPI_SLAVE_SEL);
@@ -550,7 +552,8 @@  static inline void disable_cs(struct s3c64xx_spi_driver_data *sdd,
 	if (sdd->tgl_spi == spi)
 		sdd->tgl_spi = NULL;
 
-	gpio_set_value(cs->line, spi->mode & SPI_CS_HIGH ? 0 : 1);
+	if (cs->cs_gpio)
+		gpio_set_value(cs->line, spi->mode & SPI_CS_HIGH ? 0 : 1);
 
 	/* Quiese the signals */
 	writel(S3C64XX_SPI_SLAVE_SIG_INACT,
@@ -889,7 +892,12 @@  static struct s3c64xx_spi_csinfo *s3c64xx_get_slave_ctrldata(
 		return ERR_PTR(-ENOMEM);
 	}
 
-	cs->line = of_get_named_gpio(data_np, "cs-gpio", 0);
+	if (of_find_property(data_np, "cs-gpio", NULL)) {
+		/* The CS line is asserted/deasserted by the gpio pin */
+		cs->cs_gpio = true;
+		cs->line = of_get_named_gpio(data_np, "cs-gpio", 0);
+	}
+
 	if (!gpio_is_valid(cs->line)) {
 		dev_err(&spi->dev, "chip select gpio is not specified or invalid\n");
 		kfree(cs);
@@ -929,7 +937,8 @@  static int s3c64xx_spi_setup(struct spi_device *spi)
 		return -ENODEV;
 	}
 
-	if (!spi_get_ctldata(spi)) {
+	/* Request gpio only if cs line is asserted by gpio pins */
+	if (cs->cs_gpio) {
 		err = gpio_request_one(cs->line, GPIOF_OUT_INIT_HIGH,
 				       dev_name(&spi->dev));
 		if (err) {
@@ -938,9 +947,11 @@  static int s3c64xx_spi_setup(struct spi_device *spi)
 				cs->line, err);
 			goto err_gpio_req;
 		}
-		spi_set_ctldata(spi, cs);
 	}
 
+	if (!spi_get_ctldata(spi))
+		spi_set_ctldata(spi, cs);
+
 	sci = sdd->cntrlr_info;
 
 	spin_lock_irqsave(&sdd->lock, flags);
@@ -1028,7 +1039,7 @@  static void s3c64xx_spi_cleanup(struct spi_device *spi)
 {
 	struct s3c64xx_spi_csinfo *cs = spi_get_ctldata(spi);
 
-	if (cs) {
+	if (cs && cs->cs_gpio) {
 		gpio_free(cs->line);
 		if (spi->dev.of_node)
 			kfree(cs);
diff --git a/include/linux/platform_data/spi-s3c64xx.h b/include/linux/platform_data/spi-s3c64xx.h
index ceba18d..0343d8d 100644
--- a/include/linux/platform_data/spi-s3c64xx.h
+++ b/include/linux/platform_data/spi-s3c64xx.h
@@ -17,6 +17,8 @@  struct platform_device;
  * struct s3c64xx_spi_csinfo - ChipSelect description
  * @fb_delay: Slave specific feedback delay.
  *            Refer to FB_CLK_SEL register definition in SPI chapter.
+ * @cs_gpio: CS line status, 'true' if CS line is asserted by gpio.
+ * 	     'false' if asserted by internal dedicated pin.
  * @line: Custom 'identity' of the CS line.
  *
  * This is per SPI-Slave Chipselect information.
@@ -25,6 +27,7 @@  struct platform_device;
  */
 struct s3c64xx_spi_csinfo {
 	u8 fb_delay;
+	bool cs_gpio;
 	unsigned line;
 };