| Message ID | 20230901111548.12733-1-brgl@bgdev.pl (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [RFT,v2] spi: bcm2835: reduce the abuse of the GPIO API | expand |
On Fri, Sep 1, 2023 at 1:15 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > 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. > > Link: https://www.spinics.net/lists/linux-gpio/msg36218.html > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > +#include <linux/cleanup.h> (...) > - 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.) > + 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 Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
On Fri, Sep 01, 2023 at 02:15:39PM +0200, Linus Walleij wrote: > On Fri, Sep 1, 2023 at 1:15 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > + 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...
On Fri, Sep 1, 2023 at 2:31 PM Mark Brown <broonie@kernel.org> wrote: > > On Fri, Sep 01, 2023 at 02:15:39PM +0200, Linus Walleij wrote: > > On Fri, Sep 1, 2023 at 1:15 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > > > + 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
On Fri, Sep 01, 2023 at 02:33:03PM +0200, Bartosz Golaszewski wrote: > On Fri, Sep 1, 2023 at 2:31 PM Mark Brown <broonie@kernel.org> wrote: > > On Fri, Sep 01, 2023 at 02:15:39PM +0200, Linus Walleij wrote: > > > On Fri, Sep 1, 2023 at 1:15 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > > > + 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.) > > 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.
On Fri, Sep 1, 2023 at 2:31 PM Mark Brown <broonie@kernel.org> wrote: > > > + 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
On Sat, Sep 02, 2023 at 06:56:51PM +0200, Linus Walleij wrote: > On Fri, Sep 1, 2023 at 2:31 PM Mark Brown <broonie@kernel.org> wrote: > > > > > + 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. Competition is always good :-) > 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.
On Fri, Sep 01, 2023 at 02:15:39PM +0200, Linus Walleij wrote: > On Fri, Sep 1, 2023 at 1:15 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > 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. > > > > Link: https://www.spinics.net/lists/linux-gpio/msg36218.html > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > +#include <linux/cleanup.h> > (...) > > - 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. > 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
diff --git a/drivers/spi/spi-bcm2835.c b/drivers/spi/spi-bcm2835.c index e7bb2714678a..e06738705075 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 */ @@ -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));