Re:Re: [PATCH 1/2] hid-mcp2200 added driver for MCP2200 GPIOs

Le 11/06/2023 �� 18:48, Johannes Roith a ��crit��:

quoted

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 <redacted>

Hi,

a few nits below, should it help the review.

[...]

quoted

--- /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 <johannes@gnu-linux.rocks>
+ *
+ * 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?

quoted

+#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.

quoted

+#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.

quoted

+
+/* 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.

quoted

+};
+
+#define MCP_NGPIO 8
+

[...]

quoted

+/* 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.

quoted

+
+	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)

quoted

+		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.

quoted

+	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().

quoted

+}
+
+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?

quoted

+	unsigned long bmap_value = (value<<gpio_nr);

Uneeded () and missing spaces aoud <<

quoted

+
+	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)

quoted

+
+	mask = (1 << gpio_nr);

Uneeded ()
Does using BIT makes sense here?

quoted

+	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().

quoted

+
+	/* 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?

quoted

+	bmap_value = (value << gpio_nr);

Uneeded ()

quoted

+
+	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.

[...]

quoted

+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

quoted

+	if (ret < 0) {
+		dev_err(&hdev->dev, "Unable to register gpiochip\n");

hid_err() to be consistent?

quoted

+		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

quoted

+		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