diff mbox series

[RFT] spi: bcm2835: reduce the abuse of the GPIO API

Message ID 20230831194934.19628-1-brgl@bgdev.pl
State New
Headers show
Series [RFT] spi: bcm2835: reduce the abuse of the GPIO API | expand

Commit Message

Bartosz Golaszewski Aug. 31, 2023, 7:49 p.m. UTC 
From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Currently the bcm2835 SPI driver uses functions meant for GPIO providers
exclusively to locate the GPIO chip it gets its CS pins from and request
the relevant pin. I don't know the background and what bug forced this.
I can however propose a slightly better solution that allows the driver
to request the GPIO correctly using a temporary lookup table.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
This is only build-tested. It should work, but it would be great if
someone from broadcom could test this.

 drivers/spi/spi-bcm2835.c | 54 ++++++++++++++++++++++-----------------
 1 file changed, 30 insertions(+), 24 deletions(-)

Comments

Andy Shevchenko Aug. 31, 2023, 11:27 p.m. UTC | #1 
On Thu, Aug 31, 2023 at 09:49:34PM +0200, Bartosz Golaszewski wrote:

> This is only build-tested. It should work, but it would be great if
> someone from broadcom could test this.

Side note: Seems your build test setup misses kernel-doc validation.
With `scripts/kernel-doc -v -none -Wall ...` you can get some warnings.
Bartosz Golaszewski Sept. 1, 2023, 7:40 a.m. UTC | #2 
On Fri, Sep 1, 2023 at 1:25 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Thu, Aug 31, 2023 at 09:49:34PM +0200, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > Currently the bcm2835 SPI driver uses functions meant for GPIO providers
> > exclusively to locate the GPIO chip it gets its CS pins from and request
> > the relevant pin. I don't know the background and what bug forced this.
>
> ...
>
> >       /*
> > +      * TODO: The code below is a slightly better alternative to the utter
> > +      * abuse of the GPIO API that I found here before. It creates a
> > +      * temporary lookup table, assigns it to the SPI device, gets the GPIO
> > +      * descriptor and then releases the lookup table.
> >        *
> > +      * Still the real problem is unsolved. Looks like the cs_gpiods table
> > +      * is not assigned correctly from DT?
> >        */
>
> I'm not sure why this quirk is here. AFAIR the SPI CS quirks are located in
> gpiolib-of.c.
>

I'm not sure this is a good candidate for the GPIOLIB quirks. This is
the SPI setup callback (which makes me think - I should have used
gpiod_get(), not devm_gpiod_get() and then put the descriptor in
.cleanup()) and not probe. It would be great to get some background on
why this is even needed in the first place. The only reason I see is
booting the driver with an invalid device-tree that doesn't assign the
GPIO to the SPI controller.

Bart
Linus Walleij Sept. 1, 2023, 9:22 a.m. UTC | #3 
On Fri, Sep 1, 2023 at 10:55 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:

> > I'm not sure this is a good candidate for the GPIOLIB quirks. This is
> > the SPI setup callback (which makes me think - I should have used
> > gpiod_get(), not devm_gpiod_get() and then put the descriptor in
> > .cleanup()) and not probe. It would be great to get some background on
> > why this is even needed in the first place. The only reason I see is
> > booting the driver with an invalid device-tree that doesn't assign the
> > GPIO to the SPI controller.
>
> Maybe Lukas knows more?

He does!
The background can be found here:
https://www.spinics.net/lists/linux-gpio/msg36218.html
(hm this "spinics" archive should be imported to lore...)

Yours,
Linus Walleij
diff mbox series

Patch

diff --git a/drivers/spi/spi-bcm2835.c b/drivers/spi/spi-bcm2835.c
index e7bb2714678a..3c422f0e1087 100644
--- a/drivers/spi/spi-bcm2835.c
+++ b/drivers/spi/spi-bcm2835.c
@@ -11,6 +11,7 @@ 
  * spi-atmel.c, Copyright (C) 2006 Atmel Corporation
  */
 
+#include <linux/cleanup.h>
 #include <linux/clk.h>
 #include <linux/completion.h>
 #include <linux/debugfs.h>
@@ -26,9 +27,10 @@ 
 #include <linux/of_address.h>
 #include <linux/platform_device.h>
 #include <linux/gpio/consumer.h>
