Re: [PATCH v7 3/4] SFH: Transport Driver to add support of AMD Sensor Fusion Hub (SFH)
flat view
From: Andy Shevchenko <hidden>
Date: 2020-08-19 10:32:26
Also in:
linux-iio, lkml
On Tue, Aug 11, 2020 at 12:31 AM Sandeep Singh [off-list ref] wrote:
This part of module will provide the interaction between HID framework
of the module
and client layer.This module will registered client layer with
register
HID framework.
...
+/*
Is it kernel doc or not? Fix this in all occurrences.
quoted hunk ↗ jump to hunk
+ * amdtp_hid_parse() - hid-core .parse() callback + * @hid: hid device instance + * + * This function gets called during call to hid_add_device + * + * Return: 0 on success and non zero on error + */
...
quoted hunk ↗ jump to hunk
+static void amdtp_hid_request(struct hid_device *hid, struct hid_report *rep, int reqtype) +{ + int rc; + + switch (reqtype) { + case HID_REQ_GET_REPORT: + rc = amd_sfh_get_report(hid, rep->id, rep->type); + if (rc) + pr_err("AMDSFH get report error ");
Use dev_err() and similar in the other places.
quoted hunk ↗ jump to hunk
+ break; + case HID_REQ_SET_REPORT: + amd_sfh_set_report(hid, rep->id, reqtype); + break; + default: + break; + } +}
...
quoted hunk ↗ jump to hunk
+ if (!cli_data->request_done[i]) + ret = wait_event_interruptible_timeout(hid_data->hid_wait, + cli_data->request_done[i], 1500);
Magic number!
+ if (ret > 0) + return 0;
Usually we return errors first...
quoted hunk ↗ jump to hunk
+ else if (ret == -ERESTARTSYS) + return -ERESTARTSYS; + else + return -ETIMEDOUT; +}
...
+ hid = hid_allocate_device();
+ if (IS_ERR(hid)) {+ rc = PTR_ERR(hid); + return rc;
Why do you need two lines here?
+ }
...
+ hid_data = kzalloc(sizeof(*hid_data), GFP_KERNEL);
devm_kzalloc() ?
quoted hunk ↗ jump to hunk
+ if (!hid_data) { + rc = -ENOMEM; + goto err_hid_data; + }
...
+#define AMD_SFH_HID_VENDOR 1022
Decimal?!
+#define AMD_SFH_HID_PRODUCT 0x0001
...
quoted hunk ↗ jump to hunk
+#include <linux/dma-mapping.h> +#include <linux/hid.h> +#include <linux/list.h> +#include <linux/slab.h> +#include <linux/workqueue.h>
+#include <uapi/asm-generic/errno-base.h>
Huh?! linux/errno.h + blank line here.
quoted hunk ↗ jump to hunk
+#include "hid_descriptor/amd_sfh_hid_descriptor.h" +#include "amdsfh_hid.h" +#include "amd_mp2_pcie.h"
...
+#define PERIOD 200
Too generic name (namespace?), absence of unit. ...
+ for (i = 0; i < cli_data->num_hid_devices; i++) {
+ if (cli_data->hid_sensor_hubs[i] == hid) {if (!...) continue; ?
quoted hunk ↗ jump to hunk
+ struct request_list *new = kzalloc(sizeof(*new), GFP_KERNEL); + + if (!new) + return -ENOMEM; + new->current_index = i; + new->sensor_idx = cli_data->sensor_idx[i]; + new->hid = hid; + new->report_type = report_type; + new->report_id = report_id; + cli_data->report_id[i] = report_id; + cli_data->request_done[i] = false; + list_add(&new->list, &req_list.list); + break; + } + }
-- With Best Regards, Andy Shevchenko