diff mbox

[5/8] gpio: 74x164: Add output pin support

Message ID 1346834457-6257-5-git-send-email-maxime.ripard@free-electrons.com (mailing list archive)
State New, archived
Headers show

Commit Message

Maxime Ripard Sept. 5, 2012, 8:40 a.m. UTC
The shift registers have an output that, when enabled, propagates the
values of its internal register to the pins. Make use of this pin
through the output-latch-gpio dt property.

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 drivers/gpio/gpio-74x164.c |   32 +++++++++++++++++++++++++++++++-
 1 file changed, 31 insertions(+), 1 deletion(-)

Comments

Thomas Petazzoni Sept. 5, 2012, 9:20 a.m. UTC | #1
Le Wed,  5 Sep 2012 10:40:54 +0200,
Maxime Ripard <maxime.ripard@free-electrons.com> a écrit :

> The shift registers have an output that, when enabled, propagates the
> values of its internal register to the pins. Make use of this pin
> through the output-latch-gpio dt property.
> 
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>

Well, thinking more about this, and reviewing the i.MX 28 datasheet a
bit more thoroughly, I don't think you need this GPIO thing. It is an
active-low pin that should be kept low during the entire duration of
the transfer, which basically is a chip select. And in turns out that
the CFA10049 board uses pins that can be controlled as chip selects
directly by the SPI controller, so you should rather use this rather
than introduce a specially handled GPIO. This needs testing, but I
don't see why it wouldn't work.

Best regards,

Thomas
Eric Bénard Sept. 5, 2012, 9:46 a.m. UTC | #2
Hi Thomas,

Le Wed, 5 Sep 2012 11:20:12 +0200,
Thomas Petazzoni <thomas.petazzoni@free-electrons.com> a écrit :

> Le Wed,  5 Sep 2012 10:40:54 +0200,
> Maxime Ripard <maxime.ripard@free-electrons.com> a écrit :
> 
> > The shift registers have an output that, when enabled, propagates the
> > values of its internal register to the pins. Make use of this pin
> > through the output-latch-gpio dt property.
> > 
> > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> 
> Well, thinking more about this, and reviewing the i.MX 28 datasheet a
> bit more thoroughly, I don't think you need this GPIO thing. It is an
> active-low pin that should be kept low during the entire duration of
> the transfer, which basically is a chip select. And in turns out that
> the CFA10049 board uses pins that can be controlled as chip selects
> directly by the SPI controller, so you should rather use this rather
> than introduce a specially handled GPIO. This needs testing, but I
> don't see why it wouldn't work.
> 
are you talking of the /OE pin of the 74HC595 ?

Eric
Thomas Petazzoni Sept. 5, 2012, 10:09 a.m. UTC | #3
Le Wed, 5 Sep 2012 11:46:45 +0200,
Eric Bénard <eric@eukrea.com> a écrit :

> > Well, thinking more about this, and reviewing the i.MX 28 datasheet a
> > bit more thoroughly, I don't think you need this GPIO thing. It is an
> > active-low pin that should be kept low during the entire duration of
> > the transfer, which basically is a chip select. And in turns out that
> > the CFA10049 board uses pins that can be controlled as chip selects
> > directly by the SPI controller, so you should rather use this rather
> > than introduce a specially handled GPIO. This needs testing, but I
> > don't see why it wouldn't work.
> > 
> are you talking of the /OE pin of the 74HC595 ?

No. Our chip select lines are connected to the STCP pin. It is
technically not a chip select, but a low-to-high transition on this pin
triggers the apparition on the output pins of the 74HC595 of the
contents of the storage register. So, doing a high-to-low transition
before the SPI transfer, and then a low-to-high transition after the
SPI transfer will actually do what we want. But maybe this is an abuse
of the chip select concept, I don't know.

I don't think the /OE pin can be used as a chip select, because when it
is high, the state of the output pins is not maintained to their
previous state: the pins are turned into the high impedance state. If
used as a chip select, it would mean that the value of the output pins
transition from their old state to high impedance at the beginning of
the transfer, and then from the high impedance to their new state at
the end of the transfer. And of course, this signal is inverted
compared to a normal chip select.

