Re: [PATCH v2] PCI: dra7xx: Fix reset behaviour

From: Luca Ceresoli <luca@lucaceresoli.net>
To: "Linus Walleij" <linus.walleij@linaro.org>,
	"Pali Rohár" <pali@kernel.org>,
	"Bartosz Golaszewski" <bgolaszewski@baylibre.com>
Cc: Kishon Vijay Abraham I <kishon@ti.com>,
	Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>,
	linux-pci <linux-pci@vger.kernel.org>,
	Linux-OMAP <linux-omap@vger.kernel.org>,
	Linux ARM <linux-arm-kernel@lists.infradead.org>,
	linux-kernel <linux-kernel@vger.kernel.org>,
	Rob Herring <robh@kernel.org>,
	Bjorn Helgaas <bhelgaas@google.com>,
	"open list:GPIO SUBSYSTEM" <linux-gpio@vger.kernel.org>
Subject: Re: [PATCH v2] PCI: dra7xx: Fix reset behaviour
Date: Fri, 25 Jun 2021 10:05:52 +0200	[thread overview]
Message-ID: <20966897-c2c9-92e2-c73e-78e0e8acc13d@lucaceresoli.net> (raw)
In-Reply-To: <CACRpkdZyMr-8Qmf3S7R+RcWe5shhnMeBoEsJoQdREimpB-xw+g@mail.gmail.com>

Hi,

On 25/06/21 02:09, Linus Walleij wrote:
> On Fri, Jun 25, 2021 at 1:34 AM Pali Rohár <pali@kernel.org> wrote:
> 
>>> gpiod_set_value(gpiod, 1) == assert the line
>>> gpiod_set_value(gpiod, 0) == de-assert the line
>>
>> Problem is that some pci controller drivers (e.g. pci-j721e.c or
>> pcie-rockchip-host.c) expects that gpiod_set_value_cansleep(gpiod, 1)
>> de-asserts the line and it is already used in this way.
>>
>> Which is opposite of the behavior which you wrote above.
> 
> I sketched a way out of the problem using a quirk in
> gpiolib in another response. We have a few of these
> cases where we have to code our way out of mistakes,
> such things happen.
> 
> The problem is common, and due to the fact that device tree
> authors ignores the flag GPIO_ACTIVE_HIGH (which has
> been around since the early days of device tree on PowerPC)
> instead they opt to do the inversion in code. Which violates the
> contract that the DT should describe the hardware.
> 
> The ambition of the DT specifications/schemas are to be operating
> system independent, and this kind of stuff creates a situation
> where other operating systems can't use the specification without
> also going and looking at how Linux has implemented stuff.
> Which is against the ambition of the device tree work.
> 
>> I would suggest to define enum/macro with word ASSERT and DEASSERT in
>> its name instead of just true/false boolean or 0/1 int.
>>
>> In case of this PERST# misunderstanding, having assert/deassert in name
>> should really help.
> 
> Hm that looks useful, Bart &co what do you think?
> 
> #define GPIOD_ASSERTED 1
> #define GPIOD_DEASSERTED 0
> 
> in consumer.h, would that be helpful for users?

Looks like a good idea to me. It would help making people aware that
gpiod_set_value() & co do _not_ take a physical line value. It's done
that way since ages, it's documented, yet many developers are still
unaware of that...

-- 
Luca