Received-SPF: pass (google.com: domain of linux-wireless+bounces-2347-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Message-ID: <5ff15ae65ccb7c144ba8e0f07dc2ac25bd8b2b42.camel@egauge.net>
Subject: Re: RFQ: wifi: wilc1000: make wilc1000-spi bus-probe useful
From: David Mosberger-Tang <davidm@egauge.net>
Date: Mon, 22 Jan 2024 20:41:06 +0000 (UTC)
In-Reply-To: <e17d654a-8bd5-4327-8bf2-ad81c2f7355b@bootlin.com>
References: <90fb762e5840f9d5a6ae46f81692fb947a7796a4.camel@egauge.net>
	 <e17d654a-8bd5-4327-8bf2-ad81c2f7355b@bootlin.com>
Organization: eGauge Systems LLC
Content-Type: text/plain; charset=iso-8859-1
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.44.4-0ubuntu2 
Precedence: bulk
MIME-Version: 1.0
To: Alexis =?iso-8859-1?q?Lothor=E9?= <alexis.lothore@bootlin.com>,
	Ajay.Kathat@microchip.com
Cc: linux-wireless@vger.kernel.org, kvalo@kernel.org

Alexis,

Thanks for your feedback!

On Mon, 2024-01-22 at 15:19 +0100, Alexis Lothor=E9 wrote:
> Hello,
>=20
> On 1/19/24 22:51, David Mosberger-Tang wrote:
> > The current version of the wilc1000 driver has a probe function that si=
mply
> > 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 res=
ult 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.
> >=20
> > 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 missin=
g
> > something? Would it make sense to merge something along these lines int=
o
> > mainline?
>=20
> The general statement sounds relevant to me, it looks not so useful to re=
gister
> the corresponding netdevice if we can not even detect the chip at probe t=
ime.
> 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
>=20
> >=20
> >  --david
> >=20
> > diff --git a/drivers/net/wireless/microchip/wilc1000/spi.c
>=20
> [...]
>=20
> > + /* we need power to configure the bus protocol and to read the chip i=
d: */
> > +
> > + wilc_wlan_power(wilc, true);
> > +
> > + ret =3D wilc_spi_configure_bus_protocol(wilc);
> > +
> > + wilc_wlan_power(wilc, false);
> > +
> > + if (ret) {
> > + ret =3D -ENODEV;
>=20
> 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 abse=
nce 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 pres=
ent
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 funct=
ion
since it felt wrong to do that in a function called "configure bus protocol=
".

> > + goto netdev_cleanup;
> > + }
> > +
> >  return 0;
> >  netdev_cleanup:
>=20
> [...]
>=20
> > @@ -1187,16 +1189,38 @@ static int wilc_spi_init(struct wilc *wilc, boo=
l resume)
> >  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);
>=20
> I guess something will break here. This updates now mark the chip as init=
ialized
> (sp_priv->isinit) at probe time, but unpower the chip before finishing pr=
obe, so
> this wilc_wlan_power(wilc, true) needs more likely to be called earlier i=
n
> 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 s=
et in
wilc_spi_init(), not in wilc_bus_probe().  That is the reason I had to upda=
te
the comment for the "isinit" member in wilc_spi.  Let me know if I'm missin=
g
something, though.

  --david