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.
> > +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.
> 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.
> > +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.
> 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).
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@xxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
