[PATCH v2 0/2] Input: tegra-kbc - configure through device tree

[PATCH v2 0/2] Input: tegra-kbc - configure through device tree

[Olof Johansson]

The following two patches add device-tree bindings and probing to tegra-kbc,
and they also provide a simplified way of specifying the, to date, only known
non-default keymap.

Fixes since v1: added debounce/repeat delay properties, setup of pin_cfg
and verified that it works as expected on seaboard. Actual device tree
update will be posted separately for inclusion through tegra tree.


-Olof

[PATCH v2 1/2] Input: tegra-kbc - add device tree bindings

[Olof Johansson]

This adds a simple device tree binding to the tegra keyboard controller.

Also, mark the default keymap as __devinitdata since it is not referenced
after boot, and shorten the name to not have to avoid line wraps.

Signed-off-by: Olof Johansson <olof@lixom.net>
---
 .../devicetree/bindings/input/tegra-kbc.txt        |   18 +++++
 drivers/input/keyboard/tegra-kbc.c                 |   70 ++++++++++++++++++--
 2 files changed, 83 insertions(+), 5 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/input/tegra-kbc.txt

diff --git a/Documentation/devicetree/bindings/input/tegra-kbc.txt b/Documentation/devicetree/bindings/input/tegra-kbc.txt
new file mode 100644
index 0000000..a7c1d40
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/tegra-kbc.txt
@@ -0,0 +1,18 @@
+* Tegra keyboard controller
+
+Required properties:
+- compatible : "nvidia,tegra20-kbc"
+
+Optional properties:
+- debounce-delay: delay in milliseconds per row scan for debouncing
+- repeat-delay: delay in milliseconds before repeat starts
+- ghost-filter : enable ghost filtering for this device
+- wakeup-source : configure keyboard as a wakeup source for suspend/resume
+
+Example:
+
+keyboard: keyboard {
+	compatible = "nvidia,tegra20-kbc";
+	reg = <0x7000e200 0x100>;
+	ghost-filter;
+};
diff --git a/drivers/input/keyboard/tegra-kbc.c b/drivers/input/keyboard/tegra-kbc.c
index cf3228b..4ce6cc8 100644
--- a/drivers/input/keyboard/tegra-kbc.c
+++ b/drivers/input/keyboard/tegra-kbc.c
@@ -26,6 +26,7 @@
 #include <linux/delay.h>
 #include <linux/io.h>
 #include <linux/interrupt.h>
+#include <linux/of.h>
 #include <linux/clk.h>
 #include <linux/slab.h>
 #include <mach/clk.h>
@@ -82,7 +83,7 @@ struct tegra_kbc {
 	struct clk *clk;
 };
 
