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

[Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>]

On Mon, Jan 06, 2025 at 07:38:42AM +0100, Michał Kępień wrote:
> Hi Linus,
> 
> > 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 WIFI if and only if we are probing
> > ATH79K wifi from the AHB bus (used for SoCs).
> 
> I don't know how likely it is today that this patch will get merged,

I believe the idea is to have this (okay, something that moves to GPIO descriptors)
merged at some point, better sooner than later.

> but it turned out to be useful for fixing an OpenWRT issue [1][2].  However,
> the patch required some tweaking in order to make it work, so I assumed
> it cannot hurt to provide some feedback on it.

Feedback is much appreciated, esp. from the real users on real HW!

...

> > +	lookup->dev_id = "ath9k";
> 
> Since the devm_gpiod_get_index() call in ath9k_hw_gpio_cfg_soc() passes
> ah->dev as the first argument, "ath9k" is not the string that
> gpiod_find_lookup_table() will use for matching the lookup table;
> instead, it will be the wireless device's name, e.g. "18100000.wmac" on
> my router (which is built on Atheros 9344).  This causes
> devm_gpiod_get_index() to return -ENOENT [3].

Yeah, there is a fundamental issue with this patch. The part that adds a GPIO
table has to be part either of:
1) Device Tree (or other firmware description);
2) board file;
3) quirk in the wireless driver.

The GPIO driver won't ever know the all of the details of the zillion of
possible platforms on this chip and the resource configurations.

...

> > +	for (i = 0; i < ATH79K_WIFI_DESCS; i++) {
> > +		lookup->table[i] = (struct gpiod_lookup)
> > +			GPIO_LOOKUP_IDX(label, 0, NULL, i,
> 
> This sets the chip_hwnum member of every registered lookup table entry
> to 0 (second GPIO_LOOKUP_IDX() argument), which causes all 32 GPIOs
> registered here to be erroneously mapped to the GPIO chip's first line.
> I believe the second argument for GPIO_LOOKUP_IDX() should also be 'i'
> here - or at least that is what made the patch work for me (after fixing
> the lookup table matching issue).

Good catch! (Also note my comments to the patch which I done previously).

...

> > +	/* Obtains a system specific GPIO descriptor from another GPIO controller */
> > +	gpiod = devm_gpiod_get_index(ah->dev, NULL, gpio, flags);
> 
> Since using the resource-managed version of gpiod_get_index() requires
> providing a valid pointer to a struct device as the first argument and
> the name of that device is not going to be "ath9k", some other means of
> matching this call with the lookup table registered in
> ath79_gpio_register_wifi_descriptors() needs to be devised.
> 
> I resorted to the NULL-matching fallback in gpiod_find_lookup_table(),
> which enables a lookup table with dev_id set to NULL to be matched for a
> gpiod_get_index() call with dev also set to NULL, coupled with setting
> con_id in all the lookup table entries and in the gpiod_get_index() call
> to a matching string.

Yeah, but this way it's even worse hack :-(.
So, the only driver that knows about the device name is the Wi-Fi driver.
Otherwise this should come by other means as I listed above.

-- 
With Best Regards,
Andy Shevchenko