| Message ID | 20240823161131.94305-2-marex@denx.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | [v2,1/4] dt-bindings: wireless: wilc1000: Document WILC3000 compatible string | expand |
On Fri, Aug 23, 2024 at 06:08:57PM +0200, 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> ... > diff --git a/drivers/net/wireless/microchip/wilc1000/wlan.c b/drivers/net/wireless/microchip/wilc1000/wlan.c ... > @@ -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; Hi Marek, Should this unwind as is the case elsewhere in this function? ret = -EINVAL; goto fail; Flagged by Smatch. > + } > + > + netdev_dbg(dev, "chipid (%08x)\n", chipid); > } > > if (!wilc->vmm_table) ...
On 8/23/24 7:46 PM, Simon Horman wrote: > On Fri, Aug 23, 2024 at 06:08:57PM +0200, 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> > > ... > >> diff --git a/drivers/net/wireless/microchip/wilc1000/wlan.c b/drivers/net/wireless/microchip/wilc1000/wlan.c > > ... > >> @@ -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; > > Hi Marek, > > Should this unwind as is the case elsewhere in this function? It should, will fix in V3, thanks. > ret = -EINVAL; > goto fail; > > Flagged by Smatch. What's the trick here ?
+ Dan On Fri, Aug 23, 2024 at 10:38:59PM +0200, Marek Vasut wrote: > On 8/23/24 7:46 PM, Simon Horman wrote: > > On Fri, Aug 23, 2024 at 06:08:57PM +0200, 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> > > > > ... > > > > > diff --git a/drivers/net/wireless/microchip/wilc1000/wlan.c b/drivers/net/wireless/microchip/wilc1000/wlan.c > > > > ... > > > > > @@ -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; > > > > Hi Marek, > > > > Should this unwind as is the case elsewhere in this function? > > It should, will fix in V3, thanks. > > > ret = -EINVAL; > > goto fail; > > > > Flagged by Smatch. > > What's the trick here ? Smatch is here. I don't think there is much of a trick other than running it :) https://github.com/error27/smatch
On 8/24/24 2:44 PM, Simon Horman wrote: > + Dan > > On Fri, Aug 23, 2024 at 10:38:59PM +0200, Marek Vasut wrote: >> On 8/23/24 7:46 PM, Simon Horman wrote: >>> On Fri, Aug 23, 2024 at 06:08:57PM +0200, 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> >>> >>> ... >>> >>>> diff --git a/drivers/net/wireless/microchip/wilc1000/wlan.c b/drivers/net/wireless/microchip/wilc1000/wlan.c >>> >>> ... >>> >>>> @@ -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; >>> >>> Hi Marek, >>> >>> Should this unwind as is the case elsewhere in this function? >> >> It should, will fix in V3, thanks. >> >>> ret = -EINVAL; >>> goto fail; >>> >>> Flagged by Smatch. >> >> What's the trick here ? > > Smatch is here. I don't think there is much of a trick other than running it :) > > https://github.com/error27/smatch Thanks !
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 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 ? >> + 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 .
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(-)