Re: [PATCH 5/7] Support new Alps device that name is T4

[Jiri Kosina]

On Tue, 12 Sep 2017, Masaki Ota wrote:

> From: Masaki Ota 
> -Add T4 device code and Product ID
> -This device is used on HP EliteBook 1000 series and Zbook Stduio
[ ... snip ... ]
> Signed-off-by: Masaki Ota 
> +static int t4_read_write_register(struct hid_device *hdev, u32 address,
> + u8 *read_val, u8 write_val, bool read_flag)
> +{
> + int ret;
> + u16 check_sum;
> + u8 *input;
> + u8 *readbuf;
> +
> + input = kzalloc(T4_FEATURE_REPORT_LEN, GFP_KERNEL);
> + if (!input)
> + return -ENOMEM;
> +
> + input[0] = T4_FEATURE_REPORT_ID;
> + if (read_flag) {
> + input[1] = T4_CMD_REGISTER_READ;
> + input[8] = 0x00;
> + } else {
> + input[1] = T4_CMD_REGISTER_WRITE;
> + input[8] = write_val;
> + }
> + put_unaligned_le32(address, input + 2);
> + input[6] = 1;
> + input[7] = 0;
> +
> + /* Calculate the checksum */
> + check_sum = t4_calc_check_sum(input, 1, 8);
> + input[9] = (u8)check_sum;
> + input[10] = (u8)(check_sum >> 8);
> + input[11] = 0;
> +
> + ret = hid_hw_raw_request(hdev, T4_FEATURE_REPORT_ID, input,
> + T4_FEATURE_REPORT_LEN,
> + HID_FEATURE_REPORT, HID_REQ_SET_REPORT);
> +
> + if (ret < 0) {
> + dev_err(>dev, "failed to read command (%d)\n", ret);
> + goto exit;
> + }
> +
> + if (read_flag) {
> + readbuf = kzalloc(T4_FEATURE_REPORT_LEN, GFP_KERNEL);
> + if (!readbuf) {
> + ret = -ENOMEM;
> + goto exit;
> + }
> +
> + ret = hid_hw_raw_request(hdev, T4_FEATURE_REPORT_ID, readbuf,
> + T4_FEATURE_REPORT_LEN,
> + HID_FEATURE_REPORT, HID_REQ_GET_REPORT);
> + if (ret < 0) {
> + dev_err(>dev, "failed read register (%d)\n", ret);
> + kfree(readbuf);
> + goto exit;
> + }
> +
> + if (*(u32 *)[6] != address) {
> + dev_err(>dev, "read register address error 
> (%x,%x)\n",
> + *(u32 *)[6], address);
> + kfree(readbuf);
> + goto exit;
> + }
> +
> + if (*(u16 *)[10] != 1) {
> + dev_err(>dev, "read register size error (%x)\n",
> + *(u16 *)[10]);
> + kfree(readbuf);
> + goto exit;
> + }
> +
> + check_sum = t4_calc_check_sum(readbuf, 6, 7);
> + if (*(u16 *)[13] != check_sum) {
> + dev_err(>dev, "read register checksum error 
> (%x,%x)\n",
> + *(u16 *)[13], check_sum);
> + kfree(readbuf);
> + goto exit;
> + }
> +
> + *read_val = readbuf[12];
> +
> + kfree(readbuf);

You have a lot of

kfree(readbuf);
goto exit;

duplicates here, and you're freeing the buffer before returning anyway in 
all cases, so it'd make sense to introduce another label (say 
exit_readbuf) before the exit one, and free it there in a common exit 
path.

[ ... snip ... ]
> +static int t4_raw_event(struct alps_dev *hdata, u8 *data, int size)
> +{
> + unsigned int x, y, z;
> + int i;
> + struct t4_input_report *p_report = (struct t4_input_report *)data;
> +
> + if (!data)
> + return 0;
> + for (i = 0; i < hdata->max_fingers; i++) {
> + x = p_report->contact[i].x_hi << 8 | p_report->contact[i].x_lo;
> + y = p_report->contact[i].y_hi << 8 | p_report->contact[i].y_lo;
> + y = hdata->y_max - y + hdata->y_min;
> + z = (p_report->contact[i].palm < 0x80 &&
> + p_report->contact[i].palm > 0) * 62;
> + if (x == 0x) {
> + x = 0;
> + y = 0;
> + z = 0;
> + }
> + input_mt_slot(hdata->input, i);
> +
> + input_mt_report_slot_state(hdata->input,
> + MT_TOOL_FINGER, z != 0);
> +
> + if (!z)
> + continue;
> +
> + input_report_abs(hdata->input, ABS_MT_POSITION_X, x);
> + input_report_abs(hdata->input, ABS_MT_POSITION_Y, y);
> + input_report_abs(hdata->input, ABS_MT_PRESSURE, z);
> + }
> + input_mt_sync_frame(hdata->input);
> +
> + input_report_key(hdata->input, BTN_LEFT,p_report->button);

Extra tab here?

> -static int alps_post_resume(struct hid_device *hdev)
> +static int __maybe_unused alps_post_reset(struct hid_device *hdev)

Do we really need the __maybe_unused annotation here? Why not just hide 
the whole  post-reset callback handling into a #ifdef CONFIG_PM block?

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH 5/7] Support new Alps device that name is T4

[Jiri Kosina]

On Tue, 12 Sep 2017, Masaki Ota wrote:

> From: Masaki Ota 
> -Add T4 device code and Product ID
> -This device is used on HP EliteBook 1000 series and Zbook Stduio
[ ... snip ... ]
> Signed-off-by: Masaki Ota 
> +static int t4_read_write_register(struct hid_device *hdev, u32 address,
> + u8 *read_val, u8 write_val, bool read_flag)
> +{
> + int ret;
> + u16 check_sum;
> + u8 *input;
> + u8 *readbuf;
> +
> + input = kzalloc(T4_FEATURE_REPORT_LEN, GFP_KERNEL);
> + if (!input)
> + return -ENOMEM;
> +
> + input[0] = T4_FEATURE_REPORT_ID;
> + if (read_flag) {
> + input[1] = T4_CMD_REGISTER_READ;
> + input[8] = 0x00;
> + } else {
> + input[1] = T4_CMD_REGISTER_WRITE;
> + input[8] = write_val;
> + }
> + put_unaligned_le32(address, input + 2);
> + input[6] = 1;
> + input[7] = 0;
> +
> + /* Calculate the checksum */
> + check_sum = t4_calc_check_sum(input, 1, 8);
> + input[9] = (u8)check_sum;
> + input[10] = (u8)(check_sum >> 8);
> + input[11] = 0;
> +
> + ret = hid_hw_raw_request(hdev, T4_FEATURE_REPORT_ID, input,
> + T4_FEATURE_REPORT_LEN,
> + HID_FEATURE_REPORT, HID_REQ_SET_REPORT);
> +
> + if (ret < 0) {
> + dev_err(>dev, "failed to read command (%d)\n", ret);
> + goto exit;
> + }
> +
> + if (read_flag) {
> + readbuf = kzalloc(T4_FEATURE_REPORT_LEN, GFP_KERNEL);
> + if (!readbuf) {
> + ret = -ENOMEM;
> + goto exit;
> + }
> +
> + ret = hid_hw_raw_request(hdev, T4_FEATURE_REPORT_ID, readbuf,
> + T4_FEATURE_REPORT_LEN,
> + HID_FEATURE_REPORT, HID_REQ_GET_REPORT);
> + if (ret < 0) {
> + dev_err(>dev, "failed read register (%d)\n", ret);
> + kfree(readbuf);
> + goto exit;
> + }
> +
> + if (*(u32 *)[6] != address) {
> + dev_err(>dev, "read register address error 
> (%x,%x)\n",
> + *(u32 *)[6], address);
> + kfree(readbuf);
> + goto exit;
> + }
> +
> + if (*(u16 *)[10] != 1) {
> + dev_err(>dev, "read register size error (%x)\n",
> + *(u16 *)[10]);
> + kfree(readbuf);
> + goto exit;
> + }
> +
> + check_sum = t4_calc_check_sum(readbuf, 6, 7);
> + if (*(u16 *)[13] != check_sum) {
> + dev_err(>dev, "read register checksum error 
> (%x,%x)\n",
> + *(u16 *)[13], check_sum);
> + kfree(readbuf);
> + goto exit;
> + }
> +
> + *read_val = readbuf[12];
> +
> + kfree(readbuf);

You have a lot of

kfree(readbuf);
goto exit;

duplicates here, and you're freeing the buffer before returning anyway in 
all cases, so it'd make sense to introduce another label (say 
exit_readbuf) before the exit one, and free it there in a common exit 
path.

[ ... snip ... ]
> +static int t4_raw_event(struct alps_dev *hdata, u8 *data, int size)
> +{
> + unsigned int x, y, z;
> + int i;
> + struct t4_input_report *p_report = (struct t4_input_report *)data;
> +
> + if (!data)
> + return 0;
> + for (i = 0; i < hdata->max_fingers; i++) {
> + x = p_report->contact[i].x_hi << 8 | p_report->contact[i].x_lo;
> + y = p_report->contact[i].y_hi << 8 | p_report->contact[i].y_lo;
> + y = hdata->y_max - y + hdata->y_min;
> + z = (p_report->contact[i].palm < 0x80 &&
> + p_report->contact[i].palm > 0) * 62;
> + if (x == 0x) {
> + x = 0;
> + y = 0;
> + z = 0;
> + }
> + input_mt_slot(hdata->input, i);
> +
> + input_mt_report_slot_state(hdata->input,
> + MT_TOOL_FINGER, z != 0);
> +
> + if (!z)
> + continue;
> +
> + input_report_abs(hdata->input, ABS_MT_POSITION_X, x);
> + input_report_abs(hdata->input, ABS_MT_POSITION_Y, y);
> + input_report_abs(hdata->input, ABS_MT_PRESSURE, z);
> + }
> + input_mt_sync_frame(hdata->input);
> +
> + input_report_key(hdata->input, BTN_LEFT,p_report->button);

Extra tab here?

> -static int alps_post_resume(struct hid_device *hdev)
> +static int __maybe_unused alps_post_reset(struct hid_device *hdev)

Do we really need the __maybe_unused annotation here? Why not just hide 
the whole  post-reset callback handling into a #ifdef CONFIG_PM block?

Thanks,

-- 
Jiri Kosina
SUSE Labs

[PATCH 5/7] Support new Alps device that name is T4

[Masaki Ota]

From: Masaki Ota 
-Add T4 device code and Product ID
-This device is used on HP EliteBook 1000 series and Zbook Stduio

Signed-off-by: Masaki Ota 
---
 drivers/hid/hid-alps.c | 343 ++---
 drivers/hid/hid-core.c |   3 +-
 drivers/hid/hid-ids.h  |   1 +
 3 files changed, 326 insertions(+), 21 deletions(-)

diff --git a/drivers/hid/hid-alps.c b/drivers/hid/hid-alps.c
index 4c323b58e009..0a6c54fb47c8 100644
--- a/drivers/hid/hid-alps.c
+++ b/drivers/hid/hid-alps.c
@@ -52,8 +52,30 @@
 #define ADDRESS_U1_PAD_BTN 0x00800052
 #define ADDRESS_U1_SP_BTN  0x0080009F
 
+#define T4_INPUT_REPORT_LENsizeof(struct t4_input_report)
+#define T4_FEATURE_REPORT_LEN  T4_INPUT_REPORT_LEN
+#define T4_FEATURE_REPORT_ID   7
+#define T4_CMD_REGISTER_READ   0x08
+#define T4_CMD_REGISTER_WRITE  0x07
+
+#define T4_ADDRESS_BASE0xC2C0
+#define PRM_SYS_CONFIG_1   (T4_ADDRESS_BASE + 0x0002)
+#define T4_PRM_FEED_CONFIG_1   (T4_ADDRESS_BASE + 0x0004)
+#define T4_PRM_FEED_CONFIG_4   (T4_ADDRESS_BASE + 0x001A)
+#define T4_PRM_ID_CONFIG_3 (T4_ADDRESS_BASE + 0x00B0)
+
+
+#define T4_FEEDCFG4_ADVANCED_ABS_ENABLE0x01
+#define T4_I2C_ABS 0x78
+
+#define T4_COUNT_PER_ELECTRODE 256
 #define MAX_TOUCHES5
 
+enum dev_num {
+   U1,
+   T4,
+   UNKNOWN,
+};
 /**
  * struct u1_data
  *
@@ -74,11 +96,12 @@
  * @btn_cnt: number of buttons
  * @sp_btn_cnt: number of stick buttons
  */
-struct u1_dev {
+struct alps_dev {
struct input_dev *input;
struct input_dev *input2;
struct hid_device *hdev;
 
+   enum dev_num dev_type;
u8  max_fingers;
u8  has_sp;
u8  sp_btn_info;
@@ -92,6 +115,145 @@ struct u1_dev {
u32 sp_btn_cnt;
 };
 
+struct t4_contact_data {
+   u8  palm;
+   u8  x_lo;
+   u8  x_hi;
+   u8  y_lo;
+   u8  y_hi;
+};
+
+struct t4_input_report {
+   u8  reportID;
+   u8  numContacts;
+   struct t4_contact_data contact[5];
+   u8  button;
+   u8  track[5];
+   u8  zx[5], zy[5];
+   u8  palmTime[5];
+   u8  kilroy;
+   u16 timeStamp;
+};
+
+static u16 t4_calc_check_sum(u8 *buffer,
+   unsigned long offset, unsigned long length)
+{
+   u16 sum1 = 0xFF, sum2 = 0xFF;
+   unsigned long i = 0;
+
+   if (offset + length >= 50)
+   return 0;
+
+   while (length > 0) {
+   u32 tlen = length > 20 ? 20 : length;
+
+   length -= tlen;
+
+   do {
+   sum1 += buffer[offset + i];
+   sum2 += sum1;
+   i++;
+   } while (--tlen > 0);
+
+   sum1 = (sum1 & 0xFF) + (sum1 >> 8);
+   sum2 = (sum2 & 0xFF) + (sum2 >> 8);
+   }
+
+   sum1 = (sum1 & 0xFF) + (sum1 >> 8);
+   sum2 = (sum2 & 0xFF) + (sum2 >> 8);
+
+   return(sum2 << 8 | sum1);
+}
+
+static int t4_read_write_register(struct hid_device *hdev, u32 address,
+   u8 *read_val, u8 write_val, bool read_flag)
+{
+   int ret;
+   u16 check_sum;
+   u8 *input;
+   u8 *readbuf;
+
+   input = kzalloc(T4_FEATURE_REPORT_LEN, GFP_KERNEL);
+   if (!input)
+   return -ENOMEM;
+
+   input[0] = T4_FEATURE_REPORT_ID;
+   if (read_flag) {
+   input[1] = T4_CMD_REGISTER_READ;
+   input[8] = 0x00;
+   } else {
+   input[1] = T4_CMD_REGISTER_WRITE;
+   input[8] = write_val;
+   }
+   put_unaligned_le32(address, input + 2);
+   input[6] = 1;
+   input[7] = 0;
+
+   /* Calculate the checksum */
+   check_sum = t4_calc_check_sum(input, 1, 8);
+   input[9] = (u8)check_sum;
+   input[10] = (u8)(check_sum >> 8);
+   input[11] = 0;
+
+   ret = hid_hw_raw_request(hdev, T4_FEATURE_REPORT_ID, input,
+   T4_FEATURE_REPORT_LEN,
+   HID_FEATURE_REPORT, HID_REQ_SET_REPORT);
+
+   if (ret < 0) {
+   dev_err(>dev, "failed to read command (%d)\n", ret);
+   goto exit;
+   }
+
+   if (read_flag) {
+   readbuf = kzalloc(T4_FEATURE_REPORT_LEN, GFP_KERNEL);
+   if (!readbuf) {
+   ret = -ENOMEM;
+   goto exit;
+   }
+
+   ret = hid_hw_raw_request(hdev, T4_FEATURE_REPORT_ID, readbuf,
+   T4_FEATURE_REPORT_LEN,
+   HID_FEATURE_REPORT, HID_REQ_GET_REPORT);
+   if (ret < 0) {
+   dev_err(>dev, "failed read register (%d)\n", ret);
+   kfree(readbuf);
+   goto exit;
+   }
+
+   

[PATCH 5/7] Support new Alps device that name is T4

[Masaki Ota]

From: Masaki Ota 
-Add T4 device code and Product ID
-This device is used on HP EliteBook 1000 series and Zbook Stduio

Signed-off-by: Masaki Ota 
---
 drivers/hid/hid-alps.c | 343 ++---
 drivers/hid/hid-core.c |   3 +-
 drivers/hid/hid-ids.h  |   1 +
 3 files changed, 326 insertions(+), 21 deletions(-)

diff --git a/drivers/hid/hid-alps.c b/drivers/hid/hid-alps.c
index 4c323b58e009..0a6c54fb47c8 100644
--- a/drivers/hid/hid-alps.c
+++ b/drivers/hid/hid-alps.c
@@ -52,8 +52,30 @@
 #define ADDRESS_U1_PAD_BTN 0x00800052
 #define ADDRESS_U1_SP_BTN  0x0080009F
 
+#define T4_INPUT_REPORT_LENsizeof(struct t4_input_report)
+#define T4_FEATURE_REPORT_LEN  T4_INPUT_REPORT_LEN
+#define T4_FEATURE_REPORT_ID   7
+#define T4_CMD_REGISTER_READ   0x08
+#define T4_CMD_REGISTER_WRITE  0x07
+
+#define T4_ADDRESS_BASE0xC2C0
+#define PRM_SYS_CONFIG_1   (T4_ADDRESS_BASE + 0x0002)
+#define T4_PRM_FEED_CONFIG_1   (T4_ADDRESS_BASE + 0x0004)
+#define T4_PRM_FEED_CONFIG_4   (T4_ADDRESS_BASE + 0x001A)
+#define T4_PRM_ID_CONFIG_3 (T4_ADDRESS_BASE + 0x00B0)
+
+
+#define T4_FEEDCFG4_ADVANCED_ABS_ENABLE0x01
+#define T4_I2C_ABS 0x78
+
+#define T4_COUNT_PER_ELECTRODE 256
 #define MAX_TOUCHES5
 
+enum dev_num {
+   U1,
+   T4,
+   UNKNOWN,
+};
 /**
  * struct u1_data
  *
@@ -74,11 +96,12 @@
  * @btn_cnt: number of buttons
  * @sp_btn_cnt: number of stick buttons
  */
-struct u1_dev {
+struct alps_dev {
struct input_dev *input;
struct input_dev *input2;
struct hid_device *hdev;
 
+   enum dev_num dev_type;
u8  max_fingers;
u8  has_sp;
u8  sp_btn_info;
@@ -92,6 +115,145 @@ struct u1_dev {
u32 sp_btn_cnt;
 };
 
+struct t4_contact_data {
+   u8  palm;
+   u8  x_lo;
+   u8  x_hi;
+   u8  y_lo;
+   u8  y_hi;
+};
+
+struct t4_input_report {
+   u8  reportID;
+   u8  numContacts;
+   struct t4_contact_data contact[5];
+   u8  button;
+   u8  track[5];
+   u8  zx[5], zy[5];
+   u8  palmTime[5];
+   u8  kilroy;
+   u16 timeStamp;
+};
+
+static u16 t4_calc_check_sum(u8 *buffer,
+   unsigned long offset, unsigned long length)
+{
+   u16 sum1 = 0xFF, sum2 = 0xFF;
+   unsigned long i = 0;
+
+   if (offset + length >= 50)
+   return 0;
+
+   while (length > 0) {
+   u32 tlen = length > 20 ? 20 : length;
+
+   length -= tlen;
+
+   do {
+   sum1 += buffer[offset + i];
+   sum2 += sum1;
+   i++;
+   } while (--tlen > 0);
+
+   sum1 = (sum1 & 0xFF) + (sum1 >> 8);
+   sum2 = (sum2 & 0xFF) + (sum2 >> 8);
+   }
+
+   sum1 = (sum1 & 0xFF) + (sum1 >> 8);
+   sum2 = (sum2 & 0xFF) + (sum2 >> 8);
+
+   return(sum2 << 8 | sum1);
+}
+
+static int t4_read_write_register(struct hid_device *hdev, u32 address,
+   u8 *read_val, u8 write_val, bool read_flag)
+{
+   int ret;
+   u16 check_sum;
+   u8 *input;
+   u8 *readbuf;
+
+   input = kzalloc(T4_FEATURE_REPORT_LEN, GFP_KERNEL);
+   if (!input)
+   return -ENOMEM;
+
+   input[0] = T4_FEATURE_REPORT_ID;
+   if (read_flag) {
+   input[1] = T4_CMD_REGISTER_READ;
+   input[8] = 0x00;
+   } else {
+   input[1] = T4_CMD_REGISTER_WRITE;
+   input[8] = write_val;
+   }
+   put_unaligned_le32(address, input + 2);
+   input[6] = 1;
+   input[7] = 0;
+
+   /* Calculate the checksum */
+   check_sum = t4_calc_check_sum(input, 1, 8);
+   input[9] = (u8)check_sum;
+   input[10] = (u8)(check_sum >> 8);
+   input[11] = 0;
+
+   ret = hid_hw_raw_request(hdev, T4_FEATURE_REPORT_ID, input,
+   T4_FEATURE_REPORT_LEN,
+   HID_FEATURE_REPORT, HID_REQ_SET_REPORT);
+
+   if (ret < 0) {
+   dev_err(>dev, "failed to read command (%d)\n", ret);
+   goto exit;
+   }
+
+   if (read_flag) {
+   readbuf = kzalloc(T4_FEATURE_REPORT_LEN, GFP_KERNEL);
+   if (!readbuf) {
+   ret = -ENOMEM;
+   goto exit;
+   }
+
+   ret = hid_hw_raw_request(hdev, T4_FEATURE_REPORT_ID, readbuf,
+   T4_FEATURE_REPORT_LEN,
+   HID_FEATURE_REPORT, HID_REQ_GET_REPORT);
+   if (ret < 0) {
+   dev_err(>dev, "failed read register (%d)\n", ret);
+   kfree(readbuf);
+   goto exit;
+   }
+
+   if (*(u32 *)[6] != address) {
+