Re: [PATCH v3] staging: wfx: Get descriptors for GPIOs

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Sunday 28 June 2020 10:52:36 CEST Linus Walleij wrote:
> 
> The code has the functionality to insert the GPIO lines using
> the global GPIO numbers through module parameters.
> 
> As we are clearly deprecating the use of global GPIO numbers
> look up the GPIO descriptors from the device instead. This
> usually falls back to device hardware descriptions using e.g.
> device tree or ACPI. This device clearly supports device
> tree when used over SPI for example.
> 
> For example, this can be supplied in the device tree like so:
> 
>   wfx@0x01 {
>       compatible = "silabs,wf200";
>       reset-gpios = <&gpio0 1>;
>       wakeup-gpios = <&gpio0 2>;
>   };
>
> Cc: Jérôme Pouiller <jerome.pouiller@xxxxxxxxxx>
> Signed-off-by: Linus Walleij <linus.walleij@xxxxxxxxxx>
> ---
> ChangeLog v2->v3:
> - ERR_CAST not PTR_CAST
> ChangeLog v1->v2:
> - Fixed a cast and a variable name.
> - I still don't know how to compile this but hey the zeroday
>   robot does.
> ---
>  drivers/staging/wfx/bus_spi.c | 11 +++++----
>  drivers/staging/wfx/main.c    | 42 ++++-------------------------------
>  drivers/staging/wfx/main.h    |  2 --
>  3 files changed, 9 insertions(+), 46 deletions(-)
> 
> diff --git a/drivers/staging/wfx/bus_spi.c b/drivers/staging/wfx/bus_spi.c
> index e8da61fb096b..88ca5d453e83 100644
> --- a/drivers/staging/wfx/bus_spi.c
> +++ b/drivers/staging/wfx/bus_spi.c
> @@ -8,7 +8,6 @@
>   */
>  #include <linux/module.h>
>  #include <linux/delay.h>
> -#include <linux/gpio.h>
>  #include <linux/gpio/consumer.h>
>  #include <linux/spi/spi.h>
>  #include <linux/interrupt.h>
> @@ -21,10 +20,6 @@
>  #include "main.h"
>  #include "bh.h"
> 
> -static int gpio_reset = -2;
> -module_param(gpio_reset, int, 0644);
> -MODULE_PARM_DESC(gpio_reset, "gpio number for reset. -1 for none.");
> -
>  #define SET_WRITE 0x7FFF        /* usage: and operation */
>  #define SET_READ 0x8000         /* usage: or operation */
> 
> @@ -211,10 +206,14 @@ static int wfx_spi_probe(struct spi_device *func)
>                 bus->need_swab = true;
>         spi_set_drvdata(func, bus);
> 
> -       bus->gpio_reset = wfx_get_gpio(&func->dev, gpio_reset, "reset");
> +       bus->gpio_reset = devm_gpiod_get_optional(&func->dev, "reset"
> +                                                 GPIOD_OUT_HIGH);

The original code asks for GPIOD_OUT_LOW.

