This converts some Wireless network drivers to use GPIO descriptors,
and some just have unused header inclusions.
The Intersil PL54 driver is intentionally untouched because Arnd
is cleaning it up fully.
Signed-off-by: Linus Walleij <[email protected]>
---
Linus Walleij (6):
wifi: ath9k: Obtain system GPIOS from descriptors
wifi: ti: wlcore: sdio: Drop unused include
brcm80211: brcmsmac: Drop legacy header
wifi: mwifiex: Drop unused headers
wifi: plfxlc: Drop unused include
wifi: cw1200: Convert to GPIO descriptors
drivers/net/wireless/ath/ath9k/hw.c | 29 ++++-----
drivers/net/wireless/ath/ath9k/hw.h | 3 +-
.../net/wireless/broadcom/brcm80211/brcmsmac/led.c | 1 -
drivers/net/wireless/marvell/mwifiex/main.h | 2 -
drivers/net/wireless/purelifi/plfxlc/mac.c | 1 -
drivers/net/wireless/st/cw1200/cw1200_sdio.c | 42 +++++++------
drivers/net/wireless/st/cw1200/cw1200_spi.c | 71 ++++++++++++----------
drivers/net/wireless/ti/wlcore/sdio.c | 1 -
include/linux/platform_data/net-cw1200.h | 4 --
9 files changed, 82 insertions(+), 72 deletions(-)
---
base-commit: 6613476e225e090cc9aad49be7fa504e290dd33d
change-id: 20240122-descriptors-wireless-b8da95dcab35
Best regards,
--
Linus Walleij <[email protected]>
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 <[email protected]>
---
drivers/net/wireless/ath/ath9k/hw.c | 29 +++++++++++++++--------------
drivers/net/wireless/ath/ath9k/hw.h | 3 ++-
2 files changed, 17 insertions(+), 15 deletions(-)
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;
--
2.34.1
The CW1200 uses two GPIOs to control the powerup and reset
pins, get these from GPIO descriptors instead of being passed
as platform data from boardfiles.
The RESET line will need to be marked as active low as we will
let gpiolib handle the polarity inversion.
The SDIO case is a bit special since the "card" need to be
powered up before it gets detected on the SDIO bus and
properly probed. Fix this by using board-specific GPIOs
assigned to device "NULL".
There are currently no in-tree users.
Signed-off-by: Linus Walleij <[email protected]>
---
drivers/net/wireless/st/cw1200/cw1200_sdio.c | 42 +++++++++-------
drivers/net/wireless/st/cw1200/cw1200_spi.c | 71 ++++++++++++++++------------
include/linux/platform_data/net-cw1200.h | 4 --
3 files changed, 65 insertions(+), 52 deletions(-)
diff --git a/drivers/net/wireless/st/cw1200/cw1200_sdio.c b/drivers/net/wireless/st/cw1200/cw1200_sdio.c
index 4c30b5772ce0..00c4731d8f8e 100644
--- a/drivers/net/wireless/st/cw1200/cw1200_sdio.c
+++ b/drivers/net/wireless/st/cw1200/cw1200_sdio.c
@@ -8,7 +8,7 @@
#include <linux/module.h>
#include <linux/interrupt.h>
-#include <linux/gpio.h>
+#include <linux/gpio/consumer.h>
#include <linux/delay.h>
#include <linux/mmc/host.h>
#include <linux/mmc/sdio_func.h>
@@ -178,12 +178,15 @@ static int cw1200_sdio_irq_unsubscribe(struct hwbus_priv *self)
return ret;
}
+/* Like the rest of the driver, this only supports one device per system */
+static struct gpio_desc *cw1200_reset;
+static struct gpio_desc *cw1200_powerup;
+
static int cw1200_sdio_off(const struct cw1200_platform_data_sdio *pdata)
{
- if (pdata->reset) {
- gpio_set_value(pdata->reset, 0);
+ if (cw1200_reset) {
+ gpiod_set_value(cw1200_reset, 0);
msleep(30); /* Min is 2 * CLK32K cycles */
- gpio_free(pdata->reset);
}
if (pdata->power_ctrl)
@@ -196,16 +199,21 @@ static int cw1200_sdio_off(const struct cw1200_platform_data_sdio *pdata)
static int cw1200_sdio_on(const struct cw1200_platform_data_sdio *pdata)
{
- /* Ensure I/Os are pulled low */
- if (pdata->reset) {
- gpio_request(pdata->reset, "cw1200_wlan_reset");
- gpio_direction_output(pdata->reset, 0);
+ /* Ensure I/Os are pulled low (reset is active low) */
+ cw1200_reset = devm_gpiod_get_optional(NULL, "reset", GPIOD_OUT_HIGH);
+ if (IS_ERR(cw1200_reset)) {
+ pr_err("could not get CW1200 SDIO reset GPIO\n");
+ return PTR_ERR(cw1200_reset);
}
- if (pdata->powerup) {
- gpio_request(pdata->powerup, "cw1200_wlan_powerup");
- gpio_direction_output(pdata->powerup, 0);
+ gpiod_set_consumer_name(cw1200_reset, "cw1200_wlan_reset");
+ cw1200_powerup = devm_gpiod_get_optional(NULL, "powerup", GPIOD_OUT_LOW);
+ if (IS_ERR(cw1200_powerup)) {
+ pr_err("could not get CW1200 SDIO powerup GPIO\n");
+ return PTR_ERR(cw1200_powerup);
}
- if (pdata->reset || pdata->powerup)
+ gpiod_set_consumer_name(cw1200_powerup, "cw1200_wlan_powerup");
+
+ if (cw1200_reset || cw1200_powerup)
msleep(10); /* Settle time? */
/* Enable 3v3 and 1v8 to hardware */
@@ -226,13 +234,13 @@ static int cw1200_sdio_on(const struct cw1200_platform_data_sdio *pdata)
}
/* Enable POWERUP signal */
- if (pdata->powerup) {
- gpio_set_value(pdata->powerup, 1);
+ if (cw1200_powerup) {
+ gpiod_set_value(cw1200_powerup, 1);
msleep(250); /* or more..? */
}
- /* Enable RSTn signal */
- if (pdata->reset) {
- gpio_set_value(pdata->reset, 1);
+ /* Deassert RSTn signal, note active low */
+ if (cw1200_reset) {
+ gpiod_set_value(cw1200_reset, 0);
msleep(50); /* Or more..? */
}
return 0;
diff --git a/drivers/net/wireless/st/cw1200/cw1200_spi.c b/drivers/net/wireless/st/cw1200/cw1200_spi.c
index c82c0688b549..9df7f46588b4 100644
--- a/drivers/net/wireless/st/cw1200/cw1200_spi.c
+++ b/drivers/net/wireless/st/cw1200/cw1200_spi.c
@@ -11,7 +11,7 @@
*/
#include <linux/module.h>
-#include <linux/gpio.h>
+#include <linux/gpio/consumer.h>
#include <linux/delay.h>
#include <linux/spinlock.h>
#include <linux/interrupt.h>
@@ -38,6 +38,8 @@ struct hwbus_priv {
const struct cw1200_platform_data_spi *pdata;
spinlock_t lock; /* Serialize all bus operations */
wait_queue_head_t wq;
+ struct gpio_desc *reset;
+ struct gpio_desc *powerup;
int claimed;
};
@@ -275,12 +277,12 @@ static void cw1200_spi_irq_unsubscribe(struct hwbus_priv *self)
free_irq(self->func->irq, self);
}
-static int cw1200_spi_off(const struct cw1200_platform_data_spi *pdata)
+static int cw1200_spi_off(struct hwbus_priv *self, const struct cw1200_platform_data_spi *pdata)
{
- if (pdata->reset) {
- gpio_set_value(pdata->reset, 0);
+ if (self->reset) {
+ /* Assert RESET, note active low */
+ gpiod_set_value(self->reset, 1);
msleep(30); /* Min is 2 * CLK32K cycles */
- gpio_free(pdata->reset);
}
if (pdata->power_ctrl)
@@ -291,18 +293,12 @@ static int cw1200_spi_off(const struct cw1200_platform_data_spi *pdata)
return 0;
}
-static int cw1200_spi_on(const struct cw1200_platform_data_spi *pdata)
+static int cw1200_spi_on(struct hwbus_priv *self, const struct cw1200_platform_data_spi *pdata)
{
/* Ensure I/Os are pulled low */
- if (pdata->reset) {
- gpio_request(pdata->reset, "cw1200_wlan_reset");
- gpio_direction_output(pdata->reset, 0);
- }
- if (pdata->powerup) {
- gpio_request(pdata->powerup, "cw1200_wlan_powerup");
- gpio_direction_output(pdata->powerup, 0);
- }
- if (pdata->reset || pdata->powerup)
+ gpiod_direction_output(self->reset, 1); /* Active low */
+ gpiod_direction_output(self->powerup, 0);
+ if (self->reset || self->powerup)
msleep(10); /* Settle time? */
/* Enable 3v3 and 1v8 to hardware */
@@ -323,13 +319,13 @@ static int cw1200_spi_on(const struct cw1200_platform_data_spi *pdata)
}
/* Enable POWERUP signal */
- if (pdata->powerup) {
- gpio_set_value(pdata->powerup, 1);
+ if (self->powerup) {
+ gpiod_set_value(self->powerup, 1);
msleep(250); /* or more..? */
}
- /* Enable RSTn signal */
- if (pdata->reset) {
- gpio_set_value(pdata->reset, 1);
+ /* Assert RSTn signal, note active low */
+ if (self->reset) {
+ gpiod_set_value(self->reset, 0);
msleep(50); /* Or more..? */
}
return 0;
@@ -381,20 +377,33 @@ static int cw1200_spi_probe(struct spi_device *func)
spi_get_chipselect(func, 0), func->mode, func->bits_per_word,
func->max_speed_hz);
- if (cw1200_spi_on(plat_data)) {
+ self = devm_kzalloc(&func->dev, sizeof(*self), GFP_KERNEL);
+ if (!self) {
+ pr_err("Can't allocate SPI hwbus_priv.");
+ return -ENOMEM;
+ }
+
+ /* Request reset asserted */
+ self->reset = devm_gpiod_get_optional(&func->dev, "reset", GPIOD_OUT_HIGH);
+ if (IS_ERR(self->reset))
+ return dev_err_probe(&func->dev, PTR_ERR(self->reset),
+ "could not get reset GPIO\n");
+ gpiod_set_consumer_name(self->reset, "cw1200_wlan_reset");
+
+ self->powerup = devm_gpiod_get_optional(&func->dev, "powerup", GPIOD_OUT_LOW);
+ if (IS_ERR(self->powerup))
+ return dev_err_probe(&func->dev, PTR_ERR(self->powerup),
+ "could not get powerup GPIO\n");
+ gpiod_set_consumer_name(self->reset, "cw1200_wlan_powerup");
+
+ if (cw1200_spi_on(self, plat_data)) {
pr_err("spi_on() failed!\n");
- return -1;
+ return -ENODEV;
}
if (spi_setup(func)) {
pr_err("spi_setup() failed!\n");
- return -1;
- }
-
- self = devm_kzalloc(&func->dev, sizeof(*self), GFP_KERNEL);
- if (!self) {
- pr_err("Can't allocate SPI hwbus_priv.");
- return -ENOMEM;
+ return -ENODEV;
}
self->pdata = plat_data;
@@ -416,7 +425,7 @@ static int cw1200_spi_probe(struct spi_device *func)
if (status) {
cw1200_spi_irq_unsubscribe(self);
- cw1200_spi_off(plat_data);
+ cw1200_spi_off(self, plat_data);
}
return status;
@@ -434,7 +443,7 @@ static void cw1200_spi_disconnect(struct spi_device *func)
self->core = NULL;
}
}
- cw1200_spi_off(dev_get_platdata(&func->dev));
+ cw1200_spi_off(self, dev_get_platdata(&func->dev));
}
static int __maybe_unused cw1200_spi_suspend(struct device *dev)
diff --git a/include/linux/platform_data/net-cw1200.h b/include/linux/platform_data/net-cw1200.h
index c510734405bb..89d0ec6f7d46 100644
--- a/include/linux/platform_data/net-cw1200.h
+++ b/include/linux/platform_data/net-cw1200.h
@@ -14,8 +14,6 @@ struct cw1200_platform_data_spi {
/* All others are optional */
bool have_5ghz;
- int reset; /* GPIO to RSTn signal (0 disables) */
- int powerup; /* GPIO to POWERUP signal (0 disables) */
int (*power_ctrl)(const struct cw1200_platform_data_spi *pdata,
bool enable); /* Control 3v3 / 1v8 supply */
int (*clk_ctrl)(const struct cw1200_platform_data_spi *pdata,
@@ -30,8 +28,6 @@ struct cw1200_platform_data_sdio {
/* All others are optional */
bool have_5ghz;
bool no_nptb; /* SDIO hardware does not support non-power-of-2-blocksizes */
- int reset; /* GPIO to RSTn signal (0 disables) */
- int powerup; /* GPIO to POWERUP signal (0 disables) */
int irq; /* IRQ line or 0 to use SDIO IRQ */
int (*power_ctrl)(const struct cw1200_platform_data_sdio *pdata,
bool enable); /* Control 3v3 / 1v8 supply */
--
2.34.1
The file is including the legacy GPIO header <linux/gpio.h> but
is not using any symbols from it, drop the header.
Signed-off-by: Linus Walleij <[email protected]>
---
drivers/net/wireless/ti/wlcore/sdio.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/net/wireless/ti/wlcore/sdio.c b/drivers/net/wireless/ti/wlcore/sdio.c
index f0686635db46..45dcc7b400c3 100644
--- a/drivers/net/wireless/ti/wlcore/sdio.c
+++ b/drivers/net/wireless/ti/wlcore/sdio.c
@@ -16,7 +16,6 @@
#include <linux/mmc/sdio_ids.h>
#include <linux/mmc/card.h>
#include <linux/mmc/host.h>
-#include <linux/gpio.h>
#include <linux/pm_runtime.h>
#include <linux/printk.h>
#include <linux/of.h>
--
2.34.1
Linus Walleij <[email protected]> writes:
> The CW1200 uses two GPIOs to control the powerup and reset
> pins, get these from GPIO descriptors instead of being passed
> as platform data from boardfiles.
>
> The RESET line will need to be marked as active low as we will
> let gpiolib handle the polarity inversion.
>
> The SDIO case is a bit special since the "card" need to be
> powered up before it gets detected on the SDIO bus and
> properly probed. Fix this by using board-specific GPIOs
> assigned to device "NULL".
>
> There are currently no in-tree users.
>
> Signed-off-by: Linus Walleij <[email protected]>
[...]
> +/* Like the rest of the driver, this only supports one device per system */
> +static struct gpio_desc *cw1200_reset;
> +static struct gpio_desc *cw1200_powerup;
Side comment about cw1200 driver: it's pretty bad that the driver
supports only one device per system. As I haven't seen any real activity
for this driver a long time I thinking of just removing it in the near
future.
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
Linus Walleij <[email protected]> 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 <[email protected]>
This seems reasonable, thanks!
Acked-by: Toke Høiland-Jørgensen <[email protected]>
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;
> }
--
With Best Regards,
Andy Shevchenko
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 <[email protected]> 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.
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
Andy Shevchenko <[email protected]> writes:
> On Wed, Jan 31, 2024 at 11:37:19PM +0100, Linus Walleij wrote:
>> This converts some Wireless network drivers to use GPIO descriptors,
>> and some just have unused header inclusions.
>>
>> The Intersil PL54 driver is intentionally untouched because Arnd
>> is cleaning it up fully.
>
> Thanks for doing this! We pretty much want to get rid of gpio.h along with
> of_gpio.h ASAP, that's why I expect this series to be applied in a fastest
> possible manner.
This is for -next, right?
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
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);
--
With Best Regards,
Andy Shevchenko
"Arnd Bergmann" <[email protected]> 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 Wed, Jan 31, 2024 at 11:37:19PM +0100, Linus Walleij wrote:
> This converts some Wireless network drivers to use GPIO descriptors,
> and some just have unused header inclusions.
>
> The Intersil PL54 driver is intentionally untouched because Arnd
> is cleaning it up fully.
Thanks for doing this! We pretty much want to get rid of gpio.h along with
of_gpio.h ASAP, that's why I expect this series to be applied in a fastest
possible manner.
Reviewed-by: Andy Shevchenko <[email protected]>
--
With Best Regards,
Andy Shevchenko
On Thu, Feb 1, 2024 at 1:53 PM Kalle Valo <[email protected]> wrote:
> Andy Shevchenko <[email protected]> writes:
>
> > On Wed, Jan 31, 2024 at 11:37:19PM +0100, Linus Walleij wrote:
> >> This converts some Wireless network drivers to use GPIO descriptors,
> >> and some just have unused header inclusions.
> >>
> >> The Intersil PL54 driver is intentionally untouched because Arnd
> >> is cleaning it up fully.
> >
> > Thanks for doing this! We pretty much want to get rid of gpio.h along with
> > of_gpio.h ASAP, that's why I expect this series to be applied in a fastest
> > possible manner.
>
> This is for -next, right?
Yes. The urgency is "in the next few kernel cycles" not "tomorrow".
Yours,
Linus Walleij
On Thu, Feb 1, 2024, at 15:18, Toke Høiland-Jørgensen wrote:
> "Arnd Bergmann" <[email protected]> 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" <[email protected]> writes:
> On Thu, Feb 1, 2024, at 15:18, Toke Høiland-Jørgensen wrote:
>> "Arnd Bergmann" <[email protected]> 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 <[email protected]> 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 <[email protected]>
> Acked-by: Toke Høiland-Jørgensen <[email protected]>
I understood that there will be a new version for ath9k so dropping
patch 1.
Patch set to Changes Requested.
--
https://patchwork.kernel.org/project/linux-wireless/patch/[email protected]/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
Linus Walleij <[email protected]> wrote:
> The file is including the legacy GPIO header <linux/gpio.h> but
> is not using any symbols from it, drop the header.
>
> Signed-off-by: Linus Walleij <[email protected]>
5 patches applied to wireless-next.git, thanks.
04e9c8af8b2d wifi: ti: wlcore: sdio: Drop unused include
b303de763b63 wifi: brcmsmac: Drop legacy header
163857d99531 wifi: mwifiex: Drop unused headers
d8da5a213812 wifi: plfxlc: Drop unused include
2719a9e7156c wifi: cw1200: Convert to GPIO descriptors
--
https://patchwork.kernel.org/project/linux-wireless/patch/[email protected]/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches