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@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()).
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
_______________________________________________
greybus-dev mailing list
greybus-dev@xxxxxxxxxxxxxxxx
https://lists.linaro.org/mailman/listinfo/greybus-dev
|