This device is used on Lenovo V130-15IKB variants and uses
the same registers as U1.
Signed-off-by: Artem Borisov <[email protected]>
---
drivers/hid/hid-alps.c | 1 +
drivers/hid/hid-ids.h | 2 +-
2 files changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/hid/hid-alps.c b/drivers/hid/hid-alps.c
index fa704153cb00..c2a2bd528890 100644
--- a/drivers/hid/hid-alps.c
+++ b/drivers/hid/hid-alps.c
@@ -802,6 +802,7 @@ static int alps_probe(struct hid_device *hdev, const struct hid_device_id *id)
break;
case HID_DEVICE_ID_ALPS_U1_DUAL:
case HID_DEVICE_ID_ALPS_U1:
+ case HID_DEVICE_ID_ALPS_1657:
data->dev_type = U1;
break;
default:
diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index b18b13147a6f..324fddb37e27 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -81,7 +81,7 @@
#define HID_DEVICE_ID_ALPS_U1 0x1215
#define HID_DEVICE_ID_ALPS_T4_BTNLESS 0x120C
#define HID_DEVICE_ID_ALPS_1222 0x1222
-
+#define HID_DEVICE_ID_ALPS_1657 0x121E
#define USB_VENDOR_ID_AMI 0x046b
#define USB_DEVICE_ID_AMI_VIRT_KEYBOARD_AND_MOUSE 0xff10
--
2.26.0
AUI1657 doesn't follow the same logic for resolution calculation, since
the resulting values are incorrect. Instead, it reports the actual
resolution values in place of the pitch ones.
While we're at it, also refactor the whole resolution logic to make it more
generic and sensible for multiple device support.
Signed-off-by: Artem Borisov <[email protected]>
---
drivers/hid/hid-alps.c | 41 +++++++++++++++++++++++++----------------
1 file changed, 25 insertions(+), 16 deletions(-)
diff --git a/drivers/hid/hid-alps.c b/drivers/hid/hid-alps.c
index c2a2bd528890..494c08cca645 100644
--- a/drivers/hid/hid-alps.c
+++ b/drivers/hid/hid-alps.c
@@ -83,8 +83,8 @@ enum dev_num {
* @max_fingers: total number of fingers
* @has_sp: boolean of sp existense
* @sp_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_res: resolution of X
+ * @y_res: resolution of Y
* @x_max: maximum x coordinate value
* @y_max: maximum y coordinate value
* @x_min: minimum x coordinate value
@@ -100,9 +100,10 @@ struct alps_dev {
enum dev_num dev_type;
u8 max_fingers;
u8 has_sp;
+ u8 no_pitch;
u8 sp_btn_info;
- u32 x_active_len_mm;
- u32 y_active_len_mm;
+ u32 x_res;
+ u32 y_res;
u32 x_max;
u32 y_max;
u32 x_min;
@@ -550,10 +551,6 @@ static int u1_init(struct hid_device *hdev, struct alps_dev *pri_data)
dev_err(&hdev->dev, "failed U1_RESO_DWN_ABS (%d)\n", ret);
goto exit;
}
- pri_data->x_active_len_mm =
- (pitch_x * (sen_line_num_x - 1)) / 10;
- pri_data->y_active_len_mm =
- (pitch_y * (sen_line_num_y - 1)) / 10;
pri_data->x_max =
(resolution << 2) * (sen_line_num_x - 1);
@@ -562,6 +559,18 @@ static int u1_init(struct hid_device *hdev, struct alps_dev *pri_data)
(resolution << 2) * (sen_line_num_y - 1);
pri_data->y_min = 1;
+ if (pri_data->no_pitch) {
+ pri_data->x_res = pitch_x;
+ pri_data->y_res = pitch_y;
+ } else {
+ pri_data->x_res =
+ (pri_data->x_max - 1) /
+ ((pitch_x * (sen_line_num_x - 1)) / 10);
+ pri_data->y_res =
+ (pri_data->y_max - 1) /
+ ((pitch_y * (sen_line_num_y - 1)) / 10);
+ }
+
ret = u1_read_write_register(hdev, ADDRESS_U1_PAD_BTN,
&tmp, 0, true);
if (ret < 0) {
@@ -622,7 +631,7 @@ static int T4_init(struct hid_device *hdev, struct alps_dev *pri_data)
pri_data->x_min = T4_COUNT_PER_ELECTRODE;
pri_data->y_max = sen_line_num_y * T4_COUNT_PER_ELECTRODE;
pri_data->y_min = T4_COUNT_PER_ELECTRODE;
- pri_data->x_active_len_mm = pri_data->y_active_len_mm = 0;
+ pri_data->x_res = pri_data->y_res = 0;
pri_data->btn_cnt = 1;
ret = t4_read_write_register(hdev, PRM_SYS_CONFIG_1, &tmp, 0, true);
@@ -675,7 +684,7 @@ static int alps_input_configured(struct hid_device *hdev, struct hid_input *hi)
struct alps_dev *data = hid_get_drvdata(hdev);
struct input_dev *input = hi->input, *input2;
int ret;
- int res_x, res_y, i;
+ int i;
data->input = input;
@@ -706,12 +715,9 @@ static int alps_input_configured(struct hid_device *hdev, struct hid_input *hi)
input_set_abs_params(input, ABS_MT_POSITION_Y,
data->y_min, data->y_max, 0, 0);
- if (data->x_active_len_mm && data->y_active_len_mm) {
- res_x = (data->x_max - 1) / data->x_active_len_mm;
- res_y = (data->y_max - 1) / data->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);
+ if (data->x_res && data->y_res) {
+ input_abs_set_res(input, ABS_MT_POSITION_X, data->x_res);
+ input_abs_set_res(input, ABS_MT_POSITION_Y, data->y_res);
}
input_set_abs_params(input, ABS_MT_PRESSURE, 0, 64, 0, 0);
@@ -802,8 +808,11 @@ static int alps_probe(struct hid_device *hdev, const struct hid_device_id *id)
break;
case HID_DEVICE_ID_ALPS_U1_DUAL:
case HID_DEVICE_ID_ALPS_U1:
+ data->dev_type = U1;
+ break;
case HID_DEVICE_ID_ALPS_1657:
data->dev_type = U1;
+ data->no_pitch = 1;
break;
default:
data->dev_type = UNKNOWN;
--
2.26.0
On Mon, 6 Apr 2020, Artem Borisov wrote:
> AUI1657 doesn't follow the same logic for resolution calculation, since
> the resulting values are incorrect. Instead, it reports the actual
> resolution values in place of the pitch ones.
> While we're at it, also refactor the whole resolution logic to make it more
> generic and sensible for multiple device support.
Let's add Masaki Ota to ack this change if possible.
Would it be possible to be a little bit more verbose in the changelog,
explaining *how* (or why) is the patch making the logic more generic and
sensible?
Thanks!
> Signed-off-by: Artem Borisov <[email protected]>
> ---
> drivers/hid/hid-alps.c | 41 +++++++++++++++++++++++++----------------
> 1 file changed, 25 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/hid/hid-alps.c b/drivers/hid/hid-alps.c
> index c2a2bd528890..494c08cca645 100644
> --- a/drivers/hid/hid-alps.c
> +++ b/drivers/hid/hid-alps.c
> @@ -83,8 +83,8 @@ enum dev_num {
> * @max_fingers: total number of fingers
> * @has_sp: boolean of sp existense
> * @sp_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_res: resolution of X
> + * @y_res: resolution of Y
> * @x_max: maximum x coordinate value
> * @y_max: maximum y coordinate value
> * @x_min: minimum x coordinate value
> @@ -100,9 +100,10 @@ struct alps_dev {
> enum dev_num dev_type;
> u8 max_fingers;
> u8 has_sp;
> + u8 no_pitch;
> u8 sp_btn_info;
> - u32 x_active_len_mm;
> - u32 y_active_len_mm;
> + u32 x_res;
> + u32 y_res;
> u32 x_max;
> u32 y_max;
> u32 x_min;
> @@ -550,10 +551,6 @@ static int u1_init(struct hid_device *hdev, struct alps_dev *pri_data)
> dev_err(&hdev->dev, "failed U1_RESO_DWN_ABS (%d)\n", ret);
> goto exit;
> }
> - pri_data->x_active_len_mm =
> - (pitch_x * (sen_line_num_x - 1)) / 10;
> - pri_data->y_active_len_mm =
> - (pitch_y * (sen_line_num_y - 1)) / 10;
>
> pri_data->x_max =
> (resolution << 2) * (sen_line_num_x - 1);
> @@ -562,6 +559,18 @@ static int u1_init(struct hid_device *hdev, struct alps_dev *pri_data)
> (resolution << 2) * (sen_line_num_y - 1);
> pri_data->y_min = 1;
>
> + if (pri_data->no_pitch) {
> + pri_data->x_res = pitch_x;
> + pri_data->y_res = pitch_y;
> + } else {
> + pri_data->x_res =
> + (pri_data->x_max - 1) /
> + ((pitch_x * (sen_line_num_x - 1)) / 10);
> + pri_data->y_res =
> + (pri_data->y_max - 1) /
> + ((pitch_y * (sen_line_num_y - 1)) / 10);
> + }
> +
> ret = u1_read_write_register(hdev, ADDRESS_U1_PAD_BTN,
> &tmp, 0, true);
> if (ret < 0) {
> @@ -622,7 +631,7 @@ static int T4_init(struct hid_device *hdev, struct alps_dev *pri_data)
> pri_data->x_min = T4_COUNT_PER_ELECTRODE;
> pri_data->y_max = sen_line_num_y * T4_COUNT_PER_ELECTRODE;
> pri_data->y_min = T4_COUNT_PER_ELECTRODE;
> - pri_data->x_active_len_mm = pri_data->y_active_len_mm = 0;
> + pri_data->x_res = pri_data->y_res = 0;
> pri_data->btn_cnt = 1;
>
> ret = t4_read_write_register(hdev, PRM_SYS_CONFIG_1, &tmp, 0, true);
> @@ -675,7 +684,7 @@ static int alps_input_configured(struct hid_device *hdev, struct hid_input *hi)
> struct alps_dev *data = hid_get_drvdata(hdev);
> struct input_dev *input = hi->input, *input2;
> int ret;
> - int res_x, res_y, i;
> + int i;
>
> data->input = input;
>
> @@ -706,12 +715,9 @@ static int alps_input_configured(struct hid_device *hdev, struct hid_input *hi)
> input_set_abs_params(input, ABS_MT_POSITION_Y,
> data->y_min, data->y_max, 0, 0);
>
> - if (data->x_active_len_mm && data->y_active_len_mm) {
> - res_x = (data->x_max - 1) / data->x_active_len_mm;
> - res_y = (data->y_max - 1) / data->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);
> + if (data->x_res && data->y_res) {
> + input_abs_set_res(input, ABS_MT_POSITION_X, data->x_res);
> + input_abs_set_res(input, ABS_MT_POSITION_Y, data->y_res);
> }
>
> input_set_abs_params(input, ABS_MT_PRESSURE, 0, 64, 0, 0);
> @@ -802,8 +808,11 @@ static int alps_probe(struct hid_device *hdev, const struct hid_device_id *id)
> break;
> case HID_DEVICE_ID_ALPS_U1_DUAL:
> case HID_DEVICE_ID_ALPS_U1:
> + data->dev_type = U1;
> + break;
> case HID_DEVICE_ID_ALPS_1657:
> data->dev_type = U1;
> + data->no_pitch = 1;
> break;
> default:
> data->dev_type = UNKNOWN;
> --
> 2.26.0
>
--
Jiri Kosina
SUSE Labs
AUI1657 doesn't follow the same logic for resolution calculation, since
the resulting values are incorrect. Instead, it reports the actual
resolution values in place of the pitch ones.
While we're at it, also refactor the whole resolution logic to make it more
generic and sensible for multiple device support.
There are two main logical problems with the current code:
1. active_len_mm values are only used for resolution calculation on U1,
yet are exposed globally as part of alps_dev structure.
2. The resolution calculation process happens in alps_input_configured,
while everything else is calculated in u1_init function.
These problems become more apparent when we try to support a device
that doesn't follow the same resolution calculation logic as U1.
Since alps_input_configured is a device-agnostic function, we should
avoid doing any measurements there and handle them in device-specific
init functions like u1/T4_init instead.
To eliminate these problems we add global x_res and y_res values
and populate them on a device-specific basis in the according init
functions.
Signed-off-by: Artem Borisov <[email protected]>
---
drivers/hid/hid-alps.c | 41 +++++++++++++++++++++++++----------------
1 file changed, 25 insertions(+), 16 deletions(-)
diff --git a/drivers/hid/hid-alps.c b/drivers/hid/hid-alps.c
index c2a2bd528890..494c08cca645 100644
--- a/drivers/hid/hid-alps.c
+++ b/drivers/hid/hid-alps.c
@@ -83,8 +83,8 @@ enum dev_num {
* @max_fingers: total number of fingers
* @has_sp: boolean of sp existense
* @sp_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_res: resolution of X
+ * @y_res: resolution of Y
* @x_max: maximum x coordinate value
* @y_max: maximum y coordinate value
* @x_min: minimum x coordinate value
@@ -100,9 +100,10 @@ struct alps_dev {
enum dev_num dev_type;
u8 max_fingers;
u8 has_sp;
+ u8 no_pitch;
u8 sp_btn_info;
- u32 x_active_len_mm;
- u32 y_active_len_mm;
+ u32 x_res;
+ u32 y_res;
u32 x_max;
u32 y_max;
u32 x_min;
@@ -550,10 +551,6 @@ static int u1_init(struct hid_device *hdev, struct alps_dev *pri_data)
dev_err(&hdev->dev, "failed U1_RESO_DWN_ABS (%d)\n", ret);
goto exit;
}
- pri_data->x_active_len_mm =
- (pitch_x * (sen_line_num_x - 1)) / 10;
- pri_data->y_active_len_mm =
- (pitch_y * (sen_line_num_y - 1)) / 10;
pri_data->x_max =
(resolution << 2) * (sen_line_num_x - 1);
@@ -562,6 +559,18 @@ static int u1_init(struct hid_device *hdev, struct alps_dev *pri_data)
(resolution << 2) * (sen_line_num_y - 1);
pri_data->y_min = 1;
+ if (pri_data->no_pitch) {
+ pri_data->x_res = pitch_x;
+ pri_data->y_res = pitch_y;
+ } else {
+ pri_data->x_res =
+ (pri_data->x_max - 1) /
+ ((pitch_x * (sen_line_num_x - 1)) / 10);
+ pri_data->y_res =
+ (pri_data->y_max - 1) /
+ ((pitch_y * (sen_line_num_y - 1)) / 10);
+ }
+
ret = u1_read_write_register(hdev, ADDRESS_U1_PAD_BTN,
&tmp, 0, true);
if (ret < 0) {
@@ -622,7 +631,7 @@ static int T4_init(struct hid_device *hdev, struct alps_dev *pri_data)
pri_data->x_min = T4_COUNT_PER_ELECTRODE;
pri_data->y_max = sen_line_num_y * T4_COUNT_PER_ELECTRODE;
pri_data->y_min = T4_COUNT_PER_ELECTRODE;
- pri_data->x_active_len_mm = pri_data->y_active_len_mm = 0;
+ pri_data->x_res = pri_data->y_res = 0;
pri_data->btn_cnt = 1;
ret = t4_read_write_register(hdev, PRM_SYS_CONFIG_1, &tmp, 0, true);
@@ -675,7 +684,7 @@ static int alps_input_configured(struct hid_device *hdev, struct hid_input *hi)
struct alps_dev *data = hid_get_drvdata(hdev);
struct input_dev *input = hi->input, *input2;
int ret;
- int res_x, res_y, i;
+ int i;
data->input = input;
@@ -706,12 +715,9 @@ static int alps_input_configured(struct hid_device *hdev, struct hid_input *hi)
input_set_abs_params(input, ABS_MT_POSITION_Y,
data->y_min, data->y_max, 0, 0);
- if (data->x_active_len_mm && data->y_active_len_mm) {
- res_x = (data->x_max - 1) / data->x_active_len_mm;
- res_y = (data->y_max - 1) / data->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);
+ if (data->x_res && data->y_res) {
+ input_abs_set_res(input, ABS_MT_POSITION_X, data->x_res);
+ input_abs_set_res(input, ABS_MT_POSITION_Y, data->y_res);
}
input_set_abs_params(input, ABS_MT_PRESSURE, 0, 64, 0, 0);
@@ -802,8 +808,11 @@ static int alps_probe(struct hid_device *hdev, const struct hid_device_id *id)
break;
case HID_DEVICE_ID_ALPS_U1_DUAL:
case HID_DEVICE_ID_ALPS_U1:
+ data->dev_type = U1;
+ break;
case HID_DEVICE_ID_ALPS_1657:
data->dev_type = U1;
+ data->no_pitch = 1;
break;
default:
data->dev_type = UNKNOWN;
--
2.26.0
Hi, Cao-san,
Do you know AUI1657 device? This device looks U1.
I think recent all U1 devices work as PTP.
Linux also supports PTP, so I think we should add something ID to Linux source code. (I remember a something flag is already exist.)
Best Regards,
Masaki Ota
-----Original Message-----
From: Artem Borisov <[email protected]>
Sent: Friday, April 10, 2020 8:00 AM
Cc: [email protected]; $BB@ED(B $B??4n(B Masaki Ota <[email protected]>; Artem Borisov <[email protected]>; Benjamin Tissoires <[email protected]>; Henrik Rydberg <[email protected]>; [email protected]; [email protected]
Subject: [PATCH 2/2] HID: alps: Refactor axis resolution logic
AUI1657 doesn't follow the same logic for resolution calculation, since the resulting values are incorrect. Instead, it reports the actual resolution values in place of the pitch ones.
While we're at it, also refactor the whole resolution logic to make it more generic and sensible for multiple device support.
There are two main logical problems with the current code:
1. active_len_mm values are only used for resolution calculation on U1, yet are exposed globally as part of alps_dev structure.
2. The resolution calculation process happens in alps_input_configured, while everything else is calculated in u1_init function.
These problems become more apparent when we try to support a device that doesn't follow the same resolution calculation logic as U1.
Since alps_input_configured is a device-agnostic function, we should avoid doing any measurements there and handle them in device-specific init functions like u1/T4_init instead.
To eliminate these problems we add global x_res and y_res values and populate them on a device-specific basis in the according init functions.
Signed-off-by: Artem Borisov <[email protected]>
---
drivers/hid/hid-alps.c | 41 +++++++++++++++++++++++++----------------
1 file changed, 25 insertions(+), 16 deletions(-)
diff --git a/drivers/hid/hid-alps.c b/drivers/hid/hid-alps.c index c2a2bd528890..494c08cca645 100644
--- a/drivers/hid/hid-alps.c
+++ b/drivers/hid/hid-alps.c
@@ -83,8 +83,8 @@ enum dev_num {
* @max_fingers: total number of fingers
* @has_sp: boolean of sp existense
* @sp_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_res: resolution of X
+ * @y_res: resolution of Y
* @x_max: maximum x coordinate value
* @y_max: maximum y coordinate value
* @x_min: minimum x coordinate value
@@ -100,9 +100,10 @@ struct alps_dev {
enum dev_num dev_type;
u8 max_fingers;
u8 has_sp;
+ u8 no_pitch;
u8 sp_btn_info;
- u32 x_active_len_mm;
- u32 y_active_len_mm;
+ u32 x_res;
+ u32 y_res;
u32 x_max;
u32 y_max;
u32 x_min;
@@ -550,10 +551,6 @@ static int u1_init(struct hid_device *hdev, struct alps_dev *pri_data)
dev_err(&hdev->dev, "failed U1_RESO_DWN_ABS (%d)\n", ret);
goto exit;
}
- pri_data->x_active_len_mm =
- (pitch_x * (sen_line_num_x - 1)) / 10;
- pri_data->y_active_len_mm =
- (pitch_y * (sen_line_num_y - 1)) / 10;
pri_data->x_max =
(resolution << 2) * (sen_line_num_x - 1); @@ -562,6 +559,18 @@ static int u1_init(struct hid_device *hdev, struct alps_dev *pri_data)
(resolution << 2) * (sen_line_num_y - 1);
pri_data->y_min = 1;
+ if (pri_data->no_pitch) {
+ pri_data->x_res = pitch_x;
+ pri_data->y_res = pitch_y;
+ } else {
+ pri_data->x_res =
+ (pri_data->x_max - 1) /
+ ((pitch_x * (sen_line_num_x - 1)) / 10);
+ pri_data->y_res =
+ (pri_data->y_max - 1) /
+ ((pitch_y * (sen_line_num_y - 1)) / 10);
+ }
+
ret = u1_read_write_register(hdev, ADDRESS_U1_PAD_BTN,
&tmp, 0, true);
if (ret < 0) {
@@ -622,7 +631,7 @@ static int T4_init(struct hid_device *hdev, struct alps_dev *pri_data)
pri_data->x_min = T4_COUNT_PER_ELECTRODE;
pri_data->y_max = sen_line_num_y * T4_COUNT_PER_ELECTRODE;
pri_data->y_min = T4_COUNT_PER_ELECTRODE;
- pri_data->x_active_len_mm = pri_data->y_active_len_mm = 0;
+ pri_data->x_res = pri_data->y_res = 0;
pri_data->btn_cnt = 1;
ret = t4_read_write_register(hdev, PRM_SYS_CONFIG_1, &tmp, 0, true); @@ -675,7 +684,7 @@ static int alps_input_configured(struct hid_device *hdev, struct hid_input *hi)
struct alps_dev *data = hid_get_drvdata(hdev);
struct input_dev *input = hi->input, *input2;
int ret;
- int res_x, res_y, i;
+ int i;
data->input = input;
@@ -706,12 +715,9 @@ static int alps_input_configured(struct hid_device *hdev, struct hid_input *hi)
input_set_abs_params(input, ABS_MT_POSITION_Y,
data->y_min, data->y_max, 0, 0);
- if (data->x_active_len_mm && data->y_active_len_mm) {
- res_x = (data->x_max - 1) / data->x_active_len_mm;
- res_y = (data->y_max - 1) / data->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);
+ if (data->x_res && data->y_res) {
+ input_abs_set_res(input, ABS_MT_POSITION_X, data->x_res);
+ input_abs_set_res(input, ABS_MT_POSITION_Y, data->y_res);
}
input_set_abs_params(input, ABS_MT_PRESSURE, 0, 64, 0, 0); @@ -802,8 +808,11 @@ static int alps_probe(struct hid_device *hdev, const struct hid_device_id *id)
break;
case HID_DEVICE_ID_ALPS_U1_DUAL:
case HID_DEVICE_ID_ALPS_U1:
+ data->dev_type = U1;
+ break;
case HID_DEVICE_ID_ALPS_1657:
data->dev_type = U1;
+ data->no_pitch = 1;
break;
default:
data->dev_type = UNKNOWN;
--
2.26.0
On Fri, 10 Apr 2020, Masaki Ota wrote:
> Hi, Cao-san,
>
> Do you know AUI1657 device? This device looks U1. I think recent all U1
> devices work as PTP. Linux also supports PTP, so I think we should add
> something ID to Linux source code. (I remember a something flag is
> already exist.)
That's actually partially covered by Artem's 1/1 patch you have not been
CCed on -- please see it here:
http://lore.kernel.org/r/[email protected]
If this could be made more generic to cover a group of devices all-in-one,
that'd be even better of course.
Thanks,
--
Jiri Kosina
SUSE Labs
Hi Ota-san,
Thanks for your checking.
In fact, some of the U1 devices work as non-PTP.
AUI1657 is using U1(KGDBCHA004A) whose firmware just supports mouse mode and legacy mode.
Best Regards,
----------------------------------------------
Jason Cao(曹晓建)
-----Original Message-----
From: 太田 真喜 Masaki Ota <[email protected]>
Sent: Friday, April 10, 2020 8:29 AM
To: 曹 曉建 Xiaojian Cao <[email protected]>; Artem Borisov <[email protected]>
Cc: [email protected]; Benjamin Tissoires <[email protected]>; Henrik Rydberg <[email protected]>; [email protected]; [email protected]
Subject: RE: [PATCH 2/2] HID: alps: Refactor axis resolution logic
Hi, Cao-san,
Do you know AUI1657 device? This device looks U1.
I think recent all U1 devices work as PTP.
Linux also supports PTP, so I think we should add something ID to Linux source code. (I remember a something flag is already exist.)
Best Regards,
Masaki Ota
-----Original Message-----
From: Artem Borisov <[email protected]>
Sent: Friday, April 10, 2020 8:00 AM
Cc: [email protected]; 太田 真喜 Masaki Ota <[email protected]>; Artem Borisov <[email protected]>; Benjamin Tissoires <[email protected]>; Henrik Rydberg <[email protected]>; [email protected]; [email protected]
Subject: [PATCH 2/2] HID: alps: Refactor axis resolution logic
AUI1657 doesn't follow the same logic for resolution calculation, since the resulting values are incorrect. Instead, it reports the actual resolution values in place of the pitch ones.
While we're at it, also refactor the whole resolution logic to make it more generic and sensible for multiple device support.
There are two main logical problems with the current code:
1. active_len_mm values are only used for resolution calculation on U1, yet are exposed globally as part of alps_dev structure.
2. The resolution calculation process happens in alps_input_configured, while everything else is calculated in u1_init function.
These problems become more apparent when we try to support a device that doesn't follow the same resolution calculation logic as U1.
Since alps_input_configured is a device-agnostic function, we should avoid doing any measurements there and handle them in device-specific init functions like u1/T4_init instead.
To eliminate these problems we add global x_res and y_res values and populate them on a device-specific basis in the according init functions.
Signed-off-by: Artem Borisov <[email protected]>
---
drivers/hid/hid-alps.c | 41 +++++++++++++++++++++++++----------------
1 file changed, 25 insertions(+), 16 deletions(-)
diff --git a/drivers/hid/hid-alps.c b/drivers/hid/hid-alps.c index c2a2bd528890..494c08cca645 100644
--- a/drivers/hid/hid-alps.c
+++ b/drivers/hid/hid-alps.c
@@ -83,8 +83,8 @@ enum dev_num {
* @max_fingers: total number of fingers
* @has_sp: boolean of sp existense
* @sp_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_res: resolution of X
+ * @y_res: resolution of Y
* @x_max: maximum x coordinate value
* @y_max: maximum y coordinate value
* @x_min: minimum x coordinate value
@@ -100,9 +100,10 @@ struct alps_dev {
enum dev_num dev_type;
u8 max_fingers;
u8 has_sp;
+ u8 no_pitch;
u8 sp_btn_info;
- u32 x_active_len_mm;
- u32 y_active_len_mm;
+ u32 x_res;
+ u32 y_res;
u32 x_max;
u32 y_max;
u32 x_min;
@@ -550,10 +551,6 @@ static int u1_init(struct hid_device *hdev, struct alps_dev *pri_data)
dev_err(&hdev->dev, "failed U1_RESO_DWN_ABS (%d)\n", ret);
goto exit;
}
- pri_data->x_active_len_mm =
- (pitch_x * (sen_line_num_x - 1)) / 10;
- pri_data->y_active_len_mm =
- (pitch_y * (sen_line_num_y - 1)) / 10;
pri_data->x_max =
(resolution << 2) * (sen_line_num_x - 1); @@ -562,6 +559,18 @@ static int u1_init(struct hid_device *hdev, struct alps_dev *pri_data)
(resolution << 2) * (sen_line_num_y - 1);
pri_data->y_min = 1;
+ if (pri_data->no_pitch) {
+ pri_data->x_res = pitch_x;
+ pri_data->y_res = pitch_y;
+ } else {
+ pri_data->x_res =
+ (pri_data->x_max - 1) /
+ ((pitch_x * (sen_line_num_x - 1)) / 10);
+ pri_data->y_res =
+ (pri_data->y_max - 1) /
+ ((pitch_y * (sen_line_num_y - 1)) / 10);
+ }
+
ret = u1_read_write_register(hdev, ADDRESS_U1_PAD_BTN,
&tmp, 0, true);
if (ret < 0) {
@@ -622,7 +631,7 @@ static int T4_init(struct hid_device *hdev, struct alps_dev *pri_data)
pri_data->x_min = T4_COUNT_PER_ELECTRODE;
pri_data->y_max = sen_line_num_y * T4_COUNT_PER_ELECTRODE;
pri_data->y_min = T4_COUNT_PER_ELECTRODE;
- pri_data->x_active_len_mm = pri_data->y_active_len_mm = 0;
+ pri_data->x_res = pri_data->y_res = 0;
pri_data->btn_cnt = 1;
ret = t4_read_write_register(hdev, PRM_SYS_CONFIG_1, &tmp, 0, true); @@ -675,7 +684,7 @@ static int alps_input_configured(struct hid_device *hdev, struct hid_input *hi)
struct alps_dev *data = hid_get_drvdata(hdev);
struct input_dev *input = hi->input, *input2;
int ret;
- int res_x, res_y, i;
+ int i;
data->input = input;
@@ -706,12 +715,9 @@ static int alps_input_configured(struct hid_device *hdev, struct hid_input *hi)
input_set_abs_params(input, ABS_MT_POSITION_Y,
data->y_min, data->y_max, 0, 0);
- if (data->x_active_len_mm && data->y_active_len_mm) {
- res_x = (data->x_max - 1) / data->x_active_len_mm;
- res_y = (data->y_max - 1) / data->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);
+ if (data->x_res && data->y_res) {
+ input_abs_set_res(input, ABS_MT_POSITION_X, data->x_res);
+ input_abs_set_res(input, ABS_MT_POSITION_Y, data->y_res);
}
input_set_abs_params(input, ABS_MT_PRESSURE, 0, 64, 0, 0); @@ -802,8 +808,11 @@ static int alps_probe(struct hid_device *hdev, const struct hid_device_id *id)
break;
case HID_DEVICE_ID_ALPS_U1_DUAL:
case HID_DEVICE_ID_ALPS_U1:
+ data->dev_type = U1;
+ break;
case HID_DEVICE_ID_ALPS_1657:
data->dev_type = U1;
+ data->no_pitch = 1;
break;
default:
data->dev_type = UNKNOWN;
--
2.26.0
Hi, Cao-san,
I got it. I also confirmed this touchpad is a special.
What do you think this code modification?
Best Regards,
Masaki Ota
-----Original Message-----
From: 曹 曉建 Xiaojian Cao <[email protected]>
Sent: Friday, April 10, 2020 10:03 AM
To: 太田 真喜 Masaki Ota <[email protected]>; Artem Borisov <[email protected]>
Cc: [email protected]; Benjamin Tissoires <[email protected]>; Henrik Rydberg <[email protected]>; [email protected]; [email protected]
Subject: RE: [PATCH 2/2] HID: alps: Refactor axis resolution logic
Hi Ota-san,
Thanks for your checking.
In fact, some of the U1 devices work as non-PTP.
AUI1657 is using U1(KGDBCHA004A) whose firmware just supports mouse mode and legacy mode.
Best Regards,
----------------------------------------------
Jason Cao(曹晓建)
-----Original Message-----
From: 太田 真喜 Masaki Ota <[email protected]>
Sent: Friday, April 10, 2020 8:29 AM
To: 曹 曉建 Xiaojian Cao <[email protected]>; Artem Borisov <[email protected]>
Cc: [email protected]; Benjamin Tissoires <[email protected]>; Henrik Rydberg <[email protected]>; [email protected]; [email protected]
Subject: RE: [PATCH 2/2] HID: alps: Refactor axis resolution logic
Hi, Cao-san,
Do you know AUI1657 device? This device looks U1.
I think recent all U1 devices work as PTP.
Linux also supports PTP, so I think we should add something ID to Linux source code. (I remember a something flag is already exist.)
Best Regards,
Masaki Ota
-----Original Message-----
From: Artem Borisov <[email protected]>
Sent: Friday, April 10, 2020 8:00 AM
Cc: [email protected]; 太田 真喜 Masaki Ota <[email protected]>; Artem Borisov <[email protected]>; Benjamin Tissoires <[email protected]>; Henrik Rydberg <[email protected]>; [email protected]; [email protected]
Subject: [PATCH 2/2] HID: alps: Refactor axis resolution logic
AUI1657 doesn't follow the same logic for resolution calculation, since the resulting values are incorrect. Instead, it reports the actual resolution values in place of the pitch ones.
While we're at it, also refactor the whole resolution logic to make it more generic and sensible for multiple device support.
There are two main logical problems with the current code:
1. active_len_mm values are only used for resolution calculation on U1, yet are exposed globally as part of alps_dev structure.
2. The resolution calculation process happens in alps_input_configured, while everything else is calculated in u1_init function.
These problems become more apparent when we try to support a device that doesn't follow the same resolution calculation logic as U1.
Since alps_input_configured is a device-agnostic function, we should avoid doing any measurements there and handle them in device-specific init functions like u1/T4_init instead.
To eliminate these problems we add global x_res and y_res values and populate them on a device-specific basis in the according init functions.
Signed-off-by: Artem Borisov <[email protected]>
---
drivers/hid/hid-alps.c | 41 +++++++++++++++++++++++++----------------
1 file changed, 25 insertions(+), 16 deletions(-)
diff --git a/drivers/hid/hid-alps.c b/drivers/hid/hid-alps.c index c2a2bd528890..494c08cca645 100644
--- a/drivers/hid/hid-alps.c
+++ b/drivers/hid/hid-alps.c
@@ -83,8 +83,8 @@ enum dev_num {
* @max_fingers: total number of fingers
* @has_sp: boolean of sp existense
* @sp_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_res: resolution of X
+ * @y_res: resolution of Y
* @x_max: maximum x coordinate value
* @y_max: maximum y coordinate value
* @x_min: minimum x coordinate value
@@ -100,9 +100,10 @@ struct alps_dev {
enum dev_num dev_type;
u8 max_fingers;
u8 has_sp;
+ u8 no_pitch;
u8 sp_btn_info;
- u32 x_active_len_mm;
- u32 y_active_len_mm;
+ u32 x_res;
+ u32 y_res;
u32 x_max;
u32 y_max;
u32 x_min;
@@ -550,10 +551,6 @@ static int u1_init(struct hid_device *hdev, struct alps_dev *pri_data)
dev_err(&hdev->dev, "failed U1_RESO_DWN_ABS (%d)\n", ret);
goto exit;
}
- pri_data->x_active_len_mm =
- (pitch_x * (sen_line_num_x - 1)) / 10;
- pri_data->y_active_len_mm =
- (pitch_y * (sen_line_num_y - 1)) / 10;
pri_data->x_max =
(resolution << 2) * (sen_line_num_x - 1); @@ -562,6 +559,18 @@ static int u1_init(struct hid_device *hdev, struct alps_dev *pri_data)
(resolution << 2) * (sen_line_num_y - 1);
pri_data->y_min = 1;
+ if (pri_data->no_pitch) {
+ pri_data->x_res = pitch_x;
+ pri_data->y_res = pitch_y;
+ } else {
+ pri_data->x_res =
+ (pri_data->x_max - 1) /
+ ((pitch_x * (sen_line_num_x - 1)) / 10);
+ pri_data->y_res =
+ (pri_data->y_max - 1) /
+ ((pitch_y * (sen_line_num_y - 1)) / 10);
+ }
+
ret = u1_read_write_register(hdev, ADDRESS_U1_PAD_BTN,
&tmp, 0, true);
if (ret < 0) {
@@ -622,7 +631,7 @@ static int T4_init(struct hid_device *hdev, struct alps_dev *pri_data)
pri_data->x_min = T4_COUNT_PER_ELECTRODE;
pri_data->y_max = sen_line_num_y * T4_COUNT_PER_ELECTRODE;
pri_data->y_min = T4_COUNT_PER_ELECTRODE;
- pri_data->x_active_len_mm = pri_data->y_active_len_mm = 0;
+ pri_data->x_res = pri_data->y_res = 0;
pri_data->btn_cnt = 1;
ret = t4_read_write_register(hdev, PRM_SYS_CONFIG_1, &tmp, 0, true); @@ -675,7 +684,7 @@ static int alps_input_configured(struct hid_device *hdev, struct hid_input *hi)
struct alps_dev *data = hid_get_drvdata(hdev);
struct input_dev *input = hi->input, *input2;
int ret;
- int res_x, res_y, i;
+ int i;
data->input = input;
@@ -706,12 +715,9 @@ static int alps_input_configured(struct hid_device *hdev, struct hid_input *hi)
input_set_abs_params(input, ABS_MT_POSITION_Y,
data->y_min, data->y_max, 0, 0);
- if (data->x_active_len_mm && data->y_active_len_mm) {
- res_x = (data->x_max - 1) / data->x_active_len_mm;
- res_y = (data->y_max - 1) / data->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);
+ if (data->x_res && data->y_res) {
+ input_abs_set_res(input, ABS_MT_POSITION_X, data->x_res);
+ input_abs_set_res(input, ABS_MT_POSITION_Y, data->y_res);
}
input_set_abs_params(input, ABS_MT_PRESSURE, 0, 64, 0, 0); @@ -802,8 +808,11 @@ static int alps_probe(struct hid_device *hdev, const struct hid_device_id *id)
break;
case HID_DEVICE_ID_ALPS_U1_DUAL:
case HID_DEVICE_ID_ALPS_U1:
+ data->dev_type = U1;
+ break;
case HID_DEVICE_ID_ALPS_1657:
data->dev_type = U1;
+ data->no_pitch = 1;
break;
default:
data->dev_type = UNKNOWN;
--
2.26.0
Hi Jiri-san,
Thanks for your information. I will look into it.
Best Regards,
----------------------------------------------
Jason Cao(曹晓建)
-----Original Message-----
From: Jiri Kosina <[email protected]>
Sent: Friday, April 10, 2020 8:47 AM
To: 太田 真喜 Masaki Ota <[email protected]>
Cc: 曹 曉建 Xiaojian Cao <[email protected]>; Artem Borisov <[email protected]>; Benjamin Tissoires <[email protected]>; Henrik Rydberg <[email protected]>; [email protected]; [email protected]
Subject: RE: [PATCH 2/2] HID: alps: Refactor axis resolution logic
On Fri, 10 Apr 2020, Masaki Ota wrote:
> Hi, Cao-san,
>
> Do you know AUI1657 device? This device looks U1. I think recent all
> U1 devices work as PTP. Linux also supports PTP, so I think we should
> add something ID to Linux source code. (I remember a something flag is
> already exist.)
That's actually partially covered by Artem's 1/1 patch you have not been CCed on -- please see it here:
http://lore.kernel.org/r/[email protected]
If this could be made more generic to cover a group of devices all-in-one, that'd be even better of course.
Thanks,
--
Jiri Kosina
SUSE Labs
Hi Ota-san,
I need to study the background first, then I will update my understanding about it.
Best Regards,
----------------------------------------------
Jason Cao(曹晓建)
-----Original Message-----
From: 太田 真喜 Masaki Ota <[email protected]>
Sent: Friday, April 10, 2020 9:51 AM
To: 曹 曉建 Xiaojian Cao <[email protected]>; Artem Borisov <[email protected]>
Cc: [email protected]; Benjamin Tissoires <[email protected]>; Henrik Rydberg <[email protected]>; [email protected]; [email protected]
Subject: RE: [PATCH 2/2] HID: alps: Refactor axis resolution logic
Hi, Cao-san,
I got it. I also confirmed this touchpad is a special.
What do you think this code modification?
Best Regards,
Masaki Ota
-----Original Message-----
From: 曹 曉建 Xiaojian Cao <[email protected]>
Sent: Friday, April 10, 2020 10:03 AM
To: 太田 真喜 Masaki Ota <[email protected]>; Artem Borisov <[email protected]>
Cc: [email protected]; Benjamin Tissoires <[email protected]>; Henrik Rydberg <[email protected]>; [email protected]; [email protected]
Subject: RE: [PATCH 2/2] HID: alps: Refactor axis resolution logic
Hi Ota-san,
Thanks for your checking.
In fact, some of the U1 devices work as non-PTP.
AUI1657 is using U1(KGDBCHA004A) whose firmware just supports mouse mode and legacy mode.
Best Regards,
----------------------------------------------
Jason Cao(曹晓建)
-----Original Message-----
From: 太田 真喜 Masaki Ota <[email protected]>
Sent: Friday, April 10, 2020 8:29 AM
To: 曹 曉建 Xiaojian Cao <[email protected]>; Artem Borisov <[email protected]>
Cc: [email protected]; Benjamin Tissoires <[email protected]>; Henrik Rydberg <[email protected]>; [email protected]; [email protected]
Subject: RE: [PATCH 2/2] HID: alps: Refactor axis resolution logic
Hi, Cao-san,
Do you know AUI1657 device? This device looks U1.
I think recent all U1 devices work as PTP.
Linux also supports PTP, so I think we should add something ID to Linux source code. (I remember a something flag is already exist.)
Best Regards,
Masaki Ota
-----Original Message-----
From: Artem Borisov <[email protected]>
Sent: Friday, April 10, 2020 8:00 AM
Cc: [email protected]; 太田 真喜 Masaki Ota <[email protected]>; Artem Borisov <[email protected]>; Benjamin Tissoires <[email protected]>; Henrik Rydberg <[email protected]>; [email protected]; [email protected]
Subject: [PATCH 2/2] HID: alps: Refactor axis resolution logic
AUI1657 doesn't follow the same logic for resolution calculation, since the resulting values are incorrect. Instead, it reports the actual resolution values in place of the pitch ones.
While we're at it, also refactor the whole resolution logic to make it more generic and sensible for multiple device support.
There are two main logical problems with the current code:
1. active_len_mm values are only used for resolution calculation on U1, yet are exposed globally as part of alps_dev structure.
2. The resolution calculation process happens in alps_input_configured, while everything else is calculated in u1_init function.
These problems become more apparent when we try to support a device that doesn't follow the same resolution calculation logic as U1.
Since alps_input_configured is a device-agnostic function, we should avoid doing any measurements there and handle them in device-specific init functions like u1/T4_init instead.
To eliminate these problems we add global x_res and y_res values and populate them on a device-specific basis in the according init functions.
Signed-off-by: Artem Borisov <[email protected]>
---
drivers/hid/hid-alps.c | 41 +++++++++++++++++++++++++----------------
1 file changed, 25 insertions(+), 16 deletions(-)
diff --git a/drivers/hid/hid-alps.c b/drivers/hid/hid-alps.c index c2a2bd528890..494c08cca645 100644
--- a/drivers/hid/hid-alps.c
+++ b/drivers/hid/hid-alps.c
@@ -83,8 +83,8 @@ enum dev_num {
* @max_fingers: total number of fingers
* @has_sp: boolean of sp existense
* @sp_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_res: resolution of X
+ * @y_res: resolution of Y
* @x_max: maximum x coordinate value
* @y_max: maximum y coordinate value
* @x_min: minimum x coordinate value
@@ -100,9 +100,10 @@ struct alps_dev {
enum dev_num dev_type;
u8 max_fingers;
u8 has_sp;
+ u8 no_pitch;
u8 sp_btn_info;
- u32 x_active_len_mm;
- u32 y_active_len_mm;
+ u32 x_res;
+ u32 y_res;
u32 x_max;
u32 y_max;
u32 x_min;
@@ -550,10 +551,6 @@ static int u1_init(struct hid_device *hdev, struct alps_dev *pri_data)
dev_err(&hdev->dev, "failed U1_RESO_DWN_ABS (%d)\n", ret);
goto exit;
}
- pri_data->x_active_len_mm =
- (pitch_x * (sen_line_num_x - 1)) / 10;
- pri_data->y_active_len_mm =
- (pitch_y * (sen_line_num_y - 1)) / 10;
pri_data->x_max =
(resolution << 2) * (sen_line_num_x - 1); @@ -562,6 +559,18 @@ static int u1_init(struct hid_device *hdev, struct alps_dev *pri_data)
(resolution << 2) * (sen_line_num_y - 1);
pri_data->y_min = 1;
+ if (pri_data->no_pitch) {
+ pri_data->x_res = pitch_x;
+ pri_data->y_res = pitch_y;
+ } else {
+ pri_data->x_res =
+ (pri_data->x_max - 1) /
+ ((pitch_x * (sen_line_num_x - 1)) / 10);
+ pri_data->y_res =
+ (pri_data->y_max - 1) /
+ ((pitch_y * (sen_line_num_y - 1)) / 10);
+ }
+
ret = u1_read_write_register(hdev, ADDRESS_U1_PAD_BTN,
&tmp, 0, true);
if (ret < 0) {
@@ -622,7 +631,7 @@ static int T4_init(struct hid_device *hdev, struct alps_dev *pri_data)
pri_data->x_min = T4_COUNT_PER_ELECTRODE;
pri_data->y_max = sen_line_num_y * T4_COUNT_PER_ELECTRODE;
pri_data->y_min = T4_COUNT_PER_ELECTRODE;
- pri_data->x_active_len_mm = pri_data->y_active_len_mm = 0;
+ pri_data->x_res = pri_data->y_res = 0;
pri_data->btn_cnt = 1;
ret = t4_read_write_register(hdev, PRM_SYS_CONFIG_1, &tmp, 0, true); @@ -675,7 +684,7 @@ static int alps_input_configured(struct hid_device *hdev, struct hid_input *hi)
struct alps_dev *data = hid_get_drvdata(hdev);
struct input_dev *input = hi->input, *input2;
int ret;
- int res_x, res_y, i;
+ int i;
data->input = input;
@@ -706,12 +715,9 @@ static int alps_input_configured(struct hid_device *hdev, struct hid_input *hi)
input_set_abs_params(input, ABS_MT_POSITION_Y,
data->y_min, data->y_max, 0, 0);
- if (data->x_active_len_mm && data->y_active_len_mm) {
- res_x = (data->x_max - 1) / data->x_active_len_mm;
- res_y = (data->y_max - 1) / data->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);
+ if (data->x_res && data->y_res) {
+ input_abs_set_res(input, ABS_MT_POSITION_X, data->x_res);
+ input_abs_set_res(input, ABS_MT_POSITION_Y, data->y_res);
}
input_set_abs_params(input, ABS_MT_PRESSURE, 0, 64, 0, 0); @@ -802,8 +808,11 @@ static int alps_probe(struct hid_device *hdev, const struct hid_device_id *id)
break;
case HID_DEVICE_ID_ALPS_U1_DUAL:
case HID_DEVICE_ID_ALPS_U1:
+ data->dev_type = U1;
+ break;
case HID_DEVICE_ID_ALPS_1657:
data->dev_type = U1;
+ data->no_pitch = 1;
break;
default:
data->dev_type = UNKNOWN;
--
2.26.0
On Mon, 6 Apr 2020, Artem Borisov wrote:
> This device is used on Lenovo V130-15IKB variants and uses
> the same registers as U1.
>
> Signed-off-by: Artem Borisov <[email protected]>
I am in the meantime applying this one, and postponing the axis resolution
logic followup, waiting for Alps people to grok it.
Thanks,
--
Jiri Kosina
SUSE Labs
Hi.
I can't quite understand your objections.
1. Do you mean that we can't use AUI1657 because there are already
drivers which do that (and I'm sure there are not) or that there are
more devices using 0x121E?
2. The second instance of product ID is not used anywhere in the code
at least for U1 so I didn't see any point to add another unused value
that replicates the previous one.
3. I obviously don't have any internal documentation to argue with
that, but you have mentioned above that aui1657 is just another panel
based on U1, Apart from that, the touchpad is fully functional with
the current code submitted which wouldn't make any sense if the report
IDs were wrong.
I'm running Arch with 5.6.3 kernel, though I'm not sure how that
information will be relevant to the patch.
Thanks.
ср, 15 апр. 2020 г. в 10:47, Xiaojian Cao <[email protected]>:
>
> Hi Artem,
>
>
>
> We’ve reviewed your modifications, and already found some problems as below:
>
> It’s not suitable to use HWID AUI1657 in the source code, we because there are many projects using the same touchpad. It will confuse the coder in the future.
>
>
>
> There are 2 places that maintain the Product ID/Device ID in the mainstream driver, currently your modifications just maintained one of them.
>
>
>
> If we need to support the new Product ID/Device ID, then the report IDs will also be modified. Alps Alpine have different report ID list for different products.
>
>
>
> BTW, what is your OS version and what’s its Linux kernel version?
>
>
>
> Thank you.
>
>
>
>
>
> Best Regards,
>
> ----------------------------------------------
>
> Jason Cao(曹晓建)
>
>
>
> From: Artem Borisov <[email protected]>
> Sent: Wednesday, April 15, 2020 1:51 AM
> To: 曹 曉建 Xiaojian Cao <[email protected]>
> Cc: 太田 真喜 Masaki Ota <[email protected]>; [email protected]; Benjamin Tissoires <[email protected]>; Henrik Rydberg <[email protected]>; [email protected]; [email protected]; 野村 哲哉 Tetsuya Nomura <[email protected]>; Vadim Klishko <[email protected]>; #ALCHT_ML_POD_CN <[email protected]>
> Subject: Re: [PATCH 2/2] HID: alps: Refactor axis resolution logic
>
>
>
> Any updates on that yet?
>
> пт, 10 апр. 2020 г., 06:28 Xiaojian Cao <[email protected]>:
>
> Hi Ota-san,
>
> I need to study the background first, then I will update my understanding about it.
>
>
> Best Regards,
> ----------------------------------------------
> Jason Cao(曹晓建)
>
>
> -----Original Message-----
> From: 太田 真喜 Masaki Ota <[email protected]>
> Sent: Friday, April 10, 2020 9:51 AM
> To: 曹 曉建 Xiaojian Cao <[email protected]>; Artem Borisov <[email protected]>
> Cc: [email protected]; Benjamin Tissoires <[email protected]>; Henrik Rydberg <[email protected]>; [email protected]; [email protected]
> Subject: RE: [PATCH 2/2] HID: alps: Refactor axis resolution logic
>
> Hi, Cao-san,
>
> I got it. I also confirmed this touchpad is a special.
> What do you think this code modification?
>
> Best Regards,
> Masaki Ota
> -----Original Message-----
> From: 曹 曉建 Xiaojian Cao <[email protected]>
> Sent: Friday, April 10, 2020 10:03 AM
> To: 太田 真喜 Masaki Ota <[email protected]>; Artem Borisov <[email protected]>
> Cc: [email protected]; Benjamin Tissoires <[email protected]>; Henrik Rydberg <[email protected]>; [email protected]; [email protected]
> Subject: RE: [PATCH 2/2] HID: alps: Refactor axis resolution logic
>
> Hi Ota-san,
>
> Thanks for your checking.
> In fact, some of the U1 devices work as non-PTP.
> AUI1657 is using U1(KGDBCHA004A) whose firmware just supports mouse mode and legacy mode.
>
> Best Regards,
> ----------------------------------------------
> Jason Cao(曹晓建)
>
>
> -----Original Message-----
> From: 太田 真喜 Masaki Ota <[email protected]>
> Sent: Friday, April 10, 2020 8:29 AM
> To: 曹 曉建 Xiaojian Cao <[email protected]>; Artem Borisov <[email protected]>
> Cc: [email protected]; Benjamin Tissoires <[email protected]>; Henrik Rydberg <[email protected]>; [email protected]; [email protected]
> Subject: RE: [PATCH 2/2] HID: alps: Refactor axis resolution logic
>
> Hi, Cao-san,
>
> Do you know AUI1657 device? This device looks U1.
> I think recent all U1 devices work as PTP.
> Linux also supports PTP, so I think we should add something ID to Linux source code. (I remember a something flag is already exist.)
>
> Best Regards,
> Masaki Ota
> -----Original Message-----
> From: Artem Borisov <[email protected]>
> Sent: Friday, April 10, 2020 8:00 AM
> Cc: [email protected]; 太田 真喜 Masaki Ota <[email protected]>; Artem Borisov <[email protected]>; Benjamin Tissoires <[email protected]>; Henrik Rydberg <[email protected]>; [email protected]; [email protected]
> Subject: [PATCH 2/2] HID: alps: Refactor axis resolution logic
>
> AUI1657 doesn't follow the same logic for resolution calculation, since the resulting values are incorrect. Instead, it reports the actual resolution values in place of the pitch ones.
> While we're at it, also refactor the whole resolution logic to make it more generic and sensible for multiple device support.
>
> There are two main logical problems with the current code:
> 1. active_len_mm values are only used for resolution calculation on U1, yet are exposed globally as part of alps_dev structure.
> 2. The resolution calculation process happens in alps_input_configured, while everything else is calculated in u1_init function.
>
> These problems become more apparent when we try to support a device that doesn't follow the same resolution calculation logic as U1.
> Since alps_input_configured is a device-agnostic function, we should avoid doing any measurements there and handle them in device-specific init functions like u1/T4_init instead.
>
> To eliminate these problems we add global x_res and y_res values and populate them on a device-specific basis in the according init functions.
>
> Signed-off-by: Artem Borisov <[email protected]>
> ---
> drivers/hid/hid-alps.c | 41 +++++++++++++++++++++++++----------------
> 1 file changed, 25 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/hid/hid-alps.c b/drivers/hid/hid-alps.c index c2a2bd528890..494c08cca645 100644
> --- a/drivers/hid/hid-alps.c
> +++ b/drivers/hid/hid-alps.c
> @@ -83,8 +83,8 @@ enum dev_num {
> * @max_fingers: total number of fingers
> * @has_sp: boolean of sp existense
> * @sp_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_res: resolution of X
> + * @y_res: resolution of Y
> * @x_max: maximum x coordinate value
> * @y_max: maximum y coordinate value
> * @x_min: minimum x coordinate value
> @@ -100,9 +100,10 @@ struct alps_dev {
> enum dev_num dev_type;
> u8 max_fingers;
> u8 has_sp;
> + u8 no_pitch;
> u8 sp_btn_info;
> - u32 x_active_len_mm;
> - u32 y_active_len_mm;
> + u32 x_res;
> + u32 y_res;
> u32 x_max;
> u32 y_max;
> u32 x_min;
> @@ -550,10 +551,6 @@ static int u1_init(struct hid_device *hdev, struct alps_dev *pri_data)
> dev_err(&hdev->dev, "failed U1_RESO_DWN_ABS (%d)\n", ret);
> goto exit;
> }
> - pri_data->x_active_len_mm =
> - (pitch_x * (sen_line_num_x - 1)) / 10;
> - pri_data->y_active_len_mm =
> - (pitch_y * (sen_line_num_y - 1)) / 10;
>
> pri_data->x_max =
> (resolution << 2) * (sen_line_num_x - 1); @@ -562,6 +559,18 @@ static int u1_init(struct hid_device *hdev, struct alps_dev *pri_data)
> (resolution << 2) * (sen_line_num_y - 1);
> pri_data->y_min = 1;
>
> + if (pri_data->no_pitch) {
> + pri_data->x_res = pitch_x;
> + pri_data->y_res = pitch_y;
> + } else {
> + pri_data->x_res =
> + (pri_data->x_max - 1) /
> + ((pitch_x * (sen_line_num_x - 1)) / 10);
> + pri_data->y_res =
> + (pri_data->y_max - 1) /
> + ((pitch_y * (sen_line_num_y - 1)) / 10);
> + }
> +
> ret = u1_read_write_register(hdev, ADDRESS_U1_PAD_BTN,
> &tmp, 0, true);
> if (ret < 0) {
> @@ -622,7 +631,7 @@ static int T4_init(struct hid_device *hdev, struct alps_dev *pri_data)
> pri_data->x_min = T4_COUNT_PER_ELECTRODE;
> pri_data->y_max = sen_line_num_y * T4_COUNT_PER_ELECTRODE;
> pri_data->y_min = T4_COUNT_PER_ELECTRODE;
> - pri_data->x_active_len_mm = pri_data->y_active_len_mm = 0;
> + pri_data->x_res = pri_data->y_res = 0;
> pri_data->btn_cnt = 1;
>
> ret = t4_read_write_register(hdev, PRM_SYS_CONFIG_1, &tmp, 0, true); @@ -675,7 +684,7 @@ static int alps_input_configured(struct hid_device *hdev, struct hid_input *hi)
> struct alps_dev *data = hid_get_drvdata(hdev);
> struct input_dev *input = hi->input, *input2;
> int ret;
> - int res_x, res_y, i;
> + int i;
>
> data->input = input;
>
> @@ -706,12 +715,9 @@ static int alps_input_configured(struct hid_device *hdev, struct hid_input *hi)
> input_set_abs_params(input, ABS_MT_POSITION_Y,
> data->y_min, data->y_max, 0, 0);
>
> - if (data->x_active_len_mm && data->y_active_len_mm) {
> - res_x = (data->x_max - 1) / data->x_active_len_mm;
> - res_y = (data->y_max - 1) / data->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);
> + if (data->x_res && data->y_res) {
> + input_abs_set_res(input, ABS_MT_POSITION_X, data->x_res);
> + input_abs_set_res(input, ABS_MT_POSITION_Y, data->y_res);
> }
>
> input_set_abs_params(input, ABS_MT_PRESSURE, 0, 64, 0, 0); @@ -802,8 +808,11 @@ static int alps_probe(struct hid_device *hdev, const struct hid_device_id *id)
> break;
> case HID_DEVICE_ID_ALPS_U1_DUAL:
> case HID_DEVICE_ID_ALPS_U1:
> + data->dev_type = U1;
> + break;
> case HID_DEVICE_ID_ALPS_1657:
> data->dev_type = U1;
> + data->no_pitch = 1;
> break;
> default:
> data->dev_type = UNKNOWN;
> --
> 2.26.0
Hi Artem,
Thanks for your checking, my feedbacks are as below:
1.It is about the coding style that we should not use HWID in the string "HID_DEVICE_ID_ALPS_1657", there are a large number of HWIDs using this touchpad. We should use the device type information in this string, such as "U1_UNICORN_LEGACY".
2.This is also about the coding style, we'd better make sure they are the same with each other, or just leave one list there.
3.If we'd like to upgrade the mainstream driver for only this project, then it works, but for the other projects, it has problems. If there is enough time, we'd like to prepare a better patch for most of our products.
Thank you for sharing the kernel version. I think, the review result would not be reliable if we don't have the correct source code of your kernel version.
Best Regards,
----------------------------------------------
Jason Cao(曹晓建)
-----Original Message-----
From: Artem Borisov <[email protected]>
Sent: Wednesday, April 15, 2020 5:16 PM
To: 曹 曉建 Xiaojian Cao <[email protected]>
Cc: 太田 真喜 Masaki Ota <[email protected]>; [email protected]; Benjamin Tissoires <[email protected]>; Henrik Rydberg <[email protected]>; [email protected]; [email protected]; 野村 哲哉 Tetsuya Nomura <[email protected]>; Vadim Klishko <[email protected]>; #ALCHT_ML_POD_CN <[email protected]>
Subject: Re: [PATCH 2/2] HID: alps: Refactor axis resolution logic
Hi.
I can't quite understand your objections.
1. Do you mean that we can't use AUI1657 because there are already drivers which do that (and I'm sure there are not) or that there are more devices using 0x121E?
2. The second instance of product ID is not used anywhere in the code at least for U1 so I didn't see any point to add another unused value that replicates the previous one.
3. I obviously don't have any internal documentation to argue with that, but you have mentioned above that aui1657 is just another panel based on U1, Apart from that, the touchpad is fully functional with the current code submitted which wouldn't make any sense if the report IDs were wrong.
I'm running Arch with 5.6.3 kernel, though I'm not sure how that information will be relevant to the patch.
Thanks.
ср, 15 апр. 2020 г. в 10:47, Xiaojian Cao <[email protected]>:
>
> Hi Artem,
>
>
>
> We’ve reviewed your modifications, and already found some problems as below:
>
> It’s not suitable to use HWID AUI1657 in the source code, we because there are many projects using the same touchpad. It will confuse the coder in the future.
>
>
>
> There are 2 places that maintain the Product ID/Device ID in the mainstream driver, currently your modifications just maintained one of them.
>
>
>
> If we need to support the new Product ID/Device ID, then the report IDs will also be modified. Alps Alpine have different report ID list for different products.
>
>
>
> BTW, what is your OS version and what’s its Linux kernel version?
>
>
>
> Thank you.
>
>
>
>
>
> Best Regards,
>
> ----------------------------------------------
>
> Jason Cao(曹晓建)
>
>
>
> From: Artem Borisov <[email protected]>
> Sent: Wednesday, April 15, 2020 1:51 AM
> To: 曹 曉建 Xiaojian Cao <[email protected]>
> Cc: 太田 真喜 Masaki Ota <[email protected]>; [email protected];
> Benjamin Tissoires <[email protected]>; Henrik Rydberg
> <[email protected]>; [email protected];
> [email protected]; 野村 哲哉 Tetsuya Nomura
> <[email protected]>; Vadim Klishko <[email protected]>;
> #ALCHT_ML_POD_CN <[email protected]>
> Subject: Re: [PATCH 2/2] HID: alps: Refactor axis resolution logic
>
>
>
> Any updates on that yet?
>
> пт, 10 апр. 2020 г., 06:28 Xiaojian Cao <[email protected]>:
>
> Hi Ota-san,
>
> I need to study the background first, then I will update my understanding about it.
>
>
> Best Regards,
> ----------------------------------------------
> Jason Cao(曹晓建)
>
>
> -----Original Message-----
> From: 太田 真喜 Masaki Ota <[email protected]>
> Sent: Friday, April 10, 2020 9:51 AM
> To: 曹 曉建 Xiaojian Cao <[email protected]>; Artem Borisov
> <[email protected]>
> Cc: [email protected]; Benjamin Tissoires
> <[email protected]>; Henrik Rydberg <[email protected]>;
> [email protected]; [email protected]
> Subject: RE: [PATCH 2/2] HID: alps: Refactor axis resolution logic
>
> Hi, Cao-san,
>
> I got it. I also confirmed this touchpad is a special.
> What do you think this code modification?
>
> Best Regards,
> Masaki Ota
> -----Original Message-----
> From: 曹 曉建 Xiaojian Cao <[email protected]>
> Sent: Friday, April 10, 2020 10:03 AM
> To: 太田 真喜 Masaki Ota <[email protected]>; Artem Borisov
> <[email protected]>
> Cc: [email protected]; Benjamin Tissoires
> <[email protected]>; Henrik Rydberg <[email protected]>;
> [email protected]; [email protected]
> Subject: RE: [PATCH 2/2] HID: alps: Refactor axis resolution logic
>
> Hi Ota-san,
>
> Thanks for your checking.
> In fact, some of the U1 devices work as non-PTP.
> AUI1657 is using U1(KGDBCHA004A) whose firmware just supports mouse mode and legacy mode.
>
> Best Regards,
> ----------------------------------------------
> Jason Cao(曹晓建)
>
>
> -----Original Message-----
> From: 太田 真喜 Masaki Ota <[email protected]>
> Sent: Friday, April 10, 2020 8:29 AM
> To: 曹 曉建 Xiaojian Cao <[email protected]>; Artem Borisov
> <[email protected]>
> Cc: [email protected]; Benjamin Tissoires
> <[email protected]>; Henrik Rydberg <[email protected]>;
> [email protected]; [email protected]
> Subject: RE: [PATCH 2/2] HID: alps: Refactor axis resolution logic
>
> Hi, Cao-san,
>
> Do you know AUI1657 device? This device looks U1.
> I think recent all U1 devices work as PTP.
> Linux also supports PTP, so I think we should add something ID to
> Linux source code. (I remember a something flag is already exist.)
>
> Best Regards,
> Masaki Ota
> -----Original Message-----
> From: Artem Borisov <[email protected]>
> Sent: Friday, April 10, 2020 8:00 AM
> Cc: [email protected]; 太田 真喜 Masaki Ota <[email protected]>;
> Artem Borisov <[email protected]>; Benjamin Tissoires
> <[email protected]>; Henrik Rydberg <[email protected]>;
> [email protected]; [email protected]
> Subject: [PATCH 2/2] HID: alps: Refactor axis resolution logic
>
> AUI1657 doesn't follow the same logic for resolution calculation, since the resulting values are incorrect. Instead, it reports the actual resolution values in place of the pitch ones.
> While we're at it, also refactor the whole resolution logic to make it more generic and sensible for multiple device support.
>
> There are two main logical problems with the current code:
> 1. active_len_mm values are only used for resolution calculation on U1, yet are exposed globally as part of alps_dev structure.
> 2. The resolution calculation process happens in alps_input_configured, while everything else is calculated in u1_init function.
>
> These problems become more apparent when we try to support a device that doesn't follow the same resolution calculation logic as U1.
> Since alps_input_configured is a device-agnostic function, we should avoid doing any measurements there and handle them in device-specific init functions like u1/T4_init instead.
>
> To eliminate these problems we add global x_res and y_res values and populate them on a device-specific basis in the according init functions.
>
> Signed-off-by: Artem Borisov <[email protected]>
> ---
> drivers/hid/hid-alps.c | 41 +++++++++++++++++++++++++----------------
> 1 file changed, 25 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/hid/hid-alps.c b/drivers/hid/hid-alps.c index
> c2a2bd528890..494c08cca645 100644
> --- a/drivers/hid/hid-alps.c
> +++ b/drivers/hid/hid-alps.c
> @@ -83,8 +83,8 @@ enum dev_num {
> * @max_fingers: total number of fingers
> * @has_sp: boolean of sp existense
> * @sp_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_res: resolution of X
> + * @y_res: resolution of Y
> * @x_max: maximum x coordinate value
> * @y_max: maximum y coordinate value
> * @x_min: minimum x coordinate value @@ -100,9 +100,10 @@ struct
> alps_dev {
> enum dev_num dev_type;
> u8 max_fingers;
> u8 has_sp;
> + u8 no_pitch;
> u8 sp_btn_info;
> - u32 x_active_len_mm;
> - u32 y_active_len_mm;
> + u32 x_res;
> + u32 y_res;
> u32 x_max;
> u32 y_max;
> u32 x_min;
> @@ -550,10 +551,6 @@ static int u1_init(struct hid_device *hdev, struct alps_dev *pri_data)
> dev_err(&hdev->dev, "failed U1_RESO_DWN_ABS (%d)\n", ret);
> goto exit;
> }
> - pri_data->x_active_len_mm =
> - (pitch_x * (sen_line_num_x - 1)) / 10;
> - pri_data->y_active_len_mm =
> - (pitch_y * (sen_line_num_y - 1)) / 10;
>
> pri_data->x_max =
> (resolution << 2) * (sen_line_num_x - 1); @@ -562,6 +559,18 @@ static int u1_init(struct hid_device *hdev, struct alps_dev *pri_data)
> (resolution << 2) * (sen_line_num_y - 1);
> pri_data->y_min = 1;
>
> + if (pri_data->no_pitch) {
> + pri_data->x_res = pitch_x;
> + pri_data->y_res = pitch_y;
> + } else {
> + pri_data->x_res =
> + (pri_data->x_max - 1) /
> + ((pitch_x * (sen_line_num_x - 1)) / 10);
> + pri_data->y_res =
> + (pri_data->y_max - 1) /
> + ((pitch_y * (sen_line_num_y - 1)) / 10);
> + }
> +
> ret = u1_read_write_register(hdev, ADDRESS_U1_PAD_BTN,
> &tmp, 0, true);
> if (ret < 0) {
> @@ -622,7 +631,7 @@ static int T4_init(struct hid_device *hdev, struct alps_dev *pri_data)
> pri_data->x_min = T4_COUNT_PER_ELECTRODE;
> pri_data->y_max = sen_line_num_y * T4_COUNT_PER_ELECTRODE;
> pri_data->y_min = T4_COUNT_PER_ELECTRODE;
> - pri_data->x_active_len_mm = pri_data->y_active_len_mm = 0;
> + pri_data->x_res = pri_data->y_res = 0;
> pri_data->btn_cnt = 1;
>
> ret = t4_read_write_register(hdev, PRM_SYS_CONFIG_1, &tmp, 0, true); @@ -675,7 +684,7 @@ static int alps_input_configured(struct hid_device *hdev, struct hid_input *hi)
> struct alps_dev *data = hid_get_drvdata(hdev);
> struct input_dev *input = hi->input, *input2;
> int ret;
> - int res_x, res_y, i;
> + int i;
>
> data->input = input;
>
> @@ -706,12 +715,9 @@ static int alps_input_configured(struct hid_device *hdev, struct hid_input *hi)
> input_set_abs_params(input, ABS_MT_POSITION_Y,
> data->y_min,
> data->y_max, 0, 0);
>
> - if (data->x_active_len_mm && data->y_active_len_mm) {
> - res_x = (data->x_max - 1) / data->x_active_len_mm;
> - res_y = (data->y_max - 1) / data->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);
> + if (data->x_res && data->y_res) {
> + input_abs_set_res(input, ABS_MT_POSITION_X, data->x_res);
> + input_abs_set_res(input, ABS_MT_POSITION_Y,
> + data->y_res);
> }
>
> input_set_abs_params(input, ABS_MT_PRESSURE, 0, 64, 0, 0); @@ -802,8 +808,11 @@ static int alps_probe(struct hid_device *hdev, const struct hid_device_id *id)
> break;
> case HID_DEVICE_ID_ALPS_U1_DUAL:
> case HID_DEVICE_ID_ALPS_U1:
> + data->dev_type = U1;
> + break;
> case HID_DEVICE_ID_ALPS_1657:
> data->dev_type = U1;
> + data->no_pitch = 1;
> break;
> default:
> data->dev_type = UNKNOWN;
> --
> 2.26.0
Well, at that point it's something you should talk to hid maintainers
about. The base patch is already submitted and scheduled for 5.7 as
it's enough to make this touchpad work. The second one is mainly
necessary to fix the deadzones around the corners and that was the one
to review. If you prefer to not have any major refactoring until you
prepare a better patch, I'm okay with that, since we can workaround
the resolution issue via libinput quirks for now.
ср, 15 апр. 2020 г. в 13:54, Xiaojian Cao <[email protected]>:
>
> Hi Artem,
>
> Thanks for your checking, my feedbacks are as below:
> 1.It is about the coding style that we should not use HWID in the string "HID_DEVICE_ID_ALPS_1657", there are a large number of HWIDs using this touchpad. We should use the device type information in this string, such as "U1_UNICORN_LEGACY".
> 2.This is also about the coding style, we'd better make sure they are the same with each other, or just leave one list there.
> 3.If we'd like to upgrade the mainstream driver for only this project, then it works, but for the other projects, it has problems. If there is enough time, we'd like to prepare a better patch for most of our products.
>
> Thank you for sharing the kernel version. I think, the review result would not be reliable if we don't have the correct source code of your kernel version.
>
>
>
> Best Regards,
> ----------------------------------------------
> Jason Cao(曹晓建)
>
>
> -----Original Message-----
> From: Artem Borisov <[email protected]>
> Sent: Wednesday, April 15, 2020 5:16 PM
> To: 曹 曉建 Xiaojian Cao <[email protected]>
> Cc: 太田 真喜 Masaki Ota <[email protected]>; [email protected]; Benjamin Tissoires <[email protected]>; Henrik Rydberg <[email protected]>; [email protected]; [email protected]; 野村 哲哉 Tetsuya Nomura <[email protected]>; Vadim Klishko <[email protected]>; #ALCHT_ML_POD_CN <[email protected]>
> Subject: Re: [PATCH 2/2] HID: alps: Refactor axis resolution logic
>
> Hi.
>
> I can't quite understand your objections.
> 1. Do you mean that we can't use AUI1657 because there are already drivers which do that (and I'm sure there are not) or that there are more devices using 0x121E?
> 2. The second instance of product ID is not used anywhere in the code at least for U1 so I didn't see any point to add another unused value that replicates the previous one.
> 3. I obviously don't have any internal documentation to argue with that, but you have mentioned above that aui1657 is just another panel based on U1, Apart from that, the touchpad is fully functional with the current code submitted which wouldn't make any sense if the report IDs were wrong.
>
> I'm running Arch with 5.6.3 kernel, though I'm not sure how that information will be relevant to the patch.
>
> Thanks.
>
>
> ср, 15 апр. 2020 г. в 10:47, Xiaojian Cao <[email protected]>:
> >
> > Hi Artem,
> >
> >
> >
> > We’ve reviewed your modifications, and already found some problems as below:
> >
> > It’s not suitable to use HWID AUI1657 in the source code, we because there are many projects using the same touchpad. It will confuse the coder in the future.
> >
> >
> >
> > There are 2 places that maintain the Product ID/Device ID in the mainstream driver, currently your modifications just maintained one of them.
> >
> >
> >
> > If we need to support the new Product ID/Device ID, then the report IDs will also be modified. Alps Alpine have different report ID list for different products.
> >
> >
> >
> > BTW, what is your OS version and what’s its Linux kernel version?
> >
> >
> >
> > Thank you.
> >
> >
> >
> >
> >
> > Best Regards,
> >
> > ----------------------------------------------
> >
> > Jason Cao(曹晓建)
> >
> >
> >
> > From: Artem Borisov <[email protected]>
> > Sent: Wednesday, April 15, 2020 1:51 AM
> > To: 曹 曉建 Xiaojian Cao <[email protected]>
> > Cc: 太田 真喜 Masaki Ota <[email protected]>; [email protected];
> > Benjamin Tissoires <[email protected]>; Henrik Rydberg
> > <[email protected]>; [email protected];
> > [email protected]; 野村 哲哉 Tetsuya Nomura
> > <[email protected]>; Vadim Klishko <[email protected]>;
> > #ALCHT_ML_POD_CN <[email protected]>
> > Subject: Re: [PATCH 2/2] HID: alps: Refactor axis resolution logic
> >
> >
> >
> > Any updates on that yet?
> >
> > пт, 10 апр. 2020 г., 06:28 Xiaojian Cao <[email protected]>:
> >
> > Hi Ota-san,
> >
> > I need to study the background first, then I will update my understanding about it.
> >
> >
> > Best Regards,
> > ----------------------------------------------
> > Jason Cao(曹晓建)
> >
> >
> > -----Original Message-----
> > From: 太田 真喜 Masaki Ota <[email protected]>
> > Sent: Friday, April 10, 2020 9:51 AM
> > To: 曹 曉建 Xiaojian Cao <[email protected]>; Artem Borisov
> > <[email protected]>
> > Cc: [email protected]; Benjamin Tissoires
> > <[email protected]>; Henrik Rydberg <[email protected]>;
> > [email protected]; [email protected]
> > Subject: RE: [PATCH 2/2] HID: alps: Refactor axis resolution logic
> >
> > Hi, Cao-san,
> >
> > I got it. I also confirmed this touchpad is a special.
> > What do you think this code modification?
> >
> > Best Regards,
> > Masaki Ota
> > -----Original Message-----
> > From: 曹 曉建 Xiaojian Cao <[email protected]>
> > Sent: Friday, April 10, 2020 10:03 AM
> > To: 太田 真喜 Masaki Ota <[email protected]>; Artem Borisov
> > <[email protected]>
> > Cc: [email protected]; Benjamin Tissoires
> > <[email protected]>; Henrik Rydberg <[email protected]>;
> > [email protected]; [email protected]
> > Subject: RE: [PATCH 2/2] HID: alps: Refactor axis resolution logic
> >
> > Hi Ota-san,
> >
> > Thanks for your checking.
> > In fact, some of the U1 devices work as non-PTP.
> > AUI1657 is using U1(KGDBCHA004A) whose firmware just supports mouse mode and legacy mode.
> >
> > Best Regards,
> > ----------------------------------------------
> > Jason Cao(曹晓建)
> >
> >
> > -----Original Message-----
> > From: 太田 真喜 Masaki Ota <[email protected]>
> > Sent: Friday, April 10, 2020 8:29 AM
> > To: 曹 曉建 Xiaojian Cao <[email protected]>; Artem Borisov
> > <[email protected]>
> > Cc: [email protected]; Benjamin Tissoires
> > <[email protected]>; Henrik Rydberg <[email protected]>;
> > [email protected]; [email protected]
> > Subject: RE: [PATCH 2/2] HID: alps: Refactor axis resolution logic
> >
> > Hi, Cao-san,
> >
> > Do you know AUI1657 device? This device looks U1.
> > I think recent all U1 devices work as PTP.
> > Linux also supports PTP, so I think we should add something ID to
> > Linux source code. (I remember a something flag is already exist.)
> >
> > Best Regards,
> > Masaki Ota
> > -----Original Message-----
> > From: Artem Borisov <[email protected]>
> > Sent: Friday, April 10, 2020 8:00 AM
> > Cc: [email protected]; 太田 真喜 Masaki Ota <[email protected]>;
> > Artem Borisov <[email protected]>; Benjamin Tissoires
> > <[email protected]>; Henrik Rydberg <[email protected]>;
> > [email protected]; [email protected]
> > Subject: [PATCH 2/2] HID: alps: Refactor axis resolution logic
> >
> > AUI1657 doesn't follow the same logic for resolution calculation, since the resulting values are incorrect. Instead, it reports the actual resolution values in place of the pitch ones.
> > While we're at it, also refactor the whole resolution logic to make it more generic and sensible for multiple device support.
> >
> > There are two main logical problems with the current code:
> > 1. active_len_mm values are only used for resolution calculation on U1, yet are exposed globally as part of alps_dev structure.
> > 2. The resolution calculation process happens in alps_input_configured, while everything else is calculated in u1_init function.
> >
> > These problems become more apparent when we try to support a device that doesn't follow the same resolution calculation logic as U1.
> > Since alps_input_configured is a device-agnostic function, we should avoid doing any measurements there and handle them in device-specific init functions like u1/T4_init instead.
> >
> > To eliminate these problems we add global x_res and y_res values and populate them on a device-specific basis in the according init functions.
> >
> > Signed-off-by: Artem Borisov <[email protected]>
> > ---
> > drivers/hid/hid-alps.c | 41 +++++++++++++++++++++++++----------------
> > 1 file changed, 25 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/hid/hid-alps.c b/drivers/hid/hid-alps.c index
> > c2a2bd528890..494c08cca645 100644
> > --- a/drivers/hid/hid-alps.c
> > +++ b/drivers/hid/hid-alps.c
> > @@ -83,8 +83,8 @@ enum dev_num {
> > * @max_fingers: total number of fingers
> > * @has_sp: boolean of sp existense
> > * @sp_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_res: resolution of X
> > + * @y_res: resolution of Y
> > * @x_max: maximum x coordinate value
> > * @y_max: maximum y coordinate value
> > * @x_min: minimum x coordinate value @@ -100,9 +100,10 @@ struct
> > alps_dev {
> > enum dev_num dev_type;
> > u8 max_fingers;
> > u8 has_sp;
> > + u8 no_pitch;
> > u8 sp_btn_info;
> > - u32 x_active_len_mm;
> > - u32 y_active_len_mm;
> > + u32 x_res;
> > + u32 y_res;
> > u32 x_max;
> > u32 y_max;
> > u32 x_min;
> > @@ -550,10 +551,6 @@ static int u1_init(struct hid_device *hdev, struct alps_dev *pri_data)
> > dev_err(&hdev->dev, "failed U1_RESO_DWN_ABS (%d)\n", ret);
> > goto exit;
> > }
> > - pri_data->x_active_len_mm =
> > - (pitch_x * (sen_line_num_x - 1)) / 10;
> > - pri_data->y_active_len_mm =
> > - (pitch_y * (sen_line_num_y - 1)) / 10;
> >
> > pri_data->x_max =
> > (resolution << 2) * (sen_line_num_x - 1); @@ -562,6 +559,18 @@ static int u1_init(struct hid_device *hdev, struct alps_dev *pri_data)
> > (resolution << 2) * (sen_line_num_y - 1);
> > pri_data->y_min = 1;
> >
> > + if (pri_data->no_pitch) {
> > + pri_data->x_res = pitch_x;
> > + pri_data->y_res = pitch_y;
> > + } else {
> > + pri_data->x_res =
> > + (pri_data->x_max - 1) /
> > + ((pitch_x * (sen_line_num_x - 1)) / 10);
> > + pri_data->y_res =
> > + (pri_data->y_max - 1) /
> > + ((pitch_y * (sen_line_num_y - 1)) / 10);
> > + }
> > +
> > ret = u1_read_write_register(hdev, ADDRESS_U1_PAD_BTN,
> > &tmp, 0, true);
> > if (ret < 0) {
> > @@ -622,7 +631,7 @@ static int T4_init(struct hid_device *hdev, struct alps_dev *pri_data)
> > pri_data->x_min = T4_COUNT_PER_ELECTRODE;
> > pri_data->y_max = sen_line_num_y * T4_COUNT_PER_ELECTRODE;
> > pri_data->y_min = T4_COUNT_PER_ELECTRODE;
> > - pri_data->x_active_len_mm = pri_data->y_active_len_mm = 0;
> > + pri_data->x_res = pri_data->y_res = 0;
> > pri_data->btn_cnt = 1;
> >
> > ret = t4_read_write_register(hdev, PRM_SYS_CONFIG_1, &tmp, 0, true); @@ -675,7 +684,7 @@ static int alps_input_configured(struct hid_device *hdev, struct hid_input *hi)
> > struct alps_dev *data = hid_get_drvdata(hdev);
> > struct input_dev *input = hi->input, *input2;
> > int ret;
> > - int res_x, res_y, i;
> > + int i;
> >
> > data->input = input;
> >
> > @@ -706,12 +715,9 @@ static int alps_input_configured(struct hid_device *hdev, struct hid_input *hi)
> > input_set_abs_params(input, ABS_MT_POSITION_Y,
> > data->y_min,
> > data->y_max, 0, 0);
> >
> > - if (data->x_active_len_mm && data->y_active_len_mm) {
> > - res_x = (data->x_max - 1) / data->x_active_len_mm;
> > - res_y = (data->y_max - 1) / data->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);
> > + if (data->x_res && data->y_res) {
> > + input_abs_set_res(input, ABS_MT_POSITION_X, data->x_res);
> > + input_abs_set_res(input, ABS_MT_POSITION_Y,
> > + data->y_res);
> > }
> >
> > input_set_abs_params(input, ABS_MT_PRESSURE, 0, 64, 0, 0); @@ -802,8 +808,11 @@ static int alps_probe(struct hid_device *hdev, const struct hid_device_id *id)
> > break;
> > case HID_DEVICE_ID_ALPS_U1_DUAL:
> > case HID_DEVICE_ID_ALPS_U1:
> > + data->dev_type = U1;
> > + break;
> > case HID_DEVICE_ID_ALPS_1657:
> > data->dev_type = U1;
> > + data->no_pitch = 1;
> > break;
> > default:
> > data->dev_type = UNKNOWN;
> > --
> > 2.26.0
On Wed, 15 Apr 2020, Xiaojian Cao wrote:
> Thanks for your checking, my feedbacks are as below:
>
> 1.It is about the coding style that we should not use HWID in the string
> "HID_DEVICE_ID_ALPS_1657", there are a large number of HWIDs using this
> touchpad. We should use the device type information in this string, such
> as "U1_UNICORN_LEGACY".
Ok, thanks for the feedback. Based on it, I am queuing the patch below.
HID_DEVICE_ID_ALPS_1657 PID is too specific, as there are many other
ALPS hardware IDs using this particular touchpad.
Rename the identifier to HID_DEVICE_ID_ALPS_U1_UNICORN_LEGACY in order
to describe reality better.
Fixes: 640e403b1fd24 ("HID: alps: Add AUI1657 device ID")
Reported-by: Xiaojian Cao <[email protected]>
Signed-off-by: Jiri Kosina <[email protected]>
---
drivers/hid/hid-alps.c | 2 +-
drivers/hid/hid-ids.h | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/hid/hid-alps.c b/drivers/hid/hid-alps.c
index c2a2bd528890..b2ad319a74b9 100644
--- a/drivers/hid/hid-alps.c
+++ b/drivers/hid/hid-alps.c
@@ -802,7 +802,7 @@ static int alps_probe(struct hid_device *hdev, const struct hid_device_id *id)
break;
case HID_DEVICE_ID_ALPS_U1_DUAL:
case HID_DEVICE_ID_ALPS_U1:
- case HID_DEVICE_ID_ALPS_1657:
+ case HID_DEVICE_ID_ALPS_U1_UNICORN_LEGACY:
data->dev_type = U1;
break;
default:
diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index e3e2fa6733fb..6eb25b9e8575 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -79,9 +79,9 @@
#define HID_DEVICE_ID_ALPS_U1_DUAL_PTP 0x121F
#define HID_DEVICE_ID_ALPS_U1_DUAL_3BTN_PTP 0x1220
#define HID_DEVICE_ID_ALPS_U1 0x1215
+#define HID_DEVICE_ID_ALPS_U1_UNICORN_LEGACY 0x121E
#define HID_DEVICE_ID_ALPS_T4_BTNLESS 0x120C
#define HID_DEVICE_ID_ALPS_1222 0x1222
-#define HID_DEVICE_ID_ALPS_1657 0x121E
#define USB_VENDOR_ID_AMI 0x046b
#define USB_DEVICE_ID_AMI_VIRT_KEYBOARD_AND_MOUSE 0xff10
--
Jiri Kosina
SUSE Labs
Hi Artem,
Yes, of course, we are also talking with Jiri.
BTW:
What's your job and responsibility?
Best Regards,
----------------------------------------------
Jason Cao(曹晓建)
-----Original Message-----
From: Artem Borisov <[email protected]>
Sent: Wednesday, April 15, 2020 6:15 PM
To: 曹 曉建 Xiaojian Cao <[email protected]>
Cc: 太田 真喜 Masaki Ota <[email protected]>; [email protected]; Benjamin Tissoires <[email protected]>; Henrik Rydberg <[email protected]>; [email protected]; [email protected]; 野村 哲哉 Tetsuya Nomura <[email protected]>; Vadim Klishko <[email protected]>; #ALCHT_ML_POD_CN <[email protected]>
Subject: Re: [PATCH 2/2] HID: alps: Refactor axis resolution logic
Well, at that point it's something you should talk to hid maintainers about. The base patch is already submitted and scheduled for 5.7 as it's enough to make this touchpad work. The second one is mainly necessary to fix the deadzones around the corners and that was the one to review. If you prefer to not have any major refactoring until you prepare a better patch, I'm okay with that, since we can workaround the resolution issue via libinput quirks for now.
ср, 15 апр. 2020 г. в 13:54, Xiaojian Cao <[email protected]>:
>
> Hi Artem,
>
> Thanks for your checking, my feedbacks are as below:
> 1.It is about the coding style that we should not use HWID in the string "HID_DEVICE_ID_ALPS_1657", there are a large number of HWIDs using this touchpad. We should use the device type information in this string, such as "U1_UNICORN_LEGACY".
> 2.This is also about the coding style, we'd better make sure they are the same with each other, or just leave one list there.
> 3.If we'd like to upgrade the mainstream driver for only this project, then it works, but for the other projects, it has problems. If there is enough time, we'd like to prepare a better patch for most of our products.
>
> Thank you for sharing the kernel version. I think, the review result would not be reliable if we don't have the correct source code of your kernel version.
>
>
>
> Best Regards,
> ----------------------------------------------
> Jason Cao(曹晓建)
>
>
> -----Original Message-----
> From: Artem Borisov <[email protected]>
> Sent: Wednesday, April 15, 2020 5:16 PM
> To: 曹 曉建 Xiaojian Cao <[email protected]>
> Cc: 太田 真喜 Masaki Ota <[email protected]>; [email protected];
> Benjamin Tissoires <[email protected]>; Henrik Rydberg
> <[email protected]>; [email protected];
> [email protected]; 野村 哲哉 Tetsuya Nomura
> <[email protected]>; Vadim Klishko <[email protected]>;
> #ALCHT_ML_POD_CN <[email protected]>
> Subject: Re: [PATCH 2/2] HID: alps: Refactor axis resolution logic
>
> Hi.
>
> I can't quite understand your objections.
> 1. Do you mean that we can't use AUI1657 because there are already drivers which do that (and I'm sure there are not) or that there are more devices using 0x121E?
> 2. The second instance of product ID is not used anywhere in the code at least for U1 so I didn't see any point to add another unused value that replicates the previous one.
> 3. I obviously don't have any internal documentation to argue with that, but you have mentioned above that aui1657 is just another panel based on U1, Apart from that, the touchpad is fully functional with the current code submitted which wouldn't make any sense if the report IDs were wrong.
>
> I'm running Arch with 5.6.3 kernel, though I'm not sure how that information will be relevant to the patch.
>
> Thanks.
>
>
> ср, 15 апр. 2020 г. в 10:47, Xiaojian Cao <[email protected]>:
> >
> > Hi Artem,
> >
> >
> >
> > We’ve reviewed your modifications, and already found some problems as below:
> >
> > It’s not suitable to use HWID AUI1657 in the source code, we because there are many projects using the same touchpad. It will confuse the coder in the future.
> >
> >
> >
> > There are 2 places that maintain the Product ID/Device ID in the mainstream driver, currently your modifications just maintained one of them.
> >
> >
> >
> > If we need to support the new Product ID/Device ID, then the report IDs will also be modified. Alps Alpine have different report ID list for different products.
> >
> >
> >
> > BTW, what is your OS version and what’s its Linux kernel version?
> >
> >
> >
> > Thank you.
> >
> >
> >
> >
> >
> > Best Regards,
> >
> > ----------------------------------------------
> >
> > Jason Cao(曹晓建)
> >
> >
> >
> > From: Artem Borisov <[email protected]>
> > Sent: Wednesday, April 15, 2020 1:51 AM
> > To: 曹 曉建 Xiaojian Cao <[email protected]>
> > Cc: 太田 真喜 Masaki Ota <[email protected]>; [email protected];
> > Benjamin Tissoires <[email protected]>; Henrik Rydberg
> > <[email protected]>; [email protected];
> > [email protected]; 野村 哲哉 Tetsuya Nomura
> > <[email protected]>; Vadim Klishko <[email protected]>;
> > #ALCHT_ML_POD_CN <[email protected]>
> > Subject: Re: [PATCH 2/2] HID: alps: Refactor axis resolution logic
> >
> >
> >
> > Any updates on that yet?
> >
> > пт, 10 апр. 2020 г., 06:28 Xiaojian Cao <[email protected]>:
> >
> > Hi Ota-san,
> >
> > I need to study the background first, then I will update my understanding about it.
> >
> >
> > Best Regards,
> > ----------------------------------------------
> > Jason Cao(曹晓建)
> >
> >
> > -----Original Message-----
> > From: 太田 真喜 Masaki Ota <[email protected]>
> > Sent: Friday, April 10, 2020 9:51 AM
> > To: 曹 曉建 Xiaojian Cao <[email protected]>; Artem Borisov
> > <[email protected]>
> > Cc: [email protected]; Benjamin Tissoires
> > <[email protected]>; Henrik Rydberg
> > <[email protected]>; [email protected];
> > [email protected]
> > Subject: RE: [PATCH 2/2] HID: alps: Refactor axis resolution logic
> >
> > Hi, Cao-san,
> >
> > I got it. I also confirmed this touchpad is a special.
> > What do you think this code modification?
> >
> > Best Regards,
> > Masaki Ota
> > -----Original Message-----
> > From: 曹 曉建 Xiaojian Cao <[email protected]>
> > Sent: Friday, April 10, 2020 10:03 AM
> > To: 太田 真喜 Masaki Ota <[email protected]>; Artem Borisov
> > <[email protected]>
> > Cc: [email protected]; Benjamin Tissoires
> > <[email protected]>; Henrik Rydberg
> > <[email protected]>; [email protected];
> > [email protected]
> > Subject: RE: [PATCH 2/2] HID: alps: Refactor axis resolution logic
> >
> > Hi Ota-san,
> >
> > Thanks for your checking.
> > In fact, some of the U1 devices work as non-PTP.
> > AUI1657 is using U1(KGDBCHA004A) whose firmware just supports mouse mode and legacy mode.
> >
> > Best Regards,
> > ----------------------------------------------
> > Jason Cao(曹晓建)
> >
> >
> > -----Original Message-----
> > From: 太田 真喜 Masaki Ota <[email protected]>
> > Sent: Friday, April 10, 2020 8:29 AM
> > To: 曹 曉建 Xiaojian Cao <[email protected]>; Artem Borisov
> > <[email protected]>
> > Cc: [email protected]; Benjamin Tissoires
> > <[email protected]>; Henrik Rydberg
> > <[email protected]>; [email protected];
> > [email protected]
> > Subject: RE: [PATCH 2/2] HID: alps: Refactor axis resolution logic
> >
> > Hi, Cao-san,
> >
> > Do you know AUI1657 device? This device looks U1.
> > I think recent all U1 devices work as PTP.
> > Linux also supports PTP, so I think we should add something ID to
> > Linux source code. (I remember a something flag is already exist.)
> >
> > Best Regards,
> > Masaki Ota
> > -----Original Message-----
> > From: Artem Borisov <[email protected]>
> > Sent: Friday, April 10, 2020 8:00 AM
> > Cc: [email protected]; 太田 真喜 Masaki Ota <[email protected]>;
> > Artem Borisov <[email protected]>; Benjamin Tissoires
> > <[email protected]>; Henrik Rydberg
> > <[email protected]>; [email protected];
> > [email protected]
> > Subject: [PATCH 2/2] HID: alps: Refactor axis resolution logic
> >
> > AUI1657 doesn't follow the same logic for resolution calculation, since the resulting values are incorrect. Instead, it reports the actual resolution values in place of the pitch ones.
> > While we're at it, also refactor the whole resolution logic to make it more generic and sensible for multiple device support.
> >
> > There are two main logical problems with the current code:
> > 1. active_len_mm values are only used for resolution calculation on U1, yet are exposed globally as part of alps_dev structure.
> > 2. The resolution calculation process happens in alps_input_configured, while everything else is calculated in u1_init function.
> >
> > These problems become more apparent when we try to support a device that doesn't follow the same resolution calculation logic as U1.
> > Since alps_input_configured is a device-agnostic function, we should avoid doing any measurements there and handle them in device-specific init functions like u1/T4_init instead.
> >
> > To eliminate these problems we add global x_res and y_res values and populate them on a device-specific basis in the according init functions.
> >
> > Signed-off-by: Artem Borisov <[email protected]>
> > ---
> > drivers/hid/hid-alps.c | 41
> > +++++++++++++++++++++++++----------------
> > 1 file changed, 25 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/hid/hid-alps.c b/drivers/hid/hid-alps.c index
> > c2a2bd528890..494c08cca645 100644
> > --- a/drivers/hid/hid-alps.c
> > +++ b/drivers/hid/hid-alps.c
> > @@ -83,8 +83,8 @@ enum dev_num {
> > * @max_fingers: total number of fingers
> > * @has_sp: boolean of sp existense
> > * @sp_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_res: resolution of X
> > + * @y_res: resolution of Y
> > * @x_max: maximum x coordinate value
> > * @y_max: maximum y coordinate value
> > * @x_min: minimum x coordinate value @@ -100,9 +100,10 @@ struct
> > alps_dev {
> > enum dev_num dev_type;
> > u8 max_fingers;
> > u8 has_sp;
> > + u8 no_pitch;
> > u8 sp_btn_info;
> > - u32 x_active_len_mm;
> > - u32 y_active_len_mm;
> > + u32 x_res;
> > + u32 y_res;
> > u32 x_max;
> > u32 y_max;
> > u32 x_min;
> > @@ -550,10 +551,6 @@ static int u1_init(struct hid_device *hdev, struct alps_dev *pri_data)
> > dev_err(&hdev->dev, "failed U1_RESO_DWN_ABS (%d)\n", ret);
> > goto exit;
> > }
> > - pri_data->x_active_len_mm =
> > - (pitch_x * (sen_line_num_x - 1)) / 10;
> > - pri_data->y_active_len_mm =
> > - (pitch_y * (sen_line_num_y - 1)) / 10;
> >
> > pri_data->x_max =
> > (resolution << 2) * (sen_line_num_x - 1); @@ -562,6 +559,18 @@ static int u1_init(struct hid_device *hdev, struct alps_dev *pri_data)
> > (resolution << 2) * (sen_line_num_y - 1);
> > pri_data->y_min = 1;
> >
> > + if (pri_data->no_pitch) {
> > + pri_data->x_res = pitch_x;
> > + pri_data->y_res = pitch_y;
> > + } else {
> > + pri_data->x_res =
> > + (pri_data->x_max - 1) /
> > + ((pitch_x * (sen_line_num_x - 1)) / 10);
> > + pri_data->y_res =
> > + (pri_data->y_max - 1) /
> > + ((pitch_y * (sen_line_num_y - 1)) / 10);
> > + }
> > +
> > ret = u1_read_write_register(hdev, ADDRESS_U1_PAD_BTN,
> > &tmp, 0, true);
> > if (ret < 0) {
> > @@ -622,7 +631,7 @@ static int T4_init(struct hid_device *hdev, struct alps_dev *pri_data)
> > pri_data->x_min = T4_COUNT_PER_ELECTRODE;
> > pri_data->y_max = sen_line_num_y * T4_COUNT_PER_ELECTRODE;
> > pri_data->y_min = T4_COUNT_PER_ELECTRODE;
> > - pri_data->x_active_len_mm = pri_data->y_active_len_mm = 0;
> > + pri_data->x_res = pri_data->y_res = 0;
> > pri_data->btn_cnt = 1;
> >
> > ret = t4_read_write_register(hdev, PRM_SYS_CONFIG_1, &tmp, 0, true); @@ -675,7 +684,7 @@ static int alps_input_configured(struct hid_device *hdev, struct hid_input *hi)
> > struct alps_dev *data = hid_get_drvdata(hdev);
> > struct input_dev *input = hi->input, *input2;
> > int ret;
> > - int res_x, res_y, i;
> > + int i;
> >
> > data->input = input;
> >
> > @@ -706,12 +715,9 @@ static int alps_input_configured(struct hid_device *hdev, struct hid_input *hi)
> > input_set_abs_params(input, ABS_MT_POSITION_Y,
> > data->y_min,
> > data->y_max, 0, 0);
> >
> > - if (data->x_active_len_mm && data->y_active_len_mm) {
> > - res_x = (data->x_max - 1) / data->x_active_len_mm;
> > - res_y = (data->y_max - 1) / data->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);
> > + if (data->x_res && data->y_res) {
> > + input_abs_set_res(input, ABS_MT_POSITION_X, data->x_res);
> > + input_abs_set_res(input, ABS_MT_POSITION_Y,
> > + data->y_res);
> > }
> >
> > input_set_abs_params(input, ABS_MT_PRESSURE, 0, 64, 0, 0); @@ -802,8 +808,11 @@ static int alps_probe(struct hid_device *hdev, const struct hid_device_id *id)
> > break;
> > case HID_DEVICE_ID_ALPS_U1_DUAL:
> > case HID_DEVICE_ID_ALPS_U1:
> > + data->dev_type = U1;
> > + break;
> > case HID_DEVICE_ID_ALPS_1657:
> > data->dev_type = U1;
> > + data->no_pitch = 1;
> > break;
> > default:
> > data->dev_type = UNKNOWN;
> > --
> > 2.26.0
Hi Jiri,
It's my pleasure.
The latest modification is suitable now.
Best Regards,
----------------------------------------------
Jason Cao(曹晓建)
-----Original Message-----
From: Jiri Kosina <[email protected]>
Sent: Wednesday, April 15, 2020 8:55 PM
To: 曹 曉建 Xiaojian Cao <[email protected]>
Cc: Artem Borisov <[email protected]>; 太田 真喜 Masaki Ota <[email protected]>; Benjamin Tissoires <[email protected]>; Henrik Rydberg <[email protected]>; [email protected]; [email protected]; 野村 哲哉 Tetsuya Nomura <[email protected]>; Vadim Klishko <[email protected]>; #ALCHT_ML_POD_CN <[email protected]>
Subject: RE: [PATCH 2/2] HID: alps: Refactor axis resolution logic
On Wed, 15 Apr 2020, Xiaojian Cao wrote:
> Thanks for your checking, my feedbacks are as below:
>
> 1.It is about the coding style that we should not use HWID in the
> string "HID_DEVICE_ID_ALPS_1657", there are a large number of HWIDs
> using this touchpad. We should use the device type information in this
> string, such as "U1_UNICORN_LEGACY".
Ok, thanks for the feedback. Based on it, I am queuing the patch below.
HID_DEVICE_ID_ALPS_1657 PID is too specific, as there are many other ALPS hardware IDs using this particular touchpad.
Rename the identifier to HID_DEVICE_ID_ALPS_U1_UNICORN_LEGACY in order to describe reality better.
Fixes: 640e403b1fd24 ("HID: alps: Add AUI1657 device ID")
Reported-by: Xiaojian Cao <[email protected]>
Signed-off-by: Jiri Kosina <[email protected]>
---
drivers/hid/hid-alps.c | 2 +-
drivers/hid/hid-ids.h | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/hid/hid-alps.c b/drivers/hid/hid-alps.c index c2a2bd528890..b2ad319a74b9 100644
--- a/drivers/hid/hid-alps.c
+++ b/drivers/hid/hid-alps.c
@@ -802,7 +802,7 @@ static int alps_probe(struct hid_device *hdev, const struct hid_device_id *id)
break;
case HID_DEVICE_ID_ALPS_U1_DUAL:
case HID_DEVICE_ID_ALPS_U1:
- case HID_DEVICE_ID_ALPS_1657:
+ case HID_DEVICE_ID_ALPS_U1_UNICORN_LEGACY:
data->dev_type = U1;
break;
default:
diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h index e3e2fa6733fb..6eb25b9e8575 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -79,9 +79,9 @@
#define HID_DEVICE_ID_ALPS_U1_DUAL_PTP 0x121F
#define HID_DEVICE_ID_ALPS_U1_DUAL_3BTN_PTP 0x1220
#define HID_DEVICE_ID_ALPS_U1 0x1215
+#define HID_DEVICE_ID_ALPS_U1_UNICORN_LEGACY 0x121E
#define HID_DEVICE_ID_ALPS_T4_BTNLESS 0x120C
#define HID_DEVICE_ID_ALPS_1222 0x1222
-#define HID_DEVICE_ID_ALPS_1657 0x121E
#define USB_VENDOR_ID_AMI 0x046b
#define USB_DEVICE_ID_AMI_VIRT_KEYBOARD_AND_MOUSE 0xff10
--
Jiri Kosina
SUSE Labs
Hi.
Right now I'm employed as a mobile OS engineer, if that matters.