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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel



[Index of Archives]     [Linux Driver Backports]     [DMA Engine]     [Linux GPIO]     [Linux SPI]     [Video for Linux]     [Linux USB Devel]     [Linux Coverity]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux