[PATCH v2 0/4] Add ff-memless-next and make hid-lg4ff use it

[PATCH v2 0/4] Add ff-memless-next and make hid-lg4ff use it

[Michal Malý]

Hi everybody,

this patch series is a result of my work to improve FFB support for memoryless
devices. ff-memless-next is an improvement over the currently available
ff-memless which is well suited for joypads but cannot handle more advanced
devices such as racing wheels properly. As I have explained in one of RFCs
regarding ff-memless-next, the extent of the changes makes implementing
ff-memless-next as a patch to ff-memless unfeasible. As of now there is a total
of 27 drivers using ff-memless (including lg4ff) - a lot of them joypads.
I do not have access to any FFB joypad at the moment so I cannot
implement the functionality required to handle joypads properly - namely FF_RUMBLE
and emulation of FF_PERIODIC through FF_RUMBLE.
The plan is to implement the missing functionality and replace ff-memless completely
in the future. 

Second part of this series ports lg4ff driver over to ff-memless-next.
The immediate benefit of this is support of all periodic effects and ramp effect.

v2 addresses a few issues that have not been noticed at the time v1 was
submitted. Specific fixes are mentioned in the respective patches.

Michal M.

 Michal Malý (4):
  INPUT: Add ff-memless-next module
  HID: Port hid-lg4ff to ff-memless-next
  HID: Add support for periodic effects in hid-lg4ff
  HID: Add support for ramp effect in hid-lg4ff

 Documentation/input/ff-memless-next.txt | 141 ++++++
 drivers/hid/Kconfig                     |   2 +-
 drivers/hid/hid-lg4ff.c                 |  93 ++--
 drivers/input/Kconfig                   |  11 +
 drivers/input/Makefile                  |   1 +
 drivers/input/ff-memless-next.c         | 789 ++++++++++++++++++++++++++++++++
 include/linux/input/ff-memless-next.h   |  32 ++
 7 files changed, 1033 insertions(+), 36 deletions(-)
--
 1.9.0

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

[PATCH v2 1/4] Add ff-memless-next driver

[Michal Malý]

Introduce ff-memless-next module as a possible future replacement of
ff-memless.

Tested-by: Elias Vanderstuyft <elias.vds@gmail.com>
Signed-off-by: Michal Malý <madcatxster@prifuk.cz>
---
 v2:
  Handle upload and removal of uncombinable effects correctly
  Remove redundant information from the documentation file
  Invert direction of force along Y axis to conform to common conventions
  Set FF_GAIN bit

 Documentation/input/ff-memless-next.txt | 141 ++++++
 drivers/input/Kconfig                   |  11 +
 drivers/input/Makefile                  |   1 +
 drivers/input/ff-memless-next.c         | 789 ++++++++++++++++++++++++++++++++
 include/linux/input/ff-memless-next.h   |  32 ++
 5 files changed, 974 insertions(+)
 create mode 100644 Documentation/input/ff-memless-next.txt
 create mode 100644 drivers/input/ff-memless-next.c
 create mode 100644 include/linux/input/ff-memless-next.h

diff --git a/Documentation/input/ff-memless-next.txt b/Documentation/input/ff-memless-next.txt
new file mode 100644
index 0000000..1b550dc
--- /dev/null
+++ b/Documentation/input/ff-memless-next.txt
@@ -0,0 +1,141 @@
+"ff-memless-next" force feedback module for memoryless devices.
+By Michal Malý <madcatxster@prifuk.cz> on 2013/12/21
+----------------------------------------------------------------------------
+
+1. Introduction
+~~~~~~~~~~~~~~~
+This document describes basic working principles of the "ff-memless-next"
+module and its purpose. This document also contains a summary
+of the "ff-memless-next" API and brief instructions on how to use it to write
+a hardware-specific backend with "ff-memless-next". Some specifics
+of ff-memless-next that might be of interest for userspace developers
+are also discussed.
+
+2. Basic principles of ff-memless-next
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+A lot of commonly used force feedback devices do not have a memory of their
+own. This means that it is not possible to upload a force feedback effect
+to such a device and let the device's CPU handle the playback. Instead,
+the device relies solely on its driver to tell it what force to generate.
+"ff-memless-next" was written to serve in this capacity. Its purpose is to
+calculate the overall force the device should apply and pass the result to
+a hardware-specific backend which then submits the appropriate command to
+the device.
+
+"ff-memless-next" differentiates between two types of force feedback effects,
+"combinable" and "uncombinable".
+"Combinable" effects are effects that generate a force of a given
+magnitude and direction. The magnitude can change in time according to the
+envelope of the effect and its waveform; the latter applies only to periodic
+effects. Force generated by "combinable" effect does not depend on the position
+of the device. Forces generated by each "combinable" effect that is active
+are periodically recalculated as needed and superposed into one overall force.
+"Combinable" effects are FF_CONSTANT, FF_PERIODIC and FF_RAMP.
+
+"Uncombinable" effects generate a force that depends on the position of
+the device. "ff-memless-next" assumes that the device takes care of processing
+such an effect. However, a device might have a limit on how many "uncombinable"
+effects can be active at once and this limit might be lower than the maximum
+effect count set in "ff-memless-next". "ff-memless-next" allows a
+hardware-specific driver to check whether the device is able to play
+an "uncombinable" effect. As of now an error during effect upload is not
+reported back to userspace. Please be prepared that this might change
+in the future.
+"Uncombinable" effects are FF_DAMPER, FF_FRICTION, FF_INERTIA and FF_SPRING.
+
+FF_CUSTOM is currently not handled by "ff-memless-next".
+
+3. API provided by "ff-memless-next"
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+"ff-memless-next" provides an API for developers of hardware-specific
+drivers. In order to use the API, the hardware-specific driver should
+include <linux/input/ff-memless-next.h>
+Functions and structs defined by this API are:
+
+int input_ff_create_mlnx(struct input_dev *dev, void *data,
+			 int(*control_effect)(struct input_dev *, void *, const struct mlnx_effect_command *),
+			 const u16 update_rate)
+- Any hardware-specific driver that wants to use "ff-memless-next" must call
+this function. The function takes care of creating and registering a force
+feedback device within the kernel. It also initializes resources needed by
+"ff-memless-next" to handle a new device. No other initialization steps are necessary.
+	Parameters:
+	* dev - pointer to valid struct input_dev
+	* data - pointer to custom data the hardware-specific backend
+		 might need to pass to the control_effect() callback function
+		 (discussed later). * data will be freed automatically by
+		 "ff-memless-next" upon device's destruction.
+	* control_effect - A callback function. "ff-memless-next" will call
+			   this function when it is done processing effects.
+			   Implementation of control_effect() in the
+			   hardware-specific driver should create an appropriate
+			   command and submit it to the device.
+			   This function is called with dev->event_lock
+			   spinlock held.
+	update_rate - Rate in milliseconds at which envelopes and periodic
+		      effects are recalculated. Minimum value is limited by the
+		      kernel timer resolution and changes with value of
+		      CONFIG_HZ.
+
+struct mlnx_effect_command
+^^^^^^^^^^^^^^^^^^^^^^^^^^
+- This struct is passed to the hardware-specific backend in
+the control_effect() function. See "ff-memless-next.h" for details.
+
+enum mlnx_commands
+^^^^^^^^^^^^^^^^^^
+- Types of commands generated by ff-memless-next
+	MLNX_START_COMBINED - Start or update "combinable" effect
+	MLNX_STOP_COMBINED - Stop "combinable" effect. In most cases this means
+			     to set the force to zero.
+	MLNX_UPLOAD_UNCOMB - Check if the device can accept and play
+			     "uncombinable" effect and upload the effect into
+			     the device's internal memory.
+			     Hardware-specific driver should return 0
+			     on success.
+	MLNX_ERASE_UNCOMB - Remove "uncombinable" effect from device's
+			    internal memory.
+			    Hardware-specific driver should return 0
+			    on success.
+	MLNX_START_UNCOMB - Start playback of "uncombinable" effect.
+	MLNX_STOP_UNCOMB - Stop playback of "uncombinable" effect.
+
+struct mlnx_simple_force
+^^^^^^^^^^^^^^^^^^^^^^^^
+	x - overall force along X axis
+	y - overall force along Y axis
+
+struct mlnx_uncomb_effect
+^^^^^^^^^^^^^^^^^^^^^^^^^
+- Information about "uncombinable" effect.
+	id - internal ID of the effect
+	* effect - pointer to the internal copy of the effect kept by
+		   "ff-memless-next". This pointer will remain valid until
+		   the effect is removed.
+
+Sample implementation of a dummy driver that uses "ff-memless-next" is
+available at "git://prifuk.cz/ff-dummy-device". Link to the source will
+be kept up to date.
+
+4. Specifics of "ff-memless-next" for userspace developers
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+None of the persons involved in development or testing of "ff-memless-next"
+is aware of any reference force feedback specifications. "ff-memless-next"
+tries to adhere to Microsoft's DirectInput specifications because we
+believe that is what most developers have experience with.
+
+- Waveforms of periodic effects do not start at the origin, but as follows:
+	SAW_UP: At minimum
+	SAW_DOWN: At maximum
+	SQUARE: At maximum
+	TRIANGLE: At maximum
+	SINE: At origin
+
+- Updating periodic effects:
+	- All periodic effects are restarted when their parameters are updated.
+
+- Updating effects of finite duration:
+	- Once an effect of finite length finishes playing, it is considered
+	  stopped. Only effects that are playing can be updated on the fly.
+	  Therefore effects of finite duration can be updated only until
+	  they finish playing.
diff --git a/drivers/input/Kconfig b/drivers/input/Kconfig
index a11ff74..ba05100 100644
--- a/drivers/input/Kconfig
+++ b/drivers/input/Kconfig
@@ -77,6 +77,17 @@ config INPUT_MATRIXKMAP
 	  To compile this driver as a module, choose M here: the
 	  module will be called matrix-keymap.
 
