diff mbox series

[03/12] arm64: dts: ti: k3-j721e-beagleboneai64: Fixup reference to phandles array

Message ID 20230601152636.858553-4-nm@ti.com (mailing list archive)
State New, archived
Headers show
Series arm64: dts: ti: Fix up references to phandles | expand

Commit Message

Nishanth Menon June 1, 2023, 3:26 p.m. UTC
When referring to array of phandles, using <> to separate the array
entries is better notation as it makes potential errors with phandle and
cell arguments easier to catch. Fix the outliers to be consistent with
the rest of the usage.

Cc: Robert Nelson <robertcnelson@gmail.com>
Signed-off-by: Nishanth Menon <nm@ti.com>
---
 .../boot/dts/ti/k3-j721e-beagleboneai64.dts   | 29 ++++++++++---------
 1 file changed, 15 insertions(+), 14 deletions(-)

Comments

Kumar, Udit June 5, 2023, 5:01 p.m. UTC | #1
Hi Nishanth

On 6/1/2023 8:56 PM, Nishanth Menon wrote:
> When referring to array of phandles, using <> to separate the array
> entries is better notation as it makes potential errors with phandle and
> cell arguments easier to catch. Fix the outliers to be consistent with
> the rest of the usage.
>
> Cc: Robert Nelson <robertcnelson@gmail.com>
> Signed-off-by: Nishanth Menon <nm@ti.com>
> ---
>   .../boot/dts/ti/k3-j721e-beagleboneai64.dts   | 29 ++++++++++---------
>   1 file changed, 15 insertions(+), 14 deletions(-)
>
> diff --git a/arch/arm64/boot/dts/ti/k3-j721e-beagleboneai64.dts b/arch/arm64/boot/dts/ti/k3-j721e-beagleboneai64.dts
> index 37c24b077b6a..c13246a9ed8f 100644
> --- a/arch/arm64/boot/dts/ti/k3-j721e-beagleboneai64.dts
> +++ b/arch/arm64/boot/dts/ti/k3-j721e-beagleboneai64.dts
> @@ -593,7 +593,7 @@ &main_i2c0 {
>   &main_i2c1 {
>   	status = "okay";
>   	pinctrl-names = "default";
> -	pinctrl-0 = <&main_i2c1_pins_default &csi1_gpio_pins_default>;
> +	pinctrl-0 = <&main_i2c1_pins_default>, <&csi1_gpio_pins_default>;
>   	clock-frequency = <400000>;
>   };
>   
> @@ -623,7 +623,7 @@ &main_i2c4 {
>   &main_i2c5 {
>   	status = "okay";
>   	pinctrl-names = "default";
> -	pinctrl-0 = <&main_i2c5_pins_default &csi0_gpio_pins_default>;
> +	pinctrl-0 = <&main_i2c5_pins_default>, <&csi0_gpio_pins_default>;
>   	clock-frequency = <400000>;
>   };
>   
> @@ -639,7 +639,7 @@ &main_i2c6 {
>   &wkup_i2c0 {
>   	status = "okay";
>   	pinctrl-names = "default";
> -	pinctrl-0 = <&wkup_i2c0_pins_default &eeprom_wp_pins_default>;
> +	pinctrl-0 = <&wkup_i2c0_pins_default>, <&eeprom_wp_pins_default>;
>   	clock-frequency = <400000>;

Why we need more than 2 pio lines for i2c node ,


> [...]
>
Nishanth Menon June 5, 2023, 8:49 p.m. UTC | #2
On 22:31-20230605, Kumar, Udit wrote:
[...]
> > diff --git a/arch/arm64/boot/dts/ti/k3-j721e-beagleboneai64.dts b/arch/arm64/boot/dts/ti/k3-j721e-beagleboneai64.dts
> > index 37c24b077b6a..c13246a9ed8f 100644
> > --- a/arch/arm64/boot/dts/ti/k3-j721e-beagleboneai64.dts
> > +++ b/arch/arm64/boot/dts/ti/k3-j721e-beagleboneai64.dts
[...]
> > @@ -639,7 +639,7 @@ &main_i2c6 {
> >   &wkup_i2c0 {
> >   	status = "okay";
> >   	pinctrl-names = "default";
> > -	pinctrl-0 = <&wkup_i2c0_pins_default &eeprom_wp_pins_default>;
> > +	pinctrl-0 = <&wkup_i2c0_pins_default>, <&eeprom_wp_pins_default>;
> >   	clock-frequency = <400000>;
> 
> Why we need more than 2 pio lines for i2c node ,

pio lines? I am not sure I understand. If you are suggesting
eeprom_wp_pins to be moved to the eeprom node, It is probably
un-related to this series, but OK, i think it is probably a valid
change (unless Robert sees a reason why he did it the way he did).
Kumar, Udit June 6, 2023, 4:26 a.m. UTC | #3
Hi Nishanth,

On 6/6/2023 2:19 AM, Nishanth Menon wrote:
> On 22:31-20230605, Kumar, Udit wrote:
> [...]
>>> diff --git a/arch/arm64/boot/dts/ti/k3-j721e-beagleboneai64.dts b/arch/arm64/boot/dts/ti/k3-j721e-beagleboneai64.dts
>>> index 37c24b077b6a..c13246a9ed8f 100644
>>> --- a/arch/arm64/boot/dts/ti/k3-j721e-beagleboneai64.dts
>>> +++ b/arch/arm64/boot/dts/ti/k3-j721e-beagleboneai64.dts
> [...]
>>> @@ -639,7 +639,7 @@ &main_i2c6 {
>>>    &wkup_i2c0 {
>>>    	status = "okay";
>>>    	pinctrl-names = "default";
>>> -	pinctrl-0 = <&wkup_i2c0_pins_default &eeprom_wp_pins_default>;
>>> +	pinctrl-0 = <&wkup_i2c0_pins_default>, <&eeprom_wp_pins_default>;
>>>    	clock-frequency = <400000>;
>> Why we need more than 2 pio lines for i2c node ,
> pio lines? I am not sure I understand. If you are suggesting
> eeprom_wp_pins to be moved to the eeprom node, It is probably
> un-related to this series, but OK, i think it is probably a valid
> change (unless Robert sees a reason why he did it the way he did).

correct, I am suggesting to move  eeprom_wp_pins_default to eeprom node.

i2c needs 2 lines which are defined in wkup_i2c0_pins_default, Adding 
eeprom_wp_pins_default will not be true representation of i2c node.

It will be good to have similar changes in main_i2c1 and main_i2c5  node 
for csi0_gpio_pins_default and csi1_gpio_pins_default.
Nishanth Menon June 6, 2023, 11:41 a.m. UTC | #4
On 09:56-20230606, Kumar, Udit wrote:
> On 6/6/2023 2:19 AM, Nishanth Menon wrote:
> > On 22:31-20230605, Kumar, Udit wrote:
> > [...]
> > > > diff --git a/arch/arm64/boot/dts/ti/k3-j721e-beagleboneai64.dts b/arch/arm64/boot/dts/ti/k3-j721e-beagleboneai64.dts
> > > > index 37c24b077b6a..c13246a9ed8f 100644
> > > > --- a/arch/arm64/boot/dts/ti/k3-j721e-beagleboneai64.dts
> > > > +++ b/arch/arm64/boot/dts/ti/k3-j721e-beagleboneai64.dts
> > [...]
> > > > @@ -639,7 +639,7 @@ &main_i2c6 {
> > > >    &wkup_i2c0 {
> > > >    	status = "okay";
> > > >    	pinctrl-names = "default";
> > > > -	pinctrl-0 = <&wkup_i2c0_pins_default &eeprom_wp_pins_default>;
> > > > +	pinctrl-0 = <&wkup_i2c0_pins_default>, <&eeprom_wp_pins_default>;
> > > >    	clock-frequency = <400000>;
> > > Why we need more than 2 pio lines for i2c node ,
> > pio lines? I am not sure I understand. If you are suggesting
> > eeprom_wp_pins to be moved to the eeprom node, It is probably
> > un-related to this series, but OK, i think it is probably a valid
> > change (unless Robert sees a reason why he did it the way he did).
> 
> correct, I am suggesting to move  eeprom_wp_pins_default to eeprom node.
> 
> i2c needs 2 lines which are defined in wkup_i2c0_pins_default, Adding
> eeprom_wp_pins_default will not be true representation of i2c node.
> 
> It will be good to have similar changes in main_i2c1 and main_i2c5  node for
> csi0_gpio_pins_default and csi1_gpio_pins_default.

Robert: your opinion here?
Robert Nelson June 6, 2023, 2:27 p.m. UTC | #5
On Mon, Jun 5, 2023 at 11:27 PM Kumar, Udit <u-kumar1@ti.com> wrote:
>
> Hi Nishanth,
>
> On 6/6/2023 2:19 AM, Nishanth Menon wrote:
> > On 22:31-20230605, Kumar, Udit wrote:
> > [...]
> >>> diff --git a/arch/arm64/boot/dts/ti/k3-j721e-beagleboneai64.dts b/arch/arm64/boot/dts/ti/k3-j721e-beagleboneai64.dts
> >>> index 37c24b077b6a..c13246a9ed8f 100644
> >>> --- a/arch/arm64/boot/dts/ti/k3-j721e-beagleboneai64.dts
> >>> +++ b/arch/arm64/boot/dts/ti/k3-j721e-beagleboneai64.dts
> > [...]
> >>> @@ -639,7 +639,7 @@ &main_i2c6 {
> >>>    &wkup_i2c0 {
> >>>     status = "okay";
> >>>     pinctrl-names = "default";
> >>> -   pinctrl-0 = <&wkup_i2c0_pins_default &eeprom_wp_pins_default>;
> >>> +   pinctrl-0 = <&wkup_i2c0_pins_default>, <&eeprom_wp_pins_default>;
> >>>     clock-frequency = <400000>;
> >> Why we need more than 2 pio lines for i2c node ,
> > pio lines? I am not sure I understand. If you are suggesting
> > eeprom_wp_pins to be moved to the eeprom node, It is probably
> > un-related to this series, but OK, i think it is probably a valid
> > change (unless Robert sees a reason why he did it the way he did).
>
> correct, I am suggesting to move  eeprom_wp_pins_default to eeprom node.
>
> i2c needs 2 lines which are defined in wkup_i2c0_pins_default, Adding
> eeprom_wp_pins_default will not be true representation of i2c node.
>
> It will be good to have similar changes in main_i2c1 and main_i2c5  node
> for csi0_gpio_pins_default and csi1_gpio_pins_default.

I agree, moving eeprom_wp_pins_default into the eeprom node itself is
much cleaner going forward.

While we may have a lot of historical situations in the git tree where
we just dumped all pin configurations into the base node, that's not
the best practice going forward today.

Regards,
diff mbox series

Patch

diff --git a/arch/arm64/boot/dts/ti/k3-j721e-beagleboneai64.dts b/arch/arm64/boot/dts/ti/k3-j721e-beagleboneai64.dts
index 37c24b077b6a..c13246a9ed8f 100644
--- a/arch/arm64/boot/dts/ti/k3-j721e-beagleboneai64.dts
+++ b/arch/arm64/boot/dts/ti/k3-j721e-beagleboneai64.dts
@@ -593,7 +593,7 @@  &main_i2c0 {
 &main_i2c1 {
 	status = "okay";
 	pinctrl-names = "default";
-	pinctrl-0 = <&main_i2c1_pins_default &csi1_gpio_pins_default>;
+	pinctrl-0 = <&main_i2c1_pins_default>, <&csi1_gpio_pins_default>;
 	clock-frequency = <400000>;
 };
 
@@ -623,7 +623,7 @@  &main_i2c4 {
 &main_i2c5 {
 	status = "okay";
 	pinctrl-names = "default";
-	pinctrl-0 = <&main_i2c5_pins_default &csi0_gpio_pins_default>;
+	pinctrl-0 = <&main_i2c5_pins_default>, <&csi0_gpio_pins_default>;
 	clock-frequency = <400000>;
 };
 
@@ -639,7 +639,7 @@  &main_i2c6 {
 &wkup_i2c0 {
 	status = "okay";
 	pinctrl-names = "default";
-	pinctrl-0 = <&wkup_i2c0_pins_default &eeprom_wp_pins_default>;
+	pinctrl-0 = <&wkup_i2c0_pins_default>, <&eeprom_wp_pins_default>;
 	clock-frequency = <400000>;
 
 	eeprom@50 {
@@ -680,7 +680,8 @@  &main_gpio7 {
 
 &wkup_gpio0 {
 	pinctrl-names = "default";
-	pinctrl-0 = <&mcu_adc0_pins_default &mcu_adc1_pins_default &mikro_bus_pins_default>;
+	pinctrl-0 = <&mcu_adc0_pins_default>, <&mcu_adc1_pins_default>,
+		    <&mikro_bus_pins_default>;
 };
 
 &wkup_gpio1 {
@@ -759,7 +760,7 @@  serdes2_usb_link: phy@1 {
 
 &usbss1 {
 	pinctrl-names = "default";
-	pinctrl-0 = <&main_usbss1_pins_default &mcu_usbss1_pins_default>;
+	pinctrl-0 = <&main_usbss1_pins_default>, <&mcu_usbss1_pins_default>;
 	ti,vbus-divider;
 };
 
@@ -1001,55 +1002,55 @@  mbox_c71_0: mbox-c71-0 {
 };
 
 &mcu_r5fss0_core0 {
-	mboxes = <&mailbox0_cluster0 &mbox_mcu_r5fss0_core0>;
+	mboxes = <&mailbox0_cluster0>, <&mbox_mcu_r5fss0_core0>;
 	memory-region = <&mcu_r5fss0_core0_dma_memory_region>,
 			<&mcu_r5fss0_core0_memory_region>;
 };
 
 &mcu_r5fss0_core1 {
-	mboxes = <&mailbox0_cluster0 &mbox_mcu_r5fss0_core1>;
+	mboxes = <&mailbox0_cluster0>, <&mbox_mcu_r5fss0_core1>;
 	memory-region = <&mcu_r5fss0_core1_dma_memory_region>,
 			<&mcu_r5fss0_core1_memory_region>;
 };
 
 &main_r5fss0_core0 {
-	mboxes = <&mailbox0_cluster1 &mbox_main_r5fss0_core0>;
+	mboxes = <&mailbox0_cluster1>, <&mbox_main_r5fss0_core0>;
 	memory-region = <&main_r5fss0_core0_dma_memory_region>,
 			<&main_r5fss0_core0_memory_region>;
 };
 
 &main_r5fss0_core1 {
-	mboxes = <&mailbox0_cluster1 &mbox_main_r5fss0_core1>;
+	mboxes = <&mailbox0_cluster1>, <&mbox_main_r5fss0_core1>;
 	memory-region = <&main_r5fss0_core1_dma_memory_region>,
 			<&main_r5fss0_core1_memory_region>;
 };
 
 &main_r5fss1_core0 {
-	mboxes = <&mailbox0_cluster2 &mbox_main_r5fss1_core0>;
+	mboxes = <&mailbox0_cluster2>, <&mbox_main_r5fss1_core0>;
 	memory-region = <&main_r5fss1_core0_dma_memory_region>,
 			<&main_r5fss1_core0_memory_region>;
 };
 
 &main_r5fss1_core1 {
-	mboxes = <&mailbox0_cluster2 &mbox_main_r5fss1_core1>;
+	mboxes = <&mailbox0_cluster2>, <&mbox_main_r5fss1_core1>;
 	memory-region = <&main_r5fss1_core1_dma_memory_region>,
 			<&main_r5fss1_core1_memory_region>;
 };
 
 &c66_0 {
-	mboxes = <&mailbox0_cluster3 &mbox_c66_0>;
+	mboxes = <&mailbox0_cluster3>, <&mbox_c66_0>;
 	memory-region = <&c66_0_dma_memory_region>,
 			<&c66_0_memory_region>;
 };
 
 &c66_1 {
-	mboxes = <&mailbox0_cluster3 &mbox_c66_1>;
+	mboxes = <&mailbox0_cluster3>, <&mbox_c66_1>;
 	memory-region = <&c66_1_dma_memory_region>,
 			<&c66_1_memory_region>;
 };
 
 &c71_0 {
-	mboxes = <&mailbox0_cluster4 &mbox_c71_0>;
+	mboxes = <&mailbox0_cluster4>, <&mbox_c71_0>;
 	memory-region = <&c71_0_dma_memory_region>,
 			<&c71_0_memory_region>;
 };