-static const u32 tegra_kbc_default_keymap[] = {
+static const u32 default_keymap[] __devinitdata = {
 	KEY(0, 2, KEY_W),
 	KEY(0, 3, KEY_S),
 	KEY(0, 4, KEY_A),
@@ -217,9 +218,9 @@ static const u32 tegra_kbc_default_keymap[] = {
 	KEY(31, 4, KEY_HELP),
 };
 
-static const struct matrix_keymap_data tegra_kbc_default_keymap_data = {
-	.keymap		= tegra_kbc_default_keymap,
-	.keymap_size	= ARRAY_SIZE(tegra_kbc_default_keymap),
+static const struct matrix_keymap_data default_keymap_data __devinitdata = {
+	.keymap		= default_keymap,
+	.keymap_size	= ARRAY_SIZE(default_keymap),
 };
 
 static void tegra_kbc_report_released_keys(struct input_dev *input,
@@ -576,6 +577,55 @@ tegra_kbc_check_pin_cfg(const struct tegra_kbc_platform_data *pdata,
 	return true;
 }
 
+#ifdef CONFIG_OF
+static struct tegra_kbc_platform_data * __devinit tegra_kbc_dt_parse_pdata(
+	struct platform_device *pdev)
+{
+	struct tegra_kbc_platform_data *pdata;
+	struct device_node *np = pdev->dev.of_node;
+	int i;
+	u32 prop;
+
+	if (!np)
+		return NULL;
+
+	pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
+
+	if (!of_property_read_u32(np, "debounce-delay", &prop))
+		pdata->debounce_cnt = prop;
+
+	if (!of_property_read_u32(np, "repeat-delay", &prop))
+		pdata->repeat_cnt = prop;
+
+	if (of_find_property(np, "needs-ghost-filter", NULL))
+		pdata->use_ghost_filter = true;
+
+	if (of_find_property(np, "wakeup-source", NULL))
+		pdata->wakeup = true;
+
+	/* All currently known keymaps with device tree support use the same
+	 * pin_cfg, so set it up here.
+	 */
+	for (i = 0; i < KBC_MAX_ROW; i++) {
+		pdata->pin_cfg[i].num = i;
+		pdata->pin_cfg[i].is_row = true;
+	}
+	
+	for (i = 0; i < KBC_MAX_COL; i++) {
+		pdata->pin_cfg[KBC_MAX_ROW + i].num = i;
+		pdata->pin_cfg[KBC_MAX_ROW + i].is_row = false;
+	}
+
+	return pdata;
+}
+#else
+static inline struct tegra_kbc_platform_data *tegra_kbc_dt_parse_pdata(
+	struct platform_device *pdev)
+{
+	return NULL;
+}
+#endif
+
 static int __devinit tegra_kbc_probe(struct platform_device *pdev)
 {
 	const struct tegra_kbc_platform_data *pdata = pdev->dev.platform_data;
@@ -590,6 +640,9 @@ static int __devinit tegra_kbc_probe(struct platform_device *pdev)
 	unsigned int scan_time_rows;
 
 	if (!pdata)
+		pdata = tegra_kbc_dt_parse_pdata(pdev);
+
+	if (!pdata)
 		return -EINVAL;
 
 	if (!tegra_kbc_check_pin_cfg(pdata, &pdev->dev, &num_rows))
@@ -671,7 +724,7 @@ static int __devinit tegra_kbc_probe(struct platform_device *pdev)
 
 	kbc->use_fn_map = pdata->use_fn_map;
 	kbc->use_ghost_filter = pdata->use_ghost_filter;
-	keymap_data = pdata->keymap_data ?: &tegra_kbc_default_keymap_data;
+	keymap_data = pdata->keymap_data ?: &default_keymap_data;
 	matrix_keypad_build_keymap(keymap_data, KBC_ROW_SHIFT,
 				   input_dev->keycode, input_dev->keybit);
 
@@ -793,6 +846,12 @@ static int tegra_kbc_resume(struct device *dev)
 
 static SIMPLE_DEV_PM_OPS(tegra_kbc_pm_ops, tegra_kbc_suspend, tegra_kbc_resume);
 
+static const struct of_device_id tegra_kbc_of_match[] = {
+	{ .compatible = "nvidia,tegra20-kbc", },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, tegra_kbc_of_match);
+
 static struct platform_driver tegra_kbc_driver = {
 	.probe		= tegra_kbc_probe,
 	.remove		= __devexit_p(tegra_kbc_remove),
@@ -800,6 +859,7 @@ static struct platform_driver tegra_kbc_driver = {
 		.name	= "tegra-kbc",
 		.owner  = THIS_MODULE,
 		.pm	= &tegra_kbc_pm_ops,
+		.of_match_table = tegra_kbc_of_match,
 	},
 };
 
-- 
1.7.8.GIT

[PATCH v2 2/2] Input: tegra-kbc - add default chromeos map

[Olof Johansson]

This adds a simple way to specify the ChromeOS-specific keyboard map
instead of the "standard" map that is used on other Tegra devices such
as Harmony-with-keyboard and Seaboard.

I expect the number of different keyboard layouts to be quite limited,
and not many should be added over time. So instead of encoding the layout
in the device tree, with all the can of worms that entails w.r.t. agreeing
on a suitable binding, just add a property to specify that this is the
map to be used, and include it in the driver.

If, over time, the number of mappings increase, the binding can be
updated to include a custom map as a new property, without having to
worry about backwards compatibility on existing devices.

Signed-off-by: Olof Johansson <olof@lixom.net>
---
 .../devicetree/bindings/input/tegra-kbc.txt        |    3 +
 drivers/input/keyboard/tegra-kbc.c                 |  103 ++++++++++++++++++++
 2 files changed, 106 insertions(+), 0 deletions(-)

diff --git a/Documentation/devicetree/bindings/input/tegra-kbc.txt b/Documentation/devicetree/bindings/input/tegra-kbc.txt
index a7c1d40..c9ad6ea 100644
--- a/Documentation/devicetree/bindings/input/tegra-kbc.txt
+++ b/Documentation/devicetree/bindings/input/tegra-kbc.txt
@@ -8,6 +8,8 @@ Optional properties:
 - repeat-delay: delay in milliseconds before repeat starts
 - ghost-filter : enable ghost filtering for this device
 - wakeup-source : configure keyboard as a wakeup source for suspend/resume
+- chromeos-layout : use the standard ChromeOS layout instead of default PC-style
+  		    keyboard if this property is present.
 
 Example:
 
@@ -15,4 +17,5 @@ keyboard: keyboard {
 	compatible = "nvidia,tegra20-kbc";
 	reg = <0x7000e200 0x100>;
 	ghost-filter;
+	chromeos-layout;
 };
diff --git a/drivers/input/keyboard/tegra-kbc.c b/drivers/input/keyboard/tegra-kbc.c
index 4ce6cc8..0f0c5de 100644
--- a/drivers/input/keyboard/tegra-kbc.c
+++ b/drivers/input/keyboard/tegra-kbc.c
@@ -223,6 +223,106 @@ static const struct matrix_keymap_data default_keymap_data __devinitdata = {
 	.keymap_size	= ARRAY_SIZE(default_keymap),
 };
 
+static const u32 cros_kbd_keymap[] __devinitdata = {
+	KEY(0, 2, KEY_LEFTCTRL),
+	KEY(0, 4, KEY_RIGHTCTRL),
+
+	KEY(1, 0, KEY_LEFTMETA),
+	KEY(1, 1, KEY_ESC),
+	KEY(1, 2, KEY_TAB),
+	KEY(1, 3, KEY_GRAVE),
+	KEY(1, 4, KEY_A),
+	KEY(1, 5, KEY_Z),
+	KEY(1, 6, KEY_1),
+	KEY(1, 7, KEY_Q),
+
+	KEY(2, 0, KEY_F1),
+	KEY(2, 1, KEY_F4),
+	KEY(2, 2, KEY_F3),
+	KEY(2, 3, KEY_F2),
+	KEY(2, 4, KEY_D),
+	KEY(2, 5, KEY_C),
+	KEY(2, 6, KEY_3),
+	KEY(2, 7, KEY_E),
+
+	KEY(4, 0, KEY_B),
+	KEY(4, 1, KEY_G),
+	KEY(4, 2, KEY_T),
+	KEY(4, 3, KEY_5),
+	KEY(4, 4, KEY_F),
+	KEY(4, 5, KEY_V),
+	KEY(4, 6, KEY_4),
+	KEY(4, 7, KEY_R),
+
+	KEY(5, 0, KEY_F10),
+	KEY(5, 1, KEY_F7),
+	KEY(5, 2, KEY_F6),
+	KEY(5, 3, KEY_F5),
+	KEY(5, 4, KEY_S),
+	KEY(5, 5, KEY_X),
+	KEY(5, 6, KEY_2),
+	KEY(5, 7, KEY_W),
+
+	KEY(6, 0, KEY_RO),
+	KEY(6, 2, KEY_RIGHTBRACE),
+	KEY(6, 4, KEY_K),
+	KEY(6, 5, KEY_COMMA),
+	KEY(6, 6, KEY_8),
+	KEY(6, 7, KEY_I),
+
+	KEY(8, 0, KEY_N),
+	KEY(8, 1, KEY_H),
+	KEY(8, 2, KEY_Y),
+	KEY(8, 3, KEY_6),
+	KEY(8, 4, KEY_J),
+	KEY(8, 5, KEY_M),
+	KEY(8, 6, KEY_7),
+	KEY(8, 7, KEY_U),
+
+	KEY(9, 2, KEY_102ND),
+	KEY(9, 5, KEY_LEFTSHIFT),
+	KEY(9, 7, KEY_RIGHTSHIFT),
+
+	KEY(10, 0, KEY_EQUAL),
+	KEY(10, 1, KEY_APOSTROPHE),
+	KEY(10, 2, KEY_LEFTBRACE),
+	KEY(10, 3, KEY_MINUS),
+	KEY(10, 4, KEY_SEMICOLON),
+	KEY(10, 5, KEY_SLASH),
+	KEY(10, 6, KEY_0),
+	KEY(10, 7, KEY_P),
+
+	KEY(11, 1, KEY_F9),
+	KEY(11, 2, KEY_F8),
+	KEY(11, 4, KEY_L),
+	KEY(11, 5, KEY_DOT),
+	KEY(11, 6, KEY_9),
+	KEY(11, 7, KEY_O),
+
+	KEY(13, 0, KEY_RIGHTALT),
+	KEY(13, 2, KEY_YEN),
+	KEY(13, 4, KEY_BACKSLASH),
+
+	KEY(13, 6, KEY_LEFTALT),
+
+	KEY(14, 1, KEY_BACKSPACE),
+	KEY(14, 3, KEY_BACKSLASH),
+	KEY(14, 4, KEY_ENTER),
+	KEY(14, 5, KEY_SPACE),
+	KEY(14, 6, KEY_DOWN),
+	KEY(14, 7, KEY_UP),
+
+	KEY(15, 1, KEY_MUHENKAN),
+	KEY(15, 3, KEY_HENKAN),
+	KEY(15, 6, KEY_RIGHT),
+	KEY(15, 7, KEY_LEFT),
+};
+
+static const struct matrix_keymap_data cros_keymap_data __devinitdata = {
+	.keymap		= cros_kbd_keymap,
+	.keymap_size	= ARRAY_SIZE(cros_kbd_keymap),
+};
+
 static void tegra_kbc_report_released_keys(struct input_dev *input,
 					   unsigned short old_keycodes[],
 					   unsigned int old_num_keys,
@@ -603,6 +703,9 @@ static struct tegra_kbc_platform_data * __devinit tegra_kbc_dt_parse_pdata(
 	if (of_find_property(np, "wakeup-source", NULL))
 		pdata->wakeup = true;
 
+	if (of_find_property(np, "chromeos-layout", NULL))
+		pdata->keymap_data = &cros_keymap_data;
+
 	/* All currently known keymaps with device tree support use the same
 	 * pin_cfg, so set it up here.
 	 */
-- 
1.7.8.GIT

[PATCH v2 1/2] Input: tegra-kbc - add device tree bindings

[Dmitry Torokhov]

Hi Olof,

On Tue, Dec 27, 2011 at 10:19:29PM -0800, Olof Johansson wrote:
> This adds a simple device tree binding to the tegra keyboard controller.
> 
> Also, mark the default keymap as __devinitdata since it is not referenced
> after boot, and shorten the name to not have to avoid line wraps.

No, let's keep the names prefixed to match the rest of the driver(s).

> +
> +	pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);

Error handling is missing. I also dislike devm_* facilities as it causes
inconsistencies in the way we handle releasing of resources: some of
them will be released automatically while others need t be released
manually. I prefer having consistent model.

Does the patch below still work for you?

Thanks.

-- 
Dmitry

Input: tegra-kbc - add device tree bindings

From: Olof Johansson <olof@lixom.net>

This adds a simple device tree binding to the tegra keyboard controller.

Also, mark the default keymap as __devinitdata since it is not referenced
after boot.

Signed-off-by: Olof Johansson <olof@lixom.net>
Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
---

 .../devicetree/bindings/input/tegra-kbc.txt        |   18 ++++
 drivers/input/keyboard/tegra-kbc.c                 |   92 ++++++++++++++++++--
 2 files changed, 102 insertions(+), 8 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/input/tegra-kbc.txt


diff --git a/Documentation/devicetree/bindings/input/tegra-kbc.txt b/Documentation/devicetree/bindings/input/tegra-kbc.txt
new file mode 100644
index 0000000..5ecfa99
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/tegra-kbc.txt
@@ -0,0 +1,18 @@
+* Tegra keyboard controller
+
+Required properties:
+- compatible: "nvidia,tegra20-kbc"
+
+Optional properties:
+- debounce-delay: delay in milliseconds per row scan for debouncing
+- repeat-delay: delay in milliseconds before repeat starts
+- ghost-filter: enable ghost filtering for this device
+- wakeup-source: configure keyboard as a wakeup source for suspend/resume
+
+Example:
+
+keyboard: keyboard {
+	compatible = "nvidia,tegra20-kbc";
+	reg = <0x7000e200 0x100>;
+	ghost-filter;
+};
diff --git a/drivers/input/keyboard/tegra-kbc.c b/drivers/input/keyboard/tegra-kbc.c
index 08d02f2..b77725d 100644
--- a/drivers/input/keyboard/tegra-kbc.c
+++ b/drivers/input/keyboard/tegra-kbc.c
@@ -26,6 +26,7 @@
 #include <linux/delay.h>
 #include <linux/io.h>
 #include <linux/interrupt.h>
+#include <linux/of.h>
 #include <linux/clk.h>
 #include <linux/slab.h>
 #include <mach/clk.h>
@@ -82,7 +83,7 @@ struct tegra_kbc {
 	struct clk *clk;
 };
 
-static const u32 tegra_kbc_default_keymap[] = {
+static const u32 tegra_kbc_default_keymap[] __devinitdata = {
 	KEY(0, 2, KEY_W),
 	KEY(0, 3, KEY_S),
 	KEY(0, 4, KEY_A),
@@ -217,7 +218,8 @@ static const u32 tegra_kbc_default_keymap[] = {
 	KEY(31, 4, KEY_HELP),
 };
 
-static const struct matrix_keymap_data tegra_kbc_default_keymap_data = {
+static const
+struct matrix_keymap_data tegra_kbc_default_keymap_data __devinitdata = {
 	.keymap		= tegra_kbc_default_keymap,
 	.keymap_size	= ARRAY_SIZE(tegra_kbc_default_keymap),
 };
@@ -576,6 +578,56 @@ tegra_kbc_check_pin_cfg(const struct tegra_kbc_platform_data *pdata,
 	return true;
 }
 
+#ifdef CONFIG_OF
+static struct tegra_kbc_platform_data * __devinit
+tegra_kbc_dt_parse_pdata(struct platform_device *pdev)
+{
+	struct tegra_kbc_platform_data *pdata;
+	struct device_node *np = pdev->dev.of_node;
+
+	if (!np)
+		return NULL;
+
+	pdata = kzalloc(sizeof(*pdata), GFP_KERNEL);
+	if (!pdata)
+		return NULL;
+
+	if (!of_property_read_u32(np, "debounce-delay", &prop))
+		pdata->debounce_cnt = prop;
+
+	if (!of_property_read_u32(np, "repeat-delay", &prop))
+		pdata->repeat_cnt = prop;
+
+	if (of_find_property(np, "needs-ghost-filter", NULL))
+		pdata->use_ghost_filter = true;
+
+	if (of_find_property(np, "wakeup-source", NULL))
+		pdata->wakeup = true;
+
+	/*
+	 * All currently known keymaps with device tree support use the same
+	 * pin_cfg, so set it up here.
+	 */
+	for (i = 0; i < KBC_MAX_ROW; i++) {
+		pdata->pin_cfg[i].num = i;
+		pdata->pin_cfg[i].is_row = true;
+	}
+
+	for (i = 0; i < KBC_MAX_COL; i++) {
+		pdata->pin_cfg[KBC_MAX_ROW + i].num = i;
+		pdata->pin_cfg[KBC_MAX_ROW + i].is_row = false;
+	}
+
+	return pdata;
+}
+#else
+static inline struct tegra_kbc_platform_data *tegra_kbc_dt_parse_pdata(
+	struct platform_device *pdev)
+{
+	return NULL;
+}
+#endif
+
 static int __devinit tegra_kbc_probe(struct platform_device *pdev)
 {
 	const struct tegra_kbc_platform_data *pdata = pdev->dev.platform_data;
@@ -590,21 +642,28 @@ static int __devinit tegra_kbc_probe(struct platform_device *pdev)
 	unsigned int scan_time_rows;
 
 	if (!pdata)
-		return -EINVAL;
+		pdata = tegra_kbc_dt_parse_pdata(pdev);
 
-	if (!tegra_kbc_check_pin_cfg(pdata, &pdev->dev, &num_rows))
+	if (!pdata)
 		return -EINVAL;
 
+	if (!tegra_kbc_check_pin_cfg(pdata, &pdev->dev, &num_rows)) {
+		err = -EINVAL;
+		goto err_free_pdata;
+	}
+
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	if (!res) {
 		dev_err(&pdev->dev, "failed to get I/O memory\n");
-		return -ENXIO;
+		err = -ENXIO;
+		goto err_free_pdata;
 	}
 
 	irq = platform_get_irq(pdev, 0);
 	if (irq < 0) {
 		dev_err(&pdev->dev, "failed to get keyboard IRQ\n");
-		return -ENXIO;
+		err = -ENXIO;
+		goto err_free_pdata;
 	}
 
 	kbc = kzalloc(sizeof(*kbc), GFP_KERNEL);
@@ -706,6 +765,9 @@ err_free_mem_region:
 err_free_mem:
 	input_free_device(input_dev);
 	kfree(kbc);
+err_free_pdata:
+	if (!pdev->dev.platform_data)
+		kfree(pdata);
 
 	return err;
 }
@@ -715,6 +777,8 @@ static int __devexit tegra_kbc_remove(struct platform_device *pdev)
 	struct tegra_kbc *kbc = platform_get_drvdata(pdev);
 	struct resource *res;
 
+	platform_set_drvdata(pdev, NULL);
+
 	free_irq(kbc->irq, pdev);
 	clk_put(kbc->clk);
 
@@ -723,9 +787,14 @@ static int __devexit tegra_kbc_remove(struct platform_device *pdev)
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	release_mem_region(res->start, resource_size(res));
 
-	kfree(kbc);
+	/*
+	 * If we do not have platform data attached to the device we
+	 * allocated it ourselves and thus need to free it.
+	 */
+	if (!pdev->dev.platform_data)
+		kfree(kbc->pdata);
 
-	platform_set_drvdata(pdev, NULL);
+	kfree(kbc);
 
 	return 0;
 }
@@ -793,6 +862,12 @@ static int tegra_kbc_resume(struct device *dev)
 
 static SIMPLE_DEV_PM_OPS(tegra_kbc_pm_ops, tegra_kbc_suspend, tegra_kbc_resume);
 
+static const struct of_device_id tegra_kbc_of_match[] = {
+	{ .compatible = "nvidia,tegra20-kbc", },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, tegra_kbc_of_match);
+
 static struct platform_driver tegra_kbc_driver = {
 	.probe		= tegra_kbc_probe,
 	.remove		= __devexit_p(tegra_kbc_remove),
@@ -800,6 +875,7 @@ static struct platform_driver tegra_kbc_driver = {
 		.name	= "tegra-kbc",
 		.owner  = THIS_MODULE,
 		.pm	= &tegra_kbc_pm_ops,
+		.of_match_table = tegra_kbc_of_match,
 	},
 };
 module_platform_driver(tegra_kbc_driver);

[PATCH v2 2/2] Input: tegra-kbc - add default chromeos map

[Dmitry Torokhov]

On Tue, Dec 27, 2011 at 10:19:30PM -0800, Olof Johansson wrote:
> This adds a simple way to specify the ChromeOS-specific keyboard map
> instead of the "standard" map that is used on other Tegra devices such
> as Harmony-with-keyboard and Seaboard.
> 
> I expect the number of different keyboard layouts to be quite limited,
> and not many should be added over time. So instead of encoding the layout
> in the device tree, with all the can of worms that entails w.r.t. agreeing
> on a suitable binding, just add a property to specify that this is the
> map to be used, and include it in the driver.
> 
> If, over time, the number of mappings increase, the binding can be
> updated to include a custom map as a new property, without having to
> worry about backwards compatibility on existing devices.
> 

I'd rather we did not make shortcuts and did the proper DT binding.
Samsung folks want the similar thing so making a generic DT keymap
parser and using it in the driver would be the best way.

Another option is to simply loading proper keymap by the early
userspace.

Thanks.

-- 
Dmitry

[PATCH v2 1/2] Input: tegra-kbc - add device tree bindings

[Olof Johansson]

Hi,

On Tue, Dec 27, 2011 at 10:48 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> Hi Olof,
>
> On Tue, Dec 27, 2011 at 10:19:29PM -0800, Olof Johansson wrote:
>> This adds a simple device tree binding to the tegra keyboard controller.
>>
>> Also, mark the default keymap as __devinitdata since it is not referenced
>> after boot, and shorten the name to not have to avoid line wraps.
>
> No, let's keep the names prefixed to match the rest of the driver(s).

Sure, if you prefer.

>> +
>> + ? ? pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
>
> Error handling is missing. I also dislike devm_* facilities as it causes
> inconsistencies in the way we handle releasing of resources: some of
> them will be released automatically while others need t be released
> manually. I prefer having consistent model.

I disagree -- I think the devm allocators are an excellent invention
instead of open-coding the error handling, which tends to be error
prone and just adds special cases just like you explained below.

But, it's your subsystem and with that being said...

> Does the patch below still work for you?

... sure, if you're happy I'm happy -- it gets the work done, couldn't
ask for more. No need to argue the details further. :)


-Olof

[PATCH v2 2/2] Input: tegra-kbc - add default chromeos map

[Olof Johansson]

Hi,

On Tue, Dec 27, 2011 at 10:50 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On Tue, Dec 27, 2011 at 10:19:30PM -0800, Olof Johansson wrote:
>> This adds a simple way to specify the ChromeOS-specific keyboard map
>> instead of the "standard" map that is used on other Tegra devices such
>> as Harmony-with-keyboard and Seaboard.
>>
>> I expect the number of different keyboard layouts to be quite limited,
>> and not many should be added over time. So instead of encoding the layout
>> in the device tree, with all the can of worms that entails w.r.t. agreeing
>> on a suitable binding, just add a property to specify that this is the
>> map to be used, and include it in the driver.
>>
>> If, over time, the number of mappings increase, the binding can be
>> updated to include a custom map as a new property, without having to
>> worry about backwards compatibility on existing devices.
>>
>
> I'd rather we did not make shortcuts and did the proper DT binding.
> Samsung folks want the similar thing so making a generic DT keymap
> parser and using it in the driver would be the best way.

Ok, fair point. I found the last posted version of the samsung driver
in my archives so I'll continue the discussion there (see separate
followup there).

> Another option is to simply loading proper keymap by the early
> userspace.

I would prefer to do it in hardware, mostly because I don't want to
have to add the keymaps to all possible distros I'd be interested in
running on this hardware. :)


-Olof

[PATCH v2 1/2] Input: tegra-kbc - add device tree bindings

[Stephen Warren]

Dmitry Torokhov wrote at Tuesday, December 27, 2011 11:49 PM:
> On Tue, Dec 27, 2011 at 10:19:29PM -0800, Olof Johansson wrote:
> > This adds a simple device tree binding to the tegra keyboard controller.
...
> > +	pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
> 
> Error handling is missing. I also dislike devm_* facilities as it causes
> inconsistencies in the way we handle releasing of resources: some of
> them will be released automatically while others need t be released
> manually. I prefer having consistent model.

I have to say that I also disagree here. Weren't the devm_* function
specifically added to allow people not to write all the error-handling
code themselves. Forcing everyone not to use them doesn't seem right.
There have been a ton of patches throughout the kernel converting drivers
to use devm_* and at the same time removing and simplifying error
handling.

-- 
nvpublic

[PATCH v2 2/2] Input: tegra-kbc - add default chromeos map

[Stephen Warren]

Olof Johansson wrote at Wednesday, December 28, 2011 12:33 AM:
> On Tue, Dec 27, 2011 at 10:50 PM, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> > On Tue, Dec 27, 2011 at 10:19:30PM -0800, Olof Johansson wrote:
> >> This adds a simple way to specify the ChromeOS-specific keyboard map
> >> instead of the "standard" map that is used on other Tegra devices such
> >> as Harmony-with-keyboard and Seaboard.
> >>
> >> I expect the number of different keyboard layouts to be quite limited,
> >> and not many should be added over time. So instead of encoding the layout
> >> in the device tree, with all the can of worms that entails w.r.t. agreeing
> >> on a suitable binding, just add a property to specify that this is the
> >> map to be used, and include it in the driver.
> >>
> >> If, over time, the number of mappings increase, the binding can be
> >> updated to include a custom map as a new property, without having to
> >> worry about backwards compatibility on existing devices.
> >>
> >
> > I'd rather we did not make shortcuts and did the proper DT binding.
> > Samsung folks want the similar thing so making a generic DT keymap
> > parser and using it in the driver would be the best way.
> 
> Ok, fair point. I found the last posted version of the samsung driver
> in my archives so I'll continue the discussion there (see separate
> followup there).

I agree here. Simon Glass has posted some patches to U-Boot to add the
Tegra KBC driver including DT bindings. Can you please co-ordinate with
him to ensure the bindings you're proposing for the kernel match those
he's proposing for U-Boot; from memory, I don't think they are so far.
Also, we need to work out the issue of the kernel needing just the base
and function-modified keymap, but U-Boot needing a shift- and ctrl-
modified keymap since there's no input layer handling shift/ctrl in
U-Boot. I'd prefer that the DT binding be encoded as:

matrix -> keycode for base
matrix -> keycode for function-modified

With the binding for those two defined by either the Tegra KBC binding or
a generic matrix KBC binding.

keycode -> keycode for shift-modified
keycode -> keycode for ctrl-modified

With the binding for those two defined by some kind of generic modifier
mapping binding, so it could feed into the input layer on Linux, although
in U-Boot, the Tegra KBC driver would have to handle it itself, since
there's nowhere else to handle it.

-- 
nvpublic

[PATCH v2 2/2] Input: tegra-kbc - add default chromeos map

[Olof Johansson]

On Wed, Dec 28, 2011 at 10:06 PM, Stephen Warren <swarren@nvidia.com> wrote:
> Olof Johansson wrote at Wednesday, December 28, 2011 12:33 AM:
>> On Tue, Dec 27, 2011 at 10:50 PM, Dmitry Torokhov
>> <dmitry.torokhov@gmail.com> wrote:
>> > On Tue, Dec 27, 2011 at 10:19:30PM -0800, Olof Johansson wrote:
>> >> This adds a simple way to specify the ChromeOS-specific keyboard map
>> >> instead of the "standard" map that is used on other Tegra devices such
>> >> as Harmony-with-keyboard and Seaboard.
>> >>
>> >> I expect the number of different keyboard layouts to be quite limited,
>> >> and not many should be added over time. So instead of encoding the layout
>> >> in the device tree, with all the can of worms that entails w.r.t. agreeing
>> >> on a suitable binding, just add a property to specify that this is the
>> >> map to be used, and include it in the driver.
>> >>
>> >> If, over time, the number of mappings increase, the binding can be
>> >> updated to include a custom map as a new property, without having to
>> >> worry about backwards compatibility on existing devices.
>> >>
>> >
>> > I'd rather we did not make shortcuts and did the proper DT binding.
>> > Samsung folks want the similar thing so making a generic DT keymap
>> > parser and using it in the driver would be the best way.
>>
>> Ok, fair point. I found the last posted version of the samsung driver
>> in my archives so I'll continue the discussion there (see separate
>> followup there).
>
> I agree here. Simon Glass has posted some patches to U-Boot to add the
> Tegra KBC driver including DT bindings. Can you please co-ordinate with
> him to ensure the bindings you're proposing for the kernel match those
> he's proposing for U-Boot; from memory, I don't think they are so far.
> Also, we need to work out the issue of the kernel needing just the base
> and function-modified keymap, but U-Boot needing a shift- and ctrl-
> modified keymap since there's no input layer handling shift/ctrl in
> U-Boot.

I answered this in the other email; since the modifier keymaps don't
actually describe hardware functionality but instead software
remappings, it should be done in u-boot, and the device tree binding
should be focused on describing the hardware.

> I'd prefer that the DT binding be encoded as:
>
> matrix -> keycode for base
> matrix -> keycode for function-modified
>
> With the binding for those two defined by either the Tegra KBC binding or
> a generic matrix KBC binding.
>
> keycode -> keycode for shift-modified
> keycode -> keycode for ctrl-modified
>
> With the binding for those two defined by some kind of generic modifier
> mapping binding, so it could feed into the input layer on Linux, although
> in U-Boot, the Tegra KBC driver would have to handle it itself, since
> there's nowhere else to handle it.

This is in my opinion unnecessarily complicated, and it's trying to
push too much upper-layer information down into the hardware
description. Keeping the binding to the bare minimum needed to
describe the actual hardware configuration is the natural common
ground not just between u-boot and linux, but also with other
operating systems and between different hardware vendors.


-Olof

[PATCH v2 2/2] Input: tegra-kbc - add default chromeos map

[Stephen Warren]

Olof Johansson wrote at Wednesday, December 28, 2011 11:42 PM:
> On Wed, Dec 28, 2011 at 10:06 PM, Stephen Warren <swarren@nvidia.com> wrote:
> > Olof Johansson wrote at Wednesday, December 28, 2011 12:33 AM:
> >> On Tue, Dec 27, 2011 at 10:50 PM, Dmitry Torokhov
> >> <dmitry.torokhov@gmail.com> wrote:
> >> > On Tue, Dec 27, 2011 at 10:19:30PM -0800, Olof Johansson wrote:
> >> >> This adds a simple way to specify the ChromeOS-specific keyboard map
> >> >> instead of the "standard" map that is used on other Tegra devices such
> >> >> as Harmony-with-keyboard and Seaboard.
> >> >>
> >> >> I expect the number of different keyboard layouts to be quite limited,
> >> >> and not many should be added over time. So instead of encoding the layout
> >> >> in the device tree, with all the can of worms that entails w.r.t. agreeing
> >> >> on a suitable binding, just add a property to specify that this is the
> >> >> map to be used, and include it in the driver.
> >> >>
> >> >> If, over time, the number of mappings increase, the binding can be
> >> >> updated to include a custom map as a new property, without having to
> >> >> worry about backwards compatibility on existing devices.
> >> >>
> >> >
> >> > I'd rather we did not make shortcuts and did the proper DT binding.
> >> > Samsung folks want the similar thing so making a generic DT keymap
> >> > parser and using it in the driver would be the best way.
> >>
> >> Ok, fair point. I found the last posted version of the samsung driver
> >> in my archives so I'll continue the discussion there (see separate
> >> followup there).
> >
> > I agree here. Simon Glass has posted some patches to U-Boot to add the
> > Tegra KBC driver including DT bindings. Can you please co-ordinate with
> > him to ensure the bindings you're proposing for the kernel match those
> > he's proposing for U-Boot; from memory, I don't think they are so far.
> > Also, we need to work out the issue of the kernel needing just the base
> > and function-modified keymap, but U-Boot needing a shift- and ctrl-
> > modified keymap since there's no input layer handling shift/ctrl in
> > U-Boot.
> 
> I answered this in the other email; since the modifier keymaps don't
> actually describe hardware functionality but instead software
> remappings, it should be done in u-boot, and the device tree binding
> should be focused on describing the hardware.

For function/Fn, I think representing that in DT makes sense; there's
no simple/standard mapping that any driver could implement to translate
a raw keypress to an fn-keypress; the labeling is completely board-
specific, and the Fn key is really multi-plexing two completely different
keys onto a single physical key (on my x86 laptop, I have a single key
for both Insert and Delete, differentiating by using the Fn key). PC
(PS/2 or USB) keyboards handle Fn internally to the HW, so I think this
gives precedent for considering an Fn mapping part of the HW for a matrix
Keyboard.

For shift/ctrl, I agree not having the mappings in DT makes sense. However,
Simon Glass gave me pushback on this when I requested the removal of those
properties in the U-Boot patch.

Note that my comments immediately below were more about how to modify
Simon Glass's patch to be acceptable rather than your proposal.

> > I'd prefer that the DT binding be encoded as:
> >
> > matrix -> keycode for base
> > matrix -> keycode for function-modified
> >
> > With the binding for those two defined by either the Tegra KBC binding or
> > a generic matrix KBC binding.
> >
> > keycode -> keycode for shift-modified
> > keycode -> keycode for ctrl-modified
> >
> > With the binding for those two defined by some kind of generic modifier
> > mapping binding, so it could feed into the input layer on Linux, although
> > in U-Boot, the Tegra KBC driver would have to handle it itself, since
> > there's nowhere else to handle it.
> 
> This is in my opinion unnecessarily complicated, and it's trying to
> push too much upper-layer information down into the hardware
> description. Keeping the binding to the bare minimum needed to
> describe the actual hardware configuration is the natural common
> ground not just between u-boot and linux, but also with other
> operating systems and between different hardware vendors.

-- 
nvpublic

[PATCH v2 2/2] Input: tegra-kbc - add default chromeos map

[Olof Johansson]

On Wed, Dec 28, 2011 at 10:58 PM, Stephen Warren <swarren@nvidia.com> wrote:
> Olof Johansson wrote at Wednesday, December 28, 2011 11:42 PM:
>> On Wed, Dec 28, 2011 at 10:06 PM, Stephen Warren <swarren@nvidia.com> wrote:
>> > Olof Johansson wrote at Wednesday, December 28, 2011 12:33 AM:
>> >> On Tue, Dec 27, 2011 at 10:50 PM, Dmitry Torokhov
>> >> <dmitry.torokhov@gmail.com> wrote:
>> >> > On Tue, Dec 27, 2011 at 10:19:30PM -0800, Olof Johansson wrote:
>> >> >> This adds a simple way to specify the ChromeOS-specific keyboard map
>> >> >> instead of the "standard" map that is used on other Tegra devices such
>> >> >> as Harmony-with-keyboard and Seaboard.
>> >> >>
>> >> >> I expect the number of different keyboard layouts to be quite limited,
>> >> >> and not many should be added over time. So instead of encoding the layout
>> >> >> in the device tree, with all the can of worms that entails w.r.t. agreeing
>> >> >> on a suitable binding, just add a property to specify that this is the
>> >> >> map to be used, and include it in the driver.
>> >> >>
>> >> >> If, over time, the number of mappings increase, the binding can be
>> >> >> updated to include a custom map as a new property, without having to
>> >> >> worry about backwards compatibility on existing devices.
>> >> >>
>> >> >
>> >> > I'd rather we did not make shortcuts and did the proper DT binding.
>> >> > Samsung folks want the similar thing so making a generic DT keymap
>> >> > parser and using it in the driver would be the best way.
>> >>
>> >> Ok, fair point. I found the last posted version of the samsung driver
>> >> in my archives so I'll continue the discussion there (see separate
>> >> followup there).
>> >
>> > I agree here. Simon Glass has posted some patches to U-Boot to add the
>> > Tegra KBC driver including DT bindings. Can you please co-ordinate with
>> > him to ensure the bindings you're proposing for the kernel match those
>> > he's proposing for U-Boot; from memory, I don't think they are so far.
>> > Also, we need to work out the issue of the kernel needing just the base
>> > and function-modified keymap, but U-Boot needing a shift- and ctrl-
>> > modified keymap since there's no input layer handling shift/ctrl in
>> > U-Boot.
>>
>> I answered this in the other email; since the modifier keymaps don't
>> actually describe hardware functionality but instead software
>> remappings, it should be done in u-boot, and the device tree binding
>> should be focused on describing the hardware.
>
> For function/Fn, I think representing that in DT makes sense; there's
> no simple/standard mapping that any driver could implement to translate
> a raw keypress to an fn-keypress; the labeling is completely board-
> specific, and the Fn key is really multi-plexing two completely different
> keys onto a single physical key (on my x86 laptop, I have a single key
> for both Insert and Delete, differentiating by using the Fn key).

I'm torn. The analogy here is more about localized keyboards than
anything else, and the solution there is to handle it in higher layers
of the input stack.

I could be OK with _one_ additional optional keymap for the fn
modifier, but I think it's a slippery slope. I know Rakesh got
pushback when he originally tried to present the hardware that way in
the  platform data, so I guess we might get pushback there now as
well. Dmitry?

> PC
> (PS/2 or USB) keyboards handle Fn internally to the HW, so I think this
> gives precedent for considering an Fn mapping part of the HW for a matrix
> Keyboard.

Right, the modifier key in that case is either a physical modification
of which circuits get closed, or at least it's treated that way by the
EC that presents it in such a manner.

> For shift/ctrl, I agree not having the mappings in DT makes sense. However,
> Simon Glass gave me pushback on this when I requested the removal of those
> properties in the U-Boot patch.
>
> Note that my comments immediately below were more about how to modify
> Simon Glass's patch to be acceptable rather than your proposal.

Ok, I think we're in agreement on that. And since they already need
software mappings for that, they could add one for unmodified keys and
use that for the linux-keycode -> ascii char translation if needed.


-Olof

[PATCH v2 1/2] Input: tegra-kbc - add device tree bindings

[Dmitry Torokhov]

On Wed, Dec 28, 2011 at 10:00:06PM -0800, Stephen Warren wrote:
> Dmitry Torokhov wrote at Tuesday, December 27, 2011 11:49 PM:
> > On Tue, Dec 27, 2011 at 10:19:29PM -0800, Olof Johansson wrote:
> > > This adds a simple device tree binding to the tegra keyboard controller.
> ...
> > > +	pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
> > 
> > Error handling is missing. I also dislike devm_* facilities as it causes
> > inconsistencies in the way we handle releasing of resources: some of
> > them will be released automatically while others need t be released
> > manually. I prefer having consistent model.
> 
> I have to say that I also disagree here. Weren't the devm_* function
> specifically added to allow people not to write all the error-handling
> code themselves.

I guess they were. Unfortunately they also introduce inconsistency in
the way resources are tracked and freed since some of them support
devm_* facilities while others don't. I might be OK with drivers that
use one model for all resources but this patch was mixing the two.

devm_* facilities are also not free; you are saving on error handling
code but pay with memory footprint required to implement tracking.

> Forcing everyone not to use them doesn't seem right.
> There have been a ton of patches throughout the kernel converting drivers
> to use devm_* and at the same time removing and simplifying error
> handling.
>

Thanks.

-- 
Dmitry

[PATCH v2 2/2] Input: tegra-kbc - add default chromeos map

[Dmitry Torokhov]

On Wed, Dec 28, 2011 at 11:13:50PM -0800, Olof Johansson wrote:
> On Wed, Dec 28, 2011 at 10:58 PM, Stephen Warren <swarren@nvidia.com> wrote:
> > Olof Johansson wrote at Wednesday, December 28, 2011 11:42 PM:
> >> On Wed, Dec 28, 2011 at 10:06 PM, Stephen Warren <swarren@nvidia.com> wrote:
> >> > Olof Johansson wrote at Wednesday, December 28, 2011 12:33 AM:
> >> >> On Tue, Dec 27, 2011 at 10:50 PM, Dmitry Torokhov
> >> >> <dmitry.torokhov@gmail.com> wrote:
> >> >> > On Tue, Dec 27, 2011 at 10:19:30PM -0800, Olof Johansson wrote:
> >> >> >> This adds a simple way to specify the ChromeOS-specific keyboard map
> >> >> >> instead of the "standard" map that is used on other Tegra devices such
> >> >> >> as Harmony-with-keyboard and Seaboard.
> >> >> >>
> >> >> >> I expect the number of different keyboard layouts to be quite limited,
> >> >> >> and not many should be added over time. So instead of encoding the layout
> >> >> >> in the device tree, with all the can of worms that entails w.r.t. agreeing
> >> >> >> on a suitable binding, just add a property to specify that this is the
> >> >> >> map to be used, and include it in the driver.
> >> >> >>
> >> >> >> If, over time, the number of mappings increase, the binding can be
> >> >> >> updated to include a custom map as a new property, without having to
> >> >> >> worry about backwards compatibility on existing devices.
> >> >> >>
> >> >> >
> >> >> > I'd rather we did not make shortcuts and did the proper DT binding.
> >> >> > Samsung folks want the similar thing so making a generic DT keymap
> >> >> > parser and using it in the driver would be the best way.
> >> >>
> >> >> Ok, fair point. I found the last posted version of the samsung driver
> >> >> in my archives so I'll continue the discussion there (see separate
> >> >> followup there).
> >> >
> >> > I agree here. Simon Glass has posted some patches to U-Boot to add the
> >> > Tegra KBC driver including DT bindings. Can you please co-ordinate with
> >> > him to ensure the bindings you're proposing for the kernel match those
> >> > he's proposing for U-Boot; from memory, I don't think they are so far.
> >> > Also, we need to work out the issue of the kernel needing just the base
> >> > and function-modified keymap, but U-Boot needing a shift- and ctrl-
> >> > modified keymap since there's no input layer handling shift/ctrl in
> >> > U-Boot.
> >>
> >> I answered this in the other email; since the modifier keymaps don't
> >> actually describe hardware functionality but instead software
> >> remappings, it should be done in u-boot, and the device tree binding
> >> should be focused on describing the hardware.
> >
> > For function/Fn, I think representing that in DT makes sense; there's
> > no simple/standard mapping that any driver could implement to translate
> > a raw keypress to an fn-keypress; the labeling is completely board-
> > specific, and the Fn key is really multi-plexing two completely different
> > keys onto a single physical key (on my x86 laptop, I have a single key
> > for both Insert and Delete, differentiating by using the Fn key).
> 
> I'm torn. The analogy here is more about localized keyboards than
> anything else, and the solution there is to handle it in higher layers
> of the input stack.
> 
> I could be OK with _one_ additional optional keymap for the fn
> modifier, but I think it's a slippery slope. I know Rakesh got
> pushback when he originally tried to present the hardware that way in
> the  platform data, so I guess we might get pushback there now as
> well. Dmitry?

