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@xxxxxxx>
>>
>> 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@xxxxxxx>
>> Signed-off-by: Akshata MukundShetty <akshata.mukundshetty@xxxxxxx>
>> Signed-off-by: Basavaraj Natikar <basavaraj.natikar@xxxxxxx>
>
> 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);
>
