[1/6] wifi: ath9k: Obtain system GPIOS from descriptors

  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(-)
  

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.