Hi,
On 12/4/23 11:15, Shyam Sundar S K wrote:
> From: Basavaraj Natikar <Basavaraj.Natikar@xxxxxxx>
>
> 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@xxxxxxx>
> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@xxxxxxx>
> Signed-off-by: Basavaraj Natikar <Basavaraj.Natikar@xxxxxxx>
> ---
> 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>
> 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@xxxxxxx>
> */
> +#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>
> 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
