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@xxxxxxxxxx>
> > Cc: Alex Elder <elder@xxxxxxxxxx>
> > Signed-off-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> > ---
> > 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
_______________________________________________
greybus-dev mailing list
greybus-dev@xxxxxxxxxxxxxxxx
https://lists.linaro.org/mailman/listinfo/greybus-dev
|