* Re: [PATCH 1/7] HID: provide a helper for validating hid reports
From: Benjamin Tissoires @ 2013-09-09 12:33 UTC (permalink / raw)
To: Kees Cook
Cc: linux-input, Benjamin Tissoires, Jiri Kosina, Henrik Rydberg,
stable
In-Reply-To: <1378312645-27736-2-git-send-email-keescook@chromium.org>
On Wed, Sep 4, 2013 at 6:37 PM, Kees Cook <keescook@chromium.org> wrote:
> Many drivers need to validate the characteristics of their HID report
> during initialization to avoid misusing the reports. This adds a common
> helper to perform validation of the report exisitng, the field existing,
> and the expected number of values within the field.
>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> Cc: stable@kernel.org
>
> ---
Thanks for the v2.
I'm happy with this one.
Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
Cheers,
Benjamin
^ permalink raw reply
* [PATCH] hid-elo: some systems cannot stomach work around
From: oliver @ 2013-09-09 10:16 UTC (permalink / raw)
To: linux-usb, linux-input, jkosina, jslaby; +Cc: Oliver Neukum
From: Oliver Neukum <oneukum@suse.de>
Some systems although they have firmware class 'M', which usually
needs a work around to not crash, must not be subjected to the
work around because the work around crashes them. They cannot be
told apart by their own device descriptor, but as they are part
of compound devices can be identified by looking at their siblings.
Signed-off-by: Oliver Neukum <oneukum@suse.de>
---
drivers/hid/hid-elo.c | 35 ++++++++++++++++++++++++++++++++++-
1 file changed, 34 insertions(+), 1 deletion(-)
diff --git a/drivers/hid/hid-elo.c b/drivers/hid/hid-elo.c
index f042a6c..64ac53e 100644
--- a/drivers/hid/hid-elo.c
+++ b/drivers/hid/hid-elo.c
@@ -181,7 +181,40 @@ fail:
*/
static bool elo_broken_firmware(struct usb_device *dev)
{
- return use_fw_quirk && le16_to_cpu(dev->descriptor.bcdDevice) == 0x10d;
+ struct usb_device *hub = dev->parent;
+ struct usb_device *child = NULL;
+ u16 fw_lvl = le16_to_cpu(dev->descriptor.bcdDevice);
+ u16 child_vid, child_pid;
+ int i;
+
+ if (!use_fw_quirk)
+ return false;
+ if (fw_lvl != 0x10d)
+ return false;
+
+ /*iterate sibling devices of the touch controller*/
+ usb_hub_for_each_child(hub, i, child) {
+ child_vid = le16_to_cpu(child->descriptor.idVendor);
+ child_pid = le16_to_cpu(child->descriptor.idProduct);
+
+ /*
+ * If one of the devices below is present attached as a sibling of
+ * the touch controller then this is a newer IBM 4820 monitor that
+ * does not need the IBM-requested workaround if fw level is
+ * 0x010d - aka 'M'.
+ * No other HW can have this combination.
+ */
+ if (child_vid==0x04b3) {
+ switch (child_pid) {
+ case 0x4676: /*4820 21x Video*/
+ case 0x4677: /*4820 51x Video*/
+ case 0x4678: /*4820 2Lx Video*/
+ case 0x4679: /*4820 5Lx Video*/
+ return false;
+ }
+ }
+ }
+ return true;
}
static int elo_probe(struct hid_device *hdev, const struct hid_device_id *id)
--
1.8.3.1
^ permalink raw reply related
* Re: [RFC] surface-input
From: David Herrmann @ 2013-09-09 9:35 UTC (permalink / raw)
To: Florian Echtler
Cc: Benjamin Tissoires, linux-input, Henrik Rydberg, Dmitry Torokhov
In-Reply-To: <522D860E.4020909@butterbrot.org>
Hi
On Mon, Sep 9, 2013 at 10:25 AM, Florian Echtler <floe@butterbrot.org> wrote:
> On 06.09.2013 15:25, Benjamin Tissoires wrote:
>> Some generic comments:
>> - please always inline the code in the message, it is *much* easier to review and comment it
>> - please directly use a patch format: if the code is good, Dmitry can take it directly through his tree
>> - add the following people for further submissions:
>> * Henrik - multitouch maintainer in the kernel, and I'm sure he will be happy to see this stuff
>> * Dmitry - input maintainer, the driver will go through his tree, so it's better to let him know as soon as possible of the different discussions.
>> - please stick to the kernel formatting guidelines (without orders: do not use C99-style ("// ..."), do not mix tabs and spaces, stick to 80 columns, etc..). The whole documentation is in Documentation/CodingStyle, and use the script scripts/checkpatch.pl to validate most of these.
>> - I don't think a separate ".h" will be accepted as the declarations will not be used outside of the driver. Just merge the header on top of you .c file.
>
> Benjamin and David, thanks for your feedback. I will integrate this and
> get back to you with a proper patch ASAP.
>
> I have one additional question which is more about "future-proofing"
> this driver. The SUR40 hardware does also provide a raw video image from
> the touch sensor over another USB _endpoint_. Since it's not a different
> _interface_, this can only be handled in the same driver as far as I can
> tell. At some point in the future, I would like to add a V4L2 interface
> to also support the video endpoint - are there any issues I should
> already try to watch out for in the input part?
Media drivers use input-devices to report input data. So no, there is
no reason to change your driver's design. Once you want
media-functionality, you simply add a v4l2 device during hid_probe()
and remove it in hid_remove(). User-space can see the relation between
both via sysfs.
It is never wrong to think about the API _now_. But as long as it's
only a single input-dev and one v4l2-dev, I'd recommend to put both
below your usb-device and you're fine.
Regarding internal driver-design, you should not bother now. Make it
work for input and make it work well.
Cheers
David
^ permalink raw reply
* Re: [RFC] surface-input
From: Florian Echtler @ 2013-09-09 8:25 UTC (permalink / raw)
To: Benjamin Tissoires, David Herrmann
Cc: linux-input, Henrik Rydberg, Dmitry Torokhov
In-Reply-To: <5229D7CF.4090401@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1712 bytes --]
On 06.09.2013 15:25, Benjamin Tissoires wrote:
> Some generic comments:
> - please always inline the code in the message, it is *much* easier to review and comment it
> - please directly use a patch format: if the code is good, Dmitry can take it directly through his tree
> - add the following people for further submissions:
> * Henrik - multitouch maintainer in the kernel, and I'm sure he will be happy to see this stuff
> * Dmitry - input maintainer, the driver will go through his tree, so it's better to let him know as soon as possible of the different discussions.
> - please stick to the kernel formatting guidelines (without orders: do not use C99-style ("// ..."), do not mix tabs and spaces, stick to 80 columns, etc..). The whole documentation is in Documentation/CodingStyle, and use the script scripts/checkpatch.pl to validate most of these.
> - I don't think a separate ".h" will be accepted as the declarations will not be used outside of the driver. Just merge the header on top of you .c file.
Benjamin and David, thanks for your feedback. I will integrate this and
get back to you with a proper patch ASAP.
I have one additional question which is more about "future-proofing"
this driver. The SUR40 hardware does also provide a raw video image from
the touch sensor over another USB _endpoint_. Since it's not a different
_interface_, this can only be handled in the same driver as far as I can
tell. At some point in the future, I would like to add a V4L2 interface
to also support the video endpoint - are there any issues I should
already try to watch out for in the input part?
Thanks again, and best regards, Florian
--
SENT FROM MY DEC VT50 TERMINAL
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply
* [PATCH] Input: cypress_ps2 - Remove casting the return value which is a void pointer
From: Jingoo Han @ 2013-09-09 5:37 UTC (permalink / raw)
To: 'Dmitry Torokhov'
Cc: 'Dmitry Torokhov', linux-input, 'Dudley Du',
'Jingoo Han'
Casting the return value which is a void pointer is redundant.
The conversion from void pointer to any other pointer type is
guaranteed by the C programming language.
Signed-off-by: Jingoo Han <jg1.han@samsung.com>
---
drivers/input/mouse/cypress_ps2.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/input/mouse/cypress_ps2.c b/drivers/input/mouse/cypress_ps2.c
index f51765f..ef651cc 100644
--- a/drivers/input/mouse/cypress_ps2.c
+++ b/drivers/input/mouse/cypress_ps2.c
@@ -679,7 +679,7 @@ int cypress_init(struct psmouse *psmouse)
{
struct cytp_data *cytp;
- cytp = (struct cytp_data *)kzalloc(sizeof(struct cytp_data), GFP_KERNEL);
+ cytp = kzalloc(sizeof(struct cytp_data), GFP_KERNEL);
psmouse->private = (void *)cytp;
if (cytp == NULL)
return -ENOMEM;
--
1.7.10.4
^ permalink raw reply related
* Re: [PATCH 2/2] iio: ti_am335x_adc: Add continuous sampling support
From: Jonathan Cameron @ 2013-09-08 13:42 UTC (permalink / raw)
To: Zubair Lutfullah
Cc: dmitry.torokhov-Re5JQEeQqe8AvxtiuMwx3w,
linux-iio-u79uwXL29TY76Z2rM5mHXA,
linux-input-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
bigeasy-hfZtesqFncYOwBW4kG4KsQ,
gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r
In-Reply-To: <1378034277-26728-3-git-send-email-zubair.lutfullah-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
On 09/01/13 12:17, Zubair Lutfullah wrote:
> Previously the driver had only one-shot reading functionality.
> This patch adds triggered buffer support to the driver.
>
> Continuous sampling starts when buffer is enabled.
> And samples are pushed to userpace by the trigger which
> triggers automatically at every hardware interrupt
> of FIFO1 filling with samples upto threshold value.
>
> Userspace responsibility to stop sampling by writing zero
> in the buffer enable file.
>
> Patil Rachna (TI) laid the ground work for ADC HW register access.
> Russ Dill (TI) fixed bugs in the driver relevant to FIFOs and IRQs.
>
> I fixed channel scanning so multiple ADC channels can be read
> simultaneously and pushed to userspace.
> Restructured the driver to fit IIO ABI.
> And added trigger support.
>
> Signed-off-by: Zubair Lutfullah <zubair.lutfullah-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Acked-by: Greg Kroah-Hartman <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org>
> Signed-off-by: Russ Dill <Russ.Dill-l0cyMroinI0@public.gmane.org>
> Acked-by: Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> Acked-by: Sebastian Andrzej Siewior <bigeasy-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
Hi
I'm afraid I'm still unconvinced that we can use the trigger infrastructure
as done here. This trigger from a fifo threshold does not agree with how it
happens in any other driver. Unfortunately as the first hardware buffered
device we have had in recent times (there is the sca3000 in staging, but that
does not have a front end kfifo) you get to break some new ground.
I've suggested below how I would handle this. In short, drop the trigger
and drive the buffers directly, marking the mode as INDIO_BUFFER_HARDWARE to
avoid the core code complaining at the lack of trigger. It will end up simpler
and not expose a trigger to userspace that is doing 'interesting' things.
This is the only way I can envision a hardware buffer equiped device like this
working. It's certainly what I've been intending to do with the sca3000
driver when I finally get around to bringing it up to date. I don't think
we can take the current approach because it would give us horribly inconsistent
userspace interfaces we can't maintain in the long run.
Few other little bits and bobs inline to do with unused variables and
a few places where a small comment or two might make the operation easier
to follow when we have all forgotten how the hardware works ;)
Jonathan
> ---
> drivers/iio/adc/Kconfig | 1 +
> drivers/iio/adc/ti_am335x_adc.c | 243 +++++++++++++++++++++++++++++++---
> include/linux/mfd/ti_am335x_tscadc.h | 13 ++
> 3 files changed, 237 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 09371cb..cfa2039 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -167,6 +167,7 @@ config TI_ADC081C
> config TI_AM335X_ADC
> tristate "TI's AM335X ADC driver"
> depends on MFD_TI_AM335X_TSCADC
> + select IIO_TRIGGERED_BUFFER
> help
> Say yes here to build support for Texas Instruments ADC
> driver which is also a MFD client.
> diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
> index a952538..1626e16 100644
> --- a/drivers/iio/adc/ti_am335x_adc.c
> +++ b/drivers/iio/adc/ti_am335x_adc.c
> @@ -24,16 +24,23 @@
> #include <linux/iio/iio.h>
> #include <linux/of.h>
> #include <linux/of_device.h>
Ouch, how did that slip in here. Dropping this should have been
in a separate patch as it doesn't really have anything to do with
the rest of this series.
> -#include <linux/iio/machine.h>
> #include <linux/iio/driver.h>
>
> #include <linux/mfd/ti_am335x_tscadc.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/trigger.h>
> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/triggered_buffer.h>
>
> struct tiadc_device {
> struct ti_tscadc_dev *mfd_tscadc;
> int channels;
> u8 channel_line[8];
> u8 channel_step[8];
> + int irq;
> + int buffer_en_ch_steps;
> + struct iio_trigger *trig;
> + u32 *data;
> };
>
> static unsigned int tiadc_readl(struct tiadc_device *adc, unsigned int reg)
> @@ -56,10 +63,16 @@ static u32 get_adc_step_mask(struct tiadc_device *adc_dev)
> return step_en;
> }
>
> -static void tiadc_step_config(struct tiadc_device *adc_dev)
> +static u32 get_adc_step_bit(struct tiadc_device *adc_dev, int chan)
> {
> + return 1 << adc_dev->channel_step[chan];
> +}
> +
> +static void tiadc_step_config(struct iio_dev *indio_dev)
> +{
> + struct tiadc_device *adc_dev = iio_priv(indio_dev);
> unsigned int stepconfig;
> - int i, steps;
> + int i, steps, chan;
Whilst I agree that pulling chan up here is slightly cleaner, technically that
is a bit of unconnected cleanup and should have been in a precursor patch.
(though don't bother doing so now!)
>
> /*
> * There are 16 configurable steps and 8 analog input
> @@ -72,11 +85,13 @@ static void tiadc_step_config(struct tiadc_device *adc_dev)
> */
>
> steps = TOTAL_STEPS - adc_dev->channels;
> - stepconfig = STEPCONFIG_AVG_16 | STEPCONFIG_FIFO1;
> + if (iio_buffer_enabled(indio_dev))
> + stepconfig = STEPCONFIG_AVG_16 | STEPCONFIG_FIFO1
> + | STEPCONFIG_MODE_SWCNT;
> + else
> + stepconfig = STEPCONFIG_AVG_16 | STEPCONFIG_FIFO1;
>
> for (i = 0; i < adc_dev->channels; i++) {
> - int chan;
> -
> chan = adc_dev->channel_line[i];
> tiadc_writel(adc_dev, REG_STEPCONFIG(steps),
> stepconfig | STEPCONFIG_INP(chan));
> @@ -85,7 +100,167 @@ static void tiadc_step_config(struct tiadc_device *adc_dev)
> adc_dev->channel_step[i] = steps;
> steps++;
> }
> +}
> +
> +static irqreturn_t tiadc_irq(int irq, void *private)
> +{
> + struct iio_dev *indio_dev = private;
> + struct tiadc_device *adc_dev = iio_priv(indio_dev);
> + unsigned int status, config;
> + status = tiadc_readl(adc_dev, REG_IRQSTATUS);
> +
> + /*
> + * ADC and touchscreen share the IRQ line.
> + * FIFO0 interrupts are used by TSC. Handle FIFO1 IRQs here only
> + */
> + if (status & IRQENB_FIFO1OVRRUN) {
> + /* FIFO Overrun. Clear flag. Disable/Enable ADC to recover */
> + config = tiadc_readl(adc_dev, REG_CTRL);
> + config &= ~(CNTRLREG_TSCSSENB);
> + tiadc_writel(adc_dev, REG_CTRL, config);
> + tiadc_writel(adc_dev, REG_IRQSTATUS, IRQENB_FIFO1OVRRUN
> + | IRQENB_FIFO1UNDRFLW | IRQENB_FIFO1THRES);
> + tiadc_writel(adc_dev, REG_CTRL, (config | CNTRLREG_TSCSSENB));
> + return IRQ_HANDLED;
> + } else if (status & IRQENB_FIFO1THRES) {
> + /* Trigger to push FIFO data to iio buffer */
> + tiadc_writel(adc_dev, REG_IRQCLR, IRQENB_FIFO1THRES);
So this is triggering at the threshold. Thus it isn't acting as a conventional
trigger in the IIO sense at all. It is of no real use to anything outside
this driver, so at the very least add the two callbacks to prevent this
driver using any other trigger and the trigger being used by any other device.
However, I'm really not happy with this solution.
Take a look at running with no trigger at all and calling the buffer
filling more directly. The issue is that if we have a trigger now it will
become part of the userspace interface and become a pain to drop later.
It is valid here I think to use the INDIO_BUFFER_HARDWARE mode (which skips
the checks for a trigger) then just don't call iio_triggered_buffer_post_enable
or iio_triggered_buffer_predisable. Then make this a threaded interrupt and
put your tiadc_trigger_h as the thread. Finally just return IRQ_WAKE_THREAD
in this case. You'll have to pull a few bits out of
industrialio-triggered-buffer.c to create the kfifo etc, but no more than
a few lines given the pollfunc won't exist.
Actually, might be cleaner to move the flush case into the thread as well
and just have a single check in this top half of the handler. That bit
is up to you given it is really a question of how long it take and hence
if it is acceptable in interrupt context.
> + iio_trigger_poll(indio_dev->trig, iio_get_time_ns());
> + return IRQ_HANDLED;
> + } else
> + return IRQ_NONE;
> +
> +}
> +
> +static irqreturn_t tiadc_trigger_h(int irq, void *p)
> +{
> + struct iio_poll_func *pf = p;
> + struct iio_dev *indio_dev = pf->indio_dev;
> + struct tiadc_device *adc_dev = iio_priv(indio_dev);
> + int i, k, fifo1count, read;
> + u32 *data = adc_dev->data;
> +
Hmm.. This results in all data in the fifo being read from a single
trigger. I think I would still be happier if in the case of hardware
buffers like this we didn't use a visible trigger at all. Such
a trigger has a somewhat fuzzy meaning to say the least.
> + fifo1count = tiadc_readl(adc_dev, REG_FIFO1CNT);
> + for (k = 0; k < fifo1count; k = k + i) {
As mentioned below, why not use 16 bit storage in the kfifo?
> + for (i = 0; i < (indio_dev->scan_bytes)/4; i++) {
> + read = tiadc_readl(adc_dev, REG_FIFO1);
> + data[i] = read & FIFOREAD_DATA_MASK;
> + }
> + iio_push_to_buffers(indio_dev, (u8 *) data);
> + }
>
> + tiadc_writel(adc_dev, REG_IRQSTATUS, IRQENB_FIFO1THRES);
> + tiadc_writel(adc_dev, REG_IRQENABLE, IRQENB_FIFO1THRES);
> +
> + iio_trigger_notify_done(indio_dev->trig);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static int tiadc_buffer_preenable(struct iio_dev *indio_dev)
> +{
> + struct tiadc_device *adc_dev = iio_priv(indio_dev);
> + int i, fifo1count, read;
> +
> + tiadc_writel(adc_dev, REG_IRQCLR, (IRQENB_FIFO1THRES |
> + IRQENB_FIFO1OVRRUN |
> + IRQENB_FIFO1UNDRFLW));
> +
Why would there be data in the fifo? Seems to me that flushing
after disabling it and before enabling it should not both be
necessary? Is this just a case of playing safe?
> + /* Flush FIFO before starting sampling */
> + fifo1count = tiadc_readl(adc_dev, REG_FIFO1CNT);
> + for (i = 0; i < fifo1count; i++)
> + read = tiadc_readl(adc_dev, REG_FIFO1);
> +
> + return iio_sw_buffer_preenable(indio_dev);
> +}
> +
> +static int tiadc_buffer_postenable(struct iio_dev *indio_dev)
> +{
> + struct tiadc_device *adc_dev = iio_priv(indio_dev);
> + struct iio_buffer *buffer = indio_dev->buffer;
> + unsigned int enb = 0;
> + u8 bit;
> +
> + adc_dev->data = kmalloc(indio_dev->scan_bytes, GFP_KERNEL);
> + if (adc_dev->data == NULL)
> + return -ENOMEM;
> +
> + tiadc_step_config(indio_dev);
> + for_each_set_bit(bit, buffer->scan_mask, adc_dev->channels)
> + enb |= (get_adc_step_bit(adc_dev, bit) << 1);
> + adc_dev->buffer_en_ch_steps = enb;
> +
> + am335x_tsc_se_set(adc_dev->mfd_tscadc, enb);
> +
> + tiadc_writel(adc_dev, REG_IRQSTATUS, IRQENB_FIFO1THRES
> + | IRQENB_FIFO1OVRRUN | IRQENB_FIFO1UNDRFLW);
> + tiadc_writel(adc_dev, REG_IRQENABLE, IRQENB_FIFO1THRES
> + | IRQENB_FIFO1OVRRUN);
> +
> + return iio_triggered_buffer_postenable(indio_dev);
> +}
> +
> +static int tiadc_buffer_predisable(struct iio_dev *indio_dev)
> +{
> + struct tiadc_device *adc_dev = iio_priv(indio_dev);
> + int fifo1count, i, read;
> +
> + 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);
> +
> + /* Flush FIFO of any leftover data */
> + fifo1count = tiadc_readl(adc_dev, REG_FIFO1CNT);
> + for (i = 0; i < fifo1count; i++)
> + read = tiadc_readl(adc_dev, REG_FIFO1);
> +
> + return iio_triggered_buffer_predisable(indio_dev);
> +}
> +
> +static int tiadc_buffer_postdisable(struct iio_dev *indio_dev)
> +{
> + struct tiadc_device *adc_dev = iio_priv(indio_dev);
> +
> + tiadc_step_config(indio_dev);
> + kfree(adc_dev->data);
blank line here please.
> + return 0;
> +}
> +
> +static const struct iio_buffer_setup_ops tiadc_buffer_setup_ops = {
> + .preenable = &tiadc_buffer_preenable,
> + .postenable = &tiadc_buffer_postenable,
> + .predisable = &tiadc_buffer_predisable,
> + .postdisable = &tiadc_buffer_postdisable,
> +};
> +
> +static const struct iio_trigger_ops tiadc_trigger_ops = {
> + .owner = THIS_MODULE,
> +};
> +
If you want a utility function for allocation, convention would
say have one for freeing as well so that the code is obviously
balanced and easy to check. Also this registers the buffer as
well so the name should probably reflect that.
> +static int tiadc_iio_allocate_trigger(struct iio_dev *indio_dev)
> +{
> + struct tiadc_device *adc_dev = iio_priv(indio_dev);
> + struct iio_trigger *trig = adc_dev->trig;
> + int ret;
> +
devm_iio_trigger_alloc is now available and will simplify things a little.
> + trig = iio_trigger_alloc("%s-dev%d", indio_dev->name, indio_dev->id);
> + if (trig == NULL)
> + return -ENOMEM;
> +
> + trig->dev.parent = indio_dev->dev.parent;
> + trig->ops = &tiadc_trigger_ops;
> + iio_trigger_set_drvdata(trig, indio_dev);
> +
> + ret = iio_trigger_register(trig);
> + if (ret)
> + goto err_free_trigger;
> +
> + return 0;
> +
> +err_free_trigger:
> + iio_trigger_free(adc_dev->trig);
> +
> + return ret;
> }
>
> static const char * const chan_name_ain[] = {
> @@ -120,6 +295,7 @@ static int tiadc_channel_init(struct iio_dev *indio_dev, int channels)
> chan->channel = adc_dev->channel_line[i];
> chan->info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
> chan->datasheet_name = chan_name_ain[chan->channel];
> + chan->scan_index = i;
> chan->scan_type.sign = 'u';
> chan->scan_type.realbits = 12;
> chan->scan_type.storagebits = 32;
I never noticed this before but putting 12 bit values in 32 bit storage
does seem rather wasteful. Far as I can see above there is no real
reason for doing so. If there is, then please add a comment justifying this
choice!
> @@ -142,11 +318,14 @@ static int tiadc_read_raw(struct iio_dev *indio_dev,
> struct tiadc_device *adc_dev = iio_priv(indio_dev);
> int i, map_val;
> unsigned int fifo1count, read, stepid;
> - u32 step = UINT_MAX;
> bool found = false;
> u32 step_en;
> unsigned long timeout = jiffies + usecs_to_jiffies
> (IDLE_TIMEOUT * adc_dev->channels);
> +
> + if (iio_buffer_enabled(indio_dev))
> + return -EBUSY;
> +
> step_en = get_adc_step_mask(adc_dev);
> am335x_tsc_se_set(adc_dev->mfd_tscadc, step_en);
>
> @@ -168,15 +347,6 @@ static int tiadc_read_raw(struct iio_dev *indio_dev,
> * Hence we need to flush out this data.
> */
Is the comment above still valid? I'd guess it referred to this code
which has now gone.
>
> - for (i = 0; i < ARRAY_SIZE(adc_dev->channel_step); i++) {
> - if (chan->channel == adc_dev->channel_line[i]) {
> - step = adc_dev->channel_step[i];
> - break;
> - }
> - }
> - if (WARN_ON_ONCE(step == UINT_MAX))
> - return -EINVAL;
> -
> fifo1count = tiadc_readl(adc_dev, REG_FIFO1CNT);
> for (i = 0; i < fifo1count; i++) {
> read = tiadc_readl(adc_dev, REG_FIFO1);
> @@ -231,26 +401,49 @@ static int tiadc_probe(struct platform_device *pdev)
> channels++;
> }
> adc_dev->channels = channels;
Use the devm interfaces and there is no need to keep a copy
of the irq number about.
> + adc_dev->irq = adc_dev->mfd_tscadc->irq;
>
> indio_dev->dev.parent = &pdev->dev;
> indio_dev->name = dev_name(&pdev->dev);
> indio_dev->modes = INDIO_DIRECT_MODE;
> indio_dev->info = &tiadc_info;
>
> - tiadc_step_config(adc_dev);
> + tiadc_step_config(indio_dev);
> + tiadc_writel(adc_dev, REG_FIFO1THR, FIFO1_THRESHOLD);
>
> err = tiadc_channel_init(indio_dev, adc_dev->channels);
> if (err < 0)
> return err;
>
> - err = iio_device_register(indio_dev);
> + err = tiadc_iio_allocate_trigger(indio_dev);
> if (err)
> goto err_free_channels;
>
devm_request_irq will simplify error handling a little.
> + err = request_irq(adc_dev->irq, tiadc_irq, IRQF_SHARED,
> + indio_dev->name, indio_dev);
> + if (err)
> + goto err_unregister_trigger;
> +
> + err = iio_triggered_buffer_setup(indio_dev, NULL,
> + &tiadc_trigger_h, &tiadc_buffer_setup_ops);
> + if (err)
> + goto err_free_irq;
> +
> + err = iio_device_register(indio_dev);
> + if (err)
> + goto err_buffer_unregister;
> +
> platform_set_drvdata(pdev, indio_dev);
>
> return 0;
>
> +err_buffer_unregister:
> + iio_buffer_unregister(indio_dev);
> +err_free_irq:
> + free_irq(adc_dev->irq, indio_dev);
> +err_unregister_trigger:
> + iio_trigger_unregister(adc_dev->trig);
> + iio_trigger_free(adc_dev->trig);
> err_free_channels:
> tiadc_channels_remove(indio_dev);
> return err;
> @@ -262,7 +455,11 @@ static int tiadc_remove(struct platform_device *pdev)
> struct tiadc_device *adc_dev = iio_priv(indio_dev);
> u32 step_en;
>
> + iio_trigger_unregister(adc_dev->trig);
> + iio_trigger_free(adc_dev->trig);
There is a devm trigger allocation function now. Using that
would make things a little cleaner.
> + free_irq(adc_dev->irq, indio_dev);
Best to use the devm interfaces for irq handling unless
there is a good reason to not do so (cleaner code ;)
> iio_device_unregister(indio_dev);
We would normally expect this function to run in exact
reverse order of the probe. That is not the case here
so please reorder things so it is.
> + iio_buffer_unregister(indio_dev);
> tiadc_channels_remove(indio_dev);
>
> step_en = get_adc_step_mask(adc_dev);
> @@ -298,10 +495,16 @@ static int tiadc_resume(struct device *dev)
>
> /* Make sure ADC is powered up */
This comment doesn't seem to me to be true any more.
> restore = tiadc_readl(adc_dev, REG_CTRL);
> - restore &= ~(CNTRLREG_POWERDOWN);
Some coments here would be good to explain the sequence that
is going on.
I'm guessing a bit, but looks like you turn off the touch screen
but leave everything else alone, then set the threshold then
reenable the touch screen before powering up the screen?
> + restore &= ~(CNTRLREG_TSCSSENB);
> tiadc_writel(adc_dev, REG_CTRL, restore);
>
> - tiadc_step_config(adc_dev);
> + tiadc_writel(adc_dev, REG_FIFO1THR, FIFO1_THRESHOLD);
> + tiadc_step_config(indio_dev);
> +
> + /* Make sure ADC is powered up */
> + restore &= ~(CNTRLREG_POWERDOWN);
> + restore |= CNTRLREG_TSCSSENB;
> + tiadc_writel(adc_dev, REG_CTRL, restore);
>
> return 0;
> }
> diff --git a/include/linux/mfd/ti_am335x_tscadc.h b/include/linux/mfd/ti_am335x_tscadc.h
> index db1791b..a372ebf 100644
> --- a/include/linux/mfd/ti_am335x_tscadc.h
> +++ b/include/linux/mfd/ti_am335x_tscadc.h
> @@ -46,17 +46,25 @@
> /* Step Enable */
> #define STEPENB_MASK (0x1FFFF << 0)
> #define STEPENB(val) ((val) << 0)
> +#define ENB(val) (1 << (val))
> +#define STPENB_STEPENB STEPENB(0x1FFFF)
> +#define STPENB_STEPENB_TC STEPENB(0x1FFF)
>
> /* IRQ enable */
> #define IRQENB_HW_PEN BIT(0)
> #define IRQENB_FIFO0THRES BIT(2)
> +#define IRQENB_FIFO0OVRRUN BIT(3)
> +#define IRQENB_FIFO0UNDRFLW BIT(4)
> #define IRQENB_FIFO1THRES BIT(5)
> +#define IRQENB_FIFO1OVRRUN BIT(6)
> +#define IRQENB_FIFO1UNDRFLW BIT(7)
> #define IRQENB_PENUP BIT(9)
>
> /* Step Configuration */
> #define STEPCONFIG_MODE_MASK (3 << 0)
> #define STEPCONFIG_MODE(val) ((val) << 0)
> #define STEPCONFIG_MODE_HWSYNC STEPCONFIG_MODE(2)
> +#define STEPCONFIG_MODE_SWCNT STEPCONFIG_MODE(1)
> #define STEPCONFIG_AVG_MASK (7 << 2)
> #define STEPCONFIG_AVG(val) ((val) << 2)
> #define STEPCONFIG_AVG_16 STEPCONFIG_AVG(4)
> @@ -124,6 +132,7 @@
> #define MAX_CLK_DIV 7
> #define TOTAL_STEPS 16
> #define TOTAL_CHANNELS 8
> +#define FIFO1_THRESHOLD 19
>
> /*
> * ADC runs at 3MHz, and it takes
> @@ -153,6 +162,10 @@ struct ti_tscadc_dev {
>
> /* adc device */
> struct adc_device *adc;
> +
> + /* Context save */
> + unsigned int irqstat;
Where are these used?
> + unsigned int ctrl;
> };
>
> static inline struct ti_tscadc_dev *ti_tscadc_dev_get(struct platform_device *p)
>
^ permalink raw reply
* Re: [PATCH 1/2] input: ti_am335x_tsc: Enable shared IRQ for TSC
From: Jonathan Cameron @ 2013-09-08 11:29 UTC (permalink / raw)
To: Zubair Lutfullah
Cc: dmitry.torokhov-Re5JQEeQqe8AvxtiuMwx3w,
linux-iio-u79uwXL29TY76Z2rM5mHXA,
linux-input-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
bigeasy-hfZtesqFncYOwBW4kG4KsQ,
gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r
In-Reply-To: <1378034277-26728-2-git-send-email-zubair.lutfullah-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
On 09/01/13 12:17, Zubair Lutfullah wrote:
> Enable shared IRQ to allow ADC to share IRQ line from
> parent MFD core. Only FIFO0 IRQs are for TSC and handled
> on the TSC side.
>
> Step mask would be updated from cached variable only previously.
> In rare cases when both TSC and ADC are used, the cached
> variable gets mixed up.
> The step mask is written with the required mask every time.
>
> Rachna Patil (TI) laid ground work for shared IRQ.
>
> Signed-off-by: Zubair Lutfullah <zubair.lutfullah-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Whilst I would have prefered an mfd under these drivers and elegant handling
of the interrupts, I guess if this works it is at least not terribly invasive.
However, this does need an Ack from Dmitry before I can take it (or for
Dmitry to take it himself?)
> ---
> drivers/input/touchscreen/ti_am335x_tsc.c | 12 +++++++++---
> 1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/input/touchscreen/ti_am335x_tsc.c b/drivers/input/touchscreen/ti_am335x_tsc.c
> index e1c5300..24e625c 100644
> --- a/drivers/input/touchscreen/ti_am335x_tsc.c
> +++ b/drivers/input/touchscreen/ti_am335x_tsc.c
> @@ -52,6 +52,7 @@ struct titsc {
> u32 config_inp[4];
> u32 bit_xp, bit_xn, bit_yp, bit_yn;
> u32 inp_xp, inp_xn, inp_yp, inp_yn;
> + u32 step_mask;
> };
>
> static unsigned int titsc_readl(struct titsc *ts, unsigned int reg)
> @@ -196,7 +197,8 @@ static void titsc_step_config(struct titsc *ts_dev)
>
> /* The steps1 … end and bit 0 for TS_Charge */
> stepenable = (1 << (end_step + 2)) - 1;
> - am335x_tsc_se_set(ts_dev->mfd_tscadc, stepenable);
> + ts_dev->step_mask = stepenable;
> + am335x_tsc_se_set(ts_dev->mfd_tscadc, ts_dev->step_mask);
> }
>
> static void titsc_read_coordinates(struct titsc *ts_dev,
> @@ -260,6 +262,10 @@ static irqreturn_t titsc_irq(int irq, void *dev)
> unsigned int fsm;
>
> status = titsc_readl(ts_dev, REG_IRQSTATUS);
> + /*
> + * ADC and touchscreen share the IRQ line.
> + * FIFO1 interrupts are used by ADC. Handle FIFO0 IRQs here only
> + */
> if (status & IRQENB_FIFO0THRES) {
>
> titsc_read_coordinates(ts_dev, &x, &y, &z1, &z2);
> @@ -316,7 +322,7 @@ static irqreturn_t titsc_irq(int irq, void *dev)
>
> if (irqclr) {
> titsc_writel(ts_dev, REG_IRQSTATUS, irqclr);
> - am335x_tsc_se_update(ts_dev->mfd_tscadc);
> + am335x_tsc_se_set(ts_dev->mfd_tscadc, ts_dev->step_mask);
> return IRQ_HANDLED;
> }
> return IRQ_NONE;
> @@ -389,7 +395,7 @@ static int titsc_probe(struct platform_device *pdev)
> }
>
> err = request_irq(ts_dev->irq, titsc_irq,
> - 0, pdev->dev.driver->name, ts_dev);
> + IRQF_SHARED, pdev->dev.driver->name, ts_dev);
> if (err) {
> dev_err(&pdev->dev, "failed to allocate irq.\n");
> goto err_free_mem;
>
^ permalink raw reply
* Re: [PATCH v4] Input: evdev - add EVIOCREVOKE ioctl
From: David Herrmann @ 2013-09-07 17:41 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: open list:HID CORE LAYER, Kristian Høgsberg
In-Reply-To: <c552d883-34a3-4317-9053-be7ea190cebe@email.android.com>
Hi Dmitry
On Sat, Sep 7, 2013 at 7:16 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> David Herrmann <dh.herrmann@gmail.com> wrote:
>>Given the recent ABS_*-regression, I wrote a bunch of more aggressive
>>stress-tests for this. I didn't found any regressions if EVIOCREVOKE is
>>not
>>used, but one with revoke on an empty queue in evdev_read(). Now fixed.
>>Please
>>let me know what your plans for this patch are (-next or -rc1?) so we
>>can
>>schedule accordingly.
>
> I think this one is safer than extending axis numbers as there is no concerns about breaking stuff - it's brand new. So I think we can work it in -rc1.
Sounds good, thanks.
The ABS-fixes will take some time, yepp. We can retry these with the
guitar/drums ABS bits for 3.13. EVIOC[SG]ABS2 is probably the way to
go.
>> As a side note, will you attend LPC this year? We
>>have a
>>bunch of fancy stuff I'd like your opinion on (including
>>device-properties, device-detection, ABS_* bit extension).
>
> Yes. I'll get to new Orleans a couple days earlier (Sunday night) so if you are in town we could meet for drinks or such.
I'll arrive there on Saturday-evening, will drop you a message. See
you in New Orleans.
Thanks
David
^ permalink raw reply
* Re: [GIT] HID for 3.12 merge window
From: David Herrmann @ 2013-09-07 17:31 UTC (permalink / raw)
To: Linus Torvalds
Cc: Dmitry Torokhov, Markus Trippelsdorf, Jiri Kosina, linux-kernel,
open list:HID CORE LAYER
In-Reply-To: <CA+55aFye+TFiXPeJhB9fJjUcMR6LynjsZU+sjckKk2YoYOZDnw@mail.gmail.com>
Hi Linus
On Sat, Sep 7, 2013 at 6:52 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Sat, Sep 7, 2013 at 12:31 AM, David Herrmann <dh.herrmann@gmail.com> wrote:
>>
>> The bug only occurs for multi-touch devices (their ABS_* bits are
>>>0x1f), that's why I didn't see it happening during my tests..
>
> It definitely affects keyboards too. That's how I noticed it. That is
> with the logitech "universal" receiver, though, so maybe that counts
> as a potential multi-touch device..
I have been quite wrong with that statement, yepp. xorg-evdev fetches
the whole device-information. So if a keyboard provides an affected
ABS_* bit, it'll break, too. My ancient hardware didn't use any of the
higher ABS bits, though.
I'm sorry for the troubles caused. ABS_MAX must be at most 0x3f. So
please, go ahead and revert the 3 mentioned patches.
Thanks
David
^ permalink raw reply
* Re: [PATCH v4] Input: evdev - add EVIOCREVOKE ioctl
From: Dmitry Torokhov @ 2013-09-07 17:16 UTC (permalink / raw)
To: David Herrmann, linux-input; +Cc: Kristian Høgsberg
In-Reply-To: <1378551653-805-1-git-send-email-dh.herrmann@gmail.com>
David Herrmann <dh.herrmann@gmail.com> wrote:
>If we have multiple sessions on a system, we normally don't want
>background sessions to read input events. Otherwise, it could capture
>passwords and more entered by the user on the foreground session. This
>is
>a real world problem as the recent XMir development showed:
> http://mjg59.dreamwidth.org/27327.html
>
>We currently rely on sessions to release input devices when being
>deactivated. This relies on trust across sessions. But that's not given
>on
>usual systems. We therefore need a way to control which processes have
>access to input devices.
>
>With VTs the kernel simply routed them through the active /dev/ttyX.
>This
>is not possible with evdev devices, though. Moreover, we want to avoid
>routing input-devices through some dispatcher-daemon in userspace
>(which
>would add some latency).
>
>This patch introduces EVIOCREVOKE. If called on an evdev fd, this
>revokes
>device-access irrecoverably for that *single* open-file. Hence, once
>you
>call EVIOCREVOKE on any dup()ed fd, all fds for that open-file will be
>rather useless now (but still valid compared to close()!). This allows
>us
>to pass fds directly to session-processes from a trusted source. The
>source keeps a dup()ed fd and revokes access once the session-process
>is
>no longer active.
>Compared to the EVIOCMUTE proposal, we can avoid the CAP_SYS_ADMIN
>restriction now as there is no way to revive the fd again. Hence, a
>user
>is free to call EVIOCREVOKE themself to kill the fd.
>
>Additionally, this ioctl allows multi-layer access-control (again
>compared
>to EVIOCMUTE which was limited to one layer via CAP_SYS_ADMIN). A
>middle
>layer can simply request a new open-file from the layer above and pass
>it
>to the layer below. Now each layer can call EVIOCREVOKE on the fds to
>revoke access for all layers below, at the expense of one fd per layer.
>
>There's already ongoing experimental user-space work which demonstrates
>how it can be used:
>http://lists.freedesktop.org/archives/systemd-devel/2013-August/012897.html
>
>Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
>---
>Hi Dmitry
>
>Given the recent ABS_*-regression, I wrote a bunch of more aggressive
>stress-tests for this. I didn't found any regressions if EVIOCREVOKE is
>not
>used, but one with revoke on an empty queue in evdev_read(). Now fixed.
>Please
>let me know what your plans for this patch are (-next or -rc1?) so we
>can
>schedule accordingly.
I think this one is safer than extending axis numbers as there is no concerns about breaking stuff - it's brand new. So I think we can work it in -rc1.
> As a side note, will you attend LPC this year? We
>have a
>bunch of fancy stuff I'd like your opinion on (including
>device-properties, device-detection, ABS_* bit extension).
Yes. I'll get to new Orleans a couple days earlier (Sunday night) so if you are in town we could meet for drinks or such.
>
>Thanks and cheers
>David
>
> drivers/input/evdev.c | 37 +++++++++++++++++++++++++++++++------
> include/uapi/linux/input.h | 1 +
> 2 files changed, 32 insertions(+), 6 deletions(-)
>
>diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
>index d2b34fb..b6ded17 100644
>--- a/drivers/input/evdev.c
>+++ b/drivers/input/evdev.c
>@@ -48,6 +48,7 @@ struct evdev_client {
> struct evdev *evdev;
> struct list_head node;
> int clkid;
>+ bool revoked;
> unsigned int bufsize;
> struct input_event buffer[];
> };
>@@ -164,6 +165,9 @@ static void evdev_pass_values(struct evdev_client
>*client,
> struct input_event event;
> bool wakeup = false;
>
>+ if (client->revoked)
>+ return;
>+
> event.time = ktime_to_timeval(client->clkid == CLOCK_MONOTONIC ?
> mono : real);
>
>@@ -240,7 +244,7 @@ static int evdev_flush(struct file *file,
>fl_owner_t id)
> if (retval)
> return retval;
>
>- if (!evdev->exist)
>+ if (!evdev->exist || client->revoked)
> retval = -ENODEV;
> else
> retval = input_flush_device(&evdev->handle, file);
>@@ -429,7 +433,7 @@ static ssize_t evdev_write(struct file *file, const
>char __user *buffer,
> if (retval)
> return retval;
>
>- if (!evdev->exist) {
>+ if (!evdev->exist || client->revoked) {
> retval = -ENODEV;
> goto out;
> }
>@@ -482,7 +486,7 @@ static ssize_t evdev_read(struct file *file, char
>__user *buffer,
> return -EINVAL;
>
> for (;;) {
>- if (!evdev->exist)
>+ if (!evdev->exist || client->revoked)
> return -ENODEV;
>
> if (client->packet_head == client->tail &&
>@@ -511,7 +515,7 @@ static ssize_t evdev_read(struct file *file, char
>__user *buffer,
> if (!(file->f_flags & O_NONBLOCK)) {
> error = wait_event_interruptible(evdev->wait,
> client->packet_head != client->tail ||
>- !evdev->exist);
>+ !evdev->exist || client->revoked);
> if (error)
> return error;
> }
>@@ -529,7 +533,11 @@ static unsigned int evdev_poll(struct file *file,
>poll_table *wait)
>
> poll_wait(file, &evdev->wait, wait);
>
>- mask = evdev->exist ? POLLOUT | POLLWRNORM : POLLHUP | POLLERR;
>+ if (evdev->exist && !client->revoked)
>+ mask = POLLOUT | POLLWRNORM;
>+ else
>+ mask = POLLHUP | POLLERR;
>+
> if (client->packet_head != client->tail)
> mask |= POLLIN | POLLRDNORM;
>
>@@ -795,6 +803,17 @@ static int evdev_handle_mt_request(struct
>input_dev *dev,
> return 0;
> }
>
>+static int evdev_revoke(struct evdev *evdev, struct evdev_client
>*client,
>+ struct file *file)
>+{
>+ client->revoked = true;
>+ evdev_ungrab(evdev, client);
>+ input_flush_device(&evdev->handle, file);
>+ wake_up_interruptible(&evdev->wait);
>+
>+ return 0;
>+}
>+
> static long evdev_do_ioctl(struct file *file, unsigned int cmd,
> void __user *p, int compat_mode)
> {
>@@ -857,6 +876,12 @@ static long evdev_do_ioctl(struct file *file,
>unsigned int cmd,
> else
> return evdev_ungrab(evdev, client);
>
>+ case EVIOCREVOKE:
>+ if (p)
>+ return -EINVAL;
>+ else
>+ return evdev_revoke(evdev, client, file);
>+
> case EVIOCSCLOCKID:
> if (copy_from_user(&i, p, sizeof(unsigned int)))
> return -EFAULT;
>@@ -1002,7 +1027,7 @@ static long evdev_ioctl_handler(struct file
>*file, unsigned int cmd,
> if (retval)
> return retval;
>
>- if (!evdev->exist) {
>+ if (!evdev->exist || client->revoked) {
> retval = -ENODEV;
> goto out;
> }
>diff --git a/include/uapi/linux/input.h b/include/uapi/linux/input.h
>index 2fb6fae..d61c61c 100644
>--- a/include/uapi/linux/input.h
>+++ b/include/uapi/linux/input.h
>@@ -152,6 +152,7 @@ struct input_keymap_entry {
>#define EVIOCGEFFECTS _IOR('E', 0x84, int) /* Report number of
>effects playable at the same time */
>
> #define EVIOCGRAB _IOW('E', 0x90, int) /* Grab/Release device */
>+#define EVIOCREVOKE _IOW('E', 0x91, int) /* Revoke device access */
>
>#define EVIOCSCLOCKID _IOW('E', 0xa0, int) /* Set clockid to be used
>for timestamps */
>
Hi David,
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [GIT] HID for 3.12 merge window
From: Dmitry Torokhov @ 2013-09-07 17:12 UTC (permalink / raw)
To: David Herrmann, Benjamin Tissoires
Cc: Linus Torvalds, Markus Trippelsdorf, Jiri Kosina, linux-kernel,
open list:HID CORE LAYER
In-Reply-To: <CANq1E4ROgWt2oZt9BVgt4uzmHDfTy6DBzTswHvH=ziVRkjfXvg@mail.gmail.com>
David Herrmann <dh.herrmann@gmail.com> wrote:
>Hi
>
>On Sat, Sep 7, 2013 at 11:20 AM, Benjamin Tissoires
><benjamin.tissoires@gmail.com> wrote:
>>
>>
>> On 07/09/13 10:57, David Herrmann wrote:
>>> Hi
>>>
>>> On Sat, Sep 7, 2013 at 10:24 AM, Benjamin Tissoires
>>>> I'm not particularly in favor of adding semantic between ABS_MISC
>and
>>>> ABS_MT_SLOT. We already did that once with the ABS_MT_* stuff, and
>the
>>>> problem comes from devices showing several axes, not really mapped
>>>> (like joysticks). These axes are all mapped to ABS_MISC, but the
>input
>>>> core function map them to ABS_MISC+N. This way some joysticks show
>>>> multitouch axes and are treated as such, whereas the axis are just
>>>> regular absolute axes. (not sure I am clear enough... :( )
>>>>
>>>> Currently, nothing is done in X or in Wayland to detect these false
>>>> multitouch devices with a large number of absolute axes. But if we
>>>> keep a hole between ABS_MISC and ABS_MT_SLOT, we could add an
>>>> heuristic like:
>>>> "if all the axes between ABS_MISC and ABS_MT_SLOT are available,
>then
>>>> ABS_MT_SLOT does not mean multitouch, but ABS_MISC+6."
>>>
>>> So Xorg just took control over the unused ABS_* slots? Hmm, then I
>>> have no other idea than reverting the 3 patches.
>>> Sadly, that also means there's no way for us to easily extend the
>>> ABS_* bits. If someone has an idea, let me know. But for now I'll
>just
>>> put the drums/guitar patches on hold.
>>>
>>
>> Well, I would say the "problem" lies in hid-input.c, line 919:
>>>>>>>>>>>>>>>>>>>>
>> while (usage->code <= max && test_and_set_bit(usage->code,
>bit))
>> usage->code = find_next_zero_bit(bit, max + 1,
>usage->code);
>>>>>>>>>>>>>>>>>>>>
>>
>> So if you try to map several times ABS_MISC or any other axis, then
>you are mapping ABS_MISC+N.
>>
>> Anyway, the only problematic axis is ABS_MISC, as it is a default
>fallback. IMO, we can extend the ABS_* definitions after ABS_MT_TOOL_Y,
>like the original patch you proposed without having to deal with the
>above problem (though this will report the patch series to v3.13).
>> To prevent future mistakes, we should just define ABS_MISC0 (0x29) to
>ABS_MISC5 (0x2e) to fill the hole.
>
>As mentioned earlier in this thread, ABS_* is limited to 0x3f, so we
>have only 2 more free slots after ABS_MT_TOOL_Y.
>We will figure something out for 3.13.
>
>I will also prepare a patch for ABS_MISC0-MISC5.
>
Regardless of whether we settle on using lower bits or bite the bullet and extend number of axis I think it's time for EVIOCGABS2 that would take axis in its argument instead of encoding it in ioctl number - that idea wasn't that good.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: Game Controllers
From: Bastien Nocera @ 2013-09-07 17:01 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: Todd Showalter, David Herrmann, open list:HID CORE LAYER
In-Reply-To: <20130503155000.GA5567@core.coreip.homeip.net>
Em Fri, 2013-05-03 às 08:50 -0700, Dmitry Torokhov escreveu:
> On Thu, May 02, 2013 at 05:10:42PM -0400, Todd Showalter wrote:
> > On Thu, May 2, 2013 at 4:45 PM, Dmitry Torokhov
> > <dmitry.torokhov@gmail.com> wrote:
> >
> > > Again, the kernel will try to do the right thing and produce the correct
> > > events for new input devices, but we are not going to introduce a copy
> > > of evdev protocol that limits the events to "standard gamepad" ones.
> >
> > Ok. I can see there's no point arguing this. What can I do to at
> > least make sure future devices behave? Is there anything that can be
> > done about current devices other than translate in userspace?
>
> Please post the patches for discussion and keep en eye on changes going
> into drivers/input/joystick and drivers/hid as you have a vested
> interest of everyone getting it right.
>
> I am not sure about status of PS3 controller, I think Bastien was
> working on supporting it from userspace?
Antonio Ospite has patches against bluez5 to make it work[1], though
some more work is definitely needed. Benjamin Tissoires has a test
device now, so between them, they should be able to get it running
again.
Cheers
[1]: http://pkgs.fedoraproject.org/cgit/bluez.git/tree/playstation-peripheral-pugin-v5.x.patch
--
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: [GIT] HID for 3.12 merge window
From: Linus Torvalds @ 2013-09-07 16:52 UTC (permalink / raw)
To: David Herrmann
Cc: Dmitry Torokhov, Markus Trippelsdorf, Jiri Kosina, linux-kernel,
open list:HID CORE LAYER
In-Reply-To: <CANq1E4Q85ewwKs7VoooZVapC7-Cvy6-Xp4rGL1-1Ee7w1EcT6w@mail.gmail.com>
On Sat, Sep 7, 2013 at 12:31 AM, David Herrmann <dh.herrmann@gmail.com> wrote:
>
> The bug only occurs for multi-touch devices (their ABS_* bits are
>>0x1f), that's why I didn't see it happening during my tests..
It definitely affects keyboards too. That's how I noticed it. That is
with the logitech "universal" receiver, though, so maybe that counts
as a potential multi-touch device..
Linus
^ permalink raw reply
* [PATCH v4] Input: evdev - add EVIOCREVOKE ioctl
From: David Herrmann @ 2013-09-07 11:00 UTC (permalink / raw)
To: linux-input; +Cc: Dmitry Torokhov, Kristian Høgsberg, David Herrmann
In-Reply-To: <1378044450-8327-1-git-send-email-dh.herrmann@gmail.com>
If we have multiple sessions on a system, we normally don't want
background sessions to read input events. Otherwise, it could capture
passwords and more entered by the user on the foreground session. This is
a real world problem as the recent XMir development showed:
http://mjg59.dreamwidth.org/27327.html
We currently rely on sessions to release input devices when being
deactivated. This relies on trust across sessions. But that's not given on
usual systems. We therefore need a way to control which processes have
access to input devices.
With VTs the kernel simply routed them through the active /dev/ttyX. This
is not possible with evdev devices, though. Moreover, we want to avoid
routing input-devices through some dispatcher-daemon in userspace (which
would add some latency).
This patch introduces EVIOCREVOKE. If called on an evdev fd, this revokes
device-access irrecoverably for that *single* open-file. Hence, once you
call EVIOCREVOKE on any dup()ed fd, all fds for that open-file will be
rather useless now (but still valid compared to close()!). This allows us
to pass fds directly to session-processes from a trusted source. The
source keeps a dup()ed fd and revokes access once the session-process is
no longer active.
Compared to the EVIOCMUTE proposal, we can avoid the CAP_SYS_ADMIN
restriction now as there is no way to revive the fd again. Hence, a user
is free to call EVIOCREVOKE themself to kill the fd.
Additionally, this ioctl allows multi-layer access-control (again compared
to EVIOCMUTE which was limited to one layer via CAP_SYS_ADMIN). A middle
layer can simply request a new open-file from the layer above and pass it
to the layer below. Now each layer can call EVIOCREVOKE on the fds to
revoke access for all layers below, at the expense of one fd per layer.
There's already ongoing experimental user-space work which demonstrates
how it can be used:
http://lists.freedesktop.org/archives/systemd-devel/2013-August/012897.html
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
Hi Dmitry
Given the recent ABS_*-regression, I wrote a bunch of more aggressive
stress-tests for this. I didn't found any regressions if EVIOCREVOKE is not
used, but one with revoke on an empty queue in evdev_read(). Now fixed. Please
let me know what your plans for this patch are (-next or -rc1?) so we can
schedule accordingly. As a side note, will you attend LPC this year? We have a
bunch of fancy stuff I'd like your opinion on (including
device-properties, device-detection, ABS_* bit extension).
Thanks and cheers
David
drivers/input/evdev.c | 37 +++++++++++++++++++++++++++++++------
include/uapi/linux/input.h | 1 +
2 files changed, 32 insertions(+), 6 deletions(-)
diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
index d2b34fb..b6ded17 100644
--- a/drivers/input/evdev.c
+++ b/drivers/input/evdev.c
@@ -48,6 +48,7 @@ struct evdev_client {
struct evdev *evdev;
struct list_head node;
int clkid;
+ bool revoked;
unsigned int bufsize;
struct input_event buffer[];
};
@@ -164,6 +165,9 @@ static void evdev_pass_values(struct evdev_client *client,
struct input_event event;
bool wakeup = false;
+ if (client->revoked)
+ return;
+
event.time = ktime_to_timeval(client->clkid == CLOCK_MONOTONIC ?
mono : real);
@@ -240,7 +244,7 @@ static int evdev_flush(struct file *file, fl_owner_t id)
if (retval)
return retval;
- if (!evdev->exist)
+ if (!evdev->exist || client->revoked)
retval = -ENODEV;
else
retval = input_flush_device(&evdev->handle, file);
@@ -429,7 +433,7 @@ static ssize_t evdev_write(struct file *file, const char __user *buffer,
if (retval)
return retval;
- if (!evdev->exist) {
+ if (!evdev->exist || client->revoked) {
retval = -ENODEV;
goto out;
}
@@ -482,7 +486,7 @@ static ssize_t evdev_read(struct file *file, char __user *buffer,
return -EINVAL;
for (;;) {
- if (!evdev->exist)
+ if (!evdev->exist || client->revoked)
return -ENODEV;
if (client->packet_head == client->tail &&
@@ -511,7 +515,7 @@ static ssize_t evdev_read(struct file *file, char __user *buffer,
if (!(file->f_flags & O_NONBLOCK)) {
error = wait_event_interruptible(evdev->wait,
client->packet_head != client->tail ||
- !evdev->exist);
+ !evdev->exist || client->revoked);
if (error)
return error;
}
@@ -529,7 +533,11 @@ static unsigned int evdev_poll(struct file *file, poll_table *wait)
poll_wait(file, &evdev->wait, wait);
- mask = evdev->exist ? POLLOUT | POLLWRNORM : POLLHUP | POLLERR;
+ if (evdev->exist && !client->revoked)
+ mask = POLLOUT | POLLWRNORM;
+ else
+ mask = POLLHUP | POLLERR;
+
if (client->packet_head != client->tail)
mask |= POLLIN | POLLRDNORM;
@@ -795,6 +803,17 @@ static int evdev_handle_mt_request(struct input_dev *dev,
return 0;
}
+static int evdev_revoke(struct evdev *evdev, struct evdev_client *client,
+ struct file *file)
+{
+ client->revoked = true;
+ evdev_ungrab(evdev, client);
+ input_flush_device(&evdev->handle, file);
+ wake_up_interruptible(&evdev->wait);
+
+ return 0;
+}
+
static long evdev_do_ioctl(struct file *file, unsigned int cmd,
void __user *p, int compat_mode)
{
@@ -857,6 +876,12 @@ static long evdev_do_ioctl(struct file *file, unsigned int cmd,
else
return evdev_ungrab(evdev, client);
+ case EVIOCREVOKE:
+ if (p)
+ return -EINVAL;
+ else
+ return evdev_revoke(evdev, client, file);
+
case EVIOCSCLOCKID:
if (copy_from_user(&i, p, sizeof(unsigned int)))
return -EFAULT;
@@ -1002,7 +1027,7 @@ static long evdev_ioctl_handler(struct file *file, unsigned int cmd,
if (retval)
return retval;
- if (!evdev->exist) {
+ if (!evdev->exist || client->revoked) {
retval = -ENODEV;
goto out;
}
diff --git a/include/uapi/linux/input.h b/include/uapi/linux/input.h
index 2fb6fae..d61c61c 100644
--- a/include/uapi/linux/input.h
+++ b/include/uapi/linux/input.h
@@ -152,6 +152,7 @@ struct input_keymap_entry {
#define EVIOCGEFFECTS _IOR('E', 0x84, int) /* Report number of effects playable at the same time */
#define EVIOCGRAB _IOW('E', 0x90, int) /* Grab/Release device */
+#define EVIOCREVOKE _IOW('E', 0x91, int) /* Revoke device access */
#define EVIOCSCLOCKID _IOW('E', 0xa0, int) /* Set clockid to be used for timestamps */
--
1.8.4
^ permalink raw reply related
* Re: [GIT] HID for 3.12 merge window
From: David Herrmann @ 2013-09-07 9:29 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Dmitry Torokhov, Linus Torvalds, Markus Trippelsdorf, Jiri Kosina,
linux-kernel, open list:HID CORE LAYER
In-Reply-To: <522AEFE9.30402@gmail.com>
Hi
On Sat, Sep 7, 2013 at 11:20 AM, Benjamin Tissoires
<benjamin.tissoires@gmail.com> wrote:
>
>
> On 07/09/13 10:57, David Herrmann wrote:
>> Hi
>>
>> On Sat, Sep 7, 2013 at 10:24 AM, Benjamin Tissoires
>>> I'm not particularly in favor of adding semantic between ABS_MISC and
>>> ABS_MT_SLOT. We already did that once with the ABS_MT_* stuff, and the
>>> problem comes from devices showing several axes, not really mapped
>>> (like joysticks). These axes are all mapped to ABS_MISC, but the input
>>> core function map them to ABS_MISC+N. This way some joysticks show
>>> multitouch axes and are treated as such, whereas the axis are just
>>> regular absolute axes. (not sure I am clear enough... :( )
>>>
>>> Currently, nothing is done in X or in Wayland to detect these false
>>> multitouch devices with a large number of absolute axes. But if we
>>> keep a hole between ABS_MISC and ABS_MT_SLOT, we could add an
>>> heuristic like:
>>> "if all the axes between ABS_MISC and ABS_MT_SLOT are available, then
>>> ABS_MT_SLOT does not mean multitouch, but ABS_MISC+6."
>>
>> So Xorg just took control over the unused ABS_* slots? Hmm, then I
>> have no other idea than reverting the 3 patches.
>> Sadly, that also means there's no way for us to easily extend the
>> ABS_* bits. If someone has an idea, let me know. But for now I'll just
>> put the drums/guitar patches on hold.
>>
>
> Well, I would say the "problem" lies in hid-input.c, line 919:
>>>>>>>>>>>>>>>>>>>
> while (usage->code <= max && test_and_set_bit(usage->code, bit))
> usage->code = find_next_zero_bit(bit, max + 1, usage->code);
>>>>>>>>>>>>>>>>>>>
>
> So if you try to map several times ABS_MISC or any other axis, then you are mapping ABS_MISC+N.
>
> Anyway, the only problematic axis is ABS_MISC, as it is a default fallback. IMO, we can extend the ABS_* definitions after ABS_MT_TOOL_Y, like the original patch you proposed without having to deal with the above problem (though this will report the patch series to v3.13).
> To prevent future mistakes, we should just define ABS_MISC0 (0x29) to ABS_MISC5 (0x2e) to fill the hole.
As mentioned earlier in this thread, ABS_* is limited to 0x3f, so we
have only 2 more free slots after ABS_MT_TOOL_Y.
We will figure something out for 3.13.
I will also prepare a patch for ABS_MISC0-MISC5.
Regards
David
^ permalink raw reply
* Re: [GIT] HID for 3.12 merge window
From: David Herrmann @ 2013-09-07 8:57 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Dmitry Torokhov, Linus Torvalds, Markus Trippelsdorf, Jiri Kosina,
linux-kernel, open list:HID CORE LAYER
In-Reply-To: <CAN+gG=Ge2dKvNvT=cY-QFiV=LMV7rddMs9APCb0v68yMGns1Ew@mail.gmail.com>
Hi
On Sat, Sep 7, 2013 at 10:24 AM, Benjamin Tissoires
<benjamin.tissoires@gmail.com> wrote:
> On Sat, Sep 7, 2013 at 9:31 AM, David Herrmann <dh.herrmann@gmail.com> wrote:
>> Hi
>>
>> On Sat, Sep 7, 2013 at 5:22 AM, Dmitry Torokhov
>> <dmitry.torokhov@gmail.com> wrote:
>>> On Fri, Sep 06, 2013 at 06:00:29PM -0700, Linus Torvalds wrote:
>>>> On Fri, Sep 6, 2013 at 5:58 PM, Dmitry Torokhov
>>>> <dmitry.torokhov@gmail.com> wrote:
>>>> >
>>>> > The patch still had problems so I'd revert it and wii bits and try again later.
>>>>
>>>> Ok. Mind giving me a list of commits, so that I don't have to do a
>>>> trial-and-error thing? I know the primary commit that causes problems,
>>>> but there are commits that seem to depend on it..
>>>
>>>
>>> Sorry for the delay. I believe you need to revert:
>>>
>>> 73f8645 HID: wiimote: add support for Guitar-Hero drums
>>> 61e0065 Input: introduce BTN/ABS bits for drums and guitars
>>>
>>> Hmm... there also was "HID: wiimote: add support for Guitar-Hero
>>> guitars" but I do not see it...
>>
>> The commits to revert are:
>>
>> commit 61e00655e9cb82e034eb72b95a51072e718d14a7
>> Author: David Herrmann <dh.herrmann@gmail.com>
>> Date: Mon Aug 26 19:14:46 2013 +0200
>>
>> Input: introduce BTN/ABS bits for drums and guitars
>>
>> commit 73f8645db1913ab2475ec3c1cee8d5f748963aa7
>> Author: David Herrmann <dh.herrmann@gmail.com>
>> Date: Mon Aug 26 19:14:47 2013 +0200
>>
>> HID: wiimote: add support for Guitar-Hero drums
>>
>> commit 8e22ecb603c88b7397ab2e80b7b0811b8a1318f5
>> Author: Nicolas Adenis-Lamarre <nicolas.adenis.lamarre@gmail.com>
>> Date: Mon Aug 26 19:14:48 2013 +0200
>>
>> HID: wiimote: add support for Guitar-Hero guitars
>>
>> Last one is not from me, so I guess that's why Dmitry missed it. And
>> sorry for the delay, stupid UTC+1..
>> The bug only occurs for multi-touch devices (their ABS_* bits are
>>>0x1f), that's why I didn't see it happening during my tests..
>>
>> If you didn't revert it, yet, attached (and inlined) is a patch which
>> reverts the ABS_MAX change and just moves the new identifiers in
>> between the others. On top of 3.12-rc1, this should fix all issues. If
>> you already reverted the patches, I guess we will just push it in
>> linux-next again and wait for 3.13?
>>
>> Thanks
>> David
>>
>> (Inlined patch below, also attached to mail as gmail hates long lines)
>>
>> From 13d549489b4bd2eade719cf2ec8c37724be6e640 Mon Sep 17 00:00:00 2001
>> From: David Herrmann <dh.herrmann@gmail.com>
>> Date: Sat, 7 Sep 2013 09:17:11 +0200
>> Subject: [PATCH] Revert/Fix "Input: introduce BTN/ABS bits for drums and
>> guitars"
>>
>> commit 61e00655e9cb82e034eb72b95a51072e718d14a7
>> Author: David Herrmann <dh.herrmann@gmail.com>
>> Date: Mon Aug 26 19:14:46 2013 +0200
>>
>> Input: introduce BTN/ABS bits for drums and guitars
>>
>> This introduced several new identifiers for drums/guitar devices. However,
>> it also increased ABS_MAX. Unfortunately, 0x3f is already the maximum we
>> can use for ABS_MAX due to the limited EVIOCSABS ioctl.
>>
>> Revert this commit and move the new identifiers somewhere in between. We
>> need to figure out some other way to add new identifiers for the future.
>> To avoid this happening again, I added a small comment.
>>
>> This also fixes an issue where xf86-input-evdev failed on EVIOCGABS() due
>> to ABS_MAX not being a full mask.
>>
>> Markus Trippelsdorf <markus@trippelsdorf.de>
>> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
>> ---
>> include/linux/mod_devicetable.h | 2 +-
>> include/uapi/linux/input.h | 33 ++++++++++++++++++---------------
>> 2 files changed, 19 insertions(+), 16 deletions(-)
>>
>> diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
>> index 329aa30..45e9214 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 0x4f
>> +#define INPUT_DEVICE_ID_ABS_MAX 0x3f
>> #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 76457ee..12c33f8 100644
>> --- a/include/uapi/linux/input.h
>> +++ b/include/uapi/linux/input.h
>> @@ -819,8 +819,24 @@ struct input_keymap_entry {
>>
>> #define ABS_VOLUME 0x20
>>
>> +/* Drums and guitars (mostly toys) */
>> +#define ABS_TOM_FAR_LEFT 0x21
>> +#define ABS_TOM_LEFT 0x22
>> +#define ABS_TOM_RIGHT 0x23
>> +#define ABS_TOM_FAR_RIGHT 0x24
>> +#define ABS_CYMBAL_FAR_LEFT 0x25
>> +#define ABS_CYMBAL_LEFT 0x26
>> +#define ABS_CYMBAL_RIGHT 0x27
>> +
>> #define ABS_MISC 0x28
>>
>> +/* Drums and guitars continued */
>> +#define ABS_CYMBAL_FAR_RIGHT 0x29
>> +#define ABS_BASS 0x2a
>> +#define ABS_HI_HAT 0x2b
>> +#define ABS_FRET_BOARD 0x2c /* Guitar fret board, vertical pos */
>> +#define ABS_WHAMMY_BAR 0x2d /* Guitar whammy bar (or vibrato) */
>> +
>
> I'm not particularly in favor of adding semantic between ABS_MISC and
> ABS_MT_SLOT. We already did that once with the ABS_MT_* stuff, and the
> problem comes from devices showing several axes, not really mapped
> (like joysticks). These axes are all mapped to ABS_MISC, but the input
> core function map them to ABS_MISC+N. This way some joysticks show
> multitouch axes and are treated as such, whereas the axis are just
> regular absolute axes. (not sure I am clear enough... :( )
>
> Currently, nothing is done in X or in Wayland to detect these false
> multitouch devices with a large number of absolute axes. But if we
> keep a hole between ABS_MISC and ABS_MT_SLOT, we could add an
> heuristic like:
> "if all the axes between ABS_MISC and ABS_MT_SLOT are available, then
> ABS_MT_SLOT does not mean multitouch, but ABS_MISC+6."
So Xorg just took control over the unused ABS_* slots? Hmm, then I
have no other idea than reverting the 3 patches.
Sadly, that also means there's no way for us to easily extend the
ABS_* bits. If someone has an idea, let me know. But for now I'll just
put the drums/guitar patches on hold.
Thanks
David
^ permalink raw reply
* Re: [GIT] HID for 3.12 merge window
From: Benjamin Tissoires @ 2013-09-07 8:24 UTC (permalink / raw)
To: David Herrmann
Cc: Dmitry Torokhov, Linus Torvalds, Markus Trippelsdorf, Jiri Kosina,
linux-kernel, open list:HID CORE LAYER
In-Reply-To: <CANq1E4Q85ewwKs7VoooZVapC7-Cvy6-Xp4rGL1-1Ee7w1EcT6w@mail.gmail.com>
On Sat, Sep 7, 2013 at 9:31 AM, David Herrmann <dh.herrmann@gmail.com> wrote:
> Hi
>
> On Sat, Sep 7, 2013 at 5:22 AM, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
>> On Fri, Sep 06, 2013 at 06:00:29PM -0700, Linus Torvalds wrote:
>>> On Fri, Sep 6, 2013 at 5:58 PM, Dmitry Torokhov
>>> <dmitry.torokhov@gmail.com> wrote:
>>> >
>>> > The patch still had problems so I'd revert it and wii bits and try again later.
>>>
>>> Ok. Mind giving me a list of commits, so that I don't have to do a
>>> trial-and-error thing? I know the primary commit that causes problems,
>>> but there are commits that seem to depend on it..
>>
>>
>> Sorry for the delay. I believe you need to revert:
>>
>> 73f8645 HID: wiimote: add support for Guitar-Hero drums
>> 61e0065 Input: introduce BTN/ABS bits for drums and guitars
>>
>> Hmm... there also was "HID: wiimote: add support for Guitar-Hero
>> guitars" but I do not see it...
>
> The commits to revert are:
>
> commit 61e00655e9cb82e034eb72b95a51072e718d14a7
> Author: David Herrmann <dh.herrmann@gmail.com>
> Date: Mon Aug 26 19:14:46 2013 +0200
>
> Input: introduce BTN/ABS bits for drums and guitars
>
> commit 73f8645db1913ab2475ec3c1cee8d5f748963aa7
> Author: David Herrmann <dh.herrmann@gmail.com>
> Date: Mon Aug 26 19:14:47 2013 +0200
>
> HID: wiimote: add support for Guitar-Hero drums
>
> commit 8e22ecb603c88b7397ab2e80b7b0811b8a1318f5
> Author: Nicolas Adenis-Lamarre <nicolas.adenis.lamarre@gmail.com>
> Date: Mon Aug 26 19:14:48 2013 +0200
>
> HID: wiimote: add support for Guitar-Hero guitars
>
> Last one is not from me, so I guess that's why Dmitry missed it. And
> sorry for the delay, stupid UTC+1..
> The bug only occurs for multi-touch devices (their ABS_* bits are
>>0x1f), that's why I didn't see it happening during my tests..
>
> If you didn't revert it, yet, attached (and inlined) is a patch which
> reverts the ABS_MAX change and just moves the new identifiers in
> between the others. On top of 3.12-rc1, this should fix all issues. If
> you already reverted the patches, I guess we will just push it in
> linux-next again and wait for 3.13?
>
> Thanks
> David
>
> (Inlined patch below, also attached to mail as gmail hates long lines)
>
> From 13d549489b4bd2eade719cf2ec8c37724be6e640 Mon Sep 17 00:00:00 2001
> From: David Herrmann <dh.herrmann@gmail.com>
> Date: Sat, 7 Sep 2013 09:17:11 +0200
> Subject: [PATCH] Revert/Fix "Input: introduce BTN/ABS bits for drums and
> guitars"
>
> commit 61e00655e9cb82e034eb72b95a51072e718d14a7
> Author: David Herrmann <dh.herrmann@gmail.com>
> Date: Mon Aug 26 19:14:46 2013 +0200
>
> Input: introduce BTN/ABS bits for drums and guitars
>
> This introduced several new identifiers for drums/guitar devices. However,
> it also increased ABS_MAX. Unfortunately, 0x3f is already the maximum we
> can use for ABS_MAX due to the limited EVIOCSABS ioctl.
>
> Revert this commit and move the new identifiers somewhere in between. We
> need to figure out some other way to add new identifiers for the future.
> To avoid this happening again, I added a small comment.
>
> This also fixes an issue where xf86-input-evdev failed on EVIOCGABS() due
> to ABS_MAX not being a full mask.
>
> Markus Trippelsdorf <markus@trippelsdorf.de>
> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
> ---
> include/linux/mod_devicetable.h | 2 +-
> include/uapi/linux/input.h | 33 ++++++++++++++++++---------------
> 2 files changed, 19 insertions(+), 16 deletions(-)
>
> diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
> index 329aa30..45e9214 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 0x4f
> +#define INPUT_DEVICE_ID_ABS_MAX 0x3f
> #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 76457ee..12c33f8 100644
> --- a/include/uapi/linux/input.h
> +++ b/include/uapi/linux/input.h
> @@ -819,8 +819,24 @@ struct input_keymap_entry {
>
> #define ABS_VOLUME 0x20
>
> +/* Drums and guitars (mostly toys) */
> +#define ABS_TOM_FAR_LEFT 0x21
> +#define ABS_TOM_LEFT 0x22
> +#define ABS_TOM_RIGHT 0x23
> +#define ABS_TOM_FAR_RIGHT 0x24
> +#define ABS_CYMBAL_FAR_LEFT 0x25
> +#define ABS_CYMBAL_LEFT 0x26
> +#define ABS_CYMBAL_RIGHT 0x27
> +
> #define ABS_MISC 0x28
>
> +/* Drums and guitars continued */
> +#define ABS_CYMBAL_FAR_RIGHT 0x29
> +#define ABS_BASS 0x2a
> +#define ABS_HI_HAT 0x2b
> +#define ABS_FRET_BOARD 0x2c /* Guitar fret board, vertical pos */
> +#define ABS_WHAMMY_BAR 0x2d /* Guitar whammy bar (or vibrato) */
> +
I'm not particularly in favor of adding semantic between ABS_MISC and
ABS_MT_SLOT. We already did that once with the ABS_MT_* stuff, and the
problem comes from devices showing several axes, not really mapped
(like joysticks). These axes are all mapped to ABS_MISC, but the input
core function map them to ABS_MISC+N. This way some joysticks show
multitouch axes and are treated as such, whereas the axis are just
regular absolute axes. (not sure I am clear enough... :( )
Currently, nothing is done in X or in Wayland to detect these false
multitouch devices with a large number of absolute axes. But if we
keep a hole between ABS_MISC and ABS_MT_SLOT, we could add an
heuristic like:
"if all the axes between ABS_MISC and ABS_MT_SLOT are available, then
ABS_MT_SLOT does not mean multitouch, but ABS_MISC+6."
Cheers,
Benjamin
> #define ABS_MT_SLOT 0x2f /* MT slot being modified */
> #define ABS_MT_TOUCH_MAJOR 0x30 /* Major axis of touching ellipse */
> #define ABS_MT_TOUCH_MINOR 0x31 /* Minor axis (omit if circular) */
> @@ -837,21 +853,8 @@ struct input_keymap_entry {
> #define ABS_MT_TOOL_X 0x3c /* Center X tool position */
> #define ABS_MT_TOOL_Y 0x3d /* Center Y tool position */
>
> -/* Drums and guitars (mostly toys) */
> -#define ABS_TOM_FAR_LEFT 0x40
> -#define ABS_TOM_LEFT 0x41
> -#define ABS_TOM_RIGHT 0x42
> -#define ABS_TOM_FAR_RIGHT 0x43
> -#define ABS_CYMBAL_FAR_LEFT 0x44
> -#define ABS_CYMBAL_LEFT 0x45
> -#define ABS_CYMBAL_RIGHT 0x46
> -#define ABS_CYMBAL_FAR_RIGHT 0x47
> -#define ABS_BASS 0x48
> -#define ABS_HI_HAT 0x49
> -#define ABS_FRET_BOARD 0x4a /* Guitar fret board, vertical pos */
> -#define ABS_WHAMMY_BAR 0x4b /* Guitar whammy bar (or vibrato) */
> -
> -#define ABS_MAX 0x4f
> +/* EVIOCSABS() does not support ABS_MAX > 0x3f */
> +#define ABS_MAX 0x3f
> #define ABS_CNT (ABS_MAX+1)
>
> /*
> --
> 1.8.4
> --
> 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
* [PATCH] Input: i8042 - i8042_flush fix for a full 8042 buffer
From: Andrey Moiseev @ 2013-09-07 7:45 UTC (permalink / raw)
To: dmitry.torokhov; +Cc: linux-input, linux-kernel, Andrey Moiseev
When 8042 internal data buffer is full, the driver
erroneously decides that the controller is not present.
i8042_flush returns the number of flushed bytes, which is
in 0 - I8042_BUFFER_SIZE range inclusive. Therefore, i8042_flush
has no way to indicate an error. Moreover i8042_controller_check
takes initially full buffer (i8042_flush returned
I8042_BUFFER_SIZE) as a sign of absence of the controller.
The fix:
i8042_flush should perform one more status check after
I8042_BUFFER_SIZE bytes have been read, and, if it succeeds,
indicate the error by returning -1. i8042_controller_check
should be changed appropriately.
Signed-off-by: Andrey Moiseev <o2g.org.ru@gmail.com>
---
diff --git a/drivers/input/serio/i8042.c b/drivers/input/serio/i8042.c
index 78e4de4..cdda786 100644
--- a/drivers/input/serio/i8042.c
+++ b/drivers/input/serio/i8042.c
@@ -222,29 +222,32 @@ static int i8042_wait_write(void)
static int i8042_flush(void)
{
unsigned long flags;
unsigned char data, str;
int i = 0;
spin_lock_irqsave(&i8042_lock, flags);
- while (((str = i8042_read_status()) & I8042_STR_OBF) && (i < I8042_BUFFER_SIZE)) {
+ while (((str = i8042_read_status()) & I8042_STR_OBF) && (i <= I8042_BUFFER_SIZE)) {
udelay(50);
data = i8042_read_data();
i++;
dbg("%02x <- i8042 (flush, %s)\n",
data, str & I8042_STR_AUXDATA ? "aux" : "kbd");
}
spin_unlock_irqrestore(&i8042_lock, flags);
+ if (i == I8042_BUFFER_SIZE + 1)
+ return -1;
+
return i;
}
/*
* i8042_command() executes a command on the i8042. It also sends the input
* parameter(s) of the commands to it, and receives the output value(s). The
* parameters are to be stored in the param array, and the output is placed
* into the same array. The number of the parameters and output values is
* encoded in bits 8-11 of the command number.
*/
@@ -849,11 +852,11 @@ static int __init i8042_check_aux(void)
static int i8042_controller_check(void)
{
- if (i8042_flush() == I8042_BUFFER_SIZE) {
+ if (i8042_flush() == -1) {
pr_err("No controller found\n");
return -ENODEV;
}
return 0;
}
--
1.8.4
^ permalink raw reply related
* Re: [GIT] HID for 3.12 merge window
From: David Herrmann @ 2013-09-07 7:32 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Linus Torvalds, Markus Trippelsdorf, Jiri Kosina, linux-kernel,
open list:HID CORE LAYER
In-Reply-To: <CANq1E4Q85ewwKs7VoooZVapC7-Cvy6-Xp4rGL1-1Ee7w1EcT6w@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 2192 bytes --]
And this time also attached...
On Sat, Sep 7, 2013 at 9:31 AM, David Herrmann <dh.herrmann@gmail.com> wrote:
> Hi
>
> On Sat, Sep 7, 2013 at 5:22 AM, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
>> On Fri, Sep 06, 2013 at 06:00:29PM -0700, Linus Torvalds wrote:
>>> On Fri, Sep 6, 2013 at 5:58 PM, Dmitry Torokhov
>>> <dmitry.torokhov@gmail.com> wrote:
>>> >
>>> > The patch still had problems so I'd revert it and wii bits and try again later.
>>>
>>> Ok. Mind giving me a list of commits, so that I don't have to do a
>>> trial-and-error thing? I know the primary commit that causes problems,
>>> but there are commits that seem to depend on it..
>>
>>
>> Sorry for the delay. I believe you need to revert:
>>
>> 73f8645 HID: wiimote: add support for Guitar-Hero drums
>> 61e0065 Input: introduce BTN/ABS bits for drums and guitars
>>
>> Hmm... there also was "HID: wiimote: add support for Guitar-Hero
>> guitars" but I do not see it...
>
> The commits to revert are:
>
> commit 61e00655e9cb82e034eb72b95a51072e718d14a7
> Author: David Herrmann <dh.herrmann@gmail.com>
> Date: Mon Aug 26 19:14:46 2013 +0200
>
> Input: introduce BTN/ABS bits for drums and guitars
>
> commit 73f8645db1913ab2475ec3c1cee8d5f748963aa7
> Author: David Herrmann <dh.herrmann@gmail.com>
> Date: Mon Aug 26 19:14:47 2013 +0200
>
> HID: wiimote: add support for Guitar-Hero drums
>
> commit 8e22ecb603c88b7397ab2e80b7b0811b8a1318f5
> Author: Nicolas Adenis-Lamarre <nicolas.adenis.lamarre@gmail.com>
> Date: Mon Aug 26 19:14:48 2013 +0200
>
> HID: wiimote: add support for Guitar-Hero guitars
>
> Last one is not from me, so I guess that's why Dmitry missed it. And
> sorry for the delay, stupid UTC+1..
> The bug only occurs for multi-touch devices (their ABS_* bits are
>>0x1f), that's why I didn't see it happening during my tests..
>
> If you didn't revert it, yet, attached (and inlined) is a patch which
> reverts the ABS_MAX change and just moves the new identifiers in
> between the others. On top of 3.12-rc1, this should fix all issues. If
> you already reverted the patches, I guess we will just push it in
> linux-next again and wait for 3.13?
>
> Thanks
> David
[..snap..]
[-- Attachment #2: 0001-Revert-Fix-Input-introduce-BTN-ABS-bits-for-drums-an.patch --]
[-- Type: application/octet-stream, Size: 3540 bytes --]
From 13d549489b4bd2eade719cf2ec8c37724be6e640 Mon Sep 17 00:00:00 2001
From: David Herrmann <dh.herrmann@gmail.com>
Date: Sat, 7 Sep 2013 09:17:11 +0200
Subject: [PATCH] Revert/Fix "Input: introduce BTN/ABS bits for drums and
guitars"
commit 61e00655e9cb82e034eb72b95a51072e718d14a7
Author: David Herrmann <dh.herrmann@gmail.com>
Date: Mon Aug 26 19:14:46 2013 +0200
Input: introduce BTN/ABS bits for drums and guitars
This introduced several new identifiers for drums/guitar devices. However,
it also increased ABS_MAX. Unfortunately, 0x3f is already the maximum we
can use for ABS_MAX due to the limited EVIOCSABS ioctl.
Revert this commit and move the new identifiers somewhere in between. We
need to figure out some other way to add new identifiers for the future.
To avoid this happening again, I added a small comment.
This also fixes an issue where xf86-input-evdev failed on EVIOCGABS() due
to ABS_MAX not being a full mask.
Markus Trippelsdorf <markus@trippelsdorf.de>
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
include/linux/mod_devicetable.h | 2 +-
include/uapi/linux/input.h | 33 ++++++++++++++++++---------------
2 files changed, 19 insertions(+), 16 deletions(-)
diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
index 329aa30..45e9214 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 0x4f
+#define INPUT_DEVICE_ID_ABS_MAX 0x3f
#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 76457ee..12c33f8 100644
--- a/include/uapi/linux/input.h
+++ b/include/uapi/linux/input.h
@@ -819,8 +819,24 @@ struct input_keymap_entry {
#define ABS_VOLUME 0x20
+/* Drums and guitars (mostly toys) */
+#define ABS_TOM_FAR_LEFT 0x21
+#define ABS_TOM_LEFT 0x22
+#define ABS_TOM_RIGHT 0x23
+#define ABS_TOM_FAR_RIGHT 0x24
+#define ABS_CYMBAL_FAR_LEFT 0x25
+#define ABS_CYMBAL_LEFT 0x26
+#define ABS_CYMBAL_RIGHT 0x27
+
#define ABS_MISC 0x28
+/* Drums and guitars continued */
+#define ABS_CYMBAL_FAR_RIGHT 0x29
+#define ABS_BASS 0x2a
+#define ABS_HI_HAT 0x2b
+#define ABS_FRET_BOARD 0x2c /* Guitar fret board, vertical pos */
+#define ABS_WHAMMY_BAR 0x2d /* Guitar whammy bar (or vibrato) */
+
#define ABS_MT_SLOT 0x2f /* MT slot being modified */
#define ABS_MT_TOUCH_MAJOR 0x30 /* Major axis of touching ellipse */
#define ABS_MT_TOUCH_MINOR 0x31 /* Minor axis (omit if circular) */
@@ -837,21 +853,8 @@ struct input_keymap_entry {
#define ABS_MT_TOOL_X 0x3c /* Center X tool position */
#define ABS_MT_TOOL_Y 0x3d /* Center Y tool position */
-/* Drums and guitars (mostly toys) */
-#define ABS_TOM_FAR_LEFT 0x40
-#define ABS_TOM_LEFT 0x41
-#define ABS_TOM_RIGHT 0x42
-#define ABS_TOM_FAR_RIGHT 0x43
-#define ABS_CYMBAL_FAR_LEFT 0x44
-#define ABS_CYMBAL_LEFT 0x45
-#define ABS_CYMBAL_RIGHT 0x46
-#define ABS_CYMBAL_FAR_RIGHT 0x47
-#define ABS_BASS 0x48
-#define ABS_HI_HAT 0x49
-#define ABS_FRET_BOARD 0x4a /* Guitar fret board, vertical pos */
-#define ABS_WHAMMY_BAR 0x4b /* Guitar whammy bar (or vibrato) */
-
-#define ABS_MAX 0x4f
+/* EVIOCSABS() does not support ABS_MAX > 0x3f */
+#define ABS_MAX 0x3f
#define ABS_CNT (ABS_MAX+1)
/*
--
1.8.4
^ permalink raw reply related
* Re: [GIT] HID for 3.12 merge window
From: David Herrmann @ 2013-09-07 7:31 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Linus Torvalds, Markus Trippelsdorf, Jiri Kosina, linux-kernel,
open list:HID CORE LAYER
In-Reply-To: <20130907032240.GA1256@core.coreip.homeip.net>
Hi
On Sat, Sep 7, 2013 at 5:22 AM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On Fri, Sep 06, 2013 at 06:00:29PM -0700, Linus Torvalds wrote:
>> On Fri, Sep 6, 2013 at 5:58 PM, Dmitry Torokhov
>> <dmitry.torokhov@gmail.com> wrote:
>> >
>> > The patch still had problems so I'd revert it and wii bits and try again later.
>>
>> Ok. Mind giving me a list of commits, so that I don't have to do a
>> trial-and-error thing? I know the primary commit that causes problems,
>> but there are commits that seem to depend on it..
>
>
> Sorry for the delay. I believe you need to revert:
>
> 73f8645 HID: wiimote: add support for Guitar-Hero drums
> 61e0065 Input: introduce BTN/ABS bits for drums and guitars
>
> Hmm... there also was "HID: wiimote: add support for Guitar-Hero
> guitars" but I do not see it...
The commits to revert are:
commit 61e00655e9cb82e034eb72b95a51072e718d14a7
Author: David Herrmann <dh.herrmann@gmail.com>
Date: Mon Aug 26 19:14:46 2013 +0200
Input: introduce BTN/ABS bits for drums and guitars
commit 73f8645db1913ab2475ec3c1cee8d5f748963aa7
Author: David Herrmann <dh.herrmann@gmail.com>
Date: Mon Aug 26 19:14:47 2013 +0200
HID: wiimote: add support for Guitar-Hero drums
commit 8e22ecb603c88b7397ab2e80b7b0811b8a1318f5
Author: Nicolas Adenis-Lamarre <nicolas.adenis.lamarre@gmail.com>
Date: Mon Aug 26 19:14:48 2013 +0200
HID: wiimote: add support for Guitar-Hero guitars
Last one is not from me, so I guess that's why Dmitry missed it. And
sorry for the delay, stupid UTC+1..
The bug only occurs for multi-touch devices (their ABS_* bits are
>0x1f), that's why I didn't see it happening during my tests..
If you didn't revert it, yet, attached (and inlined) is a patch which
reverts the ABS_MAX change and just moves the new identifiers in
between the others. On top of 3.12-rc1, this should fix all issues. If
you already reverted the patches, I guess we will just push it in
linux-next again and wait for 3.13?
Thanks
David
(Inlined patch below, also attached to mail as gmail hates long lines)
>From 13d549489b4bd2eade719cf2ec8c37724be6e640 Mon Sep 17 00:00:00 2001
From: David Herrmann <dh.herrmann@gmail.com>
Date: Sat, 7 Sep 2013 09:17:11 +0200
Subject: [PATCH] Revert/Fix "Input: introduce BTN/ABS bits for drums and
guitars"
commit 61e00655e9cb82e034eb72b95a51072e718d14a7
Author: David Herrmann <dh.herrmann@gmail.com>
Date: Mon Aug 26 19:14:46 2013 +0200
Input: introduce BTN/ABS bits for drums and guitars
This introduced several new identifiers for drums/guitar devices. However,
it also increased ABS_MAX. Unfortunately, 0x3f is already the maximum we
can use for ABS_MAX due to the limited EVIOCSABS ioctl.
Revert this commit and move the new identifiers somewhere in between. We
need to figure out some other way to add new identifiers for the future.
To avoid this happening again, I added a small comment.
This also fixes an issue where xf86-input-evdev failed on EVIOCGABS() due
to ABS_MAX not being a full mask.
Markus Trippelsdorf <markus@trippelsdorf.de>
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
include/linux/mod_devicetable.h | 2 +-
include/uapi/linux/input.h | 33 ++++++++++++++++++---------------
2 files changed, 19 insertions(+), 16 deletions(-)
diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
index 329aa30..45e9214 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 0x4f
+#define INPUT_DEVICE_ID_ABS_MAX 0x3f
#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 76457ee..12c33f8 100644
--- a/include/uapi/linux/input.h
+++ b/include/uapi/linux/input.h
@@ -819,8 +819,24 @@ struct input_keymap_entry {
#define ABS_VOLUME 0x20
+/* Drums and guitars (mostly toys) */
+#define ABS_TOM_FAR_LEFT 0x21
+#define ABS_TOM_LEFT 0x22
+#define ABS_TOM_RIGHT 0x23
+#define ABS_TOM_FAR_RIGHT 0x24
+#define ABS_CYMBAL_FAR_LEFT 0x25
+#define ABS_CYMBAL_LEFT 0x26
+#define ABS_CYMBAL_RIGHT 0x27
+
#define ABS_MISC 0x28
+/* Drums and guitars continued */
+#define ABS_CYMBAL_FAR_RIGHT 0x29
+#define ABS_BASS 0x2a
+#define ABS_HI_HAT 0x2b
+#define ABS_FRET_BOARD 0x2c /* Guitar fret board, vertical pos */
+#define ABS_WHAMMY_BAR 0x2d /* Guitar whammy bar (or vibrato) */
+
#define ABS_MT_SLOT 0x2f /* MT slot being modified */
#define ABS_MT_TOUCH_MAJOR 0x30 /* Major axis of touching ellipse */
#define ABS_MT_TOUCH_MINOR 0x31 /* Minor axis (omit if circular) */
@@ -837,21 +853,8 @@ struct input_keymap_entry {
#define ABS_MT_TOOL_X 0x3c /* Center X tool position */
#define ABS_MT_TOOL_Y 0x3d /* Center Y tool position */
-/* Drums and guitars (mostly toys) */
-#define ABS_TOM_FAR_LEFT 0x40
-#define ABS_TOM_LEFT 0x41
-#define ABS_TOM_RIGHT 0x42
-#define ABS_TOM_FAR_RIGHT 0x43
-#define ABS_CYMBAL_FAR_LEFT 0x44
-#define ABS_CYMBAL_LEFT 0x45
-#define ABS_CYMBAL_RIGHT 0x46
-#define ABS_CYMBAL_FAR_RIGHT 0x47
-#define ABS_BASS 0x48
-#define ABS_HI_HAT 0x49
-#define ABS_FRET_BOARD 0x4a /* Guitar fret board, vertical pos */
-#define ABS_WHAMMY_BAR 0x4b /* Guitar whammy bar (or vibrato) */
-
-#define ABS_MAX 0x4f
+/* EVIOCSABS() does not support ABS_MAX > 0x3f */
+#define ABS_MAX 0x3f
#define ABS_CNT (ABS_MAX+1)
/*
--
1.8.4
^ permalink raw reply related
* [git pull] Input updates for 3.12-rc0
From: Dmitry Torokhov @ 2013-09-07 3:34 UTC (permalink / raw)
To: Linus Torvalds; +Cc: linux-kernel, linux-input
[-- Attachment #1: Type: text/plain, Size: 4956 bytes --]
Hi Linus,
Please pull from:
git://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git for-linus
or
master.kernel.org:/pub/scm/linux/kernel/git/dtor/input.git for-linus
to receive updates for the input subsystem. You will get a new driver
for slidebar on Ideapad laptops and a bunch of assorted driver fixes.
Changelog:
---------
Andrey Moiseev (1):
Input: add driver for slidebar on Lenovo IdeaPad laptops
Bo Shen (1):
Input: qt1070 - add power management ops
David Herrmann (1):
Input: add SYN_MAX and SYN_CNT constants
Fabio Estevam (2):
Input: egalax-ts - fix typo and improve text
Input: max11801_ts - convert to devm
Geert Uytterhoeven (1):
Input: cyttsp4 - kill 'defined but not used' compiler warnings
Illia Smyrnov (5):
Input: omap-keypad - use bitfiled instead of hardcoded values
Input: omap-keypad - convert to threaded IRQ
Input: omap-keypad - clear interrupts on open
Input: omap-keypad - enable wakeup capability for keypad.
Input: omap-keypad - set up irq type from DT
Javier Martinez Canillas (1):
Input: MAINTAINERS - change maintainer for cyttsp driver
Jingoo Han (5):
Input: max7359 - add CONFIG_PM_SLEEP to suspend/resume
Input: cy8ctmg110_ts - add CONFIG_PM_SLEEP to suspend/resume
Input: eeti_ts - add CONFIG_PM_SLEEP to suspend/resume
Input: pwm-beeper - add CONFIG_PM_SLEEP to suspend/resume
Input: joysticks - use dev_get_platdata()
Julia Lawall (2):
Input: tegra-kbc - simplify use of devm_ioremap_resource
Input: keyboard, serio - simplify use of devm_ioremap_resource
Mathieu J. Poirier (1):
Input: sysrq - DT binding for key sequence
Peter Ujfalusi (1):
Input: twl6040-vibra - remove support for legacy (pdata) mode
Ping Cheng (1):
Input: wacom - integrate resolution calculation
Sachin Kamat (4):
Input: wistron_btns - fix incorrect placement of __initconst
Input: lifebook - fix incorrect placement of __initconst
Input: synaptics - fix incorrect placement of __initconst
Input: htcpen - fix incorrect placement of __initdata
Stefan Lippers-Hollmann (3):
Input: wistron_btns - drop bogus MODULE_VERSION macro
Input: wistron_btns - mark the Medion MD96500 keymap as tested
Input: wistron_btns - add MODULE_DEVICE_TABLE
Wei Yongjun (3):
Input: as5011 - fix error return code in as5011_probe()
Input: wacom - fix error return code in wacom_probe()
Input: cyttsp4 - remove useless NULL test from cyttsp4_watchdog_timer()
Diffstat:
--------
.../devicetree/bindings/input/input-reset.txt | 33 ++
.../bindings/input/touchscreen/egalax-ts.txt | 2 +-
MAINTAINERS | 11 +-
drivers/input/joystick/as5011.c | 3 +-
drivers/input/joystick/maplecontrol.c | 4 +-
drivers/input/keyboard/imx_keypad.c | 7 +-
drivers/input/keyboard/max7359_keypad.c | 2 +-
drivers/input/keyboard/nspire-keypad.c | 7 +-
drivers/input/keyboard/omap4-keypad.c | 95 ++++--
drivers/input/keyboard/qt1070.c | 27 ++
drivers/input/keyboard/spear-keyboard.c | 7 +-
drivers/input/keyboard/tegra-kbc.c | 7 +-
drivers/input/misc/Kconfig | 10 +
drivers/input/misc/Makefile | 1 +
drivers/input/misc/ideapad_slidebar.c | 358 +++++++++++++++++++++
drivers/input/misc/pwm-beeper.c | 2 +-
drivers/input/misc/twl6040-vibra.c | 41 +--
drivers/input/misc/wistron_btns.c | 6 +-
drivers/input/mouse/lifebook.c | 2 +-
drivers/input/mouse/synaptics.c | 4 +-
drivers/input/serio/arc_ps2.c | 7 +-
drivers/input/serio/i8042.h | 24 --
drivers/input/serio/olpc_apsp.c | 3 -
drivers/input/tablet/wacom_sys.c | 87 +++--
drivers/input/tablet/wacom_wac.c | 19 +-
drivers/input/touchscreen/cy8ctmg110_ts.c | 6 +-
drivers/input/touchscreen/cyttsp4_core.c | 203 ++++++------
drivers/input/touchscreen/eeti_ts.c | 6 +-
drivers/input/touchscreen/htcpen.c | 2 +-
drivers/input/touchscreen/max11801_ts.c | 37 +--
drivers/tty/sysrq.c | 42 +++
include/linux/i8042.h | 24 ++
include/uapi/linux/input.h | 2 +
33 files changed, 770 insertions(+), 321 deletions(-)
create mode 100644 Documentation/devicetree/bindings/input/input-reset.txt
create mode 100644 drivers/input/misc/ideapad_slidebar.c
--
Dmitry
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [GIT] HID for 3.12 merge window
From: Dmitry Torokhov @ 2013-09-07 3:22 UTC (permalink / raw)
To: Linus Torvalds
Cc: David Herrmann, Markus Trippelsdorf, Jiri Kosina, linux-kernel,
open list:HID CORE LAYER
In-Reply-To: <CA+55aFzLy+-VF022ORuyWk-qQO7gDSQ1LZES1+bEUxz0QxrH-g@mail.gmail.com>
On Fri, Sep 06, 2013 at 06:00:29PM -0700, Linus Torvalds wrote:
> On Fri, Sep 6, 2013 at 5:58 PM, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> >
> > The patch still had problems so I'd revert it and wii bits and try again later.
>
> Ok. Mind giving me a list of commits, so that I don't have to do a
> trial-and-error thing? I know the primary commit that causes problems,
> but there are commits that seem to depend on it..
Sorry for the delay. I believe you need to revert:
73f8645 HID: wiimote: add support for Guitar-Hero drums
61e0065 Input: introduce BTN/ABS bits for drums and guitars
Hmm... there also was "HID: wiimote: add support for Guitar-Hero
guitars" but I do not see it...
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [GIT] HID for 3.12 merge window
From: Linus Torvalds @ 2013-09-07 1:00 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: David Herrmann, Markus Trippelsdorf, Jiri Kosina, linux-kernel,
open list:HID CORE LAYER
In-Reply-To: <506b9d43-f734-40f9-b137-0749a496fd6d@email.android.com>
On Fri, Sep 6, 2013 at 5:58 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> The patch still had problems so I'd revert it and wii bits and try again later.
Ok. Mind giving me a list of commits, so that I don't have to do a
trial-and-error thing? I know the primary commit that causes problems,
but there are commits that seem to depend on it..
Linus
^ permalink raw reply
* Re: [GIT] HID for 3.12 merge window
From: Dmitry Torokhov @ 2013-09-07 0:58 UTC (permalink / raw)
To: Linus Torvalds, David Herrmann
Cc: Markus Trippelsdorf, Jiri Kosina, linux-kernel,
open list:HID CORE LAYER
In-Reply-To: <CA+55aFzg-dv-N7UjXx2pxUJ+QQOOy0qtp_e94PDSM=tVd=LgUw@mail.gmail.com>
Linus Torvalds <torvalds@linux-foundation.org> wrote:
>On Fri, Sep 6, 2013 at 2:50 PM, David Herrmann <dh.herrmann@gmail.com>
>wrote:
>> Hi
>>
>> On Fri, Sep 6, 2013 at 10:20 PM, Markus Trippelsdorf
>>>
>>> commit 61e00655e9cb82e034eb72b95a51072e718d14a7
>>> Author: David Herrmann <dh.herrmann@gmail.com>
>>> Date: Mon Aug 26 19:14:46 2013 +0200
>>>
>>> Input: introduce BTN/ABS bits for drums and guitars
>>>
>>> The commit above breaks my Logitech mouse. The mouse cursor just
>sits in
>>> the middle of the screen and doesn't react to movements. dmesg is
>>> normal, but Xorg.0.log says:
>>
>> Ok, the issue is the kernel assumes ABS_MAX to be a power-of-2 minus
>1
>> (used as mask). That wasn't really obvious to me. Attached is a patch
>> which should fix that. Could you apply it on top of linus/master and
>> give it a try?
>
>Gah. I just wasted too much time bisecting down my logitech wireless
>keyboard not working to within a few commits of this, and decided to
>just try your patch.
>
>And yes, it makes my keyboard work.
>
>Dmitry, should I just apply the patch, or should we revert and use
>other bits? Please, this needs to be resolved, I stopped merging when
>I noticed this problem..
The patch still had problems so I'd revert it and wii bits and try again later.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [GIT] HID for 3.12 merge window
From: Linus Torvalds @ 2013-09-06 23:57 UTC (permalink / raw)
To: David Herrmann
Cc: Markus Trippelsdorf, Jiri Kosina, linux-kernel,
open list:HID CORE LAYER, Dmitry Torokhov
In-Reply-To: <CANq1E4Rf4c4zj1mD1aPr93Q2_M65DCEJhuLqcBWa5uH9tcKuWA@mail.gmail.com>
On Fri, Sep 6, 2013 at 2:50 PM, David Herrmann <dh.herrmann@gmail.com> wrote:
> Hi
>
> On Fri, Sep 6, 2013 at 10:20 PM, Markus Trippelsdorf
>>
>> commit 61e00655e9cb82e034eb72b95a51072e718d14a7
>> Author: David Herrmann <dh.herrmann@gmail.com>
>> Date: Mon Aug 26 19:14:46 2013 +0200
>>
>> Input: introduce BTN/ABS bits for drums and guitars
>>
>> The commit above breaks my Logitech mouse. The mouse cursor just sits in
>> the middle of the screen and doesn't react to movements. dmesg is
>> normal, but Xorg.0.log says:
>
> Ok, the issue is the kernel assumes ABS_MAX to be a power-of-2 minus 1
> (used as mask). That wasn't really obvious to me. Attached is a patch
> which should fix that. Could you apply it on top of linus/master and
> give it a try?
Gah. I just wasted too much time bisecting down my logitech wireless
keyboard not working to within a few commits of this, and decided to
just try your patch.
And yes, it makes my keyboard work.
Dmitry, should I just apply the patch, or should we revert and use
other bits? Please, this needs to be resolved, I stopped merging when
I noticed this problem..
Linus
^ 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