gpio: 74x164: Use module_spi_driver boiler plate function

Signed-off-by Maxime Ripard <[email protected]>

drivers/gpio/gpio-74x164.c +2 -3

 	dev_set_drvdata(&spi->dev, NULL);
 
 	ret = gpiochip_remove(&chip->gpio_chip);
-	if (!ret) {
+	if (!ret)
 		mutex_destroy(&chip->lock);
-		kfree(chip);
-	} else
+	else
 		dev_err(&spi->dev, "Failed to remove the GPIO controller: %d\n",
 				ret);
 

Signature · 7 lines

1.7.9.5


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Signed-off-by Maxime Ripard <[email protected]>

Signed-off-by Maxime Ripard <[email protected]>

---
 drivers/gpio/gpio-74x164.c |    8 +++-----

Signed-off-by Maxime Ripard <[email protected]>

Signed-off-by Maxime Ripard <[email protected]>

drivers/gpio/gpio-74x164.c +1 -1

 	chip->gpio_chip.direction_output = gen_74x164_direction_output;
 	chip->gpio_chip.get = gen_74x164_get_value;
 	chip->gpio_chip.set = gen_74x164_set_value;
-	chip->gpio_chip.base = pdata->base;
+	chip->gpio_chip.base = -1;
 	chip->gpio_chip.ngpio = 8;
 	chip->gpio_chip.can_sleep = 1;
 	chip->gpio_chip.dev = &spi->dev;

include/linux/spi/74x164.h deleted +0 -9

deleted file mode 100644
index 0aa6acc..0000000
@@ -1,9 +0,0 @@
-#ifndef LINUX_SPI_74X164_H
-#define LINUX_SPI_74X164_H
-
-struct gen_74x164_chip_platform_data {
-	/* number assigned to the first GPIO */
-	unsigned	base;
-};
-
-#endif

Signature · 7 lines

1.7.9.5


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Quoted 1 line

Signed-off-by Maxime Ripard <[email protected]>

No, please don't do it that way, rather check if the platform_data pointer is null, and if it is, fall-back to dynamic gpio base assignment. There is a currently out of tree user of this for which it matters to be able to set a specific gpio base numbering.

Signed-off-by Maxime Ripard <[email protected]>

Signed-off-by Maxime Ripard <[email protected]>

drivers/gpio/gpio-74x164.c +7 -0

 	return ret;
 }
 
+static const struct of_device_id gen_74x164_dt_ids[] = {
+	{ .compatible = "fairchild,74hc595" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, gen_74x164_dt_ids);
+
 static struct spi_driver gen_74x164_driver = {
 	.driver = {
 		.name		= "74x164",
 		.owner		= THIS_MODULE,
+		.of_match_table	= of_match_ptr(gen_74x164_dt_ids),
 	},
 	.probe		= gen_74x164_probe,
 	.remove		= __devexit_p(gen_74x164_remove),

Signature · 7 lines

1.7.9.5


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Signed-off-by Maxime Ripard <[email protected]>

Signed-off-by Maxime Ripard <[email protected]>

---
 drivers/gpio/gpio-74x164.c |    7 +++++++

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 <[email protected]>

Le Wed, 5 Sep 2012 10:40:54 +0200,
Maxime Ripard <[email protected]> a ?crit :

Quoted 5 lines

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 <[email protected]>

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.

Quoted 9 lines

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 ?

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.

Le Wed, 5 Sep 2012 12:09:06 +0200,
Thomas Petazzoni <[email protected]> a ?crit :

Quoted 7 lines

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 ;-)

Quoted 1 line

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).

Le Wed, 5 Sep 2012 12:26:52 +0200,
Eric B?nard <[email protected]> a ?crit :

Quoted 10 lines

Le Wed, 5 Sep 2012 12:09:06 +0200,
Thomas Petazzoni <[email protected]> a ?crit :

Quoted 7 lines

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.

Quoted 1 line

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?

Quoted 4 lines

Quoted 1 line

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.

Quoted 4 lines

Quoted 1 line

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.

Le Wed, 5 Sep 2012 14:22:44 +0200,
Eric B?nard <[email protected]> a ?crit :

Quoted 7 lines

Quoted 4 lines

Quoted 1 line

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.

Le Wed, 5 Sep 2012 14:29:52 +0200,
Thomas Petazzoni <[email protected]> a ?crit :

Quoted 17 lines

Le Wed, 5 Sep 2012 14:22:44 +0200,
Eric B?nard <[email protected]> a ?crit :

Quoted 7 lines

Quoted 4 lines

Quoted 1 line

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:

Quoted 9 lines

*) 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 ?

Signed-off-by Maxime Ripard <[email protected]>

---
 drivers/gpio/gpio-74x164.c |   13 +------------
 1 file changed, 1 insertion(+), 12 deletions(-)

drivers/gpio/gpio-74x164.c +1 -12

index a31ad6f..2975036 100644
@@ -159,18 +159,7 @@ static struct spi_driver gen_74x164_driver = {
 	.probe		= gen_74x164_probe,
 	.remove		= __devexit_p(gen_74x164_remove),
 };
-
-static int __init gen_74x164_init(void)
-{
-	return spi_register_driver(&gen_74x164_driver);
-}
-subsys_initcall(gen_74x164_init);
-
-static void __exit gen_74x164_exit(void)
-{
-	spi_unregister_driver(&gen_74x164_driver);
-}
-module_exit(gen_74x164_exit);
+module_spi_driver(gen_74x164_driver);
 
 MODULE_AUTHOR("Gabor Juhos <[email protected]>");
 MODULE_AUTHOR("Miguel Gaio <[email protected]>");

Signature · 7 lines

1.7.9.5


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Signed-off-by Maxime Ripard <[email protected]>

Signed-off-by Maxime Ripard <[email protected]>

---
 drivers/gpio/gpio-74x164.c |   13 +------------
 1 file changed, 1 insertion(+), 12 deletions(-)

Quoted 4 lines · Maxime Ripard

Signed-off-by Maxime Ripard <[email protected]>

---
 drivers/gpio/gpio-74x164.c |   13 +------------
 1 file changed, 1 insertion(+), 12 deletions(-)

Acked-by Linus Walleij <[email protected]>

I guess all of these will go through your kernel tree? Tell me if you want me to apply these to my GPIO tree.

Hi Linus,
Le 06/09/2012 09:21, Linus Walleij a ?crit :

Quoted 5 lines

Acked-by Linus Walleij <[email protected]>

I guess all of these will go through your kernel tree? Tell me if you want me to apply these to my GPIO tree.

Since I don't have any kernel tree, I guess you can apply them in your tree :)