On Fri, Oct 13, 2023 at 07:02:27AM +0800, Chen-Yu Tsai wrote:
> Some of the new MediaTek board entries were inserted in a chronological
> order, or just randomly. This makes it harder to search for an entry.
>
> Sort the entries by first grouping by SoC, then sorting by board
> compatible strings. Also add a comment at the top asking people to do
> the same.

Acked-by: Conor Dooley <[email protected]>

Thanks,
Conor.

Attachments:

(No filename) (443.00 B)
signature.asc (235.00 B)
Download all attachments

2023-10-13 15:12:10

by Conor Dooley

[permalink] [raw]

Subject: Re: [PATCH 2/9] dt-bindings: arm: mediatek: Add MT8186 Tentacruel / Tentacool Chromebooks

On Fri, Oct 13, 2023 at 07:02:28AM +0800, Chen-Yu Tsai wrote:
> Add entries for MT8186 based Tentacruel / Tentacool Chromebooks. The two
> are based on the same board design: the former is a convertible device
> with a touchscreen, stylus, and some extra buttons; the latter is a
> clamshell device and lacks these additional features.
>
> 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.

Wha? Given your ordering, is a "google,tentacruel-sku262144" a super-set
of "google,tentacruel-sku262145"? If not, this compatible ordering
doesn't make sense. I can't tell from your description, and the
absence of a
items:
- const: google,tentacruel-sku262145
- const: google,tentacruel-sku262146
- const: google,tentacruel-sku262147
- const: google,tentacruel
- const: mediatek,mt8186
suggests that there is no google,tentacruel-sku262145
device?

Cheers,
Conor.

>
> Signed-off-by: Chen-Yu Tsai <[email protected]>
> ---
> .../devicetree/bindings/arm/mediatek.yaml | 26 +++++++++++++++++++
> 1 file changed, 26 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/arm/mediatek.yaml b/Documentation/devicetree/bindings/arm/mediatek.yaml
> index 60337b439744..aa7e6734b336 100644
> --- a/Documentation/devicetree/bindings/arm/mediatek.yaml
> +++ b/Documentation/devicetree/bindings/arm/mediatek.yaml
> @@ -206,6 +206,32 @@ properties:
> - enum:
> - mediatek,mt8183-pumpkin
> - const: mediatek,mt8183
> + - description: Google Tentacruel (ASUS Chromebook CM14 Flip CM1402F)
> + items:
> + - const: google,tentacruel-sku262144
> + - const: google,tentacruel-sku262145
> + - const: google,tentacruel-sku262146
> + - const: google,tentacruel-sku262147
> + - const: google,tentacruel
> + - const: mediatek,mt8186
> + - description: Google Tentacruel (ASUS Chromebook CM14 Flip CM1402F)
> + items:
> + - const: google,tentacruel-sku262148
> + - const: google,tentacruel-sku262149
> + - const: google,tentacruel-sku262150
> + - const: google,tentacruel-sku262151
> + - const: google,tentacruel
> + - const: mediatek,mt8186
> + - description: Google Tentacool (ASUS Chromebook CM14 CM1402C)
> + items:
> + - const: google,tentacruel-sku327681
> + - const: google,tentacruel
> + - const: mediatek,mt8186
> + - description: Google Tentacool (ASUS Chromebook CM14 CM1402C)
> + items:
> + - const: google,tentacruel-sku327683
> + - const: google,tentacruel
> + - const: mediatek,mt8186
> - items:
> - enum:
> - mediatek,mt8186-evb
> --
> 2.42.0.655.g421f12c284-goog
>

Attachments:

(No filename) (3.05 kB)
signature.asc (235.00 B)
Download all attachments

2023-10-13 15:12:44

On Fri, Oct 13, 2023 at 10:29:25AM -0700, Chen-Yu Tsai wrote:
> On Fri, Oct 13, 2023 at 8:11 AM Conor Dooley <[email protected]> wrote:
> >
> > On Fri, Oct 13, 2023 at 07:02:28AM +0800, Chen-Yu Tsai wrote:
> > > Add entries for MT8186 based Tentacruel / Tentacool Chromebooks. The two
> > > are based on the same board design: the former is a convertible device
> > > with a touchscreen, stylus, and some extra buttons; the latter is a
> > > clamshell device and lacks these additional features.
> > >
> > > 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.
> >
> > Wha? Given your ordering, is a "google,tentacruel-sku262144" a super-set
> > of "google,tentacruel-sku262145"? If not, this compatible ordering
> > doesn't make sense. I can't tell from your description, and the
> > absence of a
> > items:
> > - const: google,tentacruel-sku262145
> > - const: google,tentacruel-sku262146
> > - const: google,tentacruel-sku262147
> > - const: google,tentacruel
> > - const: mediatek,mt8186
> > suggests that there is no google,tentacruel-sku262145
> > device?
>
> AFAIK all four SKUs exist. And as far as the main processor is concerned,
> they look completely identical, so they should share the same device tree.
> As mentioned in the commit message, the differences are only visible to
> the embedded controller, which fuses the sensor inputs.

Then it makes very little sense to write a binding like this.
If this was just for the 252144 SKU, this would be fine.
For the other SKUs, there is no way to uniquely identify them, as
all four of google,tentacruel-sku262144, google,tentacruel-sku262145,
google,tentacruel-sku262146 and google,tentacruel-sku262147 must be
present.
Given that, why even bother including the SKUs in the first place,
since no information can be derived from them that cannot be derived
from google,tentacruel?
There's something that I am clearly missing here...

Also, why is the order inverted, with the lower SKUs being super-sets of
the higher ones? The Hana one you show below makes a little more sense
in that regard.

> Writing it this way avoids having four identical device tree files.
>
> We also do this for many other device families, though those cover
> different revisions, such as:
>
> - description: Google Hana (Lenovo Chromebook N23 Yoga, C330, 300e,...)
> items:
> - const: google,hana-rev6
> - const: google,hana-rev5
> - const: google,hana-rev4
> - const: google,hana-rev3
> - const: google,hana
> - const: mediatek,mt8173
>
>
> ChenYu
>
> > Cheers,
> > Conor.
> >
> > >
> > > Signed-off-by: Chen-Yu Tsai <[email protected]>
> > > ---
> > > .../devicetree/bindings/arm/mediatek.yaml | 26 +++++++++++++++++++
> > > 1 file changed, 26 insertions(+)
> > >
> > > diff --git a/Documentation/devicetree/bindings/arm/mediatek.yaml b/Documentation/devicetree/bindings/arm/mediatek.yaml
> > > index 60337b439744..aa7e6734b336 100644
> > > --- a/Documentation/devicetree/bindings/arm/mediatek.yaml
> > > +++ b/Documentation/devicetree/bindings/arm/mediatek.yaml
> > > @@ -206,6 +206,32 @@ properties:
> > > - enum:
> > > - mediatek,mt8183-pumpkin
> > > - const: mediatek,mt8183
> > > + - description: Google Tentacruel (ASUS Chromebook CM14 Flip CM1402F)
> > > + items:
> > > + - const: google,tentacruel-sku262144
> > > + - const: google,tentacruel-sku262145
> > > + - const: google,tentacruel-sku262146
> > > + - const: google,tentacruel-sku262147
> > > + - const: google,tentacruel
> > > + - const: mediatek,mt8186
> > > + - description: Google Tentacruel (ASUS Chromebook CM14 Flip CM1402F)
> > > + items:
> > > + - const: google,tentacruel-sku262148
> > > + - const: google,tentacruel-sku262149
> > > + - const: google,tentacruel-sku262150
> > > + - const: google,tentacruel-sku262151
> > > + - const: google,tentacruel
> > > + - const: mediatek,mt8186
> > > + - description: Google Tentacool (ASUS Chromebook CM14 CM1402C)
> > > + items:
> > > + - const: google,tentacruel-sku327681
> > > + - const: google,tentacruel
> > > + - const: mediatek,mt8186
> > > + - description: Google Tentacool (ASUS Chromebook CM14 CM1402C)
> > > + items:
> > > + - const: google,tentacruel-sku327683
> > > + - const: google,tentacruel
> > > + - const: mediatek,mt8186
> > > - items:
> > > - enum:
> > > - mediatek,mt8186-evb
> > > --
> > > 2.42.0.655.g421f12c284-goog
> > >

Attachments:

(No filename) (5.02 kB)
signature.asc (235.00 B)
Download all attachments

2023-10-13 18:19:46

by Chen-Yu Tsai

[permalink] [raw]

Subject: Re: [PATCH 2/9] dt-bindings: arm: mediatek: Add MT8186 Tentacruel / Tentacool Chromebooks

On Fri, Oct 13, 2023 at 10:55 AM Conor Dooley <[email protected]> wrote:
>
> On Fri, Oct 13, 2023 at 10:29:25AM -0700, Chen-Yu Tsai wrote:
> > On Fri, Oct 13, 2023 at 8:11 AM Conor Dooley <[email protected]> wrote:
> > >
> > > On Fri, Oct 13, 2023 at 07:02:28AM +0800, Chen-Yu Tsai wrote:
> > > > Add entries for MT8186 based Tentacruel / Tentacool Chromebooks. The two
> > > > are based on the same board design: the former is a convertible device
> > > > with a touchscreen, stylus, and some extra buttons; the latter is a
> > > > clamshell device and lacks these additional features.
> > > >
> > > > 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.
> > >
> > > Wha? Given your ordering, is a "google,tentacruel-sku262144" a super-set
> > > of "google,tentacruel-sku262145"? If not, this compatible ordering
> > > doesn't make sense. I can't tell from your description, and the
> > > absence of a
> > > items:
> > > - const: google,tentacruel-sku262145
> > > - const: google,tentacruel-sku262146
> > > - const: google,tentacruel-sku262147
> > > - const: google,tentacruel
> > > - const: mediatek,mt8186
> > > suggests that there is no google,tentacruel-sku262145
> > > device?
> >
> > AFAIK all four SKUs exist. And as far as the main processor is concerned,
> > they look completely identical, so they should share the same device tree.
> > As mentioned in the commit message, the differences are only visible to
> > the embedded controller, which fuses the sensor inputs.
>
> Then it makes very little sense to write a binding like this.
> If this was just for the 252144 SKU, this would be fine.
> For the other SKUs, there is no way to uniquely identify them, as
> all four of google,tentacruel-sku262144, google,tentacruel-sku262145,
> google,tentacruel-sku262146 and google,tentacruel-sku262147 must be
> present.
> Given that, why even bother including the SKUs in the first place,
> since no information can be derived from them that cannot be derived
> from google,tentacruel?
> There's something that I am clearly missing here...

There are incompatible variants of google,tentacruel. This is why this
patch has four google,tentacruel based entries. Of them, two are Tentacool,
which are clamshell laptops, and two of them are Tentacruel, which are
convertibles.

Within each group there are two variants: the second variant swaps out
the I2C touchpad controller. These two controllers use the same I2C
address but use different compatible strings, so it's not possible to
have them coexist within the same device tree file like we do for many
other second source components.

So the relationship looks like the following:

google,tentacruel --- Tentacruel --- google,tentacruel-sku26214[4567]
| |
| -- google,tentacruel-sku2621{48,49,50,51}
|
-- Tentacool ---- google,tentacruel-sku327681
|
--- google,tentacruel-sku327683

Also, the devices themselves only know their own SKU ID. The firmware
will generate a list of compatible strings like:

google,tentacruel-rev4-sku262144
google,tentacruel-rev4
google,tentacruel-sku262144
google,tentacruel

and try to find a match in the kernel FIT image. The method we currently
use is to include all the applicable board compatible strings.

> Also, why is the order inverted, with the lower SKUs being super-sets of
> the higher ones? The Hana one you show below makes a little more sense
> in that regard.

Either way works. The SKU IDs have no particular order. Nor do the
individual bits in the SKU ID have meaning. They are just arbitrarily
assigned by the manufacturer. They just have to all be present so any
of the SKUs will match.

Hope that explains things better.

Regards
ChenYu

> > Writing it this way avoids having four identical device tree files.
> >
> > We also do this for many other device families, though those cover
> > different revisions, such as:
> >
> > - description: Google Hana (Lenovo Chromebook N23 Yoga, C330, 300e,...)
> > items:
> > - const: google,hana-rev6
> > - const: google,hana-rev5
> > - const: google,hana-rev4
> > - const: google,hana-rev3
> > - const: google,hana
> > - const: mediatek,mt8173
> >
> >
> > ChenYu
> >
> > > Cheers,
> > > Conor.
> > >
> > > >
> > > > Signed-off-by: Chen-Yu Tsai <[email protected]>
> > > > ---
> > > > .../devicetree/bindings/arm/mediatek.yaml | 26 +++++++++++++++++++
> > > > 1 file changed, 26 insertions(+)
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/arm/mediatek.yaml b/Documentation/devicetree/bindings/arm/mediatek.yaml
> > > > index 60337b439744..aa7e6734b336 100644
> > > > --- a/Documentation/devicetree/bindings/arm/mediatek.yaml
> > > > +++ b/Documentation/devicetree/bindings/arm/mediatek.yaml
> > > > @@ -206,6 +206,32 @@ properties:
> > > > - enum:
> > > > - mediatek,mt8183-pumpkin
> > > > - const: mediatek,mt8183
> > > > + - description: Google Tentacruel (ASUS Chromebook CM14 Flip CM1402F)
> > > > + items:
> > > > + - const: google,tentacruel-sku262144
> > > > + - const: google,tentacruel-sku262145
> > > > + - const: google,tentacruel-sku262146
> > > > + - const: google,tentacruel-sku262147
> > > > + - const: google,tentacruel
> > > > + - const: mediatek,mt8186
> > > > + - description: Google Tentacruel (ASUS Chromebook CM14 Flip CM1402F)
> > > > + items:
> > > > + - const: google,tentacruel-sku262148
> > > > + - const: google,tentacruel-sku262149
> > > > + - const: google,tentacruel-sku262150
> > > > + - const: google,tentacruel-sku262151
> > > > + - const: google,tentacruel
> > > > + - const: mediatek,mt8186
> > > > + - description: Google Tentacool (ASUS Chromebook CM14 CM1402C)
> > > > + items:
> > > > + - const: google,tentacruel-sku327681
> > > > + - const: google,tentacruel
> > > > + - const: mediatek,mt8186
> > > > + - description: Google Tentacool (ASUS Chromebook CM14 CM1402C)
> > > > + items:
> > > > + - const: google,tentacruel-sku327683
> > > > + - const: google,tentacruel
> > > > + - const: mediatek,mt8186
> > > > - items:
> > > > - enum:
> > > > - mediatek,mt8186-evb
> > > > --
> > > > 2.42.0.655.g421f12c284-goog
> > > >

2023-10-14 14:10:44

2023-10-16 17:13:02

by Chen-Yu Tsai

[permalink] [raw]

Subject: Re: [PATCH 6/9] arm64: dts: mediatek: Add MT8186 Krabby platform based Tentacruel / Tentacool

On Mon, Oct 16, 2023 at 2:21 AM Eugen Hristev
<[email protected]> wrote:
>
> On 10/13/23 02:02, Chen-Yu Tsai wrote:
> > 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.
> >
>
> Hi Chen-Yu,
>
> The Krabby design has the touchscreen ? if the touchscreen is for
> Tentacruel ,then the touchscreen should be described in the tentacruel
> dtsi, not in the Krabby and then deleted for tentacool.

Yes. The Krabby design is the reference design and has all the bells and
whistles. Vendors can choose to drop features for their market segmenting
purposes.

In essence the vendors are taking a Google design and tweaking it to suite
their needs. Sometimes things get deleted. Deleting the node seems like
a proper way to describe it.

ChenYu

> Or is there any reason for describing the touchscreen for Krabby ?
> One additional way to solve this is to have the touchsreen as separate
> dtsi and only include it for the boards that have it, e.g. tentacruel .
> Eugen
> > 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.
> >
> [...]
>

2023-10-16 22:09:40

by Chen-Yu Tsai

[permalink] [raw]

Subject: Re: [PATCH 6/9] arm64: dts: mediatek: Add MT8186 Krabby platform based Tentacruel / Tentacool

On Mon, Oct 16, 2023 at 2:19 AM AngeloGioacchino Del Regno
<[email protected]> wrote:
>
> Il 13/10/23 01:02, 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.
> >
>
> Hello Chen-Yu,
>
> Thanks for the patch! However, there are a few things to improve...
>
> the general shape of this looks mostly fine, but there's some comment, check below.
>
> > Signed-off-by: Chen-Yu Tsai <[email protected]>
> > ---
> > arch/arm64/boot/dts/mediatek/Makefile | 4 +
> > .../dts/mediatek/mt8186-corsola-krabby.dtsi | 130 ++
> > .../mt8186-corsola-tentacool-sku327681.dts | 49 +
> > .../mt8186-corsola-tentacool-sku327683.dts | 26 +
> > .../mt8186-corsola-tentacruel-sku262144.dts | 44 +
> > .../mt8186-corsola-tentacruel-sku262148.dts | 28 +
> > .../boot/dts/mediatek/mt8186-corsola.dtsi | 1704 +++++++++++++++++
> > 7 files changed, 1985 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
> > diff --git a/arch/arm64/boot/dts/mediatek/mt8186-corsola-krabby.dtsi b/arch/arm64/boot/dts/mediatek/mt8186-corsola-krabby.dtsi
> > new file mode 100644
> > index 000000000000..0586c24f1e12
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/mediatek/mt8186-corsola-krabby.dtsi
> > @@ -0,0 +1,130 @@
> > +// SPDX-License-Identifier: (GPL-2.0 OR MIT)
> > +/*
> > + * Copyright 2023 Google LLC
> > + */
> > +
> > +/dts-v1/;
> > +#include "mt8186-corsola.dtsi"
> > +#include <dt-bindings/gpio/gpio.h>
> > +
> > +/ {
> > + aliases {
> > + i2c4 = &i2c4;
> > + };
> > +};
> > +
> > +&dsi_out {
> > + remote-endpoint = <&ps8640_in>;
> > +};
> > +
> > +&i2c0 {
> > + clock-frequency = <400000>;
> > +
> > + ps8640: edp-bridge@8 {
> > + compatible = "parade,ps8640";
> > + reg = <0x8>;
> > + powerdown-gpios = <&pio 96 GPIO_ACTIVE_LOW>;
> > + reset-gpios = <&pio 98 GPIO_ACTIVE_LOW>;
> > + pinctrl-names = "default";
> > + pinctrl-0 = <&ps8640_pins>;
> > + vdd12-supply = <&mt6366_vrf12_reg>;
> > + vdd33-supply = <&mt6366_vcn33_reg>;
> > +
> > + ports {
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > +
> > + port@0 {
> > + reg = <0>;
> > +
> > + ps8640_in: endpoint {
> > + remote-endpoint = <&dsi_out>;
> > + };
> > + };
> > +
> > + port@1 {
> > + reg = <1>;
> > +
> > + ps8640_out: endpoint {
> > + remote-endpoint = <&panel_in>;
> > + };
> > + };
> > + };
> > +
> > + aux-bus {
> > + panel {
> > + compatible = "edp-panel";
> > + power-supply = <&pp3300_disp_x>;
> > + backlight = <&backlight_lcd0>;
> > +
> > + port {
> > + panel_in: endpoint {
> > + remote-endpoint = <&ps8640_out>;
> > + };
> > + };
> > + };
> > + };
> > + };
> > +};
> > +
> > +&i2c1 {
> > + i2c-scl-internal-delay-ns = <10000>;
> > +
> > + touchscreen: touchscreen@10 {
> > + compatible = "hid-over-i2c";
> > + reg = <0x10>;
> > + interrupts-extended = <&pio 12 IRQ_TYPE_LEVEL_LOW>;
> > + pinctrl-names = "default";
> > + pinctrl-0 = <&touchscreen_pins>;
> > + post-power-on-delay-ms = <10>;
> > + hid-descr-addr = <0x0001>;
> > + vdd-supply = <&pp3300_s3>;
> > + };
> > +};
> > +
> > +&i2c4 {
> > + pinctrl-names = "default";
> > + pinctrl-0 = <&i2c4_pins>;
> > + clock-frequency = <400000>;
> > + status = "okay";
> > +
> > + proximity@28 {
> > + pinctrl-names = "default";
> > + pinctrl-0 = <&sar_sensor_pins>;
>
> pinctrl nodes at the beginning? Noupe. :-)

Moved to under interrupts. Also changed interrupts to interrupts-extended.

> > + compatible = "semtech,sx9324";
> > + reg = <0x28>;
> > + interrupt-parent = <&pio>;
> > + interrupts = <5 IRQ_TYPE_LEVEL_LOW>;
> > + vdd-supply = <&mt6366_vio18_reg>;
> > + svdd-supply = <&mt6366_vio18_reg>;
> > + #io-channel-cells = <1>;
> > + };
> > +};
> > +
> > +&pio {
> > + i2c4_pins: i2c4-pins {
> > + pins-bus {
> > + pinmux = <PINMUX_GPIO136__FUNC_SDA4>,
> > + <PINMUX_GPIO135__FUNC_SCL4>;
> > + bias-disable;
> > + drive-strength = <4>;
> > + input-enable;
> > + };
> > + };
> > +
> > + ps8640_pins: ps8640-pins {
> > + pins {
>
> pins-pwrdn-rst

Done.

> > + pinmux = <PINMUX_GPIO96__FUNC_GPIO96>,
> > + <PINMUX_GPIO98__FUNC_GPIO98>;
> > + output-low;
> > + };
> > + };
> > +
> > + sar_sensor_pins: sar-sensor-pins {
> > + pin-irq {
> > + pinmux = <PINMUX_GPIO5__FUNC_GPIO5>;
> > + input-enable;
> > + bias-pull-up;
> > + };
> > + };
> > +};
> > diff --git a/arch/arm64/boot/dts/mediatek/mt8186-corsola-tentacool-sku327681.dts b/arch/arm64/boot/dts/mediatek/mt8186-corsola-tentacool-sku327681.dts
> > new file mode 100644
> > index 000000000000..8dfc24afabea
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/mediatek/mt8186-corsola-tentacool-sku327681.dts
> > @@ -0,0 +1,49 @@
> > +// SPDX-License-Identifier: (GPL-2.0 OR MIT)
> > +/*
> > + * Copyright 2023 Google LLC
> > + */
> > +
> > +/dts-v1/;
> > +#include "mt8186-corsola-krabby.dtsi"
> > +
> > +/ {
> > + model = "Google Tentacool board";
> > + compatible = "google,tentacruel-sku327681", "google,tentacruel", "mediatek,mt8186";
> > + chassis-type = "laptop";
> > +};
> > +
> > +/delete-node/ &gpio_keys;
> > +/delete-node/ &touchscreen;
>
> You don't need to delete the touchscreen node if you're disabling i2c1 entirely.
> Besides, I'd rather see TS disabled by default, then enabled on a per-board-basis
> as I think that this clarifies that "board X" has touchscreen, but "board Y" does
> not, because it's "enabled" only on "board X".

