Message-ID: <87a5unt25h.fsf@protonmail.com>
Date:   Sat, 19 Aug 2023 16:42:07 +0000
From:   Rahul Rameshbabu <sergeantsagara@...tonmail.com>
To:     johannes@...-linux.rocks
Cc:     jikos@...nel.org, benjamin.tissoires@...hat.com,
        linux-kernel@...r.kernel.org, linux-input@...r.kernel.org,
        andi.shyti@...nel.org, christophe.jaillet@...adoo.fr,
        rdunlap@...radead.org, ak@...klinger.de
Subject: Re: [PATCH v5] hid-mcp2200: added driver for GPIOs of MCP2200

Commit message subject should probably start with "HID: mcp2200:"
rather than "hid-mcp2200:". This matches mcp2221.

  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/log/drivers/hid/hid-mcp2221.c

On Fri, 18 Aug, 2023 12:48:52 +0200 <johannes@...-linux.rocks> wrote:
> From: Johannes Roith <johannes@...-linux.rocks>
>
> 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.
>

The content below should not be in the commit message body but rather
git notes or a section in the patch that will not be applied.

> Feedback from reviewers Christophe JAILLET <christophe.jaillet@...adoo.fr>
> and Andi Shyti <andi.shyti@...nel.org> was added.
>
> Changelog:
>
> - v1: added driver
> - v2: added ProductID in hid-ids.h
> - v3: added feedback from review and make patch compilable
> - v4: added more feedback from second review, tested as module and
>   builtin on x86 and arm64
> - v5: adopted Kconfig to style guidelines, fixed mcp_set_direction

For your reference in case you find it helpful.

  * https://git-scm.com/docs/git-notes
  * https://git-scm.com/docs/git-format-patch#Documentation/git-format-patch.txt---notesltrefgt

>
> Signed-off-by: Johannes Roith <johannes@...-linux.rocks>
> ---
>  drivers/hid/Kconfig       |  10 +
>  drivers/hid/Makefile      |   1 +
>  drivers/hid/hid-ids.h     |   1 +
>  drivers/hid/hid-mcp2200.c | 419 ++++++++++++++++++++++++++++++++++++++
>  4 files changed, 431 insertions(+)
>  create mode 100644 drivers/hid/hid-mcp2200.c
>
> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> index e11c1c803676..efe01b473cc2 100644
> --- a/drivers/hid/Kconfig
> +++ b/drivers/hid/Kconfig
> @@ -1301,6 +1301,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.
> +

Alphabetically, HID_MCP2200 should be before HID_MCP2221.

>  config HID_KUNIT_TEST
>  	tristate "KUnit tests for HID" if !KUNIT_ALL_TESTS
>  	depends on KUNIT
> diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
> index 7a9e160158f7..050f740304c4 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

Similar comment as previous here.

>  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-ids.h b/drivers/hid/hid-ids.h
> index 8a310f8ff20f..4926ca41225b 100644
> --- a/drivers/hid/hid-ids.h
> +++ b/drivers/hid/hid-ids.h
> @@ -914,6 +914,7 @@
>  #define USB_DEVICE_ID_PICK16F1454_V2	0xf2f7
>  #define USB_DEVICE_ID_LUXAFOR		0xf372
>  #define USB_DEVICE_ID_MCP2221		0x00dd
> +#define USB_DEVICE_ID_MCP2200		0x00df

This file is sorted primarily by the define name rather than the device
id, so I would expect the define for mcp2200 to be before mcp2221 (even
though its device id would be after).

  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/hid/hid-ids.h?id=aa9ea98cca3a56869df1dc6347f3e3bfe0c01f44#n449

