RE: [PATCH v3] input: tegra-kbc - Add tegra keyboard driver

[Rakesh Iyer]

> On Fri, Jan 07, 2011 at 09:45:07AM -0800, riyer@xxxxxxxxxx wrote:
> > +
> > +struct tegra_kbc {
> > +	void __iomem *mmio;
> > +	struct input_dev *idev;
> > +	int irq;
> > +	unsigned int wake_enable_rows;
> > +	unsigned int wake_enable_cols;
> > +	spinlock_t lock;
> > +	unsigned int repoll_time;
> > +	unsigned long cp_dly_jiffies;
> > +	int fifo[KBC_MAX_KPENT];
> > +	const struct tegra_kbc_platform_data *pdata;
> > +	int *plain_keycode;
> > +	int *fn_keycode;
> 
> There should not be separate keycodes for FN and normal kys - FN is just
> a modifier, like SHIFT or CTRL or ALT are and shoudl be handled in upper
> layers.

We had the same thought regarding Function as a modifier akin to Shift, Ctrl an Alt.
But this does not appear to be the case. 

This keyboard is used primarily for Laptop-like form factors where function keys are used to overload the existing keys. 
In addition the Fn key mappings aren't identical in different keyboard layouts.

So at this point in order to function with the target hardware and the target operating system which is Chrome, this modifier keymap is necessary.

Let me know what you think.

> 
> Also you should wire up keycode/keycodemax/keycodesize in input_dev
> structuire so that keymap can be retrieved via EVIOCGKEYCODE and
> modified via EVIOGSKEYCODE.

I will proceed to do this.

>  Also, because keymap is modifiable, the
> original keymap should be copied in per-device structure, leaving
> original intact. It (the original) also should be marked as const.
I did not get the significance to preserving a copy of the original keymap.
In what situation would we use that.


> Since this papears to be a matrix keypad consider using definitions from
> linux/input/matrix_keypad.h
>

I looked at the linux/input/matrix_keypad.h and noticed it shares a decent amount of structure with the tegra keyboard.
But the tegra kbc hardware differences from the matrix_keypad are significant enough to warrant its own structure.

Regards
Rakesh

> -----Original Message-----
> From: Dmitry Torokhov [mailto:dmitry.torokhov@xxxxxxxxx]
> Sent: Monday, January 10, 2011 2:16 PM
> To: Rakesh Iyer
> Cc: jj@xxxxxxxxxxxxx; tsoni@xxxxxxxxxxxxxx; shubhrajyoti@xxxxxx; ccross@xxxxxxxxxxx;
> konkers@xxxxxxxxxxx; olof@xxxxxxxxx; Andrew Chew; linux-tegra@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; linux-input@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH v3] input: tegra-kbc - Add tegra keyboard driver
> 
> Hi Rakesh,
> 
> On Fri, Jan 07, 2011 at 09:45:07AM -0800, riyer@xxxxxxxxxx wrote:
> > +
> > +struct tegra_kbc {
> > +	void __iomem *mmio;
> > +	struct input_dev *idev;
> > +	int irq;
> > +	unsigned int wake_enable_rows;
> > +	unsigned int wake_enable_cols;
> > +	spinlock_t lock;
> > +	unsigned int repoll_time;
> > +	unsigned long cp_dly_jiffies;
> > +	int fifo[KBC_MAX_KPENT];
> > +	const struct tegra_kbc_platform_data *pdata;
> > +	int *plain_keycode;
> > +	int *fn_keycode;
> 
> There should not be separate keycodes for FN and normal kys - FN is just
> a modifier, like SHIFT or CTRL or ALT are and shoudl be handled in upper
> layers.
> 
> Also you should wire up keycode/keycodemax/keycodesize in input_dev
> structuire so that keymap can be retrieved via EVIOCGKEYCODE and
> modified via EVIOGSKEYCODE. Also, because keymap is modifiable, the
> original keymap should be copied in per-device structure, leaving
> original intact. It (the original) also should be marked as const.
> 
> Since this papears to be a matrix keypad consider using definitions from
> linux/input/matrix_keypad.h
> 
> Thank you.
> 
> --
> Dmitry
--
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/