[PATCH 1/3] Input: imx_keypad - Add device tree support

[PATCH 1/3] Input: imx_keypad - Add device tree support

[Liu Ying]

This patch adds device tree support for imx keypad driver.
1) Basic device tree binding support.
2) Since the probe function needs keymap and keymap size
   information, we get them from device tree if it is
   supported, otherwise, we fall back to legacy platform data.
3) Support to configure keypad pins via pinctrl system.

Signed-off-by: Liu Ying <Ying.Liu@freescale.com>
---
 .../devicetree/bindings/input/imx-keypad.txt       |   54 +++++++++++++
 drivers/input/keyboard/imx_keypad.c                |   80 ++++++++++++++++++--
 2 files changed, 127 insertions(+), 7 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/input/imx-keypad.txt

diff --git a/Documentation/devicetree/bindings/input/imx-keypad.txt b/Documentation/devicetree/bindings/input/imx-keypad.txt
new file mode 100644
index 0000000..1c6e16d
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/imx-keypad.txt
@@ -0,0 +1,54 @@
+* Freescale i.MX Keypad Port(KPP) device tree bindings
+
+The KPP is designed to interface with a keypad matrix with 2-point contact
+or 3-point contact keys. The KPP is designed to simplify the software task
+of scanning a keypad matrix. The KPP is capable of detecting, debouncing,
+and decoding one or multiple keys pressed simultaneously on a keypad.
+
+Required SoC Specific Properties:
+- compatible: Should be "fsl,imx-kpp".
+
+- reg: Physical base address of the KPP and length of memory mapped
+  region.
+
+- interrupts: The KPP interrupt number to the CPU(s).
+
+- clocks: The clock provided by the SoC to the KPP. Some SoCs use ipg clock,
+others use dummy clock(The clock for the KPP is provided by the SoCs
+automatically).
+
+Required Board Specific Properties:
+- pinctrl-names: The definition can be found at
+pinctrl/pinctrl-bindings.txt.
+
+- pinctrl-0: The definition can be found at
+pinctrl/pinctrl-bindings.txt.
+
+- linux,keymap: The definition can be found at
+bindings/input/matrix-keymap.txt.
+
+Example:
+kpp: kpp at 73f94000 {
+	compatible = "fsl,imx-kpp";
+	reg = <0x73f94000 0x4000>;
+	interrupts = <60>;
+	clocks = <&clks 0>;
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_kpp_1>;
+	linux,keymap = <0x00000067	/* KEY_UP */
+			0x0001006c	/* KEY_DOWN */
+			0x00020072	/* KEY_VOLUMEDOWN */
+			0x00030066	/* KEY_HOME */
+			0x0100006a	/* KEY_RIGHT */
+			0x01010069	/* KEY_LEFT */
+			0x0102001c	/* KEY_ENTER */
+			0x01030073	/* KEY_VOLUMEUP */
+			0x02000040	/* KEY_F6 */
+			0x02010042	/* KEY_F8 */
+			0x02020043	/* KEY_F9 */
+			0x02030044	/* KEY_F10 */
+			0x0300003b	/* KEY_F1 */
+			0x0301003c	/* KEY_F2 */
+			0x0302003d	/* KEY_F3 */
+			0x03030074>;	/* KEY_POWER */
+};
diff --git a/drivers/input/keyboard/imx_keypad.c b/drivers/input/keyboard/imx_keypad.c
index 6d150e3..e4f5c6a 100644
--- a/drivers/input/keyboard/imx_keypad.c
+++ b/drivers/input/keyboard/imx_keypad.c
@@ -20,6 +20,8 @@
 #include <linux/jiffies.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
+#include <linux/of.h>
+#include <linux/pinctrl/consumer.h>
 #include <linux/platform_device.h>
 #include <linux/slab.h>
 #include <linux/timer.h>
@@ -414,17 +416,76 @@ open_err:
 	return -EIO;
 }
 
