Re: [RFC net-next/wireless-next v1 2/2] ath10k: move device_get_mac_address() and pass errors up the chain

2021-11-02 Thread Mathias Kresin

Hey Christian,

just a drive-by comment inline.

Mathias

10/30/21 7:41 PM, Christian Lamparter:

device_get_mac_address() can now return -EPROBE_DEFER.
This has to be passed back to the device subsystem so
the driver will be probed again at a later time.

This was somewhat involved because the best place for this
seemed in ath10k_core_create() right after allocation.
Thing is that ath10k_core_create() was setup to either
return a valid ath10k* instance, or NULL. So each ath10k
implementation has to be modified to account for ERR_PTR.

This introduces a new side-effect: the returned error codes
from ath10k_core_create() will now be passed along. It's no
longer just -ENOMEM.

Note: If device_get_mac_address() didn't get a valid MAC from
either the DT/ACPI, nvmem, etc... the driver will just generate
random MAC (same as it did before).

Signed-off-by: Christian Lamparter 
---
@Kalle from what I can tell, this is how nvmem-mac could be
done with the existing device_get_mac_address() - at a
different place. The reason for the move was that -EPROBE_DEFER
needs to be returned by the pci/usb/snoc/ahb _probe functions().
This wasn't possible in the old location. As ath10k deferres
the "bring-up" process into a workqueue task which can't return
any errors (it just printk/dev_err them at the end).
Also, When I was asking around about this. The common consensus was
to just post it and see. This is based on net-next + wireless-testing

  drivers/net/wireless/ath/ath10k/ahb.c  |  8 +---
  drivers/net/wireless/ath/ath10k/core.c | 14 --
  drivers/net/wireless/ath/ath10k/pci.c  |  8 +---
  drivers/net/wireless/ath/ath10k/sdio.c |  8 +---
  drivers/net/wireless/ath/ath10k/snoc.c |  8 +---
  drivers/net/wireless/ath/ath10k/usb.c  |  8 +---
  6 files changed, 33 insertions(+), 21 deletions(-)

  


diff --git a/drivers/net/wireless/ath/ath10k/ahb.c 
b/drivers/net/wireless/ath/ath10k/ahb.c
index ab8f77ae5e66..ad282a06b376 100644
--- a/drivers/net/wireless/ath/ath10k/ahb.c
+++ b/drivers/net/wireless/ath/ath10k/ahb.c
@@ -745,9 +745,11 @@ static int ath10k_ahb_probe(struct platform_device *pdev)
size = sizeof(*ar_pci) + sizeof(*ar_ahb);
ar = ath10k_core_create(size, >dev, ATH10K_BUS_AHB,
hw_rev, _ahb_hif_ops);
-   if (!ar) {
-   dev_err(>dev, "failed to allocate core\n");
-   return -ENOMEM;
+   if (IS_ERR(ar)) {
+   ret = PTR_ERR(ar);
+   if (ret != -EPROBE_DEFER)
+   dev_err(>dev, "failed to allocate core: %d\n", 
ret);


There's a helper for that: dev_err_probe().


+   return ret;
}
  
  	ath10k_dbg(ar, ATH10K_DBG_BOOT, "ahb probe\n");

diff --git a/drivers/net/wireless/ath/ath10k/core.c 
b/drivers/net/wireless/ath/ath10k/core.c
index 72a366aa9f60..85d2e8143101 100644
--- a/drivers/net/wireless/ath/ath10k/core.c
+++ b/drivers/net/wireless/ath/ath10k/core.c
@@ -3291,8 +3291,6 @@ static int ath10k_core_probe_fw(struct ath10k *ar)
ath10k_debug_print_board_info(ar);
}
  
-	device_get_mac_address(ar->dev, ar->mac_addr);

