linux-kernel - Re: [PATCH RFC] wifi: wilc1000: fix reset line assert/deassert polarity

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Fri, 16 Feb 2024 16:54:24 +0000
From: Conor Dooley <conor@...nel.org>
To: Kalle Valo <kvalo@...nel.org>
Cc: Ajay.Kathat@...rochip.com, alexis.lothore@...tlin.com,
	davidm@...uge.net, linux-wireless@...r.kernel.org,
	claudiu.beznea@...on.dev, thomas.petazzoni@...tlin.com,
	linux-kernel@...r.kernel.org, devicetree@...r.kernel.org
Subject: Re: [PATCH RFC] wifi: wilc1000: fix reset line assert/deassert
 polarity

On Fri, Feb 16, 2024 at 06:01:52PM +0200, Kalle Valo wrote:
> (Adding devicetree list for comments)
> 
> <Ajay.Kathat@...rochip.com> writes:
> 
> > On 2/13/24 09:58, Alexis Lothoré wrote:
> >> 
> >> On 2/13/24 17:42, David Mosberger-Tang wrote:
> >>> On Tue, 2024-02-13 at 16:22 +0100, Alexis Lothoré wrote:
> >>>> When using a wilc1000 chip over a spi bus, users can optionally define a
> >>>> reset gpio and a chip enable gpio. The reset line of wilc1000 is active
> >>>> 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-gpios
> >>>> properties") and correctly indicates that the reset line is an active-low
> >>>> signal. However, the corresponding driver part, brought by commit
> >>>> ec031ac4792c ("wilc1000: Add reset/enable GPIO support to SPI driver"), 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" during power
> >>>> up, which in fact asserts the reset line when device tree describes the
> >>>> reset line as GPIO_ACTIVE_LOW).
> >>>
> >>> Note that commit ec031ac4792c is doing the right thing in regards to an
> >>> ACTIVE_LOW RESET pin and the binding documentation is consistent with that code.
> >>>
> >>> It was later on that commit fcf690b0 flipped the RESET line polarity 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.
> >> 
> >> Ah, you are right, and I was wrong citing your GPIOs patch as faulty
> >> (git-blaming too fast !), thanks for the clarification. I missed this 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 using
> > DT entry from porting guide. One approach is to retain the current code
> > and document this if needed.
> 
> 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.

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.

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?

> I'll add devicetree list so hopefully people there can comment also,
> full patch available in [3].
> 
> Alexis, if there are no more comments I'm in favor submitting the revert
> you mentioned.

From a dt-bindings point of view, the aforementioned revert seems
correct and would be
Acked-by: Conor Dooley <conor.dooley@...rochip.com>

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?

Cheers,
Conor.

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)