[PATCH 0/5] wifi: ath9k: remove platform_data support

[PATCH 0/5] wifi: ath9k: remove platform_data support

[Rosen Penev]

This series removes platform_data support as it's deprecated, unused,
and replaced by OF. Some OF bindings were added to match (improve?) OF
support in ath9k compared to platform_data.

Rosen Penev (5):
  wifi: ath9k: eeprom: remove platform data
  wifi: ath9k: btcoex: remove platform_data
  wifi: ath9k: add extra options to of_init
  dt-bindings: net: wireless: ath9k: add new options
  wifi: ath9k: remove ath9k_platform_data

 .../bindings/net/wireless/qca,ath9k.yaml      | 12 ++++
 .../wireless/ath/ath9k/ath9k_pci_owl_loader.c |  1 -
 drivers/net/wireless/ath/ath9k/btcoex.c       | 16 +----
 drivers/net/wireless/ath/ath9k/eeprom.c       | 12 ----
 drivers/net/wireless/ath/ath9k/hw.c           |  2 +-
 drivers/net/wireless/ath/ath9k/init.c         | 59 +++----------------
 include/linux/ath9k_platform.h                | 51 ----------------
 7 files changed, 25 insertions(+), 128 deletions(-)
 delete mode 100644 include/linux/ath9k_platform.h

-- 
2.46.0

[PATCH 1/5] wifi: ath9k: eeprom: remove platform data

[Rosen Penev]

There are no more board files defining platform data for this driver and
eeprom support through NVMEM has already been implemented. No need to
keep this old functionality around.

Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/net/wireless/ath/ath9k/eeprom.c | 12 ------------
 1 file changed, 12 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/eeprom.c b/drivers/net/wireless/ath/ath9k/eeprom.c
index efb7889142d4..df58dc02e104 100644
--- a/drivers/net/wireless/ath/ath9k/eeprom.c
+++ b/drivers/net/wireless/ath/ath9k/eeprom.c
@@ -15,7 +15,6 @@
  */
 
 #include "hw.h"
