linux-kernel - Re: [PATCH v3 2/4] input: Cypress PS/2 Trackpad psmouse driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1354674169.11174.87.camel@fourier>
Date:	Tue, 04 Dec 2012 18:22:49 -0800
From:	Kamal Mostafa <kamal@...onical.com>
To:	Henrik Rydberg <rydberg@...omail.se>,
	Dmitry Torokhov <dmitry.torokhov@...il.com>,
	Dudley Du <dudl@...ress.com>
Cc:	linux-input@...r.kernel.org, linux-kernel@...r.kernel.org,
	David Solda <dso@...ress.com>,
	Troy Abercrombia <ta@...ress.com>,
	Kyle Fazzari <git@...tus.e4ward.com>,
	Mario Limonciello <mario_limonciello@...l.com>,
	Tim Gardner <tim.gardner@...onical.com>,
	Herton Krzesinski <herton.krzesinski@...onical.com>
Subject: Re: [PATCH v3 2/4] input: Cypress PS/2 Trackpad psmouse driver

Hi Henrik-

Thanks again for your review.  The forthcoming PATCH v4 includes the
majority of your change requests, except where noted below (and
considering my email "Subject: SEMI_MT vs. simulated mt")...


On Mon, 2012-12-03 at 08:45 +0100, Henrik Rydberg wrote:

> > +		/*
> > +		 * send extension command 0xE8 or 0xF3,
> > +		 * if send extension command failed,
> > +		 * try to send recovery command to make
> > +		 * trackpad device return to ready wait command state.
> > +		 * It alwasy success based on this recovery commands.
> 
> -EPARSE

I don't understand the meaning of that comment either.  Perhaps the
original author Dudley Du can explain it.


> > +static int cypress_read_vital_statistics(struct psmouse *psmouse)
> 
> Why do you call this statistics?

Perhaps "cypress_read_tp_metrics" would be a better name.  Any thoughts
on this, Dudley?


> > +/*
> > + * reset trackpad device to standard relative mode.
> > + * This is also the defalut mode when trackpad powered on.
> > + */
> > +static void cypress_reset(struct psmouse *psmouse)
> > +{
> > +	struct cytp_data *cytp = psmouse->private;
> > +
> > +	psmouse_reset(psmouse);
> > +
> > +	CYTP_SET_MODE_BIT(CYTP_BIT_STANDARD_REL);
> > +	CYTP_SET_PACKET_SIZE(3);
> > +
> > +	cytp->prev_contact_cnt = 0;
> > +}
> 
> I suppose it is, but is it necessary to reset to default mode?

I left this unchanged, as I think you and Dmitry agreed.


> > +
> > +static int cypress_set_input_params(struct input_dev *input,
> > +				    struct cytp_data *cytp)
> > +{
> > +	int ret;
> > +
> > +	if (cytp->mode & CYTP_BIT_ABS_MASK) {
> 
> It seems the device will always operate in this mode, so please simplify.

I #ifdef'd out the unused relative-mode code, as opposed to deleting it.
Will that be acceptable?


> > +		/* Remove HSCROLL bit */
> > +		if (report_data->contact_cnt == 4)
> > +			header_byte &= ~(ABS_HSCROLL_BIT);
> 
> Why conditionally?

Dudley, can you answer this?  I left this (and all the scroll code)
unchanged.


> > +#if 0
> > +	/* FIXME: cypress_reconnect() never works...
> > +	 * just let psmouse re-init() us for now.
> > +	 */
> > +	psmouse->reconnect = cypress_reconnect;
> > +#endif
> 
> This needs to be removed or fixed, of course.

Per Dudley, this does work in his disconnect/reconnect scenario, so I
re-enabled it.  We will continue to investigate why it fails
(harmlessly) in my suspend/resume scenario.


> > +	int tp_max_abs_x;  /* Max X absolution units can be reported. */
> > +	int tp_max_abs_y;  /* Max Y absolution units can be reported. */
> 
> I do not think we are seeking absolution here. :-)

Ha!  Speak for yourself, Henrik!  ;-)  I'm starting to feel like maybe I
should be seeking absolution here!


 -Kamal


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/