+config INPUT_FF_MEMLESS_NEXT
+	tristate "New version of support for memoryless force feedback devices"
+	help
+	  Say Y here if you want to enable support for various memoryless
+	  force feedback devices.
+
+	  If unsure, say N.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called ff-memless-next.
+
 comment "Userland interfaces"
 
 config INPUT_MOUSEDEV
diff --git a/drivers/input/Makefile b/drivers/input/Makefile
index 5ca3f63..169e99c 100644
--- a/drivers/input/Makefile
+++ b/drivers/input/Makefile
@@ -11,6 +11,7 @@ obj-$(CONFIG_INPUT_FF_MEMLESS)	+= ff-memless.o
 obj-$(CONFIG_INPUT_POLLDEV)	+= input-polldev.o
 obj-$(CONFIG_INPUT_SPARSEKMAP)	+= sparse-keymap.o
 obj-$(CONFIG_INPUT_MATRIXKMAP)	+= matrix-keymap.o
+obj-$(CONFIG_INPUT_FF_MEMLESS_NEXT)	+= ff-memless-next.o
 
 obj-$(CONFIG_INPUT_MOUSEDEV)	+= mousedev.o
 obj-$(CONFIG_INPUT_JOYDEV)	+= joydev.o
diff --git a/drivers/input/ff-memless-next.c b/drivers/input/ff-memless-next.c
new file mode 100644
index 0000000..843a223
--- /dev/null
+++ b/drivers/input/ff-memless-next.c
@@ -0,0 +1,789 @@
+/*
+ * Force feedback support for memoryless devices
+ *
+ * This module is based on "ff-memless" orignally written by Anssi Hannula.
+ * It is extended to support all force feedback effects currently supported
+ * by the Linux input stack.
+ *
+ * Copyright(c) 2013 Michal Maly <madcatxster@prifuk.cz>
+ *
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/slab.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/spinlock.h>
+#include <linux/jiffies.h>
+#include <linux/fixp-arith.h>
+#include <linux/input/ff-memless-next.h>
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Michal \"MadCatX\" Maly");
+MODULE_DESCRIPTION("Force feedback support for memoryless force feedback devices");
+
+#define FF_MAX_EFFECTS 16
+#define FF_MIN_EFFECT_LENGTH ((1000 / CONFIG_HZ) + 1)
+#define FF_EFFECT_STARTED 1
+#define FF_EFFECT_PLAYING 2
+
+
+struct mlnx_effect {
+	struct ff_effect effect;
+	unsigned long flags;
+	unsigned long begin_at;
+	unsigned long stop_at;
+	unsigned long updated_at;
+	unsigned long attack_stop;
+	unsigned long fade_begin;
+	int repeat;
+};
+
+struct mlnx_device {
+	u8 combinable_playing;
+	unsigned long update_rate_jiffies;
+	void *private;
+	struct mlnx_effect effects[FF_MAX_EFFECTS];
+	u16 gain;
+	struct timer_list timer;
+	struct input_dev *dev;
+
+	int (*control_effect)(struct input_dev *, void *,
+			      const struct mlnx_effect_command *);
+};
+
+static s32 mlnx_calculate_x_force(const s32 level,
+						  const u16 direction)
+{
+	s32 new = (level * -fixp_sin(direction)) >> FRAC_N;
+	pr_debug("x force: %d\n", new);
+	return new;
+}
+
+static s32 mlnx_calculate_y_force(const s32 level,
+						  const u16 direction)
+{
+	s32 new = (level * fixp_cos(direction)) >> FRAC_N;
+	pr_debug("y force: %d\n", new);
+	return new;
+}
+
+static bool mlnx_is_combinable(const struct ff_effect *effect)
+{
+	switch (effect->type) {
+	case FF_CONSTANT:
+	case FF_PERIODIC:
+	case FF_RAMP:
+		return true;
+	default:
+		return false;
+	}
+}
+
+static bool mlnx_is_conditional(const struct ff_effect *effect)
+{
+	switch (effect->type) {
+	case FF_DAMPER:
+	case FF_FRICTION:
+	case FF_INERTIA:
+	case FF_SPRING:
+		return true;
+	default:
+		return false;
+	}
+}
+
+static void mlnx_clr_playing(struct mlnx_effect *mlnxeff)
+{
+	__clear_bit(FF_EFFECT_PLAYING, &mlnxeff->flags);
+}
+
+static void mlnx_clr_started(struct mlnx_effect *mlnxeff)
+{
+	__clear_bit(FF_EFFECT_STARTED, &mlnxeff->flags);
+}
+
+static bool mlnx_is_playing(const struct mlnx_effect *mlnxeff)
+{
+	return test_bit(FF_EFFECT_PLAYING, &mlnxeff->flags);
+}
+
+static bool mlnx_is_started(const struct mlnx_effect *mlnxeff)
+{
+	return test_bit(FF_EFFECT_STARTED, &mlnxeff->flags);
+}
+
+static bool mlnx_test_set_playing(struct mlnx_effect *mlnxeff)
+{
+	return test_and_set_bit(FF_EFFECT_PLAYING, &mlnxeff->flags);
+}
+
+static const struct ff_envelope *mlnx_get_envelope(const struct ff_effect *effect)
+{
+	static const struct ff_envelope empty;
+
+	switch (effect->type) {
+	case FF_CONSTANT:
+		return &effect->u.constant.envelope;
+	case FF_PERIODIC:
+		return &effect->u.periodic.envelope;
+	case FF_RAMP:
+		return &effect->u.ramp.envelope;
+	default:
+		return &empty;
+	}
+}
+
+/* Some devices might have a limit on how many uncombinable effects
+ * can be played at once */
+static int mlnx_upload_conditional(struct mlnx_device *mlnxdev,
+				   const struct ff_effect *effect)
+{
+	struct mlnx_effect_command ecmd = {
+		.cmd = MLNX_UPLOAD_UNCOMB,
+		.u.uncomb.id = effect->id,
+		.u.uncomb.effect = effect
+	};
+	return mlnxdev->control_effect(mlnxdev->dev, mlnxdev->private, &ecmd);
+}
+
+static int mlnx_erase_conditional(struct mlnx_device *mlnxdev,
+				  const struct ff_effect *effect)
+{
+	struct mlnx_effect_command ecmd = {
+		.cmd = MLNX_ERASE_UNCOMB,
+		.u.uncomb.id = effect->id,
+		.u.uncomb.effect = effect
+	};
+	return mlnxdev->control_effect(mlnxdev->dev, mlnxdev->private, &ecmd);
+}
+
+static void mlnx_set_envelope_times(struct mlnx_effect *mlnxeff)
+{
+	const struct ff_effect *effect = &mlnxeff->effect;
+	const struct ff_envelope *envelope = mlnx_get_envelope(effect);
+
+	if (envelope->attack_length) {
+		unsigned long j = msecs_to_jiffies(envelope->attack_length);
+		mlnxeff->attack_stop = mlnxeff->begin_at + j;
+	}
+	if (effect->replay.length && envelope->fade_length) {
+		unsigned long j = msecs_to_jiffies(envelope->fade_length);
+		mlnxeff->fade_begin = mlnxeff->stop_at - j;
+	}
+}
+
+static void mlnx_set_trip_times(struct mlnx_effect *mlnxeff,
+				const unsigned long now)
+{
+	const struct ff_effect *effect = &mlnxeff->effect;
+	const u16 replay_delay = effect->replay.delay;
+	const u16 replay_length = effect->replay.length;
+
+	mlnxeff->begin_at = now + msecs_to_jiffies(replay_delay);
+	mlnxeff->stop_at = mlnxeff->begin_at + msecs_to_jiffies(replay_length);
+	mlnxeff->updated_at = mlnxeff->begin_at;
+}
+
+static void mlnx_start_effect(struct mlnx_effect *mlnxeff)
+{
+	const unsigned long now = jiffies;
+
+	mlnx_set_trip_times(mlnxeff, now);
+	mlnx_set_envelope_times(mlnxeff);
+	__set_bit(FF_EFFECT_STARTED, &mlnxeff->flags);
+}
+
+static void mlnx_stop_effect(struct mlnx_device *mlnxdev,
+			     const struct mlnx_effect *mlnxeff)
+{
+	switch (mlnxeff->effect.type) {
+	case FF_CONSTANT:
+	case FF_PERIODIC:
+	case FF_RAMP:
+		if (--mlnxdev->combinable_playing == 0) {
+			const struct mlnx_effect_command c = {
+				.cmd = MLNX_STOP_COMBINED
+			};
+			mlnxdev->control_effect(mlnxdev->dev, mlnxdev->private,
+						&c);
+		}
+		return;
+	case FF_DAMPER:
+	case FF_FRICTION:
+	case FF_INERTIA:
+	case FF_SPRING:
+	{
+		const struct mlnx_effect_command c = {
+			.cmd = MLNX_STOP_UNCOMB,
+			.u.uncomb.id = mlnxeff->effect.id,
+			.u.uncomb.effect = &mlnxeff->effect
+		};
+		mlnxdev->control_effect(mlnxdev->dev, mlnxdev->private, &c);
+		return;
+	}
+	default:
+		return;
+	}
+}
+
+static int mlnx_restart_effect(struct mlnx_device *mlnxdev,
+			       struct mlnx_effect *mlnxeff)
+{
+	const struct ff_effect *effect = &mlnxeff->effect;
+	const unsigned long now = jiffies;
+
+	if (mlnx_is_combinable(effect)) {
+		if (effect->replay.delay)
+			mlnx_stop_effect(mlnxdev, mlnxeff);
+		else
+			mlnxdev->combinable_playing--;
+	} else if (mlnx_is_conditional(effect)) {
+		int ret;
+		if (effect->replay.delay)
+			mlnx_stop_effect(mlnxdev, mlnxeff);
+
+		ret = mlnx_upload_conditional(mlnxdev, &mlnxeff->effect);
+		if (ret)
+			return ret;
+	}
+
+	mlnx_set_trip_times(mlnxeff, now);
+	mlnx_set_envelope_times(mlnxeff);
+
+	return 0;
+}
+
+static s32 mlnx_apply_envelope(const struct mlnx_effect *mlnxeff,
+			       const s32 level)
+{
+	const struct ff_effect *effect = &mlnxeff->effect;
+	const struct ff_envelope *envelope = mlnx_get_envelope(effect);
+	const unsigned long now = jiffies;
+	const s32 alevel = abs(level);
+
+	/* Effect has an envelope with nonzero attack time */
+	if (envelope->attack_length && time_before(now, mlnxeff->attack_stop)) {
+		const s32 clength = jiffies_to_msecs(now - mlnxeff->begin_at);
+		const s32 alength = envelope->attack_length;
+		const s32 dlevel = (alevel - envelope->attack_level)
+				 * clength / alength;
+		return level < 0 ? -(dlevel + envelope->attack_level) :
+				    (dlevel + envelope->attack_level);
+	} else if (envelope->fade_length && time_before_eq(mlnxeff->fade_begin, now)) {
+		const s32 clength = jiffies_to_msecs(now - mlnxeff->fade_begin);
+		const s32 flength = envelope->fade_length;
+		const s32 dlevel = (envelope->fade_level - alevel)
+				 * clength / flength;
+		return level < 0 ? -(dlevel + alevel) : (dlevel + alevel);
+	}
+
+	return level;
+}
+
+static s32 mlnx_calculate_periodic(struct mlnx_effect *mlnxeff, const s32 level)
+{
+	const struct ff_effect *effect = &mlnxeff->effect;
+	const unsigned long now = jiffies;
+	const u16 period = effect->u.periodic.period;
+	const u16 phase = effect->u.periodic.phase;
+	const s16 offset = effect->u.periodic.offset;
+	s32 new = level;
+	u16 t = (jiffies_to_msecs(now - mlnxeff->begin_at) + phase) % period;
+
+	switch (effect->u.periodic.waveform) {
+	case FF_SINE:
+	{
+		u16 degrees = (360 * t) / period;
+		new = ((level * fixp_sin(degrees)) >> FRAC_N) + offset;
+		break;
+	}
+	case FF_SQUARE:
+	{
+		u16 degrees = (360 * t) / period;
+		new = level * (degrees < 180 ? 1 : -1) + offset;
+		break;
+	}
+	case FF_SAW_UP:
+		new = 2 * level * t / period - level + offset;
+		break;
+	case FF_SAW_DOWN:
+		new = level - 2 * level * t / period + offset;
+		break;
+	case FF_TRIANGLE:
+	{
+		new = (2 * abs(level - (2 * level * t) / period));
+		new = new - abs(level) + offset;
+		break;
+	}
+	case FF_CUSTOM:
+		pr_debug("Custom waveform is not handled by this driver\n");
+		return level;
+	default:
+		pr_err("Invalid waveform\n");
+		return level;
+	}
+
+	/* Ensure that the offset did not make the value exceed s16 range */
+	new = clamp(new, -0x7fff, 0x7fff);
+	pr_debug("level: %d, t: %u\n", new, t);
+	return new;
+}
+
+static s32 mlnx_calculate_ramp(const struct mlnx_effect *mlnxeff)
+{
+	const struct ff_effect *effect = &mlnxeff->effect;
+	const struct ff_envelope *envelope = mlnx_get_envelope(effect);
+	const unsigned long now = jiffies;
+	const u16 length = effect->replay.length;
+	const s16 mean = (effect->u.ramp.start_level + effect->u.ramp.end_level) / 2;
+	const u16 t = jiffies_to_msecs(now - mlnxeff->begin_at);
+	s32 start = effect->u.ramp.start_level;
+	s32 end = effect->u.ramp.end_level;
+	s32 new;
+
+	if (envelope->attack_length && time_before(now, mlnxeff->attack_stop)) {
+		const s32 clength = jiffies_to_msecs(now - mlnxeff->begin_at);
+		const s32 alength = envelope->attack_length;
+		s32 attack_level;
+		if (end > start)
+			attack_level = mean - envelope->attack_level;
+		else
+			attack_level = mean + envelope->attack_level;
+		start = (start - attack_level) * clength / alength
+		      + attack_level;
+	} else if (envelope->fade_length && time_before_eq(mlnxeff->fade_begin, now)) {
+		const s32 clength = jiffies_to_msecs(now - mlnxeff->fade_begin);
+		const s32 flength = envelope->fade_length;
+		s32 fade_level;
+		if (end > start)
+			fade_level = mean + envelope->fade_level;
+		else
+			fade_level = mean - envelope->fade_level;
+		end = (fade_level - end) * clength / flength + end;
+	}
+
+	new = ((end - start) * t) / length + start;
+	new = clamp(new, -0x7fff, 0x7fff);
+	pr_debug("RAMP level: %d, t: %u\n", new, t);
+	return new;
+}
+
+static void mlnx_destroy(struct ff_device *dev)
+{
+	struct mlnx_device *mlnxdev = dev->private;
+	del_timer_sync(&mlnxdev->timer);
+
+	kfree(mlnxdev->private);
+}
+
+static unsigned long mlnx_get_envelope_update_time(const struct mlnx_effect *mlnxeff,
+						   const unsigned long update_rate_jiffies)
+{
+	const struct ff_effect *effect = &mlnxeff->effect;
+	const struct ff_envelope *envelope = mlnx_get_envelope(effect);
+	const unsigned long now = jiffies;
+	unsigned long fade_next;
+
+	/* Effect has an envelope with nonzero attack time */
+	if (envelope->attack_length && time_before(now, mlnxeff->attack_stop)) {
+		if (time_before(mlnxeff->updated_at + update_rate_jiffies, mlnxeff->attack_stop))
+			return mlnxeff->updated_at + update_rate_jiffies;
+
+		return mlnxeff->attack_stop;
+	}
+
+	/* Effect has an envelope with nonzero fade time */
+	if (mlnxeff->effect.replay.length) {
+		if (!envelope->fade_length)
+			return mlnxeff->stop_at;
+
+		/* Schedule the next update when the fade begins */
+		if (time_before(mlnxeff->updated_at, mlnxeff->fade_begin))
+			return mlnxeff->fade_begin;
+
+		/* Already fading */
+		fade_next = mlnxeff->updated_at + update_rate_jiffies;
+
+		/* Schedule update when the effect stops */
+		if (time_after(fade_next, mlnxeff->stop_at))
+			return mlnxeff->stop_at;
+		/* Schedule update at the next checkpoint */
+			return fade_next;
+	}
+
+	/* There is no envelope */
+
+	/* Prevent the effect from being started twice */
+	if (mlnxeff->begin_at == now && mlnx_is_playing(mlnxeff))
+		return now - 1;
+
+	return mlnxeff->begin_at;
+}
+
+static unsigned long mlnx_get_update_time(struct mlnx_effect *mlnxeff,
+				const unsigned long update_rate_jiffies)
+{
+	const unsigned long now = jiffies;
+	unsigned long time, update_periodic;
+
+	switch (mlnxeff->effect.type) {
+	/* Constant effect does not change with time, but it can have
+	 * an envelope and a duration */
+	case FF_CONSTANT:
+		return mlnx_get_envelope_update_time(mlnxeff,
+						     update_rate_jiffies);
+	/* Periodic and ramp effects have to be periodically updated */
+	case FF_PERIODIC:
+	case FF_RAMP:
+		time = mlnx_get_envelope_update_time(mlnxeff,
+						     update_rate_jiffies);
+		update_periodic = mlnxeff->updated_at + update_rate_jiffies;
+
+		/* Periodic effect has to be updated earlier than envelope
+		 * or envelope update time is in the past */
+		if (time_before(update_periodic, time) || time_before(time, now))
+			return update_periodic;
+		/* Envelope needs to be updated */
+		return time;
+	case FF_DAMPER:
+	case FF_FRICTION:
+	case FF_INERTIA:
+	case FF_SPRING:
+	default:
+		if (time_after_eq(mlnxeff->begin_at, now))
+			return mlnxeff->begin_at;
+
+		return mlnxeff->stop_at;
+	}
+}
+
+static void mlnx_schedule_playback(struct mlnx_device *mlnxdev)
+{
+	struct mlnx_effect *mlnxeff;
+	const unsigned long now = jiffies;
+	int events = 0;
+	int i;
+	unsigned long earliest = 0;
+	unsigned long time;
+
+	/* Iterate over all effects and determine the earliest
+	 * time when we have to attend to any */
+	for (i = 0; i < FF_MAX_EFFECTS; i++) {
+		mlnxeff = &mlnxdev->effects[i];
+
+		if (!mlnx_is_started(mlnxeff))
+			continue; /* Effect is not started, skip it */
+
+		if (mlnx_is_playing(mlnxeff))
+			time = mlnx_get_update_time(mlnxeff,
+						mlnxdev->update_rate_jiffies);
+		else
+			time = mlnxeff->begin_at;
+
+		pr_debug("Update time for effect %d: %lu\n", i, time);
+
+		/* Scheduled time is in the future and is either
+		 * before the current earliest time or it is
+		 * the first valid time value in this pass */
+		if (time_before_eq(now, time) &&
+		    (++events == 1 || time_before(time, earliest)))
+			earliest = time;
+	}
+
+	if (events) {
+		pr_debug("Events: %d, earliest: %lu\n", events, earliest);
+		mod_timer(&mlnxdev->timer, earliest);
+	}
+}
+
+static void mlnx_add_force(struct mlnx_effect *mlnxeff, s32 *cfx, s32 *cfy,
+			   const u16 gain)
+{
+	const struct ff_effect *effect = &mlnxeff->effect;
+	u16 direction;
+	s32 level;
+
+	pr_debug("Processing effect type %d, ID %d\n",
+		 mlnxeff->effect.type, mlnxeff->effect.id);
+
+	direction = mlnxeff->effect.direction * 360 / 0xffff;
+	pr_debug("Direction deg: %u\n", direction);
+
+	switch (mlnxeff->effect.type) {
+	case FF_CONSTANT:
+		level = mlnx_apply_envelope(mlnxeff, effect->u.constant.level);
+		break;
+	case FF_PERIODIC:
+		level = mlnx_apply_envelope(mlnxeff,
+					    effect->u.periodic.magnitude);
+		level = mlnx_calculate_periodic(mlnxeff, level);
+		break;
+	case FF_RAMP:
+		level = mlnx_calculate_ramp(mlnxeff);
+		break;
+	default:
+		pr_err("Effect %d not handled by mlnx_add_force\n",
+		       mlnxeff->effect.type);
+		return;
+	}
+
+	*cfx += mlnx_calculate_x_force(level, direction) * gain / 0xffff;
+	*cfy += mlnx_calculate_y_force(level, direction) * gain / 0xffff;
+}
+
+static void mlnx_play_effects(struct mlnx_device *mlnxdev)
+{
+	const u16 gain = mlnxdev->gain;
+	const unsigned long now = jiffies;
+	int i;
+	int cfx = 0;
+	int cfy = 0;
+
+	for (i = 0; i < FF_MAX_EFFECTS; i++) {
+		struct mlnx_effect *mlnxeff = &mlnxdev->effects[i];
+
+		if (!mlnx_is_started(mlnxeff)) {
+			pr_debug("Effect %hd/%d not started\n",
+				 mlnxeff->effect.id, i);
+			continue;
+		}
+
+		if (time_before(now, mlnxeff->begin_at)) {
+			pr_debug("Effect %hd/%d begins at a later time\n",
+				 mlnxeff->effect.id, i);
+			continue;
+		}
+
+		if (time_before_eq(mlnxeff->stop_at, now) && mlnxeff->effect.replay.length) {
+			pr_debug("Effect %hd/%d has to be stopped\n",
+				 mlnxeff->effect.id, i);
+
+			mlnx_clr_playing(mlnxeff);
+			if (--mlnxeff->repeat > 0)
+				mlnx_restart_effect(mlnxdev, mlnxeff);
+			else {
+				mlnx_clr_started(mlnxeff);
+				mlnx_stop_effect(mlnxdev, mlnxeff);
+				if (mlnx_is_conditional(&mlnxeff->effect))
+					mlnx_erase_conditional(mlnxdev, &mlnxeff->effect);
+			}
+
+			continue;
+		}
+
+		switch (mlnxeff->effect.type) {
+		case FF_CONSTANT:
+		case FF_PERIODIC:
+		case FF_RAMP:
+			if (!mlnx_test_set_playing(mlnxeff)) {
+				mlnxdev->combinable_playing++;
+				pr_debug("Starting combinable effect, total %u\n",
+					 mlnxdev->combinable_playing);
+			}
+			mlnx_add_force(mlnxeff, &cfx, &cfy, gain);
+			break;
+		case FF_DAMPER:
+		case FF_FRICTION:
+		case FF_INERTIA:
+		case FF_SPRING:
+			if (!mlnx_test_set_playing(mlnxeff)) {
+				const struct mlnx_effect_command ecmd = {
+					.cmd = MLNX_START_UNCOMB,
+					.u.uncomb.id = i,
+					.u.uncomb.effect = &mlnxeff->effect
+				};
+				mlnxdev->control_effect(mlnxdev->dev,
+						      mlnxdev->private, &ecmd);
+			}
+			break;
+		default:
+			pr_debug("Unhandled type of effect\n");
+		}
+		mlnxeff->updated_at = now;
+	}
+
+	if (mlnxdev->combinable_playing) {
+		const struct mlnx_effect_command ecmd = {
+			.cmd = MLNX_START_COMBINED,
+			.u.simple_force = {
+				.x = clamp(cfx, -0x7fff, 0x7fff),
+				.y = clamp(cfy, -0x7fff, 0x7fff)
+			}
+		};
+		mlnxdev->control_effect(mlnxdev->dev, mlnxdev->private, &ecmd);
+	}
+
+	mlnx_schedule_playback(mlnxdev);
+}
+
+static void mlnx_set_gain(struct input_dev *dev, u16 gain)
+{
+	struct mlnx_device *mlnxdev = dev->ff->private;
+	int i;
+
+	mlnxdev->gain = gain;
+
+	for (i = 0; i < FF_MAX_EFFECTS; i++)
+		mlnx_clr_playing(&mlnxdev->effects[i]);
+
+	mlnx_play_effects(mlnxdev);
+}
+
+static int mlnx_startstop(struct input_dev *dev, int effect_id, int repeat)
+{
+	struct mlnx_device *mlnxdev = dev->ff->private;
+	struct mlnx_effect *mlnxeff = &mlnxdev->effects[effect_id];
+	int ret;
+
+	if (repeat > 0) {
+		pr_debug("Starting effect ID %d\n", effect_id);
+		mlnxeff->repeat = repeat;
+
+		if (!mlnx_is_started(mlnxeff)) {
+			/* Check that device has a free effect slot */
+			if (mlnx_is_conditional(&mlnxeff->effect)) {
+				ret = mlnx_upload_conditional(mlnxdev, &mlnxeff->effect);
+				if (ret) {
+					/* Device effect slots are all occupied */
+					pr_debug("No free effect slot for EID %d\n", effect_id);
+					return ret;
+				}
+			}
+			mlnx_start_effect(mlnxeff);
+		}
+	} else {
+		pr_debug("Stopping effect ID %d\n", effect_id);
+		if (mlnx_is_started(mlnxeff)) {
+			if (mlnx_is_playing(mlnxeff)) {
+				mlnx_clr_playing(mlnxeff);
+				mlnx_stop_effect(mlnxdev, mlnxeff);
+			}
+			mlnx_clr_started(mlnxeff);
+
+			if (mlnx_is_conditional(&mlnxeff->effect))
+				return mlnx_erase_conditional(mlnxdev, &mlnxeff->effect);
+		} else {
+			pr_debug("Effect ID %d already stopped\n", effect_id);
+			return 0;
+		}
+	}
+	mlnx_play_effects(mlnxdev);
+
+	return 0;
+}
+
+static void mlnx_timer_fired(unsigned long data)
+{
+	struct input_dev *dev = (struct input_dev *)data;
+	unsigned long flags;
+
+	spin_lock_irqsave(&dev->event_lock, flags);
+	mlnx_play_effects(dev->ff->private);
+	spin_unlock_irqrestore(&dev->event_lock, flags);
+}
+
+static int mlnx_upload(struct input_dev *dev, struct ff_effect *effect,
+		       struct ff_effect *old)
+{
+	struct mlnx_device *mlnxdev = dev->ff->private;
+	struct mlnx_effect *mlnxeff = &mlnxdev->effects[effect->id];
+	const u16 length = effect->replay.length;
+	const u16 delay = effect->replay.delay;
+	int ret, fade_from;
+
+	/* Effect's timing is below kernel timer resolution */
+	if (length && length < FF_MIN_EFFECT_LENGTH)
+		effect->replay.length = FF_MIN_EFFECT_LENGTH;
+	if (delay && delay < FF_MIN_EFFECT_LENGTH)
+		effect->replay.delay = FF_MIN_EFFECT_LENGTH;
+
+	/* Periodic effects must have a non-zero period */
+	if (effect->type == FF_PERIODIC) {
+		if (!effect->u.periodic.period)
+			return -EINVAL;
+	}
+	/* Ramp effects cannot be infinite */
+	if (effect->type == FF_RAMP && !length)
+		return -EINVAL;
+
+	if (mlnx_is_combinable(effect)) {
+		const struct ff_envelope *envelope = mlnx_get_envelope(effect);
+
+		/* Infinite effects cannot fade */
+		if (!length && envelope->fade_length > 0)
+			return -EINVAL;
+		/* Fade length cannot be greater than effect duration */
+		fade_from = (int)length - (int)envelope->fade_length;
+		if (fade_from < 0)
+			return -EINVAL;
+		/* Envelope cannot start fading before it finishes attacking */
+		if (fade_from < envelope->attack_length && fade_from > 0)
+			return -EINVAL;
+	}
+
+
+	spin_lock_irq(&dev->event_lock);
+	mlnxeff->effect = *effect; /* Keep internal copy of the effect */
+	/* Check if the effect being modified is playing */
+	if (mlnx_is_started(mlnxeff)) {
+		if (mlnx_is_playing(mlnxeff)) {
+			mlnx_clr_playing(mlnxeff);
+			ret = mlnx_restart_effect(mlnxdev, mlnxeff);
+
+			if (ret) {
+				/* Restore the original effect */
+				if (old)
+					mlnxeff->effect = *old;
+				spin_unlock_irq(&dev->event_lock);
+				return ret;
+			}
+		}
+
+		mlnx_schedule_playback(mlnxdev);
+		spin_unlock_irq(&dev->event_lock);
+		return 0;
+	}
+
+	spin_unlock_irq(&dev->event_lock);
+
+	return 0;
+}
+
+int input_ff_create_mlnx(struct input_dev *dev, void *data,
+			 int(*control_effect)(struct input_dev *, void *, const struct mlnx_effect_command *),
+			 const u16 update_rate)
+{
+	struct mlnx_device *mlnxdev;
+	int ret;
+
+	mlnxdev = kzalloc(sizeof(*mlnxdev), GFP_KERNEL);
+	if (!mlnxdev)
+		return -ENOMEM;
+
+	mlnxdev->dev = dev;
+	mlnxdev->private = data;
+	mlnxdev->control_effect = control_effect;
+	mlnxdev->gain = 0xffff;
+	mlnxdev->update_rate_jiffies = update_rate < FF_MIN_EFFECT_LENGTH ?
+				       FF_MIN_EFFECT_LENGTH : update_rate;
+	input_set_capability(dev, EV_FF, FF_GAIN);
+	setup_timer(&mlnxdev->timer, mlnx_timer_fired, (unsigned long)dev);
+
+	ret = input_ff_create(dev, FF_MAX_EFFECTS);
+	if (ret) {
+		kfree(mlnxdev);
+		return ret;
+	}
+
+	dev->ff->private = mlnxdev;
+	dev->ff->upload = mlnx_upload;
+	dev->ff->set_gain = mlnx_set_gain;
+	dev->ff->destroy = mlnx_destroy;
+	dev->ff->playback = mlnx_startstop;
+
+	pr_debug("Device successfully registered.\n");
+	return 0;
+}
+EXPORT_SYMBOL_GPL(input_ff_create_mlnx);
diff --git a/include/linux/input/ff-memless-next.h b/include/linux/input/ff-memless-next.h
new file mode 100644
index 0000000..ba89ba1
--- /dev/null
+++ b/include/linux/input/ff-memless-next.h
@@ -0,0 +1,32 @@
+#include <linux/input.h>
+
+enum mlnx_commands {
+	MLNX_START_COMBINED,
+	MLNX_STOP_COMBINED,
+	MLNX_START_UNCOMB,
+	MLNX_STOP_UNCOMB,
+	MLNX_UPLOAD_UNCOMB,
+	MLNX_ERASE_UNCOMB
+};
+
+struct mlnx_simple_force {
+	const s32 x;
+	const s32 y;
+};
+
+struct mlnx_uncomb_effect {
+	const int id;
+	const struct ff_effect *effect;
+};
+
+struct mlnx_effect_command {
+	const enum mlnx_commands cmd;
+	union {
+		const struct mlnx_simple_force simple_force;
+		const struct mlnx_uncomb_effect uncomb;
+	} u;
+};
+
+int input_ff_create_mlnx(struct input_dev *dev, void *data,
+			 int(*control_effect)(struct input_dev *, void *, const struct mlnx_effect_command *),
+			 const u16 update_rate);
-- 
1.9.0


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

