On 5/30/06, Anssi Hannula <[email protected]> wrote:
Implement a new force feedback interface, in which all non-driver-specific
operations are separated to a common module. This includes handling effect
type validations, effect timers, locking, etc.
Still looking at it, couple of random points for now...
The code should be built as part of the input module, but unfortunately that
would require renaming input.c, which we don't want to do. So instead we make
INPUT_FF_EFFECTS a bool so that it cannot be built as a module.
I am not opposed to rename input.c, I wonder what pending changes
besides David's header cleanup Andrew had in mind.
@@ -865,6 +865,9 @@ struct input_dev {
unsigned long sndbit[NBITS(SND_MAX)];
unsigned long ffbit[NBITS(FF_MAX)];
unsigned long swbit[NBITS(SW_MAX)];
+
+ struct ff_device *ff;
+ struct mutex ff_lock;
I believe that ff_lock should be part of ff_device and be only used to
controll access when uploading/erasing effects. The teardown process
should make sure that device inactive anyway only then remove
ff_device from input_dev; by that time noone should be able to
upload/erase effects. Therefore ff_lock is not needed to protect
dev->ff.
===================================================================
--- linux-2.6.17-rc4-git12.orig/drivers/input/input.c 2006-05-27 02:28:57.000000000 +0300
+++ linux-2.6.17-rc4-git12/drivers/input/input.c 2006-05-27 02:38:35.000000000 +0300
@@ -733,6 +733,17 @@ static void input_dev_release(struct cla
{
struct input_dev *dev = to_input_dev(class_dev);
+ if (dev->ff) {
+ struct ff_device *ff = dev->ff;
+ clear_bit(EV_FF, dev->evbit);
+ mutex_lock(&dev->ff_lock);
+ del_timer_sync(&ff->timer);
This is too late. We need to stop timer when device gets unregistered.
Clearing FF bits is pointless here as device is about to disappear;
locking is also not needed because we are guaranteed to be the last
user of the device structure.
I wonder if ff should be released right at unregister time...
+ dev->flush = NULL;
+ dev->ff = NULL;
+ mutex_unlock(&dev->ff_lock);
+ kfree(ff);
+ }
+
kfree(dev);
module_put(THIS_MODULE);
}
+static inline int input_ff_safe_lock(struct input_dev *dev)
+{
+ mutex_lock(&dev->ff_lock);
+ if (dev->ff)
+ return 0;
+
+ mutex_unlock(&dev->ff_lock);
+ return 1;
+}
This needs to go away. Users should check whether a device supports FF
and if it is then it is device's responsibility to keep it there until
untregister time. We don't expect FF capabilities to flip/flop on a
live device.
+static void input_ff_calc_timer(struct ff_device *ff)
+{
+ int i;
+ int events = 0;
+ unsigned long next_time = 0;
...
+
+ if (time_after(jiffies, event_time)) {
+ event_time = jiffies;
Should it be next_time = jiffies? We want to schedule thetimer ASAP, right?
+
+/**
+ * abs() with -0x8000 => 0x7fff exception
+ */
+static inline u16 input_ff_unsign(s16 value)
+{
+ if (value == -0x8000)
+ return 0x7fff;
+
+ return (value < 0 ? -value : value);
+}
Why is it needed?
+
+/**
+ * Safe sum
+ * @a: Integer to sum
+ * @b: Integer to sum
+ * @limit: The sum limit
+ *
+ * If @a+@b is above @limit, return @limit
+ */
+static int input_ff_safe_sum(int a, int b, int limit)
+{
+ int c;
+ if (!a)
+ return b;
+ c = a + b;
+ if (c > limit)
+ return limit;
+ return c;
+}
As it was mentioned the result will not be limited if a == 0. Is it intended?
PLease don;t start making any changes yet, I am still looking...
--
Dmitry
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
