Re: [PATCH v6 3/3] input: samsung-keypad - Add samsung keypad driver

[Joonyoung Shim <jy0922.shim@samsung.com>]

On 7/21/2010 5:07 PM, Dmitry Torokhov wrote:
> Hi Joonyoung,
> 
> On Wed, Jul 21, 2010 at 02:42:22PM +0900, Joonyoung Shim wrote:
>> Hi, Dmitry.
>>
>> On 7/9/2010 4:39 PM, Joonyoung Shim wrote:
>>> On 7/9/2010 4:07 PM, Kukjin Kim wrote:
>>>> Joonyoung Shim wrote:
>>>>> On 7/7/2010 8:52 AM, Kukjin Kim wrote:
>>>>>> Joonyoung Shim wrote:
>>>>>>> Hi, Naveen.
>>>>>>>
>>>>>>> On 6/28/2010 8:33 PM, Joonyoung Shim wrote:
>>>>>>>> This patch adds support for keypad driver running on Samsung cpus. This
>>>>>>>> driver is tested on GONI and Aquila board using S5PC110 cpu.
>>>>>>>>
>>>>>>>> Signed-off-by: Joonyoung Shim <jy0922.shim@samsung.com>
>>>>>>>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>>>>>>>> ---
>>>>>>>>  drivers/input/keyboard/Kconfig          |    9 +
>>>>>>>>  drivers/input/keyboard/Makefile         |    1 +
>>>>>>>>  drivers/input/keyboard/samsung-keypad.c |  491
>>>>>>> +++++++++++++++++++++++++++++++
>>>>>>>>  3 files changed, 501 insertions(+), 0 deletions(-)
>>>>>>>>  create mode 100644 drivers/input/keyboard/samsung-keypad.c
>>>>>>>>
>>>>>>> Could you please test this v6 keypad driver on your various targets?
>>>>>> Hi,
>>>>>>
>>>>>> I received the test result from Naveen.
>>>>>> And now he is debugging about that.
>>>>>>
>>>>>> ..his e-mail client has some problem..so I replied...
>>>>>>
>>>>>> ---
>>>>>> From: Naveen Ch
>>>>>>
>>>>>> I've tested with v6 patches and they are working fine on SMDK6410, SMDKC100
>>>>> and SMDKV210, except for some header inclusion (linux/sched.h) in samsung-
>>>>> keypad.c.
>>>>>> The build breaking error is like following:
>>>>>> (make s5pc100_defconfig and select input->keypad->samsung keypad)
>>>>>>
>>>>>> drivers/input/keyboard/samsung-keypad.c: In function 'samsung_keypad_irq':
>>>>>> drivers/input/keyboard/samsung-keypad.c:170: error: 'TASK_UNINTERRUPTIBLE'
>>>>> undeclared (first use in this function)
>>>>>> drivers/input/keyboard/samsung-keypad.c:170: error: (Each undeclared identifier
>>>>> is reported only once
>>>>>> drivers/input/keyboard/samsung-keypad.c:170: error: for each function it appears
>>>>> in.)
>>>>>> drivers/input/keyboard/samsung-keypad.c:170: error: implicit declaration of
>>>>> function 'schedule_timeout'
>>>>>> drivers/input/keyboard/samsung-keypad.c: In function 'samsung_keypad_stop':
>>>>>> drivers/input/keyboard/samsung-keypad.c:202: error: 'TASK_NORMAL'
>>>>> undeclared (first use in this function)
>>>>>> make[3]: *** [drivers/input/keyboard/samsung-keypad.o] Error 1
>>>>>>
>>>>> This error occurs on s5pc100_defconfig having disabled CONFIG_PREEMPT
>>>>> option. I think sched.h should be included to remove like this
>>>>> dependency in driver, are there other ways?
>>>> I'm not sure root cause is that. But as you said, I think need inclusion it for prevent the problem.
>>>> Actually didn't happen it before..so maybe the some core functions have moved between rc-3 and earlier versions...
>>> The sched.h is included by below path.
>>>
>>> interrupt.h -> hardirq.h -> smp_lock.h -> sched.h
>>>
>>> The smp_lock.h in hardirq.h is included if CONFIG_PREEMPT is defined. 
>>> The samsung keypad driver haven't needed including sched.h before
>>> Dmitry's patch is applied..
>>>
>>>> Anyway, if no any issues, could you please re-submit updated it soon?
>>> I haven't get yet Dmitry's review of v6 patch set, so i am waiting it.
>>>
>> Ping.
>>
>> Do you have any feedback? If ok, i want to go to input tree or samsung
>> tree these added sched.h including.
> 
> I applied the patch adding samsung-keypad.c and the platform data
> arch/arm/plat-samsung/include/plat/keypad.h and I expect that
> patches to actually enable keypad will go through appropriate
> platform tree(s).
> 

OK, the rest will go through samsung platform tree

> The only caveat is that I removed declarations for copying platform data
> around and for gpio config function - if it is there there is no point
> of invoking it through the pointer in platform data.
> 

I agree, but currently samsung platform framework uses such this way. I
believe these can change later at once.

> Overall I am confused why you set up keypad in platform code in the
> way you do. I'd put the device definitions and all supporting functions
> in the same module that registers platform device and be done with it.
> 

Thanks.