Received-SPF: pass (google.com: domain of linux-wireless+bounces-2278-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <90fb762e5840f9d5a6ae46f81692fb947a7796a4.camel@egauge.net>
Subject: RFQ: wifi: wilc1000: make wilc1000-spi bus-probe useful
From: David Mosberger-Tang <davidm@egauge.net>
Date: Fri, 19 Jan 2024 21:51:39 +0000 (UTC)
Organization: eGauge Systems LLC
Content-Type: text/plain; charset=us-ascii
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.44.4-0ubuntu2 
Precedence: bulk
MIME-Version: 1.0
To: Ajay.Kathat@microchip.com
Cc: linux-wireless@vger.kernel.org, kvalo@kernel.org

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 pla=
ce 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()? T=
he
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?

 --david

diff --git a/drivers/net/wireless/microchip/wilc1000/spi.c
b/drivers/net/wireless/microchip/wilc1000/spi.c
index 6bac52527e38..c7ab816d65bc 100644
--- a/drivers/net/wireless/microchip/wilc1000/spi.c
+++ b/drivers/net/wireless/microchip/wilc1000/spi.c
@@ -42,7 +42,7 @@ MODULE_PARM_DESC(enable_crc16,
 #define WILC_SPI_RSP_HDR_EXTRA_DATA 8
 struct wilc_spi {
- bool isinit; /* true if SPI protocol has been configured */
+ bool isinit; /* true if wilc_spi_init was successful */
 bool probing_crc; /* true if we're probing chip's CRC config */
 bool crc7_enabled; /* true if crc7 is currently enabled */
 bool crc16_enabled; /* true if crc16 is currently enabled */
@@ -55,6 +55,7 @@ struct wilc_spi {
 static const struct wilc_hif_func wilc_hif_spi;
 static int wilc_spi_reset(struct wilc *wilc);
+static int wilc_spi_configure_bus_protocol(struct wilc *);
 /********************************************
 *
@@ -232,6 +233,22 @@ static int wilc_bus_probe(struct spi_device *spi)
 }
 clk_prepare_enable(wilc->rtc_clk);
+ dev_info(&spi->dev, "crc7=3D%sabled crc16=3D%sabled",
+ enable_crc7 ? "en" : "dis", enable_crc16 ? "en" : "dis");
+
+ /* we need power to configure the bus protocol and to read the chip id: *=
/
+
+ wilc_wlan_power(wilc, true);
+
+ ret =3D wilc_spi_configure_bus_protocol(wilc);
+
+ wilc_wlan_power(wilc, false);
+
+ if (ret) {
+ ret =3D -ENODEV;
+ goto netdev_cleanup;
+ }
+
 return 0;
 netdev_cleanup:
@@ -1108,7 +1125,7 @@ static int wilc_spi_deinit(struct wilc *wilc)
 return 0;
 }
-static int wilc_spi_init(struct wilc *wilc, bool resume)
+static int wilc_spi_configure_bus_protocol (struct wilc *wilc)
 {
 struct spi_device *spi =3D to_spi_device(wilc->dev);
 struct wilc_spi *spi_priv =3D wilc->bus_data;
@@ -1116,21 +1133,6 @@ static int wilc_spi_init(struct wilc *wilc, bool res=
ume)
 u32 chipid;
 int ret, i;
- if (spi_priv->isinit) {
- /* Confirm we can read chipid register without error: */
- ret =3D wilc_spi_read_reg(wilc, WILC_CHIPID, &chipid);
- if (ret =3D=3D 0)
- return 0;
-
- dev_err(&spi->dev, "Fail cmd read chip id...\n");
- }
-
- wilc_wlan_power(wilc, true);
-
- /*
- * configure protocol
- */
-
 /*
 * Infer the CRC settings that are currently in effect. This
 * is necessary because we can't be sure that the chip has
@@ -1147,7 +1149,7 @@ static int wilc_spi_init(struct wilc *wilc, bool resu=
me)
 }
 if (ret) {
 dev_err(&spi->dev, "Failed with CRC7 on and off.\n");
- goto fail;
+ return ret;
 }
 /* set up the desired CRC configuration: */
@@ -1170,7 +1172,7 @@ static int wilc_spi_init(struct wilc *wilc, bool resu=
me)
 dev_err(&spi->dev,
 "[wilc spi %d]: Failed internal write reg\n",
 __LINE__);
- goto fail;
+ return ret;
 }
 /* update our state to match new protocol settings: */
 spi_priv->crc7_enabled =3D enable_crc7;
@@ -1187,16 +1189,38 @@ static int wilc_spi_init(struct wilc *wilc, bool re=
sume)
 ret =3D 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 =3D to_spi_device(wilc->dev);
+ struct wilc_spi *spi_priv =3D wilc->bus_data;
+ u32 chipid;
+ int ret;
+
+ if (spi_priv->isinit) {
+ /* Confirm we can read chipid register without error: */
+ ret =3D wilc_spi_read_reg(wilc, WILC_CHIPID, &chipid);
+ if (ret =3D=3D 0)
+ return 0;
+
+ dev_err(&spi->dev, "Fail cmd read chip id...\n");
+ }
+
+ wilc_wlan_power(wilc, true);
+
+ ret =3D wilc_spi_configure_bus_protocol(wilc);
+ if (ret) {
+ wilc_wlan_power(wilc, false);
+ return ret;
 }
 spi_priv->isinit =3D true;
 return 0;
-
- fail:
- wilc_wlan_power(wilc, false);
- return ret;
 }
 static int wilc_spi_read_size(struct wilc *wilc, u32 *size)