I think having separate Fn map is OK since, as Stephen correctly
mentioned, it usually emulates as Fn works in AT keyboards, i.e.
completely hidden by the firmware. Also the set of keys produced by Fn
is usually "control"-like and requiring keymaps would stop clients
listening on /dev/input/eventX from recognizing them.

Here is what I wrote to Rakesh:

> Yes, indeed, you are stumbling against limitation of X which does not
> allow to pass keycodes above 255 through it.
> 
> One option would be assigning KEY_LEFTMETA or KEY_RIGHTMETA to your 'Fn'
> key and then adjusting keymaps, but after I looked again at the keys you
> have assigned by default to your Fn combinations  can see how one would
> want to avoid involving X's keymaps and be able to generate needed
> keycodes directly (volume, brightness and other control events) so that
> infrastructure need not be hooked into X to be able to react to them.

The Shift, Ctrl and other modifiers, on the other hand, should be done
in userspace so that standard locale-specific keymap could be loaded and
used.

> 
> > PC
> > (PS/2 or USB) keyboards handle Fn internally to the HW, so I think this
> > gives precedent for considering an Fn mapping part of the HW for a matrix
> > Keyboard.
> 
> Right, the modifier key in that case is either a physical modification
> of which circuits get closed, or at least it's treated that way by the
> EC that presents it in such a manner.
> 
> > For shift/ctrl, I agree not having the mappings in DT makes sense. However,
> > Simon Glass gave me pushback on this when I requested the removal of those
> > properties in the U-Boot patch.
> >
> > Note that my comments immediately below were more about how to modify
> > Simon Glass's patch to be acceptable rather than your proposal.
> 
> Ok, I think we're in agreement on that. And since they already need
> software mappings for that, they could add one for unmodified keys and
> use that for the linux-keycode -> ascii char translation if needed.
> 

