| Message ID | nZMYgsXB3gdFVoIR3TeMjdbHiP4STlPINtmdH7TkH-nLrHS5APVXn00Z-L89Bjnam4_EBf1GLqI5KAZDZhFnH9hyWGyCOGJQKZzpyN2tqlE=@protonmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Jiri Kosina |
| Headers | show |
| Series | [v3,1/4] HID: logitech: Add MX Mice over Bluetooth | expand |
On Sun, 2019-10-06 at 01:04 +0000, Mazin Rezk 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. > > Signed-off-by: Mazin Rezk <mnrzk@protonmail.com> > --- > drivers/hid/hid-logitech-hidpp.c | 92 ++++++++++++++++++++++++++++++++ > 1 file changed, 92 insertions(+) > > diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c > index a0efa8a43213..64ac94c581aa 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,84 @@ 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_feature(struct hidpp_device *hidpp, Can you change this to `hidpp20_featureset_get_feature_id` please? So that we keep in sync with the documentation, and to avoid minor confusion as IRoot has a `get_feature` function. > + 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_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; > +} Just a nitpick but can we put this before `hidpp20_featureset_get_feature`? This way we keep the ID order. > + > +static int hidpp20_get_features(struct hidpp_device *hidpp) > +{ > + int ret; > + u8 featureset_index, featureset_type; > + u8 i; > + > + hidpp->feature_count = 0; > + > + 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(hidpp, featureset_index, > + i, &(hidpp->features[i])); > + > + return ret; > +} > + > /* -------------------------------------------------------------------------- */ > /* 0x0005: GetDeviceNameType */ > /* -------------------------------------------------------------------------- */ Please use `DeviceNameType` here to keep in sync with the documentation. > @@ -3625,6 +3706,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 */ > + 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; > + } > + } else { > + hidpp->feature_count = 0; > + } I have not looked at the whole code that much but is the else really needed here? > + > if (connected && (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP)) { > ret = wtp_get_config(hidpp); > if (ret) > -- > 2.23.0 >
On Sunday, October 6, 2019 11:25 AM, Filipe Laíns <lains@archlinux.org> wrote: > On Sun, 2019-10-06 at 01:04 +0000, Mazin Rezk 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. > > > > Signed-off-by: Mazin Rezk <mnrzk@protonmail.com> > > --- > > drivers/hid/hid-logitech-hidpp.c | 92 ++++++++++++++++++++++++++++++++ > > 1 file changed, 92 insertions(+) > > > > diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c > > index a0efa8a43213..64ac94c581aa 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,84 @@ 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_feature(struct hidpp_device *hidpp, > > Can you change this to `hidpp20_featureset_get_feature_id` please? So > that we keep in sync with the documentation, and to avoid minor > confusion as IRoot has a `get_feature` function. I will change this in v4, thanks. > > > + 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_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; > > +} > > Just a nitpick but can we put this before > `hidpp20_featureset_get_feature`? This way we keep the ID order. That makes sense. I will change this in v4, thanks. > > > + > > +static int hidpp20_get_features(struct hidpp_device *hidpp) > > +{ > > + int ret; > > + u8 featureset_index, featureset_type; > > + u8 i; > > + > > + hidpp->feature_count = 0; > > + > > + 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(hidpp, featureset_index, > > + i, &(hidpp->features[i])); > > + > > + return ret; > > +} > > + > > /* -------------------------------------------------------------------------- */ > > /* 0x0005: GetDeviceNameType */ > > /* -------------------------------------------------------------------------- */ > > Please use `DeviceNameType` here to keep in sync with the > documentation. Since I have not modified GetDeviceNameType in this patch, I will keep it the way it was for now. This could probably be changed in a different and unrelated patch. > > > @@ -3625,6 +3706,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 */ > > + 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; > > + } > > + } else { > > + hidpp->feature_count = 0; > > + } > > I have not looked at the whole code that much but is the else really > needed here? I wanted to initialize feature_count to 0 if the device was either HID++ 1.0 or did not support FeatureSet. This was so that, just in case its features array was accessed, it would not try to check an uninitialized array. Although, I could probably remove the else statement and set feature_count to 0 before the if statement. I would also be able to remove the redundant initialization statement in hidpp20_get_features. I will make these changes in v4, thanks. > > > + > > if (connected && (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP)) { > > ret = wtp_get_config(hidpp); > > if (ret) > > -- > > 2.23.0 > > > -- > Filipe Laíns > 3DCE 51D6 0930 EBA4 7858 BA41 46F6 33CB B0EB 4BF2
On Sun, 2019-10-06 at 19:29 +0000, Mazin Rezk wrote: > > > /* ----------------------------------------------------------- > > > --------------- */ > > > /* 0x0005: > > > GetDeviceNameType > > > */ > > > /* ----------------------------------------------------------- > > > --------------- */ > > > > Please use `DeviceNameType` here to keep in sync with the > > documentation. > > Since I have not modified GetDeviceNameType in this patch, I will > keep it > the way it was for now. This could probably be changed in a different > and > unrelated patch. Ah, sorry. On my quick look it seemed to be included in the patch :facepalm:. Thanks, Filipe Laíns
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c index a0efa8a43213..64ac94c581aa 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,84 @@ 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_feature(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_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_get_features(struct hidpp_device *hidpp) +{ + int ret; + u8 featureset_index, featureset_type; + u8 i; + + hidpp->feature_count = 0; + + 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(hidpp, featureset_index, + i, &(hidpp->features[i])); + + return ret; +} + /* -------------------------------------------------------------------------- */ /* 0x0005: GetDeviceNameType */ /* -------------------------------------------------------------------------- */ @@ -3625,6 +3706,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 */ + 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; + } + } else { + hidpp->feature_count = 0; + } + if (connected && (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP)) { ret = wtp_get_config(hidpp); if (ret)
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. Signed-off-by: Mazin Rezk <mnrzk@protonmail.com> --- drivers/hid/hid-logitech-hidpp.c | 92 ++++++++++++++++++++++++++++++++ 1 file changed, 92 insertions(+) -- 2.23.0