From: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
To: Wojciech Macek <wmacek@chromium.org>
Cc: Rob Herring <robh@kernel.org>,
Krzysztof Kozlowski <krzk+dt@kernel.org>,
Conor Dooley <conor+dt@kernel.org>,
Matthias Brugger <matthias.bgg@gmail.com>,
Chen-Yu Tsai <wenst@chromium.org>,
Rafal Milecki <rafal@milecki.pl>,
Hsin-Yi Wang <hsinyi@chromium.org>,
Sean Wang <sean.wang@mediatek.com>,
devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
linux-mediatek@lists.infradead.org
Subject: Re: [PATCH v2 2/2] arm64: dts: mediatek: mt8186: Add Starmie device
Date: Mon, 25 Nov 2024 13:04:19 +0100 [thread overview]
Message-ID: <a9353ff0-ff29-474d-bfbe-4275d7eb36e8@collabora.com> (raw)
In-Reply-To: <CAJrw_jnER6ozh+TiD=nw-DJWf78DKFy-PEVezv-H4ArTrXHS9A@mail.gmail.com>
Il 25/11/24 11:15, Wojciech Macek ha scritto:
> Sure, I will work on it. Thanks.
>
>>> +
>>> + compatible = "ilitek,ili9882t";
>
>> I can't find this compatible anywhere in any kernel driver. That won't
> work.
>
> Actually, the compat is defined in ./drivers/hid/i2c-hid/i2c-hid-of-elan.c
>
Oh, I typo'ed the compatible while grepping. Sorry, you're right about that.
Cheers,
Angelo
>
> Regards,
> Wojtek
>
> On Mon, Nov 25, 2024 at 10:05 AM AngeloGioacchino Del Regno <
> angelogioacchino.delregno@collabora.com> wrote:
>
>> Il 25/11/24 09:21, Wojciech Macek ha scritto:
>>> Add support for Starmie Chromebooks.
>>>
>>> Signed-off-by: Wojciech Macek <wmacek@chromium.org>
>>> ---
>>> Changelog v2-v1:
>>> - no change
>>>
>>> arch/arm64/boot/dts/mediatek/Makefile | 2 +
>>> .../mediatek/mt8186-corsola-starmie-sku0.dts | 29 ++
>>> .../mediatek/mt8186-corsola-starmie-sku1.dts | 46 ++
>>> .../dts/mediatek/mt8186-corsola-starmie.dtsi | 480 ++++++++++++++++++
>>> 4 files changed, 557 insertions(+)
>>> create mode 100644
>> arch/arm64/boot/dts/mediatek/mt8186-corsola-starmie-sku0.dts
>>> create mode 100644
>> arch/arm64/boot/dts/mediatek/mt8186-corsola-starmie-sku1.dts
>>> create mode 100644
>> arch/arm64/boot/dts/mediatek/mt8186-corsola-starmie.dtsi
>>>
>>> diff --git a/arch/arm64/boot/dts/mediatek/Makefile
>> b/arch/arm64/boot/dts/mediatek/Makefile
>>> index 8fd7b2bb7a159..2ee6266ddf43d 100644
>>> --- a/arch/arm64/boot/dts/mediatek/Makefile
>>> +++ b/arch/arm64/boot/dts/mediatek/Makefile
>>> @@ -59,6 +59,8 @@ dtb-$(CONFIG_ARCH_MEDIATEK) +=
>> mt8186-corsola-magneton-sku393216.dtb
>>> dtb-$(CONFIG_ARCH_MEDIATEK) += mt8186-corsola-magneton-sku393217.dtb
>>> dtb-$(CONFIG_ARCH_MEDIATEK) += mt8186-corsola-magneton-sku393218.dtb
>>> dtb-$(CONFIG_ARCH_MEDIATEK) += mt8186-corsola-rusty-sku196608.dtb
>>> +dtb-$(CONFIG_ARCH_MEDIATEK) += mt8186-corsola-starmie-sku0.dtb
>>> +dtb-$(CONFIG_ARCH_MEDIATEK) += mt8186-corsola-starmie-sku1.dtb
>>> dtb-$(CONFIG_ARCH_MEDIATEK) += mt8186-corsola-steelix-sku131072.dtb
>>> dtb-$(CONFIG_ARCH_MEDIATEK) += mt8186-corsola-steelix-sku131073.dtb
>>> dtb-$(CONFIG_ARCH_MEDIATEK) += mt8186-corsola-tentacool-sku327681.dtb
>>> diff --git
>> a/arch/arm64/boot/dts/mediatek/mt8186-corsola-starmie-sku0.dts
>> b/arch/arm64/boot/dts/mediatek/mt8186-corsola-starmie-sku0.dts
>>> new file mode 100644
>>> index 0000000000000..ca0b8492bbef5
>>> --- /dev/null
>>> +++ b/arch/arm64/boot/dts/mediatek/mt8186-corsola-starmie-sku0.dts
>>> @@ -0,0 +1,29 @@
>>> +// SPDX-License-Identifier: (GPL-2.0 OR MIT)
>>> +/*
>>> + * Copyright 2023 Google LLC
>>> + */
>>> +
>>> +/dts-v1/;
>>> +#include "mt8186-corsola-starmie.dtsi"
>>> +
>>> +/ {
>>> + model = "Google Starmie sku0 board";
>>> + compatible = "google,starmie-sku0", "google,starmie-sku2",
>>> + "google,starmie-sku3", "google,starmie",
>>> + "mediatek,mt8186";
>>> +};
>>> +
>>> +&panel {
>>> + compatible = "starry,ili9882t";
>>> +};
>>> +
>>> +&i2c_tunnel {
>>> + /delete-node/ sbs-battery@b;
>>> +
>>> + battery: sbs-battery@f {
>>> + compatible = "sbs,sbs-battery";
>>> + reg = <0xf>;
>>> + sbs,i2c-retry-count = <2>;
>>> + sbs,poll-retry-count = <1>;
>>> + };
>>> +};
>>> diff --git
>> a/arch/arm64/boot/dts/mediatek/mt8186-corsola-starmie-sku1.dts
>> b/arch/arm64/boot/dts/mediatek/mt8186-corsola-starmie-sku1.dts
>>> new file mode 100644
>>> index 0000000000000..2ba4c083a58c6
>>> --- /dev/null
>>> +++ b/arch/arm64/boot/dts/mediatek/mt8186-corsola-starmie-sku1.dts
>>> @@ -0,0 +1,46 @@
>>> +// SPDX-License-Identifier: (GPL-2.0 OR MIT)
>>> +/*
>>> + * Copyright 2023 Google LLC
>>> + */
>>> +
>>> +/dts-v1/;
>>> +#include "mt8186-corsola-starmie.dtsi"
>>> +
>>> +/ {
>>> + model = "Google Starmie sku1 board";
>>> + compatible = "google,starmie-sku1", "google,starmie-sku4",
>>> + "google,starmie", "mediatek,mt8186";
>>> +};
>>> +
>>> +&panel {
>>> + compatible = "starry,himax83102-j02";
>>> +};
>>> +
>>> +&i2c1 {
>>> + /delete-node/ touchscreen@41;
>>> + touchscreen_himax: touchscreen@4f {
>>> + status = "okay";
>>
>> Okay is the default.
>>
>>> +
>>> + compatible = "hid-over-i2c";
>>> + reg = <0x4f>;
>>> + interrupt-parent = <&pio>;
>>> + interrupts = <12 IRQ_TYPE_LEVEL_LOW>;
>>> + pinctrl-names = "default";
>>> + pinctrl-0 = <&touchscreen_pins>;
>>> + vdd-supply = <&mt6366_vio18_reg>;
>>> + panel = <&panel>;
>>> + post-power-on-delay-ms = <450>;
>>> + hid-descr-addr = <0x0001>;
>>> + };
>>> +};
>>> +
>>> +&i2c_tunnel {
>>> + /delete-node/ sbs-battery@b;
>>
>> Would status = "disabled" not work for sbs-battery@b?
>>
>>> +
>>> + battery: sbs-battery@f {
>>
>> You're defining sbs-battery@f in every starmie dts, you can move that to
>> the
>> starmie dtsi instead, so that you can avoid all the useless duplication.
>>
>>> + compatible = "sbs,sbs-battery";
>>> + reg = <0xf>;
>>> + sbs,i2c-retry-count = <2>;
>>> + sbs,poll-retry-count = <1>;
>>> + };
>>> +};
>>> diff --git a/arch/arm64/boot/dts/mediatek/mt8186-corsola-starmie.dtsi
>> b/arch/arm64/boot/dts/mediatek/mt8186-corsola-starmie.dtsi
>>> new file mode 100644
>>> index 0000000000000..28ac65d28143e
>>> --- /dev/null
>>> +++ b/arch/arm64/boot/dts/mediatek/mt8186-corsola-starmie.dtsi
>>> @@ -0,0 +1,480 @@
>>> +// SPDX-License-Identifier: (GPL-2.0 OR MIT)
>>> +/*
>>> + * Copyright 2023 Google LLC
>>> + */
>>> +
>>> +/dts-v1/;
>>> +#include "mt8186-corsola.dtsi"
>>> +
>>> +/delete-node/ &dsi_out;
>>
>> Instead of hacking in a delete-node, you can just change mt8186.dtsi at
>> this point,
>> or you can use the current dsi_out phandle. I would prefer that you do the
>> latter,
>> as it's going to be more convenient later when I'll have to migrate this
>> platform
>> to the full OF Graph for the display controller.
>>
>>> +/delete-node/ &keyboard_controller;
>>> +
>>> +/ {
>>> + en_pp6000_mipi_disp_150ma: en-pp6000-mipi-disp-150ma {
>>> + compatible = "regulator-fixed";
>>> + regulator-name = "en_pp6000_mipi_disp_150ma";
>>> + gpio = <&pio 154 GPIO_ACTIVE_HIGH>;
>>> + enable-active-high;
>>> + pinctrl-names = "default";
>>> + pinctrl-0 = <&pp6000_mipi_disp_150ma_fixed_pins>;
>>> + };
>>> +
>>> + tboard_thermistor1: thermal-sensor1 {
>>> + compatible = "generic-adc-thermal";
>>> + #thermal-sensor-cells = <0>;
>>> + io-channels = <&auxadc 0>;
>>> + io-channel-names = "sensor-channel";
>>> + temperature-lookup-table = < (-5000) 1492
>>> + 0 1413
>>> + 5000 1324
>>> + 10000 1227
>>> + 15000 1121
>>> + 20000 1017
>>> + 25000 900
>>> + 30000 797
>>> + 35000 698
>>> + 40000 606
>>> + 45000 522
>>> + 50000 449
>>> + 55000 383
>>> + 60000 327
>>> + 65000 278
>>> + 70000 236
>>> + 75000 201
>>> + 80000 171
>>> + 85000 145
>>> + 90000 163
>>> + 95000 124
>>> + 100000 91
>>> + 105000 78
>>> + 110000 67
>>> + 115000 58
>>> + 120000 50
>>> + 125000 44>;
>>> + };
>>> +
>>> + tboard_thermistor2: thermal-sensor2 {
>>> + compatible = "generic-adc-thermal";
>>> + #thermal-sensor-cells = <0>;
>>> + io-channels = <&auxadc 1>;
>>> + io-channel-names = "sensor-channel";
>>> + temperature-lookup-table = < (-5000) 1492
>>> + 0 1413
>>> + 5000 1324
>>> + 10000 1227
>>> + 15000 1121
>>> + 20000 1017
>>> + 25000 900
>>> + 30000 797
>>> + 35000 698
>>> + 40000 606
>>> + 45000 522
>>> + 50000 449
>>> + 55000 383
>>> + 60000 327
>>> + 65000 278
>>> + 70000 236
>>> + 75000 201
>>> + 80000 171
>>> + 85000 145
>>> + 90000 163
>>> + 95000 124
>>> + 100000 91
>>> + 105000 78
>>> + 110000 67
>>> + 115000 58
>>> + 120000 50
>>> + 125000 44>;
>>> + };
>>> +};
>>> +
>>> +&cros_ec {
>>> + cbas: cbas {
>>> + compatible = "google,cros-cbas";
>>> + };
>>> +
>>> + keyboard-controller {
>>> + compatible = "google,cros-ec-keyb-switches";
>>> + };
>>> +};
>>> +
>>> +&dsi0 {
>>> + status = "okay";
>>> + #address-cells = <1>;
>>> + #size-cells = <0>;
>>> + panel: panel@0 {
>>> + /* compatible will be set in board dts */
>>> + reg = <0>;
>>> + enable-gpios = <&pio 98 0>;
>>> + pinctrl-names = "default";
>>> + pinctrl-0 = <&panel_pins_default>;
>>> + avdd-supply = <&en_pp6000_mipi_disp>;
>>> + avee-supply = <&en_pp6000_mipi_disp_150ma>;
>>> + pp1800-supply = <&mt6366_vio18_reg>;
>>> + backlight = <&backlight_lcd0>;
>>> + rotation = <270>;
>>> + port {
>>> + panel_in: endpoint {
>>> + remote-endpoint = <&dsi_out>;
>>> + };
>>> + };
>>> + };
>>> +
>>> + ports {
>>> + port {
>>> + dsi_out: endpoint {
>>> + remote-endpoint = <&panel_in>;
>>> + };
>>> + };
>>> + };
>>> +};
>>> +
>>> +&i2c0 {
>>> + status = "disabled";
>>> +};
>>> +
>>> +&i2c1 {
>>> + touchscreen: touchscreen@41 {
>>> + status = "okay";
>>
>> Status is okay by default.
>>
>>> +
>>> + compatible = "ilitek,ili9882t";
>>
>> I can't find this compatible anywhere in any kernel driver. That won't
>> work.
>>
>>> + reg = <0x41>;
>>> + interrupt-parent = <&pio>;
>>> + interrupts = <12 IRQ_TYPE_LEVEL_LOW>;
>>
>> interrupts-extended please
>>
>>> + pinctrl-names = "default";
>>> + pinctrl-0 = <&touchscreen_pins>;
>>> + panel = <&panel>;
>>> + reset-gpios = <&pio 60 GPIO_ACTIVE_LOW>;
>>> + vccio-supply = <&mt6366_vio18_reg>;
>>> + };
>>> +};
>>> +
>>> +&i2c2 {
>>> + status = "disabled";
>>> +};
>>> +
>>> +&i2c4 {
>>> + status = "disabled";
>>> +};
>>> +
>>> +&i2c5 {
>>> + clock-frequency = <400000>;
>>> +
>>> +};
>>> +
>>> +&mmc1_pins_default {
>>> + pins-clk {
>>> + drive-strength = <MTK_DRIVE_8mA>;
>>
>> Please stop using MTK_DRIVE_xxmA definitions. This is just <8>.
>>
>>> + };
>>> +
>>> + pins-cmd-dat {
>>> + drive-strength = <MTK_DRIVE_8mA>;
>>> + };
>>> +};
>>> +
>>> +&mmc1_pins_uhs {
>>> + pins-clk {
>>> + drive-strength = <MTK_DRIVE_8mA>;
>>> + };
>>> +
>>> + pins-cmd-dat {
>>> + drive-strength = <MTK_DRIVE_8mA>;
>>> + };
>>> +};
>>> +
>>> +&pen_insert {
>>> + wakeup-event-action = <EV_ACT_ANY>;
>>> +};
>>> +
>>> +&pio {
>>
>> ..snip..
>>
>>> +
>>> + dpi_pin_default: dpi-pin-default {
>>> + pins-cmd-dat {
>>> + pinmux = <PINMUX_GPIO103__FUNC_GPIO103>,
>>> + <PINMUX_GPIO104__FUNC_GPIO104>,
>>> + <PINMUX_GPIO105__FUNC_GPIO105>,
>>> + <PINMUX_GPIO106__FUNC_GPIO106>,
>>> + <PINMUX_GPIO107__FUNC_GPIO107>,
>>> + <PINMUX_GPIO108__FUNC_GPIO108>,
>>> + <PINMUX_GPIO109__FUNC_GPIO109>,
>>> + <PINMUX_GPIO110__FUNC_GPIO110>,
>>> + <PINMUX_GPIO111__FUNC_GPIO111>,
>>> + <PINMUX_GPIO112__FUNC_GPIO112>,
>>> + <PINMUX_GPIO113__FUNC_GPIO113>,
>>> + <PINMUX_GPIO114__FUNC_GPIO114>,
>>> + <PINMUX_GPIO101__FUNC_GPIO101>,
>>> + <PINMUX_GPIO100__FUNC_GPIO100>,
>>> + <PINMUX_GPIO102__FUNC_GPIO102>,
>>> + <PINMUX_GPIO99__FUNC_GPIO99>;
>>> + drive-strength = <MTK_DRIVE_10mA>;
>>
>> Please stop using MTK_DRIVE_xxmA definitions. This is <10>.
>>
>>
>>> + output-low;
>>> + };
>>> + };
>>> +
>>> + dpi_pin_func: dpi-pin-func {
>>> + pins-cmd-dat {
>>> + pinmux = <PINMUX_GPIO103__FUNC_DPI_DATA0>,
>>> + <PINMUX_GPIO104__FUNC_DPI_DATA1>,
>>> + <PINMUX_GPIO105__FUNC_DPI_DATA2>,
>>> + <PINMUX_GPIO106__FUNC_DPI_DATA3>,
>>> + <PINMUX_GPIO107__FUNC_DPI_DATA4>,
>>> + <PINMUX_GPIO108__FUNC_DPI_DATA5>,
>>> + <PINMUX_GPIO109__FUNC_DPI_DATA6>,
>>> + <PINMUX_GPIO110__FUNC_DPI_DATA7>,
>>> + <PINMUX_GPIO111__FUNC_DPI_DATA8>,
>>> + <PINMUX_GPIO112__FUNC_DPI_DATA9>,
>>> + <PINMUX_GPIO113__FUNC_DPI_DATA10>,
>>> + <PINMUX_GPIO114__FUNC_DPI_DATA11>,
>>> + <PINMUX_GPIO101__FUNC_DPI_HSYNC>,
>>> + <PINMUX_GPIO100__FUNC_DPI_VSYNC>,
>>> + <PINMUX_GPIO102__FUNC_DPI_DE>,
>>> + <PINMUX_GPIO99__FUNC_DPI_PCLK>;
>>> + drive-strength = <MTK_DRIVE_10mA>;
>>> + };
>>> + };
>>> +
>>> + edp_panel_fixed_pins: edp-panel-fixed-pins {
>>> + pins1 {
>>
>> I don't see where you're using this pin. Please don't add unused pins.
>>
>>> + pinmux = <PINMUX_GPIO153__FUNC_GPIO153>;
>>> + output-low;
>>> + };
>>> + };
>>> +
>>> + pp6000_mipi_disp_150ma_fixed_pins:
>> pp6000-mipi-disp-150ma-fixed-pins {
>>> + pins1 {
>>
>> pins-en {
>>
>>> + pinmux = <PINMUX_GPIO154__FUNC_GPIO154>;
>>> + output-low;
>>> + };
>>> + };
>>> +
>>> + panel_pins_default: panel-pins-default {
>>> + pins1 {
>>
>> pins-en {
>>
>>> + pinmux = <PINMUX_GPIO98__FUNC_GPIO98>;
>>> + output-low;
>>> + };
>>> + };
>>> + wifi_pins_pwrseq: wifipwrseq {
>>
>> Like this, that's unused.
>>
>> You do have a wifi_enable_pin in mt8186-corsola.dtsi though, so override
>> it.
>>
>>> + pins-wifi-enable {
>>> + pinmux = <PINMUX_GPIO51__FUNC_GPIO51>;
>>> + };
>>> + };
>>> +};
>>> +
>>> +&usb_c1 {
>>> + status = "disabled";
>>> +};
>>> +
>>> +&thermal_zones {
>>> + tboard1 {
>>> + polling-delay = <1000>; /* milliseconds */
>>> + polling-delay-passive = <0>; /* milliseconds */
>>> + thermal-sensors = <&tboard_thermistor1>;
>>> + };
>>> +
>>> + tboard2 {
>>> + polling-delay = <1000>; /* milliseconds */
>>> + polling-delay-passive = <0>; /* milliseconds */
>>> + thermal-sensors = <&tboard_thermistor2>;
>>> + };
>>> +};
>>> +
>>> +&wifi_pwrseq {
>>> + reset-gpios = <&pio 51 1>;
>>> +};
>>> +
>>> +en_pp6000_mipi_disp: &pp3300_disp_x {
>>
>> ....but pp6000 is not pp3300, so move the pp3300 to the relevant board dts
>> and define the pp6000 here, or names won't match.
>>
>>> + regulator-name = "en_pp6000_mipi_disp";
>>> + gpio = <&pio 153 GPIO_ACTIVE_HIGH>;
>>> + regulator-enable-ramp-delay = <3000>;
>>> + /delete-property/ regulator-boot-on;
>>> +};
>>
>> Regards,
>> Angelo
>>
>
prev parent reply other threads:[~2024-11-25 12:06 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-11-25 8:21 [PATCH v2 0/2] Add Starmie Chromebook Wojciech Macek
2024-11-25 8:21 ` [PATCH v2 1/2] dt-bindings: arm: mediatek: Add MT8186 Starmie Chromebooks Wojciech Macek
2024-11-25 9:07 ` AngeloGioacchino Del Regno
2024-11-25 18:32 ` Conor Dooley
2024-11-26 4:36 ` Chen-Yu Tsai
2024-11-26 17:50 ` Conor Dooley
2024-11-25 8:21 ` [PATCH v2 2/2] arm64: dts: mediatek: mt8186: Add Starmie device Wojciech Macek
2024-11-25 9:05 ` AngeloGioacchino Del Regno
[not found] ` <CAJrw_jnER6ozh+TiD=nw-DJWf78DKFy-PEVezv-H4ArTrXHS9A@mail.gmail.com>
2024-11-25 12:04 ` AngeloGioacchino Del Regno
2024-11-25 12:04 ` AngeloGioacchino Del Regno [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=a9353ff0-ff29-474d-bfbe-4275d7eb36e8@collabora.com \
--to=angelogioacchino.delregno@collabora.com \
--cc=conor+dt@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=hsinyi@chromium.org \
--cc=krzk+dt@kernel.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mediatek@lists.infradead.org \
--cc=matthias.bgg@gmail.com \
--cc=rafal@milecki.pl \
--cc=robh@kernel.org \
--cc=sean.wang@mediatek.com \
--cc=wenst@chromium.org \
--cc=wmacek@chromium.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.