| Message ID | jCmT1QOunDmSu59iO1T1xj2-WfFeGhMn_i5knEWTCoph9VH1oxjpYf3Q6CWH7BRrq1NTVYBsAVJcIgu8azAEmFZJA8PzLfH3bHBcWNbFqeY=@protonmail.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Jiri Kosina |
| Headers | show |
| Series | [v4,1/4] HID: logitech: Add MX Mice over Bluetooth | expand |
On Fri, Oct 11, 2019 at 2:57 AM Mazin Rezk <mnrzk@protonmail.com> wrote: > > On Saturday, October 5, 2019 9:04 PM, Mazin Rezk <mnrzk@protonmail.com> wrote: > > > This patch adds support for the 0x0001 (FeatureSet) feature. This feature > > is used to look up the feature ID of a feature index on a device and list > > the total count of features on the device. > > > > I also added the hidpp20_get_features function which iterates through all > > feature indexes on the device and stores a map of them in features an > > hidpp_device struct. This function runs when an HID++ 2.0 device is probed. > > Changes in the version: > - Renamed hidpp20_featureset_get_feature to > hidpp20_featureset_get_feature_id. > > - Re-ordered hidpp20_featureset_get_count and > hidpp20_featureset_get_feature_id based on their command IDs. > > - Made feature_count initialize to 0 before running hidpp20_get_features. I still need to decide whether or not we need to take this one. We historically did not do this to mimic the Windows driver at the time. However, the driver is full of quirks that could be detected instead of hardcoded thanks to this functions. So we might want to switch to auto-detection of those quirks so we can keep 'quirks' for actual defects that can't be auto-detected. But, if we want to go this route, we need to actually make use of it. So this patch should be part of a series where we use it, not added by its own. Can you drop it from the series? And maybe possibly work on a cleanup of some of the auto detection, like the HIDPP_QUIRK_HI_RES_SCROLL_X2121 which you can easily test? But this would need to happen in a second series, once this one gets merged in. Cheers, Benjamin > > Signed-off-by: Mazin Rezk <mnrzk@protonmail.com> > --- > drivers/hid/hid-logitech-hidpp.c | 90 ++++++++++++++++++++++++++++++++ > 1 file changed, 90 insertions(+) > > diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c > index a0efa8a43213..2062be922c08 100644 > --- a/drivers/hid/hid-logitech-hidpp.c > +++ b/drivers/hid/hid-logitech-hidpp.c > @@ -190,6 +190,9 @@ struct hidpp_device { > > struct hidpp_battery battery; > struct hidpp_scroll_counter vertical_wheel_counter; > + > + u16 *features; > + u8 feature_count; > }; > > /* HID++ 1.0 error codes */ > @@ -911,6 +914,82 @@ static int hidpp_root_get_protocol_version(struct hidpp_device *hidpp) > return 0; > } > > +/* -------------------------------------------------------------------------- */ > +/* 0x0001: FeatureSet */ > +/* -------------------------------------------------------------------------- */ > + > +#define HIDPP_PAGE_FEATURESET 0x0001 > + > +#define CMD_FEATURESET_GET_COUNT 0x00 > +#define CMD_FEATURESET_GET_FEATURE 0x11 > + > +static int hidpp20_featureset_get_count(struct hidpp_device *hidpp, > + u8 feature_index, u8 *count) > +{ > + struct hidpp_report response; > + int ret; > + > + ret = hidpp_send_fap_command_sync(hidpp, feature_index, > + CMD_FEATURESET_GET_COUNT, NULL, 0, &response); > + > + if (ret) > + return ret; > + > + *count = response.fap.params[0]; > + > + return ret; > +} > + > +static int hidpp20_featureset_get_feature_id(struct hidpp_device *hidpp, > + u8 featureset_index, u8 feature_index, u16 *feature_id) > +{ > + struct hidpp_report response; > + int ret; > + > + ret = hidpp_send_fap_command_sync(hidpp, featureset_index, > + CMD_FEATURESET_GET_FEATURE, &feature_index, 1, &response); > + > + if (ret) > + return ret; > + > + *feature_id = (response.fap.params[0] << 8) | response.fap.params[1]; > + > + return ret; > +} > + > +static int hidpp20_get_features(struct hidpp_device *hidpp) > +{ > + int ret; > + u8 featureset_index, featureset_type; > + u8 i; > + > + ret = hidpp_root_get_feature(hidpp, HIDPP_PAGE_FEATURESET, > + &featureset_index, &featureset_type); > + > + if (ret == -ENOENT) { > + hid_warn(hidpp->hid_dev, "Unable to retrieve feature set."); > + return 0; > + } > + > + if (ret) > + return ret; > + > + ret = hidpp20_featureset_get_count(hidpp, featureset_index, > + &hidpp->feature_count); > + > + if (ret) > + return ret; > + > + hidpp->features = devm_kzalloc(&hidpp->hid_dev->dev, > + hidpp->feature_count * sizeof(u16), GFP_KERNEL); > + > + for (i = 0; i < hidpp->feature_count && !ret; i++) > + ret = hidpp20_featureset_get_feature_id(hidpp, featureset_index, > + i, &(hidpp->features[i])); > + > + return ret; > +} > + > /* -------------------------------------------------------------------------- */ > /* 0x0005: GetDeviceNameType */ > /* -------------------------------------------------------------------------- */ > @@ -3625,6 +3704,17 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id) > hidpp_overwrite_name(hdev); > } > > + /* Cache feature indexes and IDs to check reports faster */ > + hidpp->feature_count = 0; > + > + if (hidpp->protocol_major >= 2) { > + if (hidpp20_get_features(hidpp)) { > + hid_err(hdev, "%s:hidpp20_get_features returned error\n", > + __func__); > + goto hid_hw_init_fail; > + } > + } > + > if (connected && (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP)) { > ret = wtp_get_config(hidpp); > if (ret) > -- > 2.23.0
On Fri, Oct 11, 2019 at 10:33 AM Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote: > > On Fri, Oct 11, 2019 at 2:57 AM Mazin Rezk <mnrzk@protonmail.com> wrote: > > > > On Saturday, October 5, 2019 9:04 PM, Mazin Rezk <mnrzk@protonmail.com> wrote: > > > > > This patch adds support for the 0x0001 (FeatureSet) feature. This feature > > > is used to look up the feature ID of a feature index on a device and list > > > the total count of features on the device. > > > > > > I also added the hidpp20_get_features function which iterates through all > > > feature indexes on the device and stores a map of them in features an > > > hidpp_device struct. This function runs when an HID++ 2.0 device is probed. > > > > Changes in the version: > > - Renamed hidpp20_featureset_get_feature to > > hidpp20_featureset_get_feature_id. > > > > - Re-ordered hidpp20_featureset_get_count and > > hidpp20_featureset_get_feature_id based on their command IDs. > > > > - Made feature_count initialize to 0 before running hidpp20_get_features. > > I still need to decide whether or not we need to take this one. We > historically did not do this to mimic the Windows driver at the time. > However, the driver is full of quirks that could be detected instead > of hardcoded thanks to this functions. So we might want to switch to > auto-detection of those quirks so we can keep 'quirks' for actual > defects that can't be auto-detected. > > But, if we want to go this route, we need to actually make use of it. > So this patch should be part of a series where we use it, not added by > its own. > > Can you drop it from the series? > And maybe possibly work on a cleanup of some of the auto detection, > like the HIDPP_QUIRK_HI_RES_SCROLL_X2121 which you can easily test? > But this would need to happen in a second series, once this one gets > merged in. While reviewing 4/4, I realized why you need this one. See my comments in 4/4, I still believe the case is not strong enough to spend some time to enable the feature for barely no gain. Cheers, Benjamin
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c index a0efa8a43213..2062be922c08 100644 --- a/drivers/hid/hid-logitech-hidpp.c +++ b/drivers/hid/hid-logitech-hidpp.c @@ -190,6 +190,9 @@ struct hidpp_device { struct hidpp_battery battery; struct hidpp_scroll_counter vertical_wheel_counter; + + u16 *features; + u8 feature_count; }; /* HID++ 1.0 error codes */ @@ -911,6 +914,82 @@ static int hidpp_root_get_protocol_version(struct hidpp_device *hidpp) return 0; } +/* -------------------------------------------------------------------------- */ +/* 0x0001: FeatureSet */ +/* -------------------------------------------------------------------------- */ + +#define HIDPP_PAGE_FEATURESET 0x0001 + +#define CMD_FEATURESET_GET_COUNT 0x00 +#define CMD_FEATURESET_GET_FEATURE 0x11 + +static int hidpp20_featureset_get_count(struct hidpp_device *hidpp, + u8 feature_index, u8 *count) +{ + struct hidpp_report response; + int ret; + + ret = hidpp_send_fap_command_sync(hidpp, feature_index, + CMD_FEATURESET_GET_COUNT, NULL, 0, &response); + + if (ret) + return ret; + + *count = response.fap.params[0]; + + return ret; +} + +static int hidpp20_featureset_get_feature_id(struct hidpp_device *hidpp, + u8 featureset_index, u8 feature_index, u16 *feature_id) +{ + struct hidpp_report response; + int ret; + + ret = hidpp_send_fap_command_sync(hidpp, featureset_index, + CMD_FEATURESET_GET_FEATURE, &feature_index, 1, &response); + + if (ret) + return ret; + + *feature_id = (response.fap.params[0] << 8) | response.fap.params[1]; + + return ret; +} + +static int hidpp20_get_features(struct hidpp_device *hidpp) +{ + int ret; + u8 featureset_index, featureset_type; + u8 i; + + ret = hidpp_root_get_feature(hidpp, HIDPP_PAGE_FEATURESET, + &featureset_index, &featureset_type); + + if (ret == -ENOENT) { + hid_warn(hidpp->hid_dev, "Unable to retrieve feature set."); + return 0; + } + + if (ret) + return ret; + + ret = hidpp20_featureset_get_count(hidpp, featureset_index, + &hidpp->feature_count); + + if (ret) + return ret; + + hidpp->features = devm_kzalloc(&hidpp->hid_dev->dev, + hidpp->feature_count * sizeof(u16), GFP_KERNEL); + + for (i = 0; i < hidpp->feature_count && !ret; i++) + ret = hidpp20_featureset_get_feature_id(hidpp, featureset_index, + i, &(hidpp->features[i])); + + return ret; +} + /* -------------------------------------------------------------------------- */ /* 0x0005: GetDeviceNameType */ /* -------------------------------------------------------------------------- */ @@ -3625,6 +3704,17 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id) hidpp_overwrite_name(hdev); } + /* Cache feature indexes and IDs to check reports faster */ + hidpp->feature_count = 0; + + if (hidpp->protocol_major >= 2) { + if (hidpp20_get_features(hidpp)) { + hid_err(hdev, "%s:hidpp20_get_features returned error\n", + __func__); + goto hid_hw_init_fail; + } + } + if (connected && (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP)) { ret = wtp_get_config(hidpp); if (ret)