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

[AngeloGioacchino Del Regno]

Il 18/01/21 14:19, Linus Walleij ha scritto:
> On Mon, Jan 11, 2021 at 7:29 PM AngeloGioacchino Del Regno
> <angelogioacchino.delregno@xxxxxxxxxxxxxx> wrote:
>
> > The Awinic AW9523(B) is a multi-function I2C gpio expander in a
> > TQFN-24L package, featuring PWM (max 37mA per pin, or total max
> > power 3.2Watts) for LED driving capability.
> >
> > It has two ports with 8 pins per port (for a total of 16 pins),
> > configurable as either PWM with 1/256 stepping or GPIO input/output,
> > 1.8V logic input; each GPIO can be configured as input or output
> > independently from each other.
> >
> > This IC also has an internal interrupt controller, which is capable
> > of generating an interrupt for each GPIO, depending on the
> > configuration, and will raise an interrupt on the INTN pin to
> > advertise this to an external interrupt controller.
> >
> > Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@xxxxxxxxxxxxxx>
>
> I really like the looks of this new version! :)
>
> Just some minor questions now:
>
> > +static int aw9523_gpio_irq_type(struct irq_data *d, unsigned int type)
> > +{
> > +       switch (type) {
> > +       case IRQ_TYPE_NONE:
> > +       case IRQ_TYPE_LEVEL_MASK:
> > +       case IRQ_TYPE_LEVEL_HIGH:
> > +       case IRQ_TYPE_LEVEL_LOW:
> > +               return 0;
>
> This still doesn't make sense, I think you can see why.
>
> If level IRQs can fire both when the line is high and low without
> any configuration in any register whatsoever, then IRQs are
> *constantly* fireing. Which I suppose they are not.
>
> Something is wrong here. Either you just support one
> of them or there is a way to configure whether to act as
> high or low level.
>
> I'm also mildly sceptic because GPIO expanders ... do they
> really support level IRQs, and does it make sense when you
> think about it? I would rather expect edge IRQs as these have
> a natural trigger point and level IRQs may very well be gone
> by the time you come around to read the status register.
> I suppose it can be done but the events must be really slow
> then, slower than the 400kHz of an I2C bus.
>
> Can you please look over the interrupt logic in your specs?
>
> Yours,
> Linus Walleij

Have I misunderstood this part of the API?

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.

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.

Thanks
- Angelo