Note that I may be completely wrong, I'm a software guy, not a hardware
one :)

Thomas
Eric Bénard Sept. 5, 2012, 10:26 a.m. UTC | #4
Hi,

Le Wed, 5 Sep 2012 12:09:06 +0200,
Thomas Petazzoni <thomas.petazzoni@free-electrons.com> a écrit :
> I don't think the /OE pin can be used as a chip select, because when it
> is high, the state of the output pins is not maintained to their
> previous state: the pins are turned into the high impedance state. If
> used as a chip select, it would mean that the value of the output pins
> transition from their old state to high impedance at the beginning of
> the transfer, and then from the high impedance to their new state at
> the end of the transfer.

OK that's exactly what I was thinking to ;-)

> And of course, this signal is inverted compared to a normal chip select.

polarity of the chip select can vary (mc13xxx-spi has an active high CS
for example).

Eric
Thomas Petazzoni Sept. 5, 2012, 11:56 a.m. UTC | #5
Le Wed, 5 Sep 2012 12:26:52 +0200,
Eric Bénard <eric@eukrea.com> a écrit :

> Le Wed, 5 Sep 2012 12:09:06 +0200,
> Thomas Petazzoni <thomas.petazzoni@free-electrons.com> a écrit :
> > I don't think the /OE pin can be used as a chip select, because when it
> > is high, the state of the output pins is not maintained to their
> > previous state: the pins are turned into the high impedance state. If
> > used as a chip select, it would mean that the value of the output pins
> > transition from their old state to high impedance at the beginning of
> > the transfer, and then from the high impedance to their new state at
> > the end of the transfer.
> 
> OK that's exactly what I was thinking to ;-)

Good. So, do you think it's reasonable to use the STCP as a chip-select
for this device?

> > And of course, this signal is inverted compared to a normal chip select.
> 
> polarity of the chip select can vary (mc13xxx-spi has an active high CS
> for example).

Sure, but I guess it depends on capabilities of the SPI controller in
your SoC. I haven't seen a way of configuring the polarity of the chip
selects in the i.MX28 SPI controller, but maybe I've missed it.
Anyway, /OE isn't usable as a chip-select, so we don't care.

Best regards,

Thomas
Eric Bénard Sept. 5, 2012, 12:22 p.m. UTC | #6
Le Wed, 5 Sep 2012 13:56:46 +0200,
Thomas Petazzoni <thomas.petazzoni@free-electrons.com> a écrit :

> Le Wed, 5 Sep 2012 12:26:52 +0200,
> Eric Bénard <eric@eukrea.com> a écrit :
> 
> > Le Wed, 5 Sep 2012 12:09:06 +0200,
> > Thomas Petazzoni <thomas.petazzoni@free-electrons.com> a écrit :
> > > I don't think the /OE pin can be used as a chip select, because when it
> > > is high, the state of the output pins is not maintained to their
> > > previous state: the pins are turned into the high impedance state. If
> > > used as a chip select, it would mean that the value of the output pins
> > > transition from their old state to high impedance at the beginning of
> > > the transfer, and then from the high impedance to their new state at
> > > the end of the transfer.
> > 
> > OK that's exactly what I was thinking to ;-)
> 
> Good. So, do you think it's reasonable to use the STCP as a chip-select
> for this device?
> 
in your case maybe but that really depends on how the chip is wired to
the CPU so I'm not sure that can be a generic choice.

> > > And of course, this signal is inverted compared to a normal chip select.
> > 
> > polarity of the chip select can vary (mc13xxx-spi has an active high CS
> > for example).
> 
> Sure, but I guess it depends on capabilities of the SPI controller in
> your SoC. I haven't seen a way of configuring the polarity of the chip
> selects in the i.MX28 SPI controller, but maybe I've missed it.
> Anyway, /OE isn't usable as a chip-select, so we don't care.
> 
that's a reason why the chip select can be a simple gpio (even if
it's wired on a SPI chip select capable pin)

Eric
Thomas Petazzoni Sept. 5, 2012, 12:29 p.m. UTC | #7
Le Wed, 5 Sep 2012 14:22:44 +0200,
Eric Bénard <eric@eukrea.com> a écrit :

> > > OK that's exactly what I was thinking to ;-)
> > 
> > Good. So, do you think it's reasonable to use the STCP as a chip-select
> > for this device?
>
> in your case maybe but that really depends on how the chip is wired to
> the CPU so I'm not sure that can be a generic choice.

I'm not sure to see how this chip could be wired differently to the
CPU. The CPU must do a low-to-high transition on STCP to get the values
out from the internal register.

So, in other words, we don't know how to choose between:

 *) Keep the current solution Maxime has implemented, where we have a
    specific latch GPIO property in the Device Tree for this 74HC595,
    and the 74HC595 driver is responsible for toggling this GPIO when
    needed.

 *) Use the internal SPI controller logic to control this pin as a
    chip-select, and therefore get rid of Maxime's code with the
    specific latch GPIO property in the Device Tree.

Thoughts?

Thomas
Eric Bénard Sept. 5, 2012, 12:54 p.m. UTC | #8
Le Wed, 5 Sep 2012 14:29:52 +0200,
Thomas Petazzoni <thomas.petazzoni@free-electrons.com> a écrit :

> Le Wed, 5 Sep 2012 14:22:44 +0200,
> Eric Bénard <eric@eukrea.com> a écrit :
> 
> > > > OK that's exactly what I was thinking to ;-)
> > > 
> > > Good. So, do you think it's reasonable to use the STCP as a chip-select
> > > for this device?
> >
> > in your case maybe but that really depends on how the chip is wired to
> > the CPU so I'm not sure that can be a generic choice.
> 
> I'm not sure to see how this chip could be wired differently to the
> CPU. The CPU must do a low-to-high transition on STCP to get the values
> out from the internal register.
> 
> So, in other words, we don't know how to choose between:
> 
>  *) Keep the current solution Maxime has implemented, where we have a
>     specific latch GPIO property in the Device Tree for this 74HC595,
>     and the 74HC595 driver is responsible for toggling this GPIO when
>     needed.
> 
>  *) Use the internal SPI controller logic to control this pin as a
>     chip-select, and therefore get rid of Maxime's code with the
>     specific latch GPIO property in the Device Tree.
> 
daisy chaining won't work in the second case (I may be wrong but IIRC
the chip select will toggle at each spi_write) unless you configure the
spi controller to send a 8x(number of 74HC595) bits word :

 	gpio_set_value(chip->chip_select, 0);
	for (i = chip->registers - 1; i >= 0; i--) {
		ret = spi_write(chip->spi,
				chip->buffer + i, sizeof(u8));
		if (ret)
			return ret;
	}

 	gpio_set_value(chip->chip_select, 1);

Eric
Maxime Ripard Sept. 5, 2012, 1:02 p.m. UTC | #9
Le 05/09/2012 14:54, Eric Bénard a écrit :
> Le Wed, 5 Sep 2012 14:29:52 +0200,
> Thomas Petazzoni <thomas.petazzoni@free-electrons.com> a écrit :
> 
>> Le Wed, 5 Sep 2012 14:22:44 +0200,
>> Eric Bénard <eric@eukrea.com> a écrit :
>>
>>>>> OK that's exactly what I was thinking to ;-)
>>>>
>>>> Good. So, do you think it's reasonable to use the STCP as a chip-select
>>>> for this device?
>>>
>>> in your case maybe but that really depends on how the chip is wired to
>>> the CPU so I'm not sure that can be a generic choice.
>>
>> I'm not sure to see how this chip could be wired differently to the
>> CPU. The CPU must do a low-to-high transition on STCP to get the values
>> out from the internal register.
>>
>> So, in other words, we don't know how to choose between:
>>
>>  *) Keep the current solution Maxime has implemented, where we have a
>>     specific latch GPIO property in the Device Tree for this 74HC595,
>>     and the 74HC595 driver is responsible for toggling this GPIO when
>>     needed.
>>
>>  *) Use the internal SPI controller logic to control this pin as a
>>     chip-select, and therefore get rid of Maxime's code with the
>>     specific latch GPIO property in the Device Tree.
>>
> daisy chaining won't work in the second case (I may be wrong but IIRC
> the chip select will toggle at each spi_write) unless you configure the
> spi controller to send a 8x(number of 74HC595) bits word :
> 
>  	gpio_set_value(chip->chip_select, 0);
> 	for (i = chip->registers - 1; i >= 0; i--) {
> 		ret = spi_write(chip->spi,
> 				chip->buffer + i, sizeof(u8));
> 		if (ret)
> 			return ret;
> 	}
> 
>  	gpio_set_value(chip->chip_select, 1);

