Hi Rafi,
On Thu, Feb 11, 2010 at 07:19:03PM -0500, Rafi Rubin wrote:
> Added a quirk to enable distinct input devices. The digitizer utilizes
> three inputs to represent pen, multitouch and a normal touch screen.
>
> With the Pen partitioned, it behaves well and does not need special
> handling.
>
> Also, I set names to the input devices to clarify the functions of the
> various inputs.
>
Overall looks much nicer (goes for all 4 patches) and reader should be
able to see what is going on much easier now.
Couple of formatting nits still (I am pretty sure scripts/checkpatch.pl
warns about these as well):
> Signed-off-by: Rafi Rubin <rafi@xxxxxxxxxxxxxx>
> ---
> drivers/hid/hid-ntrig.c | 46 ++++++++++++++++++++++++++++++++++++++++++----
> 1 files changed, 42 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/hid/hid-ntrig.c b/drivers/hid/hid-ntrig.c
> index 49ce69d..4bb5ed0 100644
> --- a/drivers/hid/hid-ntrig.c
> +++ b/drivers/hid/hid-ntrig.c
> @@ -42,6 +42,10 @@ static int ntrig_input_mapping(struct hid_device *hdev, struct hid_input *hi,
> struct hid_field *field, struct hid_usage *usage,
> unsigned long **bit, int *max)
> {
> + /* No special mappings needed for the pen */
> + if(field->application == HID_DG_PEN)
> + return 0;
Please add space after keyword and before opening paren ("if (...)",
"While (...)", etc) here and elsewhere.
> +
> switch (usage->hid & HID_USAGE_PAGE) {
>
> case HID_UP_GENDESK:
> @@ -104,6 +108,9 @@ static int ntrig_input_mapped(struct hid_device *hdev, struct hid_input *hi,
> struct hid_field *field, struct hid_usage *usage,
> unsigned long **bit, int *max)
> {
> + /* No special mappings needed for the pen */
> + if(field->application == HID_DG_PEN)
> + return 0;
> if (usage->type == EV_KEY || usage->type == EV_REL
> || usage->type == EV_ABS)
> clear_bit(usage->code, *bit);
> @@ -123,6 +130,10 @@ static int ntrig_event (struct hid_device *hid, struct hid_field *field,
> struct input_dev *input = field->hidinput->input;
> struct ntrig_data *nd = hid_get_drvdata(hid);
>
> + /* No special handling needed for the pen */
> + if(field->application == HID_DG_PEN)
> + return 0;
> +
> if (hid->claimed & HID_CLAIMED_INPUT) {
> switch (usage->hid) {
>
> @@ -241,6 +252,11 @@ static int ntrig_probe(struct hid_device *hdev, const struct hid_device_id *id)
> {
> int ret;
> struct ntrig_data *nd;
> + struct hid_input *hidinput;
> + struct input_dev *input;
> +
> + if (id->driver_data)
> + hdev->quirks |= HID_QUIRK_MULTI_INPUT;
>
> nd = kmalloc(sizeof(struct ntrig_data), GFP_KERNEL);
> if (!nd) {
> @@ -255,14 +271,36 @@ static int ntrig_probe(struct hid_device *hdev, const struct hid_device_id *id)
> if (!ret)
> ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
>
> - if (ret)
> + if (ret) {
> kfree (nd);
What about hid_hw_stop()? Overall I'd rather see the standard error
unwinding path with gotos.
> + return ret;
> + }
> +
> + list_for_each_entry(hidinput, &hdev->inputs, list) {
> + input = hidinput->input;
> + switch (hidinput->report->field[0]->application) {
> + case HID_DG_PEN:
> + input->name = "N-Trig Pen";
> + break;
> + case HID_DG_TOUCHSCREEN:
> + /*
> + * The physical touchscreen (single touch) input
> + * has a value for physical, whereas the multitouch
> + * only has logical input fields.
> + */
> + if(hidinput->report->field[0]->physical) {
> + input->name = "N-Trig Touchscreen";
> + } else {
> + input->name = "N-Trig MultiTouch";
> + }
No need for curly braces for single-line statements. Maybe compress to
?: statement?
--
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
