Re: [PATCH 1/2] HID: amd_sfh: Add support to export device operating states

[Shyam Sundar S K <Shyam-sundar.S-k@amd.com>]

On 12/12/2024 21:16, Mario Limonciello wrote:
> On 12/12/2024 09:19, Shyam Sundar S K wrote:
>> From: Basavaraj Natikar <basavaraj.natikar@amd.com>
>>
>> Add support to export device operating states, such as laptop
>> placement,
>> platform types and propagate this data to AMD PMF driver for use in
>> actions.
>>
>> To retrieve the device operating states data, SRA sensor support
>> need to
>> be enabled in AMD SFH driver. So add support to enable the SRA sensor.
>>
>> Co-developed-by: Akshata MukundShetty <akshata.mukundshetty@amd.com>
>> Signed-off-by: Akshata MukundShetty <akshata.mukundshetty@amd.com>
>> Signed-off-by: Basavaraj Natikar <basavaraj.natikar@amd.com>
> 
> When you send someone else's patch but don't change it you are still
> supposed to add your "own" S-o-b.

ah! Thanks. I missed to add it.

> 
> I have two small nits below.
> 

Sure, but I have a question to Hans and Ilpo

while we address the remarks what should be approach for merging this
series? Should it go via pdx86 tree or hid because patch 2/2 is
dependent of 1/2.