+static struct of_device_id imx_keypad_of_match[] = {
+	{ .compatible = "fsl,imx-kpp", },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, imx_keypad_of_match);
+
+#ifdef CONFIG_OF
+static int imx_keypad_parse_dt(struct platform_device *pdev,
+			       uint32_t *keymap, int *keymap_size)
+{
+	struct device_node *np = pdev->dev.of_node;
+	struct device *dev = &pdev->dev;
+	unsigned int proplen, i;
+	const __be32 *prop;
+
+	if (!np)
+		return -ENODEV;
+
+	prop = of_get_property(np, "linux,keymap", &proplen);
+	if (!prop) {
+		dev_err(dev, "OF: linux,keymap property not defined in %s\n",
+				np->full_name);
+		return -ENOENT;
+	}
+
+	if (proplen % sizeof(u32)) {
+		dev_err(dev, "OF: Malformed keycode property in %s\n",
+				np->full_name);
+		return -EINVAL;
+	}
+
+	*keymap_size = proplen / sizeof(u32);
+	if (*keymap_size > MAX_MATRIX_KEY_NUM) {
+		dev_err(dev, "OF: linux,keymap size overflow\n");
+		return -EINVAL;
+	}
+
+	for (i = 0; i < *keymap_size; i++)
+		keymap[i] = be32_to_cpup(prop + i);
+
+	return 0;
+}
+#else
+static inline int imx_keypad_parse_dt(struct platform_device *pdev,
+				      uint32_t *keymap, int *keymap_size)
+{
+	return -ENODEV;
+}
+#endif
+
 static int imx_keypad_probe(struct platform_device *pdev)
 {
 	const struct matrix_keymap_data *keymap_data = pdev->dev.platform_data;
 	struct imx_keypad *keypad;
 	struct input_dev *input_dev;
 	struct resource *res;
-	int irq, error, i;
+	int irq, error, i, keymap_size = 0;
+	uint32_t keymap[MAX_MATRIX_KEY_NUM];
+	struct pinctrl *pinctrl;
 
-	if (keymap_data == NULL) {
-		dev_err(&pdev->dev, "no keymap defined\n");
-		return -EINVAL;
+	error = imx_keypad_parse_dt(pdev, keymap, &keymap_size);
+	if (error) {
+		if (keymap_data) {
+			keymap_size = keymap_data->keymap_size;
+			memcpy(keymap, keymap_data->keymap,
+					keymap_size * sizeof(keymap[0]));
+		} else {
+			dev_err(&pdev->dev, "no keymap defined\n");
+			return -EINVAL;
+		}
 	}
 
 	irq = platform_get_irq(pdev, 0);
@@ -473,6 +534,10 @@ static int imx_keypad_probe(struct platform_device *pdev)
 		goto failed_free_priv;
 	}
 
+	pinctrl = devm_pinctrl_get_select_default(&pdev->dev);
+	if (IS_ERR(pinctrl))
+		dev_warn(&pdev->dev, "failed to get default pinctrl\n");
+
 	keypad->clk = clk_get(&pdev->dev, NULL);
 	if (IS_ERR(keypad->clk)) {
 		dev_err(&pdev->dev, "failed to get keypad clock\n");
@@ -481,9 +546,9 @@ static int imx_keypad_probe(struct platform_device *pdev)
 	}
 
 	/* Search for rows and cols enabled */
-	for (i = 0; i < keymap_data->keymap_size; i++) {
-		keypad->rows_en_mask |= 1 << KEY_ROW(keymap_data->keymap[i]);
-		keypad->cols_en_mask |= 1 << KEY_COL(keymap_data->keymap[i]);
+	for (i = 0; i < keymap_size; i++) {
+		keypad->rows_en_mask |= 1 << KEY_ROW(keymap[i]);
+		keypad->cols_en_mask |= 1 << KEY_COL(keymap[i]);
 	}
 
 	if (keypad->rows_en_mask > ((1 << MAX_MATRIX_KEY_ROWS) - 1) ||
@@ -631,6 +696,7 @@ static struct platform_driver imx_keypad_driver = {
 		.name	= "imx-keypad",
 		.owner	= THIS_MODULE,
 		.pm	= &imx_kbd_pm_ops,
+		.of_match_table = of_match_ptr(imx_keypad_of_match),
 	},
 	.probe		= imx_keypad_probe,
 	.remove		= imx_keypad_remove,
-- 
1.7.1

[PATCH 2/3] ARM: dts: imx: Add imx51 KPP entry

[Liu Ying]

1) Add KPP device node entry.
2) Add one KPP pinctrl entry.

Signed-off-by: Liu Ying <Ying.Liu@freescale.com>
---
 arch/arm/boot/dts/imx51.dtsi |   22 ++++++++++++++++++++++
 1 files changed, 22 insertions(+), 0 deletions(-)

diff --git a/arch/arm/boot/dts/imx51.dtsi b/arch/arm/boot/dts/imx51.dtsi
index 1f5d45e..3150a31 100644
--- a/arch/arm/boot/dts/imx51.dtsi
+++ b/arch/arm/boot/dts/imx51.dtsi
@@ -221,6 +221,14 @@
 				#interrupt-cells = <2>;
 			};
 
+			kpp: kpp at 73f94000 {
+				compatible = "fsl,imx-kpp";
+				reg = <0x73f94000 0x4000>;
+				interrupts = <60>;
+				clocks = <&clks 0>;
+				status = "disabled";
+			};
+
 			wdog1: wdog at 73f98000 {
 				compatible = "fsl,imx51-wdt", "fsl,imx21-wdt";
 				reg = <0x73f98000 0x4000>;
@@ -410,6 +418,20 @@
 						>;
 					};
 				};
+				kpp {
+					pinctrl_kpp_1: kppgrp-1 {
+						fsl,pins = <
+							438 0xe0	/* MX51_PAD_KEY_ROW0__KEY_ROW0 */
+							439 0xe0	/* MX51_PAD_KEY_ROW1__KEY_ROW1 */
+							440 0xe0	/* MX51_PAD_KEY_ROW2__KEY_ROW2 */
+							441 0xe0	/* MX51_PAD_KEY_ROW3__KEY_ROW3 */
+							442 0xe8	/* MX51_PAD_KEY_COL0__KEY_COL0 */
+							444 0xe8	/* MX51_PAD_KEY_COL1__KEY_COL1 */
+							446 0xe8	/* MX51_PAD_KEY_COL2__KEY_COL2 */
+							448 0xe8	/* MX51_PAD_KEY_COL3__KEY_COL3 */
+						>;
+					};
+				};
 			};
 
 			pwm1: pwm at 73fb4000 {
-- 
1.7.1

[PATCH 3/3] ARM i.MX51 babbage: Add keypad support

[Liu Ying]

The keypad is on the accessory board of i.MX51 babbage
main board and is driven by Keypad Port(KPP) in SoC.
The keymap is the same to i.MX25 3stack platform as
the accessory board schematic tells that it is designed
in this way.

Signed-off-by: Liu Ying <Ying.Liu@freescale.com>
---
 arch/arm/boot/dts/imx51-babbage.dts |   22 ++++++++++++++++++++++
 1 files changed, 22 insertions(+), 0 deletions(-)

diff --git a/arch/arm/boot/dts/imx51-babbage.dts b/arch/arm/boot/dts/imx51-babbage.dts
index 567e7ee..a9bb799 100644
--- a/arch/arm/boot/dts/imx51-babbage.dts
+++ b/arch/arm/boot/dts/imx51-babbage.dts
@@ -281,3 +281,25 @@
 		mux-ext-port = <3>;
 	};
 };
