Re: [PATCH 2/2] Input: misc: introduce palmas-pwrbutton

[Nishanth Menon]

Hi Dmitry
On 08/19/2014 12:23 AM, Dmitry Torokhov wrote:
Thanks for the review.

[...]
> > +
> > +/**
> > + * pwron_irq() - button press isr
> > + * @irq:		irq
> > + * @palmas_pwron:	pwron struct
> > + */
> > +static irqreturn_t pwron_irq(int irq, void *palmas_pwron)
> > +{
> > +	struct palmas_pwron *pwron = palmas_pwron;
> > +	struct input_dev *input_dev = pwron->input_dev;
> > +
> > +	cancel_delayed_work_sync(&pwron->input_work);
> > +
> > +	pwron->current_state = PALMAS_PWR_KEY_PRESS;
> > +
> > +	input_report_key(input_dev, KEY_POWER, pwron->current_state);
> > +	pm_wakeup_event(input_dev->dev.parent, 0);
> > +	input_sync(input_dev);
> > +
> > +	schedule_delayed_work(&pwron->input_work, 0);
>
> Instead of cancel/schedule use mod_delayed_work. BTW, why do you need to
> schedule immediately instead of waiting key_recheck_ms? Also, are there any

Good point, I had missed these. Will fix.

> concerns about need to debounce?

I believe PMIC already takes care of debounce, let me see if there are 
configuration registers possible. if yes, I think it might be nice to 
add in.

[...]

> > +
> > +	irq = platform_get_irq(pdev, 0);
> > +
> > +	device_init_wakeup(dev, 1);
> > +
> > +	ret = devm_request_threaded_irq(dev, irq, NULL, pwron_irq,
> > +					IRQF_TRIGGER_HIGH |
> > +					IRQF_TRIGGER_LOW,
> > +					dev_name(dev),
> > +					pwron);
>
> I am confused about this code sequence. Why do we get IRQ, then set up wakeup,
> and then request irq? Normally you get irq number, and then you request it, and
> then do other stuff.

Uggh.. right.. will fix.

> > +	if (ret < 0) {
> > +		dev_err(dev, "Can't get IRQ for pwron: %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	enable_irq_wake(irq);
>
> Shouldn't this be in suspend callback?

yes, it should have been.. my bad.. :( thanks for catching it.

> > +
> > +	ret = input_register_device(input_dev);
> > +	if (ret) {
> > +		dev_dbg(dev, "Can't register power button: %d\n", ret);
> > +		goto out_irq_wake;
> > +	}
> > +	pwron->irq = irq;
> > +
> > +	pwron->key_recheck_ms = PALMAS_PWR_KEY_Q_TIME_MS;
> > +
> > +	platform_set_drvdata(pdev, pwron);
> > +
> > +	return 0;
> > +
> > +out_irq_wake:
> > +	disable_irq_wake(irq);
> > +
> > +	return ret;
> > +}
> > +
> > +static int palmas_pwron_remove(struct platform_device *pdev)
> > +{
> > +	struct palmas_pwron *pwron = platform_get_drvdata(pdev);
> > +
> > +	disable_irq_wake(pwron->irq);
>
> Should be in resume callback().

yep.

> > +	input_unregister_device(pwron->input_dev);
>
> With devm you do not need to unregister input device. However this has problem:
> what will happen if interrupt arrives here and we schedule workqueue? You need
> free interrupt then cancel work and then free input device. Similar needs to be
> done in probe(). I'd recommend not use devm_* here as you need to manually
> unwind anyway.

True. I will fix these as well.

> > +
> > +	return 0;
> > +}
> > +
> > +#ifdef CONFIG_PM
> > +/**
> > + * palmas_pwron_suspend() - suspend handler
> > + * @dev:	power button device
> > + *
> > + * Cancel all pending work items for the power button
> > + */
> > +static int palmas_pwron_suspend(struct device *dev)
> > +{
> > +	struct platform_device *pdev = to_platform_device(dev);
> > +	struct palmas_pwron *pwron = platform_get_drvdata(pdev);
> > +
> > +	cancel_delayed_work_sync(&pwron->input_work);
> > +
> > +	return 0;
> > +}
> > +
> > +static UNIVERSAL_DEV_PM_OPS(palmas_pwron_pm, palmas_pwron_suspend, NULL, NULL);
>
> Why universal? Do they make sense for runtime pm?
>
> > +
> > +#else
> > +static UNIVERSAL_DEV_PM_OPS(palmas_pwron_pm, NULL, NULL, NULL);
> > +#endif
>
> You do not need to protect these with #ifdef and have 2 versions, just pull
> UNIVERSAL_DEV_PM_OPS (and change to SIMPLE_DEV_PM_OPS) out of #idef code.

I will just switch over to SIMPLE_DEV_PM_OPS here.. it is better here. 
Thanks for the suggestion.

> > +
> > +#ifdef CONFIG_OF
> > +static struct of_device_id of_palmas_pwr_match[] = {
> > +	{.compatible = "ti,palmas-pwrbutton"},
> > +	{},
> > +};
> > +
> > +MODULE_DEVICE_TABLE(of, of_palmas_pwr_match);
> > +#endif
> > +
> > +static struct platform_driver palmas_pwron_driver = {
> > +	.probe = palmas_pwron_probe,
> > +	.remove = palmas_pwron_remove,
> > +	.driver = {
> > +		   .name = "palmas_pwrbutton",
> > +		   .owner = THIS_MODULE,
> > +		   .of_match_table = of_match_ptr(of_palmas_pwr_match),
> > +		   .pm = &palmas_pwron_pm,
> > +		   },
>
> Weird indentation here.

Ugggh.. Lindent.. :(


---
Regards,
Nishanth Menon
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/