Thanks,
Shyam
>> ---
>>   drivers/hid/amd-sfh-hid/amd_sfh_common.h      |  1 +
>>   drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_init.c | 22 ++++++++++++
>>   .../amd-sfh-hid/sfh1_1/amd_sfh_interface.c    | 35 +++++++++++++++
>> ++++
>>   .../amd-sfh-hid/sfh1_1/amd_sfh_interface.h    | 20 +++++++++++
>>   include/linux/amd-pmf-io.h                    | 15 ++++++++
>>   5 files changed, 93 insertions(+)
>>
>> diff --git a/drivers/hid/amd-sfh-hid/amd_sfh_common.h b/drivers/hid/
>> amd-sfh-hid/amd_sfh_common.h
>> index e5620d7db569..799b8686a88a 100644
>> --- a/drivers/hid/amd-sfh-hid/amd_sfh_common.h
>> +++ b/drivers/hid/amd-sfh-hid/amd_sfh_common.h
>> @@ -43,6 +43,7 @@ struct amd_mp2_sensor_info {
>>   struct sfh_dev_status {
>>       bool is_hpd_present;
>>       bool is_als_present;
>> +    bool is_sra_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 db36d87d5634..03c028f1aab4 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
>> @@ -30,6 +30,7 @@ static int amd_sfh_get_sensor_num(struct
>> amd_mp2_dev *mp2, u8 *sensor_id)
>>           case ACCEL_IDX:
>>           case GYRO_IDX:
>>           case MAG_IDX:
>> +        case SRA_IDX:
>>           case ALS_IDX:
>>           case HPD_IDX:
>>               if (BIT(i) & slist->sl.sensors)
>> @@ -58,6 +59,8 @@ static const char *get_sensor_name(int idx)
>>           return "gyroscope";
>>       case MAG_IDX:
>>           return "magnetometer";
>> +    case SRA_IDX:
>> +        return "SRA";
>>       case ALS_IDX:
>>           return "ALS";
>>       case HPD_IDX:
>> @@ -130,6 +133,23 @@ static int amd_sfh1_1_hid_client_init(struct
>> amd_mp2_dev *privdata)
>>         for (i = 0; i < cl_data->num_hid_devices; i++) {
>>           cl_data->sensor_sts[i] = SENSOR_DISABLED;
>> +
>> +        if (cl_data->num_hid_devices == 1 && cl_data->sensor_idx[0]
>> == SRA_IDX)
>> +            break;
>> +
>> +        if (cl_data->sensor_idx[i] == SRA_IDX) {
>> +            info.sensor_idx = cl_data->sensor_idx[i];
>> +            writel(0, privdata->mmio + amd_get_p2c_val(privdata, 0));
>> +            mp2_ops->start(privdata, info);
>> +            status = amd_sfh_wait_for_response
>> +                (privdata, cl_data->sensor_idx[i], ENABLE_SENSOR);
>> +
>> +            cl_data->sensor_sts[i] = (status == 0) ?
>> SENSOR_ENABLED : SENSOR_DISABLED;
>> +            if (cl_data->sensor_sts[i] == SENSOR_ENABLED)
>> +                privdata->dev_en.is_sra_present = true;
>> +            continue;
>> +        }
>> +
>>           cl_data->sensor_requested_cnt[i] = 0;
>>           cl_data->cur_hid_dev = i;
>>           cl_idx = cl_data->sensor_idx[i];
>> @@ -181,6 +201,8 @@ static int amd_sfh1_1_hid_client_init(struct
>> amd_mp2_dev *privdata)
>>       }
>>         for (i = 0; i < cl_data->num_hid_devices; i++) {
>> +        if (cl_data->sensor_idx[i] == SRA_IDX)
>> +            continue;
>>           cl_data->cur_hid_dev = i;
>>           if (cl_data->sensor_sts[i] == SENSOR_ENABLED) {
>>               cl_data->is_any_sensor_enabled = true;
>> 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 4676f060da26..b4c0d96ab152 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
>> @@ -87,6 +87,38 @@ void sfh_interface_init(struct amd_mp2_dev *mp2)
>>       emp2 = mp2;
>>   }
>>   +static int amd_sfh_mode_info(u32 *platform_type, u32
>> *laptop_placement)
>> +{
>> +    struct sfh_op_mode mode;
>> +
>> +    if (!platform_type || !laptop_placement)
>> +        return -EINVAL;
>> +
>> +    if (!emp2 || !emp2->dev_en.is_sra_present)
>> +        return -ENODEV;
>> +
>> +    mode.val = readl(emp2->mmio + amd_get_c2p_val(emp2, 3));
>> +
>> +    *platform_type = mode.op_mode.devicemode;
>> +
>> +    if (mode.op_mode.ontablestate == 1)
>> +        *laptop_placement = ON_TABLE;
>> +    else if (mode.op_mode.ontablestate == 2)
>> +        *laptop_placement = ON_LAP_MOTION;
>> +    else if (mode.op_mode.inbagstate == 1)
>> +        *laptop_placement = IN_BAG;
>> +    else if (mode.op_mode.outbagstate == 1)
>> +        *laptop_placement = OUT_OF_BAG;
>> +    else if (mode.op_mode.ontablestate == 0 ||
>> mode.op_mode.inbagstate == 0 ||
>> +         mode.op_mode.outbagstate == 0)
>> +        *laptop_placement = LP_UNKNOWN;
>> +    else if (mode.op_mode.ontablestate == 3 ||
>> mode.op_mode.inbagstate == 3 ||
>> +         mode.op_mode.outbagstate == 3)
>> +        *laptop_placement = LP_UNDEFINED;
> 
> What do you think of doing a pr_warn_once() when you end up with an
> undefined placement?  This could help point out that the driver needs
> to be changed for a newly created mode that the hardware detected.
> 
>> +
>> +    return 0;
>> +}
>> +
>>   static int amd_sfh_hpd_info(u8 *user_present)
>>   {
>>       struct hpd_status hpdstatus;
>> @@ -131,6 +163,9 @@ int amd_get_sfh_info(struct amd_sfh_info
>> *sfh_info, enum sfh_message_type op)
>>               return amd_sfh_hpd_info(&sfh_info->user_present);
>>           case MT_ALS:
>>               return amd_sfh_als_info(&sfh_info->ambient_light);
>> +        case MT_SRA:
>> +            return amd_sfh_mode_info(&sfh_info->platform_type,
>> +                         &sfh_info->laptop_placement);
>>           }
>>       }
>>       return -EINVAL;
>> 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 2c211d28764d..f7eb2539bccc 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
>> @@ -22,6 +22,7 @@ enum sensor_index {
>>       ACCEL_IDX,
>>       GYRO_IDX,
>>       MAG_IDX,
>> +    SRA_IDX,
>>       ALS_IDX = 4,
>>       HPD_IDX = 5,
> 
> IIRC in C enums start at 0 right?  So ALS_IDX and HPD_IDX don't need
> explicit assingments anymore.
> 
>>       MAX_IDX = 15,
>> @@ -164,6 +165,25 @@ struct hpd_status {
>>       };
>>   };
>>   +struct sfh_op_mode {
>> +    union {
>> +        u32 val;
>> +        struct {
>> +            u32 mode        : 3;
>> +            u32 lidstatus        : 1;
>> +            u32 angle        : 10;
>> +            u32 inbagstatedbg    : 2;
>> +            u32 ontablestate    : 2;
>> +            u32 inbagstate        : 2;
>> +            u32 outbagstate        : 2;
>> +            u32 inbagmlcstate    : 1;
>> +            u32 powerstate        : 2;
>> +            u32 data        : 3;
>> +            u32 devicemode        : 4;
>> +        } op_mode;
>> +    };
>> +};
>> +
>>   void sfh_interface_init(struct amd_mp2_dev *mp2);
>>   void sfh_deinit_emp2(void);
>>   void amd_sfh1_1_set_desc_ops(struct amd_mp2_ops *mp2_ops);
>> diff --git a/include/linux/amd-pmf-io.h b/include/linux/amd-pmf-io.h
>> index b4f818205216..01f2b12c56a6 100644
>> --- a/include/linux/amd-pmf-io.h
>> +++ b/include/linux/amd-pmf-io.h
>> @@ -18,10 +18,12 @@
>>    * enum sfh_message_type - Query the SFH message type
>>    * @MT_HPD: Message ID to know the Human presence info from MP2 FW
>>    * @MT_ALS: Message ID to know the Ambient light info from MP2 FW
>> + * @MT_SRA: Message ID to know the SRA data from MP2 FW
>>    */
>>   enum sfh_message_type {
>>       MT_HPD,
>>       MT_ALS,
>> +    MT_SRA,
>>   };
>>     /**
>> @@ -40,10 +42,23 @@ enum sfh_hpd_info {
>>    * struct amd_sfh_info - get HPD sensor info from MP2 FW
>>    * @ambient_light: Populates the ambient light information
>>    * @user_present: Populates the user presence information
>> + * @platform_type: Operating modes (clmashell, flat, tent, etc.)
>> + * @laptop_placement: Device states (ontable, onlap, outbag)
>>    */
>>   struct amd_sfh_info {
>>       u32 ambient_light;
>>       u8 user_present;
>> +    u32 platform_type;
>> +    u32 laptop_placement;
>> +};
>> +
>> +enum laptop_placement {
>> +    LP_UNKNOWN = 0,
>> +    ON_TABLE,
>> +    ON_LAP_MOTION,
>> +    IN_BAG,
>> +    OUT_OF_BAG,
>> +    LP_UNDEFINED,
>>   };
>>     int amd_get_sfh_info(struct amd_sfh_info *sfh_info, enum
>> sfh_message_type op);
>