Re: [PATCH v4 1/3] Input: ALPS - Fix TrackStick support for SS5 hardware

[Paul Donohue <linux-kernel@PaulSD.com>]

On Wed, Nov 09, 2016 at 01:12:31PM +0100, Pali Rohár wrote:
> On Tuesday 08 November 2016 10:14:30 Paul Donohue wrote:
> > --- a/drivers/input/mouse/alps.c
> > +++ b/drivers/input/mouse/alps.c
> > @@ -1346,6 +1346,18 @@ static void alps_process_packet_ss4_v2(struct psmouse *psmouse)
> >  
> >  	priv->multi_packet = 0;
> >  
> > +	/* Report trackstick */
> > +	if (alps_get_pkt_id_ss4_v2(packet) == SS4_PACKET_ID_STICK) {
> 
> I would propose to show warning when trackstick packet is received on
> device marked as non-trackstick. It would help to debug possible
> problems in future...
> 
> if (!(priv->flags & ALPS_DUALPOINT))
>         psmouse_warn(psmouse, "Rejected trackstick packet from non DualPoint device");
For patch 1/3, the original alps_decode_ss4_v2() prints this warning,
so I don't think alps_process_packet_ss4_v2() needs to print it again.
Since this patch is a bug fix, I was trying to avoid any unnecessary
changes, and change just enough to fix the actual bug, so it is clear to
future readers what the bug was and how it was fixed.

In patch 2/3, I moved the warning from alps_decode_ss4_v2() to
alps_process_packet_ss4_v2(), since I agree it probably makes more sense
to print the warning in alps_process_packet_ss4_v2().  I also
removed the if statement from alps_decode_ss4_v2() entirely because
alps_decode_ss4_v2() decodes the trackstick buttons even in cases where
the trackstick packet is going to be rejected, and I thought it was a
bit confusing that alps_decode_ss4_v2() does not also decode the
trackstick movements even in cases where the trackstick packet is going
to be rejected (either they should both be decoded, or neither should be
decoded, and decoding both rather than skipping both requires less
logic).