Re: [PATCH 3/3] ARM: dts: imx6ul: Add Variscite Concerto board support

["Antonin Godard" <antonin.godard@bootlin.com>]

Hi Krzysztof,

On Tue Jan 21, 2025 at 5:03 PM CET, Krzysztof Kozlowski wrote:
> On 21/01/2025 10:33, Antonin Godard wrote:
>> This patch adds support for the Variscite Concerto Carrier Board.
>
> Please do not use "This commit/patch/change", but imperative mood. See
> longer explanation here:
> https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95

Will do in v2.

>>
>> This Carrier-Board has the following:
>> - LVDS interface for the VLCD-CAP-GLD-LVDS 7" LCD 800 x 480 touch
>>   display (not configured)
>> - USB Host + USB OTG Connector
>> - 10/100 Mbps Ethernet
>> - miniPCI-Express slot
>> - SD Card connector
>> - Audio Headphone/Line In jack connectors
>> - S-ATA
>> - On-board DMIC
>> 
>> Product Page: https://www.variscite.com/product/single-board-computers/concerto-board
>> 
>> This file is based on the one provided by Variscite on their own kernel,
>> but adapted for mainline.
>> 
>> Signed-off-by: Antonin Godard <antonin.godard@bootlin.com>
>> ---
>>  arch/arm/boot/dts/nxp/imx/Makefile                 |   1 +
>>  .../boot/dts/nxp/imx/imx6ul-var-som-concerto.dts   | 331 +++++++++++++++++++++
>>  2 files changed, 332 insertions(+)
>> 
>> diff --git a/arch/arm/boot/dts/nxp/imx/Makefile b/arch/arm/boot/dts/nxp/imx/Makefile
>> index 39a153536d2a2b8f75b5fbe4332660f89442064a..94c9bc94cc8e2daa1fb3b5686b0b58db1f6678b6 100644
>> --- a/arch/arm/boot/dts/nxp/imx/Makefile
>> +++ b/arch/arm/boot/dts/nxp/imx/Makefile
>> @@ -329,6 +329,7 @@ dtb-$(CONFIG_SOC_IMX6UL) += \
>>  	imx6ul-tx6ul-0010.dtb \
>>  	imx6ul-tx6ul-0011.dtb \
>>  	imx6ul-tx6ul-mainboard.dtb \
>> +	imx6ul-var-som-concerto.dtb \
>>  	imx6ull-14x14-evk.dtb \
>>  	imx6ull-colibri-aster.dtb \
>>  	imx6ull-colibri-emmc-aster.dtb \
>> diff --git a/arch/arm/boot/dts/nxp/imx/imx6ul-var-som-concerto.dts b/arch/arm/boot/dts/nxp/imx/imx6ul-var-som-concerto.dts
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..4289641d94c5a72ba985f339652039dbf13da40c
>> --- /dev/null
>> +++ b/arch/arm/boot/dts/nxp/imx/imx6ul-var-som-concerto.dts
>> @@ -0,0 +1,331 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +/*
>> + * Support for Variscite MX6 Concerto Carrier board with the VAR-SOM-MX6UL
>> + * Variscite SoM mounted on it
>> + *
>> + * Copyright 2019 Variscite Ltd.
>> + * Copyright 2025 Bootlin
>> + */
>> +
>> +#include "imx6ul-var-som.dtsi"
>> +
>> +/ {
>> +	model = "Variscite VAR-SOM-MX6UL Concerto Board";
>> +	compatible = "variscite,mx6concerto", "variscite,var-som-imx6ul", "fsl,imx6ul";
>> +
>> +	backlight {
>> +		compatible = "pwm-backlight";
>> +		pwms = <&pwm4 0 20000 0>;
>> +		brightness-levels = <0 4 8 16 32 64 128 255>;
>> +		default-brightness-level = <6>;
>> +		status = "okay";
>
> Which file disables it?

This is a mistake, I forgot to remove this when removing the parts I couldn't
test. Will remove this node in v2.

>> +	};
>> +
>> +	chosen {
>> +		stdout-path = &uart1;
>> +	};
>> +
>> +	gpio-keys {
>> +		compatible = "gpio-keys";
>> +		pinctrl-names = "default";
>> +		pinctrl-0 = <&pinctrl_gpio_key_back>, <&pinctrl_gpio_key_wakeup>;
>> +
>> +		key-back {
>> +			gpios = <&gpio4 14 GPIO_ACTIVE_LOW>;
>> +			linux,code = <KEY_BACK>;
>> +		};
>> +
>> +		key-wakeup {
>> +			gpios = <&gpio5 8 GPIO_ACTIVE_LOW>;
>> +			linux,code = <KEY_WAKEUP>;
>> +			wakeup-source;
>> +		};
>> +	};
>> +
>> +	leds {
>> +		compatible = "gpio-leds";
>> +		pinctrl-names = "default";
>> +		pinctrl-0 = <&pinctrl_gpio_leds>;
>> +
>> +		gpled2 {
>
> led-0
> led-1
> led-2

Will rename this node to led-0, and set the label to "gpled2" (this is how it's
named on the schematic/datasheet).

> Are there other leds here?

Nothing else is obvious to me on the schematic. There is no gpled0 or gpled1.

>> +			gpios = <&gpio1 25 GPIO_ACTIVE_HIGH>;
>> +			linux,default-trigger = "heartbeat";
>
> Missing function and color

Will set the function to STATUS, color green. This is a general purpose led that
can be used for debugging purposes or as a status indicator, I think.

>> +		};
>> +	};
>> +};
>> +
>> +&can1 {
>> +	pinctrl-names = "default";
>> +	pinctrl-0 = <&pinctrl_flexcan1>;
>> +	status = "okay";
>> +};
>> +
>> +&fec1 {
>> +	status = "disabled";
>> +};
>> +
>> +&fec2 {
>> +	pinctrl-names = "default";
>> +	pinctrl-0 = <&pinctrl_enet2>, <&pinctrl_enet2_gpio>, <&pinctrl_enet2_mdio>;
>> +	phy-mode = "rmii";
>> +	phy-handle = <&ethphy1>;
>> +	phy-reset-gpios = <&gpio5 5 GPIO_ACTIVE_LOW>;
>> +	phy-reset-duration = <100>;
>> +	status = "okay";
>> +
>> +	mdio {
>> +		#address-cells = <1>;
>> +		#size-cells = <0>;
>> +
>> +		ethphy1: ethernet-phy@3 {
>> +			compatible = "ethernet-phy-ieee802.3-c22";
>> +			micrel,rmii-reference-clock-select-25-mhz = <1>;
>> +			micrel,led-mode = <0>;
>> +			clocks = <&rmii_ref_clk>;
>> +			clock-names = "rmii-ref";
>> +			reg = <3>;
>> +		};
>> +	};
>> +};
>> +
>> +&i2c1 {
>> +	clock-frequency = <100000>;
>> +	pinctrl-names = "default";
>> +	pinctrl-0 = <&pinctrl_i2c1>;
>> +	status = "okay";
>> +
>> +	/* DS1337 RTC module */
>
> Drop comment, obvious. This cannot be anything else, because node name
> and compatible told that.

Will do in v2.

>> +	rtc@68 {
>> +		/*
>> +		 * To actually use this interrupt
>> +		 * connect pins J14.8 & J14.10 on the Concerto-Board.
>> +		 */
>> +		compatible = "dallas,ds1337";
>> +		reg = <0x68>;
>> +		pinctrl-names = "default";
>> +		pinctrl-0 = <&pinctrl_rtc>;
>> +		interrupt-parent = <&gpio1>;
>> +		interrupts = <10 IRQ_TYPE_EDGE_FALLING>;
>> +	};
>> +};
>
>
> Best regards,
> Krzysztof

Thanks for the review,
Antonin

-- 
Antonin Godard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com