Linux Input/HID development
 help / color / mirror / Atom feed
* Re: [Xen-devel] [PATCH] Input: xen-kbdfront - allow better run-time configuration
From: Jason Andryuk @ 2018-04-19 13:10 UTC (permalink / raw)
  To: Oleksandr Andrushchenko
  Cc: Juergen Gross, xen-devel, linux-input, open list, dmitry.torokhov,
	lyan, Boris Ostrovsky, andrii_chepurnyi, Oleksandr Andrushchenko
In-Reply-To: <d935e7d9-090f-a8b1-7f12-5e0a7f0adf1b@gmail.com>

On Thu, Apr 19, 2018 at 9:01 AM, Oleksandr Andrushchenko
<andr2000@gmail.com> wrote:
>
> Ok, so I'll send v2 with the following changes:
>
> diff --git a/drivers/input/misc/xen-kbdfront.c
> b/drivers/input/misc/xen-kbdfront.c
> index a3306aad40b0..d8cca212f737 100644
> --- a/drivers/input/misc/xen-kbdfront.c
> +++ b/drivers/input/misc/xen-kbdfront.c
> @@ -51,13 +51,13 @@ module_param_array(ptr_size, int, NULL, 0444);
>  MODULE_PARM_DESC(ptr_size,
>         "Pointing device width, height in pixels (default 800,600)");
>
> -static unsigned int no_ptr_dev;
> -module_param(no_ptr_dev, uint, 0);
> +static bool no_ptr_dev;
> +module_param(no_ptr_dev, bool, 0);
>  MODULE_PARM_DESC(no_ptr_dev,
>         "If set then no virtual pointing device exposed to the guest");
>
> -static unsigned int no_kbd_dev;
> -module_param(no_kbd_dev, uint, 0);
> +static bool no_kbd_dev;
> +module_param(no_kbd_dev, bool, 0);
>  MODULE_PARM_DESC(no_kbd_dev,
>         "If set then no virtual keyboard device exposed to the guest");

I prefer direct logic over inverse logic.  Maybe just use kbd_dev,
default to true, but allow it to be set off?

static bool kbd_dev = true;
module_param(kbd_dev, bool, 0);

Regards,
Jason

^ permalink raw reply

* Re: [PATCH] Input: xen-kbdfront - allow better run-time configuration
From: Oleksandr Andrushchenko @ 2018-04-19 13:01 UTC (permalink / raw)
  To: Juergen Gross, xen-devel, linux-input, linux-kernel,
	dmitry.torokhov, lyan, boris.ostrovsky
  Cc: andrii_chepurnyi, Oleksandr Andrushchenko
In-Reply-To: <c833ab0d-746c-c734-3dbe-e02ae7951d1d@suse.com>

On 04/19/2018 03:52 PM, Juergen Gross wrote:
> On 19/04/18 13:44, Oleksandr Andrushchenko wrote:
>> On 04/19/2018 02:25 PM, Juergen Gross wrote:
>>> On 18/04/18 17:04, Oleksandr Andrushchenko wrote:
>>>> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>>>>
>>>> It is now only possible to control if multi-touch virtual device
>>>> is created or not (via the corresponding XenStore entries),
>>>> but keyboard and pointer devices are always created.
>>> Why don't you want to go that route for keyboard and mouse, too?
>>> Or does this really make no sense?
>> Well, I would prefer not to touch anything outside Linux and
>> this driver. And these settings seem to be implementation specific.
>> So, this is why introduce Linux module parameters and don't extend
>> the kbdif protocol.
>>>> In some cases this is not desirable. For example, if virtual
>>>> keyboard device is exposed to Android then the latter won't
>>>> automatically show on-screen keyboard as it expects that a
>>>> physical keyboard device can be used for typing.
>>>>
>>>> Make it possible to configure which virtual devices are created
>>>> with module parameters:
>>>>     - no_ptr_dev=1 if no pointer device needs to be created
>>>>     - no_kbd_dev=1 if no keyboard device needs to be created
>>>> Keep old behavior by default.
>>>>
>>>> Signed-off-by: Oleksandr Andrushchenko
>>>> <oleksandr_andrushchenko@epam.com>
>>>> Suggested-by: Andrii Chepurnyi <andrii_chepurnyi@epam.com>
>>>> Tested-by: Andrii Chepurnyi <andrii_chepurnyi@epam.com>
>>>> ---
>>>>    drivers/input/misc/xen-kbdfront.c | 159 +++++++++++++++++-------------
>>>>    1 file changed, 92 insertions(+), 67 deletions(-)
>>>>
>>>> diff --git a/drivers/input/misc/xen-kbdfront.c
>>>> b/drivers/input/misc/xen-kbdfront.c
>>>> index d91f3b1c5375..a3306aad40b0 100644
>>>> --- a/drivers/input/misc/xen-kbdfront.c
>>>> +++ b/drivers/input/misc/xen-kbdfront.c
>>>> @@ -51,6 +51,16 @@ module_param_array(ptr_size, int, NULL, 0444);
>>>>    MODULE_PARM_DESC(ptr_size,
>>>>        "Pointing device width, height in pixels (default 800,600)");
>>>>    +static unsigned int no_ptr_dev;
>>>> +module_param(no_ptr_dev, uint, 0);
>>> Use type invbool instead?
>> Hm, better bool then? invbool will require parameter name change to
>> something like "with_ptr_dev" which might confuse, e.g.
>> default was to go with pointer device, now we have with_ptr_dev
>> module parameter: do I now need to set it to preserve the old behavior?
>> The answer is no (because of invbool), but you have to dig for it.
>>
>> Will bool work for you?
> As long as the default won't change from today: yes.
Ok, so I'll send v2 with the following changes:

diff --git a/drivers/input/misc/xen-kbdfront.c 
b/drivers/input/misc/xen-kbdfront.c
index a3306aad40b0..d8cca212f737 100644
--- a/drivers/input/misc/xen-kbdfront.c
+++ b/drivers/input/misc/xen-kbdfront.c
@@ -51,13 +51,13 @@ module_param_array(ptr_size, int, NULL, 0444);
  MODULE_PARM_DESC(ptr_size,
         "Pointing device width, height in pixels (default 800,600)");

-static unsigned int no_ptr_dev;
-module_param(no_ptr_dev, uint, 0);
+static bool no_ptr_dev;
+module_param(no_ptr_dev, bool, 0);
  MODULE_PARM_DESC(no_ptr_dev,
         "If set then no virtual pointing device exposed to the guest");

-static unsigned int no_kbd_dev;
-module_param(no_kbd_dev, uint, 0);
+static bool no_kbd_dev;
+module_param(no_kbd_dev, bool, 0);
  MODULE_PARM_DESC(no_kbd_dev,
         "If set then no virtual keyboard device exposed to the guest");


>
> Juergen

^ permalink raw reply related

* Re: [PATCH] Input: xen-kbdfront - allow better run-time configuration
From: Juergen Gross @ 2018-04-19 12:52 UTC (permalink / raw)
  To: Oleksandr Andrushchenko, xen-devel, linux-input, linux-kernel,
	dmitry.torokhov, lyan, boris.ostrovsky
  Cc: andrii_chepurnyi, Oleksandr Andrushchenko
In-Reply-To: <2bff035e-303e-d644-5f51-5e64150c097c@gmail.com>

On 19/04/18 13:44, Oleksandr Andrushchenko wrote:
> On 04/19/2018 02:25 PM, Juergen Gross wrote:
>> On 18/04/18 17:04, Oleksandr Andrushchenko wrote:
>>> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>>>
>>> It is now only possible to control if multi-touch virtual device
>>> is created or not (via the corresponding XenStore entries),
>>> but keyboard and pointer devices are always created.
>> Why don't you want to go that route for keyboard and mouse, too?
>> Or does this really make no sense?
> Well, I would prefer not to touch anything outside Linux and
> this driver. And these settings seem to be implementation specific.
> So, this is why introduce Linux module parameters and don't extend
> the kbdif protocol.
>>> In some cases this is not desirable. For example, if virtual
>>> keyboard device is exposed to Android then the latter won't
>>> automatically show on-screen keyboard as it expects that a
>>> physical keyboard device can be used for typing.
>>>
>>> Make it possible to configure which virtual devices are created
>>> with module parameters:
>>>    - no_ptr_dev=1 if no pointer device needs to be created
>>>    - no_kbd_dev=1 if no keyboard device needs to be created
>>> Keep old behavior by default.
>>>
>>> Signed-off-by: Oleksandr Andrushchenko
>>> <oleksandr_andrushchenko@epam.com>
>>> Suggested-by: Andrii Chepurnyi <andrii_chepurnyi@epam.com>
>>> Tested-by: Andrii Chepurnyi <andrii_chepurnyi@epam.com>
>>> ---
>>>   drivers/input/misc/xen-kbdfront.c | 159 +++++++++++++++++-------------
>>>   1 file changed, 92 insertions(+), 67 deletions(-)
>>>
>>> diff --git a/drivers/input/misc/xen-kbdfront.c
>>> b/drivers/input/misc/xen-kbdfront.c
>>> index d91f3b1c5375..a3306aad40b0 100644
>>> --- a/drivers/input/misc/xen-kbdfront.c
>>> +++ b/drivers/input/misc/xen-kbdfront.c
>>> @@ -51,6 +51,16 @@ module_param_array(ptr_size, int, NULL, 0444);
>>>   MODULE_PARM_DESC(ptr_size,
>>>       "Pointing device width, height in pixels (default 800,600)");
>>>   +static unsigned int no_ptr_dev;
>>> +module_param(no_ptr_dev, uint, 0);
>> Use type invbool instead?
> Hm, better bool then? invbool will require parameter name change to
> something like "with_ptr_dev" which might confuse, e.g.
> default was to go with pointer device, now we have with_ptr_dev
> module parameter: do I now need to set it to preserve the old behavior?
> The answer is no (because of invbool), but you have to dig for it.
> 
> Will bool work for you?

As long as the default won't change from today: yes.


Juergen

^ permalink raw reply

* Re: [PATCH] Input: xen-kbdfront - allow better run-time configuration
From: Oleksandr Andrushchenko @ 2018-04-19 11:44 UTC (permalink / raw)
  To: Juergen Gross, xen-devel, linux-input, linux-kernel,
	dmitry.torokhov, lyan, boris.ostrovsky
  Cc: andrii_chepurnyi, Oleksandr Andrushchenko
In-Reply-To: <ccb7a33b-7862-9803-fb24-67861d5ecab9@suse.com>

On 04/19/2018 02:25 PM, Juergen Gross wrote:
> On 18/04/18 17:04, Oleksandr Andrushchenko wrote:
>> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>>
>> It is now only possible to control if multi-touch virtual device
>> is created or not (via the corresponding XenStore entries),
>> but keyboard and pointer devices are always created.
> Why don't you want to go that route for keyboard and mouse, too?
> Or does this really make no sense?
Well, I would prefer not to touch anything outside Linux and
this driver. And these settings seem to be implementation specific.
So, this is why introduce Linux module parameters and don't extend
the kbdif protocol.
>> In some cases this is not desirable. For example, if virtual
>> keyboard device is exposed to Android then the latter won't
>> automatically show on-screen keyboard as it expects that a
>> physical keyboard device can be used for typing.
>>
>> Make it possible to configure which virtual devices are created
>> with module parameters:
>>    - no_ptr_dev=1 if no pointer device needs to be created
>>    - no_kbd_dev=1 if no keyboard device needs to be created
>> Keep old behavior by default.
>>
>> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>> Suggested-by: Andrii Chepurnyi <andrii_chepurnyi@epam.com>
>> Tested-by: Andrii Chepurnyi <andrii_chepurnyi@epam.com>
>> ---
>>   drivers/input/misc/xen-kbdfront.c | 159 +++++++++++++++++-------------
>>   1 file changed, 92 insertions(+), 67 deletions(-)
>>
>> diff --git a/drivers/input/misc/xen-kbdfront.c b/drivers/input/misc/xen-kbdfront.c
>> index d91f3b1c5375..a3306aad40b0 100644
>> --- a/drivers/input/misc/xen-kbdfront.c
>> +++ b/drivers/input/misc/xen-kbdfront.c
>> @@ -51,6 +51,16 @@ module_param_array(ptr_size, int, NULL, 0444);
>>   MODULE_PARM_DESC(ptr_size,
>>   	"Pointing device width, height in pixels (default 800,600)");
>>   
>> +static unsigned int no_ptr_dev;
>> +module_param(no_ptr_dev, uint, 0);
> Use type invbool instead?
Hm, better bool then? invbool will require parameter name change to
something like "with_ptr_dev" which might confuse, e.g.
default was to go with pointer device, now we have with_ptr_dev
module parameter: do I now need to set it to preserve the old behavior?
The answer is no (because of invbool), but you have to dig for it.

Will bool work for you?
>> +MODULE_PARM_DESC(no_ptr_dev,
>> +	"If set then no virtual pointing device exposed to the guest");
>> +
>> +static unsigned int no_kbd_dev;
>> +module_param(no_kbd_dev, uint, 0);
> invbool?
>
>
> Juergen

^ permalink raw reply

* Re: [PATCH] Input: xen-kbdfront - allow better run-time configuration
From: Juergen Gross @ 2018-04-19 11:25 UTC (permalink / raw)
  To: Oleksandr Andrushchenko, xen-devel, linux-input, linux-kernel,
	dmitry.torokhov, lyan, boris.ostrovsky
  Cc: andrii_chepurnyi, Oleksandr Andrushchenko
In-Reply-To: <20180418150445.9805-1-andr2000@gmail.com>

On 18/04/18 17:04, Oleksandr Andrushchenko wrote:
> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> 
> It is now only possible to control if multi-touch virtual device
> is created or not (via the corresponding XenStore entries),
> but keyboard and pointer devices are always created.

Why don't you want to go that route for keyboard and mouse, too?
Or does this really make no sense?

> In some cases this is not desirable. For example, if virtual
> keyboard device is exposed to Android then the latter won't
> automatically show on-screen keyboard as it expects that a
> physical keyboard device can be used for typing.
> 
> Make it possible to configure which virtual devices are created
> with module parameters:
>   - no_ptr_dev=1 if no pointer device needs to be created
>   - no_kbd_dev=1 if no keyboard device needs to be created
> Keep old behavior by default.
> 
> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> Suggested-by: Andrii Chepurnyi <andrii_chepurnyi@epam.com>
> Tested-by: Andrii Chepurnyi <andrii_chepurnyi@epam.com>
> ---
>  drivers/input/misc/xen-kbdfront.c | 159 +++++++++++++++++-------------
>  1 file changed, 92 insertions(+), 67 deletions(-)
> 
> diff --git a/drivers/input/misc/xen-kbdfront.c b/drivers/input/misc/xen-kbdfront.c
> index d91f3b1c5375..a3306aad40b0 100644
> --- a/drivers/input/misc/xen-kbdfront.c
> +++ b/drivers/input/misc/xen-kbdfront.c
> @@ -51,6 +51,16 @@ module_param_array(ptr_size, int, NULL, 0444);
>  MODULE_PARM_DESC(ptr_size,
>  	"Pointing device width, height in pixels (default 800,600)");
>  
> +static unsigned int no_ptr_dev;
> +module_param(no_ptr_dev, uint, 0);

Use type invbool instead?

> +MODULE_PARM_DESC(no_ptr_dev,
> +	"If set then no virtual pointing device exposed to the guest");
> +
> +static unsigned int no_kbd_dev;
> +module_param(no_kbd_dev, uint, 0);

invbool?


Juergen

^ permalink raw reply

