Re: [PATCH] Input: tsc200x: Drop hard-coded IRQ edge

[Adam Ford <aford173@gmail.com>]

On Mon, May 10, 2021 at 7:39 PM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> On Mon, May 10, 2021 at 11:29:08AM +0200, Linus Walleij wrote:
> > On Mon, May 10, 2021 at 2:22 AM Dmitry Torokhov
> > <dmitry.torokhov@gmail.com> wrote:
> > > On Mon, May 10, 2021 at 01:38:30AM +0200, Linus Walleij wrote:
> >
> > > > This edge setting should come from the device tree not
> > > > the driver. Also, most device trees sets this to the
> > > > falling edge, which is contradictory to what is hardcoded.
> > >
> > > I see there are 2 possibilities:
> > >
> > > 1. The driver has never worked
> > > 2. DT interrupt annotation is wrong.
> > >
> > > It would be nice to know if we are dealing with 1 or 2, as in case of #2
> > > we need to adjust DTSes before this patch can be applied.
> >
> > I looked closer and unfortunately the mess and confusion
> > is bizarre.
> >
> > The DTS files we know of are:
> > arch/arm/boot/dts/am3517-som.dtsi - rising
> > arch/arm/boot/dts/imx28-tx28.dts - falling
> > arch/arm/boot/dts/imx35-eukrea-cpuimx35.dtsi - low
> > arch/arm/boot/dts/imx51-eukrea-cpuimx51.dtsi - low
> > arch/arm/boot/dts/imx53-tx53-x03x.dts - falling
> > arch/arm/boot/dts/imx6q-dhcom-som.dtsi - falling
> > arch/arm/boot/dts/imx6qdl-tx6.dtsi - none
> > arch/arm/boot/dts/imx6ul-tx6ul.dtsi - none
> > arch/arm/boot/dts/imx7d-nitrogen7.dts - falling
> > arch/arm/boot/dts/logicpd-som-lv.dtsi - rising
> > arch/arm/boot/dts/logicpd-torpedo-baseboard.dtsi - rising
> > arch/arm/boot/dts/omap3-gta04.dtsi - falling
> > arch/arm/boot/dts/omap3-n900.dts - rising
> > arch/arm/boot/dts/omap4-var-som-om44.dtsi - low
> > arch/arm/boot/dts/stm32mp15xx-dhcom-som.dtsi - falling
> >
> > We can assume that some of this is the result of board
> > engineers introducing inverters on the board as is custom,
> > so the flags are actually correct when set to falling, just
> > that we don't model the inverter.
> >
> > In the case of imx6qdl-tx6 and imx6ul-tx6ul with "none" IRQ
> > type I assume this flag in the driver is actually necessary
> > for the device to work at all.
> >
> > In the cases where rising is set, the addition of the flag is
> > plain tautology, just setting what is already set.
> >
> > In the cases where falling are set the interrupts will arrive
> > on both edges (if the hardware can provide that, which is
> > not always the case) and as a result fire twice as many
> > interrupts as they should, probably with zero effect on the
> > second IRQ, just reporting nothing.
>
> That is not how we set up interrupts though. We only use
> platform-supplied trigger if caller did not specify trigger when calling
> request_irq().  From kernel/irq/manage.c::__setup_irq():
>
>         /*
>          * If the trigger type is not specified by the caller,
>          * then use the default for this interrupt.
>          */
>         if (!(new->flags & IRQF_TRIGGER_MASK))
>                 new->flags |= irqd_get_trigger_type(&desc->irq_data);
>
> So in our case, since driver specified IRQF_TRIGGER_RISING it is how
> interrupt line was configured, and what was in DTS had no effect.
>
> >
> > The combination with active low is weird. I wonder what
> > happens there.
> >
> > I am just confused now and have no idea what to do about
> > it...
> >
> > But I just CC all the Freescale and OMAP people who
> > seem to maintain these DTS files so they can clarify
> > how well assigned these edges, none and active low (!)
> > IRQs are.
>
> Hopefully they can confirm how the controller is wired on their boards
> and then we can correct invalid DTSes and then finally apply your patch
> to the driver.

I reviewed the Logicpd Torpedo (DM3730) and there isn't an interter.
I changed the device tree entry for it to falling edge instead and
rising, and it continued to work perfectly.

I'll review both the schematics and test the am3517-evm and the
logicpd som-lv, but I am going to expect the same results since
they'll all basically copy-paste of each other.

Once I've completed my analysis, I'll post device tree updates for all
the logicpd stuff.

adam

>
> Thanks.
>
> --
> Dmitry