+
+&kpp {
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_kpp_1>;
+	linux,keymap = <0x00000067	/* KEY_UP */
+			0x0001006c	/* KEY_DOWN */
+			0x00020072	/* KEY_VOLUMEDOWN */
+			0x00030066	/* KEY_HOME */
+			0x0100006a	/* KEY_RIGHT */
+			0x01010069	/* KEY_LEFT */
+			0x0102001c	/* KEY_ENTER */
+			0x01030073	/* KEY_VOLUMEUP */
+			0x02000040	/* KEY_F6 */
+			0x02010042	/* KEY_F8 */
+			0x02020043	/* KEY_F9 */
+			0x02030044	/* KEY_F10 */
+			0x0300003b	/* KEY_F1 */
+			0x0301003c	/* KEY_F2 */
+			0x0302003d	/* KEY_F3 */
+			0x03030074>;	/* KEY_POWER */
+	status = "okay";
+};
-- 
1.7.1

[PATCH 1/3] Input: imx_keypad - Add device tree support

[Dmitry Torokhov]

Hi Liu,

On Sun, Dec 30, 2012 at 09:09:04PM +0800, Liu Ying wrote:
> This patch adds device tree support for imx keypad driver.
> 1) Basic device tree binding support.
> 2) Since the probe function needs keymap and keymap size
>    information, we get them from device tree if it is
>    supported, otherwise, we fall back to legacy platform data.
> 3) Support to configure keypad pins via pinctrl system.
> 
> Signed-off-by: Liu Ying <Ying.Liu@freescale.com>
> ---
>  .../devicetree/bindings/input/imx-keypad.txt       |   54 +++++++++++++
>  drivers/input/keyboard/imx_keypad.c                |   80 ++++++++++++++++++--
>  2 files changed, 127 insertions(+), 7 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/input/imx-keypad.txt
> 
> diff --git a/Documentation/devicetree/bindings/input/imx-keypad.txt b/Documentation/devicetree/bindings/input/imx-keypad.txt
> new file mode 100644
> index 0000000..1c6e16d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/input/imx-keypad.txt
> @@ -0,0 +1,54 @@
> +* Freescale i.MX Keypad Port(KPP) device tree bindings
> +
> +The KPP is designed to interface with a keypad matrix with 2-point contact
> +or 3-point contact keys. The KPP is designed to simplify the software task
> +of scanning a keypad matrix. The KPP is capable of detecting, debouncing,
> +and decoding one or multiple keys pressed simultaneously on a keypad.
> +
> +Required SoC Specific Properties:
> +- compatible: Should be "fsl,imx-kpp".
> +
> +- reg: Physical base address of the KPP and length of memory mapped
> +  region.
> +
> +- interrupts: The KPP interrupt number to the CPU(s).
> +
> +- clocks: The clock provided by the SoC to the KPP. Some SoCs use ipg clock,
> +others use dummy clock(The clock for the KPP is provided by the SoCs
> +automatically).
> +
> +Required Board Specific Properties:
> +- pinctrl-names: The definition can be found at
> +pinctrl/pinctrl-bindings.txt.
> +
> +- pinctrl-0: The definition can be found at
> +pinctrl/pinctrl-bindings.txt.
> +
> +- linux,keymap: The definition can be found at
> +bindings/input/matrix-keymap.txt.
> +
> +Example:
> +kpp: kpp at 73f94000 {
> +	compatible = "fsl,imx-kpp";
> +	reg = <0x73f94000 0x4000>;
> +	interrupts = <60>;
> +	clocks = <&clks 0>;
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pinctrl_kpp_1>;
> +	linux,keymap = <0x00000067	/* KEY_UP */
> +			0x0001006c	/* KEY_DOWN */
> +			0x00020072	/* KEY_VOLUMEDOWN */
> +			0x00030066	/* KEY_HOME */
> +			0x0100006a	/* KEY_RIGHT */
> +			0x01010069	/* KEY_LEFT */
> +			0x0102001c	/* KEY_ENTER */
> +			0x01030073	/* KEY_VOLUMEUP */
> +			0x02000040	/* KEY_F6 */
> +			0x02010042	/* KEY_F8 */
> +			0x02020043	/* KEY_F9 */
> +			0x02030044	/* KEY_F10 */
> +			0x0300003b	/* KEY_F1 */
> +			0x0301003c	/* KEY_F2 */
> +			0x0302003d	/* KEY_F3 */
> +			0x03030074>;	/* KEY_POWER */
> +};
> diff --git a/drivers/input/keyboard/imx_keypad.c b/drivers/input/keyboard/imx_keypad.c
> index 6d150e3..e4f5c6a 100644
> --- a/drivers/input/keyboard/imx_keypad.c
> +++ b/drivers/input/keyboard/imx_keypad.c
> @@ -20,6 +20,8 @@
>  #include <linux/jiffies.h>
>  #include <linux/kernel.h>
>  #include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/pinctrl/consumer.h>
>  #include <linux/platform_device.h>
>  #include <linux/slab.h>
>  #include <linux/timer.h>
> @@ -414,17 +416,76 @@ open_err:
>  	return -EIO;
>  }
>  
> +static struct of_device_id imx_keypad_of_match[] = {
> +	{ .compatible = "fsl,imx-kpp", },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, imx_keypad_of_match);
> +
> +#ifdef CONFIG_OF
> +static int imx_keypad_parse_dt(struct platform_device *pdev,
> +			       uint32_t *keymap, int *keymap_size)
> +{
> +	struct device_node *np = pdev->dev.of_node;
> +	struct device *dev = &pdev->dev;
> +	unsigned int proplen, i;
> +	const __be32 *prop;
> +
> +	if (!np)
> +		return -ENODEV;
> +
> +	prop = of_get_property(np, "linux,keymap", &proplen);
> +	if (!prop) {
> +		dev_err(dev, "OF: linux,keymap property not defined in %s\n",
> +				np->full_name);
> +		return -ENOENT;
> +	}
> +
> +	if (proplen % sizeof(u32)) {
> +		dev_err(dev, "OF: Malformed keycode property in %s\n",
> +				np->full_name);
> +		return -EINVAL;
> +	}
> +
> +	*keymap_size = proplen / sizeof(u32);
> +	if (*keymap_size > MAX_MATRIX_KEY_NUM) {
> +		dev_err(dev, "OF: linux,keymap size overflow\n");
> +		return -EINVAL;
> +	}
> +
> +	for (i = 0; i < *keymap_size; i++)
> +		keymap[i] = be32_to_cpup(prop + i);

All of this is implemented in matrix_keypad_build_keymap(), there is no
need to do it again here.

> +
> +	return 0;
> +}
> +#else
> +static inline int imx_keypad_parse_dt(struct platform_device *pdev,
> +				      uint32_t *keymap, int *keymap_size)
> +{
> +	return -ENODEV;
> +}
> +#endif
> +
>  static int imx_keypad_probe(struct platform_device *pdev)
>  {
>  	const struct matrix_keymap_data *keymap_data = pdev->dev.platform_data;
>  	struct imx_keypad *keypad;
>  	struct input_dev *input_dev;
>  	struct resource *res;
> -	int irq, error, i;
> +	int irq, error, i, keymap_size = 0;
> +	uint32_t keymap[MAX_MATRIX_KEY_NUM];
> +	struct pinctrl *pinctrl;
>  
> -	if (keymap_data == NULL) {
> -		dev_err(&pdev->dev, "no keymap defined\n");
> -		return -EINVAL;
> +	error = imx_keypad_parse_dt(pdev, keymap, &keymap_size);
> +	if (error) {
> +		if (keymap_data) {
> +			keymap_size = keymap_data->keymap_size;
> +			memcpy(keymap, keymap_data->keymap,
> +					keymap_size * sizeof(keymap[0]));

I believe kernel-provided platform data should take precedence over
DT-supplied data.

> +		} else {
> +			dev_err(&pdev->dev, "no keymap defined\n");
> +			return -EINVAL;
> +		}
>  	}
>  
>  	irq = platform_get_irq(pdev, 0);
> @@ -473,6 +534,10 @@ static int imx_keypad_probe(struct platform_device *pdev)
>  		goto failed_free_priv;
>  	}
>  
> +	pinctrl = devm_pinctrl_get_select_default(&pdev->dev);
> +	if (IS_ERR(pinctrl))
> +		dev_warn(&pdev->dev, "failed to get default pinctrl\n");
> +

No, please let core do this for you. Linus has a patch doing just that.

>  	keypad->clk = clk_get(&pdev->dev, NULL);
>  	if (IS_ERR(keypad->clk)) {
>  		dev_err(&pdev->dev, "failed to get keypad clock\n");
> @@ -481,9 +546,9 @@ static int imx_keypad_probe(struct platform_device *pdev)
>  	}
>  
>  	/* Search for rows and cols enabled */
> -	for (i = 0; i < keymap_data->keymap_size; i++) {
> -		keypad->rows_en_mask |= 1 << KEY_ROW(keymap_data->keymap[i]);
> -		keypad->cols_en_mask |= 1 << KEY_COL(keymap_data->keymap[i]);
> +	for (i = 0; i < keymap_size; i++) {
> +		keypad->rows_en_mask |= 1 << KEY_ROW(keymap[i]);
> +		keypad->cols_en_mask |= 1 << KEY_COL(keymap[i]);
>  	}
>  
>  	if (keypad->rows_en_mask > ((1 << MAX_MATRIX_KEY_ROWS) - 1) ||
> @@ -631,6 +696,7 @@ static struct platform_driver imx_keypad_driver = {
>  		.name	= "imx-keypad",
>  		.owner	= THIS_MODULE,
>  		.pm	= &imx_kbd_pm_ops,
> +		.of_match_table = of_match_ptr(imx_keypad_of_match),
>  	},
>  	.probe		= imx_keypad_probe,
>  	.remove		= imx_keypad_remove,
> -- 
> 1.7.1
> 
> 

Thanks.

-- 
Dmitry

[PATCH 1/3] Input: imx_keypad - Add device tree support

[Shawn Guo]

Hi Liu,

On Sun, Dec 30, 2012 at 03:28:52PM -0800, Dmitry Torokhov wrote:
> > @@ -473,6 +534,10 @@ static int imx_keypad_probe(struct platform_device *pdev)
> >  		goto failed_free_priv;
> >  	}
> >  
> > +	pinctrl = devm_pinctrl_get_select_default(&pdev->dev);
> > +	if (IS_ERR(pinctrl))
> > +		dev_warn(&pdev->dev, "failed to get default pinctrl\n");
> > +
> 
> No, please let core do this for you. Linus has a patch doing just that.
> 
FYI.  Dmitry is talking about the patch[1] below.  Unfortunately, you
will not be able to base your patch on that until v3.9-rc1.

Shawn

[1] http://thread.gmane.org/gmane.linux.kernel/1409263

[PATCH 1/3] Input: imx_keypad - Add device tree support

[Shawn Guo]

On Sun, Dec 30, 2012 at 09:09:04PM +0800, Liu Ying wrote:
> +Required SoC Specific Properties:
> +- compatible: Should be "fsl,imx-kpp".

We generally use SoC name to specify the version of hardware block in
device tree binding.  In this case, the compatible string should look
like "fsl,<soc>-kpp" where <soc> is the first IMX chip using this
block.

For example, if i.MX21 is the first chip integrating the block, and
i.MX51 just integrates the same block which is completely compatible
to the one on i.MX21 for software, it should be something like below.

static struct of_device_id imx_keypad_of_match[] = {
	{ .compatible = "fsl,imx21-kpp", },
	{ /* sentinel */ }
};
MODULE_DEVICE_TABLE(of, imx_keypad_of_match);

kpp: kpp at 73f94000 {
	compatible = "fsl,imx51-kpp", "fsl,imx21-kpp";
	...
};

Shawn

[PATCH 1/3] Input: imx_keypad - Add device tree support

[Liu Ying]

Hi, Dmitry,

Thanks for reviewing this patch.

2012/12/31 Dmitry Torokhov <dmitry.torokhov@gmail.com>:
> Hi Liu,
>
> On Sun, Dec 30, 2012 at 09:09:04PM +0800, Liu Ying wrote:
>> This patch adds device tree support for imx keypad driver.
>> 1) Basic device tree binding support.
>> 2) Since the probe function needs keymap and keymap size
>>    information, we get them from device tree if it is
>>    supported, otherwise, we fall back to legacy platform data.
>> 3) Support to configure keypad pins via pinctrl system.
>>
>> Signed-off-by: Liu Ying <Ying.Liu@freescale.com>
>> ---
>>  .../devicetree/bindings/input/imx-keypad.txt       |   54 +++++++++++++
>>  drivers/input/keyboard/imx_keypad.c                |   80 ++++++++++++++++++--
>>  2 files changed, 127 insertions(+), 7 deletions(-)
>>  create mode 100644 Documentation/devicetree/bindings/input/imx-keypad.txt
>>
>> diff --git a/Documentation/devicetree/bindings/input/imx-keypad.txt b/Documentation/devicetree/bindings/input/imx-keypad.txt
>> new file mode 100644
>> index 0000000..1c6e16d
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/input/imx-keypad.txt
>> @@ -0,0 +1,54 @@
>> +* Freescale i.MX Keypad Port(KPP) device tree bindings
>> +
>> +The KPP is designed to interface with a keypad matrix with 2-point contact
>> +or 3-point contact keys. The KPP is designed to simplify the software task
>> +of scanning a keypad matrix. The KPP is capable of detecting, debouncing,
>> +and decoding one or multiple keys pressed simultaneously on a keypad.
>> +
>> +Required SoC Specific Properties:
>> +- compatible: Should be "fsl,imx-kpp".
>> +
>> +- reg: Physical base address of the KPP and length of memory mapped
>> +  region.
>> +
>> +- interrupts: The KPP interrupt number to the CPU(s).
>> +
>> +- clocks: The clock provided by the SoC to the KPP. Some SoCs use ipg clock,
>> +others use dummy clock(The clock for the KPP is provided by the SoCs
>> +automatically).
>> +
>> +Required Board Specific Properties:
>> +- pinctrl-names: The definition can be found at
>> +pinctrl/pinctrl-bindings.txt.
>> +
>> +- pinctrl-0: The definition can be found at
>> +pinctrl/pinctrl-bindings.txt.
>> +
>> +- linux,keymap: The definition can be found at
>> +bindings/input/matrix-keymap.txt.
>> +
>> +Example:
>> +kpp: kpp at 73f94000 {
>> +     compatible = "fsl,imx-kpp";
>> +     reg = <0x73f94000 0x4000>;
>> +     interrupts = <60>;
>> +     clocks = <&clks 0>;
>> +     pinctrl-names = "default";
>> +     pinctrl-0 = <&pinctrl_kpp_1>;
>> +     linux,keymap = <0x00000067      /* KEY_UP */
>> +                     0x0001006c      /* KEY_DOWN */
>> +                     0x00020072      /* KEY_VOLUMEDOWN */
>> +                     0x00030066      /* KEY_HOME */
>> +                     0x0100006a      /* KEY_RIGHT */
>> +                     0x01010069      /* KEY_LEFT */
>> +                     0x0102001c      /* KEY_ENTER */
>> +                     0x01030073      /* KEY_VOLUMEUP */
>> +                     0x02000040      /* KEY_F6 */
>> +                     0x02010042      /* KEY_F8 */
>> +                     0x02020043      /* KEY_F9 */
>> +                     0x02030044      /* KEY_F10 */
>> +                     0x0300003b      /* KEY_F1 */
>> +                     0x0301003c      /* KEY_F2 */
>> +                     0x0302003d      /* KEY_F3 */
>> +                     0x03030074>;    /* KEY_POWER */
>> +};
>> diff --git a/drivers/input/keyboard/imx_keypad.c b/drivers/input/keyboard/imx_keypad.c
>> index 6d150e3..e4f5c6a 100644
>> --- a/drivers/input/keyboard/imx_keypad.c
>> +++ b/drivers/input/keyboard/imx_keypad.c
>> @@ -20,6 +20,8 @@
>>  #include <linux/jiffies.h>
>>  #include <linux/kernel.h>
>>  #include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/pinctrl/consumer.h>
>>  #include <linux/platform_device.h>
>>  #include <linux/slab.h>
>>  #include <linux/timer.h>
>> @@ -414,17 +416,76 @@ open_err:
>>       return -EIO;
>>  }
>>
>> +static struct of_device_id imx_keypad_of_match[] = {
>> +     { .compatible = "fsl,imx-kpp", },
>> +     { /* sentinel */ }
>> +};
>> +MODULE_DEVICE_TABLE(of, imx_keypad_of_match);
>> +
>> +#ifdef CONFIG_OF
>> +static int imx_keypad_parse_dt(struct platform_device *pdev,
>> +                            uint32_t *keymap, int *keymap_size)
>> +{
>> +     struct device_node *np = pdev->dev.of_node;
>> +     struct device *dev = &pdev->dev;
>> +     unsigned int proplen, i;
>> +     const __be32 *prop;
>> +
>> +     if (!np)
>> +             return -ENODEV;
>> +
>> +     prop = of_get_property(np, "linux,keymap", &proplen);
>> +     if (!prop) {
>> +             dev_err(dev, "OF: linux,keymap property not defined in %s\n",
>> +                             np->full_name);
>> +             return -ENOENT;
>> +     }
>> +
>> +     if (proplen % sizeof(u32)) {
>> +             dev_err(dev, "OF: Malformed keycode property in %s\n",
>> +                             np->full_name);
>> +             return -EINVAL;
>> +     }
>> +
>> +     *keymap_size = proplen / sizeof(u32);
>> +     if (*keymap_size > MAX_MATRIX_KEY_NUM) {
>> +             dev_err(dev, "OF: linux,keymap size overflow\n");
>> +             return -EINVAL;
>> +     }
>> +
>> +     for (i = 0; i < *keymap_size; i++)
>> +             keymap[i] = be32_to_cpup(prop + i);
>
> All of this is implemented in matrix_keypad_build_keymap(), there is no
> need to do it again here.
The probe function of this driver needs the keymap information to set
keypad->rows_en_mask and keypad->cols_en_mask so that they can be used
later. I plan to change to parse the array keypad->keycodes[], which
is set by matrix_keypad_build_keymap(),  to get the row/col mask. Do
you think this is acceptable?
>> +
>> +     return 0;
>> +}
>> +#else
>> +static inline int imx_keypad_parse_dt(struct platform_device *pdev,
>> +                                   uint32_t *keymap, int *keymap_size)
>> +{
>> +     return -ENODEV;
>> +}
>> +#endif
>> +
>>  static int imx_keypad_probe(struct platform_device *pdev)
>>  {
>>       const struct matrix_keymap_data *keymap_data = pdev->dev.platform_data;
>>       struct imx_keypad *keypad;
>>       struct input_dev *input_dev;
>>       struct resource *res;
>> -     int irq, error, i;
>> +     int irq, error, i, keymap_size = 0;
>> +     uint32_t keymap[MAX_MATRIX_KEY_NUM];
>> +     struct pinctrl *pinctrl;
>>
>> -     if (keymap_data == NULL) {
>> -             dev_err(&pdev->dev, "no keymap defined\n");
>> -             return -EINVAL;
>> +     error = imx_keypad_parse_dt(pdev, keymap, &keymap_size);
>> +     if (error) {
>> +             if (keymap_data) {
>> +                     keymap_size = keymap_data->keymap_size;
>> +                     memcpy(keymap, keymap_data->keymap,
>> +                                     keymap_size * sizeof(keymap[0]));
>
> I believe kernel-provided platform data should take precedence over
> DT-supplied data.
Ok. I will improve the logic here.
>
>> +             } else {
>> +                     dev_err(&pdev->dev, "no keymap defined\n");
>> +                     return -EINVAL;
>> +             }
>>       }
>>
>>       irq = platform_get_irq(pdev, 0);
>> @@ -473,6 +534,10 @@ static int imx_keypad_probe(struct platform_device *pdev)
>>               goto failed_free_priv;
>>       }
>>
>> +     pinctrl = devm_pinctrl_get_select_default(&pdev->dev);
>> +     if (IS_ERR(pinctrl))
>> +             dev_warn(&pdev->dev, "failed to get default pinctrl\n");
>> +
>
> No, please let core do this for you. Linus has a patch doing just that.
Ok, I will remove this.
>
>>       keypad->clk = clk_get(&pdev->dev, NULL);
>>       if (IS_ERR(keypad->clk)) {
>>               dev_err(&pdev->dev, "failed to get keypad clock\n");
>> @@ -481,9 +546,9 @@ static int imx_keypad_probe(struct platform_device *pdev)
>>       }
>>
>>       /* Search for rows and cols enabled */
>> -     for (i = 0; i < keymap_data->keymap_size; i++) {
>> -             keypad->rows_en_mask |= 1 << KEY_ROW(keymap_data->keymap[i]);
>> -             keypad->cols_en_mask |= 1 << KEY_COL(keymap_data->keymap[i]);
>> +     for (i = 0; i < keymap_size; i++) {
>> +             keypad->rows_en_mask |= 1 << KEY_ROW(keymap[i]);
>> +             keypad->cols_en_mask |= 1 << KEY_COL(keymap[i]);
>>       }
>>
>>       if (keypad->rows_en_mask > ((1 << MAX_MATRIX_KEY_ROWS) - 1) ||
>> @@ -631,6 +696,7 @@ static struct platform_driver imx_keypad_driver = {
>>               .name   = "imx-keypad",
>>               .owner  = THIS_MODULE,
>>               .pm     = &imx_kbd_pm_ops,
>> +             .of_match_table = of_match_ptr(imx_keypad_of_match),
>>       },
>>       .probe          = imx_keypad_probe,
>>       .remove         = imx_keypad_remove,
>> --
>> 1.7.1
>>
>>
>
> Thanks.
>
> --
> Dmitry



-- 
Liu Ying

[PATCH 1/3] Input: imx_keypad - Add device tree support

[Liu Ying]

Hi, Shawn,

I plan to remove pinctrl code in the coming next version driver patch
first. Do you think this is ok or not?

2012/12/31 Shawn Guo <shawn.guo@linaro.org>:
> Hi Liu,
>
> On Sun, Dec 30, 2012 at 03:28:52PM -0800, Dmitry Torokhov wrote:
>> > @@ -473,6 +534,10 @@ static int imx_keypad_probe(struct platform_device *pdev)
>> >             goto failed_free_priv;
>> >     }
>> >
>> > +   pinctrl = devm_pinctrl_get_select_default(&pdev->dev);
>> > +   if (IS_ERR(pinctrl))
>> > +           dev_warn(&pdev->dev, "failed to get default pinctrl\n");
>> > +
>>
>> No, please let core do this for you. Linus has a patch doing just that.
>>
> FYI.  Dmitry is talking about the patch[1] below.  Unfortunately, you
> will not be able to base your patch on that until v3.9-rc1.
>
> Shawn
>
> [1] http://thread.gmane.org/gmane.linux.kernel/1409263
>



-- 
Liu Ying

[PATCH 1/3] Input: imx_keypad - Add device tree support

[Liu Ying]

Ok, I will implement 'compatible' in the usual way.
I checked freescale i.MX21 BSP code, and found that i.MX21 KPP
registers should be compatible with those of later presented SoCs.

2012/12/31 Shawn Guo <shawn.guo@linaro.org>:
> On Sun, Dec 30, 2012 at 09:09:04PM +0800, Liu Ying wrote:
>> +Required SoC Specific Properties:
>> +- compatible: Should be "fsl,imx-kpp".
>
> We generally use SoC name to specify the version of hardware block in
> device tree binding.  In this case, the compatible string should look
> like "fsl,<soc>-kpp" where <soc> is the first IMX chip using this
> block.
>
> For example, if i.MX21 is the first chip integrating the block, and
> i.MX51 just integrates the same block which is completely compatible
> to the one on i.MX21 for software, it should be something like below.
>
> static struct of_device_id imx_keypad_of_match[] = {
>         { .compatible = "fsl,imx21-kpp", },
>         { /* sentinel */ }
> };
> MODULE_DEVICE_TABLE(of, imx_keypad_of_match);
>
> kpp: kpp at 73f94000 {
>         compatible = "fsl,imx51-kpp", "fsl,imx21-kpp";
>         ...
> };
>
> Shawn
>

Thanks.


-- 
Liu Ying

[PATCH 1/3] Input: imx_keypad - Add device tree support

[Shawn Guo]

On 31 December 2012 14:45, Liu Ying <liu.y.victor@gmail.com> wrote:
> Hi, Shawn,
>
> I plan to remove pinctrl code in the coming next version driver patch
> first. Do you think this is ok or not?
>
Sure.  Just leave the dependency for Dmitry to handle :)

Shawn

[PATCH 1/3] Input: imx_keypad - Add device tree support

[Dmitry Torokhov]

On Mon, Dec 31, 2012 at 02:36:54PM +0800, Liu Ying wrote:
> Hi, Dmitry,
> 
> Thanks for reviewing this patch.
> 
> 2012/12/31 Dmitry Torokhov <dmitry.torokhov@gmail.com>:
> > Hi Liu,
> >
> > On Sun, Dec 30, 2012 at 09:09:04PM +0800, Liu Ying wrote:
> >> This patch adds device tree support for imx keypad driver.
> >> 1) Basic device tree binding support.
> >> 2) Since the probe function needs keymap and keymap size
> >>    information, we get them from device tree if it is
> >>    supported, otherwise, we fall back to legacy platform data.
> >> 3) Support to configure keypad pins via pinctrl system.
> >>
> >> Signed-off-by: Liu Ying <Ying.Liu@freescale.com>
> >> ---
> >>  .../devicetree/bindings/input/imx-keypad.txt       |   54 +++++++++++++
> >>  drivers/input/keyboard/imx_keypad.c                |   80 ++++++++++++++++++--
> >>  2 files changed, 127 insertions(+), 7 deletions(-)
> >>  create mode 100644 Documentation/devicetree/bindings/input/imx-keypad.txt
> >>
> >> diff --git a/Documentation/devicetree/bindings/input/imx-keypad.txt b/Documentation/devicetree/bindings/input/imx-keypad.txt
> >> new file mode 100644
> >> index 0000000..1c6e16d
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/input/imx-keypad.txt
> >> @@ -0,0 +1,54 @@
> >> +* Freescale i.MX Keypad Port(KPP) device tree bindings
> >> +
> >> +The KPP is designed to interface with a keypad matrix with 2-point contact
> >> +or 3-point contact keys. The KPP is designed to simplify the software task
> >> +of scanning a keypad matrix. The KPP is capable of detecting, debouncing,
> >> +and decoding one or multiple keys pressed simultaneously on a keypad.
> >> +
> >> +Required SoC Specific Properties:
> >> +- compatible: Should be "fsl,imx-kpp".
> >> +
> >> +- reg: Physical base address of the KPP and length of memory mapped
> >> +  region.
> >> +
> >> +- interrupts: The KPP interrupt number to the CPU(s).
> >> +
> >> +- clocks: The clock provided by the SoC to the KPP. Some SoCs use ipg clock,
> >> +others use dummy clock(The clock for the KPP is provided by the SoCs
> >> +automatically).
> >> +
> >> +Required Board Specific Properties:
> >> +- pinctrl-names: The definition can be found at
> >> +pinctrl/pinctrl-bindings.txt.
> >> +
> >> +- pinctrl-0: The definition can be found at
> >> +pinctrl/pinctrl-bindings.txt.
> >> +
> >> +- linux,keymap: The definition can be found at
> >> +bindings/input/matrix-keymap.txt.
> >> +
> >> +Example:
> >> +kpp: kpp at 73f94000 {
> >> +     compatible = "fsl,imx-kpp";
> >> +     reg = <0x73f94000 0x4000>;
> >> +     interrupts = <60>;
> >> +     clocks = <&clks 0>;
> >> +     pinctrl-names = "default";
> >> +     pinctrl-0 = <&pinctrl_kpp_1>;
> >> +     linux,keymap = <0x00000067      /* KEY_UP */
> >> +                     0x0001006c      /* KEY_DOWN */
> >> +                     0x00020072      /* KEY_VOLUMEDOWN */
> >> +                     0x00030066      /* KEY_HOME */
> >> +                     0x0100006a      /* KEY_RIGHT */
> >> +                     0x01010069      /* KEY_LEFT */
> >> +                     0x0102001c      /* KEY_ENTER */
> >> +                     0x01030073      /* KEY_VOLUMEUP */
> >> +                     0x02000040      /* KEY_F6 */
> >> +                     0x02010042      /* KEY_F8 */
> >> +                     0x02020043      /* KEY_F9 */
> >> +                     0x02030044      /* KEY_F10 */
> >> +                     0x0300003b      /* KEY_F1 */
> >> +                     0x0301003c      /* KEY_F2 */
> >> +                     0x0302003d      /* KEY_F3 */
> >> +                     0x03030074>;    /* KEY_POWER */
> >> +};
> >> diff --git a/drivers/input/keyboard/imx_keypad.c b/drivers/input/keyboard/imx_keypad.c
> >> index 6d150e3..e4f5c6a 100644
> >> --- a/drivers/input/keyboard/imx_keypad.c
> >> +++ b/drivers/input/keyboard/imx_keypad.c
> >> @@ -20,6 +20,8 @@
> >>  #include <linux/jiffies.h>
> >>  #include <linux/kernel.h>
> >>  #include <linux/module.h>
> >> +#include <linux/of.h>
> >> +#include <linux/pinctrl/consumer.h>
> >>  #include <linux/platform_device.h>
> >>  #include <linux/slab.h>
> >>  #include <linux/timer.h>
> >> @@ -414,17 +416,76 @@ open_err:
> >>       return -EIO;
> >>  }
> >>
> >> +static struct of_device_id imx_keypad_of_match[] = {
> >> +     { .compatible = "fsl,imx-kpp", },
> >> +     { /* sentinel */ }
> >> +};
> >> +MODULE_DEVICE_TABLE(of, imx_keypad_of_match);
> >> +
> >> +#ifdef CONFIG_OF
> >> +static int imx_keypad_parse_dt(struct platform_device *pdev,
> >> +                            uint32_t *keymap, int *keymap_size)
> >> +{
> >> +     struct device_node *np = pdev->dev.of_node;
> >> +     struct device *dev = &pdev->dev;
> >> +     unsigned int proplen, i;
> >> +     const __be32 *prop;
> >> +
> >> +     if (!np)
> >> +             return -ENODEV;
> >> +
> >> +     prop = of_get_property(np, "linux,keymap", &proplen);
> >> +     if (!prop) {
> >> +             dev_err(dev, "OF: linux,keymap property not defined in %s\n",
> >> +                             np->full_name);
> >> +             return -ENOENT;
> >> +     }
> >> +
> >> +     if (proplen % sizeof(u32)) {
> >> +             dev_err(dev, "OF: Malformed keycode property in %s\n",
> >> +                             np->full_name);
> >> +             return -EINVAL;
> >> +     }
> >> +
> >> +     *keymap_size = proplen / sizeof(u32);
> >> +     if (*keymap_size > MAX_MATRIX_KEY_NUM) {
> >> +             dev_err(dev, "OF: linux,keymap size overflow\n");
> >> +             return -EINVAL;
> >> +     }
> >> +
> >> +     for (i = 0; i < *keymap_size; i++)
> >> +             keymap[i] = be32_to_cpup(prop + i);
> >
> > All of this is implemented in matrix_keypad_build_keymap(), there is no
> > need to do it again here.
> The probe function of this driver needs the keymap information to set
> keypad->rows_en_mask and keypad->cols_en_mask so that they can be used
> later. I plan to change to parse the array keypad->keycodes[], which
> is set by matrix_keypad_build_keymap(),  to get the row/col mask. Do
> you think this is acceptable?

Yes, I believe this should be fine.

Thanks.

-- 
Dmitry