[PATCH 1/3] ARM: DT: tegra: add DT entry for KBC controller

[PATCH 1/3] ARM: DT: tegra: add DT entry for KBC controller

[Laxman Dewangan]

NVIDIA's Tegra SoCs have the matrix keyboard controller which
supports 16x8 type of matrix. The number of rows and columns
are configurable.

Add DT entry for KBC controller.

Signed-off-by: Laxman Dewangan <ldewangan-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 arch/arm/boot/dts/tegra20.dtsi |    7 +++++++
 arch/arm/boot/dts/tegra30.dtsi |    7 +++++++
 2 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/arch/arm/boot/dts/tegra20.dtsi b/arch/arm/boot/dts/tegra20.dtsi
index fe35c72..133d00d 100644
--- a/arch/arm/boot/dts/tegra20.dtsi
+++ b/arch/arm/boot/dts/tegra20.dtsi
@@ -352,6 +352,13 @@
 		status = "disabled";
 	};
 
+	kbc {
+		compatible = "nvidia,tegra20-kbc";
+		reg = <0x7000e200 0x100>;
+		interrupts = <0 85 0x04>;
+		status = "disabled";
+	};
+
 	pmc {
 		compatible = "nvidia,tegra20-pmc";
 		reg = <0x7000e400 0x400>;
diff --git a/arch/arm/boot/dts/tegra30.dtsi b/arch/arm/boot/dts/tegra30.dtsi
index f534a50..d060e46 100644
--- a/arch/arm/boot/dts/tegra30.dtsi
+++ b/arch/arm/boot/dts/tegra30.dtsi
@@ -368,6 +368,13 @@
 		status = "disabled";
 	};
 
+	kbc {
+		compatible = "nvidia,tegra30-kbc", "nvidia,tegra20-kbc";
+		reg = <0x7000e200 0x100>;
+		interrupts = <0 85 0x04>;
+		status = "disabled";
+	};
+
 	pmc {
 		compatible = "nvidia,tegra20-pmc", "nvidia,tegra30-pmc";
 		reg = <0x7000e400 0x400>;
-- 
1.7.1.1

[PATCH 2/3] ARM: tegra: config: enable KEYBOARD_TEGRA

[Laxman Dewangan]

Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
---
 arch/arm/configs/tegra_defconfig |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/arch/arm/configs/tegra_defconfig b/arch/arm/configs/tegra_defconfig
index 254f31f..e350afb 100644
--- a/arch/arm/configs/tegra_defconfig
+++ b/arch/arm/configs/tegra_defconfig
@@ -107,6 +107,7 @@ CONFIG_BRCMFMAC=m
 CONFIG_RT2X00=y
 CONFIG_RT2800USB=m
 CONFIG_INPUT_EVDEV=y
+CONFIG_KEYBOARD_TEGRA=y
 CONFIG_INPUT_MISC=y
 CONFIG_INPUT_MPU3050=y
 # CONFIG_LEGACY_PTYS is not set
-- 
1.7.1.1

Re: [PATCH 2/3] ARM: tegra: config: enable KEYBOARD_TEGRA

[Stephen Warren]

On 01/11/2013 06:33 AM, Laxman Dewangan wrote:

Squashed into Tegra's for-3.9/defconfig branch.

[PATCH RFT 3/3] ARM: tegra: dts: seaboard: enable keyboard

[Laxman Dewangan]

Enable tegra based keyboard controller and populate the key matrix for
seaboard. The key matrix was originally on driver code which is removed
to have clean driver. The key mapping is now passed through dts file.

Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
---
Requesting for testing on seaboard.
I generated this patch as Stephen suggested to have one patch for dt file
entry for keys on seaboard.

 arch/arm/boot/dts/tegra20-seaboard.dts |  138 ++++++++++++++++++++++++++++++++
 1 files changed, 138 insertions(+), 0 deletions(-)

diff --git a/arch/arm/boot/dts/tegra20-seaboard.dts b/arch/arm/boot/dts/tegra20-seaboard.dts
index f9e3ad4..b22522d 100644
--- a/arch/arm/boot/dts/tegra20-seaboard.dts
+++ b/arch/arm/boot/dts/tegra20-seaboard.dts
@@ -612,6 +612,144 @@
 		};
 	};
 
