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 <ldewan...@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.
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... -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/