Re: [PATCH v3] wifi: ath9k: Obtain system GPIOS from descriptors

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Thu, Mar 12, 2026 at 04:09:53PM +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.
> 
> Register all 32 GPIOs at index 0..31 directly in the ATH79K
> GPIO driver and associate with the NULL device (making them
> widely available) if and only if we are probing ATH79K wifi
> from the AHB bus (used for SoCs). We obtain these offsets from
> the NULL device if necessary.
> 
> These GPIOs should ideally be defined in the device tree
> instead, but we have no control over that for the legacy
> code path.
> 
> Testcompiled with the ath79 defconfig.

...

> +#define ATH79K_WIFI_DESCS 32
> +static int ath79_gpio_register_wifi_descriptors(struct device *dev,
> +						const char *label)
> +{
> +	struct gpiod_lookup_table *lookup;
> +	int i;
> +
> +	/* Create a gpiod lookup using gpiochip-local offsets + 1 for NULL */
> +	lookup = devm_kzalloc(dev,
> +			      struct_size(lookup, table, ATH79K_WIFI_DESCS + 1),
> +			      GFP_KERNEL);

> +

Redundant blank line.

> +	if (!lookup)
> +		return -ENOMEM;
> +
> +	/*
> +	 * Ugly system-wide lookup for the NULL device: we know this
> +	 * is already NULL but explicitly assign it here for people to
> +	 * know what is going on. (Yes this is an ugly legacy hack, live
> +	 * with it.)
> +	 */
> +	lookup->dev_id = NULL;
> +
> +	for (i = 0; i < ATH79K_WIFI_DESCS; i++) {
> +		lookup->table[i] = (struct gpiod_lookup)

The macro below is already compound literal, the '(struct gpiod_lookup)'
is redundant.

> +			/*
> +			 * Set the HW offset on the chip and the lookup
> +			 * index to the same value, so looking up index 0
> +			 * will get HW offset 0, index 1 HW offset 1 etc.
> +			 */
> +			GPIO_LOOKUP_IDX(label, i, "ath9k", i, GPIO_ACTIVE_HIGH);
> +	}
> +
> +	gpiod_add_lookup_table(lookup);
> +
> +	return 0;
> +}

...

> +	/*
> +	 * Obtains a system specific GPIO descriptor from another GPIO controller.
> +	 * Ideally this should come from the device tree, this is a legacy code
> +	 * path.
> +	 */
> +	gpiod = gpiod_get_index(NULL, "ath9k", gpio, flags);

> +

Redundant blank line.

> +	if (IS_ERR(gpiod)) {
> +		err = PTR_ERR(gpiod);

What about

	err = PTR_ERR_OR_ZERO(...);
	if (err) {
		...

?

>  		ath_err(ath9k_hw_common(ah), "request GPIO%d failed:%d\n",
>  			gpio, err);
>  		return;
>  	}

...

Have you considered using software nodes instead?

-- 
With Best Regards,
Andy Shevchenko