+	kbc {
+		status = "okay";
+		nvidia,debounce-delay-ms = <640>;
+		nvidia,repeat-delay-ms = <1>;
+		nvidia,kbc-row-pins = <0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15>;
+		nvidia,kbc-col-pins = <16 17 18 19 20 21 22 23>;
+		linux,keymap = <0x00020011	/* KEY_W */
+				0x0003001F	/* KEY_S */
+				0x0004001E	/* KEY_A */
+				0x0005002C	/* KEY_Z */
+				0x000701d0	/* KEY_FN */
+
+				0x0107007D	/* KEY_LEFTMETA */
+				0x02060064 	/* KEY_RIGHTALT */
+				0x02070038	/* KEY_LEFTALT */
+
+				0x03000006	/* KEY_5 */
+				0x03010005	/* KEY_4 */
+				0x03020013	/* KEY_R */
+				0x03030012	/* KEY_E */
+				0x03040021	/* KEY_F */
+				0x03050020	/* KEY_D */
+				0x0306002D	/* KEY_X */
+
+				0x04000008	/* KEY_7 */
+				0x04010007	/* KEY_6 */
+				0x04020014	/* KEY_T */
+				0x04030023	/* KEY_H */
+				0x04040022	/* KEY_G */
+				0x0405002F	/* KEY_V */
+				0x0406002E	/* KEY_C */
+				0x04070039	/* KEY_SPACE */
+
+				0x0500000A	/* KEY_9 */
+				0x05010009	/* KEY_8 */
+				0x05020016	/* KEY_U */
+				0x05030015	/* KEY_Y */
+				0x05040024	/* KEY_J */
+				0x05050031	/* KEY_N */
+				0x05060030	/* KEY_B */
+				0x0507002B	/* KEY_BACKSLASH */
+
+				0x0600000C	/* KEY_MINUS */
+				0x0601000B	/* KEY_0 */
+				0x06020018	/* KEY_O */
+				0x06030017	/* KEY_I */
+				0x06040026	/* KEY_L */
+				0x06050025	/* KEY_K */
+				0x06060033	/* KEY_COMMA */
+				0x06070032	/* KEY_M */
+
+				0x0701000D	/* KEY_EQUAL */
+				0x0702001B	/* KEY_RIGHTBRACE */
+				0x0703001C	/* KEY_ENTER */
+				0x0707008B	/* KEY_MENU */
+
+				0x08040036	/* KEY_RIGHTSHIFT */
+				0x0805002A	/* KEY_LEFTSHIFT */
+
+				0x09050061	/* KEY_RIGHTCTRL */
+				0x0907001B	/* KEY_LEFTCTRL */
+
+				0x0B00001A	/* KEY_LEFTBRACE */
+				0x0B010019	/* KEY_P */
+				0x0B020028	/* KEY_APOSTROPHE */
+				0x0B030027	/* KEY_SEMICOLON */
+				0x0B040035	/* KEY_SLASH */
+				0x0B050034	/* KEY_DOT */
+
+				0x0C000044	/* KEY_F10 */
+				0x0C010043	/* KEY_F9 */
+				0x0C02000E	/* KEY_BACKSPACE */
+				0x0C030004	/* KEY_3 */
+				0x0C040003	/* KEY_2 */
+				0x0C050067	/* KEY_UP */
+				0x0C0600D2	/* KEY_PRINT */
+				0x0C070077	/* KEY_PAUSE */
+
+				0x0D00006E	/* KEY_INSERT */
+				0x0D01006F	/* KEY_DELETE */
+				0x0D030068	/* KEY_PAGEUP  */
+				0x0D04006D	/* KEY_PAGEDOWN */
+				0x0D05006A	/* KEY_RIGHT */
+				0x0D06006C	/* KEY_DOWN */
+				0x0D070069	/* KEY_LEFT */
+
+				0x0E000057	/* KEY_F11 */
+				0x0E010058	/* KEY_F12 */
+				0x0E020042	/* KEY_F8 */
+				0x0E030010	/* KEY_Q */
+				0x0E04003E	/* KEY_F4 */
+				0x0E05003D	/* KEY_F3 */
+				0x0E060002	/* KEY_1 */1
+				0x0E070041	/* KEY_F7 */
+
+				0x0F000001	/* KEY_ESC */
+				0x0F010029	/* KEY_GRAVE */
+				0x0F02003F	/* KEY_F5 */
+				0x0F03000F	/* KEY_TAB */
+				0x0F04003B	/* KEY_F1 */
+				0x0F05003C	/* KEY_F2 */
+				0x0F06003A	/* KEY_CAPSLOCK */
+				0x0F070040	/* KEY_F6 */
+
+				/* Software Handled Function Keys */
+				0x14000047	/* KEY_KP7 */
+
+				0x15000049	/* KEY_KP9 */
+				0x15010048	/* KEY_KP8 */
+				0x1502004B	/* KEY_KP4 */
+				0x1504004F	/* KEY_KP1 */
+
+				0x1601004E	/* KEY_KPSLASH */
+				0x1602004D	/* KEY_KP6 */
+				0x1603004C	/* KEY_KP5 */
+				0x16040051	/* KEY_KP3 */
+				0x16050050	/* KEY_KP2 */
+				0x16070052	/* KEY_KP0 */
+
+				0x1B010037	/* KEY_KPASTERISK */
+				0x1B03004A	/* KEY_KPMINUS */
+				0x1B04004E	/* KEY_KPPLUS */
+				0x1B050053	/* KEY_KPDOT */
+
+				0x1C050073	/* KEY_VOLUMEUP */
+
+				0x1D030066	/* KEY_HOME */
+				0x1D04006B	/* KEY_END */
+				0x1D0500E0	/* KEY_BRIGHTNESSDOWN */
+				0x1D060072	/* KEY_VOLUMEDOWN */
+				0x1D0700E1	/* KEY_BRIGHTNESSUP */
+
+				0x1E000045	/* KEY_NUMLOCK */
+				0x1E010046	/* KEY_SCROLLLOCK */
+				0x1E020071	/* KEY_MUTE */
+
+				0x1F04008A>;	/* KEY_HELP */
+	};
 	regulators {
 		compatible = "simple-bus";
 		#address-cells = <1>;
-- 
1.7.1.1

Re: [PATCH RFT 3/3] ARM: tegra: dts: seaboard: enable keyboard

[Stephen Warren]

On 01/11/2013 06:33 AM, Laxman Dewangan wrote:
> Enable tegra based keyboard controller and populate the key matrix for
> seaboard. The key matrix was originally on driver code which is removed
> to have clean driver. The key mapping is now passed through dts file.
> 
> Signed-off-by: Laxman Dewangan <ldewangan-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
> Requesting for testing on seaboard.
> I generated this patch as Stephen suggested to have one patch for dt file
> entry for keys on seaboard.

To some extent this works; at least this patch series is probably almost
OK, but there are a bunch of issues in the KBC driver obviously:

This series won't work on its own with the current Tegra for-next
content, because:

a) The clock driver doesn't provide any KBC clock for Tegra20, either in
the old Tegra clock driver, or the first conversion to the common clock
framework, nor the most recent complete conversion to the common clock
framework. Was the driver tested at all on the upstream kernel? I
suppose it's just possible it was tested on Tegra30, although see below...

This brings up the point that the clk_get() call in the KBC driver is
probably wrong. At least with Prashant's latest common clock framework
patches applied, clk_get() returns NULL, not an error pointer for a
non-existent clock. As such, the following driver code succeeds:

> 	kbc->clk = 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;
> 	}

Should that check be if (!kbc-clk) instead? Or does the common clock
framework require if (IS_ERR_OR_NULL(kbc->clk)); hopefully not since
IS_ERR_OR_NULL shouldn't be used any more.

This then causes the later call to tegra_periph_reset_{de,}assert() to
crash since it generates a bogus pointer from the NULL pointer and
dereferences it. I assume similar things happen before Prashant's latest
clock patches.

Oh, and the KBC clock is marked TEGRA_PERIPH_NO_RESET for Tegra30, and
also for Tegra20 in the ChromeOS kernel; why is the driver trying to
assert reset on a clock that doesn't support it?

I hacked around this by adding the following to clk-tegra20.c on top of
Prashant's latest patches:

> 	/* kbc */
> 	clk = tegra_clk_periph_gate("kbc", "clk_32k", TEGRA_PERIPH_NO_RESET |
> 				    TEGRA_PERIPH_ON_APB, clk_base, 0,
> 				    36, &periph_h_regs, periph_clk_enb_refcnt);
> 	clk_register_clkdev(clk, NULL, "tegra-kbc");
> 	clks[kbc] = clk;
> 

b) There is no AUXDATA in the board file, nor clocks properties in the
.dts files, in this series to actually hook the KBC driver up to be able
to get the correct clock. This leads me to suspect the driver was never
tested upstream on either Tegra20 /or/ Tegra30.

I hacked around this by applying the following:

> diff --git a/arch/arm/boot/dts/tegra20.dtsi b/arch/arm/boot/dts/tegra20.dtsi
> index 8e97825..9ca5e94 100644
> --- a/arch/arm/boot/dts/tegra20.dtsi
> +++ b/arch/arm/boot/dts/tegra20.dtsi
> @@ -400,6 +400,7 @@
>                 reg = <0x7000e200 0x100>;
>                 interrupts = <0 85 0x04>;
>                 status = "disabled";
> +               clocks = <&tegra_car 36>;
>         };
>  
>         pmc {
> diff --git a/arch/arm/boot/dts/tegra30.dtsi b/arch/arm/boot/dts/tegra30.dtsi
> index 9dca3e4..e580045 100644
> --- a/arch/arm/boot/dts/tegra30.dtsi
> +++ b/arch/arm/boot/dts/tegra30.dtsi
> @@ -417,6 +417,7 @@
>                 reg = <0x7000e200 0x100>;
>                 interrupts = <0 85 0x04>;
>                 status = "disabled";
> +               clocks = <&tegra_car 36>;
>         };
>  
>         pmc {

I guess I can fold that fix into this series when I apply it, after
Prashant's clock patches are merged.

Once I hacked around the above issues, the driver doesn't work very well
at all. There is a *long* delay between when I press a key and when it
shows up (e.g. echo'd to the HDMI console). When I release the key the
same keypress is generated twice. Or perhaps it's a repeat, but since
it's *always* 2 extra keypresses and I doubt I always hold my finger on
the key the exact same amount of time, I think it's key release that
does this not repeat. I assume this would repro on any board, so you
won't need Seaboard to debug it. Harmony is able to read the keyboard
using the KBC.

Even with the above, I was able to validate that the keymap in the
Seaboard .dts file looks reasonable.

However, please fix the KBC driver...

Re: [PATCH RFT 3/3] ARM: tegra: dts: seaboard: enable keyboard

[Russell King - ARM Linux]

On Fri, Jan 11, 2013 at 04:09:59PM -0700, Stephen Warren wrote:
> On 01/11/2013 06:33 AM, Laxman Dewangan wrote:
> > 	kbc->clk = 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;
> > 	}
> 
> Should that check be if (!kbc-clk) instead? Or does the common clock
> framework require if (IS_ERR_OR_NULL(kbc->clk)); hopefully not since
> IS_ERR_OR_NULL shouldn't be used any more.

/**
 * clk_get - lookup and obtain a reference to a clock producer.
 * @dev: device for clock "consumer"
 * @id: clock consumer ID
 *
 * Returns a struct clk corresponding to the clock producer, or
   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 * valid IS_ERR() condition containing errno.  ...
   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Or, put another way:

	If (!IS_ERR(clk))
		The_Clock_Is_Valid();
	Else
		The_Clock_Is_Invalid();
		The_Error = PTR_ERR(clk);

Re: [PATCH RFT 3/3] ARM: tegra: dts: seaboard: enable keyboard

[Stephen Warren]

On 01/11/2013 04:24 PM, Russell King - ARM Linux wrote:
> On Fri, Jan 11, 2013 at 04:09:59PM -0700, Stephen Warren wrote:
>> On 01/11/2013 06:33 AM, Laxman Dewangan wrote:
>>> 	kbc->clk = 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;
>>> 	}
>>
>> Should that check be if (!kbc-clk) instead? Or does the common clock
>> framework require if (IS_ERR_OR_NULL(kbc->clk)); hopefully not since
>> IS_ERR_OR_NULL shouldn't be used any more.
> 
> /**
>  * clk_get - lookup and obtain a reference to a clock producer.
>  * @dev: device for clock "consumer"
>  * @id: clock consumer ID
>  *
>  * Returns a struct clk corresponding to the clock producer, or
>    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>  * valid IS_ERR() condition containing errno.  ...
>    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 
> Or, put another way:
> 
> 	If (!IS_ERR(clk))
> 		The_Clock_Is_Valid();
> 	Else
> 		The_Clock_Is_Invalid();
> 		The_Error = PTR_ERR(clk);

OK, but that doesn't appear to be what happened in practice.

Re: [PATCH RFT 3/3] ARM: tegra: dts: seaboard: enable keyboard

[Russell King - ARM Linux]

On Fri, Jan 11, 2013 at 04:27:07PM -0700, Stephen Warren wrote:
> On 01/11/2013 04:24 PM, Russell King - ARM Linux wrote:
> > On Fri, Jan 11, 2013 at 04:09:59PM -0700, Stephen Warren wrote:
> >> On 01/11/2013 06:33 AM, Laxman Dewangan wrote:
> >>> 	kbc->clk = 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;
> >>> 	}
> >>
> >> Should that check be if (!kbc-clk) instead? Or does the common clock
> >> framework require if (IS_ERR_OR_NULL(kbc->clk)); hopefully not since
> >> IS_ERR_OR_NULL shouldn't be used any more.
> > 
> > /**
> >  * clk_get - lookup and obtain a reference to a clock producer.
> >  * @dev: device for clock "consumer"
> >  * @id: clock consumer ID
> >  *
> >  * Returns a struct clk corresponding to the clock producer, or
> >    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> >  * valid IS_ERR() condition containing errno.  ...
> >    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > 
> > Or, put another way:
> > 
> > 	If (!IS_ERR(clk))
> > 		The_Clock_Is_Valid();
> > 	Else
> > 		The_Clock_Is_Invalid();
> > 		The_Error = PTR_ERR(clk);
> 
> OK, but that doesn't appear to be what happened in practice.

It's what I've been saying each time I see an abuse, the problem is
people don't care to read the documentation provided let alone
understand the interfaces.

That's precisely why IS_ERR_OR_NULL() is to be removed.  One less
thing for someone to throw a dart at as a selection method to use.
Or maybe roll a dice.  Or whatever way they do seem to choose.
(Whatever that is, it's not based on any sound engineering practice
I can make out.)

Re: [PATCH RFT 3/3] ARM: tegra: dts: seaboard: enable keyboard

[Stephen Warren]

On 01/11/2013 04:36 PM, Russell King - ARM Linux wrote:
> On Fri, Jan 11, 2013 at 04:27:07PM -0700, Stephen Warren wrote:
>> On 01/11/2013 04:24 PM, Russell King - ARM Linux wrote:
>>> On Fri, Jan 11, 2013 at 04:09:59PM -0700, Stephen Warren wrote:
>>>> On 01/11/2013 06:33 AM, Laxman Dewangan wrote:
>>>>> 	kbc->clk = 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;
>>>>> 	}
>>>>
>>>> Should that check be if (!kbc-clk) instead? Or does the common clock
>>>> framework require if (IS_ERR_OR_NULL(kbc->clk)); hopefully not since
>>>> IS_ERR_OR_NULL shouldn't be used any more.
>>>
>>> /**
>>>  * clk_get - lookup and obtain a reference to a clock producer.
>>>  * @dev: device for clock "consumer"
>>>  * @id: clock consumer ID
>>>  *
>>>  * Returns a struct clk corresponding to the clock producer, or
>>>    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>>>  * valid IS_ERR() condition containing errno.  ...
>>>    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>>>
>>> Or, put another way:
>>>
>>> 	If (!IS_ERR(clk))
>>> 		The_Clock_Is_Valid();
>>> 	Else
>>> 		The_Clock_Is_Invalid();
>>> 		The_Error = PTR_ERR(clk);
>>
>> OK, but that doesn't appear to be what happened in practice.
> 
> It's what I've been saying each time I see an abuse, the problem is
> people don't care to read the documentation provided let alone
> understand the interfaces.
> 
> That's precisely why IS_ERR_OR_NULL() is to be removed.  One less
> thing for someone to throw a dart at as a selection method to use.
> Or maybe roll a dice.  Or whatever way they do seem to choose.
> (Whatever that is, it's not based on any sound engineering practice
> I can make out.)

For the record, I did mention that IS_ERR_OR_NULL() should not be the
solution here. And the point of my email to Laxman was that he should go
figure out the answer to my question, which would entail reading the
documentation/code/...

Re: [PATCH RFT 3/3] ARM: tegra: dts: seaboard: enable keyboard

[Laxman Dewangan]

On Saturday 12 January 2013 05:09 AM, Stephen Warren wrote:
> On 01/11/2013 04:36 PM, Russell King - ARM Linux wrote:
>> On Fri, Jan 11, 2013 at 04:27:07PM -0700, Stephen Warren wrote:
>>> On 01/11/2013 04:24 PM, Russell King - ARM Linux wrote:
>>>> On Fri, Jan 11, 2013 at 04:09:59PM -0700, Stephen Warren wrote:
>>>>> On 01/11/2013 06:33 AM, Laxman Dewangan wrote:
>>>>>> 	kbc->clk = 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;
>>>>>> 	}
>>>>> Should that check be if (!kbc-clk) instead? Or does the common clock
>>>>> framework require if (IS_ERR_OR_NULL(kbc->clk)); hopefully not since
>>>>> IS_ERR_OR_NULL shouldn't be used any more.
>>>> /**
>>>>   * clk_get - lookup and obtain a reference to a clock producer.
>>>>   * @dev: device for clock "consumer"
>>>>   * @id: clock consumer ID
>>>>   *
>>>>   * Returns a struct clk corresponding to the clock producer, or
>>>>     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>>>>   * valid IS_ERR() condition containing errno.  ...
>>>>     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>>>>
>>>> Or, put another way:
>>>>
>>>> 	If (!IS_ERR(clk))
>>>> 		The_Clock_Is_Valid();
>>>> 	Else
>>>> 		The_Clock_Is_Invalid();
>>>> 		The_Error = PTR_ERR(clk);
>>> OK, but that doesn't appear to be what happened in practice.
>> It's what I've been saying each time I see an abuse, the problem is
>> people don't care to read the documentation provided let alone
>> understand the interfaces.
>>
>> That's precisely why IS_ERR_OR_NULL() is to be removed.  One less
>> thing for someone to throw a dart at as a selection method to use.
>> Or maybe roll a dice.  Or whatever way they do seem to choose.
>> (Whatever that is, it's not based on any sound engineering practice
>> I can make out.)
> For the record, I did mention that IS_ERR_OR_NULL() should not be the
> solution here. And the point of my email to Laxman was that he should go
> figure out the answer to my question, which would entail reading the
> documentation/code/...

Going through clock driver, the clk_get() should return error pointer or 
valid pointer, atleast not NULL.

Probably our clock driver is returning to NULL and hence issue is 
becasue whole world, it  check for IS_ERR or !IS_ERR() and null clk 
pointer treated as !IS_ERR().

Stephen,
Which is the tree on which Prashant changes are applied. I can debug 
from that tree.

Re: [PATCH RFT 3/3] ARM: tegra: dts: seaboard: enable keyboard

[Russell King - ARM Linux]

On Sat, Jan 12, 2013 at 04:04:28PM +0530, Laxman Dewangan wrote:
> Going through clock driver, the clk_get() should return error pointer or  
> valid pointer, atleast not NULL.

No.  clk_get() will return whatever value the 'clk' pointer found in
the clk_lookup structure contains.  Or it will return an error pointer.

This topic has had enough discussion in the past that I'll say: go and
look up in the archives emails from me on this subject.  I've been very
clear over the years about this.

> Probably our clock driver is returning to NULL and hence issue is  
> becasue whole world, it  check for IS_ERR or !IS_ERR() and null clk  
> pointer treated as !IS_ERR().

Probably not.  As far as _drivers_ go, either the clk cookie is an error
(IS_ERR() is true) _OR_ it is valid to be passed back into the clock API.
You will find statements from me in the past to that: whatever clk_get()
returns which is _not_ an error must be accepted by the rest of the clk
API.

Drivers never dereference the returned clk, so why should they care about
anything other than "is it an error" given by IS_ERR().  (notice the lack
of question mark, that's not a question to be answered.)

Re: [PATCH RFT 3/3] ARM: tegra: dts: seaboard: enable keyboard

[Laxman Dewangan]

On Saturday 12 January 2013 04:17 PM, Russell King - ARM Linux wrote:
> On Sat, Jan 12, 2013 at 04:04:28PM +0530, Laxman Dewangan wrote:
>> Going through clock driver, the clk_get() should return error pointer or
>> valid pointer, atleast not NULL.
> No.  clk_get() will return whatever value the 'clk' pointer found in
> the clk_lookup structure contains.  Or it will return an error pointer.
>
> This topic has had enough discussion in the past that I'll say: go and
> look up in the archives emails from me on this subject.  I've been very
> clear over the years about this.
>
>> Probably our clock driver is returning to NULL and hence issue is
>> becasue whole world, it  check for IS_ERR or !IS_ERR() and null clk
>> pointer treated as !IS_ERR().
> Probably not.  As far as _drivers_ go, either the clk cookie is an error
> (IS_ERR() is true) _OR_ it is valid to be passed back into the clock API.
> You will find statements from me in the past to that: whatever clk_get()
> returns which is _not_ an error must be accepted by the rest of the clk
> API.

Ok, Should we document that after clk_get(), we should check for 
IS_ERR() only for validation. The way you explain earlier.
Documentation does not have this.
/**
         struct clk *clk;
         clk = clk_get(NULL, "my_gateable_clk");

         clk_prepare(clk);
         clk_enable(clk);
**/

The clk.h explain this:
  * Returns a struct clk corresponding to the clock producer, or
  * valid IS_ERR() condition containing errno.


The IS_ERR() return false for the null pointer. So as per you, it should 
be valid if the IS_ERR() false for returned pointer and hence NULL 
pointer should be valid for the clock driver.

Now we need to change the tegra_periph_reset_*() to accept the null pointer.


> Drivers never dereference the returned clk, so why should they care about
> anything other than "is it an error" given by IS_ERR().  (notice the lack
> of question mark, that's not a question to be answered.)

The issue is that information is getting passed to other part fo driver 
where it is dereferencing the pointer. Probably we need to work on the 
reset part also to move as part of clock driver to avoid this crash.

Re: [PATCH RFT 3/3] ARM: tegra: dts: seaboard: enable keyboard

[Stephen Warren]

On 01/12/2013 03:34 AM, Laxman Dewangan wrote:
...
> Which is the tree on which Prashant changes are applied. I can debug
> from that tree.

git://nv-tegra.nvidia.com/user/swarren/linux-2.6 test-ccf-rework-v4

But as I said in some other email in response to Prashant, I'll go and
find out why the clk driver is returning NULL.

(Russell, while the clk API definition may allow NULL to be a legal clk
value, the actual implementation on Tegra at least doesn't consider NULL
to be a legal clk value, so it shouldn't be returned in the first
place). There's no issue here in the comprehension of the clk API.

Re: [PATCH RFT 3/3] ARM: tegra: dts: seaboard: enable keyboard

[Russell King - ARM Linux]

On Mon, Jan 14, 2013 at 09:45:52AM -0700, Stephen Warren wrote:
> On 01/12/2013 03:34 AM, Laxman Dewangan wrote:
> ...
> > Which is the tree on which Prashant changes are applied. I can debug
> > from that tree.
> 
> git://nv-tegra.nvidia.com/user/swarren/linux-2.6 test-ccf-rework-v4
> 
> But as I said in some other email in response to Prashant, I'll go and
> find out why the clk driver is returning NULL.

Exactly - the rule is, the rest of the clk API should accept as valid
anything which clk_get() produces _for that implementation_ which
IS_ERR() is not equal to 1.

What that means is:
- if a platform's clk_get() never returns NULL, then the rest of the clk
  API need not test for that value.

  (If a NULL pointer is passed to one of the clk API functions, crashing
  _is_ _okay_.  Consider what memset(NULL, 0, 4096) would do from the
  kernel - it doesn't test for a NULL pointer just because it can - it
  crashes so you can debug why it was passed a NULL pointer.)

- if a platform's clk_get() does return NULL, then the rest of the clk
  API is expected to deal with a NULL pointer in a way that does _not_
  result in the system crashing.

Or, to put it another way:

	clk = clk_get(...);
	if (IS_ERR(clk))
		return PTR_ERR(clk);

	clk_foo(clk);

for all possible return values of clk_get() that an implementation
_actually_ _intentionally_ returns[*] should never cause an oops.

[*] - bugs excluded.  No bugs were squashed in the creation of this email.

Re: [PATCH RFT 3/3] ARM: tegra: dts: seaboard: enable keyboard

[Laxman Dewangan]

On Saturday 12 January 2013 04:39 AM, Stephen Warren wrote:
> On 01/11/2013 06:33 AM, Laxman Dewangan wrote:
>> Enable tegra based keyboard controller and populate the key matrix for
>> seaboard. The key matrix was originally on driver code which is removed
>> to have clean driver. The key mapping is now passed through dts file.
>>
>> Signed-off-by: Laxman Dewangan <ldewangan-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>> ---
>> Requesting for testing on seaboard.
>> I generated this patch as Stephen suggested to have one patch for dt file
>> entry for keys on seaboard.

Thanks for testing.

> To some extent this works; at least this patch series is probably almost
> OK, but there are a bunch of issues in the KBC driver obviously:
>
> This series won't work on its own with the current Tegra for-next
> content, because:
>
> a) The clock driver doesn't provide any KBC clock for Tegra20, either in
> the old Tegra clock driver, or the first conversion to the common clock
> framework, nor the most recent complete conversion to the common clock
> framework. Was the driver tested at all on the upstream kernel? I
> suppose it's just possible it was tested on Tegra30, although see below...

I have cardhu and Ventana and unfortunately both does not support the 
key pad matrix. Both are on gpio keys.
I tested this patch series (along with recent kbc patches) on t114 
platform Dalmore, it is 3x3 matrix.
Yes I made a hack on the board-dt-* file for getting proper clock. I did 
not send similar change as this will not be very much used when we will 
we in CCF.


> This brings up the point that the clk_get() call in the KBC driver is
> probably wrong. At least with Prashant's latest common clock framework
> patches applied, clk_get() returns NULL, not an error pointer for a
> non-existent clock. As such, the following driver code succeeds:
>
>> 	kbc->clk = 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;
>> 	}
> Should that check be if (!kbc-clk) instead? Or does the common clock
> framework require if (IS_ERR_OR_NULL(kbc->clk)); hopefully not since
> IS_ERR_OR_NULL shouldn't be used any more.

