On Wed, Jan 06, 2021 at 01:08:15PM +0100, Johan Hovold wrote:
On Wed, Jan 06, 2021 at 01:04:04PM +0100, Johan Hovold wrote:
On Tue, Jan 05, 2021 at 04:19:02PM +0100, Greg Kroah-Hartman wrote:
The correct user/kernel api for vibrator devices is the Input rumble api, not a random sysfs file like the greybus vibrator driver currently uses.
Add support for the correct input api to the vibrator driver so that it hooks up to the kernel and userspace correctly.
Cc: Johan Hovold johan@kernel.org Cc: Alex Elder elder@kernel.org Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org
drivers/staging/greybus/vibrator.c | 59 ++++++++++++++++++++++++++++++ 1 file changed, 59 insertions(+)
diff --git a/drivers/staging/greybus/vibrator.c b/drivers/staging/greybus/vibrator.c index 0e2b188e5ca3..94110cadb5bd 100644 --- a/drivers/staging/greybus/vibrator.c +++ b/drivers/staging/greybus/vibrator.c @@ -13,13 +13,18 @@ #include <linux/kdev_t.h> #include <linux/idr.h> #include <linux/pm_runtime.h> +#include <linux/input.h> #include <linux/greybus.h> struct gb_vibrator_device { struct gb_connection *connection;
- struct input_dev *input; struct device *dev; int minor; /* vibrator minor number */ struct delayed_work delayed_work;
- bool running;
- bool on;
- struct work_struct play_work;
}; /* Greybus Vibrator operation types */ @@ -36,6 +41,7 @@ static int turn_off(struct gb_vibrator_device *vib) gb_pm_runtime_put_autosuspend(bundle);
- vib->on = false;
You update but never seem to actually use vib->on.
return ret; } @@ -59,11 +65,48 @@ static int turn_on(struct gb_vibrator_device *vib, u16 timeout_ms) return ret; }
- vib->on = true; schedule_delayed_work(&vib->delayed_work, msecs_to_jiffies(timeout_ms));
return 0; } +static void gb_vibrator_play_work(struct work_struct *work) +{
- struct gb_vibrator_device *vib =
container_of(work, struct gb_vibrator_device, play_work);- if (vib->running)
Is this test inverted?
turn_off(vib);- else
turn_on(vib, 100);Why 100 ms?
Shouldn't it just be left on indefinitely with the new API?
+}
+static int gb_vibrator_play_effect(struct input_dev *input, void *data,
struct ff_effect *effect)+{
- struct gb_vibrator_device *vib = input_get_drvdata(input);
- int level;
- level = effect->u.rumble.strong_magnitude;
- if (!level)
level = effect->u.rumble.weak_magnitude;- vib->running = level;
- schedule_work(&vib->play_work);
- return 0;
+}
+static void gb_vibrator_close(struct input_dev *input) +{
- struct gb_vibrator_device *vib = input_get_drvdata(input);
- cancel_delayed_work_sync(&vib->delayed_work);
- cancel_work_sync(&vib->play_work);
- turn_off(vib);
- vib->running = false;
+}
static void gb_vibrator_worker(struct work_struct *work) { struct delayed_work *delayed_work = to_delayed_work(work); @@ -169,10 +212,26 @@ static int gb_vibrator_probe(struct gb_bundle *bundle, INIT_DELAYED_WORK(&vib->delayed_work, gb_vibrator_worker);
- INIT_WORK(&vib->play_work, gb_vibrator_play_work);
Hmm. Looks like you forgot to actually allocate the input device...
- vib->input->name = "greybus-vibrator";
- vib->input->close = gb_vibrator_close;
- vib->input->dev.parent = &bundle->dev;
- vib->input->id.bustype = BUS_HOST;
- input_set_drvdata(vib->input, vib);
- input_set_capability(vib->input, EV_FF, FF_RUMBLE);
- retval = input_ff_create_memless(vib->input, NULL,
gb_vibrator_play_effect);- if (retval)
goto err_device_remove;Oh, and you forgot to register the input device here too (and deregister in remove()).
Ugh, wow, this patch series is messed up, sorry about that, and thanks for the review. I'll fix this up when I next get a chance and resend.
greg k-h