On Wed, Sep 29, 2021 at 9:50 AM Michal Malý
[off-list ref] wrote:

On Tue, 2021-09-28 at 10:39 +0200, Benjamin Tissoires wrote:

quoted

When removing a device, we can not do much if there is an error while
removing it. Use the common pattern of returning void there so we are
not tempted to check on the return value.
And honestly, we were not looking at it, so why bother?

Signed-off-by: Benjamin Tissoires <redacted>
---
 drivers/hid/hid-lg4ff.c | 5 ++---
 drivers/hid/hid-lg4ff.h | 4 ++--
 2 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/hid/hid-lg4ff.c b/drivers/hid/hid-lg4ff.c
index 5e6a0cef2a06..ad75c86e0bf5 100644
--- a/drivers/hid/hid-lg4ff.c
+++ b/drivers/hid/hid-lg4ff.c

@@ -1445,7 +1445,7 @@ int lg4ff_init(struct hid_device *hid)
        return error;
 }

-int lg4ff_deinit(struct hid_device *hid)
+void lg4ff_deinit(struct hid_device *hid)
 {
        struct lg4ff_device_entry *entry;
        struct lg_drv_data *drv_data;

@@ -1453,7 +1453,7 @@ int lg4ff_deinit(struct hid_device *hid)
        drv_data = hid_get_drvdata(hid);
        if (!drv_data) {
                hid_err(hid, "Error while deinitializing device, no

private driver data.\n");
-               return -1;
+               return;
        }
        entry = drv_data->device_props;
        if (!entry)

@@ -1489,5 +1489,4 @@ int lg4ff_deinit(struct hid_device *hid)
        kfree(entry);
 out:
        dbg_hid("Device successfully unregistered\n");
-       return 0;
 }

diff --git a/drivers/hid/hid-lg4ff.h b/drivers/hid/hid-lg4ff.h
index 25bc88cd877e..4440e4ea2267 100644
--- a/drivers/hid/hid-lg4ff.h
+++ b/drivers/hid/hid-lg4ff.h

@@ -10,14 +10,14 @@ int lg4ff_adjust_input_event(struct hid_device

*hid, struct hid_field *field,
 int lg4ff_raw_event(struct hid_device *hdev, struct hid_report
*report,
                u8 *rd, int size, struct lg_drv_data *drv_data);
 int lg4ff_init(struct hid_device *hdev);
-int lg4ff_deinit(struct hid_device *hdev);
+void lg4ff_deinit(struct hid_device *hdev);
 #else
 static inline int lg4ff_adjust_input_event(struct hid_device *hid,
struct hid_field *field,
                                           struct hid_usage *usage, s32
value, struct lg_drv_data *drv_data) { return 0; }
 static inline int lg4ff_raw_event(struct hid_device *hdev, struct
hid_report *report,
                u8 *rd, int size, struct lg_drv_data *drv_data) {
return 0; }
 static inline int lg4ff_init(struct hid_device *hdev) { return 0; }
-static inline int lg4ff_deinit(struct hid_device *hdev) { return 0; }
+static inline void lg4ff_deinit(struct hid_device *hdev) { return; }
 #endif

 #endif

I'll try to dig up some lg4ff hardware to make sure but AFAICT skipping
the init routines will make all of the devices look like standard HID
stuff. Mind that disabling lg4ff disables more than just FF for the
affected Logitech wheels but that probably doesn't matter.

Perhaps a bit better approach would be to remove the special handling
of these devices from the hid-lg driver altogether when the respective
submodules are switched off. That way the devices should be handled
just by the generic HID code and we won't need the dud functions at
all.

I'm OK with that approach. there are a few bits in the following code
that need changes, see my replies inline.

I only checked that this compiles. I can test this with real HW if you
find this acceptable.

That would be great, I don't have the HW :)

I would rather keep those definitions in hid-lg.h, not inlined in the code

Let's not sprinkle the code with #ifdef. Just add an inline
lg4ff_adjust_input_event() in hid-lg4ff.h that returns 0.

same here

same here :)

That's the part I was not entirely sure: I *think* not having NOGET
when using the generic path is OK now, but having a real hardware test
would be nice (if you still have this one).
Worse case I should be able to get this tested through the RHEL bug,
but that would take some time to get.

+#endif
+#ifdef CONFIG_LOGIG940_FF
+       { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_FLIGHT_SYSTEM_G940),
+               .driver_data = LG_FF3 },
+#endif
+#ifdef CONFIG_LOGIWHEELS_FF
        { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_MOMO_WHEEL),
                .driver_data = LG_NOGET | LG_FF4 },
        { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_MOMO_WHEEL2),
                .driver_data = LG_FF4 },
-       { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_VIBRATION_WHEEL),
-               .driver_data = LG_FF2 },
+       { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_WHEEL),
+               .driver_data = LG_NOGET | LG_FF4 },
+       { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_DFP_WHEEL),
+               .driver_data = LG_NOGET | LG_FF4 },
+       { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_G27_WHEEL),
+               .driver_data = LG_FF4 },
        { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_G25_WHEEL),
                .driver_data = LG_FF4 },
-       { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_DFGT_WHEEL),
+       { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_G29_WHEEL),
                .driver_data = LG_FF4 },
-       { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_G27_WHEEL),
+       { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_DFGT_WHEEL),
                .driver_data = LG_FF4 },
-       { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_DFP_WHEEL),
+       { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_WINGMAN_FFG),
                .driver_data = LG_NOGET | LG_FF4 },
+#else  /* Wii wheel still needs a bit of special handling */
        { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_WII_WHEEL),
-               .driver_data = LG_FF4 },
+               .driver_data = 0 },

I am not sure about this bit. Can you expand on it a bit?

I would keep all of those static inline defines here, as mentioned above.

This patch is also missing a change in hid-quirks.c: we need to remove
them from the hid_have_special_driver[] list, or they will simply not
bind to anything when they are plugged with the FF disabled.
We could also add #ifdef in hid_have_special_driver[], but if it works
without, that would be better actually.

Cheers,
Benjamin

 #endif