Received-SPF: pass (google.com: domain of linux-wireless+bounces-3557-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <081bce96-f485-414c-8051-e1c14271f8cc@bootlin.com>
Date: Tue, 13 Feb 2024 17:58:04 +0100
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH RFC] wifi: wilc1000: fix reset line assert/deassert
 polarity
Content-Language: en-US
To: David Mosberger-Tang <davidm@egauge.net>, linux-wireless@vger.kernel.org
Cc: Ajay Singh <ajay.kathat@microchip.com>,
 Claudiu Beznea <claudiu.beznea@tuxon.dev>, Kalle Valo <kvalo@kernel.org>,
 Thomas Petazzoni <thomas.petazzoni@bootlin.com>, linux-kernel@vger.kernel.org
References: <20240213-wilc_1000_reset_line-v1-1-e01da2b23fed@bootlin.com>
 <2ff1c701f3443e1c612a81f4077b0280850f57c6.camel@egauge.net>
From: =?UTF-8?Q?Alexis_Lothor=C3=A9?= <alexis.lothore@bootlin.com>
In-Reply-To: <2ff1c701f3443e1c612a81f4077b0280850f57c6.camel@egauge.net>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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 ?

> On our platform, we never merged commit fcf690b0 and hence our DTS already
> defines the RESET pin as GPIO_ACTIVE_LOW.  So, I don't have any issues at all
> with your patch! :-)

So in the end, the patch should be about a mere revert. I will update
accordingly when relevant, but before that I'll wait for some feedback about the
potential issue of this patch (forcing users to udpate faulty devicetree)

Thanks,
Alexis

-- 
Alexis Lothoré, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com