[v4,11/18] input: Add initial support for TWL6040 vibrator

Message ID	1307706876-4768-12-git-send-email-peter.ujfalusi@ti.com (mailing list archive)
State	Superseded, archived
Delegated to:	Tony Lindgren
Headers	show Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by demeter2.kernel.org (8.14.4/8.14.4) with ESMTP id p5ABuTnv009499 for <patchwork-omap@patchwork.kernel.org>; Fri, 10 Jun 2011 11:56:30 GMT From: Peter Ujfalusi <peter.ujfalusi@ti.com> To: Liam Girdwood <lrg@ti.com>, Tony Lindgren <tony@atomide.com>, Mark Brown <broonie@opensource.wolfsonmicro.com>, Samuel Ortiz <sameo@linux.intel.com>, Dmitry Torokhov <dmitry.torokhov@gmail.com> CC: <linux-input@vger.kernel.org>, <linux-omap@vger.kernel.org>, <linux-kernel@vger.kernel.org>, <alsa-devel@alsa-project.org>, Misael Lopez Cruz <misael.lopez@ti.com>, Jorge Eduardo Candelaria <jorge.candelaria@ti.com>, Peter Ujfalusi <peter.ujfalusi@ti.com> Subject: [PATCH v4 11/18] input: Add initial support for TWL6040 vibrator Date: Fri, 10 Jun 2011 14:54:29 +0300 Message-ID: <1307706876-4768-12-git-send-email-peter.ujfalusi@ti.com> In-Reply-To: <1307706876-4768-1-git-send-email-peter.ujfalusi@ti.com> References: <1307706876-4768-1-git-send-email-peter.ujfalusi@ti.com> MIME-Version: 1.0 Content-Type: text/plain Sender: linux-omap-owner@vger.kernel.org Precedence: bulk

Peter Ujfalusi June 10, 2011, 11:54 a.m. UTC

From: Misael Lopez Cruz <misael.lopez@ti.com>

Add twl6040_vibra as a child of MFD device twl6040_codec. This
implementation covers the PCM-to-PWM mode of TWL6040 vibrator
module.

Signed-off-by: Jorge Eduardo Candelaria <jorge.candelaria@ti.com>
Signed-off-by: Misael Lopez Cruz <misael.lopez@ti.com>
Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 drivers/input/misc/Kconfig         |   11 +
 drivers/input/misc/Makefile        |    1 +
 drivers/input/misc/twl6040-vibra.c |  428 ++++++++++++++++++++++++++++++++++++
 include/linux/i2c/twl.h            |    8 +
 4 files changed, 448 insertions(+), 0 deletions(-)
 create mode 100644 drivers/input/misc/twl6040-vibra.c

Dmitry Torokhov June 11, 2011, 11:18 p.m. UTC | #1

Hi Peter,

On Fri, Jun 10, 2011 at 02:54:29PM +0300, Peter Ujfalusi wrote:
> From: Misael Lopez Cruz <misael.lopez@ti.com>
> 
> Add twl6040_vibra as a child of MFD device twl6040_codec. This
> implementation covers the PCM-to-PWM mode of TWL6040 vibrator
> module.
> 
> Signed-off-by: Jorge Eduardo Candelaria <jorge.candelaria@ti.com>
> Signed-off-by: Misael Lopez Cruz <misael.lopez@ti.com>
> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
> ---
>  drivers/input/misc/Kconfig         |   11 +
>  drivers/input/misc/Makefile        |    1 +
>  drivers/input/misc/twl6040-vibra.c |  428 ++++++++++++++++++++++++++++++++++++
>  include/linux/i2c/twl.h            |    8 +
>  4 files changed, 448 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/input/misc/twl6040-vibra.c
> 
> diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
> index 077309a..d1bf872 100644
> --- a/drivers/input/misc/Kconfig
> +++ b/drivers/input/misc/Kconfig
> @@ -275,6 +275,17 @@ config INPUT_TWL4030_VIBRA
>  	  To compile this driver as a module, choose M here. The module will
>  	  be called twl4030_vibra.
>  
> +config INPUT_TWL6040_VIBRA
> +	tristate "Support for TWL6040 Vibrator"
> +	depends on TWL4030_CORE
> +	select TWL6040_CORE
> +	select INPUT_FF_MEMLESS
> +	help
> +	  This option enables support for TWL6040 Vibrator Driver.
> +
> +	  To compile this driver as a module, choose M here. The module will
> +	  be called twl6040_vibra.
> +
>  config INPUT_UINPUT
>  	tristate "User level driver support"
>  	help
> diff --git a/drivers/input/misc/Makefile b/drivers/input/misc/Makefile
> index 38efb2c..4da7c3a 100644
> --- a/drivers/input/misc/Makefile
> +++ b/drivers/input/misc/Makefile
> @@ -40,6 +40,7 @@ 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_TWL6040_VIBRA)	+= twl6040-vibra.o
>  obj-$(CONFIG_INPUT_UINPUT)		+= uinput.o
>  obj-$(CONFIG_INPUT_WISTRON_BTNS)	+= wistron_btns.o
>  obj-$(CONFIG_INPUT_WM831X_ON)		+= wm831x-on.o
> diff --git a/drivers/input/misc/twl6040-vibra.c b/drivers/input/misc/twl6040-vibra.c
> new file mode 100644
> index 0000000..5a54515
> --- /dev/null
> +++ b/drivers/input/misc/twl6040-vibra.c
> @@ -0,0 +1,428 @@
> +/*
> + * twl6040-vibra.c - TWL6040 Vibrator driver
> + *
> + * Author:      Jorge Eduardo Candelaria <jorge.candelaria@ti.com>
> + * Author:      Misael Lopez Cruz <misael.lopez@ti.com>
> + *
> + * Copyright:   (C) 2011 Texas Instruments, Inc.
> + *
> + * Based on twl4030-vibra.c by Henrik Saari <henrik.saari@nokia.com>
> + *				Felipe Balbi <felipe.balbi@nokia.com>
> + *				Jari Vanhala <ext-javi.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/platform_device.h>
> +#include <linux/workqueue.h>
> +#include <linux/i2c/twl.h>
> +#include <linux/mfd/twl6040.h>
> +#include <linux/slab.h>
> +#include <linux/delay.h>
> +#include <linux/regulator/consumer.h>
> +
> +#define EFFECT_DIR_180_DEG	0x8000
> +
> +/* Recommended modulation index 85% */
> +#define TWL6040_VIBRA_MOD	85
> +
> +#define TWL6040_NUM_SUPPLIES 2
> +
> +struct vibra_info {
> +	struct device *dev;
> +	struct input_dev *input_dev;
> +	struct workqueue_struct *workqueue;
> +	struct work_struct play_work;
> +	struct mutex mutex;
> +
> +	bool enabled;
> +	int weak_speed;
> +	int strong_speed;
> +	int direction;
> +
> +	unsigned int vibldrv_res;
> +	unsigned int vibrdrv_res;
> +	unsigned int viblmotor_res;
> +	unsigned int vibrmotor_res;
> +
> +	struct regulator_bulk_data supplies[TWL6040_NUM_SUPPLIES];
> +
> +	struct twl6040 *twl6040;
> +};
> +
> +static irqreturn_t twl6040_vib_irq_handler(int irq, void *data)
> +{
> +	struct vibra_info *info = data;
> +	struct twl6040 *twl6040 = info->twl6040;
> +	u8 status;
> +
> +	status = twl6040_reg_read(twl6040, TWL6040_REG_STATUS);
> +	if (status & TWL6040_VIBLOCDET) {
> +		dev_warn(info->dev, "Left Vibrator overcurrent detected\n");
> +		twl6040_clear_bits(twl6040, TWL6040_REG_VIBCTLL,
> +				   TWL6040_VIBENAL);
> +	}
> +	if (status & TWL6040_VIBROCDET) {
> +		dev_warn(info->dev, "Right Vibrator overcurrent detected\n");
> +		twl6040_clear_bits(twl6040, TWL6040_REG_VIBCTLR,
> +				   TWL6040_VIBENAR);
> +	}
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static void twl6040_vibra_enable(struct vibra_info *info)
> +{
> +	struct twl6040 *twl6040 = info->twl6040;
> +	int ret = 0;

Initialization is not needed.

> +
> +	ret = regulator_bulk_enable(ARRAY_SIZE(info->supplies), info->supplies);
> +	if (ret) {
> +		dev_err(info->dev, "failed to enable regulators %d\n", ret);
> +		return;
> +	}
> +
> +	twl6040_power(info->twl6040, 1);
> +	if (twl6040_get_rev(twl6040) <= TWL6040_REV_ES1_1) {
> +		/*
> +		 * ERRATA: Disable overcurrent protection for at least
> +		 * 3ms when enabling vibrator drivers to avoid false
> +		 * overcurrent detection
> +		 */
> +		twl6040_reg_write(twl6040, TWL6040_REG_VIBCTLL,
> +				  TWL6040_VIBENAL | TWL6040_VIBCTRLL);
> +		twl6040_reg_write(twl6040, TWL6040_REG_VIBCTLR,
> +				  TWL6040_VIBENAR | TWL6040_VIBCTRLR);
> +		usleep_range(3000, 3500);
> +	}
> +
> +	twl6040_reg_write(twl6040, TWL6040_REG_VIBCTLL,
> +			  TWL6040_VIBENAL);
> +	twl6040_reg_write(twl6040, TWL6040_REG_VIBCTLR,
> +			  TWL6040_VIBENAR);
> +
> +	info->enabled = true;
> +}
> +
> +static void twl6040_vibra_disable(struct vibra_info *info)
> +{
> +	struct twl6040 *twl6040 = info->twl6040;
> +
> +	twl6040_reg_write(twl6040, TWL6040_REG_VIBCTLL, 0x00);
> +	twl6040_reg_write(twl6040, TWL6040_REG_VIBCTLR, 0x00);
> +	twl6040_power(info->twl6040, 0);
> +
> +	regulator_bulk_disable(ARRAY_SIZE(info->supplies), info->supplies);
> +
> +	info->enabled = false;
> +}
> +
> +static u8 twl6040_vibra_code(int vddvib, int vibdrv_res, int motor_res,
> +			     int speed, int direction)
> +{
> +	int vpk, max_code;
> +	u8 vibdat;
> +
> +	/* output swing */
> +	vpk = (vddvib * motor_res * TWL6040_VIBRA_MOD) /
> +		(100 * (vibdrv_res + motor_res));
> +
> +	/* 50mV per VIBDAT code step */
> +	max_code = vpk / 50;
> +	if (max_code > TWL6040_VIBDAT_MAX)
> +		max_code = TWL6040_VIBDAT_MAX;
> +
> +	/* scale speed to max allowed code */
> +	vibdat = (u8)((speed * max_code) / USHRT_MAX);
> +
> +	/* 2's complement for direction > 180 degrees */
> +	vibdat *= direction;
> +
> +	return vibdat;
> +}
> +
> +static void twl6040_vibra_set_effect(struct vibra_info *info)
> +{
> +	struct twl6040 *twl6040 = info->twl6040;
> +	u8 vibdatl, vibdatr;
> +	int volt;
> +
> +	/* weak motor */
> +	volt = regulator_get_voltage(info->supplies[0].consumer) / 1000;
> +	vibdatl = twl6040_vibra_code(volt, info->vibldrv_res,
> +				     info->viblmotor_res,
> +				     info->weak_speed, info->direction);
> +
> +	/* strong motor */
> +	volt = regulator_get_voltage(info->supplies[1].consumer) / 1000;
> +	vibdatr = twl6040_vibra_code(volt, info->vibrdrv_res,
> +				     info->vibrmotor_res,
> +				     info->strong_speed, info->direction);
> +
> +	twl6040_reg_write(twl6040, TWL6040_REG_VIBDATL, vibdatl);
> +	twl6040_reg_write(twl6040, TWL6040_REG_VIBDATR, vibdatr);
> +}
> +
> +static void vibra_play_work(struct work_struct *work)
> +{
> +	struct vibra_info *info = container_of(work,
> +				struct vibra_info, play_work);
> +
> +	mutex_lock(&info->mutex);
> +
> +	if (info->weak_speed || info->strong_speed) {
> +		if (!info->enabled)
> +			twl6040_vibra_enable(info);
> +
> +		twl6040_vibra_set_effect(info);
> +	} else if (info->enabled)
> +		twl6040_vibra_disable(info);

Why do we play with enabling/disabling the device here? Nobody can
request playing of an effect unless the device has been opened so if we
manage power state in open/close methods we should not be doing it here
I think.

> +
> +	mutex_unlock(&info->mutex);
> +}
> +
> +static int vibra_play(struct input_dev *input, void *data,
> +		      struct ff_effect *effect)
> +{
> +	struct vibra_info *info = input_get_drvdata(input);
> +	int ret;
> +
> +	info->weak_speed = effect->u.rumble.weak_magnitude;
> +	info->strong_speed = effect->u.rumble.strong_magnitude;
> +	info->direction = effect->direction < EFFECT_DIR_180_DEG ? 1 : -1;
> +
> +	ret = queue_work(info->workqueue, &info->play_work);
> +	if (!ret) {
> +		dev_info(&input->dev, "work is already on queue\n");
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int twl6040_vibra_open(struct input_dev *input)
> +{
> +	struct vibra_info *info = input_get_drvdata(input);
> +
> +	info->workqueue = create_singlethread_workqueue("vibra");
> +	if (info->workqueue == NULL) {
> +		dev_err(&input->dev, "couldn't create workqueue\n");
> +		return -ENOMEM;
> +	}

Why do we need to create a separate workqueue? With arrival of CWQ
it looks like we should be able to use one of the system-wide
workqueues for this.

> +
> +	return 0;
> +}
> +
> +static void twl6040_vibra_close(struct input_dev *input)
> +{
> +	struct vibra_info *info = input_get_drvdata(input);
> +
> +	cancel_work_sync(&info->play_work);
> +	INIT_WORK(&info->play_work, vibra_play_work);
> +	destroy_workqueue(info->workqueue);
> +	info->workqueue = NULL;
> +
> +	mutex_lock(&info->mutex);
> +
> +	if (info->enabled)
> +		twl6040_vibra_disable(info);
> +
> +	mutex_unlock(&info->mutex);
> +}
> +
> +#if CONFIG_PM

CONFIG_PM_SLEEP.

> +static int twl6040_vibra_suspend(struct device *dev)
> +{
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct vibra_info *info = platform_get_drvdata(pdev);
> +
> +	mutex_lock(&info->mutex);
> +
> +	if (info->enabled)
> +		twl6040_vibra_disable(info);
> +
> +	mutex_unlock(&info->mutex);
> +
> +	return 0;
> +}
> +
> +static SIMPLE_DEV_PM_OPS(twl6040_vibra_pm_ops,
> +			 twl6040_vibra_suspend, NULL);

Move twl6040_vibra_pm_ops definition out of the #ifdef block so you
won't need to protect it use with ifdefs later.

> +#endif
> +
> +static int __devinit twl6040_vibra_probe(struct platform_device *pdev)
> +{
> +	struct twl4030_vibra_data *pdata = pdev->dev.platform_data;
> +	struct vibra_info *info;
> +	int ret;
> +
> +	if (!pdata) {
> +		dev_err(&pdev->dev, "platform_data not available\n");
> +		return -EINVAL;
> +	}
> +
> +	info = kzalloc(sizeof(*info), GFP_KERNEL);
> +	if (!info) {
> +		dev_err(&pdev->dev, "couldn't allocate memory\n");
> +		return -ENOMEM;
> +	}
> +
> +	info->dev = &pdev->dev;
> +	info->twl6040 = dev_get_drvdata(pdev->dev.parent);
> +	info->vibldrv_res = pdata->vibldrv_res;
> +	info->vibrdrv_res = pdata->vibrdrv_res;
> +	info->viblmotor_res = pdata->viblmotor_res;
> +	info->vibrmotor_res = pdata->vibrmotor_res;
> +	if ((!info->vibldrv_res && !info->viblmotor_res) ||
> +	    (!info->vibrdrv_res && !info->vibrmotor_res)) {
> +		dev_err(info->dev, "invalid vibra driver/motor resistance\n");
> +		ret = -EINVAL;
> +		goto err_kzalloc;
> +	}
> +
> +	mutex_init(&info->mutex);
> +	INIT_WORK(&info->play_work, vibra_play_work);
> +
> +	info->input_dev = input_allocate_device();
> +	if (info->input_dev == NULL) {
> +		dev_err(info->dev, "couldn't allocate input device\n");
> +		ret = -ENOMEM;
> +		goto err_kzalloc;
> +	}
> +
> +	input_set_drvdata(info->input_dev, info);
> +
> +	info->input_dev->name = "twl6040:vibrator";
> +	info->input_dev->id.version = 1;
> +	info->input_dev->dev.parent = pdev->dev.parent;
> +	info->input_dev->open = twl6040_vibra_open;
> +	info->input_dev->close = twl6040_vibra_close;
> +	__set_bit(FF_RUMBLE, info->input_dev->ffbit);
> +
> +	ret = input_ff_create_memless(info->input_dev, NULL, vibra_play);
> +	if (ret < 0) {
> +		dev_err(info->dev, "couldn't register vibrator to FF\n");
> +		goto err_ialloc;
> +	}
> +
> +	ret = input_register_device(info->input_dev);
> +	if (ret < 0) {
> +		dev_err(info->dev, "couldn't register input device\n");
> +		goto err_iff;
> +	}
> +
> +	platform_set_drvdata(pdev, info);
> +
> +	ret = twl6040_request_irq(info->twl6040, TWL6040_IRQ_VIB,
> +				  twl6040_vib_irq_handler, 0,
> +				  "twl6040_irq_vib", info);
> +	if (ret) {
> +		dev_err(info->dev, "VIB IRQ request failed: %d\n", ret);
> +		goto err_irq;
> +	}
> +
> +	info->supplies[0].supply = "vddvibl";
> +	info->supplies[1].supply = "vddvibr";
> +	ret = regulator_bulk_get(info->dev, ARRAY_SIZE(info->supplies),
> +				 info->supplies);
> +	if (ret) {
> +		dev_err(info->dev, "couldn't get regulators %d\n", ret);
> +		goto err_regulator;
> +	}
> +
> +	if (pdata->vddvibl_uV) {
> +		ret = regulator_set_voltage(info->supplies[0].consumer,
> +					    pdata->vddvibl_uV,
> +					    pdata->vddvibl_uV);
> +		if (ret) {
> +			dev_err(info->dev, "failed to set VDDVIBL volt %d\n",
> +				ret);
> +			goto err_voltage;
> +		}
> +	}
> +
> +	if (pdata->vddvibr_uV) {
> +		ret = regulator_set_voltage(info->supplies[1].consumer,
> +					    pdata->vddvibr_uV,
> +					    pdata->vddvibr_uV);
> +		if (ret) {
> +			dev_err(info->dev, "failed to set VDDVIBR volt %d\n",
> +				ret);
> +			goto err_voltage;
> +		}
> +	}
> +
> +	return 0;
> +
> +err_voltage:
> +	regulator_bulk_free(ARRAY_SIZE(info->supplies), info->supplies);
> +err_regulator:
> +	twl6040_free_irq(info->twl6040, TWL6040_IRQ_VIB, info);
> +err_irq:
> +	input_unregister_device(info->input_dev);
> +	info->input_dev = NULL;
> +err_iff:
> +	if (info->input_dev)
> +		input_ff_destroy(info->input_dev);
> +err_ialloc:
> +	input_free_device(info->input_dev);
> +err_kzalloc:
> +	kfree(info);
> +	return ret;
> +}
> +
> +static int __devexit twl6040_vibra_remove(struct platform_device *pdev)
> +{
> +	struct vibra_info *info = platform_get_drvdata(pdev);
> +
> +	twl6040_power(info->twl6040, 0);

If we ensure that device is powered off until open() is called, and
also powered off when close() is called, then we do not need to switch
off power here.

> +	regulator_bulk_free(ARRAY_SIZE(info->supplies), info->supplies);
> +	twl6040_free_irq(info->twl6040, TWL6040_IRQ_VIB, info);
> +	input_unregister_device(info->input_dev);
> +	kfree(info);
> +
> +	return 0;
> +}
> +
> +static struct platform_driver twl6040_vibra_driver = {
> +	.probe		= twl6040_vibra_probe,
> +	.remove		= __devexit_p(twl6040_vibra_remove),
> +	.driver		= {
> +		.name	= "twl6040-vibra",
> +		.owner	= THIS_MODULE,
> +#if CONFIG_PM
> +		.pm	= &twl6040_vibra_pm_ops,
> +#endif
> +	},
> +};
> +
> +static int __init twl6040_vibra_init(void)
> +{
> +	return platform_driver_register(&twl6040_vibra_driver);
> +}
> +module_init(twl6040_vibra_init);
> +
> +static void __exit twl6040_vibra_exit(void)
> +{
> +	platform_driver_unregister(&twl6040_vibra_driver);
> +}
> +module_exit(twl6040_vibra_exit);
> +
> +MODULE_ALIAS("platform:twl6040-vibra");
> +MODULE_DESCRIPTION("TWL6040 Vibra driver");
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Jorge Eduardo Candelaria <jorge.candelaria@ti.com>");
> +MODULE_AUTHOR("Misael Lopez Cruz <misael.lopez@ti.com>");
> diff --git a/include/linux/i2c/twl.h b/include/linux/i2c/twl.h
> index ea5baa2..685fd76 100644
> --- a/include/linux/i2c/twl.h
> +++ b/include/linux/i2c/twl.h
> @@ -669,6 +669,14 @@ struct twl4030_codec_data {
>  
>  struct twl4030_vibra_data {
>  	unsigned int	coexist;
> +
> +	/* twl6040 */
> +	unsigned int vibldrv_res;	/* left driver resistance */
> +	unsigned int vibrdrv_res;	/* right driver resistance */
> +	unsigned int viblmotor_res;	/* left motor resistance */
> +	unsigned int vibrmotor_res;	/* right motor resistance */
> +	int vddvibl_uV;			/* VDDVIBL volt, set 0 for fixed reg */
> +	int vddvibr_uV;			/* VDDVIBR volt, set 0 for fixed reg */
>  };
>  
>  struct twl4030_audio_data {
> -- 
> 1.7.5.3
> 

Thanks.

Peter Ujfalusi June 13, 2011, 9:51 a.m. UTC | #2

On Sunday 12 June 2011 01:18:54 Dmitry Torokhov wrote:
> > +static void twl6040_vibra_enable(struct vibra_info *info)
> > +{
> > +     struct twl6040 *twl6040 = info->twl6040;
> > +     int ret = 0;
> 
> Initialization is not needed.

True.

> > +static void vibra_play_work(struct work_struct *work)
> > +{
> > +     struct vibra_info *info = container_of(work,
> > +                             struct vibra_info, play_work);
> > +
> > +     mutex_lock(&info->mutex);
> > +
> > +     if (info->weak_speed || info->strong_speed) {
> > +             if (!info->enabled)
> > +                     twl6040_vibra_enable(info);
> > +
> > +             twl6040_vibra_set_effect(info);
> > +     } else if (info->enabled)
> > +             twl6040_vibra_disable(info);
> 
> Why do we play with enabling/disabling the device here? Nobody can
> request playing of an effect unless the device has been opened so if we
> manage power state in open/close methods we should not be doing it here
> I think.

We want to preserve as much power as we can.
So if application opens the driver, but it is not requesting to play any 
effect we still keep the device turned off.
When application request for stopping the effect without closing the device, 
we turn off things in a similar way.
The twl4030-vibra driver has been implemented in this way as well.

> > +
> > +     mutex_unlock(&info->mutex);
> > +}

...

> > +static int twl6040_vibra_open(struct input_dev *input)
> > +{
> > +     struct vibra_info *info = input_get_drvdata(input);
> > +
> > +     info->workqueue = create_singlethread_workqueue("vibra");
> > +     if (info->workqueue == NULL) {
> > +             dev_err(&input->dev, "couldn't create workqueue\n");
> > +             return -ENOMEM;
> > +     }
> 
> Why do we need to create a separate workqueue? With arrival of CWQ
> it looks like we should be able to use one of the system-wide
> workqueues for this.

The reason for this is to ensure that we have the lowest latency as possible 
in most case. In the embedded devices we use the vibra for tactile type of 
feedbacks as well, where few tens of ms delay can be felt.

> > +
> > +     return 0;
> > +}
> > +
> > +static void twl6040_vibra_close(struct input_dev *input)
> > +{
> > +     struct vibra_info *info = input_get_drvdata(input);
> > +
> > +     cancel_work_sync(&info->play_work);
> > +     INIT_WORK(&info->play_work, vibra_play_work);
> > +     destroy_workqueue(info->workqueue);
> > +     info->workqueue = NULL;
> > +
> > +     mutex_lock(&info->mutex);
> > +
> > +     if (info->enabled)
> > +             twl6040_vibra_disable(info);
> > +
> > +     mutex_unlock(&info->mutex);
> > +}
> > +
> > +#if CONFIG_PM
> 
> CONFIG_PM_SLEEP.

OK, will be fixed.

> > +static int twl6040_vibra_suspend(struct device *dev)
> > +{
> > +     struct platform_device *pdev = to_platform_device(dev);
> > +     struct vibra_info *info = platform_get_drvdata(pdev);
> > +
> > +     mutex_lock(&info->mutex);
> > +
> > +     if (info->enabled)
> > +             twl6040_vibra_disable(info);
> > +
> > +     mutex_unlock(&info->mutex);
> > +
> > +     return 0;
> > +}
> > +
> > +static SIMPLE_DEV_PM_OPS(twl6040_vibra_pm_ops,
> > +                      twl6040_vibra_suspend, NULL);
> 
> Move twl6040_vibra_pm_ops definition out of the #ifdef block so you
> won't need to protect it use with ifdefs later.

Thanks, I have change this.
 
> > +#endif

...

> > +static int __devexit twl6040_vibra_remove(struct platform_device *pdev)
> > +{
> > +     struct vibra_info *info = platform_get_drvdata(pdev);
> > +
> > +     twl6040_power(info->twl6040, 0);
> 
> If we ensure that device is powered off until open() is called, and
> also powered off when close() is called, then we do not need to switch
> off power here.

True, removed.

> Thanks.
> 
> --
> Dmitry

Thanks for the comments. I will update the series.

Dmitry Torokhov June 13, 2011, 9:20 p.m. UTC | #3

On Mon, Jun 13, 2011 at 12:51:16PM +0300, Péter Ujfalusi wrote:
> On Sunday 12 June 2011 01:18:54 Dmitry Torokhov wrote:
> 
> > > +static int twl6040_vibra_open(struct input_dev *input)
> > > +{
> > > +     struct vibra_info *info = input_get_drvdata(input);
> > > +
> > > +     info->workqueue = create_singlethread_workqueue("vibra");
> > > +     if (info->workqueue == NULL) {
> > > +             dev_err(&input->dev, "couldn't create workqueue\n");
> > > +             return -ENOMEM;
> > > +     }
> > 
> > Why do we need to create a separate workqueue? With arrival of CWQ
> > it looks like we should be able to use one of the system-wide
> > workqueues for this.
> 
> The reason for this is to ensure that we have the lowest latency as possible 
> in most case. In the embedded devices we use the vibra for tactile type of 
> feedbacks as well, where few tens of ms delay can be felt.

Even if you create a dedicated workqueue with CWQ it will still be using
shared pool of threads so I do not think that latency will be affected
by using system-wide workqueue. I might be mistaken though, Tejun will
correct me if I am wrong...

Thanks.

Tejun Heo June 14, 2011, 6:34 a.m. UTC | #4

Hello,

On Mon, Jun 13, 2011 at 02:20:28PM -0700, Dmitry Torokhov wrote:
> On Mon, Jun 13, 2011 at 12:51:16PM +0300, Péter Ujfalusi wrote:
> > On Sunday 12 June 2011 01:18:54 Dmitry Torokhov wrote:
> > 
> > > > +static int twl6040_vibra_open(struct input_dev *input)
> > > > +{
> > > > +     struct vibra_info *info = input_get_drvdata(input);
> > > > +
> > > > +     info->workqueue = create_singlethread_workqueue("vibra");
> > > > +     if (info->workqueue == NULL) {
> > > > +             dev_err(&input->dev, "couldn't create workqueue\n");
> > > > +             return -ENOMEM;
> > > > +     }
> > > 
> > > Why do we need to create a separate workqueue? With arrival of CWQ
> > > it looks like we should be able to use one of the system-wide
> > > workqueues for this.
> > 
> > The reason for this is to ensure that we have the lowest latency as possible 
> > in most case. In the embedded devices we use the vibra for tactile type of 
> > feedbacks as well, where few tens of ms delay can be felt.
> 
> Even if you create a dedicated workqueue with CWQ it will still be using
> shared pool of threads so I do not think that latency will be affected
> by using system-wide workqueue. I might be mistaken though, Tejun will
> correct me if I am wrong...

Yeap, using a separate workqueue doesn't do anything for latency
unless WQ_HIGHPRI and/or WQ_CPU_INTENSIVE is used; however, _please_
stay away from it unless absolutely sure it's necessary (ie. unless
you can pin point to where latency is coming from - even in that case,
the thing which induces the latency probably is the one which should
be fixed).

CMWQ is pretty good at keeping latency low unless something is
consuming large amount of CPU cycles and those work items are marked
WQ_CPU_INTENSIVE, not the other way around and WQ_HIGHPRI is for
things like MCE error reporting.

Thanks.

Peter Ujfalusi June 14, 2011, 7:17 a.m. UTC | #5

On Tuesday 14 June 2011 08:34:00 Tejun Heo wrote:
> Yeap, using a separate workqueue doesn't do anything for latency
> unless WQ_HIGHPRI and/or WQ_CPU_INTENSIVE is used; however, _please_
> stay away from it unless absolutely sure it's necessary (ie. unless
> you can pin point to where latency is coming from - even in that case,
> the thing which induces the latency probably is the one which should
> be fixed).

The latency in most cases comes from the fact, that we are running an embedded 
system. Number of peripherals are connected via I2C, these drivers are using 
workqueues to communicate with the IC.
Since only one device can communicate through I2C at the time. This is 
basically the source of the latency. It does not really matter, if the devices 
are on the same I2C bus or not, it is enough if two work belonging to device, 
which happens to be on the same I2C bus, and the first work in the queue takes 
long time to complete (reading back bigger chunk of info, configuring, etc).
Even if we could schedule the second work on the other CPU, it will be put 
waiting till the I2C bus is free, so both CPU core has work assigned, the 
first is keeping the I2C bus, the other waits for the I2C bus, and the third 
is waiting to be scheduled (which will be happening, when the first work 
finished).
IMHO the tactile feedback (vibra) should have an excuse to have separate WQ to 
avoid latency spikes.
I agree, that most cases we can use the global wq.

> CMWQ is pretty good at keeping latency low unless something is
> consuming large amount of CPU cycles and those work items are marked
> WQ_CPU_INTENSIVE, not the other way around and WQ_HIGHPRI is for
> things like MCE error reporting.

So this is not really about CPU utilization, it is due to the wide variety of 
peripherals connected to an embedded system.

Tejun Heo June 14, 2011, 7:31 a.m. UTC | #6

Hello,

On Tue, Jun 14, 2011 at 10:17:10AM +0300, Péter Ujfalusi wrote:
> The latency in most cases comes from the fact, that we are running an embedded 
> system. Number of peripherals are connected via I2C, these drivers are using 
> workqueues to communicate with the IC.
> Since only one device can communicate through I2C at the time. This is 
> basically the source of the latency. It does not really matter, if the devices 
> are on the same I2C bus or not, it is enough if two work belonging to device, 
> which happens to be on the same I2C bus, and the first work in the queue takes 
> long time to complete (reading back bigger chunk of info, configuring, etc).
> Even if we could schedule the second work on the other CPU, it will be put 
> waiting till the I2C bus is free, so both CPU core has work assigned, the 
> first is keeping the I2C bus, the other waits for the I2C bus, and the third 
> is waiting to be scheduled (which will be happening, when the first work 
> finished).
> IMHO the tactile feedback (vibra) should have an excuse to have separate WQ to 
> avoid latency spikes.
> I agree, that most cases we can use the global wq.

Thanks for the explanation.  I have a couple more questions.

* While transferring data from I2C, I suppose the work item is fully
  occupying the CPU?  If so, how long delay are we talking about?
  Millisecs?

* You said that the if one task is accessing I2C bus, the other would
  wait even if scheduled on a different CPU.  Is access to I2C bus
  protected with a spinlock?

Also, as it's currently implemented, single threaded wq's effectively
bypass concurrency level control.  This is an implementation detail
which may change in the future, so even if you're seeing lower latency
by using a separate single threaded wq, it's an accident and if you
require lower latency you should be expressing the requirement
explicitly.

Thank you.

Peter Ujfalusi June 14, 2011, 7:51 a.m. UTC | #7

On Tuesday 14 June 2011 09:31:30 Tejun Heo wrote:
> Thanks for the explanation.  I have a couple more questions.
> 
> * While transferring data from I2C, I suppose the work item is fully
>   occupying the CPU?

Not exactly on OMAP platforms at least. We do not have busy looping in low 
level driver (we wait with wait_for_completion_timeout for the transfer to be 
done), so scheduling on the same CPU can be possible.

>   If so, how long delay are we talking about?
>   Millisecs?

It is hard to predict, but it can be few tens of ms for one device, if we have 
several devices on the same bus (which we tend to have), and they want to 
read/write something at the same time we can see hundred(s) ms in total - it 
is rare to happen, and hard to reproduce, but it does happen for sure.

> * You said that the if one task is accessing I2C bus, the other would
>   wait even if scheduled on a different CPU.  Is access to I2C bus
>   protected with a spinlock?

At the bottom it is using rt_mutex_lock/unlick to protect the bus.
And yes, the others need to wait till the ongoing transfer has been finished.

> Also, as it's currently implemented, single threaded wq's effectively
> bypass concurrency level control.  This is an implementation detail
> which may change in the future, so even if you're seeing lower latency
> by using a separate single threaded wq, it's an accident and if you
> require lower latency you should be expressing the requirement
> explicitly.

I see, do you have suggestion to which would be best for this kind of 
scenarios.
In most cases global wq would be OK for this, but time-to-time we face with 
sudden latency spikes, which makes the user experience really bad.
Currently with singlethreaded wq we can avoid these spikes.

Thank you, 
Péter
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Tejun Heo June 14, 2011, 8:18 a.m. UTC | #8

Hello,

On Tue, Jun 14, 2011 at 10:51:35AM +0300, Péter Ujfalusi wrote:
> On Tuesday 14 June 2011 09:31:30 Tejun Heo wrote:
> > Thanks for the explanation.  I have a couple more questions.
> > 
> > * While transferring data from I2C, I suppose the work item is fully
> >   occupying the CPU?
> 
> Not exactly on OMAP platforms at least. We do not have busy looping in low 
> level driver (we wait with wait_for_completion_timeout for the transfer to be 
> done), so scheduling on the same CPU can be possible.
> 
> >   If so, how long delay are we talking about?
> >   Millisecs?
> 
> It is hard to predict, but it can be few tens of ms for one device, if we have 
> several devices on the same bus (which we tend to have), and they want to 
> read/write something at the same time we can see hundred(s) ms in total - it 
> is rare to happen, and hard to reproduce, but it does happen for sure.
>  
> > * You said that the if one task is accessing I2C bus, the other would
> >   wait even if scheduled on a different CPU.  Is access to I2C bus
> >   protected with a spinlock?
> 
> At the bottom it is using rt_mutex_lock/unlick to protect the bus.
> And yes, the others need to wait till the ongoing transfer has been finished.

I see, so IIUC,

* If it's using mutex and not holding CPU for the whole duration, you
  shouldn't need to do anything special for latency for other work
  items.  Workqueue code will start executing other work items as soon
  as the I2C work item goes to sleep.

* If I2C work item is burning CPU cycles for the whole duration which
  may stretch to tens / few hundreds millsecs, 1. it's doing something
  quite wrong, 2. should be marked WQ_CPU_INTENSIVE.

So, if something needs to be modified, it's the I2C stuff, not the
vibrator driver.  If I2C stuff isn't doing something wonky, there
shouldn't be a latency problem to begin with.

Thank you.

Peter Ujfalusi June 14, 2011, 10:22 a.m. UTC | #9

On Tuesday 14 June 2011 10:18:36 Tejun Heo wrote:
> I see, so IIUC,
> 
> * If it's using mutex and not holding CPU for the whole duration, you
>   shouldn't need to do anything special for latency for other work
>   items.  Workqueue code will start executing other work items as soon
>   as the I2C work item goes to sleep.

I see.
 
> * If I2C work item is burning CPU cycles for the whole duration which
>   may stretch to tens / few hundreds millsecs, 1. it's doing something
>   quite wrong, 2. should be marked WQ_CPU_INTENSIVE.
> 
> So, if something needs to be modified, it's the I2C stuff, not the
> vibrator driver.  If I2C stuff isn't doing something wonky, there
> shouldn't be a latency problem to begin with.

In case of OMAP the former is the case regarding to I2C.

However I did run a short experiments regarding to latencies:
With create_singlethread_workqueue :
Jun 14 12:54:30 omap-gentoo kernel: [  211.269531] vibra scheduling time: 30 usec
Jun 14 12:54:30 omap-gentoo kernel: [  211.300811] vibra scheduling time: 30 usec
Jun 14 12:54:33 omap-gentoo kernel: [  214.419006] vibra scheduling time: 31 usec
Jun 14 12:54:34 omap-gentoo kernel: [  214.980987] vibra scheduling time: 30 usec
Jun 14 12:54:35 omap-gentoo kernel: [  215.762115] vibra scheduling time: 30 usec
Jun 14 12:54:35 omap-gentoo kernel: [  215.816650] vibra scheduling time: 30 usec
Jun 14 12:54:35 omap-gentoo kernel: [  215.871337] vibra scheduling time: 61 usec
Jun 14 12:54:35 omap-gentoo kernel: [  215.926025] vibra scheduling time: 61 usec
Jun 14 12:54:35 omap-gentoo kernel: [  215.980743] vibra scheduling time: 61 usec
Jun 14 12:54:35 omap-gentoo kernel: [  216.035430] vibra scheduling time: 61 usec
Jun 14 12:54:38 omap-gentoo kernel: [  219.425659] vibra scheduling time: 31 usec
Jun 14 12:54:40 omap-gentoo kernel: [  220.981658] vibra scheduling time: 31 usec
Jun 14 12:54:44 omap-gentoo kernel: [  224.692504] vibra scheduling time: 30 usec
Jun 14 12:54:44 omap-gentoo kernel: [  225.067138] vibra scheduling time: 30 usec

With create_workqueue :
Jun 14 12:05:00 omap-gentoo kernel: [  304.965393] vibra scheduling time: 183 usec
Jun 14 12:05:01 omap-gentoo kernel: [  305.964996] vibra scheduling time: 61 usec
Jun 14 12:05:03 omap-gentoo kernel: [  307.684082] vibra scheduling time: 152 usec
Jun 14 12:05:06 omap-gentoo kernel: [  310.972778] vibra scheduling time: 30 usec
Jun 14 12:05:08 omap-gentoo kernel: [  312.683715] vibra scheduling time: 61 usec
Jun 14 12:05:10 omap-gentoo kernel: [  314.785675] vibra scheduling time: 183 usec
Jun 14 12:05:15 omap-gentoo kernel: [  319.800903] vibra scheduling time: 61 usec
Jun 14 12:05:16 omap-gentoo kernel: [  320.738403] vibra scheduling time: 30 usec
Jun 14 12:05:16 omap-gentoo kernel: [  320.793090] vibra scheduling time: 61 usec
Jun 14 12:05:16 omap-gentoo kernel: [  320.847778] vibra scheduling time: 61 usec
Jun 14 12:05:16 omap-gentoo kernel: [  320.902465] vibra scheduling time: 61 usec
Jun 14 12:05:16 omap-gentoo kernel: [  320.957153] vibra scheduling time: 61 usec
Jun 14 12:05:16 omap-gentoo kernel: [  320.996185] vibra scheduling time: 31 usec

This is in a system, where I do not have any other drivers on I2C bus, and I have
generated some load with this command:
grep -r generate_load /*

So, I have some CPU, and IO load as well.

At the end the differences are not that big, but with create_singlethread_workqueue
I can see less spikes.

This is with 3.0-rc2 kernel

I still think, that there is a place for the create_singlethread_workqueue, and the
tactile feedback needs such a thing.

As I recall correctly this was the reason to use create_singlethread_workqueue
in the twl4030-vibra driver as well (there were latency issues without it).

Tejun Heo June 15, 2011, 8:18 a.m. UTC | #10

Hello,

On Tue, Jun 14, 2011 at 01:22:45PM +0300, Péter Ujfalusi wrote:
> However I did run a short experiments regarding to latencies:
> With create_singlethread_workqueue :
> Jun 14 12:54:30 omap-gentoo kernel: [  211.269531] vibra scheduling time: 30 usec
> Jun 14 12:54:30 omap-gentoo kernel: [  211.300811] vibra scheduling time: 30 usec
> Jun 14 12:54:33 omap-gentoo kernel: [  214.419006] vibra scheduling time: 31 usec
> Jun 14 12:54:34 omap-gentoo kernel: [  214.980987] vibra scheduling time: 30 usec
> Jun 14 12:54:35 omap-gentoo kernel: [  215.762115] vibra scheduling time: 30 usec
> Jun 14 12:54:35 omap-gentoo kernel: [  215.816650] vibra scheduling time: 30 usec
> Jun 14 12:54:35 omap-gentoo kernel: [  215.871337] vibra scheduling time: 61 usec
> Jun 14 12:54:35 omap-gentoo kernel: [  215.926025] vibra scheduling time: 61 usec
> Jun 14 12:54:35 omap-gentoo kernel: [  215.980743] vibra scheduling time: 61 usec
> Jun 14 12:54:35 omap-gentoo kernel: [  216.035430] vibra scheduling time: 61 usec
> Jun 14 12:54:38 omap-gentoo kernel: [  219.425659] vibra scheduling time: 31 usec
> Jun 14 12:54:40 omap-gentoo kernel: [  220.981658] vibra scheduling time: 31 usec
> Jun 14 12:54:44 omap-gentoo kernel: [  224.692504] vibra scheduling time: 30 usec
> Jun 14 12:54:44 omap-gentoo kernel: [  225.067138] vibra scheduling time: 30 usec
> 
> With create_workqueue :
> Jun 14 12:05:00 omap-gentoo kernel: [  304.965393] vibra scheduling time: 183 usec
> Jun 14 12:05:01 omap-gentoo kernel: [  305.964996] vibra scheduling time: 61 usec
> Jun 14 12:05:03 omap-gentoo kernel: [  307.684082] vibra scheduling time: 152 usec
> Jun 14 12:05:06 omap-gentoo kernel: [  310.972778] vibra scheduling time: 30 usec
> Jun 14 12:05:08 omap-gentoo kernel: [  312.683715] vibra scheduling time: 61 usec
> Jun 14 12:05:10 omap-gentoo kernel: [  314.785675] vibra scheduling time: 183 usec
> Jun 14 12:05:15 omap-gentoo kernel: [  319.800903] vibra scheduling time: 61 usec
> Jun 14 12:05:16 omap-gentoo kernel: [  320.738403] vibra scheduling time: 30 usec
> Jun 14 12:05:16 omap-gentoo kernel: [  320.793090] vibra scheduling time: 61 usec
> Jun 14 12:05:16 omap-gentoo kernel: [  320.847778] vibra scheduling time: 61 usec
> Jun 14 12:05:16 omap-gentoo kernel: [  320.902465] vibra scheduling time: 61 usec
> Jun 14 12:05:16 omap-gentoo kernel: [  320.957153] vibra scheduling time: 61 usec
> Jun 14 12:05:16 omap-gentoo kernel: [  320.996185] vibra scheduling time: 31 usec
> 
> This is in a system, where I do not have any other drivers on I2C bus, and I have
> generated some load with this command:
> grep -r generate_load /*
> 
> So, I have some CPU, and IO load as well.
> 
> At the end the differences are not that big, but with create_singlethread_workqueue
> I can see less spikes.
> 
> This is with 3.0-rc2 kernel
> 
> I still think, that there is a place for the create_singlethread_workqueue, and the
> tactile feedback needs such a thing.

So, yes, WQ_HIGHPRI would make difference in latency but as can be
seen above in usecs range not msecs range and you're trading off batch
execution and processing locality for it.

> As I recall correctly this was the reason to use create_singlethread_workqueue
> in the twl4030-vibra driver as well (there were latency issues without it).

Before cmwq, the latency which can be induced by using system
workqueue can be easily upto seconds range.  With cmwq, it should be
in the usecs to few millisecs range.  If that's not enough, the use
case probably calls for dedicated RT thread or threaded IRQ handler.

No human being can feel 120usec difference and I can't see how using
HIGHPRI is justified here (which is what the code is doing
_accidentally_ by using singlethread_workqueue).

Thank you.

Tejun Heo June 15, 2011, 8:23 a.m. UTC | #11

On Wed, Jun 15, 2011 at 10:18:58AM +0200, Tejun Heo wrote:
> No human being can feel 120usec difference and I can't see how using
> HIGHPRI is justified here (which is what the code is doing
> _accidentally_ by using singlethread_workqueue).

Ooh, one more thing, and even if you insist on using HIGHPRI (please
don't), you don't need to create workqueue for each device.  You can
just create one for the whole driver in init and destroy it from exit.
What matters is the HIGHPRI attribute of the workqueue.  The number of
workqueues is completely irrelevant.

Thank you.

Peter Ujfalusi June 16, 2011, 11:13 a.m. UTC | #12

On Wednesday 15 June 2011 10:23:01 Tejun Heo wrote:
> On Wed, Jun 15, 2011 at 10:18:58AM +0200, Tejun Heo wrote:
> > No human being can feel 120usec difference and I can't see how using
> > HIGHPRI is justified here (which is what the code is doing
> > _accidentally_ by using singlethread_workqueue).
> 
> Ooh, one more thing, and even if you insist on using HIGHPRI (please
> don't), you don't need to create workqueue for each device.  You can
> just create one for the whole driver in init and destroy it from exit.
> What matters is the HIGHPRI attribute of the workqueue.  The number of
> workqueues is completely irrelevant.

Fair enough.
I'll move to create_workqueue.
If we later find issues with this (in a 'live' system), we can figure out a 
way to fix it.

Thank you for your time on this.
I'll make the changes accordingly.

Regards,
Péter
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Tejun Heo June 16, 2011, 12:02 p.m. UTC | #13

Hello,

On Thu, Jun 16, 2011 at 02:13:59PM +0300, Péter Ujfalusi wrote:
> On Wednesday 15 June 2011 10:23:01 Tejun Heo wrote:
> > On Wed, Jun 15, 2011 at 10:18:58AM +0200, Tejun Heo wrote:
> > > No human being can feel 120usec difference and I can't see how using
> > > HIGHPRI is justified here (which is what the code is doing
> > > _accidentally_ by using singlethread_workqueue).
> > 
> > Ooh, one more thing, and even if you insist on using HIGHPRI (please
> > don't), you don't need to create workqueue for each device.  You can
> > just create one for the whole driver in init and destroy it from exit.
> > What matters is the HIGHPRI attribute of the workqueue.  The number of
> > workqueues is completely irrelevant.
> 
> Fair enough.
> I'll move to create_workqueue.

I suppose you meant alloc_workqueue()? :)

Sorry about the confusing names, I'm still in the (slow) process of
deprecating older APIs.

Thanks.

Peter Ujfalusi June 16, 2011, 2:06 p.m. UTC | #14

Hello,

On Thursday 16 June 2011 14:02:44 Tejun Heo wrote:
> > I'll move to create_workqueue.
> 
> I suppose you meant alloc_workqueue()? :)

Oh, yes. I mean that.
 
> Sorry about the confusing names, I'm still in the (slow) process of
> deprecating older APIs.

Will do this soon, and I ping the other maintainers, if they see other issues 
with the series, so I can make the change at the same time.

Thanks,
Péter
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Peter Ujfalusi June 17, 2011, 9:39 a.m. UTC | #15

Hello Tejun,

On Thursday 16 June 2011 16:06:00 Ujfalusi, Peter wrote:
> > I suppose you meant alloc_workqueue()? :)
> 
> Oh, yes. I mean that.

Just avoid another series...
I have looked at the alloc_workqueue, and I'm not really sure what parameters 
should I use.
#define create_singlethread_workqueue(name)			\
	alloc_workqueue((name), WQ_UNBOUND | WQ_MEM_RECLAIM, 1)

I would use something like this instead of create_singlethread_workqueue:

alloc_workqueue("twl6040-vibra", WQ_UNBOUND | WQ_MEM_RECLAIM | WQ_HIGHPRI, 1);

Is this correct?

Thanks,
Péter
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Tejun Heo June 17, 2011, 9:43 a.m. UTC | #16

Hello, Péter.

On Fri, Jun 17, 2011 at 12:39:57PM +0300, Péter Ujfalusi wrote:
> Just avoid another series...
> I have looked at the alloc_workqueue, and I'm not really sure what parameters 
> should I use.
> #define create_singlethread_workqueue(name)			\
> 	alloc_workqueue((name), WQ_UNBOUND | WQ_MEM_RECLAIM, 1)
> 
> I would use something like this instead of create_singlethread_workqueue:
> 
> alloc_workqueue("twl6040-vibra", WQ_UNBOUND | WQ_MEM_RECLAIM | WQ_HIGHPRI, 1);

Just use the default params

	alloc_workqueue("twl6040-vibra", 0, 0);

Thanks.

Peter Ujfalusi June 17, 2011, 10:59 a.m. UTC | #17

On Friday 17 June 2011 11:43:32 Tejun Heo wrote:
> Just use the default params
> 
> 	alloc_workqueue("twl6040-vibra", 0, 0);
> 
> Thanks.

I'll do this for the next posting

Thanks,
Péter
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Dmitry Torokhov June 18, 2011, 2:57 p.m. UTC | #18

On Fri, Jun 17, 2011 at 01:59:47PM +0300, Péter Ujfalusi wrote:
> On Friday 17 June 2011 11:43:32 Tejun Heo wrote:
> > Just use the default params
> > 
> > 	alloc_workqueue("twl6040-vibra", 0, 0);
> > 
> > Thanks.
> 
> I'll do this for the next posting
> 

Tejun,

For my education, what is the benefit of creating a dedicated workqueue
with alloc_workqueue (which, as far as I understand, does not end up
having dedicated worker threads but will use the common pool) and simply
queueing the jobs on system-wide workqueue?

Thanks.

Tejun Heo June 18, 2011, 3:36 p.m. UTC | #19

Hello,

2011/6/18 Dmitry Torokhov <dmitry.torokhov@gmail.com>:
> For my education, what is the benefit of creating a dedicated workqueue
> with alloc_workqueue (which, as far as I understand, does not end up
> having dedicated worker threads but will use the common pool) and simply
> queueing the jobs on system-wide workqueue?

In this case, nothing really, but Péter seems to want to have a
dedicated workqueue so that he can later flip HIGHPRI easily if
necessary.  Usually what a separate workqueue buys are...

* It serves as a flushing domain.  ie. You can flush work items queued
to the same workqueue together.  This is useful when individual work
items can't be flushed (e.g. they free themselves) or doing so is
inefficient.

* It serves as an attribute domain.  ie. You can set WQ_* flags and
@max_active.  If using the default values, nothing really is different
from using system_wq.

Thanks.

[v4,11/18] input: Add initial support for TWL6040 vibrator

Commit Message

Comments

Patch