Re: [PATCH v2 1/4] Add ff-memless-next driver

[Anssi Hannula]

24.02.2014 01:29, Michal Malý kirjoitti:
> Introduce ff-memless-next module as a possible future replacement of
> ff-memless.
> 
> Tested-by: Elias Vanderstuyft <elias.vds@gmail.com>
> Signed-off-by: Michal Malý <madcatxster@prifuk.cz>

Some comments below.

> ---
>  v2:
>   Handle upload and removal of uncombinable effects correctly
>   Remove redundant information from the documentation file
>   Invert direction of force along Y axis to conform to common conventions
>   Set FF_GAIN bit
> 
>  Documentation/input/ff-memless-next.txt | 141 ++++++
>  drivers/input/Kconfig                   |  11 +
>  drivers/input/Makefile                  |   1 +
>  drivers/input/ff-memless-next.c         | 789 ++++++++++++++++++++++++++++++++
>  include/linux/input/ff-memless-next.h   |  32 ++
>  5 files changed, 974 insertions(+)
>  create mode 100644 Documentation/input/ff-memless-next.txt
>  create mode 100644 drivers/input/ff-memless-next.c
>  create mode 100644 include/linux/input/ff-memless-next.h
> 
> diff --git a/Documentation/input/ff-memless-next.txt b/Documentation/input/ff-memless-next.txt
> new file mode 100644
> index 0000000..1b550dc
> --- /dev/null
> +++ b/Documentation/input/ff-memless-next.txt
> @@ -0,0 +1,141 @@
[...]
> +3. API provided by "ff-memless-next"
> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> +"ff-memless-next" provides an API for developers of hardware-specific
> +drivers. In order to use the API, the hardware-specific driver should
> +include <linux/input/ff-memless-next.h>
> +Functions and structs defined by this API are:
[...]

This kind of basic API documentation belongs in the headers (kernel-doc
format).

[...]
> +	MLNX_UPLOAD_UNCOMB - Check if the device can accept and play
> +			     "uncombinable" effect and upload the effect into
> +			     the device's internal memory.
> +			     Hardware-specific driver should return 0
> +			     on success.
> +	MLNX_ERASE_UNCOMB - Remove "uncombinable" effect from device's
> +			    internal memory.
> +			    Hardware-specific driver should return 0
> +			    on success.
> +	MLNX_START_UNCOMB - Start playback of "uncombinable" effect.
> +	MLNX_STOP_UNCOMB - Stop playback of "uncombinable" effect.

These seem to be unused by any drivers?

I don't think they should be added to the kernel before there is an
implementation using them (it also makes reviewing them more difficult).

[...]
> +4. Specifics of "ff-memless-next" for userspace developers
> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> +None of the persons involved in development or testing of "ff-memless-next"
> +is aware of any reference force feedback specifications. "ff-memless-next"
> +tries to adhere to Microsoft's DirectInput specifications because we
> +believe that is what most developers have experience with.
> +
> +- Waveforms of periodic effects do not start at the origin, but as follows:
> +	SAW_UP: At minimum
> +	SAW_DOWN: At maximum
> +	SQUARE: At maximum
> +	TRIANGLE: At maximum
> +	SINE: At origin
> +
> +- Updating periodic effects:
> +	- All periodic effects are restarted when their parameters are updated.
> +
> +- Updating effects of finite duration:
> +	- Once an effect of finite length finishes playing, it is considered
> +	  stopped. Only effects that are playing can be updated on the fly.
> +	  Therefore effects of finite duration can be updated only until
> +	  they finish playing.

