Re: [PATCHv3 1/2] Input: pwm-vibra: new driver

[Sebastian Reichel]

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