I'll post a patch for these based on Jiri's for-next tree this evening.
Dmitry Torokhov writes:
> Hi Michael,
>
> On Tue, Feb 09, 2010 at 08:13:08AM -0500, Michael Poole wrote:
>> +
>> +static bool emulate_3button = 1;
>
> If it is a bool then values should be true/false.
Thanks.
>> +module_param(emulate_3button, bool, 0644);
>> +MODULE_PARM_DESC(emulate_3button, "Emulate a middle button");
>> +
>> +static int middle_button_start = -350;
>> +static int middle_button_stop = +350;
>> +
>> +static bool emulate_scroll_wheel = 1;
>> +module_param(emulate_scroll_wheel, bool, 0644);
>> +MODULE_PARM_DESC(emulate_scroll_wheel, "Emulate a scroll wheel");
>> +
>> +static bool report_touches = 1;
>> +module_param(report_touches, bool, 0644);
>> +MODULE_PARM_DESC(report_touches, "Emit touch records (otherwise, only use them for emulation)");
>> +
>> +static bool report_undeciphered = 0;
>
> No need to initialize statics to 0/false.
Okay, I'll remote that.
[snip]
>> + int last_state = test_bit(BTN_LEFT, msc->input->key) << 0 |
>> + test_bit(BTN_RIGHT, msc->input->key) << 1 |
>> + test_bit(BTN_MIDDLE, msc->input->key) << 2;
>
> Indented with spaces, we prefer tabs.
Whoops -- one of my machines must be misconfigured. I'll fix that.
[snip]
>> + if (report_undeciphered) {
>> + input_event(input, EV_MSC, MSC_RAW, tdata[7]);
>> + }
>
> No need for braces for single statement.
Okay.
[snip]
>> + if (emulate_scroll_wheel)
>> + set_bit(REL_WHEEL, input->relbit);
>
> I'd use __set_bit() instead, no need to lock the bus.
Okay. I wasn't aware of that semantic difference.
>> +
>> + if (report_touches) {
>> + set_bit(EV_ABS, input->evbit);
>> +
>> + set_bit(ABS_MT_TRACKING_ID, input->absbit);
>> + input->absmin[ABS_MT_TRACKING_ID] = 0;
>> + input->absmax[ABS_MT_TRACKING_ID] = 15;
>> + input->absfuzz[ABS_MT_TRACKING_ID] = 0;
>
> input_set_abs_params() is a bit more compact.
Thanks -- more idiomatic is always handy.
[snip]
>> + ret = hdev->hid_output_raw_report(hdev, feature_1, sizeof(feature_1),
>> + HID_FEATURE_REPORT);
>
> This will cause transmission of on-stack data through USB which it does
> not like.
Current Magic Mouse devices are Bluetooth-only. Is this an issue for
that stack as well? (hid-sony.c and hid-wacom.c also use
hid_output_raw_report() with on-stack buffers for Bluetooth devices --
if they need it too, I can write a patch for those. I have a PS3
controller that I can test with, but don't have a Wacom tablet.) Would
making those buffers static put them in an appropriate section, or does
the memory need to be allocated from a heap?
>> + if (ret != sizeof(feature_1)) {
>> + dev_err(&hdev->dev, "unable to request touch data (1:%d)\n",
>> + ret);
>> + goto err_free;
>> + }
>> + ret = hdev->hid_output_raw_report(hdev, feature_2,
>> + sizeof(feature_2), HID_FEATURE_REPORT);
>
> Same here.
>
>> + if (ret != sizeof(feature_2)) {
>> + dev_err(&hdev->dev, "unable to request touch data (2:%d)\n",
>> + ret);
>> + goto err_free;
>> + }
>> +
>> + input = input_allocate_device();
>> + if (!input) {
>> + dev_err(&hdev->dev, "can't alloc input device\n");
>> + ret = -ENOMEM;
>> + goto err_free;
>> + }
>> + magicmouse_setup_input(input, hdev);
>> +
>> + ret = input_register_device(input);
>> + if (ret) {
>> + dev_err(&hdev->dev, "input device registration failed\n");
>> + goto err_both;
>> + }
>> + msc->input = input;
>> +
>> + return 0;
>> + err_both:
>> + input_free_device(input);
>> + err_free:
>> + kfree(msc);
>> + return ret;
>
> hid_hw_stop() is missing in error path. Also see question about freeing
> report below.
Thanks, I'll add the hid_hw_stop().
>> +}
>> +
>> +static void magicmouse_remove(struct hid_device *hdev)
>> +{
>
> Do we need to unregister report (I am not that familiar with HID, who is
> responsible for cleaning report lists?)?
hid-core.c's hid_device_release() frees all the reports that registered
with hid_register_report().
Michael Poole
--
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
