On Thu, May 26, 2016 at 01:12:19PM +0200, Jiri Kosina wrote:
> On Thu, 26 May 2016, Masaki Ota wrote:
>
> > Signed-off-by: Masaki Ota <masaki.ota@xxxxxxxxxxx>
>
> Thanks for the patch. However, please supply a changelog; the driver is
> doing non-trivial operations, so at least a bit of explanation (why
> generic HID can't be used, what are interesting high-level properties of
> the protocol, etc.) would be appreciated.
>
> > ---
> > drivers/hid/Makefile | 1 +
> > drivers/hid/hid-alps.c | 513 +++++++++++++++++++++++++++++++++++++++++++++++++
> > drivers/hid/hid-core.c | 5 +
> > drivers/hid/hid-ids.h | 2 +
> > include/linux/hid.h | 1 +
>
> You need to update Kconfig as well, otherwise CONFIG_HID_ALPS wouldn't be
> selectable.
>
> [ .. snip .. ]
> > +struct u1_dev *priv;
>
> Having this global is odd. How do you handle multiple devices connected to
> the machine? Is there a reason not to use hid_set_drvdata() /
> hid_get_drvdata() for this?
>
> [ .. snip .. ]
> > +static int u1_write_register(struct hid_device *hdev, u32 address, u8 value)
> > +{
> > + int ret, i;
> > + u8 input[8];
> > + u8 check_sum;
> > +
> > + input[0] = U1_FEATURE_REPORT_ID;
> > + input[1] = U1_CMD_REGISTER_WRITE;
> > + input[2] = address & 0x000000FF;
> > + input[3] = (address & 0x0000FF00) >> 8;
> > + input[4] = (address & 0x00FF0000) >> 16;
> > + input[5] = (address & 0xFF000000) >> 24;
put_unaligned_le32(address, input + 2);
> > + input[6] = value;
> > +
> > + /* Calculate the checksum */
> > + check_sum = U1_FEATURE_REPORT_LEN_ALL;
> > + for (i = 0; i < U1_FEATURE_REPORT_LEN - 1; i++)
> > + check_sum += input[i];
> > +
> > + input[7] = check_sum;
You should also factor this out as you are sharing pretty much the same
code with the u1_read_register().
> > +
> > + ret = hid_hw_raw_request(hdev, U1_FEATURE_REPORT_ID, input,
>
> Looks like on-stack DMA here.
>
> > +static int u1_read_register(struct hid_device *hdev, u32 address, u8 *value)
> > +{
> > + int ret, i;
> > + u8 input[8];
> > + u8 readbuf[8];
> > + u8 check_sum;
> > +
> > + input[0] = U1_FEATURE_REPORT_ID;
> > + input[1] = U1_CMD_REGISTER_READ;
> > + input[2] = address & 0x000000FF;
> > + input[3] = (address & 0x0000FF00) >> 8;
> > + input[4] = (address & 0x00FF0000) >> 16;
> > + input[5] = (address & 0xFF000000) >> 24;
> > + input[6] = 0x00;
> > +
> > + /* Calculate the checksum */
> > + check_sum = U1_FEATURE_REPORT_LEN_ALL;
> > + for (i = 0; i < U1_FEATURE_REPORT_LEN - 1; i++)
> > + check_sum += input[i];
> > +
> > + input[7] = check_sum;
> > +
> > + ret = hid_hw_raw_request(hdev, U1_FEATURE_REPORT_ID, input,
>
> The same here.
>
> [ .. snip .. ]
>
> > +static int alps_input_configured(struct hid_device *hdev, struct hid_input *hi)
> > +{
> > + struct u1_dev *data = hid_get_drvdata(hdev);
> > + struct input_dev *input = hi->input, *input2;
> > + struct u1_dev devInfo;
> > + int ret;
> > + int res_x, res_y, i;
> > +
> > + /* Check device product ID*/
>
> Nit: space before the second asterisk please.
>
> > + switch (hdev->product) {
> > + case HID_PRODUCT_ID_U1:
> > + case HID_PRODUCT_ID_U1_DUAL:
> > + break;
> > + default:
> > + return 0;
> > + }
> > +
> > + priv = kzalloc(sizeof(struct u1_dev), GFP_KERNEL);
> > + input2 = input_allocate_device();
> > + if (!priv || !input2)
> > + return 0;
>
> You leak memory in case the kzalloc() succeeds but input_allocate_device()
> fails.
Also we do not really need to allocate input device until we determine
that the device actually has trackstick. And I think we should be
returning -ENOMEM here.
Thanks.
--
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
