Re: [PATCH v2] HID: corsair-void: Add Corsair Void headset family driver

[Markus Elfring]

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