* Re: [PATCH 1/1] HID: usbhid: add usb_clear_halt determination for next hid_start_in
From: Alan Stern @ 2014-08-22 18:23 UTC (permalink / raw)
To: vichy; +Cc: Jiri Kosina, linux-usb@vger.kernel.org, linux-input
In-Reply-To: <CAOVJa8HLSQ9cqEHR8srT3QikvJ6i0ZYFqmOZ7MvMpN5aULdk9A@mail.gmail.com>
On Sat, 23 Aug 2014, vichy wrote:
> from your patch, I have some questions:
> a. in Alan's version, if both HID_CLEAR_HALT and HID_RESET_PENDING are
> set, hid_reset will both "clear ep halt" and "reset devcie".
> But in original one, even HID_CLEAR_HALT and HID_RESET_PENDING are
> both set, hid_reset only do one of them.
Yes. In my patch, the clear-halt handler will turn on the
HID_RESET_PENDING bit if something goes wrong. In that case we want to
do both things.
> is there any special reason in original hid_reset to use below flow?
> if (test_bit(HID_CLEAR_HALT, &usbhid->iofl)) {
> xxxxx
> } else if (test_bit(HID_RESET_PENDING, &usbhid->iofl)) {
> xxxxxx
> }
No special reason. We probably never thought that both flags would be
set.
> b. in original hid_reset, if "Clear halt ep" or "Rest device" success
> or none of those flags raise up, it will call hid_io_error for later
> resending the urb.
No, the clear-halt part calls hid_start_in when everything works. It
doesn't call hid_io_error, because hid_start_in turns on the
HID_IN_RUNNING flag.
> Shall we need to call "hid_io_error(hid);" in the
> end if "clear halt" or "reset device" success?
In the clear-halt case, we don't have to because we call hid_start_in.
In the reset device case, we don't have to because hid_post_reset calls
hid_start_in if there are no errors.
> c. why we chose to use usb_queue_reset_device instead of usb_reset_device()?
I originally tried using usb_reset_device, and it caused a deadlock:
Unplug the HID device.
I/O error occurs. hid_io_error schedules reset_work.
The reset_work callback routine is hid_reset. It calls
usb_reset_device.
The reset fails because the device is gone. At the end of the
reset, hid_post_reset is called.
hid_post_reset returns 1 because hid_get_class_descriptor
fails.
Because the post_reset routine failed, usb_reset_device calls
usb_unbind_and_rebind_marked_interfaces.
That routine indirectly calls usbhid_disconnect.
usbhid_disconnect calls usbhid_close by way of
hid_destroy_device.
usbhid_close calls hid_cancel_delayed_stuff.
hid_cancel_delayed_stuff calls cancel_work_sync for reset_work.
So the reset_work routine tries to cancel itself synchronously while it
is running. usb_queue_reset_device was carefully written to avoid such
deadlocks.
> d. shall we "useusb_lock_device_for_reset(hid_to_usb_dev(hid),
> usbhid->intf)" or "spin_lock_irq(&usbhid->lock)" before calling
> usb_queue_reset_device?
No. usb_queue_reset_device takes care of the locking.
Alan Stern
^ permalink raw reply
* Re: [PATCH 1/1] HID: usbhid: add usb_clear_halt determination for next hid_start_in
From: vichy @ 2014-08-22 17:38 UTC (permalink / raw)
To: Alan Stern; +Cc: Jiri Kosina, linux-usb@vger.kernel.org, linux-input
In-Reply-To: <Pine.LNX.4.44L0.1408220950340.1291-100000@iolanthe.rowland.org>
hi alan and all:
> I recently posted (but did not submit) a more comprehensive solution to
> this and other related problems. For example, HID devices typically
> run at low speed or full speed. When attached through a hub to an EHCI
> controller (very common on modern systems), unplugging the device
> causes -EPIPE errors, so the driver calls usb_clear_halt and restarts
> the interrupt pipe. Of course, the same failure occurs again, so
> there's a lengthy flurry of useless error messages.
>
> This patch changes the driver so that after usb_clear_halt fails, it
> will try to do a port reset. And if that fails, the driver will be
> unbound from the device. This avoids infinite loops.
>
> What do you think?
>
> Alan Stern
>
>
>
> Index: usb-3.16/drivers/hid/usbhid/hid-core.c
> ===================================================================
> --- usb-3.16.orig/drivers/hid/usbhid/hid-core.c
> +++ usb-3.16/drivers/hid/usbhid/hid-core.c
> @@ -116,40 +116,24 @@ static void hid_reset(struct work_struct
> struct usbhid_device *usbhid =
> container_of(work, struct usbhid_device, reset_work);
> struct hid_device *hid = usbhid->hid;
> - int rc = 0;
> + int rc;
>
> if (test_bit(HID_CLEAR_HALT, &usbhid->iofl)) {
> dev_dbg(&usbhid->intf->dev, "clear halt\n");
> rc = usb_clear_halt(hid_to_usb_dev(hid), usbhid->urbin->pipe);
> clear_bit(HID_CLEAR_HALT, &usbhid->iofl);
> - hid_start_in(hid);
> - }
> -
> - else if (test_bit(HID_RESET_PENDING, &usbhid->iofl)) {
> - dev_dbg(&usbhid->intf->dev, "resetting device\n");
> - rc = usb_lock_device_for_reset(hid_to_usb_dev(hid), usbhid->intf);
> if (rc == 0) {
> - rc = usb_reset_device(hid_to_usb_dev(hid));
> - usb_unlock_device(hid_to_usb_dev(hid));
> + hid_start_in(hid);
> + } else {
> + dev_dbg(&usbhid->intf->dev,
> + "clear-halt failed: %d\n", rc);
> + set_bit(HID_RESET_PENDING, &usbhid->iofl);
> }
> - clear_bit(HID_RESET_PENDING, &usbhid->iofl);
> }
>
> - switch (rc) {
> - case 0:
> - if (!test_bit(HID_IN_RUNNING, &usbhid->iofl))
> - hid_io_error(hid);
> - break;
> - default:
> - hid_err(hid, "can't reset device, %s-%s/input%d, status %d\n",
> - hid_to_usb_dev(hid)->bus->bus_name,
> - hid_to_usb_dev(hid)->devpath,
> - usbhid->ifnum, rc);
> - /* FALLTHROUGH */
> - case -EHOSTUNREACH:
> - case -ENODEV:
> - case -EINTR:
> - break;
> + if (test_bit(HID_RESET_PENDING, &usbhid->iofl)) {
> + dev_dbg(&usbhid->intf->dev, "resetting device\n");
> + usb_queue_reset_device(usbhid->intf);
> }
> }
from your patch, I have some questions:
a. in Alan's version, if both HID_CLEAR_HALT and HID_RESET_PENDING are
set, hid_reset will both "clear ep halt" and "reset devcie".
But in original one, even HID_CLEAR_HALT and HID_RESET_PENDING are
both set, hid_reset only do one of them.
is there any special reason in original hid_reset to use below flow?
if (test_bit(HID_CLEAR_HALT, &usbhid->iofl)) {
xxxxx
} else if (test_bit(HID_RESET_PENDING, &usbhid->iofl)) {
xxxxxx
}
b. in original hid_reset, if "Clear halt ep" or "Rest device" success
or none of those flags raise up, it will call hid_io_error for later
resending the urb. Shall we need to call "hid_io_error(hid);" in the
end if "clear halt" or "reset device" success?
c. why we chose to use usb_queue_reset_device instead of usb_reset_device()?
d. shall we "useusb_lock_device_for_reset(hid_to_usb_dev(hid),
usbhid->intf)" or "spin_lock_irq(&usbhid->lock)" before calling
usb_queue_reset_device?
I append patch for explaining my questions.
Appreciate your kind help,
diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
index 256b102..aa321f9 100644
--- a/drivers/hid/usbhid/hid-core.c
+++ b/drivers/hid/usbhid/hid-core.c
@@ -116,18 +116,22 @@ static void hid_reset(struct work_struct *work)
struct usbhid_device *usbhid =
container_of(work, struct usbhid_device, reset_work);
struct hid_device *hid = usbhid->hid;
- int rc = 0;
+ int rc;
if (test_bit(HID_CLEAR_HALT, &usbhid->iofl)) {
dev_dbg(&usbhid->intf->dev, "clear halt\n");
rc = usb_clear_halt(hid_to_usb_dev(hid), usbhid->urbin->pipe);
clear_bit(HID_CLEAR_HALT, &usbhid->iofl);
- if (rc == 0)
+ if (rc == 0) {
hid_start_in(hid);
+ } else {
+ dev_dbg(&usbhid->intf->dev,
+ "clear-halt failed: %d\n", rc);
+ set_bit(HID_RESET_PENDING, &usbhid->iofl);
+ }
}
- else if (test_bit(HID_RESET_PENDING, &usbhid->iofl)) {
- dev_dbg(&usbhid->intf->dev, "resetting device\n");
+ if (test_bit(HID_RESET_PENDING, &usbhid->iofl)) {
rc = usb_lock_device_for_reset(hid_to_usb_dev(hid),
usbhid->intf);
if (rc == 0) {
rc = usb_reset_device(hid_to_usb_dev(hid));
@@ -136,22 +140,8 @@ static void hid_reset(struct work_struct *work)
clear_bit(HID_RESET_PENDING, &usbhid->iofl);
}
- switch (rc) {
- case 0:
- if (!test_bit(HID_IN_RUNNING, &usbhid->iofl))
- hid_io_error(hid);
- break;
- default:
- hid_err(hid, "can't reset device, %s-%s/input%d, status %d\n",
- hid_to_usb_dev(hid)->bus->bus_name,
- hid_to_usb_dev(hid)->devpath,
- usbhid->ifnum, rc);
- /* FALLTHROUGH */
- case -EHOSTUNREACH:
- case -ENODEV:
- case -EINTR:
- break;
- }
+ if (!test_bit(HID_IN_RUNNING, &usbhid->iofl) || (rc == 0))
+ hid_io_error(hid);
}
/* Main I/O error handler */
^ permalink raw reply related
* Re: [PATCH v2] doc: dt/bindings: input: Fix drv260x binding document
From: Felipe Balbi @ 2014-08-22 17:16 UTC (permalink / raw)
To: Dan Murphy
Cc: devicetree, linux-input, linux-kernel, dmitry.torokhov,
linux-arm-kernel
In-Reply-To: <1408727539-15996-1-git-send-email-dmurphy@ti.com>
[-- Attachment #1: Type: text/plain, Size: 2275 bytes --]
On Fri, Aug 22, 2014 at 12:12:19PM -0500, Dan Murphy wrote:
> Update the drv260x dt binding document:
> - Change the node name to the devices function not the
> device name.
> - Add vbat-supply to the example.
> - Fix indentation of the example.
>
> Signed-off-by: Dan Murphy <dmurphy@ti.com>
Reviewed-by: Felipe Balbi <balbi@ti.com>
> ---
>
> v2 - Address comments on indentation, node declaration, add vbat-supply,
> removed description as it did not make sense - https://patchwork.kernel.org/patch/4765141/
>
> .../devicetree/bindings/input/ti,drv260x.txt | 23 ++++++++++----------
> 1 file changed, 11 insertions(+), 12 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/input/ti,drv260x.txt b/Documentation/devicetree/bindings/input/ti,drv260x.txt
> index a9c8519..ee09c8f 100644
> --- a/Documentation/devicetree/bindings/input/ti,drv260x.txt
> +++ b/Documentation/devicetree/bindings/input/ti,drv260x.txt
> @@ -1,6 +1,4 @@
> -Texas Instruments - drv260x Haptics driver family
> -
> -The drv260x family serial control bus communicates through I2C protocols
> +* Texas Instruments - drv260x Haptics driver family
>
> Required properties:
> - compatible - One of:
> @@ -37,15 +35,16 @@ Optional properties:
> 3.2 v.
> Example:
>
> -drv2605l: drv2605l@5a {
> - compatible = "ti,drv2605l";
> - reg = <0x5a>;
> - enable-gpio = <&gpio1 28 GPIO_ACTIVE_HIGH>;
> - mode = <DRV260X_LRA_MODE>;
> - library-sel = <DRV260X_LIB_LRA>;
> - vib-rated-mv = <3200>;
> - vib-overdriver-mv = <3200>;
> -};
> +haptics: haptics@5a {
> + compatible = "ti,drv2605l";
> + reg = <0x5a>;
> + vbat-supply = <&vbat>;
> + enable-gpio = <&gpio1 28 GPIO_ACTIVE_HIGH>;
> + mode = <DRV260X_LRA_MODE>;
> + library-sel = <DRV260X_LIB_LRA>;
> + vib-rated-mv = <3200>;
> + vib-overdriver-mv = <3200>;
> +}
>
> For more product information please see the link below:
> http://www.ti.com/product/drv2605
> --
> 1.7.9.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
--
balbi
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* [PATCH v2] doc: dt/bindings: input: Fix drv260x binding document
From: Dan Murphy @ 2014-08-22 17:12 UTC (permalink / raw)
To: devicetree, linux-input
Cc: linux-kernel, dmitry.torokhov, linux-arm-kernel, Dan Murphy
Update the drv260x dt binding document:
- Change the node name to the devices function not the
device name.
- Add vbat-supply to the example.
- Fix indentation of the example.
Signed-off-by: Dan Murphy <dmurphy@ti.com>
---
v2 - Address comments on indentation, node declaration, add vbat-supply,
removed description as it did not make sense - https://patchwork.kernel.org/patch/4765141/
.../devicetree/bindings/input/ti,drv260x.txt | 23 ++++++++++----------
1 file changed, 11 insertions(+), 12 deletions(-)
diff --git a/Documentation/devicetree/bindings/input/ti,drv260x.txt b/Documentation/devicetree/bindings/input/ti,drv260x.txt
index a9c8519..ee09c8f 100644
--- a/Documentation/devicetree/bindings/input/ti,drv260x.txt
+++ b/Documentation/devicetree/bindings/input/ti,drv260x.txt
@@ -1,6 +1,4 @@
-Texas Instruments - drv260x Haptics driver family
-
-The drv260x family serial control bus communicates through I2C protocols
+* Texas Instruments - drv260x Haptics driver family
Required properties:
- compatible - One of:
@@ -37,15 +35,16 @@ Optional properties:
3.2 v.
Example:
-drv2605l: drv2605l@5a {
- compatible = "ti,drv2605l";
- reg = <0x5a>;
- enable-gpio = <&gpio1 28 GPIO_ACTIVE_HIGH>;
- mode = <DRV260X_LRA_MODE>;
- library-sel = <DRV260X_LIB_LRA>;
- vib-rated-mv = <3200>;
- vib-overdriver-mv = <3200>;
-};
+haptics: haptics@5a {
+ compatible = "ti,drv2605l";
+ reg = <0x5a>;
+ vbat-supply = <&vbat>;
+ enable-gpio = <&gpio1 28 GPIO_ACTIVE_HIGH>;
+ mode = <DRV260X_LRA_MODE>;
+ library-sel = <DRV260X_LIB_LRA>;
+ vib-rated-mv = <3200>;
+ vib-overdriver-mv = <3200>;
+}
For more product information please see the link below:
http://www.ti.com/product/drv2605
--
1.7.9.5
^ permalink raw reply related
* Re: [PATCH] doc: dt/bindings: input: Fix drv260x binding document
From: Murphy, Dan @ 2014-08-22 14:49 UTC (permalink / raw)
To: Balbi, Felipe
Cc: devicetree@vger.kernel.org, linux-input@vger.kernel.org,
linux-kernel@vger.kernel.org, dmitry.torokhov@gmail.com,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <20140822144524.GE22707@saruman.home>
Felipe
On 08/22/2014 09:45 AM, Balbi, Felipe wrote:
> Hi,
>
> On Fri, Aug 22, 2014 at 09:25:13AM -0500, Murphy, Dan wrote:
>> Felipe
>>
>> On 08/22/2014 09:15 AM, Balbi, Felipe wrote:
>>> On Fri, Aug 22, 2014 at 09:12:21AM -0500, Dan Murphy wrote:
>>>> Update the drv260x dt binding document:
>>>> - Change the node name to the devices function not the
>>>> device name.
>>>> - Add vbat-supply to the example.
>>>> - Fix indentation of the example.
>>>>
>>>> Signed-off-by: Dan Murphy <dmurphy@ti.com>
>>>> ---
>>>> .../devicetree/bindings/input/ti,drv260x.txt | 19 ++++++++++---------
>>>> 1 file changed, 10 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/input/ti,drv260x.txt b/Documentation/devicetree/bindings/input/ti,drv260x.txt
>>>> index a9c8519..214e25d 100644
>>>> --- a/Documentation/devicetree/bindings/input/ti,drv260x.txt
>>>> +++ b/Documentation/devicetree/bindings/input/ti,drv260x.txt
>>>> @@ -37,15 +37,16 @@ Optional properties:
>>>> 3.2 v.
>>>> Example:
>>>>
>>>> -drv2605l: drv2605l@5a {
>>>> - compatible = "ti,drv2605l";
>>>> - reg = <0x5a>;
>>>> - enable-gpio = <&gpio1 28 GPIO_ACTIVE_HIGH>;
>>>> - mode = <DRV260X_LRA_MODE>;
>>>> - library-sel = <DRV260X_LIB_LRA>;
>>>> - vib-rated-mv = <3200>;
>>>> - vib-overdriver-mv = <3200>;
>>>> -};
>>>> +haptics:@5a {
>>>
>>> haptics: haptics@5a ??
>>>
>>
>> I found both types of entries in the bindings docs.
>> There is no consistency within the input bindings documentation.
>
> I could swear neither two types have a colon character between node name
> and its base address.
>
I will add it to the fixes.
Dan
--
------------------
Dan Murphy
^ permalink raw reply
* Re: [PATCH] doc: dt/bindings: input: Fix drv260x binding document
From: Felipe Balbi @ 2014-08-22 14:45 UTC (permalink / raw)
To: Murphy, Dan
Cc: Balbi, Felipe, devicetree@vger.kernel.org,
linux-input@vger.kernel.org, linux-kernel@vger.kernel.org,
dmitry.torokhov@gmail.com, linux-arm-kernel@lists.infradead.org
In-Reply-To: <00FC9A978A94B7418C33AFAE8A35ED49DF1F66@DFLE09.ent.ti.com>
[-- Attachment #1: Type: text/plain, Size: 1607 bytes --]
Hi,
On Fri, Aug 22, 2014 at 09:25:13AM -0500, Murphy, Dan wrote:
> Felipe
>
> On 08/22/2014 09:15 AM, Balbi, Felipe wrote:
> > On Fri, Aug 22, 2014 at 09:12:21AM -0500, Dan Murphy wrote:
> >> Update the drv260x dt binding document:
> >> - Change the node name to the devices function not the
> >> device name.
> >> - Add vbat-supply to the example.
> >> - Fix indentation of the example.
> >>
> >> Signed-off-by: Dan Murphy <dmurphy@ti.com>
> >> ---
> >> .../devicetree/bindings/input/ti,drv260x.txt | 19 ++++++++++---------
> >> 1 file changed, 10 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/Documentation/devicetree/bindings/input/ti,drv260x.txt b/Documentation/devicetree/bindings/input/ti,drv260x.txt
> >> index a9c8519..214e25d 100644
> >> --- a/Documentation/devicetree/bindings/input/ti,drv260x.txt
> >> +++ b/Documentation/devicetree/bindings/input/ti,drv260x.txt
> >> @@ -37,15 +37,16 @@ Optional properties:
> >> 3.2 v.
> >> Example:
> >>
> >> -drv2605l: drv2605l@5a {
> >> - compatible = "ti,drv2605l";
> >> - reg = <0x5a>;
> >> - enable-gpio = <&gpio1 28 GPIO_ACTIVE_HIGH>;
> >> - mode = <DRV260X_LRA_MODE>;
> >> - library-sel = <DRV260X_LIB_LRA>;
> >> - vib-rated-mv = <3200>;
> >> - vib-overdriver-mv = <3200>;
> >> -};
> >> +haptics:@5a {
> >
> > haptics: haptics@5a ??
> >
>
> I found both types of entries in the bindings docs.
> There is no consistency within the input bindings documentation.
I could swear neither two types have a colon character between node name
and its base address.
--
balbi
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [GIT PULL] HID
From: Benjamin Tissoires @ 2014-08-22 14:25 UTC (permalink / raw)
To: Markus Trippelsdorf; +Cc: Jiri Kosina, linux-input, linux-kernel, Ben Hawkes
In-Reply-To: <20140822081819.GA297@x4>
On Aug 22 2014 or thereabouts, Markus Trippelsdorf wrote:
> On 2014.08.22 at 03:00 -0500, Jiri Kosina wrote:
> > On Fri, 22 Aug 2014, Markus Trippelsdorf wrote:
> >
> > > > HID: logitech: perform bounds checking on device_id early enough
> > >
> > > The commit above (ad3e14d7c5268c2e) causes the bounds checking to always
> > > fail on my monolithic kernel (without modules):
> > >
> > > ...
> > > [ 2.922617] usb 4-2: new full-speed USB device number 2 using ohci-pci
> > > [ 2.996587] udevd[98]: starting eudev version 1.0
> > > [ 3.071203] random: nonblocking pool is initialized
> > > [ 3.108360] logitech-djreceiver 0003:046D:C52B.0003: hiddev0,hidraw0: USB HID v1.11 Device [Logitech USB Receiver] on usb-0000:00:12.1-2/input2
> > > [ 3.163208] input: Logitech Unifying Device. Wireless PID:4026 as /devices/pci0000:00/0000:00:12.1/usb4/4-2/4-2:1.2/0003:046D:C52B.0003/0003:046D:C52B.0004/input/input2
> > > [ 3.163511] logitech-djdevice 0003:046D:C52B.0004: input,hidraw1: USB HID v1.11 Keyboard [Logitech Unifying Device. Wireless PID:4026] on usb-0000:00:12.1-2:1
> > > [ 3.164121] logitech-djreceiver 0003:046D:C52B.0003: logi_dj_raw_event: invalid device index:0
> > > [ 3.289261] usb 3-1: new low-speed USB device number 2 using ohci-pci
> > > [ 3.457606] input: HID 046a:0011 as /devices/pci0000:00/0000:00:12.0/usb3/3-1/3-1:1.0/0003:046A:0011.0005/input/input3
> > > [ 3.457794] hid-generic 0003:046A:0011.0005: input,hidraw2: USB HID v1.10 Keyboard [HID 046a:0011] on usb-0000:00:12.0-1/input0
> > > [ 3.712886] Switched to clocksource tsc
> >
> > Thanks a lot for a timely report.
> >
> > I am travelling till next week and don't unfortunately have the hardware
> > here with me to test momentarily, so please take this with a lot of grains
> > of salt (maybe Benjamin would be able to look into this before I'd be able
> > to on monday ... ?).
> >
> > Does the shot-in-the-dark patch below put things back in shape for you?
>
> Thanks a lot for the timely patch. It indeed fixes the issue for me.
>
Huh, my bad, I should have actually test Jiri's patch instead of
thinking "this can not break anything".
So Jiri, your patch does not work because the device index 0 is the
receiver, and 1-6 are the wireless devices attached to this receiver. So
basically, this new patch removes the ability to have 6 devices paired.
I'll try to come out with something by the end of the day which would
both prevent the out of bound and allow to have 6 true wireless devices.
Cheers,
Benjamin
^ permalink raw reply
* Re: [PATCH] doc: dt/bindings: input: Fix drv260x binding document
From: Murphy, Dan @ 2014-08-22 14:25 UTC (permalink / raw)
To: Balbi, Felipe
Cc: devicetree@vger.kernel.org, linux-input@vger.kernel.org,
linux-kernel@vger.kernel.org, dmitry.torokhov@gmail.com,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <20140822141532.GC22707@saruman.home>
Felipe
On 08/22/2014 09:15 AM, Balbi, Felipe wrote:
> On Fri, Aug 22, 2014 at 09:12:21AM -0500, Dan Murphy wrote:
>> Update the drv260x dt binding document:
>> - Change the node name to the devices function not the
>> device name.
>> - Add vbat-supply to the example.
>> - Fix indentation of the example.
>>
>> Signed-off-by: Dan Murphy <dmurphy@ti.com>
>> ---
>> .../devicetree/bindings/input/ti,drv260x.txt | 19 ++++++++++---------
>> 1 file changed, 10 insertions(+), 9 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/input/ti,drv260x.txt b/Documentation/devicetree/bindings/input/ti,drv260x.txt
>> index a9c8519..214e25d 100644
>> --- a/Documentation/devicetree/bindings/input/ti,drv260x.txt
>> +++ b/Documentation/devicetree/bindings/input/ti,drv260x.txt
>> @@ -37,15 +37,16 @@ Optional properties:
>> 3.2 v.
>> Example:
>>
>> -drv2605l: drv2605l@5a {
>> - compatible = "ti,drv2605l";
>> - reg = <0x5a>;
>> - enable-gpio = <&gpio1 28 GPIO_ACTIVE_HIGH>;
>> - mode = <DRV260X_LRA_MODE>;
>> - library-sel = <DRV260X_LIB_LRA>;
>> - vib-rated-mv = <3200>;
>> - vib-overdriver-mv = <3200>;
>> -};
>> +haptics:@5a {
>
> haptics: haptics@5a ??
>
I found both types of entries in the bindings docs.
There is no consistency within the input bindings documentation.
I have no issue doing it either way.
Dan
--
------------------
Dan Murphy
^ permalink raw reply
* Re: [PATCH] doc: dt/bindings: input: Fix drv260x binding document
From: Felipe Balbi @ 2014-08-22 14:15 UTC (permalink / raw)
To: Dan Murphy
Cc: devicetree, linux-input, linux-kernel, dmitry.torokhov,
linux-arm-kernel
In-Reply-To: <1408716741-1163-1-git-send-email-dmurphy@ti.com>
[-- Attachment #1: Type: text/plain, Size: 1147 bytes --]
On Fri, Aug 22, 2014 at 09:12:21AM -0500, Dan Murphy wrote:
> Update the drv260x dt binding document:
> - Change the node name to the devices function not the
> device name.
> - Add vbat-supply to the example.
> - Fix indentation of the example.
>
> Signed-off-by: Dan Murphy <dmurphy@ti.com>
> ---
> .../devicetree/bindings/input/ti,drv260x.txt | 19 ++++++++++---------
> 1 file changed, 10 insertions(+), 9 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/input/ti,drv260x.txt b/Documentation/devicetree/bindings/input/ti,drv260x.txt
> index a9c8519..214e25d 100644
> --- a/Documentation/devicetree/bindings/input/ti,drv260x.txt
> +++ b/Documentation/devicetree/bindings/input/ti,drv260x.txt
> @@ -37,15 +37,16 @@ Optional properties:
> 3.2 v.
> Example:
>
> -drv2605l: drv2605l@5a {
> - compatible = "ti,drv2605l";
> - reg = <0x5a>;
> - enable-gpio = <&gpio1 28 GPIO_ACTIVE_HIGH>;
> - mode = <DRV260X_LRA_MODE>;
> - library-sel = <DRV260X_LIB_LRA>;
> - vib-rated-mv = <3200>;
> - vib-overdriver-mv = <3200>;
> -};
> +haptics:@5a {
haptics: haptics@5a ??
--
balbi
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* [PATCH] doc: dt/bindings: input: Fix drv260x binding document
From: Dan Murphy @ 2014-08-22 14:12 UTC (permalink / raw)
To: devicetree, linux-input
Cc: linux-kernel, dmitry.torokhov, linux-arm-kernel, Dan Murphy
Update the drv260x dt binding document:
- Change the node name to the devices function not the
device name.
- Add vbat-supply to the example.
- Fix indentation of the example.
Signed-off-by: Dan Murphy <dmurphy@ti.com>
---
.../devicetree/bindings/input/ti,drv260x.txt | 19 ++++++++++---------
1 file changed, 10 insertions(+), 9 deletions(-)
diff --git a/Documentation/devicetree/bindings/input/ti,drv260x.txt b/Documentation/devicetree/bindings/input/ti,drv260x.txt
index a9c8519..214e25d 100644
--- a/Documentation/devicetree/bindings/input/ti,drv260x.txt
+++ b/Documentation/devicetree/bindings/input/ti,drv260x.txt
@@ -37,15 +37,16 @@ Optional properties:
3.2 v.
Example:
-drv2605l: drv2605l@5a {
- compatible = "ti,drv2605l";
- reg = <0x5a>;
- enable-gpio = <&gpio1 28 GPIO_ACTIVE_HIGH>;
- mode = <DRV260X_LRA_MODE>;
- library-sel = <DRV260X_LIB_LRA>;
- vib-rated-mv = <3200>;
- vib-overdriver-mv = <3200>;
-};
+haptics:@5a {
+ compatible = "ti,drv2605l";
+ reg = <0x5a>;
+ vbat-supply = <&vbat>;
+ enable-gpio = <&gpio1 28 GPIO_ACTIVE_HIGH>;
+ mode = <DRV260X_LRA_MODE>;
+ library-sel = <DRV260X_LIB_LRA>;
+ vib-rated-mv = <3200>;
+ vib-overdriver-mv = <3200>;
+}
For more product information please see the link below:
http://www.ti.com/product/drv2605
--
1.7.9.5
^ permalink raw reply related
* [PATCH] input: misc: drv260x: add check for ERM mode and LRA Libraries
From: Dan Murphy @ 2014-08-22 14:11 UTC (permalink / raw)
To: linux-input, dmitry.torokhov; +Cc: linux-kernel, linux-arm-kernel, Dan Murphy
Add a check to ensure that LRA libraries are not mixed with
the ERM mode.
If ERM mode and the Library is empty "OR" the
LRA library then exit.
As the LRA and empty libraries are not applicable for
the ERM actuator.
Signed-off-by: Dan Murphy <dmurphy@ti.com>
---
drivers/input/misc/drv260x.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/drivers/input/misc/drv260x.c b/drivers/input/misc/drv260x.c
index a7a19e6..d6a26a7 100644
--- a/drivers/input/misc/drv260x.c
+++ b/drivers/input/misc/drv260x.c
@@ -564,6 +564,14 @@ static int drv260x_probe(struct i2c_client *client,
return -EINVAL;
}
+ if (haptics->mode == DRV260X_ERM_MODE &&
+ haptics->library == DRV260X_LIB_EMPTY ||
+ haptics->library == DRV260X_LIB_LRA) {
+ dev_err(&client->dev,
+ "ERM Mode with LRA Library mismatch\n");
+ return -EINVAL;
+ }
+
haptics->regulator = devm_regulator_get(&client->dev, "vbat");
if (IS_ERR(haptics->regulator)) {
error = PTR_ERR(haptics->regulator);
--
1.7.9.5
^ permalink raw reply related
* [PATCH] input: misc: drv260x remove defines not used
From: Dan Murphy @ 2014-08-22 14:11 UTC (permalink / raw)
To: linux-input, dmitry.torokhov; +Cc: linux-kernel, linux-arm-kernel, Dan Murphy
Removing some #defines that are not and should
never be used pertaining to I2C.
Removing:
define DRV260X_ALLOWED_R_BYTES 25
define DRV260X_ALLOWED_W_BYTES 2
define DRV260X_MAX_RW_RETRIES 5
define DRV260X_I2C_RETRY_DELAY 10
Signed-off-by: Dan Murphy <dmurphy@ti.com>
---
drivers/input/misc/drv260x.c | 5 -----
1 file changed, 5 deletions(-)
diff --git a/drivers/input/misc/drv260x.c b/drivers/input/misc/drv260x.c
index e90e3b8..a7a19e6 100644
--- a/drivers/input/misc/drv260x.c
+++ b/drivers/input/misc/drv260x.c
@@ -66,11 +66,6 @@
#define DRV260X_LRA_RES_PERIOD 0x22
#define DRV260X_MAX_REG 0x23
-#define DRV260X_ALLOWED_R_BYTES 25
-#define DRV260X_ALLOWED_W_BYTES 2
-#define DRV260X_MAX_RW_RETRIES 5
-#define DRV260X_I2C_RETRY_DELAY 10
-
#define DRV260X_GO_BIT 0x01
/* Library Selection */
--
1.7.9.5
^ permalink raw reply related
* Re: [PATCH 1/1] HID: usbhid: add usb_clear_halt determination for next hid_start_in
From: Alan Stern @ 2014-08-22 13:56 UTC (permalink / raw)
To: vichy; +Cc: Jiri Kosina, linux-usb@vger.kernel.org, linux-input
In-Reply-To: <CAOVJa8F5HoS48wYn5-yHCeioYNOOGEKwXg6WujCQH0QxA44o=Q@mail.gmail.com>
On Fri, 22 Aug 2014, vichy wrote:
> hi Jiri:
>
> 2014-08-22 15:45 GMT+08:00 Jiri Kosina <jkosina@suse.cz>:
> > On Fri, 22 Aug 2014, CheChun Kuo wrote:
> >
> >> HID IR device will not response to any command send from host when
> >> it is finishing paring and tring to reset itself. During this period of
> >> time, usb_cleaer_halt will be fail and if hid_start_in soon again, we
> >> has the possibility trap in infinite loop.
> >>
> >> Signed-off-by: CheChun Kuo <vichy.kuo@gmail.com>
> >> ---
> >> drivers/hid/usbhid/hid-core.c | 3 ++-
> >> 1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
> >> index 79cf503..256b102 100644
> >> --- a/drivers/hid/usbhid/hid-core.c
> >> +++ b/drivers/hid/usbhid/hid-core.c
> >> @@ -122,7 +122,8 @@ static void hid_reset(struct work_struct *work)
> >> dev_dbg(&usbhid->intf->dev, "clear halt\n");
> >> rc = usb_clear_halt(hid_to_usb_dev(hid), usbhid->urbin->pipe);
> >> clear_bit(HID_CLEAR_HALT, &usbhid->iofl);
> >> - hid_start_in(hid);
> >> + if (rc == 0)
> >> + hid_start_in(hid);
> >> }
> >
> > I'd need a more detailed explanation for this patch, as I don't understand
> > neither the text in the changelog nor the patch itself. Namely:
> >
> > - which IR devices are showing this behavior?
> The USB hid device that will transform IR signal to usb command when
> user press "volume up/down", "voice recording", etc.
>
> > - where does the infinite loop come from? hid_reset() should error out
> > properly if usb_clear_halt() fails and HID_IN_RUNNING flag is not set
> Below I briefly draw the timing when this issue happen
>
> i) hid_irq_in get URB status as -EPIPE
> ii) set HID_CLEAR_HALT flag and schedule hid_reset work
> iii) hid_reset call usb_clear_halt and hid_start_in again
> iv) if device still doesn't response host command, it will go to i)
> above and keep looping
>
> thanks for your help,
I recently posted (but did not submit) a more comprehensive solution to
this and other related problems. For example, HID devices typically
run at low speed or full speed. When attached through a hub to an EHCI
controller (very common on modern systems), unplugging the device
causes -EPIPE errors, so the driver calls usb_clear_halt and restarts
the interrupt pipe. Of course, the same failure occurs again, so
there's a lengthy flurry of useless error messages.
This patch changes the driver so that after usb_clear_halt fails, it
will try to do a port reset. And if that fails, the driver will be
unbound from the device. This avoids infinite loops.
What do you think?
Alan Stern
Index: usb-3.16/drivers/hid/usbhid/hid-core.c
===================================================================
--- usb-3.16.orig/drivers/hid/usbhid/hid-core.c
+++ usb-3.16/drivers/hid/usbhid/hid-core.c
@@ -116,40 +116,24 @@ static void hid_reset(struct work_struct
struct usbhid_device *usbhid =
container_of(work, struct usbhid_device, reset_work);
struct hid_device *hid = usbhid->hid;
- int rc = 0;
+ int rc;
if (test_bit(HID_CLEAR_HALT, &usbhid->iofl)) {
dev_dbg(&usbhid->intf->dev, "clear halt\n");
rc = usb_clear_halt(hid_to_usb_dev(hid), usbhid->urbin->pipe);
clear_bit(HID_CLEAR_HALT, &usbhid->iofl);
- hid_start_in(hid);
- }
-
- else if (test_bit(HID_RESET_PENDING, &usbhid->iofl)) {
- dev_dbg(&usbhid->intf->dev, "resetting device\n");
- rc = usb_lock_device_for_reset(hid_to_usb_dev(hid), usbhid->intf);
if (rc == 0) {
- rc = usb_reset_device(hid_to_usb_dev(hid));
- usb_unlock_device(hid_to_usb_dev(hid));
+ hid_start_in(hid);
+ } else {
+ dev_dbg(&usbhid->intf->dev,
+ "clear-halt failed: %d\n", rc);
+ set_bit(HID_RESET_PENDING, &usbhid->iofl);
}
- clear_bit(HID_RESET_PENDING, &usbhid->iofl);
}
- switch (rc) {
- case 0:
- if (!test_bit(HID_IN_RUNNING, &usbhid->iofl))
- hid_io_error(hid);
- break;
- default:
- hid_err(hid, "can't reset device, %s-%s/input%d, status %d\n",
- hid_to_usb_dev(hid)->bus->bus_name,
- hid_to_usb_dev(hid)->devpath,
- usbhid->ifnum, rc);
- /* FALLTHROUGH */
- case -EHOSTUNREACH:
- case -ENODEV:
- case -EINTR:
- break;
+ if (test_bit(HID_RESET_PENDING, &usbhid->iofl)) {
+ dev_dbg(&usbhid->intf->dev, "resetting device\n");
+ usb_queue_reset_device(usbhid->intf);
}
}
^ permalink raw reply
* Re: [RESEND PATCH 6/7] mfd: cros_ec: Instantiate sub-devices from device tree
From: Javier Martinez Canillas @ 2014-08-22 11:27 UTC (permalink / raw)
To: Lee Jones
Cc: Wolfram Sang, Dmitry Torokhov, Doug Anderson, Simon Glass,
Bill Richardson, Andrew Bresticker, Derek Basehore, Todd Broch,
Olof Johansson, Andreas Färber, linux-i2c, linux-input,
linux-samsung-soc
In-Reply-To: <20140821142542.GN4266@lee--X1>
Hello Lee,
On 08/21/2014 04:25 PM, Lee Jones wrote:
>>
>> static const struct mfd_cell cros_devs[] = {
>> - {
>> - .name = "cros-ec-keyb",
>> - .id = 1,
>> - .of_compatible = "google,cros-ec-keyb",
>> - },
>> - {
>> - .name = "cros-ec-i2c-tunnel",
>> - .id = 2,
>> - .of_compatible = "google,cros-ec-i2c-tunnel",
>> - },
>> };
>
> Why are you keeping this round if it's empty?
>
Right, I'll just remove it.
>> }
>> +#ifdef CONFIG_OF
>> + /*
>> + * Add sub-devices declared in the device tree. NOTE they should NOT be
>> + * declared in cros_devs
>> + */
>> + for_each_child_of_node(dev->of_node, node) {
>> + char name[128];
>> + struct mfd_cell cell = {
>> + .id = 0,
>> + .name = name,
>> + };
>> +
>> + if (of_modalias_node(node, name, sizeof(name)) < 0) {
>> + dev_err(dev, "modalias failure on %s\n",
>> + node->full_name);
>> + continue;
>> + }
>> + dev_dbg(dev, "adding MFD sub-device %s\n", node->name);
>> + cell.of_compatible = of_get_property(node, "compatible", NULL);
>> + err = mfd_add_devices(dev, ++id, &cell, 1, NULL, ec_dev->irq,
>> + NULL);
>> + if (err)
>> + dev_err(dev, "fail to add %s\n", node->full_name);
>> + }
>> +#endif
>
> This is grim!
>
> Why don't you use of_platform_populate()?
>
Indeed, I think it may just work and all these is not needed.
Thanks for your feedback and best regards,
Javier
^ permalink raw reply
* Re: [RESEND PATCH 5/7] mfd: cros_ec: wait for completion of commands that return IN_PROGRESS
From: Javier Martinez Canillas @ 2014-08-22 11:24 UTC (permalink / raw)
To: Lee Jones
Cc: Wolfram Sang, Dmitry Torokhov, Doug Anderson, Simon Glass,
Bill Richardson, Andrew Bresticker, Derek Basehore, Todd Broch,
Olof Johansson, Andreas Färber,
linux-i2c-u79uwXL29TY76Z2rM5mHXA,
linux-input-u79uwXL29TY76Z2rM5mHXA,
linux-samsung-soc-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20140821142100.GM4266@lee--X1>
Hello Lee,
Thanks a lot for your feedback.
On 08/21/2014 04:21 PM, Lee Jones wrote:
> On Wed, 20 Aug 2014, Javier Martinez Canillas wrote:
>
>> From: Andrew Bresticker <abrestic-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
>>
>> When an EC command returns EC_RES_IN_PROGRESS, we need to query
>> the state of the EC until it indicates that it is no longer busy.
>> Do this in cros_ec_cmd_xfer() under the EC's mutex so that other
>> commands (e.g. keyboard, I2C passtru) aren't issued to the EC while
>> it is working on the in-progress command.
>>
>> Signed-off-by: Andrew Bresticker <abrestic-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
>> Reviewed-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
>> Signed-off-by: Javier Martinez Canillas <javier.martinez-ZGY8ohtN/8pPYcu2f3hruQ@public.gmane.org>
>> ---
>> drivers/mfd/cros_ec.c | 35 ++++++++++++++++++++++++++++++++++-
>> 1 file changed, 34 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/mfd/cros_ec.c b/drivers/mfd/cros_ec.c
>> index c53804a..634c434 100644
>> --- a/drivers/mfd/cros_ec.c
>> +++ b/drivers/mfd/cros_ec.c
>> @@ -23,6 +23,10 @@
>> #include <linux/mfd/core.h>
>> #include <linux/mfd/cros_ec.h>
>> #include <linux/mfd/cros_ec_commands.h>
>> +#include <linux/delay.h>
>> +
>> +#define EC_COMMAND_RETRIES 50
>> +#define EC_RETRY_DELAY_MS 10
>>
>> int cros_ec_prepare_tx(struct cros_ec_device *ec_dev,
>> struct cros_ec_command *msg)
>> @@ -65,10 +69,39 @@ EXPORT_SYMBOL(cros_ec_check_result);
>> int cros_ec_cmd_xfer(struct cros_ec_device *ec_dev,
>> struct cros_ec_command *msg)
>> {
>> - int ret;
>> + int ret, i;
>>
>> mutex_lock(&ec_dev->lock);
>> ret = ec_dev->cmd_xfer(ec_dev, msg);
>> + if (ret == -EAGAIN && msg->result == EC_RES_IN_PROGRESS) {
>> + /*
>> + * Query the EC's status until it's no longer busy or
>> + * we encounter an error.
>> + */
>> + for (i = 0; i < EC_COMMAND_RETRIES; i++) {
>> + struct cros_ec_command status_msg;
>> + struct ec_response_get_comms_status status;
>> +
>> + msleep(EC_RETRY_DELAY_MS);
>> +
>> + status_msg.version = 0;
>> + status_msg.command = EC_CMD_GET_COMMS_STATUS;
>> + status_msg.outdata = NULL;
>> + status_msg.outsize = 0;
>> + status_msg.indata = (uint8_t *)&status;
>> + status_msg.insize = sizeof(status);
>> +
>> + ret = ec_dev->cmd_xfer(ec_dev, &status_msg);
>> + if (ret < 0)
>> + break;
>> +
>> + msg->result = status_msg.result;
>> + if (status_msg.result != EC_RES_SUCCESS)
>> + break;
>> + if (!(status.flags & EC_COMMS_STATUS_PROCESSING))
>> + break;
>> + }
>> + }
>
> Wow! Things just got ugly real fast.
>
> Do the *xfer() calls fiddle with msg passed into cros_ec_cmd_xfer()?
> If not, why is it necessary to keep populating it?
>
Not really, I see that only struct cros_ec_command .result and .indata
fields are modified by the cmd_xfer() function handlers so I agree with
you that these variables can be defined outside of the for loop and
reused. I think that even zero'ing indata is not needed between calls
since on success the full sizeof(status) is copied and on failure it
doesn't matter what is there.
Will change this when doing the re-spin.
> If all this stuff is necessary (and I really hope that it's not) I
> think it would be better to have the for() loop as the outer layer.
> Then we only have one instance of cmd_xfer() invocation and we save a
> layer of tabbing.
>
Most of this doesn't need to be inside the for loop as you said but there
is a need for two struct cros_ec_command *msg though, one for the actual
command made by the caller and other to query the EC if it already
finished processing the first command.
>> mutex_unlock(&ec_dev->lock);
>>
>> return ret;
>
Best regards,
Javier
^ permalink raw reply
* Re: [PATCH 1/1] HID: usbhid: add usb_clear_halt determination for next hid_start_in
From: vichy @ 2014-08-22 10:43 UTC (permalink / raw)
To: Jiri Kosina; +Cc: linux-usb@vger.kernel.org, linux-input
In-Reply-To: <alpine.LNX.2.00.1408220240430.23162@pobox.suse.cz>
hi Jiri:
2014-08-22 15:45 GMT+08:00 Jiri Kosina <jkosina@suse.cz>:
> On Fri, 22 Aug 2014, CheChun Kuo wrote:
>
>> HID IR device will not response to any command send from host when
>> it is finishing paring and tring to reset itself. During this period of
>> time, usb_cleaer_halt will be fail and if hid_start_in soon again, we
>> has the possibility trap in infinite loop.
>>
>> Signed-off-by: CheChun Kuo <vichy.kuo@gmail.com>
>> ---
>> drivers/hid/usbhid/hid-core.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
>> index 79cf503..256b102 100644
>> --- a/drivers/hid/usbhid/hid-core.c
>> +++ b/drivers/hid/usbhid/hid-core.c
>> @@ -122,7 +122,8 @@ static void hid_reset(struct work_struct *work)
>> dev_dbg(&usbhid->intf->dev, "clear halt\n");
>> rc = usb_clear_halt(hid_to_usb_dev(hid), usbhid->urbin->pipe);
>> clear_bit(HID_CLEAR_HALT, &usbhid->iofl);
>> - hid_start_in(hid);
>> + if (rc == 0)
>> + hid_start_in(hid);
>> }
>
> I'd need a more detailed explanation for this patch, as I don't understand
> neither the text in the changelog nor the patch itself. Namely:
>
> - which IR devices are showing this behavior?
The USB hid device that will transform IR signal to usb command when
user press "volume up/down", "voice recording", etc.
> - where does the infinite loop come from? hid_reset() should error out
> properly if usb_clear_halt() fails and HID_IN_RUNNING flag is not set
Below I briefly draw the timing when this issue happen
i) hid_irq_in get URB status as -EPIPE
ii) set HID_CLEAR_HALT flag and schedule hid_reset work
iii) hid_reset call usb_clear_halt and hid_start_in again
iv) if device still doesn't response host command, it will go to i)
above and keep looping
thanks for your help,
^ permalink raw reply
* RE: [PATCH v4 4/14] input: cyapa: add cyapa key function interfaces in sysfs system
From: Dudley Du @ 2014-08-22 8:41 UTC (permalink / raw)
To: 'Dmitry Torokhov'
Cc: 'Rafael J. Wysocki', 'Benson Leung',
'Patrik Fimml', linux-input, linux-kernel, Dudley Du
In-Reply-To: <20140821182848.GD5854@core.coreip.homeip.net>
> -----Original Message-----
> From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com]
> Sent: Friday, August 22, 2014 2:29 AM
> To: Dudley Du
> Cc: Rafael J. Wysocki; Benson Leung; Patrik Fimml; linux-input@vger.kernel.org;
> linux-kernel@vger.kernel.org; Dudley Du
> Subject: Re: [PATCH v4 4/14] input: cyapa: add cyapa key function interfaces
> in sysfs system
>
> On Thu, Jul 17, 2014 at 02:51:04PM +0800, Dudley Du wrote:
> > Add key basic function interfaces in cyapa driver in sysfs system,
> > these interfaces are commonly used in pre- and after production, and
> > for trackpad device state checking, manage and firmware image updating.
> > These interfaces including firmware_version and product_id interfaces
> > for reading firmware version and trackpad device product id values,
> > and including update_fw interface to command firmware image update
> > process. Also including baseline and calibrate interfaces, so can
> > read and check the trackpad device states. If the baseline values are
> > invalid, then can use calibrate interface to recover it.
> > TEST=test on Chromebooks.
> >
> > Signed-off-by: Dudley Du <dudl@cypress.com>
> > ---
> > drivers/input/mouse/cyapa.c | 190
> +++++++++++++++++++++++++++++++++++++++++++
> > drivers/input/mouse/cyapa.h | 27 ++++++
> > 2 files changed, 217 insertions(+)
> >
> > diff --git a/drivers/input/mouse/cyapa.c b/drivers/input/mouse/cyapa.c
> > index 6a2783b..53c9d59 100644
> > --- a/drivers/input/mouse/cyapa.c
> > +++ b/drivers/input/mouse/cyapa.c
> > @@ -388,6 +388,78 @@ static void cyapa_detect(struct cyapa *cyapa)
> > }
> > }
> >
> > +static int cyapa_firmware(struct cyapa *cyapa, const char *fw_name)
> > +{
> > + struct device *dev = &cyapa->client->dev;
> > + int ret;
> > + const struct firmware *fw;
> > +
> > + ret = request_firmware(&fw, fw_name, dev);
> > + if (ret) {
> > + dev_err(dev, "Could not load firmware from %s, %d\n",
> > + fw_name, ret);
> > + return ret;
> > + }
> > +
> > + if (cyapa->ops->check_fw) {
> > + ret = cyapa->ops->check_fw(cyapa, fw);
> > + if (ret) {
> > + dev_err(dev, "Invalid CYAPA firmware image: %s\n",
> > + fw_name);
> > + goto done;
> > + }
> > + } else {
> > + dev_err(dev, "Unknown status, operation forbidden, gen=%d\n",
> > + cyapa->gen);
> > + ret = -ENOTSUPP;
>
> I'd say EIO or EINVAL.
Thanks, I will set it to EINVAL in next release.
>
> > + goto done;
> > + }
> > +
> > + /*
> > + * Resume the potentially suspended device because doing FW
> > + * update on a device not in the FULL mode has a chance to
> > + * fail.
> > + */
> > + pm_runtime_get_sync(dev);
> > +
> > + if (cyapa->ops->bl_enter) {
> > + ret = cyapa->ops->bl_enter(cyapa);
> > + if (ret)
> > + goto err_detect;
> > + }
> > +
> > + if (cyapa->ops->bl_activate) {
> > + ret = cyapa->ops->bl_activate(cyapa);
> > + if (ret)
> > + goto err_detect;
> > + }
> > +
> > + if (cyapa->ops->bl_initiate) {
> > + ret = cyapa->ops->bl_initiate(cyapa, fw);
> > + if (ret)
> > + goto err_detect;
> > + }
> > +
> > + if (cyapa->ops->update_fw) {
> > + ret = cyapa->ops->update_fw(cyapa, fw);
> > + if (ret)
> > + goto err_detect;
> > + }
> > +
> > + if (cyapa->ops->bl_verify_app_integrity) {
> > + ret = cyapa->ops->bl_verify_app_integrity(cyapa);
> > + if (ret)
> > + goto err_detect;
> > + }
> > +
> > +err_detect:
> > + pm_runtime_put_noidle(dev);
> > +
> > +done:
> > + release_firmware(fw);
> > + return ret;
> > +}
> > +
> > /*
> > * Sysfs Interface.
> > */
> > @@ -584,6 +656,120 @@ static void cyapa_start_runtime(struct cyapa *cyapa)
> > static void cyapa_start_runtime(struct cyapa *cyapa) {}
> > #endif /* CONFIG_PM_RUNTIME */
> >
> > +static ssize_t cyapa_show_fm_ver(struct device *dev,
> > + struct device_attribute *attr, char *buf)
> > +{
> > + int ret;
> > + struct cyapa *cyapa = dev_get_drvdata(dev);
> > +
> > + mutex_lock(&cyapa->state_sync_lock);
> > + ret = scnprintf(buf, PAGE_SIZE, "%d.%d\n", cyapa->fw_maj_ver,
> > + cyapa->fw_min_ver);
> > + mutex_unlock(&cyapa->state_sync_lock);
> > + return ret;
> > +}
> > +
> > +static ssize_t cyapa_show_product_id(struct device *dev,
> > + struct device_attribute *attr, char *buf)
> > +{
> > + int ret;
> > + struct cyapa *cyapa = dev_get_drvdata(dev);
> > +
> > + mutex_lock(&cyapa->state_sync_lock);
> > + ret = scnprintf(buf, PAGE_SIZE, "%s\n", cyapa->product_id);
> > + mutex_unlock(&cyapa->state_sync_lock);
> > + return ret;
> > +}
> > +
> > +static ssize_t cyapa_update_fw_store(struct device *dev,
> > + struct device_attribute *attr,
> > + const char *buf, size_t count)
> > +{
> > + struct cyapa *cyapa = dev_get_drvdata(dev);
> > + const char *fw_name;
> > + int ret;
> > +
> > + /* Do not allow paths that step out of /lib/firmware */
> > + if (strstr(buf, "../") != NULL)
> > + return -EINVAL;
>
> This should go away, it does not help anything.
Thanks. It will be removed in next release.
>
> > +
> > + if (!strncmp(buf, "1", count) || !strncmp(buf, "1\n", count))
> > + fw_name = CYAPA_FW_NAME;
> > + else
> > + fw_name = buf;
>
> I'd rather you either required firmware name to be passed always or settle on
> given name. Otherwise why can't I call my firmware file '1'?
>
Thanks. I will modify to require firmware name to be passed always.
> > +
> > + mutex_lock(&cyapa->state_sync_lock);
> > +
> > + ret = cyapa_firmware(cyapa, fw_name);
> > + if (ret)
> > + dev_err(dev, "firmware update failed, %d\n", ret);
> > + else
> > + dev_dbg(dev, "firmware update succeeded\n");
> > +
> > + mutex_unlock(&cyapa->state_sync_lock);
> > +
> > + /* Redetect trackpad device states. */
> > + cyapa_detect_async(cyapa, 0);
> > +
> > + return ret ? ret : count;
> > +}
> > +
> > +static ssize_t cyapa_calibrate_store(struct device *dev,
> > + struct device_attribute *attr,
> > + const char *buf, size_t count)
> > +{
> > + struct cyapa *cyapa = dev_get_drvdata(dev);
> > + int ret;
> > +
> > + mutex_lock(&cyapa->state_sync_lock);
> > +
> > + if (!cyapa->ops->calibrate_store) {
> > + dev_err(dev, "Calibrate operation not permitted.\n");
> > + ret = -ENOTSUPP;
>
> Permitted as in supported or forbidden?
The "permitted" means not supported, because device state is unknown.
I will modify the word "permitted" to "supported".
Thanks.
>
> > + } else
> > + ret = cyapa->ops->calibrate_store(dev, attr, buf, count);
> > +
> > + mutex_unlock(&cyapa->state_sync_lock);
> > + return ret < 0 ? ret : count;
> > +}
> > +
> > +static ssize_t cyapa_show_baseline(struct device *dev,
> > + struct device_attribute *attr, char *buf)
> > +{
> > + struct cyapa *cyapa = dev_get_drvdata(dev);
> > + ssize_t ret;
> > +
> > + mutex_lock(&cyapa->state_sync_lock);
> > +
> > + if (!cyapa->ops->show_baseline) {
> > + dev_err(dev, "Calibrate operation not permitted.\n");
> > + ret = -ENOTSUPP;
> > + } else
> > + ret = cyapa->ops->show_baseline(dev, attr, buf);
> > +
> > + mutex_unlock(&cyapa->state_sync_lock);
> > + return ret;
> > +}
> > +
> > +static DEVICE_ATTR(firmware_version, S_IRUGO, cyapa_show_fm_ver, NULL);
> > +static DEVICE_ATTR(product_id, S_IRUGO, cyapa_show_product_id, NULL);
> > +static DEVICE_ATTR(update_fw, S_IWUSR, NULL, cyapa_update_fw_store);
> > +static DEVICE_ATTR(baseline, S_IRUGO, cyapa_show_baseline, NULL);
> > +static DEVICE_ATTR(calibrate, S_IWUSR, NULL, cyapa_calibrate_store);
> > +
> > +static struct attribute *cyapa_sysfs_entries[] = {
> > + &dev_attr_firmware_version.attr,
> > + &dev_attr_product_id.attr,
> > + &dev_attr_update_fw.attr,
> > + &dev_attr_baseline.attr,
> > + &dev_attr_calibrate.attr,
> > + NULL,
> > +};
> > +
> > +static const struct attribute_group cyapa_sysfs_group = {
> > + .attrs = cyapa_sysfs_entries,
> > +};
> > +
> > void cyapa_detect_async(void *data, async_cookie_t cookie)
> > {
> > struct cyapa *cyapa = (struct cyapa *)data;
> > @@ -680,6 +866,9 @@ static int cyapa_probe(struct i2c_client *client,
> > goto err_uninit_tp_modules;
> > }
> >
> > + if (sysfs_create_group(&client->dev.kobj, &cyapa_sysfs_group))
> > + dev_warn(dev, "error creating sysfs entries.\n");
> > +
> > #ifdef CONFIG_PM_SLEEP
> > if (device_can_wakeup(dev) &&
> > sysfs_merge_group(&client->dev.kobj, &cyapa_power_wakeup_group))
> > @@ -704,6 +893,7 @@ static int cyapa_remove(struct i2c_client *client)
> > struct cyapa *cyapa = i2c_get_clientdata(client);
> >
> > pm_runtime_disable(&client->dev);
> > + sysfs_remove_group(&client->dev.kobj, &cyapa_sysfs_group);
> >
> > #ifdef CONFIG_PM_SLEEP
> > sysfs_unmerge_group(&client->dev.kobj, &cyapa_power_wakeup_group);
> > diff --git a/drivers/input/mouse/cyapa.h b/drivers/input/mouse/cyapa.h
> > index 9c1f0b91..567ab08 100644
> > --- a/drivers/input/mouse/cyapa.h
> > +++ b/drivers/input/mouse/cyapa.h
> > @@ -171,6 +171,22 @@ struct cyapa;
> > typedef bool (*cb_sort)(struct cyapa *, u8 *, int);
> >
> > struct cyapa_dev_ops {
> > + int (*check_fw)(struct cyapa *, const struct firmware *);
> > + int (*bl_enter)(struct cyapa *);
> > + int (*bl_activate)(struct cyapa *);
> > + int (*bl_initiate)(struct cyapa *, const struct firmware *);
> > + int (*update_fw)(struct cyapa *, const struct firmware *);
> > + int (*bl_verify_app_integrity)(struct cyapa *);
> > + int (*bl_deactivate)(struct cyapa *);
> > +
> > + ssize_t (*show_baseline)(struct device *,
> > + struct device_attribute *, char *);
> > + ssize_t (*calibrate_store)(struct device *,
> > + struct device_attribute *, const char *, size_t);
> > +
> > + int (*read_fw)(struct cyapa *);
> > + int (*read_raw_data)(struct cyapa *);
> > +
> > int (*initialize)(struct cyapa *cyapa);
> > int (*uninitialize)(struct cyapa *cyapa);
> >
> > @@ -243,6 +259,17 @@ struct cyapa {
> > */
> > struct mutex state_sync_lock;
> >
> > + /* Per-instance debugfs root */
> > + struct dentry *dentry_dev;
> > +
> > + /* Buffer to store firmware read using debugfs */
> > + struct mutex debugfs_mutex;
> > + u8 *fw_image;
> > + size_t fw_image_size;
> > + /* Buffer to store sensors' raw data */
> > + u8 *tp_raw_data;
> > + size_t tp_raw_data_size;
> > +
> > const struct cyapa_dev_ops *ops;
> > };
> >
> > --
> > 1.7.9.5
> >
> >
>
> Thanks.
>
> --
> Dmitry
^ permalink raw reply
* RE: [PATCH v4 1/14] input: cyapa: re-architecture driver to support multi-trackpads in one driver
From: Dudley Du @ 2014-08-22 8:41 UTC (permalink / raw)
To: 'Dmitry Torokhov', Dudley Du
Cc: Rafael J. Wysocki, Benson Leung, Patrik Fimml, linux-input,
linux-kernel
In-Reply-To: <20140821182101.GC5854@core.coreip.homeip.net>
Dmitry,
Thanks for your feedback.
Thanks,
Dudley
> -----Original Message-----
> From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com]
> Sent: Friday, August 22, 2014 2:21 AM
> To: Dudley Du
> Cc: Rafael J. Wysocki; Benson Leung; Patrik Fimml; linux-input@vger.kernel.org;
> linux-kernel@vger.kernel.org; Dudley Du
> Subject: Re: [PATCH v4 1/14] input: cyapa: re-architecture driver to support
> multi-trackpads in one driver
>
> On Thu, Jul 17, 2014 at 02:47:24PM +0800, Dudley Du wrote:
> > In order to support two different communication protocol based trackpad
> > device in one cyapa, the new cyapa driver is re-designed with
> > one cyapa driver core and two devices' functions component.
> > The cyapa driver core is contained in this patch, it supplies the basic
> > function with input and kernel system and also defined the interfaces
> > that the devices' functions component needs to apply and support.
> > Also, in order to speed up the system boot time, the device states
> > detecting and probing process is put into the async thread.
> > TEST=test on Chromebooks.
> >
> > Signed-off-by: Dudley Du <dudl@cypress.com>
> > ---
> > drivers/input/mouse/Makefile | 4 +-
> > drivers/input/mouse/cyapa.c | 1083 ++++++++++++++-------------------------
> ---
> > drivers/input/mouse/cyapa.h | 275 +++++++++++
> > 3 files changed, 646 insertions(+), 716 deletions(-)
> > create mode 100644 drivers/input/mouse/cyapa.h
> >
> > diff --git a/drivers/input/mouse/Makefile b/drivers/input/mouse/Makefile
> > index c25efdb..8608eb7 100644
> > --- a/drivers/input/mouse/Makefile
> > +++ b/drivers/input/mouse/Makefile
> > @@ -8,7 +8,7 @@ obj-$(CONFIG_MOUSE_AMIGA) += amimouse.o
> > obj-$(CONFIG_MOUSE_APPLETOUCH) += appletouch.o
> > obj-$(CONFIG_MOUSE_ATARI) += atarimouse.o
> > obj-$(CONFIG_MOUSE_BCM5974) += bcm5974.o
> > -obj-$(CONFIG_MOUSE_CYAPA) += cyapa.o
> > +obj-$(CONFIG_MOUSE_CYAPA) += cyapatp.o
> > obj-$(CONFIG_MOUSE_GPIO) += gpio_mouse.o
> > obj-$(CONFIG_MOUSE_INPORT) += inport.o
> > obj-$(CONFIG_MOUSE_LOGIBM) += logibm.o
> > @@ -34,3 +34,5 @@ psmouse-$(CONFIG_MOUSE_PS2_SENTELIC) += sentelic.o
> > psmouse-$(CONFIG_MOUSE_PS2_TRACKPOINT) += trackpoint.o
> > psmouse-$(CONFIG_MOUSE_PS2_TOUCHKIT) += touchkit_ps2.o
> > psmouse-$(CONFIG_MOUSE_PS2_CYPRESS) += cypress_ps2.o
> > +
> > +cyapatp-y := cyapa.o
> > diff --git a/drivers/input/mouse/cyapa.c b/drivers/input/mouse/cyapa.c
> > index b409c3d..5fc8dbe 100644
> > --- a/drivers/input/mouse/cyapa.c
> > +++ b/drivers/input/mouse/cyapa.c
> > @@ -6,7 +6,7 @@
> > * Daniel Kurtz <djkurtz@chromium.org>
> > * Benson Leung <bleung@chromium.org>
> > *
> > - * Copyright (C) 2011-2012 Cypress Semiconductor, Inc.
> > + * Copyright (C) 2011-2014 Cypress Semiconductor, Inc.
> > * Copyright (C) 2011-2012 Google, Inc.
> > *
> > * This file is subject to the terms and conditions of the GNU General
> Public
> > @@ -14,731 +14,114 @@
> > * more details.
> > */
> >
> > +#include <linux/debugfs.h>
> > #include <linux/delay.h>
> > #include <linux/i2c.h>
> > #include <linux/input.h>
> > #include <linux/input/mt.h>
> > #include <linux/interrupt.h>
> > #include <linux/module.h>
> > +#include <linux/mutex.h>
> > #include <linux/slab.h>
> > +#include <linux/uaccess.h>
> > +#include <linux/pm_runtime.h>
> > +#include "cyapa.h"
> >
> > -/* APA trackpad firmware generation */
> > -#define CYAPA_GEN3 0x03 /* support MT-protocol B with tracking ID. */
> > -
> > -#define CYAPA_NAME "Cypress APA Trackpad (cyapa)"
> > -
> > -/* commands for read/write registers of Cypress trackpad */
> > -#define CYAPA_CMD_SOFT_RESET 0x00
> > -#define CYAPA_CMD_POWER_MODE 0x01
> > -#define CYAPA_CMD_DEV_STATUS 0x02
> > -#define CYAPA_CMD_GROUP_DATA 0x03
> > -#define CYAPA_CMD_GROUP_CMD 0x04
> > -#define CYAPA_CMD_GROUP_QUERY 0x05
> > -#define CYAPA_CMD_BL_STATUS 0x06
> > -#define CYAPA_CMD_BL_HEAD 0x07
> > -#define CYAPA_CMD_BL_CMD 0x08
> > -#define CYAPA_CMD_BL_DATA 0x09
> > -#define CYAPA_CMD_BL_ALL 0x0a
> > -#define CYAPA_CMD_BLK_PRODUCT_ID 0x0b
> > -#define CYAPA_CMD_BLK_HEAD 0x0c
> > -
> > -/* report data start reg offset address. */
> > -#define DATA_REG_START_OFFSET 0x0000
> > -
> > -#define BL_HEAD_OFFSET 0x00
> > -#define BL_DATA_OFFSET 0x10
> > -
> > -/*
> > - * Operational Device Status Register
> > - *
> > - * bit 7: Valid interrupt source
> > - * bit 6 - 4: Reserved
> > - * bit 3 - 2: Power status
> > - * bit 1 - 0: Device status
> > - */
> > -#define REG_OP_STATUS 0x00
> > -#define OP_STATUS_SRC 0x80
> > -#define OP_STATUS_POWER 0x0c
> > -#define OP_STATUS_DEV 0x03
> > -#define OP_STATUS_MASK (OP_STATUS_SRC | OP_STATUS_POWER | OP_STATUS_DEV)
> > -
> > -/*
> > - * Operational Finger Count/Button Flags Register
> > - *
> > - * bit 7 - 4: Number of touched finger
> > - * bit 3: Valid data
> > - * bit 2: Middle Physical Button
> > - * bit 1: Right Physical Button
> > - * bit 0: Left physical Button
> > - */
> > -#define REG_OP_DATA1 0x01
> > -#define OP_DATA_VALID 0x08
> > -#define OP_DATA_MIDDLE_BTN 0x04
> > -#define OP_DATA_RIGHT_BTN 0x02
> > -#define OP_DATA_LEFT_BTN 0x01
> > -#define OP_DATA_BTN_MASK (OP_DATA_MIDDLE_BTN | OP_DATA_RIGHT_BTN | \
> > - OP_DATA_LEFT_BTN)
> > -
> > -/*
> > - * Bootloader Status Register
> > - *
> > - * bit 7: Busy
> > - * bit 6 - 5: Reserved
> > - * bit 4: Bootloader running
> > - * bit 3 - 1: Reserved
> > - * bit 0: Checksum valid
> > - */
> > -#define REG_BL_STATUS 0x01
> > -#define BL_STATUS_BUSY 0x80
> > -#define BL_STATUS_RUNNING 0x10
> > -#define BL_STATUS_DATA_VALID 0x08
> > -#define BL_STATUS_CSUM_VALID 0x01
> > -
> > -/*
> > - * Bootloader Error Register
> > - *
> > - * bit 7: Invalid
> > - * bit 6: Invalid security key
> > - * bit 5: Bootloading
> > - * bit 4: Command checksum
> > - * bit 3: Flash protection error
> > - * bit 2: Flash checksum error
> > - * bit 1 - 0: Reserved
> > - */
> > -#define REG_BL_ERROR 0x02
> > -#define BL_ERROR_INVALID 0x80
> > -#define BL_ERROR_INVALID_KEY 0x40
> > -#define BL_ERROR_BOOTLOADING 0x20
> > -#define BL_ERROR_CMD_CSUM 0x10
> > -#define BL_ERROR_FLASH_PROT 0x08
> > -#define BL_ERROR_FLASH_CSUM 0x04
> > -
> > -#define BL_STATUS_SIZE 3 /* length of bootloader status registers */
> > -#define BLK_HEAD_BYTES 32
> > -
> > -#define PRODUCT_ID_SIZE 16
> > -#define QUERY_DATA_SIZE 27
> > -#define REG_PROTOCOL_GEN_QUERY_OFFSET 20
> > -
> > -#define REG_OFFSET_DATA_BASE 0x0000
> > -#define REG_OFFSET_COMMAND_BASE 0x0028
> > -#define REG_OFFSET_QUERY_BASE 0x002a
> > -
> > -#define CAPABILITY_LEFT_BTN_MASK (0x01 << 3)
> > -#define CAPABILITY_RIGHT_BTN_MASK (0x01 << 4)
> > -#define CAPABILITY_MIDDLE_BTN_MASK (0x01 << 5)
> > -#define CAPABILITY_BTN_MASK (CAPABILITY_LEFT_BTN_MASK | \
> > - CAPABILITY_RIGHT_BTN_MASK | \
> > - CAPABILITY_MIDDLE_BTN_MASK)
> > -
> > -#define CYAPA_OFFSET_SOFT_RESET REG_OFFSET_COMMAND_BASE
> > -
> > -#define REG_OFFSET_POWER_MODE (REG_OFFSET_COMMAND_BASE + 1)
> > -
> > -#define PWR_MODE_MASK 0xfc
> > -#define PWR_MODE_FULL_ACTIVE (0x3f << 2)
> > -#define PWR_MODE_IDLE (0x05 << 2) /* default sleep time is 50 ms. */
> > -#define PWR_MODE_OFF (0x00 << 2)
> > -
> > -#define PWR_STATUS_MASK 0x0c
> > -#define PWR_STATUS_ACTIVE (0x03 << 2)
> > -#define PWR_STATUS_IDLE (0x02 << 2)
> > -#define PWR_STATUS_OFF (0x00 << 2)
> > -
> > -/*
> > - * CYAPA trackpad device states.
> > - * Used in register 0x00, bit1-0, DeviceStatus field.
> > - * Other values indicate device is in an abnormal state and must be reset.
> > - */
> > -#define CYAPA_DEV_NORMAL 0x03
> > -#define CYAPA_DEV_BUSY 0x01
> > -
> > -enum cyapa_state {
> > - CYAPA_STATE_OP,
> > - CYAPA_STATE_BL_IDLE,
> > - CYAPA_STATE_BL_ACTIVE,
> > - CYAPA_STATE_BL_BUSY,
> > - CYAPA_STATE_NO_DEVICE,
> > -};
> > -
> > -
> > -struct cyapa_touch {
> > - /*
> > - * high bits or x/y position value
> > - * bit 7 - 4: high 4 bits of x position value
> > - * bit 3 - 0: high 4 bits of y position value
> > - */
> > - u8 xy_hi;
> > - u8 x_lo; /* low 8 bits of x position value. */
> > - u8 y_lo; /* low 8 bits of y position value. */
> > - u8 pressure;
> > - /* id range is 1 - 15. It is incremented with every new touch. */
> > - u8 id;
> > -} __packed;
> > -
> > -/* The touch.id is used as the MT slot id, thus max MT slot is 15 */
> > -#define CYAPA_MAX_MT_SLOTS 15
> > -
> > -struct cyapa_reg_data {
> > - /*
> > - * bit 0 - 1: device status
> > - * bit 3 - 2: power mode
> > - * bit 6 - 4: reserved
> > - * bit 7: interrupt valid bit
> > - */
> > - u8 device_status;
> > - /*
> > - * bit 7 - 4: number of fingers currently touching pad
> > - * bit 3: valid data check bit
> > - * bit 2: middle mechanism button state if exists
> > - * bit 1: right mechanism button state if exists
> > - * bit 0: left mechanism button state if exists
> > - */
> > - u8 finger_btn;
> > - /* CYAPA reports up to 5 touches per packet. */
> > - struct cyapa_touch touches[5];
> > -} __packed;
> > -
> > -/* The main device structure */
> > -struct cyapa {
> > - enum cyapa_state state;
> > -
> > - struct i2c_client *client;
> > - struct input_dev *input;
> > - char phys[32]; /* device physical location */
> > - int irq;
> > - bool irq_wake; /* irq wake is enabled */
> > - bool smbus;
> > -
> > - /* read from query data region. */
> > - char product_id[16];
> > - u8 btn_capability;
> > - u8 gen;
> > - int max_abs_x;
> > - int max_abs_y;
> > - int physical_size_x;
> > - int physical_size_y;
> > -};
> > -
> > -static const u8 bl_deactivate[] = { 0x00, 0xff, 0x3b, 0x00, 0x01, 0x02,
> 0x03,
> > - 0x04, 0x05, 0x06, 0x07 };
> > -static const u8 bl_exit[] = { 0x00, 0xff, 0xa5, 0x00, 0x01, 0x02, 0x03,
> 0x04,
> > - 0x05, 0x06, 0x07 };
> > -
> > -struct cyapa_cmd_len {
> > - u8 cmd;
> > - u8 len;
> > -};
> >
> > #define CYAPA_ADAPTER_FUNC_NONE 0
> > #define CYAPA_ADAPTER_FUNC_I2C 1
> > #define CYAPA_ADAPTER_FUNC_SMBUS 2
> > #define CYAPA_ADAPTER_FUNC_BOTH 3
> >
> > -/*
> > - * macros for SMBus communication
> > - */
> > -#define SMBUS_READ 0x01
> > -#define SMBUS_WRITE 0x00
> > -#define SMBUS_ENCODE_IDX(cmd, idx) ((cmd) | (((idx) & 0x03) << 1))
> > -#define SMBUS_ENCODE_RW(cmd, rw) ((cmd) | ((rw) & 0x01))
> > -#define SMBUS_BYTE_BLOCK_CMD_MASK 0x80
> > -#define SMBUS_GROUP_BLOCK_CMD_MASK 0x40
> > -
> > - /* for byte read/write command */
> > -#define CMD_RESET 0
> > -#define CMD_POWER_MODE 1
> > -#define CMD_DEV_STATUS 2
> > -#define SMBUS_BYTE_CMD(cmd) (((cmd) & 0x3f) << 1)
> > -#define CYAPA_SMBUS_RESET SMBUS_BYTE_CMD(CMD_RESET)
> > -#define CYAPA_SMBUS_POWER_MODE SMBUS_BYTE_CMD(CMD_POWER_MODE)
> > -#define CYAPA_SMBUS_DEV_STATUS SMBUS_BYTE_CMD(CMD_DEV_STATUS)
> > -
> > - /* for group registers read/write command */
> > -#define REG_GROUP_DATA 0
> > -#define REG_GROUP_CMD 2
> > -#define REG_GROUP_QUERY 3
> > -#define SMBUS_GROUP_CMD(grp) (0x80 | (((grp) & 0x07) << 3))
> > -#define CYAPA_SMBUS_GROUP_DATA SMBUS_GROUP_CMD(REG_GROUP_DATA)
> > -#define CYAPA_SMBUS_GROUP_CMD SMBUS_GROUP_CMD(REG_GROUP_CMD)
> > -#define CYAPA_SMBUS_GROUP_QUERY SMBUS_GROUP_CMD(REG_GROUP_QUERY)
> > -
> > - /* for register block read/write command */
> > -#define CMD_BL_STATUS 0
> > -#define CMD_BL_HEAD 1
> > -#define CMD_BL_CMD 2
> > -#define CMD_BL_DATA 3
> > -#define CMD_BL_ALL 4
> > -#define CMD_BLK_PRODUCT_ID 5
> > -#define CMD_BLK_HEAD 6
> > -#define SMBUS_BLOCK_CMD(cmd) (0xc0 | (((cmd) & 0x1f) << 1))
> > -
> > -/* register block read/write command in bootloader mode */
> > -#define CYAPA_SMBUS_BL_STATUS SMBUS_BLOCK_CMD(CMD_BL_STATUS)
> > -#define CYAPA_SMBUS_BL_HEAD SMBUS_BLOCK_CMD(CMD_BL_HEAD)
> > -#define CYAPA_SMBUS_BL_CMD SMBUS_BLOCK_CMD(CMD_BL_CMD)
> > -#define CYAPA_SMBUS_BL_DATA SMBUS_BLOCK_CMD(CMD_BL_DATA)
> > -#define CYAPA_SMBUS_BL_ALL SMBUS_BLOCK_CMD(CMD_BL_ALL)
> > -
> > -/* register block read/write command in operational mode */
> > -#define CYAPA_SMBUS_BLK_PRODUCT_ID SMBUS_BLOCK_CMD(CMD_BLK_PRODUCT_ID)
> > -#define CYAPA_SMBUS_BLK_HEAD SMBUS_BLOCK_CMD(CMD_BLK_HEAD)
> > -
> > -static const struct cyapa_cmd_len cyapa_i2c_cmds[] = {
> > - { CYAPA_OFFSET_SOFT_RESET, 1 },
> > - { REG_OFFSET_COMMAND_BASE + 1, 1 },
> > - { REG_OFFSET_DATA_BASE, 1 },
> > - { REG_OFFSET_DATA_BASE, sizeof(struct cyapa_reg_data) },
> > - { REG_OFFSET_COMMAND_BASE, 0 },
> > - { REG_OFFSET_QUERY_BASE, QUERY_DATA_SIZE },
> > - { BL_HEAD_OFFSET, 3 },
> > - { BL_HEAD_OFFSET, 16 },
> > - { BL_HEAD_OFFSET, 16 },
> > - { BL_DATA_OFFSET, 16 },
> > - { BL_HEAD_OFFSET, 32 },
> > - { REG_OFFSET_QUERY_BASE, PRODUCT_ID_SIZE },
> > - { REG_OFFSET_DATA_BASE, 32 }
> > -};
> > +#define CYAPA_DEBUGFS_READ_FW "read_fw"
> > +#define CYAPA_DEBUGFS_RAW_DATA "raw_data"
> > +#define CYAPA_FW_NAME "cyapa.bin"
> > +
> > +const char unique_str[] = "CYTRA";
> > +
> >
> > -static const struct cyapa_cmd_len cyapa_smbus_cmds[] = {
> > - { CYAPA_SMBUS_RESET, 1 },
> > - { CYAPA_SMBUS_POWER_MODE, 1 },
> > - { CYAPA_SMBUS_DEV_STATUS, 1 },
> > - { CYAPA_SMBUS_GROUP_DATA, sizeof(struct cyapa_reg_data) },
> > - { CYAPA_SMBUS_GROUP_CMD, 2 },
> > - { CYAPA_SMBUS_GROUP_QUERY, QUERY_DATA_SIZE },
> > - { CYAPA_SMBUS_BL_STATUS, 3 },
> > - { CYAPA_SMBUS_BL_HEAD, 16 },
> > - { CYAPA_SMBUS_BL_CMD, 16 },
> > - { CYAPA_SMBUS_BL_DATA, 16 },
> > - { CYAPA_SMBUS_BL_ALL, 32 },
> > - { CYAPA_SMBUS_BLK_PRODUCT_ID, PRODUCT_ID_SIZE },
> > - { CYAPA_SMBUS_BLK_HEAD, 16 },
> > -};
> >
> > -static ssize_t cyapa_i2c_reg_read_block(struct cyapa *cyapa, u8 reg, size_t
> len,
> > +ssize_t cyapa_i2c_reg_read_block(struct cyapa *cyapa, u8 reg, size_t len,
> > u8 *values)
> > {
> > return i2c_smbus_read_i2c_block_data(cyapa->client, reg, len, values);
> > }
> >
> > -static ssize_t cyapa_i2c_reg_write_block(struct cyapa *cyapa, u8 reg,
> > +ssize_t cyapa_i2c_reg_write_block(struct cyapa *cyapa, u8 reg,
> > size_t len, const u8 *values)
> > {
> > return i2c_smbus_write_i2c_block_data(cyapa->client, reg, len, values);
> > }
> >
> > -/*
> > - * cyapa_smbus_read_block - perform smbus block read command
> > - * @cyapa - private data structure of the driver
> > - * @cmd - the properly encoded smbus command
> > - * @len - expected length of smbus command result
> > - * @values - buffer to store smbus command result
> > - *
> > - * Returns negative errno, else the number of bytes written.
> > - *
> > - * Note:
> > - * In trackpad device, the memory block allocated for I2C register map
> > - * is 256 bytes, so the max read block for I2C bus is 256 bytes.
> > - */
> > -static ssize_t cyapa_smbus_read_block(struct cyapa *cyapa, u8 cmd, size_t
> len,
> > - u8 *values)
> > -{
> > - ssize_t ret;
> > - u8 index;
> > - u8 smbus_cmd;
> > - u8 *buf;
> > - struct i2c_client *client = cyapa->client;
> > -
> > - if (!(SMBUS_BYTE_BLOCK_CMD_MASK & cmd))
> > - return -EINVAL;
> > -
> > - if (SMBUS_GROUP_BLOCK_CMD_MASK & cmd) {
> > - /* read specific block registers command. */
> > - smbus_cmd = SMBUS_ENCODE_RW(cmd, SMBUS_READ);
> > - ret = i2c_smbus_read_block_data(client, smbus_cmd, values);
> > - goto out;
> > - }
> > -
> > - ret = 0;
> > - for (index = 0; index * I2C_SMBUS_BLOCK_MAX < len; index++) {
> > - smbus_cmd = SMBUS_ENCODE_IDX(cmd, index);
> > - smbus_cmd = SMBUS_ENCODE_RW(smbus_cmd, SMBUS_READ);
> > - buf = values + I2C_SMBUS_BLOCK_MAX * index;
> > - ret = i2c_smbus_read_block_data(client, smbus_cmd, buf);
> > - if (ret < 0)
> > - goto out;
> > - }
> > -
> > -out:
> > - return ret > 0 ? len : ret;
> > -}
> > -
> > -static s32 cyapa_read_byte(struct cyapa *cyapa, u8 cmd_idx)
> > -{
> > - u8 cmd;
> > -
> > - if (cyapa->smbus) {
> > - cmd = cyapa_smbus_cmds[cmd_idx].cmd;
> > - cmd = SMBUS_ENCODE_RW(cmd, SMBUS_READ);
> > - } else {
> > - cmd = cyapa_i2c_cmds[cmd_idx].cmd;
> > - }
> > - return i2c_smbus_read_byte_data(cyapa->client, cmd);
> > -}
> > -
> > -static s32 cyapa_write_byte(struct cyapa *cyapa, u8 cmd_idx, u8 value)
> > -{
> > - u8 cmd;
> > -
> > - if (cyapa->smbus) {
> > - cmd = cyapa_smbus_cmds[cmd_idx].cmd;
> > - cmd = SMBUS_ENCODE_RW(cmd, SMBUS_WRITE);
> > - } else {
> > - cmd = cyapa_i2c_cmds[cmd_idx].cmd;
> > - }
> > - return i2c_smbus_write_byte_data(cyapa->client, cmd, value);
> > -}
> > -
> > -static ssize_t cyapa_read_block(struct cyapa *cyapa, u8 cmd_idx, u8 *values)
> > -{
> > - u8 cmd;
> > - size_t len;
> > -
> > - if (cyapa->smbus) {
> > - cmd = cyapa_smbus_cmds[cmd_idx].cmd;
> > - len = cyapa_smbus_cmds[cmd_idx].len;
> > - return cyapa_smbus_read_block(cyapa, cmd, len, values);
> > - } else {
> > - cmd = cyapa_i2c_cmds[cmd_idx].cmd;
> > - len = cyapa_i2c_cmds[cmd_idx].len;
> > - return cyapa_i2c_reg_read_block(cyapa, cmd, len, values);
> > - }
> > -}
> > -
> > -/*
> > - * Query device for its current operating state.
> > - *
> > - */
> > -static int cyapa_get_state(struct cyapa *cyapa)
> > +/* Returns 0 on success, else negative errno on failure. */
> > +ssize_t cyapa_i2c_read(struct cyapa *cyapa, u8 reg, size_t len,
> > + u8 *values)
> > {
> > int ret;
> > - u8 status[BL_STATUS_SIZE];
> > -
> > - cyapa->state = CYAPA_STATE_NO_DEVICE;
> > -
> > - /*
> > - * Get trackpad status by reading 3 registers starting from 0.
> > - * If the device is in the bootloader, this will be BL_HEAD.
> > - * If the device is in operation mode, this will be the DATA regs.
> > - *
> > - */
> > - ret = cyapa_i2c_reg_read_block(cyapa, BL_HEAD_OFFSET, BL_STATUS_SIZE,
> > - status);
> > -
> > - /*
> > - * On smbus systems in OP mode, the i2c_reg_read will fail with
> > - * -ETIMEDOUT. In this case, try again using the smbus equivalent
> > - * command. This should return a BL_HEAD indicating CYAPA_STATE_OP.
> > - */
> > - if (cyapa->smbus && (ret == -ETIMEDOUT || ret == -ENXIO))
> > - ret = cyapa_read_block(cyapa, CYAPA_CMD_BL_STATUS, status);
> > -
> > - if (ret != BL_STATUS_SIZE)
> > - goto error;
> > -
> > - if ((status[REG_OP_STATUS] & OP_STATUS_SRC) == OP_STATUS_SRC) {
> > - switch (status[REG_OP_STATUS] & OP_STATUS_DEV) {
> > - case CYAPA_DEV_NORMAL:
> > - case CYAPA_DEV_BUSY:
> > - cyapa->state = CYAPA_STATE_OP;
> > - break;
> > - default:
> > - ret = -EAGAIN;
> > - goto error;
> > - }
> > - } else {
> > - if (status[REG_BL_STATUS] & BL_STATUS_BUSY)
> > - cyapa->state = CYAPA_STATE_BL_BUSY;
> > - else if (status[REG_BL_ERROR] & BL_ERROR_BOOTLOADING)
> > - cyapa->state = CYAPA_STATE_BL_ACTIVE;
> > - else
> > - cyapa->state = CYAPA_STATE_BL_IDLE;
> > - }
> > -
> > - return 0;
> > -error:
> > - return (ret < 0) ? ret : -EAGAIN;
> > + struct i2c_client *client = cyapa->client;
> > + struct i2c_msg msgs[] = {
> > + {
> > + .addr = client->addr,
> > + .flags = 0,
> > + .len = 1,
> > + .buf = ®,
> > + },
> > + {
> > + .addr = client->addr,
> > + .flags = I2C_M_RD,
> > + .len = len,
> > + .buf = values,
> > + },
> > + };
> > +
> > + ret = i2c_transfer(client->adapter, msgs, 2);
>
> You need to watch out for partial transfers. It should be:
>
> if (ret != ARRAY_SIZE(msgs))
> return ret < 0 ? ret : -EIO;
>
> return 0;
>
Thanks, I will update in next release.
> > +
> > + return ret < 0 ? ret : 0;
> > }
> >
> > -/*
> > - * Poll device for its status in a loop, waiting up to timeout for a
> response.
> > - *
> > - * When the device switches state, it usually takes ~300 ms.
> > - * However, when running a new firmware image, the device must calibrate
> its
> > - * sensors, which can take as long as 2 seconds.
> > +/**
> > + * cyapa_i2c_write - Execute i2c block data write operation
> > + * @cyapa: Handle to this driver
> > + * @ret: Offset of the data to written in the register map
> > + * @len: number of bytes to write
> > + * @values: Data to be written
> > *
> > - * Note: The timeout has granularity of the polling rate, which is 100 ms.
> > - *
> > - * Returns:
> > - * 0 when the device eventually responds with a valid non-busy state.
> > - * -ETIMEDOUT if device never responds (too many -EAGAIN)
> > - * < 0 other errors
> > + * Return negative errno code on error; return zero when success.
> > */
> > -static int cyapa_poll_state(struct cyapa *cyapa, unsigned int timeout)
> > +ssize_t cyapa_i2c_write(struct cyapa *cyapa, u8 reg,
> > + size_t len, const void *values)
> > {
> > int ret;
> > - int tries = timeout / 100;
> > -
> > - ret = cyapa_get_state(cyapa);
> > - while ((ret || cyapa->state >= CYAPA_STATE_BL_BUSY) && tries--) {
> > - msleep(100);
> > - ret = cyapa_get_state(cyapa);
> > - }
> > - return (ret == -EAGAIN || ret == -ETIMEDOUT) ? -ETIMEDOUT : ret;
> > -}
> > + struct i2c_client *client = cyapa->client;
> > + char data[32], *buf;
> >
> > -static int cyapa_bl_deactivate(struct cyapa *cyapa)
> > -{
> > - int ret;
> > + if (len > 31)
> > + buf = kzalloc(len + 1, GFP_KERNEL);
> > + else
> > + buf = data;
> >
> > - ret = cyapa_i2c_reg_write_block(cyapa, 0, sizeof(bl_deactivate),
> > - bl_deactivate);
> > - if (ret < 0)
> > - return ret;
> > + buf[0] = reg;
> > + memcpy(&buf[1], values, len);
> > + ret = i2c_master_send(client, buf, len + 1);
> >
> > - /* wait for bootloader to switch to idle state; should take < 100ms */
> > - msleep(100);
> > - ret = cyapa_poll_state(cyapa, 500);
> > - if (ret < 0)
> > - return ret;
> > - if (cyapa->state != CYAPA_STATE_BL_IDLE)
> > - return -EAGAIN;
> > - return 0;
> > + if (buf != data)
> > + kfree(buf);
> > + return (ret == (len + 1)) ? 0 : ((ret < 0) ? ret : -EIO);
> > }
> >
> > -/*
> > - * Exit bootloader
> > - *
> > - * Send bl_exit command, then wait 50 - 100 ms to let device transition to
> > - * operational mode. If this is the first time the device's firmware is
> > - * running, it can take up to 2 seconds to calibrate its sensors. So, poll
> > - * the device's new state for up to 2 seconds.
> > - *
> > - * Returns:
> > - * -EIO failure while reading from device
> > - * -EAGAIN device is stuck in bootloader, b/c it has invalid firmware
> > - * 0 device is supported and in operational mode
> > - */
> > -static int cyapa_bl_exit(struct cyapa *cyapa)
> > +void cyapa_default_irq_handler(struct cyapa *cyapa)
> > {
> > - int ret;
> > -
> > - ret = cyapa_i2c_reg_write_block(cyapa, 0, sizeof(bl_exit), bl_exit);
> > - if (ret < 0)
> > - return ret;
> > -
> > - /*
> > - * Wait for bootloader to exit, and operation mode to start.
> > - * Normally, this takes at least 50 ms.
> > - */
> > - usleep_range(50000, 100000);
> > /*
> > - * In addition, when a device boots for the first time after being
> > - * updated to new firmware, it must first calibrate its sensors, which
> > - * can take up to an additional 2 seconds.
> > + * Do redetecting when device states is still unknown and
> > + * interrupt envent is received from device.
> > */
> > - ret = cyapa_poll_state(cyapa, 2000);
> > - if (ret < 0)
> > - return ret;
> > - if (cyapa->state != CYAPA_STATE_OP)
> > - return -EAGAIN;
> > -
> > - return 0;
> > -}
> > -
> > -/*
> > - * Set device power mode
> > - *
> > - */
> > -static int cyapa_set_power_mode(struct cyapa *cyapa, u8 power_mode)
> > -{
> > - struct device *dev = &cyapa->client->dev;
> > - int ret;
> > - u8 power;
> > -
> > - if (cyapa->state != CYAPA_STATE_OP)
> > - return 0;
> > -
> > - ret = cyapa_read_byte(cyapa, CYAPA_CMD_POWER_MODE);
> > - if (ret < 0)
> > - return ret;
> > -
> > - power = ret & ~PWR_MODE_MASK;
> > - power |= power_mode & PWR_MODE_MASK;
> > - ret = cyapa_write_byte(cyapa, CYAPA_CMD_POWER_MODE, power);
> > - if (ret < 0)
> > - dev_err(dev, "failed to set power_mode 0x%02x err = %d\n",
> > - power_mode, ret);
> > - return ret;
> > -}
> > -
> > -static int cyapa_get_query_data(struct cyapa *cyapa)
> > -{
> > - u8 query_data[QUERY_DATA_SIZE];
> > - int ret;
> > -
> > - if (cyapa->state != CYAPA_STATE_OP)
> > - return -EBUSY;
> > -
> > - ret = cyapa_read_block(cyapa, CYAPA_CMD_GROUP_QUERY, query_data);
> > - if (ret < 0)
> > - return ret;
> > - if (ret != QUERY_DATA_SIZE)
> > - return -EIO;
> > -
> > - memcpy(&cyapa->product_id[0], &query_data[0], 5);
> > - cyapa->product_id[5] = '-';
> > - memcpy(&cyapa->product_id[6], &query_data[5], 6);
> > - cyapa->product_id[12] = '-';
> > - memcpy(&cyapa->product_id[13], &query_data[11], 2);
> > - cyapa->product_id[15] = '\0';
> > -
> > - cyapa->btn_capability = query_data[19] & CAPABILITY_BTN_MASK;
> > -
> > - cyapa->gen = query_data[20] & 0x0f;
> > -
> > - cyapa->max_abs_x = ((query_data[21] & 0xf0) << 4) | query_data[22];
> > - cyapa->max_abs_y = ((query_data[21] & 0x0f) << 8) | query_data[23];
> > -
> > - cyapa->physical_size_x =
> > - ((query_data[24] & 0xf0) << 4) | query_data[25];
> > - cyapa->physical_size_y =
> > - ((query_data[24] & 0x0f) << 8) | query_data[26];
> > -
> > - return 0;
> > + async_schedule(cyapa_detect_async, cyapa);
>
> I think I already asked this before - why do we need to try and schedule async
> detect from interrupt handler. In what cases we will fail to initialize the
> device during normal probing, but then are fine when stray interrupt arrives?
>
1) Because gen5 TP devices use interrupt to sync the command and response, and
in detecting, some commands must be sent and executed in attached devices, so
the interrupt must be usable in detecting.
So if do not schedule async detect from interrupt handler, the interrupt
handler will be taken, and cannot enter again, which will cause the command to
the device failed, it will also cause long time detecting.
2) detecting will fail when no device attached or the attached device is not
supported gen3 or gen5 TP devices or there is some issue with the device
that cannot enter into active working mode or stay in bootloader mode
for invalid firmware detected.
And it's tested that it's save when stray interrupt arrives in detecting.
Thanks.
> > }
> >
> > -/*
> > - * Check if device is operational.
> > - *
> > - * An operational device is responding, has exited bootloader, and has
> > - * firmware supported by this driver.
> > - *
> > - * Returns:
> > - * -EBUSY no device or in bootloader
> > - * -EIO failure while reading from device
> > - * -EAGAIN device is still in bootloader
> > - * if ->state = CYAPA_STATE_BL_IDLE, device has invalid firmware
> > - * -EINVAL device is in operational mode, but not supported by this
> driver
> > - * 0 device is supported
> > - */
> > -static int cyapa_check_is_operational(struct cyapa *cyapa)
> > -{
> > - struct device *dev = &cyapa->client->dev;
> > - static const char unique_str[] = "CYTRA";
> > - int ret;
> > -
> > - ret = cyapa_poll_state(cyapa, 2000);
> > - if (ret < 0)
> > - return ret;
> > - switch (cyapa->state) {
> > - case CYAPA_STATE_BL_ACTIVE:
> > - ret = cyapa_bl_deactivate(cyapa);
> > - if (ret)
> > - return ret;
> > -
> > - /* Fallthrough state */
> > - case CYAPA_STATE_BL_IDLE:
> > - ret = cyapa_bl_exit(cyapa);
> > - if (ret)
> > - return ret;
> > -
> > - /* Fallthrough state */
> > - case CYAPA_STATE_OP:
> > - ret = cyapa_get_query_data(cyapa);
> > - if (ret < 0)
> > - return ret;
> > -
> > - /* only support firmware protocol gen3 */
> > - if (cyapa->gen != CYAPA_GEN3) {
> > - dev_err(dev, "unsupported protocol version (%d)",
> > - cyapa->gen);
> > - return -EINVAL;
> > - }
> > -
> > - /* only support product ID starting with CYTRA */
> > - if (memcmp(cyapa->product_id, unique_str,
> > - sizeof(unique_str) - 1) != 0) {
> > - dev_err(dev, "unsupported product ID (%s)\n",
> > - cyapa->product_id);
> > - return -EINVAL;
> > - }
> > - return 0;
> > -
> > - default:
> > - return -EIO;
> > - }
> > - return 0;
> > -}
> > -
> > -static irqreturn_t cyapa_irq(int irq, void *dev_id)
> > -{
> > - struct cyapa *cyapa = dev_id;
> > - struct device *dev = &cyapa->client->dev;
> > - struct input_dev *input = cyapa->input;
> > - struct cyapa_reg_data data;
> > - int i;
> > - int ret;
> > - int num_fingers;
> > -
> > - if (device_may_wakeup(dev))
> > - pm_wakeup_event(dev, 0);
> > -
> > - ret = cyapa_read_block(cyapa, CYAPA_CMD_GROUP_DATA, (u8 *)&data);
> > - if (ret != sizeof(data))
> > - goto out;
> > -
> > - if ((data.device_status & OP_STATUS_SRC) != OP_STATUS_SRC ||
> > - (data.device_status & OP_STATUS_DEV) != CYAPA_DEV_NORMAL ||
> > - (data.finger_btn & OP_DATA_VALID) != OP_DATA_VALID) {
> > - goto out;
> > - }
> > -
> > - num_fingers = (data.finger_btn >> 4) & 0x0f;
> > - for (i = 0; i < num_fingers; i++) {
> > - const struct cyapa_touch *touch = &data.touches[i];
> > - /* Note: touch->id range is 1 to 15; slots are 0 to 14. */
> > - int slot = touch->id - 1;
> > -
> > - input_mt_slot(input, slot);
> > - input_mt_report_slot_state(input, MT_TOOL_FINGER, true);
> > - input_report_abs(input, ABS_MT_POSITION_X,
> > - ((touch->xy_hi & 0xf0) << 4) | touch->x_lo);
> > - input_report_abs(input, ABS_MT_POSITION_Y,
> > - ((touch->xy_hi & 0x0f) << 8) | touch->y_lo);
> > - input_report_abs(input, ABS_MT_PRESSURE, touch->pressure);
> > - }
> > -
> > - input_mt_sync_frame(input);
> > -
> > - if (cyapa->btn_capability & CAPABILITY_LEFT_BTN_MASK)
> > - input_report_key(input, BTN_LEFT,
> > - data.finger_btn & OP_DATA_LEFT_BTN);
> > -
> > - if (cyapa->btn_capability & CAPABILITY_MIDDLE_BTN_MASK)
> > - input_report_key(input, BTN_MIDDLE,
> > - data.finger_btn & OP_DATA_MIDDLE_BTN);
> > -
> > - if (cyapa->btn_capability & CAPABILITY_RIGHT_BTN_MASK)
> > - input_report_key(input, BTN_RIGHT,
> > - data.finger_btn & OP_DATA_RIGHT_BTN);
> > -
> > - input_sync(input);
> > +const struct cyapa_dev_ops cyapa_default_ops = {
> > + .irq_handler = cyapa_default_irq_handler
> > +};
> >
> > -out:
> > - return IRQ_HANDLED;
> > -}
> >
> > static u8 cyapa_check_adapter_functionality(struct i2c_client *client)
> > {
> > @@ -772,19 +155,40 @@ static int cyapa_create_input_dev(struct cyapa *cyapa)
> > input->phys = cyapa->phys;
> > input->id.bustype = BUS_I2C;
> > input->id.version = 1;
> > - input->id.product = 0; /* means any product in eventcomm. */
> > + input->id.product = 0; /* Means any product in eventcomm. */
> > input->dev.parent = &cyapa->client->dev;
> >
> > input_set_drvdata(input, cyapa);
> >
> > __set_bit(EV_ABS, input->evbit);
> >
> > - /* finger position */
> > + /* Finger position */
> > input_set_abs_params(input, ABS_MT_POSITION_X, 0, cyapa->max_abs_x, 0,
> > 0);
> > input_set_abs_params(input, ABS_MT_POSITION_Y, 0, cyapa->max_abs_y, 0,
> > 0);
> > - input_set_abs_params(input, ABS_MT_PRESSURE, 0, 255, 0, 0);
> > + input_set_abs_params(input, ABS_MT_PRESSURE, 0, cyapa->max_z, 0, 0);
> > + if (cyapa->gen > CYAPA_GEN3) {
> > + input_set_abs_params(input, ABS_MT_TOUCH_MAJOR, 0, 255, 0, 0);
> > + input_set_abs_params(input, ABS_MT_TOUCH_MINOR, 0, 255, 0, 0);
> > + /*
> > + * Orientation is the angle between the vertical axis and
> > + * the major axis of the contact ellipse.
> > + * The range is -127 to 127.
> > + * the positive direction is clockwise form the vertical axis.
> > + * If the ellipse of contact degenerates into a circle,
> > + * orientation is reported as 0.
> > + *
> > + * Also, for Gen5 trackpad the accurate of this orientation
> > + * value is value + (-30 ~ 30).
> > + */
> > + input_set_abs_params(input, ABS_MT_ORIENTATION,
> > + -127, 127, 0, 0);
> > + }
> > + if (cyapa->gen >= CYAPA_GEN5) {
> > + input_set_abs_params(input, ABS_MT_WIDTH_MAJOR, 0, 255, 0, 0);
> > + input_set_abs_params(input, ABS_MT_WIDTH_MINOR, 0, 255, 0, 0);
> > + }
> >
> > input_abs_set_res(input, ABS_MT_POSITION_X,
> > cyapa->max_abs_x / cyapa->physical_size_x);
> > @@ -801,7 +205,7 @@ static int cyapa_create_input_dev(struct cyapa *cyapa)
> > if (cyapa->btn_capability == CAPABILITY_LEFT_BTN_MASK)
> > __set_bit(INPUT_PROP_BUTTONPAD, input->propbit);
> >
> > - /* handle pointer emulation and unused slots in core */
> > + /* Handle pointer emulation and unused slots in core */
> > ret = input_mt_init_slots(input, CYAPA_MAX_MT_SLOTS,
> > INPUT_MT_POINTER | INPUT_MT_DROP_UNUSED);
> > if (ret) {
> > @@ -823,6 +227,224 @@ err_free_device:
> > return ret;
> > }
> >
> > +/*
> > + * Check if device is operational.
> > + *
> > + * An operational device is responding, has exited bootloader, and has
> > + * firmware supported by this driver.
> > + *
> > + * Returns:
> > + * -EBUSY no device or in bootloader
> > + * -EIO failure while reading from device
> > + * -EAGAIN device is still in bootloader
> > + * if ->state = CYAPA_STATE_BL_IDLE, device has invalid firmware
> > + * -EINVAL device is in operational mode, but not supported by this
> driver
> > + * 0 device is supported
> > + */
> > +static int cyapa_check_is_operational(struct cyapa *cyapa)
> > +{
> > + int ret;
> > +
> > + ret = cyapa_poll_state(cyapa, 4000);
> > + if (ret)
> > + return ret;
> > +
> > + switch (cyapa->gen) {
> > + default:
> > + cyapa->ops = &cyapa_default_ops;
> > + cyapa->gen = CYAPA_GEN_UNKNOWN;
> > + break;
> > + }
> > +
> > + if (cyapa->ops->operational_check)
> > + ret = cyapa->ops->operational_check(cyapa);
> > + else
> > + ret = -EBUSY;
> > +
> > + return ret;
> > +}
> > +
> > +
> > +static irqreturn_t cyapa_irq(int irq, void *dev_id)
> > +{
> > + struct cyapa *cyapa = dev_id;
> > + struct input_dev *input = cyapa->input;
> > + bool cont;
> > +
> > + /* Interrupt event maybe cuased by host command to trackpad device. */
> > + cont = true;
> > + if (cyapa->ops->irq_cmd_handler)
> > + cont = cyapa->ops->irq_cmd_handler(cyapa);
> > +
> > + /* Interrupt event maybe from trackpad device input reporting. */
> > + if (cont && cyapa->ops->irq_handler) {
> > + if (!mutex_trylock(&cyapa->state_sync_lock)) {
> > + if (cyapa->ops->sort_empty_output_data)
> > + cyapa->ops->sort_empty_output_data(cyapa,
> > + NULL, NULL, NULL);
> > + goto out;
> > + }
> > +
> > + if (!input) {
> > + if (cyapa->ops->sort_empty_output_data)
> > + cyapa->ops->sort_empty_output_data(cyapa,
> > + NULL, NULL, NULL);
> > + } else
> > + cyapa->ops->irq_handler(cyapa);
> > +
> > + mutex_unlock(&cyapa->state_sync_lock);
> > + }
> > +out:
> > + return IRQ_HANDLED;
> > +}
> > +
> > +/*
> > + * Query device for its current operating state.
> > + */
> > +static int cyapa_get_state(struct cyapa *cyapa)
> > +{
> > + cyapa->state = CYAPA_STATE_NO_DEVICE;
> > +
> > + return -ENODEV;
> > +}
> > +
> > +/*
> > + * Poll device for its status in a loop, waiting up to timeout for a
> response.
> > + *
> > + * When the device switches state, it usually takes ~300 ms.
> > + * However, when running a new firmware image, the device must calibrate
> its
> > + * sensors, which can take as long as 2 seconds.
> > + *
> > + * Note: The timeout has granularity of the polling rate, which is 100 ms.
> > + *
> > + * Returns:
> > + * 0 when the device eventually responds with a valid non-busy state.
> > + * -ETIMEDOUT if device never responds (too many -EAGAIN)
> > + * < 0 other errors
> > + */
> > +int cyapa_poll_state(struct cyapa *cyapa, unsigned int timeout)
> > +{
> > + int ret;
> > + int tries = timeout / 100;
> > +
> > + ret = cyapa_get_state(cyapa);
> > + while ((ret || cyapa->state >= CYAPA_STATE_BL_BUSY) && tries--) {
> > + msleep(100);
> > + ret = cyapa_get_state(cyapa);
> > + }
> > +
> > + return (ret == -EAGAIN || ret == -ETIMEDOUT) ? -ETIMEDOUT : ret;
> > +}
> > +
> > +static void cyapa_detect(struct cyapa *cyapa)
> > +{
> > + struct device *dev = &cyapa->client->dev;
> > + char *envp[2] = {"ERROR=1", NULL};
> > + int ret;
> > +
> > + ret = cyapa_check_is_operational(cyapa);
> > + if (ret == -ETIMEDOUT)
> > + dev_err(dev, "no device detected, %d\n", ret);
> > + else if (ret)
> > + dev_err(dev, "device detected, but not operational, %d\n", ret);
> > +
> > + if (ret) {
> > + kobject_uevent_env(&dev->kobj, KOBJ_CHANGE, envp);
> > + return;
> > + }
> > +
> > + if (!cyapa->input) {
> > + ret = cyapa_create_input_dev(cyapa);
> > + if (ret)
> > + dev_err(dev, "create input_dev instance failed, %d\n",
> > + ret);
> > +
> > + /*
> > + * On some systems, a system crash / warm boot does not reset
> > + * the device's current power mode to FULL_ACTIVE.
> > + * If such an event happens during suspend, after the device
> > + * has been put in a low power mode, the device will still be
> > + * in low power mode on a subsequent boot, since there was
> > + * never a matching resume().
> > + * Handle this by always forcing full power here, when a
> > + * device is first detected to be in operational mode.
> > + */
> > + if (cyapa->ops->set_power_mode) {
> > + ret = cyapa->ops->set_power_mode(cyapa,
> > + PWR_MODE_FULL_ACTIVE, 0);
> > + if (ret)
> > + dev_warn(dev, "set active power failed, %d\n",
> > + ret);
> > + }
> > + }
> > +}
> > +
> > +/*
> > + * Sysfs Interface.
> > + */
> > +
> > +/*
> > + * cyapa_sleep_time_to_pwr_cmd and cyapa_pwr_cmd_to_sleep_time
> > + *
> > + * These are helper functions that convert to and from integer idle
> > + * times and register settings to write to the PowerMode register.
> > + * The trackpad supports between 20ms to 1000ms scan intervals.
> > + * The time will be increased in increments of 10ms from 20ms to 100ms.
> > + * From 100ms to 1000ms, time will be increased in increments of 20ms.
> > + *
> > + * When Idle_Time < 100, the format to convert Idle_Time to Idle_Command is:
> > + * Idle_Command = Idle Time / 10;
> > + * When Idle_Time >= 100, the format to convert Idle_Time to Idle_Command
> is:
> > + * Idle_Command = Idle Time / 20 + 5;
> > + */
> > +u8 cyapa_sleep_time_to_pwr_cmd(u16 sleep_time)
> > +{
> > + if (sleep_time < 20)
> > + sleep_time = 20; /* minimal sleep time. */
> > + else if (sleep_time > 1000)
> > + sleep_time = 1000; /* maximal sleep time. */
>
> clamp_val()
Thanks. clamp_pwr_cmd_sleep_time() will be used to above code.
>
> > +
> > + if (sleep_time < 100)
> > + return ((sleep_time / 10) << 2) & PWR_MODE_MASK;
> > + else
> > + return ((sleep_time / 20 + 5) << 2) & PWR_MODE_MASK;
> > +}
> > +
> > +u16 cyapa_pwr_cmd_to_sleep_time(u8 pwr_mode)
> > +{
> > + u8 encoded_time = pwr_mode >> 2;
> > +
> > + return (encoded_time < 10) ? encoded_time * 10
> > + : (encoded_time - 5) * 20;
> > +}
> > +
> > +void cyapa_detect_async(void *data, async_cookie_t cookie)
> > +{
> > + struct cyapa *cyapa = (struct cyapa *)data;
> > +
> > + mutex_lock(&cyapa->state_sync_lock);
> > +
> > + /* Keep synchronized with sys interface process threads. */
> > + cyapa_detect(cyapa);
> > +
> > + mutex_unlock(&cyapa->state_sync_lock);
> > +}
> > +
> > +static void cyapa_detect_and_start(void *data, async_cookie_t cookie)
> > +{
> > + cyapa_detect_async(data, cookie);
> > +}
> > +
> > +static int cyapa_tp_modules_init(struct cyapa *cyapa)
> > +{
> > + return 0;
> > +}
> > +
> > +static int cyapa_tp_modules_uninit(struct cyapa *cyapa)
> > +{
> > + return 0;
> > +}
> > +
> > static int cyapa_probe(struct i2c_client *client,
> > const struct i2c_device_id *dev_id)
> > {
> > @@ -830,6 +452,7 @@ static int cyapa_probe(struct i2c_client *client,
> > u8 adapter_func;
> > struct cyapa *cyapa;
> > struct device *dev = &client->dev;
> > + union i2c_smbus_data dummy;
> >
> > adapter_func = cyapa_check_adapter_functionality(client);
> > if (adapter_func == CYAPA_ADAPTER_FUNC_NONE) {
> > @@ -837,39 +460,43 @@ static int cyapa_probe(struct i2c_client *client,
> > return -EIO;
> > }
> >
> > + /* Make sure there is something at this address */
> > + if (dev->of_node && i2c_smbus_xfer(client->adapter, client->addr, 0,
> > + I2C_SMBUS_READ, 0, I2C_SMBUS_BYTE, &dummy) < 0)
> > + return -ENODEV;
> > +
> > cyapa = kzalloc(sizeof(struct cyapa), GFP_KERNEL);
> > if (!cyapa) {
> > dev_err(dev, "allocate memory for cyapa failed\n");
> > return -ENOMEM;
> > }
> >
> > - cyapa->gen = CYAPA_GEN3;
> > cyapa->client = client;
> > i2c_set_clientdata(client, cyapa);
> > sprintf(cyapa->phys, "i2c-%d-%04x/input0", client->adapter->nr,
> > client->addr);
> >
> > - /* i2c isn't supported, use smbus */
> > - if (adapter_func == CYAPA_ADAPTER_FUNC_SMBUS)
> > - cyapa->smbus = true;
> > - cyapa->state = CYAPA_STATE_NO_DEVICE;
> > - ret = cyapa_check_is_operational(cyapa);
> > + ret = cyapa_tp_modules_init(cyapa);
> > if (ret) {
> > - dev_err(dev, "device not operational, %d\n", ret);
> > - goto err_mem_free;
> > + dev_err(dev, "initialize device modules failed.\n");
> > + goto err_unregister_device;
> > }
> >
> > - ret = cyapa_create_input_dev(cyapa);
> > - if (ret) {
> > - dev_err(dev, "create input_dev instance failed, %d\n", ret);
> > - goto err_mem_free;
> > - }
> > + cyapa->gen = CYAPA_GEN_UNKNOWN;
> > + cyapa->ops = &cyapa_default_ops;
> > + mutex_init(&cyapa->state_sync_lock);
> >
> > - ret = cyapa_set_power_mode(cyapa, PWR_MODE_FULL_ACTIVE);
> > - if (ret) {
> > - dev_err(dev, "set active power failed, %d\n", ret);
> > - goto err_unregister_device;
> > - }
> > + /* i2c isn't supported, use smbus */
> > + if (adapter_func == CYAPA_ADAPTER_FUNC_SMBUS)
> > + cyapa->smbus = true;
> > + cyapa->state = CYAPA_STATE_NO_DEVICE;
> > + /*
> > + * Set to hard code default, they will be updated with trackpad set
> > + * default values after probe and initialized.
> > + */
> > + cyapa->suspend_power_mode = PWR_MODE_SLEEP;
> > + cyapa->suspend_sleep_time =
> > + cyapa_pwr_cmd_to_sleep_time(cyapa->suspend_power_mode);
> >
> > cyapa->irq = client->irq;
> > ret = request_threaded_irq(cyapa->irq,
> > @@ -880,14 +507,17 @@ static int cyapa_probe(struct i2c_client *client,
> > cyapa);
> > if (ret) {
> > dev_err(dev, "IRQ request failed: %d\n, ", ret);
> > - goto err_unregister_device;
> > + goto err_uninit_tp_modules;
> > }
> >
> > + async_schedule(cyapa_detect_and_start, cyapa);
> > return 0;
> >
> > +err_uninit_tp_modules:
> > + cyapa_tp_modules_uninit(cyapa);
> > err_unregister_device:
> > input_unregister_device(cyapa->input);
> > -err_mem_free:
> > + i2c_set_clientdata(client, NULL);
> > kfree(cyapa);
> >
> > return ret;
> > @@ -898,8 +528,12 @@ static int cyapa_remove(struct i2c_client *client)
> > struct cyapa *cyapa = i2c_get_clientdata(client);
> >
>
> You schedule detect asynchronously so you need to make sure it is not running
> (and will not be running) when you try to unbind the driver here.
Thanks. I will update it in next release.
To avoid this problem, a removed flag is introduced and is also synced by the
cyapa->state_sync_lock mutex lock. So when this driver is removing, it's sure
that the detecting asynchronous thread is not running, and if there's any event
cause the asynchronous thread is entered, it will do nothing and will exit immediately.
>
> Thanks.
>
>
> --
> Dmitry
^ permalink raw reply
* Re: [GIT PULL] HID
From: Markus Trippelsdorf @ 2014-08-22 8:18 UTC (permalink / raw)
To: Jiri Kosina; +Cc: linux-input, linux-kernel, Ben Hawkes, Benjamin Tissoires
In-Reply-To: <alpine.LNX.2.00.1408220257050.23162@pobox.suse.cz>
On 2014.08.22 at 03:00 -0500, Jiri Kosina wrote:
> On Fri, 22 Aug 2014, Markus Trippelsdorf wrote:
>
> > > HID: logitech: perform bounds checking on device_id early enough
> >
> > The commit above (ad3e14d7c5268c2e) causes the bounds checking to always
> > fail on my monolithic kernel (without modules):
> >
> > ...
> > [ 2.922617] usb 4-2: new full-speed USB device number 2 using ohci-pci
> > [ 2.996587] udevd[98]: starting eudev version 1.0
> > [ 3.071203] random: nonblocking pool is initialized
> > [ 3.108360] logitech-djreceiver 0003:046D:C52B.0003: hiddev0,hidraw0: USB HID v1.11 Device [Logitech USB Receiver] on usb-0000:00:12.1-2/input2
> > [ 3.163208] input: Logitech Unifying Device. Wireless PID:4026 as /devices/pci0000:00/0000:00:12.1/usb4/4-2/4-2:1.2/0003:046D:C52B.0003/0003:046D:C52B.0004/input/input2
> > [ 3.163511] logitech-djdevice 0003:046D:C52B.0004: input,hidraw1: USB HID v1.11 Keyboard [Logitech Unifying Device. Wireless PID:4026] on usb-0000:00:12.1-2:1
> > [ 3.164121] logitech-djreceiver 0003:046D:C52B.0003: logi_dj_raw_event: invalid device index:0
> > [ 3.289261] usb 3-1: new low-speed USB device number 2 using ohci-pci
> > [ 3.457606] input: HID 046a:0011 as /devices/pci0000:00/0000:00:12.0/usb3/3-1/3-1:1.0/0003:046A:0011.0005/input/input3
> > [ 3.457794] hid-generic 0003:046A:0011.0005: input,hidraw2: USB HID v1.10 Keyboard [HID 046a:0011] on usb-0000:00:12.0-1/input0
> > [ 3.712886] Switched to clocksource tsc
>
> Thanks a lot for a timely report.
>
> I am travelling till next week and don't unfortunately have the hardware
> here with me to test momentarily, so please take this with a lot of grains
> of salt (maybe Benjamin would be able to look into this before I'd be able
> to on monday ... ?).
>
> Does the shot-in-the-dark patch below put things back in shape for you?
Thanks a lot for the timely patch. It indeed fixes the issue for me.
--
Markus
^ permalink raw reply
* Re: [GIT PULL] HID
From: Jiri Kosina @ 2014-08-22 8:00 UTC (permalink / raw)
To: Markus Trippelsdorf
Cc: linux-input, linux-kernel, Ben Hawkes, Benjamin Tissoires
In-Reply-To: <20140822074030.GA297@x4>
On Fri, 22 Aug 2014, Markus Trippelsdorf wrote:
> > HID: logitech: perform bounds checking on device_id early enough
>
> The commit above (ad3e14d7c5268c2e) causes the bounds checking to always
> fail on my monolithic kernel (without modules):
>
> ...
> [ 2.922617] usb 4-2: new full-speed USB device number 2 using ohci-pci
> [ 2.996587] udevd[98]: starting eudev version 1.0
> [ 3.071203] random: nonblocking pool is initialized
> [ 3.108360] logitech-djreceiver 0003:046D:C52B.0003: hiddev0,hidraw0: USB HID v1.11 Device [Logitech USB Receiver] on usb-0000:00:12.1-2/input2
> [ 3.163208] input: Logitech Unifying Device. Wireless PID:4026 as /devices/pci0000:00/0000:00:12.1/usb4/4-2/4-2:1.2/0003:046D:C52B.0003/0003:046D:C52B.0004/input/input2
> [ 3.163511] logitech-djdevice 0003:046D:C52B.0004: input,hidraw1: USB HID v1.11 Keyboard [Logitech Unifying Device. Wireless PID:4026] on usb-0000:00:12.1-2:1
> [ 3.164121] logitech-djreceiver 0003:046D:C52B.0003: logi_dj_raw_event: invalid device index:0
> [ 3.289261] usb 3-1: new low-speed USB device number 2 using ohci-pci
> [ 3.457606] input: HID 046a:0011 as /devices/pci0000:00/0000:00:12.0/usb3/3-1/3-1:1.0/0003:046A:0011.0005/input/input3
> [ 3.457794] hid-generic 0003:046A:0011.0005: input,hidraw2: USB HID v1.10 Keyboard [HID 046a:0011] on usb-0000:00:12.0-1/input0
> [ 3.712886] Switched to clocksource tsc
Thanks a lot for a timely report.
I am travelling till next week and don't unfortunately have the hardware
here with me to test momentarily, so please take this with a lot of grains
of salt (maybe Benjamin would be able to look into this before I'd be able
to on monday ... ?).
Does the shot-in-the-dakr patch below put things back in shape for you?
diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c
index b7ba829..d9ae77f 100644
--- a/drivers/hid/hid-logitech-dj.c
+++ b/drivers/hid/hid-logitech-dj.c
@@ -861,7 +861,7 @@ static void logi_dj_remove(struct hid_device *hdev)
* have finished and no more raw_event callbacks should arrive after
* the remove callback was triggered so no locks are put around the
* code below */
- for (i = 0; i < (DJ_MAX_PAIRED_DEVICES + DJ_DEVICE_INDEX_MIN); i++) {
+ for (i = 0; i < (DJ_MAX_PAIRED_DEVICES + DJ_DEVICE_INDEX_MIN + 1); i++) {
dj_dev = djrcv_dev->paired_dj_devices[i];
if (dj_dev != NULL) {
hid_destroy_device(dj_dev->hdev);
diff --git a/drivers/hid/hid-logitech-dj.h b/drivers/hid/hid-logitech-dj.h
index 4a40003..1f630e4 100644
--- a/drivers/hid/hid-logitech-dj.h
+++ b/drivers/hid/hid-logitech-dj.h
@@ -27,8 +27,8 @@
#define DJ_MAX_PAIRED_DEVICES 6
#define DJ_MAX_NUMBER_NOTIFICATIONS 8
-#define DJ_DEVICE_INDEX_MIN 1
-#define DJ_DEVICE_INDEX_MAX 6
+#define DJ_DEVICE_INDEX_MIN 0
+#define DJ_DEVICE_INDEX_MAX 5
#define DJREPORT_SHORT_LENGTH 15
#define DJREPORT_LONG_LENGTH 32
@@ -97,7 +97,7 @@ struct dj_report {
struct dj_receiver_dev {
struct hid_device *hdev;
struct dj_device *paired_dj_devices[DJ_MAX_PAIRED_DEVICES +
- DJ_DEVICE_INDEX_MIN];
+ DJ_DEVICE_INDEX_MIN + 1];
struct work_struct work;
struct kfifo notif_fifo;
spinlock_t lock;
--
Jiri Kosina
SUSE Labs
^ permalink raw reply related
* Re: [PATCH 1/1] HID: usbhid: add usb_clear_halt determination for next hid_start_in
From: Jiri Kosina @ 2014-08-22 7:45 UTC (permalink / raw)
To: CheChun Kuo; +Cc: linux-usb, linux-input
In-Reply-To: <1408692913-12336-1-git-send-email-vichy.kuo@gmail.com>
On Fri, 22 Aug 2014, CheChun Kuo wrote:
> HID IR device will not response to any command send from host when
> it is finishing paring and tring to reset itself. During this period of
> time, usb_cleaer_halt will be fail and if hid_start_in soon again, we
> has the possibility trap in infinite loop.
>
> Signed-off-by: CheChun Kuo <vichy.kuo@gmail.com>
> ---
> drivers/hid/usbhid/hid-core.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
> index 79cf503..256b102 100644
> --- a/drivers/hid/usbhid/hid-core.c
> +++ b/drivers/hid/usbhid/hid-core.c
> @@ -122,7 +122,8 @@ static void hid_reset(struct work_struct *work)
> dev_dbg(&usbhid->intf->dev, "clear halt\n");
> rc = usb_clear_halt(hid_to_usb_dev(hid), usbhid->urbin->pipe);
> clear_bit(HID_CLEAR_HALT, &usbhid->iofl);
> - hid_start_in(hid);
> + if (rc == 0)
> + hid_start_in(hid);
> }
I'd need a more detailed explanation for this patch, as I don't understand
neither the text in the changelog nor the patch itself. Namely:
- which IR devices are showing this behavior?
- where does the infinite loop come from? hid_reset() should error out
properly if usb_clear_halt() fails and HID_IN_RUNNING flag is not set
Thanks,
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [GIT PULL] HID
From: Markus Trippelsdorf @ 2014-08-22 7:40 UTC (permalink / raw)
To: Jiri Kosina; +Cc: linux-input, linux-kernel, Ben Hawkes, Benjamin Tissoires
In-Reply-To: <alpine.LNX.2.00.1408211146110.23162@pobox.suse.cz>
On 2014.08.21 at 12:43 -0500, Jiri Kosina wrote:
> HID: logitech: perform bounds checking on device_id early enough
The commit above (ad3e14d7c5268c2e) causes the bounds checking to always
fail on my monolithic kernel (without modules):
...
[ 2.922617] usb 4-2: new full-speed USB device number 2 using ohci-pci
[ 2.996587] udevd[98]: starting eudev version 1.0
[ 3.071203] random: nonblocking pool is initialized
[ 3.108360] logitech-djreceiver 0003:046D:C52B.0003: hiddev0,hidraw0: USB HID v1.11 Device [Logitech USB Receiver] on usb-0000:00:12.1-2/input2
[ 3.163208] input: Logitech Unifying Device. Wireless PID:4026 as /devices/pci0000:00/0000:00:12.1/usb4/4-2/4-2:1.2/0003:046D:C52B.0003/0003:046D:C52B.0004/input/input2
[ 3.163511] logitech-djdevice 0003:046D:C52B.0004: input,hidraw1: USB HID v1.11 Keyboard [Logitech Unifying Device. Wireless PID:4026] on usb-0000:00:12.1-2:1
[ 3.164121] logitech-djreceiver 0003:046D:C52B.0003: logi_dj_raw_event: invalid device index:0
[ 3.289261] usb 3-1: new low-speed USB device number 2 using ohci-pci
[ 3.457606] input: HID 046a:0011 as /devices/pci0000:00/0000:00:12.0/usb3/3-1/3-1:1.0/0003:046A:0011.0005/input/input3
[ 3.457794] hid-generic 0003:046A:0011.0005: input,hidraw2: USB HID v1.10 Keyboard [HID 046a:0011] on usb-0000:00:12.0-1/input0
[ 3.712886] Switched to clocksource tsc
...
--
Markus
^ permalink raw reply
* [PATCH 1/1] HID: usbhid: add usb_clear_halt determination for next hid_start_in
From: CheChun Kuo @ 2014-08-22 7:35 UTC (permalink / raw)
To: linux-usb, linux-input; +Cc: jkosina, CheChun Kuo
HID IR device will not response to any command send from host when it is finishing paring and tring to reset itself.
During this period of time, usb_cleaer_halt will be fail and if hid_start_in soon again, we has the possibility trap in infinite loop.
Signed-off-by: CheChun Kuo <vichy.kuo@gmail.com>
---
drivers/hid/usbhid/hid-core.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
index 79cf503..256b102 100644
--- a/drivers/hid/usbhid/hid-core.c
+++ b/drivers/hid/usbhid/hid-core.c
@@ -122,7 +122,8 @@ static void hid_reset(struct work_struct *work)
dev_dbg(&usbhid->intf->dev, "clear halt\n");
rc = usb_clear_halt(hid_to_usb_dev(hid), usbhid->urbin->pipe);
clear_bit(HID_CLEAR_HALT, &usbhid->iofl);
- hid_start_in(hid);
+ if (rc == 0)
+ hid_start_in(hid);
}
else if (test_bit(HID_RESET_PENDING, &usbhid->iofl)) {
--
1.7.9.5
^ permalink raw reply related
* Re: [PATCH 1/2] doc: dt/bindings: input: introduce TI DRV2667 haptic driver description
From: Sergei Shtylyov @ 2014-08-21 19:32 UTC (permalink / raw)
To: Murphy, Dan, linux-input@vger.kernel.org,
dmitry.torokhov@gmail.com
Cc: devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <00FC9A978A94B7418C33AFAE8A35ED49DF0ABE@DFLE09.ent.ti.com>
On 08/21/2014 11:14 PM, Murphy, Dan wrote:
[...]
>>> DRV2667 is a haptic/vibrator driver for Linear Resonant Actuators.
>>> Adding dt binding for this part
>>> Signed-off-by: Dan Murphy <dmurphy@ti.com>
>>> ---
>>> .../devicetree/bindings/input/ti,drv2667.txt | 18 ++++++++++++++++++
>>> 1 file changed, 18 insertions(+)
>>> create mode 100644 Documentation/devicetree/bindings/input/ti,drv2667.txt
>>> diff --git a/Documentation/devicetree/bindings/input/ti,drv2667.txt b/Documentation/devicetree/bindings/input/ti,drv2667.txt
>>> new file mode 100644
>>> index 0000000..525216d
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/input/ti,drv2667.txt
>>> @@ -0,0 +1,18 @@
>>> +Texas Instruments - drv2667 Haptics driver
>>> +
>>> +The drv2667 uses serial control bus communicates through I2C protocols
>>> +
>>> +Required properties:
>>> + - compatible - "ti,drv2667" - DRV2667
>>> + - reg - I2C slave address
>>> + - vbat-supply - Required supply regulator
>>> +
>>> +Example:
>>> +
>>> +drv2667: drv2667@59 {
>> The ePAPR standard [1] has something to say on this matter:
> What is this reference? Did not know this existed so wanted to read up on it.
Sorry, forgot to paste the link. :-<
[1] http://www.power.org/resources/downloads/Power_ePAPR_APPROVED_v1.0.pdf
WBR, Sergei
^ permalink raw reply
* Re: [PATCH 1/2] doc: dt/bindings: input: introduce TI DRV2667 haptic driver description
From: Murphy, Dan @ 2014-08-21 19:14 UTC (permalink / raw)
To: Sergei Shtylyov, linux-input@vger.kernel.org,
dmitry.torokhov@gmail.com
Cc: devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <53F642AF.3070200@cogentembedded.com>
Sergei
Thanks for the additional comments
On 08/21/2014 02:04 PM, Sergei Shtylyov wrote:
> Hello.
>
> On 08/21/2014 10:19 PM, Dan Murphy wrote:
>
>> DRV2667 is a haptic/vibrator driver for Linear Resonant Actuators.
>> Adding dt binding for this part
>
>> Signed-off-by: Dan Murphy <dmurphy@ti.com>
>> ---
>> .../devicetree/bindings/input/ti,drv2667.txt | 18 ++++++++++++++++++
>> 1 file changed, 18 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/input/ti,drv2667.txt
>
>> diff --git a/Documentation/devicetree/bindings/input/ti,drv2667.txt b/Documentation/devicetree/bindings/input/ti,drv2667.txt
>> new file mode 100644
>> index 0000000..525216d
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/input/ti,drv2667.txt
>> @@ -0,0 +1,18 @@
>> +Texas Instruments - drv2667 Haptics driver
>> +
>> +The drv2667 uses serial control bus communicates through I2C protocols
>> +
>> +Required properties:
>> + - compatible - "ti,drv2667" - DRV2667
>> + - reg - I2C slave address
>> + - vbat-supply - Required supply regulator
>> +
>> +Example:
>> +
>> +drv2667: drv2667@59 {
>
> The ePAPR standard [1] has something to say on this matter:
What is this reference? Did not know this existed so wanted to read up on it.
I can change it to haptics or vibrator
>
> "The name of a node should be somewhat generic, reflecting the function of the
> device and not its precise programming model."
>
>> + compatible = "ti,drv2667";
>
> Indented too much to the right.
One tab to many. Got it
>
>> + reg = <0x59>;
>
> Still no "vbat-supply"... :-/
>
Still have not sent in v2 of the patch yet. Waiting on other comments.
>> +};
> [...]
>
> WBR, Sergei
>
>
--
------------------
Dan Murphy
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox