Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Fri, 10 Apr 2020 00:21:12 +0200 (CEST)
From:   Jiri Kosina <jikos@kernel.org>
To:     Artem Borisov <dedsa2002@gmail.com>
cc:     Benjamin Tissoires <benjamin.tissoires@redhat.com>,
        Henrik Rydberg <rydberg@bitmath.org>,
        linux-input@vger.kernel.org, linux-kernel@vger.kernel.org,
        Masaki Ota <masaki.ota@jp.alps.com>
Subject: Re: [PATCH 2/2] HID: alps: Refactor axis resolution logic
In-Reply-To: <20200405235517.18203-2-dedsa2002@gmail.com>
Message-ID: <nycvar.YFH.7.76.2004100019450.19713@cbobk.fhfr.pm>
References: <20200405235517.18203-1-dedsa2002@gmail.com> <20200405235517.18203-2-dedsa2002@gmail.com>
User-Agent: Alpine 2.21 (LSU 202 2017-01-01)
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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 <dedsa2002@gmail.com>
> ---
>  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