Re: [PATCH 03/03] [INPUT][KEYBOARD] Add new keypad driver for s3c series SoCs

[Kyungmin Park <kmpark@infradead.org>]

2009/9/7 Harald Welte <laforge@gnumonks.org>:
> Dear all,
>
> On Mon, Sep 07, 2009 at 12:25:14PM +0900, Kyungmin Park wrote:
>> > +static const unsigned char s3c_keycode[] = {
>> > +       1, 2, KEY_1, KEY_Q, KEY_A, 6, 7, KEY_LEFT,
>> > +       9, 10, KEY_2, KEY_W, KEY_S, KEY_Z, KEY_RIGHT, 16,
>> > +       17, 18, KEY_3, KEY_E, KEY_D, KEY_X, 23, KEY_UP,
>> > +       25, 26, KEY_4, KEY_R, KEY_F, KEY_C, 31, 32,
>> > +       33, KEY_O, KEY_5, KEY_T, KEY_G, KEY_V, KEY_DOWN, KEY_BACKSPACE,
>> > +       KEY_P, KEY_0, KEY_6, KEY_Y, KEY_H, KEY_SPACE, 47, 48,
>> > +       KEY_M, KEY_L, KEY_7, KEY_U, KEY_J, KEY_N, 55, KEY_ENTER,
>> > +       KEY_LEFTSHIFT, KEY_9, KEY_8, KEY_I, KEY_K, KEY_B, 63, KEY_COMMA,
>> > +};
>>
>> Why do you use fixed keycode? Dose it provided from board specific
>> platform? and If we want to use only 3 * 3 keypad then how do you
>> handle this one?
>
> As Jinsung mailed, the keymap can always be reconfigured from userspace.
>
> Whether or not the board specific code wants to set a different default keymap
> is probably a matter of taste.  omap-kbd has it, pxa27x_kbd also has it.  So
> yes, there is some reason to align with what they are doing.

Right it's a matter of taste. but I can't see the provided default key
maps. these got the keymap from platform instead of driver itself.

Even though userspace can re-configure it. why give a burden to the
application programmer. In most case board got fixed keymap and
changed based on some scenarios.

>
>> > +struct s3c_keypad {
>> > +       struct s3c_platform_keypad *pdata;
>> > +       unsigned char keycodes[ARRAY_SIZE(s3c_keycode)];
>>
>> We don't need full keycode array here. It should be allocated with
>> required size.
>
> I think this is really an implementation detail up to the author.  Having
> a static array with the maximum size (8*8 == 64) is not a big waste of
> memory, if you can on the other hand save some code complexity for dynamic
> kmalloc()/kfree() and the associated error paths.

With this approaches, it will be increased at s5pc110 since it
provides 14 * 8 matrix. whatever I only used 3 * 3.

>
>> Why do you use timer. As other input drivers how about to use workqueue?
>
> Sorry, what exactly do you mean?  I see many (if not all) other keypad drivers
> also using a timer.

Right it's my taste. In my experience timer method is hide some bug at
the omap keypad drivers development.

>
>> > +static int s3c_keypad_open(struct input_dev *dev)
>> > +{
>> > +       struct s3c_keypad *keypad = input_get_drvdata(dev);
>> > +       u32 cfg;
>> > +
>> > +       clk_enable(keypad->clk);
>> > +
>> > +       /* init keypad h/w block */
>> > +       cfg = readl(keypad->regs + S3C_KEYIFCON);
>> > +       cfg &= ~S3C_KEYIF_CON_MASK_ALL;
>> > +       cfg |= (S3C_KEYIF_INT_F_EN | S3C_KEYIF_INT_R_EN |
>> > +                       S3C_KEYIF_DF_EN | S3C_KEYIF_FC_EN);
>> > +       writel(cfg, keypad->regs + S3C_KEYIFCON);
>> > +
>> > +       cfg = readl(keypad->regs + S3C_KEYIFFC);
>> > +       cfg |= 0x1;
>>
>> What's the meaning of '0x1'?
>
> I agree. Jinsung: The 0x1 should not be a number but a #define from
> the register definition.
>
>> > +       if (keypad->timer_enabled) {
>> > +               mod_timer(&keypad->timer, keypad->timer.expires);
>> > +       } else {
>>
>> useless curly braces
>
> That's what I said, too.  However, the kernel CodingStyle document explicitly
> states that if there is an 'else' block with multiple lines, the first block
> should also have curly braces.   Despite this, I have not seen it much in use.
>
>> > +       keypad->pdata = pdata;
>> > +       memcpy(keypad->keycodes, s3c_keycode, sizeof(keypad->keycodes));
>>
>> memcpy??? if you don't modify s3c_keycode. just assign it.
>
> keypad->keycodes is a pointer to the keycode array in the s3c private data
> structure.  This memory can be changed from userspace by using the keypad
> set/get ioctl's (e.g. 'loadkeys' command).
>
> s3c_keycode is a const array of the default keymap that is loaded during boot.
> right now there is only one 6410 specific default keymap, but as you suggested
> there should probably be a platform_data (board specific) default keymap.
>
> In any case, I personally believe the memcpy to memory that is owned by
> the driver is a good idea, since userspace can modify it.
>
>> Finally, We did duplicated works. We already implement the keypad driver for
>> s5pc100 & s5pc110. but we use workqueue structure instead of timer.
>
> This is why System LSI and DMC Lab should coordinate more on what they work.
>
> System LSI's kernel tree is on git.kernel.org and updated every night, so you
> (or other interested parties) can stay up-to-date with what is happening.  If
> you base your work on top of System LSI's tree, you would always follow their
> work.  System LSI's tree is what is scheduled to be submitted to mainline.

Even though it's working at Samsung SoCs. but it's not mean it fits
the mainline kernel.
It's based on SMDK board and focused on SMDK.
I mean it's not generic. Some codes are hard-coded and don't call
proper frameworks APIs. especially clocks

>
>> I will post the our works.
>
> I'm not sure if two different Samsung departments posting competing drivers to
> the mainline mailing lists will really help.  You need to define which group
> will be the 'master' inside Samsung (the logical choice would be System LSI).

It's simple. give a choice to customers if there are two versions. you
think it's some duplicated works
it's better to others.

I don't hope just push their codes to mainline with these discussion.

Thank you,
Kyungmin Park

>
> You should then submit your code / modifications as patches to that master
> tree, and System LSI is submitting it mainline.
>
> The same should be the case for other departments who work on Samsung SoC
> related kernel code..
>
> Regards,
> --
> - Harald Welte <laforge@gnumonks.org>           http://laforge.gnumonks.org/
> ============================================================================
> "Privacy in residential applications is a desirable marketing option."
>                                                  (ETSI EN 300 175-7 Ch. A6)
>
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html