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.
>> }
>>
>> 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
