* [PATCH v3 3/3] input: appletouch: fix jumps when additional fingers are detected
From: Clinton Sprain @ 2014-03-09 6:19 UTC (permalink / raw)
To: linux-input; +Cc: Dmitry Torokhov, Henrik Rydberg
In-Reply-To: <531C042D.5090103@gmail.com>
Addresses issues related to when a second finger enters or leaves the field,
causing the cursor to jump or the page to scroll unexpectedly; now, we
discard any movement change that happens at the exact moment we detect a
change in the number of fingers touching the trackpad. This doesn't
completely resolve the issue but does greatly mitigate it.
Signed-off-by: Clinton Sprain <clintonsprain@gmail.com>
---
drivers/input/mouse/appletouch.c | 27 +++++++++++++++++++++------
1 file changed, 21 insertions(+), 6 deletions(-)
diff --git a/drivers/input/mouse/appletouch.c b/drivers/input/mouse/appletouch.c
index 0252bab..0c7bc6e 100644
--- a/drivers/input/mouse/appletouch.c
+++ b/drivers/input/mouse/appletouch.c
@@ -206,6 +206,7 @@ struct atp {
bool valid; /* are the samples valid? */
bool size_detect_done;
bool overflow_warned;
+ int fingers_old; /* last reported finger count */
int x_old; /* last reported x/y, */
int y_old; /* used for smoothing */
signed char xy_cur[ATP_XSENSORS + ATP_YSENSORS];
@@ -508,7 +509,7 @@ static void atp_complete_geyser_1_2(struct urb *urb)
{
int x, y, x_z, y_z, x_f, y_f;
int retval, i, j;
- int key;
+ int key, fingers;
struct atp *dev = urb->context;
int status = atp_status_check(urb);
@@ -591,7 +592,9 @@ static void atp_complete_geyser_1_2(struct urb *urb)
dev->info->yfact, &y_z, &y_f);
key = dev->data[dev->info->datalen - 1] & ATP_STATUS_BUTTON;
- if (x && y) {
+ fingers = max(x_f, y_f);
+
+ if (x && y && (fingers == dev->fingers_old)) {
if (dev->x_old != -1) {
x = (dev->x_old * 7 + x) >> 3;
y = (dev->y_old * 7 + y) >> 3;
@@ -608,7 +611,7 @@ static void atp_complete_geyser_1_2(struct urb *urb)
input_report_abs(dev->input, ABS_Y, y);
input_report_abs(dev->input, ABS_PRESSURE,
min(ATP_PRESSURE, x_z + y_z));
- atp_report_fingers(dev->input, max(x_f, y_f));
+ atp_report_fingers(dev->input, fingers);
}
dev->x_old = x;
dev->y_old = y;
@@ -616,6 +619,7 @@ static void atp_complete_geyser_1_2(struct urb *urb)
} else if (!x && !y) {
dev->x_old = dev->y_old = -1;
+ dev->fingers_old = 0;
input_report_key(dev->input, BTN_TOUCH, 0);
input_report_abs(dev->input, ABS_PRESSURE, 0);
atp_report_fingers(dev->input, 0);
@@ -624,6 +628,10 @@ static void atp_complete_geyser_1_2(struct urb *urb)
memset(dev->xy_acc, 0, sizeof(dev->xy_acc));
}
+ if (fingers != dev->fingers_old)
+ dev->x_old = dev->y_old = -1;
+ dev->fingers_old = fingers;
+
input_report_key(dev->input, BTN_LEFT, key);
input_sync(dev->input);
@@ -641,7 +649,7 @@ static void atp_complete_geyser_3_4(struct urb *urb)
{
int x, y, x_z, y_z, x_f, y_f;
int retval, i, j;
- int key;
+ int key, fingers;
struct atp *dev = urb->context;
int status = atp_status_check(urb);
@@ -703,7 +711,9 @@ static void atp_complete_geyser_3_4(struct urb *urb)
dev->info->yfact, &y_z, &y_f);
key = dev->data[dev->info->datalen - 1] & ATP_STATUS_BUTTON;
- if (x && y) {
+ fingers = max(x_f, y_f);
+
+ if (x && y && (fingers == dev->fingers_old)) {
if (dev->x_old != -1) {
x = (dev->x_old * 7 + x) >> 3;
y = (dev->y_old * 7 + y) >> 3;
@@ -720,7 +730,7 @@ static void atp_complete_geyser_3_4(struct urb *urb)
input_report_abs(dev->input, ABS_Y, y);
input_report_abs(dev->input, ABS_PRESSURE,
min(ATP_PRESSURE, x_z + y_z));
- atp_report_fingers(dev->input, max(x_f, y_f));
+ atp_report_fingers(dev->input, fingers);
}
dev->x_old = x;
dev->y_old = y;
@@ -728,6 +738,7 @@ static void atp_complete_geyser_3_4(struct urb *urb)
} else if (!x && !y) {
dev->x_old = dev->y_old = -1;
+ dev->fingers_old = 0;
input_report_key(dev->input, BTN_TOUCH, 0);
input_report_abs(dev->input, ABS_PRESSURE, 0);
atp_report_fingers(dev->input, 0);
@@ -736,6 +747,10 @@ static void atp_complete_geyser_3_4(struct urb *urb)
memset(dev->xy_acc, 0, sizeof(dev->xy_acc));
}
+ if (fingers != dev->fingers_old)
+ dev->x_old = dev->y_old = -1;
+ dev->fingers_old = fingers;
+
input_report_key(dev->input, BTN_LEFT, key);
input_sync(dev->input);
--
1.7.9.5
^ permalink raw reply related
* Re: [linux-sunxi] Re: [PATCH 6/7] regulator: AXP20x: Add support for regulators subsystem
From: Mark Brown @ 2014-03-09 7:51 UTC (permalink / raw)
To: Carlo Caione
Cc: Maxime Ripard, linux-arm-kernel, hdegoede, emilio, wens, sameo,
lee.jones, devicetree, dmitry.torokhov, linux-input, linux-doc,
lgirdwood, linux-sunxi
In-Reply-To: <CAOQ7t2YSidSPVSBrBzj3R1SYS_Xb10a2vfEs+u1DEkrhEJQCaQ@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 586 bytes --]
On Sat, Mar 08, 2014 at 12:43:04PM +0100, Carlo Caione wrote:
> On Fri, Mar 7, 2014 at 7:22 PM, Maxime Ripard
> > On Sat, Mar 01, 2014 at 05:45:51PM +0100, Carlo Caione wrote:
> >> + return platform_driver_register(&axp20x_regulator_driver);
> > I thought the AXP was only connected through I2C? How is that a
> > platform device then?
> Not really. It is plain wrong.
> I'll fix it in v2.
Are you sure this is wrong? For MFDs the core MFD is a driver of
whatever bus type is in use but the function drivers are platform
devices which talk to the hardware via the core device.
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: Re: [PATCH 6/7] regulator: AXP20x: Add support for regulators subsystem
From: Carlo Caione @ 2014-03-09 8:56 UTC (permalink / raw)
To: Mark Brown
Cc: Maxime Ripard, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
hdegoede-H+wXaHxf7aLQT0dZR+AlfA, emilio-0Z03zUJReD5OxF6Tv1QG9Q,
wens-jdAy2FN1RRM, sameo-VuQAYsv1563Yd54FQh9/CA,
lee.jones-QSEj5FYQhm4dnm+yROfE0A,
devicetree-u79uwXL29TY76Z2rM5mHXA,
dmitry.torokhov-Re5JQEeQqe8AvxtiuMwx3w,
linux-input-u79uwXL29TY76Z2rM5mHXA,
linux-doc-u79uwXL29TY76Z2rM5mHXA,
lgirdwood-Re5JQEeQqe8AvxtiuMwx3w,
linux-sunxi-/JYPxA39Uh5TLH3MbocFFw
In-Reply-To: <20140309075148.GJ28112-GFdadSzt00ze9xe1eoZjHA@public.gmane.org>
On Sun, Mar 09, 2014 at 07:51:48AM +0000, Mark Brown wrote:
> On Sat, Mar 08, 2014 at 12:43:04PM +0100, Carlo Caione wrote:
> > On Fri, Mar 7, 2014 at 7:22 PM, Maxime Ripard
> > > On Sat, Mar 01, 2014 at 05:45:51PM +0100, Carlo Caione wrote:
>
> > >> + return platform_driver_register(&axp20x_regulator_driver);
>
> > > I thought the AXP was only connected through I2C? How is that a
> > > platform device then?
>
> > Not really. It is plain wrong.
> > I'll fix it in v2.
>
> Are you sure this is wrong? For MFDs the core MFD is a driver of
> whatever bus type is in use but the function drivers are platform
> devices which talk to the hardware via the core device.
ok, I spoke too soon. Sorry for the noise.
--
Carlo Caione
^ permalink raw reply
* Re: [PATCH 1/7] mfd: AXP20x: Add mfd driver for AXP20x PMIC
From: Maxime Ripard @ 2014-03-09 9:11 UTC (permalink / raw)
To: Carlo Caione
Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-sunxi-/JYPxA39Uh5TLH3MbocFFw,
hdegoede-H+wXaHxf7aLQT0dZR+AlfA, emilio-0Z03zUJReD5OxF6Tv1QG9Q,
wens-jdAy2FN1RRM, sameo-VuQAYsv1563Yd54FQh9/CA,
lee.jones-QSEj5FYQhm4dnm+yROfE0A,
devicetree-u79uwXL29TY76Z2rM5mHXA,
dmitry.torokhov-Re5JQEeQqe8AvxtiuMwx3w,
linux-input-u79uwXL29TY76Z2rM5mHXA,
linux-doc-u79uwXL29TY76Z2rM5mHXA,
lgirdwood-Re5JQEeQqe8AvxtiuMwx3w, broonie-DgEjT+Ai2ygdnm+yROfE0A
In-Reply-To: <20140308113141.GA14009-bi+AKbBUZKZeIdyRz4JgOMwOAu8XWILU@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 789 bytes --]
Hi Carlo,
On Sat, Mar 08, 2014 at 12:31:41PM +0100, Carlo Caione wrote:
> > > + axp20x->pm_off = of_property_read_bool(node, "axp,system-power-controller");
> > > +
> > > + if (axp20x->pm_off && !pm_power_off) {
> > > + axp20x_pm_power_off = axp20x;
> > > + pm_power_off = axp20x_power_off;
> > > + }
> >
> > I don't think we have any other way to actually power down the board.
> >
> > Is there any reason why we would not need this property?
>
> In case you have multiple PMIC on the board in this way you con define
> which is the one apt to power off.
Do we even have such a board?
Let's leave this out, we can always add it later if needs be.
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH v3 2/3] input: appletouch: implement sensor data smoothing
From: Henrik Rydberg @ 2014-03-09 13:54 UTC (permalink / raw)
To: Clinton Sprain, linux-input; +Cc: Dmitry Torokhov
In-Reply-To: <531C0797.9060908@gmail.com>
Hi Clinton,
> Use smoothed version of sensor array data to calculate movement and add weight
> to prior values when calculating average. This gives more granular and more
> predictable movement.
Great patch, just one thing:
> + /* Handle other edge. */
> + smooth_tmp[nb_sensors + 7] = (smooth[nb_sensors + 7] + smooth[nb_sensors + 6]) >> 1;
> +
> + for (i = 0; i < nb_sensors + 7; i++)
> + smooth[i] = smooth_tmp[i];
Should be "< nb_sensors + 8" here, no?
Thanks,
Henrik
^ permalink raw reply
* Re: [PATCH v3 2/3] input: appletouch: implement sensor data smoothing
From: Clinton Sprain @ 2014-03-09 15:03 UTC (permalink / raw)
To: Henrik Rydberg, linux-input; +Cc: Dmitry Torokhov
In-Reply-To: <531C72AC.5030909@euromail.se>
Ah, you are correct. Good catch. The last few far edge values are guaranteed to be 0 on all but the older 17" devices so I didn't see anything askew.
Thanks,
Clinton
On 03/09/2014 08:54 AM, Henrik Rydberg wrote:
> Hi Clinton,
>
>> Use smoothed version of sensor array data to calculate movement and add weight
>> to prior values when calculating average. This gives more granular and more
>> predictable movement.
>
> Great patch, just one thing:
>
>> + /* Handle other edge. */
>> + smooth_tmp[nb_sensors + 7] = (smooth[nb_sensors + 7] + smooth[nb_sensors + 6]) >> 1;
>> +
>> + for (i = 0; i < nb_sensors + 7; i++)
>> + smooth[i] = smooth_tmp[i];
>
> Should be "< nb_sensors + 8" here, no?
>
> Thanks,
> Henrik
>
^ permalink raw reply
* Re: drivers/input/input.c and a broken USB HID Contour ShuttlePRO v2
From: Greg KH @ 2014-03-09 17:19 UTC (permalink / raw)
To: Harald Albrecht; +Cc: linux-usb, linux-input
In-Reply-To: <531C8EF4.9090609@gmx.net>
On Sun, Mar 09, 2014 at 04:55:32PM +0100, Harald Albrecht wrote:
> I would like to improve support for the multimedia controllers from
> Contour company, as there currently get relative events swalled by input.c.
>
> The background: Contour seems to have messed up the USB HID descriptions
> of their multimedia controller devices in that these report to use
> relative axis when in fact they are creating EV_REL events with absolute
> coordinate values. The problem here is, that mapping these events to
> EV_ABS will break all existing software that supports these devices.
>
> Unfortunately, drivers/input/input.c correctly swallows any relative
> events with a relative movement value of zero...
>
> case EV_REL:
> if (is_event_supported(code, dev->relbit, REL_MAX) && value)
> disposition = INPUT_PASS_TO_HANDLERS;
> break;
>
> Simply removing the "&& value" would most probably have adverse effect
> on many software. Unfortunately, there is yet no "quirks" handling
> present in input.c.
>
> How can I add such quirks handling in a way that would be acceptable by
> the kernel maintainers?
The people on linux-input@vger.kernel.org (now cc:ed) might know more...
^ permalink raw reply
* Re: drivers/input/input.c and a broken USB HID Contour ShuttlePRO v2
From: Harald Albrecht @ 2014-03-09 17:31 UTC (permalink / raw)
Cc: linux-usb, linux-input
In-Reply-To: <20140309171907.GC30196@kroah.com>
It seems that I will need help from both mailing lists. I've researched
more and would like to propose this idea ... and I need feedback from
the input and usb kernel maintainers in order to get my idea right.
So for the input part I would like to propose to introduce a new input
property INPUT_PROP_REL_IS_ABS with value 0x04 (this would be the next
available "propbit" in input land (include/linux/input.h).
#define INPUT_PROP_REL_IS_ABS 0x04 /* relative events report absolute
coordinates */
When handling EV_REL events in drivers/input/input.c we then could check
for the propbit being set and then always pass events from such devices
to the handlers. This would allow us to selectively enable this fix for
broken devices only.
static int input_get_disposition(struct input_dev *dev, unsigned int
type, unsigned int code, int value)
{
...
if (is_event_supported(code, dev->relbit, REL_MAX) &&
(value || test_bit(INPUT_PROP_REL_IS_ABS, dev->propbit))
disposition = INPUT_PASS_TO_HANDLERS;
As for the USB part I'm currently lost. My idea is to have a contour USB
device module that gets registered as a HID (sub?) driver so the generic
HID can do all the normal handling. The part I don't know yet is how to
switch on the propbit I mentioned in the previous paragraph. Can this be
done in the input_mapped function of such an HID subdriver? Do I need to
chain my own input_mapped to the generic one? I have to admit that I'm
lost at this time, as this is my first journey into Linux driver land
and Linux USB HID territory... So in my simple mind I would think of
something like...
static int contour_input_mapped(struct hid_device *hdev, struct
hid_input *hi,
struct hid_field *field, struct hid_usage *usage, unsigned long
**bit,
int *max)
{
__set_bit(INPUT_PROP_REL_IS_ABS, hi->input->propbit);
return 0;
}
static const struct hid_device_id contour_devices[] = {
{ HID_USB_DEVICE(0x0b33, 0x0030) }
{ }
};
static struct hid_driver contour_driver = {
.name = "contour",
.id_table = contour_devices,
.input_mapped = contour_input_mapped,
};
module_hid_driver(contour_driver);
MODULE_DESCRIPTION("Driver for Contour ShuttlePRO v2");
MODULE_LICENSE("GPL");
So, please bear with me and help is greatly appreciated!
-- TheDiveO
On 09.03.2014 18:19, Greg KH wrote:
> On Sun, Mar 09, 2014 at 04:55:32PM +0100, Harald Albrecht wrote:
>> I would like to improve support for the multimedia controllers from
>> Contour company, as there currently get relative events swalled by input.c.
>>
>> The background: Contour seems to have messed up the USB HID descriptions
>> of their multimedia controller devices in that these report to use
>> relative axis when in fact they are creating EV_REL events with absolute
>> coordinate values. The problem here is, that mapping these events to
>> EV_ABS will break all existing software that supports these devices.
>>
>> Unfortunately, drivers/input/input.c correctly swallows any relative
>> events with a relative movement value of zero...
>>
>> case EV_REL:
>> if (is_event_supported(code, dev->relbit, REL_MAX) && value)
>> disposition = INPUT_PASS_TO_HANDLERS;
>> break;
>>
>> Simply removing the "&& value" would most probably have adverse effect
>> on many software. Unfortunately, there is yet no "quirks" handling
>> present in input.c.
>>
>> How can I add such quirks handling in a way that would be acceptable by
>> the kernel maintainers?
> The people on linux-input@vger.kernel.org (now cc:ed) might know more...
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [Patch v2] Surface Pro 2 USB composite device handling
From: Derya @ 2014-03-09 17:51 UTC (permalink / raw)
To: linux-input, Benjamin Tissoires, Jiri Kosina
This patch set is a reworked version of the one, that I submitted on Friday.
Followed the Benjamin's suggestions (many thanks)
- The first patch reverses commit 117309c51dca42121f70cacec801511b76acf75c
- The second readds the product ids and ensures the load of hid-core
instead of hid-multiouch
^ permalink raw reply
* [PATCH 1/2] Revert "HID: microsoft: Add ID's for Surface Type/Touch, Cover 2"
From: Derya @ 2014-03-09 17:53 UTC (permalink / raw)
To: linux-input, Benjamin Tissoires, Jiri Kosina
In-Reply-To: <531CAA1F.4080809@yahoo.de>
This reverts commit 117309c51dca42121f70cacec801511b76acf75c.
The MS Surface Pro 2 has an USB composite device with 3 interfaces
- interface 0 - sensor hub
- interface 1 - wacom digitizer
- interface 2 - the keyboard cover, if one is attached
This USB composite device changes it product id dependent on if and which
keyboard cover is attached. Adding the covers to hid_have_special_driver
prevents loading the right hid drivers for the other two interfaces, all 3
get loaded with hid-microsoft. We don't even need hid-microsoft for the
keyboards. We have to revert this to load the right hid modules for each
interface.
Signed-off-by: Derya <derya.kiran@yahoo.de>
---
drivers/hid/hid-core.c | 2 --
drivers/hid/hid-ids.h | 2 --
drivers/hid/hid-microsoft.c | 4 ----
3 files changed, 8 deletions(-)
diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index cc32a6f..bb5c494 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -1780,8 +1780,6 @@ static const struct hid_device_id
hid_have_special_driver[] = {
{ HID_USB_DEVICE(USB_VENDOR_ID_MICROSOFT,
USB_DEVICE_ID_MS_PRESENTER_8K_USB) },
{ HID_USB_DEVICE(USB_VENDOR_ID_MICROSOFT,
USB_DEVICE_ID_MS_DIGITAL_MEDIA_3K) },
{ HID_USB_DEVICE(USB_VENDOR_ID_MICROSOFT,
USB_DEVICE_ID_WIRELESS_OPTICAL_DESKTOP_3_0) },
- { HID_USB_DEVICE(USB_VENDOR_ID_MICROSOFT,
USB_DEVICE_ID_MS_TYPE_COVER_2) },
- { HID_USB_DEVICE(USB_VENDOR_ID_MICROSOFT,
USB_DEVICE_ID_MS_TOUCH_COVER_2) },
{ HID_USB_DEVICE(USB_VENDOR_ID_MONTEREY,
USB_DEVICE_ID_GENIUS_KB29E) },
{ HID_USB_DEVICE(USB_VENDOR_ID_NTRIG,
USB_DEVICE_ID_NTRIG_TOUCH_SCREEN) },
{ HID_USB_DEVICE(USB_VENDOR_ID_NTRIG,
USB_DEVICE_ID_NTRIG_TOUCH_SCREEN_1) },
diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index 22f28d6..07cd28c 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -624,8 +624,6 @@
#define USB_DEVICE_ID_MS_PRESENTER_8K_USB 0x0713
#define USB_DEVICE_ID_MS_DIGITAL_MEDIA_3K 0x0730
#define USB_DEVICE_ID_MS_COMFORT_MOUSE_4500 0x076c
-#define USB_DEVICE_ID_MS_TOUCH_COVER_2 0x07a7
-#define USB_DEVICE_ID_MS_TYPE_COVER_2 0x07a9
#define USB_VENDOR_ID_MOJO 0x8282
#define USB_DEVICE_ID_RETRO_ADAPTER 0x3201
diff --git a/drivers/hid/hid-microsoft.c b/drivers/hid/hid-microsoft.c
index 404a3a8..c6ef6ee 100644
--- a/drivers/hid/hid-microsoft.c
+++ b/drivers/hid/hid-microsoft.c
@@ -208,10 +208,6 @@ static const struct hid_device_id ms_devices[] = {
.driver_data = MS_NOGET },
{ HID_USB_DEVICE(USB_VENDOR_ID_MICROSOFT,
USB_DEVICE_ID_MS_COMFORT_MOUSE_4500),
.driver_data = MS_DUPLICATE_USAGES },
- { HID_USB_DEVICE(USB_VENDOR_ID_MICROSOFT,
USB_DEVICE_ID_MS_TYPE_COVER_2),
- .driver_data = 0 },
- { HID_USB_DEVICE(USB_VENDOR_ID_MICROSOFT,
USB_DEVICE_ID_MS_TOUCH_COVER_2),
- .driver_data = 0 },
{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_MICROSOFT,
USB_DEVICE_ID_MS_PRESENTER_8K_BT),
.driver_data = MS_PRESENTER },
--
1.8.3.2
^ permalink raw reply related
* [PATCH 2/2] Surface Pro 2 USB composite device handling
From: Derya @ 2014-03-09 17:56 UTC (permalink / raw)
To: linux-input, Benjamin Tissoires, Jiri Kosina
In-Reply-To: <531CAA1F.4080809@yahoo.de>
Surface Pro 2 USB composite device handling
(device changes it product id with attached keyboard cover)
- Adds MS Surface Pro 2's composite device ids, with and without attached
keyboard, to the hid-ids
- Ensures to load hid-core instead of hid-multitouch for the keyboard covers
- Adds HID_QUIRK_NO_INIT_INPUT_REPORTS to prevent USB submit urb failure
during keyboard cover (de)attaching
Signed-off-by: Derya <derya.kiran@yahoo.de>
---
drivers/hid/hid-core.c | 9 +++++++++
drivers/hid/hid-ids.h | 3 +++
drivers/hid/usbhid/hid-quirks.c | 3 +++
3 files changed, 15 insertions(+)
diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index bb5c494..f4e4820 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -775,6 +775,15 @@ static int hid_scan_report(struct hid_device *hid)
if ((parser->scan_flags & HID_SCAN_FLAG_MT_WIN_8) &&
(hid->group == HID_GROUP_MULTITOUCH))
hid->group = HID_GROUP_MULTITOUCH_WIN_8;
+
+ /*
+ * Handle vendor specific handlings
+ */
+ if ((hid->vendor == USB_VENDOR_ID_MICROSOFT) &&
+ (hid->product == USB_DEVICE_ID_MS_TYPE_COVER_2 ||
+ hid->product == USB_DEVICE_ID_MS_TOUCH_COVER_2) &&
+ (hid->group == HID_GROUP_MULTITOUCH))
+ hid->group = HID_GROUP_GENERIC;
vfree(parser);
return 0;
diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index 07cd28c..f61c0d8 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -624,6 +624,9 @@
#define USB_DEVICE_ID_MS_PRESENTER_8K_USB 0x0713
#define USB_DEVICE_ID_MS_DIGITAL_MEDIA_3K 0x0730
#define USB_DEVICE_ID_MS_COMFORT_MOUSE_4500 0x076c
+#define USB_DEVICE_ID_MS_SURFACE_PRO_2 0x0799
+#define USB_DEVICE_ID_MS_TOUCH_COVER_2 0x07a7
+#define USB_DEVICE_ID_MS_TYPE_COVER_2 0x07a9
#define USB_VENDOR_ID_MOJO 0x8282
#define USB_DEVICE_ID_RETRO_ADAPTER 0x3201
diff --git a/drivers/hid/usbhid/hid-quirks.c
b/drivers/hid/usbhid/hid-quirks.c
index dbd8387..7c4f86e 100644
--- a/drivers/hid/usbhid/hid-quirks.c
+++ b/drivers/hid/usbhid/hid-quirks.c
@@ -73,6 +73,9 @@ static const struct hid_blacklist {
{ USB_VENDOR_ID_FORMOSA, USB_DEVICE_ID_FORMOSA_IR_RECEIVER,
HID_QUIRK_NO_INIT_REPORTS },
{ USB_VENDOR_ID_FREESCALE, USB_DEVICE_ID_FREESCALE_MX28,
HID_QUIRK_NOGET },
{ USB_VENDOR_ID_MGE, USB_DEVICE_ID_MGE_UPS, HID_QUIRK_NOGET },
+ { USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_MS_SURFACE_PRO_2,
HID_QUIRK_NO_INIT_INPUT_REPORTS },
+ { USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_MS_TYPE_COVER_2,
HID_QUIRK_NO_INIT_INPUT_REPORTS },
+ { USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_MS_TOUCH_COVER_2,
HID_QUIRK_NO_INIT_INPUT_REPORTS },
{ USB_VENDOR_ID_MSI, USB_DEVICE_ID_MSI_GX680R_LED_PANEL,
HID_QUIRK_NO_INIT_REPORTS },
{ USB_VENDOR_ID_NEXIO, USB_DEVICE_ID_NEXIO_MULTITOUCH_PTI0750,
HID_QUIRK_NO_INIT_REPORTS },
{ USB_VENDOR_ID_NOVATEK, USB_DEVICE_ID_NOVATEK_MOUSE,
HID_QUIRK_NO_INIT_REPORTS },
--
1.8.3.2
^ permalink raw reply related
* Re: [PATCH v2 4/8] Input: pixcir_i2c_ts: Use Type-B Multi-Touch protocol
From: Roger Quadros @ 2014-03-10 8:57 UTC (permalink / raw)
To: Henrik Rydberg, dmitry.torokhov
Cc: jcbian, balbi, dmurphy, mugunthanvnm, linux-input, linux-kernel,
devicetree
In-Reply-To: <531B3331.9050008@euromail.se>
Hi Henrik,
On 03/08/2014 05:11 PM, Henrik Rydberg wrote:
> Hi Roger,
>
> the MT implementation seems mostly fine, just one curiosity:
>
>> static irqreturn_t pixcir_ts_isr(int irq, void *dev_id)
>> {
>> struct pixcir_i2c_ts_data *tsdata = dev_id;
>> const struct pixcir_ts_platform_data *pdata = tsdata->chip;
>> + struct pixcir_report_data report;
>>
>> while (!tsdata->exiting) {
>> - pixcir_ts_poscheck(tsdata);
>> -
>> - if (gpio_get_value(pdata->gpio_attb))
>> + /* parse packet */
>> + pixcir_ts_parse(tsdata, &report);
>> +
>> + /* report it */
>> + pixcir_ts_report(tsdata, &report);
>> +
>> + if (gpio_get_value(pdata->gpio_attb)) {
>> + if (report.num_touches) {
>> + /*
>> + * Last report with no finger up?
>> + * Do it now then.
>> + */
>> + input_mt_sync_frame(tsdata->input);
>> + input_sync(tsdata->input);
>
> Why is this special handling needed?
This is needed because the controller doesn't always report when all fingers
have left the screen. e.g. report might contain 3 fingers touched and then
gpio_attb line is de-asserted. There's no report with 0 fingers touched even
if the user's fingers have left the screen. So we never detect a BUTTON_UP.
Without this s/w workaround we observe side effects like buttons being pressed
but not released. To me it looks like a bug in the controller.
cheers,
-roger
^ permalink raw reply
* Re: [PATCH 5/5] ARM: multi_v7_defconfig: add st-keyscan driver
From: Lee Jones @ 2014-03-10 10:34 UTC (permalink / raw)
To: Gabriel FERNANDEZ
Cc: Dmitry Torokhov, Rob Herring, Pawel Moll, Mark Rutland,
Ian Campbell, Kumar Gala, Rob Landley, Russell King, Grant Likely,
devicetree, linux-doc, linux-kernel, linux-arm-kernel,
linux-input, kernel
In-Reply-To: <1393990772-9567-6-git-send-email-gabriel.fernandez@st.com>
> This patch adds KEYBOARD_ST_KEYSCAN config
>
> Signed-off-by: Gabriel Fernandez <gabriel.fernandez@st.com>
> ---
> arch/arm/configs/multi_v7_defconfig | 1 +
> 1 file changed, 1 insertion(+)
You should capitalise ST Keyscan in the $SUBJECT line, but other than that:
Acked-by: Lee Jones <lee.jones@linaro.org>
> diff --git a/arch/arm/configs/multi_v7_defconfig b/arch/arm/configs/multi_v7_defconfig
> index ee69829..5e926981 100644
> --- a/arch/arm/configs/multi_v7_defconfig
> +++ b/arch/arm/configs/multi_v7_defconfig
> @@ -131,6 +131,7 @@ CONFIG_INPUT_EVDEV=y
> CONFIG_KEYBOARD_GPIO=y
> CONFIG_KEYBOARD_TEGRA=y
> CONFIG_KEYBOARD_SPEAR=y
> +CONFIG_KEYBOARD_ST_KEYSCAN=y
> CONFIG_KEYBOARD_CROS_EC=y
> CONFIG_MOUSE_PS2_ELANTECH=y
> CONFIG_INPUT_MISC=y
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* Re: [PATCH v3 3/4] HID: sony: do not rely on hid_output_raw_report
From: Antonio Ospite @ 2014-03-10 11:32 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Benjamin Tissoires, Jiri Kosina, David Herrmann, David Barksdale,
linux-input, linux-kernel
In-Reply-To: <1394337163-32478-4-git-send-email-benjamin.tissoires@redhat.com>
On Sat, 8 Mar 2014 22:52:42 -0500
Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote:
> hid_out_raw_report is going to be obsoleted as it is not part of the
> unified HID low level transport documentation
> (Documentation/hid/hid-transport.txt)
>
> To do so, we need to introduce two new quirks:
> * HID_QUIRK_NO_OUTPUT_REPORTS_ON_INTR_EP: this quirks prevents the
> transport driver to use the interrupt channel to send output report
> (and thus force to use HID_REQ_SET_REPORT command)
> * HID_QUIRK_SKIP_OUTPUT_REPORT_ID: this one forces usbhid to not
> include the report ID in the buffer it sends to the device through
> HID_REQ_SET_REPORT in case of an output report
>
> This also fixes a regression introduced in commit 3a75b24949a8
> (HID: hidraw: replace hid_output_raw_report() calls by appropriates ones).
> The hidraw API was not able to communicate with the PS3 SixAxis
> controllers in USB mode.
>
> Reviewed-by: David Herrmann <dh.herrmann@gmail.com>
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
>
I confirm that using hidraw via USB works again now, thanks, so the
patch is OK:
Tested-by: Antonio Ospite <ao2@ao2.it>
However I still get the failure with hidraw via BT, I am going to look
into that.
> ---
> changes in v3:
> - no changes
>
> changes in v2:
> - removed usb.h include
> - renamed HID_QUIRK_NO_OUTPUT_REPORTS_ON_INTR_EP
> - fix typo
>
>
> drivers/hid/hid-sony.c | 60 ++++++++++---------------------------------
> drivers/hid/hidraw.c | 3 ++-
> drivers/hid/usbhid/hid-core.c | 7 ++++-
> include/linux/hid.h | 2 ++
> 4 files changed, 24 insertions(+), 48 deletions(-)
>
> diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
> index b5fe65e..4884bb5 100644
> --- a/drivers/hid/hid-sony.c
> +++ b/drivers/hid/hid-sony.c
> @@ -29,7 +29,6 @@
> #include <linux/hid.h>
> #include <linux/module.h>
> #include <linux/slab.h>
> -#include <linux/usb.h>
> #include <linux/leds.h>
> #include <linux/power_supply.h>
> #include <linux/spinlock.h>
> @@ -1007,45 +1006,6 @@ static int sony_mapping(struct hid_device *hdev, struct hid_input *hi,
> }
>
> /*
> - * The Sony Sixaxis does not handle HID Output Reports on the Interrupt EP
> - * like it should according to usbhid/hid-core.c::usbhid_output_raw_report()
> - * so we need to override that forcing HID Output Reports on the Control EP.
> - *
> - * There is also another issue about HID Output Reports via USB, the Sixaxis
> - * does not want the report_id as part of the data packet, so we have to
> - * discard buf[0] when sending the actual control message, even for numbered
> - * reports, humpf!
> - */
> -static int sixaxis_usb_output_raw_report(struct hid_device *hid, __u8 *buf,
> - size_t count, unsigned char report_type)
> -{
> - struct usb_interface *intf = to_usb_interface(hid->dev.parent);
> - struct usb_device *dev = interface_to_usbdev(intf);
> - struct usb_host_interface *interface = intf->cur_altsetting;
> - int report_id = buf[0];
> - int ret;
> -
> - if (report_type == HID_OUTPUT_REPORT) {
> - /* Don't send the Report ID */
> - buf++;
> - count--;
> - }
> -
> - ret = usb_control_msg(dev, usb_sndctrlpipe(dev, 0),
> - HID_REQ_SET_REPORT,
> - USB_DIR_OUT | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
> - ((report_type + 1) << 8) | report_id,
> - interface->desc.bInterfaceNumber, buf, count,
> - USB_CTRL_SET_TIMEOUT);
> -
> - /* Count also the Report ID, in case of an Output report. */
> - if (ret > 0 && report_type == HID_OUTPUT_REPORT)
> - ret++;
> -
> - return ret;
> -}
> -
> -/*
> * Sending HID_REQ_GET_REPORT changes the operation mode of the ps3 controller
> * to "operational". Without this, the ps3 controller will not report any
> * events.
> @@ -1305,11 +1265,8 @@ static void sixaxis_state_worker(struct work_struct *work)
> buf[10] |= sc->led_state[2] << 3;
> buf[10] |= sc->led_state[3] << 4;
>
> - if (sc->quirks & SIXAXIS_CONTROLLER_USB)
> - hid_output_raw_report(sc->hdev, buf, sizeof(buf), HID_OUTPUT_REPORT);
> - else
> - hid_hw_raw_request(sc->hdev, 0x01, buf, sizeof(buf),
> - HID_OUTPUT_REPORT, HID_REQ_SET_REPORT);
> + hid_hw_raw_request(sc->hdev, 0x01, buf, sizeof(buf), HID_OUTPUT_REPORT,
> + HID_REQ_SET_REPORT);
> }
>
> static void dualshock4_state_worker(struct work_struct *work)
> @@ -1659,7 +1616,18 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id)
> }
>
> if (sc->quirks & SIXAXIS_CONTROLLER_USB) {
> - hdev->hid_output_raw_report = sixaxis_usb_output_raw_report;
> + /*
> + * The Sony Sixaxis does not handle HID Output Reports on the
> + * Interrupt EP like it could, so we need to force HID Output
> + * Reports to use HID_REQ_SET_REPORT on the Control EP.
> + *
> + * There is also another issue about HID Output Reports via USB,
> + * the Sixaxis does not want the report_id as part of the data
> + * packet, so we have to discard buf[0] when sending the actual
> + * control message, even for numbered reports, humpf!
> + */
> + hdev->quirks |= HID_QUIRK_NO_OUTPUT_REPORTS_ON_INTR_EP;
> + hdev->quirks |= HID_QUIRK_SKIP_OUTPUT_REPORT_ID;
> ret = sixaxis_set_operational_usb(hdev);
> sc->worker_initialized = 1;
> INIT_WORK(&sc->state_worker, sixaxis_state_worker);
> diff --git a/drivers/hid/hidraw.c b/drivers/hid/hidraw.c
> index 2cc484c..ffa648c 100644
> --- a/drivers/hid/hidraw.c
> +++ b/drivers/hid/hidraw.c
> @@ -149,7 +149,8 @@ static ssize_t hidraw_send_report(struct file *file, const char __user *buffer,
> goto out_free;
> }
>
> - if (report_type == HID_OUTPUT_REPORT) {
> + if ((report_type == HID_OUTPUT_REPORT) &&
> + !(dev->quirks & HID_QUIRK_NO_OUTPUT_REPORTS_ON_INTR_EP)) {
> ret = hid_hw_output_report(dev, buf, count);
> /*
> * compatibility with old implementation of USB-HID and I2C-HID:
> diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
> index 0d1d875..3bc7cad 100644
> --- a/drivers/hid/usbhid/hid-core.c
> +++ b/drivers/hid/usbhid/hid-core.c
> @@ -894,7 +894,12 @@ static int usbhid_set_raw_report(struct hid_device *hid, unsigned int reportnum,
> int ret, skipped_report_id = 0;
>
> /* Byte 0 is the report number. Report data starts at byte 1.*/
> - buf[0] = reportnum;
> + if ((rtype == HID_OUTPUT_REPORT) &&
> + (hid->quirks & HID_QUIRK_SKIP_OUTPUT_REPORT_ID))
> + buf[0] = 0;
> + else
> + buf[0] = reportnum;
> +
> if (buf[0] == 0x0) {
> /* Don't send the Report ID */
> buf++;
> diff --git a/include/linux/hid.h b/include/linux/hid.h
> index e224516..2cd7174 100644
> --- a/include/linux/hid.h
> +++ b/include/linux/hid.h
> @@ -290,6 +290,8 @@ struct hid_item {
> #define HID_QUIRK_NO_EMPTY_INPUT 0x00000100
> #define HID_QUIRK_NO_INIT_INPUT_REPORTS 0x00000200
> #define HID_QUIRK_SKIP_OUTPUT_REPORTS 0x00010000
> +#define HID_QUIRK_SKIP_OUTPUT_REPORT_ID 0x00020000
> +#define HID_QUIRK_NO_OUTPUT_REPORTS_ON_INTR_EP 0x00040000
> #define HID_QUIRK_FULLSPEED_INTERVAL 0x10000000
> #define HID_QUIRK_NO_INIT_REPORTS 0x20000000
> #define HID_QUIRK_NO_IGNORE 0x40000000
> --
> 1.8.5.3
>
--
Antonio Ospite
http://ao2.it
A: Because it messes up the order in which people normally read text.
See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?
^ permalink raw reply
* Re: [PATCH 1/5] drivers: input: keyboard: st-keyscan: add keyscan driver
From: Lee Jones @ 2014-03-10 11:48 UTC (permalink / raw)
To: Gabriel FERNANDEZ
Cc: Dmitry Torokhov, Rob Herring, Pawel Moll, Mark Rutland,
Ian Campbell, Kumar Gala, Rob Landley, Russell King, Grant Likely,
devicetree, linux-doc, linux-kernel, linux-arm-kernel,
linux-input, kernel, Giuseppe Condorelli
In-Reply-To: <1393990772-9567-2-git-send-email-gabriel.fernandez@st.com>
Hi Gabi,
Sorry for the delay. It was a hectic week last week.
As promised:
> This patch adds ST Keyscan driver to use the keypad hw a subset
> of ST boards provide. Specific board setup will be put in the
> given dt.
>
> Signed-off-by: Giuseppe Condorelli <giuseppe.condorelli@st.com>
> Signed-off-by: Gabriel Fernandez <gabriel.fernandez@st.com>
Are you sure these are in the correct order?
What is the history of this commit?
> ---
> .../devicetree/bindings/input/st-keypad.txt | 50 ++++
This should be submitted as a seperate patch.
> drivers/input/keyboard/Kconfig | 12 +
> drivers/input/keyboard/Makefile | 1 +
> drivers/input/keyboard/st-keyscan.c | 323 +++++++++++++++++++++
> 4 files changed, 386 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/input/st-keypad.txt
> create mode 100644 drivers/input/keyboard/st-keyscan.c
>
> diff --git a/Documentation/devicetree/bindings/input/st-keypad.txt b/Documentation/devicetree/bindings/input/st-keypad.txt
> new file mode 100644
> index 0000000..63eb07a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/input/st-keypad.txt
> @@ -0,0 +1,50 @@
> +* ST Keypad controller device tree bindings
s/device tree/Device Tree
> +
> +The ST keypad controller device tree binding is based on the
As above.
> +matrix-keymap.
> +
> +Required properties:
> +
> +- compatible: "st,keypad"
I think there will be subsequent st,keypad drivers? Is there any way
we can make this compatible string more specific to _this_ device?
st,stih4xx-keypad?
> +- reg: Register base address of st-keypad controler.
s/address/address and size AND s/controler/controller
> +- interrupts: Interrupt numberof st-keypad controler.
s/numberof/number for the
> +- clocks: Must contain one entry, for the module clock.
> + See ../clocks/clock-bindings.txt for details.
Great!
> +- pinctrl-0: Should specify pin control groups used for this controller.
> +
> +- pinctrl-names: Should contain only one value - "default".
Like to ../pinctrl/pinctrl-bindings.txt
> +- linux,keymap: The keymap for keys as described in the binding document
> + devicetree/bindings/input/matrix-keymap.txt.
> +
> +- keypad,num-rows: Number of row lines connected to the keypad controller.
> +
> +- keypad,num-columns: Number of column lines connected to the keypad
> + controller.
> +
> +- st,debounce_us: Debouncing interval time in microseconds
I'm sure there will be a shared binding for de-bounce.
If not, there certainly should be.
> +
> +
Superfluous new lines.
> +Example:
> +
> +keyscan: keyscan@fe4b0000 {
> + compatible = "st,keypad";
Is there any way we can make this more specific to _this_ IP?
> + reg = <0xfe4b0000 0x2000>;
> + interrupts = <GIC_SPI 212 IRQ_TYPE_NONE>;
> + clocks = <&CLK_SYSIN>;
> + pinctrl-names = "default";
> + pinctrl-0 = <&pinctrl_keyscan>;
> +
> + keypad,num-rows = <4>;
> + keypad,num-columns = <4>;
> + st,debounce_us = <5000>;
> + linux,keymap = < /* in0 in1 in2 in3 */
Do these line up in the file? I know Git can be a bit funny about this
sometimes.
> + KEY_F13 KEY_F9 KEY_F5 KEY_F1 /* out0 */
> + KEY_F14 KEY_F10 KEY_F6 KEY_F2 /* out1 */
> + KEY_F15 KEY_F11 KEY_F7 KEY_F3 /* out2 */
> + KEY_F16 KEY_F12 KEY_F8 KEY_F4 >; /* out3 */
> +};
> diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig
> index a673c9f..9e29125 100644
> --- a/drivers/input/keyboard/Kconfig
> +++ b/drivers/input/keyboard/Kconfig
> @@ -512,6 +512,18 @@ config KEYBOARD_STOWAWAY
> To compile this driver as a module, choose M here: the
> module will be called stowaway.
>
> +config KEYBOARD_ST_KEYSCAN
> + tristate "STMicroelectronics keyscan support"
> + depends on ARCH_STI
> + select INPUT_EVDEV
> + select INPUT_MATRIXKMAP
> + help
> + Say Y here if you want to use a keypad attached to the keyscan block
> + on some STMicroelectronics SoC devices.
Might be worth mentioning which ones.
> + To compile this driver as a module, choose M here: the
> + module will be called stm-keyscan.
> +
> config KEYBOARD_SUNKBD
> tristate "Sun Type 4 and Type 5 keyboard"
> select SERIO
> diff --git a/drivers/input/keyboard/Makefile b/drivers/input/keyboard/Makefile
> index a699b61..5fd020a 100644
> --- a/drivers/input/keyboard/Makefile
> +++ b/drivers/input/keyboard/Makefile
> @@ -50,6 +50,7 @@ obj-$(CONFIG_KEYBOARD_SH_KEYSC) += sh_keysc.o
> obj-$(CONFIG_KEYBOARD_SPEAR) += spear-keyboard.o
> obj-$(CONFIG_KEYBOARD_STMPE) += stmpe-keypad.o
> obj-$(CONFIG_KEYBOARD_STOWAWAY) += stowaway.o
> +obj-$(CONFIG_KEYBOARD_ST_KEYSCAN) += st-keyscan.o
> obj-$(CONFIG_KEYBOARD_SUNKBD) += sunkbd.o
> obj-$(CONFIG_KEYBOARD_TC3589X) += tc3589x-keypad.o
> obj-$(CONFIG_KEYBOARD_TEGRA) += tegra-kbc.o
> diff --git a/drivers/input/keyboard/st-keyscan.c b/drivers/input/keyboard/st-keyscan.c
> new file mode 100644
> index 0000000..606cc19
> --- /dev/null
> +++ b/drivers/input/keyboard/st-keyscan.c
> @@ -0,0 +1,323 @@
> +/*
> + * STMicroelectronics Key Scanning driver
> + *
> + * Copyright (c) 2014 STMicroelectonics Ltd.
> + * Author: Stuart Menefy <stuart.menefy@st.com>
> + *
> + * Based on sh_keysc.c, copyright 2008 Magnus Damm
Did sh_keysc.c ever exist in Mainline?
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/interrupt.h>
> +#include <linux/platform_device.h>
> +#include <linux/clk.h>
> +#include <linux/io.h>
> +#include <linux/input/matrix_keypad.h>
> +
> +#define ST_KEYSCAN_MAXKEYS 16
> +
> +#define KEYSCAN_CONFIG_OFF 0x000
> +#define KEYSCAN_CONFIG_ENABLE 1
0x001?
> +#define KEYSCAN_DEBOUNCE_TIME_OFF 0x004
> +#define KEYSCAN_MATRIX_STATE_OFF 0x008
> +#define KEYSCAN_MATRIX_DIM_OFF 0x00c
Odd that these are 3 digit padded? Is there a reason for that?
> +#define KEYSCAN_MATRIX_DIM_X_SHIFT 0
> +#define KEYSCAN_MATRIX_DIM_Y_SHIFT 2
For all 'ST_KEYSCAN_...'?
> +struct keypad_platform_data {
> + const struct matrix_keymap_data *keymap_data;
> + unsigned int num_out_pads;
> + unsigned int num_in_pads;
> + unsigned int debounce_us;
> +};
> +
> +struct keyscan_priv {
> + void __iomem *base;
> + int irq;
> + struct clk *clk;
> + struct input_dev *input_dev;
> + struct keypad_platform_data *config;
> + unsigned int last_state;
> + u32 keycodes[ST_KEYSCAN_MAXKEYS];
Seems odd to limit this. Can't the information come from DT
i.e. keypad,num-rows and keypad,num-columns?
> +};
> +
> +static irqreturn_t keyscan_isr(int irq, void *dev_id)
> +{
> + struct platform_device *pdev = dev_id;
> + struct keyscan_priv *priv = platform_get_drvdata(pdev);
> + int state;
> + int change;
> +
> + state = readl(priv->base + KEYSCAN_MATRIX_STATE_OFF) & 0xffff;
> + change = priv->last_state ^ state;
> +
> + while (change) {
> + int scancode = __ffs(change);
> + int down = state & (1 << scancode);
int down = state & BIT(scancode);
> + input_report_key(priv->input_dev, priv->keycodes[scancode],
> + down);
> + change ^= 1 << scancode;
change ^= BIT(scancode);
> + };
> +
> + input_sync(priv->input_dev);
> +
> + priv->last_state = state;
> +
> + return IRQ_HANDLED;
> +}
> +
> +static void keyscan_start(struct keyscan_priv *priv)
> +{
> + clk_enable(priv->clk);
> +
> + writel(priv->config->debounce_us * (clk_get_rate(priv->clk) / 1000000),
> + priv->base + KEYSCAN_DEBOUNCE_TIME_OFF);
> +
> + writel(((priv->config->num_in_pads - 1) << KEYSCAN_MATRIX_DIM_X_SHIFT) |
> + ((priv->config->num_out_pads - 1) << KEYSCAN_MATRIX_DIM_Y_SHIFT),
> + priv->base + KEYSCAN_MATRIX_DIM_OFF);
> +
> + writel(KEYSCAN_CONFIG_ENABLE, priv->base + KEYSCAN_CONFIG_OFF);
> +}
> +
> +static void keyscan_stop(struct keyscan_priv *priv)
> +{
> + writel(0, priv->base + KEYSCAN_CONFIG_OFF);
> +
> + clk_disable(priv->clk);
> +}
> +
> +static int keypad_matrix_key_parse_dt(struct keyscan_priv *st_kp)
> +{
> + struct device *dev = st_kp->input_dev->dev.parent;
> + struct device_node *np = dev->of_node;
> + struct keypad_platform_data *pdata;
> + int error;
> +
> + pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
> + if (!pdata) {
> + dev_err(dev, "failed to allocate driver pdata\n");
If the system is OOM, this sort of error message will be pretty
redundant. Just return -ENOMEM instead.
> + return -ENOMEM;
> + }
> +
> + error = matrix_keypad_parse_of_params(dev, &pdata->num_out_pads,
> + &pdata->num_in_pads);
> + if (error) {
> + dev_err(dev, "failed to parse keypad params\n");
> + return error;
Nit: It's pretty unusual to use this for a standard error handling
variable. Consider 'ret' or 'err' as a replacement.
> + }
> +
> + of_property_read_u32(np, "st,debounce_us", &pdata->debounce_us);
Isn't this a required property? Might be worth checking the return
value if so.
> + st_kp->config = pdata;
> +
> + dev_info(dev, "rows=%d col=%d debounce=%d\n",
> + pdata->num_out_pads,
> + pdata->num_in_pads,
> + pdata->debounce_us);
Might be worth moving this down passed the final point of failure.
> + error = of_property_read_u32_array(np, "linux,keymap",
> + st_kp->keycodes, ST_KEYSCAN_MAXKEYS);
> + if (error) {
> + dev_err(dev, "failed to parse keymap\n");
> + return error;
> + }
> +
> + return 0;
> +}
> +
> +static int __init keyscan_probe(struct platform_device *pdev)
> +{
> + struct keypad_platform_data *pdata =
> + dev_get_platdata(&pdev->dev);
Do we really support platform data still?
> + struct keyscan_priv *st_kp;
> + struct input_dev *input_dev;
> + struct device *dev = &pdev->dev;
dev and pdev are used randomly in probe(). Just remove the dev
de-reference and use &pdev->dev exclusively.
It's pretty common to de-reference 'np = pdev->dev.of_node' though.
> + struct resource *res;
> + int len;
> + int error;
> + int i;
> +
> + if (!pdata && !pdev->dev.of_node) {
> + dev_err(&pdev->dev, "no keymap defined\n");
> + return -EINVAL;
> + }
> +
> + st_kp = devm_kzalloc(dev, sizeof(*st_kp), GFP_KERNEL);
> + if (!st_kp) {
> + dev_err(dev, "failed to allocate driver data\n");
> + return -ENOMEM;
I wouldn't print any messages on -ENOMEM personally. It's not a driver
failure per se, but a system one where the user will soon be notified.
> + }
> + st_kp->config = pdata;
You only want to do this in the !np case.
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!res) {
> + dev_err(dev, "no I/O memory specified\n");
> + return -ENXIO;
> + }
> +
> + len = resource_size(res);
> + if (!devm_request_mem_region(dev, res->start, len, pdev->name)) {
> + dev_err(dev, "failed to reserve I/O memory\n");
> + return -EBUSY;
> + }
> +
> + st_kp->base = devm_ioremap_nocache(dev, res->start, len);
> + if (st_kp->base == NULL) {
> + dev_err(dev, "failed to remap I/O memory\n");
> + return -ENXIO;
> + }
Replace the two above with devm_ioremap_resource().
Make sure the IORESOURCE_CACHEABLE is set.
> + st_kp->irq = platform_get_irq(pdev, 0);
> + if (st_kp->irq < 0) {
> + dev_err(dev, "no IRQ specified\n");
> + return -ENXIO;
No such device or address, are you sure?
Perhaps -EINVAL would be better, as one should be specified.
> + }
> +
> + error = devm_request_irq(dev, st_kp->irq, keyscan_isr, 0,
> + pdev->name, pdev);
> + if (error) {
> + dev_err(dev, "failed to request IRQ\n");
> + return error;
> + }
> +
> + input_dev = devm_input_allocate_device(&pdev->dev);
> + if (!input_dev) {
> + dev_err(&pdev->dev, "failed to allocate the input device\n");
> + return -ENOMEM;
> + }
> +
> + st_kp->clk = devm_clk_get(dev, NULL);
> + if (IS_ERR(st_kp->clk)) {
> + dev_err(dev, "cannot get clock");
> + return PTR_ERR(st_kp->clk);
> + }
> +
> + input_dev = input_allocate_device();
> + if (!input_dev) {
> + dev_err(dev, "failed to allocate input device\n");
> + return -ENOMEM;
> + }
Wasn't this done already?
> + st_kp->input_dev = input_dev;
> +
> + input_dev->name = pdev->name;
> + input_dev->phys = "keyscan-keys/input0";
> + input_dev->dev.parent = dev;
> +
> + input_dev->id.bustype = BUS_HOST;
> + input_dev->id.vendor = 0x0001;
> + input_dev->id.product = 0x0001;
> + input_dev->id.version = 0x0100;
Any chance we can #define these?
> + if (!pdata) {
> + error = keypad_matrix_key_parse_dt(st_kp);
> + if (error)
> + return error;
> + pdata = st_kp->config;
Is pdata used after this?
> + }
> +
> + input_dev->keycode = st_kp->keycodes;
> + input_dev->keycodesize = sizeof(st_kp->keycodes[0]);
> + input_dev->keycodemax = ARRAY_SIZE(st_kp->keycodes);
> +
> + for (i = 0; i < ST_KEYSCAN_MAXKEYS; i++)
> + input_set_capability(input_dev, EV_KEY, st_kp->keycodes[i]);
> + __clear_bit(KEY_RESERVED, input_dev->keybit);
> +
> + error = input_register_device(input_dev);
> + if (error) {
> + dev_err(dev, "failed to register input device\n");
> + input_free_device(input_dev);
> + platform_set_drvdata(pdev, NULL);
You don't need to do this anymore. It's taken care of elsewhere.
> + return error;
> + }
> +
> + platform_set_drvdata(pdev, st_kp);
> +
> + keyscan_start(st_kp);
> +
> + device_set_wakeup_capable(&pdev->dev, 1);
> +
> + return 0;
> +}
> +
> +static int __exit keyscan_remove(struct platform_device *pdev)
> +{
> + struct keyscan_priv *priv = platform_get_drvdata(pdev);
> +
> + keyscan_stop(priv);
> +
> + input_unregister_device(priv->input_dev);
> + platform_set_drvdata(pdev, NULL);
> +
> + return 0;
I already saw that Dmitry commented on the rest of the file.
<snip>
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* Re: [PATCH 2/5] ARM: STi: DT: add keyscan for stih415
From: Lee Jones @ 2014-03-10 11:50 UTC (permalink / raw)
To: Gabriel FERNANDEZ
Cc: Dmitry Torokhov, Rob Herring, Pawel Moll, Mark Rutland,
Ian Campbell, Kumar Gala, Rob Landley, Russell King, Grant Likely,
devicetree, linux-doc, linux-kernel, linux-arm-kernel,
linux-input, kernel, Giuseppe CONDORELLI
In-Reply-To: <1393990772-9567-3-git-send-email-gabriel.fernandez@st.com>
> From: Giuseppe CONDORELLI <giuseppe.condorelli@st.com>
>
> Add keyscan support for stih415.
> It is put disabled by default because it is not enabled on all boards
> Also there are PIOs conflict with already claimed lines.
>
> Signed-off-by: Giuseppe Condorelli <giuseppe.condorelli@st.com>
> Signed-off-by: Gabriel Fernandez <gabriel.fernandez@st.com>
> ---
> arch/arm/boot/dts/stih415-pinctrl.dtsi | 16 ++++++++++++++++
> arch/arm/boot/dts/stih415.dtsi | 10 ++++++++++
> 2 files changed, 26 insertions(+)
<snip>
> + keyscan: keyscan@fe4b0000 {
> + compatible = "st,keypad";
> + status = "disabled";
> + reg = <0xfe4b0000 0x2000>;
> + interrupts = <GIC_SPI 212 IRQ_TYPE_NONE>;
> + clocks = <&CLK_SYSIN>;
> + pinctrl-names = "default";
Small nit here, but otherwise:
Acked-by: Lee Jones <lee.jones@linaro.org>
> + pinctrl-0 = <&pinctrl_keyscan>;
> + };
> };
> };
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 3/5] ARM: STi: DT: add keyscan for stih416
From: Lee Jones @ 2014-03-10 11:52 UTC (permalink / raw)
To: Gabriel FERNANDEZ
Cc: Dmitry Torokhov, Rob Herring, Pawel Moll, Mark Rutland,
Ian Campbell, Kumar Gala, Rob Landley, Russell King, Grant Likely,
devicetree, linux-doc, linux-kernel, linux-arm-kernel,
linux-input, kernel, Giuseppe Condorelli
In-Reply-To: <1393990772-9567-4-git-send-email-gabriel.fernandez@st.com>
> Add keyscan support for stih416.
> It is disabled by default given that it is not enabled on all boards.
> Also there are PIOs conflict with already claimed lines.
>
> Signed-off-by: Giuseppe Condorelli <giuseppe.condorelli@st.com>
> Signed-off-by: Gabriel Fernandez <gabriel.fernandez@st.com>
> ---
> arch/arm/boot/dts/stih416-pinctrl.dtsi | 16 ++++++++++++++++
> arch/arm/boot/dts/stih416.dtsi | 10 ++++++++++
> 2 files changed, 26 insertions(+)
Looks good:
Acked-by: Lee Jones <lee.jones@linaro.org>
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 4/5] ARM: STi: DT: add keyscan for stih41x-b2000
From: Lee Jones @ 2014-03-10 11:54 UTC (permalink / raw)
To: Gabriel FERNANDEZ
Cc: Dmitry Torokhov, Rob Herring, Pawel Moll, Mark Rutland,
Ian Campbell, Kumar Gala, Rob Landley, Russell King, Grant Likely,
devicetree, linux-doc, linux-kernel, linux-arm-kernel,
linux-input, kernel, Giuseppe Condorelli
In-Reply-To: <1393990772-9567-5-git-send-email-gabriel.fernandez@st.com>
On Wed, 05 Mar 2014, Gabriel FERNANDEZ wrote:
> Add keyscan setup for stih415/h416 b2000.
> Both have same raw/column lines number, debounce time and keymap.
>
> Signed-off-by: Giuseppe Condorelli <giuseppe.condorelli@st.com>
> Signed-off-by: Gabriel Fernandez <gabriel.fernandez@st.com>
> ---
> arch/arm/boot/dts/stih41x-b2000.dtsi | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> diff --git a/arch/arm/boot/dts/stih41x-b2000.dtsi b/arch/arm/boot/dts/stih41x-b2000.dtsi
> index 1e6aa92..cf06beb 100644
> --- a/arch/arm/boot/dts/stih41x-b2000.dtsi
> +++ b/arch/arm/boot/dts/stih41x-b2000.dtsi
> @@ -6,6 +6,7 @@
> * it under the terms of the GNU General Public License version 2 as
> * publishhed by the Free Software Foundation.
> */
> +#include <dt-bindings/input/input.h>
> / {
>
> memory{
> @@ -46,5 +47,16 @@
>
> status = "okay";
> };
> +
> + keyscan: keyscan@fe4b0000 {
> + keypad,num-rows = <4>;
> + keypad,num-columns = <4>;
> + st,debounce_us = <5000>;
> + linux,keymap = < /* in0 in1 in2 in3 */
Nit: We should try to line the headers up with their associated values
(perhaps they just look misalined in the email and are actually fine
in the text file?)
> + KEY_F13 KEY_F9 KEY_F5 KEY_F1 /* out0 */
> + KEY_F14 KEY_F10 KEY_F6 KEY_F2 /* out1 */
> + KEY_F15 KEY_F11 KEY_F7 KEY_F3 /* out2 */
> + KEY_F16 KEY_F12 KEY_F8 KEY_F4 >; /* out3 */
> + };
Patch looks good though:
Acked-by: Lee Jones <lee.jones@linaro.org>
> };
> };
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 01/05] input synaptics-rmi4: Split F01 definitions out into header file for use by reflash.
From: Courtney Cavin @ 2014-03-10 14:46 UTC (permalink / raw)
To: Christopher Heiny
Cc: Dmitry Torokhov, Linux Input, Andrew Duggan, Vincent Huang,
Vivian Ly, Daniel Rosenberg, Linus Walleij, Benjamin Tissoires,
David Herrmann, Jiri Kosina
In-Reply-To: <1394245795-17347-1-git-send-email-cheiny@synaptics.com>
On Sat, Mar 08, 2014 at 03:29:51AM +0100, Christopher Heiny wrote:
> Signed-off-by: Christopher Heiny <cheiny@synaptics.com>
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> Cc: Linux Walleij <linus.walleij@linaro.org>
> Cc: David Herrmann <dh.herrmann@gmail.com>
> Cc: Jiri Kosina <jkosina@suse.cz>
>
> ---
>
> drivers/input/rmi4/rmi_f01.c | 96 ++-----------------------------------
> drivers/input/rmi4/rmi_f01.h | 110 +++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 114 insertions(+), 92 deletions(-)
$SUBJECT is 83 characters long. Please be more brief and provide a
description.
[...]
> +#define RMI_SLEEP_MODE_NORMAL 0x00
> +#define RMI_SLEEP_MODE_SENSOR_SLEEP 0x01
> +#define RMI_SLEEP_MODE_RESERVED0 0x02
> +#define RMI_SLEEP_MODE_RESERVED1 0x03
> +
> +#define RMI_IS_VALID_SLEEPMODE(mode) \
> + (mode >= RMI_SLEEP_MODE_NORMAL && mode <= RMI_SLEEP_MODE_RESERVED1)
> +
I might be missing something, but these seem like the only defines used
in the flash code. Why not keep these in the f01 driver, and export
a couple more functions, like rmi_f01_reset() and rmi_f01_set_sleep_mode() ?
-Courtney
^ permalink raw reply
* Re: [PATCH 03/05] input synaptics-rmi4: rmi_f01 - Fix a comment and add a diagnostic output message.
From: Courtney Cavin @ 2014-03-10 14:51 UTC (permalink / raw)
To: Christopher Heiny
Cc: Dmitry Torokhov, Linux Input, Andrew Duggan, Vincent Huang,
Vivian Ly, Daniel Rosenberg, Linus Walleij, Benjamin Tissoires,
David Herrmann, Jiri Kosina
In-Reply-To: <1394245795-17347-3-git-send-email-cheiny@synaptics.com>
On Sat, Mar 08, 2014 at 03:29:53AM +0100, Christopher Heiny wrote:
> In debugging certain touch sensor failures, it's useful to know
> whether the device is stuck in bootloader, so print a message
> to that effect.
>
> Also, point to the actual location of the defs for the F01 CTRL0
> bitfields.
>
> Signed-off-by: Christopher Heiny <cheiny@synaptics.com>
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> Cc: Linux Walleij <linus.walleij@linaro.org>
> Cc: David Herrmann <dh.herrmann@gmail.com>
> Cc: Jiri Kosina <jkosina@suse.cz>
>
> ---
>
> drivers/input/rmi4/rmi_f01.c | 9 ++++++++-
> 1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/input/rmi4/rmi_f01.c b/drivers/input/rmi4/rmi_f01.c
> index 8504865..a078d7d 100644
> --- a/drivers/input/rmi4/rmi_f01.c
> +++ b/drivers/input/rmi4/rmi_f01.c
> @@ -16,7 +16,7 @@
> #include "rmi_f01.h"
>
> /**
> - * @ctrl0 - see the bit definitions above.
> + * @ctrl0 - see the bit definitions in rmi_f01.h.
> * @doze_interval - controls the interval between checks for finger presence
> * when the touch sensor is in doze mode, in units of 10ms.
> * @wakeup_threshold - controls the capacitance threshold at which the touch
> @@ -415,6 +415,13 @@ static int rmi_f01_probe(struct rmi_function *fn)
> return error;
> }
>
> + driver_data->f01_bootloader_mode =
> + RMI_F01_STATUS_BOOTLOADER(device_status);
> + if (driver_data->f01_bootloader_mode)
> + dev_warn(&rmi_dev->dev,
> + "WARNING: RMI4 device is in bootloader mode!\n");
> +
> +
The logic here is a bit odd. Would it make sense to put this warning in
the if condition below? IIRC you can't have a configured device while
in bootloader mode.
> if (RMI_F01_STATUS_UNCONFIGURED(device_status)) {
> dev_err(&fn->dev,
> "Device was reset during configuration process, status: %#02x!\n",
-Courtney
^ permalink raw reply
* Re: [PATCH 04/05] input synaptics-rmi4: rmi_driver - Export some symbols and functions for use by reflash.
From: Courtney Cavin @ 2014-03-10 15:04 UTC (permalink / raw)
To: Christopher Heiny
Cc: Dmitry Torokhov, Linux Input, Andrew Duggan, Vincent Huang,
Vivian Ly, Daniel Rosenberg, Linus Walleij, Benjamin Tissoires,
David Herrmann, Jiri Kosina
In-Reply-To: <1394245795-17347-4-git-send-email-cheiny@synaptics.com>
On Sat, Mar 08, 2014 at 03:29:54AM +0100, Christopher Heiny wrote:
> Reflash functionality will need to unload the existing functions and
> rescan the PDT before starting reflash; then reload the functions
> afterwards.
>
> Signed-off-by: Christopher Heiny <cheiny@synaptics.com>
> Signed-off-by: Vincent Huang <vincent.huang@tw.synaptics.com>
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> Cc: Linux Walleij <linus.walleij@linaro.org>
> Cc: David Herrmann <dh.herrmann@gmail.com>
> Cc: Jiri Kosina <jkosina@suse.cz>
>
> ---
>
> drivers/input/rmi4/rmi_driver.c | 165 ++++++++++++++++++++++------------------
> drivers/input/rmi4/rmi_driver.h | 22 +++---
> 2 files changed, 101 insertions(+), 86 deletions(-)
[...]
> diff --git a/drivers/input/rmi4/rmi_driver.h b/drivers/input/rmi4/rmi_driver.h
[...]
> -int rmi_read_pdt_entry(struct rmi_device *rmi_dev, struct pdt_entry *entry,
> - u16 pdt_address);
> +#define RMI_SCAN_CONTINUE 0
> +#define RMI_SCAN_DONE 1
> +
> +int rmi_scan_pdt(struct rmi_device *rmi_dev, void *ctx,
> + int (*callback)(struct rmi_device *rmi_dev,
> + void *ctx, const struct pdt_entry *entry));
I don't really like this callback. The main reason for it is early
abort of PDT scanning, right? It is really that beneficial?
>
> bool rmi_is_physical_driver(struct device_driver *);
> int rmi_register_physical_driver(void);
> @@ -113,4 +109,10 @@ void rmi_unregister_physical_driver(void);
> int rmi_register_f01_handler(void);
> void rmi_unregister_f01_handler(void);
>
> +int check_bootloader_mode(struct rmi_device *rmi_dev,
> + const struct pdt_entry *pdt);
This is a silly function name to put in a header. rmi_* perhaps?
> +void rmi_free_function_list(struct rmi_device *rmi_dev);
> +int rmi_driver_detect_functions(struct rmi_device *rmi_dev);
> +
> +
> #endif
-Courtney
^ permalink raw reply
* Re: [PATCH 1/5] drivers: input: keyboard: st-keyscan: add keyscan driver
From: Dmitry Torokhov @ 2014-03-10 15:28 UTC (permalink / raw)
To: Lee Jones
Cc: Gabriel FERNANDEZ, Rob Herring, Pawel Moll, Mark Rutland,
Ian Campbell, Kumar Gala, Rob Landley, Russell King, Grant Likely,
devicetree, linux-doc, linux-kernel, linux-arm-kernel,
linux-input, kernel, Giuseppe Condorelli
In-Reply-To: <20140310114819.GO14976@lee--X1>
Hi Lee,
On Mon, Mar 10, 2014 at 11:48:19AM +0000, Lee Jones wrote:
> Hi Gabi,
>
> Sorry for the delay. It was a hectic week last week.
>
> As promised:
>
> > This patch adds ST Keyscan driver to use the keypad hw a subset
> > of ST boards provide. Specific board setup will be put in the
> > given dt.
> >
> > Signed-off-by: Giuseppe Condorelli <giuseppe.condorelli@st.com>
> > Signed-off-by: Gabriel Fernandez <gabriel.fernandez@st.com>
>
> Are you sure these are in the correct order?
>
> What is the history of this commit?
>
> > ---
> > .../devicetree/bindings/input/st-keypad.txt | 50 ++++
>
> This should be submitted as a seperate patch.
Why do we have such requirement? To me it would make more sense to add
binding documentation in the same commit as the code that uses these
bindings.
[...]
> > +
> > + error = matrix_keypad_parse_of_params(dev, &pdata->num_out_pads,
> > + &pdata->num_in_pads);
> > + if (error) {
> > + dev_err(dev, "failed to parse keypad params\n");
> > + return error;
>
> Nit: It's pretty unusual to use this for a standard error handling
> variable. Consider 'ret' or 'err' as a replacement.
I like "error", in fact there are a lot of these in input. I use "error" for
data that is only returned from error path and "retval" when the same
variable is returned in both success and error paths.
> > +
> > + input_dev->id.bustype = BUS_HOST;
> > + input_dev->id.vendor = 0x0001;
> > + input_dev->id.product = 0x0001;
> > + input_dev->id.version = 0x0100;
>
> Any chance we can #define these?
Even better would be not use 0x0001 as vendor as there (unfortunately)
quite a few other drivers use it already. Either omit or chose something
else. Does ST have PCI or USB VID assigned?
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH 1/5] drivers: input: keyboard: st-keyscan: add keyscan driver
From: Lee Jones @ 2014-03-10 15:38 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Gabriel FERNANDEZ, Rob Herring, Pawel Moll, Mark Rutland,
Ian Campbell, Kumar Gala, Rob Landley, Russell King, Grant Likely,
devicetree, linux-doc, linux-kernel, linux-arm-kernel,
linux-input, kernel, Giuseppe Condorelli
In-Reply-To: <20140310152859.GA29054@core.coreip.homeip.net>
> > > This patch adds ST Keyscan driver to use the keypad hw a subset
> > > of ST boards provide. Specific board setup will be put in the
> > > given dt.
> > >
> > > Signed-off-by: Giuseppe Condorelli <giuseppe.condorelli@st.com>
> > > Signed-off-by: Gabriel Fernandez <gabriel.fernandez@st.com>
> >
> > Are you sure these are in the correct order?
> >
> > What is the history of this commit?
> >
> > > ---
> > > .../devicetree/bindings/input/st-keypad.txt | 50 ++++
> >
> > This should be submitted as a seperate patch.
>
> Why do we have such requirement? To me it would make more sense to add
> binding documentation in the same commit as the code that uses these
> bindings.
I'm inclined to agree with you and that's actually how we used to do
it, but a decision was made by the DT guys at one of the Kernel
Summits to submit Documentation as a separate patch.
> [...]
>
> > > +
> > > + error = matrix_keypad_parse_of_params(dev, &pdata->num_out_pads,
> > > + &pdata->num_in_pads);
> > > + if (error) {
> > > + dev_err(dev, "failed to parse keypad params\n");
> > > + return error;
> >
> > Nit: It's pretty unusual to use this for a standard error handling
> > variable. Consider 'ret' or 'err' as a replacement.
>
> I like "error", in fact there are a lot of these in input. I use "error" for
> data that is only returned from error path and "retval" when the same
> variable is returned in both success and error paths.
If that's your preference then I'm cool with it too. Scrap my comment.
[...]
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* Re: [PATCH 1/5] drivers: input: keyboard: st-keyscan: add keyscan driver
From: Dmitry Torokhov @ 2014-03-10 15:58 UTC (permalink / raw)
To: Lee Jones
Cc: Gabriel FERNANDEZ, Rob Herring, Pawel Moll, Mark Rutland,
Ian Campbell, Kumar Gala, Rob Landley, Russell King, Grant Likely,
devicetree, linux-doc, linux-kernel, linux-arm-kernel,
linux-input, kernel, Giuseppe Condorelli
In-Reply-To: <20140310153815.GE13661@lee--X1>
On Mon, Mar 10, 2014 at 03:38:15PM +0000, Lee Jones wrote:
> > > > This patch adds ST Keyscan driver to use the keypad hw a subset
> > > > of ST boards provide. Specific board setup will be put in the
> > > > given dt.
> > > >
> > > > Signed-off-by: Giuseppe Condorelli <giuseppe.condorelli@st.com>
> > > > Signed-off-by: Gabriel Fernandez <gabriel.fernandez@st.com>
> > >
> > > Are you sure these are in the correct order?
> > >
> > > What is the history of this commit?
> > >
> > > > ---
> > > > .../devicetree/bindings/input/st-keypad.txt | 50 ++++
> > >
> > > This should be submitted as a seperate patch.
> >
> > Why do we have such requirement? To me it would make more sense to add
> > binding documentation in the same commit as the code that uses these
> > bindings.
>
> I'm inclined to agree with you and that's actually how we used to do
> it, but a decision was made by the DT guys at one of the Kernel
> Summits to submit Documentation as a separate patch.
Do you have background for this decision? To me it is akin splitting
header file into a separate patch.
--
Dmitry
^ permalink raw reply
* Re: [PATCH 05/05] input synaptics-rmi4: Add reflash support
From: Courtney Cavin @ 2014-03-10 16:24 UTC (permalink / raw)
To: Christopher Heiny
Cc: Dmitry Torokhov, Linux Input, Andrew Duggan, Vincent Huang,
Vivian Ly, Daniel Rosenberg, Linus Walleij, Benjamin Tissoires,
David Herrmann, Jiri Kosina
In-Reply-To: <1394245795-17347-5-git-send-email-cheiny@synaptics.com>
On Sat, Mar 08, 2014 at 03:29:55AM +0100, Christopher Heiny wrote:
> Add support for reflashing V5 bootloader firmwares into
> RMI4 devices.
Throughout this driver: I'm not sure of the name 'reflash'. Maybe just
'flash(ing)'?
>
> Signed-off-by: Christopher Heiny <cheiny@synaptics.com>
> Signed-off-by: Vincent Huang <vincent.huang@tw.synaptics.com>
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> Cc: Linux Walleij <linus.walleij@linaro.org>
> Cc: David Herrmann <dh.herrmann@gmail.com>
> Cc: Jiri Kosina <jkosina@suse.cz>
>
> ---
>
> drivers/input/rmi4/Kconfig | 9 +
> drivers/input/rmi4/Makefile | 1 +
> drivers/input/rmi4/rmi_bus.c | 3 +
> drivers/input/rmi4/rmi_driver.h | 11 +
> drivers/input/rmi4/rmi_fw_update.c | 961 +++++++++++++++++++++++++++++++++++++
> 5 files changed, 985 insertions(+)
>
> diff --git a/drivers/input/rmi4/Kconfig b/drivers/input/rmi4/Kconfig
> index d0c7b6e..9b88b6a 100644
> --- a/drivers/input/rmi4/Kconfig
> +++ b/drivers/input/rmi4/Kconfig
> @@ -25,6 +25,15 @@ config RMI4_DEBUG
>
> If unsure, say N.
>
> +config RMI4_FWLIB
Err. LIB?
> + bool "RMI4 Firmware Update"
> + depends on RMI4_CORE
> + help
> + Say Y here to enable in-kernel firmware update capability.
> +
> + Add support to the RMI4 core to enable reflashing of device
> + firmware.
Please provide more description here of what someone is supposed to do
to utilize this support, and what it actually does. The term "update"
here is generic enough to cause some confusion.
> +
> config RMI4_I2C
> tristate "RMI4 I2C Support"
> depends on RMI4_CORE && I2C
> diff --git a/drivers/input/rmi4/Makefile b/drivers/input/rmi4/Makefile
> index 5c6bad5..570ea80 100644
> --- a/drivers/input/rmi4/Makefile
> +++ b/drivers/input/rmi4/Makefile
> @@ -1,5 +1,6 @@
> obj-$(CONFIG_RMI4_CORE) += rmi_core.o
> rmi_core-y := rmi_bus.o rmi_driver.o rmi_f01.o
> +rmi_core-$(CONFIG_RMI4_FWLIB) += rmi_fw_update.o
Ok. Now I'm thoroughly confused, and I haven't even gotten to the code
yet. FWLIB => rmi_fw_update => rmi_reflash. What are we doing again?
>
> # Function drivers
> obj-$(CONFIG_RMI4_F11) += rmi_f11.o
> diff --git a/drivers/input/rmi4/rmi_bus.c b/drivers/input/rmi4/rmi_bus.c
> index 6e0454a..3c93d08 100644
> --- a/drivers/input/rmi4/rmi_bus.c
> +++ b/drivers/input/rmi4/rmi_bus.c
> @@ -117,6 +117,8 @@ int rmi_register_transport_device(struct rmi_transport_dev *xport)
> if (error)
> goto err_put_device;
>
> + rmi_reflash_init(rmi_dev);
> +
> dev_dbg(xport->dev, "%s: Registered %s as %s.\n", __func__,
> pdata->sensor_name, dev_name(&rmi_dev->dev));
>
> @@ -139,6 +141,7 @@ void rmi_unregister_transport_device(struct rmi_transport_dev *xport)
> struct rmi_device *rmi_dev = xport->rmi_dev;
>
> device_del(&rmi_dev->dev);
> + rmi_reflash_cleanup(rmi_dev);
> rmi_physical_teardown_debugfs(rmi_dev);
> put_device(&rmi_dev->dev);
> }
> diff --git a/drivers/input/rmi4/rmi_driver.h b/drivers/input/rmi4/rmi_driver.h
[...]
> +#ifdef CONFIG_RMI4_FWLIB
> +void rmi_reflash_init(struct rmi_device *rmi_dev);
> +void rmi_reflash_cleanup(struct rmi_device *rmi_dev);
> +#else
> +#define rmi_reflash_init(rmi_dev)
> +#define rmi_reflash_cleanup(rmi_dev)
Please use static inline functions here. It helps the compiler tell
you when you do something wrong.
> +#endif
>
> #endif
> diff --git a/drivers/input/rmi4/rmi_fw_update.c b/drivers/input/rmi4/rmi_fw_update.c
[...]
> +struct rmi_reflash_data {
> + struct rmi_device *rmi_dev;
> + bool force;
> + ulong busy;
> + char name_buf[RMI_NAME_BUFFER_SIZE];
> + char *img_name;
const?
> + struct pdt_entry f01_pdt;
> + struct f01_basic_properties f01_props;
> + u8 device_status;
> + struct pdt_entry f34_pdt;
> + u8 bootloader_id[2];
> + struct rmi_f34_queries f34_queries;
> + u16 f34_status_address;
> + struct rmi_f34_control_status f34_controls;
> + const u8 *firmware_data;
> + const u8 *config_data;
> + struct work_struct reflash_work;
> +};
[...]
> +static int rmi_read_f01_status(struct rmi_reflash_data *data)
> +{
> + int retval;
> +
> + retval = rmi_read(data->rmi_dev, data->f01_pdt.data_base_addr,
> + &data->device_status);
> + if (retval)
> + return retval;
> +
> + return 0;
> +}
This function is used one time, just calls rmi_read... There's no need
to wrap this read.
[...]
> +static int rmi_wait_for_idle(struct rmi_reflash_data *data, int timeout_ms)
> +{
> + int timeout_count = ((timeout_ms * 1000) / RMI_MAX_SLEEP_TIME_US) + 1;
> + int count = 0;
> + struct rmi_f34_control_status *controls = &data->f34_controls;
> + int retval;
> +
> + do {
> + if (count || timeout_count == 1)
> + usleep_range(RMI_MIN_SLEEP_TIME_US,
> + RMI_MAX_SLEEP_TIME_US);
> + retval = rmi_read_f34_controls(data);
> + count++;
> + if (retval)
> + continue;
> + else if (IS_IDLE(controls)) {
> + if (dev_WARN_ONCE(&data->rmi_dev->dev,
> + !data->f34_controls.program_enabled,
> + "Reflash is idle but program_enabled bit isn't set.\n"
> + ))
> + /*
> + * This works around a bug in certain device
> + * firmwares, where the idle state is reached,
> + * but the program_enabled bit is not yet set.
> + */
If it's a bug in devices, but it's ok to try again as a workaround, is
there a good reason to print a stacktrace? Does the user care?
> + continue;
> + return 0;
> + }
> + } while (count < timeout_count);
> +
> + dev_err(&data->rmi_dev->dev,
> + "ERROR: Timeout waiting for idle status.\n");
> + dev_err(&data->rmi_dev->dev, "Command: %#04x\n", controls->command);
> + dev_err(&data->rmi_dev->dev, "Status: %#04x\n", controls->status);
> + dev_err(&data->rmi_dev->dev, "Enabled: %d\n",
> + controls->program_enabled);
> + dev_err(&data->rmi_dev->dev, "Idle: %d\n", IS_IDLE(controls));
> + return -ETIMEDOUT;
> +}
[...]
> +static int rmi_write_f34_command(struct rmi_reflash_data *data, u8 command)
> +{
> + int retval;
> + struct rmi_device *rmi_dev = data->rmi_dev;
> +
> + retval = rmi_write(rmi_dev, data->f34_status_address, command);
> + if (retval < 0) {
> + dev_err(&rmi_dev->dev,
> + "Failed to write F34 command %#04x. Code: %d.\n",
> + command, retval);
> + return retval;
> + }
> +
> + return 0;
> +}
This function is used three times, and calls one function without any
wrapping magic. You could easily call rmi_write in each place.
[...]
> +static void rmi_reset_device(struct rmi_reflash_data *data)
It really feels like this should have an error return.
> +{
> + int retval;
> + const struct rmi_device_platform_data *pdata =
> + rmi_get_platform_data(data->rmi_dev);
> +
> + dev_dbg(&data->rmi_dev->dev, "Resetting...\n");
> + retval = rmi_write(data->rmi_dev, data->f01_pdt.command_base_addr,
> + RMI_F01_CMD_DEVICE_RESET);
> + if (retval < 0)
> + dev_warn(&data->rmi_dev->dev,
> + "WARNING - post-flash reset failed, code: %d.\n",
> + retval);
> + msleep(pdata->reset_delay_ms ?: RMI_F01_DEFAULT_RESET_DELAY_MS);
> + dev_dbg(&data->rmi_dev->dev, "Reset completed.\n");
> +}
> +static int rmi_write_firmware(struct rmi_reflash_data *data)
> +{
> + return rmi_write_blocks(data, (u8 *) data->firmware_data,
> + data->f34_queries.fw_block_count, RMI_F34_WRITE_FW_BLOCK);
> +}
Called once.
> +
> +static int rmi_write_configuration(struct rmi_reflash_data *data)
> +{
> + return rmi_write_blocks(data, (u8 *) data->config_data,
> + data->f34_queries.config_block_count,
> + RMI_F34_WRITE_CONFIG_BLOCK);
> +}
Called once.
> +
> +static void rmi_reflash_firmware(struct rmi_reflash_data *data)
Also feels like this should have an error return.
[...]
> +static void rmi_fw_update(struct rmi_device *rmi_dev)
Same.
> +{
[...]
> + retval = rmi_read_f34_queries(data);
> + if (retval) {
> + dev_err(&rmi_dev->dev, "F34 queries failed, code = %d.\n",
> + retval);
> + goto done;
> + }
> + if (data->img_name && strlen(data->img_name))
data->img_name && data->img_name[0] ? No need to waste extra cycles.
> + snprintf(firmware_name, RMI_NAME_BUFFER_SIZE,
> + rmi_fw_name_format, data->img_name);
> + else if (pdata->firmware_name && strlen(pdata->firmware_name))
Same.
> + snprintf(firmware_name, RMI_NAME_BUFFER_SIZE,
> + rmi_fw_name_format, pdata->firmware_name);
> + else {
> + if (!strlen(data->f01_props.product_id)) {
Same.
> + dev_err(&rmi_dev->dev, "Product ID is missing or empty - will not reflash.\n");
> + goto done;
> + }
> + snprintf(firmware_name, RMI_NAME_BUFFER_SIZE,
> + rmi_fw_name_format, data->f01_props.product_id);
> + }
[...]
> + if (rmi_go_nogo(data, header)) {
> + dev_dbg(&rmi_dev->dev, "Go/NoGo said go.\n");
> + rmi_free_function_list(rmi_dev);
> + rmi_reflash_firmware(data);
> + rmi_reset_device(data);
> + rmi_driver_detect_functions(rmi_dev);
> + } else
> + dev_dbg(&rmi_dev->dev, "Go/NoGo said don't reflash.\n");
Documentation/CodingStyle says:
} else {
...
}
[...]
> +}
[...]
> +static ssize_t rmi_reflash_force_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct rmi_device *rmi_dev = to_rmi_device(dev);
> + struct rmi_driver_data *drv_data = dev_get_drvdata(&rmi_dev->dev);
> + struct rmi_reflash_data *data = drv_data->reflash_data;
> + int retval;
> + unsigned long val;
> +
> + if (test_and_set_bit(0, &data->busy))
> + return -EBUSY;
> +
> + retval = kstrtoul(buf, 10, &val);
> + if (retval)
> + count = retval;
> + else
> + data->force = !!val;
Hrm. Perhaps '42' doesn't make sense here. Maybe add:
else if (val > 1)
count = -EINVAL;
> +
> + clear_bit(0, &data->busy);
> +
> + return count;
> +}
> +
> +static ssize_t rmi_reflash_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct rmi_device *rmi_dev = to_rmi_device(dev);
> + struct rmi_driver_data *drv_data = dev_get_drvdata(&rmi_dev->dev);
> + struct rmi_reflash_data *data = drv_data->reflash_data;
> +
> + return snprintf(buf, PAGE_SIZE, "%u\n", test_bit(0, &data->busy));
> +}
> +
> +static ssize_t rmi_reflash_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + int retval;
> + unsigned long val;
> + struct rmi_device *rmi_dev = to_rmi_device(dev);
> + struct rmi_driver_data *drv_data = dev_get_drvdata(&rmi_dev->dev);
> + struct rmi_reflash_data *data = drv_data->reflash_data;
> +
> + retval = kstrtoul(buf, 10, &val);
> + if (retval)
> + return retval;
> +
> + if (test_and_set_bit(0, &data->busy))
> + return -EBUSY;
> +
> + if (val)
> + /*
> + * TODO: Here we start a work thread to go do the reflash, but
> + * maybe we can just use request_firmware_timeout().
> + */
Mmm.. Yes. It would make the lifecycle of this busy bit much more
obvious. Otherwise perhaps call request_firmware_nowait() ? It already
does this work queue ... uh ... work for you.
> + schedule_work(&data->reflash_work);
> + else
> + clear_bit(0, &data->busy);
> +
> + return count;
> +}
[...]
> +void rmi_reflash_init(struct rmi_device *rmi_dev)
> +{
> + int error;
> + struct rmi_driver_data *drv_data = dev_get_drvdata(&rmi_dev->dev);
> + struct rmi_reflash_data *data;
> +
> + dev_dbg(&rmi_dev->dev, "%s called.\n", __func__);
> +
> + data = devm_kzalloc(&rmi_dev->dev, sizeof(struct rmi_reflash_data),
> + GFP_KERNEL);
The memory ownership here is odd. Maybe kzalloc, and just return that
data?
> +
> + error = sysfs_create_group(&rmi_dev->dev.kobj, &rmi_reflash_attributes);
> + if (error) {
> + dev_warn(&rmi_dev->dev, "Failed to create reflash sysfs attributes.\n");
> + return;
> + }
> +
> + INIT_WORK(&data->reflash_work, rmi_reflash_work);
> + data->rmi_dev = rmi_dev;
> + drv_data->reflash_data = data;
> +}
> +
> +void rmi_reflash_cleanup(struct rmi_device *rmi_dev)
> +{
> + struct rmi_driver_data *drv_data = dev_get_drvdata(&rmi_dev->dev);
> + struct rmi_reflash_data *data = drv_data->reflash_data;
> +
> + sysfs_remove_group(&rmi_dev->dev.kobj, &rmi_reflash_attributes);
> + devm_kfree(&rmi_dev->dev, data);
Who owns this memory again? devm_ doesn't seem right for this use-case.
> +}
-Courtney
^ 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