Re: [PATCH 1/2] staging: greybus: vibrator: use proper API for vibrator devices

[Johan Hovold]

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