[PATCH V2 0/4] input: keyboard: tegra: cleanups and DT supports

[PATCH V2 0/4] input: keyboard: tegra: cleanups and DT supports

[Laxman Dewangan]

This patch series:
 - fix build warning,
 - use devm_* for allocation,
 - make column/rows configuration through DT and 
 - remove the rarely used  key mapping table.

Changes from V1:
- renames the rows and pins property array.
- nit cleanups.

Laxman Dewangan (4):
  input: keyboard: tegra: fix build warning
  input: keyboard: tegra: use devm_* for resource allocation
  input: keyboard: tegra: add support for rows/cols configuration from
    dt
  input: keyboard: tegra: remove default key mapping

 .../bindings/input/nvidia,tegra20-kbc.txt          |   22 ++
 drivers/input/keyboard/tegra-kbc.c                 |  345 ++++++--------------
 2 files changed, 118 insertions(+), 249 deletions(-)

[PATCH v2 1/4] input: keyboard: tegra: fix build warning

[Laxman Dewangan]

Fix the following build warning when building driver with CONFIG_PM_SLEEP
not selected.

tegra-kbc.c:360:13: warning: 'tegra_kbc_set_keypress_interrupt' defined but not used [-Wunused-function]

Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
---
Changes form V1:
- none

 drivers/input/keyboard/tegra-kbc.c |   24 ++++++++++++------------
 1 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/input/keyboard/tegra-kbc.c b/drivers/input/keyboard/tegra-kbc.c
index c76f968..f1d3ba0 100644
--- a/drivers/input/keyboard/tegra-kbc.c
+++ b/drivers/input/keyboard/tegra-kbc.c
@@ -357,18 +357,6 @@ static void tegra_kbc_set_fifo_interrupt(struct tegra_kbc *kbc, bool enable)
 	writel(val, kbc->mmio + KBC_CONTROL_0);
 }
 
-static void tegra_kbc_set_keypress_interrupt(struct tegra_kbc *kbc, bool enable)
-{
-	u32 val;
-
-	val = readl(kbc->mmio + KBC_CONTROL_0);
-	if (enable)
-		val |= KBC_CONTROL_KEYPRESS_INT_EN;
-	else
-		val &= ~KBC_CONTROL_KEYPRESS_INT_EN;
-	writel(val, kbc->mmio + KBC_CONTROL_0);
-}
-
 static void tegra_kbc_keypress_timer(unsigned long data)
 {
 	struct tegra_kbc *kbc = (struct tegra_kbc *)data;
@@ -866,6 +854,18 @@ static int tegra_kbc_remove(struct platform_device *pdev)
 }
 
 #ifdef CONFIG_PM_SLEEP