-- 
Dmitry

[PATCH v2 2/2] Input: tegra-kbc - add default chromeos map

[Olof Johansson]

On Thu, Dec 29, 2011 at 1:41 AM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On Wed, Dec 28, 2011 at 11:13:50PM -0800, Olof Johansson wrote:
>> On Wed, Dec 28, 2011 at 10:58 PM, Stephen Warren <swarren@nvidia.com> wrote:
>> > Olof Johansson wrote at Wednesday, December 28, 2011 11:42 PM:
>> >> On Wed, Dec 28, 2011 at 10:06 PM, Stephen Warren <swarren@nvidia.com> wrote:
>> >> > Olof Johansson wrote at Wednesday, December 28, 2011 12:33 AM:
>> >> >> On Tue, Dec 27, 2011 at 10:50 PM, Dmitry Torokhov
>> >> >> <dmitry.torokhov@gmail.com> wrote:
>> >> >> > On Tue, Dec 27, 2011 at 10:19:30PM -0800, Olof Johansson wrote:
>> >> >> >> This adds a simple way to specify the ChromeOS-specific keyboard map
>> >> >> >> instead of the "standard" map that is used on other Tegra devices such
>> >> >> >> as Harmony-with-keyboard and Seaboard.
>> >> >> >>
>> >> >> >> I expect the number of different keyboard layouts to be quite limited,
>> >> >> >> and not many should be added over time. So instead of encoding the layout
>> >> >> >> in the device tree, with all the can of worms that entails w.r.t. agreeing
>> >> >> >> on a suitable binding, just add a property to specify that this is the
>> >> >> >> map to be used, and include it in the driver.
>> >> >> >>
>> >> >> >> If, over time, the number of mappings increase, the binding can be
>> >> >> >> updated to include a custom map as a new property, without having to
>> >> >> >> worry about backwards compatibility on existing devices.
>> >> >> >>
>> >> >> >
>> >> >> > I'd rather we did not make shortcuts and did the proper DT binding.
>> >> >> > Samsung folks want the similar thing so making a generic DT keymap
>> >> >> > parser and using it in the driver would be the best way.
>> >> >>
>> >> >> Ok, fair point. I found the last posted version of the samsung driver
>> >> >> in my archives so I'll continue the discussion there (see separate
>> >> >> followup there).
>> >> >
>> >> > I agree here. Simon Glass has posted some patches to U-Boot to add the
>> >> > Tegra KBC driver including DT bindings. Can you please co-ordinate with
>> >> > him to ensure the bindings you're proposing for the kernel match those
>> >> > he's proposing for U-Boot; from memory, I don't think they are so far.
>> >> > Also, we need to work out the issue of the kernel needing just the base
>> >> > and function-modified keymap, but U-Boot needing a shift- and ctrl-
>> >> > modified keymap since there's no input layer handling shift/ctrl in
>> >> > U-Boot.
>> >>
>> >> I answered this in the other email; since the modifier keymaps don't
>> >> actually describe hardware functionality but instead software
>> >> remappings, it should be done in u-boot, and the device tree binding
>> >> should be focused on describing the hardware.
>> >
>> > For function/Fn, I think representing that in DT makes sense; there's
>> > no simple/standard mapping that any driver could implement to translate
>> > a raw keypress to an fn-keypress; the labeling is completely board-
>> > specific, and the Fn key is really multi-plexing two completely different
>> > keys onto a single physical key (on my x86 laptop, I have a single key
>> > for both Insert and Delete, differentiating by using the Fn key).
>>
>> I'm torn. The analogy here is more about localized keyboards than
>> anything else, and the solution there is to handle it in higher layers
>> of the input stack.
>>
>> I could be OK with _one_ additional optional keymap for the fn
>> modifier, but I think it's a slippery slope. I know Rakesh got
>> pushback when he originally tried to present the hardware that way in
>> the ?platform data, so I guess we might get pushback there now as
>> well. Dmitry?
>
> I think having separate Fn map is OK since, as Stephen correctly
> mentioned, it usually emulates as Fn works in AT keyboards, i.e.
> completely hidden by the firmware. Also the set of keys produced by Fn
> is usually "control"-like and requiring keymaps would stop clients
> listening on /dev/input/eventX from recognizing them.

