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

Hi Dmitry,

On 2/2/2011 2:03 PM, Dmitry Torokhov wrote:

Hi Anirudh,

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

quoted

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

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

Looks like someone forgot "git add" on it. We will fix this in v3.

quoted

+
+#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 '@').

Ack. It will be fixed.

quoted

+
+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.

This will be fixed through pmic8058 core driver, when we submit. We are aware of it,
and the all the sub-device driver will be changed to do it properly once we release
them again with pmic core driver submission.

quoted

+
+	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.

Ack

quoted

+
+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.

Ok. We will check and fix.

quoted

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

Need to call platform_set_drvdata(pdev, NULL);

Ack.

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

Yes. Let me check and add it to v3 patch series.

I will drop the PMIC8058 OTHC from v3 series, as Mark suggested to explore ASoC way of 
doing it.

---Trilok Soni

-- 
Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.