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

[Trilok Soni <tsoni@codeaurora.org>]

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

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

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

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

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

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