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:
> > 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.
Ahh, that sounds like another useful improvement!
> 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.
Great! I'll send an update patch soon (properly formatted this time, I
hope...).
> 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 ?
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.
In the new patch, I broke out the chipid validation code into its own function
since it felt wrong to do that in a function called "configure bus protocol".
> > + 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).
Oh, no that's definitely not the intention. The garbled formatting makes
reading the patch more confusing than it should be, but isinit still gets set in
wilc_spi_init(), not in wilc_bus_probe(). That is the reason I had to update
the comment for the "isinit" member in wilc_spi. Let me know if I'm missing
something, though.
--david
