Re: [PATCH] Haptic class support (v2)

[Kyungmin Park]

Hi,

On Wed, Oct 7, 2009 at 3:35 AM, Trilok Soni <soni.trilok@xxxxxxxxx> wrote:
> Hi Kyungmin,
>
> On Tue, Oct 6, 2009 at 1:15 PM, Kyungmin Park <kmpark@xxxxxxxxxxxxx> wrote:
>> This patch includes two haptic devices, isa1000 and isa1200
>> ISA1000 is gpio based haptic, but isa1200 is based on I2C
>> Both are working on Samsung SoCs and tested.
>>
>> To enable the haptic, echo 1 > /sys/class/haptic/${name}/enable
>> You can adjust the level by echo ${level} > /sys/class/haptic/${name}/enable
>> or
>> With oneshot feature, echo ${msec time} > /sys/class/haptic/${name}/oneshot
>>
>> Signed-off-by: Kyungmin Park <kyungmin.park@xxxxxxxxxxx>
>
> Many thanks for the posting the update and example drivers. Here are
> my suggestions to make it mainlined fast:
>
> 1. split these patches - probably in following order would be good
>   a) haptics class
>   b) haptics samsung pwm driver
>   c) isa1200 haptics driver
>   d) add maintainers entry in MAINTAINERS file under your name for haptics :)
>   d) please add short documentation under Documentation/haptics ?
> directory describe what haptics class is all about, and atleast
> userspace interfaces which you are providing using sysfs file. We can
> use led class documentation as example here. Short documentation is
> fine to begin with.

Thank you for suggestion. I will split the patches. and resend.

>   e) if possible create your own git haptics project tree at
> kernel.org and add it to linux-next, but I would also suggest that as
> no. of updates to these framework will be not so frequent, so we can
> also follow -mm route and let Andrew pick up your patches and get them
> cooked under -mm.

I think no need to create new haptic tree. it's simple class to handle
haptic drivers.

>
>>
>> diff --git a/drivers/Kconfig b/drivers/Kconfig
>> index 48bbdbe..d44a601 100644
>> --- a/drivers/Kconfig
>> +++ b/drivers/Kconfig
>> @@ -62,6 +62,8 @@ source "drivers/power/Kconfig"
>>
>>  source "drivers/hwmon/Kconfig"
>>
>> +source "drivers/haptic/Kconfig"
>> +
>>  source "drivers/thermal/Kconfig"
>>
>>  source "drivers/watchdog/Kconfig"
>> diff --git a/drivers/Makefile b/drivers/Makefile
>> index 6ee53c7..16b8f67 100644
>> --- a/drivers/Makefile
>> +++ b/drivers/Makefile
>> @@ -77,6 +77,7 @@ obj-$(CONFIG_PPS)             += pps/
>>  obj-$(CONFIG_W1)               += w1/
>>  obj-$(CONFIG_POWER_SUPPLY)     += power/
>>  obj-$(CONFIG_HWMON)            += hwmon/
>> +obj-$(CONFIG_HAPTIC)           += haptic/
>>  obj-$(CONFIG_THERMAL)          += thermal/
>>  obj-$(CONFIG_WATCHDOG)         += watchdog/
>>  obj-$(CONFIG_PHONE)            += telephony/
>> diff --git a/drivers/haptic/Kconfig b/drivers/haptic/Kconfig
>> new file mode 100644
>> index 0000000..9acb02a
>> --- /dev/null
>> +++ b/drivers/haptic/Kconfig
>> @@ -0,0 +1,31 @@
>> +menuconfig HAPTIC
>> +       bool "HAPTIC support"
>> +       help
>> +         Say Y to enalbe haptic support. It enables the haptic and controlled
>
> s/enalbe/enable

Fixed.

>
>> +         from both userspace and kernel
>
> Please explain how from kernel? We are only providing sysfs interface
> for this framework, and I don't see in-kernel control of haptics
> driver in your patch. Also I don't see any need of in-kernel control
> of them though.

It's wrong description. It's handled from userspace not kernel.

>
>> +
>> +if HAPTIC
>> +
>> +config HAPTIC_CLASS
>> +       tristate "Haptic Class Support"
>> +       help
>> +         This option enables the haptic sysfs class in /sys/class/haptic.
>> +
>> +comment "Haptic drivers"
>> +
>> +config HAPTIC_SAMSUNG_PWM
>> +       tristate "Haptic Support for SAMSUNG PWM-controlled motor (ISA1000)"
>> +       depends on HAPTIC_CLASS && (ARCH_S3C64XX || ARCH_S5PC1XX)
>> +       help
>> +         This options enables support for haptic connected to GPIO lines
>> +         controlled by a PWM timer on SAMSUNG CPUs.
>> +
>> +comment "Haptic chips"
>> +
>> +config HAPTIC_ISA1200
>> +       tristate "Haptic Motor"
>> +       depends on HAPTIC_CLASS && I2C
>> +       help
>> +         The ISA1200 is a high performance enhanced haptic motor driver
>
> This chip also seems to support external pwm mode like isa1000, so you
> could explicitly add that this driver is about controlling it through
> i2c only, but there could be a possibility of writing the same through
> external pwm mode also.

Actually it supports both internal and external. but in my hardware it
connects with external.
So I implement it as previous isa1000 does.
Someone can implement it as internal.

>
>> +
>> +static int samsung_pwm_haptic_probe(struct platform_device *pdev)
>> +{
>
> __devinit?

Fixed.

>
>> +       struct haptic_platform_data *pdata = pdev->dev.platform_data;
>> +       struct samsung_pwm_haptic *haptic;
>> +       int ret;
>> +
>> +       haptic = kzalloc(sizeof(struct samsung_pwm_haptic), GFP_KERNEL);
>> +       if (!haptic) {
>> +               dev_err(&pdev->dev, "No memory for device\n");
>> +               return -ENOMEM;
>> +       }
>> +static int samsung_pwm_haptic_remove(struct platform_device *pdev)
>> +{
>
> __devexit

Fixed

>
>> +
>> +static int __devexit isa1200_remove(struct i2c_client *client)
>> +{
>> +       return 0;
>
> nothing to remove?

Sorry I'm not yet implemented. Will fix it.

>
>> +}
>> +
>> +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;
>> +}
>
> #ifdef CONFIG_PM checks please.

Of course.

Thank you,
Kyungmin Park
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/