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

[Stephan Gerhold]

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.

If you hardcode this behavior in the DT you are essentially describing
that the hardware is incapable of detecting the press event before the
release event. This is not the case, right? There may be use cases where
someone would still want to detect the key press (rather than just
release).

Thanks,
Stephan