Re: [PATCH v9] ath10k: add LED and GPIO controlling support for various chipsets
Hi Sebastian, Thank you for the patch! Perhaps something to improve: [auto build test WARNING on wireless-drivers-next/master] [also build test WARNING on v4.16-rc2 next-20180223] [cannot apply to ath6kl/ath-next] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/s-gottschall-dd-wrt-com/ath10k-add-LED-and-GPIO-controlling-support-for-various-chipsets/20180224-185406 base: https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/wireless-drivers-next.git master reproduce: # apt-get install sparse make ARCH=x86_64 allmodconfig make C=1 CF=-D__CHECK_ENDIAN__ sparse warnings: (new ones prefixed by >>) >> drivers/net/wireless/ath/ath10k/gpio.c:84:5: sparse: symbol >> 'ath10k_register_gpio_chip' was not declared. Should it be Please review and possibly fold the followup patch. --- 0-DAY kernel test infrastructureOpen Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation
Re: [PATCH v9] ath10k: add LED and GPIO controlling support for various chipsets
Hi Sebastian, On Thu, Feb 22, 2018 at 6:09 PM, Sebastian Gottschall wrote: > Am 22.02.2018 um 23:17 schrieb Steve deRosier: >> >> On Thu, Feb 22, 2018 at 3:15 AM, wrote: >>> >>> From: Sebastian Gottschall >>> >>> Adds LED and GPIO Control support for 988x, 9887, 9888, 99x0, 9984 based >>> chipsets with on chipset connected led's >>> using WMI Firmware API. >>> The LED device will get available named as "ath10k-phyX" at sysfs and can >>> be controlled with various triggers. >>> adds also debugfs interface for gpio control. >>> >>> Signed-off-by: Sebastian Gottschall >>> >>> v2 add correct gpio count per chipset and remove IPQ4019 support since >>> this chipset does not make use of specific gpios) >>> v5 fix compiling without LED_CLASS and GPIOLIB support, fix also error >>> by kbuild test robot which does not occur in standard builds. curious >>> v6 correct return values and fix comment style >>> v7 fix ath10k_unregister_led for compiling without LED_CLASS >>> v8 fix various code design issues reported by reviewers >>> v9 move led and led code to separate sourcefile (gpio.c) >>> --- >>> drivers/net/wireless/ath/ath10k/Kconfig | 3 + >>> drivers/net/wireless/ath/ath10k/Makefile | 1 + >>> drivers/net/wireless/ath/ath10k/core.c| 28 - >>> drivers/net/wireless/ath/ath10k/core.h| 33 - >>> drivers/net/wireless/ath/ath10k/debug.c | 142 ++ >>> drivers/net/wireless/ath/ath10k/gpio.c| 196 >>> ++ >>> drivers/net/wireless/ath/ath10k/hw.h | 2 + >>> drivers/net/wireless/ath/ath10k/mac.c | 5 + >>> drivers/net/wireless/ath/ath10k/wmi-ops.h | 36 +- >>> drivers/net/wireless/ath/ath10k/wmi-tlv.c | 65 ++ >>> drivers/net/wireless/ath/ath10k/wmi.c | 46 +++ >>> drivers/net/wireless/ath/ath10k/wmi.h | 36 ++ >>> 12 files changed, 590 insertions(+), 3 deletions(-) >>> create mode 100644 drivers/net/wireless/ath/ath10k/gpio.c >>> >>> diff --git a/drivers/net/wireless/ath/ath10k/Kconfig >>> b/drivers/net/wireless/ath/ath10k/Kconfig >>> index deb5ae21a559..c46b7658a820 100644 >>> --- a/drivers/net/wireless/ath/ath10k/Kconfig >>> +++ b/drivers/net/wireless/ath/ath10k/Kconfig >>> @@ -2,6 +2,9 @@ config ATH10K >>> tristate "Atheros 802.11ac wireless cards support" >>> depends on MAC80211 && HAS_DMA >>> select ATH_COMMON >>> +select MAC80211_LEDS >>> +select LEDS_CLASS >>> +select NEW_LEDS >>> select CRC32 >>> select WANT_DEV_COREDUMP >>> ---help--- >> >> Pure question: do we require LEDS, or is this an option? I assumed all >> along it was an optional thing - ie if GPIOs and/or LEDs were >> configured, we added this code. And if not optional, then what happens >> if we are on a platform that can't support LEDS_CLASS or NEW_LEDS, >> assuming there is such a thing? > > i took this behaviour from ath9k/ath5k, both driver auto select > MAC80211_LEDS and LEDS_CLASS (by the way MAC80211_LEDS auto selects > LEDS_CLASS too). only chipsets which are known to have preconfigured leds > are used > and led will only turn on if you enable it in sysfs by assigning a trigger > to the led. > for sure this can be removed from the Kconfig. it will compile without it as > well. so this is optional. i just added it since all other drivers behave in > the same way. > in addition. LEDS_CLASS / NEW_LEDS is a kernel api, not a driver. so all > platforms to support LEDS_CLASS. just enabling LEDS_CLASS will not cause any > platform specific dependency. >> >> >> >>> diff --git a/drivers/net/wireless/ath/ath10k/Makefile >>> b/drivers/net/wireless/ath/ath10k/Makefile >>> index 6739ac26fd29..bcb234f0b940 100644 >>> --- a/drivers/net/wireless/ath/ath10k/Makefile >>> +++ b/drivers/net/wireless/ath/ath10k/Makefile >>> @@ -20,6 +20,7 @@ ath10k_core-$(CONFIG_NL80211_TESTMODE) += testmode.o >>> ath10k_core-$(CONFIG_ATH10K_TRACING) += trace.o >>> ath10k_core-$(CONFIG_THERMAL) += thermal.o >>> ath10k_core-$(CONFIG_MAC80211_DEBUGFS) += debugfs_sta.o >>> +ath10k_core-y += gpio.o >>> ath10k_core-$(CONFIG_PM) += wow.o >>> ath10k_core-$(CONFIG_DEV_COREDUMP) += coredump.o >>> >> I'd expect it would be something like `ath10k_core-$(CONFIG_GPIO) += >> gpoi.o`. Maybe I'm mistaken. > > gpio.c contains the MAC80211_LEDS code and has a guard for GPIOLIB inside. > it can be compiled even if GPIOLIB is disabled in system. (which is indeed > platform specific) > and leds are still supported without gpio support since it does contain a > led only driver and a separate gpio driver. > i suggest more somelike like > > ath10k_core-$(ATH10K_LEDS) += gpio.o > > if you want it totally optional which requires some other small changes as > well (mainly renaming some ifdefs) > >> (Note, I didn't look up the actual config option name, I'm guessing. >> Assume I wrote something reasonable for my example.) >> >> I only ask the above two questions, not bec
Re: [PATCH v9] ath10k: add LED and GPIO controlling support for various chipsets
Am 22.02.2018 um 23:17 schrieb Steve deRosier: On Thu, Feb 22, 2018 at 3:15 AM, wrote: From: Sebastian Gottschall Adds LED and GPIO Control support for 988x, 9887, 9888, 99x0, 9984 based chipsets with on chipset connected led's using WMI Firmware API. The LED device will get available named as "ath10k-phyX" at sysfs and can be controlled with various triggers. adds also debugfs interface for gpio control. Signed-off-by: Sebastian Gottschall v2 add correct gpio count per chipset and remove IPQ4019 support since this chipset does not make use of specific gpios) v5 fix compiling without LED_CLASS and GPIOLIB support, fix also error by kbuild test robot which does not occur in standard builds. curious v6 correct return values and fix comment style v7 fix ath10k_unregister_led for compiling without LED_CLASS v8 fix various code design issues reported by reviewers v9 move led and led code to separate sourcefile (gpio.c) --- drivers/net/wireless/ath/ath10k/Kconfig | 3 + drivers/net/wireless/ath/ath10k/Makefile | 1 + drivers/net/wireless/ath/ath10k/core.c| 28 - drivers/net/wireless/ath/ath10k/core.h| 33 - drivers/net/wireless/ath/ath10k/debug.c | 142 ++ drivers/net/wireless/ath/ath10k/gpio.c| 196 ++ drivers/net/wireless/ath/ath10k/hw.h | 2 + drivers/net/wireless/ath/ath10k/mac.c | 5 + drivers/net/wireless/ath/ath10k/wmi-ops.h | 36 +- drivers/net/wireless/ath/ath10k/wmi-tlv.c | 65 ++ drivers/net/wireless/ath/ath10k/wmi.c | 46 +++ drivers/net/wireless/ath/ath10k/wmi.h | 36 ++ 12 files changed, 590 insertions(+), 3 deletions(-) create mode 100644 drivers/net/wireless/ath/ath10k/gpio.c diff --git a/drivers/net/wireless/ath/ath10k/Kconfig b/drivers/net/wireless/ath/ath10k/Kconfig index deb5ae21a559..c46b7658a820 100644 --- a/drivers/net/wireless/ath/ath10k/Kconfig +++ b/drivers/net/wireless/ath/ath10k/Kconfig @@ -2,6 +2,9 @@ config ATH10K tristate "Atheros 802.11ac wireless cards support" depends on MAC80211 && HAS_DMA select ATH_COMMON +select MAC80211_LEDS +select LEDS_CLASS +select NEW_LEDS select CRC32 select WANT_DEV_COREDUMP ---help--- Pure question: do we require LEDS, or is this an option? I assumed all along it was an optional thing - ie if GPIOs and/or LEDs were configured, we added this code. And if not optional, then what happens if we are on a platform that can't support LEDS_CLASS or NEW_LEDS, assuming there is such a thing? i took this behaviour from ath9k/ath5k, both driver auto select MAC80211_LEDS and LEDS_CLASS (by the way MAC80211_LEDS auto selects LEDS_CLASS too). only chipsets which are known to have preconfigured leds are used and led will only turn on if you enable it in sysfs by assigning a trigger to the led. for sure this can be removed from the Kconfig. it will compile without it as well. so this is optional. i just added it since all other drivers behave in the same way. in addition. LEDS_CLASS / NEW_LEDS is a kernel api, not a driver. so all platforms to support LEDS_CLASS. just enabling LEDS_CLASS will not cause any platform specific dependency. diff --git a/drivers/net/wireless/ath/ath10k/Makefile b/drivers/net/wireless/ath/ath10k/Makefile index 6739ac26fd29..bcb234f0b940 100644 --- a/drivers/net/wireless/ath/ath10k/Makefile +++ b/drivers/net/wireless/ath/ath10k/Makefile @@ -20,6 +20,7 @@ ath10k_core-$(CONFIG_NL80211_TESTMODE) += testmode.o ath10k_core-$(CONFIG_ATH10K_TRACING) += trace.o ath10k_core-$(CONFIG_THERMAL) += thermal.o ath10k_core-$(CONFIG_MAC80211_DEBUGFS) += debugfs_sta.o +ath10k_core-y += gpio.o ath10k_core-$(CONFIG_PM) += wow.o ath10k_core-$(CONFIG_DEV_COREDUMP) += coredump.o I'd expect it would be something like `ath10k_core-$(CONFIG_GPIO) += gpoi.o`. Maybe I'm mistaken. gpio.c contains the MAC80211_LEDS code and has a guard for GPIOLIB inside. it can be compiled even if GPIOLIB is disabled in system. (which is indeed platform specific) and leds are still supported without gpio support since it does contain a led only driver and a separate gpio driver. i suggest more somelike like ath10k_core-$(ATH10K_LEDS) += gpio.o if you want it totally optional which requires some other small changes as well (mainly renaming some ifdefs) (Note, I didn't look up the actual config option name, I'm guessing. Assume I wrote something reasonable for my example.) I only ask the above two questions, not because I think it's wrong, but because I don't quite know the intention and it makes me wonder. Other than those questions, which are optional as the code looks correct to me, the rest of it looks fine to me. i fully understand your concerns, i just followed other drivers behaviour. for sure its more sane to make it optional and to compile it out of not required. but then i suggest todo the same in
Re: [PATCH v9] ath10k: add LED and GPIO controlling support for various chipsets
On Thu, Feb 22, 2018 at 3:15 AM, wrote: > From: Sebastian Gottschall > > Adds LED and GPIO Control support for 988x, 9887, 9888, 99x0, 9984 based > chipsets with on chipset connected led's > using WMI Firmware API. > The LED device will get available named as "ath10k-phyX" at sysfs and can be > controlled with various triggers. > adds also debugfs interface for gpio control. > > Signed-off-by: Sebastian Gottschall > > v2 add correct gpio count per chipset and remove IPQ4019 support since this > chipset does not make use of specific gpios) > v5 fix compiling without LED_CLASS and GPIOLIB support, fix also error by > kbuild test robot which does not occur in standard builds. curious > v6 correct return values and fix comment style > v7 fix ath10k_unregister_led for compiling without LED_CLASS > v8 fix various code design issues reported by reviewers > v9 move led and led code to separate sourcefile (gpio.c) > --- > drivers/net/wireless/ath/ath10k/Kconfig | 3 + > drivers/net/wireless/ath/ath10k/Makefile | 1 + > drivers/net/wireless/ath/ath10k/core.c| 28 - > drivers/net/wireless/ath/ath10k/core.h| 33 - > drivers/net/wireless/ath/ath10k/debug.c | 142 ++ > drivers/net/wireless/ath/ath10k/gpio.c| 196 > ++ > drivers/net/wireless/ath/ath10k/hw.h | 2 + > drivers/net/wireless/ath/ath10k/mac.c | 5 + > drivers/net/wireless/ath/ath10k/wmi-ops.h | 36 +- > drivers/net/wireless/ath/ath10k/wmi-tlv.c | 65 ++ > drivers/net/wireless/ath/ath10k/wmi.c | 46 +++ > drivers/net/wireless/ath/ath10k/wmi.h | 36 ++ > 12 files changed, 590 insertions(+), 3 deletions(-) > create mode 100644 drivers/net/wireless/ath/ath10k/gpio.c > > diff --git a/drivers/net/wireless/ath/ath10k/Kconfig > b/drivers/net/wireless/ath/ath10k/Kconfig > index deb5ae21a559..c46b7658a820 100644 > --- a/drivers/net/wireless/ath/ath10k/Kconfig > +++ b/drivers/net/wireless/ath/ath10k/Kconfig > @@ -2,6 +2,9 @@ config ATH10K > tristate "Atheros 802.11ac wireless cards support" > depends on MAC80211 && HAS_DMA > select ATH_COMMON > +select MAC80211_LEDS > +select LEDS_CLASS > +select NEW_LEDS > select CRC32 > select WANT_DEV_COREDUMP > ---help--- Pure question: do we require LEDS, or is this an option? I assumed all along it was an optional thing - ie if GPIOs and/or LEDs were configured, we added this code. And if not optional, then what happens if we are on a platform that can't support LEDS_CLASS or NEW_LEDS, assuming there is such a thing? > diff --git a/drivers/net/wireless/ath/ath10k/Makefile > b/drivers/net/wireless/ath/ath10k/Makefile > index 6739ac26fd29..bcb234f0b940 100644 > --- a/drivers/net/wireless/ath/ath10k/Makefile > +++ b/drivers/net/wireless/ath/ath10k/Makefile > @@ -20,6 +20,7 @@ ath10k_core-$(CONFIG_NL80211_TESTMODE) += testmode.o > ath10k_core-$(CONFIG_ATH10K_TRACING) += trace.o > ath10k_core-$(CONFIG_THERMAL) += thermal.o > ath10k_core-$(CONFIG_MAC80211_DEBUGFS) += debugfs_sta.o > +ath10k_core-y += gpio.o > ath10k_core-$(CONFIG_PM) += wow.o > ath10k_core-$(CONFIG_DEV_COREDUMP) += coredump.o > I'd expect it would be something like `ath10k_core-$(CONFIG_GPIO) += gpoi.o`. Maybe I'm mistaken. (Note, I didn't look up the actual config option name, I'm guessing. Assume I wrote something reasonable for my example.) I only ask the above two questions, not because I think it's wrong, but because I don't quite know the intention and it makes me wonder. Other than those questions, which are optional as the code looks correct to me, the rest of it looks fine to me. Reviewed-by: Steve deRosier - Steve
[PATCH v9] ath10k: add LED and GPIO controlling support for various chipsets
From: Sebastian Gottschall
Adds LED and GPIO Control support for 988x, 9887, 9888, 99x0, 9984 based
chipsets with on chipset connected led's
using WMI Firmware API.
The LED device will get available named as "ath10k-phyX" at sysfs and can be
controlled with various triggers.
adds also debugfs interface for gpio control.
Signed-off-by: Sebastian Gottschall
v2 add correct gpio count per chipset and remove IPQ4019 support since this
chipset does not make use of specific gpios)
v5 fix compiling without LED_CLASS and GPIOLIB support, fix also error by
kbuild test robot which does not occur in standard builds. curious
v6 correct return values and fix comment style
v7 fix ath10k_unregister_led for compiling without LED_CLASS
v8 fix various code design issues reported by reviewers
v9 move led and led code to separate sourcefile (gpio.c)
---
drivers/net/wireless/ath/ath10k/Kconfig | 3 +
drivers/net/wireless/ath/ath10k/Makefile | 1 +
drivers/net/wireless/ath/ath10k/core.c| 28 -
drivers/net/wireless/ath/ath10k/core.h| 33 -
drivers/net/wireless/ath/ath10k/debug.c | 142 ++
drivers/net/wireless/ath/ath10k/gpio.c| 196 ++
drivers/net/wireless/ath/ath10k/hw.h | 2 +
drivers/net/wireless/ath/ath10k/mac.c | 5 +
drivers/net/wireless/ath/ath10k/wmi-ops.h | 36 +-
drivers/net/wireless/ath/ath10k/wmi-tlv.c | 65 ++
drivers/net/wireless/ath/ath10k/wmi.c | 46 +++
drivers/net/wireless/ath/ath10k/wmi.h | 36 ++
12 files changed, 590 insertions(+), 3 deletions(-)
create mode 100644 drivers/net/wireless/ath/ath10k/gpio.c
diff --git a/drivers/net/wireless/ath/ath10k/Kconfig
b/drivers/net/wireless/ath/ath10k/Kconfig
index deb5ae21a559..c46b7658a820 100644
--- a/drivers/net/wireless/ath/ath10k/Kconfig
+++ b/drivers/net/wireless/ath/ath10k/Kconfig
@@ -2,6 +2,9 @@ config ATH10K
tristate "Atheros 802.11ac wireless cards support"
depends on MAC80211 && HAS_DMA
select ATH_COMMON
+select MAC80211_LEDS
+select LEDS_CLASS
+select NEW_LEDS
select CRC32
select WANT_DEV_COREDUMP
---help---
diff --git a/drivers/net/wireless/ath/ath10k/Makefile
b/drivers/net/wireless/ath/ath10k/Makefile
index 6739ac26fd29..bcb234f0b940 100644
--- a/drivers/net/wireless/ath/ath10k/Makefile
+++ b/drivers/net/wireless/ath/ath10k/Makefile
@@ -20,6 +20,7 @@ ath10k_core-$(CONFIG_NL80211_TESTMODE) += testmode.o
ath10k_core-$(CONFIG_ATH10K_TRACING) += trace.o
ath10k_core-$(CONFIG_THERMAL) += thermal.o
ath10k_core-$(CONFIG_MAC80211_DEBUGFS) += debugfs_sta.o
+ath10k_core-y += gpio.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 f3ec13b80b20..d7f89ca98c2d 100644
--- a/drivers/net/wireless/ath/ath10k/core.c
+++ b/drivers/net/wireless/ath/ath10k/core.c
@@ -21,6 +21,10 @@
#include
#include
#include
+#include
+#include
+#include
+
#include "core.h"
#include "mac.h"
@@ -65,6 +69,8 @@ static const struct ath10k_hw_params ath10k_hw_params_list[]
= {
.id = QCA988X_HW_2_0_VERSION,
.dev_id = QCA988X_2_0_DEVICE_ID,
.name = "qca988x hw2.0",
+ .led_pin = 1,
+ .gpio_count = 24,
.patch_load_addr = QCA988X_HW_2_0_PATCH_LOAD_ADDR,
.uart_pin = 7,
.cc_wraparound_type = ATH10K_HW_CC_WRAP_SHIFTED_ALL,
@@ -94,6 +100,8 @@ static const struct ath10k_hw_params ath10k_hw_params_list[]
= {
.id = QCA988X_HW_2_0_VERSION,
.dev_id = QCA988X_2_0_DEVICE_ID_UBNT,
.name = "qca988x hw2.0 ubiquiti",
+ .led_pin = 1,
+ .gpio_count = 24,
.patch_load_addr = QCA988X_HW_2_0_PATCH_LOAD_ADDR,
.uart_pin = 7,
.cc_wraparound_type = ATH10K_HW_CC_WRAP_SHIFTED_ALL,
@@ -123,6 +131,8 @@ static const struct ath10k_hw_params
ath10k_hw_params_list[] = {
.id = QCA9887_HW_1_0_VERSION,
.dev_id = QCA9887_1_0_DEVICE_ID,
.name = "qca9887 hw1.0",
+ .led_pin = 1,
+ .gpio_count = 24,
.patch_load_addr = QCA9887_HW_1_0_PATCH_LOAD_ADDR,
.uart_pin = 7,
.cc_wraparound_type = ATH10K_HW_CC_WRAP_SHIFTED_ALL,
@@ -267,6 +277,8 @@ static const struct ath10k_hw_params
ath10k_hw_params_list[] = {
.id = QCA99X0_HW_2_0_DEV_VERSION,
.dev_id = QCA99X0_2_0_DEVICE_ID,
.name = "qca99x0 hw2.0",
+ .led_pin = 17,
+ .gpio_count = 35,
.patch_load_addr = QCA99X0_HW_2_0_PATCH_LOAD_ADDR,
.uart_pin = 7,
.otp_exe_param = 0x0700,
@@ -301,6 +313,8 @@ static const struct ath10k_hw_params
ath10k_hw_