Stopped effects should still be able to be updated.

Anyway, if you want to target userspace developers with this
information, you should put it in generic documentation ("it is not
guaranteed that X", etc.). If not, don't say "for userspace developers".

[...]
> diff --git a/drivers/input/ff-memless-next.c b/drivers/input/ff-memless-next.c
> new file mode 100644
> index 0000000..843a223
> --- /dev/null
> +++ b/drivers/input/ff-memless-next.c
> @@ -0,0 +1,789 @@
> +/*
> + * Force feedback support for memoryless devices
> + *
> + * This module is based on "ff-memless" orignally written by Anssi Hannula.
> + * It is extended to support all force feedback effects currently supported
> + * by the Linux input stack.
> + *
> + * Copyright(c) 2013 Michal Maly <madcatxster@prifuk.cz>
> + *
> + */
[...]
> +static int mlnx_upload(struct input_dev *dev, struct ff_effect *effect,
> +		       struct ff_effect *old)
> +{
[...]
> +		}
> +
> +		mlnx_schedule_playback(mlnxdev);
> +		spin_unlock_irq(&dev->event_lock);
> +		return 0;

The above two lines are unneeded.

> +	}
> +
> +	spin_unlock_irq(&dev->event_lock);
> +
> +	return 0;
> +}
> +
[...]
> diff --git a/include/linux/input/ff-memless-next.h b/include/linux/input/ff-memless-next.h
> new file mode 100644
> index 0000000..ba89ba1
> --- /dev/null
> +++ b/include/linux/input/ff-memless-next.h
> @@ -0,0 +1,32 @@
[...]
> +int input_ff_create_mlnx(struct input_dev *dev, void *data,
> +			 int(*control_effect)(struct input_dev *, void *, const struct mlnx_effect_command *),
> +			 const u16 update_rate);

Why is update_rate now driver-selectable?

-- 
Anssi Hannula

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

Re: [PATCH v2 1/4] Add ff-memless-next driver

[Michal Malý]

On Monday 24 of February 2014 02:32:29 Anssi Hannula wrote:
> 24.02.2014 01:29, Michal Malý kirjoitti:
> > Introduce ff-memless-next module as a possible future replacement of
> > ff-memless.
> > 
> > Tested-by: Elias Vanderstuyft <elias.vds@gmail.com>
> > Signed-off-by: Michal Malý <madcatxster@prifuk.cz>
> 
> Some comments below.
> 
> > ---
> > 
> >  v2:
> >   Handle upload and removal of uncombinable effects correctly
> >   Remove redundant information from the documentation file
> >   Invert direction of force along Y axis to conform to common conventions
> >   Set FF_GAIN bit
> >  
> >  Documentation/input/ff-memless-next.txt | 141 ++++++
> >  drivers/input/Kconfig                   |  11 +
> >  drivers/input/Makefile                  |   1 +
> >  drivers/input/ff-memless-next.c         | 789
> >  ++++++++++++++++++++++++++++++++ include/linux/input/ff-memless-next.h  
> >  |  32 ++
> >  5 files changed, 974 insertions(+)
> >  create mode 100644 Documentation/input/ff-memless-next.txt
> >  create mode 100644 drivers/input/ff-memless-next.c
> >  create mode 100644 include/linux/input/ff-memless-next.h
> > 
> > diff --git a/Documentation/input/ff-memless-next.txt
> > b/Documentation/input/ff-memless-next.txt new file mode 100644
> > index 0000000..1b550dc
> > --- /dev/null
> > +++ b/Documentation/input/ff-memless-next.txt
> > @@ -0,0 +1,141 @@
> 
> [...]
> 
> > +3. API provided by "ff-memless-next"
> > +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > +"ff-memless-next" provides an API for developers of hardware-specific
> > +drivers. In order to use the API, the hardware-specific driver should
> > +include <linux/input/ff-memless-next.h>
> 
> > +Functions and structs defined by this API are:
> [...]
> 
> This kind of basic API documentation belongs in the headers (kernel-doc
> format).
> 
> [...]
> 
> > +	MLNX_UPLOAD_UNCOMB - Check if the device can accept and play
> > +			     "uncombinable" effect and upload the effect into
> > +			     the device's internal memory.
> > +			     Hardware-specific driver should return 0
> > +			     on success.
> > +	MLNX_ERASE_UNCOMB - Remove "uncombinable" effect from device's
> > +			    internal memory.
> > +			    Hardware-specific driver should return 0
> > +			    on success.
> > +	MLNX_START_UNCOMB - Start playback of "uncombinable" effect.
> > +	MLNX_STOP_UNCOMB - Stop playback of "uncombinable" effect.
> 
> These seem to be unused by any drivers?
> 
> I don't think they should be added to the kernel before there is an
> implementation using them (it also makes reviewing them more difficult).

