Received-SPF: pass (google.com: domain of linux-wireless+bounces-3689-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Fri, 16 Feb 2024 16:55:16 +0000
From: Conor Dooley <conor@kernel.org>
To: Kalle Valo <kvalo@kernel.org>
Cc: Ajay.Kathat@microchip.com, alexis.lothore@bootlin.com,
	davidm@egauge.net, linux-wireless@vger.kernel.org,
	claudiu.beznea@tuxon.dev, thomas.petazzoni@bootlin.com,
	linux-kernel@vger.kernel.org, devicetree@vger.kernel.org
Subject: Re: [PATCH RFC] wifi: wilc1000: fix reset line assert/deassert
 polarity
Message-ID: <20240216-spinster-decade-e136ac3e72d0@spud>
References: <20240213-wilc_1000_reset_line-v1-1-e01da2b23fed@bootlin.com>
 <2ff1c701f3443e1c612a81f4077b0280850f57c6.camel@egauge.net>
 <081bce96-f485-414c-8051-e1c14271f8cc@bootlin.com>
 <aac398e4-d870-4ba2-8877-b98afecb8d1b@microchip.com>
 <877cj4o0sv.fsf@kernel.org>
 <20240216-reckless-freedom-4768ce41e939@spud>
Precedence: bulk
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha256;
	protocol="application/pgp-signature"; boundary="7bS3SWgYAKyG5UJl"
Content-Disposition: inline
In-Reply-To: <20240216-reckless-freedom-4768ce41e939@spud>


--7bS3SWgYAKyG5UJl
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Fri, Feb 16, 2024 at 04:54:29PM +0000, Conor Dooley wrote:
> On Fri, Feb 16, 2024 at 06:01:52PM +0200, Kalle Valo wrote:
> > (Adding devicetree list for comments)
> >=20
> > <Ajay.Kathat@microchip.com> writes:
> >=20
> > > On 2/13/24 09:58, Alexis Lothor=E9 wrote:
> > >>=20
> > >> On 2/13/24 17:42, David Mosberger-Tang wrote:
> > >>> On Tue, 2024-02-13 at 16:22 +0100, Alexis Lothor=E9 wrote:
> > >>>> When using a wilc1000 chip over a spi bus, users can optionally de=
fine a
> > >>>> reset gpio and a chip enable gpio. The reset line of wilc1000 is a=
ctive
> > >>>> low, so to hold the chip in reset, a low (physical) value must be =
applied.
> > >>>>
> > >>>> The corresponding device tree binding documentation was introduced=
 by
> > >>>> commit f31ee3c0a555 ("wilc1000: Document enable-gpios and reset-gp=
ios
> > >>>> properties") and correctly indicates that the reset line is an act=
ive-low
> > >>>> signal. However, the corresponding driver part, brought by commit
> > >>>> ec031ac4792c ("wilc1000: Add reset/enable GPIO support to SPI driv=
er"), is
> > >>>> misusing the gpiod APIs and apply an inverted logic when powering =
up/down
> > >>>> the chip (for example, setting the reset line to a logic "1" durin=
g power
> > >>>> up, which in fact asserts the reset line when device tree describe=
s the
> > >>>> reset line as GPIO_ACTIVE_LOW).
> > >>>
> > >>> Note that commit ec031ac4792c is doing the right thing in regards t=
o an
> > >>> ACTIVE_LOW RESET pin and the binding documentation is consistent wi=
th that code.
> > >>>
> > >>> It was later on that commit fcf690b0 flipped the RESET line polarit=
y to treat it
> > >>> as GPIO_ACTIVE_HIGH.  I never understood why that was done and, as =
you noted, it
> > >>> introduced in inconsistency with the binding documentation.
> > >>=20
> > >> Ah, you are right, and I was wrong citing your GPIOs patch as faulty
> > >> (git-blaming too fast !), thanks for the clarification. I missed thi=
s patch from
> > >> Ajay (fcf690b0) flipping the reset logic. Maybe he had issues while =
missing
> > >> proper device tree configuration and then submitted this flip ?
> > >
> > > Indeed, it was done to align the code as per the DT entry suggested in
> > > WILC1000/3000 porting guide[1 -page 18], which is already used by most
> > > of the existing users. This change has impact on the users who are us=
ing
> > > DT entry from porting guide. One approach is to retain the current co=
de
> > > and document this if needed.
> >=20
> > So if I'm understanding the situation correctly Microchip's porting
> > guide[1] doesn't match with kernel.org documentation[2]? I'm not the
> > expert here but from my point of view the issue is clear: the code needs
> > to follow kernel.org documentation[2], not external documentation.
>=20
> My point of view would definitely be that drivers in the mainline kernel
> absolutely should respect the ABI defined in the dt-binding. What a vendor
> decides to do in their own tree I suppose is their problem, but I would
> advocate that vendor kernels would also respect the ABI from mainline.
>=20
> Looking a bit more closely at the porting guide, it contains other
> properties that are not present in the dt-binding - undocumented
> compatibles and a different enable gpio property for example.
> I guess it (and the vendor version of the driver) never got updated when
> wilc1000 supported landed in mainline?
>=20
> > I'll add devicetree list so hopefully people there can comment also,
> > full patch available in [3].
> >=20
> > Alexis, if there are no more comments I'm in favor submitting the revert
> > you mentioned.
>=20
> From a dt-bindings point of view, the aforementioned revert seems
> correct and would be
> Acked-by: Conor Dooley <conor.dooley@microchip.com>

Maybe an R-b is more suitable here, too used to acking trivial patches
that are dt related..

>=20
> Getting off my dt-binding maintainer high-horse, linux4microchip is going
> be updating to a 6.6 based kernel in the coming weeks - maybe that's a
> good time to update the vendor kernel wilc drivers (and therefore the
> porting guide?) to match the properties used by mainline Ajay?
>=20
> Cheers,
> Conor.


--7bS3SWgYAKyG5UJl
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iHUEABYIAB0WIQRh246EGq/8RLhDjO14tDGHoIJi0gUCZc+TcwAKCRB4tDGHoIJi
0snwAQCPW30mRLhwRM6OGWxdq7BKSiI8nrYXqB0r/w1KcadwmwEA1PCfj6XX3FCc
y79qIG1+H9bjozbWYxMRtTlzNmeRnQA=
=YeuA
-----END PGP SIGNATURE-----

--7bS3SWgYAKyG5UJl--