spi: bcm2835: reduce the abuse of the GPIO API
Subsystems (4)
PATCH 1/1 spi: bcm2835: reduce the abuse of the GPIO API
From: Bartosz Golaszewski <[email protected]>
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.
---
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(-)
drivers/spi/spi-bcm2835.c +30 -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 */
@@ -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 ));
Signature · 1 line
2.39.2
kernel test robot
build botkernel test robot noticed the following build warnings:
Hi Bartosz,
kernel test robot noticed the following build warnings:
[auto build test WARNING on broonie-spi/for-next]
[also build test WARNING on linus/master next-20230831] [cannot apply to v6.5]
[If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Bartosz-Golaszewski/spi-bcm2835-reduce-the-abuse-of-the-GPIO-API/20230901-035139 base: https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next patch link: https://lore.kernel.org/r/20230831194934.19628-1-brgl%40bgdev.pl patch subject: [RFT PATCH] spi: bcm2835: reduce the abuse of the GPIO API config: m68k-allyesconfig (https://download.01.org/0day-ci/archive/20230901/[email protected]/config) compiler: m68k-linux-gcc (GCC) 13.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20230901/[email protected]/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All warnings (new ones prefixed by >>):
>> drivers/spi/spi-bcm2835.c:146: warning: Function parameter or member 'cs_gpio' not described in 'bcm2835_spi'
vim +146 drivers/spi/spi-bcm2835.c
f8043872e79614 Chris Boot 2013-03-11 77
ff245d90ebed8d Martin Sperl 2019-04-23 78 /* define polling limits */
cbd632ea8ee4ae Jason Yan 2020-09-12 79 static unsigned int polling_limit_us = 30;
ff245d90ebed8d Martin Sperl 2019-04-23 80 module_param(polling_limit_us, uint, 0664);
ff245d90ebed8d Martin Sperl 2019-04-23 81 MODULE_PARM_DESC(polling_limit_us,
ff245d90ebed8d Martin Sperl 2019-04-23 82 "time in us to run a transfer in polling mode\n");
ff245d90ebed8d Martin Sperl 2019-04-23 83
acf0f856959937 Lukas Wunner 2018-11-08 84 /**
acf0f856959937 Lukas Wunner 2018-11-08 85 * struct bcm2835_spi - BCM2835 SPI controller
acf0f856959937 Lukas Wunner 2018-11-08 86 * @regs: base address of register map
acf0f856959937 Lukas Wunner 2018-11-08 87 * @clk: core clock, divided to calculate serial clock
c45c1e82bba130 Alexandru Tachici 2021-07-17 88 * @clk_hz: core clock cached speed
acf0f856959937 Lukas Wunner 2018-11-08 89 * @irq: interrupt, signals TX FIFO empty or RX FIFO ¾ full
3bd7f6589f67f0 Lukas Wunner 2018-11-08 90 * @tfr: SPI transfer currently processed
afe7e36360f4c9 Robin Murphy 2020-06-16 91 * @ctlr: SPI controller reverse lookup
acf0f856959937 Lukas Wunner 2018-11-08 92 * @tx_buf: pointer whence next transmitted byte is read
acf0f856959937 Lukas Wunner 2018-11-08 93 * @rx_buf: pointer where next received byte is written
acf0f856959937 Lukas Wunner 2018-11-08 94 * @tx_len: remaining bytes to transmit
acf0f856959937 Lukas Wunner 2018-11-08 95 * @rx_len: remaining bytes to receive
3bd7f6589f67f0 Lukas Wunner 2018-11-08 96 * @tx_prologue: bytes transmitted without DMA if first TX sglist entry's
3bd7f6589f67f0 Lukas Wunner 2018-11-08 97 * length is not a multiple of 4 (to overcome hardware limitation)
3bd7f6589f67f0 Lukas Wunner 2018-11-08 98 * @rx_prologue: bytes received without DMA if first RX sglist entry's
3bd7f6589f67f0 Lukas Wunner 2018-11-08 99 * length is not a multiple of 4 (to overcome hardware limitation)
3bd7f6589f67f0 Lukas Wunner 2018-11-08 100 * @tx_spillover: whether @tx_prologue spills over to second TX sglist entry
154f7da56f1ecb Martin Sperl 2019-04-23 101 * @debugfs_dir: the debugfs directory - neede to remove debugfs when
154f7da56f1ecb Martin Sperl 2019-04-23 102 * unloading the module
154f7da56f1ecb Martin Sperl 2019-04-23 103 * @count_transfer_polling: count of how often polling mode is used
154f7da56f1ecb Martin Sperl 2019-04-23 104 * @count_transfer_irq: count of how often interrupt mode is used
154f7da56f1ecb Martin Sperl 2019-04-23 105 * @count_transfer_irq_after_polling: count of how often we fall back to
154f7da56f1ecb Martin Sperl 2019-04-23 106 * interrupt mode after starting in polling mode.
154f7da56f1ecb Martin Sperl 2019-04-23 107 * These are counted as well in @count_transfer_polling and
154f7da56f1ecb Martin Sperl 2019-04-23 108 * @count_transfer_irq
154f7da56f1ecb Martin Sperl 2019-04-23 109 * @count_transfer_dma: count how often dma mode is used
00be843bc1c3c7 Yang Yingliang 2023-07-28 110 * @target: SPI target currently selected
8259bf667a0f9e Lukas Wunner 2019-09-11 111 * (used by bcm2835_spi_dma_tx_done() to write @clear_rx_cs)
8259bf667a0f9e Lukas Wunner 2019-09-11 112 * @tx_dma_active: whether a TX DMA descriptor is in progress
8259bf667a0f9e Lukas Wunner 2019-09-11 113 * @rx_dma_active: whether a RX DMA descriptor is in progress
8259bf667a0f9e Lukas Wunner 2019-09-11 114 * (used by bcm2835_spi_dma_tx_done() to handle a race)
2b8279aec1829d Lukas Wunner 2019-09-11 115 * @fill_tx_desc: preallocated TX DMA descriptor used for RX-only transfers
2b8279aec1829d Lukas Wunner 2019-09-11 116 * (cyclically copies from zero page to TX FIFO)
2b8279aec1829d Lukas Wunner 2019-09-11 117 * @fill_tx_addr: bus address of zero page
acf0f856959937 Lukas Wunner 2018-11-08 118 */
f8043872e79614 Chris Boot 2013-03-11 119 struct bcm2835_spi {
f8043872e79614 Chris Boot 2013-03-11 120 void __iomem *regs;
f8043872e79614 Chris Boot 2013-03-11 121 struct clk *clk;
1098696c0d4d2d Bartosz Golaszewski 2023-08-31 122 struct gpio_desc *cs_gpio;
c45c1e82bba130 Alexandru Tachici 2021-07-17 123 unsigned long clk_hz;
f8043872e79614 Chris Boot 2013-03-11 124 int irq;
3bd7f6589f67f0 Lukas Wunner 2018-11-08 125 struct spi_transfer *tfr;
afe7e36360f4c9 Robin Murphy 2020-06-16 126 struct spi_controller *ctlr;
f8043872e79614 Chris Boot 2013-03-11 127 const u8 *tx_buf;
f8043872e79614 Chris Boot 2013-03-11 128 u8 *rx_buf;
e34ff011c70e5f Martin Sperl 2015-03-26 129 int tx_len;
e34ff011c70e5f Martin Sperl 2015-03-26 130 int rx_len;
3bd7f6589f67f0 Lukas Wunner 2018-11-08 131 int tx_prologue;
3bd7f6589f67f0 Lukas Wunner 2018-11-08 132 int rx_prologue;
b31a9299bca66c Lukas Wunner 2018-11-29 133 unsigned int tx_spillover;
154f7da56f1ecb Martin Sperl 2019-04-23 134
154f7da56f1ecb Martin Sperl 2019-04-23 135 struct dentry *debugfs_dir;
154f7da56f1ecb Martin Sperl 2019-04-23 136 u64 count_transfer_polling;
154f7da56f1ecb Martin Sperl 2019-04-23 137 u64 count_transfer_irq;
154f7da56f1ecb Martin Sperl 2019-04-23 138 u64 count_transfer_irq_after_polling;
154f7da56f1ecb Martin Sperl 2019-04-23 139 u64 count_transfer_dma;
8259bf667a0f9e Lukas Wunner 2019-09-11 140
00be843bc1c3c7 Yang Yingliang 2023-07-28 141 struct bcm2835_spidev *target;
8259bf667a0f9e Lukas Wunner 2019-09-11 142 unsigned int tx_dma_active;
8259bf667a0f9e Lukas Wunner 2019-09-11 143 unsigned int rx_dma_active;
2b8279aec1829d Lukas Wunner 2019-09-11 144 struct dma_async_tx_descriptor *fill_tx_desc;
2b8279aec1829d Lukas Wunner 2019-09-11 145 dma_addr_t fill_tx_addr;ec679bda639fe8 Lukas Wunner 2021-05-27 @146 };ec679bda639fe8 Lukas Wunner 2021-05-27 147
Signature · 2 lines
0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests/wiki
Quoted 5 lines · Bartosz Golaszewski
From: Bartosz Golaszewski <[email protected]>
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.
...
drivers/spi/spi-bcm2835.c static int bcm2835_spi_setup(struct spi_device *spi) · line 1296
Quoted 9 lines
drivers/spi/spi-bcm2835.c +6
-0/* +* 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.
Signature · 2 lines
With Best Regards, Andy Shevchenko
Quoted 20 lines · Andy Shevchenko
Quoted 5 lines · Bartosz Golaszewski
From: Bartosz Golaszewski <[email protected]>
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.
...
drivers/spi/spi-bcm2835.c static int bcm2835_spi_setup(struct spi_device *spi) · line 1296
Quoted 9 lines
/*drivers/spi/spi-bcm2835.c +6
-0+* 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? */
Quoted 3 lines
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
Quoted 9 lines · Bartosz Golaszewski
Quoted 7 lines · Andy Shevchenko
Quoted 5 lines · Bartosz Golaszewski
From: Bartosz Golaszewski <[email protected]>
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.
...
Quoted 14 lines
Quoted 10 lines
drivers/spi/spi-bcm2835.c static int bcm2835_spi_setup(struct spi_device *spi) · line 1296
Quoted 9 lines
/*drivers/spi/spi-bcm2835.c +6
-0+* 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? */ Quoted 3 lines
I'm not sure why this quirk is here. AFAIR the SPI CS quirks are located in
gpiolib-of.c.
Quoted 7 lines
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(), notdevm_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?
Signature · 2 lines
With Best Regards, Andy Shevchenko
Quoted 9 lines · Andy Shevchenko
Quoted 7 lines
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(), notdevm_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
Quoted 14 lines · Linus Walleij
Quoted 9 lines · Andy Shevchenko
Quoted 7 lines
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(), notdevm_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?
Quoted 7 lines
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
Quoted 2 lines · Bartosz Golaszewski
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.
Signature · 2 lines
With Best Regards, Andy Shevchenko