+static void tegra_kbc_set_keypress_interrupt(struct tegra_kbc *kbc, bool enable)
+{
+	u32 val;
+
+	val = readl(kbc->mmio + KBC_CONTROL_0);
+	if (enable)
+		val |= KBC_CONTROL_KEYPRESS_INT_EN;
+	else
+		val &= ~KBC_CONTROL_KEYPRESS_INT_EN;
+	writel(val, kbc->mmio + KBC_CONTROL_0);
+}
+
 static int tegra_kbc_suspend(struct device *dev)
 {
 	struct platform_device *pdev = to_platform_device(dev);
-- 
1.7.1.1

[PATCH v2 2/4] input: keyboard: tegra: use devm_* for resource allocation

[Laxman Dewangan]

Use devm_* for memory, clock, input device allocation. This reduces
code for freeing these resources.

Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
---
Changes from V1:
None

 drivers/input/keyboard/tegra-kbc.c |   93 +++++++++++-------------------------
 1 files changed, 28 insertions(+), 65 deletions(-)

diff --git a/drivers/input/keyboard/tegra-kbc.c b/drivers/input/keyboard/tegra-kbc.c
index f1d3ba0..c036425 100644
--- a/drivers/input/keyboard/tegra-kbc.c
+++ b/drivers/input/keyboard/tegra-kbc.c
@@ -618,7 +618,7 @@ static struct tegra_kbc_platform_data *tegra_kbc_dt_parse_pdata(
 	if (!np)
 		return NULL;
 
-	pdata = kzalloc(sizeof(*pdata), GFP_KERNEL);
+	pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
 	if (!pdata)
 		return NULL;
 
@@ -700,33 +700,36 @@ static int tegra_kbc_probe(struct platform_device *pdev)
 	if (!pdata)
 		pdata = tegra_kbc_dt_parse_pdata(pdev);
 
-	if (!pdata)
+	if (!pdata) {
+		dev_err(&pdev->dev, "Platform data missing\n");
 		return -EINVAL;
-
-	if (!tegra_kbc_check_pin_cfg(pdata, &pdev->dev, &num_rows)) {
-		err = -EINVAL;
-		goto err_free_pdata;
 	}
 
+	if (!tegra_kbc_check_pin_cfg(pdata, &pdev->dev, &num_rows))
+		return -EINVAL;
+
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	if (!res) {
 		dev_err(&pdev->dev, "failed to get I/O memory\n");
-		err = -ENXIO;
-		goto err_free_pdata;
+		return -ENXIO;
 	}
 
 	irq = platform_get_irq(pdev, 0);
 	if (irq < 0) {
 		dev_err(&pdev->dev, "failed to get keyboard IRQ\n");
-		err = -ENXIO;
-		goto err_free_pdata;
+		return -ENXIO;
+	}
+
+	kbc = devm_kzalloc(&pdev->dev, sizeof(*kbc), GFP_KERNEL);
+	if (!kbc) {
+		dev_err(&pdev->dev, "failed to alloc memory for kbc\n");
+		return -ENOMEM;
 	}
 
-	kbc = kzalloc(sizeof(*kbc), GFP_KERNEL);
-	input_dev = input_allocate_device();
-	if (!kbc || !input_dev) {
-		err = -ENOMEM;
-		goto err_free_mem;
+	input_dev = devm_input_allocate_device(&pdev->dev);
+	if (!input_dev) {
+		dev_err(&pdev->dev, "failed to allocate input device\n");
+		return -ENOMEM;
 	}
 
 	kbc->pdata = pdata;
@@ -735,25 +738,16 @@ static int tegra_kbc_probe(struct platform_device *pdev)
 	spin_lock_init(&kbc->lock);
 	setup_timer(&kbc->timer, tegra_kbc_keypress_timer, (unsigned long)kbc);
 
-	res = request_mem_region(res->start, resource_size(res), pdev->name);
-	if (!res) {
-		dev_err(&pdev->dev, "failed to request I/O memory\n");
-		err = -EBUSY;
-		goto err_free_mem;
-	}
-
-	kbc->mmio = ioremap(res->start, resource_size(res));
+	kbc->mmio = devm_request_and_ioremap(&pdev->dev, res);
 	if (!kbc->mmio) {
-		dev_err(&pdev->dev, "failed to remap I/O memory\n");
-		err = -ENXIO;
-		goto err_free_mem_region;
+		dev_err(&pdev->dev, "Cannot request memregion/iomap address\n");
+		return -EADDRNOTAVAIL;
 	}
 
-	kbc->clk = clk_get(&pdev->dev, NULL);
+	kbc->clk = devm_clk_get(&pdev->dev, NULL);
 	if (IS_ERR(kbc->clk)) {
 		dev_err(&pdev->dev, "failed to get keyboard clock\n");
-		err = PTR_ERR(kbc->clk);
-		goto err_iounmap;
+		return PTR_ERR(kbc->clk);
 	}
 
 	/*
@@ -778,9 +772,9 @@ static int tegra_kbc_probe(struct platform_device *pdev)
 	input_dev->close = tegra_kbc_close;
 
 	err = tegra_kbd_setup_keymap(kbc);
-	if (err) {
+	if (err < 0) {
 		dev_err(&pdev->dev, "failed to setup keymap\n");
-		goto err_put_clk;
+		return err;
 	}
 
 	__set_bit(EV_REP, input_dev->evbit);
@@ -790,15 +784,15 @@ static int tegra_kbc_probe(struct platform_device *pdev)
 
 	err = request_irq(kbc->irq, tegra_kbc_isr,
 			  IRQF_NO_SUSPEND | IRQF_TRIGGER_HIGH, pdev->name, kbc);
-	if (err) {
+	if (err < 0) {
 		dev_err(&pdev->dev, "failed to request keyboard IRQ\n");
-		goto err_put_clk;
+		return err;
 	}
 
 	disable_irq(kbc->irq);
 
 	err = input_register_device(kbc->idev);
-	if (err) {
+	if (err < 0) {
 		dev_err(&pdev->dev, "failed to register input device\n");
 		goto err_free_irq;
 	}
@@ -810,46 +804,15 @@ static int tegra_kbc_probe(struct platform_device *pdev)
 
 err_free_irq:
 	free_irq(kbc->irq, pdev);
-err_put_clk:
-	clk_put(kbc->clk);
-err_iounmap:
-	iounmap(kbc->mmio);
-err_free_mem_region:
-	release_mem_region(res->start, resource_size(res));
-err_free_mem:
-	input_free_device(input_dev);
-	kfree(kbc);
-err_free_pdata:
-	if (!pdev->dev.platform_data)
-		kfree(pdata);
-
 	return err;
 }
 
 static int 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);
-
 	input_unregister_device(kbc->idev);
-	iounmap(kbc->mmio);
-	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	release_mem_region(res->start, resource_size(res));
-
-	/*
-	 * 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);
-
-	kfree(kbc);
-
 	return 0;
 }
 
-- 
1.7.1.1

[PATCH v2] input: keyboard: tegra: add support for rows/cols configuration from dt

[Laxman Dewangan]

The NVIDIA's Tegra KBC has maximum 24 pins to make matrix keypad.
Any pin can be configured as row or column. The maximum column pin
can be 8 and maximum row pin can be 16.

Remove the assumption that all first 16 pins will be used as row
and remaining as columns and Add the property for configuring pins
to either row or column from DT. Update the devicetree binding
document accordingly.

Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
---
Changes from V1:
- renames kbc-rows and kbc-cols to kbc-row-pins and kbc-col-pins.
- cleanusp.

 .../bindings/input/nvidia,tegra20-kbc.txt          |   22 ++++++
 drivers/input/keyboard/tegra-kbc.c                 |   74 +++++++++++++++-----
 2 files changed, 79 insertions(+), 17 deletions(-)

diff --git a/Documentation/devicetree/bindings/input/nvidia,tegra20-kbc.txt b/Documentation/devicetree/bindings/input/nvidia,tegra20-kbc.txt
index 72683be..2995fae 100644
--- a/Documentation/devicetree/bindings/input/nvidia,tegra20-kbc.txt
+++ b/Documentation/devicetree/bindings/input/nvidia,tegra20-kbc.txt
@@ -1,7 +1,18 @@
 * Tegra keyboard controller
+The key controller has maximum 24 pins to make matrix keypad. Any pin
+can be configured as row or column. The maximum column pin can be 8
+and maximum row pins can be 16 for Tegra20/Tegra30.
 
 Required properties:
 - compatible: "nvidia,tegra20-kbc"
+- reg: Register base address of KBC.
+- interrupts: Interrupt number for the KBC.
+- nvidia,kbc-row-pins: The KBC pins which are configured as row. This is an
+  array of pin numbers which is used as rows.
+- nvidia,kbc-col-pins: The KBC pins which are configured as column. This is an
+  array of pin numbers which is used as column.
+- linux,keymap: The keymap for keys as described in the binding document
+  devicetree/bindings/input/matrix-keymap.txt.
 
 Optional properties, in addition to those specified by the shared
 matrix-keyboard bindings:
@@ -19,5 +30,16 @@ Example:
 keyboard: keyboard {
 	compatible = "nvidia,tegra20-kbc";
 	reg = <0x7000e200 0x100>;
+	interrupts = <0 85 0x04>;
 	nvidia,ghost-filter;
+	nvidia,debounce-delay-ms = <640>;
+	nvidia,kbc-row-pins = <0 1 2>;    /* pin 0, 1, 2 as rows */
+	nvidia,kbc-col-pins = <11 12 13>; /* pin 11, 12, 13 as columns */
+	linux,keymap = <0x00000074
+			0x00010067
+			0x00020066
+			0x01010068
+			0x02000069
+			0x02010070
+			0x02020071>;
 };
diff --git a/drivers/input/keyboard/tegra-kbc.c b/drivers/input/keyboard/tegra-kbc.c
index c036425..b65971d 100644
--- a/drivers/input/keyboard/tegra-kbc.c
+++ b/drivers/input/keyboard/tegra-kbc.c
@@ -614,13 +614,16 @@ static struct tegra_kbc_platform_data *tegra_kbc_dt_parse_pdata(
 	struct device_node *np = pdev->dev.of_node;
 	u32 prop;
 	int i;
-
-	if (!np)
-		return NULL;
+	u32 num_rows = 0;
+	u32 num_cols = 0;
+	u32 cols_cfg[KBC_MAX_GPIO];
+	u32 rows_cfg[KBC_MAX_GPIO];
+	int proplen;
+	int ret;
 
 	pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
 	if (!pdata)
-		return NULL;
+		return ERR_PTR(-ENOMEM);
 
 	if (!of_property_read_u32(np, "nvidia,debounce-delay-ms", &prop))
 		pdata->debounce_cnt = prop;
@@ -634,18 +637,55 @@ static struct tegra_kbc_platform_data *tegra_kbc_dt_parse_pdata(
 	if (of_find_property(np, "nvidia,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].type = PIN_CFG_ROW;
+	if (!of_get_property(np, "nvidia,kbc-row-pins", &proplen)) {
+		dev_err(&pdev->dev, "property nvidia,kbc-row-pins not found\n");
+		return ERR_PTR(-ENOENT);
+	}
+	num_rows = proplen / sizeof(u32);
+
+	if (!of_get_property(np, "nvidia,kbc-col-pins", &proplen)) {
+		dev_err(&pdev->dev, "property nvidia,kbc-col-pins not found\n");
+		return ERR_PTR(-ENOENT);
+	}
+	num_cols = proplen / sizeof(u32);
+
+	if (!of_get_property(np, "linux,keymap", &proplen)) {
+		dev_err(&pdev->dev, "property linux,keymap not found\n");
+		return ERR_PTR(-ENOENT);
+	}
+
+	if (!num_rows || !num_cols || ((num_rows + num_cols) > KBC_MAX_GPIO)) {
+		dev_err(&pdev->dev,
+			"keypad rows/columns not porperly specified\n");
+		return ERR_PTR(-EINVAL);
 	}
 
-	for (i = 0; i < KBC_MAX_COL; i++) {
-		pdata->pin_cfg[KBC_MAX_ROW + i].num = i;
-		pdata->pin_cfg[KBC_MAX_ROW + i].type = PIN_CFG_COL;
+	/* Set all pins as non-configured */
+	for (i = 0; i < KBC_MAX_GPIO; i++)
+		pdata->pin_cfg[i].type = PIN_CFG_IGNORE;
+
+	ret = of_property_read_u32_array(np, "nvidia,kbc-row-pins",
+				rows_cfg, num_rows);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "Rows configurations are not proper\n");
+		return ERR_PTR(-ENOENT);
+	}
+
+	ret = of_property_read_u32_array(np, "nvidia,kbc-col-pins",
+				cols_cfg, num_cols);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "Cols configurations are not proper\n");
+		return ERR_PTR(-ENOENT);
+	}
+
+	for (i = 0; i < num_rows; i++) {
+		pdata->pin_cfg[rows_cfg[i]].type = PIN_CFG_ROW;
+		pdata->pin_cfg[rows_cfg[i]].num = i;
+	}
+
+	for (i = 0; i < num_cols; i++) {
+		pdata->pin_cfg[cols_cfg[i]].type = PIN_CFG_COL;
+		pdata->pin_cfg[cols_cfg[i]].num = i;
 	}
 
 	return pdata;
@@ -697,12 +737,12 @@ static int tegra_kbc_probe(struct platform_device *pdev)
 	unsigned int debounce_cnt;
 	unsigned int scan_time_rows;
 
-	if (!pdata)
+	if (!pdata && pdev->dev.of_node)
 		pdata = tegra_kbc_dt_parse_pdata(pdev);
 
-	if (!pdata) {
+	if (IS_ERR_OR_NULL(pdata)) {
 		dev_err(&pdev->dev, "Platform data missing\n");
-		return -EINVAL;
+		return pdata ? PTR_ERR(pdata) : -EINVAL;
 	}
 
 	if (!tegra_kbc_check_pin_cfg(pdata, &pdev->dev, &num_rows))
-- 
1.7.1.1

[PATCH v2 4/4] input: keyboard: tegra: remove default key mapping

[Laxman Dewangan]

Tegra KBC driver have the default key mapping for 16x8 configuration.
The key mapping can be provided through platform data or through DT
and the mapping varies from platform to platform, hence this default
mapping is not so useful. Remove the default mapping to reduce the code
lines of the driver.

Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
---
Changes from V1:
- None

 drivers/input/keyboard/tegra-kbc.c |  156 +-----------------------------------
 1 files changed, 1 insertions(+), 155 deletions(-)

diff --git a/drivers/input/keyboard/tegra-kbc.c b/drivers/input/keyboard/tegra-kbc.c
index ef7a0ac..c6e4985 100644
--- a/drivers/input/keyboard/tegra-kbc.c
+++ b/drivers/input/keyboard/tegra-kbc.c
@@ -87,147 +87,6 @@ struct tegra_kbc {
 	struct clk *clk;
 };
 
-static const u32 tegra_kbc_default_keymap[] = {
-	KEY(0, 2, KEY_W),
-	KEY(0, 3, KEY_S),
-	KEY(0, 4, KEY_A),
-	KEY(0, 5, KEY_Z),
-	KEY(0, 7, KEY_FN),
-
-	KEY(1, 7, KEY_LEFTMETA),
-
-	KEY(2, 6, KEY_RIGHTALT),
-	KEY(2, 7, KEY_LEFTALT),
-
-	KEY(3, 0, KEY_5),
-	KEY(3, 1, KEY_4),
-	KEY(3, 2, KEY_R),
-	KEY(3, 3, KEY_E),
-	KEY(3, 4, KEY_F),
-	KEY(3, 5, KEY_D),
-	KEY(3, 6, KEY_X),
-
-	KEY(4, 0, KEY_7),
-	KEY(4, 1, KEY_6),
-	KEY(4, 2, KEY_T),
-	KEY(4, 3, KEY_H),
-	KEY(4, 4, KEY_G),
-	KEY(4, 5, KEY_V),
-	KEY(4, 6, KEY_C),
-	KEY(4, 7, KEY_SPACE),
-
-	KEY(5, 0, KEY_9),
-	KEY(5, 1, KEY_8),
-	KEY(5, 2, KEY_U),
-	KEY(5, 3, KEY_Y),
-	KEY(5, 4, KEY_J),
-	KEY(5, 5, KEY_N),
-	KEY(5, 6, KEY_B),
-	KEY(5, 7, KEY_BACKSLASH),
-
-	KEY(6, 0, KEY_MINUS),
-	KEY(6, 1, KEY_0),
-	KEY(6, 2, KEY_O),
-	KEY(6, 3, KEY_I),
-	KEY(6, 4, KEY_L),
-	KEY(6, 5, KEY_K),
-	KEY(6, 6, KEY_COMMA),
-	KEY(6, 7, KEY_M),
-
-	KEY(7, 1, KEY_EQUAL),
-	KEY(7, 2, KEY_RIGHTBRACE),
-	KEY(7, 3, KEY_ENTER),
-	KEY(7, 7, KEY_MENU),
-
-	KEY(8, 4, KEY_RIGHTSHIFT),
-	KEY(8, 5, KEY_LEFTSHIFT),
-
-	KEY(9, 5, KEY_RIGHTCTRL),
-	KEY(9, 7, KEY_LEFTCTRL),
-
-	KEY(11, 0, KEY_LEFTBRACE),
-	KEY(11, 1, KEY_P),
-	KEY(11, 2, KEY_APOSTROPHE),
-	KEY(11, 3, KEY_SEMICOLON),
-	KEY(11, 4, KEY_SLASH),
-	KEY(11, 5, KEY_DOT),
-
-	KEY(12, 0, KEY_F10),
-	KEY(12, 1, KEY_F9),
-	KEY(12, 2, KEY_BACKSPACE),
-	KEY(12, 3, KEY_3),
-	KEY(12, 4, KEY_2),
-	KEY(12, 5, KEY_UP),
-	KEY(12, 6, KEY_PRINT),
-	KEY(12, 7, KEY_PAUSE),
-
-	KEY(13, 0, KEY_INSERT),
-	KEY(13, 1, KEY_DELETE),
-	KEY(13, 3, KEY_PAGEUP),
-	KEY(13, 4, KEY_PAGEDOWN),
-	KEY(13, 5, KEY_RIGHT),
-	KEY(13, 6, KEY_DOWN),
-	KEY(13, 7, KEY_LEFT),
-
-	KEY(14, 0, KEY_F11),
-	KEY(14, 1, KEY_F12),
-	KEY(14, 2, KEY_F8),
-	KEY(14, 3, KEY_Q),
-	KEY(14, 4, KEY_F4),
-	KEY(14, 5, KEY_F3),
-	KEY(14, 6, KEY_1),
-	KEY(14, 7, KEY_F7),
-
-	KEY(15, 0, KEY_ESC),
-	KEY(15, 1, KEY_GRAVE),
-	KEY(15, 2, KEY_F5),
-	KEY(15, 3, KEY_TAB),
-	KEY(15, 4, KEY_F1),
-	KEY(15, 5, KEY_F2),
-	KEY(15, 6, KEY_CAPSLOCK),
-	KEY(15, 7, KEY_F6),
-
-	/* Software Handled Function Keys */
-	KEY(20, 0, KEY_KP7),
-
-	KEY(21, 0, KEY_KP9),
-	KEY(21, 1, KEY_KP8),
-	KEY(21, 2, KEY_KP4),
-	KEY(21, 4, KEY_KP1),
-
-	KEY(22, 1, KEY_KPSLASH),
-	KEY(22, 2, KEY_KP6),
-	KEY(22, 3, KEY_KP5),
-	KEY(22, 4, KEY_KP3),
-	KEY(22, 5, KEY_KP2),
-	KEY(22, 7, KEY_KP0),
-
-	KEY(27, 1, KEY_KPASTERISK),
-	KEY(27, 3, KEY_KPMINUS),
-	KEY(27, 4, KEY_KPPLUS),
-	KEY(27, 5, KEY_KPDOT),
-
-	KEY(28, 5, KEY_VOLUMEUP),
-
-	KEY(29, 3, KEY_HOME),
-	KEY(29, 4, KEY_END),
-	KEY(29, 5, KEY_BRIGHTNESSDOWN),
-	KEY(29, 6, KEY_VOLUMEDOWN),
-	KEY(29, 7, KEY_BRIGHTNESSUP),
-
-	KEY(30, 0, KEY_NUMLOCK),
-	KEY(30, 1, KEY_SCROLLLOCK),
-	KEY(30, 2, KEY_MUTE),
-
-	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 void tegra_kbc_report_released_keys(struct input_dev *input,
 					   unsigned short old_keycodes[],
 					   unsigned int old_num_keys,
@@ -701,26 +560,13 @@ static int tegra_kbd_setup_keymap(struct tegra_kbc *kbc)
 	const struct tegra_kbc_platform_data *pdata = kbc->pdata;
 	const struct matrix_keymap_data *keymap_data = pdata->keymap_data;
 	unsigned int keymap_rows = KBC_MAX_KEY;
-	int retval;
 
 	if (keymap_data && pdata->use_fn_map)
 		keymap_rows *= 2;
 
-	retval = matrix_keypad_build_keymap(keymap_data, NULL,
+	return matrix_keypad_build_keymap(keymap_data, NULL,
 					    keymap_rows, KBC_MAX_COL,
 					    kbc->keycode, kbc->idev);
-	if (retval == -ENOSYS || retval == -ENOENT) {
-		/*
-		 * If there is no OF support in kernel or keymap
-		 * property is missing, use default keymap.
-		 */
-		retval = matrix_keypad_build_keymap(
-					&tegra_kbc_default_keymap_data, NULL,
-					keymap_rows, KBC_MAX_COL,
-					kbc->keycode, kbc->idev);
-	}
-
-	return retval;
 }
 
 static int tegra_kbc_probe(struct platform_device *pdev)
-- 
1.7.1.1

Re: [PATCH v2 2/4] input: keyboard: tegra: use devm_* for resource allocation

[Dmitry Torokhov]

Hi Laxman,

On Sat, Jan 05, 2013 at 01:15:08PM +0530, Laxman Dewangan wrote:
> Use devm_* for memory, clock, input device allocation. This reduces
> code for freeing these resources.
> 
> Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
> ---
> Changes from V1:
> None
> 
>  drivers/input/keyboard/tegra-kbc.c |   93 +++++++++++-------------------------
>  1 files changed, 28 insertions(+), 65 deletions(-)
> 
> diff --git a/drivers/input/keyboard/tegra-kbc.c b/drivers/input/keyboard/tegra-kbc.c
> index f1d3ba0..c036425 100644
> --- a/drivers/input/keyboard/tegra-kbc.c
> +++ b/drivers/input/keyboard/tegra-kbc.c
> @@ -618,7 +618,7 @@ static struct tegra_kbc_platform_data *tegra_kbc_dt_parse_pdata(
>  	if (!np)
>  		return NULL;
>  
> -	pdata = kzalloc(sizeof(*pdata), GFP_KERNEL);
> +	pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
>  	if (!pdata)
>  		return NULL;
>  
> @@ -700,33 +700,36 @@ static int tegra_kbc_probe(struct platform_device *pdev)
>  	if (!pdata)
>  		pdata = tegra_kbc_dt_parse_pdata(pdev);
>  
> -	if (!pdata)
> +	if (!pdata) {
> +		dev_err(&pdev->dev, "Platform data missing\n");
>  		return -EINVAL;
> -
> -	if (!tegra_kbc_check_pin_cfg(pdata, &pdev->dev, &num_rows)) {
> -		err = -EINVAL;
> -		goto err_free_pdata;
>  	}
>  
> +	if (!tegra_kbc_check_pin_cfg(pdata, &pdev->dev, &num_rows))
> +		return -EINVAL;
> +
>  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  	if (!res) {
>  		dev_err(&pdev->dev, "failed to get I/O memory\n");
> -		err = -ENXIO;
> -		goto err_free_pdata;
> +		return -ENXIO;
>  	}
>  
>  	irq = platform_get_irq(pdev, 0);
>  	if (irq < 0) {
>  		dev_err(&pdev->dev, "failed to get keyboard IRQ\n");
> -		err = -ENXIO;
> -		goto err_free_pdata;
> +		return -ENXIO;
> +	}
> +
> +	kbc = devm_kzalloc(&pdev->dev, sizeof(*kbc), GFP_KERNEL);
> +	if (!kbc) {
> +		dev_err(&pdev->dev, "failed to alloc memory for kbc\n");
> +		return -ENOMEM;
>  	}
>  
> -	kbc = kzalloc(sizeof(*kbc), GFP_KERNEL);
> -	input_dev = input_allocate_device();
> -	if (!kbc || !input_dev) {
> -		err = -ENOMEM;
> -		goto err_free_mem;
> +	input_dev = devm_input_allocate_device(&pdev->dev);
> +	if (!input_dev) {
> +		dev_err(&pdev->dev, "failed to allocate input device\n");
> +		return -ENOMEM;
>  	}
>  
>  	kbc->pdata = pdata;
> @@ -735,25 +738,16 @@ static int tegra_kbc_probe(struct platform_device *pdev)
>  	spin_lock_init(&kbc->lock);
>  	setup_timer(&kbc->timer, tegra_kbc_keypress_timer, (unsigned long)kbc);
>  
> -	res = request_mem_region(res->start, resource_size(res), pdev->name);
> -	if (!res) {
> -		dev_err(&pdev->dev, "failed to request I/O memory\n");
> -		err = -EBUSY;
> -		goto err_free_mem;
> -	}
> -
> -	kbc->mmio = ioremap(res->start, resource_size(res));
> +	kbc->mmio = devm_request_and_ioremap(&pdev->dev, res);
>  	if (!kbc->mmio) {
> -		dev_err(&pdev->dev, "failed to remap I/O memory\n");
> -		err = -ENXIO;
> -		goto err_free_mem_region;
> +		dev_err(&pdev->dev, "Cannot request memregion/iomap address\n");
> +		return -EADDRNOTAVAIL;

Erm, no, -EBUSY please.

>  	}
>  
> -	kbc->clk = clk_get(&pdev->dev, NULL);
> +	kbc->clk = devm_clk_get(&pdev->dev, NULL);
>  	if (IS_ERR(kbc->clk)) {
>  		dev_err(&pdev->dev, "failed to get keyboard clock\n");
> -		err = PTR_ERR(kbc->clk);
> -		goto err_iounmap;
> +		return PTR_ERR(kbc->clk);
>  	}
>  
>  	/*
> @@ -778,9 +772,9 @@ static int tegra_kbc_probe(struct platform_device *pdev)
>  	input_dev->close = tegra_kbc_close;
>  
>  	err = tegra_kbd_setup_keymap(kbc);
> -	if (err) {
> +	if (err < 0) {

Why is this change? As far as I can see tegra_kbd_setup_keymap() never
returns positive values.

>  		dev_err(&pdev->dev, "failed to setup keymap\n");
> -		goto err_put_clk;
> +		return err;
>  	}
>  
>  	__set_bit(EV_REP, input_dev->evbit);
> @@ -790,15 +784,15 @@ static int tegra_kbc_probe(struct platform_device *pdev)
>  
>  	err = request_irq(kbc->irq, tegra_kbc_isr,
>  			  IRQF_NO_SUSPEND | IRQF_TRIGGER_HIGH, pdev->name, kbc);
> -	if (err) {
> +	if (err < 0) {

Neither request_irq(). BTW, why not devm_request_irq?

>  		dev_err(&pdev->dev, "failed to request keyboard IRQ\n");
> -		goto err_put_clk;
> +		return err;
>  	}
>  
>  	disable_irq(kbc->irq);
>  
>  	err = input_register_device(kbc->idev);
> -	if (err) {
> +	if (err < 0) {

Again, input_register_device() returns 0 on success.

>  		dev_err(&pdev->dev, "failed to register input device\n");
>  		goto err_free_irq;
>  	}
> @@ -810,46 +804,15 @@ static int tegra_kbc_probe(struct platform_device *pdev)
>  
>  err_free_irq:
>  	free_irq(kbc->irq, pdev);
> -err_put_clk:
> -	clk_put(kbc->clk);
> -err_iounmap:
> -	iounmap(kbc->mmio);
> -err_free_mem_region:
> -	release_mem_region(res->start, resource_size(res));
> -err_free_mem:
> -	input_free_device(input_dev);
> -	kfree(kbc);
> -err_free_pdata:
> -	if (!pdev->dev.platform_data)
> -		kfree(pdata);
> -
>  	return err;
>  }
>  
>  static int 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);
> -
>  	input_unregister_device(kbc->idev);

You do not need to call input_unregister_device for managed input
devices, and if you switch request_irq to managed variant you can remove 
tegra_kbc_remove() altogether.

> -	iounmap(kbc->mmio);
> -	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> -	release_mem_region(res->start, resource_size(res));
> -
> -	/*
> -	 * 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);
> -
> -	kfree(kbc);
> -
>  	return 0;
>  }
>  
> -- 
> 1.7.1.1
> 

Thanks.

-- 
Dmitry

Re: [PATCH v2 2/4] input: keyboard: tegra: use devm_* for resource allocation

[Laxman Dewangan]

HI Dmitry,
Thanks for quick review.

I will take care of your comment in next version. Some have my answer.


On Saturday 05 January 2013 01:36 PM, Dmitry Torokhov wrote:
> Hi Laxman,
>
> On Sat, Jan 05, 2013 at 01:15:08PM +0530, Laxman Dewangan wrote:
>> Use devm_* for memory, clock, input device allocation. This reduces
>> code for freeing these resources.

>>   	err = tegra_kbd_setup_keymap(kbc);
>> -	if (err) {
>> +	if (err < 0) {
> Why is this change? As far as I can see tegra_kbd_setup_keymap() never
> returns positive values.

Ok, mostly errors are in negative and hence this change, I will revert 
it and will keep original.

>
>>   		dev_err(&pdev->dev, "failed to setup keymap\n");
>> -		goto err_put_clk;
>> +		return err;
>>   	}
>>   
>>   	__set_bit(EV_REP, input_dev->evbit);
>> @@ -790,15 +784,15 @@ static int tegra_kbc_probe(struct platform_device *pdev)
>>   
>>   	err = request_irq(kbc->irq, tegra_kbc_isr,
>>   			  IRQF_NO_SUSPEND | IRQF_TRIGGER_HIGH, pdev->name, kbc);
>> -	if (err) {
>> +	if (err < 0) {
> Neither request_irq(). BTW, why not devm_request_irq?

I understand from Mark B on different patches that using 
devm_request_irq() can create race condition when removing device. 
Interrupt can occur when device resource release is in process and so it 
can cause isr call which can use the freed pointer. devm_request_irq() 
should be avoided.


Thanks,
Laxman

Re: [PATCH v2 2/4] input: keyboard: tegra: use devm_* for resource allocation

[Dmitry Torokhov]

On Sat, Jan 05, 2013 at 04:50:58PM +0530, Laxman Dewangan wrote:
> HI Dmitry,
> Thanks for quick review.
> 
> I will take care of your comment in next version. Some have my answer.
> 
> 
> On Saturday 05 January 2013 01:36 PM, Dmitry Torokhov wrote:
> >Hi Laxman,
> >
> >On Sat, Jan 05, 2013 at 01:15:08PM +0530, Laxman Dewangan wrote:
> >>Use devm_* for memory, clock, input device allocation. This reduces
> >>code for freeing these resources.
> 
> >>  	err = tegra_kbd_setup_keymap(kbc);
> >>-	if (err) {
> >>+	if (err < 0) {
> >Why is this change? As far as I can see tegra_kbd_setup_keymap() never
> >returns positive values.
> 
> Ok, mostly errors are in negative and hence this change, I will
> revert it and will keep original.
> 
> >
> >>  		dev_err(&pdev->dev, "failed to setup keymap\n");
> >>-		goto err_put_clk;
> >>+		return err;
> >>  	}
> >>  	__set_bit(EV_REP, input_dev->evbit);
> >>@@ -790,15 +784,15 @@ static int tegra_kbc_probe(struct platform_device *pdev)
> >>  	err = request_irq(kbc->irq, tegra_kbc_isr,
> >>  			  IRQF_NO_SUSPEND | IRQF_TRIGGER_HIGH, pdev->name, kbc);
> >>-	if (err) {
> >>+	if (err < 0) {
> >Neither request_irq(). BTW, why not devm_request_irq?
> 
> I understand from Mark B on different patches that using
> devm_request_irq() can create race condition when removing device.
> Interrupt can occur when device resource release is in process and
> so it can cause isr call which can use the freed pointer.
> devm_request_irq() should be avoided.

devm_request_irq() has a potential of creating a race condition, but it
depents on the driver. In this particular case tegra driver ensures that
interrupts are inhibited when input device is unregistered by providing
tegra_kbc_close() method, so in this particular case it is safe to
use devm_request_irq().

Also, when using managed input devices, the unregistering and final
freeing is a 2-step process, so even in absence of close() method, if
initialization sequence was:

	devm_input_allocate_device()
	...
	devm_request_irq()
	...
	input_unregister_device()

then order of freeing resources (behind the scenes) will be

	devm_input_device_unregister();
	/* input device is still present in memory and can
	 * handle input_event() calls.
	 */
	free_irq();
	devm_input_device_release();

So using managed request_irq() _together_ with managed input devices is
OK.

Thanks.

-- 
Dmitry

Re: [PATCH v2 2/4] input: keyboard: tegra: use devm_* for resource allocation

[Laxman Dewangan]

On Sunday 06 January 2013 04:48 AM, Dmitry Torokhov wrote:
> On Sat, Jan 05, 2013 at 04:50:58PM +0530, Laxman Dewangan wrote:
>> HI Dmitry,
>> Thanks for quick review.
>>
>> I will take care of your comment in next version. Some have my answer.
>>
>>
>> On Saturday 05 January 2013 01:36 PM, Dmitry Torokhov wrote:
>>> Hi Laxman,
>>>
>>> On Sat, Jan 05, 2013 at 01:15:08PM +0530, Laxman Dewangan wrote:
>>>> Use devm_* for memory, clock, input device allocation. This reduces
>>>> code for freeing these resources.
>>>>   	err = tegra_kbd_setup_keymap(kbc);
>>>> -	if (err) {
>>>> +	if (err < 0) {
>>> Why is this change? As far as I can see tegra_kbd_setup_keymap() never
>>> returns positive values.
>> Ok, mostly errors are in negative and hence this change, I will
>> revert it and will keep original.
>>
>>>>   		dev_err(&pdev->dev, "failed to setup keymap\n");
>>>> -		goto err_put_clk;
>>>> +		return err;
>>>>   	}
>>>>   	__set_bit(EV_REP, input_dev->evbit);
>>>> @@ -790,15 +784,15 @@ static int tegra_kbc_probe(struct platform_device *pdev)
>>>>   	err = request_irq(kbc->irq, tegra_kbc_isr,
>>>>   			  IRQF_NO_SUSPEND | IRQF_TRIGGER_HIGH, pdev->name, kbc);
>>>> -	if (err) {
>>>> +	if (err < 0) {
>>> Neither request_irq(). BTW, why not devm_request_irq?
>> I understand from Mark B on different patches that using
>> devm_request_irq() can create race condition when removing device.
>> Interrupt can occur when device resource release is in process and
>> so it can cause isr call which can use the freed pointer.
>> devm_request_irq() should be avoided.
> devm_request_irq() has a potential of creating a race condition, but it
> depents on the driver. In this particular case tegra driver ensures that
> interrupts are inhibited when input device is unregistered by providing
> tegra_kbc_close() method, so in this particular case it is safe to
> use devm_request_irq().
>
> Also, when using managed input devices, the unregistering and final
> freeing is a 2-step process, so even in absence of close() method, if
> initialization sequence was:
>
> 	devm_input_allocate_device()
> 	...
> 	devm_request_irq()
> 	...
> 	input_unregister_device()
>
> then order of freeing resources (behind the scenes) will be
>
> 	devm_input_device_unregister();
> 	/* input device is still present in memory and can
> 	 * handle input_event() calls.
> 	 */
> 	free_irq();
> 	devm_input_device_release();
>
> So using managed request_irq() _together_ with managed input devices is
> OK.

Thanks for detail explanation. I will modify and sent the new version of 
patch.

Re: [PATCH v2 2/4] input: keyboard: tegra: use devm_* for resource allocation

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1154 bytes --]

On Sat, Jan 05, 2013 at 12:06:58AM -0800, Dmitry Torokhov wrote:
> On Sat, Jan 05, 2013 at 01:15:08PM +0530, Laxman Dewangan wrote:
[...]
> > @@ -735,25 +738,16 @@ static int tegra_kbc_probe(struct platform_device *pdev)
> >  	spin_lock_init(&kbc->lock);
> >  	setup_timer(&kbc->timer, tegra_kbc_keypress_timer, (unsigned long)kbc);
> >  
> > -	res = request_mem_region(res->start, resource_size(res), pdev->name);
> > -	if (!res) {
> > -		dev_err(&pdev->dev, "failed to request I/O memory\n");
> > -		err = -EBUSY;
> > -		goto err_free_mem;
> > -	}
> > -
> > -	kbc->mmio = ioremap(res->start, resource_size(res));
> > +	kbc->mmio = devm_request_and_ioremap(&pdev->dev, res);
> >  	if (!kbc->mmio) {
> > -		dev_err(&pdev->dev, "failed to remap I/O memory\n");
> > -		err = -ENXIO;
> > -		goto err_free_mem_region;
> > +		dev_err(&pdev->dev, "Cannot request memregion/iomap address\n");
> > +		return -EADDRNOTAVAIL;
> 
> Erm, no, -EBUSY please.

EADDRNOTAVAIL is the canonical error for devm_request_and_ioremap()
failure. The kerneldoc comment in lib/devres.c even gives a short
example that uses this error code.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v2 2/4] input: keyboard: tegra: use devm_* for resource allocation

[Dmitry Torokhov]

On Sun, Jan 06, 2013 at 08:27:39PM +0100, Thierry Reding wrote:
> On Sat, Jan 05, 2013 at 12:06:58AM -0800, Dmitry Torokhov wrote:
> > On Sat, Jan 05, 2013 at 01:15:08PM +0530, Laxman Dewangan wrote:
> [...]
> > > @@ -735,25 +738,16 @@ static int tegra_kbc_probe(struct platform_device *pdev)
> > >  	spin_lock_init(&kbc->lock);
> > >  	setup_timer(&kbc->timer, tegra_kbc_keypress_timer, (unsigned long)kbc);
> > >  
> > > -	res = request_mem_region(res->start, resource_size(res), pdev->name);
> > > -	if (!res) {
> > > -		dev_err(&pdev->dev, "failed to request I/O memory\n");
> > > -		err = -EBUSY;
> > > -		goto err_free_mem;
> > > -	}
> > > -
> > > -	kbc->mmio = ioremap(res->start, resource_size(res));
> > > +	kbc->mmio = devm_request_and_ioremap(&pdev->dev, res);
> > >  	if (!kbc->mmio) {
> > > -		dev_err(&pdev->dev, "failed to remap I/O memory\n");
> > > -		err = -ENXIO;
> > > -		goto err_free_mem_region;
> > > +		dev_err(&pdev->dev, "Cannot request memregion/iomap address\n");
> > > +		return -EADDRNOTAVAIL;
> > 
> > Erm, no, -EBUSY please.
> 
> EADDRNOTAVAIL is the canonical error for devm_request_and_ioremap()
> failure. The kerneldoc comment in lib/devres.c even gives a short
> example that uses this error code.

I am sorry, but I do not consider a function that was added a little
over a year ago as a canon. If you look at the uses of EADDRNOTAVAIL it
is used predominantly in networking code to indicate that attempted
_network_ address is not available.

Thanks.

-- 
Dmitry

Re: [PATCH v2 2/4] input: keyboard: tegra: use devm_* for resource allocation

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2347 bytes --]

On Sun, Jan 06, 2013 at 11:57:48AM -0800, Dmitry Torokhov wrote:
> On Sun, Jan 06, 2013 at 08:27:39PM +0100, Thierry Reding wrote:
> > On Sat, Jan 05, 2013 at 12:06:58AM -0800, Dmitry Torokhov wrote:
> > > On Sat, Jan 05, 2013 at 01:15:08PM +0530, Laxman Dewangan wrote:
> > [...]
> > > > @@ -735,25 +738,16 @@ static int tegra_kbc_probe(struct platform_device *pdev)
> > > >  	spin_lock_init(&kbc->lock);
> > > >  	setup_timer(&kbc->timer, tegra_kbc_keypress_timer, (unsigned long)kbc);
> > > >  
> > > > -	res = request_mem_region(res->start, resource_size(res), pdev->name);
> > > > -	if (!res) {
> > > > -		dev_err(&pdev->dev, "failed to request I/O memory\n");
> > > > -		err = -EBUSY;
> > > > -		goto err_free_mem;
> > > > -	}
> > > > -
> > > > -	kbc->mmio = ioremap(res->start, resource_size(res));
> > > > +	kbc->mmio = devm_request_and_ioremap(&pdev->dev, res);
> > > >  	if (!kbc->mmio) {
> > > > -		dev_err(&pdev->dev, "failed to remap I/O memory\n");
> > > > -		err = -ENXIO;
> > > > -		goto err_free_mem_region;
> > > > +		dev_err(&pdev->dev, "Cannot request memregion/iomap address\n");
> > > > +		return -EADDRNOTAVAIL;
> > > 
> > > Erm, no, -EBUSY please.
> > 
> > EADDRNOTAVAIL is the canonical error for devm_request_and_ioremap()
> > failure. The kerneldoc comment in lib/devres.c even gives a short
> > example that uses this error code.
> 
> I am sorry, but I do not consider a function that was added a little
> over a year ago as a canon. If you look at the uses of EADDRNOTAVAIL it
> is used predominantly in networking code to indicate that attempted
> _network_ address is not available.

EBUSY might be misleading, though. devm_request_and_ioremap() can fail
in both the request_mem_region() and ioremap() calls. Furthermore it'd
be good to settle on a consistent error-code instead of doing it
differently depending on subsystem and/or driver. Currently the various
error codes used are:

	EBUSY, EADDRNOTAVAIL, ENXIO, ENOMEM, ENODEV, ENOENT, EINVAL,
	EIO, EFAULT, EADDRINUSE

Also if we can settle on one error code we should follow up with a patch
to make it consistent across the tree and also update that kerneldoc
comment. I volunteer to do that if nobody else steps up. I'm also Cc'ing
Wolfram (the original author), maybe he has some thoughts on this.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v2 2/4] input: keyboard: tegra: use devm_* for resource allocation

[Dmitry Torokhov]

On Wed, Jan 09, 2013 at 08:07:45AM +0100, Thierry Reding wrote:
> On Sun, Jan 06, 2013 at 11:57:48AM -0800, Dmitry Torokhov wrote:
> > On Sun, Jan 06, 2013 at 08:27:39PM +0100, Thierry Reding wrote:
> > > On Sat, Jan 05, 2013 at 12:06:58AM -0800, Dmitry Torokhov wrote:
> > > > On Sat, Jan 05, 2013 at 01:15:08PM +0530, Laxman Dewangan wrote:
> > > [...]
> > > > > @@ -735,25 +738,16 @@ static int tegra_kbc_probe(struct platform_device *pdev)
> > > > >  	spin_lock_init(&kbc->lock);
> > > > >  	setup_timer(&kbc->timer, tegra_kbc_keypress_timer, (unsigned long)kbc);
> > > > >  
> > > > > -	res = request_mem_region(res->start, resource_size(res), pdev->name);
> > > > > -	if (!res) {
> > > > > -		dev_err(&pdev->dev, "failed to request I/O memory\n");
> > > > > -		err = -EBUSY;
> > > > > -		goto err_free_mem;
> > > > > -	}
> > > > > -
> > > > > -	kbc->mmio = ioremap(res->start, resource_size(res));
> > > > > +	kbc->mmio = devm_request_and_ioremap(&pdev->dev, res);
> > > > >  	if (!kbc->mmio) {
> > > > > -		dev_err(&pdev->dev, "failed to remap I/O memory\n");
> > > > > -		err = -ENXIO;
> > > > > -		goto err_free_mem_region;
> > > > > +		dev_err(&pdev->dev, "Cannot request memregion/iomap address\n");
> > > > > +		return -EADDRNOTAVAIL;
> > > > 
> > > > Erm, no, -EBUSY please.
> > > 
> > > EADDRNOTAVAIL is the canonical error for devm_request_and_ioremap()
> > > failure. The kerneldoc comment in lib/devres.c even gives a short
> > > example that uses this error code.
> > 
> > I am sorry, but I do not consider a function that was added a little
> > over a year ago as a canon. If you look at the uses of EADDRNOTAVAIL it
> > is used predominantly in networking code to indicate that attempted
> > _network_ address is not available.
> 
> EBUSY might be misleading, though. devm_request_and_ioremap() can fail
> in both the request_mem_region() and ioremap() calls. Furthermore it'd
> be good to settle on a consistent error-code instead of doing it
> differently depending on subsystem and/or driver. Currently the various
> error codes used are:
> 
> 	EBUSY, EADDRNOTAVAIL, ENXIO, ENOMEM, ENODEV, ENOENT, EINVAL,
> 	EIO, EFAULT, EADDRINUSE
> 
> Also if we can settle on one error code we should follow up with a patch
> to make it consistent across the tree and also update that kerneldoc
> comment. I volunteer to do that if nobody else steps up. I'm also Cc'ing
> Wolfram (the original author), maybe he has some thoughts on this.
> 

If you going to change all drivers make devm_request_and_ioremap()
return ERR_PTR()-encoded errors and then we can differentiate what
part of it failed.

Thanks.

-- 
Dmitry

Re: [PATCH v2 2/4] input: keyboard: tegra: use devm_* for resource allocation

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2983 bytes --]

On Wed, Jan 09, 2013 at 01:19:39AM -0800, Dmitry Torokhov wrote:
> On Wed, Jan 09, 2013 at 08:07:45AM +0100, Thierry Reding wrote:
> > On Sun, Jan 06, 2013 at 11:57:48AM -0800, Dmitry Torokhov wrote:
> > > On Sun, Jan 06, 2013 at 08:27:39PM +0100, Thierry Reding wrote:
> > > > On Sat, Jan 05, 2013 at 12:06:58AM -0800, Dmitry Torokhov wrote:
> > > > > On Sat, Jan 05, 2013 at 01:15:08PM +0530, Laxman Dewangan wrote:
> > > > [...]
> > > > > > @@ -735,25 +738,16 @@ static int tegra_kbc_probe(struct platform_device *pdev)
> > > > > >  	spin_lock_init(&kbc->lock);
> > > > > >  	setup_timer(&kbc->timer, tegra_kbc_keypress_timer, (unsigned long)kbc);
> > > > > >  
> > > > > > -	res = request_mem_region(res->start, resource_size(res), pdev->name);
> > > > > > -	if (!res) {
> > > > > > -		dev_err(&pdev->dev, "failed to request I/O memory\n");
> > > > > > -		err = -EBUSY;
> > > > > > -		goto err_free_mem;
> > > > > > -	}
> > > > > > -
> > > > > > -	kbc->mmio = ioremap(res->start, resource_size(res));
> > > > > > +	kbc->mmio = devm_request_and_ioremap(&pdev->dev, res);
> > > > > >  	if (!kbc->mmio) {
> > > > > > -		dev_err(&pdev->dev, "failed to remap I/O memory\n");
> > > > > > -		err = -ENXIO;
> > > > > > -		goto err_free_mem_region;
> > > > > > +		dev_err(&pdev->dev, "Cannot request memregion/iomap address\n");
> > > > > > +		return -EADDRNOTAVAIL;
> > > > > 
> > > > > Erm, no, -EBUSY please.
> > > > 
> > > > EADDRNOTAVAIL is the canonical error for devm_request_and_ioremap()
> > > > failure. The kerneldoc comment in lib/devres.c even gives a short
> > > > example that uses this error code.
> > > 
> > > I am sorry, but I do not consider a function that was added a little
> > > over a year ago as a canon. If you look at the uses of EADDRNOTAVAIL it
> > > is used predominantly in networking code to indicate that attempted
> > > _network_ address is not available.
> > 
> > EBUSY might be misleading, though. devm_request_and_ioremap() can fail
> > in both the request_mem_region() and ioremap() calls. Furthermore it'd
> > be good to settle on a consistent error-code instead of doing it
> > differently depending on subsystem and/or driver. Currently the various
> > error codes used are:
> > 
> > 	EBUSY, EADDRNOTAVAIL, ENXIO, ENOMEM, ENODEV, ENOENT, EINVAL,
> > 	EIO, EFAULT, EADDRINUSE
> > 
> > Also if we can settle on one error code we should follow up with a patch
> > to make it consistent across the tree and also update that kerneldoc
> > comment. I volunteer to do that if nobody else steps up. I'm also Cc'ing
> > Wolfram (the original author), maybe he has some thoughts on this.
> > 
> 
> If you going to change all drivers make devm_request_and_ioremap()
> return ERR_PTR()-encoded errors and then we can differentiate what
> part of it failed.

Yeah, that thought also crossed my mind. I'll give other people some
time to comment before hurling myself into preparing patches.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v2 2/4] input: keyboard: tegra: use devm_* for resource allocation

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 4207 bytes --]

On Wed, Jan 09, 2013 at 10:23:52AM +0100, Thierry Reding wrote:
> On Wed, Jan 09, 2013 at 01:19:39AM -0800, Dmitry Torokhov wrote:
> > On Wed, Jan 09, 2013 at 08:07:45AM +0100, Thierry Reding wrote:
> > > On Sun, Jan 06, 2013 at 11:57:48AM -0800, Dmitry Torokhov wrote:
> > > > On Sun, Jan 06, 2013 at 08:27:39PM +0100, Thierry Reding wrote:
> > > > > On Sat, Jan 05, 2013 at 12:06:58AM -0800, Dmitry Torokhov wrote:
> > > > > > On Sat, Jan 05, 2013 at 01:15:08PM +0530, Laxman Dewangan wrote:
> > > > > [...]
> > > > > > > @@ -735,25 +738,16 @@ static int tegra_kbc_probe(struct platform_device *pdev)
> > > > > > >  	spin_lock_init(&kbc->lock);
> > > > > > >  	setup_timer(&kbc->timer, tegra_kbc_keypress_timer, (unsigned long)kbc);
> > > > > > >  
> > > > > > > -	res = request_mem_region(res->start, resource_size(res), pdev->name);
> > > > > > > -	if (!res) {
> > > > > > > -		dev_err(&pdev->dev, "failed to request I/O memory\n");
> > > > > > > -		err = -EBUSY;
> > > > > > > -		goto err_free_mem;
> > > > > > > -	}
> > > > > > > -
> > > > > > > -	kbc->mmio = ioremap(res->start, resource_size(res));
> > > > > > > +	kbc->mmio = devm_request_and_ioremap(&pdev->dev, res);
> > > > > > >  	if (!kbc->mmio) {
> > > > > > > -		dev_err(&pdev->dev, "failed to remap I/O memory\n");
> > > > > > > -		err = -ENXIO;
> > > > > > > -		goto err_free_mem_region;
> > > > > > > +		dev_err(&pdev->dev, "Cannot request memregion/iomap address\n");
> > > > > > > +		return -EADDRNOTAVAIL;
> > > > > > 
> > > > > > Erm, no, -EBUSY please.
> > > > > 
> > > > > EADDRNOTAVAIL is the canonical error for devm_request_and_ioremap()
> > > > > failure. The kerneldoc comment in lib/devres.c even gives a short
> > > > > example that uses this error code.
> > > > 
> > > > I am sorry, but I do not consider a function that was added a little
> > > > over a year ago as a canon. If you look at the uses of EADDRNOTAVAIL it
> > > > is used predominantly in networking code to indicate that attempted
> > > > _network_ address is not available.
> > > 
> > > EBUSY might be misleading, though. devm_request_and_ioremap() can fail
> > > in both the request_mem_region() and ioremap() calls. Furthermore it'd
> > > be good to settle on a consistent error-code instead of doing it
> > > differently depending on subsystem and/or driver. Currently the various
> > > error codes used are:
> > > 
> > > 	EBUSY, EADDRNOTAVAIL, ENXIO, ENOMEM, ENODEV, ENOENT, EINVAL,
> > > 	EIO, EFAULT, EADDRINUSE
> > > 
> > > Also if we can settle on one error code we should follow up with a patch
> > > to make it consistent across the tree and also update that kerneldoc
> > > comment. I volunteer to do that if nobody else steps up. I'm also Cc'ing
> > > Wolfram (the original author), maybe he has some thoughts on this.
> > > 
> > 
> > If you going to change all drivers make devm_request_and_ioremap()
> > return ERR_PTR()-encoded errors and then we can differentiate what
> > part of it failed.
> 
> Yeah, that thought also crossed my mind. I'll give other people some
> time to comment before hurling myself into preparing patches.

I've prepared a patch that changes devm_request_and_ioremap() to return
ERR_PTR()-encoded errors and adjusts all callers. As you can imagine it
is a bit on the huge side. scripts/get_maintainer.pl lists 156 people
and mailing lists. I've gone through the list, and as far as I can tell
everyone on that list is actually affected by the patch, so there's not
much potential for tuning it down.

There is also the issue of whose tree this should go into. Unfortunately
the patch can't be broken down into smaller chunks because it changes
how the devm_request_and_ioremap() function's return value is handled in
an incompatible way, so it won't be possible to gradually phase this in.
Furthermore I can imagine that until the end of the release cycle new
code may be added on which the same transformations need to be done. I
have a semantic patch to do the bulk of the work, but quite a bit of
coordination will be required.

I'm adding Arnd and Greg on Cc, maybe they can advise on how best to
handle this kind of patch.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v2 2/4] input: keyboard: tegra: use devm_* for resource allocation

[Greg Kroah-Hartman]

On Mon, Jan 14, 2013 at 04:49:59PM +0100, Thierry Reding wrote:
> On Wed, Jan 09, 2013 at 10:23:52AM +0100, Thierry Reding wrote:
> > On Wed, Jan 09, 2013 at 01:19:39AM -0800, Dmitry Torokhov wrote:
> > > On Wed, Jan 09, 2013 at 08:07:45AM +0100, Thierry Reding wrote:
> > > > On Sun, Jan 06, 2013 at 11:57:48AM -0800, Dmitry Torokhov wrote:
> > > > > On Sun, Jan 06, 2013 at 08:27:39PM +0100, Thierry Reding wrote:
> > > > > > On Sat, Jan 05, 2013 at 12:06:58AM -0800, Dmitry Torokhov wrote:
> > > > > > > On Sat, Jan 05, 2013 at 01:15:08PM +0530, Laxman Dewangan wrote:
> > > > > > [...]
> > > > > > > > @@ -735,25 +738,16 @@ static int tegra_kbc_probe(struct platform_device *pdev)
> > > > > > > >  	spin_lock_init(&kbc->lock);
> > > > > > > >  	setup_timer(&kbc->timer, tegra_kbc_keypress_timer, (unsigned long)kbc);
> > > > > > > >  
> > > > > > > > -	res = request_mem_region(res->start, resource_size(res), pdev->name);
> > > > > > > > -	if (!res) {
> > > > > > > > -		dev_err(&pdev->dev, "failed to request I/O memory\n");
> > > > > > > > -		err = -EBUSY;
> > > > > > > > -		goto err_free_mem;
> > > > > > > > -	}
> > > > > > > > -
> > > > > > > > -	kbc->mmio = ioremap(res->start, resource_size(res));
> > > > > > > > +	kbc->mmio = devm_request_and_ioremap(&pdev->dev, res);
> > > > > > > >  	if (!kbc->mmio) {
> > > > > > > > -		dev_err(&pdev->dev, "failed to remap I/O memory\n");
> > > > > > > > -		err = -ENXIO;
> > > > > > > > -		goto err_free_mem_region;
> > > > > > > > +		dev_err(&pdev->dev, "Cannot request memregion/iomap address\n");
> > > > > > > > +		return -EADDRNOTAVAIL;
> > > > > > > 
> > > > > > > Erm, no, -EBUSY please.
> > > > > > 
> > > > > > EADDRNOTAVAIL is the canonical error for devm_request_and_ioremap()
> > > > > > failure. The kerneldoc comment in lib/devres.c even gives a short
> > > > > > example that uses this error code.
> > > > > 
> > > > > I am sorry, but I do not consider a function that was added a little
> > > > > over a year ago as a canon. If you look at the uses of EADDRNOTAVAIL it
> > > > > is used predominantly in networking code to indicate that attempted
> > > > > _network_ address is not available.
> > > > 
> > > > EBUSY might be misleading, though. devm_request_and_ioremap() can fail
> > > > in both the request_mem_region() and ioremap() calls. Furthermore it'd
> > > > be good to settle on a consistent error-code instead of doing it
> > > > differently depending on subsystem and/or driver. Currently the various
> > > > error codes used are:
> > > > 
> > > > 	EBUSY, EADDRNOTAVAIL, ENXIO, ENOMEM, ENODEV, ENOENT, EINVAL,
> > > > 	EIO, EFAULT, EADDRINUSE
> > > > 
> > > > Also if we can settle on one error code we should follow up with a patch
> > > > to make it consistent across the tree and also update that kerneldoc
> > > > comment. I volunteer to do that if nobody else steps up. I'm also Cc'ing
> > > > Wolfram (the original author), maybe he has some thoughts on this.
> > > > 
> > > 
> > > If you going to change all drivers make devm_request_and_ioremap()
> > > return ERR_PTR()-encoded errors and then we can differentiate what
> > > part of it failed.
> > 
> > Yeah, that thought also crossed my mind. I'll give other people some
> > time to comment before hurling myself into preparing patches.
> 
> I've prepared a patch that changes devm_request_and_ioremap() to return
> ERR_PTR()-encoded errors and adjusts all callers. As you can imagine it
> is a bit on the huge side. scripts/get_maintainer.pl lists 156 people
> and mailing lists. I've gone through the list, and as far as I can tell
> everyone on that list is actually affected by the patch, so there's not
> much potential for tuning it down.
> 
> There is also the issue of whose tree this should go into. Unfortunately
> the patch can't be broken down into smaller chunks because it changes
> how the devm_request_and_ioremap() function's return value is handled in
> an incompatible way, so it won't be possible to gradually phase this in.
> Furthermore I can imagine that until the end of the release cycle new
> code may be added on which the same transformations need to be done. I
> have a semantic patch to do the bulk of the work, but quite a bit of
> coordination will be required.
> 
> I'm adding Arnd and Greg on Cc, maybe they can advise on how best to
> handle this kind of patch.

You should provide a "wrapper" function that does the correct return
value, convert drivers over to it, and then, when everyone is changed,
drop the old call.  To change 156 different drivers all at once, in a
way that is not detectable by the compiler breaking the build, is not a
good thing to do at all.

By doing it in this manner, it will take longer, but you can push the
patches through the different maintainer's trees.  If they are slow,
I'll be glad to take the remaining patches in my driver-core tree to do
the final bits.

Hope this helps,

greg k-h

Re: [PATCH v2 2/4] input: keyboard: tegra: use devm_* for resource allocation

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 5465 bytes --]

On Mon, Jan 14, 2013 at 08:16:44AM -0800, Greg Kroah-Hartman wrote:
> On Mon, Jan 14, 2013 at 04:49:59PM +0100, Thierry Reding wrote:
> > On Wed, Jan 09, 2013 at 10:23:52AM +0100, Thierry Reding wrote:
> > > On Wed, Jan 09, 2013 at 01:19:39AM -0800, Dmitry Torokhov wrote:
> > > > On Wed, Jan 09, 2013 at 08:07:45AM +0100, Thierry Reding wrote:
> > > > > On Sun, Jan 06, 2013 at 11:57:48AM -0800, Dmitry Torokhov wrote:
> > > > > > On Sun, Jan 06, 2013 at 08:27:39PM +0100, Thierry Reding wrote:
> > > > > > > On Sat, Jan 05, 2013 at 12:06:58AM -0800, Dmitry Torokhov wrote:
> > > > > > > > On Sat, Jan 05, 2013 at 01:15:08PM +0530, Laxman Dewangan wrote:
> > > > > > > [...]
> > > > > > > > > @@ -735,25 +738,16 @@ static int tegra_kbc_probe(struct platform_device *pdev)
> > > > > > > > >  	spin_lock_init(&kbc->lock);
> > > > > > > > >  	setup_timer(&kbc->timer, tegra_kbc_keypress_timer, (unsigned long)kbc);
> > > > > > > > >  
> > > > > > > > > -	res = request_mem_region(res->start, resource_size(res), pdev->name);
> > > > > > > > > -	if (!res) {
> > > > > > > > > -		dev_err(&pdev->dev, "failed to request I/O memory\n");
> > > > > > > > > -		err = -EBUSY;
> > > > > > > > > -		goto err_free_mem;
> > > > > > > > > -	}
> > > > > > > > > -
> > > > > > > > > -	kbc->mmio = ioremap(res->start, resource_size(res));
> > > > > > > > > +	kbc->mmio = devm_request_and_ioremap(&pdev->dev, res);
> > > > > > > > >  	if (!kbc->mmio) {
> > > > > > > > > -		dev_err(&pdev->dev, "failed to remap I/O memory\n");
> > > > > > > > > -		err = -ENXIO;
> > > > > > > > > -		goto err_free_mem_region;
> > > > > > > > > +		dev_err(&pdev->dev, "Cannot request memregion/iomap address\n");
> > > > > > > > > +		return -EADDRNOTAVAIL;
> > > > > > > > 
> > > > > > > > Erm, no, -EBUSY please.
> > > > > > > 
> > > > > > > EADDRNOTAVAIL is the canonical error for devm_request_and_ioremap()
> > > > > > > failure. The kerneldoc comment in lib/devres.c even gives a short
> > > > > > > example that uses this error code.
> > > > > > 
> > > > > > I am sorry, but I do not consider a function that was added a little
> > > > > > over a year ago as a canon. If you look at the uses of EADDRNOTAVAIL it
> > > > > > is used predominantly in networking code to indicate that attempted
> > > > > > _network_ address is not available.
> > > > > 
> > > > > EBUSY might be misleading, though. devm_request_and_ioremap() can fail
> > > > > in both the request_mem_region() and ioremap() calls. Furthermore it'd
> > > > > be good to settle on a consistent error-code instead of doing it
> > > > > differently depending on subsystem and/or driver. Currently the various
> > > > > error codes used are:
> > > > > 
> > > > > 	EBUSY, EADDRNOTAVAIL, ENXIO, ENOMEM, ENODEV, ENOENT, EINVAL,
> > > > > 	EIO, EFAULT, EADDRINUSE
> > > > > 
> > > > > Also if we can settle on one error code we should follow up with a patch
> > > > > to make it consistent across the tree and also update that kerneldoc
> > > > > comment. I volunteer to do that if nobody else steps up. I'm also Cc'ing
> > > > > Wolfram (the original author), maybe he has some thoughts on this.
> > > > > 
> > > > 
> > > > If you going to change all drivers make devm_request_and_ioremap()
> > > > return ERR_PTR()-encoded errors and then we can differentiate what
> > > > part of it failed.
> > > 
> > > Yeah, that thought also crossed my mind. I'll give other people some
> > > time to comment before hurling myself into preparing patches.
> > 
> > I've prepared a patch that changes devm_request_and_ioremap() to return
> > ERR_PTR()-encoded errors and adjusts all callers. As you can imagine it
> > is a bit on the huge side. scripts/get_maintainer.pl lists 156 people
> > and mailing lists. I've gone through the list, and as far as I can tell
> > everyone on that list is actually affected by the patch, so there's not
> > much potential for tuning it down.
> > 
> > There is also the issue of whose tree this should go into. Unfortunately
> > the patch can't be broken down into smaller chunks because it changes
> > how the devm_request_and_ioremap() function's return value is handled in
> > an incompatible way, so it won't be possible to gradually phase this in.
> > Furthermore I can imagine that until the end of the release cycle new
> > code may be added on which the same transformations need to be done. I
> > have a semantic patch to do the bulk of the work, but quite a bit of
> > coordination will be required.
> > 
> > I'm adding Arnd and Greg on Cc, maybe they can advise on how best to
> > handle this kind of patch.
> 
> You should provide a "wrapper" function that does the correct return
> value, convert drivers over to it, and then, when everyone is changed,
> drop the old call.  To change 156 different drivers all at once, in a
> way that is not detectable by the compiler breaking the build, is not a
> good thing to do at all.
> 
> By doing it in this manner, it will take longer, but you can push the
> patches through the different maintainer's trees.  If they are slow,
> I'll be glad to take the remaining patches in my driver-core tree to do
> the final bits.

It certainly sounds like a less complicated way to do it. But it also
involves adding a function with a made up name and drop a function with
a perfectly good name instead. I wouldn't even know what name to choose
for the new API.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v2 2/4] input: keyboard: tegra: use devm_* for resource allocation

[Arnd Bergmann]

On Monday 14 January 2013, Thierry Reding wrote:
> It certainly sounds like a less complicated way to do it. But it also
> involves adding a function with a made up name and drop a function with
> a perfectly good name instead. I wouldn't even know what name to choose
> for the new API.
> 

How about devm_ioremap_resource()? Sounds equally fitting, and is shorter.

	Arnd

Re: [PATCH v2 2/4] input: keyboard: tegra: use devm_* for resource allocation

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 519 bytes --]

On Mon, Jan 14, 2013 at 10:24:11PM +0000, Arnd Bergmann wrote:
> On Monday 14 January 2013, Thierry Reding wrote:
> > It certainly sounds like a less complicated way to do it. But it also
> > involves adding a function with a made up name and drop a function with
> > a perfectly good name instead. I wouldn't even know what name to choose
> > for the new API.
> > 
> 
> How about devm_ioremap_resource()? Sounds equally fitting, and is shorter.

Yes, that sounds good. Thanks for the suggestion.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v2 2/4] input: keyboard: tegra: use devm_* for resource allocation

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 826 bytes --]

On Mon, Jan 14, 2013 at 10:24:11PM +0000, Arnd Bergmann wrote:
> On Monday 14 January 2013, Thierry Reding wrote:
> > It certainly sounds like a less complicated way to do it. But it also
> > involves adding a function with a made up name and drop a function with
> > a perfectly good name instead. I wouldn't even know what name to choose
> > for the new API.
> > 
> 
> How about devm_ioremap_resource()? Sounds equally fitting, and is shorter.

2 cents, I wrote back then:

* the new function is not named 'devm_ioremap_resource' because it does more than
  just ioremap, namely request_mem_region. I didn't want to create implicit
  knowledge here.

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH v2 2/4] input: keyboard: tegra: use devm_* for resource allocation

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 3757 bytes --]

Hi,

> > > > > I am sorry, but I do not consider a function that was added a little
> > > > > over a year ago as a canon. If you look at the uses of EADDRNOTAVAIL it
> > > > > is used predominantly in networking code to indicate that attempted
> > > > > _network_ address is not available.
> > > > 
> > > > EBUSY might be misleading, though. devm_request_and_ioremap() can fail
> > > > in both the request_mem_region() and ioremap() calls. Furthermore it'd
> > > > be good to settle on a consistent error-code instead of doing it
> > > > differently depending on subsystem and/or driver. Currently the various
> > > > error codes used are:
> > > > 
> > > > 	EBUSY, EADDRNOTAVAIL, ENXIO, ENOMEM, ENODEV, ENOENT, EINVAL,
> > > > 	EIO, EFAULT, EADDRINUSE
> > > > 
> > > > Also if we can settle on one error code we should follow up with a patch
> > > > to make it consistent across the tree and also update that kerneldoc
> > > > comment. I volunteer to do that if nobody else steps up. I'm also Cc'ing
> > > > Wolfram (the original author), maybe he has some thoughts on this.

Handling the error case was the biggest discussion back then. I
initially did not want to use ERR_PTR, because I see already enough
patches adding a forgotten ERR_PTR to drivers. My initial idea was to
return a simple errno and have the pointer a function argument. I was
convinced [1], however, that the dev_err printout is enough to make
visible what actually went wrong and return a NULL pointer instead. So
much for why the function does NOT return a PTR_ERR, and I still prefer
that.

Then, I added the example code in the documentation using EADDRNOTAVAIL.
Yes, I was brave with this one. Yet, EINVAL, EBUSY, ENOENT, did not
really cut it and are so heavily used in drivers that they turned into a
generic "something is wrong" error. I tried here to use a not overloaded
error code in order to be specific again. Since the patches were
accepted, I assumed it wasn't seen as a namespace violation. (Then
again, it probably would have been if that error code would go out to
userspace) Naturally, I didn't have the resources to check all patches
for a consistent error code.

> > > If you going to change all drivers make devm_request_and_ioremap()
> > > return ERR_PTR()-encoded errors and then we can differentiate what
> > > part of it failed.
> > 
> > Yeah, that thought also crossed my mind. I'll give other people some
> > time to comment before hurling myself into preparing patches.

As said above, that was argued away when committing the patches.

But there is more to that:

When working with this function, there was also the idea to abstract
getting the resource away. Which then gave Sascha Hauer and me the
question, if drivers really have to do this or if this couldn't be done
by the kernel somehow, i.e. giving the drivers already the resources
they need, completely prepared.

Of course, then we would need a similar function for interrupt
resources. Which has much bigger problem with return codes, since we
then step into the area of the "0 is no interrupt" topic (while
platform_get_irq returns an error code).

As a result, I got the impression that the whole topic needs ONE
concentrated, major rehaul or at least a master plan. Adding an idea
here and there doesn't seem to cut it, at least not in the way
devm_request_and_ioremap() was done. I would be interested in doing that
but my resources don't allow me to even think about it at the moment,
sadly.

Regards,

   Wolfram

[1] http://lkml.org/lkml/2011/10/24/278

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH v2 2/4] input: keyboard: tegra: use devm_* for resource allocation

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 4527 bytes --]

On Tue, Jan 15, 2013 at 02:06:23PM +0100, Wolfram Sang wrote:
> Hi,
> 
> > > > > > I am sorry, but I do not consider a function that was added a little
> > > > > > over a year ago as a canon. If you look at the uses of EADDRNOTAVAIL it
> > > > > > is used predominantly in networking code to indicate that attempted
> > > > > > _network_ address is not available.
> > > > > 
> > > > > EBUSY might be misleading, though. devm_request_and_ioremap() can fail
> > > > > in both the request_mem_region() and ioremap() calls. Furthermore it'd
> > > > > be good to settle on a consistent error-code instead of doing it
> > > > > differently depending on subsystem and/or driver. Currently the various
> > > > > error codes used are:
> > > > > 
> > > > > 	EBUSY, EADDRNOTAVAIL, ENXIO, ENOMEM, ENODEV, ENOENT, EINVAL,
> > > > > 	EIO, EFAULT, EADDRINUSE
> > > > > 
> > > > > Also if we can settle on one error code we should follow up with a patch
> > > > > to make it consistent across the tree and also update that kerneldoc
> > > > > comment. I volunteer to do that if nobody else steps up. I'm also Cc'ing
> > > > > Wolfram (the original author), maybe he has some thoughts on this.
> 
> Handling the error case was the biggest discussion back then. I
> initially did not want to use ERR_PTR, because I see already enough
> patches adding a forgotten ERR_PTR to drivers. My initial idea was to
> return a simple errno and have the pointer a function argument. I was
> convinced [1], however, that the dev_err printout is enough to make
> visible what actually went wrong and return a NULL pointer instead. So
> much for why the function does NOT return a PTR_ERR, and I still prefer
> that.
> 
> Then, I added the example code in the documentation using EADDRNOTAVAIL.
> Yes, I was brave with this one. Yet, EINVAL, EBUSY, ENOENT, did not
> really cut it and are so heavily used in drivers that they turned into a
> generic "something is wrong" error. I tried here to use a not overloaded
> error code in order to be specific again. Since the patches were
> accepted, I assumed it wasn't seen as a namespace violation. (Then
> again, it probably would have been if that error code would go out to
> userspace) Naturally, I didn't have the resources to check all patches
> for a consistent error code.

The problem with the current approach is that people (me included) keep
telling people to use this or that error code in an attempt to achieve
some kind of consistency. Also using an error message to distinguish
between reasons for failure makes it impossible to handle the error
other than by visual inspection. Granted, there are currently no code
paths that require this.

One problem with the original patch was also that it didn't actually
convert any existing uses, so there was little chance of anyone noticing
potential problems. More than a year later this function is used by many
subsystems and a lot of drivers. It just so happened that I observed how
many people just didn't know what error codes to choose and often just
grabbing one randomly.

By adding devm_ioremap_resource() and having it return ERR_PTR()-encoded
error codes we get rid of all these problems and put the responsibility
for choosing the error code where, in my opinion, it belongs: the
failing function.

> > > > If you going to change all drivers make devm_request_and_ioremap()
> > > > return ERR_PTR()-encoded errors and then we can differentiate what
> > > > part of it failed.
> > > 
> > > Yeah, that thought also crossed my mind. I'll give other people some
> > > time to comment before hurling myself into preparing patches.
> 
> As said above, that was argued away when committing the patches.
> 
> But there is more to that:
> 
> When working with this function, there was also the idea to abstract
> getting the resource away. Which then gave Sascha Hauer and me the
> question, if drivers really have to do this or if this couldn't be done
> by the kernel somehow, i.e. giving the drivers already the resources
> they need, completely prepared.

I'm not sure I like that very much. That could possibly lead to a new
problem where drivers that need to do something special have to jump
through hoops to achieve something that may otherwise be simple.

Anyway, if people don't think this is a sensible conversion I should
waste no more time on it. On the other hand I have the patch series
ready so I might as well post it for broader review.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v2 2/4] input: keyboard: tegra: use devm_* for resource allocation

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 3966 bytes --]

