Received-SPF: pass (google.com: domain of linux-kernel+bounces-14239-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20231213150435.4134390-1-wenst@chromium.org> <20231213150435.4134390-7-wenst@chromium.org>
 <19c98266-0a8e-4e99-8766-cdf31a3a97b6@collabora.com>
In-Reply-To: <19c98266-0a8e-4e99-8766-cdf31a3a97b6@collabora.com>
From: Chen-Yu Tsai <wenst@chromium.org>
Date: Tue, 2 Jan 2024 18:25:46 +0800
Message-ID: <CAGXv+5E7yzwgEnSUaS3YerK_kcO853AJsq=gBbkWruh86mXbgQ@mail.gmail.com>
Subject: Re: [PATCH v4 6/9] arm64: dts: mediatek: Add MT8186 Krabby platform
 based Tentacruel / Tentacool
To: Eugen Hristev <eugen.hristev@collabora.com>
Cc: Matthias Brugger <matthias.bgg@gmail.com>, 
	AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>, Rob Herring <robh+dt@kernel.org>, 
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>, Conor Dooley <conor+dt@kernel.org>, 
	devicetree@vger.kernel.org, linux-mediatek@lists.infradead.org, 
	linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, 
	Conor Dooley <conor.dooley@microchip.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi,

On Mon, Jan 1, 2024 at 10:09=E2=80=AFPM Eugen Hristev
<eugen.hristev@collabora.com> wrote:
>
> Hello Chen-Yu,
>
> There is still some nonconformity with the bindings, please see below:
>
> On 12/13/23 17:04, 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 <wenst@chromium.org>
> > Acked-by: Conor Dooley <conor.dooley@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 sourc=
e
> >   component resource sharing.
> > - Fix copyright year
> > - Fixed touchscreen supply name
> > ---
>
> [snip]
>
> > +
> > +&i2c3 {
> > +     pinctrl-names =3D "default";
> > +     pinctrl-0 =3D <&i2c3_pins>;
> > +     clock-frequency =3D <100000>;
> > +     status =3D "okay";
> > +
> > +     it6505dptx: dp-bridge@5c {
> > +             compatible =3D "ite,it6505";
>
> dp-bridge@5c: '#address-cells', '#size-cells', '#sound-dai-cells' do not =
match any
> of the regexes: 'pinctrl-[0-9]+'
>         from schema $id: http://devicetree.org/schemas/display/bridge/ite=
,it6505.yaml#

Will add a patch to update the bindings.

> > +             reg =3D <0x5c>;
> > +             interrupts-extended =3D <&pio 8 IRQ_TYPE_LEVEL_LOW>;
> > +             pinctrl-names =3D "default";
> > +             pinctrl-0 =3D <&it6505_pins>;
> > +             #address-cells =3D <1>;
> > +             #size-cells =3D <0>;
>
> /soc/i2c@1100f000/dp-bridge@5c: unnecessary #address-cells/#size-cells wi=
thout
> "ranges" or child "reg" property

Dropped.

> > +             #sound-dai-cells =3D <0>;
> > +             ovdd-supply =3D <&mt6366_vsim2_reg>;
> > +             pwr18-supply =3D <&pp1800_dpbrdg_dx>;
> > +             reset-gpios =3D <&pio 177 GPIO_ACTIVE_HIGH>;
> > +
> > +             ports {
> > +                     #address-cells =3D <1>;
> > +                     #size-cells =3D <0>;
> > +
> > +                     port@0 {
> > +                             reg =3D <0>;
> > +
> > +                             it6505_in: endpoint {
> > +                                     link-frequencies =3D /bits/ 64 <1=
50000000>;
> > +                                     remote-endpoint =3D <&dpi_out>;
> > +                             };
> > +                     };
> > +
> > +                     port@1 {
> > +                             reg =3D <1>;
> > +                     };
> > +             };
> > +     };
> > +};
> > +
>
> [snip]
>
> > +&spi1 {
> > +     pinctrl-names =3D "default";
> > +     pinctrl-0 =3D <&spi1_pins>;
> > +     mediatek,pad-select =3D <0>;
> > +     status =3D "okay";
> > +
> > +     cros_ec: ec@0 {
> > +             compatible =3D "google,cros-ec-spi";
> > +             reg =3D <0>;
> > +             interrupts-extended =3D <&pio 13 IRQ_TYPE_LEVEL_LOW>;
> > +             pinctrl-names =3D "default";
> > +             pinctrl-0 =3D <&ec_ap_int>;
> > +             spi-max-frequency =3D <1000000>;
> > +
> > +             i2c_tunnel: i2c-tunnel {
> > +                     compatible =3D "google,cros-ec-i2c-tunnel";
> > +                     google,remote-bus =3D <1>;
> > +                     #address-cells =3D <1>;
> > +                     #size-cells =3D <0>;
> > +             };
> > +
> > +             typec {
> > +                     compatible =3D "google,cros-ec-typec";
> > +                     #address-cells =3D <1>;
> > +                     #size-cells =3D <0>;
> > +
> > +                     usb_c0: connector@0 {
> > +                             compatible =3D "usb-c-connector";
> > +                             reg =3D <0>;
> > +                             label =3D "left";
> > +                             power-role =3D "dual";
> > +                             data-role =3D "host";
> > +                             try-power-role =3D "source";
> > +
> > +                             ports {
> > +                                     #address-cells =3D <1>;
> > +                                     #size-cells =3D <0>;
> > +
> typec:connector@0:ports: 'port@0' is a required property
> > +                                     port@1 {
> > +                                             reg =3D <1>;
> > +
> > +                                             typec_port0: endpoint { }=
;
> > +                                     };
> > +                             };
> > +                     };
> > +
> > +                     usb_c1: connector@1 {
> > +                             compatible =3D "usb-c-connector";
> > +                             reg =3D <1>;
> > +                             label =3D "right";
> > +                             power-role =3D "dual";
> > +                             data-role =3D "host";
> > +                             try-power-role =3D "source";
> > +
> > +                             ports {
> connector@1: Unevaluated properties are not allowed ('ports' was unexpect=
ed)
>         from schema $id: http://devicetree.org/schemas/connector/usb-conn=
ector.yaml#

Not sure why this is happening. Maybe because the sub-schema validation
failed?

In any case, I will drop the whole ports section. This can be re-added once
all the type-C mux stuff has been sorted out.

> > +                                     #address-cells =3D <1>;
> > +                                     #size-cells =3D <0>;
> > +
> > +                                     port@1 {
> connector@0: ports: 'port@0' is a required property
> > +                                             reg =3D <1>;
> > +
> > +                                             typec_port1: endpoint { }=
;
> > +                                     };
> > +                             };
> > +                     };
> > +             };
> > +     };
> > +};
> > +
>
> [snip]
>
> > +
> > +&usb_host1 {
> > +     #address-cells =3D <2>;
> > +     #size-cells =3D <2>;
>
>  usb@11281000: usb@11280000:#address-cells:0:0: 1 was expected
>         from schema $id: http://devicetree.org/schemas/usb/mediatek,mtu3.=
yaml#
> usb@11281000: usb@11280000:#size-cells:0:0: 0 was expected

Dropped.

> > +     vbus-supply =3D <&usb_p1_vbus>;
> > +     status =3D "okay";
> > +};
> > +
> > +&watchdog {
> > +     mediatek,reset-by-toprgu;
> > +};
> > +
> > +#include <arm/cros-ec-keyboard.dtsi>
> > +#include <arm/cros-ec-sbs.dtsi>
>
>
> Eugen

Thanks for the review. Since the merge window is just around the corner,
I will send a new version later this month.

ChenYu