RFQ: wifi: wilc1000: make wilc1000-spi bus-probe useful

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?

 --david

 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=%sabled crc16=%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 = wilc_spi_configure_bus_protocol(wilc);
+
+ wilc_wlan_power(wilc, false);
+
+ if (ret) {
+ ret = -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 = to_spi_device(wilc->dev);
 struct wilc_spi *spi_priv = wilc->bus_data;
@@ -1116,21 +1133,6 @@ static int wilc_spi_init(struct wilc *wilc, bool resume)
 u32 chipid;
 int ret, i;
- 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);
-
- /*
- * 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 resume)
 }
 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 resume)
 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 = enable_crc7;
@@ -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);
+
+ ret = wilc_spi_configure_bus_protocol(wilc);
+ if (ret) {
+ wilc_wlan_power(wilc, false);
+ return ret;
 }
 spi_priv->isinit = true;
 return 0;
-
- fail:
- wilc_wlan_power(wilc, false);
- return ret;
 }
 static int wilc_spi_read_size(struct wilc *wilc, u32 *size)

On 1/19/24 14:51, David Mosberger-Tang wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> 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?
> 

I think it is the WILC driver design that the firmware/chip operations
are executed only when the netdev interface(wlan0) is up. The firmware
is started only after the interface is up. However, it should be okay to
read the register values since the bus interface is up.

As I understand, this condition is raised since the auto-load started to
work after the patch[1], now the driver is getting loaded at the boot-up
time.
Actually, the auto-detect(hot-plug) for SPI bus can't be supported like
the SDIO bus where the driver gets loaded/unloaded when the device is
connected/removed. In case of SPI devices, the driver probe will be
called at the boot-up time based on the Device-tree(DT) entry. If the
SPI device is connected after board boot-up, the board reboot is
required for probe function to get called i.e. even wilc_bus_probe()
returns failure for first time then the probe will not get called again.
One way to handle this is by modifying the DT entry of the system to
define whether the SPI device is connected or not.


1.
https://github.com/torvalds/linux/commit/f2f16ae9cc9cba4e3c70941cf6a6443c9ea920f4


Regards,
Ajay

On Mon, 2024-01-22 at 16:57 +0000, Ajay.Kathat@microchip.com wrote:
> On 1/19/24 14:51, David Mosberger-Tang wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> > 
> > 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?
> > 
> 
> I think it is the WILC driver design that the firmware/chip operations
> are executed only when the netdev interface(wlan0) is up. The firmware
> is started only after the interface is up. However, it should be okay to
> read the register values since the bus interface is up.

Yep, I didn't see any issues in my testing.

> As I understand, this condition is raised since the auto-load started to
> work after the patch[1], now the driver is getting loaded at the boot-up
> time.
> Actually, the auto-detect(hot-plug) for SPI bus can't be supported like
> the SDIO bus where the driver gets loaded/unloaded when the device is
> connected/removed. In case of SPI devices, the driver probe will be
> called at the boot-up time based on the Device-tree(DT) entry. If the
> SPI device is connected after board boot-up, the board reboot is
> required for probe function to get called i.e. even wilc_bus_probe()
> returns failure for first time then the probe will not get called again.
> One way to handle this is by modifying the DT entry of the system to
> define whether the SPI device is connected or not.

Makes sense.  In our system, we don't have the ability to dynamically patch the
device tree so we rely on driver probing to confirm that a device actually
exists.

  --david

Alexis Lothoré <alexis.lothore@bootlin.com> writes:

> Hello David,
> just reacting to your answers, but I will take a look at your updated patch
>
> On 1/22/24 21:41, David Mosberger-Tang wrote:
>> Alexis,
>> 
>> Thanks for your feedback!
>> 
>> On Mon, 2024-01-22 at 15:19 +0100, Alexis Lothoré wrote:
>>> Hello,
>>>
>>> On 1/19/24 22:51, David Mosberger-Tang wrote:
>
> [...]
>
>>>> + 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 ?
>> 
>> Hmmh, I'm happy to change it, but, as it happens, all failure returns in
>> wilc_spi_configure_bus_protocol() basically mean that the device isn't present
>> or a device is present which the driver doesn't support, so I think -ENODEV is
>> more useful than returning -EINVAL (as would be the case).  Let me know if you
>> still think I should change it.
>
> Yeah, but then I would change wilc_spi_configure_bus_protocol() to return
> -ENODEV instead of -EINVAL, if that's really the cause, and just let calling
> functions propagate it. That may just be a personal taste, but I find it pretty
> tedious to debug some error code and eventually realize that some intermediate
> function just rewrote the real error to another one (and sometime, loosing some
> info while doing so).

Yeah, changing error values is very much discouraged.