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

[Sumit Garg]

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 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).

As of now there isn't such a use-case for the HMIBSC board.

-Sumit

>
> Thanks,
> Stephan