| Message ID | 20180226084406.2093-1-s.gottschall@dd-wrt.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Feb 26, 2018 at 12:44 AM, <s.gottschall@dd-wrt.com> wrote: > > From: Sebastian Gottschall <s.gottschall@newmedia-net.de> > > 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> > > 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) > v10 compile fix if gpiolib isnt included > v11 make register_gpio_chip static. advise by krobot > v12 fix warning > --- > drivers/net/wireless/ath/ath10k/Kconfig | 10 ++ > drivers/net/wireless/ath/ath10k/Makefile | 1 + > drivers/net/wireless/ath/ath10k/core.c | 28 ++++- > drivers/net/wireless/ath/ath10k/core.h | 62 +++++++++- > drivers/net/wireless/ath/ath10k/debug.c | 146 ++++++++++++++++++++++ > 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, 630 insertions(+), 3 deletions(-) > create mode 100644 drivers/net/wireless/ath/ath10k/gpio.c Assuming that kbuild robot doesn't kick back another build-time warning, it looks OK to me. Reviewed-by: Steve deRosier <derosier@cal-sierra.com> -- Steve deRosier Cal-Sierra Consulting LLC https://www.cal-sierra.com/
Am 27.02.2018 um 18:03 schrieb Steve deRosier: > On Mon, Feb 26, 2018 at 12:44 AM, <s.gottschall@dd-wrt.com> wrote: >> From: Sebastian Gottschall <s.gottschall@newmedia-net.de> >> >> 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> >> >> 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) >> v10 compile fix if gpiolib isnt included >> v11 make register_gpio_chip static. advise by krobot >> v12 fix warning >> --- >> drivers/net/wireless/ath/ath10k/Kconfig | 10 ++ >> drivers/net/wireless/ath/ath10k/Makefile | 1 + >> drivers/net/wireless/ath/ath10k/core.c | 28 ++++- >> drivers/net/wireless/ath/ath10k/core.h | 62 +++++++++- >> drivers/net/wireless/ath/ath10k/debug.c | 146 ++++++++++++++++++++++ >> 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, 630 insertions(+), 3 deletions(-) >> create mode 100644 drivers/net/wireless/ath/ath10k/gpio.c > > Assuming that kbuild robot doesn't kick back another build-time > warning, it looks OK to me. :-) seems so > > Reviewed-by: Steve deRosier <derosier@cal-sierra.com> > > -- > Steve deRosier > Cal-Sierra Consulting LLC > https://www.cal-sierra.com/ >
On 26 February 2018 at 09:44, <s.gottschall@dd-wrt.com> wrote: > From: Sebastian Gottschall <s.gottschall@newmedia-net.de> > > 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. Hi Sebastian, I just noticed this patch and have one question. It seems you register GPIO chip and that WiFi LED is controller by a GPIO. Shouldn't you use leds-gpio driver and just register platform device with gpio_led_platform_data? That way you could avoid some code duplication I think? It seems to be the purpose of leds-gpio driver. > Signed-off-by: Sebastian Gottschall <s.gottschall@dd-wrt.com> > > 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) > v10 compile fix if gpiolib isnt included > v11 make register_gpio_chip static. advise by krobot > v12 fix warning > --- > drivers/net/wireless/ath/ath10k/Kconfig | 10 ++ > drivers/net/wireless/ath/ath10k/Makefile | 1 + > drivers/net/wireless/ath/ath10k/core.c | 28 ++++- > drivers/net/wireless/ath/ath10k/core.h | 62 +++++++++- > drivers/net/wireless/ath/ath10k/debug.c | 146 ++++++++++++++++++++++ > 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, 630 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..5d61d499dca4 100644 > --- a/drivers/net/wireless/ath/ath10k/Kconfig > +++ b/drivers/net/wireless/ath/ath10k/Kconfig > @@ -10,6 +10,16 @@ config ATH10K > > If you choose to build a module, it'll be called ath10k. > > +config ATH10K_LEDS > + bool "SoftLED Support" > + depends on ATH10K > + select MAC80211_LEDS > + select LEDS_CLASS > + select NEW_LEDS > + default y > + help > + This option is necessary, if you want LED support for chipset connected led pins > + > config ATH10K_PCI > tristate "Atheros ath10k PCI support" > depends on ATH10K && PCI > diff --git a/drivers/net/wireless/ath/ath10k/Makefile b/drivers/net/wireless/ath/ath10k/Makefile > index 6739ac26fd29..eccc9806fa43 100644 > --- a/drivers/net/wireless/ath/ath10k/Makefile > +++ b/drivers/net/wireless/ath/ath10k/Makefile > @@ -20,6 +20,7 @@ ath10k_core-$(CONFIG_NL80211_TESTMODE) += testmode.o > ath10k_core-$(CONFIG_ATH10K_TRACING) += trace.o > ath10k_core-$(CONFIG_THERMAL) += thermal.o > ath10k_core-$(CONFIG_MAC80211_DEBUGFS) += debugfs_sta.o > +ath10k_core-$(CONFIG_ATH10K_LEDS) += 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 <linux/dmi.h> > #include <linux/ctype.h> > #include <asm/byteorder.h> > +#include <linux/leds.h> > +#include <linux/platform_device.h> > +#include <linux/version.h> > + > > #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 = 0x00000700, > @@ -301,6 +313,8 @@ static const struct ath10k_hw_params ath10k_hw_params_list[] = { > .id = QCA9984_HW_1_0_DEV_VERSION, > .dev_id = QCA9984_1_0_DEVICE_ID, > .name = "qca9984/qca9994 hw1.0", > + .led_pin = 17, > + .gpio_count = 35, > .patch_load_addr = QCA9984_HW_1_0_PATCH_LOAD_ADDR, > .uart_pin = 7, > .cc_wraparound_type = ATH10K_HW_CC_WRAP_SHIFTED_EACH, > @@ -340,6 +354,8 @@ static const struct ath10k_hw_params ath10k_hw_params_list[] = { > .id = QCA9888_HW_2_0_DEV_VERSION, > .dev_id = QCA9888_2_0_DEVICE_ID, > .name = "qca9888 hw2.0", > + .led_pin = 17, > + .gpio_count = 35, > .patch_load_addr = QCA9888_HW_2_0_PATCH_LOAD_ADDR, > .uart_pin = 7, > .cc_wraparound_type = ATH10K_HW_CC_WRAP_SHIFTED_EACH, > @@ -2372,8 +2388,15 @@ int ath10k_core_start(struct ath10k *ar, enum ath10k_firmware_mode mode, > if (status) > goto err_hif_stop; > > - return 0; > + > + status = ath10k_attach_led(ar); > + if (status) > + goto err_no_led; > > + ath10k_attach_gpio(ar); > + > +err_no_led: > + return 0; > err_hif_stop: > ath10k_hif_stop(ar); > err_htt_rx_detach: > @@ -2673,6 +2696,9 @@ void ath10k_core_unregister(struct ath10k *ar) > ath10k_core_free_board_files(ar); > > ath10k_debug_unregister(ar); > + > + ath10k_unregister_gpio_chip(ar); > + ath10k_unregister_led(ar); > } > EXPORT_SYMBOL(ath10k_core_unregister); > > diff --git a/drivers/net/wireless/ath/ath10k/core.h b/drivers/net/wireless/ath/ath10k/core.h > index c624b96f8b84..ed1789e716e9 100644 > --- a/drivers/net/wireless/ath/ath10k/core.h > +++ b/drivers/net/wireless/ath/ath10k/core.h > @@ -25,6 +25,8 @@ > #include <linux/pci.h> > #include <linux/uuid.h> > #include <linux/time.h> > +#include <linux/gpio.h> > +#include <linux/leds.h> > > #include "htt.h" > #include "htc.h" > @@ -812,6 +814,62 @@ struct ath10k_per_peer_tx_stats { > u32 reserved2; > }; > > +struct ath10k_gpiocontrol { > + struct ath10k *ar; > + > + /* since we have no gpio read method, these are the state variables for debugfs. */ > + u32 gpio_set_num; > + u32 gpio_num; > + u32 gpio_input; > + u32 gpio_pull_type; > + u32 gpio_intr_mode; > + u32 gpio_set; > + > +#if IS_ENABLED(CONFIG_GPIOLIB) > + struct gpio_chip gchip; > +#endif /* CONFIG_GPIOLIB */ > + struct gpio_led wifi_led; > + struct led_classdev cdev; > + char label[48]; > + u32 gpio_state_dir; /* same as for debugfs, but for gpiochip implementation */ > + u32 gpio_state_pin; > +}; > + > +#if IS_ENABLED(CONFIG_ATH10K_LEDS) > +void ath10k_unregister_led(struct ath10k *ar); > +void ath10k_reset_led_pin(struct ath10k *ar); > +int ath10k_attach_led(struct ath10k *ar); > +#else > +static inline void ath10k_unregister_led(struct ath10k *ar) > +{ > + > +} > +static inline void ath10k_reset_led_pin(struct ath10k *ar) > +{ > + > +} > +static inline int ath10k_attach_led(struct ath10k *ar) > +{ > + return -ENODEV; > +} > +#endif > + > +#if IS_ENABLED(CONFIG_ATH10K_LEDS) && IS_ENABLED(CONFIG_GPIOLIB) > +void ath10k_unregister_gpio_chip(struct ath10k *ar); > +#else > +static inline void ath10k_unregister_gpio_chip(struct ath10k *ar) > +{ > + > +} > +#endif > +#if IS_ENABLED(CONFIG_ATH10K_LEDS) > +int ath10k_attach_gpio(struct ath10k *ar); > +#else > +static inline int ath10k_attach_gpio(struct ath10k *ar) > +{ > + return -ENODEV; > +} > +#endif > struct ath10k { > struct ath_common ath_common; > struct ieee80211_hw *hw; > @@ -840,7 +898,9 @@ struct ath10k { > u32 low_5ghz_chan; > u32 high_5ghz_chan; > bool ani_enabled; > - > +#if IS_ENABLED(CONFIG_ATH10K_LEDS) > + struct ath10k_gpiocontrol *gpio; > +#endif > bool p2p; > > struct { > diff --git a/drivers/net/wireless/ath/ath10k/debug.c b/drivers/net/wireless/ath/ath10k/debug.c > index 1b9c092d210f..a8baa7424633 100644 > --- a/drivers/net/wireless/ath/ath10k/debug.c > +++ b/drivers/net/wireless/ath/ath10k/debug.c > @@ -1084,6 +1084,126 @@ static ssize_t ath10k_write_fw_dbglog(struct file *file, > return ret; > } > > +#if IS_ENABLED(CONFIG_ATH10K_LEDS) > +static ssize_t ath10k_read_gpio_config(struct file *file, char __user *user_buf, > + size_t count, loff_t *ppos) > +{ > + struct ath10k *ar = file->private_data; > + struct ath10k_gpiocontrol *gpio = ar->gpio; > + size_t len; > + char buf[96]; > + if (!gpio) > + return 0; > + > + len = scnprintf(buf, sizeof(buf), "%u %u %u %u\n", gpio->gpio_num, gpio->gpio_input, gpio->gpio_pull_type, gpio->gpio_intr_mode); > + > + return simple_read_from_buffer(user_buf, count, ppos, buf, len); > +} > + > + > +static ssize_t ath10k_write_gpio_config(struct file *file, > + const char __user *user_buf, > + size_t count, loff_t *ppos) > +{ > + struct ath10k *ar = file->private_data; > + struct ath10k_gpiocontrol *gpio = ar->gpio; > + int ret; > + char buf[96]; > + u32 gpio_num, input, pull_type, intr_mode; > + if (!gpio) > + return -EINVAL; > + > + simple_write_to_buffer(buf, sizeof(buf) - 1, ppos, user_buf, count); > + > + /* make sure that buf is null terminated */ > + buf[sizeof(buf) - 1] = 0; > + > + gpio->gpio_num = gpio_num; > + gpio->gpio_input = input; > + gpio->gpio_pull_type = pull_type; > + gpio->gpio_intr_mode = intr_mode; > + ret = sscanf(buf, "%u %u %u %u", &gpio_num, &input, &pull_type, &intr_mode); > + > + if (!ret) > + return -EINVAL; > + > + > + mutex_lock(&ar->conf_mutex); > + > + > + ret = ath10k_wmi_gpio_config(ar, gpio_num, input, pull_type, intr_mode); > + > + if (ret) { > + ath10k_warn(ar, "gpio_config cfg failed from debugfs: %d\n", ret); > + goto exit; > + } > + ret = count; > + > +exit: > + mutex_unlock(&ar->conf_mutex); > + > + return ret; > +} > + > +static ssize_t ath10k_read_gpio_output(struct file *file, char __user *user_buf, > + size_t count, loff_t *ppos) > +{ > + struct ath10k *ar = file->private_data; > + struct ath10k_gpiocontrol *gpio = ar->gpio; > + size_t len; > + char buf[96]; > + if (!gpio) > + return 0; > + > + len = scnprintf(buf, sizeof(buf), "%u %u\n", gpio->gpio_num, gpio->gpio_set); > + > + return simple_read_from_buffer(user_buf, count, ppos, buf, len); > +} > + > + > +static ssize_t ath10k_write_gpio_output(struct file *file, > + const char __user *user_buf, > + size_t count, loff_t *ppos) > +{ > + struct ath10k *ar = file->private_data; > + struct ath10k_gpiocontrol *gpio = ar->gpio; > + int ret; > + char buf[96]; > + u32 gpio_num, set; > + if (!gpio) > + return -EINVAL; > + > + simple_write_to_buffer(buf, sizeof(buf) - 1, ppos, user_buf, count); > + > + /* make sure that buf is null terminated */ > + buf[sizeof(buf) - 1] = 0; > + > + gpio->gpio_set_num = gpio_num; > + gpio->gpio_set = set; > + ret = sscanf(buf, "%u %u", &gpio_num, &set); > + > + if (!ret) > + return -EINVAL; > + > + > + mutex_lock(&ar->conf_mutex); > + > + > + ret = ath10k_wmi_gpio_output(ar, gpio_num, set); > + > + if (ret) { > + ath10k_warn(ar, "gpio_output cfg failed from debugfs: %d\n", ret); > + goto exit; > + } > + ret = count; > + > +exit: > + mutex_unlock(&ar->conf_mutex); > + > + return ret; > +} > +#endif > + > /* TODO: Would be nice to always support ethtool stats, would need to > * move the stats storage out of ath10k_debug, or always have ath10k_debug > * struct available.. > @@ -1252,6 +1372,25 @@ static const struct file_operations fops_fw_dbglog = { > .llseek = default_llseek, > }; > > + > +#if IS_ENABLED(CONFIG_ATH10K_LEDS) > +static const struct file_operations fops_gpio_output = { > + .read = ath10k_read_gpio_output, > + .write = ath10k_write_gpio_output, > + .open = simple_open, > + .owner = THIS_MODULE, > + .llseek = default_llseek, > +}; > + > +static const struct file_operations fops_gpio_config = { > + .read = ath10k_read_gpio_config, > + .write = ath10k_write_gpio_config, > + .open = simple_open, > + .owner = THIS_MODULE, > + .llseek = default_llseek, > +}; > +#endif > + > static int ath10k_debug_cal_data_fetch(struct ath10k *ar) > { > u32 hi_addr; > @@ -2259,6 +2398,13 @@ int ath10k_debug_register(struct ath10k *ar) > debugfs_create_file("fw_dbglog", 0600, ar->debug.debugfs_phy, ar, > &fops_fw_dbglog); > > +#if IS_ENABLED(CONFIG_ATH10K_LEDS) > + debugfs_create_file("gpio_output", 0600, ar->debug.debugfs_phy, ar, > + &fops_gpio_output); > + > + debugfs_create_file("gpio_config", 0600, ar->debug.debugfs_phy, ar, > + &fops_gpio_config); > +#endif > debugfs_create_file("cal_data", 0400, ar->debug.debugfs_phy, ar, > &fops_cal_data); > > diff --git a/drivers/net/wireless/ath/ath10k/gpio.c b/drivers/net/wireless/ath/ath10k/gpio.c > new file mode 100644 > index 000000000000..e9ce44bb21b0 > --- /dev/null > +++ b/drivers/net/wireless/ath/ath10k/gpio.c > @@ -0,0 +1,196 @@ > +/* > + * Copyright (c) 2005-2011 Atheros Communications Inc. > + * Copyright (c) 2011-2017 Qualcomm Atheros, Inc. > + * Copyright (c) 2018 Sebastian Gottschall <s.gottschall@dd-wrt.com> > + * > + * Permission to use, copy, modify, and/or distribute this software for any > + * purpose with or without fee is hereby granted, provided that the above > + * copyright notice and this permission notice appear in all copies. > + * > + * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES > + * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF > + * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR > + * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES > + * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN > + * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF > + * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. > + */ > + > + > +#include <linux/module.h> > +#include <linux/firmware.h> > +#include <linux/of.h> > +#include <linux/dmi.h> > +#include <linux/ctype.h> > +#include <asm/byteorder.h> > +#include <linux/leds.h> > +#include <linux/platform_device.h> > +#include <linux/version.h> > + > + > +#include "core.h" > +#include "wmi.h" > +#include "wmi-ops.h" > + > +#if IS_ENABLED(CONFIG_GPIOLIB) > +static int ath10k_gpio_pin_cfg_input(struct gpio_chip *chip, unsigned offset) > +{ > + struct ath10k_gpiocontrol *gpio = container_of(chip, struct ath10k_gpiocontrol, gchip); > + ath10k_wmi_gpio_config(gpio->ar, offset, 1, WMI_GPIO_PULL_NONE, WMI_GPIO_INTTYPE_DISABLE); /* configure to input */ > + gpio->gpio_state_dir = 1; > + return 0; > +} > + > +/* gpio_chip handler : set GPIO to output */ > +static int ath10k_gpio_pin_cfg_output(struct gpio_chip *chip, unsigned offset, > + int value) > +{ > + struct ath10k_gpiocontrol *gpio = container_of(chip, struct ath10k_gpiocontrol, gchip); > + > + ath10k_wmi_gpio_config(gpio->ar, offset, 0, WMI_GPIO_PULL_NONE, WMI_GPIO_INTTYPE_DISABLE); /* configure to output */ > + ath10k_wmi_gpio_output(gpio->ar, offset, value); > + gpio->gpio_state_dir = 0; > + gpio->gpio_state_pin = value; > + return 0; > +} > + > +/* gpio_chip handler : query GPIO direction (0=out, 1=in) */ > +static int ath10k_gpio_pin_get_dir(struct gpio_chip *chip, unsigned offset) > +{ > + struct ath10k_gpiocontrol *gpio = container_of(chip, struct ath10k_gpiocontrol, gchip); > + > + return gpio->gpio_state_dir; > +} > + > +/* gpio_chip handler : get GPIO pin value */ > +static int ath10k_gpio_pin_get(struct gpio_chip *chip, unsigned offset) > +{ > + struct ath10k_gpiocontrol *gpio = container_of(chip, struct ath10k_gpiocontrol, gchip); > + > + return gpio->gpio_state_pin; > +} > + > +/* gpio_chip handler : set GPIO pin to value */ > +static void ath10k_gpio_pin_set(struct gpio_chip *chip, unsigned offset, > + int value) > +{ > + struct ath10k_gpiocontrol *gpio = container_of(chip, struct ath10k_gpiocontrol, gchip); > + > + ath10k_wmi_gpio_output(gpio->ar, offset, value); > + gpio->gpio_state_pin = value; > +} > + > +/* register GPIO chip */ > +static int ath10k_register_gpio_chip(struct ath10k *ar) > +{ > + > + struct ath10k_gpiocontrol *gpio = ar->gpio; > + if (!gpio) { > + return -ENODEV; > + } > + gpio->gchip.parent = ar->dev; > + gpio->gchip.base = -1; /* determine base automatically */ > + gpio->gchip.ngpio = ar->hw_params.gpio_count; > + gpio->gchip.label = gpio->label; > + gpio->gchip.direction_input = ath10k_gpio_pin_cfg_input; > + gpio->gchip.direction_output = ath10k_gpio_pin_cfg_output; > + gpio->gchip.get_direction = ath10k_gpio_pin_get_dir; > + gpio->gchip.get = ath10k_gpio_pin_get; > + gpio->gchip.set = ath10k_gpio_pin_set; > + > + if (gpiochip_add(&gpio->gchip)) { > + dev_err(ar->dev, "Error while registering gpio chip\n"); > + return -ENODEV; > + } > + gpio->gchip.owner = NULL; > + gpio->ar = ar; > + return 0; > +} > + > +/* remove GPIO chip */ > +void ath10k_unregister_gpio_chip(struct ath10k *ar) > +{ > + struct ath10k_gpiocontrol *gpio = ar->gpio; > + if (gpio) { > + gpiochip_remove(&gpio->gchip); > + } > +} > + > +int ath10k_attach_gpio(struct ath10k *ar) > +{ > + if (ar->hw_params.led_pin) { /* only attach if non zero since some chipsets are unsupported yet */ > + return ath10k_register_gpio_chip(ar); > + } > + > + return -ENODEV; > +} > + > +#endif > + > + > +static void ath10k_led_brightness(struct led_classdev *led_cdev, > + enum led_brightness brightness) > +{ > + struct ath10k_gpiocontrol *gpio = container_of(led_cdev, struct ath10k_gpiocontrol, cdev); > + struct gpio_led *led = &gpio->wifi_led; > + if (gpio->ar->state == ATH10K_STATE_ON) { > + gpio->gpio_state_pin = (brightness != LED_OFF) ^ led->active_low; > + ath10k_wmi_gpio_output(gpio->ar, led->gpio, gpio->gpio_state_pin); > + } > +} > + > +static int ath10k_add_led(struct ath10k *ar, struct gpio_led *gpioled) > +{ > + int ret; > + struct ath10k_gpiocontrol *gpio = ar->gpio; > + gpio->cdev.name = gpioled->name; > + gpio->cdev.default_trigger = gpioled->default_trigger; > + gpio->cdev.brightness_set = ath10k_led_brightness; > + > + ret = led_classdev_register(wiphy_dev(ar->hw->wiphy), &gpio->cdev); > + if (ret < 0) > + return ret; > + > + return 0; > +} > + > +void ath10k_unregister_led(struct ath10k *ar) > +{ > + struct ath10k_gpiocontrol *gpio = ar->gpio; > + if (gpio) { > + led_classdev_unregister(&gpio->cdev); > + kfree(gpio); > + } > +} > + > +void ath10k_reset_led_pin(struct ath10k *ar) > +{ > + /* need to reset gpio state */ > + if (ar->hw_params.led_pin) { > + 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); > + } > +} > + > +int ath10k_attach_led(struct ath10k *ar) > +{ > + struct ath10k_gpiocontrol *gpio; > + if (ar->gpio) { /* already registered, ignore */ > + return -EINVAL; > + } > + gpio = kzalloc(sizeof(struct ath10k_gpiocontrol), GFP_KERNEL); > + if (!gpio) { > + return -ENOMEM; > + } > + ar->gpio = gpio; > + snprintf(gpio->label, sizeof(gpio->label), "ath10k-%s", > + wiphy_name(ar->hw->wiphy)); > + gpio->wifi_led.active_low = 1; > + gpio->wifi_led.gpio = ar->hw_params.led_pin; > + gpio->wifi_led.name = gpio->label; > + gpio->wifi_led.default_state = LEDS_GPIO_DEFSTATE_KEEP; > + > + ath10k_add_led(ar, &gpio->wifi_led); > + ath10k_reset_led_pin(ar); /* initially we need to configure the led pin to output */ > + return 0; > +} > diff --git a/drivers/net/wireless/ath/ath10k/hw.h b/drivers/net/wireless/ath/ath10k/hw.h > index 413b1b4321f7..af91d0202e45 100644 > --- a/drivers/net/wireless/ath/ath10k/hw.h > +++ b/drivers/net/wireless/ath/ath10k/hw.h > @@ -495,6 +495,8 @@ struct ath10k_hw_params { > const char *name; > u32 patch_load_addr; > int uart_pin; > + int led_pin; /* 1 for peregrine, 17 for beeliner */ > + int gpio_count; /* 24 for peregrine, 35 for beeliner */ > u32 otp_exe_param; > > /* Type of hw cycle counter wraparound logic, for more info > diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c > index ebb3f1b046f3..6c4925dfcde9 100644 > --- a/drivers/net/wireless/ath/ath10k/mac.c > +++ b/drivers/net/wireless/ath/ath10k/mac.c > @@ -4601,6 +4601,11 @@ static int ath10k_start(struct ieee80211_hw *hw) > switch (ar->state) { > case ATH10K_STATE_OFF: > ar->state = ATH10K_STATE_ON; > + /* > + * 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_reset_led_pin(ar); > break; > case ATH10K_STATE_RESTARTING: > ar->state = ATH10K_STATE_RESTARTED; > diff --git a/drivers/net/wireless/ath/ath10k/wmi-ops.h b/drivers/net/wireless/ath/ath10k/wmi-ops.h > index 14093cfdc505..a38d6a8f7ec5 100644 > --- a/drivers/net/wireless/ath/ath10k/wmi-ops.h > +++ b/drivers/net/wireless/ath/ath10k/wmi-ops.h > @@ -197,8 +197,13 @@ struct wmi_ops { > (struct ath10k *ar, > enum wmi_bss_survey_req_type type); > struct sk_buff *(*gen_echo)(struct ath10k *ar, u32 value); > + > + 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); > > static inline int > @@ -957,6 +962,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_nowait(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_nowait(ar, skb, ar->wmi.cmd->gpio_output_cmdid); > +} > > static inline int > ath10k_wmi_dbglog_cfg(struct ath10k *ar, u64 module_enable, u32 log_level) > @@ -1034,7 +1068,7 @@ ath10k_wmi_pdev_get_temperature(struct ath10k *ar) > if (IS_ERR(skb)) > return PTR_ERR(skb); > > - return ath10k_wmi_cmd_send(ar, skb, > + return ath10k_wmi_cmd_send_nowait(ar, skb, > ar->wmi.cmd->pdev_get_temperature_cmdid); > } > > diff --git a/drivers/net/wireless/ath/ath10k/wmi-tlv.c b/drivers/net/wireless/ath/ath10k/wmi-tlv.c > index ae77a007ae07..2bfba63f1dcf 100644 > --- a/drivers/net/wireless/ath/ath10k/wmi-tlv.c > +++ b/drivers/net/wireless/ath/ath10k/wmi-tlv.c > @@ -3250,6 +3250,69 @@ ath10k_wmi_tlv_op_gen_echo(struct ath10k *ar, u32 value) > return skb; > } > > +static struct sk_buff * > +ath10k_wmi_tlv_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 wmi_tlv *tlv; > + struct sk_buff *skb; > + void *ptr; > + size_t len; > + > + len = sizeof(*tlv) + sizeof(*cmd); > + skb = ath10k_wmi_alloc_skb(ar, len); > + if (!skb) > + return ERR_PTR(-ENOMEM); > + > + ptr = (void *)skb->data; > + tlv = ptr; > + tlv->tag = __cpu_to_le16(WMI_TLV_TAG_STRUCT_GPIO_CONFIG_CMD); > + tlv->len = __cpu_to_le16(sizeof(*cmd)); > + > + 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); > + > + ptr += sizeof(*tlv); > + ptr += sizeof(*cmd); > + > + ath10k_dbg(ar, ATH10K_DBG_WMI, "wmi tlv 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_tlv_op_gen_gpio_output(struct ath10k *ar, u32 gpio_num, u32 set) > +{ > + struct wmi_gpio_output_cmd *cmd; > + struct wmi_tlv *tlv; > + struct sk_buff *skb; > + void *ptr; > + size_t len; > + > + len = sizeof(*tlv) + sizeof(*cmd); > + skb = ath10k_wmi_alloc_skb(ar, len); > + if (!skb) > + return ERR_PTR(-ENOMEM); > + > + ptr = (void *)skb->data; > + tlv = ptr; > + tlv->tag = __cpu_to_le16(WMI_TLV_TAG_STRUCT_GPIO_OUTPUT_CMD); > + tlv->len = __cpu_to_le16(sizeof(*cmd)); > + > + cmd = (struct wmi_gpio_output_cmd *)skb->data; > + cmd->gpio_num = __cpu_to_le32(gpio_num); > + cmd->set = __cpu_to_le32(set); > + > + ptr += sizeof(*tlv); > + ptr += sizeof(*cmd); > + > + ath10k_dbg(ar, ATH10K_DBG_WMI, "wmi tlv gpio_output gpio_num 0x%08x set 0x%08x\n", gpio_num, set); > + return skb; > +} > + > + > static struct sk_buff * > ath10k_wmi_tlv_op_gen_vdev_spectral_conf(struct ath10k *ar, > const struct wmi_vdev_spectral_conf_arg *arg) > @@ -3727,6 +3790,8 @@ static const struct wmi_ops wmi_tlv_ops = { > .fw_stats_fill = ath10k_wmi_main_op_fw_stats_fill, > .get_vdev_subtype = ath10k_wmi_op_get_vdev_subtype, > .gen_echo = ath10k_wmi_tlv_op_gen_echo, > + .gen_gpio_config = ath10k_wmi_tlv_op_gen_gpio_config, > + .gen_gpio_output = ath10k_wmi_tlv_op_gen_gpio_output, > .gen_vdev_spectral_conf = ath10k_wmi_tlv_op_gen_vdev_spectral_conf, > .gen_vdev_spectral_enable = ath10k_wmi_tlv_op_gen_vdev_spectral_enable, > }; > diff --git a/drivers/net/wireless/ath/ath10k/wmi.c b/drivers/net/wireless/ath/ath10k/wmi.c > index 58dc2189ba49..b56e5a673a8c 100644 > --- a/drivers/net/wireless/ath/ath10k/wmi.c > +++ b/drivers/net/wireless/ath/ath10k/wmi.c > @@ -6646,6 +6646,41 @@ 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) > @@ -8153,6 +8188,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 */ > @@ -8223,6 +8261,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 */ > @@ -8294,6 +8334,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 */ > }; > > @@ -8364,6 +8406,8 @@ static const struct wmi_ops wmi_10_2_4_ops = { > .gen_pdev_enable_adaptive_cca = > ath10k_wmi_op_gen_pdev_enable_adaptive_cca, > .get_vdev_subtype = ath10k_wmi_10_2_4_op_get_vdev_subtype, > + .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 */ > @@ -8439,6 +8483,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 c7b30ed9015d..dc180a86dc3b 100644 > --- a/drivers/net/wireless/ath/ath10k/wmi.h > +++ b/drivers/net/wireless/ath/ath10k/wmi.h > @@ -2906,6 +2906,42 @@ 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.14.1 >
Am 27.02.2018 um 23:08 schrieb Rafał Miłecki: > On 26 February 2018 at 09:44, <s.gottschall@dd-wrt.com> wrote: >> From: Sebastian Gottschall <s.gottschall@newmedia-net.de> >> >> 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. > Hi Sebastian, > > I just noticed this patch and have one question. It seems you register > GPIO chip and that WiFi LED is controller by a GPIO. Shouldn't you use > leds-gpio driver and just register platform device with > gpio_led_platform_data? That way you could avoid some code duplication > I think? It seems to be the purpose of leds-gpio driver. leds-gpio is crap and limited. you can just register one platform data at kernel runtime since its identified by its object name "led-gpio" but the kernel forbidds to register 2 platform datas with the same name consider the ar71xx devices with qca988x wifi chipsets. they all have already a led platform data registered at boottime. a second can't be registered anymore so gpio_led is useless at all for most developers on such platforms. its mainly used for early kernel platform data initialisation for system leds. at the beginning of development of this patch i started in that way and found out this limitation, so i had to rewrite everything. the good point now is, it works even without gpiolib support since the gpio device is just a option, but not required for led control.
Hi! > >>adds also debugfs interface for gpio control. > >Hi Sebastian, > > > >I just noticed this patch and have one question. It seems you register > >GPIO chip and that WiFi LED is controller by a GPIO. Shouldn't you use > >leds-gpio driver and just register platform device with > >gpio_led_platform_data? That way you could avoid some code duplication > >I think? It seems to be the purpose of leds-gpio driver. > leds-gpio is crap and limited. you can just register one platform data at > kernel runtime since its identified by its object name "led-gpio" but the > kernel forbidds to register 2 platform datas with the same name > consider the ar71xx devices with qca988x wifi chipsets. they all have > already a led platform data registered > at boottime. a second can't be registered anymore so gpio_led is useless at > all for most developers on such platforms. its mainly used for early kernel > platform data initialisation for system leds. If leds-gpio has limitations, please fix those, rather then introducing duplicated code. NAK. Pavel
>> leds-gpio is crap and limited. you can just register one platform data at >> kernel runtime since its identified by its object name "led-gpio" but the >> kernel forbidds to register 2 platform datas with the same name >> consider the ar71xx devices with qca988x wifi chipsets. they all have >> already a led platform data registered >> at boottime. a second can't be registered anymore so gpio_led is useless at >> all for most developers on such platforms. its mainly used for early kernel >> platform data initialisation for system leds. > If leds-gpio has limitations, please fix those, rather then > introducing duplicated code. there is no duplicated code introduced and there is no solution for it. consider that all wifi drivers with softled support are going that way with registering a own led driver. see ath9k for instance. gpio-led cannot be used for it and there is no way to support multiple platform datas with the same name. its a kernel limitation Sebastian > > NAK. > Pavel
On 2 March 2018 at 10:22, Sebastian Gottschall <s.gottschall@dd-wrt.com> wrote: >>> leds-gpio is crap and limited. you can just register one platform data at >>> kernel runtime since its identified by its object name "led-gpio" but the >>> kernel forbidds to register 2 platform datas with the same name >>> consider the ar71xx devices with qca988x wifi chipsets. they all have >>> already a led platform data registered >>> at boottime. a second can't be registered anymore so gpio_led is useless >>> at >>> all for most developers on such platforms. its mainly used for early >>> kernel >>> platform data initialisation for system leds. >> >> If leds-gpio has limitations, please fix those, rather then >> introducing duplicated code. > > there is no duplicated code introduced and there is no solution for it. > consider that all wifi drivers with softled support > are going that way with registering a own led driver. see ath9k for > instance. gpio-led cannot be used for it and there is no way to > support multiple platform datas with the same name. its a kernel limitation I just reviewed some mips arch patch adding support for more LEDs for selected devices: [PATCH] MIPS: BCM47XX: Add Luxul XAP1500/XWR1750 WiFi LEDs https://patchwork.linux-mips.org/patch/18681/ It seems to be simply adding another call to the gpio_led_register_device(). It seems to me you can call that function multiple times and register multiple structs with LEDs. Isn't all you need something like this? static const struct gpio_led ath10k_leds[] = { { .name = "ath10k:color:function", .active_low = 1, .default_state = LEDS_GPIO_DEFSTATE_KEEP, } }; static struct gpio_led_platform_data bcm47xx_leds_pdata_extra = { leds = ath10k_leds; num_leds = ARRAY_SIZE(ath10k_leds); }; ath10k_leds.gpio = ar->hw_params.led_pin; gpio_led_register_device(0, &ath10k_leds);
Am 07.03.2018 um 17:22 schrieb Rafał Miłecki: > On 2 March 2018 at 10:22, Sebastian Gottschall <s.gottschall@dd-wrt.com> wrote: >>>> leds-gpio is crap and limited. you can just register one platform data at >>>> kernel runtime since its identified by its object name "led-gpio" but the >>>> kernel forbidds to register 2 platform datas with the same name >>>> consider the ar71xx devices with qca988x wifi chipsets. they all have >>>> already a led platform data registered >>>> at boottime. a second can't be registered anymore so gpio_led is useless >>>> at >>>> all for most developers on such platforms. its mainly used for early >>>> kernel >>>> platform data initialisation for system leds. >>> If leds-gpio has limitations, please fix those, rather then >>> introducing duplicated code. >> there is no duplicated code introduced and there is no solution for it. >> consider that all wifi drivers with softled support >> are going that way with registering a own led driver. see ath9k for >> instance. gpio-led cannot be used for it and there is no way to >> support multiple platform datas with the same name. its a kernel limitation > I just reviewed some mips arch patch adding support for more LEDs for > selected devices: > [PATCH] MIPS: BCM47XX: Add Luxul XAP1500/XWR1750 WiFi LEDs > https://patchwork.linux-mips.org/patch/18681/ > > It seems to be simply adding another call to the > gpio_led_register_device(). It seems to me you can call that function > multiple times and register multiple structs with LEDs. > > Isn't all you need something like this? > > static const struct gpio_led ath10k_leds[] = { > { > .name = "ath10k:color:function", > .active_low = 1, > .default_state = LEDS_GPIO_DEFSTATE_KEEP, > } > }; > > static struct gpio_led_platform_data bcm47xx_leds_pdata_extra = { > leds = ath10k_leds; > num_leds = ARRAY_SIZE(ath10k_leds); > }; > > ath10k_leds.gpio = ar->hw_params.led_pin; > gpio_led_register_device(0, &ath10k_leds); the problem are other architectures which have already registered gpio_led at system start like ar71xx you cannot register a second one. so a independend led driver is a requirement for direct control Sebastian >
On Wed 2018-03-07 18:54:41, Sebastian Gottschall wrote: > Am 07.03.2018 um 17:22 schrieb Rafał Miłecki: > >On 2 March 2018 at 10:22, Sebastian Gottschall <s.gottschall@dd-wrt.com> wrote: > >>>>leds-gpio is crap and limited. you can just register one platform data at > >>>>kernel runtime since its identified by its object name "led-gpio" but the > >>>>kernel forbidds to register 2 platform datas with the same name > >>>>consider the ar71xx devices with qca988x wifi chipsets. they all have > >>>>already a led platform data registered > >>>>at boottime. a second can't be registered anymore so gpio_led is useless > >>>>at > >>>>all for most developers on such platforms. its mainly used for early > >>>>kernel > >>>>platform data initialisation for system leds. > >>>If leds-gpio has limitations, please fix those, rather then > >>>introducing duplicated code. > >>there is no duplicated code introduced and there is no solution for it. > >>consider that all wifi drivers with softled support > >>are going that way with registering a own led driver. see ath9k for > >>instance. gpio-led cannot be used for it and there is no way to > >>support multiple platform datas with the same name. its a kernel limitation > >I just reviewed some mips arch patch adding support for more LEDs for > >selected devices: > >[PATCH] MIPS: BCM47XX: Add Luxul XAP1500/XWR1750 WiFi LEDs > >https://patchwork.linux-mips.org/patch/18681/ > > > >It seems to be simply adding another call to the > >gpio_led_register_device(). It seems to me you can call that function > >multiple times and register multiple structs with LEDs. > > > >Isn't all you need something like this? > > > >static const struct gpio_led ath10k_leds[] = { > > { > > .name = "ath10k:color:function", > > .active_low = 1, > > .default_state = LEDS_GPIO_DEFSTATE_KEEP, > > } > >}; > > > >static struct gpio_led_platform_data bcm47xx_leds_pdata_extra = { > > leds = ath10k_leds; > > num_leds = ARRAY_SIZE(ath10k_leds); > >}; > > > >ath10k_leds.gpio = ar->hw_params.led_pin; > >gpio_led_register_device(0, &ath10k_leds); > the problem are other architectures which have already registered gpio_led > at system start like ar71xx > you cannot register a second one. so a independend led driver is a > requirement for direct control If the limitation indeed exists, please fix the limitation rather than working around it in each and every driver. Pavel
Am 08.03.2018 um 10:02 schrieb Pavel Machek: > On Wed 2018-03-07 18:54:41, Sebastian Gottschall wrote: >> Am 07.03.2018 um 17:22 schrieb Rafał Miłecki: >>> On 2 March 2018 at 10:22, Sebastian Gottschall <s.gottschall@dd-wrt.com> wrote: >>>>>> leds-gpio is crap and limited. you can just register one platform data at >>>>>> kernel runtime since its identified by its object name "led-gpio" but the >>>>>> kernel forbidds to register 2 platform datas with the same name >>>>>> consider the ar71xx devices with qca988x wifi chipsets. they all have >>>>>> already a led platform data registered >>>>>> at boottime. a second can't be registered anymore so gpio_led is useless >>>>>> at >>>>>> all for most developers on such platforms. its mainly used for early >>>>>> kernel >>>>>> platform data initialisation for system leds. >>>>> If leds-gpio has limitations, please fix those, rather then >>>>> introducing duplicated code. >>>> there is no duplicated code introduced and there is no solution for it. >>>> consider that all wifi drivers with softled support >>>> are going that way with registering a own led driver. see ath9k for >>>> instance. gpio-led cannot be used for it and there is no way to >>>> support multiple platform datas with the same name. its a kernel limitation >>> I just reviewed some mips arch patch adding support for more LEDs for >>> selected devices: >>> [PATCH] MIPS: BCM47XX: Add Luxul XAP1500/XWR1750 WiFi LEDs >>> https://patchwork.linux-mips.org/patch/18681/ >>> >>> It seems to be simply adding another call to the >>> gpio_led_register_device(). It seems to me you can call that function >>> multiple times and register multiple structs with LEDs. >>> >>> Isn't all you need something like this? >>> >>> static const struct gpio_led ath10k_leds[] = { >>> { >>> .name = "ath10k:color:function", >>> .active_low = 1, >>> .default_state = LEDS_GPIO_DEFSTATE_KEEP, >>> } >>> }; >>> >>> static struct gpio_led_platform_data bcm47xx_leds_pdata_extra = { >>> leds = ath10k_leds; >>> num_leds = ARRAY_SIZE(ath10k_leds); >>> }; >>> >>> ath10k_leds.gpio = ar->hw_params.led_pin; >>> gpio_led_register_device(0, &ath10k_leds); >> the problem are other architectures which have already registered gpio_led >> at system start like ar71xx >> you cannot register a second one. so a independend led driver is a >> requirement for direct control > If the limitation indeed exists, please fix the limitation rather than > working around it in each and every driver. see ath9k. its exact the same implementation. in addition my variant does also work without gpiolib support. so it can be used even if the kernel is configured without gpio support. and not to forget, using a own led driver is more ligthweight from the call path for each led on / off event which is important for low performance embedded devices Sebastian > Pavel >
On Thu 2018-03-08 13:33:29, Sebastian Gottschall wrote: > Am 08.03.2018 um 10:02 schrieb Pavel Machek: > >On Wed 2018-03-07 18:54:41, Sebastian Gottschall wrote: > >>Am 07.03.2018 um 17:22 schrieb Rafał Miłecki: > >>>On 2 March 2018 at 10:22, Sebastian Gottschall <s.gottschall@dd-wrt.com> wrote: > >>>>>>leds-gpio is crap and limited. you can just register one platform data at > >>>>>>kernel runtime since its identified by its object name "led-gpio" but the > >>>>>>kernel forbidds to register 2 platform datas with the same name > >>>>>>consider the ar71xx devices with qca988x wifi chipsets. they all have > >>>>>>already a led platform data registered > >>>>>>at boottime. a second can't be registered anymore so gpio_led is useless > >>>>>>at > >>>>>>all for most developers on such platforms. its mainly used for early > >>>>>>kernel > >>>>>>platform data initialisation for system leds. > >>>>>If leds-gpio has limitations, please fix those, rather then > >>>>>introducing duplicated code. > >>>>there is no duplicated code introduced and there is no solution for it. > >>>>consider that all wifi drivers with softled support > >>>>are going that way with registering a own led driver. see ath9k for > >>>>instance. gpio-led cannot be used for it and there is no way to > >>>>support multiple platform datas with the same name. its a kernel limitation > >>>I just reviewed some mips arch patch adding support for more LEDs for > >>>selected devices: > >>>[PATCH] MIPS: BCM47XX: Add Luxul XAP1500/XWR1750 WiFi LEDs > >>>https://patchwork.linux-mips.org/patch/18681/ > >>> > >>>It seems to be simply adding another call to the > >>>gpio_led_register_device(). It seems to me you can call that function > >>>multiple times and register multiple structs with LEDs. > >>> > >>>Isn't all you need something like this? > >>> > >>>static const struct gpio_led ath10k_leds[] = { > >>> { > >>> .name = "ath10k:color:function", > >>> .active_low = 1, > >>> .default_state = LEDS_GPIO_DEFSTATE_KEEP, > >>> } > >>>}; > >>> > >>>static struct gpio_led_platform_data bcm47xx_leds_pdata_extra = { > >>> leds = ath10k_leds; > >>> num_leds = ARRAY_SIZE(ath10k_leds); > >>>}; > >>> > >>>ath10k_leds.gpio = ar->hw_params.led_pin; > >>>gpio_led_register_device(0, &ath10k_leds); > >>the problem are other architectures which have already registered gpio_led > >>at system start like ar71xx > >>you cannot register a second one. so a independend led driver is a > >>requirement for direct control > >If the limitation indeed exists, please fix the limitation rather than > >working around it in each and every driver. > see ath9k. its exact the same implementation. Ok, so one more driver to fix. > in addition my variant does also work without gpiolib support. so it can be > used even if the kernel is configured > without gpio support. > and not to forget, using a own led driver is more ligthweight from the call > path for each led on / off event which is important for > low performance embedded devices We are not going to copy&paste code because such code works without libraries, and we are not going to copy&paste code because that uses less cache during calls. Sorry. NAK. Please fix your patch. Pavel
Pavel Machek <pavel@ucw.cz> writes: >> >>>ath10k_leds.gpio = ar->hw_params.led_pin; >> >>>gpio_led_register_device(0, &ath10k_leds); >> >> >> >>the problem are other architectures which have already registered gpio_led >> >>at system start like ar71xx >> >>you cannot register a second one. so a independend led driver is a >> >>requirement for direct control >> > >> >If the limitation indeed exists, please fix the limitation rather than >> >working around it in each and every driver. >> see ath9k. its exact the same implementation. > > Ok, so one more driver to fix. > >> in addition my variant does also work without gpiolib support. so it >> can be used even if the kernel is configured without gpio support. >> and not to forget, using a own led driver is more ligthweight from >> the call path for each led on / off event which is important for low >> performance embedded devices > > We are not going to copy&paste code because such code works without > libraries, and we are not going to copy&paste code because that uses > less cache during calls. Sorry. > > NAK. Please fix your patch. To me it's not that black and white, sometimes copying code is simpler than trying to bring up complicated or restricting frameworks (talking in general here). I haven't been able to review this patch in detail yet but from a quick look most of the code is about standard ath10k infrastructure code. How many lines of code would using leds-gpio save?
Am 08.03.2018 um 15:05 schrieb Pavel Machek: > On Thu 2018-03-08 13:33:29, Sebastian Gottschall wrote: >> Am 08.03.2018 um 10:02 schrieb Pavel Machek: >>> On Wed 2018-03-07 18:54:41, Sebastian Gottschall wrote: >>>> Am 07.03.2018 um 17:22 schrieb Rafał Miłecki: >>>>> On 2 March 2018 at 10:22, Sebastian Gottschall <s.gottschall@dd-wrt.com> wrote: >>>>>>>> leds-gpio is crap and limited. you can just register one platform data at >>>>>>>> kernel runtime since its identified by its object name "led-gpio" but the >>>>>>>> kernel forbidds to register 2 platform datas with the same name >>>>>>>> consider the ar71xx devices with qca988x wifi chipsets. they all have >>>>>>>> already a led platform data registered >>>>>>>> at boottime. a second can't be registered anymore so gpio_led is useless >>>>>>>> at >>>>>>>> all for most developers on such platforms. its mainly used for early >>>>>>>> kernel >>>>>>>> platform data initialisation for system leds. >>>>>>> If leds-gpio has limitations, please fix those, rather then >>>>>>> introducing duplicated code. >>>>>> there is no duplicated code introduced and there is no solution for it. >>>>>> consider that all wifi drivers with softled support >>>>>> are going that way with registering a own led driver. see ath9k for >>>>>> instance. gpio-led cannot be used for it and there is no way to >>>>>> support multiple platform datas with the same name. its a kernel limitation >>>>> I just reviewed some mips arch patch adding support for more LEDs for >>>>> selected devices: >>>>> [PATCH] MIPS: BCM47XX: Add Luxul XAP1500/XWR1750 WiFi LEDs >>>>> https://patchwork.linux-mips.org/patch/18681/ >>>>> >>>>> It seems to be simply adding another call to the >>>>> gpio_led_register_device(). It seems to me you can call that function >>>>> multiple times and register multiple structs with LEDs. >>>>> >>>>> Isn't all you need something like this? >>>>> >>>>> static const struct gpio_led ath10k_leds[] = { >>>>> { >>>>> .name = "ath10k:color:function", >>>>> .active_low = 1, >>>>> .default_state = LEDS_GPIO_DEFSTATE_KEEP, >>>>> } >>>>> }; >>>>> >>>>> static struct gpio_led_platform_data bcm47xx_leds_pdata_extra = { >>>>> leds = ath10k_leds; >>>>> num_leds = ARRAY_SIZE(ath10k_leds); >>>>> }; >>>>> >>>>> ath10k_leds.gpio = ar->hw_params.led_pin; >>>>> gpio_led_register_device(0, &ath10k_leds); >>>> the problem are other architectures which have already registered gpio_led >>>> at system start like ar71xx >>>> you cannot register a second one. so a independend led driver is a >>>> requirement for direct control >>> If the limitation indeed exists, please fix the limitation rather than >>> working around it in each and every driver. >> see ath9k. its exact the same implementation. > Ok, so one more driver to fix. > >> in addition my variant does also work without gpiolib support. so it can be >> used even if the kernel is configured >> without gpio support. >> and not to forget, using a own led driver is more ligthweight from the call >> path for each led on / off event which is important for >> low performance embedded devices > We are not going to copy&paste code because such code works without > libraries, and we are not going to copy&paste code because that uses > less cache during calls. Sorry. > > NAK. Please fix your patch. show me a proof that its copy & paste. because its not > > Pavel
Am 08.03.2018 um 15:29 schrieb Kalle Valo: > Pavel Machek <pavel@ucw.cz> writes: > >>>>>> ath10k_leds.gpio = ar->hw_params.led_pin; >>>>>> gpio_led_register_device(0, &ath10k_leds); >>>>> the problem are other architectures which have already registered gpio_led >>>>> at system start like ar71xx >>>>> you cannot register a second one. so a independend led driver is a >>>>> requirement for direct control >>>> If the limitation indeed exists, please fix the limitation rather than >>>> working around it in each and every driver. >>> see ath9k. its exact the same implementation. >> Ok, so one more driver to fix. >> >>> in addition my variant does also work without gpiolib support. so it >>> can be used even if the kernel is configured without gpio support. >>> and not to forget, using a own led driver is more ligthweight from >>> the call path for each led on / off event which is important for low >>> performance embedded devices >> We are not going to copy&paste code because such code works without >> libraries, and we are not going to copy&paste code because that uses >> less cache during calls. Sorry. >> >> NAK. Please fix your patch. > To me it's not that black and white, sometimes copying code is simpler > than trying to bring up complicated or restricting frameworks (talking > in general here). > > I haven't been able to review this patch in detail yet but from a quick > look most of the code is about standard ath10k infrastructure code. How > many lines of code would using leds-gpio save? nothing. the led driver code is already very small. (see gpio.c in the patch) i had a leds-gpio implementation but it will not work since a platform data with the name "leds-gpio" does already exist on various platforms (ath79 for instance) so a second leds-gpio can't be registered. so this solution does simply not work and asking me for fixing the kernel generic platform data handling like pavel did, is really out of mind and a own led driver has also better performance than using the leds-gpio->gpiolib->ath10k gpio driver callpath. if someone is still complaining that i added a gpio feature driver as well to my patch, i can simply remove that usefull feature and all would be happy. but i think having access to all gpios of the card instead of just the led gpio makes sense to. thats why i implemented a gpiolib driver as well Sebastian
Hi! > show me a proof that its copy & paste. because its not I don't have to prove you anything. Sorry. But you said: > >>see ath9k. its exact the same implementation. We don't want to have exact same code multiple times in the tree. Pavel
Am 08.03.2018 um 16:04 schrieb Pavel Machek: > Hi! > >> show me a proof that its copy & paste. because its not > I don't have to prove you anything. Sorry. then i will deny your argument because its false. > > But you said: > >>>> see ath9k. its exact the same implementation. > We don't want to have exact same code multiple times in the tree. it isnt the exact same code, its just the way its done. i mean registering a led driver function happens multiple times in the kernel you cannot say that calling led_classdev_register with the required parameters and function implementation is a case of code duplication. then i would just say using of "printk" is a case of code duplication. registering a led driver is nothing unusual, the implementation of the led driver is different each time. the implementation for led_brightness is very different there are many led drivers in the kernel. all are going the same way. i checked the kernel drivers just right now. almost all major wireless drivers are comming with a led driver without using gpiolib your way is a case of codebloating since a registering gpio-leds requires a gpio driver for each wireless driver, even if its sometimes just a single register write for a led and no real gpio. a gpio driver is more complex and bigger than just a led driver. i just wrote a optional gpio driver to get access to all gpios available on the card. some vendors are using these in a unusual way i have seen that vendors used them for reset button input etc. this is why i made it. so dont take this as a argument for going a impossible way (again. the kernel does not support multiple platform datas with the same name THE KERNEL not leds-gpio. so once a leds-gpio platform data is registered, no other driver can register a new one. in addition the kernel must have gpiolib support which increases the kernel size. the best way is always the most simple way and the smallest performant way. and again. i did not duplicate the code of ath9k, i just used it as documentation to write a own led driver in a simple way now a list of wireless drivers with a led driver intersil carl9170 ath5k rt2x00 b43legacy b43 iwlegacy rtl8187 brcmsmac iwlwifi Sebastian > > Pavel
On 2018-03-08 15:05, Pavel Machek wrote: > On Thu 2018-03-08 13:33:29, Sebastian Gottschall wrote: >> Am 08.03.2018 um 10:02 schrieb Pavel Machek: >> >On Wed 2018-03-07 18:54:41, Sebastian Gottschall wrote: >> >>Am 07.03.2018 um 17:22 schrieb Rafał Miłecki: >> >>>On 2 March 2018 at 10:22, Sebastian Gottschall <s.gottschall@dd-wrt.com> wrote: >> >>>>>>leds-gpio is crap and limited. you can just register one platform data at >> >>>>>>kernel runtime since its identified by its object name "led-gpio" but the >> >>>>>>kernel forbidds to register 2 platform datas with the same name >> >>>>>>consider the ar71xx devices with qca988x wifi chipsets. they all have >> >>>>>>already a led platform data registered >> >>>>>>at boottime. a second can't be registered anymore so gpio_led is useless >> >>>>>>at >> >>>>>>all for most developers on such platforms. its mainly used for early >> >>>>>>kernel >> >>>>>>platform data initialisation for system leds. >> >>>>>If leds-gpio has limitations, please fix those, rather then >> >>>>>introducing duplicated code. >> >>>>there is no duplicated code introduced and there is no solution for it. >> >>>>consider that all wifi drivers with softled support >> >>>>are going that way with registering a own led driver. see ath9k for >> >>>>instance. gpio-led cannot be used for it and there is no way to >> >>>>support multiple platform datas with the same name. its a kernel limitation >> >>>I just reviewed some mips arch patch adding support for more LEDs for >> >>>selected devices: >> >>>[PATCH] MIPS: BCM47XX: Add Luxul XAP1500/XWR1750 WiFi LEDs >> >>>https://patchwork.linux-mips.org/patch/18681/ >> >>> >> >>>It seems to be simply adding another call to the >> >>>gpio_led_register_device(). It seems to me you can call that function >> >>>multiple times and register multiple structs with LEDs. >> >>> >> >>>Isn't all you need something like this? >> >>> >> >>>static const struct gpio_led ath10k_leds[] = { >> >>> { >> >>> .name = "ath10k:color:function", >> >>> .active_low = 1, >> >>> .default_state = LEDS_GPIO_DEFSTATE_KEEP, >> >>> } >> >>>}; >> >>> >> >>>static struct gpio_led_platform_data bcm47xx_leds_pdata_extra = { >> >>> leds = ath10k_leds; >> >>> num_leds = ARRAY_SIZE(ath10k_leds); >> >>>}; >> >>> >> >>>ath10k_leds.gpio = ar->hw_params.led_pin; >> >>>gpio_led_register_device(0, &ath10k_leds); >> >>the problem are other architectures which have already registered gpio_led >> >>at system start like ar71xx >> >>you cannot register a second one. so a independend led driver is a >> >>requirement for direct control >> >If the limitation indeed exists, please fix the limitation rather than >> >working around it in each and every driver. >> see ath9k. its exact the same implementation. > > Ok, so one more driver to fix. > >> in addition my variant does also work without gpiolib support. so it can be >> used even if the kernel is configured >> without gpio support. >> and not to forget, using a own led driver is more ligthweight from the call >> path for each led on / off event which is important for >> low performance embedded devices > > We are not going to copy&paste code because such code works without > libraries, and we are not going to copy&paste code because that uses > less cache during calls. Sorry. > > NAK. Please fix your patch. Since this discussion seems to have taken a rather weird turn, I've taken a look at the specific code, and from my point of view the code that sets up the LED (including callback) is so trivial that it's simply not worth dealing with adding the leds-gpio driver to the mix. It adds extra complexity and an extra dependency for no reason at all. There's no extra functionality to be gained by using it. - Felix
Hi Felix, On Fri, Mar 9, 2018 at 2:46 AM, Felix Fietkau <nbd@nbd.name> wrote: > On 2018-03-08 15:05, Pavel Machek wrote: >> On Thu 2018-03-08 13:33:29, Sebastian Gottschall wrote: >>> Am 08.03.2018 um 10:02 schrieb Pavel Machek: >>> >On Wed 2018-03-07 18:54:41, Sebastian Gottschall wrote: >>> >>Am 07.03.2018 um 17:22 schrieb Rafał Miłecki: >>> >>>On 2 March 2018 at 10:22, Sebastian Gottschall <s.gottschall@dd-wrt.com> wrote: >>> >>>>>>leds-gpio is crap and limited. you can just register one platform data at >>> >>>>>>kernel runtime since its identified by its object name "led-gpio" but the >>> >>>>>>kernel forbidds to register 2 platform datas with the same name >>> >>>>>>consider the ar71xx devices with qca988x wifi chipsets. they all have >>> >>>>>>already a led platform data registered >>> >>>>>>at boottime. a second can't be registered anymore so gpio_led is useless >>> >>>>>>at >>> >>>>>>all for most developers on such platforms. its mainly used for early >>> >>>>>>kernel >>> >>>>>>platform data initialisation for system leds. >>> >>>>>If leds-gpio has limitations, please fix those, rather then >>> >>>>>introducing duplicated code. >>> >>>>there is no duplicated code introduced and there is no solution for it. >>> >>>>consider that all wifi drivers with softled support >>> >>>>are going that way with registering a own led driver. see ath9k for >>> >>>>instance. gpio-led cannot be used for it and there is no way to >>> >>>>support multiple platform datas with the same name. its a kernel limitation >>> >>>I just reviewed some mips arch patch adding support for more LEDs for >>> >>>selected devices: >>> >>>[PATCH] MIPS: BCM47XX: Add Luxul XAP1500/XWR1750 WiFi LEDs >>> >>>https://patchwork.linux-mips.org/patch/18681/ >>> >>> >>> >>>It seems to be simply adding another call to the >>> >>>gpio_led_register_device(). It seems to me you can call that function >>> >>>multiple times and register multiple structs with LEDs. >>> >>> >>> >>>Isn't all you need something like this? >>> >>> >>> >>>static const struct gpio_led ath10k_leds[] = { >>> >>> { >>> >>> .name = "ath10k:color:function", >>> >>> .active_low = 1, >>> >>> .default_state = LEDS_GPIO_DEFSTATE_KEEP, >>> >>> } >>> >>>}; >>> >>> >>> >>>static struct gpio_led_platform_data bcm47xx_leds_pdata_extra = { >>> >>> leds = ath10k_leds; >>> >>> num_leds = ARRAY_SIZE(ath10k_leds); >>> >>>}; >>> >>> >>> >>>ath10k_leds.gpio = ar->hw_params.led_pin; >>> >>>gpio_led_register_device(0, &ath10k_leds); >>> >>the problem are other architectures which have already registered gpio_led >>> >>at system start like ar71xx >>> >>you cannot register a second one. so a independend led driver is a >>> >>requirement for direct control >>> >If the limitation indeed exists, please fix the limitation rather than >>> >working around it in each and every driver. >>> see ath9k. its exact the same implementation. >> >> Ok, so one more driver to fix. >> >>> in addition my variant does also work without gpiolib support. so it can be >>> used even if the kernel is configured >>> without gpio support. >>> and not to forget, using a own led driver is more ligthweight from the call >>> path for each led on / off event which is important for >>> low performance embedded devices >> >> We are not going to copy&paste code because such code works without >> libraries, and we are not going to copy&paste code because that uses >> less cache during calls. Sorry. >> >> NAK. Please fix your patch. Since this discussion seems to have taken a rather weird turn, I've > taken a look at the specific code, and from my point of view the code > that sets up the LED (including callback) is so trivial that it's simply > not worth dealing with adding the leds-gpio driver to the mix. > It adds extra complexity and an extra dependency for no reason at all. > There's no extra functionality to be gained by using it. Stupid question: If the LED driver isn't using the GPIO subsystem (when enabled), what happens if the user uses the GPIO subsystem to fiddle with the pin the LED is connected to? Thanks,
>> taken a look at the specific code, and from my point of view the code >> that sets up the LED (including callback) is so trivial that it's simply >> not worth dealing with adding the leds-gpio driver to the mix. >> It adds extra complexity and an extra dependency for no reason at all. >> There's no extra functionality to be gained by using it. > Stupid question: If the LED driver isn't using the GPIO subsystem > (when enabled), what happens if the user uses the GPIO subsystem to > fiddle with the pin the LED is connected to? in our case here it can coexist and will not have a negative effect since the led will still run. (except if you reconfigure the gpio to input) for sure it makes no sense. but however i can block the gpio for beeing reused if the led driver took it. this has been made in ath9k for instance. in ath10k i did not do it, since there is no technical requirement for it. but that might not be the case for all gpio controllers, so there is no generic answer. for ath10k. harmless Sebastian > > Thanks, >
10.03.2018 08:56, Sebastian Gottschall: > >>> taken a look at the specific code, and from my point of view the code >>> that sets up the LED (including callback) is so trivial that it's simply >>> not worth dealing with adding the leds-gpio driver to the mix. >>> It adds extra complexity and an extra dependency for no reason at all. >>> There's no extra functionality to be gained by using it. >> Stupid question: If the LED driver isn't using the GPIO subsystem >> (when enabled), what happens if the user uses the GPIO subsystem to >> fiddle with the pin the LED is connected to? > > in our case here it can coexist and will not have a negative effect > since the led will still run. (except if you reconfigure the gpio to input) > for sure it makes no sense. Well, it will have negative effects as I noticed in OpenWrt. If the same GPIO is controlled by the internal LED function it can't be really used via the GPIO controller as there will be "bogus" changes of the GPIO pin state. > but however i can block the gpio for beeing reused if the led driver > took it. this has been made in ath9k for instance. Most likely I'm not aware of the upstream code you are talking about. But OpenWrt has a custom patch which registers the ath9k pins as GPIO controller. As soon as there are pins, manufactures will use them and the assumption about a default led connected to a specific pin will fail. I've seen (home)routers routers using the "default" ath9k LED pin as button or for LEDs with a different purpose than "wireless". I never was able to find any kind of information which explains why exactly this specific pin is used for the ath9k-led. But that's another story. My solution for OpenWrt was a patch which prevents the creation of the ath9k-led if the "default" pin is used as GPIO. If mach files are used, the ath9k led isn't created (ath9k led pin is set to -1) in case the platform data have a button or led using the same pin. If the device tree is used, the ath9k led isn't created (ath9k led pin is set to -1) if the devicetree node for the ath9k device has the gpio-controller property. I'm not really proud of the code and it can be most likely done better, but it fixes the issue outlined above. I've done it this way since the use of the GPIO controller should always override any kind of default LEDs. In the end ath[0|10]k-led is only required for hardware configurations where the wireless isn't fixed like with miniPCIe, USB ... wireless cards. If the configuration is fixed - like it is for most of the (home)routers - the GPIO controller can be registered via the devicetree and gpio-leds can be used. Something similar can be most likely done via mach files (I barely touch mach files). I'm aware of the issue since the first version of your patch. But since I'm very interested in getting the ath10k pins exposed as gpio controller, I planned to add a workaround similar to what we have for ath9k to OpenWrt. Mathias
Am 12.03.2018 um 08:53 schrieb Mathias Kresin: > 10.03.2018 08:56, Sebastian Gottschall: >> >>>> taken a look at the specific code, and from my point of view the code >>>> that sets up the LED (including callback) is so trivial that it's >>>> simply >>>> not worth dealing with adding the leds-gpio driver to the mix. >>>> It adds extra complexity and an extra dependency for no reason at all. >>>> There's no extra functionality to be gained by using it. >>> Stupid question: If the LED driver isn't using the GPIO subsystem >>> (when enabled), what happens if the user uses the GPIO subsystem to >>> fiddle with the pin the LED is connected to? >> >> in our case here it can coexist and will not have a negative effect >> since the led will still run. (except if you reconfigure the gpio to >> input) >> for sure it makes no sense. > > Well, it will have negative effects as I noticed in OpenWrt. If the > same GPIO is controlled by the internal LED function it can't be > really used via the GPIO controller as there will be "bogus" changes > of the GPIO pin state. yes, it will have bogus changes, but it will not raise any other issues than the bogus changes your forcefully triggered. why disallowing something, if there is no reason for. but i give you a example for a use. if the wifi interface is down or not used, you can still take control about the led. so set the led to any state you want, no matter if the trigger is still installed > >> but however i can block the gpio for beeing reused if the led driver >> took it. this has been made in ath9k for instance. > > Most likely I'm not aware of the upstream code you are talking about. > But OpenWrt has a custom patch which registers the ath9k pins as GPIO > controller. thats the code i'm talking about, but as far as i know this code is upstream. > > As soon as there are pins, manufactures will use them and the > assumption about a default led connected to a specific pin will fail. nope. manufactures are mainly using the qca propertiery driver and this qca driver uses fixed hardcoded led pins. 1 for qca988x and 17 for everything else. the led pins i assigned are following the qca specification for this chipsets and not to forget i tested it on many devices > > I've seen (home)routers routers using the "default" ath9k LED pin as > button or for LEDs with a different purpose than "wireless". I never > was able to find any kind of information which explains why exactly > this specific pin is used for the ath9k-led. But that's another story. i know. but thats not the case for ath10k so far and i'm following exact the same way qca is doing by themself. i just know 2 cases. vendors to use the led pin of the chipset or they dont and use standard system leds with any assignment and in all cases with no exception the default led pin was used. > > My solution for OpenWrt was a patch which prevents the creation of the > ath9k-led if the "default" pin is used as GPIO. i have seen that code, but decided no to follow it since any developer is responsible for configuring whatever he wants. if it doesnt lead to a crash i see no reason for preventing it. but thats my philosophy. > > If mach files are used, the ath9k led isn't created (ath9k led pin is > set to -1) in case the platform data have a button or led using the > same pin. If the device tree is used, the ath9k led isn't created > (ath9k led pin is set to -1) if the devicetree node for the ath9k > device has the gpio-controller property. I'm not really proud of the > code and it can be most likely done better, but it fixes the issue > outlined above. sounds very inflexible or better "intransparent". from the black box view its hard to understand how its handled without reading the code. its has no clean logic. and your issue above is no issue. if a dev wants to make stupid things, let him do. he learns from his misstakes and he does not harm the hardware nor the stability. there is no reason for extra hyper security barriers to guide someone how todo it. but here are samples to reuse a gpio from 2 points as i explained at the beginning. i could imagine todo a manual error blinking or something if a interface is down. not sure, but other people might be more creative here. > > I've done it this way since the use of the GPIO controller should > always override any kind of default LEDs. > > In the end ath[0|10]k-led is only required for hardware configurations > where the wireless isn't fixed like with miniPCIe, USB ... wireless > cards. If the configuration is fixed - like it is for most of the > (home)routers - the GPIO controller can be registered via the > devicetree and gpio-leds can be used. Something similar can be most > likely done via mach files (I barely touch mach files). ath79 uses mach files and no dts yet. qca ipq platforms are using dts. and i have a mikrotik minipcie cards with wireless leds :-) all 3 ways are valid. and i have to note that i wrote that patch for embedded devices like openwrt does. this code is a requirement to get the leds to work on several devices supported by openwrt. > > I'm aware of the issue since the first version of your patch. But > since I'm very interested in getting the ath10k pins exposed as gpio > controller, I planned to add a workaround similar to what we have for > ath9k to OpenWrt. feel free to make any addition patch. but consider that the driver already contains a gpio controler. but in that case just avoid using of the led trigger. but for universal use the gpio controler isnt enough unfortunatly. (ath79 for instance) i mean creating a additional platformdata for ath10k to initialize the pin in mach files is really not usefull if it can be done from userspace in sysfs > > Mathias >
s.gottschall@dd-wrt.com writes: > 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> [...] > @@ -1034,7 +1068,7 @@ ath10k_wmi_pdev_get_temperature(struct ath10k *ar) > if (IS_ERR(skb)) > return PTR_ERR(skb); > > - return ath10k_wmi_cmd_send(ar, skb, > + return ath10k_wmi_cmd_send_nowait(ar, skb, > ar->wmi.cmd->pdev_get_temperature_cmdid); > } This looks odd, I don't think it belongs to this patch. Also you made a some sort of record, your patch had 181 checkpatch warnings! Do you use Word as your editor or what? :) But please do check your editor settings and read the coding style documents. https://wireless.wiki.kernel.org/en/users/drivers/ath10k/codingstyle I'll send v13. Here are the warnings from ath10k-check: drivers/net/wireless/ath/ath10k/gpio.c:19: Please don't use multiple blank lines drivers/net/wireless/ath/ath10k/gpio.c:30: Please don't use multiple blank lines drivers/net/wireless/ath/ath10k/gpio.c:36: Prefer 'unsigned int' to bare use of 'unsigned' drivers/net/wireless/ath/ath10k/gpio.c:38: line over 90 characters drivers/net/wireless/ath/ath10k/gpio.c:39: line over 90 characters drivers/net/wireless/ath/ath10k/gpio.c:39: Missing a blank line after declarations drivers/net/wireless/ath/ath10k/gpio.c:45: Prefer 'unsigned int' to bare use of 'unsigned' drivers/net/wireless/ath/ath10k/gpio.c:46: Alignment should match open parenthesis drivers/net/wireless/ath/ath10k/gpio.c:48: line over 90 characters drivers/net/wireless/ath/ath10k/gpio.c:50: line over 90 characters drivers/net/wireless/ath/ath10k/gpio.c:58: Prefer 'unsigned int' to bare use of 'unsigned' drivers/net/wireless/ath/ath10k/gpio.c:60: line over 90 characters drivers/net/wireless/ath/ath10k/gpio.c:66: Prefer 'unsigned int' to bare use of 'unsigned' drivers/net/wireless/ath/ath10k/gpio.c:68: line over 90 characters drivers/net/wireless/ath/ath10k/gpio.c:74: Prefer 'unsigned int' to bare use of 'unsigned' drivers/net/wireless/ath/ath10k/gpio.c:75: Alignment should match open parenthesis drivers/net/wireless/ath/ath10k/gpio.c:77: line over 90 characters drivers/net/wireless/ath/ath10k/gpio.c:86: trailing whitespace drivers/net/wireless/ath/ath10k/gpio.c:86: please, no spaces at the start of a line drivers/net/wireless/ath/ath10k/gpio.c:86: Blank lines aren't necessary after an open brace '{' drivers/net/wireless/ath/ath10k/gpio.c:88: Missing a blank line after declarations drivers/net/wireless/ath/ath10k/gpio.c:88: braces {} are not necessary for single statement blocks drivers/net/wireless/ath/ath10k/gpio.c:113: trailing whitespace drivers/net/wireless/ath/ath10k/gpio.c:114: Missing a blank line after declarations drivers/net/wireless/ath/ath10k/gpio.c:114: braces {} are not necessary for single statement blocks drivers/net/wireless/ath/ath10k/gpio.c:121: line over 90 characters drivers/net/wireless/ath/ath10k/gpio.c:130: Please don't use multiple blank lines drivers/net/wireless/ath/ath10k/gpio.c:132: Alignment should match open parenthesis drivers/net/wireless/ath/ath10k/gpio.c:134: line over 90 characters drivers/net/wireless/ath/ath10k/gpio.c:136: Missing a blank line after declarations drivers/net/wireless/ath/ath10k/gpio.c:145: trailing whitespace drivers/net/wireless/ath/ath10k/gpio.c:146: Missing a blank line after declarations drivers/net/wireless/ath/ath10k/gpio.c:159: trailing whitespace drivers/net/wireless/ath/ath10k/gpio.c:160: Missing a blank line after declarations drivers/net/wireless/ath/ath10k/gpio.c:166: trailing whitespace drivers/net/wireless/ath/ath10k/gpio.c:169: trailing whitespace drivers/net/wireless/ath/ath10k/gpio.c:170: line over 90 characters drivers/net/wireless/ath/ath10k/gpio.c:178: Missing a blank line after declarations drivers/net/wireless/ath/ath10k/gpio.c:181: Prefer kzalloc(sizeof(*gpio)...) over kzalloc(sizeof(struct ath10k_gpiocontrol)...) drivers/net/wireless/ath/ath10k/gpio.c:182: braces {} are not necessary for single statement blocks drivers/net/wireless/ath/ath10k/gpio.c:192: trailing whitespace drivers/net/wireless/ath/ath10k/gpio.c:194: line over 90 characters drivers/net/wireless/ath/ath10k/gpio.c:196: trailing whitespace drivers/net/wireless/ath/ath10k/core.h:865: trailing whitespace drivers/net/wireless/ath/ath10k/core.h:865: line over 90 characters drivers/net/wireless/ath/ath10k/core.h:871: trailing whitespace drivers/net/wireless/ath/ath10k/core.h:890: Blank lines aren't necessary after an open brace '{' drivers/net/wireless/ath/ath10k/core.h:891: Blank lines aren't necessary before a close brace '}' drivers/net/wireless/ath/ath10k/core.h:892: Please use a blank line after function/struct/union/enum declarations drivers/net/wireless/ath/ath10k/core.h:894: Blank lines aren't necessary after an open brace '{' drivers/net/wireless/ath/ath10k/core.h:895: Blank lines aren't necessary before a close brace '}' drivers/net/wireless/ath/ath10k/core.h:896: Please use a blank line after function/struct/union/enum declarations drivers/net/wireless/ath/ath10k/core.h:905: trailing whitespace drivers/net/wireless/ath/ath10k/core.h:907: Blank lines aren't necessary after an open brace '{' drivers/net/wireless/ath/ath10k/core.h:908: Blank lines aren't necessary before a close brace '}' drivers/net/wireless/ath/ath10k/core.c:29: Please don't use multiple blank lines drivers/net/wireless/ath/ath10k/core.c:74: trailing whitespace drivers/net/wireless/ath/ath10k/core.c:105: trailing whitespace drivers/net/wireless/ath/ath10k/core.c:136: trailing whitespace drivers/net/wireless/ath/ath10k/core.c:282: trailing whitespace drivers/net/wireless/ath/ath10k/core.c:318: trailing whitespace drivers/net/wireless/ath/ath10k/core.c:359: trailing whitespace drivers/net/wireless/ath/ath10k/core.c:2396: trailing whitespace drivers/net/wireless/ath/ath10k/core.c:2396: Please don't use multiple blank lines drivers/net/wireless/ath/ath10k/core.c:2403: trailing whitespace drivers/net/wireless/ath/ath10k/debug.c:1089: Alignment should match open parenthesis drivers/net/wireless/ath/ath10k/debug.c:1092: trailing whitespace drivers/net/wireless/ath/ath10k/debug.c:1095: Missing a blank line after declarations drivers/net/wireless/ath/ath10k/debug.c:1098: line over 90 characters drivers/net/wireless/ath/ath10k/debug.c:1103: Please don't use multiple blank lines drivers/net/wireless/ath/ath10k/debug.c:1105: Alignment should match open parenthesis drivers/net/wireless/ath/ath10k/debug.c:1109: trailing whitespace drivers/net/wireless/ath/ath10k/debug.c:1113: Missing a blank line after declarations drivers/net/wireless/ath/ath10k/debug.c:1130: Please don't use multiple blank lines drivers/net/wireless/ath/ath10k/debug.c:1133: Please don't use multiple blank lines drivers/net/wireless/ath/ath10k/debug.c:1149: Alignment should match open parenthesis drivers/net/wireless/ath/ath10k/debug.c:1152: trailing whitespace drivers/net/wireless/ath/ath10k/debug.c:1155: Missing a blank line after declarations drivers/net/wireless/ath/ath10k/debug.c:1163: Please don't use multiple blank lines drivers/net/wireless/ath/ath10k/debug.c:1165: Alignment should match open parenthesis drivers/net/wireless/ath/ath10k/debug.c:1169: trailing whitespace drivers/net/wireless/ath/ath10k/debug.c:1173: Missing a blank line after declarations drivers/net/wireless/ath/ath10k/debug.c:1188: Please don't use multiple blank lines drivers/net/wireless/ath/ath10k/debug.c:1191: Please don't use multiple blank lines drivers/net/wireless/ath/ath10k/debug.c:1375: Please don't use multiple blank lines drivers/net/wireless/ath/ath10k/wmi-ops.h:211: Please don't use multiple blank lines drivers/net/wireless/ath/ath10k/wmi-ops.h:986: trailing whitespace drivers/net/wireless/ath/ath10k/wmi-ops.h:986: Please use a blank line after function/struct/union/enum declarations drivers/net/wireless/ath/ath10k/wmi-ops.h:987: line over 90 characters drivers/net/wireless/ath/ath10k/wmi-ops.h:1001: trailing whitespace drivers/net/wireless/ath/ath10k/wmi-tlv.c:3340: line over 90 characters drivers/net/wireless/ath/ath10k/wmi-tlv.c:3342: please, no spaces at the start of a line drivers/net/wireless/ath/ath10k/wmi-tlv.c:3343: please, no spaces at the start of a line drivers/net/wireless/ath/ath10k/wmi-tlv.c:3344: please, no spaces at the start of a line drivers/net/wireless/ath/ath10k/wmi-tlv.c:3345: trailing whitespace drivers/net/wireless/ath/ath10k/wmi-tlv.c:3345: please, no spaces at the start of a line drivers/net/wireless/ath/ath10k/wmi-tlv.c:3346: please, no spaces at the start of a line drivers/net/wireless/ath/ath10k/wmi-tlv.c:3348: please, no spaces at the start of a line drivers/net/wireless/ath/ath10k/wmi-tlv.c:3349: please, no spaces at the start of a line drivers/net/wireless/ath/ath10k/wmi-tlv.c:3350: please, no spaces at the start of a line drivers/net/wireless/ath/ath10k/wmi-tlv.c:3353: please, no spaces at the start of a line drivers/net/wireless/ath/ath10k/wmi-tlv.c:3354: please, no spaces at the start of a line drivers/net/wireless/ath/ath10k/wmi-tlv.c:3355: please, no spaces at the start of a line drivers/net/wireless/ath/ath10k/wmi-tlv.c:3356: please, no spaces at the start of a line drivers/net/wireless/ath/ath10k/wmi-tlv.c:3358: please, no spaces at the start of a line drivers/net/wireless/ath/ath10k/wmi-tlv.c:3359: please, no spaces at the start of a line drivers/net/wireless/ath/ath10k/wmi-tlv.c:3360: please, no spaces at the start of a line drivers/net/wireless/ath/ath10k/wmi-tlv.c:3361: please, no spaces at the start of a line drivers/net/wireless/ath/ath10k/wmi-tlv.c:3362: please, no spaces at the start of a line drivers/net/wireless/ath/ath10k/wmi-tlv.c:3364: please, no spaces at the start of a line drivers/net/wireless/ath/ath10k/wmi-tlv.c:3365: please, no spaces at the start of a line drivers/net/wireless/ath/ath10k/wmi-tlv.c:3367: line over 90 characters drivers/net/wireless/ath/ath10k/wmi-tlv.c:3367: please, no spaces at the start of a line drivers/net/wireless/ath/ath10k/wmi-tlv.c:3368: please, no spaces at the start of a line drivers/net/wireless/ath/ath10k/wmi-tlv.c:3374: please, no spaces at the start of a line drivers/net/wireless/ath/ath10k/wmi-tlv.c:3375: please, no spaces at the start of a line drivers/net/wireless/ath/ath10k/wmi-tlv.c:3376: please, no spaces at the start of a line drivers/net/wireless/ath/ath10k/wmi-tlv.c:3377: trailing whitespace drivers/net/wireless/ath/ath10k/wmi-tlv.c:3377: please, no spaces at the start of a line drivers/net/wireless/ath/ath10k/wmi-tlv.c:3378: please, no spaces at the start of a line drivers/net/wireless/ath/ath10k/wmi-tlv.c:3380: please, no spaces at the start of a line drivers/net/wireless/ath/ath10k/wmi-tlv.c:3381: please, no spaces at the start of a line drivers/net/wireless/ath/ath10k/wmi-tlv.c:3382: please, no spaces at the start of a line drivers/net/wireless/ath/ath10k/wmi-tlv.c:3385: please, no spaces at the start of a line drivers/net/wireless/ath/ath10k/wmi-tlv.c:3386: please, no spaces at the start of a line drivers/net/wireless/ath/ath10k/wmi-tlv.c:3387: please, no spaces at the start of a line drivers/net/wireless/ath/ath10k/wmi-tlv.c:3388: please, no spaces at the start of a line drivers/net/wireless/ath/ath10k/wmi-tlv.c:3390: please, no spaces at the start of a line drivers/net/wireless/ath/ath10k/wmi-tlv.c:3391: please, no spaces at the start of a line drivers/net/wireless/ath/ath10k/wmi-tlv.c:3392: please, no spaces at the start of a line drivers/net/wireless/ath/ath10k/wmi-tlv.c:3394: please, no spaces at the start of a line drivers/net/wireless/ath/ath10k/wmi-tlv.c:3395: please, no spaces at the start of a line drivers/net/wireless/ath/ath10k/wmi-tlv.c:3397: line over 90 characters drivers/net/wireless/ath/ath10k/wmi-tlv.c:3397: please, no spaces at the start of a line drivers/net/wireless/ath/ath10k/wmi-tlv.c:3398: please, no spaces at the start of a line drivers/net/wireless/ath/ath10k/wmi-tlv.c:3401: Please don't use multiple blank lines drivers/net/wireless/ath/ath10k/mac.c:4622: trailing whitespace drivers/net/wireless/ath/ath10k/mac.c:4623: line over 90 characters drivers/net/wireless/ath/ath10k/mac.c:4625: trailing whitespace drivers/net/wireless/ath/ath10k/mac.c:4626: trailing whitespace drivers/net/wireless/ath/ath10k/wmi.h:2934: please, no spaces at the start of a line drivers/net/wireless/ath/ath10k/wmi.h:2935: please, no spaces at the start of a line drivers/net/wireless/ath/ath10k/wmi.h:2936: please, no spaces at the start of a line drivers/net/wireless/ath/ath10k/wmi.h:2940: please, no spaces at the start of a line drivers/net/wireless/ath/ath10k/wmi.h:2941: please, no spaces at the start of a line drivers/net/wireless/ath/ath10k/wmi.h:2942: please, no spaces at the start of a line drivers/net/wireless/ath/ath10k/wmi.h:2943: please, no spaces at the start of a line drivers/net/wireless/ath/ath10k/wmi.h:2944: please, no spaces at the start of a line drivers/net/wireless/ath/ath10k/wmi.h:2945: please, no spaces at the start of a line drivers/net/wireless/ath/ath10k/wmi.h:2950: please, no spaces at the start of a line drivers/net/wireless/ath/ath10k/wmi.h:2951: please, no spaces at the start of a line drivers/net/wireless/ath/ath10k/wmi.h:2952: please, no spaces at the start of a line drivers/net/wireless/ath/ath10k/wmi.h:2953: please, no spaces at the start of a line drivers/net/wireless/ath/ath10k/wmi.h:2958: please, no spaces at the start of a line drivers/net/wireless/ath/ath10k/wmi.h:2959: please, no spaces at the start of a line drivers/net/wireless/ath/ath10k/wmi.h:2964: please, no spaces at the start of a line drivers/net/wireless/ath/ath10k/wmi.h:2967: Please don't use multiple blank lines drivers/net/wireless/ath/ath10k/wmi.c:6957: line over 90 characters drivers/net/wireless/ath/ath10k/wmi.c:6959: please, no spaces at the start of a line drivers/net/wireless/ath/ath10k/wmi.c:6960: please, no spaces at the start of a line drivers/net/wireless/ath/ath10k/wmi.c:6962: please, no spaces at the start of a line drivers/net/wireless/ath/ath10k/wmi.c:6963: please, no spaces at the start of a line drivers/net/wireless/ath/ath10k/wmi.c:6965: please, no spaces at the start of a line drivers/net/wireless/ath/ath10k/wmi.c:6966: please, no spaces at the start of a line drivers/net/wireless/ath/ath10k/wmi.c:6967: please, no spaces at the start of a line drivers/net/wireless/ath/ath10k/wmi.c:6968: please, no spaces at the start of a line drivers/net/wireless/ath/ath10k/wmi.c:6969: please, no spaces at the start of a line drivers/net/wireless/ath/ath10k/wmi.c:6971: line over 90 characters drivers/net/wireless/ath/ath10k/wmi.c:6971: please, no spaces at the start of a line drivers/net/wireless/ath/ath10k/wmi.c:6972: please, no spaces at the start of a line drivers/net/wireless/ath/ath10k/wmi.c:6978: please, no spaces at the start of a line drivers/net/wireless/ath/ath10k/wmi.c:6979: please, no spaces at the start of a line drivers/net/wireless/ath/ath10k/wmi.c:6981: please, no spaces at the start of a line drivers/net/wireless/ath/ath10k/wmi.c:6982: please, no spaces at the start of a line drivers/net/wireless/ath/ath10k/wmi.c:6984: please, no spaces at the start of a line drivers/net/wireless/ath/ath10k/wmi.c:6985: please, no spaces at the start of a line drivers/net/wireless/ath/ath10k/wmi.c:6986: please, no spaces at the start of a line drivers/net/wireless/ath/ath10k/wmi.c:6987: line over 90 characters drivers/net/wireless/ath/ath10k/wmi.c:6987: please, no spaces at the start of a line drivers/net/wireless/ath/ath10k/wmi.c:6988: please, no spaces at the start of a line drivers/net/wireless/ath/ath10k/wmi.c:8583: trailing whitespace
Am 05.04.2018 um 16:44 schrieb Kalle Valo: > s.gottschall@dd-wrt.com writes: > >> 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> > [...] > >> @@ -1034,7 +1068,7 @@ ath10k_wmi_pdev_get_temperature(struct ath10k *ar) >> if (IS_ERR(skb)) >> return PTR_ERR(skb); >> - return ath10k_wmi_cmd_send(ar, skb, >> + return ath10k_wmi_cmd_send_nowait(ar, skb, >> ar->wmi.cmd->pdev_get_temperature_cmdid); >> } > This looks odd, I don't think it belongs to this patch. thats true. but due the nature of this function i found it better to use nowait here. better if i split it up? > > Also you made a some sort of record, your patch had 181 checkpatch > warnings! Do you use Word as your editor or what? But please do check > your editor settings and read the coding style documents. no? i use midnight commander for all of my code since more than 20 years and its the first time that i see such warnings. is there any special coding rule for ath10k which differs from the kernel rules? and where is ath10k-check located? > > https://wireless.wiki.kernel.org/en/users/drivers/ath10k/codingstyle > > I'll send v13. > > Here are the warnings from ath10k-check: > > drivers/net/wireless/ath/ath10k/gpio.c:19: Please don't use multiple > blank lines > drivers/net/wireless/ath/ath10k/gpio.c:30: Please don't use multiple > blank lines > drivers/net/wireless/ath/ath10k/gpio.c:36: Prefer 'unsigned int' to > bare use of 'unsigned' > drivers/net/wireless/ath/ath10k/gpio.c:38: line over 90 characters > drivers/net/wireless/ath/ath10k/gpio.c:39: line over 90 characters > drivers/net/wireless/ath/ath10k/gpio.c:39: Missing a blank line after > declarations > drivers/net/wireless/ath/ath10k/gpio.c:45: Prefer 'unsigned int' to > bare use of 'unsigned' > drivers/net/wireless/ath/ath10k/gpio.c:46: Alignment should match open > parenthesis > drivers/net/wireless/ath/ath10k/gpio.c:48: line over 90 characters > drivers/net/wireless/ath/ath10k/gpio.c:50: line over 90 characters > drivers/net/wireless/ath/ath10k/gpio.c:58: Prefer 'unsigned int' to > bare use of 'unsigned' > drivers/net/wireless/ath/ath10k/gpio.c:60: line over 90 characters > drivers/net/wireless/ath/ath10k/gpio.c:66: Prefer 'unsigned int' to > bare use of 'unsigned' > drivers/net/wireless/ath/ath10k/gpio.c:68: line over 90 characters > drivers/net/wireless/ath/ath10k/gpio.c:74: Prefer 'unsigned int' to > bare use of 'unsigned' > drivers/net/wireless/ath/ath10k/gpio.c:75: Alignment should match open > parenthesis > drivers/net/wireless/ath/ath10k/gpio.c:77: line over 90 characters > drivers/net/wireless/ath/ath10k/gpio.c:86: trailing whitespace > drivers/net/wireless/ath/ath10k/gpio.c:86: please, no spaces at the > start of a line > drivers/net/wireless/ath/ath10k/gpio.c:86: Blank lines aren't > necessary after an open brace '{' > drivers/net/wireless/ath/ath10k/gpio.c:88: Missing a blank line after > declarations > drivers/net/wireless/ath/ath10k/gpio.c:88: braces {} are not necessary > for single statement blocks > drivers/net/wireless/ath/ath10k/gpio.c:113: trailing whitespace > drivers/net/wireless/ath/ath10k/gpio.c:114: Missing a blank line after > declarations > drivers/net/wireless/ath/ath10k/gpio.c:114: braces {} are not > necessary for single statement blocks > drivers/net/wireless/ath/ath10k/gpio.c:121: line over 90 characters > drivers/net/wireless/ath/ath10k/gpio.c:130: Please don't use multiple > blank lines > drivers/net/wireless/ath/ath10k/gpio.c:132: Alignment should match > open parenthesis > drivers/net/wireless/ath/ath10k/gpio.c:134: line over 90 characters > drivers/net/wireless/ath/ath10k/gpio.c:136: Missing a blank line after > declarations > drivers/net/wireless/ath/ath10k/gpio.c:145: trailing whitespace > drivers/net/wireless/ath/ath10k/gpio.c:146: Missing a blank line after > declarations > drivers/net/wireless/ath/ath10k/gpio.c:159: trailing whitespace > drivers/net/wireless/ath/ath10k/gpio.c:160: Missing a blank line after > declarations > drivers/net/wireless/ath/ath10k/gpio.c:166: trailing whitespace > drivers/net/wireless/ath/ath10k/gpio.c:169: trailing whitespace > drivers/net/wireless/ath/ath10k/gpio.c:170: line over 90 characters > drivers/net/wireless/ath/ath10k/gpio.c:178: Missing a blank line after > declarations > drivers/net/wireless/ath/ath10k/gpio.c:181: Prefer > kzalloc(sizeof(*gpio)...) over kzalloc(sizeof(struct > ath10k_gpiocontrol)...) > drivers/net/wireless/ath/ath10k/gpio.c:182: braces {} are not > necessary for single statement blocks > drivers/net/wireless/ath/ath10k/gpio.c:192: trailing whitespace > drivers/net/wireless/ath/ath10k/gpio.c:194: line over 90 characters > drivers/net/wireless/ath/ath10k/gpio.c:196: trailing whitespace > drivers/net/wireless/ath/ath10k/core.h:865: trailing whitespace > drivers/net/wireless/ath/ath10k/core.h:865: line over 90 characters > drivers/net/wireless/ath/ath10k/core.h:871: trailing whitespace > drivers/net/wireless/ath/ath10k/core.h:890: Blank lines aren't > necessary after an open brace '{' > drivers/net/wireless/ath/ath10k/core.h:891: Blank lines aren't > necessary before a close brace '}' > drivers/net/wireless/ath/ath10k/core.h:892: Please use a blank line > after function/struct/union/enum declarations > drivers/net/wireless/ath/ath10k/core.h:894: Blank lines aren't > necessary after an open brace '{' > drivers/net/wireless/ath/ath10k/core.h:895: Blank lines aren't > necessary before a close brace '}' > drivers/net/wireless/ath/ath10k/core.h:896: Please use a blank line > after function/struct/union/enum declarations > drivers/net/wireless/ath/ath10k/core.h:905: trailing whitespace > drivers/net/wireless/ath/ath10k/core.h:907: Blank lines aren't > necessary after an open brace '{' > drivers/net/wireless/ath/ath10k/core.h:908: Blank lines aren't > necessary before a close brace '}' > drivers/net/wireless/ath/ath10k/core.c:29: Please don't use multiple > blank lines > drivers/net/wireless/ath/ath10k/core.c:74: trailing whitespace > drivers/net/wireless/ath/ath10k/core.c:105: trailing whitespace > drivers/net/wireless/ath/ath10k/core.c:136: trailing whitespace > drivers/net/wireless/ath/ath10k/core.c:282: trailing whitespace > drivers/net/wireless/ath/ath10k/core.c:318: trailing whitespace > drivers/net/wireless/ath/ath10k/core.c:359: trailing whitespace > drivers/net/wireless/ath/ath10k/core.c:2396: trailing whitespace > drivers/net/wireless/ath/ath10k/core.c:2396: Please don't use multiple > blank lines > drivers/net/wireless/ath/ath10k/core.c:2403: trailing whitespace > drivers/net/wireless/ath/ath10k/debug.c:1089: Alignment should match > open parenthesis > drivers/net/wireless/ath/ath10k/debug.c:1092: trailing whitespace > drivers/net/wireless/ath/ath10k/debug.c:1095: Missing a blank line > after declarations > drivers/net/wireless/ath/ath10k/debug.c:1098: line over 90 characters > drivers/net/wireless/ath/ath10k/debug.c:1103: Please don't use > multiple blank lines > drivers/net/wireless/ath/ath10k/debug.c:1105: Alignment should match > open parenthesis > drivers/net/wireless/ath/ath10k/debug.c:1109: trailing whitespace > drivers/net/wireless/ath/ath10k/debug.c:1113: Missing a blank line > after declarations > drivers/net/wireless/ath/ath10k/debug.c:1130: Please don't use > multiple blank lines > drivers/net/wireless/ath/ath10k/debug.c:1133: Please don't use > multiple blank lines > drivers/net/wireless/ath/ath10k/debug.c:1149: Alignment should match > open parenthesis > drivers/net/wireless/ath/ath10k/debug.c:1152: trailing whitespace > drivers/net/wireless/ath/ath10k/debug.c:1155: Missing a blank line > after declarations > drivers/net/wireless/ath/ath10k/debug.c:1163: Please don't use > multiple blank lines > drivers/net/wireless/ath/ath10k/debug.c:1165: Alignment should match > open parenthesis > drivers/net/wireless/ath/ath10k/debug.c:1169: trailing whitespace > drivers/net/wireless/ath/ath10k/debug.c:1173: Missing a blank line > after declarations > drivers/net/wireless/ath/ath10k/debug.c:1188: Please don't use > multiple blank lines > drivers/net/wireless/ath/ath10k/debug.c:1191: Please don't use > multiple blank lines > drivers/net/wireless/ath/ath10k/debug.c:1375: Please don't use > multiple blank lines > drivers/net/wireless/ath/ath10k/wmi-ops.h:211: Please don't use > multiple blank lines > drivers/net/wireless/ath/ath10k/wmi-ops.h:986: trailing whitespace > drivers/net/wireless/ath/ath10k/wmi-ops.h:986: Please use a blank line > after function/struct/union/enum declarations > drivers/net/wireless/ath/ath10k/wmi-ops.h:987: line over 90 characters > drivers/net/wireless/ath/ath10k/wmi-ops.h:1001: trailing whitespace > drivers/net/wireless/ath/ath10k/wmi-tlv.c:3340: line over 90 characters > drivers/net/wireless/ath/ath10k/wmi-tlv.c:3342: please, no spaces at > the start of a line > drivers/net/wireless/ath/ath10k/wmi-tlv.c:3343: please, no spaces at > the start of a line > drivers/net/wireless/ath/ath10k/wmi-tlv.c:3344: please, no spaces at > the start of a line > drivers/net/wireless/ath/ath10k/wmi-tlv.c:3345: trailing whitespace > drivers/net/wireless/ath/ath10k/wmi-tlv.c:3345: please, no spaces at > the start of a line > drivers/net/wireless/ath/ath10k/wmi-tlv.c:3346: please, no spaces at > the start of a line > drivers/net/wireless/ath/ath10k/wmi-tlv.c:3348: please, no spaces at > the start of a line > drivers/net/wireless/ath/ath10k/wmi-tlv.c:3349: please, no spaces at > the start of a line > drivers/net/wireless/ath/ath10k/wmi-tlv.c:3350: please, no spaces at > the start of a line > drivers/net/wireless/ath/ath10k/wmi-tlv.c:3353: please, no spaces at > the start of a line > drivers/net/wireless/ath/ath10k/wmi-tlv.c:3354: please, no spaces at > the start of a line > drivers/net/wireless/ath/ath10k/wmi-tlv.c:3355: please, no spaces at > the start of a line > drivers/net/wireless/ath/ath10k/wmi-tlv.c:3356: please, no spaces at > the start of a line > drivers/net/wireless/ath/ath10k/wmi-tlv.c:3358: please, no spaces at > the start of a line > drivers/net/wireless/ath/ath10k/wmi-tlv.c:3359: please, no spaces at > the start of a line > drivers/net/wireless/ath/ath10k/wmi-tlv.c:3360: please, no spaces at > the start of a line > drivers/net/wireless/ath/ath10k/wmi-tlv.c:3361: please, no spaces at > the start of a line > drivers/net/wireless/ath/ath10k/wmi-tlv.c:3362: please, no spaces at > the start of a line > drivers/net/wireless/ath/ath10k/wmi-tlv.c:3364: please, no spaces at > the start of a line > drivers/net/wireless/ath/ath10k/wmi-tlv.c:3365: please, no spaces at > the start of a line > drivers/net/wireless/ath/ath10k/wmi-tlv.c:3367: line over 90 characters > drivers/net/wireless/ath/ath10k/wmi-tlv.c:3367: please, no spaces at > the start of a line > drivers/net/wireless/ath/ath10k/wmi-tlv.c:3368: please, no spaces at > the start of a line > drivers/net/wireless/ath/ath10k/wmi-tlv.c:3374: please, no spaces at > the start of a line > drivers/net/wireless/ath/ath10k/wmi-tlv.c:3375: please, no spaces at > the start of a line > drivers/net/wireless/ath/ath10k/wmi-tlv.c:3376: please, no spaces at > the start of a line > drivers/net/wireless/ath/ath10k/wmi-tlv.c:3377: trailing whitespace > drivers/net/wireless/ath/ath10k/wmi-tlv.c:3377: please, no spaces at > the start of a line > drivers/net/wireless/ath/ath10k/wmi-tlv.c:3378: please, no spaces at > the start of a line > drivers/net/wireless/ath/ath10k/wmi-tlv.c:3380: please, no spaces at > the start of a line > drivers/net/wireless/ath/ath10k/wmi-tlv.c:3381: please, no spaces at > the start of a line > drivers/net/wireless/ath/ath10k/wmi-tlv.c:3382: please, no spaces at > the start of a line > drivers/net/wireless/ath/ath10k/wmi-tlv.c:3385: please, no spaces at > the start of a line > drivers/net/wireless/ath/ath10k/wmi-tlv.c:3386: please, no spaces at > the start of a line > drivers/net/wireless/ath/ath10k/wmi-tlv.c:3387: please, no spaces at > the start of a line > drivers/net/wireless/ath/ath10k/wmi-tlv.c:3388: please, no spaces at > the start of a line > drivers/net/wireless/ath/ath10k/wmi-tlv.c:3390: please, no spaces at > the start of a line > drivers/net/wireless/ath/ath10k/wmi-tlv.c:3391: please, no spaces at > the start of a line > drivers/net/wireless/ath/ath10k/wmi-tlv.c:3392: please, no spaces at > the start of a line > drivers/net/wireless/ath/ath10k/wmi-tlv.c:3394: please, no spaces at > the start of a line > drivers/net/wireless/ath/ath10k/wmi-tlv.c:3395: please, no spaces at > the start of a line > drivers/net/wireless/ath/ath10k/wmi-tlv.c:3397: line over 90 characters > drivers/net/wireless/ath/ath10k/wmi-tlv.c:3397: please, no spaces at > the start of a line > drivers/net/wireless/ath/ath10k/wmi-tlv.c:3398: please, no spaces at > the start of a line > drivers/net/wireless/ath/ath10k/wmi-tlv.c:3401: Please don't use > multiple blank lines > drivers/net/wireless/ath/ath10k/mac.c:4622: trailing whitespace > drivers/net/wireless/ath/ath10k/mac.c:4623: line over 90 characters > drivers/net/wireless/ath/ath10k/mac.c:4625: trailing whitespace > drivers/net/wireless/ath/ath10k/mac.c:4626: trailing whitespace > drivers/net/wireless/ath/ath10k/wmi.h:2934: please, no spaces at the > start of a line > drivers/net/wireless/ath/ath10k/wmi.h:2935: please, no spaces at the > start of a line > drivers/net/wireless/ath/ath10k/wmi.h:2936: please, no spaces at the > start of a line > drivers/net/wireless/ath/ath10k/wmi.h:2940: please, no spaces at the > start of a line > drivers/net/wireless/ath/ath10k/wmi.h:2941: please, no spaces at the > start of a line > drivers/net/wireless/ath/ath10k/wmi.h:2942: please, no spaces at the > start of a line > drivers/net/wireless/ath/ath10k/wmi.h:2943: please, no spaces at the > start of a line > drivers/net/wireless/ath/ath10k/wmi.h:2944: please, no spaces at the > start of a line > drivers/net/wireless/ath/ath10k/wmi.h:2945: please, no spaces at the > start of a line > drivers/net/wireless/ath/ath10k/wmi.h:2950: please, no spaces at the > start of a line > drivers/net/wireless/ath/ath10k/wmi.h:2951: please, no spaces at the > start of a line > drivers/net/wireless/ath/ath10k/wmi.h:2952: please, no spaces at the > start of a line > drivers/net/wireless/ath/ath10k/wmi.h:2953: please, no spaces at the > start of a line > drivers/net/wireless/ath/ath10k/wmi.h:2958: please, no spaces at the > start of a line > drivers/net/wireless/ath/ath10k/wmi.h:2959: please, no spaces at the > start of a line > drivers/net/wireless/ath/ath10k/wmi.h:2964: please, no spaces at the > start of a line > drivers/net/wireless/ath/ath10k/wmi.h:2967: Please don't use multiple > blank lines > drivers/net/wireless/ath/ath10k/wmi.c:6957: line over 90 characters > drivers/net/wireless/ath/ath10k/wmi.c:6959: please, no spaces at the > start of a line > drivers/net/wireless/ath/ath10k/wmi.c:6960: please, no spaces at the > start of a line > drivers/net/wireless/ath/ath10k/wmi.c:6962: please, no spaces at the > start of a line > drivers/net/wireless/ath/ath10k/wmi.c:6963: please, no spaces at the > start of a line > drivers/net/wireless/ath/ath10k/wmi.c:6965: please, no spaces at the > start of a line > drivers/net/wireless/ath/ath10k/wmi.c:6966: please, no spaces at the > start of a line > drivers/net/wireless/ath/ath10k/wmi.c:6967: please, no spaces at the > start of a line > drivers/net/wireless/ath/ath10k/wmi.c:6968: please, no spaces at the > start of a line > drivers/net/wireless/ath/ath10k/wmi.c:6969: please, no spaces at the > start of a line > drivers/net/wireless/ath/ath10k/wmi.c:6971: line over 90 characters > drivers/net/wireless/ath/ath10k/wmi.c:6971: please, no spaces at the > start of a line > drivers/net/wireless/ath/ath10k/wmi.c:6972: please, no spaces at the > start of a line > drivers/net/wireless/ath/ath10k/wmi.c:6978: please, no spaces at the > start of a line > drivers/net/wireless/ath/ath10k/wmi.c:6979: please, no spaces at the > start of a line > drivers/net/wireless/ath/ath10k/wmi.c:6981: please, no spaces at the > start of a line > drivers/net/wireless/ath/ath10k/wmi.c:6982: please, no spaces at the > start of a line > drivers/net/wireless/ath/ath10k/wmi.c:6984: please, no spaces at the > start of a line > drivers/net/wireless/ath/ath10k/wmi.c:6985: please, no spaces at the > start of a line > drivers/net/wireless/ath/ath10k/wmi.c:6986: please, no spaces at the > start of a line > drivers/net/wireless/ath/ath10k/wmi.c:6987: line over 90 characters > drivers/net/wireless/ath/ath10k/wmi.c:6987: please, no spaces at the > start of a line > drivers/net/wireless/ath/ath10k/wmi.c:6988: please, no spaces at the > start of a line > drivers/net/wireless/ath/ath10k/wmi.c:8583: trailing whitespace >
i have some comments about your check warnings. some of them are bogus. for instance they advise to use "unsigned int" instead of "unsigned". this might be better, but the original kernel header uses "unsigned" as api definition. so i decided to use the same declarations here. for the rest like whitespaces i will make a new version and remove them Am 05.04.2018 um 20:01 schrieb Sebastian Gottschall: > Am 05.04.2018 um 16:44 schrieb Kalle Valo: >> s.gottschall@dd-wrt.com writes: >> >>> 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> >> [...] >> >>> @@ -1034,7 +1068,7 @@ ath10k_wmi_pdev_get_temperature(struct ath10k >>> *ar) >>> if (IS_ERR(skb)) >>> return PTR_ERR(skb); >>> - return ath10k_wmi_cmd_send(ar, skb, >>> + return ath10k_wmi_cmd_send_nowait(ar, skb, >>> ar->wmi.cmd->pdev_get_temperature_cmdid); >>> } >> This looks odd, I don't think it belongs to this patch. > thats true. but due the nature of this function i found it better to > use nowait here. better if i split it up? >> >> Also you made a some sort of record, your patch had 181 checkpatch >> warnings! Do you use Word as your editor or what? But please do check >> your editor settings and read the coding style documents. > no? i use midnight commander for all of my code since more than 20 years > and its the first time that i see such warnings. is there any special > coding rule for ath10k which differs from the kernel rules? > and where is ath10k-check located? >> >> https://wireless.wiki.kernel.org/en/users/drivers/ath10k/codingstyle >> >> I'll send v13. >> >> Here are the warnings from ath10k-check: >> >> drivers/net/wireless/ath/ath10k/gpio.c:19: Please don't use multiple >> blank lines >> drivers/net/wireless/ath/ath10k/gpio.c:30: Please don't use multiple >> blank lines >> drivers/net/wireless/ath/ath10k/gpio.c:36: Prefer 'unsigned int' to >> bare use of 'unsigned' >> drivers/net/wireless/ath/ath10k/gpio.c:38: line over 90 characters >> drivers/net/wireless/ath/ath10k/gpio.c:39: line over 90 characters >> drivers/net/wireless/ath/ath10k/gpio.c:39: Missing a blank line after >> declarations >> drivers/net/wireless/ath/ath10k/gpio.c:45: Prefer 'unsigned int' to >> bare use of 'unsigned' >> drivers/net/wireless/ath/ath10k/gpio.c:46: Alignment should match >> open parenthesis >> drivers/net/wireless/ath/ath10k/gpio.c:48: line over 90 characters >> drivers/net/wireless/ath/ath10k/gpio.c:50: line over 90 characters >> drivers/net/wireless/ath/ath10k/gpio.c:58: Prefer 'unsigned int' to >> bare use of 'unsigned' >> drivers/net/wireless/ath/ath10k/gpio.c:60: line over 90 characters >> drivers/net/wireless/ath/ath10k/gpio.c:66: Prefer 'unsigned int' to >> bare use of 'unsigned' >> drivers/net/wireless/ath/ath10k/gpio.c:68: line over 90 characters >> drivers/net/wireless/ath/ath10k/gpio.c:74: Prefer 'unsigned int' to >> bare use of 'unsigned' >> drivers/net/wireless/ath/ath10k/gpio.c:75: Alignment should match >> open parenthesis >> drivers/net/wireless/ath/ath10k/gpio.c:77: line over 90 characters >> drivers/net/wireless/ath/ath10k/gpio.c:86: trailing whitespace >> drivers/net/wireless/ath/ath10k/gpio.c:86: please, no spaces at the >> start of a line >> drivers/net/wireless/ath/ath10k/gpio.c:86: Blank lines aren't >> necessary after an open brace '{' >> drivers/net/wireless/ath/ath10k/gpio.c:88: Missing a blank line after >> declarations >> drivers/net/wireless/ath/ath10k/gpio.c:88: braces {} are not >> necessary for single statement blocks >> drivers/net/wireless/ath/ath10k/gpio.c:113: trailing whitespace >> drivers/net/wireless/ath/ath10k/gpio.c:114: Missing a blank line >> after declarations >> drivers/net/wireless/ath/ath10k/gpio.c:114: braces {} are not >> necessary for single statement blocks >> drivers/net/wireless/ath/ath10k/gpio.c:121: line over 90 characters >> drivers/net/wireless/ath/ath10k/gpio.c:130: Please don't use multiple >> blank lines >> drivers/net/wireless/ath/ath10k/gpio.c:132: Alignment should match >> open parenthesis >> drivers/net/wireless/ath/ath10k/gpio.c:134: line over 90 characters >> drivers/net/wireless/ath/ath10k/gpio.c:136: Missing a blank line >> after declarations >> drivers/net/wireless/ath/ath10k/gpio.c:145: trailing whitespace >> drivers/net/wireless/ath/ath10k/gpio.c:146: Missing a blank line >> after declarations >> drivers/net/wireless/ath/ath10k/gpio.c:159: trailing whitespace >> drivers/net/wireless/ath/ath10k/gpio.c:160: Missing a blank line >> after declarations >> drivers/net/wireless/ath/ath10k/gpio.c:166: trailing whitespace >> drivers/net/wireless/ath/ath10k/gpio.c:169: trailing whitespace >> drivers/net/wireless/ath/ath10k/gpio.c:170: line over 90 characters >> drivers/net/wireless/ath/ath10k/gpio.c:178: Missing a blank line >> after declarations >> drivers/net/wireless/ath/ath10k/gpio.c:181: Prefer >> kzalloc(sizeof(*gpio)...) over kzalloc(sizeof(struct >> ath10k_gpiocontrol)...) >> drivers/net/wireless/ath/ath10k/gpio.c:182: braces {} are not >> necessary for single statement blocks >> drivers/net/wireless/ath/ath10k/gpio.c:192: trailing whitespace >> drivers/net/wireless/ath/ath10k/gpio.c:194: line over 90 characters >> drivers/net/wireless/ath/ath10k/gpio.c:196: trailing whitespace >> drivers/net/wireless/ath/ath10k/core.h:865: trailing whitespace >> drivers/net/wireless/ath/ath10k/core.h:865: line over 90 characters >> drivers/net/wireless/ath/ath10k/core.h:871: trailing whitespace >> drivers/net/wireless/ath/ath10k/core.h:890: Blank lines aren't >> necessary after an open brace '{' >> drivers/net/wireless/ath/ath10k/core.h:891: Blank lines aren't >> necessary before a close brace '}' >> drivers/net/wireless/ath/ath10k/core.h:892: Please use a blank line >> after function/struct/union/enum declarations >> drivers/net/wireless/ath/ath10k/core.h:894: Blank lines aren't >> necessary after an open brace '{' >> drivers/net/wireless/ath/ath10k/core.h:895: Blank lines aren't >> necessary before a close brace '}' >> drivers/net/wireless/ath/ath10k/core.h:896: Please use a blank line >> after function/struct/union/enum declarations >> drivers/net/wireless/ath/ath10k/core.h:905: trailing whitespace >> drivers/net/wireless/ath/ath10k/core.h:907: Blank lines aren't >> necessary after an open brace '{' >> drivers/net/wireless/ath/ath10k/core.h:908: Blank lines aren't >> necessary before a close brace '}' >> drivers/net/wireless/ath/ath10k/core.c:29: Please don't use multiple >> blank lines >> drivers/net/wireless/ath/ath10k/core.c:74: trailing whitespace >> drivers/net/wireless/ath/ath10k/core.c:105: trailing whitespace >> drivers/net/wireless/ath/ath10k/core.c:136: trailing whitespace >> drivers/net/wireless/ath/ath10k/core.c:282: trailing whitespace >> drivers/net/wireless/ath/ath10k/core.c:318: trailing whitespace >> drivers/net/wireless/ath/ath10k/core.c:359: trailing whitespace >> drivers/net/wireless/ath/ath10k/core.c:2396: trailing whitespace >> drivers/net/wireless/ath/ath10k/core.c:2396: Please don't use >> multiple blank lines >> drivers/net/wireless/ath/ath10k/core.c:2403: trailing whitespace >> drivers/net/wireless/ath/ath10k/debug.c:1089: Alignment should match >> open parenthesis >> drivers/net/wireless/ath/ath10k/debug.c:1092: trailing whitespace >> drivers/net/wireless/ath/ath10k/debug.c:1095: Missing a blank line >> after declarations >> drivers/net/wireless/ath/ath10k/debug.c:1098: line over 90 characters >> drivers/net/wireless/ath/ath10k/debug.c:1103: Please don't use >> multiple blank lines >> drivers/net/wireless/ath/ath10k/debug.c:1105: Alignment should match >> open parenthesis >> drivers/net/wireless/ath/ath10k/debug.c:1109: trailing whitespace >> drivers/net/wireless/ath/ath10k/debug.c:1113: Missing a blank line >> after declarations >> drivers/net/wireless/ath/ath10k/debug.c:1130: Please don't use >> multiple blank lines >> drivers/net/wireless/ath/ath10k/debug.c:1133: Please don't use >> multiple blank lines >> drivers/net/wireless/ath/ath10k/debug.c:1149: Alignment should match >> open parenthesis >> drivers/net/wireless/ath/ath10k/debug.c:1152: trailing whitespace >> drivers/net/wireless/ath/ath10k/debug.c:1155: Missing a blank line >> after declarations >> drivers/net/wireless/ath/ath10k/debug.c:1163: Please don't use >> multiple blank lines >> drivers/net/wireless/ath/ath10k/debug.c:1165: Alignment should match >> open parenthesis >> drivers/net/wireless/ath/ath10k/debug.c:1169: trailing whitespace >> drivers/net/wireless/ath/ath10k/debug.c:1173: Missing a blank line >> after declarations >> drivers/net/wireless/ath/ath10k/debug.c:1188: Please don't use >> multiple blank lines >> drivers/net/wireless/ath/ath10k/debug.c:1191: Please don't use >> multiple blank lines >> drivers/net/wireless/ath/ath10k/debug.c:1375: Please don't use >> multiple blank lines >> drivers/net/wireless/ath/ath10k/wmi-ops.h:211: Please don't use >> multiple blank lines >> drivers/net/wireless/ath/ath10k/wmi-ops.h:986: trailing whitespace >> drivers/net/wireless/ath/ath10k/wmi-ops.h:986: Please use a blank >> line after function/struct/union/enum declarations >> drivers/net/wireless/ath/ath10k/wmi-ops.h:987: line over 90 characters >> drivers/net/wireless/ath/ath10k/wmi-ops.h:1001: trailing whitespace >> drivers/net/wireless/ath/ath10k/wmi-tlv.c:3340: line over 90 characters >> drivers/net/wireless/ath/ath10k/wmi-tlv.c:3342: please, no spaces at >> the start of a line >> drivers/net/wireless/ath/ath10k/wmi-tlv.c:3343: please, no spaces at >> the start of a line >> drivers/net/wireless/ath/ath10k/wmi-tlv.c:3344: please, no spaces at >> the start of a line >> drivers/net/wireless/ath/ath10k/wmi-tlv.c:3345: trailing whitespace >> drivers/net/wireless/ath/ath10k/wmi-tlv.c:3345: please, no spaces at >> the start of a line >> drivers/net/wireless/ath/ath10k/wmi-tlv.c:3346: please, no spaces at >> the start of a line >> drivers/net/wireless/ath/ath10k/wmi-tlv.c:3348: please, no spaces at >> the start of a line >> drivers/net/wireless/ath/ath10k/wmi-tlv.c:3349: please, no spaces at >> the start of a line >> drivers/net/wireless/ath/ath10k/wmi-tlv.c:3350: please, no spaces at >> the start of a line >> drivers/net/wireless/ath/ath10k/wmi-tlv.c:3353: please, no spaces at >> the start of a line >> drivers/net/wireless/ath/ath10k/wmi-tlv.c:3354: please, no spaces at >> the start of a line >> drivers/net/wireless/ath/ath10k/wmi-tlv.c:3355: please, no spaces at >> the start of a line >> drivers/net/wireless/ath/ath10k/wmi-tlv.c:3356: please, no spaces at >> the start of a line >> drivers/net/wireless/ath/ath10k/wmi-tlv.c:3358: please, no spaces at >> the start of a line >> drivers/net/wireless/ath/ath10k/wmi-tlv.c:3359: please, no spaces at >> the start of a line >> drivers/net/wireless/ath/ath10k/wmi-tlv.c:3360: please, no spaces at >> the start of a line >> drivers/net/wireless/ath/ath10k/wmi-tlv.c:3361: please, no spaces at >> the start of a line >> drivers/net/wireless/ath/ath10k/wmi-tlv.c:3362: please, no spaces at >> the start of a line >> drivers/net/wireless/ath/ath10k/wmi-tlv.c:3364: please, no spaces at >> the start of a line >> drivers/net/wireless/ath/ath10k/wmi-tlv.c:3365: please, no spaces at >> the start of a line >> drivers/net/wireless/ath/ath10k/wmi-tlv.c:3367: line over 90 characters >> drivers/net/wireless/ath/ath10k/wmi-tlv.c:3367: please, no spaces at >> the start of a line >> drivers/net/wireless/ath/ath10k/wmi-tlv.c:3368: please, no spaces at >> the start of a line >> drivers/net/wireless/ath/ath10k/wmi-tlv.c:3374: please, no spaces at >> the start of a line >> drivers/net/wireless/ath/ath10k/wmi-tlv.c:3375: please, no spaces at >> the start of a line >> drivers/net/wireless/ath/ath10k/wmi-tlv.c:3376: please, no spaces at >> the start of a line >> drivers/net/wireless/ath/ath10k/wmi-tlv.c:3377: trailing whitespace >> drivers/net/wireless/ath/ath10k/wmi-tlv.c:3377: please, no spaces at >> the start of a line >> drivers/net/wireless/ath/ath10k/wmi-tlv.c:3378: please, no spaces at >> the start of a line >> drivers/net/wireless/ath/ath10k/wmi-tlv.c:3380: please, no spaces at >> the start of a line >> drivers/net/wireless/ath/ath10k/wmi-tlv.c:3381: please, no spaces at >> the start of a line >> drivers/net/wireless/ath/ath10k/wmi-tlv.c:3382: please, no spaces at >> the start of a line >> drivers/net/wireless/ath/ath10k/wmi-tlv.c:3385: please, no spaces at >> the start of a line >> drivers/net/wireless/ath/ath10k/wmi-tlv.c:3386: please, no spaces at >> the start of a line >> drivers/net/wireless/ath/ath10k/wmi-tlv.c:3387: please, no spaces at >> the start of a line >> drivers/net/wireless/ath/ath10k/wmi-tlv.c:3388: please, no spaces at >> the start of a line >> drivers/net/wireless/ath/ath10k/wmi-tlv.c:3390: please, no spaces at >> the start of a line >> drivers/net/wireless/ath/ath10k/wmi-tlv.c:3391: please, no spaces at >> the start of a line >> drivers/net/wireless/ath/ath10k/wmi-tlv.c:3392: please, no spaces at >> the start of a line >> drivers/net/wireless/ath/ath10k/wmi-tlv.c:3394: please, no spaces at >> the start of a line >> drivers/net/wireless/ath/ath10k/wmi-tlv.c:3395: please, no spaces at >> the start of a line >> drivers/net/wireless/ath/ath10k/wmi-tlv.c:3397: line over 90 characters >> drivers/net/wireless/ath/ath10k/wmi-tlv.c:3397: please, no spaces at >> the start of a line >> drivers/net/wireless/ath/ath10k/wmi-tlv.c:3398: please, no spaces at >> the start of a line >> drivers/net/wireless/ath/ath10k/wmi-tlv.c:3401: Please don't use >> multiple blank lines >> drivers/net/wireless/ath/ath10k/mac.c:4622: trailing whitespace >> drivers/net/wireless/ath/ath10k/mac.c:4623: line over 90 characters >> drivers/net/wireless/ath/ath10k/mac.c:4625: trailing whitespace >> drivers/net/wireless/ath/ath10k/mac.c:4626: trailing whitespace >> drivers/net/wireless/ath/ath10k/wmi.h:2934: please, no spaces at the >> start of a line >> drivers/net/wireless/ath/ath10k/wmi.h:2935: please, no spaces at the >> start of a line >> drivers/net/wireless/ath/ath10k/wmi.h:2936: please, no spaces at the >> start of a line >> drivers/net/wireless/ath/ath10k/wmi.h:2940: please, no spaces at the >> start of a line >> drivers/net/wireless/ath/ath10k/wmi.h:2941: please, no spaces at the >> start of a line >> drivers/net/wireless/ath/ath10k/wmi.h:2942: please, no spaces at the >> start of a line >> drivers/net/wireless/ath/ath10k/wmi.h:2943: please, no spaces at the >> start of a line >> drivers/net/wireless/ath/ath10k/wmi.h:2944: please, no spaces at the >> start of a line >> drivers/net/wireless/ath/ath10k/wmi.h:2945: please, no spaces at the >> start of a line >> drivers/net/wireless/ath/ath10k/wmi.h:2950: please, no spaces at the >> start of a line >> drivers/net/wireless/ath/ath10k/wmi.h:2951: please, no spaces at the >> start of a line >> drivers/net/wireless/ath/ath10k/wmi.h:2952: please, no spaces at the >> start of a line >> drivers/net/wireless/ath/ath10k/wmi.h:2953: please, no spaces at the >> start of a line >> drivers/net/wireless/ath/ath10k/wmi.h:2958: please, no spaces at the >> start of a line >> drivers/net/wireless/ath/ath10k/wmi.h:2959: please, no spaces at the >> start of a line >> drivers/net/wireless/ath/ath10k/wmi.h:2964: please, no spaces at the >> start of a line >> drivers/net/wireless/ath/ath10k/wmi.h:2967: Please don't use multiple >> blank lines >> drivers/net/wireless/ath/ath10k/wmi.c:6957: line over 90 characters >> drivers/net/wireless/ath/ath10k/wmi.c:6959: please, no spaces at the >> start of a line >> drivers/net/wireless/ath/ath10k/wmi.c:6960: please, no spaces at the >> start of a line >> drivers/net/wireless/ath/ath10k/wmi.c:6962: please, no spaces at the >> start of a line >> drivers/net/wireless/ath/ath10k/wmi.c:6963: please, no spaces at the >> start of a line >> drivers/net/wireless/ath/ath10k/wmi.c:6965: please, no spaces at the >> start of a line >> drivers/net/wireless/ath/ath10k/wmi.c:6966: please, no spaces at the >> start of a line >> drivers/net/wireless/ath/ath10k/wmi.c:6967: please, no spaces at the >> start of a line >> drivers/net/wireless/ath/ath10k/wmi.c:6968: please, no spaces at the >> start of a line >> drivers/net/wireless/ath/ath10k/wmi.c:6969: please, no spaces at the >> start of a line >> drivers/net/wireless/ath/ath10k/wmi.c:6971: line over 90 characters >> drivers/net/wireless/ath/ath10k/wmi.c:6971: please, no spaces at the >> start of a line >> drivers/net/wireless/ath/ath10k/wmi.c:6972: please, no spaces at the >> start of a line >> drivers/net/wireless/ath/ath10k/wmi.c:6978: please, no spaces at the >> start of a line >> drivers/net/wireless/ath/ath10k/wmi.c:6979: please, no spaces at the >> start of a line >> drivers/net/wireless/ath/ath10k/wmi.c:6981: please, no spaces at the >> start of a line >> drivers/net/wireless/ath/ath10k/wmi.c:6982: please, no spaces at the >> start of a line >> drivers/net/wireless/ath/ath10k/wmi.c:6984: please, no spaces at the >> start of a line >> drivers/net/wireless/ath/ath10k/wmi.c:6985: please, no spaces at the >> start of a line >> drivers/net/wireless/ath/ath10k/wmi.c:6986: please, no spaces at the >> start of a line >> drivers/net/wireless/ath/ath10k/wmi.c:6987: line over 90 characters >> drivers/net/wireless/ath/ath10k/wmi.c:6987: please, no spaces at the >> start of a line >> drivers/net/wireless/ath/ath10k/wmi.c:6988: please, no spaces at the >> start of a line >> drivers/net/wireless/ath/ath10k/wmi.c:8583: trailing whitespace >> >
Sebastian Gottschall <s.gottschall@dd-wrt.com> writes: > Am 05.04.2018 um 16:44 schrieb Kalle Valo: >> s.gottschall@dd-wrt.com writes: >> >>> 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> >> [...] >> >>> @@ -1034,7 +1068,7 @@ ath10k_wmi_pdev_get_temperature(struct ath10k *ar) >>> if (IS_ERR(skb)) >>> return PTR_ERR(skb); >>> - return ath10k_wmi_cmd_send(ar, skb, >>> + return ath10k_wmi_cmd_send_nowait(ar, skb, >>> ar->wmi.cmd->pdev_get_temperature_cmdid); >>> } >> This looks odd, I don't think it belongs to this patch. > > thats true. but due the nature of this function i found it better to > use nowait here. better if i split it up? Yes, this should be done in a separate patch with a proper commit log explaining why it's needed. >> Also you made a some sort of record, your patch had 181 checkpatch >> warnings! Do you use Word as your editor or what? But please do check >> your editor settings and read the coding style documents. > > no? i use midnight commander for all of my code since more than 20 years > and its the first time that i see such warnings. is there any special > coding rule for ath10k which differs from the kernel rules? You got even the indentation wrong in multiple functions and indentation rules have been the same as long as I remember. And checkpatch has been around a long time already, that should not be new to anyone submitting patches. > and where is ath10k-check located? Check the link I provided: >> https://wireless.wiki.kernel.org/en/users/drivers/ath10k/codingstyle
Sebastian Gottschall <s.gottschall@dd-wrt.com> writes: > i have some comments about your check warnings. > some of them are bogus. for instance they advise to use "unsigned int" > instead of "unsigned". this might be better, but > the original kernel header uses "unsigned" as api definition. so i > decided to use the same declarations here. Sure, but using "unsigned int" won't create any harm either. > for the rest like whitespaces i will make a new version and remove them You missed my comment in my earlier mail that I'll send the next version. I have already fixed all the warnings so let me submit it. I'll do also some other changes.
(adding back the lists, please don't top post and trim your quotes) Sebastian Gottschall <s.gottschall@dd-wrt.com> writes: > okay. ath10k-check is buggy and doesnt work. so it doesnt help much > with code styles > > root@seg-desktop:/xfs/ath10k/ath.gpio# ./ath10k-check > global: 'drivers/net/wireless/ath/ath10k/Kconfig' is not a source file. > Traceback (most recent call last): > File "./ath10k-check", line 461, in <module> > main() > File "./ath10k-check", line 455, in main > ret = run_checkpatch(args) > File "./ath10k-check", line 275, in run_checkpatch > output = subprocess.check_output(cmd, shell=True) > File "/usr/lib/python2.7/subprocess.py", line 219, in check_output > raise CalledProcessError(retcode, cmd, output=output) > subprocess.CalledProcessError: Command 'global -f > drivers/net/wireless/ath/ath10k/Kconfig' returned non-zero exit status 1 You are missing gtags, see 'ath10k-check --help' for how to install: Requirements (all available in $PATH): * gcc * sparse * checkpatch.pl * gtags (from package global)
Am 06.04.2018 um 10:07 schrieb Kalle Valo: > Sebastian Gottschall <s.gottschall@dd-wrt.com> writes: > >> i have some comments about your check warnings. >> some of them are bogus. for instance they advise to use "unsigned int" >> instead of "unsigned". this might be better, but >> the original kernel header uses "unsigned" as api definition. so i >> decided to use the same declarations here. > Sure, but using "unsigned int" won't create any harm either. > >> for the rest like whitespaces i will make a new version and remove them > You missed my comment in my earlier mail that I'll send the next > version. I have already fixed all the warnings so let me submit it. I'll > do also some other changes. oh okay. thank you. then forget all the rest. i will take care of these issues with the next patch i submit >
Am 06.04.2018 um 10:09 schrieb Kalle Valo: > (adding back the lists, please don't top post and trim your quotes) > > Sebastian Gottschall <s.gottschall@dd-wrt.com> writes: > >> okay. ath10k-check is buggy and doesnt work. so it doesnt help much >> with code styles >> >> root@seg-desktop:/xfs/ath10k/ath.gpio# ./ath10k-check >> global: 'drivers/net/wireless/ath/ath10k/Kconfig' is not a source file. >> Traceback (most recent call last): >> File "./ath10k-check", line 461, in <module> >> main() >> File "./ath10k-check", line 455, in main >> ret = run_checkpatch(args) >> File "./ath10k-check", line 275, in run_checkpatch >> output = subprocess.check_output(cmd, shell=True) >> File "/usr/lib/python2.7/subprocess.py", line 219, in check_output >> raise CalledProcessError(retcode, cmd, output=output) >> subprocess.CalledProcessError: Command 'global -f >> drivers/net/wireless/ath/ath10k/Kconfig' returned non-zero exit status 1 > You are missing gtags, see 'ath10k-check --help' for how to install: > > Requirements (all available in $PATH): > > * gcc > * sparse > * checkpatch.pl > * gtags (from package global) ahm i did read this and this was all installed. but ath10k-check refuses to work root@seg-desktop:/home/seg/DEV# gtags --version gtags (GNU GLOBAL) 6.5.7 Copyright (c) 1996-2017 Tama Communications Corporation License GPLv3+: GNU GPL version 3 or later <http://www.gnu.org/licenses/gpl.html> This is free software; you are free to change and redistribute it. There is NO WARRANTY, to the extent permitted by law. >
Sebastian Gottschall <s.gottschall@dd-wrt.com> writes: > Am 06.04.2018 um 10:09 schrieb Kalle Valo: >> (adding back the lists, please don't top post and trim your quotes) >> >> Sebastian Gottschall <s.gottschall@dd-wrt.com> writes: >> >>> okay. ath10k-check is buggy and doesnt work. so it doesnt help much >>> with code styles >>> >>> root@seg-desktop:/xfs/ath10k/ath.gpio# ./ath10k-check >>> global: 'drivers/net/wireless/ath/ath10k/Kconfig' is not a source file. >>> Traceback (most recent call last): >>> File "./ath10k-check", line 461, in <module> >>> main() >>> File "./ath10k-check", line 455, in main >>> ret = run_checkpatch(args) >>> File "./ath10k-check", line 275, in run_checkpatch >>> output = subprocess.check_output(cmd, shell=True) >>> File "/usr/lib/python2.7/subprocess.py", line 219, in check_output >>> raise CalledProcessError(retcode, cmd, output=output) >>> subprocess.CalledProcessError: Command 'global -f >>> drivers/net/wireless/ath/ath10k/Kconfig' returned non-zero exit status 1 >> >> You are missing gtags, see 'ath10k-check --help' for how to install: >> >> Requirements (all available in $PATH): >> >> * gcc >> * sparse >> * checkpatch.pl >> * gtags (from package global) > > ahm i did read this and this was all installed. but ath10k-check > refuses to work > > root@seg-desktop:/home/seg/DEV# gtags --version > gtags (GNU GLOBAL) 6.5.7 > Copyright (c) 1996-2017 Tama Communications Corporation > License GPLv3+: GNU GPL version 3 or later > <http://www.gnu.org/licenses/gpl.html> > This is free software; you are free to change and redistribute it. > There is NO WARRANTY, to the extent permitted by law. Damn, I guess something has changed in gtags as I use older version: gtags - GNU GLOBAL 5.7.1
(adding back lists) Sebastian Gottschall <s.gottschall@dd-wrt.com> writes: > Am 06.04.2018 um 10:25 schrieb Kalle Valo: >> Sebastian Gottschall <s.gottschall@dd-wrt.com> writes: >> >>> Am 06.04.2018 um 10:09 schrieb Kalle Valo: >>>> (adding back the lists, please don't top post and trim your quotes) >>>> >>>> Sebastian Gottschall <s.gottschall@dd-wrt.com> writes: >>>> >>>>> okay. ath10k-check is buggy and doesnt work. so it doesnt help much >>>>> with code styles >>>>> >>>>> root@seg-desktop:/xfs/ath10k/ath.gpio# ./ath10k-check >>>>> global: 'drivers/net/wireless/ath/ath10k/Kconfig' is not a source file. >>>>> Traceback (most recent call last): >>>>> File "./ath10k-check", line 461, in <module> >>>>> main() >>>>> File "./ath10k-check", line 455, in main >>>>> ret = run_checkpatch(args) >>>>> File "./ath10k-check", line 275, in run_checkpatch >>>>> output = subprocess.check_output(cmd, shell=True) >>>>> File "/usr/lib/python2.7/subprocess.py", line 219, in check_output >>>>> raise CalledProcessError(retcode, cmd, output=output) >>>>> subprocess.CalledProcessError: Command 'global -f >>>>> drivers/net/wireless/ath/ath10k/Kconfig' returned non-zero exit status 1 >>>> You are missing gtags, see 'ath10k-check --help' for how to install: >>>> >>>> Requirements (all available in $PATH): >>>> >>>> * gcc >>>> * sparse >>>> * checkpatch.pl >>>> * gtags (from package global) >>> ahm i did read this and this was all installed. but ath10k-check >>> refuses to work >>> >>> root@seg-desktop:/home/seg/DEV# gtags --version >>> gtags (GNU GLOBAL) 6.5.7 >>> Copyright (c) 1996-2017 Tama Communications Corporation >>> License GPLv3+: GNU GPL version 3 or later >>> <http://www.gnu.org/licenses/gpl.html> >>> This is free software; you are free to change and redistribute it. >>> There is NO WARRANTY, to the extent permitted by law. >> Damn, I guess something has changed in gtags as I use older version: >> >> gtags - GNU GLOBAL 5.7.1 > > okay. i will try to use a older version and see what happens You can also run checkpatch manually (sorry for the long line): checkpatch.pl --strict -q --terse --no-summary --max-line-length=90 --show-types --ignore MSLEEP,USLEEP_RANGE,PRINTK_WITHOUT_KERN_LEVEL,NETWORKING_BLOCK_COMMENT_STYLE,LINUX_VERSION_CODE,COMPLEX_MACRO,PREFER_DEV_LEVEL,PREFER_PR_LEVEL,COMPARISON_TO_NULL,BIT_MACRO,CONSTANT_COMPARISON,MACRO_WITH_FLOW_CONTROL,CONST_STRUCT,MACRO_ARG_REUSE,MACRO_ARG_PRECEDENCE foo.patch I modified ath10k-check --help to print the full commandline, just haven't pushed it out yet.
diff --git a/drivers/net/wireless/ath/ath10k/Kconfig b/drivers/net/wireless/ath/ath10k/Kconfig index deb5ae21a559..5d61d499dca4 100644 --- a/drivers/net/wireless/ath/ath10k/Kconfig +++ b/drivers/net/wireless/ath/ath10k/Kconfig @@ -10,6 +10,16 @@ config ATH10K If you choose to build a module, it'll be called ath10k. +config ATH10K_LEDS + bool "SoftLED Support" + depends on ATH10K + select MAC80211_LEDS + select LEDS_CLASS + select NEW_LEDS + default y + help + This option is necessary, if you want LED support for chipset connected led pins + config ATH10K_PCI tristate "Atheros ath10k PCI support" depends on ATH10K && PCI diff --git a/drivers/net/wireless/ath/ath10k/Makefile b/drivers/net/wireless/ath/ath10k/Makefile index 6739ac26fd29..eccc9806fa43 100644 --- a/drivers/net/wireless/ath/ath10k/Makefile +++ b/drivers/net/wireless/ath/ath10k/Makefile @@ -20,6 +20,7 @@ ath10k_core-$(CONFIG_NL80211_TESTMODE) += testmode.o ath10k_core-$(CONFIG_ATH10K_TRACING) += trace.o ath10k_core-$(CONFIG_THERMAL) += thermal.o ath10k_core-$(CONFIG_MAC80211_DEBUGFS) += debugfs_sta.o +ath10k_core-$(CONFIG_ATH10K_LEDS) += 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 <linux/dmi.h> #include <linux/ctype.h> #include <asm/byteorder.h> +#include <linux/leds.h> +#include <linux/platform_device.h> +#include <linux/version.h> + #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 = 0x00000700, @@ -301,6 +313,8 @@ static const struct ath10k_hw_params ath10k_hw_params_list[] = { .id = QCA9984_HW_1_0_DEV_VERSION, .dev_id = QCA9984_1_0_DEVICE_ID, .name = "qca9984/qca9994 hw1.0", + .led_pin = 17, + .gpio_count = 35, .patch_load_addr = QCA9984_HW_1_0_PATCH_LOAD_ADDR, .uart_pin = 7, .cc_wraparound_type = ATH10K_HW_CC_WRAP_SHIFTED_EACH, @@ -340,6 +354,8 @@ static const struct ath10k_hw_params ath10k_hw_params_list[] = { .id = QCA9888_HW_2_0_DEV_VERSION, .dev_id = QCA9888_2_0_DEVICE_ID, .name = "qca9888 hw2.0", + .led_pin = 17, + .gpio_count = 35, .patch_load_addr = QCA9888_HW_2_0_PATCH_LOAD_ADDR, .uart_pin = 7, .cc_wraparound_type = ATH10K_HW_CC_WRAP_SHIFTED_EACH, @@ -2372,8 +2388,15 @@ int ath10k_core_start(struct ath10k *ar, enum ath10k_firmware_mode mode, if (status) goto err_hif_stop; - return 0; + + status = ath10k_attach_led(ar); + if (status) + goto err_no_led; + ath10k_attach_gpio(ar); + +err_no_led: + return 0; err_hif_stop: ath10k_hif_stop(ar); err_htt_rx_detach: @@ -2673,6 +2696,9 @@ void ath10k_core_unregister(struct ath10k *ar) ath10k_core_free_board_files(ar); ath10k_debug_unregister(ar); + + ath10k_unregister_gpio_chip(ar); + ath10k_unregister_led(ar); } EXPORT_SYMBOL(ath10k_core_unregister); diff --git a/drivers/net/wireless/ath/ath10k/core.h b/drivers/net/wireless/ath/ath10k/core.h index c624b96f8b84..ed1789e716e9 100644 --- a/drivers/net/wireless/ath/ath10k/core.h +++ b/drivers/net/wireless/ath/ath10k/core.h @@ -25,6 +25,8 @@ #include <linux/pci.h> #include <linux/uuid.h> #include <linux/time.h> +#include <linux/gpio.h> +#include <linux/leds.h> #include "htt.h" #include "htc.h" @@ -812,6 +814,62 @@ struct ath10k_per_peer_tx_stats { u32 reserved2; }; +struct ath10k_gpiocontrol { + struct ath10k *ar; + + /* since we have no gpio read method, these are the state variables for debugfs. */ + u32 gpio_set_num; + u32 gpio_num; + u32 gpio_input; + u32 gpio_pull_type; + u32 gpio_intr_mode; + u32 gpio_set; + +#if IS_ENABLED(CONFIG_GPIOLIB) + struct gpio_chip gchip; +#endif /* CONFIG_GPIOLIB */ + struct gpio_led wifi_led; + struct led_classdev cdev; + char label[48]; + u32 gpio_state_dir; /* same as for debugfs, but for gpiochip implementation */ + u32 gpio_state_pin; +}; + +#if IS_ENABLED(CONFIG_ATH10K_LEDS) +void ath10k_unregister_led(struct ath10k *ar); +void ath10k_reset_led_pin(struct ath10k *ar); +int ath10k_attach_led(struct ath10k *ar); +#else +static inline void ath10k_unregister_led(struct ath10k *ar) +{ + +} +static inline void ath10k_reset_led_pin(struct ath10k *ar) +{ + +} +static inline int ath10k_attach_led(struct ath10k *ar) +{ + return -ENODEV; +} +#endif + +#if IS_ENABLED(CONFIG_ATH10K_LEDS) && IS_ENABLED(CONFIG_GPIOLIB) +void ath10k_unregister_gpio_chip(struct ath10k *ar); +#else +static inline void ath10k_unregister_gpio_chip(struct ath10k *ar) +{ + +} +#endif +#if IS_ENABLED(CONFIG_ATH10K_LEDS) +int ath10k_attach_gpio(struct ath10k *ar); +#else +static inline int ath10k_attach_gpio(struct ath10k *ar) +{ + return -ENODEV; +} +#endif struct ath10k { struct ath_common ath_common; struct ieee80211_hw *hw; @@ -840,7 +898,9 @@ struct ath10k { u32 low_5ghz_chan; u32 high_5ghz_chan; bool ani_enabled; - +#if IS_ENABLED(CONFIG_ATH10K_LEDS) + struct ath10k_gpiocontrol *gpio; +#endif bool p2p; struct { diff --git a/drivers/net/wireless/ath/ath10k/debug.c b/drivers/net/wireless/ath/ath10k/debug.c index 1b9c092d210f..a8baa7424633 100644 --- a/drivers/net/wireless/ath/ath10k/debug.c +++ b/drivers/net/wireless/ath/ath10k/debug.c @@ -1084,6 +1084,126 @@ static ssize_t ath10k_write_fw_dbglog(struct file *file, return ret; } +#if IS_ENABLED(CONFIG_ATH10K_LEDS) +static ssize_t ath10k_read_gpio_config(struct file *file, char __user *user_buf, + size_t count, loff_t *ppos) +{ + struct ath10k *ar = file->private_data; + struct ath10k_gpiocontrol *gpio = ar->gpio; + size_t len; + char buf[96]; + if (!gpio) + return 0; + + len = scnprintf(buf, sizeof(buf), "%u %u %u %u\n", gpio->gpio_num, gpio->gpio_input, gpio->gpio_pull_type, gpio->gpio_intr_mode); + + return simple_read_from_buffer(user_buf, count, ppos, buf, len); +} + + +static ssize_t ath10k_write_gpio_config(struct file *file, + const char __user *user_buf, + size_t count, loff_t *ppos) +{ + struct ath10k *ar = file->private_data; + struct ath10k_gpiocontrol *gpio = ar->gpio; + int ret; + char buf[96]; + u32 gpio_num, input, pull_type, intr_mode; + if (!gpio) + return -EINVAL; + + simple_write_to_buffer(buf, sizeof(buf) - 1, ppos, user_buf, count); + + /* make sure that buf is null terminated */ + buf[sizeof(buf) - 1] = 0; + + gpio->gpio_num = gpio_num; + gpio->gpio_input = input; + gpio->gpio_pull_type = pull_type; + gpio->gpio_intr_mode = intr_mode; + ret = sscanf(buf, "%u %u %u %u", &gpio_num, &input, &pull_type, &intr_mode); + + if (!ret) + return -EINVAL; + + + mutex_lock(&ar->conf_mutex); + + + ret = ath10k_wmi_gpio_config(ar, gpio_num, input, pull_type, intr_mode); + + if (ret) { + ath10k_warn(ar, "gpio_config cfg failed from debugfs: %d\n", ret); + goto exit; + } + ret = count; + +exit: + mutex_unlock(&ar->conf_mutex); + + return ret; +} + +static ssize_t ath10k_read_gpio_output(struct file *file, char __user *user_buf, + size_t count, loff_t *ppos) +{ + struct ath10k *ar = file->private_data; + struct ath10k_gpiocontrol *gpio = ar->gpio; + size_t len; + char buf[96]; + if (!gpio) + return 0; + + len = scnprintf(buf, sizeof(buf), "%u %u\n", gpio->gpio_num, gpio->gpio_set); + + return simple_read_from_buffer(user_buf, count, ppos, buf, len); +} + + +static ssize_t ath10k_write_gpio_output(struct file *file, + const char __user *user_buf, + size_t count, loff_t *ppos) +{ + struct ath10k *ar = file->private_data; + struct ath10k_gpiocontrol *gpio = ar->gpio; + int ret; + char buf[96]; + u32 gpio_num, set; + if (!gpio) + return -EINVAL; + + simple_write_to_buffer(buf, sizeof(buf) - 1, ppos, user_buf, count); + + /* make sure that buf is null terminated */ + buf[sizeof(buf) - 1] = 0; + + gpio->gpio_set_num = gpio_num; + gpio->gpio_set = set; + ret = sscanf(buf, "%u %u", &gpio_num, &set); + + if (!ret) + return -EINVAL; + + + mutex_lock(&ar->conf_mutex); + + + ret = ath10k_wmi_gpio_output(ar, gpio_num, set); + + if (ret) { + ath10k_warn(ar, "gpio_output cfg failed from debugfs: %d\n", ret); + goto exit; + } + ret = count; + +exit: + mutex_unlock(&ar->conf_mutex); + + return ret; +} +#endif + /* TODO: Would be nice to always support ethtool stats, would need to * move the stats storage out of ath10k_debug, or always have ath10k_debug * struct available.. @@ -1252,6 +1372,25 @@ static const struct file_operations fops_fw_dbglog = { .llseek = default_llseek, }; + +#if IS_ENABLED(CONFIG_ATH10K_LEDS) +static const struct file_operations fops_gpio_output = { + .read = ath10k_read_gpio_output, + .write = ath10k_write_gpio_output, + .open = simple_open, + .owner = THIS_MODULE, + .llseek = default_llseek, +}; + +static const struct file_operations fops_gpio_config = { + .read = ath10k_read_gpio_config, + .write = ath10k_write_gpio_config, + .open = simple_open, + .owner = THIS_MODULE, + .llseek = default_llseek, +}; +#endif + static int ath10k_debug_cal_data_fetch(struct ath10k *ar) { u32 hi_addr; @@ -2259,6 +2398,13 @@ int ath10k_debug_register(struct ath10k *ar) debugfs_create_file("fw_dbglog", 0600, ar->debug.debugfs_phy, ar, &fops_fw_dbglog); +#if IS_ENABLED(CONFIG_ATH10K_LEDS) + debugfs_create_file("gpio_output", 0600, ar->debug.debugfs_phy, ar, + &fops_gpio_output); + + debugfs_create_file("gpio_config", 0600, ar->debug.debugfs_phy, ar, + &fops_gpio_config); +#endif debugfs_create_file("cal_data", 0400, ar->debug.debugfs_phy, ar, &fops_cal_data); diff --git a/drivers/net/wireless/ath/ath10k/gpio.c b/drivers/net/wireless/ath/ath10k/gpio.c new file mode 100644 index 000000000000..e9ce44bb21b0 --- /dev/null +++ b/drivers/net/wireless/ath/ath10k/gpio.c @@ -0,0 +1,196 @@ +/* + * Copyright (c) 2005-2011 Atheros Communications Inc. + * Copyright (c) 2011-2017 Qualcomm Atheros, Inc. + * Copyright (c) 2018 Sebastian Gottschall <s.gottschall@dd-wrt.com> + * + * Permission to use, copy, modify, and/or distribute this software for any + * purpose with or without fee is hereby granted, provided that the above + * copyright notice and this permission notice appear in all copies. + * + * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES + * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF + * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR + * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES + * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN + * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF + * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. + */ + + +#include <linux/module.h> +#include <linux/firmware.h> +#include <linux/of.h> +#include <linux/dmi.h> +#include <linux/ctype.h> +#include <asm/byteorder.h> +#include <linux/leds.h> +#include <linux/platform_device.h> +#include <linux/version.h> + + +#include "core.h" +#include "wmi.h" +#include "wmi-ops.h" + +#if IS_ENABLED(CONFIG_GPIOLIB) +static int ath10k_gpio_pin_cfg_input(struct gpio_chip *chip, unsigned offset) +{ + struct ath10k_gpiocontrol *gpio = container_of(chip, struct ath10k_gpiocontrol, gchip); + ath10k_wmi_gpio_config(gpio->ar, offset, 1, WMI_GPIO_PULL_NONE, WMI_GPIO_INTTYPE_DISABLE); /* configure to input */ + gpio->gpio_state_dir = 1; + return 0; +} + +/* gpio_chip handler : set GPIO to output */ +static int ath10k_gpio_pin_cfg_output(struct gpio_chip *chip, unsigned offset, + int value) +{ + struct ath10k_gpiocontrol *gpio = container_of(chip, struct ath10k_gpiocontrol, gchip); + + ath10k_wmi_gpio_config(gpio->ar, offset, 0, WMI_GPIO_PULL_NONE, WMI_GPIO_INTTYPE_DISABLE); /* configure to output */ + ath10k_wmi_gpio_output(gpio->ar, offset, value); + gpio->gpio_state_dir = 0; + gpio->gpio_state_pin = value; + return 0; +} + +/* gpio_chip handler : query GPIO direction (0=out, 1=in) */ +static int ath10k_gpio_pin_get_dir(struct gpio_chip *chip, unsigned offset) +{ + struct ath10k_gpiocontrol *gpio = container_of(chip, struct ath10k_gpiocontrol, gchip); + + return gpio->gpio_state_dir; +} + +/* gpio_chip handler : get GPIO pin value */ +static int ath10k_gpio_pin_get(struct gpio_chip *chip, unsigned offset) +{ + struct ath10k_gpiocontrol *gpio = container_of(chip, struct ath10k_gpiocontrol, gchip); + + return gpio->gpio_state_pin; +} + +/* gpio_chip handler : set GPIO pin to value */ +static void ath10k_gpio_pin_set(struct gpio_chip *chip, unsigned offset, + int value) +{ + struct ath10k_gpiocontrol *gpio = container_of(chip, struct ath10k_gpiocontrol, gchip); + + ath10k_wmi_gpio_output(gpio->ar, offset, value); + gpio->gpio_state_pin = value; +} + +/* register GPIO chip */ +static int ath10k_register_gpio_chip(struct ath10k *ar) +{ + + struct ath10k_gpiocontrol *gpio = ar->gpio; + if (!gpio) { + return -ENODEV; + } + gpio->gchip.parent = ar->dev; + gpio->gchip.base = -1; /* determine base automatically */ + gpio->gchip.ngpio = ar->hw_params.gpio_count; + gpio->gchip.label = gpio->label; + gpio->gchip.direction_input = ath10k_gpio_pin_cfg_input; + gpio->gchip.direction_output = ath10k_gpio_pin_cfg_output; + gpio->gchip.get_direction = ath10k_gpio_pin_get_dir; + gpio->gchip.get = ath10k_gpio_pin_get; + gpio->gchip.set = ath10k_gpio_pin_set; + + if (gpiochip_add(&gpio->gchip)) { + dev_err(ar->dev, "Error while registering gpio chip\n"); + return -ENODEV; + } + gpio->gchip.owner = NULL; + gpio->ar = ar; + return 0; +} + +/* remove GPIO chip */ +void ath10k_unregister_gpio_chip(struct ath10k *ar) +{ + struct ath10k_gpiocontrol *gpio = ar->gpio; + if (gpio) { + gpiochip_remove(&gpio->gchip); + } +} + +int ath10k_attach_gpio(struct ath10k *ar) +{ + if (ar->hw_params.led_pin) { /* only attach if non zero since some chipsets are unsupported yet */ + return ath10k_register_gpio_chip(ar); + } + + return -ENODEV; +} + +#endif + + +static void ath10k_led_brightness(struct led_classdev *led_cdev, + enum led_brightness brightness) +{ + struct ath10k_gpiocontrol *gpio = container_of(led_cdev, struct ath10k_gpiocontrol, cdev); + struct gpio_led *led = &gpio->wifi_led; + if (gpio->ar->state == ATH10K_STATE_ON) { + gpio->gpio_state_pin = (brightness != LED_OFF) ^ led->active_low; + ath10k_wmi_gpio_output(gpio->ar, led->gpio, gpio->gpio_state_pin); + } +} + +static int ath10k_add_led(struct ath10k *ar, struct gpio_led *gpioled) +{ + int ret; + struct ath10k_gpiocontrol *gpio = ar->gpio; + gpio->cdev.name = gpioled->name; + gpio->cdev.default_trigger = gpioled->default_trigger; + gpio->cdev.brightness_set = ath10k_led_brightness; + + ret = led_classdev_register(wiphy_dev(ar->hw->wiphy), &gpio->cdev); + if (ret < 0) + return ret; + + return 0; +} + +void ath10k_unregister_led(struct ath10k *ar) +{ + struct ath10k_gpiocontrol *gpio = ar->gpio; + if (gpio) { + led_classdev_unregister(&gpio->cdev); + kfree(gpio); + } +} + +void ath10k_reset_led_pin(struct ath10k *ar) +{ + /* need to reset gpio state */ + if (ar->hw_params.led_pin) { + 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); + } +} + +int ath10k_attach_led(struct ath10k *ar) +{ + struct ath10k_gpiocontrol *gpio; + if (ar->gpio) { /* already registered, ignore */ + return -EINVAL; + } + gpio = kzalloc(sizeof(struct ath10k_gpiocontrol), GFP_KERNEL); + if (!gpio) { + return -ENOMEM; + } + ar->gpio = gpio; + snprintf(gpio->label, sizeof(gpio->label), "ath10k-%s", + wiphy_name(ar->hw->wiphy)); + gpio->wifi_led.active_low = 1; + gpio->wifi_led.gpio = ar->hw_params.led_pin; + gpio->wifi_led.name = gpio->label; + gpio->wifi_led.default_state = LEDS_GPIO_DEFSTATE_KEEP; + + ath10k_add_led(ar, &gpio->wifi_led); + ath10k_reset_led_pin(ar); /* initially we need to configure the led pin to output */ + return 0; +} diff --git a/drivers/net/wireless/ath/ath10k/hw.h b/drivers/net/wireless/ath/ath10k/hw.h index 413b1b4321f7..af91d0202e45 100644 --- a/drivers/net/wireless/ath/ath10k/hw.h +++ b/drivers/net/wireless/ath/ath10k/hw.h @@ -495,6 +495,8 @@ struct ath10k_hw_params { const char *name; u32 patch_load_addr; int uart_pin; + int led_pin; /* 1 for peregrine, 17 for beeliner */ + int gpio_count; /* 24 for peregrine, 35 for beeliner */ u32 otp_exe_param; /* Type of hw cycle counter wraparound logic, for more info diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c index ebb3f1b046f3..6c4925dfcde9 100644 --- a/drivers/net/wireless/ath/ath10k/mac.c +++ b/drivers/net/wireless/ath/ath10k/mac.c @@ -4601,6 +4601,11 @@ static int ath10k_start(struct ieee80211_hw *hw) switch (ar->state) { case ATH10K_STATE_OFF: ar->state = ATH10K_STATE_ON; + /* + * 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_reset_led_pin(ar); break; case ATH10K_STATE_RESTARTING: ar->state = ATH10K_STATE_RESTARTED; diff --git a/drivers/net/wireless/ath/ath10k/wmi-ops.h b/drivers/net/wireless/ath/ath10k/wmi-ops.h index 14093cfdc505..a38d6a8f7ec5 100644 --- a/drivers/net/wireless/ath/ath10k/wmi-ops.h +++ b/drivers/net/wireless/ath/ath10k/wmi-ops.h @@ -197,8 +197,13 @@ struct wmi_ops { (struct ath10k *ar, enum wmi_bss_survey_req_type type); struct sk_buff *(*gen_echo)(struct ath10k *ar, u32 value); + + 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); static inline int @@ -957,6 +962,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_nowait(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_nowait(ar, skb, ar->wmi.cmd->gpio_output_cmdid); +} static inline int ath10k_wmi_dbglog_cfg(struct ath10k *ar, u64 module_enable, u32 log_level) @@ -1034,7 +1068,7 @@ ath10k_wmi_pdev_get_temperature(struct ath10k *ar) if (IS_ERR(skb)) return PTR_ERR(skb); - return ath10k_wmi_cmd_send(ar, skb, + return ath10k_wmi_cmd_send_nowait(ar, skb, ar->wmi.cmd->pdev_get_temperature_cmdid); } diff --git a/drivers/net/wireless/ath/ath10k/wmi-tlv.c b/drivers/net/wireless/ath/ath10k/wmi-tlv.c index ae77a007ae07..2bfba63f1dcf 100644 --- a/drivers/net/wireless/ath/ath10k/wmi-tlv.c +++ b/drivers/net/wireless/ath/ath10k/wmi-tlv.c @@ -3250,6 +3250,69 @@ ath10k_wmi_tlv_op_gen_echo(struct ath10k *ar, u32 value) return skb; } +static struct sk_buff * +ath10k_wmi_tlv_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 wmi_tlv *tlv; + struct sk_buff *skb; + void *ptr; + size_t len; + + len = sizeof(*tlv) + sizeof(*cmd); + skb = ath10k_wmi_alloc_skb(ar, len); + if (!skb) + return ERR_PTR(-ENOMEM); + + ptr = (void *)skb->data; + tlv = ptr; + tlv->tag = __cpu_to_le16(WMI_TLV_TAG_STRUCT_GPIO_CONFIG_CMD); + tlv->len = __cpu_to_le16(sizeof(*cmd)); + + 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); + + ptr += sizeof(*tlv); + ptr += sizeof(*cmd); + + ath10k_dbg(ar, ATH10K_DBG_WMI, "wmi tlv 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_tlv_op_gen_gpio_output(struct ath10k *ar, u32 gpio_num, u32 set) +{ + struct wmi_gpio_output_cmd *cmd; + struct wmi_tlv *tlv; + struct sk_buff *skb; + void *ptr; + size_t len; + + len = sizeof(*tlv) + sizeof(*cmd); + skb = ath10k_wmi_alloc_skb(ar, len); + if (!skb) + return ERR_PTR(-ENOMEM); + + ptr = (void *)skb->data; + tlv = ptr; + tlv->tag = __cpu_to_le16(WMI_TLV_TAG_STRUCT_GPIO_OUTPUT_CMD); + tlv->len = __cpu_to_le16(sizeof(*cmd)); + + cmd = (struct wmi_gpio_output_cmd *)skb->data; + cmd->gpio_num = __cpu_to_le32(gpio_num); + cmd->set = __cpu_to_le32(set); + + ptr += sizeof(*tlv); + ptr += sizeof(*cmd); + + ath10k_dbg(ar, ATH10K_DBG_WMI, "wmi tlv gpio_output gpio_num 0x%08x set 0x%08x\n", gpio_num, set); + return skb; +} + + static struct sk_buff * ath10k_wmi_tlv_op_gen_vdev_spectral_conf(struct ath10k *ar, const struct wmi_vdev_spectral_conf_arg *arg) @@ -3727,6 +3790,8 @@ static const struct wmi_ops wmi_tlv_ops = { .fw_stats_fill = ath10k_wmi_main_op_fw_stats_fill, .get_vdev_subtype = ath10k_wmi_op_get_vdev_subtype, .gen_echo = ath10k_wmi_tlv_op_gen_echo, + .gen_gpio_config = ath10k_wmi_tlv_op_gen_gpio_config, + .gen_gpio_output = ath10k_wmi_tlv_op_gen_gpio_output, .gen_vdev_spectral_conf = ath10k_wmi_tlv_op_gen_vdev_spectral_conf, .gen_vdev_spectral_enable = ath10k_wmi_tlv_op_gen_vdev_spectral_enable, }; diff --git a/drivers/net/wireless/ath/ath10k/wmi.c b/drivers/net/wireless/ath/ath10k/wmi.c index 58dc2189ba49..b56e5a673a8c 100644 --- a/drivers/net/wireless/ath/ath10k/wmi.c +++ b/drivers/net/wireless/ath/ath10k/wmi.c @@ -6646,6 +6646,41 @@ 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) @@ -8153,6 +8188,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 */ @@ -8223,6 +8261,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 */ @@ -8294,6 +8334,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 */ }; @@ -8364,6 +8406,8 @@ static const struct wmi_ops wmi_10_2_4_ops = { .gen_pdev_enable_adaptive_cca = ath10k_wmi_op_gen_pdev_enable_adaptive_cca, .get_vdev_subtype = ath10k_wmi_10_2_4_op_get_vdev_subtype, + .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 */ @@ -8439,6 +8483,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 c7b30ed9015d..dc180a86dc3b 100644 --- a/drivers/net/wireless/ath/ath10k/wmi.h +++ b/drivers/net/wireless/ath/ath10k/wmi.h @@ -2906,6 +2906,42 @@ 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;