Hi,

> > Then, I added the example code in the documentation using EADDRNOTAVAIL.
> > Yes, I was brave with this one. Yet, EINVAL, EBUSY, ENOENT, did not
> > really cut it and are so heavily used in drivers that they turned into a
> > generic "something is wrong" error. I tried here to use a not overloaded
> > error code in order to be specific again. Since the patches were
> > accepted, I assumed it wasn't seen as a namespace violation. (Then
> > again, it probably would have been if that error code would go out to
> > userspace) Naturally, I didn't have the resources to check all patches
> > for a consistent error code.
> 
> The problem with the current approach is that people (me included) keep
> telling people to use this or that error code in an attempt to achieve
> some kind of consistency. Also using an error message to distinguish
> between reasons for failure makes it impossible to handle the error
> other than by visual inspection. Granted, there are currently no code
> paths that require this.

Yes, so currently, this is rather a cosmetic change. And for that, it is
quite intrusive. I think something like this is needed somewhen as part
of a bigger overhaul. That's what I called "master plan" last time. That
could be that resource handling gets aligned in general, also taking
e.g. interrupt resources into account. But only changing error code
handling for this function, doesn't seem too useful to me and might even
need another change later, then.

> One problem with the original patch was also that it didn't actually
> convert any existing uses, so there was little chance of anyone noticing

Like with the error codes now, there are so many different ways of
handling resources that I did not want to mass convert all the drivers
without being able to test them. I was hoping for a migration over time
with patches from people who really tested what they did.

> potential problems. More than a year later this function is used by many
> subsystems and a lot of drivers. It just so happened that I observed how
> many people just didn't know what error codes to choose and often just
> grabbing one randomly.

Yes, and concerning resources this needs cleaning on a bigger scale IMO.

> By adding devm_ioremap_resource() and having it return ERR_PTR()-encoded
> error codes we get rid of all these problems and put the responsibility
> for choosing the error code where, in my opinion, it belongs: the
> failing function.

For completeness, it leaves the problem that people might forget to use
ERR_PTR (seen that often in the clk framework). And the change is huge
while I can't see any real benefit right now.

> > When working with this function, there was also the idea to abstract
> > getting the resource away. Which then gave Sascha Hauer and me the
> > question, if drivers really have to do this or if this couldn't be done
> > by the kernel somehow, i.e. giving the drivers already the resources
> > they need, completely prepared.
> 
> I'm not sure I like that very much. That could possibly lead to a new
> problem where drivers that need to do something special have to jump
> through hoops to achieve something that may otherwise be simple.

I assume that drivers are already doing something special :) So, that
would turn up in the conversion process. I can't promise that it would
really work, it would need some playing around.

