Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Message-ID: <0b042c71-4d07-76a6-53bb-94bbd4bad6c0@collabora.com>
Date:   Mon, 28 Mar 2022 16:50:02 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.7.0
Subject: Re: [PATCH v2 2/4] arm64: dts: mediatek: Add device-tree for MT8195
 Demo board
Content-Language: en-US
To:     Fabien Parent <fparent@baylibre.com>
Cc:     Matthias Brugger <matthias.bgg@gmail.com>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzk+dt@kernel.org>,
        linux-arm-kernel@lists.infradead.org,
        linux-mediatek@lists.infradead.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org
References: <20220327200312.3090515-1-fparent@baylibre.com>
 <20220327200312.3090515-3-fparent@baylibre.com>
 <94d231cf-ce4c-22f5-b9af-41ae68f1e659@collabora.com>
 <20220328144107.ed4xwzuiezzixqrx@radium>
From:   AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@collabora.com>
In-Reply-To: <20220328144107.ed4xwzuiezzixqrx@radium>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

Il 28/03/22 16:41, Fabien Parent ha scritto:
> On Mon, Mar 28, 2022 at 03:47:09PM +0200, AngeloGioacchino Del Regno wrote:
>> Il 27/03/22 22:03, Fabien Parent ha scritto:
>>> Add basic device-tree for the MT8195 Demo board. The
>>> Demo board is made by MediaTek and has a MT8195 SoC,
>>> associated with the MT6359 and MT6360 PMICs, and
>>> the MT7921 connectivity chip.
>>>
>>> The IOs available on that board are:
>>> * 1 USB Type-C connector with DP aux mode support
>>> * 1 USB Type-A connector
>>> * 1 full size HDMI RX and 1 full size HDMI TX connector
>>> * 1 uSD slot
>>> * 40 pins header
>>> * SPI interface header
>>> * 1 M.2 slot
>>> * 1 audio jack
>>> * 1 micro-USB port for serial debug
>>> * 2 connectors for DSI displays
>>> * 3 connectors for CSI cameras
>>> * 1 connector for a eDP panel
>>> * 1 MMC storage
>>>
>>> This commit adds basic support in order to be able to boot.
>>>
>>> Signed-off-by: Fabien Parent <fparent@baylibre.com>
>>> ---
>>> v2:
>>>    * remove empty i2c nodes
>>>    * remove empty spi node
>>>    * remove unused pcie pinctrls
>>>    * fixup node nodes to not contains underscore
>>>    * rename mt6360 pmic node
>>>    * move mmc1 node right after mmc0 node
>>>    * use generic node name for gpio-keys
>>>    * uniformize pinctrl node names
>>>
>>>    arch/arm64/boot/dts/mediatek/Makefile        |   1 +
>>>    arch/arm64/boot/dts/mediatek/mt8195-demo.dts | 447 +++++++++++++++++++
>>>    2 files changed, 448 insertions(+)
>>>    create mode 100644 arch/arm64/boot/dts/mediatek/mt8195-demo.dts
>>>
>>
>> ..snip..
>>
>>> diff --git a/arch/arm64/boot/dts/mediatek/mt8195-demo.dts b/arch/arm64/boot/dts/mediatek/mt8195-demo.dts
>>> new file mode 100644
>>> index 000000000000..d94b4e01159a
>>> --- /dev/null
>>> +++ b/arch/arm64/boot/dts/mediatek/mt8195-demo.dts
>>> @@ -0,0 +1,447 @@
>>
>> ..snip..
>>
>>> +
>>> +	gpio-keys {
>>> +		compatible = "gpio-keys";
>>> +		input-name = "gpio-keys";
>>> +		pinctrl-names = "default";
>>> +		pinctrl-0 = <&gpio_keys_pins>;
>>> +
>>> +		key-0 {
>>
>> key-volup is more descriptive, can you please change that?
> 
> Which review should I follow, yours or the one from Krzysztof? Because both reviews are contradictory
> 

There are a lot of "vol-down", "vol-up" (etc) instances, lots of which are on
Qualcomm device-trees... so I guess this is just about personal preference...

Honestly, before sending my review I forgot to check Krzysztof's one (sorry!!),
but I think that this kind of node names is highly subjective... at least, I am
not aware of any rule about having to use "generic" names.

Check Documentation/devicetree/bindings/input/gpio-keys.yaml - in the example,
you can find "specific" node names, like:

         up {

             label = "GPIO Key UP";

In any case, as I said, it's my personal preference to have it named as something
like "key-volup" or "vol-up" or .. a name that is describing the key, but, as a
personal preference, it is nothing mandatory, not even from my side.

If anyone else has reasons to disagree, shrug, it's fine :))

>>
>>> +			gpios = <&pio 106 GPIO_ACTIVE_LOW>;
>>> +			label = "volume_up";
>>> +			linux,code = <KEY_VOLUMEUP>;
>>> +			wakeup-source;
>>> +			debounce-interval = <15>;
>>> +		};
>>> +	};
>>> +};