From: Bartosz Golaszewski <brgl@bgdev.pl> To: Mark Brown <broonie@kernel.org>, Florian Fainelli <florian.fainelli@broadcom.com>, Andy Shevchenko <andriy.shevchenko@linux.intel.com>, Linus Walleij <linus.walleij@linaro.org>, Ray Jui <rjui@broadcom.com>, Scott Branden <sbranden@broadcom.com> Cc: Broadcom internal kernel review list <bcm-kernel-feedback-list@broadcom.com>, linux-spi@vger.kernel.org, linux-rpi-kernel@lists.infradead.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, linux-gpio@vger.kernel.org, Bartosz Golaszewski <bartosz.golaszewski@linaro.org> Subject: [PATCH v3] spi: bcm2835: reduce the abuse of the GPIO API Date: Mon, 11 Sep 2023 18:15:53 +0200 [thread overview] Message-ID: <20230911161553.24313-1-brgl@bgdev.pl> (raw) 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> --- This is only build-tested. It should work, but it would be great if someone from broadcom could test this. Andy pointed out elsewhere that we can use GPIO_LOOKUP() even in dynamically allocated tables, hence v3. 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 v2 -> v3: - use GPIO_LOOKUP() macro for creating the lookup entry as it looks better and results in less LOC drivers/spi/spi-bcm2835.c | 60 +++++++++++++++++++++++---------------- 1 file changed, 35 insertions(+), 25 deletions(-) diff --git a/drivers/spi/spi-bcm2835.c b/drivers/spi/spi-bcm2835.c index e7bb2714678a..dfd9c4997052 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,36 @@ 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; + } + + lookup->dev_id = dev_name(&spi->dev); + lookup->table[0] = GPIO_LOOKUP("pinctrl-bcm2835", + 8 - (spi_get_chipselect(spi, 0)), + "cs", GPIO_LOOKUP_FLAGS_DEFAULT); - /* get the gpio chip for the base */ - chip = gpiochip_find("pinctrl-bcm2835", chip_match_name); - if (!chip) - return 0; - - 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)); -- 2.39.2 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
WARNING: multiple messages have this Message-ID (diff)
From: Bartosz Golaszewski <brgl@bgdev.pl> To: Mark Brown <broonie@kernel.org>, Florian Fainelli <florian.fainelli@broadcom.com>, Andy Shevchenko <andriy.shevchenko@linux.intel.com>, Linus Walleij <linus.walleij@linaro.org>, Ray Jui <rjui@broadcom.com>, Scott Branden <sbranden@broadcom.com> Cc: Broadcom internal kernel review list <bcm-kernel-feedback-list@broadcom.com>, linux-spi@vger.kernel.org, linux-rpi-kernel@lists.infradead.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, linux-gpio@vger.kernel.org, Bartosz Golaszewski <bartosz.golaszewski@linaro.org> Subject: [PATCH v3] spi: bcm2835: reduce the abuse of the GPIO API Date: Mon, 11 Sep 2023 18:15:53 +0200 [thread overview] Message-ID: <20230911161553.24313-1-brgl@bgdev.pl> (raw) 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> --- This is only build-tested. It should work, but it would be great if someone from broadcom could test this. Andy pointed out elsewhere that we can use GPIO_LOOKUP() even in dynamically allocated tables, hence v3. 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 v2 -> v3: - use GPIO_LOOKUP() macro for creating the lookup entry as it looks better and results in less LOC drivers/spi/spi-bcm2835.c | 60 +++++++++++++++++++++++---------------- 1 file changed, 35 insertions(+), 25 deletions(-) diff --git a/drivers/spi/spi-bcm2835.c b/drivers/spi/spi-bcm2835.c index e7bb2714678a..dfd9c4997052 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,36 @@ 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; + } + + lookup->dev_id = dev_name(&spi->dev); + lookup->table[0] = GPIO_LOOKUP("pinctrl-bcm2835", + 8 - (spi_get_chipselect(spi, 0)), + "cs", GPIO_LOOKUP_FLAGS_DEFAULT); - /* get the gpio chip for the base */ - chip = gpiochip_find("pinctrl-bcm2835", chip_match_name); - if (!chip) - return 0; - - 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)); -- 2.39.2
next reply other threads:[~2023-09-11 16:16 UTC|newest] Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top 2023-09-11 16:15 Bartosz Golaszewski [this message] 2023-09-11 16:15 ` [PATCH v3] spi: bcm2835: reduce the abuse of the GPIO API Bartosz Golaszewski 2023-09-11 16:17 ` Bartosz Golaszewski 2023-09-11 16:17 ` Bartosz Golaszewski 2023-09-18 15:45 ` Mark Brown 2023-09-18 15:45 ` Mark Brown
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20230911161553.24313-1-brgl@bgdev.pl \ --to=brgl@bgdev.pl \ --cc=andriy.shevchenko@linux.intel.com \ --cc=bartosz.golaszewski@linaro.org \ --cc=bcm-kernel-feedback-list@broadcom.com \ --cc=broonie@kernel.org \ --cc=florian.fainelli@broadcom.com \ --cc=linus.walleij@linaro.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-gpio@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-rpi-kernel@lists.infradead.org \ --cc=linux-spi@vger.kernel.org \ --cc=rjui@broadcom.com \ --cc=sbranden@broadcom.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.