> Anyway, if people don't think this is a sensible conversion I should
> waste no more time on it. On the other hand I have the patch series
> ready so I might as well post it for broader review.

Working on patches is hardly a waste. Even if not accepted, you gain
understanding. Please put me on CC, if you post the patches.

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH v2 2/4] input: keyboard: tegra: use devm_* for resource allocation

[Grant Likely]

On Wed, 9 Jan 2013 01:19:39 -0800, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> On Wed, Jan 09, 2013 at 08:07:45AM +0100, Thierry Reding wrote:
> > On Sun, Jan 06, 2013 at 11:57:48AM -0800, Dmitry Torokhov wrote:
> > > On Sun, Jan 06, 2013 at 08:27:39PM +0100, Thierry Reding wrote:
> > > > On Sat, Jan 05, 2013 at 12:06:58AM -0800, Dmitry Torokhov wrote:
> > > > > On Sat, Jan 05, 2013 at 01:15:08PM +0530, Laxman Dewangan wrote:
> > > > [...]
> > > > > > @@ -735,25 +738,16 @@ static int tegra_kbc_probe(struct platform_device *pdev)
> > > > > >  	spin_lock_init(&kbc->lock);
> > > > > >  	setup_timer(&kbc->timer, tegra_kbc_keypress_timer, (unsigned long)kbc);
> > > > > >  
> > > > > > -	res = request_mem_region(res->start, resource_size(res), pdev->name);
> > > > > > -	if (!res) {
> > > > > > -		dev_err(&pdev->dev, "failed to request I/O memory\n");
> > > > > > -		err = -EBUSY;
> > > > > > -		goto err_free_mem;
> > > > > > -	}
> > > > > > -
> > > > > > -	kbc->mmio = ioremap(res->start, resource_size(res));
> > > > > > +	kbc->mmio = devm_request_and_ioremap(&pdev->dev, res);
> > > > > >  	if (!kbc->mmio) {
> > > > > > -		dev_err(&pdev->dev, "failed to remap I/O memory\n");
> > > > > > -		err = -ENXIO;
> > > > > > -		goto err_free_mem_region;
> > > > > > +		dev_err(&pdev->dev, "Cannot request memregion/iomap address\n");
> > > > > > +		return -EADDRNOTAVAIL;
> > > > > 
> > > > > Erm, no, -EBUSY please.
> > > > 
> > > > EADDRNOTAVAIL is the canonical error for devm_request_and_ioremap()
> > > > failure. The kerneldoc comment in lib/devres.c even gives a short
> > > > example that uses this error code.
> > > 
> > > I am sorry, but I do not consider a function that was added a little
> > > over a year ago as a canon. If you look at the uses of EADDRNOTAVAIL it
> > > is used predominantly in networking code to indicate that attempted
> > > _network_ address is not available.
> > 
> > EBUSY might be misleading, though. devm_request_and_ioremap() can fail
> > in both the request_mem_region() and ioremap() calls. Furthermore it'd
> > be good to settle on a consistent error-code instead of doing it
> > differently depending on subsystem and/or driver. Currently the various
> > error codes used are:
> > 
> > 	EBUSY, EADDRNOTAVAIL, ENXIO, ENOMEM, ENODEV, ENOENT, EINVAL,
> > 	EIO, EFAULT, EADDRINUSE
> > 
> > Also if we can settle on one error code we should follow up with a patch
> > to make it consistent across the tree and also update that kerneldoc
> > comment. I volunteer to do that if nobody else steps up. I'm also Cc'ing
> > Wolfram (the original author), maybe he has some thoughts on this.

This really doesn't matter. As far as the driver is concerned if the
memory isn't available then it is a failure. Period. Who cares what
the reason was.

> If you going to change all drivers make devm_request_and_ioremap()
> return ERR_PTR()-encoded errors and then we can differentiate what
> part of it failed.

The ERR_PTR() pattern is actually worse when it comes to reading and
understanding code. Us C coders have had beaten into us that the correct
test for an invalid pointer is "if (!ptr)". ERR_PTR() is actively
dangerous in this regard because it breaks that pattern, but you cannot
tell from looking at the API that it is wrong.

There are places where it makes sense, but *please* don't use the
ERR_PTR pattern unless absolutely necessary.

Rarely are the actual error codes important for kernel internal stuff.
The error codes only really matter when passing them up to userspace
where they have specific meanings. As far as in-kernel stuff goes, the
policy for choosing error codes consists of "go look at the errno file
and choose one that looks vaguely relevant".

g.