They are used by my dummy driver (link to the source is in the documentation). 
Handling uncombinable effects in lg4ff would be a bit more involved so I 
decided to add support for these in a separate patch series.

> [...]
> 
> > +4. Specifics of "ff-memless-next" for userspace developers
> > +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > +None of the persons involved in development or testing of
> > "ff-memless-next" +is aware of any reference force feedback
> > specifications. "ff-memless-next" +tries to adhere to Microsoft's
> > DirectInput specifications because we +believe that is what most
> > developers have experience with.
> > +
> > +- Waveforms of periodic effects do not start at the origin, but as
> > follows: +	SAW_UP: At minimum
> > +	SAW_DOWN: At maximum
> > +	SQUARE: At maximum
> > +	TRIANGLE: At maximum
> > +	SINE: At origin
> > +
> > +- Updating periodic effects:
> > +	- All periodic effects are restarted when their parameters are 
updated.
> > +
> > +- Updating effects of finite duration:
> > +	- Once an effect of finite length finishes playing, it is considered
> > +	  stopped. Only effects that are playing can be updated on the fly.
> > +	  Therefore effects of finite duration can be updated only until
> > +	  they finish playing.
> 
> Stopped effects should still be able to be updated.
> 
> Anyway, if you want to target userspace developers with this
> information, you should put it in generic documentation ("it is not
> guaranteed that X", etc.). If not, don't say "for userspace developers".
> 
> [...]
> 
> > diff --git a/drivers/input/ff-memless-next.c
> > b/drivers/input/ff-memless-next.c new file mode 100644
> > index 0000000..843a223
> > --- /dev/null
> > +++ b/drivers/input/ff-memless-next.c
> > @@ -0,0 +1,789 @@
> > +/*
> > + * Force feedback support for memoryless devices
> > + *
> > + * This module is based on "ff-memless" orignally written by Anssi
> > Hannula. + * It is extended to support all force feedback effects
> > currently supported + * by the Linux input stack.
> > + *
> > + * Copyright(c) 2013 Michal Maly <madcatxster@prifuk.cz>
> > + *
> > + */
> 
> [...]
> 
> > +static int mlnx_upload(struct input_dev *dev, struct ff_effect *effect,
> > +		       struct ff_effect *old)
> > +{
> 
> [...]
> 
> > +		}
> > +
> > +		mlnx_schedule_playback(mlnxdev);
> > +		spin_unlock_irq(&dev->event_lock);
> > +		return 0;
> 
> The above two lines are unneeded.
Oops...

> > +	}
> > +
> > +	spin_unlock_irq(&dev->event_lock);
> > +
> > +	return 0;
> > +}
> > +
> 
> [...]
> 
> > diff --git a/include/linux/input/ff-memless-next.h
> > b/include/linux/input/ff-memless-next.h new file mode 100644
> > index 0000000..ba89ba1
> > --- /dev/null
> > +++ b/include/linux/input/ff-memless-next.h
> > @@ -0,0 +1,32 @@
> 
> [...]
> 
> > +int input_ff_create_mlnx(struct input_dev *dev, void *data,
> > +			 int(*control_effect)(struct input_dev *, void *, const 
struct
> > mlnx_effect_command *), +			 const u16 update_rate);
> 
> Why is update_rate now driver-selectable?
Because it is my intention to replace ff-memless with MLNX, I want to account 
for (very old?) devices that might have a limit on how many commands they can 
accept within a given timeframe. Update interval in ff-memless is 50 msecs 
whereas Logitech wheels can go as low as 8 msecs. The lower the update 
interval - the better the quality of periodic, ramp and envelope effects. 
Allowing the driver to choose how fast will MLNX generate updates seemed like 
the best way to go.


Your feedback is very much appreciated...

Michal M.

Re: [PATCH v2 1/4] Add ff-memless-next driver

[Michal Malý]

I forgot to comment on this:
> Stopped effects should still be able to be updated.
Fair enough. I could not find a definitive answer to what is expected behavior 
in this case. As far as I can tell, there are following cases:
- Effects with no duration and delay: they shall be updated right away.
- Effects with delay: updated parameters shall be sent to the device at the 
correct time.
- "Expired" effects with finite duration - ??? I assumed that effects with 
finite duration that have already finished playing cannot be updated because 
it does not seem to make much sense to do so. Shall such an effect be 
restarted when it is updated? Is there any guideline I should stick to I am 
not aware of?

Michal M.

Re: [PATCH v2 1/4] Add ff-memless-next driver

[Anssi Hannula]

24.02.2014 03:54, Michal Malý kirjoitti:
> I forgot to comment on this:
>> Stopped effects should still be able to be updated.
> Fair enough. I could not find a definitive answer to what is expected behavior 
> in this case. As far as I can tell, there are following cases:
> - Effects with no duration and delay: they shall be updated right away.
> - Effects with delay: updated parameters shall be sent to the device at the 
> correct time.
> - "Expired" effects with finite duration - ??? I assumed that effects with 
> finite duration that have already finished playing cannot be updated because 
> it does not seem to make much sense to do so.

"Expired" effects can always be played back again later - if that
happens, the updated parameters will be in effect.

> Shall such an effect be 
> restarted when it is updated?

No.

> Is there any guideline I should stick to I am 
> not aware of?

Only "3.7 Dynamic update of an effect" of input/ff.txt.

-- 
Anssi Hannula
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/4] Add ff-memless-next driver

[Michal Malý]

On Monday 24 of February 2014 04:11:04 Anssi Hannula wrote:
> 24.02.2014 03:54, Michal Malý kirjoitti:
> > I forgot to comment on this:
> >> Stopped effects should still be able to be updated.
> > 
> > Fair enough. I could not find a definitive answer to what is expected
> > behavior in this case. As far as I can tell, there are following cases:
> > - Effects with no duration and delay: they shall be updated right away.
> > - Effects with delay: updated parameters shall be sent to the device at
> > the
> > correct time.
> > - "Expired" effects with finite duration - ??? I assumed that effects with
> > finite duration that have already finished playing cannot be updated
> > because it does not seem to make much sense to do so.
> 
> "Expired" effects can always be played back again later - if that
> happens, the updated parameters will be in effect.
Okay, let me make this clear. When an effect "expires", update of its 
parameters shall not trigger a restart, therefore it will not play. However, 
when its parameters are updated and the effect is started again at some point, 
the updated parameters shall be used. I will double-check this just to be on 
the safe side, but MLNX always stores the updated parameters no matter what 
the status of the effect is, so it should behave exactly as you suggest. 
Perhaps it was just me misinterpreting the semantics of "update"?

To bring this to a conclusion:
- Do you think that this work is worth mainlining?
- If it is, what do you suggest I change?

Thanks a lot for your input.

Michal M.
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 2/4] Port hid-lg4ff to ff-memless-next

[Michal Malý]

Port hid-lg4ff to ff-memless-next

Tested-by: Elias Vanderstuyft <elias.vds@gmail.com>
Signed-off-by: Michal Malý <madcatxster@prifuk.cz>
---
 v2: Differentiate between "set force to zero" and "stop force completely"

 drivers/hid/Kconfig     |  2 +-
 drivers/hid/hid-lg4ff.c | 86 +++++++++++++++++++++++++++++--------------------
 2 files changed, 52 insertions(+), 36 deletions(-)

diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index f722001..11ac17e 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -390,7 +390,7 @@ config LOGIG940_FF
 config LOGIWHEELS_FF
 	bool "Logitech wheels configuration and force feedback support"
 	depends on HID_LOGITECH
-	select INPUT_FF_MEMLESS
+	select INPUT_FF_MEMLESS_NEXT
 	default LOGITECH_FF
 	help
 	  Say Y here if you want to enable force feedback and range setting
diff --git a/drivers/hid/hid-lg4ff.c b/drivers/hid/hid-lg4ff.c
index befe0e3..95d2a9f 100644
--- a/drivers/hid/hid-lg4ff.c
+++ b/drivers/hid/hid-lg4ff.c
@@ -25,6 +25,7 @@
 
 
 #include <linux/input.h>
+#include <linux/input/ff-memless-next.h>
 #include <linux/usb.h>
 #include <linux/hid.h>
 
@@ -44,6 +45,8 @@
 #define G27_REV_MAJ 0x12
 #define G27_REV_MIN 0x38
 
+#define FF_UPDATE_RATE 8
+
 #define to_hid_device(pdev) container_of(pdev, struct hid_device, dev)
 
 static void hid_lg4ff_set_range_dfp(struct hid_device *hid, u16 range);
@@ -182,47 +185,60 @@ int lg4ff_adjust_input_event(struct hid_device *hid, struct hid_field *field,
 	}
 }
 
