spi: pxa2xx: Add support for GPIO descriptor chip selects

PATCH New Jan Kiszka 2 participants 4 messages Nov 17, 2017 – Nov 22, 2017

Subsystems (3)

SPI PXA2xx/PXA3xx support Intel LPSS (Low Power SubSystem) drivers

lore

Share & tools

lore

Jan Kiszka

author

2017-11-17 20:45 UTC #0 lore raw

PATCH 1/1 spi: pxa2xx: Add support for GPIO descriptor chip selects

From: Mika Westerberg <[email protected]>

commit 99f499cd650405bbe6a9b5386d4b11ee81514fb7 upstream.

The driver uses custom chip_info coming from platform data for chip selects implemented as GPIOs. If the system lacks board files setting up the platform data, it is not possible to use GPIOs as chip selects.

This adds support for GPIO descriptors so that regardless of the underlying firmware interface (DT, ACPI or platform data) the driver can request GPIOs used as chip selects and configure them accordingly.

The custom chip_info GPIO support is still left there to make sure the existing systems keep working as expected.

Signed-off-by Mika Westerberg <[email protected]>

Signed-off-by Mark Brown <[email protected]>

Signed-off-by Jan Kiszka <[email protected]>

---

...and another one I missed as dependency for our IOT2000.

 drivers/spi/spi-pxa2xx.c | 57 +++++++++++++++++++++++++++++++++++++++++++++---
 drivers/spi/spi-pxa2xx.h |  3 +++
 2 files changed, 57 insertions(+), 3 deletions(-)

drivers/spi/spi-pxa2xx.c +54 -3

index 3cac73e4c3e4..21339ca204d9 100644
@@ -1122,9 +1122,26 @@ static int pxa2xx_spi_unprepare_transfer(struct spi_master *master)
 static int setup_cs(struct spi_device *spi, struct chip_data *chip,
 		    struct pxa2xx_spi_chip *chip_info)
 {
+	struct driver_data *drv_data = spi_master_get_devdata(spi->master);
 	int err = 0;
 
-	if (chip == NULL || chip_info == NULL)
+	if (chip == NULL)
+		return 0;
+
+	if (drv_data->cs_gpiods) {
+		struct gpio_desc *gpiod;
+
+		gpiod = drv_data->cs_gpiods[spi->chip_select];
+		if (gpiod) {
+			chip->gpio_cs = desc_to_gpio(gpiod);
+			chip->gpio_cs_inverted = spi->mode & SPI_CS_HIGH;
+			gpiod_set_value(gpiod, chip->gpio_cs_inverted);
+		}
+
+		return 0;
+	}
+
+	if (chip_info == NULL)
 		return 0;
 
 	/* NOTE: setup() can be called multiple times, possibly with

@@ -1305,7 +1322,8 @@ static void cleanup(struct spi_device *spi)
 	if (!chip)
 		return;
 
-	if (drv_data->ssp_type != CE4100_SSP && gpio_is_valid(chip->gpio_cs))
+	if (drv_data->ssp_type != CE4100_SSP && !drv_data->cs_gpiods &&
+	    gpio_is_valid(chip->gpio_cs))
 		gpio_free(chip->gpio_cs);
 
 	kfree(chip);

@@ -1453,7 +1471,7 @@ static int pxa2xx_spi_probe(struct platform_device *pdev)
 	struct driver_data *drv_data;
 	struct ssp_device *ssp;
 	const struct lpss_config *config;
-	int status;
+	int status, count;
 	u32 tmp;
 
 	platform_info = dev_get_platdata(dev);

@@ -1590,6 +1608,39 @@ static int pxa2xx_spi_probe(struct platform_device *pdev)
 	}
 	master->num_chipselect = platform_info->num_chipselect;
 
+	count = gpiod_count(&pdev->dev, "cs");
+	if (count > 0) {
+		int i;
+
+		master->num_chipselect = max_t(int, count,
+			master->num_chipselect);
+
+		drv_data->cs_gpiods = devm_kcalloc(&pdev->dev,
+			master->num_chipselect, sizeof(struct gpio_desc *),
+			GFP_KERNEL);
+		if (!drv_data->cs_gpiods) {
+			status = -ENOMEM;
+			goto out_error_clock_enabled;
+		}
+
+		for (i = 0; i < master->num_chipselect; i++) {
+			struct gpio_desc *gpiod;
+
+			gpiod = devm_gpiod_get_index(dev, "cs", i,
+						     GPIOD_OUT_HIGH);
+			if (IS_ERR(gpiod)) {
+				/* Means use native chip select */
+				if (PTR_ERR(gpiod) == -ENOENT)
+					continue;
+
+				status = (int)PTR_ERR(gpiod);
+				goto out_error_clock_enabled;
+			} else {
+				drv_data->cs_gpiods[i] = gpiod;
+			}
+		}
+	}
+
 	tasklet_init(&drv_data->pump_transfers, pump_transfers,
 		     (unsigned long)drv_data);

drivers/spi/spi-pxa2xx.h +3 -0

index 58efa98313aa..1320538f8623 100644
@@ -80,6 +80,9 @@ struct driver_data {
 	void (*cs_control)(u32 command);
 
 	void __iomem *lpss_base;
+
+	/* GPIOs for chip selects */
+	struct gpio_desc **cs_gpiods;
 };
 
 struct chip_data {

Ben Hutchings

2017-11-22 16:53 UTC #1 lore raw

PATCH 1/1 spi: pxa2xx: Add support for GPIO descriptor chip selects

Quoted 38 lines · Jan Kiszka

From: Mika Westerberg <[email protected]>
commit 99f499cd650405bbe6a9b5386d4b11ee81514fb7 upstream.
The driver uses custom chip_info coming from platform data for chip selects implemented as GPIOs. If the system lacks board files setting up the platform data, it is not possible to use GPIOs as chip selects.
This adds support for GPIO descriptors so that regardless of the underlying firmware interface (DT, ACPI or platform data) the driver can request GPIOs used as chip selects and configure them accordingly.
The custom chip_info GPIO support is still left there to make sure the existing systems keep working as expected.
Signed-off-by Mika Westerberg <[email protected]>
Signed-off-by Mark Brown <[email protected]>
Signed-off-by Jan Kiszka <[email protected]>
---
...and another one I missed as dependency for our IOT2000.
?drivers/spi/spi-pxa2xx.c | 57 +++++++++++++++++++++++++++++++++++++++++++++--- ?drivers/spi/spi-pxa2xx.h |??3 +++
?2 files changed, 57 insertions(+), 3 deletions(-)
drivers/spi/spi-pxa2xx.c +1 -0
index 3cac73e4c3e4..21339ca204d9 100644
@@ -1122,9 +1122,26 @@ static int pxa2xx_spi_unprepare_transfer(struct spi_master *master)
 ?static int setup_cs(struct spi_device *spi, struct chip_data *chip,
 ?		????struct pxa2xx_spi_chip *chip_info)
 ?{
+	struct driver_data *drv_data = spi_master_get_devdata(spi->master);
 ?	int err = 0;
?
drivers/spi/spi-pxa2xx.c +0 -1
-	if (chip == NULL || chip_info == NULL)

drivers/spi/spi-pxa2xx.c static int pxa2xx_spi_unprepare_transfer(struct spi_master *master) · line 1137

Quoted 10 lines

drivers/spi/spi-pxa2xx.c +10 -0

+	if (chip == NULL)
+		return 0;
+
+	if (drv_data->cs_gpiods) {
+		struct gpio_desc *gpiod;
+
+		gpiod = drv_data->cs_gpiods[spi->chip_select];
+		if (gpiod) {
+			chip->gpio_cs = desc_to_gpio(gpiod);
+			chip->gpio_cs_inverted = spi->mode & SPI_CS_HIGH;

This assigns a value of 0 or 4 to chip->gpio_cs_inverted, when it surely should be 0 or 1. With the current gpiolib implementation this appears to work anyway, but I wouldn't want to assume that's true.

[...]

Quoted 8 lines

drivers/spi/spi-pxa2xx.c +0 -0
@@ -1305,7 +1322,8 @@ static void cleanup(struct spi_device *spi)
 ????????if (!chip)
 ????????????????return;
?
-???????if (drv_data->ssp_type != CE4100_SSP && gpio_is_valid(chip->gpio_cs)) +???????if (drv_data->ssp_type != CE4100_SSP && !drv_data->cs_gpiods && +???????????gpio_is_valid(chip->gpio_cs)) ????????????????gpio_free(chip->gpio_cs);

[...]

This is really weird; I think the driver should consistently use managed or unmanaged allocation of GPIOs.

But I don't think these are blockers for backporting, so I've applied this anyway.

Ben.

Signature · 2 lines

Ben Hutchings
Software Developer, Codethink Ltd.

Jan Kiszka

author

2017-11-22 17:01 UTC #2 lore raw

PATCH 1/1 spi: pxa2xx: Add support for GPIO descriptor chip selects

Quoted 45 lines · Ben Hutchings

Quoted 44 lines · Jan Kiszka
From: Mika Westerberg <[email protected]>
commit 99f499cd650405bbe6a9b5386d4b11ee81514fb7 upstream.
The driver uses custom chip_info coming from platform data for chip selects implemented as GPIOs. If the system lacks board files setting up the platform data, it is not possible to use GPIOs as chip selects.
This adds support for GPIO descriptors so that regardless of the underlying firmware interface (DT, ACPI or platform data) the driver can request GPIOs used as chip selects and configure them accordingly.
The custom chip_info GPIO support is still left there to make sure the existing systems keep working as expected.
Signed-off-by Mika Westerberg <[email protected]>
Signed-off-by Mark Brown <[email protected]>
Signed-off-by Jan Kiszka <[email protected]>
---
...and another one I missed as dependency for our IOT2000.
?drivers/spi/spi-pxa2xx.c | 57 +++++++++++++++++++++++++++++++++++++++++++++--- ?drivers/spi/spi-pxa2xx.h |??3 +++
?2 files changed, 57 insertions(+), 3 deletions(-)
drivers/spi/spi-pxa2xx.c +1 -0
index 3cac73e4c3e4..21339ca204d9 100644
@@ -1122,9 +1122,26 @@ static int pxa2xx_spi_unprepare_transfer(struct spi_master *master)
 ?static int setup_cs(struct spi_device *spi, struct chip_data *chip,
 ?		????struct pxa2xx_spi_chip *chip_info)
 ?{
+	struct driver_data *drv_data = spi_master_get_devdata(spi->master);
 ?	int err = 0;
?
drivers/spi/spi-pxa2xx.c +6 -1
-	if (chip == NULL || chip_info == NULL)
+	if (chip == NULL)
+		return 0;
+
+	if (drv_data->cs_gpiods) {
+		struct gpio_desc *gpiod;
+

Quoted 8 lines

drivers/spi/spi-pxa2xx.c static int pxa2xx_spi_unprepare_transfer(struct spi_master *master) · line 1137
Quoted 4 lines
drivers/spi/spi-pxa2xx.c +4 -0
+		gpiod = drv_data->cs_gpiods[spi->chip_select];
+		if (gpiod) {
+			chip->gpio_cs = desc_to_gpio(gpiod);
+			chip->gpio_cs_inverted = spi->mode & SPI_CS_HIGH;
This assigns a value of 0 or 4 to chip->gpio_cs_inverted, when it surely should be 0 or 1. With the current gpiolib implementation this appears to work anyway, but I wouldn't want to assume that's true.

Indeed.

Quoted 12 lines

[...]
Quoted 8 lines
drivers/spi/spi-pxa2xx.c +0 -0
@@ -1305,7 +1322,8 @@ static void cleanup(struct spi_device *spi)
 ????????if (!chip)
 ????????????????return;
?
-???????if (drv_data->ssp_type != CE4100_SSP && gpio_is_valid(chip->gpio_cs)) +???????if (drv_data->ssp_type != CE4100_SSP && !drv_data->cs_gpiods && +???????????gpio_is_valid(chip->gpio_cs)) ????????????????gpio_free(chip->gpio_cs);
[...]

Quoted 2 lines

This is really weird; I think the driver should consistently use managed or unmanaged allocation of GPIOs.

Yes, there is another patch in upstream to improve this:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=c18d925fca20d33c3d04e5002a883f62d699543e

Quoted 4 lines

But I don't think these are blockers for backporting, so I've applied this anyway.

Thanks (also for cross-checking),
Jan

Ben Hutchings

2017-11-22 17:15 UTC #3 lore raw

PATCH 1/1 spi: pxa2xx: Add support for GPIO descriptor chip selects

Quoted 70 lines · Jan Kiszka

Quoted 38 lines · Ben Hutchings
Quoted 37 lines · Jan Kiszka
From: Mika Westerberg <[email protected]>
commit 99f499cd650405bbe6a9b5386d4b11ee81514fb7 upstream.
The driver uses custom chip_info coming from platform data for chip selects implemented as GPIOs. If the system lacks board files setting up the platform data, it is not possible to use GPIOs as chip selects.
This adds support for GPIO descriptors so that regardless of the underlying firmware interface (DT, ACPI or platform data) the driver can request GPIOs used as chip selects and configure them accordingly.
The custom chip_info GPIO support is still left there to make sure the existing systems keep working as expected.
Signed-off-by Mika Westerberg <[email protected]>
Signed-off-by Mark Brown <[email protected]>
Signed-off-by Jan Kiszka <[email protected]>
---
...and another one I missed as dependency for our IOT2000.
?drivers/spi/spi-pxa2xx.c | 57 +++++++++++++++++++++++++++++++++++++++++++++--- ?drivers/spi/spi-pxa2xx.h |??3 +++
?2 files changed, 57 insertions(+), 3 deletions(-)
drivers/spi/spi-pxa2xx.c +0 -0
index 3cac73e4c3e4..21339ca204d9 100644
@@ -1122,9 +1122,26 @@ static int pxa2xx_spi_unprepare_transfer(struct spi_master *master)
 ?static int setup_cs(struct spi_device *spi, struct chip_data *chip,
 > > > ?		????struct pxa2xx_spi_chip *chip_info)
 ?{
 > > > +	struct driver_data *drv_data = spi_master_get_devdata(spi->master);
 > > > ?	int err = 0;
?
Quoted 15 lines
drivers/spi/spi-pxa2xx.c static int pxa2xx_spi_unprepare_transfer(struct spi_master *master) · line 1137
Quoted 11 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
drivers/spi/spi-pxa2xx.c +2 -1
-	if (chip == NULL || chip_info == NULL)
+	if (chip == NULL)
+		return 0;
drivers/spi/spi-pxa2xx.c +8 -0
+
+	if (drv_data->cs_gpiods) {
+		struct gpio_desc *gpiod;
+
+		gpiod = drv_data->cs_gpiods[spi->chip_select];
+		if (gpiod) {
+			chip->gpio_cs = desc_to_gpio(gpiod);
+			chip->gpio_cs_inverted = spi->mode & SPI_CS_HIGH;
This assigns a value of 0 or 4 to chip->gpio_cs_inverted, when it surely should be 0 or 1.??With the current gpiolib implementation this appears to work anyway, but I wouldn't want to assume that's true.
Indeed.
Quoted 13 lines
[...]
Quoted 8 lines
drivers/spi/spi-pxa2xx.c +0 -0
@@ -1305,7 +1322,8 @@ static void cleanup(struct spi_device *spi)
 ????????if (!chip)
 ????????????????return;
?
-???????if (drv_data->ssp_type != CE4100_SSP && gpio_is_valid(chip->gpio_cs)) +???????if (drv_data->ssp_type != CE4100_SSP && !drv_data->cs_gpiods && +???????????gpio_is_valid(chip->gpio_cs)) ????????????????gpio_free(chip->gpio_cs);
[...]

Quoted 6 lines

Quoted 2 lines
This is really weird; I think the driver should consistently use managed or unmanaged allocation of GPIOs.
Yes, there is another patch in upstream to improve this:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=c18d925fca20d33c3d04e5002a883f62d699543e

I'm talking about use of devm_gpio_get() (with automatic free) vs gpio_request()/gpio_free().

Ben.

Quoted 8 lines

Quoted 4 lines
But I don't think these are blockers for backporting, so I've applied this anyway.
Thanks (also for cross-checking),
Jan

Signature · 2 lines

Ben Hutchings
Software Developer, Codethink Ltd.