Hello, On 1/19/24 22:51, David Mosberger-Tang wrote: > The current version of the wilc1000 driver has a probe function that simply > assumes the chip is present. It is only later, in wilc_spi_init(), that the > driver verifies that it can actually communicate with the chip. The result of > this is that the net device (typically wlan0) is created and remains in place as > long as the wilc1000-spi driver is loaded, even if the WILC1000 chip is not > present or not working. > > Is there any reason not to detect the chip's present in wilc_bus_probe()? The > patch below (relative to 5.15.147) works for me, but perhaps I'm missing > something? Would it make sense to merge something along these lines into > mainline? The general statement sounds relevant to me, it looks not so useful to register the corresponding netdevice if we can not even detect the chip at probe time. I have a series under work which, by "side effect", accomplishes the same kind of detection: it aims to fix faulty mac address (00:00:00:00:00:00) which is set correctly only after interface has been brought up. The series tries to read the mac address from NV memory right at probe time. If it fails, it can make the probe procedure fail and not register the wireless device. Nonetheless, validating chip presence with chip id sounds better than with mac address from NV memory. Aside from that, I have a few more specific comments below > > --david > > diff --git a/drivers/net/wireless/microchip/wilc1000/spi.c [...] > + /* we need power to configure the bus protocol and to read the chip id: */ > + > + wilc_wlan_power(wilc, true); > + > + ret = wilc_spi_configure_bus_protocol(wilc); > + > + wilc_wlan_power(wilc, false); > + > + if (ret) { > + ret = -ENODEV; I would keep wilc_spi_configure_bus_protocol original error instead of rewriting/forcing it to -ENODEV here. I mean, if something fails in wilc_spi_configure_bus_protocol but not right at the first attempt to communicate with the chip, it does not translate automatically to an absence of chip, right ? > + goto netdev_cleanup; > + } > + > return 0; > netdev_cleanup: [...] > @@ -1187,16 +1189,38 @@ static int wilc_spi_init(struct wilc *wilc, bool resume) > ret = wilc_spi_read_reg(wilc, WILC_CHIPID, &chipid); > if (ret) { > dev_err(&spi->dev, "Fail cmd read chip id...\n"); > - goto fail; > + return ret; > + } > + return 0; > +} > + > +static int wilc_spi_init(struct wilc *wilc, bool resume) > +{ > + struct spi_device *spi = to_spi_device(wilc->dev); > + struct wilc_spi *spi_priv = wilc->bus_data; > + u32 chipid; > + int ret; > + > + if (spi_priv->isinit) { > + /* Confirm we can read chipid register without error: */ > + ret = wilc_spi_read_reg(wilc, WILC_CHIPID, &chipid); > + if (ret == 0) > + return 0; > + > + dev_err(&spi->dev, "Fail cmd read chip id...\n"); > + } > + > + wilc_wlan_power(wilc, true); I guess something will break here. This updates now mark the chip as initialized (sp_priv->isinit) at probe time, but unpower the chip before finishing probe, so this wilc_wlan_power(wilc, true) needs more likely to be called earlier in wilc_spi_init (ie: before trying to read again the chip id). More generally, there is an important change about the meaning of this flag (meant: chip is on and configured, now means: I know chip is here but it may be unpowered), and since wlan.c can check for this flag to know if it can communicate with the chip, there will be an issue here. Thanks, Alexis -- Alexis Lothoré, Bootlin Embedded Linux and Kernel engineering https://bootlin.com