On 1/27/24 01:43, David Mosberger-Tang wrote:
> Previously, the driver created a net device (typically wlan0) as soon
> as the module was loaded. This commit changes the driver to follow
> normal Linux convention of creating the net device only when bus
> probing detects a supported chip.
As already mentioned multiple times, I am skeptical about the validity of
keeping netdev registration before chip presence check, but I am not the
maintainer, so I let Ajay and Kalle decide for this. Aside from that, and from
the kasan warning which is not especially related to the series (and not
observed in nominal case), I still have a few minor comments below
> Signed-off-by: David Mosberger-Tang <davidm@xxxxxxxxxx>
> ---
> changelog:
> V1: original version
> V2: Add missing forward declarations
> V3: Add missing forward declarations, actually :-(
> V4: - rearranged function order to improve readability
> - now relative to wireless-next repository
> - avoid change error return value and have lower-level functions
> directly return -ENODEV instead
> - removed any mention of WILC3000
> - export and use existing is_wilc1000() for chipid validation
> - replaced strbool() function with open code
>
> drivers/net/wireless/microchip/wilc1000/spi.c | 74 ++++++++++++++-----
> .../net/wireless/microchip/wilc1000/wlan.c | 3 +-
> .../net/wireless/microchip/wilc1000/wlan.h | 1 +
> 3 files changed, 59 insertions(+), 19 deletions(-)
[...]
> @@ -1142,7 +1170,7 @@ static int wilc_spi_init(struct wilc *wilc, bool resume)
> }
> if (ret) {
> dev_err(&spi->dev, "Failed with CRC7 on and off.\n");
> - return ret;
> + return -ENODEV;
You are still rewriting error codes here. At a lower level, sure, but still...
When I suggested setting -ENODEV at lower level, I was thinking about places
where no explicit error code was already in use, but
spi_internal_read/spi_internal_write already generate proper error codes. Or am
I missing a constraint, like the probe chain really needing -ENODEV ?
> }
>
> /* set up the desired CRC configuration: */
> @@ -1165,7 +1193,7 @@ static int wilc_spi_init(struct wilc *wilc, bool resume)
> dev_err(&spi->dev,
> "[wilc spi %d]: Failed internal write reg\n",
> __LINE__);
> - return ret;
> + return -ENODEV;
> }
> /* update our state to match new protocol settings: */
> spi_priv->crc7_enabled = enable_crc7;
> @@ -1176,17 +1204,27 @@ static int wilc_spi_init(struct wilc *wilc, bool resume)
>
> spi_priv->probing_crc = false;
>
> + return 0;
> +}
> +
> +static int wilc_validate_chipid(struct wilc *wilc)
> +{
> + struct spi_device *spi = to_spi_device(wilc->dev);
> + u32 chipid;
> + int ret;
> +
> /*
> * make sure can read chip id without protocol error
> */
> ret = wilc_spi_read_reg(wilc, WILC_CHIPID, &chipid);
> if (ret) {
> dev_err(&spi->dev, "Fail cmd read chip id...\n");
> - return ret;
> + return -ENODEV;
> + }
> + if (!is_wilc1000(chipid)) {
> + dev_err(&spi->dev, "Unknown chip id 0x%x\n", chipid);
> + return -ENODEV;
> }
> -
> - spi_priv->isinit = true;
> -
> return 0;
> }
>
> diff --git a/drivers/net/wireless/microchip/wilc1000/wlan.c b/drivers/net/wireless/microchip/wilc1000/wlan.c
> index 6b2f2269ddf8..3130a3ea8d71 100644
> --- a/drivers/net/wireless/microchip/wilc1000/wlan.c
> +++ b/drivers/net/wireless/microchip/wilc1000/wlan.c
> @@ -12,10 +12,11 @@
>
> #define WAKE_UP_TRIAL_RETRY 10000
>
> -static inline bool is_wilc1000(u32 id)
> +bool is_wilc1000(u32 id)
> {
> return (id & (~WILC_CHIP_REV_FIELD)) == WILC_1000_BASE_ID;
> }
> +EXPORT_SYMBOL_GPL(is_wilc1000);
nit: Since the function is not static anymore, it would have been nice to move
it with the other exported functions to maintain the existing functions ordering
> static inline void acquire_bus(struct wilc *wilc, enum bus_acquire acquire)
> {
> diff --git a/drivers/net/wireless/microchip/wilc1000/wlan.h b/drivers/net/wireless/microchip/wilc1000/wlan.h
> index f02775f7e41f..ebdfb0afaf71 100644
> --- a/drivers/net/wireless/microchip/wilc1000/wlan.h
> +++ b/drivers/net/wireless/microchip/wilc1000/wlan.h
> @@ -409,6 +409,7 @@ struct wilc_cfg_rsp {
>
> struct wilc_vif;
>
> +bool is_wilc1000(u32 id);
> int wilc_wlan_firmware_download(struct wilc *wilc, const u8 *buffer,
> u32 buffer_size);
> int wilc_wlan_start(struct wilc *wilc);
--
Alexis Lothoré, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
