On Thu, 12 Dec 2024, Shyam Sundar S K wrote:
> 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.
Hi,
Given pdx86 pmf driver gets much more changes overall, it would seem
better to merge the series through pdx86 tree. But I also want to mention
that generally it's also possible to make requests on merge path as the
submitter of the series, in particular, it is good to take into account
if you know there are patches that might conflict with the changes
(within this kernel cycle) to make the merge window less problematic for
maintainers.
[In some cases it's possible to create an immutable branch which can be
merged by two (or more) subsystems but I don't think it provides added
value here given how low traffic amd-sfh-hid is.]
--
i.
> >> ---
> >> 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);
> >
>