-#include <linux/ath9k_platform.h>
 
 void ath9k_hw_analog_shift_regwrite(struct ath_hw *ah, u32 reg, u32 val)
 {
@@ -119,14 +118,6 @@ static bool ath9k_hw_nvram_read_array(u16 *blob, size_t blob_size,
 	return true;
 }
 
-static bool ath9k_hw_nvram_read_pdata(struct ath9k_platform_data *pdata,
-				      off_t offset, u16 *data)
-{
-	return ath9k_hw_nvram_read_array(pdata->eeprom_data,
-					 ARRAY_SIZE(pdata->eeprom_data),
-					 offset, data);
-}
-
 static bool ath9k_hw_nvram_read_firmware(const struct firmware *eeprom_blob,
 					 off_t offset, u16 *data)
 {
@@ -146,15 +137,12 @@ static bool ath9k_hw_nvram_read_nvmem(struct ath_hw *ah, off_t offset,
 bool ath9k_hw_nvram_read(struct ath_hw *ah, u32 off, u16 *data)
 {
 	struct ath_common *common = ath9k_hw_common(ah);
-	struct ath9k_platform_data *pdata = ah->dev->platform_data;
 	bool ret;
 
 	if (ah->nvmem_blob)
 		ret = ath9k_hw_nvram_read_nvmem(ah, off, data);
 	else if (ah->eeprom_blob)
 		ret = ath9k_hw_nvram_read_firmware(ah->eeprom_blob, off, data);
-	else if (pdata && !pdata->use_eeprom)
-		ret = ath9k_hw_nvram_read_pdata(pdata, off, data);
 	else
 		ret = common->bus_ops->eeprom_read(common, off, data);
 
-- 
2.46.0

[PATCH 2/5] wifi: ath9k: btcoex: remove platform_data

[Rosen Penev]

This is completely unused as platform files are no longer used anywhere.

Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/net/wireless/ath/ath9k/btcoex.c | 16 +++-------------
 1 file changed, 3 insertions(+), 13 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/btcoex.c b/drivers/net/wireless/ath/ath9k/btcoex.c
index 9b393a8f7c3a..ad3a3fda1b9c 100644
--- a/drivers/net/wireless/ath/ath9k/btcoex.c
+++ b/drivers/net/wireless/ath/ath9k/btcoex.c
@@ -16,7 +16,6 @@
 
 #include <linux/export.h>
 #include <linux/types.h>
-#include <linux/ath9k_platform.h>
 #include "hw.h"
 
 enum ath_bt_mode {
@@ -115,23 +114,14 @@ static void ath9k_hw_btcoex_pin_init(struct ath_hw *ah, u8 wlanactive_gpio,
 				     u8 btactive_gpio, u8 btpriority_gpio)
 {
 	struct ath_btcoex_hw *btcoex_hw = &ah->btcoex_hw;
-	struct ath9k_platform_data *pdata = ah->dev->platform_data;
 
 	if (btcoex_hw->scheme != ATH_BTCOEX_CFG_2WIRE &&
 	    btcoex_hw->scheme != ATH_BTCOEX_CFG_3WIRE)
 		return;
 
-	/* bt priority GPIO will be ignored by 2 wire scheme */
-	if (pdata && (pdata->bt_active_pin || pdata->bt_priority_pin ||
-		      pdata->wlan_active_pin)) {
-		btcoex_hw->btactive_gpio = pdata->bt_active_pin;
-		btcoex_hw->wlanactive_gpio = pdata->wlan_active_pin;
-		btcoex_hw->btpriority_gpio = pdata->bt_priority_pin;
-	} else {
-		btcoex_hw->btactive_gpio = btactive_gpio;
-		btcoex_hw->wlanactive_gpio = wlanactive_gpio;
-		btcoex_hw->btpriority_gpio = btpriority_gpio;
-	}
+	btcoex_hw->btactive_gpio = btactive_gpio;
+	btcoex_hw->wlanactive_gpio = wlanactive_gpio;
+	btcoex_hw->btpriority_gpio = btpriority_gpio;
 }
 
 void ath9k_hw_btcoex_init_scheme(struct ath_hw *ah)
-- 
2.46.0

[PATCH 3/5] wifi: ath9k: add extra options to of_init

[Rosen Penev]

This is in preparation for removing platform_data support from ath9k.
Except for led-active-high, these are already present downstream in
OpenWrt and never upstreamed.

For tx_gain_buffalo, just set it based on the device. The flag was made
for a single device. Instead of adding an extra option, just look for
the compatible string.

Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/net/wireless/ath/ath9k/init.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/net/wireless/ath/ath9k/init.c b/drivers/net/wireless/ath/ath9k/init.c
index 7fad7e75af6a..88db51ec7e55 100644
--- a/drivers/net/wireless/ath/ath9k/init.c
+++ b/drivers/net/wireless/ath/ath9k/init.c
@@ -670,6 +670,7 @@ static int ath9k_of_init(struct ath_softc *sc)
 	struct ath_common *common = ath9k_hw_common(ah);
 	enum ath_bus_type bus_type = common->bus_ops->ath_bus_type;
 	char eeprom_name[100];
+	u8 led_pin;
 	int ret;
 
 	if (!of_device_is_available(np))
@@ -691,6 +692,12 @@ static int ath9k_of_init(struct ath_softc *sc)
 		ah->ah_flags |= AH_NO_EEP_SWAP;
 	}
 
+	if (!of_property_read_u8(np, "qca,led-pin", &led_pin))
+		ah->led_pin = led_pin;
+
+	ah->config.led_active_high = of_property_read_bool(np, "qca,led-active-high");
+	ah->config.tx_gain_buffalo = of_device_is_compatible(np, "buffalo,wzr-hp-g450h");
+
 	of_get_mac_address(np, common->macaddr);
 
 	return 0;
-- 
2.46.0

[PATCH 4/5] dt-bindings: net: wireless: ath9k: add new options

[Rosen Penev]

These platform_data options are now available for OF.

Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 .../devicetree/bindings/net/wireless/qca,ath9k.yaml  | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/wireless/qca,ath9k.yaml b/Documentation/devicetree/bindings/net/wireless/qca,ath9k.yaml
index 0e5412cff2bc..5c293d558a94 100644
--- a/Documentation/devicetree/bindings/net/wireless/qca,ath9k.yaml
+++ b/Documentation/devicetree/bindings/net/wireless/qca,ath9k.yaml
@@ -44,6 +44,16 @@ properties:
 
   ieee80211-freq-limit: true
 
+  qca,led-active-high:
+    $ref: /schemas/types.yaml#/definitions/flag
+    description:
+      Indicates that the LED pin is active high instead of low
+
+  qca,led-pin:
+    $ref: /schemas/types.yaml#/definitions/uint8
+    description:
+      Sets the GPIO number for the LED pin instead of the default
+
   qca,no-eeprom:
     $ref: /schemas/types.yaml#/definitions/flag
     description:
@@ -75,6 +85,8 @@ examples:
         reg = <0 0 0 0 0>;
         interrupts = <3>;
         qca,no-eeprom;
+        qca,led-active-high;
+        qca,led-pin = /bits/ 8 <11>;
       };
     };
   - |
-- 
2.46.0

[PATCH 5/5] wifi: ath9k: remove ath9k_platform_data

[Rosen Penev]

Completely unused here in favor of Device Tree based setup. The DT code
in here should currently match what is available with platform files.
Any such lapse can always be added.

Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 .../wireless/ath/ath9k/ath9k_pci_owl_loader.c |  1 -
 drivers/net/wireless/ath/ath9k/hw.c           |  2 +-
 drivers/net/wireless/ath/ath9k/init.c         | 52 +------------------
 include/linux/ath9k_platform.h                | 51 ------------------
 4 files changed, 3 insertions(+), 103 deletions(-)
 delete mode 100644 include/linux/ath9k_platform.h

diff --git a/drivers/net/wireless/ath/ath9k/ath9k_pci_owl_loader.c b/drivers/net/wireless/ath/ath9k/ath9k_pci_owl_loader.c
index 004ca5f536be..fe1013a3a588 100644
--- a/drivers/net/wireless/ath/ath9k/ath9k_pci_owl_loader.c
+++ b/drivers/net/wireless/ath/ath9k/ath9k_pci_owl_loader.c
@@ -18,7 +18,6 @@
 #include <linux/pci.h>
 #include <linux/delay.h>
 #include <linux/platform_device.h>
-#include <linux/ath9k_platform.h>
 #include <linux/nvmem-consumer.h>
 #include <linux/workqueue.h>
 
diff --git a/drivers/net/wireless/ath/ath9k/hw.c b/drivers/net/wireless/ath/ath9k/hw.c
index 04a4b9ea61c3..36db734c74ae 100644
--- a/drivers/net/wireless/ath/ath9k/hw.c
+++ b/drivers/net/wireless/ath/ath9k/hw.c
@@ -490,7 +490,7 @@ static void ath9k_hw_init_macaddr(struct ath_hw *ah)
 	u16 eeval;
 	static const u32 EEP_MAC[] = { EEP_MAC_LSW, EEP_MAC_MID, EEP_MAC_MSW };
 
-	/* MAC address may already be loaded via ath9k_platform_data */
+	/* MAC address may already be loaded via NVMEM */
 	if (is_valid_ether_addr(common->macaddr))
 		return;
 
diff --git a/drivers/net/wireless/ath/ath9k/init.c b/drivers/net/wireless/ath/ath9k/init.c
index 88db51ec7e55..58dbf5e13151 100644
--- a/drivers/net/wireless/ath/ath9k/init.c
+++ b/drivers/net/wireless/ath/ath9k/init.c
@@ -18,7 +18,6 @@
 
 #include <linux/dma-mapping.h>
 #include <linux/slab.h>
-#include <linux/ath9k_platform.h>
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/of_net.h>
@@ -583,8 +582,8 @@ static int ath9k_nvmem_request_eeprom(struct ath_softc *sc)
 
 		/* nvmem cell might not be defined, or the nvmem
 		 * subsystem isn't included. In this case, follow
-		 * the established "just return 0;" convention of
-		 * ath9k_init_platform to say:
+		 * the established "just return 0;" convention
+		 * to say:
 		 * "All good. Nothing to see here. Please go on."
 		 */
 		if (err == -ENOENT || err == -EOPNOTSUPP)
@@ -620,49 +619,6 @@ static int ath9k_nvmem_request_eeprom(struct ath_softc *sc)
 	return 0;
 }
 
-static int ath9k_init_platform(struct ath_softc *sc)
-{
-	struct ath9k_platform_data *pdata = sc->dev->platform_data;
-	struct ath_hw *ah = sc->sc_ah;
-	struct ath_common *common = ath9k_hw_common(ah);
-	int ret;
-
-	if (!pdata)
-		return 0;
-
-	if (!pdata->use_eeprom) {
-		ah->ah_flags &= ~AH_USE_EEPROM;
-		ah->gpio_mask = pdata->gpio_mask;
-		ah->gpio_val = pdata->gpio_val;
-		ah->led_pin = pdata->led_pin;
-		ah->is_clk_25mhz = pdata->is_clk_25mhz;
-		ah->get_mac_revision = pdata->get_mac_revision;
-		ah->external_reset = pdata->external_reset;
-		ah->disable_2ghz = pdata->disable_2ghz;
-		ah->disable_5ghz = pdata->disable_5ghz;
-
-		if (!pdata->endian_check)
-			ah->ah_flags |= AH_NO_EEP_SWAP;
-	}
-
-	if (pdata->eeprom_name) {
-		ret = ath9k_eeprom_request(sc, pdata->eeprom_name);
-		if (ret)
-			return ret;
-	}
-
-	if (pdata->led_active_high)
-		ah->config.led_active_high = true;
-
-	if (pdata->tx_gain_buffalo)
-		ah->config.tx_gain_buffalo = true;
-
-	if (pdata->macaddr)
-		ether_addr_copy(common->macaddr, pdata->macaddr);
-
-	return 0;
-}
-
 static int ath9k_of_init(struct ath_softc *sc)
 {
 	struct device_node *np = sc->dev->of_node;
@@ -755,10 +711,6 @@ static int ath9k_init_softc(u16 devid, struct ath_softc *sc,
 	 */
 	ath9k_init_pcoem_platform(sc);
 
-	ret = ath9k_init_platform(sc);
-	if (ret)
-		return ret;
-
 	ret = ath9k_of_init(sc);
 	if (ret)
 		return ret;
diff --git a/include/linux/ath9k_platform.h b/include/linux/ath9k_platform.h
deleted file mode 100644
index 76860a461ed2..000000000000
--- a/include/linux/ath9k_platform.h
+++ /dev/null
@@ -1,51 +0,0 @@
-/*
- * Copyright (c) 2008 Atheros Communications Inc.
- * Copyright (c) 2009 Gabor Juhos <juhosg@openwrt.org>
- * Copyright (c) 2009 Imre Kaloz <kaloz@openwrt.org>
- *
- * 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.
- */
-
-#ifndef _LINUX_ATH9K_PLATFORM_H
-#define _LINUX_ATH9K_PLATFORM_H
-
-#define ATH9K_PLAT_EEP_MAX_WORDS	2048
-
-struct ath9k_platform_data {
-	const char *eeprom_name;
-
-	u16 eeprom_data[ATH9K_PLAT_EEP_MAX_WORDS];
-	u8 *macaddr;
-
-	int led_pin;
-	u32 gpio_mask;
-	u32 gpio_val;
-
-	u32 bt_active_pin;
-	u32 bt_priority_pin;
-	u32 wlan_active_pin;
-
-	bool endian_check;
-	bool is_clk_25mhz;
-	bool tx_gain_buffalo;
-	bool disable_2ghz;
-	bool disable_5ghz;
-	bool led_active_high;
-
-	int (*get_mac_revision)(void);
-	int (*external_reset)(void);
-
-	bool use_eeprom;
-};
-
-#endif /* _LINUX_ATH9K_PLATFORM_H */
-- 
2.46.0

Re: [PATCH 4/5] dt-bindings: net: wireless: ath9k: add new options

[Krzysztof Kozlowski]

On 05/09/2024 20:09, Rosen Penev wrote:
> These platform_data options are now available for OF.

This explains nothing. Describe the hardware, not driver.

> 
> Signed-off-by: Rosen Penev <rosenp@gmail.com>

<form letter>
Please use scripts/get_maintainers.pl to get a list of necessary people
and lists to CC. It might happen, that command when run on an older
kernel, gives you outdated entries. Therefore please be sure you base
your patches on recent Linux kernel.

Tools like b4 or scripts/get_maintainer.pl provide you proper list of
people, so fix your workflow. Tools might also fail if you work on some
ancient tree (don't, instead use mainline) or work on fork of kernel
(don't, instead use mainline). Just use b4 and everything should be
fine, although remember about `b4 prep --auto-to-cc` if you added new
patches to the patchset.

You missed at least devicetree list (maybe more), so this won't be
tested by automated tooling. Performing review on untested code might be
a waste of time.

Please kindly resend and include all necessary To/Cc entries.
</form letter>

> ---
>  .../devicetree/bindings/net/wireless/qca,ath9k.yaml  | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/wireless/qca,ath9k.yaml b/Documentation/devicetree/bindings/net/wireless/qca,ath9k.yaml
> index 0e5412cff2bc..5c293d558a94 100644
> --- a/Documentation/devicetree/bindings/net/wireless/qca,ath9k.yaml
> +++ b/Documentation/devicetree/bindings/net/wireless/qca,ath9k.yaml
> @@ -44,6 +44,16 @@ properties:
>  
>    ieee80211-freq-limit: true
>  
> +  qca,led-active-high:
> +    $ref: /schemas/types.yaml#/definitions/flag
> +    description:
> +      Indicates that the LED pin is active high instead of low

Where is DTS user of it?

Best regards,
Krzysztof

Re: [PATCH 3/5] wifi: ath9k: add extra options to of_init

[Krzysztof Kozlowski]

On 05/09/2024 20:09, Rosen Penev wrote:
> This is in preparation for removing platform_data support from ath9k.
> Except for led-active-high, these are already present downstream in
> OpenWrt and never upstreamed.
> 
> For tx_gain_buffalo, just set it based on the device. The flag was made
> for a single device. Instead of adding an extra option, just look for
> the compatible string.
> 

Bindings are before users.

> Signed-off-by: Rosen Penev <rosenp@gmail.com>
> ---
>  drivers/net/wireless/ath/ath9k/init.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/net/wireless/ath/ath9k/init.c b/drivers/net/wireless/ath/ath9k/init.c
> index 7fad7e75af6a..88db51ec7e55 100644
> --- a/drivers/net/wireless/ath/ath9k/init.c
> +++ b/drivers/net/wireless/ath/ath9k/init.c
> @@ -670,6 +670,7 @@ static int ath9k_of_init(struct ath_softc *sc)
>  	struct ath_common *common = ath9k_hw_common(ah);
>  	enum ath_bus_type bus_type = common->bus_ops->ath_bus_type;
>  	char eeprom_name[100];
> +	u8 led_pin;
>  	int ret;
>  
>  	if (!of_device_is_available(np))
> @@ -691,6 +692,12 @@ static int ath9k_of_init(struct ath_softc *sc)
>  		ah->ah_flags |= AH_NO_EEP_SWAP;
>  	}
>  
> +	if (!of_property_read_u8(np, "qca,led-pin", &led_pin))
> +		ah->led_pin = led_pin;
> +
> +	ah->config.led_active_high = of_property_read_bool(np, "qca,led-active-high");
> +	ah->config.tx_gain_buffalo = of_device_is_compatible(np, "buffalo,wzr-hp-g450h");

There is no such compatible.

Please run scripts/checkpatch.pl and fix reported warnings. Then please
run `scripts/checkpatch.pl --strict` and (probably) fix more warnings.
Some warnings can be ignored, especially from --strict run, but the code
here looks like it needs a fix. Feel free to get in touch if the warning
is not clear.

Best regards,
Krzysztof

Re: [PATCH 4/5] dt-bindings: net: wireless: ath9k: add new options

[Krzysztof Kozlowski]

On 05/09/2024 20:09, Rosen Penev wrote:
> These platform_data options are now available for OF.
> 
> Signed-off-by: Rosen Penev <rosenp@gmail.com>
> ---
>  .../devicetree/bindings/net/wireless/qca,ath9k.yaml  | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/wireless/qca,ath9k.yaml b/Documentation/devicetree/bindings/net/wireless/qca,ath9k.yaml
> index 0e5412cff2bc..5c293d558a94 100644
> --- a/Documentation/devicetree/bindings/net/wireless/qca,ath9k.yaml
> +++ b/Documentation/devicetree/bindings/net/wireless/qca,ath9k.yaml
> @@ -44,6 +44,16 @@ properties:
>  
>    ieee80211-freq-limit: true
>  
> +  qca,led-active-high:
> +    $ref: /schemas/types.yaml#/definitions/flag
> +    description:
> +      Indicates that the LED pin is active high instead of low
> +
> +  qca,led-pin:

So this all looks like re-inventing standard GPIOs, at least for devices
using GPIOLIB. Your commit msg needs proper rationale.

Best regards,
Krzysztof

Re: [PATCH 4/5] dt-bindings: net: wireless: ath9k: add new options

[Rosen Penev]

On Thu, Sep 5, 2024 at 2:29 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On 05/09/2024 20:09, Rosen Penev wrote:
> > These platform_data options are now available for OF.
>
> This explains nothing. Describe the hardware, not driver.
>
> >
> > Signed-off-by: Rosen Penev <rosenp@gmail.com>
>
> <form letter>
> Please use scripts/get_maintainers.pl to get a list of necessary people
> and lists to CC. It might happen, that command when run on an older
> kernel, gives you outdated entries. Therefore please be sure you base
> your patches on recent Linux kernel.
>
> Tools like b4 or scripts/get_maintainer.pl provide you proper list of
> people, so fix your workflow. Tools might also fail if you work on some
> ancient tree (don't, instead use mainline) or work on fork of kernel
> (don't, instead use mainline). Just use b4 and everything should be
> fine, although remember about `b4 prep --auto-to-cc` if you added new
> patches to the patchset.
>
> You missed at least devicetree list (maybe more), so this won't be
> tested by automated tooling. Performing review on untested code might be
> a waste of time.
>
> Please kindly resend and include all necessary To/Cc entries.
Will do.
> </form letter>
>
> > ---
> >  .../devicetree/bindings/net/wireless/qca,ath9k.yaml  | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/net/wireless/qca,ath9k.yaml b/Documentation/devicetree/bindings/net/wireless/qca,ath9k.yaml
> > index 0e5412cff2bc..5c293d558a94 100644
> > --- a/Documentation/devicetree/bindings/net/wireless/qca,ath9k.yaml
> > +++ b/Documentation/devicetree/bindings/net/wireless/qca,ath9k.yaml
> > @@ -44,6 +44,16 @@ properties:
> >
> >    ieee80211-freq-limit: true
> >
> > +  qca,led-active-high:
> > +    $ref: /schemas/types.yaml#/definitions/flag
> > +    description:
> > +      Indicates that the LED pin is active high instead of low
>
> Where is DTS user of it?
None at this time. There are only downstream users.

This maps to the led_active_high in ath9k_platform_device , which gets
removed in a subsequent commit.

drivers/net/wireless/mediatek/mt76/mac80211.c uses led-sources as the
name for the same purpose. Maybe rename?

>
> Best regards,
> Krzysztof
>

Re: [PATCH 4/5] dt-bindings: net: wireless: ath9k: add new options

[Rosen Penev]

On Thu, Sep 5, 2024 at 2:33 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On 05/09/2024 20:09, Rosen Penev wrote:
> > These platform_data options are now available for OF.
> >
> > Signed-off-by: Rosen Penev <rosenp@gmail.com>
> > ---
> >  .../devicetree/bindings/net/wireless/qca,ath9k.yaml  | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/net/wireless/qca,ath9k.yaml b/Documentation/devicetree/bindings/net/wireless/qca,ath9k.yaml
> > index 0e5412cff2bc..5c293d558a94 100644
> > --- a/Documentation/devicetree/bindings/net/wireless/qca,ath9k.yaml
> > +++ b/Documentation/devicetree/bindings/net/wireless/qca,ath9k.yaml
> > @@ -44,6 +44,16 @@ properties:
> >
> >    ieee80211-freq-limit: true
> >
> > +  qca,led-active-high:
> > +    $ref: /schemas/types.yaml#/definitions/flag
> > +    description:
> > +      Indicates that the LED pin is active high instead of low
> > +
> > +  qca,led-pin:
>
> So this all looks like re-inventing standard GPIOs, at least for devices
> using GPIOLIB. Your commit msg needs proper rationale.
git grep led_classdev_register drivers/net/wireless/ | wc -l
16

Both old and new drivers register their own GPIO devices to control LEDs.
>
> Best regards,
> Krzysztof
>

Re: [PATCH 3/5] wifi: ath9k: add extra options to of_init

[Rosen Penev]

On Thu, Sep 5, 2024 at 2:30 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On 05/09/2024 20:09, Rosen Penev wrote:
> > This is in preparation for removing platform_data support from ath9k.
> > Except for led-active-high, these are already present downstream in
> > OpenWrt and never upstreamed.
> >
> > For tx_gain_buffalo, just set it based on the device. The flag was made
> > for a single device. Instead of adding an extra option, just look for
> > the compatible string.
> >
>
> Bindings are before users.
>
> > Signed-off-by: Rosen Penev <rosenp@gmail.com>
> > ---
> >  drivers/net/wireless/ath/ath9k/init.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> >
> > diff --git a/drivers/net/wireless/ath/ath9k/init.c b/drivers/net/wireless/ath/ath9k/init.c
> > index 7fad7e75af6a..88db51ec7e55 100644
> > --- a/drivers/net/wireless/ath/ath9k/init.c
> > +++ b/drivers/net/wireless/ath/ath9k/init.c
> > @@ -670,6 +670,7 @@ static int ath9k_of_init(struct ath_softc *sc)
> >       struct ath_common *common = ath9k_hw_common(ah);
> >       enum ath_bus_type bus_type = common->bus_ops->ath_bus_type;
> >       char eeprom_name[100];
> > +     u8 led_pin;
> >       int ret;
> >
> >       if (!of_device_is_available(np))
> > @@ -691,6 +692,12 @@ static int ath9k_of_init(struct ath_softc *sc)
> >               ah->ah_flags |= AH_NO_EEP_SWAP;
> >       }
> >
> > +     if (!of_property_read_u8(np, "qca,led-pin", &led_pin))
> > +             ah->led_pin = led_pin;
> > +
> > +     ah->config.led_active_high = of_property_read_bool(np, "qca,led-active-high");
> > +     ah->config.tx_gain_buffalo = of_device_is_compatible(np, "buffalo,wzr-hp-g450h");
>
> There is no such compatible.
Correct. There is none at this time. As the commit message states,
tx_gain_buffalo was introduced for this device and this device only.
If it is to be upstreamed, this would be the name for it. OTOH, the
device is old and probably can't run modern kernels comfortably. It
might make sense to eventually remove this completely.

My goal with this commit is to match ath9k_platform_device capability
to OF before removing it.
>
> Please run scripts/checkpatch.pl and fix reported warnings. Then please
> run `scripts/checkpatch.pl --strict` and (probably) fix more warnings.
> Some warnings can be ignored, especially from --strict run, but the code
> here looks like it needs a fix. Feel free to get in touch if the warning
> is not clear.
>
> Best regards,
> Krzysztof
>

Re: [PATCH 4/5] dt-bindings: net: wireless: ath9k: add new options

[Krzysztof Kozlowski]

On 06/09/2024 00:35, Rosen Penev wrote:
> On Thu, Sep 5, 2024 at 2:29 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>>
>> On 05/09/2024 20:09, Rosen Penev wrote:
>>> These platform_data options are now available for OF.
>>
>> This explains nothing. Describe the hardware, not driver.
>>
>>>
>>> Signed-off-by: Rosen Penev <rosenp@gmail.com>
>>
>> <form letter>
>> Please use scripts/get_maintainers.pl to get a list of necessary people
>> and lists to CC. It might happen, that command when run on an older
>> kernel, gives you outdated entries. Therefore please be sure you base
>> your patches on recent Linux kernel.
>>
>> Tools like b4 or scripts/get_maintainer.pl provide you proper list of
>> people, so fix your workflow. Tools might also fail if you work on some
>> ancient tree (don't, instead use mainline) or work on fork of kernel
>> (don't, instead use mainline). Just use b4 and everything should be
>> fine, although remember about `b4 prep --auto-to-cc` if you added new
>> patches to the patchset.
>>
>> You missed at least devicetree list (maybe more), so this won't be
>> tested by automated tooling. Performing review on untested code might be
>> a waste of time.
>>
>> Please kindly resend and include all necessary To/Cc entries.
> Will do.
>> </form letter>
>>
>>> ---
>>>  .../devicetree/bindings/net/wireless/qca,ath9k.yaml  | 12 ++++++++++++
>>>  1 file changed, 12 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/net/wireless/qca,ath9k.yaml b/Documentation/devicetree/bindings/net/wireless/qca,ath9k.yaml
>>> index 0e5412cff2bc..5c293d558a94 100644
>>> --- a/Documentation/devicetree/bindings/net/wireless/qca,ath9k.yaml
>>> +++ b/Documentation/devicetree/bindings/net/wireless/qca,ath9k.yaml
>>> @@ -44,6 +44,16 @@ properties:
>>>
>>>    ieee80211-freq-limit: true
>>>
>>> +  qca,led-active-high:
>>> +    $ref: /schemas/types.yaml#/definitions/flag
>>> +    description:
>>> +      Indicates that the LED pin is active high instead of low
>>
>> Where is DTS user of it?
> None at this time. There are only downstream users.

We do not add stuff for downstream.

> 
> This maps to the led_active_high in ath9k_platform_device , which gets
> removed in a subsequent commit.
> 
> drivers/net/wireless/mediatek/mt76/mac80211.c uses led-sources as the
> name for the same purpose. Maybe rename?

Again, you add undocumented, unused compatibles along with unused
properties. We don't care about downstream kernels. You need upstream users.

Best regards,
Krzysztof

Re: [PATCH 4/5] dt-bindings: net: wireless: ath9k: add new options

[Rosen Penev]

On Fri, Sep 6, 2024 at 12:18 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On 06/09/2024 00:35, Rosen Penev wrote:
> > On Thu, Sep 5, 2024 at 2:29 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> >>
> >> On 05/09/2024 20:09, Rosen Penev wrote:
> >>> These platform_data options are now available for OF.
> >>
> >> This explains nothing. Describe the hardware, not driver.
> >>
> >>>
> >>> Signed-off-by: Rosen Penev <rosenp@gmail.com>
> >>
> >> <form letter>
> >> Please use scripts/get_maintainers.pl to get a list of necessary people
> >> and lists to CC. It might happen, that command when run on an older
> >> kernel, gives you outdated entries. Therefore please be sure you base
> >> your patches on recent Linux kernel.
> >>
> >> Tools like b4 or scripts/get_maintainer.pl provide you proper list of
> >> people, so fix your workflow. Tools might also fail if you work on some
> >> ancient tree (don't, instead use mainline) or work on fork of kernel
> >> (don't, instead use mainline). Just use b4 and everything should be
> >> fine, although remember about `b4 prep --auto-to-cc` if you added new
> >> patches to the patchset.
> >>
> >> You missed at least devicetree list (maybe more), so this won't be
> >> tested by automated tooling. Performing review on untested code might be
> >> a waste of time.
> >>
> >> Please kindly resend and include all necessary To/Cc entries.
> > Will do.
> >> </form letter>
> >>
> >>> ---
> >>>  .../devicetree/bindings/net/wireless/qca,ath9k.yaml  | 12 ++++++++++++
> >>>  1 file changed, 12 insertions(+)
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/net/wireless/qca,ath9k.yaml b/Documentation/devicetree/bindings/net/wireless/qca,ath9k.yaml
> >>> index 0e5412cff2bc..5c293d558a94 100644
> >>> --- a/Documentation/devicetree/bindings/net/wireless/qca,ath9k.yaml
> >>> +++ b/Documentation/devicetree/bindings/net/wireless/qca,ath9k.yaml
> >>> @@ -44,6 +44,16 @@ properties:
> >>>
> >>>    ieee80211-freq-limit: true
> >>>
> >>> +  qca,led-active-high:
> >>> +    $ref: /schemas/types.yaml#/definitions/flag
> >>> +    description:
> >>> +      Indicates that the LED pin is active high instead of low
> >>
> >> Where is DTS user of it?
> > None at this time. There are only downstream users.
>
> We do not add stuff for downstream.
>
> >
> > This maps to the led_active_high in ath9k_platform_device , which gets
> > removed in a subsequent commit.
> >
> > drivers/net/wireless/mediatek/mt76/mac80211.c uses led-sources as the
> > name for the same purpose. Maybe rename?
>
> Again, you add undocumented, unused compatibles along with unused
> properties. We don't care about downstream kernels. You need upstream users.
OK. Will remove in v2.
>
> Best regards,
> Krzysztof
>