lkml.org 
[lkml]   [2011]   [Feb]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [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


    \
     
     \ /
      Last update: 2011-02-02 09:35    [W:0.042 / U:152.644 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site