[PATCH] HID: TWL4030: add HID Force Feedback vibra

[PATCH] HID: TWL4030: add HID Force Feedback vibra

[Jari Vanhala]

TWL4030 Vibrator implemented via HID Force Feedback
interface. This uses MFD TWL4030 codec and own workqueue.

Signed-off-by: Jari Vanhala <ext-jari.vanhala@nokia.com>
---
 drivers/hid/Kconfig             |    8 +
 drivers/hid/Makefile            |    1 +
 drivers/hid/hid-twl4030-vibra.c |  292 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 301 insertions(+), 0 deletions(-)
 create mode 100644 drivers/hid/hid-twl4030-vibra.c

diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index 71d4c07..9bb4e0a 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -372,6 +372,14 @@ config ZEROPLUS_FF
 	  Say Y here if you have a Zeroplus based game controller and want
 	  to have force feedback support for it.
 
+config HID_TWL4030_VIBRA
+	tristate "HID Support for TWL4030 Vibrator"
+	depends on TWL4030_CORE
+	select TWL4030_CODEC
+	select INPUT_FF_MEMLESS
+	---help---
+	  This option enables support for TWL4030 Vibrator Driver.
+
 endmenu
 
 endif # HID_SUPPORT
diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
index 0b2618f..192cde6 100644
--- a/drivers/hid/Makefile
+++ b/drivers/hid/Makefile
@@ -55,6 +55,7 @@ obj-$(CONFIG_HID_TOPSEED)	+= hid-topseed.o
 obj-$(CONFIG_HID_TWINHAN)	+= hid-twinhan.o
 obj-$(CONFIG_HID_ZEROPLUS)	+= hid-zpff.o
 obj-$(CONFIG_HID_WACOM)		+= hid-wacom.o
+obj-$(CONFIG_HID_TWL4030_VIBRA)	+= hid-twl4030-vibra.o
 
 obj-$(CONFIG_USB_HID)		+= usbhid/
 obj-$(CONFIG_USB_MOUSE)		+= usbhid/
