Re: [PATCH v3 3/3] arm64: dts: qcom: apq8016: Add Schneider HMIBSC board DTS

[Stephan Gerhold]

On Wed, Mar 20, 2024 at 12:10:32PM +0530, Sumit Garg wrote:
> On Tue, 19 Mar 2024 at 19:43, Stephan Gerhold <stephan@xxxxxxxxxxx> wrote:
> > On Mon, Mar 18, 2024 at 03:20:46PM +0530, Sumit Garg wrote:
> > > On Fri, 15 Mar 2024 at 20:43, Stephan Gerhold <stephan@xxxxxxxxxxx> wrote:
> > > > On Fri, Mar 15, 2024 at 11:37:07AM +0530, Sumit Garg wrote:
> > > > > Add Schneider Electric HMIBSC board DTS. The HMIBSC board is an IIoT Edge
> > > > > Box Core board based on the Qualcomm APQ8016E SoC.
> > > > >
> > > > > Support for Schneider Electric HMIBSC. Features:
> > > > > - Qualcomm Snapdragon 410C SoC - APQ8016 (4xCortex A53, Adreno 306)
> > > > > - 1GiB RAM
> > > > > - 8GiB eMMC, SD slot
> > > > > - WiFi and Bluetooth
> > > > > - 2x Host, 1x Device USB port
> > > > > - HDMI
> > > > > - Discrete TPM2 chip over SPI
> > > > > - USB ethernet adaptors (soldered)
> > > > >
> > > > > Co-developed-by: Jagdish Gediya <jagdish.gediya@xxxxxxxxxx>
> > > > > Signed-off-by: Jagdish Gediya <jagdish.gediya@xxxxxxxxxx>
> > > > > Reviewed-by: Caleb Connolly <caleb.connolly@xxxxxxxxxx>
> > > > > Signed-off-by: Sumit Garg <sumit.garg@xxxxxxxxxx>
> > > > > ---
> > > > >  arch/arm64/boot/dts/qcom/Makefile             |   1 +
> > > > >  .../dts/qcom/apq8016-schneider-hmibsc.dts     | 510 ++++++++++++++++++
> > > > >  2 files changed, 511 insertions(+)
> > > > >  create mode 100644 arch/arm64/boot/dts/qcom/apq8016-schneider-hmibsc.dts
> > > > >
> > > > > diff --git a/arch/arm64/boot/dts/qcom/Makefile b/arch/arm64/boot/dts/qcom/Makefile
> > > > > index 39889d5f8e12..ad55e52e950b 100644
> > > > > --- a/arch/arm64/boot/dts/qcom/Makefile
> > > > > +++ b/arch/arm64/boot/dts/qcom/Makefile
> > > > > @@ -5,6 +5,7 @@ apq8016-sbc-usb-host-dtbs     := apq8016-sbc.dtb apq8016-sbc-usb-host.dtbo
> > > > >
> > > > >  dtb-$(CONFIG_ARCH_QCOM)      += apq8016-sbc-usb-host.dtb
> > > > >  dtb-$(CONFIG_ARCH_QCOM)      += apq8016-sbc-d3-camera-mezzanine.dtb
> > > > > +dtb-$(CONFIG_ARCH_QCOM)      += apq8016-schneider-hmibsc.dtb
> > > > >  dtb-$(CONFIG_ARCH_QCOM)      += apq8039-t2.dtb
> > > > >  dtb-$(CONFIG_ARCH_QCOM)      += apq8094-sony-xperia-kitakami-karin_windy.dtb
> > > > >  dtb-$(CONFIG_ARCH_QCOM)      += apq8096-db820c.dtb
> > > > > diff --git a/arch/arm64/boot/dts/qcom/apq8016-schneider-hmibsc.dts b/arch/arm64/boot/dts/qcom/apq8016-schneider-hmibsc.dts
> > > > > new file mode 100644
> > > > > index 000000000000..9c79a31a04db
> > > > > --- /dev/null
> > > > > +++ b/arch/arm64/boot/dts/qcom/apq8016-schneider-hmibsc.dts
> > > > > @@ -0,0 +1,510 @@
> > > > > [...]
> > > > > +
> > > > > +&pm8916_resin {
> > > > > +     interrupts = <0x0 0x8 1 IRQ_TYPE_EDGE_FALLING>;
> > > > > +     linux,code = <KEY_POWER>;
> > > > > +     status = "okay";
> > > > > +};
> > > >
> > > > What is the goal of overriding the interrupt here? It looks like you are
> > > > changing the interrupt type from IRQ_TYPE_EDGE_BOTH to FALLING. This
> > > > sounds a bit like you want the driver to receive just button release
> > > > events (or just press events, not sure about the polarity). I'm not sure
> > > > if the driver will handle this correctly.
> > >
> > > The use-case here is to just act upon button release events and the
> > > driver handles this appropriately. Final use-case of the reset button:
> > >
> > > - Short press and release leads to normal Linux reboot.
> > > - Long press for more than 10 sec or so leads to a hard reset.
> > >
> > > With IRQ_TYPE_EDGE_BOTH, that's not achievable because just a simple
> > > press leads to Linux reboot.
> > >
> >
> > Thanks for explaining your use case. Is the DT really the right place to
> > describe this? In the hardware, this is just a button that provides both
> > press and release events. Linux typically forwards these events to user
> > space, without interpreting them in any way. This means you likely have
> > some user space component that listens to the events (e.g. systemd
> > logind). Ideally that component should be reconfigured to trigger the
> > reboot on release instead of press.
> 
> I am not sure if that's really the case. I only see power key value to
> be reported as follows:
> 
> input_report_key(pwrkey->input, pwrkey->code, 1);
>                     or
> input_report_key(pwrkey->input, pwrkey->code, 0);
> 
> It's not like a press event being a rising edge (0->1) or a release
> event being a falling edge (1->0) reported. AFAICS, a reboot is issued
> whenever the value of power key is reported as "1".
> 

If you look inside the input_report_key() function you can see that the
input subsystem internally tracks the key state. input_get_disposition()
returns INPUT_IGNORE_EVENT if the key bit already has the same value.
Only when the key changes its state, an event is sent to user space.
This means that all events reported to user space are effectively
rising/falling edges (an event with value "1" is a rising edge 0->1, an
event with value "0" is a falling edge 1->0).

The only reason why setting IRQ_TYPE_EDGE_FALLING works here is because
of the workaround added in commit be8fc023ef64 ("Input: pm8941-pwrkey -
simulate missed key press events") [1]. No event is reported when you
start pressing the key. When you release the key, you get a key press
event (rising edge) immediately followed by a key release (falling
edge). But the workaround was added to handle potentially missed
interrupts, not to inhibit reporting key press events.

In my opinion, if you want to perform an action on key release rather
than key press then you should adjust the user space program to do so.
>From the point of view of the hardware DT (and even the kernel), the key
press happens when you actually start pressing the key, and not later
when you release it.

Thanks,
Stephan

[1]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=be8fc023ef64dcb11042aaa4bb0f29f7e0335d85