* [PATCH v2] Input: i8042 - Fix KBD port cannot wake up system from suspend-to-idle
From: Kai-Heng Feng @ 2018-04-19  8:49 UTC (permalink / raw)
  To: dmitry.torokhov; +Cc: linux-input, linux-kernel, Kai-Heng Feng

From: Kai Heng Feng <kai.heng.feng@canonical.com>

Commit f13b2065de81 ("Input: i8042 - allow KBD and AUX ports to wake up
from suspend-to-idle") make system in s2idle can be woken up by i8042
keyboard, but it's disabled by default.

In commit 3e6e15a862a0 ("Input: enable remote wakeup for PNP i8042
keyboard ports") states that "Keyboard ports are always supposed to be
wakeup-enabled", it should be enabled by default. Keyboard wakeup from
s2idles is also the default behavior for other x86 OSes.

In addition to PNP port, enable keyboard wakeup from serio port by
default for x86 systems.

Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
---
v2:
Keep the PNP wakeup part.
Only enable for x86 systems.
Update commit message.

 drivers/input/serio/i8042.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/input/serio/i8042.c b/drivers/input/serio/i8042.c
index 824f4c1c1f31..0b6878e8037b 100644
--- a/drivers/input/serio/i8042.c
+++ b/drivers/input/serio/i8042.c
@@ -1400,6 +1400,12 @@ static void __init i8042_register_ports(void)
 				i8042_ports[i].irq);
 			serio_register_port(serio);
 			device_set_wakeup_capable(&serio->dev, true);
+
+#ifdef CONFIG_X86
+			/* Keyboard ports are always supposed to be wakeup-enabled */
+			if (i == I8042_KBD_PORT_NO)
+				device_wakeup_enable(&serio->dev);
+#endif
 		}
 	}
 }
-- 
2.17.0

^ permalink raw reply related

* [PATCH v3 1/3] resource: Use list_head to link sibling resource
From: Baoquan He @ 2018-04-19  0:18 UTC (permalink / raw)
  To: linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b,
	robh+dt-DgEjT+Ai2ygdnm+yROfE0A,
	dan.j.williams-ral2JQCrhuEAvxtiuMwx3w,
	nicolas.pitre-QSEj5FYQhm4dnm+yROfE0A, josh-iaAMLnmF4UmaiuxdJuQwMA
  Cc: Brijesh Singh, devicetree-u79uwXL29TY76Z2rM5mHXA, David Airlie,
	linux-pci-u79uwXL29TY76Z2rM5mHXA, Wei Yang, Keith Busch,
	Yaowei Bai, K. Y. Srinivasan, Frank Rowand, Lorenzo Pieralisi,
	Stephen Hemminger, Baoquan He,
	linux-nvdimm-hn68Rpc1hR1g9hUCZPvPmw, Patrik Jakobsson,
	linux-input-u79uwXL29TY76Z2rM5mHXA, Borislav Petkov, Tom Lendacky,
	Haiyang Zhang, Jérôme Glisse, Bjorn Helgaas,
	Thomas Gleixner, Jonathan Derrick, Greg Kroah-Hartman
In-Reply-To: <20180419001848.3041-1-bhe-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

The struct resource uses singly linked list to link siblings. It's not
easy to do reverse iteration on sibling list. So replace it with list_head.

And this makes codes in kernel/resource.c more readable after refactoring
than pointer operation.

Besides, type of member variables of struct resource, sibling and child, are
changed from 'struct resource *' to 'struct list_head'. This brings two
pointers of size increase.

Suggested-by: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Baoquan He <bhe@redhat.com>
Cc: Patrik Jakobsson <patrik.r.jakobsson@gmail.com>
Cc: David Airlie <airlied@linux.ie>
Cc: "K. Y. Srinivasan" <kys@microsoft.com>
Cc: Haiyang Zhang <haiyangz@microsoft.com>
Cc: Stephen Hemminger <sthemmin@microsoft.com>
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Frank Rowand <frowand.list@gmail.com>
Cc: Keith Busch <keith.busch@intel.com>
Cc: Jonathan Derrick <jonathan.derrick@intel.com>
Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: "Jérôme Glisse" <jglisse@redhat.com>
Cc: Borislav Petkov <bp@suse.de>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Yaowei Bai <baiyaowei@cmss.chinamobile.com>
Cc: Wei Yang <richard.weiyang@gmail.com>
Cc: devel@linuxdriverproject.org
Cc: linux-input@vger.kernel.org
Cc: linux-nvdimm@lists.01.org
Cc: devicetree@vger.kernel.org
Cc: linux-pci@vger.kernel.org
---
v2->v3:
  Rename resource functions first_child() and sibling() to
  resource_first_chils() and resource_sibling(). Dan suggested this.

  Move resource_first_chils() and resource_sibling() to linux/ioport.h
  and make them as inline function. Rob suggested this. Accordingly add
  linux/list.h including in linux/ioport.h, please help review if this
  bring efficiency degradation or code redundancy.

  The change on struct resource {} bring two pointers of size increase,
  mention this in git log to make it more specifically, Rob suggested
  this.

 arch/sparc/kernel/ioport.c                  |   2 +-
 drivers/gpu/drm/drm_memory.c                |   3 +-
 drivers/gpu/drm/gma500/gtt.c                |   5 +-
 drivers/hv/vmbus_drv.c                      |  52 ++++----
 drivers/input/joystick/iforce/iforce-main.c |   4 +-
 drivers/nvdimm/e820.c                       |   2 +-
 drivers/nvdimm/namespace_devs.c             |   6 +-
 drivers/nvdimm/nd.h                         |   5 +-
 drivers/of/address.c                        |   4 +-
 drivers/parisc/lba_pci.c                    |   4 +-
 drivers/pci/host/vmd.c                      |   8 +-
 drivers/pci/probe.c                         |   2 +
 drivers/pci/setup-bus.c                     |   2 +-
 include/linux/ioport.h                      |  17 ++-
 kernel/resource.c                           | 181 +++++++++++++---------------
 15 files changed, 148 insertions(+), 149 deletions(-)

diff --git a/arch/sparc/kernel/ioport.c b/arch/sparc/kernel/ioport.c
index 3bcef9ce74df..4e91fbbbedcc 100644
--- a/arch/sparc/kernel/ioport.c
+++ b/arch/sparc/kernel/ioport.c
@@ -669,7 +669,7 @@ static int sparc_io_proc_show(struct seq_file *m, void *v)
 	struct resource *root = m->private, *r;
 	const char *nm;
 