>
>  #define USB_VENDOR_ID_MICROSOFT		0x045e
>  #define USB_DEVICE_ID_SIDEWINDER_GV	0x003b
> diff --git a/drivers/hid/hid-mcp2200.c b/drivers/hid/hid-mcp2200.c
> new file mode 100644
> index 000000000000..ed4483fa5bdf
> --- /dev/null
> +++ b/drivers/hid/hid-mcp2200.c
> @@ -0,0 +1,419 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * MCP2200 - Microchip USB to GPIO bridge
> + *
> + * Copyright (c) 2023, Johannes Roith <johannes@...-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/completion.h>
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/gpio/driver.h>
> +#include <linux/hid.h>
> +#include <linux/hidraw.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include "hid-ids.h"
> +
> +/* Commands codes in a raw output report */
> +#define SET_CLEAR_OUTPUTS	0x08
> +#define CONFIGURE		0x10
> +#define READ_EE			0x20
> +#define WRITE_EE		0x40
> +#define READ_ALL		0x80
> +
> +/* MCP GPIO direction encoding */
> +enum MCP_IO_DIR {
> +	MCP2200_DIR_OUT = 0x00,
> +	MCP2200_DIR_IN  = 0x01,
> +};
> +
> +/* Altternative pin assignments */
> +#define TXLED		2
> +#define RXLED		3
> +#define USBCFG		6
> +#define 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);
> +	read_all = kzalloc(sizeof(struct mcp_read_all), GFP_KERNEL);
> +	if (!read_all)
> +		return -ENOMEM;

My personal recommendation is to just have a single DMA buffer allocated
for the mcp2200 instance rather than having to call the allocator and
release the memory per command.

Here is a reference.

  https://git.kernel.org/pub/scm/linux/kernel/git/hid/hid.git/tree/drivers/hid/hid-nvidia-shield.c?id=1d7546042f8fdc4bc39ab91ec966203e2d64f8bd#n502

> +
> +	read_all->cmd = READ_ALL;
> +
> +	mutex_lock(&mcp->lock);
> +	len = hid_hw_output_report(mcp->hdev, (u8 *) read_all,
> +				   sizeof(struct mcp_read_all));
> +
> +	mutex_unlock(&mcp->lock);
> +	kfree(read_all);
> +
> +	if (len != sizeof(struct mcp_read_all))
> +		return -EINVAL;
> +
> +	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;
> +
> +	mutex_unlock(&mcp->lock);
> +	kfree(cmd);
> +}
> +
> +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)

Can be simplified as 'if (status)'.

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

You do not need to zero init these variables? It's useful to avoid
initializing variables unnecessarily for compile time checking/static
analysis purposes.

> +
> +	mask = (1 << gpio_nr);
> +	mcp_get_multiple(gc, &mask, &bits);
> +	return bits > 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 = conf->io_bmap;
> +		mcp->config_alt_pins = config_alt_pins;
> +	} else {
> +		mutex_unlock(&mcp->lock);
> +		kfree(conf);
> +		return -EIO;
> +	}
> +
> +	mutex_unlock(&mcp->lock);
> +	kfree(conf);
> +
> +	/* 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)

'if (!ret)' can be used here.

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

While this is not problematic today, I think it would be better to
return 0 here rather than 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) {

Need to call hid_hw_stop here since hid_hw_start succeeded.

> +		hid_err(hdev, "can't open device\n");
> +		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)

You probably want to call mcp2200_hid_unregister here since the action
was not added?

> +		return ret;
> +
> +	mcp->gc = template_chip;
> +	mcp->gc.parent = &hdev->dev;
> +
> +	ret = devm_gpiochip_add_data(&hdev->dev, &mcp->gc, mcp);
> +	if (ret < 0) {
> +		hid_err(hdev, "Unable to register gpiochip\n");
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static void mcp2200_remove(struct hid_device *hdev)
> +{
> +	(void) hdev;
> +	/*
> +	 * With no remove function you sometimes get a segmentation fault when
> +	 * unloading the module or disconnecting the USB device
> +	 */

The reason you run into this is likely because of the action added to
devm conflicting with hid_device_remove....

I recommend not depending on devm for teardown rather than making a stub
remove function to work around the issue.

  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.

> +}
> +
> +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 <johannes@...-linux.rocks>");
> +MODULE_DESCRIPTION("MCP2200 Microchip HID USB to GPIO bridge");
> +MODULE_LICENSE("GPL");

--
Thanks,

Rahul Rameshbabu

Powered by blists - more mailing lists