| Message ID | 20240131-descriptors-wireless-v1-1-e1c7c5d68746@linaro.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | Convert some wireless drivers to use GPIO descriptors | expand |
Linus Walleij <linus.walleij@linaro.org> writes: > The ath9k has an odd use of system-wide GPIOs: if the chip > does not have internal GPIO capability, it will try to obtain a > GPIO line from the system GPIO controller: > > if (BIT(gpio) & ah->caps.gpio_mask) > ath9k_hw_gpio_cfg_wmac(...); > else if (AR_SREV_SOC(ah)) > ath9k_hw_gpio_cfg_soc(ah, gpio, out, label); > > Where ath9k_hw_gpio_cfg_soc() will attempt to issue > gpio_request_one() passing the local GPIO number of the controller > (0..31) to gpio_request_one(). > > This is somewhat peculiar and possibly even dangerous: there is > nowadays no guarantee of the numbering of these system-wide > GPIOs, and assuming that GPIO 0..31 as used by ath9k would > correspond to GPIOs 0..31 on the system as a whole seems a bit > wild. > > My best guess is that everyone actually using this driver has > support for the local (custom) GPIO API and the bit in > h->caps.gpio_mask is always set for any GPIO the driver may > try to obtain, so this facility to use system-wide GPIOs is > actually unused and could be deleted. > > Anyway: I cannot know if this is really the case, so implement > a fallback handling using GPIO descriptors obtained from the > ah->dev device indexed 0..31. These can for example be passed > in the device tree, ACPI or through board files. I doubt that > anyone will use them, but this makes it possible to obtain a > system-wide GPIO for any of the 0..31 GPIOs potentially > requested by the driver. > > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> This seems reasonable, thanks! Acked-by: Toke Høiland-Jørgensen <toke@toke.dk>
On Wed, Jan 31, 2024 at 11:37:20PM +0100, Linus Walleij wrote: > The ath9k has an odd use of system-wide GPIOs: if the chip > does not have internal GPIO capability, it will try to obtain a > GPIO line from the system GPIO controller: > > if (BIT(gpio) & ah->caps.gpio_mask) > ath9k_hw_gpio_cfg_wmac(...); > else if (AR_SREV_SOC(ah)) > ath9k_hw_gpio_cfg_soc(ah, gpio, out, label); > > Where ath9k_hw_gpio_cfg_soc() will attempt to issue > gpio_request_one() passing the local GPIO number of the controller > (0..31) to gpio_request_one(). > > This is somewhat peculiar and possibly even dangerous: there is > nowadays no guarantee of the numbering of these system-wide > GPIOs, and assuming that GPIO 0..31 as used by ath9k would > correspond to GPIOs 0..31 on the system as a whole seems a bit > wild. > > My best guess is that everyone actually using this driver has > support for the local (custom) GPIO API and the bit in > h->caps.gpio_mask is always set for any GPIO the driver may > try to obtain, so this facility to use system-wide GPIOs is > actually unused and could be deleted. > > Anyway: I cannot know if this is really the case, so implement > a fallback handling using GPIO descriptors obtained from the > ah->dev device indexed 0..31. These can for example be passed > in the device tree, ACPI or through board files. I doubt that > anyone will use them, but this makes it possible to obtain a > system-wide GPIO for any of the 0..31 GPIOs potentially > requested by the driver. ... > + /* Obtains a system specific GPIO descriptor from another GPIO controller */ > + gpiod = devm_gpiod_get_index(ah->dev, NULL, gpio, flags); > + Unnecessary blank line, please don't add it. > + if (IS_ERR(gpiod)) { > + err = PTR_ERR(gpiod); > ath_err(ath9k_hw_common(ah), "request GPIO%d failed:%d\n", > gpio, err); > return; > }
On Wed, Jan 31, 2024, at 23:37, Linus Walleij wrote: > The ath9k has an odd use of system-wide GPIOs: if the chip > does not have internal GPIO capability, it will try to obtain a > GPIO line from the system GPIO controller: > > if (BIT(gpio) & ah->caps.gpio_mask) > ath9k_hw_gpio_cfg_wmac(...); > else if (AR_SREV_SOC(ah)) > ath9k_hw_gpio_cfg_soc(ah, gpio, out, label); > > Where ath9k_hw_gpio_cfg_soc() will attempt to issue > gpio_request_one() passing the local GPIO number of the controller > (0..31) to gpio_request_one(). > > This is somewhat peculiar and possibly even dangerous: there is > nowadays no guarantee of the numbering of these system-wide > GPIOs, and assuming that GPIO 0..31 as used by ath9k would > correspond to GPIOs 0..31 on the system as a whole seems a bit > wild. I would expect that it still works, as the SoCs that integrate ath9k hardware tend to be quite simple and only have a single gpio controller (drivers/gpio/gpio-ath79.c) with no more than 32 lines. Even on machines with an i2c gpio expander they likely come first. ath9k_gpio_cap_init() is how the gpio_mask gets initialized based on the chip model, and none of them have a mask with anything higher than the low 16 bits set. However, this is a mix of PCI devices with on-chip GPIOs that are handled through gpiolib. > My best guess is that everyone actually using this driver has > support for the local (custom) GPIO API and the bit in > h->caps.gpio_mask is always set for any GPIO the driver may > try to obtain, so this facility to use system-wide GPIOs is > actually unused and could be deleted. > > Anyway: I cannot know if this is really the case, so implement > a fallback handling using GPIO descriptors obtained from the > ah->dev device indexed 0..31. These can for example be passed > in the device tree, ACPI or through board files. I doubt that > anyone will use them, but this makes it possible to obtain a > system-wide GPIO for any of the 0..31 GPIOs potentially > requested by the driver. The platform data was dropped in favor of DT in commit 85b9686dae30 ("MIPS: ath79: drop platform device registration code"), but neither version actually initializes the btcoex gpio lines, and as far as I can tell, btcoex only really happens in the PCI version with internal gpio, so there is no need to actually read it from pdata in static void ath9k_hw_btcoex_pin_init(struct ath_hw *ah, u8 wlanactive_gpio, u8 btactive_gpio, u8 btpriority_gpio) { struct ath_btcoex_hw *btcoex_hw = &ah->btcoex_hw; struct ath9k_platform_data *pdata = ah->dev->platform_data; if (btcoex_hw->scheme != ATH_BTCOEX_CFG_2WIRE && btcoex_hw->scheme != ATH_BTCOEX_CFG_3WIRE) return; /* bt priority GPIO will be ignored by 2 wire scheme */ if (pdata && (pdata->bt_active_pin || pdata->bt_priority_pin || pdata->wlan_active_pin)) { btcoex_hw->btactive_gpio = pdata->bt_active_pin; btcoex_hw->wlanactive_gpio = pdata->wlan_active_pin; btcoex_hw->btpriority_gpio = pdata->bt_priority_pin; } else { btcoex_hw->btactive_gpio = btactive_gpio; btcoex_hw->wlanactive_gpio = wlanactive_gpio; btcoex_hw->btpriority_gpio = btpriority_gpio; } } After the board file removal, the LED gpio line seems to have exactly one remaining user in openwrt using a board file for a PCI connected (non-soc) ath9k device on a powerpc platform: https://git.openwrt.org/?p=openwrt/openwrt.git;a=blob;f=target/linux/mpc85xx/files/arch/powerpc/platforms/85xx/tl_wdr4900_v1.c;hb=refs/heads/main#l50 > @@ -2832,8 +2833,8 @@ u32 ath9k_hw_gpio_get(struct ath_hw *ah, u32 gpio) > val = REG_READ(ah, AR_GPIO_IN(ah)) & BIT(gpio); > else > val = MS_REG_READ(AR, gpio); > - } else if (BIT(gpio) & ah->caps.gpio_requested) { > - val = gpio_get_value(gpio) & BIT(gpio); > + } else if (ah->gpiods[gpio]) { > + val = gpiod_get_value(ah->gpiods[gpio]); > } else { > WARN_ON(1); > } > @@ -2856,8 +2857,8 @@ void ath9k_hw_set_gpio(struct ath_hw *ah, u32 > gpio, u32 val) > AR7010_GPIO_OUT : AR_GPIO_IN_OUT(ah); > > REG_RMW(ah, out_addr, val << gpio, BIT(gpio)); > - } else if (BIT(gpio) & ah->caps.gpio_requested) { > - gpio_set_value(gpio, val); > + } else if (ah->gpiods[gpio]) { > + gpiod_set_value(ah->gpiods[gpio], val); > } else { > WARN_ON(1); > } I don't think this is a good way to handle the gpiolib variants. What I'd try instead is to move the abstraction so on-chip gpio numbers don't get confused with gpiolib numbers, essentially getting rid of the latter entirely. I think something like the experiment below would work: Arnd diff --git a/drivers/net/wireless/ath/ath9k/btcoex.c b/drivers/net/wireless/ath/ath9k/btcoex.c index 9b393a8f7c3a..32f2113c13cb 100644 --- a/drivers/net/wireless/ath/ath9k/btcoex.c +++ b/drivers/net/wireless/ath/ath9k/btcoex.c @@ -115,23 +115,15 @@ static void ath9k_hw_btcoex_pin_init(struct ath_hw *ah, u8 wlanactive_gpio, u8 btactive_gpio, u8 btpriority_gpio) { struct ath_btcoex_hw *btcoex_hw = &ah->btcoex_hw; - struct ath9k_platform_data *pdata = ah->dev->platform_data; if (btcoex_hw->scheme != ATH_BTCOEX_CFG_2WIRE && btcoex_hw->scheme != ATH_BTCOEX_CFG_3WIRE) return; /* bt priority GPIO will be ignored by 2 wire scheme */ - if (pdata && (pdata->bt_active_pin || pdata->bt_priority_pin || - pdata->wlan_active_pin)) { - btcoex_hw->btactive_gpio = pdata->bt_active_pin; - btcoex_hw->wlanactive_gpio = pdata->wlan_active_pin; - btcoex_hw->btpriority_gpio = pdata->bt_priority_pin; - } else { - btcoex_hw->btactive_gpio = btactive_gpio; - btcoex_hw->wlanactive_gpio = wlanactive_gpio; - btcoex_hw->btpriority_gpio = btpriority_gpio; - } + btcoex_hw->btactive_gpio = btactive_gpio; + btcoex_hw->wlanactive_gpio = wlanactive_gpio; + btcoex_hw->btpriority_gpio = btpriority_gpio; } void ath9k_hw_btcoex_init_scheme(struct ath_hw *ah) diff --git a/drivers/net/wireless/ath/ath9k/gpio.c b/drivers/net/wireless/ath/ath9k/gpio.c index b457e52dd365..82b19c6a11fc 100644 --- a/drivers/net/wireless/ath/ath9k/gpio.c +++ b/drivers/net/wireless/ath/ath9k/gpio.c @@ -15,6 +15,7 @@ */ #include "ath9k.h" +#include <linux/gpio/consumer.h> /********************************/ /* LED functions */ @@ -27,7 +28,11 @@ static void ath_fill_led_pin(struct ath_softc *sc) struct ath_hw *ah = sc->sc_ah; /* Set default led pin if invalid */ - if (ah->led_pin < 0) { + if (AR_SREV_SOC(ah)) { + ah->led_gpio = gpiod_get(ah->dev, "led", 0); + if (IS_ERR(ah->led_gpio)) + ah->led_gpio = NULL; + } else if (ah->led_pin < 0) { if (AR_SREV_9287(ah)) ah->led_pin = ATH_LED_PIN_9287; else if (AR_SREV_9485(ah)) @@ -57,7 +62,10 @@ static void ath_led_brightness(struct led_classdev *led_cdev, if (sc->sc_ah->config.led_active_high) val = !val; - ath9k_hw_set_gpio(sc->sc_ah, sc->sc_ah->led_pin, val); + if (sc->sc_ah->led_gpio) + gpiod_set_value(sc->sc_ah->led_gpio, val); + else + ath9k_hw_set_gpio(sc->sc_ah, sc->sc_ah->led_pin, val); } void ath_deinit_leds(struct ath_softc *sc) @@ -68,7 +76,8 @@ void ath_deinit_leds(struct ath_softc *sc) ath_led_brightness(&sc->led_cdev, LED_OFF); led_classdev_unregister(&sc->led_cdev); - ath9k_hw_gpio_free(sc->sc_ah, sc->sc_ah->led_pin); + if (sc->sc_ah->led_gpio) + gpiod_put(sc->sc_ah->led_gpio); } void ath_init_leds(struct ath_softc *sc) diff --git a/drivers/net/wireless/ath/ath9k/htc_drv_gpio.c b/drivers/net/wireless/ath/ath9k/htc_drv_gpio.c index ecb848b60725..07720101e9cb 100644 --- a/drivers/net/wireless/ath/ath9k/htc_drv_gpio.c +++ b/drivers/net/wireless/ath/ath9k/htc_drv_gpio.c @@ -15,6 +15,7 @@ */ #include "htc.h" +#include <linux/gpio/consumer.h> /******************/ /* BTCOEX */ @@ -229,8 +230,11 @@ void ath9k_led_work(struct work_struct *work) struct ath9k_htc_priv, led_work); - ath9k_hw_set_gpio(priv->ah, priv->ah->led_pin, - (priv->brightness == LED_OFF)); + if (priv->ah->led_gpio) + gpiod_set_value(priv->ah->led_gpio, priv->brightness != LED_OFF); + else + ath9k_hw_set_gpio(priv->ah, priv->ah->led_pin, + (priv->brightness == LED_OFF)); } static void ath9k_led_brightness(struct led_classdev *led_cdev, @@ -254,12 +258,20 @@ void ath9k_deinit_leds(struct ath9k_htc_priv *priv) led_classdev_unregister(&priv->led_cdev); cancel_work_sync(&priv->led_work); - ath9k_hw_gpio_free(priv->ah, priv->ah->led_pin); + if (priv->ah->led_gpio) + gpiod_put(priv->ah->led_gpio); + else + ath9k_hw_gpio_free(priv->ah, priv->ah->led_pin); } void ath9k_configure_leds(struct ath9k_htc_priv *priv) { + if (priv->ah->led_gpio) { + gpiod_set_value(priv->ah->led_gpio, 1); + return; + } + /* Configure gpio 1 for output */ ath9k_hw_gpio_request_out(priv->ah, priv->ah->led_pin, "ath9k-led", @@ -272,7 +284,11 @@ void ath9k_init_leds(struct ath9k_htc_priv *priv) { int ret; - if (AR_SREV_9287(priv->ah)) + if (AR_SREV_SOC(priv->ah)) { + priv->ah->led_gpio = gpiod_get(priv->ah->dev, "led", 0); + if (IS_ERR(priv->ah->led_gpio)) + priv->ah->led_gpio = NULL; + } else if (AR_SREV_9287(priv->ah)) priv->ah->led_pin = ATH_LED_PIN_9287; else if (AR_SREV_9271(priv->ah)) priv->ah->led_pin = ATH_LED_PIN_9271; diff --git a/drivers/net/wireless/ath/ath9k/hw.c b/drivers/net/wireless/ath/ath9k/hw.c index 5982e0db45f9..d8663bd9df7c 100644 --- a/drivers/net/wireless/ath/ath9k/hw.c +++ b/drivers/net/wireless/ath/ath9k/hw.c @@ -2722,26 +2722,6 @@ static void ath9k_hw_gpio_cfg_output_mux(struct ath_hw *ah, u32 gpio, u32 type) } } -/* BSP should set the corresponding MUX register correctly. - */ -static void ath9k_hw_gpio_cfg_soc(struct ath_hw *ah, u32 gpio, bool out, - const char *label) -{ - int err; - - if (ah->caps.gpio_requested & BIT(gpio)) - return; - - err = gpio_request_one(gpio, out ? GPIOF_OUT_INIT_LOW : GPIOF_IN, label); - if (err) { - ath_err(ath9k_hw_common(ah), "request GPIO%d failed:%d\n", - gpio, err); - return; - } - - ah->caps.gpio_requested |= BIT(gpio); -} - static void ath9k_hw_gpio_cfg_wmac(struct ath_hw *ah, u32 gpio, bool out, u32 ah_signal_type) { @@ -2775,8 +2755,6 @@ static void ath9k_hw_gpio_request(struct ath_hw *ah, u32 gpio, bool out, if (BIT(gpio) & ah->caps.gpio_mask) ath9k_hw_gpio_cfg_wmac(ah, gpio, out, ah_signal_type); - else if (AR_SREV_SOC(ah)) - ath9k_hw_gpio_cfg_soc(ah, gpio, out, label); else WARN_ON(1); } @@ -2800,11 +2778,6 @@ void ath9k_hw_gpio_free(struct ath_hw *ah, u32 gpio) return; WARN_ON(gpio >= ah->caps.num_gpio_pins); - - if (ah->caps.gpio_requested & BIT(gpio)) { - gpio_free(gpio); - ah->caps.gpio_requested &= ~BIT(gpio); - } } EXPORT_SYMBOL(ath9k_hw_gpio_free); @@ -2832,8 +2805,6 @@ u32 ath9k_hw_gpio_get(struct ath_hw *ah, u32 gpio) val = REG_READ(ah, AR_GPIO_IN(ah)) & BIT(gpio); else val = MS_REG_READ(AR, gpio); - } else if (BIT(gpio) & ah->caps.gpio_requested) { - val = gpio_get_value(gpio) & BIT(gpio); } else { WARN_ON(1); } @@ -2856,8 +2827,6 @@ void ath9k_hw_set_gpio(struct ath_hw *ah, u32 gpio, u32 val) AR7010_GPIO_OUT : AR_GPIO_IN_OUT(ah); REG_RMW(ah, out_addr, val << gpio, BIT(gpio)); - } else if (BIT(gpio) & ah->caps.gpio_requested) { - gpio_set_value(gpio, val); } else { WARN_ON(1); } diff --git a/drivers/net/wireless/ath/ath9k/hw.h b/drivers/net/wireless/ath/ath9k/hw.h index 450ab19b1d4e..eff27c901a81 100644 --- a/drivers/net/wireless/ath/ath9k/hw.h +++ b/drivers/net/wireless/ath/ath9k/hw.h @@ -910,6 +910,8 @@ struct ath_hw { u32 gpio_mask; u32 gpio_val; + struct gpio_desc *led_gpio; + struct ar5416IniArray ini_dfs; struct ar5416IniArray iniModes; struct ar5416IniArray iniCommon; diff --git a/drivers/net/wireless/ath/ath9k/init.c b/drivers/net/wireless/ath/ath9k/init.c index 7fad7e75af6a..68562310bf18 100644 --- a/drivers/net/wireless/ath/ath9k/init.c +++ b/drivers/net/wireless/ath/ath9k/init.c @@ -632,9 +632,6 @@ static int ath9k_init_platform(struct ath_softc *sc) if (!pdata->use_eeprom) { ah->ah_flags &= ~AH_USE_EEPROM; - ah->gpio_mask = pdata->gpio_mask; - ah->gpio_val = pdata->gpio_val; - ah->led_pin = pdata->led_pin; ah->is_clk_25mhz = pdata->is_clk_25mhz; ah->get_mac_revision = pdata->get_mac_revision; ah->external_reset = pdata->external_reset; diff --git a/drivers/net/wireless/ath/ath9k/main.c b/drivers/net/wireless/ath/ath9k/main.c index c48ff0ffbfef..89bb773c5e15 100644 --- a/drivers/net/wireless/ath/ath9k/main.c +++ b/drivers/net/wireless/ath/ath9k/main.c @@ -16,6 +16,7 @@ #include <linux/nl80211.h> #include <linux/delay.h> +#include <linux/gpio/consumer.h> #include "ath9k.h" #include "btcoex.h" @@ -731,6 +732,8 @@ static int ath9k_start(struct ieee80211_hw *hw) ath9k_hw_gpio_request_out(ah, ah->led_pin, NULL, AR_GPIO_OUTPUT_MUX_AS_OUTPUT); } + if (ah->led_gpio) + gpiod_set_value(ah->led_gpio, 1); /* * Reset key cache to sane defaults (all entries cleared) instead of @@ -948,6 +951,8 @@ static void ath9k_stop(struct ieee80211_hw *hw) (ah->config.led_active_high) ? 0 : 1); ath9k_hw_gpio_request_in(ah, ah->led_pin, NULL); } + if (ah->led_gpio) + gpiod_set_value(ah->led_gpio, 0); ath_prepare_reset(sc); diff --git a/include/linux/ath9k_platform.h b/include/linux/ath9k_platform.h index 76860a461ed2..cffdb5de407e 100644 --- a/include/linux/ath9k_platform.h +++ b/include/linux/ath9k_platform.h @@ -27,14 +27,6 @@ struct ath9k_platform_data { u16 eeprom_data[ATH9K_PLAT_EEP_MAX_WORDS]; u8 *macaddr; - int led_pin; - u32 gpio_mask; - u32 gpio_val; - - u32 bt_active_pin; - u32 bt_priority_pin; - u32 wlan_active_pin; - bool endian_check; bool is_clk_25mhz; bool tx_gain_buffalo;
Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes: > On Wed, Jan 31, 2024 at 11:37:20PM +0100, Linus Walleij wrote: > >> The ath9k has an odd use of system-wide GPIOs: if the chip >> does not have internal GPIO capability, it will try to obtain a >> GPIO line from the system GPIO controller: >> >> if (BIT(gpio) & ah->caps.gpio_mask) >> ath9k_hw_gpio_cfg_wmac(...); >> else if (AR_SREV_SOC(ah)) >> ath9k_hw_gpio_cfg_soc(ah, gpio, out, label); >> >> Where ath9k_hw_gpio_cfg_soc() will attempt to issue >> gpio_request_one() passing the local GPIO number of the controller >> (0..31) to gpio_request_one(). >> >> This is somewhat peculiar and possibly even dangerous: there is >> nowadays no guarantee of the numbering of these system-wide >> GPIOs, and assuming that GPIO 0..31 as used by ath9k would >> correspond to GPIOs 0..31 on the system as a whole seems a bit >> wild. >> >> My best guess is that everyone actually using this driver has >> support for the local (custom) GPIO API and the bit in >> h->caps.gpio_mask is always set for any GPIO the driver may >> try to obtain, so this facility to use system-wide GPIOs is >> actually unused and could be deleted. >> >> Anyway: I cannot know if this is really the case, so implement >> a fallback handling using GPIO descriptors obtained from the >> ah->dev device indexed 0..31. These can for example be passed >> in the device tree, ACPI or through board files. I doubt that >> anyone will use them, but this makes it possible to obtain a >> system-wide GPIO for any of the 0..31 GPIOs potentially >> requested by the driver. > > ... > >> + /* Obtains a system specific GPIO descriptor from another GPIO controller */ >> + gpiod = devm_gpiod_get_index(ah->dev, NULL, gpio, flags); > >> + > > Unnecessary blank line, please don't add it. I can fix that in the pending branch, no need to resend because of this.
On Thu, Feb 01, 2024 at 01:20:16PM +0100, Arnd Bergmann wrote: > On Wed, Jan 31, 2024, at 23:37, Linus Walleij wrote: FWIW, some nit-picks below. ... > - if (ah->led_pin < 0) { > + if (AR_SREV_SOC(ah)) { > + ah->led_gpio = gpiod_get(ah->dev, "led", 0); > + if (IS_ERR(ah->led_gpio)) > + ah->led_gpio = NULL; Slightly better to have something like desc = gpiod_get_optional(); if (!IS_ERR(desc)) led_gpio = desc; > + } else if (ah->led_pin < 0) { ... > + if (sc->sc_ah->led_gpio) Dup check > + gpiod_put(sc->sc_ah->led_gpio); ... > #include "htc.h" > +#include <linux/gpio/consumer.h> First to include linux/* ones? ... > + ath9k_hw_set_gpio(priv->ah, priv->ah->led_pin, > + (priv->brightness == LED_OFF)); Unnecessary parentheses. > } ... > + if (AR_SREV_SOC(priv->ah)) { > + priv->ah->led_gpio = gpiod_get(priv->ah->dev, "led", 0); > + if (IS_ERR(priv->ah->led_gpio)) > + priv->ah->led_gpio = NULL; _optional() ? > + } else if (AR_SREV_9287(priv->ah)) ... > + if (ah->led_gpio) Dup check. > + gpiod_set_value(ah->led_gpio, 1); > ... > + if (ah->led_gpio) Ditto. > + gpiod_set_value(ah->led_gpio, 0);
On Thu, Feb 1, 2024, at 14:17, Andy Shevchenko wrote: > On Thu, Feb 01, 2024 at 01:20:16PM +0100, Arnd Bergmann wrote: >> On Wed, Jan 31, 2024, at 23:37, Linus Walleij wrote: > >> + } else if (ah->led_pin < 0) { > > ... > >> + if (sc->sc_ah->led_gpio) > > Dup check I don't know what you mean here. To explain what I'm trying to do: The idea is that the LED is always backed by either gpiolib or the internal gpio controller on the PCI device. This means every access to an LED must be guarded with if (gpiodesc) gpio_*(gpiodesc); else internal(ah); We could probably go a little further in the cleanup and throw out the gpiolib path entirely, instead relying on the existing leds-gpio driver. Since there are currently no upstream users of the gpiolib path, that would likely lead to cleaner code but require more changes to any out-of-tree users that rely on the platform_data to pass the GPIOs today. Arnd
"Arnd Bergmann" <arnd@arndb.de> writes: > On Thu, Feb 1, 2024, at 14:17, Andy Shevchenko wrote: >> On Thu, Feb 01, 2024 at 01:20:16PM +0100, Arnd Bergmann wrote: >>> On Wed, Jan 31, 2024, at 23:37, Linus Walleij wrote: >> >>> + } else if (ah->led_pin < 0) { >> >> ... >> >>> + if (sc->sc_ah->led_gpio) >> >> Dup check > > I don't know what you mean here. To explain what I'm > trying to do: The idea is that the LED is always backed > by either gpiolib or the internal gpio controller on > the PCI device. This means every access to an LED must > be guarded with > > if (gpiodesc) > gpio_*(gpiodesc); > else > internal(ah); > > We could probably go a little further in the cleanup and > throw out the gpiolib path entirely, instead relying > on the existing leds-gpio driver. Since there are currently > no upstream users of the gpiolib path, that would likely > lead to cleaner code but require more changes to any > out-of-tree users that rely on the platform_data to > pass the GPIOs today. There being exactly one such out of tree user (per your up-thread email) in OpenWrt? Or are you aware of others? -Toke
On Thu, Feb 1, 2024, at 15:18, Toke Høiland-Jørgensen wrote: > "Arnd Bergmann" <arnd@arndb.de> writes: > >> We could probably go a little further in the cleanup and >> throw out the gpiolib path entirely, instead relying >> on the existing leds-gpio driver. Since there are currently >> no upstream users of the gpiolib path, that would likely >> lead to cleaner code but require more changes to any >> out-of-tree users that rely on the platform_data to >> pass the GPIOs today. > > There being exactly one such out of tree user (per your up-thread > email) in OpenWrt? Or are you aware of others? Actually, on a closer look not even that: the ath9k LED support in openwrt is quite different from upstream, and it just uses gpio-led there, with a gpio provider in the driver for the internal gpios. We can probably just remove the gpiolib consumer side from ath9k entirely then: it's not needed for the PCI devices at all, and the SoC devices no longer use it upstream or in openwrt. Arnd
"Arnd Bergmann" <arnd@arndb.de> writes: > On Thu, Feb 1, 2024, at 15:18, Toke Høiland-Jørgensen wrote: >> "Arnd Bergmann" <arnd@arndb.de> writes: >> >>> We could probably go a little further in the cleanup and >>> throw out the gpiolib path entirely, instead relying >>> on the existing leds-gpio driver. Since there are currently >>> no upstream users of the gpiolib path, that would likely >>> lead to cleaner code but require more changes to any >>> out-of-tree users that rely on the platform_data to >>> pass the GPIOs today. >> >> There being exactly one such out of tree user (per your up-thread >> email) in OpenWrt? Or are you aware of others? > > Actually, on a closer look not even that: the ath9k LED support > in openwrt is quite different from upstream, and it just uses > gpio-led there, with a gpio provider in the driver for the > internal gpios. > > We can probably just remove the gpiolib consumer side from > ath9k entirely then: it's not needed for the PCI devices > at all, and the SoC devices no longer use it upstream or > in openwrt. Alright cool, in that case I am OK with just ripping it out entirely :) -Toke
Linus Walleij <linus.walleij@linaro.org> wrote: > The ath9k has an odd use of system-wide GPIOs: if the chip > does not have internal GPIO capability, it will try to obtain a > GPIO line from the system GPIO controller: > > if (BIT(gpio) & ah->caps.gpio_mask) > ath9k_hw_gpio_cfg_wmac(...); > else if (AR_SREV_SOC(ah)) > ath9k_hw_gpio_cfg_soc(ah, gpio, out, label); > > Where ath9k_hw_gpio_cfg_soc() will attempt to issue > gpio_request_one() passing the local GPIO number of the controller > (0..31) to gpio_request_one(). > > This is somewhat peculiar and possibly even dangerous: there is > nowadays no guarantee of the numbering of these system-wide > GPIOs, and assuming that GPIO 0..31 as used by ath9k would > correspond to GPIOs 0..31 on the system as a whole seems a bit > wild. > > My best guess is that everyone actually using this driver has > support for the local (custom) GPIO API and the bit in > h->caps.gpio_mask is always set for any GPIO the driver may > try to obtain, so this facility to use system-wide GPIOs is > actually unused and could be deleted. > > Anyway: I cannot know if this is really the case, so implement > a fallback handling using GPIO descriptors obtained from the > ah->dev device indexed 0..31. These can for example be passed > in the device tree, ACPI or through board files. I doubt that > anyone will use them, but this makes it possible to obtain a > system-wide GPIO for any of the 0..31 GPIOs potentially > requested by the driver. > > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > Acked-by: Toke Høiland-Jørgensen <toke@toke.dk> I understood that there will be a new version for ath9k so dropping patch 1. Patch set to Changes Requested.
diff --git a/drivers/net/wireless/ath/ath9k/hw.c b/drivers/net/wireless/ath/ath9k/hw.c index 5982e0db45f9..ee6705836746 100644 --- a/drivers/net/wireless/ath/ath9k/hw.c +++ b/drivers/net/wireless/ath/ath9k/hw.c @@ -20,7 +20,7 @@ #include <linux/time.h> #include <linux/bitops.h> #include <linux/etherdevice.h> -#include <linux/gpio.h> +#include <linux/gpio/consumer.h> #include <asm/unaligned.h> #include "hw.h" @@ -2727,19 +2727,25 @@ static void ath9k_hw_gpio_cfg_output_mux(struct ath_hw *ah, u32 gpio, u32 type) static void ath9k_hw_gpio_cfg_soc(struct ath_hw *ah, u32 gpio, bool out, const char *label) { + enum gpiod_flags flags = out ? GPIOD_OUT_LOW : GPIOD_IN; + struct gpio_desc *gpiod; int err; - if (ah->caps.gpio_requested & BIT(gpio)) + if (ah->gpiods[gpio]) return; - err = gpio_request_one(gpio, out ? GPIOF_OUT_INIT_LOW : GPIOF_IN, label); - if (err) { + /* Obtains a system specific GPIO descriptor from another GPIO controller */ + gpiod = devm_gpiod_get_index(ah->dev, NULL, gpio, flags); + + if (IS_ERR(gpiod)) { + err = PTR_ERR(gpiod); ath_err(ath9k_hw_common(ah), "request GPIO%d failed:%d\n", gpio, err); return; } - ah->caps.gpio_requested |= BIT(gpio); + gpiod_set_consumer_name(gpiod, label); + ah->gpiods[gpio] = gpiod; } static void ath9k_hw_gpio_cfg_wmac(struct ath_hw *ah, u32 gpio, bool out, @@ -2800,11 +2806,6 @@ void ath9k_hw_gpio_free(struct ath_hw *ah, u32 gpio) return; WARN_ON(gpio >= ah->caps.num_gpio_pins); - - if (ah->caps.gpio_requested & BIT(gpio)) { - gpio_free(gpio); - ah->caps.gpio_requested &= ~BIT(gpio); - } } EXPORT_SYMBOL(ath9k_hw_gpio_free); @@ -2832,8 +2833,8 @@ u32 ath9k_hw_gpio_get(struct ath_hw *ah, u32 gpio) val = REG_READ(ah, AR_GPIO_IN(ah)) & BIT(gpio); else val = MS_REG_READ(AR, gpio); - } else if (BIT(gpio) & ah->caps.gpio_requested) { - val = gpio_get_value(gpio) & BIT(gpio); + } else if (ah->gpiods[gpio]) { + val = gpiod_get_value(ah->gpiods[gpio]); } else { WARN_ON(1); } @@ -2856,8 +2857,8 @@ void ath9k_hw_set_gpio(struct ath_hw *ah, u32 gpio, u32 val) AR7010_GPIO_OUT : AR_GPIO_IN_OUT(ah); REG_RMW(ah, out_addr, val << gpio, BIT(gpio)); - } else if (BIT(gpio) & ah->caps.gpio_requested) { - gpio_set_value(gpio, val); + } else if (ah->gpiods[gpio]) { + gpiod_set_value(ah->gpiods[gpio], val); } else { WARN_ON(1); } diff --git a/drivers/net/wireless/ath/ath9k/hw.h b/drivers/net/wireless/ath/ath9k/hw.h index 450ab19b1d4e..1eb4ff8955ae 100644 --- a/drivers/net/wireless/ath/ath9k/hw.h +++ b/drivers/net/wireless/ath/ath9k/hw.h @@ -19,6 +19,7 @@ #include <linux/if_ether.h> #include <linux/delay.h> +#include <linux/gpio/consumer.h> #include <linux/io.h> #include <linux/firmware.h> @@ -302,7 +303,6 @@ struct ath9k_hw_capabilities { u8 max_rxchains; u8 num_gpio_pins; u32 gpio_mask; - u32 gpio_requested; u8 rx_hp_qdepth; u8 rx_lp_qdepth; u8 rx_status_len; @@ -783,6 +783,7 @@ struct ath_hw { struct ath9k_hw_capabilities caps; struct ath9k_channel channels[ATH9K_NUM_CHANNELS]; struct ath9k_channel *curchan; + struct gpio_desc *gpiods[32]; union { struct ar5416_eeprom_def def;
The ath9k has an odd use of system-wide GPIOs: if the chip does not have internal GPIO capability, it will try to obtain a GPIO line from the system GPIO controller: if (BIT(gpio) & ah->caps.gpio_mask) ath9k_hw_gpio_cfg_wmac(...); else if (AR_SREV_SOC(ah)) ath9k_hw_gpio_cfg_soc(ah, gpio, out, label); Where ath9k_hw_gpio_cfg_soc() will attempt to issue gpio_request_one() passing the local GPIO number of the controller (0..31) to gpio_request_one(). This is somewhat peculiar and possibly even dangerous: there is nowadays no guarantee of the numbering of these system-wide GPIOs, and assuming that GPIO 0..31 as used by ath9k would correspond to GPIOs 0..31 on the system as a whole seems a bit wild. My best guess is that everyone actually using this driver has support for the local (custom) GPIO API and the bit in h->caps.gpio_mask is always set for any GPIO the driver may try to obtain, so this facility to use system-wide GPIOs is actually unused and could be deleted. Anyway: I cannot know if this is really the case, so implement a fallback handling using GPIO descriptors obtained from the ah->dev device indexed 0..31. These can for example be passed in the device tree, ACPI or through board files. I doubt that anyone will use them, but this makes it possible to obtain a system-wide GPIO for any of the 0..31 GPIOs potentially requested by the driver. Signed-off-by: Linus Walleij <linus.walleij@linaro.org> --- drivers/net/wireless/ath/ath9k/hw.c | 29 +++++++++++++++-------------- drivers/net/wireless/ath/ath9k/hw.h | 3 ++- 2 files changed, 17 insertions(+), 15 deletions(-)