[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <df1acc44-62c3-1ac6-103e-561baf8e38e4@wanadoo.fr>
Date: Fri, 28 Jul 2023 00:30:53 +0200
From: Christophe JAILLET <christophe.jaillet@...adoo.fr>
To: Johannes Roith <johannes@...-linux.rocks>, jikos@...nel.org,
benjamin.tissoires@...hat.com
Cc: linux-kernel@...r.kernel.org, linux-input@...r.kernel.org,
andi.shyti@...nel.org
Subject: Re: [PATCH] hid-mcp2200: added driver for GPIOs of MCP2200
Le 23/06/2023 à 13:01, 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.
>
> Feedback from reviewers Christophe JAILLET <christophe.jaillet@...adoo.fr>
> and Andi Shyti <andi.shyti@...nel.org> was added.
>
> Signed-off-by: Johannes Roith <johannes@...-linux.rocks>
> ---
[...]
> +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()?
> + if (ret < 0) {
> + dev_err(&hdev->dev, "Unable to register gpiochip\n");
hid_err() to be consistent?
> + hid_hw_stop(hdev);
Isn't it already run by mcp2200_hid_unregister() registered a few lines
above?
CJ
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static void mcp2200_remove(struct hid_device *hdev)
> +{
> + struct mcp2200 *mcp;
> +
> + mcp = hid_get_drvdata(hdev);
> + gpiochip_remove(&mcp->gc);
> +}
> +
[...]
Powered by blists - more mailing lists