-#include <linux/gpio/machine.h> /* FIXME: using chip internals */
-#include <linux/gpio/driver.h> /* FIXME: using chip internals */
+#include <linux/gpio/machine.h> /* FIXME: using GPIO lookup tables */
 #include <linux/of_irq.h>
+#include <linux/overflow.h>
+#include <linux/slab.h>
 #include <linux/spi/spi.h>
 
 /* SPI register offsets */
@@ -117,6 +119,7 @@  MODULE_PARM_DESC(polling_limit_us,
 struct bcm2835_spi {
 	void __iomem *regs;
 	struct clk *clk;
+	struct gpio_desc *cs_gpio;
 	unsigned long clk_hz;
 	int irq;
 	struct spi_transfer *tfr;
@@ -1156,11 +1159,6 @@  static void bcm2835_spi_handle_err(struct spi_controller *ctlr,
 	bcm2835_spi_reset_hw(bs);
 }
 
-static int chip_match_name(struct gpio_chip *chip, void *data)
-{
-	return !strcmp(chip->label, data);
-}
-
 static void bcm2835_spi_cleanup(struct spi_device *spi)
 {
 	struct bcm2835_spidev *target = spi_get_ctldata(spi);
@@ -1221,7 +1219,7 @@  static int bcm2835_spi_setup(struct spi_device *spi)
 	struct spi_controller *ctlr = spi->controller;
 	struct bcm2835_spi *bs = spi_controller_get_devdata(ctlr);
 	struct bcm2835_spidev *target = spi_get_ctldata(spi);
-	struct gpio_chip *chip;
+	struct gpiod_lookup_table *lookup __free(kfree) = NULL;
 	int ret;
 	u32 cs;
 
@@ -1288,29 +1286,37 @@  static int bcm2835_spi_setup(struct spi_device *spi)
 	}
 
 	/*
-	 * Translate native CS to GPIO
+	 * TODO: The code below is a slightly better alternative to the utter
+	 * abuse of the GPIO API that I found here before. It creates a
+	 * temporary lookup table, assigns it to the SPI device, gets the GPIO
+	 * descriptor and then releases the lookup table.
 	 *
-	 * FIXME: poking around in the gpiolib internals like this is
-	 * not very good practice. Find a way to locate the real problem
-	 * and fix it. Why is the GPIO descriptor in spi->cs_gpiod
-	 * sometimes not assigned correctly? Erroneous device trees?
+	 * Still the real problem is unsolved. Looks like the cs_gpiods table
+	 * is not assigned correctly from DT?
 	 */
+	lookup = kzalloc(struct_size(lookup, table, 1), GFP_KERNEL);
+	if (!lookup) {
+		ret = -ENOMEM;
+		goto err_cleanup;
+	}
 
-	/* get the gpio chip for the base */
-	chip = gpiochip_find("pinctrl-bcm2835", chip_match_name);
-	if (!chip)
-		return 0;
+	lookup->dev_id = dev_name(&spi->dev);
+	lookup->table[0].key = "pinctrl-bcm2835";
+	lookup->table[0].chip_hwnum = (8 - (spi_get_chipselect(spi, 0)));
+	lookup->table[0].con_id = "cs";
+	lookup->table[0].flags = GPIO_LOOKUP_FLAGS_DEFAULT;
 
-	spi_set_csgpiod(spi, 0, gpiochip_request_own_desc(chip,
-							  8 - (spi_get_chipselect(spi, 0)),
-							  DRV_NAME,
-							  GPIO_LOOKUP_FLAGS_DEFAULT,
-							  GPIOD_OUT_LOW));
-	if (IS_ERR(spi_get_csgpiod(spi, 0))) {
-		ret = PTR_ERR(spi_get_csgpiod(spi, 0));
+	gpiod_add_lookup_table(lookup);
+
+	bs->cs_gpio = devm_gpiod_get(&spi->dev, "cs", GPIOD_OUT_LOW);
+	gpiod_remove_lookup_table(lookup);
+	if (IS_ERR(bs->cs_gpio)) {
+		ret = PTR_ERR(bs->cs_gpio);
 		goto err_cleanup;
 	}
 
+	spi_set_csgpiod(spi, 0, bs->cs_gpio);
+
 	/* and set up the "mode" and level */
 	dev_info(&spi->dev, "setting up native-CS%i to use GPIO\n",
 		 spi_get_chipselect(spi, 0));