Re: High CPU Usage - bad output on hp6xx keyboard
Hi Kristoffer, On Tue, May 13, 2008 at 10:53:56PM +0200, Kristoffer Ericson wrote: Hey Dmitry Paul Want your input on best way to handle this. As you know the HP 6xx keyboard is using the poll interrupt. I believe my current issue is due to the fact that it uses a fixed time (50ms) to check the keyboard for new data. At high cpu usage, it doesn't complete the routine in 50ms, so it starts to pile up and outputs bad chars (like 5). In the old driver it instead used a timer that at end of function setup to run within one HZ, that enabled only one routine to be active at one time. I could use spinlocks to force only one active, but Im somewhat concerned at what cost this would imply. I do not know how many requests could pile up and what this would mean further down the line. Ive increased the poll to 100ms-200ms and the problem occured less and less. However it also means that the keyboard is slower that usual. Instead with timer, it will essentially get it done when it can and dont schedule other stuff until its ready. It is the same with the input-polldev. The polling is done form a work that is submitted into ipolldevd workqueue that resubmits itself. There is only one work active or pending for every polled input device. What you are seeing is most likely input autorepeat kicking in if the polling thread is not scheduled fast enough, although the CPU should be pretty busy since default repeat delay is 250 ms. -- Dmitry
Re: [HP6xx/Patch] - update touchscreen driver
Hi Kristoffer,
On Sun, Apr 27, 2008 at 04:49:44PM +0200, Kristoffer Ericson wrote:
-
-static struct input_dev *hp680_ts_dev;
+struct input_dev *dev;
Why does it need to be global? dev a a name is ratehr non-descriptive
too given that we are dealing with input devices, platform devices and
other kinds of devices.
+static void do_softint(struct delayed_work *work);
static DECLARE_DELAYED_WORK(work, do_softint);
-static void do_softint(struct work_struct *work)
+static void do_softint(struct delayed_work *work)
{
int absx = 0, absy = 0;
u8 scpdr;
@@ -53,77 +56,104 @@ static void do_softint(struct work_struct *work)
touched = ctrl_inb(PHDR) PHDR_TS_PEN_DOWN;
}
+
+ /* note, tslib currently expects ABS_PRESSURE also */
But that will change in the future... I dont see the need for such
comment.
if (touched) {
- input_report_key(hp680_ts_dev, BTN_TOUCH, 1);
- input_report_abs(hp680_ts_dev, ABS_X, absx);
- input_report_abs(hp680_ts_dev, ABS_Y, absy);
+ input_report_abs(dev, ABS_X, absx);
+ input_report_abs(dev, ABS_Y, absy);
+ input_report_key(dev, BTN_TOUCH, 1);
} else {
- input_report_key(hp680_ts_dev, BTN_TOUCH, 0);
+ input_report_key(dev, BTN_TOUCH, 0);
}
-
- input_sync(hp680_ts_dev);
+ input_sync(dev);
enable_irq(HP680_TS_IRQ);
}
-static irqreturn_t hp680_ts_interrupt(int irq, void *dev)
+static irqreturn_t hp680_ts_interrupt(int irq, void *pdev)
{
disable_irq_nosync(irq);
schedule_delayed_work(work, HZ / 20);
-
return IRQ_HANDLED;
}
-static int __init hp680_ts_init(void)
+static int __devinit jornada680_ts_probe(struct platform_device *pdev)
{
- int err;
+ int error;
- hp680_ts_dev = input_allocate_device();
- if (!hp680_ts_dev)
- return -ENOMEM;
+ dev = input_allocate_device();
- hp680_ts_dev-evbit[0] = BIT_MASK(EV_ABS) | BIT_MASK(EV_KEY);
- hp680_ts_dev-keybit[BIT_WORD(BTN_TOUCH)] = BIT_MASK(BTN_TOUCH);
+ if (!dev) {
+ printk(KERN_INFO ts: failed to allocate device\n);
Just ts:? driver name: is usually more informative.
+ error = -ENODEV;
-ENOMEM
+ return error;
+ }
- input_set_abs_params(hp680_ts_dev, ABS_X,
- HP680_TS_ABS_X_MIN, HP680_TS_ABS_X_MAX, 0, 0);
- input_set_abs_params(hp680_ts_dev, ABS_Y,
- HP680_TS_ABS_Y_MIN, HP680_TS_ABS_Y_MAX, 0, 0);
+ dev-name = HP Jornada 6xx Touchscreen;
+ dev-phys = jornadats/input0;
jornada_ts/input0
+ dev-id.bustype = BUS_HOST;
+ dev-dev.parent = pdev-dev;
+ dev-evbit[0] = BIT_MASK(EV_ABS) | BIT_MASK(EV_KEY);
+ dev-absbit[0] = BIT_MASK(ABS_X) | BIT_MASK(ABS_Y);
+ dev-keybit[BIT_WORD(BTN_TOUCH)] = BIT_MASK(BTN_TOUCH);
+ input_set_abs_params(dev, ABS_X, 40, 950, 0, 0);
+ input_set_abs_params(dev, ABS_Y, 80, 910, 0, 0);
+
+ error = input_register_device(dev);
+ if (error) {
+ printk(KERN_ERR ts: failed to register device\n);
+ goto fail2;
+ }
- hp680_ts_dev-name = HP Jornada touchscreen;
- hp680_ts_dev-phys = hp680_ts/input0;
+ error = request_irq(HP680_TS_IRQ, hp680_ts_interrupt,
+ IRQF_DISABLED, HP6xx Touchscreen driver, NULL);
- if (request_irq(HP680_TS_IRQ, hp680_ts_interrupt,
- IRQF_DISABLED, MODNAME, 0) 0) {
- printk(KERN_ERR hp680_touchscreen.c: Can't allocate irq %d\n,
-HP680_TS_IRQ);
- err = -EBUSY;
- goto fail1;
+ if (error) {
+ printk(KERN_INFO ts: unable to aquire irq\n);
acquire.
+ goto fail3;
}
- err = input_register_device(hp680_ts_dev);
- if (err)
- goto fail2;
-
return 0;
- fail2: free_irq(HP680_TS_IRQ, NULL);
- cancel_delayed_work(work);
- flush_scheduled_work();
- fail1: input_free_device(hp680_ts_dev);
- return err;
+fail3:
+ input_unregister_device(dev);
+fail2:
+ input_free_device(dev);
Dont ever call input_free_device() after calling
input_unregister_device(). The old flow was correct.
+ return error;
}
-static void __exit hp680_ts_exit(void)
+static int __devexit jornada680_ts_remove(struct platform_device *pdev)
{
- free_irq(HP680_TS_IRQ, NULL);
cancel_delayed_work(work);
flush_scheduled_work();
- input_unregister_device(hp680_ts_dev);
+ free_irq(HP680_TS_IRQ, pdev);
You have to free IRQ first. Or disable it. Otherwise you may get
another work scheduled after you flush it.
+ input_unregister_device(dev);
+ return 0;
+}
+
+/* work with hotplug and coldplug */
+MODULE_ALIAS(platform:jornada_ts);
+
+static struct platform_driver jornada680_ts_driver = {
+
Re: [patch 2.6.25-rc8] omap-keypad: fix build warning
On Fri, Apr 11, 2008 at 01:09:18AM -0700, David Brownell wrote: From: David Brownell [EMAIL PROTECTED] Build fixes: drivers/input/keyboard/omap-keypad.c: In function 'omap_kp_probe': drivers/input/keyboard/omap-keypad.c:418: warning: 'row_idx' is used uninitialized in this function drivers/input/keyboard/omap-keypad.c:421: warning: 'col_idx' is used uninitialized in this function These variables are useful when cpu_is_omap24xx(), and otherwise just for useless cleanup. Signed-off-by: David Brownell [EMAIL PROTECTED] Signed-off-by: Tony Lindgren [EMAIL PROTECTED] Applied, thank you David. -- Dmitry
Re: [RESEND patch 2.6.25-rc8] gpio_keys irq handling
On Fri, Apr 11, 2008 at 12:35:55AM -0700, David Brownell wrote: From: David Brownell [EMAIL PROTECTED] Cleanup IRQ handling in gpio_keys: bail after handling the IRQ, and report IRQ_NONE if we never handle it. Signed-off-by: David Brownell [EMAIL PROTECTED] Cc: Phil Blundell [EMAIL PROTECTED] Cc: Dmitry Torokhov [EMAIL PROTECTED] Applied, thank you David. -- Dmitry
Re: [RESEND patch 2.6.25-rc8] ads7846 vref support (ads7843)
On Fri, Apr 11, 2008 at 01:15:25AM -0700, David Brownell wrote: On Friday 11 April 2008, David Brownell wrote: Updated since the version sent 5-January (evidently bounced from list archives) ... to remove the warning fix that was merged (from someone else) on 10-March. Could you please check the following commit in my tree and make sure it is the right one: http://git.kernel.org/?p=linux/kernel/git/dtor/input.git;a=commit;h=7c6d0ee14cb7a4cfad4864dc196256da5749bc0c MAINTAINERS says [EMAIL PROTECTED] but the only archives I could find seems to say @atrey.karlin.mff.cuni.cz... I eventually found an @vger archive, but it seems to have no postings since February (unlike the @atrey one). Those two lost patches certainly made it to that archive: http://www.mail-archive.com/[EMAIL PROTECTED]/msg00086.html http://www.mail-archive.com/[EMAIL PROTECTED]/msg00103.html It seems maybe the MAINTAINERS entry may be incorrect ... @vger is the correct only, we are migrating from @atrey due to heavy spam issues. -- Dmitry
Re: [FIX / HP7xx] - Fix touchscreen to work without tsdev
[Picking up old thread...] On Fri, Mar 21, 2008 at 10:43:14PM +, Richard Purdie wrote: Later on in the code, BTN_TOUCH should do that same job but that isn't looked for in the initial check_fd() code. tslib can be easily fixed to address that issue and if nobody else writes a patch I'll aim to do so although it might not be for a few days. Having said that I don't see why exporting ABS_PRESSURE of 0 and 1 isn't a valid thing to do, they are pressure readings, just of a rather limited range! Because it does not convey any new data (BTN_TOUCH already indicates that the screen is being touched) and so the kernel and userspace have to spin wheels passing and processing/discarding useless events. -- Dmitry
Re: [FIX / HP7xx] - Fix touchscreen to work without tsdev
Hi Kristoffer,
On Fri, Mar 21, 2008 at 08:40:19PM -0700, Kristoffer Ericson wrote:
This patch makes the jornada 7xx touchscreen work without tsdev. It changes
the ABS values and makes sure the report_key is done in right order (for
tslib).
We also change the printk's to use ERR instead of INFO.
I dont think that this patch should be applied. The hardware does not
report pressure and thus the driver should not generate pressure
events. Order of events should not matter either, thet should all be
accumulated until userspace gets EV_SYN/SYN_REPORT message and then
processed all together.
It sounds like tslib is pretty broken in that regard. Should we bring
tsdev back till the issues are resolved? Richard?
Signed-off-by: Kristoffer Ericson [EMAIL PROTECTED]
diff --git a/drivers/input/touchscreen/jornada720_ts.c
b/drivers/input/touchscreen/jornada720_ts.c
index 42a1c9a..5d68fad 100644
--- a/drivers/input/touchscreen/jornada720_ts.c
+++ b/drivers/input/touchscreen/jornada720_ts.c
@@ -1,6 +1,8 @@
/*
* drivers/input/touchscreen/jornada720_ts.c
*
+ * HP Jornada 710/720/728 Touchscreen Driver
+ *
* Copyright (C) 2007 Kristoffer Ericson [EMAIL PROTECTED]
*
* Copyright (C) 2006 Filip Zyzniewski [EMAIL PROTECTED]
@@ -10,7 +12,6 @@
* it under the terms of the GNU General Public License version 2 as
* published by the Free Software Foundation.
*
- * HP Jornada 710/720/729 Touchscreen Driver
*/
#include linux/platform_device.h
@@ -72,6 +73,7 @@ static irqreturn_t jornada720_ts_interrupt(int irq, void
*dev_id)
/* If GPIO_GPIO9 is set to high then report pen up */
if (GPLR GPIO_GPIO(9)) {
+ input_report_abs(input, ABS_PRESSURE, 0);
input_report_key(input, BTN_TOUCH, 0);
input_sync(input);
} else {
@@ -84,12 +86,12 @@ static irqreturn_t jornada720_ts_interrupt(int irq, void
*dev_id)
x = jornada720_ts_average(jornada_ts-x_data);
y = jornada720_ts_average(jornada_ts-y_data);
- input_report_key(input, BTN_TOUCH, 1);
input_report_abs(input, ABS_X, x);
input_report_abs(input, ABS_Y, y);
+ input_report_abs(input, ABS_PRESSURE, 1);
+ input_report_key(input, BTN_TOUCH, 1);
input_sync(input);
}
-
jornada_ssp_end();
}
@@ -118,24 +120,27 @@ static int __devinit jornada720_ts_probe(struct
platform_device *pdev)
input_dev-phys = jornadats/input0;
input_dev-id.bustype = BUS_HOST;
input_dev-dev.parent = pdev-dev;
+ input_dev-evbit[0] = BIT_MASK(EV_KEY) | BIT_MASK(EV_ABS);
+ input_dev-absbit[0] = BIT_MASK(ABS_X) | BIT_MASK(ABS_Y) |
BIT_MASK(ABS_PRESSURE);
+ input_dev-keybit[BIT_WORD(BTN_TOUCH)] = BIT_MASK(BTN_TOUCH);
- input_dev-evbit[0] = BIT(EV_KEY) | BIT(EV_ABS);
- input_dev-keybit[LONG(BTN_TOUCH)] = BIT(BTN_TOUCH);
- input_set_abs_params(input_dev, ABS_X, 270, 3900, 0, 0);
- input_set_abs_params(input_dev, ABS_Y, 180, 3700, 0, 0);
+ input_set_abs_params(input_dev, ABS_X, 79, 995, 0, 0);
+ input_set_abs_params(input_dev, ABS_Y, 75, 910, 0, 0);
error = request_irq(IRQ_GPIO9,
jornada720_ts_interrupt,
IRQF_DISABLED | IRQF_TRIGGER_RISING,
HP7XX Touchscreen driver, pdev);
if (error) {
- printk(KERN_INFO HP7XX TS : Unable to acquire irq!\n);
+ printk(KERN_ERR ts : unable to acquire irq!\n);
goto fail1;
}
error = input_register_device(jornada_ts-dev);
- if (error)
+ if (error) {
+ printk(KERN_ERR ts : unable to register device\n);
goto fail2;
+ }
return 0;
--
Kristoffer Ericson [EMAIL PROTECTED]
--
Dmitry
Re: [HP j6xx / FIX] - Fix so touchscreen driver works again
On Thu, Mar 06, 2008 at 10:51:47PM +0100, Jiri Kosina wrote: On Thu, 6 Mar 2008, Kristoffer Ericson wrote: The general replies I've gotten when asking to change filenames is that they don't want to break back-portability or whatever. To tell you the truth im not fully in the clear what they ment. Perhaps that it would be harder to track changes if the filename changes, or that if changing names was more allowed it could cause many people to start changing stuff around. git can track renames, so that shouldn't be a hard issue. Dmitry will certainly have a better idea. The idea there are 2 kind of users. First kind used udev and other standard facilities for module loading and this does not care about module name. FOr such user there is no benefit in renaming the module. The second kind of user (and I am not talking about the ned user) rolls out his own startup mechanisms, potentially with statically defined module names. Here rename will actually hurt. Thats why I am usually against renames unless there is bigger benefit. Thanks. -- Dmitry
Re: Question regarding input phys
On Thu, Feb 07, 2008 at 11:05:04PM +0100, Kristoffer Ericson wrote: On Thu, 7 Feb 2008 16:50:55 -0500 Dmitry Torokhov [EMAIL PROTECTED] wrote: Hi Kristoffer, On Thu, Feb 07, 2008 at 10:33:34PM +0100, Kristoffer Ericson wrote: Dmitry, (or anyone else) in what way is the phys = blablabla/input0 important aside from placement in /sys? Im asking because all my keyboard/touchscreen end with input0 and wondering if I need to set one to /input1 atleast. It is supposed to describe where physically the device is. So if you have a signle device sonnected to a controller you would alwaay use it. But if you have, for exabple, USB keyboard with embedded touchpad and USB presents it as 2 separate interfaces you will get input0 and input1. Yeah I think I get it. So seperate devices can all use input0 without any issues. Also another quick question, where should the device numbers exist? I mean inside /sys. I seem to need to create those devfiles myself as udev don't pick them up. The input device itself does not have a device number, only interfaces attached to it (evdev, mousedev, etc) have dev attribute. -- Dmitry
Re: [PATCH 1/6] Core driver for WM97xx touchscreens
Hi Mark, On Sat, Jan 26, 2008 at 05:28:31PM +, Mark Brown wrote: + + /* register our battery device */ + wm-battery_dev = platform_device_alloc(wm97xx-battery, 0); + if (!wm-battery_dev) + goto batt_err; ... + batt_err: + input_unregister_device(wm-input_dev); + kfree(wm); + return ret; +} The probe error handling is not quite correct. When we reach the fragment above ret variable is 0 so if platfrom_device_alloc() fails we will return 0 and the device will be considered bound but in half-dead state. Please make sure that proper error is returned in all cases. Also please do not mix out of line and in-line error unwinding (input_free_device() should be called in the error path and if you are concerned about double-free after input_unregister_device() just set wm-input_dev to NULL there). Thanks. -- Dmitry
Re: Question regarding input phys
Hi Kristoffer, On Thu, Feb 07, 2008 at 10:33:34PM +0100, Kristoffer Ericson wrote: Dmitry, (or anyone else) in what way is the phys = blablabla/input0 important aside from placement in /sys? Im asking because all my keyboard/touchscreen end with input0 and wondering if I need to set one to /input1 atleast. It is supposed to describe where physically the device is. So if you have a signle device sonnected to a controller you would alwaay use it. But if you have, for exabple, USB keyboard with embedded touchpad and USB presents it as 2 separate interfaces you will get input0 and input1. Hope this helps. -- Dmitry
Re: [PATCH] Core driver for WM97xx touchscreens
Hi Mark,
On Fri, Jan 18, 2008 at 04:27:06PM +, Mark Brown wrote:
This patch series adds support for the touchscreen controllers provided
by Wolfson Microelectronics WM97xx series chips in both polled and
streaming modes.
Thank you for the patches. Some comments below.
+static int wm97xx_ts_input_open(struct input_dev *idev)
+{
+ struct wm97xx *wm = (struct wm97xx *)input_get_drvdata(idev);
No need to cast.
+
+ mutex_lock(wm-ts_mutex);
+ /* first time opened ? */
+ if (wm-ts_use_count++ == 0) {
You do not need to implement a counter here. Input core makes sure
that open and close are only called for the fist and last user
respectivly. ts_mutex can go away as well.
+ /* set up touch configuration */
+ wm-input_dev-name = wm97xx touchscreen;
+ wm-input_dev-open = wm97xx_ts_input_open;
+ wm-input_dev-close = wm97xx_ts_input_close;
+ set_bit(EV_ABS, wm-input_dev-evbit);
+ set_bit(ABS_X, wm-input_dev-absbit);
+ set_bit(ABS_Y, wm-input_dev-absbit);
+ set_bit(ABS_PRESSURE, wm-input_dev-absbit);
+ wm-input_dev-absmax[ABS_X] = abs_x[1];
+ wm-input_dev-absmax[ABS_Y] = abs_y[1];
+ wm-input_dev-absmax[ABS_PRESSURE] = abs_p[1];
+ wm-input_dev-absmin[ABS_X] = abs_x[0];
+ wm-input_dev-absmin[ABS_Y] = abs_y[0];
+ wm-input_dev-absmin[ABS_PRESSURE] = abs_p[0];
+ wm-input_dev-absfuzz[ABS_X] = abs_x[2];
+ wm-input_dev-absfuzz[ABS_Y] = abs_y[2];
+ wm-input_dev-absfuzz[ABS_PRESSURE] = abs_p[2];
input_set-abs_params(wm-input_dev, ABS_X, ...);
input_set-abs_params(wm-input_dev, ABS_Y, ...);
input_set-abs_params(wm-input_dev, ABS_X, ...);
I'd also recoomend using a temp for wm-input_dev, shoudl save a couple
of bytes.
+ input_set_drvdata(wm-input_dev, wm);
Also wm-input_dev-dev.parent = dev; to put it in the proper place
in sysfs hierarchy.
+ ret = input_register_device(wm-input_dev);
+ if (ret 0) {
Add input_free_device(); here.
+ kfree(wm);
+ return -ENOMEM;
+ }
I will need some more time to review and understand the need for the new bus
in the driver.
Thanks.
--
Dmitry
Re: {Jornada7xx} patches (keyboard fix driver Kconfig cleanup)
Hi Kristoffer, On Jan 13, 2008 6:05 AM, Kristoffer Ericson [EMAIL PROTECTED] wrote: Greetings Dmitry, Seems like I've been sending to bad email adress. Was wondering of the status of my patches sent about a month ago (jornada keyboard). I've attached them just in case. The first one has been accepted into Andrew's tree. No, you have been sending the patches to the correct addresses, unfortunately I was rather busy the last couple of months and unable to spend pretty much any time on kernel. I expect that I should have more time by the end of the month and so will be more responsive. I apologize for keeping silence for so long. The first patch was apllied to my tree for quite some time, and I just applied the second one a nd sent a pull request to Linus. However in the second path I removed changes to config symbols, leaving only changes to the labels and help texts. I do not htink that we want gratuosly rename existing config symbols and break existing configs. The names are not visible to users anyway. -- Dmitry
Re: [PATCH] Pass EV_PWR events to event handlers
On Jan 1, 2008 7:22 PM, Richard Purdie [EMAIL PROTECTED] wrote: input_handle_event() used to pass EV_PWR events to event handlers but no longer does so in 2.6.23. Modules to trigger power management events based on input power events exist but rely on the EV_PWR events being passed to the input event handlers. This patch makes these events available again. Applied to my tree - for-linus branch. Thank you Richard. -- Dmitry
Re: [PATCH] Fix spitzkbd suspend key handling
On Jan 1, 2008 7:17 PM, Richard Purdie [EMAIL PROTECTED] wrote: The spitz keyboard driver reports KEY_SUSPEND events but doesn't register its use of this event in the keybit bitfield, breaking input events for this key. This patch fixes that by registering the key in the keybit bitfield. Applied to my tree - for-linus branch. Thank you Richard. -- Dmitry P.S. Try to migrate to [EMAIL PROTECTED], I'd like to get off atrey server
Re: keyfuzz... KEY_FN and keyboard with no Insert key
Hi Federico, On Jan 1, 2008 5:50 PM, Federico Ferri [EMAIL PROTECTED] wrote: again it's me dealing with Apple Keyboards... I got the latest [1], which unfortunately lacks an Insert key (yes, I could type 'i' instead of hitting Intert inside vim, but I'd like to get my Ins key back... you know...) till today I used keyfuzz to remap KEY_F13 to the (missing since ever!) KEY_SYSRQ, this way: echo 0x00070068 99 | keyfuzz -s -d/dev/input/event1 today I spent hours on trying to understand how the above worked. what I learned today is: * usage code is split into PAGE and the CODE itself... so, I see my usage code is 0x68 of page 7 RIght. (but I didn't find in the hid code where/what are those pages) http://www.usb.org/developers/devclass_docs/Hut1_12.pdf * usage code is mapped thru the hid_keyboard[256] array at drivers/hid/hid-input.c:44 so I believed now I know how to find more usage codes, but... surprise! KEY_FN which is 0x1d0 (464) doesn't fit at all into that table now here I don't really understand how things work (and this is not a powerbook!). do you think it is possible to remap that FN key to the more appropriate (at least for its location*) INS key [with keyfuzz]? can you give me the usage code? please, more generally, teach me how to find those usages in such situations.. I would really appreciate! *: would otherwise be reasonable to fix this stuff in hid-input or whatever, hard-replacing that key? No. It is only your preference to have FN map to INS. a FN key which does not act like a modifier** is totally useless for me, while missing the INS key it is a big handicap. **: on Macs, the FN key combined with the special keys should output one of: 113 (Mute), 114 (VolumeDown), 115 (VolumeUp), 161 (EjectCD), 163 (NextSong), 164 (PlayPause), 165 (PreviousSong). [[this does not happen on linux, or at least I can't hit those keycodes - tested in evtest- using FN+F##... so my guess that FN doesn't act as a modifier]] those keycode should came out of the second input devices (yes, the apple USB keyboard is seen as two distinct event devices, one for keyboard, one for special keys) You need to lok at hid_pb_fnmode option of the HID driver. It has to do with enabling aforemention FN translation on powerbooks. It only works if the device is marked with HID_QUIRK_POWERBOOK_HAS_FN so please check the VID/PID of your device. Since you mentioned that it is not a powerbook you may have to add that quirk dynamically via sysfs. -- Dmitry
Re: [PATCH] Fujitsu application panel driver
Hi Stephen,
On Oct 23, 2007 2:55 PM, Stephen Hemminger
[EMAIL PROTECTED] wrote:
+static void mail_led_set(struct led_classdev *led,
+enum led_brightness value)
+{
+ struct apanel *ap = container_of(led, struct apanel, mail_led);
+ if (value)
+ ap-led_bits |= 0x8000;
+ else
+ ap-led_bits = ~0x8000;
+ ap-led_bits = value;
+ schedule_work(ap-led_work);
+}
I was just about to apply the driver (and I folded in the other 4
patches you sent to me) but then I noticed the code above. It looks
like the conditional does not have any effect, ap-led_bits will be
overwritten with the raw value anyway. Please advise.
Thank you.
--
Dmitry
Re: [PATCH] USB IDs for KBGear Pablo tablet for kbtab driver
Hi John, On Dec 6, 2007 7:44 AM, John Pham [EMAIL PROTECTED] wrote: Ignore the above, looks like my xserver was reading off /dev/input/mice - the kernel kbtab driver appears to allow the tablet to emulate a mouse, but doesn't really work w/ pressure sensitivity, With kb_pressure_click=-1 module paramenet kbtab driver will send ABS_PRESSURE events to the userspace instead of converting them to BTN_LEFT. -- Dmitry
Re: [PATCH 1/1] [INPUT/KEYPAD] Blackfin BF54x keypad driver: keypad does not exist on BF544 parts
On Friday 23 November 2007, Bryan Wu wrote: From: Mike Frysinger [EMAIL PROTECTED] Signed-off-by: Mike Frysinger [EMAIL PROTECTED] Signed-off-by: Bryan Wu [EMAIL PROTECTED] Applied, thank you Mike Bryan. -- Dmitry
Re: [PATCH] ACPI: Put button input devices in correct place in sysfs
On 11/11/07, Andrey Borzenkov [EMAIL PROTECTED] wrote: On Tuesday 06 November 2007, Dmitry Torokhov wrote: Hi Andrey, On Nov 6, 2007 12:51 PM, Andrey Borzenkov [EMAIL PROTECTED] wrote: Properly set up parent on input device registered by the button driver. Seems to be a popular topic today :) + input-cdev.dev = device-dev; Please don't use cdev, but rather input_dev-dev.parent. cdev is going away soon. I sent a patch a couple of days ago to teh acpi list... You mean button patch? I could find only video one. Right, I got confused between the two... Just in case, here is updated version. Please forward to Len Brown with Acked-by: Dmitry Torokhov [EMAIL PROTECTED] Thanks! -- Dmitry
Re: [PATCH] Fix incorrect usage of strncpy and strncat in i8042_pnp_kbd_probe(); drivers/input/serio/i8042-x86ia64io.h
Hi Roel, On Nov 5, 2007 3:24 PM, Roel Kluin [EMAIL PROTECTED] wrote: See http://www.gratisoft.us/todd/papers/strlcpy.html -- Fix incorrect length argument for strncpy and strncat by replacing them with What is incorrect about the argument? respectively strlcpy and strlcat I will gladly take a patch replacing strncat with strlcat but don't error out if the buffer is too small - it is oK if PNP name is printed truncated, it does not affect KBC operations. Thanks. -- Dmitry
[PATCH] HID: Don't access input_dev-private directly
Hi Jiri,
I'd like for the patch below to go to Linus rather sooner than
later because I want to commit patch removing input_dev-private...
..Or I could push it through my tree if you give me your Acked-by.
--
Dmitry
Subject: HID: Don't access input_dev-private directly
From: Dmitry Torokhov [EMAIL PROTECTED]
input_{get|set}_drvdata() helpers should be used instead.
Signed-off-by: Dmitry Torokhov [EMAIL PROTECTED]
---
drivers/hid/hid-input.c |4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
Index: linux/drivers/hid/hid-input.c
===
--- linux.orig/drivers/hid/hid-input.c
+++ linux/drivers/hid/hid-input.c
@@ -297,7 +297,7 @@ static struct hid_usage *hidinput_find_k
static int hidinput_getkeycode(struct input_dev *dev, int scancode,
int *keycode)
{
- struct hid_device *hid = dev-private;
+ struct hid_device *hid = input_get_drvdata(dev);
struct hid_usage *usage;
usage = hidinput_find_key(hid, scancode, 0);
@@ -311,7 +311,7 @@ static int hidinput_getkeycode(struct in
static int hidinput_setkeycode(struct input_dev *dev, int scancode,
int keycode)
{
- struct hid_device *hid = dev-private;
+ struct hid_device *hid = input_get_drvdata(dev);
struct hid_usage *usage;
int old_keycode;
Re: [RFC: 2.6 patch] remove the USB_STORAGE_ONETOUCH driver
On Sunday 28 October 2007 16:39, Jiri Kosina wrote: On Sun, 28 Oct 2007, Torsten Kaiser wrote: But it looks like that uses another driver: hiddev0hidraw1: USB HID v1.10 Device [Western Digital External HDD] on usb-:00:02.1-5.2 Otherwise I would be willing to try to test this, if someone would tell me how to check that the second commit did fix the suspend problem. If you need to unbind usbhid driver from the device and bind another one, you can use the 'unbind' and 'bind' files in sysfs. As soon as the driver you are willing to test is bound to the device, you can go ahead with testing any functionality you wish (probably suspend/resume cycle is needed here?). If the device is claimed by usbhid driver (because its descriptor probably states that it's HID-compliant device) and should be claimed by another driver, it's necessary to add it to usbhid blacklist -- just let me know. Maybe onetouch driver is not needed after all. I wonder what key/button does HID deriver reports when it binds to it... Torsten, could you please post your /proc/bus/input/devices? -- Dmitry
Re: [PATCH] appletouch: add myself as maintainer
On Wednesday 24 October 2007 07:46, Johannes Berg wrote: After the last patch that broke appletouch for powerbooks again (many more have previously been proposed), I'd like to take over maintainership of this driver to make sure it doesn't break again in the future. Signed-off-by: Johannes Berg [EMAIL PROTECTED] Applied. -- Dmitry
Re: [PATCH] Fujitsu application panel driver
Hi Stephen,
On Tuesday 23 October 2007 15:55, Stephen Hemminger wrote:
+
+static int apanel_setkeycode(struct input_dev *idev, int scancode, int
keycode)
+{
+ struct apanel *ap = idev-private;
+
+ if (keycode 0 || keycode KEY_MAX)
+ return -EINVAL;
+
+ if (scancode 0 || scancode = MAX_PANEL_KEYS)
+ return -EINVAL;
scancode = idev-keycodemax is prbably better here - we don't want to
allow setting keycode for unsupported buttons.
+
+ clear_bit(ap-keymap[scancode], idev-keybit);
This will not work if one has same code assigned to 2 buttons. Pretty
degenerate case, I know...
+ ap-keymap[scancode] = keycode;
+ set_bit(keycode, idev-keybit);
+ return 0;
+}
--
Dmitry
Re: [PATCH] Input: Support for a less exclusive grab.
On 10/26/07, Zephaniah E. Hull [EMAIL PROTECTED] wrote: On Thu, Oct 25, 2007 at 01:37:34AM -0400, Ryan Lortie wrote: On Wed, 2007-24-10 at 11:35 -0400, Zephaniah E. Hull wrote: We need a way to, at the absolute minimum, unbind the keyboard from the text console. The current solution sucks for things like rfkill. I'm not convinced that Ryan's fix is any better, but just saying that X should open the console and ignore the characters is simply not an option as far as I am concerned for X. Can you think of any other way to separate things like rfkill/evdev from things like the text console that's less hacky than my 'priority' scheme? What we really want to give is exclusitivity verses other 'end users', as opposed the 'filters'. I'm defining an 'end user' to be a handler that cares about all the events from a device and plans on doing something with it. That would be the console layer for keyboards, /dev/input/mice and /dev/input/mousen for mice, X for both of those, etc. A 'filter' cares about a key or two, and might even want to remove it from the stream, rfkill is a good example. No, rfkill does not want to remove anything from the event stream. It perfectly happy with other users seeing the same events and doing their own things with them. Now, how do we design for that? Not a clue right now, still thinking about it really. I really think that it should be solved by applications themselves. Applications should only open devices they are interested in and only process events they are interested in. The rest should be simply ignored by the application. This is the only sane way. Otherwise we will need to split applications into first and second class citizens and have to manage dependencies between them. The only exception is console because both kerenl and X are fighting over control of VT switching. As a workaround one could open console in raw mode. If this feels too dirty I guess we could have ioctl for taking over console for applications willing to do so. The ioctl would disable VT switching and touching LEDs by the console driver. -- Dmitry
Re: [PATCH] input - Add Euro and Dollar keys
On Thursday 25 October 2007, Carlos Corbacho wrote: From: Carlos Corbacho [EMAIL PROTECTED] Most newer Acer laptops (from 2005 onwards) now ship with an extra Dollar and Euro key either side of the 'Up' arrow. These cannot be mapped in the traditional way, since they are not combination keys. Applied, thank you Carlos. I tought about KEY_CURRENCY some more and I don't think it would work well in this case. -- Dmitry
Re: [PATCH] Input: Support for a less exclusive grab.
Hi Ryan, On 9/28/07, Ryan Lortie [EMAIL PROTECTED] wrote: Hello. I have been working on a more flexible system for blocking the delivery of input events to other agents in the system. My approach is basically summed up as follows: - split the current purpose of input_handle into two parts - input_handle continues to exist as a mechanism for tracking which handlers are currently interested in a particular device - a new input_client now exists as a mechanism for tracking tracking who is interested in having events delivered from a particular device. As an example, for evdev, one input_handle will exist per event device in /dev/input and one input_client will exist per open(). - cause input events to be delivered to 'clients' (ie: first argument is an input_client instead of input_handle) - add a concept of 'priority' - the new input_client structure has a priority field - the list of input_client's per device is kept sorted by this field - add the ability for the handler 'event' function to block further event propagation. This is done by making the handler return 'int' instead of 'void'. 0: continue normally. 1: stop. - add set priority ioctl to evdev - add set requested keys ioctl to evdev. This causes delivery of only certain keycodes to this particular open(). I have no particular interest in doing this sort of masking for other (non-key) events but maybe it makes sense. - add set filter ioctl to evdev. This causes any requested keys to be consumed and not further delivered (ie: the event handler for this client will return 1). - also, as a small style issue: I made what I believe to be a simplification to the switch() statement in the evdev ioctl code. Whether this is actually a simplification or not is a matter of opinion. :) - port the non-evdev handlers (including rfkill) to the new handler/client interface in the most trivial way possible: open the input_client in the places where the old code used to call open with the input_handler. The motivation is to allow arbitrary things in user-space to have a rich support over blocking (possibly a select set of) key events from being delivered to anything below a certain 'priority'. I believe this will solve the problem of X wanting to block key delivery to the VT module while still allowing the events to go through to rfkill (which presumably will be given a higher priority). This will also allow hal's multimedia key reporting code to be improved in two ways: first, it can block the multimedia keys from being delivered to X (and causing weird ^[[29~ things to appear in my terminals) and second, it results in hal only waking up when an interesting key is pressed (currently it wakes up on every single keypress). I like the idea of limiting number of events that client wants to be delivered to it. I think we need to extend it from keybit only to event bitmask + keybit. This way, if keyboard has a scrollwheel or a touchpad installed, HAL can still specify EV_KEY + KEY_XXX and ignore all REL_* and ABS_* events. Priority/filter idea is different matter. I don't think it is a giood solution. There will always be an arms race, new applications would like to get in front of the queue all the time and it will be hard to manage. X should just keep console open in raw mode and simply ignore all events coming from it while using evdev driver. I understand that X's hotplug support is getting there so you should be able to switch to pure evdev setup pretty easy. I have written a patch. It is over 40kb so I don't include it directly here. It is available in its full form (or broken up) at this url: http://desrt.mcmaster.ca/code/input-patch/ This patch is not being proposed for inclusion in its current state. In addition to the many problems that I'm sure I don't even realise, here are some that I would specifically like feedback on: - how should we decide what gets what priority level? Exactly. You can't predict all future uses. There will always be someone wanting to get in front of the line. - is it appropriate to have INPUT_PRIORITY_HIGH/LOW/SYSTEM/DEBUG, etc. macros in input.h? - why did the old code call flush() on the device before closing when disconnecting? It seems particularly silly now that with my patch it will be called n times (one for each client) To remove any force-feedback effects installed by client that is being disconnected. - how are switches supposed to be indented? the checkpatch script in the kernel complained about my style despite following what was already in that file. I think I fixed this. - I haven't tested on big endian or compat systems at all. As far as I know, the relevant code doesn't even compile. Help here is appreciated (I don't have such a machine!). - there is small thread-safety issue with how I handle
Re: [PATCH] Input: Support for a less exclusive grab.
On 10/23/07, Ryan Lortie [EMAIL PROTECTED] wrote: On Tue, 2007-23-10 at 09:21 -0400, Dmitry Torokhov wrote: Priority/filter idea is different matter. I don't think it is a giood solution. There will always be an arms race, new applications would like to get in front of the queue all the time and it will be hard to manage. X should just keep console open in raw mode and simply ignore all events coming from it while using evdev driver. I understand that X's hotplug support is getting there so you should be able to switch to pure evdev setup pretty easy. The only reason that I don't like the numeric priority system is that I believe it is arbitrary and a bit kludgy. I don't think an arms race will be a problem. We don't have a lot of closed source applications running as root on Linux and open source projects either choose sane values or get patched (by users, distributions, etc) or don't get used. I do believe there needs to be some concept of filtering events in such a way that they reach some clients but not others -- that's half of the purpose of this thread. rfkill wants to be able to see keypresses while they are kept away from X. No, rfkill want to see keypresses, period. It does not care if there are other applications also seeing the same keypresses, it just does not want keypresses stolen from it. Rfkill should work just fine if there is a debug application capturing events or a OSD utility monitoring RF state. Actually it would be great if amy of the control utililities were split in 2 parts - one is daemon running even without X and controlling RF starte, brightness, suspend, etc, etc, and another part running in X (or some other environment) providing OSD services. I mean it is really annoying when you realize you can't suspend witha keypress because you happen to be a KDM promot and not fully logged into a KDE session... I'd be open to another way of handling this but I think any other way will necessarily be more complex to use. As for X using evdev, it still puts you in a position of two pieces of software being required to know what events have been (conceptually) filtered by various filters running on the system. ie: the filter itself needs to know, and also X needs to know so that it can remove those key events from normal delivery. Yes, applications need to decide whether they want to process certain events or not. But I think that they shodul do it for themselves, not for other applications. Otherwise dependencies are just insane and you risk to disturb the peace just by adding another piece in the mix. -- Dmitry
Re: [PATCH 1/1] INPUT/BF54x-KEYPAD driver: Remove useless line -errno returned by irq_request
On Thursday 18 October 2007, Bryan Wu wrote: From: Michael Hennerich [EMAIL PROTECTED] Cc: Andrey Panin [EMAIL PROTECTED] Signed-off-by: Michael Hennerich [EMAIL PROTECTED] Signed-off-by: Bryan Wu [EMAIL PROTECTED] Applied, thank you Michael, Bryan. -- Dmitry
Re: Power button policy and mechanism
Hi Kristoffer, On 10/16/07, Kristoffer Ericson [EMAIL PROTECTED] wrote: Greetings Dmitry, Is the suggested approach on handling powerbutton (in keyboard driver) to simply push out the event and let userland handle it? Yes. The reason Im asking this is because as you might know Im maintainer for two mini-laptop style pda's (HP7xx HP6xx) and it would simplify my life alot if I didn't need to depend on userland applications to be able to suspend/resume. For instance HP6XX receives an interrupt call whenever the powerbutton is pressed. Now I could just push out the event and let another program handle it but considering it would take a minimum amount of lines to let it simply suspend/resume I feel its a waste. Previously the hp6xx has been allowed to do this policy way but that was when LinuxSH stod as a side branch to main tree. Now when everything gets merged into mainline I need to decide how to do this. This is mainly an embedded issue, but I feel it's quite important. It should apply to other devices also like for example Zaurus branches (those with keyboard and designated power button). So in short: 1. Does mainline policy allow static power button events inside kernel (power button == suspend/resume)? Why/Why Not? Could it be that you may want to prevent suspend from happening? Or delay it until system completes some important operation? Do something else, like cleanly disconnect your network connections? With actual handling done in userspace it's all possible. With suspend done directly in kernel it is much harder and couples input subsystem with power management too tightly. However if you are dead-set on doin it in kernel you coudl register an input_handler in your platform PM code and it will attach to your keyboard. Look for power.c module in older kernels for example. 2. Seeing as my knowledge about this area isn't the best I would appreciate all opinions on the subject from the gurus. Richard Purdie I think might have some pointers. -- Dmitry
Re: Enabling extra scancodes on some Acer laptops
Hi Carlos, On 10/14/07, Carlos Corbacho [EMAIL PROTECTED] wrote: After calling set_keyboard_quirk(), the extra keys start generating proper scancodes, and can be mapped as normal via setkeycodes. At the moment, I've added this quirk to acer_acpi (it's currently only applied on the few systems we know that need it via DMI matching), but I'm not really sure if this is the right place for it as: 1) acer_acpi is still out of tree 2) Does this really belong in acer_acpi (I'm not really sure if it falls under what I'm doing - this seems to border a bit too much on the input tree side), or somewhere further up the input tree (wistron-btns isn't much use here, as the quirk would then be limited to 32 bit only, whereas some of the machines that need this quirk can run a 64 bit kernel)? I think it could be added to i8042 driver to enable it. How messy is detection? I am also going to apply the patch that exports i8042_command() so it can be used by other modules (Clevo LED driver needs it). -- Dmitry
Re: Enabling extra scancodes on some Acer laptops
On 10/16/07, Carlos Corbacho [EMAIL PROTECTED] wrote:
Dimitry,
On Tuesday 16 October 2007 15:37:37 you wrote:
I think it could be added to i8042 driver to enable it. How messy is
detection?
I use simple DMI matching in acer_acpi to do this on known broken laptops.
(acerhk calls directly into the BIOS to find this information - but I suspect
the data from that could easily be extracted and converted to proper DMI
table entries).
These are the the three that I know of (and are supported by acer_acpi) that
require this quirk:
static struct dmi_system_id dritek_extension_quirk[] = {
{
.callback = dmi_matched,
.ident = Acer Aspire 5650,
.matches = {
DMI_MATCH(DMI_SYS_VENDOR, Acer),
DMI_MATCH(DMI_PRODUCT_NAME, Aspire 5650),
},
},
{
.callback = dmi_matched,
.ident = Acer Aspire 5680,
.matches = {
DMI_MATCH(DMI_SYS_VENDOR, Acer),
DMI_MATCH(DMI_PRODUCT_NAME, Aspire 5680),
},
},
{
.callback = dmi_matched,
.ident = Acer TravelMate 2490,
.matches = {
DMI_MATCH(DMI_SYS_VENDOR, Acer),
DMI_MATCH(DMI_PRODUCT_NAME, TravelMate 2490),
},
},
{}
};
OK, we should be able to add it to i8042 pretty easily.
--
Dmitry
Re: [PATCH try #3] Input/Joystick Driver: add support AD7142 joystick driver
On 10/16/07, Bryan Wu [EMAIL PROTECTED] wrote: On 10/16/07, Dmitry Torokhov [EMAIL PROTECTED] wrote: On 10/15/07, Bryan Wu [EMAIL PROTECTED] wrote: On 10/15/07, Dmitry Torokhov [EMAIL PROTECTED] wrote: Completion is just not a good abstraction here... Please use work abstraction and possibly a separate workqueue. Yes, I agree with you now, although I have a little concern about the possibility of big delay introduced by workqueue. Having a separate workqueue should isolate the driver from users hogging keventd. Otherwise the speed should be pretty much the same as with a kthread. Does this driver need the create a new kthread instead of keventd? I think keventd might be sufficient for this driver. No it does not have to start a new workqueue. I'd start with keventd and only implement a separate workqueue later if I saw the driver being starved by other keventd users. -- Dmitry
Re: Problem with appletouch driver in Linux version 2.6.23-rc7
Hi Soeren, On 10/13/07, Soeren Sonnenburg [EMAIL PROTECTED] wrote: I am not sure whether it is worth fighting for. For the moment I think we should just use the attached patch (where I as you suggest set idlecount to 2). So if there are no objections, Dmity please apply. I already applied the previous version of the patch. I think we are getting to best is the enemy of good stage at this point so I'll drop this one, at leat fro now. Thanks. -- Dmitry
Re: [patch 1/8] m68k: Atari input drivers cleanup
Hi Geert,
On 10/13/07, Geert Uytterhoeven [EMAIL PROTECTED] wrote:
m68k: Atari input drivers cleanup:
- memleak on failed init/register of input devices fixed
- correct keycodes table (Atari keycodes are almost, but not entirely, equal
to Linux keycodes).
Signed-off-by: Michael Schmitz [EMAIL PROTECTED]
Signed-off-by: Geert Uytterhoeven [EMAIL PROTECTED]
Looks much better, thank you.
- input_register_device(atakbd_dev);
+ /* error check */
+ if (input_register_device(atakbd_dev)) {
+ input_free_device(atakbd_dev);
+ return -ENOMEM;
+ }
I'd be more happy if we returned real error reported by
input_register_device() instead of substituting it with -ENOMEM.
--
Dmitry
Re: [PATCH try #3] Input/Joystick Driver: add support AD7142 joystick driver
Hi Bryan,
On 10/15/07, Bryan Wu [EMAIL PROTECTED] wrote:
+
+static int ad7142_thread(void *nothing)
+{
+ do {
+ wait_for_completion(ad7142_completion);
+ ad7142_decode();
+ enable_irq(CONFIG_BFIN_JOYSTICK_IRQ_PFX);
+ } while (!kthread_should_stop());
+
No, this is not going to work well:
- you at least need to reinitialize the completion before enabling
IRQ, otherwise you will spin in a very tight loop
- if noone would touch the joystick ad7142_clsoe would() block
infinitely because noone would signal the completion and
ad7142_thread() would never stop.
Completion is just not a good abstraction here... Please use work
abstraction and possibly a separate workqueue.
+
+ ad7142_task = kthread_run(ad7142_thread, NULL, ad7142_task);
+ if (IS_ERR(ad7142_task)) {
+ printk(KERN_ERR serio: Failed to start kseriod\n);
kseriod?
+ return PTR_ERR(ad7142_task);
+ }
+ return 0;
+}
+
+static void ad7142_close(struct input_dev *dev)
+{
Don't you need to write something over i2c to tell the device to shut
down? As it is now I expect the device to continue raising its IRQ
until kernel decides that it is unhandled and should be ignored.
+ free_irq(CONFIG_BFIN_JOYSTICK_IRQ_PFX, ad7142_interrupt);
Ok, so you freeing IRQ here, but it is allocated in ad7142_probe().
What happen if you try to open device after it was closed?
+ kthread_stop(ad7142_task);
+}
+
+static int __init ad7142_init(void)
+{
+ return i2c_add_driver(ad7142_driver);
+}
+
+static void __exit ad7142_exit(void)
+{
+ i2c_del_driver(ad7142_driver);
+ input_unregister_device(ad7142_dev);
input_unregister_device() should be in ad7142_detach_client? I am not
sure i2c - there seems to be 2 interface styles and you probably need
to use the new one. I am CC-inj Jean on this.
+}
+
+module_init(ad7142_init);
+module_exit(ad7142_exit);
--
1.5.3.4
--
Dmitry
Re: [PATCH 2/3] Input/Touchscreen Driver: add support AD7877 touchscreen driver
Hi Michael,
On 10/15/07, Hennerich, Michael [EMAIL PROTECTED] wrote:
+
+static int ad7877_read(struct device *dev, u16 reg)
+{
+ struct spi_device *spi = to_spi_device(dev);
+ struct ser_req *req = kzalloc(sizeof *req,
GFP_KERNEL);
How many reads can happen at once? Maybe allocate 1 ser_req per
touchcsreen when creating it?
ad7877_read_adc, ad7877_read and ad7877_write are just used by the sysfs
hooks. Touchscreen samples are read by the kthread using a different
message struct. So far each sysfs invocation got its own storage for the
spi message, which then is handed over to the SPI bus driver.
The SPI bus driver serializes transfers in a kthread.
Two different processes could access the drivers sysfs hooks.
Using one ser_req per touch screen could require additional locking?
Things at is, looks pretty safe to me.
OK, fair enough.
--
Dmitry
Re: [PATCH try #3] Input/Joystick Driver: add support AD7142 joystick driver
On 10/15/07, Bryan Wu [EMAIL PROTECTED] wrote: On 10/15/07, Dmitry Torokhov [EMAIL PROTECTED] wrote: Completion is just not a good abstraction here... Please use work abstraction and possibly a separate workqueue. Yes, I agree with you now, although I have a little concern about the possibility of big delay introduced by workqueue. Having a separate workqueue should isolate the driver from users hogging keventd. Otherwise the speed should be pretty much the same as with a kthread. Thanks a lot for you kindly review. I will resend update patch later. Thank you for not getting frustrated with all my change requests. Btw, blackfin keypad driver is in my tree and should be in mainline once Linus does the pull I requested. -- Dmitry
Re: [PATCH try #2] Input/Joystick Driver: add support AD7142 joystick driver
Hi Ahmed, On 10/12/07, Ahmed S. Darwish [EMAIL PROTECTED] wrote: On Fri, Oct 12, 2007 at 03:38:47PM +0800, Bryan Wu wrote: Signed-off-by: Bryan Wu [EMAIL PROTECTED] --- Hi Bryan, Why creating module's own kthread to call ad7142_decode and process keycodes instead of using a tasklet ? Yo can't access i2c from a tasklet context. Isn't disabling device interrupts from the begining of the ISR ad7142_interrupt till the kthread ad7142_thread got waked-up and scheduled a long time, espicially if there's a high load on the userspace side ? It is OK - you disable a specific interrupt line preventing it from raising any more IRQs until current one is serviced. This is different from disabling interrupts on CPU. Minor issues below. + +/* RADC stage 0 - 11 result (uncompensated) actually located in SRAM */ +#define ADCRESULT_S0 0x0B +#define ADCRESULT_S1 0x0C +#define ADCRESULT_S2 0x0D +#define ADCRESULT_S3 0x0E +#define ADCRESULT_S4 0x0F +#define ADCRESULT_S5 0x10 +#define ADCRESULT_S6 0x11 +#define ADCRESULT_S7 0x12 +#define ADCRESULT_S8 0x13 +#define ADCRESULT_S9 0x14 +#define ADCRESULT_S100x15 +#define ADCRESULT_S110x16 + Keeping last two lines aligned with their above counterparts ? I believe they are aligned if you aplly the patch. -- Dmitry
Re: [PATCH try #2] Input/Joystick Driver: add support AD7142 joystick driver
On 10/12/07, Bryan Wu [EMAIL PROTECTED] wrote:
On Fri, 2007-10-12 at 11:50 -0400, Dmitry Torokhov wrote:
On 10/12/07, Bryan Wu [EMAIL PROTECTED] wrote:
On Fri, 2007-10-12 at 10:49 -0400, Dmitry Torokhov wrote:
Hi Bryan,
On 10/12/07, Bryan Wu [EMAIL PROTECTED] wrote:
+
[snip]
+
+static void ad7142_close(struct input_dev *dev)
+{
+ free_irq(CONFIG_BFIN_JOYSTICK_IRQ_PFX, ad7142_interrupt);
+ kthread_stop(ad7142_task);
Don't you need to write something over i2c to shut the devoce off?
What stops it from continuing to generate interrupts?
Actually, I am going to use completion to replace the whole
wait_interrupt_xxx and intr_flag things which original from Aubrey. How
do you think of that?
I don't think it is a very good idea - for me completion is one-time
deal. You use it and then you are done. How about firing a work from
interrupt and either rely on the default workqueue (keventd) or create
your own to execute it?
completion is a wrapper of workqueue and simpler to use.
my method:
1. In kthread:
do {
|___|___wait_for_completion(ad7142_completion);
|___|___ad7142_decode();
|___|___enable_irq(CONFIG_BFIN_JOYSTICK_IRQ_PFX);
|___} while (!kthread_should_stop());
2. In irq handler will fire complete(ad7142_completion);
This is simpler and understand easier
You also need to re-initialize completion every time you done
processing in kthread otherwise you will be constantly doing
ad7142_decode(). Plus, how are you going to stop kthread (completion
may not be signalled for a long time if nobody touches the joystick).
--
Dmitry
Re: [PATCH try #2] Input/Joystick Driver: add support AD7142 joystick driver
On 10/12/07, Bryan Wu [EMAIL PROTECTED] wrote:
On Fri, 2007-10-12 at 10:49 -0400, Dmitry Torokhov wrote:
Hi Bryan,
On 10/12/07, Bryan Wu [EMAIL PROTECTED] wrote:
+
[snip]
+
+static void ad7142_close(struct input_dev *dev)
+{
+ free_irq(CONFIG_BFIN_JOYSTICK_IRQ_PFX, ad7142_interrupt);
+ kthread_stop(ad7142_task);
Don't you need to write something over i2c to shut the devoce off?
What stops it from continuing to generate interrupts?
Actually, I am going to use completion to replace the whole
wait_interrupt_xxx and intr_flag things which original from Aubrey. How
do you think of that?
I don't think it is a very good idea - for me completion is one-time
deal. You use it and then you are done. How about firing a work from
interrupt and either rely on the default workqueue (keventd) or create
your own to execute it?
Thank you.
--
Dmitry
Re: [PATCH try #2] Input/Joystick Driver: add support AD7142 joystick driver
On 10/12/07, Ahmed S. Darwish [EMAIL PROTECTED] wrote: On Fri, Oct 12, 2007 at 01:29:31PM -0400, Dmitry Torokhov wrote: Isn't disabling device interrupts from the begining of the ISR ad7142_interrupt till the kthread ad7142_thread got waked-up and scheduled a long time, espicially if there's a high load on the userspace side ? It is OK - you disable a specific interrupt line preventing it from raising any more IRQs until current one is serviced. Won't this affect system responsiveness if the IRQ line was shared ? You are right, this would not work in case of shared IRQs. Hovewer this driver is for an embedded arch and the line is not shared. This is different from disabling interrupts on CPU. mm, Why disabling interrupts in general. Doesn't IRQ hanlers of the same kind got executed in a serialized fashion even on SMPs ?. If so, why not just wakeup our custom-thread or use workqueues and let them do their business ? Because you don't want CPU to be continually interrupted while untill right thread gets scheduled. I think using work[queue] is the best solution for this driver, but you still want to mask that particular IRQ off until you do the read. -- Dmitry
Re: GeneralTouch USB Touchscreen Support
Hi Jiri,
On Monday 08 October 2007, Jiri Kosina wrote:
On Mon, 8 Oct 2007, Dmitry Torokhov wrote:
The patch looks good, thanks a lot. Can I please have your
Signed-off-by: so I can apply it? Thanks!
Hi,
my only concern with this patch is -- could we please keep the entries in
hid_blacklist[] properly sorted? (i.e. first by quirk type, then by vendor
name).
Also, patch to hid-quirks seem to contain some whitespace changes, I'd
like to do this separately if possible.
Besides that, I am fine with that and you can add
Signed-off-by: Jiri Koisna [EMAIL PROTECTED]
for the HID part of the patch if you want to.
I'd rather HID part go through your tree. Here is is split out.
--
Dmitry
Subject: HID: Add GeneralTouch touchscreen to the blacklist
From: Ilya Frolov [EMAIL PROTECTED]
GeneralTouch touchscreens are handled by usbtouchscreen driver,
make sure HID ignores them.
Signed-off-by: Ilya Frolov [EMAIL PROTECTED]
Signed-off-by: Dmitry Torokhov [EMAIL PROTECTED]
---
drivers/hid/usbhid/hid-quirks.c |6 ++
1 file changed, 6 insertions(+)
Index: work/drivers/hid/usbhid/hid-quirks.c
===
--- work.orig/drivers/hid/usbhid/hid-quirks.c
+++ work/drivers/hid/usbhid/hid-quirks.c
@@ -110,6 +110,8 @@
#define USB_VENDOR_ID_GAMERON 0x0810
#define USB_DEVICE_ID_GAMERON_DUAL_PSX_ADAPTOR 0x0001
+#define USB_VENDOR_ID_GENERAL_TOUCH0x0dfc
+
#define USB_VENDOR_ID_GLAB 0x06c2
#define USB_DEVICE_ID_4_PHIDGETSERVO_300x0038
#define USB_DEVICE_ID_1_PHIDGETSERVO_300x0039
@@ -392,6 +394,10 @@ static const struct hid_blacklist {
{ USB_VENDOR_ID_DELORME, USB_DEVICE_ID_DELORME_EARTHMATE,
HID_QUIRK_IGNORE },
{ USB_VENDOR_ID_DELORME, USB_DEVICE_ID_DELORME_EM_LT20,
HID_QUIRK_IGNORE },
{ USB_VENDOR_ID_ESSENTIAL_REALITY, USB_DEVICE_ID_ESSENTIAL_REALITY_P5,
HID_QUIRK_IGNORE },
+ { USB_VENDOR_ID_GENERAL_TOUCH, 0x0001, HID_QUIRK_IGNORE },
+ { USB_VENDOR_ID_GENERAL_TOUCH, 0x0002, HID_QUIRK_IGNORE },
+ { USB_VENDOR_ID_GENERAL_TOUCH, 0x0003, HID_QUIRK_IGNORE },
+ { USB_VENDOR_ID_GENERAL_TOUCH, 0x0004, HID_QUIRK_IGNORE },
{ USB_VENDOR_ID_GLAB, USB_DEVICE_ID_4_PHIDGETSERVO_30, HID_QUIRK_IGNORE
},
{ USB_VENDOR_ID_GLAB, USB_DEVICE_ID_1_PHIDGETSERVO_30, HID_QUIRK_IGNORE
},
{ USB_VENDOR_ID_GLAB, USB_DEVICE_ID_0_0_4_IF_KIT, HID_QUIRK_IGNORE },
Re: [PATCH 2/3] Input/Touchscreen Driver: add support AD7877 touchscreen driver
Hi Bryan, Michael,
On 10/11/07, Bryan Wu [EMAIL PROTECTED] wrote:
+
+static int gpio3 = 0;
No need to initialize.
+
+static int ad7877_read(struct device *dev, u16 reg)
+{
+ struct spi_device *spi = to_spi_device(dev);
+ struct ser_req *req = kzalloc(sizeof *req, GFP_KERNEL);
How many reads can happen at once? Maybe allocate 1 ser_req per
touchcsreen when creating it?
+
+ if (likely(x z1 !device_suspended(ts-spi-dev))) {
+ /* compute touch pressure resistance using equation #2 */
+ Rt = z2;
+ Rt -= z1;
+ Rt *= x;
+ Rt *= ts-x_plate_ohms;
+ Rt /= z1;
+ Rt = (Rt + 2047) 12;
+ } else
+ Rt = 0;
+
+ if (Rt) {
+ input_report_abs(input_dev, ABS_X, x);
+ input_report_abs(input_dev, ABS_Y, y);
+ sync = 1;
+ }
+
+ if (sync) {
+ input_report_abs(input_dev, ABS_PRESSURE, Rt);
+ input_sync(input_dev);
+ }
Confused about the logic - you set sync only if Rt != 0 so why don't
fold second if into the first one?
+/* Must be called with ts-lock held */
+static void ad7877_disable(struct ad7877 *ts)
+{
+ if (ts-disabled)
+ return;
+
+ ts-disabled = 1;
+
+ if (!ts-pending) {
+ ts-irq_disabled = 1;
+ disable_irq(ts-spi-irq);
+ } else {
+ /* the kthread will run at least once more, and
+* leave everything in a clean state, IRQ disabled
+*/
+ while (ts-pending) {
+ spin_unlock_irq(ts-lock);
+ msleep(1);
+ spin_lock_irq(ts-lock);
+ }
+ }
+
+ /* we know the chip's in lowpower mode since we always
+* leave it that way after every request
+*/
+
+}
This looks scary. Can you try moving locking inside ad7877_enable and
ad7877_disable?
+
+static int __devinit ad7877_probe(struct spi_device *spi)
+{
+ struct ad7877 *ts;
+ struct input_dev*input_dev;
+ struct ad7877_platform_data *pdata = spi-dev.platform_data;
+ struct spi_message *m;
+ int err;
+ u16 verify;
+
+
+ if (!spi-irq) {
+ dev_dbg(spi-dev, no IRQ?\n);
+ return -ENODEV;
+ }
+
+
+ if (!pdata) {
+ dev_dbg(spi-dev, no platform data?\n);
+ return -ENODEV;
+ }
+
+
+ /* don't exceed max specified SPI CLK frequency */
+ if (spi-max_speed_hz MAX_SPI_FREQ_HZ) {
+ dev_dbg(spi-dev, SPI CLK %d Hz?\n,spi-max_speed_hz);
+ return -EINVAL;
+ }
+
+ ts = kzalloc(sizeof(struct ad7877), GFP_KERNEL);
+ input_dev = input_allocate_device();
+ if (!ts || !input_dev) {
+ err = -ENOMEM;
+ goto err_free_mem;
+ }
+
+
+ dev_set_drvdata(spi-dev, ts);
+ spi-dev.power.power_state = PMSG_ON;
+
+ ts-spi = spi;
+ ts-input = input_dev;
+ ts-intr_flag = 0;
+ init_timer(ts-timer);
+ ts-timer.data = (unsigned long) ts;
+ ts-timer.function = ad7877_timer;
+
+ spin_lock_init(ts-lock);
+
+ ts-model = pdata-model ? : 7877;
+ ts-vref_delay_usecs = pdata-vref_delay_usecs ? : 100;
+ ts-x_plate_ohms = pdata-x_plate_ohms ? : 400;
+ ts-pressure_max = pdata-pressure_max ? : ~0;
+
+
+ ts-stopacq_polarity = pdata-stopacq_polarity;
+ ts-first_conversion_delay = pdata-first_conversion_delay;
+ ts-acquisition_time = pdata-acquisition_time;
+ ts-averaging = pdata-averaging;
+ ts-pen_down_acc_interval = pdata-pen_down_acc_interval;
+
+ snprintf(ts-phys, sizeof(ts-phys), %s/input0, spi-dev.bus_id);
+
+ input_dev-name = AD7877 Touchscreen;
+ input_dev-phys = ts-phys;
+ input_dev-cdev.dev = spi-dev;
Please use input_dev-dev.parent, i will kill 'cdev' soon.
+
+ err = input_register_device(input_dev);
+ if (err)
+ goto err_remove_attr;
+
+ ts-intr_flag = 0;
+
+ ad7877_task = kthread_run(ad7877_thread, ts, ad7877_ktsd);
+
+if (IS_ERR(ad7877_task)) {
+printk(KERN_ERR ts: Failed to start ad7877_task\n);
+goto err_remove_attr;
You shoudl use input_unregister_device() once it was registered. So
you need something like
goto err_unregister_idev;
...
err_unregister_idev:
input_unregister_device(input_dev);
input-dve = NULL; /* so we don't try to free it later */
err_remove_attr:
...
+}
+
+ return 0;
+
+ err_remove_attr:
+
+
Re: [PATCH try #3] Blackfin BF54x Input Keypad controller driver
On Thursday 11 October 2007, Bryan Wu wrote:
From: Michael Hennerich [EMAIL PROTECTED]
Date: Fri, 12 Oct 2007 00:20:19 +0800
Subject: [PATCH] Blackfin BF54x Input Keypad controller driver
[try #2] Changelog:
- Coding style issue fixes
- using a temp variable for bf54x_kpad-input
- Other updates according to Dmitry's review
[try #3] Changelog:
- Coding style cleanups
- Use local copy of keymap
- Remove empty PM functions
- Use input_set_drvdata() since input-private is going away
This looks very good, thank youvery much. I have only 1 more suggestion
(and I can make the changes locally, you don't need to resubmit) -
since it is highly unlikely that we will have a keycode 65535 do you
think we could change keymap to unsigned short. It would save you a
couple of bytes. I am also going to change input-cdev.dev = pdev-dev;
to input_dev-dev.parent = pdev-dev;. Please let me know if you are
OK with it.
Thanks!
Cc: Dmitry Torokhov [EMAIL PROTECTED]
Signed-off-by: Michael Hennerich [EMAIL PROTECTED]
Signed-off-by: Bryan Wu [EMAIL PROTECTED]
---
arch/blackfin/mach-bf548/boards/ezkit.c |2 +-
drivers/input/keyboard/Kconfig |9 +
drivers/input/keyboard/Makefile |2 +-
drivers/input/keyboard/bf54x-keys.c | 378
++
include/asm-blackfin/mach-bf548/bf54x_keys.h | 17 ++
5 files changed, 406 insertions(+), 2 deletions(-)
create mode 100644 drivers/input/keyboard/bf54x-keys.c
create mode 100644 include/asm-blackfin/mach-bf548/bf54x_keys.h
diff --git a/arch/blackfin/mach-bf548/boards/ezkit.c
b/arch/blackfin/mach-bf548/boards/ezkit.c
index 2c47db4..6734b3d 100644
--- a/arch/blackfin/mach-bf548/boards/ezkit.c
+++ b/arch/blackfin/mach-bf548/boards/ezkit.c
@@ -88,7 +88,7 @@ static struct platform_device bf54x_lq043_device = {
#endif
#if defined(CONFIG_KEYBOARD_BFIN) || defined(CONFIG_KEYBOARD_BFIN_MODULE)
-static int bf548_keymap[] = {
+static unsigned int bf548_keymap[] = {
KEYVAL(0, 0, KEY_ENTER),
KEYVAL(0, 1, KEY_HELP),
KEYVAL(0, 2, KEY_0),
diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig
index c97d5eb..2136a9e 100644
--- a/drivers/input/keyboard/Kconfig
+++ b/drivers/input/keyboard/Kconfig
@@ -253,4 +253,13 @@ config KEYBOARD_GPIO
To compile this driver as a module, choose M here: the
module will be called gpio-keys.
+config KEYBOARD_BFIN
+ tristate Blackfin BF54x keypad support
+ depends on (BF54x)
+ help
+ Say Y here if you want to use the BF54x keypad.
+
+ To compile this driver as a module, choose M here: the
+ module will be called bf54x-keypad.
+
endif
diff --git a/drivers/input/keyboard/Makefile b/drivers/input/keyboard/Makefile
index 28d211b..3123277 100644
--- a/drivers/input/keyboard/Makefile
+++ b/drivers/input/keyboard/Makefile
@@ -21,4 +21,4 @@ obj-$(CONFIG_KEYBOARD_OMAP) += omap-keypad.o
obj-$(CONFIG_KEYBOARD_PXA27x)+= pxa27x_keyboard.o
obj-$(CONFIG_KEYBOARD_AAED2000) += aaed2000_kbd.o
obj-$(CONFIG_KEYBOARD_GPIO) += gpio_keys.o
-
+obj-$(CONFIG_KEYBOARD_BFIN) += bf54x-keys.o
diff --git a/drivers/input/keyboard/bf54x-keys.c
b/drivers/input/keyboard/bf54x-keys.c
new file mode 100644
index 000..2833c60
--- /dev/null
+++ b/drivers/input/keyboard/bf54x-keys.c
@@ -0,0 +1,378 @@
+/*
+ * File: drivers/input/keyboard/bf54x-keys.c
+ * Based on:
+ * Author: Michael Hennerich [EMAIL PROTECTED]
+ *
+ * Created:
+ * Description: keypad driver for Analog Devices Blackfin BF54x Processors
+ *
+ *
+ * Modified:
+ * Copyright 2007 Analog Devices Inc.
+ *
+ * Bugs: Enter bugs at http://blackfin.uclinux.org/
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, see the file COPYING, or write
+ * to the Free Software Foundation, Inc.,
+ * 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
+ */
+
+#include linux/module.h
+#include linux/version.h
+
+#include linux/init.h
+#include linux/fs.h
+#include linux/interrupt.h
+#include linux/irq.h
+#include linux/sched.h
+#include linux/pm.h
+#include linux/sysctl.h
+#include linux/proc_fs.h
+#include linux/delay.h
+#include linux/platform_device.h
+#include linux/input.h
+#include linux/irq.h
Re: [PATCH] Generic, platform independent matrix keyboard support
On 10/9/07, David Brownell [EMAIL PROTECTED] wrote: On Tuesday 09 October 2007, Pavel Pisa wrote: On the other hand I cannot imagine electronic designed wanting so many wires. Reduction of the wires counts is why matrix keyboards are used. The 32 bits limitation then means that maximal supported keyboard can be 32x32 key = 1024 keys, that is quite imposing. Yes: http://weblog.sinteur.com/?p=20412 :) Thank you David ;) -- Dmitry
Re: [PATCH] Generic, platform independent matrix keyboard support
On 10/9/07, Pavel Pisa [EMAIL PROTECTED] wrote:
Name of corresponding platform and PCI methods is probe.
But name can be changed if new one is seen as more logical.
There you indeed probing devices for supported functionality. But here
you just register a new object with the system (note that all such
methods have 'register' in them - device_register(),
input_register_device() , etc).
+
+ input_dev = input_allocate_device();
+ if (!input_dev)
+ goto fail_arr_alloc;
+
+ kbd-input = input_dev;
+ kbd-hw_dev = dev;
+ dev_set_drvdata(dev, kbd);
Does not belong here.
Where I can store pointer to specialized part.
I mean setting up kbd does not belong here. You set it up (by setting
hw_dev and calling dev_set_drvdadat) before calling
genmatrix_register().
+
+ /* Setup input device */
+ input_dev-name = GenMatrix Keyboard;
+ input_dev-phys = kbd-phys;
+ input_dev-dev.parent = dev;
Make this input_dev-dev.parent = kbd-dev; and get rid of dev argument.
I am not fully sure what it would cause in device model hierarchy,
but I try to learn and check that.
I meant input_dev-dev.parent = kbd-hw_dev;
+
+ input_dev-id.bustype = BUS_HOST;
+ input_dev-id.vendor = 0x0001;
+ input_dev-id.product = 0x0001;
+ input_dev-id.version = 0x0100;
+
+ input_dev-evbit[0] = BIT(EV_KEY) | BIT(EV_REP); /* BIT(EV_PWR) |
BIT(EV_SW); */ + input_dev-keycode = kbd-keycode;
+ input_dev-keycodesize = sizeof(*kbd-keycode);
+ input_dev-keycodemax = kbd-keycode_cnt;
+
+ for (i = 0; i kbd-keycode_cnt; i++)
+ set_bit(kbd-keycode[i], input_dev-keybit);
+ clear_bit(0, input_dev-keybit);
+ /*
+ set_bit(SW_LID, input_dev-swbit);
+ set_bit(SW_TABLET_MODE, input_dev-swbit);
+ set_bit(SW_HEADPHONE_INSERT, input_dev-swbit);
+ */
+
+ err = kbd-setup(kbd-hw_dev);
+ if (err) {
+ dev_err(kbd-hw_dev, low-level hardware setup
failed\n); + goto fail;
+ }
+
+ err = input_register_device(input_dev);
+ if (err) {
+ dev_err(kbd-hw_dev, input device registration\n);
+ goto fail_to_register;
+ }
+
+ mod_timer(kbd-timer, jiffies + 1);
Do not start timer/IRQs until there are users. Implement
inptu_dev-open() abd -close() and do it from there.
OK, that is right solution. I look at that.
+static int genmatrix_remove(struct device *dev)
+{
..
+ dev_set_drvdata(dev, NULL);
+
+ kfree(kbd-phys);
+ kfree(kbd-down_arr);
+ kfree(kbd-chng_arr);
+ kfree(kbd-keycode);
I don't see you allocate kbd-keycode, why are you freeing it?
Somebody has to remove it. May it be, that free calls could/should
be distributed different way between genmatrix_plat_remove()
and genmatrix_remove().
+
+ kfree(kbd);
Actually, why are you freeing kbd? If it was not allocated in probe(),
it should not be freed in remove().
Somebody has to free it. The genmatrix_remove() may be more
appropriate, but I would be happy to not teach that how pointer to
kbd is stored in dev hierarchy. But move could be right thing to do.
But kbd may be a part of a bigger structure and not allocated
separately. I prefer the following rule - if a layer did not allocate
a resource it should not try to free it.
--
Dmitry
Re: [PATCH] Generic, platform independent matrix keyboard support
Hi Pavel,
On 10/9/07, Pavel Pisa [EMAIL PROTECTED] wrote:
Subject: Generic, platform independent matrix keyboard support
From: Pavel Pisa [EMAIL PROTECTED]
The genmatrix_kbd module provides support for matrix keyboard
where switches interconnects return lines with port driven
scan lines. Actual version code allow to register concrete
hardware described by platform device. Hardware can be described
by list of output and input pins and manipulation can be delegated
to GPIO layer or hardware specific setup(), release(), activate_all()
deactivate_all() and scan_single() functions can be defined.
Only commenting on the generic part, not GPIO for now...
+
+#include linux/genmatrix_kbd.h
+
+/* Some sane defaults */
+#define KEY_PUSH_T 20
+#define KEY_RELEASE_T 10
+#define KEY_PUSHCHECK_T 20
+
+/* Individual key states */
+#define KEY_STATE_IDLE 0
+#define KEY_STATE_PUSH 1
+#define KEY_STATE_RELEASE 2
+#define KEY_STATE_PUSHED 4
+#define KEY_STATE_NOISE8
+#define KEY_STATE_BUSY (KEY_STATE_PUSH|KEY_STATE_RELEASE)
+
+/* Bit flags */
+#define GENMATRIX_FLG_WITH_IRQ_b 0
+#define GENMATRIX_FLG_IRQ_EN_b 1
+
+typedef unsigned genmatrix_retmask_t;
+
+struct genmatrix_kbd {
+ struct input_dev *input;
+ struct device *hw_dev;
+ char *phys;
+ unsigned long flags;
+
+ /* Platform specific information */
+ unsigned scan_cnt;
+ unsigned ret_cnt;
+
+ int (*setup)(struct device *dev);
+ void (*release)(struct device *dev);
+ unsigned (*activate_all)(struct device *dev, int setup_irq);
+ void (*deactivate_all)(struct device *dev, int disable_irq);
+ unsigned (*scan_single)(struct device *dev, unsigned scannr);
+
+ /* State of keyboard matrix and key press reporting */
+ genmatrix_retmask_t *down_arr; /* array of size scan_cnt */
+ genmatrix_retmask_t *chng_arr; /* array of size scan_cnt */
+ unsigned char key_hit;
I see you are setting it but never use...
+ intkey_last_changed;
+
+ /* Internal state for repeat processing */
+ intkey_state;
+ unsigned long check_time;
Input core alredy has repeat processing. Is it not sufficient?
+
+ unsigned long push_time;
+ unsigned long release_time;
+ unsigned long pushcheck_time;
+
+ spinlock_t lock;
OK, you have a lock, but I don't see it being used anywhere.
+ struct timer_list timer;
+
+ /* Scancode to key translation */
+ unsigned keycode_cnt;
+ unsigned char *keycode;
+
+ intsuspended;
What is 'suspended' member needed for?
+};
+
+/**
+ * genmatrix_scan - Scan keyboard matrix and report requests for state change
+ * @kbd: keyboard instance data and state structure
+ *
+ * Scans keyboard matrix connected row by row by calling function
+ * mx1_kbd_onerow(). Number of scanned output lines is defined
+ * by %KBD_SCAN_CNT. Checks read keyboard state against @down_arr
+ * and updates @key_change_arr array. The @down_arr state is
+ * left unchanged. It is changed later by kbd_down() function.
+ * Returns 0, if no keyboard activity is found. Returns 1
+ * if at least one key is pressed. Returns 2 or 3 in case
+ * of detected change.
+ */
The comment refers to non-existing functions. This makes harder to
fugure out what is going on.
+static int genmatrix_scan(struct genmatrix_kbd *kbd)
+{
+ int i, ret = 0;
+ genmatrix_retmask_t val, chng;
+ for (i = 0; i kbd-scan_cnt; i++) {
+ val = kbd-scan_single(kbd-hw_dev, i);
+ chng = val ^ kbd-down_arr[i];
+ kbd-chng_arr[i] = chng;
Ok, so there can be only 32 keys per row... Kind of unfortunate
limitation for generic layer. Although if you forget about matrix
stuff and convert down_arr and chg_arr to be just a bitmaps and pass
it to scan_single along with irq (if any) that could be generic
enough.
+ if (val)
+ ret |= 1;
+ if (chng)
+ ret |= 2;
+ }
+ return ret;
+}
+
+/**
+ * genmatrix_down - Detects changed key scancode and applies changes to
matrix state
+ * @kbd: keyboard instance data and state structure
+ *
+ * Functions check @chng_arr and process changes.
+ * It updates its internal state @key_state, does
+ * noise cancellation and repeat timing, then updates
+ * @down_arr, stores detected scancode to @key_last_changed
+ * and calls modifiers processing kbd_scan2mod().
+ * Return value is zero if no change is detected.
+ * In other case evaluated scancode is returned.
+ * Variable @key_hit signals by value 1 pressed key, by value
+ * 2 key release.
+ */
+static int genmatrix_down(struct genmatrix_kbd *kbd)
+{
+ int si, ri = 0;
Re: Handling transition into suspend through keyboard driver
Hi Kristoffer, On 10/8/07, Kristoffer Ericson [EMAIL PROTECTED] wrote: Greetings, Dmitry, Im implementing suspend for hp7xx currently. I've added EV_PWR to bitflags and linked so POWER_KEY gets reported correctly. Is this the correct approach? Yes, it is. Keyboard drivers sghoudl just emit proper events and we expect userspace to react on those properly. Look at corgi and spitz drivers. -- Dmitry
Re: [PATCH try #2] Blackfin BF54x Input Keypad controller driver
Hi Bryan,
On 10/4/07, Bryan Wu [EMAIL PROTECTED] wrote:
From: Michael Hennerich [EMAIL PROTECTED]
Blackfin BF54x Input Keypad controller driver:
[try #2] Changelog:
- Coding style issue fixes
- using a temp variable for bf54x_kpad-input
- Other updates according to Dmitry's review
I would like to ask you for a couple of additional changes:
+
+static u16 per_rows[] = {
Change to const perhaps?
+ P_KEY_ROW7,
+ P_KEY_ROW6,
+ P_KEY_ROW5,
+ P_KEY_ROW4,
+ P_KEY_ROW3,
+ P_KEY_ROW2,
+ P_KEY_ROW1,
+ P_KEY_ROW0,
+ 0
+};
+
+static u16 per_cols[] = {
Same here.
+ P_KEY_COL7,
+ P_KEY_COL6,
+ P_KEY_COL5,
+ P_KEY_COL4,
+ P_KEY_COL3,
+ P_KEY_COL2,
+ P_KEY_COL1,
+ P_KEY_COL0,
+ 0
+};
+
+
+ if (bfin_kpad_get_keypressed(bf54x_kpad)) {
+ disable_irq(bf54x_kpad-irq);
+ bf54x_kpad-lastkey = key;
+ bf54x_kpad-timer.expires = jiffies
+ + bf54x_kpad-keyup_test_jiffies;
+ add_timer(bf54x_kpad-timer);
mod_timer()?
+
+ input-keycode = pdata-keymap;
Please consider allocating memory for keycode so that platfrom data
can be kept constant.
+ input-keycodesize = sizeof(unsigned short);
+ input-keycodemax = pdata-keymapsize;
+
+ /* setup input device */
+ set_bit(EV_KEY, input-evbit);
+
+ if (pdata-repeat)
+ set_bit(EV_REP, input-evbit);
+
__set_bit() should suffice here and above.
+ for (i = 0; i pdata-keymapsize; i++)
+ __set_bit(pdata-keymap[i] KEY_MAX, input-keybit);
+
+ __clear_bit(KEY_RESERVED, input-keybit);
+
+ error = input_register_device(input);
+
+ if (error) {
+ printk(KERN_ERR DRV_NAME
+ : Unable to register input device (%d)\n, error);
+ goto out4;
+ }
+
+ /* Init Keypad Key Up/Release test timer */
+
+ init_timer(bf54x_kpad-timer);
+ bf54x_kpad-timer.function = bfin_kpad_timer;
+ bf54x_kpad-timer.data = (unsigned long) pdev;
+
setup_timer()?
+
+ bfin_write_KPAD_PRESCALE(bfin_kpad_get_prescale(TIME_SCALE));
+
+ bfin_write_KPAD_CTLpdata-cols - 1) 13) KPAD_COLEN) |
+ (((pdata-rows - 1) 10) KPAD_ROWEN) |
+ (2 KPAD_IRQMODE));
+
+
+ bfin_write_KPAD_CTL(bfin_read_KPAD_CTL() | KPAD_EN);
+
+ printk(KERN_ERR DRV_NAME
+ : Blackfin BF54x Keypad registered IRQ %d\n,
bf54x_kpad-irq);
+
+ return 0;
+
+
+out4:
+ input_free_device(input);
+out3:
+ free_irq(bf54x_kpad-irq, pdev);
+out2:
+ peripheral_free_list(per_cols[MAX_RC - pdata-cols]);
+out1:
+ peripheral_free_list(per_rows[MAX_RC - pdata-rows]);
+out:
+ kfree(bf54x_kpad);
+ platform_set_drvdata(pdev, NULL);
+
+ return error;
+}
+
+static int __devexit bfin_kpad_remove(struct platform_device *pdev)
+{
+ struct bfin_kpad_platform_data *pdata = pdev-dev.platform_data;
+ struct bf54x_kpad *bf54x_kpad = platform_get_drvdata(pdev);
+
+
+ del_timer_sync(bf54x_kpad-timer);
+ free_irq(bf54x_kpad-irq, pdev);
+
+ peripheral_free_list(per_rows[MAX_RC - pdata-rows]);
+ peripheral_free_list(per_cols[MAX_RC - pdata-cols]);
+
+ input_unregister_device(bf54x_kpad-input);
+
+ kfree(bf54x_kpad);
+ platform_set_drvdata(pdev, NULL);
+
+ return 0;
+}
+
+#ifdef CONFIG_PM
+static int bfin_kpad_suspend(struct platform_device *pdev, pm_message_t
state)
+{
Do you need these empty functions? At least stop timer here.
Thanks!
Oh, one more thing - do not access input-private directly - it is
going away - use input_set_drvdata() and input_get_drvdata().
--
Dmitry
Re: GeneralTouch USB Touchscreen Support
Hi Ilya, On 10/8/07, if [EMAIL PROTECTED] wrote: Hello! Sending patch as told to. Formal notes: Patch made against 2.6.22.9 sources. Marketing is lying, quick google for model ( TL6B17S) shows they say it has 4096x4096 sensitivity, while device itself do not report more than 0x0470 (i set 0x0500 as max), so with monitors greater than 1024 pixels special calibration is needed. I wrote mail to manufacturer asking for linux drivers but got no response, so i had no way but to sniff the usb bus and figure out how to make the device work, and reversing is not illegal in my country. I agree with GPL of course, the following patch can be used according to it ;-) The patch looks good, thanks a lot. Can I please have your Signed-off-by: so I can apply it? Thanks! -- Dmitry
Re: even newer version of USB HID autosuspend
On 10/8/07, Oliver Neukum [EMAIL PROTECTED] wrote: Am Sonntag 07 Oktober 2007 schrieb Dmitry Torokhov: I'd say do not autosuspend keyboards (and other led-equipped input devices) unless user explicitely requested to do so and then turn the leds off. So your answer is that we shouldn't care about the LEDs being on and suspend regardless? Yes, I think so. -- Dmitry
Re: even newer version of USB HID autosuspend
On Saturday 06 October 2007, Oliver Neukum wrote: This leaves me with a planning difficulty. For keyboards it is not fully transparent. The LEDs are switched off when then keyboard is suspended. Should I now implement a mode where active LEDs block suspension and if so, how do I switch it on? Maybe module parameter of usbhid could be used to specify the desired behavior? Yes, but what are the desired behaviors? I'd say do not autosuspend keyboards (and other led-equipped input devices) unless user explicitely requested to do so and then turn the leds off. We have too many module parameters... -- Dmitry
Re: [PATCH] input: move gameports out of SERIO menu
Hi Randy, On Tuesday 02 October 2007, Randy Dunlap wrote: From: Randy Dunlap [EMAIL PROTECTED] Move the gameport support menu out of the SERIO menu and put it between touchscreens and misc. devices in the main input layer menu. Change it to use menuconfig instead of config. Or was it meant to be listed under Hardware I/O ports? It sure seems odd there to me. Yes, they are meant to be under Hardware I/O ports. Like serio ports they represent hardware ports to which devices (such as gamepads or a joysticks can be connected to). -- Dmitry
Re: new version of usbhid autosuspend patch
Hi Oliver,
On 9/27/07, Oliver Neukum [EMAIL PROTECTED] wrote:
+int hid_check_keys_pressed(struct hid_device *hid)
+{
+ struct hid_input *hidinput, *next;
+ int i;
+
+ list_for_each_entry_safe(hidinput, next, hid-inputs, list) {
+ for (i = 0; i NBITS(KEY_MAX); i++)
+ if (hidinput-input-key[i])
+ return 1;
+ }
Why are you using the list_for_each_entry_safe? You do not alter the
list in your loop.
--
Dmitry
Re: Problem with appletouch driver in Linux version 2.6.23-rc7
On 9/26/07, Soeren Sonnenburg [EMAIL PROTECTED] wrote:
On Mon, 2007-09-24 at 15:22 +0200, Thomas Rohwer wrote:
Hello,
could you please re-send the patch? I for some reason have yet to see
it ...
here it is again, adressing also the comments from Dmitry.
Thomas, Matthew and Dmitry,
I think there is another bug in this. I mean whenever a mouse button is
pressed or the mouse is moved the counter should be reset - no?
Currently the idle counter is just increased...
I mean shouldn't it be
if (x || y || key)
dev-idlecount=0;
if (!x !y !key)
{
dev-idlecount++;
if (dev-idlecount == 10) {
dev-valid = 0;
schedule_work(dev-work);
}
}
Yes, I think you are right. I guess one could trigger an extra reset
by pressing the button repeatedly witout touching the pad. But because
we won't do reset while the button is pressed we'll never lose release
event.
I guess we want to fix it but it can wait for 2.6.24. A patch woudl be
appreciated.
Thanks!
--
Dmitry
Re: [linux-usb-devel] detecting when keys are being pressed on a keyboard
Hi Oliver, On 9/24/07, Oliver Neukum [EMAIL PROTECTED] wrote: Am Freitag 21 September 2007 schrieb Jiri Kosina: On Fri, 21 Sep 2007, Oliver Neukum wrote: I got the strange case of a keyboard going into autosuspend while a key was being pressed. The key release never arrived and I had to unplug the keyboard to get the key unstuck. Is there a clean way to learn from hid-core.c if a keyboard has keys pressed? Hi Oliver, HID doesn't keep any permanent state by itself. If you want to know whether a given key is currently pressed or not, you'd have to inspect the bitfields inside input_dev*, I am afraid. I see no way to do this without a race condition. The field isn't locked as far as I can tell. You can take input_dev-event_lock to stop event from propagating through input core while you are evaluating the bits. Input lcoking changes are im -mm and will be merged into 2.6.24. Secondly, what is to happen in case of a system wide suspend? Will the system wake up thinking that the key is still pressed? For now it will but it is about to change. Input device will send 'release' events for all pressed keys from their suspend method. -- Dmitry
Re: [linux-usb-devel] detecting when keys are being pressed on a keyboard
On 9/24/07, Oliver Neukum [EMAIL PROTECTED] wrote: Am Montag 24 September 2007 schrieb Dmitry Torokhov: Hi Oliver, On 9/24/07, Oliver Neukum [EMAIL PROTECTED] wrote: Am Freitag 21 September 2007 schrieb Jiri Kosina: Hi Oliver, HID doesn't keep any permanent state by itself. If you want to know whether a given key is currently pressed or not, you'd have to inspect the bitfields inside input_dev*, I am afraid. I see no way to do this without a race condition. The field isn't locked as far as I can tell. You can take input_dev-event_lock to stop event from propagating through input core while you are evaluating the bits. Input lcoking changes are im -mm and will be merged into 2.6.24. Hi Dmitry, I was thinking about a second approach. As all keypresses run through the interrupt handlers of the hid driver, how about checking the bit field in the interrupt handler after calling hid_input_report() ? That should work as well I think. -- Dmitry
Re: Handling touchscreen buttons
On Monday 24 September 2007, Kristoffer Ericson wrote: Greetings, On my jornada7xx I have 4 buttons on the rightside of the screen (going from Y-low - Y-high). They are part of the touchscreen. Since they aren't usually used in normal X window handling I was thinking that I should have the driver detect if the touch happend on one of those buttons. The general idea is that I could somehow export detection into /sys so that ordinary applications could make use of them. Example would be that touching one button could be scripted to start an xterm or anything else. Im interested to know if anyone has any examples or suggestion how to best accomplish this? I'd just send KEY_PROG1 .. KEY_PROG4 events through the same input device you send the rest of touchscreen events. -- Dmitry
Re: [PATCH] xpad: fix Kconfig to avoid compile error
On 9/19/07, Andreas Herrmann [EMAIL PROTECTED] wrote: On Tue, Sep 18, 2007 at 01:52:11PM -0400, Dmitry Torokhov wrote: To avoid this kernel configuration, do auto-select LEDS_CLASS for JOYSTCK_XPAD_LEDS. I'd rather it depend on LEDS_CLASS (but properly). I believe I posted a patch yesterday. Didn't see this as I am not subscribed to one of the input or joystick mailing lists (and didn't find any archive for that lists either). So today I have checked your input.git tree on git.kernel.org. Your patch (commit be1945c47248fa9c0a7cf4a04c1f4c32928656d2) works fine if you rename LED_CLASS to LEDS_CLASS ... So better fix that typo before your tree is pulled into Linus' tree. ;-) Oops, thank you ;) -- Dmitry
Re: [HP7XX/PATCH] - HP720 Jornada Touchscreen (Updated, please use this one)
On 9/20/07, Kristoffer Ericson [EMAIL PROTECTED] wrote:
+ if (jornada_ssp_inout(GETTOUCHSAMPLES == TXDUMMY)) {
Are you sure? Looks like paren is pisplaced.
+
+ ret = request_irq(IRQ_GPIO9,
+ jornada720_ts_interrupt,
+ IRQF_DISABLED | IRQF_TRIGGER_RISING,
+ HP7XX Touchscreen driver, jornada_ts);
+ if (ret) {
+ printk(KERN_INFO HP7XX TS : Unable to aquire irq!\n);
+ goto failed2;
+ }
+
+ if (input_register_device(input_dev))
+ goto failed2;
You will leak IRQ_GPIO9 if input_register_device fails.
Please do not forget adding Signed-off-by: to all of your patches so I
don;t have to hunt for them through old e-mails. Thanks!
--
Dmitry
Re: sysfs change of input/event devices in 2.6.23rc breaks udev
On 9/10/07, Greg KH [EMAIL PROTECTED] wrote: On Mon, Sep 10, 2007 at 01:28:47AM -0400, Dmitry Torokhov wrote: On Sunday 09 September 2007 19:03, Kay Sievers wrote: On 9/8/07, Anssi Hannula [EMAIL PROTECTED] wrote: However, the change that broke id_path of udev is that /sys/class/input/event5/device is now a symlink to the inputX directory instead of being the same as the device symlink in inputX directory, i.e. to ../../../devices/platform/pcspkr in this case. Udev id_path uses that directory to construct the ID_PATH variable. Should the sysfs structure be reverted or should udev be adapted to handle traversing /device symlink twice? I think the former, as there should be considerably more time to adapt udev for coming changes in sysfs. Udev's path_id script is too dumb to follow the device link of stacked class devices in the CONFIG_SYSFS_DEPRECATED=y layout. Does this change fix it for you? http://git.kernel.org/?p=linux/hotplug/udev.git;a=commitdiff_plain;h=b1ac36ff5e3756cefc79967a26280056da31bf6f Hmm, fixing udev is good but users will not get the change in time. I think we need to adjust SYSFS_DEPRECATED code to produce old results. Something like the patch below. I wonder what Greg would think... Hm, I don't understand. Didn't the original conversion of the input layer by Kay not have this kind of problem? What did your changes do differently to cause this driver core change to be needed? If I understand it correctly Kay's convesion had the same issue. With class devices device link points to class_dev-device instead of class_dev-parent. If you want to keep compatibility with old sysfs layout when moving from class devices to regular devices then you need to skip couple of parents till you get to real device. This only matters for input because this was the only subsystem with class devices stacked. -- Dmitry
Re: [2.6 patch] make the dummy touchkit_ps2_detect() static
Hi Adrian, On Tuesday 14 August 2007 17:22, Adrian Bunk wrote: The dummy touchkit_ps2_detect() for the CONFIG_MOUSE_PS2_TOUCHKIT=n case shouldn't be a global function. Applied, thank you. Btw, sorry for the long silence - I had a hard drive crash and the day after I restored everything the disk controller decided to die as well and write garbage all over the new disk ;( -- Dmitry
Re: [PATCH] Try harder to probe finicky rodents
On Sunday 22 July 2007 13:24, Alon Ziv wrote: My rodent appears to be extra-finicky, and requires both a PSMOUSE_RESET_DIS and a PSMOUSE_RESET_BAT before it is unconfused enough to be probed. Signed-off-by: Alon Ziv [EMAIL PROTECTED] Applied, thank you. -- Dmitry
Re: [patch 1/3] m68k/mac: Make mac_hid_mouse_emulate_buttons() declaration visible
On Sunday 22 July 2007 08:51, Geert Uytterhoeven wrote: On Sun, 22 Jul 2007, Dmitry Torokhov wrote: On Saturday 21 July 2007 04:27, Geert Uytterhoeven wrote: On Fri, 20 Jul 2007, Dmitry Torokhov wrote: I am OK with adding a new header file. I was just saying that placing that declaration in linux/hid.h makes about the same sense as putting it into linux/scsi.h At first I just wanted to move it. Then I thought about the angry comments I would get about not moving it to a header file ;-) linux/hid.h looked like the best candidate. linux/kbd_kern.h is another option. linux/kbd_kern.h sounds much better. And so it will be. Applied to 'for-linus' branch of input tree, thank you. -- Dmitry
Re: [PATCH][08/37] Clean up duplicate includes in drivers/input/
On Saturday 21 July 2007 11:02, Jesper Juhl wrote: Hi, This patch cleans up duplicate includes in drivers/input/ Applied to for-linus branch of input tree, thank you. -- Dmitry
[RFC/RFT 5/5] Input: joydev - implement proper locking
Input: joydev - implement proper locking
Signed-off-by: Dmitry Torokhov [EMAIL PROTECTED]
---
drivers/input/joydev.c | 745 -
1 files changed, 493 insertions(+), 252 deletions(-)
Index: work/drivers/input/joydev.c
===
--- work.orig/drivers/input/joydev.c
+++ work/drivers/input/joydev.c
@@ -43,6 +43,8 @@ struct joydev {
struct input_handle handle;
wait_queue_head_t wait;
struct list_head client_list;
+ spinlock_t client_lock; /* protects client_list */
+ struct mutex mutex;
struct device dev;
struct js_corr corr[ABS_MAX + 1];
@@ -61,31 +63,61 @@ struct joydev_client {
int head;
int tail;
int startup;
+ spinlock_t buffer_lock; /* protects access to buffer, head and tail */
struct fasync_struct *fasync;
struct joydev *joydev;
struct list_head node;
};
static struct joydev *joydev_table[JOYDEV_MINORS];
+static DEFINE_MUTEX(joydev_table_mutex);
static int joydev_correct(int value, struct js_corr *corr)
{
switch (corr-type) {
- case JS_CORR_NONE:
- break;
- case JS_CORR_BROKEN:
- value = value corr-coef[0] ? (value corr-coef[1]
? 0 :
- ((corr-coef[3] * (value - corr-coef[1]))
14)) :
- ((corr-coef[2] * (value - corr-coef[0]))
14);
- break;
- default:
- return 0;
+
+ case JS_CORR_NONE:
+ break;
+
+ case JS_CORR_BROKEN:
+ value = value corr-coef[0] ? (value corr-coef[1] ? 0 :
+ ((corr-coef[3] * (value - corr-coef[1])) 14)) :
+ ((corr-coef[2] * (value - corr-coef[0])) 14);
+ break;
+
+ default:
+ return 0;
}
return value -32767 ? -32767 : (value 32767 ? 32767 : value);
}
-static void joydev_event(struct input_handle *handle, unsigned int type,
unsigned int code, int value)
+static void joydev_pass_event(struct joydev_client *client,
+ struct js_event *event)
+{
+ struct joydev *joydev = client-joydev;
+
+ /*
+* IRQs already disabled, just acquire the lock
+*/
+ spin_lock(client-buffer_lock);
+
+ client-buffer[client-head] = *event;
+
+ if (client-startup == joydev-nabs + joydev-nkey) {
+ client-head++;
+ client-head = JOYDEV_BUFFER_SIZE - 1;
+ if (client-tail == client-head)
+ client-startup = 0;
+ }
+
+ spin_unlock(client-buffer_lock);
+
+ kill_fasync(client-fasync, SIGIO, POLL_IN);
+}
+
+static void joydev_event(struct input_handle *handle,
+unsigned int type, unsigned int code, int value)
{
struct joydev *joydev = handle-private;
struct joydev_client *client;
@@ -93,39 +125,32 @@ static void joydev_event(struct input_ha
switch (type) {
- case EV_KEY:
- if (code BTN_MISC || value == 2)
- return;
- event.type = JS_EVENT_BUTTON;
- event.number = joydev-keymap[code - BTN_MISC];
- event.value = value;
- break;
-
- case EV_ABS:
- event.type = JS_EVENT_AXIS;
- event.number = joydev-absmap[code];
- event.value = joydev_correct(value, joydev-corr +
event.number);
- if (event.value == joydev-abs[event.number])
- return;
- joydev-abs[event.number] = event.value;
- break;
+ case EV_KEY:
+ if (code BTN_MISC || value == 2)
+ return;
+ event.type = JS_EVENT_BUTTON;
+ event.number = joydev-keymap[code - BTN_MISC];
+ event.value = value;
+ break;
- default:
+ case EV_ABS:
+ event.type = JS_EVENT_AXIS;
+ event.number = joydev-absmap[code];
+ event.value = joydev_correct(value,
+ joydev-corr[event.number]);
+ if (event.value == joydev-abs[event.number])
return;
+ joydev-abs[event.number] = event.value;
+ break;
+
+ default:
+ return;
}
event.time = jiffies_to_msecs(jiffies);
- list_for_each_entry(client, joydev-client_list, node) {
-
- memcpy(client-buffer + client-head, event, sizeof(struct
js_event));
-
- if (client-startup == joydev-nabs + joydev-nkey)
- if (client
[RFC/RFT 2/5] evdev - implement proper locking
Input: evdev - implement proper locking
Signed-off-by: Dmitry Torokhov [EMAIL PROTECTED]
---
drivers/input/evdev.c | 719 +-
1 files changed, 476 insertions(+), 243 deletions(-)
Index: work/drivers/input/evdev.c
===
--- work.orig/drivers/input/evdev.c
+++ work/drivers/input/evdev.c
@@ -30,6 +30,8 @@ struct evdev {
wait_queue_head_t wait;
struct evdev_client *grab;
struct list_head client_list;
+ spinlock_t client_lock; /* protects client_list */
+ struct mutex mutex;
struct device dev;
};
@@ -37,39 +39,53 @@ struct evdev_client {
struct input_event buffer[EVDEV_BUFFER_SIZE];
int head;
int tail;
+ spinlock_t buffer_lock; /* protects access to buffer, head and tail */
struct fasync_struct *fasync;
struct evdev *evdev;
struct list_head node;
};
static struct evdev *evdev_table[EVDEV_MINORS];
+static DEFINE_MUTEX(evdev_table_mutex);
-static void evdev_event(struct input_handle *handle, unsigned int type,
unsigned int code, int value)
+static void evdev_pass_event(struct evdev_client *client,
+struct input_event *event)
+{
+ /*
+* Interrupts are disabled, just acquire the lock
+*/
+ spin_lock(client-buffer_lock);
+ client-buffer[client-head++] = *event;
+ client-head = EVDEV_BUFFER_SIZE - 1;
+ spin_unlock(client-buffer_lock);
+
+ kill_fasync(client-fasync, SIGIO, POLL_IN);
+}
+
+/*
+ * Pass incoming event to all connected clients. Note that we are
+ * caleld under a spinlock with interrupts off so we don't need
+ * to use rcu_read_lock() here. Writers will be using syncronize_sched()
+ * instead of synchrnoize_rcu().
+ */
+static void evdev_event(struct input_handle *handle,
+ unsigned int type, unsigned int code, int value)
{
struct evdev *evdev = handle-private;
struct evdev_client *client;
+ struct input_event event;
- if (evdev-grab) {
- client = evdev-grab;
-
- do_gettimeofday(client-buffer[client-head].time);
- client-buffer[client-head].type = type;
- client-buffer[client-head].code = code;
- client-buffer[client-head].value = value;
- client-head = (client-head + 1) (EVDEV_BUFFER_SIZE - 1);
-
- kill_fasync(client-fasync, SIGIO, POLL_IN);
- } else
- list_for_each_entry(client, evdev-client_list, node) {
-
- do_gettimeofday(client-buffer[client-head].time);
- client-buffer[client-head].type = type;
- client-buffer[client-head].code = code;
- client-buffer[client-head].value = value;
- client-head = (client-head + 1) (EVDEV_BUFFER_SIZE
- 1);
-
- kill_fasync(client-fasync, SIGIO, POLL_IN);
- }
+ do_gettimeofday(event.time);
+ event.type = type;
+ event.code = code;
+ event.value = value;
+
+ client = rcu_dereference(evdev-grab);
+ if (client)
+ evdev_pass_event(client, event);
+ else
+ list_for_each_entry_rcu(client, evdev-client_list, node)
+ evdev_pass_event(client, event);
wake_up_interruptible(evdev-wait);
}
@@ -88,38 +104,142 @@ static int evdev_flush(struct file *file
{
struct evdev_client *client = file-private_data;
struct evdev *evdev = client-evdev;
+ int retval;
+
+ retval = mutex_lock_interruptible(evdev-mutex);
+ if (retval)
+ return retval;
if (!evdev-exist)
- return -ENODEV;
+ retval = -ENODEV;
+ else
+ retval = input_flush_device(evdev-handle, file);
- return input_flush_device(evdev-handle, file);
+ mutex_unlock(evdev-mutex);
+ return retval;
}
static void evdev_free(struct device *dev)
{
struct evdev *evdev = container_of(dev, struct evdev, dev);
- evdev_table[evdev-minor] = NULL;
kfree(evdev);
}
+/*
+ * Grabs an event device (along with underlying input device).
+ * This function is called with evdev-mutex taken.
+ */
+static int evdev_grab(struct evdev *evdev, struct evdev_client *client)
+{
+ int error;
+
+ if (evdev-grab)
+ return -EBUSY;
+
+ error = input_grab_device(evdev-handle);
+ if (error)
+ return error;
+
+ rcu_assign_pointer(evdev-grab, client);
+ /*
+* We don't use synchronize_rcu() here because read-size
+* critical section is protected by a spinlock instead
+* of rcu_read_lock().
+*/
+ synchronize_sched();
+
+ return 0;
+}
+
+static int evdev_ungrab(struct evdev *evdev, struct evdev_client *client
[RFC/RFT 4/5] Input: mousedev - implement proper locking
Input: mousedev - implement proper locking
Signed-off-by: Dmitry Torokhov [EMAIL PROTECTED]
---
drivers/input/mousedev.c | 736 +--
1 files changed, 464 insertions(+), 272 deletions(-)
Index: work/drivers/input/mousedev.c
===
--- work.orig/drivers/input/mousedev.c
+++ work/drivers/input/mousedev.c
@@ -61,9 +61,11 @@ struct mousedev {
int open;
int minor;
char name[16];
+ struct input_handle handle;
wait_queue_head_t wait;
struct list_head client_list;
- struct input_handle handle;
+ spinlock_t client_lock; /* protects client_list */
+ struct mutex mutex;
struct device dev;
struct list_head mixdev_node;
@@ -113,108 +115,137 @@ static unsigned char mousedev_imex_seq[]
static struct input_handler mousedev_handler;
static struct mousedev *mousedev_table[MOUSEDEV_MINORS];
+static DEFINE_MUTEX(mousedev_table_mutex);
static struct mousedev *mousedev_mix;
static LIST_HEAD(mousedev_mix_list);
+static void mixdev_open_devices(void);
+static void mixdev_close_devices(void);
+
#define fx(i) (mousedev-old_x[(mousedev-pkt_count - (i)) 03])
#define fy(i) (mousedev-old_y[(mousedev-pkt_count - (i)) 03])
-static void mousedev_touchpad_event(struct input_dev *dev, struct mousedev
*mousedev, unsigned int code, int value)
+static void mousedev_touchpad_event(struct input_dev *dev,
+ struct mousedev *mousedev,
+ unsigned int code, int value)
{
int size, tmp;
enum { FRACTION_DENOM = 128 };
switch (code) {
- case ABS_X:
- fx(0) = value;
- if (mousedev-touch mousedev-pkt_count = 2) {
- size = dev-absmax[ABS_X] - dev-absmin[ABS_X];
- if (size == 0)
- size = 256 * 2;
- tmp = ((value - fx(2)) * (256 *
FRACTION_DENOM)) / size;
- tmp += mousedev-frac_dx;
- mousedev-packet.dx = tmp / FRACTION_DENOM;
- mousedev-frac_dx = tmp - mousedev-packet.dx *
FRACTION_DENOM;
- }
- break;
- case ABS_Y:
- fy(0) = value;
- if (mousedev-touch mousedev-pkt_count = 2) {
- /* use X size to keep the same scale */
- size = dev-absmax[ABS_X] - dev-absmin[ABS_X];
- if (size == 0)
- size = 256 * 2;
- tmp = -((value - fy(2)) * (256 *
FRACTION_DENOM)) / size;
- tmp += mousedev-frac_dy;
- mousedev-packet.dy = tmp / FRACTION_DENOM;
- mousedev-frac_dy = tmp - mousedev-packet.dy *
FRACTION_DENOM;
- }
- break;
+ case ABS_X:
+ fx(0) = value;
+ if (mousedev-touch mousedev-pkt_count = 2) {
+ size = dev-absmax[ABS_X] - dev-absmin[ABS_X];
+ if (size == 0)
+ size = 256 * 2;
+ tmp = ((value - fx(2)) * 256 * FRACTION_DENOM) / size;
+ tmp += mousedev-frac_dx;
+ mousedev-packet.dx = tmp / FRACTION_DENOM;
+ mousedev-frac_dx =
+ tmp - mousedev-packet.dx * FRACTION_DENOM;
+ }
+ break;
+
+ case ABS_Y:
+ fy(0) = value;
+ if (mousedev-touch mousedev-pkt_count = 2) {
+ /* use X size to keep the same scale */
+ size = dev-absmax[ABS_X] - dev-absmin[ABS_X];
+ if (size == 0)
+ size = 256 * 2;
+ tmp = -((value - fy(2)) * 256 * FRACTION_DENOM) / size;
+ tmp += mousedev-frac_dy;
+ mousedev-packet.dy = tmp / FRACTION_DENOM;
+ mousedev-frac_dy = tmp -
+ mousedev-packet.dy * FRACTION_DENOM;
+ }
+ break;
}
}
-static void mousedev_abs_event(struct input_dev *dev, struct mousedev
*mousedev, unsigned int code, int value)
+static void mousedev_abs_event(struct input_dev *dev, struct mousedev
*mousedev,
+ unsigned int code, int value)
{
int size;
switch (code) {
- case ABS_X:
- size = dev-absmax[ABS_X] - dev-absmin[ABS_X];
- if (size == 0)
- size = xres ? : 1
[RFC/RFT 1/5] Input: implement proper locking in input core
Input: implement proper locking in input core
Also add some kerneldoc documentation to input.h
Signed-off-by: Dmitry Torokhov [EMAIL PROTECTED]
---
drivers/input/input.c | 656 --
include/linux/input.h | 112 +++-
2 files changed, 585 insertions(+), 183 deletions(-)
Index: work/drivers/input/input.c
===
--- work.orig/drivers/input/input.c
+++ work/drivers/input/input.c
@@ -17,10 +17,10 @@
#include linux/major.h
#include linux/proc_fs.h
#include linux/seq_file.h
-#include linux/interrupt.h
#include linux/poll.h
#include linux/device.h
#include linux/mutex.h
+#include linux/rcupdate.h
MODULE_AUTHOR(Vojtech Pavlik [EMAIL PROTECTED]);
MODULE_DESCRIPTION(Input core);
@@ -31,167 +31,242 @@ MODULE_LICENSE(GPL);
static LIST_HEAD(input_dev_list);
static LIST_HEAD(input_handler_list);
+/*
+ * input_mutex protects access to both input_dev_list and input_handler_list.
+ * This also causes input_[un]register_device and input_[un]register_handler
+ * be mutually exclusive which simplifies locking in drivers implementing
+ * input handlers.
+ */
+static DEFINE_MUTEX(input_mutex);
+
static struct input_handler *input_table[8];
-/**
- * input_event() - report new input event
- * @dev: device that generated the event
- * @type: type of the event
- * @code: event code
- * @value: value of the event
- *
- * This function should be used by drivers implementing various input devices
- * See also input_inject_event()
- */
-void input_event(struct input_dev *dev, unsigned int type, unsigned int code,
int value)
+static inline int is_event_supported(unsigned int code,
+unsigned long *bm, unsigned int max)
{
- struct input_handle *handle;
+ return code = max test_bit(code, bm);
+}
- if (type EV_MAX || !test_bit(type, dev-evbit))
- return;
+static int input_defuzz_abs_event(int value, int old_val, int fuzz)
+{
+ if (fuzz) {
+ if (value old_val - fuzz / 2 value old_val + fuzz / 2)
+ return value;
- add_input_randomness(type, code, value);
+ if (value old_val - fuzz value old_val + fuzz)
+ return (old_val * 3 + value) / 4;
- switch (type) {
+ if (value old_val - fuzz * 2 value old_val + fuzz * 2)
+ return (old_val + value) / 2;
+ }
- case EV_SYN:
- switch (code) {
- case SYN_CONFIG:
- if (dev-event)
- dev-event(dev, type, code,
value);
- break;
-
- case SYN_REPORT:
- if (dev-sync)
- return;
- dev-sync = 1;
- break;
- }
- break;
+ return value;
+}
- case EV_KEY:
+/*
+ * Pass event through all open handles. This function is called with
+ * dev-event_lock held and interrupts disabled. Because of that we
+ * do not need to use rcu_read_lock() here although we are using RCU
+ * to access handle list.
+ */
+static void input_pass_event(struct input_dev *dev,
+unsigned int type, unsigned int code, int value)
+{
+ struct input_handle *handle = rcu_dereference(dev-grab);
- if (code KEY_MAX || !test_bit(code, dev-keybit) ||
!!test_bit(code, dev-key) == value)
- return;
+ if (handle)
+ handle-handler-event(handle, type, code, value);
+ else
+ list_for_each_entry_rcu(handle, dev-h_list, d_node)
+ if (handle-open)
+ handle-handler-event(handle,
+ type, code, value);
+}
- if (value == 2)
- break;
+/*
+ * Generate software autorepeat event. Note that we take
+ * dev-event_lock here to avoid racing with input_event
+ * which may cause keys get stuck.
+ */
+static void input_repeat_key(unsigned long data)
+{
+ struct input_dev *dev = (void *) data;
- change_bit(code, dev-key);
+ spin_lock_irq(dev-event_lock);
- if (test_bit(EV_REP, dev-evbit)
dev-rep[REP_PERIOD] dev-rep[REP_DELAY] dev-timer.data value) {
- dev-repeat_key = code;
- mod_timer(dev-timer, jiffies +
msecs_to_jiffies(dev-rep[REP_DELAY]));
- }
+ if (test_bit(dev-repeat_key, dev-key)
+ is_event_supported(dev-repeat_key, dev-keybit, KEY_MAX
Re: [RFC/RFT 1/5] Input: implement proper locking in input core
Hi Jeff, On Tuesday 24 July 2007 01:35, Jeff Garzik wrote: spin_lock_irq() should generally be avoided. In cases like the first case -- input_repeat_key() -- you are making incorrect assumptions about the state of interrupts. The other cases are probably ok, but in general spin_lock_irq() has a long history of being very fragile and quite often wrong. Use spin_lock_irqsave() to be safe. Definitely in input_repeat_key(), but I strongly recommend removing spin_lock_irq() from all your patches here. Thasnk you for looking at the patches. Actually I went back and forth between spin_lock_irq and spin_lock_irqsave.. I will change back to irqsave version, it is indeed safer. -- Dmitry
Re: [patch 1/3] m68k/mac: Make mac_hid_mouse_emulate_buttons() declaration visible
Hi Geert, On Saturday 21 July 2007 04:27, Geert Uytterhoeven wrote: On Fri, 20 Jul 2007, Dmitry Torokhov wrote: I am OK with adding a new header file. I was just saying that placing that declaration in linux/hid.h makes about the same sense as putting it into linux/scsi.h At first I just wanted to move it. Then I thought about the angry comments I would get about not moving it to a header file ;-) linux/hid.h looked like the best candidate. linux/kbd_kern.h is another option. linux/kbd_kern.h sounds much better. -- Dmitry
Re: [PATCH] HP Jornada 7xx keyboard support
Hi Kristoffer,
On Saturday 21 July 2007 20:48, Kristoffer Ericson wrote:
Greetings,
You got the description text before right?.
Here's the signoff line:
signed-off-by: Kristoffer Ericson [EMAIL PROTECTED]
I must say I am really pissed off. The code that you send not only was
never tested on a real hardware, it was not even compiled once. I see
a stream of missing parens, wrongly named variables, etc, etc, starting
with rev 2 of your patch and going into rev 4 (the last one).
I tried to fix all the issues I found, please make sure the code
compiles and _works_ before resubmitting. I will not be able to
apply it at the time anyway because jornada ssp pieces have not been
merged yet.
I am a bit concerned with -ETIMEOUT error code. It is not a standard error,
is it something that is going to be added to arm arch code?
--
Dmitry
Subject: HP Jornada 7xx keyboard support
From: Kristoffer Ericson [EMAIL PROTECTED]
Input: HP Jornada 7xx keyboard driver
Signed-off-by: Kristoffer Ericson [EMAIL PROTECTED]
Signed-off-by: Dmitry Torokhov [EMAIL PROTECTED]
---
drivers/input/keyboard/Kconfig |7 +
drivers/input/keyboard/Makefile |1
drivers/input/keyboard/jornada720_kbd.c | 173
3 files changed, 181 insertions(+)
Index: work/drivers/input/keyboard/Kconfig
===
--- work.orig/drivers/input/keyboard/Kconfig
+++ work/drivers/input/keyboard/Kconfig
@@ -68,6 +68,13 @@ config KEYBOARD_ATKBD_RDI_KEYCODES
right-hand column will be interpreted as the key shown in the
left-hand column.
+config KEYBOARD_JORNADA720
+tristate HP 720 Keyboard Driver
+depends on SA1100_JORNADA720_SSP SA1100_SSP
+help
+ Say Y here to add support for the HP Jornada 7xx (710/720/728) onboard
+ keyboard. Its generally a good idea.
+
config KEYBOARD_SUNKBD
tristate Sun Type 4 and Type 5 keyboard
select SERIO
Index: work/drivers/input/keyboard/Makefile
===
--- work.orig/drivers/input/keyboard/Makefile
+++ work/drivers/input/keyboard/Makefile
@@ -21,4 +21,5 @@ obj-$(CONFIG_KEYBOARD_OMAP) += omap-key
obj-$(CONFIG_KEYBOARD_PXA27x) += pxa27x_keyboard.o
obj-$(CONFIG_KEYBOARD_AAED2000)+= aaed2000_kbd.o
obj-$(CONFIG_KEYBOARD_GPIO)+= gpio_keys.o
+obj-$(CONFIG_KEYBOARD_JORNADA720) += jornada720_kbd.o
Index: work/drivers/input/keyboard/jornada720_kbd.c
===
--- /dev/null
+++ work/drivers/input/keyboard/jornada720_kbd.c
@@ -0,0 +1,173 @@
+/*
+ * drivers/input/keyboard/jornada720_kbd.c
+ *
+ * HP Jornada 720 keyboard platform driver
+ *
+ * Copyright (C) 2006/2007 Kristoffer Ericson [EMAIL PROTECTED]
+ *Copyright (C) 2006 jornada 720 kbd driver by Filip Zyzniewsk [EMAIL
PROTECTED]
+ * based on (C) 2004 jornada 720 kbd driver by Alex Lange [EMAIL
PROTECTED]
+ *
+ * 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/device.h
+#include linux/init.h
+#include linux/interrupt.h
+#include linux/init.h
+#include linux/input.h
+#include linux/kernel.h
+#include linux/module.h
+#include linux/platform_device.h
+
+#include asm/arch/jornada720.h
+#include asm/hardware.h
+
+MODULE_AUTHOR(Kristoffer Ericson [EMAIL PROTECTED]);
+MODULE_DESCRIPTION(HP Jornada 720 keyboard driver);
+MODULE_LICENSE(GPL);
+
+static unsigned char jornada_standard_keymap[128] = {
/* ROW */
+ 0, KEY_ESC, KEY_F1, KEY_F2, KEY_F3, KEY_F4, KEY_F5, KEY_F6, KEY_F7,
/* #1 */
+ KEY_F8, KEY_F9, KEY_F10, KEY_F11, KEY_VOLUMEUP, KEY_VOLUMEDOWN,
KEY_MUTE, /* - */
+ 0, KEY_1, KEY_2, KEY_3, KEY_4, KEY_5, KEY_6, KEY_7, KEY_8, KEY_9,
/* #2 */
+ KEY_0, KEY_MINUS, KEY_EQUAL,0, 0, 0,
/* - */
+ 0, KEY_Q, KEY_W, KEY_E, KEY_R, KEY_T, KEY_Y, KEY_U, KEY_I, KEY_O,
/* #3 */
+ KEY_P, KEY_BACKSLASH, KEY_BACKSPACE, 0, 0, 0,
/* - */
+ 0, KEY_A, KEY_S, KEY_D, KEY_F, KEY_G, KEY_H, KEY_J, KEY_K, KEY_L,
/* #4 */
+ KEY_SEMICOLON, KEY_LEFTBRACE, KEY_RIGHTBRACE, 0, 0, 0,
/* - */
+ 0, KEY_Z, KEY_X, KEY_C, KEY_V, KEY_B, KEY_N, KEY_M, KEY_COMMA,
/* #5 */
+ KEY_DOT, KEY_KPMINUS, KEY_APOSTROPHE, KEY_ENTER, 0, 0,0,
/* - */
+ 0, KEY_TAB, 0, KEY_LEFTSHIFT, 0, KEY_APOSTROPHE, 0, 0, 0, 0,
/* #6 */
+ KEY_UP, 0, KEY_RIGHTSHIFT, 0, 0, 0,0, 0, 0, 0, 0, KEY_LEFTALT,
KEY_GRAVE, /* - */
+ 0, 0, KEY_LEFT, KEY_DOWN, KEY_RIGHT, 0, 0, 0, 0,0
Re: [HP Jornada 6XX] - Onboard Keyboard support
On Saturday 21 July 2007 21:15, Kristoffer Ericson wrote: Greetings, Here we go again with another keyboard patch (this time for hp6xx). It consists of three files, where one contains the generic scan_keyb routines with needed header and the third is the specific hp6xx driver. Please do not use scan_keyb. It goes around input core and directly into legacy keyboard driver. For drivers that periodically poll/scan their device input_polldev might be of interest. -- Dmitry
Re: [patch 1/3] m68k/mac: Make mac_hid_mouse_emulate_buttons() declaration visible
Hi Geert, On 7/20/07, Geert Uytterhoeven [EMAIL PROTECTED] wrote: From: Geert Uytterhoeven [EMAIL PROTECTED] m68k/mac: Make mac_hid_mouse_emulate_buttons() declaration visible drivers/char/keyboard.c: In function 'kbd_keycode': drivers/char/keyboard.c:1142: error: implicit declaration of function 'mac_hid_mouse_emulate_buttons' The forward declaration of mac_hid_mouse_emulate_buttons() is not visible on m68k because it's hidden in the middle of a big #ifdef block. Move it to linux/hid.h, correct the type of the second parameter, and include linux/hid.h where needed. linux/hid.h contains definitions needed for drivers speaking HID protocol, I don't think we want to put quirks for legacy keyboard driver there. I'd just move the #ifdef within drivers/char/keyboard.c for now. BTW, I don't think that mac button emulation will work well when x86 evdev-based driver gains popularity - it grabs the device and so no event will flow through keyboard driver... We'd need a new solution... -- Dmitry
Re: [PATCH] USB: driver for CM109 chipset
On 7/12/07, Vojtech Pavlik [EMAIL PROTECTED] wrote: On Thu, Jul 12, 2007 at 12:22:10PM -0400, Dmitry Torokhov wrote: Hmm, they use KEY_0 through KEY_9 now. Which results in the phone sending 'é+ěščřžýáí' instead of '0123456789' on a Czech keyboard, which is definitely not what's intended. Similarly for many other European keyboards. Hmm, I uttely confused. Why when atkbd emits KEY_0 it you get 0 in the shell (don't you?) but different result with phone keypad? No, on a Czech keyboard you don't. You press KEY_0, you get 'é'. The French layout gives sililar results. (For '0' you need to press KEY_SHIFT KEY_0.) Oh, I completely forgot that there are keyboards that have numbers on upper register. I think I used such keymap on Yamaha MSX2 in high school... 20 years ago..? But the keypad only works wif you have NumLock on right? -- Dmitry
Re: [PATCH] HP Jornada 7xx keyboard support
Hi Kristoffer,
On 7/18/07, Kristoffer Ericson [EMAIL PROTECTED] wrote:
Try #2
Thanks for feedback, Ive tried a different approach to get it better handled.
Thank you for making the changes I requested, however there is still an issue:
+
+ ret = request_irq(IRQ_GPIO0,
+ jornada720_kbd_interrupt,
+ IRQF_DISABLED | IRQF_TRIGGER_FALLING,
+ jornadakbd, input_dev);
+ if (ret) {
+ printk(KERN_WARNING jornadakbd : Unable to grab IRQ\n);
+ goto failed;
+ }
+
+ err = input_register_device(jornadakbd-input);
+ if (err)
+ goto failed;
+
Here if input_register_device() fails you free the memory but forget
to free IRQ.
--
Dmitry
Re: [PATCH] HP Jornada 7xx keyboard support
On 7/21/07, Kristoffer Ericson [EMAIL PROTECTED] wrote: Greetings, Ive added it to free IRQ as you said, a minor change is also that jornada720.h defines are set in CAPS (just changed that for Russell). Ive also added the Kconfig and Makefile. Btw, do you keep patchtracker (like Russell) or drag from mail (like Paul)? Get from the mail. + +failed: + free_irq(IRQ_GPIO0, input_dev); Still not quire right - you may get to failed: when memory allocation fails or request_irq fails. We should not free IRQ that we did not get. Please split into 2 labels and jump accordingly. -- Dmitry
Re: [PATCH] input: change SysRq keycode for systems without SysRq key
On 7/19/07, federico ferri [EMAIL PROTECTED] wrote: Dmitry Torokhov ha scritto: # echo 84 183 | keyfuzz -s -d/dev/input/event1 EVIOCGKEYCODE: Invalid argument # echo 84 183 | keyfuzz -s -d/dev/input/event2 EVIOCGKEYCODE: Invalid argument # echo 0x05d 0x0b7 | keyfuzz -s -d/dev/input/event2 EVIOCGKEYCODE: Invalid argument What is 84? For SUB you map a usage to a keycode and regular key usages start with 0x0007... 84 == 0x5d (KEY_SYSRQ) 183 is the code generated from my keyboard when I press F13 Ahh, I see. But when you press F13 hardware does not generate 183. USB generates some big number (below), PS/2 generates sequence like 0x5d (not sure) and keyboard connected to a serial port generates something else. Keyfuzz needs scancode in the format that is driver-specific. I don't get what usage means here; USB's scancode. my keyboard has no fancy/multimedia keys (except 4 buttons_ volumeup, volume-down, mute, eject, but those belong to another event device) the keyfuzz manual page says: The scancode/keycode translation tables as read from STDIN [...]. All other lines have to contain a scancode and a keycode number separated by white space. The numbers may be specified either in decimal or in hexadecimal notation. [...] I typed my commands with the above in mind. anyway, if I try to load the bundled translation tables (r.g. the default one) to my event device, I get the same error. That is undersandable because your keyboard is difefrent and does not use the same scancodes as AT keyboard. do you see some solution? perhaps can you supply an example of re-mapping F13 to SysRq? KEY_F13 is normally assigned to usage 0x00070068. Since you want your F13 to be used as SysRq you need map that usage to KEY_SYSRQ. Please try doing: echo 458856 84 | keyfuzz -s -d/dev/input/eventX -- Dmitry
Re: [PATCH 2/3] leds-clevo-mail: hw accelerated LED blink extension
On 7/17/07, Németh Márton [EMAIL PROTECTED] wrote: +Hardware accelerated blink of LEDs +== + +Some LEDs can be programmed to blink without any CPU interaction. To +support this feature, a LED driver can optionally implement the +blink_set() function (see linux/leds.h). If implemeted, the +ledtrig-timer tries to use it. The blink_set() function should return +1 if the blink setting is supported, or 0 otherwise, which means that +the LED will be turned on and off from software. Normally object methods in kernel return 0 on success and error code (such as -EINVAL for unsupported parameters) on error. -- Dmitry
Re: [PATCH] input: change SysRq keycode for systems without SysRq key
Hi Linus, On 7/15/07, Linus Torvalds [EMAIL PROTECTED] wrote: On Mon, 16 Jul 2007, federico ferri wrote: Linus Torvalds ha scritto: Well, this is totally untested, and I won't guarantee that this works at all, but this is how to generally do these kinds of things.. YAY! it works great. tried with: # echo 183 /sys/module/keyboard/parameters/sysrq_key but also keyboard.sys_rq=183 on the command line should work; I'll discover that on next reboot. Yes, please do verify. thank you, Linus! You're welcome. (now that this has done in the proper way, is this patch going to be merged in the tree?) I'm certainly ok with it, but I guess it's more up to Dmitry. If he ack's it, I can just commit it. Dmitry? Any reason not to do this? Recent kernels have the ability to remap keymap for USB keyboards via EVIOCSKEYCODE ioctl (we allowed 0adjusting keymaps on PS/2 keyboards for a long time). So instead of having the new parameter redefining SysRq keycode Frederico can remap one of the keys on his keyboard to generate KEY_SYSRQ. This way SysRq should still work if he plugs in another USB keyboard that has SysRq key or a PS/2 keyboard. -- Dmitry
Re: Keyboard problem
Hi Natalie, On 7/15/07, Natalie Protasevich [EMAIL PROTECTED] wrote: Hello, We have a problem with keybords here. It only hits several systems out of many, and really impossible to reproduce because it happens on different combinations of hardware/software and no one managed to find out what triggers it. I had it on my workstation for a while, then it was gone (was as of Friday when I hit it again). Keyboard stops responding while mouse is still functional, if you hold a key for a few seconds it reacts and displays a key (several of them). If I hold down well ctrl-alt-F1 then I'm able to switch to text screen and keyboard is just fine there. So it happens while X is running, seems more likely when memory intensive apps are running (firefox, kernel builds). Doesn't depend on type of videocard, whether it is double or single monitor configuration, type of mouse (USB or PS2 - but our HW guys said the systems have PS2 connector through the USB bridge) - nothing seems to matter only presence of X. Restarting X fixes a problem, but it is still very disruptive to everyone affected. It seems like Linux input driver, USB and X may be involved. What can be done to diagnose this problem? There is nothing in the logs, only couple messages in X log which tell that screen saver was run a couple times and mysterious message about Screen 0 and 1 now sharing resources. Do we need USB sniffer to trace traffic? Anything we can do to get information from the kernel? Maybe instrumenting keyboard driver and Xorg keyboard library somehow, use kprobes. Any recommendations? What keyboard driver are you using in X? Isit the standard keyboard driver or xf86-input-evdev? If the former you can try loading evbug module (which will dump all input events into syslog) and wait for the condition to appear. Then connect to the box remotely (ssh/telnet) and see if keypresses still make to syslog. If they do that means that USB/HID and input core work fine and the propblem lies in the upper layers. -- Dmitry
Re: Force Feedback: Thrustmaster FGT Wheel quick-and-dirty in hid-lgff.c or hid-tmff.c
On 7/16/07, Dmitry Torokhov [EMAIL PROTECTED] wrote: OK, how about this then? Ignore it, it missing couple of bits; Anssi's is more complete I think. -- Dmitry
Re: Force Feedback: Thrustmaster FGT Wheel quick-and-dirty in hid-lgff.c or hid-tmff.c
On 7/16/07, Dmitry Torokhov [EMAIL PROTECTED] wrote: On 7/16/07, Dmitry Torokhov [EMAIL PROTECTED] wrote: OK, how about this then? Ignore it, it missing couple of bits; Anssi's is more complete I think. Not quite what I wanted either, what about this one? -- Dmitry hid-add-thrustmaster-fgt-wheel.patch Description: Binary data
Re: [PATCH] Input: Support for a less exclusive grab.
Hi Zephaniah, On Saturday 09 June 2007 04:48, Zephaniah E. Hull wrote: EVIOCGRAB is nice and very useful, however over time I've gotten multiple requests to make it possible for applications to get events straight from the event device while xf86-input-evdev is getting events from the same device. Here is the least invasive patch I could think of, it changes the behavior of EVIOCGRAB in some cases, specificly behavior is identical if the argument is 0 or 1, however if the argument is true and != 1, then it does a 'non exclusive grab', a better name might be handy. What this does is allow the events to go to everything that's using evdev to get events, but grabs it from anything else. About as close to what people want as I can get, and fairly non-invasive. Unfortunately this also robs non-legacy input handlers (such as rfkill-input) of input events. Does xf86-input-evdev really needs to grab devices exclusively? I guess we can't abandon the standard keyboard driver until X supports hotplugging. How close is it to support devices coming and going? If we can't remain as is until X hotplug is ready then I'd rather had a separate ioctl that disables legacy input handlers (keyboard, mousedev) for a given input device. -- Dmitry
Re: [PATCH] Input: Support for a less exclusive grab.
On Tuesday 12 June 2007 01:12, Zephaniah E. Hull wrote: On Tue, Jun 12, 2007 at 01:07:13AM -0400, Dmitry Torokhov wrote: Hi Zephaniah, On Saturday 09 June 2007 04:48, Zephaniah E. Hull wrote: EVIOCGRAB is nice and very useful, however over time I've gotten multiple requests to make it possible for applications to get events straight from the event device while xf86-input-evdev is getting events from the same device. Here is the least invasive patch I could think of, it changes the behavior of EVIOCGRAB in some cases, specificly behavior is identical if the argument is 0 or 1, however if the argument is true and != 1, then it does a 'non exclusive grab', a better name might be handy. What this does is allow the events to go to everything that's using evdev to get events, but grabs it from anything else. About as close to what people want as I can get, and fairly non-invasive. Unfortunately this also robs non-legacy input handlers (such as rfkill-input) of input events. Does xf86-input-evdev really needs to grab devices exclusively? I guess we can't abandon the standard keyboard driver until X supports hotplugging. How close is it to support devices coming and going? Er, to explain. The current EVIOCGRAB does an exclusive grab that prohibits rfkill-input and friends from working. I understand that. As it is the only way to disable the legacy input handlers, xf86-input-evdev has been using it since we added it. Like I said I would love if xf86-input-evdev did not grab the device at all. The patch is to let us cause only things that use /dev/input/eventn to get events, thus, a non-exclusive grab. This basicly disables the legacy input handlers, and it's the least invasive patch I could think of. But rfkill-input is not a legacy handler. My objection is that with your solution you still will rob handlers such rfkill-input of events. Going for a separate ioctl would also work, but in some ways it would make supporting it more of a pain. I don't care _that_ much either way, as long as we can get a way to disable the legacy events while allowing other things to get the events too. Zephaniah E. Hull. If we can't remain as is until X hotplug is ready then I'd rather had a separate ioctl that disables legacy input handlers (keyboard, mousedev) for a given input device. -- Dmitry -- Dmitry
Re: [PATCH] input: make 2 macros in gameport.c TSC-aware
Hi, On Monday 11 June 2007 00:12, Miltiadis Margaronis wrote: This makes DELTA and GET_TIME in drivers/input/gameport/gameport.c similar to the ones in drivers/input/joystick/analog.c . Worked on 2.6.22-rc4-git2. I was told with the introduction of tickless kernels and such the best option is to convert gameport to use hires timers. -- Dmitry
Re: [PATCH] input: fix broken behaviour of Dell Latitude special keys
Hi, On Sunday 10 June 2007 13:42, Giel de Nijs wrote: Hi, Following up on http://thread.gmane.org/gmane.linux.kernel.input/1375 here's a new patch to fix the fact that most Fn+F? special keys on (at least) the Dell Latitude laptops don't generate a key release event. Thank you for the patch. Is there any way I could see data coming from i8042 when you press on these special keys? If you could do: echo 1 /sys/module/i8042/parameters/debug tehn presses and released all these keys echo 0 /sys/module/i8042/parameters/debug and send me dmesg I woudl appreciate that. -- Dmitry
Re: [PATCH] Input: Support for a less exclusive grab.
On Tuesday 12 June 2007 01:23, Zephaniah E. Hull wrote: On Tue, Jun 12, 2007 at 01:19:59AM -0400, Dmitry Torokhov wrote: Like I said I would love if xf86-input-evdev did not grab the device at all. We have to disable the legacy input handlers somehow, not doing so simply isn't an option. I do not follow. If user's xorg.conf does not use /dev/input/mice and does not use kbd driver then grabbing is not required, is it? Now, as far as I understand, lack of hotplug support in X is the main obstacle for removing mouse and kbd drivers, correct? But rfkill-input is not a legacy handler. My objection is that with your solution you still will rob handlers such rfkill-input of events. Urgh. So, any thoughts on how to identify legacy input handlers in the input system? I guess keyboard and mousedev will have to be flagged as such in kernel. -- Dmitry
Re: [PATCH 1/1] gpio_mouse driver (rewritten to GIT kernel)
Hi,
On Thursday 31 May 2007 07:38, Hans-Christian Egtvedt wrote:
This patch adds support for simulating a mouse using GPIO lines.
Thank you for updating the patch. It looks much better now, I see only
couple of issues:
+
+ if (!pdata) {
+ dev_dbg(pdev-dev, no platform data\n);
+ ret = -ENXIO;
I think -EINVAL suits here better.
+
+ /* Mouse direction, required. */
+ ret = gpio_request(pdata-up, gpio_mouse_up);
+ if (ret) {
+ dev_dbg(pdev-dev, fail up pin\n);
+ goto out_gpio_up;
+ }
Repeated gpio requests could be folded together.
+
+ if (input)
+ input_unregister_polled_device(input);
You also need input_free_polled_device(). This is different from regular
input devices becase polled device is not refcounted (but corresponding
input device is).
+
+struct platform_driver gpio_mouse_device_driver = {
+ .remove = __exit_p(gpio_mouse_remove),
I think gpio_mouse_remove shoudl be __devexit so unbinding via sysfs still
works.
I tried implementing my suggestions, the result is the patch below. Please
take a look at it and if you agree with it (and it still works) I will
apply it. Thank you!
--
Dmitry
From: Hans-Christian Egtvedt [EMAIL PROTECTED]
Input: add gpio-mouse driver
Adds support for simulating a mouse using GPIO lines. The driver
needs an appropriate platform device to be created by architecture
code.
The driver has been tested on AT32AP7000 microprocessor using the
ATSTK1000 development board.
Signed-off-by: Hans-Christian Egtvedt [EMAIL PROTECTED]
Signed-off-by: Dmitry Torokhov [EMAIL PROTECTED]
---
drivers/input/mouse/Kconfig | 17 +++
drivers/input/mouse/Makefile |1
drivers/input/mouse/gpio_mouse.c | 196 +++
include/linux/gpio_mouse.h | 61
4 files changed, 275 insertions(+)
Index: work/drivers/input/mouse/Kconfig
===
--- work.orig/drivers/input/mouse/Kconfig
+++ work/drivers/input/mouse/Kconfig
@@ -216,4 +216,21 @@ config MOUSE_HIL
help
Say Y here to support HIL pointers.
+config MOUSE_GPIO
+ tristate GPIO mouse
+ depends on GENERIC_GPIO
+ select INPUT_MISC
+ select INPUT_POLLDEV
+ help
+ This driver simulates a mouse on GPIO lines of various CPUs (and some
+ other chips).
+
+ Say Y here if your device has buttons or a simple joystick connected
+ directly to GPIO lines. Your board-specific setup logic must also
+ provide a platform device and platform data saying which GPIOs are
+ used.
+
+ To compile this driver as a module, choose M here: the
+ module will be called gpio_mouse.
+
endif
Index: work/drivers/input/mouse/Makefile
===
--- work.orig/drivers/input/mouse/Makefile
+++ work/drivers/input/mouse/Makefile
@@ -15,6 +15,7 @@ obj-$(CONFIG_MOUSE_PS2) += psmouse.o
obj-$(CONFIG_MOUSE_SERIAL) += sermouse.o
obj-$(CONFIG_MOUSE_HIL)+= hil_ptr.o
obj-$(CONFIG_MOUSE_VSXXXAA)+= vsxxxaa.o
+obj-$(CONFIG_MOUSE_GPIO) += gpio_mouse.o
psmouse-objs := psmouse-base.o synaptics.o
Index: work/drivers/input/mouse/gpio_mouse.c
===
--- /dev/null
+++ work/drivers/input/mouse/gpio_mouse.c
@@ -0,0 +1,196 @@
+/*
+ * Driver for simulating a mouse on GPIO lines.
+ *
+ * Copyright (C) 2007 Atmel Corporation
+ *
+ * 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/init.h
+#include linux/version.h
+#include linux/module.h
+#include linux/platform_device.h
+#include linux/input-polldev.h
+#include linux/gpio_mouse.h
+
+#include asm/gpio.h
+
+/*
+ * Timer function which is run every scan_ms ms when the device is opened.
+ * The dev input varaible is set to the the input_dev pointer.
+ */
+static void gpio_mouse_scan(struct input_polled_dev *dev)
+{
+ struct gpio_mouse_platform_data *gpio = dev-private;
+ struct input_dev *input = dev-input;
+ int x, y;
+
+ if (gpio-bleft = 0)
+ input_report_key(input, BTN_LEFT,
+ gpio_get_value(gpio-bleft) ^ gpio-polarity);
+ if (gpio-bmiddle = 0)
+ input_report_key(input, BTN_MIDDLE,
+ gpio_get_value(gpio-bmiddle) ^ gpio-polarity);
+ if (gpio-bright = 0)
+ input_report_key(input, BTN_RIGHT,
+ gpio_get_value(gpio-bright) ^ gpio-polarity);
+
+ x = (gpio_get_value(gpio-right) ^ gpio-polarity)
+ - (gpio_get_value(gpio-left) ^ gpio-polarity);
+ y = (gpio_get_value(gpio-down) ^ gpio-polarity
Re: Making KEY_UNKNOWN really useful to userland
On 5/31/07, Henrique de Moraes Holschuh [EMAIL PROTECTED] wrote: On Thu, 31 May 2007, Richard Hughes wrote: I am on it on the thinkpad-acpi side, so at least for that, you don't have to worry. I am still waiting an answer about which event is the correct one to output scancodes, but the thinkpad-acpi GIT tree is already updated with the above, tentively using MSC_SCAN to report scan codes. I thought I sent out email saying MSC_SCAN is what you want to use, MSC_RAW is raw data from device, potentially having make/break codes included/encoded. Apparently my mail broke even more than I thought. Highjacking the thread somewhat - Richard, I saw your patch for toshiba_acpi. Please use input-polldev to set up polled devices. It will create work queue for you and will make sure that polling is stopped when device is closed. Also I don't think you want to use KEY_BREAK. What is the expected function of that key? Please copy me on input-related changes in the tree. Thank you. -- Dmitry
Re: [PATCH] Input: document the proper usage of EV_KEY and KEY_UNKNOWN
On Thursday 31 May 2007 21:44, Matthew Garrett wrote: On Thu, May 31, 2007 at 10:29:28PM -0300, Henrique de Moraes Holschuh wrote: On Fri, 01 Jun 2007, Matthew Garrett wrote: Given existing userspace, it's never useful to generate KEY_UNKNOWN. Adding extra information to the event doesn't alter that. It will not break anything, and it is trivial to write an application to intelligently handle KEY_UNKNOWN+scancode events. This really is not a reason to not do it, at all. It's not trivial at all. You need to introduce a mechanism for noting a KEY_UNKNOWN keypress. It then needs to signal the user (dbus is probably the best layer for this), but you need to ensure that you only signal the user who is currently at the keyboard. This needs to be presented to the user via some sort of UI, which will then need to signal some sort of privileged process to actually change the keymap. Not necessarily priveleged - you most likely already change ownership of event devices to user who is logged at console (so your force feedback joysticks work). When the user logs out, you'll then need to unmap the key again and repeat as necessary for any new user who logs in. I think we should aim at the most common case - when there are no multiple users on the box. Then the utility that detects KEY_UNKNOWN just saves the mapping user chose and automatically reload keymap upon next reboot. Note that KEY_UNKNOWN solution does not preculde futher customization on per-user base once default action is established. Alternatively, we could generate a keycode and then let the user map that to an X keysym. We've even already got code to do this. There is world outside of X. I think using positional keycodes would also be a mistake. We just need a slightly larger set of keycodes representing user-definable keys. There's 4 of them already - I really can't imagine there being many keyboards with a significantly larger set of unlabelled keys. I had this exact PoV, too, until Dmitry reminded me that keycodes are *global* to the system in practice, and that different keys (as in keys that have no correlation between their position, labels or lack thereof, and function) in different input devices would end up mapped to KEY_PROGx by default. That's a ridiculously niche case, and can be handled in userspace. Just have udev do remapping when it detects multiple keyboards that both have KEY_PROG* layers, or let X have different keymaps for different input devices. We shouldn't make the (by far) common case significantly more difficult to deal with this one. No, it is not a niche case. I think it is much more common than the case where you have multiple users for the same box using different keymaps. Even if box is shared there most likely will be one person setting it up in the beginning and the rest will follow his/her setup. -- Dmitry
Re: [PATCH 1/1] gpio_mouse driver
On 5/30/07, Hans-Christian Egtvedt [EMAIL PROTECTED] wrote: On Tue, 2007-05-29 at 11:36 -0400, Dmitry Torokhov wrote: Hi, On 5/29/07, Hans-Christian Egtvedt [EMAIL PROTECTED] wrote: This patch adds support for simulating a mouse using GPIO lines. The driver needs a platform_data struct to be defined and registered with the appropriate platform_device. The driver has been tested on AT32AP7000 microprocessor using the ATSTK1000 development board. It looks sane although I would recommend switching to input-polldev when implementing a polled input device. Oh, I was not aware about this, it seems like just the thing I need. Could it be scheduled for after the official kernel has this included? AFAICT it will be released with 2.6.22? Because merge window for 2.6.22 is closed the driver will only be merged in the mainline when 2.6.23 window opens. For now it will stay in my tree (and because Andrew pulls form me it will also show up in -mm). Because of that I would like to get the driver in shape from the beginning - my tree obviously does have these changes. Thank you. -- Dmitry
Re: [PATCH] new driver for wireless xbox receiver for review
Hi Brain, [Added Jan to CC list] On 5/29/07, Brian Magnuson [EMAIL PROTECTED] wrote: Hi Dimitri, Thanks for taking the time to review. My reponses are inline. -Brian * Dmitry Torokhov [EMAIL PROTECTED] [2007-05-29 20:03]: Thnk you for the patch. Looking at the code I do not see one device supporintg multiple controllers... You have one input device per usb device/interface and the properties of said device are known beforehand. So I would just create and register input device right there in xboxrcvr_probe() and get rid of your build and teardown works. The fact that is it a wireless device and at transmitter may at times be out of range is not important. Actually, I rather liked the fact that the input devices only show up when there is a controller connected to the receiver, since there can be anywhere from 0-4 controllers alive. This way there are no device nodes for non-existant controllers. Let's keep it simple. This way we don't have to worry about races between build and teardown work pieces and the driver is much simplier. After all we don't do dynamic device creation for wireless mice (non blue-tooth). And if we did it would make some programs unhappy if they did not see that device all the time... Even with controller - imagine you are playing and somebody calls you - you walk away with your controller in hand and when you come back it stops working because we tore down one device and created the a one but application is still latched to the old device - not nice. Overall I would like support for this wireless receiver to be incorporated into xpad.c driver. Knew that was coming :). Should be able to manage it. Since the wireless model returns a expanded data format it needs to be handled specially. Another flag in the xpad_device struct? We have xtype field, I think it should be used. -- Dmitry
Re: [PATCH] ACPI: thinkpad-acpi: add thinkpad keys to input.h
On 5/29/07, Henrique de Moraes Holschuh [EMAIL PROTECTED] wrote: On Mon, 28 May 2007, Dmitry Torokhov wrote: On Sunday 27 May 2007 08:15, Henrique de Moraes Holschuh wrote: On Sat, 26 May 2007, Dmitry Torokhov wrote: I am unconvinced that we need new keycodes. Isn't there a better default keycodes for these keys? You mentioned that fn+f5 controls radio on many thinkpads, why don't you use KEY_WLAN in your keymap? No can do the KEY-WLAN thing, sorry. I *don't* have a way to know, unless I add a model-specific map table to the kernel, and I have been told by numerous people to don't even try, unless it is for quirks, etc. Why not? It thinkpad-acpi is a box-specific driver and you could try to setup proper keymap depending on models. We do that in wistron_btns and it doers not even need alot of memory (keymaps and dmi data is marked __initdata and is discarded). Well, I will try, let's see if ACPI upstream will take it after I tell them it was decided to be the best approach by the input layer people. Yes, it can be __initdata, so it should not cause any drawbacks. But I will still need to add keys, and I still think that a bunch of 32 or so HOSTSPECIFIC keys is a very very good idea to have, *even* if I add some model specific knowledge and already remap a few of the hot key keyboard to less generic events where possible. I think that adding anything like HOSTSPECIFIC is a failure on our part. That means that you need to make programs out there aware of multiple hosts and their usage. You can't just say that you going to teach X and the rest will use X facilities because there is world outside of X. Programs like HAL, network management (rfkill) other daemons getting input directly from /dev/input/eventX will have to be made aware. This is not good. I really don't like KEY_FN_F1..KEY_FN_BACKSPACE either. What are they supposed to do? Just being an unique value to be mapped onto something useful? But why not use that useful keycode to begin with? I'd rather leave the keys unmapped and rely on initsripts (possibly with help from distributions vendors) to load proper keymap then add something that must be retranslated over and over again. Really, what are we to do with that input.h scancode map? It *IS* supposed to be absolute, i.e. one is not supposed to reuse keys in there if the functionality *or* the generic description is not an exact match. Are there any markings on those keys? Only a few of them. And the ones I wanted to add are *not* marked in most models :-) The markings change a bit from model to model, and we have a *very* incomplete list of those... Well, what kind of functions you would like them to have? You, as a maintainer, can chose defaults. Since you (well, not you, the driver) provide a way for a user to adjust keymap there should be no problem even if someone does not like the values you chose. Having sensible defaults is a good thing, otherwise many people will not even know that they have these separate keys. Alternatively, if you let me add keys, I will need a few of them, and they are also generic (not necessarily thinkpad-specific). Stuff like: KEY_FN_BACKSPACE, KEY_VENDORHOMEPAGE, And what are their intended functions would be? How KEY_VENDORHOMEPAGE is different from KEY_HOMEPAGE? -- Dmitry
Re: [PATCH] ACPI: thinkpad-acpi: add thinkpad keys to input.h
Hi Matthew, On 5/30/07, Matthew Garrett [EMAIL PROTECTED] wrote: On Wed, May 30, 2007 at 09:57:11AM -0400, Dmitry Torokhov wrote: I really don't like KEY_FN_F1..KEY_FN_BACKSPACE either. What are they supposed to do? Just being an unique value to be mapped onto something useful? But why not use that useful keycode to begin with? We've already got KEY_PROG* - is this not the sort of situation they're for? (ie, keys that aren't mapped to a specific purpose but would be potentially useful to userspace at the per-user level) Right. These are they keys we have no idea how to use these, leave it to the user. Do we really need more of these? We have quite a few codes that might be useful. I just don't want to keep adding a new input keycode every time we encounter an unmarked key somewhere. I'd rather leave the keys unmapped and rely on initsripts (possibly with help from distributions vendors) to load proper keymap then add something that must be retranslated over and over again. Changing the keymap is a privileged operation, so sending /some/ sort of keycode by default would probably be good. It's up to the security policy on a particular box. One could change /dev/input/evdev ownership to the user currently logged on physical console. Overall I think adjusting the keymap at boot time (so per-installation case) will cover most of users. Well, what kind of functions you would like them to have? You, as a maintainer, can chose defaults. Since you (well, not you, the driver) provide a way for a user to adjust keymap there should be no problem even if someone does not like the values you chose. Having sensible defaults is a good thing, otherwise many people will not even know that they have these separate keys. Some of the Thinkpad keys send events even without there being any label, so I don't think there's a sane default other than leaving it up to the user. On the other hand, I'm not especially keen on sending literals like FN_BACKSPACE - it's hugely special-cased. That's why maintainer gets to chose his favorite keymap (from available keycodes ;) ) and anyone who's unhappy with the coise gets to add couple of lines in rc.local. Also there is DMI and possibility to load install different keymaps this way (which Ithink is good for vendor-specific drivers - I woudl not add dmi to atkbd for example because you never know what external keyboard user may plug in). -- Dmitry
Re: [PATCH] ACPI: thinkpad-acpi: add thinkpad keys to input.h
On 5/30/07, Matthew Garrett [EMAIL PROTECTED] wrote: On Wed, May 30, 2007 at 10:18:17AM -0400, Dmitry Torokhov wrote: Hi Matthew, We've already got KEY_PROG* - is this not the sort of situation they're for? (ie, keys that aren't mapped to a specific purpose but would be potentially useful to userspace at the per-user level) Right. These are they keys we have no idea how to use these, leave it to the user. Do we really need more of these? We have quite a few codes that might be useful. I just don't want to keep adding a new input keycode every time we encounter an unmarked key somewhere. Sorry, I wasn't clear - I was thinking that they should just be used for this case, rather than that more of them be added. Ah, OK. Changing the keymap is a privileged operation, so sending /some/ sort of keycode by default would probably be good. It's up to the security policy on a particular box. One could change /dev/input/evdev ownership to the user currently logged on physical console. Most users will be logged into X, so it's the X keymap that's the most interesting one. X tools know how to remap the X keymap without requiring any sort of special privileges, so all we need is for the keycode to generate /something/. I think KEY_PROG* would make the most sense, and that's what we've adopted in Ubuntu. Not all world is X :) Actually few of FN keys, like KEY_WLAN, KEY_SLEEP, etc should be handled not [only] by X but by other layers. -- Dmitry
Re: [PATCH] ACPI: thinkpad-acpi: add thinkpad keys to input.h
On 5/30/07, Henrique de Moraes Holschuh [EMAIL PROTECTED] wrote: On Wed, 30 May 2007, Dmitry Torokhov wrote: On 5/29/07, Henrique de Moraes Holschuh [EMAIL PROTECTED] wrote: But I will still need to add keys, and I still think that a bunch of 32 or so HOSTSPECIFIC keys is a very very good idea to have, *even* if I add some model specific knowledge and already remap a few of the hot key keyboard to less generic events where possible. I think that adding anything like HOSTSPECIFIC is a failure on our part. That means that you need to make programs out there aware of Well, you have to choose one of three possibilities for an unlabelled key: 1. Generate SOMETHING that has an undefined meaning or function, but which is unique for that keyboard (KEY_PROG/KEY_HOSTSPECIFIC) How do you guarantee that KEY_PROG* is unique for the keyboard? What do you do if you have 2 devices generating KEY_PROG1? 2. Generate SOMETHING that has a non-specific function, but a well defined meaning (KEY_FN_F1) And we have plenty of those. 3. Do nothing. Well, probably not nothing. Map it to KEY_UNKNOWN and have userspace listen to such events and inform user when it presses such a key that such and such happened and tell him how to map it to something useful. I *REALLY* do not like (3), and so far I have not seen a single good technical reason to go with it. Reasons: do not require expaning current keymap preserving space for more useful keycodes. From the technically sound ones, you need to either have the keycodes you need for (2) (i.e. KEY_FN_BACKSPACE), or a big enough set of keycodes to use (1) (i.e. KEY_PROG5..KEY_PROGn, where n should probably be at least 8). Why 8? Why not 16? Or 32 just to make sure? I really don't like KEY_FN_F1..KEY_FN_BACKSPACE either. What are they supposed to do? Just being an unique value to be mapped onto something useful? But why not use that useful keycode to begin with? Yes, just an unique value to be mapped onto something useful, by something in userspace. Just like KEY_PROG, actually. In _every_ program that gets events directly? One can't use a useful keycode for two reasons: 1. Because the key has no set function (it is unmarked) 2. Because it has, or could have, a set function, but we have no clue which is it. KEY_UNKNOWN then. I'd rather leave the keys unmapped and rely on initsripts (possibly with help from distributions vendors) to load proper keymap then add something that must be retranslated over and over again. I don't. I can live with it, of course, but if we are going to go that way, what IS the excuse for a big lot of the keys already in input.h? We have been adding the unique keycodes and functional keycodes because it is *useful*. Because they most of them describe an expected _action_ that would happend after keypress. [...skipped...] And what are their intended functions would be? How KEY_VENDORHOMEPAGE is different from KEY_HOMEPAGE? KEY_VENDORHOMEPAGE is exactly that. It is marked with the vendor's name. KEY_HOMEPAGE has a defined function inherited from that other O.S. which is to open up the default browser on the default homepage. I can easily see both keys existing on a system. OK, right now we have: KEY_WWW KEY_VENDOR KEY_HOMEPAGE defines in input.h. Are you sayign that none of these would suit? As for stuff like KEY_FN_BACKSPACE, well, I don't really care, as long as I have *something* unique and not incorrect to use. But if we are not going to add extra KEY_FN_ keycodes, why don't we just convert them all to KEY_PROG, so that they can be useful to all and not just to people who have FN_whatever keys? We could add aliases if you really want... Can you tell me on _your_ thinkpad what markings you have? I mean there should be a common pattern on thinkpads and the rest may be guessed (you mentioned that FN-F5 is used to turn off radio quite often so if thinkpad driver detects radio switch it makes sence to just go ahead and mark FN-F5 as KEY_WLAN, doesn't it?) -- Dmitry
Re: [PATCH 1/1] gpio_mouse driver
Hi, On 5/29/07, Hans-Christian Egtvedt [EMAIL PROTECTED] wrote: This patch adds support for simulating a mouse using GPIO lines. The driver needs a platform_data struct to be defined and registered with the appropriate platform_device. The driver has been tested on AT32AP7000 microprocessor using the ATSTK1000 development board. It looks sane although I would recommend switching to input-polldev when implementing a polled input device. + + input-name = pdev-name; + input-cdev.dev = pdev-dev; Please use input-dev.parent = pdev-dev. Input devices are being moved from class_device to struct device. + input-private = pdata; + + /* +* Revisit: is bustype, vendor, product and version needed to +* input-id? And if they should be present, what values should they +* have? +*/ BUS_HOST seems to be most suitable here. The rest may stay 0. + + /* private */ + struct timer_list timer; +}; I don't think it is a good idea to have timer structure in platform data which should really be constant. Timer shoudl be part of the stucture created when driver binds to a device. I can see you may not want to introduce extra complexity in the driver; however if you use input-polldev it will handle timer for you. -- Dmitry
