No subject

idea of the DeviceTree "gpio-keys" and "gpio-keys-polled"
binding is. The catch with this is that upstream Linux isn't the
only OS with a driver for it. For example FreeBSD has a driver
implementing the bindings as well ... and I think the newer FDT 
u-boots could as well in the future(but not yet?). So it is sort
of important for the thing to behave more or less the same across
the platforms. 

(OpenWrt's gpio-button-hotplug has still a few gotchas:
 - no support for irq-only buttons
 - no wake-up support (noboby really implements pm)
 - only a small list of special buttons (in button_map) are supported
 - no autorepeat support (no idea if this is needed or not)).
 - gpio_keys (non polled) doesn't care about pdata->enable/disable

> > You can see that gpio_keys_polled_check_state() always "ate" initial
> > state event for polled buttons (yes, both states - pressed and unpressed -) got
> > ignored.  Which I think is very wrong... mostly because it goes against the 
> > decades of experience I have with "holding down buttons while powering up devices"
> > and expect them to get noticed properly :D. That's why my code only eats the initial
> > unpressed/0 event, but reports the pressed button event. This should still be
> > compatible with the current failsafe setup. 
> 
> Honestly, i think our expectations divert here a little ;)
> 
> OpenWrt Wiki states "It listens for a button press inside a specific two second window,
> which is indicated with LEDs and by transmitting an UDP package."
> 
> and
> 
> "Recommended for most users: Wait for a flashing LED and press a button.
> This is usually the easiest method once you figure out the correct moment."
> 
> So yes, this differs from the usual "Hold a button and insert power". However,
> entering failsafe on any button state change event would be the 
> behavior i suspect.

Heh, I was about to mention the wiki (and /lib/preinit/30_failsafe_wait)
the as well in the reply. But I cut that part, because it was too long already.
I was basically praising the author who wrote  "... and *press* a button." and
not something along the line of "change the state of the button" in that
LED-blinkenlights window.

/lib/preinit/30_failsafe_wait also echos
 "Please press button now to enter failsafe" as well as
 "- failsafe button "`cat /tmp/failsafe_button`" was pressed -"

it would be odd(er) if this script triggers when a button is released, no?.
 
> > Related Note: 
> > As for the sudden burst of the "device is always entering failsafe".
> > I can see how this is causing problems now. Because if the polarity of
> > a button was declared (or that got copied from another device without
> > checking ;) ) with the wrong way; This will now become a problem because
> > it "physically unpressed" button gets reported as "pressed" and this causes
> > the device to always enter failsafe (unless the very button with the wrong
> > polarity is pressed, which in this context means unpressed).
> 
> This is - sadly - not the reason why I'm observing this issue :(
> Polarity is correctly declared for my devolo board.


Hm, this is what I know about the devilish devolo boards (based on your mails).
(Yes, I read your comment at the end.)

|After configuring the GPIO direction to input, the value of the GPIO reads 0 (pressed).
|After ~10ms, this changes to 1 (not pressed). I suppose your proposed solution does not work
|as interrupts are only registered after configuring the GPIO line as input and the GPIO line
|changes after registering the interrupt. So we are reading the interrupt state too early.

(current) target/linux/ath79/dts/qca9558_devolo_dvl1xxx.dtsi

|        keys {
|                compatible = "gpio-keys";
|
|                reset {
|                        label = "Reset button";
|                        linux,code = <KEY_RESTART>;
|                        gpios = <&gpio 18 GPIO_ACTIVE_LOW>;
|                        debounce-interval = <60>;
|                };
|        };

Since you specified the debounce-interval of 60ms, the gpio-keys (non polled)
variant should not get the faulty/early value since with the proposed patch,
gpio_keys_probe() now uses:

					gpio_keys_button_probe() // sets direction

					[...]

                 schedule_delayed_work(&bdata->work,
                                      msecs_to_jiffies(bdata->software_debounce));

                ret = devm_request_threaded_irq(&pdev->dev,
                        bdata->irq, NULL, button_handle_irq,
                        irqflags, dev_name(&pdev->dev), bdata);


so the work-item that reads and deals with the event is scheduled to run in ~60ms
right before the (gpio)irq gets switched on. This should be enough time to guard
against the gap caused by the gpio direction change.

Note:
If a IRQ is sent out right away, button_handle_irq() calls

        mod_delayed_work(system_wq, &bdata->work,
                         msecs_to_jiffies(bdata->software_debounce));

(so it also waits around for software_debounce to pass).

> > However, I think this is a problem of the individual dts/board support code.
> > But sadly I have currently no time to monitor the forums, bugtracker, ML or
> > github for these problems... so what to do?
> 
> This is clearly nothing we should worry about. I made myself also guilty of this
> mistake and it makes things weird in so many ways.
> 
> It's a bug in the boards integration and nothing we have to catch on our layer :)

we can do debouncing at this layer. 

(I also have a special case with the MyBook Lives. Their "reset key" is currently
not working in OpenWrt because it needs a enable gpio line to be asserted. The
problem with it is that if the line is asserted, the SPI-NOR on which the uboot
resides is switched off. So if the line is gpio-hogged, the MBL won't reboot
anymore and I'm looking to extend the gpio-keys-polled one day to maybe make it
possible to switch the line for the duration of the gpio button read. So there
is finally full support for the device.)

> > 
> > As everyone who follows the thread can see: The struggle is real!
> > Even for something as seemingly simple and benign as a gpio-button.
> 
> Okay, so how do we want to proceed? I would propose to merge your iteration
> (as it was working for me in both modes) to master, wait for people to complain
> and then pickit to 19.07? It should be an improvement over the current situation
> anyways. I would view the failsafe mode on a continuous press as a new 
> "feature" here ;) 

Ok, so the devolo boards are a not failsafing the whole time with that patch
applied? Because I would hate to do that. (I noticed that ath79-gpio emulates
rising & falling IRQ trigger via software, so there's some room to test if
this has an effect on the irq ghosting).

As for how to proceed. Well, the patch version has been up for around a week 
I think. If all stays quiet for the next 8-9 hours, then yes. I'll try to 
take the shot ;).

Regards,
Christian



_______________________________________________
openwrt-devel mailing list
openwrt-devel at lists.openwrt.org
https://lists.openwrt.org/mailman/listinfo/openwrt-devel