Re: [PATCH v5 7/7] dts: ti: k3-am625-beagleplay: Add mikroBUS

[Andrew Davis <afd@ti.com>]

On 6/27/24 11:26 AM, Ayush Singh wrote:
> DONOTMERGE
> 
> Add mikroBUS connector and some mikroBUS boards support for Beagleplay.
> The mikroBUS boards node should probably be moved to a more appropriate
> location but I am not quite sure where it should go since it is not
> dependent on specific arch.
> 
> Signed-off-by: Ayush Singh <ayush@beagleboard.org>
> ---
>   arch/arm64/boot/dts/ti/k3-am625-beagleplay.dts | 94 +++++++++++++++++++++++---
>   1 file changed, 86 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/ti/k3-am625-beagleplay.dts b/arch/arm64/boot/dts/ti/k3-am625-beagleplay.dts
> index 70de288d728e..3f3cd70345c4 100644
> --- a/arch/arm64/boot/dts/ti/k3-am625-beagleplay.dts
> +++ b/arch/arm64/boot/dts/ti/k3-am625-beagleplay.dts
> @@ -38,6 +38,7 @@ aliases {
>   		serial2 = &main_uart0;
>   		usb0 = &usb0;
>   		usb1 = &usb1;
> +		mikrobus0 = &mikrobus0;
>   	};
>   
>   	chosen {
> @@ -227,6 +228,56 @@ simple-audio-card,codec {
>   		};
>   	};
>   
> +	mikrobus0: mikrobus-connector {
> +		compatible = "mikrobus-connector";
> +		pinctrl-names = "default", "pwm_default", "pwm_gpio",
> +				"uart_default", "uart_gpio", "i2c_default",
> +				"i2c_gpio", "spi_default", "spi_gpio";
> +		pinctrl-0 = <&mikrobus_gpio_pins_default>;
> +		pinctrl-1 = <&mikrobus_pwm_pins_default>;
> +		pinctrl-2 = <&mikrobus_pwm_pins_gpio>;
> +		pinctrl-3 = <&mikrobus_uart_pins_default>;
> +		pinctrl-4 = <&mikrobus_uart_pins_gpio>;
> +		pinctrl-5 = <&mikrobus_i2c_pins_default>;
> +		pinctrl-6 = <&mikrobus_i2c_pins_gpio>;
> +		pinctrl-7 = <&mikrobus_spi_pins_default>;
> +		pinctrl-8 = <&mikrobus_spi_pins_gpio>;
> +
> +		mikrobus-gpio-names = "pwm", "int", "rx", "tx", "scl", "sda",
> +				      "mosi", "miso", "sck", "cs", "rst", "an";
> +		mikrobus-gpios = <&main_gpio1 11 GPIO_ACTIVE_HIGH>,
> +				 <&main_gpio1 9 GPIO_ACTIVE_HIGH>,
> +				 <&main_gpio1 24 GPIO_ACTIVE_HIGH>,
> +				 <&main_gpio1 25 GPIO_ACTIVE_HIGH>,
> +				 <&main_gpio1 22 GPIO_ACTIVE_HIGH>,
> +				 <&main_gpio1 23 GPIO_ACTIVE_HIGH>,
> +				 <&main_gpio1 7 GPIO_ACTIVE_HIGH>,
> +				 <&main_gpio1 8 GPIO_ACTIVE_HIGH>,
> +				 <&main_gpio1 14 GPIO_ACTIVE_HIGH>,
> +				 <&main_gpio1 13 GPIO_ACTIVE_HIGH>,
> +				 <&main_gpio1 12 GPIO_ACTIVE_HIGH>,
> +				 <&main_gpio1 10 GPIO_ACTIVE_HIGH>;
> +
> +		spi-controller = <&main_spi2>;
> +		spi-cs = <0>;
> +		spi-cs-names = "default";
> +
> +		board = <&lsm6dsl_click>;
> +	};
> +
> +	mikrobus_boards {
> +		thermo_click: thermo-click {
> +			compatible = "maxim,max31855k", "mikrobus-spi";

I might be missing something, but your solution cannot possibly be
to list every click board that could be connected (all 1500+ of them)
to every mikroBUS connector on every device's DT file..

Each click board should have a single DTSO overlay file to describe the
click board, one per click board total. And then that overlay should
apply cleanly to any device that has a mikroBUS interface.

Which means you have not completely solved the fundamental problem of
abstracting the mikroBUS connector in DT. Each of these click device child
nodes has to be under the parent connector node. Which means a phandle
to the parent node, which is not generically named. For instance
if my board has 2 connectors, I would have mikrobus0 and mikrobus1,
the click board's overlay would look like this:

/dts-v1/;
/plugin/;

&mikrobus0 {
	status = "okay";

	mikrobus_board {
		thermo-click {
			compatible = "maxim,max31855k", "mikrobus-spi";
			spi-max-frequency = <1000000>;
			pinctrl-apply = "spi_default";
		};
	};
};

I think this solution is almost there, but once you solve the above
issue, we could just apply the right overlay for our attached click
board ahead of time and not need the mikroBUS bus driver at all.

Andrew

> +			spi-max-frequency = <1000000>;
> +			pinctrl-apply = "spi_default";
> +		};
> +
> +		lsm6dsl_click: lsm6dsl-click {
> +			compatible = "st,lsm6ds3", "mikrobus-spi";
> +			spi-max-frequency = <1000000>;
> +			pinctrl-apply = "spi_default";
> +		};
> +	};
>   };
>   
>   &main_pmx0 {
> @@ -387,6 +438,18 @@ AM62X_IOPAD(0x01f0, PIN_OUTPUT, 5) /* (A18) EXT_REFCLK1.CLKOUT0 */
>   		>;
>   	};
>   
> +	mikrobus_pwm_pins_default: mikrobus-pwm-default-pins {
> +		pinctrl-single,pins = <
> +			AM62X_IOPAD(0x01a4, PIN_INPUT, 2) /* (B20) MCASP0_ACLKX.ECAP2_IN_APWM_OUT */
> +		>;
> +	};
> +
> +	mikrobus_pwm_pins_gpio: mikrobus-pwm-gpio-pins {
> +		pinctrl-single,pins = <
> +			AM62X_IOPAD(0x01a4, PIN_INPUT, 7) /* (B20) MCASP0_ACLKX.GPIO1_11 */
> +		>;
> +	};
> +
>   	mikrobus_i2c_pins_default: mikrobus-i2c-default-pins {
>   		pinctrl-single,pins = <
>   			AM62X_IOPAD(0x01d0, PIN_INPUT_PULLUP, 2) /* (A15) UART0_CTSn.I2C3_SCL */
> @@ -394,6 +457,13 @@ AM62X_IOPAD(0x01d4, PIN_INPUT_PULLUP, 2) /* (B15) UART0_RTSn.I2C3_SDA */
>   		>;
>   	};
>   
> +	mikrobus_i2c_pins_gpio: mikrobus-i2c-gpio-pins {
> +		pinctrl-single,pins = <
> +			AM62X_IOPAD(0x01d0, PIN_INPUT, 7) /* (A15) UART0_CTSn.GPIO1_22 */
> +			AM62X_IOPAD(0x01d4, PIN_INPUT, 7) /* (B15) UART0_RTSn.GPIO1_23 */
> +		>;
> +	};
> +
>   	mikrobus_uart_pins_default: mikrobus-uart-default-pins {
>   		pinctrl-single,pins = <
>   			AM62X_IOPAD(0x01d8, PIN_INPUT, 1) /* (C15) MCAN0_TX.UART5_RXD */
> @@ -401,6 +471,13 @@ AM62X_IOPAD(0x01dc, PIN_OUTPUT, 1) /* (E15) MCAN0_RX.UART5_TXD */
>   		>;
>   	};
>   
> +	mikrobus_uart_pins_gpio: mikrobus-uart-gpio-pins {
> +		pinctrl-single,pins = <
> +			AM62X_IOPAD(0x01d8, PIN_INPUT, 7) /* (C15) MCAN0_TX.GPIO1_24 */
> +			AM62X_IOPAD(0x01dc, PIN_INPUT, 7) /* (E15) MCAN0_RX.GPIO1_25 */
> +		>;
> +	};
> +
>   	mikrobus_spi_pins_default: mikrobus-spi-default-pins {
>   		pinctrl-single,pins = <
>   			AM62X_IOPAD(0x01b0, PIN_INPUT, 1) /* (A20) MCASP0_ACLKR.SPI2_CLK */
> @@ -410,6 +487,15 @@ AM62X_IOPAD(0x0198, PIN_INPUT, 1) /* (A19) MCASP0_AXR2.SPI2_D1 */
>   		>;
>   	};
>   
> +	mikrobus_spi_pins_gpio: mikrobus-spi-gpio-pins {
> +		pinctrl-single,pins = <
> +			AM62X_IOPAD(0x0194, PIN_INPUT, 7) /* (B19) MCASP0_AXR3.GPIO1_7 */
> +			AM62X_IOPAD(0x0198, PIN_INPUT, 7) /* (A19) MCASP0_AXR2.GPIO1_8 */
> +			AM62X_IOPAD(0x01ac, PIN_INPUT, 7) /* (E19) MCASP0_AFSR.GPIO1_13 */
> +			AM62X_IOPAD(0x01b0, PIN_INPUT, 7) /* (A20) MCASP0_ACLKR.GPIO1_14 */
> +		>;
> +	};
> +
>   	mikrobus_gpio_pins_default: mikrobus-gpio-default-pins {
>   		bootph-all;
>   		pinctrl-single,pins = <
> @@ -630,8 +716,6 @@ &main_gpio0 {
>   
>   &main_gpio1 {
>   	bootph-all;
> -	pinctrl-names = "default";
> -	pinctrl-0 = <&mikrobus_gpio_pins_default>;
>   	gpio-line-names = "", "", "", "", "",			/* 0-4 */
>   		"SPE_RSTN", "SPE_INTN", "MIKROBUS_GPIO1_7",	/* 5-7 */
>   		"MIKROBUS_GPIO1_8", "MIKROBUS_GPIO1_9",		/* 8-9 */
> @@ -804,15 +888,11 @@ it66121_out: endpoint {
>   };
>   
>   &main_i2c3 {
> -	pinctrl-names = "default";
> -	pinctrl-0 = <&mikrobus_i2c_pins_default>;
>   	clock-frequency = <400000>;
>   	status = "okay";
>   };
>   
>   &main_spi2 {
> -	pinctrl-names = "default";
> -	pinctrl-0 = <&mikrobus_spi_pins_default>;
>   	status = "okay";
>   };
>   
> @@ -876,8 +956,6 @@ &main_uart1 {
>   };
>   
>   &main_uart5 {
> -	pinctrl-names = "default";
> -	pinctrl-0 = <&mikrobus_uart_pins_default>;
>   	status = "okay";
>   };
>   
>