Hi Dmitry,
On Sun, May 07, 2017 at 02:38:00PM -0700, Dmitry Torokhov wrote:
> > + * This program is free software; you can redistribute it and/or modify it
> > + * under the terms of the GNU General Public License as published by the
> > + * Free Software Foundation; either version 2 of the License, or (at your
> > + * option) any later version.
> > + */
> > +
> > +#define DEBUG
>
> I do not think this is needed.
Leftover from writing the driver :) Will remove in v4.
> > +
> > +#include <linux/input.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/of_device.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pwm.h>
> > +#include <linux/regulator/consumer.h>
> > +#include <linux/slab.h>
> > +
> > +/**
>
> This is not kernel doc, so no "/**".
Ack.
> > + * Motorola Droid 4 (also known as mapphone), has a vibrator, which pulses
> > + * 1x on rising edge. Increasing the pwm period results in more pulses per
> > + * second, but reduces intensity. There is also a second channel to control
> > + * the vibrator's rotation direction to increase effect. The following
> > + * numbers were determined manually. Going below 12.5 Hz means, clearly
> > + * noticeable pauses and at 30 Hz the vibration is just barely noticable
> > + * anymore.
> > + */
> > +#define MAPPHONE_MIN_FREQ 125 /* 12.5 Hz */
> > +#define MAPPHONE_MAX_FREQ 300 /* 30.0 Hz */
> > +
> > [...]
> > +
> > + vibrator->pwm_dir = devm_pwm_get(&pdev->dev, "direction");
> > + err = PTR_ERR_OR_ZERO(vibrator->pwm_dir);
> > + if (err == -ENODATA) {
> > + vibrator->pwm_dir = NULL;
> > + } else if (err == -EPROBE_DEFER) {
> > + return err;
> > + } else if (err) {
> > + dev_err(&pdev->dev, "Failed to request direction pwm: %d", err);
> > + return err;
> > + } else {
> > + /* Sync up PWM state and ensure it is off. */
> > + pwm_init_state(vibrator->pwm_dir, &state);
> > + state.enabled = false;
> > + err = pwm_apply_state(vibrator->pwm_dir, &state);
> > + if (err) {
> > + dev_err(&pdev->dev, "failed to apply initial PWM state: %d",
> > + err);
> > + return err;
> > + }
> > + }
>
> I wonder if the above is not better with "switch":
>
> switch (err) {
> case 0:
> /* Sync up PWM state and ensure it is off. */
> pwm_init_state(vibrator->pwm_dir, &state);
> state.enabled = false;
> err = pwm_apply_state(vibrator->pwm_dir, &state);
> if (err) {
> dev_err(&pdev->dev,
> "failed to apply initial PWM state: %d", err);
> return err;
> }
> break;
>
> case -ENODATA:
> /* Direction PWM is optional */
> vibrator->pwm_dir = NULL;
> break;
>
> default:
> dev_err(&pdev->dev, "Failed to request direction pwm: %d", err);
> /* Fall through */
>
> case -EPROBE_DEFER:
> return err;
> }
Ack.
> > +
> > + vibrator->hw = of_device_get_match_data(&pdev->dev);
> > + if (!vibrator->hw)
> > + vibrator->hw = &pwm_vib_hw_generic;
> > +
> > + input->name = "pwm-vibrator";
> > + input->id.bustype = BUS_HOST;
> > + input->dev.parent = &pdev->dev;
> > + input->close = pwm_vibrator_close;
> > +
> > + input_set_drvdata(input, vibrator);
> > + input_set_capability(input, EV_FF, FF_RUMBLE);
> > +
> > + err = input_ff_create_memless(input, NULL, pwm_vibrator_play_effect);
> > + if (err) {
> > + dev_err(&pdev->dev, "Couldn't create FF dev: %d", err);
> > + return err;
> > + }
> > +
> > + err = input_register_device(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 pwm_vibrator_suspend(struct device *dev)
> > +{
> > + struct platform_device *pdev = to_platform_device(dev);
> > + struct pwm_vibrator *vibrator = platform_get_drvdata(pdev);
> > + struct input_dev *input = vibrator->input;
> > + unsigned long flags;
> > +
> > + spin_lock_irqsave(&input->event_lock, flags);
>
> Hmm, no, this is not goting to work. The original patch had a chance if
> PWM was not sleeping, but with introduction of regulator and work this
> definitely sleeps.
Actually PWM is sleeping, that's why I added work (regulator was
added later) :)
> I think we should solve issue of events [not] being delivered during
> suspend transition in input core, and simply drop spin_lock_irqsave()
> here and in resume().
Sounds good. will you take care of the input-core change?
> > + cancel_work_sync(&vibrator->play_work);
> > + if (vibrator->level)
> > + pwm_vibrator_stop(vibrator);
> > + spin_unlock_irqrestore(&input->event_lock, flags);
> > +
> > + return 0;
> > +}
> > +
> > +static int __maybe_unused pwm_vibrator_resume(struct device *dev)
> > +{
> > + struct platform_device *pdev = to_platform_device(dev);
> > + struct pwm_vibrator *vibrator = platform_get_drvdata(pdev);
> > + struct input_dev *input = vibrator->input;
> > + unsigned long flags;
> > +
> > + spin_lock_irqsave(&input->event_lock, flags);
> > + if (vibrator->level)
> > + pwm_vibrator_start(vibrator);
> > + spin_unlock_irqrestore(&input->event_lock, flags);
> > +
> > + return 0;
> > +}
> > +
> > +static SIMPLE_DEV_PM_OPS(pwm_vibrator_pm_ops,
> > + pwm_vibrator_suspend, pwm_vibrator_resume);
> > +
> > +#ifdef CONFIG_OF
> > +
> > +#define PWM_VIB_COMPAT(of_compatible, cfg) { \
> > + .compatible = of_compatible, \
> > + .data = &cfg, \
> > +}
> > +
> > +static const struct of_device_id pwm_vibra_dt_match_table[] = {
> > + PWM_VIB_COMPAT("pwm-vibrator", pwm_vib_hw_generic),
> > + PWM_VIB_COMPAT("motorola,mapphone-pwm-vibrator", pwm_vib_hw_mapphone),
> > + {},
> > +};
> > +MODULE_DEVICE_TABLE(of, pwm_vibra_dt_match_table);
> > +#endif
> > +
> > +static struct platform_driver pwm_vibrator_driver = {
> > + .probe = pwm_vibrator_probe,
> > + .driver = {
> > + .name = "pwm-vibrator",
> > + .pm = &pwm_vibrator_pm_ops,
> > + .of_match_table = of_match_ptr(pwm_vibra_dt_match_table),
> > + },
> > +};
> > +module_platform_driver(pwm_vibrator_driver);
> > +
> > +MODULE_AUTHOR("Sebastian Reichel <sre@xxxxxxxxxx>");
> > +MODULE_DESCRIPTION("PWM vibrator driver");
> > +MODULE_LICENSE("GPL");
> > +MODULE_ALIAS("platform:pwm-vibrator");
> > --
> > 2.11.0
> >
>
> Thanks.
Thanks for the review.
-- Sebastian
Attachment:
signature.asc
Description: PGP signature
