Hi, Dmitry, Sorry, I misunderstood my code. It does not need to use global. I will send patch again. Best Regards, Masaki Ota -----Original Message----- From: 太田 真喜 Masaki Ota Sent: Tuesday, June 21, 2016 1:43 PM To: 'Dmitry Torokhov'; Masaki Ota Cc: jikos@xxxxxxxxxx; benjamin.tissorires@xxxxxxxxxx; peter.hutterer@xxxxxxxxx; hdegoede@xxxxxxxxxx; linux-input@xxxxxxxxxxxxxxx; 斉藤 直樹 Naoki Saito Subject: RE: [PATCH] Alps I2C HID Touchpad-Stick support Hi, Dmitry, > +struct u1_dev *priv; I do not understand why you need this global. You can allocate arbitrary memory in probe() and you actually already do that, why do you need second copy? Yes, actually I don't want to use global, but I want to use it in alps_raw_event() for StickPointer. Do you have any idea? About other your points, I will change the codes. Best Regards, Masaki Ota -----Original Message----- From: Dmitry Torokhov [mailto:dmitry.torokhov@xxxxxxxxx] Sent: Tuesday, June 21, 2016 12:39 PM To: Masaki Ota Cc: jikos@xxxxxxxxxx; benjamin.tissorires@xxxxxxxxxx; peter.hutterer@xxxxxxxxx; hdegoede@xxxxxxxxxx; linux-input@xxxxxxxxxxxxxxx; 太田 真喜 Masaki Ota; 斉藤 直樹 Naoki Saito Subject: Re: [PATCH] Alps I2C HID Touchpad-Stick support Hi Masaki, On Thu, Jun 16, 2016 at 06:45:57PM +0900, Masaki Ota wrote: > From Masaki Ota <masaki.ota@xxxxxxxxxxx> > > <ChangeList> > - Add hid-alps.c for Support Alps I2C HID Touchpad and Stick device. > - Add hid-alps.txt to check this device spec. > - Add Alps-JP vendor ID to hid-idh.h for support this device. > - To build this module, add Alps module definition to Makefile. > Even though Jiri has accepted the patch the driver can be improved further: ... > diff --git a/drivers/hid/hid-alps.c b/drivers/hid/hid-alps.c new file > mode 100644 index 0000000..b79c318 > --- /dev/null > +++ b/drivers/hid/hid-alps.c > @@ -0,0 +1,525 @@ > +/* > + * Copyright (c) 2016 Masaki Ota <masaki.ota@xxxxxxxxxxx> > + * > + * This program is free software; you can redistribute it and/or > +modify it > + * under the terms of the GNU General Public License as published by > +the Free > + * Software Foundation; either version 2 of the License, or (at your > +option) > + * any later version. > + */ > + > +#include <linux/kernel.h> > +#include <linux/hid.h> > +#include <linux/input.h> > +#include <linux/input/mt.h> > +#include <linux/module.h> > +#include <asm/unaligned.h> > +#include "hid-ids.h" > + > +/* ALPS Device Product ID */ > +#define HID_PRODUCT_ID_T3_BTNLESS 0xD0C0 > +#define HID_PRODUCT_ID_COSMO 0x1202 > +#define HID_PRODUCT_ID_U1_PTP_1 0x1207 > +#define HID_PRODUCT_ID_U1 0x1209 > +#define HID_PRODUCT_ID_U1_PTP_2 0x120A > +#define HID_PRODUCT_ID_U1_DUAL 0x120B > +#define HID_PRODUCT_ID_T4_BTNLESS 0x120C > + > +#define DEV_SINGLEPOINT 0x01 > +#define DEV_DUALPOINT 0x02 > + > +#define U1_MOUSE_REPORT_ID 0x01 /* Mouse data ReportID */ > +#define U1_ABSOLUTE_REPORT_ID 0x03 /* Absolute data ReportID */ > +#define U1_FEATURE_REPORT_ID 0x05 /* Feature ReportID */ > +#define U1_SP_ABSOLUTE_REPORT_ID 0x06 /* Feature ReportID */ > + > +#define U1_FEATURE_REPORT_LEN 0x08 /* Feature Report Length */ > +#define U1_FEATURE_REPORT_LEN_ALL 0x0A > +#define U1_CMD_REGISTER_READ 0xD1 > +#define U1_CMD_REGISTER_WRITE 0xD2 > + > +#define U1_DEVTYPE_SP_SUPPORT 0x10 /* SP Support */ > +#define U1_DISABLE_DEV 0x01 > +#define U1_TP_ABS_MODE 0x02 > +#define U1_SP_ABS_MODE 0x80 > + > +#define ADDRESS_U1_DEV_CTRL_1 0x00800040 > +#define ADDRESS_U1_DEVICE_TYP 0x00800043 > +#define ADDRESS_U1_NUM_SENS_X 0x00800047 > +#define ADDRESS_U1_NUM_SENS_Y 0x00800048 > +#define ADDRESS_U1_PITCH_SENS_X 0x00800049 > +#define ADDRESS_U1_PITCH_SENS_Y 0x0080004A > +#define ADDRESS_U1_RESO_DWN_ABS 0x0080004E > +#define ADDRESS_U1_PAD_BTN 0x00800052 > +#define ADDRESS_U1_SP_BTN 0x0080009F > + > +#define MAX_TOUCHES 5 > + > +/** > + * struct u1_data > + * > + * @input: pointer to the kernel input device > + * @input2: pointer to the kernel input2 device > + * @hdev: pointer to the struct hid_device > + * > + * @dev_ctrl: device control parameter > + * @dev_type: device type > + * @sen_line_num_x: number of sensor line of X > + * @sen_line_num_y: number of sensor line of Y > + * @pitch_x: sensor pitch of X > + * @pitch_y: sensor pitch of Y > + * @resolution: resolution > + * @btn_info: button information > + * @x_active_len_mm: active area length of X (mm) > + * @y_active_len_mm: active area length of Y (mm) > + * @x_max: maximum x coordinate value > + * @y_max: maximum y coordinate value > + * @btn_cnt: number of buttons > + * @sp_btn_cnt: number of stick buttons */ struct u1_dev { > + struct input_dev *input; > + struct input_dev *input2; > + struct hid_device *hdev; > + > + u8 dev_ctrl; > + u8 dev_type; > + u8 sen_line_num_x; > + u8 sen_line_num_y; > + u8 pitch_x; > + u8 pitch_y; > + u8 resolution; > + u8 btn_info; > + u8 sp_btn_info; > + u32 x_active_len_mm; > + u32 y_active_len_mm; > + u32 x_max; > + u32 y_max; > + u32 btn_cnt; > + u32 sp_btn_cnt; > +}; > + > +struct u1_dev *priv; I do not understand why you need this global. You can allocate arbitrary memory in probe() and you actually already do that, why do you need second copy? > + > +static int u1_read_write_register(struct hid_device *hdev, u32 address, > + u8 *read_val, u8 write_val, bool read_flag) { > + int ret, i; > + u8 check_sum; > + u8 *input; > + u8 *readbuf; > + > + input = kzalloc(sizeof(u8)*U1_FEATURE_REPORT_LEN, GFP_KERNEL); No need to do sizeof(u8) ever, it is always 1 as per standard. > + if (!input) > + return -ENOMEM; > + > + readbuf = kzalloc(sizeof(u8)*U1_FEATURE_REPORT_LEN, GFP_KERNEL); Why do we allocate readbuf even when !read_flag? > + if (!readbuf) { > + kfree(input); > + return -ENOMEM; > + } > + > + input[0] = U1_FEATURE_REPORT_ID; > + if (read_flag) { > + input[1] = U1_CMD_REGISTER_READ; > + input[6] = 0x00; > + } else { > + input[1] = U1_CMD_REGISTER_WRITE; > + input[6] = write_val; > + } > + > + put_unaligned_le32(address, input + 2); > + > + /* 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, > + sizeof(input), HID_FEATURE_REPORT, HID_REQ_SET_REPORT); Input is a pointer, so sizeof(input) is eitehr 4 or 8, not length of the buffer. > + > + if (ret < 0) { > + dev_err(&hdev->dev, "failed to read command (%d)\n", ret); > + goto exit; > + } > + > + if (read_flag) { > + ret = hid_hw_raw_request(hdev, U1_FEATURE_REPORT_ID, readbuf, > + sizeof(readbuf), HID_FEATURE_REPORT, Same here. > + HID_REQ_GET_REPORT); > + > + if (ret < 0) { > + dev_err(&hdev->dev, "failed read register (%d)\n", ret); > + goto exit; > + } > + > + *read_val = readbuf[6]; > + } > + > + kfree(input); > + kfree(readbuf); > + return 0; > + > +exit: > + kfree(input); > + kfree(readbuf); > + return ret; The error and success cases could be combined. > +} > + > +static int alps_raw_event(struct hid_device *hdev, > + struct hid_report *report, u8 *data, int size) { > + int x[MAX_TOUCHES], y[MAX_TOUCHES], z[MAX_TOUCHES]; > + int i, left, right, middle; > + short sp_x, sp_y, sp_z; > + struct u1_dev *hdata = hid_get_drvdata(hdev); > + > + switch (data[0]) { > + case U1_MOUSE_REPORT_ID: > + break; > + case U1_FEATURE_REPORT_ID: > + break; > + case U1_ABSOLUTE_REPORT_ID: > + for (i = 0; i < MAX_TOUCHES; i++) { > + x[i] = (data[3+(5*i)] | (data[4+(5*i)] << 8)); > + y[i] = (data[5+(5*i)] | (data[6+(5*i)] << 8)); get_unaligned_le16(). > + z[i] = data[7+(5*i)] & 0x7F; > + left = data[1] & 0x1; > + right = (data[1] & 0x2) >> 1; > + middle = (data[1] & 0x4) >> 2; Why do we calculate left/right/middle on each contact and not only once? > + > + input_mt_slot(hdata->input, i); > + > + if (z[i] != 0) { > + input_mt_report_slot_state(hdata->input, > + MT_TOOL_FINGER, 1); > + } else { > + input_mt_report_slot_state(hdata->input, > + MT_TOOL_FINGER, 0); > + break; > + } > + > + input_event(hdata->input, EV_ABS, > + ABS_MT_POSITION_X, x[i]); > + input_event(hdata->input, EV_ABS, > + ABS_MT_POSITION_Y, y[i]); > + input_event(hdata->input, EV_ABS, > + ABS_MT_PRESSURE, z[i]); > + } > + > + input_mt_sync_frame(hdata->input); > + input_sync(hdata->input); > + > + input_event(hdata->input, EV_KEY, BTN_LEFT, left); > + input_event(hdata->input, EV_KEY, BTN_RIGHT, right); > + input_event(hdata->input, EV_KEY, BTN_MIDDLE, middle); Why do we report buttons outside of event packet (i.e. after doing input_sync)? > + > + return 1; > + > + case U1_SP_ABSOLUTE_REPORT_ID: > + sp_x = (data[2] | (data[3] << 8)); > + sp_y = (data[4] | (data[5] << 8)); get_unaligned_le16() > + sp_z = (data[6] | data[7]) & 0x7FFF; > + left = data[1] & 0x1; > + right = (data[1] & 0x2) >> 1; > + middle = (data[1] & 0x4) >> 2; > + > + sp_x = sp_x / 8; > + sp_y = sp_y / 8; > + > + input_event(priv->input2, EV_REL, REL_X, sp_x); > + input_event(priv->input2, EV_REL, REL_Y, sp_y); > + > + input_event(priv->input2, EV_KEY, BTN_LEFT, left); > + input_event(priv->input2, EV_KEY, BTN_RIGHT, right); > + input_event(priv->input2, EV_KEY, BTN_MIDDLE, middle); > + > + input_sync(priv->input2); > + > + return 1; > + } > + > + return 0; > +} > + > +#ifdef CONFIG_PM > +static int alps_post_reset(struct hid_device *hdev) { > + return u1_read_write_register(hdev, ADDRESS_U1_DEV_CTRL_1, > + NULL, U1_TP_ABS_MODE, false); > +} > + > +static int alps_post_resume(struct hid_device *hdev) { > + return u1_read_write_register(hdev, ADDRESS_U1_DEV_CTRL_1, > + NULL, U1_TP_ABS_MODE, false); > +} > +#endif /* CONFIG_PM */ > + > +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 */ > + switch (hdev->product) { > + case HID_PRODUCT_ID_U1: > + case HID_PRODUCT_ID_U1_DUAL: > + break; > + default: > + return 0; > + } > + > + data->input = input; > + > + hid_dbg(hdev, "Opening low level driver\n"); > + ret = hid_hw_open(hdev); > + if (ret) > + return ret; > + > + /* Allow incoming hid reports */ > + hid_device_io_start(hdev); > + > + /* Device initialization */ > + ret = u1_read_write_register(hdev, ADDRESS_U1_DEV_CTRL_1, > + &devInfo.dev_ctrl, 0, true); > + if (ret < 0) { > + dev_err(&hdev->dev, "failed U1_DEV_CTRL_1 (%d)\n", ret); > + goto exit; > + } > + > + devInfo.dev_ctrl &= ~U1_DISABLE_DEV; > + devInfo.dev_ctrl |= U1_TP_ABS_MODE; > + ret = u1_read_write_register(hdev, ADDRESS_U1_DEV_CTRL_1, > + NULL, devInfo.dev_ctrl, false); > + if (ret < 0) { > + dev_err(&hdev->dev, "failed to change TP mode (%d)\n", ret); > + goto exit; > + } > + > + ret = u1_read_write_register(hdev, ADDRESS_U1_NUM_SENS_X, > + &devInfo.sen_line_num_x, 0, true); > + if (ret < 0) { > + dev_err(&hdev->dev, "failed U1_NUM_SENS_X (%d)\n", ret); > + goto exit; > + } > + > + ret = u1_read_write_register(hdev, ADDRESS_U1_NUM_SENS_Y, > + &devInfo.sen_line_num_y, 0, true); > + if (ret < 0) { > + dev_err(&hdev->dev, "failed U1_NUM_SENS_Y (%d)\n", ret); > + goto exit; > + } > + > + ret = u1_read_write_register(hdev, ADDRESS_U1_PITCH_SENS_X, > + &devInfo.pitch_x, 0, true); > + if (ret < 0) { > + dev_err(&hdev->dev, "failed U1_PITCH_SENS_X (%d)\n", ret); > + goto exit; > + } > + > + ret = u1_read_write_register(hdev, ADDRESS_U1_PITCH_SENS_Y, > + &devInfo.pitch_y, 0, true); > + if (ret < 0) { > + dev_err(&hdev->dev, "failed U1_PITCH_SENS_Y (%d)\n", ret); > + goto exit; > + } > + > + ret = u1_read_write_register(hdev, ADDRESS_U1_RESO_DWN_ABS, > + &devInfo.resolution, 0, true); > + if (ret < 0) { > + dev_err(&hdev->dev, "failed U1_RESO_DWN_ABS (%d)\n", ret); > + goto exit; > + } > + > + ret = u1_read_write_register(hdev, ADDRESS_U1_PAD_BTN, > + &devInfo.btn_info, 0, true); > + if (ret < 0) { > + dev_err(&hdev->dev, "failed U1_PAD_BTN (%d)\n", ret); > + goto exit; > + } > + > + /* Check StickPointer device */ > + ret = u1_read_write_register(hdev, ADDRESS_U1_DEVICE_TYP, > + &devInfo.dev_type, 0, true); > + if (ret < 0) { > + dev_err(&hdev->dev, "failed U1_DEVICE_TYP (%d)\n", ret); > + goto exit; > + } > + > + devInfo.x_active_len_mm = > + (devInfo.pitch_x * (devInfo.sen_line_num_x - 1)) / 10; > + devInfo.y_active_len_mm = > + (devInfo.pitch_y * (devInfo.sen_line_num_y - 1)) / 10; > + > + devInfo.x_max = > + (devInfo.resolution << 2) * (devInfo.sen_line_num_x - 1); > + devInfo.y_max = > + (devInfo.resolution << 2) * (devInfo.sen_line_num_y - 1); > + > + __set_bit(EV_ABS, input->evbit); > + input_set_abs_params(input, ABS_MT_POSITION_X, 1, devInfo.x_max, 0, 0); > + input_set_abs_params(input, ABS_MT_POSITION_Y, 1, devInfo.y_max, 0, > +0); > + > + if (devInfo.x_active_len_mm && devInfo.y_active_len_mm) { > + res_x = (devInfo.x_max - 1) / devInfo.x_active_len_mm; > + res_y = (devInfo.y_max - 1) / devInfo.y_active_len_mm; > + > + input_abs_set_res(input, ABS_MT_POSITION_X, res_x); > + input_abs_set_res(input, ABS_MT_POSITION_Y, res_y); > + } > + > + input_set_abs_params(input, ABS_MT_PRESSURE, 0, 64, 0, 0); > + > + input_mt_init_slots(input, MAX_TOUCHES, INPUT_MT_POINTER); > + > + __set_bit(EV_KEY, input->evbit); > + if ((devInfo.btn_info & 0x0F) == (devInfo.btn_info & 0xF0) >> 4) { > + devInfo.btn_cnt = (devInfo.btn_info & 0x0F); > + } else { > + /* Button pad */ > + devInfo.btn_cnt = 1; > + __set_bit(INPUT_PROP_BUTTONPAD, input->propbit); > + } > + > + for (i = 0; i < devInfo.btn_cnt; i++) > + __set_bit(BTN_LEFT + i, input->keybit); > + > + > + /* Stick device initialization */ > + if (devInfo.dev_type & U1_DEVTYPE_SP_SUPPORT) { > + > + priv = kzalloc(sizeof(struct u1_dev), GFP_KERNEL); > + if (!priv) { > + hid_device_io_stop(hdev); > + hid_hw_close(hdev); > + return -ENOMEM; > + } > + > + input2 = input_allocate_device(); > + if (!input2) { > + input_free_device(input2); > + goto exit; > + } > + > + priv->input2 = input2; > + > + devInfo.dev_ctrl |= U1_SP_ABS_MODE; > + ret = u1_read_write_register(hdev, ADDRESS_U1_DEV_CTRL_1, > + NULL, devInfo.dev_ctrl, false); > + if (ret < 0) { > + dev_err(&hdev->dev, "failed SP mode (%d)\n", ret); > + input_free_device(input2); > + goto exit; > + } > + > + ret = u1_read_write_register(hdev, ADDRESS_U1_SP_BTN, > + &devInfo.sp_btn_info, 0, true); > + if (ret < 0) { > + dev_err(&hdev->dev, "failed U1_SP_BTN (%d)\n", ret); > + input_free_device(input2); > + goto exit; > + } > + > + input2->phys = input->phys; > + input2->name = "DualPoint Stick"; > + input2->id.bustype = BUS_I2C; > + input2->id.vendor = input->id.vendor; > + input2->id.product = input->id.product; > + input2->id.version = input->id.version; > + input2->dev.parent = input->dev.parent; > + > + __set_bit(EV_KEY, input2->evbit); > + devInfo.sp_btn_cnt = (devInfo.sp_btn_info & 0x0F); > + for (i = 0; i < devInfo.sp_btn_cnt; i++) > + __set_bit(BTN_LEFT + i, input2->keybit); > + > + __set_bit(EV_REL, input2->evbit); > + __set_bit(REL_X, input2->relbit); > + __set_bit(REL_Y, input2->relbit); > + __set_bit(INPUT_PROP_POINTER, input2->propbit); > + __set_bit(INPUT_PROP_POINTING_STICK, input2->propbit); > + > + if (input_register_device(priv->input2)) { > + input_free_device(input2); > + goto exit; > + } > + } > + > +exit: > + hid_device_io_stop(hdev); > + hid_hw_close(hdev); > + return ret; > +} > + > +static int alps_input_mapping(struct hid_device *hdev, > + struct hid_input *hi, struct hid_field *field, > + struct hid_usage *usage, unsigned long **bit, int *max) { > + return -1; > +} > + > +static int alps_probe(struct hid_device *hdev, const struct > +hid_device_id *id) { > + struct u1_dev *data = NULL; > + int ret; > + > + data = devm_kzalloc(&hdev->dev, sizeof(struct u1_dev), GFP_KERNEL); > + if (!data) > + return -ENOMEM; > + > + data->hdev = hdev; > + hid_set_drvdata(hdev, data); > + > + hdev->quirks |= HID_QUIRK_NO_INIT_REPORTS; > + > + ret = hid_parse(hdev); > + if (ret) { > + hid_err(hdev, "parse failed\n"); > + return ret; > + } > + > + ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT); > + if (ret) { > + hid_err(hdev, "hw start failed\n"); > + return ret; > + } > + > + return 0; > +} > + > +static void alps_remove(struct hid_device *hdev) { > + hid_hw_stop(hdev); > + kfree(priv); > +} > + > +static const struct hid_device_id alps_id[] = { > + { HID_DEVICE(HID_BUS_ANY, HID_GROUP_ANY, > + USB_VENDOR_ID_ALPS_JP, HID_ANY_ID) }, > + { } > +}; > +MODULE_DEVICE_TABLE(hid, alps_id); > + > +static struct hid_driver alps_driver = { > + .name = "hid-alps", > + .id_table = alps_id, > + .probe = alps_probe, > + .remove = alps_remove, > + .raw_event = alps_raw_event, > + .input_mapping = alps_input_mapping, > + .input_configured = alps_input_configured, > +#ifdef CONFIG_PM > + .resume = alps_post_resume, > + .reset_resume = alps_post_reset, > +#endif > +}; > + > +module_hid_driver(alps_driver); > + > +MODULE_AUTHOR("Masaki Ota <masaki.ota@xxxxxxxxxxx>"); > +MODULE_DESCRIPTION("ALPS HID driver"); MODULE_LICENSE("GPL"); > diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h index > b6ff6e7..cac68c7 100644 > --- a/drivers/hid/hid-ids.h > +++ b/drivers/hid/hid-ids.h > @@ -67,6 +67,8 @@ > #define USB_VENDOR_ID_ALPS 0x0433 > #define USB_DEVICE_ID_IBM_GAMEPAD 0x1101 > > +#define USB_VENDOR_ID_ALPS_JP 0x044E > + > #define USB_VENDOR_ID_ANTON 0x1130 > #define USB_DEVICE_ID_ANTON_TOUCH_PAD 0x3101 > > -- > 2.7.4 > 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
