LKML Archive on lore.kernel.org
 help / color / Atom feed
From: Luca Weiss <luca@z3ntu.xyz>
To: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Rob Herring <robh+dt@kernel.org>,
	Mark Rutland <mark.rutland@arm.com>,
	Mauro Carvalho Chehab <mchehab+samsung@kernel.org>,
	Pascal PAILLET-LME <p.paillet@st.com>, Coly Li <colyli@suse.de>,
	Lee Jones <lee.jones@linaro.org>,
	Xiaotong Lu <xiaotong.lu@spreadtrum.com>,
	Brian Masney <masneyb@onstation.org>,
	Rob Herring <robh@kernel.org>,
	Baolin Wang <baolin.wang@linaro.org>,
	Andy Gross <andy.gross@linaro.org>,
	David Brown <david.brown@linaro.org>,
	"open list:ARM/QUALCOMM SUPPORT" <linux-arm-msm@vger.kernel.org>,
	"open list:INPUT (KEYBOARD, MOUSE, JOYSTICK,
	TOUCHSCREEN)..."  <linux-input@vger.kernel.org>,
	"open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
	<devicetree@vger.kernel.org>,
	open list <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2 2/3] Input: add a driver for GPIO controllable vibrators
Date: Tue, 09 Apr 2019 13:48:35 +0200
Message-ID: <2691889.jnjB3NjM6o@g550jk> (raw)
In-Reply-To: <20190407221857.GB162359@dtor-ws>

Hello Dmitry,

see inline comments - and:
your last two comments should probably be also applied to the pwm-vibra.c 
driver, as that's where these code blocks come from.

On Montag, 8. April 2019 00:18:57 CEST Dmitry Torokhov wrote:
> Hi Luca,
> 
> On Sun, Apr 07, 2019 at 05:58:41PM +0200, Luca Weiss wrote:
> > Provide a simple driver for GPIO controllable vibrators.
> > It will be used by the Fairphone 2.
> > 
> > Signed-off-by: Luca Weiss <luca@z3ntu.xyz>
> > ---
> > 
> >  drivers/input/misc/Kconfig      |  12 ++
> >  drivers/input/misc/Makefile     |   1 +
> >  drivers/input/misc/gpio-vibra.c | 214 ++++++++++++++++++++++++++++++++
> >  3 files changed, 227 insertions(+)
> >  create mode 100644 drivers/input/misc/gpio-vibra.c
> > 
> > diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
> > index e15ed1bb8558..6dfe9e2fe5b1 100644
> > --- a/drivers/input/misc/Kconfig
> > +++ b/drivers/input/misc/Kconfig
> > @@ -290,6 +290,18 @@ config INPUT_GPIO_DECODER
> > 
> >  	 To compile this driver as a module, choose M here: the module
> >  	 will be called gpio_decoder.
> > 
> > +config INPUT_GPIO_VIBRA
> > +	tristate "GPIO vibrator support"
> > +	depends on GPIOLIB || COMPILE_TEST
> > +	select INPUT_FF_MEMLESS
> > +	help
> > +	  Say Y here to get support for GPIO based vibrator devices.
> > +
> > +	  If unsure, say N.
> > +
> > +	  To compile this driver as a module, choose M here: the module 
will be
> > +	  called gpio-vibra.
> > +
> > 
> >  config INPUT_IXP4XX_BEEPER
> >  
> >  	tristate "IXP4XX Beeper support"
> >  	depends on ARCH_IXP4XX
> > 
> > diff --git a/drivers/input/misc/Makefile b/drivers/input/misc/Makefile
> > index b936c5b1d4ac..f38ebbdb05e2 100644
> > --- a/drivers/input/misc/Makefile
> > +++ b/drivers/input/misc/Makefile
> > @@ -36,6 +36,7 @@ obj-$(CONFIG_INPUT_DRV2667_HAPTICS)	+= drv2667.o
> > 
> >  obj-$(CONFIG_INPUT_GP2A)		+= gp2ap002a00f.o
> >  obj-$(CONFIG_INPUT_GPIO_BEEPER)		+= gpio-beeper.o
> >  obj-$(CONFIG_INPUT_GPIO_DECODER)	+= gpio_decoder.o
> > 
> > +obj-$(CONFIG_INPUT_GPIO_VIBRA)		+= gpio-vibra.o
> > 
> >  obj-$(CONFIG_INPUT_HISI_POWERKEY)	+= hisi_powerkey.o
> >  obj-$(CONFIG_HP_SDC_RTC)		+= hp_sdc_rtc.o
> >  obj-$(CONFIG_INPUT_IMS_PCU)		+= ims-pcu.o
> > 
> > diff --git a/drivers/input/misc/gpio-vibra.c
> > b/drivers/input/misc/gpio-vibra.c new file mode 100644
> > index 000000000000..14f9534668c8
> > --- /dev/null
> > +++ b/drivers/input/misc/gpio-vibra.c
> > @@ -0,0 +1,214 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + *  GPIO vibrator driver
> > + *
> > + *  Copyright (C) 2019 Luca Weiss <luca@z3ntu.xyz>
> > + *
> > + *  Based on PWM vibrator driver:
> > + *  Copyright (C) 2017 Collabora Ltd.
> > + *
> > + *  Based on previous work from:
> > + *  Copyright (C) 2012 Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > + *
> > + *  Based on PWM beeper driver:
> > + *  Copyright (C) 2010, Lars-Peter Clausen <lars@metafoo.de>
> > + */
> > +
> > +#include <linux/gpio/consumer.h>
> > +#include <linux/input.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/of_device.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/property.h>
> > +#include <linux/regulator/consumer.h>
> > +#include <linux/slab.h>
> > +
> > +enum vibrator_state {
> > +	VIBRATOR_OFF,
> > +	VIBRATOR_ON
> > +};
> 
> I'd probably go with simple "bool running".
Changed.
> 
> > +
> > +struct gpio_vibrator {
> > +	struct input_dev *input;
> > +	struct gpio_desc *gpio;
> > +	struct regulator *vcc;
> > +
> > +	struct work_struct play_work;
> > +	enum vibrator_state state;
> > +	bool vcc_on;
> > +};
> > +
> > +static int gpio_vibrator_start(struct gpio_vibrator *vibrator)
> > +{
> > +	struct device *pdev = vibrator->input->dev.parent;
> > +	int err;
> > +
> > +	if (!vibrator->vcc_on) {
> > +		err = regulator_enable(vibrator->vcc);
> > +		if (err) {
> > +			dev_err(pdev, "failed to enable regulator: 
%d", err);
> > +			return err;
> > +		}
> > +		vibrator->vcc_on = true;
> > +	}
> > +
> > +	gpiod_set_value(vibrator->gpio, 1);
> 
> Since this is called by work item and thus from context that can sleep,
> we can use gpiod_set_value_cansleep() so that "slow" GPIOs can be used
> with the driver.
Changed.
> 
> > +
> > +	return 0;
> > +}
> > +
> > +static void gpio_vibrator_stop(struct gpio_vibrator *vibrator)
> > +{
> > +	gpiod_set_value(vibrator->gpio, 0);
> > +
> > +	if (vibrator->vcc_on) {
> > +		regulator_disable(vibrator->vcc);
> > +		vibrator->vcc_on = false;
> > +	}
> > +}
> > +
> > +static void gpio_vibrator_play_work(struct work_struct *work)
> > +{
> > +	struct gpio_vibrator *vibrator = container_of(work,
> > +					struct gpio_vibrator, 
play_work);
> > +
> > +	if (vibrator->state == VIBRATOR_ON)
> > +		gpio_vibrator_start(vibrator);
> > +	else
> > +		gpio_vibrator_stop(vibrator);
> > +}
> > +
> > +static int gpio_vibrator_play_effect(struct input_dev *dev, void *data,
> > +				    struct ff_effect *effect)
> > +{
> > +	struct gpio_vibrator *vibrator = input_get_drvdata(dev);
> > +
> > +	int level = effect->u.rumble.strong_magnitude;
> > +
> > +	if (!level)
> > +		level = effect->u.rumble.weak_magnitude;
> > +
> > +	if (level)
> > +		vibrator->state = VIBRATOR_ON;
> > +	else
> > +		vibrator->state = VIBRATOR_OFF;
> > +
> > +	schedule_work(&vibrator->play_work);
> > +
> > +	return 0;
> > +}
> > +
> > +static void gpio_vibrator_close(struct input_dev *input)
> > +{
> > +	struct gpio_vibrator *vibrator = input_get_drvdata(input);
> > +
> > +	cancel_work_sync(&vibrator->play_work);
> > +	gpio_vibrator_stop(vibrator);
> 
> Do you want to update vibrator->state here?
I've looked at that a bit and it makes sense when the vibrator is on, the 
input it closed (and the vibrator stopped but the variable is still 'on'), 
then the device is suspended and resumed, the vibrator will be turned on 
again, so I'll change that.
> 
> > +}
> > +
> > +static int gpio_vibrator_probe(struct platform_device *pdev)
> > +{
> > +	struct gpio_vibrator *vibrator;
> > +	int err;
> > +
> > +	vibrator = devm_kzalloc(&pdev->dev, sizeof(*vibrator), 
GFP_KERNEL);
> > +	if (!vibrator)
> > +		return -ENOMEM;
> > +
> > +	vibrator->input = devm_input_allocate_device(&pdev->dev);
> > +	if (!vibrator->input)
> > +		return -ENOMEM;
> > +
> > +	vibrator->vcc = devm_regulator_get(&pdev->dev, "vcc");
> > +	err = PTR_ERR_OR_ZERO(vibrator->vcc);
> > +	if (err) {
> > +		if (err != -EPROBE_DEFER)
> > +			dev_err(&pdev->dev, "Failed to request 
regulator: %d",
> > +				err);
> > +		return err;
> > +	}
> > +
> > +	vibrator->gpio = devm_gpiod_get(&pdev->dev, "enable", 
GPIOD_OUT_LOW);
> > +	err = PTR_ERR_OR_ZERO(vibrator->gpio);
> > +	if (err) {
> > +		if (err != -EPROBE_DEFER)
> > +			dev_err(&pdev->dev, "Failed to request main 
gpio: %d",
> > +				err);
> > +		return err;
> > +	}
> > +
> > +	INIT_WORK(&vibrator->play_work, gpio_vibrator_play_work);
> > +
> > +	vibrator->input->name = "gpio-vibrator";
> > +	vibrator->input->id.bustype = BUS_HOST;
> > +	vibrator->input->dev.parent = &pdev->dev;
> 
> No need to set as you used devm_input_device_alloc().
Changed.
> > +	vibrator->input->close = gpio_vibrator_close;
> > +
> > +	input_set_drvdata(vibrator->input, vibrator);
> > +	input_set_capability(vibrator->input, EV_FF, FF_RUMBLE);
> > +
> > +	err = input_ff_create_memless(vibrator->input, NULL,
> > +				      gpio_vibrator_play_effect);
> > +	if (err) {
> > +		dev_err(&pdev->dev, "Couldn't create FF dev: %d", err);
> > +		return err;
> > +	}
> > +
> > +	err = input_register_device(vibrator->input);
> > +	if (err) {
> > +		dev_err(&pdev->dev, "Couldn't register input dev: %d", 
err);
> > +		return err;
> > +	}
> > +
> > +	platform_set_drvdata(pdev, vibrator);
> > +
> > +	return 0;
> > +}
> > +
> > +static int __maybe_unused gpio_vibrator_suspend(struct device *dev)
> > +{
> > +	struct gpio_vibrator *vibrator = dev_get_drvdata(dev);
> > +
> > +	cancel_work_sync(&vibrator->play_work);
> > +	if (vibrator->state == VIBRATOR_ON)
> > +		gpio_vibrator_stop(vibrator);
> > +
> > +	return 0;
> > +}
> > +
> > +static int __maybe_unused gpio_vibrator_resume(struct device *dev)
> > +{
> > +	struct gpio_vibrator *vibrator = dev_get_drvdata(dev);
> > +
> > +	if (vibrator->state == VIBRATOR_ON)
> > +		gpio_vibrator_start(vibrator);
> > +
> > +	return 0;
> > +}
> > +
> > +static SIMPLE_DEV_PM_OPS(gpio_vibrator_pm_ops,
> > +			 gpio_vibrator_suspend, 
gpio_vibrator_resume);
> > +
> > +#ifdef CONFIG_OF
> > +static const struct of_device_id gpio_vibra_dt_match_table[] = {
> > +	{ .compatible = "gpio-vibrator" },
> > +	{},
> > +};
> > +MODULE_DEVICE_TABLE(of, gpio_vibra_dt_match_table);
> > +#endif
> > +
> > +static struct platform_driver gpio_vibrator_driver = {
> > +	.probe	= gpio_vibrator_probe,
> > +	.driver	= {
> > +		.name	= "gpio-vibrator",
> > +		.pm	= &gpio_vibrator_pm_ops,
> > +		.of_match_table = 
of_match_ptr(gpio_vibra_dt_match_table),
> > +	},
> > +};
> > +module_platform_driver(gpio_vibrator_driver);
> > +
> > +MODULE_AUTHOR("Luca Weiss <luca@z3ntu.xy>");
> > +MODULE_DESCRIPTION("GPIO vibrator driver");
> > +MODULE_LICENSE("GPL");
> > +MODULE_ALIAS("platform:gpio-vibrator");
> 
> Thanks.

Thank you for the review, will send a v3 soon!
Luca



  reply index

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-04-07 15:58 [PATCH v2 1/3] dt-bindings: input: add GPIO controllable vibrator Luca Weiss
2019-04-07 15:58 ` [PATCH v2 2/3] Input: add a driver for GPIO controllable vibrators Luca Weiss
2019-04-07 22:18   ` Dmitry Torokhov
2019-04-09 11:48     ` Luca Weiss [this message]
2019-04-07 15:58 ` [PATCH v2 3/3] ARM: dts: msm8974-FP2: Add vibration motor Luca Weiss

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=2691889.jnjB3NjM6o@g550jk \
    --to=luca@z3ntu.xyz \
    --cc=andy.gross@linaro.org \
    --cc=baolin.wang@linaro.org \
    --cc=colyli@suse.de \
    --cc=david.brown@linaro.org \
    --cc=devicetree@vger.kernel.org \
    --cc=dmitry.torokhov@gmail.com \
    --cc=lee.jones@linaro.org \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-input@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mark.rutland@arm.com \
    --cc=masneyb@onstation.org \
    --cc=mchehab+samsung@kernel.org \
    --cc=p.paillet@st.com \
    --cc=robh+dt@kernel.org \
    --cc=robh@kernel.org \
    --cc=xiaotong.lu@spreadtrum.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git
	git clone --mirror https://lore.kernel.org/lkml/8 lkml/git/8.git
	git clone --mirror https://lore.kernel.org/lkml/9 lkml/git/9.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org
	public-inbox-index lkml

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git