Feedback-ID: i8771445c:Fastmail
Date:   Mon, 13 Feb 2023 10:18:03 +0100
From:   Maxime Ripard <maxime@cerno.tech>
To:     pelzi@flying-snail.de
Cc:     Andre Przywara <andre.przywara@arm.com>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Chen-Yu Tsai <wens@csie.org>,
        Jernej Skrabec <jernej.skrabec@gmail.com>,
        Samuel Holland <samuel@sholland.org>,
        devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        linux-sunxi@lists.linux.dev, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] ARM: dts: allwinner: minimize irq debounce filter per
 default
Message-ID: <20230213091803.bxle6ly2sapodsbs@houat>
References: <Y+FaVorMl37F5Dve@debian-qemu.internal.flying-snail.de>
 <20230207011608.2ce24d17@slackpad.lan>
 <d0534762-3785-ec2d-8d1e-aba0e39f701b@feldner-bv.de>
 <20230209202952.673d5a60@slackpad.lan>
 <20230210082936.qefzz4fsp3jpalvp@houat>
 <20230210094425.474cfba5@donnerap.cambridge.arm.com>
 <20230210100620.z6j7rvkiwyu7paij@houat>
 <20230210101814.2d36ae57@donnerap.cambridge.arm.com>
 <20230213084329.ulckaigwd7dof37u@houat>
 <c3dda403-6963-040a-3827-443edf0a377a@flying-snail.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable
In-Reply-To: <c3dda403-6963-040a-3827-443edf0a377a@flying-snail.de>
Precedence: bulk

On Mon, Feb 13, 2023 at 09:49:55AM +0100, pelzi@flying-snail.de wrote:
> Am 13.02.23 um 09:43 schrieb Maxime Ripard:
> > On Fri, Feb 10, 2023 at 10:18:14AM +0000, Andre Przywara wrote:
> > > > > Not sure if you were actually arguing this, but the change I sket=
ched
> > > > > above (interpreting 0 as 24MHz/1) is separate though, as the curr=
ent
> > > > > default is "no DT property", and not 0. There is no input-debounce
> > > > > property user in the kernel tree at the moment, so we wouldn't br=
eak
> > > > > anyone. The only thing that would change is if a downstream user =
was
> > > > > relying on "0" being interpreted as "skip the setup", which isn't
> > > > > really documented and could be argued to be an implementation det=
ail.
> > > > >=20
> > > > > So I'd suggest to implement 0 as "lowest possible", and documenti=
ng that
> > > > > and the 32KHz/1 default if no property is given.
> > > > Ah, my bad.
> > > >=20
> > > > There's another thing to consider: there's already a generic per-pin
> > > > input-debounce property in pinctrl.
> > > >=20
> > > > Since we can't control it per pin but per bank, we moved it to the
> > > > controller back then, but there's always been this (implicit)
> > > > expectation that it was behaving the same way.
> > > >=20
> > > > And the generic, per-pin, input-debounce documentation says:
> > > >=20
> > > > > Takes the debounce time in usec as argument or 0 to disable debou=
ncing
> > > > I agree that silently ignoring it is not great, but interpreting 0 =
as
> > > > the lowest possible is breaking that behaviour which, I believe, is=
 a
> > > > worse outcome.
> > > Is it really? If I understand the hardware manuals correctly, we cann=
ot
> > > really turn that feature off, so isn't the lowest possible time perio=
d (24
> > > MHz/1 at the moment) the closest we can get to "turn it off"? So
> > > implementing this would bring us actually closer to the documented
> > > behaviour? Or did I get the meaning of this time period wrong?
> > > At least that's my understanding of how it fixed Andreas' problem: 1=
=B5s
> > > is still not "off", but much better than the 31=B5s of the default. T=
he new
> > > 0 would then be 0.041=B5s.
> > My point was that the property we share the name (and should share the
> > semantics with) documents 0 as disabled. We would have a behavior that
> > doesn't disable it. It's inconsistent.
> >=20
> > The reason doesn't really matter, we would share the same name but have
> > a completely different behavior, this is super confusing to me.
>=20
> I got the point. As far as I can tell from the datasheet, it is not possi=
ble
> to actually switch off input-debounce. But as a debounce filter is actual=
ly
> a low-pass filter, setting the cut-off frequency as high as possible,
> appears to be the equivalent to switching it off.

It's not really a matter of hardware here, it's a matter of driver
behavior vs generic behavior from the framework. The hardware obviously
influences the former, but it's marginal in that discussion.

As that whole discussion shows, whether the frequency would be high
enough is application dependent, and thus we cannot just claim that it's
equivalent in all circumstances.

Making such an assumption will just bite someone else down the road,
except this time we will have users (you, I'd assume) relying on that
behavior so we wouldn't be able to address it.

But I also agree with the fact that doing nothing with 0 is bad UX and
confusing as well.

I still think that we can address both by just erroring out on 0 /
printing an error message so that it's obvious that we can't support it,
and we wouldn't change the semantics of the property either.

And then you can set the actual debouncing time you need instead of
"whatever" in the device tree.

Maxime