I don't know. What you're saying kind of makes sense. OOTH we could say
that they were "deleted" from the end product, so deleting the nodes also
makes sense.

> > +
> > +&i2c1 {
> > + status = "disabled";
> > +};
> > +
> > +&keyboard_controller {
> > + function-row-physmap = <
> > + MATRIX_KEY(0x00, 0x02, 0) /* T1 */
> > + MATRIX_KEY(0x03, 0x02, 0) /* T2 */
> > + MATRIX_KEY(0x02, 0x02, 0) /* T3 */
> > + MATRIX_KEY(0x01, 0x02, 0) /* T4 */
> > + MATRIX_KEY(0x03, 0x04, 0) /* T5 */
> > + MATRIX_KEY(0x02, 0x04, 0) /* T6 */
> > + MATRIX_KEY(0x01, 0x04, 0) /* T7 */
> > + MATRIX_KEY(0x02, 0x09, 0) /* T8 */
> > + MATRIX_KEY(0x01, 0x09, 0) /* T9 */
> > + MATRIX_KEY(0x00, 0x04, 0) /* T10 */
> > + >;
> > +
> > + linux,keymap = <
> > + MATRIX_KEY(0x00, 0x02, KEY_BACK)
> > + MATRIX_KEY(0x03, 0x02, KEY_REFRESH)
> > + MATRIX_KEY(0x02, 0x02, KEY_ZOOM)
> > + MATRIX_KEY(0x01, 0x02, KEY_SCALE)
> > + MATRIX_KEY(0x03, 0x04, KEY_SYSRQ)
> > + MATRIX_KEY(0x02, 0x04, KEY_BRIGHTNESSDOWN)
> > + MATRIX_KEY(0x01, 0x04, KEY_BRIGHTNESSUP)
> > + MATRIX_KEY(0x02, 0x09, KEY_MUTE)
> > + MATRIX_KEY(0x01, 0x09, KEY_VOLUMEDOWN)
> > + MATRIX_KEY(0x00, 0x04, KEY_VOLUMEUP)
> > + CROS_STD_MAIN_KEYMAP
> > + >;
> > +};
> > diff --git a/arch/arm64/boot/dts/mediatek/mt8186-corsola-tentacool-sku327683.dts b/arch/arm64/boot/dts/mediatek/mt8186-corsola-tentacool-sku327683.dts
> > new file mode 100644
> > index 000000000000..9b10336ede06
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/mediatek/mt8186-corsola-tentacool-sku327683.dts
> > @@ -0,0 +1,26 @@
> > +// SPDX-License-Identifier: (GPL-2.0 OR MIT)
> > +/*
> > + * Copyright 2023 Google LLC
> > + */
> > +
> > +#include "mt8186-corsola-tentacool-sku327681.dts"
> > +
> > +/ {
> > + compatible = "google,tentacruel-sku327683", "google,tentacruel", "mediatek,mt8186";
> > +};
> > +
> > +/* This variant replaces only the touchpad controller. */
> > +&i2c2 {
> > + /delete-node/ touchpad@15;
> > +
> > + touchpad@15 {
> > + pinctrl-names = "default";
> > + pinctrl-0 = <&trackpad_pin>;
> > + compatible = "hid-over-i2c";
> > + reg = <0x15>;
> > + interrupts-extended = <&pio 11 IRQ_TYPE_LEVEL_LOW>;
> > + hid-descr-addr = <0x0001>;
> > + vcc-supply = <&pp3300_s3>;
> > + wakeup-source;
> > + };
> > +};
> > diff --git a/arch/arm64/boot/dts/mediatek/mt8186-corsola-tentacruel-sku262144.dts b/arch/arm64/boot/dts/mediatek/mt8186-corsola-tentacruel-sku262144.dts
> > new file mode 100644
> > index 000000000000..26e9f19758cb
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/mediatek/mt8186-corsola-tentacruel-sku262144.dts
> > @@ -0,0 +1,44 @@
> > +// SPDX-License-Identifier: (GPL-2.0 OR MIT)
> > +/*
> > + * Copyright 2022 Google LLC
> > + */
> > +
> > +/dts-v1/;
> > +#include "mt8186-corsola-krabby.dtsi"
> > +
> > +/ {
> > + model = "Google Tentacruel board";
> > + compatible = "google,tentacruel-sku262144", "google,tentacruel-sku262145",
> > + "google,tentacruel-sku262146", "google,tentacruel-sku262147",
> > + "google,tentacruel", "mediatek,mt8186";
> > + chassis-type = "convertible";
> > +};
> > +
> > +&keyboard_controller {
> > + function-row-physmap = <
> > + MATRIX_KEY(0x00, 0x02, 0) /* T1 */
> > + MATRIX_KEY(0x03, 0x02, 0) /* T2 */
> > + MATRIX_KEY(0x02, 0x02, 0) /* T3 */
> > + MATRIX_KEY(0x01, 0x02, 0) /* T4 */
> > + MATRIX_KEY(0x03, 0x04, 0) /* T5 */
> > + MATRIX_KEY(0x02, 0x04, 0) /* T6 */
> > + MATRIX_KEY(0x01, 0x04, 0) /* T7 */
> > + MATRIX_KEY(0x02, 0x09, 0) /* T8 */
> > + MATRIX_KEY(0x01, 0x09, 0) /* T9 */
> > + MATRIX_KEY(0x00, 0x04, 0) /* T10 */
> > + >;
> > +
> > + linux,keymap = <
> > + MATRIX_KEY(0x00, 0x02, KEY_BACK)
> > + MATRIX_KEY(0x03, 0x02, KEY_REFRESH)
> > + MATRIX_KEY(0x02, 0x02, KEY_ZOOM)
> > + MATRIX_KEY(0x01, 0x02, KEY_SCALE)
> > + MATRIX_KEY(0x03, 0x04, KEY_SYSRQ)
> > + MATRIX_KEY(0x02, 0x04, KEY_BRIGHTNESSDOWN)
> > + MATRIX_KEY(0x01, 0x04, KEY_BRIGHTNESSUP)
> > + MATRIX_KEY(0x02, 0x09, KEY_MUTE)
> > + MATRIX_KEY(0x01, 0x09, KEY_VOLUMEDOWN)
> > + MATRIX_KEY(0x00, 0x04, KEY_VOLUMEUP)
> > + CROS_STD_MAIN_KEYMAP
> > + >;
> > +};
> > 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..59d4d21fc7d3
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/mediatek/mt8186-corsola-tentacruel-sku262148.dts
> > @@ -0,0 +1,28 @@
> > +// SPDX-License-Identifier: (GPL-2.0 OR MIT)
> > +/*
> > + * Copyright 2022 Google LLC
> > + */
> > +
> > +#include "mt8186-corsola-tentacruel-sku262144.dts"
> > +
> > +/ {
> > + compatible = "google,tentacruel-sku262144", "google,tentacruel-sku262145",
> > + "google,tentacruel-sku262146", "google,tentacruel-sku262147",
> > + "google,tentacruel", "mediatek,mt8186";
> > +};
> > +
> > +/* This variant replaces only the touchpad controller. */
> > +&i2c2 {
> > + /delete-node/ touchpad@15;
> > +
> > + touchpad@15 {
> > + pinctrl-names = "default";
> > + pinctrl-0 = <&trackpad_pin>;
> > + compatible = "hid-over-i2c";
> > + reg = <0x15>;
> > + interrupts-extended = <&pio 11 IRQ_TYPE_LEVEL_LOW>;
> > + hid-descr-addr = <0x0001>;
> > + vcc-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..42ebab1bafeb
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/mediatek/mt8186-corsola.dtsi
> > @@ -0,0 +1,1704 @@
> > +// 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>
> > +
> > +/ {
> > + aliases {
> > + i2c0 = &i2c0;
> > + i2c1 = &i2c1;
> > + i2c2 = &i2c2;
> > + i2c3 = &i2c3;
> > + i2c5 = &i2c5;
> > + mmc0 = &mmc0;
> > + mmc1 = &mmc1;
> > + serial0 = &uart0;
> > + };
> > +
> > + chosen {
> > + stdout-path = "serial0:115200n8";
> > + };
> > +
> > + memory@40000000 {
> > + device_type = "memory";
> > + /* The size should be filled in by the bootloader. */
> > + reg = <0 0x40000000 0 0>;
> > + };
> > +
> > + backlight_lcd0: backlight-lcd0 {
> > + compatible = "pwm-backlight";
> > + pwms = <&pwm0 0 500000>;
> > + power-supply = <&ppvar_sys>;
> > + enable-gpios = <&pio 152 0>;
> > + brightness-levels = <0 1023>;
> > + num-interpolated-steps = <1023>;
> > + default-brightness-level = <576>;
> > + };
> > +
> > + btsco: bt-sco {
> > + compatible = "linux,bt-sco";
> > + #sound-dai-cells = <0>;
> > + };
> > +
> > + dmic_codec: dmic-codec {
> > + compatible = "dmic-codec";
> > + num-channels = <2>;
> > + wakeup-delay-ms = <50>;
> > + #sound-dai-cells = <0>;
> > + };
> > +
> > + gpio_keys: gpio-keys {
> > + compatible = "gpio-keys";
> > + pinctrl-names = "default";
> > + pinctrl-0 = <&pen_eject>;
> > +
> > + pen_insert: pen-insert-switch {
> > + label = "Pen Insert";
> > + /* Insert = low, eject = high */
> > + gpios = <&pio 18 GPIO_ACTIVE_LOW>;
> > + linux,code = <SW_PEN_INSERTED>;
> > + linux,input-type = <EV_SW>;
> > + wakeup-event-action = <EV_ACT_DEASSERTED>;
> > + wakeup-source;
> > + };
> > + };
> > +
> > + pp1800_dpbrdg_dx: regulator-pp1800-dpbrdg-dx {
> > + compatible = "regulator-fixed";
> > + pinctrl-names = "default";
> > + pinctrl-0 = <&en_pp1800_dpbrdg>;
> > + regulator-name = "pp1800_dpbrdg_dx";
> > + enable-active-high;
> > + gpio = <&pio 39 GPIO_ACTIVE_HIGH>;
> > + vin-supply = <&mt6366_vio18_reg>;
> > + };
> > +
> > + pp3300_disp_x: regulator-pp3300-disp-x {
> > + compatible = "regulator-fixed";
> > + regulator-name = "pp3300_disp_x";
> > + pinctrl-names = "default";
> > + pinctrl-0 = <&edp_panel_fixed_pins>;
> > + enable-active-high;
> > + regulator-boot-on;
> > + gpio = <&pio 153 GPIO_ACTIVE_HIGH>;
> > + vin-supply = <&pp3300_z2>;
> > + };
> > +
> > + /* system wide LDO 3.3V power rail */
> > + pp3300_z5: regulator-pp3300-ldo-z5 {
> > + compatible = "regulator-fixed";
> > + regulator-name = "pp3300_ldo_z5";
> > + regulator-always-on;
> > + regulator-boot-on;
> > + regulator-min-microvolt = <3300000>;
> > + regulator-max-microvolt = <3300000>;
> > + vin-supply = <&ppvar_sys>;
> > + };
> > +
> > + /* separately switched 3.3V power rail */
> > + pp3300_s3: regulator-pp3300-s3 {
> > + compatible = "regulator-fixed";
> > + regulator-name = "pp3300_s3";
> > + /* automatically sequenced by PMIC EXT_PMIC_EN2 */
> > + regulator-always-on;
> > + regulator-boot-on;
> > + vin-supply = <&pp3300_z2>;
> > + };
> > +
> > + /* system wide 3.3V power rail */
> > + pp3300_z2: regulator-pp3300-z2 {
> > + compatible = "regulator-fixed";
> > + regulator-name = "pp3300_z2";
> > + /* EN pin tied to pp4200_z2, which is controlled by EC */
> > + regulator-always-on;
> > + regulator-boot-on;
> > + regulator-min-microvolt = <3300000>;
> > + regulator-max-microvolt = <3300000>;
> > + vin-supply = <&ppvar_sys>;
> > + };
> > +
> > + /* system wide 4.2V power rail */
> > + pp4200_z2: regulator-pp4200-z2 {
> > + compatible = "regulator-fixed";
> > + regulator-name = "pp4200_z2";
> > + /* controlled by EC */
> > + regulator-always-on;
> > + regulator-boot-on;
> > + regulator-min-microvolt = <4200000>;
> > + regulator-max-microvolt = <4200000>;
> > + vin-supply = <&ppvar_sys>;
> > + };
> > +
> > + /* system wide switching 5.0V power rail */
> > + pp5000_z2: regulator-pp5000-z2 {
> > + compatible = "regulator-fixed";
> > + regulator-name = "pp5000_z2";
> > + /* controlled by EC */
> > + regulator-always-on;
> > + regulator-boot-on;
> > + regulator-min-microvolt = <5000000>;
> > + regulator-max-microvolt = <5000000>;
> > + vin-supply = <&ppvar_sys>;
> > + };
> > +
> > + /* system wide semi-regulated power rail from battery or USB */
> > + ppvar_sys: regulator-ppvar-sys {
> > + compatible = "regulator-fixed";
> > + regulator-name = "ppvar_sys";
> > + regulator-always-on;
> > + regulator-boot-on;
> > + };
> > +
> > + reserved_memory: reserved-memory {
> > + #address-cells = <2>;
> > + #size-cells = <2>;
> > + ranges;
> > +
> > + adsp_dma_mem: memory@61000000 {
> > + compatible = "shared-dma-pool";
> > + reg = <0 0x61000000 0 0x100000>;
> > + no-map;
> > + };
> > +
> > + adsp_mem: memory@60000000 {
> > + compatible = "shared-dma-pool";
> > + reg = <0 0x60000000 0 0xA00000>;
> > + no-map;
> > + };
> > +
> > + scp_mem: memory@50000000 {
> > + compatible = "shared-dma-pool";
> > + reg = <0 0x50000000 0 0x10a0000>;
> > + no-map;
> > + };
> > + };
> > +
> > + sound: sound {
> > + compatible = "mediatek,mt8186-mt6366-rt1019-rt5682s-sound";
> > + pinctrl-names = "aud_clk_mosi_off",
> > + "aud_clk_mosi_on",
> > + "aud_clk_miso_off",
> > + "aud_clk_miso_on",
> > + "aud_dat_miso_off",
> > + "aud_dat_miso_on",
> > + "aud_dat_mosi_off",
> > + "aud_dat_mosi_on",
> > + "aud_gpio_i2s0_off",
> > + "aud_gpio_i2s0_on",
> > + "aud_gpio_i2s1_off",
> > + "aud_gpio_i2s1_on",
> > + "aud_gpio_i2s2_off",
> > + "aud_gpio_i2s2_on",
> > + "aud_gpio_i2s3_off",
> > + "aud_gpio_i2s3_on",
> > + "aud_gpio_tdm_off",
> > + "aud_gpio_tdm_on",
> > + "aud_gpio_pcm_off",
> > + "aud_gpio_pcm_on",
> > + "aud_gpio_dmic_sec";
> > + pinctrl-0 = <&aud_clk_mosi_off>;
> > + pinctrl-1 = <&aud_clk_mosi_on>;
> > + pinctrl-2 = <&aud_clk_miso_off>;
> > + pinctrl-3 = <&aud_clk_miso_on>;
> > + pinctrl-4 = <&aud_dat_miso_off>;
> > + pinctrl-5 = <&aud_dat_miso_on>;
> > + pinctrl-6 = <&aud_dat_mosi_off>;
> > + pinctrl-7 = <&aud_dat_mosi_on>;
> > + pinctrl-8 = <&aud_gpio_i2s0_off>;
> > + pinctrl-9 = <&aud_gpio_i2s0_on>;
> > + pinctrl-10 = <&aud_gpio_i2s1_off>;
> > + pinctrl-11 = <&aud_gpio_i2s1_on>;
> > + pinctrl-12 = <&aud_gpio_i2s2_off>;
> > + pinctrl-13 = <&aud_gpio_i2s2_on>;
> > + pinctrl-14 = <&aud_gpio_i2s3_off>;
> > + pinctrl-15 = <&aud_gpio_i2s3_on>;
> > + pinctrl-16 = <&aud_gpio_tdm_off>;
> > + pinctrl-17 = <&aud_gpio_tdm_on>;
> > + pinctrl-18 = <&aud_gpio_pcm_off>;
> > + pinctrl-19 = <&aud_gpio_pcm_on>;
> > + pinctrl-20 = <&aud_gpio_dmic_sec>;
> > + mediatek,adsp = <&adsp>;
> > + mediatek,platform = <&afe>;
> > +
> > + playback-codecs {
> > + sound-dai = <&it6505dptx>, <&rt1019p>;
> > + };
> > +
> > + headset-codec {
> > + sound-dai = <&rt5682s 0>;
> > + };
> > + };
> > +
> > + rt1019p: speaker-codec {
> > + compatible = "realtek,rt1019p";
> > + pinctrl-names = "default";
> > + pinctrl-0 = <&rt1019p_pins_default>;
> > + sdb-gpios = <&pio 150 GPIO_ACTIVE_HIGH>;
> > + #sound-dai-cells = <0>;
> > + };
> > +
> > + usb_p1_vbus: regulator-usb-p1-vbus {
> > + compatible = "regulator-fixed";
> > + regulator-name = "vbus1";
> > + regulator-min-microvolt = <5000000>;
> > + regulator-max-microvolt = <5000000>;
> > + enable-active-high;
> > + gpio = <&pio 148 GPIO_ACTIVE_HIGH>;
> > + vin-supply = <&pp5000_z2>;
> > + };
> > +
> > + wifi_pwrseq: wifi-pwrseq {
> > + compatible = "mmc-pwrseq-simple";
> > + pinctrl-names = "default";
> > + pinctrl-0 = <&wifi_enable_pin>;
> > + post-power-on-delay-ms = <50>;
> > + reset-gpios = <&pio 54 GPIO_ACTIVE_LOW>;
> > + };
> > +
> > + wifi_wakeup: wifi-wakeup {
> > + compatible = "gpio-keys";
> > + pinctrl-names = "default";
> > + pinctrl-0 = <&wifi_wakeup_pin>;
>
> All of them do have a WoWLAN event pin, but not all of them have got a pen
> insertion pin... okay.
>
> So, you should either set status = "disabled" to both and then enable them
> in each board dts file, or you should merge them and delete the pen insertion
> node wherever not present.
>
> Using /delete-node/ is not wrong, of course, but I don't really like seeing
> that... it's probably just boiling down to a personal preference, so, unless
> you have strong opinions against that, I'd like to see the first suggestion
> applied (status = disabled on all, enable on a per-board basis).

The pin insertion "gpio-keys" node is a separate instance. So I assume
your comment applies to that one and not this one?

Merging them and then deleting the pin-insert node also requires overriding
the pinctrl property. Seems more work than necessary.

What you prefer here seems to be the opposite of what we have for the
touchscreen.

> > +
> > + wowlan-event {
> > + label = "Wake on WiFi";
> > + gpios = <&pio 7 GPIO_ACTIVE_LOW>;
> > + linux,code = <KEY_WAKEUP>;
> > + wakeup-source;
> > + };
> > + };
> > +};
> > +
> > +&adsp {
> > + memory-region = <&adsp_dma_mem>, <&adsp_mem>;
> > + status = "fail"; /* causes stall */
>
> The adsp is default disabled, you don't need to set it to "fail", as this is
> designed to work, not to fail, and this can be fixed.
>
> When things cannot be "really" fixed, then I agree on setting status = "fail",
> but again, this is not the case.

Fixed.

> > +};
> > +
> > +&afe {
> > + i2s0-share = "I2S1";
> > + i2s3-share = "I2S2";
> > + status = "okay";
> > +};
> > +
> > +&cci {
> > + proc-supply = <&mt6366_vproc12_reg>;
> > +};
> > +
> > +&cpu0 {
> > + proc-supply = <&mt6366_vproc12_reg>;
> > +};
> > +
> > +&cpu1 {
> > + proc-supply = <&mt6366_vproc12_reg>;
> > +};
> > +
> > +&cpu2 {
> > + proc-supply = <&mt6366_vproc12_reg>;
> > +};
> > +
> > +&cpu3 {
> > + proc-supply = <&mt6366_vproc12_reg>;
> > +};
> > +
> > +&cpu4 {
> > + proc-supply = <&mt6366_vproc12_reg>;
> > +};
> > +
> > +&cpu5 {
> > + proc-supply = <&mt6366_vproc12_reg>;
> > +};
> > +
> > +&cpu6 {
> > + proc-supply = <&mt6366_vproc11_reg>;
> > +};
> > +
> > +&cpu7 {
> > + proc-supply = <&mt6366_vproc11_reg>;
> > +};
> > +
> > +&dpi {
> > + pinctrl-names = "default", "sleep";
> > + pinctrl-0 = <&dpi_pins_default>;
> > + pinctrl-1 = <&dpi_pins_sleep>;
> > + status = "okay";
> > +};
> > +
> > +&dpi_out {
> > + remote-endpoint = <&it6505_in>;
> > +};
> > +
> > +&dsi0 {
> > + status = "okay";
> > +};
> > +
> > +&gic {
> > + mediatek,broken-save-restore-fw;
> > +};
> > +
> > +&gpu {
> > + mali-supply = <&mt6366_vgpu_reg>;
> > + status = "okay";
> > +};
> > +
> > +&i2c0 {
> > + pinctrl-names = "default";
> > + pinctrl-0 = <&i2c0_pins>;
> > + status = "okay";
> > +};
> > +
> > +&i2c1 {
> > + pinctrl-names = "default";
> > + pinctrl-0 = <&i2c1_pins>;
> > + clock-frequency = <400000>;
> > + i2c-scl-internal-delay-ns = <8000>;
> > + status = "okay";
> > +};
> > +
> > +&i2c2 {
> > + pinctrl-names = "default";
> > + pinctrl-0 = <&i2c2_pins>;
> > + clock-frequency = <400000>;
> > + i2c-scl-internal-delay-ns = <10000>;
> > + status = "okay";
> > +
> > + touchpad@15 {
>
> A "touchpad" is not an input device, it is a product name... so, this
> is a "trackpad" instead :-)

Fixed.

> > + compatible = "elan,ekth3000";
>
> Though, this compatible name is getting overridden in multiple cases, even if
> the majority of the properties are effectively the same, as in, the interrupts
> are the same, i2c addr is the same, power supplies are the same, pinctrl....
> ...what I would do here (even though it's a bit strange to see somehow) is...
>
> trackpad: trackpad@15 {
> /* Compatible string is be filled in board specific dts files */
> reg = <0x15>;
> interrupts-extended = <&pio 11 IRQ_TYPE_LEVEL_LOW>;
> pinctrl-names = "default";
> pinctrl-0 = <&trackpad_pin>;
> vcc-supply = <&pp3300_s3>;
> wakeup-source;
> }
>
>
> In tentacruel-sku262144.dts:
>
> &trackpad {
> compatible = "hid-over-i2c";
> hid-descr-addr = <0x0001>;
> };
>
>
> In tentacool-sku327683.dts:
>
> &trackpad {
> compatible = "elan,ekth3000";
> };
>
>
> > + reg = <0x15>;
> > + interrupts-extended = <&pio 11 IRQ_TYPE_LEVEL_LOW>;
> > + pinctrl-names = "default";
> > + pinctrl-0 = <&trackpad_pin>;
> > + vcc-supply = <&pp3300_s3>;
> > + wakeup-source;
> > + };
> > +};
> > +
> > +&i2c3 {
> > + pinctrl-names = "default";
> > + pinctrl-0 = <&i2c3_pins>;
> > + clock-frequency = <100000>;
> > + status = "okay";
> > +
> > + it6505dptx: dp-bridge@5c {
> > + pinctrl-names = "default";
> > + pinctrl-0 = <&it6505_pins>;
> > + compatible = "ite,it6505";
> > + reg = <0x5c>;
> > + interrupts-extended = <&pio 8 IRQ_TYPE_LEVEL_LOW>;
> > + ovdd-supply = <&mt6366_vsim2_reg>;
> > + pwr18-supply = <&pp1800_dpbrdg_dx>;
> > + reset-gpios = <&pio 177 GPIO_ACTIVE_HIGH>;
>
> pinctrl-names, pinctrl-0 here

Actually I don't know what the general preference is, or what makes more
sense.

> > + no-laneswap;
>
> The driver has this property, that's true, but I can't find that in the bindings.
> Also, are you sure that we really need this?
>
> Checking the it6505 driver, the lane swap could also be set/unset with
> EXTCON_PROP_USB_TYPEC_POLARITY, check it6505_get_extcon_property()...
>
> ...but if we really need that property here, the bindings must have it.

This probably needs to be figured out with all the type-C muxing stuff.
I'll drop it.

> > +
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > + #sound-dai-cells = <0>;
> > +
> > + ports {
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > +
> > + port@0 {
> > + reg = <0>;
> > +
> > + it6505_in: endpoint {
> > + link-frequencies = /bits/ 64 <150000000>;
> > + remote-endpoint = <&dpi_out>;
> > + };
> > + };
> > +
> > + port@1 {
> > + reg = <1>;
> > + };
> > + };
> > + };
> > +};
> > +
> > +&i2c5 {
> > + pinctrl-names = "default";
> > + pinctrl-0 = <&i2c5_pins>;
> > + status = "okay";
> > +
> > + rt5682s: codec@1a {
> > + compatible = "realtek,rt5682s";
> > + reg = <0x1a>;
> > + interrupts-extended = <&pio 17 IRQ_TYPE_EDGE_BOTH>;
> > + realtek,jd-src = <1>;
>
> I would move realtek,jd-src...
>
> > + AVDD-supply = <&mt6366_vio18_reg>;
> > + DBVDD-supply = <&mt6366_vio18_reg>;
> > + LDO1-IN-supply = <&mt6366_vio18_reg>;
> > + MICVDD-supply = <&pp3300_z2>;
>
> ...here... but I don't really have strong opinions about that.

I can see some sense in that. Moved.

> > + #sound-dai-cells = <1>;
> > + };
> > +};
> > +
>
> ..snip..
>
> > +
> > + aud_clk_mosi_off: aud-clk-mosi-off-pins {
> > + pins-cmd-dat {
>
> I'd say that those are pins-clk-sync instead.

Fixed.

> > + pinmux = <PINMUX_GPIO166__FUNC_GPIO166>,
> > + <PINMUX_GPIO167__FUNC_GPIO167>;
> > + input-enable;
> > + bias-pull-down;
> > + };
> > + };
> > +
> > + aud_clk_mosi_on: aud-clk-mosi-on-pins {
> > + pins-cmd-dat {
>
> same

Fixed.

> > + pinmux = <PINMUX_GPIO166__FUNC_AUD_CLK_MOSI>,
> > + <PINMUX_GPIO167__FUNC_AUD_SYNC_MOSI>;
> > + };
> > + };
> > +
> > + aud_clk_miso_off: aud-clk-miso-off-pins {
> > + pins-cmd-dat {
>
> and here, and for all pins referencing "FUNC_AUD_CLK" and "FUNC_AUD_SYNC".

Fixed them all.

> > + pinmux = <PINMUX_GPIO170__FUNC_GPIO170>,
> > + <PINMUX_GPIO171__FUNC_GPIO171>;
> > + input-enable;
> > + bias-pull-down;
> > + };
> > + };
> > +
> > + aud_clk_miso_on: aud-clk-miso-on-pins {
> > + pins-cmd-dat {
> > + pinmux = <PINMUX_GPIO170__FUNC_AUD_CLK_MISO>,
> > + <PINMUX_GPIO171__FUNC_AUD_SYNC_MISO>;
> > + };
> > + };
> > +
> > + aud_dat_mosi_off: aud-dat-mosi-off-pins {
> > + pins-cmd-dat {
> > + pinmux = <PINMUX_GPIO168__FUNC_GPIO168>,
> > + <PINMUX_GPIO169__FUNC_GPIO169>;
> > + input-enable;
> > + bias-pull-down;
> > + };
> > + };
> > +
> > + aud_dat_mosi_on: aud-dat-mosi-on-pins {
> > + pins-cmd-dat {
>
> pins-dat or pins-bus

pins-dat it is.

> > + pinmux = <PINMUX_GPIO168__FUNC_AUD_DAT_MOSI0>,
> > + <PINMUX_GPIO169__FUNC_AUD_DAT_MOSI1>;
> > + };
> > + };
> > +
> > + aud_dat_miso_off: aud-dat-miso-off-pins {
> > + pins-cmd-dat {
> > + pinmux = <PINMUX_GPIO172__FUNC_GPIO172>,
> > + <PINMUX_GPIO173__FUNC_GPIO173>;
> > + input-enable;
> > + bias-pull-down;
> > + };
> > + };
> > +
> > + aud_dat_miso_on: aud-dat-miso-on-pins {
> > + pins-cmd-dat {
> > + pinmux = <PINMUX_GPIO172__FUNC_AUD_DAT_MISO0>,
> > + <PINMUX_GPIO173__FUNC_AUD_DAT_MISO1>;
> > + input-schmitt-enable;
> > + bias-disable;
> > + };
> > + };
> > +
> > + aud_gpio_i2s0_off: aud-gpio-i2s0-off-pins {
> > + pins-cmd-dat {
> > + pinmux = <PINMUX_GPIO3__FUNC_GPIO3>;
> > + };
> > + };
> > +
> > + aud_gpio_i2s0_on: aud-gpio-i2s0-on-pins {
> > + pins-cmd-dat {
>
> pins-sdata

Fixed.

> > + pinmux = <PINMUX_GPIO3__FUNC_I2S0_DI>;
> > + };
> > + };
> > +
> > + aud_gpio_i2s1_off: aud-gpio-i2s-off-pins {
> > + pins-cmd-dat {
> > + pinmux = <PINMUX_GPIO56__FUNC_GPIO56>,
> > + <PINMUX_GPIO57__FUNC_GPIO57>,
> > + <PINMUX_GPIO58__FUNC_GPIO58>,
> > + <PINMUX_GPIO59__FUNC_GPIO59>;
> > + output-low;
> > + };
> > + };
> > +
> > + aud_gpio_i2s1_on: aud-gpio-i2s1-on-pins {
> > + pins-cmd-dat {
>
> pins-clk-sdata

Fixed.

> > + pinmux = <PINMUX_GPIO56__FUNC_I2S1_DO>,
> > + <PINMUX_GPIO57__FUNC_I2S1_BCK>,
> > + <PINMUX_GPIO58__FUNC_I2S1_LRCK>,
> > + <PINMUX_GPIO59__FUNC_I2S1_MCK>;
> > + };
> > + };
> > +
> > + aud_gpio_i2s2_off: aud-gpio-i2s2-off-pins {
> > + pins-cmd-dat {
> > + pinmux = <PINMUX_GPIO26__FUNC_GPIO26>,
> > + <PINMUX_GPIO27__FUNC_GPIO27>;
> > + output-low;
> > + };
> > + };
> > +
> > + aud_gpio_i2s2_on: aud-gpio-i2s2-on-pins {
> > + pins-cmd-dat {
>
> Those are bit and word clock pins: pins-clk

Fixed.

> > + pinmux = <PINMUX_GPIO26__FUNC_I2S2_BCK>,
> > + <PINMUX_GPIO27__FUNC_I2S2_LRCK>;
> > + drive-strength = <4>;
> > + };
> > + };
> > +
> > + aud_gpio_i2s3_off: aud-gpio-i2s3-off-pins {
> > + pins-cmd-dat {
> > + pinmux = <PINMUX_GPIO4__FUNC_GPIO4>;
> > + output-low;
> > + };
> > + };
> > +
> > + aud_gpio_i2s3_on: aud-gpio-i2s3-on-pins {
> > + pins-cmd-dat {
>
> pins-sdata

Fixed.

> > + pinmux = <PINMUX_GPIO4__FUNC_I2S3_DO>;
> > + drive-strength = <4>;
> > + };
> > + };
> > +
> > + aud_gpio_tdm_off: aud-gpio-tdm-off-pins { };
> > +
> > + aud_gpio_tdm_on: aud-gpio-tdm-on-pins { };
> > +
> > + aud_gpio_pcm_off: aud-gpio-pcm-off-pins {
> > + pins-cmd-dat {
> > + pinmux = <PINMUX_GPIO115__FUNC_GPIO115>,
> > + <PINMUX_GPIO116__FUNC_GPIO116>,
> > + <PINMUX_GPIO117__FUNC_GPIO117>,
> > + <PINMUX_GPIO118__FUNC_GPIO118>;
> > + output-low;
> > + };
> > + };
> > +
> > + aud_gpio_pcm_on: aud-gpio-pcm-on-pins {
> > + pins-cmd-dat {
>
> pins-clk-dat

pins-clk-sdata to match the rest?

> > + pinmux = <PINMUX_GPIO115__FUNC_PCM_CLK>,
> > + <PINMUX_GPIO116__FUNC_PCM_SYNC>,
> > + <PINMUX_GPIO117__FUNC_PCM_DI>,
> > + <PINMUX_GPIO118__FUNC_PCM_DO>;
> > + };
> > + };
> > +
> > + aud_gpio_dmic_sec: aud-gpio-dmic-sec-pins {
> > + pins {
> > + pinmux = <PINMUX_GPIO23__FUNC_GPIO23>;
> > + output-low;
> > + };
> > + };
> > +
> > + bt_pins_reset: bt-reset-pins {
> > + pins-bt-reset {
> > + pinmux = <PINMUX_GPIO155__FUNC_GPIO155>;
> > + output-high;
> > + };
> > + };
> > +
>
> ..snip..
>
> > +
> > + ec_ap_int: cros-ec-int-pins {
> > + pins1 {
>
> pins-ec-ap-int-odl

Fixed.

> > + pinmux = <PINMUX_GPIO13__FUNC_GPIO13>;
> > + input-enable;
> > + };
> > + };
> > +
> > + edp_panel_fixed_pins: edp-panel-fixed-pins {
> > + pins1 {
>
> pins-vreg-en

Fixed.

> > + pinmux = <PINMUX_GPIO153__FUNC_GPIO153>;
> > + output-high;
> > + };
> > + };
> > +
> > + en_pp1800_dpbrdg: en-pp1800-dpbrdg-pins {
> > + pins {
>
> pins-vreg-en....

Fixed.

> > + pinmux = <PINMUX_GPIO39__FUNC_GPIO39>;
> > + output-low;
> > + };
> > + };
> > +
> > + gsc_ap_int_odl: gsc-ap-int-odl-pins {
>
> cr50_int: cr50-irq-default-pins {
> pins-gsc-ap-int-odl {
> ...
> }
> }

I'd just stick to gsc. The actual model used on these platforms is Ti50
anyway, which is the newer model. Either way they are called the Google
Security Chip, or GSC for short.

gsc_ap_int_odl is also what the line is called in the schematic.

For now I have:

gsc_int: gsc-int-pins {
pins-gsc-ap-int-odl {

What do you think?

> > + pins1 {
> > + pinmux = <PINMUX_GPIO15__FUNC_GPIO15>;
> > + input-enable;
> > + };
> > + };
> > +
>
> ..snip..
>
> > +
> > + mmc1_pins_eint: mmc1-eint-pins {
> > + pins_dat1 {
>
> pins-dat1

Fixed.

> > + pinmux = <PINMUX_GPIO87__FUNC_GPIO87>;
> > + input-enable;
> > + bias-pull-up = <MTK_PUPD_SET_R1R0_01>;
> > + };
> > + };
> > +
> > + nor_pins_default: nor-default-pins {
> > + pins0 {
>
> pins-clk-dat

Fixed.
> > + pinmux = <PINMUX_GPIO63__FUNC_SPINOR_IO0>,
> > + <PINMUX_GPIO61__FUNC_SPINOR_CK>,
> > + <PINMUX_GPIO64__FUNC_SPINOR_IO1>;
> > + drive-strength = <6>;
> > + bias-pull-down;
> > + };
> > +
> > + pins1 {
>
> pins-cs-dat

Fixed.

> > + pinmux = <PINMUX_GPIO62__FUNC_SPINOR_CS>,
> > + <PINMUX_GPIO65__FUNC_SPINOR_IO2>,
> > + <PINMUX_GPIO66__FUNC_SPINOR_IO3>;
> > + drive-strength = <6>;
> > + bias-pull-up;
> > + };
> > + };
> > +
> > + pen_eject: pen-eject-pins {
> > + pins {
> > + pinmux = <PINMUX_GPIO18__FUNC_GPIO18>;
> > + input-enable;
> > + /* External pull-up. */
> > + bias-disable;
> > + };
> > + };
> > +
> > + pwm0_pin: pwm0-default-pins {
>
> pins-disp-pwm

I believe you meant "disp-pwm-pins"?

First level node requires "-pins$", not "^pins".

> > + pins {
> > + pinmux = <PINMUX_GPIO97__FUNC_DISP_PWM>;
> > + output-high;
> > + };
> > + };
> > +
> > + rt1019p_pins_default: rt1019p-default-pins {
> > + pins {
>
> pins-sdb

Fixed.

> > + pinmux = <PINMUX_GPIO150__FUNC_GPIO150>;
> > + output-low;
> > + };
> > + };
> > +
> > + scp_pins: scp-default-pins {
> > + pins-scp-uart {
> > + pinmux = <PINMUX_GPIO48__FUNC_TP_URXD2_AO>,
> > + <PINMUX_GPIO49__FUNC_TP_UTXD2_AO>;
> > + };
> > + };
> > +
> > + spi1_pins: spi1-pins {
> > + pins-spi {
> > + pinmux = <PINMUX_GPIO40__FUNC_SPI1_CLK_A>,
> > + <PINMUX_GPIO41__FUNC_SPI1_CSB_A>,
> > + <PINMUX_GPIO42__FUNC_SPI1_MO_A>,
> > + <PINMUX_GPIO43__FUNC_SPI1_MI_A>;
>
> Shouldn't MISO be pull-down?!
>
> clk, mo, csb: bias-disable
> mi: bias-pull-down

AFAIK SPI is a push-pull interface. No biasing should be used.

> > + bias-disable;
> > + input-enable;
> > + };
> > + };
> > +
> > + spi2_pins: spi2-pins {
> > + pins-spi {
> > + pinmux = <PINMUX_GPIO44__FUNC_SPI2_CLK_A>,
> > + <PINMUX_GPIO45__FUNC_GPIO45>,
> > + <PINMUX_GPIO46__FUNC_SPI2_MO_A>,
> > + <PINMUX_GPIO47__FUNC_SPI2_MI_A>;
>
> Same question here.
>
> P.S.: if yes, pins-cs-mosi-clk, pins-miso; if not, pins-bus.

pins-bus it is.

> > + bias-disable;
> > + input-enable;
> > + };
> > + };
> > +
> > + spmi_pins: spmi-pins {
> > + pins-spmi {
>
> pins-bus

Fixed.

> > + pinmux = <PINMUX_GPIO183__FUNC_SPMI_SCL>,
> > + <PINMUX_GPIO184__FUNC_SPMI_SDA>;
> > + };
> > + };
> > +
> > + touchscreen_pins: touchscreen-pins {
> > + pins-irq {
> > + pinmux = <PINMUX_GPIO12__FUNC_GPIO12>;
> > + input-enable;
> > + bias-pull-up;
> > + };
> > +
> > + pins-reset {
> > + pinmux = <PINMUX_GPIO60__FUNC_GPIO60>;
> > + output-high;
> > + };
> > +
> > + pins-report-sw {
> > + pinmux = <PINMUX_GPIO37__FUNC_GPIO37>;
> > + output-low;
> > + };
> > + };
> > +
> > + trackpad_pin: trackpad-default-pins {
> > + pins-int-n {
> > + pinmux = <PINMUX_GPIO11__FUNC_GPIO11>;
> > + input-enable;
> > + bias-disable; /* pulled externally */
> > + };
> > + };
> > +
> > + wifi_enable_pin: wifi-enable-pins {
> > + pins-wifi-enable {
> > + pinmux = <PINMUX_GPIO54__FUNC_GPIO54>;
> > + };
> > + };
> > +
> > + wifi_wakeup_pin: wifi-wakeup-pins {
> > + pins-wifi-wakeup {
> > + pinmux = <PINMUX_GPIO7__FUNC_GPIO7>;
> > + input-enable;
> > + };
> > + };
> > +};
> > +
> > +&pwm0 {
> > + pinctrl-names = "default";
> > + pinctrl-0 = <&pwm0_pin>;
> > + status = "okay";
> > +};
> > +
> > +&pwrap {
> > + pmic {
> > + compatible = "mediatek,mt6366", "mediatek,mt6358";
> > + interrupt-controller;
> > + interrupts-extended = <&pio 201 IRQ_TYPE_LEVEL_HIGH>;
> > + #interrupt-cells = <2>;
> > +
> > + mt6366codec: codec {
> > + compatible = "mediatek,mt6366-sound", "mediatek,mt6358-sound";
> > + Avdd-supply = <&mt6366_vaud28_reg>;
> > + mediatek,dmic-mode = <1>; /* one-wire */
> > + };
> > +
> > + mt6366_regulators: regulators {
> > + compatible = "mediatek,mt6366-regulator", "mediatek,mt6358-regulator";
> > + vsys-ldo1-supply = <&pp4200_z2>;
> > + vsys-ldo2-supply = <&pp4200_z2>;
> > + vsys-ldo3-supply = <&pp4200_z2>;
> > + vsys-vcore-supply = <&pp4200_z2>;
> > + vsys-vdram1-supply = <&pp4200_z2>;
> > + vsys-vgpu-supply = <&pp4200_z2>;
> > + vsys-vmodem-supply = <&pp4200_z2>;
> > + vsys-vpa-supply = <&pp4200_z2>;
> > + vsys-vproc11-supply = <&pp4200_z2>;
> > + vsys-vproc12-supply = <&pp4200_z2>;
> > + vsys-vs1-supply = <&pp4200_z2>;
> > + vsys-vs2-supply = <&pp4200_z2>;
> > + vs1-ldo1-supply = <&mt6366_vs1_reg>;
> > + vs2-ldo1-supply = <&mt6366_vdram1_reg>;
> > + vs2-ldo2-supply = <&mt6366_vs2_reg>;
> > + vs2-ldo3-supply = <&mt6366_vs2_reg>;
> > +
> > + vcore {
> > + regulator-name = "pp0750_dvdd_core";
> > + regulator-min-microvolt = <550000>;
> > + regulator-max-microvolt = <800000>;
> > + regulator-ramp-delay = <6250>;
> > + regulator-enable-ramp-delay = <200>;
> > + regulator-allowed-modes = <0 1>;

Looks like I need to update this to use the macros from the header file.

> > + regulator-always-on;
> > + };
> > +
> > + mt6366_vdram1_reg: vdram1 {
> > + regulator-name = "pp1125_emi_vdd2";
> > + regulator-min-microvolt = <1125000>;
> > + regulator-max-microvolt = <1125000>;
> > + regulator-ramp-delay = <12500>;
> > + regulator-enable-ramp-delay = <0>;
> > + regulator-allowed-modes = <0 1>;
> > + regulator-always-on;
> > + };
> > +
> > + mt6366_vgpu_reg: vgpu {
> > + regulator-name = "ppvar_dvdd_gpu";
>
> Can we please rename this to "ppvar_dvdd_vgpu"?
> The regulator coupler driver parses "vgpu"... :-)

OK... This is what the name is on the schematic. I'll fix the name
and add a comment for it.

> > + regulator-min-microvolt = <600000>;
> > + regulator-max-microvolt = <950000>;
> > + regulator-ramp-delay = <6250>;
> > + regulator-enable-ramp-delay = <200>;
> > + regulator-allowed-modes = <0 1>;
>
> Couple this regulator with VSRAM, max spread 100000.

Done.

> > + };
> > +
>
> ..snip..
>
> > +
> > + mt6366_vsram_gpu_reg: vsram-gpu {
> > + regulator-name = "pp0900_dvdd_sram_gpu";
> > + regulator-min-microvolt = <850000>;
> > + regulator-max-microvolt = <1050000>;
> > + regulator-ramp-delay = <6250>;
> > + regulator-enable-ramp-delay = <240>;
>
> Couple with VGPU, same max spread.

Done.

> > + };
> > +
> > + mt6366_vsram_others_reg: vsram-others {
> > + regulator-name = "pp0900_dvdd_sram_core";
> > + regulator-min-microvolt = <900000>;
> > + regulator-max-microvolt = <900000>;
> > + regulator-ramp-delay = <6250>;
> > + regulator-enable-ramp-delay = <240>;
> > + regulator-always-on;
> > + };
> > +
> > + mt6366_vsram_proc11_reg: vsram-proc11 {
> > + regulator-name = "pp0900_dvdd_sram_bc";
> > + regulator-min-microvolt = <850000>;
> > + regulator-max-microvolt = <1120000>;
> > + regulator-ramp-delay = <6250>;
> > + regulator-enable-ramp-delay = <240>;
> > + regulator-always-on;
> > + };
> > +
> > + mt6366_vsram_proc12_reg: vsram-proc12 {
> > + regulator-name = "pp0900_dvdd_sram_lc";
> > + regulator-min-microvolt = <850000>;
> > + regulator-max-microvolt = <1120000>;
> > + regulator-ramp-delay = <6250>;
> > + regulator-enable-ramp-delay = <240>;
> > + regulator-always-on;
> > + };
> > +
> > + vusb {
> > + regulator-name = "pp3070_vusb";
> > + regulator-min-microvolt = <3000000>;
> > + regulator-max-microvolt = <3070000>;
> > + regulator-enable-ramp-delay = <270>;
> > + regulator-always-on;
> > + };
> > +
> > + vxo22 {
> > + regulator-name = "pp2240_vxo22";
> > + regulator-min-microvolt = <2200000>;
> > + regulator-max-microvolt = <2240000>;
> > + regulator-enable-ramp-delay = <120>;
> > + /* Feeds DCXO internally */
> > + regulator-always-on;
> > + };
> > + };
> > +
> > + rtc {
> > + compatible = "mediatek,mt6366-rtc", "mediatek,mt6358-rtc";
> > + };
> > + };
> > +};
> > +
> > +&scp {
> > + pinctrl-names = "default";
> > + pinctrl-0 = <&scp_pins>;
> > + firmware-name = "mediatek/mt8186/scp.img";
> > + memory-region = <&scp_mem>;
> > + status = "okay";
> > +
> > + cros-ec-rpmsg {
> > + compatible = "google,cros-ec-rpmsg";
> > + mediatek,rpmsg-name = "cros-ec-rpmsg";
> > + };
> > +};
> > +
> > +&spi1 {
> > + pinctrl-names = "default";
> > + pinctrl-0 = <&spi1_pins>;
> > + mediatek,pad-select = <0>;
> > + status = "okay";
> > +
> > + cros_ec: ec@0 {
> > + compatible = "google,cros-ec-spi";
> > + reg = <0>;
> > + spi-max-frequency = <1000000>;
> > + interrupt-parent = <&pio>;
> > + interrupts = <13 IRQ_TYPE_LEVEL_LOW>;
>
> interrupts-extended please

Fixed.

> > + pinctrl-names = "default";
> > + pinctrl-0 = <&ec_ap_int>;
> > +
> > + i2c_tunnel: i2c-tunnel {
> > + compatible = "google,cros-ec-i2c-tunnel";
> > + google,remote-bus = <1>;
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > + };
> > +
> > + typec {
> > + compatible = "google,cros-ec-typec";
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > +
> > + usb_c0: connector@0 {
> > + compatible = "usb-c-connector";
> > + reg = <0>;
> > + label = "left";
> > + power-role = "dual";
> > + data-role = "host";
> > + try-power-role = "source";
> > +
> > + ports {
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > +
> > + port@1 {
> > + reg = <1>;
> > +
> > + typec_port0: endpoint { };
> > + };
> > + };
> > + };
> > +
> > + usb_c1: connector@1 {
> > + compatible = "usb-c-connector";
> > + reg = <1>;
> > + label = "right";
> > + power-role = "dual";
> > + data-role = "host";
> > + try-power-role = "source";
> > +
> > + ports {
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > +
> > + port@1 {
> > + reg = <1>;
> > +
> > + typec_port1: endpoint { };
> > + };
> > + };
> > + };
> > + };
> > + };
> > +};
> > +
> > +&spi2 {
> > + pinctrl-names = "default";
> > + pinctrl-0 = <&spi2_pins>;
> > + cs-gpios = <&pio 45 GPIO_ACTIVE_LOW>;
> > + mediatek,pad-select = <0>;
> > + status = "okay";
> > +
> > + cr50: cr50@0 {
>
> Do we really need a phandle on cr50?
>
> Also, I'd say tpm@0 instead.

Dropped label and renamed node to tpm.

Thanks for the review. I think at some point I messed up my files and
that resulted in a bunch of things that I fixed up getting reverted.

Regards
ChenYu

> > + compatible = "google,cr50";
> > + reg = <0>;
> > + spi-max-frequency = <1000000>;
> > + pinctrl-names = "default";
> > + pinctrl-0 = <&gsc_ap_int_odl>;
> > + interrupt-parent = <&pio>;
> > + interrupts = <15 IRQ_TYPE_EDGE_RISING>;
> > + };
> > +};
> > +
>
> Cheers!
> Angelo

2023-10-18 15:08:12

On Fri, Oct 13, 2023 at 07:02:28AM +0800, Chen-Yu Tsai wrote:
> Add entries for MT8186 based Tentacruel / Tentacool Chromebooks. The two
> are based on the same board design: the former is a convertible device
> with a touchscreen, stylus, and some extra buttons; the latter is a
> clamshell device and lacks these additional features.
>
> 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 <[email protected]>

I had a chat with Doug at Plumbers about the limitations of your
firmware. As a result, I am fine with acking this if you switch the
order of the sku compatibles to be in descending order.
The firmware can handle that, right?

Cheers,
Conor.

> ---
> .../devicetree/bindings/arm/mediatek.yaml | 26 +++++++++++++++++++
> 1 file changed, 26 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/arm/mediatek.yaml b/Documentation/devicetree/bindings/arm/mediatek.yaml
> index 60337b439744..aa7e6734b336 100644
> --- a/Documentation/devicetree/bindings/arm/mediatek.yaml
> +++ b/Documentation/devicetree/bindings/arm/mediatek.yaml
> @@ -206,6 +206,32 @@ properties:
> - enum:
> - mediatek,mt8183-pumpkin
> - const: mediatek,mt8183
> + - description: Google Tentacruel (ASUS Chromebook CM14 Flip CM1402F)
> + items:
> + - const: google,tentacruel-sku262144
> + - const: google,tentacruel-sku262145
> + - const: google,tentacruel-sku262146
> + - const: google,tentacruel-sku262147
> + - const: google,tentacruel
> + - const: mediatek,mt8186
> + - description: Google Tentacruel (ASUS Chromebook CM14 Flip CM1402F)
> + items:
> + - const: google,tentacruel-sku262148
> + - const: google,tentacruel-sku262149
> + - const: google,tentacruel-sku262150
> + - const: google,tentacruel-sku262151
> + - const: google,tentacruel
> + - const: mediatek,mt8186
> + - description: Google Tentacool (ASUS Chromebook CM14 CM1402C)
> + items:
> + - const: google,tentacruel-sku327681
> + - const: google,tentacruel
> + - const: mediatek,mt8186
> + - description: Google Tentacool (ASUS Chromebook CM14 CM1402C)
> + items:
> + - const: google,tentacruel-sku327683
> + - const: google,tentacruel
> + - const: mediatek,mt8186
> - items:
> - enum:
> - mediatek,mt8186-evb
> --
> 2.42.0.655.g421f12c284-goog
>

Attachments:

(No filename) (2.82 kB)
signature.asc (235.00 B)
Download all attachments

2023-11-27 02:23:42

by Chen-Yu Tsai

[permalink] [raw]

Subject: Re: [PATCH 2/9] dt-bindings: arm: mediatek: Add MT8186 Tentacruel / Tentacool Chromebooks

On Fri, Nov 24, 2023 at 10:09 PM Conor Dooley <[email protected]> wrote:
>
> On Fri, Oct 13, 2023 at 07:02:28AM +0800, Chen-Yu Tsai wrote:
> > Add entries for MT8186 based Tentacruel / Tentacool Chromebooks. The two
> > are based on the same board design: the former is a convertible device
> > with a touchscreen, stylus, and some extra buttons; the latter is a
> > clamshell device and lacks these additional features.
> >
> > 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 <[email protected]>
>
> I had a chat with Doug at Plumbers about the limitations of your
> firmware. As a result, I am fine with acking this if you switch the
> order of the sku compatibles to be in descending order.

Thank you for taking the effort to understand our weird firmware
implementation. And a thank you to Doug for discussing this. I'll
send out the next version shortly.

> The firmware can handle that, right?

Yes. The firmware basically takes a list of machine compatibles, and
runs them against all the DTs it has with of_machine_is_compatible().

Thanks
ChenYu

>
> Cheers,
> Conor.
>
> > ---
> > .../devicetree/bindings/arm/mediatek.yaml | 26 +++++++++++++++++++
> > 1 file changed, 26 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/arm/mediatek.yaml b/Documentation/devicetree/bindings/arm/mediatek.yaml
> > index 60337b439744..aa7e6734b336 100644
> > --- a/Documentation/devicetree/bindings/arm/mediatek.yaml
> > +++ b/Documentation/devicetree/bindings/arm/mediatek.yaml
> > @@ -206,6 +206,32 @@ properties:
> > - enum:
> > - mediatek,mt8183-pumpkin
> > - const: mediatek,mt8183
> > + - description: Google Tentacruel (ASUS Chromebook CM14 Flip CM1402F)
> > + items:
> > + - const: google,tentacruel-sku262144
> > + - const: google,tentacruel-sku262145
> > + - const: google,tentacruel-sku262146
> > + - const: google,tentacruel-sku262147
> > + - const: google,tentacruel
> > + - const: mediatek,mt8186
> > + - description: Google Tentacruel (ASUS Chromebook CM14 Flip CM1402F)
> > + items:
> > + - const: google,tentacruel-sku262148
> > + - const: google,tentacruel-sku262149
> > + - const: google,tentacruel-sku262150
> > + - const: google,tentacruel-sku262151
> > + - const: google,tentacruel
> > + - const: mediatek,mt8186
> > + - description: Google Tentacool (ASUS Chromebook CM14 CM1402C)
> > + items:
> > + - const: google,tentacruel-sku327681
> > + - const: google,tentacruel
> > + - const: mediatek,mt8186
> > + - description: Google Tentacool (ASUS Chromebook CM14 CM1402C)
> > + items:
> > + - const: google,tentacruel-sku327683
> > + - const: google,tentacruel
> > + - const: mediatek,mt8186
> > - items:
> > - enum:
> > - mediatek,mt8186-evb
> > --
> > 2.42.0.655.g421f12c284-goog
> >

2023-11-27 03:56:47

by Chen-Yu Tsai

[permalink] [raw]

Subject: Re: [PATCH 7/9] arm64: dts: mediatek: Introduce MT8186 Steelix

On Mon, Oct 16, 2023 at 4:36 PM Eugen Hristev
<[email protected]> wrote:
>
> On 10/13/23 02:02, Chen-Yu Tsai wrote:
> > The MT8186 Steelix, also known as the Lenovo 300e Yoga Chromebook Gen 4,
> > is a convertible device based on a common design of the same name. The
> > device comes in different variants. Of them, whether a world facing
> > camera is integrated is the only differentiating factor between the
> > two device trees added. The different SKU IDs describe this alone.
> >
> > The other device difference is the touchpad component used. This is
> > simply handled by having both possible components described in the
> > device tree, and letting the implementation figure out which one is
> > actually available. The system bootloader / firmware does not
> > differentiate this in that they share the same SKU IDs.
> >
> > Signed-off-by: Chen-Yu Tsai <[email protected]>
> > ---
> > arch/arm64/boot/dts/mediatek/Makefile | 2 +
> > .../mt8186-corsola-steelix-sku131072.dts | 18 ++
> > .../mt8186-corsola-steelix-sku131073.dts | 18 ++
> > .../dts/mediatek/mt8186-corsola-steelix.dtsi | 197 ++++++++++++++++++
> > 4 files changed, 235 insertions(+)
> > create mode 100644 arch/arm64/boot/dts/mediatek/mt8186-corsola-steelix-sku131072.dts
> > create mode 100644 arch/arm64/boot/dts/mediatek/mt8186-corsola-steelix-sku131073.dts
> > create mode 100644 arch/arm64/boot/dts/mediatek/mt8186-corsola-steelix.dtsi
> >

[...]

> > diff --git a/arch/arm64/boot/dts/mediatek/mt8186-corsola-steelix.dtsi b/arch/arm64/boot/dts/mediatek/mt8186-corsola-steelix.dtsi
> > new file mode 100644
> > index 000000000000..8488f2f9dc4d
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/mediatek/mt8186-corsola-steelix.dtsi
> > @@ -0,0 +1,197 @@

[...]

> > +&i2c2 {
> > + i2c-scl-internal-delay-ns = <22000>;
> > +
> > + /* second source component */
> > + touchpad@2c {
> > + compatible = "hid-over-i2c";
> > + reg = <0x2c>;
> > + hid-descr-addr = <0x20>;
> > + interrupts-extended = <&pio 11 IRQ_TYPE_LEVEL_LOW>;
> > + pinctrl-names = "default";
> > + pinctrl-0 = <&trackpad_pin>;
> > + vdd-supply = <&pp3300_s3>;
> > + wakeup-source;
> > + };
>
> Hi Chen-Yu,
>
> This is not okay, the `trackpad_pin` is shared with touchpad@15 , and if
> drivers are probed in parallel , this can lead to a conflict on this pin
> (GPIO11)

You're right. I've moved it under i2c2 for now, as we do for other platforms
with second source components.

This should hopefully be cleaned up once we have a hardware prober.

ChenYu

2023-11-27 04:00:43

by Chen-Yu Tsai

[permalink] [raw]

Subject: Re: [PATCH 6/9] arm64: dts: mediatek: Add MT8186 Krabby platform based Tentacruel / Tentacool

On Mon, Oct 23, 2023 at 5:05 PM Eugen Hristev
<[email protected]> wrote:
>
> On 10/13/23 02:02, Chen-Yu Tsai wrote:
> > 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 <[email protected]>
> > ---
> > arch/arm64/boot/dts/mediatek/Makefile | 4 +
> > .../dts/mediatek/mt8186-corsola-krabby.dtsi | 130 ++
> > .../mt8186-corsola-tentacool-sku327681.dts | 49 +
> > .../mt8186-corsola-tentacool-sku327683.dts | 26 +
> > .../mt8186-corsola-tentacruel-sku262144.dts | 44 +
> > .../mt8186-corsola-tentacruel-sku262148.dts | 28 +
> > .../boot/dts/mediatek/mt8186-corsola.dtsi | 1704 +++++++++++++++++
> > 7 files changed, 1985 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/mt8186-corsola.dtsi b/arch/arm64/boot/dts/mediatek/mt8186-corsola.dtsi
> > new file mode 100644
> > index 000000000000..42ebab1bafeb
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/mediatek/mt8186-corsola.dtsi
> > @@ -0,0 +1,1704 @@

[...]

>
> Hi Chen-Yu,
>
> These two aud_gpio_tdm empty entries cause this message to be printed:
>
> [ 12.949519] mt8186_mt6366_rt1019_rt5682s sound: there is not valid
> maps for state aud_gpio_tdm_off
> [ 12.959025] mt8186_mt6366_rt1019_rt5682s sound: there is not valid
> maps for state aud_gpio_tdm_on

I guess we need to teach the sound driver to ignore empty pinmux states?

ChenYu

2023-11-27 13:06:21

by AngeloGioacchino Del Regno

[permalink] [raw]

Subject: Re: [PATCH 6/9] arm64: dts: mediatek: Add MT8186 Krabby platform based Tentacruel / Tentacool

Il 27/11/23 05:00, Chen-Yu Tsai ha scritto:
> On Mon, Oct 23, 2023 at 5:05 PM Eugen Hristev
> <[email protected]> wrote:
>>
>> On 10/13/23 02:02, Chen-Yu Tsai wrote:
>>> 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 <[email protected]>
>>> ---
>>> arch/arm64/boot/dts/mediatek/Makefile | 4 +
>>> .../dts/mediatek/mt8186-corsola-krabby.dtsi | 130 ++
>>> .../mt8186-corsola-tentacool-sku327681.dts | 49 +
>>> .../mt8186-corsola-tentacool-sku327683.dts | 26 +
>>> .../mt8186-corsola-tentacruel-sku262144.dts | 44 +
>>> .../mt8186-corsola-tentacruel-sku262148.dts | 28 +
>>> .../boot/dts/mediatek/mt8186-corsola.dtsi | 1704 +++++++++++++++++
>>> 7 files changed, 1985 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/mt8186-corsola.dtsi b/arch/arm64/boot/dts/mediatek/mt8186-corsola.dtsi
>>> new file mode 100644
>>> index 000000000000..42ebab1bafeb
>>> --- /dev/null
>>> +++ b/arch/arm64/boot/dts/mediatek/mt8186-corsola.dtsi
>>> @@ -0,0 +1,1704 @@
>
> [...]
>
>>
>> Hi Chen-Yu,
>>
>> These two aud_gpio_tdm empty entries cause this message to be printed:
>>
>> [ 12.949519] mt8186_mt6366_rt1019_rt5682s sound: there is not valid
>> maps for state aud_gpio_tdm_off
>> [ 12.959025] mt8186_mt6366_rt1019_rt5682s sound: there is not valid
>> maps for state aud_gpio_tdm_on
>
> I guess we need to teach the sound driver to ignore empty pinmux states?

...and we need to teach to the bindings that the aud_gpio_tdm_{off,on} are
optional states and can be omitted.

Cheers,
Angelo

>
> ChenYu

2023-12-01 15:32:43

by Eugen Hristev

[permalink] [raw]

Subject: Re: [PATCH 6/9] arm64: dts: mediatek: Add MT8186 Krabby platform based Tentacruel / Tentacool

On 11/27/23 15:05, AngeloGioacchino Del Regno wrote:
> Il 27/11/23 05:00, Chen-Yu Tsai ha scritto:
>> On Mon, Oct 23, 2023 at 5:05 PM Eugen Hristev
>> <[email protected]> wrote:
>>>
>>> On 10/13/23 02:02, Chen-Yu Tsai wrote:
>>>> 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 <[email protected]>
>>>> ---
>>>> arch/arm64/boot/dts/mediatek/Makefile | 4 +
>>>> .../dts/mediatek/mt8186-corsola-krabby.dtsi | 130 ++
>>>> .../mt8186-corsola-tentacool-sku327681.dts | 49 +
>>>> .../mt8186-corsola-tentacool-sku327683.dts | 26 +
>>>> .../mt8186-corsola-tentacruel-sku262144.dts | 44 +
>>>> .../mt8186-corsola-tentacruel-sku262148.dts | 28 +
>>>> .../boot/dts/mediatek/mt8186-corsola.dtsi | 1704 +++++++++++++++++
>>>> 7 files changed, 1985 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/mt8186-corsola.dtsi b/arch/arm64/boot/dts/mediatek/mt8186-corsola.dtsi
>>>> new file mode 100644
>>>> index 000000000000..42ebab1bafeb
>>>> --- /dev/null
>>>> +++ b/arch/arm64/boot/dts/mediatek/mt8186-corsola.dtsi
>>>> @@ -0,0 +1,1704 @@
>>
>> [...]
>>
>>>
>>> Hi Chen-Yu,
>>>
>>> These two aud_gpio_tdm empty entries cause this message to be printed:
>>>
>>> [ 12.949519] mt8186_mt6366_rt1019_rt5682s sound: there is not valid
>>> maps for state aud_gpio_tdm_off
>>> [ 12.959025] mt8186_mt6366_rt1019_rt5682s sound: there is not valid
>>> maps for state aud_gpio_tdm_on
>>
>> I guess we need to teach the sound driver to ignore empty pinmux states?
>
> ...and we need to teach to the bindings that the aud_gpio_tdm_{off,on} are
> optional states and can be omitted.

Hello Angelo,

The binding does not appear to specify any required states. Am I getting this
wrong? Hence, we can just remove these states from the DT node.

Hello Chen-Yu,

The error message comes from the pinctrl, from dt_to_map_one_config() .
I believe we can safely just remove these two if corsola device does not use TDM.
From my perspective it is pointless to have two empty pinctrl states just for the
sake of it if there are no pins involved.
Please correct me if you have more knowledge about it.

Thanks,
Eugen

>
> Cheers,
> Angelo
>
>>
>> ChenYu
>
>
>