lkml.org 
[lkml]   [2023]   [Aug]   [31]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    Subject[RFT PATCH] spi: bcm2835: reduce the abuse of the GPIO API
    Date
    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(-)

    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));
    --
    2.39.2
    \
     
     \ /
      Last update: 2023-08-31 21:50    [W:8.099 / U:0.360 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site