Message-ID: <Z30V1KzhqY2drhmm@collins>
Date: Tue, 7 Jan 2025 12:53:56 +0100
From: Paul Kocialkowski <paulk@...-base.io>
To: Linus Walleij <linus.walleij@...aro.org>
Cc: Maxime Ripard <mripard@...nel.org>,
	linux-arm-kernel@...ts.infradead.org, linux-sunxi@...ts.linux.dev,
	linux-kernel@...r.kernel.org,
	Uwe Kleine-König <ukleinek@...nel.org>,
	Chen-Yu Tsai <wens@...e.org>,
	Jernej Skrabec <jernej.skrabec@...il.com>,
	Samuel Holland <samuel@...lland.org>,
	Paul Kocialkowski <contact@...lk.fr>
Subject: Re: [PATCH] pinctrl: sunxi: Use minimal debouncing period as default

Hi Linus,

Le Tue 17 Dec 24, 14:39, Linus Walleij a écrit :
> Some discussion here, and some emotions involved.
> 
> I can't seem to follow the technical matter because of all the
> social matters :/

Thanks for looking into this. It's a bit of a heated discussion but nothing
personal, really. The context is that Maxime and I often disagree on what
constitutes ABI breakage or not. To be fair he has already proven me wrong
in the past and I'm still learning.

The debate here is about what the default behavior should be.

> On Tue, Nov 19, 2024 at 4:00 PM Paul Kocialkowski <paulk@...-base.io> wrote:
> > My use-case here was hooking up a sparkfun sensor board by the way,
> > not some very advanced corner-case.
> 
> Are you adding this as a DT overlay or by modifying an existing device
> tree?

The answer would be an overlay. There's no device that I know of that has this
sensor built-in.

> Does this sensor have an established device tree binding?

It is underway. You can find it at:
https://patchwork.kernel.org/project/linux-iio/patch/20241130174212.3298371-1-paulk@sys-base.io/

Nothing really stands out and the short interrupt time is mentionned there.

> Are you using that sparkfun sensor by a kernel driver or from userspace
> using the GPIO character device?

With a dedicated IIO driver currently under review.

> I noticed that the sunxi GPIO driver is implementing the
> .set_config() callback calling gpiochip_generic_config,
> which makes it call down to the pin control driver to set up
> the pin config.
> 
> which would in turn make it eligible to use
> the gpiod_set_debounce() callback to push down the debounce
> period.
> 
> But pinctrl-sunxi.c's sunxi_pconf_set() does *NOT* implement
> support for setting up the debounce, because it is (as I understand
> it) not part of the pin config hardware, but part of the interrupt
> generator hardware, correct?

Yes I believe this is correct. Debouncing is said (according to scarce
documentation) to be tied to the interrupt part, not GPIO in general.

> In that case I think we the gpiochip .set_config() callback should
> be modified something like this (pseudo code):
> 
> sunxi_pinctrl_gpio_set_config()
> {
>     if (irq_is_in_use && param_is_debounce) {
>         modify_irq_debounce_according_to_param()
>         return 0;
>     }
>     gpiochip_generic_config()
> }
> 
> pctl->chip->set_config = sunxi_pinctrl_gpio_set_config()

I didn't even know the gpio API supported this, thanks!
Then it would make sense for the driver to specify this.

> Maybe the debounce can also be set even if the line is not used
> for IRQ? I'm not sure.

My guess would be that it can't.

> In either case the latter would give the GPIO driver a handle
> on the debounce, which is good because the irqchip
> generally does not.
> 
> There is a way it is possible to use the interrupt with desired debounce
> setting by first getting the GPIO descriptor and modify the debounce
> setting and then getting the interrupt from the GPIO descriptor:

However I see the debounce time is specified in microseconds, which is longer
that the minimum achievable and probably too close to the limit for the sensor
board, that generates the interrupt for 1 us only.

Is it specified that a value of 0 to gpiod_set_debounce would select the
lowest debouncing time?

> struct gpio_desc *g;
> int irq;
> 
> g = gpiod_get(dev, "dataready", GPIOD_IN);
> gpiod_set_debounce(g, 1);
> irq = gpiod_to_irq(g);
> ...request irq...
> 
> Here I assume the line out from the sensor is named "dataready"
> the actual component likely has a name like that for the line.
> 
> This requires changes in the device tree as well, so that a GPIO
> line is assigned to the sensor instead of "just" an interrupt:
> 
> sensor {
>   dataready-gpios = <&gpio 14>;
>   ...
> };
> 
> instead of:
> 
> sensor {
>   interrupt-parent = <&gpio>;
>   interrupts = <14 IRQ_TYPE_EDGE_RISING>;
> };
> 
> Unfortunately this is one of the areas where DT bindings are
> a bit ambiguous. Some just assign the GPIO line as an interrupt
> as both APIs are available, sometimes a proper GPIO line is
> preferred for the above reasons.

I can see how that would be confusing indeed. And the same change would be
required for pretty much any driver that could have the same problem.
I'm not sure this is desirable.

Would there be a way to do the opposite and grab the gpio handle from the
irq line, as to leave the dt binding unchanged?

Cheers,

Paul

-- 
Paul Kocialkowski,

Independent contractor - sys-base - https://www.sys-base.io/
Free software developer - https://www.paulk.fr/

Expert in multimedia, graphics and embedded hardware support with Linux.

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)

Powered by blists - more mailing lists