| Message ID | 20250527111047.920622-1-Basavaraj.Natikar@amd.com |
|---|---|
| State | New |
| Headers | show |
| Series | HID: amd_sfh: Enable operating mode | expand |
+Denis On 5/27/2025 4:10 AM, Basavaraj Natikar wrote: > Add changes to enable operating modes in the driver to allow the FW to > activate and retrieve data from relevant sensors. This enables the FW to > take necessary actions based on the operating modes. > > 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> Comparing this to the series that was submitted by Denis [1] I notice that the main tangible difference is that this isn't exported into the HID descriptor. So how does userspace know the current operating mode with this patch? Link: https://lore.kernel.org/linux-input/20250309194934.1759953-2-benato.denis96@gmail.com/ [1] > --- > drivers/hid/amd-sfh-hid/amd_sfh_client.c | 23 +++++++++++++++++++++++ > drivers/hid/amd-sfh-hid/amd_sfh_pcie.c | 4 ++++ > drivers/hid/amd-sfh-hid/amd_sfh_pcie.h | 1 + > 3 files changed, 28 insertions(+) > > diff --git a/drivers/hid/amd-sfh-hid/amd_sfh_client.c b/drivers/hid/amd-sfh-hid/amd_sfh_client.c > index 3438d392920f..0f2cbae39b2b 100644 > --- a/drivers/hid/amd-sfh-hid/amd_sfh_client.c > +++ b/drivers/hid/amd-sfh-hid/amd_sfh_client.c > @@ -146,6 +146,8 @@ static const char *get_sensor_name(int idx) > return "gyroscope"; > case mag_idx: > return "magnetometer"; > + case op_idx: > + return "operating-mode"; > case als_idx: > case ACS_IDX: /* ambient color sensor */ > return "ALS"; > @@ -243,6 +245,20 @@ int amd_sfh_hid_client_init(struct amd_mp2_dev *privdata) > rc = -ENOMEM; > goto cleanup; > } > + > + if (cl_data->sensor_idx[i] == op_idx) { > + info.period = AMD_SFH_IDLE_LOOP; > + info.sensor_idx = cl_data->sensor_idx[i]; > + info.dma_address = cl_data->sensor_dma_addr[i]; > + mp2_ops->start(privdata, info); > + cl_data->sensor_sts[i] = amd_sfh_wait_for_response(privdata, > + cl_data->sensor_idx[i], > + SENSOR_ENABLED); > + if (cl_data->sensor_sts[i] == SENSOR_ENABLED) > + cl_data->is_any_sensor_enabled = true; > + continue; > + } > + > cl_data->sensor_sts[i] = SENSOR_DISABLED; > cl_data->sensor_requested_cnt[i] = 0; > cl_data->cur_hid_dev = i; > @@ -303,6 +319,13 @@ int amd_sfh_hid_client_init(struct amd_mp2_dev *privdata) > > for (i = 0; i < cl_data->num_hid_devices; i++) { > cl_data->cur_hid_dev = i; > + if (cl_data->sensor_idx[i] == op_idx) { > + dev_dbg(dev, "sid 0x%x (%s) status 0x%x\n", > + cl_data->sensor_idx[i], get_sensor_name(cl_data->sensor_idx[i]), > + cl_data->sensor_sts[i]); > + continue; > + } > + > if (cl_data->sensor_sts[i] == SENSOR_ENABLED) { > rc = amdtp_hid_probe(i, cl_data); > if (rc) > diff --git a/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c b/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c > index 1c1fd63330c9..2983af969579 100644 > --- a/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c > +++ b/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c > @@ -29,6 +29,7 @@ > #define ACEL_EN BIT(0) > #define GYRO_EN BIT(1) > #define MAGNO_EN BIT(2) > +#define OP_EN BIT(15) > #define HPD_EN BIT(16) > #define ALS_EN BIT(19) > #define ACS_EN BIT(22) > @@ -232,6 +233,9 @@ int amd_mp2_get_sensor_num(struct amd_mp2_dev *privdata, u8 *sensor_id) > if (MAGNO_EN & activestatus) > sensor_id[num_of_sensors++] = mag_idx; > > + if (OP_EN & activestatus) > + sensor_id[num_of_sensors++] = op_idx; > + > if (ALS_EN & activestatus) > sensor_id[num_of_sensors++] = als_idx; > > diff --git a/drivers/hid/amd-sfh-hid/amd_sfh_pcie.h b/drivers/hid/amd-sfh-hid/amd_sfh_pcie.h > index 05e400a4a83e..2eb61f4e8434 100644 > --- a/drivers/hid/amd-sfh-hid/amd_sfh_pcie.h > +++ b/drivers/hid/amd-sfh-hid/amd_sfh_pcie.h > @@ -79,6 +79,7 @@ enum sensor_idx { > accel_idx = 0, > gyro_idx = 1, > mag_idx = 2, > + op_idx = 15, > als_idx = 19 > }; >
On 6/9/2025 3:21 PM, Mario Limonciello wrote: > +Denis > > On 5/27/2025 4:10 AM, Basavaraj Natikar wrote: >> Add changes to enable operating modes in the driver to allow the FW to >> activate and retrieve data from relevant sensors. This enables the FW to >> take necessary actions based on the operating modes. >> >> 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> > > Comparing this to the series that was submitted by Denis [1] I notice > that the main tangible difference is that this isn't exported into the > HID descriptor. So how does userspace know the current operating mode > with this patch? > > Link: https://lore.kernel.org/linux-input/20250309194934.1759953-2- > benato.denis96@gmail.com/ [1] Recently a bug report came in: https://bugzilla.kernel.org/show_bug.cgi?id=220224 It was very weird because it advertised a tablet mode switch which I believe comes from the patch that I linked being added to a downstream kernel. So there appears to be problems with the descriptor from that patch. Considering this, I think what you're doing is fine for now; appears firmware is notified on tablet mode. If there is a need for userspace to react to sfh then some variation of a descriptor will be needed at that point. Reviewed-by: Mario Limonciello <mario.limonciello@amd.com> > >> --- >> drivers/hid/amd-sfh-hid/amd_sfh_client.c | 23 +++++++++++++++++++++++ >> drivers/hid/amd-sfh-hid/amd_sfh_pcie.c | 4 ++++ >> drivers/hid/amd-sfh-hid/amd_sfh_pcie.h | 1 + >> 3 files changed, 28 insertions(+) >> >> diff --git a/drivers/hid/amd-sfh-hid/amd_sfh_client.c b/drivers/hid/ >> amd-sfh-hid/amd_sfh_client.c >> index 3438d392920f..0f2cbae39b2b 100644 >> --- a/drivers/hid/amd-sfh-hid/amd_sfh_client.c >> +++ b/drivers/hid/amd-sfh-hid/amd_sfh_client.c >> @@ -146,6 +146,8 @@ static const char *get_sensor_name(int idx) >> return "gyroscope"; >> case mag_idx: >> return "magnetometer"; >> + case op_idx: >> + return "operating-mode"; >> case als_idx: >> case ACS_IDX: /* ambient color sensor */ >> return "ALS"; >> @@ -243,6 +245,20 @@ int amd_sfh_hid_client_init(struct amd_mp2_dev >> *privdata) >> rc = -ENOMEM; >> goto cleanup; >> } >> + >> + if (cl_data->sensor_idx[i] == op_idx) { >> + info.period = AMD_SFH_IDLE_LOOP; >> + info.sensor_idx = cl_data->sensor_idx[i]; >> + info.dma_address = cl_data->sensor_dma_addr[i]; >> + mp2_ops->start(privdata, info); >> + cl_data->sensor_sts[i] = amd_sfh_wait_for_response(privdata, >> + cl_data->sensor_idx[i], >> + SENSOR_ENABLED); >> + if (cl_data->sensor_sts[i] == SENSOR_ENABLED) >> + cl_data->is_any_sensor_enabled = true; >> + continue; >> + } >> + >> cl_data->sensor_sts[i] = SENSOR_DISABLED; >> cl_data->sensor_requested_cnt[i] = 0; >> cl_data->cur_hid_dev = i; >> @@ -303,6 +319,13 @@ int amd_sfh_hid_client_init(struct amd_mp2_dev >> *privdata) >> for (i = 0; i < cl_data->num_hid_devices; i++) { >> cl_data->cur_hid_dev = i; >> + if (cl_data->sensor_idx[i] == op_idx) { >> + dev_dbg(dev, "sid 0x%x (%s) status 0x%x\n", >> + cl_data->sensor_idx[i], get_sensor_name(cl_data- >> >sensor_idx[i]), >> + cl_data->sensor_sts[i]); >> + continue; >> + } >> + >> if (cl_data->sensor_sts[i] == SENSOR_ENABLED) { >> rc = amdtp_hid_probe(i, cl_data); >> if (rc) >> diff --git a/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c b/drivers/hid/amd- >> sfh-hid/amd_sfh_pcie.c >> index 1c1fd63330c9..2983af969579 100644 >> --- a/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c >> +++ b/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c >> @@ -29,6 +29,7 @@ >> #define ACEL_EN BIT(0) >> #define GYRO_EN BIT(1) >> #define MAGNO_EN BIT(2) >> +#define OP_EN BIT(15) >> #define HPD_EN BIT(16) >> #define ALS_EN BIT(19) >> #define ACS_EN BIT(22) >> @@ -232,6 +233,9 @@ int amd_mp2_get_sensor_num(struct amd_mp2_dev >> *privdata, u8 *sensor_id) >> if (MAGNO_EN & activestatus) >> sensor_id[num_of_sensors++] = mag_idx; >> + if (OP_EN & activestatus) >> + sensor_id[num_of_sensors++] = op_idx; >> + >> if (ALS_EN & activestatus) >> sensor_id[num_of_sensors++] = als_idx; >> diff --git a/drivers/hid/amd-sfh-hid/amd_sfh_pcie.h b/drivers/hid/amd- >> sfh-hid/amd_sfh_pcie.h >> index 05e400a4a83e..2eb61f4e8434 100644 >> --- a/drivers/hid/amd-sfh-hid/amd_sfh_pcie.h >> +++ b/drivers/hid/amd-sfh-hid/amd_sfh_pcie.h >> @@ -79,6 +79,7 @@ enum sensor_idx { >> accel_idx = 0, >> gyro_idx = 1, >> mag_idx = 2, >> + op_idx = 15, >> als_idx = 19 >> }; > >
On 6/13/25 21:46, Mario Limonciello wrote: > On 6/9/2025 3:21 PM, Mario Limonciello wrote: >> +Denis >> >> On 5/27/2025 4:10 AM, Basavaraj Natikar wrote: >>> Add changes to enable operating modes in the driver to allow the FW to >>> activate and retrieve data from relevant sensors. This enables the FW to >>> take necessary actions based on the operating modes. >>> >>> 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> >> >> Comparing this to the series that was submitted by Denis [1] I notice that the main tangible difference is that this isn't exported into the HID descriptor. So how does userspace know the current operating mode with this patch? >> >> Link: https://lore.kernel.org/linux-input/20250309194934.1759953-2- benato.denis96@gmail.com/ [1] > > Recently a bug report came in: https://bugzilla.kernel.org/show_bug.cgi?id=220224 > > It was very weird because it advertised a tablet mode switch which I believe comes from the patch that I linked being added to a downstream kernel. So there appears to be problems with the descriptor from that patch. > > Considering this, I think what you're doing is fine for now; appears firmware is notified on tablet mode. Unfortunately this version of the patch is also causing problems for the user above. Attached is their journal log with the kernel traces. See [1] for the GitHub issue.
On 6/14/25 1:51 AM, Eric Naim wrote: > On 6/13/25 21:46, Mario Limonciello wrote: >> On 6/9/2025 3:21 PM, Mario Limonciello wrote: >>> +Denis >>> >>> On 5/27/2025 4:10 AM, Basavaraj Natikar wrote: >>>> Add changes to enable operating modes in the driver to allow the FW to >>>> activate and retrieve data from relevant sensors. This enables the FW to >>>> take necessary actions based on the operating modes. >>>> >>>> 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> >>> >>> Comparing this to the series that was submitted by Denis [1] I notice that the main tangible difference is that this isn't exported into the HID descriptor. So how does userspace know the current operating mode with this patch? >>> >>> Link: https://lore.kernel.org/linux-input/20250309194934.1759953-2- benato.denis96@gmail.com/ [1] >> >> Recently a bug report came in: https://bugzilla.kernel.org/show_bug.cgi?id=220224 >> >> It was very weird because it advertised a tablet mode switch which I believe comes from the patch that I linked being added to a downstream kernel. So there appears to be problems with the descriptor from that patch. >> >> Considering this, I think what you're doing is fine for now; appears firmware is notified on tablet mode. > > Unfortunately this version of the patch is also causing problems for the user above. Attached is their journal log with the kernel traces. See [1] for the GitHub issue. > I looked through the patch and I have a theory on the problem. Is the issue that MAX_HID_DEVICES wasn't incremented? Because previously there was only "6" kinda before and now this is a 7th. This incremental diff would fix it if so. diff --git a/drivers/hid/amd-sfh-hid/amd_sfh_hid.h b/drivers/hid/amd-sfh-hid/amd_sfh_hid.h index 1c91be8daeddf..7452b03029538 100644 --- a/drivers/hid/amd-sfh-hid/amd_sfh_hid.h +++ b/drivers/hid/amd-sfh-hid/amd_sfh_hid.h @@ -11,7 +11,7 @@ #ifndef AMDSFH_HID_H #define AMDSFH_HID_H -#define MAX_HID_DEVICES 6 +#define MAX_HID_DEVICES 7 #define AMD_SFH_HID_VENDOR 0x1022 #define AMD_SFH_HID_PRODUCT 0x0001
diff --git a/drivers/hid/amd-sfh-hid/amd_sfh_client.c b/drivers/hid/amd-sfh-hid/amd_sfh_client.c index 3438d392920f..0f2cbae39b2b 100644 --- a/drivers/hid/amd-sfh-hid/amd_sfh_client.c +++ b/drivers/hid/amd-sfh-hid/amd_sfh_client.c @@ -146,6 +146,8 @@ static const char *get_sensor_name(int idx) return "gyroscope"; case mag_idx: return "magnetometer"; + case op_idx: + return "operating-mode"; case als_idx: case ACS_IDX: /* ambient color sensor */ return "ALS"; @@ -243,6 +245,20 @@ int amd_sfh_hid_client_init(struct amd_mp2_dev *privdata) rc = -ENOMEM; goto cleanup; } + + if (cl_data->sensor_idx[i] == op_idx) { + info.period = AMD_SFH_IDLE_LOOP; + info.sensor_idx = cl_data->sensor_idx[i]; + info.dma_address = cl_data->sensor_dma_addr[i]; + mp2_ops->start(privdata, info); + cl_data->sensor_sts[i] = amd_sfh_wait_for_response(privdata, + cl_data->sensor_idx[i], + SENSOR_ENABLED); + if (cl_data->sensor_sts[i] == SENSOR_ENABLED) + cl_data->is_any_sensor_enabled = true; + continue; + } + cl_data->sensor_sts[i] = SENSOR_DISABLED; cl_data->sensor_requested_cnt[i] = 0; cl_data->cur_hid_dev = i; @@ -303,6 +319,13 @@ int amd_sfh_hid_client_init(struct amd_mp2_dev *privdata) for (i = 0; i < cl_data->num_hid_devices; i++) { cl_data->cur_hid_dev = i; + if (cl_data->sensor_idx[i] == op_idx) { + dev_dbg(dev, "sid 0x%x (%s) status 0x%x\n", + cl_data->sensor_idx[i], get_sensor_name(cl_data->sensor_idx[i]), + cl_data->sensor_sts[i]); + continue; + } + if (cl_data->sensor_sts[i] == SENSOR_ENABLED) { rc = amdtp_hid_probe(i, cl_data); if (rc) diff --git a/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c b/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c index 1c1fd63330c9..2983af969579 100644 --- a/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c +++ b/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c @@ -29,6 +29,7 @@ #define ACEL_EN BIT(0) #define GYRO_EN BIT(1) #define MAGNO_EN BIT(2) +#define OP_EN BIT(15) #define HPD_EN BIT(16) #define ALS_EN BIT(19) #define ACS_EN BIT(22) @@ -232,6 +233,9 @@ int amd_mp2_get_sensor_num(struct amd_mp2_dev *privdata, u8 *sensor_id) if (MAGNO_EN & activestatus) sensor_id[num_of_sensors++] = mag_idx; + if (OP_EN & activestatus) + sensor_id[num_of_sensors++] = op_idx; + if (ALS_EN & activestatus) sensor_id[num_of_sensors++] = als_idx; diff --git a/drivers/hid/amd-sfh-hid/amd_sfh_pcie.h b/drivers/hid/amd-sfh-hid/amd_sfh_pcie.h index 05e400a4a83e..2eb61f4e8434 100644 --- a/drivers/hid/amd-sfh-hid/amd_sfh_pcie.h +++ b/drivers/hid/amd-sfh-hid/amd_sfh_pcie.h @@ -79,6 +79,7 @@ enum sensor_idx { accel_idx = 0, gyro_idx = 1, mag_idx = 2, + op_idx = 15, als_idx = 19 };