* Re: [PATCH 1/1] Input: s3c2410 - Trivial cleanup in header file
From: Dmitry Torokhov @ 2014-02-14 7:57 UTC (permalink / raw)
To: Sachin Kamat; +Cc: linux-input, trivial
In-Reply-To: <1392361539-24397-1-git-send-email-sachin.kamat@linaro.org>
On Fri, Feb 14, 2014 at 12:35:39PM +0530, Sachin Kamat wrote:
> Commit 436d42c61c3e ("ARM: samsung: move platform_data definitions")
> moved the files to the current location but forgot to remove the pointer
> to its previous location. Clean it up. While at it also change the header
> file protection macros appropriately and remove whitespace errors.
>
> Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org>
Applied, thank you.
> ---
> include/linux/platform_data/touchscreen-s3c2410.h | 17 ++++++++---------
> 1 file changed, 8 insertions(+), 9 deletions(-)
>
> diff --git a/include/linux/platform_data/touchscreen-s3c2410.h b/include/linux/platform_data/touchscreen-s3c2410.h
> index 26fdb22e0fc2..58dc7c5ae63b 100644
> --- a/include/linux/platform_data/touchscreen-s3c2410.h
> +++ b/include/linux/platform_data/touchscreen-s3c2410.h
> @@ -1,5 +1,4 @@
> -/* arch/arm/plat-samsung/include/plat/ts.h
> - *
> +/*
> * Copyright (c) 2005 Arnaud Patard <arnaud.patard@rtp-net.org>
> *
> * This program is free software; you can redistribute it and/or modify
> @@ -7,14 +6,14 @@
> * published by the Free Software Foundation.
> */
>
> -#ifndef __ASM_ARM_TS_H
> -#define __ASM_ARM_TS_H
> +#ifndef __TOUCHSCREEN_S3C2410_H
> +#define __TOUCHSCREEN_S3C2410_H
>
> struct s3c2410_ts_mach_info {
> - int delay;
> - int presc;
> - int oversampling_shift;
> - void (*cfg_gpio)(struct platform_device *dev);
> + int delay;
> + int presc;
> + int oversampling_shift;
> + void (*cfg_gpio)(struct platform_device *dev);
> };
>
> extern void s3c24xx_ts_set_platdata(struct s3c2410_ts_mach_info *);
> @@ -22,4 +21,4 @@ extern void s3c24xx_ts_set_platdata(struct s3c2410_ts_mach_info *);
> /* defined by architecture to configure gpio */
> extern void s3c24xx_ts_cfg_gpio(struct platform_device *dev);
>
> -#endif /* __ASM_ARM_TS_H */
> +#endif /*__TOUCHSCREEN_S3C2410_H */
> --
> 1.7.9.5
>
--
Dmitry
^ permalink raw reply
* Re: [PATCH v3] input: sirfsoc-onkey - report onkey untouch event by detecting pin status
From: Barry Song @ 2014-02-14 8:49 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: linux-input@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
DL-SHA-WorkGroupLinux, Xianglong Du, Rongjun Ying, Barry Song
In-Reply-To: <20140214075717.GA14682@core.coreip.homeip.net>
2014-02-14 15:57 GMT+08:00 Dmitry Torokhov <dmitry.torokhov@gmail.com>:
> Hi Barry,
>
> On Fri, Feb 14, 2014 at 11:20:01AM +0800, Barry Song wrote:
>> From: Xianglong Du <Xianglong.Du@csr.com>
>>
>> this patch adds a delayed_work to detect the untouch of onkey since HW will
>> not generate interrupt for it.
>>
>> at the same time, we move the KEY event to POWER instead of SUSPEND, which
>> will be suitable for both Android and Linux. Userspace PowerManager Daemon
>> will decide to suspend or shutdown based on how long we have touched onkey
>>
>> Signed-off-by: Xianglong Du <Xianglong.Du@csr.com>
>> Signed-off-by: Rongjun Ying <Rongjun.Ying@csr.com>
>> Signed-off-by: Barry Song <Baohua.Song@csr.com>
>> ---
>> -v3: move to use custom devres action
>
> Thank you for making the changes, however it seems that we can control
> whether the device generates interrupts or not and so we can implement
> open and close methods. If patch below works then your
> cancel_delayed_work() call should go into sirfosc_pwrc_close() and we do
> not need to use devm_free_irq() not custom action.
this one looks making lots of senses. it makes sure HW will not
trigger any SW behaviour before probe() finishes, and also makes sure
HW will not trigger any SW behaviour in remove(). i'd like xianglong
to give a quick test on it.
>
> Thanks.
>
> --
> Dmitry
>
>
> Input: sirfsoc-onkey - implement open and close methods
>
> From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>
> We can control whetehr device generates interrupts or not so let's
> implement open and close methods of input device so that we do not do any
> processing until there are users.
>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
> drivers/input/misc/sirfsoc-onkey.c | 50 +++++++++++++++++++++++++++++-------
> 1 file changed, 40 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/input/misc/sirfsoc-onkey.c b/drivers/input/misc/sirfsoc-onkey.c
> index e8897c3..dc7db65 100644
> --- a/drivers/input/misc/sirfsoc-onkey.c
> +++ b/drivers/input/misc/sirfsoc-onkey.c
> @@ -49,6 +49,35 @@ static irqreturn_t sirfsoc_pwrc_isr(int irq, void *dev_id)
> return IRQ_HANDLED;
> }
>
> +static void sirfsoc_pwrc_toggle_interrupts(struct sirfsoc_pwrc_drvdata *pwrcdrv,
> + bool enable)
> +{
> + u32 int_mask;
> +
> + int_mask = sirfsoc_rtc_iobrg_readl(pwrcdrv->pwrc_base + PWRC_INT_MASK);
> + if (enable)
> + int_mask |= PWRC_ON_KEY_BIT;
> + else
> + int_mask &= ~PWRC_ON_KEY_BIT;
> + sirfsoc_rtc_iobrg_writel(int_mask, pwrcdrv->pwrc_base + PWRC_INT_MASK);
> +}
> +
> +static int sirfsoc_pwrc_open(struct input_dev *input)
> +{
> + struct sirfsoc_pwrc_drvdata *pwrcdrv = input_get_drvdata(input);
> +
> + sirfsoc_pwrc_toggle_interrupts(pwrcdrv, true);
> +
> + return 0;
> +}
> +
> +static void sirfsoc_pwrc_close(struct input_dev *input)
> +{
> + struct sirfsoc_pwrc_drvdata *pwrcdrv = input_get_drvdata(input);
> +
> + sirfsoc_pwrc_toggle_interrupts(pwrcdrv, false);
> +}
> +
> static const struct of_device_id sirfsoc_pwrc_of_match[] = {
> { .compatible = "sirf,prima2-pwrc" },
> {},
> @@ -70,7 +99,7 @@ static int sirfsoc_pwrc_probe(struct platform_device *pdev)
> }
>
> /*
> - * we can't use of_iomap because pwrc is not mapped in memory,
> + * We can't use of_iomap because pwrc is not mapped in memory,
> * the so-called base address is only offset in rtciobrg
> */
> error = of_property_read_u32(np, "reg", &pwrcdrv->pwrc_base);
> @@ -88,6 +117,11 @@ static int sirfsoc_pwrc_probe(struct platform_device *pdev)
> pwrcdrv->input->phys = "pwrc/input0";
> pwrcdrv->input->evbit[0] = BIT_MASK(EV_PWR);
>
> + pwrcdrv->input->open = sirfsoc_pwrc_open;
> + pwrcdrv->input->close = sirfsoc_pwrc_close;
> +
> + input_set_drvdata(pwrcdrv->input, pwrcdrv);
> +
> irq = platform_get_irq(pdev, 0);
> error = devm_request_irq(&pdev->dev, irq,
> sirfsoc_pwrc_isr, IRQF_SHARED,
> @@ -98,11 +132,6 @@ static int sirfsoc_pwrc_probe(struct platform_device *pdev)
> return error;
> }
>
> - sirfsoc_rtc_iobrg_writel(
> - sirfsoc_rtc_iobrg_readl(pwrcdrv->pwrc_base + PWRC_INT_MASK) |
> - PWRC_ON_KEY_BIT,
> - pwrcdrv->pwrc_base + PWRC_INT_MASK);
> -
> error = input_register_device(pwrcdrv->input);
> if (error) {
> dev_err(&pdev->dev,
> @@ -129,15 +158,16 @@ static int pwrc_resume(struct device *dev)
> {
> struct platform_device *pdev = to_platform_device(dev);
> struct sirfsoc_pwrc_drvdata *pwrcdrv = platform_get_drvdata(pdev);
> + struct input_dev *input = pwrcdrv->input;
>
> /*
> * Do not mask pwrc interrupt as we want pwrc work as a wakeup source
> * if users touch X_ONKEY_B, see arch/arm/mach-prima2/pm.c
> */
> - sirfsoc_rtc_iobrg_writel(
> - sirfsoc_rtc_iobrg_readl(
> - pwrcdrv->pwrc_base + PWRC_INT_MASK) | PWRC_ON_KEY_BIT,
> - pwrcdrv->pwrc_base + PWRC_INT_MASK);
> + mutex_lock(&input->mutex);
> + if (input->users)
> + sirfsoc_pwrc_toggle_interrupts(pwrcdrv, true);
> + mutex_unlock(&input->mutex);
>
> return 0;
> }
-barry
^ permalink raw reply
* RE: [PATCH v3] input: sirfsoc-onkey - report onkey untouch event by detecting pin status
From: Xianglong Du @ 2014-02-14 10:32 UTC (permalink / raw)
To: Barry Song, Dmitry Torokhov
Cc: Rongjun Ying, DL-SHA-WorkGroupLinux, Barry Song,
linux-arm-kernel@lists.infradead.org, linux-input@vger.kernel.org
In-Reply-To: <CAGsJ_4xqnZAO6vpkgTEHmfp9+nbWa1qwQ4pda6B2sSrEtbV3ZA@mail.gmail.com>
Tested-by: Xianglong Du <Xianglong.Du@csr.com>
________________________________________
From: Barry Song [21cnbao@gmail.com]
Sent: Friday, February 14, 2014 16:49
To: Dmitry Torokhov
Cc: linux-input@vger.kernel.org; linux-arm-kernel@lists.infradead.org; DL-SHA-WorkGroupLinux; Xianglong Du; Rongjun Ying; Barry Song
Subject: Re: [PATCH v3] input: sirfsoc-onkey - report onkey untouch event by detecting pin status
2014-02-14 15:57 GMT+08:00 Dmitry Torokhov <dmitry.torokhov@gmail.com>:
> Hi Barry,
>
> On Fri, Feb 14, 2014 at 11:20:01AM +0800, Barry Song wrote:
>> From: Xianglong Du <Xianglong.Du@csr.com>
>>
>> this patch adds a delayed_work to detect the untouch of onkey since HW will
>> not generate interrupt for it.
>>
>> at the same time, we move the KEY event to POWER instead of SUSPEND, which
>> will be suitable for both Android and Linux. Userspace PowerManager Daemon
>> will decide to suspend or shutdown based on how long we have touched onkey
>>
>> Signed-off-by: Xianglong Du <Xianglong.Du@csr.com>
>> Signed-off-by: Rongjun Ying <Rongjun.Ying@csr.com>
>> Signed-off-by: Barry Song <Baohua.Song@csr.com>
>> ---
>> -v3: move to use custom devres action
>
> Thank you for making the changes, however it seems that we can control
> whether the device generates interrupts or not and so we can implement
> open and close methods. If patch below works then your
> cancel_delayed_work() call should go into sirfosc_pwrc_close() and we do
> not need to use devm_free_irq() not custom action.
this one looks making lots of senses. it makes sure HW will not
trigger any SW behaviour before probe() finishes, and also makes sure
HW will not trigger any SW behaviour in remove(). i'd like xianglong
to give a quick test on it.
>
> Thanks.
>
> --
> Dmitry
>
>
> Input: sirfsoc-onkey - implement open and close methods
>
> From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>
> We can control whetehr device generates interrupts or not so let's
> implement open and close methods of input device so that we do not do any
> processing until there are users.
>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
> drivers/input/misc/sirfsoc-onkey.c | 50 +++++++++++++++++++++++++++++-------
> 1 file changed, 40 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/input/misc/sirfsoc-onkey.c b/drivers/input/misc/sirfsoc-onkey.c
> index e8897c3..dc7db65 100644
> --- a/drivers/input/misc/sirfsoc-onkey.c
> +++ b/drivers/input/misc/sirfsoc-onkey.c
> @@ -49,6 +49,35 @@ static irqreturn_t sirfsoc_pwrc_isr(int irq, void *dev_id)
> return IRQ_HANDLED;
> }
>
> +static void sirfsoc_pwrc_toggle_interrupts(struct sirfsoc_pwrc_drvdata *pwrcdrv,
> + bool enable)
> +{
> + u32 int_mask;
> +
> + int_mask = sirfsoc_rtc_iobrg_readl(pwrcdrv->pwrc_base + PWRC_INT_MASK);
> + if (enable)
> + int_mask |= PWRC_ON_KEY_BIT;
> + else
> + int_mask &= ~PWRC_ON_KEY_BIT;
> + sirfsoc_rtc_iobrg_writel(int_mask, pwrcdrv->pwrc_base + PWRC_INT_MASK);
> +}
> +
> +static int sirfsoc_pwrc_open(struct input_dev *input)
> +{
> + struct sirfsoc_pwrc_drvdata *pwrcdrv = input_get_drvdata(input);
> +
> + sirfsoc_pwrc_toggle_interrupts(pwrcdrv, true);
> +
> + return 0;
> +}
> +
> +static void sirfsoc_pwrc_close(struct input_dev *input)
> +{
> + struct sirfsoc_pwrc_drvdata *pwrcdrv = input_get_drvdata(input);
> +
> + sirfsoc_pwrc_toggle_interrupts(pwrcdrv, false);
> +}
> +
> static const struct of_device_id sirfsoc_pwrc_of_match[] = {
> { .compatible = "sirf,prima2-pwrc" },
> {},
> @@ -70,7 +99,7 @@ static int sirfsoc_pwrc_probe(struct platform_device *pdev)
> }
>
> /*
> - * we can't use of_iomap because pwrc is not mapped in memory,
> + * We can't use of_iomap because pwrc is not mapped in memory,
> * the so-called base address is only offset in rtciobrg
> */
> error = of_property_read_u32(np, "reg", &pwrcdrv->pwrc_base);
> @@ -88,6 +117,11 @@ static int sirfsoc_pwrc_probe(struct platform_device *pdev)
> pwrcdrv->input->phys = "pwrc/input0";
> pwrcdrv->input->evbit[0] = BIT_MASK(EV_PWR);
>
> + pwrcdrv->input->open = sirfsoc_pwrc_open;
> + pwrcdrv->input->close = sirfsoc_pwrc_close;
> +
> + input_set_drvdata(pwrcdrv->input, pwrcdrv);
> +
> irq = platform_get_irq(pdev, 0);
> error = devm_request_irq(&pdev->dev, irq,
> sirfsoc_pwrc_isr, IRQF_SHARED,
> @@ -98,11 +132,6 @@ static int sirfsoc_pwrc_probe(struct platform_device *pdev)
> return error;
> }
>
> - sirfsoc_rtc_iobrg_writel(
> - sirfsoc_rtc_iobrg_readl(pwrcdrv->pwrc_base + PWRC_INT_MASK) |
> - PWRC_ON_KEY_BIT,
> - pwrcdrv->pwrc_base + PWRC_INT_MASK);
> -
> error = input_register_device(pwrcdrv->input);
> if (error) {
> dev_err(&pdev->dev,
> @@ -129,15 +158,16 @@ static int pwrc_resume(struct device *dev)
> {
> struct platform_device *pdev = to_platform_device(dev);
> struct sirfsoc_pwrc_drvdata *pwrcdrv = platform_get_drvdata(pdev);
> + struct input_dev *input = pwrcdrv->input;
>
> /*
> * Do not mask pwrc interrupt as we want pwrc work as a wakeup source
> * if users touch X_ONKEY_B, see arch/arm/mach-prima2/pm.c
> */
> - sirfsoc_rtc_iobrg_writel(
> - sirfsoc_rtc_iobrg_readl(
> - pwrcdrv->pwrc_base + PWRC_INT_MASK) | PWRC_ON_KEY_BIT,
> - pwrcdrv->pwrc_base + PWRC_INT_MASK);
> + mutex_lock(&input->mutex);
> + if (input->users)
> + sirfsoc_pwrc_toggle_interrupts(pwrcdrv, true);
> + mutex_unlock(&input->mutex);
>
> return 0;
> }
-barry
To report this email as spam click https://www.mailcontrol.com/sr/MZbqvYs5QwJvpeaetUwhCQ== .
Member of the CSR plc group of companies. CSR plc registered in England and Wales, registered number 4187346, registered office Churchill House, Cambridge Business Park, Cowley Road, Cambridge, CB4 0WZ, United Kingdom
More information can be found at www.csr.com. Keep up to date with CSR on our technical blog, www.csr.com/blog, CSR people blog, www.csr.com/people, YouTube, www.youtube.com/user/CSRplc, Facebook, www.facebook.com/pages/CSR/191038434253534, or follow us on Twitter at www.twitter.com/CSR_plc.
New for 2014, you can now access the wide range of products powered by aptX at www.aptx.com.
^ permalink raw reply
* Re: [PATCH 1/5] HID: logitech-dj: Fix USB 3.0 issue
From: Nestor Lopez Casado @ 2014-02-14 13:35 UTC (permalink / raw)
To: Jiri Kosina
Cc: Benjamin Tissoires, Benjamin Tissoires, Andrew de los Reyes,
open list:HID CORE LAYER, linux-kernel@vger.kernel.org
In-Reply-To: <CAE7qMrpEnF3y-dVP14190y33YNidCdjtJeO9PcMVcBBJDJsYuw@mail.gmail.com>
We discussed this issue with one of the receiver firmware developers today.
We do see a clear issue when more than one DJ command (those with
report id 0x20, 0x21) are sent to the receiver in quick sequence. The
second command can be trashed if the first is still under processing,
and there is no way for the host to know when the receiver has
actually finished processing the first DJ command.
Both the first and the second commands are ACKed at the USB level.
For hidpp commands (those with report id 0x10, 0x11) there is a
response sent from the receiver to the host, and the host should wait
for the response before sending a new command.
For some DJ commands (those with report id 0x20, 0x21) there is no
response at all, as is the case for the 0x80 (switch to dj mode with
keep alive)
So the ugly sleep accounts to workaround this issue on the receiver.
There may be a cleaner workaround that could be tested (I did not test
myself, but looking at the receiver code it does seem as a plausible
solution)
Invert the order in which the commands are sent to the receiver:
First send the CMD_GET_PAIRED_DEVICES (0x81) to the receiver, and wait
to get all the device paired notifications, (the last notification is
specially tagged), then send the CMD_SWITCH (0x80).
Using this mechanism we guarantee that the receiver never sees a
second command while still processing the previous one and no explicit
sleep will be required.
For me, the ugly sleep suffices, as long as we understand the root
cause of the problem.
Cheers,
-nestor
On Fri, Feb 14, 2014 at 2:32 PM, Nestor Lopez Casado
<nlopezcasad@logitech.com> wrote:
> We discussed this issue with one of the receiver firmware developers today.
>
> We do see a clear issue when more than one DJ command (those with report id
> 0x20, 0x21) are sent to the receiver in quick sequence. The second command
> can be trashed if the first is still under processing, and there is no way
> for the host to know when the receiver has actually finished processing the
> first DJ command.
>
> Both the first and the second commands are ACKed at the USB level.
>
> For hidpp commands (those with report id 0x10, 0x11) there is a response
> sent from the receiver to the host, and the host should wait for the
> response before sending a new command.
>
> For some DJ commands (those with report id 0x20, 0x21) there is no response
> at all, as is the case for the 0x80 (switch to dj mode with keep alive)
>
> So the ugly sleep accounts to workaround this issue on the receiver.
>
> There may be a cleaner workaround that could be tested (I did not test
> myself, but looking at the receiver code it does seem as a plausible
> solution)
>
> Invert the order in which the commands are sent to the receiver:
>
> First send the CMD_GET_PAIRED_DEVICES (0x81) to the receiver, and wait to
> get all the device paired notifications, (the last notification is specially
> tagged), then send the CMD_SWITCH (0x80).
> Using this mechanism we guarantee that the receiver never sees a second
> command while still processing the previous one and no explicit sleep will
> be required.
>
> For me, the ugly sleep suffices, as long as we understand the root cause of
> the problem.
>
> Cheers,
> -nestor
>
>
> On Fri, Jan 17, 2014 at 9:48 AM, Nestor Lopez Casado
> <nlopezcasad@logitech.com> wrote:
>>
>> On Thu, Jan 16, 2014 at 10:50 PM, Jiri Kosina <jkosina@suse.cz> wrote:
>> > On Wed, 8 Jan 2014, Benjamin Tissoires wrote:
>> >
>> >> From: Benjamin Tisssoires <benjamin.tissoires@redhat.com>
>> >>
>> >> This fix (not very clean though) should fix the long time USB3
>> >> issue that was spotted last year. The rational has been given by
>> >> Hans de Goede:
>> >
>> > Man this is so ugly ... makes one wonder how come that other OSes don't
>> > suffer from this. Are they really *that much* slower? :)
>> >
>> > I have applied this (and just this) one for 3.14.
>>
>> This is clearly a workaround for an underlying bug.
>> Most probably the issue is in the receiver firmware, and the bug
>> happens to become apparent easier when it is plugged in a usb 3.0
>> port.
>> We are experiencing some issues when the receiver is plugged to usb
>> 3.0 ports in other OSes as well, these could also be explained by the
>> same phenomenon of vendor requests being missed by the receiver. But
>> we dont have a clear understanding of the problem yet.
>>
>> >
>> > Thanks,
>> >
>> > --
>> > Jiri Kosina
>> > SUSE Labs
>> Cheers,
>> -nestor
>
>
^ permalink raw reply
* Re: [PATCH v3] input: sirfsoc-onkey - report onkey untouch event by detecting pin status
From: Barry Song @ 2014-02-14 13:41 UTC (permalink / raw)
To: Xianglong Du
Cc: Dmitry Torokhov, linux-input@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, DL-SHA-WorkGroupLinux,
Rongjun Ying, Barry Song
In-Reply-To: <847BC5012124A94BB5CA1B26E022CCA3012BAFDC27@SHAASIEXM01.ASIA.ROOT.PRI>
>> Input: sirfsoc-onkey - implement open and close methods
>>
>> From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>>
>> We can control whetehr device generates interrupts or not so let's
>> implement open and close methods of input device so that we do not do any
>> processing until there are users.
>>
>> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Tested-by: Xianglong Du <Xianglong.Du@csr.com>
Dmitry, will you push this one to your tree so that i can rebase others?
>> ---
>> drivers/input/misc/sirfsoc-onkey.c | 50 +++++++++++++++++++++++++++++-------
>> 1 file changed, 40 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/input/misc/sirfsoc-onkey.c b/drivers/input/misc/sirfsoc-onkey.c
>> index e8897c3..dc7db65 100644
>> --- a/drivers/input/misc/sirfsoc-onkey.c
>> +++ b/drivers/input/misc/sirfsoc-onkey.c
>> @@ -49,6 +49,35 @@ static irqreturn_t sirfsoc_pwrc_isr(int irq, void *dev_id)
>> return IRQ_HANDLED;
>> }
>>
>> +static void sirfsoc_pwrc_toggle_interrupts(struct sirfsoc_pwrc_drvdata *pwrcdrv,
>> + bool enable)
>> +{
>> + u32 int_mask;
>> +
>> + int_mask = sirfsoc_rtc_iobrg_readl(pwrcdrv->pwrc_base + PWRC_INT_MASK);
>> + if (enable)
>> + int_mask |= PWRC_ON_KEY_BIT;
>> + else
>> + int_mask &= ~PWRC_ON_KEY_BIT;
>> + sirfsoc_rtc_iobrg_writel(int_mask, pwrcdrv->pwrc_base + PWRC_INT_MASK);
>> +}
>> +
>> +static int sirfsoc_pwrc_open(struct input_dev *input)
>> +{
>> + struct sirfsoc_pwrc_drvdata *pwrcdrv = input_get_drvdata(input);
>> +
>> + sirfsoc_pwrc_toggle_interrupts(pwrcdrv, true);
>> +
>> + return 0;
>> +}
>> +
>> +static void sirfsoc_pwrc_close(struct input_dev *input)
>> +{
>> + struct sirfsoc_pwrc_drvdata *pwrcdrv = input_get_drvdata(input);
>> +
>> + sirfsoc_pwrc_toggle_interrupts(pwrcdrv, false);
>> +}
>> +
>> static const struct of_device_id sirfsoc_pwrc_of_match[] = {
>> { .compatible = "sirf,prima2-pwrc" },
>> {},
>> @@ -70,7 +99,7 @@ static int sirfsoc_pwrc_probe(struct platform_device *pdev)
>> }
>>
>> /*
>> - * we can't use of_iomap because pwrc is not mapped in memory,
>> + * We can't use of_iomap because pwrc is not mapped in memory,
>> * the so-called base address is only offset in rtciobrg
>> */
>> error = of_property_read_u32(np, "reg", &pwrcdrv->pwrc_base);
>> @@ -88,6 +117,11 @@ static int sirfsoc_pwrc_probe(struct platform_device *pdev)
>> pwrcdrv->input->phys = "pwrc/input0";
>> pwrcdrv->input->evbit[0] = BIT_MASK(EV_PWR);
>>
>> + pwrcdrv->input->open = sirfsoc_pwrc_open;
>> + pwrcdrv->input->close = sirfsoc_pwrc_close;
>> +
>> + input_set_drvdata(pwrcdrv->input, pwrcdrv);
>> +
>> irq = platform_get_irq(pdev, 0);
>> error = devm_request_irq(&pdev->dev, irq,
>> sirfsoc_pwrc_isr, IRQF_SHARED,
>> @@ -98,11 +132,6 @@ static int sirfsoc_pwrc_probe(struct platform_device *pdev)
>> return error;
>> }
>>
>> - sirfsoc_rtc_iobrg_writel(
>> - sirfsoc_rtc_iobrg_readl(pwrcdrv->pwrc_base + PWRC_INT_MASK) |
>> - PWRC_ON_KEY_BIT,
>> - pwrcdrv->pwrc_base + PWRC_INT_MASK);
>> -
>> error = input_register_device(pwrcdrv->input);
>> if (error) {
>> dev_err(&pdev->dev,
>> @@ -129,15 +158,16 @@ static int pwrc_resume(struct device *dev)
>> {
>> struct platform_device *pdev = to_platform_device(dev);
>> struct sirfsoc_pwrc_drvdata *pwrcdrv = platform_get_drvdata(pdev);
>> + struct input_dev *input = pwrcdrv->input;
>>
>> /*
>> * Do not mask pwrc interrupt as we want pwrc work as a wakeup source
>> * if users touch X_ONKEY_B, see arch/arm/mach-prima2/pm.c
>> */
>> - sirfsoc_rtc_iobrg_writel(
>> - sirfsoc_rtc_iobrg_readl(
>> - pwrcdrv->pwrc_base + PWRC_INT_MASK) | PWRC_ON_KEY_BIT,
>> - pwrcdrv->pwrc_base + PWRC_INT_MASK);
>> + mutex_lock(&input->mutex);
>> + if (input->users)
>> + sirfsoc_pwrc_toggle_interrupts(pwrcdrv, true);
>> + mutex_unlock(&input->mutex);
>>
>> return 0;
>> }
>
> -barry
-barry
^ permalink raw reply
* [PATCH] Input: Support in the elantech driver of the trackpoint present on for instance Lenovo L530
From: Ulrik De Bie @ 2014-02-14 20:51 UTC (permalink / raw)
To: linux-input
Cc: linux-kernel, Dmitry Torokhov, David Herrmann, Jonathan Aquilina,
Ulrik De Bie
Sorry it took me some time to have an email addres that would allow clean
mails. Below you can find the updated patch previously sent on linux-input
mailinglist. David Herrman asked to resent this patch as a proper git patch so
here it is now.
It also available on https://github.com/ulrikdb/linux.git elantech_trackpoint
from commit 38dbfb59d1175ef458d006556061adeaa8751b72 (Linus 3.14-rc1) up to
d69e103a35c944721966105790d14adf79098a4c
Please when responding please send either to linux-input or put Ulrik De Bie <ulrik.debie-os@e2big.org> in CC:, Thanks.
The Lenovo L530 trackpoint does not work out of the box. It gives sync errors
as shown below when the trackpoint or trackpoint mouse buttons are pressed and
no input is received by userspace:
[ 29.010641] psmouse serio1: Touchpad at isa0060/serio1/input0 lost sync at byte 6
The touchpad does work.
The alternative is to do a downgrade to generic ps/2 mouse (modprobe psmouse proto=bare)
but this has the disadvantage that touchpad can't be disabled (I want trackpoint, not touchpad).
With this patch, the trackpoint is provided as another input device; currently called 'TPPS/2 IBM TrackPoint'
The trackpoint now succesfully works and I can disable the touchpad with synclient TouchPadOff=1
The patch will also output messages that do not follow the expected pattern.
In the mean time I've seen 2 unknown packets occasionally:
0x04 , 0x00 , 0x00 , 0x00 , 0x00 , 0x00
0x00 , 0x00 , 0x00 , 0x02 , 0x00 , 0x00
I don't know what those are for, but they can be safely ignored.
Currently all packets that are not known to v3 touchpad and where packet[3] (the fourth byte) lowest
nibble is 6 are now recognized as PACKET_TRACKPOINT and processed by the new elantech_report_trackpoint.
This has been verified to work on a laptop where the touchpad/trackpoint combined identify themselves as:
psmouse serio1: elantech: assuming hardware version 3 (with firmware version 0x350f02)
psmouse serio1: elantech: Synaptics capabilities query result 0xb9, 0x15, 0x0c.
Since I can't send clean email from yahoo, I've switched to a different email address: ulrik.debie-os@e2big.org
Signed-off-by: Ulrik De Bie <ulrik_opensource-kernel@yahoo.com>
Signed-off-by: Ulrik De Bie <ulrik.debie-os@e2big.org>
Signed-off-by: Ulrik De Bie <ulrik.debie-os@e2big.org>
---
drivers/input/mouse/elantech.c | 78 +++++++++++++++++++++++++++++++++++++++++-
drivers/input/mouse/elantech.h | 3 ++
2 files changed, 80 insertions(+), 1 deletion(-)
diff --git a/drivers/input/mouse/elantech.c b/drivers/input/mouse/elantech.c
index ef1cf52..21d693b 100644
--- a/drivers/input/mouse/elantech.c
+++ b/drivers/input/mouse/elantech.c
@@ -402,6 +402,54 @@ static void elantech_report_absolute_v2(struct psmouse *psmouse)
input_sync(dev);
}
+static void elantech_report_trackpoint(struct psmouse *psmouse,
+ int packet_type)
+{
+ /* byte 0: 0 0 ~sx ~sy 0 M R L */
+ /* byte 1: sx 0 0 0 0 0 0 0 */
+ /* byte 2: sy 0 0 0 0 0 0 0 */
+ /* byte 3: 0 0 sy sx 0 1 1 0 */
+ /* byte 4: x7 x6 x5 x4 x3 x2 x1 x0 */
+ /* byte 5: y7 y6 y5 y4 y3 y2 y1 y0 */
+
+ /*
+ * x and y are written in two's complement spread
+ * over 9 bits with sx/sy the relative top bit and
+ * x7..x0 and y7..y0 the lower bits.
+ * The sign of y is opposite to what the input driver
+ * expects for a relative movement
+ */
+
+ struct elantech_data *etd = psmouse->private;
+ struct input_dev *dev2 = etd->dev2;
+ unsigned char *packet = psmouse->packet;
+ int x, y;
+ input_report_key(dev2, BTN_LEFT, packet[0] & 0x01);
+ input_report_key(dev2, BTN_RIGHT, packet[0] & 0x02);
+ input_report_key(dev2, BTN_MIDDLE, packet[0] & 0x04);
+ x = (s32) ((u32) ((packet[1] & 0x80) ? 0UL : 0xFFFFFF00UL) | (u32)
+ packet[4]);
+ y = -(s32) ((u32) ((packet[2] & 0x80) ? 0UL : 0xFFFFFF00UL) | (u32)
+ packet[5]);
+ input_report_rel(dev2, REL_X, x);
+ input_report_rel(dev2, REL_Y, y);
+ switch ((((u32) packet[0] & 0xF8) << 24) | ((u32) packet[1] << 16)
+ | (u32) packet[2] << 8 | (u32) packet[3]) {
+ case 0x00808036UL:
+ case 0x10008026UL:
+ case 0x20800016UL:
+ case 0x30000006UL:
+ break;
+ default:
+ /* Dump unexpected packet sequences if debug=1 (default) */
+ if (etd->debug == 1)
+ elantech_packet_dump(psmouse);
+ break;
+ }
+
+ input_sync(dev2);
+}
+
/*
* Interpret complete data packets and report absolute mode input events for
* hardware version 3. (12 byte packets for two fingers)
@@ -700,8 +748,11 @@ static int elantech_packet_check_v3(struct psmouse *psmouse)
if ((packet[0] & 0x0c) == 0x0c && (packet[3] & 0xce) == 0x0c)
return PACKET_V3_TAIL;
+ if ((packet[3]&0x0f) == 0x06)
+ return PACKET_TRACKPOINT;
}
+
return PACKET_UNKNOWN;
}
@@ -783,7 +834,10 @@ static psmouse_ret_t elantech_process_byte(struct psmouse *psmouse)
if (packet_type == PACKET_UNKNOWN)
return PSMOUSE_BAD_DATA;
- elantech_report_absolute_v3(psmouse, packet_type);
+ if (packet_type == PACKET_TRACKPOINT)
+ elantech_report_trackpoint(psmouse, packet_type);
+ else
+ elantech_report_absolute_v3(psmouse, packet_type);
break;
case 4:
@@ -1400,10 +1454,15 @@ int elantech_init(struct psmouse *psmouse)
struct elantech_data *etd;
int i, error;
unsigned char param[3];
+ struct input_dev *dev2;
psmouse->private = etd = kzalloc(sizeof(struct elantech_data), GFP_KERNEL);
if (!etd)
return -ENOMEM;
+ dev2 = input_allocate_device();
+ if (!dev2)
+ goto init_fail;
+ etd->dev2 = dev2;
psmouse_reset(psmouse);
@@ -1463,9 +1522,26 @@ int elantech_init(struct psmouse *psmouse)
psmouse->reconnect = elantech_reconnect;
psmouse->pktsize = etd->hw_version > 1 ? 6 : 4;
+ snprintf(etd->phys, sizeof(etd->phys), "%s/input1",
+ psmouse->ps2dev.serio->phys);
+ dev2->phys = etd->phys;
+ dev2->name = "TPPS/2 IBM TrackPoint";
+ dev2->id.bustype = BUS_I8042;
+ dev2->id.vendor = 0x0002;
+ dev2->id.product = PSMOUSE_ELANTECH;
+ dev2->id.version = 0x0000;
+ dev2->dev.parent = &psmouse->ps2dev.serio->dev;
+ dev2->evbit[0] = BIT_MASK(EV_KEY) | BIT_MASK(EV_REL);
+ dev2->relbit[BIT_WORD(REL_X)] = BIT_MASK(REL_X) | BIT_MASK(REL_Y);
+ dev2->keybit[BIT_WORD(BTN_LEFT)] =
+ BIT_MASK(BTN_LEFT) | BIT_MASK(BTN_MIDDLE) | BIT_MASK(BTN_RIGHT);
+
+ if (input_register_device(etd->dev2))
+ goto init_fail;
return 0;
init_fail:
+ input_free_device(dev2);
kfree(etd);
return -1;
}
diff --git a/drivers/input/mouse/elantech.h b/drivers/input/mouse/elantech.h
index 036a04a..27cf191 100644
--- a/drivers/input/mouse/elantech.h
+++ b/drivers/input/mouse/elantech.h
@@ -94,6 +94,7 @@
#define PACKET_V4_HEAD 0x05
#define PACKET_V4_MOTION 0x06
#define PACKET_V4_STATUS 0x07
+#define PACKET_TRACKPOINT 0x08
/*
* track up to 5 fingers for v4 hardware
@@ -114,6 +115,8 @@ struct finger_pos {
};
struct elantech_data {
+ struct input_dev *dev2; /* Relative device */
+ char phys[32];
unsigned char reg_07;
unsigned char reg_10;
unsigned char reg_11;
--
1.8.5.3
^ permalink raw reply related
* Re: [PATCH 03/11] Input: synaptics-rmi4 - do not update configuration in rmi_f01_probe()
From: Christopher Heiny @ 2014-02-14 23:00 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Andrew Duggan, Vincent Huang, Vivian Ly, Daniel Rosenberg,
Linus Walleij, Benjamin Tissoires, Courtney Cavin, Linux Input,
Linux Kernel
In-Reply-To: <20140213215432.GA6225@core.coreip.homeip.net>
On 02/13/2014 01:54 PM, Dmitry Torokhov wrote:
> On Thu, Feb 13, 2014 at 11:23:44AM -0800, Christopher Heiny wrote:
>> >On 02/12/2014 09:27 PM, Dmitry Torokhov wrote:
>>> > >Do not write configuration data in probe(), we have config() for that.
>> >
>> >Then we should call config() in rmi_function_probe() to ensure that
>> >any platform data or device tree configuration settings get written
>> >to the device.
>
> Well, yes, we may elect to update device configuration in probe, but
> then we should not be doing that 2nd time in ->config(). We shoudl pick
> either one or another.
But as the code currently stands, config() is only called when a device
reset is detected, not during initialization. So if there's platform
specific configuration data that needs to be written to a function, it
won't get written until after a device reset occurs, which might never
happen. So either we need to write that data (or call config()) in each
function's probe(), or in rmi_function_probe().
^ permalink raw reply
* Re: Questions about the documentation/specification of Linux ForceFeedback input.h
From: Anssi Hannula @ 2014-02-15 2:05 UTC (permalink / raw)
To: Elias Vanderstuyft
Cc: Dmitry Torokhov, dtor, johann.deneux, linux-input,
Michal Malý
In-Reply-To: <CADbOyBSeNKe6hZWdLMnE+SPyu__NwD4WKHv_cAmOz-NTKuzxSA@mail.gmail.com>
15.02.2014 01:28, Elias Vanderstuyft kirjoitti:
> Hi everyone,
Hi!
>
> If you receive my mail, it is either because you:
> - are listed under the MAINTAINERS for "/include/uapi/linux/input.h":
> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/MAINTAINERS#n4410
> - are listed as an author of "/Documentation/input/ff.txt" or of
> "/Documentation/input/interactive.fig"
>
> In the process of testing the new driver (ff-memless-next) Michal
> (CC-ed) is working on, this allowed me to gain more insight in the
> internal workings of Linux FF. That's why I decided to review the
> userspace code that currently uses most of Linux FF functionality:
> Wine's DInput translation layer (and eventually SDL2.)
> Trying to get everything correct, I noticed both
> "/include/uapi/linux/input.h" and "/Documentation/input/ff.txt"
> provide not enough information for the following things:
>
> 1)
> The real meaning of 'directions', declared in
> http://lxr.free-electrons.com/source/include/uapi/linux/input.h#L1113
> :
> "Direction of the effect" is encoded as follows: ...
> But it is not clear whether 'direction of the effect' means either:
> - positive direction of the force the user should apply to counteract
> the force that the joystick applies; or
> - positive direction of the force applied by joystick
> From my intuition, I think the latter is (silently?) meant by input.h
> If you're interested why this is so important, I attached a document
> "DInputVsLinuxDirections.txt" that tries to explain the dilemma if a
> translation layer between DInput and Linux input is to be written.
>From input.h:
* Direction of the effect is encoded as follows:
* 0 deg -> 0x0000 (down)
* 90 deg -> 0x4000 (left)
* 180 deg -> 0x8000 (up)
* 270 deg -> 0xC000 (right)
The directions in parantheses are the direction of applied force.
However, there is actually a 1:1 mapping between DInput polar
coordinates and our direction parameter; DInput polar coordinates have 0
deg = up, 90 deg = right, etc, so they are exactly flipped and therefore
match our values due to the reverse definition.
Looking at your DInputVsLinuxDirections.txt, you seem to have mixed
different definitions of Carts: For DInput you use -1 = north, but for
Linux +1 = up, while you use -1 = west/left for both. This causes the
1st and 3rd entries on both of the Mapping tables to be reversed. When
that is fixed, the table #2 shows the correct result.
> 2)
> It is not clear how to create a FF effect with infinite duration
> (replay.length):
> http://lxr.free-electrons.com/source/include/uapi/linux/input.h#L966
> Only the following is written about duration values in general:
> "All duration values are expressed in ms. Values above 32767 ms
> (0x7fff) should not be used and have unspecified results."
> Michal solved this by associating replay.length==0 for infinite, which
> makes sense since otherwise the effect would not have been started
> anyway.
Infinite duration can be achieved with length = 0.
> 3)
> Many Linux FF effect parameters don't have a clear explanation of the
> allowed range, and their corresponding meaning at the extrema.
> Here I list the ones that need more explanation, also take a look at
> "interactive.fig" in the kernel documentation (but also included as
> attachment):
> - left_saturation and right_saturation; and
left_saturation = maximum force on the negative ("left") side of the
center point
right_saturation = same for positive side
0x0 => no force,
0xFFFF => maximum force.
> - deadband
The range from center point wherein the effect has no effect, with
0x0 => no dead band
0xFFFF => dead band encompassing the entire axis, effect not active
anywhere.
Assuming center offset of 0, though. Not sure how the currently
supported devices interpret 0xFFFF with non-zero center offset, i.e. if
the effect is then still active in the extreme opposite end of the axis.
You wrote below that this is indeed the case with DInput, so it is
highly likely this is how the devices handle it as well.
> They all have __u16 types, but "/include/uapi/linux/input.h" does not
> say what the maximum values are.
> I'm doing a proposal to define and document this (in the Linux kernel)
> in the following way, also take a look at my attachment
> "interactiveAlteredWithRanges.svg" for the modified version:
> Max Range of {right_saturation and left_saturation} = 0x7FFF
> Because the maximal value of the saturation bound can be only
> half of the total range covered by the max negative to max positive
> bounds.
> And also because they are related to some form of force, and
> all other forms of force magnitude in Linux FF have a maximum value of
> 0x7FFF
I'm not really convinced that the different range from the other
magnitude values is a reason enough to change the definition here.
> Max Range of {deadband} = 0xFFFF
> This is a bit harder to explain:
> - First, I would suggest to alter the deadband definition in
> figure "interactive.fig":
> I would define deadband as going from a deadband-bound to
> the center (BTW, This is how MSDN defines it: "In other words, the
> condition is not active between lOffset minus lDeadBand and lOffset
> plus lDeadBand."),
> instead of from a deadband-bound to the other deadband-bound.
> => Same spec as in DInput.
With 0xFFFF being the maximum deadband with center offset 0, it does not
matter if deadband is defined as range from center or total width,
maximum is 0xFFFF in both cases.
> - Now, knowing that ff_condition_effect->center is __s16:
> The worst case scenario is that "center = -32768" or
> "center = +32767" while still wanting to cover the whole region (this
> is actually not possible with DInput's specs: in that case, they can
> only cover half of the total region):
> Then, to keep the scale of "center" and "deadband" the
> same, "deadband = 65535" = +32767 - -32768 = 0xFFFF
Interesting idea. However, if this is not possible in DInput, this means
the devices will likely not support it either, since they are using the
DInput effect model (as are we).
I tried to confirm this with my SWFF2 device, but either it has stopped
working properly or, more likely, there is a regression in the kernel...
no time to debug now, though, so added to my TODO (HID_REQ_GET_REPORT
requests don't seem to go through properly).
> I expect we will have to add/document the answers to my questions in
> the appropriate file "/include/uapi/linux/input.h" or
> "/Documentation/input/ff.txt", so that other userspace developers (and
> maybe also kernel devs) don't face the same ambiguities.
>
>
> Thank you very much for your time,
Thanks for looking into this.
> Elias
>
--
Anssi Hannula
^ permalink raw reply
* [PATCH 2/2] Input: imx_keypad - Omit error message devm_kzalloc() failure
From: Fabio Estevam @ 2014-02-15 2:59 UTC (permalink / raw)
To: dmitry.torokhov; +Cc: linux-input, Fabio Estevam
In-Reply-To: <1392433187-7756-1-git-send-email-festevam@gmail.com>
From: Fabio Estevam <fabio.estevam@freescale.com>
There is no need to print an error message in the driver for devm_kzalloc()
failure because OOM core code handles it properly.
Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
---
drivers/input/keyboard/imx_keypad.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/input/keyboard/imx_keypad.c b/drivers/input/keyboard/imx_keypad.c
index de07035..40ad98d 100644
--- a/drivers/input/keyboard/imx_keypad.c
+++ b/drivers/input/keyboard/imx_keypad.c
@@ -450,10 +450,8 @@ static int imx_keypad_probe(struct platform_device *pdev)
keypad = devm_kzalloc(&pdev->dev, sizeof(struct imx_keypad),
GFP_KERNEL);
- if (!keypad) {
- dev_err(&pdev->dev, "not enough memory for driver data\n");
+ if (!keypad)
return -ENOMEM;
- }
keypad->input_dev = input_dev;
keypad->irq = irq;
--
1.8.1.2
^ permalink raw reply related
* [PATCH 1/2] Input: imx_keypad - Propagate the real error code on platform_get_irq() failure
From: Fabio Estevam @ 2014-02-15 2:59 UTC (permalink / raw)
To: dmitry.torokhov; +Cc: linux-input, Fabio Estevam
From: Fabio Estevam <fabio.estevam@freescale.com>
No need to return a 'fake' return value on platform_get_irq() failure.
Just return the error code itself instead.
Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
---
drivers/input/keyboard/imx_keypad.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/input/keyboard/imx_keypad.c b/drivers/input/keyboard/imx_keypad.c
index cbf4f80..de07035 100644
--- a/drivers/input/keyboard/imx_keypad.c
+++ b/drivers/input/keyboard/imx_keypad.c
@@ -439,7 +439,7 @@ static int imx_keypad_probe(struct platform_device *pdev)
irq = platform_get_irq(pdev, 0);
if (irq < 0) {
dev_err(&pdev->dev, "no irq defined in platform data\n");
- return -EINVAL;
+ return irq;
}
input_dev = devm_input_allocate_device(&pdev->dev);
--
1.8.1.2
^ permalink raw reply related
* Re: [PATCH] HID: hid-sensor-hub: quirk for STM Sensor hub
From: Jonathan Cameron @ 2014-02-15 11:00 UTC (permalink / raw)
To: Archana Patni, jkosina; +Cc: linux-input, srinivas.pandruvada
In-Reply-To: <1391411656-32684-1-git-send-email-archana.patni@linux.intel.com>
On 03/02/14 07:14, Archana Patni wrote:
> Added STM sensor hub vendor id in HID_SENSOR_HUB_ENUM_QUIRK to
> fix report descriptors. These devices uses old FW which uses
> logical 0 as minimum. In these, HID reports are not using proper
> collection classes. So we need to fix report descriptors,for
> such devices. This will not have any impact, if the FW uses
> logical 1 as minimum.
>
> We look for usage id for "power and report state", and modify
> logical minimum value to 1.
>
> This is a follow-up patch to commit id 875e36f8.
>
> Signed-off-by: Archana Patni <archana.patni@linux.intel.com>
> Reviewed-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
Hi Jiri,
I'm fine with any of these going in through the hid tree without without
my Ack given they are just marking which drivers need the quirk and
which do not.
Jonathan
> ---
> drivers/hid/hid-ids.h | 3 +++
> drivers/hid/hid-sensor-hub.c | 3 +++
> 2 files changed, 6 insertions(+)
>
> diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
> index 5a5248f..1fc5bee 100644
> --- a/drivers/hid/hid-ids.h
> +++ b/drivers/hid/hid-ids.h
> @@ -451,6 +451,9 @@
> #define USB_VENDOR_ID_INTEL_1 0x8087
> #define USB_DEVICE_ID_INTEL_HID_SENSOR 0x09fa
>
> +#define USB_VENDOR_ID_STM_0 0x0483
> +#define USB_DEVICE_ID_STM_HID_SENSOR 0x91d1
> +
> #define USB_VENDOR_ID_ION 0x15e4
> #define USB_DEVICE_ID_ICADE 0x0132
>
> diff --git a/drivers/hid/hid-sensor-hub.c b/drivers/hid/hid-sensor-hub.c
> index 46f4480..9c22e14 100644
> --- a/drivers/hid/hid-sensor-hub.c
> +++ b/drivers/hid/hid-sensor-hub.c
> @@ -665,6 +665,9 @@ static const struct hid_device_id sensor_hub_devices[] = {
> { HID_DEVICE(HID_BUS_ANY, HID_GROUP_SENSOR_HUB, USB_VENDOR_ID_INTEL_1,
> USB_DEVICE_ID_INTEL_HID_SENSOR),
> .driver_data = HID_SENSOR_HUB_ENUM_QUIRK},
> + { HID_DEVICE(HID_BUS_ANY, HID_GROUP_SENSOR_HUB, USB_VENDOR_ID_STM_0,
> + USB_DEVICE_ID_STM_HID_SENSOR),
> + .driver_data = HID_SENSOR_HUB_ENUM_QUIRK},
> { HID_DEVICE(HID_BUS_ANY, HID_GROUP_SENSOR_HUB, HID_ANY_ID,
> HID_ANY_ID) },
> { }
>
^ permalink raw reply
* Re: Questions about the documentation/specification of Linux ForceFeedback input.h
From: Elias Vanderstuyft @ 2014-02-15 12:14 UTC (permalink / raw)
To: Anssi Hannula, Michal Malý
Cc: Dmitry Torokhov, dtor, Johann Deneux, linux-input
In-Reply-To: <52FECB6E.4000004@iki.fi>
On Sat, Feb 15, 2014 at 3:05 AM, Anssi Hannula <anssi.hannula@iki.fi> wrote:
> 15.02.2014 01:28, Elias Vanderstuyft kirjoitti:
>> Hi everyone,
>
> Hi!
>
>>
>> If you receive my mail, it is either because you:
>> - are listed under the MAINTAINERS for "/include/uapi/linux/input.h":
>> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/MAINTAINERS#n4410
>> - are listed as an author of "/Documentation/input/ff.txt" or of
>> "/Documentation/input/interactive.fig"
>>
>> In the process of testing the new driver (ff-memless-next) Michal
>> (CC-ed) is working on, this allowed me to gain more insight in the
>> internal workings of Linux FF. That's why I decided to review the
>> userspace code that currently uses most of Linux FF functionality:
>> Wine's DInput translation layer (and eventually SDL2.)
>> Trying to get everything correct, I noticed both
>> "/include/uapi/linux/input.h" and "/Documentation/input/ff.txt"
>> provide not enough information for the following things:
>>
>> 1)
>> The real meaning of 'directions', declared in
>> http://lxr.free-electrons.com/source/include/uapi/linux/input.h#L1113
>> :
>> "Direction of the effect" is encoded as follows: ...
>> But it is not clear whether 'direction of the effect' means either:
>> - positive direction of the force the user should apply to counteract
>> the force that the joystick applies; or
>> - positive direction of the force applied by joystick
>> From my intuition, I think the latter is (silently?) meant by input.h
>> If you're interested why this is so important, I attached a document
>> "DInputVsLinuxDirections.txt" that tries to explain the dilemma if a
>> translation layer between DInput and Linux input is to be written.
>
> From input.h:
> * Direction of the effect is encoded as follows:
> * 0 deg -> 0x0000 (down)
> * 90 deg -> 0x4000 (left)
> * 180 deg -> 0x8000 (up)
> * 270 deg -> 0xC000 (right)
>
> The directions in parantheses are the direction of applied force.
Alright, thanks! That is invaluable information, maybe it should be
added to input.h .
It will avoid a lot of confusion for former DInput devs.
>
> However, there is actually a 1:1 mapping between DInput polar
> coordinates and our direction parameter; DInput polar coordinates have 0
> deg = up, 90 deg = right, etc, so they are exactly flipped and therefore
> match our values due to the reverse definition.
>
> Looking at your DInputVsLinuxDirections.txt, you seem to have mixed
> different definitions of Carts: For DInput you use -1 = north, but for
> Linux +1 = up, while you use -1 = west/left for both.
So I assume you agree that I got the DInput part right? ((0, -1) = north)
Then my mistake lies in the assumption that (0, +1) = up, so I should
flip the y-axis to correct the Linux part.
I'll explain how I originally derived the Linux part:
Michal documented (in "ff-memless-next.txt") the Linux directions in
the following way:
"
Direction of the effect's force translates to Cartesian coordinates system
as follows:
Direction = 0: Down
Direction (0; 16384): 3rd quadrant
Direction = 16384: Left
Direction (16385; 32768): 2nd quadrant
Direction = 32768: Up
Direction (32769; 49152): 1st quadrant
Direction = 49152: Right
Direction (49153; 65535) :4th quadrant
Direction = 65565: Down
"
For a Cartesian coordinates system:
- The (-1, 0)-axis (=-x) is the intersection of 3rd quadrant and 2nd
quadrant => Left
- The (0, +1)-axis (=+y) is the intersection of 2nd quadrant and 1st
quadrant => Up
- The (+1, 0)-axis (=+x) is the intersection of 1st quadrant and 4th
quadrant => Right
- The (0, -1)-axis (=-y) is the intersection of 4th quadrant and 3rd
quadrant => Down
Michal's approach seems logical to me, if he made a mistake, it's
caused by the lack of documentation of input.h : it should mention
what axes (-x, +x, -y or +y) the words (left, right, down and up)
correspond with.
So, which interpretation is the right one?
(I did not find anything in the Linux documentation that states "there
is actually a 1:1 mapping between DInput polar coordinates and our
direction parameter")
> This causes the
> 1st and 3rd entries on both of the Mapping tables to be reversed. When
> that is fixed, the table #2 shows the correct result.
>
>> 2)
>> It is not clear how to create a FF effect with infinite duration
>> (replay.length):
>> http://lxr.free-electrons.com/source/include/uapi/linux/input.h#L966
>> Only the following is written about duration values in general:
>> "All duration values are expressed in ms. Values above 32767 ms
>> (0x7fff) should not be used and have unspecified results."
>> Michal solved this by associating replay.length==0 for infinite, which
>> makes sense since otherwise the effect would not have been started
>> anyway.
>
> Infinite duration can be achieved with length = 0.
Thanks! This has to be documented.
>
>> 3)
>> Many Linux FF effect parameters don't have a clear explanation of the
>> allowed range, and their corresponding meaning at the extrema.
>> Here I list the ones that need more explanation, also take a look at
>> "interactive.fig" in the kernel documentation (but also included as
>> attachment):
>> - left_saturation and right_saturation; and
>
> left_saturation = maximum force on the negative ("left") side of the
> center point
> right_saturation = same for positive side
>
> 0x0 => no force,
> 0xFFFF => maximum force.
OK, thanks for giving the definition. I think these things can be
understood from "interactive.fig", so there's no need to write
additional doc about this topic.
>
>> - deadband
>
> The range from center point wherein the effect has no effect
Notice this contradicts with "interactive.fig", this figure defines
'deadband' as bound-to-bound, not as center-to-bound (as with DInput).
> , with
> 0x0 => no dead band
> 0xFFFF => dead band encompassing the entire axis, effect not active
> anywhere.
>
> Assuming center offset of 0, though. Not sure how the currently
> supported devices interpret 0xFFFF with non-zero center offset, i.e. if
> the effect is then still active in the extreme opposite end of the axis.
> You wrote below that this is indeed the case with DInput, so it is
> highly likely this is how the devices handle it as well.
Careful, here there's a catch:
I did not write that in DInput "if deadband == maxValue(deadband)
with non-zero center offset, then the effect is still active in the
extreme opposite end of the axis.",
I wrote that in DInput "if deadband == maxValue(deadband) with
maximum non-zero center offset, then the effect can only cover half of
the total region". For reference: maxValue(deadband)=10000
Now I'm proposing to let DInput maxValue(deadband) correspond to linux
deadband 0x7FFF.
And still allowing linux deadband to be maximal 0xFFFF => two times
the maximum range of DInput
>
>
>> They all have __u16 types, but "/include/uapi/linux/input.h" does not
>> say what the maximum values are.
>> I'm doing a proposal to define and document this (in the Linux kernel)
>> in the following way, also take a look at my attachment
>> "interactiveAlteredWithRanges.svg" for the modified version:
>> Max Range of {right_saturation and left_saturation} = 0x7FFF
>> Because the maximal value of the saturation bound can be only
>> half of the total range covered by the max negative to max positive
>> bounds.
>> And also because they are related to some form of force, and
>> all other forms of force magnitude in Linux FF have a maximum value of
>> 0x7FFF
>
> I'm not really convinced that the different range from the other
> magnitude values is a reason enough to change the definition here.
After reviewing this mail, I totally agree with you, sorry for that.
>
>> Max Range of {deadband} = 0xFFFF
>> This is a bit harder to explain:
>> - First, I would suggest to alter the deadband definition in
>> figure "interactive.fig":
>> I would define deadband as going from a deadband-bound to
>> the center (BTW, This is how MSDN defines it: "In other words, the
>> condition is not active between lOffset minus lDeadBand and lOffset
>> plus lDeadBand."),
>> instead of from a deadband-bound to the other deadband-bound.
>> => Same spec as in DInput.
>
> With 0xFFFF being the maximum deadband with center offset 0, it does not
> matter if deadband is defined as range from center or total width,
> maximum is 0xFFFF in both cases.
Indeed, because (assume applying 0xFFFF linux deadband with center offset 0):
a) If DInput maxValue(deadband)=10000 (=half of the total region) maps
to 0x7FFF in the Linux case (=my proposal) if linux deadband is
defined as range from center:
the driver will clamp {0 + 0xFFFF =approx 0 + 2 * 0x7FFF} to
0x7FFF for both left as right side of center => Total region is
covered by the deadband.
b) If DInput maxValue(deadband)=10000 (=half of the total region) maps
to 0x7FFF in the Linux case (=my proposal) if linux deadband is
defined as total width:
the driver will set {0 + 0xFFFF / 2 =approx 0 + 0x7FFF} for both
left as right side of center => Total region is covered by the
deadband.
If we define linux deadband as range from center, like a), the Linux
FF API can represent more variations of conditional effects than
DInput can (and also with greater resolution), and thus becomes
superior in that aspect.
We will have to do the clamping anyway for devices that only accept
left and right deadband bounds, i.e. Logitech wheels.
For devices that only accept a single deadband value, like your SWFF2
as you mentioned below, the linux deadband would only need to be
shifted "<< 1" before being send to the device, if they accept u16 and
work exactly like DInput (range from center, and
maxValue(deadband)=half of total region).
>
>> - Now, knowing that ff_condition_effect->center is __s16:
>> The worst case scenario is that "center = -32768" or
>> "center = +32767" while still wanting to cover the whole region (this
>> is actually not possible with DInput's specs: in that case, they can
>> only cover half of the total region):
>> Then, to keep the scale of "center" and "deadband" the
>> same, "deadband = 65535" = +32767 - -32768 = 0xFFFF
>
> Interesting idea. However, if this is not possible in DInput, this means
> the devices will likely not support it either, since they are using the
> DInput effect model (as are we).
This is no problem, use "<< 1" as mentioned above.
>
>
> I tried to confirm this with my SWFF2 device, but either it has stopped
> working properly or, more likely, there is a regression in the kernel...
> no time to debug now, though, so added to my TODO (HID_REQ_GET_REPORT
> requests don't seem to go through properly).
>
>
>> I expect we will have to add/document the answers to my questions in
>> the appropriate file "/include/uapi/linux/input.h" or
>> "/Documentation/input/ff.txt", so that other userspace developers (and
>> maybe also kernel devs) don't face the same ambiguities.
>>
>>
>> Thank you very much for your time,
>
> Thanks for looking into this.
>
>> Elias
>>
>
>
> --
> Anssi Hannula
Elias
^ permalink raw reply
* Re: Questions about the documentation/specification of Linux ForceFeedback input.h
From: Anssi Hannula @ 2014-02-15 14:04 UTC (permalink / raw)
To: Elias Vanderstuyft
Cc: Michal Malý, Dmitry Torokhov, dtor, Johann Deneux,
linux-input
In-Reply-To: <CADbOyBQ1iA_7c4Rik7nBPvqAwVouoQDL33L3PE2RiVLnmFz1YQ@mail.gmail.com>
15.02.2014 14:14, Elias Vanderstuyft kirjoitti:
> On Sat, Feb 15, 2014 at 3:05 AM, Anssi Hannula <anssi.hannula@iki.fi> wrote:
>> 15.02.2014 01:28, Elias Vanderstuyft kirjoitti:
>>> Hi everyone,
>>
>> Hi!
>>
>>>
>>> If you receive my mail, it is either because you:
>>> - are listed under the MAINTAINERS for "/include/uapi/linux/input.h":
>>> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/MAINTAINERS#n4410
>>> - are listed as an author of "/Documentation/input/ff.txt" or of
>>> "/Documentation/input/interactive.fig"
>>>
>>> In the process of testing the new driver (ff-memless-next) Michal
>>> (CC-ed) is working on, this allowed me to gain more insight in the
>>> internal workings of Linux FF. That's why I decided to review the
>>> userspace code that currently uses most of Linux FF functionality:
>>> Wine's DInput translation layer (and eventually SDL2.)
>>> Trying to get everything correct, I noticed both
>>> "/include/uapi/linux/input.h" and "/Documentation/input/ff.txt"
>>> provide not enough information for the following things:
>>>
>>> 1)
>>> The real meaning of 'directions', declared in
>>> http://lxr.free-electrons.com/source/include/uapi/linux/input.h#L1113
>>> :
>>> "Direction of the effect" is encoded as follows: ...
>>> But it is not clear whether 'direction of the effect' means either:
>>> - positive direction of the force the user should apply to counteract
>>> the force that the joystick applies; or
>>> - positive direction of the force applied by joystick
>>> From my intuition, I think the latter is (silently?) meant by input.h
>>> If you're interested why this is so important, I attached a document
>>> "DInputVsLinuxDirections.txt" that tries to explain the dilemma if a
>>> translation layer between DInput and Linux input is to be written.
>>
>> From input.h:
>> * Direction of the effect is encoded as follows:
>> * 0 deg -> 0x0000 (down)
>> * 90 deg -> 0x4000 (left)
>> * 180 deg -> 0x8000 (up)
>> * 270 deg -> 0xC000 (right)
>>
>> The directions in parantheses are the direction of applied force.
>
> Alright, thanks! That is invaluable information, maybe it should be
> added to input.h .
> It will avoid a lot of confusion for former DInput devs.
>
>>
>> However, there is actually a 1:1 mapping between DInput polar
>> coordinates and our direction parameter; DInput polar coordinates have 0
>> deg = up, 90 deg = right, etc, so they are exactly flipped and therefore
>> match our values due to the reverse definition.
>>
>> Looking at your DInputVsLinuxDirections.txt, you seem to have mixed
>> different definitions of Carts: For DInput you use -1 = north, but for
>> Linux +1 = up, while you use -1 = west/left for both.
>
> So I assume you agree that I got the DInput part right? ((0, -1) = north)
I guess so.
> Then my mistake lies in the assumption that (0, +1) = up, so I should
> flip the y-axis to correct the Linux part.
> I'll explain how I originally derived the Linux part:
> Michal documented (in "ff-memless-next.txt") the Linux directions in
> the following way:
> "
> Direction of the effect's force translates to Cartesian coordinates system
> as follows:
> Direction = 0: Down
> Direction (0; 16384): 3rd quadrant
> Direction = 16384: Left
> Direction (16385; 32768): 2nd quadrant
> Direction = 32768: Up
> Direction (32769; 49152): 1st quadrant
> Direction = 49152: Right
> Direction (49153; 65535) :4th quadrant
> Direction = 65565: Down
> "
The above is correct.
> For a Cartesian coordinates system:
> - The (-1, 0)-axis (=-x) is the intersection of 3rd quadrant and 2nd
> quadrant => Left
> - The (0, +1)-axis (=+y) is the intersection of 2nd quadrant and 1st
> quadrant => Up
> - The (+1, 0)-axis (=+x) is the intersection of 1st quadrant and 4th
> quadrant => Right
> - The (0, -1)-axis (=-y) is the intersection of 4th quadrant and 3rd
> quadrant => Down
Not sure why you've arbitrarily chosen reverse definition of Y axis
here, when all DInput and Linux joysticks, mouses, etc. have -y as "up".
> Michal's approach seems logical to me, if he made a mistake, it's
> caused by the lack of documentation of input.h : it should mention
> what axes (-x, +x, -y or +y) the words (left, right, down and up)
> correspond with.
Well, the "left", "right", "down", "up" correspond to directions from
the user perspective. I don't think that is ambigiuous at all, as I
don't see how you could consider "down" to be away from user?
I'm not against more documentation if it helps, though.
> So, which interpretation is the right one?
The one where the directions provided in input.h match physical
direction of the force effect from user perspective on a 2-axis joystick
device.
> (I did not find anything in the Linux documentation that states "there
> is actually a 1:1 mapping between DInput polar coordinates and our
> direction parameter")
It is not stated explicitely, it just naturally follows.
1. Both use a clock-wise direction.
1. DInput direction definition is reversed, as they use the "counteract"
direction. We can flip to correct.
2. DInput direction has 0 = up/north, we have 0 = down/south,
i.e. the exact opposite, so we must flip the direction.
DInput direction = flipped(flipped(Linux direction))
= Linux direction
>> This causes the
>> 1st and 3rd entries on both of the Mapping tables to be reversed. When
>> that is fixed, the table #2 shows the correct result.
>>
>>> 2)
>>> It is not clear how to create a FF effect with infinite duration
>>> (replay.length):
>>> http://lxr.free-electrons.com/source/include/uapi/linux/input.h#L966
>>> Only the following is written about duration values in general:
>>> "All duration values are expressed in ms. Values above 32767 ms
>>> (0x7fff) should not be used and have unspecified results."
>>> Michal solved this by associating replay.length==0 for infinite, which
>>> makes sense since otherwise the effect would not have been started
>>> anyway.
>>
>> Infinite duration can be achieved with length = 0.
>
> Thanks! This has to be documented.
>
>>
>>> 3)
>>> Many Linux FF effect parameters don't have a clear explanation of the
>>> allowed range, and their corresponding meaning at the extrema.
>>> Here I list the ones that need more explanation, also take a look at
>>> "interactive.fig" in the kernel documentation (but also included as
>>> attachment):
>>> - left_saturation and right_saturation; and
>>
>> left_saturation = maximum force on the negative ("left") side of the
>> center point
>> right_saturation = same for positive side
>>
>> 0x0 => no force,
>> 0xFFFF => maximum force.
>
> OK, thanks for giving the definition. I think these things can be
> understood from "interactive.fig", so there's no need to write
> additional doc about this topic.
>
>>
>>> - deadband
>>
>> The range from center point wherein the effect has no effect
>
> Notice this contradicts with "interactive.fig", this figure defines
> 'deadband' as bound-to-bound, not as center-to-bound (as with DInput).
There is no difference between those definitions that I can see:
Axis range
|------------------------------------------------------|
Definition from interactive.fig, bound-to-bound:
0x0:
|--------------------------X---------------------------|
0x8000 (covers half of the end-to-end area):
|------------XXXXXXXXXXXXXXXXXXXXXXXXXXXXXX------------|
0xFFFF:
|XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX|
Center-to-bound definition:
0x0:
|--------------------------X---------------------------|
0x8000 (covers half of the center-to-end area):
|------------XXXXXXXXXXXXXXXXXXXXXXXXXXXXXX------------|
0xFFFF:
|XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX|
>> , with
>> 0x0 => no dead band
>> 0xFFFF => dead band encompassing the entire axis, effect not active
>> anywhere.
>>
>> Assuming center offset of 0, though. Not sure how the currently
>> supported devices interpret 0xFFFF with non-zero center offset, i.e. if
>> the effect is then still active in the extreme opposite end of the axis.
>> You wrote below that this is indeed the case with DInput, so it is
>> highly likely this is how the devices handle it as well.
>
> Careful, here there's a catch:
> I did not write that in DInput "if deadband == maxValue(deadband)
> with non-zero center offset, then the effect is still active in the
> extreme opposite end of the axis.",
> I wrote that in DInput "if deadband == maxValue(deadband) with
> maximum non-zero center offset, then the effect can only cover half of
> the total region". For reference: maxValue(deadband)=10000
I don't see the difference in those statements here. Note that deadband
is the area where the effect is *not* active.
> Now I'm proposing to let DInput maxValue(deadband) correspond to linux
> deadband 0x7FFF.
> And still allowing linux deadband to be maximal 0xFFFF => two times
> the maximum range of DInput
>
>>
>>
>>> They all have __u16 types, but "/include/uapi/linux/input.h" does not
>>> say what the maximum values are.
>>> I'm doing a proposal to define and document this (in the Linux kernel)
>>> in the following way, also take a look at my attachment
>>> "interactiveAlteredWithRanges.svg" for the modified version:
>>> Max Range of {right_saturation and left_saturation} = 0x7FFF
>>> Because the maximal value of the saturation bound can be only
>>> half of the total range covered by the max negative to max positive
>>> bounds.
>>> And also because they are related to some form of force, and
>>> all other forms of force magnitude in Linux FF have a maximum value of
>>> 0x7FFF
>>
>> I'm not really convinced that the different range from the other
>> magnitude values is a reason enough to change the definition here.
>
> After reviewing this mail, I totally agree with you, sorry for that.
>
>>
>>> Max Range of {deadband} = 0xFFFF
>>> This is a bit harder to explain:
>>> - First, I would suggest to alter the deadband definition in
>>> figure "interactive.fig":
>>> I would define deadband as going from a deadband-bound to
>>> the center (BTW, This is how MSDN defines it: "In other words, the
>>> condition is not active between lOffset minus lDeadBand and lOffset
>>> plus lDeadBand."),
>>> instead of from a deadband-bound to the other deadband-bound.
>>> => Same spec as in DInput.
>>
>> With 0xFFFF being the maximum deadband with center offset 0, it does not
>> matter if deadband is defined as range from center or total width,
>> maximum is 0xFFFF in both cases.
>
> Indeed, because (assume applying 0xFFFF linux deadband with center offset 0):
> a) If DInput maxValue(deadband)=10000 (=half of the total region) maps
> to 0x7FFF in the Linux case (=my proposal) if linux deadband is
> defined as range from center:
> the driver will clamp {0 + 0xFFFF =approx 0 + 2 * 0x7FFF} to
> 0x7FFF for both left as right side of center => Total region is
> covered by the deadband.
> b) If DInput maxValue(deadband)=10000 (=half of the total region) maps
> to 0x7FFF in the Linux case (=my proposal) if linux deadband is
> defined as total width:
> the driver will set {0 + 0xFFFF / 2 =approx 0 + 0x7FFF} for both
> left as right side of center => Total region is covered by the
> deadband.
>
> If we define linux deadband as range from center, like a), the Linux
> FF API can represent more variations of conditional effects than
> DInput can (and also with greater resolution), and thus becomes
> superior in that aspect.
Can you provide any example effect parameters on your proposed system
that would not be possible on DInput?
If I understood correctly, what you are saying is an effect like this
(percentages in parantheses are from left extreme of the axis to right
extreme of the axis):
sat_left = 0xFFFF
sat_right = 0xFFFF
coeff_left = 0x4000
coeff_right = -0x4000
center = 0x6000 (87.5% point)
deadband (your definition) = 0xA000
(62.5% of full area from 87.5% point to either direction, i.e.
25%..150% => (clamping) 25%..100%)
So that deadband can reach further left than it would be with DInput
definition (which maxes at 0x7FFF of your definition, which would not
reach the 25% mark at the left side).
However, from what I can see, you can achieve the exact same effect with
these parameters (DInput deadband definition):
sat_left = 0xFFFF
sat_right = 0x0000
coeff_left = 0x4000
coeff_right = 0x0000
center = 0x0000 (50% point)
deadband (DInput/ours) = 0x8000 (50% total area, or 50% of
center-to-end, so it reaches 25%..75%)
On the left side, the effect is exactly the same as before with same
parameters (sat 0xffff, coeff 0x4000, starts at 25% point).
On the right side the parameters differ, but the end-result is the same,
there is no effect at all:
- In the first example, the right side is fully in deadband area,
causing the effect to have zero effect.
- In my variant with our definition, the right side has zero saturation,
causing the effect to have zero effect.
So the present definitions (and DInput definitions) can achieve the same
effects as your proposed definitions, unless I'm missing something,
making the change unneeded.
> We will have to do the clamping anyway for devices that only accept
> left and right deadband bounds, i.e. Logitech wheels.
> For devices that only accept a single deadband value, like your SWFF2
> as you mentioned below, the linux deadband would only need to be
> shifted "<< 1" before being send to the device, if they accept u16 and
> work exactly like DInput (range from center, and
> maxValue(deadband)=half of total region).
I'm not sure what you are trying to accomplish with "<< 1" here. If the
device can't accept the deadband you want, left-shifting wouldn't fix that.
>>
>>> - Now, knowing that ff_condition_effect->center is __s16:
>>> The worst case scenario is that "center = -32768" or
>>> "center = +32767" while still wanting to cover the whole region (this
>>> is actually not possible with DInput's specs: in that case, they can
>>> only cover half of the total region):
>>> Then, to keep the scale of "center" and "deadband" the
>>> same, "deadband = 65535" = +32767 - -32768 = 0xFFFF
>>
>> Interesting idea. However, if this is not possible in DInput, this means
>> the devices will likely not support it either, since they are using the
>> DInput effect model (as are we).
>
> This is no problem, use "<< 1" as mentioned above.
>
>>
>>
>> I tried to confirm this with my SWFF2 device, but either it has stopped
>> working properly or, more likely, there is a regression in the kernel...
>> no time to debug now, though, so added to my TODO (HID_REQ_GET_REPORT
>> requests don't seem to go through properly).
>>
>>
>>> I expect we will have to add/document the answers to my questions in
>>> the appropriate file "/include/uapi/linux/input.h" or
>>> "/Documentation/input/ff.txt", so that other userspace developers (and
>>> maybe also kernel devs) don't face the same ambiguities.
>>>
>>>
>>> Thank you very much for your time,
>>
>> Thanks for looking into this.
>>
>>> Elias
>>>
>>
>>
>> --
>> Anssi Hannula
>
> Elias
>
--
Anssi Hannula
^ permalink raw reply
* [PATCH] HID: sony: Correct Sixaxis battery reporting
From: Frank Praznik @ 2014-02-15 18:35 UTC (permalink / raw)
To: linux-input; +Cc: jkosina, Frank Praznik
The battery_charging and cable_state flags were backwards on the Sixaxis.
The low bit of report byte 30 is 0 when charging and 1 when not.
Bit 5 of byte 31 is 0 when a USB cable is connected and 1 when not.
Signed-off-by: Frank Praznik <frank.praznik@oh.rr.com>
---
drivers/hid/hid-sony.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
index 8eac6a6..283b2a3 100644
--- a/drivers/hid/hid-sony.c
+++ b/drivers/hid/hid-sony.c
@@ -850,12 +850,12 @@ static void sixaxis_parse_report(struct sony_sc *sc, __u8 *rd, int size)
*/
if (rd[30] >= 0xee) {
battery_capacity = 100;
- battery_charging = rd[30] & 0x01;
+ battery_charging = !(rd[30] & 0x01);
} else {
battery_capacity = sixaxis_battery_capacity[rd[30]];
battery_charging = 0;
}
- cable_state = (rd[31] >> 4) & 0x01;
+ cable_state = !((rd[31] >> 4) & 0x01);
spin_lock_irqsave(&sc->lock, flags);
sc->cable_state = cable_state;
--
1.8.5.3
^ permalink raw reply related
* Re: Questions about the documentation/specification of Linux ForceFeedback input.h
From: Elias Vanderstuyft @ 2014-02-15 20:32 UTC (permalink / raw)
To: Anssi Hannula, Michal Malý
Cc: Dmitry Torokhov, dtor, Johann Deneux, linux-input
In-Reply-To: <52FF73D7.6030108@iki.fi>
On Sat, Feb 15, 2014 at 3:04 PM, Anssi Hannula <anssi.hannula@iki.fi> wrote:
> 15.02.2014 14:14, Elias Vanderstuyft kirjoitti:
>> On Sat, Feb 15, 2014 at 3:05 AM, Anssi Hannula <anssi.hannula@iki.fi> wrote:
>>> 15.02.2014 01:28, Elias Vanderstuyft kirjoitti:
>>>> Hi everyone,
>>>
>>> Hi!
>>>
>>>>
>>>> If you receive my mail, it is either because you:
>>>> - are listed under the MAINTAINERS for "/include/uapi/linux/input.h":
>>>> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/MAINTAINERS#n4410
>>>> - are listed as an author of "/Documentation/input/ff.txt" or of
>>>> "/Documentation/input/interactive.fig"
>>>>
>>>> In the process of testing the new driver (ff-memless-next) Michal
>>>> (CC-ed) is working on, this allowed me to gain more insight in the
>>>> internal workings of Linux FF. That's why I decided to review the
>>>> userspace code that currently uses most of Linux FF functionality:
>>>> Wine's DInput translation layer (and eventually SDL2.)
>>>> Trying to get everything correct, I noticed both
>>>> "/include/uapi/linux/input.h" and "/Documentation/input/ff.txt"
>>>> provide not enough information for the following things:
>>>>
>>>> 1)
>>>> The real meaning of 'directions', declared in
>>>> http://lxr.free-electrons.com/source/include/uapi/linux/input.h#L1113
>>>> :
>>>> "Direction of the effect" is encoded as follows: ...
>>>> But it is not clear whether 'direction of the effect' means either:
>>>> - positive direction of the force the user should apply to counteract
>>>> the force that the joystick applies; or
>>>> - positive direction of the force applied by joystick
>>>> From my intuition, I think the latter is (silently?) meant by input.h
>>>> If you're interested why this is so important, I attached a document
>>>> "DInputVsLinuxDirections.txt" that tries to explain the dilemma if a
>>>> translation layer between DInput and Linux input is to be written.
>>>
>>> From input.h:
>>> * Direction of the effect is encoded as follows:
>>> * 0 deg -> 0x0000 (down)
>>> * 90 deg -> 0x4000 (left)
>>> * 180 deg -> 0x8000 (up)
>>> * 270 deg -> 0xC000 (right)
>>>
>>> The directions in parantheses are the direction of applied force.
>>
>> Alright, thanks! That is invaluable information, maybe it should be
>> added to input.h .
>> It will avoid a lot of confusion for former DInput devs.
>>
>>>
>>> However, there is actually a 1:1 mapping between DInput polar
>>> coordinates and our direction parameter; DInput polar coordinates have 0
>>> deg = up, 90 deg = right, etc, so they are exactly flipped and therefore
>>> match our values due to the reverse definition.
>>>
>>> Looking at your DInputVsLinuxDirections.txt, you seem to have mixed
>>> different definitions of Carts: For DInput you use -1 = north, but for
>>> Linux +1 = up, while you use -1 = west/left for both.
>>
>> So I assume you agree that I got the DInput part right? ((0, -1) = north)
>
> I guess so.
>
>> Then my mistake lies in the assumption that (0, +1) = up, so I should
>> flip the y-axis to correct the Linux part.
>> I'll explain how I originally derived the Linux part:
>> Michal documented (in "ff-memless-next.txt") the Linux directions in
>> the following way:
>> "
>> Direction of the effect's force translates to Cartesian coordinates system
>> as follows:
>> Direction = 0: Down
>> Direction (0; 16384): 3rd quadrant
>> Direction = 16384: Left
>> Direction (16385; 32768): 2nd quadrant
>> Direction = 32768: Up
>> Direction (32769; 49152): 1st quadrant
>> Direction = 49152: Right
>> Direction (49153; 65535) :4th quadrant
>> Direction = 65565: Down
>> "
>
> The above is correct.
>
>> For a Cartesian coordinates system:
>> - The (-1, 0)-axis (=-x) is the intersection of 3rd quadrant and 2nd
>> quadrant => Left
>> - The (0, +1)-axis (=+y) is the intersection of 2nd quadrant and 1st
>> quadrant => Up
>> - The (+1, 0)-axis (=+x) is the intersection of 1st quadrant and 4th
>> quadrant => Right
>> - The (0, -1)-axis (=-y) is the intersection of 4th quadrant and 3rd
>> quadrant => Down
>
> Not sure why you've arbitrarily chosen reverse definition of Y axis
> here,
I really tried to derive the above in a non-arbitrary manner, here you
can verify why I 'chose' the +y axis as the intersection of 2nd
quadrant and 1st quadrant:
http://en.wikipedia.org/wiki/File:Cartesian_coordinates_2D.svg
And because you said that the part about directions in
"ff-memless-next.txt" is correct, it follows that Up (which lies
between the 2nd quadrant and 1st quadrant) corresponds with +y
direction.
I hope this makes sense.
> when all DInput and Linux joysticks, mouses, etc. have -y as "up".
Alright, I did not know that, at least not for Linux. Where in the
Linux documentation can I find this?
>
>> Michal's approach seems logical to me, if he made a mistake, it's
>> caused by the lack of documentation of input.h : it should mention
>> what axes (-x, +x, -y or +y) the words (left, right, down and up)
>> correspond with.
>
> Well, the "left", "right", "down", "up" correspond to directions from
> the user perspective.
That makes sense.
> I don't think that is ambigiuous at all, as I
> don't see how you could consider "down" to be away from user?
Yes, that's true. But it does not say to which axes (and polarity) they map.
>
> I'm not against more documentation if it helps, though.
>
>> So, which interpretation is the right one?
>
> The one where the directions provided in input.h match physical
> direction of the force effect from user perspective on a 2-axis joystick
> device.
That's a nice explanation! For the doc, also add to what axis each
direction corresponds to.
>
>> (I did not find anything in the Linux documentation that states "there
>> is actually a 1:1 mapping between DInput polar coordinates and our
>> direction parameter")
>
> It is not stated explicitely, it just naturally follows.
>
> 1. Both use a clock-wise direction.
> 1. DInput direction definition is reversed, as they use the "counteract"
> direction. We can flip to correct.
> 2. DInput direction has 0 = up/north, we have 0 = down/south,
> i.e. the exact opposite, so we must flip the direction.
>
> DInput direction = flipped(flipped(Linux direction))
> = Linux direction
Alright, now I understand how it works.
But: the thing I wrote about the Cartesian coordinates system (the +y
axis lies between 2nd quadrant and 1st quadrant) is mathematically
correct (see referenced link to wiki), so, to fix the only
contradiction left, we will have to change the explanation about
directions in "ff-memless-next.txt" to the following text:
"
Direction of the effect's force translates to Cartesian coordinates system
as follows:
Direction = 0: Down: +y
Direction (0; 16384): 2nd quadrant
Direction = 16384: Left: -x
Direction (16385; 32768): 3th quadrant
Direction = 32768: Up: -y
Direction (32769; 49152): 4rd quadrant
Direction = 49152: Right: +x
Direction (49153; 65535) :1st quadrant
Direction = 65565: Down: +y
"
As you can see, I only changed the quadrants (and added axes for
convenience), now they agree to
"http://en.wikipedia.org/wiki/File:Cartesian_coordinates_2D.svg", and
to input.h , so everyone should be happy :)
Can you confirm the modification is correct?
>
>>> This causes the
>>> 1st and 3rd entries on both of the Mapping tables to be reversed. When
>>> that is fixed, the table #2 shows the correct result.
>>>
>>>> 2)
>>>> It is not clear how to create a FF effect with infinite duration
>>>> (replay.length):
>>>> http://lxr.free-electrons.com/source/include/uapi/linux/input.h#L966
>>>> Only the following is written about duration values in general:
>>>> "All duration values are expressed in ms. Values above 32767 ms
>>>> (0x7fff) should not be used and have unspecified results."
>>>> Michal solved this by associating replay.length==0 for infinite, which
>>>> makes sense since otherwise the effect would not have been started
>>>> anyway.
>>>
>>> Infinite duration can be achieved with length = 0.
>>
>> Thanks! This has to be documented.
>>
>>>
>>>> 3)
>>>> Many Linux FF effect parameters don't have a clear explanation of the
>>>> allowed range, and their corresponding meaning at the extrema.
>>>> Here I list the ones that need more explanation, also take a look at
>>>> "interactive.fig" in the kernel documentation (but also included as
>>>> attachment):
>>>> - left_saturation and right_saturation; and
>>>
>>> left_saturation = maximum force on the negative ("left") side of the
>>> center point
>>> right_saturation = same for positive side
>>>
>>> 0x0 => no force,
>>> 0xFFFF => maximum force.
>>
>> OK, thanks for giving the definition. I think these things can be
>> understood from "interactive.fig", so there's no need to write
>> additional doc about this topic.
>>
>>>
>>>> - deadband
>>>
>>> The range from center point wherein the effect has no effect
>>
>> Notice this contradicts with "interactive.fig", this figure defines
>> 'deadband' as bound-to-bound, not as center-to-bound (as with DInput).
>
> There is no difference between those definitions that I can see:
>
> Axis range
> |------------------------------------------------------|
>
> Definition from interactive.fig, bound-to-bound:
> 0x0:
> |--------------------------X---------------------------|
> 0x8000 (covers half of the end-to-end area):
> |------------XXXXXXXXXXXXXXXXXXXXXXXXXXXXXX------------|
> 0xFFFF:
> |XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX|
Alright, agreed with this part: deadband units or dimensions are the
same as the ones of the center offset parameter.
>
> Center-to-bound definition:
> 0x0:
> |--------------------------X---------------------------|
> 0x8000 (covers half of the center-to-end area):
> |------------XXXXXXXXXXXXXXXXXXXXXXXXXXXXXX------------|
> 0xFFFF:
> |XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX|
Ah, I see, this is where we disagree: now you're not working with the
same units or dimensions as the ones of the center offset parameter:
you have multiplied them by a factor 2 (that's where my "<< 1" came
from, for going from my definition to your definition).
This is my proposal of how it could be:
Center-to-bound definition (same dimensions as center offset parameter):
0x0:
|--------------------------X---------------------------|
0x4000 (covers half of the center-to-end area):
|------------XXXXXXXXXXXXXXXXXXXXXXXXXXXXXX------------|
0x8000 (covers whole the center-to-end area):
|XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX|
0xC000:
|XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX| (clamped)
0xFFFF:
|XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX| (clamped)
>
>
>>> , with
>>> 0x0 => no dead band
>>> 0xFFFF => dead band encompassing the entire axis, effect not active
>>> anywhere.
>>>
>>> Assuming center offset of 0, though. Not sure how the currently
>>> supported devices interpret 0xFFFF with non-zero center offset, i.e. if
>>> the effect is then still active in the extreme opposite end of the axis.
>>> You wrote below that this is indeed the case with DInput, so it is
>>> highly likely this is how the devices handle it as well.
>>
>> Careful, here there's a catch:
>> I did not write that in DInput "if deadband == maxValue(deadband)
>> with non-zero center offset, then the effect is still active in the
>> extreme opposite end of the axis.",
>> I wrote that in DInput "if deadband == maxValue(deadband) with
>> maximum non-zero center offset, then the effect can only cover half of
>> the total region". For reference: maxValue(deadband)=10000
>
> I don't see the difference in those statements here.
There is, when assuming same dimensions as center offset parameter.
Otherwise, you're right.
> Note that deadband
> is the area where the effect is *not* active.
I know, that's why it's called *dead*-band ;)
>
>> Now I'm proposing to let DInput maxValue(deadband) correspond to linux
>> deadband 0x7FFF.
>> And still allowing linux deadband to be maximal 0xFFFF => two times
>> the maximum range of DInput
>>
>>>
>>>
>>>> They all have __u16 types, but "/include/uapi/linux/input.h" does not
>>>> say what the maximum values are.
>>>> I'm doing a proposal to define and document this (in the Linux kernel)
>>>> in the following way, also take a look at my attachment
>>>> "interactiveAlteredWithRanges.svg" for the modified version:
>>>> Max Range of {right_saturation and left_saturation} = 0x7FFF
>>>> Because the maximal value of the saturation bound can be only
>>>> half of the total range covered by the max negative to max positive
>>>> bounds.
>>>> And also because they are related to some form of force, and
>>>> all other forms of force magnitude in Linux FF have a maximum value of
>>>> 0x7FFF
>>>
>>> I'm not really convinced that the different range from the other
>>> magnitude values is a reason enough to change the definition here.
>>
>> After reviewing this mail, I totally agree with you, sorry for that.
>>
>>>
>>>> Max Range of {deadband} = 0xFFFF
>>>> This is a bit harder to explain:
>>>> - First, I would suggest to alter the deadband definition in
>>>> figure "interactive.fig":
>>>> I would define deadband as going from a deadband-bound to
>>>> the center (BTW, This is how MSDN defines it: "In other words, the
>>>> condition is not active between lOffset minus lDeadBand and lOffset
>>>> plus lDeadBand."),
>>>> instead of from a deadband-bound to the other deadband-bound.
>>>> => Same spec as in DInput.
>>>
>>> With 0xFFFF being the maximum deadband with center offset 0, it does not
>>> matter if deadband is defined as range from center or total width,
>>> maximum is 0xFFFF in both cases.
>>
>> Indeed, because (assume applying 0xFFFF linux deadband with center offset 0):
>> a) If DInput maxValue(deadband)=10000 (=half of the total region) maps
>> to 0x7FFF in the Linux case (=my proposal) if linux deadband is
>> defined as range from center:
>> the driver will clamp {0 + 0xFFFF =approx 0 + 2 * 0x7FFF} to
>> 0x7FFF for both left as right side of center => Total region is
>> covered by the deadband.
>> b) If DInput maxValue(deadband)=10000 (=half of the total region) maps
>> to 0x7FFF in the Linux case (=my proposal) if linux deadband is
>> defined as total width:
>> the driver will set {0 + 0xFFFF / 2 =approx 0 + 0x7FFF} for both
>> left as right side of center => Total region is covered by the
>> deadband.
>>
>> If we define linux deadband as range from center, like a), the Linux
>> FF API can represent more variations of conditional effects than
>> DInput can (and also with greater resolution), and thus becomes
>> superior in that aspect.
>
> Can you provide any example effect parameters on your proposed system
> that would not be possible on DInput?
Sure:
Center-to-bound definition (same dimensions as center offset parameter):
Assume center offset is set to the maximum non-zero value +32767
0x0:
|-----------------------------------------------------X|
0x4000 (covers a quarter of the end-to-end area):
|----------------------------------------XXXXXXXXXXXXXX|
0x8000 (covers half of the end-to-end area):
|---------------------------XXXXXXXXXXXXXXXXXXXXXXXXXXX|
0xC000 (covers three quarters of the end-to-end area):
|-------------XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX|
0xFFFF (covers whole the end-to-end area):
|XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX|
>
> If I understood correctly, what you are saying is an effect like this
> (percentages in parantheses are from left extreme of the axis to right
> extreme of the axis):
> sat_left = 0xFFFF
> sat_right = 0xFFFF
> coeff_left = 0x4000
> coeff_right = -0x4000
> center = 0x6000 (87.5% point)
> deadband (your definition) = 0xA000
> (62.5% of full area from 87.5% point to either direction, i.e.
> 25%..150% => (clamping) 25%..100%)
>
> So that deadband can reach further left than it would be with DInput
> definition (which maxes at 0x7FFF of your definition, which would not
> reach the 25% mark at the left side).
Exactly.
>
> However, from what I can see, you can achieve the exact same effect with
> these parameters (DInput deadband definition):
> sat_left = 0xFFFF
> sat_right = 0x0000
> coeff_left = 0x4000
> coeff_right = 0x0000
> center = 0x0000 (50% point)
> deadband (DInput/ours) = 0x8000 (50% total area, or 50% of
> center-to-end, so it reaches 25%..75%)
>
> On the left side, the effect is exactly the same as before with same
> parameters (sat 0xffff, coeff 0x4000, starts at 25% point).
>
> On the right side the parameters differ, but the end-result is the same,
> there is no effect at all:
> - In the first example, the right side is fully in deadband area,
> causing the effect to have zero effect.
> - In my variant with our definition, the right side has zero saturation,
> causing the effect to have zero effect.
>
>
> So the present definitions (and DInput definitions) can achieve the same
> effects as your proposed definitions, unless I'm missing something,
> making the change unneeded.
Agreed.
But the benefit could be to make the user application less complex,
since otherwise, when a special case like the one you mentioned above
(with deadband larger than half of the total region), they would also
need to change the saturation values. This is not really a problem
when the conditional effect is a 'static' one (i.e. center and
deadband do not change over time), but it might come in handy for
'dynamic' conditional effects.
Although honestly I think that situation is extremely rare, so I'm OK
with it if we leave the current definition as is.
And now I realize that for devices that require the deadband paramater
to be passed explicitly (and use the present definition) like SWFF2,
this either would not work, or would require additional (and maybe
complex) kernel driver code, which is not what we want.
So, I agree to leave its definition unchanged, sorry for the discussion ;)
I just have to be sure about this to improve Wine's DInput translation layer.
>
>> We will have to do the clamping anyway for devices that only accept
>> left and right deadband bounds, i.e. Logitech wheels.
>> For devices that only accept a single deadband value, like your SWFF2
>> as you mentioned below, the linux deadband would only need to be
>> shifted "<< 1" before being send to the device, if they accept u16 and
>> work exactly like DInput (range from center, and
>> maxValue(deadband)=half of total region).
>
> I'm not sure what you are trying to accomplish with "<< 1" here. If the
> device can't accept the deadband you want, left-shifting wouldn't fix that.
See above about the factor 2.
>
>>>
>>>> - Now, knowing that ff_condition_effect->center is __s16:
>>>> The worst case scenario is that "center = -32768" or
>>>> "center = +32767" while still wanting to cover the whole region (this
>>>> is actually not possible with DInput's specs: in that case, they can
>>>> only cover half of the total region):
>>>> Then, to keep the scale of "center" and "deadband" the
>>>> same, "deadband = 65535" = +32767 - -32768 = 0xFFFF
>>>
>>> Interesting idea. However, if this is not possible in DInput, this means
>>> the devices will likely not support it either, since they are using the
>>> DInput effect model (as are we).
>>
>> This is no problem, use "<< 1" as mentioned above.
>>
>>>
>>>
>>> I tried to confirm this with my SWFF2 device, but either it has stopped
>>> working properly or, more likely, there is a regression in the kernel...
>>> no time to debug now, though, so added to my TODO (HID_REQ_GET_REPORT
>>> requests don't seem to go through properly).
>>>
>>>
>>>> I expect we will have to add/document the answers to my questions in
>>>> the appropriate file "/include/uapi/linux/input.h" or
>>>> "/Documentation/input/ff.txt", so that other userspace developers (and
>>>> maybe also kernel devs) don't face the same ambiguities.
>>>>
>>>>
>>>> Thank you very much for your time,
>>>
>>> Thanks for looking into this.
>>>
>>>> Elias
>>>>
>>>
>>>
>>> --
>>> Anssi Hannula
>>
>> Elias
>>
>
>
> --
> Anssi Hannula
Elias
^ permalink raw reply
* Re: [PATCH 2/2] input: adp5588-keys: get value from data out when dir is out
From: Dmitry Torokhov @ 2014-02-15 21:14 UTC (permalink / raw)
To: Jean-Francois Dagenais
Cc: michael.hennerich, linus.walleij, gnurou, linux-gpio, linux-input
In-Reply-To: <1392051929-32290-2-git-send-email-jeff.dagenais@gmail.com>
On Mon, Feb 10, 2014 at 12:05:29PM -0500, Jean-Francois Dagenais wrote:
> As discussed here: http://ez.analog.com/message/35852,
> the 5587 revC and 5588 revB spec sheets contain a mistake
> in the GPIO_DAT_STATx register description.
>
> According to R.Shnell at ADI, as well as my own
> observations, it should read:
> "GPIO data status (shows GPIO state when read for inputs)".
>
> This commit changes the get value function accordingly.
>
> Signed-off-by: Jean-Francois Dagenais <jeff.dagenais@gmail.com>
Applied, thank you.
> ---
> drivers/input/keyboard/adp5588-keys.c | 12 +++++++++++-
> 1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/input/keyboard/adp5588-keys.c b/drivers/input/keyboard/adp5588-keys.c
> index bb3b57b..5ef7fcf 100644
> --- a/drivers/input/keyboard/adp5588-keys.c
> +++ b/drivers/input/keyboard/adp5588-keys.c
> @@ -76,8 +76,18 @@ static int adp5588_gpio_get_value(struct gpio_chip *chip, unsigned off)
> struct adp5588_kpad *kpad = container_of(chip, struct adp5588_kpad, gc);
> unsigned int bank = ADP5588_BANK(kpad->gpiomap[off]);
> unsigned int bit = ADP5588_BIT(kpad->gpiomap[off]);
> + int val;
>
> - return !!(adp5588_read(kpad->client, GPIO_DAT_STAT1 + bank) & bit);
> + mutex_lock(&kpad->gpio_lock);
> +
> + if (kpad->dir[bank] & bit)
> + val = kpad->dat_out[bank];
> + else
> + val = adp5588_read(kpad->client, GPIO_DAT_STAT1 + bank);
> +
> + mutex_unlock(&kpad->gpio_lock);
> +
> + return !!(val & bit);
> }
>
> static void adp5588_gpio_set_value(struct gpio_chip *chip,
> --
> 1.8.5.3
>
--
Dmitry
^ permalink raw reply
* Re: Questions about the documentation/specification of Linux ForceFeedback input.h
From: Anssi Hannula @ 2014-02-15 23:04 UTC (permalink / raw)
To: Elias Vanderstuyft, Michal Malý
Cc: Dmitry Torokhov, dtor, Johann Deneux, linux-input
In-Reply-To: <CADbOyBROrPtqptnnh+Xtb9+X7EhGMudaHRai2wQLga5vca=rxg@mail.gmail.com>
15.02.2014 22:32, Elias Vanderstuyft kirjoitti:
> On Sat, Feb 15, 2014 at 3:04 PM, Anssi Hannula <anssi.hannula@iki.fi> wrote:
>> 15.02.2014 14:14, Elias Vanderstuyft kirjoitti:
>>> On Sat, Feb 15, 2014 at 3:05 AM, Anssi Hannula <anssi.hannula@iki.fi> wrote:
>>>> 15.02.2014 01:28, Elias Vanderstuyft kirjoitti:
[...]
>>>>> 1)
>>>>> The real meaning of 'directions', declared in
>>>>> http://lxr.free-electrons.com/source/include/uapi/linux/input.h#L1113
>>>>> :
>>>>> "Direction of the effect" is encoded as follows: ...
>>>>> But it is not clear whether 'direction of the effect' means either:
>>>>> - positive direction of the force the user should apply to counteract
>>>>> the force that the joystick applies; or
>>>>> - positive direction of the force applied by joystick
>>>>> From my intuition, I think the latter is (silently?) meant by input.h
>>>>> If you're interested why this is so important, I attached a document
>>>>> "DInputVsLinuxDirections.txt" that tries to explain the dilemma if a
>>>>> translation layer between DInput and Linux input is to be written.
>>>>
>>>> From input.h:
>>>> * Direction of the effect is encoded as follows:
>>>> * 0 deg -> 0x0000 (down)
>>>> * 90 deg -> 0x4000 (left)
>>>> * 180 deg -> 0x8000 (up)
>>>> * 270 deg -> 0xC000 (right)
>>>>
>>>> The directions in parantheses are the direction of applied force.
>>>
>>> Alright, thanks! That is invaluable information, maybe it should be
>>> added to input.h .
>>> It will avoid a lot of confusion for former DInput devs.
>>>
>>>>
>>>> However, there is actually a 1:1 mapping between DInput polar
>>>> coordinates and our direction parameter; DInput polar coordinates have 0
>>>> deg = up, 90 deg = right, etc, so they are exactly flipped and therefore
>>>> match our values due to the reverse definition.
>>>>
>>>> Looking at your DInputVsLinuxDirections.txt, you seem to have mixed
>>>> different definitions of Carts: For DInput you use -1 = north, but for
>>>> Linux +1 = up, while you use -1 = west/left for both.
>>>
>>> So I assume you agree that I got the DInput part right? ((0, -1) = north)
>>
>> I guess so.
>>
>>> Then my mistake lies in the assumption that (0, +1) = up, so I should
>>> flip the y-axis to correct the Linux part.
>>> I'll explain how I originally derived the Linux part:
>>> Michal documented (in "ff-memless-next.txt") the Linux directions in
>>> the following way:
>>> "
>>> Direction of the effect's force translates to Cartesian coordinates system
>>> as follows:
>>> Direction = 0: Down
>>> Direction (0; 16384): 3rd quadrant
>>> Direction = 16384: Left
>>> Direction (16385; 32768): 2nd quadrant
>>> Direction = 32768: Up
>>> Direction (32769; 49152): 1st quadrant
>>> Direction = 49152: Right
>>> Direction (49153; 65535) :4th quadrant
>>> Direction = 65565: Down
>>> "
>>
>> The above is correct.
>>
>>> For a Cartesian coordinates system:
>>> - The (-1, 0)-axis (=-x) is the intersection of 3rd quadrant and 2nd
>>> quadrant => Left
>>> - The (0, +1)-axis (=+y) is the intersection of 2nd quadrant and 1st
>>> quadrant => Up
>>> - The (+1, 0)-axis (=+x) is the intersection of 1st quadrant and 4th
>>> quadrant => Right
>>> - The (0, -1)-axis (=-y) is the intersection of 4th quadrant and 3rd
>>> quadrant => Down
>>
>> Not sure why you've arbitrarily chosen reverse definition of Y axis
>> here,
>
> I really tried to derive the above in a non-arbitrary manner, here you
> can verify why I 'chose' the +y axis as the intersection of 2nd
> quadrant and 1st quadrant:
> http://en.wikipedia.org/wiki/File:Cartesian_coordinates_2D.svg
> And because you said that the part about directions in
> "ff-memless-next.txt" is correct, it follows that Up (which lies
> between the 2nd quadrant and 1st quadrant) corresponds with +y
> direction.
> I hope this makes sense.
>
>> when all DInput and Linux joysticks, mouses, etc. have -y as "up".
>
> Alright, I did not know that, at least not for Linux. Where in the
> Linux documentation can I find this?
I don't think it is explicitely said either. I guess one could say it is
"standard industry practice" (not limited to input, by the way, with
e.g. in images the pixel row and column values are increasing right and
down, not right and up).
I don't oppose more explicit documentation, though.
>>
>>> Michal's approach seems logical to me, if he made a mistake, it's
>>> caused by the lack of documentation of input.h : it should mention
>>> what axes (-x, +x, -y or +y) the words (left, right, down and up)
>>> correspond with.
>>
>> Well, the "left", "right", "down", "up" correspond to directions from
>> the user perspective.
>
> That makes sense.
>
>> I don't think that is ambigiuous at all, as I
>> don't see how you could consider "down" to be away from user?
>
> Yes, that's true. But it does not say to which axes (and polarity) they map.
>
>>
>> I'm not against more documentation if it helps, though.
>>
>>> So, which interpretation is the right one?
>>
>> The one where the directions provided in input.h match physical
>> direction of the force effect from user perspective on a 2-axis joystick
>> device.
>
> That's a nice explanation! For the doc, also add to what axis each
> direction corresponds to.
>
>>
>>> (I did not find anything in the Linux documentation that states "there
>>> is actually a 1:1 mapping between DInput polar coordinates and our
>>> direction parameter")
>>
>> It is not stated explicitely, it just naturally follows.
>>
>> 1. Both use a clock-wise direction.
>> 1. DInput direction definition is reversed, as they use the "counteract"
>> direction. We can flip to correct.
>> 2. DInput direction has 0 = up/north, we have 0 = down/south,
>> i.e. the exact opposite, so we must flip the direction.
>>
>> DInput direction = flipped(flipped(Linux direction))
>> = Linux direction
>
> Alright, now I understand how it works.
> But: the thing I wrote about the Cartesian coordinates system (the +y
> axis lies between 2nd quadrant and 1st quadrant) is mathematically
> correct (see referenced link to wiki), so, to fix the only
> contradiction left, we will have to change the explanation about
> directions in "ff-memless-next.txt" to the following text:
> "
> Direction of the effect's force translates to Cartesian coordinates system
> as follows:
> Direction = 0: Down: +y
> Direction (0; 16384): 2nd quadrant
> Direction = 16384: Left: -x
> Direction (16385; 32768): 3th quadrant
> Direction = 32768: Up: -y
> Direction (32769; 49152): 4rd quadrant
> Direction = 49152: Right: +x
> Direction (49153; 65535) :1st quadrant
> Direction = 65565: Down: +y
> "
> As you can see, I only changed the quadrants (and added axes for
> convenience), now they agree to
> "http://en.wikipedia.org/wiki/File:Cartesian_coordinates_2D.svg", and
> to input.h , so everyone should be happy :)
> Can you confirm the modification is correct?
I think so.
However, assigning quadrants here is a bit confusing to me here with the
non-mathematical-traditional reversed Y axis. I.e. my first instinct is
that the upper-right quadrant is the first one, while it is not the case
here...
But if it helps...
>>
>>>> This causes the
>>>> 1st and 3rd entries on both of the Mapping tables to be reversed. When
>>>> that is fixed, the table #2 shows the correct result.
[...]
>>>>> 3)
>>>>> Many Linux FF effect parameters don't have a clear explanation of the
>>>>> allowed range, and their corresponding meaning at the extrema.
>>>>> Here I list the ones that need more explanation, also take a look at
>>>>> "interactive.fig" in the kernel documentation (but also included as
>>>>> attachment):
>>>>> - left_saturation and right_saturation; and
>>>>
>>>> left_saturation = maximum force on the negative ("left") side of the
>>>> center point
>>>> right_saturation = same for positive side
>>>>
>>>> 0x0 => no force,
>>>> 0xFFFF => maximum force.
>>>
>>> OK, thanks for giving the definition. I think these things can be
>>> understood from "interactive.fig", so there's no need to write
>>> additional doc about this topic.
>>>
>>>>
>>>>> - deadband
>>>>
>>>> The range from center point wherein the effect has no effect
>>>
>>> Notice this contradicts with "interactive.fig", this figure defines
>>> 'deadband' as bound-to-bound, not as center-to-bound (as with DInput).
>>
>> There is no difference between those definitions that I can see:
>>
>> Axis range
>> |------------------------------------------------------|
>>
>> Definition from interactive.fig, bound-to-bound:
>> 0x0:
>> |--------------------------X---------------------------|
>> 0x8000 (covers half of the end-to-end area):
>> |------------XXXXXXXXXXXXXXXXXXXXXXXXXXXXXX------------|
>> 0xFFFF:
>> |XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX|
>
> Alright, agreed with this part: deadband units or dimensions are the
> same as the ones of the center offset parameter.
>
>>
>> Center-to-bound definition:
>> 0x0:
>> |--------------------------X---------------------------|
>> 0x8000 (covers half of the center-to-end area):
>> |------------XXXXXXXXXXXXXXXXXXXXXXXXXXXXXX------------|
>> 0xFFFF:
>> |XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX|
>
> Ah, I see, this is where we disagree: now you're not working with the
> same units or dimensions as the ones of the center offset parameter:
> you have multiplied them by a factor 2 (that's where my "<< 1" came
> from, for going from my definition to your definition).
Yes, the center offset and deadband are in "relative" units, so due to
the different value range the scales differ by a factor of 2.
I agree it is a bit unfortunate the scales do not match, but IMHO not a
totally hopeless situation.
> This is my proposal of how it could be:
>
> Center-to-bound definition (same dimensions as center offset parameter):
> 0x0:
> |--------------------------X---------------------------|
> 0x4000 (covers half of the center-to-end area):
> |------------XXXXXXXXXXXXXXXXXXXXXXXXXXXXXX------------|
> 0x8000 (covers whole the center-to-end area):
> |XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX|
> 0xC000:
> |XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX| (clamped)
> 0xFFFF:
> |XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX| (clamped)
Yes, this I understood.
>>
>>
>>>> , with
>>>> 0x0 => no dead band
>>>> 0xFFFF => dead band encompassing the entire axis, effect not active
>>>> anywhere.
>>>>
>>>> Assuming center offset of 0, though. Not sure how the currently
>>>> supported devices interpret 0xFFFF with non-zero center offset, i.e. if
>>>> the effect is then still active in the extreme opposite end of the axis.
>>>> You wrote below that this is indeed the case with DInput, so it is
>>>> highly likely this is how the devices handle it as well.
>>>
>>> Careful, here there's a catch:
>>> I did not write that in DInput "if deadband == maxValue(deadband)
>>> with non-zero center offset, then the effect is still active in the
>>> extreme opposite end of the axis.",
>>> I wrote that in DInput "if deadband == maxValue(deadband) with
>>> maximum non-zero center offset, then the effect can only cover half of
>>> the total region". For reference: maxValue(deadband)=10000
>>
>> I don't see the difference in those statements here.
>
> There is, when assuming same dimensions as center offset parameter.
> Otherwise, you're right.
Right.
>> Note that deadband
>> is the area where the effect is *not* active.
>
> I know, that's why it's called *dead*-band ;)
>
>>
>>> Now I'm proposing to let DInput maxValue(deadband) correspond to linux
>>> deadband 0x7FFF.
>>> And still allowing linux deadband to be maximal 0xFFFF => two times
>>> the maximum range of DInput
>>>
>>>>
>>>>
>>>>> They all have __u16 types, but "/include/uapi/linux/input.h" does not
>>>>> say what the maximum values are.
>>>>> I'm doing a proposal to define and document this (in the Linux kernel)
>>>>> in the following way, also take a look at my attachment
>>>>> "interactiveAlteredWithRanges.svg" for the modified version:
>>>>> Max Range of {right_saturation and left_saturation} = 0x7FFF
>>>>> Because the maximal value of the saturation bound can be only
>>>>> half of the total range covered by the max negative to max positive
>>>>> bounds.
>>>>> And also because they are related to some form of force, and
>>>>> all other forms of force magnitude in Linux FF have a maximum value of
>>>>> 0x7FFF
>>>>
>>>> I'm not really convinced that the different range from the other
>>>> magnitude values is a reason enough to change the definition here.
>>>
>>> After reviewing this mail, I totally agree with you, sorry for that.
>>>
>>>>
>>>>> Max Range of {deadband} = 0xFFFF
>>>>> This is a bit harder to explain:
>>>>> - First, I would suggest to alter the deadband definition in
>>>>> figure "interactive.fig":
>>>>> I would define deadband as going from a deadband-bound to
>>>>> the center (BTW, This is how MSDN defines it: "In other words, the
>>>>> condition is not active between lOffset minus lDeadBand and lOffset
>>>>> plus lDeadBand."),
>>>>> instead of from a deadband-bound to the other deadband-bound.
>>>>> => Same spec as in DInput.
>>>>
>>>> With 0xFFFF being the maximum deadband with center offset 0, it does not
>>>> matter if deadband is defined as range from center or total width,
>>>> maximum is 0xFFFF in both cases.
>>>
>>> Indeed, because (assume applying 0xFFFF linux deadband with center offset 0):
>>> a) If DInput maxValue(deadband)=10000 (=half of the total region) maps
>>> to 0x7FFF in the Linux case (=my proposal) if linux deadband is
>>> defined as range from center:
>>> the driver will clamp {0 + 0xFFFF =approx 0 + 2 * 0x7FFF} to
>>> 0x7FFF for both left as right side of center => Total region is
>>> covered by the deadband.
>>> b) If DInput maxValue(deadband)=10000 (=half of the total region) maps
>>> to 0x7FFF in the Linux case (=my proposal) if linux deadband is
>>> defined as total width:
>>> the driver will set {0 + 0xFFFF / 2 =approx 0 + 0x7FFF} for both
>>> left as right side of center => Total region is covered by the
>>> deadband.
>>>
>>> If we define linux deadband as range from center, like a), the Linux
>>> FF API can represent more variations of conditional effects than
>>> DInput can (and also with greater resolution), and thus becomes
>>> superior in that aspect.
>>
>> Can you provide any example effect parameters on your proposed system
>> that would not be possible on DInput?
>
> Sure:
>
> Center-to-bound definition (same dimensions as center offset parameter):
> Assume center offset is set to the maximum non-zero value +32767
> 0x0:
> |-----------------------------------------------------X|
> 0x4000 (covers a quarter of the end-to-end area):
> |----------------------------------------XXXXXXXXXXXXXX|
> 0x8000 (covers half of the end-to-end area):
> |---------------------------XXXXXXXXXXXXXXXXXXXXXXXXXXX|
> 0xC000 (covers three quarters of the end-to-end area):
> |-------------XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX|
> 0xFFFF (covers whole the end-to-end area):
> |XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX|
I meant a complete set of effect parameters that will cause an effect
not possible with the DInput definitions, like I provided below. Your
above example seems to be the same case I covered below in my example,
in which case you can simply move the center point left a bit and use a
zero right-side saturation value, without needing a large deadband value.
>>
>> If I understood correctly, what you are saying is an effect like this
>> (percentages in parantheses are from left extreme of the axis to right
>> extreme of the axis):
>> sat_left = 0xFFFF
>> sat_right = 0xFFFF
>> coeff_left = 0x4000
>> coeff_right = -0x4000
>> center = 0x6000 (87.5% point)
>> deadband (your definition) = 0xA000
>> (62.5% of full area from 87.5% point to either direction, i.e.
>> 25%..150% => (clamping) 25%..100%)
>>
>> So that deadband can reach further left than it would be with DInput
>> definition (which maxes at 0x7FFF of your definition, which would not
>> reach the 25% mark at the left side).
>
> Exactly.
>
>>
>> However, from what I can see, you can achieve the exact same effect with
>> these parameters (DInput deadband definition):
>> sat_left = 0xFFFF
>> sat_right = 0x0000
>> coeff_left = 0x4000
>> coeff_right = 0x0000
>> center = 0x0000 (50% point)
>> deadband (DInput/ours) = 0x8000 (50% total area, or 50% of
>> center-to-end, so it reaches 25%..75%)
>>
>> On the left side, the effect is exactly the same as before with same
>> parameters (sat 0xffff, coeff 0x4000, starts at 25% point).
>>
>> On the right side the parameters differ, but the end-result is the same,
>> there is no effect at all:
>> - In the first example, the right side is fully in deadband area,
>> causing the effect to have zero effect.
>> - In my variant with our definition, the right side has zero saturation,
>> causing the effect to have zero effect.
>>
>>
>> So the present definitions (and DInput definitions) can achieve the same
>> effects as your proposed definitions, unless I'm missing something,
>> making the change unneeded.
>
> Agreed.
>
> But the benefit could be to make the user application less complex,
> since otherwise, when a special case like the one you mentioned above
> (with deadband larger than half of the total region), they would also
> need to change the saturation values. This is not really a problem
> when the conditional effect is a 'static' one (i.e. center and
> deadband do not change over time), but it might come in handy for
> 'dynamic' conditional effects.
True. Though it will likely not outweigh the benefit of using the same
definition as DInput.
> Although honestly I think that situation is extremely rare, so I'm OK
> with it if we leave the current definition as is.
> And now I realize that for devices that require the deadband paramater
> to be passed explicitly (and use the present definition) like SWFF2,
> this either would not work, or would require additional (and maybe
> complex) kernel driver code, which is not what we want.
>
> So, I agree to leave its definition unchanged, sorry for the discussion ;)
> I just have to be sure about this to improve Wine's DInput translation layer.
No problem with the discussion, better too much discussion than not
enough :)
As you've seen, the effect model should very closely match the DInput
model (as that is what the devices tend to support), mostly just the
units are different (our units use the 16-bit range as a whole, i.e.
there are no out-of-range values).
If you have any specific suggestions (patches preferred, otherwise this
ends up further from the head of my TODO list :) ) to improve docs,
those would be welcome.
>>> We will have to do the clamping anyway for devices that only accept
>>> left and right deadband bounds, i.e. Logitech wheels.
>>> For devices that only accept a single deadband value, like your SWFF2
>>> as you mentioned below, the linux deadband would only need to be
>>> shifted "<< 1" before being send to the device, if they accept u16 and
>>> work exactly like DInput (range from center, and
>>> maxValue(deadband)=half of total region).
>>
>> I'm not sure what you are trying to accomplish with "<< 1" here. If the
>> device can't accept the deadband you want, left-shifting wouldn't fix that.
>
> See above about the factor 2.
>
>>
>>>>
>>>>> - Now, knowing that ff_condition_effect->center is __s16:
>>>>> The worst case scenario is that "center = -32768" or
>>>>> "center = +32767" while still wanting to cover the whole region (this
>>>>> is actually not possible with DInput's specs: in that case, they can
>>>>> only cover half of the total region):
>>>>> Then, to keep the scale of "center" and "deadband" the
>>>>> same, "deadband = 65535" = +32767 - -32768 = 0xFFFF
>>>>
>>>> Interesting idea. However, if this is not possible in DInput, this means
>>>> the devices will likely not support it either, since they are using the
>>>> DInput effect model (as are we).
>>>
>>> This is no problem, use "<< 1" as mentioned above.
>>>
>>>>
>>>>
>>>> I tried to confirm this with my SWFF2 device, but either it has stopped
>>>> working properly or, more likely, there is a regression in the kernel...
>>>> no time to debug now, though, so added to my TODO (HID_REQ_GET_REPORT
>>>> requests don't seem to go through properly).
>>>>
>>>>
>>>>> I expect we will have to add/document the answers to my questions in
>>>>> the appropriate file "/include/uapi/linux/input.h" or
>>>>> "/Documentation/input/ff.txt", so that other userspace developers (and
>>>>> maybe also kernel devs) don't face the same ambiguities.
>>>>>
>>>>>
>>>>> Thank you very much for your time,
>>>>
>>>> Thanks for looking into this.
>>>>
>>>>> Elias
>>>>>
>>>>
>>>>
>>>> --
>>>> Anssi Hannula
>>>
>>> Elias
>>>
>>
>>
>> --
>> Anssi Hannula
>
> Elias
>
--
Anssi Hannula
^ permalink raw reply
* Re: Questions about the documentation/specification of Linux ForceFeedback input.h
From: Elias Vanderstuyft @ 2014-02-15 23:47 UTC (permalink / raw)
To: Anssi Hannula
Cc: Michal Malý, Dmitry Torokhov, dtor, Johann Deneux,
linux-input
In-Reply-To: <52FFF276.1060704@iki.fi>
On Sun, Feb 16, 2014 at 12:04 AM, Anssi Hannula <anssi.hannula@iki.fi> wrote:
> 15.02.2014 22:32, Elias Vanderstuyft kirjoitti:
>> On Sat, Feb 15, 2014 at 3:04 PM, Anssi Hannula <anssi.hannula@iki.fi> wrote:
>>> 15.02.2014 14:14, Elias Vanderstuyft kirjoitti:
>>>> On Sat, Feb 15, 2014 at 3:05 AM, Anssi Hannula <anssi.hannula@iki.fi> wrote:
>>>>> 15.02.2014 01:28, Elias Vanderstuyft kirjoitti:
> [...]
>>>>>> 1)
>>>>>> The real meaning of 'directions', declared in
>>>>>> http://lxr.free-electrons.com/source/include/uapi/linux/input.h#L1113
>>>>>> :
>>>>>> "Direction of the effect" is encoded as follows: ...
>>>>>> But it is not clear whether 'direction of the effect' means either:
>>>>>> - positive direction of the force the user should apply to counteract
>>>>>> the force that the joystick applies; or
>>>>>> - positive direction of the force applied by joystick
>>>>>> From my intuition, I think the latter is (silently?) meant by input.h
>>>>>> If you're interested why this is so important, I attached a document
>>>>>> "DInputVsLinuxDirections.txt" that tries to explain the dilemma if a
>>>>>> translation layer between DInput and Linux input is to be written.
>>>>>
>>>>> From input.h:
>>>>> * Direction of the effect is encoded as follows:
>>>>> * 0 deg -> 0x0000 (down)
>>>>> * 90 deg -> 0x4000 (left)
>>>>> * 180 deg -> 0x8000 (up)
>>>>> * 270 deg -> 0xC000 (right)
>>>>>
>>>>> The directions in parantheses are the direction of applied force.
>>>>
>>>> Alright, thanks! That is invaluable information, maybe it should be
>>>> added to input.h .
>>>> It will avoid a lot of confusion for former DInput devs.
>>>>
>>>>>
>>>>> However, there is actually a 1:1 mapping between DInput polar
>>>>> coordinates and our direction parameter; DInput polar coordinates have 0
>>>>> deg = up, 90 deg = right, etc, so they are exactly flipped and therefore
>>>>> match our values due to the reverse definition.
>>>>>
>>>>> Looking at your DInputVsLinuxDirections.txt, you seem to have mixed
>>>>> different definitions of Carts: For DInput you use -1 = north, but for
>>>>> Linux +1 = up, while you use -1 = west/left for both.
>>>>
>>>> So I assume you agree that I got the DInput part right? ((0, -1) = north)
>>>
>>> I guess so.
>>>
>>>> Then my mistake lies in the assumption that (0, +1) = up, so I should
>>>> flip the y-axis to correct the Linux part.
>>>> I'll explain how I originally derived the Linux part:
>>>> Michal documented (in "ff-memless-next.txt") the Linux directions in
>>>> the following way:
>>>> "
>>>> Direction of the effect's force translates to Cartesian coordinates system
>>>> as follows:
>>>> Direction = 0: Down
>>>> Direction (0; 16384): 3rd quadrant
>>>> Direction = 16384: Left
>>>> Direction (16385; 32768): 2nd quadrant
>>>> Direction = 32768: Up
>>>> Direction (32769; 49152): 1st quadrant
>>>> Direction = 49152: Right
>>>> Direction (49153; 65535) :4th quadrant
>>>> Direction = 65565: Down
>>>> "
>>>
>>> The above is correct.
>>>
>>>> For a Cartesian coordinates system:
>>>> - The (-1, 0)-axis (=-x) is the intersection of 3rd quadrant and 2nd
>>>> quadrant => Left
>>>> - The (0, +1)-axis (=+y) is the intersection of 2nd quadrant and 1st
>>>> quadrant => Up
>>>> - The (+1, 0)-axis (=+x) is the intersection of 1st quadrant and 4th
>>>> quadrant => Right
>>>> - The (0, -1)-axis (=-y) is the intersection of 4th quadrant and 3rd
>>>> quadrant => Down
>>>
>>> Not sure why you've arbitrarily chosen reverse definition of Y axis
>>> here,
>>
>> I really tried to derive the above in a non-arbitrary manner, here you
>> can verify why I 'chose' the +y axis as the intersection of 2nd
>> quadrant and 1st quadrant:
>> http://en.wikipedia.org/wiki/File:Cartesian_coordinates_2D.svg
>> And because you said that the part about directions in
>> "ff-memless-next.txt" is correct, it follows that Up (which lies
>> between the 2nd quadrant and 1st quadrant) corresponds with +y
>> direction.
>> I hope this makes sense.
>>
>>> when all DInput and Linux joysticks, mouses, etc. have -y as "up".
>>
>> Alright, I did not know that, at least not for Linux. Where in the
>> Linux documentation can I find this?
>
> I don't think it is explicitely said either. I guess one could say it is
> "standard industry practice" (not limited to input, by the way, with
> e.g. in images the pixel row and column values are increasing right and
> down, not right and up).
>
> I don't oppose more explicit documentation, though.
>
>>>
>>>> Michal's approach seems logical to me, if he made a mistake, it's
>>>> caused by the lack of documentation of input.h : it should mention
>>>> what axes (-x, +x, -y or +y) the words (left, right, down and up)
>>>> correspond with.
>>>
>>> Well, the "left", "right", "down", "up" correspond to directions from
>>> the user perspective.
>>
>> That makes sense.
>>
>>> I don't think that is ambigiuous at all, as I
>>> don't see how you could consider "down" to be away from user?
>>
>> Yes, that's true. But it does not say to which axes (and polarity) they map.
>>
>>>
>>> I'm not against more documentation if it helps, though.
>>>
>>>> So, which interpretation is the right one?
>>>
>>> The one where the directions provided in input.h match physical
>>> direction of the force effect from user perspective on a 2-axis joystick
>>> device.
>>
>> That's a nice explanation! For the doc, also add to what axis each
>> direction corresponds to.
>>
>>>
>>>> (I did not find anything in the Linux documentation that states "there
>>>> is actually a 1:1 mapping between DInput polar coordinates and our
>>>> direction parameter")
>>>
>>> It is not stated explicitely, it just naturally follows.
>>>
>>> 1. Both use a clock-wise direction.
>>> 1. DInput direction definition is reversed, as they use the "counteract"
>>> direction. We can flip to correct.
>>> 2. DInput direction has 0 = up/north, we have 0 = down/south,
>>> i.e. the exact opposite, so we must flip the direction.
>>>
>>> DInput direction = flipped(flipped(Linux direction))
>>> = Linux direction
>>
>> Alright, now I understand how it works.
>> But: the thing I wrote about the Cartesian coordinates system (the +y
>> axis lies between 2nd quadrant and 1st quadrant) is mathematically
>> correct (see referenced link to wiki), so, to fix the only
>> contradiction left, we will have to change the explanation about
>> directions in "ff-memless-next.txt" to the following text:
>> "
>> Direction of the effect's force translates to Cartesian coordinates system
>> as follows:
>> Direction = 0: Down: +y
>> Direction (0; 16384): 2nd quadrant
>> Direction = 16384: Left: -x
>> Direction (16385; 32768): 3th quadrant
>> Direction = 32768: Up: -y
>> Direction (32769; 49152): 4rd quadrant
>> Direction = 49152: Right: +x
>> Direction (49153; 65535) :1st quadrant
>> Direction = 65565: Down: +y
>> "
>> As you can see, I only changed the quadrants (and added axes for
>> convenience), now they agree to
>> "http://en.wikipedia.org/wiki/File:Cartesian_coordinates_2D.svg", and
>> to input.h , so everyone should be happy :)
>> Can you confirm the modification is correct?
>
> I think so.
>
> However, assigning quadrants here is a bit confusing to me here with the
> non-mathematical-traditional reversed Y axis. I.e. my first instinct is
> that the upper-right quadrant is the first one, while it is not the case
> here...
I totally understand your confusion about the quadrants: Michal seems
so have meant the 'quadrants' that can be seen in the text about
directions in "input.h", which are different of the classical
mathematical Cartesian coordinate system. I probably should ask Michal
to clearify that in the doc of "ff-memless-next.txt".
>
> But if it helps...
>
>
>>>
>>>>> This causes the
>>>>> 1st and 3rd entries on both of the Mapping tables to be reversed. When
>>>>> that is fixed, the table #2 shows the correct result.
> [...]
>>>>>> 3)
>>>>>> Many Linux FF effect parameters don't have a clear explanation of the
>>>>>> allowed range, and their corresponding meaning at the extrema.
>>>>>> Here I list the ones that need more explanation, also take a look at
>>>>>> "interactive.fig" in the kernel documentation (but also included as
>>>>>> attachment):
>>>>>> - left_saturation and right_saturation; and
>>>>>
>>>>> left_saturation = maximum force on the negative ("left") side of the
>>>>> center point
>>>>> right_saturation = same for positive side
>>>>>
>>>>> 0x0 => no force,
>>>>> 0xFFFF => maximum force.
>>>>
>>>> OK, thanks for giving the definition. I think these things can be
>>>> understood from "interactive.fig", so there's no need to write
>>>> additional doc about this topic.
>>>>
>>>>>
>>>>>> - deadband
>>>>>
>>>>> The range from center point wherein the effect has no effect
>>>>
>>>> Notice this contradicts with "interactive.fig", this figure defines
>>>> 'deadband' as bound-to-bound, not as center-to-bound (as with DInput).
>>>
>>> There is no difference between those definitions that I can see:
>>>
>>> Axis range
>>> |------------------------------------------------------|
>>>
>>> Definition from interactive.fig, bound-to-bound:
>>> 0x0:
>>> |--------------------------X---------------------------|
>>> 0x8000 (covers half of the end-to-end area):
>>> |------------XXXXXXXXXXXXXXXXXXXXXXXXXXXXXX------------|
>>> 0xFFFF:
>>> |XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX|
>>
>> Alright, agreed with this part: deadband units or dimensions are the
>> same as the ones of the center offset parameter.
>>
>>>
>>> Center-to-bound definition:
>>> 0x0:
>>> |--------------------------X---------------------------|
>>> 0x8000 (covers half of the center-to-end area):
>>> |------------XXXXXXXXXXXXXXXXXXXXXXXXXXXXXX------------|
>>> 0xFFFF:
>>> |XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX|
>>
>> Ah, I see, this is where we disagree: now you're not working with the
>> same units or dimensions as the ones of the center offset parameter:
>> you have multiplied them by a factor 2 (that's where my "<< 1" came
>> from, for going from my definition to your definition).
>
> Yes, the center offset and deadband are in "relative" units, so due to
> the different value range the scales differ by a factor of 2.
>
> I agree it is a bit unfortunate the scales do not match, but IMHO not a
> totally hopeless situation.
>
>> This is my proposal of how it could be:
>>
>> Center-to-bound definition (same dimensions as center offset parameter):
>> 0x0:
>> |--------------------------X---------------------------|
>> 0x4000 (covers half of the center-to-end area):
>> |------------XXXXXXXXXXXXXXXXXXXXXXXXXXXXXX------------|
>> 0x8000 (covers whole the center-to-end area):
>> |XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX|
>> 0xC000:
>> |XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX| (clamped)
>> 0xFFFF:
>> |XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX| (clamped)
>
> Yes, this I understood.
>
>>>
>>>
>>>>> , with
>>>>> 0x0 => no dead band
>>>>> 0xFFFF => dead band encompassing the entire axis, effect not active
>>>>> anywhere.
>>>>>
>>>>> Assuming center offset of 0, though. Not sure how the currently
>>>>> supported devices interpret 0xFFFF with non-zero center offset, i.e. if
>>>>> the effect is then still active in the extreme opposite end of the axis.
>>>>> You wrote below that this is indeed the case with DInput, so it is
>>>>> highly likely this is how the devices handle it as well.
>>>>
>>>> Careful, here there's a catch:
>>>> I did not write that in DInput "if deadband == maxValue(deadband)
>>>> with non-zero center offset, then the effect is still active in the
>>>> extreme opposite end of the axis.",
>>>> I wrote that in DInput "if deadband == maxValue(deadband) with
>>>> maximum non-zero center offset, then the effect can only cover half of
>>>> the total region". For reference: maxValue(deadband)=10000
>>>
>>> I don't see the difference in those statements here.
>>
>> There is, when assuming same dimensions as center offset parameter.
>> Otherwise, you're right.
>
> Right.
>
>>> Note that deadband
>>> is the area where the effect is *not* active.
>>
>> I know, that's why it's called *dead*-band ;)
>>
>>>
>>>> Now I'm proposing to let DInput maxValue(deadband) correspond to linux
>>>> deadband 0x7FFF.
>>>> And still allowing linux deadband to be maximal 0xFFFF => two times
>>>> the maximum range of DInput
>>>>
>>>>>
>>>>>
>>>>>> They all have __u16 types, but "/include/uapi/linux/input.h" does not
>>>>>> say what the maximum values are.
>>>>>> I'm doing a proposal to define and document this (in the Linux kernel)
>>>>>> in the following way, also take a look at my attachment
>>>>>> "interactiveAlteredWithRanges.svg" for the modified version:
>>>>>> Max Range of {right_saturation and left_saturation} = 0x7FFF
>>>>>> Because the maximal value of the saturation bound can be only
>>>>>> half of the total range covered by the max negative to max positive
>>>>>> bounds.
>>>>>> And also because they are related to some form of force, and
>>>>>> all other forms of force magnitude in Linux FF have a maximum value of
>>>>>> 0x7FFF
>>>>>
>>>>> I'm not really convinced that the different range from the other
>>>>> magnitude values is a reason enough to change the definition here.
>>>>
>>>> After reviewing this mail, I totally agree with you, sorry for that.
>>>>
>>>>>
>>>>>> Max Range of {deadband} = 0xFFFF
>>>>>> This is a bit harder to explain:
>>>>>> - First, I would suggest to alter the deadband definition in
>>>>>> figure "interactive.fig":
>>>>>> I would define deadband as going from a deadband-bound to
>>>>>> the center (BTW, This is how MSDN defines it: "In other words, the
>>>>>> condition is not active between lOffset minus lDeadBand and lOffset
>>>>>> plus lDeadBand."),
>>>>>> instead of from a deadband-bound to the other deadband-bound.
>>>>>> => Same spec as in DInput.
>>>>>
>>>>> With 0xFFFF being the maximum deadband with center offset 0, it does not
>>>>> matter if deadband is defined as range from center or total width,
>>>>> maximum is 0xFFFF in both cases.
>>>>
>>>> Indeed, because (assume applying 0xFFFF linux deadband with center offset 0):
>>>> a) If DInput maxValue(deadband)=10000 (=half of the total region) maps
>>>> to 0x7FFF in the Linux case (=my proposal) if linux deadband is
>>>> defined as range from center:
>>>> the driver will clamp {0 + 0xFFFF =approx 0 + 2 * 0x7FFF} to
>>>> 0x7FFF for both left as right side of center => Total region is
>>>> covered by the deadband.
>>>> b) If DInput maxValue(deadband)=10000 (=half of the total region) maps
>>>> to 0x7FFF in the Linux case (=my proposal) if linux deadband is
>>>> defined as total width:
>>>> the driver will set {0 + 0xFFFF / 2 =approx 0 + 0x7FFF} for both
>>>> left as right side of center => Total region is covered by the
>>>> deadband.
>>>>
>>>> If we define linux deadband as range from center, like a), the Linux
>>>> FF API can represent more variations of conditional effects than
>>>> DInput can (and also with greater resolution), and thus becomes
>>>> superior in that aspect.
>>>
>>> Can you provide any example effect parameters on your proposed system
>>> that would not be possible on DInput?
>>
>> Sure:
>>
>> Center-to-bound definition (same dimensions as center offset parameter):
>> Assume center offset is set to the maximum non-zero value +32767
>> 0x0:
>> |-----------------------------------------------------X|
>> 0x4000 (covers a quarter of the end-to-end area):
>> |----------------------------------------XXXXXXXXXXXXXX|
>> 0x8000 (covers half of the end-to-end area):
>> |---------------------------XXXXXXXXXXXXXXXXXXXXXXXXXXX|
>> 0xC000 (covers three quarters of the end-to-end area):
>> |-------------XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX|
>> 0xFFFF (covers whole the end-to-end area):
>> |XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX|
>
> I meant a complete set of effect parameters that will cause an effect
> not possible with the DInput definitions, like I provided below. Your
> above example seems to be the same case I covered below in my example,
> in which case you can simply move the center point left a bit and use a
> zero right-side saturation value, without needing a large deadband value.
Indeed, it's always possible to find a set of parameters in DInput to
match the same result as with using any parameters with the deadband
of my proposal.
>
>>>
>>> If I understood correctly, what you are saying is an effect like this
>>> (percentages in parantheses are from left extreme of the axis to right
>>> extreme of the axis):
>>> sat_left = 0xFFFF
>>> sat_right = 0xFFFF
>>> coeff_left = 0x4000
>>> coeff_right = -0x4000
>>> center = 0x6000 (87.5% point)
>>> deadband (your definition) = 0xA000
>>> (62.5% of full area from 87.5% point to either direction, i.e.
>>> 25%..150% => (clamping) 25%..100%)
>>>
>>> So that deadband can reach further left than it would be with DInput
>>> definition (which maxes at 0x7FFF of your definition, which would not
>>> reach the 25% mark at the left side).
>>
>> Exactly.
>>
>>>
>>> However, from what I can see, you can achieve the exact same effect with
>>> these parameters (DInput deadband definition):
>>> sat_left = 0xFFFF
>>> sat_right = 0x0000
>>> coeff_left = 0x4000
>>> coeff_right = 0x0000
>>> center = 0x0000 (50% point)
>>> deadband (DInput/ours) = 0x8000 (50% total area, or 50% of
>>> center-to-end, so it reaches 25%..75%)
>>>
>>> On the left side, the effect is exactly the same as before with same
>>> parameters (sat 0xffff, coeff 0x4000, starts at 25% point).
>>>
>>> On the right side the parameters differ, but the end-result is the same,
>>> there is no effect at all:
>>> - In the first example, the right side is fully in deadband area,
>>> causing the effect to have zero effect.
>>> - In my variant with our definition, the right side has zero saturation,
>>> causing the effect to have zero effect.
>>>
>>>
>>> So the present definitions (and DInput definitions) can achieve the same
>>> effects as your proposed definitions, unless I'm missing something,
>>> making the change unneeded.
>>
>> Agreed.
>>
>> But the benefit could be to make the user application less complex,
>> since otherwise, when a special case like the one you mentioned above
>> (with deadband larger than half of the total region), they would also
>> need to change the saturation values. This is not really a problem
>> when the conditional effect is a 'static' one (i.e. center and
>> deadband do not change over time), but it might come in handy for
>> 'dynamic' conditional effects.
>
> True. Though it will likely not outweigh the benefit of using the same
> definition as DInput.
>
>
>> Although honestly I think that situation is extremely rare, so I'm OK
>> with it if we leave the current definition as is.
>> And now I realize that for devices that require the deadband paramater
>> to be passed explicitly (and use the present definition) like SWFF2,
>> this either would not work, or would require additional (and maybe
>> complex) kernel driver code, which is not what we want.
>>
>> So, I agree to leave its definition unchanged, sorry for the discussion ;)
>> I just have to be sure about this to improve Wine's DInput translation layer.
>
> No problem with the discussion, better too much discussion than not
> enough :)
>
> As you've seen, the effect model should very closely match the DInput
> model (as that is what the devices tend to support), mostly just the
> units are different (our units use the 16-bit range as a whole, i.e.
> there are no out-of-range values).
>
> If you have any specific suggestions (patches preferred, otherwise this
> ends up further from the head of my TODO list :) ) to improve docs,
> those would be welcome.
Alright, when I find enough spare time, I will bundle all additional
documentation and propose it as a patch.
>
>
>>>> We will have to do the clamping anyway for devices that only accept
>>>> left and right deadband bounds, i.e. Logitech wheels.
>>>> For devices that only accept a single deadband value, like your SWFF2
>>>> as you mentioned below, the linux deadband would only need to be
>>>> shifted "<< 1" before being send to the device, if they accept u16 and
>>>> work exactly like DInput (range from center, and
>>>> maxValue(deadband)=half of total region).
>>>
>>> I'm not sure what you are trying to accomplish with "<< 1" here. If the
>>> device can't accept the deadband you want, left-shifting wouldn't fix that.
>>
>> See above about the factor 2.
>>
>>>
>>>>>
>>>>>> - Now, knowing that ff_condition_effect->center is __s16:
>>>>>> The worst case scenario is that "center = -32768" or
>>>>>> "center = +32767" while still wanting to cover the whole region (this
>>>>>> is actually not possible with DInput's specs: in that case, they can
>>>>>> only cover half of the total region):
>>>>>> Then, to keep the scale of "center" and "deadband" the
>>>>>> same, "deadband = 65535" = +32767 - -32768 = 0xFFFF
>>>>>
>>>>> Interesting idea. However, if this is not possible in DInput, this means
>>>>> the devices will likely not support it either, since they are using the
>>>>> DInput effect model (as are we).
>>>>
>>>> This is no problem, use "<< 1" as mentioned above.
>>>>
>>>>>
>>>>>
>>>>> I tried to confirm this with my SWFF2 device, but either it has stopped
>>>>> working properly or, more likely, there is a regression in the kernel...
>>>>> no time to debug now, though, so added to my TODO (HID_REQ_GET_REPORT
>>>>> requests don't seem to go through properly).
>>>>>
>>>>>
>>>>>> I expect we will have to add/document the answers to my questions in
>>>>>> the appropriate file "/include/uapi/linux/input.h" or
>>>>>> "/Documentation/input/ff.txt", so that other userspace developers (and
>>>>>> maybe also kernel devs) don't face the same ambiguities.
>>>>>>
>>>>>>
>>>>>> Thank you very much for your time,
>>>>>
>>>>> Thanks for looking into this.
>>>>>
>>>>>> Elias
>>>>>>
>>>>>
>>>>>
>>>>> --
>>>>> Anssi Hannula
>>>>
>>>> Elias
>>>>
>>>
>>>
>>> --
>>> Anssi Hannula
>>
>> Elias
>>
>
>
> --
> Anssi Hannula
Elias
^ permalink raw reply
* Re: [PATCH 03/14] HID: core: implement generic .request()
From: David Herrmann @ 2014-02-16 16:43 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Benjamin Tissoires, Jiri Kosina, open list:HID CORE LAYER,
linux-kernel
In-Reply-To: <CAN+gG=EcXQLivrUPXCk8imrY0T2n3Twc9yujxPGmi=LeoOrsCg@mail.gmail.com>
Hi
On Thu, Feb 13, 2014 at 4:21 PM, Benjamin Tissoires
<benjamin.tissoires@gmail.com> wrote:
> On Wed, Feb 12, 2014 at 5:25 AM, David Herrmann <dh.herrmann@gmail.com> wrote:
>> Hi
>>
>> On Mon, Feb 10, 2014 at 6:58 PM, Benjamin Tissoires
>> <benjamin.tissoires@redhat.com> wrote:
>>> .request() can be emulated through .raw_request()
>>> we can implement this emulation in hid-core, and make .request
>>> not mandatory for transport layer drivers.
>>>
>>> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
>>> ---
>>> drivers/hid/hid-core.c | 45 ++++++++++++++++++++++++++++++++++++++++++++-
>>> include/linux/hid.h | 5 ++++-
>>> 2 files changed, 48 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
>>> index 18fe49b..f36778a 100644
>>> --- a/drivers/hid/hid-core.c
>>> +++ b/drivers/hid/hid-core.c
>>> @@ -1248,6 +1248,11 @@ void hid_output_report(struct hid_report *report, __u8 *data)
>>> }
>>> EXPORT_SYMBOL_GPL(hid_output_report);
>>>
>>> +static int hid_report_len(struct hid_report *report)
>>> +{
>>> + return ((report->size - 1) >> 3) + 1 + (report->id > 0) + 7;
>>
>> Just for clarity, this is equivalent to the following, right?
>>
>> return DIV_ROUND_UP(report->size, 8) + !!(report->id > 0) + 7;
>
> yes, it should (at least that's what I understand too :)
>
>>
>> I always have to read that shifting code twice to get it.. Maybe add a
>> comment explaining the different entries here.
>
> good idea.
>
>>
>>> +}
>>> +
>>> /*
>>> * Allocator for buffer that is going to be passed to hid_output_report()
>>> */
>>> @@ -1258,7 +1263,7 @@ u8 *hid_alloc_report_buf(struct hid_report *report, gfp_t flags)
>>> * of implement() working on 8 byte chunks
>>> */
>>>
>>> - int len = ((report->size - 1) >> 3) + 1 + (report->id > 0) + 7;
>>> + int len = hid_report_len(report);
>>>
>>> return kmalloc(len, flags);
>>> }
>>> @@ -1314,6 +1319,44 @@ static struct hid_report *hid_get_report(struct hid_report_enum *report_enum,
>>> return report;
>>> }
>>>
>>> +/*
>>> + * Implement a generic .request() callback, using .raw_request()
>>> + * DO NOT USE in hid drivers directly, but through hid_hw_request instead.
>>> + */
>>> +void __hid_request(struct hid_device *hid, struct hid_report *report,
>>> + int reqtype)
>>> +{
>>> + char *buf;
>>> + int ret;
>>> + int len;
>>> +
>>> + if (!hid->ll_driver->raw_request)
>>> + return;
>>> +
>>> + buf = hid_alloc_report_buf(report, GFP_KERNEL);
>>> + if (!buf)
>>> + return;
>>> +
>>> + len = hid_report_len(report);
>
> actually, after sending the patches, I was wondering if we should use
> the +7 in hid_report_len.
> "len" is used in .raw_request(), and the +7 was only for the implement(), right?
>
> So maybe a device can reject this because the size of the report is too big...
>
> Jiri, David, any ideas?
Yeah, we should allocate the +7 size, but we shouldn't use it as
"length" argument. We should just silently guarantee the buffer is big
enough.
Thanks
David
> Cheers,
> Benjamin
>
>>> +
>>> + if (reqtype == HID_REQ_SET_REPORT)
>>> + hid_output_report(report, buf);
>>> +
>>> + ret = hid->ll_driver->raw_request(hid, report->id, buf, len,
>>> + report->type, reqtype);
>>> + if (ret < 0) {
>>> + dbg_hid("unable to complete request: %d\n", ret);
>>> + goto out;
>>> + }
>>> +
>>> + if (reqtype == HID_REQ_GET_REPORT)
>>> + hid_input_report(hid, report->type, buf, ret, 0);
>>> +
>>> +out:
>>> + kfree(buf);
>>> +}
>>> +EXPORT_SYMBOL_GPL(__hid_request);
>>> +
>>
>> Looks good to me.
>>
>> Reviewed-by: David Herrmann <dh.herrmann@gmail.com>
>>
>> Thanks
>> David
>>
>>> int hid_report_raw_event(struct hid_device *hid, int type, u8 *data, int size,
>>> int interrupt)
>>> {
>>> diff --git a/include/linux/hid.h b/include/linux/hid.h
>>> index a837ede..09fbbd7 100644
>>> --- a/include/linux/hid.h
>>> +++ b/include/linux/hid.h
>>> @@ -753,6 +753,7 @@ struct hid_field *hidinput_get_led_field(struct hid_device *hid);
>>> unsigned int hidinput_count_leds(struct hid_device *hid);
>>> __s32 hidinput_calc_abs_res(const struct hid_field *field, __u16 code);
>>> void hid_output_report(struct hid_report *report, __u8 *data);
>>> +void __hid_request(struct hid_device *hid, struct hid_report *rep, int reqtype);
>>> u8 *hid_alloc_report_buf(struct hid_report *report, gfp_t flags);
>>> struct hid_device *hid_allocate_device(void);
>>> struct hid_report *hid_register_report(struct hid_device *device, unsigned type, unsigned id);
>>> @@ -965,7 +966,9 @@ static inline void hid_hw_request(struct hid_device *hdev,
>>> struct hid_report *report, int reqtype)
>>> {
>>> if (hdev->ll_driver->request)
>>> - hdev->ll_driver->request(hdev, report, reqtype);
>>> + return hdev->ll_driver->request(hdev, report, reqtype);
>>> +
>>> + __hid_request(hdev, report, reqtype);
>>> }
>>>
>>> /**
>>> --
>>> 1.8.3.1
>>>
^ permalink raw reply
* Re: [PATCH v2 7/9] HID: remove hid_get_raw_report in struct hid_device
From: Jiri Kosina @ 2014-02-17 13:15 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Benjamin Tissoires, David Herrmann, Frank Praznik, linux-input,
linux-kernel
In-Reply-To: <1391636004-29354-8-git-send-email-benjamin.tissoires@redhat.com>
On Wed, 5 Feb 2014, Benjamin Tissoires wrote:
> dev->hid_get_raw_report(X) and hid_hw_raw_request(X, HID_REQ_GET_REPORT)
> are strictly equivalent. Switch the hid subsystem to the hid_hw notation
> and remove the field .hid_get_raw_report in struct hid_device.
>
> Reviewed-by: David Herrmann <dh.herrmann@gmail.com>
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> ---
> drivers/hid/hid-input.c | 6 +++---
> drivers/hid/hid-sony.c | 3 ++-
> drivers/hid/hidraw.c | 7 ++++---
> drivers/hid/i2c-hid/i2c-hid.c | 1 -
> drivers/hid/uhid.c | 1 -
> drivers/hid/usbhid/hid-core.c | 1 -
> include/linux/hid.h | 3 ---
> net/bluetooth/hidp/core.c | 1 -
> 8 files changed, 9 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
> index 68db2db..6253e95 100644
> --- a/drivers/hid/hid-input.c
> +++ b/drivers/hid/hid-input.c
> @@ -350,9 +350,9 @@ static int hidinput_get_battery_property(struct power_supply *psy,
> ret = -ENOMEM;
> break;
> }
> - ret = dev->hid_get_raw_report(dev, dev->battery_report_id,
> - buf, 2,
> - dev->battery_report_type);
> + ret = hid_hw_raw_request(dev, dev->battery_report_id, buf, 2,
> + dev->battery_report_type,
> + HID_REQ_GET_REPORT);
>
> if (ret != 2) {
> ret = -ENODATA;
> diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
> index 2bd3f13..b51db79 100644
> --- a/drivers/hid/hid-sony.c
> +++ b/drivers/hid/hid-sony.c
> @@ -810,7 +810,8 @@ static int sixaxis_set_operational_usb(struct hid_device *hdev)
> if (!buf)
> return -ENOMEM;
>
> - ret = hdev->hid_get_raw_report(hdev, 0xf2, buf, 17, HID_FEATURE_REPORT);
> + ret = hid_hw_raw_request(hdev, 0xf2, buf, 17, HID_FEATURE_REPORT,
> + HID_REQ_GET_REPORT);
>
> if (ret < 0)
> hid_err(hdev, "can't set operational mode\n");
> diff --git a/drivers/hid/hidraw.c b/drivers/hid/hidraw.c
> index cb0137b..4b2dc95 100644
> --- a/drivers/hid/hidraw.c
> +++ b/drivers/hid/hidraw.c
> @@ -189,7 +189,7 @@ static ssize_t hidraw_get_report(struct file *file, char __user *buffer, size_t
>
> dev = hidraw_table[minor]->hid;
>
> - if (!dev->hid_get_raw_report) {
> + if (!dev->ll_driver->raw_request) {
> ret = -ENODEV;
> goto out;
> }
> @@ -216,14 +216,15 @@ static ssize_t hidraw_get_report(struct file *file, char __user *buffer, size_t
>
> /*
> * Read the first byte from the user. This is the report number,
> - * which is passed to dev->hid_get_raw_report().
> + * which is passed to hid_hw_raw_request().
> */
> if (copy_from_user(&report_number, buffer, 1)) {
> ret = -EFAULT;
> goto out_free;
> }
>
> - ret = dev->hid_get_raw_report(dev, report_number, buf, count, report_type);
> + ret = hid_hw_raw_request(dev, report_number, buf, count, report_type,
> + HID_REQ_GET_REPORT);
>
> if (ret < 0)
> goto out_free;
> diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
> index 923ff81..f57de7f 100644
> --- a/drivers/hid/i2c-hid/i2c-hid.c
> +++ b/drivers/hid/i2c-hid/i2c-hid.c
> @@ -1003,7 +1003,6 @@ static int i2c_hid_probe(struct i2c_client *client,
>
> hid->driver_data = client;
> hid->ll_driver = &i2c_hid_ll_driver;
> - hid->hid_get_raw_report = i2c_hid_get_raw_report;
> hid->hid_output_raw_report = i2c_hid_output_raw_report;
> hid->dev.parent = &client->dev;
> ACPI_COMPANION_SET(&hid->dev, ACPI_COMPANION(&client->dev));
> diff --git a/drivers/hid/uhid.c b/drivers/hid/uhid.c
> index f5a2b19..12439e1 100644
> --- a/drivers/hid/uhid.c
> +++ b/drivers/hid/uhid.c
> @@ -404,7 +404,6 @@ static int uhid_dev_create(struct uhid_device *uhid,
> hid->uniq[63] = 0;
>
> hid->ll_driver = &uhid_hid_driver;
> - hid->hid_get_raw_report = uhid_hid_get_raw;
I'll queue a followup patch on top of this that completely removes
uhid_hid_get_raw(), as it's unused now.
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH v3 0/6] HID: sony: Add full Bluetooth support for the Dualshock 4
From: Jiri Kosina @ 2014-02-17 13:18 UTC (permalink / raw)
To: David Herrmann; +Cc: Frank Praznik, open list:HID CORE LAYER
In-Reply-To: <CANq1E4TNzAuhUsTi_QA-NaqH2MbzS3awuJu0Tx0QyeL-MEBrmA@mail.gmail.com>
On Wed, 12 Feb 2014, David Herrmann wrote:
> > v3 of this patch series rebases the code against Benjamin Tissoires' HID
> > transport layer changes to use the safe inline hid_hw_* functions which
> > eliminates the need to check the function pointers manually.
> >
> > It adds an explicit Bluetooth initialization function instead of the controller
> > input report state being set as a side effect of initializing the LED system.
> >
> > It also uses a new DUALSHOCK4_CONTROLLER macro to simplify conditional cases
> > where the connection type is irrelevant.
>
> The series looks good to me.
>
> Reviewed-by: David Herrmann <dh.herrmann@gmail.com>
I finally made it to reviewing this lot. Now applied, thanks everybody.
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH v2 7/9] HID: remove hid_get_raw_report in struct hid_device
From: Benjamin Tissoires @ 2014-02-17 13:19 UTC (permalink / raw)
To: Jiri Kosina
Cc: Benjamin Tissoires, David Herrmann, Frank Praznik, linux-input,
linux-kernel@vger.kernel.org
In-Reply-To: <alpine.LNX.2.00.1402171414490.1192@pobox.suse.cz>
On Mon, Feb 17, 2014 at 8:15 AM, Jiri Kosina <jkosina@suse.cz> wrote:
> On Wed, 5 Feb 2014, Benjamin Tissoires wrote:
>
>> dev->hid_get_raw_report(X) and hid_hw_raw_request(X, HID_REQ_GET_REPORT)
>> are strictly equivalent. Switch the hid subsystem to the hid_hw notation
>> and remove the field .hid_get_raw_report in struct hid_device.
>>
>> Reviewed-by: David Herrmann <dh.herrmann@gmail.com>
>> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
>> ---
>> drivers/hid/hid-input.c | 6 +++---
>> drivers/hid/hid-sony.c | 3 ++-
>> drivers/hid/hidraw.c | 7 ++++---
>> drivers/hid/i2c-hid/i2c-hid.c | 1 -
>> drivers/hid/uhid.c | 1 -
>> drivers/hid/usbhid/hid-core.c | 1 -
>> include/linux/hid.h | 3 ---
>> net/bluetooth/hidp/core.c | 1 -
>> 8 files changed, 9 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
>> index 68db2db..6253e95 100644
>> --- a/drivers/hid/hid-input.c
>> +++ b/drivers/hid/hid-input.c
>> @@ -350,9 +350,9 @@ static int hidinput_get_battery_property(struct power_supply *psy,
>> ret = -ENOMEM;
>> break;
>> }
>> - ret = dev->hid_get_raw_report(dev, dev->battery_report_id,
>> - buf, 2,
>> - dev->battery_report_type);
>> + ret = hid_hw_raw_request(dev, dev->battery_report_id, buf, 2,
>> + dev->battery_report_type,
>> + HID_REQ_GET_REPORT);
>>
>> if (ret != 2) {
>> ret = -ENODATA;
>> diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
>> index 2bd3f13..b51db79 100644
>> --- a/drivers/hid/hid-sony.c
>> +++ b/drivers/hid/hid-sony.c
>> @@ -810,7 +810,8 @@ static int sixaxis_set_operational_usb(struct hid_device *hdev)
>> if (!buf)
>> return -ENOMEM;
>>
>> - ret = hdev->hid_get_raw_report(hdev, 0xf2, buf, 17, HID_FEATURE_REPORT);
>> + ret = hid_hw_raw_request(hdev, 0xf2, buf, 17, HID_FEATURE_REPORT,
>> + HID_REQ_GET_REPORT);
>>
>> if (ret < 0)
>> hid_err(hdev, "can't set operational mode\n");
>> diff --git a/drivers/hid/hidraw.c b/drivers/hid/hidraw.c
>> index cb0137b..4b2dc95 100644
>> --- a/drivers/hid/hidraw.c
>> +++ b/drivers/hid/hidraw.c
>> @@ -189,7 +189,7 @@ static ssize_t hidraw_get_report(struct file *file, char __user *buffer, size_t
>>
>> dev = hidraw_table[minor]->hid;
>>
>> - if (!dev->hid_get_raw_report) {
>> + if (!dev->ll_driver->raw_request) {
>> ret = -ENODEV;
>> goto out;
>> }
>> @@ -216,14 +216,15 @@ static ssize_t hidraw_get_report(struct file *file, char __user *buffer, size_t
>>
>> /*
>> * Read the first byte from the user. This is the report number,
>> - * which is passed to dev->hid_get_raw_report().
>> + * which is passed to hid_hw_raw_request().
>> */
>> if (copy_from_user(&report_number, buffer, 1)) {
>> ret = -EFAULT;
>> goto out_free;
>> }
>>
>> - ret = dev->hid_get_raw_report(dev, report_number, buf, count, report_type);
>> + ret = hid_hw_raw_request(dev, report_number, buf, count, report_type,
>> + HID_REQ_GET_REPORT);
>>
>> if (ret < 0)
>> goto out_free;
>> diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
>> index 923ff81..f57de7f 100644
>> --- a/drivers/hid/i2c-hid/i2c-hid.c
>> +++ b/drivers/hid/i2c-hid/i2c-hid.c
>> @@ -1003,7 +1003,6 @@ static int i2c_hid_probe(struct i2c_client *client,
>>
>> hid->driver_data = client;
>> hid->ll_driver = &i2c_hid_ll_driver;
>> - hid->hid_get_raw_report = i2c_hid_get_raw_report;
>> hid->hid_output_raw_report = i2c_hid_output_raw_report;
>> hid->dev.parent = &client->dev;
>> ACPI_COMPANION_SET(&hid->dev, ACPI_COMPANION(&client->dev));
>> diff --git a/drivers/hid/uhid.c b/drivers/hid/uhid.c
>> index f5a2b19..12439e1 100644
>> --- a/drivers/hid/uhid.c
>> +++ b/drivers/hid/uhid.c
>> @@ -404,7 +404,6 @@ static int uhid_dev_create(struct uhid_device *uhid,
>> hid->uniq[63] = 0;
>>
>> hid->ll_driver = &uhid_hid_driver;
>> - hid->hid_get_raw_report = uhid_hid_get_raw;
>
> I'll queue a followup patch on top of this that completely removes
> uhid_hid_get_raw(), as it's unused now.
Jiri, please don't. It was a mistake which was fixed in the next patch
series (2/14: HID: uHID: implement .raw_request). It was already
reviewed by David, so you can apply it right now.
Cheers,
Benjamin
>
> --
> Jiri Kosina
> SUSE Labs
^ permalink raw reply
* Re: [PATCH v2 0/9] HID: spring cleanup v2
From: Jiri Kosina @ 2014-02-17 13:19 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Benjamin Tissoires, David Herrmann, Frank Praznik, linux-input,
linux-kernel
In-Reply-To: <1391636004-29354-1-git-send-email-benjamin.tissoires@redhat.com>
On Wed, 5 Feb 2014, Benjamin Tissoires wrote:
> Hi guys,
>
> well, here comes the promised v2 of the ll_transport cleanup.
>
> As I said, I removed patches which need some more work, and kept only the
> trivial ones. I also added David's documentation, which gives us a net
> difference of +210 lines of code :(
> Let's say that we still have a net worth of -106 lines of actual code :)
>
> Cheers,
> Benjamin
>
> Changes since v1:
> - removed uhid, i2c-hid patches
> - removed the previous 11/11 (move hid_output_raw_report to hid_ll_driver)
> - hid-logitech-dj: use hid_hw_raw_request instead of hid_output_report (2/9)
> - add documentation - I removed the hid_input_event() doc (9/9)
Good job, thanks! Now applied.
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH v2 7/9] HID: remove hid_get_raw_report in struct hid_device
From: Jiri Kosina @ 2014-02-17 13:20 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Benjamin Tissoires, David Herrmann, Frank Praznik, linux-input,
linux-kernel@vger.kernel.org
In-Reply-To: <CAN+gG=E__6ejV_BqdtM8iwyM-UCdB1G49nvEZn3WTeELixqs4Q@mail.gmail.com>
On Mon, 17 Feb 2014, Benjamin Tissoires wrote:
> >> diff --git a/drivers/hid/uhid.c b/drivers/hid/uhid.c
> >> index f5a2b19..12439e1 100644
> >> --- a/drivers/hid/uhid.c
> >> +++ b/drivers/hid/uhid.c
> >> @@ -404,7 +404,6 @@ static int uhid_dev_create(struct uhid_device *uhid,
> >> hid->uniq[63] = 0;
> >>
> >> hid->ll_driver = &uhid_hid_driver;
> >> - hid->hid_get_raw_report = uhid_hid_get_raw;
> >
> > I'll queue a followup patch on top of this that completely removes
> > uhid_hid_get_raw(), as it's unused now.
>
> Jiri, please don't. It was a mistake which was fixed in the next patch
> series (2/14: HID: uHID: implement .raw_request). It was already
> reviewed by David, so you can apply it right now.
Yeah, I just came to the same conclusion during review as we speak. I will
sort it out.
Thanks.
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* USB keyboard occasional key stuck
From: Daniel J Blueman @ 2014-02-17 13:23 UTC (permalink / raw)
To: linux-usb-u79uwXL29TY76Z2rM5mHXA,
linux-input-u79uwXL29TY76Z2rM5mHXA, Linux Kernel
Cc: Dmitry Torokhov
Across 5+ years of kernels, I've been seeing occasional (1-2 times per
day) key-stuck issues where eg a fn+delete combo repeats delete until
I press delete again. I've seen this happen with fn+ctrl+left, leaving
left held and likewise with right.
This has occurred on Apple laptops, external USB keyboards and Dell
laptops, so seems like a linux USB input issue, as I haven't seen
occur on Windows or MacOS on the same hardware.
It seems a good move for me to rebuild and run a kernel with some USB
HID instrumentation to locate this issue over time. Without apriori
knowledge of the linux USB input stack, what is a good initial
approach?
Thanks,
Daniel
--
Daniel J Blueman
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox