Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Mattijs Korpershoek <mkorpershoek@baylibre.com>
To:     Eirin Nya <nyanpasu256@gmail.com>, linux-input@vger.kernel.org
Cc:     linux-kernel@vger.kernel.org, Eirin Nya <nyanpasu256@gmail.com>
Subject: Re: [PATCH V2 1/3] Input: elantech - Remove redundant field
 elantech_data::y_max
In-Reply-To: <20221014111533.908-2-nyanpasu256@gmail.com>
References: <20221014111533.908-1-nyanpasu256@gmail.com>
 <20221014111533.908-2-nyanpasu256@gmail.com>
Date:   Fri, 14 Oct 2022 15:10:12 +0200
Message-ID: <87sfjqzh3f.fsf@baylibre.com>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

On Fri, Oct 14, 2022 at 04:15, Eirin Nya <nyanpasu256@gmail.com> wrote:

> elantech_data::y_max is copied from elantech_device_info::y_max, and
> neither is mutated after initialization. So remove the redundant
> variable to prevent future bugs when updating y_max.
>
> Signed-off-by: Eirin Nya <nyanpasu256@gmail.com>

Hi Eirin,

Thank you for v2. I know you're new to kernel development so here is
some more "process feedback".

I've already reviewed this in v1:
https://lore.kernel.org/all/87ilkv7ogc.fsf@mkorpershoek-xps-13-9370.home/

After getting a "Reviewed-by" reply on one of the patches, it is
customary to add that in the commit message footer, if the patch is
unchanged. This encourages reviewers and gives them some credit for
their review :)

This is documented at:
https://www.kernel.org/doc/html/latest/process/submitting-patches.html#reviewer-s-statement-of-oversight

To quote the doc:
> Both Tested-by and Reviewed-by tags, once received on mailing list from
> tester or reviewer, should be added by author to the applicable patches
> when sending next versions.

So please, if you have to send a v3 at some point, please add:

Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>

Note that it's not needed to send a v3 *JUST* to include the trailers.
The maintainer will pick them up if he decides to merge this.