Alright, I'll revise the bindings (and make room for an optional
linux,fn-keymap property) tomorrow morning.

[...]

> The Shift, Ctrl and other modifiers, on the other hand, should be done
> in userspace so that standard locale-specific keymap could be loaded and
> used.

Alright. Now we just have to rope in Simon into agreement as well and
we'll be all set. He's back from his vacation next week.


-Olof

[PATCH v2 1/2] Input: tegra-kbc - add device tree bindings

[Grant Likely]

On Thu, Dec 29, 2011 at 01:23:08AM -0800, Dmitry Torokhov wrote:
> On Wed, Dec 28, 2011 at 10:00:06PM -0800, Stephen Warren wrote:
> > Dmitry Torokhov wrote at Tuesday, December 27, 2011 11:49 PM:
> > > On Tue, Dec 27, 2011 at 10:19:29PM -0800, Olof Johansson wrote:
> > > > This adds a simple device tree binding to the tegra keyboard controller.
> > ...
> > > > +	pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
> > > 
> > > Error handling is missing. I also dislike devm_* facilities as it causes
> > > inconsistencies in the way we handle releasing of resources: some of
> > > them will be released automatically while others need t be released
> > > manually. I prefer having consistent model.
> > 
> > I have to say that I also disagree here. Weren't the devm_* function
> > specifically added to allow people not to write all the error-handling
> > code themselves.
> 
> I guess they were. Unfortunately they also introduce inconsistency in
> the way resources are tracked and freed since some of them support
> devm_* facilities while others don't. I might be OK with drivers that
> use one model for all resources but this patch was mixing the two.
> 
> devm_* facilities are also not free; you are saving on error handling
> code but pay with memory footprint required to implement tracking.

