Received-SPF: pass (google.com: domain of linux-kernel+bounces-68935-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
From: Kalle Valo <kvalo@kernel.org>
To: <Ajay.Kathat@microchip.com>
Cc: <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
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>
Date: Fri, 16 Feb 2024 18:01:52 +0200
In-Reply-To: <aac398e4-d870-4ba2-8877-b98afecb8d1b@microchip.com> (Ajay
	Kathat's message of "Thu, 15 Feb 2024 04:35:46 +0000")
Message-ID: <877cj4o0sv.fsf@kernel.org>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/28.2 (gnu/linux)
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable

(Adding devicetree list for comments)

<Ajay.Kathat@microchip.com> writes:

> On 2/13/24 09:58, Alexis Lothor=C3=A9 wrote:
>>=20
>> On 2/13/24 17:42, David Mosberger-Tang wrote:
>>> On Tue, 2024-02-13 at 16:22 +0100, Alexis Lothor=C3=A9 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 appl=
ied.
>>>>
>>>> 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/d=
own
>>>> the chip (for example, setting the reset line to a logic "1" during po=
wer
>>>> 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 t=
hat 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.
>>=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 this pa=
tch from
>> Ajay (fcf690b0) flipping the reset logic. Maybe he had issues while miss=
ing
>> 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.

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.


[1] https://ww1.microchip.com/downloads/en/DeviceDoc/ATWILC1000-ATWILC3000-=
ATWILC-Devices-Linux-Porting-Guide-User-Guide-DS70005329C.pdf

[2] Documentation/devicetree/bindings/net/wireless/microchip,wilc1000.yaml

[3] https://patchwork.kernel.org/project/linux-wireless/patch/20240213-wilc=
_1000_reset_line-v1-1-e01da2b23fed@bootlin.com/

--=20
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatc=
hes