Message-ID: <420e5184-fe35-4725-95c4-782cc096252f@collabora.com>
Date: Fri, 19 Jan 2024 14:19:52 +0100
From: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>
To: Chen-Yu Tsai <wenst@...omium.org>
Cc: Matthias Brugger <matthias.bgg@...il.com>,
 Rob Herring <robh+dt@...nel.org>,
 Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
 Conor Dooley <conor+dt@...nel.org>, devicetree@...r.kernel.org,
 linux-mediatek@...ts.infradead.org, linux-arm-kernel@...ts.infradead.org,
 linux-kernel@...r.kernel.org, Eugen Hristev <eugen.hristev@...labora.com>,
 Conor Dooley <conor.dooley@...rochip.com>
Subject: Re: [PATCH v4 6/9] arm64: dts: mediatek: Add MT8186 Krabby platform
 based Tentacruel / Tentacool

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@...labora.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@...omium.org>
>>> Acked-by: Conor Dooley <conor.dooley@...rochip.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@15;
>>> +
>>> +     trackpad@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@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@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

Powered by blists - more mailing lists