FWIW, I agree with Olof and Stephen.  The devm_* functions are a good thing
and I strongly encourage driver authors to use them.  The cost in memory
footprint is pretty negligible, but making it easier for driver authors to
get things right is huge.

g.

[PATCH v2 1/2] Input: tegra-kbc - add device tree bindings

[Grant Likely]

On Tue, Dec 27, 2011 at 10:19:29PM -0800, Olof Johansson wrote:
> This adds a simple device tree binding to the tegra keyboard controller.
> 
> Also, mark the default keymap as __devinitdata since it is not referenced
> after boot, and shorten the name to not have to avoid line wraps.
> 
> Signed-off-by: Olof Johansson <olof@lixom.net>
> ---
>  .../devicetree/bindings/input/tegra-kbc.txt        |   18 +++++
>  drivers/input/keyboard/tegra-kbc.c                 |   70 ++++++++++++++++++--
>  2 files changed, 83 insertions(+), 5 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/input/tegra-kbc.txt
> 
> diff --git a/Documentation/devicetree/bindings/input/tegra-kbc.txt b/Documentation/devicetree/bindings/input/tegra-kbc.txt
> new file mode 100644
> index 0000000..a7c1d40
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/input/tegra-kbc.txt
> @@ -0,0 +1,18 @@
> +* Tegra keyboard controller
> +
> +Required properties:
> +- compatible : "nvidia,tegra20-kbc"
> +
> +Optional properties:
> +- debounce-delay: delay in milliseconds per row scan for debouncing
> +- repeat-delay: delay in milliseconds before repeat starts
> +- ghost-filter : enable ghost filtering for this device
> +- wakeup-source : configure keyboard as a wakeup source for suspend/resume

