Hi Kyungmin, On Thu, Oct 8, 2009 at 3:14 PM, Trilok Soni <soni.trilok@xxxxxxxxx> wrote: > Hi Kyungmin, > > Adding linux-i2c as it is i2c client driver and Ben Dooks for samsung > pwm driver API usage. > > On Wed, Oct 7, 2009 at 11:48 AM, Kyungmin Park > <kyungmin.park@xxxxxxxxxxx> wrote: >> I2C based ISA1200 haptic driver support. >> This chip supports both internal and external. >> But only external configuration are implemented. > > Probably following text would look better: > > Add Imagis ISA1200 haptics chip driver support. This chip supports > internal and external PWM configuration. This patch only supports > external PWM configuration. > >> new file mode 100644 >> index 0000000..51c4ea4 >> --- /dev/null >> +++ b/drivers/haptic/isa1200.c >> @@ -0,0 +1,429 @@ >> +/* >> + * isa1200.c - Haptic Motor >> + * >> + * Copyright (C) 2009 Samsung Electronics >> + * Kyungmin Park <kyungmin.park@xxxxxxxxxxx> >> + * >> + * 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. >> + */ >> + >> +#include <linux/module.h> >> +#include <linux/init.h> >> +#include <linux/i2c.h> >> +#include <linux/gpio.h> >> +#include <linux/haptic.h> >> +#include <linux/delay.h> >> +#include <linux/err.h> >> +#include <linux/pwm.h> >> +#include <linux/ctype.h> >> +#include <linux/workqueue.h> >> +#include <linux/i2c/isa1200.h> >> +#include "haptic.h" >> + >> +struct isa1200_chip { >> + struct i2c_client *client; >> + struct pwm_device *pwm; >> + struct haptic_classdev cdev; >> + struct work_struct work; >> + struct timer_list timer; >> + >> + unsigned int len; /* LDO enable */ >> + unsigned int hen; /* Haptic enable */ >> + >> + int enable; >> + int powered; >> + >> + int level; >> + int level_max; >> + >> + int ldo_level; >> +}; >> + >> + >> +static void isa1200_chip_power_on(struct isa1200_chip *haptic) >> +{ >> + if (haptic->powered) >> + return; >> + haptic->powered = 1; >> + /* Use smart mode enable control */ > > You mean here that, enabling smart mode control by writing ISA1200 > register over I2C will be added later, right? > >> + >> +static void isa1200_setup(struct i2c_client *client) >> +{ >> + struct isa1200_chip *chip = i2c_get_clientdata(client); >> + int value; >> + >> + gpio_set_value(chip->len, 1); >> + udelay(250); >> + gpio_set_value(chip->len, 1); >> + > > Please clarify: > > In your hardware configuration, you are enabling internal LDO above? > It may not be true for all the hardware configuration and it might be > grounded. If true, please make this as platform data so that it can be > selected run time. > >> + value = isa1200_read_reg(client, ISA1200_SCTRL0); >> + value &= ~ISA1200_LDOADJ_MASK; >> + value |= chip->ldo_level; >> + isa1200_write_reg(client, ISA1200_SCTRL0, value); >> + >> + value = ISA1200_HAPDREN | ISA1200_OVERHL | ISA1200_HAPDIGMOD_PWM_IN | >> + ISA1200_PWMMOD_DIVIDER_128; >> + isa1200_write_reg(client, ISA1200_HCTRL0, value); >> + >> + value = ISA1200_EXTCLKSEL | ISA1200_BIT6_ON | ISA1200_MOTTYP_LRA | > > Probably motor type could be different too. Please see what other > parameters you could become as platform data for this chip and can be > tuned by h/w designer for the product design. > >> + ISA1200_SMARTEN | ISA1200_SMARTOFFT_64; > > Oh. You are enabling smart control here. > >> + isa1200_write_reg(client, ISA1200_HCTRL1, value); >> + >> + value = isa1200_read_reg(client, ISA1200_HCTRL2); >> + value |= ISA1200_SEEN; >> + isa1200_write_reg(client, ISA1200_HCTRL2, value); >> + isa1200_chip_power_off(chip); >> + isa1200_chip_power_on(chip); >> + >> + /* TODO */ > > ?? some todo text? > >> +} >> + >> +static int __devinit isa1200_probe(struct i2c_client *client, >> + const struct i2c_device_id *id) >> +{ >> + struct isa1200_chip *chip; >> + struct haptic_platform_data *pdata; >> + int ret; >> + > > You need to add i2c_check_functionality call with smbus_byte_data. > >> + pdata = client->dev.platform_data; >> + if (!pdata) { >> + dev_err(&client->dev, "%s: no platform data\n", __func__); >> + return -EINVAL; >> + } >> + >> + chip = kzalloc(sizeof(struct isa1200_chip), GFP_KERNEL); >> + if (!chip) >> + return -ENOMEM; >> + >> + chip->client = client; >> + chip->cdev.set = isa1200_chip_set; >> + chip->cdev.get = isa1200_chip_get; >> + chip->cdev.show_enable = isa1200_chip_show_enable; >> + chip->cdev.store_enable = isa1200_chip_store_enable; >> + chip->cdev.store_oneshot = isa1200_chip_store_oneshot; >> + chip->cdev.show_level = isa1200_chip_show_level; >> + chip->cdev.store_level = isa1200_chip_store_level; >> + chip->cdev.show_level_max = isa1200_chip_show_level_max; >> + chip->cdev.name = pdata->name; >> + chip->enable = 0; >> + chip->level = PWM_HAPTIC_DEFAULT_LEVEL; >> + chip->level_max = PWM_HAPTIC_DEFAULT_LEVEL; >> + chip->ldo_level = pdata->ldo_level; >> + >> + if (pdata->setup_pin) >> + pdata->setup_pin(); >> + chip->len = pdata->gpio; >> + chip->hen = pdata->gpio; >> + chip->pwm = pwm_request(pdata->pwm_timer, "haptic"); >> + if (IS_ERR(chip->pwm)) { >> + dev_err(&client->dev, "unable to request PWM for haptic.\n"); >> + ret = PTR_ERR(chip->pwm); >> + goto error_pwm; >> + } >> + >> + INIT_WORK(&chip->work, isa1200_chip_work); >> + >> + /* register our new haptic device */ >> + ret = haptic_classdev_register(&client->dev, &chip->cdev); >> + if (ret < 0) { >> + dev_err(&client->dev, "haptic_classdev_register failed\n"); >> + goto error_classdev; >> + } >> + >> + ret = sysfs_create_group(&chip->cdev.dev->kobj, &haptic_group); >> + if (ret) >> + goto error_enable; > > Why the user of haptics class has to call this? I assume that once the > user of haptics class registers with it, the class itself should > create the necessary sysfs files and user driver doesn't have to worry > about it except providing necessary hooks. > >> + >> + init_timer(&chip->timer); >> + chip->timer.data = (unsigned long)chip; >> + chip->timer.function = &isa1200_chip_timer; > > like to use setup_timer? > >> + >> + i2c_set_clientdata(client, chip); >> + >> + if (gpio_is_valid(pdata->gpio)) { >> + ret = gpio_request(pdata->gpio, "haptic enable"); >> + if (ret) >> + goto error_gpio; >> + gpio_direction_output(pdata->gpio, 1); >> + } >> + >> + isa1200_setup(client); >> + >> + printk(KERN_INFO "isa1200 %s registered\n", pdata->name); >> + return 0; >> + >> +error_gpio: >> + gpio_free(pdata->gpio); >> +error_enable: >> + sysfs_remove_group(&chip->cdev.dev->kobj, &haptic_group); >> +error_classdev: >> + haptic_classdev_unregister(&chip->cdev); >> +error_pwm: >> + pwm_free(chip->pwm); >> + kfree(chip); >> + return ret; >> +} >> + >> +static int __devexit isa1200_remove(struct i2c_client *client) >> +{ >> + struct isa1200_chip *chip = i2c_get_clientdata(client); >> + >> + if (gpio_is_valid(chip->len)) >> + gpio_free(chip->len); >> + >> + sysfs_remove_group(&chip->cdev.dev->kobj, &haptic_group); >> + haptic_classdev_unregister(&chip->cdev); > > Where is del_timer_sync and cancel_work ? > >> + pwm_free(chip->pwm); >> + kfree(chip); >> + return 0; >> +} >> + >> +#ifdef CONFIG_PM >> +static int isa1200_suspend(struct i2c_client *client, pm_message_t mesg) >> +{ >> + struct isa1200_chip *chip = i2c_get_clientdata(client); >> + isa1200_chip_power_off(chip); >> + return 0; >> +} >> + >> +static int isa1200_resume(struct i2c_client *client) >> +{ >> + isa1200_setup(client); >> + return 0; >> +} >> +#else >> +#define isa1200_suspend NULL >> +#define isa1200_resume NULL >> +#endif >> + >> +static const struct i2c_device_id isa1200_id[] = { >> + { "isa1200", 0 }, For haptics use-case it is very natural to have multiple instance of these chips to driver different say top-bottom or right-and-left mounted motors to create various patterns. I hope haptics class and this isa1200 is safe for such usage including sysfs naming conventions. -- ---Trilok Soni http://triloksoni.wordpress.com http://www.linkedin.com/in/triloksoni -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html