Hi,
On 4/15/24 5:54 PM, Benjamin Tissoires wrote:
> [Ccing Hans as well for input]
>
> On Apr 13 2024, kde@xxxxxxxxxxxx wrote:
>> From: Allan Sandfeld Jensen <allan.jensen@xxxxx>
>>
>
> FWIW, this patch neesd a commit description and signed-offs
>
>> ---
>> drivers/hid/hid-ids.h | 1 +
>> drivers/hid/hid-logitech-dj.c | 10 +++++++++-
>> drivers/hid/hid-logitech-hidpp.c | 2 ++
>> 3 files changed, 12 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
>> index 2235d78784b1..4b79c4578d32 100644
>> --- a/drivers/hid/hid-ids.h
>> +++ b/drivers/hid/hid-ids.h
>> @@ -849,6 +849,7 @@
>> #define USB_DEVICE_ID_LOGITECH_NANO_RECEIVER_LIGHTSPEED_1 0xc539
>> #define USB_DEVICE_ID_LOGITECH_NANO_RECEIVER_LIGHTSPEED_1_1 0xc53f
>> #define USB_DEVICE_ID_LOGITECH_NANO_RECEIVER_POWERPLAY 0xc53a
>> +#define USB_DEVICE_ID_LOGITECH_BOLT_RECEIVER 0xc548
>> #define USB_DEVICE_ID_SPACETRAVELLER 0xc623
>> #define USB_DEVICE_ID_SPACENAVIGATOR 0xc626
>> #define USB_DEVICE_ID_DINOVO_DESKTOP 0xc704
>> diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c
>> index c358778e070b..92b41ae5a47c 100644
>> --- a/drivers/hid/hid-logitech-dj.c
>> +++ b/drivers/hid/hid-logitech-dj.c
>> @@ -120,6 +120,7 @@ enum recvr_type {
>> recvr_type_27mhz,
>> recvr_type_bluetooth,
>> recvr_type_dinovo,
>> + recvr_type_bolt,
>
> I am *really* hesitant in integrating the bolt receiver into
> logitech-dj.ko:
> - the bolt receiver is not capable of making the distinction between the
> source of the events (so only one mouse/keyboard can be used at the
> time)
> - we still have a couple of outstanding and impossible to debug issues
> with some high resolution mice connected over the unifying receiver,
> and adding one more receiver makes me nervous
> - I have a strong feeling by reading the code that the keyboard part
> will fail (there is a comment "For the keyboard, we can reuse the same
> report by using the second byte which is constant in the USB HID
> report descriptor." though I can't seem to find this constant report
> on the bolt receiver)
> - what are the benefits of adding it?
> - will it break fwupd?
FWIW I'm also not in favor of stretching drivers/hid/hid-logitech-dj.c
even further to also support the new bolt stuff.
AFAIK the new bolt stuff is significantly different.
Allan, I see in your other reply that you are mainly after
highres scrolling and since the bolt receiver does not do
per paired device addressing I wonder if you cannot just
get that by treating the bolt receiver as a wired HIDPP
device and just directly listing it as such in
hid-logitech-hidpp.c ?
The whole purpose of hid-logitech-dj.c is to create 1 virtual
hidpp devices per paired device and with bolt that is not
possible, so I think that we should circumvent hid-logitech-dj.c
for bolt and if we want to use any hidpp features do so
by directly listing the receivers in hid-logitech-hidpp.c .
Regards,
Hans
>
>> };
>>
>> struct dj_report {
>> @@ -1068,6 +1069,7 @@ static void logi_hidpp_recv_queue_notif(struct hid_device *hdev,
>> workitem.reports_supported |= STD_KEYBOARD;
>> break;
>> case 0x0f:
>> + case 0x10:
>> case 0x11:
>> device_type = "eQUAD Lightspeed 1.2";
>> logi_hidpp_dev_conn_notif_equad(hdev, hidpp_report, &workitem);
>> @@ -1430,7 +1432,8 @@ static int logi_dj_ll_parse(struct hid_device *hid)
>> dbg_hid("%s: sending a mouse descriptor, reports_supported: %llx\n",
>> __func__, djdev->reports_supported);
>> if (djdev->dj_receiver_dev->type == recvr_type_gaming_hidpp ||
>> - djdev->dj_receiver_dev->type == recvr_type_mouse_only)
>> + djdev->dj_receiver_dev->type == recvr_type_mouse_only ||
>> + djdev->dj_receiver_dev->type == recvr_type_bolt)
>> rdcat(rdesc, &rsize, mse_high_res_descriptor,
>> sizeof(mse_high_res_descriptor));
>> else if (djdev->dj_receiver_dev->type == recvr_type_27mhz)
>> @@ -1773,6 +1776,7 @@ static int logi_dj_probe(struct hid_device *hdev,
>> case recvr_type_dj: no_dj_interfaces = 3; break;
>> case recvr_type_hidpp: no_dj_interfaces = 2; break;
>> case recvr_type_gaming_hidpp: no_dj_interfaces = 3; break;
>> + case recvr_type_bolt: no_dj_interfaces = 4; break;
>
> 4? The device I have here only has 3 (unless I misremember how this is
> supposed to be working).
>
>> case recvr_type_mouse_only: no_dj_interfaces = 2; break;
>> case recvr_type_27mhz: no_dj_interfaces = 2; break;
>> case recvr_type_bluetooth: no_dj_interfaces = 2; break;
>> @@ -1950,6 +1954,10 @@ static const struct hid_device_id logi_dj_receivers[] = {
>> HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH,
>> USB_DEVICE_ID_LOGITECH_UNIFYING_RECEIVER_2),
>> .driver_data = recvr_type_dj},
>> + { /* Logitech bolt receiver (0xc548) */
>> + HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH,
>> + USB_DEVICE_ID_LOGITECH_BOLT_RECEIVER),
>> + .driver_data = recvr_type_bolt},
>>
>> { /* Logitech Nano mouse only receiver (0xc52f) */
>> HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH,
>> diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
>> index 3c00e6ac8e76..509142982daa 100644
>> --- a/drivers/hid/hid-logitech-hidpp.c
>> +++ b/drivers/hid/hid-logitech-hidpp.c
>> @@ -4380,6 +4380,8 @@ static const struct hid_device_id hidpp_devices[] = {
>> HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb023) },
>> { /* MX Master 3S mouse over Bluetooth */
>> HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb034) },
>> + { /* MX Anywhere 3SB mouse over Bluetooth */
>> + HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb038) },
>
> That I can accept, however know that there is a regression in bluez
> 0.73[0] (but it should be fixed in 0.74)
>
> Cheers,
> Benjamin
>
>
> [0] https://github.com/bluez/bluez/issues/778#issuecomment-2048870358
>
>> {}
>> };
>>
>> --
>> 2.39.2
>>
>