I'd like to see a "tegra," prefix on these custom properties, but otherwise
the binding looks okay to me.  You should also consider a '-ms' suffix on
the millisecond properties.

Otherwise;

Acked-by: Grant Likely <grant.likely@secretlab.ca>

> +
> +Example:
> +
> +keyboard: keyboard {
> +	compatible = "nvidia,tegra20-kbc";
> +	reg = <0x7000e200 0x100>;
> +	ghost-filter;
> +};
> diff --git a/drivers/input/keyboard/tegra-kbc.c b/drivers/input/keyboard/tegra-kbc.c
> index cf3228b..4ce6cc8 100644
> --- a/drivers/input/keyboard/tegra-kbc.c
> +++ b/drivers/input/keyboard/tegra-kbc.c
> @@ -26,6 +26,7 @@
>  #include <linux/delay.h>
>  #include <linux/io.h>
>  #include <linux/interrupt.h>
> +#include <linux/of.h>
>  #include <linux/clk.h>
>  #include <linux/slab.h>
>  #include <mach/clk.h>
> @@ -82,7 +83,7 @@ struct tegra_kbc {
>  	struct clk *clk;
>  };
>  
> -static const u32 tegra_kbc_default_keymap[] = {
> +static const u32 default_keymap[] __devinitdata = {
>  	KEY(0, 2, KEY_W),
>  	KEY(0, 3, KEY_S),
>  	KEY(0, 4, KEY_A),
> @@ -217,9 +218,9 @@ static const u32 tegra_kbc_default_keymap[] = {
>  	KEY(31, 4, KEY_HELP),
>  };
>  
> -static const struct matrix_keymap_data tegra_kbc_default_keymap_data = {
> -	.keymap		= tegra_kbc_default_keymap,
> -	.keymap_size	= ARRAY_SIZE(tegra_kbc_default_keymap),
> +static const struct matrix_keymap_data default_keymap_data __devinitdata = {
> +	.keymap		= default_keymap,
> +	.keymap_size	= ARRAY_SIZE(default_keymap),
>  };
>  
>  static void tegra_kbc_report_released_keys(struct input_dev *input,
> @@ -576,6 +577,55 @@ tegra_kbc_check_pin_cfg(const struct tegra_kbc_platform_data *pdata,
>  	return true;
>  }
>  
> +#ifdef CONFIG_OF
> +static struct tegra_kbc_platform_data * __devinit tegra_kbc_dt_parse_pdata(
> +	struct platform_device *pdev)
> +{
> +	struct tegra_kbc_platform_data *pdata;
> +	struct device_node *np = pdev->dev.of_node;
> +	int i;
> +	u32 prop;
> +
> +	if (!np)
> +		return NULL;
> +
> +	pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
> +
> +	if (!of_property_read_u32(np, "debounce-delay", &prop))
> +		pdata->debounce_cnt = prop;
> +
> +	if (!of_property_read_u32(np, "repeat-delay", &prop))
> +		pdata->repeat_cnt = prop;
> +
> +	if (of_find_property(np, "needs-ghost-filter", NULL))
> +		pdata->use_ghost_filter = true;
> +
> +	if (of_find_property(np, "wakeup-source", NULL))
> +		pdata->wakeup = true;
> +
> +	/* All currently known keymaps with device tree support use the same
> +	 * pin_cfg, so set it up here.
> +	 */
> +	for (i = 0; i < KBC_MAX_ROW; i++) {
> +		pdata->pin_cfg[i].num = i;
> +		pdata->pin_cfg[i].is_row = true;
> +	}
> +	
> +	for (i = 0; i < KBC_MAX_COL; i++) {
> +		pdata->pin_cfg[KBC_MAX_ROW + i].num = i;
> +		pdata->pin_cfg[KBC_MAX_ROW + i].is_row = false;
> +	}
> +
> +	return pdata;
> +}
> +#else
> +static inline struct tegra_kbc_platform_data *tegra_kbc_dt_parse_pdata(
> +	struct platform_device *pdev)
> +{
> +	return NULL;
> +}
> +#endif
> +
>  static int __devinit tegra_kbc_probe(struct platform_device *pdev)
>  {
>  	const struct tegra_kbc_platform_data *pdata = pdev->dev.platform_data;
> @@ -590,6 +640,9 @@ static int __devinit tegra_kbc_probe(struct platform_device *pdev)
>  	unsigned int scan_time_rows;
>  
>  	if (!pdata)
> +		pdata = tegra_kbc_dt_parse_pdata(pdev);
> +
> +	if (!pdata)
>  		return -EINVAL;
>  
>  	if (!tegra_kbc_check_pin_cfg(pdata, &pdev->dev, &num_rows))
> @@ -671,7 +724,7 @@ static int __devinit tegra_kbc_probe(struct platform_device *pdev)
>  
>  	kbc->use_fn_map = pdata->use_fn_map;
>  	kbc->use_ghost_filter = pdata->use_ghost_filter;
> -	keymap_data = pdata->keymap_data ?: &tegra_kbc_default_keymap_data;
> +	keymap_data = pdata->keymap_data ?: &default_keymap_data;
>  	matrix_keypad_build_keymap(keymap_data, KBC_ROW_SHIFT,
>  				   input_dev->keycode, input_dev->keybit);
>  
> @@ -793,6 +846,12 @@ static int tegra_kbc_resume(struct device *dev)
>  
>  static SIMPLE_DEV_PM_OPS(tegra_kbc_pm_ops, tegra_kbc_suspend, tegra_kbc_resume);
>  
> +static const struct of_device_id tegra_kbc_of_match[] = {
> +	{ .compatible = "nvidia,tegra20-kbc", },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, tegra_kbc_of_match);
> +
>  static struct platform_driver tegra_kbc_driver = {
>  	.probe		= tegra_kbc_probe,
>  	.remove		= __devexit_p(tegra_kbc_remove),
> @@ -800,6 +859,7 @@ static struct platform_driver tegra_kbc_driver = {
>  		.name	= "tegra-kbc",
>  		.owner  = THIS_MODULE,
>  		.pm	= &tegra_kbc_pm_ops,
> +		.of_match_table = tegra_kbc_of_match,
>  	},
>  };
>  
> -- 
> 1.7.8.GIT
> 

[PATCH v2 1/2] Input: tegra-kbc - add device tree bindings

[Olof Johansson]

On Mon, Jan 2, 2012 at 12:18 AM, Grant Likely <grant.likely@secretlab.ca> wrote:

> I'd like to see a "tegra," prefix on these custom properties, but otherwise
> the binding looks okay to me. ?You should also consider a '-ms' suffix on
> the millisecond properties.

Ok, I added "nvidia," prefix for the properties, and pushed down the
linux,fn-keymap binding to this level (but I left the implementation
as a FIXME).

> Otherwise;
>
> Acked-by: Grant Likely <grant.likely@secretlab.ca>

Thanks, added and I'll repost the patch together with the shared
binding in a bit.


-Olof