diff mbox series

[v7,2/7] wifi: wilc1000: Clean up usage of wilc_get_chipid()

Message ID 20241003111529.41232-2-marex@denx.de (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series [v7,1/7] dt-bindings: wireless: wilc1000: Document WILC3000 compatible string | expand

Checks

Context Check Description
netdev/tree_selection success Not a local patch

Commit Message

Marek Vasut Oct. 3, 2024, 11:14 a.m. UTC 
Reduce the use of wilc_get_chipid(), use cached chip ID wherever
possible. Remove duplicated partial chip ID read implementations
from the driver. Update wilc_get_chipid() to always read the chip
ID out of the hardware and update the cached chip ID, and make it
return a proper return value instead of a chipid. Call wilc_get_chipid()
early to make the cached chip ID available to various sites using
is_wilc1000() to access the cached chip ID.

Reviewed-by: Alexis Lothoré <alexis.lothore@bootlin.com>
Signed-off-by: Marek Vasut <marex@denx.de>
---
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Adham Abozaeid <adham.abozaeid@microchip.com>
Cc: Ajay Singh <ajay.kathat@microchip.com>
Cc: Alexis Lothoré <alexis.lothore@bootlin.com>
Cc: Claudiu Beznea <claudiu.beznea@tuxon.dev>
Cc: Conor Dooley <conor+dt@kernel.org>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Kalle Valo <kvalo@kernel.org>
Cc: Krzysztof Kozlowski <krzk+dt@kernel.org>
Cc: Paolo Abeni <pabeni@redhat.com>
Cc: Rob Herring <robh@kernel.org>
Cc: devicetree@vger.kernel.org
Cc: linux-wireless@vger.kernel.org
Cc: netdev@vger.kernel.org
---
V2: New patch
V3: - Undo setup in wilc_wlan_init() if chip is neither wilc1000 or wilc3000
    - Make wilc_get_chipid() return proper return value
V4: - Drop wilc_get_chipid() from netdev.c
    - Add RB from Alexis
V5: No change
V6: - Get chipid early in probe
    - Drop RB
    - Rebase on next-20240926
V7: - Update commit message
    - Reinstate RB
    - Rebase on next-20241003 / wireless-next/main 5a4d42c1688c
      with v2 wifi: wilc1000: Keep slot powered on during suspend/resume
---
 .../net/wireless/microchip/wilc1000/netdev.c  |  6 +-
 .../net/wireless/microchip/wilc1000/sdio.c    | 17 +----
 drivers/net/wireless/microchip/wilc1000/spi.c |  2 +-
 .../net/wireless/microchip/wilc1000/wlan.c    | 74 +++++++++++--------
 .../net/wireless/microchip/wilc1000/wlan.h    |  2 +-
 5 files changed, 52 insertions(+), 49 deletions(-)

Comments

Alexis Lothoré (eBPF Foundation) Oct. 3, 2024, 5:33 p.m. UTC | #1 
On 10/3/24 13:14, Marek Vasut wrote:
> Reduce the use of wilc_get_chipid(), use cached chip ID wherever
> possible. Remove duplicated partial chip ID read implementations
> from the driver. Update wilc_get_chipid() to always read the chip
> ID out of the hardware and update the cached chip ID, and make it
> return a proper return value instead of a chipid. Call wilc_get_chipid()
> early to make the cached chip ID available to various sites using
> is_wilc1000() to access the cached chip ID.
> 
> Reviewed-by: Alexis Lothoré <alexis.lothore@bootlin.com>
> Signed-off-by: Marek Vasut <marex@denx.de>

[...]

> +int wilc_get_chipid(struct wilc *wilc)
> +{
> +	u32 chipid = 0;
> +	u32 rfrevid = 0;
> +
> +	if (wilc->chipid == 0) {
> +		wilc->hif_func->hif_read_reg(wilc, WILC_CHIPID, &chipid);
> +		wilc->hif_func->hif_read_reg(wilc, WILC_RF_REVISION_ID,
> +					     &rfrevid);
> +		if (!is_wilc1000(chipid)) {
> +			wilc->chipid = 0;
> +			return -EINVAL;
> +		}
> +		if (chipid == WILC_1000_BASE_ID_2A) { /* 0x1002A0 */
> +			if (rfrevid != 0x1)
> +				chipid = WILC_1000_BASE_ID_2A_REV1;
> +		} else if (chipid == WILC_1000_BASE_ID_2B) { /* 0x1002B0 */
> +			if (rfrevid == 0x4)
> +				chipid = WILC_1000_BASE_ID_2B_REV1;
> +			else if (rfrevid != 0x3)
> +				chipid = WILC_1000_BASE_ID_2B_REV2;
> +		}
> +
> +		wilc->chipid = chipid;
> +	}
> +
> +	return 0;
> +}

My bad for not having spotted it in v6, but you are still missing an
EXPORT_SYMBOL_GPL(wilc_get_chipid) here, making the build fail if wilc support
is built as module:

ERROR: modpost: "wilc_get_chipid"
[drivers/net/wireless/microchip/wilc1000/wilc1000-sdio.ko] undefined!
ERROR: modpost: "wilc_get_chipid"
[drivers/net/wireless/microchip/wilc1000/wilc1000-spi.ko] undefined!
make[2]: *** [scripts/Makefile.modpost:145: Module.symvers] Error 1
make[1]: *** [/home/alexis/src/microchip/linux/Makefile:1878: modpost] Error 2
make: *** [Makefile:224: __sub-make] Error 2
Marek Vasut Oct. 3, 2024, 6:31 p.m. UTC | #2 
On 10/3/24 7:33 PM, Alexis Lothoré wrote:
> On 10/3/24 13:14, Marek Vasut wrote:
>> Reduce the use of wilc_get_chipid(), use cached chip ID wherever
>> possible. Remove duplicated partial chip ID read implementations
>> from the driver. Update wilc_get_chipid() to always read the chip
>> ID out of the hardware and update the cached chip ID, and make it
>> return a proper return value instead of a chipid. Call wilc_get_chipid()
>> early to make the cached chip ID available to various sites using
>> is_wilc1000() to access the cached chip ID.
>>
>> Reviewed-by: Alexis Lothoré <alexis.lothore@bootlin.com>
>> Signed-off-by: Marek Vasut <marex@denx.de>
> 
> [...]
> 
>> +int wilc_get_chipid(struct wilc *wilc)
>> +{
>> +	u32 chipid = 0;
>> +	u32 rfrevid = 0;
>> +
>> +	if (wilc->chipid == 0) {
>> +		wilc->hif_func->hif_read_reg(wilc, WILC_CHIPID, &chipid);
>> +		wilc->hif_func->hif_read_reg(wilc, WILC_RF_REVISION_ID,
>> +					     &rfrevid);
>> +		if (!is_wilc1000(chipid)) {
>> +			wilc->chipid = 0;
>> +			return -EINVAL;
>> +		}
>> +		if (chipid == WILC_1000_BASE_ID_2A) { /* 0x1002A0 */
>> +			if (rfrevid != 0x1)
>> +				chipid = WILC_1000_BASE_ID_2A_REV1;
>> +		} else if (chipid == WILC_1000_BASE_ID_2B) { /* 0x1002B0 */
>> +			if (rfrevid == 0x4)
>> +				chipid = WILC_1000_BASE_ID_2B_REV1;
>> +			else if (rfrevid != 0x3)
>> +				chipid = WILC_1000_BASE_ID_2B_REV2;
>> +		}
>> +
>> +		wilc->chipid = chipid;
>> +	}
>> +
>> +	return 0;
>> +}
> 
> My bad for not having spotted it in v6, but you are still missing an
> EXPORT_SYMBOL_GPL(wilc_get_chipid) here, making the build fail if wilc support
> is built as module:
> 
> ERROR: modpost: "wilc_get_chipid"
> [drivers/net/wireless/microchip/wilc1000/wilc1000-sdio.ko] undefined!
> ERROR: modpost: "wilc_get_chipid"
> [drivers/net/wireless/microchip/wilc1000/wilc1000-spi.ko] undefined!
> make[2]: *** [scripts/Makefile.modpost:145: Module.symvers] Error 1
> make[1]: *** [/home/alexis/src/microchip/linux/Makefile:1878: modpost] Error 2
> make: *** [Makefile:224: __sub-make] Error 2
Fixed in V8, thanks.

Before I send V8, can you have a look at the last two patches in this 
series? They need some RB/TB.

Thanks !
diff mbox series

Patch

diff --git a/drivers/net/wireless/microchip/wilc1000/netdev.c b/drivers/net/wireless/microchip/wilc1000/netdev.c
index 9ecf3fb29b558..086e70d833e06 100644
--- a/drivers/net/wireless/microchip/wilc1000/netdev.c
+++ b/drivers/net/wireless/microchip/wilc1000/netdev.c
@@ -195,13 +195,13 @@  static int wilc_wlan_get_firmware(struct net_device *dev)
 {
 	struct wilc_vif *vif = netdev_priv(dev);
 	struct wilc *wilc = vif->wilc;
-	int chip_id;
 	const struct firmware *wilc_fw;
 	int ret;
 
-	chip_id = wilc_get_chipid(wilc, false);
+	if (!is_wilc1000(wilc->chipid))
+		return -EINVAL;
 
-	netdev_info(dev, "ChipID [%x] loading firmware [%s]\n", chip_id,
+	netdev_info(dev, "WILC1000 loading firmware [%s]\n",
 		    WILC1000_FW(WILC1000_API_VER));
 
 	ret = request_firmware(&wilc_fw, WILC1000_FW(WILC1000_API_VER),
diff --git a/drivers/net/wireless/microchip/wilc1000/sdio.c b/drivers/net/wireless/microchip/wilc1000/sdio.c
index d67662b6b2a1a..7bc6d788f33a7 100644
--- a/drivers/net/wireless/microchip/wilc1000/sdio.c
+++ b/drivers/net/wireless/microchip/wilc1000/sdio.c
@@ -182,6 +182,10 @@  static int wilc_sdio_probe(struct sdio_func *func,
 
 	wilc_sdio_init(wilc, false);
 
+	ret = wilc_get_chipid(wilc);
+	if (ret)
+		goto dispose_irq;
+
 	ret = wilc_load_mac_from_nv(wilc);
 	if (ret) {
 		pr_err("Can not retrieve MAC address from chip\n");
@@ -667,7 +671,6 @@  static int wilc_sdio_init(struct wilc *wilc, bool resume)
 	struct wilc_sdio *sdio_priv = wilc->bus_data;
 	struct sdio_cmd52 cmd;
 	int loop, ret;
-	u32 chipid;
 
 	/**
 	 *      function 0 csa enable
@@ -756,18 +759,6 @@  static int wilc_sdio_init(struct wilc *wilc, bool resume)
 		return ret;
 	}
 
-	/**
-	 *      make sure can read back chip id correctly
-	 **/
-	if (!resume) {
-		ret = wilc_sdio_read_reg(wilc, WILC_CHIPID, &chipid);
-		if (ret) {
-			dev_err(&func->dev, "Fail cmd read chip id...\n");
-			return ret;
-		}
-		dev_err(&func->dev, "chipid (%08x)\n", chipid);
-	}
-
 	sdio_priv->isinit = true;
 	return 0;
 }
diff --git a/drivers/net/wireless/microchip/wilc1000/spi.c b/drivers/net/wireless/microchip/wilc1000/spi.c
index 05b577b1068ea..81cf82c9175ef 100644
--- a/drivers/net/wireless/microchip/wilc1000/spi.c
+++ b/drivers/net/wireless/microchip/wilc1000/spi.c
@@ -245,7 +245,7 @@  static int wilc_bus_probe(struct spi_device *spi)
 	if (ret)
 		goto power_down;
 
-	ret = wilc_validate_chipid(wilc);
+	ret = wilc_get_chipid(wilc);
 	if (ret)
 		goto power_down;
 
diff --git a/drivers/net/wireless/microchip/wilc1000/wlan.c b/drivers/net/wireless/microchip/wilc1000/wlan.c
index 533939e71534a..1f113a55ea6aa 100644
--- a/drivers/net/wireless/microchip/wilc1000/wlan.c
+++ b/drivers/net/wireless/microchip/wilc1000/wlan.c
@@ -1402,9 +1402,37 @@  int wilc_send_config_pkt(struct wilc_vif *vif, u8 mode, struct wid *wids,
 	return ret;
 }
 
+int wilc_get_chipid(struct wilc *wilc)
+{
+	u32 chipid = 0;
+	u32 rfrevid = 0;
+
+	if (wilc->chipid == 0) {
+		wilc->hif_func->hif_read_reg(wilc, WILC_CHIPID, &chipid);
+		wilc->hif_func->hif_read_reg(wilc, WILC_RF_REVISION_ID,
+					     &rfrevid);
+		if (!is_wilc1000(chipid)) {
+			wilc->chipid = 0;
+			return -EINVAL;
+		}
+		if (chipid == WILC_1000_BASE_ID_2A) { /* 0x1002A0 */
+			if (rfrevid != 0x1)
+				chipid = WILC_1000_BASE_ID_2A_REV1;
+		} else if (chipid == WILC_1000_BASE_ID_2B) { /* 0x1002B0 */
+			if (rfrevid == 0x4)
+				chipid = WILC_1000_BASE_ID_2B_REV1;
+			else if (rfrevid != 0x3)
+				chipid = WILC_1000_BASE_ID_2B_REV2;
+		}
+
+		wilc->chipid = chipid;
+	}
+
+	return 0;
+}
+
 static int init_chip(struct net_device *dev)
 {
-	u32 chipid;
 	u32 reg;
 	int ret = 0;
 	struct wilc_vif *vif = netdev_priv(dev);
@@ -1412,9 +1440,11 @@  static int init_chip(struct net_device *dev)
 
 	acquire_bus(wilc, WILC_BUS_ACQUIRE_AND_WAKEUP);
 
-	chipid = wilc_get_chipid(wilc, true);
+	ret = wilc_get_chipid(wilc);
+	if (ret)
+		goto release;
 
-	if ((chipid & 0xfff) != 0xa0) {
+	if ((wilc->chipid & 0xfff) != 0xa0) {
 		ret = wilc->hif_func->hif_read_reg(wilc,
 						   WILC_CORTUS_RESET_MUX_SEL,
 						   &reg);
@@ -1445,34 +1475,6 @@  static int init_chip(struct net_device *dev)
 	return ret;
 }
 
-u32 wilc_get_chipid(struct wilc *wilc, bool update)
-{
-	u32 chipid = 0;
-	u32 rfrevid = 0;
-
-	if (wilc->chipid == 0 || update) {
-		wilc->hif_func->hif_read_reg(wilc, WILC_CHIPID, &chipid);
-		wilc->hif_func->hif_read_reg(wilc, WILC_RF_REVISION_ID,
-					     &rfrevid);
-		if (!is_wilc1000(chipid)) {
-			wilc->chipid = 0;
-			return wilc->chipid;
-		}
-		if (chipid == WILC_1000_BASE_ID_2A) { /* 0x1002A0 */
-			if (rfrevid != 0x1)
-				chipid = WILC_1000_BASE_ID_2A_REV1;
-		} else if (chipid == WILC_1000_BASE_ID_2B) { /* 0x1002B0 */
-			if (rfrevid == 0x4)
-				chipid = WILC_1000_BASE_ID_2B_REV1;
-			else if (rfrevid != 0x3)
-				chipid = WILC_1000_BASE_ID_2B_REV2;
-		}
-
-		wilc->chipid = chipid;
-	}
-	return wilc->chipid;
-}
-
 int wilc_load_mac_from_nv(struct wilc *wl)
 {
 	int ret = -EINVAL;
@@ -1535,9 +1537,19 @@  int wilc_wlan_init(struct net_device *dev)
 	if (!wilc->hif_func->hif_is_init(wilc)) {
 		acquire_bus(wilc, WILC_BUS_ACQUIRE_ONLY);
 		ret = wilc->hif_func->hif_init(wilc, false);
+		if (!ret)
+			ret = wilc_get_chipid(wilc);
 		release_bus(wilc, WILC_BUS_RELEASE_ONLY);
 		if (ret)
 			goto fail;
+
+		if (!is_wilc1000(wilc->chipid)) {
+			netdev_err(dev, "Unsupported chipid: %x\n", wilc->chipid);
+			ret = -EINVAL;
+			goto fail;
+		}
+
+		netdev_dbg(dev, "chipid (%08x)\n", wilc->chipid);
 	}
 
 	if (!wilc->vmm_table)
diff --git a/drivers/net/wireless/microchip/wilc1000/wlan.h b/drivers/net/wireless/microchip/wilc1000/wlan.h
index dd2fb3c2f06a2..44dce53d24916 100644
--- a/drivers/net/wireless/microchip/wilc1000/wlan.h
+++ b/drivers/net/wireless/microchip/wilc1000/wlan.h
@@ -443,6 +443,6 @@  void chip_wakeup(struct wilc *wilc);
 int wilc_send_config_pkt(struct wilc_vif *vif, u8 mode, struct wid *wids,
 			 u32 count);
 int wilc_wlan_init(struct net_device *dev);
-u32 wilc_get_chipid(struct wilc *wilc, bool update);
+int wilc_get_chipid(struct wilc *wilc);
 int wilc_load_mac_from_nv(struct wilc *wilc);
 #endif