All of lore.kernel.org
 help / color / mirror / Atom feed
From: Joris Offouga <offougajoris@gmail.com>
To: Marco Felsch <m.felsch@pengutronix.de>
Cc: Mark Rutland <mark.rutland@arm.com>,
	"open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS"
	<devicetree@vger.kernel.org>,
	Otavio Salvador <otavio@ossystems.com.br>,
	Fabio Estevam <festevam@gmail.com>,
	Sascha Hauer <s.hauer@pengutronix.de>,
	open list <linux-kernel@vger.kernel.org>,
	Rob Herring <robh+dt@kernel.org>,
	NXP Linux Team <linux-imx@nxp.com>,
	Pengutronix Kernel Team <kernel@pengutronix.de>,
	Shawn Guo <shawnguo@kernel.org>,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH] ARM: dts: imx7d-pico: Add LCD support
Date: Fri, 25 Oct 2019 23:16:47 +0200	[thread overview]
Message-ID: <5a73d00e-397a-f4ed-2bfa-bb26324685ba@gmail.com> (raw)
In-Reply-To: <20191025184544.7gwwbsrketjtwrwi@pengutronix.de>

Hi Marco,

Le 25/10/2019 à 20:45, Marco Felsch a écrit :
> Hi Joris,
>
> On 19-10-25 10:22, Joris Offouga wrote:
>> Add support for the VXT VL050-8048NT-C01 panel connected through
>> the 24 bit parallel LCDIF interface.
>>
>> Signed-off-by: Joris Offouga <offougajoris@gmail.com>
>> Signed-off-by: Fabio Estevam <festevam@gmail.com>
>> Signed-off-by: Otavio Salvador <otavio@ossystems.com.br>
>> ---
>>   arch/arm/boot/dts/imx7d-pico.dtsi | 84 +++++++++++++++++++++++++++++++
>>   1 file changed, 84 insertions(+)
>>
>> diff --git a/arch/arm/boot/dts/imx7d-pico.dtsi b/arch/arm/boot/dts/imx7d-pico.dtsi
>> index 6f50ebf31a0a..9042b1e6f1db 100644
>> --- a/arch/arm/boot/dts/imx7d-pico.dtsi
>> +++ b/arch/arm/boot/dts/imx7d-pico.dtsi
>> @@ -69,6 +69,37 @@
>>   		clocks = <&clks IMX7D_CLKO2_ROOT_DIV>;
>>   		clock-names = "ext_clock";
>>   	};
>> +
>> +	backlight: backlight {
>> +		compatible = "pwm-backlight";
>> +		pinctrl-names = "default";
>> +		pinctrl-0 = <&pinctrl_backlight>;
>> +		pwms = <&pwm4 0 50000 0>;
>                                        ^
>       If not inverted please drop the this flag.
> Also you need to add
>
> &pwm4 {
> 	status = "okay";
> };
This  node is already provide but i agree with you i move pinctrl in 
pwm4 node and rename the muxing
>
> And so you can do the pwm pinctrl within that node, see below.
>
>> +		brightness-levels = <0 36 72 108 144 180 216 255>;
>> +		default-brightness-level = <6>;
>> +		status = "okay";
> status can be dropped too.
>
>> +	};
>> +
>> +	reg_lcd_3v3: regulator-lcd-3v3 {
>> +		compatible = "regulator-fixed";
>> +		regulator-name = "lcd-3v3";
>> +		regulator-min-microvolt = <3300000>;
>> +		regulator-max-microvolt = <3300000>;
>> +		gpio = <&gpio1 6 GPIO_ACTIVE_HIGH>;
> Missing the muxing?
No this pin is already drive
>
>> +		enable-active-high;
>> +	};
>> +
>> +	panel {
>> +		compatible = "vxt,vl050-8048nt-c01";
>> +		backlight = <&backlight>;
>> +		power-supply = <&reg_lcd_3v3>;
>> +
>> +		port {
>> +			panel_in: endpoint {
>> +				remote-endpoint = <&display_out>;
>> +			};
>> +		};
>> +	};
> Please sort the nodes alphabetical.

okay thanks for your review

otherwise Fabio made me notice that I should leave his From however with 
the changes made I should put mine?

Best regards,

Joris