If clk_get() failed then it should be return ptr_err. if not then it 
need to change almost all driver i.e. i2c/spi.
If clk_get(0 returns NULL then should be part of the clock CCF changes 
series to change here also.


> This then causes the later call to tegra_periph_reset_{de,}assert() to
> crash since it generates a bogus pointer from the NULL pointer and
> dereferences it. I assume similar things happen before Prashant's latest
> clock patches.

No, Currently clk_get() returns the PTR_ERR and probe fail. This is well 
tested till now. Something changed with CCF.


>
> Oh, and the KBC clock is marked TEGRA_PERIPH_NO_RESET for Tegra30, and
> also for Tegra20 in the ChromeOS kernel; why is the driver trying to
> assert reset on a clock that doesn't support it?
This is something our kbc controller reset and clock design.
KBC controller is on Always Power ON domain so that it can work even 
when system is in sleep.
The KBC clock is enabled through two places, PMC control register and 
CAR register set.
KBC controller is only reset through PMC control register.
This is not well implemented either on our downstream or in mainline. 
Sometime back I tried to implement it in downstream but was having lots 
of comment and not able to complete this. Possibly we will talk 
internally that how can we implement this.


>
> I hacked around this by adding the following to clk-tegra20.c on top of
> Prashant's latest patches:
>
>> 	/* kbc */
>> 	clk = tegra_clk_periph_gate("kbc", "clk_32k", TEGRA_PERIPH_NO_RESET |
>> 				    TEGRA_PERIPH_ON_APB, clk_base, 0,
>> 				    36, &periph_h_regs, periph_clk_enb_refcnt);
>> 	clk_register_clkdev(clk, NULL, "tegra-kbc");
>> 	clks[kbc] = clk;
>>
> b) There is no AUXDATA in the board file, nor clocks properties in the
> .dts files, in this series to actually hook the KBC driver up to be able
> to get the correct clock. This leads me to suspect the driver was never
> tested upstream on either Tegra20 /or/ Tegra30.

