Message-Id: <613ae419-9a2c-477e-8b19-8a29d42a3164@app.fastmail.com>
Date: Thu, 01 Feb 2024 13:20:16 +0100
From: "Arnd Bergmann" <arnd@...db.de>
To: "Linus Walleij" <linus.walleij@...aro.org>,
 Toke Høiland-Jørgensen <toke@...e.dk>,
 "Kalle Valo" <kvalo@...nel.org>, "Arend van Spriel" <aspriel@...il.com>,
 "Franky Lin" <franky.lin@...adcom.com>,
 "Hante Meuleman" <hante.meuleman@...adcom.com>,
 "Andy Shevchenko" <andriy.shevchenko@...ux.intel.com>,
 "Lee Jones" <lee@...nel.org>, "Brian Norris" <briannorris@...omium.org>,
 "Srinivasan Raju" <srini.raju@...elifi.com>
Cc: linux-wireless@...r.kernel.org, linux-kernel@...r.kernel.org,
 brcm80211-dev-list.pdl@...adcom.com
Subject: Re: [PATCH 1/6] wifi: ath9k: Obtain system GPIOS from descriptors

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;

Powered by blists - more mailing lists