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.
Andy Shevchenko Sept. 1, 2023, 8:55 a.m. UTC | #2 
On Fri, Sep 01, 2023 at 09:40:11AM +0200, Bartosz Golaszewski wrote:
> 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.

Maybe Lukas knows more?
Bartosz Golaszewski Sept. 1, 2023, 9:34 a.m. UTC | #3 
On Fri, Sep 1, 2023 at 11:22 AM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> 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

Thanks! I will fix the patch and add this link to the commit message.

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