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; 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) + turn_off(vib); + else + turn_on(vib, 100); +} + +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); + 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; + gb_pm_runtime_put_autosuspend(bundle);
return 0;
+err_device_remove: + device_unregister(vib->dev); err_ida_remove: ida_simple_remove(&minors, vib->minor); err_connection_disable:
No need for a custom sysfs api for the greybus vibrator driver now that it is hooked up to the kernel's input layer, so rip it out.
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 | 125 ++--------------------------- 1 file changed, 5 insertions(+), 120 deletions(-)
diff --git a/drivers/staging/greybus/vibrator.c b/drivers/staging/greybus/vibrator.c index 94110cadb5bd..d93c8f7e1bd6 100644 --- a/drivers/staging/greybus/vibrator.c +++ b/drivers/staging/greybus/vibrator.c @@ -19,9 +19,6 @@ 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; @@ -45,7 +42,7 @@ static int turn_off(struct gb_vibrator_device *vib) return ret; }
-static int turn_on(struct gb_vibrator_device *vib, u16 timeout_ms) +static int turn_on(struct gb_vibrator_device *vib) { struct gb_bundle *bundle = vib->connection->bundle; int ret; @@ -54,10 +51,6 @@ static int turn_on(struct gb_vibrator_device *vib, u16 timeout_ms) if (ret) return ret;
- /* Vibrator was switched ON earlier */ - if (cancel_delayed_work_sync(&vib->delayed_work)) - turn_off(vib); - ret = gb_operation_sync(vib->connection, GB_VIBRATOR_TYPE_ON, NULL, 0, NULL, 0); if (ret) { @@ -66,8 +59,6 @@ static int turn_on(struct gb_vibrator_device *vib, u16 timeout_ms) }
vib->on = true; - schedule_delayed_work(&vib->delayed_work, msecs_to_jiffies(timeout_ms)); - return 0; }
@@ -79,7 +70,7 @@ static void gb_vibrator_play_work(struct work_struct *work) if (vib->running) turn_off(vib); else - turn_on(vib, 100); + turn_on(vib); }
static int gb_vibrator_play_effect(struct input_dev *input, void *data, @@ -101,68 +92,17 @@ 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); - struct gb_vibrator_device *vib = - container_of(delayed_work, - struct gb_vibrator_device, - delayed_work); - - turn_off(vib); -} - -static ssize_t timeout_store(struct device *dev, struct device_attribute *attr, - const char *buf, size_t count) -{ - struct gb_vibrator_device *vib = dev_get_drvdata(dev); - unsigned long val; - int retval; - - retval = kstrtoul(buf, 10, &val); - if (retval < 0) { - dev_err(dev, "could not parse timeout value %d\n", retval); - return retval; - } - - if (val) - retval = turn_on(vib, (u16)val); - else - retval = turn_off(vib); - if (retval) - return retval; - - return count; -} -static DEVICE_ATTR_WO(timeout); - -static struct attribute *vibrator_attrs[] = { - &dev_attr_timeout.attr, - NULL, -}; -ATTRIBUTE_GROUPS(vibrator); - -static struct class vibrator_class = { - .name = "vibrator", - .owner = THIS_MODULE, - .dev_groups = vibrator_groups, -}; - -static DEFINE_IDA(minors); - static int gb_vibrator_probe(struct gb_bundle *bundle, const struct greybus_bundle_id *id) { struct greybus_descriptor_cport *cport_desc; struct gb_connection *connection; struct gb_vibrator_device *vib; - struct device *dev; int retval;
if (bundle->num_cports != 1) @@ -192,26 +132,6 @@ static int gb_vibrator_probe(struct gb_bundle *bundle, if (retval) goto err_connection_destroy;
- /* - * For now we create a device in sysfs for the vibrator, but odds are - * there is a "real" device somewhere in the kernel for this, but I - * can't find it at the moment... - */ - vib->minor = ida_simple_get(&minors, 0, 0, GFP_KERNEL); - if (vib->minor < 0) { - retval = vib->minor; - goto err_connection_disable; - } - dev = device_create(&vibrator_class, &bundle->dev, - MKDEV(0, 0), vib, "vibrator%d", vib->minor); - if (IS_ERR(dev)) { - retval = -EINVAL; - goto err_ida_remove; - } - vib->dev = dev; - - INIT_DELAYED_WORK(&vib->delayed_work, gb_vibrator_worker); - INIT_WORK(&vib->play_work, gb_vibrator_play_work); vib->input->name = "greybus-vibrator"; vib->input->close = gb_vibrator_close; @@ -224,16 +144,12 @@ static int gb_vibrator_probe(struct gb_bundle *bundle, retval = input_ff_create_memless(vib->input, NULL, gb_vibrator_play_effect); if (retval) - goto err_device_remove; + goto err_connection_disable;
gb_pm_runtime_put_autosuspend(bundle);
return 0;
-err_device_remove: - device_unregister(vib->dev); -err_ida_remove: - ida_simple_remove(&minors, vib->minor); err_connection_disable: gb_connection_disable(connection); err_connection_destroy: @@ -253,11 +169,8 @@ static void gb_vibrator_disconnect(struct gb_bundle *bundle) if (ret) gb_pm_runtime_get_noresume(bundle);
- if (cancel_delayed_work_sync(&vib->delayed_work)) - turn_off(vib); + turn_off(vib);
- device_unregister(vib->dev); - ida_simple_remove(&minors, vib->minor); gb_connection_disable(vib->connection); gb_connection_destroy(vib->connection); kfree(vib); @@ -275,34 +188,6 @@ static struct greybus_driver gb_vibrator_driver = { .disconnect = gb_vibrator_disconnect, .id_table = gb_vibrator_id_table, }; - -static __init int gb_vibrator_init(void) -{ - int retval; - - retval = class_register(&vibrator_class); - if (retval) - return retval; - - retval = greybus_register(&gb_vibrator_driver); - if (retval) - goto err_class_unregister; - - return 0; - -err_class_unregister: - class_unregister(&vibrator_class); - - return retval; -} -module_init(gb_vibrator_init); - -static __exit void gb_vibrator_exit(void) -{ - greybus_deregister(&gb_vibrator_driver); - class_unregister(&vibrator_class); - ida_destroy(&minors); -} -module_exit(gb_vibrator_exit); +module_greybus_driver(gb_vibrator_driver);
MODULE_LICENSE("GPL v2");
Hi Greg,
I love your patch! Yet something to improve:
[auto build test ERROR on staging/staging-testing]
url: https://github.com/0day-ci/linux/commits/Greg-Kroah-Hartman/staging-greybus-... base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git 1e9a9c7cba3ca5cbd3201a9f3b8dc6e8d7bef1c0 config: i386-randconfig-a013-20210105 (attached as .config) compiler: gcc-9 (Debian 9.3.0-15) 9.3.0 reproduce (this is a W=1 build): # https://github.com/0day-ci/linux/commit/4bfe9c5dc24fa35861d281448fd4091f652c... git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Greg-Kroah-Hartman/staging-greybus-vibrator-use-proper-API-for-vibrator-devices/20210105-232001 git checkout 4bfe9c5dc24fa35861d281448fd4091f652c2076 # save the attached .config to linux build tree make W=1 ARCH=i386
If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot lkp@intel.com
All errors (new ones prefixed by >>):
ld: drivers/staging/greybus/vibrator.o: in function `gb_vibrator_probe':
drivers/staging/greybus/vibrator.c:224: undefined reference to `input_ff_create_memless'
vim +224 drivers/staging/greybus/vibrator.c
158 159 static int gb_vibrator_probe(struct gb_bundle *bundle, 160 const struct greybus_bundle_id *id) 161 { 162 struct greybus_descriptor_cport *cport_desc; 163 struct gb_connection *connection; 164 struct gb_vibrator_device *vib; 165 struct device *dev; 166 int retval; 167 168 if (bundle->num_cports != 1) 169 return -ENODEV; 170 171 cport_desc = &bundle->cport_desc[0]; 172 if (cport_desc->protocol_id != GREYBUS_PROTOCOL_VIBRATOR) 173 return -ENODEV; 174 175 vib = kzalloc(sizeof(*vib), GFP_KERNEL); 176 if (!vib) 177 return -ENOMEM; 178 179 connection = gb_connection_create(bundle, le16_to_cpu(cport_desc->id), 180 NULL); 181 if (IS_ERR(connection)) { 182 retval = PTR_ERR(connection); 183 goto err_free_vib; 184 } 185 gb_connection_set_data(connection, vib); 186 187 vib->connection = connection; 188 189 greybus_set_drvdata(bundle, vib); 190 191 retval = gb_connection_enable(connection); 192 if (retval) 193 goto err_connection_destroy; 194 195 /* 196 * For now we create a device in sysfs for the vibrator, but odds are 197 * there is a "real" device somewhere in the kernel for this, but I 198 * can't find it at the moment... 199 */ 200 vib->minor = ida_simple_get(&minors, 0, 0, GFP_KERNEL); 201 if (vib->minor < 0) { 202 retval = vib->minor; 203 goto err_connection_disable; 204 } 205 dev = device_create(&vibrator_class, &bundle->dev, 206 MKDEV(0, 0), vib, "vibrator%d", vib->minor); 207 if (IS_ERR(dev)) { 208 retval = -EINVAL; 209 goto err_ida_remove; 210 } 211 vib->dev = dev; 212 213 INIT_DELAYED_WORK(&vib->delayed_work, gb_vibrator_worker); 214 215 INIT_WORK(&vib->play_work, gb_vibrator_play_work); 216 vib->input->name = "greybus-vibrator"; 217 vib->input->close = gb_vibrator_close; 218 vib->input->dev.parent = &bundle->dev; 219 vib->input->id.bustype = BUS_HOST; 220 221 input_set_drvdata(vib->input, vib); 222 input_set_capability(vib->input, EV_FF, FF_RUMBLE); 223
224 retval = input_ff_create_memless(vib->input, NULL,
225 gb_vibrator_play_effect); 226 if (retval) 227 goto err_device_remove; 228 229 gb_pm_runtime_put_autosuspend(bundle); 230 231 return 0; 232 233 err_device_remove: 234 device_unregister(vib->dev); 235 err_ida_remove: 236 ida_simple_remove(&minors, vib->minor); 237 err_connection_disable: 238 gb_connection_disable(connection); 239 err_connection_destroy: 240 gb_connection_destroy(connection); 241 err_free_vib: 242 kfree(vib); 243 244 return retval; 245 } 246
--- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
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;- gb_pm_runtime_put_autosuspend(bundle);
return 0; +err_device_remove:
- device_unregister(vib->dev);
I know you're removing this in the next patch, but as the class device has been registered you need to cancel the delayed work and turn off the motor here too (side note: looks like this is done in the wrong order in remove()).
err_ida_remove: ida_simple_remove(&minors, vib->minor); err_connection_disable:
Johan
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()).
gb_pm_runtime_put_autosuspend(bundle); return 0; +err_device_remove:
- device_unregister(vib->dev);
I know you're removing this in the next patch, but as the class device has been registered you need to cancel the delayed work and turn off the motor here too (side note: looks like this is done in the wrong order in remove()).
err_ida_remove: ida_simple_remove(&minors, vib->minor); err_connection_disable:
Johan
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
-
Greg Kroah-Hartman -
Johan Hovold -
kernel test robot