Re: [PATCH] pinctrl: sunxi: Use minimal debouncing period as default

[Maxime Ripard <mripard@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 4141 bytes --]

On Tue, Nov 19, 2024 at 04:00:48PM +0100, Paul Kocialkowski wrote:
> Hi Maxime,
> 
> Le Tue 19 Nov 24, 15:43, Maxime Ripard a écrit :
> > On Tue, Nov 19, 2024 at 03:08:05PM +0100, Paul Kocialkowski wrote:
> > > From: Paul Kocialkowski <contact@paulk.fr>
> > > 
> > > The sunxi external interrupts (available from GPIO pins) come with a
> > > built-in debouncing mechanism that cannot be disabled. It can be
> > > configured to use either the low-frequency oscillator (32 KHz) or the
> > > high-frequency oscillator (24 MHz), with a pre-scaler.
> > > 
> > > The pinctrl code supports an input-debounce device-tree property to set
> > > a specific debouncing period and choose which clock source is most
> > > relevant. However the property is specified in microseconds, which is
> > > longer than the minimal period achievable from the high-frequency
> > > oscillator without a pre-scaler.
> > 
> > That can be fixed by introducing a new property with a ns resolution.
> 
> Sure but my point here is rather about what should be default behavior.
> 
> The issue I had will remain unsolved by default even with a new property,
> since people will still need to patch their device-tree to apply it.
> 
> > > When the property is missing, the reset configuration is kept, which
> > > selects the low-frequency oscillator without pre-scaling. This severely
> > > limits the possible interrupt periods that can be detected.
> > > 
> > > Instead of keeping this default, use the minimal debouncing period from
> > > the high-frequency oscillator without a pre-scaler to allow the largest
> > > possible range of interrupt periods.
> > > 
> > > This issue was encountered with a peripheral that generates active-low
> > > interrupts for 1 us. No interrupt was detected with the default setup,
> > > while it is now correctly detected with this change.
> > 
> > I don't think it's wise. If the debouncing is kept as is, the worst case
> > scenario is the one you had: a device doesn't work, you change it,
> > everything works.
> 
> I think this worst-case scenario is very bad and not what people will
> expect. In addition it is difficult to debug the issue without specific
> knowledge about the SoC.
>
> My use-case here was hooking up a sparkfun sensor board by the way,
> not some very advanced corner-case.

Are you really arguing that a single sparkfun sensor not working is a
worse outcome than the system not booting?

> > If we set it up as fast as it can however, then our risk becomes
> > thousands of spurious interrupts, which is much more detrimental to the
> > system.
> 
> Keep in mind that this only concerns external GPIO-based interrupts,
> which have to be explicitely hooked to a device. If a device or circuit
> does generate such spurious interrupts, I think it makes sense that it
> would be reflected by default.

I mean... debouncing is here for a reason. Any hardware button will
generate plenty of interrupts when you press it precisely because it
bounces.

> Also the notion of spurious interrupt is pretty vague. Having lots of
> interrupts happening may be the desired behavior in many cases.

Which cases?

> In any case I don't think it makes sense for the platform code to impose
> what a reasonable period for interrupts is (especially with such a large
> period as default).

So you don't think it makes sense for the platform code to impose a
reasonable period, so you want to impose a (more, obviously) reasonable
period?

If anything, the status quo doesn't impose anything, it just rolls with
the hardware default. Yours would impose one though.

> Some drivers also have mechanisms to detect spurious interrupts based
> on their specific use case.
> 
> > And that's without accounting the fact that devices might have relied on
> > that default for years
> 
> They definitely shouldn't have. This feels much closer to a bug, and relying
> on a bug not being fixed is not a reasonable expectation.

No, it's not a bug, really. It might be inconvenient to you, and that's
fine, but it's definitely not a bug.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]