On Mon, Apr 23, 2018 at 08:18:48AM -0700, Jason Gerecke wrote:
> On Sun, Apr 22, 2018 at 4:06 PM, Peter Hutterer
> <peter.hutte...@who-t.net> wrote:
> > On Fri, Apr 20, 2018 at 02:33:21PM -0700, Jason Gerecke wrote:
> >> The 2.6.36 kernel removes several members from `struct input_dev` (e.g.
> >> abs, absres) and replaces them with a structs and accessor functions.
> >> To allow the input-wacom code to compile under both old and new kernels,
> >> commits ca9786f and the mailinglist version of ab2ea683fb conditionally
> >> defined their own implementation of the accessor function for older
> >> kernels.
> >>
> >> It was noticed, however, that this did not compile correctly on RHEL 6
> >> systems. It seems that the accessor API introduced in 2.6.36 is provided
> >> in their customized "2.6.32" kernel. This results in a redefinition error
> >> that halts compilation. To work around this, commit 58d8320541 removed
> >> the condtional and renamed our implementation of the accessor. Commit
> >> ab2ea683fb was also similarly modified from its mailinglist version prior
> >> to being committed. This change prevented the redefinition on RHEL 6 and
> >> also worked fine for pre-2.6.36 kernels. The change ended up breaking
> >> compilation under stock 2.6.36 kernels since the members used by the
> >> renamed function were removed.
> >>
> >> To ensure the code compiles in all cases, we need to be a little more
> >> clever. We make use of the recently-added "WACOM_LINUX_TRY_COMPILE"
> >> configure macro to see if the kernel provides the accessor API or not.
> >> If it does, we make use of it; of not, we access the members directly.
> >>
> >> Fixes: 58d8320541 ("2.6.30: define wacom_input_abs_get_val")
> >> Fixes: ab2ea683fb ("2.6.32: Backport resolution support to 2.6.32")
> >> Signed-off-by: Jason Gerecke <jason.gere...@wacom.com>
> >> ---
> >> 2.6.32/wacom_wac.c | 8 ++++++++
> >> configure.ac | 15 +++++++++++++++
> >> 2 files changed, 23 insertions(+)
> >>
> >> diff --git a/2.6.32/wacom_wac.c b/2.6.32/wacom_wac.c
> >> index 3ad7aae..11c7513 100644
> >> --- a/2.6.32/wacom_wac.c
> >> +++ b/2.6.32/wacom_wac.c
> >> @@ -924,7 +924,11 @@ static int wacom_intuos_irq(struct wacom_wac *wacom)
> >>
> >> static int wacom_input_abs_get_val(struct input_dev *input, unsigned int
> >> axis)
> >> {
> >> +#ifndef WACOM_ABSACCESSOR_36
> >> return input->abs[axis];
> >> +#else
> >> + return input_abs_get_val(input, axis);
> >> +#endif
> >
> > I've been staring at this for a bit and quickly skimmed through the commits
> > metioned above but I'm still unsure why you couldn't just leave
> > input_abs_get_val() here and make that function's existence (or internal
> > implementation) dependent on the WACOM_ABSACCESSOR_36 define. Would save you
> > a buch of ifdefs.
> >
>
> It would definitely be possible to do that, but I don't really see an
> ifdef savings. I suppose if we shuffled the two functions to be next
> to each other we could get away with one ifdef instead of two, but (at
> least for the moment) I'm perfectly happy leaving things where they
> are and named with the 'wacom_' prefix to make it clear at the point
> of use that we're not necessarily dealing with an upstream function.
the two benefits you get: the #ifdef in only one location, where the
accessor functions are defined. and a better ability to cherry-pick because
you should have less conflicts in those.
Cheers,
Peter
> >> }
> >>
> >> static void wacom_multitouch_generic_finger(struct wacom_wac *wacom,
> >> @@ -2080,7 +2084,11 @@ void wacom_setup_device_quirks(struct wacom *wacom)
> >>
> >> static inline void wacom_input_abs_set_res(struct input_dev *dev,
> >> unsigned int axis, int val)
> >> {
> >> +#ifndef WACOM_ABSACCESSOR_36
> >
> > personal nit: I prefer something like HAVE_INPUT_ABS_SET_RES because it
> > makes the whole thing more obvious when reading. WACOM_ABSACCESSOR_36 is a
> > bit harder to understand (naming it WACOM_ABSACESSOR_v2_36 might make it
> > easier, but still).
> >
>
> I was copying the existing style introduced by Benjamin
> ('WACOM_POWERSUPPLY_41'), but now that you mention it, I also prefer
> the more boolean-like reading of your suggestion. Maybe we could use a
> follow-up patch to fix both of these :D
>
> >> dev->absres[axis] = val;
> >> +#else
> >> + input_abs_set_res(dev, axis, val);
> >> +#endif
> >> }
> >>
> >> static void wacom_abs_set_axis(struct input_dev *input_dev,
> >> diff --git a/configure.ac b/configure.ac
> >> index 1cb4394..a14a569 100644
> >> --- a/configure.ac
> >> +++ b/configure.ac
> >> @@ -194,6 +194,21 @@ struct power_supply_desc test;
> >> AC_MSG_RESULT([pre-v4.1])
> >> ])
> >>
> >> +dnl RedHat entreprise Linux 6.x backports abs accessor functions from
> >> 2.6.36
> >
> > typo, also officially it's "Enterprise" (capital E)
> >
>
> Good catch >.<
>
> >> +AC_MSG_CHECKING(abs accessor version)
> >> +WACOM_LINUX_TRY_COMPILE([
> >> +#include <linux/input.h>
> >> +],[
> >> +struct input_dev test;
> >> +input_abs_get_res(&test, 0);
> >
> > fwiw, just passing NULL should work here, makes the check just one line.
> >
> > The input-wacom repo is decidedly yours, so this is all very much your
> > decision :)
> >
> > Cheers,
> > Peter
> >
>
> Thanks as always for the good feedback! I'll have to unfortunately put
> some of your suggestions/questions on the back burner (srcdir !=
> builddir, HAVE_INPUT_ABS_SET_RES) due to other time commitments. Maybe
> Ping or Aaron can pick them up?
>
> Jason
> ---
> Now instead of four in the eights place /
> you’ve got three, ‘Cause you added one /
> (That is to say, eight) to the two, /
> But you can’t take seven from three, /
> So you look at the sixty-fours....
>
> >> +],[
> >> + HAVE_ABSACCESSOR_36=yes
> >> + AC_MSG_RESULT([v2.6.36+])
> >> + AC_DEFINE([WACOM_ABSACCESSOR_36], [], [kernel uses abs accessor
> >> macros from v2.6.36+])
> >> +],[
> >> + HAVE_ABSACCESSOR_36=no
> >> + AC_MSG_RESULT([pre-v2.6.36])
> >> +])
> >>
> >> dnl Check which version of the driver we should compile
> >> AC_DEFUN([WCM_EXPLODE], [$(echo "$1" | awk '{split($[0],x,"[[^0-9]]");
> >> printf("%03d%03d%03d\n",x[[1]],x[[2]],x[[3]]);}')])
> >> --
> >> 2.17.0
> >>
> >>
> >> ------------------------------------------------------------------------------
> >> Check out the vibrant tech community on one of the world's most
> >> engaging tech sites, Slashdot.org! http://sdm.link/slashdot
> >> _______________________________________________
> >> Linuxwacom-devel mailing list
> >> Linuxwacom-devel@lists.sourceforge.net
> >> https://lists.sourceforge.net/lists/listinfo/linuxwacom-devel
> >>
>
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Linuxwacom-devel mailing list
Linuxwacom-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxwacom-devel
