On Fri, Jun 16, 2023 at 08:03:23PM +0300, Kalle Valo wrote:
Christian Marangi <ansuelsmth@xxxxxxxxx> writes:
From: Sebastian Gottschall <s.gottschall@xxxxxxxxxx>
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 <s.gottschall@xxxxxxxxxx>
Reviewed-by: Steve deRosier <derosier@xxxxxxxxxxxxxx>
[kvalo: major reorg and cleanup]
Signed-off-by: Kalle Valo <kvalo@xxxxxxxxxxxxxx>
[ansuel: rebase and small cleanup]
Signed-off-by: Christian Marangi <ansuelsmth@xxxxxxxxx>
Tested-by: Stefan Lippers-Hollmann <s.l-h@xxxxxx>
---
Hi,
this is a very old patch from 2018 that somehow was talked till 2020
with Kavlo asked to rebase and resubmit and nobody did.
So here we are in 2023 with me trying to finally have this upstream.
A summarize of the situation.
- The patch is from years in OpenWRT. Used by anything that has ath10k
card and a LED connected.
- This patch is also used by the fw variant from Candela Tech with no
problem reported.
- It was pointed out that this caused some problem with ipq4019 SoC
but the problem was actually caused by a different bug related to
interrupts.
I honestly hope we can have this feature merged since it's really
funny to have something that was so near merge and jet still not
present and with devices not supporting this simple but useful
feature.
Indeed, we should finally get this in. Thanks for working on it.
I did some minor changes to the patch, they are in my pending branch:
https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/commit/?h=pending&id=686464864538158f22842dc49eddea6fa50e59c1
My comments below, please review my changes. No need to resend because
of these.
Hi,
very happy this is going further.
--- a/drivers/net/wireless/ath/ath10k/Kconfig
+++ b/drivers/net/wireless/ath/ath10k/Kconfig
@@ -67,6 +67,23 @@ 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 enables LEDs support for chipset LED pins.
+ Each pin is connected via GPIO and can be controlled using
+ WMI Firmware API.
+
+ The LED device will get available named as "ath10k-phyX" at sysfs and
+ can be controlled with various triggers.
+
+ Say Y, if you have LED pins connected to the ath10k wireless card.
I'm not sure anymore if we should ask anything from the user, better to
enable automatically if LED support is enabled in the kernel. So I
simplified this to:
config ATH10K_LEDS
bool
depends on ATH10K
depends on LEDS_CLASS=y || LEDS_CLASS=MAC80211
default y
This follows what mt76 does:
config MT76_LEDS
bool
depends on MT76_CORE
depends on LEDS_CLASS=y || MT76_CORE=LEDS_CLASS
default y
I remember there was the same discussion in a previous series. OK for me
for making this by default, only concern is any buildbot error (if any)
Anyway OK for the change.
@@ -65,6 +66,7 @@ static const struct ath10k_hw_params
ath10k_hw_params_list[] = {
.dev_id = QCA988X_2_0_DEVICE_ID,
.bus = ATH10K_BUS_PCI,
.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,
I prefer following the field order from struct ath10k_hw_params
declaration and also setting fields explicitly to zero (even though
there are gaps still) so I changed that for every entry.
Thanks for the change, np for me.
+int ath10k_leds_register(struct ath10k *ar)
+{
+ int ret;
+
+ if (ar->hw_params.led_pin == 0)
+ /* leds not supported */
+ return 0;
+
+ snprintf(ar->leds.label, sizeof(ar->leds.label), "ath10k-%s",
+ wiphy_name(ar->hw->wiphy));
+ ar->leds.wifi_led.active_low = 1;
+ ar->leds.wifi_led.gpio = ar->hw_params.led_pin;
+ ar->leds.wifi_led.name = ar->leds.label;
+ ar->leds.wifi_led.default_state = LEDS_GPIO_DEFSTATE_KEEP;
+
+ ar->leds.cdev.name = ar->leds.label;
+ ar->leds.cdev.brightness_set_blocking = ath10k_leds_set_brightness_blocking;
+
+ /* FIXME: this assignment doesn't make sense as it's NULL, remove it? */
+ ar->leds.cdev.default_trigger = ar->leds.wifi_led.default_trigger;
But what to do with this FIXME?
It was pushed by you in v13.
I could be wrong but your idea was to prepare for future support of
other patch that would set the default_trigger to the mac80211 tpt.
We might got both confused by default_trigger and default_state.
default_trigger is actually never set and is NULL (actually it's 0)
We have other 2 patch that adds tpt rates for the mac80211 LED trigger
and set this trigger as the default one but honestly I would chose a
different implementation than hardcoding everything.
If it's ok for you, I would drop the comment and the default_trigger and
I will send a follow-up patch to this adding DT support by using
led_classdev_register_ext and defining init_data.
(and this indirectly would permit better LED naming and defining of
default-trigger in DT)
Also ideally I will also send a patch for default_state following
standard LED implementation. (to set default_state in DT)
I would prefer this approach as the LED patch already took way too much
time and I think it's better to merge this initial version and then
improve it.
