Re: [PATCH 3/4] arm64: dts: apple: Add touchbar digitizer nodes

From: Krzysztof Kozlowski
Date: Wed Nov 27 2024 - 03:55:23 EST


On Tue, Nov 26, 2024 at 09:48:01PM +0100, Sasha Finkelstein wrote:
> Adds device tree entries for the touchbar digitizer
>
> Co-developed-by: Janne Grunau <j@xxxxxxxxxx>
> Signed-off-by: Janne Grunau <j@xxxxxxxxxx>
> Signed-off-by: Sasha Finkelstein <fnkl.kernel@xxxxxxxxx>
> ---
> arch/arm64/boot/dts/apple/t8103-j293.dts | 24 ++++++++++++++++++++++++
> arch/arm64/boot/dts/apple/t8103.dtsi | 19 +++++++++++++++++++
> arch/arm64/boot/dts/apple/t8112-j493.dts | 20 ++++++++++++++++++++
> arch/arm64/boot/dts/apple/t8112.dtsi | 14 ++++++++++++++
> 4 files changed, 77 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/apple/t8103-j293.dts b/arch/arm64/boot/dts/apple/t8103-j293.dts
> index 56b0c67bfcda321b60c621de092643017693ff91..a1c4e5731f2147121a9845bc9f34d224025fb145 100644
> --- a/arch/arm64/boot/dts/apple/t8103-j293.dts
> +++ b/arch/arm64/boot/dts/apple/t8103-j293.dts
> @@ -28,6 +28,10 @@ led-0 {
> default-state = "keep";
> };
> };
> +
> + aliases {

Do not add nodes to the end, but in appropriate place. Either ordered by
name, as DTS coding style asks, or in logical place matching existing
convention (convention: aliases are always the first node).

> + touchbar0 = &touchbar0;

Not used, drop.

> + };
> };
>
> &bluetooth0 {
> @@ -38,6 +42,26 @@ &wifi0 {
> brcm,board-type = "apple,honshu";
> };
>
> +&spi0 {

Also unusual placement - between 'w' and 'i'... unless you keep here
the second style of sorting (matching DTSI)?

> + status = "okay";
> +
> + touchbar0: touchbar@0 {
> + compatible = "apple,j293-touchbar",
> + "apple,z2-touchbar", "apple,z2-multitouch";
> + reg = <0>;
> + spi-max-frequency = <11500000>;
> + spi-cs-setup-delay-ns = <2000>;
> + spi-cs-hold-delay-ns = <2000>;
> + reset-gpios = <&pinctrl_ap 139 GPIO_ACTIVE_LOW>;
> + cs-gpios = <&pinctrl_ap 109 0>;

Use proper GPIO flag define.

> + interrupts-extended = <&pinctrl_ap 194 IRQ_TYPE_EDGE_FALLING>;
> + firmware-name = "apple/dfrmtfw-j293.bin";
> + touchscreen-size-x = <23045>;
> + touchscreen-size-y = <640>;
> + label = "MacBookPro17,1 Touch Bar";
> + };
> +};
> +
> &i2c2 {
> status = "okay";
> };
> diff --git a/arch/arm64/boot/dts/apple/t8103.dtsi b/arch/arm64/boot/dts/apple/t8103.dtsi
> index 9b0dad6b618444ac6b1c9735c50cccfc3965f947..dc72aae3844bf33579f623f0b01abc7de4033af4 100644
> --- a/arch/arm64/boot/dts/apple/t8103.dtsi
> +++ b/arch/arm64/boot/dts/apple/t8103.dtsi
> @@ -326,6 +326,13 @@ clkref: clock-ref {
> clock-output-names = "clkref";
> };
>
> + clk_200m: clock-200m {
> + compatible = "fixed-clock";
> + #clock-cells = <0>;
> + clock-frequency = <200000000>;
> + clock-output-names = "clk_200m";
> + };
> +
> /*
> * This is a fabulated representation of the input clock
> * to NCO since we don't know the true clock tree.
> @@ -441,6 +448,18 @@ fpwm1: pwm@235044000 {
> status = "disabled";
> };
>
> + spi0: spi@235100000 {
> + compatible = "apple,t8103-spi", "apple,spi";
> + reg = <0x2 0x35100000 0x0 0x4000>;
> + interrupt-parent = <&aic>;
> + interrupts = <AIC_IRQ 614 IRQ_TYPE_LEVEL_HIGH>;
> + clocks = <&clk_200m>;
> + power-domains = <&ps_spi0>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> + status = "disabled"; /* only used in J293 */
> + };
> +
> serial0: serial@235200000 {
> compatible = "apple,s5l-uart";
> reg = <0x2 0x35200000 0x0 0x1000>;
> diff --git a/arch/arm64/boot/dts/apple/t8112-j493.dts b/arch/arm64/boot/dts/apple/t8112-j493.dts
> index 0ad908349f55406783942735a2e9dad54cda00ec..03fb711b3a1fa767ba70807a6d3404e4d52eb783 100644
> --- a/arch/arm64/boot/dts/apple/t8112-j493.dts
> +++ b/arch/arm64/boot/dts/apple/t8112-j493.dts
> @@ -20,6 +20,7 @@ / {
> aliases {
> bluetooth0 = &bluetooth0;
> wifi0 = &wifi0;
> + touchbar0 = &touchbar0;

Do not add to the end of lists/properties/nodes etc, but keep order,
usually alphabetical. This avoids conflicts or allows conflicting
series to be still merged. That's a general rule for most of
development (Makefiles, DTS, Kconfigs, lists in DT bindings).

Best regards,
Krzysztof