-	for (r = root->child; r != NULL; r = r->sibling) {
+	list_for_each_entry(r, &root->child, sibling) {
 		if ((nm = r->name) == NULL) nm = "???";
 		seq_printf(m, "%016llx-%016llx: %s\n",
 				(unsigned long long)r->start,
diff --git a/drivers/gpu/drm/drm_memory.c b/drivers/gpu/drm/drm_memory.c
index 3c54044214db..53e300a993dc 100644
--- a/drivers/gpu/drm/drm_memory.c
+++ b/drivers/gpu/drm/drm_memory.c
@@ -155,9 +155,8 @@ u64 drm_get_max_iomem(void)
 	struct resource *tmp;
 	resource_size_t max_iomem = 0;
 
-	for (tmp = iomem_resource.child; tmp; tmp = tmp->sibling) {
+	list_for_each_entry(tmp, &iomem_resource.child, sibling)
 		max_iomem = max(max_iomem,  tmp->end);
-	}
 
 	return max_iomem;
 }
diff --git a/drivers/gpu/drm/gma500/gtt.c b/drivers/gpu/drm/gma500/gtt.c
index 3949b0990916..addd3bc009af 100644
--- a/drivers/gpu/drm/gma500/gtt.c
+++ b/drivers/gpu/drm/gma500/gtt.c
@@ -565,7 +565,7 @@ int psb_gtt_init(struct drm_device *dev, int resume)
 int psb_gtt_restore(struct drm_device *dev)
 {
 	struct drm_psb_private *dev_priv = dev->dev_private;
-	struct resource *r = dev_priv->gtt_mem->child;
+	struct resource *r;
 	struct gtt_range *range;
 	unsigned int restored = 0, total = 0, size = 0;
 
@@ -573,14 +573,13 @@ int psb_gtt_restore(struct drm_device *dev)
 	mutex_lock(&dev_priv->gtt_mutex);
 	psb_gtt_init(dev, 1);
 
-	while (r != NULL) {
+	list_for_each_entry(r, &dev_priv->gtt_mem->child, sibling) {
 		range = container_of(r, struct gtt_range, resource);
 		if (range->pages) {
 			psb_gtt_insert(dev, range, 1);
 			size += range->resource.end - range->resource.start;
 			restored++;
 		}
-		r = r->sibling;
 		total++;
 	}
 	mutex_unlock(&dev_priv->gtt_mutex);
diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index b10fe26c4891..d87ec5a1bc4c 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -1412,9 +1412,8 @@ static acpi_status vmbus_walk_resources(struct acpi_resource *res, void *ctx)
 {
 	resource_size_t start = 0;
 	resource_size_t end = 0;
-	struct resource *new_res;
+	struct resource *new_res, *tmp;
 	struct resource **old_res = &hyperv_mmio;
-	struct resource **prev_res = NULL;
 
 	switch (res->type) {
 
@@ -1461,44 +1460,36 @@ static acpi_status vmbus_walk_resources(struct acpi_resource *res, void *ctx)
 	/*
 	 * If two ranges are adjacent, merge them.
 	 */
-	do {
-		if (!*old_res) {
-			*old_res = new_res;
-			break;
-		}
-
-		if (((*old_res)->end + 1) == new_res->start) {
-			(*old_res)->end = new_res->end;
+	if (!*old_res) {
+		*old_res = new_res;
+		return AE_OK;
+	}
+	tmp = *old_res;
+	list_for_each_entry_from(tmp, &tmp->parent->child, sibling) {
+		if ((tmp->end + 1) == new_res->start) {
+			tmp->end = new_res->end;
 			kfree(new_res);
 			break;
 		}
 
-		if ((*old_res)->start == new_res->end + 1) {
-			(*old_res)->start = new_res->start;
+		if (tmp->start == new_res->end + 1) {
+			tmp->start = new_res->start;
 			kfree(new_res);
 			break;
 		}
 
-		if ((*old_res)->start > new_res->end) {
-			new_res->sibling = *old_res;
-			if (prev_res)
-				(*prev_res)->sibling = new_res;
-			*old_res = new_res;
+		if (tmp->start > new_res->end) {
+			list_add(&new_res->sibling, tmp->sibling.prev);
 			break;
 		}
-
-		prev_res = old_res;
-		old_res = &(*old_res)->sibling;
-
-	} while (1);
+	}
 
 	return AE_OK;
 }
 
 static int vmbus_acpi_remove(struct acpi_device *device)
 {
-	struct resource *cur_res;
-	struct resource *next_res;
+	struct resource *res;
 
 	if (hyperv_mmio) {
 		if (fb_mmio) {
@@ -1507,10 +1498,9 @@ static int vmbus_acpi_remove(struct acpi_device *device)
 			fb_mmio = NULL;
 		}
 
-		for (cur_res = hyperv_mmio; cur_res; cur_res = next_res) {
-			next_res = cur_res->sibling;
-			kfree(cur_res);
-		}
+		res = hyperv_mmio;
+		list_for_each_entry_from(res, &res->parent->child, sibling)
+			kfree(res);
 	}
 
 	return 0;
@@ -1596,7 +1586,8 @@ int vmbus_allocate_mmio(struct resource **new, struct hv_device *device_obj,
 		}
 	}
 
-	for (iter = hyperv_mmio; iter; iter = iter->sibling) {
+	iter = hyperv_mmio;
+	list_for_each_entry_from(iter, &iter->parent->child, sibling) {
 		if ((iter->start >= max) || (iter->end <= min))
 			continue;
 
@@ -1639,7 +1630,8 @@ void vmbus_free_mmio(resource_size_t start, resource_size_t size)
 	struct resource *iter;
 
 	down(&hyperv_mmio_lock);
-	for (iter = hyperv_mmio; iter; iter = iter->sibling) {
+	iter = hyperv_mmio;
+	list_for_each_entry_from(iter, &iter->parent->child, sibling) {
 		if ((iter->start >= start + size) || (iter->end <= start))
 			continue;
 
diff --git a/drivers/input/joystick/iforce/iforce-main.c b/drivers/input/joystick/iforce/iforce-main.c
index daeeb4c7e3b0..5c0be27b33ff 100644
--- a/drivers/input/joystick/iforce/iforce-main.c
+++ b/drivers/input/joystick/iforce/iforce-main.c
@@ -305,8 +305,8 @@ int iforce_init_device(struct iforce *iforce)
 	iforce->device_memory.end = 200;
 	iforce->device_memory.flags = IORESOURCE_MEM;
 	iforce->device_memory.parent = NULL;
-	iforce->device_memory.child = NULL;
-	iforce->device_memory.sibling = NULL;
+	INIT_LIST_HEAD(&iforce->device_memory.child);
+	INIT_LIST_HEAD(&iforce->device_memory.sibling);
 
 /*
  * Wait until device ready - until it sends its first response.
diff --git a/drivers/nvdimm/e820.c b/drivers/nvdimm/e820.c
index 6f9a6ffd7cde..513e661bb0d8 100644
--- a/drivers/nvdimm/e820.c
+++ b/drivers/nvdimm/e820.c
@@ -53,7 +53,7 @@ static int e820_pmem_probe(struct platform_device *pdev)
 		goto err;
 	platform_set_drvdata(pdev, nvdimm_bus);
 
-	for (p = iomem_resource.child; p ; p = p->sibling) {
+	list_for_each_entry(p, &iomem_resource.child, sibling) {
 		struct nd_region_desc ndr_desc;
 
 		if (p->desc != IORES_DESC_PERSISTENT_MEMORY_LEGACY)
diff --git a/drivers/nvdimm/namespace_devs.c b/drivers/nvdimm/namespace_devs.c
index 28afdd668905..f53d410d9981 100644
--- a/drivers/nvdimm/namespace_devs.c
+++ b/drivers/nvdimm/namespace_devs.c
@@ -637,7 +637,7 @@ static resource_size_t scan_allocate(struct nd_region *nd_region,
  retry:
 	first = 0;
 	for_each_dpa_resource(ndd, res) {
-		struct resource *next = res->sibling, *new_res = NULL;
+		struct resource *next = resource_sibling(res), *new_res = NULL;
 		resource_size_t allocate, available = 0;
 		enum alloc_loc loc = ALLOC_ERR;
 		const char *action;
@@ -763,7 +763,7 @@ static resource_size_t scan_allocate(struct nd_region *nd_region,
 	 * an initial "pmem-reserve pass".  Only do an initial BLK allocation
 	 * when none of the DPA space is reserved.
 	 */
-	if ((is_pmem || !ndd->dpa.child) && n == to_allocate)
+	if ((is_pmem || list_empty(&ndd->dpa.child)) && n == to_allocate)
 		return init_dpa_allocation(label_id, nd_region, nd_mapping, n);
 	return n;
 }
@@ -779,7 +779,7 @@ static int merge_dpa(struct nd_region *nd_region,
  retry:
 	for_each_dpa_resource(ndd, res) {
 		int rc;
-		struct resource *next = res->sibling;
+		struct resource *next = resource_sibling(res);
 		resource_size_t end = res->start + resource_size(res);
 
 		if (!next || strcmp(res->name, label_id->id) != 0
diff --git a/drivers/nvdimm/nd.h b/drivers/nvdimm/nd.h
index 32e0364b48b9..da7da15e03e7 100644
--- a/drivers/nvdimm/nd.h
+++ b/drivers/nvdimm/nd.h
@@ -102,11 +102,10 @@ unsigned sizeof_namespace_label(struct nvdimm_drvdata *ndd);
 		(unsigned long long) (res ? res->start : 0), ##arg)
 
 #define for_each_dpa_resource(ndd, res) \
-	for (res = (ndd)->dpa.child; res; res = res->sibling)
+	list_for_each_entry(res, &(ndd)->dpa.child, sibling)
 
 #define for_each_dpa_resource_safe(ndd, res, next) \
-	for (res = (ndd)->dpa.child, next = res ? res->sibling : NULL; \
-			res; res = next, next = next ? next->sibling : NULL)
+	list_for_each_entry_safe(res, next, &(ndd)->dpa.child, sibling)
 
 struct nd_percpu_lane {
 	int count;
diff --git a/drivers/of/address.c b/drivers/of/address.c
index 53349912ac75..e2e25719ab52 100644
--- a/drivers/of/address.c
+++ b/drivers/of/address.c
@@ -330,7 +330,9 @@ int of_pci_range_to_resource(struct of_pci_range *range,
 {
 	int err;
 	res->flags = range->flags;
-	res->parent = res->child = res->sibling = NULL;
+	res->parent = NULL;
+	INIT_LIST_HEAD(&res->child);
+	INIT_LIST_HEAD(&res->sibling);
 	res->name = np->full_name;
 
 	if (res->flags & IORESOURCE_IO) {
diff --git a/drivers/parisc/lba_pci.c b/drivers/parisc/lba_pci.c
index 69bd98421eb1..7482bdfd1959 100644
--- a/drivers/parisc/lba_pci.c
+++ b/drivers/parisc/lba_pci.c
@@ -170,8 +170,8 @@ lba_dump_res(struct resource *r, int d)
 	for (i = d; i ; --i) printk(" ");
 	printk(KERN_DEBUG "%p [%lx,%lx]/%lx\n", r,
 		(long)r->start, (long)r->end, r->flags);
-	lba_dump_res(r->child, d+2);
-	lba_dump_res(r->sibling, d);
+	lba_dump_res(resource_first_child(&r->child), d+2);
+	lba_dump_res(resource_sibling(r), d);
 }
 
 
diff --git a/drivers/pci/host/vmd.c b/drivers/pci/host/vmd.c
index 930a8fa08bd6..c3000af903ea 100644
--- a/drivers/pci/host/vmd.c
+++ b/drivers/pci/host/vmd.c
@@ -520,14 +520,14 @@ static struct pci_ops vmd_ops = {
 
 static void vmd_attach_resources(struct vmd_dev *vmd)
 {
-	vmd->dev->resource[VMD_MEMBAR1].child = &vmd->resources[1];
-	vmd->dev->resource[VMD_MEMBAR2].child = &vmd->resources[2];
+	list_add(&vmd->resources[1].sibling, &vmd->dev->resource[VMD_MEMBAR1].child);
+	list_add(&vmd->resources[2].sibling, &vmd->dev->resource[VMD_MEMBAR2].child);
 }
 
 static void vmd_detach_resources(struct vmd_dev *vmd)
 {
-	vmd->dev->resource[VMD_MEMBAR1].child = NULL;
-	vmd->dev->resource[VMD_MEMBAR2].child = NULL;
+	INIT_LIST_HEAD(&vmd->dev->resource[VMD_MEMBAR1].child);
+	INIT_LIST_HEAD(&vmd->dev->resource[VMD_MEMBAR2].child);
 }
 
 /*
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index ac91b6fd0bcd..d162c77bec29 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -59,6 +59,8 @@ static struct resource *get_pci_domain_busn_res(int domain_nr)
 	r->res.start = 0;
 	r->res.end = 0xff;
 	r->res.flags = IORESOURCE_BUS | IORESOURCE_PCI_FIXED;
+	INIT_LIST_HEAD(&r->res.child);
+	INIT_LIST_HEAD(&r->res.sibling);
 
 	list_add_tail(&r->list, &pci_domain_busn_res_list);
 
diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index 072784f55ea5..0d5e30004ca6 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -2107,7 +2107,7 @@ int pci_reassign_bridge_resources(struct pci_dev *bridge, unsigned long type)
 				continue;
 
 			/* Ignore BARs which are still in use */
-			if (res->child)
+			if (!list_empty(&res->child))
 				continue;
 
 			ret = add_to_list(&saved, bridge, res, 0, 0);
diff --git a/include/linux/ioport.h b/include/linux/ioport.h
index da0ebaec25f0..225d13d3500a 100644
--- a/include/linux/ioport.h
+++ b/include/linux/ioport.h
@@ -12,6 +12,7 @@
 #ifndef __ASSEMBLY__
 #include <linux/compiler.h>
 #include <linux/types.h>
+#include <linux/list.h>
 /*
  * Resources are tree-like, allowing
  * nesting etc..
@@ -22,7 +23,8 @@ struct resource {
 	const char *name;
 	unsigned long flags;
 	unsigned long desc;
-	struct resource *parent, *sibling, *child;
+	struct list_head child, sibling;
+	struct resource *parent;
 };
 
 /*
@@ -215,7 +217,6 @@ static inline bool resource_contains(struct resource *r1, struct resource *r2)
 	return r1->start <= r2->start && r1->end >= r2->end;
 }
 
-
 /* Convenience shorthand with allocation */
 #define request_region(start,n,name)		__request_region(&ioport_resource, (start), (n), (name), 0)
 #define request_muxed_region(start,n,name)	__request_region(&ioport_resource, (start), (n), (name), IORESOURCE_MUXED)
@@ -286,6 +287,18 @@ static inline bool resource_overlaps(struct resource *r1, struct resource *r2)
        return (r1->start <= r2->end && r1->end >= r2->start);
 }
 
+static inline struct resource *resource_sibling(struct resource *res)
+{
+	if (res->parent && !list_is_last(&res->sibling, &res->parent->child))
+		return list_next_entry(res, sibling);
+	return NULL;
+}
+
+static inline struct resource *resource_first_child(struct list_head *head)
+{
+	return list_first_entry_or_null(head, struct resource, sibling);
+}
+
 
 #endif /* __ASSEMBLY__ */
 #endif	/* _LINUX_IOPORT_H */
diff --git a/kernel/resource.c b/kernel/resource.c
index 2af6c03858b9..4f560991c130 100644
--- a/kernel/resource.c
+++ b/kernel/resource.c
@@ -31,6 +31,8 @@ struct resource ioport_resource = {
 	.start	= 0,
 	.end	= IO_SPACE_LIMIT,
 	.flags	= IORESOURCE_IO,
+	.sibling = LIST_HEAD_INIT(ioport_resource.sibling),
+	.child  = LIST_HEAD_INIT(ioport_resource.child),
 };
 EXPORT_SYMBOL(ioport_resource);
 
@@ -39,6 +41,8 @@ struct resource iomem_resource = {
 	.start	= 0,
 	.end	= -1,
 	.flags	= IORESOURCE_MEM,
+	.sibling = LIST_HEAD_INIT(iomem_resource.sibling),
+	.child  = LIST_HEAD_INIT(iomem_resource.child),
 };
 EXPORT_SYMBOL(iomem_resource);
 
@@ -57,20 +61,20 @@ static DEFINE_RWLOCK(resource_lock);
  * by boot mem after the system is up. So for reusing the resource entry
  * we need to remember the resource.
  */
-static struct resource *bootmem_resource_free;
+static struct list_head bootmem_resource_free = LIST_HEAD_INIT(bootmem_resource_free);
 static DEFINE_SPINLOCK(bootmem_resource_lock);
 
 static struct resource *next_resource(struct resource *p, bool sibling_only)
 {
 	/* Caller wants to traverse through siblings only */
 	if (sibling_only)
-		return p->sibling;
+		return resource_sibling(p);
 
-	if (p->child)
-		return p->child;
-	while (!p->sibling && p->parent)
+	if (!list_empty(&p->child))
+		return resource_first_child(&p->child);
+	while (!resource_sibling(p) && p->parent)
 		p = p->parent;
-	return p->sibling;
+	return resource_sibling(p);
 }
 
 static void *r_next(struct seq_file *m, void *v, loff_t *pos)
@@ -90,7 +94,7 @@ static void *r_start(struct seq_file *m, loff_t *pos)
 	struct resource *p = m->private;
 	loff_t l = 0;
 	read_lock(&resource_lock);
-	for (p = p->child; p && l < *pos; p = r_next(m, p, &l))
+	for (p = resource_first_child(&p->child); p && l < *pos; p = r_next(m, p, &l))
 		;
 	return p;
 }
@@ -186,8 +190,7 @@ static void free_resource(struct resource *res)
 
 	if (!PageSlab(virt_to_head_page(res))) {
 		spin_lock(&bootmem_resource_lock);
-		res->sibling = bootmem_resource_free;
-		bootmem_resource_free = res;
+		list_add(&res->sibling, &bootmem_resource_free);
 		spin_unlock(&bootmem_resource_lock);
 	} else {
 		kfree(res);
@@ -199,10 +202,9 @@ static struct resource *alloc_resource(gfp_t flags)
 	struct resource *res = NULL;
 
 	spin_lock(&bootmem_resource_lock);
-	if (bootmem_resource_free) {
-		res = bootmem_resource_free;
-		bootmem_resource_free = res->sibling;
-	}
+	res = resource_first_child(&bootmem_resource_free);
+	if (res)
+		list_del(&res->sibling);
 	spin_unlock(&bootmem_resource_lock);
 
 	if (res)
@@ -210,6 +212,8 @@ static struct resource *alloc_resource(gfp_t flags)
 	else
 		res = kzalloc(sizeof(struct resource), flags);
 
+	INIT_LIST_HEAD(&res->child);
+	INIT_LIST_HEAD(&res->sibling);
 	return res;
 }
 
@@ -218,7 +222,7 @@ static struct resource * __request_resource(struct resource *root, struct resour
 {
 	resource_size_t start = new->start;
 	resource_size_t end = new->end;
-	struct resource *tmp, **p;
+	struct resource *tmp;
 
 	if (end < start)
 		return root;
@@ -226,64 +230,62 @@ static struct resource * __request_resource(struct resource *root, struct resour
 		return root;
 	if (end > root->end)
 		return root;
-	p = &root->child;
-	for (;;) {
-		tmp = *p;
-		if (!tmp || tmp->start > end) {
-			new->sibling = tmp;
-			*p = new;
+
+	if (list_empty(&root->child)) {
+		list_add(&new->sibling, &root->child);
+		new->parent = root;
+		INIT_LIST_HEAD(&new->child);
+		return NULL;
+	}
+
+	list_for_each_entry(tmp, &root->child, sibling) {
+		if (tmp->start > end) {
+			list_add(&new->sibling, tmp->sibling.prev);
 			new->parent = root;
+			INIT_LIST_HEAD(&new->child);
 			return NULL;
 		}
-		p = &tmp->sibling;
 		if (tmp->end < start)
 			continue;
 		return tmp;
 	}
+
+	list_add_tail(&new->sibling, &root->child);
+	new->parent = root;
+	INIT_LIST_HEAD(&new->child);
+	return NULL;
 }
 
 static int __release_resource(struct resource *old, bool release_child)
 {
-	struct resource *tmp, **p, *chd;
+	struct resource *tmp, *next, *chd;
 
-	p = &old->parent->child;
-	for (;;) {
-		tmp = *p;
-		if (!tmp)
-			break;
+	list_for_each_entry_safe(tmp, next, &old->parent->child, sibling) {
 		if (tmp == old) {
-			if (release_child || !(tmp->child)) {
-				*p = tmp->sibling;
+			if (release_child || list_empty(&tmp->child)) {
+				list_del(&tmp->sibling);
 			} else {
-				for (chd = tmp->child;; chd = chd->sibling) {
+				list_for_each_entry(chd, &tmp->child, sibling)
 					chd->parent = tmp->parent;
-					if (!(chd->sibling))
-						break;
-				}
-				*p = tmp->child;
-				chd->sibling = tmp->sibling;
+				list_splice(&tmp->child, tmp->sibling.prev);
+				list_del(&tmp->sibling);
 			}
+
 			old->parent = NULL;
 			return 0;
 		}
-		p = &tmp->sibling;
 	}
 	return -EINVAL;
 }
 
 static void __release_child_resources(struct resource *r)
 {
-	struct resource *tmp, *p;
+	struct resource *tmp, *next;
 	resource_size_t size;
 
-	p = r->child;
-	r->child = NULL;
-	while (p) {
-		tmp = p;
-		p = p->sibling;
-
+	list_for_each_entry_safe(tmp, next, &r->child, sibling) {
 		tmp->parent = NULL;
-		tmp->sibling = NULL;
+		INIT_LIST_HEAD(&tmp->sibling);
 		__release_child_resources(tmp);
 
 		printk(KERN_DEBUG "release child resource %pR\n", tmp);
@@ -292,6 +294,8 @@ static void __release_child_resources(struct resource *r)
 		tmp->start = 0;
 		tmp->end = size - 1;
 	}
+
+	INIT_LIST_HEAD(&tmp->child);
 }
 
 void release_child_resources(struct resource *r)
@@ -376,7 +380,8 @@ static int find_next_iomem_res(struct resource *res, unsigned long desc,
 
 	read_lock(&resource_lock);
 
-	for (p = iomem_resource.child; p; p = next_resource(p, sibling_only)) {
+	for (p = resource_first_child(&iomem_resource.child); p;
+			p = next_resource(p, sibling_only)) {
 		if ((p->flags & res->flags) != res->flags)
 			continue;
 		if ((desc != IORES_DESC_NONE) && (desc != p->desc))
@@ -564,7 +569,7 @@ int region_intersects(resource_size_t start, size_t size, unsigned long flags,
 	struct resource *p;
 
 	read_lock(&resource_lock);
-	for (p = iomem_resource.child; p ; p = p->sibling) {
+	list_for_each_entry(p, &iomem_resource.child, sibling) {
 		bool is_type = (((p->flags & flags) == flags) &&
 				((desc == IORES_DESC_NONE) ||
 				 (desc == p->desc)));
@@ -618,7 +623,7 @@ static int __find_resource(struct resource *root, struct resource *old,
 			 resource_size_t  size,
 			 struct resource_constraint *constraint)
 {
-	struct resource *this = root->child;
+	struct resource *this = resource_first_child(&root->child);
 	struct resource tmp = *new, avail, alloc;
 
 	tmp.start = root->start;
@@ -628,7 +633,7 @@ static int __find_resource(struct resource *root, struct resource *old,
 	 */
 	if (this && this->start == root->start) {
 		tmp.start = (this == old) ? old->start : this->end + 1;
-		this = this->sibling;
+		this = resource_sibling(this);
 	}
 	for(;;) {
 		if (this)
@@ -664,7 +669,7 @@ next:		if (!this || this->end == root->end)
 
 		if (this != old)
 			tmp.start = this->end + 1;
-		this = this->sibling;
+		this = resource_sibling(this);
 	}
 	return -EBUSY;
 }
@@ -708,7 +713,7 @@ static int reallocate_resource(struct resource *root, struct resource *old,
 		goto out;
 	}
 
-	if (old->child) {
+	if (!list_empty(&old->child)) {
 		err = -EBUSY;
 		goto out;
 	}
@@ -789,7 +794,7 @@ struct resource *lookup_resource(struct resource *root, resource_size_t start)
 	struct resource *res;
 
 	read_lock(&resource_lock);
-	for (res = root->child; res; res = res->sibling) {
+	list_for_each_entry(res, &root->child, sibling) {
 		if (res->start == start)
 			break;
 	}
@@ -822,32 +827,27 @@ static struct resource * __insert_resource(struct resource *parent, struct resou
 			break;
 	}
 
-	for (next = first; ; next = next->sibling) {
+	for (next = first; ; next = resource_sibling(next)) {
 		/* Partial overlap? Bad, and unfixable */
 		if (next->start < new->start || next->end > new->end)
 			return next;
-		if (!next->sibling)
+		if (!resource_sibling(next))
 			break;
-		if (next->sibling->start > new->end)
+		if (resource_sibling(next)->start > new->end)
 			break;
 	}
-
 	new->parent = parent;
-	new->sibling = next->sibling;
-	new->child = first;
+	list_add(&new->sibling, &next->sibling);
+	INIT_LIST_HEAD(&new->child);
 
-	next->sibling = NULL;
-	for (next = first; next; next = next->sibling)
+	/*
+	 * From first to next, they all fall into new's region, so change them
+	 * as new's children.
+	 */
+	list_cut_position(&new->child, first->sibling.prev, &next->sibling);
+	list_for_each_entry(next, &new->child, sibling)
 		next->parent = new;
 
-	if (parent->child == first) {
-		parent->child = new;
-	} else {
-		next = parent->child;
-		while (next->sibling != first)
-			next = next->sibling;
-		next->sibling = new;
-	}
 	return NULL;
 }
 
@@ -969,19 +969,17 @@ static int __adjust_resource(struct resource *res, resource_size_t start,
 	if ((start < parent->start) || (end > parent->end))
 		goto out;
 
-	if (res->sibling && (res->sibling->start <= end))
+	if (resource_sibling(res) && (resource_sibling(res)->start <= end))
 		goto out;
 
-	tmp = parent->child;
-	if (tmp != res) {
-		while (tmp->sibling != res)
-			tmp = tmp->sibling;
+	if (res->sibling.prev != &parent->child) {
+		tmp = list_prev_entry(res, sibling);
 		if (start <= tmp->end)
 			goto out;
 	}
 
 skip:
-	for (tmp = res->child; tmp; tmp = tmp->sibling)
+	list_for_each_entry(tmp, &res->child, sibling)
 		if ((tmp->start < start) || (tmp->end > end))
 			goto out;
 
@@ -1206,34 +1204,32 @@ EXPORT_SYMBOL(__request_region);
 void __release_region(struct resource *parent, resource_size_t start,
 			resource_size_t n)
 {
-	struct resource **p;
+	struct resource *res;
 	resource_size_t end;
 
-	p = &parent->child;
+	res = resource_first_child(&parent->child);
 	end = start + n - 1;
 
 	write_lock(&resource_lock);
 
 	for (;;) {
-		struct resource *res = *p;
-
 		if (!res)
 			break;
 		if (res->start <= start && res->end >= end) {
 			if (!(res->flags & IORESOURCE_BUSY)) {
-				p = &res->child;
+				res = resource_first_child(&res->child);
 				continue;
 			}
 			if (res->start != start || res->end != end)
 				break;
-			*p = res->sibling;
+			list_del(&res->sibling);
 			write_unlock(&resource_lock);
 			if (res->flags & IORESOURCE_MUXED)
 				wake_up(&muxed_resource_wait);
 			free_resource(res);
 			return;
 		}
-		p = &res->sibling;
+		res = resource_sibling(res);
 	}
 
 	write_unlock(&resource_lock);
@@ -1268,9 +1264,7 @@ EXPORT_SYMBOL(__release_region);
 int release_mem_region_adjustable(struct resource *parent,
 			resource_size_t start, resource_size_t size)
 {
-	struct resource **p;
-	struct resource *res;
-	struct resource *new_res;
+	struct resource *res, *new_res;
 	resource_size_t end;
 	int ret = -EINVAL;
 
@@ -1281,16 +1275,16 @@ int release_mem_region_adjustable(struct resource *parent,
 	/* The alloc_resource() result gets checked later */
 	new_res = alloc_resource(GFP_KERNEL);
 
-	p = &parent->child;
+	res = resource_first_child(&parent->child);
 	write_lock(&resource_lock);
 
-	while ((res = *p)) {
+	while ((res)) {
 		if (res->start >= end)
 			break;
 
 		/* look for the next resource if it does not fit into */
 		if (res->start > start || res->end < end) {
-			p = &res->sibling;
+			res = resource_sibling(res);
 			continue;
 		}
 
@@ -1298,14 +1292,14 @@ int release_mem_region_adjustable(struct resource *parent,
 			break;
 
 		if (!(res->flags & IORESOURCE_BUSY)) {
-			p = &res->child;
+			res = resource_first_child(&res->child);
 			continue;
 		}
 
 		/* found the target resource; let's adjust accordingly */
 		if (res->start == start && res->end == end) {
 			/* free the whole entry */
-			*p = res->sibling;
+			list_del(&res->sibling);
 			free_resource(res);
 			ret = 0;
 		} else if (res->start == start && res->end != end) {
@@ -1328,14 +1322,13 @@ int release_mem_region_adjustable(struct resource *parent,
 			new_res->flags = res->flags;
 			new_res->desc = res->desc;
 			new_res->parent = res->parent;
-			new_res->sibling = res->sibling;
-			new_res->child = NULL;
+			INIT_LIST_HEAD(&new_res->child);
 
 			ret = __adjust_resource(res, res->start,
 						start - res->start);
 			if (ret)
 				break;
-			res->sibling = new_res;
+			list_add(&new_res->sibling, &res->sibling);
 			new_res = NULL;
 		}
 
@@ -1516,7 +1509,7 @@ static int __init reserve_setup(char *str)
 			res->end = io_start + io_num - 1;
 			res->flags |= IORESOURCE_BUSY;
 			res->desc = IORES_DESC_NONE;
-			res->child = NULL;
+			INIT_LIST_HEAD(&res->child);
 			if (request_resource(parent, res) == 0)
 				reserved = x+1;
 		}
@@ -1536,7 +1529,7 @@ int iomem_map_sanity_check(resource_size_t addr, unsigned long size)
 	loff_t l;
 
 	read_lock(&resource_lock);
-	for (p = p->child; p ; p = r_next(NULL, p, &l)) {
+	for (p = resource_first_child(&p->child); p; p = r_next(NULL, p, &l)) {
 		/*
 		 * We can probably skip the resources without
 		 * IORESOURCE_IO attribute?
@@ -1592,7 +1585,7 @@ bool iomem_is_exclusive(u64 addr)
 	addr = addr & PAGE_MASK;
 
 	read_lock(&resource_lock);
-	for (p = p->child; p ; p = r_next(NULL, p, &l)) {
+	for (p = resource_first_child(&p->child); p; p = r_next(NULL, p, &l)) {
 		/*
 		 * We can probably skip the resources without
 		 * IORESOURCE_IO attribute?
-- 
2.13.6

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

^ permalink raw reply related

* [PATCH] Input: xen-kbdfront - allow better run-time configuration
From: Oleksandr Andrushchenko @ 2018-04-18 15:04 UTC (permalink / raw)
  To: xen-devel, linux-input, linux-kernel, dmitry.torokhov, jgross,
	lyan, boris.ostrovsky
  Cc: andr2000, andrii_chepurnyi, Oleksandr Andrushchenko

From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>

It is now only possible to control if multi-touch virtual device
is created or not (via the corresponding XenStore entries),
but keyboard and pointer devices are always created.
In some cases this is not desirable. For example, if virtual
keyboard device is exposed to Android then the latter won't
automatically show on-screen keyboard as it expects that a
physical keyboard device can be used for typing.

Make it possible to configure which virtual devices are created
with module parameters:
  - no_ptr_dev=1 if no pointer device needs to be created
  - no_kbd_dev=1 if no keyboard device needs to be created
Keep old behavior by default.

Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
Suggested-by: Andrii Chepurnyi <andrii_chepurnyi@epam.com>
Tested-by: Andrii Chepurnyi <andrii_chepurnyi@epam.com>
---
 drivers/input/misc/xen-kbdfront.c | 159 +++++++++++++++++-------------
 1 file changed, 92 insertions(+), 67 deletions(-)

diff --git a/drivers/input/misc/xen-kbdfront.c b/drivers/input/misc/xen-kbdfront.c
index d91f3b1c5375..a3306aad40b0 100644
--- a/drivers/input/misc/xen-kbdfront.c
+++ b/drivers/input/misc/xen-kbdfront.c
@@ -51,6 +51,16 @@ module_param_array(ptr_size, int, NULL, 0444);
 MODULE_PARM_DESC(ptr_size,
 	"Pointing device width, height in pixels (default 800,600)");
 
+static unsigned int no_ptr_dev;
+module_param(no_ptr_dev, uint, 0);
+MODULE_PARM_DESC(no_ptr_dev,
+	"If set then no virtual pointing device exposed to the guest");
+
+static unsigned int no_kbd_dev;
+module_param(no_kbd_dev, uint, 0);
+MODULE_PARM_DESC(no_kbd_dev,
+	"If set then no virtual keyboard device exposed to the guest");
+
 static int xenkbd_remove(struct xenbus_device *);
 static int xenkbd_connect_backend(struct xenbus_device *, struct xenkbd_info *);
 static void xenkbd_disconnect_backend(struct xenkbd_info *);
@@ -63,6 +73,9 @@ static void xenkbd_disconnect_backend(struct xenkbd_info *);
 static void xenkbd_handle_motion_event(struct xenkbd_info *info,
 				       struct xenkbd_motion *motion)
 {
+	if (unlikely(!info->ptr))
+		return;
+
 	input_report_rel(info->ptr, REL_X, motion->rel_x);
 	input_report_rel(info->ptr, REL_Y, motion->rel_y);
 	if (motion->rel_z)
@@ -73,6 +86,9 @@ static void xenkbd_handle_motion_event(struct xenkbd_info *info,
 static void xenkbd_handle_position_event(struct xenkbd_info *info,
 					 struct xenkbd_position *pos)
 {
+	if (unlikely(!info->ptr))
+		return;
+
 	input_report_abs(info->ptr, ABS_X, pos->abs_x);
 	input_report_abs(info->ptr, ABS_Y, pos->abs_y);
 	if (pos->rel_z)
@@ -97,6 +113,9 @@ static void xenkbd_handle_key_event(struct xenkbd_info *info,
 		return;
 	}
 
+	if (unlikely(!dev))
+		return;
+
 	input_event(dev, EV_KEY, key->keycode, value);
 	input_sync(dev);
 }
@@ -192,7 +211,7 @@ static int xenkbd_probe(struct xenbus_device *dev,
 				  const struct xenbus_device_id *id)
 {
 	int ret, i;
-	unsigned int abs, touch;
+	unsigned int touch;
 	struct xenkbd_info *info;
 	struct input_dev *kbd, *ptr, *mtouch;
 
@@ -211,24 +230,6 @@ static int xenkbd_probe(struct xenbus_device *dev,
 	if (!info->page)
 		goto error_nomem;
 
-	/* Set input abs params to match backend screen res */
-	abs = xenbus_read_unsigned(dev->otherend,
-				   XENKBD_FIELD_FEAT_ABS_POINTER, 0);
-	ptr_size[KPARAM_X] = xenbus_read_unsigned(dev->otherend,
-						  XENKBD_FIELD_WIDTH,
-						  ptr_size[KPARAM_X]);
-	ptr_size[KPARAM_Y] = xenbus_read_unsigned(dev->otherend,
-						  XENKBD_FIELD_HEIGHT,
-						  ptr_size[KPARAM_Y]);
-	if (abs) {
-		ret = xenbus_write(XBT_NIL, dev->nodename,
-				   XENKBD_FIELD_REQ_ABS_POINTER, "1");
-		if (ret) {
-			pr_warn("xenkbd: can't request abs-pointer\n");
-			abs = 0;
-		}
-	}
-
 	touch = xenbus_read_unsigned(dev->nodename,
 				     XENKBD_FIELD_FEAT_MTOUCH, 0);
 	if (touch) {
@@ -241,60 +242,84 @@ static int xenkbd_probe(struct xenbus_device *dev,
 	}
 
 	/* keyboard */
-	kbd = input_allocate_device();
-	if (!kbd)
-		goto error_nomem;
-	kbd->name = "Xen Virtual Keyboard";
-	kbd->phys = info->phys;
-	kbd->id.bustype = BUS_PCI;
-	kbd->id.vendor = 0x5853;
-	kbd->id.product = 0xffff;
-
-	__set_bit(EV_KEY, kbd->evbit);
-	for (i = KEY_ESC; i < KEY_UNKNOWN; i++)
-		__set_bit(i, kbd->keybit);
-	for (i = KEY_OK; i < KEY_MAX; i++)
-		__set_bit(i, kbd->keybit);
-
-	ret = input_register_device(kbd);
-	if (ret) {
-		input_free_device(kbd);
-		xenbus_dev_fatal(dev, ret, "input_register_device(kbd)");
-		goto error;
+	if (!no_kbd_dev) {
+		kbd = input_allocate_device();
+		if (!kbd)
+			goto error_nomem;
+		kbd->name = "Xen Virtual Keyboard";
+		kbd->phys = info->phys;
+		kbd->id.bustype = BUS_PCI;
+		kbd->id.vendor = 0x5853;
+		kbd->id.product = 0xffff;
+
+		__set_bit(EV_KEY, kbd->evbit);
+		for (i = KEY_ESC; i < KEY_UNKNOWN; i++)
+			__set_bit(i, kbd->keybit);
+		for (i = KEY_OK; i < KEY_MAX; i++)
+			__set_bit(i, kbd->keybit);
+
+		ret = input_register_device(kbd);
+		if (ret) {
+			input_free_device(kbd);
+			xenbus_dev_fatal(dev, ret, "input_register_device(kbd)");
+			goto error;
+		}
+		info->kbd = kbd;
 	}
-	info->kbd = kbd;
 
 	/* pointing device */
-	ptr = input_allocate_device();
-	if (!ptr)
-		goto error_nomem;
-	ptr->name = "Xen Virtual Pointer";
-	ptr->phys = info->phys;
-	ptr->id.bustype = BUS_PCI;
-	ptr->id.vendor = 0x5853;
-	ptr->id.product = 0xfffe;
-
-	if (abs) {
-		__set_bit(EV_ABS, ptr->evbit);
-		input_set_abs_params(ptr, ABS_X, 0, ptr_size[KPARAM_X], 0, 0);
-		input_set_abs_params(ptr, ABS_Y, 0, ptr_size[KPARAM_Y], 0, 0);
-	} else {
-		input_set_capability(ptr, EV_REL, REL_X);
-		input_set_capability(ptr, EV_REL, REL_Y);
-	}
-	input_set_capability(ptr, EV_REL, REL_WHEEL);
+	if (!no_ptr_dev) {
+		unsigned int abs;
+
+		/* Set input abs params to match backend screen res */
+		abs = xenbus_read_unsigned(dev->otherend,
+					   XENKBD_FIELD_FEAT_ABS_POINTER, 0);
+		ptr_size[KPARAM_X] = xenbus_read_unsigned(dev->otherend,
+							  XENKBD_FIELD_WIDTH,
+							  ptr_size[KPARAM_X]);
+		ptr_size[KPARAM_Y] = xenbus_read_unsigned(dev->otherend,
+							  XENKBD_FIELD_HEIGHT,
+							  ptr_size[KPARAM_Y]);
+		if (abs) {
+			ret = xenbus_write(XBT_NIL, dev->nodename,
+					   XENKBD_FIELD_REQ_ABS_POINTER, "1");
+			if (ret) {
+				pr_warn("xenkbd: can't request abs-pointer\n");
+				abs = 0;
+			}
+		}
 
-	__set_bit(EV_KEY, ptr->evbit);
-	for (i = BTN_LEFT; i <= BTN_TASK; i++)
-		__set_bit(i, ptr->keybit);
+		ptr = input_allocate_device();
+		if (!ptr)
+			goto error_nomem;
+		ptr->name = "Xen Virtual Pointer";
+		ptr->phys = info->phys;
+		ptr->id.bustype = BUS_PCI;
+		ptr->id.vendor = 0x5853;
+		ptr->id.product = 0xfffe;
+
+		if (abs) {
+			__set_bit(EV_ABS, ptr->evbit);
+			input_set_abs_params(ptr, ABS_X, 0, ptr_size[KPARAM_X], 0, 0);
+			input_set_abs_params(ptr, ABS_Y, 0, ptr_size[KPARAM_Y], 0, 0);
+		} else {
+			input_set_capability(ptr, EV_REL, REL_X);
+			input_set_capability(ptr, EV_REL, REL_Y);
+		}
+		input_set_capability(ptr, EV_REL, REL_WHEEL);
 
-	ret = input_register_device(ptr);
-	if (ret) {
-		input_free_device(ptr);
-		xenbus_dev_fatal(dev, ret, "input_register_device(ptr)");
-		goto error;
+		__set_bit(EV_KEY, ptr->evbit);
+		for (i = BTN_LEFT; i <= BTN_TASK; i++)
+			__set_bit(i, ptr->keybit);
+
+		ret = input_register_device(ptr);
+		if (ret) {
+			input_free_device(ptr);
+			xenbus_dev_fatal(dev, ret, "input_register_device(ptr)");
+			goto error;
+		}
+		info->ptr = ptr;
 	}
-	info->ptr = ptr;
 
 	/* multi-touch device */
 	if (touch) {
-- 
2.17.0

^ permalink raw reply related

* Re: [PATCH v3 06/11] iio: inkern: add module put/get on iio dev module when requesting channels
From: Lars-Peter Clausen @ 2018-04-18 12:37 UTC (permalink / raw)
  To: Jonathan Cameron, Dmitry Torokhov
  Cc: Eugen Hristev, Jonathan Cameron, ludovic.desroches,
	alexandre.belloni, linux-arm-kernel, devicetree, linux-kernel,
	linux-iio, linux-input, nicolas.ferre, robh
In-Reply-To: <20180418103548.000078b7@huawei.com>

On 04/18/2018 11:35 AM, Jonathan Cameron wrote:
> On Tue, 17 Apr 2018 12:19:06 -0700
> Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> 
>> Hi Eugen,
>>
>> On Tue, Apr 17, 2018 at 10:39:24AM +0300, Eugen Hristev wrote:
>>>
>>>
>>> On 17.04.2018 02:58, Dmitry Torokhov wrote:  
>>>> On Sun, Apr 15, 2018 at 08:33:21PM +0100, Jonathan Cameron wrote:  
>>>>> On Tue, 10 Apr 2018 11:57:52 +0300
>>>>> Eugen Hristev <eugen.hristev@microchip.com> wrote:
>>>>>   
>>>>>> When requesting channels for a particular consumer device,
>>>>>> besides requesting the device (incrementing the reference counter), also
>>>>>> do it for the driver module of the iio dev. This will avoid the situation
>>>>>> where the producer IIO device can be removed and the consumer is still
>>>>>> present in the kernel.
>>>>>>
>>>>>> Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
>>>>>> ---
>>>>>>   drivers/iio/inkern.c | 8 +++++++-
>>>>>>   1 file changed, 7 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
>>>>>> index ec98790..68d9b87 100644
>>>>>> --- a/drivers/iio/inkern.c
>>>>>> +++ b/drivers/iio/inkern.c
>>>>>> @@ -11,6 +11,7 @@
>>>>>>   #include <linux/slab.h>
>>>>>>   #include <linux/mutex.h>
>>>>>>   #include <linux/of.h>
>>>>>> +#include <linux/module.h>
>>>>>>   #include <linux/iio/iio.h>
>>>>>>   #include "iio_core.h"
>>>>>> @@ -152,6 +153,7 @@ static int __of_iio_channel_get(struct iio_channel *channel,
>>>>>>   	if (index < 0)
>>>>>>   		goto err_put;
>>>>>>   	channel->channel = &indio_dev->channels[index];
>>>>>> +	try_module_get(channel->indio_dev->driver_module);  
>>>>>
>>>>> And if it fails? (the module we are trying to get is going away...)
>>>>>
>>>>> We should try and handle it I think. Be it by just erroring out of here.  
>>>>
>>>> Even more, this has nothing to do with modules. A device can go away for
>>>> any number of reasons (we unbind it manually via sysfs, we pull the USB
>>>> plug from the host in case it is USB-connected device, we unload I2C
>>>> adapter for the bus device resides on, we kick underlying PCI device)
>>>> and we should be able to handle this in some fashion. Handling errors
>>>> from reads and ignoring garbage is one of methods.
>>>>
>>>> FWIW this is a NACK from me.
>>>>
>>>> Thanks.  
>>> Hello,
>>>
>>> This patch is actually a "best effort attempt" for the consumer driver
>>> (touch driver) to get a reference to the producer of the data (the IIO
>>> device), when it requests the specific channels.
>>> As of this moment, there is no attempt whatsoever for the consumer to have a
>>> reference on the producer driver. Thus, the producer can be removed at any
>>> time, and the consumer will fail ungraciously.  
>>
>> This is the root of the issue. The consumer should be prepared to handle
>> errors from producer.
>>
>>> I can change the perspective from "best effort" to "mandatory" to get a
>>> reference to the producer, or you wish to stop trying to get any reference
>>> at all (remove this patch completely) ?  
>>
>> You should take reference to the device itself (if it is not taken
>> already), so it does not disappear completely and you can continue using
>> IIO API to access it, and IIO API should be prepared to deal with "dead"
>> devices, but as I pointed in my other email, trying to pin the driver
>> is quite pointless as there are myriad other ways of device stopping
>> working besides module unloading.
>>
>> In any case, I think this problem is outside of the scope of this
>> patchset that adds a generic resistive touchscreen, so if you want to
>> continue working on this I'd recommend moving it into a separate series.
>>
>> Thanks.
>>
> Agreed, this one has come up a number of times before.  Quite a lot of
> work got done by (IIRC) Lars Peter Clausen to stabilize things in various
> unexpected 'going away' events.  Of course there may be paths we have
> added since that (it was years ago) that can cause trouble...
> 
> Anyhow, separate issue as Dmitry says, let's deal with it separately.

We do properly get a ref to the device. Not getting a ref to the module is
on purpose. Cause as Dmitry said it does nothing the device can disappear at
any point either way. I second the NACK.

^ permalink raw reply

* Re: [PATCH] Input: alps - Demystify trackstick initialization for v3 and v6 protocols
From: Pali Rohár @ 2018-04-18 11:40 UTC (permalink / raw)
  To: Dmitry Torokhov
  Cc: linux-input@vger.kernel.org, linux-kernel@vger.kernel.org,
	Masaki Ota
In-Reply-To: <20180321164126.fr4srexf5pxz4pud@pali>

Dmitry, ping

On Wednesday 21 March 2018 17:41:26 Pali Rohár wrote:
> That is pity, but OK.
> 
> Anyway, as wrote patch which I sent in the first email matches this
> documentation.
> 
> Dmitry, can you review/comment/accept/reject this patch?
> 
> On Monday 19 March 2018 08:41:19 Masaki Ota wrote:
> > Hi, Pali,
> > 
> > v3/v6 devices are T3 type, and it can use only Method 2.
> > P36 (At this time GLIDEPOINT_T3 uses the Method 2 ....)
> > T3 has the potential that uses Method2, but I think it needs to change Firmware.
> > 
> > Best Regards,
> > Masaki Ota
> > -----Original Message-----
> > From: Pali Rohár [mailto:pali.rohar@gmail.com] 
> > Sent: Friday, March 16, 2018 7:58 PM
> > To: 太田 真喜 Masaki Ota <masaki.ota@jp.alps.com>
> > Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>; linux-input@vger.kernel.org; linux-kernel@vger.kernel.org
> > Subject: Re: [PATCH] Input: alps - Demystify trackstick initialization for v3 and v6 protocols
> > 
> > Great, thank you. Enabling that RAW/extended/SP4 mode is done by sequence E6, E6, E6, F3, C8, F3, 14 (written in page 35) and it matches what function alps_trackstick_enter_extended_mode_v3_v6() in my patch is doing. So is correct.
> > 
> > On page 36 I see that there is described Method 1 for reporting stick data which prevents cursor jumps. Seems that kernel uses Method 2.
> > Method 1 depends on some prioritization.
> > 
> > Do you have some information how to activate Method 1? Sometimes I observe that problem with "cursor jumps" and from Method 1 could prevent it. So I would like to try experimenting...
> > 
> > On Wednesday 14 March 2018 23:56:46 Masaki Ota wrote:
> > > Hi, Pali,
> > > 
> > > I have added Appendix.
> > > According to this spec documents, SP raw mode is SP 4 byte mode.
> > > I think Extended mode meaning is almost all the same as Raw mode.
> > > The description of how to set is written in Page 35.
> > > 
> > > Best Regards,
> > > Masaki Ota
> > > -----Original Message-----
> > > From: Pali Rohár [mailto:pali.rohar@gmail.com]
> > > Sent: Thursday, March 15, 2018 7:58 AM
> > > To: 太田 真喜 Masaki Ota <masaki.ota@jp.alps.com>
> > > Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>; 
> > > linux-input@vger.kernel.org; linux-kernel@vger.kernel.org
> > > Subject: Re: [PATCH] Input: alps - Demystify trackstick initialization 
> > > for v3 and v6 protocols
> > > 
> > > Hi! Thank you for information.
> > > 
> > > Your PS/2 Aux Port Control description seems to really matches. Just there is reverse order of bits. Bit 0 in description is highest, therefore matches BIT(7) macro.
> > > 
> > > Bit 6 in description (BIT(1) in code) describes SP Extended Mode which alps.c enabled. And in that description is written:
> > > 
> > > "If 1 SP is extended packet format (driver must set SP raw mode and GP absolute mode)."
> > > 
> > > Do you have any idea what "SP raw mode" is? How to set it? For me it looks like it could be that extended mode of trackstick itself.
> > > 
> > > "GP absolute mode" I guess is GlidePoint absolute mode, therefore 
> > > enable
> > > 6 byte absolute mode for touchpad.
> > > 
> > > And for Bit 7 (BIT(0)) is written:
> > > 
> > > This bit is used with the PS/2 Aux port to use the Pass-Thru mode ( see appendix A ). Do you have some information about this appendix A?
> > > 
> > > On Wednesday 14 March 2018 10:21:43 Masaki Ota wrote:
> > > > Hi, Pali,
> > > > 
> > > > I just picked up the spec which relates with trackstic.
> > > > 
> > > > Best Regards,
> > > > Masaki Ota
> > > > -----Original Message-----
> > > > From: Pali Rohár [mailto:pali.rohar@gmail.com]
> > > > Sent: Tuesday, March 13, 2018 8:14 AM
> > > > To: 太田 真喜 Masaki Ota <masaki.ota@jp.alps.com>; Dmitry Torokhov 
> > > > <dmitry.torokhov@gmail.com>
> > > > Cc: linux-input@vger.kernel.org; linux-kernel@vger.kernel.org
> > > > Subject: Re: [PATCH] Input: alps - Demystify trackstick 
> > > > initialization for v3 and v6 protocols
> > > > 
> > > > Masaki, if you have access to the internal ALPS v3 / Rushmore 
> > > > documentation, I would like to have a review of this patch or 
> > > > confirmation of those information :-)
> > > 
> > > --
> > > Pali Rohár
> > > pali.rohar@gmail.com
> > 
> > --
> > Pali Rohár
> > pali.rohar@gmail.com
> 

-- 
Pali Rohár
pali.rohar@gmail.com

^ permalink raw reply

* Re: [PATCH v3 06/11] iio: inkern: add module put/get on iio dev module when requesting channels
From: Jonathan Cameron @ 2018-04-18  9:35 UTC (permalink / raw)
  To: Dmitry Torokhov
  Cc: devicetree, alexandre.belloni, lars, robh, linux-iio,
	linux-kernel, ludovic.desroches, Jonathan Cameron, linux-input,
	Eugen Hristev, linux-arm-kernel
In-Reply-To: <20180417191906.GB67621@dtor-ws>

On Tue, 17 Apr 2018 12:19:06 -0700
Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:

> Hi Eugen,
> 
> On Tue, Apr 17, 2018 at 10:39:24AM +0300, Eugen Hristev wrote:
> > 
> > 
> > On 17.04.2018 02:58, Dmitry Torokhov wrote:  
> > > On Sun, Apr 15, 2018 at 08:33:21PM +0100, Jonathan Cameron wrote:  
> > > > On Tue, 10 Apr 2018 11:57:52 +0300
> > > > Eugen Hristev <eugen.hristev@microchip.com> wrote:
> > > >   
> > > > > When requesting channels for a particular consumer device,
> > > > > besides requesting the device (incrementing the reference counter), also
> > > > > do it for the driver module of the iio dev. This will avoid the situation
> > > > > where the producer IIO device can be removed and the consumer is still
> > > > > present in the kernel.
> > > > > 
> > > > > Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
> > > > > ---
> > > > >   drivers/iio/inkern.c | 8 +++++++-
> > > > >   1 file changed, 7 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
> > > > > index ec98790..68d9b87 100644
> > > > > --- a/drivers/iio/inkern.c
> > > > > +++ b/drivers/iio/inkern.c
> > > > > @@ -11,6 +11,7 @@
> > > > >   #include <linux/slab.h>
> > > > >   #include <linux/mutex.h>
> > > > >   #include <linux/of.h>
> > > > > +#include <linux/module.h>
> > > > >   #include <linux/iio/iio.h>
> > > > >   #include "iio_core.h"
> > > > > @@ -152,6 +153,7 @@ static int __of_iio_channel_get(struct iio_channel *channel,
> > > > >   	if (index < 0)
> > > > >   		goto err_put;
> > > > >   	channel->channel = &indio_dev->channels[index];
> > > > > +	try_module_get(channel->indio_dev->driver_module);  
> > > > 
> > > > And if it fails? (the module we are trying to get is going away...)
> > > > 
> > > > We should try and handle it I think. Be it by just erroring out of here.  
> > > 
> > > Even more, this has nothing to do with modules. A device can go away for
> > > any number of reasons (we unbind it manually via sysfs, we pull the USB
> > > plug from the host in case it is USB-connected device, we unload I2C
> > > adapter for the bus device resides on, we kick underlying PCI device)
> > > and we should be able to handle this in some fashion. Handling errors
> > > from reads and ignoring garbage is one of methods.
> > > 
> > > FWIW this is a NACK from me.
> > > 
> > > Thanks.  
> > Hello,
> > 
> > This patch is actually a "best effort attempt" for the consumer driver
> > (touch driver) to get a reference to the producer of the data (the IIO
> > device), when it requests the specific channels.
> > As of this moment, there is no attempt whatsoever for the consumer to have a
> > reference on the producer driver. Thus, the producer can be removed at any
> > time, and the consumer will fail ungraciously.  
> 
> This is the root of the issue. The consumer should be prepared to handle
> errors from producer.
> 
> > I can change the perspective from "best effort" to "mandatory" to get a
> > reference to the producer, or you wish to stop trying to get any reference
> > at all (remove this patch completely) ?  
> 
> You should take reference to the device itself (if it is not taken
> already), so it does not disappear completely and you can continue using
> IIO API to access it, and IIO API should be prepared to deal with "dead"
> devices, but as I pointed in my other email, trying to pin the driver
> is quite pointless as there are myriad other ways of device stopping
> working besides module unloading.
> 
> In any case, I think this problem is outside of the scope of this
> patchset that adds a generic resistive touchscreen, so if you want to
> continue working on this I'd recommend moving it into a separate series.
> 
> Thanks.
> 
Agreed, this one has come up a number of times before.  Quite a lot of
work got done by (IIRC) Lars Peter Clausen to stabilize things in various
unexpected 'going away' events.  Of course there may be paths we have
added since that (it was years ago) that can cause trouble...

Anyhow, separate issue as Dmitry says, let's deal with it separately.

Jonathan

^ permalink raw reply

* Re: [PATCH v3 03/11] iio: Add channel for Position Relative
From: Jonathan Cameron @ 2018-04-18  9:32 UTC (permalink / raw)
  To: Eugen Hristev
  Cc: Jonathan Cameron, ludovic.desroches, alexandre.belloni,
	linux-arm-kernel, devicetree, linux-kernel, linux-iio,
	linux-input, nicolas.ferre, dmitry.torokhov, robh
In-Reply-To: <02533699-a2bc-7c0c-4d8a-c10bb0102667@microchip.com>

On Tue, 17 Apr 2018 10:30:46 +0300
Eugen Hristev <eugen.hristev@microchip.com> wrote:

> On 15.04.2018 22:29, Jonathan Cameron wrote:
> > On Tue, 10 Apr 2018 11:57:49 +0300
> > Eugen Hristev <eugen.hristev@microchip.com> wrote:
> >   
> >> Add new channel type for relative position on a pad.
> >>
> >> These type of analog sensor offers the position of a pen
> >> on a touchpad, and is represented as a voltage, which can be
> >> converted to a position on X and Y axis on the pad.
> >> The channel will hand the relative position on the pad in both directions.
> >>
> >> The channel can then be consumed by a touchscreen driver or
> >> read as-is for a raw indication of the touchpen on a touchpad.
> >>
> >> Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
> >> ---
> >> Changes in v2:
> >>   - modified channel name to relative position as suggested.
> >>   - modified kernel version to 4.18 (presumable)
> >>
> >>   Documentation/ABI/testing/sysfs-bus-iio | 12 ++++++++++++
> >>   drivers/iio/industrialio-core.c         |  1 +
> >>   include/uapi/linux/iio/types.h          |  1 +
> >>   tools/iio/iio_event_monitor.c           |  2 ++
> >>   4 files changed, 16 insertions(+)
> >>
> >> diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
> >> index 6a5f34b..42a9287 100644
> >> --- a/Documentation/ABI/testing/sysfs-bus-iio
> >> +++ b/Documentation/ABI/testing/sysfs-bus-iio
> >> @@ -190,6 +190,18 @@ Description:
> >>   		but should match other such assignments on device).
> >>   		Units after application of scale and offset are m/s^2.
> >>   
> >> +What:		/sys/bus/iio/devices/iio:deviceX/in_positionrelative_x_raw
> >> +What:		/sys/bus/iio/devices/iio:deviceX/in_positionrelative_y_raw
> >> +KernelVersion:	4.18
> >> +Contact:	linux-iio@vger.kernel.org
> >> +Description:
> >> +		Relative position in direction x or y on a pad (may be
> >> +		arbitrarily assigned but should match other such assignments on
> >> +		device).
> >> +		Units after application of scale and offset are milli percents
> >> +		from the pad's size in both directions. Should be calibrated by
> >> +		the consumer.  
> > I know the milli percent comes form the humidity equivalent, but I wonder
> > if we are right to follow that.  10^-5 is a pretty random base unit (though
> > I got argued into it being a standard choice for humidity sensors IIRC...
> > 
> > What do people think?  We could go with 1 for full range or just percent perhaps?
> > 
> > I'm not that fussed about staying consistent with humidity - we are unlikely
> > to end up with sensors doing both anytime soon so there shouldn't be
> > any confusion...
> > 
> > Jonathan  
> Hello Jonathan,
> 
> For my specific use case, percents are not enough, as this would mean 
> the resolution will be pretty low. If we have a touchpanel of a higher 
> resolution, having the position only ranged 0 to 100 is pretty bad.
> Having millipercents means we can have a resolution up to 100,000 points 
> in range, which is fine for the hardware as of this moment.
> Of course, specific drivers reporting these channels can define a 
> specific range if they desire (report just in the first percent if you 
> wish (0-1000) or so, which would mean a lower resolution.
> Centi-percent would work fine as well I believe (0-10,000 range), but to 
> keep consistent with your suggestion regarding relative humidity, I 
> picked this larger scale.
> In my specific case, since we have a 12 bit ADC, range is 0-4096 for 
> this value.
> Calibrating these values to the actual size of the touchpanel is left 
> for the consumer to do (map 4096x4096 to the actual resolution)
> 
> If you have a better idea than milli-percents I can change it, no problem.
> 

It doesn't actually matter what the defined 'base units' are from a question
of what we can represent.  That is just handled by suppling an appropriate
SCALE for the channel.  All this is about is trying to provide something
consistent rather than having any effect on the actual range.  Will let
this sit a while longer...

Jonathan

> Eugen
> >> +
> >>   [...]  
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH v6 28/30] drm/rockchip: Disable PSR from reboot notifier
From: Enric Balletbo Serra @ 2018-04-18  9:18 UTC (permalink / raw)
  To: Tomasz Figa
  Cc: David Airlie, dri-devel, Linux Kernel Mailing List,
	Thierry Reding, Laurent Pinchart, Yakir Yang, kernel,
	Marek Szyprowski, linux-samsung-soc, rydberg, Krzysztof Kozlowski,
	open list:ARM/Rockchip SoC..., Kukjin Kim, linux-input,
	Ørjan Eide, Caesar Wang, Jeffy,
	list@263.net:IOMMU DRIVERS <iommu@lists.linux-foundation.org>, Joerg Roedel <joro@8bytes.org>, ,
	Mark yao
In-Reply-To: <CAAFQd5C_RhqSCfEXNPQ0zH-O1PGk6xGEdvzsSxnfz9kJ5G4Ceg@mail.gmail.com>

Hi Andrzej, Tomasz

2018-04-16 15:12 GMT+02:00 Tomasz Figa <tfiga@chromium.org>:
> Hi Andrzej,
>
> On Mon, Apr 16, 2018 at 6:57 PM Andrzej Hajda <a.hajda@samsung.com> wrote:
>
>> On 05.04.2018 11:49, Enric Balletbo i Serra wrote:
>> > From: Tomasz Figa <tfiga@chromium.org>
>> >
>> > It looks like the driver subsystem detaches devices from power domains
>> > at shutdown without consent of the drivers.
>
>> It looks bit strange. Could you elaborate more on it. Could you show the
>> code performing the detach?
>
> It not only looks strange, but it is strange. The code was present in 4.4:
>
> https://elixir.bootlin.com/linux/v4.4.128/source/drivers/base/platform.c#L553
>
> but was apparently removed in 4.5:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/drivers/base/platform.c?h=next-20180416&id=2d30bb0b3889adf09b342722b2ce596c0763bc93
>
> So we might not need this patch anymore.
>

Right, seems that we don't need this patch anymore, I'll do more few
tests and likely remove this patch from this series. Thanks for
catching this.

Best regards,
  Enric

> Best regards,
> Tomasz
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply

* [PATCH v2] HID: i2c-hid: Fix resume issue on Raydium touchscreen device
From: Aaron Ma @ 2018-04-18  8:27 UTC (permalink / raw)
  To: aaron.ma, dan.carpenter, jikos, benjamin.tissoires, linux-input,
	linux-kernel
In-Reply-To: <nycvar.YFH.7.76.1804121506360.28129@cbobk.fhfr.pm>

When Rayd touchscreen resumed from S3, it issues too many errors like:
i2c_hid i2c-RAYD0001:00: i2c_hid_get_input: incomplete report (58/5442)

And all the report data are corrupted, touchscreen is unresponsive.

Fix this by re-sending report description command after resume.
Add device ID as a quirk.

V2:
The V1 patch leads to the following static checker warning:

	drivers/hid/i2c-hid/i2c-hid.c:1244 i2c_hid_resume()
	info: return a literal instead of 'ret'

This test was inverted in V1 patch.
Change the condition of condition.

Cc: stable@vger.kernel.org
Cc: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Aaron Ma <aaron.ma@canonical.com>
---
 drivers/hid/hid-ids.h         |  3 +++
 drivers/hid/i2c-hid/i2c-hid.c | 13 +++++++++++++
 2 files changed, 16 insertions(+)

diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index 5a3a7ead3012..0b5cc910f62e 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -525,6 +525,9 @@
 #define I2C_VENDOR_ID_HANTICK		0x0911
 #define I2C_PRODUCT_ID_HANTICK_5288	0x5288
 
+#define I2C_VENDOR_ID_RAYD		0x2386
+#define I2C_PRODUCT_ID_RAYD_3118	0x3118
+
 #define USB_VENDOR_ID_HANWANG		0x0b57
 #define USB_DEVICE_ID_HANWANG_TABLET_FIRST	0x5000
 #define USB_DEVICE_ID_HANWANG_TABLET_LAST	0x8fff
diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
index 97689e98e53f..963328674e93 100644
--- a/drivers/hid/i2c-hid/i2c-hid.c
+++ b/drivers/hid/i2c-hid/i2c-hid.c
@@ -47,6 +47,7 @@
 /* quirks to control the device */
 #define I2C_HID_QUIRK_SET_PWR_WAKEUP_DEV	BIT(0)
 #define I2C_HID_QUIRK_NO_IRQ_AFTER_RESET	BIT(1)
+#define I2C_HID_QUIRK_RESEND_REPORT_DESCR	BIT(2)
 
 /* flags */
 #define I2C_HID_STARTED		0
@@ -171,6 +172,8 @@ static const struct i2c_hid_quirks {
 		I2C_HID_QUIRK_SET_PWR_WAKEUP_DEV },
 	{ I2C_VENDOR_ID_HANTICK, I2C_PRODUCT_ID_HANTICK_5288,
 		I2C_HID_QUIRK_NO_IRQ_AFTER_RESET },
+	{ I2C_VENDOR_ID_RAYD, I2C_PRODUCT_ID_RAYD_3118,
+		I2C_HID_QUIRK_RESEND_REPORT_DESCR },
 	{ 0, 0 }
 };
 
@@ -1220,6 +1223,16 @@ static int i2c_hid_resume(struct device *dev)
 	if (ret)
 		return ret;
 
+	/* RAYDIUM device (2386:3118) need to re-send report descr cmd
+	 * after resume, after this it will be back normal.
+	 * otherwise it issues too many incomplete reports.
+	 */
+	if (ihid->quirks & I2C_HID_QUIRK_RESEND_REPORT_DESCR) {
+		ret = i2c_hid_command(client, &hid_report_descr_cmd, NULL, 0);
+		if (ret)
+			return ret;
+	}
+
 	if (hid->driver && hid->driver->reset_resume) {
 		ret = hid->driver->reset_resume(hid);
 		return ret;
-- 
2.14.3

^ permalink raw reply related

* Re: atmel_mxt_ts on droid4: powersave seems to break the driver
From: Pavel Machek @ 2018-04-17 19:39 UTC (permalink / raw)
  To: Dmitry Torokhov
  Cc: mutt, kernel list, linux-arm-kernel, linux-omap, tony, sre,
	nekit1000, mpartap, merlijn, nick, rydberg, linux-input, bleung
In-Reply-To: <20180417182021.GA67621@dtor-ws>

[-- Attachment #1: Type: text/plain, Size: 1394 bytes --]

Hi!

> > v4.17-rc1 on motorola droid 4.
> > 
> > If I disable/reenable touschreen with xinput, it fails, with this in
> > the logs:
> > 
> > [ 1633.749450] cpcap-usb-phy cpcap-usb-phy.0: connected to USB host
> > [ 1655.938751] atmel_mxt_ts 1-004a: __mxt_read_reg: i2c transfer
> > failed (-121)
> > [ 1655.945800] atmel_mxt_ts 1-004a: Failed to read T44 and T5 (-121)
> > [ 1663.829498] cpcap-usb-phy cpcap-usb-phy.0: connected to USB host
> > 
> > Unfortunately, xscreensaver seems to break it in a similar way.
> > 
> 
> There were a few changes to the Atmel driver, ending with commit
> 96a938aa214e965d5b4a2f10443b29cad14289b9, but they mostly dealt with
> removal of platform data and moving to generic device properties...
> Can you try bisecting this series:
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/bleung/chrome-platform.git ib-chrome-platform-input-atmel-mx-ts-platform-removal
> 

To avoid any misunderstanding, I'm not saying this is
regression from v4.16. I did some testing on v4.16 (and it was first
kernel to work there at all), but I did not have good enough userland
to play with touchscreen disable/enable.

Plus, I'm not sure this is 100% reproducible, let me play some more.

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

^ permalink raw reply

* Re: [PATCH v3 06/11] iio: inkern: add module put/get on iio dev module when requesting channels
From: Dmitry Torokhov @ 2018-04-17 19:19 UTC (permalink / raw)
  To: Eugen Hristev
  Cc: Jonathan Cameron, ludovic.desroches, alexandre.belloni,
	linux-arm-kernel, devicetree, linux-kernel, linux-iio,
	linux-input, nicolas.ferre, robh
In-Reply-To: <67771f4c-d30c-4a28-2a9b-d5585186d60a@microchip.com>

Hi Eugen,

On Tue, Apr 17, 2018 at 10:39:24AM +0300, Eugen Hristev wrote:
> 
> 
> On 17.04.2018 02:58, Dmitry Torokhov wrote:
> > On Sun, Apr 15, 2018 at 08:33:21PM +0100, Jonathan Cameron wrote:
> > > On Tue, 10 Apr 2018 11:57:52 +0300
> > > Eugen Hristev <eugen.hristev@microchip.com> wrote:
> > > 
> > > > When requesting channels for a particular consumer device,
> > > > besides requesting the device (incrementing the reference counter), also
> > > > do it for the driver module of the iio dev. This will avoid the situation
> > > > where the producer IIO device can be removed and the consumer is still
> > > > present in the kernel.
> > > > 
> > > > Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
> > > > ---
> > > >   drivers/iio/inkern.c | 8 +++++++-
> > > >   1 file changed, 7 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
> > > > index ec98790..68d9b87 100644
> > > > --- a/drivers/iio/inkern.c
> > > > +++ b/drivers/iio/inkern.c
> > > > @@ -11,6 +11,7 @@
> > > >   #include <linux/slab.h>
> > > >   #include <linux/mutex.h>
> > > >   #include <linux/of.h>
> > > > +#include <linux/module.h>
> > > >   #include <linux/iio/iio.h>
> > > >   #include "iio_core.h"
> > > > @@ -152,6 +153,7 @@ static int __of_iio_channel_get(struct iio_channel *channel,
> > > >   	if (index < 0)
> > > >   		goto err_put;
> > > >   	channel->channel = &indio_dev->channels[index];
> > > > +	try_module_get(channel->indio_dev->driver_module);
> > > 
> > > And if it fails? (the module we are trying to get is going away...)
> > > 
> > > We should try and handle it I think. Be it by just erroring out of here.
> > 
> > Even more, this has nothing to do with modules. A device can go away for
> > any number of reasons (we unbind it manually via sysfs, we pull the USB
> > plug from the host in case it is USB-connected device, we unload I2C
> > adapter for the bus device resides on, we kick underlying PCI device)
> > and we should be able to handle this in some fashion. Handling errors
> > from reads and ignoring garbage is one of methods.
> > 
> > FWIW this is a NACK from me.
> > 
> > Thanks.
> Hello,
> 
> This patch is actually a "best effort attempt" for the consumer driver
> (touch driver) to get a reference to the producer of the data (the IIO
> device), when it requests the specific channels.
> As of this moment, there is no attempt whatsoever for the consumer to have a
> reference on the producer driver. Thus, the producer can be removed at any
> time, and the consumer will fail ungraciously.

This is the root of the issue. The consumer should be prepared to handle
errors from producer.

> I can change the perspective from "best effort" to "mandatory" to get a
> reference to the producer, or you wish to stop trying to get any reference
> at all (remove this patch completely) ?

You should take reference to the device itself (if it is not taken
already), so it does not disappear completely and you can continue using
IIO API to access it, and IIO API should be prepared to deal with "dead"
devices, but as I pointed in my other email, trying to pin the driver
is quite pointless as there are myriad other ways of device stopping
working besides module unloading.

In any case, I think this problem is outside of the scope of this
patchset that adds a generic resistive touchscreen, so if you want to
continue working on this I'd recommend moving it into a separate series.

Thanks.

-- 
Dmitry

^ permalink raw reply

* Re: atmel_mxt_ts on droid4: powersave seems to break the driver
From: Dmitry Torokhov @ 2018-04-17 18:20 UTC (permalink / raw)
  To: Pavel Machek
  Cc: mutt, kernel list, linux-arm-kernel, linux-omap, tony, sre,
	nekit1000, mpartap, merlijn, nick, rydberg, linux-input, bleung
In-Reply-To: <20180417073811.GA9450@amd>

Hi Pavel,

On Tue, Apr 17, 2018 at 09:38:11AM +0200, Pavel Machek wrote:
> HI!
> 
> v4.17-rc1 on motorola droid 4.
> 
> If I disable/reenable touschreen with xinput, it fails, with this in
> the logs:
> 
> [ 1633.749450] cpcap-usb-phy cpcap-usb-phy.0: connected to USB host
> [ 1655.938751] atmel_mxt_ts 1-004a: __mxt_read_reg: i2c transfer
> failed (-121)
> [ 1655.945800] atmel_mxt_ts 1-004a: Failed to read T44 and T5 (-121)
> [ 1663.829498] cpcap-usb-phy cpcap-usb-phy.0: connected to USB host
> 
> Unfortunately, xscreensaver seems to break it in a similar way.
> 

There were a few changes to the Atmel driver, ending with commit
96a938aa214e965d5b4a2f10443b29cad14289b9, but they mostly dealt with
removal of platform data and moving to generic device properties...
Can you try bisecting this series:

git://git.kernel.org/pub/scm/linux/kernel/git/bleung/chrome-platform.git ib-chrome-platform-input-atmel-mx-ts-platform-removal

Thanks!

-- 
Dmitry

^ permalink raw reply

* [PATCH 2/2] HID: input: append a suffix matching the application
From: Benjamin Tissoires @ 2018-04-17 13:18 UTC (permalink / raw)
  To: Jiri Kosina
  Cc: Dmitry Torokhov, Peter Hutterer, linux-input, linux-kernel,
	Benjamin Tissoires
In-Reply-To: <20180417131833.8551-1-benjamin.tissoires@redhat.com>

Given that we create one input node per application, we should name
the input node accordingly to not lose userspace.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/hid-input.c      | 64 ++++++++++++++++++++++++++++++++++++++------
 drivers/hid/hid-multitouch.c | 30 +++++++--------------
 include/linux/hid.h          |  1 +
 3 files changed, 66 insertions(+), 29 deletions(-)

diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index 0803d5adefa7..886d81df50d4 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -1500,15 +1500,56 @@ static void report_features(struct hid_device *hid)
 		}
 }
 
-static struct hid_input *hidinput_allocate(struct hid_device *hid)
+static struct hid_input *hidinput_allocate(struct hid_device *hid,
+					   unsigned application)
 {
 	struct hid_input *hidinput = kzalloc(sizeof(*hidinput), GFP_KERNEL);
 	struct input_dev *input_dev = input_allocate_device();
-	if (!hidinput || !input_dev) {
-		kfree(hidinput);
-		input_free_device(input_dev);
-		hid_err(hid, "Out of memory during hid input probe\n");
-		return NULL;
+	const char *suffix = NULL;
+
+	if (!hidinput || !input_dev)
+		goto fail;
+
+	if ((hid->quirks & HID_QUIRK_INPUT_PER_APP) &&
+	    hid->maxapplication > 1) {
+		switch (application) {
+		case HID_GD_KEYBOARD:
+			suffix = "Keyboard";
+			break;
+		case HID_GD_KEYPAD:
+			suffix = "Keypad";
+			break;
+		case HID_GD_MOUSE:
+			suffix = "Mouse";
+			break;
+		case HID_DG_STYLUS:
+			suffix = "Pen";
+			break;
+		case HID_DG_TOUCHSCREEN:
+			suffix = "Touchscreen";
+			break;
+		case HID_DG_TOUCHPAD:
+			suffix = "Touchpad";
+			break;
+		case HID_GD_SYSTEM_CONTROL:
+			suffix = "System Control";
+			break;
+		case HID_CP_CONSUMER_CONTROL:
+			suffix = "Consumer Control";
+			break;
+		case HID_GD_WIRELESS_RADIO_CTLS:
+			suffix = "Wireless Radio Control";
+			break;
+		default:
+			break;
+		}
+	}
+
+	if (suffix) {
+		hidinput->name = kasprintf(GFP_KERNEL, "%s %s",
+					   hid->name, suffix);
+		if (!hidinput->name)
+			goto fail;
 	}
 
 	input_set_drvdata(input_dev, hid);
@@ -1518,7 +1559,7 @@ static struct hid_input *hidinput_allocate(struct hid_device *hid)
 	input_dev->setkeycode = hidinput_setkeycode;
 	input_dev->getkeycode = hidinput_getkeycode;
 
-	input_dev->name = hid->name;
+	input_dev->name = hidinput->name ? hidinput->name : hid->name;
 	input_dev->phys = hid->phys;
 	input_dev->uniq = hid->uniq;
 	input_dev->id.bustype = hid->bus;
@@ -1530,6 +1571,12 @@ static struct hid_input *hidinput_allocate(struct hid_device *hid)
 	list_add_tail(&hidinput->list, &hid->inputs);
 
 	return hidinput;
+
+fail:
+	kfree(hidinput);
+	input_free_device(input_dev);
+	hid_err(hid, "Out of memory during hid input probe\n");
+	return NULL;
 }
 
 static bool hidinput_has_been_populated(struct hid_input *hidinput)
@@ -1575,6 +1622,7 @@ static void hidinput_cleanup_hidinput(struct hid_device *hid,
 
 	list_del(&hidinput->list);
 	input_free_device(hidinput->input);
+	kfree(hidinput->name);
 
 	for (k = HID_INPUT_REPORT; k <= HID_OUTPUT_REPORT; k++) {
 		if (k == HID_OUTPUT_REPORT &&
@@ -1686,7 +1734,7 @@ int hidinput_connect(struct hid_device *hid, unsigned int force)
 				hidinput = hidinput_match_application(report);
 
 			if (!hidinput) {
-				hidinput = hidinput_allocate(hid);
+				hidinput = hidinput_allocate(hid, report->application);
 				if (!hidinput)
 					goto out_unwind;
 			}
diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 2a0caf2e24ce..cfbaedc55e02 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -1294,33 +1294,21 @@ static int mt_input_configured(struct hid_device *hdev, struct hid_input *hi)
 	} else {
 		switch (field->application) {
 		case HID_GD_KEYBOARD:
-			suffix = "Keyboard";
-			break;
 		case HID_GD_KEYPAD:
-			suffix = "Keypad";
-			break;
 		case HID_GD_MOUSE:
-			suffix = "Mouse";
-			break;
-		case HID_DG_STYLUS:
-			suffix = "Pen";
-			/* force BTN_STYLUS to allow tablet matching in udev */
-			__set_bit(BTN_STYLUS, hi->input->keybit);
-			break;
-		case HID_DG_TOUCHSCREEN:
-			/* we do not set suffix = "Touchscreen" */
-			break;
 		case HID_DG_TOUCHPAD:
-			suffix = "Touchpad";
-			break;
 		case HID_GD_SYSTEM_CONTROL:
-			suffix = "System Control";
-			break;
 		case HID_CP_CONSUMER_CONTROL:
-			suffix = "Consumer Control";
-			break;
 		case HID_GD_WIRELESS_RADIO_CTLS:
-			suffix = "Wireless Radio Control";
+			/* already handled by hid core */
+			break;
+		case HID_DG_TOUCHSCREEN:
+			/* we do not set suffix = "Touchscreen" */
+			hi->input->name = hdev->name;
+			break;
+		case HID_DG_STYLUS:
+			/* force BTN_STYLUS to allow tablet matching in udev */
+			__set_bit(BTN_STYLUS, hi->input->keybit);
 			break;
 		case HID_VD_ASUS_CUSTOM_MEDIA_KEYS:
 			suffix = "Custom Media Keys";
diff --git a/include/linux/hid.h b/include/linux/hid.h
index 8d52cefedfe5..1d1387773a7c 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -511,6 +511,7 @@ struct hid_input {
 	struct list_head list;
 	struct hid_report *report;
 	struct input_dev *input;
+	const char *name;
 	bool registered;
 };
 
-- 
2.14.3

^ permalink raw reply related

* [PATCH 1/2] HID: generic: create one input report per application type
From: Benjamin Tissoires @ 2018-04-17 13:18 UTC (permalink / raw)
  To: Jiri Kosina
  Cc: Dmitry Torokhov, Peter Hutterer, linux-input, linux-kernel,
	Benjamin Tissoires
In-Reply-To: <20180417131833.8551-1-benjamin.tissoires@redhat.com>

It is not a good idea to try to fit all types of applications in the
same input report. There are a lot of devices that are needing
the quirk HID_MULTI_INPUT but this quirk doesn't match the actual HID
description as it is based on the report ID.

Given that most devices with MULTI_INPUT I can think of split nicely
the devices inputs into application, it is a good thing to split the
devices by default based on this assumption.

Also make hid-multitouch following this rule, to not have to deal
with too many input created

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/hid-core.c       | 10 +++++++---
 drivers/hid/hid-generic.c    | 15 +++++++++++++++
 drivers/hid/hid-gfrm.c       |  2 +-
 drivers/hid/hid-input.c      | 20 +++++++++++++++++++-
 drivers/hid/hid-magicmouse.c |  6 +++---
 drivers/hid/hid-multitouch.c |  4 ++--
 include/linux/hid.h          |  5 ++++-
 7 files changed, 51 insertions(+), 11 deletions(-)

diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 5d7cc6bbbac6..ac4799abcc4d 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -57,7 +57,8 @@ MODULE_PARM_DESC(ignore_special_drivers, "Ignore any special drivers and handle
  * Register a new report for a device.
  */
 
-struct hid_report *hid_register_report(struct hid_device *device, unsigned type, unsigned id)
+struct hid_report *hid_register_report(struct hid_device *device, unsigned type,
+				       unsigned id, unsigned application)
 {
 	struct hid_report_enum *report_enum = device->report_enum + type;
 	struct hid_report *report;
@@ -78,6 +79,7 @@ struct hid_report *hid_register_report(struct hid_device *device, unsigned type,
 	report->type = type;
 	report->size = 0;
 	report->device = device;
+	report->application = application;
 	report_enum->report_id_hash[id] = report;
 
 	list_add_tail(&report->list, &report_enum->report_list);
@@ -224,8 +226,10 @@ static int hid_add_field(struct hid_parser *parser, unsigned report_type, unsign
 	unsigned usages;
 	unsigned offset;
 	unsigned i;
+	unsigned application = hid_lookup_collection(parser, HID_COLLECTION_APPLICATION);
 
-	report = hid_register_report(parser->device, report_type, parser->global.report_id);
+	report = hid_register_report(parser->device, report_type,
+				     parser->global.report_id, application);
 	if (!report) {
 		hid_err(parser->device, "hid_register_report failed\n");
 		return -1;
@@ -259,7 +263,7 @@ static int hid_add_field(struct hid_parser *parser, unsigned report_type, unsign
 
 	field->physical = hid_lookup_collection(parser, HID_COLLECTION_PHYSICAL);
 	field->logical = hid_lookup_collection(parser, HID_COLLECTION_LOGICAL);
-	field->application = hid_lookup_collection(parser, HID_COLLECTION_APPLICATION);
+	field->application = application;
 
 	for (i = 0; i < usages; i++) {
 		unsigned j = i;
diff --git a/drivers/hid/hid-generic.c b/drivers/hid/hid-generic.c
index c25b4718de44..3b6eccbc2519 100644
--- a/drivers/hid/hid-generic.c
+++ b/drivers/hid/hid-generic.c
@@ -56,6 +56,20 @@ static bool hid_generic_match(struct hid_device *hdev,
 	return true;
 }
 
+static int hid_generic_probe(struct hid_device *hdev,
+			     const struct hid_device_id *id)
+{
+	int ret;
+
+	hdev->quirks |= HID_QUIRK_INPUT_PER_APP;
+
+	ret = hid_parse(hdev);
+	if (ret)
+		return ret;
+
+	return hid_hw_start(hdev, HID_CONNECT_DEFAULT);
+}
+
 static const struct hid_device_id hid_table[] = {
 	{ HID_DEVICE(HID_BUS_ANY, HID_GROUP_ANY, HID_ANY_ID, HID_ANY_ID) },
 	{ }
@@ -66,6 +80,7 @@ static struct hid_driver hid_generic = {
 	.name = "hid-generic",
 	.id_table = hid_table,
 	.match = hid_generic_match,
+	.probe = hid_generic_probe,
 };
 module_hid_driver(hid_generic);
 
diff --git a/drivers/hid/hid-gfrm.c b/drivers/hid/hid-gfrm.c
index 075b1c020846..cf477f8c8f4c 100644
--- a/drivers/hid/hid-gfrm.c
+++ b/drivers/hid/hid-gfrm.c
@@ -116,7 +116,7 @@ static int gfrm_probe(struct hid_device *hdev, const struct hid_device_id *id)
 		 * those reports reach gfrm_raw_event() from hid_input_report().
 		 */
 		if (!hid_register_report(hdev, HID_INPUT_REPORT,
-					 GFRM100_SEARCH_KEY_REPORT_ID)) {
+					 GFRM100_SEARCH_KEY_REPORT_ID, 0)) {
 			ret = -ENOMEM;
 			goto done;
 		}
diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index 04056773102e..0803d5adefa7 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -1607,6 +1607,20 @@ static struct hid_input *hidinput_match(struct hid_report *report)
 	return NULL;
 }
 
+static struct hid_input *hidinput_match_application(struct hid_report *report)
+{
+	struct hid_device *hid = report->device;
+	struct hid_input *hidinput;
+
+	list_for_each_entry(hidinput, &hid->inputs, list) {
+		if (hidinput->report &&
+		    hidinput->report->application == report->application)
+			return hidinput;
+	}
+
+	return NULL;
+}
+
 static inline void hidinput_configure_usages(struct hid_input *hidinput,
 					     struct hid_report *report)
 {
@@ -1667,6 +1681,9 @@ int hidinput_connect(struct hid_device *hid, unsigned int force)
 			 */
 			if (hid->quirks & HID_QUIRK_MULTI_INPUT)
 				hidinput = hidinput_match(report);
+			else if (hid->maxapplication > 1 &&
+				 (hid->quirks & HID_QUIRK_INPUT_PER_APP))
+				hidinput = hidinput_match_application(report);
 
 			if (!hidinput) {
 				hidinput = hidinput_allocate(hid);
@@ -1676,7 +1693,8 @@ int hidinput_connect(struct hid_device *hid, unsigned int force)
 
 			hidinput_configure_usages(hidinput, report);
 
-			if (hid->quirks & HID_QUIRK_MULTI_INPUT)
+			if (hid->quirks &
+			    (HID_QUIRK_MULTI_INPUT | HID_QUIRK_INPUT_PER_APP))
 				hidinput->report = report;
 		}
 	}
diff --git a/drivers/hid/hid-magicmouse.c b/drivers/hid/hid-magicmouse.c
index 42ed887ba0be..b454c4386157 100644
--- a/drivers/hid/hid-magicmouse.c
+++ b/drivers/hid/hid-magicmouse.c
@@ -531,12 +531,12 @@ static int magicmouse_probe(struct hid_device *hdev,
 
 	if (id->product == USB_DEVICE_ID_APPLE_MAGICMOUSE)
 		report = hid_register_report(hdev, HID_INPUT_REPORT,
-			MOUSE_REPORT_ID);
+			MOUSE_REPORT_ID, 0);
 	else { /* USB_DEVICE_ID_APPLE_MAGICTRACKPAD */
 		report = hid_register_report(hdev, HID_INPUT_REPORT,
-			TRACKPAD_REPORT_ID);
+			TRACKPAD_REPORT_ID, 0);
 		report = hid_register_report(hdev, HID_INPUT_REPORT,
-			DOUBLE_REPORT_ID);
+			DOUBLE_REPORT_ID, 0);
 	}
 
 	if (!report) {
diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 8551766b75fa..2a0caf2e24ce 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -1458,10 +1458,10 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
 
 	/*
 	 * This allows the driver to handle different input sensors
-	 * that emits events through different reports on the same HID
+	 * that emits events through different applications on the same HID
 	 * device.
 	 */
-	hdev->quirks |= HID_QUIRK_MULTI_INPUT;
+	hdev->quirks |= HID_QUIRK_INPUT_PER_APP;
 
 	timer_setup(&td->release_timer, mt_expired_timeout, 0);
 
diff --git a/include/linux/hid.h b/include/linux/hid.h
index 0267aa5c1ea3..8d52cefedfe5 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -341,6 +341,7 @@ struct hid_item {
 /* BIT(8) reserved for backward compatibility, was HID_QUIRK_NO_EMPTY_INPUT */
 /* BIT(9) reserved for backward compatibility, was NO_INIT_INPUT_REPORTS */
 #define HID_QUIRK_ALWAYS_POLL			BIT(10)
+#define HID_QUIRK_INPUT_PER_APP			BIT(11)
 #define HID_QUIRK_SKIP_OUTPUT_REPORTS		BIT(16)
 #define HID_QUIRK_SKIP_OUTPUT_REPORT_ID		BIT(17)
 #define HID_QUIRK_NO_OUTPUT_REPORTS_ON_INTR_EP	BIT(18)
@@ -466,6 +467,7 @@ struct hid_report {
 	struct list_head list;
 	unsigned id;					/* id of this report */
 	unsigned type;					/* report type */
+	unsigned application;				/* application usage for this report */
 	struct hid_field *field[HID_MAX_FIELDS];	/* fields of the report */
 	unsigned maxfield;				/* maximum valid field index */
 	unsigned size;					/* size of the report (bits) */
@@ -859,7 +861,8 @@ 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);
+struct hid_report *hid_register_report(struct hid_device *device, unsigned type,
+				       unsigned id, unsigned application);
 int hid_parse_report(struct hid_device *hid, __u8 *start, unsigned size);
 struct hid_report *hid_validate_values(struct hid_device *hid,
 				       unsigned int type, unsigned int id,
-- 
2.14.3

^ permalink raw reply related

* Re: [PATCH v2 2/2] HID: input: do not increment usages when a duplicate is found
From: Benjamin Tissoires @ 2018-04-17 13:18 UTC (permalink / raw)
  To: Jiri Kosina
  Cc: Dmitry Torokhov, Peter Hutterer, linux-input, linux-kernel,
	Benjamin Tissoires
In-Reply-To: <CAO-hwJKQRFBP8g_o5rZ68k1XGG3DPHoeaM64gNoDeF9ngYE9rg@mail.gmail.com>

FYI, these are the two patches I mentioned earlier.

checkpatch.pl still complains about them so do not merge them right away, but
this should give you a better idea.
Also, this is the tip of my local tree, so there is a high chance it doesn't
apply cleanly on your for-next branch.

Cheers,
Benjamin

Benjamin Tissoires (2):
  HID: generic: create one input report per application type
  HID: input: append a suffix matching the application

 drivers/hid/hid-core.c       | 10 ++++--
 drivers/hid/hid-generic.c    | 15 ++++++++
 drivers/hid/hid-gfrm.c       |  2 +-
 drivers/hid/hid-input.c      | 84 +++++++++++++++++++++++++++++++++++++++-----
 drivers/hid/hid-magicmouse.c |  6 ++--
 drivers/hid/hid-multitouch.c | 34 ++++++------------
 include/linux/hid.h          |  6 +++-
 7 files changed, 117 insertions(+), 40 deletions(-)

-- 
2.14.3

^ permalink raw reply

* Re: [PATCH v2 2/2] HID: input: do not increment usages when a duplicate is found
From: Benjamin Tissoires @ 2018-04-17 12:56 UTC (permalink / raw)
  To: Jiri Kosina
  Cc: Dmitry Torokhov, Peter Hutterer, open list:HID CORE LAYER, lkml
In-Reply-To: <nycvar.YFH.7.76.1804171410360.28129@cbobk.fhfr.pm>

On Tue, Apr 17, 2018 at 2:11 PM, Jiri Kosina <jikos@kernel.org> wrote:
> On Mon, 16 Apr 2018, Dmitry Torokhov wrote:
>
>> So what is happening with this series? I think we should get it them
>> in; there is really no reason for bumping ABS_MISC till it gets into
>> ABS_MT_* range on some devices that are out there.
>>
>> FWIW
>>
>> Acked-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>
> Sorry, I somehow lost this one. Now queued for 4.18.

Thanks Jiri, and thanks Dmitry for the reminder.

Jiri, BTW, I have 2 patch locally that should mitigate the negative
impact from this patch if there is any,

While trying to merging hid-multitouch into hid-core, I came to
realize that hid-generic should probably split the HID collections by
applications. If a device has a joystick and a mouse collection, there
is no point merging these two collections together. This is the most
common use case for reusing the same ABS_X value in one device.

I'll try to post the patches today, though I have some internal deadline :(

Cheers,
Benjamin

^ permalink raw reply

* Re: [PATCH v2 2/2] HID: input: do not increment usages when a duplicate is found
From: Jiri Kosina @ 2018-04-17 12:11 UTC (permalink / raw)
  To: Dmitry Torokhov
  Cc: Peter Hutterer, Benjamin Tissoires, linux-input, linux-kernel
In-Reply-To: <20180416235156.GE77055@dtor-ws>

On Mon, 16 Apr 2018, Dmitry Torokhov wrote:

> So what is happening with this series? I think we should get it them
> in; there is really no reason for bumping ABS_MISC till it gets into
> ABS_MT_* range on some devices that are out there.
> 
> FWIW
> 
> Acked-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Sorry, I somehow lost this one. Now queued for 4.18.

Thanks,

-- 
Jiri Kosina
SUSE Labs

^ permalink raw reply

* Re: [PATCH v6] Fix modifier keys for Redragon Asura Keyboard
From: Jiri Kosina @ 2018-04-17 11:58 UTC (permalink / raw)
  To: Robert Munteanu; +Cc: Benjamin Tissoires, linux-kernel, linux-input
In-Reply-To: <20180416213824.14688-1-rombert@apache.org>

On Tue, 17 Apr 2018, Robert Munteanu wrote:

> This adds a new driver for the Redragon Asura keyboard. The Asura
> keyboard contains an error in the HID descriptor which causes all
> modifier keys to be mapped to left shift. Additionally, we suppress
> the creation of a second, not working, keyboard device.
> 
> Signed-off-by: Robert Munteanu <rombert@apache.org>

Applied, thanks.

-- 
Jiri Kosina
SUSE Labs

^ permalink raw reply

* Re: [PATCH v2 0/9] Input: support for latest Lenovo thinkpads (series 80)
From: Aaron Ma @ 2018-04-17 11:42 UTC (permalink / raw)
  To: 廖崇榮, 'Benjamin Tissoires'
  Cc: 'Oliver Haessler', 'Benjamin Berg',
	'Rob Herring', devicetree,
	'open list:HID CORE LAYER', 'lkml',
	'Dmitry Torokhov'
In-Reply-To: <00f701d3d627$bcf41de0$36dc59a0$@emc.com.tw>

Hi Dmitry and Benjamin:

Could you apply my patch too?

It add LEN0096 that Benjamin's patch doesn't include.

+	"LEN0096", /* X280 */

Regards,
Aaron

^ permalink raw reply

* Re: [PATCH v6] Fix modifier keys for Redragon Asura Keyboard
From: Benjamin Tissoires @ 2018-04-17  8:50 UTC (permalink / raw)
  To: Robert Munteanu; +Cc: Jiri Kosina, lkml, open list:HID CORE LAYER
In-Reply-To: <20180416213824.14688-1-rombert@apache.org>

On Mon, Apr 16, 2018 at 11:38 PM, Robert Munteanu <rombert@apache.org> wrote:
> This adds a new driver for the Redragon Asura keyboard. The Asura
> keyboard contains an error in the HID descriptor which causes all
> modifier keys to be mapped to left shift. Additionally, we suppress
> the creation of a second, not working, keyboard device.
>
> Signed-off-by: Robert Munteanu <rombert@apache.org>
> ---

Looks good to me now:
Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>

Thanks Robert for respinning the series!

Cheers,
Benjamin

>  drivers/hid/Kconfig        |  7 ++++
>  drivers/hid/Makefile       |  1 +
>  drivers/hid/hid-ids.h      |  1 +
>  drivers/hid/hid-redragon.c | 86 ++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 95 insertions(+)
>  create mode 100644 drivers/hid/hid-redragon.c
>
> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> index 19c499f5623d..1125e4813716 100644
> --- a/drivers/hid/Kconfig
> +++ b/drivers/hid/Kconfig
> @@ -560,6 +560,13 @@ config HID_MAYFLASH
>         Say Y here if you have HJZ Mayflash PS3 game controller adapters
>         and want to enable force feedback support.
>
> +config HID_REDRAGON
> +       tristate "Redragon keyboards"
> +       depends on HID
> +       default !EXPERT
> +       ---help---
> +    Support for Redragon keyboards that need fix-ups to work properly.
> +
>  config HID_MICROSOFT
>         tristate "Microsoft non-fully HID-compliant devices"
>         depends on HID
> diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
> index eb13b9e92d85..a36f3f40ba63 100644
> --- a/drivers/hid/Makefile
> +++ b/drivers/hid/Makefile
> @@ -84,6 +84,7 @@ hid-picolcd-$(CONFIG_DEBUG_FS)                += hid-picolcd_debugfs.o
>
>  obj-$(CONFIG_HID_PLANTRONICS)  += hid-plantronics.o
>  obj-$(CONFIG_HID_PRIMAX)       += hid-primax.o
> +obj-$(CONFIG_HID_REDRAGON)     += hid-redragon.o
>  obj-$(CONFIG_HID_RETRODE)      += hid-retrode.o
>  obj-$(CONFIG_HID_ROCCAT)       += hid-roccat.o hid-roccat-common.o \
>         hid-roccat-arvo.o hid-roccat-isku.o hid-roccat-kone.o \
> diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
> index 9454ac134ce2..41a64d0e91f9 100644
> --- a/drivers/hid/hid-ids.h
> +++ b/drivers/hid/hid-ids.h
> @@ -599,6 +599,7 @@
>  #define USB_VENDOR_ID_JESS             0x0c45
>  #define USB_DEVICE_ID_JESS_YUREX       0x1010
>  #define USB_DEVICE_ID_ASUS_MD_5112     0x5112
> +#define USB_DEVICE_ID_REDRAGON_ASURA   0x760b
>
>  #define USB_VENDOR_ID_JESS2            0x0f30
>  #define USB_DEVICE_ID_JESS2_COLOR_RUMBLE_PAD 0x0111
> diff --git a/drivers/hid/hid-redragon.c b/drivers/hid/hid-redragon.c
> new file mode 100644
> index 000000000000..daf59578bf93
> --- /dev/null
> +++ b/drivers/hid/hid-redragon.c
> @@ -0,0 +1,86 @@
> +/*
> + *  HID driver for Redragon keyboards
> + *
> + *  Copyright (c) 2017 Robert Munteanu
> + *  SPDX-License-Identifier: GPL-2.0+
> + */
> +
> +/*
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License as published by the Free
> + * Software Foundation; either version 2 of the License, or (at your option)
> + * any later version.
> + */
> +
> +#include <linux/device.h>
> +#include <linux/hid.h>
> +#include <linux/module.h>
> +
> +#include "hid-ids.h"
> +
> +
> +/*
> + * The Redragon Asura keyboard sends an incorrect HID descriptor.
> + * At byte 100 it contains
> + *
> + *   0x81, 0x00
> + *
> + * which is Input (Data, Arr, Abs), but it should be
> + *
> + *   0x81, 0x02
> + *
> + * which is Input (Data, Var, Abs), which is consistent with the way
> + * key codes are generated.
> + */
> +
> +static __u8 *redragon_report_fixup(struct hid_device *hdev, __u8 *rdesc,
> +       unsigned int *rsize)
> +{
> +       if (*rsize >= 102 && rdesc[100] == 0x81 && rdesc[101] == 0x00) {
> +               dev_info(&hdev->dev, "Fixing Redragon ASURA report descriptor.\n");
> +               rdesc[101] = 0x02;
> +       }
> +
> +       return rdesc;
> +}
> +
> +static int redragon_probe(struct hid_device *dev,
> +       const struct hid_device_id *id)
> +{
> +       int ret;
> +
> +       ret = hid_parse(dev);
> +       if (ret) {
> +               hid_err(dev, "parse failed\n");
> +               return ret;
> +       }
> +
> +       /* do not register unused input device */
> +       if (dev->maxapplication == 1)
> +               return 0;
> +
> +       ret = hid_hw_start(dev, HID_CONNECT_DEFAULT);
> +       if (ret) {
> +               hid_err(dev, "hw start failed\n");
> +               return ret;
> +       }
> +
> +       return 0;
> +}
> +static const struct hid_device_id redragon_devices[] = {
> +       {HID_USB_DEVICE(USB_VENDOR_ID_JESS, USB_DEVICE_ID_REDRAGON_ASURA)},
> +       {}
> +};
> +
> +MODULE_DEVICE_TABLE(hid, redragon_devices);
> +
> +static struct hid_driver redragon_driver = {
> +       .name = "redragon",
> +       .id_table = redragon_devices,
> +       .report_fixup = redragon_report_fixup,
> +       .probe = redragon_probe
> +};
> +
> +module_hid_driver(redragon_driver);
> +
> +MODULE_LICENSE("GPL");
> --
> 2.16.3
>

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox