Hi Dmitry,

On Sun, May 07, 2017 at 02:38:00PM -0700, Dmitry Torokhov wrote:

quoted

+ *  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.

quoted

+
+#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.

quoted

+ * 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.

quoted

+
+	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?

quoted

+	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 [off-list ref]");
+MODULE_DESCRIPTION("PWM vibrator driver");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:pwm-vibrator");
-- 
2.11.0

Thanks.

Thanks for the review.

-- Sebastian