[PATCH v4 6/9] arm64: dts: mediatek: Add MT8186 Krabby platform based Tentacruel / Tentacool
AngeloGioacchino Del Regno
angelogioacchino.delregno at collabora.com
Fri Jan 19 05:19:52 PST 2024
Il 14/12/23 15:37, Chen-Yu Tsai ha scritto:
> On Thu, Dec 14, 2023 at 7:31 PM AngeloGioacchino Del Regno
> <angelogioacchino.delregno at collabora.com> wrote:
>>
>> Il 13/12/23 16:04, Chen-Yu Tsai ha scritto:
>>> Tentacruel and Tentacool are MT8186 based Chromebooks based on the
>>> Krabby design.
>>>
>>> Tentacruel, also known as the ASUS Chromebook CM14 Flip CM1402F, is a
>>> convertible device with touchscreen and stylus.
>>>
>>> Tentacool, also known as the ASUS Chromebook CM14 CM1402C, is a laptop
>>> device. It does not have a touchscreen or stylus.
>>>
>>> The two devices both have two variants. The difference is a second
>>> source touchpad controller that shares the same address as the original,
>>> but is incompatible.
>>>
>>> The extra SKU IDs for the Tentacruel devices map to different sensor
>>> components attached to the Embedded Controller. These are not visible
>>> to the main processor.
>>>
>>> Signed-off-by: Chen-Yu Tsai <wenst at chromium.org>
>>> Acked-by: Conor Dooley <conor.dooley at microchip.com>
>>> ---
>>> Changes since v3:
>>> - Reorder some properties to conform better to the newly proposed DT
>>> style guidelines
>>> - Drop unused labels
>>> - Rename bt-sco node name to bt-sco-codec
>>> - Drop i2s*-share properties from afe node
>>> - Drop aud_gpio_tdm_{on,off} pinctrl nodes
>>> - Replace interrupts with interrupts-extended in tpm node
>>> - Enable adsp device
>>>
>>> Changes since v2:
>>> - Picked up Conor's ack
>>> - Rename touchpad to trackpad
>>> - Drop pinctrl properties from trackpad in tentacruel/tentacool second
>>> source trackpad
>>>
>>> Changes since v1:
>>> - Reorder SKU numbers in descending order.
>>> - Fixed pinconfig node names
>>> - Moved pinctrl-* properties after interrupts-*
>>> - Switched to interrupts-extended for external components
>>> - Marked ADSP as explicitly disabled, with a comment explaining that it
>>> stalls the system
>>> - Renamed "touchpad" to "trackpad"
>>> - Dropped bogus "no-laneswap" property from it6505 node
>>> - Moved "realtek,jd-src" property to after all the regulator supplies
>>> - Switched to macros for MT6366 regulator "regulator-allowed-modes"
>>> - Renamed "vgpu" regulator name to allow coupling, with a comment
>>> containing the name used in the design
>>> - Renamed "cr50" node name to "tpm"
>>> - Moved trackpad_pins reference up to i2c2; workaround for second source
>>> component resource sharing.
>>> - Fix copyright year
>>> - Fixed touchscreen supply name
>>> ---
>>> arch/arm64/boot/dts/mediatek/Makefile | 4 +
>>> .../dts/mediatek/mt8186-corsola-krabby.dtsi | 129 ++
>>> .../mt8186-corsola-tentacool-sku327681.dts | 57 +
>>> .../mt8186-corsola-tentacool-sku327683.dts | 24 +
>>> .../mt8186-corsola-tentacruel-sku262144.dts | 44 +
>>> .../mt8186-corsola-tentacruel-sku262148.dts | 26 +
>>> .../boot/dts/mediatek/mt8186-corsola.dtsi | 1707 +++++++++++++++++
>>> 7 files changed, 1991 insertions(+)
>>> create mode 100644 arch/arm64/boot/dts/mediatek/mt8186-corsola-krabby.dtsi
>>> create mode 100644 arch/arm64/boot/dts/mediatek/mt8186-corsola-tentacool-sku327681.dts
>>> create mode 100644 arch/arm64/boot/dts/mediatek/mt8186-corsola-tentacool-sku327683.dts
>>> create mode 100644 arch/arm64/boot/dts/mediatek/mt8186-corsola-tentacruel-sku262144.dts
>>> create mode 100644 arch/arm64/boot/dts/mediatek/mt8186-corsola-tentacruel-sku262148.dts
>>> create mode 100644 arch/arm64/boot/dts/mediatek/mt8186-corsola.dtsi
>>>
>>> diff --git a/arch/arm64/boot/dts/mediatek/Makefile b/arch/arm64/boot/dts/mediatek/Makefile
>>> index e6e7592a3645..442af61b1305 100644
>>> --- a/arch/arm64/boot/dts/mediatek/Makefile
>>> +++ b/arch/arm64/boot/dts/mediatek/Makefile
>>> @@ -43,6 +43,10 @@ dtb-$(CONFIG_ARCH_MEDIATEK) += mt8183-kukui-kodama-sku32.dtb
>>> dtb-$(CONFIG_ARCH_MEDIATEK) += mt8183-kukui-krane-sku0.dtb
>>> dtb-$(CONFIG_ARCH_MEDIATEK) += mt8183-kukui-krane-sku176.dtb
>>> dtb-$(CONFIG_ARCH_MEDIATEK) += mt8183-pumpkin.dtb
>>> +dtb-$(CONFIG_ARCH_MEDIATEK) += mt8186-corsola-tentacool-sku327681.dtb
>>> +dtb-$(CONFIG_ARCH_MEDIATEK) += mt8186-corsola-tentacool-sku327683.dtb
>>> +dtb-$(CONFIG_ARCH_MEDIATEK) += mt8186-corsola-tentacruel-sku262144.dtb
>>> +dtb-$(CONFIG_ARCH_MEDIATEK) += mt8186-corsola-tentacruel-sku262148.dtb
>>> dtb-$(CONFIG_ARCH_MEDIATEK) += mt8186-evb.dtb
>>> dtb-$(CONFIG_ARCH_MEDIATEK) += mt8192-asurada-hayato-r1.dtb
>>> dtb-$(CONFIG_ARCH_MEDIATEK) += mt8192-asurada-hayato-r5-sku2.dtb
>>
>> ..snip..
>>
>>> diff --git a/arch/arm64/boot/dts/mediatek/mt8186-corsola-tentacruel-sku262148.dts b/arch/arm64/boot/dts/mediatek/mt8186-corsola-tentacruel-sku262148.dts
>>> new file mode 100644
>>> index 000000000000..447b57b12b41
>>> --- /dev/null
>>> +++ b/arch/arm64/boot/dts/mediatek/mt8186-corsola-tentacruel-sku262148.dts
>>> @@ -0,0 +1,26 @@
>>> +// SPDX-License-Identifier: (GPL-2.0 OR MIT)
>>> +/*
>>> + * Copyright 2023 Google LLC
>>> + */
>>> +
>>> +#include "mt8186-corsola-tentacruel-sku262144.dts"
>>> +
>>> +/ {
>>> + compatible = "google,tentacruel-sku262151", "google,tentacruel-sku262150",
>>> + "google,tentacruel-sku262149", "google,tentacruel-sku262148",
>>> + "google,tentacruel", "mediatek,mt8186";
>>> +};
>>> +
>>> +/* This variant replaces only the trackpad controller. */
>>> +&i2c2 {
>>> + /delete-node/ trackpad at 15;
>>> +
>>> + trackpad at 15 {
>>> + compatible = "hid-over-i2c";
>>> + reg = <0x15>;
>>> + interrupts-extended = <&pio 11 IRQ_TYPE_LEVEL_LOW>;
>>> + hid-descr-addr = <0x0001>;
>>> + vdd-supply = <&pp3300_s3>;
>>> + wakeup-source;
>>> + };
>>> +};
>>> diff --git a/arch/arm64/boot/dts/mediatek/mt8186-corsola.dtsi b/arch/arm64/boot/dts/mediatek/mt8186-corsola.dtsi
>>> new file mode 100644
>>> index 000000000000..adbeb0c765d3
>>> --- /dev/null
>>> +++ b/arch/arm64/boot/dts/mediatek/mt8186-corsola.dtsi
>>> @@ -0,0 +1,1707 @@
>>> +// SPDX-License-Identifier: (GPL-2.0 OR MIT)
>>> +/*
>>> + * Copyright (C) 2022 MediaTek Inc.
>>> + */
>>> +/dts-v1/;
>>> +#include "mt8186.dtsi"
>>> +#include <dt-bindings/pinctrl/mt8186-pinfunc.h>
>>> +#include <dt-bindings/gpio/gpio.h>
>>> +#include <dt-bindings/input/input.h>
>>> +#include <dt-bindings/input/gpio-keys.h>
>>> +#include <dt-bindings/regulator/mediatek,mt6397-regulator.h>
>>> +
>>
>> ..snip..
>>
>>> +
>>> +&i2c2 {
>>> + pinctrl-names = "default";
>>> + /*
>>> + * Trackpad pin put here to work around second source components
>>> + * sharing the pinmux in steelix designs.
>>> + */
>>> + pinctrl-0 = <&i2c2_pins>, <&trackpad_pin>;
>>> + clock-frequency = <400000>;
>>> + i2c-scl-internal-delay-ns = <10000>;
>>> + status = "okay";
>>> +
>>> + trackpad at 15 {
>>> + compatible = "elan,ekth3000";
>>
>> You forgot to change this one.
>>
>> Remove compatible from this node and stop using /delete-node/ in device specific
>> devicetrees.
>
> I believe I already replied to the previous version why we can't and shouldn't
> do that.
>
> "elan,ekth3000" and "hid-over-i2c" have incompatible bindings, specifically
> the former uses "vcc-supply" while the latter uses "vdd-supply".
>
> One has to track down if a property is a "sharable" property or not,
> then put it in the correct place, otherwise getting DT binding validation
> errors.
>
> To me it seems cleaner to just delete the node wholesale to not have any
> remnants of the original node, i.e. start from a clean slate. Trying to
> "share" common properties is asking for a headache.
>
> This feels more like a preference thing. I also asked if deleting the node
> through the label would be cleaner, but you didn't reply.
>
I am truly sorry for not replying to that - and for replying to this one very late.
Ok, in that case, I still don't really like seeing /delete-node/ statements... and
unless there is a truly valid reason to have those, can we please just get each
node to each *.dts?
Just to clarify, a valid scenario to have a /delete-node/ statement there would be
the following:
- Same platform, like Corsola in this case
- Four devices
- Three have one trackpad (better if expected more with the same TP)
- One has the other trackpad
Even though I am flexible with that, I just don't see the value in deleting the
trackpad because we've got a 50-50 situation here!
At this point IMO it's worth defining the trackpad twice + twice instead... but
if you expect to have more devices, conforming to the valid scenario that I have
described above, then okay - keep going with /delete-node/.
> Also, "elan,ekth3000" is the one that was designed and tested on the Corsola
> reference design, so I think it rightfully belongs there. The Tentacruel
> SKU replaced it with another part, so the original part should be disabled,
> or in this case, deleted, because otherwise there would be a conflict.
If you really want to document the fact that the Corsola Reference Design
suggests to use a Elan eKTH3000 part, you can always leave a comment in the
dtsi, though, that's interesting mostly only to OEMs... but anyway I'm not
against having a comment, at all.
Cheers,
Angelo
>
>
> Regards
> ChenYu
>
>
>
>>> + reg = <0x15>;
>>> + interrupts-extended = <&pio 11 IRQ_TYPE_LEVEL_LOW>;
>>> + vcc-supply = <&pp3300_s3>;
>>> + wakeup-source;
>>> + };
>>> +};
>>
>>
>> corsola.dtsi (here):
>>
>> &i2c2 {
>> pinctrl-names = "default";
>> /*
>> * Trackpad pin put here to work around second source components
>> * sharing the pinmux in steelix designs.
>> */
>> pinctrl-0 = <&i2c2_pins>, <&trackpad_pin>;
>> clock-frequency = <400000>;
>> i2c-scl-internal-delay-ns = <10000>;
>> status = "okay";
>>
>> trackpad_i2c2_15: trackpad at 15 {
>> /*
>> * Those are common properties for i2c2 trackpad on Corsola boards.
>> * The compatible string is declared in device specific devicetrees
>> */
>> reg = <0x15>;
>> interrupts-extended = <&pio 11 IRQ_TYPE_LEVEL_LOW>;
>> vcc-supply = <&pp3300_s3>;
>> wakeup-source;
>> status = "disabled";
>> };
>> };
>>
>> corsola-some-device.dts:
>>
>> &trackpad_i2c2_15 {
>> compatible = "hid-over-i2c";
>> hid-descr-addr = <0x0001>;
>> status = "okay";
>> };
>>
>> corsola-some-other-device.dts:
>>
>> &trackpad_i2c2_15 {
>> compatible = "elan,ekth3000";
>> status = "okay";
>> };
>>
>> ....everything else looks good.
>>
>> Cheers,
>> Angelo
More information about the linux-arm-kernel
mailing list