And what about using the spi_message structure and one single spi_sync
instead of several spi_write ? Would that work in our case ?
Eric Bénard Sept. 5, 2012, 1:27 p.m. UTC | #10
Le Wed, 05 Sep 2012 15:02:09 +0200,
Maxime Ripard <maxime.ripard@free-electrons.com> a écrit :
> And what about using the spi_message structure and one single spi_sync
> instead of several spi_write ? Would that work in our case ?
> 
if you are sure that the controler will keep the chip select low during
the whole transfer that should work (that means on some spi controllers
you will have to configure the chip select as a plain gpio depending on
the number of buffer you chain).

Eric
diff mbox

Patch

diff --git a/drivers/gpio/gpio-74x164.c b/drivers/gpio/gpio-74x164.c
index 613067c..ff8d3d8 100644
--- a/drivers/gpio/gpio-74x164.c
+++ b/drivers/gpio/gpio-74x164.c
@@ -13,6 +13,7 @@ 
 #include <linux/mutex.h>
 #include <linux/spi/spi.h>
 #include <linux/gpio.h>
+#include <linux/of_gpio.h>
 #include <linux/slab.h>
 #include <linux/module.h>
 
@@ -21,6 +22,7 @@  struct gen_74x164_chip {
 	struct gpio_chip	gpio_chip;
 	struct mutex		lock;
 	u8			port_config;
+	int			chip_select;
 };
 
 static struct gen_74x164_chip *gpio_to_74x164_chip(struct gpio_chip *gc)
@@ -30,8 +32,14 @@  static struct gen_74x164_chip *gpio_to_74x164_chip(struct gpio_chip *gc)
 
 static int __gen_74x164_write_config(struct gen_74x164_chip *chip)
 {
-	return spi_write(chip->spi,
+	int ret;
+
+	gpio_set_value(chip->chip_select, 0);
+	ret = spi_write(chip->spi,
 			 &chip->port_config, sizeof(chip->port_config));
+	gpio_set_value(chip->chip_select, 1);
+
+	return ret;
 }
 
 static int gen_74x164_get_value(struct gpio_chip *gc, unsigned offset)
@@ -73,6 +81,11 @@  static int __devinit gen_74x164_probe(struct spi_device *spi)
 	struct gen_74x164_chip *chip;
 	int ret;
 
+	if (!spi->dev.of_node) {
+		dev_err(&spi->dev, "No device tree data available.\n");
+		return -EINVAL;
+	}
+
 	/*
 	 * bits_per_word cannot be configured in platform data
 	 */
@@ -102,6 +115,23 @@  static int __devinit gen_74x164_probe(struct spi_device *spi)
 	chip->gpio_chip.dev = &spi->dev;
 	chip->gpio_chip.owner = THIS_MODULE;
 
+	chip->chip_select = of_get_named_gpio(spi->dev.of_node, "output-latch-gpio", 0);
+	if (gpio_is_valid(chip->chip_select)) {
+		int flags = GPIOF_OUT_INIT_HIGH;
+		if (of_get_property(spi->dev.of_node, "latch-active-low", NULL))
+			flags = GPIOF_OUT_INIT_LOW;
+		ret = devm_gpio_request_one(&spi->dev, chip->chip_select,
+					    flags, "output-latch");
+		if (ret) {
+			dev_err(&spi->dev,
+				"failed to request gpio %d: %d\n",
+				chip->chip_select, ret);
+			goto exit_destroy;
+		}
+	}
+
+	gpio_set_value(chip->chip_select, 1);
+
 	ret = __gen_74x164_write_config(chip);
 	if (ret) {
 		dev_err(&spi->dev, "Failed writing: %d\n", ret);