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

[Mario Limonciello <mario.limonciello@amd.com>]

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.

I have two small nits below.

> ---
>   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);