Re: [PATCH v13] ath10k: add LED and GPIO controlling support for various chipsets
Am 09.04.2018 um 17:49 schrieb Kalle Valo: Sebastian Gottschallwrites: you removed the call to leds_start from certain locations but you seem to have ignored the comment i wrote above the function call. IIRC I moved the comment to ath10k_leds_start(). there is a reason why i reinitialize the gpio output state in these locations. the firmware for 9984 and 99xx resets the gpio registers at certain points. this will lead to a non working led code. What are the certain points exactly? I tried to be careful that from firmware's point of view the functionality is the same, even if I moved the call to a different location. Did you test the patch? Is it broken now? i reviewed the firmware code as well, but havent found the reason. can be core/chipset specific and not firmware software related. i just can say it doesnt happen on 988x, just newer chipsets are affected The naming refers to phases of ath10k initialisation which are: create() - destroy() register() - unregister() start() - stop() So the naming doesn't mean that every ath10k_foo_start() has to start something, it just describes the phase of driver initialisation it's called in. yes, but its not a initialisation too. its just a gpio pin reset. the initialisation is the trigger code itself from my point of view Is this really so frequently called that we need to think about CPU time? How often are we expecting the LED state to change? But I'm not really following you, from firmware point of view the functionality should be the same as with your patch. a typical tpt trigger as used in openwrt for instance may trigger it several times per second. i mean it might be really just a micro effect, but i just save cpu time whereever i can since i'm focused in embedded development so if you want to follow this up. remove ath10k_leds_start and insert ath10k_wmi_gpio_config(ar, ar->hw_params.led_pin, 0, WMI_GPIO_PULL_NONE, WMI_GPIO_INTTYPE_DISABLE); in ath10k_leds_set_brightness_blocking Calling ath10k_wmi_gpio_config() every time sounds like quite odd to me and your patch didn't do that either. Are you sure this is really needed? yes. i tested this. you must set the gpio to output before setting it to any value, in case the output state was resetted back to input or any other default value. (which it does on 9984 at certain events) i tested this on a netgear r7800 which is ipq8064 based with 2 9984 chipsets. but i cannot reproduce this on mips routers with 988x chipsets. but as you say. its odd to call it every time. so i just call it with the reset method when neccessary. in our case, when interface operation starts. -- Mit freundlichen Grüssen / Regards Sebastian Gottschall / CTO NewMedia-NET GmbH - DD-WRT Firmensitz: Stubenwaldallee 21a, 64625 Bensheim Registergericht: Amtsgericht Darmstadt, HRB 25473 Geschäftsführer: Peter Steinhäuser, Christian Scheele http://www.dd-wrt.com email: s.gottsch...@dd-wrt.com Tel.: +496251-582650 / Fax: +496251-5826565
Re: [PATCH v13] ath10k: add LED and GPIO controlling support for various chipsets
Sebastian Gottschallwrites: > you removed the call to leds_start from certain locations but you seem > to have ignored the comment i wrote above the function call. IIRC I moved the comment to ath10k_leds_start(). > there is a reason why i reinitialize the gpio output state in these > locations. the firmware for 9984 and 99xx resets the gpio registers > at certain points. this will lead to a non working led code. What are the certain points exactly? I tried to be careful that from firmware's point of view the functionality is the same, even if I moved the call to a different location. Did you test the patch? Is it broken now? > so you must set the gpio output to high again and this is the reason > why the function is called "reset_led_pin" and not led_start because > it doesnt start the led in any way. The naming refers to phases of ath10k initialisation which are: create() - destroy() register() - unregister() start() - stop() So the naming doesn't mean that every ath10k_foo_start() has to start something, it just describes the phase of driver initialisation it's called in. > it just resets the output state there is only one work around you may > do. you set the gpio out register to high on every led trigger, but > this is what i wanted to avoid to save cpu time since a wmi call is > more expensive than just a register write. Is this really so frequently called that we need to think about CPU time? How often are we expecting the LED state to change? But I'm not really following you, from firmware point of view the functionality should be the same as with your patch. > so if you want to follow this up. remove ath10k_leds_start > and insert > > ath10k_wmi_gpio_config(ar, ar->hw_params.led_pin, 0, > WMI_GPIO_PULL_NONE, WMI_GPIO_INTTYPE_DISABLE); > > in ath10k_leds_set_brightness_blocking Calling ath10k_wmi_gpio_config() every time sounds like quite odd to me and your patch didn't do that either. Are you sure this is really needed? -- Kalle Valo
Re: [PATCH v13] ath10k: add LED and GPIO controlling support for various chipsets
Hi On 2018-04-06, Kalle Valo wrote: > From: Sebastian Gottschall> > Adds LED and GPIO Control support for 988x, 9887, 9888, 99x0, 9984 based > chipsets with on chipset connected led's using WMI Firmware API. The LED > device will get available named as "ath10k-phyX" at sysfs and can be > controlled > with various triggers. adds also debugfs interface for gpio control. > > Signed-off-by: Sebastian Gottschall > Reviewed-by: Steve deRosier > [kvalo: major reorg and cleanup] > Signed-off-by: Kalle Valo Tested-by: Stefan Lippers-Hollmann [...] I've been able to test v13 of this patch, including the changes regarding removing ath10k_leds_start() and moving the ath10k_wmi_gpio_config() call into ath10k_leds_set_brightness_blocking() as raised by Sebastian Gottschall, on a ZyXEL NBG6817 (using two QCA9984 wlan cards and firmware 10.4-3.5.3-00053) running OpenWrt/ master r6644-eda27d7557 (ipq8065, kernel 4.9.92 with backports wt-2017-11-01-0-gfe248fc2c180/ v4.14-rc2-1-31-g86cf0e5d). /sys/class/leds/ath10k-phy0/ and /sys/class/leds/ath10k-phy1/ are registered correctly and the white wlan LEDs connected to the ath10k GPIOs are now working as designed (controlled via phy0tpt/ phy1tpt). Thanks a lot to everyone involved! Regards Stefan Lippers-Hollmann
Re: [PATCH v13] ath10k: add LED and GPIO controlling support for various chipsets
Hello Kalle you removed the call to leds_start from certain locations but you seem to have ignored the comment i wrote above the function call. there is a reason why i reinitialize the gpio output state in these locations. the firmware for 9984 and 99xx resets the gpio registers at certain points. this will lead to a non working led code. so you must set the gpio output to high again and this is the reason why the function is called "reset_led_pin" and not led_start because it doesnt start the led in any way. it just resets the output state there is only one work around you may do. you set the gpio out register to high on every led trigger, but this is what i wanted to avoid to save cpu time since a wmi call is more expensive than just a register write. so if you want to follow this up. remove ath10k_leds_start and insert ath10k_wmi_gpio_config(ar, ar->hw_params.led_pin, 0, WMI_GPIO_PULL_NONE, WMI_GPIO_INTTYPE_DISABLE); in ath10k_leds_set_brightness_blocking but in any way the function name ath10k_leds_start is wrong. it doesnt describe the behaviour of the function Am 06.04.2018 um 17:17 schrieb Kalle Valo: From: Sebastian GottschallAdds LED and GPIO Control support for 988x, 9887, 9888, 99x0, 9984 based chipsets with on chipset connected led's using WMI Firmware API. The LED device will get available named as "ath10k-phyX" at sysfs and can be controlled with various triggers. adds also debugfs interface for gpio control. Signed-off-by: Sebastian Gottschall Reviewed-by: Steve deRosier [kvalo: major reorg and cleanup] Signed-off-by: Kalle Valo --- v13: * only compile tested! * fix all checkpatch warnings * fix commit log * sizeof(struct ath10k_gpiocontrol) -> sizeof(*gpio) * unsigned -> unsigned int * remove GPIOLIB code, that should be added in a separate patch * rename gpio.c to leds.c * add leds.h * rename some functions: ath10k_attach_led() -> ath10k_leds_register() ath10k_unregister_led() -> ath10k_leds_unregister() ath10k_reset_led_pin() -> ath10k_leds_start() * call ath10k_leds_unregister() before ath10k_thermal_unregister() to preserve ordering * call ath10k_leds_start() only from ath10k_core_start() and not from mac.c * rename struct ath10k_gpiocontrol as anonymous function under struct ath10k::leds, no need for memory allocation * merge ath10k_add_led() to ath10k_attach_led(), which is it's only caller * remove #if IS_ENABLED() checks from most of places, memory savings from those were not worth it * Kconfig help text improvement and move it lower in the menu, also don't enable it by default * switch to set_brightness_blocking() so that the callback can sleep, then no need to use ath10k_wmi_cmd_send_nowait() and can take mutex to access ar->state * don't touch ath10k_wmi_pdev_get_temperature() * as QCA6174/QCA9377 are not (yet) supported don't add the command to WMI-TLV interface * remove debugfs interface, that should be added in another patch * cleanup includes drivers/net/wireless/ath/ath10k/Kconfig | 10 +++ drivers/net/wireless/ath/ath10k/Makefile | 1 + drivers/net/wireless/ath/ath10k/core.c| 22 +++ drivers/net/wireless/ath/ath10k/core.h| 9 ++- drivers/net/wireless/ath/ath10k/hw.h | 1 + drivers/net/wireless/ath/ath10k/leds.c| 103 ++ drivers/net/wireless/ath/ath10k/leds.h| 45 + drivers/net/wireless/ath/ath10k/mac.c | 1 + drivers/net/wireless/ath/ath10k/wmi-ops.h | 32 ++ drivers/net/wireless/ath/ath10k/wmi-tlv.c | 2 + drivers/net/wireless/ath/ath10k/wmi.c | 54 drivers/net/wireless/ath/ath10k/wmi.h | 35 ++ 12 files changed, 314 insertions(+), 1 deletion(-) create mode 100644 drivers/net/wireless/ath/ath10k/leds.c create mode 100644 drivers/net/wireless/ath/ath10k/leds.h diff --git a/drivers/net/wireless/ath/ath10k/Kconfig b/drivers/net/wireless/ath/ath10k/Kconfig index deb5ae21a559..c4108c1bbe92 100644 --- a/drivers/net/wireless/ath/ath10k/Kconfig +++ b/drivers/net/wireless/ath/ath10k/Kconfig @@ -52,6 +52,16 @@ config ATH10K_DEBUGFS If unsure, say Y to make it easier to debug problems. +config ATH10K_LEDS + bool "Atheros ath10k LED support" + depends on ATH10K + select MAC80211_LEDS + select LEDS_CLASS + select NEW_LEDS + default y + help + This option is necessary, if you want LED support for chipset connected led pins. If unsure, say N. + config ATH10K_SPECTRAL bool "Atheros ath10k spectral scan support" depends on ATH10K_DEBUGFS diff --git a/drivers/net/wireless/ath/ath10k/Makefile b/drivers/net/wireless/ath/ath10k/Makefile index 6739ac26fd29..16c822421c39 100644 --- a/drivers/net/wireless/ath/ath10k/Makefile +++
Re: [PATCH v13] ath10k: add LED and GPIO controlling support for various chipsets
Kalle Valowrites: > From: Sebastian Gottschall > > Adds LED and GPIO Control support for 988x, 9887, 9888, 99x0, 9984 based > chipsets with on chipset connected led's using WMI Firmware API. The LED > device will get available named as "ath10k-phyX" at sysfs and can be > controlled > with various triggers. adds also debugfs interface for gpio control. > > Signed-off-by: Sebastian Gottschall >From and Signed-off-by fields do not match. Sebastian, which domain should it be? > Reviewed-by: Steve deRosier > [kvalo: major reorg and cleanup] > Signed-off-by: Kalle Valo > --- > > v13: > > * only compile tested! So testing is very welcome. > --- a/drivers/net/wireless/ath/ath10k/Kconfig > +++ b/drivers/net/wireless/ath/ath10k/Kconfig > @@ -52,6 +52,16 @@ config ATH10K_DEBUGFS > > If unsure, say Y to make it easier to debug problems. > > +config ATH10K_LEDS > + bool "Atheros ath10k LED support" > + depends on ATH10K > + select MAC80211_LEDS > + select LEDS_CLASS > + select NEW_LEDS > + default y > + help > + This option is necessary, if you want LED support for chipset > connected led pins. If unsure, say N. > + I started to wonder is this Kconfig option even needed, feels pretty pointless. Why not just make config ATH10K depend on LED stuff, just like ath9k already does? > --- a/drivers/net/wireless/ath/ath10k/core.h > +++ b/drivers/net/wireless/ath/ath10k/core.h > @@ -25,6 +25,7 @@ > #include > #include > #include > +#include > > #include "htt.h" > #include "htc.h" > @@ -883,7 +884,6 @@ struct ath10k { > u32 low_5ghz_chan; > u32 high_5ghz_chan; > bool ani_enabled; > - > bool p2p; > > struct { Unrelated change, I'll fix that in v14. -- Kalle Valo
[PATCH v13] ath10k: add LED and GPIO controlling support for various chipsets
From: Sebastian GottschallAdds LED and GPIO Control support for 988x, 9887, 9888, 99x0, 9984 based chipsets with on chipset connected led's using WMI Firmware API. The LED device will get available named as "ath10k-phyX" at sysfs and can be controlled with various triggers. adds also debugfs interface for gpio control. Signed-off-by: Sebastian Gottschall Reviewed-by: Steve deRosier [kvalo: major reorg and cleanup] Signed-off-by: Kalle Valo --- v13: * only compile tested! * fix all checkpatch warnings * fix commit log * sizeof(struct ath10k_gpiocontrol) -> sizeof(*gpio) * unsigned -> unsigned int * remove GPIOLIB code, that should be added in a separate patch * rename gpio.c to leds.c * add leds.h * rename some functions: ath10k_attach_led() -> ath10k_leds_register() ath10k_unregister_led() -> ath10k_leds_unregister() ath10k_reset_led_pin() -> ath10k_leds_start() * call ath10k_leds_unregister() before ath10k_thermal_unregister() to preserve ordering * call ath10k_leds_start() only from ath10k_core_start() and not from mac.c * rename struct ath10k_gpiocontrol as anonymous function under struct ath10k::leds, no need for memory allocation * merge ath10k_add_led() to ath10k_attach_led(), which is it's only caller * remove #if IS_ENABLED() checks from most of places, memory savings from those were not worth it * Kconfig help text improvement and move it lower in the menu, also don't enable it by default * switch to set_brightness_blocking() so that the callback can sleep, then no need to use ath10k_wmi_cmd_send_nowait() and can take mutex to access ar->state * don't touch ath10k_wmi_pdev_get_temperature() * as QCA6174/QCA9377 are not (yet) supported don't add the command to WMI-TLV interface * remove debugfs interface, that should be added in another patch * cleanup includes drivers/net/wireless/ath/ath10k/Kconfig | 10 +++ drivers/net/wireless/ath/ath10k/Makefile | 1 + drivers/net/wireless/ath/ath10k/core.c| 22 +++ drivers/net/wireless/ath/ath10k/core.h| 9 ++- drivers/net/wireless/ath/ath10k/hw.h | 1 + drivers/net/wireless/ath/ath10k/leds.c| 103 ++ drivers/net/wireless/ath/ath10k/leds.h| 45 + drivers/net/wireless/ath/ath10k/mac.c | 1 + drivers/net/wireless/ath/ath10k/wmi-ops.h | 32 ++ drivers/net/wireless/ath/ath10k/wmi-tlv.c | 2 + drivers/net/wireless/ath/ath10k/wmi.c | 54 drivers/net/wireless/ath/ath10k/wmi.h | 35 ++ 12 files changed, 314 insertions(+), 1 deletion(-) create mode 100644 drivers/net/wireless/ath/ath10k/leds.c create mode 100644 drivers/net/wireless/ath/ath10k/leds.h diff --git a/drivers/net/wireless/ath/ath10k/Kconfig b/drivers/net/wireless/ath/ath10k/Kconfig index deb5ae21a559..c4108c1bbe92 100644 --- a/drivers/net/wireless/ath/ath10k/Kconfig +++ b/drivers/net/wireless/ath/ath10k/Kconfig @@ -52,6 +52,16 @@ config ATH10K_DEBUGFS If unsure, say Y to make it easier to debug problems. +config ATH10K_LEDS + bool "Atheros ath10k LED support" + depends on ATH10K + select MAC80211_LEDS + select LEDS_CLASS + select NEW_LEDS + default y + help + This option is necessary, if you want LED support for chipset connected led pins. If unsure, say N. + config ATH10K_SPECTRAL bool "Atheros ath10k spectral scan support" depends on ATH10K_DEBUGFS diff --git a/drivers/net/wireless/ath/ath10k/Makefile b/drivers/net/wireless/ath/ath10k/Makefile index 6739ac26fd29..16c822421c39 100644 --- a/drivers/net/wireless/ath/ath10k/Makefile +++ b/drivers/net/wireless/ath/ath10k/Makefile @@ -20,6 +20,7 @@ ath10k_core-$(CONFIG_NL80211_TESTMODE) += testmode.o ath10k_core-$(CONFIG_ATH10K_TRACING) += trace.o ath10k_core-$(CONFIG_THERMAL) += thermal.o ath10k_core-$(CONFIG_MAC80211_DEBUGFS) += debugfs_sta.o +ath10k_core-$(CONFIG_ATH10K_LEDS) += leds.o ath10k_core-$(CONFIG_PM) += wow.o ath10k_core-$(CONFIG_DEV_COREDUMP) += coredump.o diff --git a/drivers/net/wireless/ath/ath10k/core.c b/drivers/net/wireless/ath/ath10k/core.c index 8a3020dbd4cf..be5958e98102 100644 --- a/drivers/net/wireless/ath/ath10k/core.c +++ b/drivers/net/wireless/ath/ath10k/core.c @@ -34,6 +34,7 @@ #include "testmode.h" #include "wmi-ops.h" #include "coredump.h" +#include "leds.h" unsigned int ath10k_debug_mask; static unsigned int ath10k_cryptmode_param; @@ -66,6 +67,7 @@ static const struct ath10k_hw_params ath10k_hw_params_list[] = { .id = QCA988X_HW_2_0_VERSION, .dev_id = QCA988X_2_0_DEVICE_ID, .name = "qca988x hw2.0", + .led_pin = 1, .patch_load_addr = QCA988X_HW_2_0_PATCH_LOAD_ADDR, .uart_pin = 7, .cc_wraparound_type = ATH10K_HW_CC_WRAP_SHIFTED_ALL, @@ -95,6