-static int hid_lg4ff_play(struct input_dev *dev, void *data, struct ff_effect *effect)
+static int hid_lg4ff_start_combined(struct hid_device *hid, struct hid_report *report,
+				    const struct mlnx_simple_force *force)
+{
+	__s32 *value = report->field[0]->value;
+	int scaled_x;
+
+	/* Scale down from MLNX range */
+	scaled_x = 0x80 - (force->x * 0xff / 0xffff);
+
+	value[0] = 0x11;	/* Slot 1 */
+	value[1] = 0x08;
+	value[2] = scaled_x;
+	value[3] = 0x80;
+	value[4] = 0x00;
+	value[5] = 0x00;
+	value[6] = 0x00;
+
+	hid_hw_request(hid, report, HID_REQ_SET_REPORT);
+	return 0;
+}
+
+static int hid_lg4ff_stop_combined(struct hid_device *hid, struct hid_report *report)
+{
+	__s32 *value = report->field[0]->value;
+
+	value[0] = 0x13;	/* Slot 1 */
+	value[1] = 0x00;
+	value[2] = 0x00;
+	value[3] = 0x00;
+	value[4] = 0x00;
+	value[5] = 0x00;
+	value[6] = 0x00;
+
+	hid_hw_request(hid, report, HID_REQ_SET_REPORT);
+	return 0;
+}
+
+static int hid_lg4ff_control(struct input_dev *dev, void *data, const struct mlnx_effect_command *command)
 {
 	struct hid_device *hid = input_get_drvdata(dev);
 	struct list_head *report_list = &hid->report_enum[HID_OUTPUT_REPORT].report_list;
 	struct hid_report *report = list_entry(report_list->next, struct hid_report, list);
-	__s32 *value = report->field[0]->value;
-	int x;
-
-#define CLAMP(x) do { if (x < 0) x = 0; else if (x > 0xff) x = 0xff; } while (0)
-
-	switch (effect->type) {
-	case FF_CONSTANT:
-		x = effect->u.ramp.start_level + 0x80;	/* 0x80 is no force */
-		CLAMP(x);
-
-		if (x == 0x80) {
-			/* De-activate force in slot-1*/
-			value[0] = 0x13;
-			value[1] = 0x00;
-			value[2] = 0x00;
-			value[3] = 0x00;
-			value[4] = 0x00;
-			value[5] = 0x00;
-			value[6] = 0x00;
-
-			hid_hw_request(hid, report, HID_REQ_SET_REPORT);
-			return 0;
-		}
-
-		value[0] = 0x11;	/* Slot 1 */
-		value[1] = 0x08;
-		value[2] = x;
-		value[3] = 0x80;
-		value[4] = 0x00;
-		value[5] = 0x00;
-		value[6] = 0x00;
 
-		hid_hw_request(hid, report, HID_REQ_SET_REPORT);
+	switch (command->cmd) {
+	case MLNX_START_COMBINED:
+		return hid_lg4ff_start_combined(hid, report, &command->u.simple_force);
+		break;
+	case MLNX_STOP_COMBINED:
+		return hid_lg4ff_stop_combined(hid, report);
 		break;
+	default:
+		dbg_hid("Unsupported effect command");
+		return -EINVAL;
 	}
-	return 0;
 }
 
 /* Sends default autocentering command compatible with
@@ -608,7 +624,7 @@ int lg4ff_init(struct hid_device *hid)
 	for (j = 0; lg4ff_devices[i].ff_effects[j] >= 0; j++)
 		set_bit(lg4ff_devices[i].ff_effects[j], dev->ffbit);
 
-	error = input_ff_create_memless(dev, NULL, hid_lg4ff_play);
+	error = input_ff_create_mlnx(dev, (void *)NULL, hid_lg4ff_control, FF_UPDATE_RATE);
 
 	if (error)
 		return error;
-- 
1.9.0


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

[PATCH v2 3/4] hid-lg4ff: Add support for periodic effects

[Michal Malý]

hid-lg4ff: Add support for periodic effects

Tested-by: Elias Vanderstuyft <elias.vds@gmail.com>
Signed-off-by: Michal Malý <madcatxster@prifuk.cz>
---
 drivers/hid/hid-lg4ff.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/hid/hid-lg4ff.c b/drivers/hid/hid-lg4ff.c
index 95d2a9f..d7d8d83 100644
--- a/drivers/hid/hid-lg4ff.c
+++ b/drivers/hid/hid-lg4ff.c
@@ -71,6 +71,12 @@ struct lg4ff_device_entry {
 
 static const signed short lg4ff_wheel_effects[] = {
 	FF_CONSTANT,
+	FF_PERIODIC,
+	FF_SQUARE,
+	FF_TRIANGLE,
+	FF_SINE,
+	FF_SAW_UP,
+	FF_SAW_DOWN,
 	FF_AUTOCENTER,
 	-1
 };
-- 
1.9.0


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

[PATCH v2 4/4] hid-lg4ff: Add support for ramp effect

[Michal Malý]

hid-lg4ff: Add support for ramp effect

Tested-by: Elias Vanderstuyft <elias.vds@gmail.com>
Signed-off-by: Michal Malý <madcatxster@prifuk.cz>
---
 drivers/hid/hid-lg4ff.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/hid/hid-lg4ff.c b/drivers/hid/hid-lg4ff.c
index d7d8d83..e5c90bb 100644
--- a/drivers/hid/hid-lg4ff.c
+++ b/drivers/hid/hid-lg4ff.c
@@ -71,6 +71,7 @@ struct lg4ff_device_entry {
 
 static const signed short lg4ff_wheel_effects[] = {
 	FF_CONSTANT,
+	FF_RAMP,
 	FF_PERIODIC,
 	FF_SQUARE,
 	FF_TRIANGLE,
-- 
1.9.0


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

Re: [PATCH v2 0/4] Add ff-memless-next and make hid-lg4ff use it

[Anssi Hannula]

24.02.2014 01:24, Michal Malý kirjoitti:
> Hi everybody,

Hi,

> this patch series is a result of my work to improve FFB support for memoryless
> devices. ff-memless-next is an improvement over the currently available
> ff-memless which is well suited for joypads but cannot handle more advanced
> devices such as racing wheels properly. As I have explained in one of RFCs
> regarding ff-memless-next, the extent of the changes makes implementing
> ff-memless-next as a patch to ff-memless unfeasible. As of now there is a total
> of 27 drivers using ff-memless (including lg4ff) - a lot of them joypads.
> I do not have access to any FFB joypad at the moment so I cannot
> implement the functionality required to handle joypads properly - namely FF_RUMBLE
> and emulation of FF_PERIODIC through FF_RUMBLE.
> The plan is to implement the missing functionality and replace ff-memless completely
> in the future. 

I think we should extend the current ff-memless instead of duplicating
its functionality (even on a "for now" basis).

Having looked at ff-memless-next briefly, it seems very similar to
ff-memless on its basic working principle, and therefore I don't really
see why extending ff-memless would be too cumbersome. Unless I'm missing
something - in that case, feel free to point it out to me :)

Duplicating the module makes reviewing it somewhat difficult since the
changes are not clearly visible.

As for the amount of drivers using ff-memless, those are ~all very
simple (single function call registering a single callback) so it should
be easy to apply any API conversion if needed.
And I don't see a real need for you to have access to a rumble joypad -
that support is already implemented in ff-memless, and other people can
test that it isn't broken by your changes.

Regardless, thanks for looking into this.

> 
> Second part of this series ports lg4ff driver over to ff-memless-next.
> The immediate benefit of this is support of all periodic effects and ramp effect.
> 
> v2 addresses a few issues that have not been noticed at the time v1 was
> submitted. Specific fixes are mentioned in the respective patches.
> 
> Michal M.
> 
>  Michal Malý (4):
>   INPUT: Add ff-memless-next module
>   HID: Port hid-lg4ff to ff-memless-next
>   HID: Add support for periodic effects in hid-lg4ff
>   HID: Add support for ramp effect in hid-lg4ff
> 
>  Documentation/input/ff-memless-next.txt | 141 ++++++
>  drivers/hid/Kconfig                     |   2 +-
>  drivers/hid/hid-lg4ff.c                 |  93 ++--
>  drivers/input/Kconfig                   |  11 +
>  drivers/input/Makefile                  |   1 +
>  drivers/input/ff-memless-next.c         | 789 ++++++++++++++++++++++++++++++++
>  include/linux/input/ff-memless-next.h   |  32 ++
>  7 files changed, 1033 insertions(+), 36 deletions(-)
> --
>  1.9.0
> 
> --
> 


-- 
Anssi Hannula
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 0/4] Add ff-memless-next and make hid-lg4ff use it

[Michal Malý]

On Monday 24 of February 2014 02:32:27 Anssi Hannula wrote:
> 24.02.2014 01:24, Michal Malý kirjoitti:
> > Hi everybody,
> 
> Hi,
> 
> > this patch series is a result of my work to improve FFB support for
> > memoryless devices. ff-memless-next is an improvement over the currently
> > available ff-memless which is well suited for joypads but cannot handle
> > more advanced devices such as racing wheels properly. As I have explained
> > in one of RFCs regarding ff-memless-next, the extent of the changes makes
> > implementing ff-memless-next as a patch to ff-memless unfeasible. As of
> > now there is a total of 27 drivers using ff-memless (including lg4ff) - a
> > lot of them joypads. I do not have access to any FFB joypad at the moment
> > so I cannot
> > implement the functionality required to handle joypads properly - namely
> > FF_RUMBLE and emulation of FF_PERIODIC through FF_RUMBLE.
> > The plan is to implement the missing functionality and replace ff-memless
> > completely in the future.
> 
> I think we should extend the current ff-memless instead of duplicating
> its functionality (even on a "for now" basis).
> 
> Having looked at ff-memless-next briefly, it seems very similar to
> ff-memless on its basic working principle, and therefore I don't really
> see why extending ff-memless would be too cumbersome. Unless I'm missing
> something - in that case, feel free to point it out to me :)

Deciding whether to patch ff-memless or write a new driver from scratch was a 
perfect example of being caught between the rock and a hard place. I am not
particularly fond of the fact that we would have two modules doing pretty much
the same thing. My reasons for writing a separate module were:
- Periodic effects. ff-memless doesn't do "real" periodic effects, it simply 
emulates them through rumble effect. Devices without rumble effect support 
require emulation through constant force effect. Just this was not something 
one could write in one afternoon:)
- Conditional effects. These effects cannot be by nature combined into one 
overall force (at least not easily) so they have to be handled one by one - 
this is a concept ff-memless does not seem to consider. FFB devices have 
limits as to how many conditional (referred to as "uncombinable" in MLNX) 
effects can be active simultaneously, etc.
All in all it seemed less error prone to write a new driver based on the ff-
memless logic, test and deploy it on devices I have access to and once we are 
sure there are no nasty regressions port the rest of the drivers to the new 
API. Given the scope of the changes I am afraid that a "patch" to ff-memless 
would be pretty close to a rewrite anyway.

> Duplicating the module makes reviewing it somewhat difficult since the
> changes are not clearly visible.
> 
> As for the amount of drivers using ff-memless, those are ~all very
> simple (single function call registering a single callback) so it should
> be easy to apply any API conversion if needed.
> And I don't see a real need for you to have access to a rumble joypad -
> that support is already implemented in ff-memless, and other people can
> test that it isn't broken by your changes.
It has been my intention to add handling of rumble effects in a followup 
patch. I wanted to limit the extent of changes I dump in a one massive patch, 
especially when I cannot test the rumble effect on a real hardware.

Michal
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 0/4] Add ff-memless-next and make hid-lg4ff use it

[Elias Vanderstuyft]

On Mon, Feb 24, 2014 at 1:58 AM, Michal Malý <madcatxster@prifuk.cz> wrote:
> On Monday 24 of February 2014 02:32:27 Anssi Hannula wrote:
>>
>> I think we should extend the current ff-memless instead of duplicating
>> its functionality (even on a "for now" basis).
>>
>> Having looked at ff-memless-next briefly, it seems very similar to
>> ff-memless on its basic working principle, and therefore I don't really
>> see why extending ff-memless would be too cumbersome. Unless I'm missing
>> something - in that case, feel free to point it out to me :)
>
> Deciding whether to patch ff-memless or write a new driver from scratch was a
> perfect example of being caught between the rock and a hard place. I am not
> particularly fond of the fact that we would have two modules doing pretty much
> the same thing. My reasons for writing a separate module were:
> - Periodic effects. ff-memless doesn't do "real" periodic effects, it simply
> emulates them through rumble effect. Devices without rumble effect support
> require emulation through constant force effect. Just this was not something
> one could write in one afternoon:)
> - Conditional effects. These effects cannot be by nature combined into one
> overall force (at least not easily) so they have to be handled one by one -
> this is a concept ff-memless does not seem to consider. FFB devices have
> limits as to how many conditional (referred to as "uncombinable" in MLNX)
> effects can be active simultaneously, etc.
> All in all it seemed less error prone to write a new driver based on the ff-
> memless logic, test and deploy it on devices I have access to and once we are
> sure there are no nasty regressions port the rest of the drivers to the new
> API. Given the scope of the changes I am afraid that a "patch" to ff-memless
> would be pretty close to a rewrite anyway.

And add the fact that we already heavily tested the ff-memless-next driver.
Unless you do a diff between the original ff-memless.c and the current
ff-memless-next.c (which will result in a rather unintuitive patch),
it would be a huge waste of time to retest the modified (when doing
efforts to create an intuitive patch) ff-memless-next.c, considered my
total time spend on testing (and not to speak of the time that Michal
spent to fix the corresponding bugs.)
I know that might not be much of an argument, but on the other side,
my motivation to test again from scratch will be much lower (I can't
change much on that, I'm afraid), which would eventually lead to lower
reliability of the final product.

Elias

Re: [PATCH v2 0/4] Add ff-memless-next and make hid-lg4ff use it

[Dmitry Torokhov]

On Mon, Feb 24, 2014 at 10:17:25PM +0100, Elias Vanderstuyft wrote:
> On Mon, Feb 24, 2014 at 1:58 AM, Michal Malý <madcatxster@prifuk.cz> wrote:
> > On Monday 24 of February 2014 02:32:27 Anssi Hannula wrote:
> >>
> >> I think we should extend the current ff-memless instead of duplicating
> >> its functionality (even on a "for now" basis).
> >>
> >> Having looked at ff-memless-next briefly, it seems very similar to
> >> ff-memless on its basic working principle, and therefore I don't really
> >> see why extending ff-memless would be too cumbersome. Unless I'm missing
> >> something - in that case, feel free to point it out to me :)
> >
> > Deciding whether to patch ff-memless or write a new driver from scratch was a
> > perfect example of being caught between the rock and a hard place. I am not
> > particularly fond of the fact that we would have two modules doing pretty much
> > the same thing. My reasons for writing a separate module were:
> > - Periodic effects. ff-memless doesn't do "real" periodic effects, it simply
> > emulates them through rumble effect. Devices without rumble effect support
> > require emulation through constant force effect. Just this was not something
> > one could write in one afternoon:)
> > - Conditional effects. These effects cannot be by nature combined into one
> > overall force (at least not easily) so they have to be handled one by one -
> > this is a concept ff-memless does not seem to consider. FFB devices have
> > limits as to how many conditional (referred to as "uncombinable" in MLNX)
> > effects can be active simultaneously, etc.
> > All in all it seemed less error prone to write a new driver based on the ff-
> > memless logic, test and deploy it on devices I have access to and once we are
> > sure there are no nasty regressions port the rest of the drivers to the new
> > API. Given the scope of the changes I am afraid that a "patch" to ff-memless
> > would be pretty close to a rewrite anyway.
> 
> And add the fact that we already heavily tested the ff-memless-next driver.
> Unless you do a diff between the original ff-memless.c and the current
> ff-memless-next.c (which will result in a rather unintuitive patch),
> it would be a huge waste of time to retest the modified (when doing
> efforts to create an intuitive patch) ff-memless-next.c, considered my
> total time spend on testing (and not to speak of the time that Michal
> spent to fix the corresponding bugs.)
> I know that might not be much of an argument, but on the other side,
> my motivation to test again from scratch will be much lower (I can't
> change much on that, I'm afraid), which would eventually lead to lower
> reliability of the final product.

On the other hand having 2 drivers implementing very similar
functionality would lead to general confusion as to which one should be
used; they will also have to be maintained.

I would rather see them merged into one driver providing necessary
services to all memoryless FF devices.

Thanks.

-- 
Dmitry

Re: [PATCH v2 0/4] Add ff-memless-next and make hid-lg4ff use it

[Michal Malý]

On Monday 24 of February 2014 13:48:18 Dmitry Torokhov wrote:
> On Mon, Feb 24, 2014 at 10:17:25PM +0100, Elias Vanderstuyft wrote:
> > On Mon, Feb 24, 2014 at 1:58 AM, Michal Malý <madcatxster@prifuk.cz> 
wrote:
> > > On Monday 24 of February 2014 02:32:27 Anssi Hannula wrote:
> > >> I think we should extend the current ff-memless instead of duplicating
> > >> its functionality (even on a "for now" basis).
> > >> 
> > >> Having looked at ff-memless-next briefly, it seems very similar to
> > >> ff-memless on its basic working principle, and therefore I don't really
> > >> see why extending ff-memless would be too cumbersome. Unless I'm
> > >> missing
> > >> something - in that case, feel free to point it out to me :)
> > > 
> > > Deciding whether to patch ff-memless or write a new driver from scratch
> > > was a perfect example of being caught between the rock and a hard
> > > place. I am not particularly fond of the fact that we would have two
> > > modules doing pretty much the same thing. My reasons for writing a
> > > separate module were:
> > > - Periodic effects. ff-memless doesn't do "real" periodic effects, it
> > > simply emulates them through rumble effect. Devices without rumble
> > > effect support require emulation through constant force effect. Just
> > > this was not something one could write in one afternoon:)
> > > - Conditional effects. These effects cannot be by nature combined into
> > > one
> > > overall force (at least not easily) so they have to be handled one by
> > > one -
> > > this is a concept ff-memless does not seem to consider. FFB devices have
> > > limits as to how many conditional (referred to as "uncombinable" in
> > > MLNX)
> > > effects can be active simultaneously, etc.
> > > All in all it seemed less error prone to write a new driver based on the
> > > ff- memless logic, test and deploy it on devices I have access to and
> > > once we are sure there are no nasty regressions port the rest of the
> > > drivers to the new API. Given the scope of the changes I am afraid that
> > > a "patch" to ff-memless would be pretty close to a rewrite anyway.
> > 
> > And add the fact that we already heavily tested the ff-memless-next
> > driver.
> > Unless you do a diff between the original ff-memless.c and the current
> > ff-memless-next.c (which will result in a rather unintuitive patch),
> > it would be a huge waste of time to retest the modified (when doing
> > efforts to create an intuitive patch) ff-memless-next.c, considered my
> > total time spend on testing (and not to speak of the time that Michal
> > spent to fix the corresponding bugs.)
> > I know that might not be much of an argument, but on the other side,
> > my motivation to test again from scratch will be much lower (I can't
> > change much on that, I'm afraid), which would eventually lead to lower
> > reliability of the final product.
> 
> On the other hand having 2 drivers implementing very similar
> functionality would lead to general confusion as to which one should be
> used; they will also have to be maintained.
> 
> I would rather see them merged into one driver providing necessary
> services to all memoryless FF devices.
> 
> Thanks.

Very well. It that case I guess the most sensible thing to do would be to add 
FF_RUMBLE to ff-memless-next and replace ff-memless completely. As Anssi 
pointed out a lot of the drivers that currently use ff-memless are very simple 
so any risk of breakage will hopefully be minimal.

As I don't have any device with rumble effect support I'll appreciate help on 
this front

Michal M.
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html