| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <5d5443650910121156x37ba6cd7w230bda9aef4df55@mail.gmail.com>
Date: Tue, 13 Oct 2009 00:26:53 +0530
From: Trilok Soni <soni.trilok@...il.com>
To: Kyungmin Park <kyungmin.park@...sung.com>
Cc: linux-kernel@...r.kernel.org, linux-input@...r.kernel.org,
linux-i2c@...r.kernel.org, ben-linux@...ff.org
Subject: Re: [PATCH 6/6] haptic: ISA1200 haptic device support
Hi Kyungmin,
On Thu, Oct 8, 2009 at 3:14 PM, Trilok Soni <soni.trilok@...il.com> 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@...sung.com> 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@...sung.com>
>> + *
>> + * 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-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists