Re: [RFC v2 PATCH 7/7] input: misc: Add support for PM8058 based vibrator driver

Hi Anirudh,

On Tue, Feb 01, 2011 at 07:17:43PM +0530, Anirudh Ghayal wrote:

+
+#include <linux/mfd/pmic8058.h>
+#include <linux/input/pmic8058-vibrator.h>

Where is this header file? Shouldn't it be part of this patch?

+
+#define VIB_DRV			0x4A
+
+#define VIB_DRV_SEL_MASK	0xf8
+#define VIB_DRV_SEL_SHIFT	0x03
+#define VIB_DRV_EN_MANUAL_MASK	0xfc
+
+#define VIB_MAX_LEVEL_mV	(3100)
+#define VIB_MIN_LEVEL_mV	(1200)
+#define VIB_MAX_LEVELS		(VIB_MAX_LEVEL_mV - VIB_MIN_LEVEL_mV)
+
+#define MAX_FF_SPEED		0xff
+
+/*
+ * struct pmic8058_vib - structure to hold vibrator data
+ * vib_input_dev: input device supporting force feedback
+ * work: work structure to set the vibration parameters
+ * dev: device supporting force feedback
+ * pdata: platform data
+ * pm_chip: reference to pmic chip to carry out read/write operations
+ * speed: speed of vibration set from userland
+ * state: state of vibrator
+ * level: level of vibration to set in the chip
+ * reg_vib_drv: VIB_DRV register value

Hmm, this is kind of kerneldoc but not quite (kerneldoc's arguments
start with '@').

+ *
+ */
+struct pmic8058_vib {
+	struct input_dev *vib_input_dev;
+	struct work_struct work;
+	struct device *dev;
+	const struct pmic8058_vibrator_pdata *pdata;
+	struct pm8058_chip	*pm_chip;
+	int speed;
+	int state;
+	int level;
+	u8  reg_vib_drv;
+};
+
+/*
+ * pmic8058_vib_read_u8 - helper to read a byte from pmic chip
+ * vib: pointer to vibrator structure
+ * data: placeholder for data to be read
+ * reg: register address
+ *
+ */
+static int pmic8058_vib_read_u8(struct pmic8058_vib *vib,
+				 u8 *data, u16 reg)
+{
+	int rc;
+
+	rc = pm8058_read(vib->pm_chip, reg, data, 1);
+	if (rc < 0)
+		dev_warn(vib->dev, "Error reading pmic8058 reg 0x%x(0x%x)\n",
+				reg, rc);
+	return rc;
+}
+
+/*
+ * pmic8058_vib_write_u8 - helper to write a byte to pmic chip
+ * vib: pointer to vibrator structure
+ * data: data to write
+ * reg: register address
+ *
+ */
+static int pmic8058_vib_write_u8(struct pmic8058_vib *vib,
+				 u8 data, u16 reg)
+{
+	int rc;
+
+	rc = pm8058_write(vib->pm_chip, reg, &data, 1);
+	if (rc < 0)
+		dev_warn(vib->dev, "Error writing pmic8058 reg 0x%x(0x%x)\n",
+				reg, rc);
+	return rc;
+}
+
+/*
+ * pmic8058_vib_set - handler to start/stop vibration
+ * vib: pointer to vibrator structure
+ * on: state to set
+ *
+ */
+static int pmic8058_vib_set(struct pmic8058_vib *vib, int on)
+{
+	int rc;
+	u8 val;
+
+	val = vib->reg_vib_drv;
+
+	if (on)
+		val |= ((vib->level << VIB_DRV_SEL_SHIFT) & VIB_DRV_SEL_MASK);
+	else
+		val &= ~VIB_DRV_SEL_MASK;
+
+	rc = pmic8058_vib_write_u8(vib, val, VIB_DRV);
+	if (rc < 0)
+		return rc;
+
+	vib->reg_vib_drv = val;
+	return rc;
+}
+
+/*
+ * pmic8058_work_handler - worker to set vibration level
+ * work: pointer to work_struct
+ *
+ */
+static void pmic8058_work_handler(struct work_struct *work)
+{
+	struct pmic8058_vib *vib;
+	int rc;
+	u8 val;
+
+	vib  = container_of(work, struct pmic8058_vib, work);
+
+	rc = pmic8058_vib_read_u8(vib, &val, VIB_DRV);
+	if (rc < 0)
+		return;
+
+	/*
+	 * pmic vibrator supports voltage ranges from 1.2 to 3.1V, so
+	 * scale the level to fit into these ranges.
+	 */
+	if (vib->speed) {
+		vib->state = 1;
+		vib->level = ((VIB_MAX_LEVELS * vib->speed) / MAX_FF_SPEED) +
+						VIB_MIN_LEVEL_mV;
+		vib->level /= 100;
+	} else {
+		vib->state = 0;
+		vib->level = VIB_MIN_LEVEL_mV / 100;
+	}
+	pmic8058_vib_set(vib, vib->state);
+}
+
+/*
+ * pmic8058_vib_play_effect - function to handle vib effects.
+ * dev: input device pointer
+ * data: data of effect
+ * effect: effect to play
+ *
+ * Currently this driver supports only rumble effects.
+ *
+ */
+static int pmic8058_vib_play_effect(struct input_dev *dev, void *data,
+		      struct ff_effect *effect)
+{
+	struct pmic8058_vib *vib = input_get_drvdata(dev);
+
+	vib->speed = effect->u.rumble.strong_magnitude >> 8;
+	if (!vib->speed)
+		vib->speed = effect->u.rumble.weak_magnitude >> 9;
+	schedule_work(&vib->work);
+	return 0;
+}
+
+static int __devinit pmic8058_vib_probe(struct platform_device *pdev)
+
+{
+	struct pm8058_chip *pm_chip;
+	struct pmic8058_vib *vib;
+	const struct pmic8058_vibrator_pdata *pdata = pdev->dev.platform_data;
+	int rc;
+	u8 val;
+
+	pm_chip = platform_get_drvdata(pdev);

The parent should not abuse platform data of _this_ device, it belongs
to this driver. In fact you overwrite it with pmic8058_vib below, which
means that you can't unbind the driver and bind it again.

Please find other way to pass pm_chip.

+	if (pm_chip == NULL) {
+		dev_err(&pdev->dev, "no parent data passed in\n");
+		return -EINVAL;
+	}
+
+	if (!pdata)
+		return -EINVAL;
+
+	if (pdata->level_mV < VIB_MIN_LEVEL_mV ||
+			 pdata->level_mV > VIB_MAX_LEVEL_mV)
+		return -EINVAL;
+
+	vib = kzalloc(sizeof(*vib), GFP_KERNEL);
+	if (!vib)
+		return -ENOMEM;
+
+	vib->pm_chip	= pm_chip;
+	vib->pdata	= pdata;
+	vib->dev	= &pdev->dev;
+
+	INIT_WORK(&vib->work, pmic8058_work_handler);
+
+	vib->vib_input_dev = input_allocate_device();
+
+	if (vib->vib_input_dev == NULL) {
+		dev_err(&pdev->dev, "couldn't allocate input device\n");
+		rc = -ENOMEM;
+		goto err_alloc_dev;
+	}
+
+	input_set_drvdata(vib->vib_input_dev, vib);
+
+	vib->vib_input_dev->name = "pmic8058_vibrator";
+	vib->vib_input_dev->id.version = 1;
+	vib->vib_input_dev->dev.parent = &pdev->dev;
+
+	/* operate in manual mode */
+	rc = pmic8058_vib_read_u8(vib, &val, VIB_DRV);
+	if (rc < 0)
+		goto err_read_vib;
+	val &= ~VIB_DRV_EN_MANUAL_MASK;
+	rc = pmic8058_vib_write_u8(vib, val, VIB_DRV);
+	if (rc < 0)
+		goto err_read_vib;
+
+	vib->reg_vib_drv = val;
+
+	input_set_capability(vib->vib_input_dev, EV_FF, FF_RUMBLE);
+
+	rc = input_ff_create_memless(vib->vib_input_dev, NULL,
+					pmic8058_vib_play_effect);
+	if (rc < 0) {
+		dev_dbg(&pdev->dev, "couldn't register vibrator to FF\n");
+		goto err_create_memless;
+	}
+
+	platform_set_drvdata(pdev, vib);
+
+	rc = input_register_device(vib->vib_input_dev);
+	if (rc < 0) {
+		dev_dbg(&pdev->dev, "couldn't register input device\n");
+		goto err_reg_input_dev;
+	}

platform_set_drvdata(pdev, vib) should go here so you do not need to
clean it if input_register_device() fails.

+	return 0;
+
+err_reg_input_dev:
+	input_ff_destroy(vib->vib_input_dev);
+err_create_memless:
+err_read_vib:
+	input_free_device(vib->vib_input_dev);
+err_alloc_dev:
+	kfree(vib);
+	return rc;
+}
+
+static int __devexit pmic8058_vib_remove(struct platform_device *pdev)
+{
+	struct pmic8058_vib *vib = platform_get_drvdata(pdev);
+
+	cancel_work_sync(&vib->work);
+	if (vib->state)
+		pmic8058_vib_set(vib, 0);

This should probably be part of pmic8058_vib_close() method.

+
+	input_unregister_device(vib->vib_input_dev);
+	kfree(vib);

Need to call platform_set_drvdata(pdev, NULL);

+	return 0;
+}
+
+static struct platform_driver pmic8058_vib_driver = {
+	.probe		= pmic8058_vib_probe,
+	.remove		= __devexit_p(pmic8058_vib_remove),
+	.driver		= {
+		.name	= "pm8058-vib",
+		.owner	= THIS_MODULE,
+	},
+};

What about PM? Do you need to shut off vibration if device goes to sleep?

Thanks.

-- 
Dmitry