[PATCH 14/15] platform/x86/amd/pmf: Add PMF-AMDSFH interface for HPD

Mario Limonciello mario.limonciello at amd.com
Fri Sep 22 21:04:32 UTC 2023


On 9/22/2023 14:04, Mario Limonciello wrote:
> On 9/22/2023 12:50, Shyam Sundar S K wrote:
>> From: Basavaraj Natikar <Basavaraj.Natikar at 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 at amd.com>
>> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k at amd.com>
>> Signed-off-by: Basavaraj Natikar <Basavaraj.Natikar at 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(-)

Somthing else I noticed about this change.

I believe you should be adding to Kconfig:
depends on AMD_SFH_HID

>>
>> 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)
>>   {
>>       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;
> 
> Looks like this is missing a tab.
> 
>> +                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 at 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;
> 
> Extra whitespace here.
> 
>> +}
>> +

IMO you should add some kernel doc here or in the header as good 
practice for exported symbols.

>> +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;
> 
> This looks like it should probably be -EINVAL.
> 
>>   }
>> +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);
> 
> amd_get_sfh_info() is int.
> 
> Shouldn't you look for the return code here?
> 
>> +    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 at amd.com>
>> + * Authors: Shyam Sundar S K <Shyam-sundar.S-k at amd.com>
>> + *          Basavaraj Natikar <Basavaraj.Natikar at 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 */
>> +};
>> +
>> +int amd_get_sfh_info(struct amd_sfh_info *sfh_info, enum 
>> sfh_message_type op);
>> +
>>   #endif
> 



More information about the amd-gfx mailing list