| Message ID | 20240823161131.94305-2-marex@denx.de |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/4] dt-bindings: wireless: wilc1000: Document WILC3000 compatible string | expand |
Hello Marek, On 8/23/24 18:08, Marek Vasut wrote: > Do not use wilc_get_chipid() outside of wlan.c . Instead, call > wilc_get_chipid() right after the SDIO/SPI interface has been > initialized to cache the device chipid, and then use the cached > chipid throughout the driver. Make wilc_get_chipid() static and > remove its prototype from wlan.h . > > 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: Marek Vasut <marex@denx.de> > 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 > --- [...] > +static u32 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); If we search for WILC_CHIPID in the whole driver, there are still two places manually reading this register. Shouldn't those places also benefit from wilc_get_chipid ? > + wilc->hif_func->hif_read_reg(wilc, WILC_RF_REVISION_ID, > + &rfrevid); > + if (!is_wilc1000(chipid)) { > + wilc->chipid = 0; While at it, since you have trimmed the update parameter, it would be nice to also fix this return value (ie make wilc_getchipid() not return 0 but a real error code if we can not read the chip id. Thanks, Alexis
On 8/27/24 17:34, Marek Vasut wrote: > On 8/27/24 9:51 AM, Alexis Lothoré wrote: > > Hi, > >>> +static u32 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); >> If we search for WILC_CHIPID in the whole driver, there are still two places >> manually reading this register. Shouldn't those places also benefit from >> wilc_get_chipid ? > > Both the one in wilc_wlan_start() and wilc_validate_chipid() look more like some > sort of communication check attempt, rather than reading out the chipid for any > sort of actual chip identification purpose. I could simply remove those ? Agree about the purpose of this reading in wilc_wlan_start and wilc_validate_chipid. And about removing those: I would say why not. wilc_validate_chipid has proven to be quite useful to diagnose some early communication failure, but I guess there are enough communications attempts around (wilc_spi_configure_bus_protocol, wilc_load_mac_from_nv) to still validate than we are able to communicate with the chip at probe time. > >>> + wilc->hif_func->hif_read_reg(wilc, WILC_RF_REVISION_ID, >>> + &rfrevid); >>> + if (!is_wilc1000(chipid)) { >>> + wilc->chipid = 0; >> >> While at it, since you have trimmed the update parameter, it would be nice to >> also fix this return value (ie make wilc_getchipid() not return 0 but a real >> error code if we can not read the chip id. > > Fixed in V3, thanks . Great, thanks Alexis
diff --git a/drivers/net/wireless/microchip/wilc1000/sdio.c b/drivers/net/wireless/microchip/wilc1000/sdio.c index 41122199d51eb..90357c89ae29b 100644 --- a/drivers/net/wireless/microchip/wilc1000/sdio.c +++ b/drivers/net/wireless/microchip/wilc1000/sdio.c @@ -671,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 @@ -760,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/wlan.c b/drivers/net/wireless/microchip/wilc1000/wlan.c index 533939e71534a..1aab2f2dc159f 100644 --- a/drivers/net/wireless/microchip/wilc1000/wlan.c +++ b/drivers/net/wireless/microchip/wilc1000/wlan.c @@ -1402,6 +1402,35 @@ int wilc_send_config_pkt(struct wilc_vif *vif, u8 mode, struct wid *wids, return ret; } +static u32 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 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; +} + static int init_chip(struct net_device *dev) { u32 chipid; @@ -1412,7 +1441,7 @@ static int init_chip(struct net_device *dev) acquire_bus(wilc, WILC_BUS_ACQUIRE_AND_WAKEUP); - chipid = wilc_get_chipid(wilc, true); + chipid = wilc_get_chipid(wilc); if ((chipid & 0xfff) != 0xa0) { ret = wilc->hif_func->hif_read_reg(wilc, @@ -1445,34 +1474,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; @@ -1527,6 +1528,7 @@ int wilc_wlan_init(struct net_device *dev) int ret = 0; struct wilc_vif *vif = netdev_priv(dev); struct wilc *wilc; + u32 chipid = 0; wilc = vif->wilc; @@ -1535,9 +1537,18 @@ 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) + chipid = wilc_get_chipid(wilc); release_bus(wilc, WILC_BUS_RELEASE_ONLY); if (ret) goto fail; + + if (!is_wilc1000(chipid)) { + netdev_err(dev, "Unsupported chipid: %x\n", chipid); + return -EINVAL; + } + + netdev_dbg(dev, "chipid (%08x)\n", 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..ae187192a79c6 100644 --- a/drivers/net/wireless/microchip/wilc1000/wlan.h +++ b/drivers/net/wireless/microchip/wilc1000/wlan.h @@ -443,6 +443,5 @@ 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_load_mac_from_nv(struct wilc *wilc); #endif
Do not use wilc_get_chipid() outside of wlan.c . Instead, call wilc_get_chipid() right after the SDIO/SPI interface has been initialized to cache the device chipid, and then use the cached chipid throughout the driver. Make wilc_get_chipid() static and remove its prototype from wlan.h . 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: Marek Vasut <marex@denx.de> 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 --- .../net/wireless/microchip/wilc1000/sdio.c | 13 ---- .../net/wireless/microchip/wilc1000/wlan.c | 69 +++++++++++-------- .../net/wireless/microchip/wilc1000/wlan.h | 1 - 3 files changed, 40 insertions(+), 43 deletions(-)