Re: [PATCH 1/2] ARM: tegra: add Acer Chromebook 13 device tree

[Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>]

On 08/13/2014 01:07 PM, Dylan Reid wrote:
> On Wed, Aug 13, 2014 at 10:10 AM, Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:
>> On 08/12/2014 07:56 PM, Dylan Reid wrote:
>>>
>>> The Acer Chromebook 13, codenamed "Big", contains an NVIDIA tegra124
>>> processor and is similar to the Venice2 reference platform.
>>>
>>> The keyboard, USB 2, audio, HDMI, sdcard and emmc have been tested
>>> and work on the 1366x768 models.  I haven't tried on the HD systems
>>> yet.
>>>
>>> WiFi does not yet work, it needs at least some PMIC changes to enable
>>> the 32k clock.
>>>
>>> The elan trackpad is not yet functional but hopefully will be soon as
>>> there are patches under review.
>>>
>>> There is also an issue on reboot because the TPM isn't reset.  It will
>>> cause the stock firmware to enter recovery mode.  This can be worked
>>> around by an EC-reset, press refresh and power at the same time.
>>
>>
>>> diff --git a/arch/arm/boot/dts/tegra124-big.dts
>>> b/arch/arm/boot/dts/tegra124-big.dts
>>
>>
>> I think we need to include the SKU name in the filename and compatible value
>> below, or at least plan out that for other SKUs, we'll add the SKU name on.
>>
>>
>>> +/ {
>>> +       model = "Google Big";
>>> +       compatible = "google,nyan-big", "nvidia,tegra124";
>>
>>
>> I think it'd be more user-friendly if the filename and compatible value more
>> obviously tied to the end-user-visible product name.
>
> I'll change the model to "Acer Chromebook 13"

Would mentioning "CB5" in the model be a good idea; that's what it's 
sold as.

> If we need a separate DT for the FullHD skew, then can we add a file
> 'tegra124-nyan-big-full-hd.dtsi'?  We may also add
> 'tegra124-nyan-big-full-hd-touch.dtsi' as well.  They would just
> override the necessary things from this file.

*.dts rather than *.dtsi I suspect? Yes, having a common include file 
and separate top-level files sounds like a good idea.

>>> +       gpio-keys {
>>> +               compatible = "gpio-keys";
>>> +
>>> +               lid {
>>> +                       label = "Lid";
>>> +                       gpios = <&gpio TEGRA_GPIO(R, 4) GPIO_ACTIVE_LOW>;
>>> +                       linux,input-type = <5>;
>>> +                       linux,code = <0>;
>>
>>
>> Aren't there #defines for the 5 and 0 there?
>
> yes for code, apparently not for input-type.  I think 5 means EV_SW,
> maybe that should get added to include/dt-bindings/input somewhere?

Yes, that sounds like a good idea.