* [PATCH v14] ath10k: add LED and GPIO controlling support for various chipsets
@ 2023-06-11 8:05 Christian Marangi
2023-06-16 17:03 ` Kalle Valo
2024-05-15 8:59 ` Kalle Valo
0 siblings, 2 replies; 18+ messages in thread
From: Christian Marangi @ 2023-06-11 8:05 UTC (permalink / raw)
To: Kalle Valo, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, linux-kernel, ath10k, linux-wireless, netdev
Cc: Sebastian Gottschall, Steve deRosier, Kalle Valo,
Christian Marangi, Stefan Lippers-Hollmann
From: Sebastian Gottschall <s.gottschall@dd-wrt.com>
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@dd-wrt.com>
Reviewed-by: Steve deRosier <derosier@cal-sierra.com>
[kvalo: major reorg and cleanup]
Signed-off-by: Kalle Valo <kvalo@codeaurora.org>
[ansuel: rebase and small cleanup]
Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
Tested-by: Stefan Lippers-Hollmann <s.l-h@gmx.de>
---
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.
Changes v14:
- Fixed SPDX missing tag in leds.c and leds.h
- Fixes discrepancy between From: and Signed-off-by:
- Improve and rework kconfig description
- Improve commit description
- Add very old tested-by tag
(v13 changelog from Kavlo from 2018 [1])
* 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
(old changelog from really old patch v12 from 2018 [1])
v12 fix warning
v11 make register_gpio_chip static. advise by krobot
v10 compile fix if gpiolib isnt included
v9 move led and led code to separate sourcefile (gpio.c)
v8 fix various code design issues reported by reviewers
v7 fix ath10k_unregister_led for compiling without LED_CLASS
v6 correct return values and fix comment style
v5 fix compiling without LED_CLASS and GPIOLIB support, fix also error by kbuild test robot which does not occur in standard builds. curious
v2 add correct gpio count per chipset and remove IPQ4019 support since this chipset does not make use of specific gpios)
[1] https://patchwork.kernel.org/project/ath10k/patch/20180226084406.2093-1-s.gottschall@dd-wrt.com/
[2] https://patchwork.kernel.org/project/ath10k/patch/1523027875-5143-1-git-send-email-kvalo@codeaurora.org/
drivers/net/wireless/ath/ath10k/Kconfig | 17 +++++
drivers/net/wireless/ath/ath10k/Makefile | 1 +
drivers/net/wireless/ath/ath10k/core.c | 21 ++++++
drivers/net/wireless/ath/ath10k/core.h | 8 ++
drivers/net/wireless/ath/ath10k/hw.h | 1 +
drivers/net/wireless/ath/ath10k/leds.c | 92 +++++++++++++++++++++++
drivers/net/wireless/ath/ath10k/leds.h | 34 +++++++++
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, 298 insertions(+)
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 e6ea884cafc1..d8726090d70a 100644
--- 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.
+
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 142c777b287f..02bf9b629038 100644
--- a/drivers/net/wireless/ath/ath10k/Makefile
+++ b/drivers/net/wireless/ath/ath10k/Makefile
@@ -19,6 +19,7 @@ ath10k_core-$(CONFIG_ATH10K_SPECTRAL) += spectral.o
ath10k_core-$(CONFIG_NL80211_TESTMODE) += testmode.o
ath10k_core-$(CONFIG_ATH10K_TRACING) += trace.o
ath10k_core-$(CONFIG_THERMAL) += thermal.o
+ath10k_core-$(CONFIG_ATH10K_LEDS) += leds.o
ath10k_core-$(CONFIG_MAC80211_DEBUGFS) += debugfs_sta.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 6cdb225b7eac..a8dbaf0a74c4 100644
--- a/drivers/net/wireless/ath/ath10k/core.c
+++ b/drivers/net/wireless/ath/ath10k/core.c
@@ -26,6 +26,7 @@
#include "testmode.h"
#include "wmi-ops.h"
#include "coredump.h"
+#include "leds.h"
unsigned int ath10k_debug_mask;
EXPORT_SYMBOL(ath10k_debug_mask);
@@ -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,
@@ -146,6 +148,7 @@ static const struct ath10k_hw_params ath10k_hw_params_list[] = {
.dev_id = QCA9887_1_0_DEVICE_ID,
.bus = ATH10K_BUS_PCI,
.name = "qca9887 hw1.0",
+ .led_pin = 1,
.patch_load_addr = QCA9887_HW_1_0_PATCH_LOAD_ADDR,
.uart_pin = 7,
.cc_wraparound_type = ATH10K_HW_CC_WRAP_SHIFTED_ALL,
@@ -387,6 +390,7 @@ static const struct ath10k_hw_params ath10k_hw_params_list[] = {
.dev_id = QCA99X0_2_0_DEVICE_ID,
.bus = ATH10K_BUS_PCI,
.name = "qca99x0 hw2.0",
+ .led_pin = 17,
.patch_load_addr = QCA99X0_HW_2_0_PATCH_LOAD_ADDR,
.uart_pin = 7,
.otp_exe_param = 0x00000700,
@@ -433,6 +437,7 @@ static const struct ath10k_hw_params ath10k_hw_params_list[] = {
.dev_id = QCA9984_1_0_DEVICE_ID,
.bus = ATH10K_BUS_PCI,
.name = "qca9984/qca9994 hw1.0",
+ .led_pin = 17,
.patch_load_addr = QCA9984_HW_1_0_PATCH_LOAD_ADDR,
.uart_pin = 7,
.cc_wraparound_type = ATH10K_HW_CC_WRAP_SHIFTED_EACH,
@@ -486,6 +491,7 @@ static const struct ath10k_hw_params ath10k_hw_params_list[] = {
.dev_id = QCA9888_2_0_DEVICE_ID,
.bus = ATH10K_BUS_PCI,
.name = "qca9888 hw2.0",
+ .led_pin = 17,
.patch_load_addr = QCA9888_HW_2_0_PATCH_LOAD_ADDR,
.uart_pin = 7,
.cc_wraparound_type = ATH10K_HW_CC_WRAP_SHIFTED_EACH,
@@ -3222,6 +3228,10 @@ int ath10k_core_start(struct ath10k *ar, enum ath10k_firmware_mode mode,
goto err_hif_stop;
}
+ status = ath10k_leds_start(ar);
+ if (status)
+ goto err_hif_stop;
+
return 0;
err_hif_stop:
@@ -3480,9 +3490,18 @@ static void ath10k_core_register_work(struct work_struct *work)
goto err_spectral_destroy;
}
+ status = ath10k_leds_register(ar);
+ if (status) {
+ ath10k_err(ar, "could not register leds: %d\n",
+ status);
+ goto err_thermal_unregister;
+ }
+
set_bit(ATH10K_FLAG_CORE_REGISTERED, &ar->dev_flags);
return;
+err_thermal_unregister:
+ ath10k_thermal_unregister(ar);
err_spectral_destroy:
ath10k_spectral_destroy(ar);
err_debug_destroy:
@@ -3518,6 +3537,8 @@ void ath10k_core_unregister(struct ath10k *ar)
if (!test_bit(ATH10K_FLAG_CORE_REGISTERED, &ar->dev_flags))
return;
+ ath10k_leds_unregister(ar);
+
ath10k_thermal_unregister(ar);
/* Stop spectral before unregistering from mac80211 to remove the
* relayfs debugfs file cleanly. Otherwise the parent debugfs tree
diff --git a/drivers/net/wireless/ath/ath10k/core.h b/drivers/net/wireless/ath/ath10k/core.h
index 4b5239de4018..2a15fcdad0b9 100644
--- a/drivers/net/wireless/ath/ath10k/core.h
+++ b/drivers/net/wireless/ath/ath10k/core.h
@@ -14,6 +14,7 @@
#include <linux/pci.h>
#include <linux/uuid.h>
#include <linux/time.h>
+#include <linux/leds.h>
#include "htt.h"
#include "htc.h"
@@ -1255,6 +1256,13 @@ struct ath10k {
bool utf_monitor;
} testmode;
+ struct {
+ struct gpio_led wifi_led;
+ struct led_classdev cdev;
+ char label[48];
+ u32 gpio_state_pin;
+ } leds;
+
struct {
/* protected by data_lock */
u32 rx_crc_err_drop;
diff --git a/drivers/net/wireless/ath/ath10k/hw.h b/drivers/net/wireless/ath/ath10k/hw.h
index 9643031a4427..9d66293517b5 100644
--- a/drivers/net/wireless/ath/ath10k/hw.h
+++ b/drivers/net/wireless/ath/ath10k/hw.h
@@ -519,6 +519,7 @@ struct ath10k_hw_params {
const char *name;
u32 patch_load_addr;
int uart_pin;
+ int led_pin;
u32 otp_exe_param;
/* Type of hw cycle counter wraparound logic, for more info
diff --git a/drivers/net/wireless/ath/ath10k/leds.c b/drivers/net/wireless/ath/ath10k/leds.c
new file mode 100644
index 000000000000..8f5d66bb50b9
--- /dev/null
+++ b/drivers/net/wireless/ath/ath10k/leds.c
@@ -0,0 +1,92 @@
+// SPDX-License-Identifier: ISC
+/*
+ * Copyright (c) 2005-2011 Atheros Communications Inc.
+ * Copyright (c) 2011-2017 Qualcomm Atheros, Inc.
+ * Copyright (c) 2018 Sebastian Gottschall <s.gottschall@dd-wrt.com>
+ * Copyright (c) 2018-2023, The Linux Foundation. All rights reserved.
+ */
+
+#include <linux/leds.h>
+
+#include "core.h"
+#include "wmi.h"
+#include "wmi-ops.h"
+
+#include "leds.h"
+
+static int ath10k_leds_set_brightness_blocking(struct led_classdev *led_cdev,
+ enum led_brightness brightness)
+{
+ struct ath10k *ar = container_of(led_cdev, struct ath10k,
+ leds.cdev);
+ struct gpio_led *led = &ar->leds.wifi_led;
+
+ mutex_lock(&ar->conf_mutex);
+
+ if (ar->state != ATH10K_STATE_ON)
+ goto out;
+
+ ar->leds.gpio_state_pin = (brightness != LED_OFF) ^ led->active_low;
+ ath10k_wmi_gpio_output(ar, led->gpio, ar->leds.gpio_state_pin);
+
+out:
+ mutex_unlock(&ar->conf_mutex);
+
+ return 0;
+}
+
+int ath10k_leds_start(struct ath10k *ar)
+{
+ if (ar->hw_params.led_pin == 0)
+ /* leds not supported */
+ return 0;
+
+ /* under some circumstances, the gpio pin gets reconfigured
+ * to default state by the firmware, so we need to
+ * reconfigure it this behaviour has only ben seen on
+ * QCA9984 and QCA99XX devices so far
+ */
+ ath10k_wmi_gpio_config(ar, ar->hw_params.led_pin, 0,
+ WMI_GPIO_PULL_NONE, WMI_GPIO_INTTYPE_DISABLE);
+ ath10k_wmi_gpio_output(ar, ar->hw_params.led_pin, 1);
+
+ return 0;
+}
+
+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;
+
+ ret = led_classdev_register(wiphy_dev(ar->hw->wiphy), &ar->leds.cdev);
+ if (ret)
+ return ret;
+
+ return 0;
+}
+
+void ath10k_leds_unregister(struct ath10k *ar)
+{
+ if (ar->hw_params.led_pin == 0)
+ /* leds not supported */
+ return;
+
+ led_classdev_unregister(&ar->leds.cdev);
+}
+
diff --git a/drivers/net/wireless/ath/ath10k/leds.h b/drivers/net/wireless/ath/ath10k/leds.h
new file mode 100644
index 000000000000..1254e21e87de
--- /dev/null
+++ b/drivers/net/wireless/ath/ath10k/leds.h
@@ -0,0 +1,34 @@
+/* SPDX-License-Identifier: ISC */
+/*
+ * Copyright (c) 2005-2011 Atheros Communications Inc.
+ * Copyright (c) 2011-2017 Qualcomm Atheros, Inc.
+ * Copyright (c) 2018 Sebastian Gottschall <s.gottschall@dd-wrt.com>
+ * Copyright (c) 2018-2023, The Linux Foundation. All rights reserved.
+ */
+
+#ifndef _LEDS_H_
+#define _LEDS_H_
+
+#include "core.h"
+
+#ifdef CONFIG_ATH10K_LEDS
+void ath10k_leds_unregister(struct ath10k *ar);
+int ath10k_leds_start(struct ath10k *ar);
+int ath10k_leds_register(struct ath10k *ar);
+#else
+static inline void ath10k_leds_unregister(struct ath10k *ar)
+{
+}
+
+static inline int ath10k_leds_start(struct ath10k *ar)
+{
+ return 0;
+}
+
+static inline int ath10k_leds_register(struct ath10k *ar)
+{
+ return 0;
+}
+
+#endif
+#endif /* _LEDS_H_ */
diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
index 03e7bc5b6c0b..54649b24e690 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -24,6 +24,7 @@
#include "wmi-tlv.h"
#include "wmi-ops.h"
#include "wow.h"
+#include "leds.h"
/*********/
/* Rates */
diff --git a/drivers/net/wireless/ath/ath10k/wmi-ops.h b/drivers/net/wireless/ath/ath10k/wmi-ops.h
index aa57d807491c..f3f6b5954b27 100644
--- a/drivers/net/wireless/ath/ath10k/wmi-ops.h
+++ b/drivers/net/wireless/ath/ath10k/wmi-ops.h
@@ -226,7 +226,10 @@ struct wmi_ops {
const struct wmi_bb_timing_cfg_arg *arg);
struct sk_buff *(*gen_per_peer_per_tid_cfg)(struct ath10k *ar,
const struct wmi_per_peer_per_tid_cfg_arg *arg);
+ struct sk_buff *(*gen_gpio_config)(struct ath10k *ar, u32 gpio_num,
+ u32 input, u32 pull_type, u32 intr_mode);
+ struct sk_buff *(*gen_gpio_output)(struct ath10k *ar, u32 gpio_num, u32 set);
};
int ath10k_wmi_cmd_send(struct ath10k *ar, struct sk_buff *skb, u32 cmd_id);
@@ -1122,6 +1125,35 @@ ath10k_wmi_force_fw_hang(struct ath10k *ar,
return ath10k_wmi_cmd_send(ar, skb, ar->wmi.cmd->force_fw_hang_cmdid);
}
+static inline int ath10k_wmi_gpio_config(struct ath10k *ar, u32 gpio_num,
+ u32 input, u32 pull_type, u32 intr_mode)
+{
+ struct sk_buff *skb;
+
+ if (!ar->wmi.ops->gen_gpio_config)
+ return -EOPNOTSUPP;
+
+ skb = ar->wmi.ops->gen_gpio_config(ar, gpio_num, input, pull_type, intr_mode);
+ if (IS_ERR(skb))
+ return PTR_ERR(skb);
+
+ return ath10k_wmi_cmd_send(ar, skb, ar->wmi.cmd->gpio_config_cmdid);
+}
+
+static inline int ath10k_wmi_gpio_output(struct ath10k *ar, u32 gpio_num, u32 set)
+{
+ struct sk_buff *skb;
+
+ if (!ar->wmi.ops->gen_gpio_config)
+ return -EOPNOTSUPP;
+
+ skb = ar->wmi.ops->gen_gpio_output(ar, gpio_num, set);
+ if (IS_ERR(skb))
+ return PTR_ERR(skb);
+
+ return ath10k_wmi_cmd_send(ar, skb, ar->wmi.cmd->gpio_output_cmdid);
+}
+
static inline int
ath10k_wmi_dbglog_cfg(struct ath10k *ar, u64 module_enable, u32 log_level)
{
diff --git a/drivers/net/wireless/ath/ath10k/wmi-tlv.c b/drivers/net/wireless/ath/ath10k/wmi-tlv.c
index 6b6aa3c36744..8a3a3bde2446 100644
--- a/drivers/net/wireless/ath/ath10k/wmi-tlv.c
+++ b/drivers/net/wireless/ath/ath10k/wmi-tlv.c
@@ -4601,6 +4601,8 @@ static const struct wmi_ops wmi_tlv_ops = {
.gen_echo = ath10k_wmi_tlv_op_gen_echo,
.gen_vdev_spectral_conf = ath10k_wmi_tlv_op_gen_vdev_spectral_conf,
.gen_vdev_spectral_enable = ath10k_wmi_tlv_op_gen_vdev_spectral_enable,
+ /* .gen_gpio_config not implemented */
+ /* .gen_gpio_output not implemented */
};
static const struct wmi_peer_flags_map wmi_tlv_peer_flags_map = {
diff --git a/drivers/net/wireless/ath/ath10k/wmi.c b/drivers/net/wireless/ath/ath10k/wmi.c
index 05fa7d4c0e1a..25352fa6162d 100644
--- a/drivers/net/wireless/ath/ath10k/wmi.c
+++ b/drivers/net/wireless/ath/ath10k/wmi.c
@@ -7472,6 +7472,49 @@ ath10k_wmi_op_gen_peer_set_param(struct ath10k *ar, u32 vdev_id,
return skb;
}
+static struct sk_buff *ath10k_wmi_op_gen_gpio_config(struct ath10k *ar,
+ u32 gpio_num, u32 input,
+ u32 pull_type, u32 intr_mode)
+{
+ struct wmi_gpio_config_cmd *cmd;
+ struct sk_buff *skb;
+
+ skb = ath10k_wmi_alloc_skb(ar, sizeof(*cmd));
+ if (!skb)
+ return ERR_PTR(-ENOMEM);
+
+ cmd = (struct wmi_gpio_config_cmd *)skb->data;
+ cmd->pull_type = __cpu_to_le32(pull_type);
+ cmd->gpio_num = __cpu_to_le32(gpio_num);
+ cmd->input = __cpu_to_le32(input);
+ cmd->intr_mode = __cpu_to_le32(intr_mode);
+
+ ath10k_dbg(ar, ATH10K_DBG_WMI, "wmi gpio_config gpio_num 0x%08x input 0x%08x pull_type 0x%08x intr_mode 0x%08x\n",
+ gpio_num, input, pull_type, intr_mode);
+
+ return skb;
+}
+
+static struct sk_buff *ath10k_wmi_op_gen_gpio_output(struct ath10k *ar,
+ u32 gpio_num, u32 set)
+{
+ struct wmi_gpio_output_cmd *cmd;
+ struct sk_buff *skb;
+
+ skb = ath10k_wmi_alloc_skb(ar, sizeof(*cmd));
+ if (!skb)
+ return ERR_PTR(-ENOMEM);
+
+ cmd = (struct wmi_gpio_output_cmd *)skb->data;
+ cmd->gpio_num = __cpu_to_le32(gpio_num);
+ cmd->set = __cpu_to_le32(set);
+
+ ath10k_dbg(ar, ATH10K_DBG_WMI, "wmi gpio_output gpio_num 0x%08x set 0x%08x\n",
+ gpio_num, set);
+
+ return skb;
+}
+
static struct sk_buff *
ath10k_wmi_op_gen_set_psmode(struct ath10k *ar, u32 vdev_id,
enum wmi_sta_ps_mode psmode)
@@ -9138,6 +9181,9 @@ static const struct wmi_ops wmi_ops = {
.fw_stats_fill = ath10k_wmi_main_op_fw_stats_fill,
.get_vdev_subtype = ath10k_wmi_op_get_vdev_subtype,
.gen_echo = ath10k_wmi_op_gen_echo,
+ .gen_gpio_config = ath10k_wmi_op_gen_gpio_config,
+ .gen_gpio_output = ath10k_wmi_op_gen_gpio_output,
+
/* .gen_bcn_tmpl not implemented */
/* .gen_prb_tmpl not implemented */
/* .gen_p2p_go_bcn_ie not implemented */
@@ -9208,6 +9254,8 @@ static const struct wmi_ops wmi_10_1_ops = {
.fw_stats_fill = ath10k_wmi_10x_op_fw_stats_fill,
.get_vdev_subtype = ath10k_wmi_op_get_vdev_subtype,
.gen_echo = ath10k_wmi_op_gen_echo,
+ .gen_gpio_config = ath10k_wmi_op_gen_gpio_config,
+ .gen_gpio_output = ath10k_wmi_op_gen_gpio_output,
/* .gen_bcn_tmpl not implemented */
/* .gen_prb_tmpl not implemented */
/* .gen_p2p_go_bcn_ie not implemented */
@@ -9280,6 +9328,8 @@ static const struct wmi_ops wmi_10_2_ops = {
.gen_delba_send = ath10k_wmi_op_gen_delba_send,
.fw_stats_fill = ath10k_wmi_10x_op_fw_stats_fill,
.get_vdev_subtype = ath10k_wmi_op_get_vdev_subtype,
+ .gen_gpio_config = ath10k_wmi_op_gen_gpio_config,
+ .gen_gpio_output = ath10k_wmi_op_gen_gpio_output,
/* .gen_pdev_enable_adaptive_cca not implemented */
};
@@ -9351,6 +9401,8 @@ static const struct wmi_ops wmi_10_2_4_ops = {
ath10k_wmi_op_gen_pdev_enable_adaptive_cca,
.get_vdev_subtype = ath10k_wmi_10_2_4_op_get_vdev_subtype,
.gen_bb_timing = ath10k_wmi_10_2_4_op_gen_bb_timing,
+ .gen_gpio_config = ath10k_wmi_op_gen_gpio_config,
+ .gen_gpio_output = ath10k_wmi_op_gen_gpio_output,
/* .gen_bcn_tmpl not implemented */
/* .gen_prb_tmpl not implemented */
/* .gen_p2p_go_bcn_ie not implemented */
@@ -9432,6 +9484,8 @@ static const struct wmi_ops wmi_10_4_ops = {
.gen_pdev_bss_chan_info_req = ath10k_wmi_10_2_op_gen_pdev_bss_chan_info,
.gen_echo = ath10k_wmi_op_gen_echo,
.gen_pdev_get_tpc_config = ath10k_wmi_10_2_4_op_gen_pdev_get_tpc_config,
+ .gen_gpio_config = ath10k_wmi_op_gen_gpio_config,
+ .gen_gpio_output = ath10k_wmi_op_gen_gpio_output,
};
int ath10k_wmi_attach(struct ath10k *ar)
diff --git a/drivers/net/wireless/ath/ath10k/wmi.h b/drivers/net/wireless/ath/ath10k/wmi.h
index 6d04a66fe5e0..6442a76077dd 100644
--- a/drivers/net/wireless/ath/ath10k/wmi.h
+++ b/drivers/net/wireless/ath/ath10k/wmi.h
@@ -3030,6 +3030,41 @@ enum wmi_10_4_feature_mask {
};
+/* WMI_GPIO_CONFIG_CMDID */
+enum {
+ WMI_GPIO_PULL_NONE,
+ WMI_GPIO_PULL_UP,
+ WMI_GPIO_PULL_DOWN,
+};
+
+enum {
+ WMI_GPIO_INTTYPE_DISABLE,
+ WMI_GPIO_INTTYPE_RISING_EDGE,
+ WMI_GPIO_INTTYPE_FALLING_EDGE,
+ WMI_GPIO_INTTYPE_BOTH_EDGE,
+ WMI_GPIO_INTTYPE_LEVEL_LOW,
+ WMI_GPIO_INTTYPE_LEVEL_HIGH
+};
+
+/* WMI_GPIO_CONFIG_CMDID */
+struct wmi_gpio_config_cmd {
+ __le32 gpio_num; /* GPIO number to be setup */
+ __le32 input; /* 0 - Output/ 1 - Input */
+ __le32 pull_type; /* Pull type defined above */
+ __le32 intr_mode; /* Interrupt mode defined above (Input) */
+} __packed;
+
+/* WMI_GPIO_OUTPUT_CMDID */
+struct wmi_gpio_output_cmd {
+ __le32 gpio_num; /* GPIO number to be setup */
+ __le32 set; /* Set the GPIO pin*/
+} __packed;
+
+/* WMI_GPIO_INPUT_EVENTID */
+struct wmi_gpio_input_event {
+ __le32 gpio_num; /* GPIO number which changed state */
+} __packed;
+
struct wmi_ext_resource_config_10_4_cmd {
/* contains enum wmi_host_platform_type */
__le32 host_platform_config;
--
2.40.1
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v14] ath10k: add LED and GPIO controlling support for various chipsets
2023-06-16 17:03 ` Kalle Valo
@ 2023-06-16 11:35 ` Christian Marangi
2023-06-16 21:27 ` Christian Marangi
0 siblings, 1 reply; 18+ messages in thread
From: Christian Marangi @ 2023-06-16 11:35 UTC (permalink / raw)
To: Kalle Valo
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
linux-kernel, ath10k, linux-wireless, netdev,
Sebastian Gottschall, Steve deRosier, Stefan Lippers-Hollmann
On Fri, Jun 16, 2023 at 08:03:23PM +0300, Kalle Valo wrote:
> Christian Marangi <ansuelsmth@gmail.com> writes:
>
> > From: Sebastian Gottschall <s.gottschall@dd-wrt.com>
> >
> > 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@dd-wrt.com>
> > Reviewed-by: Steve deRosier <derosier@cal-sierra.com>
> > [kvalo: major reorg and cleanup]
> > Signed-off-by: Kalle Valo <kvalo@codeaurora.org>
> > [ansuel: rebase and small cleanup]
> > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> > Tested-by: Stefan Lippers-Hollmann <s.l-h@gmx.de>
> > ---
> >
> > 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.
--
Ansuel
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v14] ath10k: add LED and GPIO controlling support for various chipsets
2023-06-11 8:05 [PATCH v14] ath10k: add LED and GPIO controlling support for various chipsets Christian Marangi
@ 2023-06-16 17:03 ` Kalle Valo
2023-06-16 11:35 ` Christian Marangi
2024-05-15 8:59 ` Kalle Valo
1 sibling, 1 reply; 18+ messages in thread
From: Kalle Valo @ 2023-06-16 17:03 UTC (permalink / raw)
To: Christian Marangi
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
linux-kernel, ath10k, linux-wireless, netdev,
Sebastian Gottschall, Steve deRosier, Stefan Lippers-Hollmann
Christian Marangi <ansuelsmth@gmail.com> writes:
> From: Sebastian Gottschall <s.gottschall@dd-wrt.com>
>
> 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@dd-wrt.com>
> Reviewed-by: Steve deRosier <derosier@cal-sierra.com>
> [kvalo: major reorg and cleanup]
> Signed-off-by: Kalle Valo <kvalo@codeaurora.org>
> [ansuel: rebase and small cleanup]
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> Tested-by: Stefan Lippers-Hollmann <s.l-h@gmx.de>
> ---
>
> 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.
> --- 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
> @@ -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.
> +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?
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v14] ath10k: add LED and GPIO controlling support for various chipsets
2023-06-16 11:35 ` Christian Marangi
@ 2023-06-16 21:27 ` Christian Marangi
2023-08-21 10:46 ` Ansuel Smith
0 siblings, 1 reply; 18+ messages in thread
From: Christian Marangi @ 2023-06-16 21:27 UTC (permalink / raw)
To: Kalle Valo
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
linux-kernel, ath10k, linux-wireless, netdev,
Sebastian Gottschall, Steve deRosier, Stefan Lippers-Hollmann
[-- Attachment #1: Type: text/plain, Size: 6737 bytes --]
On Fri, Jun 16, 2023 at 01:35:04PM +0200, Christian Marangi wrote:
> On Fri, Jun 16, 2023 at 08:03:23PM +0300, Kalle Valo wrote:
> > Christian Marangi <ansuelsmth@gmail.com> writes:
> >
> > > From: Sebastian Gottschall <s.gottschall@dd-wrt.com>
> > >
> > > 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@dd-wrt.com>
> > > Reviewed-by: Steve deRosier <derosier@cal-sierra.com>
> > > [kvalo: major reorg and cleanup]
> > > Signed-off-by: Kalle Valo <kvalo@codeaurora.org>
> > > [ansuel: rebase and small cleanup]
> > > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> > > Tested-by: Stefan Lippers-Hollmann <s.l-h@gmx.de>
> > > ---
> > >
> > > 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.
If you want to check out I attached the 2 patch (one dt-bindings and the
one for the code) that I will submit when this will be merged (the
change is with the assumption that the FIXME line is dropped)
Tested and works correctly with my use case of wifi card attached with
pcie. This implementation permits to declare the default trigger in DT
instead of hardcoding.
--
Ansuel
[-- Attachment #2: 0001-dt-bindings-net-wireless-qcom-ath10k-Document-LED-no.patch --]
[-- Type: text/x-diff, Size: 2279 bytes --]
From d1e1a93fbfcbe711659f7ed7480633bf4d382377 Mon Sep 17 00:00:00 2001
From: Christian Marangi <ansuelsmth@gmail.com>
Date: Fri, 16 Jun 2023 22:58:20 +0200
Subject: [PATCH 1/3] dt-bindings: net: wireless: qcom: ath10k: Document LED
node support
Ath10k based wifi cards can support a LED connected via GPIO internally.
The LED is configured used WMI call. Document support for the LED node
controllable standard LED bindings.
While at it adds also an example for PCIe where LED is commonly
connected and used on routers.
Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
.../bindings/net/wireless/qcom,ath10k.yaml | 42 +++++++++++++++++++
1 file changed, 42 insertions(+)
diff --git a/Documentation/devicetree/bindings/net/wireless/qcom,ath10k.yaml b/Documentation/devicetree/bindings/net/wireless/qcom,ath10k.yaml
index c85ed330426d..7528ece8eff7 100644
--- a/Documentation/devicetree/bindings/net/wireless/qcom,ath10k.yaml
+++ b/Documentation/devicetree/bindings/net/wireless/qcom,ath10k.yaml
@@ -157,6 +157,11 @@ properties:
vdd-3.3-ch1-supply:
description: Secondary Wi-Fi antenna supply
+ led:
+ $ref: /schemas/leds/common.yaml#
+
+ unevaluatedProperties: false
+
required:
- compatible
- reg
@@ -258,6 +263,43 @@ allOf:
- interrupts
examples:
+ # PCIe
+ - |
+ #include <dt-bindings/leds/common.h>
+
+ pci@1b500000 {
+ reg = <0x1b500000 0x1000
+ 0x1b502000 0x80
+ 0x1b600000 0x100
+ 0x0ff00000 0x100000>;
+ device_type = "pci";
+ #address-cells = <3>;
+ #size-cells = <2>;
+
+ ranges = <0x81000000 0x0 0x00000000 0x0fe00000 0x0 0x00010000 /* I/O */
+ 0x82000000 0x0 0x08000000 0x08000000 0x0 0x07e00000>; /* MEM */
+
+ /* ... */
+
+ bridge@0,0 {
+ reg = <0x00000000 0 0 0 0>;
+ #address-cells = <3>;
+ #size-cells = <2>;
+ ranges;
+
+ wifi@1,0 {
+ compatible = "qcom,ath10k";
+ reg = <0x00010000 0 0 0 0>;
+
+ led {
+ default-state = "keep";
+ color = <LED_COLOR_ID_WHITE>;
+ function = LED_FUNCTION_WLAN;
+ };
+ };
+ };
+ };
+
# SNoC
- |
#include <dt-bindings/clock/qcom,rpmcc.h>
--
2.40.1
[-- Attachment #3: 0002-wifi-ath10k-start-LED-with-the-previous-defined-cdev.patch --]
[-- Type: text/x-diff, Size: 1663 bytes --]
From 1e867963acbc7dd744fb07949928bc493244ee81 Mon Sep 17 00:00:00 2001
From: Christian Marangi <ansuelsmth@gmail.com>
Date: Fri, 16 Jun 2023 23:22:28 +0200
Subject: [PATCH 2/3] wifi: ath10k: start LED with the previous defined cdev
brightness
In preparation for DT support for LED, change ath10k_leds_start to init
the LED to the previous defined cdev brightness.
The LED is set to OFF by default since we can't understand the previous
state of the LED due to a limitation in the WMI calls exposed by the fw.
Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
drivers/net/wireless/ath/ath10k/leds.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/net/wireless/ath/ath10k/leds.c b/drivers/net/wireless/ath/ath10k/leds.c
index 6146d4d69013..b3b3025b4a38 100644
--- a/drivers/net/wireless/ath/ath10k/leds.c
+++ b/drivers/net/wireless/ath/ath10k/leds.c
@@ -48,7 +48,8 @@ int ath10k_leds_start(struct ath10k *ar)
*/
ath10k_wmi_gpio_config(ar, ar->hw_params.led_pin, 0,
WMI_GPIO_PULL_NONE, WMI_GPIO_INTTYPE_DISABLE);
- ath10k_wmi_gpio_output(ar, ar->hw_params.led_pin, 1);
+ ath10k_wmi_gpio_output(ar, ar->hw_params.led_pin,
+ ar->leds.cdev.brightness ^ ar->leds.wifi_led.active_low);
return 0;
}
@@ -69,6 +70,8 @@ int ath10k_leds_register(struct ath10k *ar)
ar->leds.wifi_led.default_state = LEDS_GPIO_DEFSTATE_KEEP;
ar->leds.cdev.name = ar->leds.label;
+ /* By default LED is set OFF */
+ ar->leds.cdev.brightness = 0;
ar->leds.cdev.brightness_set_blocking = ath10k_leds_set_brightness_blocking;
ret = led_classdev_register(wiphy_dev(ar->hw->wiphy), &ar->leds.cdev);
--
2.40.1
[-- Attachment #4: 0003-wifi-ath10k-add-DT-support-for-LED-definition.patch --]
[-- Type: text/x-diff, Size: 2587 bytes --]
From 9437101f4ab7e06118be83180be6c8f471c1f804 Mon Sep 17 00:00:00 2001
From: Christian Marangi <ansuelsmth@gmail.com>
Date: Fri, 16 Jun 2023 23:02:26 +0200
Subject: [PATCH 3/3] wifi: ath10k: add DT support for LED definition
If supported, the LED definition for the ath10k wifi card is all
hardcoded with static names, triggers and default-state.
Add DT support for the supported LED to permit custom names, define a
default state and a default trigger.
To identify these special LED, devname_mandatory is set to true and each
LED is prefixed with ath10k- and the wireless phy name.
The non-DT implementation is still supported and DT definition is not
mandatory.
Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
drivers/net/wireless/ath/ath10k/leds.c | 38 +++++++++++++++++++++++++-
1 file changed, 37 insertions(+), 1 deletion(-)
diff --git a/drivers/net/wireless/ath/ath10k/leds.c b/drivers/net/wireless/ath/ath10k/leds.c
index b3b3025b4a38..f2f31eb26b8e 100644
--- a/drivers/net/wireless/ath/ath10k/leds.c
+++ b/drivers/net/wireless/ath/ath10k/leds.c
@@ -54,8 +54,39 @@ int ath10k_leds_start(struct ath10k *ar)
return 0;
}
+static int ath10k_leds_of_init(struct ath10k *ar, struct led_init_data *init_data)
+{
+ struct fwnode_handle *led = NULL;
+ enum led_default_state state;
+
+ led = device_get_named_child_node(ar->dev, "led");
+ if (!led)
+ return -ENOENT;
+
+ /* LED will be init on ath10k core start. */
+ state = led_init_default_state_get(led);
+ switch (state) {
+ case LEDS_DEFSTATE_ON:
+ ar->leds.cdev.brightness = 1;
+ break;
+ /* KEEP will start the LED to OFF by default */
+ case LEDS_DEFSTATE_KEEP:
+ default:
+ ar->leds.cdev.brightness = 0;
+ }
+
+ init_data->default_label = "wifi";
+ init_data->fwnode = led;
+ init_data->devname_mandatory = true;
+ init_data->devicename = ar->leds.label;
+
+ return 0;
+}
+
int ath10k_leds_register(struct ath10k *ar)
{
+ struct led_init_data *init_data_ptr = NULL;
+ struct led_init_data init_data = { };
int ret;
if (ar->hw_params.led_pin == 0)
@@ -74,7 +105,12 @@ int ath10k_leds_register(struct ath10k *ar)
ar->leds.cdev.brightness = 0;
ar->leds.cdev.brightness_set_blocking = ath10k_leds_set_brightness_blocking;
- ret = led_classdev_register(wiphy_dev(ar->hw->wiphy), &ar->leds.cdev);
+ /* Support DT defined led. init_data_ptr is NULL if DT is not supported. */
+ if (!ath10k_leds_of_init(ar, &init_data))
+ init_data_ptr = &init_data;
+
+ ret = led_classdev_register_ext(wiphy_dev(ar->hw->wiphy), &ar->leds.cdev,
+ init_data_ptr);
if (ret)
return ret;
--
2.40.1
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v14] ath10k: add LED and GPIO controlling support for various chipsets
2023-06-16 21:27 ` Christian Marangi
@ 2023-08-21 10:46 ` Ansuel Smith
2024-05-09 4:50 ` Kalle Valo
0 siblings, 1 reply; 18+ messages in thread
From: Ansuel Smith @ 2023-08-21 10:46 UTC (permalink / raw)
To: Kalle Valo
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
linux-kernel, ath10k, linux-wireless, netdev,
Sebastian Gottschall, Steve deRosier, Stefan Lippers-Hollmann
Il giorno sab 17 giu 2023 alle ore 19:28 Christian Marangi
<ansuelsmth@gmail.com> ha scritto:
>
> On Fri, Jun 16, 2023 at 01:35:04PM +0200, Christian Marangi wrote:
> > On Fri, Jun 16, 2023 at 08:03:23PM +0300, Kalle Valo wrote:
> > > Christian Marangi <ansuelsmth@gmail.com> writes:
> > >
> > > > From: Sebastian Gottschall <s.gottschall@dd-wrt.com>
> > > >
> > > > 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@dd-wrt.com>
> > > > Reviewed-by: Steve deRosier <derosier@cal-sierra.com>
> > > > [kvalo: major reorg and cleanup]
> > > > Signed-off-by: Kalle Valo <kvalo@codeaurora.org>
> > > > [ansuel: rebase and small cleanup]
> > > > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> > > > Tested-by: Stefan Lippers-Hollmann <s.l-h@gmx.de>
> > > > ---
> > > >
> > > > 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.
>
> If you want to check out I attached the 2 patch (one dt-bindings and the
> one for the code) that I will submit when this will be merged (the
> change is with the assumption that the FIXME line is dropped)
>
> Tested and works correctly with my use case of wifi card attached with
> pcie. This implementation permits to declare the default trigger in DT
> instead of hardcoding.
>
Any news with this? Did I notice the LEDs patch are still in pending...
Since I notice the process is a bit slow I wonder if we can also queue
the 2 patch i attached in the previous email so we can speed things up?
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v14] ath10k: add LED and GPIO controlling support for various chipsets
2023-08-21 10:46 ` Ansuel Smith
@ 2024-05-09 4:50 ` Kalle Valo
2024-05-09 10:04 ` Christian Marangi
2024-05-09 16:37 ` Jeff Johnson
0 siblings, 2 replies; 18+ messages in thread
From: Kalle Valo @ 2024-05-09 4:50 UTC (permalink / raw)
To: Ansuel Smith
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
linux-kernel, ath10k, linux-wireless, netdev,
Sebastian Gottschall, Steve deRosier, Stefan Lippers-Hollmann
Ansuel Smith <ansuelsmth@gmail.com> writes:
> Il giorno sab 17 giu 2023 alle ore 19:28 Christian Marangi
> <ansuelsmth@gmail.com> ha scritto:
>
>>
>> On Fri, Jun 16, 2023 at 01:35:04PM +0200, Christian Marangi wrote:
>> > On Fri, Jun 16, 2023 at 08:03:23PM +0300, Kalle Valo wrote:
>> > > Christian Marangi <ansuelsmth@gmail.com> writes:
>> > >
>> > > > From: Sebastian Gottschall <s.gottschall@dd-wrt.com>
>> > > >
>> > > > 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@dd-wrt.com>
>> > > > Reviewed-by: Steve deRosier <derosier@cal-sierra.com>
>> > > > [kvalo: major reorg and cleanup]
>> > > > Signed-off-by: Kalle Valo <kvalo@codeaurora.org>
>> > > > [ansuel: rebase and small cleanup]
>> > > > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
>> > > > Tested-by: Stefan Lippers-Hollmann <s.l-h@gmx.de>
>> > > > ---
>> > > >
>> > > > 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.
>>
>> If you want to check out I attached the 2 patch (one dt-bindings and the
>> one for the code) that I will submit when this will be merged (the
>> change is with the assumption that the FIXME line is dropped)
>>
>> Tested and works correctly with my use case of wifi card attached with
>> pcie. This implementation permits to declare the default trigger in DT
>> instead of hardcoding.
>>
>
> Any news with this? Did I notice the LEDs patch are still in pending...
Sorry for the delay but finally I looked at this again. I decided to
just remove the fixme and otherwise it looks good for me. Please check
my changes:
https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/commit/?h=pending&id=688130a66ed49f20ca0ce02c3987f6a474f7c93a
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v14] ath10k: add LED and GPIO controlling support for various chipsets
2024-05-09 4:50 ` Kalle Valo
@ 2024-05-09 10:04 ` Christian Marangi
2024-05-10 8:53 ` Sebastian Gottschall
2024-05-10 13:50 ` Kalle Valo
2024-05-09 16:37 ` Jeff Johnson
1 sibling, 2 replies; 18+ messages in thread
From: Christian Marangi @ 2024-05-09 10:04 UTC (permalink / raw)
To: Kalle Valo
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
linux-kernel, ath10k, linux-wireless, netdev,
Sebastian Gottschall, Steve deRosier, Stefan Lippers-Hollmann
On Thu, May 09, 2024 at 07:50:40AM +0300, Kalle Valo wrote:
> Ansuel Smith <ansuelsmth@gmail.com> writes:
>
> > Il giorno sab 17 giu 2023 alle ore 19:28 Christian Marangi
> > <ansuelsmth@gmail.com> ha scritto:
> >
> >>
> >> On Fri, Jun 16, 2023 at 01:35:04PM +0200, Christian Marangi wrote:
> >> > On Fri, Jun 16, 2023 at 08:03:23PM +0300, Kalle Valo wrote:
> >> > > Christian Marangi <ansuelsmth@gmail.com> writes:
> >> > >
> >> > > > From: Sebastian Gottschall <s.gottschall@dd-wrt.com>
> >> > > >
> >> > > > 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@dd-wrt.com>
> >> > > > Reviewed-by: Steve deRosier <derosier@cal-sierra.com>
> >> > > > [kvalo: major reorg and cleanup]
> >> > > > Signed-off-by: Kalle Valo <kvalo@codeaurora.org>
> >> > > > [ansuel: rebase and small cleanup]
> >> > > > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> >> > > > Tested-by: Stefan Lippers-Hollmann <s.l-h@gmx.de>
> >> > > > ---
> >> > > >
> >> > > > 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.
> >>
> >> If you want to check out I attached the 2 patch (one dt-bindings and the
> >> one for the code) that I will submit when this will be merged (the
> >> change is with the assumption that the FIXME line is dropped)
> >>
> >> Tested and works correctly with my use case of wifi card attached with
> >> pcie. This implementation permits to declare the default trigger in DT
> >> instead of hardcoding.
> >>
> >
> > Any news with this? Did I notice the LEDs patch are still in pending...
>
> Sorry for the delay but finally I looked at this again. I decided to
> just remove the fixme and otherwise it looks good for me. Please check
> my changes:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/commit/?h=pending&id=688130a66ed49f20ca0ce02c3987f6a474f7c93a
>
All ok for me, Just I notice the ATH10K_LEDS is not exposed anymore? Is
that intended?
Aside from this very happy that we are finally finishing with this long
lasting feature!
--
Ansuel
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v14] ath10k: add LED and GPIO controlling support for various chipsets
2024-05-09 4:50 ` Kalle Valo
2024-05-09 10:04 ` Christian Marangi
@ 2024-05-09 16:37 ` Jeff Johnson
2024-05-09 16:48 ` Jeff Johnson
1 sibling, 1 reply; 18+ messages in thread
From: Jeff Johnson @ 2024-05-09 16:37 UTC (permalink / raw)
To: Kalle Valo, Ansuel Smith
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
linux-kernel, ath10k, linux-wireless, netdev,
Sebastian Gottschall, Steve deRosier, Stefan Lippers-Hollmann
On 5/8/2024 9:50 PM, Kalle Valo wrote:
> Sorry for the delay but finally I looked at this again. I decided to
> just remove the fixme and otherwise it looks good for me. Please check
> my changes:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/commit/?h=pending&id=688130a66ed49f20ca0ce02c3987f6a474f7c93a
>
I have a question about the copyrights in the two new files:
+ * Copyright (c) 2018-2023, The Linux Foundation. All rights reserved.
My understanding is that Qualcomm's affiliation with Linux Foundation via Code
Aurora ended in December 2021, and hence any contributions in 2022-2023 should
be the copyright of Qualcomm Innovation Center, Inc.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v14] ath10k: add LED and GPIO controlling support for various chipsets
2024-05-09 16:37 ` Jeff Johnson
@ 2024-05-09 16:48 ` Jeff Johnson
2024-05-10 13:52 ` Kalle Valo
2024-05-10 14:14 ` Christian Marangi
0 siblings, 2 replies; 18+ messages in thread
From: Jeff Johnson @ 2024-05-09 16:48 UTC (permalink / raw)
To: Kalle Valo, Ansuel Smith
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
linux-kernel, ath10k, linux-wireless, netdev,
Sebastian Gottschall, Steve deRosier, Stefan Lippers-Hollmann
On 5/9/2024 9:37 AM, Jeff Johnson wrote:
> On 5/8/2024 9:50 PM, Kalle Valo wrote:
>> Sorry for the delay but finally I looked at this again. I decided to
>> just remove the fixme and otherwise it looks good for me. Please check
>> my changes:
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/commit/?h=pending&id=688130a66ed49f20ca0ce02c3987f6a474f7c93a
>>
>
> I have a question about the copyrights in the two new files:
> + * Copyright (c) 2018-2023, The Linux Foundation. All rights reserved.
>
> My understanding is that Qualcomm's affiliation with Linux Foundation via Code
> Aurora ended in December 2021, and hence any contributions in 2022-2023 should
> be the copyright of Qualcomm Innovation Center, Inc.
>
>
ok it seems like Kalle's v13 had:
+ * Copyright (c) 2018, The Linux Foundation. All rights reserved.
and Ansuel's v14 has:
+ * Copyright (c) 2018-2023, The Linux Foundation. All rights reserved.
So Ansuel, is your work on behalf of The Linux Foundation?
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v14] ath10k: add LED and GPIO controlling support for various chipsets
2024-05-09 10:04 ` Christian Marangi
@ 2024-05-10 8:53 ` Sebastian Gottschall
2024-05-10 13:50 ` Kalle Valo
1 sibling, 0 replies; 18+ messages in thread
From: Sebastian Gottschall @ 2024-05-10 8:53 UTC (permalink / raw)
To: Christian Marangi, Kalle Valo
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
linux-kernel, ath10k, linux-wireless, netdev, Steve deRosier,
Stefan Lippers-Hollmann
Am 09.05.2024 um 12:04 schrieb Christian Marangi:
> On Thu, May 09, 2024 at 07:50:40AM +0300, Kalle Valo wrote:
>> Ansuel Smith <ansuelsmth@gmail.com> writes:
>>
>>> Il giorno sab 17 giu 2023 alle ore 19:28 Christian Marangi
>>> <ansuelsmth@gmail.com> ha scritto:
>>>
>>>> On Fri, Jun 16, 2023 at 01:35:04PM +0200, Christian Marangi wrote:
>>>>> On Fri, Jun 16, 2023 at 08:03:23PM +0300, Kalle Valo wrote:
>>>>>> Christian Marangi <ansuelsmth@gmail.com> writes:
>>>>>>
>>>>>>> From: Sebastian Gottschall <s.gottschall@dd-wrt.com>
>>>>>>>
>>>>>>> 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@dd-wrt.com>
>>>>>>> Reviewed-by: Steve deRosier <derosier@cal-sierra.com>
>>>>>>> [kvalo: major reorg and cleanup]
>>>>>>> Signed-off-by: Kalle Valo <kvalo@codeaurora.org>
>>>>>>> [ansuel: rebase and small cleanup]
>>>>>>> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
>>>>>>> Tested-by: Stefan Lippers-Hollmann <s.l-h@gmx.de>
>>>>>>> ---
>>>>>>>
>>>>>>> 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.
>>>> If you want to check out I attached the 2 patch (one dt-bindings and the
>>>> one for the code) that I will submit when this will be merged (the
>>>> change is with the assumption that the FIXME line is dropped)
>>>>
>>>> Tested and works correctly with my use case of wifi card attached with
>>>> pcie. This implementation permits to declare the default trigger in DT
>>>> instead of hardcoding.
>>>>
>>> Any news with this? Did I notice the LEDs patch are still in pending...
>> Sorry for the delay but finally I looked at this again. I decided to
>> just remove the fixme and otherwise it looks good for me. Please check
>> my changes:
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/commit/?h=pending&id=688130a66ed49f20ca0ce02c3987f6a474f7c93a
>>
> All ok for me, Just I notice the ATH10K_LEDS is not exposed anymore? Is
> that intended?
>
> Aside from this very happy that we are finally finishing with this long
> lasting feature!
since ATH10K_LEDS is no exposed option anymore. how is this feature
enabled then? Its not selected by any dependency
Sebastian
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v14] ath10k: add LED and GPIO controlling support for various chipsets
2024-05-09 10:04 ` Christian Marangi
2024-05-10 8:53 ` Sebastian Gottschall
@ 2024-05-10 13:50 ` Kalle Valo
2024-05-10 14:09 ` Christian Marangi
1 sibling, 1 reply; 18+ messages in thread
From: Kalle Valo @ 2024-05-10 13:50 UTC (permalink / raw)
To: Christian Marangi
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
linux-kernel, ath10k, linux-wireless, netdev,
Sebastian Gottschall, Steve deRosier, Stefan Lippers-Hollmann
Christian Marangi <ansuelsmth@gmail.com> writes:
>>
>> Sorry for the delay but finally I looked at this again. I decided to
>> just remove the fixme and otherwise it looks good for me. Please check
>> my changes:
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/commit/?h=pending&id=688130a66ed49f20ca0ce02c3987f6a474f7c93a
>>
>
> All ok for me, Just I notice the ATH10K_LEDS is not exposed anymore? Is
> that intended?
Yes. It follows the same idea as other wireless drivers do, for example iwlwifi:
config IWLWIFI_LEDS
bool
depends on LEDS_CLASS=y || LEDS_CLASS=MAC80211
depends on IWLMVM || IWLDVM
select LEDS_TRIGGERS
select MAC80211_LEDS
default y
So what this patch now does:
config ATH10K_LEDS
bool
depends on ATH10K
depends on LEDS_CLASS=y || LEDS_CLASS=MAC80211
default y
The idea being that if LEDS_CLASS is enabled then ATH10K_LEDS is
automatically enabled. But please let us know if something is wrong
here.
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v14] ath10k: add LED and GPIO controlling support for various chipsets
2024-05-09 16:48 ` Jeff Johnson
@ 2024-05-10 13:52 ` Kalle Valo
2024-05-10 14:14 ` Christian Marangi
1 sibling, 0 replies; 18+ messages in thread
From: Kalle Valo @ 2024-05-10 13:52 UTC (permalink / raw)
To: Jeff Johnson
Cc: Ansuel Smith, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, linux-kernel, ath10k, linux-wireless, netdev,
Sebastian Gottschall, Steve deRosier, Stefan Lippers-Hollmann
Jeff Johnson <quic_jjohnson@quicinc.com> writes:
> On 5/9/2024 9:37 AM, Jeff Johnson wrote:
>> On 5/8/2024 9:50 PM, Kalle Valo wrote:
>>> Sorry for the delay but finally I looked at this again. I decided to
>>> just remove the fixme and otherwise it looks good for me. Please check
>>> my changes:
>>>
>>> https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/commit/?h=pending&id=688130a66ed49f20ca0ce02c3987f6a474f7c93a
>>>
>>
>> I have a question about the copyrights in the two new files:
>> + * Copyright (c) 2018-2023, The Linux Foundation. All rights reserved.
>>
>> My understanding is that Qualcomm's affiliation with Linux Foundation via Code
>> Aurora ended in December 2021, and hence any contributions in 2022-2023 should
>> be the copyright of Qualcomm Innovation Center, Inc.
>>
>>
>
> ok it seems like Kalle's v13 had:
> + * Copyright (c) 2018, The Linux Foundation. All rights reserved.
>
> and Ansuel's v14 has:
> + * Copyright (c) 2018-2023, The Linux Foundation. All rights reserved.
>
> So Ansuel, is your work on behalf of The Linux Foundation?
BTW in the pending branch I can change the copyright back to original so
no need to resend because of this. But I'll need guidance from Ansuel.
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v14] ath10k: add LED and GPIO controlling support for various chipsets
2024-05-10 13:50 ` Kalle Valo
@ 2024-05-10 14:09 ` Christian Marangi
0 siblings, 0 replies; 18+ messages in thread
From: Christian Marangi @ 2024-05-10 14:09 UTC (permalink / raw)
To: Kalle Valo
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
linux-kernel, ath10k, linux-wireless, netdev,
Sebastian Gottschall, Steve deRosier, Stefan Lippers-Hollmann
On Fri, May 10, 2024 at 04:50:52PM +0300, Kalle Valo wrote:
> Christian Marangi <ansuelsmth@gmail.com> writes:
>
> >>
> >> Sorry for the delay but finally I looked at this again. I decided to
> >> just remove the fixme and otherwise it looks good for me. Please check
> >> my changes:
> >>
> >> https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/commit/?h=pending&id=688130a66ed49f20ca0ce02c3987f6a474f7c93a
> >>
> >
> > All ok for me, Just I notice the ATH10K_LEDS is not exposed anymore? Is
> > that intended?
>
> Yes. It follows the same idea as other wireless drivers do, for example iwlwifi:
>
> config IWLWIFI_LEDS
> bool
> depends on LEDS_CLASS=y || LEDS_CLASS=MAC80211
> depends on IWLMVM || IWLDVM
> select LEDS_TRIGGERS
> select MAC80211_LEDS
> default y
>
> So what this patch now does:
>
> config ATH10K_LEDS
> bool
> depends on ATH10K
> depends on LEDS_CLASS=y || LEDS_CLASS=MAC80211
> default y
>
> The idea being that if LEDS_CLASS is enabled then ATH10K_LEDS is
> automatically enabled. But please let us know if something is wrong
> here.
>
Sure, was just asking some clarification about it. Not a problem for me
since we are using other pattern to handle this.
--
Ansuel
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v14] ath10k: add LED and GPIO controlling support for various chipsets
2024-05-09 16:48 ` Jeff Johnson
2024-05-10 13:52 ` Kalle Valo
@ 2024-05-10 14:14 ` Christian Marangi
2024-05-10 14:54 ` Jeff Johnson
1 sibling, 1 reply; 18+ messages in thread
From: Christian Marangi @ 2024-05-10 14:14 UTC (permalink / raw)
To: Jeff Johnson
Cc: Kalle Valo, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, linux-kernel, ath10k, linux-wireless, netdev,
Sebastian Gottschall, Steve deRosier, Stefan Lippers-Hollmann
On Thu, May 09, 2024 at 09:48:08AM -0700, Jeff Johnson wrote:
> On 5/9/2024 9:37 AM, Jeff Johnson wrote:
> > On 5/8/2024 9:50 PM, Kalle Valo wrote:
> >> Sorry for the delay but finally I looked at this again. I decided to
> >> just remove the fixme and otherwise it looks good for me. Please check
> >> my changes:
> >>
> >> https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/commit/?h=pending&id=688130a66ed49f20ca0ce02c3987f6a474f7c93a
> >>
> >
> > I have a question about the copyrights in the two new files:
> > + * Copyright (c) 2018-2023, The Linux Foundation. All rights reserved.
> >
> > My understanding is that Qualcomm's affiliation with Linux Foundation via Code
> > Aurora ended in December 2021, and hence any contributions in 2022-2023 should
> > be the copyright of Qualcomm Innovation Center, Inc.
> >
> >
>
> ok it seems like Kalle's v13 had:
> + * Copyright (c) 2018, The Linux Foundation. All rights reserved.
>
> and Ansuel's v14 has:
> + * Copyright (c) 2018-2023, The Linux Foundation. All rights reserved.
>
> So Ansuel, is your work on behalf of The Linux Foundation?
>
When I resubmitted this at times, I just updated the copyright to the
current year so I guess it was wrong doing that?
As you can see from the copyright header this patch went all around and
I think at the end (around 2018) the Linux copyright was added as it was
submitted upstream. (can't remember if maintainers were asking that)
So me watching the old year and resubmitting it, just updated the date.
Soo I think we should revert to 2018?
--
Ansuel
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v14] ath10k: add LED and GPIO controlling support for various chipsets
2024-05-10 14:14 ` Christian Marangi
@ 2024-05-10 14:54 ` Jeff Johnson
2024-05-11 14:17 ` Kalle Valo
0 siblings, 1 reply; 18+ messages in thread
From: Jeff Johnson @ 2024-05-10 14:54 UTC (permalink / raw)
To: Christian Marangi
Cc: Kalle Valo, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, linux-kernel, ath10k, linux-wireless, netdev,
Sebastian Gottschall, Steve deRosier, Stefan Lippers-Hollmann
On 5/10/2024 7:14 AM, Christian Marangi wrote:
> On Thu, May 09, 2024 at 09:48:08AM -0700, Jeff Johnson wrote:
>> On 5/9/2024 9:37 AM, Jeff Johnson wrote:
>>> On 5/8/2024 9:50 PM, Kalle Valo wrote:
>>>> Sorry for the delay but finally I looked at this again. I decided to
>>>> just remove the fixme and otherwise it looks good for me. Please check
>>>> my changes:
>>>>
>>>> https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/commit/?h=pending&id=688130a66ed49f20ca0ce02c3987f6a474f7c93a
>>>>
>>>
>>> I have a question about the copyrights in the two new files:
>>> + * Copyright (c) 2018-2023, The Linux Foundation. All rights reserved.
>>>
>>> My understanding is that Qualcomm's affiliation with Linux Foundation via Code
>>> Aurora ended in December 2021, and hence any contributions in 2022-2023 should
>>> be the copyright of Qualcomm Innovation Center, Inc.
>>>
>>>
>>
>> ok it seems like Kalle's v13 had:
>> + * Copyright (c) 2018, The Linux Foundation. All rights reserved.
>>
>> and Ansuel's v14 has:
>> + * Copyright (c) 2018-2023, The Linux Foundation. All rights reserved.
>>
>> So Ansuel, is your work on behalf of The Linux Foundation?
>>
>
> When I resubmitted this at times, I just updated the copyright to the
> current year so I guess it was wrong doing that?
>
> As you can see from the copyright header this patch went all around and
> I think at the end (around 2018) the Linux copyright was added as it was
> submitted upstream. (can't remember if maintainers were asking that)
>
> So me watching the old year and resubmitting it, just updated the date.
>
> Soo I think we should revert to 2018?
>
Yes, in this case changing the Linux Foundation copyright back to 2018 is correct.
/jeff
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v14] ath10k: add LED and GPIO controlling support for various chipsets
2024-05-10 14:54 ` Jeff Johnson
@ 2024-05-11 14:17 ` Kalle Valo
2024-05-13 15:08 ` Jeff Johnson
0 siblings, 1 reply; 18+ messages in thread
From: Kalle Valo @ 2024-05-11 14:17 UTC (permalink / raw)
To: Jeff Johnson
Cc: Christian Marangi, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, linux-kernel, ath10k, linux-wireless, netdev,
Sebastian Gottschall, Steve deRosier, Stefan Lippers-Hollmann
Jeff Johnson <quic_jjohnson@quicinc.com> writes:
> On 5/10/2024 7:14 AM, Christian Marangi wrote:
>
>> On Thu, May 09, 2024 at 09:48:08AM -0700, Jeff Johnson wrote:
>>> On 5/9/2024 9:37 AM, Jeff Johnson wrote:
>>>> On 5/8/2024 9:50 PM, Kalle Valo wrote:
>>>>> Sorry for the delay but finally I looked at this again. I decided to
>>>>> just remove the fixme and otherwise it looks good for me. Please check
>>>>> my changes:
>>>>>
>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/commit/?h=pending&id=688130a66ed49f20ca0ce02c3987f6a474f7c93a
>>>>>
>>>>
>>>> I have a question about the copyrights in the two new files:
>>>> + * Copyright (c) 2018-2023, The Linux Foundation. All rights reserved.
>>>>
>>>> My understanding is that Qualcomm's affiliation with Linux Foundation via Code
>>>> Aurora ended in December 2021, and hence any contributions in 2022-2023 should
>>>> be the copyright of Qualcomm Innovation Center, Inc.
>>>>
>>>>
>>>
>>> ok it seems like Kalle's v13 had:
>>> + * Copyright (c) 2018, The Linux Foundation. All rights reserved.
>>>
>>> and Ansuel's v14 has:
>>> + * Copyright (c) 2018-2023, The Linux Foundation. All rights reserved.
>>>
>>> So Ansuel, is your work on behalf of The Linux Foundation?
>>>
>>
>> When I resubmitted this at times, I just updated the copyright to the
>> current year so I guess it was wrong doing that?
>>
>> As you can see from the copyright header this patch went all around and
>> I think at the end (around 2018) the Linux copyright was added as it was
>> submitted upstream. (can't remember if maintainers were asking that)
>>
>> So me watching the old year and resubmitting it, just updated the date.
>>
>> Soo I think we should revert to 2018?
>>
>
> Yes, in this case changing the Linux Foundation copyright back to 2018 is correct.
I changed it now back to 2018, please check:
https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/commit/?h=pending&id=5eff06bef76b6d4e1553c2d4978025c329d8db35
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v14] ath10k: add LED and GPIO controlling support for various chipsets
2024-05-11 14:17 ` Kalle Valo
@ 2024-05-13 15:08 ` Jeff Johnson
0 siblings, 0 replies; 18+ messages in thread
From: Jeff Johnson @ 2024-05-13 15:08 UTC (permalink / raw)
To: Kalle Valo
Cc: Christian Marangi, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, linux-kernel, ath10k, linux-wireless, netdev,
Sebastian Gottschall, Steve deRosier, Stefan Lippers-Hollmann
On 5/11/2024 7:17 AM, Kalle Valo wrote:
> Jeff Johnson <quic_jjohnson@quicinc.com> writes:
>
>> On 5/10/2024 7:14 AM, Christian Marangi wrote:
>>
>>> On Thu, May 09, 2024 at 09:48:08AM -0700, Jeff Johnson wrote:
>>>> On 5/9/2024 9:37 AM, Jeff Johnson wrote:
>>>>> On 5/8/2024 9:50 PM, Kalle Valo wrote:
>>>>>> Sorry for the delay but finally I looked at this again. I decided to
>>>>>> just remove the fixme and otherwise it looks good for me. Please check
>>>>>> my changes:
>>>>>>
>>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/commit/?h=pending&id=688130a66ed49f20ca0ce02c3987f6a474f7c93a
>>>>>>
>>>>>
>>>>> I have a question about the copyrights in the two new files:
>>>>> + * Copyright (c) 2018-2023, The Linux Foundation. All rights reserved.
>>>>>
>>>>> My understanding is that Qualcomm's affiliation with Linux Foundation via Code
>>>>> Aurora ended in December 2021, and hence any contributions in 2022-2023 should
>>>>> be the copyright of Qualcomm Innovation Center, Inc.
>>>>>
>>>>>
>>>>
>>>> ok it seems like Kalle's v13 had:
>>>> + * Copyright (c) 2018, The Linux Foundation. All rights reserved.
>>>>
>>>> and Ansuel's v14 has:
>>>> + * Copyright (c) 2018-2023, The Linux Foundation. All rights reserved.
>>>>
>>>> So Ansuel, is your work on behalf of The Linux Foundation?
>>>>
>>>
>>> When I resubmitted this at times, I just updated the copyright to the
>>> current year so I guess it was wrong doing that?
>>>
>>> As you can see from the copyright header this patch went all around and
>>> I think at the end (around 2018) the Linux copyright was added as it was
>>> submitted upstream. (can't remember if maintainers were asking that)
>>>
>>> So me watching the old year and resubmitting it, just updated the date.
>>>
>>> Soo I think we should revert to 2018?
>>>
>>
>> Yes, in this case changing the Linux Foundation copyright back to 2018 is correct.
>
> I changed it now back to 2018, please check:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/commit/?h=pending&id=5eff06bef76b6d4e1553c2d4978025c329d8db35
>
LGTM, thanks!
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v14] ath10k: add LED and GPIO controlling support for various chipsets
2023-06-11 8:05 [PATCH v14] ath10k: add LED and GPIO controlling support for various chipsets Christian Marangi
2023-06-16 17:03 ` Kalle Valo
@ 2024-05-15 8:59 ` Kalle Valo
1 sibling, 0 replies; 18+ messages in thread
From: Kalle Valo @ 2024-05-15 8:59 UTC (permalink / raw)
To: Christian Marangi
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
linux-kernel, ath10k, linux-wireless, netdev,
Sebastian Gottschall, Steve deRosier, Kalle Valo,
Christian Marangi, Stefan Lippers-Hollmann
Christian Marangi <ansuelsmth@gmail.com> wrote:
> 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@dd-wrt.com>
> Reviewed-by: Steve deRosier <derosier@cal-sierra.com>
> [kvalo: major reorg and cleanup]
> Signed-off-by: Kalle Valo <kvalo@codeaurora.org>
> [ansuel: rebase and small cleanup]
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> Tested-by: Stefan Lippers-Hollmann <s.l-h@gmx.de>
> Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com>
Patch applied to ath-next branch of ath.git, thanks.
8e1debd82466 wifi: ath10k: add LED and GPIO controlling support for various chipsets
--
https://patchwork.kernel.org/project/linux-wireless/patch/20230611080505.17393-1-ansuelsmth@gmail.com/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2024-05-15 8:59 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-06-11 8:05 [PATCH v14] ath10k: add LED and GPIO controlling support for various chipsets Christian Marangi
2023-06-16 17:03 ` Kalle Valo
2023-06-16 11:35 ` Christian Marangi
2023-06-16 21:27 ` Christian Marangi
2023-08-21 10:46 ` Ansuel Smith
2024-05-09 4:50 ` Kalle Valo
2024-05-09 10:04 ` Christian Marangi
2024-05-10 8:53 ` Sebastian Gottschall
2024-05-10 13:50 ` Kalle Valo
2024-05-10 14:09 ` Christian Marangi
2024-05-09 16:37 ` Jeff Johnson
2024-05-09 16:48 ` Jeff Johnson
2024-05-10 13:52 ` Kalle Valo
2024-05-10 14:14 ` Christian Marangi
2024-05-10 14:54 ` Jeff Johnson
2024-05-11 14:17 ` Kalle Valo
2024-05-13 15:08 ` Jeff Johnson
2024-05-15 8:59 ` Kalle Valo
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).