| 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 |
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 , > [...] >
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).
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.
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?
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 --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>; };
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(-)