On 1/30/24 02:06, Alexis Lothoré wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> 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
>
IMO the order of netdev registration before chip validity should be
fine. Since wilc_bus_probe() is already doing netdev_cleanup that takes
care of errors path after netdev registration. For this patch, it is
okay to follow the same approach like the previous implementation.
However, in the patch, please take care of disabling 'wilc->rtc_clk' in
wilc_bus_probe() for the failure condition.
static int wilc_bus_probe(struct spi_device *spi) {
...
+power_down:
+ clk_disable_unprepare(wilc->rtc_clk);
+ wilc_wlan_power(wilc, false);
netdev_cleanup:
wilc_netdev_cleanup(wilc);
I hope this patch is tested for failure scenario(when WILC1000 SPI
device is not connected) as well.
>> 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
>