>
>>   };
>>   
>>   &clks {
>> @@ -230,6 +261,18 @@
>>   	};
>>   };
>>   
>> +&lcdif {
>> +	pinctrl-names = "default";
>> +	pinctrl-0 = <&pinctrl_lcdif>;
>> +	status = "okay";
>> +
>> +	port {
>> +		display_out: endpoint {
>> +			remote-endpoint = <&panel_in>;
>> +		};
>> +	};
>> +};
>> +
>>   &sai1 {
>>   	pinctrl-names = "default";
>>   	pinctrl-0 = <&pinctrl_sai1>;
>> @@ -349,6 +392,13 @@
>>   };
>>   
>>   &iomuxc {
>> +
>> +	pinctrl_backlight: backlight {
>                                      ^
>                          please add 'grp'
>> +		fsl,pins = <
>> +			MX7D_PAD_GPIO1_IO11__PWM4_OUT		0x0
>> +		>;
>> +	};
> IMHO the muxing is part of the pwm4 node. So rename it to
> 'pinctrl_pwm4: pwm4grp'.
>
> Regards,
>    Marco
>
>> +
>>   	pinctrl_ecspi3: ecspi3grp {
>>   		fsl,pins = <
>>   			MX7D_PAD_I2C1_SCL__ECSPI3_MISO		0x2
>> @@ -413,6 +463,40 @@
>>   		>;
>>   	};
>>   
>> +	pinctrl_lcdif: lcdifgrp {
>> +		fsl,pins = <
>> +			MX7D_PAD_LCD_DATA00__LCD_DATA0		0x79
>> +			MX7D_PAD_LCD_DATA01__LCD_DATA1		0x79
>> +			MX7D_PAD_LCD_DATA02__LCD_DATA2		0x79
>> +			MX7D_PAD_LCD_DATA03__LCD_DATA3		0x79
>> +			MX7D_PAD_LCD_DATA04__LCD_DATA4		0x79
>> +			MX7D_PAD_LCD_DATA05__LCD_DATA5		0x79
>> +			MX7D_PAD_LCD_DATA06__LCD_DATA6		0x79
>> +			MX7D_PAD_LCD_DATA07__LCD_DATA7		0x79
>> +			MX7D_PAD_LCD_DATA08__LCD_DATA8		0x79
>> +			MX7D_PAD_LCD_DATA09__LCD_DATA9		0x79
>> +			MX7D_PAD_LCD_DATA10__LCD_DATA10		0x79
>> +			MX7D_PAD_LCD_DATA11__LCD_DATA11		0x79
>> +			MX7D_PAD_LCD_DATA12__LCD_DATA12		0x79
>> +			MX7D_PAD_LCD_DATA13__LCD_DATA13		0x79
>> +			MX7D_PAD_LCD_DATA14__LCD_DATA14		0x79
>> +			MX7D_PAD_LCD_DATA15__LCD_DATA15		0x79
>> +			MX7D_PAD_LCD_DATA16__LCD_DATA16		0x79
>> +			MX7D_PAD_LCD_DATA17__LCD_DATA17		0x79
>> +			MX7D_PAD_LCD_DATA18__LCD_DATA18		0x79
>> +			MX7D_PAD_LCD_DATA19__LCD_DATA19		0x79
>> +			MX7D_PAD_LCD_DATA20__LCD_DATA20		0x79
>> +			MX7D_PAD_LCD_DATA21__LCD_DATA21		0x79
>> +			MX7D_PAD_LCD_DATA22__LCD_DATA22		0x79
>> +			MX7D_PAD_LCD_DATA23__LCD_DATA23		0x79
>> +			MX7D_PAD_LCD_CLK__LCD_CLK		0x79
>> +			MX7D_PAD_LCD_ENABLE__LCD_ENABLE		0x78
>> +			MX7D_PAD_LCD_VSYNC__LCD_VSYNC		0x78
>> +			MX7D_PAD_LCD_HSYNC__LCD_HSYNC		0x78
>> +			MX7D_PAD_LCD_RESET__GPIO3_IO4		0x14
>> +		>;
>> +	};
>> +
>>   	pinctrl_pwm1: pwm1 {
>>   		fsl,pins = <
>>   			MX7D_PAD_GPIO1_IO08__PWM1_OUT   0x7f
>> -- 
>> 2.17.1
>>
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

WARNING: multiple messages have this Message-ID (diff)
From: Joris Offouga <offougajoris@gmail.com>
To: Marco Felsch <m.felsch@pengutronix.de>
Cc: linux-arm-kernel@lists.infradead.org,
	Mark Rutland <mark.rutland@arm.com>,
	"open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
	<devicetree@vger.kernel.org>,
	Otavio Salvador <otavio@ossystems.com.br>,
	Shawn Guo <shawnguo@kernel.org>,
	Sascha Hauer <s.hauer@pengutronix.de>,
	open list <linux-kernel@vger.kernel.org>,
	Rob Herring <robh+dt@kernel.org>,
	NXP Linux Team <linux-imx@nxp.com>,
	Pengutronix Kernel Team <kernel@pengutronix.de>,
	Fabio Estevam <festevam@gmail.com>
Subject: Re: [PATCH] ARM: dts: imx7d-pico: Add LCD support
Date: Fri, 25 Oct 2019 23:16:47 +0200	[thread overview]
Message-ID: <5a73d00e-397a-f4ed-2bfa-bb26324685ba@gmail.com> (raw)
In-Reply-To: <20191025184544.7gwwbsrketjtwrwi@pengutronix.de>

Hi Marco,

Le 25/10/2019 à 20:45, Marco Felsch a écrit :
> Hi Joris,
>
> On 19-10-25 10:22, Joris Offouga wrote:
>> Add support for the VXT VL050-8048NT-C01 panel connected through
>> the 24 bit parallel LCDIF interface.
>>
>> Signed-off-by: Joris Offouga <offougajoris@gmail.com>
>> Signed-off-by: Fabio Estevam <festevam@gmail.com>
>> Signed-off-by: Otavio Salvador <otavio@ossystems.com.br>
>> ---
>>   arch/arm/boot/dts/imx7d-pico.dtsi | 84 +++++++++++++++++++++++++++++++
>>   1 file changed, 84 insertions(+)
>>
>> diff --git a/arch/arm/boot/dts/imx7d-pico.dtsi b/arch/arm/boot/dts/imx7d-pico.dtsi
>> index 6f50ebf31a0a..9042b1e6f1db 100644
>> --- a/arch/arm/boot/dts/imx7d-pico.dtsi
>> +++ b/arch/arm/boot/dts/imx7d-pico.dtsi
>> @@ -69,6 +69,37 @@
>>   		clocks = <&clks IMX7D_CLKO2_ROOT_DIV>;
>>   		clock-names = "ext_clock";
>>   	};
>> +
>> +	backlight: backlight {
>> +		compatible = "pwm-backlight";
>> +		pinctrl-names = "default";
>> +		pinctrl-0 = <&pinctrl_backlight>;
>> +		pwms = <&pwm4 0 50000 0>;
>                                        ^
>       If not inverted please drop the this flag.
> Also you need to add
>
> &pwm4 {
> 	status = "okay";
> };
This  node is already provide but i agree with you i move pinctrl in 
pwm4 node and rename the muxing
>
> And so you can do the pwm pinctrl within that node, see below.
>
>> +		brightness-levels = <0 36 72 108 144 180 216 255>;
>> +		default-brightness-level = <6>;
>> +		status = "okay";
> status can be dropped too.
>
>> +	};
>> +
>> +	reg_lcd_3v3: regulator-lcd-3v3 {
>> +		compatible = "regulator-fixed";
>> +		regulator-name = "lcd-3v3";
>> +		regulator-min-microvolt = <3300000>;
>> +		regulator-max-microvolt = <3300000>;
>> +		gpio = <&gpio1 6 GPIO_ACTIVE_HIGH>;
> Missing the muxing?
No this pin is already drive
>
>> +		enable-active-high;
>> +	};
>> +
>> +	panel {
>> +		compatible = "vxt,vl050-8048nt-c01";
>> +		backlight = <&backlight>;
>> +		power-supply = <&reg_lcd_3v3>;
>> +
>> +		port {
>> +			panel_in: endpoint {
>> +				remote-endpoint = <&display_out>;
>> +			};
>> +		};
>> +	};
> Please sort the nodes alphabetical.

okay thanks for your review

otherwise Fabio made me notice that I should leave his From however with 
the changes made I should put mine?

Best regards,

Joris

>
>>   };
>>   
>>   &clks {
>> @@ -230,6 +261,18 @@
>>   	};
>>   };
>>   
>> +&lcdif {
>> +	pinctrl-names = "default";
>> +	pinctrl-0 = <&pinctrl_lcdif>;
>> +	status = "okay";
>> +
>> +	port {
>> +		display_out: endpoint {
>> +			remote-endpoint = <&panel_in>;
>> +		};
>> +	};
>> +};
>> +
>>   &sai1 {
>>   	pinctrl-names = "default";
>>   	pinctrl-0 = <&pinctrl_sai1>;
>> @@ -349,6 +392,13 @@
>>   };
>>   
>>   &iomuxc {
>> +
>> +	pinctrl_backlight: backlight {
>                                      ^
>                          please add 'grp'
>> +		fsl,pins = <
>> +			MX7D_PAD_GPIO1_IO11__PWM4_OUT		0x0
>> +		>;
>> +	};
> IMHO the muxing is part of the pwm4 node. So rename it to
> 'pinctrl_pwm4: pwm4grp'.
>
> Regards,
>    Marco
>
>> +
>>   	pinctrl_ecspi3: ecspi3grp {
>>   		fsl,pins = <
>>   			MX7D_PAD_I2C1_SCL__ECSPI3_MISO		0x2
>> @@ -413,6 +463,40 @@
>>   		>;
>>   	};
>>   
>> +	pinctrl_lcdif: lcdifgrp {
>> +		fsl,pins = <
>> +			MX7D_PAD_LCD_DATA00__LCD_DATA0		0x79
>> +			MX7D_PAD_LCD_DATA01__LCD_DATA1		0x79
>> +			MX7D_PAD_LCD_DATA02__LCD_DATA2		0x79
>> +			MX7D_PAD_LCD_DATA03__LCD_DATA3		0x79
>> +			MX7D_PAD_LCD_DATA04__LCD_DATA4		0x79
>> +			MX7D_PAD_LCD_DATA05__LCD_DATA5		0x79
>> +			MX7D_PAD_LCD_DATA06__LCD_DATA6		0x79
>> +			MX7D_PAD_LCD_DATA07__LCD_DATA7		0x79
>> +			MX7D_PAD_LCD_DATA08__LCD_DATA8		0x79
>> +			MX7D_PAD_LCD_DATA09__LCD_DATA9		0x79
>> +			MX7D_PAD_LCD_DATA10__LCD_DATA10		0x79
>> +			MX7D_PAD_LCD_DATA11__LCD_DATA11		0x79
>> +			MX7D_PAD_LCD_DATA12__LCD_DATA12		0x79
>> +			MX7D_PAD_LCD_DATA13__LCD_DATA13		0x79
>> +			MX7D_PAD_LCD_DATA14__LCD_DATA14		0x79
>> +			MX7D_PAD_LCD_DATA15__LCD_DATA15		0x79
>> +			MX7D_PAD_LCD_DATA16__LCD_DATA16		0x79
>> +			MX7D_PAD_LCD_DATA17__LCD_DATA17		0x79
>> +			MX7D_PAD_LCD_DATA18__LCD_DATA18		0x79
>> +			MX7D_PAD_LCD_DATA19__LCD_DATA19		0x79
>> +			MX7D_PAD_LCD_DATA20__LCD_DATA20		0x79
>> +			MX7D_PAD_LCD_DATA21__LCD_DATA21		0x79
>> +			MX7D_PAD_LCD_DATA22__LCD_DATA22		0x79
>> +			MX7D_PAD_LCD_DATA23__LCD_DATA23		0x79
>> +			MX7D_PAD_LCD_CLK__LCD_CLK		0x79
>> +			MX7D_PAD_LCD_ENABLE__LCD_ENABLE		0x78
>> +			MX7D_PAD_LCD_VSYNC__LCD_VSYNC		0x78
>> +			MX7D_PAD_LCD_HSYNC__LCD_HSYNC		0x78
>> +			MX7D_PAD_LCD_RESET__GPIO3_IO4		0x14
>> +		>;
>> +	};
>> +
>>   	pinctrl_pwm1: pwm1 {
>>   		fsl,pins = <
>>   			MX7D_PAD_GPIO1_IO08__PWM1_OUT   0x7f
>> -- 
>> 2.17.1
>>
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>

  reply	other threads:[~2019-10-25 21:17 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-10-25  8:22 [PATCH] ARM: dts: imx7d-pico: Add LCD support Joris Offouga
2019-10-25  8:22 ` Joris Offouga
2019-10-25 13:42 ` Fabio Estevam
2019-10-25 13:42   ` Fabio Estevam
2019-10-25 18:45 ` Marco Felsch
2019-10-25 18:45   ` Marco Felsch
2019-10-25 21:16   ` Joris Offouga [this message]
2019-10-25 21:16     ` Joris Offouga
2019-10-26 19:20     ` Fabio Estevam
2019-10-26 19:20       ` Fabio Estevam
2019-10-26 20:29       ` Joris Offouga
2019-10-26 20:29         ` Joris Offouga

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=5a73d00e-397a-f4ed-2bfa-bb26324685ba@gmail.com \
    --to=offougajoris@gmail.com \
    --cc=devicetree@vger.kernel.org \
    --cc=festevam@gmail.com \
    --cc=kernel@pengutronix.de \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-imx@nxp.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=m.felsch@pengutronix.de \
    --cc=mark.rutland@arm.com \
    --cc=otavio@ossystems.com.br \
    --cc=robh+dt@kernel.org \
    --cc=s.hauer@pengutronix.de \
    --cc=shawnguo@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.