I have my local change for the AUXDATA board file which I did not sent. 
I think CCF does not require and this is the reason. I can sent this if 
it needed.

> I hacked around this by applying the following:

Yes, hack is required here. Either hack in the board file for auxdata or 
in dts file for clock entry when you have ccf ready.

> I guess I can fold that fix into this series when I apply it, after
> Prashant's clock patches are merged.
>
> Once I hacked around the above issues, the driver doesn't work very well
> at all. There is a *long* delay between when I press a key and when it
> shows up (e.g. echo'd to the HDMI console). When I release the key the
> same keypress is generated twice. Or perhaps it's a repeat, but since
> it's *always* 2 extra keypresses and I doubt I always hold my finger on
> the key the exact same amount of time, I think it's key release that
> does this not repeat. I assume this would repro on any board, so you
> won't need Seaboard to debug it. Harmony is able to read the keyboard
> using the KBC.
>
> Even with the above, I was able to validate that the keymap in the
> Seaboard .dts file looks reasonable.
>
> However, please fix the KBC driver...

I did not see any issue with the 3x3 matrix. I think all these about is 
to tuning debaunce and other parameter also. Another thing is that to 
make sure that all pinmuxes are properly configured.

However, again I will try to enable kbc on harmony and run it.

Re: [PATCH RFT 3/3] ARM: tegra: dts: seaboard: enable keyboard

[Stephen Warren]

On 01/12/2013 03:02 AM, Laxman Dewangan wrote:
> On Saturday 12 January 2013 04:39 AM, Stephen Warren wrote:
>> On 01/11/2013 06:33 AM, Laxman Dewangan wrote:
>>> Enable tegra based keyboard controller and populate the key matrix for
>>> seaboard. The key matrix was originally on driver code which is removed
>>> to have clean driver. The key mapping is now passed through dts file.
>>>
>>> Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
>>> ---
>>> Requesting for testing on seaboard.
>>> I generated this patch as Stephen suggested to have one patch for dt
>>> file
>>> entry for keys on seaboard.
> 
> Thanks for testing.

>> Oh, and the KBC clock is marked TEGRA_PERIPH_NO_RESET for Tegra30, and
>> also for Tegra20 in the ChromeOS kernel; why is the driver trying to
>> assert reset on a clock that doesn't support it?
>
> This is something our kbc controller reset and clock design.
> KBC controller is on Always Power ON domain so that it can work even
> when system is in sleep.
> The KBC clock is enabled through two places, PMC control register and
> CAR register set.
> KBC controller is only reset through PMC control register.
> This is not well implemented either on our downstream or in mainline.
> Sometime back I tried to implement it in downstream but was having lots
> of comment and not able to complete this. Possibly we will talk
> internally that how can we implement this.

I guess there's little point in the KBC driver calling the Tegra clock
reset APIs then. Still, since the code is already there and it's not
doing any harm, I guess it's fine to leave it there; if we ever enhance
the clock driver to implement reset for those clocks via the PMC, it
will all just magically work.

>> Once I hacked around the above issues, the driver doesn't work very well
>> at all. There is a *long* delay between when I press a key and when it
>> shows up (e.g. echo'd to the HDMI console). When I release the key the
>> same keypress is generated twice. Or perhaps it's a repeat, but since
>> it's *always* 2 extra keypresses and I doubt I always hold my finger on
>> the key the exact same amount of time, I think it's key release that
>> does this not repeat. I assume this would repro on any board, so you
>> won't need Seaboard to debug it. Harmony is able to read the keyboard
>> using the KBC.
>>
>> Even with the above, I was able to validate that the keymap in the
>> Seaboard .dts file looks reasonable.
>>
>> However, please fix the KBC driver...
> 
> I did not see any issue with the 3x3 matrix. I think all these about is
> to tuning debaunce and other parameter also. Another thing is that to
> make sure that all pinmuxes are properly configured.

OK, I'll go look at some downstream kernels for Seaboard and see if the
debounce etc. parameters all match up.

> However, again I will try to enable kbc on harmony and run it.

That would be extremely useful; I worry that if you tested it on
Tegra114, that means using a downstream kernel only, and also whether
there are any HW differences between SoC versions. That's quite a lot of
variables that could explain the issues I'm seeing.

Re: [PATCH RFT 3/3] ARM: tegra: dts: seaboard: enable keyboard

[Stephen Warren]

On 01/14/2013 03:09 PM, Stephen Warren wrote:
> On 01/12/2013 03:02 AM, Laxman Dewangan wrote:
>> On Saturday 12 January 2013 04:39 AM, Stephen Warren wrote:
>>> On 01/11/2013 06:33 AM, Laxman Dewangan wrote:
>>>> Enable tegra based keyboard controller and populate the key matrix for
>>>> seaboard. The key matrix was originally on driver code which is removed
>>>> to have clean driver. The key mapping is now passed through dts file.
>>>>
>>>> Signed-off-by: Laxman Dewangan <ldewangan-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>>>> ---
>>>> Requesting for testing on seaboard.
>>>> I generated this patch as Stephen suggested to have one patch for dt
>>>> file
>>>> entry for keys on seaboard.
>>
>> Thanks for testing.

>>> Once I hacked around the above issues, the driver doesn't work very well
>>> at all. There is a *long* delay between when I press a key and when it
>>> shows up (e.g. echo'd to the HDMI console). When I release the key the
>>> same keypress is generated twice. Or perhaps it's a repeat, but since
>>> it's *always* 2 extra keypresses and I doubt I always hold my finger on
>>> the key the exact same amount of time, I think it's key release that
>>> does this not repeat. I assume this would repro on any board, so you
>>> won't need Seaboard to debug it. Harmony is able to read the keyboard
>>> using the KBC.
>>>
>>> Even with the above, I was able to validate that the keymap in the
>>> Seaboard .dts file looks reasonable.
>>>
>>> However, please fix the KBC driver...
>>
>> I did not see any issue with the 3x3 matrix. I think all these about is
>> to tuning debaunce and other parameter also. Another thing is that to
>> make sure that all pinmuxes are properly configured.
> 
> OK, I'll go look at some downstream kernels for Seaboard and see if the
> debounce etc. parameters all match up.

How odd. The problems were indeed solved by using the downstream
debounce/... timings. It's odd that using a shorter debounce time leads
to /not/ generating spurious keypress events; perhaps the HW is buggy
for the extremely large debounce value that you chose.

I also noticed that one entry in the keymap had a typo.

Can you please roll the patch below into yours:

diff --git a/arch/arm/boot/dts/tegra20-seaboard.dts
b/arch/arm/boot/dts/tegra20-seaboard.dts
index b22522d..2e87330 100644
--- a/arch/arm/boot/dts/tegra20-seaboard.dts
+++ b/arch/arm/boot/dts/tegra20-seaboard.dts
@@ -614,8 +614,9 @@

        kbc {
                status = "okay";
-               nvidia,debounce-delay-ms = <640>;
-               nvidia,repeat-delay-ms = <1>;
+               nvidia,debounce-delay-ms = <32>;
+               nvidia,repeat-delay-ms = <160>;
+               nvidia,ghost-filter;
                nvidia,kbc-row-pins = <0 1 2 3 4 5 6 7 8 9 10 11 12 13
14 15>;
                nvidia,kbc-col-pins = <16 17 18 19 20 21 22 23>;
                linux,keymap = <0x00020011      /* KEY_W */
@@ -672,7 +673,7 @@
                                0x0805002A      /* KEY_LEFTSHIFT */

                                0x09050061      /* KEY_RIGHTCTRL */
-                               0x0907001B      /* KEY_LEFTCTRL */
+                               0x0907001D      /* KEY_LEFTCTRL */

                                0x0B00001A      /* KEY_LEFTBRACE */
                                0x0B010019      /* KEY_P */

Re: [PATCH 1/3] ARM: DT: tegra: add DT entry for KBC controller

[Stephen Warren]

On 01/11/2013 06:33 AM, Laxman Dewangan wrote:
> NVIDIA's Tegra SoCs have the matrix keyboard controller which
> supports 16x8 type of matrix. The number of rows and columns
> are configurable.
> 
> Add DT entry for KBC controller.

Applied to Tegra's for-3.9/dt branch.

I added the required clocks property for this patch to work with
Prashant's common clock framework port, although it's commented out so
far, until the CCF rework is actually applied.