> ---
>  drivers/input/mouse/elantech.c | 17 ++++++++---------
>  drivers/input/mouse/elantech.h |  1 -
>  2 files changed, 8 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/input/mouse/elantech.c b/drivers/input/mouse/elantech.c
> index ece97f8c6a..79e31611fc 100644
> --- a/drivers/input/mouse/elantech.c
> +++ b/drivers/input/mouse/elantech.c
> @@ -360,7 +360,7 @@ static void elantech_report_absolute_v1(struct psmouse *psmouse)
>  		input_report_abs(dev, ABS_X,
>  			((packet[1] & 0x0c) << 6) | packet[2]);
>  		input_report_abs(dev, ABS_Y,
> -			etd->y_max - (((packet[1] & 0x03) << 8) | packet[3]));
> +			etd->info.y_max - (((packet[1] & 0x03) << 8) | packet[3]));
>  	}
>  
>  	input_report_key(dev, BTN_TOOL_FINGER, fingers == 1);
> @@ -435,7 +435,7 @@ static void elantech_report_absolute_v2(struct psmouse *psmouse)
>  		 * byte 4:  .   .   .   .  y11 y10 y9  y8
>  		 * byte 5: y7  y6  y5  y4  y3  y2  y1  y0
>  		 */
> -		y1 = etd->y_max - (((packet[4] & 0x0f) << 8) | packet[5]);
> +		y1 = etd->info.y_max - (((packet[4] & 0x0f) << 8) | packet[5]);
>  
>  		pres = (packet[1] & 0xf0) | ((packet[4] & 0xf0) >> 4);
>  		width = ((packet[0] & 0x30) >> 2) | ((packet[3] & 0x30) >> 4);
> @@ -450,7 +450,7 @@ static void elantech_report_absolute_v2(struct psmouse *psmouse)
>  		 */
>  		x1 = (((packet[0] & 0x10) << 4) | packet[1]) << 2;
>  		/* byte 2: ay7 ay6 ay5 ay4 ay3 ay2 ay1 ay0 */
> -		y1 = etd->y_max -
> +		y1 = etd->info.y_max -
>  			((((packet[0] & 0x20) << 3) | packet[2]) << 2);
>  		/*
>  		 * byte 3:  .   .  by8 bx8  .   .   .   .
> @@ -458,7 +458,7 @@ static void elantech_report_absolute_v2(struct psmouse *psmouse)
>  		 */
>  		x2 = (((packet[3] & 0x10) << 4) | packet[4]) << 2;
>  		/* byte 5: by7 by8 by5 by4 by3 by2 by1 by0 */
> -		y2 = etd->y_max -
> +		y2 = etd->info.y_max -
>  			((((packet[3] & 0x20) << 3) | packet[5]) << 2);
>  
>  		/* Unknown so just report sensible values */
> @@ -579,7 +579,7 @@ static void elantech_report_absolute_v3(struct psmouse *psmouse,
>  		 * byte 4:  .   .   .   .  y11 y10 y9  y8
>  		 * byte 5: y7  y6  y5  y4  y3  y2  y1  y0
>  		 */
> -		y1 = etd->y_max - (((packet[4] & 0x0f) << 8) | packet[5]);
> +		y1 = etd->info.y_max - (((packet[4] & 0x0f) << 8) | packet[5]);
>  		break;
>  
>  	case 2:
> @@ -593,7 +593,7 @@ static void elantech_report_absolute_v3(struct psmouse *psmouse,
>  			 * byte 4:   .    .    .    .  ay11 ay10 ay9  ay8
>  			 * byte 5: ay7  ay6  ay5  ay4  ay3  ay2  ay1  ay0
>  			 */
> -			etd->mt[0].y = etd->y_max -
> +			etd->mt[0].y = etd->info.y_max -
>  				(((packet[4] & 0x0f) << 8) | packet[5]);
>  			/*
>  			 * wait for next packet
> @@ -605,7 +605,7 @@ static void elantech_report_absolute_v3(struct psmouse *psmouse,
>  		x1 = etd->mt[0].x;
>  		y1 = etd->mt[0].y;
>  		x2 = ((packet[1] & 0x0f) << 8) | packet[2];
> -		y2 = etd->y_max - (((packet[4] & 0x0f) << 8) | packet[5]);
> +		y2 = etd->info.y_max - (((packet[4] & 0x0f) << 8) | packet[5]);
>  		break;
>  	}
>  
> @@ -681,7 +681,7 @@ static void process_packet_head_v4(struct psmouse *psmouse)
>  		return;
>  
>  	etd->mt[id].x = ((packet[1] & 0x0f) << 8) | packet[2];
> -	etd->mt[id].y = etd->y_max - (((packet[4] & 0x0f) << 8) | packet[5]);
> +	etd->mt[id].y = etd->info.y_max - (((packet[4] & 0x0f) << 8) | packet[5]);
>  	pres = (packet[1] & 0xf0) | ((packet[4] & 0xf0) >> 4);
>  	traces = (packet[0] & 0xf0) >> 4;
>  
> @@ -1253,7 +1253,6 @@ static int elantech_set_input_params(struct psmouse *psmouse)
>  		input_abs_set_res(dev, ABS_MT_POSITION_Y, info->y_res);
>  	}
>  
> -	etd->y_max = y_max;
>  	etd->width = width;
>  
>  	return 0;
> diff --git a/drivers/input/mouse/elantech.h b/drivers/input/mouse/elantech.h
> index 571e6ca11d..1ec004f72d 100644
> --- a/drivers/input/mouse/elantech.h
> +++ b/drivers/input/mouse/elantech.h
> @@ -180,7 +180,6 @@ struct elantech_data {
>  	unsigned char reg_25;
>  	unsigned char reg_26;
>  	unsigned int single_finger_reports;
> -	unsigned int y_max;
>  	unsigned int width;
>  	struct finger_pos mt[ETP_MAX_FINGERS];
>  	unsigned char parity[256];
> -- 
> 2.38.0