[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <2ab875b8216dd32d0d1e495a52a20c02a40e3e5d.camel@hexdev.de>
Date: Thu, 02 May 2024 12:41:07 +0200
From: Christoph Fritz <christoph.fritz@...dev.de>
To: Jiri Slaby <jirislaby@...nel.org>
Cc: Oliver Hartkopp <socketcan@...tkopp.net>, Marc Kleine-Budde
<mkl@...gutronix.de>, Vincent Mailhol <mailhol.vincent@...adoo.fr>, "David
S . Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, Rob
Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor
Dooley <conor+dt@...nel.org>, Jiri Kosina <jikos@...nel.org>, Benjamin
Tissoires <bentiss@...nel.org>, Greg Kroah-Hartman
<gregkh@...uxfoundation.org>, Sebastian Reichel <sre@...nel.org>, Linus
Walleij <linus.walleij@...aro.org>, Andreas Lauser
<andreas.lauser@...cedes-benz.com>, Jonathan Corbet <corbet@....net>,
Pavel Pisa <pisa@....felk.cvut.cz>, linux-can@...r.kernel.org,
netdev@...r.kernel.org, devicetree@...r.kernel.org,
linux-input@...r.kernel.org, linux-serial@...r.kernel.org
Subject: Re: [PATCH v2 02/12] HID: hexLIN: Add support for USB LIN bus
adapter
On Thu, 2024-05-02 at 10:30 +0200, Jiri Slaby wrote:
> On 02. 05. 24, 9:55, Christoph Fritz wrote:
> > This patch introduces driver support for the hexLIN USB LIN bus adapter,
> > enabling LIN communication over USB for both controller and responder
> > modes. The driver interfaces with the CAN_LIN framework for userland
> > connectivity.
> >
> > For more details on the adapter, visit: https://hexdev.de/hexlin/
> >
> > Tested-by: Andreas Lauser <andreas.lauser@...cedes-benz.com>
> > Signed-off-by: Christoph Fritz <christoph.fritz@...dev.de>
> ...
> > --- /dev/null
> > +++ b/drivers/hid/hid-hexdev-hexlin.c
> > @@ -0,0 +1,630 @@
> ...
> > +static int hexlin_stop(struct lin_device *ldev)
> > +{
> > + struct hid_device *hdev = to_hid_device(ldev->dev);
> > + struct hexlin_priv_data *priv = hid_get_drvdata(hdev);
> > +
> > + hid_hw_close(hdev);
> > +
> > + priv->is_error = true;
> > + complete(&priv->wait_in_report);
> > +
> > + mutex_lock(&priv->tx_lock);
> > + mutex_unlock(&priv->tx_lock);
>
> This is a weird way to implement a completion. It looks like you need
> another one.
They are not necessary, even more so when I can drop the
mutex_destroy() below.
>
> > + return 0;
> > +}
> ...> +static int hexlin_probe(struct hid_device *hdev,
> > + const struct hid_device_id *id)
> > +{
> > + struct hexlin_priv_data *priv;
> > + int ret;
> > +
> > + priv = devm_kzalloc(&hdev->dev, sizeof(*priv), GFP_KERNEL);
> > + if (!priv)
> > + return -ENOMEM;
> > +
> > + priv->hid_dev = hdev;
> > + hid_set_drvdata(hdev, priv);
> > +
> > + mutex_init(&priv->tx_lock);
> > +
> > + ret = hid_parse(hdev);
> > + if (ret) {
> > + hid_err(hdev, "hid parse failed with %d\n", ret);
> > + goto fail_and_free;
> > + }
> > +
> > + ret = hid_hw_start(hdev, HID_CONNECT_DRIVER);
> > + if (ret) {
> > + hid_err(hdev, "hid hw start failed with %d\n", ret);
> > + goto fail_and_stop;
> > + }
> > +
> > + ret = hid_hw_open(hdev);
> > + if (ret) {
> > + hid_err(hdev, "hid hw open failed with %d\n", ret);
> > + goto fail_and_close;
> > + }
> > +
> > + init_completion(&priv->wait_in_report);
> > +
> > + hid_device_io_start(hdev);
> > +
> > + ret = init_hw(priv);
> > + if (ret)
> > + goto fail_and_close;
> > +
> > + priv->ldev = register_lin(&hdev->dev, &hexlin_ldo);
> > + if (IS_ERR_OR_NULL(priv->ldev)) {
> > + ret = PTR_ERR(priv->ldev);
> > + goto fail_and_close;
> > + }
> > +
> > + hid_hw_close(hdev);
> > +
> > + hid_info(hdev, "hexLIN (fw-version: %u) probed\n", priv->fw_version);
> > +
> > + return 0;
> > +
> > +fail_and_close:
> > + hid_hw_close(hdev);
> > +fail_and_stop:
> > + hid_hw_stop(hdev);
> > +fail_and_free:
> > + mutex_destroy(&priv->tx_lock);
> > + return ret;
> > +}
> > +
> > +static void hexlin_remove(struct hid_device *hdev)
> > +{
> > + struct hexlin_priv_data *priv = hid_get_drvdata(hdev);
> > +
> > + unregister_lin(priv->ldev);
> > + hid_hw_stop(hdev);
> > + mutex_destroy(&priv->tx_lock);
>
> It is unusual to destroy a mutex. Why do you do that?
>
Just for code clarity and it should help if someone wants to use
CONFIG_DEBUG_MUTEXES.
To be able to drop the lock/unlock from above, I could add the
lock/unlock here or just drop the mutex_destroy() completely.
I'll just drop it in upcoming v3.
> > +}
> ...
> > +static int __init hexlin_init(void)
> > +{
> > + return hid_register_driver(&hexlin_driver);
> > +}
> > +
> > +static void __exit hexlin_exit(void)
> > +{
> > + hid_unregister_driver(&hexlin_driver);
> > +}
>
>
>
> > +
> > +/*
> > + * When compiled into the kernel, initialize after the hid bus.
> > + */
> > +late_initcall(hexlin_init);
>
> Hmm, why not module_init() then? (And module_hid_driver().)
Looking at the other hid drivers and testing with just
module_hid_driver(hexlin_driver)
works here fine for compiled into the kernel and as a module.
>
> > +module_exit(hexlin_exit);
> > +
> > +MODULE_LICENSE("GPL");
> > +MODULE_AUTHOR("Christoph Fritz <christoph.fritz@...dev.de>");
> > +MODULE_DESCRIPTION("LIN bus driver for hexLIN USB adapter");
Thanks
-- Christoph
Powered by blists - more mailing lists