Re: [PATCH v2 1/2] pinctrl: Add driver for Awinic AW9523/B I2C GPIO Expander

From: Linus Walleij <linus.walleij@linaro.org>
To: AngeloGioacchino Del Regno  <angelogioacchino.delregno@somainline.org>
Cc: "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	konrad.dybcio@somainline.org, marijn.suijten@somainline.org,
	martin.botka@somainline.org, phone-devel@vger.kernel.org,
	"open list:GPIO SUBSYSTEM" <linux-gpio@vger.kernel.org>,
	"open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
	<devicetree@vger.kernel.org>, Rob Herring <robh+dt@kernel.org>
Subject: Re: [PATCH v2 1/2] pinctrl: Add driver for Awinic AW9523/B I2C GPIO Expander
Date: Fri, 22 Jan 2021 10:59:50 +0100	[thread overview]
Message-ID: <CACRpkdacfa6usOZtc+A=ZxEpB1ij_gAKX2PLMOaX0mY_0qHp6A@mail.gmail.com> (raw)
In-Reply-To: <1e34145b-a04a-1cbb-7fbc-87c69b8dcfd7@somainline.org>

On Mon, Jan 18, 2021 at 3:38 PM AngeloGioacchino Del Regno
<angelogioacchino.delregno@somainline.org> wrote:

> By the way, this is really LEVEL irq, not EDGE... To avoid any
> misunderstanding, I think that the best way to show you what I
> am seeing is to just copy-paste the relevant piece from the
> datasheet for this hardware (it's not a confidential datasheet
> and freely found on the internet).
>
> Check this out:
> " External MCU is required acknowledge by INTN pin. INTN is open-drain
> out-
> put, low-level active, and need external pull-up resistor.

This talks about what polarity (active low) the pin from the expander
to the SoC/CPU is. It has nothing to do with the line into the
expander.

> When AW9523B detect port change, any input state from high-level to
> low-level or from
>   low-level to high-level will generate interrupt after
> 8us internal deglitch. "
>
> ...but since the datasheet is sometimes unclear about "things" (I am
> mostly sure that they have translated it to english from chinese), I
> have actually checked whether the INTN pin was pushed LOW when one of
> the inputs goes from HIGH to LOW.. and.. it does... and as you imagine
> yeah.. it's slow.. and yes, as slow as you can imagine. :)
>
> So, in short, this chip is raising an interrupt when any input changes
> state, regardless of the change being LOW->HIGH or HIGH->LOW.

This means that the expander only supports
IRQ_TYPE_EDGE_BOTH and nothing else.

"port change" above means edges.

Augment your driver to only accept this type.

The consumers better request IRQ_TYPE_EDGE_BOTH
(from a device tree for example) and consumers better
handle the fact that they get interrupts on both rising
and falling edge as well, else they may need special
code to handle it. This is not a very nice feature of
the expander, it would be more helpful to users to
get interrupts on only rising or only falling edges, but
as written, it will generate interrupts on both transitions.

Yours,
Linus Walleij