* [PATCH v6 03/36] ARM: dts: qcom: msm8960: introduce label for PMIC keypad
From: Dmitry Baryshkov @ 2023-09-28 11:02 UTC (permalink / raw)
To: Andy Gross, Bjorn Andersson, Konrad Dybcio, Rob Herring,
Krzysztof Kozlowski
Cc: linux-arm-msm, devicetree, linux-input
In-Reply-To: <20230928110309.1212221-1-dmitry.baryshkov@linaro.org>
To simplify MSM8960 CDP board file, add label to PMIC keypad node.
Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
arch/arm/boot/dts/qcom/qcom-msm8960-cdp.dts | 20 +++++++++-----------
arch/arm/boot/dts/qcom/qcom-msm8960.dtsi | 2 +-
2 files changed, 10 insertions(+), 12 deletions(-)
diff --git a/arch/arm/boot/dts/qcom/qcom-msm8960-cdp.dts b/arch/arm/boot/dts/qcom/qcom-msm8960-cdp.dts
index 6c1bc3818883..4641b4f2195d 100644
--- a/arch/arm/boot/dts/qcom/qcom-msm8960-cdp.dts
+++ b/arch/arm/boot/dts/qcom/qcom-msm8960-cdp.dts
@@ -88,17 +88,15 @@ clk-pins {
};
};
-&pmicintc {
- keypad@148 {
- linux,keymap = <
- MATRIX_KEY(0, 0, KEY_VOLUMEUP)
- MATRIX_KEY(0, 1, KEY_VOLUMEDOWN)
- MATRIX_KEY(0, 2, KEY_CAMERA_FOCUS)
- MATRIX_KEY(0, 3, KEY_CAMERA)
- >;
- keypad,num-rows = <1>;
- keypad,num-columns = <5>;
- };
+&pm8921_keypad {
+ linux,keymap = <
+ MATRIX_KEY(0, 0, KEY_VOLUMEUP)
+ MATRIX_KEY(0, 1, KEY_VOLUMEDOWN)
+ MATRIX_KEY(0, 2, KEY_CAMERA_FOCUS)
+ MATRIX_KEY(0, 3, KEY_CAMERA)
+ >;
+ keypad,num-rows = <1>;
+ keypad,num-columns = <5>;
};
&rpm {
diff --git a/arch/arm/boot/dts/qcom/qcom-msm8960.dtsi b/arch/arm/boot/dts/qcom/qcom-msm8960.dtsi
index d13080fcbeea..a34fda93d6a4 100644
--- a/arch/arm/boot/dts/qcom/qcom-msm8960.dtsi
+++ b/arch/arm/boot/dts/qcom/qcom-msm8960.dtsi
@@ -283,7 +283,7 @@ pwrkey@1c {
pull-up;
};
- keypad@148 {
+ pm8921_keypad: keypad@148 {
compatible = "qcom,pm8921-keypad";
reg = <0x148>;
interrupt-parent = <&pmicintc>;
--
2.39.2
^ permalink raw reply related
* [PATCH v6 02/36] ARM: dts: qcom: apq8064: correct XOADC register address
From: Dmitry Baryshkov @ 2023-09-28 11:02 UTC (permalink / raw)
To: Andy Gross, Bjorn Andersson, Konrad Dybcio, Rob Herring,
Krzysztof Kozlowski
Cc: linux-arm-msm, devicetree, linux-input, Krzysztof Kozlowski
In-Reply-To: <20230928110309.1212221-1-dmitry.baryshkov@linaro.org>
The XOADC is present at the address 0x197 rather than just 197. It
doesn't change a lot (since the driver hardcodes all register
addresses), but the DT should present correct address anyway.
Fixes: c4b70883ee33 ("ARM: dts: add XOADC and IIO HWMON to APQ8064")
Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
arch/arm/boot/dts/qcom/qcom-apq8064.dtsi | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm/boot/dts/qcom/qcom-apq8064.dtsi b/arch/arm/boot/dts/qcom/qcom-apq8064.dtsi
index 516f0d2495e2..950adb63af70 100644
--- a/arch/arm/boot/dts/qcom/qcom-apq8064.dtsi
+++ b/arch/arm/boot/dts/qcom/qcom-apq8064.dtsi
@@ -738,7 +738,7 @@ pwrkey@1c {
xoadc: xoadc@197 {
compatible = "qcom,pm8921-adc";
- reg = <197>;
+ reg = <0x197>;
interrupts-extended = <&pmicintc 78 IRQ_TYPE_EDGE_RISING>;
#address-cells = <2>;
#size-cells = <0>;
--
2.39.2
^ permalink raw reply related
* [PATCH v6 01/36] dt-bindings: input: qcom,pm8921-keypad: convert to YAML format
From: Dmitry Baryshkov @ 2023-09-28 11:02 UTC (permalink / raw)
To: Andy Gross, Bjorn Andersson, Konrad Dybcio, Rob Herring,
Krzysztof Kozlowski
Cc: linux-arm-msm, devicetree, linux-input, Dmitry Torokhov,
Krzysztof Kozlowski
In-Reply-To: <20230928110309.1212221-1-dmitry.baryshkov@linaro.org>
Convert the bindings for the keypad subdevices of Qualcomm PM8921 and
PM8058 PMICs from text to YAML format.
While doing the conversion also drop the linux,keypad-no-autorepeat
The property was never used by DT files. Both input and DT binding
maintainers consider that bindings should switch to assertive
(linux,autorepeat) instead of negating (no-autorepeat) property.
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
.../bindings/input/qcom,pm8921-keypad.yaml | 89 ++++++++++++++++++
.../bindings/input/qcom,pm8xxx-keypad.txt | 90 -------------------
2 files changed, 89 insertions(+), 90 deletions(-)
create mode 100644 Documentation/devicetree/bindings/input/qcom,pm8921-keypad.yaml
delete mode 100644 Documentation/devicetree/bindings/input/qcom,pm8xxx-keypad.txt
diff --git a/Documentation/devicetree/bindings/input/qcom,pm8921-keypad.yaml b/Documentation/devicetree/bindings/input/qcom,pm8921-keypad.yaml
new file mode 100644
index 000000000000..88764adcd696
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/qcom,pm8921-keypad.yaml
@@ -0,0 +1,89 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/input/qcom,pm8921-keypad.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm PM8921 PMIC KeyPad
+
+maintainers:
+ - Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
+
+allOf:
+ - $ref: input.yaml#
+ - $ref: matrix-keymap.yaml#
+
+properties:
+ compatible:
+ enum:
+ - qcom,pm8058-keypad
+ - qcom,pm8921-keypad
+
+ reg:
+ maxItems: 1
+
+ interrupts:
+ items:
+ - description: key sense
+ - description: key stuck
+
+ wakeup-source:
+ type: boolean
+ description: use any event on keypad as wakeup event
+
+ linux,keypad-wakeup:
+ type: boolean
+ deprecated: true
+ description: legacy version of the wakeup-source property
+
+ debounce:
+ description:
+ Time in microseconds that key must be pressed or
+ released for state change interrupt to trigger.
+ $ref: /schemas/types.yaml#/definitions/uint32
+
+ scan-delay:
+ $ref: /schemas/types.yaml#/definitions/uint32
+ description: time in microseconds to pause between successive scans of the
+ matrix array
+
+ row-hold:
+ $ref: /schemas/types.yaml#/definitions/uint32
+ description: time in nanoseconds to pause between scans of each row in the
+ matrix array.
+
+required:
+ - compatible
+ - reg
+ - interrupts
+ - linux,keymap
+
+unevaluatedProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/input/input.h>
+ #include <dt-bindings/interrupt-controller/irq.h>
+ pmic {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ keypad@148 {
+ compatible = "qcom,pm8921-keypad";
+ reg = <0x148>;
+ interrupt-parent = <&pmicintc>;
+ interrupts = <74 IRQ_TYPE_EDGE_RISING>, <75 IRQ_TYPE_EDGE_RISING>;
+ linux,keymap = <
+ MATRIX_KEY(0, 0, KEY_VOLUMEUP)
+ MATRIX_KEY(0, 1, KEY_VOLUMEDOWN)
+ MATRIX_KEY(0, 2, KEY_CAMERA_FOCUS)
+ MATRIX_KEY(0, 3, KEY_CAMERA)
+ >;
+ keypad,num-rows = <1>;
+ keypad,num-columns = <5>;
+ debounce = <15>;
+ scan-delay = <32>;
+ row-hold = <91500>;
+ };
+ };
+...
diff --git a/Documentation/devicetree/bindings/input/qcom,pm8xxx-keypad.txt b/Documentation/devicetree/bindings/input/qcom,pm8xxx-keypad.txt
deleted file mode 100644
index 4a9dc6ba96b1..000000000000
--- a/Documentation/devicetree/bindings/input/qcom,pm8xxx-keypad.txt
+++ /dev/null
@@ -1,90 +0,0 @@
-Qualcomm PM8xxx PMIC Keypad
-
-PROPERTIES
-
-- compatible:
- Usage: required
- Value type: <string>
- Definition: must be one of:
- "qcom,pm8058-keypad"
- "qcom,pm8921-keypad"
-
-- reg:
- Usage: required
- Value type: <prop-encoded-array>
- Definition: address of keypad control register
-
-- interrupts:
- Usage: required
- Value type: <prop-encoded-array>
- Definition: the first interrupt specifies the key sense interrupt
- and the second interrupt specifies the key stuck interrupt.
- The format of the specifier is defined by the binding
- document describing the node's interrupt parent.
-
-- linux,keymap:
- Usage: required
- Value type: <prop-encoded-array>
- Definition: the linux keymap. More information can be found in
- input/matrix-keymap.txt.
-
-- linux,keypad-no-autorepeat:
- Usage: optional
- Value type: <bool>
- Definition: don't enable autorepeat feature.
-
-- wakeup-source:
- Usage: optional
- Value type: <bool>
- Definition: use any event on keypad as wakeup event.
- (Legacy property supported: "linux,keypad-wakeup")
-
-- keypad,num-rows:
- Usage: required
- Value type: <u32>
- Definition: number of rows in the keymap. More information can be found
- in input/matrix-keymap.txt.
-
-- keypad,num-columns:
- Usage: required
- Value type: <u32>
- Definition: number of columns in the keymap. More information can be
- found in input/matrix-keymap.txt.
-
-- debounce:
- Usage: optional
- Value type: <u32>
- Definition: time in microseconds that key must be pressed or release
- for key sense interrupt to trigger.
-
-- scan-delay:
- Usage: optional
- Value type: <u32>
- Definition: time in microseconds to pause between successive scans
- of the matrix array.
-
-- row-hold:
- Usage: optional
- Value type: <u32>
- Definition: time in nanoseconds to pause between scans of each row in
- the matrix array.
-
-EXAMPLE
-
- keypad@148 {
- compatible = "qcom,pm8921-keypad";
- reg = <0x148>;
- interrupt-parent = <&pmicintc>;
- interrupts = <74 1>, <75 1>;
- linux,keymap = <
- MATRIX_KEY(0, 0, KEY_VOLUMEUP)
- MATRIX_KEY(0, 1, KEY_VOLUMEDOWN)
- MATRIX_KEY(0, 2, KEY_CAMERA_FOCUS)
- MATRIX_KEY(0, 3, KEY_CAMERA)
- >;
- keypad,num-rows = <1>;
- keypad,num-columns = <5>;
- debounce = <15>;
- scan-delay = <32>;
- row-hold = <91500>;
- };
--
2.39.2
^ permalink raw reply related
* [PATCH v6 00/36] ARM: dts: qcom: cleanup PMIC usage
From: Dmitry Baryshkov @ 2023-09-28 11:02 UTC (permalink / raw)
To: Andy Gross, Bjorn Andersson, Konrad Dybcio, Rob Herring,
Krzysztof Kozlowski
Cc: linux-arm-msm, devicetree, linux-input
While reviewing APQ8064 CPUFreq patchset, Konrad pointed out that PMICs
are not a part of SoC and as such do not belong to the per-SoC files.
Cleanup the way 32-bit Qualcomm platforms treat PMICs:
- Move SSBI PMICs to separate files (as a bonus merging two different
instances of PM8921, benefitting both platforms).
- Include such PMIC files only from the board files, keeping SoC file
generic.
- Move RPM regulator definitions to board files too. They do not belong
to the SoC dtsi files for the same reason.
- Move PMIC-specific GPIOs and supply properties to individual board
files.
Note, enabling DT schema triggers warnings for pmic:led@48 in
qcom-apq8060-dragonboard.dts. This node uses custom ('cm3605') trigger
to make the LED follow the state of the proximity / ALS device.
Previously [1] Rob pointed out that this is not the best way and the
device should be switched to `trigger-sources' instead. However as I do
not have this device, I'm not brave enough to introduce these changes.
Note2: DT binding changes are largely independent from the DTS changes,
they can be applied separately.
[1] https://lore.kernel.org/linux-arm-msm/20221205220709.GA2713165-robh@kernel.org
Changes since v5:
- Dropped accepted patches
- Provided proper commit message for the last two patches (Konrad)
Changes since v4:
- Rebased on top of linux-next
Changes since v3:
- Dropped the interrupts/interrupts-extended patch, it is handled by
dtschema itself (Krzysztof)
Changes since v3:
- Moved PMIC interrupts to board DT files, they are not a property of
the board, not the SoC.
- Dropped qcom, prefix from ssbi node names in ipq8064 and mdm9615 DT
files.
Changes since v2:
- Rebased on top of linux-next to fix conflict
- Picked up dt-bindings patches from old, not-fully merged series.
- qcom,pm8921-keypad: droped the no-autorepeat property (Rob, Dmitry)
- Moved qcom,ssbi to /bus/ (Krzysztof)
Changes since v1:
- To ease reviewing break cleanups from the "split PMIC" patches
(Konrad).
Dmitry Baryshkov (36):
dt-bindings: input: qcom,pm8921-keypad: convert to YAML format
ARM: dts: qcom: apq8064: correct XOADC register address
ARM: dts: qcom: msm8960: introduce label for PMIC keypad
ARM: dts: qcom: msm8660-surf: use keypad label directly
ARM: dts: qcom: apq8064-nexus7: move sdcc1 node to proper place
ARM: dts: qcom: mdm9615-wp8548-mangoh-green: group include clauses
ARM: dts: qcom: strip prefix from PMIC files
ARM: dts: qcom: apq8064: fix PMIC node labels
ARM: dts: qcom: mdm9615: fix PMIC node labels
ARM: dts: qcom: msm8660: fix PMIC node labels
ARM: dts: qcom: msm8960: fix PMIC node labels
ARM: dts: qcom: apq8064: move PMIC interrupts to the board files
ARM: dts: qcom: mdm9615: move PMIC interrupts to the board files
ARM: dts: qcom: msm8660: move PMIC interrupts to the board files
ARM: dts: qcom: msm8960: move PMIC interrupts to the board files
ARM: dts: qcom: msm8960: split PMIC to separate dtsi files
ARM: dts: qcom: apq8064: split PMICs to separate dtsi files
ARM: dts: qcom: mdm9615: split PMIC to separate dtsi files
ARM: dts: qcom: msm8660: split PMIC to separate dtsi files
ARM: dts: qcom: pm8058: reorder nodes
ARM: dts: qcom: pm8921: reorder nodes
ARM: dts: qcom: pm8018: move reg property
ARM: dts: qcom: pm8921: move reg property
ARM: dts: qcom: pm8058: use defined IRQ flags
ARM: dts: qcom: pm8921: switch to interrupts-extended
ARM: dts: qcom: pm8018: switch to interrupts-extended
ARM: dts: qcom: pm8058: switch to interrupts-extended
ARM: dts: qcom: apq8064: move RPM regulators to board files
ARM: dts: qcom: mdm9615: move RPM regulators to board files
ARM: dts: qcom: msm8660: move RPM regulators to board files
ARM: dts: qcom: msm8960: drop useless rpm regulators node
ARM: dts: qcom: msm8974: move regulators to board files
ARM: dts: qcom: pm8921: Disable keypad by default
ARM: dts: qcom: apq8060-dragonboard: rename mpp ADC channels to
adc-channel
ARM: dts: qcom: ipq8064: drop qcom, prefix from SSBI node name
ARM: dts: qcom: mdm9615: drop qcom, prefix from SSBI node name
.../bindings/input/qcom,pm8921-keypad.yaml | 89 +++++++
.../bindings/input/qcom,pm8xxx-keypad.txt | 90 --------
arch/arm/boot/dts/qcom/pm8018.dtsi | 55 +++++
arch/arm/boot/dts/qcom/pm8058.dtsi | 159 +++++++++++++
.../qcom/{qcom-pm8226.dtsi => pm8226.dtsi} | 0
arch/arm/boot/dts/qcom/pm8821.dtsi | 22 ++
.../qcom/{qcom-pm8841.dtsi => pm8841.dtsi} | 0
arch/arm/boot/dts/qcom/pm8921.dtsi | 137 +++++++++++
.../qcom/{qcom-pm8941.dtsi => pm8941.dtsi} | 0
.../qcom/{qcom-pma8084.dtsi => pma8084.dtsi} | 0
.../dts/qcom/{qcom-pmx55.dtsi => pmx55.dtsi} | 0
.../dts/qcom/{qcom-pmx65.dtsi => pmx65.dtsi} | 0
.../dts/qcom/qcom-apq8026-asus-sparrow.dts | 2 +-
.../dts/qcom/qcom-apq8026-huawei-sturgeon.dts | 2 +-
.../boot/dts/qcom/qcom-apq8026-lg-lenok.dts | 2 +-
.../qcom-apq8026-samsung-matisse-wifi.dts | 2 +-
.../dts/qcom/qcom-apq8060-dragonboard.dts | 164 ++++++++-----
.../dts/qcom/qcom-apq8064-asus-nexus7-flo.dts | 70 +++---
.../boot/dts/qcom/qcom-apq8064-cm-qs600.dts | 35 ++-
.../boot/dts/qcom/qcom-apq8064-ifc6410.dts | 42 ++--
.../qcom-apq8064-sony-xperia-lagan-yuga.dts | 111 +++++----
arch/arm/boot/dts/qcom/qcom-apq8064.dtsi | 201 +---------------
.../dts/qcom/qcom-apq8074-dragonboard.dts | 31 ++-
.../boot/dts/qcom/qcom-apq8084-ifc6540.dts | 2 +-
arch/arm/boot/dts/qcom/qcom-apq8084-mtp.dts | 2 +-
arch/arm/boot/dts/qcom/qcom-ipq8064.dtsi | 2 +-
.../qcom/qcom-mdm9615-wp8548-mangoh-green.dts | 4 +-
.../boot/dts/qcom/qcom-mdm9615-wp8548.dtsi | 143 +++++++++++-
arch/arm/boot/dts/qcom/qcom-mdm9615.dtsi | 183 +--------------
arch/arm/boot/dts/qcom/qcom-msm8660-surf.dts | 61 +++--
arch/arm/boot/dts/qcom/qcom-msm8660.dtsi | 217 +-----------------
arch/arm/boot/dts/qcom/qcom-msm8960-cdp.dts | 27 ++-
.../qcom/qcom-msm8960-samsung-expressatt.dts | 7 +-
arch/arm/boot/dts/qcom/qcom-msm8960.dtsi | 45 +---
.../qcom-msm8974-lge-nexus5-hammerhead.dts | 31 ++-
.../qcom/qcom-msm8974-sony-xperia-rhine.dtsi | 31 ++-
arch/arm/boot/dts/qcom/qcom-msm8974.dtsi | 27 ---
.../qcom/qcom-msm8974pro-fairphone-fp2.dts | 31 ++-
.../qcom/qcom-msm8974pro-oneplus-bacon.dts | 31 ++-
.../dts/qcom/qcom-msm8974pro-samsung-klte.dts | 12 +-
...-msm8974pro-sony-xperia-shinano-castor.dts | 31 ++-
arch/arm/boot/dts/qcom/qcom-sdx55-mtp.dts | 2 +-
arch/arm/boot/dts/qcom/qcom-sdx55-t55.dts | 2 +-
.../dts/qcom/qcom-sdx55-telit-fn980-tlb.dts | 2 +-
arch/arm/boot/dts/qcom/qcom-sdx65-mtp.dts | 2 +-
45 files changed, 1137 insertions(+), 972 deletions(-)
create mode 100644 Documentation/devicetree/bindings/input/qcom,pm8921-keypad.yaml
delete mode 100644 Documentation/devicetree/bindings/input/qcom,pm8xxx-keypad.txt
create mode 100644 arch/arm/boot/dts/qcom/pm8018.dtsi
create mode 100644 arch/arm/boot/dts/qcom/pm8058.dtsi
rename arch/arm/boot/dts/qcom/{qcom-pm8226.dtsi => pm8226.dtsi} (100%)
create mode 100644 arch/arm/boot/dts/qcom/pm8821.dtsi
rename arch/arm/boot/dts/qcom/{qcom-pm8841.dtsi => pm8841.dtsi} (100%)
create mode 100644 arch/arm/boot/dts/qcom/pm8921.dtsi
rename arch/arm/boot/dts/qcom/{qcom-pm8941.dtsi => pm8941.dtsi} (100%)
rename arch/arm/boot/dts/qcom/{qcom-pma8084.dtsi => pma8084.dtsi} (100%)
rename arch/arm/boot/dts/qcom/{qcom-pmx55.dtsi => pmx55.dtsi} (100%)
rename arch/arm/boot/dts/qcom/{qcom-pmx65.dtsi => pmx65.dtsi} (100%)
--
2.39.2
^ permalink raw reply
* Re: [regression] Resume broken on T14s Gen1 (AMD) due to "Input: psmouse - add delay when deactivating for SMBus mode"
From: Thorsten Leemhuis @ 2023-09-28 9:08 UTC (permalink / raw)
To: Jeffery Miller
Cc: Andrew Duggan, Dmitry Torokhov, Benjamin Tissoires, linux-input,
linux-kernel, Linux kernel regressions list
In-Reply-To: <cf87d6a5-7ff3-4add-8c48-fd3447b32697@leemhuis.info>
On 27.09.23 19:23, Thorsten Leemhuis wrote:
> On 27.09.23 17:55, Jeffery Miller wrote:
>> On Wed, Sep 27, 2023 at 10:43 AM Jeffery Miller
>> <jefferymiller@google.com> wrote:
>>> On Wed, Sep 27, 2023 at 3:54 AM Thorsten Leemhuis <linux@leemhuis.info> wrote:
>>>>
>>>> My dmesg from a kernel with the revert:
>>>> https://www.leemhuis.info/files/misc/dmesg
>
> Thx for looking into this!
>
>>> In this dmesg output it shows that this is an elantech smbus device:
>>> ```
>>> [ 4.260415] psmouse serio1: elantech: assuming hardware version 4 (with firmware version 0x7f3001)
>>> [ 4.279297] psmouse serio1: elantech: Synaptics capabilities query result 0x90, 0x18, 0x0f.
>>> [ 4.292788] psmouse serio1: elantech: Elan sample query result 00, 80, c9
>>> [ 4.319184] psmouse serio1: elantech: Elan ic body: 0x10, current fw version: 0x3
>>> ...
>>> [ 4.346951] psmouse serio1: elantech: Trying to set up SMBus access
>>> [ 4.346986] psmouse serio1: elantech: SMbus companion is not ready yet
>>> [ 4.369993] input: ETPS/2 Elantech TrackPoint as /devices/platform/i8042/serio1/input/input7
>>> [ 4.376200] systemd[1]: bpf-lsm: LSM BPF program attached
>>> [ 4.385192] input: ETPS/2 Elantech Touchpad as /devices/platform/i8042/serio1/input/input5
>>> ```
>>> The change in 92e24e0e57f72e shouldn't affect the elantouch device as elantech_setup_smbus
>>> initializes `psmouse_smbus_init` with need_deactivate = false.
>
> Hmmm. Wondering if I should warm up the compiler again to recheck my
> result one more time[1].
Just did that. Ran "make clean" and compiled mainline as of now
(633b47cb009d) and the machine does never resume from s2idle; then I
reverted 92e24e0e57f7 and compiled again (for completeness: without
running "make clean" beforehand) and with that kernel s2idle resume
works perfectly fine.
Wondering if I or the compiler is doing something stupid here -- or if
we missed some small but important detail somewhere.
Ciao, Thorsten
>>> Did you store dmesg logs from boot without the applied patch?
>> I intended to ask if you have logs from a boot without 92e24e0e57f72e reverted.
>
> https://www.leemhuis.info/files/misc/dmesg-6.6-rc3-vanilla
>
>>> If the delay was being applied the timestamps should show the 30ms delay between
>>> `psmouse serio1: elantech: Trying to set up SMBus access`
>>> and
>>> `psmouse serio1: elantech: SMbus companion is not ready yet`
>
> Unless I missed something there is not difference. :-/
>
> Ciao, Thorsten
>
> [1] FWIW, this is my bisect log
>
> """
>> git bisect start
>> # status: waiting for both good and bad commits
>> # bad: [6465e260f48790807eef06b583b38ca9789b6072] Linux 6.6-rc3
>> git bisect bad 6465e260f48790807eef06b583b38ca9789b6072
>> # status: waiting for good commit(s), bad commit known
>> # good: [2dde18cd1d8fac735875f2e4987f11817cc0bc2c] Linux 6.5
>> git bisect good 2dde18cd1d8fac735875f2e4987f11817cc0bc2c
>> # good: [4fb0dacb78c6a041bbd38ddd998df806af5c2c69] Merge tag 'sound-6.6-rc1' of git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound
>> git bisect good 4fb0dacb78c6a041bbd38ddd998df806af5c2c69
>> # good: [307d59039fb26212a84a9aa6a134a7d2bdea34ca] Merge tag 'media/v6.6-1' of git://git.kernel.org/pub/scm/linux/kernel/git/mchehab/linux-media
>> git bisect good 307d59039fb26212a84a9aa6a134a7d2bdea34ca
>> # bad: [4a0fc73da97efd23a383ca839e6fe86410268f6b] Merge tag 's390-6.6-2' of git://git.kernel.org/pub/scm/linux/kernel/git/s390/linux
>> git bisect bad 4a0fc73da97efd23a383ca839e6fe86410268f6b
>> # good: [e4f1b8202fb59c56a3de7642d50326923670513f] Merge tag 'for_linus' of git://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost
>> git bisect good e4f1b8202fb59c56a3de7642d50326923670513f
>> # good: [5eea5820c7340d39e56e169e1b87199391105f6b] Merge tag 'mm-stable-2023-09-04-14-00' of git://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm
>> git bisect good 5eea5820c7340d39e56e169e1b87199391105f6b
>> # good: [65d6e954e37872fd9afb5ef3fc0481bb3c2f20f4] Merge tag 'gfs2-v6.5-rc5-fixes' of git://git.kernel.org/pub/scm/linux/kernel/git/gfs2/linux-gfs2
>> git bisect good 65d6e954e37872fd9afb5ef3fc0481bb3c2f20f4
>> # bad: [744a759492b5c57ff24a6e8aabe47b17ad8ee964] Merge tag 'input-for-v6.6-rc0' of git://git.kernel.org/pub/scm/linux/kernel/git/dtor/input
>> git bisect bad 744a759492b5c57ff24a6e8aabe47b17ad8ee964
>> # good: [dbce1a7d5dce7318d8465b1e0d052ef1d2202237] Input: Explicitly include correct DT includes
>> git bisect good dbce1a7d5dce7318d8465b1e0d052ef1d2202237
>> # good: [29057cc5bddc785ea0a11534d7ad2546fa0872d3] Merge tag 'linux-watchdog-6.6-rc1' of git://www.linux-watchdog.org/linux-watchdog
>> git bisect good 29057cc5bddc785ea0a11534d7ad2546fa0872d3
>> # bad: [3e4bb047b23375a34dbf5885709ac3729d9cfb22] Input: qt2160 - convert to use devm_* api
>> git bisect bad 3e4bb047b23375a34dbf5885709ac3729d9cfb22
>> # good: [e175eae16c1bf92062f1f431a95f476a61a77c48] Input: mcs-touchkey - convert to use devm_* api
>> git bisect good e175eae16c1bf92062f1f431a95f476a61a77c48
>> # bad: [92e24e0e57f72e06c2df87116557331fd2d4dda2] Input: psmouse - add delay when deactivating for SMBus mode
>> git bisect bad 92e24e0e57f72e06c2df87116557331fd2d4dda2
>> # good: [8362bf82fb5441613aac7c6c9dbb6b83def6ad3b] Input: mcs-touchkey - fix uninitialized use of error in mcs_touchkey_probe()
>> git bisect good 8362bf82fb5441613aac7c6c9dbb6b83def6ad3b
>> # first bad commit: [92e24e0e57f72e06c2df87116557331fd2d4dda2] Input: psmouse - add delay when deactivating for SMBus mode
> """
>
>
^ permalink raw reply
* Re: [PATCH 2/2] input: Imagis: add support for the IST3032C touchscreen
From: Krzysztof Kozlowski @ 2023-09-28 5:25 UTC (permalink / raw)
To: Karel Balej, Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Markuss Broks, linux-input, devicetree,
linux-kernel, Duje Mihanović, ~postmarketos/upstreaming
In-Reply-To: <20230926173531.18715-3-balejk@matfyz.cz>
On 26/09/2023 19:35, Karel Balej wrote:
> The downstream driver sets the regulator voltage to 3.1 V. Without this,
> the touchscreen generates random touches even after it is no longer
> being touched. It is unknown whether the same problem appears with other
> chips of the IST30**C series.
>
> Co-developed-by: Duje Mihanović <duje.mihanovic@skole.hr>
> Signed-off-by: Duje Mihanović <duje.mihanovic@skole.hr>
> Signed-off-by: Karel Balej <balejk@matfyz.cz>
> ---
> .../bindings/input/touchscreen/imagis,ist30xxc.yaml | 1 +
Bindings are always separate patches. Always.
Please run scripts/checkpatch.pl and fix reported warnings. Some
warnings can be ignored, but the code here looks like it needs a fix.
Feel free to get in touch if the warning is not clear.
Please use subject prefixes matching the subsystem. You can get them for
example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
your patch is touching.
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH V2 1/2] dt-bindings: input: Introduce Himax HID-over-SPI device
From: yang tylor @ 2023-09-28 2:12 UTC (permalink / raw)
To: Conor Dooley
Cc: Conor Dooley, dmitry.torokhov, robh+dt, krzysztof.kozlowski+dt,
conor+dt, jikos, benjamin.tissoires, linux-input, devicetree,
linux-kernel, poyuan_chang, hbarnor, jingyliang@chromium.org,
wuxy23, luolm1, hung poyu
In-Reply-To: <20230926-reverence-unlit-d0027225cc43@spud>
On Tue, Sep 26, 2023 at 8:53 PM Conor Dooley <conor@kernel.org> wrote:
>
> On Tue, Sep 26, 2023 at 05:52:39PM +0800, yang tylor wrote:
> > On Tue, Sep 26, 2023 at 5:02 PM Conor Dooley <conor@kernel.org> wrote:
> > >
> > > On Mon, Sep 25, 2023 at 06:16:29PM +0800, yang tylor wrote:
> > > > On Mon, Sep 25, 2023 at 4:41 PM Conor Dooley <conor.dooley@microchip.com> wrote:
> > > > >
> > > > > On Mon, Sep 25, 2023 at 09:44:21AM +0800, yang tylor wrote:
> > > > > > On Fri, Sep 22, 2023 at 11:31 PM Conor Dooley <conor@kernel.org> wrote:
> > > > > > >
> > > > > > > On Fri, Sep 22, 2023 at 05:43:54PM +0800, yang tylor wrote:
> > > > > > > > On Fri, Sep 22, 2023 at 5:22 PM Conor Dooley <conor@kernel.org> wrote:
> > > > > > > > >
> > > > > > > > > On Fri, Sep 22, 2023 at 03:56:25PM +0800, yang tylor wrote:
> > > > > > > > > > On Tue, Sep 19, 2023 at 7:09 PM Conor Dooley <conor@kernel.org> wrote:
> > > > > > > > > > > On Tue, Sep 19, 2023 at 05:31:29PM +0800, yang tylor wrote:
> > > > > > > > >
> > > > > > > > > > > > The behavior of "himax,boot_time_fw_upgrade" seems not stable and
> > > > > > > > > > > > should be removed. "himax,fw_in_flash", I use the kernel config for
> > > > > > > > > > > > user to select.
> > > > > > > > > > >
> > > > > > > > > > > That seems like a bad idea, we want to be able to build one kernel that
> > > > > > > > > > > works for all hardware at the same time.
> > > > > > > > > > >
> > > > > > > > > > I see, so I should take that back?
> > > > > > > > > > I'll explain more about it.
> > > > > > > > >
> > > > > > > > > Are there particular ICs where the firmware would always be in flash and
> > > > > > > > > others where it would never be? Or is this a choice made by the board or
> > > > > > > > > system designer?
> > > > > > > > >
> > > > > > > > Most cases it's about the system designer's decision. But some ICs may be forced
> > > > > > > > to use flash because of its architecture(multiple IC inside, need to
> > > > > > > > load firmware to
> > > > > > > > multiple IC's sram by master IC). But if there is no limitation on
> > > > > > > > this part, most system
> > > > > > > > designers will prefer flashless.
> > > > > > >
> > > > > > > Forgive me if I am not understanding correctly, there are some ICs that
> > > > > > > will need to load the firmware from flash and there are some where it
> > > > > > > will be a decision made by the designer of the board. Is the flash part
> > > > > > > of the IC or is it an external flash chip?
> > > > > > >
> > > > > >
> > > > > > Both are possible, it depends on the IC type. For TDDI, the IC is long
> > > > > > and thin, placed on panel PCB, flash will be located at the external
> > > > > > flash chip. For the OLED TP, IC is usually placed at FPC and its flash
> > > > > > is embedded, thus the IC size is large compared to TDDI. But from the
> > > > > > driver's perspective either external flash or embedded flash, the IC
> > > > > > itself will load firmware from flash automatically when reset pin is
> > > > > > released. Only if firmware is loading from the host storage system,
> > > > > > the driver needs to operate the IC in detail.
> > > > >
> > > > >
> > > > > Since there are ICs that can use the external flash or have it loaded
> > > > > from the host, it sounds like you do need a property to differentiate
> > > > > between those cases.
> > > > Yep.
> > > >
> > > > > Is it sufficient to just set the firmware-name property for these cases?
> > > > > If the property exists, then you know you need to load firmware & what
> > > > > its name is. If it doesn't, then the firmware either isn't needed or
> > > > > will be automatically loaded from the external flash.
> > >
> > > > We have a default prefix firmware name(like himax_xxxx.bin) in the driver code.
> > >
> > > How do you intend generating the name of the firmware file? I assume the
> > > same firmware doesn't work on every IC, so you'll need to pick a
> > > different one depending on the compatible?
> > >
> > If considering a firmware library line-up for all the incoming panels
> > of this driver.
> > We would use PID as part of the file name. Because all the support panels would
> > have a unique PID associated. Which will make the firmware name like
> > himax_xxx_{$PID}.bin. The problem is, we need to know PID before firmware load
> > at no flash condition. Thus PID information is required in dts when
> > no-flash-flag
> > is specified.
>
> Firstly, where does the "xxx" come from?
> And you're making it sound more like having firmware-name is suitable
> for this use case, given you need to determine the name of the file to
> use based on something that is hardware specific but is not
> dynamically detectable.
Current driver patch uses a prefix name "himax_i2chid" which comes
from the previous project
and seems not suitable for this condition, so I use "xxx" and plan to
replace it in the next version.
For finding firmware, I think both solutions are reasonable.
- provide firmware name directly: implies no-flash and use user
specified firmware, no PID info.
- provide no-flash-flag and PID info: loading firmware from organized
names with PID info.
I prefer the 2nd solution, but it needs more properties in dts. 1st
has less properties and more
intuitive.
I don't know which one is more acceptable by the community, as you
know I'm a newbie here.
Thanks,
Tylor
^ permalink raw reply
* Re: [RFC PATCH] of: device: Support 2nd sources of probeable but undiscoverable devices
From: Doug Anderson @ 2023-09-27 23:50 UTC (permalink / raw)
To: Jeff LaBundy
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, devicetree,
Benjamin Tissoires, Chen-Yu Tsai, linux-input, Jiri Kosina,
Hsin-Yi Wang, linux-gpio, linus.walleij, Dmitry Torokhov,
Johan Hovold, andriy.shevchenko, broonie, frowand.list, gregkh,
hdegoede, james.clark, james, keescook, linux-kernel, rafael,
tglx
In-Reply-To: <ZROVSAoKF9bimnSP@nixie71>
Hi,
On Tue, Sep 26, 2023 at 7:37 PM Jeff LaBundy <jeff@labundy.com> wrote:
>
> Hi Doug,
>
> On Fri, Sep 22, 2023 at 05:11:10PM -0700, Doug Anderson wrote:
> > Hi,
> >
> > On Fri, Sep 22, 2023 at 12:08 PM Rob Herring <robh+dt@kernel.org> wrote:
> > >
> > > > > This seems like overkill to me. Do we really need groups and a mutex
> > > > > for each group? Worst case is what? 2-3 groups of 2-3 devices?
> > > > > Instead, what about extending "status" with another value
> > > > > ("fail-needs-probe"? (fail-xxx is a documented value)). Currently, the
> > > > > kernel would just ignore nodes with that status. Then we can process
> > > > > those nodes separately 1-by-1.
> > > >
> > > > My worry here is that this has the potential to impact boot speed in a
> > > > non-trivial way. While trackpads and touchscreens _are_ probable,
> > > > their probe routines are often quite slow. This is even mentioned in
> > > > Dmitry's initial patches adding async probe to the kernel. See commit
> > > > 765230b5f084 ("driver-core: add asynchronous probing support for
> > > > drivers") where he specifically brings up input devices as examples.
>
> Ideally, all but one driver in a group should bail out of probe quickly if
> the device is not populated. If not, I would consider that to be a bug or at
> least room for improvement in that driver.
>
> The reason input devices can take a while to probe is because they may be
> loading FW over I2C or performing some sort of calibration procedure; only
> one driver in the group should get that far.
Hmm, that's not my experience. Specifically I've seen i2c-hid devices
whose datasheets say that you're not allowed to talk i2c to them at
all for hundreds of milliseconds after you power them on. See, for
instance, "i2c-hid-of-goodix.c" which has a "post_gpio_reset_delay_ms"
of 180 ms and "i2c-hid-of-elan.c" which has one of 300 ms.
As I understand it these touchscreens have firmware on them and that
firmware can take a while to boot. Until the firmware boots they won't
respond over i2c. This is simply not something that Linux can do
anything about.
About the best you could do would be to add a board-specific driver
that understood that it could power up the rails, wait the maximum
amount of time that all possible touchscreens might need, and then
look for i2c ACKs. I'm still hoping to hear from Rob about how I would
get a board-specific driver to load on a DT system so I can
investigate / prototype this.
> > > We could add information on the class of device. touchscreen and
> > > touchpad aliases or something.
> >
> > Ah, I see. So something like "fail-needs-probe-<class>". The
> > touchscreens could have "fail-needs-probe-touchscreen" and the
> > trackpads could have "fail-needs-probe-trackpad" ? That could work. In
> > theory that could fall back to the same solution of grabbing a mutex
> > based on the group ID...
> >
> > Also: if having the mutex in the "struct device" is seen as a bad
> > idea, it would also be easy to remove. __driver_probe_device() could
> > just make a call like "of_device_probe_start()" at the beginning that
> > locks the mutex and then "of_device_probe_end()" that unlocks it. Both
> > of those calls could easily lookup the mutex in a list, which would
> > get rid of the need to store it in the "struct device".
> >
> >
> > > > That would lead me to suggest this:
> > > >
> > > > &i2c_bus {
> > > > trackpad-prober {
> > > > compatible = "mt8173-elm-hana-trackpad-prober";
> > > >
> > > > tp1: trackpad@10 {
> > > > compatible = "hid-over-i2c";
> > > > reg = <0x10>;
> > > > ...
> > > > post-power-on-delay-ms = <200>;
> > > > };
> > > > tp2: trackpad@20 {
> > > > compatible = "hid-over-i2c";
> > > > reg = <0x20>;
> > > > ...
> > > > post-power-on-delay-ms = <200>;
> > > > };
> > > > };
> > > > };
> > > >
> > > > ...but I suspect that would be insta-NAKed because it's creating a
> > > > completely virtual device ("mt8173-elm-hana-trackpad-prober") in the
> > > > device tree. I don't know if there's something that's functionally
> > > > similar that would be OK?
>
> This solution seems a bit confusing to me, and would require more edits
> to the dts each time a second source is added. It also means one would
> have to write a small platform driver for each group of devices, correct?
No matter what we need to add something to the dts each time a second
source is added, right?
While it's true that we'd end up with some extra drivers, if we do it
correctly we don't necessarily need a driver for each group of devices
nor even a driver per board. If several boards have very similar
probing requirements then, even if they have unique "compatible"
strings they could still end up using the same Linux driver.
I've actually been talking offline with folks on ChromeOS more about
this problem as well. Chen-Yu actually pointed at a patch series (that
never landed, I guess) that has some similar ideas [1]. I guess in
that case Hans was actually constructing device tree properties
manually in the driver. I was thinking more of having all of the
options listed in the device tree and then doing something that only
causes some of them to probe.
If Rob was OK with it, I guess I could have some sort of top-level
"hwmanager" node like Hans did and then have phandle links to all the
hardware that are managed by it. Then I could just change those to
"okay"?
Ideally, though, this could somehow use device tree "overlays" I
guess. That seems like almost a perfect fit. I guess the issue here,
though, is that I'd want the overlays bundled together with the
original DT and then the board-specific "hardware prober" driver to
actually apply the overlays after probing. Does that seem sensible?
[1] https://lore.kernel.org/linux-arm-kernel/20160901190820.21987-1-hdegoede@redhat.com/
^ permalink raw reply
* Re: [regression] Resume broken on T14s Gen1 (AMD) due to "Input: psmouse - add delay when deactivating for SMBus mode"
From: Thorsten Leemhuis @ 2023-09-27 17:23 UTC (permalink / raw)
To: Jeffery Miller
Cc: Andrew Duggan, Dmitry Torokhov, Benjamin Tissoires, linux-input,
linux-kernel, Linux kernel regressions list
In-Reply-To: <CAAzPG9NWp8yPU52o7d2-jLjxjLodFOiE_AjoxmCAZ=MXtV__Aw@mail.gmail.com>
On 27.09.23 17:55, Jeffery Miller wrote:
> On Wed, Sep 27, 2023 at 10:43 AM Jeffery Miller
> <jefferymiller@google.com> wrote:
>> On Wed, Sep 27, 2023 at 3:54 AM Thorsten Leemhuis <linux@leemhuis.info> wrote:
>>>
>>> My dmesg from a kernel with the revert:
>>> https://www.leemhuis.info/files/misc/dmesg
Thx for looking into this!
>> In this dmesg output it shows that this is an elantech smbus device:
>> ```
>> [ 4.260415] psmouse serio1: elantech: assuming hardware version 4 (with firmware version 0x7f3001)
>> [ 4.279297] psmouse serio1: elantech: Synaptics capabilities query result 0x90, 0x18, 0x0f.
>> [ 4.292788] psmouse serio1: elantech: Elan sample query result 00, 80, c9
>> [ 4.319184] psmouse serio1: elantech: Elan ic body: 0x10, current fw version: 0x3
>> ...
>> [ 4.346951] psmouse serio1: elantech: Trying to set up SMBus access
>> [ 4.346986] psmouse serio1: elantech: SMbus companion is not ready yet
>> [ 4.369993] input: ETPS/2 Elantech TrackPoint as /devices/platform/i8042/serio1/input/input7
>> [ 4.376200] systemd[1]: bpf-lsm: LSM BPF program attached
>> [ 4.385192] input: ETPS/2 Elantech Touchpad as /devices/platform/i8042/serio1/input/input5
>> ```
>> The change in 92e24e0e57f72e shouldn't affect the elantouch device as elantech_setup_smbus
>> initializes `psmouse_smbus_init` with need_deactivate = false.
Hmmm. Wondering if I should warm up the compiler again to recheck my
result one more time[1].
>> Did you store dmesg logs from boot without the applied patch?
>
> I intended to ask if you have logs from a boot without 92e24e0e57f72e reverted.
https://www.leemhuis.info/files/misc/dmesg-6.6-rc3-vanilla
>> If the delay was being applied the timestamps should show the 30ms delay between
>> `psmouse serio1: elantech: Trying to set up SMBus access`
>> and
>> `psmouse serio1: elantech: SMbus companion is not ready yet`
Unless I missed something there is not difference. :-/
Ciao, Thorsten
[1] FWIW, this is my bisect log
"""
> git bisect start
> # status: waiting for both good and bad commits
> # bad: [6465e260f48790807eef06b583b38ca9789b6072] Linux 6.6-rc3
> git bisect bad 6465e260f48790807eef06b583b38ca9789b6072
> # status: waiting for good commit(s), bad commit known
> # good: [2dde18cd1d8fac735875f2e4987f11817cc0bc2c] Linux 6.5
> git bisect good 2dde18cd1d8fac735875f2e4987f11817cc0bc2c
> # good: [4fb0dacb78c6a041bbd38ddd998df806af5c2c69] Merge tag 'sound-6.6-rc1' of git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound
> git bisect good 4fb0dacb78c6a041bbd38ddd998df806af5c2c69
> # good: [307d59039fb26212a84a9aa6a134a7d2bdea34ca] Merge tag 'media/v6.6-1' of git://git.kernel.org/pub/scm/linux/kernel/git/mchehab/linux-media
> git bisect good 307d59039fb26212a84a9aa6a134a7d2bdea34ca
> # bad: [4a0fc73da97efd23a383ca839e6fe86410268f6b] Merge tag 's390-6.6-2' of git://git.kernel.org/pub/scm/linux/kernel/git/s390/linux
> git bisect bad 4a0fc73da97efd23a383ca839e6fe86410268f6b
> # good: [e4f1b8202fb59c56a3de7642d50326923670513f] Merge tag 'for_linus' of git://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost
> git bisect good e4f1b8202fb59c56a3de7642d50326923670513f
> # good: [5eea5820c7340d39e56e169e1b87199391105f6b] Merge tag 'mm-stable-2023-09-04-14-00' of git://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm
> git bisect good 5eea5820c7340d39e56e169e1b87199391105f6b
> # good: [65d6e954e37872fd9afb5ef3fc0481bb3c2f20f4] Merge tag 'gfs2-v6.5-rc5-fixes' of git://git.kernel.org/pub/scm/linux/kernel/git/gfs2/linux-gfs2
> git bisect good 65d6e954e37872fd9afb5ef3fc0481bb3c2f20f4
> # bad: [744a759492b5c57ff24a6e8aabe47b17ad8ee964] Merge tag 'input-for-v6.6-rc0' of git://git.kernel.org/pub/scm/linux/kernel/git/dtor/input
> git bisect bad 744a759492b5c57ff24a6e8aabe47b17ad8ee964
> # good: [dbce1a7d5dce7318d8465b1e0d052ef1d2202237] Input: Explicitly include correct DT includes
> git bisect good dbce1a7d5dce7318d8465b1e0d052ef1d2202237
> # good: [29057cc5bddc785ea0a11534d7ad2546fa0872d3] Merge tag 'linux-watchdog-6.6-rc1' of git://www.linux-watchdog.org/linux-watchdog
> git bisect good 29057cc5bddc785ea0a11534d7ad2546fa0872d3
> # bad: [3e4bb047b23375a34dbf5885709ac3729d9cfb22] Input: qt2160 - convert to use devm_* api
> git bisect bad 3e4bb047b23375a34dbf5885709ac3729d9cfb22
> # good: [e175eae16c1bf92062f1f431a95f476a61a77c48] Input: mcs-touchkey - convert to use devm_* api
> git bisect good e175eae16c1bf92062f1f431a95f476a61a77c48
> # bad: [92e24e0e57f72e06c2df87116557331fd2d4dda2] Input: psmouse - add delay when deactivating for SMBus mode
> git bisect bad 92e24e0e57f72e06c2df87116557331fd2d4dda2
> # good: [8362bf82fb5441613aac7c6c9dbb6b83def6ad3b] Input: mcs-touchkey - fix uninitialized use of error in mcs_touchkey_probe()
> git bisect good 8362bf82fb5441613aac7c6c9dbb6b83def6ad3b
> # first bad commit: [92e24e0e57f72e06c2df87116557331fd2d4dda2] Input: psmouse - add delay when deactivating for SMBus mode
"""
^ permalink raw reply
* Re: [regression] Resume broken on T14s Gen1 (AMD) due to "Input: psmouse - add delay when deactivating for SMBus mode"
From: Jeffery Miller @ 2023-09-27 15:55 UTC (permalink / raw)
To: Thorsten Leemhuis
Cc: Andrew Duggan, Dmitry Torokhov, Benjamin Tissoires, linux-input,
linux-kernel, Linux kernel regressions list
In-Reply-To: <CAAzPG9NkoaUz_JRtZt_JomsYj-8ZPn4QH0w0eeR-oxd55-18Qg@mail.gmail.com>
Hi Thorsten,
On Wed, Sep 27, 2023 at 10:43 AM Jeffery Miller
<jefferymiller@google.com> wrote:
>
>
> On Wed, Sep 27, 2023 at 3:54 AM Thorsten Leemhuis <linux@leemhuis.info> wrote:
>>
>> My dmesg from a kernel with the revert:
>> https://www.leemhuis.info/files/misc/dmesg
>>
> In this dmesg output it shows that this is an elantech smbus device:
> ```
> [ 4.260415] psmouse serio1: elantech: assuming hardware version 4 (with firmware version 0x7f3001)
> [ 4.279297] psmouse serio1: elantech: Synaptics capabilities query result 0x90, 0x18, 0x0f.
> [ 4.292788] psmouse serio1: elantech: Elan sample query result 00, 80, c9
> [ 4.319184] psmouse serio1: elantech: Elan ic body: 0x10, current fw version: 0x3
> ...
> [ 4.346951] psmouse serio1: elantech: Trying to set up SMBus access
> [ 4.346986] psmouse serio1: elantech: SMbus companion is not ready yet
> [ 4.369993] input: ETPS/2 Elantech TrackPoint as /devices/platform/i8042/serio1/input/input7
> [ 4.376200] systemd[1]: bpf-lsm: LSM BPF program attached
> [ 4.385192] input: ETPS/2 Elantech Touchpad as /devices/platform/i8042/serio1/input/input5
> ```
> The change in 92e24e0e57f72e shouldn't affect the elantouch device as elantech_setup_smbus
> initializes `psmouse_smbus_init` with need_deactivate = false.
>
> Did you store dmesg logs from boot without the applied patch?
I intended to ask if you have logs from a boot without 92e24e0e57f72e reverted.
> If the delay was being applied the timestamps should show the 30ms delay between
> `psmouse serio1: elantech: Trying to set up SMBus access`
> and
> `psmouse serio1: elantech: SMbus companion is not ready yet`
>
Thank You,
Jeff
^ permalink raw reply
* Re: [PATCH 1/2] input: generalize the Imagis touchscreen driver
From: Markuss Broks @ 2023-09-27 15:36 UTC (permalink / raw)
To: Karel Balej
Cc: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
linux-input, devicetree, linux-kernel, Duje Mihanović,
~postmarketos/upstreaming, Jeff LaBundy, linmengbo0689
In-Reply-To: <7b9864bf-2aa6-4510-ad98-276fbfaadc30@gmail.com>
Hi Karel,
On 9/26/23 20:35, Karel Balej wrote:
> This driver should work with other Imagis ICs of the IST30**C series.
> Make that more apparent.
To be fair, this driver should work with all Imagis3 chips, which could
be a better name for it. However, I agree with Jeff here: the driver
doesn't necessarily need renaming.
>
> Co-developed-by: Duje Mihanović<duje.mihanovic@skole.hr>
> Signed-off-by: Duje Mihanović<duje.mihanovic@skole.hr>
> Signed-off-by: Karel Balej<balejk@matfyz.cz>
> ---
> ...gis,ist3038c.yaml => imagis,ist30xxc.yaml} | 2 +-
> MAINTAINERS | 2 +-
> drivers/input/touchscreen/Kconfig | 4 +-
> drivers/input/touchscreen/imagis.c | 86 +++++++++++--------
> 4 files changed, 52 insertions(+), 42 deletions(-)
> rename Documentation/devicetree/bindings/input/touchscreen/{imagis,ist3038c.yaml => imagis,ist30xxc.yaml} (99%)
>
> diff --git a/Documentation/devicetree/bindings/input/touchscreen/imagis,ist3038c.yaml b/Documentation/devicetree/bindings/input/touchscreen/imagis,ist30xxc.yaml
> similarity index 99%
> rename from Documentation/devicetree/bindings/input/touchscreen/imagis,ist3038c.yaml
> rename to Documentation/devicetree/bindings/input/touchscreen/imagis,ist30xxc.yaml
> index 0d6b033fd5fb..09bf3a6acc5e 100644
> --- a/Documentation/devicetree/bindings/input/touchscreen/imagis,ist3038c.yaml
> +++ b/Documentation/devicetree/bindings/input/touchscreen/imagis,ist30xxc.yaml
> @@ -1,7 +1,7 @@
> # SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> %YAML 1.2
> ---
> -$id:http://devicetree.org/schemas/input/touchscreen/imagis,ist3038c.yaml#
> +$id:http://devicetree.org/schemas/input/touchscreen/imagis,ist30xxc.yaml#
> $schema:http://devicetree.org/meta-schemas/core.yaml#
>
> title: Imagis IST30XXC family touchscreen controller
> diff --git a/MAINTAINERS b/MAINTAINERS
> index b19995690904..b23e76418d94 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -10209,7 +10209,7 @@ F: drivers/usb/atm/ueagle-atm.c
> IMAGIS TOUCHSCREEN DRIVER
> M: Markuss Broks<markuss.broks@gmail.com>
> S: Maintained
> -F: Documentation/devicetree/bindings/input/touchscreen/imagis,ist3038c.yaml
> +F: Documentation/devicetree/bindings/input/touchscreen/imagis,ist30xxc.yaml
> F: drivers/input/touchscreen/imagis.c
>
> IMGTEC ASCII LCD DRIVER
> diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
> index e3e2324547b9..45503aa2653e 100644
> --- a/drivers/input/touchscreen/Kconfig
> +++ b/drivers/input/touchscreen/Kconfig
> @@ -665,10 +665,10 @@ config TOUCHSCREEN_NOVATEK_NVT_TS
> module will be called novatek-nvt-ts.
>
> config TOUCHSCREEN_IMAGIS
> - tristate "Imagis touchscreen support"
> + tristate "Imagis IST30XXC touchscreen support"
> depends on I2C
> help
> - Say Y here if you have an Imagis IST30xxC touchscreen.
> + Say Y here if you have an Imagis IST30XXC touchscreen.
> If unsure, say N.
>
> To compile this driver as a module, choose M here: the
> diff --git a/drivers/input/touchscreen/imagis.c b/drivers/input/touchscreen/imagis.c
> index 07111ca24455..4456f1b4d527 100644
> --- a/drivers/input/touchscreen/imagis.c
> +++ b/drivers/input/touchscreen/imagis.c
> @@ -11,25 +11,26 @@
> #include <linux/property.h>
> #include <linux/regulator/consumer.h>
>
> -#define IST3038C_HIB_ACCESS (0x800B << 16)
> -#define IST3038C_DIRECT_ACCESS BIT(31)
> -#define IST3038C_REG_CHIPID 0x40001000
> -#define IST3038C_REG_HIB_BASE 0x30000100
> -#define IST3038C_REG_TOUCH_STATUS (IST3038C_REG_HIB_BASE | IST3038C_HIB_ACCESS)
> -#define IST3038C_REG_TOUCH_COORD (IST3038C_REG_HIB_BASE | IST3038C_HIB_ACCESS | 0x8)
> -#define IST3038C_REG_INTR_MESSAGE (IST3038C_REG_HIB_BASE | IST3038C_HIB_ACCESS | 0x4)
> +#define IST30XXC_HIB_ACCESS (0x800B << 16)
> +#define IST30XXC_DIRECT_ACCESS BIT(31)
> +#define IST30XXC_REG_CHIPID 0x40001000
> +#define IST30XXC_REG_HIB_BASE 0x30000100
> +#define IST30XXC_REG_TOUCH_STATUS (IST30XXC_REG_HIB_BASE | IST30XXC_HIB_ACCESS)
> +#define IST30XXC_REG_TOUCH_COORD (IST30XXC_REG_HIB_BASE | IST30XXC_HIB_ACCESS | 0x8)
> +#define IST30XXC_REG_INTR_MESSAGE (IST30XXC_REG_HIB_BASE | IST30XXC_HIB_ACCESS | 0x4)
> +#define IST30XXC_CHIP_ON_DELAY_MS 60
> +#define IST30XXC_I2C_RETRY_COUNT 3
> +#define IST30XXC_MAX_FINGER_NUM 10
> +#define IST30XXC_X_MASK GENMASK(23, 12)
> +#define IST30XXC_X_SHIFT 12
> +#define IST30XXC_Y_MASK GENMASK(11, 0)
> +#define IST30XXC_AREA_MASK GENMASK(27, 24)
> +#define IST30XXC_AREA_SHIFT 24
> +#define IST30XXC_FINGER_COUNT_MASK GENMASK(15, 12)
> +#define IST30XXC_FINGER_COUNT_SHIFT 12
> +#define IST30XXC_FINGER_STATUS_MASK GENMASK(9, 0)
> +
> #define IST3038C_WHOAMI 0x38c
> -#define IST3038C_CHIP_ON_DELAY_MS 60
> -#define IST3038C_I2C_RETRY_COUNT 3
> -#define IST3038C_MAX_FINGER_NUM 10
> -#define IST3038C_X_MASK GENMASK(23, 12)
> -#define IST3038C_X_SHIFT 12
> -#define IST3038C_Y_MASK GENMASK(11, 0)
> -#define IST3038C_AREA_MASK GENMASK(27, 24)
> -#define IST3038C_AREA_SHIFT 24
> -#define IST3038C_FINGER_COUNT_MASK GENMASK(15, 12)
> -#define IST3038C_FINGER_COUNT_SHIFT 12
> -#define IST3038C_FINGER_STATUS_MASK GENMASK(9, 0)
>
> struct imagis_ts {
> struct i2c_client *client;
> @@ -57,7 +58,7 @@ static int imagis_i2c_read_reg(struct imagis_ts *ts,
> },
> };
> int ret, error;
> - int retry = IST3038C_I2C_RETRY_COUNT;
> + int retry = IST30XXC_I2C_RETRY_COUNT;
>
> /* Retry in case the controller fails to respond */
> do {
> @@ -84,7 +85,7 @@ static irqreturn_t imagis_interrupt(int irq, void *dev_id)
> int i;
> int error;
>
> - error = imagis_i2c_read_reg(ts, IST3038C_REG_INTR_MESSAGE,
> + error = imagis_i2c_read_reg(ts, IST30XXC_REG_INTR_MESSAGE,
> &intr_message);
> if (error) {
> dev_err(&ts->client->dev,
> @@ -92,20 +93,20 @@ static irqreturn_t imagis_interrupt(int irq, void *dev_id)
> goto out;
> }
>
> - finger_count = (intr_message & IST3038C_FINGER_COUNT_MASK) >>
> - IST3038C_FINGER_COUNT_SHIFT;
> - if (finger_count > IST3038C_MAX_FINGER_NUM) {
> + finger_count = (intr_message & IST30XXC_FINGER_COUNT_MASK) >>
> + IST30XXC_FINGER_COUNT_SHIFT;
> + if (finger_count > IST30XXC_MAX_FINGER_NUM) {
> dev_err(&ts->client->dev,
> "finger count %d is more than maximum supported\n",
> finger_count);
> goto out;
> }
>
> - finger_pressed = intr_message & IST3038C_FINGER_STATUS_MASK;
> + finger_pressed = intr_message & IST30XXC_FINGER_STATUS_MASK;
>
> for (i = 0; i < finger_count; i++) {
> error = imagis_i2c_read_reg(ts,
> - IST3038C_REG_TOUCH_COORD + (i * 4),
> + IST30XXC_REG_TOUCH_COORD + (i * 4),
> &finger_status);
> if (error) {
> dev_err(&ts->client->dev,
> @@ -118,12 +119,12 @@ static irqreturn_t imagis_interrupt(int irq, void *dev_id)
> input_mt_report_slot_state(ts->input_dev, MT_TOOL_FINGER,
> finger_pressed & BIT(i));
> touchscreen_report_pos(ts->input_dev, &ts->prop,
> - (finger_status & IST3038C_X_MASK) >>
> - IST3038C_X_SHIFT,
> - finger_status & IST3038C_Y_MASK, 1);
> + (finger_status & IST30XXC_X_MASK) >>
> + IST30XXC_X_SHIFT,
> + finger_status & IST30XXC_Y_MASK, 1);
> input_report_abs(ts->input_dev, ABS_MT_TOUCH_MAJOR,
> - (finger_status & IST3038C_AREA_MASK) >>
> - IST3038C_AREA_SHIFT);
> + (finger_status & IST30XXC_AREA_MASK) >>
> + IST30XXC_AREA_SHIFT);
> }
>
> input_mt_sync_frame(ts->input_dev);
> @@ -148,7 +149,7 @@ static int imagis_power_on(struct imagis_ts *ts)
> if (error)
> return error;
>
> - msleep(IST3038C_CHIP_ON_DELAY_MS);
> + msleep(IST30XXC_CHIP_ON_DELAY_MS);
>
> return 0;
> }
> @@ -220,7 +221,7 @@ static int imagis_init_input_dev(struct imagis_ts *ts)
> }
>
> error = input_mt_init_slots(input_dev,
> - IST3038C_MAX_FINGER_NUM,
> + IST30XXC_MAX_FINGER_NUM,
> INPUT_MT_DIRECT | INPUT_MT_DROP_UNUSED);
> if (error) {
> dev_err(&ts->client->dev,
> @@ -253,7 +254,7 @@ static int imagis_probe(struct i2c_client *i2c)
> {
> struct device *dev = &i2c->dev;
> struct imagis_ts *ts;
> - int chip_id, error;
> + int chip_id, dt_chip_id, error;
>
> ts = devm_kzalloc(dev, sizeof(*ts), GFP_KERNEL);
> if (!ts)
> @@ -261,6 +262,8 @@ static int imagis_probe(struct i2c_client *i2c)
>
> ts->client = i2c;
>
> + dt_chip_id = (int)(uintptr_t)device_get_match_data(&i2c->dev);
> +
> error = imagis_init_regulators(ts);
> if (error) {
> dev_err(dev, "regulator init error: %d\n", error);
> @@ -280,15 +283,15 @@ static int imagis_probe(struct i2c_client *i2c)
> }
>
> error = imagis_i2c_read_reg(ts,
> - IST3038C_REG_CHIPID | IST3038C_DIRECT_ACCESS,
> + IST30XXC_REG_CHIPID | IST30XXC_DIRECT_ACCESS,
> &chip_id);
> if (error) {
> dev_err(dev, "chip ID read failure: %d\n", error);
> return error;
> }
>
> - if (chip_id != IST3038C_WHOAMI) {
> - dev_err(dev, "unknown chip ID: 0x%x\n", chip_id);
> + if (chip_id != dt_chip_id) {
> + dev_err(dev, "unknown or misconfigured chip ID: 0x%x\n", chip_id);
> return -EINVAL;
> }
>
> @@ -345,12 +348,18 @@ static DEFINE_SIMPLE_DEV_PM_OPS(imagis_pm_ops, imagis_suspend, imagis_resume);
>
> #ifdef CONFIG_OF
> static const struct of_device_id imagis_of_match[] = {
> - { .compatible = "imagis,ist3038c", },
> + { .compatible = "imagis,ist3038c", .data = (void *)IST3038C_WHOAMI, },
> { },
> };
> MODULE_DEVICE_TABLE(of, imagis_of_match);
> #endif
>
> +static const struct i2c_device_id imagis_ts_i2c_id[] = {
> + { "ist3038c", IST3038C_WHOAMI, },
> + { },
> +};
> +MODULE_DEVICE_TABLE(i2c, imagis_ts_i2c_id);
> +
> static struct i2c_driver imagis_ts_driver = {
> .driver = {
> .name = "imagis-touchscreen", @@ -358,10 +367,11 @@ static struct i2c_driver imagis_ts_driver = {
> .of_match_table = of_match_ptr(imagis_of_match), }, .probe =
> imagis_probe, + .id_table = imagis_ts_i2c_id, };
> module_i2c_driver(imagis_ts_driver); -MODULE_DESCRIPTION("Imagis IST3038C Touchscreen Driver");
> +MODULE_DESCRIPTION("Imagis IST30XXC Touchscreen Driver");
> MODULE_AUTHOR("Markuss Broks<markuss.broks@gmail.com>");
> MODULE_LICENSE("GPL");
Additionally, there used to be my series [1] that generalized the
driver, but it never got applied. I will re-send it. It introduces
`struct imagis_properties`, which contains register addresses for the
registers that we use to read the touch input. In your specific case, I
believe it should be:
static const struct imagis_properties imagis_3032c_data =
{
.interrupt_msg_cmd = IST3038C_REG_INTR_MESSAGE,
.touch_coord_cmd = IST3038C_REG_TOUCH_COORD,
.whoami_cmd = IST3038C_REG_CHIPID,
.whoami_val = IST3032C_WHOAMI, /* change it to your whoami */
};
As for the voltage set, I believe this does not belong in a kernel
driver. You should set it in device-tree with `regulator-max-microvolt`
and `regulator-min-microvolt`.
Yours,
- Markuss
[1]:
https://lore.kernel.org/lkml/20220504152406.8730-1-markuss.broks@gmail.com/
^ permalink raw reply
* Re: [PATCH 15/15] platform/x86/amd/pmf: Add PMF-AMDSFH interface for ALS
From: Shyam Sundar S K @ 2023-09-27 13:48 UTC (permalink / raw)
To: Ilpo Järvinen
Cc: hdegoede, markgross, Basavaraj Natikar, jikos, benjamin.tissoires,
alexander.deucher, christian.koenig, Xinhui.Pan, airlied, daniel,
Patil.Reddy, mario.limonciello, platform-driver-x86, linux-input,
amd-gfx, dri-devel
In-Reply-To: <4f542b88-5c8d-4177-50d0-50b0daeaa3a4@linux.intel.com>
Hi Ilpo,
On 9/27/2023 7:03 PM, Ilpo Järvinen wrote:
> On Fri, 22 Sep 2023, Shyam Sundar S K wrote:
>
>> From: Basavaraj Natikar <Basavaraj.Natikar@amd.com>
>>
>> AMDSFH has information about the Ambient light via the Ambient
>> Light Sensor (ALS) which is part of the AMD sensor fusion hub.
>> Add PMF and AMDSFH interface to get this information.
>>
>> Co-developed-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
>> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
>> Signed-off-by: Basavaraj Natikar <Basavaraj.Natikar@amd.com>
>> ---
>> drivers/hid/amd-sfh-hid/amd_sfh_common.h | 1 +
>> drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_init.c | 6 ++++++
>> .../amd-sfh-hid/sfh1_1/amd_sfh_interface.c | 20 +++++++++++++++++++
>> drivers/platform/x86/amd/pmf/spc.c | 5 +++++
>> include/linux/amd-pmf-io.h | 2 ++
>> 5 files changed, 34 insertions(+)
>>
>> diff --git a/drivers/hid/amd-sfh-hid/amd_sfh_common.h b/drivers/hid/amd-sfh-hid/amd_sfh_common.h
>> index cd57037bf217..a1950bc6e6ce 100644
>> --- a/drivers/hid/amd-sfh-hid/amd_sfh_common.h
>> +++ b/drivers/hid/amd-sfh-hid/amd_sfh_common.h
>> @@ -39,6 +39,7 @@ struct amd_mp2_sensor_info {
>>
>> struct sfh_dev_status {
>> bool is_hpd_present;
>> + bool is_als_present;
>> };
>>
>> struct amd_mp2_dev {
>> diff --git a/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_init.c b/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_init.c
>> index 9c623456ee12..d8dad39d68b5 100644
>> --- a/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_init.c
>> +++ b/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_init.c
>> @@ -77,6 +77,9 @@ static int amd_sfh_hid_client_deinit(struct amd_mp2_dev *privdata)
>> case HPD_IDX:
>> privdata->dev_en.is_hpd_present = false;
>> break;
>> + case ALS_IDX:
>> + privdata->dev_en.is_als_present = false;
>> + break;
>> }
>>
>> if (cl_data->sensor_sts[i] == SENSOR_ENABLED) {
>> @@ -188,6 +191,9 @@ static int amd_sfh1_1_hid_client_init(struct amd_mp2_dev *privdata)
>> case HPD_IDX:
>> privdata->dev_en.is_hpd_present = true;
>> break;
>> + case ALS_IDX:
>> + privdata->dev_en.is_als_present = true;
>> + break;
>> }
>> }
>> dev_dbg(dev, "sid 0x%x (%s) status 0x%x\n",
>> diff --git a/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_interface.c b/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_interface.c
>> index 63a5bbca5a09..2f8200fc3062 100644
>> --- a/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_interface.c
>> +++ b/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_interface.c
>> @@ -94,12 +94,32 @@ static int amd_sfh_hpd_info(u8 *user_present)
>> return -ENODEV;
>> }
>>
>> +static int amd_sfh_als_info(u32 *ambient_light)
>> +{
>> + if (emp2 && emp2->dev_en.is_als_present) {
>> + struct sfh_als_data als_data;
>> + void __iomem *sensoraddr;
>> +
>> + sensoraddr = emp2->vsbase +
>> + (ALS_IDX * SENSOR_DATA_MEM_SIZE_DEFAULT) +
>> + OFFSET_SENSOR_DATA_DEFAULT;
>> + memcpy_fromio(&als_data, sensoraddr, sizeof(struct sfh_als_data));
>> + *ambient_light = float_to_int(als_data.lux);
>> +
>> + return 0;
>> + }
>> +
>> + return -ENODEV;
>> +}
>> +
>> int amd_get_sfh_info(struct amd_sfh_info *sfh_info, enum sfh_message_type op)
>> {
>> if (sfh_info) {
>> switch (op) {
>> case MT_HPD:
>> return amd_sfh_hpd_info(&sfh_info->user_present);
>> + case MT_ALS:
>> + return amd_sfh_als_info(&sfh_info->ambient_light);
>> }
>> }
>> return -1;
>> diff --git a/drivers/platform/x86/amd/pmf/spc.c b/drivers/platform/x86/amd/pmf/spc.c
>> index 97293ae25cf5..8e19b351e76f 100644
>> --- a/drivers/platform/x86/amd/pmf/spc.c
>> +++ b/drivers/platform/x86/amd/pmf/spc.c
>> @@ -49,6 +49,7 @@ void amd_pmf_dump_ta_inputs(struct amd_pmf_dev *dev, struct ta_pmf_enact_table *
>> "Connected" : "disconnected/unknown");
>> dev_dbg(dev->dev, "LID State : %s\n", in->ev_info.lid_state ? "Close" : "Open");
>> dev_dbg(dev->dev, "User Presence : %s\n", in->ev_info.user_present ? "Present" : "Away");
>> + dev_dbg(dev->dev, "Ambient Light : %d\n", in->ev_info.ambient_light);
>
> %d vs u32
Thank you very much for your feedback. I will respin a new version
soon addressing your review remarks.
Thanks,
Shyam
>
>> dev_dbg(dev->dev, "==== TA inputs END ====\n");
>> }
>> #else
>> @@ -161,6 +162,10 @@ static void amd_pmf_get_sensor_info(struct amd_pmf_dev *dev, struct ta_pmf_enact
>> {
>> struct amd_sfh_info sfh_info;
>>
>> + /* get ALS data */
>> + amd_get_sfh_info(&sfh_info, MT_ALS);
>> + in->ev_info.ambient_light = sfh_info.ambient_light;
>> +
>> /* get HPD data */
>> amd_get_sfh_info(&sfh_info, MT_HPD);
>> switch (sfh_info.user_present) {
>> diff --git a/include/linux/amd-pmf-io.h b/include/linux/amd-pmf-io.h
>> index 4f82973f6ad2..dac0af573a16 100644
>> --- a/include/linux/amd-pmf-io.h
>> +++ b/include/linux/amd-pmf-io.h
>> @@ -31,6 +31,7 @@ int amd_pmf_set_gfx_data(struct amd_gpu_pmf_data *pmf);
>> /* amd-sfh */
>> enum sfh_message_type {
>> MT_HPD,
>> + MT_ALS,
>> };
>>
>> enum hpd_info {
>> @@ -40,6 +41,7 @@ enum hpd_info {
>> };
>>
>> struct amd_sfh_info {
>> + u32 ambient_light;
>> u8 user_present;
>> /* add future caps below */
>> };
>>
>
^ permalink raw reply
* Re: [PATCH 13/15] platform/x86/amd/pmf: Add PMF-AMDGPU set interface
From: Shyam Sundar S K @ 2023-09-27 13:47 UTC (permalink / raw)
To: Hans de Goede, Christian König, markgross, basavaraj.natikar,
jikos, benjamin.tissoires, alexander.deucher, Xinhui.Pan, airlied,
daniel
Cc: amd-gfx, platform-driver-x86, dri-devel, Patil.Reddy, linux-input,
mario.limonciello
In-Reply-To: <72f7d962-e6ee-274a-74ba-aa68adf5806a@redhat.com>
Hi Hans,
On 9/27/2023 6:34 PM, Hans de Goede wrote:
> HI,
>
> On 9/26/23 15:17, Christian König wrote:
>> Am 26.09.23 um 14:56 schrieb Hans de Goede:
>>> Hi,
>>>
>>> On 9/26/23 13:24, Shyam Sundar S K wrote:
>>>> Hi Hans,
>>>>
>>>> On 9/26/2023 4:05 PM, Hans de Goede wrote:
>>>>> Hi,
>>>>>
>>>>> On 9/22/23 19:50, Shyam Sundar S K wrote:
>>>>>> For the Smart PC Solution to fully work, it has to enact to the actions
>>>>>> coming from TA. Add the initial code path for set interface to AMDGPU.
>>>>>>
>>>>>> Co-developed-by: Mario Limonciello <mario.limonciello@amd.com>
>>>>>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>>>>>> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
>>>>>> ---
>>>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_pmf.c | 21 +++++++++++++++++++++
>>>>>> drivers/platform/x86/amd/pmf/pmf.h | 2 ++
>>>>>> drivers/platform/x86/amd/pmf/tee-if.c | 19 +++++++++++++++++--
>>>>>> include/linux/amd-pmf-io.h | 1 +
>>>>>> 4 files changed, 41 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_pmf.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_pmf.c
>>>>>> index 232d11833ddc..5c567bff0548 100644
>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_pmf.c
>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_pmf.c
>>>>>> @@ -68,3 +68,24 @@ int amd_pmf_get_gfx_data(struct amd_gpu_pmf_data *pmf)
>>>>>> return 0;
>>>>>> }
>>>>>> EXPORT_SYMBOL_GPL(amd_pmf_get_gfx_data);
>>>>>> +
>>>>>> +int amd_pmf_set_gfx_data(struct amd_gpu_pmf_data *pmf)
>>>>>> +{
>>>>>> + struct drm_device *drm_dev = pci_get_drvdata(pmf->gpu_dev);
>>>>>> + struct amdgpu_device *adev = drm_to_adev(drm_dev);
>>>>>> + struct backlight_device *bd;
>>>>>> +
>>>>>> + if (!(adev->flags & AMD_IS_APU)) {
>>>>>> + DRM_ERROR("PMF-AMDGPU interface not supported\n");
>>>>>> + return -ENODEV;
>>>>>> + }
>>>>>> +
>>>>>> + bd = backlight_device_get_by_type(BACKLIGHT_RAW);
>>>>>> + if (!bd)
>>>>>> + return -ENODEV;
>>>>> This assumes that the backlight is always controller by the amdgpu's
>>>>> native backlight driver, but it might e.g. also be handled by
>>>>> eacpi-video or by nvidia_wmi_ec_backlight (when using an AMD APU +
>>>>> nvidia dgpu).
>>>> PMF is meant for AMD APUs(atleast for now) and the _HID will only be
>>>> made visible if its AMD laptop. So using amdgpu's native BACKLIGHT_RAW
>>>> should be safe, right?
>>> Users can pass say acpi_backlight=video and use the acpi_video
>>> driver for backlight control instead of the native GPU backlight
>>> control.
>>>
>>>>> For now what should be done here is to call acpi_video_get_backlight_type()
>>>>> and then translate the return value from this into a backlight-type:
>>>>>
>>>>> acpi_backlight_video -> BACKLIGHT_FIRMWARE
>>>>> acpi_backlight_vendor, -> BACKLIGHT_PLATFORM
>>>>> acpi_backlight_native, -> BACKLIGHT_RAW
>>>>> acpi_backlight_nvidia_wmi_ec, -> BACKLIGHT_FIRMWARE
>>>>> acpi_backlight_apple_gmux, -> BACKLIGHT_PLATFORM
>>>>>
>>>> I can add this change in the v2, do you insist on this?
>>> Insist is a strong word, but I think that it is a good idea to have
>>> this. Evenutally it looks like this code will need to either integrate with
>>> the drm drivers lot more; or the drm core needs to export some special
>>> hooks for this which the PMF code can then call.
>>>
>>> Actually thinking more about this, I think that the right thing to do
>>> here is make some code register brightness control as a cooling device
>>> (which I think is already done in some cases) and then have the PMF
>>> code use the cooling-device APIs for this.
>>>
>>> IMHO that would be a much cleaner solution then this hack.
>>
>> Yeah, fully agree with Hans. This looks like a rather extreme hack to me.
>
> Shyam, the cooling device interface is defined in:
>
> include/linux/thermal.h
>
> And then look for cooling_device .
>
> An example of code registering a cooling_device for backlight control is:
>
> drivers/acpi/acpi_video.c
>
> and then specifically the code starting around line 257 with:
>
> video_get_max_state()
>
> until
>
> static const struct thermal_cooling_device_ops video_cooling_ops = {
> ...
>
> And the code around line 1750 for actually registering the cooling-dev.
>
> To use the cooling_device interface witt amdgpu's native backlight control
> you will need to make the amdgpu backlight control register a cooling-device
> for this in a similar manner.
>
Thank you for pointing to the references. I will address the remarks
from Ilpo and respin a new version soon.
Thanks,
Shyam
> Regards,
>
> Hans
>
>
>
>
>>
>> Apart from that what exactly is this thing supposed to do? Prevent overheating by reducing the brightness?
>>
>> Regards,
>> Christian.
>>
>>>
>>> Regards,
>>>
>>> Hans
>>>
>>>
>>>
>>>> Thanks,
>>>> Shyam
>>>>
>>>>> Also I'm worried about probe order here, this code currently assumes
>>>>> that the GPU or other backlight driver has loaded before this runs,
>>>>> which is not necessarily the case.
>>>>>
>>>>> I think that if the backlight_device_get_by_type() fails this
>>>>> should be retried say every 10 seconds from some delayed workqueue
>>>>> for at least a couple of minutes after boot.
>>>>>
>>>>> Regards,
>>>>>
>>>>> Hans
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>> +
>>>>>> + backlight_device_set_brightness(bd, pmf->brightness);
>>>>>> +
>>>>>> + return 0;
>>>>>> +}
>>>>>> +EXPORT_SYMBOL_GPL(amd_pmf_set_gfx_data);
>>>>>> diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/x86/amd/pmf/pmf.h
>>>>>> index 9032df4ba48a..ce89cc0daa5a 100644
>>>>>> --- a/drivers/platform/x86/amd/pmf/pmf.h
>>>>>> +++ b/drivers/platform/x86/amd/pmf/pmf.h
>>>>>> @@ -73,6 +73,7 @@
>>>>>> #define PMF_POLICY_STT_SKINTEMP_APU 7
>>>>>> #define PMF_POLICY_STT_SKINTEMP_HS2 8
>>>>>> #define PMF_POLICY_SYSTEM_STATE 9
>>>>>> +#define PMF_POLICY_DISPLAY_BRIGHTNESS 12
>>>>>> #define PMF_POLICY_P3T 38
>>>>>> /* TA macros */
>>>>>> @@ -480,6 +481,7 @@ enum ta_pmf_error_type {
>>>>>> };
>>>>>> struct pmf_action_table {
>>>>>> + unsigned long display_brightness;
>>>>>> enum system_state system_state;
>>>>>> unsigned long spl; /* in mW */
>>>>>> unsigned long sppt; /* in mW */
>>>>>> diff --git a/drivers/platform/x86/amd/pmf/tee-if.c b/drivers/platform/x86/amd/pmf/tee-if.c
>>>>>> index 1608996654e8..eefffff83a4c 100644
>>>>>> --- a/drivers/platform/x86/amd/pmf/tee-if.c
>>>>>> +++ b/drivers/platform/x86/amd/pmf/tee-if.c
>>>>>> @@ -79,10 +79,10 @@ static int amd_pmf_update_uevents(struct amd_pmf_dev *dev, u16 event)
>>>>>> return 0;
>>>>>> }
>>>>>> -static void amd_pmf_apply_policies(struct amd_pmf_dev *dev, struct ta_pmf_enact_result *out)
>>>>>> +static int amd_pmf_apply_policies(struct amd_pmf_dev *dev, struct ta_pmf_enact_result *out)
>>>>>> {
>>>>>> u32 val, event = 0;
>>>>>> - int idx;
>>>>>> + int idx, ret;
>>>>>> for (idx = 0; idx < out->actions_count; idx++) {
>>>>>> val = out->actions_list[idx].value;
>>>>>> @@ -160,8 +160,23 @@ static void amd_pmf_apply_policies(struct amd_pmf_dev *dev, struct ta_pmf_enact_
>>>>>> dev->prev_data->system_state = 0;
>>>>>> }
>>>>>> break;
>>>>>> +
>>>>>> + case PMF_POLICY_DISPLAY_BRIGHTNESS:
>>>>>> + ret = amd_pmf_get_gfx_data(&dev->gfx_data);
>>>>>> + if (ret)
>>>>>> + return ret;
>>>>>> +
>>>>>> + dev->prev_data->display_brightness = dev->gfx_data.brightness;
>>>>>> + if (dev->prev_data->display_brightness != val) {
>>>>>> + dev->gfx_data.brightness = val;
>>>>>> + amd_pmf_set_gfx_data(&dev->gfx_data);
>>>>>> + dev_dbg(dev->dev, "update DISPLAY_BRIGHTNESS : %d\n", val);
>>>>>> + }
>>>>>> + break;
>>>>>> }
>>>>>> }
>>>>>> +
>>>>>> + return 0;
>>>>>> }
>>>>>> static int amd_pmf_invoke_cmd_enact(struct amd_pmf_dev *dev)
>>>>>> diff --git a/include/linux/amd-pmf-io.h b/include/linux/amd-pmf-io.h
>>>>>> index a2d4af231362..ecae387ddaa6 100644
>>>>>> --- a/include/linux/amd-pmf-io.h
>>>>>> +++ b/include/linux/amd-pmf-io.h
>>>>>> @@ -25,4 +25,5 @@ struct amd_gpu_pmf_data {
>>>>>> };
>>>>>> int amd_pmf_get_gfx_data(struct amd_gpu_pmf_data *pmf);
>>>>>> +int amd_pmf_set_gfx_data(struct amd_gpu_pmf_data *pmf);
>>>>>> #endif
>>
>
^ permalink raw reply
* Re: [PATCH 13/15] platform/x86/amd/pmf: Add PMF-AMDGPU set interface
From: Ilpo Järvinen @ 2023-09-27 13:36 UTC (permalink / raw)
To: Shyam Sundar S K
Cc: hdegoede, markgross, basavaraj.natikar, jikos, benjamin.tissoires,
alexander.deucher, christian.koenig, Xinhui.Pan, airlied, daniel,
Patil.Reddy, mario.limonciello, platform-driver-x86, linux-input,
amd-gfx, dri-devel
In-Reply-To: <20230922175056.244940-14-Shyam-sundar.S-k@amd.com>
On Fri, 22 Sep 2023, Shyam Sundar S K wrote:
> For the Smart PC Solution to fully work, it has to enact to the actions
> coming from TA. Add the initial code path for set interface to AMDGPU.
>
> Co-developed-by: Mario Limonciello <mario.limonciello@amd.com>
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_pmf.c | 21 +++++++++++++++++++++
> drivers/platform/x86/amd/pmf/pmf.h | 2 ++
> drivers/platform/x86/amd/pmf/tee-if.c | 19 +++++++++++++++++--
> include/linux/amd-pmf-io.h | 1 +
> 4 files changed, 41 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_pmf.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_pmf.c
> index 232d11833ddc..5c567bff0548 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_pmf.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_pmf.c
> @@ -68,3 +68,24 @@ int amd_pmf_get_gfx_data(struct amd_gpu_pmf_data *pmf)
> return 0;
> }
> EXPORT_SYMBOL_GPL(amd_pmf_get_gfx_data);
> +
> +int amd_pmf_set_gfx_data(struct amd_gpu_pmf_data *pmf)
> +{
> + struct drm_device *drm_dev = pci_get_drvdata(pmf->gpu_dev);
> + struct amdgpu_device *adev = drm_to_adev(drm_dev);
> + struct backlight_device *bd;
> +
> + if (!(adev->flags & AMD_IS_APU)) {
> + DRM_ERROR("PMF-AMDGPU interface not supported\n");
> + return -ENODEV;
> + }
> +
> + bd = backlight_device_get_by_type(BACKLIGHT_RAW);
> + if (!bd)
> + return -ENODEV;
> +
> + backlight_device_set_brightness(bd, pmf->brightness);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(amd_pmf_set_gfx_data);
> diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/x86/amd/pmf/pmf.h
> index 9032df4ba48a..ce89cc0daa5a 100644
> --- a/drivers/platform/x86/amd/pmf/pmf.h
> +++ b/drivers/platform/x86/amd/pmf/pmf.h
> @@ -73,6 +73,7 @@
> #define PMF_POLICY_STT_SKINTEMP_APU 7
> #define PMF_POLICY_STT_SKINTEMP_HS2 8
> #define PMF_POLICY_SYSTEM_STATE 9
> +#define PMF_POLICY_DISPLAY_BRIGHTNESS 12
> #define PMF_POLICY_P3T 38
>
> /* TA macros */
> @@ -480,6 +481,7 @@ enum ta_pmf_error_type {
> };
>
> struct pmf_action_table {
> + unsigned long display_brightness;
> enum system_state system_state;
> unsigned long spl; /* in mW */
> unsigned long sppt; /* in mW */
> diff --git a/drivers/platform/x86/amd/pmf/tee-if.c b/drivers/platform/x86/amd/pmf/tee-if.c
> index 1608996654e8..eefffff83a4c 100644
> --- a/drivers/platform/x86/amd/pmf/tee-if.c
> +++ b/drivers/platform/x86/amd/pmf/tee-if.c
> @@ -79,10 +79,10 @@ static int amd_pmf_update_uevents(struct amd_pmf_dev *dev, u16 event)
> return 0;
> }
>
> -static void amd_pmf_apply_policies(struct amd_pmf_dev *dev, struct ta_pmf_enact_result *out)
> +static int amd_pmf_apply_policies(struct amd_pmf_dev *dev, struct ta_pmf_enact_result *out)
Changing return type but no caller is changed to handle the error??
> {
> u32 val, event = 0;
> - int idx;
> + int idx, ret;
>
> for (idx = 0; idx < out->actions_count; idx++) {
> val = out->actions_list[idx].value;
> @@ -160,8 +160,23 @@ static void amd_pmf_apply_policies(struct amd_pmf_dev *dev, struct ta_pmf_enact_
> dev->prev_data->system_state = 0;
> }
> break;
> +
> + case PMF_POLICY_DISPLAY_BRIGHTNESS:
> + ret = amd_pmf_get_gfx_data(&dev->gfx_data);
> + if (ret)
> + return ret;
> +
> + dev->prev_data->display_brightness = dev->gfx_data.brightness;
> + if (dev->prev_data->display_brightness != val) {
> + dev->gfx_data.brightness = val;
> + amd_pmf_set_gfx_data(&dev->gfx_data);
> + dev_dbg(dev->dev, "update DISPLAY_BRIGHTNESS : %d\n", val);
> + }
> + break;
> }
> }
> +
> + return 0;
> }
>
> static int amd_pmf_invoke_cmd_enact(struct amd_pmf_dev *dev)
> diff --git a/include/linux/amd-pmf-io.h b/include/linux/amd-pmf-io.h
> index a2d4af231362..ecae387ddaa6 100644
> --- a/include/linux/amd-pmf-io.h
> +++ b/include/linux/amd-pmf-io.h
> @@ -25,4 +25,5 @@ struct amd_gpu_pmf_data {
> };
>
> int amd_pmf_get_gfx_data(struct amd_gpu_pmf_data *pmf);
> +int amd_pmf_set_gfx_data(struct amd_gpu_pmf_data *pmf);
> #endif
>
--
i.
^ permalink raw reply
* Re: [PATCH 15/15] platform/x86/amd/pmf: Add PMF-AMDSFH interface for ALS
From: Ilpo Järvinen @ 2023-09-27 13:33 UTC (permalink / raw)
To: Shyam Sundar S K
Cc: hdegoede, markgross, Basavaraj Natikar, jikos, benjamin.tissoires,
alexander.deucher, christian.koenig, Xinhui.Pan, airlied, daniel,
Patil.Reddy, mario.limonciello, platform-driver-x86, linux-input,
amd-gfx, dri-devel
In-Reply-To: <20230922175056.244940-16-Shyam-sundar.S-k@amd.com>
On Fri, 22 Sep 2023, Shyam Sundar S K wrote:
> From: Basavaraj Natikar <Basavaraj.Natikar@amd.com>
>
> AMDSFH has information about the Ambient light via the Ambient
> Light Sensor (ALS) which is part of the AMD sensor fusion hub.
> Add PMF and AMDSFH interface to get this information.
>
> Co-developed-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> Signed-off-by: Basavaraj Natikar <Basavaraj.Natikar@amd.com>
> ---
> drivers/hid/amd-sfh-hid/amd_sfh_common.h | 1 +
> drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_init.c | 6 ++++++
> .../amd-sfh-hid/sfh1_1/amd_sfh_interface.c | 20 +++++++++++++++++++
> drivers/platform/x86/amd/pmf/spc.c | 5 +++++
> include/linux/amd-pmf-io.h | 2 ++
> 5 files changed, 34 insertions(+)
>
> diff --git a/drivers/hid/amd-sfh-hid/amd_sfh_common.h b/drivers/hid/amd-sfh-hid/amd_sfh_common.h
> index cd57037bf217..a1950bc6e6ce 100644
> --- a/drivers/hid/amd-sfh-hid/amd_sfh_common.h
> +++ b/drivers/hid/amd-sfh-hid/amd_sfh_common.h
> @@ -39,6 +39,7 @@ struct amd_mp2_sensor_info {
>
> struct sfh_dev_status {
> bool is_hpd_present;
> + bool is_als_present;
> };
>
> struct amd_mp2_dev {
> diff --git a/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_init.c b/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_init.c
> index 9c623456ee12..d8dad39d68b5 100644
> --- a/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_init.c
> +++ b/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_init.c
> @@ -77,6 +77,9 @@ static int amd_sfh_hid_client_deinit(struct amd_mp2_dev *privdata)
> case HPD_IDX:
> privdata->dev_en.is_hpd_present = false;
> break;
> + case ALS_IDX:
> + privdata->dev_en.is_als_present = false;
> + break;
> }
>
> if (cl_data->sensor_sts[i] == SENSOR_ENABLED) {
> @@ -188,6 +191,9 @@ static int amd_sfh1_1_hid_client_init(struct amd_mp2_dev *privdata)
> case HPD_IDX:
> privdata->dev_en.is_hpd_present = true;
> break;
> + case ALS_IDX:
> + privdata->dev_en.is_als_present = true;
> + break;
> }
> }
> dev_dbg(dev, "sid 0x%x (%s) status 0x%x\n",
> diff --git a/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_interface.c b/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_interface.c
> index 63a5bbca5a09..2f8200fc3062 100644
> --- a/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_interface.c
> +++ b/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_interface.c
> @@ -94,12 +94,32 @@ static int amd_sfh_hpd_info(u8 *user_present)
> return -ENODEV;
> }
>
> +static int amd_sfh_als_info(u32 *ambient_light)
> +{
> + if (emp2 && emp2->dev_en.is_als_present) {
> + struct sfh_als_data als_data;
> + void __iomem *sensoraddr;
> +
> + sensoraddr = emp2->vsbase +
> + (ALS_IDX * SENSOR_DATA_MEM_SIZE_DEFAULT) +
> + OFFSET_SENSOR_DATA_DEFAULT;
> + memcpy_fromio(&als_data, sensoraddr, sizeof(struct sfh_als_data));
> + *ambient_light = float_to_int(als_data.lux);
> +
> + return 0;
> + }
> +
> + return -ENODEV;
> +}
> +
> int amd_get_sfh_info(struct amd_sfh_info *sfh_info, enum sfh_message_type op)
> {
> if (sfh_info) {
> switch (op) {
> case MT_HPD:
> return amd_sfh_hpd_info(&sfh_info->user_present);
> + case MT_ALS:
> + return amd_sfh_als_info(&sfh_info->ambient_light);
> }
> }
> return -1;
> diff --git a/drivers/platform/x86/amd/pmf/spc.c b/drivers/platform/x86/amd/pmf/spc.c
> index 97293ae25cf5..8e19b351e76f 100644
> --- a/drivers/platform/x86/amd/pmf/spc.c
> +++ b/drivers/platform/x86/amd/pmf/spc.c
> @@ -49,6 +49,7 @@ void amd_pmf_dump_ta_inputs(struct amd_pmf_dev *dev, struct ta_pmf_enact_table *
> "Connected" : "disconnected/unknown");
> dev_dbg(dev->dev, "LID State : %s\n", in->ev_info.lid_state ? "Close" : "Open");
> dev_dbg(dev->dev, "User Presence : %s\n", in->ev_info.user_present ? "Present" : "Away");
> + dev_dbg(dev->dev, "Ambient Light : %d\n", in->ev_info.ambient_light);
%d vs u32
> dev_dbg(dev->dev, "==== TA inputs END ====\n");
> }
> #else
> @@ -161,6 +162,10 @@ static void amd_pmf_get_sensor_info(struct amd_pmf_dev *dev, struct ta_pmf_enact
> {
> struct amd_sfh_info sfh_info;
>
> + /* get ALS data */
> + amd_get_sfh_info(&sfh_info, MT_ALS);
> + in->ev_info.ambient_light = sfh_info.ambient_light;
> +
> /* get HPD data */
> amd_get_sfh_info(&sfh_info, MT_HPD);
> switch (sfh_info.user_present) {
> diff --git a/include/linux/amd-pmf-io.h b/include/linux/amd-pmf-io.h
> index 4f82973f6ad2..dac0af573a16 100644
> --- a/include/linux/amd-pmf-io.h
> +++ b/include/linux/amd-pmf-io.h
> @@ -31,6 +31,7 @@ int amd_pmf_set_gfx_data(struct amd_gpu_pmf_data *pmf);
> /* amd-sfh */
> enum sfh_message_type {
> MT_HPD,
> + MT_ALS,
> };
>
> enum hpd_info {
> @@ -40,6 +41,7 @@ enum hpd_info {
> };
>
> struct amd_sfh_info {
> + u32 ambient_light;
> u8 user_present;
> /* add future caps below */
> };
>
--
i.
^ permalink raw reply
* Re: [PATCH 14/15] platform/x86/amd/pmf: Add PMF-AMDSFH interface for HPD
From: Ilpo Järvinen @ 2023-09-27 13:32 UTC (permalink / raw)
To: Shyam Sundar S K
Cc: hdegoede, markgross, Basavaraj Natikar, jikos, benjamin.tissoires,
alexander.deucher, christian.koenig, Xinhui.Pan, airlied, daniel,
Patil.Reddy, mario.limonciello, platform-driver-x86, linux-input,
amd-gfx, dri-devel
In-Reply-To: <20230922175056.244940-15-Shyam-sundar.S-k@amd.com>
On Fri, 22 Sep 2023, Shyam Sundar S K wrote:
> From: Basavaraj Natikar <Basavaraj.Natikar@amd.com>
>
> AMDSFH has information about the User presence information via the Human
> Presence Detection (HPD) sensor which is part of the AMD sensor fusion hub.
> Add PMF and AMDSFH interface to get this information.
>
> Co-developed-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> Signed-off-by: Basavaraj Natikar <Basavaraj.Natikar@amd.com>
> ---
> drivers/hid/amd-sfh-hid/amd_sfh_common.h | 5 ++++
> drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_desc.c | 2 +-
> drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_init.c | 11 ++++++++
> .../amd-sfh-hid/sfh1_1/amd_sfh_interface.c | 28 +++++++++++++++++++
> .../amd-sfh-hid/sfh1_1/amd_sfh_interface.h | 1 +
> drivers/platform/x86/amd/pmf/spc.c | 21 ++++++++++++++
> include/linux/amd-pmf-io.h | 22 ++++++++++++++-
> 7 files changed, 88 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/hid/amd-sfh-hid/amd_sfh_common.h b/drivers/hid/amd-sfh-hid/amd_sfh_common.h
> index 2643bb14fee2..cd57037bf217 100644
> --- a/drivers/hid/amd-sfh-hid/amd_sfh_common.h
> +++ b/drivers/hid/amd-sfh-hid/amd_sfh_common.h
> @@ -37,6 +37,10 @@ struct amd_mp2_sensor_info {
> dma_addr_t dma_address;
> };
>
> +struct sfh_dev_status {
> + bool is_hpd_present;
> +};
> +
> struct amd_mp2_dev {
> struct pci_dev *pdev;
> struct amdtp_cl_data *cl_data;
> @@ -47,6 +51,7 @@ struct amd_mp2_dev {
> struct amd_input_data in_data;
> /* mp2 active control status */
> u32 mp2_acs;
> + struct sfh_dev_status dev_en;
> };
>
> struct amd_mp2_ops {
> diff --git a/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_desc.c b/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_desc.c
> index 06bdcf072d10..d7467c41ad3b 100644
> --- a/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_desc.c
> +++ b/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_desc.c
> @@ -132,7 +132,7 @@ static void get_common_inputs(struct common_input_property *common, int report_i
> common->event_type = HID_USAGE_SENSOR_EVENT_DATA_UPDATED_ENUM;
> }
>
> -static int float_to_int(u32 flt32_val)
> +int float_to_int(u32 flt32_val)
This is way too generic name to be exposed by a driver, add proper
prefix for it to make sure it never hits a compile problem.
> {
> int fraction, shift, mantissa, sign, exp, zeropre;
>
> diff --git a/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_init.c b/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_init.c
> index e9c6413af24a..9c623456ee12 100644
> --- a/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_init.c
> +++ b/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_init.c
> @@ -73,6 +73,12 @@ static int amd_sfh_hid_client_deinit(struct amd_mp2_dev *privdata)
> int i, status;
>
> for (i = 0; i < cl_data->num_hid_devices; i++) {
> + switch (cl_data->sensor_idx[i]) {
> + case HPD_IDX:
> + privdata->dev_en.is_hpd_present = false;
> + break;
> + }
> +
> if (cl_data->sensor_sts[i] == SENSOR_ENABLED) {
> privdata->mp2_ops->stop(privdata, cl_data->sensor_idx[i]);
> status = amd_sfh_wait_for_response
> @@ -178,6 +184,11 @@ static int amd_sfh1_1_hid_client_init(struct amd_mp2_dev *privdata)
> rc = amdtp_hid_probe(i, cl_data);
> if (rc)
> goto cleanup;
> + switch (cl_data->sensor_idx[i]) {
> + case HPD_IDX:
> + privdata->dev_en.is_hpd_present = true;
> + break;
> + }
> }
> dev_dbg(dev, "sid 0x%x (%s) status 0x%x\n",
> cl_data->sensor_idx[i], get_sensor_name(cl_data->sensor_idx[i]),
> diff --git a/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_interface.c b/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_interface.c
> index 4f81ef2d4f56..63a5bbca5a09 100644
> --- a/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_interface.c
> +++ b/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_interface.c
> @@ -7,11 +7,14 @@
> *
> * Author: Basavaraj Natikar <Basavaraj.Natikar@amd.com>
> */
> +#include <linux/amd-pmf-io.h>
> #include <linux/io-64-nonatomic-lo-hi.h>
> #include <linux/iopoll.h>
>
> #include "amd_sfh_interface.h"
>
> +static struct amd_mp2_dev *emp2;
> +
> static int amd_sfh_wait_response(struct amd_mp2_dev *mp2, u8 sid, u32 cmd_id)
> {
> struct sfh_cmd_response cmd_resp;
> @@ -76,4 +79,29 @@ static struct amd_mp2_ops amd_sfh_ops = {
> void sfh_interface_init(struct amd_mp2_dev *mp2)
> {
> mp2->mp2_ops = &amd_sfh_ops;
> + emp2 = mp2;
> +}
> +
> +static int amd_sfh_hpd_info(u8 *user_present)
> +{
> + if (emp2 && emp2->dev_en.is_hpd_present) {
> + struct hpd_status hpdstatus;
> +
> + hpdstatus.val = readl(emp2->mmio + AMD_C2P_MSG(4));
> + *user_present = hpdstatus.shpd.presence;
> + return 0;
> + }
> + return -ENODEV;
> +}
> +
> +int amd_get_sfh_info(struct amd_sfh_info *sfh_info, enum sfh_message_type op)
> +{
> + if (sfh_info) {
> + switch (op) {
> + case MT_HPD:
> + return amd_sfh_hpd_info(&sfh_info->user_present);
> + }
> + }
> + return -1;
> }
> +EXPORT_SYMBOL_GPL(amd_get_sfh_info);
> diff --git a/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_interface.h b/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_interface.h
> index 9d31d5b510eb..8a36386e6bce 100644
> --- a/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_interface.h
> +++ b/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_interface.h
> @@ -149,6 +149,7 @@ struct hpd_status {
> };
> };
>
> +int float_to_int(u32 flt32_val);
> void sfh_interface_init(struct amd_mp2_dev *mp2);
> void amd_sfh1_1_set_desc_ops(struct amd_mp2_ops *mp2_ops);
> #endif
> diff --git a/drivers/platform/x86/amd/pmf/spc.c b/drivers/platform/x86/amd/pmf/spc.c
> index 5f3ab1ce09d2..97293ae25cf5 100644
> --- a/drivers/platform/x86/amd/pmf/spc.c
> +++ b/drivers/platform/x86/amd/pmf/spc.c
> @@ -48,6 +48,7 @@ void amd_pmf_dump_ta_inputs(struct amd_pmf_dev *dev, struct ta_pmf_enact_table *
> dev_dbg(dev->dev, "Primary Display State : %s\n", in->ev_info.display_state ?
> "Connected" : "disconnected/unknown");
> dev_dbg(dev->dev, "LID State : %s\n", in->ev_info.lid_state ? "Close" : "Open");
> + dev_dbg(dev->dev, "User Presence : %s\n", in->ev_info.user_present ? "Present" : "Away");
> dev_dbg(dev->dev, "==== TA inputs END ====\n");
> }
> #else
> @@ -156,6 +157,25 @@ static void amd_pmf_get_gpu_info(struct amd_pmf_dev *dev, struct ta_pmf_enact_ta
> in->ev_info.display_state = dev->gfx_data.con_status[0];
> }
>
> +static void amd_pmf_get_sensor_info(struct amd_pmf_dev *dev, struct ta_pmf_enact_table *in)
> +{
> + struct amd_sfh_info sfh_info;
> +
> + /* get HPD data */
> + amd_get_sfh_info(&sfh_info, MT_HPD);
> + switch (sfh_info.user_present) {
> + case SFH_NOT_DETECTED:
> + in->ev_info.user_present = 0xff; /* assume no sensors connected */
> + break;
> + case SFH_USER_PRESENT:
> + in->ev_info.user_present = 1;
> + break;
> + case SFH_USER_AWAY:
> + in->ev_info.user_present = 0;
> + break;
> + }
> +}
> +
> void amd_pmf_populate_ta_inputs(struct amd_pmf_dev *dev, struct ta_pmf_enact_table *in)
> {
> /* TA side lid open is 1 and close is 0, hence the ! here */
> @@ -165,4 +185,5 @@ void amd_pmf_populate_ta_inputs(struct amd_pmf_dev *dev, struct ta_pmf_enact_tab
> amd_pmf_get_battery_info(dev, in);
> amd_pmf_get_slider_info(dev, in);
> amd_pmf_get_gpu_info(dev, in);
> + amd_pmf_get_sensor_info(dev, in);
> }
> diff --git a/include/linux/amd-pmf-io.h b/include/linux/amd-pmf-io.h
> index ecae387ddaa6..4f82973f6ad2 100644
> --- a/include/linux/amd-pmf-io.h
> +++ b/include/linux/amd-pmf-io.h
> @@ -5,7 +5,8 @@
> * Copyright (c) 2023, Advanced Micro Devices, Inc.
> * All Rights Reserved.
> *
> - * Author: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> + * Authors: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> + * Basavaraj Natikar <Basavaraj.Natikar@amd.com>
> */
>
> #ifndef AMD_PMF_IO_H
> @@ -26,4 +27,23 @@ struct amd_gpu_pmf_data {
>
> int amd_pmf_get_gfx_data(struct amd_gpu_pmf_data *pmf);
> int amd_pmf_set_gfx_data(struct amd_gpu_pmf_data *pmf);
> +
> +/* amd-sfh */
> +enum sfh_message_type {
> + MT_HPD,
> +};
> +
> +enum hpd_info {
> + SFH_NOT_DETECTED,
> + SFH_USER_PRESENT,
> + SFH_USER_AWAY
> +};
> +
> +struct amd_sfh_info {
> + u8 user_present;
> + /* add future caps below */
Remove the comment?
> +};
> +
> +int amd_get_sfh_info(struct amd_sfh_info *sfh_info, enum sfh_message_type op);
> +
> #endif
>
--
i.
^ permalink raw reply
* Re: [PATCH 13/15] platform/x86/amd/pmf: Add PMF-AMDGPU set interface
From: Hans de Goede @ 2023-09-27 13:04 UTC (permalink / raw)
To: Christian König, Shyam Sundar S K, markgross,
basavaraj.natikar, jikos, benjamin.tissoires, alexander.deucher,
Xinhui.Pan, airlied, daniel
Cc: amd-gfx, platform-driver-x86, dri-devel, Patil.Reddy, linux-input,
mario.limonciello
In-Reply-To: <4e79121f-01bb-729b-1e70-043e8911cb12@amd.com>
HI,
On 9/26/23 15:17, Christian König wrote:
> Am 26.09.23 um 14:56 schrieb Hans de Goede:
>> Hi,
>>
>> On 9/26/23 13:24, Shyam Sundar S K wrote:
>>> Hi Hans,
>>>
>>> On 9/26/2023 4:05 PM, Hans de Goede wrote:
>>>> Hi,
>>>>
>>>> On 9/22/23 19:50, Shyam Sundar S K wrote:
>>>>> For the Smart PC Solution to fully work, it has to enact to the actions
>>>>> coming from TA. Add the initial code path for set interface to AMDGPU.
>>>>>
>>>>> Co-developed-by: Mario Limonciello <mario.limonciello@amd.com>
>>>>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>>>>> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
>>>>> ---
>>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_pmf.c | 21 +++++++++++++++++++++
>>>>> drivers/platform/x86/amd/pmf/pmf.h | 2 ++
>>>>> drivers/platform/x86/amd/pmf/tee-if.c | 19 +++++++++++++++++--
>>>>> include/linux/amd-pmf-io.h | 1 +
>>>>> 4 files changed, 41 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_pmf.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_pmf.c
>>>>> index 232d11833ddc..5c567bff0548 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_pmf.c
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_pmf.c
>>>>> @@ -68,3 +68,24 @@ int amd_pmf_get_gfx_data(struct amd_gpu_pmf_data *pmf)
>>>>> return 0;
>>>>> }
>>>>> EXPORT_SYMBOL_GPL(amd_pmf_get_gfx_data);
>>>>> +
>>>>> +int amd_pmf_set_gfx_data(struct amd_gpu_pmf_data *pmf)
>>>>> +{
>>>>> + struct drm_device *drm_dev = pci_get_drvdata(pmf->gpu_dev);
>>>>> + struct amdgpu_device *adev = drm_to_adev(drm_dev);
>>>>> + struct backlight_device *bd;
>>>>> +
>>>>> + if (!(adev->flags & AMD_IS_APU)) {
>>>>> + DRM_ERROR("PMF-AMDGPU interface not supported\n");
>>>>> + return -ENODEV;
>>>>> + }
>>>>> +
>>>>> + bd = backlight_device_get_by_type(BACKLIGHT_RAW);
>>>>> + if (!bd)
>>>>> + return -ENODEV;
>>>> This assumes that the backlight is always controller by the amdgpu's
>>>> native backlight driver, but it might e.g. also be handled by
>>>> eacpi-video or by nvidia_wmi_ec_backlight (when using an AMD APU +
>>>> nvidia dgpu).
>>> PMF is meant for AMD APUs(atleast for now) and the _HID will only be
>>> made visible if its AMD laptop. So using amdgpu's native BACKLIGHT_RAW
>>> should be safe, right?
>> Users can pass say acpi_backlight=video and use the acpi_video
>> driver for backlight control instead of the native GPU backlight
>> control.
>>
>>>> For now what should be done here is to call acpi_video_get_backlight_type()
>>>> and then translate the return value from this into a backlight-type:
>>>>
>>>> acpi_backlight_video -> BACKLIGHT_FIRMWARE
>>>> acpi_backlight_vendor, -> BACKLIGHT_PLATFORM
>>>> acpi_backlight_native, -> BACKLIGHT_RAW
>>>> acpi_backlight_nvidia_wmi_ec, -> BACKLIGHT_FIRMWARE
>>>> acpi_backlight_apple_gmux, -> BACKLIGHT_PLATFORM
>>>>
>>> I can add this change in the v2, do you insist on this?
>> Insist is a strong word, but I think that it is a good idea to have
>> this. Evenutally it looks like this code will need to either integrate with
>> the drm drivers lot more; or the drm core needs to export some special
>> hooks for this which the PMF code can then call.
>>
>> Actually thinking more about this, I think that the right thing to do
>> here is make some code register brightness control as a cooling device
>> (which I think is already done in some cases) and then have the PMF
>> code use the cooling-device APIs for this.
>>
>> IMHO that would be a much cleaner solution then this hack.
>
> Yeah, fully agree with Hans. This looks like a rather extreme hack to me.
Shyam, the cooling device interface is defined in:
include/linux/thermal.h
And then look for cooling_device .
An example of code registering a cooling_device for backlight control is:
drivers/acpi/acpi_video.c
and then specifically the code starting around line 257 with:
video_get_max_state()
until
static const struct thermal_cooling_device_ops video_cooling_ops = {
...
And the code around line 1750 for actually registering the cooling-dev.
To use the cooling_device interface witt amdgpu's native backlight control
you will need to make the amdgpu backlight control register a cooling-device
for this in a similar manner.
Regards,
Hans
>
> Apart from that what exactly is this thing supposed to do? Prevent overheating by reducing the brightness?
>
> Regards,
> Christian.
>
>>
>> Regards,
>>
>> Hans
>>
>>
>>
>>> Thanks,
>>> Shyam
>>>
>>>> Also I'm worried about probe order here, this code currently assumes
>>>> that the GPU or other backlight driver has loaded before this runs,
>>>> which is not necessarily the case.
>>>>
>>>> I think that if the backlight_device_get_by_type() fails this
>>>> should be retried say every 10 seconds from some delayed workqueue
>>>> for at least a couple of minutes after boot.
>>>>
>>>> Regards,
>>>>
>>>> Hans
>>>>
>>>>
>>>>
>>>>
>>>>> +
>>>>> + backlight_device_set_brightness(bd, pmf->brightness);
>>>>> +
>>>>> + return 0;
>>>>> +}
>>>>> +EXPORT_SYMBOL_GPL(amd_pmf_set_gfx_data);
>>>>> diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/x86/amd/pmf/pmf.h
>>>>> index 9032df4ba48a..ce89cc0daa5a 100644
>>>>> --- a/drivers/platform/x86/amd/pmf/pmf.h
>>>>> +++ b/drivers/platform/x86/amd/pmf/pmf.h
>>>>> @@ -73,6 +73,7 @@
>>>>> #define PMF_POLICY_STT_SKINTEMP_APU 7
>>>>> #define PMF_POLICY_STT_SKINTEMP_HS2 8
>>>>> #define PMF_POLICY_SYSTEM_STATE 9
>>>>> +#define PMF_POLICY_DISPLAY_BRIGHTNESS 12
>>>>> #define PMF_POLICY_P3T 38
>>>>> /* TA macros */
>>>>> @@ -480,6 +481,7 @@ enum ta_pmf_error_type {
>>>>> };
>>>>> struct pmf_action_table {
>>>>> + unsigned long display_brightness;
>>>>> enum system_state system_state;
>>>>> unsigned long spl; /* in mW */
>>>>> unsigned long sppt; /* in mW */
>>>>> diff --git a/drivers/platform/x86/amd/pmf/tee-if.c b/drivers/platform/x86/amd/pmf/tee-if.c
>>>>> index 1608996654e8..eefffff83a4c 100644
>>>>> --- a/drivers/platform/x86/amd/pmf/tee-if.c
>>>>> +++ b/drivers/platform/x86/amd/pmf/tee-if.c
>>>>> @@ -79,10 +79,10 @@ static int amd_pmf_update_uevents(struct amd_pmf_dev *dev, u16 event)
>>>>> return 0;
>>>>> }
>>>>> -static void amd_pmf_apply_policies(struct amd_pmf_dev *dev, struct ta_pmf_enact_result *out)
>>>>> +static int amd_pmf_apply_policies(struct amd_pmf_dev *dev, struct ta_pmf_enact_result *out)
>>>>> {
>>>>> u32 val, event = 0;
>>>>> - int idx;
>>>>> + int idx, ret;
>>>>> for (idx = 0; idx < out->actions_count; idx++) {
>>>>> val = out->actions_list[idx].value;
>>>>> @@ -160,8 +160,23 @@ static void amd_pmf_apply_policies(struct amd_pmf_dev *dev, struct ta_pmf_enact_
>>>>> dev->prev_data->system_state = 0;
>>>>> }
>>>>> break;
>>>>> +
>>>>> + case PMF_POLICY_DISPLAY_BRIGHTNESS:
>>>>> + ret = amd_pmf_get_gfx_data(&dev->gfx_data);
>>>>> + if (ret)
>>>>> + return ret;
>>>>> +
>>>>> + dev->prev_data->display_brightness = dev->gfx_data.brightness;
>>>>> + if (dev->prev_data->display_brightness != val) {
>>>>> + dev->gfx_data.brightness = val;
>>>>> + amd_pmf_set_gfx_data(&dev->gfx_data);
>>>>> + dev_dbg(dev->dev, "update DISPLAY_BRIGHTNESS : %d\n", val);
>>>>> + }
>>>>> + break;
>>>>> }
>>>>> }
>>>>> +
>>>>> + return 0;
>>>>> }
>>>>> static int amd_pmf_invoke_cmd_enact(struct amd_pmf_dev *dev)
>>>>> diff --git a/include/linux/amd-pmf-io.h b/include/linux/amd-pmf-io.h
>>>>> index a2d4af231362..ecae387ddaa6 100644
>>>>> --- a/include/linux/amd-pmf-io.h
>>>>> +++ b/include/linux/amd-pmf-io.h
>>>>> @@ -25,4 +25,5 @@ struct amd_gpu_pmf_data {
>>>>> };
>>>>> int amd_pmf_get_gfx_data(struct amd_gpu_pmf_data *pmf);
>>>>> +int amd_pmf_set_gfx_data(struct amd_gpu_pmf_data *pmf);
>>>>> #endif
>
^ permalink raw reply
* Re: [PATCH 12/15] platform/x86/amd/pmf: Add PMF-AMDGPU get interface
From: Ilpo Järvinen @ 2023-09-27 12:54 UTC (permalink / raw)
To: Shyam Sundar S K
Cc: Hans de Goede, markgross, basavaraj.natikar, jikos,
benjamin.tissoires, alexander.deucher, christian.koenig,
Xinhui.Pan, airlied, daniel, Patil.Reddy, mario.limonciello,
platform-driver-x86, linux-input, amd-gfx, dri-devel
In-Reply-To: <20230922175056.244940-13-Shyam-sundar.S-k@amd.com>
On Fri, 22 Sep 2023, Shyam Sundar S K wrote:
> In order to provide GPU inputs to TA for the Smart PC solution to work, we
> need to have interface between the PMF driver and the AMDGPU driver.
>
> Add the initial code path for get interface from AMDGPU.
>
> Co-developed-by: Mario Limonciello <mario.limonciello@amd.com>
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> ---
> drivers/gpu/drm/amd/amdgpu/Makefile | 2 +
> drivers/gpu/drm/amd/amdgpu/amdgpu.h | 1 +
> drivers/gpu/drm/amd/amdgpu/amdgpu_pmf.c | 70 +++++++++++++++++++++++++
> drivers/platform/x86/amd/pmf/Kconfig | 1 +
> drivers/platform/x86/amd/pmf/core.c | 1 +
> drivers/platform/x86/amd/pmf/pmf.h | 4 ++
> drivers/platform/x86/amd/pmf/spc.c | 13 +++++
> drivers/platform/x86/amd/pmf/tee-if.c | 22 ++++++++
> include/linux/amd-pmf-io.h | 28 ++++++++++
> 9 files changed, 142 insertions(+)
> create mode 100644 drivers/gpu/drm/amd/amdgpu/amdgpu_pmf.c
> create mode 100644 include/linux/amd-pmf-io.h
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/Makefile b/drivers/gpu/drm/amd/amdgpu/Makefile
> index 384b798a9bad..7fafccefbd7a 100644
> --- a/drivers/gpu/drm/amd/amdgpu/Makefile
> +++ b/drivers/gpu/drm/amd/amdgpu/Makefile
> @@ -86,6 +86,8 @@ amdgpu-$(CONFIG_PROC_FS) += amdgpu_fdinfo.o
>
> amdgpu-$(CONFIG_PERF_EVENTS) += amdgpu_pmu.o
>
> +amdgpu-$(CONFIG_AMD_PMF) += amdgpu_pmf.o
> +
> # add asic specific block
> amdgpu-$(CONFIG_DRM_AMDGPU_CIK)+= cik.o cik_ih.o \
> dce_v8_0.o gfx_v7_0.o cik_sdma.o uvd_v4_2.o vce_v2_0.o
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> index dc2d53081e80..475f3e248f35 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> @@ -50,6 +50,7 @@
> #include <linux/hashtable.h>
> #include <linux/dma-fence.h>
> #include <linux/pci.h>
> +#include <linux/amd-pmf-io.h>
>
> #include <drm/ttm/ttm_bo.h>
> #include <drm/ttm/ttm_placement.h>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_pmf.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_pmf.c
> new file mode 100644
> index 000000000000..232d11833ddc
> --- /dev/null
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_pmf.c
> @@ -0,0 +1,70 @@
> +/*
> + * Copyright 2023 Advanced Micro Devices, Inc.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> + * OTHER DEALINGS IN THE SOFTWARE.
> +
> + * * Author: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> + */
> +
> +#include <linux/backlight.h>
> +#include "amdgpu.h"
> +
> +int amd_pmf_get_gfx_data(struct amd_gpu_pmf_data *pmf)
> +{
> + struct drm_device *drm_dev = pci_get_drvdata(pmf->gpu_dev);
> + struct drm_mode_config *mode_config = &drm_dev->mode_config;
> + struct amdgpu_device *adev = drm_to_adev(drm_dev);
> + struct drm_connector_list_iter iter;
> + struct drm_connector *connector;
> + struct backlight_device *bd;
> + int i = 0;
> +
> + /* reset the count to zero */
> + pmf->display_count = 0;
> + if (!(adev->flags & AMD_IS_APU)) {
> + DRM_ERROR("PMF-AMDGPU interface not supported\n");
> + return -ENODEV;
> + }
> +
> + bd = backlight_device_get_by_type(BACKLIGHT_RAW);
> + if (!bd)
> + return -ENODEV;
> +
> + pmf->brightness = backlight_get_brightness(bd);
> +
> + mutex_lock(&mode_config->mutex);
> + drm_connector_list_iter_begin(drm_dev, &iter);
> +
> + drm_for_each_connector_iter(connector, &iter) {
> + if (i > MAX_SUPPORTED)
> + break;
I'd put this below right after i++.
> + if (connector->status == connector_status_connected) {
> + pmf->con_status[i] = connector->status;
> + pmf->connector_type[i] = connector->connector_type;
> + pmf->display_count++;
> + }
> + i++;
> + }
> + drm_connector_list_iter_end(&iter);
> + mutex_unlock(&mode_config->mutex);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(amd_pmf_get_gfx_data);
> diff --git a/drivers/platform/x86/amd/pmf/Kconfig b/drivers/platform/x86/amd/pmf/Kconfig
> index 437b78c6d1c5..0cd08f9ab51b 100644
> --- a/drivers/platform/x86/amd/pmf/Kconfig
> +++ b/drivers/platform/x86/amd/pmf/Kconfig
> @@ -10,6 +10,7 @@ config AMD_PMF
> depends on AMD_NB
> select ACPI_PLATFORM_PROFILE
> depends on AMDTEE
> + depends on DRM_AMDGPU
> help
> This driver provides support for the AMD Platform Management Framework.
> The goal is to enhance end user experience by making AMD PCs smarter,
> diff --git a/drivers/platform/x86/amd/pmf/core.c b/drivers/platform/x86/amd/pmf/core.c
> index dbfe7c1d6fc4..c468d208b1dc 100644
> --- a/drivers/platform/x86/amd/pmf/core.c
> +++ b/drivers/platform/x86/amd/pmf/core.c
> @@ -396,6 +396,7 @@ static int amd_pmf_probe(struct platform_device *pdev)
> }
>
> dev->cpu_id = rdev->device;
> + dev->root = rdev;
>
> err = amd_smn_read(0, AMD_PMF_BASE_ADDR_LO, &val);
> if (err) {
> diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/x86/amd/pmf/pmf.h
> index 780c442239e3..9032df4ba48a 100644
> --- a/drivers/platform/x86/amd/pmf/pmf.h
> +++ b/drivers/platform/x86/amd/pmf/pmf.h
> @@ -13,6 +13,8 @@
>
> #include <linux/acpi.h>
> #include <linux/platform_profile.h>
> +#include <linux/amd-pmf-io.h>
> +
> #define POLICY_BUF_MAX_SZ 0x4b000
> #define POLICY_SIGN_COOKIE 0x31535024
>
> @@ -224,9 +226,11 @@ struct amd_pmf_dev {
> void *shbuf;
> struct delayed_work pb_work;
> struct pmf_action_table *prev_data;
> + struct amd_gpu_pmf_data gfx_data;
> u64 policy_addr;
> void *policy_base;
> bool smart_pc_enabled;
> + struct pci_dev *root;
> };
>
> struct apmf_sps_prop_granular {
> diff --git a/drivers/platform/x86/amd/pmf/spc.c b/drivers/platform/x86/amd/pmf/spc.c
> index 5c6745f56ed1..5f3ab1ce09d2 100644
> --- a/drivers/platform/x86/amd/pmf/spc.c
> +++ b/drivers/platform/x86/amd/pmf/spc.c
> @@ -43,6 +43,10 @@ void amd_pmf_dump_ta_inputs(struct amd_pmf_dev *dev, struct ta_pmf_enact_table *
> dev_dbg(dev->dev, "Max C0 Residency : %d\n", in->ev_info.max_c0residency);
> dev_dbg(dev->dev, "GFX Busy : %d\n", in->ev_info.gfx_busy);
> dev_dbg(dev->dev, "Connected Display Count : %d\n", in->ev_info.monitor_count);
> + dev_dbg(dev->dev, "Primary Display Type : %s\n",
> + drm_get_connector_type_name(in->ev_info.display_type));
> + dev_dbg(dev->dev, "Primary Display State : %s\n", in->ev_info.display_state ?
> + "Connected" : "disconnected/unknown");
> dev_dbg(dev->dev, "LID State : %s\n", in->ev_info.lid_state ? "Close" : "Open");
> dev_dbg(dev->dev, "==== TA inputs END ====\n");
> }
> @@ -144,6 +148,14 @@ static int amd_pmf_get_slider_info(struct amd_pmf_dev *dev, struct ta_pmf_enact_
> return 0;
> }
>
> +static void amd_pmf_get_gpu_info(struct amd_pmf_dev *dev, struct ta_pmf_enact_table *in)
> +{
> + amd_pmf_get_gfx_data(&dev->gfx_data);
> + in->ev_info.monitor_count = dev->gfx_data.display_count;
> + in->ev_info.display_type = dev->gfx_data.connector_type[0];
> + in->ev_info.display_state = dev->gfx_data.con_status[0];
> +}
> +
> void amd_pmf_populate_ta_inputs(struct amd_pmf_dev *dev, struct ta_pmf_enact_table *in)
> {
> /* TA side lid open is 1 and close is 0, hence the ! here */
> @@ -152,4 +164,5 @@ void amd_pmf_populate_ta_inputs(struct amd_pmf_dev *dev, struct ta_pmf_enact_tab
> amd_pmf_get_smu_info(dev, in);
> amd_pmf_get_battery_info(dev, in);
> amd_pmf_get_slider_info(dev, in);
> + amd_pmf_get_gpu_info(dev, in);
> }
> diff --git a/drivers/platform/x86/amd/pmf/tee-if.c b/drivers/platform/x86/amd/pmf/tee-if.c
> index 3daa122f35d5..1608996654e8 100644
> --- a/drivers/platform/x86/amd/pmf/tee-if.c
> +++ b/drivers/platform/x86/amd/pmf/tee-if.c
> @@ -9,6 +9,7 @@
> */
>
> #include <linux/debugfs.h>
> +#include <linux/pci.h>
> #include <linux/tee_drv.h>
> #include <linux/uuid.h>
> #include "pmf.h"
> @@ -345,6 +346,20 @@ static int amd_pmf_get_bios_buffer(struct amd_pmf_dev *dev)
> return amd_pmf_start_policy_engine(dev);
> }
>
> +static int amd_pmf_get_gpu_handle(struct pci_dev *pdev, void *data)
> +{
> + struct amd_pmf_dev *dev = data;
> +
> + if (pdev->vendor == PCI_VENDOR_ID_ATI && pdev->devfn == 0) {
> + dev->gfx_data.gpu_dev = pci_get_device(pdev->vendor, pdev->device, NULL);
What is this attempting to do??
--
i.
> + if (dev->gfx_data.gpu_dev) {
> + pci_dev_put(pdev);
> + return 1; /* stop walking */
> + }
> + }
> + return 0; /* continue walking */
> +}
> +
> static int amd_pmf_amdtee_ta_match(struct tee_ioctl_version_data *ver, const void *data)
> {
> return ver->impl_id == TEE_IMPL_ID_AMDTEE;
> @@ -435,6 +450,12 @@ int amd_pmf_init_smart_pc(struct amd_pmf_dev *dev)
> INIT_DELAYED_WORK(&dev->pb_work, amd_pmf_invoke_cmd);
> amd_pmf_set_dram_addr(dev);
> amd_pmf_get_bios_buffer(dev);
> +
> + /* get amdgpu handle */
> + pci_walk_bus(dev->root->bus, amd_pmf_get_gpu_handle, dev);
> + if (!dev->gfx_data.gpu_dev)
> + dev_err(dev->dev, "GPU handle not found!\n");
> +
> dev->prev_data = kzalloc(sizeof(*dev->prev_data), GFP_KERNEL);
> if (!dev->prev_data)
> return -ENOMEM;
> @@ -451,5 +472,6 @@ void amd_pmf_deinit_smart_pc(struct amd_pmf_dev *dev)
> kfree(dev->prev_data);
> kfree(dev->policy_buf);
> cancel_delayed_work_sync(&dev->pb_work);
> + pci_dev_put(dev->gfx_data.gpu_dev);
> amd_pmf_tee_deinit(dev);
> }
> diff --git a/include/linux/amd-pmf-io.h b/include/linux/amd-pmf-io.h
> new file mode 100644
> index 000000000000..a2d4af231362
> --- /dev/null
> +++ b/include/linux/amd-pmf-io.h
> @@ -0,0 +1,28 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * AMD Platform Management Framework Interface
> + *
> + * Copyright (c) 2023, Advanced Micro Devices, Inc.
> + * All Rights Reserved.
> + *
> + * Author: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> + */
> +
> +#ifndef AMD_PMF_IO_H
> +#define AMD_PMF_IO_H
> +
> +#include <drm/drm_connector.h>
> +
> +#define MAX_SUPPORTED 4
> +
> +/* amdgpu */
> +struct amd_gpu_pmf_data {
> + struct pci_dev *gpu_dev;
> + enum drm_connector_status con_status[MAX_SUPPORTED];
> + int display_count;
> + int connector_type[MAX_SUPPORTED];
> + int brightness;
> +};
> +
> +int amd_pmf_get_gfx_data(struct amd_gpu_pmf_data *pmf);
> +#endif
>
^ permalink raw reply
* Re: [PATCH 10/15] platform/x86/amd/pmf: Add capability to sideload of policy binary
From: Ilpo Järvinen @ 2023-09-27 12:33 UTC (permalink / raw)
To: Shyam Sundar S K
Cc: hdegoede, markgross, basavaraj.natikar, jikos, benjamin.tissoires,
alexander.deucher, christian.koenig, Xinhui.Pan, airlied, daniel,
Patil.Reddy, mario.limonciello, platform-driver-x86, linux-input,
amd-gfx, dri-devel
In-Reply-To: <20230922175056.244940-11-Shyam-sundar.S-k@amd.com>
On Fri, 22 Sep 2023, Shyam Sundar S K wrote:
> A policy binary is OS agnostic, and the same policies are expected to work
> across the OSes. At times it becomes difficult to debug when the policies
> inside the policy binaries starts to misbehave. Add a way to sideload such
> policies independently to debug them via a debugfs entry.
>
> Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> ---
> drivers/platform/x86/amd/pmf/pmf.h | 1 +
> drivers/platform/x86/amd/pmf/tee-if.c | 60 +++++++++++++++++++++++++++
> 2 files changed, 61 insertions(+)
>
> diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/x86/amd/pmf/pmf.h
> index 61a0f3225b62..780c442239e3 100644
> --- a/drivers/platform/x86/amd/pmf/pmf.h
> +++ b/drivers/platform/x86/amd/pmf/pmf.h
> @@ -215,6 +215,7 @@ struct amd_pmf_dev {
> bool cnqf_supported;
> struct notifier_block pwr_src_notifier;
> /* Smart PC solution builder */
> + struct dentry *esbin;
> unsigned char *policy_buf;
> u32 policy_sz;
> struct tee_context *tee_ctx;
> diff --git a/drivers/platform/x86/amd/pmf/tee-if.c b/drivers/platform/x86/amd/pmf/tee-if.c
> index 4844782d93c7..fa37cfab2dc7 100644
> --- a/drivers/platform/x86/amd/pmf/tee-if.c
> +++ b/drivers/platform/x86/amd/pmf/tee-if.c
> @@ -8,6 +8,7 @@
> * Author: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> */
>
> +#include <linux/debugfs.h>
> #include <linux/tee_drv.h>
> #include <linux/uuid.h>
> #include "pmf.h"
> @@ -21,6 +22,13 @@ module_param(pb_actions_ms, int, 0644);
> MODULE_PARM_DESC(pb_actions_ms, "Policy binary actions sampling frequency (default = 1000ms)");
> #endif
>
> +#ifdef CONFIG_AMD_PMF_DEBUG
> +/* Sideload policy binaries to debug policy failures */
> +static bool pb_side_load;
> +module_param(pb_side_load, bool, 0444);
> +MODULE_PARM_DESC(pb_side_load, "Sideload policy binaries debug policy failures");
> +#endif
> +
> static const uuid_t amd_pmf_ta_uuid = UUID_INIT(0x6fd93b77, 0x3fb8, 0x524d,
> 0xb1, 0x2d, 0xc5, 0x29, 0xb1, 0x3d, 0x85, 0x43);
>
> @@ -267,6 +275,49 @@ static int amd_pmf_start_policy_engine(struct amd_pmf_dev *dev)
> return 0;
> }
>
> +#ifdef CONFIG_AMD_PMF_DEBUG
> +static ssize_t amd_pmf_get_pb_data(struct file *filp, const char __user *buf,
> + size_t length, loff_t *pos)
> +{
> + struct amd_pmf_dev *dev = filp->private_data;
> + int ret;
> +
> + /* policy binary size cannot exceed POLICY_BUF_MAX_SZ */
> + if (length > POLICY_BUF_MAX_SZ || length == 0)
> + return -EINVAL;
> +
> + dev->policy_sz = length;
> + if (copy_from_user(dev->policy_buf, buf, dev->policy_sz))
> + return -EFAULT;
> +
> + ret = amd_pmf_start_policy_engine(dev);
> + if (ret)
> + return -EINVAL;
> +
> + return length;
> +}
> +
> +static const struct file_operations pb_fops = {
> + .write = amd_pmf_get_pb_data,
> + .open = simple_open,
> +};
> +
> +int amd_pmf_open_pb(struct amd_pmf_dev *dev, struct dentry *debugfs_root)
> +{
> + struct dentry *file = NULL;
> +
> + dev->esbin = debugfs_create_dir("pb", debugfs_root);
> + if (IS_ERR(dev->esbin))
> + return -EINVAL;
> +
> + file = debugfs_create_file("update_policy", 0644, dev->esbin, dev, &pb_fops);
> + if (!file)
> + return -EINVAL;
> +
> + return 0;
> +}
> +#endif
> +
> static int amd_pmf_get_bios_buffer(struct amd_pmf_dev *dev)
> {
> dev->policy_buf = kzalloc(dev->policy_sz, GFP_KERNEL);
> @@ -279,6 +330,11 @@ static int amd_pmf_get_bios_buffer(struct amd_pmf_dev *dev)
>
> memcpy(dev->policy_buf, dev->policy_base, dev->policy_sz);
>
> +#ifdef CONFIG_AMD_PMF_DEBUG
> + if (pb_side_load)
Can't this go into amd_pmf_open_pb() as early return?
> + amd_pmf_open_pb(dev, dev->dbgfs_dir);
If you provide #else and empty amd_pmf_open_pb() above, you don't need to
do ifdefs here.
> +#endif
> +
> return amd_pmf_start_policy_engine(dev);
> }
>
> @@ -381,6 +437,10 @@ int amd_pmf_init_smart_pc(struct amd_pmf_dev *dev)
>
> void amd_pmf_deinit_smart_pc(struct amd_pmf_dev *dev)
> {
> +#ifdef CONFIG_AMD_PMF_DEBUG
> + if (pb_side_load)
> + debugfs_remove_recursive(dev->esbin);
> +#endif
Likewise here, if you add amd_pmf_remove_pb() into the above #ifdef + new
#else block with empty body, you can just call it w/o #ifdefs here.
> kfree(dev->prev_data);
> kfree(dev->policy_buf);
> cancel_delayed_work_sync(&dev->pb_work);
>
--
i.
^ permalink raw reply
* Re: [PATCH 09/15] platform/x86/amd/pmf: Add facility to dump TA inputs
From: Ilpo Järvinen @ 2023-09-27 12:25 UTC (permalink / raw)
To: Shyam Sundar S K
Cc: hdegoede, markgross, basavaraj.natikar, jikos, benjamin.tissoires,
alexander.deucher, christian.koenig, Xinhui.Pan, airlied, daniel,
Patil.Reddy, mario.limonciello, platform-driver-x86, linux-input,
amd-gfx, dri-devel
In-Reply-To: <20230922175056.244940-10-Shyam-sundar.S-k@amd.com>
On Fri, 22 Sep 2023, Shyam Sundar S K wrote:
> PMF driver sends constant inputs to TA which its gets via the other
> subsystems in the kernel. To debug certain TA issues knowing what inputs
> being sent to TA becomes critical. Add debug facility to the driver which
> can isolate Smart PC and TA related issues.
>
> Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> ---
> drivers/platform/x86/amd/pmf/pmf.h | 3 +++
> drivers/platform/x86/amd/pmf/spc.c | 37 +++++++++++++++++++++++++++
> drivers/platform/x86/amd/pmf/sps.c | 2 +-
> drivers/platform/x86/amd/pmf/tee-if.c | 1 +
> 4 files changed, 42 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/x86/amd/pmf/pmf.h
> index c5334f1177a4..61a0f3225b62 100644
> --- a/drivers/platform/x86/amd/pmf/pmf.h
> +++ b/drivers/platform/x86/amd/pmf/pmf.h
> @@ -592,6 +592,7 @@ int apmf_get_static_slider_granular(struct amd_pmf_dev *pdev,
> bool is_pprof_balanced(struct amd_pmf_dev *pmf);
> int amd_pmf_power_slider_update_event(struct amd_pmf_dev *dev);
>
> +const char *source_as_str(unsigned int state);
>
> int apmf_update_fan_idx(struct amd_pmf_dev *pdev, bool manual, u32 idx);
> int amd_pmf_set_sps_power_limits(struct amd_pmf_dev *pmf);
> @@ -622,4 +623,6 @@ int apmf_check_smart_pc(struct amd_pmf_dev *pmf_dev);
>
> /* Smart PC - TA interfaces */
> void amd_pmf_populate_ta_inputs(struct amd_pmf_dev *dev, struct ta_pmf_enact_table *in);
> +void amd_pmf_dump_ta_inputs(struct amd_pmf_dev *dev, struct ta_pmf_enact_table *in);
> +
> #endif /* PMF_H */
> diff --git a/drivers/platform/x86/amd/pmf/spc.c b/drivers/platform/x86/amd/pmf/spc.c
> index 08159cd5f853..5c6745f56ed1 100644
> --- a/drivers/platform/x86/amd/pmf/spc.c
> +++ b/drivers/platform/x86/amd/pmf/spc.c
> @@ -13,6 +13,43 @@
> #include <linux/power_supply.h>
> #include "pmf.h"
>
> +#ifdef CONFIG_AMD_PMF_DEBUG
> +static const char *ta_slider_as_str(unsigned int state)
> +{
> + switch (state) {
> + case TA_BEST_PERFORMANCE:
> + return "PERFORMANCE";
> + case TA_BETTER_PERFORMANCE:
> + return "BALANCED";
> + case TA_BEST_BATTERY:
> + return "POWER_SAVER";
> + default:
> + return "Unknown TA Slider State";
> + }
> +}
> +
> +void amd_pmf_dump_ta_inputs(struct amd_pmf_dev *dev, struct ta_pmf_enact_table *in)
> +{
> + dev_dbg(dev->dev, "==== TA inputs START ====\n");
> + dev_dbg(dev->dev, "Slider State : %s\n", ta_slider_as_str(in->ev_info.power_slider));
> + dev_dbg(dev->dev, "Power Source : %s\n", source_as_str(in->ev_info.power_source));
> + dev_dbg(dev->dev, "Battery Percentage : %d\n", in->ev_info.bat_percentage);
> + dev_dbg(dev->dev, "Designed Battery Capacity : %d\n", in->ev_info.bat_design);
> + dev_dbg(dev->dev, "Fully Charged Capacity : %d\n", in->ev_info.full_charge_capacity);
> + dev_dbg(dev->dev, "Drain Rate : %d\n", in->ev_info.drain_rate);
> + dev_dbg(dev->dev, "Socket Power : %d\n", in->ev_info.socket_power);
> + dev_dbg(dev->dev, "Skin Temperature : %d\n", in->ev_info.skin_temperature);
> + dev_dbg(dev->dev, "Avg C0 Residency : %d\n", in->ev_info.avg_c0residency);
> + dev_dbg(dev->dev, "Max C0 Residency : %d\n", in->ev_info.max_c0residency);
> + dev_dbg(dev->dev, "GFX Busy : %d\n", in->ev_info.gfx_busy);
> + dev_dbg(dev->dev, "Connected Display Count : %d\n", in->ev_info.monitor_count);
> + dev_dbg(dev->dev, "LID State : %s\n", in->ev_info.lid_state ? "Close" : "Open");
> + dev_dbg(dev->dev, "==== TA inputs END ====\n");
Again, the printf format specifiers are wrong, shouldn't the compiler warn
about them?
> +}
> +#else
> +void amd_pmf_dump_ta_inputs(struct amd_pmf_dev *dev, struct ta_pmf_enact_table *in) {}
> +#endif
> +
> static void amd_pmf_get_smu_info(struct amd_pmf_dev *dev, struct ta_pmf_enact_table *in)
> {
> u16 max, avg = 0;
> diff --git a/drivers/platform/x86/amd/pmf/sps.c b/drivers/platform/x86/amd/pmf/sps.c
> index a70e67749be3..13e36b52dfe8 100644
> --- a/drivers/platform/x86/amd/pmf/sps.c
> +++ b/drivers/platform/x86/amd/pmf/sps.c
> @@ -27,7 +27,7 @@ static const char *slider_as_str(unsigned int state)
> }
> }
>
> -static const char *source_as_str(unsigned int state)
> +const char *source_as_str(unsigned int state)
> {
> switch (state) {
> case POWER_SOURCE_AC:
> diff --git a/drivers/platform/x86/amd/pmf/tee-if.c b/drivers/platform/x86/amd/pmf/tee-if.c
> index 1629856c20b4..4844782d93c7 100644
> --- a/drivers/platform/x86/amd/pmf/tee-if.c
> +++ b/drivers/platform/x86/amd/pmf/tee-if.c
> @@ -186,6 +186,7 @@ static int amd_pmf_invoke_cmd_enact(struct amd_pmf_dev *dev)
> }
>
> if (ta_sm->pmf_result == TA_PMF_TYPE__SUCCESS && out->actions_count) {
> + amd_pmf_dump_ta_inputs(dev, in);
> dev_dbg(dev->dev, "action count:%d result:%x\n", out->actions_count,
> ta_sm->pmf_result);
> amd_pmf_apply_policies(dev, out);
>
--
i.
^ permalink raw reply
* Re: [PATCH 08/15] platform/x86/amd/pmf: Add support to update system state
From: Ilpo Järvinen @ 2023-09-27 12:22 UTC (permalink / raw)
To: Shyam Sundar S K
Cc: Hans de Goede, markgross, basavaraj.natikar, jikos,
benjamin.tissoires, alexander.deucher, christian.koenig,
Xinhui.Pan, airlied, daniel, Patil.Reddy, mario.limonciello,
platform-driver-x86, linux-input, amd-gfx, dri-devel
In-Reply-To: <20230922175056.244940-9-Shyam-sundar.S-k@amd.com>
On Fri, 22 Sep 2023, Shyam Sundar S K wrote:
> PMF driver based on the output actions from the TA can request to update
> the system states like entering s0i3, lock screen etc. by generating
> an uevent. Based on the udev rules set in the userspace the event id
> matching the uevent shall get updated accordingly using the systemctl.
>
> Sample udev rules under Documentation/admin-guide/pmf.rst.
>
> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> ---
> Documentation/admin-guide/pmf.rst | 24 ++++++++++++++++
> drivers/platform/x86/amd/pmf/pmf.h | 9 ++++++
> drivers/platform/x86/amd/pmf/tee-if.c | 40 ++++++++++++++++++++++++++-
> 3 files changed, 72 insertions(+), 1 deletion(-)
> create mode 100644 Documentation/admin-guide/pmf.rst
>
> diff --git a/Documentation/admin-guide/pmf.rst b/Documentation/admin-guide/pmf.rst
> new file mode 100644
> index 000000000000..b60f381410c3
> --- /dev/null
> +++ b/Documentation/admin-guide/pmf.rst
> @@ -0,0 +1,24 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +Set udev rules for PMF Smart PC Builder
> +---------------------------------------
> +
> +AMD PMF(Platform Management Framework) Smart PC Solution builder has to set the system states
> +like S0i3, Screen lock, hibernate etc, based on the output actions provided by the PMF
> +TA (Trusted Application).
> +
> +In order for this to work the PMF driver generates a uevent for userspace to react to. Below are
> +sample udev rules that can facilitate this experience when a machine has PMF Smart PC solution builder
> +enabled.
> +
> +Please add the following line(s) to
> +``/etc/udev/rules.d/99-local.rules``::
> + DRIVERS=="amd-pmf", ACTION=="change", ENV{EVENT_ID}=="1", RUN+="/usr/bin/systemctl suspend"
> + DRIVERS=="amd-pmf", ACTION=="change", ENV{EVENT_ID}=="2", RUN+="/usr/bin/systemctl hibernate"
> + DRIVERS=="amd-pmf", ACTION=="change", ENV{EVENT_ID}=="3", RUN+="/bin/loginctl lock-sessions"
> +
> +EVENT_ID values:
> +1= Put the system to S0i3/S2Idle
> +2= Put the system to hibernate
> +3= Lock the screen
> +
> diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/x86/amd/pmf/pmf.h
> index 897f61b75e2f..c5334f1177a4 100644
> --- a/drivers/platform/x86/amd/pmf/pmf.h
> +++ b/drivers/platform/x86/amd/pmf/pmf.h
> @@ -70,6 +70,7 @@
> #define PMF_POLICY_STT_MIN 6
> #define PMF_POLICY_STT_SKINTEMP_APU 7
> #define PMF_POLICY_STT_SKINTEMP_HS2 8
> +#define PMF_POLICY_SYSTEM_STATE 9
> #define PMF_POLICY_P3T 38
>
> /* TA macros */
> @@ -436,6 +437,13 @@ struct apmf_dyn_slider_output {
> } __packed;
>
> /* Smart PC - TA internals */
> +enum system_state {
> + SYSTEM_STATE__S0i3 = 1,
> + SYSTEM_STATE__S4,
> + SYSTEM_STATE__SCREEN_LOCK,
> + SYSTEM_STATE__MAX
> +};
> +
> enum ta_slider {
> TA_BEST_BATTERY, /* Best Battery */
> TA_BETTER_BATTERY, /* Better Battery */
> @@ -467,6 +475,7 @@ enum ta_pmf_error_type {
> };
>
> struct pmf_action_table {
> + enum system_state system_state;
> unsigned long spl; /* in mW */
> unsigned long sppt; /* in mW */
> unsigned long sppt_apuonly; /* in mW */
> diff --git a/drivers/platform/x86/amd/pmf/tee-if.c b/drivers/platform/x86/amd/pmf/tee-if.c
> index 883dd143375a..1629856c20b4 100644
> --- a/drivers/platform/x86/amd/pmf/tee-if.c
> +++ b/drivers/platform/x86/amd/pmf/tee-if.c
> @@ -24,6 +24,20 @@ MODULE_PARM_DESC(pb_actions_ms, "Policy binary actions sampling frequency (defau
> static const uuid_t amd_pmf_ta_uuid = UUID_INIT(0x6fd93b77, 0x3fb8, 0x524d,
> 0xb1, 0x2d, 0xc5, 0x29, 0xb1, 0x3d, 0x85, 0x43);
>
> +static const char *amd_pmf_uevent_as_str(unsigned int state)
> +{
> + switch (state) {
> + case SYSTEM_STATE__S0i3:
> + return "S0i3";
> + case SYSTEM_STATE__S4:
> + return "S4";
> + case SYSTEM_STATE__SCREEN_LOCK:
> + return "SCREEN_LOCK";
> + default:
> + return "Unknown Smart PC event";
> + }
> +}
> +
> static void amd_pmf_prepare_args(struct amd_pmf_dev *dev, int cmd,
> struct tee_ioctl_invoke_arg *arg,
> struct tee_param *param)
> @@ -42,9 +56,23 @@ static void amd_pmf_prepare_args(struct amd_pmf_dev *dev, int cmd,
> param[0].u.memref.shm_offs = 0;
> }
>
> +static int amd_pmf_update_uevents(struct amd_pmf_dev *dev, u16 event)
> +{
> + char *envp[2] = {};
> +
> + envp[0] = kasprintf(GFP_KERNEL, "EVENT_ID=%d", event);
> + if (!envp[0])
> + return -EINVAL;
> +
> + kobject_uevent_env(&dev->dev->kobj, KOBJ_CHANGE, envp);
> +
> + kfree(envp[0]);
> + return 0;
> +}
> +
> static void amd_pmf_apply_policies(struct amd_pmf_dev *dev, struct ta_pmf_enact_result *out)
> {
> - u32 val;
> + u32 val, event = 0;
> int idx;
>
> for (idx = 0; idx < out->actions_count; idx++) {
> @@ -113,6 +141,16 @@ static void amd_pmf_apply_policies(struct amd_pmf_dev *dev, struct ta_pmf_enact_
> dev->prev_data->p3t_limit = val;
> }
> break;
> +
> + case PMF_POLICY_SYSTEM_STATE:
> + event = val + 1;
> + if (dev->prev_data->system_state != event) {
> + amd_pmf_update_uevents(dev, event);
> + dev_dbg(dev->dev, "update SYSTEM_STATE : %s\n",
> + amd_pmf_uevent_as_str(event));
> + dev->prev_data->system_state = 0;
Is it intentional to assign 0 here? If it is, it makes
prev_data->system_state pretty useless?
> + }
> + break;
> }
> }
> }
>
--
i.
^ permalink raw reply
* Re: [PATCH 07/15] platform/x86/amd/pmf: Add support update p3t limit
From: Ilpo Järvinen @ 2023-09-27 12:19 UTC (permalink / raw)
To: Shyam Sundar S K
Cc: hdegoede, markgross, basavaraj.natikar, jikos, benjamin.tissoires,
alexander.deucher, christian.koenig, Xinhui.Pan, airlied, daniel,
Patil.Reddy, mario.limonciello, platform-driver-x86, linux-input,
amd-gfx, dri-devel
In-Reply-To: <20230922175056.244940-8-Shyam-sundar.S-k@amd.com>
On Fri, 22 Sep 2023, Shyam Sundar S K wrote:
> P3T (Peak Package Power Limit) is a metric within the SMU controller
> that can influence the power limits. Add support from the driver
> to update P3T limits accordingly.
>
> Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> ---
> drivers/platform/x86/amd/pmf/pmf.h | 3 +++
> drivers/platform/x86/amd/pmf/tee-if.c | 8 ++++++++
> 2 files changed, 11 insertions(+)
>
> diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/x86/amd/pmf/pmf.h
> index e64b4d285624..897f61b75e2f 100644
> --- a/drivers/platform/x86/amd/pmf/pmf.h
> +++ b/drivers/platform/x86/amd/pmf/pmf.h
> @@ -46,6 +46,7 @@
> #define GET_STT_MIN_LIMIT 0x1F
> #define GET_STT_LIMIT_APU 0x20
> #define GET_STT_LIMIT_HS2 0x21
> +#define SET_P3T 0x23 /* P3T: Peak Package Power Limit */
>
> /* OS slider update notification */
> #define DC_BEST_PERF 0
> @@ -69,6 +70,7 @@
> #define PMF_POLICY_STT_MIN 6
> #define PMF_POLICY_STT_SKINTEMP_APU 7
> #define PMF_POLICY_STT_SKINTEMP_HS2 8
> +#define PMF_POLICY_P3T 38
>
> /* TA macros */
> #define PMF_TA_IF_VERSION__MAJOR 1
> @@ -472,6 +474,7 @@ struct pmf_action_table {
> unsigned long stt_minlimit; /* in mW */
> unsigned long stt_skintemp_apu; /* in C */
> unsigned long stt_skintemp_hs2; /* in C */
> + unsigned long p3t_limit; /* in mW */
> };
>
> /* Input conditions */
> diff --git a/drivers/platform/x86/amd/pmf/tee-if.c b/drivers/platform/x86/amd/pmf/tee-if.c
> index eb25d5ce3a9a..883dd143375a 100644
> --- a/drivers/platform/x86/amd/pmf/tee-if.c
> +++ b/drivers/platform/x86/amd/pmf/tee-if.c
> @@ -105,6 +105,14 @@ static void amd_pmf_apply_policies(struct amd_pmf_dev *dev, struct ta_pmf_enact_
> dev->prev_data->stt_skintemp_hs2 = val;
> }
> break;
> +
> + case PMF_POLICY_P3T:
> + if (dev->prev_data->p3t_limit != val) {
> + amd_pmf_send_cmd(dev, SET_P3T, false, val, NULL);
> + dev_dbg(dev->dev, "update P3T : %d\n", val);
%d vs u32
> + dev->prev_data->p3t_limit = val;
unsigned long vs u32 ? (as in the other patch)
--
i.
^ permalink raw reply
* Re: [PATCH 04/15] platform/x86/amd/pmf: Add support for PMF Policy Binary
From: Ilpo Järvinen @ 2023-09-27 12:19 UTC (permalink / raw)
To: Shyam Sundar S K
Cc: Hans de Goede, markgross, basavaraj.natikar, jikos,
benjamin.tissoires, alexander.deucher, christian.koenig,
Xinhui.Pan, airlied, daniel, Patil.Reddy, mario.limonciello,
platform-driver-x86, linux-input, amd-gfx, dri-devel
In-Reply-To: <20230922175056.244940-5-Shyam-sundar.S-k@amd.com>
On Fri, 22 Sep 2023, Shyam Sundar S K wrote:
> PMF Policy binary is a encrypted and signed binary that will be part
> of the BIOS. PMF driver via the ACPI interface checks the existence
> of Smart PC bit. If the advertised bit is found, PMF driver walks
> the acpi namespace to find out the policy binary size and the address
> which has to be passed to the TA during the TA init sequence.
>
> The policy binary is comprised of inputs (or the events) and outputs
> (or the actions). With the PMF ecosystem, OEMs generate the policy
> binary (or could be multiple binaries) that contains a supported set
> of inputs and outputs which could be specifically carved out for each
> usage segment (or for each user also) that could influence the system
> behavior either by enriching the user experience or/and boost/throttle
> power limits.
>
> Once the TA init command succeeds, the PMF driver sends the changing
> events in the current environment to the TA for a constant sampling
> frequency time (the event here could be a lid close or open) and
> if the policy binary has corresponding action built within it, the
> TA sends the action for it in the subsequent enact command.
>
> If the inputs sent to the TA has no output defined in the policy
> binary generated by OEMs, there will be no action to be performed
> by the PMF driver.
>
> Example policies:
>
> 1) if slider is performance ; set the SPL to 40W
> Here PMF driver registers with the platform profile interface and
> when the slider position is changed, PMF driver lets the TA know
> about this. TA sends back an action to update the Sustained
> Power Limit (SPL). PMF driver updates this limit via the PMFW mailbox.
>
> 2) if user_away ; then lock the system
> Here PMF driver hooks to the AMD SFH driver to know the user presence
> and send the inputs to TA and if the condition is met, the TA sends
> the action of locking the system. PMF driver generates a uevent and
> based on the udev rule in the userland the system gets locked with
> systemctl.
>
> The intent here is to provide the OEM's to make a policy to lock the
> system when the user is away ; but the userland can make a choice to
> ignore it.
>
> and so on.
>
> The OEMs will have an utility to create numerous such policies and
> the policies shall be reviewed by AMD before signing and encrypting
> them. Policies are shared between operating systems to have seemless user
> experience.
>
> Since all this action has to happen via the "amdtee" driver, currently
> there is no caller for it in the kernel which can load the amdtee driver.
> Without amdtee driver loading onto the system the "tee" calls shall fail
> from the PMF driver. Hence an explicit "request_module" has been added
> to address this.
>
> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> ---
> +struct pmf_action_table {
> + unsigned long spl; /* in mW */
> + unsigned long sppt; /* in mW */
> + unsigned long sppt_apuonly; /* in mW */
> + unsigned long fppt; /* in mW */
> + unsigned long stt_minlimit; /* in mW */
> + unsigned long stt_skintemp_apu; /* in C */
> + unsigned long stt_skintemp_hs2; /* in C */
> +};
> +static void amd_pmf_apply_policies(struct amd_pmf_dev *dev, struct ta_pmf_enact_result *out)
> +{
> + u32 val;
> + int idx;
> +
> + for (idx = 0; idx < out->actions_count; idx++) {
> + val = out->actions_list[idx].value;
> + switch (out->actions_list[idx].action_index) {
> + case PMF_POLICY_SPL:
> + if (dev->prev_data->spl != val) {
> + amd_pmf_send_cmd(dev, SET_SPL, false, val, NULL);
> + dev_dbg(dev->dev, "update SPL : %d\n", val);
The %d does not match u32.
> + dev->prev_data->spl = val;
Why is ->spl (and the others too) unsigned long if it's only assigned u32?
> + }
> + break;
> +
> + case PMF_POLICY_SPPT:
> + if (dev->prev_data->sppt != val) {
> + amd_pmf_send_cmd(dev, SET_SPPT, false, val, NULL);
> + dev_dbg(dev->dev, "update SPPT : %d\n", val);
> + dev->prev_data->sppt = val;
> + }
> + break;
> +
> + case PMF_POLICY_FPPT:
> + if (dev->prev_data->fppt != val) {
> + amd_pmf_send_cmd(dev, SET_FPPT, false, val, NULL);
> + dev_dbg(dev->dev, "update FPPT : %d\n", val);
> + dev->prev_data->fppt = val;
> + }
> + break;
> +
> + case PMF_POLICY_SPPT_APU_ONLY:
> + if (dev->prev_data->sppt_apuonly != val) {
> + amd_pmf_send_cmd(dev, SET_SPPT_APU_ONLY, false, val, NULL);
> + dev_dbg(dev->dev, "update SPPT_APU_ONLY : %d\n", val);
> + dev->prev_data->sppt_apuonly = val;
> + }
> + break;
> +
> + case PMF_POLICY_STT_MIN:
> + if (dev->prev_data->stt_minlimit != val) {
> + amd_pmf_send_cmd(dev, SET_STT_MIN_LIMIT, false, val, NULL);
> + dev_dbg(dev->dev, "update STT_MIN : %d\n", val);
> + dev->prev_data->stt_minlimit = val;
> + }
> + break;
> +
> + case PMF_POLICY_STT_SKINTEMP_APU:
> + if (dev->prev_data->stt_skintemp_apu != val) {
> + amd_pmf_send_cmd(dev, SET_STT_LIMIT_APU, false, val, NULL);
> + dev_dbg(dev->dev, "update STT_SKINTEMP_APU : %d\n", val);
> + dev->prev_data->stt_skintemp_apu = val;
> + }
> + break;
> +
> + case PMF_POLICY_STT_SKINTEMP_HS2:
> + if (dev->prev_data->stt_skintemp_hs2 != val) {
> + amd_pmf_send_cmd(dev, SET_STT_LIMIT_HS2, false, val, NULL);
> + dev_dbg(dev->dev, "update STT_SKINTEMP_HS2 : %d\n", val);
> + dev->prev_data->stt_skintemp_hs2 = val;
> + }
> + break;
> + }
> + }
> +}
> +
--
i.
^ permalink raw reply
* Re: [RFC PATCH 2/2] hid: lenovo: move type checks to lenovo_features_set_cptkbd()
From: Martin Kepplinger @ 2023-09-27 11:20 UTC (permalink / raw)
To: Jamie Lentin; +Cc: jikos, benjamin.tissoires, linux-kernel, linux-input
In-Reply-To: <ef0f15c3b17ebbd58f7481910b3f40ff@lentin.co.uk>
Am Mittwoch, dem 27.09.2023 um 09:19 +0100 schrieb Jamie Lentin:
> On 2023-09-25 11:23, Martin Kepplinger wrote:
> > These custom commands will be sent to both the USB keyboard & mouse
> > devices but only the mouse will respond. Avoid sending known-
> > useless
> > messages by always prepending the filter before sending them.
> >
> > Suggested-by: Jamie Lentin <jm@lentin.co.uk>
> > Signed-off-by: Martin Kepplinger <martink@posteo.de>
> > ---
> > drivers/hid/hid-lenovo.c | 27 +++++++++------------------
> > 1 file changed, 9 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/hid/hid-lenovo.c b/drivers/hid/hid-lenovo.c
> > index 29aa6d372bad..922f3e5462f4 100644
> > --- a/drivers/hid/hid-lenovo.c
> > +++ b/drivers/hid/hid-lenovo.c
> > @@ -521,6 +521,14 @@ static void lenovo_features_set_cptkbd(struct
> > hid_device *hdev)
> > int ret;
> > struct lenovo_drvdata *cptkbd_data = hid_get_drvdata(hdev);
> >
> > + /* All the custom action happens on the USBMOUSE device for
> > USB */
> > + if (((hdev->product == USB_DEVICE_ID_LENOVO_CUSBKBD) ||
> > + (hdev->product == USB_DEVICE_ID_LENOVO_TPIIUSBKBD)) &&
> > + hdev->type != HID_TYPE_USBMOUSE) {
> > + hid_dbg(hdev, "Ignoring keyboard half of
> > device\n");
> > + return;
> > + }
> > +
> > /*
> > * Tell the keyboard a driver understands it, and turn F7,
> > F9, F11
> > into
> > * regular keys
> > @@ -1122,14 +1130,6 @@ static int lenovo_probe_cptkbd(struct
> > hid_device
> > *hdev)
> > int ret;
> > struct lenovo_drvdata *cptkbd_data;
> >
> > - /* All the custom action happens on the USBMOUSE device for
> > USB */
> > - if (((hdev->product == USB_DEVICE_ID_LENOVO_CUSBKBD) ||
> > - (hdev->product == USB_DEVICE_ID_LENOVO_TPIIUSBKBD)) &&
> > - hdev->type != HID_TYPE_USBMOUSE) {
> > - hid_dbg(hdev, "Ignoring keyboard half of
> > device\n");
> > - return 0;
> > - }
> > -
>
> I like the idea of doing it once then forgetting about it, but
> removing
> this will mean that the "keyboard half" will have it's own set of
> non-functional sysfs parameters I think? Currently:-
>
> # evtest
> . . .
> /dev/input/event10: ThinkPad Compact Bluetooth Keyboard with
> TrackPoint
> /dev/input/event11: Lenovo ThinkPad Compact USB Keyboard with
> TrackPoint
> /dev/input/event12: Lenovo ThinkPad Compact USB Keyboard with
> TrackPoint
>
> # ls -1 /sys/class/input/event*/device/device/fn_lock
> /sys/class/input/event10/device/device/fn_lock
> /sys/class/input/event12/device/device/fn_lock
>
> (note 11 is missing.)
>
> I think the easiest (but ugly) thing to do is to copy-paste this lump
> of
> code to the top of lenovo_reset_resume.
> Cheers,
>
> > cptkbd_data = devm_kzalloc(&hdev->dev,
> > sizeof(*cptkbd_data),
> > GFP_KERNEL);
> > @@ -1264,16 +1264,7 @@ static int lenovo_probe(struct hid_device
> > *hdev,
> > #ifdef CONFIG_PM
> > static int lenovo_reset_resume(struct hid_device *hdev)
> > {
> > - switch (hdev->product) {
> > - case USB_DEVICE_ID_LENOVO_CUSBKBD:
> > - if (hdev->type == HID_TYPE_USBMOUSE) {
> > - lenovo_features_set_cptkbd(hdev);
> > - }
> > -
> > - break;
> > - default:
> > - break;
> > - }
> > + lenovo_features_set_cptkbd(hdev);
ok. ignore my change (this whole patch) and look at your addition here,
don't you already make sure only the mouse-part gets the messages? you
just write switch()case instead of if(); what do you think is missing
here?
thanks,
martin
> >
> > return 0;
> > }
^ permalink raw reply
* [PATCH] HID parser: change usages table allocation to kvzalloc
From: Marcin Mikula @ 2023-09-27 10:58 UTC (permalink / raw)
To: linux-input; +Cc: Marcin Mikula
There are devices which deliver buggy HID descriptors, repoting
Usage Maximum for particular Usage Page of max value (0xFFFF).
By decision of kernel HID subsystem developers such number will be limited
to arbitrary value of HID_MAX_USAGES (12288), and the following log will
be printed to warn about such condition:
"kernel: hid-generic 0005:057A:0087.001A: ignoring exceeding usage max"
but still the amount of memory allocated for HID_MAX_USAGES modified value
can be quite high as for kmalloc allocation.
On some systems with limited memory (seen in ARM based embedded system),
in high memory pressure condition an attempt to kmalloc() such value
can fail.
As a consequence this HID Usage Page will not parsed, so will not be
handled and the events will be ignored.
From the user perspective part of the input described with this Usage Page
simply will not work, which is a severe condition.
Change of kzalloc to kvzalloc makes sure that this allocation will
not fail.
Example of the kernel callstack when allocation fails for reference:
kernel: kworker/1:1: page allocation failure: order:7,
mode:0x40dc0(GFP_KERNEL|__GFP_COMP|__GFP_ZERO), nodemask=(null),cpuset=/,
mems_allowed=0
Workqueue: events uhid_device_add_worker
Call trace:
dump_backtrace+0x0/0x1b4
show_stack+0x24/0x30
dump_stack+0xac/0x10c
warn_alloc+0xd8/0x158
__alloc_pages_slowpath+0x2e0/0x914
__alloc_pages_nodemask+0x1b0/0x278
kmalloc_order+0x30/0x7c
kmalloc_order_trace+0x3c/0xd4
__kmalloc+0x50/0x188
hid_add_field+0x160/0x250
hid_parser_main+0x220/0x25c
hid_open_report+0x138/0x278
hid_generic_probe+0x40/0x5c
hid_device_probe+0x130/0x154
really_probe+0x274/0x3f0
driver_probe_device+0x118/0x128
__device_attach_driver+0xc4/0x108
bus_for_each_drv+0xa4/0xc8
__device_attach+0xf4/0x17c
device_initial_probe+0x24/0x30
bus_probe_device+0x38/0x98
device_add+0x4a0/0x578
hid_add_device+0x100/0x294
uhid_device_add_worker+0x28/0x70
process_one_work+0x19c/0x294
worker_thread+0x1e4/0x274
kthread+0xdc/0xec
ret_from_fork+0x10/0x18
Signed-off-by: Marcin Mikula <marcin@helix.pl>
---
drivers/hid/hid-core.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 8992e3c1e769..0886868ee176 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -95,7 +95,7 @@ static struct hid_field *hid_register_field(struct hid_report *report, unsigned
return NULL;
}
- field = kzalloc((sizeof(struct hid_field) +
+ field = kvzalloc((sizeof(struct hid_field) +
usages * sizeof(struct hid_usage) +
3 * usages * sizeof(unsigned int)), GFP_KERNEL);
if (!field)
@@ -661,7 +661,7 @@ static void hid_free_report(struct hid_report *report)
kfree(report->field_entries);
for (n = 0; n < report->maxfield; n++)
- kfree(report->field[n]);
+ kvfree(report->field[n]);
kfree(report);
}
--
2.25.1
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox