Re: [PATCH stm32-next v2 2/2] ARM: dts: stm32: lxa-fairytux2: add Linux Automation GmbH FairyTux 2

[Krzysztof Kozlowski <krzk@kernel.org>]

On Mon, Jan 20, 2025 at 05:33:48PM +0100, Marc Kleine-Budde wrote:
> +&i2c1 {
> +	pinctrl-names = "default", "sleep";
> +	pinctrl-0 = <&i2c1_pins_b>;
> +	pinctrl-1 = <&i2c1_sleep_pins_b>;
> +	status = "okay";
> +
> +	io_board_gpio: gpio6408@20 {

Not much improved here. I gave even link with examples and there is no
example there called "gpio48548796573467346574" or whatever number there
would be.

So again: node names should be generic. gpio@

> +		compatible = "ti,tca6408";
> +		reg = <0x20>;
> +		gpio-controller;
> +		#gpio-cells = <2>;
> +		vcc-supply = <&v3v3_hdmi>;
> +		gpio-line-names = "LED1_GA_YK", "LED2_GA_YK", "LED1_GK_YA", "LED2_GK_YA",
> +				  "RS485_EN", "RS485_120R", "", "CAN_120R";
> +	};
> +};
> +
> +&led_controller_io {
> +	/*
> +	 * led-2 and led-3 are internally connected antiparallel to one
> +	 * another inside the ethernet jack like this:
> +	 * GPIO1 ---+---|led-2|>--+--- GPIO3
> +	 *          +--<|led-3|---+
> +	 * E.g. only one of the LEDs can be illuminated at a time while
> +	 * the other output must be driven low.
> +	 * This should likely be implemented using a multi color LED
> +	 * driver for antiparallel LEDs.
> +	 */
> +	led-2 {
> +		label = "green:act";

I don't see improvements here either. Where is color property? Where is
function?

Again, drop label.

> +		gpios = <&io_board_gpio 1 GPIO_ACTIVE_HIGH>;
> +	};
> +
> +	led-3 {
> +		label = "orange:act";
> +		gpios = <&io_board_gpio 3 GPIO_ACTIVE_HIGH>;
> +	};
> +};
> +
> +&usart3 {
> +	/*
> +	 * On Gen 1 FairyTux 2 only RTS can be used and not CTS as well,
> +	 * Because pins PD11 (CTS) and PI11 (USER_BTN1) share the same
> +	 * interrupt and only one of them can be used at a time.
> +	 */
> +	rts-gpios = <&gpiod 12 GPIO_ACTIVE_LOW>;
> +};
> +
> +&usbotg_hs {
> +	dr_mode = "peripheral";
> +};
> diff --git a/arch/arm/boot/dts/st/stm32mp153c-lxa-fairytux2-gen2.dts b/arch/arm/boot/dts/st/stm32mp153c-lxa-fairytux2-gen2.dts
> new file mode 100644
> index 0000000000000000000000000000000000000000..f263e30e6bcee751b5d550e00a202d2100c5f9fc
> --- /dev/null
> +++ b/arch/arm/boot/dts/st/stm32mp153c-lxa-fairytux2-gen2.dts
> @@ -0,0 +1,146 @@
> +// SPDX-License-Identifier: (GPL-2.0-or-later OR BSD-3-Clause)
> +/*
> + * Copyright (C) 2024 Leonard Göhrs, Pengutronix
> + */
> +
> +/dts-v1/;
> +
> +#include "stm32mp153c-lxa-fairytux2.dtsi"
> +
> +/ {
> +	model = "Linux Automation GmbH FairyTux 2 Gen 2";
> +	compatible = "lxa,stm32mp153c-fairytux2-gen2", "oct,stm32mp153x-osd32", "st,stm32mp153";
> +
> +	gpio-keys {
> +		compatible = "gpio-keys";
> +
> +		button-left {
> +			label = "USER_BTN1";
> +			linux,code = <KEY_ESC>;
> +			gpios = <&gpioi 10 (GPIO_ACTIVE_LOW | GPIO_PULL_UP)>;
> +		};
> +
> +		button-right {
> +			label = "USER_BTN2";
> +			linux,code = <KEY_HOME>;
> +			gpios = <&gpioe 9 (GPIO_ACTIVE_LOW | GPIO_PULL_UP)>;
> +		};
> +	};
> +};
> +
> +&gpiof {
> +	gpio-line-names = "", "", "", "", "",				/*  0 */
> +			  "", "", "", "", "",				/*  5 */
> +			  "", "", "", "", "",				/* 10 */
> +			  "";						/* 15 */
> +};
> +
> +&gpioh {
> +	gpio-line-names = "", "", "", "", "LCD_RESET",			/*  0 */
> +			  "", "", "", "", "",				/*  5 */
> +			  "", "", "GPIO1", "GPIO_INT", "",		/* 10 */
> +			  "";						/* 15 */
> +};
> +
> +&gpioi {
> +	gpio-line-names = "GPIO2", "", "", "", "",			/*  0 */
> +			  "", "", "", "ETH_", "",			/*  5 */
> +			  "", "USER_BTN1";				/* 10 */
> +};
> +
> +&i2c1 {
> +	pinctrl-names = "default", "sleep";
> +	pinctrl-0 = <&i2c1_pins_b>;
> +	pinctrl-1 = <&i2c1_sleep_pins_b>;
> +	status = "okay";
> +
> +	io_board_gpio: gpio6408@20 {

Same problem

> +		compatible = "ti,tca6408";
> +		reg = <0x20>;
> +		gpio-controller;
> +		#gpio-cells = <2>;
> +		interrupt-parent = <&gpioh>;
> +		interrupts = <13 IRQ_TYPE_LEVEL_LOW>;
> +		interrupt-controller;
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&board_tca6408_pins>;
> +		#interrupt-cells = <2>;
> +		vcc-supply = <&v3v3_hdmi>;
> +		gpio-line-names = "LED1_GA_YK", "LED2_GA_YK", "LED1_GK_YA", "USB_CC_ALERT",
> +				  "RS485_EN", "RS485_120R", "USB_CC_RESET", "CAN_120R";
> +	};
> +
> +	usb_c: tcpc@28 {

If this is typec, then typec@

> +		compatible = "st,stusb1600";
> +		reg = <0x28>;
> +		interrupt-parent = <&io_board_gpio>;
> +		interrupts = <3 IRQ_TYPE_EDGE_FALLING>;
> +		vdd-supply = <&reg_5v>;
> +		vsys-supply = <&v3v3_hdmi>;
> +
> +		connector {
> +			compatible = "usb-c-connector";
> +			label = "USB-C";
> +			power-role = "dual";
> +			typec-power-opmode = "default";
> +
> +			port {
> +				con_usbotg_hs_ep: endpoint {
> +					remote-endpoint = <&usbotg_hs_ep>;
> +				};
> +			};
> +		};
> +	};
> +
> +	io_board_eeprom: eeprom@56 {
> +		compatible = "atmel,24c04";
> +		reg = <0x56>;
> +		vcc-supply = <&v3v3_hdmi>;
> +	};
> +
> +	temperature-sensor@48 {

BTW, keep order of nodes. Preferred is by unit address.

Best regards,
Krzysztof