Re: [PATCH v6 14/15] platform/x86/amd/pmf: Add PMF-AMDSFH interface for HPD
From: Hans de Goede <hidden>
Date: 2023-12-11 09:07:28
Also in:
platform-driver-x86
Hi, On 12/4/23 11:15, 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_init.c | 14 ++++++ .../amd-sfh-hid/sfh1_1/amd_sfh_interface.c | 33 +++++++++++++ .../amd-sfh-hid/sfh1_1/amd_sfh_interface.h | 1 + drivers/platform/x86/amd/pmf/Kconfig | 1 + drivers/platform/x86/amd/pmf/spc.c | 22 +++++++++ include/linux/amd-pmf-io.h | 46 +++++++++++++++++++ 7 files changed, 122 insertions(+) create mode 100644 include/linux/amd-pmf-io.h
<snip>
quoted hunk ↗ jump to hunk
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..f8758fb70b1a 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;@@ -73,7 +76,37 @@ static struct amd_mp2_ops amd_sfh_ops = { .response = amd_sfh_wait_response, }; +void sfh_deinit_emp2(void) +{ + emp2 = NULL; +} + 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) +{ + struct hpd_status hpdstatus; + + if (!emp2 || !emp2->dev_en.is_hpd_present) + return -ENODEV; + + hpdstatus.val = readl(emp2->mmio + AMD_C2P_MSG(4)); + *user_present = hpdstatus.shpd.presence; + return 0; +} + +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 -EINVAL; } +EXPORT_SYMBOL_GPL(amd_get_sfh_info);
This whole amd_get_sfh_info() interface seems over engineered why not just export amd_sfh_hpd_info() itself and directly use that in the pmf code ? That seems a whole lot simpler to me. Also this patch MUST be split into 2 separate patches for the HID and the PMF changes. <snip>
quoted hunk ↗ jump to hunk
diff --git a/drivers/platform/x86/amd/pmf/spc.c b/drivers/platform/x86/amd/pmf/spc.c index a0423942f771..5e769dcb075a 100644 --- a/drivers/platform/x86/amd/pmf/spc.c +++ b/drivers/platform/x86/amd/pmf/spc.c@@ -10,6 +10,7 @@ */ #include <acpi/button.h> +#include <linux/amd-pmf-io.h> #include <linux/power_supply.h> #include <linux/units.h> #include "pmf.h"@@ -44,6 +45,7 @@ void amd_pmf_dump_ta_inputs(struct amd_pmf_dev *dev, struct ta_pmf_enact_table * dev_dbg(dev->dev, "Max C0 Residency: %u\n", in->ev_info.max_c0residency); dev_dbg(dev->dev, "GFX Busy: %u\n", in->ev_info.gfx_busy); 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@@ -147,6 +149,25 @@ static int amd_pmf_get_slider_info(struct amd_pmf_dev *dev, struct ta_pmf_enact_ return 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);
As Mario also pointed out, this needs to be error checked. Regards, Hans