Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.37 as permitted sender) client-ip=23.128.96.37;
Date:   Thu, 7 Dec 2023 14:41:09 +0900
From:   Chen-Yu Tsai <wenst@chromium.org>
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-arm-kernel@lists.infradead.org,
        linux-mediatek@lists.infradead.org, linux-kernel@vger.kernel.org,
        Conor Dooley <conor.dooley@microchip.com>
Subject: Re: [PATCH v3 6/9] arm64: dts: mediatek: Add MT8186 Krabby platform
 based Tentacruel / Tentacool
Message-ID: <20231207054109.GA37230@google.com>
References: <20231204084012.2281292-1-wenst@chromium.org>
 <20231204084012.2281292-7-wenst@chromium.org>
 <4a7c0cd5-d705-4369-90dd-92d6a27c4723@collabora.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <4a7c0cd5-d705-4369-90dd-92d6a27c4723@collabora.com>
Precedence: bulk

On Mon, Dec 04, 2023 at 01:55:06PM +0200, Eugen Hristev wrote:
> Hello Chen-Yu,
> 
> On 12/4/23 10:40, 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 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     | 1719 +++++++++++++++++
> >  7 files changed, 2003 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..8726b2916ef1
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/mediatek/mt8186-corsola.dtsi
> > @@ -0,0 +1,1719 @@

[...]

> > +	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>;
> 
> These two above bring errors, as discussed

Looking at the machine driver code, it seems OK to remove them. So I
will.

> > +		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>;
> > +		};
> > +	};

[...]

> > +&afe {
> > +	i2s0-share = "I2S1";
> > +	i2s3-share = "I2S2";
> 
> These i2sX-share are not documented.

Looks like this got handled directly in the machine driver.
Will remove.

> > +	status = "okay";
> > +};

[...]

> > +&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>;
> 
> While this makes things better, it's not correct. The i2c2 bus does not use the
> trackpad pin. I would believe a better way is to have a node that holds both
> trackpads and claims the pin for both at once. And that node not being the i2c bus
> as there could be more devices on the bus and the pin would be taken regardless of
> any trackpad probing , which is not right.

That is still a workaround, and it also doesn't work because there are
no bindings nor code for the extra device node level.

> Another option is to disable parallel probing for the trackpads, such that when one
> fails to probe, the pin is released and can be taken by the other one.

We thought about this but this workaround would impact everyone else not
dealing with second source components.

[...]

> > +	aud_gpio_tdm_off: aud-gpio-tdm-off-pins { };
> > +
> > +	aud_gpio_tdm_on: aud-gpio-tdm-on-pins { };
> 
> Guess have to remove these two empty pinctrls.

Yes. I'll try that. Looking at the code it should work.


Thanks for the review
ChenYu