-
ret = ath10k_core_init_firmware_features(ar);
if (ret) {
ath10k_err(ar, "fatal problem with firmware features: %d\n",
@@ -3451,11 +3449,11 @@ struct ath10k *ath10k_core_create(size_t priv_size, 
struct device *dev,
  const struct ath10k_hif_ops *hif_ops)
  {
struct ath10k *ar;
-   int ret;
+   int ret = -ENOMEM;
  
  	ar = ath10k_mac_create(priv_size);

if (!ar)
-   return NULL;
+   goto err_out;
  
  	ar->ath_common.priv = ar;

ar->ath_common.hw = ar->hw;
@@ -3464,6 +3462,10 @@ struct ath10k *ath10k_core_create(size_t priv_size, 
struct device *dev,
ar->hif.ops = hif_ops;
ar->hif.bus = bus;
  
+	ret = device_get_mac_address(dev, ar->mac_addr);

+   if (ret == -EPROBE_DEFER)
+   goto err_free_mac;
+
switch (hw_rev) {
case ATH10K_HW_QCA988X:
case ATH10K_HW_QCA9887:
@@ -3580,8 +3582,8 @@ struct ath10k *ath10k_core_create(size_t priv_size, 
struct device *dev,
destroy_workqueue(ar->workqueue);
  err_free_mac:
ath10k_mac_destroy(ar);
-
-   return NULL;
+err_out:
+   return ERR_PTR(ret);
  }
  EXPORT_SYMBOL(ath10k_core_create);
  
diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c

index 4d4e2f91e15c..f4736148a382 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -3602,9 +3602,11 @@ static int ath10k_pci_probe(struct pci_dev *pdev,
  
  	ar = ath10k_core_create(sizeof(*ar_pci), >dev, ATH10K_BUS_PCI,

hw_rev, _pci_hif_ops);
-   if (!ar) {
-   dev_err(>dev, "failed to allocate core\n");
-   return -ENOMEM;
+   if (IS_ERR(ar)) {
+   ret = PTR_ERR(ar);
+

Re: [PATCH v12] ath10k: add LED and GPIO controlling support for various chipsets

2018-03-12 Thread Mathias Kresin

10.03.2018 08:56, Sebastian Gottschall:



taken a look at the specific code, and from my point of view the code
that sets up the LED (including callback) is so trivial that it's simply
not worth dealing with adding the leds-gpio driver to the mix.
It adds extra complexity and an extra dependency for no reason at all.
There's no extra functionality to be gained by using it.

Stupid question: If the LED driver isn't using the GPIO subsystem
(when enabled), what happens if the user uses the GPIO subsystem to
fiddle with the pin the LED is connected to?


in our case here it can coexist and will not have a negative effect 
since the led will still run. (except if you reconfigure the gpio to input)

for sure it makes no sense.


Well, it will have negative effects as I noticed in OpenWrt. If the same 
GPIO is controlled by the internal LED function it can't be really used 
via the GPIO controller as there will be "bogus" changes of the GPIO pin 
state.


but however i can block the gpio for beeing reused if the led driver 
took it. this has been made in ath9k for instance.


Most likely I'm not aware of the upstream code you are talking about. 
But OpenWrt has a custom patch which registers the ath9k pins as GPIO 
controller.


As soon as there are pins, manufactures will use them and the assumption 
about a default led connected to a specific pin will fail.


I've seen (home)routers routers using the "default" ath9k LED pin as 
button or for LEDs with a different purpose than "wireless". I never was 
able to find any kind of information which explains why exactly this 
specific pin is used for the ath9k-led. But that's another story.


My solution for OpenWrt was a patch which prevents the creation of the 
ath9k-led if the "default" pin is used as GPIO.


If mach files are used, the ath9k led isn't created (ath9k led pin is 
set to -1) in case the platform data have a button or led using the same 
pin. If the device tree is used, the ath9k led isn't created (ath9k led 
pin is set to -1) if the devicetree node for the ath9k device has the 
gpio-controller property. I'm not really proud of the code and it can be 
most likely done better, but it fixes the issue outlined above.


I've done it this way since the use of the GPIO controller should always 
override any kind of default LEDs.


In the end ath[0|10]k-led is only required for hardware configurations 
where the wireless isn't fixed like with miniPCIe, USB ... wireless 
cards. If the configuration is fixed - like it is for most of the 
(home)routers - the GPIO controller can be registered via the devicetree 
and gpio-leds can be used. Something similar can be most likely done via 
mach files (I barely touch mach files).


I'm aware of the issue since the first version of your patch. But since 
I'm very interested in getting the ath10k pins exposed as gpio 
controller, I planned to add a workaround similar to what we have for 
ath9k to OpenWrt.


Mathias

___
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k