* Re: [PATCH 2/4] Input: introduce ABS_MAX2/CNT2 and friends
From: Peter Hutterer @ 2013-12-18 23:40 UTC (permalink / raw)
To: David Herrmann
Cc: linux-input, Dmitry Torokhov, Jiri Kosina, Benjamin Tissoires,
Antonio Ospite, linux-kernel, input-tools
In-Reply-To: <1387295334-1744-3-git-send-email-dh.herrmann@gmail.com>
On Tue, Dec 17, 2013 at 04:48:52PM +0100, David Herrmann wrote:
> As we painfully noticed during the 3.12 merge-window our
> EVIOCGABS/EVIOCSABS API is limited to ABS_MAX<=0x3f. We tried several
> hacks to work around it but if we ever decide to increase ABS_MAX, the
> EVIOCSABS ioctl ABI might overflow into the next byte causing horrible
> misinterpretations in the kernel that we cannot catch.
>
> Therefore, we decided to go with ABS_MAX2/CNT2 and introduce two new
> ioctls to get/set abs-params. They no longer encode the ABS code in the
> ioctl number and thus allow up to 4 billion ABS codes.
>
> The new API also allows to query multiple ABS values with one call. To
> allow EVIOCSABS2(code = 0, cnt = ABS_CNT2) we need to silently ignore
> writes to ABS_MT_SLOT. Furthermore, for better compatibility with
> newer user-space, we ignore writes to unknown codes. Hence, if we ever
> increase ABS_MAX2, new user-space will work with code=0,cnt=ABS_CNT2 just
> fine even on old kernels.
>
> Note that we also need to increase EV_VERSION so user-space can reliably
> know whether ABS2 is supported. Unfortunately, we return EINVAL instead of
> ENOSYS for unknown evdev ioctls so it's nearly impossible to catch
> reliably without EVIOCGVERSION.
>
> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
> ---
> drivers/hid/hid-debug.c | 2 +-
> drivers/hid/hid-input.c | 2 +-
> drivers/input/evdev.c | 95 +++++++++++++++++++++++++++++++-
> drivers/input/input.c | 14 ++---
> drivers/input/keyboard/goldfish_events.c | 6 +-
> drivers/input/keyboard/hil_kbd.c | 2 +-
> drivers/input/misc/uinput.c | 6 +-
> include/linux/hid.h | 2 +-
> include/linux/input.h | 6 +-
> include/uapi/linux/input.h | 42 +++++++++++++-
> include/uapi/linux/uinput.h | 2 +-
> 11 files changed, 155 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/hid/hid-debug.c b/drivers/hid/hid-debug.c
> index 8453214..d32fa30 100644
> --- a/drivers/hid/hid-debug.c
> +++ b/drivers/hid/hid-debug.c
> @@ -862,7 +862,7 @@ static const char *relatives[REL_MAX + 1] = {
> [REL_WHEEL] = "Wheel", [REL_MISC] = "Misc",
> };
>
> -static const char *absolutes[ABS_CNT] = {
> +static const char *absolutes[ABS_CNT2] = {
> [ABS_X] = "X", [ABS_Y] = "Y",
> [ABS_Z] = "Z", [ABS_RX] = "Rx",
> [ABS_RY] = "Ry", [ABS_RZ] = "Rz",
> diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
> index d97f232..a02721c 100644
> --- a/drivers/hid/hid-input.c
> +++ b/drivers/hid/hid-input.c
> @@ -1300,7 +1300,7 @@ static bool hidinput_has_been_populated(struct hid_input *hidinput)
> for (i = 0; i < BITS_TO_LONGS(REL_CNT); i++)
> r |= hidinput->input->relbit[i];
>
> - for (i = 0; i < BITS_TO_LONGS(ABS_CNT); i++)
> + for (i = 0; i < BITS_TO_LONGS(ABS_CNT2); i++)
> r |= hidinput->input->absbit[i];
>
> for (i = 0; i < BITS_TO_LONGS(MSC_CNT); i++)
> diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
> index a06e125..32b74e5 100644
> --- a/drivers/input/evdev.c
> +++ b/drivers/input/evdev.c
> @@ -643,7 +643,7 @@ static int handle_eviocgbit(struct input_dev *dev,
> case 0: bits = dev->evbit; len = EV_MAX; break;
> case EV_KEY: bits = dev->keybit; len = KEY_MAX; break;
> case EV_REL: bits = dev->relbit; len = REL_MAX; break;
> - case EV_ABS: bits = dev->absbit; len = ABS_MAX; break;
> + case EV_ABS: bits = dev->absbit; len = ABS_MAX2; break;
> case EV_MSC: bits = dev->mscbit; len = MSC_MAX; break;
> case EV_LED: bits = dev->ledbit; len = LED_MAX; break;
> case EV_SND: bits = dev->sndbit; len = SND_MAX; break;
> @@ -671,6 +671,93 @@ static int handle_eviocgbit(struct input_dev *dev,
> }
> #undef OLD_KEY_MAX
>
> +static int evdev_handle_get_abs2(struct input_dev *dev, void __user *p)
> +{
> + u32 code, cnt, valid_cnt, i;
> + struct input_absinfo2 __user *pinfo = p;
> + struct input_absinfo abs;
> +
> + if (copy_from_user(&code, &pinfo->code, sizeof(code)))
> + return -EFAULT;
> + if (copy_from_user(&cnt, &pinfo->cnt, sizeof(cnt)))
> + return -EFAULT;
> + if (!cnt)
> + return 0;
> +
> + if (!dev->absinfo)
> + valid_cnt = 0;
> + else if (code > ABS_MAX2)
> + valid_cnt = 0;
> + else if (code + cnt <= code || code + cnt > ABS_MAX2)
> + valid_cnt = ABS_MAX2 - code + 1;
> + else
> + valid_cnt = cnt;
> +
> + for (i = 0; i < valid_cnt; ++i) {
> + /*
> + * Take event lock to ensure that we are not
> + * copying data while EVIOCSABS2 changes it.
> + * Might be inconsistent, otherwise.
> + */
> + spin_lock_irq(&dev->event_lock);
> + abs = dev->absinfo[code + i];
> + spin_unlock_irq(&dev->event_lock);
> +
> + if (copy_to_user(&pinfo->info[i], &abs, sizeof(abs)))
> + return -EFAULT;
> + }
> +
> + memset(&abs, 0, sizeof(abs));
> + for (i = valid_cnt; i < cnt; ++i)
> + if (copy_to_user(&pinfo->info[i], &abs, sizeof(abs)))
> + return -EFAULT;
> +
> + return 0;
why don't you return the number of valid copied axes to the user?
that seems better even than forcing the remainder to 0.
> +}
> +
> +static int evdev_handle_set_abs2(struct input_dev *dev, void __user *p)
> +{
> + struct input_absinfo2 __user *pinfo = p;
> + struct input_absinfo *abs;
> + u32 code, cnt, i;
> + size_t size;
> +
> + if (!dev->absinfo)
> + return 0;
> + if (copy_from_user(&code, &pinfo->code, sizeof(code)))
> + return -EFAULT;
> + if (copy_from_user(&cnt, &pinfo->cnt, sizeof(cnt)))
> + return -EFAULT;
> + if (!cnt || code > ABS_MAX2)
> + return 0;
> +
> + if (code + cnt <= code || code + cnt > ABS_MAX2)
> + cnt = ABS_MAX2 - code + 1;
> +
> + size = cnt * sizeof(*abs);
> + abs = memdup_user(pinfo->info, size);
> + if (IS_ERR(abs))
> + return PTR_ERR(abs);
> +
> + /*
> + * Take event lock to ensure that we are not
> + * changing device parameters in the middle
> + * of event.
> + */
> + spin_lock_irq(&dev->event_lock);
> + for (i = 0; i < cnt; ++i) {
> + /* silently drop ABS_MT_SLOT */
> + if (code + i == ABS_MT_SLOT)
> + continue;
> +
> + dev->absinfo[code + i] = abs[i];
> + }
> + spin_unlock_irq(&dev->event_lock);
> +
> + kfree(abs);
> + return 0;
> +}
> +
> static int evdev_handle_get_keycode(struct input_dev *dev, void __user *p)
> {
> struct input_keymap_entry ke = {
> @@ -898,6 +985,12 @@ static long evdev_do_ioctl(struct file *file, unsigned int cmd,
> client->clkid = i;
> return 0;
>
> + case EVIOCGABS2:
> + return evdev_handle_get_abs2(dev, p);
> +
> + case EVIOCSABS2:
> + return evdev_handle_set_abs2(dev, p);
> +
> case EVIOCGKEYCODE:
> return evdev_handle_get_keycode(dev, p);
>
[...]
> diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
> index 927ad9a..4660ed1 100644
> --- a/drivers/input/misc/uinput.c
> +++ b/drivers/input/misc/uinput.c
> @@ -311,7 +311,7 @@ static int uinput_validate_absbits(struct input_dev *dev)
> unsigned int cnt;
> int retval = 0;
>
> - for (cnt = 0; cnt < ABS_CNT; cnt++) {
> + for (cnt = 0; cnt < ABS_CNT2; cnt++) {
> int min, max;
> if (!test_bit(cnt, dev->absbit))
> continue;
> @@ -474,7 +474,7 @@ static int uinput_setup_device2(struct uinput_device *udev,
> return -EINVAL;
>
> /* rough check to avoid huge kernel space allocations */
> - max = ABS_CNT * sizeof(*user_dev2->abs) + sizeof(*user_dev2);
> + max = ABS_CNT2 * sizeof(*user_dev2->abs) + sizeof(*user_dev2);
hmm, if you ever up the value of ABS_CNT2 and you don't have it query-able,
userspace won't be able to create a uinput device on a kernel with a smaller
ABS_CNT2. worst of all, the only error you get is EINVAL, which is also
used for invalid axis ranges, etc.
> if (count > max)
> return -EINVAL;
>
> @@ -780,7 +780,7 @@ static long uinput_ioctl_handler(struct file *file, unsigned int cmd,
> break;
>
> case UI_SET_ABSBIT:
> - retval = uinput_set_bit(arg, absbit, ABS_MAX);
> + retval = uinput_set_bit(arg, absbit, ABS_MAX2);
> break;
>
> case UI_SET_MSCBIT:
> diff --git a/include/linux/hid.h b/include/linux/hid.h
> index 31b9d29..c21d8bb 100644
> --- a/include/linux/hid.h
> +++ b/include/linux/hid.h
> @@ -828,7 +828,7 @@ static inline void hid_map_usage(struct hid_input *hidinput,
> switch (type) {
> case EV_ABS:
> *bit = input->absbit;
> - *max = ABS_MAX;
> + *max = ABS_MAX2;
> break;
> case EV_REL:
> *bit = input->relbit;
> diff --git a/include/linux/input.h b/include/linux/input.h
> index 82ce323..c6add6f 100644
> --- a/include/linux/input.h
> +++ b/include/linux/input.h
> @@ -129,7 +129,7 @@ struct input_dev {
> unsigned long evbit[BITS_TO_LONGS(EV_CNT)];
> unsigned long keybit[BITS_TO_LONGS(KEY_CNT)];
> unsigned long relbit[BITS_TO_LONGS(REL_CNT)];
> - unsigned long absbit[BITS_TO_LONGS(ABS_CNT)];
> + unsigned long absbit[BITS_TO_LONGS(ABS_CNT2)];
> unsigned long mscbit[BITS_TO_LONGS(MSC_CNT)];
> unsigned long ledbit[BITS_TO_LONGS(LED_CNT)];
> unsigned long sndbit[BITS_TO_LONGS(SND_CNT)];
> @@ -210,8 +210,8 @@ struct input_dev {
> #error "REL_MAX and INPUT_DEVICE_ID_REL_MAX do not match"
> #endif
>
> -#if ABS_MAX != INPUT_DEVICE_ID_ABS_MAX
> -#error "ABS_MAX and INPUT_DEVICE_ID_ABS_MAX do not match"
> +#if ABS_MAX2 != INPUT_DEVICE_ID_ABS_MAX
> +#error "ABS_MAX2 and INPUT_DEVICE_ID_ABS_MAX do not match"
> #endif
>
> #if MSC_MAX != INPUT_DEVICE_ID_MSC_MAX
> diff --git a/include/uapi/linux/input.h b/include/uapi/linux/input.h
> index bd24470..1856461 100644
> --- a/include/uapi/linux/input.h
> +++ b/include/uapi/linux/input.h
> @@ -32,7 +32,7 @@ struct input_event {
> * Protocol version.
> */
>
> -#define EV_VERSION 0x010001
> +#define EV_VERSION 0x010002
A history in the comments would be nice. something like
/**
* 0x010000: original version
* 0x010001: support for long scancodes
* 0x010002: added ABS_CNT2/ABS_MAX2, EVIOCSABS2, EVIOCGABS2
*/
> /*
> * IOCTLs (0x00 - 0x7f)
> @@ -74,6 +74,30 @@ struct input_absinfo {
> };
>
> /**
> + * struct input_absinfo2 - used by EVIOC[G/S]ABS2 ioctls
please spell the two out (at least once in this paragraph), it makes it grep-able.
> + * @code: First ABS code to query
> + * @cnt: Number of ABS codes to query starting at @code
> + * @info: #@cnt absinfo structures to get/set abs parameters for all codes
> + *
> + * This structure is used by the new EVIOC[G/S]ABS2 ioctls which
> + * do the same as the old EVIOC[G/S]ABS ioctls but avoid encoding
> + * the ABS code in the ioctl number. This allows a much wider
> + * range of ABS codes. Furthermore, it allows to query multiple codes with a
> + * single call.
"new" and "old" have a tendency to become "old" and "before old" quickly,
just skip both. A simple "use of EVIOCGABS/EVIOCSABS is discouraged except on kernels
without EVIOCGABS2/EVIOCSABS2 support" is enough to signal which one is preferred.
> + *
> + * Note that this silently drops any requests to set ABS_MT_SLOT. Hence, it is
> + * allowed to call this with code=0 cnt=ABS_CNT2. Furthermore, retrieving
> + * invalid codes returns all 0, setting them does nothing. So you must check
> + * with EVIOCGBIT first if you want reliable results. This behavior is needed
> + * to allow forward compatibility to new ABS codes.
I think this needs rewording, I was quite confused reading this. How about:
"For axes not present on the device and for axes exceeding the kernel's
built-in ABS_CNT2 maximum, EVIOCGABS2 sets all values in the struct absinfo
to 0. EVIOCGSABS2 silently ignores write requests to these axes.
ABS_MT_SlOT is an immutable axis and cannot be modified by EVIOCSABS2,
the respective value is silently ignored."
also, please document the return value of the ioctl.
Cheers,
Peter
> + */
> +struct input_absinfo2 {
> + __u32 code;
> + __u32 cnt;
> + struct input_absinfo info[1];
> +};
> +
> +/**
> * struct input_keymap_entry - used by EVIOCGKEYCODE/EVIOCSKEYCODE ioctls
> * @scancode: scancode represented in machine-endian form.
> * @len: length of the scancode that resides in @scancode buffer.
> @@ -153,6 +177,8 @@ struct input_keymap_entry {
>
> #define EVIOCGRAB _IOW('E', 0x90, int) /* Grab/Release device */
> #define EVIOCREVOKE _IOW('E', 0x91, int) /* Revoke device access */
> +#define EVIOCGABS2 _IOR('E', 0x92, struct input_absinfo2) /* get abs value/limits */
> +#define EVIOCSABS2 _IOW('E', 0x93, struct input_absinfo2) /* set abs value/limits */
>
> #define EVIOCSCLOCKID _IOW('E', 0xa0, int) /* Set clockid to be used for timestamps */
>
> @@ -835,11 +861,23 @@ struct input_keymap_entry {
> #define ABS_MT_TOOL_X 0x3c /* Center X tool position */
> #define ABS_MT_TOOL_Y 0x3d /* Center Y tool position */
>
> -
> +/*
> + * ABS_MAX/CNT is limited to a maximum of 0x3f due to the design of EVIOCGABS
> + * and EVIOCSABS ioctls. Other kernel APIs like uinput also hardcoded it. Do
> + * not modify this value and instead use the extended ABS_MAX2/CNT2 API.
> + */
> #define ABS_MAX 0x3f
> #define ABS_CNT (ABS_MAX+1)
>
> /*
> + * Due to API restrictions the legacy evdev API only supports ABS values up to
> + * ABS_MAX/CNT. Use the extended *ABS2 ioctls to operate on any ABS values in
> + * between ABS_MAX and ABS_MAX2.
> + */
> +#define ABS_MAX2 0x3f
> +#define ABS_CNT2 (ABS_MAX2+1)
> +
> +/*
> * Switch events
> */
>
> diff --git a/include/uapi/linux/uinput.h b/include/uapi/linux/uinput.h
> index c2e8710..27ee521 100644
> --- a/include/uapi/linux/uinput.h
> +++ b/include/uapi/linux/uinput.h
> @@ -140,7 +140,7 @@ struct uinput_user_dev2 {
> char name[UINPUT_MAX_NAME_SIZE];
> struct input_id id;
> __u32 ff_effects_max;
> - struct input_absinfo abs[ABS_CNT];
> + struct input_absinfo abs[ABS_CNT2];
> };
>
> #endif /* _UAPI__UINPUT_H_ */
> --
> 1.8.5.1
>
^ permalink raw reply
* Re: [PATCH 2/4] Input: introduce ABS_MAX2/CNT2 and friends
From: Antonio Ospite @ 2013-12-18 23:21 UTC (permalink / raw)
To: David Herrmann
Cc: open list:HID CORE LAYER, Dmitry Torokhov, Jiri Kosina,
Benjamin Tissoires, Peter Hutterer, linux-kernel, Input Tools
In-Reply-To: <CANq1E4RNa6XBaUTy=ur7Yig0iJSEaUbLzvi-Dn-hyxS2OLnyPw@mail.gmail.com>
On Wed, 18 Dec 2013 15:44:48 +0100
David Herrmann <dh.herrmann@gmail.com> wrote:
> Hi
>
> On Wed, Dec 18, 2013 at 3:27 PM, Antonio Ospite
> <ospite@studenti.unina.it> wrote:
> > Hi David,
> >
> > thanks for the update.
> >
> > On Tue, 17 Dec 2013 16:48:52 +0100
> > David Herrmann <dh.herrmann@gmail.com> wrote:
> >
> >> As we painfully noticed during the 3.12 merge-window our
> >> EVIOCGABS/EVIOCSABS API is limited to ABS_MAX<=0x3f. We tried several
> >> hacks to work around it but if we ever decide to increase ABS_MAX, the
> >> EVIOCSABS ioctl ABI might overflow into the next byte causing horrible
> >> misinterpretations in the kernel that we cannot catch.
> >>
> >> Therefore, we decided to go with ABS_MAX2/CNT2 and introduce two new
> >> ioctls to get/set abs-params. They no longer encode the ABS code in the
> >> ioctl number and thus allow up to 4 billion ABS codes.
> >>
> >
> > Just a question: did you consider the possibility of not exposing _MAX2
> > and _CNT2 in a header file at all but let those be queried? Or maybe
> > this is not necessary if we assume that userspace programs are supposed
> > to know what is the MAX _they_ support?
> >
> > BTW I was thinking for instance to a program compiled with ABS_MAX2 =
> > 0x4f but working with future kernels with an increased value, it still
> > won't be able to support values greater than 0x4f.
>
> Why would a program that was compiled with ABS_MAX2=0x4f want to use
> ABS_* values greater than 0x4f? It doesn't know of any new values so
> even if it could query ABS_MAX2, it could never know anything about
> the new constants. The only place where it is useful is for generic
> programs that just foward ABS_* codes. But these can just ignore
> ABS_MAX2 entirely and use the whole int32_t range for codes.
>
OK, that makes sense, thanks.
> If there is a specific use-case that'd require a dynamic way to
> retrieve ABS_MAX2, I can consider changing the API. But unless there's
> such a use-case, I'd like to keep it the same as for KEY_*, SW_*, ...
>
> >> The new API also allows to query multiple ABS values with one call. To
> >> allow EVIOCSABS2(code = 0, cnt = ABS_CNT2) we need to silently ignore
> >> writes to ABS_MT_SLOT. Furthermore, for better compatibility with
> >> newer user-space, we ignore writes to unknown codes. Hence, if we ever
> >> increase ABS_MAX2, new user-space will work with code=0,cnt=ABS_CNT2 just
> >> fine even on old kernels.
> >>
> >
> > To my inexperienced eye it's not obvious why ABS_MT_SLOT must be handled
> > as a special case. Maybe because I don't even know what
> > code=0,cnt=ABS_CNT2 is used for.
>
> ABS_MT_SLOT describes the current MT-slot that is reported via
> ABS_MT_* bits. It is read-only so we cannot let user-space write to
> it.
>
I see.
> >> Note that we also need to increase EV_VERSION so user-space can reliably
> >> know whether ABS2 is supported. Unfortunately, we return EINVAL instead of
> >> ENOSYS for unknown evdev ioctls so it's nearly impossible to catch
> >> reliably without EVIOCGVERSION.
> >>
> >
> > I'll check how to use that in evtest.
> >
> > Just one more comment below.
> >
> >> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
> >> ---
> >> drivers/hid/hid-debug.c | 2 +-
> >> drivers/hid/hid-input.c | 2 +-
> >> drivers/input/evdev.c | 95 +++++++++++++++++++++++++++++++-
> >> drivers/input/input.c | 14 ++---
> >> drivers/input/keyboard/goldfish_events.c | 6 +-
> >> drivers/input/keyboard/hil_kbd.c | 2 +-
> >> drivers/input/misc/uinput.c | 6 +-
> >> include/linux/hid.h | 2 +-
> >> include/linux/input.h | 6 +-
> >> include/uapi/linux/input.h | 42 +++++++++++++-
> >> include/uapi/linux/uinput.h | 2 +-
> >> 11 files changed, 155 insertions(+), 24 deletions(-)
> >>
> >
> > [...]
> >> diff --git a/include/uapi/linux/input.h b/include/uapi/linux/input.h
> >> index bd24470..1856461 100644
> >> --- a/include/uapi/linux/input.h
> >> +++ b/include/uapi/linux/input.h
> > [...]
> >>
> >> /*
> >> + * Due to API restrictions the legacy evdev API only supports ABS values up to
> >> + * ABS_MAX/CNT. Use the extended *ABS2 ioctls to operate on any ABS values in
> >> + * between ABS_MAX and ABS_MAX2.
> >> + */
> >> +#define ABS_MAX2 0x3f
> >> +#define ABS_CNT2 (ABS_MAX2+1)
> >> +
> >
> > Maybe it's just my English, but when you say "between ABS_MAX and
> > ABS_MAX2" it sounds like the old protocol still _must_ be used for
> > values <= ABS_MAX.
> >
> > IIUC this is true "only" for compatibility with older kernels right?
> > If a program decides to support only newer kernels it can check the
> > protocol version and use only the new ioctls, right?
> >
> > Maybe you can be more explicit about that in the comment?
>
> See the comment on "struct input_absinfo2". It describes the API, this
> comment just describes the ABS_* values. But if anyone has a better
> phrase to use here, I will gladly adjust it.
>
That comment says:
EVIOC[G/S]ABS2 ioctls [...] do the same as the old EVIOC
[G/S]ABS ioctls but...
My bad, I was confused by the fact that the new ioctls are presented as
a replacement for the old ones, but then I interpreted the later comment
as suggesting to use them only for values between ABS_MAX and ABS_MAX2.
Looking at how you use both mechanisms combined in libevdev it is clear
they can be used together for backward compatibility.
I like keywords, so I'd stress in the commit message that the new
protocol can either _replace_ or _extend_ the old one.
Ciao,
Antonio
--
Antonio Ospite
http://ao2.it
A: Because it messes up the order in which people normally read text.
See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?
^ permalink raw reply
* Re: [PATCH 1/4] Input: uinput: add full absinfo support
From: Peter Hutterer @ 2013-12-18 22:27 UTC (permalink / raw)
To: David Herrmann
Cc: linux-input, Dmitry Torokhov, Jiri Kosina, Benjamin Tissoires,
Antonio Ospite, linux-kernel, input-tools
In-Reply-To: <1387295334-1744-2-git-send-email-dh.herrmann@gmail.com>
On Tue, Dec 17, 2013 at 04:48:51PM +0100, David Herrmann wrote:
> We currently lack support for abs-resolution and abs-value parameters
> during uinput ABS initialization. Furthermore, our parsers don't allow
> growing ABS_CNT values. Therefore, introduce uinput_user_dev2.
>
> User-space is free to write uinput_user_dev2 objects instead of
> uinput_user_dev legacy objects now. If the kernel lacks support for it,
> our comparison for "count != sizeof(struct uinput_user_dev)" will catch
> this and return EINVAL. User-space shall retry with the legacy mode then.
>
> Internally, we transform the legacy object into uinput_user_dev2 and then
> handle both the same way.
>
> The new uinput_user_dev2 object has multiple new features:
> - abs payload now has "value" and "resolution" parameters as part of the
> switch to "struct input_absinfo". We simply copy these over.
> - Our parser allows growing ABS_CNT. We automatically detect the payload
> size of the caller, thus, calculating the ABS_CNT the program was
> compiled with.
> - A "version" field to denote the uinput-version used. This is required
> to properly support changing "struct input_user_dev2" changes in the
> future. Due to the dynamic ABS_CNT support, we cannot simply add new
> fields, as we cannot deduce the structure size from the user-space
> given size. Thus, we need the "version" field to allow changing the
> object and properly detecting it in our write() handler.
>
> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
> ---
> drivers/input/misc/uinput.c | 142 ++++++++++++++++++++++++++++++++------------
> include/uapi/linux/uinput.h | 9 +++
> 2 files changed, 114 insertions(+), 37 deletions(-)
>
> diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
> index 7728359..927ad9a 100644
> --- a/drivers/input/misc/uinput.c
> +++ b/drivers/input/misc/uinput.c
> @@ -358,14 +358,16 @@ static int uinput_allocate_device(struct uinput_device *udev)
> }
>
> static int uinput_setup_device(struct uinput_device *udev,
> - const char __user *buffer, size_t count)
> + struct uinput_user_dev2 *user_dev2,
> + size_t abscnt)
> {
> - struct uinput_user_dev *user_dev;
> struct input_dev *dev;
> int i;
> int retval;
> + struct input_absinfo *abs;
>
> - if (count != sizeof(struct uinput_user_dev))
> + /* Ensure name is filled in */
> + if (!user_dev2->name[0])
> return -EINVAL;
>
> if (!udev->dev) {
> @@ -375,37 +377,27 @@ static int uinput_setup_device(struct uinput_device *udev,
> }
>
> dev = udev->dev;
> -
> - user_dev = memdup_user(buffer, sizeof(struct uinput_user_dev));
> - if (IS_ERR(user_dev))
> - return PTR_ERR(user_dev);
> -
> - udev->ff_effects_max = user_dev->ff_effects_max;
> -
> - /* Ensure name is filled in */
> - if (!user_dev->name[0]) {
> - retval = -EINVAL;
> - goto exit;
> - }
> + udev->ff_effects_max = user_dev2->ff_effects_max;
>
> kfree(dev->name);
> - dev->name = kstrndup(user_dev->name, UINPUT_MAX_NAME_SIZE,
> + dev->name = kstrndup(user_dev2->name, UINPUT_MAX_NAME_SIZE,
> GFP_KERNEL);
> - if (!dev->name) {
> - retval = -ENOMEM;
> - goto exit;
> - }
> -
> - dev->id.bustype = user_dev->id.bustype;
> - dev->id.vendor = user_dev->id.vendor;
> - dev->id.product = user_dev->id.product;
> - dev->id.version = user_dev->id.version;
> + if (!dev->name)
> + return -ENOMEM;
>
> - for (i = 0; i < ABS_CNT; i++) {
> - input_abs_set_max(dev, i, user_dev->absmax[i]);
> - input_abs_set_min(dev, i, user_dev->absmin[i]);
> - input_abs_set_fuzz(dev, i, user_dev->absfuzz[i]);
> - input_abs_set_flat(dev, i, user_dev->absflat[i]);
> + dev->id.bustype = user_dev2->id.bustype;
> + dev->id.vendor = user_dev2->id.vendor;
> + dev->id.product = user_dev2->id.product;
> + dev->id.version = user_dev2->id.version;
> +
> + for (i = 0; i < abscnt; i++) {
> + abs = &user_dev2->abs[i];
minor nit: I'd just declare abs here.
> + input_abs_set_val(dev, i, abs->value);
> + input_abs_set_max(dev, i, abs->maximum);
> + input_abs_set_min(dev, i, abs->minimum);
> + input_abs_set_fuzz(dev, i, abs->fuzz);
> + input_abs_set_flat(dev, i, abs->flat);
> + input_abs_set_res(dev, i, abs->resolution);
> }
>
> /* check if absmin/absmax/absfuzz/absflat are filled as
> @@ -413,7 +405,7 @@ static int uinput_setup_device(struct uinput_device *udev,
> if (test_bit(EV_ABS, dev->evbit)) {
> retval = uinput_validate_absbits(dev);
> if (retval < 0)
> - goto exit;
> + return retval;
> if (test_bit(ABS_MT_SLOT, dev->absbit)) {
> int nslot = input_abs_get_max(dev, ABS_MT_SLOT) + 1;
> input_mt_init_slots(dev, nslot, 0);
> @@ -423,11 +415,84 @@ static int uinput_setup_device(struct uinput_device *udev,
> }
>
> udev->state = UIST_SETUP_COMPLETE;
> - retval = count;
> + return 0;
> +}
> +
> +static int uinput_setup_device1(struct uinput_device *udev,
> + const char __user *buffer, size_t count)
> +{
> + struct uinput_user_dev *user_dev;
> + struct uinput_user_dev2 *user_dev2;
> + int i;
> + int retval;
> +
> + if (count != sizeof(struct uinput_user_dev))
> + return -EINVAL;
> +
> + user_dev = memdup_user(buffer, sizeof(struct uinput_user_dev));
> + if (IS_ERR(user_dev))
> + return PTR_ERR(user_dev);
> +
> + user_dev2 = kmalloc(sizeof(*user_dev2), GFP_KERNEL);
> + if (!user_dev2) {
> + kfree(user_dev);
> + return -ENOMEM;
> + }
> +
> + user_dev2->version = UINPUT_VERSION;
> + memcpy(user_dev2->name, user_dev->name, UINPUT_MAX_NAME_SIZE);
> + memcpy(&user_dev2->id, &user_dev->id, sizeof(struct input_id));
you copy the id bits one-by-one into the input_dev but you memcpy it here.
is this intentional?
> + user_dev2->ff_effects_max = user_dev->ff_effects_max;
> +
> + for (i = 0; i < ABS_CNT; ++i) {
> + user_dev2->abs[i].value = 0;
> + user_dev2->abs[i].maximum = user_dev->absmax[i];
> + user_dev2->abs[i].minimum = user_dev->absmin[i];
> + user_dev2->abs[i].fuzz = user_dev->absfuzz[i];
> + user_dev2->abs[i].flat = user_dev->absflat[i];
> + user_dev2->abs[i].resolution = 0;
> + }
> +
> + retval = uinput_setup_device(udev, user_dev2, ABS_CNT);
>
> - exit:
> kfree(user_dev);
> - return retval;
> + kfree(user_dev2);
> +
> + return retval ? retval : count;
> +}
> +
> +static int uinput_setup_device2(struct uinput_device *udev,
> + const char __user *buffer, size_t count)
> +{
> + struct uinput_user_dev2 *user_dev2;
> + int retval;
> + size_t off, abscnt, max;
> +
> + /* The first revision of "uinput_user_dev2" is bigger than
> + * "uinput_user_dev" and growing. Disallow any smaller payloads. */
> + if (count <= sizeof(struct uinput_user_dev))
> + return -EINVAL;
> +
> + /* rough check to avoid huge kernel space allocations */
> + max = ABS_CNT * sizeof(*user_dev2->abs) + sizeof(*user_dev2);
> + if (count > max)
> + return -EINVAL;
> +
> + user_dev2 = memdup_user(buffer, count);
> + if (IS_ERR(user_dev2))
> + return PTR_ERR(user_dev2);
> +
> + if (user_dev2->version > UINPUT_VERSION) {
> + retval = -EINVAL;
> + } else {
> + off = offsetof(struct uinput_user_dev2, abs);
> + abscnt = (count - off) / sizeof(*user_dev2->abs);
> + retval = uinput_setup_device(udev, user_dev2, abscnt);
> + }
> +
I really wish uinput would be a bit easier to debug than just returning
-EINVAL when it's not happy. having said that, the only errno that would
remotely make sense is -ERANGE for count > max and even that is a bit meh.
Reviewed-by: Peter Hutterer <peter.hutterer@who-t.net>
Cheers,
Peter
> + kfree(user_dev2);
> +
> + return retval ? retval : count;
> }
>
> static ssize_t uinput_inject_events(struct uinput_device *udev,
> @@ -469,9 +534,12 @@ static ssize_t uinput_write(struct file *file, const char __user *buffer,
> if (retval)
> return retval;
>
> - retval = udev->state == UIST_CREATED ?
> - uinput_inject_events(udev, buffer, count) :
> - uinput_setup_device(udev, buffer, count);
> + if (udev->state == UIST_CREATED)
> + retval = uinput_inject_events(udev, buffer, count);
> + else if (count <= sizeof(struct uinput_user_dev))
> + retval = uinput_setup_device1(udev, buffer, count);
> + else
> + retval = uinput_setup_device2(udev, buffer, count);
>
> mutex_unlock(&udev->mutex);
>
> diff --git a/include/uapi/linux/uinput.h b/include/uapi/linux/uinput.h
> index fe46431..c2e8710 100644
> --- a/include/uapi/linux/uinput.h
> +++ b/include/uapi/linux/uinput.h
> @@ -134,4 +134,13 @@ struct uinput_user_dev {
> __s32 absfuzz[ABS_CNT];
> __s32 absflat[ABS_CNT];
> };
> +
> +struct uinput_user_dev2 {
> + __u8 version;
> + char name[UINPUT_MAX_NAME_SIZE];
> + struct input_id id;
> + __u32 ff_effects_max;
> + struct input_absinfo abs[ABS_CNT];
> +};
> +
> #endif /* _UAPI__UINPUT_H_ */
> --
> 1.8.5.1
>
^ permalink raw reply
* Re: am335x: IIO/ADC fixes if used together with TSC
From: Sebastian Andrzej Siewior @ 2013-12-18 18:34 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Dmitry Torokhov, Samuel Ortiz, Lee Jones, Zubair Lutfullah,
Felipe Balbi, linux-iio, linux-input, linux-kernel
In-Reply-To: <d916c9be-2614-41b8-ae92-67bf1a57ed74@email.android.com>
On 12/18/2013 06:25 PM, Jonathan Cameron wrote:
> Given timing I suspect that by the we have reviewed these will be too late in this cycle so will hit in the merge window plus stable.
>
> Also some of these are cleanups rather than fixes so should normally be separate from fixes and applied after them as they are not
> Stable material.
I tried to keep 5/5 as small as possible so I factor individual and
separate changes as good as possible as long as the drivers remained
functional. That means 1-4 are mostly pre-work for 5/5 and that makes
5/5 depends on them.
Tagging 5/5 stable I would prefer to take 1-4, too to keep the backport
effort small. The clash due to the continues mode which is v3.13 is
probably small. This said, I think it is the best to backport it stable
if someone asks for it (unless the majority here thinks otherwise).
Are you saying this is too late for v3.13 or v3.14-rc1?
Sebastian
^ permalink raw reply
* Re: am335x: IIO/ADC fixes if used together with TSC
From: Jonathan Cameron @ 2013-12-18 17:25 UTC (permalink / raw)
To: Sebastian Andrzej Siewior
Cc: Dmitry Torokhov, Samuel Ortiz, Lee Jones, Zubair Lutfullah,
Felipe Balbi, linux-iio, linux-input, linux-kernel
In-Reply-To: <1387385577-24447-1-git-send-email-bigeasy@linutronix.de>
Given timing I suspect that by the we have reviewed these will be too late in this cycle so will hit in the merge window plus stable.
Also some of these are cleanups rather than fixes so should normally be separate from fixes and applied after them as they are not
Stable material.
Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote:
>Hi,
>
>this series got a little bigger than expected with a biggy at the end.
>I didn't managed to split the patches across the three subsystems it
>touches due to the changes are required to keep it working and break
>the
>logic.
>Even in 5/5 which does three logical changes I didn't manage to split
>it
>in three patches because the driver would not be functional (sometimes
>it would
>get worse than before). This is all documented in the changelog.
>Thefore I ask to get this series applied via one tree, the iio tree
>which contains
>or depends on the most changes.
>
>The two bigger issues here are fixed by the last patch incuding
>- time outs on the iio/adc side while TSC&ADC is used together
>- a lockup issue which occurs while TSC&ADC is used together.
>
>Sebastian
>
>--
>To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Sent from my Android phone with K-9 Mail. Please excuse my brevity.
^ permalink raw reply
* [PATCH 5/5] mfd: input: iio: ti_amm335x: rework TSC/ADC synchronisation
From: Sebastian Andrzej Siewior @ 2013-12-18 16:52 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Dmitry Torokhov, Samuel Ortiz, Lee Jones, Zubair Lutfullah,
Felipe Balbi, linux-iio, linux-input, linux-kernel,
Sebastian Andrzej Siewior
In-Reply-To: <1387385577-24447-1-git-send-email-bigeasy@linutronix.de>
The ADC driver always programms all possible ADC values and discards
them except for the value IIO asked for. On the am335x-evm the driver
programs four values and it takes 500us to gather them. Reducing the number
of coversations down to (required) one also reduces the busy loop down to
125us.
This leads to another error, namely the FIFOCOUNT register is sometimes
(like one out of 10 attempts) not updated in time leading to timeouts.
The next read has the fifocount register updated.
Checking for the ADCStatus status register for beeing idle isn't a good
choice either. The problem is that it often times out if the TSC is used
at the same time. The problem is that the HW compelted the conversaton for
the ADC *and* before the driver noticed it, the HW began to perfom a
TSC conversation and so the driver never seen the HW idle. The next time
we would have two values in the fifo but since the driver reads
everything we always see the current one.
So instead of polling for the IDLE bit, we check for the fifocount. It
should be one instead of zero because we request one value.
This change in turn lead to another error :) Sometimes if TSC & ADC are
used together the TSC starts becomming interrupts even if nobody
actually touched the touchscreen. The interrupts are valid because TSC's
fifo is filled with values. This condition stops after a few ADC reads but
will occur once TSC & ADC are used at the same time. So not good.
On top of this (even without the changes I just mentioned) there is a ADC
& TSC lockup condition which was reported to my Jeff Lance including a test
case:
A busyloop of loop of "cat /sys/bus/iio/devices/iio\:device0/in_voltage4_raw"
and a mug on touch screen. Whith this setup, the hardware will lockup after
something between 20min and it could take up to a couple of hours. During that
lockup, the ADCSTAT says 0x30 (or 0x70) which means STEP_ID = IDLE and
FSM_BUSY = yes. That means it says that it is idle and busy at the same
time which is an invalid condition.
For all this reasons I decided to rework this TSC/ADC part and add a
handshake / synchronisation here:
First the ADC signals that it needs the HW and writes a 0 mask into the
SE register. The HW (if active) will complete the current conversation
and become idle. The TSC driver will gather the values from the FIFO
(woken up by an interrupt) and won't "enable" another conversation.
Instead it will wake up the ADC driver which is already waiting. The ADC
driver will start "its" converstation and once it is done, it will
enable the TSC events so the TSC will work again.
After this rework I haven't observed the lockup so far. Plus the busy
loop has been reduced from 500us to 125us.
The continues-read mode remains unchanged.
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
drivers/iio/adc/ti_am335x_adc.c | 60 +++++++++++++++++++-------
drivers/input/touchscreen/ti_am335x_tsc.c | 4 +-
drivers/mfd/ti_am335x_tscadc.c | 71 +++++++++++++++++++++++++++----
include/linux/mfd/ti_am335x_tscadc.h | 5 +++
4 files changed, 114 insertions(+), 26 deletions(-)
diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
index e737eef..252a2b5 100644
--- a/drivers/iio/adc/ti_am335x_adc.c
+++ b/drivers/iio/adc/ti_am335x_adc.c
@@ -60,6 +60,24 @@ static u32 get_adc_step_mask(struct tiadc_device *adc_dev)
return step_en;
}
+static u32 get_adc_chan_step_mask(struct tiadc_device *adc_dev,
+ struct iio_chan_spec const *chan)
+{
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(adc_dev->channel_step); i++) {
+ if (chan->channel == adc_dev->channel_line[i]) {
+ u32 step;
+
+ step = adc_dev->channel_step[i];
+ /* +1 for the charger */
+ return 1 << (step + 1);
+ }
+ }
+ WARN_ON(1);
+ return 0;
+}
+
static u32 get_adc_step_bit(struct tiadc_device *adc_dev, int chan)
{
return 1 << adc_dev->channel_step[chan];
@@ -329,34 +347,43 @@ static int tiadc_read_raw(struct iio_dev *indio_dev,
unsigned int fifo1count, read, stepid;
bool found = false;
u32 step_en;
- unsigned long timeout = jiffies + usecs_to_jiffies
- (IDLE_TIMEOUT * adc_dev->channels);
+ unsigned long timeout;
if (iio_buffer_enabled(indio_dev))
return -EBUSY;
- step_en = get_adc_step_mask(adc_dev);
+ step_en = get_adc_chan_step_mask(adc_dev, chan);
+ if (!step_en)
+ return -EINVAL;
+
+ fifo1count = tiadc_readl(adc_dev, REG_FIFO1CNT);
+ while (fifo1count--)
+ tiadc_readl(adc_dev, REG_FIFO1);
+
am335x_tsc_se_adc_set(adc_dev->mfd_tscadc, step_en);
- /* Wait for ADC sequencer to complete sampling */
- while (tiadc_readl(adc_dev, REG_ADCFSM) & SEQ_STATUS) {
- if (time_after(jiffies, timeout))
+ timeout = jiffies + usecs_to_jiffies
+ (IDLE_TIMEOUT * adc_dev->channels);
+ /* Wait for Fifo threshold interrupt */
+ while (1) {
+ fifo1count = tiadc_readl(adc_dev, REG_FIFO1CNT);
+ if (fifo1count)
+ break;
+
+ if (time_after(jiffies, timeout)) {
+ am335x_tsc_se_adc_done(adc_dev->mfd_tscadc);
return -EAGAIN;
+ }
}
map_val = chan->channel + TOTAL_CHANNELS;
/*
- * When the sub-system is first enabled,
- * the sequencer will always start with the
- * lowest step (1) and continue until step (16).
- * For ex: If we have enabled 4 ADC channels and
- * currently use only 1 out of them, the
- * sequencer still configures all the 4 steps,
- * leading to 3 unwanted data.
- * Hence we need to flush out this data.
+ * We check the complete FIFO. We programmed just one entry but in case
+ * something went wrong we left empty handed (-EAGAIN previously) and
+ * then the value apeared somehow in the FIFO we would have two entries.
+ * Therefore we read every item and keep only the latest version of the
+ * requested channel.
*/
-
- fifo1count = tiadc_readl(adc_dev, REG_FIFO1CNT);
for (i = 0; i < fifo1count; i++) {
read = tiadc_readl(adc_dev, REG_FIFO1);
stepid = read & FIFOREAD_CHNLID_MASK;
@@ -368,6 +395,7 @@ static int tiadc_read_raw(struct iio_dev *indio_dev,
*val = (u16) read;
}
}
+ am335x_tsc_se_adc_done(adc_dev->mfd_tscadc);
if (found == false)
return -EBUSY;
diff --git a/drivers/input/touchscreen/ti_am335x_tsc.c b/drivers/input/touchscreen/ti_am335x_tsc.c
index 68beada..5d46fff 100644
--- a/drivers/input/touchscreen/ti_am335x_tsc.c
+++ b/drivers/input/touchscreen/ti_am335x_tsc.c
@@ -198,7 +198,7 @@ static void titsc_step_config(struct titsc *ts_dev)
/* The steps1 … end and bit 0 for TS_Charge */
stepenable = (1 << (end_step + 2)) - 1;
ts_dev->step_mask = stepenable;
- am335x_tsc_se_set(ts_dev->mfd_tscadc, ts_dev->step_mask);
+ am335x_tsc_se_tsc_set(ts_dev->mfd_tscadc, ts_dev->step_mask);
}
static void titsc_read_coordinates(struct titsc *ts_dev,
@@ -322,7 +322,7 @@ static irqreturn_t titsc_irq(int irq, void *dev)
if (irqclr) {
titsc_writel(ts_dev, REG_IRQSTATUS, irqclr);
- am335x_tsc_se_set(ts_dev->mfd_tscadc, ts_dev->step_mask);
+ am335x_tsc_se_tsc_set(ts_dev->mfd_tscadc, ts_dev->step_mask);
return IRQ_HANDLED;
}
return IRQ_NONE;
diff --git a/drivers/mfd/ti_am335x_tscadc.c b/drivers/mfd/ti_am335x_tscadc.c
index ad560c6..dd950ec 100644
--- a/drivers/mfd/ti_am335x_tscadc.c
+++ b/drivers/mfd/ti_am335x_tscadc.c
@@ -24,6 +24,7 @@
#include <linux/pm_runtime.h>
#include <linux/of.h>
#include <linux/of_device.h>
+#include <linux/sched.h>
#include <linux/mfd/ti_am335x_tscadc.h>
@@ -48,31 +49,83 @@ static const struct regmap_config tscadc_regmap_config = {
.val_bits = 32,
};
-static void am335x_tsc_se_update(struct ti_tscadc_dev *tsadc)
+void am335x_tsc_se_set(struct ti_tscadc_dev *tsadc, u32 val)
{
+ unsigned long flags;
+
+ spin_lock_irqsave(&tsadc->reg_lock, flags);
+ tsadc->reg_se_cache |= val;
tscadc_writel(tsadc, REG_SE, tsadc->reg_se_cache);
+ spin_unlock_irqrestore(&tsadc->reg_lock, flags);
}
+EXPORT_SYMBOL_GPL(am335x_tsc_se_set);
-void am335x_tsc_se_set(struct ti_tscadc_dev *tsadc, u32 val)
+void am335x_tsc_se_tsc_set(struct ti_tscadc_dev *tsadc, u32 val)
{
unsigned long flags;
spin_lock_irqsave(&tsadc->reg_lock, flags);
- tsadc->reg_se_cache |= val;
- am335x_tsc_se_update(tsadc);
+ tsadc->reg_se_cache = val;
+ if (tsadc->adc_in_use || tsadc->adc_waiting) {
+ if (tsadc->adc_waiting)
+ wake_up(&tsadc->reg_se_wait);
+ } else {
+ tscadc_writel(tsadc, REG_SE, val);
+ }
spin_unlock_irqrestore(&tsadc->reg_lock, flags);
}
-EXPORT_SYMBOL_GPL(am335x_tsc_se_set);
+EXPORT_SYMBOL_GPL(am335x_tsc_se_tsc_set);
+
+static void am335x_tscadc_need_adc(struct ti_tscadc_dev *tsadc)
+{
+ DEFINE_WAIT(wait);
+ u32 reg;
+
+ /*
+ * disable TSC steps so it does not run while the ADC is using it. If
+ * write 0 while it is running (it just started or was already running)
+ * then it completes all steps that were enabled and stops then.
+ */
+ tscadc_writel(tsadc, REG_SE, 0);
+ reg = tscadc_readl(tsadc, REG_ADCFSM);
+ if (reg & SEQ_STATUS) {
+ tsadc->adc_waiting = true;
+ prepare_to_wait(&tsadc->reg_se_wait, &wait,
+ TASK_UNINTERRUPTIBLE);
+ spin_unlock_irq(&tsadc->reg_lock);
+
+ schedule();
+
+ spin_lock_irq(&tsadc->reg_lock);
+ finish_wait(&tsadc->reg_se_wait, &wait);
+
+ reg = tscadc_readl(tsadc, REG_ADCFSM);
+ WARN_ON(reg & SEQ_STATUS);
+ tsadc->adc_waiting = false;
+ }
+ tsadc->adc_in_use = true;
+}
void am335x_tsc_se_adc_set(struct ti_tscadc_dev *tsadc, u32 val)
{
+ spin_lock_irq(&tsadc->reg_lock);
+ am335x_tscadc_need_adc(tsadc);
+
+ tscadc_writel(tsadc, REG_SE, val);
+ spin_unlock_irq(&tsadc->reg_lock);
+}
+EXPORT_SYMBOL_GPL(am335x_tsc_se_adc_set);
+
+void am335x_tsc_se_adc_done(struct ti_tscadc_dev *tsadc)
+{
unsigned long flags;
spin_lock_irqsave(&tsadc->reg_lock, flags);
- tscadc_writel(tsadc, REG_SE, tsadc->reg_se_cache | val);
+ tsadc->adc_in_use = false;
+ tscadc_writel(tsadc, REG_SE, tsadc->reg_se_cache);
spin_unlock_irqrestore(&tsadc->reg_lock, flags);
}
-EXPORT_SYMBOL_GPL(am335x_tsc_se_adc_set);
+EXPORT_SYMBOL_GPL(am335x_tsc_se_adc_done);
void am335x_tsc_se_clr(struct ti_tscadc_dev *tsadc, u32 val)
{
@@ -80,7 +133,7 @@ void am335x_tsc_se_clr(struct ti_tscadc_dev *tsadc, u32 val)
spin_lock_irqsave(&tsadc->reg_lock, flags);
tsadc->reg_se_cache &= ~val;
- am335x_tsc_se_update(tsadc);
+ tscadc_writel(tsadc, REG_SE, tsadc->reg_se_cache);
spin_unlock_irqrestore(&tsadc->reg_lock, flags);
}
EXPORT_SYMBOL_GPL(am335x_tsc_se_clr);
@@ -188,6 +241,8 @@ static int ti_tscadc_probe(struct platform_device *pdev)
}
spin_lock_init(&tscadc->reg_lock);
+ init_waitqueue_head(&tscadc->reg_se_wait);
+
pm_runtime_enable(&pdev->dev);
pm_runtime_get_sync(&pdev->dev);
diff --git a/include/linux/mfd/ti_am335x_tscadc.h b/include/linux/mfd/ti_am335x_tscadc.h
index be44a66..3cd4f3d3 100644
--- a/include/linux/mfd/ti_am335x_tscadc.h
+++ b/include/linux/mfd/ti_am335x_tscadc.h
@@ -159,6 +159,9 @@ struct ti_tscadc_dev {
int adc_cell; /* -1 if not used */
struct mfd_cell cells[TSCADC_CELLS];
u32 reg_se_cache;
+ bool adc_waiting;
+ bool adc_in_use;
+ wait_queue_head_t reg_se_wait;
spinlock_t reg_lock;
unsigned int clk_div;
@@ -177,7 +180,9 @@ static inline struct ti_tscadc_dev *ti_tscadc_dev_get(struct platform_device *p)
}
void am335x_tsc_se_set(struct ti_tscadc_dev *tsadc, u32 val);
+void am335x_tsc_se_tsc_set(struct ti_tscadc_dev *tsadc, u32 val);
void am335x_tsc_se_adc_set(struct ti_tscadc_dev *tsadc, u32 val);
void am335x_tsc_se_clr(struct ti_tscadc_dev *tsadc, u32 val);
+void am335x_tsc_se_adc_done(struct ti_tscadc_dev *tsadc);
#endif
--
1.8.5.1
^ permalink raw reply related
* [PATCH 4/5] mfd: ti_am335x: drop am335x_tsc_se_update() from resume path
From: Sebastian Andrzej Siewior @ 2013-12-18 16:52 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Dmitry Torokhov, Samuel Ortiz, Lee Jones, Zubair Lutfullah,
Felipe Balbi, linux-iio, linux-input, linux-kernel,
Sebastian Andrzej Siewior
In-Reply-To: <1387385577-24447-1-git-send-email-bigeasy@linutronix.de>
The update of the SE register in MFD doesn't look right as it has
nothing to do with it. The better place to do it is in TSC driver (which
is already doint it) and in the ADC driver which needs this only in the
continues mode.
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
drivers/iio/adc/ti_am335x_adc.c | 2 ++
drivers/mfd/ti_am335x_tscadc.c | 1 -
2 files changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
index 9ad9bd9..e737eef 100644
--- a/drivers/iio/adc/ti_am335x_adc.c
+++ b/drivers/iio/adc/ti_am335x_adc.c
@@ -199,6 +199,7 @@ static int tiadc_buffer_predisable(struct iio_dev *indio_dev)
tiadc_writel(adc_dev, REG_IRQCLR, (IRQENB_FIFO1THRES |
IRQENB_FIFO1OVRRUN | IRQENB_FIFO1UNDRFLW));
am335x_tsc_se_clr(adc_dev->mfd_tscadc, adc_dev->buffer_en_ch_steps);
+ adc_dev->buffer_en_ch_steps = 0;
/* Flush FIFO of leftover data in the time it takes to disable adc */
fifo1count = tiadc_readl(adc_dev, REG_FIFO1CNT);
@@ -494,6 +495,7 @@ static int tiadc_resume(struct device *dev)
tiadc_writel(adc_dev, REG_CTRL, restore);
tiadc_step_config(indio_dev);
+ am335x_tsc_se_set(adc_dev->mfd_tscadc, adc_dev->buffer_en_ch_steps);
return 0;
}
diff --git a/drivers/mfd/ti_am335x_tscadc.c b/drivers/mfd/ti_am335x_tscadc.c
index 2948a41..ad560c6 100644
--- a/drivers/mfd/ti_am335x_tscadc.c
+++ b/drivers/mfd/ti_am335x_tscadc.c
@@ -309,7 +309,6 @@ static int tscadc_resume(struct device *dev)
if (tscadc_dev->tsc_cell != -1)
tscadc_idle_config(tscadc_dev);
- am335x_tsc_se_update(tscadc_dev);
restore = tscadc_readl(tscadc_dev, REG_CTRL);
tscadc_writel(tscadc_dev, REG_CTRL,
(restore | CNTRLREG_TSCSSENB));
--
1.8.5.1
^ permalink raw reply related
* [PATCH 3/5] mfd: ti_am335x_tscadc: don't read back REG_SE
From: Sebastian Andrzej Siewior @ 2013-12-18 16:52 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Dmitry Torokhov, Samuel Ortiz, Lee Jones, Zubair Lutfullah,
Felipe Balbi, linux-iio, linux-input, linux-kernel,
Sebastian Andrzej Siewior
In-Reply-To: <1387385577-24447-1-git-send-email-bigeasy@linutronix.de>
The purpose of reg_se_cache has been defeated. It should avoid the
read-back of the register to avoid the latency and the fact that the
bits are reset to 0 after the individual conversation took place.
The reason why this is required like this to work, is that read-back of
the register removes the bits of the ADC so they do not start another
conversation after the register is re-written from the TSC side for the
update.
To avoid the unrequired read-back I introduce a "set adc" variant which
does not update the cache mask. After the conversation completes, the
register bit is removed from the SE reister anyway and we don't plan a
new conversation "any time soon".
This is a small preparation for a larger sync-rework.
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
drivers/iio/adc/ti_am335x_adc.c | 2 +-
drivers/mfd/ti_am335x_tscadc.c | 12 ++++++++++--
include/linux/mfd/ti_am335x_tscadc.h | 1 +
3 files changed, 12 insertions(+), 3 deletions(-)
diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
index e948454..9ad9bd9 100644
--- a/drivers/iio/adc/ti_am335x_adc.c
+++ b/drivers/iio/adc/ti_am335x_adc.c
@@ -335,7 +335,7 @@ static int tiadc_read_raw(struct iio_dev *indio_dev,
return -EBUSY;
step_en = get_adc_step_mask(adc_dev);
- am335x_tsc_se_set(adc_dev->mfd_tscadc, step_en);
+ am335x_tsc_se_adc_set(adc_dev->mfd_tscadc, step_en);
/* Wait for ADC sequencer to complete sampling */
while (tiadc_readl(adc_dev, REG_ADCFSM) & SEQ_STATUS) {
diff --git a/drivers/mfd/ti_am335x_tscadc.c b/drivers/mfd/ti_am335x_tscadc.c
index 67d0eb4..2948a41 100644
--- a/drivers/mfd/ti_am335x_tscadc.c
+++ b/drivers/mfd/ti_am335x_tscadc.c
@@ -58,19 +58,27 @@ void am335x_tsc_se_set(struct ti_tscadc_dev *tsadc, u32 val)
unsigned long flags;
spin_lock_irqsave(&tsadc->reg_lock, flags);
- tsadc->reg_se_cache = tscadc_readl(tsadc, REG_SE);
tsadc->reg_se_cache |= val;
am335x_tsc_se_update(tsadc);
spin_unlock_irqrestore(&tsadc->reg_lock, flags);
}
EXPORT_SYMBOL_GPL(am335x_tsc_se_set);
+void am335x_tsc_se_adc_set(struct ti_tscadc_dev *tsadc, u32 val)
+{
+ unsigned long flags;
+
+ spin_lock_irqsave(&tsadc->reg_lock, flags);
+ tscadc_writel(tsadc, REG_SE, tsadc->reg_se_cache | val);
+ spin_unlock_irqrestore(&tsadc->reg_lock, flags);
+}
+EXPORT_SYMBOL_GPL(am335x_tsc_se_adc_set);
+
void am335x_tsc_se_clr(struct ti_tscadc_dev *tsadc, u32 val)
{
unsigned long flags;
spin_lock_irqsave(&tsadc->reg_lock, flags);
- tsadc->reg_se_cache = tscadc_readl(tsadc, REG_SE);
tsadc->reg_se_cache &= ~val;
am335x_tsc_se_update(tsadc);
spin_unlock_irqrestore(&tsadc->reg_lock, flags);
diff --git a/include/linux/mfd/ti_am335x_tscadc.h b/include/linux/mfd/ti_am335x_tscadc.h
index 1fe7219..be44a66 100644
--- a/include/linux/mfd/ti_am335x_tscadc.h
+++ b/include/linux/mfd/ti_am335x_tscadc.h
@@ -177,6 +177,7 @@ static inline struct ti_tscadc_dev *ti_tscadc_dev_get(struct platform_device *p)
}
void am335x_tsc_se_set(struct ti_tscadc_dev *tsadc, u32 val);
+void am335x_tsc_se_adc_set(struct ti_tscadc_dev *tsadc, u32 val);
void am335x_tsc_se_clr(struct ti_tscadc_dev *tsadc, u32 val);
#endif
--
1.8.5.1
^ permalink raw reply related
* [PATCH 2/5] mfd: ti_am335x_tscadc: make am335x_tsc_se_update() local
From: Sebastian Andrzej Siewior @ 2013-12-18 16:52 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Dmitry Torokhov, Samuel Ortiz, Lee Jones, Zubair Lutfullah,
Felipe Balbi, linux-iio, linux-input, linux-kernel,
Sebastian Andrzej Siewior
In-Reply-To: <1387385577-24447-1-git-send-email-bigeasy@linutronix.de>
since the "recent" changes, am335x_tsc_se_update() has no longer any
users outside of this file so make it local.
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
drivers/mfd/ti_am335x_tscadc.c | 3 +--
include/linux/mfd/ti_am335x_tscadc.h | 1 -
2 files changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/mfd/ti_am335x_tscadc.c b/drivers/mfd/ti_am335x_tscadc.c
index 88718ab..67d0eb4 100644
--- a/drivers/mfd/ti_am335x_tscadc.c
+++ b/drivers/mfd/ti_am335x_tscadc.c
@@ -48,11 +48,10 @@ static const struct regmap_config tscadc_regmap_config = {
.val_bits = 32,
};
-void am335x_tsc_se_update(struct ti_tscadc_dev *tsadc)
+static void am335x_tsc_se_update(struct ti_tscadc_dev *tsadc)
{
tscadc_writel(tsadc, REG_SE, tsadc->reg_se_cache);
}
-EXPORT_SYMBOL_GPL(am335x_tsc_se_update);
void am335x_tsc_se_set(struct ti_tscadc_dev *tsadc, u32 val)
{
diff --git a/include/linux/mfd/ti_am335x_tscadc.h b/include/linux/mfd/ti_am335x_tscadc.h
index d498d98f..1fe7219 100644
--- a/include/linux/mfd/ti_am335x_tscadc.h
+++ b/include/linux/mfd/ti_am335x_tscadc.h
@@ -176,7 +176,6 @@ static inline struct ti_tscadc_dev *ti_tscadc_dev_get(struct platform_device *p)
return *tscadc_dev;
}
-void am335x_tsc_se_update(struct ti_tscadc_dev *tsadc);
void am335x_tsc_se_set(struct ti_tscadc_dev *tsadc, u32 val);
void am335x_tsc_se_clr(struct ti_tscadc_dev *tsadc, u32 val);
--
1.8.5.1
^ permalink raw reply related
* [PATCH 1/5] iio: ti_am335x_adc: adjust the closing bracket in tiadc_read_raw()
From: Sebastian Andrzej Siewior @ 2013-12-18 16:52 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Dmitry Torokhov, Samuel Ortiz, Lee Jones, Zubair Lutfullah,
Felipe Balbi, linux-iio, linux-input, linux-kernel,
Sebastian Andrzej Siewior
In-Reply-To: <1387385577-24447-1-git-send-email-bigeasy@linutronix.de>
It somehow looks like the ending bracket belongs to the if statement but
it does belong to the while loop. This patch moves the bracket where it
belongs.
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
drivers/iio/adc/ti_am335x_adc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
index d4d7482..e948454 100644
--- a/drivers/iio/adc/ti_am335x_adc.c
+++ b/drivers/iio/adc/ti_am335x_adc.c
@@ -341,7 +341,7 @@ static int tiadc_read_raw(struct iio_dev *indio_dev,
while (tiadc_readl(adc_dev, REG_ADCFSM) & SEQ_STATUS) {
if (time_after(jiffies, timeout))
return -EAGAIN;
- }
+ }
map_val = chan->channel + TOTAL_CHANNELS;
/*
--
1.8.5.1
^ permalink raw reply related
* am335x: IIO/ADC fixes if used together with TSC
From: Sebastian Andrzej Siewior @ 2013-12-18 16:52 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Dmitry Torokhov, Samuel Ortiz, Lee Jones, Zubair Lutfullah,
Felipe Balbi, linux-iio-u79uwXL29TY76Z2rM5mHXA,
linux-input-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
Hi,
this series got a little bigger than expected with a biggy at the end.
I didn't managed to split the patches across the three subsystems it
touches due to the changes are required to keep it working and break the
logic.
Even in 5/5 which does three logical changes I didn't manage to split it
in three patches because the driver would not be functional (sometimes it would
get worse than before). This is all documented in the changelog.
Thefore I ask to get this series applied via one tree, the iio tree which contains
or depends on the most changes.
The two bigger issues here are fixed by the last patch incuding
- time outs on the iio/adc side while TSC&ADC is used together
- a lockup issue which occurs while TSC&ADC is used together.
Sebastian
^ permalink raw reply
* Re: [PATCH] input synaptics-rmi4 trivial: Eliminate unused local variable.
From: Dmitry Torokhov @ 2013-12-18 16:52 UTC (permalink / raw)
To: Christopher Heiny
Cc: Linux Input, Andrew Duggan, Vincent Huang, Vivian Ly,
Daniel Rosenberg, Benjamin Tissoires
In-Reply-To: <1387247151-21131-1-git-send-email-cheiny@synaptics.com>
On Mon, Dec 16, 2013 at 06:25:51PM -0800, Christopher Heiny wrote:
> One-line change to eliminate an unused local variable warning in rmi_f11.c.
>
> Signed-off-by: Christopher Heiny <cheiny@synaptics.com>
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>
Applied, thank you.
>
> ---
>
> This patch implements changes to the synaptics-rmi4 branch of
> Dmitry's input tree. The base for the patch is commit
> e0c5aec5e6144ae8391d164e2dc659f8ef2b2ba7.
>
> drivers/input/rmi4/rmi_f11.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/drivers/input/rmi4/rmi_f11.c b/drivers/input/rmi4/rmi_f11.c
> index d63ef31..c2be9de 100644
> --- a/drivers/input/rmi4/rmi_f11.c
> +++ b/drivers/input/rmi4/rmi_f11.c
> @@ -1289,7 +1289,6 @@ static int rmi_f11_register_devices(struct rmi_function *fn)
> struct f11_data *f11 = fn->data;
> struct input_dev *input_dev;
> struct input_dev *input_dev_mouse;
> - struct rmi_driver_data *driver_data = dev_get_drvdata(&rmi_dev->dev);
> struct rmi_driver *driver = rmi_dev->driver;
> struct f11_2d_sensor *sensor = &f11->sensor;
> int rc;
--
Dmitry
^ permalink raw reply
* Re: [PATCH -next] Input: zforce - fix error return code in zforce_start()
From: Dmitry Torokhov @ 2013-12-18 16:50 UTC (permalink / raw)
To: Heiko Stübner; +Cc: Wei Yongjun, rydberg, yongjun_wei, linux-input
In-Reply-To: <201312171606.49401.heiko@sntech.de>
On Tue, Dec 17, 2013 at 04:06:49PM +0100, Heiko Stübner wrote:
> Am Dienstag, 17. Dezember 2013, 04:27:16 schrieb Wei Yongjun:
> > From: Wei Yongjun <yongjun_wei@trendmicro.com.cn>
> >
> > The error code was not set if unable to set config, so the error
> > condition wasn't reflected in the return value. Fix to return a
> > negative error code from the error handling case instead of 0.
> >
> > Signed-off-by: Wei Yongjun <yongjun_wei@trendmicro.com.cn>
>
> Thanks for the catch.
>
> Acked-by: Heiko Stuebner <heiko@sntech.de>
Applied, thank you.
--
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
^ permalink raw reply
* Re: [PATCHv2] Input: fix typos in Documentation/input/gamepad.txt
From: Dmitry Torokhov @ 2013-12-18 16:39 UTC (permalink / raw)
To: Antonio Ospite; +Cc: linux-input, Randy Dunlap, David Herrmann, Jiri Kosina
In-Reply-To: <1387280024-12213-1-git-send-email-ospite@studenti.unina.it>
On Tue, Dec 17, 2013 at 12:33:44PM +0100, Antonio Ospite wrote:
> Fix some typos and while at it also use "PS" as the name for the central
> "HOME" button on Sony controllers, this is how Sony itself calls it.
>
> Reviewed-by: David Herrmann <dh.herrmann@gmail.com>
> Signed-off-by: Antonio Ospite <ospite@studenti.unina.it>
Applied, thank you.
> ---
>
> Changes since v1:
> - Fix one of the fixes: use "advise" (verb) in place of "advice" (noun)
>
> Thanks Randy.
>
> Documentation/input/gamepad.txt | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/input/gamepad.txt b/Documentation/input/gamepad.txt
> index 31bb6a4..3f6d8a5 100644
> --- a/Documentation/input/gamepad.txt
> +++ b/Documentation/input/gamepad.txt
> @@ -68,7 +68,7 @@ features that you need, first. How each feature is mapped is described below.
> Legacy drivers often don't comply to these rules. As we cannot change them
> for backwards-compatibility reasons, you need to provide fixup mappings in
> user-space yourself. Some of them might also provide module-options that
> -change the mappings so you can adivce users to set these.
> +change the mappings so you can advise users to set these.
>
> All new gamepads are supposed to comply with this mapping. Please report any
> bugs, if they don't.
> @@ -150,10 +150,10 @@ Menu-Pad:
> BTN_START
> Many pads also have a third button which is branded or has a special symbol
> and meaning. Such buttons are mapped as BTN_MODE. Examples are the Nintendo
> - "HOME" button, the XBox "X"-button or Sony "P" button.
> + "HOME" button, the XBox "X"-button or Sony "PS" button.
>
> Rumble:
> - Rumble is adverticed as FF_RUMBLE.
> + Rumble is advertised as FF_RUMBLE.
>
> ----------------------------------------------------------------------------
> Written 2013 by David Herrmann <dh.herrmann@gmail.com>
> --
> 1.8.5.1
>
--
Dmitry
^ permalink raw reply
* Re: [PATCH 2/4] Input: introduce ABS_MAX2/CNT2 and friends
From: Dmitry Torokhov @ 2013-12-18 16:36 UTC (permalink / raw)
To: David Herrmann
Cc: Antonio Ospite, open list:HID CORE LAYER, Jiri Kosina,
Benjamin Tissoires, Peter Hutterer, linux-kernel, Input Tools
In-Reply-To: <CANq1E4RNa6XBaUTy=ur7Yig0iJSEaUbLzvi-Dn-hyxS2OLnyPw@mail.gmail.com>
On Wed, Dec 18, 2013 at 03:44:48PM +0100, David Herrmann wrote:
> >>
> >> /*
> >> + * Due to API restrictions the legacy evdev API only supports ABS values up to
> >> + * ABS_MAX/CNT. Use the extended *ABS2 ioctls to operate on any ABS values in
> >> + * between ABS_MAX and ABS_MAX2.
> >> + */
> >> +#define ABS_MAX2 0x3f
> >> +#define ABS_CNT2 (ABS_MAX2+1)
> >> +
> >
> > Maybe it's just my English, but when you say "between ABS_MAX and
> > ABS_MAX2" it sounds like the old protocol still _must_ be used for
> > values <= ABS_MAX.
> >
> > IIUC this is true "only" for compatibility with older kernels right?
> > If a program decides to support only newer kernels it can check the
> > protocol version and use only the new ioctls, right?
> >
> > Maybe you can be more explicit about that in the comment?
>
> See the comment on "struct input_absinfo2". It describes the API, this
> comment just describes the ABS_* values. But if anyone has a better
> phrase to use here, I will gladly adjust it.
"Use the extended *ABS2 ioctls to access the full range of ABS values
supported by the kernel."
?
--
Dmitry
^ permalink raw reply
* Re: [PATCH 2/4] Input: introduce ABS_MAX2/CNT2 and friends
From: Dmitry Torokhov @ 2013-12-18 14:47 UTC (permalink / raw)
To: David Herrmann
Cc: linux-input, Jiri Kosina, Benjamin Tissoires, Peter Hutterer,
Antonio Ospite, linux-kernel, input-tools
In-Reply-To: <1387295334-1744-3-git-send-email-dh.herrmann@gmail.com>
On Tue, Dec 17, 2013 at 04:48:52PM +0100, David Herrmann wrote:
> As we painfully noticed during the 3.12 merge-window our
> EVIOCGABS/EVIOCSABS API is limited to ABS_MAX<=0x3f. We tried several
> hacks to work around it but if we ever decide to increase ABS_MAX, the
> EVIOCSABS ioctl ABI might overflow into the next byte causing horrible
> misinterpretations in the kernel that we cannot catch.
>
> Therefore, we decided to go with ABS_MAX2/CNT2 and introduce two new
> ioctls to get/set abs-params. They no longer encode the ABS code in the
> ioctl number and thus allow up to 4 billion ABS codes.
>
> The new API also allows to query multiple ABS values with one call. To
> allow EVIOCSABS2(code = 0, cnt = ABS_CNT2) we need to silently ignore
> writes to ABS_MT_SLOT. Furthermore, for better compatibility with
> newer user-space, we ignore writes to unknown codes. Hence, if we ever
> increase ABS_MAX2, new user-space will work with code=0,cnt=ABS_CNT2 just
> fine even on old kernels.
>
> Note that we also need to increase EV_VERSION so user-space can reliably
> know whether ABS2 is supported. Unfortunately, we return EINVAL instead of
> ENOSYS for unknown evdev ioctls so it's nearly impossible to catch
> reliably without EVIOCGVERSION.
>
> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
> ---
> drivers/hid/hid-debug.c | 2 +-
> drivers/hid/hid-input.c | 2 +-
> drivers/input/evdev.c | 95 +++++++++++++++++++++++++++++++-
> drivers/input/input.c | 14 ++---
> drivers/input/keyboard/goldfish_events.c | 6 +-
> drivers/input/keyboard/hil_kbd.c | 2 +-
> drivers/input/misc/uinput.c | 6 +-
> include/linux/hid.h | 2 +-
> include/linux/input.h | 6 +-
> include/uapi/linux/input.h | 42 +++++++++++++-
> include/uapi/linux/uinput.h | 2 +-
> 11 files changed, 155 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/hid/hid-debug.c b/drivers/hid/hid-debug.c
> index 8453214..d32fa30 100644
> --- a/drivers/hid/hid-debug.c
> +++ b/drivers/hid/hid-debug.c
> @@ -862,7 +862,7 @@ static const char *relatives[REL_MAX + 1] = {
> [REL_WHEEL] = "Wheel", [REL_MISC] = "Misc",
> };
>
> -static const char *absolutes[ABS_CNT] = {
> +static const char *absolutes[ABS_CNT2] = {
> [ABS_X] = "X", [ABS_Y] = "Y",
> [ABS_Z] = "Z", [ABS_RX] = "Rx",
> [ABS_RY] = "Ry", [ABS_RZ] = "Rz",
> diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
> index d97f232..a02721c 100644
> --- a/drivers/hid/hid-input.c
> +++ b/drivers/hid/hid-input.c
> @@ -1300,7 +1300,7 @@ static bool hidinput_has_been_populated(struct hid_input *hidinput)
> for (i = 0; i < BITS_TO_LONGS(REL_CNT); i++)
> r |= hidinput->input->relbit[i];
>
> - for (i = 0; i < BITS_TO_LONGS(ABS_CNT); i++)
> + for (i = 0; i < BITS_TO_LONGS(ABS_CNT2); i++)
> r |= hidinput->input->absbit[i];
I wonder if kittens will die if we do:
in include/uapi/linux/input:
#define ABS_MAX1 0x3f
#define ABS_MAX2 0xYY
/*
* Keep old value of ABS_MAX exported to userspace. Users ready to
* handle bigger range will have to use ABS_MAX2 explicitly.
*/
#define ABS_MAX ABS_MAX1
in include/linux/input.h
...
/* Kernel should use the new ABS range by default */
#undef ABS_MAX
#define ABS_MAX ABS_MAX2
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH 2/4] Input: introduce ABS_MAX2/CNT2 and friends
From: David Herrmann @ 2013-12-18 14:44 UTC (permalink / raw)
To: Antonio Ospite
Cc: open list:HID CORE LAYER, Dmitry Torokhov, Jiri Kosina,
Benjamin Tissoires, Peter Hutterer, linux-kernel, Input Tools
In-Reply-To: <20131218152731.0222ae3b3807601b7756c937@studenti.unina.it>
Hi
On Wed, Dec 18, 2013 at 3:27 PM, Antonio Ospite
<ospite@studenti.unina.it> wrote:
> Hi David,
>
> thanks for the update.
>
> On Tue, 17 Dec 2013 16:48:52 +0100
> David Herrmann <dh.herrmann@gmail.com> wrote:
>
>> As we painfully noticed during the 3.12 merge-window our
>> EVIOCGABS/EVIOCSABS API is limited to ABS_MAX<=0x3f. We tried several
>> hacks to work around it but if we ever decide to increase ABS_MAX, the
>> EVIOCSABS ioctl ABI might overflow into the next byte causing horrible
>> misinterpretations in the kernel that we cannot catch.
>>
>> Therefore, we decided to go with ABS_MAX2/CNT2 and introduce two new
>> ioctls to get/set abs-params. They no longer encode the ABS code in the
>> ioctl number and thus allow up to 4 billion ABS codes.
>>
>
> Just a question: did you consider the possibility of not exposing _MAX2
> and _CNT2 in a header file at all but let those be queried? Or maybe
> this is not necessary if we assume that userspace programs are supposed
> to know what is the MAX _they_ support?
>
> BTW I was thinking for instance to a program compiled with ABS_MAX2 =
> 0x4f but working with future kernels with an increased value, it still
> won't be able to support values greater than 0x4f.
Why would a program that was compiled with ABS_MAX2=0x4f want to use
ABS_* values greater than 0x4f? It doesn't know of any new values so
even if it could query ABS_MAX2, it could never know anything about
the new constants. The only place where it is useful is for generic
programs that just foward ABS_* codes. But these can just ignore
ABS_MAX2 entirely and use the whole int32_t range for codes.
If there is a specific use-case that'd require a dynamic way to
retrieve ABS_MAX2, I can consider changing the API. But unless there's
such a use-case, I'd like to keep it the same as for KEY_*, SW_*, ...
>> The new API also allows to query multiple ABS values with one call. To
>> allow EVIOCSABS2(code = 0, cnt = ABS_CNT2) we need to silently ignore
>> writes to ABS_MT_SLOT. Furthermore, for better compatibility with
>> newer user-space, we ignore writes to unknown codes. Hence, if we ever
>> increase ABS_MAX2, new user-space will work with code=0,cnt=ABS_CNT2 just
>> fine even on old kernels.
>>
>
> To my inexperienced eye it's not obvious why ABS_MT_SLOT must be handled
> as a special case. Maybe because I don't even know what
> code=0,cnt=ABS_CNT2 is used for.
ABS_MT_SLOT describes the current MT-slot that is reported via
ABS_MT_* bits. It is read-only so we cannot let user-space write to
it.
>> Note that we also need to increase EV_VERSION so user-space can reliably
>> know whether ABS2 is supported. Unfortunately, we return EINVAL instead of
>> ENOSYS for unknown evdev ioctls so it's nearly impossible to catch
>> reliably without EVIOCGVERSION.
>>
>
> I'll check how to use that in evtest.
>
> Just one more comment below.
>
>> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
>> ---
>> drivers/hid/hid-debug.c | 2 +-
>> drivers/hid/hid-input.c | 2 +-
>> drivers/input/evdev.c | 95 +++++++++++++++++++++++++++++++-
>> drivers/input/input.c | 14 ++---
>> drivers/input/keyboard/goldfish_events.c | 6 +-
>> drivers/input/keyboard/hil_kbd.c | 2 +-
>> drivers/input/misc/uinput.c | 6 +-
>> include/linux/hid.h | 2 +-
>> include/linux/input.h | 6 +-
>> include/uapi/linux/input.h | 42 +++++++++++++-
>> include/uapi/linux/uinput.h | 2 +-
>> 11 files changed, 155 insertions(+), 24 deletions(-)
>>
>
> [...]
>> diff --git a/include/uapi/linux/input.h b/include/uapi/linux/input.h
>> index bd24470..1856461 100644
>> --- a/include/uapi/linux/input.h
>> +++ b/include/uapi/linux/input.h
> [...]
>>
>> /*
>> + * Due to API restrictions the legacy evdev API only supports ABS values up to
>> + * ABS_MAX/CNT. Use the extended *ABS2 ioctls to operate on any ABS values in
>> + * between ABS_MAX and ABS_MAX2.
>> + */
>> +#define ABS_MAX2 0x3f
>> +#define ABS_CNT2 (ABS_MAX2+1)
>> +
>
> Maybe it's just my English, but when you say "between ABS_MAX and
> ABS_MAX2" it sounds like the old protocol still _must_ be used for
> values <= ABS_MAX.
>
> IIUC this is true "only" for compatibility with older kernels right?
> If a program decides to support only newer kernels it can check the
> protocol version and use only the new ioctls, right?
>
> Maybe you can be more explicit about that in the comment?
See the comment on "struct input_absinfo2". It describes the API, this
comment just describes the ABS_* values. But if anyone has a better
phrase to use here, I will gladly adjust it.
Thanks
David
^ permalink raw reply
* [PATCH] input: rotary encoder: implement quarter period mode
From: Sascha Hauer @ 2013-12-18 14:43 UTC (permalink / raw)
To: linux-input; +Cc: Sascha Hauer, Dmitry Torokhov, Daniel Mack, devicetree
Some rotary encoders have a stable state in all output state
combinations. Add support for this type of encoder.
Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Daniel Mack <daniel@zonque.org>
Cc: linux-input@vger.kernel.org
Cc: devicetree@vger.kernel.org
---
.../devicetree/bindings/input/rotary-encoder.txt | 1 +
Documentation/input/rotary-encoder.txt | 9 +++++--
drivers/input/misc/rotary_encoder.c | 30 ++++++++++++++++++++--
include/linux/rotary_encoder.h | 1 +
4 files changed, 37 insertions(+), 4 deletions(-)
diff --git a/Documentation/devicetree/bindings/input/rotary-encoder.txt b/Documentation/devicetree/bindings/input/rotary-encoder.txt
index 3315495..cbdb29b 100644
--- a/Documentation/devicetree/bindings/input/rotary-encoder.txt
+++ b/Documentation/devicetree/bindings/input/rotary-encoder.txt
@@ -15,6 +15,7 @@ Optional properties:
- rotary-encoder,rollover: Automatic rollove when the rotary value becomes
greater than the specified steps or smaller than 0. For absolute axis only.
- rotary-encoder,half-period: Makes the driver work on half-period mode.
+- rotary-encoder,quarter-period: Makes the driver work on quarter-period mode.
See Documentation/input/rotary-encoder.txt for more information.
diff --git a/Documentation/input/rotary-encoder.txt b/Documentation/input/rotary-encoder.txt
index 92e68bc..0bbff7e 100644
--- a/Documentation/input/rotary-encoder.txt
+++ b/Documentation/input/rotary-encoder.txt
@@ -9,8 +9,10 @@ peripherals with two wires. The outputs are phase-shifted by 90 degrees
and by triggering on falling and rising edges, the turn direction can
be determined.
-Some encoders have both outputs low in stable states, whereas others also have
-a stable state with both outputs high (half-period mode).
+
+Some encoders have both outputs low in stable states, others also have
+a stable state with both outputs high (half-period mode) and some have
+a stable state in all steps (quarter-period mode).
The phase diagram of these two outputs look like this:
@@ -32,6 +34,9 @@ The phase diagram of these two outputs look like this:
|<-->|
one step (half-period mode)
+ |<>|
+ one step (quarter-period mode)
+
For more information, please see
http://en.wikipedia.org/wiki/Rotary_encoder
diff --git a/drivers/input/misc/rotary_encoder.c b/drivers/input/misc/rotary_encoder.c
index 5b1aff8..264501c 100644
--- a/drivers/input/misc/rotary_encoder.c
+++ b/drivers/input/misc/rotary_encoder.c
@@ -42,7 +42,7 @@ struct rotary_encoder {
bool armed;
unsigned char dir; /* 0 - clockwise, 1 - CCW */
- char last_stable;
+ int last_stable;
};
static int rotary_encoder_get_state(const struct rotary_encoder_platform_data *pdata)
@@ -117,6 +117,28 @@ static irqreturn_t rotary_encoder_irq(int irq, void *dev_id)
return IRQ_HANDLED;
}
+static irqreturn_t rotary_encoder_quarter_period_irq(int irq, void *dev_id)
+{
+ struct rotary_encoder *encoder = dev_id;
+ int state;
+ static const u8 states[4][4] = {
+ { -1, 1, 0, -1 },
+ { 0, -1, -1, 1 },
+ { 1, -1, -1, 0 },
+ { -1, 0, 1, -1 },
+ };
+
+ state = rotary_encoder_get_state(encoder->pdata);
+
+ encoder->dir = states[encoder->last_stable][state];
+ encoder->last_stable = state;
+
+ if (encoder->dir >= 0)
+ rotary_encoder_report_event(encoder);
+
+ return IRQ_HANDLED;
+}
+
static irqreturn_t rotary_encoder_half_period_irq(int irq, void *dev_id)
{
struct rotary_encoder *encoder = dev_id;
@@ -180,7 +202,8 @@ static struct rotary_encoder_platform_data *rotary_encoder_parse_dt(struct devic
"rotary-encoder,rollover", NULL);
pdata->half_period = !!of_get_property(np,
"rotary-encoder,half-period", NULL);
-
+ pdata->quarter_period = !!of_get_property(np,
+ "rotary-encoder,quarter-period", NULL);
return pdata;
}
#else
@@ -254,6 +277,9 @@ static int rotary_encoder_probe(struct platform_device *pdev)
if (pdata->half_period) {
handler = &rotary_encoder_half_period_irq;
encoder->last_stable = rotary_encoder_get_state(pdata);
+ } else if (pdata->quarter_period) {
+ handler = &rotary_encoder_quarter_period_irq;
+ encoder->last_stable = rotary_encoder_get_state(pdata);
} else {
handler = &rotary_encoder_irq;
}
diff --git a/include/linux/rotary_encoder.h b/include/linux/rotary_encoder.h
index 3f594dc..fd0a70f 100644
--- a/include/linux/rotary_encoder.h
+++ b/include/linux/rotary_encoder.h
@@ -11,6 +11,7 @@ struct rotary_encoder_platform_data {
bool relative_axis;
bool rollover;
bool half_period;
+ bool quarter_period;
};
#endif /* __ROTARY_ENCODER_H__ */
--
1.8.5.1
^ permalink raw reply related
* Re: [PATCH V2] input synaptics-rmi4: Bug fixes to ATTN GPIO handling.
From: Dmitry Torokhov @ 2013-12-18 14:39 UTC (permalink / raw)
To: Christopher Heiny
Cc: Linux Input, Andrew Duggan, Vincent Huang, Vivian Ly,
Daniel Rosenberg, Jean Delvare, Joerie de Gram, Linus Walleij,
Benjamin Tissoires
In-Reply-To: <1387311361-29411-1-git-send-email-cheiny@synaptics.com>
Hi Chris,
On Tue, Dec 17, 2013 at 12:16:01PM -0800, Christopher Heiny wrote:
> This patch fixes two bugs in handling of the RMI4 attention line GPIO.
>
> 1) in enable_sensor(), make sure the attn_gpio is defined before attempting to
> get its value.
>
> 2) in rmi_driver_probe(), declare the name of the attn_gpio, then
> request the attn_gpio before attempting to export it.
>
> Also introduces a GPIO_LABEL constant for identifying the attention GPIO.
>
I was looking at the patch some more and I have some concerns with it.
> Signed-off-by: Christopher Heiny <cheiny@synaptics.com>
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>
>
> ---
>
> drivers/input/rmi4/rmi_driver.c | 28 +++++++++++++++++-----------
> 1 file changed, 17 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/input/rmi4/rmi_driver.c b/drivers/input/rmi4/rmi_driver.c
> index a30c7d3..33fb8f8 100644
> --- a/drivers/input/rmi4/rmi_driver.c
> +++ b/drivers/input/rmi4/rmi_driver.c
> @@ -169,7 +169,7 @@ static int enable_sensor(struct rmi_device *rmi_dev)
>
> data->enabled = true;
>
> - if (!pdata->level_triggered &&
> + if (pdata->attn_gpio && !pdata->level_triggered &&
O is perfectly fine GPIO number, you want to use gpio_is_valid() hete. I
also wonder why do you need such elaborate check. Can we simply "flush"
device before enabling interrupts?
> gpio_get_value(pdata->attn_gpio) == pdata->attn_polarity)
> retval = process_interrupt_requests(rmi_dev);
>
> @@ -807,6 +807,8 @@ static int rmi_driver_remove(struct device *dev)
> return 0;
> }
>
> +static const char GPIO_LABEL[] = "attn";
> +
> static int rmi_driver_probe(struct device *dev)
> {
> struct rmi_driver *rmi_driver;
> @@ -959,20 +961,24 @@ static int rmi_driver_probe(struct device *dev)
> }
>
> if (IS_ENABLED(CONFIG_RMI4_DEV) && pdata->attn_gpio) {
> - retval = gpio_export(pdata->attn_gpio, false);
> + retval = gpio_request(pdata->attn_gpio, GPIO_LABEL);
Here it is too late to request GPIO. You have been converting it to IRQ,
enabling that IRQ and calling gpio_get_value() so GPIO should have
already been requested by now.
So you need to move this code up. You may also consider using
gpio_request_one() and use GPIOF_EXPORT flag if you want to export it.
It would also be nice to set the direction (GPIOF_DIR_IN).
I also do not see matching call to gpio_free() in remove().
> if (retval) {
> - dev_warn(dev, "WARNING: Failed to export ATTN gpio!\n");
> - retval = 0;
> + dev_warn(dev, "WARNING: Failed to request ATTN gpio %d, code: %d.\n",
> + pdata->attn_gpio, retval);
> } else {
> - retval = gpio_export_link(dev,
> - "attn", pdata->attn_gpio);
> + retval = gpio_export(pdata->attn_gpio, false);
> if (retval) {
> - dev_warn(dev,
> - "WARNING: Failed to symlink ATTN gpio!\n");
> - retval = 0;
> + dev_warn(dev, "WARNING: Failed to export ATTN gpio %d, code: %d.\n",
> + pdata->attn_gpio, retval);
> } else {
> - dev_info(dev, "Exported ATTN GPIO %d.",
> - pdata->attn_gpio);
> + retval = gpio_export_link(dev, GPIO_LABEL,
> + pdata->attn_gpio);
> + if (retval)
> + dev_warn(dev,
> + "WARNING: Failed to symlink ATTN gpio!\n");
> + else
> + dev_info(dev, "Exported ATTN GPIO %d.",
> + pdata->attn_gpio);
> }
> }
> }
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH 4/4] Input: add motion-tracking ABS_* bits and docs
From: Antonio Ospite @ 2013-12-18 14:29 UTC (permalink / raw)
To: David Herrmann
Cc: linux-input, Dmitry Torokhov, Jiri Kosina, Benjamin Tissoires,
Peter Hutterer, linux-kernel, input-tools
In-Reply-To: <1387295334-1744-5-git-send-email-dh.herrmann@gmail.com>
On Tue, 17 Dec 2013 16:48:54 +0100
David Herrmann <dh.herrmann@gmail.com> wrote:
> Motion sensors are getting quite common in mobile devices. To avoid
> returning accelerometer data via ABS_X/Y/Z and irritating the Xorg
> mouse-driver, this adds separate ABS_* bits for that.
>
> This is needed if gaming devices want to report their normal data plus
> accelerometer/gyro data. Usually, ABS_X/Y are already used by analog
> sticks, so need separate definitions, anyway.
What about:
...so motion sensors need separate definitions anyway.
^^^^^^^^^^^^^^
It was not immediately clear to me what the "need" referred to.
>
> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
> ---
> Documentation/input/gamepad.txt | 7 ++
> Documentation/input/motion-tracking.txt | 149 ++++++++++++++++++++++++++++++++
> include/linux/mod_devicetable.h | 2 +-
> include/uapi/linux/input.h | 9 +-
> 4 files changed, 165 insertions(+), 2 deletions(-)
> create mode 100644 Documentation/input/motion-tracking.txt
>
> diff --git a/Documentation/input/gamepad.txt b/Documentation/input/gamepad.txt
> index 196dc42..eeda685 100644
> --- a/Documentation/input/gamepad.txt
> +++ b/Documentation/input/gamepad.txt
> @@ -57,6 +57,9 @@ Most gamepads have the following features:
> - Rumble
> Many devices provide force-feedback features. But are mostly just
> simple rumble motors.
> + - Motion-tracking
> + Gamepads may include motion-tracking sensors like accelerometers and
> + gyroscopes.
>
> 3. Detection
> ~~~~~~~~~~~~
> @@ -153,5 +156,9 @@ Menu-Pad:
> Rumble:
> Rumble is adverticed as FF_RUMBLE.
>
> +Motion-tracking:
> + Motion-tracking is defined in ./Documentation/input/motion-tracking.txt and
> + gamepads shall comply to the rules defined there.
> +
./Documentation/... looks like it's relative to this document, drop
the leading ./
> ----------------------------------------------------------------------------
> Written 2013 by David Herrmann <dh.herrmann@gmail.com>
> diff --git a/Documentation/input/motion-tracking.txt b/Documentation/input/motion-tracking.txt
> new file mode 100644
> index 0000000..0885c9a
> --- /dev/null
> +++ b/Documentation/input/motion-tracking.txt
> @@ -0,0 +1,149 @@
> + Motion Tracking API
> +----------------------------------------------------------------------------
> +
> +1. Intro
> +~~~~~~~~
> +Motion tracking devices produce device motion events generated from an
> +accelerometer, gyroscope or compass. This data can be returned to user-space
> +via input events. This document defines how this data is reported.
> +
> +2. Devices
> +~~~~~~~~~~
> +In this document, a "device" is one of:
> + - accelerometer
> + - gyroscope
> + - compass
> +
> +These devices returned their information via different APIs in the past. To
> +unify them and define a common API, a set of input evdev codes was created. Old
> +drivers might continue using their API, but developers are encouraged to use
> +the input evdev API for new drivers.
> +
> +2.1 Axes
> +~~~~~~~~
> +Movement data is usually returned as absolute data for the 3 axes of a device.
> +In this context, the three axes are defined as:
> + - X: Axis goes from the left to the right side of the device
> + - Y: Axis goes from the bottom to the top of the device
> + - Z: Axis goes from the back to the front of the device
> +
> +The front of a device is the side faced to the user. For a mobile-phone it
> +would be the screen. For devices without a screen, the front is usually the
> +side with the most buttons on it.
> +
> + Example: Mobile-Phone
> + +-------------------------------------------------------------------------+
> + | TOP |
> + | |
> + | |
> + | +---------------------------+ |
> + | |\ ________________________ \ .__ |
> + | \ \ \ \ \ |\ |
> + | \ \ \ __ \ \ \ RIGHT|
> + | \ \ \ /| \ \ \__ |
> + | \ \ \ __/ \ \ |\ |
> + | \ \ \ /| \ \ \ (Y Axis) |
> + | \ \ \ __/ (Z axis) \ \ \__ |
> + | \ \ \ /| \ \ |\ |
> + | LEFT \ \ \ / \ \ \ |
> + | \ \ \ FRONT \ \ \ |
> + | \ \ \ \ \ |
> + | \ \ \_______________________\ \ |
> + | \ \ ___ \ |
> + | /\ \ \__\ \ |
> + | __/ \ +---------------------------+ |
> + | /| \|___________________________| |
> + | / BACK |
> + | (X axis) |
> + | ------->------->------->-------> |
> + | |
> + | |
> + | BOTTOM |
> + +-------------------------------------------------------------------------+
> +
> +Rotation-data is reported as clock-wise rotation on an axis. For a given axes,
> +the reported rotation would be:
> + ___
> + /|
> + / | (axis)
> + /
> + .-**-.
> + / / \
> + | / \ | /
> + \ / \|/ (clock-wise rotation)
> + /
> + /
> + /
> +
> +2.2 Calibration
> +~~~~~~~~~~~~~~~
> +Motion sensors are often highly sensitive and need precise calibration. Users
> +are adviced to perform neutral-point calibration themselves or to implement a
^^^^^^^
advised
> +state-machine to normalize input data automatically.
> +
> +Kernel devices may perform their own calibration and/or normalization. However,
> +this is usually sparse and, if implemented, transparent to the user.
> +
> +There is currently no way to feed calibration data into the kernel in a generic
> +way. Proposals welcome!
> +
> +2.3 Units
> +~~~~~~~~~
> +(NOTE: This section describes an experimental API. Currently, no device complies
> +to these rules so this might change in the future.)
> +
> +Reported data shall be returned as:
> + - Acceleration: 1/1000 m per s^2
Do you think it is worth mentioning that most hardware tend to report
the values as relative to some multiple of G (9.8 m/s^2)?
Or maybe we can get back to that when some actual driver (Wiimote, PS3
controller) start using that part of the API.
> + - Rotation: 1/1000 degree per second
> +
> +However, for most devices the reported units are unknown (more precisely: no
> +one has the time to measure them and figure them out). Therefore, user-space
> +shall use abs-minimum and abs-maximum to calculate relative data and use that
> +instead. Devices which return wrong units may be fixed in the future to comply
> +to these rules.
> +
> +3.1 Accelerometer
> +~~~~~~~~~~~~~~~~~
> +Accelerometers measure movement acceleration of devices. Any combination of the
> +three available axes can be used. Usually, all three are supported.
> +
> +Data is provided as absolute acceleration. A positive integer defines the
> +acceleration in the direction of an axis. A negative integer defines
> +acceleration in the opposite direction.
> +
> +The evdev ABS codes used are:
> + - ABS_ACCEL_X: X axis
> + - ABS_ACCEL_Y: Y axis
> + - ABS_ACCEL_Z: Z axis
> +
> +3.2 Gyroscope
> +~~~~~~~~~~~~~
> +A gyroscope measures rotational speed (*not* acceleration!). Any combination of
> +the three available axes can be used. Usually, all three are supported.
> +
> +Data is provided as absolute speed. A positive integer defines the rotational
> +speed in clock-wise order around a given axis. A negative integer defines it in
> +counter-clock-wise order.
> +
> +The evdev ABS codes used are:
> + - ABS_GYRO_X: X axis (also: Pitch)
> + - ABS_GYRO_Y: Y axis (also: Roll)
> + - ABS_GYRO_Z: Z axis (also: Azimuth/Yaw)
> +
> +3.3 Compass
> +~~~~~~~~~~~
> +(NOTE: No compass device currently uses the evdev input subsystem. Thus, this
> +API is only a proposal, it hasn't been implemented, yet.)
> +
> +A compass measures the ambient magnetic field of the three defined axes. This
> +makes the data self-contained and independent of the current device position.
> +Any combination of the three axes can be used. Usually all three are supported,
> +otherwise, it's not really useful as a compass.
> +
> +Proposed evdev ABS codes are:
> + - ABS_COMPASS_X: X axis
> + - ABS_COMPASS_Y: Y axis
> + - ABS_COMPASS_Z: Z axis
> +
> +----------------------------------------------------------------------------
> + Written 2013 by David Herrmann <dh.herrmann@gmail.com>
> diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
> index 45e9214..329aa30 100644
> --- a/include/linux/mod_devicetable.h
> +++ b/include/linux/mod_devicetable.h
> @@ -277,7 +277,7 @@ struct pcmcia_device_id {
> #define INPUT_DEVICE_ID_KEY_MIN_INTERESTING 0x71
> #define INPUT_DEVICE_ID_KEY_MAX 0x2ff
> #define INPUT_DEVICE_ID_REL_MAX 0x0f
> -#define INPUT_DEVICE_ID_ABS_MAX 0x3f
> +#define INPUT_DEVICE_ID_ABS_MAX 0x4f
> #define INPUT_DEVICE_ID_MSC_MAX 0x07
> #define INPUT_DEVICE_ID_LED_MAX 0x0f
> #define INPUT_DEVICE_ID_SND_MAX 0x07
> diff --git a/include/uapi/linux/input.h b/include/uapi/linux/input.h
> index 1856461..e4c3596 100644
> --- a/include/uapi/linux/input.h
> +++ b/include/uapi/linux/input.h
> @@ -869,12 +869,19 @@ struct input_keymap_entry {
> #define ABS_MAX 0x3f
> #define ABS_CNT (ABS_MAX+1)
>
> +#define ABS_ACCEL_X 0x40 /* Accelerometer X axis */
> +#define ABS_ACCEL_Y 0x41 /* Accelerometer Y axis */
> +#define ABS_ACCEL_Z 0x42 /* Accelerometer Z axis */
> +#define ABS_GYRO_X 0x43 /* Gyroscope X axis */
> +#define ABS_GYRO_Y 0x44 /* Gyroscope Y axis */
> +#define ABS_GYRO_Z 0x45 /* Gyroscope Z axis */
> +
> /*
> * Due to API restrictions the legacy evdev API only supports ABS values up to
> * ABS_MAX/CNT. Use the extended *ABS2 ioctls to operate on any ABS values in
> * between ABS_MAX and ABS_MAX2.
> */
> -#define ABS_MAX2 0x3f
> +#define ABS_MAX2 0x4f
> #define ABS_CNT2 (ABS_MAX2+1)
>
> /*
> --
> 1.8.5.1
>
Thanks,
Antonio
--
Antonio Ospite
http://ao2.it
A: Because it messes up the order in which people normally read text.
See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?
^ permalink raw reply
* Re: [PATCH 2/4] Input: introduce ABS_MAX2/CNT2 and friends
From: Antonio Ospite @ 2013-12-18 14:27 UTC (permalink / raw)
To: David Herrmann
Cc: linux-input, Dmitry Torokhov, Jiri Kosina, Benjamin Tissoires,
Peter Hutterer, linux-kernel, input-tools
In-Reply-To: <1387295334-1744-3-git-send-email-dh.herrmann@gmail.com>
Hi David,
thanks for the update.
On Tue, 17 Dec 2013 16:48:52 +0100
David Herrmann <dh.herrmann@gmail.com> wrote:
> As we painfully noticed during the 3.12 merge-window our
> EVIOCGABS/EVIOCSABS API is limited to ABS_MAX<=0x3f. We tried several
> hacks to work around it but if we ever decide to increase ABS_MAX, the
> EVIOCSABS ioctl ABI might overflow into the next byte causing horrible
> misinterpretations in the kernel that we cannot catch.
>
> Therefore, we decided to go with ABS_MAX2/CNT2 and introduce two new
> ioctls to get/set abs-params. They no longer encode the ABS code in the
> ioctl number and thus allow up to 4 billion ABS codes.
>
Just a question: did you consider the possibility of not exposing _MAX2
and _CNT2 in a header file at all but let those be queried? Or maybe
this is not necessary if we assume that userspace programs are supposed
to know what is the MAX _they_ support?
BTW I was thinking for instance to a program compiled with ABS_MAX2 =
0x4f but working with future kernels with an increased value, it still
won't be able to support values greater than 0x4f.
> The new API also allows to query multiple ABS values with one call. To
> allow EVIOCSABS2(code = 0, cnt = ABS_CNT2) we need to silently ignore
> writes to ABS_MT_SLOT. Furthermore, for better compatibility with
> newer user-space, we ignore writes to unknown codes. Hence, if we ever
> increase ABS_MAX2, new user-space will work with code=0,cnt=ABS_CNT2 just
> fine even on old kernels.
>
To my inexperienced eye it's not obvious why ABS_MT_SLOT must be handled
as a special case. Maybe because I don't even know what
code=0,cnt=ABS_CNT2 is used for.
> Note that we also need to increase EV_VERSION so user-space can reliably
> know whether ABS2 is supported. Unfortunately, we return EINVAL instead of
> ENOSYS for unknown evdev ioctls so it's nearly impossible to catch
> reliably without EVIOCGVERSION.
>
I'll check how to use that in evtest.
Just one more comment below.
> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
> ---
> drivers/hid/hid-debug.c | 2 +-
> drivers/hid/hid-input.c | 2 +-
> drivers/input/evdev.c | 95 +++++++++++++++++++++++++++++++-
> drivers/input/input.c | 14 ++---
> drivers/input/keyboard/goldfish_events.c | 6 +-
> drivers/input/keyboard/hil_kbd.c | 2 +-
> drivers/input/misc/uinput.c | 6 +-
> include/linux/hid.h | 2 +-
> include/linux/input.h | 6 +-
> include/uapi/linux/input.h | 42 +++++++++++++-
> include/uapi/linux/uinput.h | 2 +-
> 11 files changed, 155 insertions(+), 24 deletions(-)
>
[...]
> diff --git a/include/uapi/linux/input.h b/include/uapi/linux/input.h
> index bd24470..1856461 100644
> --- a/include/uapi/linux/input.h
> +++ b/include/uapi/linux/input.h
[...]
>
> /*
> + * Due to API restrictions the legacy evdev API only supports ABS values up to
> + * ABS_MAX/CNT. Use the extended *ABS2 ioctls to operate on any ABS values in
> + * between ABS_MAX and ABS_MAX2.
> + */
> +#define ABS_MAX2 0x3f
> +#define ABS_CNT2 (ABS_MAX2+1)
> +
Maybe it's just my English, but when you say "between ABS_MAX and
ABS_MAX2" it sounds like the old protocol still _must_ be used for
values <= ABS_MAX.
IIUC this is true "only" for compatibility with older kernels right?
If a program decides to support only newer kernels it can check the
protocol version and use only the new ioctls, right?
Maybe you can be more explicit about that in the comment?
Thanks,
Antonio
--
Antonio Ospite
http://ao2.it
A: Because it messes up the order in which people normally read text.
See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?
^ permalink raw reply
* Re: [PATCH 5/9] Input: pixcir_i2c_ts: Get rid of pdata->attb_read_val()
From: Dmitry Torokhov @ 2013-12-18 14:20 UTC (permalink / raw)
To: Roger Quadros; +Cc: rydberg, jcbian, linux-input, linux-kernel, devicetree
In-Reply-To: <1387358480-8313-6-git-send-email-rogerq@ti.com>
On Wed, Dec 18, 2013 at 02:51:16PM +0530, Roger Quadros wrote:
> Get rid of the attb_read_val() platform hook. Instead,
> read the ATTB gpio directly from the driver.
>
> Fail if valid ATTB gpio is not provided by patform data.
Do you also need to define polarity?
>
> Signed-off-by: Roger Quadros <rogerq@ti.com>
> Acked-by: Mugunthan V N <mugunthanvnm@ti.com>
> ---
> drivers/input/touchscreen/pixcir_i2c_ts.c | 19 +++++++++++++++++--
> include/linux/input/pixcir_ts.h | 1 -
> 2 files changed, 17 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/input/touchscreen/pixcir_i2c_ts.c b/drivers/input/touchscreen/pixcir_i2c_ts.c
> index 3370fd9..a783d94 100644
> --- a/drivers/input/touchscreen/pixcir_i2c_ts.c
> +++ b/drivers/input/touchscreen/pixcir_i2c_ts.c
> @@ -91,11 +91,12 @@ static void pixcir_ts_poscheck(struct pixcir_i2c_ts_data *data)
> static irqreturn_t pixcir_ts_isr(int irq, void *dev_id)
> {
> struct pixcir_i2c_ts_data *tsdata = dev_id;
> + const struct pixcir_ts_platform_data *pdata = tsdata->chip;
>
> while (!tsdata->exiting) {
> pixcir_ts_poscheck(tsdata);
>
> - if (tsdata->chip->attb_read_val())
> + if (gpio_get_value(pdata->gpio_attb))
> break;
>
> msleep(20);
> @@ -301,8 +302,10 @@ static struct pixcir_ts_platform_data *pixcir_parse_dt(struct device *dev)
> return ERR_PTR(-ENOMEM);
>
> pdata->gpio_attb = of_get_named_gpio(np, "attb-gpio", 0);
> - if (!gpio_is_valid(pdata->gpio_attb))
> + if (!gpio_is_valid(pdata->gpio_attb)) {
> dev_err(dev, "Failed to get ATTB GPIO\n");
> + return ERR_PTR(-EINVAL);
> + }
>
> if (of_property_read_u32(np, "x-size", &pdata->x_size)) {
> dev_err(dev, "Failed to get x-size property\n");
> @@ -344,6 +347,11 @@ static int pixcir_i2c_ts_probe(struct i2c_client *client,
> } else if (!pdata) {
> dev_err(&client->dev, "platform data not defined\n");
> return -EINVAL;
> + } else {
> + if (!gpio_is_valid(pdata->gpio_attb)) {
> + dev_err(dev, "Invalid gpio_attb in pdata\n");
> + return -EINVAL;
> + }
> }
>
> tsdata = devm_kzalloc(dev, sizeof(*tsdata), GFP_KERNEL);
> @@ -380,6 +388,13 @@ static int pixcir_i2c_ts_probe(struct i2c_client *client,
>
> input_set_drvdata(input, tsdata);
>
> + error = devm_gpio_request_one(dev, pdata->gpio_attb,
> + GPIOF_DIR_IN, "pixcir_i2c_attb");
> + if (error) {
> + dev_err(dev, "Failed to request ATTB gpio\n");
> + return error;
> + }
> +
> error = devm_request_threaded_irq(dev, client->irq, NULL, pixcir_ts_isr,
> IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> client->name, tsdata);
> diff --git a/include/linux/input/pixcir_ts.h b/include/linux/input/pixcir_ts.h
> index f17c192..88ffdb50 100644
> --- a/include/linux/input/pixcir_ts.h
> +++ b/include/linux/input/pixcir_ts.h
> @@ -44,7 +44,6 @@ enum pixcir_int_mode {
> #define PIXCIR_INT_POL_HIGH (1UL << 2)
>
> struct pixcir_ts_platform_data {
> - int (*attb_read_val)(void);
> unsigned int x_size; /* X axis resolution */
> unsigned int y_size; /* Y axis resolution */
> int gpio_attb; /* GPIO connected to ATTB line */
> --
> 1.8.3.2
>
--
Dmitry
^ permalink raw reply
* Re: [PATCH 7/9] Input: pixcir_i2c_ts: Implement Type B Multi Touch reporting
From: Dmitry Torokhov @ 2013-12-18 14:18 UTC (permalink / raw)
To: Roger Quadros; +Cc: rydberg, jcbian, linux-input, linux-kernel, devicetree
In-Reply-To: <1387358480-8313-8-git-send-email-rogerq@ti.com>
On Wed, Dec 18, 2013 at 02:51:18PM +0530, Roger Quadros wrote:
> Some pixcir controllers e.g. tangoC family report finger IDs with
> the co-ordinates and are more suitable for Type-B MT protocol.
>
> Signed-off-by: Roger Quadros <rogerq@ti.com>
> Acked-by: Mugunthan V N <mugunthanvnm@ti.com>
> ---
> drivers/input/touchscreen/pixcir_i2c_ts.c | 202 +++++++++++++++++++++++-------
> 1 file changed, 155 insertions(+), 47 deletions(-)
>
> diff --git a/drivers/input/touchscreen/pixcir_i2c_ts.c b/drivers/input/touchscreen/pixcir_i2c_ts.c
> index ff68246..9e14415 100644
> --- a/drivers/input/touchscreen/pixcir_i2c_ts.c
> +++ b/drivers/input/touchscreen/pixcir_i2c_ts.c
> @@ -23,84 +23,173 @@
> #include <linux/slab.h>
> #include <linux/i2c.h>
> #include <linux/input.h>
> +#include <linux/input/mt.h>
> #include <linux/input/pixcir_ts.h>
> #include <linux/gpio.h>
> #include <linux/of.h>
> #include <linux/of_gpio.h>
> #include <linux/of_device.h>
>
> +#define MAX_FINGERS 5 /* Maximum supported by the driver */
> +
> struct pixcir_i2c_ts_data {
> struct i2c_client *client;
> struct input_dev *input;
> const struct pixcir_ts_platform_data *pdata;
> bool exiting;
> + u8 max_fingers; /* Maximum supported by the chip */
> };
>
> -static void pixcir_ts_poscheck(struct pixcir_i2c_ts_data *data)
> +static void pixcir_ts_typea_report(struct pixcir_i2c_ts_data *tsdata)
Hmm, I do not think we should keep Type A reports if we can do Type B.
The protocols are not new and userspace should be able to handle MT-B by
now.
> {
> - struct pixcir_i2c_ts_data *tsdata = data;
> + const struct pixcir_ts_platform_data *pdata = tsdata->pdata;
> u8 rdbuf[10], wrbuf[1] = { 0 };
> u8 touch;
> int ret;
>
> - ret = i2c_master_send(tsdata->client, wrbuf, sizeof(wrbuf));
> - if (ret != sizeof(wrbuf)) {
> - dev_err(&tsdata->client->dev,
> - "%s: i2c_master_send failed(), ret=%d\n",
> - __func__, ret);
> - return;
> - }
> + while (!tsdata->exiting) {
>
> - ret = i2c_master_recv(tsdata->client, rdbuf, sizeof(rdbuf));
> - if (ret != sizeof(rdbuf)) {
> - dev_err(&tsdata->client->dev,
> - "%s: i2c_master_recv failed(), ret=%d\n",
> - __func__, ret);
> - return;
> - }
> + ret = i2c_master_send(tsdata->client, wrbuf, sizeof(wrbuf));
> + if (ret != sizeof(wrbuf)) {
> + dev_err(&tsdata->client->dev,
> + "%s: i2c_master_send failed(), ret=%d\n",
> + __func__, ret);
> + return;
> + }
> +
> + ret = i2c_master_recv(tsdata->client, rdbuf, sizeof(rdbuf));
> + if (ret != sizeof(rdbuf)) {
> + dev_err(&tsdata->client->dev,
> + "%s: i2c_master_recv failed(), ret=%d\n",
> + __func__, ret);
> + return;
> + }
>
> - touch = rdbuf[0];
> - if (touch) {
> - u16 posx1 = (rdbuf[3] << 8) | rdbuf[2];
> - u16 posy1 = (rdbuf[5] << 8) | rdbuf[4];
> - u16 posx2 = (rdbuf[7] << 8) | rdbuf[6];
> - u16 posy2 = (rdbuf[9] << 8) | rdbuf[8];
> -
> - input_report_key(tsdata->input, BTN_TOUCH, 1);
> - input_report_abs(tsdata->input, ABS_X, posx1);
> - input_report_abs(tsdata->input, ABS_Y, posy1);
> -
> - input_report_abs(tsdata->input, ABS_MT_POSITION_X, posx1);
> - input_report_abs(tsdata->input, ABS_MT_POSITION_Y, posy1);
> - input_mt_sync(tsdata->input);
> -
> - if (touch == 2) {
> - input_report_abs(tsdata->input,
> - ABS_MT_POSITION_X, posx2);
> - input_report_abs(tsdata->input,
> - ABS_MT_POSITION_Y, posy2);
> + touch = rdbuf[0];
> + if (touch) {
> + u16 posx1 = (rdbuf[3] << 8) | rdbuf[2];
> + u16 posy1 = (rdbuf[5] << 8) | rdbuf[4];
> + u16 posx2 = (rdbuf[7] << 8) | rdbuf[6];
> + u16 posy2 = (rdbuf[9] << 8) | rdbuf[8];
> +
> + input_report_key(tsdata->input, BTN_TOUCH, 1);
> + input_report_abs(tsdata->input, ABS_X, posx1);
> + input_report_abs(tsdata->input, ABS_Y, posy1);
> +
> + input_report_abs(tsdata->input, ABS_MT_POSITION_X,
> + posx1);
> + input_report_abs(tsdata->input, ABS_MT_POSITION_Y,
> + posy1);
> input_mt_sync(tsdata->input);
> +
> + if (touch == 2) {
> + input_report_abs(tsdata->input,
> + ABS_MT_POSITION_X, posx2);
> + input_report_abs(tsdata->input,
> + ABS_MT_POSITION_Y, posy2);
> + input_mt_sync(tsdata->input);
> + }
> + } else {
> + input_report_key(tsdata->input, BTN_TOUCH, 0);
> }
> - } else {
> - input_report_key(tsdata->input, BTN_TOUCH, 0);
> - }
>
> - input_sync(tsdata->input);
> + input_sync(tsdata->input);
> +
> + if (gpio_get_value(pdata->gpio_attb))
> + break;
> +
> + msleep(20);
> + }
> }
>
> -static irqreturn_t pixcir_ts_isr(int irq, void *dev_id)
> +static void pixcir_ts_typeb_report(struct pixcir_i2c_ts_data *ts)
> {
> - struct pixcir_i2c_ts_data *tsdata = dev_id;
> - const struct pixcir_ts_platform_data *pdata = tsdata->pdata;
> + const struct pixcir_ts_platform_data *pdata = ts->pdata;
> + struct device *dev = &ts->client->dev;
> + u8 rdbuf[32], wrbuf[1] = { 0 };
> + u8 *bufptr;
> + u8 num_fingers;
> + u8 unreliable;
> + int ret, i;
> +
> + while (!ts->exiting) {
> +
> + ret = i2c_master_send(ts->client, wrbuf, sizeof(wrbuf));
> + if (ret != sizeof(wrbuf)) {
> + dev_err(dev, "%s: i2c_master_send failed(), ret=%d\n",
> + __func__, ret);
> + return;
> + }
>
> - while (!tsdata->exiting) {
> - pixcir_ts_poscheck(tsdata);
> + ret = i2c_master_recv(ts->client, rdbuf, sizeof(rdbuf));
> + if (ret != sizeof(rdbuf)) {
> + dev_err(dev, "%s: i2c_master_recv failed(), ret=%d\n",
> + __func__, ret);
> + return;
> + }
> +
> + unreliable = rdbuf[0] & 0xe0;
> +
> + if (unreliable)
> + goto next; /* ignore unreliable data */
> +
> + num_fingers = rdbuf[0] & 0x7;
> + bufptr = &rdbuf[2];
>
> + if (num_fingers > ts->max_fingers) {
> + num_fingers = ts->max_fingers;
> + dev_dbg(dev, "limiting num_fingers to %d\n",
> + num_fingers);
> + }
> +
> + for (i = 0; i < num_fingers; i++) {
> + u8 id;
> + unsigned int x, y;
> + int slot;
> +
> + id = bufptr[4];
> + slot = input_mt_get_slot_by_key(ts->input, id);
> + if (slot < 0) {
> + dev_dbg(dev, "no free slot for id 0x%x\n", id);
> + continue;
> + }
> +
> +
> + x = bufptr[1] << 8 | bufptr[0];
> + y = bufptr[3] << 8 | bufptr[2];
> +
> + input_mt_slot(ts->input, slot);
> + input_mt_report_slot_state(ts->input,
> + MT_TOOL_FINGER, true);
> +
> + input_event(ts->input, EV_ABS, ABS_MT_POSITION_X, x);
> + input_event(ts->input, EV_ABS, ABS_MT_POSITION_Y, y);
> +
> + bufptr = &bufptr[5];
> + dev_dbg(dev, "%d: id 0x%x slot %d, x %d, y %d\n",
> + i, id, slot, x, y);
> + }
> +
> + /* One frame is complete so sync it */
> + input_mt_sync_frame(ts->input);
> + input_sync(ts->input);
> +
> +next:
> if (gpio_get_value(pdata->gpio_attb))
> break;
>
> - msleep(20);
> + usleep_range(2000, 5000);
> }
> +}
> +
> +static irqreturn_t pixcir_ts_isr(int irq, void *dev_id)
> +{
> + struct pixcir_i2c_ts_data *tsdata = dev_id;
> +
> + if (tsdata->input->mt)
> + pixcir_ts_typeb_report(tsdata);
> + else
> + pixcir_ts_typea_report(tsdata);
>
> return IRQ_HANDLED;
> }
> @@ -376,9 +465,9 @@ static int pixcir_i2c_ts_probe(struct i2c_client *client,
> input->open = pixcir_input_open;
> input->close = pixcir_input_close;
>
> - __set_bit(EV_KEY, input->evbit);
> __set_bit(EV_ABS, input->evbit);
> __set_bit(BTN_TOUCH, input->keybit);
> +
> input_set_abs_params(input, ABS_X,
> 0, pdata->x_size - 1, 0, 0);
> input_set_abs_params(input, ABS_Y,
> @@ -388,6 +477,25 @@ static int pixcir_i2c_ts_probe(struct i2c_client *client,
> input_set_abs_params(input, ABS_MT_POSITION_Y,
> 0, pdata->y_size - 1, 0, 0);
>
> + /* Type-B Multi-Touch support */
> + if (pdata->chip.num_report_ids) {
> + const struct pixcir_i2c_chip_data *chip = &pdata->chip;
> +
> + tsdata->max_fingers = chip->num_report_ids;
> + if (tsdata->max_fingers > MAX_FINGERS) {
> + dev_info(dev, "Limiting maximum fingers to %d\n",
> + MAX_FINGERS);
> + tsdata->max_fingers = MAX_FINGERS;
> + }
> +
> + error = input_mt_init_slots(input, tsdata->max_fingers,
> + INPUT_MT_DIRECT | INPUT_MT_DROP_UNUSED);
> + if (error) {
> + dev_err(dev, "Error initializing Multi-Touch slots\n");
> + return error;
> + }
> + }
> +
> input_set_drvdata(input, tsdata);
>
> error = devm_gpio_request_one(dev, pdata->gpio_attb,
> --
> 1.8.3.2
>
--
Dmitry
^ permalink raw reply
* Re: [PATCH 4/9] Input: pixcir_i2c_ts: Use devres managed resource allocations
From: Dmitry Torokhov @ 2013-12-18 14:15 UTC (permalink / raw)
To: Roger Quadros; +Cc: rydberg, jcbian, linux-input, linux-kernel, devicetree
In-Reply-To: <1387358480-8313-5-git-send-email-rogerq@ti.com>
On Wed, Dec 18, 2013 at 02:51:15PM +0530, Roger Quadros wrote:
> Use devm_() and friends for allocating memory, input device
> and IRQ.
This one looks good, maybe have it first in the series when you
resubmit.
>
> Signed-off-by: Roger Quadros <rogerq@ti.com>
> Acked-by: Mugunthan V N <mugunthanvnm@ti.com>
> ---
> drivers/input/touchscreen/pixcir_i2c_ts.c | 35 ++++++++++++-------------------
> 1 file changed, 13 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/input/touchscreen/pixcir_i2c_ts.c b/drivers/input/touchscreen/pixcir_i2c_ts.c
> index ce8abcd..3370fd9 100644
> --- a/drivers/input/touchscreen/pixcir_i2c_ts.c
> +++ b/drivers/input/touchscreen/pixcir_i2c_ts.c
> @@ -346,12 +346,14 @@ static int pixcir_i2c_ts_probe(struct i2c_client *client,
> return -EINVAL;
> }
>
> - tsdata = kzalloc(sizeof(*tsdata), GFP_KERNEL);
> - input = input_allocate_device();
> - if (!tsdata || !input) {
> - dev_err(&client->dev, "Failed to allocate driver data!\n");
> - error = -ENOMEM;
> - goto err_free_mem;
> + tsdata = devm_kzalloc(dev, sizeof(*tsdata), GFP_KERNEL);
> + if (!tsdata)
> + return -ENOMEM;
> +
> + input = devm_input_allocate_device(dev);
> + if (!input) {
> + dev_err(&client->dev, "Failed to allocate input device\n");
> + return -ENOMEM;
> }
>
> tsdata->client = client;
> @@ -378,12 +380,12 @@ static int pixcir_i2c_ts_probe(struct i2c_client *client,
>
> input_set_drvdata(input, tsdata);
>
> - error = request_threaded_irq(client->irq, NULL, pixcir_ts_isr,
> + error = devm_request_threaded_irq(dev, client->irq, NULL, pixcir_ts_isr,
> IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> client->name, tsdata);
> if (error) {
> - dev_err(&client->dev, "Unable to request touchscreen IRQ.\n");
> - goto err_free_mem;
> + dev_err(dev, "failed to request irq %d\n", client->irq);
> + return error;
> }
>
> /* Always be in IDLE mode to save power, device supports auto wake */
> @@ -396,23 +398,16 @@ static int pixcir_i2c_ts_probe(struct i2c_client *client,
> /* Stop device till opened */
> error = pixcir_stop(tsdata);
> if (error)
> - goto err_free_irq;
> + return error;
>
> error = input_register_device(input);
> if (error)
> - goto err_free_irq;
> + return error;
>
> i2c_set_clientdata(client, tsdata);
> device_init_wakeup(&client->dev, 1);
>
> return 0;
> -
> -err_free_irq:
> - free_irq(client->irq, tsdata);
> -err_free_mem:
> - input_free_device(input);
> - kfree(tsdata);
> - return error;
> }
>
> static int pixcir_i2c_ts_remove(struct i2c_client *client)
> @@ -423,10 +418,6 @@ static int pixcir_i2c_ts_remove(struct i2c_client *client)
>
> tsdata->exiting = true;
> mb();
> - free_irq(client->irq, tsdata);
> -
> - input_unregister_device(tsdata->input);
> - kfree(tsdata);
>
> return 0;
> }
> --
> 1.8.3.2
>
--
Dmitry
^ permalink raw reply
* Re: [PATCH 3/9] Input: pixcir_i2c_ts: Initialize interrupt mode and power mode
From: Dmitry Torokhov @ 2013-12-18 14:14 UTC (permalink / raw)
To: Roger Quadros
Cc: rydberg-Hk7bIW8heu4wFerOooGFRg, jcbian-mY6CKx1T+M6Pt1CcHtbs0g,
linux-input-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1387358480-8313-4-git-send-email-rogerq-l0cyMroinI0@public.gmane.org>
On Wed, Dec 18, 2013 at 02:51:14PM +0530, Roger Quadros wrote:
> +
> +static int pixcir_stop(struct pixcir_i2c_ts_data *ts)
> +{
> + struct device *dev = &ts->client->dev;
> + int ret;
> +
> + /* disable interrupt generation */
> + ret = pixcir_int_enable(ts, 0);
> + if (ret) {
> + dev_err(dev, "Failed to disable interrupt generation\n");
> + return ret;
> + }
> +
> + disable_irq(ts->client->irq);
Why do you need to disable IRQ? If you disable interrupt generation in
the chip I think you only need to call synchronize_irq() to make sure
it's completed if it happens to be running. Also you need to move the
code:
tsdata->exiting = true;
mb();
here from pixcir_i2c_ts_remove() to make sure handler exits promptly.
You will also need to reset tsdata->exiting in your start method.
Thanks.
--
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox