linux-kernel - Re: [PATCH v2 2/4] arm64: dts: mediatek: Add device-tree for MT8195 Demo board

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <0b042c71-4d07-76a6-53bb-94bbd4bad6c0@collabora.com>
Date:   Mon, 28 Mar 2022 16:50:02 +0200
From:   AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@...labora.com>
To:     Fabien Parent <fparent@...libre.com>
Cc:     Matthias Brugger <matthias.bgg@...il.com>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzk+dt@...nel.org>,
        linux-arm-kernel@...ts.infradead.org,
        linux-mediatek@...ts.infradead.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/4] arm64: dts: mediatek: Add device-tree for MT8195
 Demo board

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@...libre.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>;
>>> +		};
>>> +	};
>>> +};