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@xxxxxxxxx>
> > ---
> >
> > 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@xxxxxxxxx>
> > + *
> > + * Based on PWM vibrator driver:
> > + * Copyright (C) 2017 Collabora Ltd.
> > + *
> > + * Based on previous work from:
> > + * Copyright (C) 2012 Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx>
> > + *
> > + * Based on PWM beeper driver:
> > + * Copyright (C) 2010, Lars-Peter Clausen <lars@xxxxxxxxxx>
> > + */
> > +
> > +#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@xxxxxxxx>");
> > +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
