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.
Signed-off-by: Johannes Roith <[email protected]>
create mode 100644 drivers/hid/hid-mcp2200.c
diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index 4ce012f83253..02115dcc6b0b 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -1283,6 +1283,16 @@ config HID_MCP2221
To compile this driver as a module, choose M here: the module
will be called hid-mcp2221.ko.
+config HID_MCP2200
+ tristate "Microchip MCP2200 HID USB-to-GPIO bridge"
+ depends on USB_HID
+ imply 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_KUNIT_TEST
tristate "KUnit tests for HID" if !KUNIT_ALL_TESTS
depends on KUNIT=y
diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
index 5d37cacbde33..d593fb982f7d 100644
--- a/drivers/hid/Makefile
+++ b/drivers/hid/Makefile
@@ -79,6 +79,7 @@ obj-$(CONFIG_HID_MACALLY) += hid-macally.o
obj-$(CONFIG_HID_MAGICMOUSE) += hid-magicmouse.o
obj-$(CONFIG_HID_MALTRON) += hid-maltron.o
obj-$(CONFIG_HID_MCP2221) += hid-mcp2221.o
+obj-$(CONFIG_HID_MCP2200) += hid-mcp2200.o
obj-$(CONFIG_HID_MAYFLASH) += hid-mf.o
obj-$(CONFIG_HID_MEGAWORLD_FF) += hid-megaworld.o
obj-$(CONFIG_HID_MICROSOFT) += hid-microsoft.o
diff --git a/drivers/hid/hid-mcp2200.c b/drivers/hid/hid-mcp2200.c
new file mode 100644
index 000000000000..600f3f3e073b
--- /dev/null
+++ b/drivers/hid/hid-mcp2200.c
@@ -0,0 +1,421 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * MCP2200 - Microchip USB to GPIO bridge
+ *
+ * Copyright (c) 2023, Johannes Roith <[email protected]>
+ *
+ * Datasheet: https://ww1.microchip.com/downloads/en/DeviceDoc/22228A.pdf
+ * App Note for HID: https://ww1.microchip.com/downloads/en/DeviceDoc/93066A.pdf
+ */
+#include <linux/module.h>
+#include <linux/err.h>
+#include <linux/mutex.h>
+#include <linux/bitfield.h>
+#include <linux/completion.h>
+#include <linux/delay.h>
+#include <linux/hid.h>
+#include <linux/hidraw.h>
+#include <linux/gpio/driver.h>
+#include "hid-ids.h"
+
+/* Commands codes in a raw output report */
+enum {
+ SET_CLEAR_OUTPUTS = 0x08,
+ CONFIGURE = 0x10,
+ READ_EE = 0x20,
+ WRITE_EE = 0x40,
+ READ_ALL = 0x80
+};
+
+/* MCP GPIO direction encoding */
+enum MCP_IO_DIR {
+ MCP2200_DIR_OUT = 0x00,
+ MCP2200_DIR_IN = 0x01,
+};
+
+/* Altternative pin assignments */
+enum {
+ TXLED = 2,
+ RXLED = 3,
+ USBCFG = 6,
+ SSPND = 7,
+
+};
+
+#define MCP_NGPIO 8
+
+/* CMD to set or clear a GPIO output */
+struct mcp_set_clear_outputs {
+ u8 cmd;
+ u8 dummys1[10];
+ u8 set_bmap;
+ u8 clear_bmap;
+ u8 dummys2[3];
+} __packed;
+
+/* CMD to configure the IOs */
+struct mcp_configure {
+ u8 cmd;
+ u8 dummys1[3];
+ u8 io_bmap;
+ u8 config_alt_pins;
+ u8 io_default_val_bmap;
+ u8 config_alt_options;
+ u8 baud_h;
+ u8 baud_l;
+ u8 dummys2[6];
+} __packed;
+
+/* CMD to read all parameters */
+struct mcp_read_all {
+ u8 cmd;
+ u8 dummys[15];
+} __packed;
+
+/* Response to the read all cmd */
+struct mcp_read_all_resp {
+ u8 cmd;
+ u8 eep_addr;
+ u8 dummy;
+ u8 eep_val;
+ u8 io_bmap;
+ u8 config_alt_pins;
+ u8 io_default_val_bmap;
+ u8 config_alt_options;
+ u8 baud_h;
+ u8 baud_l;
+ u8 io_port_val_bmap;
+ u8 dummys[5];
+} __packed;
+
+struct mcp2200 {
+ struct hid_device *hdev;
+ struct mutex lock;
+ struct completion wait_in_report;
+ u8 gpio_dir;
+ u8 gpio_val;
+ u8 gpio_inval;
+ u8 baud_h;
+ u8 baud_l;
+ u8 config_alt_pins;
+ u8 gpio_reset_val;
+ u8 config_alt_options;
+ int status;
+ struct gpio_chip gc;
+};
+
+/* this executes the READ_ALL cmd */
+static int mcp_cmd_read_all(struct mcp2200 *mcp)
+{
+ struct mcp_read_all *read_all;
+ int len, t;
+
+ reinit_completion(&mcp->wait_in_report);
+ mutex_lock(&mcp->lock);
+
+ read_all = kzalloc(sizeof(struct mcp_read_all), GFP_KERNEL);
+ if (!read_all)
+ return -ENOMEM;
+
+ read_all->cmd = READ_ALL;
+ len = hid_hw_output_report(mcp->hdev, (u8 *) read_all,
+ sizeof(struct mcp_read_all));
+
+ if (len != sizeof(struct mcp_read_all))
+ return -EINVAL;
+
+ kfree(read_all);
+ mutex_unlock(&mcp->lock);
+ t = wait_for_completion_timeout(&mcp->wait_in_report, msecs_to_jiffies(4000));
+ if (!t)
+ return -ETIMEDOUT;
+
+ /* return status, negative value if wrong response was received */
+ return mcp->status;
+}
+
+static void mcp_set_multiple(struct gpio_chip *gc, unsigned long *mask,
+ unsigned long *bits)
+{
+ struct mcp2200 *mcp = gpiochip_get_data(gc);
+ u8 value;
+ int status;
+ struct mcp_set_clear_outputs *cmd;
+
+ cmd = kzalloc(sizeof(struct mcp_set_clear_outputs), GFP_KERNEL);
+ if (!cmd)
+ return;
+
+ mutex_lock(&mcp->lock);
+
+ value = mcp->gpio_val & ~*mask;
+ value |= (*mask & *bits);
+
+ cmd->cmd = SET_CLEAR_OUTPUTS;
+ cmd->set_bmap = value;
+ cmd->clear_bmap = ~(value);
+
+ status = hid_hw_output_report(mcp->hdev, (u8 *) cmd,
+ sizeof(struct mcp_set_clear_outputs));
+ if (status == sizeof(struct mcp_set_clear_outputs))
+ mcp->gpio_val = value;
+
+ kfree(cmd);
+ mutex_unlock(&mcp->lock);
+}
+
+static void mcp_set(struct gpio_chip *gc, unsigned int gpio_nr, int value)
+{
+ unsigned long mask = (1 << gpio_nr);
+ unsigned long bmap_value = (value<<gpio_nr);
+
+ mcp_set_multiple(gc, &mask, &bmap_value);
+}
+
+static int mcp_get_multiple(struct gpio_chip *gc, unsigned long *mask,
+ unsigned long *bits)
+{
+ u32 val;
+ struct mcp2200 *mcp = gpiochip_get_data(gc);
+ int status;
+
+ status = mcp_cmd_read_all(mcp);
+ if (status != 0)
+ return status;
+
+ val = mcp->gpio_inval;
+ *bits = (val & *mask);
+ return 0;
+}
+
+static int mcp_get(struct gpio_chip *gc, unsigned int gpio_nr)
+{
+ unsigned long mask = 0, bits = 0;
+
+ mask = (1 << gpio_nr);
+ mcp_get_multiple(gc, &mask, &bits);
+ return (bits > 0) ? 1 : 0;
+}
+
+static int mcp_get_direction(struct gpio_chip *gc, unsigned int gpio_nr)
+{
+ struct mcp2200 *mcp = gpiochip_get_data(gc);
+
+ return (mcp->gpio_dir & (MCP2200_DIR_IN << gpio_nr))
+ ? GPIO_LINE_DIRECTION_IN : GPIO_LINE_DIRECTION_OUT;
+}
+
+static int mcp_set_direction(struct gpio_chip *gc, unsigned int gpio_nr,
+ enum MCP_IO_DIR io_direction)
+{
+ struct mcp2200 *mcp = gpiochip_get_data(gc);
+ struct mcp_configure *conf;
+ int status;
+ /* after the configure cmd we will need to set the outputs again */
+ unsigned long mask = ~(mcp->gpio_dir); /* only set outputs */
+ unsigned long bits = mcp->gpio_val;
+ /* Offsets of alternative pins in config_alt_pins, 0 is not used */
+ u8 alt_pin_conf[8] = {SSPND, USBCFG, 0, 0, 0, 0, RXLED, TXLED};
+ u8 config_alt_pins = mcp->config_alt_pins;
+
+ /* Read in the reset baudrate first, we need it later */
+ status = mcp_cmd_read_all(mcp);
+ if (status != 0)
+ return status;
+
+ conf = kzalloc(sizeof(struct mcp_configure), GFP_KERNEL);
+ if (!conf)
+ return -ENOMEM;
+ mutex_lock(&mcp->lock);
+
+ /* configure will reset the chip! */
+ conf->cmd = CONFIGURE;
+ conf->io_bmap = (mcp->gpio_dir & ~(1 << gpio_nr))
+ | (io_direction << gpio_nr);
+ /* Don't overwrite the reset parameters */
+ conf->baud_h = mcp->baud_h;
+ conf->baud_l = mcp->baud_l;
+ conf->config_alt_options = mcp->config_alt_options;
+ conf->io_default_val_bmap = mcp->gpio_reset_val;
+ /* Adjust alt. func if necessary */
+ if (alt_pin_conf[gpio_nr])
+ config_alt_pins &= ~(1 << alt_pin_conf[gpio_nr]);
+ conf->config_alt_pins = config_alt_pins;
+
+ status = hid_hw_output_report(mcp->hdev, (u8 *) conf,
+ sizeof(struct mcp_set_clear_outputs));
+ if (status == sizeof(struct mcp_set_clear_outputs)) {
+ mcp->gpio_dir &= ~(1 << gpio_nr);
+ mcp->config_alt_pins = config_alt_pins;
+ } else {
+ return -EIO;
+ }
+
+ kfree(conf);
+ mutex_unlock(&mcp->lock);
+
+ /* Configure CMD will clear all IOs -> rewrite them */
+ mcp_set_multiple(gc, &mask, &bits);
+ return 0;
+}
+
+static int mcp_direction_input(struct gpio_chip *gc, unsigned int gpio_nr)
+{
+ return mcp_set_direction(gc, gpio_nr, MCP2200_DIR_IN);
+}
+
+static int mcp_direction_output(struct gpio_chip *gc, unsigned int gpio_nr,
+ int value)
+{
+ int ret;
+ unsigned long mask, bmap_value;
+
+ mask = (1 << gpio_nr);
+ bmap_value = (value << gpio_nr);
+
+ ret = mcp_set_direction(gc, gpio_nr, MCP2200_DIR_OUT);
+ if (ret == 0)
+ mcp_set_multiple(gc, &mask, &bmap_value);
+ return ret;
+}
+
+
+static const struct gpio_chip template_chip = {
+ .label = "mcp2200",
+ .owner = THIS_MODULE,
+ .get_direction = mcp_get_direction,
+ .direction_input = mcp_direction_input,
+ .direction_output = mcp_direction_output,
+ .set = mcp_set,
+ .set_multiple = mcp_set_multiple,
+ .get = mcp_get,
+ .get_multiple = mcp_get_multiple,
+ .base = -1,
+ .ngpio = MCP_NGPIO,
+ .can_sleep = true,
+};
+
+/*
+ * MCP2200 uses interrupt endpoint for input reports. This function
+ * is called by HID layer when it receives i/p report from mcp2200,
+ * which is actually a response to the previously sent command.
+ */
+static int mcp2200_raw_event(struct hid_device *hdev, struct hid_report *report,
+ u8 *data, int size)
+{
+ struct mcp2200 *mcp = hid_get_drvdata(hdev);
+ struct mcp_read_all_resp *all_resp;
+
+ switch (data[0]) {
+ case READ_ALL:
+ all_resp = (struct mcp_read_all_resp *) data;
+ mcp->status = 0;
+ mcp->gpio_inval = all_resp->io_port_val_bmap;
+ mcp->baud_h = all_resp->baud_h;
+ mcp->baud_l = all_resp->baud_l;
+ mcp->gpio_reset_val = all_resp->io_default_val_bmap;
+ mcp->config_alt_pins = all_resp->config_alt_pins;
+ mcp->config_alt_options = all_resp->config_alt_options;
+ break;
+ default:
+ mcp->status = -EIO;
+ break;
+ }
+
+ complete(&mcp->wait_in_report);
+ return 1;
+}
+
+static void mcp2200_hid_unregister(void *ptr)
+{
+ struct hid_device *hdev = ptr;
+
+ hid_hw_close(hdev);
+ hid_hw_stop(hdev);
+}
+
+static int mcp2200_probe(struct hid_device *hdev, const struct hid_device_id *id)
+{
+ int ret;
+ struct mcp2200 *mcp;
+
+ mcp = devm_kzalloc(&hdev->dev, sizeof(*mcp), GFP_KERNEL);
+ if (!mcp)
+ return -ENOMEM;
+
+ ret = hid_parse(hdev);
+ if (ret) {
+ hid_err(hdev, "can't parse reports\n");
+ return ret;
+ }
+
+ /*
+ * This driver uses the .raw_event callback and therefore does not need any
+ * HID_CONNECT_xxx flags.
+ */
+ ret = hid_hw_start(hdev, 0);
+ if (ret) {
+ hid_err(hdev, "can't start hardware\n");
+ return ret;
+ }
+
+ hid_info(hdev, "USB HID v%x.%02x Device [%s] on %s\n", hdev->version >> 8,
+ hdev->version & 0xff, hdev->name, hdev->phys);
+
+ ret = hid_hw_open(hdev);
+ if (ret) {
+ hid_err(hdev, "can't open device\n");
+ hid_hw_stop(hdev);
+ return ret;
+ }
+
+ mutex_init(&mcp->lock);
+ init_completion(&mcp->wait_in_report);
+ hid_set_drvdata(hdev, mcp);
+ mcp->hdev = hdev;
+
+ ret = devm_add_action_or_reset(&hdev->dev, mcp2200_hid_unregister, hdev);
+ if (ret)
+ return ret;
+
+ mcp->gc = template_chip;
+ mcp->gc.parent = &hdev->dev;
+
+ ret = gpiochip_add_data(&mcp->gc, mcp);
+ if (ret < 0) {
+ dev_err(&hdev->dev, "Unable to register gpiochip\n");
+ hid_hw_stop(hdev);
+ return ret;
+ }
+
+ return 0;
+}
+
+static void mcp2200_remove(struct hid_device *hdev)
+{
+ struct mcp2200 *mcp;
+
+ mcp = hid_get_drvdata(hdev);
+ gpiochip_remove(&mcp->gc);
+}
+
+static const struct hid_device_id mcp2200_devices[] = {
+ { HID_USB_DEVICE(USB_VENDOR_ID_MICROCHIP, USB_DEVICE_ID_MCP2200) },
+ { }
+};
+MODULE_DEVICE_TABLE(hid, mcp2200_devices);
+
+static struct hid_driver mcp2200_driver = {
+ .name = "mcp2200",
+ .id_table = mcp2200_devices,
+ .probe = mcp2200_probe,
+ .remove = mcp2200_remove,
+ .raw_event = mcp2200_raw_event,
+};
+
+/* Register with HID core */
+module_hid_driver(mcp2200_driver);
+
+MODULE_AUTHOR("Johannes Roith <[email protected]>");
+MODULE_DESCRIPTION("MCP2200 Microchip HID USB to GPIO bridge");
+MODULE_LICENSE("GPL");
--
2.41.0
Added Product ID for Microchip MCP2200 so the module can be compiled
correctly.
Signed-off-by: Johannes Roith <[email protected]>
diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index 5d29abac2300..b48cb8224e0d 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -912,6 +912,7 @@
#define USB_DEVICE_ID_PICK16F1454_V2 0xf2f7
#define USB_DEVICE_ID_LUXAFOR 0xf372
#define USB_DEVICE_ID_MCP2221 0x00dd
+#define USB_DEVICE_ID_MCP2221 0x00df
#define USB_VENDOR_ID_MICROSOFT 0x045e
#define USB_DEVICE_ID_SIDEWINDER_GV 0x003b
--
2.41.0
Hi Johannes,
kernel test robot noticed the following build warnings:
[auto build test WARNING on v6.4-rc5]
[also build test WARNING on linus/master next-20230609]
[cannot apply to hid/for-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Johannes-Roith/hid-mcp2200-updated-hid-id-h/20230612-005025
base: v6.4-rc5
patch link: https://lore.kernel.org/r/20230611164811.1388-2-johannes%40gnu-linux.rocks
patch subject: [PATCH 2/2] hid-mcp2200 - updated hid-id.h
config: um-x86_64_defconfig (https://download.01.org/0day-ci/archive/20230612/[email protected]/config)
compiler: clang version 15.0.7 (https://github.com/llvm/llvm-project.git 8dfdcc7b7bf66834a761bd8de445840ef68e4d1a)
reproduce (this is a W=1 build):
mkdir -p ~/bin
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install um cross compiling tool for clang build
# apt-get install binutils-um-linux-gnu
git checkout v6.4-rc5
b4 shazam https://lore.kernel.org/r/[email protected]
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang ~/bin/make.cross W=1 O=build_dir ARCH=um olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang ~/bin/make.cross W=1 O=build_dir ARCH=um SHELL=/bin/bash drivers/
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All warnings (new ones prefixed by >>):
In file included from drivers/hid/hid-core.c:31:
In file included from include/linux/hid.h:29:
In file included from include/linux/hid_bpf.h:6:
In file included from include/linux/bpf.h:31:
In file included from include/linux/memcontrol.h:13:
In file included from include/linux/cgroup.h:26:
In file included from include/linux/kernel_stat.h:9:
In file included from include/linux/interrupt.h:11:
In file included from include/linux/hardirq.h:11:
In file included from arch/um/include/asm/hardirq.h:5:
In file included from include/asm-generic/hardirq.h:17:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/um/include/asm/io.h:24:
include/asm-generic/io.h:547:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __raw_readb(PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:560:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
~~~~~~~~~~ ^
include/uapi/linux/byteorder/little_endian.h:37:51: note: expanded from macro '__le16_to_cpu'
#define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
^
In file included from drivers/hid/hid-core.c:31:
In file included from include/linux/hid.h:29:
In file included from include/linux/hid_bpf.h:6:
In file included from include/linux/bpf.h:31:
In file included from include/linux/memcontrol.h:13:
In file included from include/linux/cgroup.h:26:
In file included from include/linux/kernel_stat.h:9:
In file included from include/linux/interrupt.h:11:
In file included from include/linux/hardirq.h:11:
In file included from arch/um/include/asm/hardirq.h:5:
In file included from include/asm-generic/hardirq.h:17:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/um/include/asm/io.h:24:
include/asm-generic/io.h:573:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
~~~~~~~~~~ ^
include/uapi/linux/byteorder/little_endian.h:35:51: note: expanded from macro '__le32_to_cpu'
#define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
^
In file included from drivers/hid/hid-core.c:31:
In file included from include/linux/hid.h:29:
In file included from include/linux/hid_bpf.h:6:
In file included from include/linux/bpf.h:31:
In file included from include/linux/memcontrol.h:13:
In file included from include/linux/cgroup.h:26:
In file included from include/linux/kernel_stat.h:9:
In file included from include/linux/interrupt.h:11:
In file included from include/linux/hardirq.h:11:
In file included from arch/um/include/asm/hardirq.h:5:
In file included from include/asm-generic/hardirq.h:17:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/um/include/asm/io.h:24:
include/asm-generic/io.h:584:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writeb(value, PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:594:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:604:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:692:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
readsb(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
include/asm-generic/io.h:700:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
readsw(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
include/asm-generic/io.h:708:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
readsl(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
include/asm-generic/io.h:717:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
writesb(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
include/asm-generic/io.h:726:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
writesw(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
include/asm-generic/io.h:735:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
writesl(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
In file included from drivers/hid/hid-core.c:36:
>> drivers/hid/hid-ids.h:915:9: warning: 'USB_DEVICE_ID_MCP2221' macro redefined [-Wmacro-redefined]
#define USB_DEVICE_ID_MCP2221 0x00df
^
drivers/hid/hid-ids.h:914:9: note: previous definition is here
#define USB_DEVICE_ID_MCP2221 0x00dd
^
13 warnings generated.
vim +/USB_DEVICE_ID_MCP2221 +915 drivers/hid/hid-ids.h
905
906 #define USB_VENDOR_ID_MICROCHIP 0x04d8
907 #define USB_DEVICE_ID_PICKIT1 0x0032
908 #define USB_DEVICE_ID_PICKIT2 0x0033
909 #define USB_DEVICE_ID_PICOLCD 0xc002
910 #define USB_DEVICE_ID_PICOLCD_BOOTLOADER 0xf002
911 #define USB_DEVICE_ID_PICK16F1454 0x0042
912 #define USB_DEVICE_ID_PICK16F1454_V2 0xf2f7
913 #define USB_DEVICE_ID_LUXAFOR 0xf372
914 #define USB_DEVICE_ID_MCP2221 0x00dd
> 915 #define USB_DEVICE_ID_MCP2221 0x00df
916
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Hi Johannes,
kernel test robot noticed the following build errors:
[auto build test ERROR on v6.4-rc5]
[also build test ERROR on linus/master next-20230609]
[cannot apply to hid/for-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Johannes-Roith/hid-mcp2200-updated-hid-id-h/20230612-005025
base: v6.4-rc5
patch link: https://lore.kernel.org/r/20230611164811.1388-1-johannes%40gnu-linux.rocks
patch subject: [PATCH 1/2] hid-mcp2200 added driver for MCP2200 GPIOs
config: x86_64-buildonly-randconfig-r003-20230612 (https://download.01.org/0day-ci/archive/20230612/[email protected]/config)
compiler: clang version 15.0.7 (https://github.com/llvm/llvm-project.git 8dfdcc7b7bf66834a761bd8de445840ef68e4d1a)
reproduce (this is a W=1 build):
mkdir -p ~/bin
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
git checkout v6.4-rc5
b4 shazam https://lore.kernel.org/r/[email protected]
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang ~/bin/make.cross W=1 O=build_dir ARCH=x86_64 olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang ~/bin/make.cross W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All errors (new ones prefixed by >>):
>> drivers/hid/hid-mcp2200.c:403:44: error: use of undeclared identifier 'USB_DEVICE_ID_MCP2200'
{ HID_USB_DEVICE(USB_VENDOR_ID_MICROCHIP, USB_DEVICE_ID_MCP2200) },
^
1 error generated.
vim +/USB_DEVICE_ID_MCP2200 +403 drivers/hid/hid-mcp2200.c
401
402 static const struct hid_device_id mcp2200_devices[] = {
> 403 { HID_USB_DEVICE(USB_VENDOR_ID_MICROCHIP, USB_DEVICE_ID_MCP2200) },
404 { }
405 };
406 MODULE_DEVICE_TABLE(hid, mcp2200_devices);
407
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Le 11/06/2023 à 18:48, Johannes Roith a écrit :
> 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.
>
> Signed-off-by: Johannes Roith <[email protected]>
Hi,
a few nits below, should it help the review.
[...]
> --- /dev/null
> +++ b/drivers/hid/hid-mcp2200.c
> @@ -0,0 +1,421 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * MCP2200 - Microchip USB to GPIO bridge
> + *
> + * Copyright (c) 2023, Johannes Roith <[email protected]>
> + *
> + * Datasheet: https://ww1.microchip.com/downloads/en/DeviceDoc/22228A.pdf
> + * App Note for HID: https://ww1.microchip.com/downloads/en/DeviceDoc/93066A.pdf
> + */
> +#include <linux/module.h>
> +#include <linux/err.h>
> +#include <linux/mutex.h>
> +#include <linux/bitfield.h>
Is this include needed?
> +#include <linux/completion.h>
> +#include <linux/delay.h>
> +#include <linux/hid.h>
> +#include <linux/hidraw.h>
> +#include <linux/gpio/driver.h>
Many maintainers prefer alphabetic order for includes.
> +#include "hid-ids.h"
> +
> +/* Commands codes in a raw output report */
> +enum {
> + SET_CLEAR_OUTPUTS = 0x08,
> + CONFIGURE = 0x10,
> + READ_EE = 0x20,
> + WRITE_EE = 0x40,
> + READ_ALL = 0x80
> +};
Does some
#define xxx BIT(n)
would make more sense than this enum?
> +
> +/* MCP GPIO direction encoding */
> +enum MCP_IO_DIR {
> + MCP2200_DIR_OUT = 0x00,
> + MCP2200_DIR_IN = 0x01,
> +};
> +
> +/* Altternative pin assignments */
> +enum {
> + TXLED = 2,
> + RXLED = 3,
> + USBCFG = 6,
> + SSPND = 7,
> +
Uneeded new line.
> +};
> +
> +#define MCP_NGPIO 8
> +
[...]
> +/* this executes the READ_ALL cmd */
> +static int mcp_cmd_read_all(struct mcp2200 *mcp)
> +{
> + struct mcp_read_all *read_all;
> + int len, t;
> +
> + reinit_completion(&mcp->wait_in_report);
> + mutex_lock(&mcp->lock);
> +
> + read_all = kzalloc(sizeof(struct mcp_read_all), GFP_KERNEL);
> + if (!read_all)
> + return -ENOMEM;
Allocation could be deone before the lock.
> +
> + read_all->cmd = READ_ALL;
> + len = hid_hw_output_report(mcp->hdev, (u8 *) read_all,
> + sizeof(struct mcp_read_all));
> +
> + if (len != sizeof(struct mcp_read_all))
kfree(read_all); ?
(or move the call just below before the test)
> + return -EINVAL;
> +
> + kfree(read_all);
> + mutex_unlock(&mcp->lock);
Mutex unlock could be done before kfree() or even before the "if
(len..." a few lines above.
> + t = wait_for_completion_timeout(&mcp->wait_in_report, msecs_to_jiffies(4000));
> + if (!t)
> + return -ETIMEDOUT;
> +
> + /* return status, negative value if wrong response was received */
> + return mcp->status;
> +}
> +
> +static void mcp_set_multiple(struct gpio_chip *gc, unsigned long *mask,
> + unsigned long *bits)
> +{
> + struct mcp2200 *mcp = gpiochip_get_data(gc);
> + u8 value;
> + int status;
> + struct mcp_set_clear_outputs *cmd;
> +
> + cmd = kzalloc(sizeof(struct mcp_set_clear_outputs), GFP_KERNEL);
> + if (!cmd)
> + return;
> +
> + mutex_lock(&mcp->lock);
> +
> + value = mcp->gpio_val & ~*mask;
> + value |= (*mask & *bits);
> +
> + cmd->cmd = SET_CLEAR_OUTPUTS;
> + cmd->set_bmap = value;
> + cmd->clear_bmap = ~(value);
> +
> + status = hid_hw_output_report(mcp->hdev, (u8 *) cmd,
> + sizeof(struct mcp_set_clear_outputs));
> + if (status == sizeof(struct mcp_set_clear_outputs))
> + mcp->gpio_val = value;
> +
> + kfree(cmd);
> + mutex_unlock(&mcp->lock);
Mutex unlock could be done before kfree().
> +}
> +
> +static void mcp_set(struct gpio_chip *gc, unsigned int gpio_nr, int value)
> +{
> + unsigned long mask = (1 << gpio_nr);
Uneeded ()
Does using BIT makes sense here?
> + unsigned long bmap_value = (value<<gpio_nr);
Uneeded () and missing spaces aoud <<
> +
> + mcp_set_multiple(gc, &mask, &bmap_value);
> +}
> +
> +static int mcp_get_multiple(struct gpio_chip *gc, unsigned long *mask,
> + unsigned long *bits)
> +{
> + u32 val;
> + struct mcp2200 *mcp = gpiochip_get_data(gc);
> + int status;
> +
> + status = mcp_cmd_read_all(mcp);
> + if (status != 0)
> + return status;
> +
> + val = mcp->gpio_inval;
> + *bits = (val & *mask);
> + return 0;
> +}
> +
> +static int mcp_get(struct gpio_chip *gc, unsigned int gpio_nr)
> +{
> + unsigned long mask = 0, bits = 0;
No need to init long (and maybe bits)
> +
> + mask = (1 << gpio_nr);
Uneeded ()
Does using BIT makes sense here?
> + mcp_get_multiple(gc, &mask, &bits);
> + return (bits > 0) ? 1 : 0;
> +}
> +
> +static int mcp_get_direction(struct gpio_chip *gc, unsigned int gpio_nr)
> +{
> + struct mcp2200 *mcp = gpiochip_get_data(gc);
> +
> + return (mcp->gpio_dir & (MCP2200_DIR_IN << gpio_nr))
> + ? GPIO_LINE_DIRECTION_IN : GPIO_LINE_DIRECTION_OUT;
> +}
> +
> +static int mcp_set_direction(struct gpio_chip *gc, unsigned int gpio_nr,
> + enum MCP_IO_DIR io_direction)
> +{
> + struct mcp2200 *mcp = gpiochip_get_data(gc);
> + struct mcp_configure *conf;
> + int status;
> + /* after the configure cmd we will need to set the outputs again */
> + unsigned long mask = ~(mcp->gpio_dir); /* only set outputs */
> + unsigned long bits = mcp->gpio_val;
> + /* Offsets of alternative pins in config_alt_pins, 0 is not used */
> + u8 alt_pin_conf[8] = {SSPND, USBCFG, 0, 0, 0, 0, RXLED, TXLED};
> + u8 config_alt_pins = mcp->config_alt_pins;
> +
> + /* Read in the reset baudrate first, we need it later */
> + status = mcp_cmd_read_all(mcp);
> + if (status != 0)
> + return status;
> +
> + conf = kzalloc(sizeof(struct mcp_configure), GFP_KERNEL);
> + if (!conf)
> + return -ENOMEM;
> + mutex_lock(&mcp->lock);
> +
> + /* configure will reset the chip! */
> + conf->cmd = CONFIGURE;
> + conf->io_bmap = (mcp->gpio_dir & ~(1 << gpio_nr))
> + | (io_direction << gpio_nr);
> + /* Don't overwrite the reset parameters */
> + conf->baud_h = mcp->baud_h;
> + conf->baud_l = mcp->baud_l;
> + conf->config_alt_options = mcp->config_alt_options;
> + conf->io_default_val_bmap = mcp->gpio_reset_val;
> + /* Adjust alt. func if necessary */
> + if (alt_pin_conf[gpio_nr])
> + config_alt_pins &= ~(1 << alt_pin_conf[gpio_nr]);
> + conf->config_alt_pins = config_alt_pins;
> +
> + status = hid_hw_output_report(mcp->hdev, (u8 *) conf,
> + sizeof(struct mcp_set_clear_outputs));
> + if (status == sizeof(struct mcp_set_clear_outputs)) {
> + mcp->gpio_dir &= ~(1 << gpio_nr);
> + mcp->config_alt_pins = config_alt_pins;
> + } else {
> + return -EIO;
> + }
> +
> + kfree(conf);
> + mutex_unlock(&mcp->lock);
Mutex unlock could be done before kfree().
> +
> + /* Configure CMD will clear all IOs -> rewrite them */
> + mcp_set_multiple(gc, &mask, &bits);
> + return 0;
> +}
> +
> +static int mcp_direction_input(struct gpio_chip *gc, unsigned int gpio_nr)
> +{
> + return mcp_set_direction(gc, gpio_nr, MCP2200_DIR_IN);
> +}
> +
> +static int mcp_direction_output(struct gpio_chip *gc, unsigned int gpio_nr,
> + int value)
> +{
> + int ret;
> + unsigned long mask, bmap_value;
> +
> + mask = (1 << gpio_nr);
Uneeded ()
Does using BIT makes sense here?
> + bmap_value = (value << gpio_nr);
Uneeded ()
> +
> + ret = mcp_set_direction(gc, gpio_nr, MCP2200_DIR_OUT);
> + if (ret == 0)
> + mcp_set_multiple(gc, &mask, &bmap_value);
> + return ret;
> +}
> +
> +
No need for 2 new lines.
[...]
> +static int mcp2200_probe(struct hid_device *hdev, const struct hid_device_id *id)
> +{
> + int ret;
> + struct mcp2200 *mcp;
> +
> + mcp = devm_kzalloc(&hdev->dev, sizeof(*mcp), GFP_KERNEL);
> + if (!mcp)
> + return -ENOMEM;
> +
> + ret = hid_parse(hdev);
> + if (ret) {
> + hid_err(hdev, "can't parse reports\n");
> + return ret;
> + }
> +
> + /*
> + * This driver uses the .raw_event callback and therefore does not need any
> + * HID_CONNECT_xxx flags.
> + */
> + ret = hid_hw_start(hdev, 0);
> + if (ret) {
> + hid_err(hdev, "can't start hardware\n");
> + return ret;
> + }
> +
> + hid_info(hdev, "USB HID v%x.%02x Device [%s] on %s\n", hdev->version >> 8,
> + hdev->version & 0xff, hdev->name, hdev->phys);
> +
> + ret = hid_hw_open(hdev);
> + if (ret) {
> + hid_err(hdev, "can't open device\n");
> + hid_hw_stop(hdev);
> + return ret;
> + }
> +
> + mutex_init(&mcp->lock);
> + init_completion(&mcp->wait_in_report);
> + hid_set_drvdata(hdev, mcp);
> + mcp->hdev = hdev;
> +
> + ret = devm_add_action_or_reset(&hdev->dev, mcp2200_hid_unregister, hdev);
> + if (ret)
> + return ret;
> +
> + mcp->gc = template_chip;
> + mcp->gc.parent = &hdev->dev;
> +
> + ret = gpiochip_add_data(&mcp->gc, mcp);
devm_gpiochip_add_data() and no .remove function?
> + if (ret < 0) {
> + dev_err(&hdev->dev, "Unable to register gpiochip\n");
hid_err() to be consistent?
> + hid_hw_stop(hdev);
hid_hw_stop() would be called twice. Once here and once because of the
devm_add_action_or_reset() above.
Just my 2c,
CJ
> + return ret;
> + }
> +
> + return 0;
> +}
[...]
Hi Johannes,
kernel test robot noticed the following build warnings:
[auto build test WARNING on v6.4-rc5]
[also build test WARNING on linus/master next-20230609]
[cannot apply to hid/for-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Johannes-Roith/hid-mcp2200-updated-hid-id-h/20230612-005025
base: v6.4-rc5
patch link: https://lore.kernel.org/r/20230611164811.1388-2-johannes%40gnu-linux.rocks
patch subject: [PATCH 2/2] hid-mcp2200 - updated hid-id.h
config: parisc-defconfig (https://download.01.org/0day-ci/archive/20230612/[email protected]/config)
compiler: hppa-linux-gcc (GCC) 12.3.0
reproduce (this is a W=1 build):
mkdir -p ~/bin
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
git checkout v6.4-rc5
b4 shazam https://lore.kernel.org/r/[email protected]
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.3.0 ~/bin/make.cross W=1 O=build_dir ARCH=parisc olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.3.0 ~/bin/make.cross W=1 O=build_dir ARCH=parisc SHELL=/bin/bash drivers/
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All warnings (new ones prefixed by >>):
In file included from drivers/hid/hid-core.c:36:
>> drivers/hid/hid-ids.h:915: warning: "USB_DEVICE_ID_MCP2221" redefined
915 | #define USB_DEVICE_ID_MCP2221 0x00df
|
drivers/hid/hid-ids.h:914: note: this is the location of the previous definition
914 | #define USB_DEVICE_ID_MCP2221 0x00dd
|
vim +/USB_DEVICE_ID_MCP2221 +915 drivers/hid/hid-ids.h
905
906 #define USB_VENDOR_ID_MICROCHIP 0x04d8
907 #define USB_DEVICE_ID_PICKIT1 0x0032
908 #define USB_DEVICE_ID_PICKIT2 0x0033
909 #define USB_DEVICE_ID_PICOLCD 0xc002
910 #define USB_DEVICE_ID_PICOLCD_BOOTLOADER 0xf002
911 #define USB_DEVICE_ID_PICK16F1454 0x0042
912 #define USB_DEVICE_ID_PICK16F1454_V2 0xf2f7
913 #define USB_DEVICE_ID_LUXAFOR 0xf372
914 #define USB_DEVICE_ID_MCP2221 0x00dd
> 915 #define USB_DEVICE_ID_MCP2221 0x00df
916
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Le 11/06/2023 à 18:48, Johannes Roith a écrit :
> Added Product ID for Microchip MCP2200 so the module can be compiled
> correctly.
>
> Signed-off-by: Johannes Roith <[email protected]>
>
> diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
> index 5d29abac2300..b48cb8224e0d 100644
> --- a/drivers/hid/hid-ids.h
> +++ b/drivers/hid/hid-ids.h
> @@ -912,6 +912,7 @@
> #define USB_DEVICE_ID_PICK16F1454_V2 0xf2f7
> #define USB_DEVICE_ID_LUXAFOR 0xf372
> #define USB_DEVICE_ID_MCP2221 0x00dd
> +#define USB_DEVICE_ID_MCP2221 0x00df
2 times USB_DEVICE_ID_MCP2221 with different values?
USB_DEVICE_ID_MCP2200?
Also this one should be merged with patch 1/2 or sent before the other
one, otherwise 1/2 can't compile.
CJ
>
> #define USB_VENDOR_ID_MICROSOFT 0x045e
> #define USB_DEVICE_ID_SIDEWINDER_GV 0x003b
>Le 11/06/2023 à 18:48, Johannes Roith a écrit :
>> 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.
>>
>> Signed-off-by: Johannes Roith <[email protected]>
>
>Hi,
>
>a few nits below, should it help the review.
>
>[...]
>
>> --- /dev/null
>> +++ b/drivers/hid/hid-mcp2200.c
>> @@ -0,0 +1,421 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * MCP2200 - Microchip USB to GPIO bridge
>> + *
>> + * Copyright (c) 2023, Johannes Roith <[email protected]>
>> + *
>> + * Datasheet: https://ww1.microchip.com/downloads/en/DeviceDoc/22228A.pdf
>> + * App Note for HID: https://ww1.microchip.com/downloads/en/DeviceDoc/93066A.pdf
>> + */
>> +#include <linux/module.h>
>> +#include <linux/err.h>
>> +#include <linux/mutex.h>
>> +#include <linux/bitfield.h>
>
>Is this include needed?
>
>> +#include <linux/completion.h>
>> +#include <linux/delay.h>
>> +#include <linux/hid.h>
>> +#include <linux/hidraw.h>
>> +#include <linux/gpio/driver.h>
>
>Many maintainers prefer alphabetic order for includes.
>
>
>> +#include "hid-ids.h"
>> +
>> +/* Commands codes in a raw output report */
>> +enum {
>> + SET_CLEAR_OUTPUTS = 0x08,
>> + CONFIGURE = 0x10,
>> + READ_EE = 0x20,
>> + WRITE_EE = 0x40,
>> + READ_ALL = 0x80
>> +};
>
>Does some
> #define xxx BIT(n)
>would make more sense than this enum?
I have changed the enum to defines. But I kept the hexadecimal format
as the commands comes from the HID appnote and I think it is more readable
that way.
>
>> +
>> +/* MCP GPIO direction encoding */
>> +enum MCP_IO_DIR {
>> + MCP2200_DIR_OUT = 0x00,
>> + MCP2200_DIR_IN = 0x01,
>> +};
>> +
>> +/* Altternative pin assignments */
>> +enum {
>> + TXLED = 2,
>> + RXLED = 3,
>> + USBCFG = 6,
>> + SSPND = 7,
>> +
>
>Uneeded new line.
>
>> +};
>> +
>> +#define MCP_NGPIO 8
>> +
>
>[...]
>
>> +/* this executes the READ_ALL cmd */
>> +static int mcp_cmd_read_all(struct mcp2200 *mcp)
>> +{
>> + struct mcp_read_all *read_all;
>> + int len, t;
>> +
>> + reinit_completion(&mcp->wait_in_report);
>> + mutex_lock(&mcp->lock);
>> +
>> + read_all = kzalloc(sizeof(struct mcp_read_all), GFP_KERNEL);
>> + if (!read_all)
>> + return -ENOMEM;
>
>Allocation could be deone before the lock.
>
>> +
>> + read_all->cmd = READ_ALL;
>> + len = hid_hw_output_report(mcp->hdev, (u8 *) read_all,
>> + sizeof(struct mcp_read_all));
>> +
>> + if (len != sizeof(struct mcp_read_all))
>
>kfree(read_all); ?
>(or move the call just below before the test)
>
>> + return -EINVAL;
>> +
>> + kfree(read_all);
>> + mutex_unlock(&mcp->lock);
>
>Mutex unlock could be done before kfree() or even before the "if
>(len..." a few lines above.
>
>> + t = wait_for_completion_timeout(&mcp->wait_in_report, msecs_to_jiffies(4000));
>> + if (!t)
>> + return -ETIMEDOUT;
>> +
>> + /* return status, negative value if wrong response was received */
>> + return mcp->status;
>> +}
>> +
>> +static void mcp_set_multiple(struct gpio_chip *gc, unsigned long *mask,
>> + unsigned long *bits)
>> +{
>> + struct mcp2200 *mcp = gpiochip_get_data(gc);
>> + u8 value;
>> + int status;
>> + struct mcp_set_clear_outputs *cmd;
>> +
>> + cmd = kzalloc(sizeof(struct mcp_set_clear_outputs), GFP_KERNEL);
>> + if (!cmd)
>> + return;
>> +
>> + mutex_lock(&mcp->lock);
>> +
>> + value = mcp->gpio_val & ~*mask;
>> + value |= (*mask & *bits);
>> +
>> + cmd->cmd = SET_CLEAR_OUTPUTS;
>> + cmd->set_bmap = value;
>> + cmd->clear_bmap = ~(value);
>> +
>> + status = hid_hw_output_report(mcp->hdev, (u8 *) cmd,
>> + sizeof(struct mcp_set_clear_outputs));
>> + if (status == sizeof(struct mcp_set_clear_outputs))
>> + mcp->gpio_val = value;
>> +
>> + kfree(cmd);
>> + mutex_unlock(&mcp->lock);
>
>Mutex unlock could be done before kfree().
>
>> +}
>> +
>> +static void mcp_set(struct gpio_chip *gc, unsigned int gpio_nr, int value)
>> +{
>> + unsigned long mask = (1 << gpio_nr);
>
>Uneeded ()
>Does using BIT makes sense here?
>
>> + unsigned long bmap_value = (value<<gpio_nr);
>
>Uneeded () and missing spaces aoud <<
>
>> +
>> + mcp_set_multiple(gc, &mask, &bmap_value);
>> +}
>> +
>> +static int mcp_get_multiple(struct gpio_chip *gc, unsigned long *mask,
>> + unsigned long *bits)
>> +{
>> + u32 val;
>> + struct mcp2200 *mcp = gpiochip_get_data(gc);
>> + int status;
>> +
>> + status = mcp_cmd_read_all(mcp);
>> + if (status != 0)
>> + return status;
>> +
>> + val = mcp->gpio_inval;
>> + *bits = (val & *mask);
>> + return 0;
>> +}
>> +
>> +static int mcp_get(struct gpio_chip *gc, unsigned int gpio_nr)
>> +{
>> + unsigned long mask = 0, bits = 0;
>
>No need to init long (and maybe bits)
>
>> +
>> + mask = (1 << gpio_nr);
>
>Uneeded ()
>Does using BIT makes sense here?
>
>> + mcp_get_multiple(gc, &mask, &bits);
>> + return (bits > 0) ? 1 : 0;
>> +}
>> +
>> +static int mcp_get_direction(struct gpio_chip *gc, unsigned int gpio_nr)
>> +{
>> + struct mcp2200 *mcp = gpiochip_get_data(gc);
>> +
>> + return (mcp->gpio_dir & (MCP2200_DIR_IN << gpio_nr))
>> + ? GPIO_LINE_DIRECTION_IN : GPIO_LINE_DIRECTION_OUT;
>> +}
>> +
>> +static int mcp_set_direction(struct gpio_chip *gc, unsigned int gpio_nr,
>> + enum MCP_IO_DIR io_direction)
>> +{
>> + struct mcp2200 *mcp = gpiochip_get_data(gc);
>> + struct mcp_configure *conf;
>> + int status;
>> + /* after the configure cmd we will need to set the outputs again */
>> + unsigned long mask = ~(mcp->gpio_dir); /* only set outputs */
>> + unsigned long bits = mcp->gpio_val;
>> + /* Offsets of alternative pins in config_alt_pins, 0 is not used */
>> + u8 alt_pin_conf[8] = {SSPND, USBCFG, 0, 0, 0, 0, RXLED, TXLED};
>> + u8 config_alt_pins = mcp->config_alt_pins;
>> +
>> + /* Read in the reset baudrate first, we need it later */
>> + status = mcp_cmd_read_all(mcp);
>> + if (status != 0)
>> + return status;
>> +
>> + conf = kzalloc(sizeof(struct mcp_configure), GFP_KERNEL);
>> + if (!conf)
>> + return -ENOMEM;
>> + mutex_lock(&mcp->lock);
>> +
>> + /* configure will reset the chip! */
>> + conf->cmd = CONFIGURE;
>> + conf->io_bmap = (mcp->gpio_dir & ~(1 << gpio_nr))
>> + | (io_direction << gpio_nr);
>> + /* Don't overwrite the reset parameters */
>> + conf->baud_h = mcp->baud_h;
>> + conf->baud_l = mcp->baud_l;
>> + conf->config_alt_options = mcp->config_alt_options;
>> + conf->io_default_val_bmap = mcp->gpio_reset_val;
>> + /* Adjust alt. func if necessary */
>> + if (alt_pin_conf[gpio_nr])
>> + config_alt_pins &= ~(1 << alt_pin_conf[gpio_nr]);
>> + conf->config_alt_pins = config_alt_pins;
>> +
>> + status = hid_hw_output_report(mcp->hdev, (u8 *) conf,
>> + sizeof(struct mcp_set_clear_outputs));
>> + if (status == sizeof(struct mcp_set_clear_outputs)) {
>> + mcp->gpio_dir &= ~(1 << gpio_nr);
>> + mcp->config_alt_pins = config_alt_pins;
>> + } else {
>> + return -EIO;
>> + }
>> +
>> + kfree(conf);
>> + mutex_unlock(&mcp->lock);
>
>Mutex unlock could be done before kfree().
>
>> +
>> + /* Configure CMD will clear all IOs -> rewrite them */
>> + mcp_set_multiple(gc, &mask, &bits);
>> + return 0;
>> +}
>> +
>> +static int mcp_direction_input(struct gpio_chip *gc, unsigned int gpio_nr)
>> +{
>> + return mcp_set_direction(gc, gpio_nr, MCP2200_DIR_IN);
>> +}
>> +
>> +static int mcp_direction_output(struct gpio_chip *gc, unsigned int gpio_nr,
>> + int value)
>> +{
>> + int ret;
>> + unsigned long mask, bmap_value;
>> +
>> + mask = (1 << gpio_nr);
>
>Uneeded ()
>Does using BIT makes sense here?
>
>> + bmap_value = (value << gpio_nr);
>
>Uneeded ()
>
>> +
>> + ret = mcp_set_direction(gc, gpio_nr, MCP2200_DIR_OUT);
>> + if (ret == 0)
>> + mcp_set_multiple(gc, &mask, &bmap_value);
>> + return ret;
>> +}
>> +
>> +
>
>No need for 2 new lines.
>
>[...]
>
>> +static int mcp2200_probe(struct hid_device *hdev, const struct hid_device_id *id)
>> +{
>> + int ret;
>> + struct mcp2200 *mcp;
>> +
>> + mcp = devm_kzalloc(&hdev->dev, sizeof(*mcp), GFP_KERNEL);
>> + if (!mcp)
>> + return -ENOMEM;
>> +
>> + ret = hid_parse(hdev);
>> + if (ret) {
>> + hid_err(hdev, "can't parse reports\n");
>> + return ret;
>> + }
>> +
>> + /*
>> + * This driver uses the .raw_event callback and therefore does not need any
>> + * HID_CONNECT_xxx flags.
>> + */
>> + ret = hid_hw_start(hdev, 0);
>> + if (ret) {
>> + hid_err(hdev, "can't start hardware\n");
>> + return ret;
>> + }
>> +
>> + hid_info(hdev, "USB HID v%x.%02x Device [%s] on %s\n", hdev->version >> 8,
>> + hdev->version & 0xff, hdev->name, hdev->phys);
>> +
>> + ret = hid_hw_open(hdev);
>> + if (ret) {
>> + hid_err(hdev, "can't open device\n");
>> + hid_hw_stop(hdev);
>> + return ret;
>> + }
>> +
>> + mutex_init(&mcp->lock);
>> + init_completion(&mcp->wait_in_report);
>> + hid_set_drvdata(hdev, mcp);
>> + mcp->hdev = hdev;
>> +
>> + ret = devm_add_action_or_reset(&hdev->dev, mcp2200_hid_unregister, hdev);
>> + if (ret)
>> + return ret;
>> +
>> + mcp->gc = template_chip;
>> + mcp->gc.parent = &hdev->dev;
>> +
>> + ret = gpiochip_add_data(&mcp->gc, mcp);
>
>devm_gpiochip_add_data() and no .remove function?
The gpiochip will be removed in mcp2200_remove
>
>> + if (ret < 0) {
>> + dev_err(&hdev->dev, "Unable to register gpiochip\n");
>
>hid_err() to be consistent?
>
>> + hid_hw_stop(hdev);
>
>hid_hw_stop() would be called twice. Once here and once because of the
>devm_add_action_or_reset() above.
I think it is only called once. It will be called when gpiochip_add_data or
hid_hw_open returns an error or when the mcp2200_hid_unregister gets called.
>
>
>Just my 2c,
>
>CJ
>
>> + return ret;
>> + }
>> +
>> + return 0;
>> +}
>
>[...]
>
Hi,
thanks for your feedback. I have added most of it in my Kernel module. For the
view things, I haven't added, I add a comment under your comment.
I am sorry for the error in hid-ids.h. It seems I forgot to run a git add hid-ids.h
to rename the second USB_DEVICE_ID_MCP2221 into USB_DEVICE_ID_MCP2200.
What do you think is the best way to deliver my driver? Should I create a new
single patch containg everything I have changed or should I go with the two
already created patches, reorder them, fixing the error in hid-ids.h and adding
a new patch including the comments from your review? This is my first
contribution, that's why I am asking.
Best regards,
Johannes
>Le 11/06/2023 à 18:48, Johannes Roith a écrit :
>> 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.
>>
>> Signed-off-by: Johannes Roith <[email protected]>
>
>Hi,
>
>a few nits below, should it help the review.
>
>[...]
>
>> --- /dev/null
>> +++ b/drivers/hid/hid-mcp2200.c
>> @@ -0,0 +1,421 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * MCP2200 - Microchip USB to GPIO bridge
>> + *
>> + * Copyright (c) 2023, Johannes Roith <[email protected]>
>> + *
>> + * Datasheet: https://ww1.microchip.com/downloads/en/DeviceDoc/22228A.pdf
>> + * App Note for HID: https://ww1.microchip.com/downloads/en/DeviceDoc/93066A.pdf
>> + */
>> +#include <linux/module.h>
>> +#include <linux/err.h>
>> +#include <linux/mutex.h>
>> +#include <linux/bitfield.h>
>
>Is this include needed?
>
>> +#include <linux/completion.h>
>> +#include <linux/delay.h>
>> +#include <linux/hid.h>
>> +#include <linux/hidraw.h>
>> +#include <linux/gpio/driver.h>
>
>Many maintainers prefer alphabetic order for includes.
>
>
>> +#include "hid-ids.h"
>> +
>> +/* Commands codes in a raw output report */
>> +enum {
>> + SET_CLEAR_OUTPUTS = 0x08,
>> + CONFIGURE = 0x10,
>> + READ_EE = 0x20,
>> + WRITE_EE = 0x40,
>> + READ_ALL = 0x80
>> +};
>
>Does some
> #define xxx BIT(n)
>would make more sense than this enum?
>
>> +
>> +/* MCP GPIO direction encoding */
>> +enum MCP_IO_DIR {
>> + MCP2200_DIR_OUT = 0x00,
>> + MCP2200_DIR_IN = 0x01,
>> +};
>> +
>> +/* Altternative pin assignments */
>> +enum {
>> + TXLED = 2,
>> + RXLED = 3,
>> + USBCFG = 6,
>> + SSPND = 7,
>> +
>
>Uneeded new line.
>
>> +};
>> +
>> +#define MCP_NGPIO 8
>> +
>
>[...]
>
>> +/* this executes the READ_ALL cmd */
>> +static int mcp_cmd_read_all(struct mcp2200 *mcp)
>> +{
>> + struct mcp_read_all *read_all;
>> + int len, t;
>> +
>> + reinit_completion(&mcp->wait_in_report);
>> + mutex_lock(&mcp->lock);
>> +
>> + read_all = kzalloc(sizeof(struct mcp_read_all), GFP_KERNEL);
>> + if (!read_all)
>> + return -ENOMEM;
>
>Allocation could be deone before the lock.
>
>> +
>> + read_all->cmd = READ_ALL;
>> + len = hid_hw_output_report(mcp->hdev, (u8 *) read_all,
>> + sizeof(struct mcp_read_all));
>> +
>> + if (len != sizeof(struct mcp_read_all))
>
>kfree(read_all); ?
>(or move the call just below before the test)
>
>> + return -EINVAL;
>> +
>> + kfree(read_all);
>> + mutex_unlock(&mcp->lock);
>
>Mutex unlock could be done before kfree() or even before the "if
>(len..." a few lines above.
>
>> + t = wait_for_completion_timeout(&mcp->wait_in_report, msecs_to_jiffies(4000));
>> + if (!t)
>> + return -ETIMEDOUT;
>> +
>> + /* return status, negative value if wrong response was received */
>> + return mcp->status;
>> +}
>> +
>> +static void mcp_set_multiple(struct gpio_chip *gc, unsigned long *mask,
>> + unsigned long *bits)
>> +{
>> + struct mcp2200 *mcp = gpiochip_get_data(gc);
>> + u8 value;
>> + int status;
>> + struct mcp_set_clear_outputs *cmd;
>> +
>> + cmd = kzalloc(sizeof(struct mcp_set_clear_outputs), GFP_KERNEL);
>> + if (!cmd)
>> + return;
>> +
>> + mutex_lock(&mcp->lock);
>> +
>> + value = mcp->gpio_val & ~*mask;
>> + value |= (*mask & *bits);
>> +
>> + cmd->cmd = SET_CLEAR_OUTPUTS;
>> + cmd->set_bmap = value;
>> + cmd->clear_bmap = ~(value);
>> +
>> + status = hid_hw_output_report(mcp->hdev, (u8 *) cmd,
>> + sizeof(struct mcp_set_clear_outputs));
>> + if (status == sizeof(struct mcp_set_clear_outputs))
>> + mcp->gpio_val = value;
>> +
>> + kfree(cmd);
>> + mutex_unlock(&mcp->lock);
>
>Mutex unlock could be done before kfree().
>
>> +}
>> +
>> +static void mcp_set(struct gpio_chip *gc, unsigned int gpio_nr, int value)
>> +{
>> + unsigned long mask = (1 << gpio_nr);
>
>Uneeded ()
>Does using BIT makes sense here?
>
>> + unsigned long bmap_value = (value<<gpio_nr);
>
>Uneeded () and missing spaces aoud <<
>
>> +
>> + mcp_set_multiple(gc, &mask, &bmap_value);
>> +}
>> +
>> +static int mcp_get_multiple(struct gpio_chip *gc, unsigned long *mask,
>> + unsigned long *bits)
>> +{
>> + u32 val;
>> + struct mcp2200 *mcp = gpiochip_get_data(gc);
>> + int status;
>> +
>> + status = mcp_cmd_read_all(mcp);
>> + if (status != 0)
>> + return status;
>> +
>> + val = mcp->gpio_inval;
>> + *bits = (val & *mask);
>> + return 0;
>> +}
>> +
>> +static int mcp_get(struct gpio_chip *gc, unsigned int gpio_nr)
>> +{
>> + unsigned long mask = 0, bits = 0;
>
>No need to init long (and maybe bits)
>
>> +
>> + mask = (1 << gpio_nr);
>
>Uneeded ()
>Does using BIT makes sense here?
>
>> + mcp_get_multiple(gc, &mask, &bits);
>> + return (bits > 0) ? 1 : 0;
>> +}
>> +
>> +static int mcp_get_direction(struct gpio_chip *gc, unsigned int gpio_nr)
>> +{
>> + struct mcp2200 *mcp = gpiochip_get_data(gc);
>> +
>> + return (mcp->gpio_dir & (MCP2200_DIR_IN << gpio_nr))
>> + ? GPIO_LINE_DIRECTION_IN : GPIO_LINE_DIRECTION_OUT;
>> +}
>> +
>> +static int mcp_set_direction(struct gpio_chip *gc, unsigned int gpio_nr,
>> + enum MCP_IO_DIR io_direction)
>> +{
>> + struct mcp2200 *mcp = gpiochip_get_data(gc);
>> + struct mcp_configure *conf;
>> + int status;
>> + /* after the configure cmd we will need to set the outputs again */
>> + unsigned long mask = ~(mcp->gpio_dir); /* only set outputs */
>> + unsigned long bits = mcp->gpio_val;
>> + /* Offsets of alternative pins in config_alt_pins, 0 is not used */
>> + u8 alt_pin_conf[8] = {SSPND, USBCFG, 0, 0, 0, 0, RXLED, TXLED};
>> + u8 config_alt_pins = mcp->config_alt_pins;
>> +
>> + /* Read in the reset baudrate first, we need it later */
>> + status = mcp_cmd_read_all(mcp);
>> + if (status != 0)
>> + return status;
>> +
>> + conf = kzalloc(sizeof(struct mcp_configure), GFP_KERNEL);
>> + if (!conf)
>> + return -ENOMEM;
>> + mutex_lock(&mcp->lock);
>> +
>> + /* configure will reset the chip! */
>> + conf->cmd = CONFIGURE;
>> + conf->io_bmap = (mcp->gpio_dir & ~(1 << gpio_nr))
>> + | (io_direction << gpio_nr);
>> + /* Don't overwrite the reset parameters */
>> + conf->baud_h = mcp->baud_h;
>> + conf->baud_l = mcp->baud_l;
>> + conf->config_alt_options = mcp->config_alt_options;
>> + conf->io_default_val_bmap = mcp->gpio_reset_val;
>> + /* Adjust alt. func if necessary */
>> + if (alt_pin_conf[gpio_nr])
>> + config_alt_pins &= ~(1 << alt_pin_conf[gpio_nr]);
>> + conf->config_alt_pins = config_alt_pins;
>> +
>> + status = hid_hw_output_report(mcp->hdev, (u8 *) conf,
>> + sizeof(struct mcp_set_clear_outputs));
>> + if (status == sizeof(struct mcp_set_clear_outputs)) {
>> + mcp->gpio_dir &= ~(1 << gpio_nr);
>> + mcp->config_alt_pins = config_alt_pins;
>> + } else {
>> + return -EIO;
>> + }
>> +
>> + kfree(conf);
>> + mutex_unlock(&mcp->lock);
>
>Mutex unlock could be done before kfree().
>
>> +
>> + /* Configure CMD will clear all IOs -> rewrite them */
>> + mcp_set_multiple(gc, &mask, &bits);
>> + return 0;
>> +}
>> +
>> +static int mcp_direction_input(struct gpio_chip *gc, unsigned int gpio_nr)
>> +{
>> + return mcp_set_direction(gc, gpio_nr, MCP2200_DIR_IN);
>> +}
>> +
>> +static int mcp_direction_output(struct gpio_chip *gc, unsigned int gpio_nr,
>> + int value)
>> +{
>> + int ret;
>> + unsigned long mask, bmap_value;
>> +
>> + mask = (1 << gpio_nr);
>
>Uneeded ()
>Does using BIT makes sense here?
>
>> + bmap_value = (value << gpio_nr);
>
>Uneeded ()
>
>> +
>> + ret = mcp_set_direction(gc, gpio_nr, MCP2200_DIR_OUT);
>> + if (ret == 0)
>> + mcp_set_multiple(gc, &mask, &bmap_value);
>> + return ret;
>> +}
>> +
>> +
>
>No need for 2 new lines.
>
>[...]
>
>> +static int mcp2200_probe(struct hid_device *hdev, const struct hid_device_id *id)
>> +{
>> + int ret;
>> + struct mcp2200 *mcp;
>> +
>> + mcp = devm_kzalloc(&hdev->dev, sizeof(*mcp), GFP_KERNEL);
>> + if (!mcp)
>> + return -ENOMEM;
>> +
>> + ret = hid_parse(hdev);
>> + if (ret) {
>> + hid_err(hdev, "can't parse reports\n");
>> + return ret;
>> + }
>> +
>> + /*
>> + * This driver uses the .raw_event callback and therefore does not need any
>> + * HID_CONNECT_xxx flags.
>> + */
>> + ret = hid_hw_start(hdev, 0);
>> + if (ret) {
>> + hid_err(hdev, "can't start hardware\n");
>> + return ret;
>> + }
>> +
>> + hid_info(hdev, "USB HID v%x.%02x Device [%s] on %s\n", hdev->version >> 8,
>> + hdev->version & 0xff, hdev->name, hdev->phys);
>> +
>> + ret = hid_hw_open(hdev);
>> + if (ret) {
>> + hid_err(hdev, "can't open device\n");
>> + hid_hw_stop(hdev);
>> + return ret;
>> + }
>> +
>> + mutex_init(&mcp->lock);
>> + init_completion(&mcp->wait_in_report);
>> + hid_set_drvdata(hdev, mcp);
>> + mcp->hdev = hdev;
>> +
>> + ret = devm_add_action_or_reset(&hdev->dev, mcp2200_hid_unregister, hdev);
>> + if (ret)
>> + return ret;
>> +
>> + mcp->gc = template_chip;
>> + mcp->gc.parent = &hdev->dev;
>> +
>> + ret = gpiochip_add_data(&mcp->gc, mcp);
>
>devm_gpiochip_add_data() and no .remove function?
>
>> + if (ret < 0) {
>> + dev_err(&hdev->dev, "Unable to register gpiochip\n");
>
>hid_err() to be consistent?
>
>> + hid_hw_stop(hdev);
>
>hid_hw_stop() would be called twice. Once here and once because of the
>devm_add_action_or_reset() above.
>
>
>Just my 2c,
>
>CJ
>
>> + return ret;
>> + }
>> +
>> + return 0;
>> +}
>
>[...]
>
Hi,
thanks for your feedback. I have added most of it in my Kernel module. For the
view things, I haven't added, I add a comment under your comment.
I am sorry for the error in hid-ids.h. It seems I forgot to run a git add hid-ids.h
to rename the second USB_DEVICE_ID_MCP2221 into USB_DEVICE_ID_MCP2200.
What do you think is the best way to deliver my driver? Should I create a new
patch containg everything I have changed or should I go with the two already
created patches, reorder them, fixing the error in hid-ids.h and adding a new
patch including the comments from your review? This is my first contribution,
that's why I am asking.
Best regards,
Johannes