[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <bd07e14e-eae8-4264-b275-9efdf635cd82@web.de>
Date: Mon, 19 Aug 2024 09:48:15 +0200
From: Markus Elfring <Markus.Elfring@....de>
To: Stuart Hayhurst <stuart.a.hayhurst@...il.com>, linux-input@...r.kernel.org
Cc: LKML <linux-kernel@...r.kernel.org>,
Benjamin Tissoires <bentiss@...nel.org>, Jiri Kosina <jikos@...nel.org>
Subject: Re: [PATCH v2] HID: corsair-void: Add Corsair Void headset family
driver
…
> +++ b/drivers/hid/hid-corsair-void.c
> @@ -0,0 +1,857 @@
…
> +static ssize_t send_alert_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
…
> + unsigned char *send_buf;
* How do you think about to use the attribute “__free(kfree)” at more places accordingly?
https://elixir.bootlin.com/linux/v6.11-rc4/source/include/linux/slab.h#L282
* Would you like to reduce scopes for such local variables?
…
> + if (!drvdata->connected)
> + return -ENODEV;
> +
> + if (drvdata->is_wired)
> + return -ENODEV;
> +
> + if (kstrtou8(buf, 10, &alert_id))
> + return -EINVAL;
> +
> + /* Only accept 0 or 1 for alert ID */
> + if (alert_id >= 2)
> + return -EINVAL;
Can condition checks be merged with the same return value (for less statements)?
> + send_buf = kmalloc(3, GFP_KERNEL);
Can such a size determination be explained better?
…
> + kfree(send_buf);
> + return ret;
> +}
…
> +static void corsair_void_battery_add_work_handler(struct work_struct *work)
> +{
> + struct corsair_void_drvdata *drvdata;
…
> + drvdata->battery = power_supply_register(drvdata->dev,
> + &drvdata->battery_desc,
> + &psy_cfg);
> +
> + if (IS_ERR(drvdata->battery)) {
…
> + drvdata->battery = NULL;
I suggest to use another local variable for the previous return value
so that such a reset can be avoided.
> + return;
> + }
…
> +static DEVICE_ATTR_RO(fw_version_receiver);
> +static DEVICE_ATTR_RO(fw_version_headset);
> +static DEVICE_ATTR_RO(microphone_up);
> +static DEVICE_ATTR_RO(sidetone_max);
> +
> +static DEVICE_ATTR_WO(send_alert);
> +static DEVICE_ATTR_WO(set_sidetone);
> +
> +static struct attribute *corsair_void_attrs[] = {
…
> +};
> +
> +static const struct attribute_group corsair_void_attr_group = {
> + .attrs = corsair_void_attrs,
> +};
Is there a need to organise device attributes into separate subgroups?
…
> +static int corsair_void_probe(struct hid_device *hid_dev,
> + const struct hid_device_id *hid_id)
> +{
> + int ret = 0;
I propose to omit the explicit initialisation for this local variable.
> + struct corsair_void_drvdata *drvdata;
…
> + drvdata = devm_kzalloc(&hid_dev->dev, sizeof(struct corsair_void_drvdata),
> + GFP_KERNEL);
Please improve such a size determination.
See also:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?h=v6.11-rc4#n953
…
> + goto success;
Please apply the statement “return 0;” instead.
> +/*failed_after_hid_start:
> + hid_hw_stop(hid_dev);*/
Please reconsider the need once more also for this information.
Regards,
Markus
Powered by blists - more mailing lists