hid-mcp2200: added driver for GPIOs of MCP2200

This works fine since the mcp2000 struct will be dynamically allocated. The reason I went with a separate allocation for the buffer was just to make it explicitly clear that no matter how a thunderstrike instance is set up, the buffer will need to be dynamically allocated for hid requests.

I have reinserted the relevant code from the core hid stack in my previous email since it's important for this discussion.

    static void hid_device_remove(struct device *dev)
    {
      struct hid_device *hdev = to_hid_device(dev);
      struct hid_driver *hdrv;

      down(&hdev->driver_input_lock);
      hdev->io_started = false;

      hdrv = hdev->driver;
      if (hdrv) {
        if (hdrv->remove)
          hdrv->remove(hdev);
        else /* default remove */
          hid_hw_stop(hdev);

hid_device_remove will call hid_hw_stop and so will mcp2200_hid_unregister because of the devm action you added.

Let me try to break down the problem first.

1. You add mcp2200_hid_unregister to the devm actions for clean up the device.
2. mcp2200_hid_unregister will call hid_hw_close and hid_hw_stop, tearing down the device.
3. hid_device_remove is invoked when the device is removed, which already calls hid_hw_stop when no remove function is registered (the expectation is the device is simple when this is the case)
4. This leads to the device already being torn down, which leads to the exception seen when the devm kicks in and mcp_hid_unregister is then triggered.
5. Using an empty remove function resolves this but indicates the driver has an inappropriate devm action in my opinion/has problematic design.

I am saying that using an empty remove function to work around the problem is not an upstream-able solution in my opinion.

Given this, I think its best to not use devm in this can and manually handle cleanup, so you do not have a stub remove function and take control of the teardown.

That said, maybe someone else can chime in on this to see if this aligns with others' preferences? At the very least though if others feel using an empty remove function is ok, I think the comment in the remove function needs to updated to add clear detail about the issue than what is currently provided. That said, I am very much against using an empty remove function to work around problematic devm practices.

/*
* With no remove function you sometimes get a segmentation fault when
* unloading the module or disconnecting the USB device */

--
Thanks,

Rahul Rameshbabu

Hi Rahul,

thank you for the explanation, now I got it.

I have added hid_hw_stop and hid_hw_close to my remove function and removed the devm_add_action_or_reset. The driver still worked well.

So, if it is okay for you, I would go this way and generate a new patch.

Best regards,
Johannes

Hi Johannes,

Glad to hear this worked out. This discussion has motivated me to take a deeper look at hid_device_remove in the near future to see what can be done for devices that need to invoke hid_hw_open without needing to explicitly implement a remove callback.

This sounds great. Excited to see your patch on the mailing list.

--
Thanks,

Rahul Rameshbabu

Added a gpiochip compatible driver to control the 8 GPIOs of the MCP2200 by using the HID interface.

Using GPIOs with alternative functions (GP0<->SSPND, GP1<->USBCFG, GP6<->RXLED, GP7<->TXLED) will reset the functions, if set (unset by default).

The driver was tested while also using the UART of the chip. Setting and reading the GPIOs has no effect on the UART communication. However, a reset is triggered after the CONFIGURE command. If the GPIO Direction is constantly changed, this will affect the communication at low baud rates. This is a hardware problem of the MCP2200 and is not caused by the driver.

---
 drivers/hid/Kconfig       |   9 +
 drivers/hid/Makefile      |   1 +
 drivers/hid/hid-ids.h     |   1 +
 drivers/hid/hid-mcp2200.c | 390 ++++++++++++++++++++++++++++++++++++++
 4 files changed, 401 insertions(+)
 create mode 100644 drivers/hid/hid-mcp2200.c

index 0cea301cc9a9..3c14644b593d 100644
@@ -1298,6 +1298,15 @@ config HID_ALPS
 	Say Y here if you have a Alps touchpads over i2c-hid or usbhid
 	and want support for its special functionalities.
 
+config HID_MCP2200
+	tristate "Microchip MCP2200 HID USB-to-GPIO bridge"
+	depends on USB_HID && GPIOLIB
+	help
+	  Provides GPIO functionality over USB-HID through MCP2200 device.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called hid-mcp2200.ko.
+
 config HID_MCP2221
 	tristate "Microchip MCP2221 HID USB-to-I2C/SMbus host support"
 	depends on USB_HID && I2C

index 8a06d0f840bc..082a728eac60 100644
@@ -79,6 +79,7 @@ obj-$(CONFIG_HID_LOGITECH_HIDPP)	+= hid-logitech-hidpp.o
 obj-$(CONFIG_HID_MACALLY)	+= hid-macally.o
 obj-$(CONFIG_HID_MAGICMOUSE)	+= hid-magicmouse.o
 obj-$(CONFIG_HID_MALTRON)	+= hid-maltron.o
+obj-$(CONFIG_HID_MCP2200)	+= hid-mcp2200.o
 obj-$(CONFIG_HID_MCP2221)	+= hid-mcp2221.o
 obj-$(CONFIG_HID_MAYFLASH)	+= hid-mf.o
 obj-$(CONFIG_HID_MEGAWORLD_FF)	+= hid-megaworld.o

index 7e499992a793..bb87ad4eb2aa 100644
@@ -915,6 +915,7 @@
 #define USB_DEVICE_ID_PICK16F1454	0x0042
 #define USB_DEVICE_ID_PICK16F1454_V2	0xf2f7
 #define USB_DEVICE_ID_LUXAFOR		0xf372
+#define USB_DEVICE_ID_MCP2200		0x00df
 #define USB_DEVICE_ID_MCP2221		0x00dd
 
 #define USB_VENDOR_ID_MICROSOFT		0x045e

2.42.0