Re: [PATCH 2/2 v3] elantech: Report multitouch with proper ABS_MT messages

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

On Tue, May 11, 2010 at 01:58:43AM +0200, Henrik Rydberg wrote:
> Éric Piel wrote:
> > Op 10-05-10 22:53, Henrik Rydberg schreef:
> >> Hi again Éric,
> >>
> >> thanks for your changes, the first patch looks better now. However, both patches
> >> still have style problems. Also, the second patch now makes unrelated changes to
> >> the ABS_X etc, so it is difficult to even know what was tested, and whether the
> >> first patch really works as claimed.
> > What are the style problems of the first patch? I tried to fix all of them.
> 
> WARNING: line over 80 characters
> #212: FILE: drivers/input/mouse/elantech.c:483:
> +		input_set_abs_params(dev, ABS_TOOL_WIDTH, ETP_WMIN_V2, ETP_WMAX_V2, 0, 0);
> 
> WARNING: line over 80 characters
> #119: FILE: drivers/input/mouse/elantech.c:498:
> +		input_set_abs_params(dev, ABS_MT_POSITION_X, ETP_XMIN_V2, ETP_XMAX_V2, 0, 0);
> 
> WARNING: line over 80 characters
> #120: FILE: drivers/input/mouse/elantech.c:499:
> +		input_set_abs_params(dev, ABS_MT_POSITION_Y, ETP_YMIN_V2, ETP_YMAX_V2, 0, 0);
> 

Do not worry about the 80 char limit that much, especially if it
produces less readable code due to wierd line splits. The code above
could be broken cleanly, but it's not a big deal.

The rest of the checkpatch warnirngs (if any) I prefer to have
addressed though.

> > 
> > Concerning the second patch, it's necessary to change the lines with
> > ABS_X, because otherwise the computation would be duplicated for the MT
> > part and it would make everything look more complicated. Honestly, I
> > think this change is still very limited and can easily be reviewed for
> > not changing the semantic.
> 
> Yes, you are right about x1 and y1. The patch patch touches the width variable
> for no apparent reason, and I stopped reading there.
> 
> If this process seems trivial and tedious to you, consider how it must appear to
> those who deal with patches every day. Most people will not even look at a patch
> with style problems, so the real review _starts_ when those issues are fixed.
> 
> Henrik

-- 
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html