> +       if (IS_ERR(bus->gpio_reset))
> +               return PTR_ERR(bus->gpio_reset);
>         if (!bus->gpio_reset) {
>                 dev_warn(&func->dev, "try to load firmware anyway\n");

In the original code, this case also generated the warning "gpio reset is
not defined" (raised from wfx_get_gpio()). I suggest to change the warning
below into "gpio reset is not defined, try to load firmware anyway\n".

>         } else {
> +               gpiod_set_consumer_name(bus->gpio_reset, "wfx reset");
>                 if (spi_get_device_id(func)->driver_data & WFX_RESET_INVERTED)
>                         gpiod_toggle_active_low(bus->gpio_reset);
>                 gpiod_set_value_cansleep(bus->gpio_reset, 1);
> diff --git a/drivers/staging/wfx/main.c b/drivers/staging/wfx/main.c
> index 6bd96f476388..d90169fe1851 100644
> --- a/drivers/staging/wfx/main.c
> +++ b/drivers/staging/wfx/main.c
> @@ -13,7 +13,6 @@
>  #include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/of_net.h>
> -#include <linux/gpio.h>
>  #include <linux/gpio/consumer.h>
>  #include <linux/mmc/sdio_func.h>
>  #include <linux/spi/spi.h>
> @@ -41,10 +40,6 @@ MODULE_DESCRIPTION("Silicon Labs 802.11 Wireless LAN driver for WFx");
>  MODULE_AUTHOR("Jérôme Pouiller <jerome.pouiller@xxxxxxxxxx>");
>  MODULE_LICENSE("GPL");
> 
> -static int gpio_wakeup = -2;
> -module_param(gpio_wakeup, int, 0644);
> -MODULE_PARM_DESC(gpio_wakeup, "gpio number for wakeup. -1 for none.");
> -
>  #define RATETAB_ENT(_rate, _rateid, _flags) { \
>         .bitrate  = (_rate),   \
>         .hw_value = (_rateid), \
> @@ -170,38 +165,6 @@ bool wfx_api_older_than(struct wfx_dev *wdev, int major, int minor)
>         return false;
>  }
> 
> -struct gpio_desc *wfx_get_gpio(struct device *dev,
> -                              int override, const char *label)
> -{
> -       struct gpio_desc *ret;
> -       char label_buf[256];
> -
> -       if (override >= 0) {
> -               snprintf(label_buf, sizeof(label_buf), "wfx_%s", label);
> -               ret = ERR_PTR(devm_gpio_request_one(dev, override,
> -                                                   GPIOF_OUT_INIT_LOW,
> -                                                   label_buf));
> -               if (!ret)
> -                       ret = gpio_to_desc(override);
> -       } else if (override == -1) {
> -               ret = NULL;
> -       } else {
> -               ret = devm_gpiod_get(dev, label, GPIOD_OUT_LOW);
> -       }
> -       if (IS_ERR_OR_NULL(ret)) {
> -               if (!ret || PTR_ERR(ret) == -ENOENT)
> -                       dev_warn(dev, "gpio %s is not defined\n", label);
> -               else
> -                       dev_warn(dev, "error while requesting gpio %s\n",
> -                                label);
> -               ret = NULL;
> -       } else {
> -               dev_dbg(dev, "using gpio %d for %s\n",
> -                       desc_to_gpio(ret), label);
> -       }
> -       return ret;
> -}
> -
>  /* NOTE: wfx_send_pds() destroy buf */
>  int wfx_send_pds(struct wfx_dev *wdev, u8 *buf, size_t len)
>  {
> @@ -340,7 +303,10 @@ struct wfx_dev *wfx_init_common(struct device *dev,
>         memcpy(&wdev->pdata, pdata, sizeof(*pdata));
>         of_property_read_string(dev->of_node, "config-file",
>                                 &wdev->pdata.file_pds);
> -       wdev->pdata.gpio_wakeup = wfx_get_gpio(dev, gpio_wakeup, "wakeup");
> +       wdev->pdata.gpio_wakeup = devm_gpiod_get(dev, "wakeup", GPIOD_IN);

GPIOD_OUT_LOW?

> +       if (IS_ERR(wdev->pdata.gpio_wakeup))
> +               return ERR_CAST(wdev->pdata.gpio_wakeup);

This code will fail if the "wakeup" attribute is not defined. I suggest to
just print a notification and continue (the previous code printed a warning,
but the level INFO is sufficient). 


> +       gpiod_set_consumer_name(wdev->pdata.gpio_wakeup, "wfx wakeup");
>         wfx_sl_fill_pdata(dev, &wdev->pdata);
> 
>         mutex_init(&wdev->conf_mutex);
> diff --git a/drivers/staging/wfx/main.h b/drivers/staging/wfx/main.h
> index f832ce409fda..c59d375dd3ad 100644
> --- a/drivers/staging/wfx/main.h
> +++ b/drivers/staging/wfx/main.h
> @@ -38,8 +38,6 @@ struct wfx_dev *wfx_init_common(struct device *dev,
>  int wfx_probe(struct wfx_dev *wdev);
>  void wfx_release(struct wfx_dev *wdev);
> 
> -struct gpio_desc *wfx_get_gpio(struct device *dev, int override,
> -                              const char *label);
>  bool wfx_api_older_than(struct wfx_dev *wdev, int major, int minor);
>  int wfx_send_pds(struct wfx_dev *wdev, u8 *buf, size_t len);
> 
> --
> 2.25.4
> 
> 


-- 
Jérôme Pouiller


_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel




[Index of Archives]     [Linux Driver Backports]     [DMA Engine]     [Linux GPIO]     [Linux SPI]     [Video for Linux]     [Linux USB Devel]     [Linux Coverity]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux