spi: bcm2835: reduce the abuse of the GPIO API
Subsystems (4)
PATCH v2 1/1 spi: bcm2835: reduce the abuse of the GPIO API
From: Bartosz Golaszewski <[email protected]>
Currently the bcm2835 SPI driver uses functions that are available exclusively to GPIO providers as a way to handle a platform quirk. Let's use a slightly better alternative that avoids poking around in GPIOLIB's internals and use GPIO lookup tables.
---
This is only build-tested. It should work, but it would be great if someone from broadcom could test this.
v1 -> v2:
- don't use devres for managing the GPIO but put it manually in .cleanup()
- add a mailing list link explaining the background of the bug
- fix kerneldoc
drivers/spi/spi-bcm2835.c | 59 +++++++++++++++++++++++---------------- 1 file changed, 35 insertions(+), 24 deletions(-)
drivers/spi/spi-bcm2835.c +35 -24
@@ -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 */
@@ -83,6 +85,7 @@ MODULE_PARM_DESC(polling_limit_us,* struct bcm2835_spi - BCM2835 SPI controller * @regs: base address of register map * @clk: core clock, divided to calculate serial clock +* @cs_gpio: chip-select GPIO descriptor * @clk_hz: core clock cached speed * @irq: interrupt, signals TX FIFO empty or RX FIFO ¾ full * @tfr: SPI transfer currently processed
@@ -117,6 +120,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,15 +1160,11 @@ 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);struct spi_controller *ctlr = spi->controller; +struct bcm2835_spi *bs =spi_controller_get_devdata (ctlr);if (target->clear_rx_desc)dmaengine_desc_free (target->clear_rx_desc);
@@ -1175,6 +1175,9 @@ static void bcm2835_spi_cleanup(struct spi_device *spi)sizeof (u32), DMA_TO_DEVICE); +gpiod_put (bs->cs_gpio); +spi_set_csgpiod (spi,0 ,NULL ); +kfree (target); }
@@ -1221,7 +1224,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 +1291,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? +* More on the problem that it addresses: +* https://www.spinics.net/lists/linux-gpio/msg36218.html */ + 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 =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 ));
Signature · 1 line
2.39.2
Quoted 11 lines · Bartosz Golaszewski
From: Bartosz Golaszewski <[email protected]>
Currently the bcm2835 SPI driver uses functions that are available exclusively to GPIO providers as a way to handle a platform quirk. Let's use a slightly better alternative that avoids poking around in GPIOLIB's internals and use GPIO lookup tables.
Signed-off-by Bartosz Golaszewski <[email protected]>+#include <
linux/cleanup.h>
(...)
drivers/spi/spi-bcm2835.c static int bcm2835_spi_setup(struct spi_device *spi) · line 1227
Quoted 2 lines
drivers/spi/spi-bcm2835.c +1
-1-struct gpio_chip *chip; + struct gpiod_lookup_table *lookup __free(kfree) = NULL;
Whoa!
This is really neat.
As noted, it will confuse static checkers at no end, but they just have to adopt. (CC to Dan C if he now runs into this.)
Quoted 1 line
+
gpiod_add_lookup_table(lookup);
Maybe we should mention the obvious advantage to the previous
hack: if there is a "cs-gpios" in the device tree, it will take precedence, because gpiod_find_and_request() will try gpiod_find_by_fwnode() *first* and only if this fails it will fall back to gpiod_find().
Hm, maybe we should go and fix these device trees? :P
Yours,
Linus Walleij
Mark Brown
maintainerQuoted 8 lines · Linus Walleij
Quoted 1 line · Bartosz Golaszewski
+ struct
gpiod_lookup_table*lookup__free(kfree)= NULL;Whoa!
This is really neat.
As noted, it will confuse static checkers at no end, but they just have to adopt. (CC to Dan C if he now runs into this.)
It also doesn't look amazing for humans, it's very not C like...
Quoted 4 lines · Mark Brown
Quoted 1 line · Linus Walleij
On Fri, Sep 1, 2023 at 1:15 PM Bartosz Golaszewski <[email protected]> wrote:
Quoted 8 lines
Quoted 6 lines
Quoted 1 line
+ struct
gpiod_lookup_table*lookup__free(kfree)= NULL;Whoa!
This is really neat.
As noted, it will confuse static checkers at no end, but they just have to adopt. (CC to Dan C if he now runs into this.)It also doesn't look amazing for humans, it's very not C like...
Once it's widely adopted, you'll see how much clearer and less prone to bugs in error paths the code becomes with autopointers. It's 2023 dammit, if we can't have flying cars, let's at least get some RAII into the kernel. :)
Bart
Mark Brown
maintainerQuoted 10 lines · Bartosz Golaszewski
Quoted 8 lines · Mark Brown
Quoted 7 lines · Linus Walleij
Quoted 1 line · Bartosz Golaszewski
+ struct
gpiod_lookup_table*lookup__free(kfree)= NULL;This is really neat.
As noted, it will confuse static checkers at no end, but they just have to adopt. (CC to Dan C if he now runs into this.)
Quoted 6 lines
Quoted 1 line
It also doesn't look amazing for humans, it's very not C like...
Once it's widely adopted, you'll see how much clearer and less prone to bugs in error paths the code becomes with autopointers. It's 2023 dammit, if we can't have flying cars, let's at least get some RAII into the kernel. :)
I use RAII extensively with other languages, I also know that with C you have to be careful and look to make sure everything is freed.
Quoted 8 lines · Mark Brown
Quoted 6 lines
Quoted 1 line
+ struct
gpiod_lookup_table*lookup__free(kfree)= NULL;Whoa!
This is really neat.
As noted, it will confuse static checkers at no end, but they just have to adopt. (CC to Dan C if he now runs into this.)It also doesn't look amazing for humans, it's very not C like...
<linux/cleanup.h> and the __free() macro was introduced by Peter Z who also very famously suggested (OTOMH!) that instead of adopting Rust to the kernel it would be possible to bring the desired Rust features into C.
Which is what he does, well the feature has been there for a while but he identified it and made it easily accessible.
Now if this path is desirable ... yeah. Maybe a matter of taste.
Yours,
Linus Walleij
Quoted 9 lines · Linus Walleij
Quoted 7 lines · Mark Brown
Quoted 6 lines
Quoted 1 line
+ struct
gpiod_lookup_table*lookup__free(kfree)= NULL;Whoa!
This is really neat.
As noted, it will confuse static checkers at no end, but they just have to adopt. (CC to Dan C if he now runs into this.)
Quoted 6 lines
Quoted 1 line
It also doesn't look amazing for humans, it's very not C like...
<
linux/cleanup.h> and the__free()macro was introduced by Peter Z who also very famously suggested (OTOMH!) that instead of adopting Rust to the kernel it would be possible to bring the desired Rust features into C.
Competition is always good :-)
Quoted 4 lines
Which is what he does, well the feature has been there for a while but he identified it and made it easily accessible.
Now if this path is desirable ... yeah. Maybe a matter of taste.
Signature · 2 lines
With Best Regards, Andy Shevchenko
Quoted 12 lines · Linus Walleij
Quoted 9 lines · Bartosz Golaszewski
From: Bartosz Golaszewski <[email protected]>
Currently the bcm2835 SPI driver uses functions that are available exclusively to GPIO providers as a way to handle a platform quirk. Let's use a slightly better alternative that avoids poking around in GPIOLIB's internals and use GPIO lookup tables.
Quoted 7 lines
Quoted 1 line
+#include <
linux/cleanup.h>(...)
drivers/spi/spi-bcm2835.c static int bcm2835_spi_setup(struct spi_device *spi) · line 1227
Quoted 2 lines
drivers/spi/spi-bcm2835.c +1
-1-struct gpio_chip *chip; + struct gpiod_lookup_table *lookup __free(kfree) = NULL;Whoa!
This is really neat.
Yeah. I like this stuff. It should fix a lot of issues. In some ways I prefer this kind of clean up to devm_ managed resources.
Quoted 2 lines
As noted, it will confuse static checkers at no end, but they just have to adopt. (CC to Dan C if he now runs into this.)
I think I updated Smatch to parse this correctly. I've tested on this patch and it seems to work okay. There probably will be some fall out in weird corners of Smatch. Let me know if you see any problems.
regards,
dan carpenter
_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel