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

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

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).

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.

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.

-- 
Dmitry