Received-SPF: pass (google.com: domain of linux-kernel+bounces-145575-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
From: Allan Sandfeld Jensen <kde@carewolf.com>
To: Benjamin Tissoires <bentiss@kernel.org>
Cc: lains@riseup.net, hadess@hadess.net, jikos@kernel.org,
 benjamin.tissoires@redhat.com, linux-input@vger.kernel.org,
 linux-kernel@vger.kernel.org, Hans de Goede <hdegoede@redhat.com>
Subject: Re: [PATCH] Logitech Anywhere 3SB support
Date: Mon, 15 Apr 2024 18:36:43 +0200
Message-ID: <2266147.iZASKD2KPV@twilight>
In-Reply-To: <ntsifcsfo5i6xisxbgfjdpe4uenygqxrt3v5sceflgipznw6cb@gnhvkjmglrtg>
References:
 <20240413095453.14816-1-kde@carewolf.com>
 <ntsifcsfo5i6xisxbgfjdpe4uenygqxrt3v5sceflgipznw6cb@gnhvkjmglrtg>
Precedence: bulk
MIME-Version: 1.0
Content-Transfer-Encoding: 7Bit
Content-Type: text/plain; charset="utf-8"

On Monday 15 April 2024 17:54:57 CEST Benjamin Tissoires wrote:
> [You don't often get email from bentiss@kernel.org. Learn why this is
> important at https://aka.ms/LearnAboutSenderIdentification ]
> 
> [Ccing Hans as well for input]
> 
> On Apr 13 2024, kde@carewolf.com wrote:
> > From: Allan Sandfeld Jensen <allan.jensen@qt.io>
> 
> 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?
> 
I added it to get hires scroll wheel events working out of the box.

The main differences for the bolt receiver as I briefly skimmed it, are 
different peering commands, which I didn't want to touch that.

For my purpose I discovered the bolt receiver operated much like the gaming-
hidpp receiver, except that I have four interfaces.

> >  };
> >  
> >  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).
> 
I am getting four. The fourth one is the one with 0x10 case I added above.
[    5.706399] logitech-djreceiver 0003:046D:C548.0003: device of type eQUAD 
Lightspeed 1.2 (0x10) connected on slot 2

You can leave out the added 0x10 case, and just treat the bolt receiver as a 
gaming_hidpp receiver I assume. I got an error there about unknown device, 
when I originaly tried just using the gaming_hidpp type, but it is possible it 
could still work (I hit another bug when I originally tried that) . I can go 
back and check if you would prefer this minimalist solution?

I dont have any additional bolt capable devices, so I can't test how it would 
work if I peered more devices.

Best regards
Allan