Re: [PATCH 2/2] platform/x86/amd/pmf: Fix to Update HPD Data When ALS is Disabled

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

On 7/29/2024 17:25, Ilpo Järvinen wrote:
> On Tue, 23 Jul 2024, Shyam Sundar S K wrote:
> 
>> If the Ambient Light Sensor (ALS) is disabled, the current code in the PMF
>> driver does not query for Human Presence Detection (HPD) data in
>> amd_pmf_get_sensor_info(). As a result, stale HPD data is used by PMF-TA
>> to evaluate policy conditions, leading to unexpected behavior in the policy
>> output actions.
>>
>> To resolve this issue, modify the PMF driver to query HPD data
>> independently of ALS.
>>
>> With this change, amd_pmf_get_sensor_info() now returns void instead of
>> int.
>>
>> Fixes: cedecdba60f4 ("platform/x86/amd/pmf: Get ambient light information from AMD SFH driver")
>> Co-developed-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
>> Signed-off-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
>> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
>> ---
>>  drivers/platform/x86/amd/pmf/spc.c | 33 +++++++++++++++---------------
>>  1 file changed, 16 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/platform/x86/amd/pmf/spc.c b/drivers/platform/x86/amd/pmf/spc.c
>> index a3dec14c3004..d9e60d63553c 100644
>> --- a/drivers/platform/x86/amd/pmf/spc.c
>> +++ b/drivers/platform/x86/amd/pmf/spc.c
>> @@ -150,7 +150,7 @@ static int amd_pmf_get_slider_info(struct amd_pmf_dev *dev, struct ta_pmf_enact_
>>  	return 0;
>>  }
>>  
>> -static int amd_pmf_get_sensor_info(struct amd_pmf_dev *dev, struct ta_pmf_enact_table *in)
>> +static void amd_pmf_get_sensor_info(struct amd_pmf_dev *dev, struct ta_pmf_enact_table *in)
>>  {
>>  	struct amd_sfh_info sfh_info;
>>  	int ret;
>> @@ -160,26 +160,25 @@ static int amd_pmf_get_sensor_info(struct amd_pmf_dev *dev, struct ta_pmf_enact_
>>  	if (!ret)
>>  		in->ev_info.ambient_light = sfh_info.ambient_light;
>>  	else
>> -		return ret;
>> +		dev_dbg(dev->dev, "ALS is not enabled\n");
>>  
>>  	/* get HPD data */
>>  	ret = amd_get_sfh_info(&sfh_info, MT_HPD);
>> -	if (ret)
>> -		return ret;
>> -
>> -	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;
>> +	if (!ret) {
>> +		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;
>> +		}
>> +	} else {
>> +		dev_dbg(dev->dev, "HPD is not enabled\n");
> 
> Is it okay to leave in->ev_info.user_present as 0 this case? Should it be 
> set to same as with SFH_NOT_DETECTED?
> 

Valid point. Realized that the PMF TA only accepts a boolean and it
only looks for information on if the user is present or not.

I have adjusted this behavior in v2. Please take a look.

> ...I now realize your print out doesn't cover that case at all and 
> user_present is not used for anything else than debug printing.
> 

entire populated "ev_info" is passed to the PMF TA to evaluate the
policy conditions. So, its just not limited to debug message.

Thanks,
Shyam