diff --git a/drivers/hid/hid-twl4030-vibra.c b/drivers/hid/hid-twl4030-vibra.c
new file mode 100644
index 0000000..1009dfb
--- /dev/null
+++ b/drivers/hid/hid-twl4030-vibra.c
@@ -0,0 +1,292 @@
+/*
+ * hid-twl4030-vibra.c - TWL4030 Vibrator driver
+ *
+ * Copyright (C) 2008-2010 Nokia Corporation
+ *
+ * Written by Henrik Saari <henrik.saari@nokia.com>
+ * Updates by Felipe Balbi <felipe.balbi@nokia.com>
+ * HID by Jari Vanhala <ext-jari.vanhala@nokia.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
+ * 02110-1301 USA
+ *
+ */
+
+#include <linux/module.h>
+#include <linux/jiffies.h>
+#include <linux/platform_device.h>
+#include <linux/workqueue.h>
+#include <linux/i2c/twl.h>
+#include <linux/mfd/twl4030-codec.h>
+
+#include <linux/hid.h>
+#include <linux/input.h>
+
+/* MODULE ID2 */
+#define LEDEN		0x00
+
+/* ForceFeedback */
+#define EFFECT_DIR_180_DEG	0x8000 /* range is 0 - 0xFFFF */
+
+struct vibra_info {
+	struct device		*dev;
+	struct input_dev	*input_dev;
+
+	struct workqueue_struct *workqueue;
+	struct work_struct	play_work;
+
+	int			enabled;
+	int			speed;
+	int			direction;
+
+	int			coexist;
+};
+
+static void vibra_disable_leds(void)
+{
+	u8 reg;
+
+	/* Disable LEDA & LEDB, cannot be used with vibra (PWM) */
+	twl_i2c_read_u8(TWL4030_MODULE_LED, &reg, LEDEN);
+	reg &= ~0x03;
+	twl_i2c_write_u8(TWL4030_MODULE_LED, LEDEN, reg);
+}
+
+/* Powers H-Bridge and enables audio clk */
+static void vibra_enable(struct vibra_info *info)
+{
+	u8 reg;
+
+	twl4030_codec_enable_resource(TWL4030_CODEC_RES_POWER);
+
+	/* turn H-Bridge on */
+	twl_i2c_read_u8(TWL4030_MODULE_AUDIO_VOICE,
+			&reg, TWL4030_REG_VIBRA_CTL);
+	twl_i2c_write_u8(TWL4030_MODULE_AUDIO_VOICE,
+			 (reg | TWL4030_VIBRA_EN), TWL4030_REG_VIBRA_CTL);
+
+	twl4030_codec_enable_resource(TWL4030_CODEC_RES_APLL);
+
+	info->enabled = 1;
+}
+
+static void vibra_disable(struct vibra_info *info)
+{
+	u8 reg;
+
+	/* Power down H-Bridge */
+	twl_i2c_read_u8(TWL4030_MODULE_AUDIO_VOICE,
+			&reg, TWL4030_REG_VIBRA_CTL);
+	twl_i2c_write_u8(TWL4030_MODULE_AUDIO_VOICE,
+			 (reg & ~TWL4030_VIBRA_EN), TWL4030_REG_VIBRA_CTL);
+
+	twl4030_codec_disable_resource(TWL4030_CODEC_RES_POWER);
+	twl4030_codec_disable_resource(TWL4030_CODEC_RES_APLL);
+
+	info->enabled = 0;
+}
+
+static void vibra_play_work(struct work_struct *work)
+{
+	struct vibra_info *info = container_of(work,
+			struct vibra_info, play_work);
+	int dir;
+	int pwm;
+	u8 reg;
+
+	dir = info->direction;
+	pwm = info->speed;
+
+	twl_i2c_read_u8(TWL4030_MODULE_AUDIO_VOICE,
+			&reg, TWL4030_REG_VIBRA_CTL);
+	if (pwm && (!info->coexist || !(reg & TWL4030_VIBRA_SEL))) {
+
+		if (!info->enabled)
+			vibra_enable(info);
+
+		/* set vibra rotation direction */
+		twl_i2c_read_u8(TWL4030_MODULE_AUDIO_VOICE,
+				&reg, TWL4030_REG_VIBRA_CTL);
+		reg = (dir) ? (reg | TWL4030_VIBRA_DIR) :
+			(reg & ~TWL4030_VIBRA_DIR);
+		twl_i2c_write_u8(TWL4030_MODULE_AUDIO_VOICE,
+				 reg, TWL4030_REG_VIBRA_CTL);
+
+		/* set PWM, 1 = max, 255 = min */
+		twl_i2c_write_u8(TWL4030_MODULE_AUDIO_VOICE,
+				 256 - pwm, TWL4030_REG_VIBRA_SET);
+	} else {
+		if (info->enabled)
+			vibra_disable(info);
+	}
+}
+
+/*** ForceFeedback ***/
+
+static int vibra_play(struct input_dev *dev, void *data,
+		      struct ff_effect *effect)
+{
+	struct vibra_info *info = data;
+
+	if (!info->workqueue)
+		return 0;
+
+	info->speed = effect->u.rumble.strong_magnitude >> 8;
+	if (!info->speed)
+		info->speed = effect->u.rumble.weak_magnitude >> 9;
+	info->direction = effect->direction < EFFECT_DIR_180_DEG ? 0 : 1;
+	queue_work(info->workqueue, &info->play_work);
+
+	return 0;
+}
+
+/*** Module ***/
+#if CONFIG_PM
+static int twl4030_vibra_suspend(struct platform_device *pdev,
+				 pm_message_t state)
+{
+	struct vibra_info *info = platform_get_drvdata(pdev);
+
+	if (info->enabled)
+		vibra_disable(info);
+
+	return 0;
+}
+
+static void twl4030_vibra_shutdown(struct platform_device *pdev)
+{
+	struct vibra_info *info = platform_get_drvdata(pdev);
+
+	if (info->enabled)
+		vibra_disable(info);
+}
+
+static int twl4030_vibra_resume(struct platform_device *pdev)
+{
+	vibra_disable_leds();
+	return 0;
+}
+#else
+#define twl4030_vibra_suspend NULL
+#define twl4030_vibra_shutdown NULL
+#define twl4030_vibra_resume NULL
+#endif
+
+static int __devinit twl4030_vibra_probe(struct platform_device *pdev)
+{
+	struct twl4030_codec_vibra_data *pdata = pdev->dev.platform_data;
+	struct vibra_info *info;
+	int ret;
+
+	if (!pdata) {
+		dev_dbg(&pdev->dev, "platform_data not available\n");
+		return -EINVAL;
+	}
+
+	info = kzalloc(sizeof(*info), GFP_KERNEL);
+	if (!info)
+		return -ENOMEM;
+
+	info->dev = &pdev->dev;
+	info->enabled = 0;
+	info->coexist = pdata->coexist;
+
+	platform_set_drvdata(pdev, info);
+
+	info->workqueue = create_singlethread_workqueue("vibra");
+	if (info->workqueue == NULL) {
+		dev_err(&pdev->dev, "couldn't create workqueue\n");
+		kfree(info);
+		return -ENOMEM;
+	}
+	INIT_WORK(&info->play_work, vibra_play_work);
+
+	info->input_dev = input_allocate_device();
+	if (info->input_dev == NULL) {
+		dev_err(&pdev->dev, "couldn't allocate input device\n");
+		kfree(info);
+		return -ENOMEM;
+	}
+	input_set_drvdata(info->input_dev, info);
+
+	info->input_dev->name = "twl4030:vibrator";
+	info->input_dev->id.version = 1;
+	info->input_dev->dev.parent = pdev->dev.parent;
+	set_bit(FF_RUMBLE, info->input_dev->ffbit);
+
+	ret = input_register_device(info->input_dev);
+	if (ret < 0) {
+		dev_dbg(&pdev->dev, "couldn't register input device\n");
+		input_free_device(info->input_dev);
+		kfree(info);
+		return ret;
+	}
+
+	ret = input_ff_create_memless(info->input_dev, info, vibra_play);
+	if (ret < 0) {
+		dev_dbg(&pdev->dev, "couldn't register vibrator to FF\n");
+		input_unregister_device(info->input_dev);
+		kfree(info);
+		return ret;
+	}
+
+	vibra_disable_leds();
+
+	return 0;
+}
+
+static int __devexit twl4030_vibra_remove(struct platform_device *pdev)
+{
+	struct vibra_info *info = platform_get_drvdata(pdev);
+
+	cancel_work_sync(&info->play_work);
+	destroy_workqueue(info->workqueue);
+	info->workqueue = NULL;
+	if (info->enabled)
+		vibra_disable(info);
+	/* this also free ff-memless which (in turn) kfree info */
+	input_unregister_device(info->input_dev);
+
+	return 0;
+}
+
+static struct platform_driver twl4030_vibra_driver = {
+	.probe		= twl4030_vibra_probe,
+	.remove		= __devexit_p(twl4030_vibra_remove),
+	.suspend	= twl4030_vibra_suspend,
+	.resume		= twl4030_vibra_resume,
+	.shutdown	= twl4030_vibra_shutdown,
+	.driver		= {
+		.name	= "twl4030_codec_vibra",
+		.owner	= THIS_MODULE,
+	},
+};
+
+static int __init twl4030_vibra_init(void)
+{
+	return platform_driver_register(&twl4030_vibra_driver);
+}
+module_init(twl4030_vibra_init);
+
+static void __exit twl4030_vibra_exit(void)
+{
+	platform_driver_unregister(&twl4030_vibra_driver);
+}
+module_exit(twl4030_vibra_exit);
+
+MODULE_ALIAS("platform:twl4030_codec_vibra");
+
+MODULE_DESCRIPTION("HID TWL4030 Vibra driver");
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Nokia Corporation");
-- 
1.6.3.3

Re: [PATCH] HID: TWL4030: add HID Force Feedback vibra

[Jiri Kosina]

On Wed, 17 Feb 2010, Jari Vanhala wrote:

> TWL4030 Vibrator implemented via HID Force Feedback interface. This uses 
> MFD TWL4030 codec and own workqueue.

Hi Jari,

thanks for the driver.

> Signed-off-by: Jari Vanhala <ext-jari.vanhala@nokia.com> ---
>  drivers/hid/Kconfig             |    8 +
>  drivers/hid/Makefile            |    1 +
>  drivers/hid/hid-twl4030-vibra.c |  292 +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 301 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/hid/hid-twl4030-vibra.c

As the device doesn't seem to be registering itself on the HID bus at all, 
I am not sure we want to put it together with all the HID drivers. Maybe 
drivers/input/misc would be more appropriate?

-- 
Jiri Kosina
SUSE Labs, Novell Inc.

Re: [PATCH] HID: TWL4030: add HID Force Feedback vibra

[Jari Vanhala]

[-- Attachment #1: Type: text/plain, Size: 824 bytes --]

On Wed, 2010-02-17 at 13:41 +0100, ext Jiri Kosina wrote:
> On Wed, 17 Feb 2010, Jari Vanhala wrote:
> As the device doesn't seem to be registering itself on the HID bus at all, 
> I am not sure we want to put it together with all the HID drivers. Maybe 
> drivers/input/misc would be more appropriate?

I felt like I forgot something.. Well, moving it to input/misc is easy
(commit attached), but I (still) wanted to try getting it to use
hid-functions. Results weren't that nice. Looking hid-core just shows,
it's thought to connect (hotplug) usb or bt devices and i2c doesn't fit
nicely to that. Making it to use hid_allocate_device(), hid_connect()
etc takes actually more code..

Leaving it as-is in hid would look little odd, so probably moving it to
drivers/input/misc is best thing. What you think? Dmitry?

	++Jam


[-- Attachment #2: hidmove --]
[-- Type: text/plain, Size: 3339 bytes --]

    Input: Change hid to misc
    
    Signed-off-by: Jari Vanhala <ext-jari.vanhala@nokia.com>

----------------------------- drivers/hid/Kconfig -----------------------------
index 9bb4e0a..71d4c07 100644
@@ -372,14 +372,6 @@ config ZEROPLUS_FF
 	  Say Y here if you have a Zeroplus based game controller and want
 	  to have force feedback support for it.
 
-config HID_TWL4030_VIBRA
-	tristate "HID Support for TWL4030 Vibrator"
-	depends on TWL4030_CORE
-	select TWL4030_CODEC
-	select INPUT_FF_MEMLESS
-	---help---
-	  This option enables support for TWL4030 Vibrator Driver.
-
 endmenu
 
 endif # HID_SUPPORT

----------------------------- drivers/hid/Makefile -----------------------------
index 192cde6..0b2618f 100644
@@ -55,7 +55,6 @@ obj-$(CONFIG_HID_TOPSEED)	+= hid-topseed.o
 obj-$(CONFIG_HID_TWINHAN)	+= hid-twinhan.o
 obj-$(CONFIG_HID_ZEROPLUS)	+= hid-zpff.o
 obj-$(CONFIG_HID_WACOM)		+= hid-wacom.o
-obj-$(CONFIG_HID_TWL4030_VIBRA)	+= hid-twl4030-vibra.o
 
 obj-$(CONFIG_USB_HID)		+= usbhid/
 obj-$(CONFIG_USB_MOUSE)		+= usbhid/

-------------------------- drivers/input/misc/Kconfig --------------------------
index 16ec523..2646ce8 100644
@@ -204,6 +204,14 @@ config INPUT_TWL4030_PWRBUTTON
 	  To compile this driver as a module, choose M here. The module will
 	  be called twl4030_pwrbutton.
 
+config INPUT_TWL4030_VIBRA
+	tristate "Support for TWL4030 Vibrator"
+	depends on TWL4030_CORE
+	select TWL4030_CODEC
+	select INPUT_FF_MEMLESS
+	help
+	  This option enables support for TWL4030 Vibrator Driver.
+
 config INPUT_UINPUT
 	tristate "User level driver support"
 	help

------------------------- drivers/input/misc/Makefile -------------------------
index a8b8485..1ab002e 100644
@@ -25,6 +25,7 @@ obj-$(CONFIG_INPUT_GPIO_ROTARY_ENCODER)	+= rotary_encoder.o
 obj-$(CONFIG_INPUT_SGI_BTNS)		+= sgi_btns.o
 obj-$(CONFIG_INPUT_SPARCSPKR)		+= sparcspkr.o
 obj-$(CONFIG_INPUT_TWL4030_PWRBUTTON)	+= twl4030-pwrbutton.o
+obj-$(CONFIG_INPUT_TWL4030_VIBRA)	+= twl4030-vibra.o
 obj-$(CONFIG_INPUT_UINPUT)		+= uinput.o
 obj-$(CONFIG_INPUT_WINBOND_CIR)		+= winbond-cir.o
 obj-$(CONFIG_INPUT_WISTRON_BTNS)	+= wistron_btns.o

---------------------- drivers/input/misc/twl4030-vibra.c ----------------------
similarity index 97%
rename from drivers/hid/hid-twl4030-vibra.c
rename to drivers/input/misc/twl4030-vibra.c
index 1009dfb..4fa4c33 100644
@@ -1,11 +1,11 @@
 /*
- * hid-twl4030-vibra.c - TWL4030 Vibrator driver
+ * twl4030-vibra.c - TWL4030 Vibrator driver
  *
  * Copyright (C) 2008-2010 Nokia Corporation
  *
  * Written by Henrik Saari <henrik.saari@nokia.com>
  * Updates by Felipe Balbi <felipe.balbi@nokia.com>
- * HID by Jari Vanhala <ext-jari.vanhala@nokia.com>
+ * Input by Jari Vanhala <ext-jari.vanhala@nokia.com>
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 2 as
@@ -29,8 +29,6 @@
 #include <linux/workqueue.h>
 #include <linux/i2c/twl.h>
 #include <linux/mfd/twl4030-codec.h>
-
-#include <linux/hid.h>
 #include <linux/input.h>
 
 /* MODULE ID2 */
@@ -287,6 +285,6 @@ module_exit(twl4030_vibra_exit);
 
 MODULE_ALIAS("platform:twl4030_codec_vibra");
 
-MODULE_DESCRIPTION("HID TWL4030 Vibra driver");
+MODULE_DESCRIPTION("TWL4030 Vibra driver");
 MODULE_LICENSE("GPL");
 MODULE_AUTHOR("Nokia Corporation");

Re: [PATCH] HID: TWL4030: add HID Force Feedback vibra

[Jiri Kosina]

On Thu, 18 Feb 2010, Jari Vanhala wrote:

> On Wed, 2010-02-17 at 13:41 +0100, ext Jiri Kosina wrote:
> > On Wed, 17 Feb 2010, Jari Vanhala wrote:
> > As the device doesn't seem to be registering itself on the HID bus at all, 
> > I am not sure we want to put it together with all the HID drivers. Maybe 
> > drivers/input/misc would be more appropriate?
> 
> I felt like I forgot something.. Well, moving it to input/misc is easy
> (commit attached), but I (still) wanted to try getting it to use
> hid-functions. Results weren't that nice. Looking hid-core just shows,
> it's thought to connect (hotplug) usb or bt devices and i2c doesn't fit
> nicely to that. Making it to use hid_allocate_device(), hid_connect()
> etc takes actually more code..

Well, the major differentiator should be whether the "on-the-wire" the 
protocol is using is (or at least tries to be in some aspects) 
HID standard compliant.

Which, as far as I understand, is not the case here.

> Leaving it as-is in hid would look little odd, so probably moving it to
> drivers/input/misc is best thing. What you think? Dmitry?

Thanks,

-- 
Jiri Kosina
SUSE Labs, Novell Inc.

Re: [PATCH] HID: TWL4030: add HID Force Feedback vibra

[Dmitry Torokhov]

On Thu, Feb 18, 2010 at 02:50:16PM +0200, Jari Vanhala wrote:
> On Wed, 2010-02-17 at 13:41 +0100, ext Jiri Kosina wrote:
> > On Wed, 17 Feb 2010, Jari Vanhala wrote:
> > As the device doesn't seem to be registering itself on the HID bus at all, 
> > I am not sure we want to put it together with all the HID drivers. Maybe 
> > drivers/input/misc would be more appropriate?
> 
> I felt like I forgot something.. Well, moving it to input/misc is easy
> (commit attached), but I (still) wanted to try getting it to use
> hid-functions. Results weren't that nice. Looking hid-core just shows,
> it's thought to connect (hotplug) usb or bt devices and i2c doesn't fit
> nicely to that. Making it to use hid_allocate_device(), hid_connect()
> etc takes actually more code..
> 
> Leaving it as-is in hid would look little odd, so probably moving it to
> drivers/input/misc is best thing. What you think? Dmitry?
> 

I can take it into misc. Still need to review it though.

-- 
Dmitry

Re: [PATCH] HID: TWL4030: add HID Force Feedback vibra

[Dmitry Torokhov]

Hi Jari,

On Wed, Feb 17, 2010 at 02:22:16PM +0200, Jari Vanhala wrote:
> TWL4030 Vibrator implemented via HID Force Feedback
> interface. This uses MFD TWL4030 codec and own workqueue.
> 
> Signed-off-by: Jari Vanhala <ext-jari.vanhala@nokia.com>
> ---
>  drivers/hid/Kconfig             |    8 +
>  drivers/hid/Makefile            |    1 +
>  drivers/hid/hid-twl4030-vibra.c |  292 +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 301 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/hid/hid-twl4030-vibra.c
> 
> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> index 71d4c07..9bb4e0a 100644
> --- a/drivers/hid/Kconfig
> +++ b/drivers/hid/Kconfig
> @@ -372,6 +372,14 @@ config ZEROPLUS_FF
>  	  Say Y here if you have a Zeroplus based game controller and want
>  	  to have force feedback support for it.
>  
> +config HID_TWL4030_VIBRA
> +	tristate "HID Support for TWL4030 Vibrator"
> +	depends on TWL4030_CORE
> +	select TWL4030_CODEC
> +	select INPUT_FF_MEMLESS
> +	---help---
> +	  This option enables support for TWL4030 Vibrator Driver.
> +

To compile this driver as a module...

>  endmenu
>  
>  endif # HID_SUPPORT
> diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
> index 0b2618f..192cde6 100644
> --- a/drivers/hid/Makefile
> +++ b/drivers/hid/Makefile
> @@ -55,6 +55,7 @@ obj-$(CONFIG_HID_TOPSEED)	+= hid-topseed.o
>  obj-$(CONFIG_HID_TWINHAN)	+= hid-twinhan.o
>  obj-$(CONFIG_HID_ZEROPLUS)	+= hid-zpff.o
>  obj-$(CONFIG_HID_WACOM)		+= hid-wacom.o
> +obj-$(CONFIG_HID_TWL4030_VIBRA)	+= hid-twl4030-vibra.o
>  
>  obj-$(CONFIG_USB_HID)		+= usbhid/
>  obj-$(CONFIG_USB_MOUSE)		+= usbhid/
> diff --git a/drivers/hid/hid-twl4030-vibra.c b/drivers/hid/hid-twl4030-vibra.c
> new file mode 100644
> index 0000000..1009dfb
> --- /dev/null
> +++ b/drivers/hid/hid-twl4030-vibra.c
> @@ -0,0 +1,292 @@
> +/*
> + * hid-twl4030-vibra.c - TWL4030 Vibrator driver
> + *
> + * Copyright (C) 2008-2010 Nokia Corporation
> + *
> + * Written by Henrik Saari <henrik.saari@nokia.com>
> + * Updates by Felipe Balbi <felipe.balbi@nokia.com>
> + * HID by Jari Vanhala <ext-jari.vanhala@nokia.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
> + * 02110-1301 USA
> + *
> + */
> +
> +#include <linux/module.h>
> +#include <linux/jiffies.h>
> +#include <linux/platform_device.h>
> +#include <linux/workqueue.h>
> +#include <linux/i2c/twl.h>
> +#include <linux/mfd/twl4030-codec.h>
> +
> +#include <linux/hid.h>
> +#include <linux/input.h>
> +
> +/* MODULE ID2 */
> +#define LEDEN		0x00
> +
> +/* ForceFeedback */
> +#define EFFECT_DIR_180_DEG	0x8000 /* range is 0 - 0xFFFF */
> +
> +struct vibra_info {
> +	struct device		*dev;
> +	struct input_dev	*input_dev;
> +
> +	struct workqueue_struct *workqueue;

Why do we need a separate workqueue? Can't keventd serve us?

> +	struct work_struct	play_work;
> +
> +	int			enabled;

Use bool?

> +	int			speed;
> +	int			direction;
> +
> +	int			coexist;

Here as well?

> +};
> +
> +static void vibra_disable_leds(void)
> +{
> +	u8 reg;
> +
> +	/* Disable LEDA & LEDB, cannot be used with vibra (PWM) */
> +	twl_i2c_read_u8(TWL4030_MODULE_LED, &reg, LEDEN);
> +	reg &= ~0x03;
> +	twl_i2c_write_u8(TWL4030_MODULE_LED, LEDEN, reg);
> +}
> +
> +/* Powers H-Bridge and enables audio clk */
> +static void vibra_enable(struct vibra_info *info)
> +{
> +	u8 reg;
> +
> +	twl4030_codec_enable_resource(TWL4030_CODEC_RES_POWER);
> +
> +	/* turn H-Bridge on */
> +	twl_i2c_read_u8(TWL4030_MODULE_AUDIO_VOICE,
> +			&reg, TWL4030_REG_VIBRA_CTL);
> +	twl_i2c_write_u8(TWL4030_MODULE_AUDIO_VOICE,
> +			 (reg | TWL4030_VIBRA_EN), TWL4030_REG_VIBRA_CTL);
> +
> +	twl4030_codec_enable_resource(TWL4030_CODEC_RES_APLL);
> +
> +	info->enabled = 1;
> +}
> +
> +static void vibra_disable(struct vibra_info *info)
> +{
> +	u8 reg;
> +
> +	/* Power down H-Bridge */
> +	twl_i2c_read_u8(TWL4030_MODULE_AUDIO_VOICE,
> +			&reg, TWL4030_REG_VIBRA_CTL);
> +	twl_i2c_write_u8(TWL4030_MODULE_AUDIO_VOICE,
> +			 (reg & ~TWL4030_VIBRA_EN), TWL4030_REG_VIBRA_CTL);
> +
> +	twl4030_codec_disable_resource(TWL4030_CODEC_RES_POWER);
> +	twl4030_codec_disable_resource(TWL4030_CODEC_RES_APLL);
> +
> +	info->enabled = 0;
> +}
> +
> +static void vibra_play_work(struct work_struct *work)
> +{
> +	struct vibra_info *info = container_of(work,
> +			struct vibra_info, play_work);
> +	int dir;
> +	int pwm;
> +	u8 reg;
> +
> +	dir = info->direction;
> +	pwm = info->speed;
> +
> +	twl_i2c_read_u8(TWL4030_MODULE_AUDIO_VOICE,
> +			&reg, TWL4030_REG_VIBRA_CTL);
> +	if (pwm && (!info->coexist || !(reg & TWL4030_VIBRA_SEL))) {
> +
> +		if (!info->enabled)
> +			vibra_enable(info);
> +
> +		/* set vibra rotation direction */
> +		twl_i2c_read_u8(TWL4030_MODULE_AUDIO_VOICE,
> +				&reg, TWL4030_REG_VIBRA_CTL);
> +		reg = (dir) ? (reg | TWL4030_VIBRA_DIR) :
> +			(reg & ~TWL4030_VIBRA_DIR);
> +		twl_i2c_write_u8(TWL4030_MODULE_AUDIO_VOICE,
> +				 reg, TWL4030_REG_VIBRA_CTL);
> +
> +		/* set PWM, 1 = max, 255 = min */
> +		twl_i2c_write_u8(TWL4030_MODULE_AUDIO_VOICE,
> +				 256 - pwm, TWL4030_REG_VIBRA_SET);
> +	} else {
> +		if (info->enabled)
> +			vibra_disable(info);
> +	}
> +}
> +
> +/*** ForceFeedback ***/
> +
> +static int vibra_play(struct input_dev *dev, void *data,
> +		      struct ff_effect *effect)
> +{
> +	struct vibra_info *info = data;
> +
> +	if (!info->workqueue)
> +		return 0;

How can workqueue be missig here?

> +
> +	info->speed = effect->u.rumble.strong_magnitude >> 8;
> +	if (!info->speed)
> +		info->speed = effect->u.rumble.weak_magnitude >> 9;
> +	info->direction = effect->direction < EFFECT_DIR_180_DEG ? 0 : 1;
> +	queue_work(info->workqueue, &info->play_work);
> +
> +	return 0;
> +}
> +
> +/*** Module ***/
> +#if CONFIG_PM
> +static int twl4030_vibra_suspend(struct platform_device *pdev,
> +				 pm_message_t state)
> +{
> +	struct vibra_info *info = platform_get_drvdata(pdev);
> +
> +	if (info->enabled)
> +		vibra_disable(info);
> +
> +	return 0;
> +}
> +
> +static void twl4030_vibra_shutdown(struct platform_device *pdev)
> +{
> +	struct vibra_info *info = platform_get_drvdata(pdev);
> +
> +	if (info->enabled)
> +		vibra_disable(info);
> +}
> +
> +static int twl4030_vibra_resume(struct platform_device *pdev)
> +{
> +	vibra_disable_leds();
> +	return 0;
> +}


Please convert ot dev_pm_ops.

> +#else
> +#define twl4030_vibra_suspend NULL
> +#define twl4030_vibra_shutdown NULL
> +#define twl4030_vibra_resume NULL
> +#endif
> +
> +static int __devinit twl4030_vibra_probe(struct platform_device *pdev)
> +{
> +	struct twl4030_codec_vibra_data *pdata = pdev->dev.platform_data;
> +	struct vibra_info *info;
> +	int ret;
> +
> +	if (!pdata) {
> +		dev_dbg(&pdev->dev, "platform_data not available\n");
> +		return -EINVAL;
> +	}
> +
> +	info = kzalloc(sizeof(*info), GFP_KERNEL);
> +	if (!info)
> +		return -ENOMEM;
> +
> +	info->dev = &pdev->dev;
> +	info->enabled = 0;
> +	info->coexist = pdata->coexist;
> +
> +	platform_set_drvdata(pdev, info);
> +
> +	info->workqueue = create_singlethread_workqueue("vibra");
> +	if (info->workqueue == NULL) {
> +		dev_err(&pdev->dev, "couldn't create workqueue\n");
> +		kfree(info);
> +		return -ENOMEM;
> +	}
> +	INIT_WORK(&info->play_work, vibra_play_work);
> +
> +	info->input_dev = input_allocate_device();
> +	if (info->input_dev == NULL) {
> +		dev_err(&pdev->dev, "couldn't allocate input device\n");
> +		kfree(info);

Leaking workqueue.

> +		return -ENOMEM;
> +	}
> +	input_set_drvdata(info->input_dev, info);
> +
> +	info->input_dev->name = "twl4030:vibrator";
> +	info->input_dev->id.version = 1;
> +	info->input_dev->dev.parent = pdev->dev.parent;
> +	set_bit(FF_RUMBLE, info->input_dev->ffbit);
> +
> +	ret = input_register_device(info->input_dev);
> +	if (ret < 0) {
> +		dev_dbg(&pdev->dev, "couldn't register input device\n");

Here as well... Can we switch to standard "goto into error path" error
handling?

> +		input_free_device(info->input_dev);
> +		kfree(info);
> +		return ret;
> +	}
> +
> +	ret = input_ff_create_memless(info->input_dev, info, vibra_play);
> +	if (ret < 0) {
> +		dev_dbg(&pdev->dev, "couldn't register vibrator to FF\n");
> +		input_unregister_device(info->input_dev);
> +		kfree(info);
> +		return ret;
> +	}
> +
> +	vibra_disable_leds();
> +
> +	return 0;
> +}
> +
> +static int __devexit twl4030_vibra_remove(struct platform_device *pdev)
> +{
> +	struct vibra_info *info = platform_get_drvdata(pdev);
> +
> +	cancel_work_sync(&info->play_work);
> +	destroy_workqueue(info->workqueue);
> +	info->workqueue = NULL;
> +	if (info->enabled)
> +		vibra_disable(info);
> +	/* this also free ff-memless which (in turn) kfree info */
> +	input_unregister_device(info->input_dev);
> +
> +	return 0;
> +}
> +
> +static struct platform_driver twl4030_vibra_driver = {
> +	.probe		= twl4030_vibra_probe,
> +	.remove		= __devexit_p(twl4030_vibra_remove),
> +	.suspend	= twl4030_vibra_suspend,
> +	.resume		= twl4030_vibra_resume,
> +	.shutdown	= twl4030_vibra_shutdown,
> +	.driver		= {
> +		.name	= "twl4030_codec_vibra",
> +		.owner	= THIS_MODULE,
> +	},
> +};
> +
> +static int __init twl4030_vibra_init(void)
> +{
> +	return platform_driver_register(&twl4030_vibra_driver);
> +}
> +module_init(twl4030_vibra_init);
> +
> +static void __exit twl4030_vibra_exit(void)
> +{
> +	platform_driver_unregister(&twl4030_vibra_driver);
> +}
> +module_exit(twl4030_vibra_exit);
> +
> +MODULE_ALIAS("platform:twl4030_codec_vibra");
> +
> +MODULE_DESCRIPTION("HID TWL4030 Vibra driver");
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Nokia Corporation");

Thanks.

-- 
Dmitry

Re: [PATCH] HID: TWL4030: add HID Force Feedback vibra

[Jari Vanhala]

On Mon, 2010-02-22 at 07:40 +0100, ext Dmitry Torokhov wrote:
> > +config HID_TWL4030_VIBRA
> > +	tristate "HID Support for TWL4030 Vibrator"
> > +	depends on TWL4030_CORE
> > +	select TWL4030_CODEC
> > +	select INPUT_FF_MEMLESS
> > +	---help---
> > +	  This option enables support for TWL4030 Vibrator Driver.
> > +
> 
> To compile this driver as a module...

Um. You mean help-text.. Added.

> > +struct vibra_info {
> > +	struct device		*dev;
> > +	struct input_dev	*input_dev;
> > +
> > +	struct workqueue_struct *workqueue;
> 
> Why do we need a separate workqueue? Can't keventd serve us?

There were problems with too long delays once in a while, own workqueue
made things much better. Also, you can change priority if needed.

> > +	struct work_struct	play_work;
> > +
> > +	int			enabled;
> 
> Use bool?

Ok.

> > +	int			speed;
> > +	int			direction;
> > +
> > +	int			coexist;
> 
> Here as well?

Ok.

> > +static int vibra_play(struct input_dev *dev, void *data,
> > +		      struct ff_effect *effect)
> > +{
> > +	struct vibra_info *info = data;
> > +
> > +	if (!info->workqueue)
> > +		return 0;
> 
> How can workqueue be missig here?

It's missing when we remove driver. ff-memless wants to stop vibra (if
it was running) and destroys info. And also we really don't want to
start worker anymore.

> > +static int twl4030_vibra_resume(struct platform_device *pdev)
> > +{
> > +	vibra_disable_leds();
> > +	return 0;
> > +}
> Please convert ot dev_pm_ops.

Hum, pm...

> > +	info->input_dev = input_allocate_device();
> > +	if (info->input_dev == NULL) {
> > +		dev_err(&pdev->dev, "couldn't allocate input device\n");
> > +		kfree(info);
> 
> Leaking workqueue.

Oops.

> > +		return -ENOMEM;
> > +	}
> > +	input_set_drvdata(info->input_dev, info);
> > +
> > +	info->input_dev->name = "twl4030:vibrator";
> > +	info->input_dev->id.version = 1;
> > +	info->input_dev->dev.parent = pdev->dev.parent;
> > +	set_bit(FF_RUMBLE, info->input_dev->ffbit);
> > +
> > +	ret = input_register_device(info->input_dev);
> > +	if (ret < 0) {
> > +		dev_dbg(&pdev->dev, "couldn't register input device\n");
> 
> Here as well... Can we switch to standard "goto into error path" error
> handling?

Too bad that freeing input is different at different stages, but I moved
simple cleanup to error path. I will send corrected version after I test
changes.

	++Jam

Re: [PATCH] HID: TWL4030: add HID Force Feedback vibra

[Dmitry Torokhov]

On Mon, Feb 22, 2010 at 03:19:36PM +0200, Jari Vanhala wrote:
> On Mon, 2010-02-22 at 07:40 +0100, ext Dmitry Torokhov wrote:
> > > +config HID_TWL4030_VIBRA
> > > +	tristate "HID Support for TWL4030 Vibrator"
> > > +	depends on TWL4030_CORE
> > > +	select TWL4030_CODEC
> > > +	select INPUT_FF_MEMLESS
> > > +	---help---
> > > +	  This option enables support for TWL4030 Vibrator Driver.
> > > +
> > 
> > To compile this driver as a module...
> 
> Um. You mean help-text.. Added.
> 
> > > +struct vibra_info {
> > > +	struct device		*dev;
> > > +	struct input_dev	*input_dev;
> > > +
> > > +	struct workqueue_struct *workqueue;
> > 
> > Why do we need a separate workqueue? Can't keventd serve us?
> 
> There were problems with too long delays once in a while, own workqueue
> made things much better. Also, you can change priority if needed.
> 

OK, in this case you shoudl consider starting it only when input device
is opened and shutting it down when it is closed.

> > > +	struct work_struct	play_work;
> > > +
> > > +	int			enabled;
> > 
> > Use bool?
> 
> Ok.
> 
> > > +	int			speed;
> > > +	int			direction;
> > > +
> > > +	int			coexist;
> > 
> > Here as well?
> 
> Ok.
> 
> > > +static int vibra_play(struct input_dev *dev, void *data,
> > > +		      struct ff_effect *effect)
> > > +{
> > > +	struct vibra_info *info = data;
> > > +
> > > +	if (!info->workqueue)
> > > +		return 0;
> > 
> > How can workqueue be missig here?
> 
> It's missing when we remove driver. ff-memless wants to stop vibra (if
> it was running) and destroys info. And also we really don't want to
> start worker anymore.

I would expect the code to make sure workqueue is present while there
are any outstanding playback requests. Otherwise you need more locking
(what stops workqueue from being deleted right after you checked it?)

> 
> > > +static int twl4030_vibra_resume(struct platform_device *pdev)
> > > +{
> > > +	vibra_disable_leds();
> > > +	return 0;
> > > +}
> > Please convert ot dev_pm_ops.
> 
> Hum, pm...
> 
> > > +	info->input_dev = input_allocate_device();
> > > +	if (info->input_dev == NULL) {
> > > +		dev_err(&pdev->dev, "couldn't allocate input device\n");
> > > +		kfree(info);
> > 
> > Leaking workqueue.
> 
> Oops.
> 
> > > +		return -ENOMEM;
> > > +	}
> > > +	input_set_drvdata(info->input_dev, info);
> > > +
> > > +	info->input_dev->name = "twl4030:vibrator";
> > > +	info->input_dev->id.version = 1;
> > > +	info->input_dev->dev.parent = pdev->dev.parent;
> > > +	set_bit(FF_RUMBLE, info->input_dev->ffbit);
> > > +
> > > +	ret = input_register_device(info->input_dev);
> > > +	if (ret < 0) {
> > > +		dev_dbg(&pdev->dev, "couldn't register input device\n");
> > 
> > Here as well... Can we switch to standard "goto into error path" error
> > handling?
> 
> Too bad that freeing input is different at different stages, but I moved
> simple cleanup to error path. I will send corrected version after I test
> changes.
> 

It is quite easy to handle - either register input device last or, after
calling unput_unregister_device() set the variable to NULL - subsequent
input_free_device() will happily accept it.

-- 
Dmitry

Re: [PATCH] HID: TWL4030: add HID Force Feedback vibra

[Jari Vanhala]

On Mon, 2010-02-22 at 19:38 +0100, ext Dmitry Torokhov wrote:
> On Mon, Feb 22, 2010 at 03:19:36PM +0200, Jari Vanhala wrote:
> > On Mon, 2010-02-22 at 07:40 +0100, ext Dmitry Torokhov wrote:
> > > > +struct vibra_info {
> > > > +	struct device		*dev;
> > > > +	struct input_dev	*input_dev;
> > > > +
> > > > +	struct workqueue_struct *workqueue;
> > > 
> > > Why do we need a separate workqueue? Can't keventd serve us?
> > 
> > There were problems with too long delays once in a while, own workqueue
> > made things much better. Also, you can change priority if needed.
> > 
> 
> OK, in this case you shoudl consider starting it only when input device
> is opened and shutting it down when it is closed.

Hum.. Delayed workqueue creating/destroying makes driver much more
complex. I create separate patch for it.

> > > > +static int vibra_play(struct input_dev *dev, void *data,
> > > > +		      struct ff_effect *effect)
> > > > +{
> > > > +	struct vibra_info *info = data;
> > > > +
> > > > +	if (!info->workqueue)
> > > > +		return 0;
> > > 
> > > How can workqueue be missig here?
> > 
> > It's missing when we remove driver. ff-memless wants to stop vibra (if
> > it was running) and destroys info. And also we really don't want to
> > start worker anymore.
> 
> I would expect the code to make sure workqueue is present while there
> are any outstanding playback requests. Otherwise you need more locking
> (what stops workqueue from being deleted right after you checked it?)

I think locking is kind of overkill. Vibra_play is run in atomic-context
and.. but I can add spinlock to protect work.

	++Jam