RE: [PATCH] Input: elantech - support new touchpad IC and extend elan's touchapd command Origianl ic-body field is not sufficient for upcoming IC, Elan ps/2 driver extend the fomation to support future IC. Signed-off-by: KT Liao <kt.liao@emc.com.tw>

Hi Ulrik,
Thanks your review, I add comment in below.

-----Original Message-----
From: ulrik.debie-os@e2big.org [mailto:ulrik.debie-os@e2big.org] 
Sent: Friday, August 04, 2017 4:52 AM
To: KT Liao
Cc: linux-kernel@vger.kernel.org; linux-input@vger.kernel.org;
dmitry.torokhov@gmail.com; phoenix@emc.com.tw
Subject: Re: [PATCH] Input: elantech - support new touchpad IC and extend
elan's touchapd command Origianl ic-body field is not sufficient for
upcoming IC, Elan ps/2 driver extend the fomation to support future IC.
Signed-off-by: KT Liao <redacted>

Hi,

Something went wrong with your subject line, it also includes the further
commit message lines and the signed off line ..

Origianl -> Original
fomation .. Do you mean information ?
[KT]:Fix typo and subject line in next patch
On Wed, Aug 02, 2017 at 07:11:41PM +0800, KT Liao wrote:

unsigned char *param)

Drop the word also:
[KT] : OK

 /* hw_version 0x0F does not need to check rate */

Isn't the line too long for this one ?

Remove this tab on a line alone you added below ..
[KT]:OK

I would propose to place the lines below (up to the end of function
elantech_set_properties) in elantech_init instead of
elantech_set_properties. 
[KT]: OK, it's better to move my part to elantech_init and skip psmouse
parameter.

elantech_data *etd)

I really appreciate the message that is given in the comment. 
Why would you store the lowest byte of fw_version once more as an int .. and
use it only once at exactly the next line ..

Alternatives I see are: 
1) Merge the two lines (so there is no info_pattern anymore)
2) Use a local variable info_pattern to store it intermediately
3) Have some macro that basically takes the lowest byte (I can't immediately
find a good example for this one)
[KT] I will use a local UCHAR info_pattern for it .

+		if (etd->send_cmd(psmouse, ETP_ICBODY_QUERY, etd->icbody)) {
+			psmouse_err(psmouse, "failed to query icbody

data\n");

%02x\n",

+				etd->icbody[0], etd->icbody[1],

etd->icbody[2]);

+		etd->fw_version	&= 0xFFFF00;
+		etd->fw_version |= etd->icbody[2];

Brr, this throws away information. Is icbody2 really meant to replace bottom
byte of fw_version ? I see no benefit of doing this ! I would propose to
drop the above 2 lines that alter fw_version.
[KT] icbody2 will be the real fw version in upcoming HW, it's only useful
when we really need to check it.

From vendor's view, original etd->fw_version is not fw-versoin. It contain

many information in it. 

I agree with your point that some information is not useful now.
How about that if I only modify below part this time to support new IC, I
think it's simple and won't cause confuse.

-		case 6 ... 14:
+		case 6 ... 15:

And add more information reading once we really need it in future patch.
Please let me know you thought. thanks

When those lines are moved to elantech_init, you don't need to add the
psmouse paramater.

 		psmouse_err(psmouse, "unknown hardware version,

aborting...\n");

So I would propose to remove this one.

 	struct finger_pos mt[ETP_MAX_FINGERS];
 	unsigned char parity[256];
 	int (*send_cmd)(struct psmouse *psmouse, unsigned char c, unsigned 
char *param);
--
2.7.4

Kind regards,
Ulrik